| Message ID | 20250203084906.681418-17-apatel@ventanamicro.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Linux SBI MPXY and RPMI drivers | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | fail | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | fail | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | success | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
On Mon, Feb 03, 2025 at 02:19:05PM +0530, Anup Patel wrote: > From: Sunil V L <sunilvl@ventanamicro.com> > > Add ACPI support for the RISC-V RPMI system MSI based irqchip driver. ... > + if (!is_of_node(dev->fwnode)) { Please, use dev_fwnode(), But why do you need this? Can't the below simply become a no-op without this check? > + rc = riscv_acpi_get_gsi_info(dev->fwnode, &priv->gsi_base, &id, Ditto. > + &priv->nr_irqs, NULL); > + if (rc) { > + dev_err(dev, "failed to find GSI mapping\n"); > + return rc; > + } > + } ... > * then we need to set it explicitly before using any platform > * MSI functions. > */ > - if (is_of_node(dev->fwnode)) > + if (is_of_node(dev->fwnode)) { > of_msi_configure(dev, to_of_node(dev->fwnode)); > + } else { > + struct irq_domain *msi_domain; > + > + msi_domain = irq_find_matching_fwnode(imsic_acpi_get_fwnode(dev), > + DOMAIN_BUS_PLATFORM_MSI); > + if (msi_domain) Hmm... The OF case above assumes this check is not needed. Why is it special otherwise? > + dev_set_msi_domain(dev, msi_domain); > + } > > if (!dev_get_msi_domain(dev)) Even here you have a check for NULL, so I believe the conditional is simply redundant. ... > +#ifdef CONFIG_ACPI > + if (!acpi_disabled) Why? > + acpi_dev_clear_dependencies(ACPI_COMPANION(dev)); > +#endif ... > +#ifdef CONFIG_ACPI Drop this ugly ifdeffery along with ACPI_PTR(). They are more harmful than useful. ... > +static const struct acpi_device_id acpi_rpmi_sysmsi_match[] = { > + { "RSCV0006", 0 }, Drop ', 0' part as it may be converted to a pointer in the future. > + {} > +}; > +MODULE_DEVICE_TABLE(acpi, acpi_rpmi_sysmsi_match); > +#endif
On Mon, Feb 03 2025 at 14:19, Anup Patel wrote: > + > static struct platform_driver rpmi_sysmsi_driver = { > .driver = { > .name = "rpmi-sysmsi", > .of_match_table = rpmi_sysmsi_match, > + .acpi_match_table = ACPI_PTR(acpi_rpmi_sysmsi_match), Please indent .name and .of_match_table accordingly. Thanks, tglx
On Mon, Feb 03, 2025 at 11:38:28AM +0200, Andy Shevchenko wrote: > On Mon, Feb 03, 2025 at 02:19:05PM +0530, Anup Patel wrote: > > From: Sunil V L <sunilvl@ventanamicro.com> > > > > Add ACPI support for the RISC-V RPMI system MSI based irqchip driver. > > ... > > > + if (!is_of_node(dev->fwnode)) { > > Please, use dev_fwnode(), > > But why do you need this? Can't the below simply become a no-op without > this check? > > > + rc = riscv_acpi_get_gsi_info(dev->fwnode, &priv->gsi_base, &id, > > Ditto. > > > + &priv->nr_irqs, NULL); > > + if (rc) { > > + dev_err(dev, "failed to find GSI mapping\n"); > > + return rc; > > + } > > + } > > ... > > > * then we need to set it explicitly before using any platform > > * MSI functions. > > */ > > - if (is_of_node(dev->fwnode)) > > + if (is_of_node(dev->fwnode)) { > > of_msi_configure(dev, to_of_node(dev->fwnode)); > > + } else { > > + struct irq_domain *msi_domain; > > + > > + msi_domain = irq_find_matching_fwnode(imsic_acpi_get_fwnode(dev), > > + DOMAIN_BUS_PLATFORM_MSI); > > > + if (msi_domain) > > Hmm... The OF case above assumes this check is not needed. Why is it special > otherwise? > > > + dev_set_msi_domain(dev, msi_domain); > > + } > > > > if (!dev_get_msi_domain(dev)) > > Even here you have a check for NULL, so I believe the conditional is simply > redundant. > > ... > > > +#ifdef CONFIG_ACPI > > > + if (!acpi_disabled) > > Why? > > > + acpi_dev_clear_dependencies(ACPI_COMPANION(dev)); > > +#endif > > ... > > > +#ifdef CONFIG_ACPI > > Drop this ugly ifdeffery along with ACPI_PTR(). They are more harmful than > useful. > > ... > > > +static const struct acpi_device_id acpi_rpmi_sysmsi_match[] = { > > + { "RSCV0006", 0 }, > > Drop ', 0' part as it may be converted to a pointer in the future. > Thanks!. Let me address your comments in next revision. Thanks, Sunil
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index 2ae44354735b..cf96382113ce 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -599,7 +599,7 @@ config RISCV_IMSIC_PCI config RISCV_RPMI_SYSMSI bool - depends on MAILBOX + depends on RISCV && MAILBOX select IRQ_DOMAIN_HIERARCHY select GENERIC_MSI_IRQ default RISCV diff --git a/drivers/irqchip/irq-riscv-rpmi-sysmsi.c b/drivers/irqchip/irq-riscv-rpmi-sysmsi.c index 3022f0924c94..1f03241920bb 100644 --- a/drivers/irqchip/irq-riscv-rpmi-sysmsi.c +++ b/drivers/irqchip/irq-riscv-rpmi-sysmsi.c @@ -8,6 +8,7 @@ #include <linux/cpu.h> #include <linux/interrupt.h> #include <linux/irqchip.h> +#include <linux/irqchip/riscv-imsic.h> #include <linux/mailbox_client.h> #include <linux/mailbox/riscv-rpmi-message.h> #include <linux/module.h> @@ -215,6 +216,7 @@ static int rpmi_sysmsi_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct rpmi_sysmsi_priv *priv; + u32 id; int rc; priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); @@ -223,6 +225,15 @@ static int rpmi_sysmsi_probe(struct platform_device *pdev) priv->dev = dev; platform_set_drvdata(pdev, priv); + if (!is_of_node(dev->fwnode)) { + rc = riscv_acpi_get_gsi_info(dev->fwnode, &priv->gsi_base, &id, + &priv->nr_irqs, NULL); + if (rc) { + dev_err(dev, "failed to find GSI mapping\n"); + return rc; + } + } + /* Setup mailbox client */ priv->client.dev = priv->dev; priv->client.rx_callback = NULL; @@ -252,8 +263,16 @@ static int rpmi_sysmsi_probe(struct platform_device *pdev) * then we need to set it explicitly before using any platform * MSI functions. */ - if (is_of_node(dev->fwnode)) + if (is_of_node(dev->fwnode)) { of_msi_configure(dev, to_of_node(dev->fwnode)); + } else { + struct irq_domain *msi_domain; + + msi_domain = irq_find_matching_fwnode(imsic_acpi_get_fwnode(dev), + DOMAIN_BUS_PLATFORM_MSI); + if (msi_domain) + dev_set_msi_domain(dev, msi_domain); + } if (!dev_get_msi_domain(dev)) return -EPROBE_DEFER; @@ -264,6 +283,10 @@ static int rpmi_sysmsi_probe(struct platform_device *pdev) priv->nr_irqs, priv, priv)) return dev_err_probe(dev, -ENOMEM, "failed to create MSI irq domain\n"); +#ifdef CONFIG_ACPI + if (!acpi_disabled) + acpi_dev_clear_dependencies(ACPI_COMPANION(dev)); +#endif dev_info(dev, "%d system MSIs registered\n", priv->nr_irqs); return 0; } @@ -273,10 +296,19 @@ static const struct of_device_id rpmi_sysmsi_match[] = { {} }; +#ifdef CONFIG_ACPI +static const struct acpi_device_id acpi_rpmi_sysmsi_match[] = { + { "RSCV0006", 0 }, + {} +}; +MODULE_DEVICE_TABLE(acpi, acpi_rpmi_sysmsi_match); +#endif + static struct platform_driver rpmi_sysmsi_driver = { .driver = { .name = "rpmi-sysmsi", .of_match_table = rpmi_sysmsi_match, + .acpi_match_table = ACPI_PTR(acpi_rpmi_sysmsi_match), }, .probe = rpmi_sysmsi_probe, };