Re: [PATCH 13/19] Silicon/NXP: Move RAM retrieval from SocLib

["Leif Lindholm" <leif@nuviainc.com>]

On Fri, Feb 07, 2020 at 18:13:22 +0530, Pankaj Bansal wrote:
> RAM retrieval using SMC commands is common to all Layerscape SOCs.
> Therefore, move it to commom MemoryInit Pei Lib.
> 
> Also added provision to reserve a portion of RAM.

"Also" is an indicator of something that should be a separate patch.
That patch could explain the use case for the reservation function.

> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  Silicon/NXP/Include/DramInfo.h                |  38 ---
>  .../Library/MemoryInitPei/MemoryInitPeiLib.c  | 235 +++++++++++++++---
>  .../Library/MemoryInitPei/MemoryInitPeiLib.h  |  24 ++
>  .../MemoryInitPei/MemoryInitPeiLib.inf        |   8 +
>  Silicon/NXP/Library/SocLib/Chassis.c          |  67 -----
>  Silicon/NXP/NxpQoriqLs.dec                    |   7 +-
>  6 files changed, 239 insertions(+), 140 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 a934aaeff1..0000000000
> --- 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 3ea7736786..eb1983bdbc 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 sort includes alphabetically.

> +
> +#include "MemoryInitPeiLib.h"
>  
> -#include <DramInfo.h>
>  
>  VOID
>  BuildMemoryTypeInformationHob (
> @@ -44,6 +46,85 @@ InitMmu (
>    }
>  }
>  
> +STATIC
> +UINTN
> +CalculateReservedMemBase (
> +  IN DRAM_REGION_INFO *DramRegions,
> +  IN UINT32           NumRegions,
> +  IN UINTN            ReservedMemSize
> +)
> +{
> +  UINTN                 ReservedMemAlignment;
> +  INTN                  Index;
> +  EFI_PHYSICAL_ADDRESS  AlignmentMask;
> +  UINTN                 RegionBaseAddress;
> +  UINTN                 RegionSize;
> +  UINTN                 ReservedBaseAddress;
> +  INTN                  Index2;
> +
> +  ReservedMemAlignment = FixedPcdGet64 (PcdReservedMemAlignment);
> +  //
> +  // Compute alignment bit mask
> +  //
> +  if (ReservedMemAlignment) {
> +    AlignmentMask = LShiftU64 (1, LowBitSet64(ReservedMemAlignment)) - 1;
> +  } else {
> +    AlignmentMask = 0;
> +  }
> +  Index = NumRegions;
> +  while (Index--) {
> +    RegionBaseAddress = DramRegions[Index].BaseAddress;
> +    RegionSize = DramRegions[Index].Size;
> +
> +    if (ReservedMemSize > RegionSize) {
> +      continue;
> +    }
> +
> +    ReservedBaseAddress = (RegionBaseAddress + RegionSize - ReservedMemSize) & (~AlignmentMask);
> +    if (ReservedBaseAddress < RegionBaseAddress) {
> +      continue;
> +    }
> +
> +    // found the region from which reserved mem is to be carved out
> +    // Need to modify the region size and create/delete region if need be
> +    RegionSize -= ReservedMemSize;
> +    if (!RegionSize) {
> +      for (Index2 = Index; Index2 < NumRegions; Index2++) {
> +        CopyMem (&DramRegions[Index2], &DramRegions[Index2 + 1], sizeof (DRAM_REGION_INFO));
> +      }
> +      break;
> +    }
> +
> +    if (ReservedBaseAddress - RegionBaseAddress) {
> +      DramRegions[Index].Size = ReservedBaseAddress - RegionBaseAddress;
> +      RegionSize -= DramRegions[Index].Size;
> +    } else {
> +      DramRegions[Index].BaseAddress = ReservedBaseAddress + ReservedMemSize;
> +      DramRegions[Index].Size = RegionSize;
> +      RegionSize = 0;
> +    }
> +
> +    if (!RegionSize) {
> +      break;
> +    }
> +
> +    for (Index2 = NumRegions; Index2 > (Index + 1); Index2--) {
> +      CopyMem (&DramRegions[Index2], &DramRegions[Index2 - 1], sizeof (DRAM_REGION_INFO));
> +    }
> +    DramRegions[Index2].BaseAddress = ReservedBaseAddress + ReservedMemSize;
> +    DramRegions[Index2].Size = RegionSize;
> +    RegionSize = 0;
> +
> +    break;
> +  }
> +
> +  if (Index == -1) {
> +    return 0;
> +  } else {
> +    return ReservedBaseAddress;
> +  }
> +}
> +
>  /*++
>  
>  Routine Description:
> @@ -68,10 +149,19 @@ MemoryPeim (
>    )
>  {
>    ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
> +  ARM_SMC_ARGS                 ArmSmcArgs;
> +  INT32                        Index;
> +  UINTN                        DramSize;
> +  UINTN                        BaseAddress;
> +  UINTN                        Size;
> +  UINTN                        Top;
> +  // Extra region gets created if we want to reserve a memory region and that creates a memory hole
> +  // because of alignement requirements
> +  DRAM_REGION_INFO             DramRegions[MAX_DRAM_REGIONS + 1];
>    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);
> @@ -86,48 +176,127 @@ MemoryPeim (
>    // Now, the permanent memory has been installed, we can call AllocatePages()
>    //
>    ResourceAttributes = (
> -    EFI_RESOURCE_ATTRIBUTE_PRESENT |
> -    EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> -    EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> -    EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> -    EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> -    EFI_RESOURCE_ATTRIBUTE_TESTED
> +      EFI_RESOURCE_ATTRIBUTE_PRESENT |
> +      EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> +      EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> +      EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> +      EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> +      EFI_RESOURCE_ATTRIBUTE_TESTED

This unrelated indentation change is wrong.

>    );
>  
> -  if (GetDramBankInfo (&DramInfo)) {
> -    DEBUG ((DEBUG_ERROR, "Failed to get DRAM information, exiting...\n"));
> -    return EFI_UNSUPPORTED;
> -  }
> +  FoundSystemMem = FALSE;
> +  ZeroMem (DramRegions, sizeof (DramRegions));
> +
> +  Index = -1;
> +  do {
> +    ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
> +    ArmSmcArgs.Arg1 = Index++;

This loop would be nicer in a helper function (which would also make
the diff more clear).

>  
> -  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;
> +    ArmCallSmc (&ArmSmcArgs);
> +    ASSERT (!(ArmSmcArgs.Arg0 && !Index));
> +    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;
> +        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);
> +
> +  // Get the reserved memory size from non volatile storage
> +  Size = FixedPcdGet64 (PcdReservedMemSize);

FixedPcd reads from non-volatile storage?

> +  if (Size) {
> +    BaseAddress = CalculateReservedMemBase (DramRegions, Index, Size);
> +    if (BaseAddress) {
> +      DEBUG ((DEBUG_INFO, "ReservedMem: start 0x%lx, size 0x%lx\n", BaseAddress, Size));
> +    }
> +  }
> +
> +  FdBase = (UINTN)FixedPcdGet64 (PcdFdBaseAddress);
> +  FdTop = FdBase + (UINTN)FixedPcdGet32 (PcdFdSize);
> +
> +  // Declare memory regios to system
> +  for (Index = MAX_DRAM_REGIONS; Index >= 0; Index--) {
> +    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

Which boot firmware are we talking about here?

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

Space after } 

> +      BuildResourceDescriptorHob (
> +          EFI_RESOURCE_SYSTEM_MEMORY,

Incorrect indentation.

> +          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 0000000000..e563b2ba8d
> --- /dev/null
> +++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h
> @@ -0,0 +1,24 @@
> +/** @file
> +*
> +*  Copyright 2020 NXP
> +*
> +*  SPDX-License-Identifier: BSD-2-Clause-Patent
> +*
> +**/
> +
> +#ifndef _MEMORY_INIT_PEI_LIB_H_
> +#define _MEMORY_INIT_PEI_LIB_H_

Please drop leading _ from include guards.

> +
> +#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
> +
> diff --git a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf
> index a5bd39415d..9adddcaf8c 100644
> --- a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf
> +++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf
> @@ -33,6 +33,7 @@
>    DebugLib
>    HobLib
>    PcdLib
> +  ArmSmcLib

Please sort library classes alphabetically.

>  
>  [Guids]
>    gEfiMemoryTypeInformationGuid
> @@ -40,6 +41,13 @@
>  [FeaturePcd]
>    gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob
>  
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdFdBaseAddress
> +  gArmTokenSpaceGuid.PcdFdSize
> +  gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize

Please sort fixedpcds alphabetically.

> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment
> +
>  [Pcd]
>    gArmTokenSpaceGuid.PcdSystemMemoryBase
>    gArmTokenSpaceGuid.PcdSystemMemorySize
> diff --git a/Silicon/NXP/Library/SocLib/Chassis.c b/Silicon/NXP/Library/SocLib/Chassis.c
> index 847331a631..1ef99e8de2 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/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> index 4f14cc9848..c327e738cc 100644
> --- a/Silicon/NXP/NxpQoriqLs.dec
> +++ b/Silicon/NXP/NxpQoriqLs.dec
> @@ -38,7 +38,10 @@
>    #
>    gNxpQoriqLsTokenSpaceGuid.PcdGurBigEndian|FALSE|BOOLEAN|0x0000311
>  
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|0x0|UINT64|0x00000315
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|0x0|UINT64|0x00000316
> +
>  [PcdsFeatureFlag]
> -  gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203|FALSE|BOOLEAN|0x00000315
> -  gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian|FALSE|BOOLEAN|0x00000316
> +  gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203|FALSE|BOOLEAN|0x00000317
> +  gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian|FALSE|BOOLEAN|0x00000318

And this hunk shows why it's a good idea to put your Pcds into groups
with gaps in between the tokens. The tokens should *not* be changing
throughout a single set, and only in unusual circumstances anyway.

/
    Leif

>  
> -- 
> 2.17.1
>