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?
>
> > +
> > 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.
>
Isn't this the PDC line after all, because the GIC only has the summary
line while the PDC (if we have d->parent_data) has a dedicated line for
this irq?
Regards,
Bjorn
> > + else
> > + msm_ack_intr_status(pctrl, g);
> > +
> > + enable_irq(irq);
> > + }
> > +
