Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210108093339.v5.1.I3ad184e3423d8e479bc3e86f5b393abb1704a1d1@changeid>
 <20210108093339.v5.4.I7cf3019783720feb57b958c95c2b684940264cd1@changeid> <161060848425.3661239.17417977666663714149@swboyd.mtv.corp.google.com>
In-Reply-To: <161060848425.3661239.17417977666663714149@swboyd.mtv.corp.google.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 14 Jan 2021 09:58:55 -0800
Message-ID: <CAD=FV=Vg1PNVPhCH9GPX=F9-EHhSGF9NaXfui+rM4SQEtA=ZMA@mail.gmail.com>
Subject: Re: [PATCH v5 4/4] pinctrl: qcom: Don't clear pending interrupts when enabling
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Jason Cooper <jason@lakedaemon.net>,
        Linus Walleij <linus.walleij@linaro.org>,
        Marc Zyngier <maz@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Neeraj Upadhyay <neeraju@codeaurora.org>,
        Rajendra Nayak <rnayak@codeaurora.org>,
        Maulik Shah <mkshah@codeaurora.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Srinivas Ramana <sramana@codeaurora.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Andy Gross <agross@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Wed, Jan 13, 2021 at 11:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> > @@ -195,6 +201,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> >         if (WARN_ON(i == g->nfuncs))
> >                 return -EINVAL;
> >
> > +       /*
> > +        * If an GPIO interrupt is setup on this pin then we need special
> > +        * handling.  Specifically interrupt detection logic will still see
> > +        * the pin twiddle even when we're muxed away.
> > +        *
> > +        * When we see a pin with an interrupt setup on it then we'll disable
> > +        * (mask) interrupts on it when we mux away until we mux back.  Note
> > +        * that disable_irq() refcounts and interrupts are disabled as long as
> > +        * at least one disable_irq() has been called.
> > +        */
> > +       if (d && i != gpio_func &&
> > +           !test_and_set_bit(d->hwirq, pctrl->disabled_for_mux))
> > +               disable_irq(irq);
>
> Does it need to be forced non-lazy so that it is actually disabled at
> the GIC?

Yes, I think non-lazy is important.  Specifically at the end I assume
that I can clear the interrupt in hardware and it will go away and
Linux never saw it.  If it was lazy, it's possible Linux saw the
interrupt and has it marked with IRQS_PENDING.

Right now we get non-lazy because we have "disable" implemented, so it
works fine.  I can be explicit.  Do I add a call to
msm_gpio_irq_reqres() like:

  irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY);

I'll wait for feedback if you think this is the right way to go before
sending the next version.


> I'm trying to understand how the lazy irq disabling plays into
> this. I think it's a don't care situation because if the line twiddles
> and triggers an irq then we'll actually disable it at the GIC in the
> genirq core and mark it pending for resend.

I think the marking as pending is a problem.  When we finally mux back
to GPIO we want to clear out anything that showed up while it was
muxed away and I'm not aware of a way to clear "IRQS_PENDING".


> I wonder if we wouldn't have
> to undo the pending state if we actually ignored it at the GIC
> forcefully. And I also worry that it may cause a random wakeup if the
> line twiddles, becomes pending at GIC and thus blocks the CPU from
> running a WFI but it isn't an irq that Linux cares about because it's
> muxed to UART, and then lazy handling runs and shuts it down. Is that
> possible?

I believe if the interrupt is masked at the GIC then it won't cause
wakeups.  Specifically to get wakeup stuff working we had to unmask
the interrupt at the GIC level.


> > +
> >         raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> >         val = msm_readl_ctl(pctrl, g);
> > @@ -204,6 +224,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> >
> >         raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >
> > +       if (d && i == gpio_func &&
> > +           test_and_clear_bit(d->hwirq, pctrl->disabled_for_mux)) {
> > +               /*
> > +                * Clear interrupts detected while not GPIO since we only
> > +                * masked things.
> > +                */
> > +               if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
> > +                       irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
>
> So if not lazy this could go away?

I don't think so.  If lazy we could have a pending interrupt tracked
in two places: in Linux and in the parent if this happened:
* disable_irq() - do nothing except mark that IRQ is disalbed.
* IRQ happened - track in Linux and actually disable (mask) the interrupt
* IRQ happened again - still tracked in Linux but now also latched in
hardware (but masked)

...so if it was lazy we'd need to clear the interrupt in two places.
With non-lazy we only have to clear the latch that happened in
hardware, right?

> Although I think this is to clear out
> the pending state in the GIC and not the PDC which is the parent.

Yeah, it clears the state that was latched in the GIC.  It just passes
through the PDC code on the way there.