lkml.org 
[lkml]   [2018]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO
On Fri, 19 Oct 2018 20:47:12 +0100,
Lina Iyer <ilina@codeaurora.org> 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.

\
 
 \ /
  Last update: 2018-10-22 11:28    [W:0.094 / U:1.620 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site