From: Marc Zyngier <marc.zyngier@arm.com>
To: Lina Iyer <ilina@codeaurora.org>
Cc: sboyd@kernel.org, evgreen@chromium.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO
Date: Mon, 22 Oct 2018 10:26:53 +0100 [thread overview]
Message-ID: <86a7n6tjqa.wl-marc.zyngier@arm.com> (raw)
In-Reply-To: <20181019194712.GB17444@codeaurora.org>
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.
next prev parent reply other threads:[~2018-10-22 9:27 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-11 0:29 [PATCH RFC 0/1] QCOM: GPIO IRQ wakeup using PDC irqchip Lina Iyer
2018-10-11 0:29 ` [PATCH RFC 1/1] drivers: pinctrl: qcom: add wakeup capability to GPIO Lina Iyer
2018-10-19 15:32 ` Lina Iyer
2018-10-19 15:53 ` Marc Zyngier
2018-10-19 19:47 ` Lina Iyer
2018-10-22 9:26 ` Marc Zyngier [this message]
2018-10-24 20:45 ` Lina Iyer
2018-10-31 7:05 ` Stephen Boyd
2018-10-31 16:10 ` Lina Iyer
2018-10-31 16:46 ` Lina Iyer
2018-11-01 0:13 ` Stephen Boyd
2018-11-01 17:16 ` Lina Iyer
2018-11-06 5:19 ` Stephen Boyd
2018-11-07 22:38 ` Lina Iyer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=86a7n6tjqa.wl-marc.zyngier@arm.com \
--to=marc.zyngier@arm.com \
--cc=evgreen@chromium.org \
--cc=ilina@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sboyd@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).