public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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


  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