* [PATCH 0/2] iommu/riscv: Add platform msi support
@ 2024-11-06 17:51 ` Andrew Jones
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2024-11-06 17:51 UTC (permalink / raw)
To: iommu, linux-riscv, linux-kernel
Cc: tjeznach, joro, will, robin.murphy, paul.walmsley, palmer, aou
The first patch is fix for an issue found while preparing the second.
The second patch adds MSI support for a platform IOMMU. The patches
may be tested with QEMU when including [1].
Based on linux-next commit 5b913f5d7d7f ("Add linux-next specific files for 20241106")
[1] https://lore.kernel.org/all/20241106133407.604587-1-dbarboza@ventanamicro.com/
Andrew Jones (2):
iommu/riscv: Free irq vectors on pci remove
iommu/riscv: Add support for platform msi
drivers/iommu/riscv/iommu-pci.c | 1 +
drivers/iommu/riscv/iommu-platform.c | 102 ++++++++++++++++++++++-----
2 files changed, 85 insertions(+), 18 deletions(-)
--
2.47.0
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 0/2] iommu/riscv: Add platform msi support @ 2024-11-06 17:51 ` Andrew Jones 0 siblings, 0 replies; 14+ messages in thread From: Andrew Jones @ 2024-11-06 17:51 UTC (permalink / raw) To: iommu, linux-riscv, linux-kernel Cc: tjeznach, joro, will, robin.murphy, paul.walmsley, palmer, aou The first patch is fix for an issue found while preparing the second. The second patch adds MSI support for a platform IOMMU. The patches may be tested with QEMU when including [1]. Based on linux-next commit 5b913f5d7d7f ("Add linux-next specific files for 20241106") [1] https://lore.kernel.org/all/20241106133407.604587-1-dbarboza@ventanamicro.com/ Andrew Jones (2): iommu/riscv: Free irq vectors on pci remove iommu/riscv: Add support for platform msi drivers/iommu/riscv/iommu-pci.c | 1 + drivers/iommu/riscv/iommu-platform.c | 102 ++++++++++++++++++++++----- 2 files changed, 85 insertions(+), 18 deletions(-) -- 2.47.0 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] iommu/riscv: Free irq vectors on pci remove 2024-11-06 17:51 ` Andrew Jones @ 2024-11-06 17:51 ` Andrew Jones -1 siblings, 0 replies; 14+ messages in thread From: Andrew Jones @ 2024-11-06 17:51 UTC (permalink / raw) To: iommu, linux-riscv, linux-kernel Cc: tjeznach, joro, will, robin.murphy, paul.walmsley, palmer, aou riscv_iommu_pci_probe() calls pci_alloc_irq_vectors() which states pci_free_irq_vectors() must be called on cleanup. Fixes: 68682e9578fb ("iommu/riscv: Add RISC-V IOMMU PCIe device driver") Signed-off-by: Andrew Jones <ajones@ventanamicro.com> --- drivers/iommu/riscv/iommu-pci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/iommu/riscv/iommu-pci.c b/drivers/iommu/riscv/iommu-pci.c index c7a89143014c..25a27e627a0e 100644 --- a/drivers/iommu/riscv/iommu-pci.c +++ b/drivers/iommu/riscv/iommu-pci.c @@ -99,6 +99,7 @@ static void riscv_iommu_pci_remove(struct pci_dev *pdev) struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev); riscv_iommu_remove(iommu); + pci_free_irq_vectors(pdev); } static const struct pci_device_id riscv_iommu_pci_tbl[] = { -- 2.47.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 1/2] iommu/riscv: Free irq vectors on pci remove @ 2024-11-06 17:51 ` Andrew Jones 0 siblings, 0 replies; 14+ messages in thread From: Andrew Jones @ 2024-11-06 17:51 UTC (permalink / raw) To: iommu, linux-riscv, linux-kernel Cc: tjeznach, joro, will, robin.murphy, paul.walmsley, palmer, aou riscv_iommu_pci_probe() calls pci_alloc_irq_vectors() which states pci_free_irq_vectors() must be called on cleanup. Fixes: 68682e9578fb ("iommu/riscv: Add RISC-V IOMMU PCIe device driver") Signed-off-by: Andrew Jones <ajones@ventanamicro.com> --- drivers/iommu/riscv/iommu-pci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/iommu/riscv/iommu-pci.c b/drivers/iommu/riscv/iommu-pci.c index c7a89143014c..25a27e627a0e 100644 --- a/drivers/iommu/riscv/iommu-pci.c +++ b/drivers/iommu/riscv/iommu-pci.c @@ -99,6 +99,7 @@ static void riscv_iommu_pci_remove(struct pci_dev *pdev) struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev); riscv_iommu_remove(iommu); + pci_free_irq_vectors(pdev); } static const struct pci_device_id riscv_iommu_pci_tbl[] = { -- 2.47.0 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] iommu/riscv: Free irq vectors on pci remove 2024-11-06 17:51 ` Andrew Jones @ 2024-11-06 19:00 ` Tomasz Jeznach -1 siblings, 0 replies; 14+ messages in thread From: Tomasz Jeznach @ 2024-11-06 19:00 UTC (permalink / raw) To: Andrew Jones Cc: iommu, linux-riscv, linux-kernel, joro, will, robin.murphy, paul.walmsley, palmer, aou On Wed, Nov 6, 2024 at 9:51 AM Andrew Jones <ajones@ventanamicro.com> wrote: > > riscv_iommu_pci_probe() calls pci_alloc_irq_vectors() which > states pci_free_irq_vectors() must be called on cleanup. > > Fixes: 68682e9578fb ("iommu/riscv: Add RISC-V IOMMU PCIe device driver") > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > --- > drivers/iommu/riscv/iommu-pci.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iommu/riscv/iommu-pci.c b/drivers/iommu/riscv/iommu-pci.c > index c7a89143014c..25a27e627a0e 100644 > --- a/drivers/iommu/riscv/iommu-pci.c > +++ b/drivers/iommu/riscv/iommu-pci.c > @@ -99,6 +99,7 @@ static void riscv_iommu_pci_remove(struct pci_dev *pdev) > struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev); > > riscv_iommu_remove(iommu); > + pci_free_irq_vectors(pdev); > } > > static const struct pci_device_id riscv_iommu_pci_tbl[] = { > -- > 2.47.0 > Andrew, interrupt release call pci_free_irq_vectors() should already be called by device managed resources framework, with unwind action callback pcim_msi_release(). Callback is registered during vectors allocation: pci_alloc_irq_vectors_affinity() __pci_enable_msix_range() pci_setup_msi_context() pcim_setup_msi_release() This driver enables device resources management with initial call to pcim_enable_device(). Are there any conditions / testing sequence the release function is not called? Thanks, - Tomasz ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] iommu/riscv: Free irq vectors on pci remove @ 2024-11-06 19:00 ` Tomasz Jeznach 0 siblings, 0 replies; 14+ messages in thread From: Tomasz Jeznach @ 2024-11-06 19:00 UTC (permalink / raw) To: Andrew Jones Cc: iommu, linux-riscv, linux-kernel, joro, will, robin.murphy, paul.walmsley, palmer, aou On Wed, Nov 6, 2024 at 9:51 AM Andrew Jones <ajones@ventanamicro.com> wrote: > > riscv_iommu_pci_probe() calls pci_alloc_irq_vectors() which > states pci_free_irq_vectors() must be called on cleanup. > > Fixes: 68682e9578fb ("iommu/riscv: Add RISC-V IOMMU PCIe device driver") > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > --- > drivers/iommu/riscv/iommu-pci.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iommu/riscv/iommu-pci.c b/drivers/iommu/riscv/iommu-pci.c > index c7a89143014c..25a27e627a0e 100644 > --- a/drivers/iommu/riscv/iommu-pci.c > +++ b/drivers/iommu/riscv/iommu-pci.c > @@ -99,6 +99,7 @@ static void riscv_iommu_pci_remove(struct pci_dev *pdev) > struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev); > > riscv_iommu_remove(iommu); > + pci_free_irq_vectors(pdev); > } > > static const struct pci_device_id riscv_iommu_pci_tbl[] = { > -- > 2.47.0 > Andrew, interrupt release call pci_free_irq_vectors() should already be called by device managed resources framework, with unwind action callback pcim_msi_release(). Callback is registered during vectors allocation: pci_alloc_irq_vectors_affinity() __pci_enable_msix_range() pci_setup_msi_context() pcim_setup_msi_release() This driver enables device resources management with initial call to pcim_enable_device(). Are there any conditions / testing sequence the release function is not called? Thanks, - Tomasz _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] iommu/riscv: Free irq vectors on pci remove 2024-11-06 19:00 ` Tomasz Jeznach @ 2024-11-07 16:35 ` Andrew Jones -1 siblings, 0 replies; 14+ messages in thread From: Andrew Jones @ 2024-11-07 16:35 UTC (permalink / raw) To: Tomasz Jeznach Cc: iommu, linux-riscv, linux-kernel, joro, will, robin.murphy, paul.walmsley, palmer, aou On Wed, Nov 06, 2024 at 11:00:55AM -0800, Tomasz Jeznach wrote: > On Wed, Nov 6, 2024 at 9:51 AM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > riscv_iommu_pci_probe() calls pci_alloc_irq_vectors() which > > states pci_free_irq_vectors() must be called on cleanup. > > > > Fixes: 68682e9578fb ("iommu/riscv: Add RISC-V IOMMU PCIe device driver") > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > drivers/iommu/riscv/iommu-pci.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/iommu/riscv/iommu-pci.c b/drivers/iommu/riscv/iommu-pci.c > > index c7a89143014c..25a27e627a0e 100644 > > --- a/drivers/iommu/riscv/iommu-pci.c > > +++ b/drivers/iommu/riscv/iommu-pci.c > > @@ -99,6 +99,7 @@ static void riscv_iommu_pci_remove(struct pci_dev *pdev) > > struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev); > > > > riscv_iommu_remove(iommu); > > + pci_free_irq_vectors(pdev); > > } > > > > static const struct pci_device_id riscv_iommu_pci_tbl[] = { > > -- > > 2.47.0 > > > > Andrew, interrupt release call pci_free_irq_vectors() should already > be called by device > managed resources framework, with unwind action callback pcim_msi_release(). > > Callback is registered during vectors allocation: > pci_alloc_irq_vectors_affinity() > __pci_enable_msix_range() > pci_setup_msi_context() > pcim_setup_msi_release() > > This driver enables device resources management with initial call to > pcim_enable_device(). Ah, thank you for the education and inspiration to look closer at how this works. It was easy to confirm with a WARN in pci_free_irq_vectors() and an echo 1 to the IOMMU device's remove sysfs node. > > Are there any conditions / testing sequence the release function is not called? No, not that I know of, this was just my knee jerk decision to mimic the call pairing I was doing on the platform side to the pci side. Sorry for the noise, this patch can be dropped. Thanks, drew ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] iommu/riscv: Free irq vectors on pci remove @ 2024-11-07 16:35 ` Andrew Jones 0 siblings, 0 replies; 14+ messages in thread From: Andrew Jones @ 2024-11-07 16:35 UTC (permalink / raw) To: Tomasz Jeznach Cc: iommu, linux-riscv, linux-kernel, joro, will, robin.murphy, paul.walmsley, palmer, aou On Wed, Nov 06, 2024 at 11:00:55AM -0800, Tomasz Jeznach wrote: > On Wed, Nov 6, 2024 at 9:51 AM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > riscv_iommu_pci_probe() calls pci_alloc_irq_vectors() which > > states pci_free_irq_vectors() must be called on cleanup. > > > > Fixes: 68682e9578fb ("iommu/riscv: Add RISC-V IOMMU PCIe device driver") > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > drivers/iommu/riscv/iommu-pci.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/iommu/riscv/iommu-pci.c b/drivers/iommu/riscv/iommu-pci.c > > index c7a89143014c..25a27e627a0e 100644 > > --- a/drivers/iommu/riscv/iommu-pci.c > > +++ b/drivers/iommu/riscv/iommu-pci.c > > @@ -99,6 +99,7 @@ static void riscv_iommu_pci_remove(struct pci_dev *pdev) > > struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev); > > > > riscv_iommu_remove(iommu); > > + pci_free_irq_vectors(pdev); > > } > > > > static const struct pci_device_id riscv_iommu_pci_tbl[] = { > > -- > > 2.47.0 > > > > Andrew, interrupt release call pci_free_irq_vectors() should already > be called by device > managed resources framework, with unwind action callback pcim_msi_release(). > > Callback is registered during vectors allocation: > pci_alloc_irq_vectors_affinity() > __pci_enable_msix_range() > pci_setup_msi_context() > pcim_setup_msi_release() > > This driver enables device resources management with initial call to > pcim_enable_device(). Ah, thank you for the education and inspiration to look closer at how this works. It was easy to confirm with a WARN in pci_free_irq_vectors() and an echo 1 to the IOMMU device's remove sysfs node. > > Are there any conditions / testing sequence the release function is not called? No, not that I know of, this was just my knee jerk decision to mimic the call pairing I was doing on the platform side to the pci side. Sorry for the noise, this patch can be dropped. Thanks, drew _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] iommu/riscv: Add support for platform msi 2024-11-06 17:51 ` Andrew Jones @ 2024-11-06 17:51 ` Andrew Jones -1 siblings, 0 replies; 14+ messages in thread From: Andrew Jones @ 2024-11-06 17:51 UTC (permalink / raw) To: iommu, linux-riscv, linux-kernel Cc: tjeznach, joro, will, robin.murphy, paul.walmsley, palmer, aou 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] iommu/riscv: Add support for platform msi @ 2024-11-06 17:51 ` Andrew Jones 0 siblings, 0 replies; 14+ messages in thread From: Andrew Jones @ 2024-11-06 17:51 UTC (permalink / raw) To: iommu, linux-riscv, linux-kernel Cc: tjeznach, joro, will, robin.murphy, paul.walmsley, palmer, aou 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 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] iommu/riscv: Add support for platform msi 2024-11-06 17:51 ` Andrew Jones @ 2024-11-06 19:45 ` Samuel Holland -1 siblings, 0 replies; 14+ messages in thread From: Samuel Holland @ 2024-11-06 19:45 UTC (permalink / raw) To: Andrew Jones, iommu, linux-riscv, linux-kernel Cc: tjeznach, joro, will, robin.murphy, paul.walmsley, palmer, aou 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[] = { ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] iommu/riscv: Add support for platform msi @ 2024-11-06 19:45 ` Samuel Holland 0 siblings, 0 replies; 14+ messages in thread From: Samuel Holland @ 2024-11-06 19:45 UTC (permalink / raw) To: Andrew Jones, iommu, linux-riscv, linux-kernel Cc: tjeznach, joro, will, robin.murphy, paul.walmsley, palmer, aou 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[] = { _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] iommu/riscv: Add support for platform msi 2024-11-06 19:45 ` Samuel Holland @ 2024-11-07 16:41 ` Andrew Jones -1 siblings, 0 replies; 14+ messages in thread From: Andrew Jones @ 2024-11-07 16:41 UTC (permalink / raw) To: Samuel Holland Cc: iommu, linux-riscv, linux-kernel, tjeznach, joro, will, robin.murphy, paul.walmsley, palmer, aou 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[] = { > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] iommu/riscv: Add support for platform msi @ 2024-11-07 16:41 ` Andrew Jones 0 siblings, 0 replies; 14+ messages in thread From: Andrew Jones @ 2024-11-07 16:41 UTC (permalink / raw) To: Samuel Holland Cc: iommu, linux-riscv, linux-kernel, tjeznach, joro, will, robin.murphy, paul.walmsley, palmer, aou 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[] = { > _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-11-07 18:01 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-11-06 17:51 [PATCH 0/2] iommu/riscv: Add platform msi support Andrew Jones 2024-11-06 17:51 ` Andrew Jones 2024-11-06 17:51 ` [PATCH 1/2] iommu/riscv: Free irq vectors on pci remove Andrew Jones 2024-11-06 17:51 ` Andrew Jones 2024-11-06 19:00 ` Tomasz Jeznach 2024-11-06 19:00 ` Tomasz Jeznach 2024-11-07 16:35 ` Andrew Jones 2024-11-07 16:35 ` Andrew Jones 2024-11-06 17:51 ` [PATCH 2/2] iommu/riscv: Add support for platform msi Andrew Jones 2024-11-06 17:51 ` Andrew Jones 2024-11-06 19:45 ` Samuel Holland 2024-11-06 19:45 ` Samuel Holland 2024-11-07 16:41 ` Andrew Jones 2024-11-07 16:41 ` Andrew Jones
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.