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...
