[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[PATCH 0/2] pinctrl: Enable support for external GPIO interrupts

[Carlo Caione]

From: Carlo Caione <carlo@endlessm.com>

In Meson SoCs we have 8 independent GPIO interrupts that can be programmed to
use any of the GPIOs in the chip as interrupt source.

These GPIOs are managed by GIC but they can be conditioned (and enabled) by
some registers external to the GIC.

GPIOs |--[mux1 or mux2]--[polarity]--[filter]--[edge_select]--> GIC

The original work has been done by Beniamino. I cleaned it up, fixed a couple
of bugs, added support for Meson8b and the fix for the .to_irq hook.

Beniamino Galvani (2):
  pinctrl: meson: Enable GPIO IRQs
  pinctrl: dt-binding: Extend meson documentation with GPIO IRQs support

 .../devicetree/bindings/pinctrl/meson,pinctrl.txt  |  12 +
 drivers/pinctrl/Kconfig                            |   1 +
 drivers/pinctrl/meson/pinctrl-meson.c              | 254 +++++++++++++++++++++
 drivers/pinctrl/meson/pinctrl-meson.h              |  18 +-
 drivers/pinctrl/meson/pinctrl-meson8.c             |  36 +--
 drivers/pinctrl/meson/pinctrl-meson8b.c            |  36 +--
 6 files changed, 326 insertions(+), 31 deletions(-)

-- 
1.9.1

[PATCH 1/2] pinctrl: meson: Enable GPIO IRQs

[Carlo Caione]

From: Beniamino Galvani <b.galvani@gmail.com>

On Meson8 and Meson8b SoCs there are 8 independent filtered GPIO
interrupt modules that can be programmed to use any of the GPIOs in the
chip as an interrupt source.

For each GPIO IRQ we have:

GPIOs --> [mux]--> [polarity]--> [filter]--> [edge select]--> GIC

The eight GPIO interrupts respond to mask/unmask/clear/etc.. just like
any other interrupt in the chip. The difference for the GPIO interrupts
is that they can be filtered and conditioned.

This patch adds support for the external GPIOs interrupts and enables
them for Meson8 and Meson8b SoCs.

Signed-off-by: Carlo Caione <carlo@endlessm.com>
Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 drivers/pinctrl/Kconfig                 |   1 +
 drivers/pinctrl/meson/pinctrl-meson.c   | 254 ++++++++++++++++++++++++++++++++
 drivers/pinctrl/meson/pinctrl-meson.h   |  18 ++-
 drivers/pinctrl/meson/pinctrl-meson8.c  |  36 +++--
 drivers/pinctrl/meson/pinctrl-meson8b.c |  36 +++--
 5 files changed, 314 insertions(+), 31 deletions(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index aeb5729..9dd7bb3 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -102,6 +102,7 @@ config PINCTRL_MESON
 	select GPIOLIB
 	select OF_GPIO
 	select REGMAP_MMIO
+	select IRQ_DOMAIN_HIERARCHY
 
 config PINCTRL_ROCKCHIP
 	bool
diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
index a70a5fe..cbbdf2d 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.c
+++ b/drivers/pinctrl/meson/pinctrl-meson.c
@@ -59,11 +59,25 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/seq_file.h>
+#include <linux/of_irq.h>
 
 #include "../core.h"
 #include "../pinctrl-utils.h"
 #include "pinctrl-meson.h"
 
+#define REG_EDGE_POL		0x00
+#define REG_GPIO_SEL0		0x04
+#define REG_GPIO_SEL1		0x08
+#define REG_FILTER		0x0c
+
+#define IRQ_FREE		(-1)
+
+#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
+#define REG_EDGE_POL_LEVEL(x)	0
+#define REG_EDGE_POL_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_HIGH(x)	0
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
+
 /**
  * meson_get_bank() - find the bank containing a given pin
  *
@@ -540,6 +554,27 @@ static int meson_gpio_get(struct gpio_chip *chip, unsigned gpio)
 	return !!(val & BIT(bit));
 }
 
+static int meson_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	struct meson_domain *domain = to_meson_domain(chip);
+	struct meson_pinctrl *pc = domain->pinctrl;
+	struct meson_bank *bank;
+	struct of_phandle_args irq_data;
+	unsigned int pin, virq;
+	int ret;
+
+	pin = domain->data->pin_base + offset;
+	ret = meson_get_bank(domain, pin, &bank);
+	if (ret)
+		return -ENXIO;
+
+	irq_data.args_count = 2;
+	irq_data.args[0] = pin;
+	virq = irq_domain_alloc_irqs(pc->irq_domain, 1, NUMA_NO_NODE, &irq_data);
+
+	return virq ? virq : -ENXIO;
+}
+
 static const struct of_device_id meson_pinctrl_dt_match[] = {
 	{
 		.compatible = "amlogic,meson8-pinctrl",
@@ -569,6 +604,7 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc)
 		domain->chip.direction_output = meson_gpio_direction_output;
 		domain->chip.get = meson_gpio_get;
 		domain->chip.set = meson_gpio_set;
+		domain->chip.to_irq = meson_gpio_to_irq;
 		domain->chip.base = domain->data->pin_base;
 		domain->chip.ngpio = domain->data->num_pins;
 		domain->chip.can_sleep = false;
@@ -680,6 +716,7 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc,
 		}
 
 		domain->of_node = np;
+		domain->pinctrl = pc;
 
 		domain->reg_mux = meson_map_resource(pc, np, "mux");
 		if (IS_ERR(domain->reg_mux)) {
@@ -710,6 +747,219 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc,
 	return 0;
 }
 
+static int meson_get_gic_irq(struct meson_pinctrl *pc, int hwirq)
+{
+	int i = 0;
+
+	for (i = 0; i < pc->num_gic_irqs; i++) {
+		if (pc->irq_map[i] == hwirq)
+			return i;
+	}
+
+	return -1;
+}
+
+static int meson_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct meson_pinctrl *pc = data->chip_data;
+	u32 val = 0;
+	int index;
+
+	dev_dbg(pc->dev, "set type of hwirq %lu to %u\n", data->hwirq, type);
+	spin_lock(&pc->lock);
+	index = meson_get_gic_irq(pc, data->hwirq);
+
+	if (index < 0) {
+		spin_unlock(&pc->lock);
+		dev_err(pc->dev, "hwirq %lu not allocated\n", data->hwirq);
+		return -EINVAL;
+	}
+
+	if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_EDGE_RISING)
+		val |= REG_EDGE_POL_EDGE(index);
+	if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_LEVEL_LOW)
+		val |= REG_EDGE_POL_LOW(index);
+
+	regmap_update_bits(pc->reg_irq, REG_EDGE_POL, REG_EDGE_POL_MASK(index),
+			   val);
+	spin_unlock(&pc->lock);
+
+	if (type == IRQ_TYPE_LEVEL_LOW)
+		type = IRQ_TYPE_LEVEL_HIGH;
+	else if (type == IRQ_TYPE_EDGE_FALLING)
+		type = IRQ_TYPE_EDGE_RISING;
+
+	data = data->parent_data;
+	data->chip->irq_set_type(data, type);
+
+	return 0;
+}
+
+static struct irq_chip meson_irq_chip = {
+	.name			= "meson-gpio-irqchip",
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_type		= meson_irq_set_type,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
+static int meson_map_gic_irq(struct irq_domain *irq_domain,
+			     irq_hw_number_t hwirq)
+{
+	struct meson_pinctrl *pc = irq_domain->host_data;
+	struct meson_domain *domain;
+	struct meson_bank *bank;
+	int index, reg, ret;
+
+	ret = meson_get_domain_and_bank(pc, hwirq, &domain, &bank);
+	if (ret)
+		return ret;
+
+	spin_lock(&pc->lock);
+
+	index = meson_get_gic_irq(pc, IRQ_FREE);
+	if (index < 0) {
+		spin_unlock(&pc->lock);
+		dev_err(pc->dev, "no free GIC interrupt found");
+		return -ENOSPC;
+	}
+
+	dev_dbg(pc->dev, "found free GIC interrupt %d\n", index);
+	pc->irq_map[index] = hwirq;
+
+	/* Setup IRQ mapping */
+	reg = index < 4 ? REG_GPIO_SEL0 : REG_GPIO_SEL1;
+	regmap_update_bits(pc->reg_irq, reg, 0xff << (index % 4) * 8,
+			   (bank->irq + hwirq - bank->first) << (index % 4) * 8);
+
+	/* Set filter to the default, undocumented value of 7 */
+	regmap_update_bits(pc->reg_irq, REG_FILTER, 0xf << index * 4,
+			   7 << index * 4);
+
+	spin_unlock(&pc->lock);
+
+	return index;
+}
+
+static int meson_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				  unsigned int nr_irqs, void *arg)
+{
+	struct meson_pinctrl *pc = domain->host_data;
+	struct of_phandle_args *irq_data = arg;
+	struct of_phandle_args gic_data;
+	irq_hw_number_t hwirq;
+	int index, ret, i;
+
+	if (irq_data->args_count != 2)
+		return -EINVAL;
+
+	hwirq = irq_data->args[0];
+	dev_dbg(pc->dev, "%s virq %d, nr %d, hwirq %lu\n",
+		__func__, virq, nr_irqs, hwirq);
+
+	for (i = 0; i < nr_irqs; i++) {
+		index = meson_map_gic_irq(domain, hwirq + i);
+		if (index < 0)
+			return index;
+
+		irq_domain_set_hwirq_and_chip(domain, virq + i,
+					      hwirq + i,
+					      &meson_irq_chip,
+					      pc);
+
+		gic_data = pc->gic_irqs[index];
+		ret = irq_domain_alloc_irqs_parent(domain, virq + i, nr_irqs,
+						   &gic_data);
+	}
+
+	return 0;
+}
+
+static void meson_irq_domain_free(struct irq_domain *domain, unsigned int virq,
+				  unsigned int nr_irqs)
+{
+	struct meson_pinctrl *pc = domain->host_data;
+	struct irq_data *irq_data;
+	int index, i;
+
+	spin_lock(&pc->lock);
+	for (i = 0; i < nr_irqs; i++) {
+		irq_data = irq_domain_get_irq_data(domain, virq + i);
+		index = meson_get_gic_irq(pc, irq_data->hwirq);
+		if (index < 0)
+			continue;
+		pc->irq_map[index] = IRQ_FREE;
+	}
+	spin_unlock(&pc->lock);
+
+	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+}
+
+static struct irq_domain_ops meson_irq_domain_ops = {
+	.alloc		= meson_irq_domain_alloc,
+	.free		= meson_irq_domain_free,
+	.xlate		= irq_domain_xlate_twocell,
+};
+
+static int meson_gpio_irq_init(struct platform_device *pdev,
+			       struct meson_pinctrl *pc)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct device_node *parent_node;
+	struct irq_domain *parent_domain;
+	int i;
+
+	parent_node = of_irq_find_parent(node);
+	if (!parent_node) {
+		dev_err(pc->dev, "can't find parent interrupt controller\n");
+		return -EINVAL;
+	}
+
+	parent_domain = irq_find_host(parent_node);
+	if (!parent_domain) {
+		dev_err(pc->dev, "can't find parent IRQ domain\n");
+		return -EINVAL;
+	}
+
+	pc->reg_irq = meson_map_resource(pc, node, "irq");
+	if (!pc->reg_irq) {
+		dev_err(pc->dev, "can't find irq registers\n");
+		return -EINVAL;
+	}
+
+	pc->num_gic_irqs = of_irq_count(node);
+	if (!pc->num_gic_irqs) {
+		dev_err(pc->dev, "no parent interrupts specified\n");
+		return -EINVAL;
+	}
+
+	pc->irq_map = devm_kmalloc(pc->dev, sizeof(int) * pc->num_gic_irqs,
+				   GFP_KERNEL);
+	if (!pc->irq_map)
+		return -ENOMEM;
+
+	pc->gic_irqs = devm_kzalloc(pc->dev, sizeof(struct of_phandle_args) *
+				    pc->num_gic_irqs, GFP_KERNEL);
+	if (!pc->gic_irqs)
+		return -ENOMEM;
+
+	for (i = 0; i < pc->num_gic_irqs; i++) {
+		of_irq_parse_one(node, i, &pc->gic_irqs[i]);
+		pc->irq_map[i] = IRQ_FREE;
+	}
+
+	pc->irq_domain = irq_domain_add_hierarchy(parent_domain, 0,
+						  pc->data->last_pin,
+						  node, &meson_irq_domain_ops,
+						  pc);
+	if (!pc->irq_domain)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int meson_pinctrl_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
@@ -749,6 +999,10 @@ static int meson_pinctrl_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = meson_gpio_irq_init(pdev, pc);
+	if (ret)
+		dev_err(pc->dev, "can't setup gpio interrupts\n");
+
 	return 0;
 }
 
diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
index 0fe7d53..281e0c4 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.h
+++ b/drivers/pinctrl/meson/pinctrl-meson.h
@@ -16,6 +16,8 @@
 #include <linux/regmap.h>
 #include <linux/types.h>
 
+struct meson_pinctrl;
+
 /**
  * struct meson_pmx_group - a pinmux group
  *
@@ -83,6 +85,7 @@ enum meson_reg_type {
  * @first:	first pin of the bank
  * @last:	last pin of the bank
  * @regs:	array of register descriptors
+ * @irq:	input mux location for IRQs
  *
  * A bank represents a set of pins controlled by a contiguous set of
  * bits in the domain registers. The structure specifies which bits in
@@ -94,6 +97,7 @@ struct meson_bank {
 	unsigned int first;
 	unsigned int last;
 	struct meson_reg_desc regs[NUM_REG];
+	unsigned int irq;
 };
 
 /**
@@ -109,6 +113,7 @@ struct meson_domain_data {
 	unsigned int num_banks;
 	unsigned int pin_base;
 	unsigned int num_pins;
+	struct meson_pinctrl *pinctrl;
 };
 
 /**
@@ -121,6 +126,7 @@ struct meson_domain_data {
  * @chip:	gpio chip associated with the domain
  * @data;	platform data for the domain
  * @node:	device tree node for the domain
+ * @pinctrl:	pinctrl struct associated with the domain
  *
  * A domain represents a set of banks controlled by the same set of
  * registers.
@@ -134,6 +140,7 @@ struct meson_domain {
 	struct gpio_chip chip;
 	struct meson_domain_data *data;
 	struct device_node *of_node;
+	struct meson_pinctrl *pinctrl;
 };
 
 struct meson_pinctrl_data {
@@ -145,6 +152,7 @@ struct meson_pinctrl_data {
 	unsigned int num_groups;
 	unsigned int num_funcs;
 	unsigned int num_domains;
+	unsigned int last_pin;
 };
 
 struct meson_pinctrl {
@@ -153,6 +161,13 @@ struct meson_pinctrl {
 	struct pinctrl_desc desc;
 	struct meson_pinctrl_data *data;
 	struct meson_domain *domains;
+
+	struct of_phandle_args *gic_irqs;
+	struct irq_domain *irq_domain;
+	unsigned int num_gic_irqs;
+	unsigned int *irq_map;
+	struct regmap *reg_irq;
+	spinlock_t lock;
 };
 
 #define PIN(x, b)	(b + x)
@@ -192,11 +207,12 @@ 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, per, peb, pr, pb, dr, db, or, ob, ir, ib, i)	\
 	{								\
 		.name	= n,						\
 		.first	= f,						\
 		.last	= l,						\
+		.irq	= i,						\
 		.regs	= {						\
 			[REG_PULLEN]	= { per, peb },			\
 			[REG_PULL]	= { pr, pb },			\
diff --git a/drivers/pinctrl/meson/pinctrl-meson8.c b/drivers/pinctrl/meson/pinctrl-meson8.c
index 7b1cc91..d941568 100644
--- a/drivers/pinctrl/meson/pinctrl-meson8.c
+++ b/drivers/pinctrl/meson/pinctrl-meson8.c
@@ -14,7 +14,12 @@
 #include <dt-bindings/gpio/meson8-gpio.h>
 #include "pinctrl-meson.h"
 
-#define AO_OFF	120
+#define EE_BASE		0
+#define EE_NPINS	120
+#define AO_BASE		120
+#define AO_NPINS	16
+
+#define AO_OFF		AO_BASE
 
 static const struct pinctrl_pin_desc meson8_pins[] = {
 	MESON_PIN(GPIOX_0, 0),
@@ -907,19 +912,19 @@ static struct meson_pmx_func meson8_functions[] = {
 };
 
 static struct meson_bank meson8_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                 pullen  pull    dir     out     in     irq */
+	BANK("X",    PIN(GPIOX_0, 0),  PIN(GPIOX_21, 0),    4,  0,  4,  0,  0,  0,  1,  0,  2,  0, 112),
+	BANK("Y",    PIN(GPIOY_0, 0),  PIN(GPIOY_16, 0),    3,  0,  3,  0,  3,  0,  4,  0,  5,  0,  95),
+	BANK("DV",   PIN(GPIODV_0, 0), PIN(GPIODV_29, 0),   0,  0,  0,  0,  7,  0,  8,  0,  9,  0,  65),
+	BANK("H",    PIN(GPIOH_0, 0),  PIN(GPIOH_9, 0),     1, 16,  1, 16,  9, 19, 10, 19, 11, 19,  29),
+	BANK("Z",    PIN(GPIOZ_0, 0),  PIN(GPIOZ_14, 0),    1,  0,  1,  0,  3, 17,  4, 17,  5, 17,  14),
+	BANK("CARD", PIN(CARD_0, 0),   PIN(CARD_6, 0),      2, 20,  2, 20,  0, 22,  1, 22,  2, 22,  58),
+	BANK("BOOT", PIN(BOOT_0, 0),   PIN(BOOT_18, 0),     2,  0,  2,  0,  9,  0, 10,  0, 11,  0,  39),
 };
 
 static struct meson_bank meson8_ao_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                      pullen  pull    dir     out     in     irq */
+	BANK("AO",   PIN(GPIOAO_0, AO_OFF), PIN(GPIO_TEST_N, AO_OFF), 0,  0,  0, 16,  0,  0,  0, 16,  1,  0,  0),
 };
 
 static struct meson_domain_data meson8_domain_data[] = {
@@ -927,15 +932,15 @@ static struct meson_domain_data meson8_domain_data[] = {
 		.name		= "banks",
 		.banks		= meson8_banks,
 		.num_banks	= ARRAY_SIZE(meson8_banks),
-		.pin_base	= 0,
-		.num_pins	= 120,
+		.pin_base	= EE_BASE,
+		.num_pins	= EE_NPINS,
 	},
 	{
 		.name		= "ao-bank",
 		.banks		= meson8_ao_banks,
 		.num_banks	= ARRAY_SIZE(meson8_ao_banks),
-		.pin_base	= 120,
-		.num_pins	= 16,
+		.pin_base	= AO_BASE,
+		.num_pins	= AO_NPINS,
 	},
 };
 
@@ -948,4 +953,5 @@ struct meson_pinctrl_data meson8_pinctrl_data = {
 	.num_groups	= ARRAY_SIZE(meson8_groups),
 	.num_funcs	= ARRAY_SIZE(meson8_functions),
 	.num_domains	= ARRAY_SIZE(meson8_domain_data),
+	.last_pin	= EE_NPINS + AO_NPINS,
 };
diff --git a/drivers/pinctrl/meson/pinctrl-meson8b.c b/drivers/pinctrl/meson/pinctrl-meson8b.c
index 9677807..c921ae3 100644
--- a/drivers/pinctrl/meson/pinctrl-meson8b.c
+++ b/drivers/pinctrl/meson/pinctrl-meson8b.c
@@ -15,7 +15,12 @@
 #include <dt-bindings/gpio/meson8b-gpio.h>
 #include "pinctrl-meson.h"
 
-#define AO_OFF	130
+#define EE_BASE		0
+#define EE_NPINS	130
+#define AO_BASE		130
+#define AO_NPINS	16
+
+#define AO_OFF		AO_BASE
 
 static const struct pinctrl_pin_desc meson8b_pins[] = {
 	MESON_PIN(GPIOX_0, 0),
@@ -855,19 +860,19 @@ static struct meson_pmx_func meson8b_functions[] = {
 };
 
 static struct meson_bank meson8b_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                   pullen  pull    dir     out     in     irq */
+	BANK("X",    PIN(GPIOX_0, 0),		PIN(GPIOX_21, 0),      4,  0,  4,  0,  0,  0,  1,  0,  2,  0,  97),
+	BANK("Y",    PIN(GPIOY_0, 0),		PIN(GPIOY_14, 0),      3,  0,  3,  0,  3,  0,  4,  0,  5,  0,  80),
+	BANK("DV",   PIN(GPIODV_9, 0),		PIN(GPIODV_29, 0),     0,  0,  0,  0,  7,  0,  8,  0,  9,  0,  59),
+	BANK("H",    PIN(GPIOH_0, 0),		PIN(GPIOH_9, 0),       1, 16,  1, 16,  9, 19, 10, 19, 11, 19,  14),
+	BANK("CARD", PIN(CARD_0, 0),		PIN(CARD_6, 0),        2, 20,  2, 20,  0, 22,  1, 22,  2, 22,  43),
+	BANK("BOOT", PIN(BOOT_0, 0),		PIN(BOOT_18, 0),       2,  0,  2,  0,  9,  0, 10,  0, 11,  0,  24),
+	BANK("DIF",  PIN(DIF_0_P, 0),		PIN(DIF_4_N, 0),       5,  8,  5,  8, 12, 12, 13, 12, 14, 12, 119),
 };
 
 static struct meson_bank meson8b_ao_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                      pullen  pull    dir     out     in    irq */
+	BANK("AO",   PIN(GPIOAO_0, AO_OFF), PIN(GPIO_TEST_N, AO_OFF), 0,  0,  0, 16,  0,  0,  0, 16,  1,  0,  0),
 };
 
 static struct meson_domain_data meson8b_domain_data[] = {
@@ -875,15 +880,15 @@ static struct meson_domain_data meson8b_domain_data[] = {
 		.name		= "banks",
 		.banks		= meson8b_banks,
 		.num_banks	= ARRAY_SIZE(meson8b_banks),
-		.pin_base	= 0,
-		.num_pins	= 130,
+		.pin_base	= EE_BASE,
+		.num_pins	= EE_NPINS,
 	},
 	{
 		.name		= "ao-bank",
 		.banks		= meson8b_ao_banks,
 		.num_banks	= ARRAY_SIZE(meson8b_ao_banks),
-		.pin_base	= 130,
-		.num_pins	= 16,
+		.pin_base	= AO_BASE,
+		.num_pins	= AO_NPINS,
 	},
 };
 
@@ -896,4 +901,5 @@ struct meson_pinctrl_data meson8b_pinctrl_data = {
 	.num_groups	= ARRAY_SIZE(meson8b_groups),
 	.num_funcs	= ARRAY_SIZE(meson8b_functions),
 	.num_domains	= ARRAY_SIZE(meson8b_domain_data),
+	.last_pin	= EE_NPINS + AO_NPINS,
 };
-- 
1.9.1

[PATCH 1/2] pinctrl: meson: Enable GPIO IRQs

[Linus Walleij]

On Mon, May 25, 2015 at 1:00 PM, Carlo Caione <carlo@caione.org> wrote:

> From: Beniamino Galvani <b.galvani@gmail.com>
>
> On Meson8 and Meson8b SoCs there are 8 independent filtered GPIO
> interrupt modules that can be programmed to use any of the GPIOs in the
> chip as an interrupt source.
>
> For each GPIO IRQ we have:
>
> GPIOs --> [mux]--> [polarity]--> [filter]--> [edge select]--> GIC
>
> The eight GPIO interrupts respond to mask/unmask/clear/etc.. just like
> any other interrupt in the chip. The difference for the GPIO interrupts
> is that they can be filtered and conditioned.
>
> This patch adds support for the external GPIOs interrupts and enables
> them for Meson8 and Meson8b SoCs.
>
> Signed-off-by: Carlo Caione <carlo@endlessm.com>
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>

Sigh this is gonna be one of these drivers that cannot use
GPIOLIB_IRQCHIP because it has the custom awkward
interrupt generator thing. I guess it is a bit faster to have
a limited number of direct IRQ lines to the CPU though so
it does come with some advantages...

> +static int meson_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct meson_domain *domain = to_meson_domain(chip);
> +       struct meson_pinctrl *pc = domain->pinctrl;
> +       struct meson_bank *bank;
> +       struct of_phandle_args irq_data;
> +       unsigned int pin, virq;

Don't call it "virq" these are Linux irqs they are not any more
virtual than any other IRQs. Call the hardware irq (pin?) hwirq
instead.

> +       int ret;
> +
> +       pin = domain->data->pin_base + offset;

This is not looking good. Nominally you should use the irqdomain to
translate hwirq to Linux IRQ.

Normally this is just

line = irq_find_mapping(domain, hwirq);

> +       ret = meson_get_bank(domain, pin, &bank);
> +       if (ret)
> +               return -ENXIO;
> +
> +       irq_data.args_count = 2;
> +       irq_data.args[0] = pin;
> +       virq = irq_domain_alloc_irqs(pc->irq_domain, 1, NUMA_NO_NODE, &irq_data);

This is wrong. You have to alloc the irqs either

(A) when the driver is probed, looping over all IRQs.
  Then pair with free():in the IRQs in the remove()
  call.

or

(B) in one of the irqchip functions like .irq_request_resources()
  then pair with freeing the IRQs in .irq_release_resources().

Reason: the irqchip API can be used orthogonally from
the gpiolib API, and there is no gurantee *at all* that consumers
will call .to_irq() before using an IRQ. Especially not in the
device tree case.

All set-up/tear-down needs to happen without relying
on .to_irq() to have been called first.

> +       return virq ? virq : -ENXIO;
> +}
> +
(...)

> +static int meson_get_gic_irq(struct meson_pinctrl *pc, int hwirq)
> +{
> +       int i = 0;
> +
> +       for (i = 0; i < pc->num_gic_irqs; i++) {
> +               if (pc->irq_map[i] == hwirq)
> +                       return i;
> +       }
> +
> +       return -1;
> +}

Looks a bit like doing irqdomains work, but I guess it's
not possible any other way, since the GIC already
has its own irqdomain.

> +static struct irq_chip meson_irq_chip = {
> +       .name                   = "meson-gpio-irqchip",
> +       .irq_mask               = irq_chip_mask_parent,
> +       .irq_unmask             = irq_chip_unmask_parent,
> +       .irq_eoi                = irq_chip_eoi_parent,
> +       .irq_set_type           = meson_irq_set_type,
> +       .irq_retrigger          = irq_chip_retrigger_hierarchy,
> +       .irq_set_affinity       = irq_chip_set_affinity_parent,

You need to add .irq_request_resources() and
.irq_release_resources() with this kind of content at
least:

static int gpiochip_irq_reqres(struct irq_data *d)
{
        struct meson_pinctrl *pc = data->chip_data;

(...)
        if (gpiochip_lock_as_irq(chip, d->hwirq))
                return -EINVAL;
        return 0;
}


static void gpiochip_irq_relres(struct irq_data *d)
{
        struct meson_pinctrl *pc = data->chip_data;

(..)
        gpiochip_unlock_as_irq(chip, d->hwirq);
}


> +static int meson_map_gic_irq(struct irq_domain *irq_domain,
> +                            irq_hw_number_t hwirq)
> +{
> +       struct meson_pinctrl *pc = irq_domain->host_data;
> +       struct meson_domain *domain;
> +       struct meson_bank *bank;
> +       int index, reg, ret;
> +
> +       ret = meson_get_domain_and_bank(pc, hwirq, &domain, &bank);
> +       if (ret)
> +               return ret;
> +
> +       spin_lock(&pc->lock);
> +
> +       index = meson_get_gic_irq(pc, IRQ_FREE);
> +       if (index < 0) {
> +               spin_unlock(&pc->lock);
> +               dev_err(pc->dev, "no free GIC interrupt found");
> +               return -ENOSPC;
> +       }

That's neat, better not run out of IRQs here.
Yeah they designed the hardware like so. ...

> +       dev_dbg(pc->dev, "found free GIC interrupt %d\n", index);
> +       pc->irq_map[index] = hwirq;

Again this is irqdomain territory.

> +static int meson_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +                                 unsigned int nr_irqs, void *arg)

No virq please. Just irq. It is a Linux IRQ, not a virtual IRQ.

> +{
> +       struct meson_pinctrl *pc = domain->host_data;
> +       struct of_phandle_args *irq_data = arg;
> +       struct of_phandle_args gic_data;
> +       irq_hw_number_t hwirq;
> +       int index, ret, i;
> +
> +       if (irq_data->args_count != 2)
> +               return -EINVAL;
> +
> +       hwirq = irq_data->args[0];
> +       dev_dbg(pc->dev, "%s virq %d, nr %d, hwirq %lu\n",
> +               __func__, virq, nr_irqs, hwirq);
> +
> +       for (i = 0; i < nr_irqs; i++) {
> +               index = meson_map_gic_irq(domain, hwirq + i);
> +               if (index < 0)
> +                       return index;
> +
> +               irq_domain_set_hwirq_and_chip(domain, virq + i,
> +                                             hwirq + i,
> +                                             &meson_irq_chip,
> +                                             pc);
> +
> +               gic_data = pc->gic_irqs[index];
> +               ret = irq_domain_alloc_irqs_parent(domain, virq + i, nr_irqs,
> +                                                  &gic_data);
> +       }

OK... hm quite complex.

> +static void meson_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> +                                 unsigned int nr_irqs)

virq -> irq

> +{
> +       struct meson_pinctrl *pc = domain->host_data;
> +       struct irq_data *irq_data;
> +       int index, i;
> +
> +       spin_lock(&pc->lock);
> +       for (i = 0; i < nr_irqs; i++) {
> +               irq_data = irq_domain_get_irq_data(domain, virq + i);
> +               index = meson_get_gic_irq(pc, irq_data->hwirq);
> +               if (index < 0)
> +                       continue;
> +               pc->irq_map[index] = IRQ_FREE;

Don't you need to call irq_dispose_mapping() for all
IRQs here? Or is the irq_domain_free_irqs_paren()
taking care of this?

Sorry if I don't fully understand this. It is a quite complex
interrupt generator logic :(

Yours,
Linus Walleij

[PATCH 1/2] pinctrl: meson: Enable GPIO IRQs

[Carlo Caione]

On Mon, Jun 1, 2015 at 4:30 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, May 25, 2015 at 1:00 PM, Carlo Caione <carlo@caione.org> wrote:
>
>> From: Beniamino Galvani <b.galvani@gmail.com>
>>
>> On Meson8 and Meson8b SoCs there are 8 independent filtered GPIO
>> interrupt modules that can be programmed to use any of the GPIOs in the
>> chip as an interrupt source.
>>
>> For each GPIO IRQ we have:
>>
>> GPIOs --> [mux]--> [polarity]--> [filter]--> [edge select]--> GIC
>>
>> The eight GPIO interrupts respond to mask/unmask/clear/etc.. just like
>> any other interrupt in the chip. The difference for the GPIO interrupts
>> is that they can be filtered and conditioned.
>>
>> This patch adds support for the external GPIOs interrupts and enables
>> them for Meson8 and Meson8b SoCs.
>>
>> Signed-off-by: Carlo Caione <carlo@endlessm.com>
>> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
>
> Sigh this is gonna be one of these drivers that cannot use
> GPIOLIB_IRQCHIP because it has the custom awkward
> interrupt generator thing. I guess it is a bit faster to have
> a limited number of direct IRQ lines to the CPU though so
> it does come with some advantages...

Yeah. Also this is probably the first pinctrl driver using the
hierarchy IRQ domain so there is still some piece missing with a lot
of trial and error.

>> +static int meson_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>> +{
>> +       struct meson_domain *domain = to_meson_domain(chip);
>> +       struct meson_pinctrl *pc = domain->pinctrl;
>> +       struct meson_bank *bank;
>> +       struct of_phandle_args irq_data;
>> +       unsigned int pin, virq;
>
> Don't call it "virq" these are Linux irqs they are not any more
> virtual than any other IRQs. Call the hardware irq (pin?) hwirq
> instead.

Ok.

>> +       int ret;
>> +
>> +       pin = domain->data->pin_base + offset;
>
> This is not looking good. Nominally you should use the irqdomain to
> translate hwirq to Linux IRQ.
>
> Normally this is just
>
> line = irq_find_mapping(domain, hwirq);

The problem I see with using irq_find_mapping() here is that at this
point we do not have yet the mapping hwirq->(v)irq. Other drivers (not
using the hierarchical IRQ domain) use irq_create_mapping() to create
the mapping in the .to_irq() hook and IIUC for hierarchical domains we
cannot use irq_create_mapping().

The point here seems to be that to allocate the IRQs on the GIC we
need to call the .alloc() of the struct irq_domain_ops. This is not
usually called by irq_create_mapping() but we need to use
irq_domain_alloc_irqs(). In irq_create_of_mapping() for example we
deal with hierarchical domains and IRQs defined in the DTS using
directly irq_domain_alloc_irqs() as I did in the following lines of
code.

So when the GPIO IRQ is defined in the DTS everything works fine since
irq_create_of_mapping() takes care of everything. The problem is when
the GPIO IRQ is requested directly from the code or from another
subsystem (i.e. MMC) since this request goes through .to_irq().

>> +       ret = meson_get_bank(domain, pin, &bank);
>> +       if (ret)
>> +               return -ENXIO;
>> +
>> +       irq_data.args_count = 2;
>> +       irq_data.args[0] = pin;
>> +       virq = irq_domain_alloc_irqs(pc->irq_domain, 1, NUMA_NO_NODE, &irq_data);
>
> This is wrong. You have to alloc the irqs either
>
> (A) when the driver is probed, looping over all IRQs.
>   Then pair with free():in the IRQs in the remove()
>   call.
>
> or
>
> (B) in one of the irqchip functions like .irq_request_resources()
>   then pair with freeing the IRQs in .irq_release_resources().

Correct me if I'm wrong but in my experiments the
.irq_request_resources() seems to be called after the .to_irq() hook.

> Reason: the irqchip API can be used orthogonally from
> the gpiolib API, and there is no gurantee *at all* that consumers
> will call .to_irq() before using an IRQ. Especially not in the
> device tree case.

This is exactly my understanding: a GPIO IRQs can be requested to this
driver in two different ways: (1) device tree or (2) gpiolib API (for
example in my case a GPIO IRQ is requested by the MMC subsystem for
card detection).
In case (1) irq_create_of_mapping() manages the hierarchical domain
just fine. The problem is for case (2) where the .to_irq() hook is
used since as said before here apparently I cannot use
irq_create_mapping().

> All set-up/tear-down needs to happen without relying
> on .to_irq() to have been called first.

Agree.

>> +       return virq ? virq : -ENXIO;
>> +}
>> +
> (...)
>

(...)

>> +{
>> +       struct meson_pinctrl *pc = domain->host_data;
>> +       struct of_phandle_args *irq_data = arg;
>> +       struct of_phandle_args gic_data;
>> +       irq_hw_number_t hwirq;
>> +       int index, ret, i;
>> +
>> +       if (irq_data->args_count != 2)
>> +               return -EINVAL;
>> +
>> +       hwirq = irq_data->args[0];
>> +       dev_dbg(pc->dev, "%s virq %d, nr %d, hwirq %lu\n",
>> +               __func__, virq, nr_irqs, hwirq);
>> +
>> +       for (i = 0; i < nr_irqs; i++) {
>> +               index = meson_map_gic_irq(domain, hwirq + i);
>> +               if (index < 0)
>> +                       return index;
>> +
>> +               irq_domain_set_hwirq_and_chip(domain, virq + i,
>> +                                             hwirq + i,
>> +                                             &meson_irq_chip,
>> +                                             pc);
>> +
>> +               gic_data = pc->gic_irqs[index];
>> +               ret = irq_domain_alloc_irqs_parent(domain, virq + i, nr_irqs,
>> +                                                  &gic_data);
>> +       }
>
> OK... hm quite complex.

Fortunately also quite standard for hierarchical IRQ domains.

>> +static void meson_irq_domain_free(struct irq_domain *domain, unsigned int virq,
>> +                                 unsigned int nr_irqs)
>
> virq -> irq

Ok

>> +{
>> +       struct meson_pinctrl *pc = domain->host_data;
>> +       struct irq_data *irq_data;
>> +       int index, i;
>> +
>> +       spin_lock(&pc->lock);
>> +       for (i = 0; i < nr_irqs; i++) {
>> +               irq_data = irq_domain_get_irq_data(domain, virq + i);
>> +               index = meson_get_gic_irq(pc, irq_data->hwirq);
>> +               if (index < 0)
>> +                       continue;
>> +               pc->irq_map[index] = IRQ_FREE;
>
> Don't you need to call irq_dispose_mapping() for all
> IRQs here? Or is the irq_domain_free_irqs_paren()
> taking care of this?

Yes. irq_domain_free_irqs_parent() takes care of everything.

> Sorry if I don't fully understand this. It is a quite complex
> interrupt generator logic :(

Totally agree. It took a while to me to grasp some of the concepts
here and still I'm missing the big picture.

Thanks for the review,

-- 
Carlo Caione

[PATCH 1/2] pinctrl: meson: Enable GPIO IRQs

[Linus Walleij]

On Tue, Jun 2, 2015 at 11:33 AM, Carlo Caione <carlo@caione.org> wrote:
> On Mon, Jun 1, 2015 at 4:30 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> Yeah. Also this is probably the first pinctrl driver using the
> hierarchy IRQ domain so there is still some piece missing with a lot
> of trial and error.

I actually want to augment all the GPIOLIB_IRQCHIP-using drivers
to use hierarchical IRQ domain, as they are all essentially cascaded
IRQ controllers.

The problem I face is getting the irqdomain of the parent. Simple
in the device tree case, but need to support all cases, so
I may have to look into adding some function that retrieves the
irqdomain for a Linux irq number or descriptor.

>> This is not looking good. Nominally you should use the irqdomain to
>> translate hwirq to Linux IRQ.
>>
>> Normally this is just
>>
>> line = irq_find_mapping(domain, hwirq);
>
> The problem I see with using irq_find_mapping() here is that at this
> point we do not have yet the mapping hwirq->(v)irq. Other drivers (not
> using the hierarchical IRQ domain) use irq_create_mapping() to create
> the mapping in the .to_irq() hook

Any other drivers doing irq_create_mapping() in .to_irq() are old
buggy as they do not support orthogonal gpio/irqchip
coexistance. Look at recent drivers.

> and IIUC for hierarchical domains we
> cannot use irq_create_mapping().

OK that is a problem I may not understand correctly ...
yeah it is referring to the parent IRQ rather than translating,
OK then I guess. Have to look at the code again in v2.

> The point here seems to be that to allocate the IRQs on the GIC we
> need to call the .alloc() of the struct irq_domain_ops. This is not
> usually called by irq_create_mapping() but we need to use
> irq_domain_alloc_irqs(). In irq_create_of_mapping() for example we
> deal with hierarchical domains and IRQs defined in the DTS using
> directly irq_domain_alloc_irqs() as I did in the following lines of
> code.

OK I kind of get it.

I now have a few GPIO drivers using parent IRQs, and they
should all use the hierarchy feature I guess. I'm thinking
about whether we can centralize stuff to gpiolib but maybe
not :/

> So when the GPIO IRQ is defined in the DTS everything works fine since
> irq_create_of_mapping() takes care of everything. The problem is when
> the GPIO IRQ is requested directly from the code or from another
> subsystem (i.e. MMC) since this request goes through .to_irq().

Yeah I get it. I think. Just a bit confused still.

>>> +       ret = meson_get_bank(domain, pin, &bank);
>>> +       if (ret)
>>> +               return -ENXIO;
>>> +
>>> +       irq_data.args_count = 2;
>>> +       irq_data.args[0] = pin;
>>> +       virq = irq_domain_alloc_irqs(pc->irq_domain, 1, NUMA_NO_NODE, &irq_data);
>>
>> This is wrong. You have to alloc the irqs either
>>
>> (A) when the driver is probed, looping over all IRQs.
>>   Then pair with free():in the IRQs in the remove()
>>   call.
>>
>> or
>>
>> (B) in one of the irqchip functions like .irq_request_resources()
>>   then pair with freeing the IRQs in .irq_release_resources().
>
> Correct me if I'm wrong but in my experiments the
> .irq_request_resources() seems to be called after the .to_irq()
> hook.

Hmmm OK... keep that part here then.

>>> +       struct meson_pinctrl *pc = domain->host_data;
>>> +       struct of_phandle_args *irq_data = arg;
>>> +       struct of_phandle_args gic_data;
>>> +       irq_hw_number_t hwirq;
>>> +       int index, ret, i;
>>> +
>>> +       if (irq_data->args_count != 2)
>>> +               return -EINVAL;
>>> +
>>> +       hwirq = irq_data->args[0];
>>> +       dev_dbg(pc->dev, "%s virq %d, nr %d, hwirq %lu\n",
>>> +               __func__, virq, nr_irqs, hwirq);
>>> +
>>> +       for (i = 0; i < nr_irqs; i++) {
>>> +               index = meson_map_gic_irq(domain, hwirq + i);
>>> +               if (index < 0)
>>> +                       return index;
>>> +
>>> +               irq_domain_set_hwirq_and_chip(domain, virq + i,
>>> +                                             hwirq + i,
>>> +                                             &meson_irq_chip,
>>> +                                             pc);
>>> +
>>> +               gic_data = pc->gic_irqs[index];
>>> +               ret = irq_domain_alloc_irqs_parent(domain, virq + i, nr_irqs,
>>> +                                                  &gic_data);
>>> +       }
>>
>> OK... hm quite complex.
>
> Fortunately also quite standard for hierarchical IRQ domains.

Thinking about trying to centralize this for all GPIO chips...

>>> +{
>>> +       struct meson_pinctrl *pc = domain->host_data;
>>> +       struct irq_data *irq_data;
>>> +       int index, i;
>>> +
>>> +       spin_lock(&pc->lock);
>>> +       for (i = 0; i < nr_irqs; i++) {
>>> +               irq_data = irq_domain_get_irq_data(domain, virq + i);
>>> +               index = meson_get_gic_irq(pc, irq_data->hwirq);
>>> +               if (index < 0)
>>> +                       continue;
>>> +               pc->irq_map[index] = IRQ_FREE;
>>
>> Don't you need to call irq_dispose_mapping() for all
>> IRQs here? Or is the irq_domain_free_irqs_paren()
>> taking care of this?
>
> Yes. irq_domain_free_irqs_parent() takes care of everything.

OK.

Yours,
Linus Walleij

[PATCH 1/2] pinctrl: meson: Enable GPIO IRQs

[Carlo Caione]

On Mon, Jun 1, 2015 at 4:30 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, May 25, 2015 at 1:00 PM, Carlo Caione <carlo@caione.org> wrote:

Hey Linus,
I was starting to write the v2, but I still have a couple of (probably
silly) questions.

>> +       int ret;
>> +
>> +       pin = domain->data->pin_base + offset;
>
> This is not looking good. Nominally you should use the irqdomain to
> translate hwirq to Linux IRQ.
>
> Normally this is just
>
> line = irq_find_mapping(domain, hwirq);

Ok, it sounds reasonable but this implies that the mapping between the
virq and the hwirq in the outermost domain already exists when the
.to_irq hook is called, right? Also IIUC for hierarchical domains the
mapping should also exist on all the irq_domains in the hierarchy.

>> +       ret = meson_get_bank(domain, pin, &bank);
>> +       if (ret)
>> +               return -ENXIO;
>> +
>> +       irq_data.args_count = 2;
>> +       irq_data.args[0] = pin;
>> +       virq = irq_domain_alloc_irqs(pc->irq_domain, 1, NUMA_NO_NODE, &irq_data);
>
> This is wrong. You have to alloc the irqs either
>
> (A) when the driver is probed, looping over all IRQs.
>   Then pair with free():in the IRQs in the remove()
>   call.

This is not really clear to me. Are you suggesting that the mapping
between the hwirq and virq should be done at probe time so that we can
use irq_find_mapping later?
IIUC for the hierarchical domains the mapping creation should be
propagated to all the domains in cascade and this is usually done
using the .alloc hook of the irq_domain_ops and at probe time we do
not still have the hwirq to pass to the parent GIC. Any idea on how to
approach this problem?

Cheers,

-- 
Carlo Caione

[PATCH 1/2] pinctrl: meson: Enable GPIO IRQs

[Linus Walleij]

On Sat, Jun 13, 2015 at 5:35 PM, Carlo Caione <carlo@caione.org> wrote:
> On Mon, Jun 1, 2015 at 4:30 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, May 25, 2015 at 1:00 PM, Carlo Caione <carlo@caione.org> wrote:
>
> Hey Linus,
> I was starting to write the v2, but I still have a couple of (probably
> silly) questions.
>
>>> +       int ret;
>>> +
>>> +       pin = domain->data->pin_base + offset;
>>
>> This is not looking good. Nominally you should use the irqdomain to
>> translate hwirq to Linux IRQ.
>>
>> Normally this is just
>>
>> line = irq_find_mapping(domain, hwirq);
>
> Ok, it sounds reasonable but this implies that the mapping between the
> virq and the hwirq in the outermost domain already exists when the
> .to_irq hook is called, right? Also IIUC for hierarchical domains the
> mapping should also exist on all the irq_domains in the hierarchy.

I guess, I am no expert in the hierarchical domains, sadly.

The point is that it should be possible to request an IRQ
from the irqchip side without having to have called .to_irq()
on the GPIO first.

>> (A) when the driver is probed, looping over all IRQs.
>>   Then pair with free():in the IRQs in the remove()
>>   call.
>
> This is not really clear to me. Are you suggesting that the mapping
> between the hwirq and virq should be done at probe time so that we can
> use irq_find_mapping later?

Yes.

> IIUC for the hierarchical domains the mapping creation should be
> propagated to all the domains in cascade and this is usually done
> using the .alloc hook of the irq_domain_ops and at probe time we do
> not still have the hwirq to pass to the parent GIC. Any idea on how to
> approach this problem?

I guess it needs to be done in some other hook on the
irqchip in that case. Just not in .to_irq() on the gpiochip.

Yours,
Linus Walleij

[PATCH 1/2] pinctrl: meson: Enable GPIO IRQs

[Carlo Caione]

On Wed, Jun 17, 2015 at 10:26 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Sat, Jun 13, 2015 at 5:35 PM, Carlo Caione <carlo@caione.org> wrote:
>> On Mon, Jun 1, 2015 at 4:30 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
...
>> Ok, it sounds reasonable but this implies that the mapping between the
>> virq and the hwirq in the outermost domain already exists when the
>> .to_irq hook is called, right? Also IIUC for hierarchical domains the
>> mapping should also exist on all the irq_domains in the hierarchy.
>
> I guess, I am no expert in the hierarchical domains, sadly.
>
> The point is that it should be possible to request an IRQ
> from the irqchip side without having to have called .to_irq()
> on the GPIO first.

Ok. Let me rephrase that to be sure we are on the same track. It
should be possible to request an IRQ without having to have called
.to_irq() on the GPIO first to allocate the IRQ descriptor.
Ok but when is this the case? If our IRQ line is in the DT, then
irq_create_of_mapping() takes care of allocating the descriptor. Now,
the only other case I know of is for MMC/slot-gpio. In this case
mmc_gpiod_request_cd_irq() only calls .to_irq() before requesting the
IRQ so there is the occasion to allocate the IRQ descriptor (that's
why probably a lot of drivers allocate the irq descriptor in there
using irq_create_mapping()).

>> IIUC for the hierarchical domains the mapping creation should be
>> propagated to all the domains in cascade and this is usually done
>> using the .alloc hook of the irq_domain_ops and at probe time we do
>> not still have the hwirq to pass to the parent GIC. Any idea on how to
>> approach this problem?
>
> I guess it needs to be done in some other hook on the
> irqchip in that case. Just not in .to_irq() on the gpiochip.

You mentioned .irq_request_resources(), but isn't it called too late
for IRQ allocation?

Thank you,

-- 
Carlo Caione

[PATCH 1/2] pinctrl: meson: Enable GPIO IRQs

[Linus Walleij]

On Mon, Jun 22, 2015 at 7:33 PM, Carlo Caione <carlo@caione.org> wrote:
> On Wed, Jun 17, 2015 at 10:26 AM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
>> On Sat, Jun 13, 2015 at 5:35 PM, Carlo Caione <carlo@caione.org> wrote:
>>> On Mon, Jun 1, 2015 at 4:30 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> ...
>>> Ok, it sounds reasonable but this implies that the mapping between the
>>> virq and the hwirq in the outermost domain already exists when the
>>> .to_irq hook is called, right? Also IIUC for hierarchical domains the
>>> mapping should also exist on all the irq_domains in the hierarchy.
>>
>> I guess, I am no expert in the hierarchical domains, sadly.
>>
>> The point is that it should be possible to request an IRQ
>> from the irqchip side without having to have called .to_irq()
>> on the GPIO first.
>
> Ok. Let me rephrase that to be sure we are on the same track. It
> should be possible to request an IRQ without having to have called
> .to_irq() on the GPIO first to allocate the IRQ descriptor.
> Ok but when is this the case? If our IRQ line is in the DT, then
> irq_create_of_mapping() takes care of allocating the descriptor. Now,
> the only other case I know of is for MMC/slot-gpio. In this case
> mmc_gpiod_request_cd_irq() only calls .to_irq() before requesting the
> IRQ so there is the occasion to allocate the IRQ descriptor (that's
> why probably a lot of drivers allocate the irq descriptor in there
> using irq_create_mapping()).

I just want to be convinced that code does not rely on .to_irq() being
called to request IRQs from the irqchip side. As long as you can convince
me that this is the case I'm OK.

>>> IIUC for the hierarchical domains the mapping creation should be
>>> propagated to all the domains in cascade and this is usually done
>>> using the .alloc hook of the irq_domain_ops and at probe time we do
>>> not still have the hwirq to pass to the parent GIC. Any idea on how to
>>> approach this problem?
>>
>> I guess it needs to be done in some other hook on the
>> irqchip in that case. Just not in .to_irq() on the gpiochip.
>
> You mentioned .irq_request_resources(), but isn't it called too late
> for IRQ allocation?

Maybe yeah. I feel a bit lost here :(

It would be nice to have a Reviewed-by from some IRQ people on
this.

Yours,
Linus Walleij

[PATCH 2/2] pinctrl: dt-binding: Extend meson documentation with GPIO IRQs support

[Carlo Caione]

From: Beniamino Galvani <b.galvani@gmail.com>

Extend the pinctrl binding documentation with the support for external
GPIO interrupts.

Signed-off-by: Carlo Caione <carlo@endlessm.com>
Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
index 3f6a524..56743eb 100644
--- a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
@@ -3,6 +3,12 @@
 Required properties for the root node:
  - compatible: "amlogic,meson8-pinctrl" or "amlogic,meson8b-pinctrl"
  - reg: address and size of registers controlling irq functionality
+ - reg-names: should be "irq"
+ - interrupt-controller: marks the device node as an interrupt controller
+ - #interrupt-cells: should be 2. The first cell is the GPIO number. The
+		     second cell is used to specify trigger type.
+ - interrupts: specifies the GPIO IRQ numbers on the GIC (GPIO_IRQ#)
+ - interrupt-parent: specifies the parent interrupt controller.
 
 === GPIO sub-nodes ===
 
@@ -46,7 +52,13 @@ pinctrl-bindings.txt
 
 	pinctrl: pinctrl at c1109880 {
 		compatible = "amlogic,meson8-pinctrl";
+		reg-names = "irq";
 		reg = <0xc1109880 0x10>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 64 1>, <0 65 1>, <0 66 1>, <0 67 1>,
+			     <0 68 1>, <0 69 1>, <0 70 1>, <0 71 1>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges;
-- 
1.9.1

[PATCH 2/2] pinctrl: dt-binding: Extend meson documentation with GPIO IRQs support

[Linus Walleij]

On Mon, May 25, 2015 at 1:00 PM, Carlo Caione <carlo@caione.org> wrote:

> From: Beniamino Galvani <b.galvani@gmail.com>
>
> Extend the pinctrl binding documentation with the support for external
> GPIO interrupts.
>
> Signed-off-by: Carlo Caione <carlo@endlessm.com>
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
(...)
> + - interrupts: specifies the GPIO IRQ numbers on the GIC (GPIO_IRQ#)

Write that this GPIO only support as many IRQs as you supply
to the controller.

>         pinctrl: pinctrl at c1109880 {
>                 compatible = "amlogic,meson8-pinctrl";
> +               reg-names = "irq";

Why? I thought this was the registers for the whole pin controller.

Yours,
Linus Walleij

[PATCH 2/2] pinctrl: dt-binding: Extend meson documentation with GPIO IRQs support

[Carlo Caione]

On Mon, Jun 1, 2015 at 4:04 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, May 25, 2015 at 1:00 PM, Carlo Caione <carlo@caione.org> wrote:
>
>> From: Beniamino Galvani <b.galvani@gmail.com>
>>
>> Extend the pinctrl binding documentation with the support for external
>> GPIO interrupts.
>>
>> Signed-off-by: Carlo Caione <carlo@endlessm.com>
>> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> (...)
>> + - interrupts: specifies the GPIO IRQ numbers on the GIC (GPIO_IRQ#)
>
> Write that this GPIO only support as many IRQs as you supply
> to the controller.

Ok.

>>         pinctrl: pinctrl at c1109880 {
>>                 compatible = "amlogic,meson8-pinctrl";
>> +               reg-names = "irq";
>
> Why? I thought this was the registers for the whole pin controller.

This register is only used to control the GPIO IRQs. All the muxing
and the basic GPIO control is done using the reg property in the
subnodes (banks).

-- 
Carlo Caione