Re: [PATCH v2 0/2] Dynamic ACPI framework for fsl layerscape platforms

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

Hi Leif,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Leif Lindholm <leif@nuviainc.com> 
Sent: 15 February 2021 01:18 PM
To: Vikas Singh <vikas.singh@puresoftware.com>; Sami Mujawar <Sami.Mujawar@arm.com>
Cc: devel@edk2.groups.io; Meenakshi Aggarwal (meenakshi.aggarwal@nxp.com) <meenakshi.aggarwal@nxp.com>; Paul Yang <Paul.Yang@arm.com>; Augustine Philips <Augustine.Philips@arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; V Sethi (v.sethi@nxp.com) <v.sethi@nxp.com>; arokia.samy <arokia.samy@puresoftware.com>; kuldip.dwivedi@puresoftware.com; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; vikas.singh@nxp.com
Subject: Re: [PATCH v2 0/2] Dynamic ACPI framework for fsl layerscape platforms

Hi Vikas,

Apologies for very slow review.

However, this set doesn't build for me.
It fails with

In file included from /work/git/edk2-platforms/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c:26:
/work/git/edk2-platforms/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.h:17:10: fatal error: Platform.h: No such file or directory
 #include <Platform.h>
           ^~~~~~~~~~~~

Since the location of ConfigurationManagerPkg suggests the intent to
use this for additional platforms in the future, this cannot be
resolved by referencing LX2160aRdbPkg.dec from
ConfigurationManagerDxe.inf.

I would suggest doing like the ARM VExpressPkg and add:
[BuildOptions]
  GCC:*_*_AARCH64_PLATFORM_FLAGS == -I$(WORKSPACE)/Platform/NXP/LX2160aRdbPkg/Include
to LX2160aRdbPkg.dsc.

Also, I'm seeing a build failure in edk2 DynamicTablesPkg when
building with NOOPT:
/work/git/edk2/DynamicTablesPkg/Library/Common/AmlLib/NameSpace/AmlNameSpace.c: In function ‘AmlEnumeratePathCallback’:
/work/git/edk2/DynamicTablesPkg/Library/Common/AmlLib/NameSpace/AmlNameSpace.c:1081:31: error: variable ‘CurrNodePath’ set but not used [-Werror=unused-but-set-variable]
   CHAR8                     * CurrNodePath;
                               ^~~~~~~~~~~~
/work/git/edk2/DynamicTablesPkg/Library/Common/AmlLib/NameSpace/AmlNameSpace.c:1078:31: error: variable ‘SearchedPath’ set but not used [-Werror=unused-but-set-variable]
   CHAR8                     * SearchedPath;
                               ^~~~~~~~~~~~

Sami - can you have a look at that?
[SAMI] We will get a patch to fix this soon. 
[/SAMI]


I'll follow up with a couple of minor comments on the patches.

Best Regards,

Leif

On Wed, Jan 27, 2021 at 12:14:57 +0530, Vikas Singh wrote:
> This patch series sets the foundation of Dynamic ACPI framework for all
> fsl layerscape platforms. In order to achieve:
>   - Configurable firmware builds.
>   - Unify firmware build for similar platforms.
>   - Minimize/eliminate human induced errors.
>   - Validate and generate firmware that complies with relevant specifications.
> this change set will introduce following changes in below defined order under
> edk2-platforms/NXP for LX2160ARDB platform.
> 
> (1) Introduced edk2-platforms/NXP/ConfigurationManager
> It creates the platform repositories dynamically during build time
> and initializes with platform specific information and serves all requestes
> coming from OEM/standard firmware table generators. Configuration Manager(CM)
> will be common for all fsl platforms.
> 
> (2) Introduced edk2-platforms/NXP/LX2160ARDB/Include/Platform.h
> It has all the declarations and the definitions specified for the platforms.
> These definitions will be inturn consumed by Configuration Manager.
> Additionally the placement of this header under "Include" dir will make these
> macro's availale to other translation units of the platform built.
> 
> (3) Introduced edk2-platforms/NXP/LX2160ARDB/AcpiTablesInclude
> This is a placeholder for - OEM specific firmware acpi table generators.
> This also includes IP specific - DSDT/SSDT generators for the OEM's platform.
> Currently Dsdt.asl is a place holder having only platform's clock related dsdt
> properties for LX2160ARDB but it is intended to extend this "Dsdt.asl" to hold
> other table as well in next patch series.
> 
> (4) Introduced a new "DYNAMIC_ACPI_ENABLE" flag to control DACPI framework.
> Currently this flag is used for LX2160ARDB and by default it is enabled.
> This flag can also extend to other fsl layerscape platforms in future.
> Changes can be referred under:
>   - LX2160ARDb.dsc
>   - LX2160ARDB.fdf
> 
> (5) Introduced a "build.sh" script under edk2-platforms/NXP/ to automate builds.
> And also mandatorily need to support Dynamic ACPI framework for all fsl platforms.
> It exports build environment variables and also invokes "Env.sh".
> 
> Vikas Singh (2):
>   Platform/NXP: Add Dynamic Acpi for layerscape platforms
>   Platform/NXP: Add OEM specific DSDT generator
> 
>  .../ConfigurationManagerDxe/ConfigurationManager.c | 843 +++++++++++++++++++++
>  .../ConfigurationManagerDxe/ConfigurationManager.h | 153 ++++
>  .../ConfigurationManagerDxe.inf                    |  52 ++
>  .../ConfigurationManagerPkg.dec                    |  23 +
>  .../Include/PlatformAcpiTableGenerator.h           |  20 +
>  Platform/NXP/Env.sh                                |  67 ++
>  .../LX2160aRdbPkg/AcpiTablesInclude/Dsdt/Clk.asl   |  40 +
>  .../LX2160aRdbPkg/AcpiTablesInclude/Dsdt/Dsdt.asl  |  15 +
>  .../AcpiTablesInclude/PlatformAcpiDsdtLib.inf      |  39 +
>  .../PlatformAcpiDsdtLib/RawDsdtGenerator.c         | 146 ++++
>  .../AcpiTablesInclude/PlatformAcpiLib.h            |  24 +
>  Platform/NXP/LX2160aRdbPkg/Include/Platform.h      | 246 ++++++
>  Platform/NXP/LX2160aRdbPkg/LX2160aRdbPkg.dec       |   4 +
>  Platform/NXP/LX2160aRdbPkg/LX2160aRdbPkg.dsc       |  29 +
>  Platform/NXP/LX2160aRdbPkg/LX2160aRdbPkg.fdf       |  12 +
>  Platform/NXP/Readme.md                             |  35 +-
>  Platform/NXP/build.sh                              | 115 +++
>  Silicon/NXP/LX2160A/LX2160A.dsc.inc                |   9 +
>  18 files changed, 1871 insertions(+), 1 deletion(-)
>  create mode 100644 Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c
>  create mode 100644 Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.h
>  create mode 100644 Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManagerDxe.inf
>  create mode 100644 Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerPkg.dec
>  create mode 100644 Platform/NXP/ConfigurationManagerPkg/Include/PlatformAcpiTableGenerator.h
>  create mode 100755 Platform/NXP/Env.sh
>  create mode 100644 Platform/NXP/LX2160aRdbPkg/AcpiTablesInclude/Dsdt/Clk.asl
>  create mode 100644 Platform/NXP/LX2160aRdbPkg/AcpiTablesInclude/Dsdt/Dsdt.asl
>  create mode 100644 Platform/NXP/LX2160aRdbPkg/AcpiTablesInclude/PlatformAcpiDsdtLib.inf
>  create mode 100644 Platform/NXP/LX2160aRdbPkg/AcpiTablesInclude/PlatformAcpiDsdtLib/RawDsdtGenerator.c
>  create mode 100644 Platform/NXP/LX2160aRdbPkg/AcpiTablesInclude/PlatformAcpiLib.h
>  create mode 100644 Platform/NXP/LX2160aRdbPkg/Include/Platform.h
>  create mode 100755 Platform/NXP/build.sh
> 
> -- 
> 2.7.4
>