Hi Drew, Thank you very much for the review!. On Mon, Jun 30, 2025 at 09:55:09AM +0200, Andrew Jones wrote: > Hi Sunil, > > I found a few nits while skimming this. > > On Mon, Jun 30, 2025 at 09:18:01AM +0530, Sunil V L wrote: > > RISC-V IO Mapping Table (RIMT) is a static ACPI table to communicate > > IOMMU information to the OS. The spec is available at [1]. > > > > The changes at high level are, > > a) Initialize data structures required for IOMMU/device > > configuration using the data from RIMT. Provide APIs required > > for device configuration. > > b) Provide an API for IOMMU drivers to register the > > fwnode with RIMT data structures. This API will create a > > fwnode for PCIe IOMMU. > > > > [1] - https://github.com/riscv-non-isa/riscv-acpi-rimt > > > > Signed-off-by: Sunil V L <sunilvl@xxxxxxxxxxxxxxxx> > > --- > > MAINTAINERS | 1 + > > arch/riscv/Kconfig | 1 + > > drivers/acpi/Kconfig | 4 + > > drivers/acpi/riscv/Kconfig | 7 + > > drivers/acpi/riscv/Makefile | 1 + > > drivers/acpi/riscv/init.c | 2 + > > drivers/acpi/riscv/init.h | 1 + > > drivers/acpi/riscv/rimt.c | 523 ++++++++++++++++++++++++++++++++++++ > > include/linux/acpi_rimt.h | 26 ++ > > 9 files changed, 566 insertions(+) > > create mode 100644 drivers/acpi/riscv/Kconfig > > create mode 100644 drivers/acpi/riscv/rimt.c > > create mode 100644 include/linux/acpi_rimt.h > > [...] > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > index 36061f4732b7..96d64e0a7b97 100644 > > --- a/arch/riscv/Kconfig > > +++ b/arch/riscv/Kconfig > > @@ -16,6 +16,7 @@ config RISCV > > select ACPI_MCFG if (ACPI && PCI) > > select ACPI_PPTT if ACPI > > select ACPI_REDUCED_HARDWARE_ONLY if ACPI > > + select ACPI_RIMT if ACPI > > Should use tab here. > Okay. > > select ACPI_SPCR_TABLE if ACPI > > select ARCH_DMA_DEFAULT_COHERENT > > select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION [...] > > +static inline int rimt_set_fwnode(struct acpi_rimt_node *rimt_node, > > + struct fwnode_handle *fwnode) > > I see this is a faithful port of arm's iort functions, but using > 'inline' in source files is pretty pointless, so we could drop > that. > Sure. > > +{ > > + struct rimt_fwnode *np; > > + > > + np = kzalloc(sizeof(*np), GFP_ATOMIC); > [...] > > + map = ACPI_ADD_PTR(struct acpi_rimt_id_mapping, node, > > + id_mapping_offset + index * sizeof(*map)); > > + > > + /* Firmware bug! */ > > + if (!map->dest_offset) { > > + pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n", > > + node, node->type); > > Should we have a pr_fmt() definition at the top of this source file for > this pr_err? > Oh yeah. Will add. > > + return NULL; > > + } > > + > [...] > > + return err; > > +} > > + > > +#else > > +int rimt_iommu_configure_id(struct device *dev, const u32 *id_in) > > +{ > > + return -ENODEV; > > +} > > This is unnecessary since we have the stub in the header file. > Right. > > +#endif > > + > [...] > > +int rimt_iommu_configure_id(struct device *dev, const u32 *id_in); > > + > > ubernit: no need for this blank line > Sure. Thanks! Sunil
