pinctrl: meson: add support for GPIO IRQs

Add a driver supporting the GPIO interrupt controller on certain Amlogic meson SoC's.

Signed-off-by Heiner Kallweit <[email protected]>

---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used v6:
- change DT property parent-interrupts to interrupts v7:
- no changes
---

?drivers/irqchip/Kconfig??????????|???5 +
?drivers/irqchip/Makefile?????????|???1 +
?drivers/irqchip/irq-meson-gpio.c | 295

+++++++++++++++++++++++++++++++++++++++
?3 files changed, 301 insertions(+)
?create mode 100644 drivers/irqchip/irq-meson-gpio.c

drivers/irqchip/Kconfig +0 -0

index 478f8ace..bdc86e14 100644
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER

? help
? ??Say yes here to add support for the IRQ combiner devices embedded
? ??in Qualcomm Technologies chips.

drivers/irqchip/Kconfig +5 -0

+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y

drivers/irqchip/Makefile +0 -0

index b64c59b8..1be482bd 100644
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-

eznps.o
?obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o
?obj-$(CONFIG_STM32_EXTI)? += irq-stm32-exti.o
?obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o

drivers/irqchip/Makefile +1 -0

+obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o

drivers/irqchip/irq-meson- renamed from drivers/irqchip/irq-meson-gpio.c +0 -0

gpio.c
new file mode 100644
index 00000000..925d00c2

drivers/irqchip/irq-meson-gpio.c new file +26 -0

@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <[email protected]>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <[email protected]>
+ * Copyright (c) 2017 Heiner Kallweit <[email protected]>
+ *
+ * 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, version 2.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define REG_EDGE_POL		0x00
+#define REG_PIN_03_SEL		0x04
+#define REG_PIN_47_SEL		0x08

drivers/irqchip/irq-meson-gpio.c new file +61 -0

+
+struct meson_irq_slot {
+	unsigned int irq;
+	unsigned int owner;
+};
+
+struct meson_data {
+	struct regmap *regmap;
+	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
+	unsigned int num_slots;
+	struct mutex lock;
+};
+
+static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -ENOSPC;
+
+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (!md->slots[i].owner) {
+			md->slots[i].owner = virq;
+			slot = i;
+		}
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}
+
+static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i;
+
+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots; i++)
+		if (md->slots[i].owner == virq) {
+			md->slots[i].owner = 0;
+			break;
+		}
+
+	mutex_unlock(&md->lock);
+}
+
+static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -EINVAL;
+
+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (md->slots[i].owner == virq)
+			slot = i;
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}
+
+static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int

hwirq)

drivers/irqchip/irq-meson-gpio.c new file +46 -0

+{
+	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
+	int shift = 8 * (idx % 4);
+
+	regmap_update_bits(md->regmap, reg, 0xff << shift,
+			???hwirq << shift);
+}
+
+static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot = meson_find_irq_slot(md, data->irq);
+
+	if (slot >= 0)
+		meson_set_hwirq(md, slot, hwirq);
+}
+
+static int meson_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot;
+	unsigned int val = 0;
+
+	if (type == IRQ_TYPE_EDGE_BOTH)
+		return -EINVAL;
+
+	slot = meson_find_irq_slot(md, data->irq);
+	if (slot < 0)
+		return slot;
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val |= REG_EDGE_POL_EDGE(slot);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_LOW(slot);
+
+	regmap_update_bits(md->regmap, REG_EDGE_POL,
+			???REG_EDGE_POL_MASK(slot), val);
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val = IRQ_TYPE_EDGE_RISING;
+	else
+		val = IRQ_TYPE_LEVEL_HIGH;
+
+	return irq_chip_set_type_parent(data, val);
+}

drivers/irqchip/irq-meson-gpio.c new file +71 -0

+
+	return 0;
+}
+
+static void meson_irq_shutdown(struct irq_data *data)
+{
+	meson_irq_set_hwirq(data, 0xff);
+	irq_chip_mask_parent(data);
+}
+
+static struct irq_chip meson_irq_chip = {
+	.name = "meson_gpio_intc",
+	.irq_set_type = meson_irq_set_type,
+	.irq_eoi = irq_chip_eoi_parent,
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_startup = meson_irq_startup,
+	.irq_shutdown = meson_irq_shutdown,
+	.irq_set_affinity = irq_chip_set_affinity_parent,
+};
+
+static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
+			???unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct meson_data *md = d->host_data;
+	irq_hw_number_t hwirq;
+	int ret, slot;
+
+	slot = meson_alloc_irq_slot(md, virq);
+	if (slot < 0)
+		return slot;
+
+	hwirq = fwspec->param[0];
+	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
+
+	parent_fwspec.fwnode = d->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0] = 0; /* SPI */
+	parent_fwspec.param[1] = md->slots[slot].irq;
+	parent_fwspec.param[2] = IRQ_TYPE_NONE;
+
+	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
+	if (ret)
+		meson_free_irq_slot(md, virq);
+
+	return ret;
+}
+
+static void meson_irq_free(struct irq_domain *d, unsigned int virq,
+			???unsigned int nr_irqs)
+{
+	struct meson_data *md = d->host_data;
+
+	irq_domain_free_irqs_common(d, virq, nr_irqs);
+	meson_free_irq_slot(md, virq);
+}
+
+static const struct irq_domain_ops meson_irq_ops = {
+	.alloc = meson_irq_alloc,
+	.free = meson_irq_free,
+	.xlate = irq_domain_xlate_twocell,
+};
+
+static int meson_get_irqs(struct meson_data *md, struct device_node *node)
+{
+	int ret, i;
+	u32 irq;
+
+	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
+		ret = of_property_read_u32_index(node, "interrupts", i,

&irq);

drivers/irqchip/irq-meson-gpio.c new file +62 -0

+		if (ret)
+			break;
+		md->slots[i].irq = irq;
+	}
+
+	md->num_slots = i;
+
+	return i ? 0 : -EINVAL;
+}
+
+static const struct regmap_config meson_regmap_config = {
+	.reg_bits???????= 32,
+	.reg_stride?????= 4,
+	.val_bits???????= 32,
+	.max_register	= REG_FILTER_SEL,
+};
+
+static int __init meson_gpio_irq_init(struct device_node *node,
+				??????struct device_node *parent)
+{
+	struct irq_domain *meson_irq_domain, *parent_domain;
+	struct meson_data *md;
+	void __iomem *io_base;
+	int ret;
+
+	md = kzalloc(sizeof(*md), GFP_KERNEL);
+	if (!md)
+		return -ENOMEM;
+
+	mutex_init(&md->lock);
+
+	io_base = of_iomap(node, 0);
+	if (!io_base)
+		return -EINVAL;
+
+	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
+	if (IS_ERR(md->regmap))
+		return PTR_ERR(md->regmap);
+
+	/* initialize to IRQ_TYPE_LEVEL_HIGH */
+	regmap_write(md->regmap, REG_EDGE_POL, 0);
+	/* disable all GPIO interrupt sources */
+	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
+	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
+	/* disable filtering */
+	regmap_write(md->regmap, REG_FILTER_SEL, 0);
+
+	ret = meson_get_irqs(md, node);
+	if (ret)
+		return ret;
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain)
+		return -ENXIO;
+
+	meson_irq_domain = irq_domain_add_hierarchy(parent_domain, 0,
+						????NUM_GPIO_HWIRQ, node,
+						????&meson_irq_ops, md);
+	return meson_irq_domain ? 0 : -EINVAL;
+}
+
+IRQCHIP_DECLARE(meson_gpio_irq, "amlogic,meson-gpio-intc",

meson_gpio_irq_init);

Quoted 82 lines

Add a driver supporting the GPIO interrupt controller on certain Amlogic meson SoC's.

Signed-off-by Heiner Kallweit <[email protected]>

---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used v6:
- change DT property parent-interrupts to interrupts v7:
- no changes

---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295
+++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

drivers/irqchip/Kconfig +5 -0

index 478f8ace..bdc86e14 100644
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y

drivers/irqchip/Makefile +0 -0

index b64c59b8..1be482bd 100644
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-

eznps.o
obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o
obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o

drivers/irqchip/Makefile +1 -0

+obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o

drivers/irqchip/irq-meson- renamed from drivers/irqchip/irq-meson-gpio.c +0 -0

gpio.c
new file mode 100644
index 00000000..925d00c2

drivers/irqchip/irq-meson-gpio.c new file +31 -0

@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <[email protected]>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <[email protected]>
+ * Copyright (c) 2017 Heiner Kallweit <[email protected]>
+ *
+ * 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, version 2.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define REG_EDGE_POL		0x00
+#define REG_PIN_03_SEL		0x04
+#define REG_PIN_47_SEL		0x08
+#define REG_FILTER_SEL		0x0c
+
+#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
+#define REG_EDGE_POL_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))

drivers/irqchip/irq-meson-gpio.c line 36

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+
+#define MAX_PARENT_IRQ_NUM	8
+
+/* maximum number of GPIO IRQs on supported platforms */
+#define MAX_NUM_GPIO_IRQ	133

Comment on V6 still applies here. 133 being the max on gxbb does not make it the max on the other supported chipset.

Review from october suggested to put this value in DT.

drivers/irqchip/irq-meson-gpio.c line 45

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c new file +9 -0

+
+/*
+ * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
+ * edge. That's due to HW constraints.
+ * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
+ * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
+ * one bit to the right.
+ */
+#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)

????

Quoted 108 lines

drivers/irqchip/irq-meson-gpio.c new file +61 -0

+
+struct meson_irq_slot {
+	unsigned int irq;
+	unsigned int owner;
+};
+
+struct meson_data {
+	struct regmap *regmap;
+	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
+	unsigned int num_slots;
+	struct mutex lock;
+};
+
+static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -ENOSPC;
+
+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (!md->slots[i].owner) {
+			md->slots[i].owner = virq;
+			slot = i;
+		}
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}
+
+static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i;
+
+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots; i++)
+		if (md->slots[i].owner == virq) {
+			md->slots[i].owner = 0;
+			break;
+		}
+
+	mutex_unlock(&md->lock);
+}
+
+static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -EINVAL;
+
+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (md->slots[i].owner == virq)
+			slot = i;
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}
+
+static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int

hwirq)

drivers/irqchip/irq-meson-gpio.c new file +46 -0

+{
+	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
+	int shift = 8 * (idx % 4);
+
+	regmap_update_bits(md->regmap, reg, 0xff << shift,
+			   hwirq << shift);
+}
+
+static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot = meson_find_irq_slot(md, data->irq);
+
+	if (slot >= 0)
+		meson_set_hwirq(md, slot, hwirq);
+}
+
+static int meson_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot;
+	unsigned int val = 0;
+
+	if (type == IRQ_TYPE_EDGE_BOTH)
+		return -EINVAL;
+
+	slot = meson_find_irq_slot(md, data->irq);
+	if (slot < 0)
+		return slot;
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val |= REG_EDGE_POL_EDGE(slot);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_LOW(slot);
+
+	regmap_update_bits(md->regmap, REG_EDGE_POL,
+			   REG_EDGE_POL_MASK(slot), val);
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val = IRQ_TYPE_EDGE_RISING;
+	else
+		val = IRQ_TYPE_LEVEL_HIGH;
+
+	return irq_chip_set_type_parent(data, val);
+}

drivers/irqchip/irq-meson-gpio.c line 162

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+
+static unsigned int meson_irq_startup(struct irq_data *data)
+{
+	irq_chip_unmask_parent(data);
+	/*
+	 * An extra bit was added to allow having the same gpio hwirq twice
+	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
+	 * gpio hwirq.
+	 */
+	meson_irq_set_hwirq(data, data->hwirq >> 1);

Comments from v6 still applies here, and this has nothing to do with the so- called percentage of niceness - whatever this means.

This is an interrupt controller, a critical part of the system. It has be correct.

Multiplying the number of IRQ lines and so changing the interface is not correct.

DT is OS independent, property should not be os-specific, let alone driver- hacked specific.

Interfaces, including DT, are important. These are supposed to be stable. We don't to change the irq numbers in every dts because "yeahhh we found another way!"

So yes, the corner cases should be covered, what drives you is not the matter here. If you have a critical need for this, then feel free to apply the patch to your kernel, no one is stopping you. If it goes upstream, people should be able to understand it and trust it, identified corner cases should be covered.

Quoted 135 lines

drivers/irqchip/irq-meson-gpio.c new file +71 -0

+
+	return 0;
+}
+
+static void meson_irq_shutdown(struct irq_data *data)
+{
+	meson_irq_set_hwirq(data, 0xff);
+	irq_chip_mask_parent(data);
+}
+
+static struct irq_chip meson_irq_chip = {
+	.name = "meson_gpio_intc",
+	.irq_set_type = meson_irq_set_type,
+	.irq_eoi = irq_chip_eoi_parent,
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_startup = meson_irq_startup,
+	.irq_shutdown = meson_irq_shutdown,
+	.irq_set_affinity = irq_chip_set_affinity_parent,
+};
+
+static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct meson_data *md = d->host_data;
+	irq_hw_number_t hwirq;
+	int ret, slot;
+
+	slot = meson_alloc_irq_slot(md, virq);
+	if (slot < 0)
+		return slot;
+
+	hwirq = fwspec->param[0];
+	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
+
+	parent_fwspec.fwnode = d->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0] = 0; /* SPI */
+	parent_fwspec.param[1] = md->slots[slot].irq;
+	parent_fwspec.param[2] = IRQ_TYPE_NONE;
+
+	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
+	if (ret)
+		meson_free_irq_slot(md, virq);
+
+	return ret;
+}
+
+static void meson_irq_free(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs)
+{
+	struct meson_data *md = d->host_data;
+
+	irq_domain_free_irqs_common(d, virq, nr_irqs);
+	meson_free_irq_slot(md, virq);
+}
+
+static const struct irq_domain_ops meson_irq_ops = {
+	.alloc = meson_irq_alloc,
+	.free = meson_irq_free,
+	.xlate = irq_domain_xlate_twocell,
+};
+
+static int meson_get_irqs(struct meson_data *md, struct device_node *node)
+{
+	int ret, i;
+	u32 irq;
+
+	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
+		ret = of_property_read_u32_index(node, "interrupts", i,

&irq);

drivers/irqchip/irq-meson-gpio.c new file +62 -0

+		if (ret)
+			break;
+		md->slots[i].irq = irq;
+	}
+
+	md->num_slots = i;
+
+	return i ? 0 : -EINVAL;
+}
+
+static const struct regmap_config meson_regmap_config = {
+	.reg_bits       = 32,
+	.reg_stride     = 4,
+	.val_bits       = 32,
+	.max_register	= REG_FILTER_SEL,
+};
+
+static int __init meson_gpio_irq_init(struct device_node *node,
+				      struct device_node *parent)
+{
+	struct irq_domain *meson_irq_domain, *parent_domain;
+	struct meson_data *md;
+	void __iomem *io_base;
+	int ret;
+
+	md = kzalloc(sizeof(*md), GFP_KERNEL);
+	if (!md)
+		return -ENOMEM;
+
+	mutex_init(&md->lock);
+
+	io_base = of_iomap(node, 0);
+	if (!io_base)
+		return -EINVAL;
+
+	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
+	if (IS_ERR(md->regmap))
+		return PTR_ERR(md->regmap);
+
+	/* initialize to IRQ_TYPE_LEVEL_HIGH */
+	regmap_write(md->regmap, REG_EDGE_POL, 0);
+	/* disable all GPIO interrupt sources */
+	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
+	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
+	/* disable filtering */
+	regmap_write(md->regmap, REG_FILTER_SEL, 0);
+
+	ret = meson_get_irqs(md, node);
+	if (ret)
+		return ret;
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain)
+		return -ENXIO;
+
+	meson_irq_domain = irq_domain_add_hierarchy(parent_domain, 0,
+						    NUM_GPIO_HWIRQ, node,
+						    &meson_irq_ops, md);
+	return meson_irq_domain ? 0 : -EINVAL;
+}
+
+IRQCHIP_DECLARE(meson_gpio_irq, "amlogic,meson-gpio-intc",

meson_gpio_irq_init);

Add a driver supporting the GPIO interrupt controller on certain Amlogic meson SoC's.

Signed-off-by Heiner Kallweit <[email protected]>

---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used v6:
- change DT property parent-interrupts to interrupts v7:
- no changes

---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

drivers/irqchip/Kconfig +5 -0

index 478f8ace..bdc86e14 100644
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y

drivers/irqchip/Makefile +1 -0

index b64c59b8..1be482bd 100644
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o

drivers/irqchip/irq-meson-gpio.c new file +26 -0

new file mode 100644
index 00000000..925d00c2
@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <[email protected]>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <[email protected]>
+ * Copyright (c) 2017 Heiner Kallweit <[email protected]>
+ *
+ * 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, version 2.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define REG_EDGE_POL		0x00
+#define REG_PIN_03_SEL		0x04
+#define REG_PIN_47_SEL		0x08

drivers/irqchip/irq-meson-gpio.c new file +14 -0

+ */
+#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)
+
+struct meson_irq_slot {
+	unsigned int irq;
+	unsigned int owner;
+};
+
+struct meson_data {
+	struct regmap *regmap;
+	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
+	unsigned int num_slots;
+	struct mutex lock;
+};

drivers/irqchip/irq-meson-gpio.c new file +12 -0

+			slot = i;
+		}
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}
+
+static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i;
+

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+
+static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -EINVAL;
+

drivers/irqchip/irq-meson-gpio.c new file +12 -0

+
+	regmap_update_bits(md->regmap, reg, 0xff << shift,
+			   hwirq << shift);
+}
+
+static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot = meson_find_irq_slot(md, data->irq);
+
+	if (slot >= 0)
+		meson_set_hwirq(md, slot, hwirq);

drivers/irqchip/irq-meson-gpio.c new file +4 -0

+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val |= REG_EDGE_POL_EDGE(slot);
+

drivers/irqchip/irq-meson-gpio.c new file +24 -0

+	irq_chip_mask_parent(data);
+}
+
+static struct irq_chip meson_irq_chip = {
+	.name = "meson_gpio_intc",
+	.irq_set_type = meson_irq_set_type,
+	.irq_eoi = irq_chip_eoi_parent,
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_startup = meson_irq_startup,
+	.irq_shutdown = meson_irq_shutdown,
+	.irq_set_affinity = irq_chip_set_affinity_parent,
+};
+
+static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct meson_data *md = d->host_data;
+	irq_hw_number_t hwirq;
+	int ret, slot;
+
+	slot = meson_alloc_irq_slot(md, virq);
+	if (slot < 0)

drivers/irqchip/irq-meson-gpio.c new file +74 -0

+
+	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
+	if (ret)
+		meson_free_irq_slot(md, virq);
+
+	return ret;
+}
+
+static void meson_irq_free(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs)
+{
+	struct meson_data *md = d->host_data;
+
+	irq_domain_free_irqs_common(d, virq, nr_irqs);
+	meson_free_irq_slot(md, virq);
+}
+
+static const struct irq_domain_ops meson_irq_ops = {
+	.alloc = meson_irq_alloc,
+	.free = meson_irq_free,
+	.xlate = irq_domain_xlate_twocell,
+};
+
+static int meson_get_irqs(struct meson_data *md, struct device_node *node)
+{
+	int ret, i;
+	u32 irq;
+
+	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
+		ret = of_property_read_u32_index(node, "interrupts", i, &irq);
+		if (ret)
+			break;
+		md->slots[i].irq = irq;
+	}
+
+	md->num_slots = i;
+
+	return i ? 0 : -EINVAL;
+}
+
+static const struct regmap_config meson_regmap_config = {
+	.reg_bits       = 32,
+	.reg_stride     = 4,
+	.val_bits       = 32,
+	.max_register	= REG_FILTER_SEL,
+};
+
+static int __init meson_gpio_irq_init(struct device_node *node,
+				      struct device_node *parent)
+{
+	struct irq_domain *meson_irq_domain, *parent_domain;
+	struct meson_data *md;
+	void __iomem *io_base;
+	int ret;
+
+	md = kzalloc(sizeof(*md), GFP_KERNEL);
+	if (!md)
+		return -ENOMEM;
+
+	mutex_init(&md->lock);
+
+	io_base = of_iomap(node, 0);
+	if (!io_base)
+		return -EINVAL;
+
+	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
+	if (IS_ERR(md->regmap))
+		return PTR_ERR(md->regmap);
+
+	/* initialize to IRQ_TYPE_LEVEL_HIGH */
+	regmap_write(md->regmap, REG_EDGE_POL, 0);
+	/* disable all GPIO interrupt sources */
+	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
+	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);

Quoted 80 lines

Add a driver supporting the GPIO interrupt controller on certain Amlogic meson SoC's.

Signed-off-by Heiner Kallweit <[email protected]>

---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used v6:
- change DT property parent-interrupts to interrupts v7:
- no changes

---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

drivers/irqchip/Kconfig +5 -0

index 478f8ace..bdc86e14 100644
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y

drivers/irqchip/Makefile +1 -0

index b64c59b8..1be482bd 100644
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o

drivers/irqchip/irq-meson-gpio.c new file +32 -0

new file mode 100644
index 00000000..925d00c2
@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <[email protected]>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <[email protected]>
+ * Copyright (c) 2017 Heiner Kallweit <[email protected]>
+ *
+ * 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, version 2.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define REG_EDGE_POL		0x00
+#define REG_PIN_03_SEL		0x04
+#define REG_PIN_47_SEL		0x08
+#define REG_FILTER_SEL		0x0c
+
+#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
+#define REG_EDGE_POL_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
+

drivers/irqchip/irq-meson-gpio.c line 36

Quoted 4 lines

drivers/irqchip/irq-meson-gpio.c new file +4 -0

+#define MAX_PARENT_IRQ_NUM	8
+
+/* maximum number of GPIO IRQs on supported platforms */
+#define MAX_NUM_GPIO_IRQ	133

Why aren't these values coming from DT? I bet that a future revision of the same HW will double these. Or at least, you could match it on the compatible string.

drivers/irqchip/irq-meson-gpio.c line 43

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+/*
+ * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
+ * edge. That's due to HW constraints.
+ * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
+ * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
+ * one bit to the right.

Please expand on how you expect this to work, specially when a random driver expects a single interrupt.

Quoted 20 lines

drivers/irqchip/irq-meson-gpio.c new file +20 -0

+ */
+#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)
+
+struct meson_irq_slot {
+	unsigned int irq;
+	unsigned int owner;
+};
+
+struct meson_data {
+	struct regmap *regmap;
+	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
+	unsigned int num_slots;
+	struct mutex lock;
+};
+
+static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -ENOSPC;
+
+	mutex_lock(&md->lock);

drivers/irqchip/irq-meson-gpio.c line 67

Quoted 4 lines

drivers/irqchip/irq-meson-gpio.c new file +4 -0

+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (!md->slots[i].owner) {
+			md->slots[i].owner = virq;

Why do you have to deal with the virq? It'd be more logical to deal with the hwirq. The usual mechanism to reserve a "slot" is to use a bitmap indexed by the hwirq. Why is that not working for you?

Quoted 12 lines

drivers/irqchip/irq-meson-gpio.c new file +12 -0

+			slot = i;
+		}
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}
+
+static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i;
+

drivers/irqchip/irq-meson-gpio.c line 89

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots; i++)
+		if (md->slots[i].owner == virq) {
+			md->slots[i].owner = 0;
+			break;
+		}
+
+	mutex_unlock(&md->lock);
+}

These two functions are basically the same...

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+
+static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -EINVAL;
+

drivers/irqchip/irq-meson-gpio.c line 104

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (md->slots[i].owner == virq)
+			slot = i;
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}

... and could be expressed in terms of this one.

drivers/irqchip/irq-meson-gpio.c line 109

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+
+static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int hwirq)
+{
+	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
+	int shift = 8 * (idx % 4);

What's this?

Quoted 16 lines

drivers/irqchip/irq-meson-gpio.c new file +16 -0

+
+	regmap_update_bits(md->regmap, reg, 0xff << shift,
+			   hwirq << shift);
+}
+
+static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot = meson_find_irq_slot(md, data->irq);
+
+	if (slot >= 0)
+		meson_set_hwirq(md, slot, hwirq);
+}
+
+static int meson_irq_set_type(struct irq_data *data, unsigned int type)
+{

drivers/irqchip/irq-meson-gpio.c line 131

Quoted 6 lines

drivers/irqchip/irq-meson-gpio.c new file +6 -0

+	struct meson_data *md = data->domain->host_data;
+	int slot;
+	unsigned int val = 0;
+
+	if (type == IRQ_TYPE_EDGE_BOTH)
+		return -EINVAL;

So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?

drivers/irqchip/irq-meson-gpio.c line 135

Quoted 4 lines

drivers/irqchip/irq-meson-gpio.c new file +4 -0

+
+	slot = meson_find_irq_slot(md, data->irq);
+	if (slot < 0)
+		return slot;

How can this happen?

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c new file +9 -0

+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val |= REG_EDGE_POL_EDGE(slot);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_LOW(slot);
+
+	regmap_update_bits(md->regmap, REG_EDGE_POL,
+			   REG_EDGE_POL_MASK(slot), val);

drivers/irqchip/irq-meson-gpio.c line 149

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val = IRQ_TYPE_EDGE_RISING;
+	else
+		val = IRQ_TYPE_LEVEL_HIGH;

How does this work? Does this HW have some magic falling->rising and low->high conversion feature? If it doesn't, I cannot see how this can work.

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+	return irq_chip_set_type_parent(data, val);
+}
+
+static unsigned int meson_irq_startup(struct irq_data *data)
+{
+	irq_chip_unmask_parent(data);

drivers/irqchip/irq-meson-gpio.c line 162

Quoted 6 lines

drivers/irqchip/irq-meson-gpio.c new file +6 -0

+	/*
+	 * An extra bit was added to allow having the same gpio hwirq twice
+	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
+	 * gpio hwirq.
+	 */
+	meson_irq_set_hwirq(data, data->hwirq >> 1);

Again. Do you support EDGE_BOTH or not?

drivers/irqchip/irq-meson-gpio.c line 169

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+	return 0;
+}
+
+static void meson_irq_shutdown(struct irq_data *data)
+{
+	meson_irq_set_hwirq(data, 0xff);

What's special about 0xff?

Quoted 29 lines

drivers/irqchip/irq-meson-gpio.c new file +29 -0

+	irq_chip_mask_parent(data);
+}
+
+static struct irq_chip meson_irq_chip = {
+	.name = "meson_gpio_intc",
+	.irq_set_type = meson_irq_set_type,
+	.irq_eoi = irq_chip_eoi_parent,
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_startup = meson_irq_startup,
+	.irq_shutdown = meson_irq_shutdown,
+	.irq_set_affinity = irq_chip_set_affinity_parent,
+};
+
+static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct meson_data *md = d->host_data;
+	irq_hw_number_t hwirq;
+	int ret, slot;
+
+	slot = meson_alloc_irq_slot(md, virq);
+	if (slot < 0)
+		return slot;
+
+	hwirq = fwspec->param[0];
+	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
+

drivers/irqchip/irq-meson-gpio.c line 203

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+	parent_fwspec.fwnode = d->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0] = 0; /* SPI */
+	parent_fwspec.param[1] = md->slots[slot].irq;
+	parent_fwspec.param[2] = IRQ_TYPE_NONE;

Hell no. Look at the GIC DT binding: there is no NONE. It is either HIGH, or RISING.

Quoted 78 lines

drivers/irqchip/irq-meson-gpio.c new file +78 -0

+
+	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
+	if (ret)
+		meson_free_irq_slot(md, virq);
+
+	return ret;
+}
+
+static void meson_irq_free(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs)
+{
+	struct meson_data *md = d->host_data;
+
+	irq_domain_free_irqs_common(d, virq, nr_irqs);
+	meson_free_irq_slot(md, virq);
+}
+
+static const struct irq_domain_ops meson_irq_ops = {
+	.alloc = meson_irq_alloc,
+	.free = meson_irq_free,
+	.xlate = irq_domain_xlate_twocell,
+};
+
+static int meson_get_irqs(struct meson_data *md, struct device_node *node)
+{
+	int ret, i;
+	u32 irq;
+
+	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
+		ret = of_property_read_u32_index(node, "interrupts", i, &irq);
+		if (ret)
+			break;
+		md->slots[i].irq = irq;
+	}
+
+	md->num_slots = i;
+
+	return i ? 0 : -EINVAL;
+}
+
+static const struct regmap_config meson_regmap_config = {
+	.reg_bits       = 32,
+	.reg_stride     = 4,
+	.val_bits       = 32,
+	.max_register	= REG_FILTER_SEL,
+};
+
+static int __init meson_gpio_irq_init(struct device_node *node,
+				      struct device_node *parent)
+{
+	struct irq_domain *meson_irq_domain, *parent_domain;
+	struct meson_data *md;
+	void __iomem *io_base;
+	int ret;
+
+	md = kzalloc(sizeof(*md), GFP_KERNEL);
+	if (!md)
+		return -ENOMEM;
+
+	mutex_init(&md->lock);
+
+	io_base = of_iomap(node, 0);
+	if (!io_base)
+		return -EINVAL;
+
+	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
+	if (IS_ERR(md->regmap))
+		return PTR_ERR(md->regmap);
+
+	/* initialize to IRQ_TYPE_LEVEL_HIGH */
+	regmap_write(md->regmap, REG_EDGE_POL, 0);
+	/* disable all GPIO interrupt sources */
+	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
+	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
+	/* disable filtering */
+	regmap_write(md->regmap, REG_FILTER_SEL, 0);
+
+	ret = meson_get_irqs(md, node);

drivers/irqchip/irq-meson-gpio.c line 287

Quoted 6 lines

drivers/irqchip/irq-meson-gpio.c new file +6 -0

+	if (ret)
+		return ret;
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain)
+		return -ENXIO;

Memory leak on all the return paths.

drivers/irqchip/irq-meson-gpio.c line 295

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c new file +8 -0

+
+	meson_irq_domain = irq_domain_add_hierarchy(parent_domain, 0,
+						    NUM_GPIO_HWIRQ, node,
+						    &meson_irq_ops, md);
+	return meson_irq_domain ? 0 : -EINVAL;
+}
+
+IRQCHIP_DECLARE(meson_gpio_irq, "amlogic,meson-gpio-intc", meson_gpio_irq_init);

Thanks,

	M.

Am 13.06.2017 um 10:31 schrieb Marc Zyngier:

Quoted 81 lines · Heiner Kallweit

Quoted 80 lines

Add a driver supporting the GPIO interrupt controller on certain Amlogic meson SoC's.

Signed-off-by Heiner Kallweit <[email protected]>

---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used v6:
- change DT property parent-interrupts to interrupts v7:
- no changes

---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

drivers/irqchip/Kconfig +5 -0

index 478f8ace..bdc86e14 100644
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y

drivers/irqchip/Makefile +1 -0

index b64c59b8..1be482bd 100644
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o

drivers/irqchip/irq-meson-gpio.c new file +32 -0

new file mode 100644
index 00000000..925d00c2
@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <[email protected]>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <[email protected]>
+ * Copyright (c) 2017 Heiner Kallweit <[email protected]>
+ *
+ * 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, version 2.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define REG_EDGE_POL		0x00
+#define REG_PIN_03_SEL		0x04
+#define REG_PIN_47_SEL		0x08
+#define REG_FILTER_SEL		0x0c
+
+#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
+#define REG_EDGE_POL_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
+

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c line 36

Quoted 4 lines

drivers/irqchip/irq-meson-gpio.c new file +4 -0

+#define MAX_PARENT_IRQ_NUM	8
+
+/* maximum number of GPIO IRQs on supported platforms */
+#define MAX_NUM_GPIO_IRQ	133

Why aren't these values coming from DT? I bet that a future revision of the same HW will double these. Or at least, you could match it on the compatible string.

Alternatively this value can be set to 255. The GPIO source is an 8 bit value with 255 being reserved for "no interrupt source assigned".

This way we cover all chips based on the same IP.

I think what we could gain by introducing an additional DT property (saving a few bytes in the irqdomain mapping table) isn't worth the effort.

Quoted 11 lines

drivers/irqchip/irq-meson-gpio.c line 43

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+/*
+ * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
+ * edge. That's due to HW constraints.
+ * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
+ * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
+ * one bit to the right.

Please expand on how you expect this to work, specially when a random driver expects a single interrupt.

The gpio interrupt controller in this chip doesn't have native support for IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio to two parent interrupts, one for each edge.

There's still no solution to achieve this in a way everybody is happy with. Therefore this feature isn't part of this patch set.

However, to be prepared to include this feature later, the interface between pinctrl/gpio and irqchip driver should (IMHO) cater for it already. Else we may have to touch the irqchip driver later and change the interface what I would like to avoid.

If a driver just needs one (parent) interrupt, it can request hwirq (2 * GPIO_HWIRQ) only. There's no issue with that.

Quoted 20 lines

Quoted 20 lines

drivers/irqchip/irq-meson-gpio.c new file +20 -0

+ */
+#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)
+
+struct meson_irq_slot {
+	unsigned int irq;
+	unsigned int owner;
+};
+
+struct meson_data {
+	struct regmap *regmap;
+	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
+	unsigned int num_slots;
+	struct mutex lock;
+};
+
+static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -ENOSPC;
+
+	mutex_lock(&md->lock);

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c line 67

Quoted 4 lines

drivers/irqchip/irq-meson-gpio.c new file +4 -0

+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (!md->slots[i].owner) {
+			md->slots[i].owner = virq;

Why do you have to deal with the virq? It'd be more logical to deal with the hwirq. The usual mechanism to reserve a "slot" is to use a bitmap indexed by the hwirq. Why is that not working for you?

Using the hwirq as owner should also be possible. Will consider this.

A slot has two members, the owner and a the associated parent irq number. Of course we could split this into a slot bitmap + an array with parent irq's indexed by slot number. Would you prefer this?

Quoted 43 lines

Quoted 12 lines

drivers/irqchip/irq-meson-gpio.c new file +12 -0

+			slot = i;
+		}
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}
+
+static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i;
+

drivers/irqchip/irq-meson-gpio.c line 89

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots; i++)
+		if (md->slots[i].owner == virq) {
+			md->slots[i].owner = 0;
+			break;
+		}
+
+	mutex_unlock(&md->lock);
+}

These two functions are basically the same...

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+
+static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -EINVAL;
+

drivers/irqchip/irq-meson-gpio.c line 104

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (md->slots[i].owner == virq)
+			slot = i;
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}

... and could be expressed in terms of this one.

Quoted 8 lines

drivers/irqchip/irq-meson-gpio.c line 109

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+
+static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int hwirq)
+{
+	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
+	int shift = 8 * (idx % 4);

What's this?

GPIO source for the eight parent irq's can be configured using two 32-bit registers with four 8-bit fields each.

Quoted 16 lines

Quoted 16 lines

drivers/irqchip/irq-meson-gpio.c new file +16 -0

+
+	regmap_update_bits(md->regmap, reg, 0xff << shift,
+			   hwirq << shift);
+}
+
+static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot = meson_find_irq_slot(md, data->irq);
+
+	if (slot >= 0)
+		meson_set_hwirq(md, slot, hwirq);
+}
+
+static int meson_irq_set_type(struct irq_data *data, unsigned int type)
+{

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c line 131

Quoted 6 lines

drivers/irqchip/irq-meson-gpio.c new file +6 -0

+	struct meson_data *md = data->domain->host_data;
+	int slot;
+	unsigned int val = 0;
+
+	if (type == IRQ_TYPE_EDGE_BOTH)
+		return -EINVAL;

So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?

We reject it in the initial version of the patch set as there's no consensus yet on some details of the workaround needed for EDGE_BOTH support.

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c line 135

Quoted 4 lines

drivers/irqchip/irq-meson-gpio.c new file +4 -0

+
+	slot = meson_find_irq_slot(md, data->irq);
+	if (slot < 0)
+		return slot;

How can this happen?

I see no way how this can happen. It was basically added to be on the safe side and fail nicely in case I miss a scenario which could cause this to fail. I can remove the check.

Quoted 9 lines

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c new file +9 -0

+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val |= REG_EDGE_POL_EDGE(slot);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_LOW(slot);
+
+	regmap_update_bits(md->regmap, REG_EDGE_POL,
+			   REG_EDGE_POL_MASK(slot), val);

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c line 149

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val = IRQ_TYPE_EDGE_RISING;
+	else
+		val = IRQ_TYPE_LEVEL_HIGH;

How does this work? Does this HW have some magic falling->rising and low->high conversion feature? If it doesn't, I cannot see how this can work.

Exactly, HW has a programmable polarity inverter.

Quoted 7 lines

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+	return irq_chip_set_type_parent(data, val);
+}
+
+static unsigned int meson_irq_startup(struct irq_data *data)
+{
+	irq_chip_unmask_parent(data);

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c line 162

Quoted 6 lines

drivers/irqchip/irq-meson-gpio.c new file +6 -0

+	/*
+	 * An extra bit was added to allow having the same gpio hwirq twice
+	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
+	 * gpio hwirq.
+	 */
+	meson_irq_set_hwirq(data, data->hwirq >> 1);

Again. Do you support EDGE_BOTH or not?

Not yet ..

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c line 169

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+	return 0;
+}
+
+static void meson_irq_shutdown(struct irq_data *data)
+{
+	meson_irq_set_hwirq(data, 0xff);

What's special about 0xff?

0xff is the reserved value indicating the no GPIO source is assigned to the parent irq.

Quoted 29 lines

Quoted 29 lines

drivers/irqchip/irq-meson-gpio.c new file +29 -0

+	irq_chip_mask_parent(data);
+}
+
+static struct irq_chip meson_irq_chip = {
+	.name = "meson_gpio_intc",
+	.irq_set_type = meson_irq_set_type,
+	.irq_eoi = irq_chip_eoi_parent,
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_startup = meson_irq_startup,
+	.irq_shutdown = meson_irq_shutdown,
+	.irq_set_affinity = irq_chip_set_affinity_parent,
+};
+
+static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct meson_data *md = d->host_data;
+	irq_hw_number_t hwirq;
+	int ret, slot;
+
+	slot = meson_alloc_irq_slot(md, virq);
+	if (slot < 0)
+		return slot;
+
+	hwirq = fwspec->param[0];
+	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
+

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c line 203

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+	parent_fwspec.fwnode = d->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0] = 0; /* SPI */
+	parent_fwspec.param[1] = md->slots[slot].irq;
+	parent_fwspec.param[2] = IRQ_TYPE_NONE;

Hell no. Look at the GIC DT binding: there is no NONE. It is either HIGH, or RISING.

OK, will be changed.

Quoted 78 lines

Quoted 78 lines

drivers/irqchip/irq-meson-gpio.c new file +78 -0

+
+	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
+	if (ret)
+		meson_free_irq_slot(md, virq);
+
+	return ret;
+}
+
+static void meson_irq_free(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs)
+{
+	struct meson_data *md = d->host_data;
+
+	irq_domain_free_irqs_common(d, virq, nr_irqs);
+	meson_free_irq_slot(md, virq);
+}
+
+static const struct irq_domain_ops meson_irq_ops = {
+	.alloc = meson_irq_alloc,
+	.free = meson_irq_free,
+	.xlate = irq_domain_xlate_twocell,
+};
+
+static int meson_get_irqs(struct meson_data *md, struct device_node *node)
+{
+	int ret, i;
+	u32 irq;
+
+	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
+		ret = of_property_read_u32_index(node, "interrupts", i, &irq);
+		if (ret)
+			break;
+		md->slots[i].irq = irq;
+	}
+
+	md->num_slots = i;
+
+	return i ? 0 : -EINVAL;
+}
+
+static const struct regmap_config meson_regmap_config = {
+	.reg_bits       = 32,
+	.reg_stride     = 4,
+	.val_bits       = 32,
+	.max_register	= REG_FILTER_SEL,
+};
+
+static int __init meson_gpio_irq_init(struct device_node *node,
+				      struct device_node *parent)
+{
+	struct irq_domain *meson_irq_domain, *parent_domain;
+	struct meson_data *md;
+	void __iomem *io_base;
+	int ret;
+
+	md = kzalloc(sizeof(*md), GFP_KERNEL);
+	if (!md)
+		return -ENOMEM;
+
+	mutex_init(&md->lock);
+
+	io_base = of_iomap(node, 0);
+	if (!io_base)
+		return -EINVAL;
+
+	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
+	if (IS_ERR(md->regmap))
+		return PTR_ERR(md->regmap);
+
+	/* initialize to IRQ_TYPE_LEVEL_HIGH */
+	regmap_write(md->regmap, REG_EDGE_POL, 0);
+	/* disable all GPIO interrupt sources */
+	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
+	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
+	/* disable filtering */
+	regmap_write(md->regmap, REG_FILTER_SEL, 0);
+
+	ret = meson_get_irqs(md, node);

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c line 287

Quoted 6 lines

drivers/irqchip/irq-meson-gpio.c new file +6 -0

+	if (ret)
+		return ret;
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain)
+		return -ENXIO;

Memory leak on all the return paths.

Indeed. Most likely copy & paste error when I used other irqchip drivers as inspiration. E.g. irq-crossbar faces the same issue, it allocates memory in crossbar_of_init which isn't free'd if irq_domain_add_hierarchy fails.

Quoted 91 lines

Am 13.06.2017 um 10:31 schrieb Marc Zyngier:

Quoted 90 lines · Heiner Kallweit

Quoted 74 lines

Add a driver supporting the GPIO interrupt controller on certain Amlogic meson SoC's.

Signed-off-by Heiner Kallweit <[email protected]>

---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used v6:
- change DT property parent-interrupts to interrupts v7:
- no changes

---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

drivers/irqchip/Kconfig +5 -0

index 478f8ace..bdc86e14 100644
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y

drivers/irqchip/Makefile +1 -0

index b64c59b8..1be482bd 100644
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o

drivers/irqchip/irq-meson-gpio.c new file +26 -0

new file mode 100644
index 00000000..925d00c2
@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <[email protected]>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <[email protected]>
+ * Copyright (c) 2017 Heiner Kallweit <[email protected]>
+ *
+ * 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, version 2.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define REG_EDGE_POL		0x00
+#define REG_PIN_03_SEL		0x04
+#define REG_PIN_47_SEL		0x08

drivers/irqchip/irq-meson-gpio.c line 36

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+#define REG_FILTER_SEL		0x0c
+
+#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
+#define REG_EDGE_POL_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
+
+#define MAX_PARENT_IRQ_NUM	8
+
+/* maximum number of GPIO IRQs on supported platforms */
+#define MAX_NUM_GPIO_IRQ	133

Why aren't these values coming from DT? I bet that a future revision of the same HW will double these. Or at least, you could match it on the compatible string.

Quoted 2 lines

Alternatively this value can be set to 255. The GPIO source is an 8 bit value with 255 being reserved for "no interrupt source assigned".

Who is reserving it? The HW? Or is that your own defined convention?

Quoted 1 line

This way we cover all chips based on the same IP.

Why? Where is that 8bit limit coming from?

Quoted 2 lines

I think what we could gain by introducing an additional DT property (saving a few bytes in the irqdomain mapping table) isn't worth the effort.

It is not about saving or wasting memory. It is about making the driver and its binding able to span multiple generation of the HW without too much churn. Which is why I'm suggesting that you either define these properties in DT *or* match the compatible string to obtain these values.

Quoted 11 lines

Quoted 11 lines

drivers/irqchip/irq-meson-gpio.c line 43

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+/*
+ * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
+ * edge. That's due to HW constraints.
+ * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
+ * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
+ * one bit to the right.

Please expand on how you expect this to work, specially when a random driver expects a single interrupt.

Quoted 3 lines

The gpio interrupt controller in this chip doesn't have native support for IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio to two parent interrupts, one for each edge.

No, that's horrible, racy, and impractical. It has been proposed in the past (for the same HW), and we're not going there again.

Quoted 7 lines

There's still no solution to achieve this in a way everybody is happy with. Therefore this feature isn't part of this patch set.

However, to be prepared to include this feature later, the interface between pinctrl/gpio and irqchip driver should (IMHO) cater for it already. Else we may have to touch the irqchip driver later and change the interface what I would like to avoid.

Don't even think of it, and considered it pre-NAKed.

Quoted 3 lines

If a driver just needs one (parent) interrupt, it can request hwirq (2 * GPIO_HWIRQ) only. There's no issue with that.

Other than being utterly useless and confusing?

Quoted 29 lines

Quoted 20 lines

Quoted 20 lines

drivers/irqchip/irq-meson-gpio.c new file +20 -0

+ */
+#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)
+
+struct meson_irq_slot {
+	unsigned int irq;
+	unsigned int owner;
+};
+
+struct meson_data {
+	struct regmap *regmap;
+	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
+	unsigned int num_slots;
+	struct mutex lock;
+};
+
+static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -ENOSPC;
+
+	mutex_lock(&md->lock);

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c line 67

Quoted 4 lines

drivers/irqchip/irq-meson-gpio.c new file +4 -0

+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (!md->slots[i].owner) {
+			md->slots[i].owner = virq;

Why do you have to deal with the virq? It'd be more logical to deal with the hwirq. The usual mechanism to reserve a "slot" is to use a bitmap indexed by the hwirq. Why is that not working for you?

Quoted 5 lines

Using the hwirq as owner should also be possible. Will consider this.

A slot has two members, the owner and a the associated parent irq number. Of course we could split this into a slot bitmap + an array with parent irq's indexed by slot number. Would you prefer this?

Again, why do you need to consider the Linux irq number? All you need to track is whether a hwirq has been allocated or not, since all the irqchip function are called with an irq_data as a parameter, which contains both the irq and hwirq values.

Quoted 51 lines

Quoted 43 lines

Quoted 12 lines

drivers/irqchip/irq-meson-gpio.c new file +12 -0

+			slot = i;
+		}
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}
+
+static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i;
+

drivers/irqchip/irq-meson-gpio.c line 89

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots; i++)
+		if (md->slots[i].owner == virq) {
+			md->slots[i].owner = 0;
+			break;
+		}
+
+	mutex_unlock(&md->lock);
+}

These two functions are basically the same...

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+
+static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -EINVAL;
+

drivers/irqchip/irq-meson-gpio.c line 104

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (md->slots[i].owner == virq)
+			slot = i;
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}

... and could be expressed in terms of this one.

Quoted 8 lines

drivers/irqchip/irq-meson-gpio.c line 109

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+
+static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int hwirq)
+{
+	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
+	int shift = 8 * (idx % 4);

What's this?

Quoted 2 lines

GPIO source for the eight parent irq's can be configured using two 32-bit registers with four 8-bit fields each.

Consider moving it to an accessor and document the mapping of the hwirqs in these registers.

Quoted 25 lines

Quoted 25 lines

Quoted 12 lines

drivers/irqchip/irq-meson-gpio.c new file +12 -0

+
+	regmap_update_bits(md->regmap, reg, 0xff << shift,
+			   hwirq << shift);
+}
+
+static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot = meson_find_irq_slot(md, data->irq);
+
+	if (slot >= 0)
+		meson_set_hwirq(md, slot, hwirq);

drivers/irqchip/irq-meson-gpio.c line 131

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+}
+
+static int meson_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot;
+	unsigned int val = 0;
+
+	if (type == IRQ_TYPE_EDGE_BOTH)
+		return -EINVAL;

So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?

Quoted 2 lines

We reject it in the initial version of the patch set as there's no consensus yet on some details of the workaround needed for EDGE_BOTH support.

There is a consensus: The HW doesn't support this feature.

Quoted 60 lines

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c line 135

Quoted 4 lines

drivers/irqchip/irq-meson-gpio.c new file +4 -0

+
+	slot = meson_find_irq_slot(md, data->irq);
+	if (slot < 0)
+		return slot;

How can this happen?

I see no way how this can happen. It was basically added to be on the safe side and fail nicely in case I miss a scenario which could cause this to fail. I can remove the check.

Quoted 9 lines

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c new file +9 -0

+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val |= REG_EDGE_POL_EDGE(slot);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_LOW(slot);
+
+	regmap_update_bits(md->regmap, REG_EDGE_POL,
+			   REG_EDGE_POL_MASK(slot), val);

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c line 149

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val = IRQ_TYPE_EDGE_RISING;
+	else
+		val = IRQ_TYPE_LEVEL_HIGH;

How does this work? Does this HW have some magic falling->rising and low->high conversion feature? If it doesn't, I cannot see how this can work.

Exactly, HW has a programmable polarity inverter.

Quoted 7 lines

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+	return irq_chip_set_type_parent(data, val);
+}
+
+static unsigned int meson_irq_startup(struct irq_data *data)
+{
+	irq_chip_unmask_parent(data);

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c line 162

Quoted 6 lines

drivers/irqchip/irq-meson-gpio.c new file +6 -0

+	/*
+	 * An extra bit was added to allow having the same gpio hwirq twice
+	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
+	 * gpio hwirq.
+	 */
+	meson_irq_set_hwirq(data, data->hwirq >> 1);

Again. Do you support EDGE_BOTH or not?

Not yet ..

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c line 169

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+	return 0;
+}
+
+static void meson_irq_shutdown(struct irq_data *data)
+{
+	meson_irq_set_hwirq(data, 0xff);

What's special about 0xff?

Quoted 2 lines

0xff is the reserved value indicating the no GPIO source is assigned to the parent irq.

Then document it, add a #define for this.

Quoted 127 lines

Quoted 29 lines

Quoted 29 lines

drivers/irqchip/irq-meson-gpio.c new file +29 -0

+	irq_chip_mask_parent(data);
+}
+
+static struct irq_chip meson_irq_chip = {
+	.name = "meson_gpio_intc",
+	.irq_set_type = meson_irq_set_type,
+	.irq_eoi = irq_chip_eoi_parent,
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_startup = meson_irq_startup,
+	.irq_shutdown = meson_irq_shutdown,
+	.irq_set_affinity = irq_chip_set_affinity_parent,
+};
+
+static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct meson_data *md = d->host_data;
+	irq_hw_number_t hwirq;
+	int ret, slot;
+
+	slot = meson_alloc_irq_slot(md, virq);
+	if (slot < 0)
+		return slot;
+
+	hwirq = fwspec->param[0];
+	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
+

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c line 203

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -0

+	parent_fwspec.fwnode = d->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0] = 0; /* SPI */
+	parent_fwspec.param[1] = md->slots[slot].irq;
+	parent_fwspec.param[2] = IRQ_TYPE_NONE;

Hell no. Look at the GIC DT binding: there is no NONE. It is either HIGH, or RISING.

OK, will be changed.

Quoted 78 lines

Quoted 78 lines

drivers/irqchip/irq-meson-gpio.c new file +78 -0

+
+	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
+	if (ret)
+		meson_free_irq_slot(md, virq);
+
+	return ret;
+}
+
+static void meson_irq_free(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs)
+{
+	struct meson_data *md = d->host_data;
+
+	irq_domain_free_irqs_common(d, virq, nr_irqs);
+	meson_free_irq_slot(md, virq);
+}
+
+static const struct irq_domain_ops meson_irq_ops = {
+	.alloc = meson_irq_alloc,
+	.free = meson_irq_free,
+	.xlate = irq_domain_xlate_twocell,
+};
+
+static int meson_get_irqs(struct meson_data *md, struct device_node *node)
+{
+	int ret, i;
+	u32 irq;
+
+	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
+		ret = of_property_read_u32_index(node, "interrupts", i, &irq);
+		if (ret)
+			break;
+		md->slots[i].irq = irq;
+	}
+
+	md->num_slots = i;
+
+	return i ? 0 : -EINVAL;
+}
+
+static const struct regmap_config meson_regmap_config = {
+	.reg_bits       = 32,
+	.reg_stride     = 4,
+	.val_bits       = 32,
+	.max_register	= REG_FILTER_SEL,
+};
+
+static int __init meson_gpio_irq_init(struct device_node *node,
+				      struct device_node *parent)
+{
+	struct irq_domain *meson_irq_domain, *parent_domain;
+	struct meson_data *md;
+	void __iomem *io_base;
+	int ret;
+
+	md = kzalloc(sizeof(*md), GFP_KERNEL);
+	if (!md)
+		return -ENOMEM;
+
+	mutex_init(&md->lock);
+
+	io_base = of_iomap(node, 0);
+	if (!io_base)
+		return -EINVAL;
+
+	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
+	if (IS_ERR(md->regmap))
+		return PTR_ERR(md->regmap);
+
+	/* initialize to IRQ_TYPE_LEVEL_HIGH */
+	regmap_write(md->regmap, REG_EDGE_POL, 0);
+	/* disable all GPIO interrupt sources */
+	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
+	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
+	/* disable filtering */
+	regmap_write(md->regmap, REG_FILTER_SEL, 0);
+
+	ret = meson_get_irqs(md, node);

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c line 287

Quoted 6 lines

drivers/irqchip/irq-meson-gpio.c new file +6 -0

+	if (ret)
+		return ret;
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain)
+		return -ENXIO;

Memory leak on all the return paths.

Quoted 3 lines

Indeed. Most likely copy & paste error when I used other irqchip drivers as inspiration. E.g. irq-crossbar faces the same issue, it allocates memory in crossbar_of_init which isn't free'd if irq_domain_add_hierarchy fails.

Then please submit a patch addressing the defect.

Thanks,

	M.

Am 15.06.2017 um 15:27 schrieb Marc Zyngier:

Quoted 92 lines · Heiner Kallweit

Quoted 91 lines

Am 13.06.2017 um 10:31 schrieb Marc Zyngier:

Quoted 90 lines · Heiner Kallweit

Quoted 74 lines

Add a driver supporting the GPIO interrupt controller on certain Amlogic meson SoC's.

Signed-off-by Heiner Kallweit <[email protected]>

---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used v6:
- change DT property parent-interrupts to interrupts v7:
- no changes

---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

drivers/irqchip/Kconfig +5 -0

index 478f8ace..bdc86e14 100644
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y

drivers/irqchip/Makefile +1 -0

index b64c59b8..1be482bd 100644
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o

drivers/irqchip/irq-meson-gpio.c new file +26 -0

new file mode 100644
index 00000000..925d00c2
@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <[email protected]>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <[email protected]>
+ * Copyright (c) 2017 Heiner Kallweit <[email protected]>
+ *
+ * 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, version 2.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define REG_EDGE_POL		0x00
+#define REG_PIN_03_SEL		0x04
+#define REG_PIN_47_SEL		0x08

drivers/irqchip/irq-meson-gpio.c line 36

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+#define REG_FILTER_SEL		0x0c
+
+#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
+#define REG_EDGE_POL_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
+
+#define MAX_PARENT_IRQ_NUM	8
+
+/* maximum number of GPIO IRQs on supported platforms */
+#define MAX_NUM_GPIO_IRQ	133

Why aren't these values coming from DT? I bet that a future revision of the same HW will double these. Or at least, you could match it on the compatible string.

Quoted 9 lines

Quoted 2 lines

Alternatively this value can be set to 255. The GPIO source is an 8 bit value with 255 being reserved for "no interrupt source assigned".

Who is reserving it? The HW? Or is that your own defined convention?

Quoted 1 line

This way we cover all chips based on the same IP.

Why? Where is that 8bit limit coming from?

The 8 bit limit is in the HW.

Quoted 19 lines

Quoted 2 lines

I think what we could gain by introducing an additional DT property (saving a few bytes in the irqdomain mapping table) isn't worth the effort.

It is not about saving or wasting memory. It is about making the driver and its binding able to span multiple generation of the HW without too much churn. Which is why I'm suggesting that you either define these properties in DT *or* match the compatible string to obtain these values.

Quoted 11 lines

Quoted 11 lines

drivers/irqchip/irq-meson-gpio.c line 43

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+/*
+ * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
+ * edge. That's due to HW constraints.
+ * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
+ * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
+ * one bit to the right.

Please expand on how you expect this to work, specially when a random driver expects a single interrupt.

Quoted 7 lines

Quoted 3 lines

The gpio interrupt controller in this chip doesn't have native support for IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio to two parent interrupts, one for each edge.

No, that's horrible, racy, and impractical. It has been proposed in the past (for the same HW), and we're not going there again.

IIRC what has been proposed before is to re-program the polarity of edge detection withing the ISR. This would match your concern that it is racy.

Here it's about using two parent irq's, one programmed to react on the rising edge whilst the other is triggered in case of falling edge. Would you consider this to be racy too?

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?

We reject it in the initial version of the patch set as there's no consensus yet on some details of the workaround needed for EDGE_BOTH support.

There is a consensus: The HW doesn't support this feature.

Means what? There is no acceptable way to support EDGE_BOTH on this HW? In this case I could stop here as for me this feature is important.

Would be somewhat a pity as there is a working solution. If it doesn't fit into the current irq(chip) framework, shouldn't this be considered a gap in the framework?

Quoted 130 lines

Am 15.06.2017 um 15:27 schrieb Marc Zyngier:

Quoted 92 lines · Heiner Kallweit

Quoted 91 lines

Am 13.06.2017 um 10:31 schrieb Marc Zyngier:

Quoted 90 lines · Heiner Kallweit

Quoted 74 lines

Add a driver supporting the GPIO interrupt controller on certain Amlogic meson SoC's.

Signed-off-by Heiner Kallweit <[email protected]>

---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used v6:
- change DT property parent-interrupts to interrupts v7:
- no changes

---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

drivers/irqchip/Kconfig +5 -0

index 478f8ace..bdc86e14 100644
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y

drivers/irqchip/Makefile +1 -0

index b64c59b8..1be482bd 100644
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o

drivers/irqchip/irq-meson-gpio.c new file +26 -0

new file mode 100644
index 00000000..925d00c2
@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <[email protected]>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <[email protected]>
+ * Copyright (c) 2017 Heiner Kallweit <[email protected]>
+ *
+ * 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, version 2.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define REG_EDGE_POL		0x00
+#define REG_PIN_03_SEL		0x04
+#define REG_PIN_47_SEL		0x08

drivers/irqchip/irq-meson-gpio.c line 36

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+#define REG_FILTER_SEL		0x0c
+
+#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
+#define REG_EDGE_POL_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
+
+#define MAX_PARENT_IRQ_NUM	8
+
+/* maximum number of GPIO IRQs on supported platforms */
+#define MAX_NUM_GPIO_IRQ	133

Why aren't these values coming from DT? I bet that a future revision of the same HW will double these. Or at least, you could match it on the compatible string.

Quoted 9 lines

Quoted 2 lines

Alternatively this value can be set to 255. The GPIO source is an 8 bit value with 255 being reserved for "no interrupt source assigned".

Who is reserving it? The HW? Or is that your own defined convention?

Quoted 1 line

This way we cover all chips based on the same IP.

Why? Where is that 8bit limit coming from?

The 8 bit limit is in the HW.

Quoted 19 lines

Quoted 2 lines

I think what we could gain by introducing an additional DT property (saving a few bytes in the irqdomain mapping table) isn't worth the effort.

It is not about saving or wasting memory. It is about making the driver and its binding able to span multiple generation of the HW without too much churn. Which is why I'm suggesting that you either define these properties in DT *or* match the compatible string to obtain these values.

Quoted 11 lines

Quoted 11 lines

drivers/irqchip/irq-meson-gpio.c line 43

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+/*
+ * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
+ * edge. That's due to HW constraints.
+ * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
+ * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
+ * one bit to the right.

Please expand on how you expect this to work, specially when a random driver expects a single interrupt.

Quoted 7 lines

Quoted 3 lines

The gpio interrupt controller in this chip doesn't have native support for IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio to two parent interrupts, one for each edge.

No, that's horrible, racy, and impractical. It has been proposed in the past (for the same HW), and we're not going there again.

Quoted 6 lines

IIRC what has been proposed before is to re-program the polarity of edge detection withing the ISR. This would match your concern that it is racy.

Here it's about using two parent irq's, one programmed to react on the rising edge whilst the other is triggered in case of falling edge. Would you consider this to be racy too?

How do you reconcile two interrupts to make look like a single one for a random, pre-existing driver?

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?

We reject it in the initial version of the patch set as there's no consensus yet on some details of the workaround needed for EDGE_BOTH support.

Quoted 4 lines

Quoted 2 lines

There is a consensus: The HW doesn't support this feature.

Means what? There is no acceptable way to support EDGE_BOTH on this HW? In this case I could stop here as for me this feature is important.

Answer my question above, which I asked in my initial review: How do you make two interrupts appear as one for a driver that wants to get signaled on each edge, using the existing API.

Quoted 4 lines

Would be somewhat a pity as there is a working solution. If it doesn't fit into the current irq(chip) framework, shouldn't this be considered a gap in the framework?

Again, this is not about the framework, which we can almost twist as we see fit. It is about the API that a driver expects. At the moment, you haven't proposed anything.

Thanks,

	M.

Am 15.06.2017 um 18:04 schrieb Marc Zyngier:

Quoted 148 lines · Heiner Kallweit

Quoted 130 lines

Am 15.06.2017 um 15:27 schrieb Marc Zyngier:

Quoted 92 lines · Heiner Kallweit

Quoted 91 lines

Am 13.06.2017 um 10:31 schrieb Marc Zyngier:

Quoted 90 lines · Heiner Kallweit

Quoted 74 lines

Add a driver supporting the GPIO interrupt controller on certain Amlogic meson SoC's.

Signed-off-by Heiner Kallweit <[email protected]>

---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used v6:
- change DT property parent-interrupts to interrupts v7:
- no changes

---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

drivers/irqchip/Kconfig +5 -0

index 478f8ace..bdc86e14 100644
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y

drivers/irqchip/Makefile +1 -0

index b64c59b8..1be482bd 100644
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o

drivers/irqchip/irq-meson-gpio.c new file +26 -0

new file mode 100644
index 00000000..925d00c2
@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <[email protected]>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <[email protected]>
+ * Copyright (c) 2017 Heiner Kallweit <[email protected]>
+ *
+ * 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, version 2.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define REG_EDGE_POL		0x00
+#define REG_PIN_03_SEL		0x04
+#define REG_PIN_47_SEL		0x08

drivers/irqchip/irq-meson-gpio.c line 36

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+#define REG_FILTER_SEL		0x0c
+
+#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
+#define REG_EDGE_POL_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
+
+#define MAX_PARENT_IRQ_NUM	8
+
+/* maximum number of GPIO IRQs on supported platforms */
+#define MAX_NUM_GPIO_IRQ	133

Why aren't these values coming from DT? I bet that a future revision of the same HW will double these. Or at least, you could match it on the compatible string.

Quoted 9 lines

Quoted 2 lines

Alternatively this value can be set to 255. The GPIO source is an 8 bit value with 255 being reserved for "no interrupt source assigned".

Who is reserving it? The HW? Or is that your own defined convention?

Quoted 1 line

This way we cover all chips based on the same IP.

Why? Where is that 8bit limit coming from?

The 8 bit limit is in the HW.

Quoted 19 lines

Quoted 2 lines

I think what we could gain by introducing an additional DT property (saving a few bytes in the irqdomain mapping table) isn't worth the effort.

It is not about saving or wasting memory. It is about making the driver and its binding able to span multiple generation of the HW without too much churn. Which is why I'm suggesting that you either define these properties in DT *or* match the compatible string to obtain these values.

Quoted 11 lines

Quoted 11 lines

drivers/irqchip/irq-meson-gpio.c line 43

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+/*
+ * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
+ * edge. That's due to HW constraints.
+ * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
+ * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
+ * one bit to the right.

Please expand on how you expect this to work, specially when a random driver expects a single interrupt.

Quoted 7 lines

Quoted 3 lines

The gpio interrupt controller in this chip doesn't have native support for IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio to two parent interrupts, one for each edge.

No, that's horrible, racy, and impractical. It has been proposed in the past (for the same HW), and we're not going there again.

Quoted 6 lines

IIRC what has been proposed before is to re-program the polarity of edge detection withing the ISR. This would match your concern that it is racy.

Here it's about using two parent irq's, one programmed to react on the rising edge whilst the other is triggered in case of falling edge. Would you consider this to be racy too?

How do you reconcile two interrupts to make look like a single one for a random, pre-existing driver?

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?

We reject it in the initial version of the patch set as there's no consensus yet on some details of the workaround needed for EDGE_BOTH support.

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

There is a consensus: The HW doesn't support this feature.

Means what? There is no acceptable way to support EDGE_BOTH on this HW? In this case I could stop here as for me this feature is important.

Answer my question above, which I asked in my initial review: How do you make two interrupts appear as one for a driver that wants to get signaled on each edge, using the existing API.

Please see the pinctrl/gpio part of the patch set.

The GPIO controller has an own IRQ domain. When requesting an interrupt in request_resources if needed two parent irq's are allocated, (was removed in the current initial version of the patch set) both calling the same ISR via a chained interrupt.

Works perfectly fine here.

Quoted 165 lines

Am 15.06.2017 um 18:04 schrieb Marc Zyngier:

Quoted 148 lines · Heiner Kallweit

Quoted 130 lines

Am 15.06.2017 um 15:27 schrieb Marc Zyngier:

Quoted 92 lines · Heiner Kallweit

Quoted 91 lines

Am 13.06.2017 um 10:31 schrieb Marc Zyngier:

Quoted 90 lines · Heiner Kallweit

Quoted 74 lines

Add a driver supporting the GPIO interrupt controller on certain Amlogic meson SoC's.

Signed-off-by Heiner Kallweit <[email protected]>

---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used v6:
- change DT property parent-interrupts to interrupts v7:
- no changes

---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

drivers/irqchip/Kconfig +5 -0

index 478f8ace..bdc86e14 100644
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y

drivers/irqchip/Makefile +1 -0

index b64c59b8..1be482bd 100644
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o

drivers/irqchip/irq-meson-gpio.c new file +26 -0

new file mode 100644
index 00000000..925d00c2
@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <[email protected]>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <[email protected]>
+ * Copyright (c) 2017 Heiner Kallweit <[email protected]>
+ *
+ * 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, version 2.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define REG_EDGE_POL		0x00
+#define REG_PIN_03_SEL		0x04
+#define REG_PIN_47_SEL		0x08

drivers/irqchip/irq-meson-gpio.c line 36

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+#define REG_FILTER_SEL		0x0c
+
+#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
+#define REG_EDGE_POL_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
+
+#define MAX_PARENT_IRQ_NUM	8
+
+/* maximum number of GPIO IRQs on supported platforms */
+#define MAX_NUM_GPIO_IRQ	133

Why aren't these values coming from DT? I bet that a future revision of the same HW will double these. Or at least, you could match it on the compatible string.

Quoted 9 lines

Quoted 2 lines

Alternatively this value can be set to 255. The GPIO source is an 8 bit value with 255 being reserved for "no interrupt source assigned".

Who is reserving it? The HW? Or is that your own defined convention?

Quoted 1 line

This way we cover all chips based on the same IP.

Why? Where is that 8bit limit coming from?

The 8 bit limit is in the HW.

Quoted 19 lines

Quoted 2 lines

I think what we could gain by introducing an additional DT property (saving a few bytes in the irqdomain mapping table) isn't worth the effort.

It is not about saving or wasting memory. It is about making the driver and its binding able to span multiple generation of the HW without too much churn. Which is why I'm suggesting that you either define these properties in DT *or* match the compatible string to obtain these values.

Quoted 11 lines

Quoted 11 lines

drivers/irqchip/irq-meson-gpio.c line 43

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+/*
+ * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
+ * edge. That's due to HW constraints.
+ * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
+ * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
+ * one bit to the right.

Please expand on how you expect this to work, specially when a random driver expects a single interrupt.

Quoted 7 lines

Quoted 3 lines

The gpio interrupt controller in this chip doesn't have native support for IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio to two parent interrupts, one for each edge.

No, that's horrible, racy, and impractical. It has been proposed in the past (for the same HW), and we're not going there again.

Quoted 6 lines

IIRC what has been proposed before is to re-program the polarity of edge detection withing the ISR. This would match your concern that it is racy.

Here it's about using two parent irq's, one programmed to react on the rising edge whilst the other is triggered in case of falling edge. Would you consider this to be racy too?

How do you reconcile two interrupts to make look like a single one for a random, pre-existing driver?

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?

We reject it in the initial version of the patch set as there's no consensus yet on some details of the workaround needed for EDGE_BOTH support.

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

There is a consensus: The HW doesn't support this feature.

Means what? There is no acceptable way to support EDGE_BOTH on this HW? In this case I could stop here as for me this feature is important.

Answer my question above, which I asked in my initial review: How do you make two interrupts appear as one for a driver that wants to get signaled on each edge, using the existing API.

Please see the pinctrl/gpio part of the patch set.

The GPIO controller has an own IRQ domain. When requesting an interrupt in request_resources if needed two parent irq's are allocated, (was removed in the current initial version of the patch set) both calling the same ISR via a chained interrupt.

Quoted 1 line

Works perfectly fine here.

Are you referring to the horror that performs interrupt allocations from within the irq_set_type callback? No way. That's beyond disgusting. And potentially broken, as the locks that are being taken were never designed to nest that way.

	M.

Am 15.06.2017 um 18:58 schrieb Marc Zyngier:

Quoted 168 lines · Heiner Kallweit

Quoted 166 lines

Am 15.06.2017 um 18:04 schrieb Marc Zyngier:

Quoted 148 lines · Heiner Kallweit

Quoted 130 lines

Am 15.06.2017 um 15:27 schrieb Marc Zyngier:

Quoted 92 lines · Heiner Kallweit

Quoted 91 lines

Am 13.06.2017 um 10:31 schrieb Marc Zyngier:

Quoted 90 lines · Heiner Kallweit

Quoted 74 lines

Add a driver supporting the GPIO interrupt controller on certain Amlogic meson SoC's.

Signed-off-by Heiner Kallweit <[email protected]>

---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used v6:
- change DT property parent-interrupts to interrupts v7:
- no changes

---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

drivers/irqchip/Kconfig +5 -0

index 478f8ace..bdc86e14 100644
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y

drivers/irqchip/Makefile +1 -0

index b64c59b8..1be482bd 100644
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o

drivers/irqchip/irq-meson-gpio.c new file +26 -0

new file mode 100644
index 00000000..925d00c2
@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <[email protected]>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <[email protected]>
+ * Copyright (c) 2017 Heiner Kallweit <[email protected]>
+ *
+ * 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, version 2.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define REG_EDGE_POL		0x00
+#define REG_PIN_03_SEL		0x04
+#define REG_PIN_47_SEL		0x08

drivers/irqchip/irq-meson-gpio.c line 36

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -0

+#define REG_FILTER_SEL		0x0c
+
+#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
+#define REG_EDGE_POL_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
+
+#define MAX_PARENT_IRQ_NUM	8
+
+/* maximum number of GPIO IRQs on supported platforms */
+#define MAX_NUM_GPIO_IRQ	133

Why aren't these values coming from DT? I bet that a future revision of the same HW will double these. Or at least, you could match it on the compatible string.

Quoted 9 lines

Quoted 2 lines

Alternatively this value can be set to 255. The GPIO source is an 8 bit value with 255 being reserved for "no interrupt source assigned".

Who is reserving it? The HW? Or is that your own defined convention?

Quoted 1 line

This way we cover all chips based on the same IP.

Why? Where is that 8bit limit coming from?

The 8 bit limit is in the HW.

Quoted 19 lines

Quoted 2 lines

I think what we could gain by introducing an additional DT property (saving a few bytes in the irqdomain mapping table) isn't worth the effort.

It is not about saving or wasting memory. It is about making the driver and its binding able to span multiple generation of the HW without too much churn. Which is why I'm suggesting that you either define these properties in DT *or* match the compatible string to obtain these values.

Quoted 11 lines

Quoted 11 lines

drivers/irqchip/irq-meson-gpio.c line 43

Quoted 7 lines

drivers/irqchip/irq-meson-gpio.c new file +7 -0

+
+/*
+ * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
+ * edge. That's due to HW constraints.
+ * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
+ * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
+ * one bit to the right.

Please expand on how you expect this to work, specially when a random driver expects a single interrupt.

Quoted 7 lines

Quoted 3 lines

The gpio interrupt controller in this chip doesn't have native support for IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio to two parent interrupts, one for each edge.

No, that's horrible, racy, and impractical. It has been proposed in the past (for the same HW), and we're not going there again.

Quoted 6 lines

IIRC what has been proposed before is to re-program the polarity of edge detection withing the ISR. This would match your concern that it is racy.

Here it's about using two parent irq's, one programmed to react on the rising edge whilst the other is triggered in case of falling edge. Would you consider this to be racy too?

How do you reconcile two interrupts to make look like a single one for a random, pre-existing driver?

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?

We reject it in the initial version of the patch set as there's no consensus yet on some details of the workaround needed for EDGE_BOTH support.

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

There is a consensus: The HW doesn't support this feature.

Means what? There is no acceptable way to support EDGE_BOTH on this HW? In this case I could stop here as for me this feature is important.

Answer my question above, which I asked in my initial review: How do you make two interrupts appear as one for a driver that wants to get signaled on each edge, using the existing API.

Please see the pinctrl/gpio part of the patch set.

The GPIO controller has an own IRQ domain. When requesting an interrupt in request_resources if needed two parent irq's are allocated, (was removed in the current initial version of the patch set) both calling the same ISR via a chained interrupt.

Works perfectly fine here.

Quoted 5 lines

Are you referring to the horror that performs interrupt allocations from within the irq_set_type callback? No way. That's beyond disgusting. And potentially broken, as the locks that are being taken were never designed to nest that way.

No, this horror was the first attempt. In v7 of the patch set this is done from the request_resources callback.