| Message ID | 1581944408-7656-2-git-send-email-mkshah@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pdc: Introduce irq_set_wake call | expand |
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.
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. >
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.
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
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.
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, };
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(-)