Re: [PATCH v2 4/4] irqchip: qcom-pdc: Introduce irq_set_wake call

From: Maulik Shah
Date: Mon Jun 22 2020 - 05:19:33 EST

Hi,

On 6/19/2020 2:56 PM, Stephen Boyd wrote:
Quoting Maulik Shah (2020-06-18 03:03:03)
On 6/16/2020 5:27 PM, Stephen Boyd wrote:
Quoting Maulik Shah (2020-06-01 04:38:25)
On 5/31/2020 12:56 AM, Stephen Boyd wrote:
Quoting Maulik Shah (2020-05-29 02:20:32)
On 5/27/2020 3:45 PM, Stephen Boyd wrote:
Quoting Maulik Shah (2020-05-23 10:11:13)
@@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
if (d->hwirq == GPIO_NO_WAKE_IRQ)
return;
+ pdc_enable_intr(d, true);
irq_chip_unmask_parent(d);
}
I find these two hunks deeply confusing. I'm not sure what the
maintainers think though. I hope it would be simpler to always enable
the hwirqs in the pdc when an irq is requested and only disable it in
the pdc when the system goes to suspend and the pdc pin isn't for an irq
that's marked for wakeup. Does that break somehow?
PDC monitors interrupts during CPUidle as well, in cases where deepest
low power mode happened from cpuidle where GIC is not active.
If we keep PDC IRQ always enabled/unmasked during idle and then
disable/mask when entering to suspend, it will break cpuidle.
How does it break cpuidle? The irqs that would be enabled/unmasked in
pdc would only be the irqs that the kernel has setup irq handlers for
(from request_irq() and friends). We want those irqs to keep working
during cpuidle and wake the CPU from the deepest idle states.
I hope it would be simpler to always enable
the hwirqs in the pdc when an irq is requested and only disable it in
the pdc when the system goes to suspend and the pdc pin isn't for an irq
that's marked for wakeup
How does it break cpuidle?
Consider a scenario..
1. All PDC irqs enabled/unmasked in HW when request_irq() happened/alloc happens
2. Client driver disable's irq. (lazy disable is there, so in HW its still unmasked) but disabled in SW.
3. Device enters deep CPUidle low power modes where only PDC monitors IRQ.
4. This IRQ can still wakeup from CPUidle since it was monitored by PDC.
5. From handler, it comes to know that IRQ is disabled in SW, so it really invokes irq_mask callback now to disable in HW.
6. This mask callback doesn't operate on PDC (since in PDC, IRQs gets masked only during suspend, all other times its enabled)
7. step 3 to 6 repeats, if this IRQ keeps on coming and waking up from deep cpuidle states.
Ok so in summary, irq is left unmasked in pdc during deep cpu idle and
it keeps waking up the CPU because it isn't masked at the PDC after the
first time it interrupts? Is this a power problem?
yes it can be a power problem.
Because from a
correctness standpoint we don't really care. It woke up the CPU because
it happened, and the GIC can decide to ignore it or not by masking it at
the GIC. I thought that the PDC wouldn't wake up the CPU if we masked
the irq at the GIC level. Is that not true?
once PDC detects IRQ, it directly doesn't wake up CPU. it replays IRQ to
GIC.

since at GIC its masked, GIC doesn't forward to cpu to immediatly wake
it up.

however after PDC detecting IRQ, it exits low power mode and
watchdog/timer can wakeup upon expiry.
Ok. So the only problem is some screaming irq that really wants to be
handled but the driver that requested it has disabled it at runtime. The
IRQ keeps kicking the CPUs out of deep idle and then eventually the
timer tick happens and we've run the CPUs in a shallower idle state for
this time?
No it may still enter deeper state next time.
Presumably we'd like to have these irqs be lazily masked at
the PDC so that they can become pending when they first arrive but not
block deep idle states if they're interrupting often while being
handled.

We do lazily disable IRQ. but didnot understand why lazily disable when they are being handled?

The edge type irqs gets masked immediatly if one irq is being handled and another comes in.

but that's not a problem.


On the other hand, we want irq wake state to be the only factor in irqs
being unmasked at the PDC on the entry to suspend. Purely
masking/unmasking at the PDC when the irq is masked in software doesn't
work because suspend/resume will break for disabled but wake enabled
irqs. But doing that makes idle work easily because we can assume during
idle that leaving it unmasked until it fires and then masking it in the
PDC until it is handled gives us good deep idle states in the face of
screaming irqs.

What are the actual requirements? Here is my attempt to boil this
discussion down into a few bullet points:

1. During system suspend, wake enabled irqs should be enabled in PDC
and all other irqs should be disabled in PDC.
yes, IRQs should be enabled in both PDC and GIC before platform (PSCI suspend) happens if they are marked for wakeup (enable_irq_wake())

2. During idle, enabled irqs must be enabled in PDC, unless they're
pending in which case they should be masked in the PDC so as to not
wake up the CPU from deep idle states

i didn't get this point.

During idle, if the driver choosen to keep IRQ enabled, it should be enabled in both PDC and GIC

if the driver choosen to keep IRQ disabled, with this series...

a. do a lay disable when driver's call disable_irq(), meaning set the SW state as disabled but leave in PDC and GIC HW as unmasked/enabled.

b. if the IRQ comes inbetween and its of edge type, the generic handle_edge_irq will really mask in HW.


3. During non-idle, non-suspend, enabled irqs must be enabled in PDC.

Or is #3 actually false and PDC has no bearing on this?

Correct, During this time (non-idle, non-suspend) PDC will be in something called "by pass mode" where it plays role of type conversion.

(a level low to level high / edge falling to edge rising) since GIC doesn't detect level low/falling edge IRQs.

Thanks,
Maulik

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation