Re: [edk2] [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired return Status from FSP.

2016-06-16 Thread Mudusuru, Giri P 
Thanks Jiewen. Looks good to me.

Reviewed-by: Giri P Mudusuru 


-Original Message-
From: Yao, Jiewen 
Sent: Thursday, June 16, 2016 7:40 PM
To: edk2-devel@lists.01.org
Cc: Mudusuru, Giri P ; Chan, Amy 
; Yarlagadda, Satya P 
Subject: [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired 
return Status from FSP.

As per FSP 2.0 spec, FSP shall not trigger system reset and instead it shall 
return from the FSP API to the BL/Wrapper with the required reset type. The 
changes are to handle the ResetRequired return code from FSP APIs and provide 
lib interface for platform to trigger the actual reset.

Cc: Giri P Mudusuru 
Cc: Amy Chan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Satya Yarlagadda 
Signed-off-by: Jiewen Yao 
---
 .../FspWrapperNotifyDxe/FspWrapperNotifyDxe.c  | 28 ++
 .../FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf|  1 +
 .../FspmWrapperPeim/FspmWrapperPeim.c  |  9 ++
 .../FspsWrapperPeim/FspsWrapperPeim.c  | 33 ++
 .../Include/Library/FspWrapperPlatformLib.h| 13 +
 .../FspWrapperPlatformLibSample.c  | 22 ++-
 6 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c 
b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
index 30c06b8..0797f44 100644
--- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
+++ b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -93,6 +94,15 @@ OnPciEnumerationComplete (
   PERF_START_EX(, "EventRec", NULL, 0, 0x6000);
   Status = CallFspNotifyPhase ();
   PERF_END_EX(, "EventRec", NULL, 0, 0x607F);
+
+  //
+  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED 
+ status  //  if ((Status >= FSP_STATUS_RESET_REQUIRED_COLD) && (Status 
+ <= FSP_STATUS_RESET_REQUIRED_8)) {
+DEBUG((DEBUG_INFO, "FSP NotifyPhase AfterPciEnumeration requested reset 
0x%x\n", Status));
+CallFspWrapperResetSystem ((UINT32)Status);  }
+
   if (Status != EFI_SUCCESS) {
 DEBUG((DEBUG_ERROR, "FSP NotifyPhase AfterPciEnumeration failed, status: 
0x%x\n", Status));
   } else {
@@ -130,6 +140,15 @@ OnReadyToBoot (
   PERF_START_EX(, "EventRec", NULL, 0, 0x4000);
   Status = CallFspNotifyPhase ();
   PERF_END_EX(, "EventRec", NULL, 0, 0x407F);
+
+  //
+  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED 
+ status  //  if ((Status >= FSP_STATUS_RESET_REQUIRED_COLD) && (Status 
+ <= FSP_STATUS_RESET_REQUIRED_8)) {
+DEBUG((DEBUG_INFO, "FSP NotifyPhase ReadyToBoot requested reset 0x%x\n", 
Status));
+CallFspWrapperResetSystem ((UINT32)Status);  }
+
   if (Status != EFI_SUCCESS) {
 DEBUG((DEBUG_ERROR, "FSP NotifyPhase ReadyToBoot failed, status: 0x%x\n", 
Status));
   } else {
@@ -179,6 +198,15 @@ OnEndOfFirmware (
   PERF_START_EX(, "EventRec", NULL, 0, 0x2000);
   Status = CallFspNotifyPhase ();
   PERF_END_EX(, "EventRec", NULL, 0, 0x207F);
+
+  //
+  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED 
+ status  //  if ((Status >= FSP_STATUS_RESET_REQUIRED_COLD) && (Status 
+ <= FSP_STATUS_RESET_REQUIRED_8)) {
+DEBUG((DEBUG_INFO, "FSP NotifyPhase EndOfFirmware requested reset 0x%x\n", 
Status));
+CallFspWrapperResetSystem ((UINT32)Status);  }
+
   if (Status != EFI_SUCCESS) {
 DEBUG((DEBUG_ERROR, "FSP NotifyPhase EndOfFirmware failed, status: 
0x%x\n", Status));
   } else {
diff --git a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf 
b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf
index d8af0aa..f851f68 100644
--- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf
+++ b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf
@@ -49,6 +49,7 @@
   DxeServicesLib
   PerformanceLib
   HobLib
+  FspWrapperPlatformLib
 
 [Protocols]
   gEfiPciEnumerationCompleteProtocolGuid## CONSUMES
diff --git a/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c 
b/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c
index 2eb3625..6144ad7 100644
--- a/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c
+++ b/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c
@@ -88,6 +88,15 @@ PeiFspMemoryInit (
   PERF_START_EX(, "EventRec", NULL, 
TimeStampCounterStart, 0xD000);
   PERF_END_EX(, "EventRec", NULL, 0, 0xD07F);
   DEBUG ((DEBUG_INFO, "Total time spent executing FspMemoryInitApi: %d 
millisecond\n", DivU64x32 (GetTimeInNanoSecond (AsmReadTsc () - 
TimeStampCounterStart), 100)));
+
+  //
+  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED 
+ status  //  if ((Status >=

[edk2] [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired return Status from FSP.

2016-06-16 Thread Jiewen Yao 
As per FSP 2.0 spec, FSP shall not trigger system reset and instead it
shall return from the FSP API to the BL/Wrapper with the required reset
type. The changes are to handle the ResetRequired return code from FSP
APIs and provide lib interface for platform to trigger the actual reset.

Cc: Giri P Mudusuru 
Cc: Amy Chan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Satya Yarlagadda 
Signed-off-by: Jiewen Yao 
---
 .../FspWrapperNotifyDxe/FspWrapperNotifyDxe.c  | 28 ++
 .../FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf|  1 +
 .../FspmWrapperPeim/FspmWrapperPeim.c  |  9 ++
 .../FspsWrapperPeim/FspsWrapperPeim.c  | 33 ++
 .../Include/Library/FspWrapperPlatformLib.h| 13 +
 .../FspWrapperPlatformLibSample.c  | 22 ++-
 6 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c 
b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
index 30c06b8..0797f44 100644
--- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
+++ b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -93,6 +94,15 @@ OnPciEnumerationComplete (
   PERF_START_EX(, "EventRec", NULL, 0, 0x6000);
   Status = CallFspNotifyPhase ();
   PERF_END_EX(, "EventRec", NULL, 0, 0x607F);
+
+  //
+  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED status
+  //
+  if ((Status >= FSP_STATUS_RESET_REQUIRED_COLD) && (Status <= 
FSP_STATUS_RESET_REQUIRED_8)) {
+DEBUG((DEBUG_INFO, "FSP NotifyPhase AfterPciEnumeration requested reset 
0x%x\n", Status));
+CallFspWrapperResetSystem ((UINT32)Status);
+  }
+
   if (Status != EFI_SUCCESS) {
 DEBUG((DEBUG_ERROR, "FSP NotifyPhase AfterPciEnumeration failed, status: 
0x%x\n", Status));
   } else {
@@ -130,6 +140,15 @@ OnReadyToBoot (
   PERF_START_EX(, "EventRec", NULL, 0, 0x4000);
   Status = CallFspNotifyPhase ();
   PERF_END_EX(, "EventRec", NULL, 0, 0x407F);
+
+  //
+  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED status
+  //
+  if ((Status >= FSP_STATUS_RESET_REQUIRED_COLD) && (Status <= 
FSP_STATUS_RESET_REQUIRED_8)) {
+DEBUG((DEBUG_INFO, "FSP NotifyPhase ReadyToBoot requested reset 0x%x\n", 
Status));
+CallFspWrapperResetSystem ((UINT32)Status);
+  }
+
   if (Status != EFI_SUCCESS) {
 DEBUG((DEBUG_ERROR, "FSP NotifyPhase ReadyToBoot failed, status: 0x%x\n", 
Status));
   } else {
@@ -179,6 +198,15 @@ OnEndOfFirmware (
   PERF_START_EX(, "EventRec", NULL, 0, 0x2000);
   Status = CallFspNotifyPhase ();
   PERF_END_EX(, "EventRec", NULL, 0, 0x207F);
+
+  //
+  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED status
+  //
+  if ((Status >= FSP_STATUS_RESET_REQUIRED_COLD) && (Status <= 
FSP_STATUS_RESET_REQUIRED_8)) {
+DEBUG((DEBUG_INFO, "FSP NotifyPhase EndOfFirmware requested reset 0x%x\n", 
Status));
+CallFspWrapperResetSystem ((UINT32)Status);
+  }
+
   if (Status != EFI_SUCCESS) {
 DEBUG((DEBUG_ERROR, "FSP NotifyPhase EndOfFirmware failed, status: 
0x%x\n", Status));
   } else {
diff --git a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf 
b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf
index d8af0aa..f851f68 100644
--- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf
+++ b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf
@@ -49,6 +49,7 @@
   DxeServicesLib
   PerformanceLib
   HobLib
+  FspWrapperPlatformLib
 
 [Protocols]
   gEfiPciEnumerationCompleteProtocolGuid## CONSUMES
diff --git a/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c 
b/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c
index 2eb3625..6144ad7 100644
--- a/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c
+++ b/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c
@@ -88,6 +88,15 @@ PeiFspMemoryInit (
   PERF_START_EX(, "EventRec", NULL, 
TimeStampCounterStart, 0xD000);
   PERF_END_EX(, "EventRec", NULL, 0, 0xD07F);
   DEBUG ((DEBUG_INFO, "Total time spent executing FspMemoryInitApi: %d 
millisecond\n", DivU64x32 (GetTimeInNanoSecond (AsmReadTsc () - 
TimeStampCounterStart), 100)));
+
+  //
+  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED status
+  //
+  if ((Status >= FSP_STATUS_RESET_REQUIRED_COLD) && (Status <= 
FSP_STATUS_RESET_REQUIRED_8)) {
+DEBUG((DEBUG_INFO, "FspMemoryInitApi requested reset 0x%x\n", Status));
+CallFspWrapperResetSystem ((UINT32)Status);
+  }
+
   if (EFI_ERROR(Status)) {
 DEBUG ((DEBUG_ERROR, "ERROR - Failed to execute FspMemoryInitApi(), Status 
= %r\n", Status));
   }
diff --git a/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c

Re: [edk2] [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired return Status from FSP

2016-06-16 Thread Mudusuru, Giri P 
Agree with your suggestion Jiewen.

Thanks,
-Giri

From: Yao, Jiewen
Sent: Thursday, June 16, 2016 3:42 PM
To: Mudusuru, Giri P ; Yarlagadda, Satya P 
; edk2-devel@lists.01.org
Cc: Yao, Jiewen 
Subject: RE: [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired 
return Status from FSP

I see. That makes sense.

How about we change "ResetType" to "FspStatusResetType" - using the full name 
to avoid confusing and align with spec?

VOID
EFIAPI
CallFspWrapperResetSystem (
  IN UINT32ResetType
  );

Thank you
Yao Jiewen

From: Mudusuru, Giri P
Sent: Friday, June 17, 2016 6:34 AM
To: Yao, Jiewen >; 
Yarlagadda, Satya P 
>; 
edk2-devel@lists.01.org
Cc: Mudusuru, Giri P 
>
Subject: RE: [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired 
return Status from FSP

Hi Jiewen,

For #1 agree we should check for >0 and <9

For #2 - FSP Reset values 0x4003 to 0x4008 are silicon specific which 
will map to Reset PPI/Protocol platform GUID types.
This solution will enable platform to map them before triggering the Reset 
PPI/Protocols.

Thanks,
-Giri

From: Yao, Jiewen
Sent: Thursday, June 16, 2016 5:04 AM
To: Yarlagadda, Satya P 
>; 
edk2-devel@lists.01.org
Cc: Mudusuru, Giri P 
>
Subject: RE: [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired 
return Status from FSP

HI
1) I have concern on using " if ((Status & ~(0xF)) == 
(FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF)))"
According to FSP spec, only below reset status are defined. It means 0x4009 
are not in scope.
I suggest we should use range check.

#define FSP_STATUS_RESET_REQUIRED_COLD 0x4001
#define FSP_STATUS_RESET_REQUIRED_WARM 0x4002
#define FSP_STATUS_RESET_REQUIRED_30x4003
#define FSP_STATUS_RESET_REQUIRED_40x4004
#define FSP_STATUS_RESET_REQUIRED_50x4005
#define FSP_STATUS_RESET_REQUIRED_60x4006
#define FSP_STATUS_RESET_REQUIRED_70x4007
#define FSP_STATUS_RESET_REQUIRED_80x4008

2) I am curious why we need introduce CallFspWrapperResetSystem, Can we just 
use ResetPpi in PEI phase, and gRT->ResetSystem in DXE phase?

Thank you
Yao Jiewen

> -Original Message-
> From: Yarlagadda, Satya P
> Sent: Thursday, June 16, 2016 7:15 PM
> To: edk2-devel@lists.01.org
> Cc: Mudusuru, Giri P 
> >; Yao, Jiewen
> >
> Subject: [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired
> return Status from FSP
>
> As per FSP 2.0 spec, FSP shall not trigger system reset and instead it
> shall return from the FSP API to the BL/Wrapper with the required reset
> type. The changes are to handle the ResetRequired return code from FSP
> APIs and provide lib interface for platform to trigger the actual reset.
>
> Cc: Giri P Mudusuru 
> >
> Cc: Jiewen Yao >
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Satya Yarlagadda 
> >
> ---
>  .../FspWrapperNotifyDxe/FspWrapperNotifyDxe.c  | 16
> 
>  .../FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf|  1 +
>  IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c  |  5
> +
>  IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c  |  5 +
>  .../Include/Library/FspWrapperPlatformLib.h| 13
> +
>  .../FspWrapperPlatformLibSample.c  | 18
> +-
>  6 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git
> a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> index 30c06b8..bc52221 100644
> --- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> +++ b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -93,6 +94,11 @@ OnPciEnumerationComplete (
>PERF_START_EX(, "EventRec", NULL, 0,
> 0x6000);
>Status = CallFspNotifyPhase ();
>PERF_END_EX(, "EventRec", NULL, 0, 0x607F);
> +  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED
> status
> +  if ((Status & ~(0xF)) == (FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF))) {
> +DEBUG ((DEBUG_INFO, "FSP NotifyPhase AfterPciEnumeration

Re: [edk2] [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired return Status from FSP

2016-06-16 Thread Yao, Jiewen 
I see. That makes sense.

How about we change "ResetType" to "FspStatusResetType" - using the full name 
to avoid confusing and align with spec?

VOID
EFIAPI
CallFspWrapperResetSystem (
  IN UINT32ResetType
  );

Thank you
Yao Jiewen

From: Mudusuru, Giri P
Sent: Friday, June 17, 2016 6:34 AM
To: Yao, Jiewen ; Yarlagadda, Satya P 
; edk2-devel@lists.01.org
Cc: Mudusuru, Giri P 
Subject: RE: [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired 
return Status from FSP

Hi Jiewen,

For #1 agree we should check for >0 and <9

For #2 - FSP Reset values 0x4003 to 0x4008 are silicon specific which 
will map to Reset PPI/Protocol platform GUID types.
This solution will enable platform to map them before triggering the Reset 
PPI/Protocols.

Thanks,
-Giri

From: Yao, Jiewen
Sent: Thursday, June 16, 2016 5:04 AM
To: Yarlagadda, Satya P 
>; 
edk2-devel@lists.01.org
Cc: Mudusuru, Giri P 
>
Subject: RE: [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired 
return Status from FSP

HI
1) I have concern on using " if ((Status & ~(0xF)) == 
(FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF)))"
According to FSP spec, only below reset status are defined. It means 0x4009 
are not in scope.
I suggest we should use range check.

#define FSP_STATUS_RESET_REQUIRED_COLD 0x4001
#define FSP_STATUS_RESET_REQUIRED_WARM 0x4002
#define FSP_STATUS_RESET_REQUIRED_30x4003
#define FSP_STATUS_RESET_REQUIRED_40x4004
#define FSP_STATUS_RESET_REQUIRED_50x4005
#define FSP_STATUS_RESET_REQUIRED_60x4006
#define FSP_STATUS_RESET_REQUIRED_70x4007
#define FSP_STATUS_RESET_REQUIRED_80x4008

2) I am curious why we need introduce CallFspWrapperResetSystem, Can we just 
use ResetPpi in PEI phase, and gRT->ResetSystem in DXE phase?

Thank you
Yao Jiewen

> -Original Message-
> From: Yarlagadda, Satya P
> Sent: Thursday, June 16, 2016 7:15 PM
> To: edk2-devel@lists.01.org
> Cc: Mudusuru, Giri P 
> >; Yao, Jiewen
> >
> Subject: [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired
> return Status from FSP
>
> As per FSP 2.0 spec, FSP shall not trigger system reset and instead it
> shall return from the FSP API to the BL/Wrapper with the required reset
> type. The changes are to handle the ResetRequired return code from FSP
> APIs and provide lib interface for platform to trigger the actual reset.
>
> Cc: Giri P Mudusuru 
> >
> Cc: Jiewen Yao >
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Satya Yarlagadda 
> >
> ---
>  .../FspWrapperNotifyDxe/FspWrapperNotifyDxe.c  | 16
> 
>  .../FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf|  1 +
>  IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c  |  5
> +
>  IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c  |  5 +
>  .../Include/Library/FspWrapperPlatformLib.h| 13
> +
>  .../FspWrapperPlatformLibSample.c  | 18
> +-
>  6 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git
> a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> index 30c06b8..bc52221 100644
> --- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> +++ b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -93,6 +94,11 @@ OnPciEnumerationComplete (
>PERF_START_EX(, "EventRec", NULL, 0,
> 0x6000);
>Status = CallFspNotifyPhase ();
>PERF_END_EX(, "EventRec", NULL, 0, 0x607F);
> +  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED
> status
> +  if ((Status & ~(0xF)) == (FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF))) {
> +DEBUG ((DEBUG_INFO, "FSP NotifyPhase AfterPciEnumeration
> requested reset 0x%x\n", Status));
> +CallFspWrapperResetSystem ((UINT32)Status);
> +  }
>if (Status != EFI_SUCCESS) {
>  DEBUG((DEBUG_ERROR, "FSP NotifyPhase AfterPciEnumeration failed,
> status: 0x%x\n", Status));
>} else {
> @@ -130,6 +136,11 @@ OnReadyToBoot (
>PERF_START_EX(, "EventRec", NULL, 0,
> 0x4000);
>Status = CallFspNotifyPhase ();
>PERF_END_EX(, "EventRec", NULL, 0, 0x407F);
> +  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED
> status
> +  if

Re: [edk2] [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired return Status from FSP

2016-06-16 Thread Mudusuru, Giri P 
Hi Jiewen,

For #1 agree we should check for >0 and <9

For #2 - FSP Reset values 0x4003 to 0x4008 are silicon specific which 
will map to Reset PPI/Protocol platform GUID types.
This solution will enable platform to map them before triggering the Reset 
PPI/Protocols.

Thanks,
-Giri

From: Yao, Jiewen
Sent: Thursday, June 16, 2016 5:04 AM
To: Yarlagadda, Satya P ; edk2-devel@lists.01.org
Cc: Mudusuru, Giri P 
Subject: RE: [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired 
return Status from FSP

HI
1) I have concern on using " if ((Status & ~(0xF)) == 
(FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF)))"
According to FSP spec, only below reset status are defined. It means 0x4009 
are not in scope.
I suggest we should use range check.

#define FSP_STATUS_RESET_REQUIRED_COLD 0x4001
#define FSP_STATUS_RESET_REQUIRED_WARM 0x4002
#define FSP_STATUS_RESET_REQUIRED_30x4003
#define FSP_STATUS_RESET_REQUIRED_40x4004
#define FSP_STATUS_RESET_REQUIRED_50x4005
#define FSP_STATUS_RESET_REQUIRED_60x4006
#define FSP_STATUS_RESET_REQUIRED_70x4007
#define FSP_STATUS_RESET_REQUIRED_80x4008

2) I am curious why we need introduce CallFspWrapperResetSystem, Can we just 
use ResetPpi in PEI phase, and gRT->ResetSystem in DXE phase?

Thank you
Yao Jiewen

> -Original Message-
> From: Yarlagadda, Satya P
> Sent: Thursday, June 16, 2016 7:15 PM
> To: edk2-devel@lists.01.org
> Cc: Mudusuru, Giri P 
> >; Yao, Jiewen
> >
> Subject: [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired
> return Status from FSP
>
> As per FSP 2.0 spec, FSP shall not trigger system reset and instead it
> shall return from the FSP API to the BL/Wrapper with the required reset
> type. The changes are to handle the ResetRequired return code from FSP
> APIs and provide lib interface for platform to trigger the actual reset.
>
> Cc: Giri P Mudusuru 
> >
> Cc: Jiewen Yao >
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Satya Yarlagadda 
> >
> ---
>  .../FspWrapperNotifyDxe/FspWrapperNotifyDxe.c  | 16
> 
>  .../FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf|  1 +
>  IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c  |  5
> +
>  IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c  |  5 +
>  .../Include/Library/FspWrapperPlatformLib.h| 13
> +
>  .../FspWrapperPlatformLibSample.c  | 18
> +-
>  6 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git
> a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> index 30c06b8..bc52221 100644
> --- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> +++ b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -93,6 +94,11 @@ OnPciEnumerationComplete (
>PERF_START_EX(, "EventRec", NULL, 0,
> 0x6000);
>Status = CallFspNotifyPhase ();
>PERF_END_EX(, "EventRec", NULL, 0, 0x607F);
> +  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED
> status
> +  if ((Status & ~(0xF)) == (FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF))) {
> +DEBUG ((DEBUG_INFO, "FSP NotifyPhase AfterPciEnumeration
> requested reset 0x%x\n", Status));
> +CallFspWrapperResetSystem ((UINT32)Status);
> +  }
>if (Status != EFI_SUCCESS) {
>  DEBUG((DEBUG_ERROR, "FSP NotifyPhase AfterPciEnumeration failed,
> status: 0x%x\n", Status));
>} else {
> @@ -130,6 +136,11 @@ OnReadyToBoot (
>PERF_START_EX(, "EventRec", NULL, 0,
> 0x4000);
>Status = CallFspNotifyPhase ();
>PERF_END_EX(, "EventRec", NULL, 0, 0x407F);
> +  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED
> status
> +  if ((Status & ~(0xF)) == (FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF))) {
> +DEBUG ((DEBUG_INFO, "FSP NotifyPhase ReadyToBoot requested
> reset 0x%x\n", Status));
> +CallFspWrapperResetSystem ((UINT32)Status);
> +  }
>if (Status != EFI_SUCCESS) {
>  DEBUG((DEBUG_ERROR, "FSP NotifyPhase ReadyToBoot failed, status:
> 0x%x\n", Status));
>} else {
> @@ -179,6 +190,11 @@ OnEndOfFirmware (
>PERF_START_EX(, "EventRec", NULL, 0,
> 0x2000);
>Status = CallFspNotifyPhase ();
>PERF_END_EX(, "EventRec", NULL, 0, 0x207F);
> +  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED
> status
> +  if ((Status & ~(0xF)) ==

Re: [edk2] [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired return Status from FSP

2016-06-16 Thread Yao, Jiewen 
HI
1) I have concern on using " if ((Status & ~(0xF)) == 
(FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF)))"
According to FSP spec, only below reset status are defined. It means 0x4009 
are not in scope.
I suggest we should use range check.

#define FSP_STATUS_RESET_REQUIRED_COLD 0x4001
#define FSP_STATUS_RESET_REQUIRED_WARM 0x4002
#define FSP_STATUS_RESET_REQUIRED_30x4003
#define FSP_STATUS_RESET_REQUIRED_40x4004
#define FSP_STATUS_RESET_REQUIRED_50x4005
#define FSP_STATUS_RESET_REQUIRED_60x4006
#define FSP_STATUS_RESET_REQUIRED_70x4007
#define FSP_STATUS_RESET_REQUIRED_80x4008

2) I am curious why we need introduce CallFspWrapperResetSystem, Can we just 
use ResetPpi in PEI phase, and gRT->ResetSystem in DXE phase?

Thank you
Yao Jiewen

> -Original Message-
> From: Yarlagadda, Satya P
> Sent: Thursday, June 16, 2016 7:15 PM
> To: edk2-devel@lists.01.org
> Cc: Mudusuru, Giri P ; Yao, Jiewen
> 
> Subject: [PATCH] IntelFsp2WrapperPkg: Add support to handle ResetRequired
> return Status from FSP
>
> As per FSP 2.0 spec, FSP shall not trigger system reset and instead it
> shall return from the FSP API to the BL/Wrapper with the required reset
> type. The changes are to handle the ResetRequired return code from FSP
> APIs and provide lib interface for platform to trigger the actual reset.
>
> Cc: Giri P Mudusuru 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Satya Yarlagadda 
> ---
>  .../FspWrapperNotifyDxe/FspWrapperNotifyDxe.c  | 16
> 
>  .../FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf|  1 +
>  IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c  |  5
> +
>  IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c  |  5 +
>  .../Include/Library/FspWrapperPlatformLib.h| 13
> +
>  .../FspWrapperPlatformLibSample.c  | 18
> +-
>  6 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git
> a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> index 30c06b8..bc52221 100644
> --- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> +++ b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -93,6 +94,11 @@ OnPciEnumerationComplete (
>PERF_START_EX(, "EventRec", NULL, 0,
> 0x6000);
>Status = CallFspNotifyPhase ();
>PERF_END_EX(, "EventRec", NULL, 0, 0x607F);
> +  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED
> status
> +  if ((Status & ~(0xF)) == (FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF))) {
> +DEBUG ((DEBUG_INFO, "FSP NotifyPhase AfterPciEnumeration
> requested reset 0x%x\n", Status));
> +CallFspWrapperResetSystem ((UINT32)Status);
> +  }
>if (Status != EFI_SUCCESS) {
>  DEBUG((DEBUG_ERROR, "FSP NotifyPhase AfterPciEnumeration failed,
> status: 0x%x\n", Status));
>} else {
> @@ -130,6 +136,11 @@ OnReadyToBoot (
>PERF_START_EX(, "EventRec", NULL, 0,
> 0x4000);
>Status = CallFspNotifyPhase ();
>PERF_END_EX(, "EventRec", NULL, 0, 0x407F);
> +  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED
> status
> +  if ((Status & ~(0xF)) == (FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF))) {
> +DEBUG ((DEBUG_INFO, "FSP NotifyPhase ReadyToBoot requested
> reset 0x%x\n", Status));
> +CallFspWrapperResetSystem ((UINT32)Status);
> +  }
>if (Status != EFI_SUCCESS) {
>  DEBUG((DEBUG_ERROR, "FSP NotifyPhase ReadyToBoot failed, status:
> 0x%x\n", Status));
>} else {
> @@ -179,6 +190,11 @@ OnEndOfFirmware (
>PERF_START_EX(, "EventRec", NULL, 0,
> 0x2000);
>Status = CallFspNotifyPhase ();
>PERF_END_EX(, "EventRec", NULL, 0, 0x207F);
> +  // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED
> status
> +  if ((Status & ~(0xF)) == (FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF))) {
> +DEBUG ((DEBUG_INFO, "FSP NotifyPhase EndOfFirmware requested
> reset 0x%x\n", Status));
> +CallFspWrapperResetSystem ((UINT32)Status);
> +  }
>if (Status != EFI_SUCCESS) {
>  DEBUG((DEBUG_ERROR, "FSP NotifyPhase EndOfFirmware failed,
> status: 0x%x\n", Status));
>} else {
> diff --git
> a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf
> b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf
> index d8af0aa..f6cf4ad 100644
> --- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf
> +++
> b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/FspWrapperNotifyDxe.inf
> @@ -44,6 +44,7 @@
>BaseMemoryLib
>UefiLib
>FspWrapperApiLib
> +  FspWrapperPlatformLib
>PeCoffLib
>CacheMaintenanceLib
>