Quoting Maulik Shah (2020-06-01 04:38:25)yes it can be a power problem.
On 5/31/2020 12:56 AM, Stephen Boyd wrote:Ok so in summary, irq is left unmasked in pdc during deep cpu idle and
Quoting Maulik Shah (2020-05-29 02:20:32)Consider a scenario..
On 5/27/2020 3:45 PM, Stephen Boyd wrote:How does it break cpuidle? The irqs that would be enabled/unmasked in
Quoting Maulik Shah (2020-05-23 10:11:13)PDC monitors interrupts during CPUidle as well, in cases where deepest
@@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)I find these two hunks deeply confusing. I'm not sure what the
if (d->hwirq == GPIO_NO_WAKE_IRQ)
return;
+ pdc_enable_intr(d, true);
irq_chip_unmask_parent(d);
}
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?
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.
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?
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.
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?
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?
We can't rely on drivers to do that.correct.Does 'default' mean the hardware register reset state?My understanding of the hardware is that the GPIO controller has linesit can affect idle path as explained above.
directly connected to various SPI lines on the GIC and PDC has a way to
monitor those direct connections and wakeup the CPUs when they trigger
the detection logic in the PDC. The enable/disable bit in PDC gates that
logic for each wire between the GPIO controller and the GIC.
So isn't it simpler to leave the PDC monitoring pins that we care about
all the time and only stop monitoring when we enter and leave suspend?
And shouldn't the driver set something sane in qcom_pdc_init() toWe don't rely on boot state, by default all interrupt will be disabled.
disable all the pdc pins so that we don't rely on boot state to
configure pins for wakeup?
I'm worried thatRight however when switching kernel, i suppose client drivers will do a
we will kexec and then various pdc pins will be enabled because the
previous kernel had them enabled but then the new kernel doesn't care
about those pins and we'll never be able to suspend or go idle. I don't
know what happens in the GIC case but I think gic_dist_config() and
things set a sane state at kernel boot.
free_irq(), then this will
clear the PDC interrupt in HW by invoking mask_irq() from within free_irq().
It's not a concern about the hardware reset state of these registers atEnable bank will be zero as part of HW reset status when booting upThis is same to GIC driver having GICD_ISENABLER register, where allWhat code sets the IRQ_ENABLE_BANK to all zero when this driver probes?
bits (one bit per interrupt) set to 0 (masked irqs) during boot up.
Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK.
first time.
boot. I'm worried that the bootloaders or previous OS will configure pdc
pins to wake us up. It's better to just force it to something sane, i.e.
everything disabled in the PDC, at driver probe time so that nothing can
be wrong.