From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-oi1-f180.google.com (mail-oi1-f180.google.com [209.85.167.180]) by mx.groups.io with SMTP id smtpd.web09.8958.1624028079720129382 for ; Fri, 18 Jun 2021 07:54:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@puresoftware.com header.s=google header.b=RPOXZlkd; spf=pass (domain: puresoftware.com, ip: 209.85.167.180, mailfrom: vikas.singh@puresoftware.com) Received: by mail-oi1-f180.google.com with SMTP id x196so10802820oif.10 for ; Fri, 18 Jun 2021 07:54:39 -0700 (PDT) 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=8YWWteXCgx4CYKOKsEYKESKxK6TWeA0Zty3Scx3ewnk=; b=RPOXZlkdy27s0yjoBDrFNJVQN+Ok0UXm0AcdyWwHYLXtIQxJvfZ6FTSdEyVMSE9d3k s9bxsPamWTT57hbLkPmdwGwqPjGXGdnslnjbJaPwNLb5x4yx/xeFjvoWGjEHiwvWwGvQ 7FQZdTPmEZT1oAAOKqzZGDUQYTGKiUCFPkZnU= 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=8YWWteXCgx4CYKOKsEYKESKxK6TWeA0Zty3Scx3ewnk=; b=iQRZo5QfaA+SKIJRIydv0EELrpx0+cs4T2IZJSctOG8uPmaNEfCLvQJz+zdJ9vcclF 1vFvTVkdXem/6gsM/jwUqsoNbNaNxF3z6MwymqnPx0DVgLhLN6915ralXp4e5UUPU3vb OMehlGjOlqmKtlgsEaj4Md8mAjHa/6Jf41KnsfrNBfocWBr1g4GenAZYrTuAwla/pBCp h4CQduBLZh/ZH+tDVHHw1bjBsC/G6ibyNNeR57d7P0qho1KlwIjL6BzOv15r6zM03qVS 4hKcLpa4hUgcP7+D8shiCI66ahc8XKsankO54T0U6c+M5JGdTFur/xQH87Mk9Iopqzti LyYQ== X-Gm-Message-State: AOAM532RL1G2P7CMVmh3zSOUeb7d0eqKvUMnppa6clZLwUyt7fCKfWwt /bUk/QSJIYGzvk/KXOzgfGgL0Y6E0mARi9BXXoBtzA== X-Google-Smtp-Source: ABdhPJzJJ7GkbTc1ib7geK97rePRrKNkzN1GOOfAiwPGoZhUyN7/ub9TM3nZYqelKvi8wbO5jbTSnWPIp0pjhzDhbVY= X-Received: by 2002:aca:4c10:: with SMTP id z16mr14590280oia.123.1624028079007; Fri, 18 Jun 2021 07:54:39 -0700 (PDT) MIME-Version: 1.0 References: <20210611155200.15535-1-vikas.singh@puresoftware.com> <20210611155200.15535-4-vikas.singh@puresoftware.com> <20210614212835.grfvqk6w2ejwlsib@leviathan> In-Reply-To: <20210614212835.grfvqk6w2ejwlsib@leviathan> From: "Vikas Singh" Date: Fri, 18 Jun 2021 20:24:12 +0530 Message-ID: Subject: Re: [PATCH V1 3/4] Platform/NXP/LS1046aFrwyPkg: Extend Dynamic ACPI support To: Leif Lindholm Cc: devel@edk2.groups.io, Sami Mujawar , meenakshi.aggarwal@nxp.com, Samer El-Haj-Mahmoud , Varun Sethi , Arokia Samy , Kuldip Dwivedi , ard.biesheuvel@arm.com, Vikas Singh , Sunny Wang Content-Type: text/plain; charset="UTF-8" On Tue, Jun 15, 2021 at 2:58 AM Leif Lindholm wrote: > > On Fri, Jun 11, 2021 at 21:21:59 +0530, Vikas Singh wrote: > > This patch set extends Configuration Manager (CM) and > > its services to leverage the Dynamic ACPI support for > > NXP's LS1046aFrwy platform. > > This patch does not touch ConfigurationManager. > Please describe what this patch does. > > My guess is it's along the lines of: > This set enables use of the ConfigurationManager framework for the > LS1046aFrwy platform. > > > Refer-https://edk2.groups.io/g/devel/message/71710 > > That is a 1326-line patch set. > What is this reference supposed to tell me? > Leif, Yes this patch will enable the usage of ConfigurationManager(CM) for LS1046aFrwy so that Dynamic ACPI table framework can be reused to generate the tables on this platform as well. And https://edk2.groups.io/g/devel/message/71710 was added just to mark these changes in continuation with the Dynamic ACPI support that we have already added for layerscape platforms. > > Signed-off-by: Vikas Singh > > --- > > Platform/NXP/LS1046aFrwyPkg/Include/Platform.h | 155 ++++++++++++++++++++ > > Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.dsc | 28 ++++ > > Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.fdf | 13 ++ > > Silicon/NXP/LS1046A/LS1046A.dsc.inc | 10 ++ > > 4 files changed, 206 insertions(+) > > > > diff --git a/Platform/NXP/LS1046aFrwyPkg/Include/Platform.h b/Platform/NXP/LS1046aFrwyPkg/Include/Platform.h > > new file mode 100644 > > index 0000000000..19e879ec6d > > --- /dev/null > > +++ b/Platform/NXP/LS1046aFrwyPkg/Include/Platform.h > > @@ -0,0 +1,155 @@ > > +/** @file > > + * Platform headers > > + * > > + * Copyright 2021 NXP > > + * Copyright 2021 Puresoftware Ltd > > + * > > + * SPDX-License-Identifier: BSD-2-Clause-Patent > > + * > > +**/ > > + > > + > > +#ifndef LS1046AFRWY_PLATFORM_H > > +#define LS1046AFRWY_PLATFORM_H > > + > > +#define EFI_ACPI_ARM_OEM_REVISION 0x00000000 > > + > > +// Soc defines > > +#define PLAT_SOC_NAME "LS1046AFRWY" > > +#define SVR_SOC_VER(svr) (((svr) >> 8) & 0xFFFFFE) > > +#define SVR_MAJOR(svr) (((svr) >> 4) & 0xf) > > +#define SVR_MINOR(svr) (((svr) >> 0) & 0xf) > > We already have three identical copies of these three macros: > Platform/NXP/LX2160aRdbPkg/Include/Platform.h > Silicon/NXP/Chassis2/Include/Chassis.h > Silicon/NXP/Chassis3V2/Include/Chassis.h > > Could they be defined once, in a single common header, rather than > adding a fourth one? > Sure, will avoid duplicates in platform headers and reuse the definitions from chassis headers (SoC level) Not sure if we can define this in a common file as these definitions can be different based on the SoC versions. > > + > > +// Gic > > +#define GIC_VERSION 2 > > +#define GICD_BASE 0x1410000 > > +#define GICC_BASE 0x142f000 > > +#define GICH_BASE 0x1440000 > > +#define GICV_BASE 0x1460000 > > + > > +// UART > > +#define UART0_BASE 0x21C0500 > > +#define UART0_IT 86 > > These (GIC and UART) definitions duplicate things already described in > Silicon/NXP/LS1046A/LS1046A.dsc.inc > I understand you concern lief, But ConfigurationManager is kind of a dead entity which will always look for platform header for any platform specific definitions However, In my opinion as you have suggested, we should start encouraging the pcd usage in platform headers instead of static #defines where ever possible. But this is not the intent of this patch series currently, hopefully you can expect PCD changes in my upcoming patch series for sure. > / > Leif > > > +#define UART0_LENGTH 0x100 > > +#define SPCR_FLOW_CONTROL_NONE 0 > > + > > +// Timer > > +#define TIMER_BLOCK_COUNT 1 > > +#define TIMER_FRAME_COUNT 4 > > +#define TIMER_WATCHDOG_COUNT 1 > > +#define TIMER_BASE_ADDRESS 0x23E0000 // a.k.a CNTControlBase > > +#define TIMER_READ_BASE_ADDRESS 0x23F0000 // a.k.a CNTReadBase > > +#define TIMER_SEC_IT 29 > > +#define TIMER_NON_SEC_IT 30 > > +#define TIMER_VIRT_IT 27 > > +#define TIMER_HYP_IT 26 > > +#define TIMER_FRAME0_IT 78 > > +#define TIMER_FRAME1_IT 79 > > +#define TIMER_FRAME2_IT 92 > > + > > +// Mcfg > > +#define LS1046A_PCI_SEG0_CONFIG_BASE 0x4000000000 > > +#define LS1046A_PCI_SEG0 0x0 > > +#define LS1046A_PCI_SEG_BUSNUM_MIN 0x0 > > +#define LS1046A_PCI_SEG_BUSNUM_MAX 0xff > > +#define LS1046A_PCI_SEG1_CONFIG_BASE 0x4800000000 > > +#define LS1046A_PCI_SEG2_CONFIG_BASE 0x5000000000 > > +#define LS1046A_PCI_SEG1 0x1 > > +#define LS1046A_PCI_SEG2 0x2 > > + > > +// Platform specific info needed by Configuration Manager > > + > > +#define CFG_MGR_TABLE_ID SIGNATURE_64 ('L','S','1','0','4','6',' ',' ') > > + > > +// Specify the OEM defined tables > > +#define OEM_ACPI_TABLES 0 > > + > > +#define PLAT_PCI_SEG0 LS1046A_PCI_SEG0 > > +#define PLAT_PCI_SEG1_CONFIG_BASE LS1046A_PCI_SEG1_CONFIG_BASE > > +#define PLAT_PCI_SEG1 LS1046A_PCI_SEG1 > > +#define PLAT_PCI_SEG_BUSNUM_MIN LS1046A_PCI_SEG_BUSNUM_MIN > > +#define PLAT_PCI_SEG_BUSNUM_MAX LS1046A_PCI_SEG_BUSNUM_MAX > > +#define PLAT_PCI_SEG2_CONFIG_BASE LS1046A_PCI_SEG2_CONFIG_BASE > > +#define PLAT_PCI_SEG2 LS1046A_PCI_SEG2 > > + > > +#define PLAT_GIC_VERSION GIC_VERSION > > +#define PLAT_GICD_BASE GICD_BASE > > +#define PLAT_GICI_BASE GICI_BASE > > +#define PLAT_GICR_BASE GICR_BASE > > +#define PLAT_GICR_LEN GICR_LEN > > +#define PLAT_GICC_BASE GICC_BASE > > +#define PLAT_GICH_BASE GICH_BASE > > +#define PLAT_GICV_BASE GICV_BASE > > + > > +#define PLAT_CPU_COUNT 4 > > +#define PLAT_GTBLOCK_COUNT 0 > > +#define PLAT_GTFRAME_COUNT 0 > > +#define PLAT_PCI_CONFG_COUNT 2 > > + > > +#define PLAT_WATCHDOG_COUNT 0 > > +#define PLAT_GIC_REDISTRIBUTOR_COUNT 0 > > +#define PLAT_GIC_ITS_COUNT 0 > > + > > +/* GIC CPU Interface information > > + GIC_ENTRY (CPUInterfaceNumber, Mpidr, PmuIrq, VGicIrq, EnergyEfficiency) > > + */ > > +#define PLAT_GIC_CPU_INTERFACE { \ > > + GICC_ENTRY (0, GET_MPID (0, 0), 138, 0x19, 0), \ > > + GICC_ENTRY (1, GET_MPID (0, 1), 139, 0x19, 0), \ > > + GICC_ENTRY (2, GET_MPID (0, 2), 127, 0x19, 0), \ > > + GICC_ENTRY (3, GET_MPID (0, 3), 129, 0x19, 0), \ > > +} > > + > > +#define PLAT_WATCHDOG_INFO \ > > + { \ > > + } \ > > + > > +#define PLAT_TIMER_BLOCK_INFO \ > > + { \ > > + } \ > > + > > +#define PLAT_TIMER_FRAME_INFO \ > > + { \ > > + } \ > > + > > +#define PLAT_GIC_DISTRIBUTOR_INFO \ > > + { \ > > + PLAT_GICD_BASE, /* UINT64 PhysicalBaseAddress */ \ > > + 0, /* UINT32 SystemVectorBase */ \ > > + PLAT_GIC_VERSION /* UINT8 GicVersion */ \ > > + } \ > > + > > +#define PLAT_GIC_REDISTRIBUTOR_INFO \ > > + { \ > > + } \ > > + > > +#define PLAT_GIC_ITS_INFO \ > > + { \ > > + } \ > > + > > +#define PLAT_MCFG_INFO \ > > + { \ > > + { \ > > + PLAT_PCI_SEG1_CONFIG_BASE, \ > > + PLAT_PCI_SEG1, \ > > + PLAT_PCI_SEG_BUSNUM_MIN, \ > > + PLAT_PCI_SEG_BUSNUM_MAX, \ > > + }, \ > > + { \ > > + PLAT_PCI_SEG2_CONFIG_BASE, \ > > + PLAT_PCI_SEG2, \ > > + PLAT_PCI_SEG_BUSNUM_MIN, \ > > + PLAT_PCI_SEG_BUSNUM_MAX, \ > > + } \ > > + } \ > > + > > +#define PLAT_SPCR_INFO \ > > + { \ > > + UART0_BASE, \ > > + UART0_IT, \ > > + 115200, \ > > + 0, \ > > + EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_INTERFACE_TYPE_16550 \ > > + } \ > > + > > +#endif // LS1046AFRWY_PLATFORM_H > > diff --git a/Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.dsc b/Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.dsc > > index 67cf15cbe4..20111e6037 100755 > > --- a/Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.dsc > > +++ b/Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.dsc > > @@ -3,6 +3,7 @@ > > # LS1046AFRWY Board package. > > # > > # Copyright 2019-2020 NXP > > +# Copyright 2021 Puresoftware Ltd > > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > # > > @@ -22,10 +23,18 @@ > > OUTPUT_DIRECTORY = Build/LS1046aFrwyPkg > > FLASH_DEFINITION = Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.fdf > > > > + # This flag controls the dynamic acpi generation > > + # > > + DEFINE DYNAMIC_ACPI_ENABLE = TRUE > > + > > !include Silicon/NXP/NxpQoriqLs.dsc.inc > > !include MdePkg/MdeLibs.dsc.inc > > !include Silicon/NXP/LS1046A/LS1046A.dsc.inc > > > > +!if $(DYNAMIC_ACPI_ENABLE) == TRUE > > + !include DynamicTablesPkg/DynamicTables.dsc.inc > > +!endif > > + > > [LibraryClasses.common] > > ArmPlatformLib|Platform/NXP/LS1046aFrwyPkg/Library/ArmPlatformLib/ArmPlatformLib.inf > > RealTimeClockLib|EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf > > @@ -46,4 +55,23 @@ > > > > Silicon/NXP/Drivers/UsbHcdInitDxe/UsbHcd.inf > > > > + # > > + # Dynamic Table Factory > > + !if $(DYNAMIC_ACPI_ENABLE) == TRUE > > + DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf { > > + > > + NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiFadtLibArm/AcpiFadtLibArm.inf > > + NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/AcpiGtdtLibArm.inf > > + NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLibArm.inf > > + NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLibArm.inf > > + NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.inf > > + } > > + !endif > > + > > + # > > + # Acpi Support > > + # > > + MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatformDxe.inf > > + MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf > > + > > ## > > diff --git a/Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.fdf b/Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.fdf > > index 34c4e5a025..f3cac033bc 100755 > > --- a/Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.fdf > > +++ b/Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.fdf > > @@ -3,6 +3,7 @@ > > # FLASH layout file for LS1046a board. > > # > > # Copyright 2019-2020 NXP > > +# Copyright 2021 Puresoftware Ltd > > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > # > > @@ -99,6 +100,18 @@ READ_LOCK_STATUS = TRUE > > INF MdeModulePkg/Universal/Metronome/Metronome.inf > > INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf > > > > + > > + # > > + # Acpi Support > > + # > > + INF MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatformDxe.inf > > + INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf > > + > > + !if $(DYNAMIC_ACPI_ENABLE) == TRUE > > + INF Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManagerDxe.inf > > + !include DynamicTablesPkg/DynamicTables.fdf.inc > > + !endif > > + > > # > > # Multiple Console IO support > > # > > diff --git a/Silicon/NXP/LS1046A/LS1046A.dsc.inc b/Silicon/NXP/LS1046A/LS1046A.dsc.inc > > index 7004533ed5..98f999edfd 100644 > > --- a/Silicon/NXP/LS1046A/LS1046A.dsc.inc > > +++ b/Silicon/NXP/LS1046A/LS1046A.dsc.inc > > @@ -2,6 +2,7 @@ > > # LS1046A Soc package. > > # > > # Copyright 2017-2020 NXP > > +# Copyright 2021 Puresoftware Ltd > > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > # > > @@ -48,4 +49,13 @@ > > [Components.common] > > MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf > > > > +# > > +# Configuration Manager > > +!if $(DYNAMIC_ACPI_ENABLE) == TRUE > > + Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManagerDxe.inf { > > + > > + *_*_*_PLATFORM_FLAGS = -I$(WORKSPACE)/Platform/NXP/LS1046aFrwyPkg/Include > > + } > > +!endif > > + > > ## > > -- > > 2.25.1 > >