Hi Leif,
Please find my response inline marked [SAMI].
Regards,
Sami Mujawar
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif Lindholm
via groups.io
Sent: 18 January 2021 04:56 PM
To: Vikas Singh <vikas.si...@puresoftware.com>
Cc: devel@edk2.groups.io; Sami Mujawar <sami.muja...@arm.com>; Meenakshi
Aggarwal (meenakshi.aggar...@nxp.com) <meenakshi.aggar...@nxp.com>; Paul Yang
<paul.y...@arm.com>; Augustine Philips <augustine.phil...@arm.com>; Samer
El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>; V Sethi (v.se...@nxp.com)
<v.se...@nxp.com>; arokia.samy <arokia.s...@puresoftware.com>; Kuldip Dwivedi
<kuldip.dwiv...@puresoftware.com>; Ard Biesheuvel <ard.biesheu...@arm.com>;
Vikas Singh <vikas.si...@nxp.com>
Subject: Re: [edk2-devel] [PATCH v0] Platform/NXP: Add Dynamic Acpi for
layerscape platforms
+Sami,
On Sat, Jan 16, 2021 at 10:15:41 +0530, Vikas Singh wrote:
> On Sun, Jan 10, 2021 at 8:56 AM Leif Lindholm <l...@nuviainc.com> wrote:
> >
> > On Tue, Dec 29, 2020 at 12:55:58 +0530, Vikas Singh wrote:
> > > These changes intend to add
> >
> > Hopefully they actually do.
> >
> > > - Common Configuration Manager (CM) for all fsl platforms.
> > > - Platform headers consumed by CM for LX2160ARDB.
> > > - Clk dsdt properties for LX2160ARDB.
> >
> > This sounds like (at least) three patches.
> >
> Leif, I plan to disintegrate the complete changes into 2 patch set's
> and will be sharing this series early next week.
> set1: Add Common Configuration Manager (CM) for all fsl platforms and
> headers consumed by CM for LX2160ARDB.
> set2: Add platform specific DSDT generator and Clk dsdt properties for
> LX2160ARDB.
This sounds sensible.
> > >
> > > Signed-off-by: Vikas Singh <vikas.si...@puresoftware.com>
> > > ---
> > > .../ConfigurationManager/ConfigurationManager.dec | 24 +
> > > .../ConfigurationManager.dsc.inc | 21 +
> > > .../ConfigurationManagerDxe/ConfigurationManager.c | 709
> > > +++++++++++++++++++++
> > > .../ConfigurationManagerDxe/ConfigurationManager.h | 229 +++++++
> > > .../ConfigurationManagerDxe.inf | 52 ++
> > > .../Include/PlatformAcpiTableGenerator.h | 20 +
> > > Platform/NXP/Env.cshrc | 73 +++
> > > .../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 | 244 +++++++
> > > Platform/NXP/LX2160aRdbPkg/LX2160aRdbPkg.dec | 6 +-
> > > Platform/NXP/LX2160aRdbPkg/LX2160aRdbPkg.dsc | 30 +
> > > Platform/NXP/LX2160aRdbPkg/LX2160aRdbPkg.fdf | 12 +
> > > Platform/NXP/build.sh | 121 ++++
> > > 17 files changed, 1804 insertions(+), 1 deletion(-)
> > > create mode 100644
> > > Platform/NXP/ConfigurationManager/ConfigurationManager.dec
> > > create mode 100644
> > > Platform/NXP/ConfigurationManager/ConfigurationManager.dsc.inc
> > > create mode 100644
> > > Platform/NXP/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
> > > create mode 100644
> > > Platform/NXP/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.h
> > > create mode 100644
> > > Platform/NXP/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf
> > > create mode 100644
> > > Platform/NXP/ConfigurationManager/Include/PlatformAcpiTableGenerator.h
> > > create mode 100755 Platform/NXP/Env.cshrc
> > > 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
> > >
> > > diff --git
> > > a/Platform/NXP/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.h
> > >
> > > b/Platform/NXP/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.h
> > > new file mode 100644
> > > index 0000000..cf09ef9
> > > --- /dev/null
> > > +++
> > > b/Platform/NXP/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.h
> > > @@ -0,0 +1,229 @@
> > > +/** @file
> > > + Configuration Manager headers
> > > +
> > > + Copyright 2020 NXP
> > > + Copyright 2020 Puresoftware Ltd
> > > +
> > > + SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +
> > > + @par Glossary:
> > > + - Cm or CM - Configuration Manager
> > > + - Obj or OBJ - Object
> > > +**/
> > > +
> > > +#ifndef CONFIGURATION_MANAGER_H__
> > > +#define CONFIGURATION_MANAGER_H__
> > > +
> > > +#include <Platform.h>
> > > +#include <PlatformAcpiTableGenerator.h>
> > > +
> > > +/** The configuration manager version
> > > +*/
> > > +#define CONFIGURATION_MANAGER_REVISION CREATE_REVISION (0, 0)
> > > +
> > > +/** The OEM ID
> > > +*/
> > > +#define CFG_MGR_OEM_ID { 'N', 'X', 'P', ' ', ' ', ' ' }
> > > +
> > > +/** A helper macro for populating the GIC CPU information
> > > + */
> > > +#define GICC_ENTRY( \
> > > + CPUInterfaceNumber, \
> > > + Mpidr, \
> > > + PmuIrq, \
> > > + VGicIrq, \
> > > + EnergyEfficiency \
> > > + ) { \
> > > + CPUInterfaceNumber, /* UINT32 CPUInterfaceNumber */ \
> > > + CPUInterfaceNumber, /* UINT32 AcpiProcessorUid */ \
> > > + EFI_ACPI_6_2_GIC_ENABLED, /* UINT32 Flags */ \
> > > + 0, /* UINT32 ParkingProtocolVersion */ \
> > > + PmuIrq, /* UINT32 PerformanceInterruptGsiv */ \
> > > + 0, /* UINT64 ParkedAddress */ \
> > > + GICC_BASE, /* UINT64 PhysicalBaseAddress */ \
> > > + GICV_BASE, /* UINT64 GICV */ \
> > > + GICH_BASE, /* UINT64 GICH */ \
> > > + VGicIrq, /* UINT32 VGICMaintenanceInterrupt */ \
> > > + 0, /* UINT64 GICRBaseAddress */ \
> > > + Mpidr, /* UINT64 MPIDR */ \
> > > + EnergyEfficiency /* UINT8 ProcessorPowerEfficiency */ \
> > > +}
> > > +
> > > +/** A helper macro for returning configuration manager objects
> > > +*/
> > > +#define HANDLE_CM_OBJECT(ObjId, CmObjectId, Object, ObjectCount) \
> > > + case ObjId: { \
> > > + CmObject->ObjectId = CmObjectId; \
> > > + CmObject->Size = sizeof (Object); \
> > > + CmObject->Data = (VOID*)&Object; \
> > > + CmObject->Count = ObjectCount; \
> > > + DEBUG (( \
> > > + DEBUG_INFO, \
> > > + #CmObjectId ": Ptr = 0x%p, Size = %d, Count = %d\n", \
> > > + CmObject->Data, \
> > > + CmObject->Size, \
> > > + CmObject->Count \
> > > + )); \
> > > + break; \
> > > + }
> >
> > This is code obfuscation. Please don't invent your own programming
> > languages. In C, the case, the start bracket, the break and the end
> > bracket always go inline.
> > The rest would be better as a static helper function than a macro.
> >
> Leif, changes are in accordance with :
> https://raw.githubusercontent.com/tianocore-docs/Docs/master/Specifications/CCS_2_1_Draft.pdf
Do you mean 5.5.2.1:
Functional macros are generally discouraged.
?
> and in reference to :
> https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.h#L94
Clearly I was asleep at the wheel when reviewing that set.
So let me state unambiguously:
Hiding *gotos* away in a header macro turns the language into
something other than C.
Sami: please rewrite the referenced code in a way that does not
obfuscate the program flow.
[SAMI] I will submit a patch that removes the macros and uses static functions.
[/SAMI]
Vikas: please submit a v2 that does not obfuscate the program flow.
> > > +/** The number of ACPI tables to install
> > > +*/
> > > +#define PLAT_ACPI_TABLE_COUNT 6
> >
> > This feels suboptimal.
> > Could this be calculated at run- or compile time?
> >
> Leif, I plan to chnage like this : PLAT_ACPI_TABLE_COUNT =
> CM_MANDATORY_TBLS + OEM_ACPI_TBLS
> Here CM_MANDATORY_TBLS must be known to CM upfront and is fixed as per
> thr CM's implementation (min tables needed to boot any os).
> OEM_ACPI_TBLS should be coming form platforms headers only.
> Unfortunately this can not be done at runtime /compile time.
OK. This is still an improvement over direct hard-coding.
> > > diff --git
> > > a/Platform/NXP/ConfigurationManager/Include/PlatformAcpiTableGenerator.h
> > > b/Platform/NXP/ConfigurationManager/Include/PlatformAcpiTableGenerator.h
> > > new file mode 100644
> > > index 0000000..da3c571
> > > --- /dev/null
> > > +++
> > > b/Platform/NXP/ConfigurationManager/Include/PlatformAcpiTableGenerator.h
> > > @@ -0,0 +1,20 @@
> > > +/** @file
> > > + Acpi Table generator headers
> > > +
> > > + Copyright 2020 NXP
> > > + Copyright 2020 Puresoftware Ltd
> > > +
> > > + SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +
> > > +**/
> > > +
> > > +#ifndef PLATFORM_ACPI_TABLE_GENERATOR_H_
> > > +#define PLATFORM_ACPI_TABLE_GENERATOR_H_
> > > +
> > > +typedef enum PlatAcpiTableId {
> > > + EPlatAcpiTableIdReserved = 0x0000, ///< Reserved
> > > + EPlatAcpiTableIdDsdt,
> > > + EPlatAcpiTableIdMax
> > > +} EPLAT_ACPI_TABLE_ID;
> >
> > Where does the EPlat prefix come from? What does it stand for?
> >
> Leif, this will be corrected.
> e.g: EPlatAcpiTableIdReserved -> PlatAcpiTableIdReserved
> These Id's will be used by OEM/platforms defined generators etc.
Makes sense.
> > > +
> > > +#endif
Best Regards,
Leif
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70545): https://edk2.groups.io/g/devel/message/70545
Mute This Topic: https://groups.io/mt/79415929/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
