Date:   Thu, 3 Dec 2020 13:04:21 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Maulik Shah <mkshah@...eaurora.org>
Cc:     Rajendra Nayak <rnayak@...eaurora.org>,
        Marc Zyngier <maz@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Linus Walleij <linus.walleij@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Neeraj Upadhyay <neeraju@...eaurora.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Srinivas Ramana <sramana@...eaurora.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Andy Gross <agross@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when
 remuxing GPIOs

Hi,

On Thu, Dec 3, 2020 at 3:22 AM Maulik Shah <mkshah@...eaurora.org> wrote:
>
> >>> +     /*
> >>> +      * Clear IRQs if switching to/from GPIO mode since muxing to/from
> >>> +      * the GPIO path can cause phantom edges.
> >>> +      */
> >>> +     old_i = (oldval & mask) >> g->mux_bit;
> >>> +     if (old_i != i &&
> >>> +         (i == pctrl->soc->gpio_func || old_i == pctrl->soc->gpio_func))
> >>> +             msm_pinctrl_clear_pending_irq(pctrl, group, irq);
> >>> +
> >> The phantom irq can come when switching to GPIO irq mode. so may be only
> >> check if (i == pctrl->soc->gpio_func) {
> > Have you tested this experimentally?
> Yes

Yes means that you tried switching away from GPIO mode and you
couldn't get a phantom interrupt?  OK, I'll re-test then.

I'll test on the Chrome OS kernel tree since that's easiest for me,
but I can test on mainline if you think it would make a difference...

1. Pick <https://crrev.com/c/2556012> and put that kernel on the device.

2. In Cr50 console, make the WP line low with:
  wp enable

3. In AP console do:
  echo bogus > /sys/module/gpio_keys/parameters/doug_test

4. See bogus interrupt:

localhost ~ # echo bogus > /sys/module/gpio_keys/parameters/doug_test
[   62.006346] DOUG: selecting state bogus
[   62.011813] DOUG: ret 0
[   62.011875] DOUG: in dual edge parent: hwirq=66, type=1
[   62.020300] DOUG: gpio_keys_gpio_isr

Can you try replicating again?


> > I have experimentally tested this and I can actually see an interrupt
> > generated when I _leave_ GPIO as well as when I enter GPIO mode.  If
> > you can't see this I can re-setup my test, but this was one of those
> > things that convinced me that the _transition_ is what was causing the
> > fake interrupt.
> >
> > I think my test CL <https://crrev.com/c/2556012/> can help you with
> > testing if you wish.
> >
> >
> >> even better if you can clear this unconditionally.
> > Why?  It should only matter if we're going to/from GPIO mode.
>
> Probably i was not clear, the phantom irq should be cleared when
> switching gpio to gpio IRQ mode.
>
> When GPIO was used as Rx line in example QUP/UART use case, it can latch
> the phantom IRQ

This is where I disagree with you.  I don't think the interrupt is
latching while it's used as an Rx line.  I think it's the pinmux
change that introduces an phantom interrupt.

Specifically, with the same test patch above, AKA
<https://crrev.com/c/2556012>, I can do this:

1. On AP:
  echo bogus > /sys/module/gpio_keys/parameters/doug_test

2. On Cr50 console:
  wp disable
  wp enable
  wp disable
  wp enable
  wp disable
  wp enable

3. Go back and check the AP and see that no interrupts fired.

Said another way: when we're muxed away the interrupts aren't getting
latched.  It's the act of changing the mux that causes the phantom
interrupts.


> but as long as its IRQ is in disabled/masked state it
> doesn't matter.

...but there's no requirement that someone would need to disable/mask
an interrupt while switching the muxing, is there?  So it does matter.


> its only when the GPIO is again set to IRQ mode with set_mux callback,
> the phantom IRQ needs clear to start as clean.
>
> So we should check only for if (i == pctrl->soc->gpio_func) then clear
> phantom IRQ.
>
> The same is case with .direction_output callback, when GPIO is used as
> output say as clock, need not clear any phantom IRQ,
>
> The reason is with every pulse of clock it can latch as pending IRQ in
> GIC_ISPEND as long as it stay as output mode/clock.
>
> its only when switching back GPIO from output direction to input & IRQ
> function, need to clear the phantom IRQ.
>
> so we do not require clear phantom irq in .direction_output callback.

I think all the above explanation is with the model that the interrupt
detection logic is still happening even when muxed away.  I don't
believe that's true.  Please run my test patch or code up something
similar yourself.


> >> In step (3) msm_gpio_irq_set_type() touches the RAW_STATUS_EN making the
> >> phantom irq pending again.
> >> To resolve this, you will need to invoke msm_pinctrl_clear_pending_irq()
> >> at the end of the msm_gpio_irq_set_type().
> >>
> >> I would like Rajendra's (already in cc) review as well on above part.
> > Ugh, so we need a clear in yet another place.  Joy.  OK, I will wait
> > for Rajendra's comment but I can add similar code in
> > msm_gpio_irq_enable().
>
> As the clearing phantom irq code in msm_gpio_irq_enable() is moved to
> separate function msm_pinctrl_clear_pending_irq(), it needs invoke from
> at the end of msm_gpio_irq_set_type() too.

Seems reasonable to me.  I'll include this in my next spin.  Still
waiting for us to agree on some of the points above before spinning,
though.

-Doug

Powered by blists - more mailing lists