Hi Doug,
On 12/4/2020 2:34 AM, Doug Anderson wrote:
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.
Its not the interrupt detection logic that is still happening when muxed
away, but the GPIO line is routed to GIC from PDC.
The GPIO line get forwarded when the system is active/out of system
level low power mode to GIC irrespective of whether GPIO is used as
interrupt or not.
Due to this it can still latch the IRQ at GIC after switching to lets
say Rx mode, whenever the line has any data recive, the line state
toggles can be latched as error interrupt at GIC.
As the interrupt is in disabled state it won't be sent to CPU.
Its only when the driver chooses to switch back to interrupt mode we
want to clear the error interrupt latched to start as clean. same is the
case when used as output direction.
Hope above is clear.
Thanks,
Maulik
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
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
