From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f46.google.com (mail-wr1-f46.google.com [209.85.221.46]) by mx.groups.io with SMTP id smtpd.web11.36394.1610988934550108434 for ; Mon, 18 Jan 2021 08:55:35 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=0AOEyAxl; spf=pass (domain: nuviainc.com, ip: 209.85.221.46, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f46.google.com with SMTP id d13so17091775wrc.13 for ; Mon, 18 Jan 2021 08:55:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=W1XU5XDfGAuDlyU8qw9PDaeBYFaECArGH+08STGODpM=; b=0AOEyAxlUZ2AHa4+zs/VoS2KPFOaLnShf2Lvv7856XvbN6+hDNBMp/iS3jiQnu8I7B Lrpc9qpMv7eA/X+i8J1GNw4S8+wjSA+pLvPuVbPJzTq4gjGbVQIhhU5f37TNRtQvxi1e ioqTWxvLiMrb/DvLp3nOUXLpCUeMVU6guoDSaH4sbaGPWsM6nuThvAxgbc8oTE9JPpqt jisFir/0PRC7JGGwKECImTTtaeq/sKxFhCI8XQnkXY/ZXNxwCNNjvKYoQY5Kj30hD32R bVk55Rri2iAnD/1uD08Qi92jCRGzLmKOcHQpHlPshOvaYAe1WpYGOEUXKaTAR4+hsNOz Y14g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=W1XU5XDfGAuDlyU8qw9PDaeBYFaECArGH+08STGODpM=; b=Carmtub6+w6pmkdo2JHsHuRR4LZg/G1XIWzRBoSWzk7KarTRAQKngzEMGOXUdIKein S5Xs7NA+poZIQALI6Tpu2BzXNDapK93XOGgAmMR0uGZHeX3hstJdS/1+zprS12ZGYD4U /KLuGKxad1whQcdhw3yaSZAEgpj06c3mSipuKUxpaAR1amjk1bvAO0BbTBww+22LFeJD gbVzIJHE5CKhoYSCMSQoLVs3DnfhtVyNeX0vo9zr/0UKyecONZRCNDAdjMXinkQA/Opd F6tW1lFRj/vXXATMyIn4Yj5wrdoynaZLJ8tspDTifBiNVtceK+Lws13cXSzWVEjnnm29 kvRw== X-Gm-Message-State: AOAM5334ROye6YwRJIMGWdGNwtdLK0Bw/OSd1QswFrLp/lvUVybj5fEG 8ZLdFYGcPYfajACNJ+q10r8J/Q== X-Google-Smtp-Source: ABdhPJxd89yfRao0vlrRVgfnL27BnQ9bq0kLYwgUEJYShHC2bBMdHw4kynDP2WsoZzdgCLVVNtYptg== X-Received: by 2002:adf:b257:: with SMTP id y23mr349281wra.371.1610988933034; Mon, 18 Jan 2021 08:55:33 -0800 (PST) Return-Path: Received: from vanye (cpc1-cmbg19-2-0-cust915.5-4.cable.virginm.net. [82.27.183.148]) by smtp.gmail.com with ESMTPSA id b7sm30078896wrv.47.2021.01.18.08.55.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Jan 2021 08:55:32 -0800 (PST) Date: Mon, 18 Jan 2021 16:55:30 +0000 From: "Leif Lindholm" To: Vikas Singh 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 Subject: Re: [PATCH v0] Platform/NXP: Add Dynamic Acpi for layerscape platforms Message-ID: <20210118165530.GX1664@vanye> References: <1609226758-19867-1-git-send-email-vikas.singh@puresoftware.com> <1609226758-19867-2-git-send-email-vikas.singh@puresoftware.com> <20210110032612.GK1664@vanye> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline +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. ? > 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