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