On Wed, Jul 03, 2024 at 09:39:22AM +0000, Sami Mujawar wrote: > Hi Sunil, > > I think we can look into that. My initial thoughts are that this can be > solved using a Pcd. However, we need a bit of investigation. > Is it ok if we address that in a separate patch? > Sure!.
Thanks, Sunil > Regards, > > Sami Mujawar > > On 03/07/2024, 10:37, "Sunil V L" <suni...@ventanamicro.com > <mailto:suni...@ventanamicro.com>> wrote: > > > 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 > <mailto: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 > > <mailto:sami.muja...@arm.com>> > > > > Regards, > > > > Sami Mujawar > > > > > > On 19/06/2024, 23:06, "Pierre Gondois" <pierre.gond...@arm.com > > <mailto:pierre.gond...@arm.com> <mailto: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> > > <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> > > <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> > > <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> > > <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> <mailto:abdullateef.at...@amd.com > > <mailto:abdullateef.at...@amd.com>>> > > Cc: Girish Mahadevan <gmahade...@nvidia.com <mailto:gmahade...@nvidia.com> > > <mailto:gmahade...@nvidia.com <mailto:gmahade...@nvidia.com>>> > > Cc: Jeff Brasen <jbra...@nvidia.com <mailto:jbra...@nvidia.com> > > <mailto:jbra...@nvidia.com <mailto:jbra...@nvidia.com>>> > > Cc: Jeshua Smith <jesh...@nvidia.com <mailto:jesh...@nvidia.com> > > <mailto:jesh...@nvidia.com <mailto:jesh...@nvidia.com>>> > > Cc: Leif Lindholm <quic_llind...@quicinc.com > > <mailto:quic_llind...@quicinc.com> <mailto:quic_llind...@quicinc.com > > <mailto:quic_llind...@quicinc.com>>> > > Cc: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com > > <mailto:meenakshi.aggar...@nxp.com> <mailto:meenakshi.aggar...@nxp.com > > <mailto:meenakshi.aggar...@nxp.com>>> > > Cc: Pierre Gondois <pierre.gond...@arm.com <mailto:pierre.gond...@arm.com> > > <mailto:pierre.gond...@arm.com <mailto:pierre.gond...@arm.com>>> > > Cc: Sami Mujawar <sami.muja...@arm.com <mailto:sami.muja...@arm.com> > > <mailto:sami.muja...@arm.com <mailto:sami.muja...@arm.com>>> > > Cc: Sunil V L <suni...@ventanamicro.com <mailto:suni...@ventanamicro.com> > > <mailto:suni...@ventanamicro.com <mailto:suni...@ventanamicro.com>>> > > Cc: Yeo Reum Yun <yeoreum....@arm.com <mailto:yeoreum....@arm.com> > > <mailto: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 (#119776): https://edk2.groups.io/g/devel/message/119776 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] -=-=-=-=-=-=-=-=-=-=-=-
