Re: [PATCH v5 4/4] pinctrl: qcom: Don't clear pending interrupts when enabling

[Bjorn Andersson]

On Thu 14 Jan 01:14 CST 2021, Stephen Boyd wrote:

> Quoting Douglas Anderson (2021-01-08 09:35:16)
> > Let's deal with the problem like this:
> > * When we mux away, we'll mask our interrupt.  This isn't necessary in
> >   the above case since the client already masked us, but it's a good
> >   idea in general.
> > * When we mux back will clear any interrupts and unmask.
> 
> I'm on board!
> 
> > 
> > Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> > Fixes: 71266d9d3936 ("pinctrl: qcom: Move clearing pending IRQ to .irq_request_resources callback")
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > ---
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index a6b0c17e2f78..d5d1f3430c6c 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -51,6 +51,7 @@
> >   * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
> >   *                  detection.
> >   * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt controller
> > + * @disabled_for_mux: These IRQs were disabled because we muxed away.
> >   * @soc:            Reference to soc_data of platform specific data.
> >   * @regs:           Base addresses for the TLMM tiles.
> >   * @phys_base:      Physical base address
> > @@ -72,6 +73,7 @@ struct msm_pinctrl {
> >         DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
> >         DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
> >         DECLARE_BITMAP(skip_wake_irqs, MAX_NR_GPIO);
> > +       DECLARE_BITMAP(disabled_for_mux, MAX_NR_GPIO);
> >  
> >         const struct msm_pinctrl_soc_data *soc;
> >         void __iomem *regs[MAX_NR_TILES];
> > @@ -179,6 +181,10 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> >                               unsigned group)
> >  {
> >         struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > +       struct gpio_chip *gc = &pctrl->chip;
> > +       unsigned int irq = irq_find_mapping(gc->irq.domain, group);
> > +       struct irq_data *d = irq_get_irq_data(irq);
> > +       unsigned int gpio_func = pctrl->soc->gpio_func;
> >         const struct msm_pingroup *g;
> >         unsigned long flags;
> >         u32 val, mask;
> > @@ -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? 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 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 was about to write a question about why we should disable the IRQ
through the irqchip framework, rather than just do it in the hardware
directly.

Which I think means that I came to the same conclusion as you, that if
we have a pin masked to non-gpio, it will still wake the system up, just
to actually disable the IRQ lazily.

Is there a problem with leaving the irq framework to believe the IRQ is
enabled while we disable the delivery in hardware?

Regards,
Bjorn

> > +
> >         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? Although I think this is to clear out
> the pending state in the GIC and not the PDC which is the parent.
> 
> > +               else
> > +                       msm_ack_intr_status(pctrl, g);
> > +
> > +               enable_irq(irq);
> > +       }
> > +