Re: [edk2-devel] [PATCH v2 7/7] Platform/ARM/N1Sdp: Enable ACPI tables and configuration manager

["PierreGondois" <pierre.gondois@arm.com>]

Hi Khasim,

On 10/10/21 19:29, Khasim Mohammed via groups.io wrote:
> This patch enables ACPI tables and configuration manager for N1SDP
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Khasim Syed Mohammed <khasim.mohammed@arm.com>
> ---
>  Platform/ARM/N1Sdp/Include/N1SdpAcpiHeader.h | 35 ++++++++++++++++++++
>  Platform/ARM/N1Sdp/N1SdpPlatform.dec         |  1 +
>  Platform/ARM/N1Sdp/N1SdpPlatform.dsc         | 10 ++++++
>  Platform/ARM/N1Sdp/N1SdpPlatform.fdf         |  9 +++++
>  4 files changed, 55 insertions(+)
>  create mode 100644 Platform/ARM/N1Sdp/Include/N1SdpAcpiHeader.h
>
> diff --git a/Platform/ARM/N1Sdp/Include/N1SdpAcpiHeader.h b/Platform/ARM/N1Sdp/Include/N1SdpAcpiHeader.h
> new file mode 100644
> index 0000000000..d81bdb3576
> --- /dev/null
> +++ b/Platform/ARM/N1Sdp/Include/N1SdpAcpiHeader.h
> @@ -0,0 +1,35 @@
> +/** @file
> +
> +  Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef N1SDP_ACPI_HEADER_
> +#define N1SDP_ACPI_HEADER_
> +
> +//
> +// ACPI table information used to initialize tables.
> +//
> +#define EFI_ACPI_ARM_OEM_ID           'A','R','M','L','T','D'   // OEMID 6 bytes long
> +#define EFI_ACPI_ARM_OEM_TABLE_ID     SIGNATURE_64('A','R','M','N','1','S','D','P') // OEM table id 8 bytes long
> +#define EFI_ACPI_ARM_OEM_REVISION     0x20181101
> +#define EFI_ACPI_ARM_CREATOR_ID       SIGNATURE_32('A','R','M',' ')
> +#define EFI_ACPI_ARM_CREATOR_REVISION 0x00000099
> +
> +// A macro to initialise the common header part of EFI ACPI tables as defined by
> +// EFI_ACPI_DESCRIPTION_HEADER structure.
> +#define ARM_ACPI_HEADER(Signature, Type, Revision) {              \
> +    Signature,                      /* UINT32  Signature */       \
> +    sizeof (Type),                  /* UINT32  Length */          \
> +    Revision,                       /* UINT8   Revision */        \
> +    0,                              /* UINT8   Checksum */        \
> +    { EFI_ACPI_ARM_OEM_ID },        /* UINT8   OemId[6] */        \
> +    EFI_ACPI_ARM_OEM_TABLE_ID,      /* UINT64  OemTableId */      \
> +    EFI_ACPI_ARM_OEM_REVISION,      /* UINT32  OemRevision */     \
> +    EFI_ACPI_ARM_CREATOR_ID,        /* UINT32  CreatorId */       \
> +    EFI_ACPI_ARM_CREATOR_REVISION   /* UINT32  CreatorRevision */ \
> +  }
> +
> +#endif /* N1SDP_ACPI_HEADER_ */
> diff --git a/Platform/ARM/N1Sdp/N1SdpPlatform.dec b/Platform/ARM/N1Sdp/N1SdpPlatform.dec
> index d56891b985..36123ddac0 100644
> --- a/Platform/ARM/N1Sdp/N1SdpPlatform.dec
> +++ b/Platform/ARM/N1Sdp/N1SdpPlatform.dec
> @@ -22,6 +22,7 @@
>  #
>  ################################################################################
>  [Includes.common]
> +  Include           # Root include for the pakage
>  
>  [LibraryClasses]
>    ArmPlatformLib|Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.inf
> diff --git a/Platform/ARM/N1Sdp/N1SdpPlatform.dsc b/Platform/ARM/N1Sdp/N1SdpPlatform.dsc
> index d5ada590e1..aba843bc29 100644
> --- a/Platform/ARM/N1Sdp/N1SdpPlatform.dsc
> +++ b/Platform/ARM/N1Sdp/N1SdpPlatform.dsc
> @@ -27,6 +27,9 @@
>  !include Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
>  !include MdePkg/MdeLibs.dsc.inc
>  
> +!include DynamicTablesPkg/DynamicTables.dsc.inc
> +!include Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManager.dsc.inc

Same remark as in "[PATCH v2 6/7] Platform/ARM/N1Sdp: Configuration
Manager for N1Sdp" about the need to have this file:

Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManager.dsc.inc

> +
>  [LibraryClasses.common]
>    ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf
>    ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
> @@ -156,6 +159,9 @@
>    gArmPlatformTokenSpaceGuid.PcdCoreCount|2
>    gArmPlatformTokenSpaceGuid.PcdClusterCount|2
>  
> +  # ACPI Table Version
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x20
> +
>    # Runtime Variable storage
>    gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable|TRUE
> @@ -215,6 +221,10 @@
>        BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>    }
>  
> +  # ACPI Support
> +  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> +  MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
> +
>    # Platform driver
>    Platform/ARM/N1Sdp/Drivers/PlatformDxe/PlatformDxe.inf
>  
> diff --git a/Platform/ARM/N1Sdp/N1SdpPlatform.fdf b/Platform/ARM/N1Sdp/N1SdpPlatform.fdf
> index 6b097438ad..6ede2b7fc8 100644
> --- a/Platform/ARM/N1Sdp/N1SdpPlatform.fdf
> +++ b/Platform/ARM/N1Sdp/N1SdpPlatform.fdf
> @@ -94,6 +94,15 @@ READ_LOCK_STATUS   = TRUE
>    INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
>    INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>  
> +  # ACPI Support
> +  INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> +
> +  # Configuration Manager
> +  INF Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf
> +
> +  # Dynamic Table fdf
> +  !include DynamicTablesPkg/DynamicTables.fdf.inc
> +
>    # Human Interface Support
>    INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
>  

Otherwise the patch looks good to me,

Regards,

Pierre