support wakeup capable GPIOs

Some interrupt controllers in a SoC, are always powered on and have a select interrupts routed to them, so that they can wakeup the SoC from suspend. Add wakeup-parent DT property to refer to these interrupt controllers.

If the interrupts routed to the wakeup parent are not sequential, than a map needs to exist to associate the same interrupt line on multiple interrupt controllers. Providing this map in every driver is cumbersome. Let's add this in the device tree and document the properties to map the interrupt specifiers

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v4:
- Added this documentation

---
 .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +29 -0

index 8a3c40829899..917b598317f5 100644
@@ -108,3 +108,42 @@ commonly used:
 			sensitivity = <7>;
 		};
 	};
+
+3) Interrupt wakeup parent
+--------------------------
+
+Some interrupt controllers in a SoC, are always powered on and have a select
+interrupts routed to them, so that they can wakeup the SoC from suspend. These
+interrupt controllers do not fall into the category of a parent interrupt
+controller and can be specified by the "wakeup-parent" property and contain a
+single phandle referring to the wakeup capable interrupt controller.
+
+   Example:
+	wakeup-parent = <&pdc_intc>;
+
+
+4) Interrupt mapping
+--------------------
+
+Sometimes interrupts may be detected by more than one interrupt controller
+(depending on which controller is active). The interrupt controllers may not
+be in hierarchy and therefore the interrupt controller driver is required to
+establish the relationship between the same interrupt at different interrupt
+controllers. If these interrupts are not sequential then a map needs to be
+specified to help identify these interrupts.
+
+Mapping the interrupt specifiers in the device tree can be done using the
+"irqdomain-map" property. The property contains interrupt specifier at the
+current interrupt controller followed by the interrupt specifier at the mapped
+interrupt controller.
+

On Wed, 13 Mar 2019 15:18:37 -0600
Lina Iyer <[email protected]> wrote:

Please do Cc Rob when posting DT related patches.

Quoted 64 lines

Some interrupt controllers in a SoC, are always powered on and have a select interrupts routed to them, so that they can wakeup the SoC from suspend. Add wakeup-parent DT property to refer to these interrupt controllers.

If the interrupts routed to the wakeup parent are not sequential, than a map needs to exist to associate the same interrupt line on multiple interrupt controllers. Providing this map in every driver is cumbersome. Let's add this in the device tree and document the properties to map the interrupt specifiers

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v4:
- Added this documentation

---
 .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +37 -0

index 8a3c40829899..917b598317f5 100644
@@ -108,3 +108,42 @@ commonly used:
 			sensitivity = <7>;
 		};
 	};
+
+3) Interrupt wakeup parent
+--------------------------
+
+Some interrupt controllers in a SoC, are always powered on and have a select
+interrupts routed to them, so that they can wakeup the SoC from suspend. These
+interrupt controllers do not fall into the category of a parent interrupt
+controller and can be specified by the "wakeup-parent" property and contain a
+single phandle referring to the wakeup capable interrupt controller.
+
+   Example:
+	wakeup-parent = <&pdc_intc>;
+
+
+4) Interrupt mapping
+--------------------
+
+Sometimes interrupts may be detected by more than one interrupt controller
+(depending on which controller is active). The interrupt controllers may not
+be in hierarchy and therefore the interrupt controller driver is required to
+establish the relationship between the same interrupt at different interrupt
+controllers. If these interrupts are not sequential then a map needs to be
+specified to help identify these interrupts.
+
+Mapping the interrupt specifiers in the device tree can be done using the
+"irqdomain-map" property. The property contains interrupt specifier at the
+current interrupt controller followed by the interrupt specifier at the mapped
+interrupt controller.
+
+   irqdomain-map = <incoming-interrupt-specifier mapped-interrupt-specifier>
+
+The optional properties "irqdomain-map-mask" and "irqdomain-map-pass-thru" may
+be provided to help interpret the valid bits of the incoming and mapped
+interrupt specifiers respectively.
+
+   Example:
+	irqdomain-map = <22 0 &intc 36 0>, <24 0 &intc 37 0>;

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt commonly used: · line 149

Quoted 2 lines

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +2 -0

+	irqdomain-map-mask = <0xff 0>;
+	irqdomain-map-pass-thru = <0 0xff>;

This doesn't quite explain how the mask and pass-thru properties are used. I guess that the mask is used to define the 'useful bits' on the incoming side, but pass-thru puzzles me. In your example, does it mean that incoming lines map to outgoing interrupt <0 0>?

Quoted 69 lines · Marc Zyngier

On Wed, 13 Mar 2019 15:18:37 -0600
Lina Iyer <[email protected]> wrote:

Please do Cc Rob when posting DT related patches.

Quoted 64 lines

Some interrupt controllers in a SoC, are always powered on and have a select interrupts routed to them, so that they can wakeup the SoC from suspend. Add wakeup-parent DT property to refer to these interrupt controllers.

If the interrupts routed to the wakeup parent are not sequential, than a map needs to exist to associate the same interrupt line on multiple interrupt controllers. Providing this map in every driver is cumbersome. Let's add this in the device tree and document the properties to map the interrupt specifiers

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v4:
- Added this documentation

---
 .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +37 -0

index 8a3c40829899..917b598317f5 100644
@@ -108,3 +108,42 @@ commonly used:
 			sensitivity = <7>;
 		};
 	};
+
+3) Interrupt wakeup parent
+--------------------------
+
+Some interrupt controllers in a SoC, are always powered on and have a select
+interrupts routed to them, so that they can wakeup the SoC from suspend. These
+interrupt controllers do not fall into the category of a parent interrupt
+controller and can be specified by the "wakeup-parent" property and contain a
+single phandle referring to the wakeup capable interrupt controller.
+
+   Example:
+	wakeup-parent = <&pdc_intc>;
+
+
+4) Interrupt mapping
+--------------------
+
+Sometimes interrupts may be detected by more than one interrupt controller
+(depending on which controller is active). The interrupt controllers may not
+be in hierarchy and therefore the interrupt controller driver is required to
+establish the relationship between the same interrupt at different interrupt
+controllers. If these interrupts are not sequential then a map needs to be
+specified to help identify these interrupts.
+
+Mapping the interrupt specifiers in the device tree can be done using the
+"irqdomain-map" property. The property contains interrupt specifier at the
+current interrupt controller followed by the interrupt specifier at the mapped
+interrupt controller.
+
+   irqdomain-map = <incoming-interrupt-specifier mapped-interrupt-specifier>
+
+The optional properties "irqdomain-map-mask" and "irqdomain-map-pass-thru" may
+be provided to help interpret the valid bits of the incoming and mapped
+interrupt specifiers respectively.
+
+   Example:
+	irqdomain-map = <22 0 &intc 36 0>, <24 0 &intc 37 0>;

Quoted 9 lines

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt commonly used: · line 149

Quoted 2 lines

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +2 -0

+	irqdomain-map-mask = <0xff 0>;
+	irqdomain-map-pass-thru = <0 0xff>;

This doesn't quite explain how the mask and pass-thru properties are used. I guess that the mask is used to define the 'useful bits' on the incoming side, but pass-thru puzzles me. In your example, does it mean that incoming lines map to outgoing interrupt <0 0>?

Sorry about the late reply.

How about this to go with the rest of the documentation -

In the above example, the input interrupt specifier map-mask <0xff 0> applied on the incoming interrupt specifier of the map <22 0>, <24 0>, returns the input interrupt 22, 24 etc. The second argument being irq type is immaterial from the map and is used from the incoming request instead. The pass-thru specifier parses the output interrupt specifier from the rest of the unparsed argments from the map <&intc 36 0>, <&intc 37 0> etc to return the output interrupt 36, 37 etc.

Quoted 83 lines · Lina Iyer

Quoted 69 lines · Marc Zyngier

On Wed, 13 Mar 2019 15:18:37 -0600
Lina Iyer <[email protected]> wrote:

Please do Cc Rob when posting DT related patches.

Quoted 64 lines

Some interrupt controllers in a SoC, are always powered on and have a select interrupts routed to them, so that they can wakeup the SoC from suspend. Add wakeup-parent DT property to refer to these interrupt controllers.

If the interrupts routed to the wakeup parent are not sequential, than a map needs to exist to associate the same interrupt line on multiple interrupt controllers. Providing this map in every driver is cumbersome. Let's add this in the device tree and document the properties to map the interrupt specifiers

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v4:
- Added this documentation

---
 .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +37 -0

index 8a3c40829899..917b598317f5 100644
@@ -108,3 +108,42 @@ commonly used:
 			sensitivity = <7>;
 		};
 	};
+
+3) Interrupt wakeup parent
+--------------------------
+
+Some interrupt controllers in a SoC, are always powered on and have a select
+interrupts routed to them, so that they can wakeup the SoC from suspend. These
+interrupt controllers do not fall into the category of a parent interrupt
+controller and can be specified by the "wakeup-parent" property and contain a
+single phandle referring to the wakeup capable interrupt controller.
+
+   Example:
+	wakeup-parent = <&pdc_intc>;
+
+
+4) Interrupt mapping
+--------------------
+
+Sometimes interrupts may be detected by more than one interrupt controller
+(depending on which controller is active). The interrupt controllers may not
+be in hierarchy and therefore the interrupt controller driver is required to
+establish the relationship between the same interrupt at different interrupt
+controllers. If these interrupts are not sequential then a map needs to be
+specified to help identify these interrupts.
+
+Mapping the interrupt specifiers in the device tree can be done using the
+"irqdomain-map" property. The property contains interrupt specifier at the
+current interrupt controller followed by the interrupt specifier at the mapped
+interrupt controller.
+
+   irqdomain-map = <incoming-interrupt-specifier mapped-interrupt-specifier>
+
+The optional properties "irqdomain-map-mask" and "irqdomain-map-pass-thru" may
+be provided to help interpret the valid bits of the incoming and mapped
+interrupt specifiers respectively.
+
+   Example:
+	irqdomain-map = <22 0 &intc 36 0>, <24 0 &intc 37 0>;

Quoted 9 lines

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt commonly used: · line 149

Quoted 2 lines

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +2 -0

+	irqdomain-map-mask = <0xff 0>;
+	irqdomain-map-pass-thru = <0 0xff>;

This doesn't quite explain how the mask and pass-thru properties are used. I guess that the mask is used to define the 'useful bits' on the incoming side, but pass-thru puzzles me. In your example, does it mean that incoming lines map to outgoing interrupt <0 0>?

Sorry about the late reply.

How about this to go with the rest of the documentation -

Quoted 7 lines

In the above example, the input interrupt specifier map-mask <0xff 0> applied on the incoming interrupt specifier of the map <22 0>, <24 0>, returns the input interrupt 22, 24 etc. The second argument being irq type is immaterial from the map and is used from the incoming request instead. The pass-thru specifier parses the output interrupt specifier from the rest of the unparsed argments from the map <&intc 36 0>, <&intc 37 0> etc to return the output interrupt 36, 37 etc.

I think you need to add #interrupt-cells in your example, which is otherwise hard to interpret.

Quoted 69 lines

On Wed, 13 Mar 2019 15:18:37 -0600
Lina Iyer <[email protected]> wrote:

Please do Cc Rob when posting DT related patches.

Quoted 64 lines

Some interrupt controllers in a SoC, are always powered on and have a select interrupts routed to them, so that they can wakeup the SoC from suspend. Add wakeup-parent DT property to refer to these interrupt controllers.

If the interrupts routed to the wakeup parent are not sequential, than a map needs to exist to associate the same interrupt line on multiple interrupt controllers. Providing this map in every driver is cumbersome. Let's add this in the device tree and document the properties to map the interrupt specifiers

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v4:
- Added this documentation

---
 .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +37 -0

index 8a3c40829899..917b598317f5 100644
@@ -108,3 +108,42 @@ commonly used:
                     sensitivity = <7>;
             };
     };
+
+3) Interrupt wakeup parent
+--------------------------
+
+Some interrupt controllers in a SoC, are always powered on and have a select
+interrupts routed to them, so that they can wakeup the SoC from suspend. These
+interrupt controllers do not fall into the category of a parent interrupt
+controller and can be specified by the "wakeup-parent" property and contain a
+single phandle referring to the wakeup capable interrupt controller.
+
+   Example:
+    wakeup-parent = <&pdc_intc>;
+
+
+4) Interrupt mapping
+--------------------
+
+Sometimes interrupts may be detected by more than one interrupt controller
+(depending on which controller is active). The interrupt controllers may not
+be in hierarchy and therefore the interrupt controller driver is required to
+establish the relationship between the same interrupt at different interrupt
+controllers. If these interrupts are not sequential then a map needs to be
+specified to help identify these interrupts.
+
+Mapping the interrupt specifiers in the device tree can be done using the
+"irqdomain-map" property. The property contains interrupt specifier at the
+current interrupt controller followed by the interrupt specifier at the mapped
+interrupt controller.
+
+   irqdomain-map = <incoming-interrupt-specifier mapped-interrupt-specifier>
+
+The optional properties "irqdomain-map-mask" and "irqdomain-map-pass-thru" may
+be provided to help interpret the valid bits of the incoming and mapped
+interrupt specifiers respectively.
+
+   Example:
+    irqdomain-map = <22 0 &intc 36 0>, <24 0 &intc 37 0>;

Quoted 9 lines

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt commonly used: · line 149

Quoted 2 lines

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +2 -0

+    irqdomain-map-mask = <0xff 0>;
+    irqdomain-map-pass-thru = <0 0xff>;

This doesn't quite explain how the mask and pass-thru properties are used. I guess that the mask is used to define the 'useful bits' on the incoming side, but pass-thru puzzles me. In your example, does it mean that incoming lines map to outgoing interrupt <0 0>?

Sorry about the late reply.

How about this to go with the rest of the documentation -

In the above example, the input interrupt specifier map-mask <0xff 0> applied on the incoming interrupt specifier of the map <22 0>, <24 0>, returns the input interrupt 22, 24 etc. The second argument being irq type is immaterial from the map and is used from the incoming request instead. The pass-thru specifier parses the output interrupt specifier from the rest of the unparsed argments from the map <&intc 36 0>, <&intc 37 0> etc to return the output interrupt 36, 37 etc.

Quoting Lina Iyer (2019-04-04 08:58:38)

Quoted 83 lines · Marc Zyngier

Quoted 69 lines

On Wed, 13 Mar 2019 15:18:37 -0600
Lina Iyer <[email protected]> wrote:

Please do Cc Rob when posting DT related patches.

Quoted 64 lines

Some interrupt controllers in a SoC, are always powered on and have a select interrupts routed to them, so that they can wakeup the SoC from suspend. Add wakeup-parent DT property to refer to these interrupt controllers.

If the interrupts routed to the wakeup parent are not sequential, than a map needs to exist to associate the same interrupt line on multiple interrupt controllers. Providing this map in every driver is cumbersome. Let's add this in the device tree and document the properties to map the interrupt specifiers

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v4:
- Added this documentation

---
 .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +37 -0

index 8a3c40829899..917b598317f5 100644
@@ -108,3 +108,42 @@ commonly used:
                     sensitivity = <7>;
             };
     };
+
+3) Interrupt wakeup parent
+--------------------------
+
+Some interrupt controllers in a SoC, are always powered on and have a select
+interrupts routed to them, so that they can wakeup the SoC from suspend. These
+interrupt controllers do not fall into the category of a parent interrupt
+controller and can be specified by the "wakeup-parent" property and contain a
+single phandle referring to the wakeup capable interrupt controller.
+
+   Example:
+    wakeup-parent = <&pdc_intc>;
+
+
+4) Interrupt mapping
+--------------------
+
+Sometimes interrupts may be detected by more than one interrupt controller
+(depending on which controller is active). The interrupt controllers may not
+be in hierarchy and therefore the interrupt controller driver is required to
+establish the relationship between the same interrupt at different interrupt
+controllers. If these interrupts are not sequential then a map needs to be
+specified to help identify these interrupts.
+
+Mapping the interrupt specifiers in the device tree can be done using the
+"irqdomain-map" property. The property contains interrupt specifier at the
+current interrupt controller followed by the interrupt specifier at the mapped
+interrupt controller.
+
+   irqdomain-map = <incoming-interrupt-specifier mapped-interrupt-specifier>
+
+The optional properties "irqdomain-map-mask" and "irqdomain-map-pass-thru" may
+be provided to help interpret the valid bits of the incoming and mapped
+interrupt specifiers respectively.
+
+   Example:
+    irqdomain-map = <22 0 &intc 36 0>, <24 0 &intc 37 0>;

Quoted 9 lines

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt commonly used: · line 149

Quoted 2 lines

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +2 -0

+    irqdomain-map-mask = <0xff 0>;
+    irqdomain-map-pass-thru = <0 0xff>;

This doesn't quite explain how the mask and pass-thru properties are used. I guess that the mask is used to define the 'useful bits' on the incoming side, but pass-thru puzzles me. In your example, does it mean that incoming lines map to outgoing interrupt <0 0>?

Sorry about the late reply.

How about this to go with the rest of the documentation -

Quoted 9 lines

In the above example, the input interrupt specifier map-mask <0xff 0> applied on the incoming interrupt specifier of the map <22 0>, <24 0>, returns the input interrupt 22, 24 etc. The second argument being irq type is immaterial from the map and is used from the incoming request instead. The pass-thru specifier parses the output interrupt specifier from the rest of the unparsed argments from the map <&intc 36 0>, <&intc 37 0> etc to return the output interrupt 36, 37 etc.

I see two things going on here. Do both need to happen?

#1: Specifying wakeup parent phandle
#2: Mapping GPIO interrupts to a parent irqdomain

Do we need the method of specifying the wakeup parent if with a dt property if we have a way to map irqdomains from one to another? I think I may have already said on the list that we must have #1 but now I'm not so sure. It looks like we could get away with just looking into the irqdomain-map and then pick out the wakeup parent that way.

The way the bindings are written shows one way to map interrupts between domains but I don't know if it lets us differentiate which irqs go from which domain to what other domain. It seems that we assume we're looking at only the GPIO to wakeup parent irqdomain mapping from the irqdomain-map property in this series. If we had a way to do this with the irqdomain map then we could avoid needing a special 'wakeup-parent' property.

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +5 -0

+Mapping the interrupt specifiers in the device tree can be done using the
+"irqdomain-map" property. The property contains interrupt specifier at the
+current interrupt controller followed by the interrupt specifier at the mapped
+interrupt controller.
+

From: Stephen Boyd <[email protected]>

Sometimes interrupts are routed from an interrupt controller to another in no specific order. Having these in the drivers makes it difficult to maintain when the same drivers supports multiple variants with different mapping. Also, specifying them in DT makes little sense with a bunch of numbers like -

	<0, 13>, <5, 32>,

It makes more sense when we can have the parent handle along with interrupt specifiers for the incoming interrupt as well as that of the outgoing interrupt like -

	<22 0 &intc 36 0>,
	<24 0 &intc 37 0>,
	<26 0 &intc 38 0>,

And the interrupt specifiers can be interpreted using these optional properties -

	irqdomain-map-mask = <0xff 0>;
	irqdomain-map-pass-thru = <0 0xff>;

The irqdomain-map-mask reads the input interrupt specifier to parse the incoming interrupt port. The format of the output port is specified with the irqdomain-map-pass-thru property.

Let's add a helper function to parse this from DT and match a struct irq_fwspec using the input interrupt specifier from the irqdomain-map and the valid bits specified in the irqdomain-map-mask and copy the output interrupt specifier from the map to irq_fwspec per the mask in irqdomain-map-pass-thru property for the matched interrupt.

Signed-off-by Stephen Boyd <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v4:
- Fix commit text spelling and verbosity
---
drivers/of/irq.c | 125 +++++++++++++++++++++++++++++++++++++++++
include/linux/of_irq.h | 1 +

2 files changed, 126 insertions(+)

drivers/of/irq.c +31 -0

index e1f6f392a4c0..a1534f947ed4 100644
@@ -273,6 +273,131 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 }
 EXPORT_SYMBOL_GPL(of_irq_parse_raw);
 
+int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out)
+{
+	char *stem_name;
+	char *cells_name, *map_name = NULL, *mask_name = NULL;
+	char *pass_name = NULL;
+	struct device_node *cur, *new = NULL;
+	const __be32 *map, *mask, *pass;
+	static const __be32 dummy_mask[] = { [0 ... MAX_PHANDLE_ARGS] = ~0 };
+	static const __be32 dummy_pass[] = { [0 ... MAX_PHANDLE_ARGS] = 0 };
+	__be32 initial_match_array[MAX_PHANDLE_ARGS];
+	const __be32 *match_array = initial_match_array;
+	int i, ret, map_len, match;
+	u32 in_size, out_size;
+
+	stem_name = "";
+	cells_name = "#interrupt-cells";
+
+	ret = -ENOMEM;
+	map_name = kasprintf(GFP_KERNEL, "irqdomain%s-map", stem_name);
+	if (!map_name)
+		goto free;
+
+	mask_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-mask", stem_name);
+	if (!mask_name)
+		goto free;
+
+	pass_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-pass-thru", stem_name);
+	if (!pass_name)
+		goto free;
+
+	/* Get the #interrupt-cells property */

drivers/of/irq.c +23 -0

+	/* Get the irqdomain-map property */
+	map = of_get_property(cur, map_name, &map_len);
+	if (!map) {
+		ret = 0;
+		goto free;
+	}
+	map_len /= sizeof(u32);
+
+	/* Get the irqdomain-map-mask property (optional) */
+	mask = of_get_property(cur, mask_name, NULL);
+	if (!mask)
+		mask = dummy_mask;
+	/* Iterate through irqdomain-map property */
+	match = 0;
+	while (map_len > (in_size + 1) && !match) {
+		/* Compare specifiers */
+		match = 1;
+		for (i = 0; i < in_size; i++, map_len--)
+			match &= !((match_array[i] ^ *map++) & mask[i]);
+
+		of_node_put(new);
+		new = of_find_node_by_phandle(be32_to_cpup(map));
+		map++;

drivers/of/irq.c +4 -0

+		if (ret)
+			goto put;
+
+		/* Check for malformed properties */

drivers/of/irq.c +37 -0

+		/* Get the irqdomain-map-pass-thru property (optional) */
+		pass = of_get_property(cur, pass_name, NULL);
+		if (!pass)
+			pass = dummy_pass;
+
+		/*
+		 * Successfully parsed a irqdomain-map translation; copy new
+		 * specifier into the out structure, keeping the
+		 * bits specified in irqdomain-map-pass-thru.
+		 */
+		match_array = map - out_size;
+		for (i = 0; i < out_size; i++) {
+			__be32 val = *(map - out_size + i);
+
+			out->param[i] = in->param[i];
+			if (i < in_size) {
+				val &= ~pass[i];
+				val |= cpu_to_be32(out->param[i]) & pass[i];
+			}
+
+			out->param[i] = be32_to_cpu(val);
+		}
+		out->param_count = in_size = out_size;
+		out->fwnode = of_node_to_fwnode(new);
+	}
+put:
+	of_node_put(cur);
+	of_node_put(new);
+free:
+	kfree(mask_name);
+	kfree(map_name);
+	kfree(pass_name);
+
+	return ret;
+}
+EXPORT_SYMBOL(of_irq_domain_map);
+
 /**
 * of_irq_parse_one - Resolve an interrupt for a device
 * @device: the device whose interrupt is to be resolved

include/linux/of_irq.h +1 -1

index 1214cabb2247..86342502a62a 100644
@@ -32,6 +32,7 @@ static inline int of_irq_parse_oldworld(struct device_node *device, int index,
 }
 #endif /* CONFIG_PPC32 && CONFIG_PPC_PMAC */
 
+extern int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out);
 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);
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO routed to the PDC as interrupts that can be used to wake the system up from deep low power modes and suspend.

Cc [email protected]

Signed-off-by Lina Iyer <[email protected]>

---
 .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt    | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +5 -1

index 665aadb5ea28..f0fedbc5d41a 100644
@@ -29,6 +29,11 @@ SDM845 platform.
 	Definition: must be 2. Specifying the pin number and flags, as defined
 		    in <dt-bindings/interrupt-controller/irq.h>
 
+- wakeup-parent:
+	Usage: optional
+	Value type: <phandle>
+	Definition: A phandle to the wakeup interrupt controller for the SoC.
+
 - gpio-controller:
 	Usage: required
 	Value type: <none>

@@ -53,7 +58,6 @@ pin, a group, or a list of pins or groups. This configuration can include the
 mux function to select on those pin(s)/group(s), and various pin configuration
 parameters, such as pull-up, drive strength, etc.
 
-
 PIN CONFIGURATION NODES:
 
 The name of each subnode is not important; all subnodes should be enumerated

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

 		qup9_active: qup9-active {
 			mux {
-- 

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Quoted 4 lines · Lina Iyer

SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO routed to the PDC as interrupts that can be used to wake the system up from deep low power modes and suspend.

Quoted 5 lines

Cc [email protected]

Signed-off-by Lina Iyer <[email protected]>

---
 .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt    | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

I still don't love this being specific to wake-up, but don't have a better suggestion. One nit, otherwise:

Reviewed-by Rob Herring <[email protected]>

Quoted 26 lines

Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +5 -1

index 665aadb5ea28..f0fedbc5d41a 100644
@@ -29,6 +29,11 @@ SDM845 platform.
 	Definition: must be 2. Specifying the pin number and flags, as defined
 		    in <dt-bindings/interrupt-controller/irq.h>
 
+- wakeup-parent:
+	Usage: optional
+	Value type: <phandle>
+	Definition: A phandle to the wakeup interrupt controller for the SoC.
+
 - gpio-controller:
 	Usage: required
 	Value type: <none>

@@ -53,7 +58,6 @@ pin, a group, or a list of pins or groups. This configuration can include the
 mux function to select on those pin(s)/group(s), and various pin configuration
 parameters, such as pull-up, drive strength, etc.
 
-
 PIN CONFIGURATION NODES:
 
 The name of each subnode is not important; all subnodes should be enumerated

Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt Example: · line 167

Quoted 4 lines

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

@@ -160,6 +164,7 @@ Example:
 		#gpio-cells = <2>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
+		wake-parent = <&pdc_intc>;

wakeup-parent

Quoted 7 lines

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

 		qup9_active: qup9-active {
 			mux {
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

---
Changes in v4:
- Remove irq_set_wake() on summary IRQ interrupt
Changes in v3:
- Use of_irq_domain_map() and pass PDC pin to parent irqdomain
Changes in v2:
- Call parent mask when masking GPIO interrupt
Changes in v1:
- Fix bug when unmasking PDC interrupt

Quoting Lina Iyer (2019-03-13 14:18:41)

Quoted 9 lines

---
Changes in v4:
- Remove irq_set_wake() on summary IRQ interrupt
Changes in v3:
- Use of_irq_domain_map() and pass PDC pin to parent irqdomain
Changes in v2:
- Call parent mask when masking GPIO interrupt
Changes in v1:
- Fix bug when unmasking PDC interrupt

[...]

drivers/pinctrl/qcom/pinctrl-msm.c static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) · line 1047

Quoted 5 lines

drivers/pinctrl/qcom/pinctrl-msm.c +5 -0

+}
+
+/*
+ * TODO: Get rid of this and push it into gpiochip_to_irq()
+ */  

Any chance this TODO can be resolved?

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 1098

Quoted 8 lines

drivers/pinctrl/qcom/pinctrl-msm.c +4 -0

@@ -994,6 +1092,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
        pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
        pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
+       chip->irq.chip = &pctrl->irq_chip;
+       chip->irq.domain_ops = &msm_gpio_domain_ops;
+       chip->irq.handler = handle_edge_irq;
+       chip->irq.default_type = IRQ_TYPE_EDGE_RISING;  

This also changed from v3. It used to be IRQ_TYPE_NONE. Specifying this here seems to cause gpiolib to print a WARN.

        /*
         * Specifying a default trigger is a terrible idea if DT or ACPI is
         * used to configure the interrupts, as you may end up with
         * conflicting triggers. Tell the user, and reset to NONE.
         */
        if (WARN(np && type != IRQ_TYPE_NONE,
                 "%s: Ignoring %u default trigger\n", np->full_name, type))
                type = IRQ_TYPE_NONE;

So I guess this change should be dropped. Or at the least, it should be split out to it's own patch and the motivations can be discussed in the commit text.

Quoted 24 lines · On Fri, 15 Mar 2019 09:28:31 -0700 Stephen Boyd <[email protected]> wrote:

Quoting Lina Iyer (2019-03-13 14:18:41)

Review static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 1098

Quoted 8 lines

(quoted diff) +4 -0

@@ -994,6 +1092,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
        pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
        pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
+       chip->irq.chip = &pctrl->irq_chip;
+       chip->irq.domain_ops = &msm_gpio_domain_ops;
+       chip->irq.handler = handle_edge_irq;
+       chip->irq.default_type = IRQ_TYPE_EDGE_RISING;  

This also changed from v3. It used to be IRQ_TYPE_NONE. Specifying this here seems to cause gpiolib to print a WARN.

        /*
         * Specifying a default trigger is a terrible idea if DT or ACPI is
         * used to configure the interrupts, as you may end up with
         * conflicting triggers. Tell the user, and reset to NONE.
         */
        if (WARN(np && type != IRQ_TYPE_NONE,
                 "%s: Ignoring %u default trigger\n", np->full_name, type))
                type = IRQ_TYPE_NONE;

Quoted 3 lines

So I guess this change should be dropped. Or at the least, it should be split out to it's own patch and the motivations can be discussed in the commit text.

It is something I requested (although I expected this to be a different patch, and even a clarification would have been OK).

One way or another, the default trigger must match the flow handler. If we set it up with IRQ_TYPE_NONE, what does it mean? The fact that IRQ_TYPE_NONE acts as a wildcard doesn't mean the handle_edge_irq flow handler is a good match for all interrupt types (it is rarely OK for level interrupts).

Quoting Marc Zyngier (2019-03-16 04:39:48)

Quoted 31 lines

Quoted 24 lines · On Fri, 15 Mar 2019 09:28:31 -0700 Stephen Boyd <[email protected]> wrote:

Quoting Lina Iyer (2019-03-13 14:18:41)

Review static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 1098

Quoted 8 lines

(quoted diff) +4 -0

@@ -994,6 +1092,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
        pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
        pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
+       chip->irq.chip = &pctrl->irq_chip;
+       chip->irq.domain_ops = &msm_gpio_domain_ops;
+       chip->irq.handler = handle_edge_irq;
+       chip->irq.default_type = IRQ_TYPE_EDGE_RISING;

This also changed from v3. It used to be IRQ_TYPE_NONE. Specifying this here seems to cause gpiolib to print a WARN.

        /*
         * Specifying a default trigger is a terrible idea if DT or ACPI is
         * used to configure the interrupts, as you may end up with
         * conflicting triggers. Tell the user, and reset to NONE.
         */
        if (WARN(np && type != IRQ_TYPE_NONE,
                 "%s: Ignoring %u default trigger\n", np->full_name, type))
                type = IRQ_TYPE_NONE;

Quoted 3 lines

So I guess this change should be dropped. Or at the least, it should be split out to it's own patch and the motivations can be discussed in the commit text.

Quoted 8 lines

It is something I requested (although I expected this to be a different patch, and even a clarification would have been OK).

One way or another, the default trigger must match the flow handler. If we set it up with IRQ_TYPE_NONE, what does it mean? The fact that IRQ_TYPE_NONE acts as a wildcard doesn't mean the handle_edge_irq flow handler is a good match for all interrupt types (it is rarely OK for level interrupts).

I think this is a question for Thierry or Linus. I'm not sure why this check was put in place in the code. I tried to dig into it really quick but I didn't find anything obvious and then I gave up.

Maybe with hierarchical irqdomains we can drop this check? I don't think the gpiolib core ever uses this 'default_type' or 'handler' for anything once we replace the irqdomain that's used for a particular gpiochip with a custom irqdomain. The only user I see, gpiochip_irq_map(), won't ever be called so it really ends up being a thing that the driver specific irqdomains should check for and reject when parsing the DT and it sees IRQ_TYPE_NONE come out.

------8<-------

drivers/gpio/gpiolib.c +0 -0

index 144af0733581..fe2f7888c473 100644

Quoting Marc Zyngier (2019-03-16 04:39:48)> > On Fri, 15 Mar 2019 09:28:31 -0700

Quoted 30 lines

Quoted 24 lines · Stephen Boyd

Quoting Lina Iyer (2019-03-13 14:18:41)

Review static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 1098

Quoted 8 lines

(quoted diff) +4 -0

@@ -994,6 +1092,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
        pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
        pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
+       chip->irq.chip = &pctrl->irq_chip;
+       chip->irq.domain_ops = &msm_gpio_domain_ops;
+       chip->irq.handler = handle_edge_irq;
+       chip->irq.default_type = IRQ_TYPE_EDGE_RISING;

This also changed from v3. It used to be IRQ_TYPE_NONE. Specifying this here seems to cause gpiolib to print a WARN.

        /*
         * Specifying a default trigger is a terrible idea if DT or ACPI is
         * used to configure the interrupts, as you may end up with
         * conflicting triggers. Tell the user, and reset to NONE.
         */
        if (WARN(np && type != IRQ_TYPE_NONE,
                 "%s: Ignoring %u default trigger\n", np->full_name, type))
                type = IRQ_TYPE_NONE;

Quoted 3 lines

So I guess this change should be dropped. Or at the least, it should be split out to it's own patch and the motivations can be discussed in the commit text.

Quoted 8 lines

It is something I requested (although I expected this to be a different patch, and even a clarification would have been OK).

One way or another, the default trigger must match the flow handler. If we set it up with IRQ_TYPE_NONE, what does it mean? The fact that IRQ_TYPE_NONE acts as a wildcard doesn't mean the handle_edge_irq flow handler is a good match for all interrupt types (it is rarely OK for level interrupts).

I think this is a question for Thierry or Linus. I'm not sure why this check was put in place in the code. I tried to dig into it really quick but I didn't find anything obvious and then I gave up.

Maybe with hierarchical irqdomains we can drop this check? I don't think the gpiolib core ever uses this 'default_type' or 'handler' for anything once we replace the irqdomain that's used for a particular gpiochip with a custom irqdomain. The only user I see, gpiochip_irq_map(), won't ever be called so it really ends up being a thing that the driver specific irqdomains should check for and reject when parsing the DT and it sees IRQ_TYPE_NONE come out.

------8<-------

drivers/gpio/gpiolib.c +0 -0

index 144af0733581..fe2f7888c473 100644

Quoted 58 lines · Stephen Boyd

Quoting Marc Zyngier (2019-03-16 04:39:48)> > On Fri, 15 Mar 2019 09:28:31 -0700

Quoted 30 lines

Quoted 24 lines · Stephen Boyd

Quoting Lina Iyer (2019-03-13 14:18:41)

Review static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 1098

Quoted 8 lines

(quoted diff) +4 -0

@@ -994,6 +1092,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
        pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
        pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
+       chip->irq.chip = &pctrl->irq_chip;
+       chip->irq.domain_ops = &msm_gpio_domain_ops;
+       chip->irq.handler = handle_edge_irq;
+       chip->irq.default_type = IRQ_TYPE_EDGE_RISING;

This also changed from v3. It used to be IRQ_TYPE_NONE. Specifying this here seems to cause gpiolib to print a WARN.

        /*
         * Specifying a default trigger is a terrible idea if DT or ACPI is
         * used to configure the interrupts, as you may end up with
         * conflicting triggers. Tell the user, and reset to NONE.
         */
        if (WARN(np && type != IRQ_TYPE_NONE,
                 "%s: Ignoring %u default trigger\n", np->full_name, type))
                type = IRQ_TYPE_NONE;

Quoted 3 lines

So I guess this change should be dropped. Or at the least, it should be split out to it's own patch and the motivations can be discussed in the commit text.

Quoted 8 lines

It is something I requested (although I expected this to be a different patch, and even a clarification would have been OK).

One way or another, the default trigger must match the flow handler. If we set it up with IRQ_TYPE_NONE, what does it mean? The fact that IRQ_TYPE_NONE acts as a wildcard doesn't mean the handle_edge_irq flow handler is a good match for all interrupt types (it is rarely OK for level interrupts).

I think this is a question for Thierry or Linus. I'm not sure why this check was put in place in the code. I tried to dig into it really quick but I didn't find anything obvious and then I gave up.

Maybe with hierarchical irqdomains we can drop this check? I don't think the gpiolib core ever uses this 'default_type' or 'handler' for anything once we replace the irqdomain that's used for a particular gpiochip with a custom irqdomain. The only user I see, gpiochip_irq_map(), won't ever be called so it really ends up being a thing that the driver specific irqdomains should check for and reject when parsing the DT and it sees IRQ_TYPE_NONE come out.

------8<-------

drivers/gpio/gpiolib.c +0 -0

index 144af0733581..fe2f7888c473 100644

drivers/gpio/gpiolib.c static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, · line 1927

Quoted 7 lines

drivers/gpio/gpiolib.c +1 -1

@@ -1922,7 +1922,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
         * used to configure the interrupts, as you may end up with
         * conflicting triggers. Tell the user, and reset to NONE.
         */
-       if (WARN(np && type != IRQ_TYPE_NONE,
+       if (WARN(!gpiochip->irq.domain_ops && np && type != IRQ_TYPE_NONE,
                 "%s: Ignoring %u default trigger\n", np->full_name, type))
                type = IRQ_TYPE_NONE;

Sorry for taking long time to answer... this got lost in some mail storms.

It's a bit of Marc Z question really but I try to answer and he can correct me.

We are now getting used to ACPI and DT always specifying the IRQ trigger type on the consumer handle: a device tells the irqchip what kind of edge or level it wants.

Things weren't always like that.

Some boards in the kernel is still using board files. (Yeah please help in modernizing them, I am doing my part.)

Old machines with GPIO irqchip jitted to the SoC irq controller sometimes had a hardcoded behavior such as edge, and the consumers would only issue something really legacy like

request_irq(42, myhandler, 0, "myirq", data);

and expect it to work, since 0 means use the default flags, it might have a platform device with this irq number passed as a resource, but that is a really dumb platform device still, and it might not have set any irqflags for the irq number it passes. It probably doesn't even know that the irq number is backed by an irq descriptor.

Since the code that e.g. DT has inside drivers/of/platform.c irq_of_parse_and_map(), will incidentally create an irq descriptor and set up these flags from the consumer flags in the device tree and call the irqchip to set up the trigger through .set_type() whenever the interrupt is requested, this is no problem for DT. Or ACPI.

But on a board file, the .set_type() will eventually be called with IRQ_TYPE_NONE, which will cause a bug, or no IRQs or something like that.

So a bunch of GPIO irqchips are created passing IRQ_TYPE_EDGE_* or IRQ_TYPE_LEVEL_* to set up a default trigger, because all the irqs on this chip use the same trigger anyway, and they only have one flow handler anyway. Everything is edge, or everything is level or so. irq_set_irq_type() will be called when mapping the GPIO to an irq, including calls from gpiod_to_irq() and friends that get used a lot in legacy code.

This happened by simply factoring custom GPIO irqchips into the gpiolib over time.

No-one has really gotten around to tracking down
all the offending callers of request_irq() and their respective interrupt providers and make sure the descriptors for all these IRQs get set up properly in drivers or board files. As far as I know. (INTERESTING WORK!)

It is a mess, really hard to fix, essentially everything need to be modernized or deleted for us to get rid of the possibility to pass a default trigger.

I guess it is possible to check all gpiochip_irqchip_add* and see if there are still chips passing something else than IRQ_TYPE_NONE. It would take some time to look at all of them, maybe it isn't used by anything anymore? Then we can simply delete this and assume it will always be set up orderly. We have modernized quite a few systems recently.

Quoting Lina Iyer (2019-03-13 14:18:41)

Quoted 9 lines

---
Changes in v4:
- Remove irq_set_wake() on summary IRQ interrupt
Changes in v3:
- Use of_irq_domain_map() and pass PDC pin to parent irqdomain
Changes in v2:
- Call parent mask when masking GPIO interrupt
Changes in v1:
- Fix bug when unmasking PDC interrupt

[...]

drivers/pinctrl/qcom/pinctrl-msm.c static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) · line 1047

Quoted 5 lines

drivers/pinctrl/qcom/pinctrl-msm.c +5 -0

+}
+
+/*
+ * TODO: Get rid of this and push it into gpiochip_to_irq()
+ */

Any chance this TODO can be resolved?

I am thinking of something like this. Would there be any issue in setting the type to IRQ_TYPE_SENSE_MASK instead of any one particular type?

---8<-----
static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) {

#ifdef CONFIG_OF_GPIO
        struct irq_fwspec fwspec;

        if (chip->of_node) {
               fwspec.fwnode = of_node_to_fwnode(chip->of_node);
               fwspec.param[0] = offset;
               fwspec.param[1] = IRQ_TYPE_SENSE_MASK;
               fwspec.param_count = 2;
               return irq_create_fwspec_mapping(&fwspec);
        }

#endif

        if (!gpiochip_irqchip_irq_valid(chip, offset))
                return -ENXIO;

        return irq_create_mapping(chip->irq.domain, offset);

Hi all,

This series adds support for wakeup capable GPIOs. It is based on Thierry's hiearchical GPIO domains. This approach is based on Stephen's idea [1]. The SoC that is used for this development is a QCOM SDM845. The current patchset is rebased on top of 5.0 and adds documentation for the wakeup-parent and irqdomain-map DT properties along with the the optional irqdomain-map-mask and irqdomain-map-pass-thru properties. Also incorporating comments from Marc on the earlier submission [2]. I cleaned up some of the change history in these patches to match the version number with that of the submission.

The dtsi patches are based on Bjorn's changes for increased address and cell size [3] and [4].

Kindly review the series.

Quoted 11 lines · Lina Iyer

Hi all,

This series adds support for wakeup capable GPIOs. It is based on Thierry's hiearchical GPIO domains. This approach is based on Stephen's idea [1]. The SoC that is used for this development is a QCOM SDM845. The current patchset is rebased on top of 5.0 and adds documentation for the wakeup-parent and irqdomain-map DT properties along with the the optional irqdomain-map-mask and irqdomain-map-pass-thru properties. Also incorporating comments from Marc on the earlier submission [2]. I cleaned up some of the change history in these patches to match the version number with that of the submission.

Quoted 4 lines

The dtsi patches are based on Bjorn's changes for increased address and cell size [3] and [4].

Kindly review the series.

What the status of this? What is the expected merge strategy?