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