pdc: Introduce irq_set_wake call
Subsystems (5)
cover pdc: Introduce irq_set_wake call
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(-)
Signature · 2 lines
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
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.
--- drivers/irqchip/qcom-pdc.c | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-)
drivers/irqchip/qcom-pdc.c +10 -17
@@ -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, };
Signature · 2 lines
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
The subject should be something different. "Fix irq wake when irqs are disabled"?
Quoting Maulik Shah (2020-02-17 05:00:07)
Quoted 7 lines
Change the way interrupts get enabled at wakeup capable PDC irq chip. Introduce
irq_set_wakecall which lets interrupts enabled at PDC withenable_irq_wakeand disabled withdisable_irq_wake.Remove
irq_disableandirq_enablecalls which now will default toirq_maskandirq_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.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [RFC 1/2] irqchip: qcom: pdc: Introduce irq_set_wake call
Quoting Maulik Shah (2020-02-21 03:20:59)
Quoted 27 lines · Stephen Boyd
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?
Quoted 7 lines
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.
Quoted 4 lines
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
Maulik,
I'd appreciate if you could Cc me on all irqchip patches.
Quoted 20 lines · Stephen Boyd
Quoting Maulik Shah (2020-02-21 03:20:59)
Quoted 19 lines · Stephen Boyd
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.
Quoted 20 lines
Quoted 12 lines
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?
Quoted 7 lines
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.
Quoted 7 lines
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?
Quoted 4 lines
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.
Signature · 1 line
Who you jivin' with that Cosmik Debris?
Re: [RFC 1/2] irqchip: qcom: pdc: Introduce irq_set_wake call
Quoted 3 lines · Marc Zyngier
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.
Quoted 12 lines · Stephen Boyd
Quoted 10 lines
Quoting Maulik Shah (2020-02-21 03:20:59)
Quoted 9 lines · Stephen Boyd
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.
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
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.
Quoted 45 lines
Quoted 21 lines
Quoted 13 lines
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?
Quoted 7 lines
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.Quoted 7 lines
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?Quoted 4 lines
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.
Quoted 6 lines
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
Signature · 1 line
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
Quoted 6 lines · Maulik Shah
Quoted 3 lines · Marc Zyngier
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 [email protected] (I accidentally replied from my personal address).
Quoted 20 lines
Quoted 20 lines · Stephen Boyd
Quoted 16 lines
Quoting Maulik Shah (2020-02-21 03:20:59)
Quoted 15 lines · Stephen Boyd
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.
Quoted 6 lines
Quoted 1 line
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 callingirq_set_wakewhen 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.
Signature · 1 line
Jazz is not dead. It just smells funny...
RFC 2/2 pinctrl: qcom: Remove forwarding irq_disable and irq_enable call to parent
In reply to Maulik ShahStop 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.
--- drivers/pinctrl/qcom/pinctrl-msm.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/pinctrl/qcom/pinctrl-msm.c +1 -6
@@ -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); }
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation