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