Re: [PATCH v2 15/28] Silicon/NXP: Move RAM retrieval from SocLib

["Pankaj Bansal" <pankaj.bansal@nxp.com>]

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Wednesday, April 1, 2020 6:13 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 15/28] Silicon/NXP: Move RAM retrieval from SocLib
> 
> On Fri, Mar 20, 2020 at 20:05:30 +0530, Pankaj Bansal wrote:
> > From: Pankaj Bansal <pankaj.bansal@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.bansal@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.

Ok.

> 
> > +
> > +#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.

Ok. I will provide more detailed comments and try to simplify the code accordingly.

> 
> > +    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.


Ok.

> 
> > +        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.

OK.

> 
> > +
> > +  FdBase = (UINTN)FixedPcdGet64 (PcdFdBaseAddress);
> > +  FdTop = FdBase + (UINTN)FixedPcdGet32 (PcdFdSize);
> > +
> > +  // Declare memory regios to system
> 
>                        regions

OK

> 
> > +  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?
> 

The DRAM region info is sorted based on the RAM address is SOC memory map.
i.e. region0 is at lower address, as compared to region1.
The goal to start from last region is to find the topmost RAM region that can contain UEFI DXE region i.e. PcdSystemMemoryUefiRegionSize.
In the upcoming patches I would update the PcdSystemMemoryBase dynamically from that region.
If UEFI were to allocate any reserved or runtime region, it would be allocated from topmost RAM region.
This ensures that maximum amount of lower RAM (32 bit addresses) are left for OS to allocate to devices that can only
work with 32bit physical addresses. E.g. legacy devices that need to DMA to 32bit addresses.

> /
>     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
> >