Messages in this thread | | | Date | Tue, 24 Nov 2020 13:06:11 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs |
| |
On 2020-11-24 12:43, Maulik Shah wrote: > Hi Marc, > > On 11/24/2020 4:45 PM, Marc Zyngier wrote: >> On 2020-11-24 10:37, Maulik Shah wrote: >> >> [...] >> >>>> static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, >>>> unsigned function, >>>> 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); >>>> const struct msm_pingroup *g; >>>> unsigned long flags; >>>> u32 val, mask; >>>> + u32 oldval; >>>> + u32 old_i; >>>> int i; >>>> g = &pctrl->soc->groups[group]; >>>> @@ -187,15 +215,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); >>> >>> disable_irq() and enable_irq() should be moved inside this if loop. >>> as >>> only use for this is to mask the IRQ when switching back to gpio IRQ >>> mode? >>> >>> i also don't think we should leave IRQ enabled at the end of this >>> function by default, probably need to check if IRQ was already >>> unmasked before disabling it, then only call enable_irq(). >> >> Why? It looks to me that this reproduces the behaviour of >> IRQCHIP_SET_TYPE_MASKED, which is highly desirable. What >> problem are you trying to address with this? > > Correct, here trying to reproduce the behaviour of > IRQCHIP_SET_TYPE_MASKED which i guess is ok once its moved inside if > loop as this is the place its switching to IRQ mode. > > but there is a problem to leave it enabled at the end of set_direction > callbacks, see below. > >> >>> >>>> + >>>> + enable_irq(irq); >>>> + >>>> return 0; >>>> } >>>> @@ -456,32 +495,45 @@ 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); >>>> + if (oldval != val) >>>> + msm_pinctrl_clear_pending_irq(pctrl, offset, irq); >>>> + >>>> + enable_irq(irq); >>> >>> i do not think we need disable_irq() and enable_irq() here, changing >>> direction to input does not mean its being used for interrupt only, >>> it >>> may be set to use something like Rx mode in UART. >>> >>> the client driver should enable IRQ when needed. >> >> And the kernel doesn't expect random interrupts to fire. Again, what >> are you trying to fix by removing these? > > I see leaving IRQ enabled here can cause problems. For example in > qcom_geni_serial.c driver before requesting IRQ, it sets the > IRQ_NOAUTOEN flag to not keep it enabled. > > see the below snippet > irq_set_status_flags(uport->irq, IRQ_NOAUTOEN); > ret = devm_request_irq(uport->dev, uport->irq, > qcom_geni_serial_isr, > IRQF_TRIGGER_HIGH, port->name, uport); > > later when this devm_request_irq() invokes .irq_request_resources > callback it will reach msm_gpio_irq_reqres() from > where msm_gpio_direction_input() is called which leaves the irq > enabled at the end with enable_irq() which was not expected by driver.
No it doesn't. disable_irq()/enable_irq() are designed to nest. If the interrupt line was disabled before the disable/enable sequence, it will still be disabled after.
> It will cause is IRQ storm since the UART geni driver uses GPIO in Rx > mode when out of suspend. The IRQ mode in GPIO is enabled > with suspend entry only. During resume the IRQ will again be disabled > and GPIO will be switched to Rx mode.
I don't see how this contradicts what is above. If the interrupt was disabled before hitting this sequence, it will still be disabled after. Am I missing something? Have you actually seen the problem on HW?
M. -- Jazz is not dead. It just smells funny...
| |