From: Pankaj Bansal <pankaj.bansal@nxp.com>
RAM retrieval using SMC commands is common to all Layerscape SOCs.
Therefore, move it to common MemoryInit Pei Lib.
Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
Notes:
V4:
- fixed line adds white space error in MemoryInitPeiLib.h
- Added SMC_OK and SMC_UOK Macros to denote the return values from
SMC calls
- Added explanation for SMC_DRAM_BANK_INFO and DRAM_REGION_INFO in
MemoryInitPeiLib.h
- Modified GetDramSize to check for return value of SMC call against
SMC_OK. Also added comments when returning 0 from this function
- Modified GetDramRegionsInfo for loop and return values as per Leif's
suggestion. Also added DEBUG_ERROR in case of return BUFFER_TOO_SMALL.
- Added SMC_OK in GetDramRegionsInfo
- Check for GetDramRegionsInfo return value in MemoryPeim
- regios -> regions
V3:
- sort headers alphabetically
- Moved DRAM region retrieval and Total DRAM size retrieval to separate
functions
- Fixed MemoryPeim function description
- Modified check on FoundSystemMem = TRUE to check the RAM region against
MemoryPeim function input arguments UefiMemoryBase and UefiMemorySize
- (!DramRegions[Index].Size) => (DramRegions[Index].Size == 0)
- (FoundSystemMem) => (FoundSystemMem == TRUE)
- Added explanation for starting for loop from the last DRAM region
Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf | 7 +-
Silicon/NXP/Library/SocLib/LS1043aSocLib.inf | 1 -
Silicon/NXP/Include/DramInfo.h | 38 ----
Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h | 38 ++++
Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c | 205 ++++++++++++++++----
Silicon/NXP/Library/SocLib/Chassis.c | 67 -------
6 files changed, 211 insertions(+), 145 deletions(-)
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 @@
[Sources]
MemoryInitPeiLib.c
-
[Packages]
ArmPkg/ArmPkg.dec
ArmPlatformPkg/ArmPlatformPkg.dec
@@ -30,6 +29,7 @@
[LibraryClasses]
ArmMmuLib
ArmPlatformLib
+ ArmSmcLib
DebugLib
HobLib
PcdLib
@@ -40,6 +40,11 @@
[FeaturePcd]
gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob
+[FixedPcd]
+ gArmTokenSpaceGuid.PcdFdBaseAddress
+ gArmTokenSpaceGuid.PcdFdSize
+ gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize
+
[Pcd]
gArmTokenSpaceGuid.PcdSystemMemoryBase
gArmTokenSpaceGuid.PcdSystemMemorySize
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 @@
Silicon/NXP/NxpQoriqLs.dec
[LibraryClasses]
- ArmSmcLib
BaseLib
DebugLib
IoAccessLib
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.h b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h
new file mode 100644
index 000000000000..7a41f4d226f1
--- /dev/null
+++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h
@@ -0,0 +1,38 @@
+/** @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
+
+// Unique SMC call to retrieve the total DDR RAM size installed in system
+// and the SOC memory map regions to which DDR RAM is mapped
+// This SMC call works in this way:
+// x1 = -1 : return x0: SMC_OK, x1: total DDR Ram size
+// x1 >= number of DRAM regions to which DDR RAM is mapped : return x0: SMC_UNK
+// 0 <= x1 < number of DRAM regions to which DDR RAM is mapped : return
+// x0: SMC_OK, x1: Base address of DRAM region,
+// x2: Size of DRAM region
+#define SMC_DRAM_BANK_INFO (0xC200FF12)
+
+// Return values from SMC calls. return values are always in x0
+#define SMC_OK 0
+#define SMC_UNK -1
+
+// Regions in SOC memory map to which DDR RAM is mapped.
+typedef struct {
+ UINTN BaseAddress;
+ UINTN Size;
+} DRAM_REGION_INFO;
+
+#endif // MEMORY_INIT_PEI_LIB_H_
diff --git a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c
index 3ea773678667..905d326893e5 100644
--- a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c
+++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c
@@ -12,13 +12,15 @@
#include <Library/ArmMmuLib.h>
#include <Library/ArmPlatformLib.h>
+#include <Library/ArmSmcLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/HobLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/PcdLib.h>
-#include <DramInfo.h>
+#include "MemoryInitPeiLib.h"
+
VOID
BuildMemoryTypeInformationHob (
@@ -44,22 +46,88 @@ InitMmu (
}
}
-/*++
+STATIC
+UINTN
+GetDramSize (
+ IN VOID
+ )
+{
+ ARM_SMC_ARGS ArmSmcArgs;
-Routine Description:
+ ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
+ ArmSmcArgs.Arg1 = -1;
+ ArmCallSmc (&ArmSmcArgs);
+ if (ArmSmcArgs.Arg0 == SMC_OK) {
+ return ArmSmcArgs.Arg1;
+ }
-Arguments:
+ // return 0 means no DDR found.
+ return 0;
+}
- FileHandle - Handle of the file being invoked.
- PeiServices - Describes the list of possible PEI Services.
+STATIC
+EFI_STATUS
+GetDramRegionsInfo (
+ OUT DRAM_REGION_INFO *DramRegions,
+ IN UINT32 NumRegions
+ )
+{
+ ARM_SMC_ARGS ArmSmcArgs;
+ UINT32 Index;
+ UINTN RemainingDramSize;
+ UINTN BaseAddress;
+ UINTN Size;
-Returns:
+ RemainingDramSize = GetDramSize ();
+ DEBUG ((DEBUG_INFO, "DRAM Total Size 0x%lx \n", RemainingDramSize));
- Status - EFI_SUCCESS if the boot mode could be set
+ // Ensure Total Dram Size is valid
+ ASSERT (RemainingDramSize != 0);
---*/
+ for (Index = 0; Index < NumRegions; Index++) {
+ ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
+ ArmSmcArgs.Arg1 = Index;
+
+ ArmCallSmc (&ArmSmcArgs);
+
+ if (ArmSmcArgs.Arg0 == SMC_OK) {
+ BaseAddress = ArmSmcArgs.Arg1;
+ Size = ArmSmcArgs.Arg2;
+ ASSERT (BaseAddress && Size);
+
+ DramRegions[Index].BaseAddress = BaseAddress;
+ DramRegions[Index].Size = Size;
+ RemainingDramSize -= Size;
+
+ DEBUG ((DEBUG_INFO, "DRAM Region[%d]: start 0x%lx, size 0x%lx\n",
+ Index, BaseAddress, Size));
+
+ if (RemainingDramSize == 0) {
+ return EFI_SUCCESS;
+ }
+ } else {
+ break;
+ }
+ }
+
+ DEBUG ((DEBUG_ERROR, "RemainingDramSize = %u !! Ensure that all DDR regions "
+ "have been accounted for\n", RemainingDramSize));
+
+ return EFI_BUFFER_TOO_SMALL;
+}
+
+/**
+ Get the installed RAM information.
+ Initialize MMU and Memory HOBs (Resource Descriptor HOBs)
+
+ @param[in] UefiMemoryBase Base address of region used by UEFI in
+ permanent memory
+ @param[in] UefiMemorySize Size of the region used by UEFI in permanent memory
+
+ @return EFI_SUCCESS Successfuly Initialize MMU and Memory HOBs.
+**/
EFI_STATUS
EFIAPI
MemoryPeim (
@@ -67,11 +135,17 @@ MemoryPeim (
IN UINT64 UefiMemorySize
)
{
- ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
- EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
- EFI_PEI_HOB_POINTERS NextHob;
- BOOLEAN Found;
- DRAM_INFO DramInfo;
+ ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
+ INT32 Index;
+ UINTN BaseAddress;
+ UINTN Size;
+ UINTN Top;
+ DRAM_REGION_INFO DramRegions[MAX_DRAM_REGIONS];
+ EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
+ UINTN FdBase;
+ UINTN FdTop;
+ BOOLEAN FoundSystemMem;
+ EFI_STATUS Status;
// Get Virtual Memory Map from the Platform Library
ArmPlatformGetVirtualMemoryMap (&MemoryTable);
@@ -94,40 +168,95 @@ MemoryPeim (
EFI_RESOURCE_ATTRIBUTE_TESTED
);
- if (GetDramBankInfo (&DramInfo)) {
- DEBUG ((DEBUG_ERROR, "Failed to get DRAM information, exiting...\n"));
- return EFI_UNSUPPORTED;
- }
+ FoundSystemMem = FALSE;
+ ZeroMem (DramRegions, sizeof (DramRegions));
- 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;
- }
- NextHob.Raw = GET_NEXT_HOB (NextHob);
+ Status = GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
+ ASSERT_EFI_ERROR (Status);
+
+ FdBase = (UINTN)FixedPcdGet64 (PcdFdBaseAddress);
+ FdTop = FdBase + (UINTN)FixedPcdGet32 (PcdFdSize);
+
+ // Declare memory regions to system
+ // 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 UEFI DXE region i.e. PcdSystemMemoryUefiRegionSize.
+ // 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.
+ for (Index = MAX_DRAM_REGIONS - 1; Index >= 0; Index--) {
+ if (DramRegions[Index].Size == 0) {
+ 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 == TRUE) {
+ continue;
+ }
+
+ Size = DramRegions[Index].Size;
+
+ if (FdBase >= BaseAddress && FdTop <= Top) {
+ Size -= (UINTN)FixedPcdGet32 (PcdFdSize);
+ }
+
+ if ((UefiMemoryBase >= BaseAddress) && (Size >= UefiMemorySize)) {
+ FoundSystemMem = TRUE;
}
}
+ ASSERT (FoundSystemMem == TRUE);
+
// Build Memory Allocation Hob
InitMmu (MemoryTable);
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;
-}
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58379): https://edk2.groups.io/g/devel/message/58379
Mute This Topic: https://groups.io/mt/73370132/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Pankaj,
I have a few minor comments on this patch, but I have pushed 1-11/24
as ca1a7187861c..725d1198e262.
On Fri, May 01, 2020 at 11:19:43 +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 common MemoryInit Pei Lib.
>
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>
> Notes:
> V4:
> - fixed line adds white space error in MemoryInitPeiLib.h
> - Added SMC_OK and SMC_UOK Macros to denote the return values from
> SMC calls
> - Added explanation for SMC_DRAM_BANK_INFO and DRAM_REGION_INFO in
> MemoryInitPeiLib.h
> - Modified GetDramSize to check for return value of SMC call against
> SMC_OK. Also added comments when returning 0 from this function
> - Modified GetDramRegionsInfo for loop and return values as per Leif's
> suggestion. Also added DEBUG_ERROR in case of return BUFFER_TOO_SMALL.
> - Added SMC_OK in GetDramRegionsInfo
> - Check for GetDramRegionsInfo return value in MemoryPeim
> - regios -> regions
>
> V3:
> - sort headers alphabetically
> - Moved DRAM region retrieval and Total DRAM size retrieval to separate
> functions
> - Fixed MemoryPeim function description
> - Modified check on FoundSystemMem = TRUE to check the RAM region against
> MemoryPeim function input arguments UefiMemoryBase and UefiMemorySize
> - (!DramRegions[Index].Size) => (DramRegions[Index].Size == 0)
> - (FoundSystemMem) => (FoundSystemMem == TRUE)
> - Added explanation for starting for loop from the last DRAM region
>
> Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf | 7 +-
> Silicon/NXP/Library/SocLib/LS1043aSocLib.inf | 1 -
> Silicon/NXP/Include/DramInfo.h | 38 ----
> Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h | 38 ++++
> Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c | 205 ++++++++++++++++----
> Silicon/NXP/Library/SocLib/Chassis.c | 67 -------
> 6 files changed, 211 insertions(+), 145 deletions(-)
>
> 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 @@
> [Sources]
> MemoryInitPeiLib.c
>
> -
> [Packages]
> ArmPkg/ArmPkg.dec
> ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -30,6 +29,7 @@
> [LibraryClasses]
> ArmMmuLib
> ArmPlatformLib
> + ArmSmcLib
> DebugLib
> HobLib
> PcdLib
> @@ -40,6 +40,11 @@
> [FeaturePcd]
> gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob
>
> +[FixedPcd]
> + gArmTokenSpaceGuid.PcdFdBaseAddress
> + gArmTokenSpaceGuid.PcdFdSize
> + gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize
> +
> [Pcd]
> gArmTokenSpaceGuid.PcdSystemMemoryBase
> gArmTokenSpaceGuid.PcdSystemMemorySize
> 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 @@
> Silicon/NXP/NxpQoriqLs.dec
>
> [LibraryClasses]
> - ArmSmcLib
> BaseLib
> DebugLib
> IoAccessLib
> 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.h b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h
> new file mode 100644
> index 000000000000..7a41f4d226f1
> --- /dev/null
> +++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h
> @@ -0,0 +1,38 @@
> +/** @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
> +
> +// Unique SMC call to retrieve the total DDR RAM size installed in system
> +// and the SOC memory map regions to which DDR RAM is mapped
> +// This SMC call works in this way:
> +// x1 = -1 : return x0: SMC_OK, x1: total DDR Ram size
> +// x1 >= number of DRAM regions to which DDR RAM is mapped : return x0: SMC_UNK
> +// 0 <= x1 < number of DRAM regions to which DDR RAM is mapped : return
> +// x0: SMC_OK, x1: Base address of DRAM region,
> +// x2: Size of DRAM region
> +#define SMC_DRAM_BANK_INFO (0xC200FF12)
> +
> +// Return values from SMC calls. return values are always in x0
> +#define SMC_OK 0
> +#define SMC_UNK -1
> +
> +// Regions in SOC memory map to which DDR RAM is mapped.
> +typedef struct {
> + UINTN BaseAddress;
> + UINTN Size;
> +} DRAM_REGION_INFO;
> +
> +#endif // MEMORY_INIT_PEI_LIB_H_
> diff --git a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c
> index 3ea773678667..905d326893e5 100644
> --- a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c
> +++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c
> @@ -12,13 +12,15 @@
>
> #include <Library/ArmMmuLib.h>
> #include <Library/ArmPlatformLib.h>
> +#include <Library/ArmSmcLib.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> #include <Library/HobLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PcdLib.h>
>
> -#include <DramInfo.h>
> +#include "MemoryInitPeiLib.h"
> +
>
> VOID
> BuildMemoryTypeInformationHob (
> @@ -44,22 +46,88 @@ InitMmu (
> }
> }
>
> -/*++
> +STATIC
> +UINTN
> +GetDramSize (
> + IN VOID
> + )
> +{
> + ARM_SMC_ARGS ArmSmcArgs;
>
> -Routine Description:
> + ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
> + ArmSmcArgs.Arg1 = -1;
Should this be SMC_UNK?
>
> + ArmCallSmc (&ArmSmcArgs);
>
> + if (ArmSmcArgs.Arg0 == SMC_OK) {
> + return ArmSmcArgs.Arg1;
> + }
>
> -Arguments:
> + // return 0 means no DDR found.
> + return 0;
> +}
>
> - FileHandle - Handle of the file being invoked.
> - PeiServices - Describes the list of possible PEI Services.
> +STATIC
> +EFI_STATUS
> +GetDramRegionsInfo (
> + OUT DRAM_REGION_INFO *DramRegions,
> + IN UINT32 NumRegions
> + )
> +{
> + ARM_SMC_ARGS ArmSmcArgs;
> + UINT32 Index;
> + UINTN RemainingDramSize;
> + UINTN BaseAddress;
> + UINTN Size;
>
> -Returns:
> + RemainingDramSize = GetDramSize ();
> + DEBUG ((DEBUG_INFO, "DRAM Total Size 0x%lx \n", RemainingDramSize));
>
> - Status - EFI_SUCCESS if the boot mode could be set
> + // Ensure Total Dram Size is valid
> + ASSERT (RemainingDramSize != 0);
>
> ---*/
> + for (Index = 0; Index < NumRegions; Index++) {
> + ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
> + ArmSmcArgs.Arg1 = Index;
> +
> + ArmCallSmc (&ArmSmcArgs);
> +
> + if (ArmSmcArgs.Arg0 == SMC_OK) {
> + BaseAddress = ArmSmcArgs.Arg1;
> + Size = ArmSmcArgs.Arg2;
> + ASSERT (BaseAddress && Size);
> +
> + DramRegions[Index].BaseAddress = BaseAddress;
> + DramRegions[Index].Size = Size;
> + RemainingDramSize -= Size;
> +
> + DEBUG ((DEBUG_INFO, "DRAM Region[%d]: start 0x%lx, size 0x%lx\n",
> + Index, BaseAddress, Size));
> +
> + if (RemainingDramSize == 0) {
> + return EFI_SUCCESS;
> + }
> + } else {
> + break;
> + }
> + }
> +
> + DEBUG ((DEBUG_ERROR, "RemainingDramSize = %u !! Ensure that all DDR regions "
> + "have been accounted for\n", RemainingDramSize));
> +
> + return EFI_BUFFER_TOO_SMALL;
> +}
> +
> +/**
> + Get the installed RAM information.
> + Initialize MMU and Memory HOBs (Resource Descriptor HOBs)
> +
> + @param[in] UefiMemoryBase Base address of region used by UEFI in
> + permanent memory
> + @param[in] UefiMemorySize Size of the region used by UEFI in permanent memory
> +
> + @return EFI_SUCCESS Successfuly Initialize MMU and Memory HOBs.
> +**/
> EFI_STATUS
> EFIAPI
> MemoryPeim (
> @@ -67,11 +135,17 @@ MemoryPeim (
> IN UINT64 UefiMemorySize
> )
> {
> - ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
> - EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
> - EFI_PEI_HOB_POINTERS NextHob;
> - BOOLEAN Found;
> - DRAM_INFO DramInfo;
> + ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
> + INT32 Index;
> + UINTN BaseAddress;
> + UINTN Size;
> + UINTN Top;
> + DRAM_REGION_INFO DramRegions[MAX_DRAM_REGIONS];
> + EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
> + UINTN FdBase;
> + UINTN FdTop;
> + BOOLEAN FoundSystemMem;
> + EFI_STATUS Status;
>
> // Get Virtual Memory Map from the Platform Library
> ArmPlatformGetVirtualMemoryMap (&MemoryTable);
> @@ -94,40 +168,95 @@ MemoryPeim (
> EFI_RESOURCE_ATTRIBUTE_TESTED
> );
>
> - if (GetDramBankInfo (&DramInfo)) {
> - DEBUG ((DEBUG_ERROR, "Failed to get DRAM information, exiting...\n"));
> - return EFI_UNSUPPORTED;
> - }
> + FoundSystemMem = FALSE;
> + ZeroMem (DramRegions, sizeof (DramRegions));
>
> - 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;
> - }
> - NextHob.Raw = GET_NEXT_HOB (NextHob);
> + Status = GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
> + ASSERT_EFI_ERROR (Status);
Slightly concerned here because we end up with a variable Status that
is only *used* in DEBUG builds. That could lead to toolchain warnings.
And I was completely serious last time around, I am OK with the return
value being cast away explicitly. What I meant with that is:
(VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
/
Leif
> +
> + FdBase = (UINTN)FixedPcdGet64 (PcdFdBaseAddress);
> + FdTop = FdBase + (UINTN)FixedPcdGet32 (PcdFdSize);
> +
> + // Declare memory regions to system
> + // 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 UEFI DXE region i.e. PcdSystemMemoryUefiRegionSize.
> + // 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.
> + for (Index = MAX_DRAM_REGIONS - 1; Index >= 0; Index--) {
> + if (DramRegions[Index].Size == 0) {
> + 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 == TRUE) {
> + continue;
> + }
> +
> + Size = DramRegions[Index].Size;
> +
> + if (FdBase >= BaseAddress && FdTop <= Top) {
> + Size -= (UINTN)FixedPcdGet32 (PcdFdSize);
> + }
> +
> + if ((UefiMemoryBase >= BaseAddress) && (Size >= UefiMemorySize)) {
> + FoundSystemMem = TRUE;
> }
> }
>
> + ASSERT (FoundSystemMem == TRUE);
> +
> // Build Memory Allocation Hob
> InitMmu (MemoryTable);
>
> 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;
> -}
> --
> 2.17.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58699): https://edk2.groups.io/g/devel/message/58699
Mute This Topic: https://groups.io/mt/73370132/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Leif,
> > + ARM_SMC_ARGS ArmSmcArgs;
> >
> > -Routine Description:
> > + ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
> > + ArmSmcArgs.Arg1 = -1;
>
> Should this be SMC_UNK?
No. SMC_OK / SMC_UNK is returned values.
While x0, x1 are arguments.
I have explained this in the MemoryInitPeiLib.h
// This SMC call works in this way:
// x1 = -1 : return x0: SMC_OK, x1: total DDR Ram size
// x1 >= number of DRAM regions to which DDR RAM is mapped : return x0: SMC_UNK
// 0 <= x1 < number of DRAM regions to which DDR RAM is mapped : return
// x0: SMC_OK, x1: Base address of DRAM region,
// x2: Size of DRAM region
>
> >
> > + ArmCallSmc (&ArmSmcArgs);
> >
> > + if (ArmSmcArgs.Arg0 == SMC_OK) {
> > + return ArmSmcArgs.Arg1;
> > + }
> >
....
> > - {
> > - Found = TRUE;
> > - break;
> > - }
> > - NextHob.Raw = GET_NEXT_HOB (NextHob);
> > + Status = GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
> > + ASSERT_EFI_ERROR (Status);
>
> Slightly concerned here because we end up with a variable Status that
> is only *used* in DEBUG builds. That could lead to toolchain warnings.
I don't think this would cause build warnings in RELEASE builds. I have tested it.
Also the Status is frequently being handled in this way in other places in edk2:
https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/PlatformPei/PlatformPeim.c#L90
>
> And I was completely serious last time around, I am OK with the return
> value being cast away explicitly. What I meant with that is:
> (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
>
I agreed with your suggestion to return EFI_BUFFER_TOO_SMALL from GetDramRegionsInfo,
But If we discard the return value it means we are OK with some RAM not being reported to UEFI firmware
and subsequently to OS. Isn't this a critical error ?
> /
> Leif
>
> > +
> > + FdBase = (UINTN)FixedPcdGet64 (PcdFdBaseAddress);
> > + FdTop = FdBase + (UINTN)FixedPcdGet32 (PcdFdSize);
> > +
> > + // Declare memory regions to system
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58776): https://edk2.groups.io/g/devel/message/58776
Mute This Topic: https://groups.io/mt/73370132/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Thu, May 07, 2020 at 07:28:30 +0000, Pankaj Bansal (OSS) wrote:
> Hi Leif,
>
> > > + ARM_SMC_ARGS ArmSmcArgs;
> > >
> > > -Routine Description:
> > > + ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
> > > + ArmSmcArgs.Arg1 = -1;
> >
> > Should this be SMC_UNK?
>
> No. SMC_OK / SMC_UNK is returned values.
> While x0, x1 are arguments.
> I have explained this in the MemoryInitPeiLib.h
OK, then that -1 needs a separate #define.
> // This SMC call works in this way:
> // x1 = -1 : return x0: SMC_OK, x1: total DDR Ram size
> // x1 >= number of DRAM regions to which DDR RAM is mapped : return x0: SMC_UNK
> // 0 <= x1 < number of DRAM regions to which DDR RAM is mapped : return
> // x0: SMC_OK, x1: Base address of DRAM region,
> // x2: Size of DRAM region
>
> >
> > >
> > > + ArmCallSmc (&ArmSmcArgs);
> > >
> > > + if (ArmSmcArgs.Arg0 == SMC_OK) {
> > > + return ArmSmcArgs.Arg1;
> > > + }
> > >
>
> ....
>
> > > - {
> > > - Found = TRUE;
> > > - break;
> > > - }
> > > - NextHob.Raw = GET_NEXT_HOB (NextHob);
> > > + Status = GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
> > > + ASSERT_EFI_ERROR (Status);
> >
> > Slightly concerned here because we end up with a variable Status that
> > is only *used* in DEBUG builds. That could lead to toolchain warnings.
>
> I don't think this would cause build warnings in RELEASE builds. I have tested it.
> Also the Status is frequently being handled in this way in other places in edk2:
>
> https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/PlatformPei/PlatformPeim.c#L90
The example you point to sets Status, overwrites it (once or twice
depending on conditionals), then returns it.
This patch in its current form sets Status, accesses it only in DEBUG
builds and does not return it.
> > And I was completely serious last time around, I am OK with the return
> > value being cast away explicitly. What I meant with that is:
> > (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
>
> I agreed with your suggestion to return EFI_BUFFER_TOO_SMALL from GetDramRegionsInfo,
> But If we discard the return value it means we are OK with some RAM not being reported to UEFI firmware
> and subsequently to OS. Isn't this a critical error ?
ASSERTs are only triggered in DEBUG builds, and send the processor
into WFI.
If it is a critical error (is it critical if you have found some RAM,
but been unable to fully reconcile all of the RAM in the system?), it
should do more than that.
I am all for properly handling that situation, but this patch has
never done that. Feel free to rework before submitting v5, or leave it
until (if) adding ACPI support and report the condition properly
through BERT.
/
Leif
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58785): https://edk2.groups.io/g/devel/message/58785
Mute This Topic: https://groups.io/mt/73370132/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
>
> On Thu, May 07, 2020 at 07:28:30 +0000, Pankaj Bansal (OSS) wrote:
> > Hi Leif,
> >
> > > > + ARM_SMC_ARGS ArmSmcArgs;
> > > >
> > > > -Routine Description:
> > > > + ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
> > > > + ArmSmcArgs.Arg1 = -1;
> > >
> > > Should this be SMC_UNK?
> >
> > No. SMC_OK / SMC_UNK is returned values.
> > While x0, x1 are arguments.
> > I have explained this in the MemoryInitPeiLib.h
>
> OK, then that -1 needs a separate #define.
OK. That I will take care.
>
> > // This SMC call works in this way:
> > // x1 = -1 : return x0: SMC_OK, x1: total DDR Ram size
> > // x1 >= number of DRAM regions to which DDR RAM is mapped : return x0:
> SMC_UNK
> > // 0 <= x1 < number of DRAM regions to which DDR RAM is mapped : return
> > // x0: SMC_OK, x1: Base address of DRAM region,
> > // x2: Size of DRAM region
> >
> > >
> > > >
> > > > + ArmCallSmc (&ArmSmcArgs);
> > > >
> > > > + if (ArmSmcArgs.Arg0 == SMC_OK) {
> > > > + return ArmSmcArgs.Arg1;
> > > > + }
> > > >
> >
> > ....
> >
> > > > - {
> > > > - Found = TRUE;
> > > > - break;
> > > > - }
> > > > - NextHob.Raw = GET_NEXT_HOB (NextHob);
> > > > + Status = GetDramRegionsInfo (DramRegions, ARRAY_SIZE
> (DramRegions));
> > > > + ASSERT_EFI_ERROR (Status);
> > >
> > > Slightly concerned here because we end up with a variable Status that
> > > is only *used* in DEBUG builds. That could lead to toolchain warnings.
> >
> > I don't think this would cause build warnings in RELEASE builds. I have tested it.
> > Also the Status is frequently being handled in this way in other places in edk2:
> >
> >
> https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/PlatformPei/
> PlatformPeim.c#L90
>
> The example you point to sets Status, overwrites it (once or twice
> depending on conditionals), then returns it.
> This patch in its current form sets Status, accesses it only in DEBUG
> builds and does not return it.
Ok. Then I can return status at the end of this function (MemoryPeim).
I still can't bring myself to just ignore the critical error status from a function.
>
> > > And I was completely serious last time around, I am OK with the return
> > > value being cast away explicitly. What I meant with that is:
> > > (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
> >
> > I agreed with your suggestion to return EFI_BUFFER_TOO_SMALL from
> GetDramRegionsInfo,
> > But If we discard the return value it means we are OK with some RAM not
> being reported to UEFI firmware
> > and subsequently to OS. Isn't this a critical error ?
>
> ASSERTs are only triggered in DEBUG builds, and send the processor
> into WFI.
>
> If it is a critical error (is it critical if you have found some RAM,
> but been unable to fully reconcile all of the RAM in the system?), it
> should do more than that.
>
> I am all for properly handling that situation, but this patch has
> never done that. Feel free to rework before submitting v5, or leave it
> until (if) adding ACPI support and report the condition properly
> through BERT.
I have referred reference platform lib https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c#L83
As per this lib, I have added ASSERTS in all the scenarios, which are critical.
I thought ASSERT means we will halt the program execution regardless of DEBUG or RELEASE build.
Can you point me to a reference platform which handles these errors using ACPI BERT methods ?
I see that in DebugLib.h :
Note that a reserved macro named MDEPKG_NDEBUG is introduced for the intention
of size reduction when compiler optimization is disabled. If MDEPKG_NDEBUG is
defined, then debug and assert related macros wrapped by it are the NULL implementations.
We have NOT defined MDEPKG_NDEBUG in our platforms for RELEASE or DEBUG builds.
Up until we have not implemented the ACPI BERT methods, can we keep it this way to avoid masking the errors?
>
> /
> Leif
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58837): https://edk2.groups.io/g/devel/message/58837
Mute This Topic: https://groups.io/mt/73370132/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Pankaj,
On Fri, May 08, 2020 at 05:31:53 +0000, Pankaj Bansal (OSS) wrote:
> > > > > - {
> > > > > - Found = TRUE;
> > > > > - break;
> > > > > - }
> > > > > - NextHob.Raw = GET_NEXT_HOB (NextHob);
> > > > > + Status = GetDramRegionsInfo (DramRegions, ARRAY_SIZE
> > (DramRegions));
> > > > > + ASSERT_EFI_ERROR (Status);
> > > >
> > > > Slightly concerned here because we end up with a variable Status that
> > > > is only *used* in DEBUG builds. That could lead to toolchain warnings.
> > >
> > > I don't think this would cause build warnings in RELEASE builds. I have tested it.
> > > Also the Status is frequently being handled in this way in other places in edk2:
> > >
> > >
> > https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/PlatformPei/
> > PlatformPeim.c#L90
> >
> > The example you point to sets Status, overwrites it (once or twice
> > depending on conditionals), then returns it.
> > This patch in its current form sets Status, accesses it only in DEBUG
> > builds and does not return it.
>
> Ok. Then I can return status at the end of this function (MemoryPeim).
> I still can't bring myself to just ignore the critical error status from a function.
If it is considered critical, it needs to be handled. If it is not
critical (but would, for example, be useful to log and tell someone
about) it can be explicitly cast away until such a time as the full
error reporting can be iplemented. That should not be confused for
ignoring it.
> > > > And I was completely serious last time around, I am OK with the return
> > > > value being cast away explicitly. What I meant with that is:
> > > > (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
> > >
> > > I agreed with your suggestion to return EFI_BUFFER_TOO_SMALL from
> > GetDramRegionsInfo,
> > > But If we discard the return value it means we are OK with some RAM not
> > being reported to UEFI firmware
> > > and subsequently to OS. Isn't this a critical error ?
> >
> > ASSERTs are only triggered in DEBUG builds, and send the processor
> > into WFI.
> >
> > If it is a critical error (is it critical if you have found some RAM,
> > but been unable to fully reconcile all of the RAM in the system?), it
> > should do more than that.
> >
> > I am all for properly handling that situation, but this patch has
> > never done that. Feel free to rework before submitting v5, or leave it
> > until (if) adding ACPI support and report the condition properly
> > through BERT.
>
> I have referred reference platform lib https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c#L83
> As per this lib, I have added ASSERTS in all the scenarios, which
> are critical.
Adding the ASSERT is good hygiene, I have no objection to that.
> I thought ASSERT means we will halt the program execution regardless
> of DEBUG or RELEASE build.
Dying without attempted recovery is not an appropriate response for a
RELEASE build. Especially in the case where we have successfully
located memory to use, but have not fully managed to reconcile all
entries.
> Can you point me to a reference platform which handles these errors
> using ACPI BERT methods ?
Sadly, no. The Hisilicon 1620 platform implements BERT support as part
of its APEI driver, but it does not appear to actually log anything.
> I see that in DebugLib.h :
>
> Note that a reserved macro named MDEPKG_NDEBUG is introduced for the intention
> of size reduction when compiler optimization is disabled. If MDEPKG_NDEBUG is
> defined, then debug and assert related macros wrapped by it are the NULL implementations.
>
> We have NOT defined MDEPKG_NDEBUG in our platforms for RELEASE or DEBUG builds.
> Up until we have not implemented the ACPI BERT methods, can we keep
> it this way to avoid masking the errors?
It would be unfortunate if the NXP platforms behaved differently from
the majority of other EDK2 platforms. It would also mean that if
someone was to create a derivative platform (which I expect is what
NXP wants), then various drivers in this reference implementation
would start failing to build if *they* added MDEPKG_NDEBUG to *their*
.dsc.
So it would be much preferred if you could add the MDEPKG_NDEBUG
stanzas to RELEASE CC_FLAGS.
But thank you for explaining why I wasn't seeing the "set but not
used" warning :)
Best Regards,
Leif
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#59058): https://edk2.groups.io/g/devel/message/59058
Mute This Topic: https://groups.io/mt/73370132/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.