[PATCH v4 1/4] irqchip: qcom-pdc: Fix phantom irq when changing between rising/falling

[PATCH v4 1/4] irqchip: qcom-pdc: Fix phantom irq when changing between rising/falling

[Douglas Anderson]

We have a problem if we use gpio-keys and configure wakeups such that
we only want one edge to wake us up.  AKA:
  wakeup-event-action = <EV_ACT_DEASSERTED>;
  wakeup-source;

Specifically we end up with a phantom interrupt that blocks suspend if
the line was already high and we want wakeups on rising edges (AKA we
want the GPIO to go low and then high again before we wake up).  The
opposite is also problematic.

Specifically, here's what's happening today:
1. Normally, gpio-keys configures to look for both edges.  Due to the
   current workaround introduced in commit c3c0c2e18d94 ("pinctrl:
   qcom: Handle broken/missing PDC dual edge IRQs on sc7180"), if the
   line was high we'd configure for falling edges.
2. At suspend time, we change to look for rising edges.
3. After qcom_pdc_gic_set_type() runs, we get a phantom interrupt.

We can solve this by just clearing the phantom interrupt.

NOTE: it is possible that this could cause problems for a client with
very specific needs, but there's not much we can do with this
hardware.  As an example, let's say the interrupt signal is currently
high and the client is looking for falling edges.  The client now
changes to look for rising edges.  The client could possibly expect
that if the line has a short pulse low (and back high) that it would
always be detected.  Specifically no matter when the pulse happened,
it should either have tripped the (old) falling edge trigger or the
(new) rising edge trigger.  We will simply not trip it.  We could
narrow down the race a bit by polling our parent before changing
types, but no matter what we do there will still be a period of time
where we can't tell the difference between a real transition (or more
than one transition) and the phantom.

Fixes: f55c73aef890 ("irqchip/pdc: Add PDC interrupt controller for QCOM SoCs")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
Tested-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
There are no dependencies between this patch and patch #2/#3.  It can
go in by itself.  Patches are only grouped together in one series
because they address similar issues.

Maulik has got confirmation from hardware guys and understands the
problem.  This patch is ready to land.

Changes in v4:
- No changes, this patch on its own ready to land.

Changes in v3:
- Adjusted the comment as per Maulik.

Changes in v2:
- 0 => false
- If irq_chip_set_type_parent() fails don't bother clearing.
- Add Fixes tag.

 drivers/irqchip/qcom-pdc.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index bd39e9de6ecf..5dc63c20b67e 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -159,6 +159,8 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
 {
 	int pin_out = d->hwirq;
 	enum pdc_irq_config_bits pdc_type;
+	enum pdc_irq_config_bits old_pdc_type;
+	int ret;
 
 	if (pin_out == GPIO_NO_WAKE_IRQ)
 		return 0;
@@ -187,9 +189,26 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
 		return -EINVAL;
 	}
 
+	old_pdc_type = pdc_reg_read(IRQ_i_CFG, pin_out);
 	pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
 
-	return irq_chip_set_type_parent(d, type);
+	ret = irq_chip_set_type_parent(d, type);
+	if (ret)
+		return ret;
+
+	/*
+	 * When we change types the PDC can give a phantom interrupt.
+	 * Clear it.  Specifically the phantom shows up when reconfiguring
+	 * polarity of interrupt without changing the state of the signal
+	 * but let's be consistent and clear it always.
+	 *
+	 * Doing this works because we have IRQCHIP_SET_TYPE_MASKED so the
+	 * interrupt will be cleared before the rest of the system sees it.
+	 */
+	if (old_pdc_type != pdc_type)
+		irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
+
+	return 0;
 }
 
 static struct irq_chip qcom_pdc_gic_chip = {
-- 
2.29.2.576.ga3fc446d84-goog

[PATCH v4 2/4] pinctrl: qcom: Allow SoCs to specify a GPIO function that's not 0

[Douglas Anderson]

There's currently a comment in the code saying function 0 is GPIO.
Instead of hardcoding it, let's add a member where an SoC can specify
it.  No known SoCs use a number other than 0, but this just makes the
code clearer.  NOTE: no SoC code needs to be updated since we can rely
on zero-initialization.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---

(no changes since v1)

 drivers/pinctrl/qcom/pinctrl-msm.c | 4 ++--
 drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 77a25bdf0da7..588df91274e2 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -210,8 +210,8 @@ static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev,
 	if (!g->nfuncs)
 		return 0;
 
-	/* For now assume function 0 is GPIO because it always is */
-	return msm_pinmux_set_mux(pctldev, g->funcs[0], offset);
+	return msm_pinmux_set_mux(pctldev,
+				  g->funcs[pctrl->soc->gpio_func], offset);
 }
 
 static const struct pinmux_ops msm_pinmux_ops = {
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 333f99243c43..e31a5167c91e 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -118,6 +118,7 @@ struct msm_gpio_wakeirq_map {
  * @wakeirq_dual_edge_errata: If true then GPIOs using the wakeirq_map need
  *                            to be aware that their parent can't handle dual
  *                            edge interrupts.
+ * @gpio_func: Which function number is GPIO (usually 0).
  */
 struct msm_pinctrl_soc_data {
 	const struct pinctrl_pin_desc *pins;
@@ -134,6 +135,7 @@ struct msm_pinctrl_soc_data {
 	const struct msm_gpio_wakeirq_map *wakeirq_map;
 	unsigned int nwakeirq_map;
 	bool wakeirq_dual_edge_errata;
+	unsigned int gpio_func;
 };
 
 extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
-- 
2.29.2.576.ga3fc446d84-goog

[PATCH v4 3/4] pinctrl: qcom: Don't clear pending interrupts when enabling

[Douglas Anderson]

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@chromium.org>
---
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);
-	}
-
 	val = msm_readl_intr_cfg(pctrl, g);
 	val |= BIT(g->intr_raw_status_bit);
 	val |= BIT(g->intr_enable_bit);
@@ -822,7 +811,7 @@ static void msm_gpio_irq_enable(struct irq_data *d)
 		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);
 }
 
 static void msm_gpio_irq_disable(struct irq_data *d)
@@ -837,11 +826,6 @@ static void msm_gpio_irq_disable(struct irq_data *d)
 		msm_gpio_irq_mask(d);
 }
 
-static void msm_gpio_irq_unmask(struct irq_data *d)
-{
-	msm_gpio_irq_clear_unmask(d, false);
-}
-
 /**
  * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by parent.
  * @d: The irq dta.
@@ -936,6 +920,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 	const struct msm_pingroup *g;
 	unsigned long flags;
+	bool was_enabled;
 	u32 val;
 
 	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
@@ -997,6 +982,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	 * could cause the INTR_STATUS to be set for EDGE interrupts.
 	 */
 	val = msm_readl_intr_cfg(pctrl, g);
+	was_enabled = val & BIT(g->intr_raw_status_bit);
 	val |= BIT(g->intr_raw_status_bit);
 	if (g->intr_detection_width == 2) {
 		val &= ~(3 << g->intr_detection_bit);
@@ -1046,6 +1032,16 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	}
 	msm_writel_intr_cfg(val, pctrl, g);
 
+	/*
+	 * The first time we set RAW_STATUS_EN it could trigger an interrupt.
+	 * Clear it.  This is safe because we have IRQCHIP_SET_TYPE_MASKED.
+	 */
+	if (!was_enabled) {
+		val = msm_readl_intr_status(pctrl, g);
+		val &= ~BIT(g->intr_status_bit);
+		msm_writel_intr_status(val, pctrl, g);
+	}
+
 	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
 		msm_gpio_update_dual_edge_pos(pctrl, g, d);
 
-- 
2.29.2.576.ga3fc446d84-goog

[PATCH v4 4/4] pinctrl: qcom: Clear possible pending parent irq when remuxing GPIOs

[Douglas Anderson]

In commit 71266d9d3936 ("pinctrl: qcom: Move clearing pending IRQ to
.irq_request_resources callback") we tried to make it so that the
"enable" didn't clear pending interrupts for interrupts that were
handled by our parent (the PDC).  Unfortunately that regressed things.
After that patch we found that sc7180-trogdor based devices could no
longer enter suspend.

Specifically in sc7180-trogdor.dtsi we configure the uart3 to have two
pinctrl states, sleep and default, and mux between the two during
runtime PM and system suspend (see geni_se_resources_{on,off}() for
more details). The difference between the sleep and default state is
that the RX pin is muxed to a GPIO during sleep and muxed to the UART
otherwise.

As per Qualcomm, when we mux the pin over to the UART function the PDC
is still watching it / latching edges.  These edges don't cause
interrupts because the current code masks the interrupt unless we're
entering suspend.  However, as soon as we enter suspend we unmask the
interrupt and it's counted as a wakeup.

Let's deal with the problem like this:
* When we mux away, we'll mask our parent.  This isn't necessary in
  the above case since the parent already masked us, but it's a good
  idea in general.
* When we mux back will clear any interrupts and unmask our parent if
  needed.

Fixes: 71266d9d3936 ("pinctrl: qcom: Move clearing pending IRQ to .irq_request_resources callback")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This patch depends on #2/#3 in the series, but not #1.  #1 can land on
its own and then #2/#3/#4 can land together even without #1.  The only
reason patch #1 and #2/#3/#4 are together in one series is because
they address similar issues.

I have done most of this patch testing on the Chrome OS 5.4 kernel
tree (with many backports) but have sanity checked it on mainline.

This patch definitely needs more testing / discussion, so please don't
land without Qualcomm confirming that it looks OK in all the cases
they are aware of.

Changes in v4:
- Totally rewrote again with my new understanding of the world.
- Split non-PDC fix and PDC fix in two.

Changes in v3:
- Fixed bug in msm_gpio_direction_output() (s/oldval =/oldval = val =/)
- Add back "if !skip_wake_irqs" test in msm_gpio_irq_enable()
- For non-PDC, clear 1st interrupt in msm_gpio_irq_set_type()

Changes in v2:
- 0 => false
- If skip_wake_irqs, don't need to clear normal intr.
- Add comment about glitches in both output and input.

 drivers/pinctrl/qcom/pinctrl-msm.c | 42 +++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index f785646d1df7..37fa95c5805c 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -171,7 +171,12 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
 			      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);
+	struct irq_data *d = irq_get_irq_data(irq);
+	unsigned int gpio_func = pctrl->soc->gpio_func;
 	const struct msm_pingroup *g;
+	bool should_manage_parent;
 	unsigned long flags;
 	u32 val, mask;
 	int i;
@@ -187,6 +192,23 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	if (WARN_ON(i == g->nfuncs))
 		return -EINVAL;
 
+	/*
+	 * If an GPIO interrupt is setup on this pin and those interrupts are
+	 * handled by our parent we need special handling.  Specifically the
+	 * parent will still see the pin twiddle even when we're muxed away.
+	 *
+	 * If our GPIO was unmasked before muxing away from GPIO we need to
+	 * mask our parent before switching so it doesn't see the twiddling.
+	 *
+	 * When we switch back we might need to clear any interrupts that were
+	 * latched while were muxed away.
+	 */
+	should_manage_parent = d && d->parent_data &&
+			       test_bit(d->hwirq, pctrl->skip_wake_irqs);
+
+	if (i != gpio_func && should_manage_parent && !irqd_irq_masked(d))
+		irq_chip_mask_parent(d);
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = msm_readl_ctl(pctrl, g);
@@ -196,6 +218,13 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
+	if (i == gpio_func && should_manage_parent) {
+		irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
+
+		if (!irqd_irq_masked(d))
+			irq_chip_unmask_parent(d);
+	}
+
 	return 0;
 }
 
@@ -1093,19 +1122,6 @@ static int msm_gpio_irq_reqres(struct irq_data *d)
 		ret = -EINVAL;
 		goto out;
 	}
-
-	/*
-	 * Clear the interrupt that may be pending before we enable
-	 * the line.
-	 * This is especially a problem with the GPIOs routed to the
-	 * PDC. These GPIOs are direct-connect interrupts to the GIC.
-	 * Disabling the interrupt line at the PDC does not prevent
-	 * the interrupt from being latched at the GIC. The state at
-	 * GIC needs to be cleared before enabling.
-	 */
-	if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
-		irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
-
 	return 0;
 out:
 	module_put(gc->owner);
-- 
2.29.2.576.ga3fc446d84-goog

Re: (subset) [PATCH v4 1/4] irqchip: qcom-pdc: Fix phantom irq when changing between rising/falling

[Marc Zyngier]

On Fri, 11 Dec 2020 14:15:35 -0800, Douglas Anderson wrote:
> We have a problem if we use gpio-keys and configure wakeups such that
> we only want one edge to wake us up.  AKA:
>   wakeup-event-action = <EV_ACT_DEASSERTED>;
>   wakeup-source;
> 
> Specifically we end up with a phantom interrupt that blocks suspend if
> the line was already high and we want wakeups on rising edges (AKA we
> want the GPIO to go low and then high again before we wake up).  The
> opposite is also problematic.
> 
> [...]

Applied to irq/irqchip-next, thanks!

[1/4] irqchip: qcom-pdc: Fix phantom irq when changing between rising/falling
      commit: 2f5fbc4305d07725bfebaedb09e57271315691ef

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.

[irqchip: irq/irqchip-next] irqchip/qcom-pdc: Fix phantom irq when changing between rising/falling

[irqchip-bot for Douglas Anderson]

The following commit has been merged into the irq/irqchip-next branch of irqchip:

Commit-ID:     2f5fbc4305d07725bfebaedb09e57271315691ef
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/2f5fbc4305d07725bfebaedb09e57271315691ef
Author:        Douglas Anderson <dianders@chromium.org>
AuthorDate:    Fri, 11 Dec 2020 14:15:35 -08:00
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Sat, 12 Dec 2020 10:46:02 

irqchip/qcom-pdc: Fix phantom irq when changing between rising/falling

We have a problem if we use gpio-keys and configure wakeups such that
we only want one edge to wake us up.  AKA:
  wakeup-event-action = <EV_ACT_DEASSERTED>;
  wakeup-source;

Specifically we end up with a phantom interrupt that blocks suspend if
the line was already high and we want wakeups on rising edges (AKA we
want the GPIO to go low and then high again before we wake up).  The
opposite is also problematic.

Specifically, here's what's happening today:
1. Normally, gpio-keys configures to look for both edges.  Due to the
   current workaround introduced in commit c3c0c2e18d94 ("pinctrl:
   qcom: Handle broken/missing PDC dual edge IRQs on sc7180"), if the
   line was high we'd configure for falling edges.
2. At suspend time, we change to look for rising edges.
3. After qcom_pdc_gic_set_type() runs, we get a phantom interrupt.

We can solve this by just clearing the phantom interrupt.

NOTE: it is possible that this could cause problems for a client with
very specific needs, but there's not much we can do with this
hardware.  As an example, let's say the interrupt signal is currently
high and the client is looking for falling edges.  The client now
changes to look for rising edges.  The client could possibly expect
that if the line has a short pulse low (and back high) that it would
always be detected.  Specifically no matter when the pulse happened,
it should either have tripped the (old) falling edge trigger or the
(new) rising edge trigger.  We will simply not trip it.  We could
narrow down the race a bit by polling our parent before changing
types, but no matter what we do there will still be a period of time
where we can't tell the difference between a real transition (or more
than one transition) and the phantom.

Fixes: f55c73aef890 ("irqchip/pdc: Add PDC interrupt controller for QCOM SoCs")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Tested-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20201211141514.v4.1.I2702919afc253e2a451bebc3b701b462b2d22344@changeid
---
 drivers/irqchip/qcom-pdc.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index bd39e9d..5dc63c2 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -159,6 +159,8 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
 {
 	int pin_out = d->hwirq;
 	enum pdc_irq_config_bits pdc_type;
+	enum pdc_irq_config_bits old_pdc_type;
+	int ret;
 
 	if (pin_out == GPIO_NO_WAKE_IRQ)
 		return 0;
@@ -187,9 +189,26 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
 		return -EINVAL;
 	}
 
+	old_pdc_type = pdc_reg_read(IRQ_i_CFG, pin_out);
 	pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
 
-	return irq_chip_set_type_parent(d, type);
+	ret = irq_chip_set_type_parent(d, type);
+	if (ret)
+		return ret;
+
+	/*
+	 * When we change types the PDC can give a phantom interrupt.
+	 * Clear it.  Specifically the phantom shows up when reconfiguring
+	 * polarity of interrupt without changing the state of the signal
+	 * but let's be consistent and clear it always.
+	 *
+	 * Doing this works because we have IRQCHIP_SET_TYPE_MASKED so the
+	 * interrupt will be cleared before the rest of the system sees it.
+	 */
+	if (old_pdc_type != pdc_type)
+		irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
+
+	return 0;
 }
 
 static struct irq_chip qcom_pdc_gic_chip = {

Re: [PATCH v4 3/4] pinctrl: qcom: Don't clear pending interrupts when enabling

[Stephen Boyd]

Quoting Douglas Anderson (2020-12-11 14:15:37)
> 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@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

One comment clarification below.

> 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
> @@ -1046,6 +1032,16 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>         }
>         msm_writel_intr_cfg(val, pctrl, g);
>  
> +       /*
> +        * The first time we set RAW_STATUS_EN it could trigger an interrupt.
> +        * Clear it.  This is safe because we have IRQCHIP_SET_TYPE_MASKED.

Clear the interrupt? 'it' is ambiguous.

> +        */
> +       if (!was_enabled) {
> +               val = msm_readl_intr_status(pctrl, g);
> +               val &= ~BIT(g->intr_status_bit);
> +               msm_writel_intr_status(val, pctrl, g);
> +       }
> +
>         if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
>                 msm_gpio_update_dual_edge_pos(pctrl, g, d);
>

Re: [PATCH v4 3/4] pinctrl: qcom: Don't clear pending interrupts when enabling

[Rajendra Nayak]

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@chromium.org>
> ---
> 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.

I currently don;t have access to any non-PDC hardware, so could not really do
any real tests, but the changes seem sane, so

Reviewed-by: Rajendra Nayak <rnayak@codeaurora.org>

> 
> 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);
> -	}
> -
>   	val = msm_readl_intr_cfg(pctrl, g);
>   	val |= BIT(g->intr_raw_status_bit);
>   	val |= BIT(g->intr_enable_bit);
> @@ -822,7 +811,7 @@ static void msm_gpio_irq_enable(struct irq_data *d)
>   		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);
>   }
>   
>   static void msm_gpio_irq_disable(struct irq_data *d)
> @@ -837,11 +826,6 @@ static void msm_gpio_irq_disable(struct irq_data *d)
>   		msm_gpio_irq_mask(d);
>   }
>   
> -static void msm_gpio_irq_unmask(struct irq_data *d)
> -{
> -	msm_gpio_irq_clear_unmask(d, false);
> -}
> -
>   /**
>    * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by parent.
>    * @d: The irq dta.
> @@ -936,6 +920,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>   	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>   	const struct msm_pingroup *g;
>   	unsigned long flags;
> +	bool was_enabled;
>   	u32 val;
>   
>   	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
> @@ -997,6 +982,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>   	 * could cause the INTR_STATUS to be set for EDGE interrupts.
>   	 */
>   	val = msm_readl_intr_cfg(pctrl, g);
> +	was_enabled = val & BIT(g->intr_raw_status_bit);
>   	val |= BIT(g->intr_raw_status_bit);
>   	if (g->intr_detection_width == 2) {
>   		val &= ~(3 << g->intr_detection_bit);
> @@ -1046,6 +1032,16 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>   	}
>   	msm_writel_intr_cfg(val, pctrl, g);
>   
> +	/*
> +	 * The first time we set RAW_STATUS_EN it could trigger an interrupt.
> +	 * Clear it.  This is safe because we have IRQCHIP_SET_TYPE_MASKED.
> +	 */
> +	if (!was_enabled) {
> +		val = msm_readl_intr_status(pctrl, g);
> +		val &= ~BIT(g->intr_status_bit);
> +		msm_writel_intr_status(val, pctrl, g);
> +	}
> +
>   	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
>   		msm_gpio_update_dual_edge_pos(pctrl, g, d);
>   
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v4 3/4] pinctrl: qcom: Don't clear pending interrupts when enabling

[Maulik Shah]

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@chromium.org>
> ---
> 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.

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.

Thanks,
Maulik

>   	val = msm_readl_intr_cfg(pctrl, g);
>   	val |= BIT(g->intr_raw_status_bit);
>   	val |= BIT(g->intr_enable_bit);
> @@ -822,7 +811,7 @@ static void msm_gpio_irq_enable(struct irq_data *d)
>   		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);
>   }
>   
>   static void msm_gpio_irq_disable(struct irq_data *d)
> @@ -837,11 +826,6 @@ static void msm_gpio_irq_disable(struct irq_data *d)
>   		msm_gpio_irq_mask(d);
>   }
>   
> -static void msm_gpio_irq_unmask(struct irq_data *d)
> -{
> -	msm_gpio_irq_clear_unmask(d, false);
> -}
> -
>   /**
>    * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by parent.
>    * @d: The irq dta.
> @@ -936,6 +920,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>   	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>   	const struct msm_pingroup *g;
>   	unsigned long flags;
> +	bool was_enabled;
>   	u32 val;
>   
>   	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
> @@ -997,6 +982,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>   	 * could cause the INTR_STATUS to be set for EDGE interrupts.
>   	 */
>   	val = msm_readl_intr_cfg(pctrl, g);
> +	was_enabled = val & BIT(g->intr_raw_status_bit);
>   	val |= BIT(g->intr_raw_status_bit);
>   	if (g->intr_detection_width == 2) {
>   		val &= ~(3 << g->intr_detection_bit);
> @@ -1046,6 +1032,16 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>   	}
>   	msm_writel_intr_cfg(val, pctrl, g);
>   
> +	/*
> +	 * The first time we set RAW_STATUS_EN it could trigger an interrupt.
> +	 * Clear it.  This is safe because we have IRQCHIP_SET_TYPE_MASKED.
> +	 */
> +	if (!was_enabled) {
> +		val = msm_readl_intr_status(pctrl, g);
> +		val &= ~BIT(g->intr_status_bit);
> +		msm_writel_intr_status(val, pctrl, g);
> +	}
> +
>   	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
>   		msm_gpio_update_dual_edge_pos(pctrl, g, d);
>   

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v4 3/4] pinctrl: qcom: Don't clear pending interrupts when enabling

[Doug Anderson]

Hi,

On Mon, Dec 21, 2020 at 8:01 AM Maulik Shah <mkshah@codeaurora.org> 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@chromium.org>
> > ---
> > 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