pintrl: meson: add support for GPIO IRQs

Document the DT binding for GPIO IRQ support on Amlogic Meson SoC's.

This documentation is intentionally not placed under interrupt-controllers as GPIO IRQ support on these SoC's acts more like an interrupt multiplexer.

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

---
v2:
- remove syscon
---
?arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 13 +++++++++++++ ?1 file changed, 13 insertions(+)

arch/arm64/boot/dts/amlogic/meson-gx.dtsi +0 -0

index 436b8750..44422b85 100644
@@ -312,6 +312,19 @@
 ?				status = "disabled";
 ?			};

?

arch/arm64/boot/dts/amlogic/meson-gx.dtsi +3 -0

+			gpio_irq at 9880 {
+				compatible = "amlogic,meson-gpio-interrupt";
+				reg = <0x0 0x09880 0x0 0x10>;

? watchdog at 98d0 {
? compatible = "amlogic,meson-gx-wdt",
"amlogic,meson-gxbb-wdt";
? reg = <0x0 0x098d0 0x0 0x10>;

Quoted 26 lines

Document the DT binding for GPIO IRQ support on Amlogic Meson SoC's.

This documentation is intentionally not placed under interrupt-controllers as GPIO IRQ support on these SoC's acts more like an interrupt multiplexer.

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

---
v2:
- remove syscon

---
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

arch/arm64/boot/dts/amlogic/meson-gx.dtsi +3 -0

index 436b8750..44422b85 100644
@@ -312,6 +312,19 @@
 				status = "disabled";
 			};
 
+			gpio_irq at 9880 {
+				compatible = "amlogic,meson-gpio-interrupt";
+				reg = <0x0 0x09880 0x0 0x10>;

arch/arm64/boot/dts/amlogic/meson-gx.dtsi line 327

Quoted 10 lines

arch/arm64/boot/dts/amlogic/meson-gx.dtsi +10 -0

+				interrupts = <GIC_SPI 64 IRQ_TYPE_NONE>,
+					     <GIC_SPI 65 IRQ_TYPE_NONE>,
+					     <GIC_SPI 66 IRQ_TYPE_NONE>,
+					     <GIC_SPI 67 IRQ_TYPE_NONE>,
+					     <GIC_SPI 68 IRQ_TYPE_NONE>,
+					     <GIC_SPI 69 IRQ_TYPE_NONE>,
+					     <GIC_SPI 70 IRQ_TYPE_NONE>,
+					     <GIC_SPI 71 IRQ_TYPE_NONE>;
+			};
+

Already tried the same thing:
https://marc.info/?l=devicetree&m=147758174404359&w=2

Irq maintainers reminded me that this is not correct as the device is not able to generate these particular irqs (it is merely routing the signal) and the flags are meaning less here

Quoted 5 lines

arch/arm64/boot/dts/amlogic/meson-gx.dtsi +0 -0

 			watchdog at 98d0 {
 				compatible = "amlogic,meson-gx-wdt",
 "amlogic,meson-gxbb-wdt";

reg = <0x0 0x098d0 0x0 0x10>;

Am 17.05.2017 um 01:28 schrieb Jerome Brunet:

Quoted 42 lines · Heiner Kallweit

Quoted 27 lines

Document the DT binding for GPIO IRQ support on Amlogic Meson SoC's.

This documentation is intentionally not placed under interrupt-controllers as GPIO IRQ support on these SoC's acts more like an interrupt multiplexer.

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

---
v2:
- remove syscon
---
?arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 13 +++++++++++++ ?1 file changed, 13 insertions(+)

arch/arm64/boot/dts/amlogic/meson-gx.dtsi +0 -0

index 436b8750..44422b85 100644
@@ -312,6 +312,19 @@
 ?				status = "disabled";
 ?			};

?

arch/arm64/boot/dts/amlogic/meson-gx.dtsi +2 -0

+			gpio_irq at 9880 {
+				compatible = "amlogic,meson-gpio-

interrupt";

arch/arm64/boot/dts/amlogic/meson-gx.dtsi +1 -0

+				reg = <0x0 0x09880 0x0 0x10>;

arch/arm64/boot/dts/amlogic/meson-gx.dtsi

Quoted 10 lines

arch/arm64/boot/dts/amlogic/meson-gx.dtsi +10 -0

+				interrupts = <GIC_SPI 64 IRQ_TYPE_NONE>,
+					?????<GIC_SPI 65 IRQ_TYPE_NONE>,
+					?????<GIC_SPI 66 IRQ_TYPE_NONE>,
+					?????<GIC_SPI 67 IRQ_TYPE_NONE>,
+					?????<GIC_SPI 68 IRQ_TYPE_NONE>,
+					?????<GIC_SPI 69 IRQ_TYPE_NONE>,
+					?????<GIC_SPI 70 IRQ_TYPE_NONE>,
+					?????<GIC_SPI 71 IRQ_TYPE_NONE>;
+			};
+

Already tried the same thing:
https://marc.info/?l=devicetree&m=147758174404359&w=2

Quoted 5 lines

Irq maintainers reminded me that this is not correct as the device is not able
to generate these particular irqs (it is merely routing the signal) and the flags are meaning less here

In one review comment to your patch series Rob mentioned to be fine with using the standard interrupts property.
However other involved people might have had a different opinion, I didn't read
all review comments.

I think the standard interrupts property is the best way.
We need mapped virq's to be used with the irq chaining. And for mapping an interrupt we need not only the hwirq but also the IRQ domain. The interrupts property and the related OF IRQ API calls like irq_of_parse_and_map provide exactly what we need.

If somebody has serious objections to using the interrupts property then we basically had to duplicate it and just give it a different name.

And such code duplication to me seems to be worse than using the interrupts property slightly different than in the theory.

Quoted 5 lines

Quoted 5 lines

? watchdog at 98d0 {
? compatible = "amlogic,meson-gx-wdt",
"amlogic,meson-gxbb-wdt";
? reg = <0x0 0x098d0 0x0 0x10>;

Add support for GPIO interrupts on Amlogic Meson SoC's.

There's a limit of 8 parent interupts which can be used in total. Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's, one for each edge.

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

---
v2:
- make the GPIO IRQ controller a separate driver
- several smaller improvements

---
 drivers/pinctrl/Kconfig                   |   1 +
 drivers/pinctrl/meson/Makefile            |   2 +-
 drivers/pinctrl/meson/pinctrl-meson-irq.c | 367 ++++++++++++++++++++++++++++++

From the GPIO-IRQ Controller perspective, the GPIOs are only a number and the pinmux code

 drivers/pinctrl/meson/pinctrl-meson.c     |   8 +-
 drivers/pinctrl/meson/pinctrl-meson.h     |   1 +
 5 files changed, 377 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pinctrl/meson/pinctrl-meson-irq.c

drivers/pinctrl/Kconfig +1 -0

index 37af5e30..f8f401a0 100644
@@ -153,6 +153,7 @@ config PINCTRL_MESON
 	select PINCONF
 	select GENERIC_PINCONF
 	select GPIOLIB
+	select GPIOLIB_IRQCHIP
 	select OF_GPIO
 	select REGMAP_MMIO
 

drivers/pinctrl/meson/Makefile +1 -1

index 27c5b512..827e416d 100644
@@ -1,3 +1,3 @@
 obj-y	+= pinctrl-meson8.o pinctrl-meson8b.o
 obj-y	+= pinctrl-meson-gxbb.o pinctrl-meson-gxl.o
-obj-y	+= pinctrl-meson.o
+obj-y	+= pinctrl-meson.o pinctrl-meson-irq.o

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +367 -0

new file mode 100644
index 00000000..c5f403f3
@@ -0,0 +1,367 @@
+/*
+ * Amlogic Meson GPIO IRQ driver
+ *
+ * Copyright 2017 Heiner Kallweit <[email protected]>
+ * Based on a first version by Jerome Brunet <[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/gpio.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include "pinctrl-meson.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))
+
+#define MESON_GPIO_MAX_PARENT_IRQ_NUM	8
+
+struct meson_gpio_irq_slot {
+	int irq;
+	int owner;
+};
+
+static struct regmap *meson_gpio_irq_regmap;
+static struct meson_gpio_irq_slot
+		meson_gpio_irq_slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
+static int meson_gpio_num_irq_slots;
+static DEFINE_MUTEX(meson_gpio_irq_slot_mutex);
+static DECLARE_BITMAP(meson_gpio_irq_locked, 256);
+
+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_to_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_irq(struct meson_pinctrl *pc, unsigned int offset)
+{
+	struct meson_bank *bank;
+	int hwirq;
+
+	offset += pc->data->pin_base;
+
+	bank = meson_pinctrl_get_bank(pc, offset);
+	if (IS_ERR(bank))
+		return PTR_ERR(bank);
+
+	hwirq = meson_gpio_to_hwirq(bank, offset);
+	if (!hwirq)
+		dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
+
+	return hwirq;
+}
+
+static int meson_gpio_data_to_hwirq(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	unsigned gpio = irqd_to_hwirq(data);
+
+	return meson_gpio_to_irq(pc, gpio);
+}
+
+static irqreturn_t meson_gpio_irq_handler(int irq, void *data)
+{
+	struct irq_data *gpio_irqdata = data;
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	int hwirq = meson_gpio_data_to_hwirq(gpio_irqdata);
+
+	/*
+	 * For some strange reason spurious interrupts created by the chip when
+	 * the interrupt source registers are written cause a deadlock here.
+	 * generic_handle_irq calls handle_simple_irq which tries to get
+	 * spinlock desc->lock. This interrupt handler is called whilst
+	 * __setup_irq holds desc->lock.
+	 * The deadlock means that both are running on the same CPU what should
+	 * not happen as __setup_irq called raw_spin_lock_irqsave thus disabling
+	 * interrupts on this CPU.
+	 * Work around this by ignoring interrupts in code protected by
+	 * chip_bus_lock (__setup_irq/__free_irq for the respective GPIO hwirq).
+	 */
+	if (test_bit(hwirq, meson_gpio_irq_locked))
+		dev_dbg(pc->dev, "spurious interrupt detected!\n");
+	else
+		generic_handle_irq(gpio_irqdata->irq);
+
+	return IRQ_HANDLED;
+}
+
+static int meson_gpio_alloc_irq_slot(struct irq_data *data, int num_slots,
+				     int *slots)
+{
+	int i, cnt = 0;
+
+	mutex_lock(&meson_gpio_irq_slot_mutex);
+
+	for (i = 0; i < meson_gpio_num_irq_slots; i++)
+		if (!meson_gpio_irq_slots[i].owner) {
+			meson_gpio_irq_slots[i].owner = data->irq;
+			slots[cnt++] = i;
+			if (cnt == num_slots)
+				break;
+		}
+
+	if (cnt < num_slots)
+		for (i = 0; i < cnt; i++)
+			meson_gpio_irq_slots[i].owner = 0;
+
+	mutex_unlock(&meson_gpio_irq_slot_mutex);
+
+	return cnt == num_slots ? 0 : -ENOSPC;
+}
+
+static void meson_gpio_free_irq_slot(struct irq_data *data)
+{
+	int i;
+
+	mutex_lock(&meson_gpio_irq_slot_mutex);
+
+	for (i = 0; i < meson_gpio_num_irq_slots; i++)
+		if (meson_gpio_irq_slots[i].owner == data->irq) {
+			free_irq(meson_gpio_irq_slots[i].irq, data);
+			meson_gpio_irq_slots[i].owner = 0;
+		}
+
+	mutex_unlock(&meson_gpio_irq_slot_mutex);
+}
+
+static int meson_gpio_find_irq_slot(struct irq_data *data, int *slots)
+{
+	int i, cnt = 0;
+
+	mutex_lock(&meson_gpio_irq_slot_mutex);
+
+	for (i = 0; i < meson_gpio_num_irq_slots; i++)
+		if (meson_gpio_irq_slots[i].owner == data->irq)
+			slots[cnt++] = i;
+
+	mutex_unlock(&meson_gpio_irq_slot_mutex);
+
+	return cnt ?: -EINVAL;
+}
+
+static void meson_gpio_set_hwirq(int idx, int hwirq)
+{
+	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
+	int shift = 8 * (idx % 4);
+
+	regmap_update_bits(meson_gpio_irq_regmap, reg, 0xff << shift,
+			   hwirq << shift);
+}
+
+static void meson_gpio_irq_set_hwirq(struct irq_data *data, int hwirq)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	int i, cnt, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
+
+	cnt = meson_gpio_find_irq_slot(data, slots);
+	if (cnt < 0) {
+		dev_err(pc->dev, "didn't find gpio irq slot\n");
+		return;
+	}
+
+	for (i = 0; i < cnt; i++)
+		meson_gpio_set_hwirq(slots[i], hwirq);
+}
+
+static void meson_gpio_irq_unmask(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	unsigned gpio = irqd_to_hwirq(data);
+	int hwirq = meson_gpio_to_irq(pc, gpio);
+
+	meson_gpio_irq_set_hwirq(data, hwirq);
+}
+
+static void meson_gpio_irq_mask(struct irq_data *data)
+{
+	meson_gpio_irq_set_hwirq(data, 0xff);
+}
+
+static void meson_gpio_irq_shutdown(struct irq_data *data)
+{
+	meson_gpio_irq_mask(data);
+	meson_gpio_free_irq_slot(data);
+}
+
+static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	int i, ret, irq, num_slots, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
+	unsigned int val = 0;
+
+	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
+	ret = meson_gpio_alloc_irq_slot(data, num_slots, slots);
+	if (ret)
+		return ret;
+
+	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_EDGE(slots[0]);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_LOW(slots[0]);
+
+	regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
+			   REG_EDGE_POL_MASK(slots[0]), val);
+
+	/*
+	 * 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.
+	 */
+	if (num_slots > 1) {
+		val = REG_EDGE_POL_EDGE(slots[1]);
+		regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
+				   REG_EDGE_POL_MASK(slots[1]), val);
+	}
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val = IRQ_TYPE_EDGE_RISING;
+	else
+		val = IRQ_TYPE_LEVEL_HIGH;
+
+	for (i = 0; i < num_slots; i++) {
+		irq = meson_gpio_irq_slots[slots[i]].irq;
+		ret = irq_set_irq_type(irq, val);
+		if (ret)
+			break;
+		ret = request_irq(irq, meson_gpio_irq_handler, 0,
+				  "GPIO parent", data);
+		if (ret)
+			break;
+	}
+
+	if (ret)
+		while (--i >= 0)
+			free_irq(meson_gpio_irq_slots[slots[i]].irq, data);
+
+	return ret;
+}
+
+static void meson_gpio_irq_bus_lock(struct irq_data *data)
+{
+	int hwirq = meson_gpio_data_to_hwirq(data);
+
+	set_bit(hwirq, meson_gpio_irq_locked);
+}
+
+static void meson_gpio_irq_bus_sync_unlock(struct irq_data *data)
+{
+	int hwirq = meson_gpio_data_to_hwirq(data);
+
+	clear_bit(hwirq, meson_gpio_irq_locked);
+}
+
+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,
+	.irq_bus_lock = meson_gpio_irq_bus_lock,
+	.irq_bus_sync_unlock = meson_gpio_irq_bus_sync_unlock,
+};
+
+static int meson_gpio_get_irqs(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int irq, i;
+
+	for (i = 0; i < MESON_GPIO_MAX_PARENT_IRQ_NUM; i++) {
+		irq = irq_of_parse_and_map(np, i);
+		if (!irq)
+			break;
+		meson_gpio_irq_slots[i].irq = irq;
+	}
+
+	meson_gpio_num_irq_slots = i;
+
+	return i ? 0 : -EINVAL;
+}
+
+static const struct regmap_config meson_gpio_regmap_config = {
+	.reg_bits       = 32,
+	.reg_stride     = 4,
+	.val_bits       = 32,
+	.max_register	= REG_FILTER_SEL,
+};
+
+static int meson_gpio_irq_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	void __iomem *io_base;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	io_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(io_base))
+		return PTR_ERR(io_base);
+
+	meson_gpio_irq_regmap = devm_regmap_init_mmio(&pdev->dev, io_base,
+						&meson_gpio_regmap_config);
+	if (IS_ERR(meson_gpio_irq_regmap))
+		return PTR_ERR(meson_gpio_irq_regmap);
+
+	/* initialize to IRQ_TYPE_LEVEL_HIGH */
+	regmap_write(meson_gpio_irq_regmap, REG_EDGE_POL, 0);
+	/* disable all GPIO interrupt sources */
+	regmap_write(meson_gpio_irq_regmap, REG_PIN_03_SEL, 0xffffffff);
+	regmap_write(meson_gpio_irq_regmap, REG_PIN_47_SEL, 0xffffffff);
+	/* disable filtering */
+	regmap_write(meson_gpio_irq_regmap, REG_FILTER_SEL, 0);
+
+	ret = meson_gpio_get_irqs(pdev);
+	if (ret)
+		return ret;
+
+	meson_pinctrl_irq_chip = &meson_gpio_irq_chip;
+
+	return 0;
+}
+
+static const struct of_device_id meson_gpio_irq_dt_match[] = {
+	{ .compatible = "amlogic,meson-gpio-interrupt" },
+	{ },
+};
+
+static struct platform_driver meson_gpio_irq_driver = {
+	.probe		= meson_gpio_irq_probe,
+	.driver = {
+		.name	= "meson-gpio-interrupt",
+		.of_match_table = meson_gpio_irq_dt_match,
+	},
+};
+builtin_platform_driver(meson_gpio_irq_driver);

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

index 39ad9861..c587f6f0 100644
@@ -62,6 +62,8 @@
 #include "../pinctrl-utils.h"
 #include "pinctrl-meson.h"
 
+struct irq_chip *meson_pinctrl_irq_chip;
+
 /**
  * meson_pinctrl_get_bank() - find the bank containing a given pin
  *

@@ -551,7 +553,8 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc)
 		return ret;
 	}
 
-	return 0;
+	return gpiochip_irqchip_add(&pc->chip, meson_pinctrl_irq_chip, 0,
+				    handle_simple_irq, IRQ_TYPE_NONE);
 }
 
 static struct regmap_config meson_regmap_config = {

@@ -640,6 +643,9 @@ static int meson_pinctrl_probe(struct platform_device *pdev)
 	struct meson_pinctrl *pc;
 	int ret;
 
+	if (!meson_pinctrl_irq_chip)
+		return -EPROBE_DEFER;
+
 	pc = devm_kzalloc(dev, sizeof(struct meson_pinctrl), GFP_KERNEL);
 	if (!pc)
 		return -ENOMEM;

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

index 40b56aff..16aab328 100644
@@ -176,6 +176,7 @@ extern struct meson_pinctrl_data meson_gxbb_periphs_pinctrl_data;
 extern struct meson_pinctrl_data meson_gxbb_aobus_pinctrl_data;
 extern struct meson_pinctrl_data meson_gxl_periphs_pinctrl_data;
 extern struct meson_pinctrl_data meson_gxl_aobus_pinctrl_data;
+extern struct irq_chip *meson_pinctrl_irq_chip;
 
 struct meson_bank *meson_pinctrl_get_bank(const struct meson_pinctrl *pc,
 					  unsigned int pin);

Quoted 7 lines

Add support for GPIO interrupts on Amlogic Meson SoC's.

There's a limit of 8 parent interupts which can be used in total. Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's, one for each edge.

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

Quoted 8 lines

---
v2:
- make the GPIO IRQ controller a separate driver
- several smaller improvements

---
 drivers/pinctrl/Kconfig                   |   1 +
 drivers/pinctrl/meson/Makefile            |   2 +-
 drivers/pinctrl/meson/pinctrl-meson-irq.c | 367 ++++++++++++++++++++++++++++++

Hi Heiner,

This code has nothing to do with GPIOs on pinmux handling here, what we figured with Jerome is that this must be an independent IRQ Controller in drivers/irqchip.

Please move it and make independent, you should be able to request irqs without any links to the pinmux/gpio since physically the GPIO lines input are always connected to this irq controller, and the pinmux has no impact on the interrupt management here.

Quoted 1 line

From the GPIO-IRQ Controller perspective, the GPIOs are only a number and the pinmux code

is only here to make the translation to a specific GPIO to a GPIO-IRQ number.

For more chance to have it upstreamed in the right way, we should :
1) Collaborate, we can chat over IRC, maybe Slack, E-mail, skype, forums, ...
2) Push an independent IRQ controller that matches the capacity of the HW
3) Push a link from the pinctrl driver to have the to_gpio_irq mapping done in the right way

Jerome spent quite a lot of time and had a chat with the IRQ subsystem maintainers to have s clear image of how this should be implemented, and it would be a good point to actually have a chat with them to elaborate find a strong solution.

I'm sorry to say that pushing this code without really understanding how and why will lead to nothing expect frustration from everybody.

Neil

Quoted 445 lines

 drivers/pinctrl/meson/pinctrl-meson.c     |   8 +-
 drivers/pinctrl/meson/pinctrl-meson.h     |   1 +
 5 files changed, 377 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pinctrl/meson/pinctrl-meson-irq.c

drivers/pinctrl/Kconfig +1 -0

index 37af5e30..f8f401a0 100644
@@ -153,6 +153,7 @@ config PINCTRL_MESON
 	select PINCONF
 	select GENERIC_PINCONF
 	select GPIOLIB
+	select GPIOLIB_IRQCHIP
 	select OF_GPIO
 	select REGMAP_MMIO
 

drivers/pinctrl/meson/Makefile +1 -1

index 27c5b512..827e416d 100644
@@ -1,3 +1,3 @@
 obj-y	+= pinctrl-meson8.o pinctrl-meson8b.o
 obj-y	+= pinctrl-meson-gxbb.o pinctrl-meson-gxl.o
-obj-y	+= pinctrl-meson.o
+obj-y	+= pinctrl-meson.o pinctrl-meson-irq.o

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +367 -0

new file mode 100644
index 00000000..c5f403f3
@@ -0,0 +1,367 @@
+/*
+ * Amlogic Meson GPIO IRQ driver
+ *
+ * Copyright 2017 Heiner Kallweit <[email protected]>
+ * Based on a first version by Jerome Brunet <[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/gpio.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include "pinctrl-meson.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))
+
+#define MESON_GPIO_MAX_PARENT_IRQ_NUM	8
+
+struct meson_gpio_irq_slot {
+	int irq;
+	int owner;
+};
+
+static struct regmap *meson_gpio_irq_regmap;
+static struct meson_gpio_irq_slot
+		meson_gpio_irq_slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
+static int meson_gpio_num_irq_slots;
+static DEFINE_MUTEX(meson_gpio_irq_slot_mutex);
+static DECLARE_BITMAP(meson_gpio_irq_locked, 256);
+
+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_to_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_irq(struct meson_pinctrl *pc, unsigned int offset)
+{
+	struct meson_bank *bank;
+	int hwirq;
+
+	offset += pc->data->pin_base;
+
+	bank = meson_pinctrl_get_bank(pc, offset);
+	if (IS_ERR(bank))
+		return PTR_ERR(bank);
+
+	hwirq = meson_gpio_to_hwirq(bank, offset);
+	if (!hwirq)
+		dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
+
+	return hwirq;
+}
+
+static int meson_gpio_data_to_hwirq(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	unsigned gpio = irqd_to_hwirq(data);
+
+	return meson_gpio_to_irq(pc, gpio);
+}
+
+static irqreturn_t meson_gpio_irq_handler(int irq, void *data)
+{
+	struct irq_data *gpio_irqdata = data;
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	int hwirq = meson_gpio_data_to_hwirq(gpio_irqdata);
+
+	/*
+	 * For some strange reason spurious interrupts created by the chip when
+	 * the interrupt source registers are written cause a deadlock here.
+	 * generic_handle_irq calls handle_simple_irq which tries to get
+	 * spinlock desc->lock. This interrupt handler is called whilst
+	 * __setup_irq holds desc->lock.
+	 * The deadlock means that both are running on the same CPU what should
+	 * not happen as __setup_irq called raw_spin_lock_irqsave thus disabling
+	 * interrupts on this CPU.
+	 * Work around this by ignoring interrupts in code protected by
+	 * chip_bus_lock (__setup_irq/__free_irq for the respective GPIO hwirq).
+	 */
+	if (test_bit(hwirq, meson_gpio_irq_locked))
+		dev_dbg(pc->dev, "spurious interrupt detected!\n");
+	else
+		generic_handle_irq(gpio_irqdata->irq);
+
+	return IRQ_HANDLED;
+}
+
+static int meson_gpio_alloc_irq_slot(struct irq_data *data, int num_slots,
+				     int *slots)
+{
+	int i, cnt = 0;
+
+	mutex_lock(&meson_gpio_irq_slot_mutex);
+
+	for (i = 0; i < meson_gpio_num_irq_slots; i++)
+		if (!meson_gpio_irq_slots[i].owner) {
+			meson_gpio_irq_slots[i].owner = data->irq;
+			slots[cnt++] = i;
+			if (cnt == num_slots)
+				break;
+		}
+
+	if (cnt < num_slots)
+		for (i = 0; i < cnt; i++)
+			meson_gpio_irq_slots[i].owner = 0;
+
+	mutex_unlock(&meson_gpio_irq_slot_mutex);
+
+	return cnt == num_slots ? 0 : -ENOSPC;
+}
+
+static void meson_gpio_free_irq_slot(struct irq_data *data)
+{
+	int i;
+
+	mutex_lock(&meson_gpio_irq_slot_mutex);
+
+	for (i = 0; i < meson_gpio_num_irq_slots; i++)
+		if (meson_gpio_irq_slots[i].owner == data->irq) {
+			free_irq(meson_gpio_irq_slots[i].irq, data);
+			meson_gpio_irq_slots[i].owner = 0;
+		}
+
+	mutex_unlock(&meson_gpio_irq_slot_mutex);
+}
+
+static int meson_gpio_find_irq_slot(struct irq_data *data, int *slots)
+{
+	int i, cnt = 0;
+
+	mutex_lock(&meson_gpio_irq_slot_mutex);
+
+	for (i = 0; i < meson_gpio_num_irq_slots; i++)
+		if (meson_gpio_irq_slots[i].owner == data->irq)
+			slots[cnt++] = i;
+
+	mutex_unlock(&meson_gpio_irq_slot_mutex);
+
+	return cnt ?: -EINVAL;
+}
+
+static void meson_gpio_set_hwirq(int idx, int hwirq)
+{
+	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
+	int shift = 8 * (idx % 4);
+
+	regmap_update_bits(meson_gpio_irq_regmap, reg, 0xff << shift,
+			   hwirq << shift);
+}
+
+static void meson_gpio_irq_set_hwirq(struct irq_data *data, int hwirq)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	int i, cnt, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
+
+	cnt = meson_gpio_find_irq_slot(data, slots);
+	if (cnt < 0) {
+		dev_err(pc->dev, "didn't find gpio irq slot\n");
+		return;
+	}
+
+	for (i = 0; i < cnt; i++)
+		meson_gpio_set_hwirq(slots[i], hwirq);
+}
+
+static void meson_gpio_irq_unmask(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	unsigned gpio = irqd_to_hwirq(data);
+	int hwirq = meson_gpio_to_irq(pc, gpio);
+
+	meson_gpio_irq_set_hwirq(data, hwirq);
+}
+
+static void meson_gpio_irq_mask(struct irq_data *data)
+{
+	meson_gpio_irq_set_hwirq(data, 0xff);
+}
+
+static void meson_gpio_irq_shutdown(struct irq_data *data)
+{
+	meson_gpio_irq_mask(data);
+	meson_gpio_free_irq_slot(data);
+}
+
+static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	int i, ret, irq, num_slots, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
+	unsigned int val = 0;
+
+	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
+	ret = meson_gpio_alloc_irq_slot(data, num_slots, slots);
+	if (ret)
+		return ret;
+
+	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_EDGE(slots[0]);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_LOW(slots[0]);
+
+	regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
+			   REG_EDGE_POL_MASK(slots[0]), val);
+
+	/*
+	 * 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.
+	 */
+	if (num_slots > 1) {
+		val = REG_EDGE_POL_EDGE(slots[1]);
+		regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
+				   REG_EDGE_POL_MASK(slots[1]), val);
+	}
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val = IRQ_TYPE_EDGE_RISING;
+	else
+		val = IRQ_TYPE_LEVEL_HIGH;
+
+	for (i = 0; i < num_slots; i++) {
+		irq = meson_gpio_irq_slots[slots[i]].irq;
+		ret = irq_set_irq_type(irq, val);
+		if (ret)
+			break;
+		ret = request_irq(irq, meson_gpio_irq_handler, 0,
+				  "GPIO parent", data);
+		if (ret)
+			break;
+	}
+
+	if (ret)
+		while (--i >= 0)
+			free_irq(meson_gpio_irq_slots[slots[i]].irq, data);
+
+	return ret;
+}
+
+static void meson_gpio_irq_bus_lock(struct irq_data *data)
+{
+	int hwirq = meson_gpio_data_to_hwirq(data);
+
+	set_bit(hwirq, meson_gpio_irq_locked);
+}
+
+static void meson_gpio_irq_bus_sync_unlock(struct irq_data *data)
+{
+	int hwirq = meson_gpio_data_to_hwirq(data);
+
+	clear_bit(hwirq, meson_gpio_irq_locked);
+}
+
+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,
+	.irq_bus_lock = meson_gpio_irq_bus_lock,
+	.irq_bus_sync_unlock = meson_gpio_irq_bus_sync_unlock,
+};
+
+static int meson_gpio_get_irqs(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int irq, i;
+
+	for (i = 0; i < MESON_GPIO_MAX_PARENT_IRQ_NUM; i++) {
+		irq = irq_of_parse_and_map(np, i);
+		if (!irq)
+			break;
+		meson_gpio_irq_slots[i].irq = irq;
+	}
+
+	meson_gpio_num_irq_slots = i;
+
+	return i ? 0 : -EINVAL;
+}
+
+static const struct regmap_config meson_gpio_regmap_config = {
+	.reg_bits       = 32,
+	.reg_stride     = 4,
+	.val_bits       = 32,
+	.max_register	= REG_FILTER_SEL,
+};
+
+static int meson_gpio_irq_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	void __iomem *io_base;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	io_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(io_base))
+		return PTR_ERR(io_base);
+
+	meson_gpio_irq_regmap = devm_regmap_init_mmio(&pdev->dev, io_base,
+						&meson_gpio_regmap_config);
+	if (IS_ERR(meson_gpio_irq_regmap))
+		return PTR_ERR(meson_gpio_irq_regmap);
+
+	/* initialize to IRQ_TYPE_LEVEL_HIGH */
+	regmap_write(meson_gpio_irq_regmap, REG_EDGE_POL, 0);
+	/* disable all GPIO interrupt sources */
+	regmap_write(meson_gpio_irq_regmap, REG_PIN_03_SEL, 0xffffffff);
+	regmap_write(meson_gpio_irq_regmap, REG_PIN_47_SEL, 0xffffffff);
+	/* disable filtering */
+	regmap_write(meson_gpio_irq_regmap, REG_FILTER_SEL, 0);
+
+	ret = meson_gpio_get_irqs(pdev);
+	if (ret)
+		return ret;
+
+	meson_pinctrl_irq_chip = &meson_gpio_irq_chip;
+
+	return 0;
+}
+
+static const struct of_device_id meson_gpio_irq_dt_match[] = {
+	{ .compatible = "amlogic,meson-gpio-interrupt" },
+	{ },
+};
+
+static struct platform_driver meson_gpio_irq_driver = {
+	.probe		= meson_gpio_irq_probe,
+	.driver = {
+		.name	= "meson-gpio-interrupt",
+		.of_match_table = meson_gpio_irq_dt_match,
+	},
+};
+builtin_platform_driver(meson_gpio_irq_driver);

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

index 39ad9861..c587f6f0 100644
@@ -62,6 +62,8 @@
 #include "../pinctrl-utils.h"
 #include "pinctrl-meson.h"
 
+struct irq_chip *meson_pinctrl_irq_chip;
+
 /**
  * meson_pinctrl_get_bank() - find the bank containing a given pin
  *

@@ -551,7 +553,8 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc)
 		return ret;
 	}
 
-	return 0;
+	return gpiochip_irqchip_add(&pc->chip, meson_pinctrl_irq_chip, 0,
+				    handle_simple_irq, IRQ_TYPE_NONE);
 }
 
 static struct regmap_config meson_regmap_config = {

@@ -640,6 +643,9 @@ static int meson_pinctrl_probe(struct platform_device *pdev)
 	struct meson_pinctrl *pc;
 	int ret;
 
+	if (!meson_pinctrl_irq_chip)
+		return -EPROBE_DEFER;
+
 	pc = devm_kzalloc(dev, sizeof(struct meson_pinctrl), GFP_KERNEL);
 	if (!pc)
 		return -ENOMEM;

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

index 40b56aff..16aab328 100644
@@ -176,6 +176,7 @@ extern struct meson_pinctrl_data meson_gxbb_periphs_pinctrl_data;
 extern struct meson_pinctrl_data meson_gxbb_aobus_pinctrl_data;
 extern struct meson_pinctrl_data meson_gxl_periphs_pinctrl_data;
 extern struct meson_pinctrl_data meson_gxl_aobus_pinctrl_data;
+extern struct irq_chip *meson_pinctrl_irq_chip;
 
 struct meson_bank *meson_pinctrl_get_bank(const struct meson_pinctrl *pc,
 					  unsigned int pin);

Am 15.05.2017 um 10:05 schrieb Neil Armstrong:

Quoted 17 lines · Heiner Kallweit

Quoted 7 lines

Add support for GPIO interrupts on Amlogic Meson SoC's.

There's a limit of 8 parent interupts which can be used in total. Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's, one for each edge.

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

Quoted 8 lines

---
v2:
- make the GPIO IRQ controller a separate driver
- several smaller improvements

---
 drivers/pinctrl/Kconfig                   |   1 +
 drivers/pinctrl/meson/Makefile            |   2 +-
 drivers/pinctrl/meson/pinctrl-meson-irq.c | 367 ++++++++++++++++++++++++++++++

Quoted 5 lines

Hi Heiner,

This code has nothing to do with GPIOs on pinmux handling here, what we figured with Jerome is that this must be an independent IRQ Controller in drivers/irqchip.

I'm not convinced and would like to hear more opinions on that. I see it like this:
The driver implements an irqchip, right. But it's not an interrupt controller. Due to using GPIOLIB_IRQCHIP the gpio controller now has an own irq domain (one for ao and one for periphs GPIO domain). Therefore the gpio-controller now also acts as interrupt-controller. And both gpio (and interrupt) controllers just use the irqchip exposed by the new driver.
Last but not least the irqchip can be used with GPIOs only.

In the irqchip implementation we need the SoC-specific mapping from GPIO number to internal GPIO IRQ number. Having to export this from drivers/pinctrl/meson for use under drivers/irqchip most likely would also cause objections.

So far my impression is that the very specific way GPIO IRQ's are handled on Meson doesn't fit perfectly into the current IRQ subsystem. Therefore the discussion about Jerome's version didn't result in the IRQ maintainers stating: do it this way .. Having said that most likely every possible approach is going to raise some concerns.

Quoted 7 lines

Please move it and make independent, you should be able to request irqs without any links to the pinmux/gpio since physically the GPIO lines input are always connected to this irq controller, and the pinmux has no impact on the interrupt management here.

Quoted 1 line

From the GPIO-IRQ Controller perspective, the GPIOs are only a number and the pinmux code

is only here to make the translation to a specific GPIO to a GPIO-IRQ number.

Quoted 9 lines

For more chance to have it upstreamed in the right way, we should :
1) Collaborate, we can chat over IRC, maybe Slack, E-mail, skype, forums, ...
2) Push an independent IRQ controller that matches the capacity of the HW
3) Push a link from the pinctrl driver to have the to_gpio_irq mapping done in the right way

Jerome spent quite a lot of time and had a chat with the IRQ subsystem maintainers to have s clear image of how this should be implemented, and it would be a good point to actually have a chat with them to elaborate find a strong solution.

I know and I really appreciate Jerome's work and his discussion with the IRQ maintainers. My current attempt was inspired by his work.
However the discussion last year ended w/o result and the topic of GPIO IRQs has been dead since then. And I think discussing approaches works best based on a concrete piece of code. Therefore I submitted my version as discussion basis. I didn't expect that everybody would be totally happy with it and it would go to mainline unchanged.

Quoted 4 lines

I'm sorry to say that pushing this code without really understanding how and why will lead to nothing expect frustration from everybody.

I can only speak for myself: I'm not frustrated and I can live with critical review comments.

Regards, Heiner

Quoted 1 line

Neil

Hi Heiner,

Quoted 23 lines · Heiner Kallweit

Am 15.05.2017 um 10:05 schrieb Neil Armstrong:

Quoted 17 lines · Heiner Kallweit

Quoted 7 lines

Add support for GPIO interrupts on Amlogic Meson SoC's.

There's a limit of 8 parent interupts which can be used in total. Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's, one for each edge.

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

Quoted 8 lines

---
v2:
- make the GPIO IRQ controller a separate driver
- several smaller improvements

---
 drivers/pinctrl/Kconfig                   |   1 +
 drivers/pinctrl/meson/Makefile            |   2 +-
 drivers/pinctrl/meson/pinctrl-meson-irq.c | 367 ++++++++++++++++++++++++++++++

Quoted 5 lines

Hi Heiner,

This code has nothing to do with GPIOs on pinmux handling here, what we figured with Jerome is that this must be an independent IRQ Controller in drivers/irqchip.

Quoted 7 lines

I'm not convinced and would like to hear more opinions on that. I see it like this:
The driver implements an irqchip, right. But it's not an interrupt controller. Due to using GPIOLIB_IRQCHIP the gpio controller now has an own irq domain (one for ao and one for periphs GPIO domain). Therefore the gpio-controller now also acts as interrupt-controller. And both gpio (and interrupt) controllers just use the irqchip exposed by the new driver.
Last but not least the irqchip can be used with GPIOs only.

In fact it's an interrupt controller since it can mask/unmask and control a singal that can wake up an interrupt for the ARM Core.

Please look at the STM32 EXTI code, the design is quite similar except they don't have a dynamic management of links, but fixed ones.
They have a proper independant IRQCHIP driver and a link from the pinctrl driver, and this should be the right design.
They have a flaw since they do the mapping from the gpio_to_irq, and Linus won't allow this anymore.

Quoted 4 lines

In the irqchip implementation we need the SoC-specific mapping from GPIO number to internal GPIO IRQ number. Having to export this from drivers/pinctrl/meson for use under drivers/irqchip most likely would also cause objections.

You won't need, this interrupt controller will take the number either from DT either from a mapping creating from the pinctrl driver. The link will only be through the irq subsystem.

Quoted 5 lines

So far my impression is that the very specific way GPIO IRQ's are handled on Meson doesn't fit perfectly into the current IRQ subsystem. Therefore the discussion about Jerome's version didn't result in the IRQ maintainers stating: do it this way .. Having said that most likely every possible approach is going to raise some concerns.

It doesn't fit exactly, but the subsystem can certainly be used to achieve it either by using all it's capacity or by eventually discussing with the maintainers to adapt it.

Jerome has some hints hot to achieve the pinctrl part with everyone "happy".

Quoted 16 lines

Quoted 7 lines

Please move it and make independent, you should be able to request irqs without any links to the pinmux/gpio since physically the GPIO lines input are always connected to this irq controller, and the pinmux has no impact on the interrupt management here.

Quoted 1 line

From the GPIO-IRQ Controller perspective, the GPIOs are only a number and the pinmux code

is only here to make the translation to a specific GPIO to a GPIO-IRQ number.

Quoted 9 lines

For more chance to have it upstreamed in the right way, we should :
1) Collaborate, we can chat over IRC, maybe Slack, E-mail, skype, forums, ...
2) Push an independent IRQ controller that matches the capacity of the HW
3) Push a link from the pinctrl driver to have the to_gpio_irq mapping done in the right way

Jerome spent quite a lot of time and had a chat with the IRQ subsystem maintainers to have s clear image of how this should be implemented, and it would be a good point to actually have a chat with them to elaborate find a strong solution.

Quoted 6 lines

I know and I really appreciate Jerome's work and his discussion with the IRQ maintainers. My current attempt was inspired by his work.
However the discussion last year ended w/o result and the topic of GPIO IRQs has been dead since then. And I think discussing approaches works best based on a concrete piece of code. Therefore I submitted my version as discussion basis. I didn't expect that everybody would be totally happy with it and it would go to mainline unchanged.

Sure, thanks for the work,

Quoted 5 lines

Quoted 4 lines

I'm sorry to say that pushing this code without really understanding how and why will lead to nothing expect frustration from everybody.

I can only speak for myself: I'm not frustrated and I can live with critical review comments.

Great ! Anyway thanks for your work and I hope this will lead to mainline !

Quoted 3 lines

Regards, Heiner

Quoted 1 line

Neil

Neil

[...]

Am 16.05.2017 um 09:54 schrieb Neil Armstrong:

Quoted 46 lines

Hi Heiner,

Quoted 26 lines · Heiner Kallweit

Am 15.05.2017 um 10:05 schrieb Neil Armstrong:

Quoted 18 lines · Heiner Kallweit

Quoted 12 lines

Add support for GPIO interrupts on Amlogic Meson SoC's.

There's a limit of 8 parent interupts which can be used in total. Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's, one for each edge.

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

---
v2:
- make the GPIO IRQ controller a separate driver
- several smaller improvements
---

Quoted 4 lines

?drivers/pinctrl/Kconfig???????????????????|???1 +
?drivers/pinctrl/meson/Makefile????????????|???2 +-
?drivers/pinctrl/meson/pinctrl-meson-irq.c | 367

++++++++++++++++++++++++++++++

Quoted 6 lines

Hi Heiner,

This code has nothing to do with GPIOs on pinmux handling here, what we figured with Jerome
is that this must be an independent IRQ Controller in drivers/irqchip.

Quoted 12 lines

I'm not convinced and would like to hear more opinions on that. I see it like this:
The driver implements an irqchip, right. But it's not an interrupt controller.
Due to using GPIOLIB_IRQCHIP the gpio controller now has an own irq domain (one for ao
and one for periphs GPIO domain). Therefore the gpio-controller now also acts as
interrupt-controller. And both gpio (and interrupt) controllers just use the irqchip
exposed by the new driver.
Last but not least the irqchip can be used with GPIOs only.

In fact it's an interrupt controller since it can mask/unmask and control a singal that
can wake up an interrupt for the ARM Core.

Quoted 10 lines

Please look at the STM32 EXTI code, the design is quite similar except they don't have a
dynamic management of links, but fixed ones.
They have a proper independant IRQCHIP driver and a link from the pinctrl driver, and this
should be the right design.
They have a flaw since they do the mapping from the gpio_to_irq, and Linus won't allow
this anymore.

At first I involve Rob as he also provided feedback regarding the DT part.

I had a look at the STM32 EXTI code and it looks very similar to Jerome's version.
Actually I'd assume that the first Meson driver attempt was modeled after STM32 EXTI.

As you just mentioned there are at least two issues:
1. The mapping can be requested via gpio_to_irq but it doesn't have to. A driver could

???also request a GPIO IRQ via DT.
2. Missing is the handling of IRQ_TYPE_EDGE_BOTH which requires the allocation of two
???parent interrupts.

When looking at the drivers under drivers/irqchip basically all of them implement not
only an irqchip but also an IRQ domain. Providing an IRQ domain seems to me to be
the main criteria whether something should be considered an interrupt controller
(please correct me if this understanding is wrong).

The STM32 EXTI drivers seems to me to be unnecessarily complex due to not using
GPIOLIB_IRQCHIP. This GPIO framework extension can hide a significant part of the
GPIO IRQ complexity.

Coming back to my version:
The new driver just implements an irqchip, no IRQ domain.
This irqchip then is
provided to the IRQ domains bound to the respective gpio controllers (for AO and PERIPHS
GPIO domain) - thanks to GPIOLIB_IRQCHIP handling of the IRQ domains comes for free.

Adding the interrupt-controller property to the gpio-controller nodes is one thing
still missing in my patch series. Then a driver can request a GPIO IRQ also via DT, e.g.:

interrupt-parent = <&gpio>;
interrupts = <GPIOZ_0 IRQ_TYPE_EDGE_BOTH>;

interrupt-parent = <&gpio_ao>;
interrupts = <GPIOAO_0 IRQ_TYPE_EDGE_BOTH>;

Advantage of having an IRQ domain per GPIO controller is being able to to use the GPIO
defines as is. Or in other words:
GPIOAO_0 and GPIOZ_0 both have the value 0. This works here due to having one IRQ domain
per GPIO controller (both using the same new irqchip).

And all I had to do is implementing one irq_chip and changing one line in the existing
pinctrl driver.
Whilst I have to admit that e.g. calling request_irq for a parent irq from the irq_set_type
callback may be something people find ugly and hacky.

Compared to this implementation the STM32 EXTI drivers needs significantly more data
structures, e.g. irqchip + irq domain in the irqchip driver, then hierarchical IRQ domain
handling + further irqchip and irq domain + irq domain ops in the pinctrl driver.

Last but not least coming back to the initial talk with Rob about where to best place the
DT docu for this irqchip implementation:
If acceptable I'd prefer to do it like .e.g in bindings/pinctrl/allwinner,sunxi-pinctrl.txt
or bindings/pinctrl/atmel,at91-pio4-pinctrl.txt.
There the interrupt-controller properties are documented as part of the pinctrl driver and
not separately under bindings/interrupt-controller.

Maybe this explains a little bit better why I chose this approach.

Rgds, Heiner

Quoted 90 lines

Quoted 6 lines

In the irqchip implementation we need the SoC-specific mapping from GPIO number
to internal GPIO IRQ number. Having to export this from drivers/pinctrl/meson for use
under drivers/irqchip most likely would also cause objections.

You won't need, this interrupt controller will take the number either from DT either
from a mapping creating from the pinctrl driver. The link will only be through the
irq subsystem.

Quoted 9 lines

So far my impression is that the very specific way GPIO IRQ's are handled on Meson
doesn't fit perfectly into the current IRQ subsystem. Therefore the discussion
about Jerome's version didn't result in the IRQ maintainers stating: do it this way ..
Having said that most likely every possible approach is going to raise some concerns.

It doesn't fit exactly, but the subsystem can certainly be used to achieve it either by
using all it's capacity or by eventually discussing with the maintainers to adapt it.

Jerome has some hints hot to achieve the pinctrl part with everyone "happy".

Quoted 28 lines

Quoted 21 lines

Please move it and make independent, you should be able to request irqs without any links
to the pinmux/gpio since physically the GPIO lines input are always connected to this
irq controller, and the pinmux has no impact on the interrupt management here.

Quoted 2 lines

From the GPIO-IRQ Controller perspective, the GPIOs are only a number and the pinmux code

is only here to make the translation to a specific GPIO to a GPIO-IRQ number.

For more chance to have it upstreamed in the right way, we should :
1) Collaborate, we can chat over IRC, maybe Slack, E-mail, skype, forums, ...
2) Push an independent IRQ controller that matches the capacity of the HW
3) Push a link from the pinctrl driver to have the to_gpio_irq mapping done in the right way

Quoted 6 lines

Jerome spent quite a lot of time and had a chat with the IRQ subsystem maintainers to have s
clear image of how this should be implemented, and it would be a good point to actually
have a chat with them to elaborate find a strong solution.

Quoted 10 lines

I know and I really appreciate Jerome's work and his discussion with the IRQ maintainers.
My current attempt was inspired by his work.
However the discussion last year ended w/o result and the topic of GPIO IRQs has been dead
since then. And I think discussing approaches works best based on a concrete piece of code.
Therefore I submitted my version as discussion basis. I didn't expect that everybody would
be totally happy with it and it would go to mainline unchanged.

Sure, thanks for the work,

Quoted 8 lines

Quoted 5 lines

I'm sorry to say that pushing this code without really understanding how and why will lead to
nothing expect frustration from everybody.

I can only speak for myself: I'm not frustrated and I can live with critical review comments.

Great ! Anyway thanks for your work and I hope this will lead to mainline !

Quoted 3 lines

Regards, Heiner

Quoted 1 line

Neil

Neil

[...]

Quoted 58 lines

Am 16.05.2017 um 09:54 schrieb Neil Armstrong:

Quoted 46 lines

Hi Heiner,

Quoted 26 lines · Heiner Kallweit

Am 15.05.2017 um 10:05 schrieb Neil Armstrong:

Quoted 18 lines · Heiner Kallweit

Quoted 11 lines

Add support for GPIO interrupts on Amlogic Meson SoC's.

There's a limit of 8 parent interupts which can be used in total. Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's, one for each edge.

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

---
v2:
- make the GPIO IRQ controller a separate driver
- several smaller improvements

Quoted 5 lines

---
 drivers/pinctrl/Kconfig                   |   1 +
 drivers/pinctrl/meson/Makefile            |   2 +-
 drivers/pinctrl/meson/pinctrl-meson-irq.c | 367

++++++++++++++++++++++++++++++

Quoted 6 lines

Hi Heiner,

This code has nothing to do with GPIOs on pinmux handling here, what we figured with Jerome
is that this must be an independent IRQ Controller in drivers/irqchip.

Quoted 12 lines

I'm not convinced and would like to hear more opinions on that. I see it like this:
The driver implements an irqchip, right. But it's not an interrupt controller.
Due to using GPIOLIB_IRQCHIP the gpio controller now has an own irq domain (one for ao
and one for periphs GPIO domain). Therefore the gpio-controller now also acts as
interrupt-controller. And both gpio (and interrupt) controllers just use the irqchip
exposed by the new driver.
Last but not least the irqchip can be used with GPIOs only.

In fact it's an interrupt controller since it can mask/unmask and control a singal that
can wake up an interrupt for the ARM Core.

Quoted 10 lines

Please look at the STM32 EXTI code, the design is quite similar except they don't have a
dynamic management of links, but fixed ones.
They have a proper independant IRQCHIP driver and a link from the pinctrl driver, and this
should be the right design.
They have a flaw since they do the mapping from the gpio_to_irq, and Linus won't allow
this anymore.

Quoted 6 lines

At first I involve Rob as he also provided feedback regarding the DT part.

I had a look at the STM32 EXTI code and it looks very similar to Jerome's version.
Actually I'd assume that the first Meson driver attempt was modeled after STM32 EXTI.

Well, no. EXTI has been merged when I already submitted the RFC for this driver, but that's not the point I suppose.

Quoted 1 line

Quoted 3 lines

As you just mentioned there are at least two issues:
1. The mapping can be requested via gpio_to_irq but it doesn't have to. A driver could

Maybe you missed it in our previous discussion with Linus but, no you can't create mapping in gpio_to_irq. This callback is fast and creating mappings might sleep because of the irq_domain mutex

https://patchwork.ozlabs.org/patch/684208/

Quoted 13 lines

also request a GPIO IRQ via DT.
2. Missing is the handling of IRQ_TYPE_EDGE_BOTH which requires the allocation of two
parent interrupts.

When looking at the drivers under drivers/irqchip basically all of them implement not
only an irqchip but also an IRQ domain. Providing an IRQ domain seems to me to be
the main criteria whether something should be considered an interrupt controller
(please correct me if this understanding is wrong).

Quoted 5 lines

The STM32 EXTI drivers seems to me to be unnecessarily complex due to not using
GPIOLIB_IRQCHIP. This GPIO framework extension can hide a significant part of the
GPIO IRQ complexity.

I can only speculate regarding the design choice of STM EXTI, but I suppose the EXTI controller is independent of the pinmux/gpio subsystem HW wise. It's only weakly linked to the gpios, in the way that you have (or can create) a "map" between the gpio and the irq line number provided.

That is also the case for the amlogic gpio-irq. I suppose that's why Neil suggested to look at EXTI. That's also why I commented that this should be first implemented as an independent controller of the gpio subsys (so in drivers/irqchip)

You may find it more complex, which is arguable, but it is a more accurate representation of the HW.

Quoted 14 lines

Coming back to my version:
The new driver just implements an irqchip, no IRQ domain.
This irqchip then is
provided to the IRQ domains bound to the respective gpio controllers (for AO and PERIPHS
GPIO domain) - thanks to GPIOLIB_IRQCHIP handling of the IRQ domains comes for free.

Adding the interrupt-controller property to the gpio-controller nodes is one thing
still missing in my patch series. Then a driver can request a GPIO IRQ also via DT, e.g.:

Quoted 6 lines

interrupt-parent = <&gpio>;
interrupts = <GPIOZ_0 IRQ_TYPE_EDGE_BOTH>;

interrupt-parent = <&gpio_ao>;
interrupts = <GPIOAO_0 IRQ_TYPE_EDGE_BOTH>;

Which is also possible in the series, where the controller is gpio-irq device itself. This is what the HW provide, so it should be possible. It would be nice to have your way working as well, which I think is possible (PSB)

Quoted 7 lines

Advantage of having an IRQ domain per GPIO controller is being able to to use the GPIO
defines as is. Or in other words:
GPIOAO_0 and GPIOZ_0 both have the value 0. This works here due to having one IRQ domain
per GPIO controller (both using the same new irqchip).

I think you are onto something. As I told you previously, the problem was to create the mappings in pinctrl w/o allocating the parent. I struggled with that back in November and I had no time to really get back to it since.

Creating domains in each gpio controller, w/ all the mapping created at startup,
stacked to the domain provided by the driver/irqchip would solve the problem.

We would just have to call find_mapping in gpio_to_irq, which is fine and allocate the parent irq in the request_ressource callback.

Quoted 7 lines

And all I had to do is implementing one irq_chip and changing one line in the existing
pinctrl driver.
Whilst I have to admit that e.g. calling request_irq for a parent irq from the irq_set_type
callback may be something people find ugly and hacky.

The fact is that the HW does not directly support IRQ_EDGE_BOTH. Yes, it would be nice to find a way to support it anyway. There two possibilities for it:
* Change the change the edge type depending on the next expected edge: Marc already stated that he is firmly against that. It is indeed not very robust
* Allocate and deallocate additional parent irq to get secondary irq in
set_type: This indeed looks hacky, I'd like to get the view of irq maintainers on this.

Quoted 7 lines

Compared to this implementation the STM32 EXTI drivers needs significantly more data
structures, e.g. irqchip + irq domain in the irqchip driver, then hierarchical IRQ domain
handling + further irqchip and irq domain + irq domain ops in the pinctrl driver.

Within reasonable limits, having more or less data, code lines does not make a driver better or worse.

Quoted 107 lines

Last but not least coming back to the initial talk with Rob about where to best place the
DT docu for this irqchip implementation:
If acceptable I'd prefer to do it like .e.g in bindings/pinctrl/allwinner,sunxi-pinctrl.txt
or bindings/pinctrl/atmel,at91-pio4-pinctrl.txt.
There the interrupt-controller properties are documented as part of the pinctrl driver and
not separately under bindings/interrupt-controller.

Maybe this explains a little bit better why I chose this approach.

Rgds, Heiner

Quoted 90 lines

Quoted 6 lines

In the irqchip implementation we need the SoC-specific mapping from GPIO number
to internal GPIO IRQ number. Having to export this from drivers/pinctrl/meson for use
under drivers/irqchip most likely would also cause objections.

You won't need, this interrupt controller will take the number either from DT either
from a mapping creating from the pinctrl driver. The link will only be through the
irq subsystem.

Quoted 9 lines

So far my impression is that the very specific way GPIO IRQ's are handled on Meson
doesn't fit perfectly into the current IRQ subsystem. Therefore the discussion
about Jerome's version didn't result in the IRQ maintainers stating: do it this way ..
Having said that most likely every possible approach is going to raise some concerns.

It doesn't fit exactly, but the subsystem can certainly be used to achieve it either by
using all it's capacity or by eventually discussing with the maintainers to adapt it.

Jerome has some hints hot to achieve the pinctrl part with everyone "happy".

Quoted 28 lines

Quoted 21 lines

Please move it and make independent, you should be able to request irqs without any links
to the pinmux/gpio since physically the GPIO lines input are always connected to this
irq controller, and the pinmux has no impact on the interrupt management here.

Quoted 2 lines

From the GPIO-IRQ Controller perspective, the GPIOs are only a number and the pinmux code

is only here to make the translation to a specific GPIO to a GPIO-IRQ number.

For more chance to have it upstreamed in the right way, we should :
1) Collaborate, we can chat over IRC, maybe Slack, E-mail, skype, forums, ...
2) Push an independent IRQ controller that matches the capacity of the HW
3) Push a link from the pinctrl driver to have the to_gpio_irq mapping done in the right way

Quoted 6 lines

Jerome spent quite a lot of time and had a chat with the IRQ subsystem maintainers to have s
clear image of how this should be implemented, and it would be a good point to actually
have a chat with them to elaborate find a strong solution.

Quoted 10 lines

I know and I really appreciate Jerome's work and his discussion with the IRQ maintainers.
My current attempt was inspired by his work.
However the discussion last year ended w/o result and the topic of GPIO IRQs has been dead
since then. And I think discussing approaches works best based on a concrete piece of code.
Therefore I submitted my version as discussion basis. I didn't expect that everybody would
be totally happy with it and it would go to mainline unchanged.

Sure, thanks for the work,

Quoted 8 lines

Quoted 5 lines

I'm sorry to say that pushing this code without really understanding how and why will lead to
nothing expect frustration from everybody.

I can only speak for myself: I'm not frustrated and I can live with critical review comments.

Great ! Anyway thanks for your work and I hope this will lead to mainline !

Quoted 3 lines

Regards, Heiner

Quoted 1 line

Neil

Neil

[...]

Am 17.05.2017 um 01:16 schrieb Jerome Brunet:

Quoted 1 line · Heiner Kallweit

Quoted 6 lines

Maybe you missed it in our previous discussion with Linus but, no you can't create mapping in gpio_to_irq. This callback is fast and creating mappings might
sleep because of the irq_domain mutex

Quoted 2 lines

https://patchwork.ozlabs.org/patch/684208/

My comment was misunderstandable. What I meant was that a driver doesn't have to
use gpio_to_irq to request an interrupt, this can also be done via DT.

Quoted 11 lines

Quoted 2 lines

I think you are onto something. As I told you previously, the problem was to

create the mappings in pinctrl w/o allocating the parent. I struggled with that
back in November and I had no time to really get back to it since.

Creating domains in each gpio controller, w/ all the mapping created at startup,
? stacked to the domain provided by the driver/irqchip would solve the problem.

Quoted 3 lines

We would just have to call find_mapping in gpio_to_irq, which is fine and allocate the parent irq in the request_ressource callback.

In request_resource we don't know the irq type yet (and therefore whether we need
one or wo parent interrupts). IMHO we can't get completely rid of allocating parents in set_type.

Quoted 11 lines

Quoted 10 lines

And all I had to do is implementing one irq_chip and changing one line in the
existing
pinctrl driver.
Whilst I have to admit that e.g. calling request_irq for a parent irq from the
irq_set_type
callback may be something people find ugly and hacky.

Quoted 10 lines

The fact is that the HW does not directly support IRQ_EDGE_BOTH. Yes, it would
be nice to find a way to support it anyway. There two possibilities for it:
* Change the change the edge type depending on the next expected edge: Marc already stated that he is firmly against that. It is indeed not very robust
* Allocate and deallocate additional parent irq to get secondary irq in
set_type: This indeed looks hacky, I'd like to get the view of irq maintainers
on this.

Support for IRQ_TYPE_EDGE_BOTH I'd consider a mandatory feature. It's needed e.g. by the SD card detection (drivers/mmc/slot-gpio.c).

Quoted 2 lines

Am 17.05.2017 um 01:16 schrieb Jerome Brunet:

Quoted 1 line · Heiner Kallweit

[snip]

Quoted 6 lines

Quoted 6 lines

Maybe you missed it in our previous discussion with Linus but, no you can't create mapping in gpio_to_irq. This callback is fast and creating mappings might
sleep because of the irq_domain mutex

Quoted 7 lines

Quoted 2 lines

https://patchwork.ozlabs.org/patch/684208/

My comment was misunderstandable. What I meant was that a driver doesn't have to
use gpio_to_irq to request an interrupt, this can also be done via DT.

If you don't provide gpio_to_irq, you won't support the drivers which only request a gpio through DT (because they need to read the pin state) and later request an irq from it (to get an event and avoid polling)

That's a fairly common use-case. IMHO, this callback should be implemented by the gpio controller, when it is an interrupt controller.

[snip]

Quoted 11 lines

Quoted 11 lines

Quoted 2 lines

I think you are onto something. As I told you previously, the problem was to

create the mappings in pinctrl w/o allocating the parent. I struggled with that
back in November and I had no time to really get back to it since.

Creating domains in each gpio controller, w/ all the mapping created at startup,
stacked to the domain provided by the driver/irqchip would solve the
problem.

Quoted 8 lines

Quoted 3 lines

We would just have to call find_mapping in gpio_to_irq, which is fine and allocate the parent irq in the request_ressource callback.

In request_resource we don't know the irq type yet (and therefore whether we need
one or wo parent interrupts). IMHO we can't get completely rid of allocating parents in set_type.

Yes you can. The HW does *not* support IRQ_BOTH on an irq line. You are trying to come up with a SW workaround to get this feature. W/O this workaround, there no reason to allocate a parent irq in set_type.
At this point, no one has come up with a clean solution to add this feature to the meson-gpio-irq device.
Maybe evolutions in the irq framework will allow this cleanly in the future, I don't think this is the case right now.

[snip]

Quoted 22 lines

Quoted 11 lines

Quoted 10 lines

And all I had to do is implementing one irq_chip and changing one line in the
existing
pinctrl driver.
Whilst I have to admit that e.g. calling request_irq for a parent irq from the
irq_set_type
callback may be something people find ugly and hacky.

Quoted 10 lines

The fact is that the HW does not directly support IRQ_EDGE_BOTH. Yes, it would
be nice to find a way to support it anyway. There two possibilities for it:
* Change the change the edge type depending on the next expected edge: Marc already stated that he is firmly against that. It is indeed not very robust
* Allocate and deallocate additional parent irq to get secondary irq in
set_type: This indeed looks hacky, I'd like to get the view of irq maintainers
on this.

Quoted 3 lines

Support for IRQ_TYPE_EDGE_BOTH I'd consider a mandatory feature. It's needed e.g. by the SD card detection (drivers/mmc/slot-gpio.c).

"You'd consider" maybe, but the fact is that it is not mandatory. Having irq based card detect (or buttons, or jack insert, or whatever) is probably more elegant, but the polled version also works well. Elegant and mandatory are not the same things

Add support for GPIO interrupts on Amlogic Meson SoC's.

There's a limit of 8 parent interupts which can be used in total. Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's, one for each edge.

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

---
v2:
- make the GPIO IRQ controller a separate driver
- several smaller improvements
---

?drivers/pinctrl/Kconfig???????????????????|???1 +
?drivers/pinctrl/meson/Makefile????????????|???2 +-
?drivers/pinctrl/meson/pinctrl-meson-irq.c | 367

++++++++++++++++++++++++++++++
?drivers/pinctrl/meson/pinctrl-meson.c?????|???8 +- ?drivers/pinctrl/meson/pinctrl-meson.h?????|???1 + ?5 files changed, 377 insertions(+), 2 deletions(-) ?create mode 100644 drivers/pinctrl/meson/pinctrl-meson-irq.c

drivers/pinctrl/Kconfig +0 -0

index 37af5e30..f8f401a0 100644
@@ -153,6 +153,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/Makefile +0 -0

index 27c5b512..827e416d 100644
@@ -1,3 +1,3 @@

?obj-y += pinctrl-meson8.o pinctrl-meson8b.o
?obj-y += pinctrl-meson-gxbb.o pinctrl-meson-gxl.o

drivers/pinctrl/meson/Makefile +1 -1

-obj-y	+= pinctrl-meson.o
+obj-y	+= pinctrl-meson.o pinctrl-meson-irq.o

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +104 -0

new file mode 100644
index 00000000..c5f403f3
@@ -0,0 +1,367 @@
+/*
+ * Amlogic Meson GPIO IRQ driver
+ *
+ * Copyright 2017 Heiner Kallweit <[email protected]>
+ * Based on a first version by Jerome Brunet <[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/gpio.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include "pinctrl-meson.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))
+
+#define MESON_GPIO_MAX_PARENT_IRQ_NUM	8
+
+struct meson_gpio_irq_slot {
+	int irq;
+	int owner;
+};
+
+static struct regmap *meson_gpio_irq_regmap;
+static struct meson_gpio_irq_slot
+		meson_gpio_irq_slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
+static int meson_gpio_num_irq_slots;
+static DEFINE_MUTEX(meson_gpio_irq_slot_mutex);
+static DECLARE_BITMAP(meson_gpio_irq_locked, 256);
+
+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_to_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_irq(struct meson_pinctrl *pc, unsigned int offset)
+{
+	struct meson_bank *bank;
+	int hwirq;
+
+	offset += pc->data->pin_base;
+
+	bank = meson_pinctrl_get_bank(pc, offset);
+	if (IS_ERR(bank))
+		return PTR_ERR(bank);
+
+	hwirq = meson_gpio_to_hwirq(bank, offset);
+	if (!hwirq)
+		dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
+
+	return hwirq;
+}
+
+static int meson_gpio_data_to_hwirq(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	unsigned gpio = irqd_to_hwirq(data);
+
+	return meson_gpio_to_irq(pc, gpio);
+}
+
+static irqreturn_t meson_gpio_irq_handler(int irq, void *data)
+{
+	struct irq_data *gpio_irqdata = data;
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	int hwirq = meson_gpio_data_to_hwirq(gpio_irqdata);
+
+	/*
+	?* For some strange reason spurious interrupts created by the chip

when

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

+	?* the interrupt source registers are written cause a deadlock here.
+	?* generic_handle_irq calls handle_simple_irq which tries to get
+	?* spinlock desc->lock. This interrupt handler is called whilst
+	?* __setup_irq holds desc->lock.
+	?* The deadlock means that both are running on the same CPU what

should

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +1 -0

+	?* not happen as __setup_irq called raw_spin_lock_irqsave thus

disabling

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +3 -0

+	?* interrupts on this CPU.
+	?* Work around this by ignoring interrupts in code protected by
+	?* chip_bus_lock (__setup_irq/__free_irq for the respective GPIO

hwirq).

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +138 -0

+	if (test_bit(hwirq, meson_gpio_irq_locked))
+		dev_dbg(pc->dev, "spurious interrupt detected!\n");
+	else
+		generic_handle_irq(gpio_irqdata->irq);
+
+	return IRQ_HANDLED;
+}
+
+static int meson_gpio_alloc_irq_slot(struct irq_data *data, int num_slots,
+				?????int *slots)
+{
+	int i, cnt = 0;
+
+	mutex_lock(&meson_gpio_irq_slot_mutex);
+
+	for (i = 0; i < meson_gpio_num_irq_slots; i++)
+		if (!meson_gpio_irq_slots[i].owner) {
+			meson_gpio_irq_slots[i].owner = data->irq;
+			slots[cnt++] = i;
+			if (cnt == num_slots)
+				break;
+		}
+
+	if (cnt < num_slots)
+		for (i = 0; i < cnt; i++)
+			meson_gpio_irq_slots[i].owner = 0;
+
+	mutex_unlock(&meson_gpio_irq_slot_mutex);
+
+	return cnt == num_slots ? 0 : -ENOSPC;
+}
+
+static void meson_gpio_free_irq_slot(struct irq_data *data)
+{
+	int i;
+
+	mutex_lock(&meson_gpio_irq_slot_mutex);
+
+	for (i = 0; i < meson_gpio_num_irq_slots; i++)
+		if (meson_gpio_irq_slots[i].owner == data->irq) {
+			free_irq(meson_gpio_irq_slots[i].irq, data);
+			meson_gpio_irq_slots[i].owner = 0;
+		}
+
+	mutex_unlock(&meson_gpio_irq_slot_mutex);
+}
+
+static int meson_gpio_find_irq_slot(struct irq_data *data, int *slots)
+{
+	int i, cnt = 0;
+
+	mutex_lock(&meson_gpio_irq_slot_mutex);
+
+	for (i = 0; i < meson_gpio_num_irq_slots; i++)
+		if (meson_gpio_irq_slots[i].owner == data->irq)
+			slots[cnt++] = i;
+
+	mutex_unlock(&meson_gpio_irq_slot_mutex);
+
+	return cnt ?: -EINVAL;
+}
+
+static void meson_gpio_set_hwirq(int idx, int hwirq)
+{
+	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
+	int shift = 8 * (idx % 4);
+
+	regmap_update_bits(meson_gpio_irq_regmap, reg, 0xff << shift,
+			???hwirq << shift);
+}
+
+static void meson_gpio_irq_set_hwirq(struct irq_data *data, int hwirq)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	int i, cnt, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
+
+	cnt = meson_gpio_find_irq_slot(data, slots);
+	if (cnt < 0) {
+		dev_err(pc->dev, "didn't find gpio irq slot\n");
+		return;
+	}
+
+	for (i = 0; i < cnt; i++)
+		meson_gpio_set_hwirq(slots[i], hwirq);
+}
+
+static void meson_gpio_irq_unmask(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	unsigned gpio = irqd_to_hwirq(data);
+	int hwirq = meson_gpio_to_irq(pc, gpio);
+
+	meson_gpio_irq_set_hwirq(data, hwirq);
+}
+
+static void meson_gpio_irq_mask(struct irq_data *data)
+{
+	meson_gpio_irq_set_hwirq(data, 0xff);
+}
+
+static void meson_gpio_irq_shutdown(struct irq_data *data)
+{
+	meson_gpio_irq_mask(data);
+	meson_gpio_free_irq_slot(data);
+}
+
+static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	int i, ret, irq, num_slots, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
+	unsigned int val = 0;
+
+	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
+	ret = meson_gpio_alloc_irq_slot(data, num_slots, slots);
+	if (ret)
+		return ret;
+
+	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_EDGE(slots[0]);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_LOW(slots[0]);
+
+	regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
+			???REG_EDGE_POL_MASK(slots[0]), val);
+
+	/*
+	?* 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.
+	?*/
+	if (num_slots > 1) {
+		val = REG_EDGE_POL_EDGE(slots[1]);
+		regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
+				???REG_EDGE_POL_MASK(slots[1]), val);
+	}
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val = IRQ_TYPE_EDGE_RISING;

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +105 -0

+		if (ret)
+			break;
+	}
+
+	if (ret)
+		while (--i >= 0)
+			free_irq(meson_gpio_irq_slots[slots[i]].irq, data);
+
+	return ret;
+}
+
+static void meson_gpio_irq_bus_lock(struct irq_data *data)
+{
+	int hwirq = meson_gpio_data_to_hwirq(data);
+
+	set_bit(hwirq, meson_gpio_irq_locked);
+}
+
+static void meson_gpio_irq_bus_sync_unlock(struct irq_data *data)
+{
+	int hwirq = meson_gpio_data_to_hwirq(data);
+
+	clear_bit(hwirq, meson_gpio_irq_locked);
+}
+
+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,
+	.irq_bus_lock = meson_gpio_irq_bus_lock,
+	.irq_bus_sync_unlock = meson_gpio_irq_bus_sync_unlock,
+};
+
+static int meson_gpio_get_irqs(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int irq, i;
+
+	for (i = 0; i < MESON_GPIO_MAX_PARENT_IRQ_NUM; i++) {
+		irq = irq_of_parse_and_map(np, i);
+		if (!irq)
+			break;
+		meson_gpio_irq_slots[i].irq = irq;
+	}
+
+	meson_gpio_num_irq_slots = i;
+
+	return i ? 0 : -EINVAL;
+}
+
+static const struct regmap_config meson_gpio_regmap_config = {
+	.reg_bits???????= 32,
+	.reg_stride?????= 4,
+	.val_bits???????= 32,
+	.max_register	= REG_FILTER_SEL,
+};
+
+static int meson_gpio_irq_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	void __iomem *io_base;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	io_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(io_base))
+		return PTR_ERR(io_base);
+
+	meson_gpio_irq_regmap = devm_regmap_init_mmio(&pdev->dev, io_base,
+						&meson_gpio_regmap_config);
+	if (IS_ERR(meson_gpio_irq_regmap))
+		return PTR_ERR(meson_gpio_irq_regmap);
+
+	/* initialize to IRQ_TYPE_LEVEL_HIGH */
+	regmap_write(meson_gpio_irq_regmap, REG_EDGE_POL, 0);
+	/* disable all GPIO interrupt sources */
+	regmap_write(meson_gpio_irq_regmap, REG_PIN_03_SEL, 0xffffffff);
+	regmap_write(meson_gpio_irq_regmap, REG_PIN_47_SEL, 0xffffffff);
+	/* disable filtering */
+	regmap_write(meson_gpio_irq_regmap, REG_FILTER_SEL, 0);
+
+	ret = meson_gpio_get_irqs(pdev);
+	if (ret)
+		return ret;
+
+	meson_pinctrl_irq_chip = &meson_gpio_irq_chip;
+
+	return 0;
+}
+
+static const struct of_device_id meson_gpio_irq_dt_match[] = {
+	{ .compatible = "amlogic,meson-gpio-interrupt" },
+	{ },
+};
+
+static struct platform_driver meson_gpio_irq_driver = {
+	.probe		= meson_gpio_irq_probe,
+	.driver = {
+		.name	= "meson-gpio-interrupt",
+		.of_match_table = meson_gpio_irq_dt_match,
+	},
+};
+builtin_platform_driver(meson_gpio_irq_driver);

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

index 39ad9861..c587f6f0 100644
@@ -62,6 +62,8 @@

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

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

+struct irq_chip *meson_pinctrl_irq_chip;
+

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

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

@@ -551,7 +553,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_pinctrl_irq_chip, 0,
+				????handle_simple_irq, IRQ_TYPE_NONE);

?}
?
?static struct regmap_config meson_regmap_config = {

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

@@ -640,6 +643,9 @@ static int meson_pinctrl_probe(struct platform_device
 *pdev)
 ?	struct meson_pinctrl *pc;
 ?	int ret;

?

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

+	if (!meson_pinctrl_irq_chip)
+		return -EPROBE_DEFER;
+

? pc = devm_kzalloc(dev, sizeof(struct meson_pinctrl), GFP_KERNEL);
? if (!pc)
? return -ENOMEM;

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

index 40b56aff..16aab328 100644
@@ -176,6 +176,7 @@ extern struct meson_pinctrl_data
 meson_gxbb_periphs_pinctrl_data;
 ?extern struct meson_pinctrl_data meson_gxbb_aobus_pinctrl_data;
 ?extern struct meson_pinctrl_data meson_gxl_periphs_pinctrl_data;
 ?extern struct meson_pinctrl_data meson_gxl_aobus_pinctrl_data;
+extern struct irq_chip *meson_pinctrl_irq_chip;

?
?struct meson_bank *meson_pinctrl_get_bank(const struct meson_pinctrl *pc,
? ??unsigned int pin);

Quoted 166 lines

Add support for GPIO interrupts on Amlogic Meson SoC's.

There's a limit of 8 parent interupts which can be used in total. Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's, one for each edge.

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

---
v2:
- make the GPIO IRQ controller a separate driver
- several smaller improvements

---
 drivers/pinctrl/Kconfig                   |   1 +
 drivers/pinctrl/meson/Makefile            |   2 +-
 drivers/pinctrl/meson/pinctrl-meson-irq.c | 367
++++++++++++++++++++++++++++++
 drivers/pinctrl/meson/pinctrl-meson.c     |   8 +-
 drivers/pinctrl/meson/pinctrl-meson.h     |   1 +
 5 files changed, 377 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pinctrl/meson/pinctrl-meson-irq.c

drivers/pinctrl/Kconfig +1 -0

index 37af5e30..f8f401a0 100644
@@ -153,6 +153,7 @@ config PINCTRL_MESON
 	select PINCONF
 	select GENERIC_PINCONF
 	select GPIOLIB
+	select GPIOLIB_IRQCHIP
 	select OF_GPIO
 	select REGMAP_MMIO
 

drivers/pinctrl/meson/Makefile +1 -1

index 27c5b512..827e416d 100644
@@ -1,3 +1,3 @@
 obj-y	+= pinctrl-meson8.o pinctrl-meson8b.o
 obj-y	+= pinctrl-meson-gxbb.o pinctrl-meson-gxl.o
-obj-y	+= pinctrl-meson.o
+obj-y	+= pinctrl-meson.o pinctrl-meson-irq.o

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +104 -0

new file mode 100644
index 00000000..c5f403f3
@@ -0,0 +1,367 @@
+/*
+ * Amlogic Meson GPIO IRQ driver
+ *
+ * Copyright 2017 Heiner Kallweit <[email protected]>
+ * Based on a first version by Jerome Brunet <[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/gpio.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include "pinctrl-meson.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))
+
+#define MESON_GPIO_MAX_PARENT_IRQ_NUM	8
+
+struct meson_gpio_irq_slot {
+	int irq;
+	int owner;
+};
+
+static struct regmap *meson_gpio_irq_regmap;
+static struct meson_gpio_irq_slot
+		meson_gpio_irq_slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
+static int meson_gpio_num_irq_slots;
+static DEFINE_MUTEX(meson_gpio_irq_slot_mutex);
+static DECLARE_BITMAP(meson_gpio_irq_locked, 256);
+
+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_to_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_irq(struct meson_pinctrl *pc, unsigned int offset)
+{
+	struct meson_bank *bank;
+	int hwirq;
+
+	offset += pc->data->pin_base;
+
+	bank = meson_pinctrl_get_bank(pc, offset);
+	if (IS_ERR(bank))
+		return PTR_ERR(bank);
+
+	hwirq = meson_gpio_to_hwirq(bank, offset);
+	if (!hwirq)
+		dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
+
+	return hwirq;
+}
+
+static int meson_gpio_data_to_hwirq(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	unsigned gpio = irqd_to_hwirq(data);
+
+	return meson_gpio_to_irq(pc, gpio);
+}
+
+static irqreturn_t meson_gpio_irq_handler(int irq, void *data)
+{
+	struct irq_data *gpio_irqdata = data;
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	int hwirq = meson_gpio_data_to_hwirq(gpio_irqdata);
+
+	/*
+	 * For some strange reason spurious interrupts created by the chip

when

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

+	 * the interrupt source registers are written cause a deadlock here.
+	 * generic_handle_irq calls handle_simple_irq which tries to get
+	 * spinlock desc->lock. This interrupt handler is called whilst
+	 * __setup_irq holds desc->lock.
+	 * The deadlock means that both are running on the same CPU what

should

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +1 -0

+	 * not happen as __setup_irq called raw_spin_lock_irqsave thus

disabling

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +3 -0

+	 * interrupts on this CPU.
+	 * Work around this by ignoring interrupts in code protected by
+	 * chip_bus_lock (__setup_irq/__free_irq for the respective GPIO

hwirq).

drivers/pinctrl/meson/pinctrl-meson-irq.c

Quoted 1 line

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +1 -0

+	 */

The explanation for this seems a bit weak. Even your comment say it does not make sense. I've never seen this 'spurious irq' during my test.

It be nice to have the beginning of an explanation before introducing such hack.

Quoted 138 lines

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +138 -0

+	if (test_bit(hwirq, meson_gpio_irq_locked))
+		dev_dbg(pc->dev, "spurious interrupt detected!\n");
+	else
+		generic_handle_irq(gpio_irqdata->irq);
+
+	return IRQ_HANDLED;
+}
+
+static int meson_gpio_alloc_irq_slot(struct irq_data *data, int num_slots,
+				     int *slots)
+{
+	int i, cnt = 0;
+
+	mutex_lock(&meson_gpio_irq_slot_mutex);
+
+	for (i = 0; i < meson_gpio_num_irq_slots; i++)
+		if (!meson_gpio_irq_slots[i].owner) {
+			meson_gpio_irq_slots[i].owner = data->irq;
+			slots[cnt++] = i;
+			if (cnt == num_slots)
+				break;
+		}
+
+	if (cnt < num_slots)
+		for (i = 0; i < cnt; i++)
+			meson_gpio_irq_slots[i].owner = 0;
+
+	mutex_unlock(&meson_gpio_irq_slot_mutex);
+
+	return cnt == num_slots ? 0 : -ENOSPC;
+}
+
+static void meson_gpio_free_irq_slot(struct irq_data *data)
+{
+	int i;
+
+	mutex_lock(&meson_gpio_irq_slot_mutex);
+
+	for (i = 0; i < meson_gpio_num_irq_slots; i++)
+		if (meson_gpio_irq_slots[i].owner == data->irq) {
+			free_irq(meson_gpio_irq_slots[i].irq, data);
+			meson_gpio_irq_slots[i].owner = 0;
+		}
+
+	mutex_unlock(&meson_gpio_irq_slot_mutex);
+}
+
+static int meson_gpio_find_irq_slot(struct irq_data *data, int *slots)
+{
+	int i, cnt = 0;
+
+	mutex_lock(&meson_gpio_irq_slot_mutex);
+
+	for (i = 0; i < meson_gpio_num_irq_slots; i++)
+		if (meson_gpio_irq_slots[i].owner == data->irq)
+			slots[cnt++] = i;
+
+	mutex_unlock(&meson_gpio_irq_slot_mutex);
+
+	return cnt ?: -EINVAL;
+}
+
+static void meson_gpio_set_hwirq(int idx, int hwirq)
+{
+	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
+	int shift = 8 * (idx % 4);
+
+	regmap_update_bits(meson_gpio_irq_regmap, reg, 0xff << shift,
+			   hwirq << shift);
+}
+
+static void meson_gpio_irq_set_hwirq(struct irq_data *data, int hwirq)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	int i, cnt, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
+
+	cnt = meson_gpio_find_irq_slot(data, slots);
+	if (cnt < 0) {
+		dev_err(pc->dev, "didn't find gpio irq slot\n");
+		return;
+	}
+
+	for (i = 0; i < cnt; i++)
+		meson_gpio_set_hwirq(slots[i], hwirq);
+}
+
+static void meson_gpio_irq_unmask(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	unsigned gpio = irqd_to_hwirq(data);
+	int hwirq = meson_gpio_to_irq(pc, gpio);
+
+	meson_gpio_irq_set_hwirq(data, hwirq);
+}
+
+static void meson_gpio_irq_mask(struct irq_data *data)
+{
+	meson_gpio_irq_set_hwirq(data, 0xff);
+}
+
+static void meson_gpio_irq_shutdown(struct irq_data *data)
+{
+	meson_gpio_irq_mask(data);
+	meson_gpio_free_irq_slot(data);
+}
+
+static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	int i, ret, irq, num_slots, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
+	unsigned int val = 0;
+
+	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
+	ret = meson_gpio_alloc_irq_slot(data, num_slots, slots);
+	if (ret)
+		return ret;
+
+	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_EDGE(slots[0]);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_LOW(slots[0]);
+
+	regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
+			   REG_EDGE_POL_MASK(slots[0]), val);
+
+	/*
+	 * 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.
+	 */
+	if (num_slots > 1) {
+		val = REG_EDGE_POL_EDGE(slots[1]);
+		regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
+				   REG_EDGE_POL_MASK(slots[1]), val);
+	}
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val = IRQ_TYPE_EDGE_RISING;

drivers/pinctrl/meson/pinctrl-meson-irq.c line 262

Quoted 10 lines

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

+	else
+		val = IRQ_TYPE_LEVEL_HIGH;
+
+	for (i = 0; i < num_slots; i++) {
+		irq = meson_gpio_irq_slots[slots[i]].irq;
+		ret = irq_set_irq_type(irq, val);
+		if (ret)
+			break;
+		ret = request_irq(irq, meson_gpio_irq_handler, 0,
+				  "GPIO parent", data);

It seems weird to use request_irq here. With the eventual reuqeste_irq in the consumer driver, wouldn't you end with to virq allocated for what is only on irq line really ?
Was changed to chained irq handling in v3 and this issue is gone therefore.

Isn't it showing that you should use domains and mappings ?

Quoted 155 lines

drivers/pinctrl/meson/pinctrl-meson-irq.c new file +105 -0

+		if (ret)
+			break;
+	}
+
+	if (ret)
+		while (--i >= 0)
+			free_irq(meson_gpio_irq_slots[slots[i]].irq, data);
+
+	return ret;
+}
+
+static void meson_gpio_irq_bus_lock(struct irq_data *data)
+{
+	int hwirq = meson_gpio_data_to_hwirq(data);
+
+	set_bit(hwirq, meson_gpio_irq_locked);
+}
+
+static void meson_gpio_irq_bus_sync_unlock(struct irq_data *data)
+{
+	int hwirq = meson_gpio_data_to_hwirq(data);
+
+	clear_bit(hwirq, meson_gpio_irq_locked);
+}
+
+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,
+	.irq_bus_lock = meson_gpio_irq_bus_lock,
+	.irq_bus_sync_unlock = meson_gpio_irq_bus_sync_unlock,
+};
+
+static int meson_gpio_get_irqs(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int irq, i;
+
+	for (i = 0; i < MESON_GPIO_MAX_PARENT_IRQ_NUM; i++) {
+		irq = irq_of_parse_and_map(np, i);
+		if (!irq)
+			break;
+		meson_gpio_irq_slots[i].irq = irq;
+	}
+
+	meson_gpio_num_irq_slots = i;
+
+	return i ? 0 : -EINVAL;
+}
+
+static const struct regmap_config meson_gpio_regmap_config = {
+	.reg_bits       = 32,
+	.reg_stride     = 4,
+	.val_bits       = 32,
+	.max_register	= REG_FILTER_SEL,
+};
+
+static int meson_gpio_irq_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	void __iomem *io_base;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	io_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(io_base))
+		return PTR_ERR(io_base);
+
+	meson_gpio_irq_regmap = devm_regmap_init_mmio(&pdev->dev, io_base,
+						&meson_gpio_regmap_config);
+	if (IS_ERR(meson_gpio_irq_regmap))
+		return PTR_ERR(meson_gpio_irq_regmap);
+
+	/* initialize to IRQ_TYPE_LEVEL_HIGH */
+	regmap_write(meson_gpio_irq_regmap, REG_EDGE_POL, 0);
+	/* disable all GPIO interrupt sources */
+	regmap_write(meson_gpio_irq_regmap, REG_PIN_03_SEL, 0xffffffff);
+	regmap_write(meson_gpio_irq_regmap, REG_PIN_47_SEL, 0xffffffff);
+	/* disable filtering */
+	regmap_write(meson_gpio_irq_regmap, REG_FILTER_SEL, 0);
+
+	ret = meson_gpio_get_irqs(pdev);
+	if (ret)
+		return ret;
+
+	meson_pinctrl_irq_chip = &meson_gpio_irq_chip;
+
+	return 0;
+}
+
+static const struct of_device_id meson_gpio_irq_dt_match[] = {
+	{ .compatible = "amlogic,meson-gpio-interrupt" },
+	{ },
+};
+
+static struct platform_driver meson_gpio_irq_driver = {
+	.probe		= meson_gpio_irq_probe,
+	.driver = {
+		.name	= "meson-gpio-interrupt",
+		.of_match_table = meson_gpio_irq_dt_match,
+	},
+};
+builtin_platform_driver(meson_gpio_irq_driver);

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

index 39ad9861..c587f6f0 100644
@@ -62,6 +62,8 @@
 #include "../pinctrl-utils.h"
 #include "pinctrl-meson.h"
 
+struct irq_chip *meson_pinctrl_irq_chip;
+
 /**
  * meson_pinctrl_get_bank() - find the bank containing a given pin
  *

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

static struct regmap_config meson_regmap_config = {

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

@@ -640,6 +643,9 @@ static int meson_pinctrl_probe(struct platform_device
 *pdev)
 	struct meson_pinctrl *pc;
 	int ret;
 
+	if (!meson_pinctrl_irq_chip)
+		return -EPROBE_DEFER;
+
 	pc = devm_kzalloc(dev, sizeof(struct meson_pinctrl), GFP_KERNEL);
 	if (!pc)

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

 		return -ENOMEM;

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

index 40b56aff..16aab328 100644
@@ -176,6 +176,7 @@ extern struct meson_pinctrl_data
 meson_gxbb_periphs_pinctrl_data;
 extern struct meson_pinctrl_data meson_gxbb_aobus_pinctrl_data;
 extern struct meson_pinctrl_data meson_gxl_periphs_pinctrl_data;
 extern struct meson_pinctrl_data meson_gxl_aobus_pinctrl_data;
+extern struct irq_chip *meson_pinctrl_irq_chip;
 
 struct meson_bank *meson_pinctrl_get_bank(const struct meson_pinctrl *pc,

unsigned int pin);