Quoting Maulik Shah (2020-03-19 02:56:31)Okay i will address this in V3.
On 3/19/2020 2:44 AM, Stephen Boyd wrote:No. IRQs that are disabled but have wake enabled on them should not wake
Quoting Maulik Shah (2020-03-16 23:47:21)Since we want to wake up in idle path LPM as well, when IRQ is marked as wake up capable and even though its disabled.
On 3/17/2020 7:34 AM, Stephen Boyd wrote:Sure. When the irq is enabled we want to enable at the GIC, but if it
What happens if irq is "disabled" in software, because this is the firstAs PDC (and GIC) have a role during "active" time as well, interrupt should be
function called on the irq, and we aren't in suspend yet. Then we get
the irq. Won't we be interrupting the CPU because we've enabled in PDC
and GIC hardware? Why doesn't this function update the wake bit and then
leave the force on irq logic to suspend entry? Will we miss an interrupt
while entering suspend because of that?
enabled in PDC and GIC HW.
isn't enabled and we're not in suspend I would think we don't want the
irq enabled at the GIC. But this code is doing that. Why?
up the CPU from deep idle states (which is what I assume you mean by
idle path LPM (Low Power Mode)). Only if the irq is enabled should it
wake the CPU from deep idle states, and in this case irq wake state has
nothing to do with that working or not.
yes i already did see the commit 461c1a7d4733. The change went in few years back now in gpiolib.That looks like a bug. It appears that gpiolib is only hooking the irqI'd think weNo it doesn't happen this way in suspend_device_irq(), not for this disabled IRQ.
would want to make enable in the PDC driver enable the parent and then
make the set_wake path just update some bitmap tracking wakeup enabled
irqs.
Then when we enter suspend we will enable any pdc interrupts only in the
PDC so that we can wakeup from suspend if that interrupt comes in. When
we wakeup we'll resend the edge interrupts to the GIC on the resume path
and level interrupts will "just work" because they'll stay asserted
throughout resume.
The bigger problem would look to be suspend entry, but in that case we
leave any interrupts enabled at the GIC on the path to suspend (see
suspend_device_irq() and how it bails out early if it's marked for
wakeup)
Agree, it's a bigger problem to set IRQ enabled at GIC HW which is already disabled in HW and SW but marked for wake up.
However suspend_device_irq() is of little or no-use here (for that particular IRQ). Let me step by step give details.
This will benefit everyone to understand this problem. Correct me if something below in not happening as I listed.
Step-1
Driver invokes enable_irq_wake() to mark their IRQ wake up capable.
Step-2
After enable_irq_wake(), drivers invokes disable_irq().
Let's break it down how this disable_irq() will traverse (in current code, without this RFC PATCH)
Step-2.1
In kernel/irq/manage.c
disable_irq() => __disable_irq_nosync() => __disable_irq() => irq_disable()
Step-2.2
This will jump to kernel/irq/chip.c
irq_disable() => __irq_disable()Â => which then calls chip's .irq_disable via desc->irq_data.chip->irq_disable(&desc->irq_data);
Note that this is a GPIO IRQ, gpiolib set's this .irq_disable for every gpio chip during registration
(see below in drivers/gpio/gpiolib.c)
irqchip->irq_disable = gpiochip_irq_disable;
So it doesn't take lazy path to disable at HW, its always unlazy (at-least for gpio IRQs)
disable path here so that it can keep track of what irqs are not in use
by gpiolib and allow them to be used for GPIO purposes when the irqs are
disabled. See commit 461c1a7d4733 ("gpiolib: override
irq_enable/disable"). That code in gpiolib should probably see if lazy
mode has been disabled on the irq and do similar things to what genirq
does and then let genirq mask the gpios if they trigger during the time
when the irq is disabled. Regardless of this design though, I don't
understand why this matters.
Step-2.3__disable_irq() considers lazy setting or not and leaves the irq
Call Jumps to gpiochip_irq_disable()
This then invokes irq_chip's .irq_disable via chip->irq.irq_disable(d)
which finally arrives at msmgpio irq_chip's msm_gpio_irq_disable() call from here.
it finds that IRQ controller is in hierarchy, so it invokes irq_chip_disable_parent().
Step-2.4
This invokes PDC irq_chip's qcom_pdc_gic_disable()
At this point,
This will go ahead and "disabled in PDC HW"
This also invokes irq_chip_disable_parent() since PDC is also in hierarchy with GIC.
Step-2.5
This invokes GIC's gic_mask_irq() since GIC doesn't have .irq_disable implemented, it instead invokes mask.
This will go ahead and "disables at GIC HW".
Final status at the end of step 2:
(a)ÂÂÂ IRQ is marked as wake up capable in SW
(b)ÂÂÂ IRQ is disabled in both SW and HW at GIC and PDC
Step-3
Device enters "suspend to RAM" which invokes suspend_device_irq()
Pasting the interested part here...
ÂÂÂÂÂÂ if (irqd_is_wakeup_set(&desc->irq_data)) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);
ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ ...
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return true;
ÂÂÂÂÂÂÂ }
ÂÂÂ ÂÂÂ ..
ÂÂÂÂÂÂÂ __disable_irq(desc);
Note that it bails out with above if condition here for IRQs that are marked wake up capable, as you have already pointed out.
For the rest of IRQs it goes ahead and disables them at HW via invoking __disable_irq() (be it a GIC HW or PDC HW)
so when device is in suspend only wake up capable IRQs are finally left enabled in HW.
unmasked if the irq is lazy disabled. But we have the
IRQCHIP_MASK_ON_SUSPEND flag set in the gpio irq controller and that
properly masks the irq through the hierarchy if necessary. Again, this
is fine and all but I don't see how it matters.
Now, If any driver that has only done step-1, then it make sense that it will bail out here and leave IRQ enabled in HW (to be precise in itsI don't see any need to change genirq by changing how
original state at HW, without disabling it)
But some drivers did step-1 and step-2 both.
Yes even for this case, it will still bail out here, since its marked as wake up capable but it defeats the purpose of bail out
(bail out is to NOT go ahead and disable at HW), but by doing step-2 driver already disabled the IRQ at HW well before
suspend_device_irq() is invoked.
In other words, IRQ status stays same as what was at the end of step-2 (disabled in HW)
So this whole function is of no use (for that particular IRQ part). Here driver is disabling IRQ on their own via step-2 and then asks
"hey remember I marked it as wake up capable in step-1, so IRQ should
resume system from suspend when it occurs" completely ignoring
the fact that its already disabled at HW in step-2.
IMO, drivers should not even do step-2 here. They should just mark irq as wake up capable and then let suspend framework decide whether
to keep it enabled or disabled in HW when entering to suspend (Read may only do step-1, then everything works fine with current code)
Hope that above details makes it clear on why I way asking earlier to
remove unnecessary disable_irq() call from driver, I don't understand its usage
(at least till now its not clear to me). May be there are some reasons like seeing spurious IRQ when entering to suspend so driver may want to disable it in HW.
But then responsibility is falling on either suspend framework to re-enable such wake up capable interrupts in HW.
This may be done by again invoking enable_irq() before bailing out for
wakeup capable IRQ in suspend_device_irq(). I don't know if this is acceptable.
Someone from To/Cc may clarify if above can be done. Note that it may create problem for other drivers which does only step-1, by calling enable_irq()
during suspend, it will update desc->depth, so this agin need to be undo when resuming.
Otherwise, it is currently falling onto the individual irq_chip's to avoid disabling in HW in the first place.
this RFC patch tries to do same and address this problem in PDC irq_chip when control reaches at Step 2.4, to NOT disable at HW when the IRQ
is already marked wake up capable, it bails out at Step 2.4 in PDC irq_chip so that interrupt is left enabled in HW at both PDC and GIC HW level.
suspend_device_irq() is written, but I'm not the maintainer of this code.
No. if IRQ is maksed at GIC it doesn't wakeup even though at PDC its unmasked.It sounds like PDC just passes along the level or edge and relies on theso we should be able to have some suspend entry hook in pdc thatI thought of this to introduce suspend hook in PDC which decides to keep wake up marked irq enabled at PDC.
enables the irq in the PDC if it's in the wakeup bitmap. Then on the
path to suspend the GIC can lose power at any point after we enable the
wakeup path in PDC and then the system should resume and get the
interrupt through the resend mechanism.
But then someone need to keep it enabled at GIC as well.
PDC does not directly forward IRQ to CPU. PDC brings SoC out of low power mode where GIC does
not have power cut, it replays the interrupt at GIC in HW and that leads to forwarding interrupt
to CPU and resume from low power mode.
So PDC and GIC HW status should need to be in sync.
parent irq chip (GIC in this case) to have the irq unmasked so it can be
seen at the CPU. If the irq is masked at the GIC does it still wakeup
the CPU if it's unmasked at the PDC?
Correct it doesn't exist.
Does the PDC have any other registers that can latch an edge type irq
across suspend so we can read it on resume? If that exists then we can
have software resend irqs at resume time. I think this was asked before
and the answer was it doesn't exist.
If there isn't any sort of latching, then why can't we unmask the irq in
the GIC hardware by calling irq_chip_enable_parent() from the suspend
hook in PDC? That should guarantee that the irq is unmasked at the GIC
when we're suspending and the GIC is about to lose power. And presumably
the GIC state is restored by hardware on resume so that the PDC can
forward the edge to the GIC which can interrupt the CPU because the
interrupt was left unmasked. The idea is to capture wake and enable
state in a bitmap, unmask at the PDC and GIC during system suspend if
it's marked for wakeup regardless of enable state, and then mask in the
PDC and GIC during resume if the irq is disabled in software. Does that
fail somehow?