pinctrl: meson: enable support for external GPIO interrupts

From: Carlo Caione <[email protected]>

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 <[email protected]>

---
 drivers/of/irq.c       | 2 +-
 include/linux/of_irq.h | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

drivers/of/irq.c +0 -0

index 902b89b..45735d5 100644

From: Carlo Caione <[email protected]>

Export of_phandle_args_to_fwspec with a new compliant name.

Signed-off-by Carlo Caione <[email protected]>

---
 include/linux/of_irq.h | 2 ++
 kernel/irq/irqdomain.c | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

include/linux/of_irq.h +2 -0

index 6f879c6..66bd92e 100644
@@ -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);

kernel/irq/irqdomain.c +2 -1

index 22aa961..b2ff223 100644
@@ -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)
 {

From: Carlo Caione <[email protected]>

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 <[email protected]>

Signed-off-by Beniamino Galvani <[email protected]>

---

Quoted 21 lines · On Mon, 23 Nov 2015 11:16:54 +0100 Carlo Caione <[email protected]> wrote:

From: Carlo Caione <[email protected]>

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 <[email protected]>

Signed-off-by Beniamino Galvani <[email protected]>

---

[...]

drivers/pinctrl/meson/pinctrl-meson.c static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc, · line 1000

Quoted 8 lines

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

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

Quoted 12 lines · Marc Zyngier

[...]

drivers/pinctrl/meson/pinctrl-meson.c static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc, · line 1000

Quoted 8 lines

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

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

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

Quoted 27 lines · Marc Zyngier

Quoted 21 lines · On Mon, 23 Nov 2015 11:16:54 +0100 Carlo Caione <[email protected]> wrote:

From: Carlo Caione <[email protected]>

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 <[email protected]>

Signed-off-by Beniamino Galvani <[email protected]>

---

[...]

Quoted 10 lines

drivers/pinctrl/meson/pinctrl-meson.c static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc, · line 1000

Quoted 8 lines

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

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

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

Quoted 39 lines · On Tue, 24 Nov 2015 10:04:50 +0100 Carlo Caione <[email protected]> wrote:

Quoted 27 lines · Marc Zyngier

Quoted 21 lines · On Mon, 23 Nov 2015 11:16:54 +0100 Carlo Caione <[email protected]> wrote:

From: Carlo Caione <[email protected]>

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 <[email protected]>

Signed-off-by Beniamino Galvani <[email protected]>

---

[...]

Quoted 10 lines

drivers/pinctrl/meson/pinctrl-meson.c static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc, · line 1000

Quoted 8 lines

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

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

Quoted 5 lines

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.

Quoted 8 lines

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

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.

From: Carlo Caione <[email protected]>

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

Signed-off-by Carlo Caione <[email protected]>

Signed-off-by Beniamino Galvani <[email protected]>

---
 Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt +0 -0

index 3f6a524..56743eb 100644

Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt +11 -0

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

Signature · 1 line

2.5.0

Quoted 16 lines · Carlo Caione

From: Carlo Caione <[email protected]>

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

Signed-off-by Carlo Caione <[email protected]>

Signed-off-by Beniamino Galvani <[email protected]>

---
 Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt +0 -0

index 3f6a524..56743eb 100644

Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt line 6

Quoted 4 lines

Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt +1 -0

@@ -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 <[email protected]>

From: Carlo Caione <[email protected]>

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