> -----Original Message-----
> From: Leif Lindholm <l...@nuviainc.com>
> Sent: Monday, March 30, 2020 5:49 PM
> To: Pankaj Bansal (OSS) <pankaj.ban...@oss.nxp.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; Michael D Kinney
> <michael.d.kin...@intel.com>; devel@edk2.groups.io; Varun Sethi
> <v.se...@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> mahm...@arm.com>; Jon Nettleton <j...@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.ban...@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.ban...@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
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56734): https://edk2.groups.io/g/devel/message/56734
Mute This Topic: https://groups.io/mt/72077462/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
