Apply platform_device_msi_init_and_alloc_irqs() to add support for
MSIs when the IOMMU is a platform device.
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
drivers/iommu/riscv/iommu-platform.c | 102 ++++++++++++++++++++++-----
1 file changed, 84 insertions(+), 18 deletions(-)
diff --git a/drivers/iommu/riscv/iommu-platform.c b/drivers/iommu/riscv/iommu-platform.c
index da336863f152..89aa622bcbde 100644
--- a/drivers/iommu/riscv/iommu-platform.c
+++ b/drivers/iommu/riscv/iommu-platform.c
@@ -11,18 +11,41 @@
*/
#include <linux/kernel.h>
+#include <linux/of_irq.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
+#include <linux/msi.h>
#include "iommu-bits.h"
#include "iommu.h"
+static void riscv_iommu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
+{
+ struct device *dev = msi_desc_to_dev(desc);
+ struct riscv_iommu_device *iommu = dev_get_drvdata(dev);
+ u16 idx = desc->msi_index;
+ u64 addr;
+
+ addr = ((u64)msg->address_hi << 32) | msg->address_lo;
+
+ if (addr != (addr & RISCV_IOMMU_MSI_CFG_TBL_ADDR))
+ pr_warn_once("uh oh, the IOMMU can't send MSIs to 0x%llx, sending to 0x%llx instead\n",
+ addr, addr & RISCV_IOMMU_MSI_CFG_TBL_ADDR);
+
+ addr &= RISCV_IOMMU_MSI_CFG_TBL_ADDR;
+
+ riscv_iommu_writeq(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_ADDR(idx), addr);
+ riscv_iommu_writel(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_DATA(idx), msg->data);
+ riscv_iommu_writel(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_CTRL(idx), 0);
+}
+
static int riscv_iommu_platform_probe(struct platform_device *pdev)
{
+ enum riscv_iommu_igs_settings igs;
struct device *dev = &pdev->dev;
struct riscv_iommu_device *iommu = NULL;
struct resource *res = NULL;
- int vec;
+ int vec, ret;
iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
if (!iommu)
@@ -40,16 +63,6 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
iommu->caps = riscv_iommu_readq(iommu, RISCV_IOMMU_REG_CAPABILITIES);
iommu->fctl = riscv_iommu_readl(iommu, RISCV_IOMMU_REG_FCTL);
- /* For now we only support WSI */
- switch (FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps)) {
- case RISCV_IOMMU_CAPABILITIES_IGS_WSI:
- case RISCV_IOMMU_CAPABILITIES_IGS_BOTH:
- break;
- default:
- return dev_err_probe(dev, -ENODEV,
- "unable to use wire-signaled interrupts\n");
- }
-
iommu->irqs_count = platform_irq_count(pdev);
if (iommu->irqs_count <= 0)
return dev_err_probe(dev, -ENODEV,
@@ -57,13 +70,60 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
if (iommu->irqs_count > RISCV_IOMMU_INTR_COUNT)
iommu->irqs_count = RISCV_IOMMU_INTR_COUNT;
- for (vec = 0; vec < iommu->irqs_count; vec++)
- iommu->irqs[vec] = platform_get_irq(pdev, vec);
+ igs = FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps);
+ switch (igs) {
+ case RISCV_IOMMU_CAPABILITIES_IGS_BOTH:
+ case RISCV_IOMMU_CAPABILITIES_IGS_MSI:
+ if (is_of_node(dev->fwnode))
+ of_msi_configure(dev, to_of_node(dev->fwnode));
+
+ if (!dev_get_msi_domain(dev)) {
+ dev_warn(dev, "failed to find an MSI domain");
+ goto msi_fail;
+ }
+
+ ret = platform_device_msi_init_and_alloc_irqs(dev, iommu->irqs_count,
+ riscv_iommu_write_msi_msg);
+ if (ret) {
+ dev_warn(dev, "failed to allocate MSIs");
+ goto msi_fail;
+ }
+
+ for (vec = 0; vec < iommu->irqs_count; vec++)
+ iommu->irqs[vec] = msi_get_virq(dev, vec);
+
+ /* Enable message-signaled interrupts, fctl.WSI */
+ if (iommu->fctl & RISCV_IOMMU_FCTL_WSI) {
+ iommu->fctl ^= RISCV_IOMMU_FCTL_WSI;
+ riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
+ }
+
+ dev_info(dev, "using MSIs\n");
+ break;
+
+msi_fail:
+ if (igs != RISCV_IOMMU_CAPABILITIES_IGS_BOTH) {
+ dev_warn(dev, "\n");
+ return dev_err_probe(dev, -ENODEV,
+ "unable to use wire-signaled interrupts\n");
+ }
+
+ dev_warn(dev, " - falling back to wired irqs\n");
+ fallthrough;
- /* Enable wire-signaled interrupts, fctl.WSI */
- if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
- iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
- riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
+ case RISCV_IOMMU_CAPABILITIES_IGS_WSI:
+ for (vec = 0; vec < iommu->irqs_count; vec++)
+ iommu->irqs[vec] = platform_get_irq(pdev, vec);
+
+ /* Enable wire-signaled interrupts, fctl.WSI */
+ if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
+ iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
+ riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
+ }
+ dev_info(dev, "using wire-signaled interrupts\n");
+ break;
+ default:
+ return dev_err_probe(dev, -ENODEV, "invalid IGS\n");
}
return riscv_iommu_init(iommu);
@@ -71,7 +131,13 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
static void riscv_iommu_platform_remove(struct platform_device *pdev)
{
- riscv_iommu_remove(dev_get_drvdata(&pdev->dev));
+ struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev);
+ bool msi = !(iommu->fctl & RISCV_IOMMU_FCTL_WSI);
+
+ riscv_iommu_remove(iommu);
+
+ if (msi)
+ platform_device_msi_free_irqs_all(&pdev->dev);
};
static const struct of_device_id riscv_iommu_of_match[] = {
--
2.47.0
Hi Drew,
On 2024-11-06 11:51 AM, Andrew Jones wrote:
> Apply platform_device_msi_init_and_alloc_irqs() to add support for
> MSIs when the IOMMU is a platform device.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
> drivers/iommu/riscv/iommu-platform.c | 102 ++++++++++++++++++++++-----
> 1 file changed, 84 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iommu/riscv/iommu-platform.c b/drivers/iommu/riscv/iommu-platform.c
> index da336863f152..89aa622bcbde 100644
> --- a/drivers/iommu/riscv/iommu-platform.c
> +++ b/drivers/iommu/riscv/iommu-platform.c
> @@ -11,18 +11,41 @@
> */
>
> #include <linux/kernel.h>
> +#include <linux/of_irq.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> +#include <linux/msi.h>
If you respin, please keep these sorted.
>
> #include "iommu-bits.h"
> #include "iommu.h"
>
> +static void riscv_iommu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> + struct device *dev = msi_desc_to_dev(desc);
> + struct riscv_iommu_device *iommu = dev_get_drvdata(dev);
> + u16 idx = desc->msi_index;
> + u64 addr;
> +
> + addr = ((u64)msg->address_hi << 32) | msg->address_lo;
> +
> + if (addr != (addr & RISCV_IOMMU_MSI_CFG_TBL_ADDR))
> + pr_warn_once("uh oh, the IOMMU can't send MSIs to 0x%llx, sending to 0x%llx instead\n",
> + addr, addr & RISCV_IOMMU_MSI_CFG_TBL_ADDR);
Can this use dev_warn_once()? And should it really be only a warning?
Configuring the IOMMU to write to some other address seems dangerous. I guess
there's no clean way to handle this error, since this function cannot fail, and
irq_chip_compose_msi_msg() isn't supposed to fail either.
> +
> + addr &= RISCV_IOMMU_MSI_CFG_TBL_ADDR;
> +
> + riscv_iommu_writeq(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_ADDR(idx), addr);
> + riscv_iommu_writel(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_DATA(idx), msg->data);
> + riscv_iommu_writel(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_CTRL(idx), 0);
> +}
> +
> static int riscv_iommu_platform_probe(struct platform_device *pdev)
> {
> + enum riscv_iommu_igs_settings igs;
> struct device *dev = &pdev->dev;
> struct riscv_iommu_device *iommu = NULL;
> struct resource *res = NULL;
> - int vec;
> + int vec, ret;
>
> iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
> if (!iommu)
> @@ -40,16 +63,6 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
> iommu->caps = riscv_iommu_readq(iommu, RISCV_IOMMU_REG_CAPABILITIES);
> iommu->fctl = riscv_iommu_readl(iommu, RISCV_IOMMU_REG_FCTL);
>
> - /* For now we only support WSI */
> - switch (FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps)) {
> - case RISCV_IOMMU_CAPABILITIES_IGS_WSI:
> - case RISCV_IOMMU_CAPABILITIES_IGS_BOTH:
> - break;
> - default:
> - return dev_err_probe(dev, -ENODEV,
> - "unable to use wire-signaled interrupts\n");
> - }
> -
> iommu->irqs_count = platform_irq_count(pdev);
> if (iommu->irqs_count <= 0)
> return dev_err_probe(dev, -ENODEV,
> @@ -57,13 +70,60 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
> if (iommu->irqs_count > RISCV_IOMMU_INTR_COUNT)
> iommu->irqs_count = RISCV_IOMMU_INTR_COUNT;
>
> - for (vec = 0; vec < iommu->irqs_count; vec++)
> - iommu->irqs[vec] = platform_get_irq(pdev, vec);
> + igs = FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps);
> + switch (igs) {
> + case RISCV_IOMMU_CAPABILITIES_IGS_BOTH:
> + case RISCV_IOMMU_CAPABILITIES_IGS_MSI:
> + if (is_of_node(dev->fwnode))
> + of_msi_configure(dev, to_of_node(dev->fwnode));
> +
> + if (!dev_get_msi_domain(dev)) {
> + dev_warn(dev, "failed to find an MSI domain");
> + goto msi_fail;
> + }
> +
> + ret = platform_device_msi_init_and_alloc_irqs(dev, iommu->irqs_count,
> + riscv_iommu_write_msi_msg);
> + if (ret) {
> + dev_warn(dev, "failed to allocate MSIs");
> + goto msi_fail;
> + }
> +
> + for (vec = 0; vec < iommu->irqs_count; vec++)
> + iommu->irqs[vec] = msi_get_virq(dev, vec);
> +
> + /* Enable message-signaled interrupts, fctl.WSI */
> + if (iommu->fctl & RISCV_IOMMU_FCTL_WSI) {
> + iommu->fctl ^= RISCV_IOMMU_FCTL_WSI;
> + riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
> + }
> +
> + dev_info(dev, "using MSIs\n");
> + break;
> +
> +msi_fail:
> + if (igs != RISCV_IOMMU_CAPABILITIES_IGS_BOTH) {
> + dev_warn(dev, "\n");
> + return dev_err_probe(dev, -ENODEV,
> + "unable to use wire-signaled interrupts\n");
Is the dev_warn() just to call attention to the following error? There's no
guarantee that these two messages will be printed adjacently.
> + }
> +
> + dev_warn(dev, " - falling back to wired irqs\n");
No need for the extra hyphen here.
Regards,
Samuel
> + fallthrough;
>
> - /* Enable wire-signaled interrupts, fctl.WSI */
> - if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
> - iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
> - riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
> + case RISCV_IOMMU_CAPABILITIES_IGS_WSI:
> + for (vec = 0; vec < iommu->irqs_count; vec++)
> + iommu->irqs[vec] = platform_get_irq(pdev, vec);
> +
> + /* Enable wire-signaled interrupts, fctl.WSI */
> + if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
> + iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
> + riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
> + }
> + dev_info(dev, "using wire-signaled interrupts\n");
> + break;
> + default:
> + return dev_err_probe(dev, -ENODEV, "invalid IGS\n");
> }
>
> return riscv_iommu_init(iommu);
> @@ -71,7 +131,13 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
>
> static void riscv_iommu_platform_remove(struct platform_device *pdev)
> {
> - riscv_iommu_remove(dev_get_drvdata(&pdev->dev));
> + struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev);
> + bool msi = !(iommu->fctl & RISCV_IOMMU_FCTL_WSI);
> +
> + riscv_iommu_remove(iommu);
> +
> + if (msi)
> + platform_device_msi_free_irqs_all(&pdev->dev);
> };
>
> static const struct of_device_id riscv_iommu_of_match[] = {
On Wed, Nov 06, 2024 at 01:45:23PM -0600, Samuel Holland wrote:
> Hi Drew,
>
> On 2024-11-06 11:51 AM, Andrew Jones wrote:
> > Apply platform_device_msi_init_and_alloc_irqs() to add support for
> > MSIs when the IOMMU is a platform device.
> >
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> > drivers/iommu/riscv/iommu-platform.c | 102 ++++++++++++++++++++++-----
> > 1 file changed, 84 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/iommu/riscv/iommu-platform.c b/drivers/iommu/riscv/iommu-platform.c
> > index da336863f152..89aa622bcbde 100644
> > --- a/drivers/iommu/riscv/iommu-platform.c
> > +++ b/drivers/iommu/riscv/iommu-platform.c
> > @@ -11,18 +11,41 @@
> > */
> >
> > #include <linux/kernel.h>
> > +#include <linux/of_irq.h>
> > #include <linux/of_platform.h>
> > #include <linux/platform_device.h>
> > +#include <linux/msi.h>
>
> If you respin, please keep these sorted.
Sure
>
> >
> > #include "iommu-bits.h"
> > #include "iommu.h"
> >
> > +static void riscv_iommu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > +{
> > + struct device *dev = msi_desc_to_dev(desc);
> > + struct riscv_iommu_device *iommu = dev_get_drvdata(dev);
> > + u16 idx = desc->msi_index;
> > + u64 addr;
> > +
> > + addr = ((u64)msg->address_hi << 32) | msg->address_lo;
> > +
> > + if (addr != (addr & RISCV_IOMMU_MSI_CFG_TBL_ADDR))
> > + pr_warn_once("uh oh, the IOMMU can't send MSIs to 0x%llx, sending to 0x%llx instead\n",
> > + addr, addr & RISCV_IOMMU_MSI_CFG_TBL_ADDR);
>
> Can this use dev_warn_once()? And should it really be only a warning?
> Configuring the IOMMU to write to some other address seems dangerous. I guess
> there's no clean way to handle this error, since this function cannot fail, and
> irq_chip_compose_msi_msg() isn't supposed to fail either.
I'll change it to a dev_err_once().
>
> > +
> > + addr &= RISCV_IOMMU_MSI_CFG_TBL_ADDR;
> > +
> > + riscv_iommu_writeq(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_ADDR(idx), addr);
> > + riscv_iommu_writel(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_DATA(idx), msg->data);
> > + riscv_iommu_writel(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_CTRL(idx), 0);
> > +}
> > +
> > static int riscv_iommu_platform_probe(struct platform_device *pdev)
> > {
> > + enum riscv_iommu_igs_settings igs;
> > struct device *dev = &pdev->dev;
> > struct riscv_iommu_device *iommu = NULL;
> > struct resource *res = NULL;
> > - int vec;
> > + int vec, ret;
> >
> > iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
> > if (!iommu)
> > @@ -40,16 +63,6 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
> > iommu->caps = riscv_iommu_readq(iommu, RISCV_IOMMU_REG_CAPABILITIES);
> > iommu->fctl = riscv_iommu_readl(iommu, RISCV_IOMMU_REG_FCTL);
> >
> > - /* For now we only support WSI */
> > - switch (FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps)) {
> > - case RISCV_IOMMU_CAPABILITIES_IGS_WSI:
> > - case RISCV_IOMMU_CAPABILITIES_IGS_BOTH:
> > - break;
> > - default:
> > - return dev_err_probe(dev, -ENODEV,
> > - "unable to use wire-signaled interrupts\n");
> > - }
> > -
> > iommu->irqs_count = platform_irq_count(pdev);
> > if (iommu->irqs_count <= 0)
> > return dev_err_probe(dev, -ENODEV,
> > @@ -57,13 +70,60 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
> > if (iommu->irqs_count > RISCV_IOMMU_INTR_COUNT)
> > iommu->irqs_count = RISCV_IOMMU_INTR_COUNT;
> >
> > - for (vec = 0; vec < iommu->irqs_count; vec++)
> > - iommu->irqs[vec] = platform_get_irq(pdev, vec);
> > + igs = FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps);
> > + switch (igs) {
> > + case RISCV_IOMMU_CAPABILITIES_IGS_BOTH:
> > + case RISCV_IOMMU_CAPABILITIES_IGS_MSI:
> > + if (is_of_node(dev->fwnode))
> > + of_msi_configure(dev, to_of_node(dev->fwnode));
> > +
> > + if (!dev_get_msi_domain(dev)) {
> > + dev_warn(dev, "failed to find an MSI domain");
> > + goto msi_fail;
> > + }
> > +
> > + ret = platform_device_msi_init_and_alloc_irqs(dev, iommu->irqs_count,
> > + riscv_iommu_write_msi_msg);
> > + if (ret) {
> > + dev_warn(dev, "failed to allocate MSIs");
> > + goto msi_fail;
> > + }
> > +
> > + for (vec = 0; vec < iommu->irqs_count; vec++)
> > + iommu->irqs[vec] = msi_get_virq(dev, vec);
> > +
> > + /* Enable message-signaled interrupts, fctl.WSI */
> > + if (iommu->fctl & RISCV_IOMMU_FCTL_WSI) {
> > + iommu->fctl ^= RISCV_IOMMU_FCTL_WSI;
> > + riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
> > + }
> > +
> > + dev_info(dev, "using MSIs\n");
> > + break;
> > +
> > +msi_fail:
> > + if (igs != RISCV_IOMMU_CAPABILITIES_IGS_BOTH) {
> > + dev_warn(dev, "\n");
> > + return dev_err_probe(dev, -ENODEV,
> > + "unable to use wire-signaled interrupts\n");
>
> Is the dev_warn() just to call attention to the following error? There's no
> guarantee that these two messages will be printed adjacently.
I'll drop the dev_warn since it was supposed to tie in with the dev_warn
above which was missing its endline on purpose, but your statement about
avoiding split logs means I should probably just do a complete log above
and complete logs here and...
>
> > + }
> > +
> > + dev_warn(dev, " - falling back to wired irqs\n");
>
> No need for the extra hyphen here.
...here, which also means the hyphen should be dropped.
>
> Regards,
> Samuel
Thanks,
drew
>
> > + fallthrough;
> >
> > - /* Enable wire-signaled interrupts, fctl.WSI */
> > - if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
> > - iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
> > - riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
> > + case RISCV_IOMMU_CAPABILITIES_IGS_WSI:
> > + for (vec = 0; vec < iommu->irqs_count; vec++)
> > + iommu->irqs[vec] = platform_get_irq(pdev, vec);
> > +
> > + /* Enable wire-signaled interrupts, fctl.WSI */
> > + if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
> > + iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
> > + riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
> > + }
> > + dev_info(dev, "using wire-signaled interrupts\n");
> > + break;
> > + default:
> > + return dev_err_probe(dev, -ENODEV, "invalid IGS\n");
> > }
> >
> > return riscv_iommu_init(iommu);
> > @@ -71,7 +131,13 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
> >
> > static void riscv_iommu_platform_remove(struct platform_device *pdev)
> > {
> > - riscv_iommu_remove(dev_get_drvdata(&pdev->dev));
> > + struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev);
> > + bool msi = !(iommu->fctl & RISCV_IOMMU_FCTL_WSI);
> > +
> > + riscv_iommu_remove(iommu);
> > +
> > + if (msi)
> > + platform_device_msi_free_irqs_all(&pdev->dev);
> > };
> >
> > static const struct of_device_id riscv_iommu_of_match[] = {
>
© 2016 - 2026 Red Hat, Inc.