lkml.org 
[lkml]   [2018]   [Sep]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend
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@codeaurora.org> 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@codeaurora.org>
>> ---
>> 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/

\
 
 \ /
  Last update: 2018-09-22 19:09    [W:0.060 / U:6.736 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site