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.muja...@arm.com> Regards, Sami Mujawar On 19/06/2024, 23:06, "Pierre Gondois" <pierre.gond...@arm.com <mailto:pierre.gond...@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.at...@amd.com <mailto:abdullateef.at...@amd.com>> Cc: Girish Mahadevan <gmahade...@nvidia.com <mailto:gmahade...@nvidia.com>> Cc: Jeff Brasen <jbra...@nvidia.com <mailto:jbra...@nvidia.com>> Cc: Jeshua Smith <jesh...@nvidia.com <mailto:jesh...@nvidia.com>> Cc: Leif Lindholm <quic_llind...@quicinc.com <mailto:quic_llind...@quicinc.com>> Cc: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com <mailto:meenakshi.aggar...@nxp.com>> Cc: Pierre Gondois <pierre.gond...@arm.com <mailto:pierre.gond...@arm.com>> Cc: Sami Mujawar <sami.muja...@arm.com <mailto:sami.muja...@arm.com>> Cc: Sunil V L <suni...@ventanamicro.com <mailto:suni...@ventanamicro.com>> Cc: Yeo Reum Yun <yeoreum....@arm.com <mailto:yeoreum....@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 (#119773): https://edk2.groups.io/g/devel/message/119773 Mute This Topic: https://groups.io/mt/106770151/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
