Re: [edk2-devel] [staging/dynamictables-reorg PATCH 00/15] Prepare libraries to support other archs

["Sunil V L" <sunilvl@ventanamicro.com>]

Hi Pierre, Sami,

Thanks a lot again for this work!.

The series looks good to me as well. I agree with Sami's suggestions.

However, I have a request for an additional change. The common ACPI
tables still use ARMH/ARMLTD as the CREATOR_ID/OEM_ID. Can they be made
architecture specific?

Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>

Thanks!
Sunil
On Wed, Jul 03, 2024 at 09:08:08AM +0000, Sami Mujawar wrote:
> Hi Pierre,
> 
> Overall, this patch series looks good to me.
> 
> I have some minor comments regarding the return value for the Arch hook functions their placement in Common folder.
> e.g. in Patch "DynamicTablesPkg: AcpiFadtLib: Prepare to support other archs"
> The file DynamicTablesPkg/Library/Acpi/Common/AcpiFadtLib/Common/CommonFadtGenerator.c
> provides an empty stub for the arch specific implementation for the FadtArchUpdate () and returns
> success. I think this function should return EFI_UNSUPPORTED to indicate that this function is not
> implemented and that the architecture needs to provide an implementation.
> 
> Also, the file name AcpiFadtLib/Common/CommonFadtGenerator.c should be changed to
> AcpiFadtLib/FadtGeneratorNull.c to clarify that the implementation does not exist.
> 
> Similar changes are required for other patches as well.
> 
> Apart from the above, in patch " DynamicTablesPkg: FdtHwInfoParserLib: Move IRQ map to arch folder"
> I think the file DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtUtility.c should be renamed to 
> DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtInterrupt.c
> 
> If you agree with the above, I will make the necessary changes before merging.
> 
> With that,
> 
> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
> 
> Regards,
> 
> Sami Mujawar
> 
> 
> On 19/06/2024, 23:06, "Pierre Gondois" <pierre.gondois@arm.com <mailto:pierre.gondois@arm.com>> wrote:
> 
> 
> The DynamicTables framework has mainly been developed/tested against Arm
> architecture. While still trying to have re-usable libraries, opening the
> framework to other architectures implies some re-organization.
> 
> 
> The libraries that are generic enough to be directly re-used are moved
> to a Common/ directory. For some libraries, additional arch-specific hooks
> have been added to allow architectures specific modifications.
> 
> 
> ---
> 
> 
> Changes can be seen at:
> https://github.com/pierregondois/edk2/tree/pg/dyntables_libraries_reorg <https://github.com/pierregondois/edk2/tree/pg/dyntables_libraries_reorg>
> 
> 
> ---
> 
> 
> References:
> 1. Staging branch creation:
> URL: https://edk2.groups.io/g/devel/message/114790 <https://edk2.groups.io/g/devel/message/114790>
> 
> 
> 2. edk2-staging Repo
> URL: https://github.com/tianocore/edk2-staging.git <https://github.com/tianocore/edk2-staging.git>
> Branch Name: dynamictables-reorg
> 
> 
> 3. edk2-platforms Repo
> URL: https://github.com/tianocore/edk2-platforms.git <https://github.com/tianocore/edk2-platforms.git>
> Branch Name: devel-dynamictables-reorg
> 
> 
> ---
> 
> 
> Cc: AbdulLateef Attar <AbdulLateef.Attar@amd.com <mailto:AbdulLateef.Attar@amd.com>>
> Cc: Girish Mahadevan <gmahadevan@nvidia.com <mailto:gmahadevan@nvidia.com>>
> Cc: Jeff Brasen <jbrasen@nvidia.com <mailto:jbrasen@nvidia.com>>
> Cc: Jeshua Smith <jeshuas@nvidia.com <mailto:jeshuas@nvidia.com>>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com <mailto:quic_llindhol@quicinc.com>>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com <mailto:meenakshi.aggarwal@nxp.com>>
> Cc: Pierre Gondois <pierre.gondois@arm.com <mailto:pierre.gondois@arm.com>>
> Cc: Sami Mujawar <sami.mujawar@arm.com <mailto:sami.mujawar@arm.com>>
> Cc: Sunil V L <sunilvl@ventanamicro.com <mailto:sunilvl@ventanamicro.com>>
> Cc: Yeo Reum Yun <YeoReum.Yun@arm.com <mailto:YeoReum.Yun@arm.com>>
> 
> 
> Pierre Gondois (15):
> DynamicTablesPkg: Acpi: Move generic libraries to common folder
> DynamicTablesPkg: Acpi: Prepare common libraries to support other
> archs
> DynamicTablesPkg: AcpiFadtLib: Prepare to support other archs
> DynamicTablesPkg: AcpiDbg2Lib: Prepare to support other archs
> DynamicTablesPkg: AcpiSpcrLib: Prepare to support other archs
> DynamicTablesPkg: AcpiSratLib: Prepare to support other archs
> DynamicTablesPkg: AcpiSsdtCpuTopologyLib: Avoid dependency on GICC
> DynamicTablesPkg: DynamicTableManagerDxe: Refactor PresenceArray
> DynamicTablesPkg: FdtHwInfoParserLib: Move ARM parsers to Arm
> directory
> DynamicTablesPkg: FdtHwInfoParserLib: Refactor to prepare for other
> archs
> DynamicTablesPkg: FdtHwInfoParserLib: Make Pci parser arch neutral
> DynamicTablesPkg: FdtHwInfoParserLib: Make Serial Port parser arch
> neutral
> DynamicTablesPkg: FdtHwInfoParserLib: Move ArmLib.h to ArmGicCParser.c
> DynamicTablesPkg: FdtHwInfoParserLib: Move IRQ map to arch folder
> DynamicTablesPkg: FdtHwInfoParserLib: Create wrapper to get INTC addr
> cells
> 
> 
> .../Arm/ArmDynamicTableManagerDxe.c | 63 +++
> .../Common/CommonDynamicTableManagerDxe.c | 58 +++
> .../DynamicTableManagerDxe.c | 70 +--
> .../DynamicTableManagerDxe.h | 63 +++
> .../DynamicTableManagerDxe.inf | 7 +
> DynamicTablesPkg/DynamicTables.dsc.inc | 64 +--
> .../SsdtCpuTopologyGenerator.h | 147 -------
> .../AcpiDbg2Lib/AcpiDbg2Lib.inf} | 22 +-
> .../Common/AcpiDbg2Lib/Arm/ArmDbg2Generator.c | 67 +++
> .../AcpiDbg2Lib/Common/CommonDbg2Generator.c | 59 +++
> .../AcpiDbg2Lib}/Dbg2Generator.c | 24 +-
> .../Acpi/Common/AcpiDbg2Lib/Dbg2Generator.h | 56 +++
> .../AcpiFadtLib/AcpiFadtLib.inf} | 16 +-
> .../Common/AcpiFadtLib/Arm/ArmFadtGenerator.c | 126 ++++++
> .../AcpiFadtLib/Common/CommonFadtGenerator.c | 46 ++
> .../AcpiFadtLib}/FadtGenerator.c | 86 +---
> .../Acpi/Common/AcpiFadtLib/FadtGenerator.h | 35 ++
> .../AcpiMcfgLib/AcpiMcfgLib.inf} | 9 +-
> .../AcpiMcfgLib}/McfgGenerator.c | 0
> .../AcpiPcctLib/AcpiPcctLib.inf} | 2 +-
> .../AcpiPcctLib}/PcctGenerator.c | 0
> .../AcpiPcctLib}/PcctGenerator.h | 0
> .../AcpiPpttLib/AcpiPpttLib.inf} | 2 +-
> .../AcpiPpttLib}/PpttGenerator.c | 0
> .../AcpiPpttLib}/PpttGenerator.h | 0
> .../AcpiRawLib/AcpiRawLib.inf} | 9 +-
> .../AcpiRawLib}/RawGenerator.c | 0
> .../AcpiSpcrLib/AcpiSpcrLib.inf} | 9 +-
> .../AcpiSpcrLib}/SpcrGenerator.c | 2 +-
> .../AcpiSratLib/AcpiSratLib.inf} | 9 +-
> .../Common/AcpiSratLib/Arm/ArmSratGenerator.c | 262 +++++++++++
> .../AcpiSratLib/Common/CommonSratGenerator.c | 77 ++++
> .../AcpiSratLib}/SratGenerator.c | 214 +--------
> .../Acpi/Common/AcpiSratLib/SratGenerator.h | 59 +++
> .../Arm/ArmSsdtCpuTopologyGenerator.c | 408 ++++++++++++++++++
> .../SsdtCpuTopologyGenerator.c | 341 ++-------------
> .../SsdtCpuTopologyGenerator.h | 343 +++++++++++++++
> .../SsdtCpuTopologyLib.inf} | 9 +-
> .../AcpiSsdtPcieLib}/SsdtPcieGenerator.c | 2 +-
> .../AcpiSsdtPcieLib}/SsdtPcieGenerator.h | 0
> .../AcpiSsdtPcieLib/SsdtPcieLib.inf} | 2 +-
> .../SsdtSerialPortGenerator.c | 0
> .../SsdtSerialPortLib.inf} | 6 +-
> .../Arm/ArmFdtHwInfoParser.c | 83 ++++
> .../FdtHwInfoParserLib/Arm/ArmFdtUtility.c | 118 +++++
> .../{ => Arm}/BootArch/ArmBootArchParser.c | 2 +-
> .../{ => Arm}/BootArch/ArmBootArchParser.h | 0
> .../GenericTimer/ArmGenericTimerParser.c | 4 +-
> .../GenericTimer/ArmGenericTimerParser.h | 0
> .../{ => Arm}/Gic/ArmGicCParser.c | 5 +-
> .../{ => Arm}/Gic/ArmGicCParser.h | 0
> .../{ => Arm}/Gic/ArmGicDParser.c | 4 +-
> .../{ => Arm}/Gic/ArmGicDParser.h | 0
> .../{ => Arm}/Gic/ArmGicDispatcher.c | 12 +-
> .../{ => Arm}/Gic/ArmGicDispatcher.h | 0
> .../{ => Arm}/Gic/ArmGicItsParser.c | 4 +-
> .../{ => Arm}/Gic/ArmGicItsParser.h | 0
> .../{ => Arm}/Gic/ArmGicMsiFrameParser.c | 4 +-
> .../{ => Arm}/Gic/ArmGicMsiFrameParser.h | 0
> .../{ => Arm}/Gic/ArmGicRParser.c | 4 +-
> .../{ => Arm}/Gic/ArmGicRParser.h | 0
> .../FdtHwInfoParserLib/FdtHwInfoParser.c | 78 +---
> .../FdtHwInfoParserLib/FdtHwInfoParser.h | 27 ++
> .../FdtHwInfoParserInclude.h | 1 -
> .../FdtHwInfoParserLib/FdtHwInfoParserLib.inf | 48 ++-
> .../Library/FdtHwInfoParserLib/FdtUtility.c | 71 ---
> .../Library/FdtHwInfoParserLib/FdtUtility.h | 30 ++
> ...igSpaceParser.c => PciConfigSpaceParser.c} | 19 +-
> ...igSpaceParser.h => PciConfigSpaceParser.h} | 10 +-
> ...mSerialPortParser.c => SerialPortParser.c} | 16 +-
> ...mSerialPortParser.h => SerialPortParser.h} | 8 +-
> 71 files changed, 2248 insertions(+), 1074 deletions(-)
> create mode 100644 DynamicTablesPkg/Drivers/DynamicTableManagerDxe/Arm/ArmDynamicTableManagerDxe.c
> create mode 100644 DynamicTablesPkg/Drivers/DynamicTableManagerDxe/Common/CommonDynamicTableManagerDxe.c
> create mode 100644 DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.h
> delete mode 100644 DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.h
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiDbg2LibArm/AcpiDbg2LibArm.inf => Common/AcpiDbg2Lib/AcpiDbg2Lib.inf} (75%)
> create mode 100644 DynamicTablesPkg/Library/Acpi/Common/AcpiDbg2Lib/Arm/ArmDbg2Generator.c
> create mode 100644 DynamicTablesPkg/Library/Acpi/Common/AcpiDbg2Lib/Common/CommonDbg2Generator.c
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiDbg2LibArm => Common/AcpiDbg2Lib}/Dbg2Generator.c (93%)
> create mode 100644 DynamicTablesPkg/Library/Acpi/Common/AcpiDbg2Lib/Dbg2Generator.h
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiFadtLibArm/AcpiFadtLibArm.inf => Common/AcpiFadtLib/AcpiFadtLib.inf} (75%)
> create mode 100644 DynamicTablesPkg/Library/Acpi/Common/AcpiFadtLib/Arm/ArmFadtGenerator.c
> create mode 100644 DynamicTablesPkg/Library/Acpi/Common/AcpiFadtLib/Common/CommonFadtGenerator.c
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiFadtLibArm => Common/AcpiFadtLib}/FadtGenerator.c (87%)
> create mode 100644 DynamicTablesPkg/Library/Acpi/Common/AcpiFadtLib/FadtGenerator.h
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiMcfgLibArm/AcpiMcfgLibArm.inf => Common/AcpiMcfgLib/AcpiMcfgLib.inf} (85%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiMcfgLibArm => Common/AcpiMcfgLib}/McfgGenerator.c (100%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiPcctLibArm/AcpiPcctLibArm.inf => Common/AcpiPcctLib/AcpiPcctLib.inf} (90%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiPcctLibArm => Common/AcpiPcctLib}/PcctGenerator.c (100%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiPcctLibArm => Common/AcpiPcctLib}/PcctGenerator.h (100%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf => Common/AcpiPpttLib/AcpiPpttLib.inf} (90%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiPpttLibArm => Common/AcpiPpttLib}/PpttGenerator.c (100%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiPpttLibArm => Common/AcpiPpttLib}/PpttGenerator.h (100%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiRawLibArm/AcpiRawLibArm.inf => Common/AcpiRawLib/AcpiRawLib.inf} (85%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiRawLibArm => Common/AcpiRawLib}/RawGenerator.c (100%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.inf => Common/AcpiSpcrLib/AcpiSpcrLib.inf} (86%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiSpcrLibArm => Common/AcpiSpcrLib}/SpcrGenerator.c (96%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiSratLibArm/AcpiSratLibArm.inf => Common/AcpiSratLib/AcpiSratLib.inf} (74%)
> create mode 100644 DynamicTablesPkg/Library/Acpi/Common/AcpiSratLib/Arm/ArmSratGenerator.c
> create mode 100644 DynamicTablesPkg/Library/Acpi/Common/AcpiSratLib/Common/CommonSratGenerator.c
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiSratLibArm => Common/AcpiSratLib}/SratGenerator.c (75%)
> create mode 100644 DynamicTablesPkg/Library/Acpi/Common/AcpiSratLib/SratGenerator.h
> create mode 100644 DynamicTablesPkg/Library/Acpi/Common/AcpiSsdtCpuTopologyLib/Arm/ArmSsdtCpuTopologyGenerator.c
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiSsdtCpuTopologyLibArm => Common/AcpiSsdtCpuTopologyLib}/SsdtCpuTopologyGenerator.c (79%)
> create mode 100644 DynamicTablesPkg/Library/Acpi/Common/AcpiSsdtCpuTopologyLib/SsdtCpuTopologyGenerator.h
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf => Common/AcpiSsdtCpuTopologyLib/SsdtCpuTopologyLib.inf} (80%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiSsdtPcieLibArm => Common/AcpiSsdtPcieLib}/SsdtPcieGenerator.c (96%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiSsdtPcieLibArm => Common/AcpiSsdtPcieLib}/SsdtPcieGenerator.h (100%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf => Common/AcpiSsdtPcieLib/SsdtPcieLib.inf} (91%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiSsdtSerialPortLibArm => Common/AcpiSsdtSerialPortLib}/SsdtSerialPortGenerator.c (100%)
> rename DynamicTablesPkg/Library/Acpi/{Arm/AcpiSsdtSerialPortLibArm/SsdtSerialPortLibArm.inf => Common/AcpiSsdtSerialPortLib/SsdtSerialPortLib.inf} (87%)
> create mode 100644 DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtHwInfoParser.c
> create mode 100644 DynamicTablesPkg/Library/FdtHwInfoParserLib/Arm/ArmFdtUtility.c
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/BootArch/ArmBootArchParser.c (95%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/BootArch/ArmBootArchParser.h (100%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/GenericTimer/ArmGenericTimerParser.c (95%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/GenericTimer/ArmGenericTimerParser.h (100%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/Gic/ArmGicCParser.c (96%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/Gic/ArmGicCParser.h (100%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/Gic/ArmGicDParser.c (95%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/Gic/ArmGicDParser.h (100%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/Gic/ArmGicDispatcher.c (92%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/Gic/ArmGicDispatcher.h (100%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/Gic/ArmGicItsParser.c (95%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/Gic/ArmGicItsParser.h (100%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/Gic/ArmGicMsiFrameParser.c (95%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/Gic/ArmGicMsiFrameParser.h (100%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/Gic/ArmGicRParser.c (95%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/{ => Arm}/Gic/ArmGicRParser.h (100%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/Pci/{ArmPciConfigSpaceParser.c => PciConfigSpaceParser.c} (95%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/Pci/{ArmPciConfigSpaceParser.h => PciConfigSpaceParser.h} (93%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/{ArmSerialPortParser.c => SerialPortParser.c} (95%)
> rename DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/{ArmSerialPortParser.h => SerialPortParser.h} (89%)
> 
> 
> -- 
> 2.25.1
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119774): https://edk2.groups.io/g/devel/message/119774
Mute This Topic: https://groups.io/mt/106770151/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-