[PATCH edk2-platforms 0/3] Add Features to NXP Platforms

[PATCH edk2-platforms 0/3] Add Features to NXP Platforms

[Pankaj Bansal]

From: Pankaj Bansal <pankaj.bansal@nxp.com>

This patch series adds some useful features to NXP platforms.
- runtime safe version of DebugLib
- Add support for reserving a chunk from RAM
- Add Support for git commit info print

Pankaj Bansal (3):
  Silicon/NXP: Use runtime safe version of DebugLib
  Silicon/NXP: Add support for reserving a chunk from RAM
  Silicon/NXP: Add Support for git commit info print

 Silicon/NXP/NxpQoriqLs.dec                    |  10 ++
 Silicon/NXP/LX2160A/LX2160A.dsc.inc           |   4 +
 Silicon/NXP/NxpQoriqLs.dsc.inc                |   4 +
 .../Library/ChassisLib/ChassisLib.inf         |   5 +
 .../Library/ChassisLib/ChassisLib.inf         |   5 +
 .../MemoryInitPeiLib/MemoryInitPeiLib.inf     |   3 +
 .../Chassis2/Library/ChassisLib/ChassisLib.c  |  17 +++
 .../Library/ChassisLib/ChassisLib.c           |  17 +++
 .../MemoryInitPeiLib/MemoryInitPeiLib.c       | 142 +++++++++++++++++-
 Silicon/NXP/set_firmware_ver.sh               |  36 +++++
 10 files changed, 241 insertions(+), 2 deletions(-)
 create mode 100755 Silicon/NXP/set_firmware_ver.sh

-- 
2.17.1

[PATCH edk2-platforms 1/3] Silicon/NXP: Use runtime safe version of DebugLib

[Pankaj Bansal]

From: Pankaj Bansal <pankaj.bansal@nxp.com>

For DXE_RUNTIME_DRIVER runtime safe version of DebugLib should be
used. Otherwise, any DEBUG print in code can result in abort in OS.

Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
 Silicon/NXP/NxpQoriqLs.dsc.inc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Silicon/NXP/NxpQoriqLs.dsc.inc b/Silicon/NXP/NxpQoriqLs.dsc.inc
index ee639d552483..06ee012c227a 100644
--- a/Silicon/NXP/NxpQoriqLs.dsc.inc
+++ b/Silicon/NXP/NxpQoriqLs.dsc.inc
@@ -163,6 +163,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
   MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
   CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
   ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
+  DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
 
 [LibraryClasses.AARCH64]
   #
-- 
2.17.1

[PATCH edk2-platforms 2/3] Silicon/NXP: Add support for reserving a chunk from RAM

[Pankaj Bansal]

From: Pankaj Bansal <pankaj.bansal@nxp.com>

Some NXP SOCs have some specialized IP blocks (like MC), which
require DDR memory to operate. This DDR memory should not be managed
by OS or UEFI.

Moreover to ensure that these IP blocks always get memory, and maximum
contiguous RAM is available for UEFI and OS to use, add the support for
reserving a chunk from RAM before reporting available RAM to UEFI.

Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
 Silicon/NXP/NxpQoriqLs.dec                                |  10 ++
 Silicon/NXP/LX2160A/LX2160A.dsc.inc                       |   4 +
 Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf |   3 +
 Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c   | 142 +++++++++++++++++++-
 4 files changed, 157 insertions(+), 2 deletions(-)

diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
index 188a9fe1f382..0e762066e547 100644
--- a/Silicon/NXP/NxpQoriqLs.dec
+++ b/Silicon/NXP/NxpQoriqLs.dec
@@ -41,3 +41,13 @@ [PcdsDynamic.common]
   gNxpQoriqLsTokenSpaceGuid.PcdPciCfgShiftEnable|FALSE|BOOLEAN|0x00000600
   gNxpQoriqLsTokenSpaceGuid.PcdPciLsGen4Ctrl|FALSE|BOOLEAN|0x00000601
   gNxpQoriqLsTokenSpaceGuid.PcdPciHideRootPort|FALSE|BOOLEAN|0x00000602
+
+  # Reserved RAM Base address alignment. This number ought to be Power of two
+  # in case no alignment is needed, this number should be 1.
+  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|0x1|UINT64|0x00000603
+  # Size of the RAM to be reserved. This RAM region is neither reported to UEFI
+  # nor to OS
+  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|0x0|UINT64|0x00000604
+  # Reserved RAM Base address which is calculated based on PcdReservedMemSize
+  # and PcdReservedMemAlignment
+  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemBase|0x0|UINT64|0x00000605
diff --git a/Silicon/NXP/LX2160A/LX2160A.dsc.inc b/Silicon/NXP/LX2160A/LX2160A.dsc.inc
index 43e361464c8e..755ca169f213 100644
--- a/Silicon/NXP/LX2160A/LX2160A.dsc.inc
+++ b/Silicon/NXP/LX2160A/LX2160A.dsc.inc
@@ -29,6 +29,10 @@ [PcdsDynamicDefault.common]
   gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x6200000
   gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0xC0C0000
 
+[PcdsDynamicHii]
+  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|L"ReservedMemAlignment"|gEfiGlobalVariableGuid|0x0|0x20000000|NV,BS
+  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|L"ReservedMemSize"|gEfiGlobalVariableGuid|0x0|0x20000000|NV,BS
+
 [PcdsFixedAtBuild.common]
   gArmTokenSpaceGuid.PcdGenericWatchdogControlBase|0x23A0000
   gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase|0x2390000
diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
index a33f8cd3f743..ed23a86b43d9 100644
--- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
+++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
@@ -49,6 +49,9 @@ [FixedPcd]
 [Pcd]
   gArmTokenSpaceGuid.PcdSystemMemoryBase
   gArmTokenSpaceGuid.PcdSystemMemorySize
+  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment
+  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemBase
+  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize
 
 [Depex]
   TRUE
diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
index 11d1f1260b35..b416323a4ced 100644
--- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
+++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
@@ -118,6 +118,127 @@ GetDramRegionsInfo (
   return EFI_BUFFER_TOO_SMALL;
 }
 
+/**
+  Calculate the base address of Reserved RAM.
+  Reserved RAM is not reported to either UEFI or OS.
+
+  @param[in, out] DramRegions  Array of type DRAM_REGION_INFO. The size of this
+                               array must be one more (+ 1) than the maximum
+                               regions supported on platform. This is because,
+                               if due to Reserved RAM alignment requirements a
+                               hole is created in any DRAM region, then the RAM
+                               after hole gets reported to UEFI and then
+                               subsequently to OS. which is why, the last entry
+                               of this array will not be parsed while
+                               calculating Reserved RAM base address. Caller
+                               must ensure that last entry of this array is zero
+                               initialized.
+  @param[in] NumRegions        Size of DramRegions array (including +1 for hole)
+  @param[in] ReservedMemSize   Size of RAM to be reserved.
+
+  @return  if successful Address of the Reserved RAM region, 0 otherwise.
+**/
+STATIC
+UINTN
+CalculateReservedMemBase (
+  IN DRAM_REGION_INFO *DramRegions,
+  IN UINT32           NumRegions,
+  IN UINTN            ReservedMemSize
+)
+{
+  UINTN                 ReservedMemAlignment;
+  EFI_PHYSICAL_ADDRESS  AlignmentMask;
+  UINTN                 RegionBaseAddress;
+  UINTN                 RegionSize;
+  UINTN                 ReservedBaseAddress;
+  INTN                  Index;
+  INTN                  Index2;
+
+  ReservedMemAlignment = PcdGet64 (PcdReservedMemAlignment);
+  //
+  // Compute alignment bit mask
+  //
+  if (ReservedMemAlignment) {
+    AlignmentMask = LShiftU64 (1, LowBitSet64(ReservedMemAlignment)) - 1;
+  } else {
+    AlignmentMask = 0;
+  }
+
+  // The DRAM region info is sorted based on the RAM address is SOC memory map.
+  // i.e. DramRegions[0] is at lower address, as compared to DramRegions[1].
+  // The goal to start from last region is to find the topmost RAM region that
+  // can contain Reserved RAM region i.e. PcdReservedMemSize.
+  // Since this RAM is not reported to either UEFI or OS, 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.
+  for (Index = NumRegions - 2; Index >=0; Index--) {
+    RegionBaseAddress = DramRegions[Index].BaseAddress;
+    RegionSize = DramRegions[Index].Size;
+
+    if (ReservedMemSize > RegionSize) {
+      continue;
+    }
+
+    ReservedBaseAddress = (RegionBaseAddress + RegionSize - ReservedMemSize);
+    ReservedBaseAddress &= (~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 == 0) {
+      // delete the region but maintain the sorted list of regions
+      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 == 0) {
+      break;
+    }
+
+    // A hole has been created in DRAM regions due to Reserved RAM alignment
+    // requirements. create a new DRAM region for DRAM memory after hole.
+    // Maintain the sorted list of regions
+    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;
+  }
+}
+
 /**
   Get the installed RAM information.
   Initialize Memory HOBs (Resource Descriptor HOBs)
@@ -135,7 +256,9 @@ MemoryInitPeiLibConstructor (
   UINTN                         BaseAddress;
   UINTN                         Size;
   UINTN                         Top;
-  DRAM_REGION_INFO              DramRegions[MAX_DRAM_REGIONS];
+  // Extra region gets created if we want to reserve a memory region and that
+  // creates a memory hole because of alignment requirements
+  DRAM_REGION_INFO              DramRegions[MAX_DRAM_REGIONS + 1];
   EFI_RESOURCE_ATTRIBUTE_TYPE   ResourceAttributes;
   UINTN                         FdBase;
   UINTN                         FdTop;
@@ -155,6 +278,21 @@ MemoryInitPeiLibConstructor (
 
   (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
 
+  // Get the reserved memory size from non volatile storage
+  Size = PcdGet64 (PcdReservedMemSize);
+  if (Size) {
+    BaseAddress = CalculateReservedMemBase (
+                    DramRegions,
+                    ARRAY_SIZE (DramRegions),
+                    Size
+                  );
+    if (BaseAddress) {
+      DEBUG ((DEBUG_INFO, "ReservedMem: start 0x%lx, size 0x%lx\n",
+        BaseAddress, Size));
+      PcdSet64S (PcdReservedMemBase, BaseAddress);
+    }
+  }
+
   FdBase = (UINTN)PcdGet64 (PcdFdBaseAddress);
   FdTop = FdBase + (UINTN)PcdGet32 (PcdFdSize);
 
@@ -168,7 +306,7 @@ MemoryInitPeiLibConstructor (
   // 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.
-  for (Index = MAX_DRAM_REGIONS - 1; Index >= 0; Index--) {
+  for (Index = MAX_DRAM_REGIONS; Index >= 0; Index--) {
     if (DramRegions[Index].Size == 0) {
       continue;
     }
-- 
2.17.1

[PATCH edk2-platforms 3/3] Silicon/NXP: Add Support for git commit info print

[Pankaj Bansal]

From: Pankaj Bansal <pankaj.bansal@nxp.com>

This patch adds the Support for printing the git commit information
in linux build environment.

Ideal place of retrieving this information should be python script in
BaseTools.

A Feature request for the same has been created:
https://bugzilla.tianocore.org/show_bug.cgi?id=2838

Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
 Silicon/NXP/NxpQoriqLs.dsc.inc                           |  3 ++
 Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf   |  5 +++
 Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf |  5 +++
 Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c     | 17 +++++++++
 Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c   | 17 +++++++++
 Silicon/NXP/set_firmware_ver.sh                          | 36 ++++++++++++++++++++
 6 files changed, 83 insertions(+)

diff --git a/Silicon/NXP/NxpQoriqLs.dsc.inc b/Silicon/NXP/NxpQoriqLs.dsc.inc
index 06ee012c227a..a0762a6ef61d 100644
--- a/Silicon/NXP/NxpQoriqLs.dsc.inc
+++ b/Silicon/NXP/NxpQoriqLs.dsc.inc
@@ -224,6 +224,9 @@ [PcdsDynamicHii.common.DEFAULT]
   gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|10
 
 [PcdsFixedAtBuild.common]
+  !ifdef $(FIRMWARE_VER)
+    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"$(FIRMWARE_VER)"
+  !endif
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
   gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
index f5dbd1349dc5..69f884af9e34 100644
--- a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
+++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
@@ -16,6 +16,7 @@ [Defines]
 
 [Packages]
   ArmPkg/ArmPkg.dec
+  MdeModulePkg/MdeModulePkg.dec
   MdePkg/MdePkg.dec
   Silicon/NXP/Chassis2/Chassis2.dec
   Silicon/NXP/NxpQoriqLs.dec
@@ -24,6 +25,7 @@ [LibraryClasses]
   IoAccessLib
   IoLib
   PcdLib
+  PrintLib
   SerialPortLib
 
 [Sources.common]
@@ -31,3 +33,6 @@ [Sources.common]
 
 [FeaturePcd]
   gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian
+
+[FixedPcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
diff --git a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf
index 75b68cc4ca2d..632acc52b20a 100644
--- a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf
+++ b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf
@@ -16,6 +16,7 @@ [Defines]
 
 [Packages]
   ArmPkg/ArmPkg.dec
+  MdeModulePkg/MdeModulePkg.dec
   MdePkg/MdePkg.dec
   Silicon/NXP/Chassis3V2/Chassis3V2.dec
   Silicon/NXP/NxpQoriqLs.dec
@@ -24,6 +25,7 @@ [LibraryClasses]
   IoAccessLib
   IoLib
   PcdLib
+  PrintLib
   SerialPortLib
 
 [Sources.common]
@@ -31,3 +33,6 @@ [Sources.common]
 
 [FeaturePcd]
   gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian
+
+[FixedPcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
index 91b19f832f00..bc782e7e3873 100644
--- a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
+++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
@@ -12,6 +12,7 @@
 #include <Library/IoAccessLib.h>
 #include <Library/IoLib.h>
 #include <Library/PcdLib.h>
+#include <Library/PrintLib.h>
 #include <Library/SerialPortLib.h>
 
 /**
@@ -89,10 +90,26 @@ ChassisInit (
   VOID
   )
 {
+  CHAR8                         Buffer[100];
+  UINTN                         CharCount;
+
   //
   // Early init serial Port to get board information.
   //
   SerialPortInitialize ();
 
+  CharCount = AsciiSPrint (
+                Buffer, sizeof (Buffer),
+                "UEFI firmware built at %a on %a. version:\n\r",
+                __TIME__, __DATE__
+              );
+  SerialPortWrite ((UINT8 *) Buffer, CharCount);
+
+  CharCount = AsciiSPrint (
+                Buffer, sizeof (Buffer), "%s\n\r",
+                (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString)
+              );
+  SerialPortWrite ((UINT8 *) Buffer, CharCount);
+
   SmmuInit ();
 }
diff --git a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
index 30f8f945b233..6d546f4754f9 100644
--- a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
+++ b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
@@ -12,6 +12,7 @@
 #include <Library/IoAccessLib.h>
 #include <Library/IoLib.h>
 #include <Library/PcdLib.h>
+#include <Library/PrintLib.h>
 #include <Library/SerialPortLib.h>
 
 /**
@@ -64,8 +65,24 @@ ChassisInit (
   VOID
   )
 {
+  CHAR8                         Buffer[100];
+  UINTN                         CharCount;
+
   //
   // Early init serial Port to get board information.
   //
   SerialPortInitialize ();
+
+  CharCount = AsciiSPrint (
+                Buffer, sizeof (Buffer),
+                "UEFI firmware built at %a on %a. version:\n\r",
+                __TIME__, __DATE__
+              );
+  SerialPortWrite ((UINT8 *) Buffer, CharCount);
+
+  CharCount = AsciiSPrint (
+                Buffer, sizeof (Buffer), "%s\n\r",
+                (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString)
+              );
+  SerialPortWrite ((UINT8 *) Buffer, CharCount);
 }
diff --git a/Silicon/NXP/set_firmware_ver.sh b/Silicon/NXP/set_firmware_ver.sh
new file mode 100755
index 000000000000..ba2336ad23dc
--- /dev/null
+++ b/Silicon/NXP/set_firmware_ver.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+#
+# Copyright 2020 NXP
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+# parse PACKAGES_PATH and set FIRMWARE_VER based on that
+
+get_git_version()
+{
+  command -v git>/dev/null 2>&1
+  if [ $? -eq 0 ] && [ -n "$1" ]
+  then
+    head_or_tag=`git -C $1 describe --tag --dirty --broken --always 2>/dev/null`
+    printf $head_or_tag
+  fi
+}
+
+if [ -n "$FIRMWARE_VER" ]; then
+  echo "Warning FIRMWARE_VER=$FIRMWARE_VER is already set"
+  echo "Please retry after 'unset FIRMWARE_VER' command"
+fi
+
+directories=$(echo $PACKAGES_PATH | tr ":" "\n")
+for dir in $directories; do
+  if [ -n "$FIRMWARE_VER" ]; then
+    FIRMWARE_VER="$FIRMWARE_VER;$(basename $dir):$(get_git_version $dir)"
+  else
+    FIRMWARE_VER=$(basename $dir):$(get_git_version $dir)
+  fi
+done
+
+echo "FIRMWARE_VER=$FIRMWARE_VER"
+export FIRMWARE_VER=$FIRMWARE_VER
+
+echo "build edk2 firmware with -D FIRMWARE_VER=\$FIRMWARE_VER"
-- 
2.17.1

Re: [PATCH edk2-platforms 2/3] Silicon/NXP: Add support for reserving a chunk from RAM

[Pankaj Bansal]

Please don't merge this Patch.

This patch needs update. The ReservedRam needs to be reported to UEFI.
I will send v2 for this patch. other patches in this series can be reviewed.

Regards,
Pankaj Bansal

> -----Original Message-----
> From: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> Sent: Wednesday, July 8, 2020 10:50 AM
> To: Leif Lindholm <leif@nuviainc.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; devel@edk2.groups.io; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [PATCH edk2-platforms 2/3] Silicon/NXP: Add support for reserving a
> chunk from RAM
> 
> From: Pankaj Bansal <pankaj.bansal@nxp.com>
> 
> Some NXP SOCs have some specialized IP blocks (like MC), which
> require DDR memory to operate. This DDR memory should not be managed
> by OS or UEFI.
> 
> Moreover to ensure that these IP blocks always get memory, and maximum
> contiguous RAM is available for UEFI and OS to use, add the support for
> reserving a chunk from RAM before reporting available RAM to UEFI.
> 
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  Silicon/NXP/NxpQoriqLs.dec                                |  10 ++
>  Silicon/NXP/LX2160A/LX2160A.dsc.inc                       |   4 +
>  Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf |   3 +
>  Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c   | 142
> +++++++++++++++++++-
>  4 files changed, 157 insertions(+), 2 deletions(-)
> 
> diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> index 188a9fe1f382..0e762066e547 100644
> --- a/Silicon/NXP/NxpQoriqLs.dec
> +++ b/Silicon/NXP/NxpQoriqLs.dec
> @@ -41,3 +41,13 @@ [PcdsDynamic.common]
> 
> gNxpQoriqLsTokenSpaceGuid.PcdPciCfgShiftEnable|FALSE|BOOLEAN|0x000006
> 00
>    gNxpQoriqLsTokenSpaceGuid.PcdPciLsGen4Ctrl|FALSE|BOOLEAN|0x00000601
> 
> gNxpQoriqLsTokenSpaceGuid.PcdPciHideRootPort|FALSE|BOOLEAN|0x0000060
> 2
> +
> +  # Reserved RAM Base address alignment. This number ought to be Power of
> two
> +  # in case no alignment is needed, this number should be 1.
> +
> gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|0x1|UINT64|0x0000
> 0603
> +  # Size of the RAM to be reserved. This RAM region is neither reported to UEFI
> +  # nor to OS
> +
> gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|0x0|UINT64|0x00000604
> +  # Reserved RAM Base address which is calculated based on
> PcdReservedMemSize
> +  # and PcdReservedMemAlignment
> +
> gNxpQoriqLsTokenSpaceGuid.PcdReservedMemBase|0x0|UINT64|0x00000605
> diff --git a/Silicon/NXP/LX2160A/LX2160A.dsc.inc
> b/Silicon/NXP/LX2160A/LX2160A.dsc.inc
> index 43e361464c8e..755ca169f213 100644
> --- a/Silicon/NXP/LX2160A/LX2160A.dsc.inc
> +++ b/Silicon/NXP/LX2160A/LX2160A.dsc.inc
> @@ -29,6 +29,10 @@ [PcdsDynamicDefault.common]
>    gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x6200000
>    gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0xC0C0000
> 
> +[PcdsDynamicHii]
> +
> gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|L"ReservedMemAlig
> nment"|gEfiGlobalVariableGuid|0x0|0x20000000|NV,BS
> +
> gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|L"ReservedMemSize"|gEfi
> GlobalVariableGuid|0x0|0x20000000|NV,BS
> +
>  [PcdsFixedAtBuild.common]
>    gArmTokenSpaceGuid.PcdGenericWatchdogControlBase|0x23A0000
>    gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase|0x2390000
> diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> index a33f8cd3f743..ed23a86b43d9 100644
> --- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> +++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> @@ -49,6 +49,9 @@ [FixedPcd]
>  [Pcd]
>    gArmTokenSpaceGuid.PcdSystemMemoryBase
>    gArmTokenSpaceGuid.PcdSystemMemorySize
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemBase
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize
> 
>  [Depex]
>    TRUE
> diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> index 11d1f1260b35..b416323a4ced 100644
> --- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> +++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> @@ -118,6 +118,127 @@ GetDramRegionsInfo (
>    return EFI_BUFFER_TOO_SMALL;
>  }
> 
> +/**
> +  Calculate the base address of Reserved RAM.
> +  Reserved RAM is not reported to either UEFI or OS.
> +
> +  @param[in, out] DramRegions  Array of type DRAM_REGION_INFO. The size
> of this
> +                               array must be one more (+ 1) than the maximum
> +                               regions supported on platform. This is because,
> +                               if due to Reserved RAM alignment requirements a
> +                               hole is created in any DRAM region, then the RAM
> +                               after hole gets reported to UEFI and then
> +                               subsequently to OS. which is why, the last entry
> +                               of this array will not be parsed while
> +                               calculating Reserved RAM base address. Caller
> +                               must ensure that last entry of this array is zero
> +                               initialized.
> +  @param[in] NumRegions        Size of DramRegions array (including +1 for hole)
> +  @param[in] ReservedMemSize   Size of RAM to be reserved.
> +
> +  @return  if successful Address of the Reserved RAM region, 0 otherwise.
> +**/
> +STATIC
> +UINTN
> +CalculateReservedMemBase (
> +  IN DRAM_REGION_INFO *DramRegions,
> +  IN UINT32           NumRegions,
> +  IN UINTN            ReservedMemSize
> +)
> +{
> +  UINTN                 ReservedMemAlignment;
> +  EFI_PHYSICAL_ADDRESS  AlignmentMask;
> +  UINTN                 RegionBaseAddress;
> +  UINTN                 RegionSize;
> +  UINTN                 ReservedBaseAddress;
> +  INTN                  Index;
> +  INTN                  Index2;
> +
> +  ReservedMemAlignment = PcdGet64 (PcdReservedMemAlignment);
> +  //
> +  // Compute alignment bit mask
> +  //
> +  if (ReservedMemAlignment) {
> +    AlignmentMask = LShiftU64 (1, LowBitSet64(ReservedMemAlignment)) - 1;
> +  } else {
> +    AlignmentMask = 0;
> +  }
> +
> +  // The DRAM region info is sorted based on the RAM address is SOC memory
> map.
> +  // i.e. DramRegions[0] is at lower address, as compared to DramRegions[1].
> +  // The goal to start from last region is to find the topmost RAM region that
> +  // can contain Reserved RAM region i.e. PcdReservedMemSize.
> +  // Since this RAM is not reported to either UEFI or OS, 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.
> +  for (Index = NumRegions - 2; Index >=0; Index--) {
> +    RegionBaseAddress = DramRegions[Index].BaseAddress;
> +    RegionSize = DramRegions[Index].Size;
> +
> +    if (ReservedMemSize > RegionSize) {
> +      continue;
> +    }
> +
> +    ReservedBaseAddress = (RegionBaseAddress + RegionSize -
> ReservedMemSize);
> +    ReservedBaseAddress &= (~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 == 0) {
> +      // delete the region but maintain the sorted list of regions
> +      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 == 0) {
> +      break;
> +    }
> +
> +    // A hole has been created in DRAM regions due to Reserved RAM alignment
> +    // requirements. create a new DRAM region for DRAM memory after hole.
> +    // Maintain the sorted list of regions
> +    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;
> +  }
> +}
> +
>  /**
>    Get the installed RAM information.
>    Initialize Memory HOBs (Resource Descriptor HOBs)
> @@ -135,7 +256,9 @@ MemoryInitPeiLibConstructor (
>    UINTN                         BaseAddress;
>    UINTN                         Size;
>    UINTN                         Top;
> -  DRAM_REGION_INFO              DramRegions[MAX_DRAM_REGIONS];
> +  // Extra region gets created if we want to reserve a memory region and that
> +  // creates a memory hole because of alignment requirements
> +  DRAM_REGION_INFO              DramRegions[MAX_DRAM_REGIONS + 1];
>    EFI_RESOURCE_ATTRIBUTE_TYPE   ResourceAttributes;
>    UINTN                         FdBase;
>    UINTN                         FdTop;
> @@ -155,6 +278,21 @@ MemoryInitPeiLibConstructor (
> 
>    (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
> 
> +  // Get the reserved memory size from non volatile storage
> +  Size = PcdGet64 (PcdReservedMemSize);
> +  if (Size) {
> +    BaseAddress = CalculateReservedMemBase (
> +                    DramRegions,
> +                    ARRAY_SIZE (DramRegions),
> +                    Size
> +                  );
> +    if (BaseAddress) {
> +      DEBUG ((DEBUG_INFO, "ReservedMem: start 0x%lx, size 0x%lx\n",
> +        BaseAddress, Size));
> +      PcdSet64S (PcdReservedMemBase, BaseAddress);
> +    }
> +  }
> +
>    FdBase = (UINTN)PcdGet64 (PcdFdBaseAddress);
>    FdTop = FdBase + (UINTN)PcdGet32 (PcdFdSize);
> 
> @@ -168,7 +306,7 @@ MemoryInitPeiLibConstructor (
>    // 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.
> -  for (Index = MAX_DRAM_REGIONS - 1; Index >= 0; Index--) {
> +  for (Index = MAX_DRAM_REGIONS; Index >= 0; Index--) {
>      if (DramRegions[Index].Size == 0) {
>        continue;
>      }
> --
> 2.17.1

Re: [PATCH edk2-platforms 1/3] Silicon/NXP: Use runtime safe version of DebugLib

[Leif Lindholm]

On Wed, Jul 08, 2020 at 00:19:31 -0500, Pankaj Bansal wrote:
> From: Pankaj Bansal <pankaj.bansal@nxp.com>
> 
> For DXE_RUNTIME_DRIVER runtime safe version of DebugLib should be
> used. Otherwise, any DEBUG print in code can result in abort in OS.
> 
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>

For this patch:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
Pushed as e5c5ad8bfb03.

> ---
>  Silicon/NXP/NxpQoriqLs.dsc.inc | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Silicon/NXP/NxpQoriqLs.dsc.inc b/Silicon/NXP/NxpQoriqLs.dsc.inc
> index ee639d552483..06ee012c227a 100644
> --- a/Silicon/NXP/NxpQoriqLs.dsc.inc
> +++ b/Silicon/NXP/NxpQoriqLs.dsc.inc
> @@ -163,6 +163,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>    MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>    CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
>    ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
> +  DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>  
>  [LibraryClasses.AARCH64]
>    #
> -- 
> 2.17.1
> 

Re: [PATCH edk2-platforms 3/3] Silicon/NXP: Add Support for git commit info print

[Leif Lindholm]

On Wed, Jul 08, 2020 at 00:19:33 -0500, Pankaj Bansal wrote:
> From: Pankaj Bansal <pankaj.bansal@nxp.com>
> 
> This patch adds the Support for printing the git commit information
> in linux build environment.
> 
> Ideal place of retrieving this information should be python script in
> BaseTools.
> 
> A Feature request for the same has been created:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2838
> 
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  Silicon/NXP/NxpQoriqLs.dsc.inc                           |  3 ++
>  Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf   |  5 +++
>  Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf |  5 +++
>  Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c     | 17 +++++++++
>  Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c   | 17 +++++++++
>  Silicon/NXP/set_firmware_ver.sh                          | 36 ++++++++++++++++++++
>  6 files changed, 83 insertions(+)
> 
> diff --git a/Silicon/NXP/NxpQoriqLs.dsc.inc b/Silicon/NXP/NxpQoriqLs.dsc.inc
> index 06ee012c227a..a0762a6ef61d 100644
> --- a/Silicon/NXP/NxpQoriqLs.dsc.inc
> +++ b/Silicon/NXP/NxpQoriqLs.dsc.inc
> @@ -224,6 +224,9 @@ [PcdsDynamicHii.common.DEFAULT]
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|10
>  
>  [PcdsFixedAtBuild.common]
> +  !ifdef $(FIRMWARE_VER)
> +    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"$(FIRMWARE_VER)"
> +  !endif
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>    gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> index f5dbd1349dc5..69f884af9e34 100644
> --- a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> @@ -16,6 +16,7 @@ [Defines]
>  
>  [Packages]
>    ArmPkg/ArmPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
>    Silicon/NXP/Chassis2/Chassis2.dec
>    Silicon/NXP/NxpQoriqLs.dec
> @@ -24,6 +25,7 @@ [LibraryClasses]
>    IoAccessLib
>    IoLib
>    PcdLib
> +  PrintLib
>    SerialPortLib
>  
>  [Sources.common]
> @@ -31,3 +33,6 @@ [Sources.common]
>  
>  [FeaturePcd]
>    gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian
> +
> +[FixedPcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> diff --git a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf
> index 75b68cc4ca2d..632acc52b20a 100644
> --- a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf
> +++ b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.inf
> @@ -16,6 +16,7 @@ [Defines]
>  
>  [Packages]
>    ArmPkg/ArmPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
>    Silicon/NXP/Chassis3V2/Chassis3V2.dec
>    Silicon/NXP/NxpQoriqLs.dec
> @@ -24,6 +25,7 @@ [LibraryClasses]
>    IoAccessLib
>    IoLib
>    PcdLib
> +  PrintLib
>    SerialPortLib
>  
>  [Sources.common]
> @@ -31,3 +33,6 @@ [Sources.common]
>  
>  [FeaturePcd]
>    gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian
> +
> +[FixedPcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> index 91b19f832f00..bc782e7e3873 100644
> --- a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> @@ -12,6 +12,7 @@
>  #include <Library/IoAccessLib.h>
>  #include <Library/IoLib.h>
>  #include <Library/PcdLib.h>
> +#include <Library/PrintLib.h>
>  #include <Library/SerialPortLib.h>
>  
>  /**
> @@ -89,10 +90,26 @@ ChassisInit (
>    VOID
>    )
>  {
> +  CHAR8                         Buffer[100];
> +  UINTN                         CharCount;
> +
>    //
>    // Early init serial Port to get board information.
>    //
>    SerialPortInitialize ();
>  
> +  CharCount = AsciiSPrint (
> +                Buffer, sizeof (Buffer),
> +                "UEFI firmware built at %a on %a. version:\n\r",
> +                __TIME__, __DATE__
> +              );
> +  SerialPortWrite ((UINT8 *) Buffer, CharCount);

Drop space before Buffer.

> +
> +  CharCount = AsciiSPrint (
> +                Buffer, sizeof (Buffer), "%s\n\r",
> +                (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString)
> +              );
> +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
> +
>    SmmuInit ();
>  }
> diff --git a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> index 30f8f945b233..6d546f4754f9 100644
> --- a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> +++ b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> @@ -12,6 +12,7 @@
>  #include <Library/IoAccessLib.h>
>  #include <Library/IoLib.h>
>  #include <Library/PcdLib.h>
> +#include <Library/PrintLib.h>
>  #include <Library/SerialPortLib.h>
>  
>  /**
> @@ -64,8 +65,24 @@ ChassisInit (
>    VOID
>    )
>  {
> +  CHAR8                         Buffer[100];
> +  UINTN                         CharCount;
> +
>    //
>    // Early init serial Port to get board information.
>    //
>    SerialPortInitialize ();
> +
> +  CharCount = AsciiSPrint (
> +                Buffer, sizeof (Buffer),
> +                "UEFI firmware built at %a on %a. version:\n\r",
> +                __TIME__, __DATE__
> +              );
> +  SerialPortWrite ((UINT8 *) Buffer, CharCount);

Drop space before Buffer.

I will note here that you are adding identical content to two
different ChassisLib implementations.

This is a strong indicator that the abstraction level is currently
incorrect and should be revisited.

> +
> +  CharCount = AsciiSPrint (
> +                Buffer, sizeof (Buffer), "%s\n\r",
> +                (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString)
> +              );
> +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
>  }
> diff --git a/Silicon/NXP/set_firmware_ver.sh b/Silicon/NXP/set_firmware_ver.sh

This script doesn't *set* anything (which is good, since I would have
rejected that outright), so the name should reflect the actual function.

> new file mode 100755
> index 000000000000..ba2336ad23dc
> --- /dev/null
> +++ b/Silicon/NXP/set_firmware_ver.sh
> @@ -0,0 +1,36 @@
> +#!/bin/sh
> +#
> +# Copyright 2020 NXP
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +# parse PACKAGES_PATH and set FIRMWARE_VER based on that

This too should reflect actual function.

> +
> +get_git_version()
> +{
> +  command -v git>/dev/null 2>&1
> +  if [ $? -eq 0 ] && [ -n "$1" ]
> +  then
> +    head_or_tag=`git -C $1 describe --tag --dirty --broken --always 2>/dev/null`
> +    printf $head_or_tag
> +  fi
> +}
> +
> +if [ -n "$FIRMWARE_VER" ]; then
> +  echo "Warning FIRMWARE_VER=$FIRMWARE_VER is already set"
> +  echo "Please retry after 'unset FIRMWARE_VER' command"
> +fi

Just do
FIRMWARE_VER=
before the first code in the script.

> +
> +directories=$(echo $PACKAGES_PATH | tr ":" "\n")

Could set IFS=: instead, it would have the same effect.

> +for dir in $directories; do
> +  if [ -n "$FIRMWARE_VER" ]; then
> +    FIRMWARE_VER="$FIRMWARE_VER;$(basename $dir):$(get_git_version $dir)"
> +  else
> +    FIRMWARE_VER=$(basename $dir):$(get_git_version $dir)
> +  fi
> +done
> +
> +echo "FIRMWARE_VER=$FIRMWARE_VER"
> +export FIRMWARE_VER=$FIRMWARE_VER

What is this line supposed to achieve?

/
    Leif

> +
> +echo "build edk2 firmware with -D FIRMWARE_VER=\$FIRMWARE_VER"
> -- 
> 2.17.1
> 

Re: [PATCH edk2-platforms 2/3] Silicon/NXP: Add support for reserving a chunk from RAM

[Leif Lindholm]

On Wed, Jul 08, 2020 at 00:19:32 -0500, Pankaj Bansal wrote:
> From: Pankaj Bansal <pankaj.bansal@nxp.com>
> 
> Some NXP SOCs have some specialized IP blocks (like MC), which
> require DDR memory to operate. This DDR memory should not be managed
> by OS or UEFI.
> 
> Moreover to ensure that these IP blocks always get memory, and maximum
> contiguous RAM is available for UEFI and OS to use, add the support for
> reserving a chunk from RAM before reporting available RAM to UEFI.

I can't shake the feeling this code has the wrong level of
complexity. It's reserving *one* memory window, at a given alignment.

> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  Silicon/NXP/NxpQoriqLs.dec                                |  10 ++
>  Silicon/NXP/LX2160A/LX2160A.dsc.inc                       |   4 +
>  Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf |   3 +
>  Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c   | 142 +++++++++++++++++++-
>  4 files changed, 157 insertions(+), 2 deletions(-)
> 
> diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> index 188a9fe1f382..0e762066e547 100644
> --- a/Silicon/NXP/NxpQoriqLs.dec
> +++ b/Silicon/NXP/NxpQoriqLs.dec
> @@ -41,3 +41,13 @@ [PcdsDynamic.common]
>    gNxpQoriqLsTokenSpaceGuid.PcdPciCfgShiftEnable|FALSE|BOOLEAN|0x00000600
>    gNxpQoriqLsTokenSpaceGuid.PcdPciLsGen4Ctrl|FALSE|BOOLEAN|0x00000601
>    gNxpQoriqLsTokenSpaceGuid.PcdPciHideRootPort|FALSE|BOOLEAN|0x00000602
> +
> +  # Reserved RAM Base address alignment. This number ought to be Power of two
> +  # in case no alignment is needed, this number should be 1.

(As has been pointed out to me in the past, 1 is also a power of 2...)

> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|0x1|UINT64|0x00000603
> +  # Size of the RAM to be reserved. This RAM region is neither reported to UEFI
> +  # nor to OS

"Reported" is an inaccurate description of the mechanism involved.

> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|0x0|UINT64|0x00000604
> +  # Reserved RAM Base address which is calculated based on PcdReservedMemSize
> +  # and PcdReservedMemAlignment
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemBase|0x0|UINT64|0x00000605

Why  use a Pcd for something that is calculated at runtime and needs
to be used immediately? Moreover, I see this Pcd getting set, but I
then don't see it getting used ever?

> diff --git a/Silicon/NXP/LX2160A/LX2160A.dsc.inc b/Silicon/NXP/LX2160A/LX2160A.dsc.inc
> index 43e361464c8e..755ca169f213 100644
> --- a/Silicon/NXP/LX2160A/LX2160A.dsc.inc
> +++ b/Silicon/NXP/LX2160A/LX2160A.dsc.inc
> @@ -29,6 +29,10 @@ [PcdsDynamicDefault.common]
>    gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x6200000
>    gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0xC0C0000
>  
> +[PcdsDynamicHii]
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|L"ReservedMemAlignment"|gEfiGlobalVariableGuid|0x0|0x20000000|NV,BS
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|L"ReservedMemSize"|gEfiGlobalVariableGuid|0x0|0x20000000|NV,BS
> +

And here we get Hii involved, but I see no connection to a user
interface form.

>  [PcdsFixedAtBuild.common]
>    gArmTokenSpaceGuid.PcdGenericWatchdogControlBase|0x23A0000
>    gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase|0x2390000
> diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> index a33f8cd3f743..ed23a86b43d9 100644
> --- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> +++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> @@ -49,6 +49,9 @@ [FixedPcd]
>  [Pcd]
>    gArmTokenSpaceGuid.PcdSystemMemoryBase
>    gArmTokenSpaceGuid.PcdSystemMemorySize
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemBase
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize
>  
>  [Depex]
>    TRUE
> diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> index 11d1f1260b35..b416323a4ced 100644
> --- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> +++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> @@ -118,6 +118,127 @@ GetDramRegionsInfo (
>    return EFI_BUFFER_TOO_SMALL;
>  }
>  
> +/**
> +  Calculate the base address of Reserved RAM.
> +  Reserved RAM is not reported to either UEFI or OS.

Here is the word "reported" again.
A region is included in the memory map or not.
If we are setting parts of the memory map aside, then we should do
that by marking them as EfiReservedMemoryType.

> +
> +  @param[in, out] DramRegions  Array of type DRAM_REGION_INFO. The size of this
> +                               array must be one more (+ 1) than the maximum
> +                               regions supported on platform. This is because,
> +                               if due to Reserved RAM alignment requirements a
> +                               hole is created in any DRAM region, then the RAM
> +                               after hole gets reported to UEFI and then
> +                               subsequently to OS. which is why, the last entry
> +                               of this array will not be parsed while
> +                               calculating Reserved RAM base address. Caller
> +                               must ensure that last entry of this array is zero
> +                               initialized.
> +  @param[in] NumRegions        Size of DramRegions array (including +1 for hole)
> +  @param[in] ReservedMemSize   Size of RAM to be reserved.
> +
> +  @return  if successful Address of the Reserved RAM region, 0 otherwise.
> +**/
> +STATIC
> +UINTN
> +CalculateReservedMemBase (
> +  IN DRAM_REGION_INFO *DramRegions,
> +  IN UINT32           NumRegions,
> +  IN UINTN            ReservedMemSize
> +)
> +{
> +  UINTN                 ReservedMemAlignment;
> +  EFI_PHYSICAL_ADDRESS  AlignmentMask;
> +  UINTN                 RegionBaseAddress;
> +  UINTN                 RegionSize;
> +  UINTN                 ReservedBaseAddress;
> +  INTN                  Index;
> +  INTN                  Index2;
> +
> +  ReservedMemAlignment = PcdGet64 (PcdReservedMemAlignment);
> +  //
> +  // Compute alignment bit mask
> +  //
> +  if (ReservedMemAlignment) {
> +    AlignmentMask = LShiftU64 (1, LowBitSet64(ReservedMemAlignment)) - 1;
> +  } else {
> +    AlignmentMask = 0;
> +  }
> +
> +  // The DRAM region info is sorted based on the RAM address is SOC memory map.
> +  // i.e. DramRegions[0] is at lower address, as compared to DramRegions[1].
> +  // The goal to start from last region is to find the topmost RAM region that
> +  // can contain Reserved RAM region i.e. PcdReservedMemSize.
> +  // Since this RAM is not reported to either UEFI or OS, 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.
> +  for (Index = NumRegions - 2; Index >=0; Index--) {
> +    RegionBaseAddress = DramRegions[Index].BaseAddress;
> +    RegionSize = DramRegions[Index].Size;
> +
> +    if (ReservedMemSize > RegionSize) {
> +      continue;
> +    }
> +
> +    ReservedBaseAddress = (RegionBaseAddress + RegionSize - ReservedMemSize);
> +    ReservedBaseAddress &= (~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 == 0) {
> +      // delete the region but maintain the sorted list of regions
> +      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 == 0) {
> +      break;
> +    }
> +
> +    // A hole has been created in DRAM regions due to Reserved RAM alignment
> +    // requirements. create a new DRAM region for DRAM memory after hole.
> +    // Maintain the sorted list of regions
> +    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;
> +  }
> +}
> +
>  /**
>    Get the installed RAM information.
>    Initialize Memory HOBs (Resource Descriptor HOBs)
> @@ -135,7 +256,9 @@ MemoryInitPeiLibConstructor (
>    UINTN                         BaseAddress;
>    UINTN                         Size;
>    UINTN                         Top;
> -  DRAM_REGION_INFO              DramRegions[MAX_DRAM_REGIONS];
> +  // Extra region gets created if we want to reserve a memory region and that
> +  // creates a memory hole because of alignment requirements
> +  DRAM_REGION_INFO              DramRegions[MAX_DRAM_REGIONS + 1];

No. Just no.

>    EFI_RESOURCE_ATTRIBUTE_TYPE   ResourceAttributes;
>    UINTN                         FdBase;
>    UINTN                         FdTop;
> @@ -155,6 +278,21 @@ MemoryInitPeiLibConstructor (
>  
>    (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
>  
> +  // Get the reserved memory size from non volatile storage
> +  Size = PcdGet64 (PcdReservedMemSize);
> +  if (Size) {
> +    BaseAddress = CalculateReservedMemBase (
> +                    DramRegions,
> +                    ARRAY_SIZE (DramRegions),
> +                    Size
> +                  );
> +    if (BaseAddress) {
> +      DEBUG ((DEBUG_INFO, "ReservedMem: start 0x%lx, size 0x%lx\n",
> +        BaseAddress, Size));
> +      PcdSet64S (PcdReservedMemBase, BaseAddress);

PcdReservedMemBase set but never used.

And how do the other controllers find out about what region has ended
up being set aside for them? At what point during the boot process do
they need it?

/
    Leif

> +    }
> +  }
> +
>    FdBase = (UINTN)PcdGet64 (PcdFdBaseAddress);
>    FdTop = FdBase + (UINTN)PcdGet32 (PcdFdSize);
>  
> @@ -168,7 +306,7 @@ MemoryInitPeiLibConstructor (
>    // 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.
> -  for (Index = MAX_DRAM_REGIONS - 1; Index >= 0; Index--) {
> +  for (Index = MAX_DRAM_REGIONS; Index >= 0; Index--) {
>      if (DramRegions[Index].Size == 0) {
>        continue;
>      }
> -- 
> 2.17.1
> 

Re: [EXTERNAL] Re: [edk2-devel] [PATCH edk2-platforms 2/3] Silicon/NXP: Add support for reserving a chunk from RAM

[Bret Barkelew]

[-- Attachment #1: Type: text/plain, Size: 12080 bytes --]

Agreed with your gut feeling.

- Bret

From: Leif Lindholm via groups.io<mailto:leif=nuviainc.com@groups.io>
Sent: Wednesday, August 5, 2020 8:12 AM
To: Pankaj Bansal<mailto:pankaj.bansal@oss.nxp.com>
Cc: Meenakshi Aggarwal<mailto:meenakshi.aggarwal@nxp.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ard Biesheuvel<mailto:ard.biesheuvel@linaro.org>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH edk2-platforms 2/3] Silicon/NXP: Add support for reserving a chunk from RAM

On Wed, Jul 08, 2020 at 00:19:32 -0500, Pankaj Bansal wrote:
> From: Pankaj Bansal <pankaj.bansal@nxp.com>
>
> Some NXP SOCs have some specialized IP blocks (like MC), which
> require DDR memory to operate. This DDR memory should not be managed
> by OS or UEFI.
>
> Moreover to ensure that these IP blocks always get memory, and maximum
> contiguous RAM is available for UEFI and OS to use, add the support for
> reserving a chunk from RAM before reporting available RAM to UEFI.

I can't shake the feeling this code has the wrong level of
complexity. It's reserving *one* memory window, at a given alignment.

> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  Silicon/NXP/NxpQoriqLs.dec                                |  10 ++
>  Silicon/NXP/LX2160A/LX2160A.dsc.inc                       |   4 +
>  Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf |   3 +
>  Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c   | 142 +++++++++++++++++++-
>  4 files changed, 157 insertions(+), 2 deletions(-)
>
> diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> index 188a9fe1f382..0e762066e547 100644
> --- a/Silicon/NXP/NxpQoriqLs.dec
> +++ b/Silicon/NXP/NxpQoriqLs.dec
> @@ -41,3 +41,13 @@ [PcdsDynamic.common]
>    gNxpQoriqLsTokenSpaceGuid.PcdPciCfgShiftEnable|FALSE|BOOLEAN|0x00000600
>    gNxpQoriqLsTokenSpaceGuid.PcdPciLsGen4Ctrl|FALSE|BOOLEAN|0x00000601
>    gNxpQoriqLsTokenSpaceGuid.PcdPciHideRootPort|FALSE|BOOLEAN|0x00000602
> +
> +  # Reserved RAM Base address alignment. This number ought to be Power of two
> +  # in case no alignment is needed, this number should be 1.

(As has been pointed out to me in the past, 1 is also a power of 2...)

> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|0x1|UINT64|0x00000603
> +  # Size of the RAM to be reserved. This RAM region is neither reported to UEFI
> +  # nor to OS

"Reported" is an inaccurate description of the mechanism involved.

> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|0x0|UINT64|0x00000604
> +  # Reserved RAM Base address which is calculated based on PcdReservedMemSize
> +  # and PcdReservedMemAlignment
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemBase|0x0|UINT64|0x00000605

Why  use a Pcd for something that is calculated at runtime and needs
to be used immediately? Moreover, I see this Pcd getting set, but I
then don't see it getting used ever?

> diff --git a/Silicon/NXP/LX2160A/LX2160A.dsc.inc b/Silicon/NXP/LX2160A/LX2160A.dsc.inc
> index 43e361464c8e..755ca169f213 100644
> --- a/Silicon/NXP/LX2160A/LX2160A.dsc.inc
> +++ b/Silicon/NXP/LX2160A/LX2160A.dsc.inc
> @@ -29,6 +29,10 @@ [PcdsDynamicDefault.common]
>    gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x6200000
>    gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0xC0C0000
>
> +[PcdsDynamicHii]
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|L"ReservedMemAlignment"|gEfiGlobalVariableGuid|0x0|0x20000000|NV,BS
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|L"ReservedMemSize"|gEfiGlobalVariableGuid|0x0|0x20000000|NV,BS
> +

And here we get Hii involved, but I see no connection to a user
interface form.

>  [PcdsFixedAtBuild.common]
>    gArmTokenSpaceGuid.PcdGenericWatchdogControlBase|0x23A0000
>    gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase|0x2390000
> diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> index a33f8cd3f743..ed23a86b43d9 100644
> --- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> +++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> @@ -49,6 +49,9 @@ [FixedPcd]
>  [Pcd]
>    gArmTokenSpaceGuid.PcdSystemMemoryBase
>    gArmTokenSpaceGuid.PcdSystemMemorySize
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemBase
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize
>
>  [Depex]
>    TRUE
> diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> index 11d1f1260b35..b416323a4ced 100644
> --- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> +++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> @@ -118,6 +118,127 @@ GetDramRegionsInfo (
>    return EFI_BUFFER_TOO_SMALL;
>  }
>
> +/**
> +  Calculate the base address of Reserved RAM.
> +  Reserved RAM is not reported to either UEFI or OS.

Here is the word "reported" again.
A region is included in the memory map or not.
If we are setting parts of the memory map aside, then we should do
that by marking them as EfiReservedMemoryType.

> +
> +  @param[in, out] DramRegions  Array of type DRAM_REGION_INFO. The size of this
> +                               array must be one more (+ 1) than the maximum
> +                               regions supported on platform. This is because,
> +                               if due to Reserved RAM alignment requirements a
> +                               hole is created in any DRAM region, then the RAM
> +                               after hole gets reported to UEFI and then
> +                               subsequently to OS. which is why, the last entry
> +                               of this array will not be parsed while
> +                               calculating Reserved RAM base address. Caller
> +                               must ensure that last entry of this array is zero
> +                               initialized.
> +  @param[in] NumRegions        Size of DramRegions array (including +1 for hole)
> +  @param[in] ReservedMemSize   Size of RAM to be reserved.
> +
> +  @return  if successful Address of the Reserved RAM region, 0 otherwise.
> +**/
> +STATIC
> +UINTN
> +CalculateReservedMemBase (
> +  IN DRAM_REGION_INFO *DramRegions,
> +  IN UINT32           NumRegions,
> +  IN UINTN            ReservedMemSize
> +)
> +{
> +  UINTN                 ReservedMemAlignment;
> +  EFI_PHYSICAL_ADDRESS  AlignmentMask;
> +  UINTN                 RegionBaseAddress;
> +  UINTN                 RegionSize;
> +  UINTN                 ReservedBaseAddress;
> +  INTN                  Index;
> +  INTN                  Index2;
> +
> +  ReservedMemAlignment = PcdGet64 (PcdReservedMemAlignment);
> +  //
> +  // Compute alignment bit mask
> +  //
> +  if (ReservedMemAlignment) {
> +    AlignmentMask = LShiftU64 (1, LowBitSet64(ReservedMemAlignment)) - 1;
> +  } else {
> +    AlignmentMask = 0;
> +  }
> +
> +  // The DRAM region info is sorted based on the RAM address is SOC memory map.
> +  // i.e. DramRegions[0] is at lower address, as compared to DramRegions[1].
> +  // The goal to start from last region is to find the topmost RAM region that
> +  // can contain Reserved RAM region i.e. PcdReservedMemSize.
> +  // Since this RAM is not reported to either UEFI or OS, 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.
> +  for (Index = NumRegions - 2; Index >=0; Index--) {
> +    RegionBaseAddress = DramRegions[Index].BaseAddress;
> +    RegionSize = DramRegions[Index].Size;
> +
> +    if (ReservedMemSize > RegionSize) {
> +      continue;
> +    }
> +
> +    ReservedBaseAddress = (RegionBaseAddress + RegionSize - ReservedMemSize);
> +    ReservedBaseAddress &= (~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 == 0) {
> +      // delete the region but maintain the sorted list of regions
> +      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 == 0) {
> +      break;
> +    }
> +
> +    // A hole has been created in DRAM regions due to Reserved RAM alignment
> +    // requirements. create a new DRAM region for DRAM memory after hole.
> +    // Maintain the sorted list of regions
> +    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;
> +  }
> +}
> +
>  /**
>    Get the installed RAM information.
>    Initialize Memory HOBs (Resource Descriptor HOBs)
> @@ -135,7 +256,9 @@ MemoryInitPeiLibConstructor (
>    UINTN                         BaseAddress;
>    UINTN                         Size;
>    UINTN                         Top;
> -  DRAM_REGION_INFO              DramRegions[MAX_DRAM_REGIONS];
> +  // Extra region gets created if we want to reserve a memory region and that
> +  // creates a memory hole because of alignment requirements
> +  DRAM_REGION_INFO              DramRegions[MAX_DRAM_REGIONS + 1];

No. Just no.

>    EFI_RESOURCE_ATTRIBUTE_TYPE   ResourceAttributes;
>    UINTN                         FdBase;
>    UINTN                         FdTop;
> @@ -155,6 +278,21 @@ MemoryInitPeiLibConstructor (
>
>    (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
>
> +  // Get the reserved memory size from non volatile storage
> +  Size = PcdGet64 (PcdReservedMemSize);
> +  if (Size) {
> +    BaseAddress = CalculateReservedMemBase (
> +                    DramRegions,
> +                    ARRAY_SIZE (DramRegions),
> +                    Size
> +                  );
> +    if (BaseAddress) {
> +      DEBUG ((DEBUG_INFO, "ReservedMem: start 0x%lx, size 0x%lx\n",
> +        BaseAddress, Size));
> +      PcdSet64S (PcdReservedMemBase, BaseAddress);

PcdReservedMemBase set but never used.

And how do the other controllers find out about what region has ended
up being set aside for them? At what point during the boot process do
they need it?

/
    Leif

> +    }
> +  }
> +
>    FdBase = (UINTN)PcdGet64 (PcdFdBaseAddress);
>    FdTop = FdBase + (UINTN)PcdGet32 (PcdFdSize);
>
> @@ -168,7 +306,7 @@ MemoryInitPeiLibConstructor (
>    // 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.
> -  for (Index = MAX_DRAM_REGIONS - 1; Index >= 0; Index--) {
> +  for (Index = MAX_DRAM_REGIONS; Index >= 0; Index--) {
>      if (DramRegions[Index].Size == 0) {
>        continue;
>      }
> --
> 2.17.1
>




[-- Attachment #2: Type: text/html, Size: 21515 bytes --]

Re: [PATCH edk2-platforms 3/3] Silicon/NXP: Add Support for git commit info print

[Pankaj Bansal]

> > index 91b19f832f00..bc782e7e3873 100644
> > --- a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> > +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> > @@ -12,6 +12,7 @@
> >  #include <Library/IoAccessLib.h>
> >  #include <Library/IoLib.h>
> >  #include <Library/PcdLib.h>
> > +#include <Library/PrintLib.h>
> >  #include <Library/SerialPortLib.h>
> >
> >  /**
> > @@ -89,10 +90,26 @@ ChassisInit (
> >    VOID
> >    )
> >  {
> > +  CHAR8                         Buffer[100];
> > +  UINTN                         CharCount;
> > +
> >    //
> >    // Early init serial Port to get board information.
> >    //
> >    SerialPortInitialize ();
> >
> > +  CharCount = AsciiSPrint (
> > +                Buffer, sizeof (Buffer),
> > +                "UEFI firmware built at %a on %a. version:\n\r",
> > +                __TIME__, __DATE__
> > +              );
> > +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
> 
> Drop space before Buffer.

OK.

> 
> > +
> > +  CharCount = AsciiSPrint (
> > +                Buffer, sizeof (Buffer), "%s\n\r",
> > +                (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString)
> > +              );
> > +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
> > +
> >    SmmuInit ();
> >  }
> > diff --git a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> > index 30f8f945b233..6d546f4754f9 100644
> > --- a/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> > +++ b/Silicon/NXP/Chassis3V2/Library/ChassisLib/ChassisLib.c
> > @@ -12,6 +12,7 @@
> >  #include <Library/IoAccessLib.h>
> >  #include <Library/IoLib.h>
> >  #include <Library/PcdLib.h>
> > +#include <Library/PrintLib.h>
> >  #include <Library/SerialPortLib.h>
> >
> >  /**
> > @@ -64,8 +65,24 @@ ChassisInit (
> >    VOID
> >    )
> >  {
> > +  CHAR8                         Buffer[100];
> > +  UINTN                         CharCount;
> > +
> >    //
> >    // Early init serial Port to get board information.
> >    //
> >    SerialPortInitialize ();
> > +
> > +  CharCount = AsciiSPrint (
> > +                Buffer, sizeof (Buffer),
> > +                "UEFI firmware built at %a on %a. version:\n\r",
> > +                __TIME__, __DATE__
> > +              );
> > +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
> 
> Drop space before Buffer.

OK.

> 
> I will note here that you are adding identical content to two
> different ChassisLib implementations.
> 
> This is a strong indicator that the abstraction level is currently
> incorrect and should be revisited.
> 

I am preparing the patches to get the core/cluster info in NXP SOCs.
I will move the version print in that code.

> > +
> > +  CharCount = AsciiSPrint (
> > +                Buffer, sizeof (Buffer), "%s\n\r",
> > +                (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString)
> > +              );
> > +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
> >  }
> > diff --git a/Silicon/NXP/set_firmware_ver.sh
> b/Silicon/NXP/set_firmware_ver.sh
> 
> This script doesn't *set* anything (which is good, since I would have
> rejected that outright), so the name should reflect the actual function.

it *does* set the FIRMWARE_VER environment variable.
FIRMWARE_VER is just like WORKSPACE and PACKAGES_PATH.

> 
> > new file mode 100755
> > index 000000000000..ba2336ad23dc
> > --- /dev/null
> > +++ b/Silicon/NXP/set_firmware_ver.sh
> > @@ -0,0 +1,36 @@
> > +#!/bin/sh
> > +#
> > +# Copyright 2020 NXP
> > +#
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +# parse PACKAGES_PATH and set FIRMWARE_VER based on that
> 
> This too should reflect actual function.

see explanation above.

> 
> > +
> > +get_git_version()
> > +{
> > +  command -v git>/dev/null 2>&1
> > +  if [ $? -eq 0 ] && [ -n "$1" ]
> > +  then
> > +    head_or_tag=`git -C $1 describe --tag --dirty --broken --always 2>/dev/null`
> > +    printf $head_or_tag
> > +  fi
> > +}
> > +
> > +if [ -n "$FIRMWARE_VER" ]; then
> > +  echo "Warning FIRMWARE_VER=$FIRMWARE_VER is already set"
> > +  echo "Please retry after 'unset FIRMWARE_VER' command"
> > +fi
> 
> Just do
> FIRMWARE_VER=
> before the first code in the script.

OK.

> 
> > +
> > +directories=$(echo $PACKAGES_PATH | tr ":" "\n")
> 
> Could set IFS=: instead, it would have the same effect.

Good suggestion. will modify.

> 
> > +for dir in $directories; do
> > +  if [ -n "$FIRMWARE_VER" ]; then
> > +    FIRMWARE_VER="$FIRMWARE_VER;$(basename $dir):$(get_git_version
> $dir)"
> > +  else
> > +    FIRMWARE_VER=$(basename $dir):$(get_git_version $dir)
> > +  fi
> > +done
> > +
> > +echo "FIRMWARE_VER=$FIRMWARE_VER"
> > +export FIRMWARE_VER=$FIRMWARE_VER
> 
> What is this line supposed to achieve?

It would set the FIRMWARE_VER environment variable when "set_firmware_ver.sh" script is executed.
After that user can build platform firmware with -D FIRMWARE_VER=$ FIRMWARE_VER flag.
I have put this suggestion for users in "set_firmware_ver.sh" script.

> 
> /
>     Leif
> 
> > +
> > +echo "build edk2 firmware with -D FIRMWARE_VER=\$FIRMWARE_VER"
> > --
> > 2.17.1
> >

Re: [PATCH edk2-platforms 3/3] Silicon/NXP: Add Support for git commit info print

[Leif Lindholm]

Hi Pankaj,

Apologies for slow response - having a bit of a heatwave and no AC...

On Sat, Aug 08, 2020 at 02:39:32 +0000, Pankaj Bansal (OSS) wrote:
> > I will note here that you are adding identical content to two
> > different ChassisLib implementations.
> > 
> > This is a strong indicator that the abstraction level is currently
> > incorrect and should be revisited.
> 
> I am preparing the patches to get the core/cluster info in NXP SOCs.
> I will move the version print in that code.

Works for me.

> > > +
> > > +  CharCount = AsciiSPrint (
> > > +                Buffer, sizeof (Buffer), "%s\n\r",
> > > +                (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString)
> > > +              );
> > > +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
> > >  }
> > > diff --git a/Silicon/NXP/set_firmware_ver.sh
> > b/Silicon/NXP/set_firmware_ver.sh
> > 
> > This script doesn't *set* anything (which is good, since I would have
> > rejected that outright), so the name should reflect the actual function.
> 
> it *does* set the FIRMWARE_VER environment variable.
> FIRMWARE_VER is just like WORKSPACE and PACKAGES_PATH.

1) OK, so the expectation is that this script should be sourced?
   This is not mentioned in the commit message, in comments in the
   script itself, and it does not warn you about that if you simply
   run it.
2) It's not like WORKSPACE or PACKAGES_PATH. Nothing in the EDK2
   BuildTools environment looks at FIRMWARE_VER.
3) It relies on PACKAGES_PATH being set in the environment.

So if I try to deduce the undocumented intended build steps, this
patch intends for the user to execute
$ . <edk2-platforms>/Silicon/NXP/set_firmware_ver.sh
which then prints for me that I should
"build edk2 firmware with -D FIRMWARE_VER=$FIRMWARE_VER"

This is not something that is of any use outside of your own build
infrastructure, so I would suggest you keep it downstream.

Having a generic (and portable) way of printing what you're currently
building isn't a bad idea however. If you were willing to create a
script for BaseTools based on roughly the below mechanics:

---

from __future__ import print_function
import os
import git

PACKAGES = os.environ['PACKAGES_PATH'].split(':')
# Trim empty entries caused by leading/trailing delimiter
try:
    while True:
        PACKAGES.remove('')
except ValueError:
    pass

for package in PACKAGES:
    REPO = git.Repo(path=package)
    HEAD = REPO.head
    SHA = REPO.git.rev_parse(HEAD, short=12)
    if REPO.is_dirty():
        STATE = 'dirty'
    else:
        STATE = 'clean'
    print('%s: %s-%s' % (os.path.basename(package), SHA, STATE))

---

I think that would be a lot more useful. If you were to add some
BinWrappers as well, you could then add in your build instructions to
use -D FIRMWARE_VER="`commandname`"

/
    Leif