qcom: support wakeup capable GPIOs

From: Thierry Reding <[email protected]>

Hierarchical IRQ domains can be used to stack different IRQ controllers on top of each other. One specific use-case where this can be useful is if a power management controller has top-level controls for wakeup interrupts. In such cases, the power management controller can be a parent to other interrupt controllers and program additional registers when an IRQ has its wake capability enabled or disabled.

Signed-off-by Thierry Reding <[email protected]>

---

drivers/irqchip/qcom-pdc-data.c new file +82 -0

new file mode 100644
index 000000000000..99b4be0af5db
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include "qcom-pdc.h"
+
+static const struct pdc_gpio_pin_map sdm845_gpio_pdc_map[] = {
+       { 1, 30 },
+       { 3, 31 },
+       { 5, 32 },
+       { 10, 33 },
+       { 11, 34 },
+       { 20, 35 },
+       { 22, 36 },
+       { 24, 37 },
+       { 26, 38 },
+       { 30, 39 },
+       { 31, 117 },
+       { 32, 41 },
+       { 34, 42 },
+       { 36, 43 },
+       { 37, 44 },
+       { 38, 45 },
+       { 39, 46 },
+       { 40, 47 },
+       { 41, 115 },
+       { 43, 49 },
+       { 44, 50 },
+       { 46, 51 },
+       { 48, 52 },
+       { 49, 118 },
+       { 52, 54 },
+       { 53, 55 },
+       { 54, 56 },
+       { 56, 57 },
+       { 57, 58 },
+       { 58, 59 },
+       { 59, 60 },
+       { 60, 61 },
+       { 61, 62 },
+       { 62, 63 },
+       { 63, 64 },
+       { 64, 65 },
+       { 66, 66 },
+       { 68, 67 },
+       { 71, 68 },
+       { 73, 69 },
+       { 77, 70 },
+       { 78, 71 },
+       { 79, 72 },
+       { 80, 73 },
+       { 84, 74 },
+       { 85, 75 },
+       { 86, 76 },
+       { 88, 77 },
+       { 89, 116 },
+       { 91, 79 },
+       { 92, 80 },
+       { 95, 81 },
+       { 96, 82 },
+       { 97, 83 },
+       { 101, 84 },
+       { 103, 85 },
+       { 104, 86 },
+       { 115, 90 },
+       { 116, 91 },
+       { 117, 92 },
+       { 118, 93 },
+       { 119, 94 },
+       { 120, 95 },
+       { 121, 96 },
+       { 122, 97 },
+       { 123, 98 },
+       { 124, 99 },
+       { 125, 100 },
+       { 127, 102 },
+       { 128, 103 },
+       { 129, 104 },
+       { 130, 105 },
+       { 132, 106 },
+       { 133, 107 },

Quoting Lina Iyer (2018-12-19 14:11:01)

Quoted 13 lines

drivers/irqchip/qcom-pdc-data.c new file +7 -0

new file mode 100644
index 000000000000..99b4be0af5db
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include "qcom-pdc.h"
+

drivers/irqchip/qcom-pdc-data.c line 92

Quoted 7 lines

drivers/irqchip/qcom-pdc-data.c new file +7 -0

+static const struct pdc_gpio_pin_data sdm845_gpio_data = {
+       .size = ARRAY_SIZE(sdm845_gpio_pdc_map),
+       .map = sdm845_gpio_pdc_map,
+};
+
+const struct of_device_id pdc_gpio_match_table[] = {
+       { .compatible = "qcom,845-pdc", .data = &sdm845_gpio_data },

Why not qcom,sdm845-pdc?

drivers/irqchip/qcom-pdc-data.c line 94

Quoted 2 lines

drivers/irqchip/qcom-pdc-data.c new file +2 -0

+       { },
+};

I wonder why we wouldn't just put this all into the qcom-pdc.c file at the bottom and then have that IRQCHIP_DECLARE() macros call small functions that pass the pdc to gpio mapping table to qcom_pdc_init() that takes a third argument?

I really hope that in the future all the gpios can be wakeup capable so that we don't need to have the table at all!

Quoted 12 lines

Quoting Lina Iyer (2018-12-19 14:11:01)

drivers/irqchip/qcom-pdc-data.c line 92

Quoted 7 lines

drivers/irqchip/qcom-pdc-data.c new file +7 -0

+static const struct pdc_gpio_pin_data sdm845_gpio_data = {
+       .size = ARRAY_SIZE(sdm845_gpio_pdc_map),
+       .map = sdm845_gpio_pdc_map,
+};
+
+const struct of_device_id pdc_gpio_match_table[] = {
+       { .compatible = "qcom,845-pdc", .data = &sdm845_gpio_data },

Why not qcom,sdm845-pdc?

The compatible matches the compatible specified in the PDC driver. Not sure why the 'sdm' was left out at that time.

Quoted 9 lines

drivers/irqchip/qcom-pdc-data.c line 94

Quoted 3 lines

drivers/irqchip/qcom-pdc-data.c new file +2 -0

+       { },
+};

I wonder why we wouldn't just put this all into the qcom-pdc.c file at the bottom and then have that IRQCHIP_DECLARE() macros call small functions that pass the pdc to gpio mapping table to qcom_pdc_init() that takes a third argument?

We could. I feel we would be adding tables like this and it just clutters up the driver file. May be in the future we could move to target specific data file like the gpios, but that could be excessive too. Thought this might be a compromise. I am open to change.

Quoted 4 lines

I really hope that in the future all the gpios can be wakeup capable so that we don't need to have the table at all!

I doubt there are plans to support that in hardware. We should plan for supporting tables like this for other chipsets based on the PDC architecture.

Quoting Lina Iyer (2019-01-07 10:48:36)

Quoted 13 lines · Stephen Boyd

Quoted 12 lines

Quoting Lina Iyer (2018-12-19 14:11:01)

drivers/irqchip/qcom-pdc-data.c line 92

Quoted 7 lines

drivers/irqchip/qcom-pdc-data.c new file +7 -0

+static const struct pdc_gpio_pin_data sdm845_gpio_data = {
+       .size = ARRAY_SIZE(sdm845_gpio_pdc_map),
+       .map = sdm845_gpio_pdc_map,
+};
+
+const struct of_device_id pdc_gpio_match_table[] = {
+       { .compatible = "qcom,845-pdc", .data = &sdm845_gpio_data },

Why not qcom,sdm845-pdc?

Quoted 2 lines

The compatible matches the compatible specified in the PDC driver. Not sure why the 'sdm' was left out at that time.

Are you going to add sdm?

Quoted 9 lines

Quoted 9 lines

drivers/irqchip/qcom-pdc-data.c line 94

Quoted 3 lines

drivers/irqchip/qcom-pdc-data.c new file +2 -0

+       { },
+};

I wonder why we wouldn't just put this all into the qcom-pdc.c file at the bottom and then have that IRQCHIP_DECLARE() macros call small functions that pass the pdc to gpio mapping table to qcom_pdc_init() that takes a third argument?

Quoted 4 lines

We could. I feel we would be adding tables like this and it just clutters up the driver file. May be in the future we could move to target specific data file like the gpios, but that could be excessive too. Thought this might be a compromise. I am open to change.

Ok. The benefit to my approach is that we don't do the string match twice. We do it once and sacrifice a little code space to jump to the real init function with the data we want. We can then put those chip tables inside some #ifdef to save space and allow configurations to turn off everything in EXPERT mode but leave everything default enabled otherwise.

Quoted 8 lines

Quoted 4 lines

I really hope that in the future all the gpios can be wakeup capable so that we don't need to have the table at all!

I doubt there are plans to support that in hardware. We should plan for supporting tables like this for other chipsets based on the PDC architecture.

Uh oh. That's sad.

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

index 665aadb5ea28..a522ca46667d 100644

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

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

@@ -160,6 +164,7 @@ Example:
 		#gpio-cells = <2>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
+		wakeup-parent = <&pdc>;
 
 		qup9_active: qup9-active {
 			mux {

Signature · 2 lines

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

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

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

index 665aadb5ea28..a522ca46667d 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>
 

Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt SDM845 platform. · line 35

Quoted 4 lines

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

+- wakeup-parent:
+	Usage: optional
+	Value type: <phandle>
+	Definition: A phandle to the wakeup interrupt controller for the SoC.

Is this really necessary? Is there more than one possible wakeup-parent node?

Quoted 24 lines

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

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

@@ -160,6 +164,7 @@ Example:
 		#gpio-cells = <2>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
+		wakeup-parent = <&pdc>;
 
 		qup9_active: qup9-active {
 			mux {
--

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

Quoted 19 lines · Rob Herring

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

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

index 665aadb5ea28..a522ca46667d 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>
 

Quoted 8 lines

Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt SDM845 platform. · line 35

Quoted 4 lines

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

+- wakeup-parent:
+    Usage: optional
+    Value type: <phandle>
+    Definition: A phandle to the wakeup interrupt controller for the SoC.

Is this really necessary? Is there more than one possible wakeup-parent node?

No. There is only one but depending on the architecture, the wakeup interrupt controller could be different device like PDC on SDM845 or MPM on SDM820.

What do you have in mind? Let me know if you have a better idea than referencing in DT.

Quoted 33 lines · Lina Iyer

Quoted 19 lines · Rob Herring

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

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

index 665aadb5ea28..a522ca46667d 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>
 

Quoted 8 lines

Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt SDM845 platform. · line 35

Quoted 4 lines

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

+- wakeup-parent:
+    Usage: optional
+    Value type: <phandle>
+    Definition: A phandle to the wakeup interrupt controller for the SoC.

Is this really necessary? Is there more than one possible wakeup-parent node?

No. There is only one but depending on the architecture, the wakeup interrupt controller could be different device like PDC on SDM845 or MPM on SDM820.

Quoted 2 lines

What do you have in mind? Let me know if you have a better idea than referencing in DT.

If there's only one possibility for a given platform, then you can just use of_find_compatible_node(). I don't think it matters that different platforms have a different device here. It's not going to be a large table and you may need to know the differences if there's not an abstracted interface to it (seems there is in your case).

Alternatively, if the PDC/MPM code knows what interrupt controller it is associated with, then it could setup that relationship and the interrupt controller code could retrieve that. Maybe the stacked domain support doesn't work in that direction (I haven't looked at the irq code much since that was added).

However, my main concern is documenting something genericish in a device specific binding. It looks like Tegra is trying to add the same thing, so this needs to be documented in a common place. One question is whether wakeup is the only use or if this should be more generally a secondary interrupt parent?

Quoted 34 lines · Rob Herring

Quoted 33 lines · Lina Iyer

Quoted 19 lines · Rob Herring

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

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

index 665aadb5ea28..a522ca46667d 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>
 

Quoted 8 lines

Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt SDM845 platform. · line 35

Quoted 4 lines

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

+- wakeup-parent:
+    Usage: optional
+    Value type: <phandle>
+    Definition: A phandle to the wakeup interrupt controller for the SoC.

Is this really necessary? Is there more than one possible wakeup-parent node?

No. There is only one but depending on the architecture, the wakeup interrupt controller could be different device like PDC on SDM845 or MPM on SDM820.

Quoted 8 lines

Quoted 2 lines

What do you have in mind? Let me know if you have a better idea than referencing in DT.

If there's only one possibility for a given platform, then you can just use of_find_compatible_node(). I don't think it matters that different platforms have a different device here. It's not going to be a large table and you may need to know the differences if there's not an abstracted interface to it (seems there is in your case).

The GPIO irqchip would be in hierarchy with the wakeup-parent irqchip and no device specific functions would be called directly. We could achieve this with compatible strings to the irqchip.

Quoted 6 lines

Alternatively, if the PDC/MPM code knows what interrupt controller it is associated with, then it could setup that relationship and the interrupt controller code could retrieve that. Maybe the stacked domain support doesn't work in that direction (I haven't looked at the irq code much since that was added).

The PDC/MPM do not know about the association.

Quoted 6 lines

However, my main concern is documenting something genericish in a device specific binding. It looks like Tegra is trying to add the same thing, so this needs to be documented in a common place. One question is whether wakeup is the only use or if this should be more generally a secondary interrupt parent?

Yes, wakeup is the only use of this interrupt parent.

It is powered by
an always-on rail and therefore can detect some interrupts that are routed to it even when the GIC is powered off. Though Tegra's implementation of the irqchip is a bit different from QCOM, the idea is generally the same. It would be helpful, if we could make this a generic enough binding.

-- Lina

Quoted 7 lines

Quoted 7 lines

However, my main concern is documenting something genericish in a device specific binding. It looks like Tegra is trying to add the same thing, so this needs to be documented in a common place. One question is whether wakeup is the only use or if this should be more generally a secondary interrupt parent?

Quoted 1 line

Yes, wakeup is the only use of this interrupt parent.

Maybe for you, but I was wondering about this more generally. Should we encode what the function (e.g. wakeup) is in the property name or have something like aux-interrupt-controller? Maybe some platforms have some need for a secondary interrupt-controller which is not wakeup. Routing interrupts to other cores perhaps?

Quoting Rob Herring (2019-01-09 11:36:56)

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

However, my main concern is documenting something genericish in a device specific binding. It looks like Tegra is trying to add the same thing, so this needs to be documented in a common place. One question is whether wakeup is the only use or if this should be more generally a secondary interrupt parent?

Quoted 8 lines

Quoted 1 line

Yes, wakeup is the only use of this interrupt parent.

Maybe for you, but I was wondering about this more generally. Should we encode what the function (e.g. wakeup) is in the property name or have something like aux-interrupt-controller? Maybe some platforms have some need for a secondary interrupt-controller which is not wakeup. Routing interrupts to other cores perhaps?

I'd say it's not the interrupt-parent, but a secondary-interrupt-parent, because it's another path that some GPIO interrupts will go through vs. the "normal" summary irq line that uses the interrupt-parent. Maybe that's similar to the interrupt partitioning that ARM is doing for PPIs that only go to some CPUs?

We don't really specify that some GPIO is corresponding to the secondary or primary interrupt controller for the GPIO controller in DT. If we did, then we could do something like the interrupt-map binding and have the index of that property be the gpio number and the interrupt parent that it maps to (either summary from the GIC or MPM pin number).

interrupt-map = <0 0 &gic GIC_SPI 208 0>,

                 <1 0 &pdc 3 0>;

interrupt-map-mask = <0xfffffff 0>;

And then we would pass the 2-cell GPIO interrupt specifier (gpio# and flags) through the table and remap it to arbitrary domain parents. We could use this same design for the SSBI and SPMI gpio interrupt controller where we're currently looking at hardcoding the base interrupt number in the driver (0xc0) and then adding the GPIO number to that to get the parent interrupt specifier.

It's sort of an abuse of interrupt-map, but I don't know if it really matters because there isn't a child of the gpio controller that is going to go through this table.

Quoting Stephen Boyd (2019-01-11 15:20:48)

Quoted 40 lines

Quoting Rob Herring (2019-01-09 11:36:56)

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

However, my main concern is documenting something genericish in a device specific binding. It looks like Tegra is trying to add the same thing, so this needs to be documented in a common place. One question is whether wakeup is the only use or if this should be more generally a secondary interrupt parent?

Quoted 8 lines

Quoted 1 line

Yes, wakeup is the only use of this interrupt parent.

Maybe for you, but I was wondering about this more generally. Should we encode what the function (e.g. wakeup) is in the property name or have something like aux-interrupt-controller? Maybe some platforms have some need for a secondary interrupt-controller which is not wakeup. Routing interrupts to other cores perhaps?

I'd say it's not the interrupt-parent, but a secondary-interrupt-parent, because it's another path that some GPIO interrupts will go through vs. the "normal" summary irq line that uses the interrupt-parent. Maybe that's similar to the interrupt partitioning that ARM is doing for PPIs that only go to some CPUs?

We don't really specify that some GPIO is corresponding to the secondary or primary interrupt controller for the GPIO controller in DT. If we did, then we could do something like the interrupt-map binding and have the index of that property be the gpio number and the interrupt parent that it maps to (either summary from the GIC or MPM pin number).

interrupt-map = <0 0 &gic GIC_SPI 208 0>,

                 <1 0 &pdc 3 0>;

interrupt-map-mask = <0xfffffff 0>;

And then we would pass the 2-cell GPIO interrupt specifier (gpio# and flags) through the table and remap it to arbitrary domain parents. We could use this same design for the SSBI and SPMI gpio interrupt controller where we're currently looking at hardcoding the base interrupt number in the driver (0xc0) and then adding the GPIO number to that to get the parent interrupt specifier.

Quoted 4 lines

It's sort of an abuse of interrupt-map, but I don't know if it really matters because there isn't a child of the gpio controller that is going to go through this table.

Rob, can you please respond?

I'm prototyping out some code to do the remapping based on this type of DT property, because it will make the irqdomain::alloc function a little simpler to implement by passing in a struct irq_fwspec and getting out a parent irq_fwspec and it will make the patches to add these mapping tables in C irrelevant. Plus, I think Lina will be happy that the pinctrl driver will know if some pin maps to the PDC or not without having to see if it fails to allocate in the parent irqdomain. But there will still need to be a property for 'wakeup-parent' unless we do something to expose irqdomain tokens into DT.

Quoting Stephen Boyd (2019-01-31 13:53:42)

Quoted 11 lines

I'm prototyping out some code to do the remapping based on this type of DT property, because it will make the irqdomain::alloc function a little simpler to implement by passing in a struct irq_fwspec and getting out a parent irq_fwspec and it will make the patches to add these mapping tables in C irrelevant. Plus, I think Lina will be happy that the pinctrl driver will know if some pin maps to the PDC or not without having to see if it fails to allocate in the parent irqdomain. But there will still need to be a property for 'wakeup-parent' unless we do something to expose irqdomain tokens into DT.

And here's the code to do this remapping idea, heavily based on the phandle remapping code. I didn't properly fix up the irq alloc function for the pinctrl driver here, but it should work out properly without much more diff. I realize now that the pass-thru mechanism will fail horribly when a specifier changes size types.

I guess we'll need to keep
that in mind if we convert the PDC driver to this too. Or we can leave the PDC driver as is and not worry about listing out the individual pins.

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

arch/arm64/boot/dts/qcom/sdm845.dtsi +67 -0

index a2241dd9c185..6b3a4227f433 100644
@@ -1213,6 +1213,83 @@
 			#interrupt-cells = <2>;
 			gpio-ranges = <&tlmm 0 0 150>;
 			wakeup-parent = <&pdc_intc>;
+			irqdomain-map = <1 0 &pdc_intc 30 0>,
+					<3 0 &pdc_intc 31 0>,
+					<5 0 &pdc_intc 32 0>,
+					<10 0 &pdc_intc 33 0>,
+					<11 0 &pdc_intc 34 0>,
+					<20 0 &pdc_intc 35 0>,
+					<22 0 &pdc_intc 36 0>,
+					<24 0 &pdc_intc 37 0>,
+					<26 0 &pdc_intc 38 0>,
+					<30 0 &pdc_intc 39 0>,
+					<31 0 &pdc_intc 117 0>,
+					<32 0 &pdc_intc 41 0>,
+					<34 0 &pdc_intc 42 0>,
+					<36 0 &pdc_intc 43 0>,
+					<37 0 &pdc_intc 44 0>,
+					<38 0 &pdc_intc 45 0>,
+					<39 0 &pdc_intc 46 0>,
+					<40 0 &pdc_intc 47 0>,
+					<41 0 &pdc_intc 115 0>,
+					<43 0 &pdc_intc 49 0>,
+					<44 0 &pdc_intc 50 0>,
+					<46 0 &pdc_intc 51 0>,
+					<48 0 &pdc_intc 52 0>,
+					<49 0 &pdc_intc 118 0>,
+					<52 0 &pdc_intc 54 0>,
+					<53 0 &pdc_intc 55 0>,
+					<54 0 &pdc_intc 56 0>,
+					<56 0 &pdc_intc 57 0>,
+					<57 0 &pdc_intc 58 0>,
+					<58 0 &pdc_intc 59 0>,
+					<59 0 &pdc_intc 60 0>,
+					<60 0 &pdc_intc 61 0>,
+					<61 0 &pdc_intc 62 0>,
+					<62 0 &pdc_intc 63 0>,
+					<63 0 &pdc_intc 64 0>,
+					<64 0 &pdc_intc 65 0>,
+					<66 0 &pdc_intc 66 0>,
+					<68 0 &pdc_intc 67 0>,
+					<71 0 &pdc_intc 68 0>,
+					<73 0 &pdc_intc 69 0>,
+					<77 0 &pdc_intc 70 0>,
+					<78 0 &pdc_intc 71 0>,
+					<79 0 &pdc_intc 72 0>,
+					<80 0 &pdc_intc 73 0>,
+					<84 0 &pdc_intc 74 0>,
+					<85 0 &pdc_intc 75 0>,
+					<86 0 &pdc_intc 76 0>,
+					<88 0 &pdc_intc 77 0>,
+					<89 0 &pdc_intc 116 0>,
+					<91 0 &pdc_intc 79 0>,
+					<92 0 &pdc_intc 80 0>,
+					<95 0 &pdc_intc 81 0>,
+					<96 0 &pdc_intc 82 0>,
+					<97 0 &pdc_intc 83 0>,
+					<101 0 &pdc_intc 84 0>,
+					<103 0 &pdc_intc 85 0>,
+					<104 0 &pdc_intc 86 0>,
+					<115 0 &pdc_intc 90 0>,
+					<116 0 &pdc_intc 91 0>,
+					<117 0 &pdc_intc 92 0>,
+					<118 0 &pdc_intc 93 0>,
+					<119 0 &pdc_intc 94 0>,
+					<120 0 &pdc_intc 95 0>,
+					<121 0 &pdc_intc 96 0>,
+					<122 0 &pdc_intc 97 0>,
+					<123 0 &pdc_intc 98 0>,
+					<124 0 &pdc_intc 99 0>,

drivers/of/irq.c +76 -0

+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 */
+	cur = to_of_node(in->fwnode);
+	ret = of_property_read_u32(cur, cells_name, &in_size);
+	if (ret < 0)
+		goto put;
+
+	/* Precalculate the match array - this simplifies match loop */
+	for (i = 0; i < in_size; i++)
+		initial_match_array[i] = cpu_to_be32(in->param[i]);
+
+	ret = -EINVAL;
+	/* 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++;
+		map_len--;
+
+		/* Check if not found */
+		if (!new)
+			goto put;
+
+		if (!of_device_is_available(new))
+			match = 0;
+
+		ret = of_property_read_u32(new, cells_name, &out_size);
+		if (ret)
+			goto put;

drivers/of/irq.c +9 -0

+	}
+	if (match) {
+		/* 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

drivers/of/irq.c +19 -0

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

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

index 9b45219893bd..0473b180dc8d 100644
@@ -1011,6 +1011,12 @@ static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	if (!domain->parent)
 		return 0;
 
+	ret = of_irq_domain_map(fwspec, &parent.fwspec);
+	if (ret)
+		return ret;
+
+	pr_info("incoming is %d %#x and outgoing is %d %#x\n", fwspec->param[0], fwspec->param[1], parent.fwspec.param[0], parent.fwspec.param[1]);
+
 	parent.fwspec.fwnode      = domain->parent->fwnode;
 	parent.fwspec.param_count = 2;
 	parent.fwspec.param[0]    = hwirq;

include/linux/of_irq.h +2 -0

index 1214cabb2247..80ba1e9be68d 100644
@@ -32,6 +32,8 @@ 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);

Thanks for the patch Stephen. Sorry, it took a while to get to this and understand how this works.

Quoted 13 lines · Stephen Boyd

Quoting Stephen Boyd (2019-01-31 13:53:42)

Quoted 11 lines

I'm prototyping out some code to do the remapping based on this type of DT property, because it will make the irqdomain::alloc function a little simpler to implement by passing in a struct irq_fwspec and getting out a parent irq_fwspec and it will make the patches to add these mapping tables in C irrelevant. Plus, I think Lina will be happy that the pinctrl driver will know if some pin maps to the PDC or not without having to see if it fails to allocate in the parent irqdomain. But there will still need to be a property for 'wakeup-parent' unless we do something to expose irqdomain tokens into DT.

Quoted 5 lines

And here's the code to do this remapping idea, heavily based on the phandle remapping code. I didn't properly fix up the irq alloc function for the pinctrl driver here, but it should work out properly without much more diff. I realize now that the pass-thru mechanism will fail horribly when a specifier changes size types.

Could you explain?

Quoted 4 lines

I guess we'll need to keep
that in mind if we convert the PDC driver to this too. Or we can leave the PDC driver as is and not worry about listing out the individual pins.

The PDC pins are more sequential and it looks clean as it. I would prefer that it be left as is.

Quoted 6 lines

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

arch/arm64/boot/dts/qcom/sdm845.dtsi +0 -0

index a2241dd9c185..6b3a4227f433 100644

arch/arm64/boot/dts/qcom/sdm845.dtsi line 1292

Quoted 13 lines

arch/arm64/boot/dts/qcom/sdm845.dtsi +13 -0

+                                      <122 0 &pdc_intc 97 0>,
+                                      <123 0 &pdc_intc 98 0>,
+                                      <124 0 &pdc_intc 99 0>,
+                                      <125 0 &pdc_intc 100 0>,
+                                      <127 0 &pdc_intc 102 0>,
+                                      <128 0 &pdc_intc 103 0>,
+                                      <129 0 &pdc_intc 104 0>,
+                                      <130 0 &pdc_intc 105 0>,
+                                      <132 0 &pdc_intc 106 0>,
+                                      <133 0 &pdc_intc 107 0>,
+                                      <145 0 &pdc_intc 108 0>;
+                      irqdomain-map-mask = <0xff 0>;
+                      irqdomain-map-pass-thru = <0 0xff>;

Where do we document these bindings?

drivers/of/irq.c int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) · line 276

Quoted 11 lines

                      qspi_clk: qspi-clk {
                              pinmux {

drivers/of/irq.c +0 -0

index 02ad93a304a4..b37f4cdfda53 100644
@@ -274,6 +274,130 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 }
 EXPORT_SYMBOL_GPL(of_irq_parse_raw);
 

I would think this would be a separate patch and I presume, I can add you as the author with your signed-off-by?

Quoted 3 lines

drivers/of/irq.c +2 -0

+int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out)
+{

Quoted 25 lines

drivers/of/irq.c +25 -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++;
+              map_len--;
+
+              /* Check if not found */
+              if (!new)
+                      goto put;
+
+              if (!of_device_is_available(new))
+                      match = 0;
+
+              ret = of_property_read_u32(new, cells_name, &out_size);
+              if (ret)
+                      goto put;
+
+              /* Check for malformed properties */
+              if (WARN_ON(out_size > MAX_PHANDLE_ARGS))
+                      goto put;

drivers/of/irq.c int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) · line 362

Quoted 6 lines

drivers/of/irq.c +6 -0

+              if (map_len < out_size)
+                      goto put;
+
+              /* Move forward by new node's #interrupt-cells amount */
+              map += out_size;
+              map_len -= out_size;

Nit: Could make this a bit simpler to understand if you broke out the loop on match instead of continuing the loop.

Quoted 12 lines

drivers/of/irq.c +12 -0

+      }
+      if (match) {
+              /* 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.
+               */

drivers/of/irq.c int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) · line 381

Quoted 7 lines

drivers/of/irq.c +7 -0

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

I don't get why the mask is inverted and reversed here again?

Quoted 3 lines

drivers/of/irq.c +3 -0

+                              val |= cpu_to_be32(out->param[i]) & pass[i];
+                      }
+

Quoted 48 lines · Lina Iyer

Quoted 34 lines · Rob Herring

Quoted 33 lines · Lina Iyer

Quoted 19 lines · Rob Herring

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

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

index 665aadb5ea28..a522ca46667d 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>
 

Quoted 8 lines

Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt SDM845 platform. · line 35

Quoted 4 lines

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

+- wakeup-parent:
+    Usage: optional
+    Value type: <phandle>
+    Definition: A phandle to the wakeup interrupt controller for the SoC.

Is this really necessary? Is there more than one possible wakeup-parent node?

No. There is only one but depending on the architecture, the wakeup interrupt controller could be different device like PDC on SDM845 or MPM on SDM820.

Quoted 8 lines

Quoted 2 lines

What do you have in mind? Let me know if you have a better idea than referencing in DT.

If there's only one possibility for a given platform, then you can just use of_find_compatible_node(). I don't think it matters that different platforms have a different device here. It's not going to be a large table and you may need to know the differences if there's not an abstracted interface to it (seems there is in your case).

The GPIO irqchip would be in hierarchy with the wakeup-parent irqchip and no device specific functions would be called directly. We could achieve this with compatible strings to the irqchip.

Quoted 7 lines

Quoted 6 lines

Alternatively, if the PDC/MPM code knows what interrupt controller it is associated with, then it could setup that relationship and the interrupt controller code could retrieve that. Maybe the stacked domain support doesn't work in that direction (I haven't looked at the irq code much since that was added).

The PDC/MPM do not know about the association.

Neither does the main interrupt controller. The association is part of SoC integration. You can describe that association in either direction and that is sufficient from a DT standpoint. You've probably picked putting this in the GIC(?) based on what works more easily with the Linux irqdomain code.

Quoted 6 lines

Quoted 6 lines

However, my main concern is documenting something genericish in a device specific binding. It looks like Tegra is trying to add the same thing, so this needs to be documented in a common place. One question is whether wakeup is the only use or if this should be more generally a secondary interrupt parent?

Quoted 1 line

Yes, wakeup is the only use of this interrupt parent.

Maybe for you, but I was wondering about this more generally. Should we encode what the function (e.g. wakeup) is in the property name or have something like aux-interrupt-controller? Maybe some platforms have some need for a secondary interrupt-controller which is not wakeup. Routing interrupts to other cores perhaps?

Quoted 9 lines

It is powered by
an always-on rail and therefore can detect some interrupts that are routed to it even when the GIC is powered off. Though Tegra's implementation of the irqchip is a bit different from QCOM, the idea is generally the same. It would be helpful, if we could make this a generic enough binding.

-- Lina

To allow GPIOs to wakeup the system from suspend or deep idle, the wakeup capable GPIOs are setup in hierarchy with interrupts from the wakeup-parent irqchip.

In older SoC's, the TLMM will handover detection to the parent irqchip and in newer SoC's, the parent irqchip may also be active as well as the TLMM and therefore the GPIOs need to be masked at TLMM to avoid duplicate interrupts. To enable both these configurations to exist, allow the parent irqchip to dictate the TLMM irqchip's behavior when masking/unmasking the interrupt.

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

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

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

@@ -967,11 +994,86 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
        return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
 }
 
+static int msm_gpio_domain_translate(struct irq_domain *d,
+                                    struct irq_fwspec *fwspec,
+                                    unsigned long *hwirq, unsigned int *type)
+{

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

+
+static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
+                                unsigned int nr_irqs, void *arg)
+{
+       int ret;
+       irq_hw_number_t hwirq;
+       struct gpio_chip *gc = domain->host_data;
+       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+       struct irq_fwspec *fwspec = arg;
+       struct qcom_irq_fwspec parent = { };
+       unsigned int type;
+
+       ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type);
+       if (ret)
+               return ret;
+
+       ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+                                           &pctrl->irq_chip, gc);
+       if (ret < 0)
+               return ret;
+
+       if (!domain->parent)
+               return 0;
+
+       parent.fwspec.fwnode      = domain->parent->fwnode;
+       parent.fwspec.param_count = 2;
+       parent.fwspec.param[0]    = hwirq;
+       parent.fwspec.param[1]    = type;
+
+       ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
+       if (ret)

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

+ */
+static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+       struct irq_fwspec fwspec;
+
+       fwspec.fwnode = of_node_to_fwnode(chip->of_node);
+       fwspec.param[0] = offset;
+       fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
+       fwspec.param_count = 2;
+
+       return irq_create_fwspec_mapping(&fwspec);
+}
+
+static const struct irq_domain_ops msm_gpio_domain_ops = {
+       .translate = msm_gpio_domain_translate,
+       .alloc     = msm_gpio_domain_alloc,
+       .free      = irq_domain_free_irqs_top,
+};
+

Quoting Lina Iyer (2018-12-19 14:11:03)

Quoted 4 lines

To allow GPIOs to wakeup the system from suspend or deep idle, the wakeup capable GPIOs are setup in hierarchy with interrupts from the wakeup-parent irqchip.

Quoted 8 lines

In older SoC's, the TLMM will handover detection to the parent irqchip and in newer SoC's, the parent irqchip may also be active as well as the TLMM and therefore the GPIOs need to be masked at TLMM to avoid duplicate interrupts. To enable both these configurations to exist, allow the parent irqchip to dictate the TLMM irqchip's behavior when masking/unmasking the interrupt.

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

I don't think I gave a signed-off-by, so you need to ask to forge my sign off here. Please change it to be:

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

and I'm not sure how much I wrote vs. you wrote anymore so perhaps also add a

Co-developed-by Stephen Boyd <[email protected]>

And finally, please just email my chromium.org email for this series because I apparently messed up the address once and now it's all going to the wrong inbox. Thanks!

Quoted 1 line

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

Can you Cc Linus Walleij and Bjorn Andersson on the whole patch series next time? Would be good to have their review on major pinctrl driver changes.

Quoted 8 lines

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

@@ -967,11 +994,86 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
        return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
 }
 
+static int msm_gpio_domain_translate(struct irq_domain *d,
+                                    struct irq_fwspec *fwspec,
+                                    unsigned long *hwirq, unsigned int *type)
+{

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

Quoted 10 lines

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

+       if (is_of_node(fwspec->fwnode)) {
+               if (fwspec->param_count < 2)
+                       return -EINVAL;
+               *hwirq = fwspec->param[0];
+               *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+               return 0;
+       }
+
+       return -EINVAL;
+}

Maybe this can be a generic function in gpiolib?

Quoted 31 lines

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

+
+static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
+                                unsigned int nr_irqs, void *arg)
+{
+       int ret;
+       irq_hw_number_t hwirq;
+       struct gpio_chip *gc = domain->host_data;
+       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+       struct irq_fwspec *fwspec = arg;
+       struct qcom_irq_fwspec parent = { };
+       unsigned int type;
+
+       ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type);
+       if (ret)
+               return ret;
+
+       ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+                                           &pctrl->irq_chip, gc);
+       if (ret < 0)
+               return ret;
+
+       if (!domain->parent)
+               return 0;
+
+       parent.fwspec.fwnode      = domain->parent->fwnode;
+       parent.fwspec.param_count = 2;
+       parent.fwspec.param[0]    = hwirq;
+       parent.fwspec.param[1]    = type;
+
+       ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
+       if (ret)

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

Quoted 10 lines

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

+               return ret;
+
+       if (parent.mask)
+               set_bit(hwirq, pctrl->wakeup_masked_irqs);
+
+       return 0;
+}
+
+/*
+ * TODO: Get rid of this and push it into gpiochip_to_irq()

Hmm.. yeah we need to do this still. I think we can have a generic two cell function similar to irq_domain_xlate_twocell() that does the fwspec creation and uses some of the things that we pass to gpiochip_irqchip_add(), like the default level type. This existing function is not good to have, so there's work to do to get rid of this.

I was also thinking that maybe we can make the alloc function above take a struct gpio_irq_fwspec structure that tells the alloc function what gpiochip the irq is for. That would mean that we need to change the gpio_to_irq() function below to be generic and stuff the chip inside the fwspec wrapper structure:

	struct gpio_irq_fwspec {
		struct irq_fwspec fwspec;
		struct gpio_chip *chip;
		unsigned int offset;
	};

but I seem to recall that was not working for some reason.

Quoted 11 lines

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

+ */
+static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+       struct irq_fwspec fwspec;
+
+       fwspec.fwnode = of_node_to_fwnode(chip->of_node);
+       fwspec.param[0] = offset;
+       fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
+       fwspec.param_count = 2;
+
+       return irq_create_fwspec_mapping(&fwspec);

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

Quoted 8 lines

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

+}
+
+static const struct irq_domain_ops msm_gpio_domain_ops = {
+       .translate = msm_gpio_domain_translate,
+       .alloc     = msm_gpio_domain_alloc,
+       .free      = irq_domain_free_irqs_top,
+};
+

Quoting Lina Iyer (2018-12-19 14:11:03)

Quoted 31 lines

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

+
+static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
+                                unsigned int nr_irqs, void *arg)
+{
+       int ret;
+       irq_hw_number_t hwirq;
+       struct gpio_chip *gc = domain->host_data;
+       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+       struct irq_fwspec *fwspec = arg;
+       struct qcom_irq_fwspec parent = { };
+       unsigned int type;
+
+       ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type);
+       if (ret)
+               return ret;
+
+       ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+                                           &pctrl->irq_chip, gc);
+       if (ret < 0)
+               return ret;
+
+       if (!domain->parent)
+               return 0;
+
+       parent.fwspec.fwnode      = domain->parent->fwnode;
+       parent.fwspec.param_count = 2;
+       parent.fwspec.param[0]    = hwirq;
+       parent.fwspec.param[1]    = type;
+
+       ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
+       if (ret)

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

Quoted 10 lines

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

+               return ret;
+
+       if (parent.mask)
+               set_bit(hwirq, pctrl->wakeup_masked_irqs);
+
+       return 0;
+}
+
+/*
+ * TODO: Get rid of this and push it into gpiochip_to_irq()

Hmm.. yeah we need to do this still. I think we can have a generic two cell function similar to irq_domain_xlate_twocell() that does the fwspec creation and uses some of the things that we pass to gpiochip_irqchip_add(), like the default level type. This existing function is not good to have, so there's work to do to get rid of this.

I was also thinking that maybe we can make the alloc function above take a struct gpio_irq_fwspec structure that tells the alloc function what gpiochip the irq is for. That would mean that we need to change the gpio_to_irq() function below to be generic and stuff the chip inside the fwspec wrapper structure:

	struct gpio_irq_fwspec {
		struct irq_fwspec fwspec;
		struct gpio_chip *chip;
		unsigned int offset;
	};

but I seem to recall that was not working for some reason.

Quoted 19 lines

+ */

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

+static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+       struct irq_fwspec fwspec;
+
+       fwspec.fwnode = of_node_to_fwnode(chip->of_node);
+       fwspec.param[0] = offset;
+       fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
+       fwspec.param_count = 2;
+
+       return irq_create_fwspec_mapping(&fwspec);
+}
+
+static const struct irq_domain_ops msm_gpio_domain_ops = {
+       .translate = msm_gpio_domain_translate,
+       .alloc     = msm_gpio_domain_alloc,
+       .free      = irq_domain_free_irqs_top,
+};
+

Quoted 56 lines

Quoting Lina Iyer (2018-12-19 14:11:03)

Quoted 31 lines

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

+
+static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
+                                unsigned int nr_irqs, void *arg)
+{
+       int ret;
+       irq_hw_number_t hwirq;
+       struct gpio_chip *gc = domain->host_data;
+       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+       struct irq_fwspec *fwspec = arg;
+       struct qcom_irq_fwspec parent = { };
+       unsigned int type;
+
+       ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type);
+       if (ret)
+               return ret;
+
+       ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+                                           &pctrl->irq_chip, gc);
+       if (ret < 0)
+               return ret;
+
+       if (!domain->parent)
+               return 0;
+
+       parent.fwspec.fwnode      = domain->parent->fwnode;
+       parent.fwspec.param_count = 2;
+       parent.fwspec.param[0]    = hwirq;
+       parent.fwspec.param[1]    = type;
+
+       ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
+       if (ret)

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

Quoted 10 lines

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

+               return ret;
+
+       if (parent.mask)
+               set_bit(hwirq, pctrl->wakeup_masked_irqs);
+
+       return 0;
+}
+
+/*
+ * TODO: Get rid of this and push it into gpiochip_to_irq()

Hmm.. yeah we need to do this still. I think we can have a generic two cell function similar to irq_domain_xlate_twocell() that does the fwspec creation and uses some of the things that we pass to gpiochip_irqchip_add(), like the default level type. This existing function is not good to have, so there's work to do to get rid of this.

I was also thinking that maybe we can make the alloc function above take a struct gpio_irq_fwspec structure that tells the alloc function what gpiochip the irq is for. That would mean that we need to change the gpio_to_irq() function below to be generic and stuff the chip inside the fwspec wrapper structure:

Quoted 8 lines

      struct gpio_irq_fwspec {
              struct irq_fwspec fwspec;
              struct gpio_chip *chip;
              unsigned int offset;
      };

but I seem to recall that was not working for some reason.

I was thinking about this. If I understand you correctly, we want to generalize the .translate and .alloc functions. We could move the .translate to generic however, the alloc would still need to be specific for the parent.mask. But we can do this without the gpio_irq_fwspec. I presume you suggest this structure so we could pass the hwirq and type to the .alloc function. but we have that in the fwspec. What am I missing?

Add PDC interrupt controller device bindings for SDM845.

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

---
Changes in v3:
- Fix PDC map, use GIC SPI port number for hwirq
Changes in v2:
- Order by address

---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

Hi,

Quoted 4 lines · Lina Iyer

Add PDC interrupt controller device bindings for SDM845.

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

Quoted 8 lines

---
Changes in v3:
- Fix PDC map, use GIC SPI port number for hwirq
Changes in v2:
- Order by address

---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

nit: ${SUBJECT} makes it sounds like you're adding something into the "Documentation/devicetree/bindings" folder, but you're not. Also you probably want the prefix "qcom", not "msm" since it ends up in the "qcom" dir. Also, subject should say that this is the interrupt controller. How about:

arm64: dts: qcom: add PDC interrupt controller for sdm845

arch/arm64/boot/dts/qcom/sdm845.dtsi line 1279

Quoted 7 lines

arch/arm64/boot/dts/qcom/sdm845.dtsi +0 -0

index b72bdb0a31a5..8e15392a6f64 100644
@@ -1278,6 +1278,15 @@
                        #reset-cells = <1>;
                };

nit: the above node looks like the tail end of pdc reset controller. That has a unit address of b2e0000. Your unit address is smaller than the the pdc reset controller so you should be above it, not below it.

arch/arm64/boot/dts/qcom/sdm845.dtsi line 1281

Quoted 2 lines

arch/arm64/boot/dts/qcom/sdm845.dtsi +1 -0

+               pdc: interrupt-controller@b220000 {

nit: Maybe the label should be "pdc_intc" not just "pdc". This is just the node for the interrupt controller, not the whole pdc, right?

arch/arm64/boot/dts/qcom/sdm845.dtsi line 1283

Quoted 2 lines

arch/arm64/boot/dts/qcom/sdm845.dtsi +2 -0

+                       compatible = "qcom,sdm845-pdc";
+                       reg = <0xb220000 0x30000>;

nit: apparently common practice for Quaclomm dts is to pad the address in the "reg" field to all 8 digits. So the above should be:

reg = <0x0b220000 0x30000>;

NOTE: it's important to _not_ pad the unit address in the node name (so you got that right). Only update the "reg". For context:

https://lkml.kernel.org/r/CAD=FV=WrvRH6QpaQ67yw2MFz8RP59ozkSfQC4+OAM_8fAbGZuw@mail.gmail.com

-Doug

Quoted 6 lines

arch/arm64/boot/dts/qcom/sdm845.dtsi line 1283

Quoted 2 lines

arch/arm64/boot/dts/qcom/sdm845.dtsi +2 -0

+                       compatible = "qcom,sdm845-pdc";
+                       reg = <0xb220000 0x30000>;

nit: apparently common practice for Quaclomm dts is to pad the address in the "reg" field to all 8 digits. So the above should be:

Quoted 5 lines

reg = <0x0b220000 0x30000>;

NOTE: it's important to _not_ pad the unit address in the node name (so you got that right). Only update the "reg". For context:

Ok.

Will take care of these in the next spin.