On Fri, Mar 20, 2020 at 20:05:30 +0530, Pankaj Bansal wrote:
> From: Pankaj Bansal <pankaj.ban...@nxp.com>
>
> RAM retrieval using SMC commands is common to all Layerscape SOCs.
> Therefore, move it to commom MemoryInit Pei Lib.
>
> Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
> ---
> Silicon/NXP/Include/DramInfo.h | 38 -----
> .../Library/MemoryInitPei/MemoryInitPeiLib.c | 137 ++++++++++++++----
> .../Library/MemoryInitPei/MemoryInitPeiLib.h | 25 ++++
> .../MemoryInitPei/MemoryInitPeiLib.inf | 7 +-
> Silicon/NXP/Library/SocLib/Chassis.c | 67 ---------
> Silicon/NXP/Library/SocLib/LS1043aSocLib.inf | 1 -
> 6 files changed, 140 insertions(+), 135 deletions(-)
> delete mode 100644 Silicon/NXP/Include/DramInfo.h
> create mode 100644 Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h
>
> diff --git a/Silicon/NXP/Include/DramInfo.h b/Silicon/NXP/Include/DramInfo.h
> deleted file mode 100644
> index a934aaeff1f5..000000000000
> --- a/Silicon/NXP/Include/DramInfo.h
> +++ /dev/null
> @@ -1,38 +0,0 @@
> -/** @file
> -* Header defining the structure for Dram Information
> -*
> -* Copyright 2019 NXP
> -*
> -* SPDX-License-Identifier: BSD-2-Clause-Patent
> -*
> -**/
> -
> -#ifndef DRAM_INFO_H_
> -#define DRAM_INFO_H_
> -
> -#include <Uefi/UefiBaseType.h>
> -
> -#define SMC_DRAM_BANK_INFO (0xC200FF12)
> -
> -typedef struct {
> - UINTN BaseAddress;
> - UINTN Size;
> -} DRAM_REGION_INFO;
> -
> -typedef struct {
> - UINT32 NumOfDrams;
> - UINT32 Reserved;
> - DRAM_REGION_INFO DramRegion[3];
> -} DRAM_INFO;
> -
> -EFI_STATUS
> -GetDramBankInfo (
> - IN OUT DRAM_INFO *DramInfo
> - );
> -
> -VOID
> -UpdateDpaaDram (
> - IN OUT DRAM_INFO *DramInfo
> - );
> -
> -#endif /* DRAM_INFO_H_ */
> diff --git a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c
> b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c
> index 3ea773678667..54d026ef1270 100644
> --- a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c
> +++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c
> @@ -17,8 +17,10 @@
> #include <Library/HobLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PcdLib.h>
> +#include <Library/ArmSmcLib.h>
Please insert alpabetically sorted.
> +
> +#include "MemoryInitPeiLib.h"
>
> -#include <DramInfo.h>
>
> VOID
> BuildMemoryTypeInformationHob (
> @@ -68,10 +70,17 @@ MemoryPeim (
> )
> {
> ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
> + ARM_SMC_ARGS ArmSmcArgs;
> + INT32 Index;
> + UINTN DramSize;
> + UINTN BaseAddress;
> + UINTN Size;
> + UINTN Top;
> + DRAM_REGION_INFO DramRegions[MAX_DRAM_REGIONS];
> EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
> - EFI_PEI_HOB_POINTERS NextHob;
> - BOOLEAN Found;
> - DRAM_INFO DramInfo;
> + UINTN FdBase;
> + UINTN FdTop;
> + BOOLEAN FoundSystemMem;
>
> // Get Virtual Memory Map from the Platform Library
> ArmPlatformGetVirtualMemoryMap (&MemoryTable);
> @@ -94,40 +103,112 @@ MemoryPeim (
> EFI_RESOURCE_ATTRIBUTE_TESTED
> );
>
> - if (GetDramBankInfo (&DramInfo)) {
> - DEBUG ((DEBUG_ERROR, "Failed to get DRAM information, exiting...\n"));
> - return EFI_UNSUPPORTED;
> - }
> -
> - while (DramInfo.NumOfDrams--) {
> - //
> - // Check if the resource for the main system memory has been declared
> - //
> - Found = FALSE;
> - NextHob.Raw = GetHobList ();
> - while ((NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
> NextHob.Raw)) != NULL) {
> - if ((NextHob.ResourceDescriptor->ResourceType ==
> EFI_RESOURCE_SYSTEM_MEMORY) &&
> - (DramInfo.DramRegion[DramInfo.NumOfDrams].BaseAddress >=
> NextHob.ResourceDescriptor->PhysicalStart) &&
> - (NextHob.ResourceDescriptor->PhysicalStart +
> NextHob.ResourceDescriptor->ResourceLength <=
> - DramInfo.DramRegion[DramInfo.NumOfDrams].BaseAddress +
> DramInfo.DramRegion[DramInfo.NumOfDrams].Size))
> - {
> - Found = TRUE;
> - break;
> + FoundSystemMem = FALSE;
> + ZeroMem (DramRegions, sizeof (DramRegions));
> +
> + Index = -1;
> + do {
> + ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
> + ArmSmcArgs.Arg1 = Index++;
> +
> + ArmCallSmc (&ArmSmcArgs);
> + ASSERT (!(ArmSmcArgs.Arg0 && !Index));
This is being a bit too clever for its own good.
We're verifying that if one of the inputs to the function we just
called is zero (i.e. is the first time around the loop), the value
returned in the struct must also be zero?
Is this equivalent to
if (Index == 0) {
ASSERT (ArmSmcArgs.Arg0 == SMC_WHATEVER_OK);
}
?
If so, please use that form.
Don't use !Index when you mean Index == 0.
Regardless, a short explanation of the protocol and expected responses
are needed unless the code can be made more self-explanatory.
> + if (!Index) {
> + DramSize = ArmSmcArgs.Arg1;
> + } else {
> + if (!ArmSmcArgs.Arg0) {
> + BaseAddress = ArmSmcArgs.Arg1;
> + Size = ArmSmcArgs.Arg2;
> + ASSERT (BaseAddress && Size);
> +
> + DramRegions[Index - 1].BaseAddress = BaseAddress;
> + DramRegions[Index - 1].Size = Size;
There's a lot of "-1" going on in this function, which should be
possible to avoid by incrementing Index at the end of the loop and
initializing it to 0. The compiler *will* do a better job at
optimizing code for you in all but the most exceptional cases, so try
to avoid optimising C.
> + DramSize -= Size;
> +
> + DEBUG ((DEBUG_INFO, "bank[%d]: start 0x%lx, size 0x%lx\n",
> + Index, BaseAddress, Size));
> }
> - NextHob.Raw = GET_NEXT_HOB (NextHob);
> + }
> + } while (DramSize && Index < MAX_DRAM_REGIONS);
> +
> + ASSERT (!DramSize);
And this (human language) semantically says "throw an exception if we
have RAM".
I can sort of start to make educated guesses at this point, but I
shouldn't have to. If making the ArmCallSmc with .Arg1 == 0 is a
special request to report the total amount of RAM in the system, that
shouldn't be part of the same loop as the calls extracting the size of
individual regions.
> +
> + FdBase = (UINTN)FixedPcdGet64 (PcdFdBaseAddress);
> + FdTop = FdBase + (UINTN)FixedPcdGet32 (PcdFdSize);
> +
> + // Declare memory regios to system
regions
> + for (Index = MAX_DRAM_REGIONS - 1; Index >= 0; Index--) {
I guess what confuses me about this loop is the combination of:
- Iterating over the DRAM regions backwards (optimization?)
- Not terminating the iteration once all of the Fd region has been
covered (the opposite of optimization).
Could something be done about that?
/
Leif
> + if (!DramRegions[Index].Size) {
> + continue;
> }
>
> - if (!Found) {
> - // Reserved the memory space occupied by the firmware volume
> + BaseAddress = DramRegions[Index].BaseAddress;
> + Top = DramRegions[Index].BaseAddress + DramRegions[Index].Size;
> +
> + // EDK2 does not have the concept of boot firmware copied into DRAM.
> + // To avoid the DXE core to overwrite this area we must create a memory
> + // allocation HOB for the region, but this only works if we split off the
> + // underlying resource descriptor as well.
> + if (FdBase >= BaseAddress && FdTop <= Top) {
> + // Update Size
> + Size = FdBase - BaseAddress;
> + if (Size) {
> + BuildResourceDescriptorHob (
> + EFI_RESOURCE_SYSTEM_MEMORY,
> + ResourceAttributes,
> + BaseAddress,
> + Size
> + );
> + }
> + // create the System Memory HOB for the firmware
> BuildResourceDescriptorHob (
> EFI_RESOURCE_SYSTEM_MEMORY,
> ResourceAttributes,
> - DramInfo.DramRegion[DramInfo.NumOfDrams].BaseAddress,
> - DramInfo.DramRegion[DramInfo.NumOfDrams].Size
> + FdBase,
> + PcdGet32 (PcdFdSize)
> );
> + // Create the System Memory HOB for the remaining region (top of the
> FD)s
> + Size = Top - FdTop;
> + if (Size) {
> + BuildResourceDescriptorHob (
> + EFI_RESOURCE_SYSTEM_MEMORY,
> + ResourceAttributes,
> + FdTop,
> + Size
> + );
> + };
> + // Mark the memory covering the Firmware Device as boot services data
> + BuildMemoryAllocationHob (FixedPcdGet64 (PcdFdBaseAddress),
> + FixedPcdGet32 (PcdFdSize),
> + EfiBootServicesData);
> + } else {
> + BuildResourceDescriptorHob (
> + EFI_RESOURCE_SYSTEM_MEMORY,
> + ResourceAttributes,
> + DramRegions[Index].BaseAddress,
> + DramRegions[Index].Size
> + );
> + }
> +
> + if (FoundSystemMem) {
> + continue;
> + }
> +
> + BaseAddress = DramRegions[Index].BaseAddress;
> + Size = DramRegions[Index].Size;
> + Top = DramRegions[Index].BaseAddress + DramRegions[Index].Size;
> +
> + if (FdBase >= BaseAddress && FdTop <= Top) {
> + Size -= (UINTN)FixedPcdGet32 (PcdFdSize);
> + }
> +
> + if (Size >= FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)) {
> + FoundSystemMem = TRUE;
> }
> }
>
> + ASSERT (FoundSystemMem);
> +
> // Build Memory Allocation Hob
> InitMmu (MemoryTable);
>
> diff --git a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h
> b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h
> new file mode 100644
> index 000000000000..edbf0ceaf638
> --- /dev/null
> +++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h
> @@ -0,0 +1,25 @@
> +/** @file
> +*
> +* Copyright 2020 NXP
> +*
> +* SPDX-License-Identifier: BSD-2-Clause-Patent
> +*
> +**/
> +
> +#ifndef MEMORY_INIT_PEI_LIB_H_
> +#define MEMORY_INIT_PEI_LIB_H_
> +
> +#include <Uefi.h>
> +
> +// Specifies the Maximum regions onto which DDR memory can be mapped in
> +// a Platform
> +#define MAX_DRAM_REGIONS 3
> +#define SMC_DRAM_BANK_INFO (0xC200FF12)
> +
> +typedef struct {
> + UINTN BaseAddress;
> + UINTN Size;
> +} DRAM_REGION_INFO;
> +
> +#endif // MEMORY_INIT_PEI_LIB_H_
> +
> diff --git a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf
> b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf
> index a5bd39415def..ad2371115b17 100644
> --- a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf
> +++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf
> @@ -18,7 +18,6 @@ [Defines]
> [Sources]
> MemoryInitPeiLib.c
>
> -
> [Packages]
> ArmPkg/ArmPkg.dec
> ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -30,6 +29,7 @@ [Packages]
> [LibraryClasses]
> ArmMmuLib
> ArmPlatformLib
> + ArmSmcLib
> DebugLib
> HobLib
> PcdLib
> @@ -40,6 +40,11 @@ [Guids]
> [FeaturePcd]
> gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob
>
> +[FixedPcd]
> + gArmTokenSpaceGuid.PcdFdBaseAddress
> + gArmTokenSpaceGuid.PcdFdSize
> + gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize
> +
> [Pcd]
> gArmTokenSpaceGuid.PcdSystemMemoryBase
> gArmTokenSpaceGuid.PcdSystemMemorySize
> diff --git a/Silicon/NXP/Library/SocLib/Chassis.c
> b/Silicon/NXP/Library/SocLib/Chassis.c
> index 847331a63152..1ef99e8de25f 100644
> --- a/Silicon/NXP/Library/SocLib/Chassis.c
> +++ b/Silicon/NXP/Library/SocLib/Chassis.c
> @@ -22,7 +22,6 @@
> #include <Library/PrintLib.h>
> #include <Library/SerialPortLib.h>
>
> -#include <DramInfo.h>
> #include "NxpChassis.h"
>
> UINT32
> @@ -75,69 +74,3 @@ SmmuInit (
> MmioWrite32 ((UINTN)SMMU_REG_NSCR0, Value);
> }
>
> -UINTN
> -GetDramSize (
> - IN VOID
> - )
> -{
> - ARM_SMC_ARGS ArmSmcArgs;
> -
> - ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
> - ArmSmcArgs.Arg1 = -1;
> -
> - ArmCallSmc (&ArmSmcArgs);
> -
> - if (ArmSmcArgs.Arg0) {
> - return 0;
> - } else {
> - return ArmSmcArgs.Arg1;
> - }
> -}
> -
> -EFI_STATUS
> -GetDramBankInfo (
> - IN OUT DRAM_INFO *DramInfo
> - )
> -{
> - ARM_SMC_ARGS ArmSmcArgs;
> - UINT32 I;
> - UINTN DramSize;
> -
> - DramSize = GetDramSize ();
> - DEBUG ((DEBUG_INFO, "DRAM Total Size 0x%lx \n", DramSize));
> -
> - // Ensure DramSize has been set
> - ASSERT (DramSize != 0);
> -
> - I = 0;
> -
> - do {
> - ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
> - ArmSmcArgs.Arg1 = I;
> -
> - ArmCallSmc (&ArmSmcArgs);
> - if (ArmSmcArgs.Arg0) {
> - if (I > 0) {
> - break;
> - } else {
> - ASSERT (ArmSmcArgs.Arg0 == 0);
> - }
> - }
> -
> - DramInfo->DramRegion[I].BaseAddress = ArmSmcArgs.Arg1;
> - DramInfo->DramRegion[I].Size = ArmSmcArgs.Arg2;
> -
> - DramSize -= DramInfo->DramRegion[I].Size;
> -
> - DEBUG ((DEBUG_INFO, "bank[%d]: start 0x%lx, size 0x%lx\n",
> - I, DramInfo->DramRegion[I].BaseAddress, DramInfo->DramRegion[I].Size));
> -
> - I++;
> - } while (DramSize);
> -
> - DramInfo->NumOfDrams = I;
> -
> - DEBUG ((DEBUG_INFO, "Number Of DRAM in system %d \n",
> DramInfo->NumOfDrams));
> -
> - return EFI_SUCCESS;
> -}
> diff --git a/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf
> b/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf
> index b7c7fc78cc8f..99d89498e0e2 100644
> --- a/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf
> +++ b/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf
> @@ -20,7 +20,6 @@ [Packages]
> Silicon/NXP/NxpQoriqLs.dec
>
> [LibraryClasses]
> - ArmSmcLib
> BaseLib
> DebugLib
> IoAccessLib
> --
> 2.17.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56844): https://edk2.groups.io/g/devel/message/56844
Mute This Topic: https://groups.io/mt/72077425/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
