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

[Stephen Boyd]

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?

> +
>         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);
> +       }
> +