[PATCH v2 0/6] pintrl: meson: add support for GPIO IRQs

[PATCH v2 0/6] pintrl: meson: add support for GPIO IRQs

[Heiner Kallweit]

This patch series is partially based on a series Jerome Brunet
submitted about half a year ago. Due to open questions this series never
made it to mainline, see https://patchwork.kernel.org/patch/9384431/

This new attempt uses GPIOLIB_IRQCHIP resulting in less needed code.
Included is also support for using two parent IRQs in case
of IRQ_TYPE_EDGE_BOTH, like in the vendor driver.

Worth to be mentioned is also that I had to work around a strange
issue with spurious interrupts resulting in a deadlock.
The affected critical section in __setup_irq is executed with interrupts
disabled, nevertheless spurious GPIO IRQs result in a deadlock what
seems to indicate that they are handled on the same CPU.
I didn't find a better explanation than a possible HW flaw so far.

The series was successfully tested on a Odroid-C2, e.g. with removing
polling for SD card insertion/removal from the mmc driver.

Changes in v2:
- separate the GPIO IRQ controller from the pinctrl driver
- minor improvements to the GPIO IRQ controller

Heiner Kallweit (5):
  pinctrl: meson: add interrupts to pinctrl data
  pinctrl: meson: document GPIO IRQ DT binding
  pinctrl: meson: add DT node for GPIO IRQ on Meson GX
  pinctrl: meson: add DT node for GPIO IRQ on Meson 8 / 8b
  pinctrl: meson: improve meson_get_bank and export it
  pinctrl: meson: add support for GPIO interrupts

 .../bindings/gpio/amlogic,meson-gpio-interrupt.txt |  30 ++
 arch/arm/boot/dts/meson8.dtsi                      |  13 +
 arch/arm/boot/dts/meson8b.dtsi                     |  13 +
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi          |  13 +
 drivers/pinctrl/Kconfig                            |   1 +
 drivers/pinctrl/meson/Makefile                     |   2 +-
 drivers/pinctrl/meson/pinctrl-meson-gxbb.c         |  22 +-
 drivers/pinctrl/meson/pinctrl-meson-gxl.c          |  20 +-
 drivers/pinctrl/meson/pinctrl-meson-irq.c          | 367 +++++++++++++++++++++
 drivers/pinctrl/meson/pinctrl-meson.c              |  67 ++--
 drivers/pinctrl/meson/pinctrl-meson.h              |  19 +-
 drivers/pinctrl/meson/pinctrl-meson8.c             |  20 +-
 drivers/pinctrl/meson/pinctrl-meson8b.c            |  32 +-
 13 files changed, 538 insertions(+), 81 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/amlogic,meson-gpio-interrupt.txt
 create mode 100644 drivers/pinctrl/meson/pinctrl-meson-irq.c

-- 
2.12.2

[PATCH v2 1/6] pintrl: meson: add interrupts to pinctrl data

[Heiner Kallweit]

From: Jerome Brunet <jbrunet@baylibre.com>
Add GPIO interrupt information to pinctrl data. Added to the original
version from Jerome was data for Meson GXL.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- no changes
---
 drivers/pinctrl/meson/pinctrl-meson-gxbb.c | 22 ++++++++++----------
 drivers/pinctrl/meson/pinctrl-meson-gxl.c  | 20 +++++++++----------
 drivers/pinctrl/meson/pinctrl-meson.h      | 15 +++++++++-----
 drivers/pinctrl/meson/pinctrl-meson8.c     | 20 +++++++++----------
 drivers/pinctrl/meson/pinctrl-meson8b.c    | 32 ++++++++++++++++++++----------
 5 files changed, 63 insertions(+), 46 deletions(-)

diff --git a/drivers/pinctrl/meson/pinctrl-meson-gxbb.c b/drivers/pinctrl/meson/pinctrl-meson-gxbb.c
index 9b00be15..e54dbeaa 100644
--- a/drivers/pinctrl/meson/pinctrl-meson-gxbb.c
+++ b/drivers/pinctrl/meson/pinctrl-meson-gxbb.c
@@ -782,20 +782,20 @@ static struct meson_pmx_func meson_gxbb_aobus_functions[] = {
 };
 
 static struct meson_bank meson_gxbb_periphs_banks[] = {
-	/*   name    first                      last                    pullen  pull    dir     out     in  */
-	BANK("X",    PIN(GPIOX_0, EE_OFF),	PIN(GPIOX_22, EE_OFF),  4,  0,  4,  0,  12, 0,  13, 0,  14, 0),
-	BANK("Y",    PIN(GPIOY_0, EE_OFF),	PIN(GPIOY_16, EE_OFF),  1,  0,  1,  0,  3,  0,  4,  0,  5,  0),
-	BANK("DV",   PIN(GPIODV_0, EE_OFF),	PIN(GPIODV_29, EE_OFF), 0,  0,  0,  0,  0,  0,  1,  0,  2,  0),
-	BANK("H",    PIN(GPIOH_0, EE_OFF),	PIN(GPIOH_3, EE_OFF),   1, 20,  1, 20,  3, 20,  4, 20,  5, 20),
-	BANK("Z",    PIN(GPIOZ_0, EE_OFF),	PIN(GPIOZ_15, EE_OFF),  3,  0,  3,  0,  9,  0,  10, 0, 11,  0),
-	BANK("CARD", PIN(CARD_0, EE_OFF),	PIN(CARD_6, EE_OFF),    2, 20,  2, 20,  6, 20,  7, 20,  8, 20),
-	BANK("BOOT", PIN(BOOT_0, EE_OFF),	PIN(BOOT_17, EE_OFF),   2,  0,  2,  0,  6,  0,  7,  0,  8,  0),
-	BANK("CLK",  PIN(GPIOCLK_0, EE_OFF),	PIN(GPIOCLK_3, EE_OFF), 3, 28,  3, 28,  9, 28, 10, 28, 11, 28),
+	/*   name    first                      last                    irq       pullen  pull    dir     out     in  */
+	BANK("X",    PIN(GPIOX_0, EE_OFF),	PIN(GPIOX_22, EE_OFF),  106, 128, 4,  0,  4,  0,  12, 0,  13, 0,  14, 0),
+	BANK("Y",    PIN(GPIOY_0, EE_OFF),	PIN(GPIOY_16, EE_OFF),   89, 105, 1,  0,  1,  0,  3,  0,  4,  0,  5,  0),
+	BANK("DV",   PIN(GPIODV_0, EE_OFF),	PIN(GPIODV_29, EE_OFF),  59,  88, 0,  0,  0,  0,  0,  0,  1,  0,  2,  0),
+	BANK("H",    PIN(GPIOH_0, EE_OFF),	PIN(GPIOH_3, EE_OFF),    30,  33, 1, 20,  1, 20,  3, 20,  4, 20,  5, 20),
+	BANK("Z",    PIN(GPIOZ_0, EE_OFF),	PIN(GPIOZ_15, EE_OFF),   14,  29, 3,  0,  3,  0,  9,  0,  10, 0, 11,  0),
+	BANK("CARD", PIN(CARD_0, EE_OFF),	PIN(CARD_6, EE_OFF),     52,  58, 2, 20,  2, 20,  6, 20,  7, 20,  8, 20),
+	BANK("BOOT", PIN(BOOT_0, EE_OFF),	PIN(BOOT_17, EE_OFF),    34,  51, 2,  0,  2,  0,  6,  0,  7,  0,  8,  0),
+	BANK("CLK",  PIN(GPIOCLK_0, EE_OFF),	PIN(GPIOCLK_3, EE_OFF), 129, 132, 3, 28,  3, 28,  9, 28, 10, 28, 11, 28),
 };
 
 static struct meson_bank meson_gxbb_aobus_banks[] = {
-	/*   name    first              last               pullen  pull    dir     out     in  */
-	BANK("AO",   PIN(GPIOAO_0, 0),  PIN(GPIOAO_13, 0), 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
+	/*   name    first              last               irq    pullen  pull    dir     out     in  */
+	BANK("AO",   PIN(GPIOAO_0, 0),  PIN(GPIOAO_13, 0), 0, 13, 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
 };
 
 struct meson_pinctrl_data meson_gxbb_periphs_pinctrl_data = {
diff --git a/drivers/pinctrl/meson/pinctrl-meson-gxl.c b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
index 998210ea..bcd8da10 100644
--- a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
+++ b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
@@ -729,19 +729,19 @@ static struct meson_pmx_func meson_gxl_aobus_functions[] = {
 };
 
 static struct meson_bank meson_gxl_periphs_banks[] = {
-	/*   name    first                      last                    pullen  pull    dir     out     in  */
-	BANK("X",    PIN(GPIOX_0, EE_OFF),	PIN(GPIOX_18, EE_OFF),  4,  0,  4,  0,  12, 0,  13, 0,  14, 0),
-	BANK("DV",   PIN(GPIODV_0, EE_OFF),	PIN(GPIODV_29, EE_OFF), 0,  0,  0,  0,  0,  0,  1,  0,  2,  0),
-	BANK("H",    PIN(GPIOH_0, EE_OFF),	PIN(GPIOH_9, EE_OFF),   1, 20,  1, 20,  3, 20,  4, 20,  5, 20),
-	BANK("Z",    PIN(GPIOZ_0, EE_OFF),	PIN(GPIOZ_15, EE_OFF),  3,  0,  3,  0,  9,  0,  10, 0, 11,  0),
-	BANK("CARD", PIN(CARD_0, EE_OFF),	PIN(CARD_6, EE_OFF),    2, 20,  2, 20,  6, 20,  7, 20,  8, 20),
-	BANK("BOOT", PIN(BOOT_0, EE_OFF),	PIN(BOOT_15, EE_OFF),   2,  0,  2,  0,  6,  0,  7,  0,  8,  0),
-	BANK("CLK",  PIN(GPIOCLK_0, EE_OFF),	PIN(GPIOCLK_1, EE_OFF), 3, 28,  3, 28,  9, 28, 10, 28, 11, 28),
+	/*   name    first                      last                    irq	  pullen  pull    dir     out     in  */
+	BANK("X",    PIN(GPIOX_0, EE_OFF),	PIN(GPIOX_18, EE_OFF),   89, 107, 4,  0,  4,  0,  12, 0,  13, 0,  14, 0),
+	BANK("DV",   PIN(GPIODV_0, EE_OFF),	PIN(GPIODV_29, EE_OFF),  83,  88, 0,  0,  0,  0,  0,  0,  1,  0,  2,  0),
+	BANK("H",    PIN(GPIOH_0, EE_OFF),	PIN(GPIOH_9, EE_OFF),    26,  35, 1, 20,  1, 20,  3, 20,  4, 20,  5, 20),
+	BANK("Z",    PIN(GPIOZ_0, EE_OFF),	PIN(GPIOZ_15, EE_OFF),   10,  25, 3,  0,  3,  0,  9,  0,  10, 0, 11,  0),
+	BANK("CARD", PIN(CARD_0, EE_OFF),	PIN(CARD_6, EE_OFF),     52,  58, 2, 20,  2, 20,  6, 20,  7, 20,  8, 20),
+	BANK("BOOT", PIN(BOOT_0, EE_OFF),	PIN(BOOT_15, EE_OFF),    36,  51, 2,  0,  2,  0,  6,  0,  7,  0,  8,  0),
+	BANK("CLK",  PIN(GPIOCLK_0, EE_OFF),	PIN(GPIOCLK_1, EE_OFF), 108, 109, 3, 28,  3, 28,  9, 28, 10, 28, 11, 28),
 };
 
 static struct meson_bank meson_gxl_aobus_banks[] = {
-	/*   name    first              last              pullen  pull    dir     out     in  */
-	BANK("AO",   PIN(GPIOAO_0, 0),  PIN(GPIOAO_9, 0), 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
+	/*   name    first              last              irq	pullen  pull    dir     out     in  */
+	BANK("AO",   PIN(GPIOAO_0, 0),  PIN(GPIOAO_9, 0), 0, 9, 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
 };
 
 struct meson_pinctrl_data meson_gxl_periphs_pinctrl_data = {
diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
index 1aa871d5..890f296f 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.h
+++ b/drivers/pinctrl/meson/pinctrl-meson.h
@@ -81,6 +81,7 @@ enum meson_reg_type {
  * @name:	bank name
  * @first:	first pin of the bank
  * @last:	last pin of the bank
+ * @irq:	hwirq base number of the bank
  * @regs:	array of register descriptors
  *
  * A bank represents a set of pins controlled by a contiguous set of
@@ -92,6 +93,8 @@ struct meson_bank {
 	const char *name;
 	unsigned int first;
 	unsigned int last;
+	int irq_first;
+	int irq_last;
 	struct meson_reg_desc regs[NUM_REG];
 };
 
@@ -147,12 +150,14 @@ struct meson_pinctrl {
 		.num_groups = ARRAY_SIZE(fn ## _groups),		\
 	}
 
-#define BANK(n, f, l, per, peb, pr, pb, dr, db, or, ob, ir, ib)		\
+#define BANK(n, f, l, fi, li, per, peb, pr, pb, dr, db, or, ob, ir, ib)	\
 	{								\
-		.name	= n,						\
-		.first	= f,						\
-		.last	= l,						\
-		.regs	= {						\
+		.name		= n,					\
+		.first		= f,					\
+		.last		= l,					\
+		.irq_first	= fi,					\
+		.irq_last	= li,					\
+		.regs = {						\
 			[REG_PULLEN]	= { per, peb },			\
 			[REG_PULL]	= { pr, pb },			\
 			[REG_DIR]	= { dr, db },			\
diff --git a/drivers/pinctrl/meson/pinctrl-meson8.c b/drivers/pinctrl/meson/pinctrl-meson8.c
index 07f1cb21..32449820 100644
--- a/drivers/pinctrl/meson/pinctrl-meson8.c
+++ b/drivers/pinctrl/meson/pinctrl-meson8.c
@@ -916,19 +916,19 @@ static struct meson_pmx_func meson8_aobus_functions[] = {
 };
 
 static struct meson_bank meson8_cbus_banks[] = {
-	/*   name    first             last                 pullen  pull    dir     out     in  */
-	BANK("X",    PIN(GPIOX_0, 0),  PIN(GPIOX_21, 0),    4,  0,  4,  0,  0,  0,  1,  0,  2,  0),
-	BANK("Y",    PIN(GPIOY_0, 0),  PIN(GPIOY_16, 0),    3,  0,  3,  0,  3,  0,  4,  0,  5,  0),
-	BANK("DV",   PIN(GPIODV_0, 0), PIN(GPIODV_29, 0),   0,  0,  0,  0,  7,  0,  8,  0,  9,  0),
-	BANK("H",    PIN(GPIOH_0, 0),  PIN(GPIOH_9, 0),     1, 16,  1, 16,  9, 19, 10, 19, 11, 19),
-	BANK("Z",    PIN(GPIOZ_0, 0),  PIN(GPIOZ_14, 0),    1,  0,  1,  0,  3, 17,  4, 17,  5, 17),
-	BANK("CARD", PIN(CARD_0, 0),   PIN(CARD_6, 0),      2, 20,  2, 20,  0, 22,  1, 22,  2, 22),
-	BANK("BOOT", PIN(BOOT_0, 0),   PIN(BOOT_18, 0),     2,  0,  2,  0,  9,  0, 10,  0, 11,  0),
+	/*   name    first             last                 irq       pullen  pull    dir     out     in  */
+	BANK("X",    PIN(GPIOX_0, 0),  PIN(GPIOX_21, 0),    112, 133, 4,  0,  4,  0,  0,  0,  1,  0,  2,  0),
+	BANK("Y",    PIN(GPIOY_0, 0),  PIN(GPIOY_16, 0),    95,  111, 3,  0,  3,  0,  3,  0,  4,  0,  5,  0),
+	BANK("DV",   PIN(GPIODV_0, 0), PIN(GPIODV_29, 0),   65,   94, 0,  0,  0,  0,  7,  0,  8,  0,  9,  0),
+	BANK("H",    PIN(GPIOH_0, 0),  PIN(GPIOH_9, 0),     29,   38, 1, 16,  1, 16,  9, 19, 10, 19, 11, 19),
+	BANK("Z",    PIN(GPIOZ_0, 0),  PIN(GPIOZ_14, 0),    14,   28, 1,  0,  1,  0,  3, 17,  4, 17,  5, 17),
+	BANK("CARD", PIN(CARD_0, 0),   PIN(CARD_6, 0),      58,   64, 2, 20,  2, 20,  0, 22,  1, 22,  2, 22),
+	BANK("BOOT", PIN(BOOT_0, 0),   PIN(BOOT_18, 0),     39,   57, 2,  0,  2,  0,  9,  0, 10,  0, 11,  0),
 };
 
 static struct meson_bank meson8_aobus_banks[] = {
-	/*   name    first                  last                      pullen  pull    dir     out     in  */
-	BANK("AO",   PIN(GPIOAO_0, AO_OFF), PIN(GPIO_TEST_N, AO_OFF), 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
+	/*   name    first                  last                      irq    pullen  pull    dir     out     in  */
+	BANK("AO",   PIN(GPIOAO_0, AO_OFF), PIN(GPIO_TEST_N, AO_OFF), 0, 13, 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
 };
 
 struct meson_pinctrl_data meson8_cbus_pinctrl_data = {
diff --git a/drivers/pinctrl/meson/pinctrl-meson8b.c b/drivers/pinctrl/meson/pinctrl-meson8b.c
index bf747eb1..71f216b5 100644
--- a/drivers/pinctrl/meson/pinctrl-meson8b.c
+++ b/drivers/pinctrl/meson/pinctrl-meson8b.c
@@ -124,6 +124,12 @@ static const struct pinctrl_pin_desc meson8b_aobus_pins[] = {
 	MESON_PIN(GPIOAO_11, AO_OFF),
 	MESON_PIN(GPIOAO_12, AO_OFF),
 	MESON_PIN(GPIOAO_13, AO_OFF),
+
+	/*
+	 * The following 2 pins are not mentionned in the public datasheet
+	 * According to this datasheet, they can't be used with the gpio
+	 * interrupt controller
+	 */
 	MESON_PIN(GPIO_BSD_EN, AO_OFF),
 	MESON_PIN(GPIO_TEST_N, AO_OFF),
 };
@@ -881,19 +887,25 @@ static struct meson_pmx_func meson8b_aobus_functions[] = {
 };
 
 static struct meson_bank meson8b_cbus_banks[] = {
-	/*   name    first                      last                   pullen  pull    dir     out     in  */
-	BANK("X",    PIN(GPIOX_0, 0),		PIN(GPIOX_21, 0),      4,  0,  4,  0,  0,  0,  1,  0,  2,  0),
-	BANK("Y",    PIN(GPIOY_0, 0),		PIN(GPIOY_14, 0),      3,  0,  3,  0,  3,  0,  4,  0,  5,  0),
-	BANK("DV",   PIN(GPIODV_9, 0),		PIN(GPIODV_29, 0),     0,  0,  0,  0,  7,  0,  8,  0,  9,  0),
-	BANK("H",    PIN(GPIOH_0, 0),		PIN(GPIOH_9, 0),       1, 16,  1, 16,  9, 19, 10, 19, 11, 19),
-	BANK("CARD", PIN(CARD_0, 0),		PIN(CARD_6, 0),        2, 20,  2, 20,  0, 22,  1, 22,  2, 22),
-	BANK("BOOT", PIN(BOOT_0, 0),		PIN(BOOT_18, 0),       2,  0,  2,  0,  9,  0, 10,  0, 11,  0),
-	BANK("DIF",  PIN(DIF_0_P, 0),		PIN(DIF_4_N, 0),       5,  8,  5,  8, 12, 12, 13, 12, 14, 12),
+	/*   name    first                      last                irq      pullen  pull    dir     out     in  */
+	BANK("X",    PIN(GPIOX_0, 0),		PIN(GPIOX_21, 0),   97, 118, 4,  0,  4,  0,  0,  0,  1,  0,  2,  0),
+	BANK("Y",    PIN(GPIOY_0, 0),		PIN(GPIOY_14, 0),   80,  96, 3,  0,  3,  0,  3,  0,  4,  0,  5,  0),
+	BANK("DV",   PIN(GPIODV_9, 0),		PIN(GPIODV_29, 0),  59,  79, 0,  0,  0,  0,  7,  0,  8,  0,  9,  0),
+	BANK("H",    PIN(GPIOH_0, 0),		PIN(GPIOH_9, 0),    14,  23, 1, 16,  1, 16,  9, 19, 10, 19, 11, 19),
+	BANK("CARD", PIN(CARD_0, 0),		PIN(CARD_6, 0),     43,  49, 2, 20,  2, 20,  0, 22,  1, 22,  2, 22),
+	BANK("BOOT", PIN(BOOT_0, 0),		PIN(BOOT_18, 0),    24,  42, 2,  0,  2,  0,  9,  0, 10,  0, 11,  0),
+
+	/*
+	 * The following bank is not mentionned in the public datasheet
+	 * There is no information whether it can be used with the gpio
+	 * interrupt controller
+	 */
+	BANK("DIF",  PIN(DIF_0_P, 0),		PIN(DIF_4_N, 0),    -1,  -1, 5,  8,  5,  8, 12, 12, 13, 12, 14, 12),
 };
 
 static struct meson_bank meson8b_aobus_banks[] = {
-	/*   name    first                  last                      pullen  pull    dir     out     in  */
-	BANK("AO",   PIN(GPIOAO_0, AO_OFF), PIN(GPIO_TEST_N, AO_OFF), 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
+	/*   name    first                  last                      irq    pullen  pull    dir     out     in  */
+	BANK("AO",   PIN(GPIOAO_0, AO_OFF), PIN(GPIO_TEST_N, AO_OFF), 0, 13, 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
 };
 
 struct meson_pinctrl_data meson8b_cbus_pinctrl_data = {
-- 
2.12.2

[PATCH v2 2/6] pintrl: meson: document GPIO IRQ DT binding

[Heiner Kallweit]

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 <hkallweit1@gmail.com>
---
v2:
- remove syscon
---
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index 436b8750..44422b85 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -312,6 +312,19 @@
 				status = "disabled";
 			};
 
+			gpio_irq@9880 {
+				compatible = "amlogic,meson-gpio-interrupt";
+				reg = <0x0 0x09880 0x0 0x10>;
+				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>;
+			};
+
 			watchdog@98d0 {
 				compatible = "amlogic,meson-gx-wdt", "amlogic,meson-gxbb-wdt";
 				reg = <0x0 0x098d0 0x0 0x10>;
-- 
2.12.2

Re: [PATCH v2 2/6] pintrl: meson: document GPIO IRQ DT binding

[Jerome Brunet]

On Fri, 2017-05-12 at 21:13 +0200, Heiner Kallweit wrote:
> 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 <hkallweit1@gmail.com>
> ---
> v2:
> - remove syscon
> ---
>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> index 436b8750..44422b85 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> @@ -312,6 +312,19 @@
>  				status = "disabled";
>  			};
>  
> +			gpio_irq@9880 {
> +				compatible = "amlogic,meson-gpio-interrupt";
> +				reg = <0x0 0x09880 0x0 0x10>;
> +				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


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

Re: [PATCH v2 2/6] pintrl: meson: document GPIO IRQ DT binding

[Heiner Kallweit]

Am 17.05.2017 um 01:28 schrieb Jerome Brunet:
> On Fri, 2017-05-12 at 21:13 +0200, Heiner Kallweit wrote:
>> 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 <hkallweit1@gmail.com>
>> ---
>> v2:
>> - remove syscon
>> ---
>>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> index 436b8750..44422b85 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> @@ -312,6 +312,19 @@
>>  				status = "disabled";
>>  			};
>>  
>> +			gpio_irq@9880 {
>> +				compatible = "amlogic,meson-gpio-interrupt";
>> +				reg = <0x0 0x09880 0x0 0x10>;
>> +				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
> 
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.

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

Re: [PATCH v2 2/6] pintrl: meson: document GPIO IRQ DT binding

[Jerome Brunet]

On Wed, 2017-05-17 at 23:02 +0200, Heiner Kallweit wrote:
> Am 17.05.2017 um 01:28 schrieb Jerome Brunet:
> > On Fri, 2017-05-12 at 21:13 +0200, Heiner Kallweit wrote:
> > > 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 <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > ---
> > > v2:
> > > - remove syscon
> > > ---
> > >  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> > > b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> > > index 436b8750..44422b85 100644
> > > --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> > > +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> > > @@ -312,6 +312,19 @@
> > >  				status = "disabled";
> > >  			};
> > >  
> > > +			gpio_irq@9880 {
> > > +				compatible = "amlogic,meson-gpio-
> > > interrupt";
> > > +				reg = <0x0 0x09880 0x0 0x10>;
> > > +				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
> > 
> 
> 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.

You got the objections in my previous comment.
* This driver does *not* generate these irq, it just route them
* IRQ_TYPE_NONE in meaningless here

This comments initially came from Mark and Marc, on the first RFC I sent lastyear. 

> And such code duplication to me seems to be worse than using the interrupts
> property slightly different than in the theory.
> 
> > 
> > >  			watchdog@98d0 {
> > >  				compatible = "amlogic,meson-gx-wdt",
> > > "amlogic,meson-gxbb-wdt";
> > >  				reg = <0x0 0x098d0 0x0 0x10>;
> 
> 

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

[PATCH v2 3/6] pintrl: meson: add DT node for GPIO IRQ on Meson GX

[Heiner Kallweit]

Add the DT node for GPIO IRQ support on AMlogic Meson GX.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- remove syscon
---
 .../bindings/gpio/amlogic,meson-gpio-interrupt.txt | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/amlogic,meson-gpio-interrupt.txt

diff --git a/Documentation/devicetree/bindings/gpio/amlogic,meson-gpio-interrupt.txt b/Documentation/devicetree/bindings/gpio/amlogic,meson-gpio-interrupt.txt
new file mode 100644
index 00000000..ba7d3015
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/amlogic,meson-gpio-interrupt.txt
@@ -0,0 +1,30 @@
+Amlogic meson GPIO interrupt controller
+
+Meson SoCs contains an interrupt controller which is able watch the SoC pads
+and generate an interrupt on edges or level. The controller is essentially a
+256 pads to 8 GIC interrupt multiplexer, with a filter block to select edge
+or level and polarity. We don't expose all 256 mux inputs because the
+documentation shows that upper part is not mapped to any pad. The actual number
+of interrupt exposed depends on the SoC.
+
+Required properties:
+
+- compatible : should be "amlogic,meson-gpio-interrupt".
+- reg : Specifies base physical address and size of the registers.
+- interrupts : list of GIC interrupts which can be used with the
+	       GPIO IRQ multiplexer
+
+Example:
+
+gpio_irq@9880 {
+	compatible = "amlogic,meson-gpio-interrupt";
+	reg = <0x0 0x09880 0x0 0x10>;
+	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>;
+	};
-- 
2.12.2

[PATCH v2 5/6] pintrl: meson: improve meson_get_bank and export it

[Heiner Kallweit]

Export meson_get_bank because it's needed for the GPIO IRQ controller.
To avoid potential name conflicts rename it to meson_pinctrl_get_bank.

In addition simplify it by returning the pointer directly and using
a ERR_PTR in case of an error.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- add this patch to the series
---
 drivers/pinctrl/meson/pinctrl-meson.c | 59 +++++++++++++++--------------------
 drivers/pinctrl/meson/pinctrl-meson.h |  3 ++
 2 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
index 66ed70c1..39ad9861 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.c
+++ b/drivers/pinctrl/meson/pinctrl-meson.c
@@ -63,28 +63,24 @@
 #include "pinctrl-meson.h"
 
 /**
- * meson_get_bank() - find the bank containing a given pin
+ * meson_pinctrl_get_bank() - find the bank containing a given pin
  *
  * @pc:		the pinctrl instance
  * @pin:	the pin number
- * @bank:	the found bank
  *
- * Return:	0 on success, a negative value on error
+ * Return:	the found bank or an ERR_PTR
  */
-static int meson_get_bank(struct meson_pinctrl *pc, unsigned int pin,
-			  struct meson_bank **bank)
+struct meson_bank *meson_pinctrl_get_bank(const struct meson_pinctrl *pc,
+					  unsigned int pin)
 {
+	struct meson_bank *bank = pc->data->banks;
 	int i;
 
-	for (i = 0; i < pc->data->num_banks; i++) {
-		if (pin >= pc->data->banks[i].first &&
-		    pin <= pc->data->banks[i].last) {
-			*bank = &pc->data->banks[i];
-			return 0;
-		}
-	}
+	for (i = 0; i < pc->data->num_banks; i++, bank++)
+		if (pin >= bank->first && pin <= bank->last)
+			return bank;
 
-	return -EINVAL;
+	return ERR_PTR(-EINVAL);
 }
 
 /**
@@ -261,9 +257,9 @@ static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
 	unsigned int reg, bit;
 	int i, ret;
 
-	ret = meson_get_bank(pc, pin, &bank);
-	if (ret)
-		return ret;
+	bank = meson_pinctrl_get_bank(pc, pin);
+	if (IS_ERR(bank))
+		return PTR_ERR(bank);
 
 	for (i = 0; i < num_configs; i++) {
 		param = pinconf_to_config_param(configs[i]);
@@ -324,9 +320,9 @@ static int meson_pinconf_get_pull(struct meson_pinctrl *pc, unsigned int pin)
 	unsigned int reg, bit, val;
 	int ret, conf;
 
-	ret = meson_get_bank(pc, pin, &bank);
-	if (ret)
-		return ret;
+	bank = meson_pinctrl_get_bank(pc, pin);
+	if (IS_ERR(bank))
+		return PTR_ERR(bank);
 
 	meson_calc_reg_and_bit(bank, pin, REG_PULLEN, &reg, &bit);
 
@@ -427,12 +423,11 @@ static int meson_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
 	struct meson_pinctrl *pc = gpiochip_get_data(chip);
 	unsigned int reg, bit, pin;
 	struct meson_bank *bank;
-	int ret;
 
 	pin = pc->data->pin_base + gpio;
-	ret = meson_get_bank(pc, pin, &bank);
-	if (ret)
-		return ret;
+	bank = meson_pinctrl_get_bank(pc, pin);
+	if (IS_ERR(bank))
+		return PTR_ERR(bank);
 
 	meson_calc_reg_and_bit(bank, pin, REG_DIR, &reg, &bit);
 
@@ -448,9 +443,9 @@ static int meson_gpio_direction_output(struct gpio_chip *chip, unsigned gpio,
 	int ret;
 
 	pin = pc->data->pin_base + gpio;
-	ret = meson_get_bank(pc, pin, &bank);
-	if (ret)
-		return ret;
+	bank = meson_pinctrl_get_bank(pc, pin);
+	if (IS_ERR(bank))
+		return PTR_ERR(bank);
 
 	meson_calc_reg_and_bit(bank, pin, REG_DIR, &reg, &bit);
 	ret = regmap_update_bits(pc->reg_gpio, reg, BIT(bit), 0);
@@ -467,11 +462,10 @@ static void meson_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
 	struct meson_pinctrl *pc = gpiochip_get_data(chip);
 	unsigned int reg, bit, pin;
 	struct meson_bank *bank;
-	int ret;
 
 	pin = pc->data->pin_base + gpio;
-	ret = meson_get_bank(pc, pin, &bank);
-	if (ret)
+	bank = meson_pinctrl_get_bank(pc, pin);
+	if (IS_ERR(bank))
 		return;
 
 	meson_calc_reg_and_bit(bank, pin, REG_OUT, &reg, &bit);
@@ -484,12 +478,11 @@ static int meson_gpio_get(struct gpio_chip *chip, unsigned gpio)
 	struct meson_pinctrl *pc = gpiochip_get_data(chip);
 	unsigned int reg, bit, val, pin;
 	struct meson_bank *bank;
-	int ret;
 
 	pin = pc->data->pin_base + gpio;
-	ret = meson_get_bank(pc, pin, &bank);
-	if (ret)
-		return ret;
+	bank = meson_pinctrl_get_bank(pc, pin);
+	if (IS_ERR(bank))
+		return PTR_ERR(bank);
 
 	meson_calc_reg_and_bit(bank, pin, REG_IN, &reg, &bit);
 	regmap_read(pc->reg_gpio, reg, &val);
diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
index 890f296f..40b56aff 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.h
+++ b/drivers/pinctrl/meson/pinctrl-meson.h
@@ -176,3 +176,6 @@ 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;
+
+struct meson_bank *meson_pinctrl_get_bank(const struct meson_pinctrl *pc,
+					  unsigned int pin);
-- 
2.12.2

[PATCH v2 4/6] pintrl: meson: add DT node for GPIO IRQ on Meson 8 / 8b

[Heiner Kallweit]

Add the DT node for GPIO IRQ support on AMlogic Meson 8 and 8b.

Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
v2:
- remove syscon
---
 arch/arm/boot/dts/meson8.dtsi  | 13 +++++++++++++
 arch/arm/boot/dts/meson8b.dtsi | 13 +++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
index ebc763ea..1962e16e 100644
--- a/arch/arm/boot/dts/meson8.dtsi
+++ b/arch/arm/boot/dts/meson8.dtsi
@@ -91,6 +91,19 @@
 		clock-frequency = <141666666>;
 	};
 
+	gpio_irq@c1109880 {
+		compatible = "amlogic,meson-gpio-interrupt";
+		reg = <0xc1109880 0x10>;
+		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>;
+	};
+
 	pinctrl_cbus: pinctrl@c1109880 {
 		compatible = "amlogic,meson8-cbus-pinctrl";
 		reg = <0xc1109880 0x10>;
diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index 828aa49c..e48017be 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -183,6 +183,19 @@
 			status = "disabled";
 		};
 
+		gpio_irq@c1109880 {
+			compatible = "amlogic,meson-gpio-interrupt";
+			reg = <0xc1109880 0x10>;
+			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>;
+		};
+
 		pinctrl_cbus: pinctrl@c1109880 {
 			compatible = "amlogic,meson8b-cbus-pinctrl";
 			reg = <0xc1109880 0x10>;
-- 
2.12.2


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

[PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Heiner Kallweit]

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 <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
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

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 37af5e30..f8f401a0 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -153,6 +153,7 @@ config PINCTRL_MESON
 	select PINCONF
 	select GENERIC_PINCONF
 	select GPIOLIB
+	select GPIOLIB_IRQCHIP
 	select OF_GPIO
 	select REGMAP_MMIO
 
diff --git a/drivers/pinctrl/meson/Makefile b/drivers/pinctrl/meson/Makefile
index 27c5b512..827e416d 100644
--- a/drivers/pinctrl/meson/Makefile
+++ b/drivers/pinctrl/meson/Makefile
@@ -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
diff --git a/drivers/pinctrl/meson/pinctrl-meson-irq.c b/drivers/pinctrl/meson/pinctrl-meson-irq.c
new file mode 100644
index 00000000..c5f403f3
--- /dev/null
+++ b/drivers/pinctrl/meson/pinctrl-meson-irq.c
@@ -0,0 +1,367 @@
+/*
+ * Amlogic Meson GPIO IRQ driver
+ *
+ * Copyright 2017 Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ * Based on a first version by Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License 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);
diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
index 39ad9861..c587f6f0 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.c
+++ b/drivers/pinctrl/meson/pinctrl-meson.c
@@ -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;
diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
index 40b56aff..16aab328 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.h
+++ b/drivers/pinctrl/meson/pinctrl-meson.h
@@ -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);
-- 
2.12.2


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

Re: [PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Neil Armstrong]

On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
> 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 <hkallweit1@gmail.com>
> ---
> 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.
>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

>  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
> 
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 37af5e30..f8f401a0 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -153,6 +153,7 @@ config PINCTRL_MESON
>  	select PINCONF
>  	select GENERIC_PINCONF
>  	select GPIOLIB
> +	select GPIOLIB_IRQCHIP
>  	select OF_GPIO
>  	select REGMAP_MMIO
>  
> diff --git a/drivers/pinctrl/meson/Makefile b/drivers/pinctrl/meson/Makefile
> index 27c5b512..827e416d 100644
> --- a/drivers/pinctrl/meson/Makefile
> +++ b/drivers/pinctrl/meson/Makefile
> @@ -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
> diff --git a/drivers/pinctrl/meson/pinctrl-meson-irq.c b/drivers/pinctrl/meson/pinctrl-meson-irq.c
> new file mode 100644
> index 00000000..c5f403f3
> --- /dev/null
> +++ b/drivers/pinctrl/meson/pinctrl-meson-irq.c
> @@ -0,0 +1,367 @@
> +/*
> + * Amlogic Meson GPIO IRQ driver
> + *
> + * Copyright 2017 Heiner Kallweit <hkallweit1@gmail.com>
> + * Based on a first version by Jerome Brunet <jbrunet@baylibre.com>
> + *
> + * 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);
> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
> index 39ad9861..c587f6f0 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson.c
> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
> @@ -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;
> diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
> index 40b56aff..16aab328 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson.h
> +++ b/drivers/pinctrl/meson/pinctrl-meson.h
> @@ -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);
> 

Re: [PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Heiner Kallweit]

Am 15.05.2017 um 10:05 schrieb Neil Armstrong:
> On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
>> 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 <hkallweit1@gmail.com>
>> ---
>> 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.
> 
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.

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

> 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

> Neil
> 
>>  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
>>
>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>> index 37af5e30..f8f401a0 100644
>> --- a/drivers/pinctrl/Kconfig
>> +++ b/drivers/pinctrl/Kconfig
>> @@ -153,6 +153,7 @@ config PINCTRL_MESON
>>  	select PINCONF
>>  	select GENERIC_PINCONF
>>  	select GPIOLIB
>> +	select GPIOLIB_IRQCHIP
>>  	select OF_GPIO
>>  	select REGMAP_MMIO
>>  
>> diff --git a/drivers/pinctrl/meson/Makefile b/drivers/pinctrl/meson/Makefile
>> index 27c5b512..827e416d 100644
>> --- a/drivers/pinctrl/meson/Makefile
>> +++ b/drivers/pinctrl/meson/Makefile
>> @@ -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
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson-irq.c b/drivers/pinctrl/meson/pinctrl-meson-irq.c
>> new file mode 100644
>> index 00000000..c5f403f3
>> --- /dev/null
>> +++ b/drivers/pinctrl/meson/pinctrl-meson-irq.c
>> @@ -0,0 +1,367 @@
>> +/*
>> + * Amlogic Meson GPIO IRQ driver
>> + *
>> + * Copyright 2017 Heiner Kallweit <hkallweit1@gmail.com>
>> + * Based on a first version by Jerome Brunet <jbrunet@baylibre.com>
>> + *
>> + * 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);
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
>> index 39ad9861..c587f6f0 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson.c
>> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
>> @@ -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;
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
>> index 40b56aff..16aab328 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson.h
>> +++ b/drivers/pinctrl/meson/pinctrl-meson.h
>> @@ -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);
>>
> 
> 

Re: [PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Neil Armstrong]

Hi Heiner,

On 05/15/2017 09:00 PM, Heiner Kallweit wrote:
> Am 15.05.2017 um 10:05 schrieb Neil Armstrong:
>> On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
>>> 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 <hkallweit1@gmail.com>
>>> ---
>>> 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.
>>
> 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.

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

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

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

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

> Regards, Heiner
> 
>> Neil

Neil

[...]

Re: [PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Heiner Kallweit]

Am 16.05.2017 um 09:54 schrieb Neil Armstrong:
> Hi Heiner,
> 
> On 05/15/2017 09:00 PM, Heiner Kallweit wrote:
>> Am 15.05.2017 um 10:05 schrieb Neil Armstrong:
>>> On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
>>>> 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 <hkallweit1@gmail.com>
>>>> ---
>>>> 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.
>>>
>> 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.
> 
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

>>
>> 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.
> 
>>
>> 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".
> 
>>
>>> 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.
>>> >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 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,
> 
>>
>>> 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 !
> 
>> Regards, Heiner
>>
>>> Neil
> 
> Neil
> 
> [...]
> 

Re: [PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Jerome Brunet]

On Tue, 2017-05-16 at 20:31 +0200, Heiner Kallweit wrote:
> Am 16.05.2017 um 09:54 schrieb Neil Armstrong:
> > Hi Heiner,
> > 
> > On 05/15/2017 09:00 PM, Heiner Kallweit wrote:
> > > Am 15.05.2017 um 10:05 schrieb Neil Armstrong:
> > > > On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
> > > > > 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 <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > > ---
> > > > > 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.
> > > > 
> > > 
> > > 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.
> > 
> 
> 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.

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

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

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

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)

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

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


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

> 
> 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
> 
> > > 
> > > 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.
> > 
> > > 
> > > 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".
> > 
> > > 
> > > > 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.
> > > > > 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 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,
> > 
> > > 
> > > > 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 !
> > 
> > > Regards, Heiner
> > > 
> > > > Neil
> > 
> > Neil
> > 
> > [...]
> > 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Heiner Kallweit]

Am 17.05.2017 um 01:16 schrieb Jerome Brunet:
> On Tue, 2017-05-16 at 20:31 +0200, Heiner Kallweit wrote:
>> Am 16.05.2017 um 09:54 schrieb Neil Armstrong:
>>> Hi Heiner,
>>>
>>> On 05/15/2017 09:00 PM, Heiner Kallweit wrote:
>>>> Am 15.05.2017 um 10:05 schrieb Neil Armstrong:
>>>>> On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
>>>>>> 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 <hkallweit1@gmail.com>
>>>>>> ---
>>>>>> 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.
>>>>>
>>>>
>>>> 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.
>>>
>>
>> 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.
> 
>>
>> 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/
> 
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.

>>    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.
> 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.
> 
>>
>> 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>;
>>
> 
> 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)
> 
>> 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.
> 
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.

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

> 
>> 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. 
> 
>>
>> 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
>>
>>>>
>>>> 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.
>>>
>>>>
>>>> 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".
>>>
>>>>
>>>>> 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.
>>>>>> 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 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,
>>>
>>>>
>>>>> 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 !
>>>
>>>> Regards, Heiner
>>>>
>>>>> Neil
>>>
>>> Neil
>>>
>>> [...]
>>>
>>
>>
> 

Re: [PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Jerome Brunet]

On Wed, 2017-05-17 at 22:36 +0200, Heiner Kallweit wrote:
> Am 17.05.2017 um 01:16 schrieb Jerome Brunet:
> > On Tue, 2017-05-16 at 20:31 +0200, Heiner Kallweit wrote:

[snip]

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

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



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

Re: [PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Heiner Kallweit]

Am 23.05.2017 um 10:38 schrieb Jerome Brunet:
> On Wed, 2017-05-17 at 22:36 +0200, Heiner Kallweit wrote:
>> Am 17.05.2017 um 01:16 schrieb Jerome Brunet:
>>> On Tue, 2017-05-16 at 20:31 +0200, Heiner Kallweit wrote:
> 
> [snip]
> 
>>>
>>> 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/
>>>
>>
>> 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.
> 
gio_to_irq is provided by the GPIOLIB_IRQCHIP core. So fortunately we don't
have to take care.

> [snip]
> 
>>>> 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.
>>>
>>
>> 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. 
> 
Sure, it's a SW workaround. However working on a solution not supporting
IRQ_TYPE_EDGE_BOTH I'd consider wasted energy (YMMV).
Waiting for a future irq framework extension most likely is no solution as we
need a solution before production of Amlogic Meson chips ends ..

I will come up with a new version of the patch set addressing (most of) the
review comments. Then we have a updated basis for further discussion.

Thanks for the review / remarks.

> [snip]
>>
>>>> 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.
>>>
>>
>> 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
> 
> 
> 
> 

Re: [PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Jerome Brunet]

On Fri, 2017-05-12 at 21:14 +0200, Heiner Kallweit wrote:
> 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 <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> 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
> 
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 37af5e30..f8f401a0 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -153,6 +153,7 @@ config PINCTRL_MESON
>  	select PINCONF
>  	select GENERIC_PINCONF
>  	select GPIOLIB
> +	select GPIOLIB_IRQCHIP
>  	select OF_GPIO
>  	select REGMAP_MMIO
>  
> diff --git a/drivers/pinctrl/meson/Makefile b/drivers/pinctrl/meson/Makefile
> index 27c5b512..827e416d 100644
> --- a/drivers/pinctrl/meson/Makefile
> +++ b/drivers/pinctrl/meson/Makefile
> @@ -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
> diff --git a/drivers/pinctrl/meson/pinctrl-meson-irq.c
> b/drivers/pinctrl/meson/pinctrl-meson-irq.c
> new file mode 100644
> index 00000000..c5f403f3
> --- /dev/null
> +++ b/drivers/pinctrl/meson/pinctrl-meson-irq.c
> @@ -0,0 +1,367 @@
> +/*
> + * Amlogic Meson GPIO IRQ driver
> + *
> + * Copyright 2017 Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + * Based on a first version by Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License 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).
> +	 */

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.

> +	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);
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 ?

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


> +		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);
> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c
> b/drivers/pinctrl/meson/pinctrl-meson.c
> index 39ad9861..c587f6f0 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson.c
> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
> @@ -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;
> diff --git a/drivers/pinctrl/meson/pinctrl-meson.h
> b/drivers/pinctrl/meson/pinctrl-meson.h
> index 40b56aff..16aab328 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson.h
> +++ b/drivers/pinctrl/meson/pinctrl-meson.h
> @@ -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);
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Heiner Kallweit]

Am 17.05.2017 um 01:07 schrieb Jerome Brunet:
> On Fri, 2017-05-12 at 21:14 +0200, Heiner Kallweit wrote:
>> 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 <hkallweit1@gmail.com>
>> ---
>> 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
>>
>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>> index 37af5e30..f8f401a0 100644
>> --- a/drivers/pinctrl/Kconfig
>> +++ b/drivers/pinctrl/Kconfig
>> @@ -153,6 +153,7 @@ config PINCTRL_MESON
>>  	select PINCONF
>>  	select GENERIC_PINCONF
>>  	select GPIOLIB
>> +	select GPIOLIB_IRQCHIP
>>  	select OF_GPIO
>>  	select REGMAP_MMIO
>>  
>> diff --git a/drivers/pinctrl/meson/Makefile b/drivers/pinctrl/meson/Makefile
>> index 27c5b512..827e416d 100644
>> --- a/drivers/pinctrl/meson/Makefile
>> +++ b/drivers/pinctrl/meson/Makefile
>> @@ -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
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson-irq.c
>> b/drivers/pinctrl/meson/pinctrl-meson-irq.c
>> new file mode 100644
>> index 00000000..c5f403f3
>> --- /dev/null
>> +++ b/drivers/pinctrl/meson/pinctrl-meson-irq.c
>> @@ -0,0 +1,367 @@
>> +/*
>> + * Amlogic Meson GPIO IRQ driver
>> + *
>> + * Copyright 2017 Heiner Kallweit <hkallweit1@gmail.com>
>> + * Based on a first version by Jerome Brunet <jbrunet@baylibre.com>
>> + *
>> + * 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).
>> +	 */
> 
> 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.
> 
In v3 of the patch series I switched from request_irq to chained irq handling.
This also fixed the spurious irq issue and the hacky workaround code was removed.

>> +	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);
> 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 ?
> 
> 
>> +		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);
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c
>> b/drivers/pinctrl/meson/pinctrl-meson.c
>> index 39ad9861..c587f6f0 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson.c
>> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
>> @@ -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;
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.h
>> b/drivers/pinctrl/meson/pinctrl-meson.h
>> index 40b56aff..16aab328 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson.h
>> +++ b/drivers/pinctrl/meson/pinctrl-meson.h
>> @@ -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);
> 

Re: [PATCH v2 0/6] pintrl: meson: add support for GPIO IRQs

[Andreas Färber]

Hi Heiner,

Am 12.05.2017 um 21:01 schrieb Heiner Kallweit:
> Heiner Kallweit (5):
>   pinctrl: meson: add interrupts to pinctrl data
>   pinctrl: meson: document GPIO IRQ DT binding
>   pinctrl: meson: add DT node for GPIO IRQ on Meson GX
>   pinctrl: meson: add DT node for GPIO IRQ on Meson 8 / 8b
>   pinctrl: meson: improve meson_get_bank and export it
>   pinctrl: meson: add support for GPIO interrupts

For some odd reason, all subjects of your actual patches arrived without
the c in pinctrl, but you have it spelled correctly above...

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)