Hi Brian,
On 05/08/2018 06:15 AM, Brian Norris wrote:
+ Doug
Hi Jeffy,
On Thu, May 03, 2018 at 02:55:53PM +0800, Jeffy Chen wrote:
We saw spurious irq when changing irq's trigger type, for example
setting gpio-keys's wakeup irq trigger type.
And according to the TRM:
"Programming the GPIO registers for interrupt capability, edge-sensitive
or level-sensitive interrupts, and interrupt polarity should be
completed prior to enabling the interrupts on Port A in order to prevent
spurious glitches on the interrupt lines to the interrupt controller."
Reported-by: Brian Norris <briannorris@xxxxxxxxxx>
Signed-off-by: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
---
drivers/pinctrl/pinctrl-rockchip.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 3924779f55785..7ff45ec8330d1 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -2727,9 +2727,19 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
return -EINVAL;
}
+ /**
The typical multiline comment style is to use only 1 asterisk -- 2
asterisks usually denote something more formal, like kerneldoc.
ok, will fix it.
+ * According to the TRM, we should keep irq disabled during programming
+ * interrupt capability to prevent spurious glitches on the interrupt
+ * lines to the interrupt controller.
+ */
It's also worth noting that this case may come up in
rockchip_irq_demux() for EDGE_BOTH triggers:
/*
* Triggering IRQ on both rising and falling edge
* needs manual intervention.
*/
if (bank->toggle_edge_mode & BIT(irq)) {
...
polarity = readl_relaxed(bank->reg_base +
GPIO_INT_POLARITY);
if (data & BIT(irq))
polarity &= ~BIT(irq);
else
polarity |= BIT(irq);
writel(polarity,
bank->reg_base + GPIO_INT_POLARITY);
AIUI, this case is not actually a problem, because this polarity
reconfiguration happens before we call generic_handle_irq(), so the
extra spurious interrupt is handled and cleared after this point. But it
seems like this should be noted somewhere in either the commit message,
the code comments, or both.
just from my testing, the spurious irq only happen when set EDGE_RISING
to a gpio which is already high level, or set EDGE_FALLING to a gpio
which is already low level.so the demux / EDGE_BOTH seem ok.
but adding comments as your suggested is a good idea, will do that.
On the other hand...this also implies there may be a race condition
there, where we might lose an interrupt if there is an edge between the
re-configuration of the polarity in rockchip_irq_demux() and the
clearing/handling of the interrupt (handle_edge_irq() ->
chip->irq_ack()). If we have an edge between there, then we might ack
it, but leave the polarity such that we aren't ready for the next
(inverted) edge.
if let me guess, the unexpected irq we saw is the hardware trying to
avoid losing irq? for example, we set a EDGE_RISING, and the hardware
saw the gpio is already high, then though it might lost an irq, so fake
one for safe?
i'll try to confirm it with IC guys.
I'm not 100% sure about the above, so it might be good to verify it. But
I also expect this means there's really no way to 100% reliably
implement both-edge trigger types on hardware like this that doesn't
directly implement it. So I'm not sure what we'd do with that knowledge.
+ data = readl(bank->reg_base + GPIO_INTEN);
+ writel_relaxed(data & ~mask, gc->reg_base + GPIO_INTEN);
Not sure if this is a needless optimization: but couldn't you only
disable the interrupt (and make the level and polarity changes) only if
the level or polarity are actually changing? For example, it's possible
to end up here when changing from EDGE_BOTH to EDGE_RISING, but we might
not actually program a new value.
right, i noticed that too, will add a patch for that in v2.
Brian
+
writel_relaxed(level, gc->reg_base + GPIO_INTTYPE_LEVEL);
writel_relaxed(polarity, gc->reg_base + GPIO_INT_POLARITY);
+ writel_relaxed(data, gc->reg_base + GPIO_INTEN);
+
irq_gc_unlock(gc);
raw_spin_unlock_irqrestore(&bank->slock, flags);
clk_disable(bank->clk);
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
