From: "Leif Lindholm" <leif@nuviainc.com>
To: Vikas Singh <vikas.singh@puresoftware.com>
Cc: devel@edk2.groups.io, Sami Mujawar <sami.mujawar@arm.com>,
meenakshi.aggarwal@nxp.com, Paul Yang <paul.yang@arm.com>,
Augustine Philips <augustine.philips@arm.com>,
samer.el-haj-mahmoud@arm.com, Varun Sethi <v.sethi@nxp.com>,
Arokia Samy <arokia.samy@puresoftware.com>,
Kuldip Dwivedi <kuldip.dwivedi@puresoftware.com>,
ard.biesheuvel@arm.com, Vikas Singh <vikas.singh@nxp.com>
Subject: Re: [PATCH v0] Platform/NXP: Add Dynamic Acpi for layerscape platforms
Date: Mon, 18 Jan 2021 16:55:30 +0000 [thread overview]
Message-ID: <20210118165530.GX1664@vanye> (raw)
In-Reply-To: <CADvVLtXDFS2wGP0=6dNb_W0OQxWNy3yb2HRw=nT8huex0gVv+w@mail.gmail.com>
+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 <leif@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.singh@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.
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
next prev parent reply other threads:[~2021-01-18 16:55 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-29 7:25 [PATCH v0] Dynamic ACPI framework for fsl layerscape platforms Vikas Singh
2020-12-29 7:25 ` [PATCH v0] Platform/NXP: Add Dynamic Acpi for " Vikas Singh
2021-01-10 3:26 ` Leif Lindholm
2021-01-16 4:45 ` Vikas Singh
2021-01-18 16:55 ` Leif Lindholm [this message]
2021-01-19 4:41 ` Vikas Singh
2021-01-26 11:19 ` Leif Lindholm
2021-01-27 6:51 ` Vikas Singh
2021-02-10 7:12 ` Vikas Singh
2021-01-19 14:37 ` [edk2-devel] " Sami Mujawar
2021-01-10 2:30 ` [PATCH v0] Dynamic ACPI framework for fsl " Leif Lindholm
2021-01-16 4:08 ` Vikas Singh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210118165530.GX1664@vanye \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox