On Sat, Sep 22 2018 at 10:29 -0600, Marc Zyngier wrote:
Hi Lina,
On Tue, 04 Sep 2018 22:18:08 +0100,
Lina Iyer <ilina@xxxxxxxxxxxxxx> wrote:
During suspend the system may power down some of the system rails. As a
result, the TLMM hw block may not be operational anymore and wakeup
capable GPIOs will not be detected. The PDC however will be operational
and the GPIOs that are routed to the PDC as IRQs can wake the system up.
To avoid being interrupted twice (for TLMM and once for PDC IRQ) when a
GPIO trips, use TLMM for active and switch to PDC for suspend. When
entering suspend, disable the TLMM wakeup interrupt and instead enable
the PDC IRQ and revert upon resume.
Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
---
Changes in v3:
- Enable PDC-IRQ swap only for edge interrupts
Changes in v2:
- Fix PDC IRQ max port, 126 is the max supported in h/w
- Use PDC hwirq in bitmap, linux numbers could be large
- Setup DISABLE_UNLAZY for both TLMM and PDC IRQs
---
[...]
+int __maybe_unused msm_pinctrl_suspend_late(struct device *dev)
+{
+ struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
+ struct irq_data *irqd;
+ unsigned int irq;
+ int i;
+
+ in_suspend = true;
+ for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
+ irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
+ irqd = irq_get_handler_data(irq);
+ /*
+ * We don't know if the TLMM will be functional
+ * or not, during the suspend. If its functional,
+ * we do not want duplicate interrupts from PDC.
+ * Hence disable the GPIO IRQ and enable PDC IRQ
+ * for edge interrupt only.
+ */
+ if (irqd_is_wakeup_set(irqd) && !irqd_is_level_type(irqd)) {
+ disable_irq_wake(irqd->irq);
There is something I don't quite get here. If the PDC is used to wake
up the platform, why is the TLMM interrupt configured as a wakeup
source the first place? Or is it just to keep things simple and not
have to track it in the TLMM driver itself?
True, it need not be. I could just avoid setting the wakeup on the TLMM
and just use the PDC interrupt as wakeup.
Also, I am exploring an option that was suggested by Stephen [1] to just
use the PDC interrupt as a parent of the GPIO IRQ and use a different
irqchip for the PDC interrupt. I ran into some issue with accessing
irqchip and irqdata of the PDC interrupt, since the irqchip was not in
hierarchy with the GPIO's irqchip. I haven't been able to find time to
resolve the issue that the set_parent_ functions, because of the
hierarchy.
Essentially, we have two different mechanisms for GPIO IRQs based on
whether they can be woken up by the PDC interrupt.
GPIO-IRQ --> PDC --> GIC
GPIO-IRQ --> TLMM SUMMARY --> GIC
Do you think the idea is feasible? It would avoid doing all this
enable/disable at every suspend and even during idle, when the TLMM
could be powered off.
+ disable_irq(irqd->irq);
+ enable_irq(irq);
+ }
+ }
Given that you're changing in_suspend and parsing the bitmap,
shouldn't take the pdc spinlock?
Since we are the the only CPU running and suspend/resume (and even idle)
would be serialized I didn't see a reason for needing a lock.
+
+ return 0;
+}
+
+int __maybe_unused msm_pinctrl_resume_late(struct device *dev)
+{
+ struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
+ struct irq_data *irqd, *pdc_irqd;
+ unsigned int irq;
+ int i;
+
+ for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
+ irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
+ irqd = irq_get_handler_data(irq);
+ pdc_irqd = irq_get_irq_data(irq);
+ /*
+ * The TLMM will be operational now, so disable
+ * the PDC IRQ for edge interrupts only.
+ */
+ if (irqd_is_wakeup_set(pdc_irqd) &&
+ !irqd_is_level_type(pdc_irqd)) {
+ disable_irq_nosync(irq);
+ enable_irq_wake(irqd->irq);
+ enable_irq(irqd->irq);
+ }
+ }
+ in_suspend = false;
Same remark about the lock.
Thanks,
Lina
[1]. https://lore.kernel.org/patchwork/patch/975423/
