Hi,
On Mon, Dec 21, 2020 at 8:01 AM Maulik Shah <mkshah@xxxxxxxxxxxxxx> wrote:
>
> Hi Doug,
>
> On 12/12/2020 3:45 AM, Douglas Anderson wrote:
> > In Linux, if a driver does disable_irq() and later does enable_irq()
> > on its interrupt, I believe it's expecting these properties:
> > * If an interrupt was pending when the driver disabled then it will
> > still be pending after the driver re-enables.
> > * If an edge-triggered interrupt comes in while an interrupt is
> > disabled it should assert when the interrupt is re-enabled.
> >
> > If you think that the above sounds a lot like the disable_irq() and
> > enable_irq() are supposed to be masking/unmasking the interrupt
> > instead of disabling/enabling it then you've made an astute
> > observation. Specifically when talking about interrupts, "mask"
> > usually means to stop posting interrupts but keep tracking them and
> > "disable" means to fully shut off interrupt detection. It's
> > unfortunate that this is so confusing, but presumably this is all the
> > way it is for historical reasons.
> >
> > Perhaps more confusing than the above is that, even though clients of
> > IRQs themselves don't have a way to request mask/unmask
> > vs. disable/enable calls, IRQ chips themselves can implement both.
> > ...and yet more confusing is that if an IRQ chip implements
> > disable/enable then they will be called when a client driver calls
> > disable_irq() / enable_irq().
> >
> > It does feel like some of the above could be cleared up. However,
> > without any other core interrupt changes it should be clear that when
> > an IRQ chip gets a request to "disable" an IRQ that it has to treat it
> > like a mask of that IRQ.
> >
> > In any case, after that long interlude you can see that the "unmask
> > and clear" can break things. Maulik tried to fix it so that we no
> > longer did "unmask and clear" in commit 71266d9d3936 ("pinctrl: qcom:
> > Move clearing pending IRQ to .irq_request_resources callback"), but it
> > only handled the PDC case (it also had problems, but that's the
> > subject of another patch). Let's fix this for the non-PDC case.
> >
> > From my understanding the source of the phantom interrupt in the
> > non-PDC case was the one that could have been introduced in
> > msm_gpio_irq_set_type(). Let's handle that one and then get rid of
> > the clear.
> >
> > Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > ---
> > I don't have lots of good test cases here, so hopefully someone from
> > Qualcomm can confirm that this works well for them and there isn't
> > some other phantom interrupt source that I'm not aware of.
> >
> > Changes in v4:
> > - ("pinctrl: qcom: Don't clear pending interrupts when enabling") split for v4.
> >
> > drivers/pinctrl/qcom/pinctrl-msm.c | 32 +++++++++++++-----------------
> > 1 file changed, 14 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index 588df91274e2..f785646d1df7 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -774,7 +774,7 @@ static void msm_gpio_irq_mask(struct irq_data *d)
> > raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > }
> >
> > -static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
> > +static void msm_gpio_irq_unmask(struct irq_data *d)
> > {
> > struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> > @@ -792,17 +792,6 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
> >
> > raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> > - if (status_clear) {
> > - /*
> > - * clear the interrupt status bit before unmask to avoid
> > - * any erroneous interrupts that would have got latched
> > - * when the interrupt is not in use.
> > - */
> > - val = msm_readl_intr_status(pctrl, g);
> > - val &= ~BIT(g->intr_status_bit);
> > - msm_writel_intr_status(val, pctrl, g);
> > - }
> > -
> Removing above does not cover the case where GPIO IRQ do not have parent
> PDC.
>
> Specifically, for edge IRQs during masking we donot clear
> intr_raw_status_bit.
> see below at msm_gpio_irq_mask()
>
> if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK)
> val &= ~BIT(g->intr_raw_status_bit);
>
> we have to keep the bit set anyway so that edges are latched while the
> line is masked.
>
> The problem is even when GPIO is set to some other function like
> "mi2s_2" it can still sense the line at make
> interrupt pending depending on the line toggle if intr_raw_status_bit is
> left set.
Ah, so it's the same problem as we have with the PDC. Makes sense.
> I have thought of solution to this,
>
> 1) During msm_gpio_irq_mask() we keep intr_raw_status_bit set already in
> today's code
> This will make edges to latch when the line is masked.
> so no change required for this.
>
> 2) During msm_pinmux_set_mux() if we set GPIO to anyother function than
> GPIO interrupt mode,
> we clear intr_raw_status_bit, so the interrupt cannot latch when GPIO is
> used in other function.
> Below snippet can be inserted in msm_pinmux_set_mux()
>
> val |= i << g->mux_bit;
> msm_writel_ctl(val, pctrl, g);
>
> + if (i != gpio_func) {
> + val = msm_readl_intr_cfg(pctrl, g);
> + val &= ~BIT(g->intr_raw_status_bit);
> + msm_writel_intr_cfg(val, pctrl, g);
> + }
> +
> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>
> 3) During msm_gpio_irq_unmask(), if the intr_raw_status_bit is not set,
> then clear the pending IRQ.
> specifically setting this bit itself can cause the error IRQ, so clear
> it when setting this.
>
> for edge IRQ, intr_raw_status_bit can only be cleared in
> msm_pinmux_set_mux() so clearing pending
> IRQ should not loose any edges since we know GPIO was used in other
> function mode like mi2s_2 for
> which we do not need to latch IRQs.
> Below snippet can be inserted in msm_gpio_irq_unmask()
>
> + was_enabled = val & BIT(g->intr_raw_status_bit);
> val |= BIT(g->intr_raw_status_bit);
> val |= BIT(g->intr_enable_bit);
> msm_writel_intr_cfg(val, pctrl, g);
>
> + if (!was_enabled) {
> + val = msm_readl_intr_status(pctrl, g);
> + val &= ~BIT(g->intr_status_bit);
> + msm_writel_intr_status(val, pctrl, g);
> + }
> +
> set_bit(d->hwirq, pctrl->enabled_irqs);
>
> This can cover the cases for which the GPIO do not have parent.
I think your solution can be made to work, but I think also we can
just use the exact same solution that I already came up with in patch
#4. We can leave the "raw" bit alone and just mask the interrupt when
we switch the mux, then clear the interrupt when we switch back.
I've now combined the PDC/non-PDC cases and it actually turned out
fairly clean I think. See what you think about v5.
-Doug
