[PATCH v2 0/2] PINT irqchip driver for NXP LPC18xx family

[PATCH v2 0/2] PINT irqchip driver for NXP LPC18xx family

[Joachim Eastwood]

The LPC1850 has no less than 3 GPIO interrupt blocks. One of these
blocks is called 'gpio pin interrupt' or just PINT. LPC1850 PINT can
handle up to 8 interrupts and these have a one-to-one relationship with
the main interrupt controller (NVIC).

The interrupts on PINT can be either level or edge trigger and supports
any polarity. This patch set adds a irqchip driver for PINT on LPC18xx.

This version address the comments from Thomas and Rob.

Changes since v1:
 - use irq_gc_ack_set_bit for edge ack
 - switch to generic functions on level mask/unmask
 - use revmap to look up hwirq in handler
 - describe the interrupts property better in dt doc

Joachim Eastwood (2):
  irqchip: add lpc18xx gpio pin interrupt driver
  devicetree: document NXP LPC1850 PINT irq controller binding

 .../interrupt-controller/nxp,lpc1850-gpio-pint.txt |  26 +++
 drivers/irqchip/Kconfig                            |   5 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-lpc18xx-gpio-pint.c            | 219 +++++++++++++++++++++
 4 files changed, 251 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
 create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c

-- 
2.8.0

[PATCH v2 1/2] irqchip: add lpc18xx gpio pin interrupt driver

[Joachim Eastwood]

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
---
 drivers/irqchip/Kconfig                 |   5 +
 drivers/irqchip/Makefile                |   1 +
 drivers/irqchip/irq-lpc18xx-gpio-pint.c | 219 ++++++++++++++++++++++++++++++++
 3 files changed, 225 insertions(+)
 create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 3e12479..0278837e 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -211,6 +211,11 @@ config KEYSTONE_IRQ
 		Support for Texas Instruments Keystone 2 IRQ controller IP which
 		is part of the Keystone 2 IPC mechanism
 
+config LPC18XX_GPIO_PINT
+	bool
+	select IRQ_DOMAIN
+	select GENERIC_IRQ_CHIP
+
 config MIPS_GIC
 	bool
 	select GENERIC_IRQ_IPI
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b03cfcb..bf60e0c 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_BCM7038_L1_IRQ)		+= irq-bcm7038-l1.o
 obj-$(CONFIG_BCM7120_L2_IRQ)		+= irq-bcm7120-l2.o
 obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
 obj-$(CONFIG_KEYSTONE_IRQ)		+= irq-keystone.o
+obj-$(CONFIG_LPC18XX_GPIO_PINT)		+= irq-lpc18xx-gpio-pint.o
 obj-$(CONFIG_MIPS_GIC)			+= irq-mips-gic.o
 obj-$(CONFIG_ARCH_MEDIATEK)		+= irq-mtk-sysirq.o
 obj-$(CONFIG_ARCH_DIGICOLOR)		+= irq-digicolor.o
diff --git a/drivers/irqchip/irq-lpc18xx-gpio-pint.c b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
new file mode 100644
index 0000000..d53e99b
--- /dev/null
+++ b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
@@ -0,0 +1,219 @@
+/*
+ * Irqchip driver for GPIO Pin Interrupt (PINT) on NXP LPC18xx/43xx.
+ *
+ * Copyright (C) 2016 Joachim Eastwood <manabian@gmail.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.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+
+/* LPC18xx GPIO pin interrupt register offsets */
+#define LPC18XX_GPIO_PINT_ISEL		0x000
+#define LPC18XX_GPIO_PINT_SIENR		0x008
+#define LPC18XX_GPIO_PINT_CIENR		0x00c
+#define LPC18XX_GPIO_PINT_SIENF		0x014
+#define LPC18XX_GPIO_PINT_CIENF		0x018
+#define LPC18XX_GPIO_PINT_IST		0x024
+
+#define PINT_MAX_IRQS			32
+
+struct lpc18xx_gpio_pint_chip {
+	struct irq_domain *domain;
+	void __iomem	  *base;
+	struct clk	  *clk;
+	unsigned int	  revmap[];
+};
+
+static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
+{
+	struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
+	unsigned int irq = irq_desc_get_irq(desc);
+	unsigned int hwirq = pint->revmap[irq];
+	unsigned int virq;
+
+	virq = irq_find_mapping(pint->domain, hwirq);
+	generic_handle_irq(virq);
+}
+
+static void lpc18xx_gpio_pint_edge_mask(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENR);
+	irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENF);
+	irq_gc_unlock(gc);
+}
+
+static void lpc18xx_gpio_pint_edge_unmask(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	u32 type, mask = d->mask;
+
+	irq_gc_lock(gc);
+	type = irqd_get_trigger_type(d);
+	if (type & IRQ_TYPE_EDGE_RISING)
+		irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENR);
+	if (type & IRQ_TYPE_EDGE_FALLING)
+		irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENF);
+	irq_gc_unlock(gc);
+}
+
+static int lpc18xx_gpio_pint_type(struct irq_data *data, unsigned int type)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+	u32 mask = data->mask;
+
+	irq_gc_lock(gc);
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		gc->type_cache |= mask;
+	else
+		gc->type_cache &= ~mask;
+	irq_reg_writel(gc, gc->type_cache, LPC18XX_GPIO_PINT_ISEL);
+
+	switch (type) {
+	case IRQ_TYPE_LEVEL_HIGH:
+		irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENF);
+		break;
+
+	case IRQ_TYPE_LEVEL_LOW:
+		irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENF);
+		break;
+
+	/* IRQ_TYPE_EDGE_* is set in lpc18xx_gpio_pint_edge_unmask */
+	}
+
+	irqd_set_trigger_type(data, type);
+	irq_setup_alt_chip(data, type);
+	irq_gc_unlock(gc);
+
+	return 0;
+}
+
+static int lpc18xx_gpio_pint_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct lpc18xx_gpio_pint_chip *pint;
+	struct irq_chip_generic *gc;
+	int irqs[PINT_MAX_IRQS];
+	unsigned int max_virqno;
+	struct resource *regs;
+	int nrirqs;
+	int i, ret;
+
+	nrirqs = of_irq_count(np);
+	if (nrirqs > PINT_MAX_IRQS)
+		return -EINVAL;
+
+	max_virqno = 0;
+	for (i = 0; i < nrirqs; i++) {
+		irqs[i] = platform_get_irq(pdev, i);
+		if (max_virqno < irqs[i])
+			max_virqno = irqs[i];
+	}
+
+	pint = devm_kzalloc(&pdev->dev,
+			    sizeof(*pint) + (sizeof(unsigned int) * max_virqno),
+			    GFP_KERNEL);
+	if (!pint)
+		return -ENOMEM;
+
+	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pint->base = devm_ioremap_resource(&pdev->dev, regs);
+	if (IS_ERR(pint->base))
+		return PTR_ERR(pint->base);
+
+
+	pint->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pint->clk)) {
+		dev_err(&pdev->dev, "input clock not found\n");
+		return PTR_ERR(pint->clk);
+	}
+
+	ret = clk_prepare_enable(pint->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to enable clock\n");
+		return ret;
+	}
+
+	pint->domain = irq_domain_add_linear(np, nrirqs, &irq_generic_chip_ops,
+					     pint);
+	if (!pint->domain) {
+		dev_err(&pdev->dev, "unable setup irq domain\n");
+		ret = -EINVAL;
+		goto err_domain_add;
+	}
+
+	ret = irq_alloc_domain_generic_chips(pint->domain, nrirqs, 2,
+					     "gpio_pint", handle_edge_irq,
+					     0, 0, 0);
+	if (ret) {
+		dev_err(&pdev->dev, "unable alloc irq domain chips\n");
+		goto err_alloc_domain_gc;
+	}
+
+	gc = irq_get_domain_generic_chip(pint->domain, 0);
+	gc->reg_base = pint->base;
+
+	gc->chip_types[0].type		    = IRQ_TYPE_EDGE_BOTH;
+	gc->chip_types[0].handler	    = handle_edge_irq;
+	gc->chip_types[0].chip.irq_ack	    = irq_gc_ack_set_bit;
+	gc->chip_types[0].chip.irq_mask	    = lpc18xx_gpio_pint_edge_mask;
+	gc->chip_types[0].chip.irq_unmask   = lpc18xx_gpio_pint_edge_unmask;
+	gc->chip_types[0].chip.irq_set_type = lpc18xx_gpio_pint_type;
+	gc->chip_types[0].regs.ack	    = LPC18XX_GPIO_PINT_IST;
+
+	gc->chip_types[1].type		    = IRQ_TYPE_LEVEL_MASK;
+	gc->chip_types[1].handler	    = handle_level_irq;
+	gc->chip_types[1].chip.irq_mask	    = irq_gc_mask_disable_reg;
+	gc->chip_types[1].chip.irq_unmask   = irq_gc_unmask_enable_reg;
+	gc->chip_types[1].chip.irq_set_type = lpc18xx_gpio_pint_type;
+	gc->chip_types[1].regs.enable	    = LPC18XX_GPIO_PINT_SIENR;
+	gc->chip_types[1].regs.disable	    = LPC18XX_GPIO_PINT_CIENR;
+
+	/* Disable and clear all interrupts */
+	writel(~0, pint->base + LPC18XX_GPIO_PINT_CIENR);
+	writel(~0, pint->base + LPC18XX_GPIO_PINT_CIENF);
+	writel(0,  pint->base + LPC18XX_GPIO_PINT_ISEL);
+	writel(~0, pint->base + LPC18XX_GPIO_PINT_IST);
+
+	for (i = 0; i < nrirqs; i++) {
+		pint->revmap[irqs[i]] = i;
+		irq_set_chained_handler_and_data(irqs[i],
+						 lpc18xx_gpio_pint_handler,
+						 pint);
+	}
+
+	return 0;
+
+err_alloc_domain_gc:
+	irq_domain_remove(pint->domain);
+err_domain_add:
+	clk_disable_unprepare(pint->clk);
+	return ret;
+}
+
+static const struct of_device_id lpc18xx_gpio_pint_match[] = {
+	{ .compatible = "nxp,lpc1850-gpio-pint" },
+	{ }
+};
+
+static struct platform_driver lpc18xx_gpio_pint_driver = {
+	.probe	= lpc18xx_gpio_pint_probe,
+	.driver	= {
+		.name = "lpc18xx-gpio-pint",
+		.of_match_table = lpc18xx_gpio_pint_match,
+	},
+};
+builtin_platform_driver(lpc18xx_gpio_pint_driver);
-- 
2.8.0

[PATCH v2 2/2] devicetree: document NXP LPC1850 PINT irq controller binding

[Joachim Eastwood]

Add binding documentation for NXP LPC1850 GPIO Pin Interrupt (PINT)
controller.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
---
 .../interrupt-controller/nxp,lpc1850-gpio-pint.txt | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
new file mode 100644
index 0000000..0d59d31
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
@@ -0,0 +1,26 @@
+NXP LPC18xx/43xx GPIO Pin Interrupt (PINT) controller
+
+Required properties:
+
+- compatible : should be "nxp,lpc1850-gpio-pint".
+- reg : Specifies base physical address and size of the registers.
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source. The value shall be 2.
+- interrupts : Must contain the interrupt numbers from the parent controller.
+	       This defines the mapping between the controllers where the first
+	       interrupt is mapped to pint hwirq 0 and so on. Which interrupts
+	       that are connected to where and the number of is device specific.
+	       For lpc1850 the mapping is 32 33 34 35 36 37 38 39 (8 in total).
+- clocks: Must contain a reference to the functional clock.
+
+Example:
+
+pint: interrupt-controller@40087000 {
+	compatible = "nxp,lpc1850-gpio-pint";
+	reg = <0x40087000 0x1000>;
+	interrupt-controller;
+	#interrupt-cells = <2>;
+	interrupts = <32 33 34 35 36 37 38 39>;
+	clocks = <&ccu1 CLK_CPU_GPIO>;
+};
-- 
2.8.0

Re: [PATCH v2 2/2] devicetree: document NXP LPC1850 PINT irq controller binding

[Rob Herring]

On Sat, Apr 02, 2016 at 06:35:24PM +0200, Joachim Eastwood wrote:
> Add binding documentation for NXP LPC1850 GPIO Pin Interrupt (PINT)
> controller.
> 
> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
> ---
>  .../interrupt-controller/nxp,lpc1850-gpio-pint.txt | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 1/2] irqchip: add lpc18xx gpio pin interrupt driver

[Marc Zyngier]

Hi Joachim,

On 02/04/16 17:35, Joachim Eastwood wrote:
> Signed-off-by: Joachim Eastwood <manabian@gmail.com>

As a start, a commit message would be appreciated.

> ---
>  drivers/irqchip/Kconfig                 |   5 +
>  drivers/irqchip/Makefile                |   1 +
>  drivers/irqchip/irq-lpc18xx-gpio-pint.c | 219 ++++++++++++++++++++++++++++++++
>  3 files changed, 225 insertions(+)
>  create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 3e12479..0278837e 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -211,6 +211,11 @@ config KEYSTONE_IRQ
>  		Support for Texas Instruments Keystone 2 IRQ controller IP which
>  		is part of the Keystone 2 IPC mechanism
>  
> +config LPC18XX_GPIO_PINT
> +	bool
> +	select IRQ_DOMAIN
> +	select GENERIC_IRQ_CHIP
> +
>  config MIPS_GIC
>  	bool
>  	select GENERIC_IRQ_IPI
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b03cfcb..bf60e0c 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_BCM7038_L1_IRQ)		+= irq-bcm7038-l1.o
>  obj-$(CONFIG_BCM7120_L2_IRQ)		+= irq-bcm7120-l2.o
>  obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
>  obj-$(CONFIG_KEYSTONE_IRQ)		+= irq-keystone.o
> +obj-$(CONFIG_LPC18XX_GPIO_PINT)		+= irq-lpc18xx-gpio-pint.o
>  obj-$(CONFIG_MIPS_GIC)			+= irq-mips-gic.o
>  obj-$(CONFIG_ARCH_MEDIATEK)		+= irq-mtk-sysirq.o
>  obj-$(CONFIG_ARCH_DIGICOLOR)		+= irq-digicolor.o
> diff --git a/drivers/irqchip/irq-lpc18xx-gpio-pint.c b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
> new file mode 100644
> index 0000000..d53e99b
> --- /dev/null
> +++ b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
> @@ -0,0 +1,219 @@
> +/*
> + * Irqchip driver for GPIO Pin Interrupt (PINT) on NXP LPC18xx/43xx.
> + *
> + * Copyright (C) 2016 Joachim Eastwood <manabian@gmail.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.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +
> +/* LPC18xx GPIO pin interrupt register offsets */
> +#define LPC18XX_GPIO_PINT_ISEL		0x000
> +#define LPC18XX_GPIO_PINT_SIENR		0x008
> +#define LPC18XX_GPIO_PINT_CIENR		0x00c
> +#define LPC18XX_GPIO_PINT_SIENF		0x014
> +#define LPC18XX_GPIO_PINT_CIENF		0x018
> +#define LPC18XX_GPIO_PINT_IST		0x024
> +
> +#define PINT_MAX_IRQS			32
> +
> +struct lpc18xx_gpio_pint_chip {
> +	struct irq_domain *domain;
> +	void __iomem	  *base;
> +	struct clk	  *clk;
> +	unsigned int	  revmap[];
> +};
> +
> +static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
> +{
> +	struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
> +	unsigned int irq = irq_desc_get_irq(desc);
> +	unsigned int hwirq = pint->revmap[irq];
> +	unsigned int virq;
> +
> +	virq = irq_find_mapping(pint->domain, hwirq);
> +	generic_handle_irq(virq);

Two things here:
- It feels a bit weird that you are indirecting everything through a
cascade interrupt. As you have a 1:1 relationship between the PINT
interrupts and their NVIC counterparts, this would be better described
as a hierarchical irqchip (see drivers/irqchip/irq-vf610-mscm-ir.c for
an example)
- If you decide to stick with the current approach, you're then missing
the usual chained_irq_{enter,exit}.

Thanks,

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

Re: [PATCH v2 1/2] irqchip: add lpc18xx gpio pin interrupt driver

[Marc Zyngier]

Hi Joachim,

On 02/04/16 17:35, Joachim Eastwood wrote:
> Signed-off-by: Joachim Eastwood <manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

As a start, a commit message would be appreciated.

> ---
>  drivers/irqchip/Kconfig                 |   5 +
>  drivers/irqchip/Makefile                |   1 +
>  drivers/irqchip/irq-lpc18xx-gpio-pint.c | 219 ++++++++++++++++++++++++++++++++
>  3 files changed, 225 insertions(+)
>  create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 3e12479..0278837e 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -211,6 +211,11 @@ config KEYSTONE_IRQ
>  		Support for Texas Instruments Keystone 2 IRQ controller IP which
>  		is part of the Keystone 2 IPC mechanism
>  
> +config LPC18XX_GPIO_PINT
> +	bool
> +	select IRQ_DOMAIN
> +	select GENERIC_IRQ_CHIP
> +
>  config MIPS_GIC
>  	bool
>  	select GENERIC_IRQ_IPI
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b03cfcb..bf60e0c 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_BCM7038_L1_IRQ)		+= irq-bcm7038-l1.o
>  obj-$(CONFIG_BCM7120_L2_IRQ)		+= irq-bcm7120-l2.o
>  obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
>  obj-$(CONFIG_KEYSTONE_IRQ)		+= irq-keystone.o
> +obj-$(CONFIG_LPC18XX_GPIO_PINT)		+= irq-lpc18xx-gpio-pint.o
>  obj-$(CONFIG_MIPS_GIC)			+= irq-mips-gic.o
>  obj-$(CONFIG_ARCH_MEDIATEK)		+= irq-mtk-sysirq.o
>  obj-$(CONFIG_ARCH_DIGICOLOR)		+= irq-digicolor.o
> diff --git a/drivers/irqchip/irq-lpc18xx-gpio-pint.c b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
> new file mode 100644
> index 0000000..d53e99b
> --- /dev/null
> +++ b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
> @@ -0,0 +1,219 @@
> +/*
> + * Irqchip driver for GPIO Pin Interrupt (PINT) on NXP LPC18xx/43xx.
> + *
> + * Copyright (C) 2016 Joachim Eastwood <manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *
> + * 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.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +
> +/* LPC18xx GPIO pin interrupt register offsets */
> +#define LPC18XX_GPIO_PINT_ISEL		0x000
> +#define LPC18XX_GPIO_PINT_SIENR		0x008
> +#define LPC18XX_GPIO_PINT_CIENR		0x00c
> +#define LPC18XX_GPIO_PINT_SIENF		0x014
> +#define LPC18XX_GPIO_PINT_CIENF		0x018
> +#define LPC18XX_GPIO_PINT_IST		0x024
> +
> +#define PINT_MAX_IRQS			32
> +
> +struct lpc18xx_gpio_pint_chip {
> +	struct irq_domain *domain;
> +	void __iomem	  *base;
> +	struct clk	  *clk;
> +	unsigned int	  revmap[];
> +};
> +
> +static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
> +{
> +	struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
> +	unsigned int irq = irq_desc_get_irq(desc);
> +	unsigned int hwirq = pint->revmap[irq];
> +	unsigned int virq;
> +
> +	virq = irq_find_mapping(pint->domain, hwirq);
> +	generic_handle_irq(virq);

Two things here:
- It feels a bit weird that you are indirecting everything through a
cascade interrupt. As you have a 1:1 relationship between the PINT
interrupts and their NVIC counterparts, this would be better described
as a hierarchical irqchip (see drivers/irqchip/irq-vf610-mscm-ir.c for
an example)
- If you decide to stick with the current approach, you're then missing
the usual chained_irq_{enter,exit}.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] irqchip: add lpc18xx gpio pin interrupt driver

[Joachim Eastwood]

Hi Marc,

On 11 April 2016 at 17:15, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Joachim,
>
> On 02/04/16 17:35, Joachim Eastwood wrote:
>> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
>
> As a start, a commit message would be appreciated.

Ops! I wonder where that disappeared to. The previous version did have one:
https://www.marc.info/?l=devicetree&m=145643797630859&w=3

I'll add it back in the next version.

>> ---
>>  drivers/irqchip/Kconfig                 |   5 +
>>  drivers/irqchip/Makefile                |   1 +
>>  drivers/irqchip/irq-lpc18xx-gpio-pint.c | 219 ++++++++++++++++++++++++++++++++
>>  3 files changed, 225 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 3e12479..0278837e 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -211,6 +211,11 @@ config KEYSTONE_IRQ
>>               Support for Texas Instruments Keystone 2 IRQ controller IP which
>>               is part of the Keystone 2 IPC mechanism
>>
>> +config LPC18XX_GPIO_PINT
>> +     bool
>> +     select IRQ_DOMAIN
>> +     select GENERIC_IRQ_CHIP
>> +
>>  config MIPS_GIC
>>       bool
>>       select GENERIC_IRQ_IPI
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index b03cfcb..bf60e0c 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -55,6 +55,7 @@ obj-$(CONFIG_BCM7038_L1_IRQ)                += irq-bcm7038-l1.o
>>  obj-$(CONFIG_BCM7120_L2_IRQ)         += irq-bcm7120-l2.o
>>  obj-$(CONFIG_BRCMSTB_L2_IRQ)         += irq-brcmstb-l2.o
>>  obj-$(CONFIG_KEYSTONE_IRQ)           += irq-keystone.o
>> +obj-$(CONFIG_LPC18XX_GPIO_PINT)              += irq-lpc18xx-gpio-pint.o
>>  obj-$(CONFIG_MIPS_GIC)                       += irq-mips-gic.o
>>  obj-$(CONFIG_ARCH_MEDIATEK)          += irq-mtk-sysirq.o
>>  obj-$(CONFIG_ARCH_DIGICOLOR)         += irq-digicolor.o
>> diff --git a/drivers/irqchip/irq-lpc18xx-gpio-pint.c b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
>> new file mode 100644
>> index 0000000..d53e99b
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
>> @@ -0,0 +1,219 @@
>> +/*
>> + * Irqchip driver for GPIO Pin Interrupt (PINT) on NXP LPC18xx/43xx.
>> + *
>> + * Copyright (C) 2016 Joachim Eastwood <manabian@gmail.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.
>> + *
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/init.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +
>> +/* LPC18xx GPIO pin interrupt register offsets */
>> +#define LPC18XX_GPIO_PINT_ISEL               0x000
>> +#define LPC18XX_GPIO_PINT_SIENR              0x008
>> +#define LPC18XX_GPIO_PINT_CIENR              0x00c
>> +#define LPC18XX_GPIO_PINT_SIENF              0x014
>> +#define LPC18XX_GPIO_PINT_CIENF              0x018
>> +#define LPC18XX_GPIO_PINT_IST                0x024
>> +
>> +#define PINT_MAX_IRQS                        32
>> +
>> +struct lpc18xx_gpio_pint_chip {
>> +     struct irq_domain *domain;
>> +     void __iomem      *base;
>> +     struct clk        *clk;
>> +     unsigned int      revmap[];
>> +};
>> +
>> +static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
>> +{
>> +     struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
>> +     unsigned int irq = irq_desc_get_irq(desc);
>> +     unsigned int hwirq = pint->revmap[irq];
>> +     unsigned int virq;
>> +
>> +     virq = irq_find_mapping(pint->domain, hwirq);
>> +     generic_handle_irq(virq);
>
> Two things here:
> - It feels a bit weird that you are indirecting everything through a
> cascade interrupt. As you have a 1:1 relationship between the PINT
> interrupts and their NVIC counterparts, this would be better described
> as a hierarchical irqchip (see drivers/irqchip/irq-vf610-mscm-ir.c for
> an example)

Okey, I'll have a look at how irq-vf610-mscm-ir handels it.


> - If you decide to stick with the current approach, you're then missing
> the usual chained_irq_{enter,exit}.

Indeed.


Thanks for looking through.


regards,
Joachim Eastwood

Re: [PATCH v2 1/2] irqchip: add lpc18xx gpio pin interrupt driver

[Joachim Eastwood]

Hi Marc,

On 11 April 2016 at 17:15, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi Joachim,
>
> On 02/04/16 17:35, Joachim Eastwood wrote:
>> Signed-off-by: Joachim Eastwood <manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> As a start, a commit message would be appreciated.

Ops! I wonder where that disappeared to. The previous version did have one:
https://www.marc.info/?l=devicetree&m=145643797630859&w=3

I'll add it back in the next version.

>> ---
>>  drivers/irqchip/Kconfig                 |   5 +
>>  drivers/irqchip/Makefile                |   1 +
>>  drivers/irqchip/irq-lpc18xx-gpio-pint.c | 219 ++++++++++++++++++++++++++++++++
>>  3 files changed, 225 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 3e12479..0278837e 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -211,6 +211,11 @@ config KEYSTONE_IRQ
>>               Support for Texas Instruments Keystone 2 IRQ controller IP which
>>               is part of the Keystone 2 IPC mechanism
>>
>> +config LPC18XX_GPIO_PINT
>> +     bool
>> +     select IRQ_DOMAIN
>> +     select GENERIC_IRQ_CHIP
>> +
>>  config MIPS_GIC
>>       bool
>>       select GENERIC_IRQ_IPI
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index b03cfcb..bf60e0c 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -55,6 +55,7 @@ obj-$(CONFIG_BCM7038_L1_IRQ)                += irq-bcm7038-l1.o
>>  obj-$(CONFIG_BCM7120_L2_IRQ)         += irq-bcm7120-l2.o
>>  obj-$(CONFIG_BRCMSTB_L2_IRQ)         += irq-brcmstb-l2.o
>>  obj-$(CONFIG_KEYSTONE_IRQ)           += irq-keystone.o
>> +obj-$(CONFIG_LPC18XX_GPIO_PINT)              += irq-lpc18xx-gpio-pint.o
>>  obj-$(CONFIG_MIPS_GIC)                       += irq-mips-gic.o
>>  obj-$(CONFIG_ARCH_MEDIATEK)          += irq-mtk-sysirq.o
>>  obj-$(CONFIG_ARCH_DIGICOLOR)         += irq-digicolor.o
>> diff --git a/drivers/irqchip/irq-lpc18xx-gpio-pint.c b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
>> new file mode 100644
>> index 0000000..d53e99b
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
>> @@ -0,0 +1,219 @@
>> +/*
>> + * Irqchip driver for GPIO Pin Interrupt (PINT) on NXP LPC18xx/43xx.
>> + *
>> + * Copyright (C) 2016 Joachim Eastwood <manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/init.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +
>> +/* LPC18xx GPIO pin interrupt register offsets */
>> +#define LPC18XX_GPIO_PINT_ISEL               0x000
>> +#define LPC18XX_GPIO_PINT_SIENR              0x008
>> +#define LPC18XX_GPIO_PINT_CIENR              0x00c
>> +#define LPC18XX_GPIO_PINT_SIENF              0x014
>> +#define LPC18XX_GPIO_PINT_CIENF              0x018
>> +#define LPC18XX_GPIO_PINT_IST                0x024
>> +
>> +#define PINT_MAX_IRQS                        32
>> +
>> +struct lpc18xx_gpio_pint_chip {
>> +     struct irq_domain *domain;
>> +     void __iomem      *base;
>> +     struct clk        *clk;
>> +     unsigned int      revmap[];
>> +};
>> +
>> +static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
>> +{
>> +     struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
>> +     unsigned int irq = irq_desc_get_irq(desc);
>> +     unsigned int hwirq = pint->revmap[irq];
>> +     unsigned int virq;
>> +
>> +     virq = irq_find_mapping(pint->domain, hwirq);
>> +     generic_handle_irq(virq);
>
> Two things here:
> - It feels a bit weird that you are indirecting everything through a
> cascade interrupt. As you have a 1:1 relationship between the PINT
> interrupts and their NVIC counterparts, this would be better described
> as a hierarchical irqchip (see drivers/irqchip/irq-vf610-mscm-ir.c for
> an example)

Okey, I'll have a look at how irq-vf610-mscm-ir handels it.


> - If you decide to stick with the current approach, you're then missing
> the usual chained_irq_{enter,exit}.

Indeed.


Thanks for looking through.


regards,
Joachim Eastwood
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html