From b55326dc969ea2d704a008d9a97583b128f54f4f Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Thu, 16 Aug 2018 13:06:46 -0700 Subject: [PATCH 1/3] pinctrl: msm: Really mask level interrupts to prevent latching 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. Signed-off-by: Stephen Boyd Reviewed-by: Douglas Anderson Reviewed-by: Bjorn Andersson Signed-off-by: Linus Walleij --- drivers/pinctrl/qcom/pinctrl-msm.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 2155a30c282b..5d72ffad32c2 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -634,6 +634,29 @@ 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); + /* + * There are two bits that control interrupt forwarding to the CPU. The + * RAW_STATUS_EN bit causes the level or edge sensed on the line to be + * latched into the interrupt status register when the hardware detects + * an irq that it's configured for (either edge for edge type or level + * for level type irq). The 'non-raw' status enable bit causes the + * hardware to assert the summary interrupt to the CPU if the latched + * status bit is set. There's a bug though, 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 + * so we can't touch that bit for edge type irqs and we have to keep + * the bit set anyway so that edges are latched while the line is masked. + * + * To make matters more complicated, leaving the RAW_STATUS_EN bit + * enabled all the time causes level interrupts to re-latch into the + * status register because the level is still present on the line after + * we ack it. We clear the raw status enable bit during mask here and + * set the bit on unmask so the interrupt can't latch into the hardware + * while it's masked. + */ + if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) + val &= ~BIT(g->intr_raw_status_bit); + val &= ~BIT(g->intr_enable_bit); writel(val, pctrl->regs + g->intr_cfg_reg); @@ -655,6 +678,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); From 823dd71f58eb2133c24af85fad056a8dbb1a76e9 Mon Sep 17 00:00:00 2001 From: Paul Burton Date: Sat, 25 Aug 2018 10:53:28 -0700 Subject: [PATCH 2/3] pinctrl: ingenic: Fix group & function error checking Commit a203728ac6bb ("pinctrl: core: Return selector to the pinctrl driver") and commit f913cfce4ee4 ("pinctrl: pinmux: Return selector to the pinctrl driver") modified the return values of pinctrl_generic_add_group() and pinmux_generic_add_function() respectively, but did so without updating their callers. This broke the pinctrl-ingenic driver, which treats non-zero return values from these functions as errors & fails to probe. For example on a MIPS Ci20: pinctrl-ingenic 10010000.pin-controller: Failed to register group uart0-hwflow pinctrl-ingenic: probe of 10010000.pin-controller failed with error 1 Without the pinctrl driver probed, other drivers go on to fail to probe too & the system is unusable. Fix this by modifying the error checks to treat only negative values as errors, matching the commits that introduced the breakage & similar changes made to other drivers. Signed-off-by: Paul Burton Fixes: a203728ac6bb ("pinctrl: core: Return selector to the pinctrl driver") Fixes: f913cfce4ee4 ("pinctrl: pinmux: Return selector to the pinctrl driver") Cc: Linus Walleij Cc: Paul Cercueil Cc: Tony Lindgren Cc: linux-gpio@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Linus Walleij --- drivers/pinctrl/pinctrl-ingenic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c index 6a1b6058b991..628817c40e3b 100644 --- a/drivers/pinctrl/pinctrl-ingenic.c +++ b/drivers/pinctrl/pinctrl-ingenic.c @@ -793,7 +793,7 @@ static int ingenic_pinctrl_probe(struct platform_device *pdev) err = pinctrl_generic_add_group(jzpc->pctl, group->name, group->pins, group->num_pins, group->data); - if (err) { + if (err < 0) { dev_err(dev, "Failed to register group %s\n", group->name); return err; @@ -806,7 +806,7 @@ static int ingenic_pinctrl_probe(struct platform_device *pdev) err = pinmux_generic_add_function(jzpc->pctl, func->name, func->group_names, func->num_group_names, func->data); - if (err) { + if (err < 0) { dev_err(dev, "Failed to register function %s\n", func->name); return err; From 5bc5a671b1f4b3aa019264ce970d3683a9ffa761 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Tue, 28 Aug 2018 09:45:37 +0100 Subject: [PATCH 3/3] pinctrl: madera: Fix possible NULL pointer with pdata config If we are being configured via pdata we don't necessarily have any gpio mappings being configured that way so pdata->gpio_config could be NULL. Signed-off-by: Richard Fitzgerald Signed-off-by: Linus Walleij --- drivers/pinctrl/cirrus/pinctrl-madera-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pinctrl/cirrus/pinctrl-madera-core.c b/drivers/pinctrl/cirrus/pinctrl-madera-core.c index ece41fb2848f..c4f4d904e4a6 100644 --- a/drivers/pinctrl/cirrus/pinctrl-madera-core.c +++ b/drivers/pinctrl/cirrus/pinctrl-madera-core.c @@ -1040,7 +1040,7 @@ static int madera_pin_probe(struct platform_device *pdev) } /* if the configuration is provided through pdata, apply it */ - if (pdata) { + if (pdata && pdata->gpio_configs) { ret = pinctrl_register_mappings(pdata->gpio_configs, pdata->n_gpio_configs); if (ret) {