diff mbox series

[v2,1/3] pinctrl: msm: Really mask level interrupts to prevent latching

Message ID 20180725222900.33231-2-swboyd@chromium.org (mailing list archive)
State Not Applicable, archived
Delegated to: Andy Gross
Headers show
Series pinctrl: msm interrupt and muxing fixes | expand

Commit Message

Stephen Boyd July 25, 2018, 10:28 p.m. UTC
The interrupt controller hardware in this pin controller has two status
enable bits. The first "normal" status enable bit enables or disables
the summary interrupt line being raised when a gpio interrupt triggers
and the "raw" status enable bit allows or prevents the hardware from
latching an interrupt into the status register for a gpio interrupt.
Currently we just toggle the "normal" status enable bit in the mask and
unmask ops so that the summary irq interrupt going to the CPU's
interrupt controller doesn't trigger for the masked gpio interrupt.

For a level triggered interrupt, the flow would be as follows: the pin
controller sees the interrupt, latches the status into the status
register, raises the summary irq to the CPU, summary irq handler runs
and calls handle_level_irq(), handle_level_irq() masks and acks the gpio
interrupt, the interrupt handler runs, and finally unmask the interrupt.
When the interrupt handler completes, we expect that the interrupt line
level will go back to the deasserted state so the genirq code can unmask
the interrupt without it triggering again.

If we only mask the interrupt by clearing the "normal" status enable bit
then we'll ack the interrupt but it will continue to show up as pending
in the status register because the raw status bit is enabled, the
hardware hasn't deasserted the line, and thus the asserted state latches
into the status register again. When the hardware deasserts the
interrupt the pin controller still thinks there is a pending unserviced
level interrupt because it latched it earlier. This behavior causes
software to see an extra interrupt for level type interrupts each time
the interrupt is handled.

Let's fix this by clearing the raw status enable bit for level type
interrupts so that the hardware stops latching the status of the
interrupt after we ack it. We don't do this for edge type interrupts
because it seems that toggling the raw status enable bit for edge type
interrupts causes spurious edge interrupts.

Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Doug Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Changes from v1:
 - Squashed raw_status_bit write into same write on unmask (Doug
   Andersson)

 drivers/pinctrl/qcom/pinctrl-msm.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Doug Anderson Aug. 13, 2018, 11:53 p.m. UTC | #1
Hi,

On Wed, Jul 25, 2018 at 3:28 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> The interrupt controller hardware in this pin controller has two status
> enable bits. The first "normal" status enable bit enables or disables
> the summary interrupt line being raised when a gpio interrupt triggers
> and the "raw" status enable bit allows or prevents the hardware from
> latching an interrupt into the status register for a gpio interrupt.
> Currently we just toggle the "normal" status enable bit in the mask and
> unmask ops so that the summary irq interrupt going to the CPU's
> interrupt controller doesn't trigger for the masked gpio interrupt.
>
> For a level triggered interrupt, the flow would be as follows: the pin
> controller sees the interrupt, latches the status into the status
> register, raises the summary irq to the CPU, summary irq handler runs
> and calls handle_level_irq(), handle_level_irq() masks and acks the gpio
> interrupt, the interrupt handler runs, and finally unmask the interrupt.
> When the interrupt handler completes, we expect that the interrupt line
> level will go back to the deasserted state so the genirq code can unmask
> the interrupt without it triggering again.
>
> If we only mask the interrupt by clearing the "normal" status enable bit
> then we'll ack the interrupt but it will continue to show up as pending
> in the status register because the raw status bit is enabled, the
> hardware hasn't deasserted the line, and thus the asserted state latches
> into the status register again. When the hardware deasserts the
> interrupt the pin controller still thinks there is a pending unserviced
> level interrupt because it latched it earlier. This behavior causes
> software to see an extra interrupt for level type interrupts each time
> the interrupt is handled.
>
> Let's fix this by clearing the raw status enable bit for level type
> interrupts so that the hardware stops latching the status of the
> interrupt after we ack it. We don't do this for edge type interrupts
> because it seems that toggling the raw status enable bit for edge type
> interrupts causes spurious edge interrupts.
>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>
> Changes from v1:
>  - Squashed raw_status_bit write into same write on unmask (Doug
>    Andersson)
>
>  drivers/pinctrl/qcom/pinctrl-msm.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 2155a30c282b..3970dc599092 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -634,6 +634,19 @@ static void msm_gpio_irq_mask(struct irq_data *d)
>         raw_spin_lock_irqsave(&pctrl->lock, flags);
>
>         val = readl(pctrl->regs + g->intr_cfg_reg);
> +       /*
> +        * Leaving the RAW_STATUS_EN bit enabled causes level interrupts that
> +        * are still asserted to re-latch after we ack them. Clear the raw
> +        * status enable bit too so the interrupt can't even latch into the
> +        * hardware while it's masked, but only do this for level interrupts
> +        * because edge interrupts have a problem with the raw status bit
> +        * toggling and causing spurious interrupts.

This whole "spurious interrupts" explanation still seems dodgy.  Have
you experienced it yourself, or is this looking through some previous
commits?  As per my comments in v1, I'd still rather the comment state
the reason as: it's important to _not_ lose edge interrupts when
masked.


> +        */
> +       if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
> +               val &= ~BIT(g->intr_raw_status_bit);
> +               writel(val, pctrl->regs + g->intr_cfg_reg);
> +       }

In v1 you claimed you were going to combine this with the next write
(you said you'd combine things in both mask and unmask).  ...is there
a reason why you didn't?  As per my comments in v1 I believe it's
safer from a correctness point of view to combine them.


-Doug
Stephen Boyd Aug. 16, 2018, 6:19 p.m. UTC | #2
Quoting Doug Anderson (2018-08-13 16:53:42)
> Hi,
> 
> On Wed, Jul 25, 2018 at 3:28 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index 2155a30c282b..3970dc599092 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -634,6 +634,19 @@ static void msm_gpio_irq_mask(struct irq_data *d)
> >         raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> >         val = readl(pctrl->regs + g->intr_cfg_reg);
> > +       /*
> > +        * Leaving the RAW_STATUS_EN bit enabled causes level interrupts that
> > +        * are still asserted to re-latch after we ack them. Clear the raw
> > +        * status enable bit too so the interrupt can't even latch into the
> > +        * hardware while it's masked, but only do this for level interrupts
> > +        * because edge interrupts have a problem with the raw status bit
> > +        * toggling and causing spurious interrupts.
> 
> This whole "spurious interrupts" explanation still seems dodgy.  Have
> you experienced it yourself, or is this looking through some previous
> commits? 

There's a similar comment in the code already from the initial commit.
See msm_gpio_irq_set_type() and this really old commit from downstream
kernels[1]. I haven't seen a problem with this myself, but I believe
there have been problems with the mask/unmask causing an interrupt to
latch into the hardware just because we change the raw status enable
bit. For level type interrupts this isn't a problem because the level is
'present' when we mask it so toggling raw status can't cause a spurious
level to be seen.

For edge types I don't believe this is a problem we need to solve. We
may mask the edge because it just keeps coming during IRQ processing and
then when we ack the line while it's masked it will be cleared out of
the status register and only relatch when the edge comes back. It only
becomes a spurious irq if we toggle this raw status bit, but we don't
ever need to do that for edge type interrupts because we always want the
edge to latch into the hardware after we ack it (even if it's masked).
So the irq status for edge types should always be forwarded to the
status register and it should always latch into the status register, but
when the irq is physically masked we don't want the CPU to be
interrupted when that happens, we just want to remember it happened to
make sure we replay the interrupt later on unmask.

> As per my comments in v1, I'd still rather the comment state
> the reason as: it's important to _not_ lose edge interrupts when
> masked.

Ok! Let me try to rewrite this comment with the reasoning for level and
edge types.

> 
> 
> > +        */
> > +       if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
> > +               val &= ~BIT(g->intr_raw_status_bit);
> > +               writel(val, pctrl->regs + g->intr_cfg_reg);
> > +       }
> 
> In v1 you claimed you were going to combine this with the next write
> (you said you'd combine things in both mask and unmask).  ...is there
> a reason why you didn't?  As per my comments in v1 I believe it's
> safer from a correctness point of view to combine them.
> 

Hmm I seem to have mis-copied the patch from one directory to another.
I'll resend with both combined because I tested that.

[1] https://source.codeaurora.org/quic/la/kernel/msm/commit/?h=msm-3.4&id=ea45a49ee524cfafe136c0ac67623e64e614ba27
diff mbox series

Patch

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 2155a30c282b..3970dc599092 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -634,6 +634,19 @@  static void msm_gpio_irq_mask(struct irq_data *d)
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = readl(pctrl->regs + g->intr_cfg_reg);
+	/*
+	 * Leaving the RAW_STATUS_EN bit enabled causes level interrupts that
+	 * are still asserted to re-latch after we ack them. Clear the raw
+	 * status enable bit too so the interrupt can't even latch into the
+	 * hardware while it's masked, but only do this for level interrupts
+	 * because edge interrupts have a problem with the raw status bit
+	 * toggling and causing spurious interrupts.
+	 */
+	if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
+		val &= ~BIT(g->intr_raw_status_bit);
+		writel(val, pctrl->regs + g->intr_cfg_reg);
+	}
+
 	val &= ~BIT(g->intr_enable_bit);
 	writel(val, pctrl->regs + g->intr_cfg_reg);
 
@@ -655,6 +668,7 @@  static void msm_gpio_irq_unmask(struct irq_data *d)
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = readl(pctrl->regs + g->intr_cfg_reg);
+	val |= BIT(g->intr_raw_status_bit);
 	val |= BIT(g->intr_enable_bit);
 	writel(val, pctrl->regs + g->intr_cfg_reg);