Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1652989 > unrolled thread

[PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2017-05-30 11:20 +0200
    Re: [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP Marc Zyngier <marc.zyngier@arm.com> - 2017-05-30 16:00 +0200
      Re: [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell  GICP Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2017-05-30 17:10 +0200
        Re: [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP Marc Zyngier <marc.zyngier@arm.com> - 2017-05-30 17:20 +0200
          Re: [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell  GICP Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2017-05-30 17:30 +0200
            Re: [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP Marc Zyngier <marc.zyngier@arm.com> - 2017-05-30 17:40 +0200

#1652989 — [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP

This commit adds a simple driver for the Marvell GICP, a hardware unit
that converts memory writes into GIC SPI interrupts. The driver doesn't
do anything but clear all interrupts at boot time, to avoid spurious
interrupts left by the firmware.

The GICP registers are directly written to in hardware by the ICU unit,
which is configured in a separate driver.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/irqchip/Kconfig          |  3 +++
 drivers/irqchip/Makefile         |  1 +
 drivers/irqchip/irq-mvebu-gicp.c | 53 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+)
 create mode 100644 drivers/irqchip/irq-mvebu-gicp.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 478f8ac..e527ee5 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -268,6 +268,9 @@ config IRQ_MXS
 	select IRQ_DOMAIN
 	select STMP_DEVICE
 
+config MVEBU_GICP
+	bool
+
 config MVEBU_ODMI
 	bool
 	select GENERIC_MSI_IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b64c59b..11eb858 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
 obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
 obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
 obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
+obj-$(CONFIG_MVEBU_GICP)		+= irq-mvebu-gicp.o
 obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
 obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
 obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
diff --git a/drivers/irqchip/irq-mvebu-gicp.c b/drivers/irqchip/irq-mvebu-gicp.c
new file mode 100644
index 0000000..4effed4
--- /dev/null
+++ b/drivers/irqchip/irq-mvebu-gicp.c
@@ -0,0 +1,53 @@
+/*
+ * Copyright (C) 2017 Marvell
+ *
+ * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/io.h>
+#include <linux/platform_device.h>
+
+#define GICP_SETSPI_NSR_OFFSET	0x0
+#define GICP_CLRSPI_NSR_OFFSET	0x8
+
+#define GICP_INT_COUNT		128
+
+static int mvebu_gicp_probe(struct platform_device *pdev)
+{
+	void __iomem *regs;
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	regs = ioremap(res->start, resource_size(res));
+	if (!regs)
+		return -ENOMEM;
+
+	for (i = 0; i < GICP_INT_COUNT; i++)
+		writel(i, regs + GICP_CLRSPI_NSR_OFFSET);
+
+	iounmap(regs);
+
+	return 0;
+}
+
+static const struct of_device_id mvebu_gicp_of_match[] = {
+	{ .compatible = "marvell,gicp", },
+	{},
+};
+
+static struct platform_driver mvebu_gicp_driver = {
+	.probe  = mvebu_gicp_probe,
+	.driver = {
+		.name = "mvebu-gicp",
+		.of_match_table = mvebu_gicp_of_match,
+	},
+};
+builtin_platform_driver(mvebu_gicp_driver);
-- 
2.7.4

[toc] | [next] | [standalone]

#1653254

On 30/05/17 10:16, Thomas Petazzoni wrote:
> This commit adds a simple driver for the Marvell GICP, a hardware unit
> that converts memory writes into GIC SPI interrupts. The driver doesn't
> do anything but clear all interrupts at boot time, to avoid spurious
> interrupts left by the firmware.
> 
> The GICP registers are directly written to in hardware by the ICU unit,
> which is configured in a separate driver.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/irqchip/Kconfig          |  3 +++
>  drivers/irqchip/Makefile         |  1 +
>  drivers/irqchip/irq-mvebu-gicp.c | 53 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+)
>  create mode 100644 drivers/irqchip/irq-mvebu-gicp.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 478f8ac..e527ee5 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -268,6 +268,9 @@ config IRQ_MXS
>  	select IRQ_DOMAIN
>  	select STMP_DEVICE
>  
> +config MVEBU_GICP
> +	bool
> +
>  config MVEBU_ODMI
>  	bool
>  	select GENERIC_MSI_IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b64c59b..11eb858 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
>  obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
>  obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
>  obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
> +obj-$(CONFIG_MVEBU_GICP)		+= irq-mvebu-gicp.o
>  obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
>  obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
>  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
> diff --git a/drivers/irqchip/irq-mvebu-gicp.c b/drivers/irqchip/irq-mvebu-gicp.c
> new file mode 100644
> index 0000000..4effed4
> --- /dev/null
> +++ b/drivers/irqchip/irq-mvebu-gicp.c
> @@ -0,0 +1,53 @@
> +/*
> + * Copyright (C) 2017 Marvell
> + *
> + * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +
> +#define GICP_SETSPI_NSR_OFFSET	0x0
> +#define GICP_CLRSPI_NSR_OFFSET	0x8
> +
> +#define GICP_INT_COUNT		128
> +
> +static int mvebu_gicp_probe(struct platform_device *pdev)
> +{
> +	void __iomem *regs;
> +	struct resource *res;
> +	int i;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	regs = ioremap(res->start, resource_size(res));
> +	if (!regs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < GICP_INT_COUNT; i++)
> +		writel(i, regs + GICP_CLRSPI_NSR_OFFSET);

What does this do on an edge interrupt? I bet this doesn't have any
effect, so you may want to use the irq_set_irqchip_state() API to clear
a potential pending state instead (and you may want to wire it in the
ICU driver itself as well).

> +
> +	iounmap(regs);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mvebu_gicp_of_match[] = {
> +	{ .compatible = "marvell,gicp", },
> +	{},
> +};
> +
> +static struct platform_driver mvebu_gicp_driver = {
> +	.probe  = mvebu_gicp_probe,
> +	.driver = {
> +		.name = "mvebu-gicp",
> +		.of_match_table = mvebu_gicp_of_match,
> +	},
> +};
> +builtin_platform_driver(mvebu_gicp_driver);
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1653302 — Re: [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP

Hello,

On Tue, 30 May 2017 14:55:57 +0100, Marc Zyngier wrote:

> > +	for (i = 0; i < GICP_INT_COUNT; i++)
> > +		writel(i, regs + GICP_CLRSPI_NSR_OFFSET);  
> 
> What does this do on an edge interrupt?

I guess nothing. What the ICU does is:

 * For level interrupts: when the interrupt wire is asserted, write to
   SETNSR, when the interrupt wire is deasserted, write to CLRNSR

 * For edge interrupts: only the interrupt assertion causes a write to
   SETNSR.

> I bet this doesn't have any effect

Indeed. But do we care? Can an edge interrupt be left pending from the
firmware?

>, so you may want to use the irq_set_irqchip_state() API to clear a
> potential pending state instead (and you may want to wire it in the
> ICU driver itself as well).

I'm not sure how to use this irq_set_irqchip_state() API. I guess it
needs a virq that corresponds to the GIC SPI interrupt, and I'm not
sure how to get that.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1653311

On 30/05/17 15:54, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 30 May 2017 14:55:57 +0100, Marc Zyngier wrote:
> 
>>> +	for (i = 0; i < GICP_INT_COUNT; i++)
>>> +		writel(i, regs + GICP_CLRSPI_NSR_OFFSET);  
>>
>> What does this do on an edge interrupt?
> 
> I guess nothing. What the ICU does is:
> 
>  * For level interrupts: when the interrupt wire is asserted, write to
>    SETNSR, when the interrupt wire is deasserted, write to CLRNSR
> 
>  * For edge interrupts: only the interrupt assertion causes a write to
>    SETNSR.
> 
>> I bet this doesn't have any effect
> 
> Indeed. But do we care? Can an edge interrupt be left pending from the
> firmware?

I cannot see why not. It is just as likely as a level interrupt.

> 
>> , so you may want to use the irq_set_irqchip_state() API to clear a
>> potential pending state instead (and you may want to wire it in the
>> ICU driver itself as well).
> 
> I'm not sure how to use this irq_set_irqchip_state() API. I guess it
> needs a virq that corresponds to the GIC SPI interrupt, and I'm not
> sure how to get that.

You do have the virtual interrupt when doing the allocation (it is
passed as a parameter). So you could perform the mapping (call into the
lower layers), and clear the pending bit using the above API.

But maybe you don't have any edge interrupt on this SoC, and it doesn't
matter.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1653316 — Re: [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP

Hello,

On Tue, 30 May 2017 16:17:41 +0100, Marc Zyngier wrote:

> > Indeed. But do we care? Can an edge interrupt be left pending from the
> > firmware?  
> 
> I cannot see why not. It is just as likely as a level interrupt.

OK.

> > I'm not sure how to use this irq_set_irqchip_state() API. I guess it
> > needs a virq that corresponds to the GIC SPI interrupt, and I'm not
> > sure how to get that.  
> 
> You do have the virtual interrupt when doing the allocation (it is
> passed as a parameter). So you could perform the mapping (call into the
> lower layers), and clear the pending bit using the above API.

So in mvebu_icu_irq_domain_alloc(), if I do:

	irq_set_irqchip_state(virq, IRQCHIP_STATE_MASKED, true);

this will go all the way to the ->irq_set_irqchip_state() in the GIC? I
thought the virq we had was referring to an irq from the ICU domain,
not from the GIC one. But maybe I'm still getting confused by all these
irq domains.

> But maybe you don't have any edge interrupt on this SoC, and it doesn't
> matter.

We currently don't have any in the devices we support in the SoC, but
since the ICU does support edge interrupts explicitly, it's nicer if we
can get this right. Plus if this actually works, we don't need the
marvell,gicp "driver" anymore.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1653322

On 30/05/17 16:25, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 30 May 2017 16:17:41 +0100, Marc Zyngier wrote:
> 
>>> Indeed. But do we care? Can an edge interrupt be left pending from the
>>> firmware?  
>>
>> I cannot see why not. It is just as likely as a level interrupt.
> 
> OK.
> 
>>> I'm not sure how to use this irq_set_irqchip_state() API. I guess it
>>> needs a virq that corresponds to the GIC SPI interrupt, and I'm not
>>> sure how to get that.  
>>
>> You do have the virtual interrupt when doing the allocation (it is
>> passed as a parameter). So you could perform the mapping (call into the
>> lower layers), and clear the pending bit using the above API.
> 
> So in mvebu_icu_irq_domain_alloc(), if I do:
> 
> 	irq_se_irqchip_state(virq, IRQCHIP_STATE_MASKED, true);

That would be

	irq_set_irqchip_state(virq, IRQCHIP_STATE_PENDING, false);

instead. It is expected that the interrupt is already masked (otherwise,
you could be in trouble).

> this will go all the way to the ->irq_set_irqchip_state() in the GIC? I

Yup.

> thought the virq we had was referring to an irq from the ICU domain,
> not from the GIC one. But maybe I'm still getting confused by all these
> irq domains.

I'm afraid you are... ;-) This is a hierarchical domain, so the virq is
constant across the whole stack, only the irqchip-specific identifier
(hwirq) changes (see how you get it from the core code, and propatage it
to the parent domain). If you implement irq_set_irqchip_state at the ICU
level by directly calling into the parent, you should be just fine.

> 
>> But maybe you don't have any edge interrupt on this SoC, and it doesn't
>> matter.
> 
> We currently don't have any in the devices we support in the SoC, but
> since the ICU does support edge interrupts explicitly, it's nicer if we
> can get this right. Plus if this actually works, we don't need the
> marvell,gicp "driver" anymore.

That'd be great.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web