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

[Lina Iyer]

On Mon, Oct 22 2018 at 03:27 -0600, Marc Zyngier wrote:
> On Fri, 19 Oct 2018 20:47:12 +0100,
> Lina Iyer <ilina@xxxxxxxxxxxxxx> wrote:
> > 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?
>
> The problem is that you cannot decide to use another flow handler, as
> this handler is a constraint imposed by the root interrupt
> controller. You can overload it, but you then need to make sure that
> the interrupt will *never* fire at the GIC level, ever.
>
> Can you actually enforce this?
Yes. With this appraoch in msm_gpio_irq_set_type(), we skip writing to
the GPIO's IRQ registers and it will not trigger an interrupt.

> Assuming you can, this could work. But then the subsequent question
> is: Why do you have the interrupt at the TLMM level at all? Overall,
> I'm a bit worried of this "now you see me, now you don't" kind of
> game behind the kernel back. Is there a way we can stop playing that
> game and stick to one single path for interrupt delivery?
The TLMM summary line will be triggered for other GPIOs that are not
wakeup capable and therefore will not have a parallel PDC interrupt.

> > > 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.
>
> It looks to me that the way this logic is split across two drivers is
> a major cause of headache. My advise is that either you have one
> single point of interrupt handling for such interrupt, or you force a
> TLMM wake-up on such an event, forcing it to handle the interrupt the
> "normal way"...
The PDC irqchip's registers are the same ones used over here as well to
configure the wakeup interrupt. So locking acrossing the pinctrl and
irqchip would be annoying, but doable. Would like to avoid it if
possible.

Thanks,
Lina