From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ot1-f49.google.com (mail-ot1-f49.google.com [209.85.210.49]) by mx.groups.io with SMTP id smtpd.web09.7337.1611031330039416306 for ; Mon, 18 Jan 2021 20:42:10 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@puresoftware.com header.s=google header.b=RTywZ/cf; spf=pass (domain: puresoftware.com, ip: 209.85.210.49, mailfrom: vikas.singh@puresoftware.com) Received: by mail-ot1-f49.google.com with SMTP id 34so7778207otd.5 for ; Mon, 18 Jan 2021 20:42:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=puresoftware.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=mcy2OT4YOAe52y4IH8/9oSK7oEE7PJD8HItNxLx0xKk=; b=RTywZ/cf6RWLCdeKm4MXnVzF4vFK09S8Fuusj7v/h2Vq9HrVnTxAaPHNA9g1bYHaw/ Mh9mBZtP/aN8QGNpE0C2fsMa3hDXa1gMuYONoWaQtvjfN/mNpJIONLfEl43vOcHBSEKz YaX44f1Pi1rIQf/3hpidJoWuegh1T2yENWblY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mcy2OT4YOAe52y4IH8/9oSK7oEE7PJD8HItNxLx0xKk=; b=hHh3czX+KhdmhvlXkY/4rEG8My7oSehiAo2Yv9oyeX231Jen0VKeIm0aNfn0n2txFF Wcm6cV3/IPsv/03Fu183ROv2tgfQmpr4zsRj+kXBgu5O8IxLNZDxlRot7EJjTrNdfrVG v1Wi+j6E8wUZ3OSjhhlk7y1P0hCEc9YkOceVRm16a5vrYV0V/cU1djyrpeP8lbdntVST 4EOjwkxgDKPuLev1x9uiu8ck6zlYgvsnWej56Ql7Yi2fsSJhglzcm2vLrott7PTdDeht Vf4Pvjs4ymYKzF4rSp2OVLEVJ9aMZ+1O1lXi4v6HBG6elCpdBO+zeNxbACZAfMMQFKHp CG8A== X-Gm-Message-State: AOAM530JIZtqpmdTpLFw0awGVOl7BmWy81u0U2nDzaBXdWZ3iEo+nAHs dusysreMIX950YVYXOaTxXYprLsjtJSlnWZdIaqzfg== X-Google-Smtp-Source: ABdhPJxudGev66KgYiiJw2JbE0TckEL9/fogHPFpH8VFBtxQapGLd54cHUWwmJ1MeBqzIiyXIj9LkFryhOUcP8AvxgU= X-Received: by 2002:a9d:741a:: with SMTP id n26mr2170947otk.210.1611031329351; Mon, 18 Jan 2021 20:42:09 -0800 (PST) MIME-Version: 1.0 References: <1609226758-19867-1-git-send-email-vikas.singh@puresoftware.com> <1609226758-19867-2-git-send-email-vikas.singh@puresoftware.com> <20210110032612.GK1664@vanye> <20210118165530.GX1664@vanye> In-Reply-To: <20210118165530.GX1664@vanye> From: "Vikas Singh" Date: Tue, 19 Jan 2021 10:11:43 +0530 Message-ID: Subject: Re: [PATCH v0] Platform/NXP: Add Dynamic Acpi for layerscape platforms To: Leif Lindholm Cc: devel@edk2.groups.io, Sami Mujawar , meenakshi.aggarwal@nxp.com, Paul Yang , Augustine Philips , samer.el-haj-mahmoud@arm.com, Varun Sethi , Arokia Samy , Kuldip Dwivedi , ard.biesheuvel@arm.com, Vikas Singh Content-Type: text/plain; charset="UTF-8" On Mon, Jan 18, 2021 at 10:25 PM Leif Lindholm 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 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 > > > > --- > > > > .../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 > > > > +#include > > > > + > > > > +/** 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 >