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
