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 <suni...@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.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 (#119774): https://edk2.groups.io/g/devel/message/119774 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] -=-=-=-=-=-=-=-=-=-=-=-
