Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

On Fri, Oct 19 2018 at 09:53 -0600, Marc Zyngier wrote:
> Hi Lina,
>
> On 19/10/18 16:32, Lina Iyer wrote:
> > Hi folks,
> >
> > On Wed, Oct 10 2018 at 18:30 -0600, Lina Iyer wrote:

[...]

> > > +static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
> > > +{
> > > +	struct irq_data *irqd = data;
> > > +	struct irq_desc *desc = irq_to_desc(irqd->irq);
> > > +
> > > +	desc->handle_irq(desc);
> > Do we see any problem calling handle_irq()?
>
> Good timing, I was just looking at this.
:) Thanks for your time.

> One thing I can see is that you will end-up calling the EOI callback on
> the root interrupt controller (the GIC), thus writing to ICC_EOIR1_EL1.
>
> But you've never acked this interrupt the first place by reading
> ICC_IAR1_EL1, and that puts you violently out of spec, according to the
> GICv3 spec (8.2.10), which reads:
>
> "A write to this register must correspond to the most recent valid read
> by this PE from an Interrupt Acknowledge Register, and must correspond
> to the INTID that was read from ICC_IAR1_EL1, otherwise the system
> behavior is UNPREDICTABLE."
>
> Here, you definitely risk the sanity of the CPU interface state machine.
Oh, thanks Marc. Will look into it. The problem is because I call
handle_irq() directly for the GPIO IRQ which is not triggered but we end
up mask, eoi etc.

How about calling handle_simple_irq(), which doesn't seem to the IRQ
registers?

> So stepping back a bit: At some point, you had a version that just
> relied on regenerating edge interrupts by writing to some register
> (knowing that level interrupts are safe by definition). Why isn't that
> the right solution? It'd avoid the above minefield by just letting the
> HW do its thing...
There are some unnecessary complexity with the approach that we are
trying to avoid.

The TLMM may or not may not be powered off (depending on the SoC state)
and Linux has no control on it. The PDC will remain powered on but we
don't want the PDC interrupt enabled always, since we will receive to
interrupts (one from TLMM and one from PDC) for every event. So we have
to keep the PDC interrupt configured as wakeup interrupt but operate on
the fact that when we go into suspend or cpuidle we will have to switch
to enabling the PDC interrupt and disabling the GPIO IRQ and swap back
when we resume. This dance is harder with cpuidle (notifying the TLMM
driver, when all the CPUs are in idle) than suspend/resume which has
nice callbacks and is what we are trying to avoid but using the PDC
interrupt always.

Thanks,
Lina