[PATCH v2 0/5] pinctrl: meson: enable support for external GPIO interrupts

[PATCH v2 0/5] pinctrl: meson: enable support for external GPIO interrupts

[Carlo Caione]

From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>

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

For discussion see comment to the [PATCH 3/5].

Changelog:

* V2:
    - Introduced .irq_request_resources() and .irq_release_resources()
    - s/virq/irq/ and s/pin/hwirq/
    - Moved to the new irq_fwspec

Carlo Caione (5):
  of/irq: export of_irq_find_parent again
  irqdomain: introduce irq_of_phandle_args_to_fwspec
  pinctrl: meson: enable GPIO IRQs
  pinctrl: dt-binding: Extend meson documentation with GPIO IRQs support
  ARM: meson: DTS: Enable GPIO IRQs

 .../devicetree/bindings/pinctrl/meson,pinctrl.txt  |  12 +
 arch/arm/boot/dts/meson8b.dtsi                     |   6 +
 drivers/of/irq.c                                   |   2 +-
 drivers/pinctrl/Kconfig                            |   1 +
 drivers/pinctrl/meson/pinctrl-meson.c              | 303 +++++++++++++++++++++
 drivers/pinctrl/meson/pinctrl-meson.h              |  18 +-
 drivers/pinctrl/meson/pinctrl-meson8.c             |  36 ++-
 drivers/pinctrl/meson/pinctrl-meson8b.c            |  36 ++-
 include/linux/of_irq.h                             |   8 +
 kernel/irq/irqdomain.c                             |   5 +-
 10 files changed, 393 insertions(+), 34 deletions(-)

-- 
2.5.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 v2 1/5] of/irq: export of_irq_find_parent again

[Carlo Caione]

From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>

of_irq_find_parent was made static since it had no users outside of
of_irq.c. Export it again since we are going to use it again.

Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
---
 drivers/of/irq.c       | 2 +-
 include/linux/of_irq.h | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 902b89b..45735d5 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -53,7 +53,7 @@ EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
  * Returns a pointer to the interrupt parent node, or NULL if the interrupt
  * parent could not be determined.
  */
-static struct device_node *of_irq_find_parent(struct device_node *child)
+struct device_node *of_irq_find_parent(struct device_node *child)
 {
 	struct device_node *p;
 	const __be32 *parp;
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 039f2ee..6f879c6 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -94,6 +94,7 @@ static inline void of_msi_configure(struct device *dev, struct device_node *np)
  */
 extern unsigned int irq_of_parse_and_map(struct device_node *node, int index);
 u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in);
+extern struct device_node *of_irq_find_parent(struct device_node *child);
 
 #else /* !CONFIG_OF && !CONFIG_SPARC */
 static inline unsigned int irq_of_parse_and_map(struct device_node *dev,
@@ -107,6 +108,11 @@ static inline u32 of_msi_map_rid(struct device *dev,
 {
 	return rid_in;
 }
+
+static inline void *of_irq_find_parent(struct device_node *child)
+{
+	return NULL;
+}
 #endif /* !CONFIG_OF */
 
 #endif /* __OF_IRQ_H */
-- 
2.5.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

Re: [PATCH v2 1/5] of/irq: export of_irq_find_parent again

[Linus Walleij]

On Mon, Nov 23, 2015 at 11:16 AM, Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:

> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>
> of_irq_find_parent was made static since it had no users outside of
> of_irq.c. Export it again since we are going to use it again.
>
> Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/of/irq.c       | 2 +-
>  include/linux/of_irq.h | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 902b89b..45735d5 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -53,7 +53,7 @@ EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
>   * Returns a pointer to the interrupt parent node, or NULL if the interrupt
>   * parent could not be determined.
>   */
> -static struct device_node *of_irq_find_parent(struct device_node *child)
> +struct device_node *of_irq_find_parent(struct device_node *child)

If you're making this public you should also
EXPORT_SYMBOL_GPL(of_irq_find_parent)

I need an ACK from Rob if this should be merged through
the GPIO tree. If it is going through the OF tree we'll have
to defer the resto of the stuff to the next merge window or
set it up on an immutable branch I can pull.

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 v2 2/5] irqdomain: introduce irq_of_phandle_args_to_fwspec

[Carlo Caione]

From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>

Export of_phandle_args_to_fwspec with a new compliant name.

Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
---
 include/linux/of_irq.h | 2 ++
 kernel/irq/irqdomain.c | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 6f879c6..66bd92e 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -34,6 +34,8 @@ static inline int of_irq_parse_oldworld(struct device_node *device, int index,
 extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq);
 extern int of_irq_parse_one(struct device_node *device, int index,
 			  struct of_phandle_args *out_irq);
+extern void irq_of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
+					  struct irq_fwspec *fwspec);
 extern unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data);
 extern int of_irq_to_resource(struct device_node *dev, int index,
 			      struct resource *r);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 22aa961..b2ff223 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -554,7 +554,7 @@ static int irq_domain_translate(struct irq_domain *d,
 	return 0;
 }
 
-static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
+void irq_of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
 				      struct irq_fwspec *fwspec)
 {
 	int i;
@@ -565,6 +565,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
 	for (i = 0; i < irq_data->args_count; i++)
 		fwspec->param[i] = irq_data->args[i];
 }
+EXPORT_SYMBOL_GPL(irq_of_phandle_args_to_fwspec);
 
 unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 {
@@ -618,7 +619,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
 {
 	struct irq_fwspec fwspec;
 
-	of_phandle_args_to_fwspec(irq_data, &fwspec);
+	irq_of_phandle_args_to_fwspec(irq_data, &fwspec);
 	return irq_create_fwspec_mapping(&fwspec);
 }
 EXPORT_SYMBOL_GPL(irq_create_of_mapping);
-- 
2.5.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

Re: [PATCH v2 2/5] irqdomain: introduce irq_of_phandle_args_to_fwspec

[Marc Zyngier]

On Mon, 23 Nov 2015 11:16:53 +0100
Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:

> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> 
> Export of_phandle_args_to_fwspec with a new compliant name.
> 
> Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> ---
>  include/linux/of_irq.h | 2 ++
>  kernel/irq/irqdomain.c | 5 +++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index 6f879c6..66bd92e 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -34,6 +34,8 @@ static inline int of_irq_parse_oldworld(struct device_node *device, int index,
>  extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq);
>  extern int of_irq_parse_one(struct device_node *device, int index,
>  			  struct of_phandle_args *out_irq);
> +extern void irq_of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
> +					  struct irq_fwspec *fwspec);
>  extern unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data);
>  extern int of_irq_to_resource(struct device_node *dev, int index,
>  			      struct resource *r);
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 22aa961..b2ff223 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -554,7 +554,7 @@ static int irq_domain_translate(struct irq_domain *d,
>  	return 0;
>  }
>  
> -static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
> +void irq_of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
>  				      struct irq_fwspec *fwspec)
>  {
>  	int i;
> @@ -565,6 +565,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
>  	for (i = 0; i < irq_data->args_count; i++)
>  		fwspec->param[i] = irq_data->args[i];
>  }
> +EXPORT_SYMBOL_GPL(irq_of_phandle_args_to_fwspec);
>  
>  unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  {
> @@ -618,7 +619,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
>  {
>  	struct irq_fwspec fwspec;
>  
> -	of_phandle_args_to_fwspec(irq_data, &fwspec);
> +	irq_of_phandle_args_to_fwspec(irq_data, &fwspec);
>  	return irq_create_fwspec_mapping(&fwspec);
>  }
>  EXPORT_SYMBOL_GPL(irq_create_of_mapping);

Given what I've said earlier about keeping fwspec structures around,
you can probably discard that patch.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/5] pinctrl: meson: enable GPIO IRQs

[Carlo Caione]

From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>

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-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
Signed-off-by: Beniamino Galvani <b.galvani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

---

Linus,
I guess it's time to respin this patch and the discussion since I see no better
way to do this. I CCed a bunch of IRQ people to check whether they have a
any suggestions.

Since one of your major concern was that the code must not rely on .to_irq()
being called to request the IRQ from the irqchip side please note that I tested
this patch both using gpiod_to_irq() and using DT, in both case it works just
fine.

The point in this patch is that we cannot allocate anything at probe time
(neither in .irq_request_resources()) since registers configuration is strictly
dependent on the pin / GPIO / hwirq is going to be setup. Following is more
detailed scheme:

GPIO0 -> [mux1 [7:0]]   ->|
GPIO1 -> [mux1 [15:8]]  ->|-|
GPIO2 -> [mux1 [23:16]] ->| |
GPIO3 -> [mux1 [31:24]] ->| |
                            |->[polarity]->[filter]->[edge select]-> GICv2
GPIO4 -> [mux2 [7:0]]   ->| |
GPIO5 -> [mux2 [15:8]]  ->| |
GPIO6 -> [mux2 [23:16]] ->|-|
GPIO7 -> [mux2 [31:24]] ->|

The 8bit bitfields in the mux1 / mux2 registers are used to link the GPIOx IRQ
to the GPIO used to trigger the IRQ (i.e. if we want to use GPIOY0 pin to
trigger the GPIO0 IRQ we set mux1[7:0] = 80).

Thanks,

---
 drivers/pinctrl/Kconfig                 |   1 +
 drivers/pinctrl/meson/pinctrl-meson.c   | 303 ++++++++++++++++++++++++++++++++
 drivers/pinctrl/meson/pinctrl-meson.h   |  18 +-
 drivers/pinctrl/meson/pinctrl-meson8.c  |  36 ++--
 drivers/pinctrl/meson/pinctrl-meson8b.c |  36 ++--
 5 files changed, 363 insertions(+), 31 deletions(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 312c78b..47f35b4 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -128,6 +128,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 84943e4..c3962b9 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.c
+++ b/drivers/pinctrl/meson/pinctrl-meson.c
@@ -59,11 +59,23 @@
 #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_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
+
 /**
  * meson_get_bank() - find the bank containing a given pin
  *
@@ -540,6 +552,30 @@ 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 irq_fwspec irq_data;
+	unsigned int hwirq, irq;
+
+	hwirq = domain->data->pin_base + offset;
+
+	if (meson_get_bank(domain, hwirq, &bank))
+		return -ENXIO;
+
+	irq_data.param_count = 2;
+	irq_data.param[0] = hwirq;
+
+	/* dummy. It will be changed later in meson_irq_set_type */
+	irq_data.param[1] = IRQ_TYPE_EDGE_RISING;
+
+	irq = irq_domain_alloc_irqs(pc->irq_domain, 1, NUMA_NO_NODE, &irq_data);
+
+	return irq ? irq : -ENXIO;
+}
+
 static const struct of_device_id meson_pinctrl_dt_match[] = {
 	{
 		.compatible = "amlogic,meson8-pinctrl",
@@ -569,6 +605,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 +717,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 +748,267 @@ 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 = irq_data_get_irq_chip_data(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;
+
+	return irq_chip_set_type_parent(data, type);
+}
+
+int meson_irq_request_resources(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = irq_data_get_irq_chip_data(data);
+	struct meson_domain *domain;
+	struct meson_bank *bank;
+
+	if (meson_get_domain_and_bank(pc, data->hwirq, &domain, &bank))
+		return -EINVAL;
+
+	if (gpiochip_lock_as_irq(&domain->chip, data->hwirq))
+		return -EINVAL;
+
+	return 0;
+}
+
+void meson_irq_release_resources(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = irq_data_get_irq_chip_data(data);
+	struct meson_domain *domain;
+	struct meson_bank *bank;
+
+	if (meson_get_domain_and_bank(pc, data->hwirq, &domain, &bank))
+		return;
+
+	gpiochip_unlock_as_irq(&domain->chip, data->hwirq);
+}
+
+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,
+	.irq_request_resources	= meson_irq_request_resources,
+	.irq_release_resources	= meson_irq_release_resources,
+};
+
+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 irq,
+				  unsigned int nr_irqs, void *arg)
+{
+	struct meson_pinctrl *pc = domain->host_data;
+	struct irq_fwspec *irq_data = arg;
+	struct irq_fwspec gic_data;
+	irq_hw_number_t hwirq;
+	int index, ret, i;
+
+	if (irq_data->param_count != 2)
+		return -EINVAL;
+
+	hwirq = irq_data->param[0];
+	dev_dbg(pc->dev, "%s irq %d, nr %d, hwirq %lu\n",
+			__func__, irq, 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, irq + i,
+					      hwirq + i,
+					      &meson_irq_chip,
+					      pc);
+
+		gic_data = pc->gic_irqs[index];
+		ret = irq_domain_alloc_irqs_parent(domain, irq + i, nr_irqs,
+						   &gic_data);
+	}
+
+	return 0;
+}
+
+static void meson_irq_domain_free(struct irq_domain *domain, unsigned int irq,
+				  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, irq + 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, irq, nr_irqs);
+}
+
+static int meson_irq_domain_translate(struct irq_domain *d,
+				      struct irq_fwspec *fwspec,
+				      unsigned long *hwirq,
+				      unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 2)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1];
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static struct irq_domain_ops meson_irq_domain_ops = {
+	.alloc		= meson_irq_domain_alloc,
+	.free		= meson_irq_domain_free,
+	.translate	= meson_irq_domain_translate,
+};
+
+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 irq_fwspec) *
+				    pc->num_gic_irqs, GFP_KERNEL);
+	if (!pc->gic_irqs)
+		return -ENOMEM;
+
+	for (i = 0; i < pc->num_gic_irqs; i++) {
+		struct of_phandle_args oirq;
+
+		of_irq_parse_one(node, i, &oirq);
+		irq_of_phandle_args_to_fwspec(&oirq, &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 +1048,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..c2f64b6 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 irq_fwspec *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,
 };
-- 
2.5.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

Re: [PATCH v2 3/5] pinctrl: meson: enable GPIO IRQs

[Marc Zyngier]

On Mon, 23 Nov 2015 11:16:54 +0100
Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:

> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> 
> 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-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Beniamino Galvani <b.galvani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> ---

[...]

> +	for (i = 0; i < pc->num_gic_irqs; i++) {
> +		struct of_phandle_args oirq;
> +
> +		of_irq_parse_one(node, i, &oirq);
> +		irq_of_phandle_args_to_fwspec(&oirq, &pc->gic_irqs[i]);
> +
> +		pc->irq_map[i] = IRQ_FREE;
> +	}

The whole thing feels weird. Why do you need to keep a set of fwspecs?
All you need is a range of interrupts that would be conveniently
represented by a bitmap (assuming your interrupts space is a mostly
contiguous range).

Overall, this patch is quite hard to review. Can you please split the
GPIO management from the irqchip side?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.
--
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: [linux-meson] Re: [PATCH v2 3/5] pinctrl: meson: enable GPIO IRQs

[Carlo Caione]

On Tue, Nov 24, 2015 at 9:28 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On Mon, 23 Nov 2015 11:16:54 +0100
> Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:
>
>> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>>
>> 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-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Beniamino Galvani <b.galvani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> ---
>
> [...]
>
>> +     for (i = 0; i < pc->num_gic_irqs; i++) {
>> +             struct of_phandle_args oirq;
>> +
>> +             of_irq_parse_one(node, i, &oirq);
>> +             irq_of_phandle_args_to_fwspec(&oirq, &pc->gic_irqs[i]);
>> +
>> +             pc->irq_map[i] = IRQ_FREE;
>> +     }
>
> The whole thing feels weird. Why do you need to keep a set of fwspecs?

We only have 8 IRQs on the GIC side that we can use for the GPIOs. The
set of fwspec is used to track these IRQs.
At probe time we read from the DTS how many and which IRQs are
reserved on the GIC for the GPIOs. Every time a GPIO IRQ is installed,
we use one of these IRQ lines.

> All you need is a range of interrupts that would be conveniently
> represented by a bitmap (assuming your interrupts space is a mostly
> contiguous range).

I could do that but it feels to me like we end up hiding from the DTS
some information that naturally should be in there: a pinctrl device
that is using 8 interrupt lines of its interrupt controller. IMO this
is not a special case that requires some special treatment.

> Overall, this patch is quite hard to review. Can you please split the
> GPIO management from the irqchip side?

Sure. Fix in v2.

Thanks for the review,

-- 
Carlo Caione
--
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: [linux-meson] Re: [PATCH v2 3/5] pinctrl: meson: enable GPIO IRQs

[Carlo Caione]

On Tue, Nov 24, 2015 at 10:04 AM, Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Nov 24, 2015 at 9:28 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:

>> [...]
>>
>>> +     for (i = 0; i < pc->num_gic_irqs; i++) {
>>> +             struct of_phandle_args oirq;
>>> +
>>> +             of_irq_parse_one(node, i, &oirq);
>>> +             irq_of_phandle_args_to_fwspec(&oirq, &pc->gic_irqs[i]);
>>> +
>>> +             pc->irq_map[i] = IRQ_FREE;
>>> +     }
>>
>> The whole thing feels weird. Why do you need to keep a set of fwspecs?
>
> We only have 8 IRQs on the GIC side that we can use for the GPIOs. The
> set of fwspec is used to track these IRQs.
> At probe time we read from the DTS how many and which IRQs are
> reserved on the GIC for the GPIOs. Every time a GPIO IRQ is installed,
> we use one of these IRQ lines.
>
>> All you need is a range of interrupts that would be conveniently
>> represented by a bitmap (assuming your interrupts space is a mostly
>> contiguous range).
>
> I could do that but it feels to me like we end up hiding from the DTS
> some information that naturally should be in there: a pinctrl device
> that is using 8 interrupt lines of its interrupt controller. IMO this
> is not a special case that requires some special treatment.

Marc,
What's your opinion on this? So I can start cooking v3

Thanks,

-- 
Carlo Caione
--
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: [linux-meson] Re: [PATCH v2 3/5] pinctrl: meson: enable GPIO IRQs

[Marc Zyngier]

On Tue, 24 Nov 2015 10:04:50 +0100
Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:

> On Tue, Nov 24, 2015 at 9:28 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Mon, 23 Nov 2015 11:16:54 +0100
> > Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:
> >
> >> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> >>
> >> 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-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> >> Signed-off-by: Beniamino Galvani <b.galvani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>
> >> ---
> >
> > [...]
> >
> >> +     for (i = 0; i < pc->num_gic_irqs; i++) {
> >> +             struct of_phandle_args oirq;
> >> +
> >> +             of_irq_parse_one(node, i, &oirq);
> >> +             irq_of_phandle_args_to_fwspec(&oirq, &pc->gic_irqs[i]);
> >> +
> >> +             pc->irq_map[i] = IRQ_FREE;
> >> +     }
> >
> > The whole thing feels weird. Why do you need to keep a set of fwspecs?
> 
> We only have 8 IRQs on the GIC side that we can use for the GPIOs. The
> set of fwspec is used to track these IRQs.
> At probe time we read from the DTS how many and which IRQs are
> reserved on the GIC for the GPIOs. Every time a GPIO IRQ is installed,
> we use one of these IRQ lines.

That doesn't answer my question. Having the raw IRQ numbers, why not.
Storing a whole fwspec (16 u32 + 1 int + 1 pointer) seems completely
overkill.

> > All you need is a range of interrupts that would be conveniently
> > represented by a bitmap (assuming your interrupts space is a mostly
> > contiguous range).
> 
> I could do that but it feels to me like we end up hiding from the DTS
> some information that naturally should be in there: a pinctrl device
> that is using 8 interrupt lines of its interrupt controller. IMO this
> is not a special case that requires some special treatment.

Well, it is also arguable that these interrupts are not the pinctrl's,
but those of the device that actually uses them (or we'd be describing
interrupts in all interrupt controller bindings, which doesn't make any
sense).

See for example the TI crossbar, which has similar functionalities. I'm
not going to take a hard line here, but the argument you have so far
doesn't seem to hold much water for me. Or there is something obvious
that I have missed, which is entirely possible.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.
--
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: [linux-meson] Re: [PATCH v2 3/5] pinctrl: meson: enable GPIO IRQs

[Carlo Caione]

On Thu, Nov 26, 2015 at 5:27 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On Tue, 24 Nov 2015 10:04:50 +0100
> Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:
>
>> On Tue, Nov 24, 2015 at 9:28 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>> > On Mon, 23 Nov 2015 11:16:54 +0100
>> > Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:
>> >
>> >> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>> >>
>> >> 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-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>> >> Signed-off-by: Beniamino Galvani <b.galvani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >>
>> >> ---
>> >
>> > [...]
>> >
>> >> +     for (i = 0; i < pc->num_gic_irqs; i++) {
>> >> +             struct of_phandle_args oirq;
>> >> +
>> >> +             of_irq_parse_one(node, i, &oirq);
>> >> +             irq_of_phandle_args_to_fwspec(&oirq, &pc->gic_irqs[i]);
>> >> +
>> >> +             pc->irq_map[i] = IRQ_FREE;
>> >> +     }
>> >
>> > The whole thing feels weird. Why do you need to keep a set of fwspecs?
>>
>> We only have 8 IRQs on the GIC side that we can use for the GPIOs. The
>> set of fwspec is used to track these IRQs.
>> At probe time we read from the DTS how many and which IRQs are
>> reserved on the GIC for the GPIOs. Every time a GPIO IRQ is installed,
>> we use one of these IRQ lines.
>
> That doesn't answer my question. Having the raw IRQ numbers, why not.
> Storing a whole fwspec (16 u32 + 1 int + 1 pointer) seems completely
> overkill.

Oh, you were referring to the type used. Agree, since we are moving
away (read below) from the DTS, probably using fwspecs is just on
overkill.
The only thing I'm not really fond of is to use a bitmap since I'm not
really sure that the interrupt space is a contiguous range for all the
Amlogic / Meson SoCs. Probably the most sensible way is using
something like TI did for 'ti,irqs-reserved' that is an array of IRQs.

>> > All you need is a range of interrupts that would be conveniently
>> > represented by a bitmap (assuming your interrupts space is a mostly
>> > contiguous range).
>>
>> I could do that but it feels to me like we end up hiding from the DTS
>> some information that naturally should be in there: a pinctrl device
>> that is using 8 interrupt lines of its interrupt controller. IMO this
>> is not a special case that requires some special treatment.
>
> Well, it is also arguable that these interrupts are not the pinctrl's,
> but those of the device that actually uses them (or we'd be describing
> interrupts in all interrupt controller bindings, which doesn't make any
> sense).

Ok, I see you point

> See for example the TI crossbar, which has similar functionalities. I'm
> not going to take a hard line here, but the argument you have so far
> doesn't seem to hold much water for me. Or there is something obvious
> that I have missed, which is entirely possible.

I hope things will be a bit clearer in v3.

Thanks,

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

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

[Carlo Caione]

From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>

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

Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
Signed-off-by: Beniamino Galvani <b.galvani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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@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;
-- 
2.5.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

Re: [PATCH v2 4/5] pinctrl: dt-binding: Extend meson documentation with GPIO IRQs support

[Rob Herring]

On Mon, Nov 23, 2015 at 11:16:55AM +0100, Carlo Caione wrote:
> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> 
> Extend the pinctrl binding documentation with the support for external
> GPIO interrupts.
> 
> Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Beniamino Galvani <b.galvani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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"

You don't really need names when there is only one. Otherwise:

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

> + - 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@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;
> -- 
> 2.5.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

Re: [linux-meson] Re: [PATCH v2 4/5] pinctrl: dt-binding: Extend meson documentation with GPIO IRQs support

[Carlo Caione]

On lun, nov 23, 2015 at 05:47:20 -0600, Rob Herring wrote:
> On Mon, Nov 23, 2015 at 11:16:55AM +0100, Carlo Caione wrote:
> > From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> > 
> > Extend the pinctrl binding documentation with the support for external
> > GPIO interrupts.
> > 
> > Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Beniamino Galvani <b.galvani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  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"
> 
> You don't really need names when there is only one. Otherwise:
> 
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

I missed this reply sorry. The name is conveniently used so that I can
use the meson_map_resource() helper function to map that and get back
the regmap struct in one shot.

I'm going to submit v3 soon without this modification leaving out your
Acked-by. Let me know if your ACK is still valid without this code
change.

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

[PATCH v2 5/5] ARM: meson: DTS: Enable GPIO IRQs

[Carlo Caione]

From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>

Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
---
 arch/arm/boot/dts/meson8b.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index ee352bf..3c0ae45 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -152,6 +152,12 @@
 		pinctrl: pinctrl@c1109880 {
 			compatible = "amlogic,meson8b-pinctrl";
 			reg = <0xc1109880 0x10>;
+			reg-names = "irq";
+			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;
-- 
2.5.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

Re: [PATCH v2 0/5] pinctrl: meson: enable support for external GPIO interrupts

[Linus Walleij]

On Mon, Nov 23, 2015 at 11:16 AM, Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:

> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>
> 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
>
> For discussion see comment to the [PATCH 3/5].
>
> Changelog:
>
> * V2:
>     - Introduced .irq_request_resources() and .irq_release_resources()
>     - s/virq/irq/ and s/pin/hwirq/
>     - Moved to the new irq_fwspec

I'm waiting for the next version of this patch set.

However I am a bit concerned that we're starting to have a few
GPIO controllers now that are not really cascaded irqchips,
but instead just shunt the line through to some other interrupt
controller through a mux or latch.

We might need to add some handling to gpiolib core that
does this, like gpiochip_add_irq_mux() that sets up the
irqdomain we have for GPIOLIB_IRQCHIP to be used like
this and pull this code into gpiolib to avoid duplication of
code and bugs.

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