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

[Marc Zyngier]

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?

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?

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

Thanks,

	M.

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