Message-ID: <CAD=FV=UOSkHQMcSV8Zq5qPfBoUu5xYzfNZqUPmymvD7PXUAN4w@mail.gmail.com>
Date:   Mon, 30 Nov 2020 13:44:35 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Maulik Shah <mkshah@...eaurora.org>,
        Rajendra Nayak <rnayak@...eaurora.org>
Cc:     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 Mon, Nov 30, 2020 at 2:33 AM Maulik Shah <mkshah@...eaurora.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

Powered by blists - more mailing lists