[RFC 0/2] pdc: Introduce irq_set_wake call

[RFC 0/2] pdc: Introduce irq_set_wake call

[Maulik Shah]

irqchip: qcom: pdc: Introduce irq_set_wake call

Some drivers using gpio interrupts want to configure gpio for wakeup using
enable_irq_wake() but during suspend entry disables irq and expects system
to resume when interrupt occurs. In the driver resume call interrupt is
re-enabled and removes wakeup capability using disable_irq_wake() one such
example is cros ec driver.

With [1] in documentation saying "An irq can be disabled with disable_irq()
and still wake the system as long as the irq has wake enabled".

In such scenario the gpio irq stays disabled at gpio irqchip but needs to
keep enabled in the hierarchy for wakeup capable parent PDC and GIC irqchip
to be able to detect and forward wake capable interrupt to CPU when system
is in sleep state like suspend.

Sending this as an RFC since this series attempts to add support for [1] by
introducing irq_set_wake call for PDC irqchip from which interrupt can be
enabled/disabled at PDC hardware. This also removes irq_chip_enable_parent
and irq_chip_disable_parent calls made from msmgpio irqchip to PDC since
enable and disable at wakeup capable irqchip is now done from irq_set_wake.

Note that *ALL* drivers using wakeup capable interrupt and want to disable
irq with disable_irq() need to call disable_irq_wake() also if they want
to stop wakeup capability at parent PDC irqchip. Not doing so will lead to
system getting woken up from sleep states.

[1] https://www.spinics.net/lists/kernel/msg3398294.html

Maulik Shah (2):
  irqchip: qcom: pdc: Introduce irq_set_wake call
  pinctrl: qcom: Remove forwarding irq_disable and irq_enable call to
    parent

 drivers/irqchip/qcom-pdc.c         | 27 ++++++++++-----------------
 drivers/pinctrl/qcom/pinctrl-msm.c |  7 +------
 2 files changed, 11 insertions(+), 23 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[RFC 1/2] irqchip: qcom: pdc: Introduce irq_set_wake call

[Maulik Shah]

Change the way interrupts get enabled at wakeup capable PDC irq chip.
Introduce irq_set_wake call which lets interrupts enabled at PDC with
enable_irq_wake and disabled with disable_irq_wake.

Remove irq_disable and irq_enable calls which now will default to irq_mask
and irq_unmask.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/irqchip/qcom-pdc.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 6ae9e1f..145d4ae 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Copyright (c) 2017-2019, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved.
  */
 
 #include <linux/err.h>
@@ -87,22 +87,17 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
 	raw_spin_unlock(&pdc_lock);
 }
 
-static void qcom_pdc_gic_disable(struct irq_data *d)
+static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on)
 {
 	if (d->hwirq == GPIO_NO_WAKE_IRQ)
-		return;
-
-	pdc_enable_intr(d, false);
-	irq_chip_disable_parent(d);
-}
+		return 0;
 
-static void qcom_pdc_gic_enable(struct irq_data *d)
-{
-	if (d->hwirq == GPIO_NO_WAKE_IRQ)
-		return;
+	if (on)
+		pdc_enable_intr(d, true);
+	else
+		pdc_enable_intr(d, false);
 
-	pdc_enable_intr(d, true);
-	irq_chip_enable_parent(d);
+	return irq_chip_set_wake_parent(d, on);
 }
 
 static void qcom_pdc_gic_mask(struct irq_data *d)
@@ -197,15 +192,13 @@ static struct irq_chip qcom_pdc_gic_chip = {
 	.irq_eoi		= irq_chip_eoi_parent,
 	.irq_mask		= qcom_pdc_gic_mask,
 	.irq_unmask		= qcom_pdc_gic_unmask,
-	.irq_disable		= qcom_pdc_gic_disable,
-	.irq_enable		= qcom_pdc_gic_enable,
 	.irq_get_irqchip_state	= qcom_pdc_gic_get_irqchip_state,
 	.irq_set_irqchip_state	= qcom_pdc_gic_set_irqchip_state,
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.irq_set_type		= qcom_pdc_gic_set_type,
+	.irq_set_wake		= qcom_pdc_gic_set_wake,
 	.flags			= IRQCHIP_MASK_ON_SUSPEND |
-				  IRQCHIP_SET_TYPE_MASKED |
-				  IRQCHIP_SKIP_SET_WAKE,
+				  IRQCHIP_SET_TYPE_MASKED,
 	.irq_set_vcpu_affinity	= irq_chip_set_vcpu_affinity_parent,
 	.irq_set_affinity	= irq_chip_set_affinity_parent,
 };
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[RFC 2/2] pinctrl: qcom: Remove forwarding irq_disable and irq_enable call to parent

[Maulik Shah]

Stop forwarding irq_disable and irq_enable from msmgpio to parent PDC.

The irq gets enabled/disabled at wakeup capable parent PDC when wakeup
capability changes with enable_irq_wake or disable_irq_wake.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 9a8daa2..a8973d2 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -803,10 +803,8 @@ static void msm_gpio_irq_enable(struct irq_data *d)
 	 * the interrupt from being latched at the GIC. The state at
 	 * GIC needs to be cleared before enabling.
 	 */
-	if (d->parent_data) {
+	if (d->parent_data)
 		irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
-		irq_chip_enable_parent(d);
-	}
 
 	msm_gpio_irq_clear_unmask(d, true);
 }
@@ -816,9 +814,6 @@ static void msm_gpio_irq_disable(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 
-	if (d->parent_data)
-		irq_chip_disable_parent(d);
-
 	if (!test_bit(d->hwirq, pctrl->skip_wake_irqs))
 		msm_gpio_irq_mask(d);
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [RFC 1/2] irqchip: qcom: pdc: Introduce irq_set_wake call

[Stephen Boyd]

The subject should be something different. "Fix irq wake when irqs are
disabled"?

Quoting Maulik Shah (2020-02-17 05:00:07)
> Change the way interrupts get enabled at wakeup capable PDC irq chip.
> Introduce irq_set_wake call which lets interrupts enabled at PDC with
> enable_irq_wake and disabled with disable_irq_wake.
> 
> Remove irq_disable and irq_enable calls which now will default to irq_mask
> and irq_unmask.
> 

This commit text is pretty useless. It says what is happening in the
patch but doesn't explain why anything is changing or why anyone should
care.

How are wakeups supposed to work when the CPU cluster power is disabled
in low power CPU idle modes? Presumably the parent irq controller is
powered off (in this case it's an ARM GIC) and we would need to have the
interrupt be "enabled" or "unmasked" at the PDC for the irq to wakeup
the cluster. We shouldn't need to enable irq wake on any irq for the CPU
to get that interrupt in deep CPU idle states.

Re: [RFC 1/2] irqchip: qcom: pdc: Introduce irq_set_wake call

[Stephen Boyd]

Quoting Maulik Shah (2020-02-21 03:20:59)
> 
> On 2/20/2020 7:51 AM, Stephen Boyd wrote:
> 
>     How are wakeups supposed to work when the CPU cluster power is disabled
>     in low power CPU idle modes? Presumably the parent irq controller is
>     powered off (in this case it's an ARM GIC) and we would need to have the
>     interrupt be "enabled" or "unmasked" at the PDC for the irq to wakeup
>     the cluster.
> 
> Correct. Interrupt needs to be "enabled" or "unmasked" at wakeup capable PDC
> for irqchip to wakeup from "deep" low power modes where parent GIC may not be
> monitoring interrupt and only PDC is monitoring.
> these "deep" low power modes can either be triggered by kernel "suspend" or
> "cpuidle" path for which drivers may or may not have registered for suspend or
> cpu/cluster pm notifications to make a decision of enabling wakeup capability.
> 
> 
>     We shouldn't need to enable irq wake on any irq for the CPU
>     to get that interrupt in deep CPU idle states.
> 
> + *
> + *     Note: irq enable/disable state is completely orthogonal
> + *     to the enable/disable state of irq wake.
> 
> i think that's what above documentation said to have wakeup capability is
> orthogonal to enable/disable state of irq, no?
> 
> A deep cpuidle entry is also orthogonal to drivers unless they register for cpu
> pm notifications.
> 
> so with this change,
> if the drivers want their interrupt to be wakeup capable during both "suspend"
> and "cpuidle" they can call "enable_irq_wake" and leave it there to be wake up
> capable.

Where is there a mention about drivers registering for cpu PM
notifications? I'm not aware of this being mentioned as a requirement.
Instead, my understanding is that deep idle states shouldn't affect irqs
from being raised to the CPU. If such an irq is enabled and can't wake
the system from deep idle and it's expected to interrupt during this
idle time then perhaps the PDC driver needs to block deep idle states
until that irq is disabled.

Does this scenario exist? It sounds like broken system design to have an
irq that can't wake from deep idle, but I see that PDC has a selective
set of pins so maybe some irqs just aren't expected to wake the system
up during idle.

> 
> This way they don't need to worry about cpu entering to deep low power mode and
> enabling wakeup capability "only when" deep low power mode get triggered.
>

Re: [RFC 1/2] irqchip: qcom: pdc: Introduce irq_set_wake call

[Marc Zyngier]

Maulik,

I'd appreciate if you could Cc me on all irqchip patches.

On 2020-02-25 17:16, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-02-21 03:20:59)
>> 
>> On 2/20/2020 7:51 AM, Stephen Boyd wrote:
>> 
>>     How are wakeups supposed to work when the CPU cluster power is 
>> disabled
>>     in low power CPU idle modes? Presumably the parent irq controller 
>> is
>>     powered off (in this case it's an ARM GIC) and we would need to 
>> have the
>>     interrupt be "enabled" or "unmasked" at the PDC for the irq to 
>> wakeup
>>     the cluster.
>> 
>> Correct. Interrupt needs to be "enabled" or "unmasked" at wakeup 
>> capable PDC
>> for irqchip to wakeup from "deep" low power modes where parent GIC may 
>> not be
>> monitoring interrupt and only PDC is monitoring.
>> these "deep" low power modes can either be triggered by kernel 
>> "suspend" or
>> "cpuidle" path for which drivers may or may not have registered for 
>> suspend or
>> cpu/cluster pm notifications to make a decision of enabling wakeup 
>> capability.

Loosing interrupt delivery in idle is not an acceptable behaviour. Idle 
!= suspend.

>> 
>> 
>>     We shouldn't need to enable irq wake on any irq for the CPU
>>     to get that interrupt in deep CPU idle states.
>> 
>> + *
>> + *     Note: irq enable/disable state is completely orthogonal
>> + *     to the enable/disable state of irq wake.
>> 
>> i think that's what above documentation said to have wakeup capability 
>> is
>> orthogonal to enable/disable state of irq, no?
>> 
>> A deep cpuidle entry is also orthogonal to drivers unless they 
>> register for cpu
>> pm notifications.
>> 
>> so with this change,
>> if the drivers want their interrupt to be wakeup capable during both 
>> "suspend"
>> and "cpuidle" they can call "enable_irq_wake" and leave it there to be 
>> wake up
>> capable.
> 
> Where is there a mention about drivers registering for cpu PM
> notifications? I'm not aware of this being mentioned as a requirement.
> Instead, my understanding is that deep idle states shouldn't affect 
> irqs
> from being raised to the CPU. If such an irq is enabled and can't wake
> the system from deep idle and it's expected to interrupt during this
> idle time then perhaps the PDC driver needs to block deep idle states
> until that irq is disabled.

Indeed. Idle states shouldn't affect irq delivery. The irq_set_wake() 
call
deals with suspend, and idle is rather different from suspend.

Conflating the two seems pretty broken, and definitely goes against the
expected behaviour of device drivers. Is the expectation now that we are
going to see a flurry of patches adding irq_set_wake() calls all over 
the map?

> Does this scenario exist? It sounds like broken system design to have 
> an
> irq that can't wake from deep idle, but I see that PDC has a selective
> set of pins so maybe some irqs just aren't expected to wake the system
> up during idle.

That'd be terribly broken. We've had a similar discussion about a NXP
platform where only some interrupts could wake take the CPU out of idle.
The end result is that we don't idle on this system.

If the PDC can't take the CPU out of idle, then idle shouldn't be 
entered
when these broken interrupts are enabled.

Thanks,

         M.
-- 
Who you jivin' with that Cosmik Debris?

Re: [RFC 1/2] irqchip: qcom: pdc: Introduce irq_set_wake call

[Maulik Shah]

On 2/27/2020 6:39 AM, Marc Zyngier wrote:
> Maulik,
>
> I'd appreciate if you could Cc me on all irqchip patches.

Sure Marc, i kept you in Cc for V2 addressing stephen's comments.

>
> On 2020-02-25 17:16, Stephen Boyd wrote:
>> Quoting Maulik Shah (2020-02-21 03:20:59)
>>>
>>> On 2/20/2020 7:51 AM, Stephen Boyd wrote:
>>>
>>>     How are wakeups supposed to work when the CPU cluster power is disabled
>>>     in low power CPU idle modes? Presumably the parent irq controller is
>>>     powered off (in this case it's an ARM GIC) and we would need to have the
>>>     interrupt be "enabled" or "unmasked" at the PDC for the irq to wakeup
>>>     the cluster.
>>>
>>> Correct. Interrupt needs to be "enabled" or "unmasked" at wakeup capable PDC
>>> for irqchip to wakeup from "deep" low power modes where parent GIC may not be
>>> monitoring interrupt and only PDC is monitoring.
>>> these "deep" low power modes can either be triggered by kernel "suspend" or
>>> "cpuidle" path for which drivers may or may not have registered for suspend or
>>> cpu/cluster pm notifications to make a decision of enabling wakeup capability.
>
> Loosing interrupt delivery in idle is not an acceptable behaviour. Idle != suspend.

Agree, we are not lossing it, but rather RFC v1 was keeping a requirement on drivers to keep wake

enabled by calling irq_set_wake when the interrupt is routed via PDC, even after coming out of suspend.

i addressed this in RFC v2.

>
>>>
>>>
>>>     We shouldn't need to enable irq wake on any irq for the CPU
>>>     to get that interrupt in deep CPU idle states.
>>>
>>> + *
>>> + *     Note: irq enable/disable state is completely orthogonal
>>> + *     to the enable/disable state of irq wake.
>>>
>>> i think that's what above documentation said to have wakeup capability is
>>> orthogonal to enable/disable state of irq, no?
>>>
>>> A deep cpuidle entry is also orthogonal to drivers unless they register for cpu
>>> pm notifications.
>>>
>>> so with this change,
>>> if the drivers want their interrupt to be wakeup capable during both "suspend"
>>> and "cpuidle" they can call "enable_irq_wake" and leave it there to be wake up
>>> capable.
>>
>> Where is there a mention about drivers registering for cpu PM
>> notifications? I'm not aware of this being mentioned as a requirement.
>> Instead, my understanding is that deep idle states shouldn't affect irqs
>> from being raised to the CPU. If such an irq is enabled and can't wake
>> the system from deep idle and it's expected to interrupt during this
>> idle time then perhaps the PDC driver needs to block deep idle states
>> until that irq is disabled.
>
> Indeed. Idle states shouldn't affect irq delivery. The irq_set_wake() call
> deals with suspend, and idle is rather different from suspend.
>
> Conflating the two seems pretty broken, and definitely goes against the
> expected behaviour of device drivers. Is the expectation now that we are
> going to see a flurry of patches adding irq_set_wake() calls all over the map?
>
>> Does this scenario exist? It sounds like broken system design to have an
>> irq that can't wake from deep idle, but I see that PDC has a selective
>> set of pins so maybe some irqs just aren't expected to wake the system
>> up during idle.
>
> That'd be terribly broken. We've had a similar discussion about a NXP
> platform where only some interrupts could wake take the CPU out of idle.
> The end result is that we don't idle on this system.
>
> If the PDC can't take the CPU out of idle, then idle shouldn't be entered
> when these broken interrupts are enabled.
>
> Thanks,
>
>         M.

This is not the case, we don't loose any interrupt in CPUidle.

Thanks,

Maulik

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [RFC 1/2] irqchip: qcom: pdc: Introduce irq_set_wake call

[Marc Zyngier]

On 2020-03-12 11:33, Maulik Shah wrote:
> On 2/27/2020 6:39 AM, Marc Zyngier wrote:
>> Maulik,
>> 
>> I'd appreciate if you could Cc me on all irqchip patches.
> 
> Sure Marc, i kept you in Cc for V2 addressing stephen's comments.

Thanks. Make sure you use maz@kernel.org (I accidentally replied from
my personal address).

> 
>> 
>> On 2020-02-25 17:16, Stephen Boyd wrote:
>>> Quoting Maulik Shah (2020-02-21 03:20:59)
>>>> 
>>>> On 2/20/2020 7:51 AM, Stephen Boyd wrote:
>>>> 
>>>>     How are wakeups supposed to work when the CPU cluster power is 
>>>> disabled
>>>>     in low power CPU idle modes? Presumably the parent irq 
>>>> controller is
>>>>     powered off (in this case it's an ARM GIC) and we would need to 
>>>> have the
>>>>     interrupt be "enabled" or "unmasked" at the PDC for the irq to 
>>>> wakeup
>>>>     the cluster.
>>>> 
>>>> Correct. Interrupt needs to be "enabled" or "unmasked" at wakeup 
>>>> capable PDC
>>>> for irqchip to wakeup from "deep" low power modes where parent GIC 
>>>> may not be
>>>> monitoring interrupt and only PDC is monitoring.
>>>> these "deep" low power modes can either be triggered by kernel 
>>>> "suspend" or
>>>> "cpuidle" path for which drivers may or may not have registered for 
>>>> suspend or
>>>> cpu/cluster pm notifications to make a decision of enabling wakeup 
>>>> capability.
>> 
>> Loosing interrupt delivery in idle is not an acceptable behaviour. 
>> Idle != suspend.
> 
> Agree, we are not lossing it, but rather RFC v1 was keeping a
> requirement on drivers to keep wake
> enabled by calling irq_set_wake when the interrupt is routed via PDC,
> even after coming out of suspend.

An endpoint driver shouldn't have to know what interrupt controller it
is connected to. So your "when the interrupt is routed via PDC" is
not enforceable.

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