Re: [edk2-platforms][PATCH V1 02/11] Platform/ARM/Morello: Add Platform Library support for Morello SoC

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Chandni,

Thank you for this patch.

Please find my feedback inline marked [SAMI].

With that updated,

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar


On 04/12/2021 12:30 PM, Chandni Cherukuri wrote:
> It includes virutal memory map for Morello SoC platform.
>
> Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
> ---
>   Platform/ARM/Morello/Library/PlatformLib/PlatformLibSoc.inf  |  44 +++++
>   Platform/ARM/Morello/Library/PlatformLib/PlatformLibMemSoc.c | 176 ++++++++++++++++++++
>   2 files changed, 220 insertions(+)
>
> diff --git a/Platform/ARM/Morello/Library/PlatformLib/PlatformLibSoc.inf b/Platform/ARM/Morello/Library/PlatformLib/PlatformLibSoc.inf
> new file mode 100644
> index 000000000000..bc31b8709152
> --- /dev/null
> +++ b/Platform/ARM/Morello/Library/PlatformLib/PlatformLibSoc.inf
> @@ -0,0 +1,44 @@
> +## @file
> +#  Platform Library for Morello SoC platform.
> +#
> +#  Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001B
> +  BASE_NAME                      = ArmMorelloLib
> +  FILE_GUID                      = 7858ED56-9716-454F-90D7-D117B05063EA
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmPlatformLib
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Platform/ARM/Morello/MorelloPlatform.dec
> +
> +[Sources.common]
> +  PlatformLib.c
> +  PlatformLibMemSoc.c
> +
> +[Sources.AARCH64]
> +  AArch64/Helper.S | GCC
> +
> +[FixedPcd]
> +  gArmMorelloTokenSpaceGuid.PcdDramBlock2Base
> +
> +  gArmTokenSpaceGuid.PcdArmPrimaryCore
> +  gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +[Guids]
> +  gEfiHobListGuid          ## CONSUMES  ## SystemTable
> +
> +[Ppis]
> +  gArmMpCoreInfoPpiGuid
> diff --git a/Platform/ARM/Morello/Library/PlatformLib/PlatformLibMemSoc.c b/Platform/ARM/Morello/Library/PlatformLib/PlatformLibMemSoc.c
> new file mode 100644
> index 000000000000..67dd8469feb8
> --- /dev/null
> +++ b/Platform/ARM/Morello/Library/PlatformLib/PlatformLibMemSoc.c
> @@ -0,0 +1,176 @@
> +/** @file
> +
> +  Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Library/ArmPlatformLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <MorelloPlatform.h>
> +
> +// The total number of descriptors, including the final "end-of-table" descriptor.
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS  9
> +
> +#if !defined (MDEPKG_NDEBUG)
> +  STATIC CONST CHAR8  *gTblAttrDesc[] = {
> +    "UNCACHED_UNBUFFERED          ",
> +    "NONSECURE_UNCACHED_UNBUFFERED",
> +    "WRITE_BACK                   ",
> +    "NONSECURE_WRITE_BACK         ",
> +    "WB_NONSHAREABLE              ",
> +    "NONSECURE_WB_NONSHAREABLE    ",
> +    "WRITE_THROUGH                ",
> +    "NONSECURE_WRITE_THROUGH      ",
> +    "DEVICE                       ",
> +    "NONSECURE_DEVICE             "
> +  };
> +#endif
> +
> +#define LOG_MEM(desc)  DEBUG ((                                             \
> +                        DEBUG_ERROR,                                        \
> +                        desc,                                               \
> +                        VirtualMemoryTable[Index].PhysicalBase,             \
> +                        (VirtualMemoryTable[Index].PhysicalBase +           \
> +                         VirtualMemoryTable[Index].Length - 1),             \
> +                        VirtualMemoryTable[Index].Length,                   \
> +                        gTblAttrDesc[VirtualMemoryTable[Index].Attributes]  \
> +                        ));
> +
> +/**
> +  Returns the Virtual Memory Map of the platform.
> +
> +  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
> +  on your platform.
> +
> +  @param[out] VirtualMemoryMap Array of ARM_MEMORY_REGION_DESCRIPTOR describing
> +                               a Physical-to-Virtual Memory mapping. This array
> +                               must be ended by a zero-filled entry.
> +**/
> +VOID
> +ArmPlatformGetVirtualMemoryMap (
> +  OUT ARM_MEMORY_REGION_DESCRIPTOR **VirtualMemoryMap
> +  )
> +{
> +  UINTN                         Index;
> +  ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
> +  EFI_RESOURCE_ATTRIBUTE_TYPE   ResourceAttributes;
> +  MORELLO_PLAT_INFO             *PlatInfo;
> +  UINT64                        DramBlock2Size;
> +
> +  Index = 0;
> +  DramBlock2Size = 0;
> +
> +  PlatInfo = (MORELLO_PLAT_INFO *)MORELLO_PLAT_INFO_STRUCT_BASE;
> +  if (PlatInfo->LocalDdrSize > MORELLO_DRAM_BLOCK1_SIZE) {
> +    DramBlock2Size = PlatInfo->LocalDdrSize - MORELLO_DRAM_BLOCK1_SIZE;
> +  }
> +
> +  if (DramBlock2Size != 0) {
[SAMI] I think the above check that DramBlock2Size != 0 is not required 
the followng code block
      <START>
> +    ResourceAttributes =
> +      EFI_RESOURCE_ATTRIBUTE_PRESENT |
> +      EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> +      EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> +      EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> +      EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> +      EFI_RESOURCE_ATTRIBUTE_TESTED;
> +
> +    BuildResourceDescriptorHob (
> +      EFI_RESOURCE_SYSTEM_MEMORY,
> +      ResourceAttributes,
> +      FixedPcdGet64 (PcdDramBlock2Base),
> +      DramBlock2Size
> +      );
      <END>
  can be moved in the previous if block after initialising DramBlock2Size.
[/SAMI]
> +  }
> +
> +  ASSERT (VirtualMemoryMap != NULL);
> +
> +  VirtualMemoryTable = AllocatePool (
> +                         sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
> +                         MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS
> +                         );
> +  if (VirtualMemoryTable == NULL) {
> +    return;
> +  }
> +
> +  DEBUG ((
> +    DEBUG_ERROR,
> +    " Memory Map\n----------------------------------------------------------\n"
> +    ));
> +  DEBUG ((
> +    DEBUG_ERROR,
> +    "Description                     :        START       -        END         " \
> +    "[        SIZE        ] {              ATTR             }\n"
> +    ));
> +
> +  // SubSystem Peripherals - Generic Watchdog
> +  VirtualMemoryTable[Index].PhysicalBase = MORELLO_GENERIC_WDOG_BASE;
> +  VirtualMemoryTable[Index].VirtualBase  = MORELLO_GENERIC_WDOG_BASE;
> +  VirtualMemoryTable[Index].Length     = MORELLO_GENERIC_WDOG_SZ;
> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +  LOG_MEM ("Generic Watchdog                : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
> +
> +  // SubSystem Peripherals - GIC-600
> +  VirtualMemoryTable[++Index].PhysicalBase = MORELLO_GIC_BASE;
> +  VirtualMemoryTable[Index].VirtualBase    = MORELLO_GIC_BASE;
> +  VirtualMemoryTable[Index].Length     = MORELLO_GIC_SZ;
> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +  LOG_MEM ("GIC-600                         : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
> +
> +  // SubSystem Peripherals - GICR-600
> +  VirtualMemoryTable[++Index].PhysicalBase = MORELLO_GICR_BASE;
> +  VirtualMemoryTable[Index].VirtualBase    = MORELLO_GICR_BASE;
> +  VirtualMemoryTable[Index].Length     = MORELLO_GICR_SZ;
> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +  LOG_MEM ("GICR-600                        : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
> +
> +  // SubSystem non-secure SRAM
> +  VirtualMemoryTable[++Index].PhysicalBase = MORELLO_NON_SECURE_SRAM_BASE;
> +  VirtualMemoryTable[Index].VirtualBase    = MORELLO_NON_SECURE_SRAM_BASE;
> +  VirtualMemoryTable[Index].Length     = MORELLO_NON_SECURE_SRAM_SZ;
> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> +  LOG_MEM ("non-secure SRAM                 : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
> +
> +  // SubSystem Pheripherals - UART0
> +  VirtualMemoryTable[++Index].PhysicalBase = MORELLO_UART0_BASE;
> +  VirtualMemoryTable[Index].VirtualBase    = MORELLO_UART0_BASE;
> +  VirtualMemoryTable[Index].Length     = MORELLO_UART0_SZ;
> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +  LOG_MEM ("UART0                           : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
> +
> +  // DDR Primary
> +  VirtualMemoryTable[++Index].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
> +  VirtualMemoryTable[Index].VirtualBase    = PcdGet64 (PcdSystemMemoryBase);
> +  VirtualMemoryTable[Index].Length     = PcdGet64 (PcdSystemMemorySize);
> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +  LOG_MEM ("DDR Primary                     : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
> +
> +  // DDR Secondary
> +  if (DramBlock2Size != 0) {
> +    VirtualMemoryTable[++Index].PhysicalBase = PcdGet64 (PcdDramBlock2Base);
> +    VirtualMemoryTable[Index].VirtualBase    = PcdGet64 (PcdDramBlock2Base);
> +    VirtualMemoryTable[Index].Length     = DramBlock2Size;
> +    VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +    LOG_MEM ("DDR Secondary                   : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
> +  }
> +
> +  // Expansion Peripherals
> +  VirtualMemoryTable[++Index].PhysicalBase = MORELLO_EXP_PERIPH_BASE;
> +  VirtualMemoryTable[Index].VirtualBase    = MORELLO_EXP_PERIPH_BASE;
> +  VirtualMemoryTable[Index].Length     = MORELLO_EXP_PERIPH_BASE_SZ;
> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +  LOG_MEM ("Expansion Peripherals           : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
> +
> +  // End of Table
> +  VirtualMemoryTable[++Index].PhysicalBase = 0;
> +  VirtualMemoryTable[Index].VirtualBase    = 0;
> +  VirtualMemoryTable[Index].Length     = 0;
> +  VirtualMemoryTable[Index].Attributes = (ARM_MEMORY_REGION_ATTRIBUTES)0;
> +
> +  ASSERT ((Index) < MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
> +  DEBUG ((DEBUG_INIT, "Virtual Memory Table setup complete.\n"));
> +
> +  *VirtualMemoryMap = VirtualMemoryTable;
> +}