pinctrl: meson: add support for GPIO IRQs

From: Jerome Brunet <[email protected]>
Add GPIO interrupt information to pinctrl data. Added to the original version from Jerome was data for Meson GXL.

Signed-off-by Jerome Brunet <[email protected]>

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

---
v2:
- no changes
v3:
- no changes
v4:
- no changes
v5:
- added Acked-by
v6:
- no changes

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

Acked-by Neil Armstrong <[email protected]>

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

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

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
---

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

+++++++++++++++++++++++++++++++++++++++
?3 files changed, 302 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..b09228ea

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 +22 -0

+}
+
+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)

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

+
+	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 +52 -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)
+{

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

+};
+
+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 79 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

---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295
+++++++++++++++++++++++++++++++++++++++
 3 files changed, 302 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..b09228ea

drivers/irqchip/irq-meson-gpio.c new file +30 -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)

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

Quoted 6 lines

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

+#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

Only valid on gxbb. Not gxl nor meson8

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)

PSB

Quoted 63 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 line 112

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -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);

Access to the registers should be somehow protected. Several irqs use the same registers. There is a potential race condition here.

Quoted 22 lines

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

+}
+
+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)

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

Quoted 10 lines

drivers/irqchip/irq-meson-gpio.c new file +10 -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);

Same here.

Quoted 13 lines

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

+
+	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);
+}
+
+static unsigned int meson_irq_startup(struct irq_data *data)
+{
+	irq_chip_unmask_parent(data);
+	/*

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

Quoted 5 lines

drivers/irqchip/irq-meson-gpio.c new file +5 -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);

Please keep in mind that any device can use this controller as irq parent. It has to make sense, even when not serving the gpio driver.

This hack means that, in DT, we'd have to multiply by 2 the values given under section "22.3 GPIO interrupts" of the datasheet. This is an example Linux specific stuff in DT.
It also means that the controller declares a lot more lines that it really has
...

This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the mapping from the irq_set_type callback of the GPIO driver, which is still think should be dropped at this point.

Quoted 57 lines

drivers/irqchip/irq-meson-gpio.c new file +57 -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);
+}

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

Quoted 5 lines

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

+
+static const struct irq_domain_ops meson_irq_ops = {
+	.alloc = meson_irq_alloc,
+	.free = meson_irq_free,
+	.xlate = irq_domain_xlate_twocell,

This is a hierarchic domain, shouldn't it implement translate rather than xlate ?

Quoted 73 lines

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

+};
+
+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 4 lines · Heiner Kallweit

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

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

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

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c new file +9 -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);

Please keep in mind that any device can use this controller as irq parent. It has to make sense, even when not serving the gpio driver.

This hack means that, in DT, we'd have to multiply by 2 the values given under section "22.3 GPIO interrupts" of the datasheet. This is an example Linux specific stuff in DT.
It also means that the controller declares a lot more lines that it really has
...

This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the mapping from the irq_set_type callback of the GPIO driver, which is still think should be dropped at this point.

Quoted 5 lines · Jerome Brunet

Quoted 4 lines · Heiner Kallweit

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

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

[...]

Quoted 19 lines

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

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c new file +9 -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);

Please keep in mind that any device can use this controller as irq parent. It has to make sense, even when not serving the gpio driver.

This hack means that, in DT, we'd have to multiply by 2 the values given under section "22.3 GPIO interrupts" of the datasheet. This is an example Linux specific stuff in DT.
It also means that the controller declares a lot more lines that it really has
...

Quoted 3 lines

This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the mapping from the irq_set_type callback of the GPIO driver, which is still think should be dropped at this point.

+1

Please drop the hack for IRQ_TYPE_BOTH so we can reach agreement on the basic design and functionality. The gymnastics required to support this hack (due to broken hardware) are getting in the way of getting basic functionality merged.

Also, we already know from previous discussions between Jerome and the IRQ maintainers that hacks for IRQ_TYPE_BOTH like this have been very thoroughly rejected. So while the IRQ maintainers haven't chimed in on this series, we have a very good idea what they will say when they do.

So please, pretty please, let's avoid giving the IRQ maintainers a fun reason to NAK, and drop the IRQ_TYPE_BOTH stuff until we have an agreed upon way to support the hardware that actually works.

Thanks,

Kevin

Am 09.06.2017 um 23:15 schrieb Kevin Hilman:

Quoted 35 lines

Quoted 5 lines · Jerome Brunet

Quoted 4 lines · Heiner Kallweit

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

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

[...]

Quoted 19 lines

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

Quoted 9 lines

drivers/irqchip/irq-meson-gpio.c new file +9 -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);

Please keep in mind that any device can use this controller as irq parent. It has to make sense, even when not serving the gpio driver.

This hack means that, in DT, we'd have to multiply by 2 the values given under section "22.3 GPIO interrupts" of the datasheet. This is an example Linux specific stuff in DT.
It also means that the controller declares a lot more lines that it really has
...

Quoted 3 lines

This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the mapping from the irq_set_type callback of the GPIO driver, which is still think should be dropped at this point.

+1

Quoted 5 lines

Please drop the hack for IRQ_TYPE_BOTH so we can reach agreement on the basic design and functionality. The gymnastics required to support this hack (due to broken hardware) are getting in the way of getting basic functionality merged.

I haven't heard any feedback on the other proposal yet:
Always reserve two parent irq's in the request_resources callback, then set_type is easy. How about this one?

Having max. 4 gpio irq's should be a fair price for a cleaner design.

Add support for GPIO interrupts and make use of the just introduced irqchip driver handling the GPIO interrupt-controller.

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

---
v5:
- changed Kconfig entry based on Neil's suggestion
- extended comments
- fixed indentation
v6:
- no changes
---
?drivers/pinctrl/Kconfig???????????????|???1 + ?drivers/pinctrl/meson/pinctrl-meson.c | 170
+++++++++++++++++++++++++++++++++-
?2 files changed, 170 insertions(+), 1 deletion(-)

drivers/pinctrl/Kconfig +0 -0

index 7ae04a97..86834dea 100644
@@ -166,6 +166,7 @@ config PINCTRL_MESON

? select PINCONF
? select GENERIC_PINCONF
? select GPIOLIB

drivers/pinctrl/Kconfig +1 -0

+	select GPIOLIB_IRQCHIP

? select OF_GPIO
? select REGMAP_MMIO
?

drivers/pinctrl/meson/pinctrl-meson.c +0 -0

index 66ed70c1..7bacd4e3 100644
@@ -62,6 +62,8 @@

?#include "../pinctrl-utils.h"
?#include "pinctrl-meson.h"
?

drivers/pinctrl/meson/pinctrl-meson.c +2 -0

+static struct irq_domain *meson_pinctrl_irq_domain;
+

?/**
? * meson_get_bank() - find the bank containing a given pin ? *

drivers/pinctrl/meson/pinctrl-meson.c +0 -0

@@ -497,6 +499,154 @@ static int meson_gpio_get(struct gpio_chip *chip,
 unsigned gpio)
 ?	return !!(val & BIT(bit));
 ?}

?

drivers/pinctrl/meson/pinctrl-meson.c +8 -0

+static struct meson_pinctrl *meson_gpio_data_to_pc(struct irq_data *data)
+{
+????????struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+
+????????return gpiochip_get_data(chip);
+}
+
+static int meson_gpio_bank_hwirq(struct meson_bank *bank, unsigned int

offset)

drivers/pinctrl/meson/pinctrl-meson.c +78 -0

+{
+????????int hwirq;
+
+????????if (bank->irq_first < 0)
+????????????????/* this bank cannot generate irqs */
+????????????????return 0;
+
+????????hwirq = offset - bank->first + bank->irq_first;
+
+????????if (hwirq > bank->irq_last)
+????????????????/* this pin cannot generate irqs */
+????????????????return 0;
+
+????????return hwirq;
+}
+
+static int meson_gpio_to_hwirq(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	unsigned int offset = data->hwirq;
+????????struct meson_bank *bank;
+????????int hwirq, ret;
+
+????????offset += pc->data->pin_base;
+
+????????ret = meson_get_bank(pc, offset, &bank);
+????????if (ret)
+????????????????return ret;
+
+????????hwirq = meson_gpio_bank_hwirq(bank, offset);
+????????if (!hwirq)
+????????????????dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
+
+????????return hwirq;
+}
+
+static void meson_gpio_irq_handler(struct irq_desc *desc)
+{
+????????struct irq_chip *chip = irq_desc_get_chip(desc);
+????????struct irq_data *gpio_irq_data = irq_desc_get_handler_data(desc);
+
+????????chained_irq_enter(chip, desc);
+
+????????if (gpio_irq_data)
+????????????????generic_handle_irq(gpio_irq_data->irq);
+
+????????chained_irq_exit(chip, desc);
+}
+
+static void meson_gpio_irq_unmask(struct irq_data *data) {}
+static void meson_gpio_irq_mask(struct irq_data *data) {}
+
+static void meson_gpio_irq_shutdown(struct irq_data *data)
+{
+	int hwirq = meson_gpio_to_hwirq(data);
+	int irq;
+
+	if (hwirq <= 0)
+		return;
+
+	/*
+	?* 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.
+	?*/
+	irq = irq_find_mapping(meson_pinctrl_irq_domain, hwirq * 2);
+	if (irq) {
+		irq_set_chained_handler_and_data(irq, handle_bad_irq, NULL);
+		irq_domain_free_irqs(irq, 1);
+	}
+
+	irq = irq_find_mapping(meson_pinctrl_irq_domain, hwirq * 2 + 1);
+	if (irq) {
+		irq_set_chained_handler_and_data(irq, handle_bad_irq, NULL);
+		irq_domain_free_irqs(irq, 1);
+	}

drivers/pinctrl/meson/pinctrl-meson.c +18 -0

+
+	if (hwirq < 0)
+		return hwirq;
+
+	if (!hwirq)
+		return -ENXIO;
+
+	fwspec.fwnode = meson_pinctrl_irq_domain->fwnode;
+	fwspec.param_count = 2;
+
+	/*
+	?* The chip can create an interrupt for either rising or falling edge
+	?* only. Therefore use two interrupts in case of IRQ_TYPE_EDGE_BOTH,
+	?* first for falling edge and second one for rising edge.
+	?*/
+	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
+
+	/* see comment in meson_gpio_irq_shutdown why we shift one bit here

*/

drivers/pinctrl/meson/pinctrl-meson.c +21 -0

+	fwspec.param[0] = hwirq << 1;
+	if (num_slots == 1)
+		fwspec.param[1] = type;
+	else
+		fwspec.param[1] = IRQ_TYPE_EDGE_FALLING;
+
+	irq = irq_create_fwspec_mapping(&fwspec);
+	if (!irq)
+		return -EINVAL;
+
+	irq_set_chained_handler_and_data(irq, meson_gpio_irq_handler, data);
+
+	if (num_slots > 1) {
+		fwspec.param[0]++;
+		fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
+		irq2 = irq_create_fwspec_mapping(&fwspec);
+		if (!irq2) {
+			irq_domain_free_irqs(irq, 1);
+			return -EINVAL;
+		}
+		irq_set_chained_handler_and_data(irq2,

meson_gpio_irq_handler, data);

drivers/pinctrl/meson/pinctrl-meson.c +13 -0

+	}
+
+????????return 0;
+}
+
+static struct irq_chip meson_gpio_irq_chip = {
+????????.name = "GPIO",
+????????.irq_set_type = meson_gpio_irq_set_type,
+????????.irq_mask = meson_gpio_irq_mask,
+????????.irq_unmask = meson_gpio_irq_unmask,
+????????.irq_shutdown = meson_gpio_irq_shutdown,
+};
+

?static const struct of_device_id meson_pinctrl_dt_match[] = {
? {
? .compatible = "amlogic,meson8-cbus-pinctrl",

drivers/pinctrl/meson/pinctrl-meson.c +0 -0

@@ -558,7 +708,8 @@ static int meson_gpiolib_register(struct meson_pinctrl
 *pc)
 ?		return ret;
 ?	}

?

drivers/pinctrl/meson/pinctrl-meson.c +2 -1

-	return 0;
+	return gpiochip_irqchip_add(&pc->chip, &meson_gpio_irq_chip, 0,
+				????handle_simple_irq, IRQ_TYPE_NONE);

?}
?
?static struct regmap_config meson_regmap_config = {

drivers/pinctrl/meson/pinctrl-meson.c +0 -0

@@ -637,6 +788,23 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl
 *pc,
 ?		return PTR_ERR(pc->reg_gpio);
 ?	}

?

drivers/pinctrl/meson/pinctrl-meson.c +1 -0

+		np = of_find_compatible_node(NULL, NULL, "amlogic,meson-gpio-

intc");

drivers/pinctrl/meson/pinctrl-meson.c +2 -0

+		if (!np) {
+			dev_err(pc->dev, "interrupt controller DT node not

found\n");

drivers/pinctrl/meson/pinctrl-meson.c +13 -0

+			return -EINVAL;
+		}
+
+		meson_pinctrl_irq_domain = irq_find_host(np);
+		if (!meson_pinctrl_irq_domain) {
+			dev_err(pc->dev, "interrupt controller not found\n");
+			of_node_put(np);
+			return -EINVAL;
+		}
+
+		of_node_put(np);
+	}
+

? return 0;
?}
?

Quoted 135 lines

Add support for GPIO interrupts and make use of the just introduced irqchip driver handling the GPIO interrupt-controller.

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

---
v5:
- changed Kconfig entry based on Neil's suggestion
- extended comments
- fixed indentation
v6:
- no changes

---
 drivers/pinctrl/Kconfig               |   1 +
 drivers/pinctrl/meson/pinctrl-meson.c | 170
+++++++++++++++++++++++++++++++++-
 2 files changed, 170 insertions(+), 1 deletion(-)

drivers/pinctrl/Kconfig +1 -0

index 7ae04a97..86834dea 100644
@@ -166,6 +166,7 @@ config PINCTRL_MESON
 	select PINCONF
 	select GENERIC_PINCONF
 	select GPIOLIB
+	select GPIOLIB_IRQCHIP
 	select OF_GPIO
 	select REGMAP_MMIO
 

drivers/pinctrl/meson/pinctrl-meson.c +88 -0

index 66ed70c1..7bacd4e3 100644
@@ -62,6 +62,8 @@
 #include "../pinctrl-utils.h"
 #include "pinctrl-meson.h"
 
+static struct irq_domain *meson_pinctrl_irq_domain;
+
 /**
  * meson_get_bank() - find the bank containing a given pin
  *

@@ -497,6 +499,154 @@ static int meson_gpio_get(struct gpio_chip *chip,
 unsigned gpio)
 	return !!(val & BIT(bit));
 }
 
+static struct meson_pinctrl *meson_gpio_data_to_pc(struct irq_data *data)
+{
+        struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+
+        return gpiochip_get_data(chip);
+}
+
+static int meson_gpio_bank_hwirq(struct meson_bank *bank, unsigned int
 offset)
+{
+        int hwirq;
+
+        if (bank->irq_first < 0)
+                /* this bank cannot generate irqs */
+                return 0;
+
+        hwirq = offset - bank->first + bank->irq_first;
+
+        if (hwirq > bank->irq_last)
+                /* this pin cannot generate irqs */
+                return 0;
+
+        return hwirq;
+}
+
+static int meson_gpio_to_hwirq(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	unsigned int offset = data->hwirq;
+        struct meson_bank *bank;
+        int hwirq, ret;
+
+        offset += pc->data->pin_base;
+
+        ret = meson_get_bank(pc, offset, &bank);
+        if (ret)
+                return ret;
+
+        hwirq = meson_gpio_bank_hwirq(bank, offset);
+        if (!hwirq)
+                dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
+
+        return hwirq;
+}
+
+static void meson_gpio_irq_handler(struct irq_desc *desc)
+{
+        struct irq_chip *chip = irq_desc_get_chip(desc);
+        struct irq_data *gpio_irq_data = irq_desc_get_handler_data(desc);
+
+        chained_irq_enter(chip, desc);
+
+        if (gpio_irq_data)
+                generic_handle_irq(gpio_irq_data->irq);
+
+        chained_irq_exit(chip, desc);
+}
+
+static void meson_gpio_irq_unmask(struct irq_data *data) {}
+static void meson_gpio_irq_mask(struct irq_data *data) {}
+
+static void meson_gpio_irq_shutdown(struct irq_data *data)
+{
+	int hwirq = meson_gpio_to_hwirq(data);
+	int irq;
+
+	if (hwirq <= 0)
+		return;
+
+	/*
+	 * 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.
+	 */
+	irq = irq_find_mapping(meson_pinctrl_irq_domain, hwirq * 2);
+	if (irq) {
+		irq_set_chained_handler_and_data(irq, handle_bad_irq, NULL);
+		irq_domain_free_irqs(irq, 1);
+	}
+
+	irq = irq_find_mapping(meson_pinctrl_irq_domain, hwirq * 2 + 1);
+	if (irq) {
+		irq_set_chained_handler_and_data(irq, handle_bad_irq, NULL);
+		irq_domain_free_irqs(irq, 1);
+	}

drivers/pinctrl/meson/pinctrl-meson.c static int meson_gpio_get(struct gpio_chip *chip, · line 599

Quoted 10 lines

drivers/pinctrl/meson/pinctrl-meson.c +10 -0

+}
+
+static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	int hwirq = meson_gpio_to_hwirq(data);
+	struct irq_fwspec fwspec;
+        int irq, irq2, num_slots;
+
+	if (irqd_is_activated(data))
+		return -EBUSY;

Again, your implementation assume that:
* irq_set_type should be called only once in the life of the irq.
* and/or irq_set_type cannot be called from irq context

I don't agree with those assumptions.
If irq_set_type could not called several times, to change the type at runtime, why would the irq framework bother providing "irq_set_irq_type" in the public API ? Why bother with irq_set_type callback at all ? It could all be done in the startup callback...

Quoted 74 lines

drivers/pinctrl/meson/pinctrl-meson.c +39 -0

+
+	if (hwirq < 0)
+		return hwirq;
+
+	if (!hwirq)
+		return -ENXIO;
+
+	fwspec.fwnode = meson_pinctrl_irq_domain->fwnode;
+	fwspec.param_count = 2;
+
+	/*
+	 * The chip can create an interrupt for either rising or falling edge
+	 * only. Therefore use two interrupts in case of IRQ_TYPE_EDGE_BOTH,
+	 * first for falling edge and second one for rising edge.
+	 */
+	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
+
+	/* see comment in meson_gpio_irq_shutdown why we shift one bit here
 */
+	fwspec.param[0] = hwirq << 1;
+	if (num_slots == 1)
+		fwspec.param[1] = type;
+	else
+		fwspec.param[1] = IRQ_TYPE_EDGE_FALLING;
+
+	irq = irq_create_fwspec_mapping(&fwspec);
+	if (!irq)
+		return -EINVAL;
+
+	irq_set_chained_handler_and_data(irq, meson_gpio_irq_handler, data);
+
+	if (num_slots > 1) {
+		fwspec.param[0]++;
+		fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
+		irq2 = irq_create_fwspec_mapping(&fwspec);
+		if (!irq2) {
+			irq_domain_free_irqs(irq, 1);
+			return -EINVAL;
+		}
+		irq_set_chained_handler_and_data(irq2,

meson_gpio_irq_handler, data);

drivers/pinctrl/meson/pinctrl-meson.c +15 -1

+	}
+
+        return 0;
+}
+
+static struct irq_chip meson_gpio_irq_chip = {
+        .name = "GPIO",
+        .irq_set_type = meson_gpio_irq_set_type,
+        .irq_mask = meson_gpio_irq_mask,
+        .irq_unmask = meson_gpio_irq_unmask,
+        .irq_shutdown = meson_gpio_irq_shutdown,
+};
+
 static const struct of_device_id meson_pinctrl_dt_match[] = {
 	{
 		.compatible = "amlogic,meson8-cbus-pinctrl",

@@ -558,7 +708,8 @@ static int meson_gpiolib_register(struct meson_pinctrl
 *pc)
 		return ret;
 	}
 
-	return 0;
+	return gpiochip_irqchip_add(&pc->chip, &meson_gpio_irq_chip, 0,
+				    handle_simple_irq, IRQ_TYPE_NONE);
 }
 

static struct regmap_config meson_regmap_config = {

drivers/pinctrl/meson/pinctrl-meson.c +0 -0

@@ -637,6 +788,23 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl
 *pc,
 		return PTR_ERR(pc->reg_gpio);
 	}
 

drivers/pinctrl/meson/pinctrl-meson.c static int meson_pinctrl_parse_dt(struct meson_pinctrl · line 792

Quoted 1 line

drivers/pinctrl/meson/pinctrl-meson.c +1 -0

+	if (!meson_pinctrl_irq_domain) {

This be in " struct meson_pinctrl *pc" not a global.
Each instance should parse its own bindings and not rely on the other one for this !

Quoted 21 lines

drivers/pinctrl/meson/pinctrl-meson.c +16 -0

+		np = of_find_compatible_node(NULL, NULL, "amlogic,meson-gpio-
 intc");
+		if (!np) {
+			dev_err(pc->dev, "interrupt controller DT node not
 found\n");
+			return -EINVAL;
+		}
+
+		meson_pinctrl_irq_domain = irq_find_host(np);
+		if (!meson_pinctrl_irq_domain) {
+			dev_err(pc->dev, "interrupt controller not found\n");
+			of_node_put(np);
+			return -EINVAL;
+		}
+
+		of_node_put(np);
+	}
+

return 0;
}