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

From: Lina Iyer
Date: Wed Oct 24 2018 - 16:45:50 EST


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