Re: [PATCH v3 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend

[Marc Zyngier]

On Sat, 22 Sep 2018 18:09:09 +0100,
Lina Iyer <ilina@xxxxxxxxxxxxxx> wrote:
> 
> 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.

[me tries to page it all in again]

You could have the PDC as part of the GPIO hierarchy:

	GPIO -> PDC -> TLMM -> GIC

and always configure the PDC as a wake-up source. I just wonder if you
can do that without setting up a parallel hierarchy between the PDC
and the GIC. We already have similar things in the tree (see OMAP's
wakeupgen), and it may be worth having a look. The lack of interrupt
replaying on the PDC is quite annoying (I have much stronger words in
mind though), and I'm not sure we can easily fix that one without this
parallel interrupt hack (you need something to inject edge interrupts
in the TLMM).

> 
> 
> >> +			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.

In that case, what is the purpose of 'in_suspend' if
msm_gpio_irq_set_wake cannot happen during the suspend/resume phases?
It all seems a bit inconsistent.

Thanks,

	N,

-- 
Jazz is not dead, it just smell funny.