On Mon, Jan 18, 2021 at 10:25 PM Leif Lindholm <l...@nuviainc.com> wrote:
>
> +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.
> ?
Leif + Sami, I was referring section 5.7.3.7, since you commented on
switch case & break statement.
However keeping section 5.5.2.1 in consideration, I have done few
changes and shared updated V1 series.
Could you please have a look on it and revert, if in case you have any concerns.
>
> > 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70532): https://edk2.groups.io/g/devel/message/70532
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]
-=-=-=-=-=-=-=-=-=-=-=-
