[PATCH v4 00/10] support wakeup capable GPIOs

[PATCH v4 00/10] support wakeup capable GPIOs

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

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

Kindly review the series.

Thanks,
Lina

[1]. https://lkml.org/lkml/2018/12/19/813
[2]. https://lkml.org/lkml/2019/2/22/716
[3]. https://patchwork.kernel.org/patch/10767511/
[4]. https://lkml.kernel.org/r/20190117042940.25487-2-bjorn.andersson@linaro.org

Lina Iyer (8):
  irqdomain: add bus token DOMAIN_BUS_WAKEUP
  of/irq: document properties for wakeup interrupt parent
  drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs
  dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
  drivers: pinctrl: msm: setup GPIO irqchip hierarchy
  arm64: dts: qcom: add PDC interrupt controller for SDM845
  arm64: dts: qcom: setup PDC as wakeup parent for GPIOs for SDM845
  arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845

Stephen Boyd (1):
  of: irq: add helper to remap interrupts to another irqdomain

Thierry Reding (1):
  gpio: Add support for hierarchical IRQ domains

 .../interrupt-controller/interrupts.txt       |  39 +++++
 .../bindings/pinctrl/qcom,sdm845-pinctrl.txt  |   7 +-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |  87 ++++++++++
 arch/arm64/configs/defconfig                  |   1 +
 drivers/gpio/gpiolib.c                        |  15 +-
 drivers/irqchip/qcom-pdc.c                    |  72 ++++++++-
 drivers/of/irq.c                              | 125 +++++++++++++++
 drivers/pinctrl/qcom/pinctrl-msm.c            | 151 +++++++++++++++---
 include/linux/gpio/driver.h                   |   6 +
 include/linux/irqdomain.h                     |   1 +
 include/linux/of_irq.h                        |   1 +
 include/linux/soc/qcom/irq.h                  |  23 +++
 12 files changed, 497 insertions(+), 31 deletions(-)
 create mode 100644 include/linux/soc/qcom/irq.h

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

[PATCH v4 01/10] gpio: Add support for hierarchical IRQ domains

[Lina Iyer]

From: Thierry Reding <treding@nvidia.com>

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 <treding@nvidia.com>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/gpio/gpiolib.c      | 15 +++++++++++----
 include/linux/gpio/driver.h |  6 ++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d1adfdf50fb3..46ccb9b174b9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1895,7 +1895,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 		type = IRQ_TYPE_NONE;
 	}
 
-	gpiochip->to_irq = gpiochip_to_irq;
+	if (!gpiochip->to_irq)
+		gpiochip->to_irq = gpiochip_to_irq;
+
 	gpiochip->irq.default_type = type;
 	gpiochip->irq.lock_key = lock_key;
 	gpiochip->irq.request_key = request_key;
@@ -1905,9 +1907,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 	else
 		ops = &gpiochip_domain_ops;
 
-	gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
-						     gpiochip->irq.first,
-						     ops, gpiochip);
+	if (gpiochip->irq.parent_domain)
+		gpiochip->irq.domain = irq_domain_add_hierarchy(gpiochip->irq.parent_domain,
+								0, gpiochip->ngpio,
+								np, ops, gpiochip);
+	else
+		gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
+							     gpiochip->irq.first,
+							     ops, gpiochip);
 	if (!gpiochip->irq.domain)
 		return -EINVAL;
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 07cddbf45186..88ef196f96ec 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -48,6 +48,12 @@ struct gpio_irq_chip {
 	 */
 	const struct irq_domain_ops *domain_ops;
 
+	/**
+	 * @parent_domain:
+	 *
+	 */
+	struct irq_domain *parent_domain;
+
 	/**
 	 * @handler:
 	 *
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 02/10] irqdomain: add bus token DOMAIN_BUS_WAKEUP

[Lina Iyer]

A single controller can handle normal interrupts and wake-up interrupts
independently, with a different numbering space. It is thus crucial to
allow the driver for such a controller discriminate between the two.

A simple way to do so is to tag the wake-up irqdomain with a "bus token"
that indicates the wake-up domain. This slightly abuses the notion of
bus, but also radically simplifies the design of such a driver. Between
two evils, we choose the least damaging.

Suggested-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v4:
	- Update commit text
---
 include/linux/irqdomain.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 35965f41d7be..05055bc992ab 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -82,6 +82,7 @@ enum irq_domain_bus_token {
 	DOMAIN_BUS_NEXUS,
 	DOMAIN_BUS_IPI,
 	DOMAIN_BUS_FSL_MC_MSI,
+	DOMAIN_BUS_WAKEUP,
 };
 
 /**
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent

[Lina Iyer]

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 <ilina@codeaurora.org>
---
Changes in v4:
	- Added this documentation
---
 .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
index 8a3c40829899..917b598317f5 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
@@ -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>;
+	irqdomain-map-mask = <0xff 0>;
+	irqdomain-map-pass-thru = <0 0xff>;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent

[Marc Zyngier]

On Wed, 13 Mar 2019 15:18:37 -0600
Lina Iyer <ilina@codeaurora.org> wrote:

Please do Cc Rob when posting DT related patches.

> 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 <ilina@codeaurora.org>
> ---
> Changes in v4:
> 	- Added this documentation
> ---
>  .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> index 8a3c40829899..917b598317f5 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> @@ -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>;
> +	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>?

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent

[Lina Iyer]

On Mon, Mar 18 2019 at 11:54 -0600, Marc Zyngier wrote:
>On Wed, 13 Mar 2019 15:18:37 -0600
>Lina Iyer <ilina@codeaurora.org> wrote:
>
>Please do Cc Rob when posting DT related patches.
>
>> 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 <ilina@codeaurora.org>
>> ---
>> Changes in v4:
>> 	- Added this documentation
>> ---
>>  .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> index 8a3c40829899..917b598317f5 100644
>> --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> @@ -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>;
>> +	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.


--Lina

Re: [PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent

[Marc Zyngier]

On 04/04/2019 16:58, Lina Iyer wrote:
> On Mon, Mar 18 2019 at 11:54 -0600, Marc Zyngier wrote:
>> On Wed, 13 Mar 2019 15:18:37 -0600
>> Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> Please do Cc Rob when posting DT related patches.
>>
>>> 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 <ilina@codeaurora.org>
>>> ---
>>> Changes in v4:
>>> 	- Added this documentation
>>> ---
>>>  .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
>>>  1 file changed, 39 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>>> index 8a3c40829899..917b598317f5 100644
>>> --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>>> @@ -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>;
>>> +	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.

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

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent

[Lina Iyer]

On Mon, Apr 15 2019 at 06:42 -0600, Marc Zyngier wrote:
>On 04/04/2019 16:58, Lina Iyer wrote:
>> On Mon, Mar 18 2019 at 11:54 -0600, Marc Zyngier wrote:
>>> On Wed, 13 Mar 2019 15:18:37 -0600
>>> Lina Iyer <ilina@codeaurora.org> wrote:
>>>
>>> Please do Cc Rob when posting DT related patches.
>>>
>>>> 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 <ilina@codeaurora.org>
>>>> ---
>>>> Changes in v4:
>>>> 	- Added this documentation
>>>> ---
>>>>  .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
>>>>  1 file changed, 39 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>>>> index 8a3c40829899..917b598317f5 100644
>>>> --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>>>> @@ -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>;
>>>> +	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.
>
>I think you need to add #interrupt-cells in your example, which is
>otherwise hard to interpret.
>
Ok. Thanks, will add.

--Lina

Re: [PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent

[Stephen Boyd]

Quoting Lina Iyer (2019-04-04 08:58:38)
> On Mon, Mar 18 2019 at 11:54 -0600, Marc Zyngier wrote:
> >On Wed, 13 Mar 2019 15:18:37 -0600
> >Lina Iyer <ilina@codeaurora.org> wrote:
> >
> >Please do Cc Rob when posting DT related patches.
> >
> >> 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 <ilina@codeaurora.org>
> >> ---
> >> Changes in v4:
> >>      - Added this documentation
> >> ---
> >>  .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
> >>  1 file changed, 39 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> >> index 8a3c40829899..917b598317f5 100644
> >> --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> >> +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> >> @@ -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>;
> >> +    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.
> 
> 

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.

Re: [PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent

[Lina Iyer]

On Tue, Apr 16 2019 at 10:54 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-04-04 08:58:38)
>> On Mon, Mar 18 2019 at 11:54 -0600, Marc Zyngier wrote:
>> >On Wed, 13 Mar 2019 15:18:37 -0600
>> >Lina Iyer <ilina@codeaurora.org> wrote:
>> >
>> >Please do Cc Rob when posting DT related patches.
>> >
>> >> 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 <ilina@codeaurora.org>
>> >> ---
>> >> Changes in v4:
>> >>      - Added this documentation
>> >> ---
>> >>  .../interrupt-controller/interrupts.txt       | 39 +++++++++++++++++++
>> >>  1 file changed, 39 insertions(+)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> >> index 8a3c40829899..917b598317f5 100644
>> >> --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> >> +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> >> @@ -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>;
>> >> +    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.
>>
>>
>
>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.
>
I thought about it. But the wakeup-parent seems to be needed outside the
irqdomain-map to setup the gpiochip's hierarchy. This could be done by
reading the map, but I am not sure if that approach is clean enough.

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

Re: [PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent

[Linus Walleij]

On Wed, Mar 13, 2019 at 10:19 PM Lina Iyer <ilina@codeaurora.org> wrote:

> +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>;
> +       irqdomain-map-mask = <0xff 0>;
> +       irqdomain-map-pass-thru = <0 0xff>;

This is looking a bit familiar to the existing interrupt-map that is
used for PCI interrupts and Swizzling back to a set of PCI
host interrupts.

I tried to document interrupt-map here:
https://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping

interrupt-map is a bit convoluted, so I don't know if it would be subject
to reuse for this. I suspect that interrupt-map, despite the name,
is for PCI only.

Yours,
Linus Walleij

[PATCH v4 04/10] of: irq: add helper to remap interrupts to another irqdomain

[Lina Iyer]

From: Stephen Boyd <swboyd@chromium.org>

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 <swboyd@chromium.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
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(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index e1f6f392a4c0..a1534f947ed4 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -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 */
+	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;
+
+		/* Check for malformed properties */
+		if (WARN_ON(out_size > MAX_PHANDLE_ARGS))
+			goto put;
+		if (map_len < out_size)
+			goto put;
+
+		/* Move forward by new node's #interrupt-cells amount */
+		map += out_size;
+		map_len -= out_size;
+	}
+	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.
+		 */
+		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
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 1214cabb2247..86342502a62a 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -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

Re: [PATCH v4 04/10] of: irq: add helper to remap interrupts to another irqdomain

[Lina Iyer]

I observed an issue with return value being set to 0, when a match is
not found in the irqdomain-map.

On Wed, Mar 13 2019 at 15:20 -0600, Lina Iyer wrote:
>From: Stephen Boyd <swboyd@chromium.org>
>
>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 <swboyd@chromium.org>
>Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>---
>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(+)
>
>diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>index e1f6f392a4c0..a1534f947ed4 100644
>--- a/drivers/of/irq.c
>+++ b/drivers/of/irq.c
>@@ -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 */
>+	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;
This value...
>+	/* 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);
Gets overwritten by this, and set to 0.
>+		if (ret)
>+			goto put;
>+
>+		/* Check for malformed properties */
>+		if (WARN_ON(out_size > MAX_PHANDLE_ARGS))
>+			goto put;
>+		if (map_len < out_size)
>+			goto put;
>+
>+		/* Move forward by new node's #interrupt-cells amount */
>+		map += out_size;
>+		map_len -= out_size;
>+	}
>+	if (match) {
If  match is not found, then ret remains 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
>diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
>index 1214cabb2247..86342502a62a 100644
>--- a/include/linux/of_irq.h
>+++ b/include/linux/of_irq.h
>@@ -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
>
--8<---
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 2653abfaf09d..e12f27403b3a 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -361,32 +361,36 @@ int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out)
                map += out_size;
                map_len -= out_size;
        }
-       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.
-                */
-               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];
-                       }
+       if (!match) {
+               ret = -EINVAL;
+               goto put;
+       }

-                       out->param[i] = be32_to_cpu(val);
+       /* 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_count = in_size = out_size;
-               out->fwnode = of_node_to_fwnode(new);
+
+               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);

[PATCH v4 05/10] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Lina Iyer]

Introduce a new domain for wakeup capable GPIOs. The domain can be
requested using the bus token DOMAIN_BUS_WAKEUP. In the following
patches, we will specify PDC as the wakeup-parent for the TLMM GPIO
irqchip. Requesting a wakeup GPIO will setup the GPIO and the
corresponding PDC interrupt as its parent.

Co-developed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v4:
	- Remove vestigial changes from v2
Changes in v3:
	- Remove PDC GPIO map data (moved to DT)
	- hwirq passed in .alloc() is a PDC pin now
Changes in v2:
	- Remove separate file for PDC GPIO map data
	- Error checks and return
	- Whitespace fixes
---
 drivers/irqchip/qcom-pdc.c   | 72 ++++++++++++++++++++++++++++++++++--
 include/linux/soc/qcom/irq.h | 23 ++++++++++++
 2 files changed, 92 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/soc/qcom/irq.h

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index faa7d61b9d6c..29118f6d84c3 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -13,12 +13,13 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/soc/qcom/irq.h>
 #include <linux/spinlock.h>
-#include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 
 #define PDC_MAX_IRQS		126
+#define PDC_MAX_GPIO_IRQS	256
 
 #define CLEAR_INTR(reg, intr)	(reg & ~(1 << intr))
 #define ENABLE_INTR(reg, intr)	(reg | (1 << intr))
@@ -169,7 +170,6 @@ static irq_hw_number_t get_parent_hwirq(int pin)
 			return (region->parent_base + pin - region->pin_base);
 	}
 
-	WARN_ON(1);
 	return ~0UL;
 }
 
@@ -232,6 +232,60 @@ static const struct irq_domain_ops qcom_pdc_ops = {
 	.free		= irq_domain_free_irqs_common,
 };
 
+static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq,
+			       unsigned int nr_irqs, void *data)
+{
+	struct qcom_irq_fwspec *qcom_fwspec = data;
+	struct irq_fwspec *fwspec = &qcom_fwspec->fwspec;
+	struct irq_fwspec parent_fwspec;
+	irq_hw_number_t hwirq, parent_hwirq;
+	unsigned int type;
+	int ret;
+
+	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return -EINVAL;
+
+	parent_hwirq = get_parent_hwirq(hwirq);
+	if (parent_hwirq == ~0UL)
+		return -EINVAL;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					    &qcom_pdc_gic_chip, NULL);
+	if (ret)
+		return ret;
+
+	qcom_fwspec->mask = true;
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		type = IRQ_TYPE_EDGE_RISING;
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		type = IRQ_TYPE_LEVEL_HIGH;
+
+	parent_fwspec.fwnode      = domain->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0]    = 0;
+	parent_fwspec.param[1]    = parent_hwirq;
+	parent_fwspec.param[2]    = type;
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+					    &parent_fwspec);
+}
+
+static int qcom_pdc_gpio_domain_select(struct irq_domain *d,
+				       struct irq_fwspec *fwspec,
+				       enum irq_domain_bus_token bus_token)
+{
+	return (bus_token == DOMAIN_BUS_WAKEUP);
+}
+
+static const struct irq_domain_ops qcom_pdc_gpio_ops = {
+	.select		= qcom_pdc_gpio_domain_select,
+	.alloc		= qcom_pdc_gpio_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
 static int pdc_setup_pin_mapping(struct device_node *np)
 {
 	int ret, n;
@@ -270,7 +324,7 @@ static int pdc_setup_pin_mapping(struct device_node *np)
 
 static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
 {
-	struct irq_domain *parent_domain, *pdc_domain;
+	struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain;
 	int ret;
 
 	pdc_base = of_iomap(node, 0);
@@ -301,6 +355,18 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
 		goto fail;
 	}
 
+	pdc_gpio_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						      PDC_MAX_GPIO_IRQS,
+						      of_fwnode_handle(node),
+						      &qcom_pdc_gpio_ops, NULL);
+	if (!pdc_gpio_domain) {
+		pr_err("%pOF: GIC domain add failed for GPIO domain\n", node);
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	irq_domain_update_bus_token(pdc_gpio_domain, DOMAIN_BUS_WAKEUP);
+
 	return 0;
 
 fail:
diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h
new file mode 100644
index 000000000000..bacc9edbce0d
--- /dev/null
+++ b/include/linux/soc/qcom/irq.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __QCOM_IRQ_H
+#define __QCOM_IRQ_H
+
+#include <linux/irqdomain.h>
+
+/**
+ * struct qcom_irq_fwspec - qcom specific irq fwspec wrapper
+ * @fwspec: irq fwspec
+ * @mask: if true, keep the irq masked in the gpio controller
+ *
+ * Use this structure to communicate between the parent irq chip, MPM or PDC,
+ * to the gpio chip, TLMM, about the gpio being allocated in the parent
+ * and if the gpio chip should keep the line masked because the parent irq
+ * chip is handling everything about the irq line.
+ */
+struct qcom_irq_fwspec {
+	struct irq_fwspec fwspec;
+	bool mask;
+};
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 06/10] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[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: devicetree@vger.kernel.org
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt    | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
index 665aadb5ea28..f0fedbc5d41a 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
@@ -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
@@ -160,6 +164,7 @@ Example:
 		#gpio-cells = <2>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
+		wake-parent = <&pdc_intc>;
 
 		qup9_active: qup9-active {
 			mux {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 06/10] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Rob Herring]

On Wed, Mar 13, 2019 at 03:18:40PM -0600, Lina Iyer wrote:
> 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: devicetree@vger.kernel.org
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  .../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 <robh@kernel.org>

> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> index 665aadb5ea28..f0fedbc5d41a 100644
> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> @@ -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
> @@ -160,6 +164,7 @@ Example:
>  		#gpio-cells = <2>;
>  		interrupt-controller;
>  		#interrupt-cells = <2>;
> +		wake-parent = <&pdc_intc>;

wakeup-parent

>  
>  		qup9_active: qup9-active {
>  			mux {
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v4 06/10] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Lina Iyer]

Thanks for the review Rob.

On Fri, Mar 15 2019 at 17:37 -0600, Rob Herring wrote:
>On Wed, Mar 13, 2019 at 03:18:40PM -0600, Lina Iyer wrote:
>> 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: devicetree@vger.kernel.org
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>>  .../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:
>
I added this to interrupts.txt. I will remove this, if the documentation in
[PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent
of this series works.

-- Lina

>Reviewed-by: Rob Herring <robh@kernel.org>
>
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
>> index 665aadb5ea28..f0fedbc5d41a 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
>> @@ -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
>> @@ -160,6 +164,7 @@ Example:
>>  		#gpio-cells = <2>;
>>  		interrupt-controller;
>>  		#interrupt-cells = <2>;
>> +		wake-parent = <&pdc_intc>;
>
>wakeup-parent
>
>>
>>  		qup9_active: qup9-active {
>>  			mux {
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>

[PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Lina Iyer]

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.

Co-developed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
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 | 151 ++++++++++++++++++++++++-----
 1 file changed, 128 insertions(+), 23 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index ee8119879c4c..97e9000fa3c3 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -17,6 +17,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinctrl.h>
@@ -27,6 +28,7 @@
 #include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
+#include <linux/soc/qcom/irq.h>
 #include <linux/reboot.h>
 #include <linux/pm.h>
 #include <linux/log2.h>
@@ -69,6 +71,7 @@ struct msm_pinctrl {
 
 	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
 	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
+	DECLARE_BITMAP(wakeup_masked_irqs, MAX_NR_GPIO);
 
 	const struct msm_pinctrl_soc_data *soc;
 	void __iomem *regs[MAX_NR_TILES];
@@ -703,6 +706,13 @@ static void msm_gpio_irq_mask(struct irq_data *d)
 
 	g = &pctrl->soc->groups[d->hwirq];
 
+	if (d->parent_data)
+		irq_chip_mask_parent(d);
+
+	/* Monitored by parent wakeup controller?*/
+	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+		return;
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = msm_readl_intr_cfg(pctrl, g);
@@ -747,6 +757,13 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
 
 	g = &pctrl->soc->groups[d->hwirq];
 
+	if (d->parent_data)
+		irq_chip_unmask_parent(d);
+
+	/* Monitored by parent wakeup controller? Keep masked */
+	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+		return;
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = msm_readl_intr_cfg(pctrl, g);
@@ -767,6 +784,10 @@ static void msm_gpio_irq_ack(struct irq_data *d)
 	unsigned long flags;
 	u32 val;
 
+	/* Handled by parent wakeup controller? Do nothing */
+	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+		return;
+
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -794,6 +815,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 
 	g = &pctrl->soc->groups[d->hwirq];
 
+	if (d->parent_data)
+		irq_chip_set_type_parent(d, type);
+
+	/* Monitored by parent wakeup controller? Keep masked */
+	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+		return 0;
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	/*
@@ -880,17 +908,10 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 
 static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
-	unsigned long flags;
+	if (d->parent_data)
+		return irq_chip_set_wake_parent(d, on);
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
-
-	irq_set_irq_wake(pctrl->irq, on);
-
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
-
-	return 0;
+	return -ENODEV;
 }
 
 static int msm_gpio_irq_reqres(struct irq_data *d)
@@ -967,11 +988,87 @@ 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)
+{
+	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;
+}
+
+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;
+
+	ret = of_irq_domain_map(fwspec, &parent.fwspec);
+	if (ret)
+		return ret;
+
+	parent.fwspec.fwnode = domain->parent->fwnode;
+
+	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
+	if (ret)
+		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()
+ */
+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,
+};
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;
 	int ret;
 	unsigned ngpio = pctrl->soc->ngpios;
+	struct device_node *dn;
 
 	if (WARN_ON(ngpio > MAX_NR_GPIO))
 		return -EINVAL;
@@ -986,6 +1083,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	chip->need_valid_mask = msm_gpio_needs_valid_mask(pctrl);
 
 	pctrl->irq_chip.name = "msmgpio";
+	pctrl->irq_chip.irq_eoi	= irq_chip_eoi_parent;
 	pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
 	pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
@@ -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;
+
+	dn = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
+	if (dn) {
+		chip->irq.parent_domain = irq_find_matching_host(dn,
+						 DOMAIN_BUS_WAKEUP);
+		of_node_put(dn);
+		if (!chip->irq.parent_domain)
+			return -EPROBE_DEFER;
+
+		chip->to_irq = msm_gpio_to_irq;
+	}
+
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
 		dev_err(pctrl->dev, "Failed register gpiochip\n");
@@ -1015,26 +1129,17 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 			dev_name(pctrl->dev), 0, 0, chip->ngpio);
 		if (ret) {
 			dev_err(pctrl->dev, "Failed to add pin range\n");
-			gpiochip_remove(&pctrl->chip);
-			return ret;
+			goto fail;
 		}
 	}
 
-	ret = gpiochip_irqchip_add(chip,
-				   &pctrl->irq_chip,
-				   0,
-				   handle_edge_irq,
-				   IRQ_TYPE_NONE);
-	if (ret) {
-		dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n");
-		gpiochip_remove(&pctrl->chip);
-		return -ENOSYS;
-	}
-
 	gpiochip_set_chained_irqchip(chip, &pctrl->irq_chip, pctrl->irq,
 				     msm_gpio_irq_handler);
 
 	return 0;
+fail:
+	gpiochip_remove(&pctrl->chip);
+	return ret;
 }
 
 static int msm_ps_hold_restart(struct notifier_block *nb, unsigned long action,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Stephen Boyd]

Quoting Lina Iyer (2019-03-13 14:18:41)
> ---
> 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
[...]
> +}
> +
> +/*
> + * TODO: Get rid of this and push it into gpiochip_to_irq()
> + */

Any chance this TODO can be resolved?

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

Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Marc Zyngier]

On Fri, 15 Mar 2019 09:28:31 -0700
Stephen Boyd <swboyd@chromium.org> wrote:

> Quoting Lina Iyer (2019-03-13 14:18:41)
> > ---
> > 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  
> [...]
> > +}
> > +
> > +/*
> > + * TODO: Get rid of this and push it into gpiochip_to_irq()
> > + */  
> 
> Any chance this TODO can be resolved?
> 
> > @@ -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.

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

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Stephen Boyd]

Quoting Marc Zyngier (2019-03-16 04:39:48)
> On Fri, 15 Mar 2019 09:28:31 -0700
> Stephen Boyd <swboyd@chromium.org> wrote:
> 
> > Quoting Lina Iyer (2019-03-13 14:18:41)
> > > @@ -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.
> 
> 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<-------
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 144af0733581..fe2f7888c473 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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;
 

Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Lina Iyer]

On Thu, Mar 21 2019 at 15:54 -0600, Stephen Boyd wrote:
>Quoting Marc Zyngier (2019-03-16 04:39:48)
>> On Fri, 15 Mar 2019 09:28:31 -0700
>> Stephen Boyd <swboyd@chromium.org> wrote:
>>
>> > Quoting Lina Iyer (2019-03-13 14:18:41)
>> > > @@ -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.
>>
>> 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<-------
>diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>index 144af0733581..fe2f7888c473 100644
>--- a/drivers/gpio/gpiolib.c
>+++ b/drivers/gpio/gpiolib.c
>@@ -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;
>
Linus,

Any thoughts on this?

-- Lina

Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Linus Walleij]

On Thu, Mar 21, 2019 at 10:54 PM Stephen Boyd <swboyd@chromium.org> wrote:
> Quoting Marc Zyngier (2019-03-16 04:39:48)> > On Fri, 15 Mar 2019 09:28:31 -0700
> > Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > Quoting Lina Iyer (2019-03-13 14:18:41)
> > > > @@ -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.
> >
> > 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<-------
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 144af0733581..fe2f7888c473 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -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.

Yours,
Linus Walleij

Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Lina Iyer]

On Wed, Apr 17 2019 at 07:59 -0600, Linus Walleij wrote:
>On Thu, Mar 21, 2019 at 10:54 PM Stephen Boyd <swboyd@chromium.org> wrote:
>> Quoting Marc Zyngier (2019-03-16 04:39:48)> > On Fri, 15 Mar 2019 09:28:31 -0700
>> > Stephen Boyd <swboyd@chromium.org> wrote:
>> >
>> > > Quoting Lina Iyer (2019-03-13 14:18:41)
>> > > > @@ -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.
>> >
>> > 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<-------
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 144af0733581..fe2f7888c473 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -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.
>
Thanks for the explanation Linus. Here is my understanding, pls. correct
me if I am wrong.

When the GPIO irqchip is in hierarchy with GIC, as in the case here a
driver would do the following -
- Read GPIO from DT
- Request virtual IRQ number from GPIO by calling gpio_to_irq()
- Request IRQ for the virtual IRQ specifying the IRQ type etc

An example from my test code -

	// Step 1
	gpio = of_get_named_gpio(pdev->dev.of_node, "test-gpios", i);
	if (!gpio_is_valid(gpio)) {
		pr_err("Invalid GPIO for error fatal %d\n", gpio);
		return -EINVAL;
	}
	// Step 2
	irq = gpio_to_irq(gpio);
	if (irq < 0) {
		pr_err("Invalid IRQ for error fatal %u\n", irq);
		return irq;
	}
	// Step 3
	ret = request_irq(irq,  test_gpio_handler,
			  IRQF_TRIGGER_HIGH, "gpio-test", NULL);
	if (ret < 0) {
		pr_err("Unable to register for error fatal IRQ handler %d",
				irq);
		return ret;
	}

Step 1 of the above, does not record the trigger type of the GPIO
Step 2 creates the IRQ mapping without knowing the trigger type
(therefore gpiolib uses IRQ_TYPE_NONE)
Step 3 knows the TYPE and sets that the trigger type on the already
created IRQ.

I was tracing this over and i think this warrants a new solution. The
issue that I see if I hardcode a specific trigger type for the GPIO in
.to_irq(), then request_irq() fails since if the driver requests a
different trigger type. And GIC-v3 warns if the IRQ type is
IRQ_TYPE_NONE as it expects the IRQ to be defined in DT and passed to
the irqchip driver as is.

I think we need a OF GPIO lib function to do step 1 and 2 in one step,
so we could read the type correctly from the DT and request instead of
IRQ_TYPE_NONE when creating the mapping. This should avoid the warning
thrown by gic_irq_domain_translate() -

	WARN_ON(*type == IRQ_TYPE_NONE && fwspec->param[0] != GIC_IRQ_TYPE_PARTITION);

and avoid the failure thrown by irq_create_fwspec_mapping()-

	pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
		hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));

Does that sound reasonable?

Thanks,
Lina

Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Lina Iyer]

On Fri, Mar 15 2019 at 10:28 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-03-13 14:18:41)
>> ---
>> 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
>[...]
>> +}
>> +
>> +/*
>> + * 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);
}
---8<----


Thanks,
Lina

Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Linus Walleij]

On Wed, Apr 17, 2019 at 6:08 PM Lina Iyer <ilina@codeaurora.org> wrote:

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

Isn't the real problem that irq_create_mapping() isn't already doing this
for you?

chip->irq.domain is created with irq_domain_add_simple() and after
Thierrys patch with irq_domain_add_hierarchy(), so I think it is simply
that the hierarchical irqdomain helper code needs to go deeper here.

gpiolib needs struct irq_domain_ops for the hierarchical irqdomain
which implements generic versions of .translate(), .alloc() and .free().
I am working on this... albeit slowly :/

Yours,
Linus Walleij

[PATCH v4 08/10] arm64: dts: qcom: add PDC interrupt controller for SDM845

[Lina Iyer]

Add PDC interrupt controller device bindings for SDM845.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v1:
	- Use updated address specification in reg
	- Rename to pdc_intc
	- Sort per address in DT
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index c27cbd3bcb0a..e55100c2705e 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1534,6 +1534,15 @@
 			#power-domain-cells = <1>;
 		};
 
+		pdc_intc: interrupt-controller@b220000 {
+			compatible = "qcom,sdm845-pdc";
+			reg = <0 0x0b220000 0 0x30000>;
+			qcom,pdc-ranges = <0 480 94>, <94 609 15>, <115 630 7>;
+			#interrupt-cells = <2>;
+			interrupt-parent = <&intc>;
+			interrupt-controller;
+		};
+
 		tsens0: thermal-sensor@c263000 {
 			compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
 			reg = <0xc263000 0x1ff>, /* TM */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 09/10] arm64: dts: qcom: setup PDC as wakeup parent for GPIOs for SDM845

[Lina Iyer]

Setup PDC wakeup parent for TLMM for SDM845 SoC.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v3:
	- Provide irqdomain-map for GPIOs that map to PDC
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 78 ++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index e55100c2705e..58f9f2111449 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -994,6 +994,84 @@
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
+			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>,
+					<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>;
 
 			qup_i2c0_default: qup-i2c0-default {
 				pinmux {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 10/10] arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845

[Lina Iyer]

Enable PDC interrupt controller for SDM845 devices. The interrupt
controller can detect wakeup capable interrupts when the SoC is in a low
power state.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index c8432e24207e..943f312772ee 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -656,6 +656,7 @@ CONFIG_QCOM_SMEM=y
 CONFIG_QCOM_SMD_RPM=y
 CONFIG_QCOM_SMP2P=y
 CONFIG_QCOM_SMSM=y
+CONFIG_QCOM_PDC=y
 CONFIG_ROCKCHIP_PM_DOMAINS=y
 CONFIG_ARCH_TEGRA_132_SOC=y
 CONFIG_ARCH_TEGRA_210_SOC=y
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 00/10] support wakeup capable GPIOs

[Marc Zyngier]

On 13/03/2019 21:18, Lina Iyer wrote:
> 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.

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

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 00/10] support wakeup capable GPIOs

[Lina Iyer]

On Mon, Apr 15 2019 at 06:43 -0600, Marc Zyngier wrote:
>On 13/03/2019 21:18, Lina Iyer wrote:
>> 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.
>
>What the status of this? What is the expected merge strategy?
>
Hi Mark,

I ran into a couple of issues, most of which have been sorted out. But
there is a hardware requirement to write up another register (to set
type), for GPIO wakeup that was missed earlier. I am trying to get that
tested out. That's the hold up. Sorry about that.

--Lina