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

["Sami Mujawar" <sami.mujawar@arm.com>]

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