diff mbox

[RFC,7/7] irqchip: [Example] dummy wired interrupt/MSI bridge driver

Message ID 1444923568-17413-8-git-send-email-marc.zyngier@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Marc Zyngier Oct. 15, 2015, 3:39 p.m. UTC
In order to demonstrate how to put together a wire/MSI bridge,
add a dummy driver that doesn't do anything at all, except
for allocating interrupts.

It comes together with an even more stupid client driver that
allocates an interrupt and dump the hierarchy of that interrupt.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/Kconfig       |   7 ++
 drivers/irqchip/Makefile      |   1 +
 drivers/irqchip/irq-msichip.c | 271 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 279 insertions(+)
 create mode 100644 drivers/irqchip/irq-msichip.c

Comments

majun (F) Nov. 4, 2015, 8 a.m. UTC | #1
Hi Marc:

? 2015/10/15 23:39, Marc Zyngier ??:
> In order to demonstrate how to put together a wire/MSI bridge,
> add a dummy driver that doesn't do anything at all, except
> for allocating interrupts.
> 
> It comes together with an even more stupid client driver that
> allocates an interrupt and dump the hierarchy of that interrupt.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
[...]
> +
> +static int msichip_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				   unsigned int nr_irqs, void *arg)
> +{
> +	int i, err;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +	struct irq_fwspec *fwspec = arg;
> +	void *data;
> +
> +	err = msichip_domain_translate(domain, fwspec, &hwirq, &type);
> +	if (err)
> +		return err;
> +

.translate function already called once in  irq_domain_translate(),
I think we don't need call this fucntion one more time here.

> +	err = platform_msi_domain_alloc(domain, virq, nr_irqs);
> +	if (err)
> +		return err;
> +
> +	data = platform_msi_get_host_data(domain);
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +					      &msichip_chip, data);
> +
> +	return 0;
> +}
> +
[...]
> +
> +static struct platform_driver msichip_driver = {
> +	.driver = {
> +		.name		= "msichip",
> +		.of_match_table	= msichip_of_match,
> +	},
> +	.probe			= msichip_probe,
> +};
> +/* Do not define this as an irqchip */
> +module_platform_driver(msichip_driver);
> +
> +

I think,for a interrupt controller, msichip driver initialization maybe is too late
for some devices which connect to this irqchip if we use module_platform_driver.

So, how about use the arch_initcall to register the msichip driver?

Thanks!
Ma Jun
Marc Zyngier Nov. 4, 2015, 9:03 a.m. UTC | #2
On 04/11/15 08:00, majun (F) wrote:
> Hi Marc:
> 
> ? 2015/10/15 23:39, Marc Zyngier ??:
>> In order to demonstrate how to put together a wire/MSI bridge,
>> add a dummy driver that doesn't do anything at all, except
>> for allocating interrupts.
>>
>> It comes together with an even more stupid client driver that
>> allocates an interrupt and dump the hierarchy of that interrupt.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
> [...]
>> +
>> +static int msichip_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> +				   unsigned int nr_irqs, void *arg)
>> +{
>> +	int i, err;
>> +	irq_hw_number_t hwirq;
>> +	unsigned int type;
>> +	struct irq_fwspec *fwspec = arg;
>> +	void *data;
>> +
>> +	err = msichip_domain_translate(domain, fwspec, &hwirq, &type);
>> +	if (err)
>> +		return err;
>> +
> 
> .translate function already called once in  irq_domain_translate(),
> I think we don't need call this fucntion one more time here.

if you don't translate it here, how do you obtain the hwirq that you
have to pass to irq_domain_set_hwirq_and_chip just below?

>> +	err = platform_msi_domain_alloc(domain, virq, nr_irqs);
>> +	if (err)
>> +		return err;
>> +
>> +	data = platform_msi_get_host_data(domain);
>> +	for (i = 0; i < nr_irqs; i++)
>> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>> +					      &msichip_chip, data);
>> +
>> +	return 0;
>> +}
>> +
> [...]
>> +
>> +static struct platform_driver msichip_driver = {
>> +	.driver = {
>> +		.name		= "msichip",
>> +		.of_match_table	= msichip_of_match,
>> +	},
>> +	.probe			= msichip_probe,
>> +};
>> +/* Do not define this as an irqchip */
>> +module_platform_driver(msichip_driver);
>> +
>> +
> 
> I think,for a interrupt controller, msichip driver initialization maybe is too late
> for some devices which connect to this irqchip if we use module_platform_driver.

That's a consequence of this design. This is why I insisted on the fact
that this is currently avoided by using deferred probe in drivers, and
that it should be solved by having a probe order. Either way, this is
not something that we can solve at that level (see the multiple proposal
for this on the various lists).

> So, how about use the arch_initcall to register the msichip driver?

You're only pushing the problem one level up. And you'll realize that
this is not enough for some random driver. This is not sustainable, and
must be addressed properly.

Thanks,

	M.
Gabriele Paoloni Nov. 5, 2015, 8:25 a.m. UTC | #3
Hi Marc

> -----Original Message-----

> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-owner@vger.kernel.org]

> On Behalf Of Marc Zyngier

> Sent: 04 November 2015 09:04

> To: majun (F); Thomas Gleixner; Jiang Liu; Jason Cooper

> Cc: linux-arm-kernel@lists.infradead.org; linux-pci@vger.kernel.org; linux-

> kernel@vger.kernel.org

> Subject: Re: [PATCH RFC 7/7] irqchip: [Example] dummy wired interrupt/MSI

> bridge driver

> 

> On 04/11/15 08:00, majun (F) wrote:

> > Hi Marc:

> >

> > ? 2015/10/15 23:39, Marc Zyngier ??:

> >> In order to demonstrate how to put together a wire/MSI bridge,

> >> add a dummy driver that doesn't do anything at all, except

> >> for allocating interrupts.

> >>

> >> It comes together with an even more stupid client driver that

> >> allocates an interrupt and dump the hierarchy of that interrupt.

> >>

> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

> >> ---

> > [...]

> >> +

> >> +static int msichip_domain_alloc(struct irq_domain *domain, unsigned int

> virq,

> >> +				   unsigned int nr_irqs, void *arg)

> >> +{

> >> +	int i, err;

> >> +	irq_hw_number_t hwirq;

> >> +	unsigned int type;

> >> +	struct irq_fwspec *fwspec = arg;

> >> +	void *data;

> >> +

> >> +	err = msichip_domain_translate(domain, fwspec, &hwirq, &type);

> >> +	if (err)

> >> +		return err;

> >> +

> >

> > .translate function already called once in  irq_domain_translate(),

> > I think we don't need call this fucntion one more time here.

> 

> if you don't translate it here, how do you obtain the hwirq that you

> have to pass to irq_domain_set_hwirq_and_chip just below?

> 

> >> +	err = platform_msi_domain_alloc(domain, virq, nr_irqs);

> >> +	if (err)

> >> +		return err;

> >> +

> >> +	data = platform_msi_get_host_data(domain);

> >> +	for (i = 0; i < nr_irqs; i++)

> >> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,

> >> +					      &msichip_chip, data);

> >> +

> >> +	return 0;

> >> +}

> >> +

> > [...]

> >> +

> >> +static struct platform_driver msichip_driver = {

> >> +	.driver = {

> >> +		.name		= "msichip",

> >> +		.of_match_table	= msichip_of_match,

> >> +	},

> >> +	.probe			= msichip_probe,

> >> +};

> >> +/* Do not define this as an irqchip */

> >> +module_platform_driver(msichip_driver);

> >> +

> >> +

> >

> > I think,for a interrupt controller, msichip driver initialization maybe is

> too late

> > for some devices which connect to this irqchip if we use

> module_platform_driver.

> 

> That's a consequence of this design. This is why I insisted on the fact

> that this is currently avoided by using deferred probe in drivers, and


Mmm using te deferred probe would mean to rework all the drivers of the
potential devices connected to mbi-gen...would that be sustainable/acceptable?

> that it should be solved by having a probe order. Either way, this is

> not something that we can solve at that level (see the multiple proposal

> for this on the various lists).


Could you point me to the relevant discussions for this...?

Thanks

Gab

> 

> > So, how about use the arch_initcall to register the msichip driver?

> 

> You're only pushing the problem one level up. And you'll realize that

> this is not enough for some random driver. This is not sustainable, and

> must be addressed properly.

> 

> Thanks,

> 

> 	M.

> --

> Jazz is not dead. It just smells funny...

> --

> To unsubscribe from this list: send the line "unsubscribe linux-pci" in

> the body of a message to majordomo@vger.kernel.org

> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier Nov. 5, 2015, 9:35 a.m. UTC | #4
On 05/11/15 08:25, Gabriele Paoloni wrote:
> Hi Marc
> 
>> -----Original Message-----
>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-owner@vger.kernel.org]
>> On Behalf Of Marc Zyngier
>> Sent: 04 November 2015 09:04
>> To: majun (F); Thomas Gleixner; Jiang Liu; Jason Cooper
>> Cc: linux-arm-kernel@lists.infradead.org; linux-pci@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> Subject: Re: [PATCH RFC 7/7] irqchip: [Example] dummy wired interrupt/MSI
>> bridge driver
>>
>> On 04/11/15 08:00, majun (F) wrote:

[...]

>>> I think,for a interrupt controller, msichip driver initialization maybe is
>> too late
>>> for some devices which connect to this irqchip if we use
>> module_platform_driver.
>>
>> That's a consequence of this design. This is why I insisted on the fact
>> that this is currently avoided by using deferred probe in drivers, and
> 
> Mmm using te deferred probe would mean to rework all the drivers of the
> potential devices connected to mbi-gen...would that be sustainable/acceptable?

I'm tempted to reply "Not my problem". Or rather, not a problem I'm
trying to solve right now (or any time soon).

I'm pretty sure that sprinkling -EPROBE_DEFER on all possible drivers
will result in a resounding NAK, which is is why I suggested that
someone with a vested interest dedicates some quality time helping those
who are trying to solve this issue for good.

>> that it should be solved by having a probe order. Either way, this is
>> not something that we can solve at that level (see the multiple proposal
>> for this on the various lists).
> 
> Could you point me to the relevant discussions for this...?

Google is, as always, your dearest friend. But here you go:

- LWN has some quality coverage of the KS discussions (assuming you're a
subscriber, otherwise you'll have to wait for another week):
http://lwn.net/Articles/662820/

- There is also Tomeu Vizoso's series, which itself builds upon other
previous attempts at solving this: https://lwn.net/Articles/658690/

Thanks,

	M.
Gabriele Paoloni Nov. 5, 2015, 9:43 a.m. UTC | #5
> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Thursday, November 05, 2015 9:36 AM
> To: Gabriele Paoloni; majun (F); Thomas Gleixner; Jiang Liu; Jason
> Cooper
> Cc: linux-arm-kernel@lists.infradead.org; linux-pci@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH RFC 7/7] irqchip: [Example] dummy wired
> interrupt/MSI bridge driver
> 
> On 05/11/15 08:25, Gabriele Paoloni wrote:
> > Hi Marc
> >
> >> -----Original Message-----
> >> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> owner@vger.kernel.org]
> >> On Behalf Of Marc Zyngier
> >> Sent: 04 November 2015 09:04
> >> To: majun (F); Thomas Gleixner; Jiang Liu; Jason Cooper
> >> Cc: linux-arm-kernel@lists.infradead.org; linux-pci@vger.kernel.org;
> linux-
> >> kernel@vger.kernel.org
> >> Subject: Re: [PATCH RFC 7/7] irqchip: [Example] dummy wired
> interrupt/MSI
> >> bridge driver
> >>
> >> On 04/11/15 08:00, majun (F) wrote:
> 
> [...]
> 
> >>> I think,for a interrupt controller, msichip driver initialization
> maybe is
> >> too late
> >>> for some devices which connect to this irqchip if we use
> >> module_platform_driver.
> >>
> >> That's a consequence of this design. This is why I insisted on the
> fact
> >> that this is currently avoided by using deferred probe in drivers,
> and
> >
> > Mmm using te deferred probe would mean to rework all the drivers of
> the
> > potential devices connected to mbi-gen...would that be
> sustainable/acceptable?
> 
> I'm tempted to reply "Not my problem". Or rather, not a problem I'm
> trying to solve right now (or any time soon).
> 
> I'm pretty sure that sprinkling -EPROBE_DEFER on all possible drivers
> will result in a resounding NAK, which is is why I suggested that
> someone with a vested interest dedicates some quality time helping
> those
> who are trying to solve this issue for good.

Yes you're right, makes perfect sense

> 
> >> that it should be solved by having a probe order. Either way, this
> is
> >> not something that we can solve at that level (see the multiple
> proposal
> >> for this on the various lists).
> >
> > Could you point me to the relevant discussions for this...?
> 
> Google is, as always, your dearest friend. But here you go:
> 
> - LWN has some quality coverage of the KS discussions (assuming you're
> a
> subscriber, otherwise you'll have to wait for another week):
> http://lwn.net/Articles/662820/
> 
> - There is also Tomeu Vizoso's series, which itself builds upon other
> previous attempts at solving this: https://lwn.net/Articles/658690/
> 

Great, many thanks for pointing them out.

I'll look into these.

Thanks again

Gab

> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...
diff mbox

Patch

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 4d7294e..73e38e6 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -193,3 +193,10 @@  config IRQ_MXS
 	def_bool y if MACH_ASM9260 || ARCH_MXS
 	select IRQ_DOMAIN
 	select STMP_DEVICE
+
+config DUMMY_MSICHIP
+	bool "Dummy irqchip on MSI"
+	depends on GENERIC_MSI_IRQ_DOMAIN
+	help
+	  Dummy stacked irqchip on top of platform MSI
+
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 177f78f..682e7dc 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -55,3 +55,4 @@  obj-$(CONFIG_RENESAS_H8S_INTC)		+= irq-renesas-h8s.o
 obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
 obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
 obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
+obj-$(CONFIG_DUMMY_MSICHIP)		+= irq-msichip.o
diff --git a/drivers/irqchip/irq-msichip.c b/drivers/irqchip/irq-msichip.c
new file mode 100644
index 0000000..82b283b
--- /dev/null
+++ b/drivers/irqchip/irq-msichip.c
@@ -0,0 +1,271 @@ 
+/*
+ * Non-functionnal example for a wired irq <-> MSI bridge
+ *
+ * Copyright (C) 2015 ARM Limited, All Rights Reserved.
+ * Author: Marc Zyngier <marc.zyngier@arm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * DT fragment to represent the MSI bridge:
+
+	intc: msichip {
+		compatible = "dummy,msichip";
+		num-msis = 32;
+		interrupt-controller;
+		interrupt-parent = <&gic>;
+		#interrupt-cells = <0x2>;
+		msi-parent = <&its 1234>;
+	};
+
+ * DT fragment to represent the device connected to the bridge:
+
+	dummy-dev {
+		compatible = "dummy,device";
+		interrupt-parent = <intc>;
+		interrupts = <0x5 0x1>;
+	};
+
+ * When "dummy,device" gets probed, it dumps the hierarchy for the
+ * interrupt it has allocated:
+
+	dummydev dummy-dev: Allocated IRQ35
+	dummydev dummy-dev: Probing OK
+	dummydev dummy-dev: IRQ35 hwirq 5 domain msichip_domain_ops
+	dummydev dummy-dev: IRQ35 hwirq 0 domain msi_domain_ops
+	dummydev dummy-dev: IRQ35 hwirq 8192 domain its_domain_ops
+	dummydev dummy-dev: IRQ35 hwirq 8192 domain gic_irq_domain_ops
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+static void msichip_mask(struct irq_data *data)
+{
+	/* Do something */
+}
+
+static void msichip_unmask(struct irq_data *data)
+{
+	/* Do something */
+}
+
+static void msichip_eoi(struct irq_data *data)
+{
+	/* Do something */
+}
+
+static int msichip_set_type(struct irq_data *data, unsigned int type)
+{
+	/* Do something */
+	return 0;
+}
+
+static int msichip_retrigger(struct irq_data *data)
+{
+	/* Do something */
+	return 0;
+}
+
+static int msichip_set_affinity(struct irq_data *data,
+				const struct cpumask *dest, bool force)
+{
+	/* Do something */
+	return 0;
+}
+
+static struct irq_chip msichip_chip = {
+	.name			= "MSICHIP",
+	.irq_mask		= msichip_mask,
+	.irq_unmask		= msichip_unmask,
+	.irq_eoi		= msichip_eoi,
+	.irq_set_type		= msichip_set_type,
+	.irq_retrigger		= msichip_retrigger,
+	.irq_set_affinity	= msichip_set_affinity,
+};
+
+static int msichip_domain_translate(struct irq_domain *d,
+				    struct irq_fwspec *fwspec,
+				    unsigned long *hwirq,
+				    unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 2)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int msichip_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				   unsigned int nr_irqs, void *arg)
+{
+	int i, err;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	struct irq_fwspec *fwspec = arg;
+	void *data;
+
+	err = msichip_domain_translate(domain, fwspec, &hwirq, &type);
+	if (err)
+		return err;
+
+	err = platform_msi_domain_alloc(domain, virq, nr_irqs);
+	if (err)
+		return err;
+
+	data = platform_msi_get_host_data(domain);
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+					      &msichip_chip, data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops msichip_domain_ops = {
+	.translate	= msichip_domain_translate,
+	.alloc		= msichip_domain_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
+struct msichip_data {
+	/* Add whatever you fancy here */
+	struct platform_device *pdev;
+};
+
+static void msichip_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
+{
+	/* Do the right thing */
+}
+
+static int msichip_probe(struct platform_device *pdev)
+{
+	struct irq_domain *domain;
+	struct msichip_data *msichip_data;
+	u32 num_msis;
+
+	dev_info(&pdev->dev, "Probing\n");
+	msichip_data = kzalloc(sizeof(*msichip_data), GFP_KERNEL);
+	if (!msichip_data)
+		return -ENOMEM;
+
+	msichip_data->pdev = pdev;
+
+	/* If there is no "num-msi" property, assume 64... */
+	if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0)
+		num_msis = 64;
+
+	dev_info(&pdev->dev, "allocating %d MSIs\n", num_msis);
+
+	domain = platform_msi_create_device_domain(&pdev->dev, num_msis,
+						   msichip_write_msi_msg,
+						   &msichip_domain_ops,
+						   msichip_data);
+
+	if (!domain){
+		kfree(msichip_data);
+		return -ENOMEM;
+	}
+
+	dev_info(&pdev->dev, "Probing OK\n");
+	return 0;
+}
+
+static const struct of_device_id msichip_of_match[] = {
+	{ .compatible = "dummy,msichip", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, msichip_of_match);
+
+static struct platform_driver msichip_driver = {
+	.driver = {
+		.name		= "msichip",
+		.of_match_table	= msichip_of_match,
+	},
+	.probe			= msichip_probe,
+};
+/* Do not define this as an irqchip */
+module_platform_driver(msichip_driver);
+
+
+
+/* Driver for a dummy device connected to the MSI bridge */
+static irqreturn_t dummydev_handler(int irq, void *dummy)
+{
+	return IRQ_HANDLED;
+}
+
+static void dummydev_dump_hierarchy(struct device *dev, int irq)
+{
+	struct irq_data *data = irq_get_irq_data(irq);
+
+	while(data) {
+		dev_info(dev, "IRQ%d hwirq %d domain %ps\n",
+			 data->irq, (int)data->hwirq, data->domain->ops);
+		data = data->parent_data;
+	}
+}
+
+static int dummydev_probe(struct platform_device *pdev)
+{
+	int irq;
+
+	dev_info(&pdev->dev, "Probing\n");
+	irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	if (!irq) {
+		dev_err(&pdev->dev, "irq allocation failed, defering\n");
+		return -EPROBE_DEFER;
+	}
+
+	dev_info(&pdev->dev, "Allocated IRQ%d\n", irq);
+
+	if (request_irq(irq, dummydev_handler, 0, "dummydev", pdev))
+		return -EINVAL;
+
+	dev_info(&pdev->dev, "Probing OK\n");
+
+	dummydev_dump_hierarchy(&pdev->dev, irq);
+	return 0;
+}
+
+static const struct of_device_id dummydev_of_match[] = {
+	{ .compatible = "dummy,device", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, dummydev_of_match);
+
+static struct platform_driver dummydev_driver = {
+	.driver = {
+		.name		= "dummydev",
+		.of_match_table	= dummydev_of_match,
+	},
+	.probe			= dummydev_probe,
+};
+
+module_platform_driver(dummydev_driver);