lkml.org 
[lkml]   [2020]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs
Hi,

On Mon, Nov 30, 2020 at 2:33 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> > [1] https://lore.kernel.org/r/603c691f-3614-d87b-075a-0889e9ffc453@codeaurora.org
> Please wait to land [1] before i confirm with HW team if this is indeed
> valid case.

Oh, oops. Somehow I thought your reply was in response to patch #3 in
the series, not #1. I responded to patch #1 in the series now to make
it clear to wait for you.


> > @@ -187,15 +217,26 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> > if (WARN_ON(i == g->nfuncs))
> > return -EINVAL;
> >
> > - raw_spin_lock_irqsave(&pctrl->lock, flags);
> > + disable_irq(irq);
> >
> > - val = msm_readl_ctl(pctrl, g);
> > + raw_spin_lock_irqsave(&pctrl->lock, flags);
> > + oldval = val = msm_readl_ctl(pctrl, g);
> > val &= ~mask;
> > val |= i << g->mux_bit;
> > msm_writel_ctl(val, pctrl, g);
> > -
> > raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >
> > + /*
> > + * 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?

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.


> > @@ -456,32 +497,49 @@ static const struct pinconf_ops msm_pinconf_ops = {
> > static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> > {
> > const struct msm_pingroup *g;
> > + unsigned int irq = irq_find_mapping(chip->irq.domain, offset);
> > struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
> > unsigned long flags;
> > + u32 oldval;
> > u32 val;
> >
> > g = &pctrl->soc->groups[offset];
> >
> > + disable_irq(irq);
> > +
> > raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> > - val = msm_readl_ctl(pctrl, g);
> > + oldval = val = msm_readl_ctl(pctrl, g);
> > val &= ~BIT(g->oe_bit);
> > msm_writel_ctl(val, pctrl, g);
> >
> > raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >
> > + /*
> > + * Clear IRQs if switching to/from input mode since that can use
> > + * a phantom edge.
> > + */
> > + if (oldval != val)
> > + msm_pinctrl_clear_pending_irq(pctrl, offset, irq);
> same as above, can you clear this unconditionally.

Any reason why? If we didn't change anything then there's no reason
to go through all this extra code?


> > static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, int value)
> > {
> > const struct msm_pingroup *g;
> > + unsigned int irq = irq_find_mapping(chip->irq.domain, offset);
> > struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
> > unsigned long flags;
> > + u32 oldval;
> > u32 val;
> >
> > g = &pctrl->soc->groups[offset];
> >
> > + disable_irq(irq);
> > +
> > raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> > val = msm_readl_io(pctrl, g);
> > @@ -491,12 +549,21 @@ static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, in
> > val &= ~BIT(g->out_bit);
> > msm_writel_io(val, pctrl, g);
> >
> > - val = msm_readl_ctl(pctrl, g);
> > + oldval = msm_readl_ctl(pctrl, g);
>
> should be, oldval = val = msm_readl_ctl(pctrl, g);
>
> otherwise val will carry invalid value.

Whoa! Good catch. How did I miss that and how did it not fail? I
will fix in a v3 but will wait until other questions are resolved
before sending.


> > val |= BIT(g->oe_bit);
> > msm_writel_ctl(val, pctrl, g);
> >
> > raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >
> > + /*
> > + * Clear IRQs if switching to/from input mode since that can use
> > + * a phantom edge.
> > + */
> > + if (oldval != val)
> > + msm_pinctrl_clear_pending_irq(pctrl, offset, irq);
>
> i don't see a reason to clear the edges when switching to output mode.
>
> can you remove the changes from .direction_output callback?

I haven't confirmed that this can glitch, however I did confirm that I
could glitch when muxing _away_ from GPIO mode. This makes me believe
that I could also glitch when muxing to an output.

I can try to concoct a test for this if necessary.


> > @@ -792,17 +859,6 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
> >
> > raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> > - if (status_clear) {
> > - /*
> > - * clear the interrupt status bit before unmask to avoid
> > - * any erroneous interrupts that would have got latched
> > - * when the interrupt is not in use.
> > - */
> > - val = msm_readl_intr_status(pctrl, g);
> > - val &= ~BIT(g->intr_status_bit);
> > - msm_writel_intr_status(val, pctrl, g);
> > - }
> > -
> Above change was clearing irq in .irq_enable callback which will do
> clear + unmask from irq_startup() at the very end.
> With your change, The problem is we have cleared the phantom irq much
> earlier in __setup_irq() phase and in below case its still latched as
> pending.
>
> 1. The client driver calls request_irq() => __setup_irq()
> 2. __setup_irq() then first invokes irq_request_resources() =>
> msm_gpio_irq_reqres() => msm_pinmux_set_mux() =>
> msm_pinctrl_clear_pending_irq()
> 3. __setup_irq() goes ahead and invokes __irq_set_trigger() =>
> msm_gpio_irq_set_type()
> 4. __setup_irq() then invokes irq_startup() => gpiochip_irq_enable() =>
> msm_gpio_irq_enable()
>
> The phantom irq gets cleared in step (2) here, but with step (3) it gets
> latched again and at the end of step (4) still get phantom irq.
> This seems because as per below comment in driver, pasting the part
> which has info,
> /*
> * The edge detection logic seems to have a problem where toggling the
> RAW_STATUS_EN bit may
> * cause the status bit to latch spuriously when there isn't any edge
> */
> 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().


> > val = msm_readl_intr_cfg(pctrl, g);
> > val |= BIT(g->intr_raw_status_bit);
> > val |= BIT(g->intr_enable_bit);
> > @@ -815,14 +871,10 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
> >
> > static void msm_gpio_irq_enable(struct irq_data *d)
> > {
> > - struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > - struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> > -
> > if (d->parent_data)
> > irq_chip_enable_parent(d);
> >
> > - if (!test_bit(d->hwirq, pctrl->skip_wake_irqs))
> > - msm_gpio_irq_clear_unmask(d, true);
> > + msm_gpio_irq_unmask(d);
>
> Still need the above if condition, the previous call
> irq_chip_enable_parent() already enabled the IRQ at PDC and GIC, so only
> go ahead to enable it at TLMM if there wasn't any parent.
>
> if (!test_bit(d->hwirq, pctrl->skip_wake_irqs))
> msm_gpio_irq_unmask(d);

Right. I'll fix it when I send the v3. Thanks!

-Doug

\
 
 \ /
  Last update: 2020-11-30 22:46    [W:0.234 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site