Hi,
On Thu, Dec 3, 2020 at 3:22 AM Maulik Shah <mkshah@xxxxxxxxxxxxxx> 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
