Messages in this thread | | | Date | Mon, 22 Oct 2018 10:26:53 +0100 | From | Marc Zyngier <> | Subject | Re: [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.
| |