Message-ID: <CAD=FV=WNp06dE3J1KcNZBBW9_07tz_vAzoutEupb74cwae+W=w@mail.gmail.com>
Date:   Fri, 8 Jan 2021 09:37:42 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Maulik Shah <mkshah@...eaurora.org>
Cc:     Marc Zyngier <maz@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Stephen Boyd <swboyd@...omium.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Srinivas Ramana <sramana@...eaurora.org>,
        Neeraj Upadhyay <neeraju@...eaurora.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Andy Gross <agross@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/4] pinctrl: qcom: Don't clear pending interrupts when enabling

Hi,

On Mon, Dec 21, 2020 at 8:01 AM Maulik Shah <mkshah@...eaurora.org> wrote:
>
> Hi Doug,
>
> On 12/12/2020 3:45 AM, Douglas Anderson wrote:
> > In Linux, if a driver does disable_irq() and later does enable_irq()
> > on its interrupt, I believe it's expecting these properties:
> > * If an interrupt was pending when the driver disabled then it will
> >    still be pending after the driver re-enables.
> > * If an edge-triggered interrupt comes in while an interrupt is
> >    disabled it should assert when the interrupt is re-enabled.
> >
> > If you think that the above sounds a lot like the disable_irq() and
> > enable_irq() are supposed to be masking/unmasking the interrupt
> > instead of disabling/enabling it then you've made an astute
> > observation.  Specifically when talking about interrupts, "mask"
> > usually means to stop posting interrupts but keep tracking them and
> > "disable" means to fully shut off interrupt detection.  It's
> > unfortunate that this is so confusing, but presumably this is all the
> > way it is for historical reasons.
> >
> > Perhaps more confusing than the above is that, even though clients of
> > IRQs themselves don't have a way to request mask/unmask
> > vs. disable/enable calls, IRQ chips themselves can implement both.
> > ...and yet more confusing is that if an IRQ chip implements
> > disable/enable then they will be called when a client driver calls
> > disable_irq() / enable_irq().
> >
> > It does feel like some of the above could be cleared up.  However,
> > without any other core interrupt changes it should be clear that when
> > an IRQ chip gets a request to "disable" an IRQ that it has to treat it
> > like a mask of that IRQ.
> >
> > In any case, after that long interlude you can see that the "unmask
> > and clear" can break things.  Maulik tried to fix it so that we no
> > longer did "unmask and clear" in commit 71266d9d3936 ("pinctrl: qcom:
> > Move clearing pending IRQ to .irq_request_resources callback"), but it
> > only handled the PDC case (it also had problems, but that's the
> > subject of another patch).  Let's fix this for the non-PDC case.
> >
> >  From my understanding the source of the phantom interrupt in the
> > non-PDC case was the one that could have been introduced in
> > msm_gpio_irq_set_type().  Let's handle that one and then get rid of
> > the clear.
> >
> > Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> > Signed-off-by: Douglas Anderson <dianders@...omium.org>
> > ---
> > I don't have lots of good test cases here, so hopefully someone from
> > Qualcomm can confirm that this works well for them and there isn't
> > some other phantom interrupt source that I'm not aware of.
> >
> > Changes in v4:
> > - ("pinctrl: qcom: Don't clear pending interrupts when enabling") split for v4.
> >
> >   drivers/pinctrl/qcom/pinctrl-msm.c | 32 +++++++++++++-----------------
> >   1 file changed, 14 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index 588df91274e2..f785646d1df7 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -774,7 +774,7 @@ static void msm_gpio_irq_mask(struct irq_data *d)
> >       raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >   }
> >
> > -static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
> > +static void msm_gpio_irq_unmask(struct irq_data *d)
> >   {
> >       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> >       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> > @@ -792,17 +792,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);
> > -     }
> > -
> Removing above does not cover the case where GPIO IRQ do not have parent
> PDC.
>
> Specifically, for edge IRQs during masking we donot clear
> intr_raw_status_bit.
> see below at msm_gpio_irq_mask()
>
>          if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK)
>                  val &= ~BIT(g->intr_raw_status_bit);
>
> we have to keep the bit set anyway so that edges are latched while the
> line is masked.
>
> The problem is even when GPIO is set to some other function like
> "mi2s_2" it can still sense the line at make
> interrupt pending depending on the line toggle if intr_raw_status_bit is
> left set.

Ah, so it's the same problem as we have with the PDC.  Makes sense.


> I have thought of solution to this,
>
> 1) During msm_gpio_irq_mask() we keep intr_raw_status_bit set already in
> today's code
> This will make edges to latch when the line is masked.
> so no change required for this.
>
> 2) During msm_pinmux_set_mux() if we set GPIO to anyother function than
> GPIO interrupt mode,
> we clear intr_raw_status_bit, so the interrupt cannot latch when GPIO is
> used in other function.
> Below snippet can be inserted in msm_pinmux_set_mux()
>
>          val |= i << g->mux_bit;
>          msm_writel_ctl(val, pctrl, g);
>
> +        if (i != gpio_func) {
> +                val = msm_readl_intr_cfg(pctrl, g);
> +                val &= ~BIT(g->intr_raw_status_bit);
> +                msm_writel_intr_cfg(val, pctrl, g);
> +        }
> +
>          raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>
> 3) During msm_gpio_irq_unmask(), if the intr_raw_status_bit is not set,
> then clear the pending IRQ.
> specifically setting this bit itself can cause the error IRQ, so clear
> it when setting this.
>
> for edge IRQ, intr_raw_status_bit can only be cleared in
> msm_pinmux_set_mux() so clearing pending
> IRQ should not loose any edges since we know GPIO was used in other
> function mode like mi2s_2 for
> which we do not need to latch IRQs.
> Below snippet can be inserted in msm_gpio_irq_unmask()
>
> +       was_enabled = val & BIT(g->intr_raw_status_bit);
>          val |= BIT(g->intr_raw_status_bit);
>          val |= BIT(g->intr_enable_bit);
>          msm_writel_intr_cfg(val, pctrl, g);
>
> +       if (!was_enabled) {
> +               val = msm_readl_intr_status(pctrl, g);
> +               val &= ~BIT(g->intr_status_bit);
> +               msm_writel_intr_status(val, pctrl, g);
> +       }
> +
>          set_bit(d->hwirq, pctrl->enabled_irqs);
>
> This can cover the cases for which the GPIO do not have parent.

I think your solution can be made to work, but I think also we can
just use the exact same solution that I already came up with in patch
#4.  We can leave the "raw" bit alone and just mask the interrupt when
we switch the mux, then clear the interrupt when we switch back.

I've now combined the PDC/non-PDC cases and it actually turned out
fairly clean I think.  See what you think about v5.

-Doug

Powered by blists - more mailing lists