Hi Jiewen, For #1 agree we should check for >0 and <9
For #2 - FSP Reset values 0x40000003 to 0x40000008 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 <[email protected]>; [email protected] Cc: Mudusuru, Giri P <[email protected]> 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 0x40000009 are not in scope. I suggest we should use range check. #define FSP_STATUS_RESET_REQUIRED_COLD 0x40000001 #define FSP_STATUS_RESET_REQUIRED_WARM 0x40000002 #define FSP_STATUS_RESET_REQUIRED_3 0x40000003 #define FSP_STATUS_RESET_REQUIRED_4 0x40000004 #define FSP_STATUS_RESET_REQUIRED_5 0x40000005 #define FSP_STATUS_RESET_REQUIRED_6 0x40000006 #define FSP_STATUS_RESET_REQUIRED_7 0x40000007 #define FSP_STATUS_RESET_REQUIRED_8 0x40000008 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: [email protected]<mailto:[email protected]> > Cc: Mudusuru, Giri P > <[email protected]<mailto:[email protected]>>; Yao, Jiewen > <[email protected]<mailto:[email protected]>> > 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 > <[email protected]<mailto:[email protected]>> > Cc: Jiewen Yao <[email protected]<mailto:[email protected]>> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Satya Yarlagadda > <[email protected]<mailto:[email protected]>> > --- > .../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 <Library/BaseMemoryLib.h> > #include <Library/UefiLib.h> > #include <Library/FspWrapperApiLib.h> > +#include <Library/FspWrapperPlatformLib.h> > #include <Library/PerformanceLib.h> > #include <Library/HobLib.h> > > @@ -93,6 +94,11 @@ OnPciEnumerationComplete ( > PERF_START_EX(&gFspApiPerformanceGuid, "EventRec", NULL, 0, > 0x6000); > Status = CallFspNotifyPhase (&NotifyPhaseParams); > PERF_END_EX(&gFspApiPerformanceGuid, "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(&gFspApiPerformanceGuid, "EventRec", NULL, 0, > 0x4000); > Status = CallFspNotifyPhase (&NotifyPhaseParams); > PERF_END_EX(&gFspApiPerformanceGuid, "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(&gFspApiPerformanceGuid, "EventRec", NULL, 0, > 0x2000); > Status = CallFspNotifyPhase (&NotifyPhaseParams); > PERF_END_EX(&gFspApiPerformanceGuid, "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 > DxeServicesLib > diff --git a/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c > b/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c > index 2eb3625..670636d 100644 > --- a/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c > +++ b/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c > @@ -88,6 +88,11 @@ PeiFspMemoryInit ( > PERF_START_EX(&gFspApiPerformanceGuid, "EventRec", NULL, > TimeStampCounterStart, 0xD000); > PERF_END_EX(&gFspApiPerformanceGuid, "EventRec", NULL, 0, > 0xD07F); > DEBUG ((DEBUG_INFO, "Total time spent executing > FspMemoryInitApi: %d millisecond\n", DivU64x32 (GetTimeInNanoSecond > (AsmReadTsc () - TimeStampCounterStart), 1000000))); > + // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED > status > + if ((Status & ~(0xF)) == (FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF))) { > + DEBUG ((DEBUG_INFO, "FspMemoryInitApi requested reset 0x%x\n", > Status)); > + CallFspWrapperResetSystem (Status); > + } > if (EFI_ERROR(Status)) { > DEBUG ((DEBUG_ERROR, "ERROR - Failed to execute > FspMemoryInitApi(), Status = %r\n", Status)); > } > diff --git a/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c > b/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c > index 9bc720f..32e0b63 100644 > --- a/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c > +++ b/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c > @@ -229,6 +229,11 @@ PeiMemoryDiscoveredNotify ( > Status = CallFspSiliconInit ((VOID *)FspsUpdDataPtr); > PERF_END_EX(&gFspApiPerformanceGuid, "EventRec", NULL, 0, 0x907F); > DEBUG ((DEBUG_INFO, "Total time spent executing FspSiliconInitApi: %d > millisecond\n", DivU64x32 (GetTimeInNanoSecond (AsmReadTsc () - > TimeStampCounterStart), 1000000))); > + // Reset the system if FSP API returned FSP_STATUS_RESET_REQUIRED > status > + if ((Status & ~(0xF)) == (FSP_STATUS_RESET_REQUIRED_COLD & ~(0xF))) { > + DEBUG ((DEBUG_INFO, "FspSiliconInitApi requested reset 0x%x\n", > Status)); > + CallFspWrapperResetSystem (Status); > + } > if (EFI_ERROR(Status)) { > DEBUG ((DEBUG_ERROR, "ERROR - Failed to execute > FspSiliconInitApi(), Status = %r\n", Status)); > } > diff --git a/IntelFsp2WrapperPkg/Include/Library/FspWrapperPlatformLib.h > b/IntelFsp2WrapperPkg/Include/Library/FspWrapperPlatformLib.h > index 30dc8df..44f975a 100644 > --- a/IntelFsp2WrapperPkg/Include/Library/FspWrapperPlatformLib.h > +++ b/IntelFsp2WrapperPkg/Include/Library/FspWrapperPlatformLib.h > @@ -70,4 +70,17 @@ GetS3MemoryInfo ( > OUT EFI_PHYSICAL_ADDRESS *S3PeiMemBase > ); > > +/** > + Perform platform related reset in FSP wrapper. > + > + @param[in] ResetType The type of reset the platform has to perform. > + > + @return Will reset the system with requested ResetType. > +**/ > +VOID > +EFIAPI > +CallFspWrapperResetSystem ( > + IN UINT32 ResetType > + ); > + > #endif > diff --git > a/IntelFsp2WrapperPkg/Library/BaseFspWrapperPlatformLibSample/FspWra > pperPlatformLibSample.c > b/IntelFsp2WrapperPkg/Library/BaseFspWrapperPlatformLibSample/FspWra > pperPlatformLibSample.c > index 9c1a84f..a5f8b99 100644 > --- > a/IntelFsp2WrapperPkg/Library/BaseFspWrapperPlatformLibSample/FspWra > pperPlatformLibSample.c > +++ > b/IntelFsp2WrapperPkg/Library/BaseFspWrapperPlatformLibSample/FspWra > pperPlatformLibSample.c > @@ -80,4 +80,20 @@ GetS3MemoryInfo ( > ) > { > return EFI_UNSUPPORTED; > -} > \ No newline at end of file > +} > + > +/** > + Perform platform related reset in FSP wrapper. > + > + @param[in] ResetType The type of reset the platform has to perform. > + > + @return Will reset the system with requested Reset type. > +**/ > +VOID > +EFIAPI > +CallFspWrapperResetSystem ( > + IN UINT32 ResetType > + ) > +{ > + return NULL; > +} > -- > 2.9.0.windows.1 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
