diff mbox series

[v11,09/14] irqchip/riscv-imsic: Add support for PCI MSI irqdomain

Message ID 20231023172800.315343-10-apatel@ventanamicro.com
State New
Headers
Series Linux RISC-V AIA Support |

Commit Message

Anup Patel Oct. 23, 2023, 5:27 p.m. UTC 
  The Linux PCI framework requires it's own dedicated MSI irqdomain so
let us create PCI MSI irqdomain as child of the IMSIC base irqdomain.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 drivers/irqchip/Kconfig                    |  7 +++
 drivers/irqchip/irq-riscv-imsic-platform.c | 51 ++++++++++++++++++++++
 drivers/irqchip/irq-riscv-imsic-state.h    |  1 +
 3 files changed, 59 insertions(+)

Comments

Björn Töpel Oct. 24, 2023, 1:09 p.m. UTC | #1 
Anup Patel <apatel@ventanamicro.com> writes:

> The Linux PCI framework requires it's own dedicated MSI irqdomain so
> let us create PCI MSI irqdomain as child of the IMSIC base irqdomain.
>
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  drivers/irqchip/Kconfig                    |  7 +++
>  drivers/irqchip/irq-riscv-imsic-platform.c | 51 ++++++++++++++++++++++
>  drivers/irqchip/irq-riscv-imsic-state.h    |  1 +
>  3 files changed, 59 insertions(+)
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index bdd80716114d..c1d69b418dfb 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -552,6 +552,13 @@ config RISCV_IMSIC
>  	select IRQ_DOMAIN_HIERARCHY
>  	select GENERIC_MSI_IRQ
>  
> +config RISCV_IMSIC_PCI
> +	bool
> +	depends on RISCV_IMSIC
> +	depends on PCI
> +	depends on PCI_MSI
> +	default RISCV_IMSIC
> +
>  config EXYNOS_IRQ_COMBINER
>  	bool "Samsung Exynos IRQ combiner support" if COMPILE_TEST
>  	depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST
> diff --git a/drivers/irqchip/irq-riscv-imsic-platform.c b/drivers/irqchip/irq-riscv-imsic-platform.c
> index 23d286cb017e..cdb659401199 100644
> --- a/drivers/irqchip/irq-riscv-imsic-platform.c
> +++ b/drivers/irqchip/irq-riscv-imsic-platform.c
> @@ -13,6 +13,7 @@
>  #include <linux/irqdomain.h>
>  #include <linux/module.h>
>  #include <linux/msi.h>
> +#include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <linux/spinlock.h>
>  #include <linux/smp.h>
> @@ -215,6 +216,42 @@ static const struct irq_domain_ops imsic_base_domain_ops = {
>  #endif
>  };
>  
> +#ifdef CONFIG_RISCV_IMSIC_PCI
> +
> +static void imsic_pci_mask_irq(struct irq_data *d)
> +{
> +	pci_msi_mask_irq(d);
> +	irq_chip_mask_parent(d);

I've asked this before, but I still don't get why you need to propagate
to the parent? Why isn't masking on PCI enough?


Björn
Anup Patel Oct. 25, 2023, 5:08 a.m. UTC | #2 
On Tue, Oct 24, 2023 at 6:39 PM Björn Töpel <bjorn@kernel.org> wrote:
>
> Anup Patel <apatel@ventanamicro.com> writes:
>
> > The Linux PCI framework requires it's own dedicated MSI irqdomain so
> > let us create PCI MSI irqdomain as child of the IMSIC base irqdomain.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  drivers/irqchip/Kconfig                    |  7 +++
> >  drivers/irqchip/irq-riscv-imsic-platform.c | 51 ++++++++++++++++++++++
> >  drivers/irqchip/irq-riscv-imsic-state.h    |  1 +
> >  3 files changed, 59 insertions(+)
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index bdd80716114d..c1d69b418dfb 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -552,6 +552,13 @@ config RISCV_IMSIC
> >       select IRQ_DOMAIN_HIERARCHY
> >       select GENERIC_MSI_IRQ
> >
> > +config RISCV_IMSIC_PCI
> > +     bool
> > +     depends on RISCV_IMSIC
> > +     depends on PCI
> > +     depends on PCI_MSI
> > +     default RISCV_IMSIC
> > +
> >  config EXYNOS_IRQ_COMBINER
> >       bool "Samsung Exynos IRQ combiner support" if COMPILE_TEST
> >       depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST
> > diff --git a/drivers/irqchip/irq-riscv-imsic-platform.c b/drivers/irqchip/irq-riscv-imsic-platform.c
> > index 23d286cb017e..cdb659401199 100644
> > --- a/drivers/irqchip/irq-riscv-imsic-platform.c
> > +++ b/drivers/irqchip/irq-riscv-imsic-platform.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/irqdomain.h>
> >  #include <linux/module.h>
> >  #include <linux/msi.h>
> > +#include <linux/pci.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/smp.h>
> > @@ -215,6 +216,42 @@ static const struct irq_domain_ops imsic_base_domain_ops = {
> >  #endif
> >  };
> >
> > +#ifdef CONFIG_RISCV_IMSIC_PCI
> > +
> > +static void imsic_pci_mask_irq(struct irq_data *d)
> > +{
> > +     pci_msi_mask_irq(d);
> > +     irq_chip_mask_parent(d);
>
> I've asked this before, but I still don't get why you need to propagate
> to the parent? Why isn't masking on PCI enough?
>

We are using hierarchical IRQ domains where IMSIC-BASE is
the root domain whereas IMSIC-PLAT domain (MSI irq domain
for platform devices) and IMSIC-PCI domain (MSI irq domain
for PCI devices). For hierarchical IRQ domains, if irq domain X
does not implement irq_mask/unmask then the parent irq
domain irq_mask/unmask is called with parent irq descriptor.

Now for IMSIC-PCI domain, the PCI framework expects the
pci_msi_mask/unmask_irq() functions to be called but if
we directly point pci_msi_mask/unmask_irq() in the IMSIC-PCI
irqchip then IMSIC-BASE (parent domain) irq_mask/umask
won't be called hence the IRQ won't be masked/unmask.
Due to this, we call both pci_msi_mask/unmask_irq() and
irq_chip_mask/unmask_parent() for IMSIC-PCI domain.

The ARM GIC driver also uses hierarchical IRQ domains
even there same thing is done.
(Refer, first 30 lines of drivers/irqchip/irq-gic-v3-its-pci-msi.c)

Regards,
Anup
Björn Töpel Oct. 25, 2023, 8:55 a.m. UTC | #3 
Anup Patel <apatel@ventanamicro.com> writes:

> On Tue, Oct 24, 2023 at 6:39 PM Björn Töpel <bjorn@kernel.org> wrote:
>>
>> Anup Patel <apatel@ventanamicro.com> writes:
>>
>> > The Linux PCI framework requires it's own dedicated MSI irqdomain so
>> > let us create PCI MSI irqdomain as child of the IMSIC base irqdomain.
>> >
>> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>> > ---
>> >  drivers/irqchip/Kconfig                    |  7 +++
>> >  drivers/irqchip/irq-riscv-imsic-platform.c | 51 ++++++++++++++++++++++
>> >  drivers/irqchip/irq-riscv-imsic-state.h    |  1 +
>> >  3 files changed, 59 insertions(+)
>> >
>> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> > index bdd80716114d..c1d69b418dfb 100644
>> > --- a/drivers/irqchip/Kconfig
>> > +++ b/drivers/irqchip/Kconfig
>> > @@ -552,6 +552,13 @@ config RISCV_IMSIC
>> >       select IRQ_DOMAIN_HIERARCHY
>> >       select GENERIC_MSI_IRQ
>> >
>> > +config RISCV_IMSIC_PCI
>> > +     bool
>> > +     depends on RISCV_IMSIC
>> > +     depends on PCI
>> > +     depends on PCI_MSI
>> > +     default RISCV_IMSIC
>> > +
>> >  config EXYNOS_IRQ_COMBINER
>> >       bool "Samsung Exynos IRQ combiner support" if COMPILE_TEST
>> >       depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST
>> > diff --git a/drivers/irqchip/irq-riscv-imsic-platform.c b/drivers/irqchip/irq-riscv-imsic-platform.c
>> > index 23d286cb017e..cdb659401199 100644
>> > --- a/drivers/irqchip/irq-riscv-imsic-platform.c
>> > +++ b/drivers/irqchip/irq-riscv-imsic-platform.c
>> > @@ -13,6 +13,7 @@
>> >  #include <linux/irqdomain.h>
>> >  #include <linux/module.h>
>> >  #include <linux/msi.h>
>> > +#include <linux/pci.h>
>> >  #include <linux/platform_device.h>
>> >  #include <linux/spinlock.h>
>> >  #include <linux/smp.h>
>> > @@ -215,6 +216,42 @@ static const struct irq_domain_ops imsic_base_domain_ops = {
>> >  #endif
>> >  };
>> >
>> > +#ifdef CONFIG_RISCV_IMSIC_PCI
>> > +
>> > +static void imsic_pci_mask_irq(struct irq_data *d)
>> > +{
>> > +     pci_msi_mask_irq(d);
>> > +     irq_chip_mask_parent(d);
>>
>> I've asked this before, but I still don't get why you need to propagate
>> to the parent? Why isn't masking on PCI enough?
>>
>
> We are using hierarchical IRQ domains where IMSIC-BASE is
> the root domain whereas IMSIC-PLAT domain (MSI irq domain
> for platform devices) and IMSIC-PCI domain (MSI irq domain
> for PCI devices). For hierarchical IRQ domains, if irq domain X
> does not implement irq_mask/unmask then the parent irq
> domain irq_mask/unmask is called with parent irq descriptor.
>
> Now for IMSIC-PCI domain, the PCI framework expects the
> pci_msi_mask/unmask_irq() functions to be called but if
> we directly point pci_msi_mask/unmask_irq() in the IMSIC-PCI
> irqchip then IMSIC-BASE (parent domain) irq_mask/umask
> won't be called hence the IRQ won't be masked/unmask.
> Due to this, we call both pci_msi_mask/unmask_irq() and
> irq_chip_mask/unmask_parent() for IMSIC-PCI domain.

Ok. I wont dig more into it for now! If the interrupt is disabled at
PCI, it seems a bit overkill to *also* mask it at the IMSIC level...


Björn
Thomas Gleixner Oct. 25, 2023, 7:59 p.m. UTC | #4 
On Mon, Oct 23 2023 at 22:57, Anup Patel wrote:
> The Linux PCI framework requires it's own dedicated MSI irqdomain so
> let us create PCI MSI irqdomain as child of the IMSIC base irqdomain.

Same here. Please don't add new incarnations of that and switch over to
per device MSI domains which is the most future proof mechanism.

Thanks,

        tglx
Thomas Gleixner Oct. 28, 2023, 6:36 p.m. UTC | #5 
On Wed, Oct 25 2023 at 10:55, Björn Töpel wrote:
>> Now for IMSIC-PCI domain, the PCI framework expects the
>> pci_msi_mask/unmask_irq() functions to be called but if
>> we directly point pci_msi_mask/unmask_irq() in the IMSIC-PCI
>> irqchip then IMSIC-BASE (parent domain) irq_mask/umask
>> won't be called hence the IRQ won't be masked/unmask.
>> Due to this, we call both pci_msi_mask/unmask_irq() and
>> irq_chip_mask/unmask_parent() for IMSIC-PCI domain.
>
> Ok. I wont dig more into it for now! If the interrupt is disabled at
> PCI, it seems a bit overkill to *also* mask it at the IMSIC level...

Only _if_ the device provides MSI masking, but that extra mask/unmask is
not the end of the world.

Thanks,

        tglx
Björn Töpel Oct. 29, 2023, 7:53 p.m. UTC | #6 
Thomas Gleixner <tglx@linutronix.de> writes:

> On Wed, Oct 25 2023 at 10:55, Björn Töpel wrote:
>>> Now for IMSIC-PCI domain, the PCI framework expects the
>>> pci_msi_mask/unmask_irq() functions to be called but if
>>> we directly point pci_msi_mask/unmask_irq() in the IMSIC-PCI
>>> irqchip then IMSIC-BASE (parent domain) irq_mask/umask
>>> won't be called hence the IRQ won't be masked/unmask.
>>> Due to this, we call both pci_msi_mask/unmask_irq() and
>>> irq_chip_mask/unmask_parent() for IMSIC-PCI domain.
>>
>> Ok. I wont dig more into it for now! If the interrupt is disabled at
>> PCI, it seems a bit overkill to *also* mask it at the IMSIC level...
>
> Only _if_ the device provides MSI masking, but that extra mask/unmask is
> not the end of the world.

Yikes -- so MSI masking is optional. Ick. :-( Thanks for the excellent
MSI vs MSI-X post in the other thread, BTW. Great stuff!
diff mbox series

Patch

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index bdd80716114d..c1d69b418dfb 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -552,6 +552,13 @@  config RISCV_IMSIC
 	select IRQ_DOMAIN_HIERARCHY
 	select GENERIC_MSI_IRQ
 
+config RISCV_IMSIC_PCI
+	bool
+	depends on RISCV_IMSIC
+	depends on PCI
+	depends on PCI_MSI
+	default RISCV_IMSIC
+
 config EXYNOS_IRQ_COMBINER
 	bool "Samsung Exynos IRQ combiner support" if COMPILE_TEST
 	depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST
diff --git a/drivers/irqchip/irq-riscv-imsic-platform.c b/drivers/irqchip/irq-riscv-imsic-platform.c
index 23d286cb017e..cdb659401199 100644
--- a/drivers/irqchip/irq-riscv-imsic-platform.c
+++ b/drivers/irqchip/irq-riscv-imsic-platform.c
@@ -13,6 +13,7 @@ 
 #include <linux/irqdomain.h>
 #include <linux/module.h>
 #include <linux/msi.h>
+#include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
 #include <linux/smp.h>
@@ -215,6 +216,42 @@  static const struct irq_domain_ops imsic_base_domain_ops = {
 #endif
 };
 
+#ifdef CONFIG_RISCV_IMSIC_PCI
+
+static void imsic_pci_mask_irq(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void imsic_pci_unmask_irq(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
+static struct irq_chip imsic_pci_irq_chip = {
+	.name			= "IMSIC-PCI",
+	.irq_mask		= imsic_pci_mask_irq,
+	.irq_unmask		= imsic_pci_unmask_irq,
+#ifdef CONFIG_SMP
+	.irq_set_affinity	= imsic_irq_set_affinity,
+#endif
+	.irq_eoi		= irq_chip_eoi_parent,
+};
+
+static struct msi_domain_ops imsic_pci_domain_ops = {
+};
+
+static struct msi_domain_info imsic_pci_domain_info = {
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+		   MSI_FLAG_PCI_MSIX | MSI_FLAG_MULTI_PCI_MSI),
+	.ops	= &imsic_pci_domain_ops,
+	.chip	= &imsic_pci_irq_chip,
+};
+
+#endif
+
 static struct irq_chip imsic_plat_irq_chip = {
 	.name			= "IMSIC-PLAT",
 #ifdef CONFIG_SMP
@@ -243,6 +280,18 @@  static int imsic_irq_domains_init(struct fwnode_handle *fwnode)
 	}
 	irq_domain_update_bus_token(imsic->base_domain, DOMAIN_BUS_NEXUS);
 
+#ifdef CONFIG_RISCV_IMSIC_PCI
+	/* Create PCI MSI domain */
+	imsic->pci_domain = pci_msi_create_irq_domain(fwnode,
+						&imsic_pci_domain_info,
+						imsic->base_domain);
+	if (!imsic->pci_domain) {
+		pr_err("%pfwP: failed to create IMSIC PCI domain\n", fwnode);
+		irq_domain_remove(imsic->base_domain);
+		return -ENOMEM;
+	}
+#endif
+
 	/* Create Platform MSI domain */
 	imsic->plat_domain = platform_msi_create_irq_domain(fwnode,
 						&imsic_plat_domain_info,
@@ -250,6 +299,8 @@  static int imsic_irq_domains_init(struct fwnode_handle *fwnode)
 	if (!imsic->plat_domain) {
 		pr_err("%pfwP: failed to create IMSIC platform domain\n",
 			fwnode);
+		if (imsic->pci_domain)
+			irq_domain_remove(imsic->pci_domain);
 		irq_domain_remove(imsic->base_domain);
 		return -ENOMEM;
 	}
diff --git a/drivers/irqchip/irq-riscv-imsic-state.h b/drivers/irqchip/irq-riscv-imsic-state.h
index 82911b8b08b4..8d209e77432e 100644
--- a/drivers/irqchip/irq-riscv-imsic-state.h
+++ b/drivers/irqchip/irq-riscv-imsic-state.h
@@ -67,6 +67,7 @@  struct imsic_priv {
 
 	/* IRQ domains (created by platform driver) */
 	struct irq_domain *base_domain;
+	struct irq_domain *pci_domain;
 	struct irq_domain *plat_domain;
 };