[PATCH] irqchip: Renesas INTC External IRQ pin driver

[PATCH] irqchip: Renesas INTC External IRQ pin driver

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

This patch adds a driver for external IRQ pins connected
to the INTC block on recent SoCs from Renesas.

The INTC hardware block usually contains a rather wide
range of features ranging from external IRQ pin handling
to legacy interrupt controller support. On older SoCs
the INTC is used as a general purpose interrupt controller
both for external IRQ pins and on-chip devices.

On more recent ARM based SoCs with Cortex-A9 the main
interrupt controller is the GIC, but IRQ trigger setup
still need to happen in the INTC hardware block.

This driver implements the glue code needed to configure
IRQ trigger and also handle mask/unmask and demux of
external IRQ pins hooked up from the INTC to the GIC.

Tested on sh73a0 and r8a7779. The hardware varies quite
a bit with SoC model, for instance register width and
bitfield widths vary wildly. The driver requires one GIC
SPI per external IRQ pin to operate.  Each driver instance
will handle up to 8 external IRQ pins.

The SoCs using this driver are currently mainly used
together with regular platform devices so this driver
allows configuration via platform data to support things
like static interrupt base address. DT support will
be added incrementally in the not so distant future.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/irqchip/Kconfig                               |    4 
 drivers/irqchip/Makefile                              |    1 
 drivers/irqchip/irq-renesas-intc-irqpin.c             |  464 +++++++++++++++++
 include/linux/platform_data/irq-renesas-intc-irqpin.h |   10 
 4 files changed, 479 insertions(+)

--- 0001/drivers/irqchip/Kconfig
+++ work/drivers/irqchip/Kconfig	2013-02-18 18:28:22.000000000 +0900
@@ -25,6 +25,10 @@ config ARM_VIC_NR
 	  The maximum number of VICs available in the system, for
 	  power management.
 
+config RENESAS_INTC_IRQPIN
+	bool
+	select IRQ_DOMAIN
+
 config VERSATILE_FPGA_IRQ
 	bool
 	select IRQ_DOMAIN
--- 0001/drivers/irqchip/Makefile
+++ work/drivers/irqchip/Makefile	2013-02-18 18:28:22.000000000 +0900
@@ -5,4 +5,5 @@ obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi.o
 obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
 obj-$(CONFIG_ARM_GIC)			+= irq-gic.o
 obj-$(CONFIG_ARM_VIC)			+= irq-vic.o
+obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
 obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
--- /dev/null
+++ work/drivers/irqchip/irq-renesas-intc-irqpin.c	2013-02-18 21:06:32.000000000 +0900
@@ -0,0 +1,464 @@
+/*
+ * Renesas INTC External IRQ Pin Driver
+ *
+ *  Copyright (C) 2013 Magnus Damm
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License
+ *
+ * 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, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/interrupt.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/platform_data/irq-renesas-intc-irqpin.h>
+
+#define INTC_IRQPIN_MAX 8 /* maximum 8 interrupts per driver instance */
+
+#define INTC_IRQPIN_REG_SENSE 0 /* ICRn */
+#define INTC_IRQPIN_REG_PRIO 1 /* INTPRInn */
+#define INTC_IRQPIN_REG_SOURCE 2 /* INTREQnn */
+#define INTC_IRQPIN_REG_MASK 3 /* INTMSKnn */
+#define INTC_IRQPIN_REG_CLEAR 4 /* INTMSKCLRnn */
+#define INTC_IRQPIN_REG_NR 5
+
+/* INTC external IRQ PIN hardware register access:
+ *
+ * SENSE is read-write 32-bit with 2-bits or 4-bits per IRQ (*)
+ * PRIO is read-write 32-bit with 4-bits per IRQ (**)
+ * SOURCE is read-only 32-bit or 8-bit with 1-bit per IRQ (***)
+ * MASK is write-only 32-bit or 8-bit with 1-bit per IRQ (***)
+ * CLEAR is write-only 32-bit or 8-bit with 1-bit per IRQ (***)
+ *
+ * (*) May be accessed by more than one driver instance - lock needed
+ * (**) Read-modify-write access by one driver instance - lock needed
+ * (***) Accessed by one driver instance only - no locking needed
+ */
+
+struct intc_irqpin_iomem {
+	void __iomem *iomem;
+	unsigned long (*read)(void __iomem *iomem);
+	void (*write)(void __iomem *iomem, unsigned long data);
+	int width;
+};  
+
+struct intc_irqpin_irq {
+	int hw_irq;
+	int irq;
+	struct intc_irqpin_priv *p;
+};  
+
+struct intc_irqpin_priv {
+	struct intc_irqpin_iomem iomem[INTC_IRQPIN_REG_NR];
+	struct intc_irqpin_irq irq[INTC_IRQPIN_MAX];
+	struct renesas_intc_irqpin_config config;
+	unsigned int number_of_irqs;
+	struct platform_device *pdev;
+	struct irq_chip irq_chip;
+	struct irq_domain *irq_domain;
+};
+
+static unsigned long intc_irqpin_read32(void __iomem *iomem)
+{
+	return ioread32(iomem);
+}
+
+static unsigned long intc_irqpin_read8(void __iomem *iomem)
+{
+	return ioread8(iomem);
+}
+
+static void intc_irqpin_write32(void __iomem *iomem, unsigned long data)
+{
+	iowrite32(data, iomem);
+}
+
+static void intc_irqpin_write8(void __iomem *iomem, unsigned long data)
+{
+	iowrite8(data, iomem);
+}
+
+static inline unsigned long intc_irqpin_read(struct intc_irqpin_priv *p,
+					     int reg)
+{
+	struct intc_irqpin_iomem *i = &p->iomem[reg];
+	return i->read(i->iomem);
+}
+
+static inline void intc_irqpin_write(struct intc_irqpin_priv *p,
+				     int reg, unsigned long data)
+{
+	struct intc_irqpin_iomem *i = &p->iomem[reg];
+	i->write(i->iomem, data);
+}
+
+static inline unsigned long intc_irqpin_hwirq_mask(struct intc_irqpin_priv *p,
+						   int reg, int hw_irq)
+{
+	return BIT((p->iomem[reg].width - 1) - hw_irq);
+}
+
+static inline void intc_irqpin_irq_write_hwirq(struct intc_irqpin_priv *p,
+					       int reg, int hw_irq)
+{
+	intc_irqpin_write(p, reg, intc_irqpin_hwirq_mask(p, reg, hw_irq));
+}
+
+static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
+
+static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
+					  int reg, int shift,
+					  int width, int value)
+{
+	unsigned long flags;
+	unsigned long tmp;
+
+	raw_spin_lock_irqsave(&intc_irqpin_lock, flags);
+
+	tmp = intc_irqpin_read(p, reg);
+	tmp &= ~(((1 << width) - 1) << shift);
+	tmp |= value << shift;
+	intc_irqpin_write(p, reg, tmp);
+
+	raw_spin_unlock_irqrestore(&intc_irqpin_lock, flags);
+}
+
+static void intc_irqpin_mask_unmask_prio(struct intc_irqpin_priv *p,
+					 int irq, int do_mask)
+{
+	int bitfield_width = 4; /* PRIO assumed to have fixed bitfield width */
+	int shift = (7 - irq) * bitfield_width; /* PRIO assumed to be 32-bit */
+
+	intc_irqpin_read_modify_write(p, INTC_IRQPIN_REG_PRIO,
+				      shift, bitfield_width,
+				      do_mask ? 0 : (1 << bitfield_width) - 1);
+}
+
+static int intc_irqpin_set_sense(struct intc_irqpin_priv *p, int irq, int value)
+{
+	int bitfield_width = p->config.sense_bitfield_width;
+	int shift = (7 - irq) * bitfield_width; /* SENSE assumed to be 32-bit */
+
+	dev_dbg(&p->pdev->dev, "sense irq = %d, mode = %d\n", irq, value);
+
+	if (value >= (1 << bitfield_width))
+		return -EINVAL;
+
+	intc_irqpin_read_modify_write(p, INTC_IRQPIN_REG_SENSE, shift,
+				      bitfield_width, value);
+	return 0;
+}
+
+static void intc_irqpin_dbg(struct intc_irqpin_irq *i, char *str)
+{
+	dev_dbg(&i->p->pdev->dev, "%s (%d:%d:%d)\n",
+		str, i->irq, i->hw_irq,
+		irq_find_mapping(i->p->irq_domain, i->hw_irq));
+}
+
+static void intc_irqpin_irq_enable(struct irq_data *d)
+{
+	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+	int hw_irq = irqd_to_hwirq(d);
+
+	intc_irqpin_dbg(&p->irq[hw_irq], "enable");
+	intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_CLEAR, hw_irq);
+}
+
+static void intc_irqpin_irq_disable(struct irq_data *d)
+{
+	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+	int hw_irq = irqd_to_hwirq(d);
+
+	intc_irqpin_dbg(&p->irq[hw_irq], "disable");
+	intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_MASK, hw_irq);
+}
+
+static void intc_irqpin_irq_enable_force(struct irq_data *d)
+{
+	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+	int irq = p->irq[irqd_to_hwirq(d)].irq;
+
+	intc_irqpin_irq_enable(d);
+	irq_get_chip(irq)->irq_unmask(irq_get_irq_data(irq));
+}
+
+static void intc_irqpin_irq_disable_force(struct irq_data *d)
+{
+	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+	int irq = p->irq[irqd_to_hwirq(d)].irq;
+
+	irq_get_chip(irq)->irq_mask(irq_get_irq_data(irq));
+	intc_irqpin_irq_disable(d);
+}
+
+#define INTC_IRQ_SENSE_VALID 0x10
+#define INTC_IRQ_SENSE(x) (x + INTC_IRQ_SENSE_VALID)
+
+static unsigned char intc_irqpin_sense[IRQ_TYPE_SENSE_MASK + 1] = {
+	[IRQ_TYPE_EDGE_FALLING] = INTC_IRQ_SENSE(0x00),
+	[IRQ_TYPE_EDGE_RISING] = INTC_IRQ_SENSE(0x01),
+	[IRQ_TYPE_LEVEL_LOW] = INTC_IRQ_SENSE(0x02),
+	[IRQ_TYPE_LEVEL_HIGH] = INTC_IRQ_SENSE(0x03),
+	[IRQ_TYPE_EDGE_BOTH] = INTC_IRQ_SENSE(0x04),
+};
+
+static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	unsigned char value = intc_irqpin_sense[type & IRQ_TYPE_SENSE_MASK];
+	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+
+	if (!(value & INTC_IRQ_SENSE_VALID))
+		return -EINVAL;
+
+	return intc_irqpin_set_sense(p, irqd_to_hwirq(d),
+				     value ^ INTC_IRQ_SENSE_VALID);
+}
+
+static irqreturn_t intc_irqpin_irq_handler(int irq, void *dev_id)
+{
+	struct intc_irqpin_irq *i = dev_id;
+	struct intc_irqpin_priv *p = i->p;
+	unsigned long bit;
+
+	intc_irqpin_dbg(i, "demux1");
+	bit = intc_irqpin_hwirq_mask(p, INTC_IRQPIN_REG_SOURCE, i->hw_irq);
+
+	if (intc_irqpin_read(p, INTC_IRQPIN_REG_SOURCE) & bit) {
+		intc_irqpin_write(p, INTC_IRQPIN_REG_SOURCE, ~bit);
+		intc_irqpin_dbg(i, "demux2");
+		generic_handle_irq(irq_find_mapping(p->irq_domain, i->hw_irq));
+		return IRQ_HANDLED;
+	}
+	return IRQ_NONE;
+}
+
+static int intc_irqpin_irq_domain_map(struct irq_domain *h, unsigned int virq,
+				      irq_hw_number_t hw)
+{
+	struct intc_irqpin_priv *p = h->host_data;
+
+	intc_irqpin_dbg(&p->irq[hw], "map");
+	irq_set_chip_data(virq, h->host_data);
+	irq_set_chip_and_handler(virq, &p->irq_chip, handle_level_irq);
+	set_irq_flags(virq, IRQF_VALID); /* kill me now */
+	return 0;
+}
+
+static struct irq_domain_ops intc_irqpin_irq_domain_ops = {
+	.map	= intc_irqpin_irq_domain_map,
+};
+
+static int intc_irqpin_probe(struct platform_device *pdev)
+{
+	struct renesas_intc_irqpin_config *pdata = pdev->dev.platform_data;
+	struct intc_irqpin_priv *p;
+	struct intc_irqpin_iomem *i;
+	struct resource *io[INTC_IRQPIN_REG_NR];
+	struct resource *irq;
+	struct irq_chip *irq_chip;
+	void (*enable_fn)(struct irq_data *d);
+	void (*disable_fn)(struct irq_data *d);
+	const char *name = dev_name(&pdev->dev);
+	int ret;
+	int k;
+
+	p = kzalloc(sizeof(*p), GFP_KERNEL);
+	if (!p) {
+		dev_err(&pdev->dev, "failed to allocate driver data\n");
+		ret = -ENOMEM;
+		goto err0;
+	}
+
+	/* deal with driver instance configuration */
+	if (pdata)
+		memcpy(&p->config, pdata, sizeof(*pdata));
+	if (!p->config.sense_bitfield_width)
+		p->config.sense_bitfield_width = 4; /* default to 4 bits */
+
+	p->pdev = pdev;
+	platform_set_drvdata(pdev, p);
+
+	/* get hold of manadatory IOMEM */
+	for (k = 0; k < INTC_IRQPIN_REG_NR; k++) {
+		io[k] = platform_get_resource(pdev, IORESOURCE_MEM, k);
+		if (!io[k]) {
+			dev_err(&pdev->dev, "not enough IOMEM resources\n");
+			ret = -EINVAL;
+			goto err1;
+		}
+	}
+
+	/* allow any number of IRQs between 1 and INTC_IRQPIN_MAX */
+	for (k = 0; k < INTC_IRQPIN_MAX; k++) {
+		irq = platform_get_resource(pdev, IORESOURCE_IRQ, k);
+		if (!irq)
+			break;
+
+		p->irq[k].hw_irq = k;
+		p->irq[k].p = p;
+		p->irq[k].irq = irq->start;
+	}
+
+	p->number_of_irqs = k;
+	if (p->number_of_irqs < 1) {
+		dev_err(&pdev->dev, "not enough IRQ resources\n");
+		ret = -EINVAL;
+		goto err1;
+	}
+
+	/* ioremap IOMEM and setup read/write callbacks */
+	for (k = 0; k < INTC_IRQPIN_REG_NR; k++) {
+		i = &p->iomem[k];
+
+		switch (resource_size(io[k])) {
+		case 1:
+			i->width = 8;
+			i->read = intc_irqpin_read8;
+			i->write = intc_irqpin_write8;
+			break;
+		case 4:
+			i->width = 32;
+			i->read = intc_irqpin_read32;
+			i->write = intc_irqpin_write32;
+			break;
+		default:
+			dev_err(&pdev->dev, "IOMEM size mismatch\n");
+			ret = -EINVAL;
+			goto err2;
+		}
+
+		i->iomem = ioremap_nocache(io[k]->start, resource_size(io[k]));
+		if (!i->iomem) {
+			dev_err(&pdev->dev, "failed to remap IOMEM\n");
+			ret = -ENXIO;
+			goto err2;
+		}
+	}
+
+	/* mask all interrupts using priority */
+	for (k = 0; k < p->number_of_irqs; k++)
+		intc_irqpin_mask_unmask_prio(p, k, 1);
+
+	/* use more severe masking method if requested */
+	if (p->config.control_parent) {
+		enable_fn = intc_irqpin_irq_enable_force;
+		disable_fn = intc_irqpin_irq_disable_force;
+	} else {
+		enable_fn = intc_irqpin_irq_enable;
+		disable_fn = intc_irqpin_irq_disable;
+	}
+
+	irq_chip = &p->irq_chip;
+	irq_chip->name = name;
+	irq_chip->irq_mask = disable_fn;
+	irq_chip->irq_unmask = enable_fn;
+	irq_chip->irq_enable = enable_fn;
+	irq_chip->irq_disable = disable_fn;
+	irq_chip->irq_set_type = intc_irqpin_irq_set_type;
+	irq_chip->flags	= IRQCHIP_SKIP_SET_WAKE;
+
+	p->irq_domain = irq_domain_add_simple(pdev->dev.of_node,
+					      p->number_of_irqs,
+					      p->config.irq_base,
+					      &intc_irqpin_irq_domain_ops, p);
+	if (!p->irq_domain) {
+		ret = -ENXIO;
+		dev_err(&pdev->dev, "cannot initialize irq domain\n");
+		goto err2;
+	}
+
+	/* request and set priority on interrupts one by one */
+	for (k = 0; k < p->number_of_irqs; k++) {
+		if (request_irq(p->irq[k].irq, intc_irqpin_irq_handler,
+				0, name, &p->irq[k])) {
+			dev_err(&pdev->dev, "failed to request low IRQ\n");
+			ret = -ENOENT;
+			goto err3;
+		}
+		intc_irqpin_mask_unmask_prio(p, k, 0);
+	}
+
+	dev_info(&pdev->dev, "driving %d irqs\n", p->number_of_irqs);
+
+	/* warn in case of mismatch if irq base is specified */
+	if (p->config.irq_base) {
+		k = irq_find_mapping(p->irq_domain, 0);
+		if (p->config.irq_base != k)
+			dev_warn(&pdev->dev, "irq base mismatch (%d/%d)\n",
+				 p->config.irq_base, k);
+	}
+	
+	return 0;
+
+err3:
+	for (; k >= 0; k--)
+		free_irq(p->irq[k - 1].irq, &p->irq[k - 1]);
+
+	irq_domain_remove(p->irq_domain);
+err2:
+	for (k = 0; k < INTC_IRQPIN_REG_NR; k++)
+		iounmap(p->iomem[k].iomem);
+err1:
+	kfree(p);
+err0:
+	return ret;
+}
+
+static int intc_irqpin_remove(struct platform_device *pdev)
+{
+	struct intc_irqpin_priv *p = platform_get_drvdata(pdev);
+	int k;
+
+	for (k = 0; k < p->number_of_irqs; k++)
+		free_irq(p->irq[k].irq, &p->irq[k]);
+
+	irq_domain_remove(p->irq_domain);
+
+	for (k = 0; k < INTC_IRQPIN_REG_NR; k++)
+		iounmap(p->iomem[k].iomem);
+
+	kfree(p);
+	return 0;
+}
+
+static struct platform_driver intc_irqpin_device_driver = {
+	.probe		= intc_irqpin_probe,
+	.remove		= intc_irqpin_remove,
+	.driver		= {
+		.name	= "renesas_intc_irqpin",
+	}
+};
+
+static int __init intc_irqpin_init(void)
+{
+	return platform_driver_register(&intc_irqpin_device_driver);
+}
+postcore_initcall(intc_irqpin_init);
+
+static void __exit intc_irqpin_exit(void)
+{
+	platform_driver_unregister(&intc_irqpin_device_driver);
+}
+module_exit(intc_irqpin_exit);
+
+MODULE_AUTHOR("Magnus Damm");
+MODULE_DESCRIPTION("Renesas INTC External IRQ Pin Driver");
+MODULE_LICENSE("GPL v2");
--- /dev/null
+++ work/include/linux/platform_data/irq-renesas-intc-irqpin.h	2013-02-18 18:28:24.000000000 +0900
@@ -0,0 +1,10 @@
+#ifndef __IRQ_RENESAS_INTC_IRQPIN_H__
+#define __IRQ_RENESAS_INTC_IRQPIN_H__
+
+struct renesas_intc_irqpin_config {
+	unsigned int sense_bitfield_width;
+	unsigned int irq_base;
+	bool control_parent;
+};
+
+#endif /* __IRQ_RENESAS_INTC_IRQPIN_H__ */

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Simon Horman]

On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> This patch adds a driver for external IRQ pins connected
> to the INTC block on recent SoCs from Renesas.
> 
> The INTC hardware block usually contains a rather wide
> range of features ranging from external IRQ pin handling
> to legacy interrupt controller support. On older SoCs
> the INTC is used as a general purpose interrupt controller
> both for external IRQ pins and on-chip devices.
> 
> On more recent ARM based SoCs with Cortex-A9 the main
> interrupt controller is the GIC, but IRQ trigger setup
> still need to happen in the INTC hardware block.
> 
> This driver implements the glue code needed to configure
> IRQ trigger and also handle mask/unmask and demux of
> external IRQ pins hooked up from the INTC to the GIC.
> 
> Tested on sh73a0 and r8a7779. The hardware varies quite
> a bit with SoC model, for instance register width and
> bitfield widths vary wildly. The driver requires one GIC
> SPI per external IRQ pin to operate.  Each driver instance
> will handle up to 8 external IRQ pins.
> 
> The SoCs using this driver are currently mainly used
> together with regular platform devices so this driver
> allows configuration via platform data to support things
> like static interrupt base address. DT support will
> be added incrementally in the not so distant future.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>

Hi Magnus, Hi all,

I do not expect this code to go through the renesas tree.  However, in
order to provide a basis for work on renesas SoCs I have added this patch
to the topic/intc-external-irq topic branch in the reneas tree on
kernel.org and merged it into topic/all+next.

In other words, I am not picking this series up to merge it or add it to
linux-next, rather I am storing it for reference.


In the course of adding the branch I noticed a 3 whitespace warnings
from git. I have highlighted them below.

> ---
> 
>  drivers/irqchip/Kconfig                               |    4 
>  drivers/irqchip/Makefile                              |    1 
>  drivers/irqchip/irq-renesas-intc-irqpin.c             |  464 +++++++++++++++++
>  include/linux/platform_data/irq-renesas-intc-irqpin.h |   10 
>  4 files changed, 479 insertions(+)
> 
> --- 0001/drivers/irqchip/Kconfig
> +++ work/drivers/irqchip/Kconfig	2013-02-18 18:28:22.000000000 +0900
> @@ -25,6 +25,10 @@ config ARM_VIC_NR
>  	  The maximum number of VICs available in the system, for
>  	  power management.
>  
> +config RENESAS_INTC_IRQPIN
> +	bool
> +	select IRQ_DOMAIN
> +
>  config VERSATILE_FPGA_IRQ
>  	bool
>  	select IRQ_DOMAIN
> --- 0001/drivers/irqchip/Makefile
> +++ work/drivers/irqchip/Makefile	2013-02-18 18:28:22.000000000 +0900
> @@ -5,4 +5,5 @@ obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi.o
>  obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
>  obj-$(CONFIG_ARM_GIC)			+= irq-gic.o
>  obj-$(CONFIG_ARM_VIC)			+= irq-vic.o
> +obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
>  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
> --- /dev/null
> +++ work/drivers/irqchip/irq-renesas-intc-irqpin.c	2013-02-18 21:06:32.000000000 +0900
> @@ -0,0 +1,464 @@
> +/*
> + * Renesas INTC External IRQ Pin Driver
> + *
> + *  Copyright (C) 2013 Magnus Damm
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License
> + *
> + * 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, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/interrupt.h>
> +#include <linux/ioport.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/irq-renesas-intc-irqpin.h>
> +
> +#define INTC_IRQPIN_MAX 8 /* maximum 8 interrupts per driver instance */
> +
> +#define INTC_IRQPIN_REG_SENSE 0 /* ICRn */
> +#define INTC_IRQPIN_REG_PRIO 1 /* INTPRInn */
> +#define INTC_IRQPIN_REG_SOURCE 2 /* INTREQnn */
> +#define INTC_IRQPIN_REG_MASK 3 /* INTMSKnn */
> +#define INTC_IRQPIN_REG_CLEAR 4 /* INTMSKCLRnn */
> +#define INTC_IRQPIN_REG_NR 5
> +
> +/* INTC external IRQ PIN hardware register access:
> + *
> + * SENSE is read-write 32-bit with 2-bits or 4-bits per IRQ (*)
> + * PRIO is read-write 32-bit with 4-bits per IRQ (**)
> + * SOURCE is read-only 32-bit or 8-bit with 1-bit per IRQ (***)
> + * MASK is write-only 32-bit or 8-bit with 1-bit per IRQ (***)
> + * CLEAR is write-only 32-bit or 8-bit with 1-bit per IRQ (***)
> + *
> + * (*) May be accessed by more than one driver instance - lock needed
> + * (**) Read-modify-write access by one driver instance - lock needed
> + * (***) Accessed by one driver instance only - no locking needed
> + */
> +
> +struct intc_irqpin_iomem {
> +	void __iomem *iomem;
> +	unsigned long (*read)(void __iomem *iomem);
> +	void (*write)(void __iomem *iomem, unsigned long data);
> +	int width;
> +};  

git tells me there is trailing whitespace here.

> +
> +struct intc_irqpin_irq {
> +	int hw_irq;
> +	int irq;
> +	struct intc_irqpin_priv *p;
> +};  

And here.

> +
> +struct intc_irqpin_priv {
> +	struct intc_irqpin_iomem iomem[INTC_IRQPIN_REG_NR];
> +	struct intc_irqpin_irq irq[INTC_IRQPIN_MAX];
> +	struct renesas_intc_irqpin_config config;
> +	unsigned int number_of_irqs;
> +	struct platform_device *pdev;
> +	struct irq_chip irq_chip;
> +	struct irq_domain *irq_domain;
> +};
> +
> +static unsigned long intc_irqpin_read32(void __iomem *iomem)
> +{
> +	return ioread32(iomem);
> +}
> +
> +static unsigned long intc_irqpin_read8(void __iomem *iomem)
> +{
> +	return ioread8(iomem);
> +}
> +
> +static void intc_irqpin_write32(void __iomem *iomem, unsigned long data)
> +{
> +	iowrite32(data, iomem);
> +}
> +
> +static void intc_irqpin_write8(void __iomem *iomem, unsigned long data)
> +{
> +	iowrite8(data, iomem);
> +}
> +
> +static inline unsigned long intc_irqpin_read(struct intc_irqpin_priv *p,
> +					     int reg)
> +{
> +	struct intc_irqpin_iomem *i = &p->iomem[reg];
> +	return i->read(i->iomem);
> +}
> +
> +static inline void intc_irqpin_write(struct intc_irqpin_priv *p,
> +				     int reg, unsigned long data)
> +{
> +	struct intc_irqpin_iomem *i = &p->iomem[reg];
> +	i->write(i->iomem, data);
> +}
> +
> +static inline unsigned long intc_irqpin_hwirq_mask(struct intc_irqpin_priv *p,
> +						   int reg, int hw_irq)
> +{
> +	return BIT((p->iomem[reg].width - 1) - hw_irq);
> +}
> +
> +static inline void intc_irqpin_irq_write_hwirq(struct intc_irqpin_priv *p,
> +					       int reg, int hw_irq)
> +{
> +	intc_irqpin_write(p, reg, intc_irqpin_hwirq_mask(p, reg, hw_irq));
> +}
> +
> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
> +
> +static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
> +					  int reg, int shift,
> +					  int width, int value)
> +{
> +	unsigned long flags;
> +	unsigned long tmp;
> +
> +	raw_spin_lock_irqsave(&intc_irqpin_lock, flags);
> +
> +	tmp = intc_irqpin_read(p, reg);
> +	tmp &= ~(((1 << width) - 1) << shift);
> +	tmp |= value << shift;
> +	intc_irqpin_write(p, reg, tmp);
> +
> +	raw_spin_unlock_irqrestore(&intc_irqpin_lock, flags);
> +}
> +
> +static void intc_irqpin_mask_unmask_prio(struct intc_irqpin_priv *p,
> +					 int irq, int do_mask)
> +{
> +	int bitfield_width = 4; /* PRIO assumed to have fixed bitfield width */
> +	int shift = (7 - irq) * bitfield_width; /* PRIO assumed to be 32-bit */
> +
> +	intc_irqpin_read_modify_write(p, INTC_IRQPIN_REG_PRIO,
> +				      shift, bitfield_width,
> +				      do_mask ? 0 : (1 << bitfield_width) - 1);
> +}
> +
> +static int intc_irqpin_set_sense(struct intc_irqpin_priv *p, int irq, int value)
> +{
> +	int bitfield_width = p->config.sense_bitfield_width;
> +	int shift = (7 - irq) * bitfield_width; /* SENSE assumed to be 32-bit */
> +
> +	dev_dbg(&p->pdev->dev, "sense irq = %d, mode = %d\n", irq, value);
> +
> +	if (value >= (1 << bitfield_width))
> +		return -EINVAL;
> +
> +	intc_irqpin_read_modify_write(p, INTC_IRQPIN_REG_SENSE, shift,
> +				      bitfield_width, value);
> +	return 0;
> +}
> +
> +static void intc_irqpin_dbg(struct intc_irqpin_irq *i, char *str)
> +{
> +	dev_dbg(&i->p->pdev->dev, "%s (%d:%d:%d)\n",
> +		str, i->irq, i->hw_irq,
> +		irq_find_mapping(i->p->irq_domain, i->hw_irq));
> +}
> +
> +static void intc_irqpin_irq_enable(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int hw_irq = irqd_to_hwirq(d);
> +
> +	intc_irqpin_dbg(&p->irq[hw_irq], "enable");
> +	intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_CLEAR, hw_irq);
> +}
> +
> +static void intc_irqpin_irq_disable(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int hw_irq = irqd_to_hwirq(d);
> +
> +	intc_irqpin_dbg(&p->irq[hw_irq], "disable");
> +	intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_MASK, hw_irq);
> +}
> +
> +static void intc_irqpin_irq_enable_force(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int irq = p->irq[irqd_to_hwirq(d)].irq;
> +
> +	intc_irqpin_irq_enable(d);
> +	irq_get_chip(irq)->irq_unmask(irq_get_irq_data(irq));
> +}
> +
> +static void intc_irqpin_irq_disable_force(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int irq = p->irq[irqd_to_hwirq(d)].irq;
> +
> +	irq_get_chip(irq)->irq_mask(irq_get_irq_data(irq));
> +	intc_irqpin_irq_disable(d);
> +}
> +
> +#define INTC_IRQ_SENSE_VALID 0x10
> +#define INTC_IRQ_SENSE(x) (x + INTC_IRQ_SENSE_VALID)
> +
> +static unsigned char intc_irqpin_sense[IRQ_TYPE_SENSE_MASK + 1] = {
> +	[IRQ_TYPE_EDGE_FALLING] = INTC_IRQ_SENSE(0x00),
> +	[IRQ_TYPE_EDGE_RISING] = INTC_IRQ_SENSE(0x01),
> +	[IRQ_TYPE_LEVEL_LOW] = INTC_IRQ_SENSE(0x02),
> +	[IRQ_TYPE_LEVEL_HIGH] = INTC_IRQ_SENSE(0x03),
> +	[IRQ_TYPE_EDGE_BOTH] = INTC_IRQ_SENSE(0x04),
> +};
> +
> +static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	unsigned char value = intc_irqpin_sense[type & IRQ_TYPE_SENSE_MASK];
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +
> +	if (!(value & INTC_IRQ_SENSE_VALID))
> +		return -EINVAL;
> +
> +	return intc_irqpin_set_sense(p, irqd_to_hwirq(d),
> +				     value ^ INTC_IRQ_SENSE_VALID);
> +}
> +
> +static irqreturn_t intc_irqpin_irq_handler(int irq, void *dev_id)
> +{
> +	struct intc_irqpin_irq *i = dev_id;
> +	struct intc_irqpin_priv *p = i->p;
> +	unsigned long bit;
> +
> +	intc_irqpin_dbg(i, "demux1");
> +	bit = intc_irqpin_hwirq_mask(p, INTC_IRQPIN_REG_SOURCE, i->hw_irq);
> +
> +	if (intc_irqpin_read(p, INTC_IRQPIN_REG_SOURCE) & bit) {
> +		intc_irqpin_write(p, INTC_IRQPIN_REG_SOURCE, ~bit);
> +		intc_irqpin_dbg(i, "demux2");
> +		generic_handle_irq(irq_find_mapping(p->irq_domain, i->hw_irq));
> +		return IRQ_HANDLED;
> +	}
> +	return IRQ_NONE;
> +}
> +
> +static int intc_irqpin_irq_domain_map(struct irq_domain *h, unsigned int virq,
> +				      irq_hw_number_t hw)
> +{
> +	struct intc_irqpin_priv *p = h->host_data;
> +
> +	intc_irqpin_dbg(&p->irq[hw], "map");
> +	irq_set_chip_data(virq, h->host_data);
> +	irq_set_chip_and_handler(virq, &p->irq_chip, handle_level_irq);
> +	set_irq_flags(virq, IRQF_VALID); /* kill me now */
> +	return 0;
> +}
> +
> +static struct irq_domain_ops intc_irqpin_irq_domain_ops = {
> +	.map	= intc_irqpin_irq_domain_map,
> +};
> +
> +static int intc_irqpin_probe(struct platform_device *pdev)
> +{
> +	struct renesas_intc_irqpin_config *pdata = pdev->dev.platform_data;
> +	struct intc_irqpin_priv *p;
> +	struct intc_irqpin_iomem *i;
> +	struct resource *io[INTC_IRQPIN_REG_NR];
> +	struct resource *irq;
> +	struct irq_chip *irq_chip;
> +	void (*enable_fn)(struct irq_data *d);
> +	void (*disable_fn)(struct irq_data *d);
> +	const char *name = dev_name(&pdev->dev);
> +	int ret;
> +	int k;
> +
> +	p = kzalloc(sizeof(*p), GFP_KERNEL);
> +	if (!p) {
> +		dev_err(&pdev->dev, "failed to allocate driver data\n");
> +		ret = -ENOMEM;
> +		goto err0;
> +	}
> +
> +	/* deal with driver instance configuration */
> +	if (pdata)
> +		memcpy(&p->config, pdata, sizeof(*pdata));
> +	if (!p->config.sense_bitfield_width)
> +		p->config.sense_bitfield_width = 4; /* default to 4 bits */
> +
> +	p->pdev = pdev;
> +	platform_set_drvdata(pdev, p);
> +
> +	/* get hold of manadatory IOMEM */
> +	for (k = 0; k < INTC_IRQPIN_REG_NR; k++) {
> +		io[k] = platform_get_resource(pdev, IORESOURCE_MEM, k);
> +		if (!io[k]) {
> +			dev_err(&pdev->dev, "not enough IOMEM resources\n");
> +			ret = -EINVAL;
> +			goto err1;
> +		}
> +	}
> +
> +	/* allow any number of IRQs between 1 and INTC_IRQPIN_MAX */
> +	for (k = 0; k < INTC_IRQPIN_MAX; k++) {
> +		irq = platform_get_resource(pdev, IORESOURCE_IRQ, k);
> +		if (!irq)
> +			break;
> +
> +		p->irq[k].hw_irq = k;
> +		p->irq[k].p = p;
> +		p->irq[k].irq = irq->start;
> +	}
> +
> +	p->number_of_irqs = k;
> +	if (p->number_of_irqs < 1) {
> +		dev_err(&pdev->dev, "not enough IRQ resources\n");
> +		ret = -EINVAL;
> +		goto err1;
> +	}
> +
> +	/* ioremap IOMEM and setup read/write callbacks */
> +	for (k = 0; k < INTC_IRQPIN_REG_NR; k++) {
> +		i = &p->iomem[k];
> +
> +		switch (resource_size(io[k])) {
> +		case 1:
> +			i->width = 8;
> +			i->read = intc_irqpin_read8;
> +			i->write = intc_irqpin_write8;
> +			break;
> +		case 4:
> +			i->width = 32;
> +			i->read = intc_irqpin_read32;
> +			i->write = intc_irqpin_write32;
> +			break;
> +		default:
> +			dev_err(&pdev->dev, "IOMEM size mismatch\n");
> +			ret = -EINVAL;
> +			goto err2;
> +		}
> +
> +		i->iomem = ioremap_nocache(io[k]->start, resource_size(io[k]));
> +		if (!i->iomem) {
> +			dev_err(&pdev->dev, "failed to remap IOMEM\n");
> +			ret = -ENXIO;
> +			goto err2;
> +		}
> +	}
> +
> +	/* mask all interrupts using priority */
> +	for (k = 0; k < p->number_of_irqs; k++)
> +		intc_irqpin_mask_unmask_prio(p, k, 1);
> +
> +	/* use more severe masking method if requested */
> +	if (p->config.control_parent) {
> +		enable_fn = intc_irqpin_irq_enable_force;
> +		disable_fn = intc_irqpin_irq_disable_force;
> +	} else {
> +		enable_fn = intc_irqpin_irq_enable;
> +		disable_fn = intc_irqpin_irq_disable;
> +	}
> +
> +	irq_chip = &p->irq_chip;
> +	irq_chip->name = name;
> +	irq_chip->irq_mask = disable_fn;
> +	irq_chip->irq_unmask = enable_fn;
> +	irq_chip->irq_enable = enable_fn;
> +	irq_chip->irq_disable = disable_fn;
> +	irq_chip->irq_set_type = intc_irqpin_irq_set_type;
> +	irq_chip->flags	= IRQCHIP_SKIP_SET_WAKE;
> +
> +	p->irq_domain = irq_domain_add_simple(pdev->dev.of_node,
> +					      p->number_of_irqs,
> +					      p->config.irq_base,
> +					      &intc_irqpin_irq_domain_ops, p);
> +	if (!p->irq_domain) {
> +		ret = -ENXIO;
> +		dev_err(&pdev->dev, "cannot initialize irq domain\n");
> +		goto err2;
> +	}
> +
> +	/* request and set priority on interrupts one by one */
> +	for (k = 0; k < p->number_of_irqs; k++) {
> +		if (request_irq(p->irq[k].irq, intc_irqpin_irq_handler,
> +				0, name, &p->irq[k])) {
> +			dev_err(&pdev->dev, "failed to request low IRQ\n");
> +			ret = -ENOENT;
> +			goto err3;
> +		}
> +		intc_irqpin_mask_unmask_prio(p, k, 0);
> +	}
> +
> +	dev_info(&pdev->dev, "driving %d irqs\n", p->number_of_irqs);
> +
> +	/* warn in case of mismatch if irq base is specified */
> +	if (p->config.irq_base) {
> +		k = irq_find_mapping(p->irq_domain, 0);
> +		if (p->config.irq_base != k)
> +			dev_warn(&pdev->dev, "irq base mismatch (%d/%d)\n",
> +				 p->config.irq_base, k);
> +	}
> +	

And here.

> +	return 0;
> +
> +err3:
> +	for (; k >= 0; k--)
> +		free_irq(p->irq[k - 1].irq, &p->irq[k - 1]);
> +
> +	irq_domain_remove(p->irq_domain);
> +err2:
> +	for (k = 0; k < INTC_IRQPIN_REG_NR; k++)
> +		iounmap(p->iomem[k].iomem);
> +err1:
> +	kfree(p);
> +err0:
> +	return ret;
> +}
> +
> +static int intc_irqpin_remove(struct platform_device *pdev)
> +{
> +	struct intc_irqpin_priv *p = platform_get_drvdata(pdev);
> +	int k;
> +
> +	for (k = 0; k < p->number_of_irqs; k++)
> +		free_irq(p->irq[k].irq, &p->irq[k]);
> +
> +	irq_domain_remove(p->irq_domain);
> +
> +	for (k = 0; k < INTC_IRQPIN_REG_NR; k++)
> +		iounmap(p->iomem[k].iomem);
> +
> +	kfree(p);
> +	return 0;
> +}
> +
> +static struct platform_driver intc_irqpin_device_driver = {
> +	.probe		= intc_irqpin_probe,
> +	.remove		= intc_irqpin_remove,
> +	.driver		= {
> +		.name	= "renesas_intc_irqpin",
> +	}
> +};
> +
> +static int __init intc_irqpin_init(void)
> +{
> +	return platform_driver_register(&intc_irqpin_device_driver);
> +}
> +postcore_initcall(intc_irqpin_init);
> +
> +static void __exit intc_irqpin_exit(void)
> +{
> +	platform_driver_unregister(&intc_irqpin_device_driver);
> +}
> +module_exit(intc_irqpin_exit);
> +
> +MODULE_AUTHOR("Magnus Damm");
> +MODULE_DESCRIPTION("Renesas INTC External IRQ Pin Driver");
> +MODULE_LICENSE("GPL v2");
> --- /dev/null
> +++ work/include/linux/platform_data/irq-renesas-intc-irqpin.h	2013-02-18 18:28:24.000000000 +0900
> @@ -0,0 +1,10 @@
> +#ifndef __IRQ_RENESAS_INTC_IRQPIN_H__
> +#define __IRQ_RENESAS_INTC_IRQPIN_H__
> +
> +struct renesas_intc_irqpin_config {
> +	unsigned int sense_bitfield_width;
> +	unsigned int irq_base;
> +	bool control_parent;
> +};
> +
> +#endif /* __IRQ_RENESAS_INTC_IRQPIN_H__ */
> 

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Kuninori Morimoto]

Hi Magnus

Thank you for this patch.
Small comment from me :)

> +struct intc_irqpin_priv {
> +	struct intc_irqpin_iomem iomem[INTC_IRQPIN_REG_NR];
> +	struct intc_irqpin_irq irq[INTC_IRQPIN_MAX];
> +	struct renesas_intc_irqpin_config config;
> +	unsigned int number_of_irqs;
> +	struct platform_device *pdev;
> +	struct irq_chip irq_chip;
> +	struct irq_domain *irq_domain;
> +};

(snip)

> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
> +
> +static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
> +					  int reg, int shift,
> +					  int width, int value)
> +{
> +	unsigned long flags;
> +	unsigned long tmp;
> +
> +	raw_spin_lock_irqsave(&intc_irqpin_lock, flags);
> +
> +	tmp = intc_irqpin_read(p, reg);
> +	tmp &= ~(((1 << width) - 1) << shift);
> +	tmp |= value << shift;
> +	intc_irqpin_write(p, reg, tmp);
> +
> +	raw_spin_unlock_irqrestore(&intc_irqpin_lock, flags);
> +}

It is possible to include this spin lock into priv ?
This local static spin lock seems not wrong, but looks strange ?

> +static int intc_irqpin_probe(struct platform_device *pdev)
> +{
> +	struct renesas_intc_irqpin_config *pdata = pdev->dev.platform_data;
> +	struct intc_irqpin_priv *p;
> +	struct intc_irqpin_iomem *i;
> +	struct resource *io[INTC_IRQPIN_REG_NR];
> +	struct resource *irq;
> +	struct irq_chip *irq_chip;
> +	void (*enable_fn)(struct irq_data *d);
> +	void (*disable_fn)(struct irq_data *d);
> +	const char *name = dev_name(&pdev->dev);
> +	int ret;
> +	int k;
> +
> +	p = kzalloc(sizeof(*p), GFP_KERNEL);

can you use devm_kzalloc() ?

> +	if (!p) {
> +		dev_err(&pdev->dev, "failed to allocate driver data\n");
> +		ret = -ENOMEM;
> +		goto err0;
> +	}
> +
> +	/* deal with driver instance configuration */
> +	if (pdata)
> +		memcpy(&p->config, pdata, sizeof(*pdata));
> +	if (!p->config.sense_bitfield_width)
> +		p->config.sense_bitfield_width = 4; /* default to 4 bits */
> +
> +	p->pdev = pdev;
> +	platform_set_drvdata(pdev, p);
> +
> +	/* get hold of manadatory IOMEM */
> +	for (k = 0; k < INTC_IRQPIN_REG_NR; k++) {
> +		io[k] = platform_get_resource(pdev, IORESOURCE_MEM, k);
> +		if (!io[k]) {
> +			dev_err(&pdev->dev, "not enough IOMEM resources\n");
> +			ret = -EINVAL;
> +			goto err1;
> +		}
> +	}
> +
> +	/* allow any number of IRQs between 1 and INTC_IRQPIN_MAX */
> +	for (k = 0; k < INTC_IRQPIN_MAX; k++) {
> +		irq = platform_get_resource(pdev, IORESOURCE_IRQ, k);
> +		if (!irq)
> +			break;
> +
> +		p->irq[k].hw_irq = k;
> +		p->irq[k].p = p;
> +		p->irq[k].irq = irq->start;
> +	}
> +
> +	p->number_of_irqs = k;
> +	if (p->number_of_irqs < 1) {
> +		dev_err(&pdev->dev, "not enough IRQ resources\n");
> +		ret = -EINVAL;
> +		goto err1;
> +	}
> +
> +	/* ioremap IOMEM and setup read/write callbacks */
> +	for (k = 0; k < INTC_IRQPIN_REG_NR; k++) {
> +		i = &p->iomem[k];
> +
> +		switch (resource_size(io[k])) {
> +		case 1:
> +			i->width = 8;
> +			i->read = intc_irqpin_read8;
> +			i->write = intc_irqpin_write8;
> +			break;
> +		case 4:
> +			i->width = 32;
> +			i->read = intc_irqpin_read32;
> +			i->write = intc_irqpin_write32;
> +			break;
> +		default:
> +			dev_err(&pdev->dev, "IOMEM size mismatch\n");
> +			ret = -EINVAL;
> +			goto err2;
> +		}
> +
> +		i->iomem = ioremap_nocache(io[k]->start, resource_size(io[k]));

devm_ioremap_nocache() or devm_request_and_ioremap()

> +		if (!i->iomem) {
> +			dev_err(&pdev->dev, "failed to remap IOMEM\n");
> +			ret = -ENXIO;
> +			goto err2;
> +		}
> +	}
> +
> +	/* mask all interrupts using priority */
> +	for (k = 0; k < p->number_of_irqs; k++)
> +		intc_irqpin_mask_unmask_prio(p, k, 1);
> +
> +	/* use more severe masking method if requested */
> +	if (p->config.control_parent) {
> +		enable_fn = intc_irqpin_irq_enable_force;
> +		disable_fn = intc_irqpin_irq_disable_force;
> +	} else {
> +		enable_fn = intc_irqpin_irq_enable;
> +		disable_fn = intc_irqpin_irq_disable;
> +	}
> +
> +	irq_chip = &p->irq_chip;
> +	irq_chip->name = name;
> +	irq_chip->irq_mask = disable_fn;
> +	irq_chip->irq_unmask = enable_fn;
> +	irq_chip->irq_enable = enable_fn;
> +	irq_chip->irq_disable = disable_fn;
> +	irq_chip->irq_set_type = intc_irqpin_irq_set_type;
> +	irq_chip->flags	= IRQCHIP_SKIP_SET_WAKE;
> +
> +	p->irq_domain = irq_domain_add_simple(pdev->dev.of_node,
> +					      p->number_of_irqs,
> +					      p->config.irq_base,
> +					      &intc_irqpin_irq_domain_ops, p);
> +	if (!p->irq_domain) {
> +		ret = -ENXIO;
> +		dev_err(&pdev->dev, "cannot initialize irq domain\n");
> +		goto err2;
> +	}
> +
> +	/* request and set priority on interrupts one by one */
> +	for (k = 0; k < p->number_of_irqs; k++) {
> +		if (request_irq(p->irq[k].irq, intc_irqpin_irq_handler,

Can you use devm_request_irq()

> +				0, name, &p->irq[k])) {
> +			dev_err(&pdev->dev, "failed to request low IRQ\n");
> +			ret = -ENOENT;
> +			goto err3;
> +		}
> +		intc_irqpin_mask_unmask_prio(p, k, 0);
> +	}
> +
> +	dev_info(&pdev->dev, "driving %d irqs\n", p->number_of_irqs);
> +
> +	/* warn in case of mismatch if irq base is specified */
> +	if (p->config.irq_base) {
> +		k = irq_find_mapping(p->irq_domain, 0);
> +		if (p->config.irq_base != k)
> +			dev_warn(&pdev->dev, "irq base mismatch (%d/%d)\n",
> +				 p->config.irq_base, k);
> +	}
> +	
> +	return 0;
> +
> +err3:
> +	for (; k >= 0; k--)
> +		free_irq(p->irq[k - 1].irq, &p->irq[k - 1]);
> +
> +	irq_domain_remove(p->irq_domain);
> +err2:
> +	for (k = 0; k < INTC_IRQPIN_REG_NR; k++)
> +		iounmap(p->iomem[k].iomem);
> +err1:
> +	kfree(p);

if you used devm_xxxx, you can remove kfree() / free_irq() / iounmap() here

> +err0:
> +	return ret;
> +}
> +
> +static int intc_irqpin_remove(struct platform_device *pdev)
> +{
> +	struct intc_irqpin_priv *p = platform_get_drvdata(pdev);
> +	int k;
> +
> +	for (k = 0; k < p->number_of_irqs; k++)
> +		free_irq(p->irq[k].irq, &p->irq[k]);
> +
> +	irq_domain_remove(p->irq_domain);
> +
> +	for (k = 0; k < INTC_IRQPIN_REG_NR; k++)
> +		iounmap(p->iomem[k].iomem);
> +
> +	kfree(p);
> +	return 0;
> +}

same as above

> --- /dev/null
> +++ work/include/linux/platform_data/irq-renesas-intc-irqpin.h	2013-02-18 18:28:24.000000000 +0900
> @@ -0,0 +1,10 @@
> +#ifndef __IRQ_RENESAS_INTC_IRQPIN_H__
> +#define __IRQ_RENESAS_INTC_IRQPIN_H__

GPL license signage ?

> +
> +struct renesas_intc_irqpin_config {
> +	unsigned int sense_bitfield_width;
> +	unsigned int irq_base;
> +	bool control_parent;
> +};
> +
> +#endif /* __IRQ_RENESAS_INTC_IRQPIN_H__ */


Best regards
---
Kuninori Morimoto

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Thomas Gleixner]

Magnus,

On Mon, 18 Feb 2013, Magnus Damm wrote:

> +static inline unsigned long intc_irqpin_read(struct intc_irqpin_priv *p,
> +					     int reg)
> +{
> +	struct intc_irqpin_iomem *i = &p->iomem[reg];

Newline between variable and code please.

> +	return i->read(i->iomem);
> +}
> +
> +static inline void intc_irqpin_write(struct intc_irqpin_priv *p,
> +				     int reg, unsigned long data)
> +{
> +	struct intc_irqpin_iomem *i = &p->iomem[reg];
> +	i->write(i->iomem, data);
> +}
> +
> +static inline unsigned long intc_irqpin_hwirq_mask(struct intc_irqpin_priv *p,
> +						   int reg, int hw_irq)
> +{
> +	return BIT((p->iomem[reg].width - 1) - hw_irq);
> +}
> +
> +static inline void intc_irqpin_irq_write_hwirq(struct intc_irqpin_priv *p,
> +					       int reg, int hw_irq)
> +{
> +	intc_irqpin_write(p, reg, intc_irqpin_hwirq_mask(p, reg, hw_irq));
> +}
> +
> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */

Shouldn't the lock be part of struct intc_irqpin_priv ?

> +static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
> +					  int reg, int shift,
> +					  int width, int value)
> +{
> +	unsigned long flags;
> +	unsigned long tmp;
> +
> +	raw_spin_lock_irqsave(&intc_irqpin_lock, flags);
> +static void intc_irqpin_dbg(struct intc_irqpin_irq *i, char *str)
> +{
> +	dev_dbg(&i->p->pdev->dev, "%s (%d:%d:%d)\n",
> +		str, i->irq, i->hw_irq,
> +		irq_find_mapping(i->p->irq_domain, i->hw_irq));

Do you really want to do a lookup for each debug invocation?

> +}
> +
> +static void intc_irqpin_irq_enable(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int hw_irq = irqd_to_hwirq(d);
> +
> +	intc_irqpin_dbg(&p->irq[hw_irq], "enable");
> +	intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_CLEAR, hw_irq);
> +}
> +
> +static void intc_irqpin_irq_disable(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int hw_irq = irqd_to_hwirq(d);
> +
> +	intc_irqpin_dbg(&p->irq[hw_irq], "disable");
> +	intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_MASK, hw_irq);
> +}
> +
> +static void intc_irqpin_irq_enable_force(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int irq = p->irq[irqd_to_hwirq(d)].irq;
> +
> +	intc_irqpin_irq_enable(d);
> +	irq_get_chip(irq)->irq_unmask(irq_get_irq_data(irq));
> +}
> +
> +static void intc_irqpin_irq_disable_force(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int irq = p->irq[irqd_to_hwirq(d)].irq;
> +
> +	irq_get_chip(irq)->irq_mask(irq_get_irq_data(irq));
> +	intc_irqpin_irq_disable(d);

Hmm. If I understand that code correctly, then the _force functions
are (un)masking another interrupt line. But this happens without
holding irq_desc[irq]->lock. Looks unsafe at least and if correct
wants a big fat comment.

> +}
> +
> +#define INTC_IRQ_SENSE_VALID 0x10
> +#define INTC_IRQ_SENSE(x) (x + INTC_IRQ_SENSE_VALID)
> +
> +static unsigned char intc_irqpin_sense[IRQ_TYPE_SENSE_MASK + 1] = {
> +	[IRQ_TYPE_EDGE_FALLING] = INTC_IRQ_SENSE(0x00),
> +	[IRQ_TYPE_EDGE_RISING] = INTC_IRQ_SENSE(0x01),
> +	[IRQ_TYPE_LEVEL_LOW] = INTC_IRQ_SENSE(0x02),
> +	[IRQ_TYPE_LEVEL_HIGH] = INTC_IRQ_SENSE(0x03),
> +	[IRQ_TYPE_EDGE_BOTH] = INTC_IRQ_SENSE(0x04),
> +};
> +
> +static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	unsigned char value = intc_irqpin_sense[type & IRQ_TYPE_SENSE_MASK];
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +
> +	if (!(value & INTC_IRQ_SENSE_VALID))
> +		return -EINVAL;
> +
> +	return intc_irqpin_set_sense(p, irqd_to_hwirq(d),
> +				     value ^ INTC_IRQ_SENSE_VALID);
> +}
> +
> +static irqreturn_t intc_irqpin_irq_handler(int irq, void *dev_id)
> +{
> +	struct intc_irqpin_irq *i = dev_id;
> +	struct intc_irqpin_priv *p = i->p;
> +	unsigned long bit;
> +
> +	intc_irqpin_dbg(i, "demux1");
> +	bit = intc_irqpin_hwirq_mask(p, INTC_IRQPIN_REG_SOURCE, i->hw_irq);
> +
> +	if (intc_irqpin_read(p, INTC_IRQPIN_REG_SOURCE) & bit) {
> +		intc_irqpin_write(p, INTC_IRQPIN_REG_SOURCE, ~bit);
> +		intc_irqpin_dbg(i, "demux2");
> +		generic_handle_irq(irq_find_mapping(p->irq_domain, i->hw_irq));

Shouldn't you cache that mapping value somewhere ?

> +		return IRQ_HANDLED;
> +	}
> +	return IRQ_NONE;
> +}
> +
> +static int intc_irqpin_irq_domain_map(struct irq_domain *h, unsigned int virq,
> +				      irq_hw_number_t hw)
> +{
> +	struct intc_irqpin_priv *p = h->host_data;
> +
> +	intc_irqpin_dbg(&p->irq[hw], "map");
> +	irq_set_chip_data(virq, h->host_data);
> +	irq_set_chip_and_handler(virq, &p->irq_chip, handle_level_irq);
> +	set_irq_flags(virq, IRQF_VALID); /* kill me now */

What needs to be killed? -ENOPARSE

Thanks,

	tglx

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Magnus Damm]

On Tue, Feb 19, 2013 at 10:03 AM, Simon Horman <horms@verge.net.au> wrote:
> On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> This patch adds a driver for external IRQ pins connected
>> to the INTC block on recent SoCs from Renesas.
>>
>> The INTC hardware block usually contains a rather wide
>> range of features ranging from external IRQ pin handling
>> to legacy interrupt controller support. On older SoCs
>> the INTC is used as a general purpose interrupt controller
>> both for external IRQ pins and on-chip devices.
>>
>> On more recent ARM based SoCs with Cortex-A9 the main
>> interrupt controller is the GIC, but IRQ trigger setup
>> still need to happen in the INTC hardware block.
>>
>> This driver implements the glue code needed to configure
>> IRQ trigger and also handle mask/unmask and demux of
>> external IRQ pins hooked up from the INTC to the GIC.
>>
>> Tested on sh73a0 and r8a7779. The hardware varies quite
>> a bit with SoC model, for instance register width and
>> bitfield widths vary wildly. The driver requires one GIC
>> SPI per external IRQ pin to operate.  Each driver instance
>> will handle up to 8 external IRQ pins.
>>
>> The SoCs using this driver are currently mainly used
>> together with regular platform devices so this driver
>> allows configuration via platform data to support things
>> like static interrupt base address. DT support will
>> be added incrementally in the not so distant future.
>>
>> Signed-off-by: Magnus Damm <damm@opensource.se>
>
> Hi Magnus, Hi all,
>
> I do not expect this code to go through the renesas tree.  However, in
> order to provide a basis for work on renesas SoCs I have added this patch
> to the topic/intc-external-irq topic branch in the reneas tree on
> kernel.org and merged it into topic/all+next.
>
> In other words, I am not picking this series up to merge it or add it to
> linux-next, rather I am storing it for reference.
>
>
> In the course of adding the branch I noticed a 3 whitespace warnings
> from git. I have highlighted them below.

Thanks Simon. I will fix those up in V2!

/ magnus

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Magnus Damm]

Hi Morimoto-san,

On Tue, Feb 19, 2013 at 10:04 AM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
> Hi Magnus
>
> Thank you for this patch.
> Small comment from me :)

Sure, thanks!

>> +struct intc_irqpin_priv {
>> +     struct intc_irqpin_iomem iomem[INTC_IRQPIN_REG_NR];
>> +     struct intc_irqpin_irq irq[INTC_IRQPIN_MAX];
>> +     struct renesas_intc_irqpin_config config;
>> +     unsigned int number_of_irqs;
>> +     struct platform_device *pdev;
>> +     struct irq_chip irq_chip;
>> +     struct irq_domain *irq_domain;
>> +};
>
> (snip)
>
>> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
>> +
>> +static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
>> +                                       int reg, int shift,
>> +                                       int width, int value)
>> +{
>> +     unsigned long flags;
>> +     unsigned long tmp;
>> +
>> +     raw_spin_lock_irqsave(&intc_irqpin_lock, flags);
>> +
>> +     tmp = intc_irqpin_read(p, reg);
>> +     tmp &= ~(((1 << width) - 1) << shift);
>> +     tmp |= value << shift;
>> +     intc_irqpin_write(p, reg, tmp);
>> +
>> +     raw_spin_unlock_irqrestore(&intc_irqpin_lock, flags);
>> +}
>
> It is possible to include this spin lock into priv ?
> This local static spin lock seems not wrong, but looks strange ?

Please see this comment (that you snipped out above):

+/* INTC external IRQ PIN hardware register access:
+ *
+ * SENSE is read-write 32-bit with 2-bits or 4-bits per IRQ (*)
+ * PRIO is read-write 32-bit with 4-bits per IRQ (**)
+ * SOURCE is read-only 32-bit or 8-bit with 1-bit per IRQ (***)
+ * MASK is write-only 32-bit or 8-bit with 1-bit per IRQ (***)
+ * CLEAR is write-only 32-bit or 8-bit with 1-bit per IRQ (***)
+ *
+ * (*) May be accessed by more than one driver instance - lock needed
+ * (**) Read-modify-write access by one driver instance - lock needed
+ * (***) Accessed by one driver instance only - no locking needed
+ */

Basically, the lock is used for SENSE and PRIO. SENSE may be shared
between multiple driver instances, so a lock in ->priv won't be
enough. PRIO may be locked using ->priv but both SENSE and PRIV are in
the slow path so I decided to handle both using a global lock.

>> +static int intc_irqpin_probe(struct platform_device *pdev)
>> +{
>> +     struct renesas_intc_irqpin_config *pdata = pdev->dev.platform_data;
>> +     struct intc_irqpin_priv *p;
>> +     struct intc_irqpin_iomem *i;
>> +     struct resource *io[INTC_IRQPIN_REG_NR];
>> +     struct resource *irq;
>> +     struct irq_chip *irq_chip;
>> +     void (*enable_fn)(struct irq_data *d);
>> +     void (*disable_fn)(struct irq_data *d);
>> +     const char *name = dev_name(&pdev->dev);
>> +     int ret;
>> +     int k;
>> +
>> +     p = kzalloc(sizeof(*p), GFP_KERNEL);
>
> can you use devm_kzalloc() ?
> devm_ioremap_nocache() or devm_request_and_ioremap()
> Can you use devm_request_irq()
> if you used devm_xxxx, you can remove kfree() / free_irq() / iounmap() here

I somehow knew that this would come up. Will give devm a go in V2 -
unless someone tells me such a change is going to make the driver more
painful to back port to LTSI-3.4.

>> --- /dev/null
>> +++ work/include/linux/platform_data/irq-renesas-intc-irqpin.h        2013-02-18 18:28:24.000000000 +0900
>> @@ -0,0 +1,10 @@
>> +#ifndef __IRQ_RENESAS_INTC_IRQPIN_H__
>> +#define __IRQ_RENESAS_INTC_IRQPIN_H__
>
> GPL license signage ?

Let me check how other files in include/linux/platform_data/ handles
this. I will follow the majority.

Thanks,

/ magnus

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Magnus Damm]

Hi Thomas,

Thanks for your help with the review!

On Tue, Feb 19, 2013 at 7:11 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Magnus,
>
> On Mon, 18 Feb 2013, Magnus Damm wrote:
>
>> +static inline unsigned long intc_irqpin_read(struct intc_irqpin_priv *p,
>> +                                          int reg)
>> +{
>> +     struct intc_irqpin_iomem *i = &p->iomem[reg];
>
> Newline between variable and code please.

Yes, I agree, will fix. I have been criticized for adding too many
newlines in the past, so I guess this is a good sign that I can flip
the other way now!

>> +     return i->read(i->iomem);
>> +}
>> +
>> +static inline void intc_irqpin_write(struct intc_irqpin_priv *p,
>> +                                  int reg, unsigned long data)
>> +{
>> +     struct intc_irqpin_iomem *i = &p->iomem[reg];
>> +     i->write(i->iomem, data);
>> +}
>> +
>> +static inline unsigned long intc_irqpin_hwirq_mask(struct intc_irqpin_priv *p,
>> +                                                int reg, int hw_irq)
>> +{
>> +     return BIT((p->iomem[reg].width - 1) - hw_irq);
>> +}
>> +
>> +static inline void intc_irqpin_irq_write_hwirq(struct intc_irqpin_priv *p,
>> +                                            int reg, int hw_irq)
>> +{
>> +     intc_irqpin_write(p, reg, intc_irqpin_hwirq_mask(p, reg, hw_irq));
>> +}
>> +
>> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
>
> Shouldn't the lock be part of struct intc_irqpin_priv ?

Good idea, but I need to lock access to the SENSE register against
multiple driver instances. This is not the case for PRIO. But because
both PRIO and SENSE are accessed in the slow path I decided to be lazy
and use the same way of locking to reduce the code size.

>> +static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
>> +                                       int reg, int shift,
>> +                                       int width, int value)
>> +{
>> +     unsigned long flags;
>> +     unsigned long tmp;
>> +
>> +     raw_spin_lock_irqsave(&intc_irqpin_lock, flags);
>> +static void intc_irqpin_dbg(struct intc_irqpin_irq *i, char *str)
>> +{
>> +     dev_dbg(&i->p->pdev->dev, "%s (%d:%d:%d)\n",
>> +             str, i->irq, i->hw_irq,
>> +             irq_find_mapping(i->p->irq_domain, i->hw_irq));
>
> Do you really want to do a lookup for each debug invocation?

Uhm, maybe no. I need to check if this compiles out in case of DEBUG=n.

>> +}
>> +
>> +static void intc_irqpin_irq_enable(struct irq_data *d)
>> +{
>> +     struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
>> +     int hw_irq = irqd_to_hwirq(d);
>> +
>> +     intc_irqpin_dbg(&p->irq[hw_irq], "enable");
>> +     intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_CLEAR, hw_irq);
>> +}
>> +
>> +static void intc_irqpin_irq_disable(struct irq_data *d)
>> +{
>> +     struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
>> +     int hw_irq = irqd_to_hwirq(d);
>> +
>> +     intc_irqpin_dbg(&p->irq[hw_irq], "disable");
>> +     intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_MASK, hw_irq);
>> +}
>> +
>> +static void intc_irqpin_irq_enable_force(struct irq_data *d)
>> +{
>> +     struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
>> +     int irq = p->irq[irqd_to_hwirq(d)].irq;
>> +
>> +     intc_irqpin_irq_enable(d);
>> +     irq_get_chip(irq)->irq_unmask(irq_get_irq_data(irq));
>> +}
>> +
>> +static void intc_irqpin_irq_disable_force(struct irq_data *d)
>> +{
>> +     struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
>> +     int irq = p->irq[irqd_to_hwirq(d)].irq;
>> +
>> +     irq_get_chip(irq)->irq_mask(irq_get_irq_data(irq));
>> +     intc_irqpin_irq_disable(d);
>
> Hmm. If I understand that code correctly, then the _force functions
> are (un)masking another interrupt line. But this happens without
> holding irq_desc[irq]->lock. Looks unsafe at least and if correct
> wants a big fat comment.

On some SoCs the masking for some IRQs seems busted, and the only sane
way to work around that is to (un)mask the parent interrupt. The
parent happens to be the GIC. I will look into how to add locking.

>> +}
>> +
>> +#define INTC_IRQ_SENSE_VALID 0x10
>> +#define INTC_IRQ_SENSE(x) (x + INTC_IRQ_SENSE_VALID)
>> +
>> +static unsigned char intc_irqpin_sense[IRQ_TYPE_SENSE_MASK + 1] = {
>> +     [IRQ_TYPE_EDGE_FALLING] = INTC_IRQ_SENSE(0x00),
>> +     [IRQ_TYPE_EDGE_RISING] = INTC_IRQ_SENSE(0x01),
>> +     [IRQ_TYPE_LEVEL_LOW] = INTC_IRQ_SENSE(0x02),
>> +     [IRQ_TYPE_LEVEL_HIGH] = INTC_IRQ_SENSE(0x03),
>> +     [IRQ_TYPE_EDGE_BOTH] = INTC_IRQ_SENSE(0x04),
>> +};
>> +
>> +static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type)
>> +{
>> +     unsigned char value = intc_irqpin_sense[type & IRQ_TYPE_SENSE_MASK];
>> +     struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
>> +
>> +     if (!(value & INTC_IRQ_SENSE_VALID))
>> +             return -EINVAL;
>> +
>> +     return intc_irqpin_set_sense(p, irqd_to_hwirq(d),
>> +                                  value ^ INTC_IRQ_SENSE_VALID);
>> +}
>> +
>> +static irqreturn_t intc_irqpin_irq_handler(int irq, void *dev_id)
>> +{
>> +     struct intc_irqpin_irq *i = dev_id;
>> +     struct intc_irqpin_priv *p = i->p;
>> +     unsigned long bit;
>> +
>> +     intc_irqpin_dbg(i, "demux1");
>> +     bit = intc_irqpin_hwirq_mask(p, INTC_IRQPIN_REG_SOURCE, i->hw_irq);
>> +
>> +     if (intc_irqpin_read(p, INTC_IRQPIN_REG_SOURCE) & bit) {
>> +             intc_irqpin_write(p, INTC_IRQPIN_REG_SOURCE, ~bit);
>> +             intc_irqpin_dbg(i, "demux2");
>> +             generic_handle_irq(irq_find_mapping(p->irq_domain, i->hw_irq));
>
> Shouldn't you cache that mapping value somewhere ?

That may speed things up, yes. Will do.

>> +             return IRQ_HANDLED;
>> +     }
>> +     return IRQ_NONE;
>> +}
>> +
>> +static int intc_irqpin_irq_domain_map(struct irq_domain *h, unsigned int virq,
>> +                                   irq_hw_number_t hw)
>> +{
>> +     struct intc_irqpin_priv *p = h->host_data;
>> +
>> +     intc_irqpin_dbg(&p->irq[hw], "map");
>> +     irq_set_chip_data(virq, h->host_data);
>> +     irq_set_chip_and_handler(virq, &p->irq_chip, handle_level_irq);
>> +     set_irq_flags(virq, IRQF_VALID); /* kill me now */
>
> What needs to be killed? -ENOPARSE

I'd like to not have to set this flag in my interrupt code.

I've written interrupt code on other architectures before, and from my
experience only ARM requires the IRQF_VALID flag to be set because the
ARM architecture software believes it is a special case. I may be
behind times - I have to admit that I've not checked the latest state
- this flag may not be needed anymore, hurray if so - but at least a
couple of years ago it was needed in case of ARM for our shared INTC
code (shared between sh and ARM).

What is your opinion about this matter?

Thanks,

/ magnus

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Thomas Gleixner]

On Tue, 19 Feb 2013, Magnus Damm wrote:
> On Tue, Feb 19, 2013 at 7:11 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
> >
> > Shouldn't the lock be part of struct intc_irqpin_priv ?
> 
> Good idea, but I need to lock access to the SENSE register against
> multiple driver instances. This is not the case for PRIO. But because
> both PRIO and SENSE are accessed in the slow path I decided to be lazy
> and use the same way of locking to reduce the code size.

Fair enough.

> >> +static void intc_irqpin_irq_enable_force(struct irq_data *d)
> >> +{
> >> +     struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> >> +     int irq = p->irq[irqd_to_hwirq(d)].irq;
> >> +
> >> +     intc_irqpin_irq_enable(d);
> >> +     irq_get_chip(irq)->irq_unmask(irq_get_irq_data(irq));
> >> +}
> >> +
> >> +static void intc_irqpin_irq_disable_force(struct irq_data *d)
> >> +{
> >> +     struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> >> +     int irq = p->irq[irqd_to_hwirq(d)].irq;
> >> +
> >> +     irq_get_chip(irq)->irq_mask(irq_get_irq_data(irq));
> >> +     intc_irqpin_irq_disable(d);
> >
> > Hmm. If I understand that code correctly, then the _force functions
> > are (un)masking another interrupt line. But this happens without
> > holding irq_desc[irq]->lock. Looks unsafe at least and if correct
> > wants a big fat comment.
> 
> On some SoCs the masking for some IRQs seems busted, and the only sane
> way to work around that is to (un)mask the parent interrupt. The
> parent happens to be the GIC. I will look into how to add locking.

Is there a 1:1 relationship between the intc interrupt and the GIC? If
yes and if nothing else fiddles with that particular GIC interrupt,
then you might get away without extra locking, but that wants a
comment at least.
 
> >> +     irq_set_chip_and_handler(virq, &p->irq_chip, handle_level_irq);
> >> +     set_irq_flags(virq, IRQF_VALID); /* kill me now */
> >
> > What needs to be killed? -ENOPARSE
> 
> I'd like to not have to set this flag in my interrupt code.

Ah.
 
> I've written interrupt code on other architectures before, and from my
> experience only ARM requires the IRQF_VALID flag to be set because the
> ARM architecture software believes it is a special case. I may be
> behind times - I have to admit that I've not checked the latest state
> - this flag may not be needed anymore, hurray if so - but at least a
> couple of years ago it was needed in case of ARM for our shared INTC
> code (shared between sh and ARM).
> 
> What is your opinion about this matter?

It provides an extra paranoia level, but I'm not sure if it's really
worth the trouble.

Thanks,

	tglx

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Paul Mundt]

On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> This patch adds a driver for external IRQ pins connected
> to the INTC block on recent SoCs from Renesas.
> 
So how exactly does this interact with the existing sh_intc code? Or is
there some reason why you have opted to bypass it in order to implement a
simplified reduced-functionality version of INTC support focused only on
external pins? If both are used together this is going to be a nightmare
for locking, and it's also non-obvious how the IRQ domains on both sides
will interact.

This needs a lot more explanation.

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Magnus Damm]

On Wed, Feb 27, 2013 at 5:23 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> This patch adds a driver for external IRQ pins connected
>> to the INTC block on recent SoCs from Renesas.
>>
> So how exactly does this interact with the existing sh_intc code? Or is
> there some reason why you have opted to bypass it in order to implement a
> simplified reduced-functionality version of INTC support focused only on
> external pins? If both are used together this is going to be a nightmare
> for locking, and it's also non-obvious how the IRQ domains on both sides
> will interact.
>
> This needs a lot more explanation.

Recent GIC-based SoCs do not make use of INTC for any on-chip I/O
devices. This driver is meant to be used as a layer between the actual
IRQ pin and the GIC. Anything else needs the full driver. The existing
non-DT INTC driver can happily coexist with this driver like it does
in the case of sh73a0 here:

[PATCH 02/03] ARM: shmobile: INTC External IRQ pin driver on sh73a0

The driver is not meant to be used with INTC-only based systems like
sh7372 and the SH architecture. I would be very happy if someone could
get their shit together and fix up DT support for the common INTC
code. This has not happened yet though. So if you know anyone with
time to spare then feel free to suggest them to work together with
Iwamatsu-san to get the DT version of the code reviewed together with
Linaro.

Thanks,

/ magnus

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Paul Mundt]

On Wed, Feb 27, 2013 at 05:35:51PM +0900, Magnus Damm wrote:
> On Wed, Feb 27, 2013 at 5:23 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> > So how exactly does this interact with the existing sh_intc code? Or is
> > there some reason why you have opted to bypass it in order to implement a
> > simplified reduced-functionality version of INTC support focused only on
> > external pins? If both are used together this is going to be a nightmare
> > for locking, and it's also non-obvious how the IRQ domains on both sides
> > will interact.
> >
> > This needs a lot more explanation.
> 
> Recent GIC-based SoCs do not make use of INTC for any on-chip I/O
> devices. This driver is meant to be used as a layer between the actual
> IRQ pin and the GIC. Anything else needs the full driver. The existing
> non-DT INTC driver can happily coexist with this driver like it does
> in the case of sh73a0 here:
> 
> [PATCH 02/03] ARM: shmobile: INTC External IRQ pin driver on sh73a0
> 
Ok, thanks for clarifying.

I suppose the main concern is how quickly this will simply turn in to a
deviated partial implementation of the full driver as newer SoCs begin
deviating from your simplified case, and we basically end up
reimplementing sh_intc anyways.

> The driver is not meant to be used with INTC-only based systems like
> sh7372 and the SH architecture. I would be very happy if someone could
> get their shit together and fix up DT support for the common INTC
> code. This has not happened yet though. So if you know anyone with
> time to spare then feel free to suggest them to work together with
> Iwamatsu-san to get the DT version of the code reviewed together with
> Linaro.
> 
I haven't heard or seen anything new on that in some time, so I assumed
the work had stalled. I'm not sure why there wasn't more effort put in to
DT support for the INTC code rather than simply coming up with a
temporary bypass shim, and I'm not sure why you think this work is
blocked by anyone (unless you're just referring to a general lack of
resources).

In any event, I'm not sure what the best option for the interim is. I
suppose we can merge the irqchips until the INTC stuff catches up, but
then we are probaby going to run in to a situation where they either have
to co-exist, or the irqchips are removed and the sh_intc code has to
carry a compat shim to deal with those DT bindings. Neither of those
options seem particularly appealing to me.

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Magnus Damm]

On Wed, Feb 27, 2013 at 5:52 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Wed, Feb 27, 2013 at 05:35:51PM +0900, Magnus Damm wrote:
>> On Wed, Feb 27, 2013 at 5:23 PM, Paul Mundt <lethal@linux-sh.org> wrote:
>> > So how exactly does this interact with the existing sh_intc code? Or is
>> > there some reason why you have opted to bypass it in order to implement a
>> > simplified reduced-functionality version of INTC support focused only on
>> > external pins? If both are used together this is going to be a nightmare
>> > for locking, and it's also non-obvious how the IRQ domains on both sides
>> > will interact.
>> >
>> > This needs a lot more explanation.
>>
>> Recent GIC-based SoCs do not make use of INTC for any on-chip I/O
>> devices. This driver is meant to be used as a layer between the actual
>> IRQ pin and the GIC. Anything else needs the full driver. The existing
>> non-DT INTC driver can happily coexist with this driver like it does
>> in the case of sh73a0 here:
>>
>> [PATCH 02/03] ARM: shmobile: INTC External IRQ pin driver on sh73a0
>>
> Ok, thanks for clarifying.
>
> I suppose the main concern is how quickly this will simply turn in to a
> deviated partial implementation of the full driver as newer SoCs begin
> deviating from your simplified case, and we basically end up
> reimplementing sh_intc anyways.

As you know, the INTC code that you are referring to is a full
interrupt controller designed to work directly with CPU cores like SH
and ARM. Newer ARM cores like Cortex-A9 all include the GIC both for
IPI purpose in case of SMP and they also implement SPI functionality
for I/O device interrupts. So the need for vendor-specific interrupt
controller IP is almost down to nothing these days.

With that in mind I do really doubt that we end up reimplementing
sh_intc. The upcoming designs that I am aware of do not change their
external IRQ pin hardware to force us to update this driver. What
happens after that I'm afraid I can't say. =)

>> The driver is not meant to be used with INTC-only based systems like
>> sh7372 and the SH architecture. I would be very happy if someone could
>> get their shit together and fix up DT support for the common INTC
>> code. This has not happened yet though. So if you know anyone with
>> time to spare then feel free to suggest them to work together with
>> Iwamatsu-san to get the DT version of the code reviewed together with
>> Linaro.
>>
> I haven't heard or seen anything new on that in some time, so I assumed
> the work had stalled. I'm not sure why there wasn't more effort put in to
> DT support for the INTC code rather than simply coming up with a
> temporary bypass shim, and I'm not sure why you think this work is
> blocked by anyone (unless you're just referring to a general lack of
> resources).

I mainly wrote this driver to support the r8a7779 SoC which can't be
driven by the existing INTC code base. During development I decided to
try to share the driver between multiple GIC-based SoCs like r8a7779
and sh73a0. The reason behind trying to share this driver between
multiple SoCs is to remove SoC-specific hacks that can't be dealt with
by the existing INTC code.

> In any event, I'm not sure what the best option for the interim is. I
> suppose we can merge the irqchips until the INTC stuff catches up, but
> then we are probaby going to run in to a situation where they either have
> to co-exist, or the irqchips are removed and the sh_intc code has to
> carry a compat shim to deal with those DT bindings. Neither of those
> options seem particularly appealing to me.

I don't really understand why you would want to use a generic table
driven driver when you have the possibility of using a
hardware-specific driver. I suppose you are wondering why no one seems
to be working on INTC DT support at this point. The truth is that we
got very little feedback and development support with interrupts in
general last autumn and on top of that our developers went their own
way instead of following advice.

Anyway, I do understand your worry about what happens if the hardware
starts to deviate, but judging by the future devices that I've seen we
should be ok for a while. Beyond that no one can tell.

Thanks,

/ magnus

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Paul Mundt]

On Wed, Feb 27, 2013 at 06:52:51PM +0900, Magnus Damm wrote:
> As you know, the INTC code that you are referring to is a full
> interrupt controller designed to work directly with CPU cores like SH
> and ARM. Newer ARM cores like Cortex-A9 all include the GIC both for
> IPI purpose in case of SMP and they also implement SPI functionality
> for I/O device interrupts. So the need for vendor-specific interrupt
> controller IP is almost down to nothing these days.
> 
Yes, I'm aware of that.

> With that in mind I do really doubt that we end up reimplementing
> sh_intc. The upcoming designs that I am aware of do not change their
> external IRQ pin hardware to force us to update this driver. What
> happens after that I'm afraid I can't say. =)
> 
While I don't expect you would end up with a full reimplmentation, my
concern is more that it would just be reproducing stuff that's already in
place. IOW, perhaps it's less work to put sh_intc on a diet, and tie the
core functionality that you need for external IRQ pins in to an irqchip
there -- with the core of the old code adapted in to some sort of common
base library, rather than coming up with a new lightweight irqchip driver
and having to incrementally pile stuff on top of it later.

> I mainly wrote this driver to support the r8a7779 SoC which can't be
> driven by the existing INTC code base. During development I decided to
> try to share the driver between multiple GIC-based SoCs like r8a7779
> and sh73a0. The reason behind trying to share this driver between
> multiple SoCs is to remove SoC-specific hacks that can't be dealt with
> by the existing INTC code.
> 
Ok, fair enough.

> I don't really understand why you would want to use a generic table
> driven driver when you have the possibility of using a
> hardware-specific driver.

For the same reason sh_intc was written in the first place, every CPU
subtype more or less had a similar set of interrupt controllers with
minor deviations. Those deviations were sufficient enough to make the
code pretty hairy in places, and what we have now is really more of a
subsystem than an irqchip driver.

Having to reproduce 90% of the code when you're adding new CPUs at the
rate of 2 a month hardly makes an SoC-specific driver realistic,
especially as you just end up with identical features being broken in
subtly differnt ways on different SoCs. That being said, a lot of that is
legacy stuff and a result of no CPU team talking to any other CPU team
ever, and it seems like things have improved enough in that regard that
perhaps a hardware-specific driver won't be a complete throw-away
disaster like it was before. It's a risk either way, I just hope your
lightweight solution remains lightweight and consistent long enough that
we don't have multiple copies of slightly different drivers doing exactly
same thing spiralling out of control and turning in to a maintenance
nightmare.

If sh_intc doesn't deal with the needs of the new GIC-backed SoCs then
that's of course something that has to be addressed regardless (whether
that be hacking up sh_intc or adding new hardware-specific irqchips).

> I suppose you are wondering why no one seems to be working on INTC DT
> support at this point. The truth is that we got very little feedback
> and development support with interrupts in general last autumn and on
> top of that our developers went their own way instead of following
> advice.
> 
I assumed it was either being rewritten or had already been merged, so I
was simply surprised to hear otherwise. I will dig through the archives a
bit later and see about getting caught up.

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Simon Horman]

On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> This patch adds a driver for external IRQ pins connected
> to the INTC block on recent SoCs from Renesas.
> 
> The INTC hardware block usually contains a rather wide
> range of features ranging from external IRQ pin handling
> to legacy interrupt controller support. On older SoCs
> the INTC is used as a general purpose interrupt controller
> both for external IRQ pins and on-chip devices.
> 
> On more recent ARM based SoCs with Cortex-A9 the main
> interrupt controller is the GIC, but IRQ trigger setup
> still need to happen in the INTC hardware block.
> 
> This driver implements the glue code needed to configure
> IRQ trigger and also handle mask/unmask and demux of
> external IRQ pins hooked up from the INTC to the GIC.
> 
> Tested on sh73a0 and r8a7779. The hardware varies quite
> a bit with SoC model, for instance register width and
> bitfield widths vary wildly. The driver requires one GIC
> SPI per external IRQ pin to operate.  Each driver instance
> will handle up to 8 external IRQ pins.
> 
> The SoCs using this driver are currently mainly used
> together with regular platform devices so this driver
> allows configuration via platform data to support things
> like static interrupt base address. DT support will
> be added incrementally in the not so distant future.

Hi Thomas,

I'm wondering how you would like to handle merging this driver.
I can think of three options but I'm sure there are others.

* You can take the patches (there is a follow-up series) yourself.
* I can prepare a pull request for you
* I can prepare a pull request for arm-soc with the shmobile patches that
  enable the driver on the r8a7779 and sh73a0.

The last option is possibly the easiest.
But in that case I'd appreciate an Ack from you on this patch.

> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
> 
>  drivers/irqchip/Kconfig                               |    4 
>  drivers/irqchip/Makefile                              |    1 
>  drivers/irqchip/irq-renesas-intc-irqpin.c             |  464 +++++++++++++++++
>  include/linux/platform_data/irq-renesas-intc-irqpin.h |   10 
>  4 files changed, 479 insertions(+)
> 
> --- 0001/drivers/irqchip/Kconfig
> +++ work/drivers/irqchip/Kconfig	2013-02-18 18:28:22.000000000 +0900
> @@ -25,6 +25,10 @@ config ARM_VIC_NR
>  	  The maximum number of VICs available in the system, for
>  	  power management.
>  
> +config RENESAS_INTC_IRQPIN
> +	bool
> +	select IRQ_DOMAIN
> +
>  config VERSATILE_FPGA_IRQ
>  	bool
>  	select IRQ_DOMAIN
> --- 0001/drivers/irqchip/Makefile
> +++ work/drivers/irqchip/Makefile	2013-02-18 18:28:22.000000000 +0900
> @@ -5,4 +5,5 @@ obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi.o
>  obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
>  obj-$(CONFIG_ARM_GIC)			+= irq-gic.o
>  obj-$(CONFIG_ARM_VIC)			+= irq-vic.o
> +obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
>  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
> --- /dev/null
> +++ work/drivers/irqchip/irq-renesas-intc-irqpin.c	2013-02-18 21:06:32.000000000 +0900
> @@ -0,0 +1,464 @@
> +/*
> + * Renesas INTC External IRQ Pin Driver
> + *
> + *  Copyright (C) 2013 Magnus Damm
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License
> + *
> + * 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, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/interrupt.h>
> +#include <linux/ioport.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/irq-renesas-intc-irqpin.h>
> +
> +#define INTC_IRQPIN_MAX 8 /* maximum 8 interrupts per driver instance */
> +
> +#define INTC_IRQPIN_REG_SENSE 0 /* ICRn */
> +#define INTC_IRQPIN_REG_PRIO 1 /* INTPRInn */
> +#define INTC_IRQPIN_REG_SOURCE 2 /* INTREQnn */
> +#define INTC_IRQPIN_REG_MASK 3 /* INTMSKnn */
> +#define INTC_IRQPIN_REG_CLEAR 4 /* INTMSKCLRnn */
> +#define INTC_IRQPIN_REG_NR 5
> +
> +/* INTC external IRQ PIN hardware register access:
> + *
> + * SENSE is read-write 32-bit with 2-bits or 4-bits per IRQ (*)
> + * PRIO is read-write 32-bit with 4-bits per IRQ (**)
> + * SOURCE is read-only 32-bit or 8-bit with 1-bit per IRQ (***)
> + * MASK is write-only 32-bit or 8-bit with 1-bit per IRQ (***)
> + * CLEAR is write-only 32-bit or 8-bit with 1-bit per IRQ (***)
> + *
> + * (*) May be accessed by more than one driver instance - lock needed
> + * (**) Read-modify-write access by one driver instance - lock needed
> + * (***) Accessed by one driver instance only - no locking needed
> + */
> +
> +struct intc_irqpin_iomem {
> +	void __iomem *iomem;
> +	unsigned long (*read)(void __iomem *iomem);
> +	void (*write)(void __iomem *iomem, unsigned long data);
> +	int width;
> +};  
> +
> +struct intc_irqpin_irq {
> +	int hw_irq;
> +	int irq;
> +	struct intc_irqpin_priv *p;
> +};  
> +
> +struct intc_irqpin_priv {
> +	struct intc_irqpin_iomem iomem[INTC_IRQPIN_REG_NR];
> +	struct intc_irqpin_irq irq[INTC_IRQPIN_MAX];
> +	struct renesas_intc_irqpin_config config;
> +	unsigned int number_of_irqs;
> +	struct platform_device *pdev;
> +	struct irq_chip irq_chip;
> +	struct irq_domain *irq_domain;
> +};
> +
> +static unsigned long intc_irqpin_read32(void __iomem *iomem)
> +{
> +	return ioread32(iomem);
> +}
> +
> +static unsigned long intc_irqpin_read8(void __iomem *iomem)
> +{
> +	return ioread8(iomem);
> +}
> +
> +static void intc_irqpin_write32(void __iomem *iomem, unsigned long data)
> +{
> +	iowrite32(data, iomem);
> +}
> +
> +static void intc_irqpin_write8(void __iomem *iomem, unsigned long data)
> +{
> +	iowrite8(data, iomem);
> +}
> +
> +static inline unsigned long intc_irqpin_read(struct intc_irqpin_priv *p,
> +					     int reg)
> +{
> +	struct intc_irqpin_iomem *i = &p->iomem[reg];
> +	return i->read(i->iomem);
> +}
> +
> +static inline void intc_irqpin_write(struct intc_irqpin_priv *p,
> +				     int reg, unsigned long data)
> +{
> +	struct intc_irqpin_iomem *i = &p->iomem[reg];
> +	i->write(i->iomem, data);
> +}
> +
> +static inline unsigned long intc_irqpin_hwirq_mask(struct intc_irqpin_priv *p,
> +						   int reg, int hw_irq)
> +{
> +	return BIT((p->iomem[reg].width - 1) - hw_irq);
> +}
> +
> +static inline void intc_irqpin_irq_write_hwirq(struct intc_irqpin_priv *p,
> +					       int reg, int hw_irq)
> +{
> +	intc_irqpin_write(p, reg, intc_irqpin_hwirq_mask(p, reg, hw_irq));
> +}
> +
> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
> +
> +static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
> +					  int reg, int shift,
> +					  int width, int value)
> +{
> +	unsigned long flags;
> +	unsigned long tmp;
> +
> +	raw_spin_lock_irqsave(&intc_irqpin_lock, flags);
> +
> +	tmp = intc_irqpin_read(p, reg);
> +	tmp &= ~(((1 << width) - 1) << shift);
> +	tmp |= value << shift;
> +	intc_irqpin_write(p, reg, tmp);
> +
> +	raw_spin_unlock_irqrestore(&intc_irqpin_lock, flags);
> +}
> +
> +static void intc_irqpin_mask_unmask_prio(struct intc_irqpin_priv *p,
> +					 int irq, int do_mask)
> +{
> +	int bitfield_width = 4; /* PRIO assumed to have fixed bitfield width */
> +	int shift = (7 - irq) * bitfield_width; /* PRIO assumed to be 32-bit */
> +
> +	intc_irqpin_read_modify_write(p, INTC_IRQPIN_REG_PRIO,
> +				      shift, bitfield_width,
> +				      do_mask ? 0 : (1 << bitfield_width) - 1);
> +}
> +
> +static int intc_irqpin_set_sense(struct intc_irqpin_priv *p, int irq, int value)
> +{
> +	int bitfield_width = p->config.sense_bitfield_width;
> +	int shift = (7 - irq) * bitfield_width; /* SENSE assumed to be 32-bit */
> +
> +	dev_dbg(&p->pdev->dev, "sense irq = %d, mode = %d\n", irq, value);
> +
> +	if (value >= (1 << bitfield_width))
> +		return -EINVAL;
> +
> +	intc_irqpin_read_modify_write(p, INTC_IRQPIN_REG_SENSE, shift,
> +				      bitfield_width, value);
> +	return 0;
> +}
> +
> +static void intc_irqpin_dbg(struct intc_irqpin_irq *i, char *str)
> +{
> +	dev_dbg(&i->p->pdev->dev, "%s (%d:%d:%d)\n",
> +		str, i->irq, i->hw_irq,
> +		irq_find_mapping(i->p->irq_domain, i->hw_irq));
> +}
> +
> +static void intc_irqpin_irq_enable(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int hw_irq = irqd_to_hwirq(d);
> +
> +	intc_irqpin_dbg(&p->irq[hw_irq], "enable");
> +	intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_CLEAR, hw_irq);
> +}
> +
> +static void intc_irqpin_irq_disable(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int hw_irq = irqd_to_hwirq(d);
> +
> +	intc_irqpin_dbg(&p->irq[hw_irq], "disable");
> +	intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_MASK, hw_irq);
> +}
> +
> +static void intc_irqpin_irq_enable_force(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int irq = p->irq[irqd_to_hwirq(d)].irq;
> +
> +	intc_irqpin_irq_enable(d);
> +	irq_get_chip(irq)->irq_unmask(irq_get_irq_data(irq));
> +}
> +
> +static void intc_irqpin_irq_disable_force(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int irq = p->irq[irqd_to_hwirq(d)].irq;
> +
> +	irq_get_chip(irq)->irq_mask(irq_get_irq_data(irq));
> +	intc_irqpin_irq_disable(d);
> +}
> +
> +#define INTC_IRQ_SENSE_VALID 0x10
> +#define INTC_IRQ_SENSE(x) (x + INTC_IRQ_SENSE_VALID)
> +
> +static unsigned char intc_irqpin_sense[IRQ_TYPE_SENSE_MASK + 1] = {
> +	[IRQ_TYPE_EDGE_FALLING] = INTC_IRQ_SENSE(0x00),
> +	[IRQ_TYPE_EDGE_RISING] = INTC_IRQ_SENSE(0x01),
> +	[IRQ_TYPE_LEVEL_LOW] = INTC_IRQ_SENSE(0x02),
> +	[IRQ_TYPE_LEVEL_HIGH] = INTC_IRQ_SENSE(0x03),
> +	[IRQ_TYPE_EDGE_BOTH] = INTC_IRQ_SENSE(0x04),
> +};
> +
> +static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	unsigned char value = intc_irqpin_sense[type & IRQ_TYPE_SENSE_MASK];
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +
> +	if (!(value & INTC_IRQ_SENSE_VALID))
> +		return -EINVAL;
> +
> +	return intc_irqpin_set_sense(p, irqd_to_hwirq(d),
> +				     value ^ INTC_IRQ_SENSE_VALID);
> +}
> +
> +static irqreturn_t intc_irqpin_irq_handler(int irq, void *dev_id)
> +{
> +	struct intc_irqpin_irq *i = dev_id;
> +	struct intc_irqpin_priv *p = i->p;
> +	unsigned long bit;
> +
> +	intc_irqpin_dbg(i, "demux1");
> +	bit = intc_irqpin_hwirq_mask(p, INTC_IRQPIN_REG_SOURCE, i->hw_irq);
> +
> +	if (intc_irqpin_read(p, INTC_IRQPIN_REG_SOURCE) & bit) {
> +		intc_irqpin_write(p, INTC_IRQPIN_REG_SOURCE, ~bit);
> +		intc_irqpin_dbg(i, "demux2");
> +		generic_handle_irq(irq_find_mapping(p->irq_domain, i->hw_irq));
> +		return IRQ_HANDLED;
> +	}
> +	return IRQ_NONE;
> +}
> +
> +static int intc_irqpin_irq_domain_map(struct irq_domain *h, unsigned int virq,
> +				      irq_hw_number_t hw)
> +{
> +	struct intc_irqpin_priv *p = h->host_data;
> +
> +	intc_irqpin_dbg(&p->irq[hw], "map");
> +	irq_set_chip_data(virq, h->host_data);
> +	irq_set_chip_and_handler(virq, &p->irq_chip, handle_level_irq);
> +	set_irq_flags(virq, IRQF_VALID); /* kill me now */
> +	return 0;
> +}
> +
> +static struct irq_domain_ops intc_irqpin_irq_domain_ops = {
> +	.map	= intc_irqpin_irq_domain_map,
> +};
> +
> +static int intc_irqpin_probe(struct platform_device *pdev)
> +{
> +	struct renesas_intc_irqpin_config *pdata = pdev->dev.platform_data;
> +	struct intc_irqpin_priv *p;
> +	struct intc_irqpin_iomem *i;
> +	struct resource *io[INTC_IRQPIN_REG_NR];
> +	struct resource *irq;
> +	struct irq_chip *irq_chip;
> +	void (*enable_fn)(struct irq_data *d);
> +	void (*disable_fn)(struct irq_data *d);
> +	const char *name = dev_name(&pdev->dev);
> +	int ret;
> +	int k;
> +
> +	p = kzalloc(sizeof(*p), GFP_KERNEL);
> +	if (!p) {
> +		dev_err(&pdev->dev, "failed to allocate driver data\n");
> +		ret = -ENOMEM;
> +		goto err0;
> +	}
> +
> +	/* deal with driver instance configuration */
> +	if (pdata)
> +		memcpy(&p->config, pdata, sizeof(*pdata));
> +	if (!p->config.sense_bitfield_width)
> +		p->config.sense_bitfield_width = 4; /* default to 4 bits */
> +
> +	p->pdev = pdev;
> +	platform_set_drvdata(pdev, p);
> +
> +	/* get hold of manadatory IOMEM */
> +	for (k = 0; k < INTC_IRQPIN_REG_NR; k++) {
> +		io[k] = platform_get_resource(pdev, IORESOURCE_MEM, k);
> +		if (!io[k]) {
> +			dev_err(&pdev->dev, "not enough IOMEM resources\n");
> +			ret = -EINVAL;
> +			goto err1;
> +		}
> +	}
> +
> +	/* allow any number of IRQs between 1 and INTC_IRQPIN_MAX */
> +	for (k = 0; k < INTC_IRQPIN_MAX; k++) {
> +		irq = platform_get_resource(pdev, IORESOURCE_IRQ, k);
> +		if (!irq)
> +			break;
> +
> +		p->irq[k].hw_irq = k;
> +		p->irq[k].p = p;
> +		p->irq[k].irq = irq->start;
> +	}
> +
> +	p->number_of_irqs = k;
> +	if (p->number_of_irqs < 1) {
> +		dev_err(&pdev->dev, "not enough IRQ resources\n");
> +		ret = -EINVAL;
> +		goto err1;
> +	}
> +
> +	/* ioremap IOMEM and setup read/write callbacks */
> +	for (k = 0; k < INTC_IRQPIN_REG_NR; k++) {
> +		i = &p->iomem[k];
> +
> +		switch (resource_size(io[k])) {
> +		case 1:
> +			i->width = 8;
> +			i->read = intc_irqpin_read8;
> +			i->write = intc_irqpin_write8;
> +			break;
> +		case 4:
> +			i->width = 32;
> +			i->read = intc_irqpin_read32;
> +			i->write = intc_irqpin_write32;
> +			break;
> +		default:
> +			dev_err(&pdev->dev, "IOMEM size mismatch\n");
> +			ret = -EINVAL;
> +			goto err2;
> +		}
> +
> +		i->iomem = ioremap_nocache(io[k]->start, resource_size(io[k]));
> +		if (!i->iomem) {
> +			dev_err(&pdev->dev, "failed to remap IOMEM\n");
> +			ret = -ENXIO;
> +			goto err2;
> +		}
> +	}
> +
> +	/* mask all interrupts using priority */
> +	for (k = 0; k < p->number_of_irqs; k++)
> +		intc_irqpin_mask_unmask_prio(p, k, 1);
> +
> +	/* use more severe masking method if requested */
> +	if (p->config.control_parent) {
> +		enable_fn = intc_irqpin_irq_enable_force;
> +		disable_fn = intc_irqpin_irq_disable_force;
> +	} else {
> +		enable_fn = intc_irqpin_irq_enable;
> +		disable_fn = intc_irqpin_irq_disable;
> +	}
> +
> +	irq_chip = &p->irq_chip;
> +	irq_chip->name = name;
> +	irq_chip->irq_mask = disable_fn;
> +	irq_chip->irq_unmask = enable_fn;
> +	irq_chip->irq_enable = enable_fn;
> +	irq_chip->irq_disable = disable_fn;
> +	irq_chip->irq_set_type = intc_irqpin_irq_set_type;
> +	irq_chip->flags	= IRQCHIP_SKIP_SET_WAKE;
> +
> +	p->irq_domain = irq_domain_add_simple(pdev->dev.of_node,
> +					      p->number_of_irqs,
> +					      p->config.irq_base,
> +					      &intc_irqpin_irq_domain_ops, p);
> +	if (!p->irq_domain) {
> +		ret = -ENXIO;
> +		dev_err(&pdev->dev, "cannot initialize irq domain\n");
> +		goto err2;
> +	}
> +
> +	/* request and set priority on interrupts one by one */
> +	for (k = 0; k < p->number_of_irqs; k++) {
> +		if (request_irq(p->irq[k].irq, intc_irqpin_irq_handler,
> +				0, name, &p->irq[k])) {
> +			dev_err(&pdev->dev, "failed to request low IRQ\n");
> +			ret = -ENOENT;
> +			goto err3;
> +		}
> +		intc_irqpin_mask_unmask_prio(p, k, 0);
> +	}
> +
> +	dev_info(&pdev->dev, "driving %d irqs\n", p->number_of_irqs);
> +
> +	/* warn in case of mismatch if irq base is specified */
> +	if (p->config.irq_base) {
> +		k = irq_find_mapping(p->irq_domain, 0);
> +		if (p->config.irq_base != k)
> +			dev_warn(&pdev->dev, "irq base mismatch (%d/%d)\n",
> +				 p->config.irq_base, k);
> +	}
> +	
> +	return 0;
> +
> +err3:
> +	for (; k >= 0; k--)
> +		free_irq(p->irq[k - 1].irq, &p->irq[k - 1]);
> +
> +	irq_domain_remove(p->irq_domain);
> +err2:
> +	for (k = 0; k < INTC_IRQPIN_REG_NR; k++)
> +		iounmap(p->iomem[k].iomem);
> +err1:
> +	kfree(p);
> +err0:
> +	return ret;
> +}
> +
> +static int intc_irqpin_remove(struct platform_device *pdev)
> +{
> +	struct intc_irqpin_priv *p = platform_get_drvdata(pdev);
> +	int k;
> +
> +	for (k = 0; k < p->number_of_irqs; k++)
> +		free_irq(p->irq[k].irq, &p->irq[k]);
> +
> +	irq_domain_remove(p->irq_domain);
> +
> +	for (k = 0; k < INTC_IRQPIN_REG_NR; k++)
> +		iounmap(p->iomem[k].iomem);
> +
> +	kfree(p);
> +	return 0;
> +}
> +
> +static struct platform_driver intc_irqpin_device_driver = {
> +	.probe		= intc_irqpin_probe,
> +	.remove		= intc_irqpin_remove,
> +	.driver		= {
> +		.name	= "renesas_intc_irqpin",
> +	}
> +};
> +
> +static int __init intc_irqpin_init(void)
> +{
> +	return platform_driver_register(&intc_irqpin_device_driver);
> +}
> +postcore_initcall(intc_irqpin_init);
> +
> +static void __exit intc_irqpin_exit(void)
> +{
> +	platform_driver_unregister(&intc_irqpin_device_driver);
> +}
> +module_exit(intc_irqpin_exit);
> +
> +MODULE_AUTHOR("Magnus Damm");
> +MODULE_DESCRIPTION("Renesas INTC External IRQ Pin Driver");
> +MODULE_LICENSE("GPL v2");
> --- /dev/null
> +++ work/include/linux/platform_data/irq-renesas-intc-irqpin.h	2013-02-18 18:28:24.000000000 +0900
> @@ -0,0 +1,10 @@
> +#ifndef __IRQ_RENESAS_INTC_IRQPIN_H__
> +#define __IRQ_RENESAS_INTC_IRQPIN_H__
> +
> +struct renesas_intc_irqpin_config {
> +	unsigned int sense_bitfield_width;
> +	unsigned int irq_base;
> +	bool control_parent;
> +};
> +
> +#endif /* __IRQ_RENESAS_INTC_IRQPIN_H__ */
> 

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Magnus Damm]

On Wed, Feb 27, 2013 at 7:28 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Wed, Feb 27, 2013 at 06:52:51PM +0900, Magnus Damm wrote:
>> As you know, the INTC code that you are referring to is a full
>> interrupt controller designed to work directly with CPU cores like SH
>> and ARM. Newer ARM cores like Cortex-A9 all include the GIC both for
>> IPI purpose in case of SMP and they also implement SPI functionality
>> for I/O device interrupts. So the need for vendor-specific interrupt
>> controller IP is almost down to nothing these days.
>>
> Yes, I'm aware of that.

Good!

>> With that in mind I do really doubt that we end up reimplementing
>> sh_intc. The upcoming designs that I am aware of do not change their
>> external IRQ pin hardware to force us to update this driver. What
>> happens after that I'm afraid I can't say. =)
>>
> While I don't expect you would end up with a full reimplmentation, my
> concern is more that it would just be reproducing stuff that's already in
> place. IOW, perhaps it's less work to put sh_intc on a diet, and tie the
> core functionality that you need for external IRQ pins in to an irqchip
> there -- with the core of the old code adapted in to some sort of common
> base library, rather than coming up with a new lightweight irqchip driver
> and having to incrementally pile stuff on top of it later.

I'm afraid that I can't really see how we would want to pile that much
stuff on top of this driver in the future. Maybe we need to adjust it
slightly, but that should be fairly easy.

>From my point of view it all boils down to this for our ARM SoCs:
1) The driver is only suitable for older GIC-based designs coming from
the Renesas side.
2) GIC-based SoCs like EMEV2 coming from ex-NEC is using different IP
and different driver.
3) Older SoCs that do not make use of GIC are using the full blown
INTC implementation.

>> I mainly wrote this driver to support the r8a7779 SoC which can't be
>> driven by the existing INTC code base. During development I decided to
>> try to share the driver between multiple GIC-based SoCs like r8a7779
>> and sh73a0. The reason behind trying to share this driver between
>> multiple SoCs is to remove SoC-specific hacks that can't be dealt with
>> by the existing INTC code.
>>
> Ok, fair enough.
>
>> I don't really understand why you would want to use a generic table
>> driven driver when you have the possibility of using a
>> hardware-specific driver.
>
> For the same reason sh_intc was written in the first place, every CPU
> subtype more or less had a similar set of interrupt controllers with
> minor deviations. Those deviations were sufficient enough to make the
> code pretty hairy in places, and what we have now is really more of a
> subsystem than an irqchip driver.
>
> Having to reproduce 90% of the code when you're adding new CPUs at the
> rate of 2 a month hardly makes an SoC-specific driver realistic,
> especially as you just end up with identical features being broken in
> subtly differnt ways on different SoCs. That being said, a lot of that is
> legacy stuff and a result of no CPU team talking to any other CPU team
> ever, and it seems like things have improved enough in that regard that
> perhaps a hardware-specific driver won't be a complete throw-away
> disaster like it was before. It's a risk either way, I just hope your
> lightweight solution remains lightweight and consistent long enough that
> we don't have multiple copies of slightly different drivers doing exactly
> same thing spiralling out of control and turning in to a maintenance
> nightmare.

Yes, I agree about the history with zillions of different SoCs, and
the INTC and PFC design choice.

But regarding the external pins in case of GIC-based designs, this
seems to be something we can reuse as a regular driver.

> If sh_intc doesn't deal with the needs of the new GIC-backed SoCs then
> that's of course something that has to be addressed regardless (whether
> that be hacking up sh_intc or adding new hardware-specific irqchips).

Ignoring things won't work, agreed.

>> I suppose you are wondering why no one seems to be working on INTC DT
>> support at this point. The truth is that we got very little feedback
>> and development support with interrupts in general last autumn and on
>> top of that our developers went their own way instead of following
>> advice.
>>
> I assumed it was either being rewritten or had already been merged, so I
> was simply surprised to hear otherwise. I will dig through the archives a
> bit later and see about getting caught up.

Sounds good.

Thanks,

/ magnus

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Thomas Gleixner]

On Wed, 6 Mar 2013, Simon Horman wrote:
> On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote:
> > The SoCs using this driver are currently mainly used
> > together with regular platform devices so this driver
> > allows configuration via platform data to support things
> > like static interrupt base address. DT support will
> > be added incrementally in the not so distant future.
> 
> Hi Thomas,
> 
> I'm wondering how you would like to handle merging this driver.
> I can think of three options but I'm sure there are others.
> 
> * You can take the patches (there is a follow-up series) yourself.
> * I can prepare a pull request for you
> * I can prepare a pull request for arm-soc with the shmobile patches that
>   enable the driver on the r8a7779 and sh73a0.
> 
> The last option is possibly the easiest.

Correct.

> But in that case I'd appreciate an Ack from you on this patch.

You want to pick the V2 series, which already has my blessing:

    https://lkml.org/lkml/2013/2/26/305

For merging it through arm-soc you have my ack now :)

Thanks,

	tglx

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Simon Horman]

On Wed, Mar 06, 2013 at 11:01:14AM +0100, Thomas Gleixner wrote:
> On Wed, 6 Mar 2013, Simon Horman wrote:
> > On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote:
> > > The SoCs using this driver are currently mainly used
> > > together with regular platform devices so this driver
> > > allows configuration via platform data to support things
> > > like static interrupt base address. DT support will
> > > be added incrementally in the not so distant future.
> > 
> > Hi Thomas,
> > 
> > I'm wondering how you would like to handle merging this driver.
> > I can think of three options but I'm sure there are others.
> > 
> > * You can take the patches (there is a follow-up series) yourself.
> > * I can prepare a pull request for you
> > * I can prepare a pull request for arm-soc with the shmobile patches that
> >   enable the driver on the r8a7779 and sh73a0.
> > 
> > The last option is possibly the easiest.
> 
> Correct.
> 
> > But in that case I'd appreciate an Ack from you on this patch.
> 
> You want to pick the V2 series, which already has my blessing:
> 
>     https://lkml.org/lkml/2013/2/26/305
> 
> For merging it through arm-soc you have my ack now :)

Thanks, I have that.

It seems that V2 adds onto this patch rather than replaces it.
So could I also get an Ack for this patch too?

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Thomas Gleixner]

On Wed, 6 Mar 2013, Simon Horman wrote:

> On Wed, Mar 06, 2013 at 11:01:14AM +0100, Thomas Gleixner wrote:
> > On Wed, 6 Mar 2013, Simon Horman wrote:
> > > On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote:
> > > > The SoCs using this driver are currently mainly used
> > > > together with regular platform devices so this driver
> > > > allows configuration via platform data to support things
> > > > like static interrupt base address. DT support will
> > > > be added incrementally in the not so distant future.
> > > 
> > > Hi Thomas,
> > > 
> > > I'm wondering how you would like to handle merging this driver.
> > > I can think of three options but I'm sure there are others.
> > > 
> > > * You can take the patches (there is a follow-up series) yourself.
> > > * I can prepare a pull request for you
> > > * I can prepare a pull request for arm-soc with the shmobile patches that
> > >   enable the driver on the r8a7779 and sh73a0.
> > > 
> > > The last option is possibly the easiest.
> > 
> > Correct.
> > 
> > > But in that case I'd appreciate an Ack from you on this patch.
> > 
> > You want to pick the V2 series, which already has my blessing:
> > 
> >     https://lkml.org/lkml/2013/2/26/305
> > 
> > For merging it through arm-soc you have my ack now :)
> 
> Thanks, I have that.
> 
> It seems that V2 adds onto this patch rather than replaces it.
> So could I also get an Ack for this patch too?

Ah,. right. V2 is an incremental fix for V1. Yes, please add my Ack.

Thanks,

	tglx
 

Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Simon Horman]

On Wed, Mar 06, 2013 at 02:05:52PM +0100, Thomas Gleixner wrote:
> On Wed, 6 Mar 2013, Simon Horman wrote:
> 
> > On Wed, Mar 06, 2013 at 11:01:14AM +0100, Thomas Gleixner wrote:
> > > On Wed, 6 Mar 2013, Simon Horman wrote:
> > > > On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote:
> > > > > The SoCs using this driver are currently mainly used
> > > > > together with regular platform devices so this driver
> > > > > allows configuration via platform data to support things
> > > > > like static interrupt base address. DT support will
> > > > > be added incrementally in the not so distant future.
> > > > 
> > > > Hi Thomas,
> > > > 
> > > > I'm wondering how you would like to handle merging this driver.
> > > > I can think of three options but I'm sure there are others.
> > > > 
> > > > * You can take the patches (there is a follow-up series) yourself.
> > > > * I can prepare a pull request for you
> > > > * I can prepare a pull request for arm-soc with the shmobile patches that
> > > >   enable the driver on the r8a7779 and sh73a0.
> > > > 
> > > > The last option is possibly the easiest.
> > > 
> > > Correct.
> > > 
> > > > But in that case I'd appreciate an Ack from you on this patch.
> > > 
> > > You want to pick the V2 series, which already has my blessing:
> > > 
> > >     https://lkml.org/lkml/2013/2/26/305
> > > 
> > > For merging it through arm-soc you have my ack now :)
> > 
> > Thanks, I have that.
> > 
> > It seems that V2 adds onto this patch rather than replaces it.
> > So could I also get an Ack for this patch too?
> 
> Ah,. right. V2 is an incremental fix for V1. Yes, please add my Ack.

Thanks.

I have added this into a new intc-external-irq branch of
the renesas tree on kernel.org and thus queued it up for v3.10.

I have also added the following patches to that branch:

irqchip: irqc: Add DT support
irqchip: intc-irqpin: Initial DT support
ARM: shmobile: Make r8a7779 INTC irqpin platform data static
ARM: shmobile: Make sh73a0 INTC irqpin platform data static
irqchip: Renesas IRQC driver
irqchip: intc-irqpin: GPL header for platform data
irqchip: intc-irqpin: Make use of devm functions
irqchip: intc-irqpin: Add force comments
irqchip: intc-irqpin: Cache mapped IRQ
irqchip: intc-irqpin: Whitespace fixes
ARM: shmobile: INTC External IRQ pin driver on r8a7779
ARM: shmobile: INTC External IRQ pin driver on sh73a0
ARM: shmobile: irq_pin() for static IRQ pin assignment

The intc-external-irq branch is merged into the next branch and
I expect it to appear in linux-next in the not to distant future.

I have removed the topic/intc-external-irq branch from the reneas tree,
the branch where these patches were being staged.