Hi, On Mon, Nov 30, 2020 at 2:33 AM Maulik Shah <mkshah@xxxxxxxxxxxxxx> wrote: > > > [1] https://lore.kernel.org/r/603c691f-3614-d87b-075a-0889e9ffc453@xxxxxxxxxxxxxx > Please wait to land [1] before i confirm with HW team if this is indeed > valid case. Oh, oops. Somehow I thought your reply was in response to patch #3 in the series, not #1. I responded to patch #1 in the series now to make it clear to wait for you. > > @@ -187,15 +217,26 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, > > if (WARN_ON(i == g->nfuncs)) > > return -EINVAL; > > > > - raw_spin_lock_irqsave(&pctrl->lock, flags); > > + disable_irq(irq); > > > > - val = msm_readl_ctl(pctrl, g); > > + raw_spin_lock_irqsave(&pctrl->lock, flags); > > + oldval = val = msm_readl_ctl(pctrl, g); > > val &= ~mask; > > val |= i << g->mux_bit; > > msm_writel_ctl(val, pctrl, g); > > - > > raw_spin_unlock_irqrestore(&pctrl->lock, flags); > > > > + /* > > + * Clear IRQs if switching to/from GPIO mode since muxing to/from > > + * the GPIO path can cause phantom edges. > > + */ > > + old_i = (oldval & mask) >> g->mux_bit; > > + if (old_i != i && > > + (i == pctrl->soc->gpio_func || old_i == pctrl->soc->gpio_func)) > > + msm_pinctrl_clear_pending_irq(pctrl, group, irq); > > + > > The phantom irq can come when switching to GPIO irq mode. so may be only > check if (i == pctrl->soc->gpio_func) { Have you tested this experimentally? I have experimentally tested this and I can actually see an interrupt generated when I _leave_ GPIO as well as when I enter GPIO mode. If you can't see this I can re-setup my test, but this was one of those things that convinced me that the _transition_ is what was causing the fake interrupt. I think my test CL <https://crrev.com/c/2556012/> can help you with testing if you wish. > even better if you can clear this unconditionally. Why? It should only matter if we're going to/from GPIO mode. > > @@ -456,32 +497,49 @@ static const struct pinconf_ops msm_pinconf_ops = { > > static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset) > > { > > const struct msm_pingroup *g; > > + unsigned int irq = irq_find_mapping(chip->irq.domain, offset); > > struct msm_pinctrl *pctrl = gpiochip_get_data(chip); > > unsigned long flags; > > + u32 oldval; > > u32 val; > > > > g = &pctrl->soc->groups[offset]; > > > > + disable_irq(irq); > > + > > raw_spin_lock_irqsave(&pctrl->lock, flags); > > > > - val = msm_readl_ctl(pctrl, g); > > + oldval = val = msm_readl_ctl(pctrl, g); > > val &= ~BIT(g->oe_bit); > > msm_writel_ctl(val, pctrl, g); > > > > raw_spin_unlock_irqrestore(&pctrl->lock, flags); > > > > + /* > > + * Clear IRQs if switching to/from input mode since that can use > > + * a phantom edge. > > + */ > > + if (oldval != val) > > + msm_pinctrl_clear_pending_irq(pctrl, offset, irq); > same as above, can you clear this unconditionally. Any reason why? If we didn't change anything then there's no reason to go through all this extra code? > > static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, int value) > > { > > const struct msm_pingroup *g; > > + unsigned int irq = irq_find_mapping(chip->irq.domain, offset); > > struct msm_pinctrl *pctrl = gpiochip_get_data(chip); > > unsigned long flags; > > + u32 oldval; > > u32 val; > > > > g = &pctrl->soc->groups[offset]; > > > > + disable_irq(irq); > > + > > raw_spin_lock_irqsave(&pctrl->lock, flags); > > > > val = msm_readl_io(pctrl, g); > > @@ -491,12 +549,21 @@ static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, in > > val &= ~BIT(g->out_bit); > > msm_writel_io(val, pctrl, g); > > > > - val = msm_readl_ctl(pctrl, g); > > + oldval = msm_readl_ctl(pctrl, g); > > should be, oldval = val = msm_readl_ctl(pctrl, g); > > otherwise val will carry invalid value. Whoa! Good catch. How did I miss that and how did it not fail? I will fix in a v3 but will wait until other questions are resolved before sending. > > val |= BIT(g->oe_bit); > > msm_writel_ctl(val, pctrl, g); > > > > raw_spin_unlock_irqrestore(&pctrl->lock, flags); > > > > + /* > > + * Clear IRQs if switching to/from input mode since that can use > > + * a phantom edge. > > + */ > > + if (oldval != val) > > + msm_pinctrl_clear_pending_irq(pctrl, offset, irq); > > i don't see a reason to clear the edges when switching to output mode. > > can you remove the changes from .direction_output callback? I haven't confirmed that this can glitch, however I did confirm that I could glitch when muxing _away_ from GPIO mode. This makes me believe that I could also glitch when muxing to an output. I can try to concoct a test for this if necessary. > > @@ -792,17 +859,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); > > - } > > - > Above change was clearing irq in .irq_enable callback which will do > clear + unmask from irq_startup() at the very end. > With your change, The problem is we have cleared the phantom irq much > earlier in __setup_irq() phase and in below case its still latched as > pending. > > 1. The client driver calls request_irq() => __setup_irq() > 2. __setup_irq() then first invokes irq_request_resources() => > msm_gpio_irq_reqres() => msm_pinmux_set_mux() => > msm_pinctrl_clear_pending_irq() > 3. __setup_irq() goes ahead and invokes __irq_set_trigger() => > msm_gpio_irq_set_type() > 4. __setup_irq() then invokes irq_startup() => gpiochip_irq_enable() => > msm_gpio_irq_enable() > > The phantom irq gets cleared in step (2) here, but with step (3) it gets > latched again and at the end of step (4) still get phantom irq. > This seems because as per below comment in driver, pasting the part > which has info, > /* > * The edge detection logic seems to have a problem where toggling the > RAW_STATUS_EN bit may > * cause the status bit to latch spuriously when there isn't any edge > */ > In step (3) msm_gpio_irq_set_type() touches the RAW_STATUS_EN making the > phantom irq pending again. > To resolve this, you will need to invoke msm_pinctrl_clear_pending_irq() > at the end of the msm_gpio_irq_set_type(). > > I would like Rajendra's (already in cc) review as well on above part. Ugh, so we need a clear in yet another place. Joy. OK, I will wait for Rajendra's comment but I can add similar code in msm_gpio_irq_enable(). > > val = msm_readl_intr_cfg(pctrl, g); > > val |= BIT(g->intr_raw_status_bit); > > val |= BIT(g->intr_enable_bit); > > @@ -815,14 +871,10 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear) > > > > static void msm_gpio_irq_enable(struct irq_data *d) > > { > > - struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > - struct msm_pinctrl *pctrl = gpiochip_get_data(gc); > > - > > if (d->parent_data) > > irq_chip_enable_parent(d); > > > > - if (!test_bit(d->hwirq, pctrl->skip_wake_irqs)) > > - msm_gpio_irq_clear_unmask(d, true); > > + msm_gpio_irq_unmask(d); > > Still need the above if condition, the previous call > irq_chip_enable_parent() already enabled the IRQ at PDC and GIC, so only > go ahead to enable it at TLMM if there wasn't any parent. > > if (!test_bit(d->hwirq, pctrl->skip_wake_irqs)) > msm_gpio_irq_unmask(d); Right. I'll fix it when I send the v3. Thanks! -Doug
