[PATCH v6 0/9] pinctrl: meson: add support for GPIO IRQs

[PATCH v6 0/9] pinctrl: 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.

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

Changes in v3:
- replace the request_irq based allocation of parent irq's with
  chained irq handling, this also fixes the spurious interrupts issue
  and allows to remove the workaround code.
  Last but not least the parent irq's are no longer visible in
  /proc/interrupts.
- minor improvements to the GPIO IRQ controller

Changes in v4:
- separate the gpio-independent interrupt controller part and make
  it a driver under drivers/irqchip

Changes in v5:
- smaller changes based on review comments
- split DT patches per ARM / ARM64

Changes in v6:
- based on suggestion by Rob Herring: rename DT property parent-interrupts to interrupts

Heiner Kallweit (7):
  pinctrl: meson: add interrupts to pinctrl data
  irqchip: add Amlogic Meson GPIO irqchip driver
  dt-bindings: add Amlogic Meson GPIO interrupt-controller DT binding documentation
  ARM: dts: meson: add GPIO interrupt-controller support
  ARM64: dts: meson: add GPIO interrupt-controller support
  pinctrl: meson: add support for GPIO interrupts
  pinctrl: meson: update DT binding documentation
  ARM: dts: meson: mark gpio controllers as interrupt controllers
  ARM64: dts: meson: mark gpio controllers as interrupt controllers

.../amlogic,meson-gpio-intc.txt                    |  26 ++
 .../devicetree/bindings/pinctrl/meson,pinctrl.txt  |   4 +
 arch/arm/boot/dts/meson8.dtsi                      |  12 +
 arch/arm/boot/dts/meson8b.dtsi                     |  12 +
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi          |   8 +
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |   4 +
 arch/arm64/boot/dts/amlogic/meson-gxl.dtsi         |   4 +
 drivers/irqchip/Kconfig                            |   5 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-meson-gpio.c                   | 296 +++++++++++++++++++++
 drivers/pinctrl/Kconfig                            |   1 +
 drivers/pinctrl/meson/pinctrl-meson-gxbb.c         |  22 +-
 drivers/pinctrl/meson/pinctrl-meson-gxl.c          |  20 +-
 drivers/pinctrl/meson/pinctrl-meson.c              | 170 +++++++++++-
 drivers/pinctrl/meson/pinctrl-meson.h              |  15 +-
 drivers/pinctrl/meson/pinctrl-meson8.c             |  20 +-
 drivers/pinctrl/meson/pinctrl-meson8b.c            |  32 ++-
 17 files changed, 605 insertions(+), 47 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

-- 
2.13.0

[PATCH v6 1/9] pinctrl: 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
v3:
- no changes
v4:
- no changes
v5:
- added Acked-by
v6:
- no changes

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
---
 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 8b716b93..6999c7a9 100644
--- a/drivers/pinctrl/meson/pinctrl-meson-gxbb.c
+++ b/drivers/pinctrl/meson/pinctrl-meson-gxbb.c
@@ -807,20 +807,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 190f50c6..2abea6e1 100644
--- a/drivers/pinctrl/meson/pinctrl-meson-gxl.c
+++ b/drivers/pinctrl/meson/pinctrl-meson-gxl.c
@@ -764,19 +764,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 e1bdf1f3..970f6f14 100644
--- a/drivers/pinctrl/meson/pinctrl-meson8.c
+++ b/drivers/pinctrl/meson/pinctrl-meson8.c
@@ -1041,19 +1041,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.13.0

Re: [PATCH v6 1/9] pinctrl: meson: add interrupts to pinctrl data

[Linus Walleij]

On Thu, Jun 8, 2017 at 9:37 PM, Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> From: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> 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-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> v2:
> - no changes
> v3:
> - no changes
> v4:
> - no changes
> v5:
> - added Acked-by
> v6:
> - no changes
>
> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

I've applied this patch to the pin control tree, as there is nothing
controversial at all about it, just encoding hardware information into
the tables as is custom.

I don't know about how to handle the rest of the series, at least
you have one less patch in your patch stack.

Yours,
Linus Walleij
--
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 v6 2/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Heiner Kallweit]

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

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used
v6:
- change DT property parent-interrupts to interrupts
---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 302 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 478f8ace..bdc86e14 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b64c59b8..1be482bd 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o
diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c
new file mode 100644
index 00000000..b09228ea
--- /dev/null
+++ b/drivers/irqchip/irq-meson-gpio.c
@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <carlo@endlessm.com>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <jbrunet@baylibre.com>
+ * Copyright (c) 2017 Heiner Kallweit <hkallweit1@gmail.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/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define REG_EDGE_POL		0x00
+#define REG_PIN_03_SEL		0x04
+#define REG_PIN_47_SEL		0x08
+#define REG_FILTER_SEL		0x0c
+
+#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
+#define REG_EDGE_POL_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
+
+#define MAX_PARENT_IRQ_NUM	8
+
+/* maximum number of GPIO IRQs on supported platforms */
+#define MAX_NUM_GPIO_IRQ	133
+
+/*
+ * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
+ * edge. That's due to HW constraints.
+ * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
+ * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
+ * one bit to the right.
+ */
+#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)
+
+struct meson_irq_slot {
+	unsigned int irq;
+	unsigned int owner;
+};
+
+struct meson_data {
+	struct regmap *regmap;
+	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
+	unsigned int num_slots;
+	struct mutex lock;
+};
+
+static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -ENOSPC;
+
+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (!md->slots[i].owner) {
+			md->slots[i].owner = virq;
+			slot = i;
+		}
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}
+
+static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i;
+
+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots; i++)
+		if (md->slots[i].owner == virq) {
+			md->slots[i].owner = 0;
+			break;
+		}
+
+	mutex_unlock(&md->lock);
+}
+
+static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -EINVAL;
+
+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (md->slots[i].owner == virq)
+			slot = i;
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}
+
+static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int hwirq)
+{
+	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
+	int shift = 8 * (idx % 4);
+
+	regmap_update_bits(md->regmap, reg, 0xff << shift,
+			   hwirq << shift);
+}
+
+static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot = meson_find_irq_slot(md, data->irq);
+
+	if (slot >= 0)
+		meson_set_hwirq(md, slot, hwirq);
+}
+
+static int meson_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot;
+	unsigned int val = 0;
+
+	if (type == IRQ_TYPE_EDGE_BOTH)
+		return -EINVAL;
+
+	slot = meson_find_irq_slot(md, data->irq);
+	if (slot < 0)
+		return slot;
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val |= REG_EDGE_POL_EDGE(slot);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_LOW(slot);
+
+	regmap_update_bits(md->regmap, REG_EDGE_POL,
+			   REG_EDGE_POL_MASK(slot), val);
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val = IRQ_TYPE_EDGE_RISING;
+	else
+		val = IRQ_TYPE_LEVEL_HIGH;
+
+	return irq_chip_set_type_parent(data, val);
+}
+
+static unsigned int meson_irq_startup(struct irq_data *data)
+{
+	irq_chip_unmask_parent(data);
+	/*
+	 * An extra bit was added to allow having the same gpio hwirq twice
+	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
+	 * gpio hwirq.
+	 */
+	meson_irq_set_hwirq(data, data->hwirq >> 1);
+
+	return 0;
+}
+
+static void meson_irq_shutdown(struct irq_data *data)
+{
+	meson_irq_set_hwirq(data, 0xff);
+	irq_chip_mask_parent(data);
+}
+
+static struct irq_chip meson_irq_chip = {
+	.name = "meson_gpio_intc",
+	.irq_set_type = meson_irq_set_type,
+	.irq_eoi = irq_chip_eoi_parent,
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_startup = meson_irq_startup,
+	.irq_shutdown = meson_irq_shutdown,
+	.irq_set_affinity = irq_chip_set_affinity_parent,
+};
+
+static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct meson_data *md = d->host_data;
+	irq_hw_number_t hwirq;
+	int ret, slot;
+
+	slot = meson_alloc_irq_slot(md, virq);
+	if (slot < 0)
+		return slot;
+
+	hwirq = fwspec->param[0];
+	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
+
+	parent_fwspec.fwnode = d->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0] = 0; /* SPI */
+	parent_fwspec.param[1] = md->slots[slot].irq;
+	parent_fwspec.param[2] = IRQ_TYPE_NONE;
+
+	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
+	if (ret)
+		meson_free_irq_slot(md, virq);
+
+	return ret;
+}
+
+static void meson_irq_free(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs)
+{
+	struct meson_data *md = d->host_data;
+
+	irq_domain_free_irqs_common(d, virq, nr_irqs);
+	meson_free_irq_slot(md, virq);
+}
+
+static const struct irq_domain_ops meson_irq_ops = {
+	.alloc = meson_irq_alloc,
+	.free = meson_irq_free,
+	.xlate = irq_domain_xlate_twocell,
+};
+
+static int meson_get_irqs(struct meson_data *md, struct device_node *node)
+{
+	int ret, i;
+	u32 irq;
+
+	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
+		ret = of_property_read_u32_index(node, "interrupts", i, &irq);
+		if (ret)
+			break;
+		md->slots[i].irq = irq;
+	}
+
+	md->num_slots = i;
+
+	return i ? 0 : -EINVAL;
+}
+
+static const struct regmap_config meson_regmap_config = {
+	.reg_bits       = 32,
+	.reg_stride     = 4,
+	.val_bits       = 32,
+	.max_register	= REG_FILTER_SEL,
+};
+
+static int __init meson_gpio_irq_init(struct device_node *node,
+				      struct device_node *parent)
+{
+	struct irq_domain *meson_irq_domain, *parent_domain;
+	struct meson_data *md;
+	void __iomem *io_base;
+	int ret;
+
+	md = kzalloc(sizeof(*md), GFP_KERNEL);
+	if (!md)
+		return -ENOMEM;
+
+	mutex_init(&md->lock);
+
+	io_base = of_iomap(node, 0);
+	if (!io_base)
+		return -EINVAL;
+
+	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
+	if (IS_ERR(md->regmap))
+		return PTR_ERR(md->regmap);
+
+	/* initialize to IRQ_TYPE_LEVEL_HIGH */
+	regmap_write(md->regmap, REG_EDGE_POL, 0);
+	/* disable all GPIO interrupt sources */
+	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
+	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
+	/* disable filtering */
+	regmap_write(md->regmap, REG_FILTER_SEL, 0);
+
+	ret = meson_get_irqs(md, node);
+	if (ret)
+		return ret;
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain)
+		return -ENXIO;
+
+	meson_irq_domain = irq_domain_add_hierarchy(parent_domain, 0,
+						    NUM_GPIO_HWIRQ, node,
+						    &meson_irq_ops, md);
+	return meson_irq_domain ? 0 : -EINVAL;
+}
+
+IRQCHIP_DECLARE(meson_gpio_irq, "amlogic,meson-gpio-intc", meson_gpio_irq_init);
-- 
2.13.0

Re: [PATCH v6 2/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Linus Walleij]

On Thu, Jun 8, 2017 at 9:38 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:

> Add a driver supporting the GPIO interrupt controller on certain
> Amlogic meson SoC's.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v6 2/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Jerome Brunet]

On Thu, 2017-06-08 at 21:38 +0200, Heiner Kallweit wrote:
> Add a driver supporting the GPIO interrupt controller on certain
> Amlogic meson SoC's.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> v5:
> - changed Kconfig entry based on Neil's suggestion
> - added authors
> - extended explanation why 2 * n hwirqs are used
> v6:
> - change DT property parent-interrupts to interrupts
> ---
>  drivers/irqchip/Kconfig          |   5 +
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-meson-gpio.c | 295
> +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 302 insertions(+)
>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 478f8ace..bdc86e14 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>  	help
>  	  Say yes here to add support for the IRQ combiner devices embedded
>  	  in Qualcomm Technologies chips.
> +
> +config MESON_GPIO_INTC
> +	bool
> +	depends on ARCH_MESON
> +	default y
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b64c59b8..1be482bd 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-
> eznps.o
>  obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
> +obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o
> diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-
> gpio.c
> new file mode 100644
> index 00000000..b09228ea
> --- /dev/null
> +++ b/drivers/irqchip/irq-meson-gpio.c
> @@ -0,0 +1,295 @@
> +/*
> + * Amlogic Meson GPIO IRQ chip driver
> + *
> + * Copyright (c) 2015 Endless Mobile, Inc.
> + * Author: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> + * Copyright (c) 2016 BayLibre, SAS.
> + * Author: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@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/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqchip.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +
> +#define REG_EDGE_POL		0x00
> +#define REG_PIN_03_SEL		0x04
> +#define REG_PIN_47_SEL		0x08
> +#define REG_FILTER_SEL		0x0c
> +
> +#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
> +#define REG_EDGE_POL_EDGE(x)	BIT(x)
> +#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
> +
> +#define MAX_PARENT_IRQ_NUM	8
> +
> +/* maximum number of GPIO IRQs on supported platforms */
> +#define MAX_NUM_GPIO_IRQ	133

Only valid on gxbb. Not gxl nor meson8

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

PSB

> +
> +struct meson_irq_slot {
> +	unsigned int irq;
> +	unsigned int owner;
> +};
> +
> +struct meson_data {
> +	struct regmap *regmap;
> +	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
> +	unsigned int num_slots;
> +	struct mutex lock;
> +};
> +
> +static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
> +{
> +	int i, slot = -ENOSPC;
> +
> +	mutex_lock(&md->lock);
> +
> +	for (i = 0; i < md->num_slots && slot < 0; i++)
> +		if (!md->slots[i].owner) {
> +			md->slots[i].owner = virq;
> +			slot = i;
> +		}
> +
> +	mutex_unlock(&md->lock);
> +
> +	return slot;
> +}
> +
> +static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
> +{
> +	int i;
> +
> +	mutex_lock(&md->lock);
> +
> +	for (i = 0; i < md->num_slots; i++)
> +		if (md->slots[i].owner == virq) {
> +			md->slots[i].owner = 0;
> +			break;
> +		}
> +
> +	mutex_unlock(&md->lock);
> +}
> +
> +static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
> +{
> +	int i, slot = -EINVAL;
> +
> +	mutex_lock(&md->lock);
> +
> +	for (i = 0; i < md->num_slots && slot < 0; i++)
> +		if (md->slots[i].owner == virq)
> +			slot = i;
> +
> +	mutex_unlock(&md->lock);
> +
> +	return slot;
> +}
> +
> +static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int
> hwirq)
> +{
> +	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
> +	int shift = 8 * (idx % 4);
> +
> +	regmap_update_bits(md->regmap, reg, 0xff << shift,
> +			   hwirq << shift);

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

> +}
> +
> +static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
> +{
> +	struct meson_data *md = data->domain->host_data;
> +	int slot = meson_find_irq_slot(md, data->irq);
> +
> +	if (slot >= 0)
> +		meson_set_hwirq(md, slot, hwirq);
> +}
> +
> +static int meson_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> +	struct meson_data *md = data->domain->host_data;
> +	int slot;
> +	unsigned int val = 0;
> +
> +	if (type == IRQ_TYPE_EDGE_BOTH)
> +		return -EINVAL;
> +
> +	slot = meson_find_irq_slot(md, data->irq);
> +	if (slot < 0)
> +		return slot;
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		val |= REG_EDGE_POL_EDGE(slot);
> +
> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
> +		val |= REG_EDGE_POL_LOW(slot);
> +
> +	regmap_update_bits(md->regmap, REG_EDGE_POL,
> +			   REG_EDGE_POL_MASK(slot), val);

Same here.


> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		val = IRQ_TYPE_EDGE_RISING;
> +	else
> +		val = IRQ_TYPE_LEVEL_HIGH;
> +
> +	return irq_chip_set_type_parent(data, val);
> +}
> +
> +static unsigned int meson_irq_startup(struct irq_data *data)
> +{
> +	irq_chip_unmask_parent(data);
> +	/*
> +	 * An extra bit was added to allow having the same gpio hwirq twice
> +	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
> +	 * gpio hwirq.
> +	 */
> +	meson_irq_set_hwirq(data, data->hwirq >> 1);

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

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

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

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

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

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

--
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 v6 2/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Heiner Kallweit]

Am 09.06.2017 um 11:07 schrieb Jerome Brunet:
> On Thu, 2017-06-08 at 21:38 +0200, Heiner Kallweit wrote:
>> Add a driver supporting the GPIO interrupt controller on certain
>> Amlogic meson SoC's.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> v5:
>> - changed Kconfig entry based on Neil's suggestion
>> - added authors
>> - extended explanation why 2 * n hwirqs are used
>> v6:
>> - change DT property parent-interrupts to interrupts
>> ---
>>  drivers/irqchip/Kconfig          |   5 +
>>  drivers/irqchip/Makefile         |   1 +
>>  drivers/irqchip/irq-meson-gpio.c | 295
>> +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 302 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 478f8ace..bdc86e14 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>>  	help
>>  	  Say yes here to add support for the IRQ combiner devices embedded
>>  	  in Qualcomm Technologies chips.
>> +
>> +config MESON_GPIO_INTC
>> +	bool
>> +	depends on ARCH_MESON
>> +	default y
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index b64c59b8..1be482bd 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-
>> eznps.o
>>  obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
>>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>> +obj-$(CONFIG_MESON_GPIO_INTC)		+= irq-meson-gpio.o
>> diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-
>> gpio.c
>> new file mode 100644
>> index 00000000..b09228ea
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-meson-gpio.c
>> @@ -0,0 +1,295 @@
>> +/*
>> + * Amlogic Meson GPIO IRQ chip driver
>> + *
>> + * Copyright (c) 2015 Endless Mobile, Inc.
>> + * Author: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>> + * Copyright (c) 2016 BayLibre, SAS.
>> + * Author: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@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/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/regmap.h>
>> +
>> +#define REG_EDGE_POL		0x00
>> +#define REG_PIN_03_SEL		0x04
>> +#define REG_PIN_47_SEL		0x08
>> +#define REG_FILTER_SEL		0x0c
>> +
>> +#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
>> +#define REG_EDGE_POL_EDGE(x)	BIT(x)
>> +#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
>> +
>> +#define MAX_PARENT_IRQ_NUM	8
>> +
>> +/* maximum number of GPIO IRQs on supported platforms */
>> +#define MAX_NUM_GPIO_IRQ	133
> 
> Only valid on gxbb. Not gxl nor meson8
> 
This max value is meant to be the max for all supported chips.
Meson8 and GXL have less gpio irq sources.

>> +
>> +/*
>> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
>> + * edge. That's due to HW constraints.
>> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
>> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
>> + * one bit to the right.
>> + */
>> +#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)
> 
> PSB
> 
>> +
>> +struct meson_irq_slot {
>> +	unsigned int irq;
>> +	unsigned int owner;
>> +};
>> +
>> +struct meson_data {
>> +	struct regmap *regmap;
>> +	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
>> +	unsigned int num_slots;
>> +	struct mutex lock;
>> +};
>> +
>> +static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
>> +{
>> +	int i, slot = -ENOSPC;
>> +
>> +	mutex_lock(&md->lock);
>> +
>> +	for (i = 0; i < md->num_slots && slot < 0; i++)
>> +		if (!md->slots[i].owner) {
>> +			md->slots[i].owner = virq;
>> +			slot = i;
>> +		}
>> +
>> +	mutex_unlock(&md->lock);
>> +
>> +	return slot;
>> +}
>> +
>> +static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
>> +{
>> +	int i;
>> +
>> +	mutex_lock(&md->lock);
>> +
>> +	for (i = 0; i < md->num_slots; i++)
>> +		if (md->slots[i].owner == virq) {
>> +			md->slots[i].owner = 0;
>> +			break;
>> +		}
>> +
>> +	mutex_unlock(&md->lock);
>> +}
>> +
>> +static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
>> +{
>> +	int i, slot = -EINVAL;
>> +
>> +	mutex_lock(&md->lock);
>> +
>> +	for (i = 0; i < md->num_slots && slot < 0; i++)
>> +		if (md->slots[i].owner == virq)
>> +			slot = i;
>> +
>> +	mutex_unlock(&md->lock);
>> +
>> +	return slot;
>> +}
>> +
>> +static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int
>> hwirq)
>> +{
>> +	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
>> +	int shift = 8 * (idx % 4);
>> +
>> +	regmap_update_bits(md->regmap, reg, 0xff << shift,
>> +			   hwirq << shift);
> 
> Access to the registers should be somehow protected. Several irqs use the same
> registers. There is a potential race condition here.
> 
regmap comes with an integrated locking.

>> +}
>> +
>> +static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
>> +{
>> +	struct meson_data *md = data->domain->host_data;
>> +	int slot = meson_find_irq_slot(md, data->irq);
>> +
>> +	if (slot >= 0)
>> +		meson_set_hwirq(md, slot, hwirq);
>> +}
>> +
>> +static int meson_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> +	struct meson_data *md = data->domain->host_data;
>> +	int slot;
>> +	unsigned int val = 0;
>> +
>> +	if (type == IRQ_TYPE_EDGE_BOTH)
>> +		return -EINVAL;
>> +
>> +	slot = meson_find_irq_slot(md, data->irq);
>> +	if (slot < 0)
>> +		return slot;
>> +
>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>> +		val |= REG_EDGE_POL_EDGE(slot);
>> +
>> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
>> +		val |= REG_EDGE_POL_LOW(slot);
>> +
>> +	regmap_update_bits(md->regmap, REG_EDGE_POL,
>> +			   REG_EDGE_POL_MASK(slot), val);
> 
> Same here.
> 
> 
>> +
>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>> +		val = IRQ_TYPE_EDGE_RISING;
>> +	else
>> +		val = IRQ_TYPE_LEVEL_HIGH;
>> +
>> +	return irq_chip_set_type_parent(data, val);
>> +}
>> +
>> +static unsigned int meson_irq_startup(struct irq_data *data)
>> +{
>> +	irq_chip_unmask_parent(data);
>> +	/*
>> +	 * An extra bit was added to allow having the same gpio hwirq twice
>> +	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
>> +	 * gpio hwirq.
>> +	 */
>> +	meson_irq_set_hwirq(data, data->hwirq >> 1);
> 
> Please keep in mind that any device can use this controller as irq parent.
> It has to make sense, even when not serving the gpio driver.
> 
Yes, theoretically there could be other drivers accessing this
gpio irq controller. But most likely they would face the same issue too
(support for IRQ_TYPE_EDGE_BOTH) and is there any realistic foreseeable
use case where another driver would access the gpio irq controller?

For me the question is:
Do we want to go with something that is 80% nice, supports all major
use cases and is available now (it can still be improved later)?
Or do we want to go on waiting (like since Oct 2016) for something that
suddenly steps out of the dark, is 100% nice and covers all corner cases?

I'm also a big fan of proper architecture. But I'm also somewhat
result-driven. W/o this I'm afraid I would have lost my job long ago.

Last but not least I think what we have here is still much better
than most vendor drivers. And v6 of the patch set already includes major
changes based on the various review comments.

> This hack means that, in DT, we'd have to multiply by 2 the values given under
> section "22.3 GPIO interrupts" of the datasheet. This is an example Linux
> specific stuff in DT.
> It also means that the controller declares a lot more lines that it really has
> ... 
> 
> This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the
> mapping from the irq_set_type callback of the GPIO driver, which is still think
> should be dropped at this point.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static void meson_irq_shutdown(struct irq_data *data)
>> +{
>> +	meson_irq_set_hwirq(data, 0xff);
>> +	irq_chip_mask_parent(data);
>> +}
>> +
>> +static struct irq_chip meson_irq_chip = {
>> +	.name = "meson_gpio_intc",
>> +	.irq_set_type = meson_irq_set_type,
>> +	.irq_eoi = irq_chip_eoi_parent,
>> +	.irq_mask = irq_chip_mask_parent,
>> +	.irq_unmask = irq_chip_unmask_parent,
>> +	.irq_startup = meson_irq_startup,
>> +	.irq_shutdown = meson_irq_shutdown,
>> +	.irq_set_affinity = irq_chip_set_affinity_parent,
>> +};
>> +
>> +static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
>> +			   unsigned int nr_irqs, void *data)
>> +{
>> +	struct irq_fwspec parent_fwspec, *fwspec = data;
>> +	struct meson_data *md = d->host_data;
>> +	irq_hw_number_t hwirq;
>> +	int ret, slot;
>> +
>> +	slot = meson_alloc_irq_slot(md, virq);
>> +	if (slot < 0)
>> +		return slot;
>> +
>> +	hwirq = fwspec->param[0];
>> +	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
>> +
>> +	parent_fwspec.fwnode = d->parent->fwnode;
>> +	parent_fwspec.param_count = 3;
>> +	parent_fwspec.param[0] = 0; /* SPI */
>> +	parent_fwspec.param[1] = md->slots[slot].irq;
>> +	parent_fwspec.param[2] = IRQ_TYPE_NONE;
>> +
>> +	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
>> +	if (ret)
>> +		meson_free_irq_slot(md, virq);
>> +
>> +	return ret;
>> +}
>> +
>> +static void meson_irq_free(struct irq_domain *d, unsigned int virq,
>> +			   unsigned int nr_irqs)
>> +{
>> +	struct meson_data *md = d->host_data;
>> +
>> +	irq_domain_free_irqs_common(d, virq, nr_irqs);
>> +	meson_free_irq_slot(md, virq);
>> +}
>> +
>> +static const struct irq_domain_ops meson_irq_ops = {
>> +	.alloc = meson_irq_alloc,
>> +	.free = meson_irq_free,
>> +	.xlate = irq_domain_xlate_twocell,
> 
> This is a hierarchic domain, shouldn't it implement translate rather than xlate
> ?
> 
If translate isn't defined there's a fallback to xlate. irq_domain_xlate_twocell
comes for free so I didn't see any benefit in re-implementing it as a translate
callback.

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

--
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 v6 2/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Kevin Hilman]

Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> writes:

> On Thu, 2017-06-08 at 21:38 +0200, Heiner Kallweit wrote:
>> Add a driver supporting the GPIO interrupt controller on certain
>> Amlogic meson SoC's.
>> 
>> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

[...]

>> +static unsigned int meson_irq_startup(struct irq_data *data)
>> +{
>> +	irq_chip_unmask_parent(data);
>> +	/*
>> +	 * An extra bit was added to allow having the same gpio hwirq twice
>> +	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
>> +	 * gpio hwirq.
>> +	 */
>> +	meson_irq_set_hwirq(data, data->hwirq >> 1);
>
> Please keep in mind that any device can use this controller as irq parent.
> It has to make sense, even when not serving the gpio driver.
>
> This hack means that, in DT, we'd have to multiply by 2 the values given under
> section "22.3 GPIO interrupts" of the datasheet. This is an example Linux
> specific stuff in DT.
> It also means that the controller declares a lot more lines that it really has
> ... 
>
> This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the
> mapping from the irq_set_type callback of the GPIO driver, which is still think
> should be dropped at this point.

+1

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

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

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

Thanks,

Kevin
--
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 v6 2/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Heiner Kallweit]

Am 09.06.2017 um 23:15 schrieb Kevin Hilman:
> Jerome Brunet <jbrunet@baylibre.com> writes:
> 
>> On Thu, 2017-06-08 at 21:38 +0200, Heiner Kallweit wrote:
>>> Add a driver supporting the GPIO interrupt controller on certain
>>> Amlogic meson SoC's.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> [...]
> 
>>> +static unsigned int meson_irq_startup(struct irq_data *data)
>>> +{
>>> +	irq_chip_unmask_parent(data);
>>> +	/*
>>> +	 * An extra bit was added to allow having the same gpio hwirq twice
>>> +	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
>>> +	 * gpio hwirq.
>>> +	 */
>>> +	meson_irq_set_hwirq(data, data->hwirq >> 1);
>>
>> Please keep in mind that any device can use this controller as irq parent.
>> It has to make sense, even when not serving the gpio driver.
>>
>> This hack means that, in DT, we'd have to multiply by 2 the values given under
>> section "22.3 GPIO interrupts" of the datasheet. This is an example Linux
>> specific stuff in DT.
>> It also means that the controller declares a lot more lines that it really has
>> ... 
>>
>> This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the
>> mapping from the irq_set_type callback of the GPIO driver, which is still think
>> should be dropped at this point.
> 
> +1
> 
> Please drop the hack for IRQ_TYPE_BOTH so we can reach agreement on the
> basic design and functionality.  The gymnastics required to support this
> hack (due to broken hardware) are getting in the way of getting basic
> functionality merged.
> 
I haven't heard any feedback on the other proposal yet:
Always reserve two parent irq's in the request_resources callback,
then set_type is easy. How about this one?

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

Rgds, Heiner

> Also, we already know from previous discussions between Jerome and the
> IRQ maintainers that hacks for IRQ_TYPE_BOTH like this have been very
> thoroughly rejected.  So while the IRQ maintainers haven't chimed in on
> this series, we have a very good idea what they will say when they do.
> 
> So please, pretty please, let's avoid giving the IRQ maintainers a fun
> reason to NAK, and drop the IRQ_TYPE_BOTH stuff until we have an agreed
> upon way to support the hardware that actually works.
> 
> Thanks,
> 
> Kevin
> 

Re: [PATCH v6 2/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Kevin Hilman]

Heiner Kallweit <hkallweit1@gmail.com> writes:

> Am 09.06.2017 um 23:15 schrieb Kevin Hilman:
>> Jerome Brunet <jbrunet@baylibre.com> writes:
>> 
>>> On Thu, 2017-06-08 at 21:38 +0200, Heiner Kallweit wrote:
>>>> Add a driver supporting the GPIO interrupt controller on certain
>>>> Amlogic meson SoC's.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> 
>> [...]
>> 
>>>> +static unsigned int meson_irq_startup(struct irq_data *data)
>>>> +{
>>>> +	irq_chip_unmask_parent(data);
>>>> +	/*
>>>> +	 * An extra bit was added to allow having the same gpio hwirq twice
>>>> +	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
>>>> +	 * gpio hwirq.
>>>> +	 */
>>>> +	meson_irq_set_hwirq(data, data->hwirq >> 1);
>>>
>>> Please keep in mind that any device can use this controller as irq parent.
>>> It has to make sense, even when not serving the gpio driver.
>>>
>>> This hack means that, in DT, we'd have to multiply by 2 the values given under
>>> section "22.3 GPIO interrupts" of the datasheet. This is an example Linux
>>> specific stuff in DT.
>>> It also means that the controller declares a lot more lines that it really has
>>> ... 
>>>
>>> This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the
>>> mapping from the irq_set_type callback of the GPIO driver, which is still think
>>> should be dropped at this point.
>> 
>> +1
>> 
>> Please drop the hack for IRQ_TYPE_BOTH so we can reach agreement on the
>> basic design and functionality.  The gymnastics required to support this
>> hack (due to broken hardware) are getting in the way of getting basic
>> functionality merged.
>> 
> I haven't heard any feedback on the other proposal yet:
> Always reserve two parent irq's in the request_resources callback,
> then set_type is easy. How about this one?

The feedback is the same: let's consider that after getting the basics
reviewed, accepted and merged.

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

Yuck, but better than no support for both _BOTH I guess.

But again, discussion of hacks for the hardware limitation is getting in
the way discussing and merging the basics.  Let's get the basics right
first, then add functionality.  

I understand it can be rather frustrating to give up features and/or
functionality for having something "clean", but unfortunately, that's
how the upstream linux kernel community has always worked.

Most maintainers prefer clean, well-designed and maintainable code that
comes in small, easily reviewable chunks over something that's difficult
to understand with hacks and workarounds.  Even if that means a limited
feature set initially.

Additional functionality (even if hacky) is much easier to review,
discuss and maintain *later* when it's on top of a starting point that
is clean.

Kevin

[PATCH v6 3/9] dt-bindings: add Amlogic Meson GPIO interrupt-controller DT binding documentation

[Heiner Kallweit]

Add dt binding documentation for Amlogic meson GPIO interrupt controller.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Rob Herring <robh@kernel.org>
---
v5:
- added Reviewed-by
v6:
- rename parent-interrupts to interrupts
---
 .../amlogic,meson-gpio-intc.txt                    | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt
new file mode 100644
index 00000000..2f20d212
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt
@@ -0,0 +1,26 @@
+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. The actual number of interrupt exposed depends on the
+SoC.
+
+Required properties:
+
+- compatible : should be "amlogic,meson-gpio-intc".
+- reg : Specifies base physical address and size of the registers.
+- interrupt-controller : Identifies the node as an interrupt controller.
+- #interrupt-cells : should be 2.
+- interrupts : list of GIC interrupts which can be used with the
+	       GPIO IRQ multiplexer
+
+Example:
+
+gpio_intc: interrupt-controller@9880 {
+	compatible = "amlogic,meson-gpio-intc";
+	reg = <0x0 0x09880 0x0 0x10>;
+	interrupt-controller;
+	#interrupt-cells = <2>;
+	interrupts = <64 65 66 67 68 69 70 71>;
+};
-- 
2.13.0

[PATCH v6 4/9] ARM: dts: meson: add GPIO interrupt-controller support

[Heiner Kallweit]

Add support for the GPIO interupt controller of certain Amlogic Meson
Soc's.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
v5:
- split ARM and ARM64 into separate patches
- added Reviewed-by
v6:
- change parent-interrupts to interrupts
---
 arch/arm/boot/dts/meson8.dtsi  | 8 ++++++++
 arch/arm/boot/dts/meson8b.dtsi | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
index 69930773..b4c4175d 100644
--- a/arch/arm/boot/dts/meson8.dtsi
+++ b/arch/arm/boot/dts/meson8.dtsi
@@ -126,6 +126,14 @@
 };
 
 &cbus {
+	gpio_intc: interrupt-controller@9880 {
+		compatible = "amlogic,meson-gpio-intc";
+		reg = <0x9880 0x10>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		interrupts = <64 65 66 67 68 79 70 71>;
+	};
+
 	pinctrl_cbus: pinctrl@9880 {
 		compatible = "amlogic,meson8-cbus-pinctrl";
 		reg = <0x9880 0x10>;
diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index d9f116a4..fedfef24 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -112,6 +112,14 @@
 };
 
 &cbus {
+	gpio_intc: interrupt-controller@9880 {
+		compatible = "amlogic,meson-gpio-intc";
+		reg = <0x9880 0x10>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		interrupts = <64 65 66 67 68 79 70 71>;
+	};
+
 	clkc: clock-controller@4000 {
 		#clock-cells = <1>;
 		compatible = "amlogic,meson8b-clkc";
-- 
2.13.0

[PATCH v6 5/9] ARM64: dts: meson: add GPIO interrupt-controller support

[Heiner Kallweit]

Add support for the GPIO interupt controller of certain Amlogic Meson
Soc's.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
v5:
- split ARM and ARM64 into separate patches
- added Reviewed-by
v6:
- change parent-interrupts to interrupts
---
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index 436b8750..66dde761 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -312,6 +312,14 @@
 				status = "disabled";
 			};
 
+			gpio_intc: interrupt-controller@9880 {
+				compatible = "amlogic,meson-gpio-intc";
+				reg = <0x0 0x09880 0x0 0x10>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				interrupts = <64 65 66 67 68 79 70 71>;
+			};
+
 			watchdog@98d0 {
 				compatible = "amlogic,meson-gx-wdt", "amlogic,meson-gxbb-wdt";
 				reg = <0x0 0x098d0 0x0 0x10>;
-- 
2.13.0

[PATCH v6 6/9] pinctrl: meson: add support for GPIO interrupts

[Heiner Kallweit]

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

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v5:
- changed Kconfig entry based on Neil's suggestion
- extended comments
- fixed indentation
v6:
- no changes
---
 drivers/pinctrl/Kconfig               |   1 +
 drivers/pinctrl/meson/pinctrl-meson.c | 170 +++++++++++++++++++++++++++++++++-
 2 files changed, 170 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 7ae04a97..86834dea 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -166,6 +166,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/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
index 66ed70c1..7bacd4e3 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"
 
+static struct irq_domain *meson_pinctrl_irq_domain;
+
 /**
  * meson_get_bank() - find the bank containing a given pin
  *
@@ -497,6 +499,154 @@ static int meson_gpio_get(struct gpio_chip *chip, unsigned gpio)
 	return !!(val & BIT(bit));
 }
 
+static struct meson_pinctrl *meson_gpio_data_to_pc(struct irq_data *data)
+{
+        struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+
+        return gpiochip_get_data(chip);
+}
+
+static int meson_gpio_bank_hwirq(struct meson_bank *bank, unsigned int offset)
+{
+        int hwirq;
+
+        if (bank->irq_first < 0)
+                /* this bank cannot generate irqs */
+                return 0;
+
+        hwirq = offset - bank->first + bank->irq_first;
+
+        if (hwirq > bank->irq_last)
+                /* this pin cannot generate irqs */
+                return 0;
+
+        return hwirq;
+}
+
+static int meson_gpio_to_hwirq(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	unsigned int offset = data->hwirq;
+        struct meson_bank *bank;
+        int hwirq, ret;
+
+        offset += pc->data->pin_base;
+
+        ret = meson_get_bank(pc, offset, &bank);
+        if (ret)
+                return ret;
+
+        hwirq = meson_gpio_bank_hwirq(bank, offset);
+        if (!hwirq)
+                dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
+
+        return hwirq;
+}
+
+static void meson_gpio_irq_handler(struct irq_desc *desc)
+{
+        struct irq_chip *chip = irq_desc_get_chip(desc);
+        struct irq_data *gpio_irq_data = irq_desc_get_handler_data(desc);
+
+        chained_irq_enter(chip, desc);
+
+        if (gpio_irq_data)
+                generic_handle_irq(gpio_irq_data->irq);
+
+        chained_irq_exit(chip, desc);
+}
+
+static void meson_gpio_irq_unmask(struct irq_data *data) {}
+static void meson_gpio_irq_mask(struct irq_data *data) {}
+
+static void meson_gpio_irq_shutdown(struct irq_data *data)
+{
+	int hwirq = meson_gpio_to_hwirq(data);
+	int irq;
+
+	if (hwirq <= 0)
+		return;
+
+	/*
+	 * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts,
+	 * one for each edge. That's due to HW constraints.
+	 * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can
+	 * have one GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq
+	 * by shifting hwirq one bit to the right.
+	 */
+	irq = irq_find_mapping(meson_pinctrl_irq_domain, hwirq * 2);
+	if (irq) {
+		irq_set_chained_handler_and_data(irq, handle_bad_irq, NULL);
+		irq_domain_free_irqs(irq, 1);
+	}
+
+	irq = irq_find_mapping(meson_pinctrl_irq_domain, hwirq * 2 + 1);
+	if (irq) {
+		irq_set_chained_handler_and_data(irq, handle_bad_irq, NULL);
+		irq_domain_free_irqs(irq, 1);
+	}
+}
+
+static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	int hwirq = meson_gpio_to_hwirq(data);
+	struct irq_fwspec fwspec;
+        int irq, irq2, num_slots;
+
+	if (irqd_is_activated(data))
+		return -EBUSY;
+
+	if (hwirq < 0)
+		return hwirq;
+
+	if (!hwirq)
+		return -ENXIO;
+
+	fwspec.fwnode = meson_pinctrl_irq_domain->fwnode;
+	fwspec.param_count = 2;
+
+	/*
+	 * The chip can create an interrupt for either rising or falling edge
+	 * only. Therefore use two interrupts in case of IRQ_TYPE_EDGE_BOTH,
+	 * first for falling edge and second one for rising edge.
+	 */
+	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
+
+	/* see comment in meson_gpio_irq_shutdown why we shift one bit here */
+	fwspec.param[0] = hwirq << 1;
+	if (num_slots == 1)
+		fwspec.param[1] = type;
+	else
+		fwspec.param[1] = IRQ_TYPE_EDGE_FALLING;
+
+	irq = irq_create_fwspec_mapping(&fwspec);
+	if (!irq)
+		return -EINVAL;
+
+	irq_set_chained_handler_and_data(irq, meson_gpio_irq_handler, data);
+
+	if (num_slots > 1) {
+		fwspec.param[0]++;
+		fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
+		irq2 = irq_create_fwspec_mapping(&fwspec);
+		if (!irq2) {
+			irq_domain_free_irqs(irq, 1);
+			return -EINVAL;
+		}
+		irq_set_chained_handler_and_data(irq2, meson_gpio_irq_handler, data);
+	}
+
+        return 0;
+}
+
+static struct irq_chip meson_gpio_irq_chip = {
+        .name = "GPIO",
+        .irq_set_type = meson_gpio_irq_set_type,
+        .irq_mask = meson_gpio_irq_mask,
+        .irq_unmask = meson_gpio_irq_unmask,
+        .irq_shutdown = meson_gpio_irq_shutdown,
+};
+
 static const struct of_device_id meson_pinctrl_dt_match[] = {
 	{
 		.compatible = "amlogic,meson8-cbus-pinctrl",
@@ -558,7 +708,8 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc)
 		return ret;
 	}
 
-	return 0;
+	return gpiochip_irqchip_add(&pc->chip, &meson_gpio_irq_chip, 0,
+				    handle_simple_irq, IRQ_TYPE_NONE);
 }
 
 static struct regmap_config meson_regmap_config = {
@@ -637,6 +788,23 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc,
 		return PTR_ERR(pc->reg_gpio);
 	}
 
+	if (!meson_pinctrl_irq_domain) {
+		np = of_find_compatible_node(NULL, NULL, "amlogic,meson-gpio-intc");
+		if (!np) {
+			dev_err(pc->dev, "interrupt controller DT node not found\n");
+			return -EINVAL;
+		}
+
+		meson_pinctrl_irq_domain = irq_find_host(np);
+		if (!meson_pinctrl_irq_domain) {
+			dev_err(pc->dev, "interrupt controller not found\n");
+			of_node_put(np);
+			return -EINVAL;
+		}
+
+		of_node_put(np);
+	}
+
 	return 0;
 }
 
-- 
2.13.0

Re: [PATCH v6 6/9] pinctrl: meson: add support for GPIO interrupts

[Jerome Brunet]

On Thu, 2017-06-08 at 21:39 +0200, Heiner Kallweit wrote:
> Add support for GPIO interrupts and make use of the just introduced
> irqchip driver handling the GPIO interrupt-controller.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v5:
> - changed Kconfig entry based on Neil's suggestion
> - extended comments
> - fixed indentation
> v6:
> - no changes
> ---
>  drivers/pinctrl/Kconfig               |   1 +
>  drivers/pinctrl/meson/pinctrl-meson.c | 170
> +++++++++++++++++++++++++++++++++-
>  2 files changed, 170 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 7ae04a97..86834dea 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -166,6 +166,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/pinctrl-meson.c
> b/drivers/pinctrl/meson/pinctrl-meson.c
> index 66ed70c1..7bacd4e3 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"
>  
> +static struct irq_domain *meson_pinctrl_irq_domain;
> +
>  /**
>   * meson_get_bank() - find the bank containing a given pin
>   *
> @@ -497,6 +499,154 @@ static int meson_gpio_get(struct gpio_chip *chip,
> unsigned gpio)
>  	return !!(val & BIT(bit));
>  }
>  
> +static struct meson_pinctrl *meson_gpio_data_to_pc(struct irq_data *data)
> +{
> +        struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> +
> +        return gpiochip_get_data(chip);
> +}
> +
> +static int meson_gpio_bank_hwirq(struct meson_bank *bank, unsigned int
> offset)
> +{
> +        int hwirq;
> +
> +        if (bank->irq_first < 0)
> +                /* this bank cannot generate irqs */
> +                return 0;
> +
> +        hwirq = offset - bank->first + bank->irq_first;
> +
> +        if (hwirq > bank->irq_last)
> +                /* this pin cannot generate irqs */
> +                return 0;
> +
> +        return hwirq;
> +}
> +
> +static int meson_gpio_to_hwirq(struct irq_data *data)
> +{
> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
> +	unsigned int offset = data->hwirq;
> +        struct meson_bank *bank;
> +        int hwirq, ret;
> +
> +        offset += pc->data->pin_base;
> +
> +        ret = meson_get_bank(pc, offset, &bank);
> +        if (ret)
> +                return ret;
> +
> +        hwirq = meson_gpio_bank_hwirq(bank, offset);
> +        if (!hwirq)
> +                dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
> +
> +        return hwirq;
> +}
> +
> +static void meson_gpio_irq_handler(struct irq_desc *desc)
> +{
> +        struct irq_chip *chip = irq_desc_get_chip(desc);
> +        struct irq_data *gpio_irq_data = irq_desc_get_handler_data(desc);
> +
> +        chained_irq_enter(chip, desc);
> +
> +        if (gpio_irq_data)
> +                generic_handle_irq(gpio_irq_data->irq);
> +
> +        chained_irq_exit(chip, desc);
> +}
> +
> +static void meson_gpio_irq_unmask(struct irq_data *data) {}
> +static void meson_gpio_irq_mask(struct irq_data *data) {}
> +
> +static void meson_gpio_irq_shutdown(struct irq_data *data)
> +{
> +	int hwirq = meson_gpio_to_hwirq(data);
> +	int irq;
> +
> +	if (hwirq <= 0)
> +		return;
> +
> +	/*
> +	 * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts,
> +	 * one for each edge. That's due to HW constraints.
> +	 * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can
> +	 * have one GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq
> +	 * by shifting hwirq one bit to the right.
> +	 */
> +	irq = irq_find_mapping(meson_pinctrl_irq_domain, hwirq * 2);
> +	if (irq) {
> +		irq_set_chained_handler_and_data(irq, handle_bad_irq, NULL);
> +		irq_domain_free_irqs(irq, 1);
> +	}
> +
> +	irq = irq_find_mapping(meson_pinctrl_irq_domain, hwirq * 2 + 1);
> +	if (irq) {
> +		irq_set_chained_handler_and_data(irq, handle_bad_irq, NULL);
> +		irq_domain_free_irqs(irq, 1);
> +	}
> +}
> +
> +static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> +	int hwirq = meson_gpio_to_hwirq(data);
> +	struct irq_fwspec fwspec;
> +        int irq, irq2, num_slots;
> +
> +	if (irqd_is_activated(data))
> +		return -EBUSY;

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

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


> +
> +	if (hwirq < 0)
> +		return hwirq;
> +
> +	if (!hwirq)
> +		return -ENXIO;
> +
> +	fwspec.fwnode = meson_pinctrl_irq_domain->fwnode;
> +	fwspec.param_count = 2;
> +
> +	/*
> +	 * The chip can create an interrupt for either rising or falling edge
> +	 * only. Therefore use two interrupts in case of IRQ_TYPE_EDGE_BOTH,
> +	 * first for falling edge and second one for rising edge.
> +	 */
> +	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
> +
> +	/* see comment in meson_gpio_irq_shutdown why we shift one bit here
> */
> +	fwspec.param[0] = hwirq << 1;
> +	if (num_slots == 1)
> +		fwspec.param[1] = type;
> +	else
> +		fwspec.param[1] = IRQ_TYPE_EDGE_FALLING;
> +
> +	irq = irq_create_fwspec_mapping(&fwspec);
> +	if (!irq)
> +		return -EINVAL;
> +
> +	irq_set_chained_handler_and_data(irq, meson_gpio_irq_handler, data);
> +
> +	if (num_slots > 1) {
> +		fwspec.param[0]++;
> +		fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> +		irq2 = irq_create_fwspec_mapping(&fwspec);
> +		if (!irq2) {
> +			irq_domain_free_irqs(irq, 1);
> +			return -EINVAL;
> +		}
> +		irq_set_chained_handler_and_data(irq2,
> meson_gpio_irq_handler, data);
> +	}
> +
> +        return 0;
> +}
> +
> +static struct irq_chip meson_gpio_irq_chip = {
> +        .name = "GPIO",
> +        .irq_set_type = meson_gpio_irq_set_type,
> +        .irq_mask = meson_gpio_irq_mask,
> +        .irq_unmask = meson_gpio_irq_unmask,
> +        .irq_shutdown = meson_gpio_irq_shutdown,
> +};
> +
>  static const struct of_device_id meson_pinctrl_dt_match[] = {
>  	{
>  		.compatible = "amlogic,meson8-cbus-pinctrl",
> @@ -558,7 +708,8 @@ static int meson_gpiolib_register(struct meson_pinctrl
> *pc)
>  		return ret;
>  	}
>  
> -	return 0;
> +	return gpiochip_irqchip_add(&pc->chip, &meson_gpio_irq_chip, 0,
> +				    handle_simple_irq, IRQ_TYPE_NONE);
>  }
>  
>  static struct regmap_config meson_regmap_config = {
> @@ -637,6 +788,23 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl
> *pc,
>  		return PTR_ERR(pc->reg_gpio);
>  	}
>  
> +	if (!meson_pinctrl_irq_domain) {

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

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

Re: [PATCH v6 6/9] pinctrl: meson: add support for GPIO interrupts

[Heiner Kallweit]

Am 09.06.2017 um 11:06 schrieb Jerome Brunet:
> On Thu, 2017-06-08 at 21:39 +0200, Heiner Kallweit wrote:
>> Add support for GPIO interrupts and make use of the just introduced
>> irqchip driver handling the GPIO interrupt-controller.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v5:
>> - changed Kconfig entry based on Neil's suggestion
>> - extended comments
>> - fixed indentation
>> v6:
>> - no changes
>> ---
>>  drivers/pinctrl/Kconfig               |   1 +
>>  drivers/pinctrl/meson/pinctrl-meson.c | 170
>> +++++++++++++++++++++++++++++++++-
>>  2 files changed, 170 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>> index 7ae04a97..86834dea 100644
>> --- a/drivers/pinctrl/Kconfig
>> +++ b/drivers/pinctrl/Kconfig
>> @@ -166,6 +166,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/pinctrl-meson.c
>> b/drivers/pinctrl/meson/pinctrl-meson.c
>> index 66ed70c1..7bacd4e3 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"
>>  
>> +static struct irq_domain *meson_pinctrl_irq_domain;
>> +
>>  /**
>>   * meson_get_bank() - find the bank containing a given pin
>>   *
>> @@ -497,6 +499,154 @@ static int meson_gpio_get(struct gpio_chip *chip,
>> unsigned gpio)
>>  	return !!(val & BIT(bit));
>>  }
>>  
>> +static struct meson_pinctrl *meson_gpio_data_to_pc(struct irq_data *data)
>> +{
>> +        struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
>> +
>> +        return gpiochip_get_data(chip);
>> +}
>> +
>> +static int meson_gpio_bank_hwirq(struct meson_bank *bank, unsigned int
>> offset)
>> +{
>> +        int hwirq;
>> +
>> +        if (bank->irq_first < 0)
>> +                /* this bank cannot generate irqs */
>> +                return 0;
>> +
>> +        hwirq = offset - bank->first + bank->irq_first;
>> +
>> +        if (hwirq > bank->irq_last)
>> +                /* this pin cannot generate irqs */
>> +                return 0;
>> +
>> +        return hwirq;
>> +}
>> +
>> +static int meson_gpio_to_hwirq(struct irq_data *data)
>> +{
>> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
>> +	unsigned int offset = data->hwirq;
>> +        struct meson_bank *bank;
>> +        int hwirq, ret;
>> +
>> +        offset += pc->data->pin_base;
>> +
>> +        ret = meson_get_bank(pc, offset, &bank);
>> +        if (ret)
>> +                return ret;
>> +
>> +        hwirq = meson_gpio_bank_hwirq(bank, offset);
>> +        if (!hwirq)
>> +                dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
>> +
>> +        return hwirq;
>> +}
>> +
>> +static void meson_gpio_irq_handler(struct irq_desc *desc)
>> +{
>> +        struct irq_chip *chip = irq_desc_get_chip(desc);
>> +        struct irq_data *gpio_irq_data = irq_desc_get_handler_data(desc);
>> +
>> +        chained_irq_enter(chip, desc);
>> +
>> +        if (gpio_irq_data)
>> +                generic_handle_irq(gpio_irq_data->irq);
>> +
>> +        chained_irq_exit(chip, desc);
>> +}
>> +
>> +static void meson_gpio_irq_unmask(struct irq_data *data) {}
>> +static void meson_gpio_irq_mask(struct irq_data *data) {}
>> +
>> +static void meson_gpio_irq_shutdown(struct irq_data *data)
>> +{
>> +	int hwirq = meson_gpio_to_hwirq(data);
>> +	int irq;
>> +
>> +	if (hwirq <= 0)
>> +		return;
>> +
>> +	/*
>> +	 * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts,
>> +	 * one for each edge. That's due to HW constraints.
>> +	 * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can
>> +	 * have one GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq
>> +	 * by shifting hwirq one bit to the right.
>> +	 */
>> +	irq = irq_find_mapping(meson_pinctrl_irq_domain, hwirq * 2);
>> +	if (irq) {
>> +		irq_set_chained_handler_and_data(irq, handle_bad_irq, NULL);
>> +		irq_domain_free_irqs(irq, 1);
>> +	}
>> +
>> +	irq = irq_find_mapping(meson_pinctrl_irq_domain, hwirq * 2 + 1);
>> +	if (irq) {
>> +		irq_set_chained_handler_and_data(irq, handle_bad_irq, NULL);
>> +		irq_domain_free_irqs(irq, 1);
>> +	}
>> +}
>> +
>> +static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> +	int hwirq = meson_gpio_to_hwirq(data);
>> +	struct irq_fwspec fwspec;
>> +        int irq, irq2, num_slots;
>> +
>> +	if (irqd_is_activated(data))
>> +		return -EBUSY;
> 
> Again, your implementation assume that:
> * irq_set_type should be called only once in the life of the irq.
> * and/or irq_set_type cannot be called from irq context
> 
> I don't agree with those assumptions.
> If irq_set_type could not called several times, to change the type at runtime,
> why would the irq framework bother providing "irq_set_irq_type" in the public
> API ? Why bother with irq_set_type callback at all ? It could all be done in the
> startup callback... 
> 
Due to the hardware restrictions we need a workaround for supporting
IRQ_TYPE_EDGE_BOTH. It may not support each and every theoretically possible
use case of irq_set_irq_type() but it works in the relevant scenarios and we
would have something NOW.
If somebody isn't happy with it he can still improve it.
Else we go on discussing until these chips are discontinued.

Against most negative side effects of calling irq_set_irq_type again we're
protected by rejecting calls when the IRQ is activated already.

And I made one more proposal:
We could in general reserve two parent irq's. Then we don't have the issue
with irq_set_irq_type (paying the price of supporting max. 4 gpio irq's).

> 
>> +
>> +	if (hwirq < 0)
>> +		return hwirq;
>> +
>> +	if (!hwirq)
>> +		return -ENXIO;
>> +
>> +	fwspec.fwnode = meson_pinctrl_irq_domain->fwnode;
>> +	fwspec.param_count = 2;
>> +
>> +	/*
>> +	 * The chip can create an interrupt for either rising or falling edge
>> +	 * only. Therefore use two interrupts in case of IRQ_TYPE_EDGE_BOTH,
>> +	 * first for falling edge and second one for rising edge.
>> +	 */
>> +	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
>> +
>> +	/* see comment in meson_gpio_irq_shutdown why we shift one bit here
>> */
>> +	fwspec.param[0] = hwirq << 1;
>> +	if (num_slots == 1)
>> +		fwspec.param[1] = type;
>> +	else
>> +		fwspec.param[1] = IRQ_TYPE_EDGE_FALLING;
>> +
>> +	irq = irq_create_fwspec_mapping(&fwspec);
>> +	if (!irq)
>> +		return -EINVAL;
>> +
>> +	irq_set_chained_handler_and_data(irq, meson_gpio_irq_handler, data);
>> +
>> +	if (num_slots > 1) {
>> +		fwspec.param[0]++;
>> +		fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
>> +		irq2 = irq_create_fwspec_mapping(&fwspec);
>> +		if (!irq2) {
>> +			irq_domain_free_irqs(irq, 1);
>> +			return -EINVAL;
>> +		}
>> +		irq_set_chained_handler_and_data(irq2,
>> meson_gpio_irq_handler, data);
>> +	}
>> +
>> +        return 0;
>> +}
>> +
>> +static struct irq_chip meson_gpio_irq_chip = {
>> +        .name = "GPIO",
>> +        .irq_set_type = meson_gpio_irq_set_type,
>> +        .irq_mask = meson_gpio_irq_mask,
>> +        .irq_unmask = meson_gpio_irq_unmask,
>> +        .irq_shutdown = meson_gpio_irq_shutdown,
>> +};
>> +
>>  static const struct of_device_id meson_pinctrl_dt_match[] = {
>>  	{
>>  		.compatible = "amlogic,meson8-cbus-pinctrl",
>> @@ -558,7 +708,8 @@ static int meson_gpiolib_register(struct meson_pinctrl
>> *pc)
>>  		return ret;
>>  	}
>>  
>> -	return 0;
>> +	return gpiochip_irqchip_add(&pc->chip, &meson_gpio_irq_chip, 0,
>> +				    handle_simple_irq, IRQ_TYPE_NONE);
>>  }
>>  
>>  static struct regmap_config meson_regmap_config = {
>> @@ -637,6 +788,23 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl
>> *pc,
>>  		return PTR_ERR(pc->reg_gpio);
>>  	}
>>  
>> +	if (!meson_pinctrl_irq_domain) {
> 
> This be in " struct meson_pinctrl *pc" not a global.
> Each instance should parse its own bindings and not rely on the other one for
> this !
> 
There can only be one such gpio irq controller node. Of course we could do
exactly the same parsing with exactly the same result twice, bit I don't
really see a benefit in it.

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

[PATCH v6 7/9] pinctrl: meson: update DT binding documentation

[Heiner Kallweit]

The GPIO controllers act as interrupt controllers now,
therefore update the related DT binding documentation.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Rob Herring <robh@kernel.org>
---
v5:
- separate documentation update from actual DT update
- add Reviewed-by
v6:
- add Acked-by
---
 Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
index 2392557e..49a8118a 100644
--- a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
@@ -24,6 +24,8 @@ Required properties for sub-nodes are:
    when it is missing the "pull" registers are used instead
  - gpio-controller: identifies the node as a gpio controller
  - #gpio-cells: must be 2
+ - interrupt-controller: identifies the node as interrupt controller
+ - #interrupt-cells: must be 2
 
 === Other sub-nodes ===
 
@@ -62,6 +64,8 @@ pinctrl-bindings.txt
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
                };
 
 		nand {
-- 
2.13.0

[PATCH v6 8/9] ARM: dts: meson: mark gpio controllers as interrupt controllers

[Heiner Kallweit]

Make the GPIO controllers act as interrupt controllers.

Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
v5:
- separate ARM / ARM64 DT changes
- add Reviewed-by
v6:
- no changes
---
 arch/arm/boot/dts/meson8.dtsi  | 4 ++++
 arch/arm/boot/dts/meson8b.dtsi | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
index b4c4175d..c64ddd58 100644
--- a/arch/arm/boot/dts/meson8.dtsi
+++ b/arch/arm/boot/dts/meson8.dtsi
@@ -106,6 +106,8 @@
 			reg-names = "mux", "pull", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_aobus 0 120 16>;
 		};
 
@@ -149,6 +151,8 @@
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_cbus 0 0 120>;
 		};
 
diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index fedfef24..dd7ed539 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -99,6 +99,8 @@
 			reg-names = "mux", "pull", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_aobus 0 130 16>;
 		};
 
@@ -174,6 +176,8 @@
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_cbus 0 0 130>;
 		};
 	};
-- 
2.13.0



--
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 v6 9/9] ARM64: dts: meson: mark gpio controllers as interrupt controllers

[Heiner Kallweit]

Make the GPIO controllers act as interrupt controllers.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
v5:
- separate ARM / ARM64 DT changes
- add Reviewed-by
v6:
- no changes
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 4 ++++
 arch/arm64/boot/dts/amlogic/meson-gxl.dtsi  | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index 6f8b3011..a771dae8 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -111,6 +111,8 @@
 			reg-names = "mux", "pull", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_aobus 0 0 14>;
 		};
 
@@ -356,6 +358,8 @@
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_periphs 0 14 120>;
 		};
 
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
index 3efad5f0..16b584f5 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
@@ -80,6 +80,8 @@
 			reg-names = "mux", "pull", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_aobus 0 0 14>;
 		};
 
@@ -244,6 +246,8 @@
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_periphs 0 14 101>;
 		};
 
-- 
2.13.0