From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.6962.1634632810699051896 for ; Tue, 19 Oct 2021 01:40:11 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 2D997D6E; Tue, 19 Oct 2021 01:40:10 -0700 (PDT) Received: from [192.168.1.16] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 2F7B33F73D; Tue, 19 Oct 2021 01:40:07 -0700 (PDT) Message-ID: <5c9fa02c-b769-bce8-7ff5-9d2fced0d9d5@arm.com> Date: Tue, 19 Oct 2021 09:40:07 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [edk2-devel] [PATCH v2 6/7] Platform/ARM/N1Sdp: Configuration Manager for N1Sdp To: devel@edk2.groups.io, khasim.mohammed@arm.com Cc: nd@arm.com, Sami Mujawar , Chandni Cherukuri , Manoj Kumar , Patrik Berglund , anukou01 , Sayanta Pattanayak References: <20211010182956.13526-1-khasim.mohammed@arm.com> <20211010182956.13526-7-khasim.mohammed@arm.com> From: "PierreGondois" In-Reply-To: <20211010182956.13526-7-khasim.mohammed@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 > Signed-off-by: Khasim Syed Mohammed > Signed-off-by: Chandni Cherukuri > Signed-off-by: Manoj Kumar > Signed-off-by: Patrik Berglund > Signed-off-by: anukou01 > Signed-off-by: Sayanta Pattanayak > --- > .../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.
> +# > +# 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.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > + @par Glossary: > + - Cm or CM - Configuration Manager > + - Obj or OBJ - Object > +**/ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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]