Re: [edk2-devel] [PATCH v2 6/7] Platform/ARM/N1Sdp: Configuration Manager for N1Sdp

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

Hi Khasim,

2 minor comments,

On 10/10/21 19:29, Khasim Mohammed via groups.io wrote:
> The dynamic tables framework utilizes the configuration manager
> protocol to get the platform specific information required for
> building the firmware tables.
>
> The configuration manager is a platform specific component that
> collates the platform hardware information and builds an abstract
> platform configuration repository. The configuration manager also
> implements the configuration manager protocol which returns the
> hardware information requested by the table generators.
>
> This patch implements the configuration manager for N1SDP
> platform. It enables support for generating the following
> ACPI tables:
>              1. FACP
>              2. DSDT
>              3. GTDT
>              4. APIC
>              5. SPCR
>              6. DBG2
>              7. PPTT
>              8. IORT
>              9. MCFG
>             10. SSDT - PCI
>             11. SSDT - REMOTE PCI
>
> Also added :
> HMAT table and expose CCIX memory as EFI_MEMORY_SP
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Khasim Syed Mohammed <khasim.mohammed@arm.com>
> Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
> Signed-off-by: Manoj Kumar <manoj.kumar3@arm.com>
> Signed-off-by: Patrik Berglund <patrik.berglund@arm.com>
> Signed-off-by: anukou01 <anurag.koul@arm.com>
> Signed-off-by: Sayanta Pattanayak <sayanta.pattanayak@arm.com>
> ---
>  .../ConfigurationManager.dsc.inc              |   16 +
>  .../ConfigurationManager.c                    | 2199 +++++++++++++++++
>  .../ConfigurationManager.h                    |  307 +++
>  .../ConfigurationManagerDxe.inf               |  167 ++
>  .../ConfigurationManagerDxe/Hmat.c            |  103 +
>  .../ConfigurationManagerDxe/Platform.h        |   92 +
>  6 files changed, 2884 insertions(+)
>  create mode 100644 Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManager.dsc.inc
>  create mode 100644 Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
>  create mode 100644 Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.h
>  create mode 100644 Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf
>  create mode 100644 Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/Hmat.c
>  create mode 100644 Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/Platform.h
>
> diff --git a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManager.dsc.inc b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManager.dsc.inc
> new file mode 100644
> index 0000000000..bcd4bf334d
> --- /dev/null
> +++ b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManager.dsc.inc
> @@ -0,0 +1,16 @@
> +## @file
> +#  dsc include file for Configuration Manager
> +#
> +#  Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +
> +[BuildOptions]
> +
> +[Components.common]
> +  # Configuration Manager
> +  Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf

I think a .dsc.inc file was created for Platform/ARM/JunoPkg/ArmJuno.dsc
because the Juno can either build the ACPI tables with the
DynamicTablesPkg or get them statically.

Since the N1Sdp doesn't have static ACPI tables, is it necessary to have
this .dsc.inc file ? We could just add the ConfigurationManagerDxe.inf
component in the N1Sdp.dsc .

> diff --git a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
> new file mode 100644
> index 0000000000..8fc2a89674
> --- /dev/null
> +++ b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
> @@ -0,0 +1,2199 @@
> +/** @file
> +  Configuration Manager Dxe
> +
> +  Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Glossary:
> +    - Cm or CM   - Configuration Manager
> +    - Obj or OBJ - Object
> +**/
> +
> +#include <IndustryStandard/DebugPort2Table.h>
> +#include <IndustryStandard/IoRemappingTable.h>
> +#include <IndustryStandard/MemoryMappedConfigurationSpaceAccessTable.h>
> +#include <IndustryStandard/SerialPortConsoleRedirectionTable.h>
> +#include <Library/ArmLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <NeoverseN1Soc.h>
> +#include <Protocol/AcpiTable.h>
> +#include <Protocol/ConfigurationManagerProtocol.h>
> +
> +#include "ConfigurationManager.h"
> +#include "N1SdpAcpiHeader.h"
> +#include "Platform.h"
> +

[snip]

> +
> +/** A structure describing the configuration manager protocol interface.
> +*/
> +STATIC
> +CONST
> +EDKII_CONFIGURATION_MANAGER_PROTOCOL N1sdpPlatformConfigManagerProtocol = {
> +  CREATE_REVISION(1,0),
> +  N1sdpPlatformGetObject,
> +  N1sdpPlatformSetObject,
> +  &N1sdpRepositoryInfo
> +};
> +
> +/**
> +  Entrypoint of Configuration Manager Dxe.
> +
> +  @param  ImageHandle
> +  @param  SystemTable
> +
> +  @return EFI_SUCCESS
> +  @return EFI_LOAD_ERROR
> +  @return EFI_OUT_OF_RESOURCES
> +**/

Can you add the [in] in the parameter definition ?

[snip]