Re: [PATCH v2 28/28] Platform/NXP/LS1043aRdbPkg: Add PEI Phase


Pankaj Bansal
 

-----Original Message-----
From: Leif Lindholm <leif@nuviainc.com>
Sent: Monday, March 30, 2020 5:49 PM
To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Michael D Kinney
<michael.d.kinney@intel.com>; devel@edk2.groups.io; Varun Sethi
<V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
Mahmoud@arm.com>; Jon Nettleton <jon@solid-run.com>
Subject: Re: [PATCH v2 28/28] Platform/NXP/LS1043aRdbPkg: Add PEI Phase

On Fri, Mar 20, 2020 at 20:05:43 +0530, Pankaj Bansal wrote:
From: Pankaj Bansal <pankaj.bansal@nxp.com>

Add PEI phase to LS1043aRdb. This is needed becuase we need to have
dynamic PCDs support to be able to reserve memory before reporting
memory to UEFI fimrware.

Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc | 9 ---
Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.fdf | 18 +++--
.../MemoryInitPeiLib/MemoryInitPeiLib.c | 77 ++++++++++---------
.../MemoryInitPeiLib/MemoryInitPeiLib.inf | 3 +-
Silicon/NXP/NxpQoriqLs.dsc.inc | 59 ++++++++++----
5 files changed, 99 insertions(+), 67 deletions(-)

diff --git a/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc
b/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc
index d486c9b36fab..d45fd67c03b5 100644
--- a/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc
+++ b/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc
@@ -30,15 +30,6 @@ [LibraryClasses.common]
RealTimeClockLib|Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.inf

[PcdsFixedAtBuild.common]
-
- #
- # LS1043a board Specific PCDs
- # XX (DRAM - Region 1 2GB)
- # (NOR - IFC Region 1 512MB)
- gArmTokenSpaceGuid.PcdSystemMemoryBase|0x80000000
- gArmTokenSpaceGuid.PcdSystemMemorySize|0x7BE00000
-
gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x02000000
-
#
# RTC Pcds
#
diff --git a/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.fdf
b/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.fdf
index 99fbc87e1200..931d0bb14f9b 100644
--- a/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.fdf
+++ b/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.fdf
@@ -24,10 +24,10 @@

[FD.LS1043ARDB_EFI]
BaseAddress = 0x82000000|gArmTokenSpaceGuid.PcdFdBaseAddress #The
base address of the FLASH Device.
-Size = 0x000ED000|gArmTokenSpaceGuid.PcdFdSize #The size in
bytes of the FLASH Device
+Size = 0x00140000|gArmTokenSpaceGuid.PcdFdSize #The size in
bytes of the FLASH Device
ErasePolarity = 1
-BlockSize = 0x1
-NumBlocks = 0xED000
+BlockSize = 0x40000
+NumBlocks = 0x5

#################################################################
###############
#
@@ -44,7 +44,7 @@ [FD.LS1043ARDB_EFI]
# RegionType <FV, DATA, or FILE>
#
#################################################################
###############
-0x00000000|0x000ED000
+0x00000000|0x00140000
gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize
FV = FVMAIN_COMPACT

@@ -159,7 +159,15 @@ [FV.FVMAIN_COMPACT]
READ_LOCK_CAP = TRUE
READ_LOCK_STATUS = TRUE

- INF ArmPlatformPkg/PrePi/PeiUniCore.inf
+ INF ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
+ INF MdeModulePkg/Core/Pei/PeiMain.inf
+ INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
+ INF
MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
+ INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+ INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
+ INF ArmPkg/Drivers/CpuPei/CpuPei.inf
+ INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
+ INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf

FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF
PROCESSING_REQUIRED = TRUE {
diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
index 54d026ef1270..7fdf9cb77a6e 100644
--- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
+++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
@@ -46,30 +46,12 @@ InitMmu (
}
}

-/*++
-
-Routine Description:
-
-
-
-Arguments:
-
- FileHandle - Handle of the file being invoked.
- PeiServices - Describes the list of possible PEI Services.
-
-Returns:
-
- Status - EFI_SUCCESS if the boot mode could be set
-
---*/
The above line caused me an unexpected level of excitement this
morning, as my "put back the CRs SMTP strips out" script treated the
--- as a diff separator.

Now, I *have* seen the use of /*++ --*/ elsewhere in the tree, but
this syntax is *not* described in the coding style and should not be
used. While this is a delete statement, there is an addition below
using the same format. The doxygen tags to use are /** and **/.

Fortunately, I can't spot any of these in the rest of the set.

Please send an updated version of this patch - alone if it's the only
patch that needs changes, or with a v4 if such is required.
I have not received any comments on other patches so far.
Does that mean all patches are OK (except above)?
If that is the case, then I can send only this patch after update.
If some rework is needed for other patches as well, I will send this patch along with other reworked patches in v3.


EFI_STATUS
EFIAPI
-MemoryPeim (
- IN EFI_PHYSICAL_ADDRESS UefiMemoryBase,
- IN UINT64 UefiMemorySize
+MemoryInitPeiLibConstructor (
+ VOID
)
{
- ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
ARM_SMC_ARGS ArmSmcArgs;
INT32 Index;
UINTN DramSize;
@@ -82,18 +64,6 @@ MemoryPeim (
UINTN FdTop;
BOOLEAN FoundSystemMem;

- // Get Virtual Memory Map from the Platform Library
- ArmPlatformGetVirtualMemoryMap (&MemoryTable);
-
- //
- // Ensure MemoryTable[0].Length which is size of DRAM has been set
- // by ArmPlatformGetVirtualMemoryMap ()
- //
- ASSERT (MemoryTable[0].Length != 0);
-
- //
- // Now, the permanent memory has been installed, we can call
AllocatePages()
- //
ResourceAttributes = (
EFI_RESOURCE_ATTRIBUTE_PRESENT |
EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
@@ -133,8 +103,8 @@ MemoryPeim (

ASSERT (!DramSize);

- FdBase = (UINTN)FixedPcdGet64 (PcdFdBaseAddress);
- FdTop = FdBase + (UINTN)FixedPcdGet32 (PcdFdSize);
+ FdBase = (UINTN)PcdGet64 (PcdFdBaseAddress);
+ FdTop = FdBase + (UINTN)PcdGet32 (PcdFdSize);

// Declare memory regios to system
for (Index = MAX_DRAM_REGIONS - 1; Index >= 0; Index--) {
@@ -178,8 +148,8 @@ MemoryPeim (
);
};
// Mark the memory covering the Firmware Device as boot services data
- BuildMemoryAllocationHob (FixedPcdGet64 (PcdFdBaseAddress),
- FixedPcdGet32 (PcdFdSize),
+ BuildMemoryAllocationHob (PcdGet64 (PcdFdBaseAddress),
+ PcdGet32 (PcdFdSize),
EfiBootServicesData);
} else {
BuildResourceDescriptorHob (
@@ -199,16 +169,49 @@ MemoryPeim (
Top = DramRegions[Index].BaseAddress + DramRegions[Index].Size;

if (FdBase >= BaseAddress && FdTop <= Top) {
- Size -= (UINTN)FixedPcdGet32 (PcdFdSize);
+ Size -= (UINTN)PcdGet32 (PcdFdSize);
}

if (Size >= FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)) {
FoundSystemMem = TRUE;
+ PcdSet64S (PcdSystemMemoryBase, BaseAddress);
+ PcdSet64S (PcdSystemMemorySize, Size);
}
}

ASSERT (FoundSystemMem);

+ return EFI_SUCCESS;
+}
+
+/*++
Here is the incorrect addition.

(I'm not reviewing the set backwards, this was just the only patch
that wouldn't apply cleanly after conversion.)

/
Leif

+
+Routine Description:
+
+
+
+Arguments:
+
+ FileHandle - Handle of the file being invoked.
+ PeiServices - Describes the list of possible PEI Services.
+
+Returns:
+
+ Status - EFI_SUCCESS if the boot mode could be set
+
+--*/
+EFI_STATUS
+EFIAPI
+MemoryPeim (
+ IN EFI_PHYSICAL_ADDRESS UefiMemoryBase,
+ IN UINT64 UefiMemorySize
+ )
+{
+ ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
+
+ // Get Virtual Memory Map from the Platform Library
+ ArmPlatformGetVirtualMemoryMap (&MemoryTable);
+
// Build Memory Allocation Hob
InitMmu (MemoryTable);

diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
index ad2371115b17..a33f8cd3f743 100644
--- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
+++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
@@ -13,7 +13,8 @@ [Defines]
FILE_GUID = 55ddb6e0-70b5-11e0-b33e-0002a5d5c51b
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = MemoryInitPeiLib|SEC PEIM DXE_DRIVER
+ LIBRARY_CLASS = MemoryInitPeiLib|PEIM
+ CONSTRUCTOR = MemoryInitPeiLibConstructor

[Sources]
MemoryInitPeiLib.c
diff --git a/Silicon/NXP/NxpQoriqLs.dsc.inc b/Silicon/NXP/NxpQoriqLs.dsc.inc
index b2b10ce28a93..a3f18abb37b1 100644
--- a/Silicon/NXP/NxpQoriqLs.dsc.inc
+++ b/Silicon/NXP/NxpQoriqLs.dsc.inc
@@ -93,6 +93,7 @@ [LibraryClasses.common]
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
NonDiscoverableDeviceRegistrationLib|MdeModulePkg/Library/NonDiscoverabl
eDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeRep
ortStatusCodeLib.inf
+
UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecomp
ressLib.inf

I2cLib|Silicon/NXP/Library/I2cLib/I2cLib.inf
ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetS
ystemLib.inf
@@ -106,20 +107,24 @@ [LibraryClasses.common]

[LibraryClasses.common.SEC]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
-
UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecomp
ressLib.inf
-
ExtractGuidedSectionLib|EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/P
rePiExtractGuidedSectionLib.inf
-
LzmaDecompressLib|MdeModulePkg/Library/LzmaCustomDecompressLib/Lzma
CustomDecompressLib.inf
- PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
- HobLib|EmbeddedPkg/Library/PrePiHobLib/PrePiHobLib.inf
-
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiH
obListPointerLib.inf
-
MemoryAllocationLib|EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiM
emoryAllocationLib.inf
+
DebugAgentLib|ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAgentSymb
olsBaseLib.inf
+ HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
+ PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
+
PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLib/PeiServi
cesTablePointerLib.inf
+
MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllo
cationLib.inf
+
+[LibraryClasses.common.PEI_CORE]
+ PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
+ HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
+ PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
+
MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllo
cationLib.inf
+
PeiCoreEntryPoint|MdePkg/Library/PeiCoreEntryPoint/PeiCoreEntryPoint.inf
PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib
.inf
+
ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedSectionLib/PeiExtrac
tGuidedSectionLib.inf
+
ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiRepo
rtStatusCodeLib.inf
+
OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/
OemHookStatusCodeLibNull.inf

- # 1/123 faster than Stm or Vstm version
- BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
-
- # Uncomment to turn on GDB stub in SEC.
-
#DebugAgentLib|EmbeddedPkg/Library/GdbDebugAgent/GdbDebugAgent.inf
+
PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLib/PeiServi
cesTablePointerLib.inf

[LibraryClasses.common.PEIM]
PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
@@ -128,14 +133,16 @@ [LibraryClasses.common.PEIM]
PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLib/PeiServi
cesTablePointerLib.inf
HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllo
cationLib.inf
+
PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib
.inf
+
ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedSectionLib/PeiExtrac
tGuidedSectionLib.inf
ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiRepo
rtStatusCodeLib.inf
+
OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/
OemHookStatusCodeLibNull.inf

[LibraryClasses.common.DXE_CORE]
HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
MemoryAllocationLib|MdeModulePkg/Library/DxeCoreMemoryAllocationLib/D
xeCoreMemoryAllocationLib.inf
DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtra
ctGuidedSectionLib.inf
-
UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecomp
ressLib.inf
DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
PerformanceLib|MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerf
ormanceLib.inf

@@ -207,6 +214,9 @@ [PcdsDynamicDefault.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution|640
gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution|480

+ gArmTokenSpaceGuid.PcdSystemMemoryBase|0
+ gArmTokenSpaceGuid.PcdSystemMemorySize|0
+
[PcdsDynamicHii.common.DEFAULT]
gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalV
ariableGuid|0x0|10

@@ -227,6 +237,12 @@ [PcdsFixedAtBuild.common]
gEfiMdePkgTokenSpaceGuid.PcdPostCodePropertyMask|0
gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|320

+ ## Base of DRAM
+ ## since TFA puts Fd at 0x2000000 offset from DRAM base, we can use this
space
+ ## for temporary ram
+ gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x80000000
+
gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x02000000
+
!if $(TARGET) == RELEASE
gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x27
gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x81000001
@@ -284,13 +300,26 @@ [PcdsFixedAtBuild.common]
#################################################################
###############
[Components.common]
#
- # SEC
+ # PEI Phase modules
#
- ArmPlatformPkg/PrePi/PeiUniCore.inf
+ ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
+
+ MdeModulePkg/Core/Pei/PeiMain.inf
MdeModulePkg/Universal/PCD/Pei/Pcd.inf {
<LibraryClasses>
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
}
+ MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
+ MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+
+ ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
+ ArmPkg/Drivers/CpuPei/CpuPei.inf
+ ArmPlatformPkg/PlatformPei/PlatformPeim.inf
+
+ MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf {
+ <LibraryClasses>
+
NULL|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecom
pressLib.inf
+ }

#
# DXE
--
2.17.1

Join devel@edk2.groups.io to automatically receive all group messages.