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

[PATCH v2 1/3] 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>
---
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.

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..f91e7d5aea25 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 if a line is already
+	 * high and we change to rising or if a line is already low and we
+	 * change to falling 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.454.gaff20da3a2-goog

[PATCH v2 2/3] 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>
---

(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.454.gaff20da3a2-goog

[PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs

[Douglas Anderson]

Conceptually, we can envision the input on Qualcomm SoCs to pass
through a bunch of blocks between coming into the chip and becoming a
GPIO interrupt.  From guessing and running a handful of tests, I
believe that we can represent the state of the world with a drawing
that looks something like this:

 +-----------------+       +-----------------+       +-----------------+
 |      INPUT      |  -->  |      PINMUX     |       |    IS_INPUT     |
 +-----------------+       |                 |  -->  |                 |
                           | output bogus (?)|       | output bogus (?)|
                           | if not muxed    |       | if input disab. |
                           +-----------------+       +-----------------+
                                                              |
          +---------------------------------------------------+--> to PDC
          |
          V
 +-----------------+       +-----------------+       +-----------------+
 | INTR RAW ENABLE |       | DETECTION LOGIC |       | STATUS REGISTER |
 |                 |       |                 |       |                 |
 | output bogus (?)|  -->  | maybe handles   |       | latches inputs  |
 | if disabled     |       | polarity diffs  |  -->  | that are high   |
 +-----------------+       |                 |       |                 |
                           | maybe debounces |       | write 1 to clr  |
                           | level irqs      |       +-----------------+
                           +-----------------+                |
                                                              |
          +---------------------------------------------------+
          |
          V
 +-----------------+
 |      ENABLE     |
 |                 |       +-----------------+
 | nothing passes  |  -->  |   SUMMARY IRQ   |
 | through if      |       +-----------------+
 | disabled        |
 +-----------------+

The above might not be 100% exact, but for the purpose of this
discussion, the point is that there are a whole bunch of gates and
transformations on the input before it gets to the circuitry that
generates interrupts.

As you might guess, if you reconfigure one of the gates in the above
diagram while the system is configured to detect interrupts things get
a little wacky.  Specifically it appears that if you gate the input at
any step it can cause various glitches in the later steps because they
are still paying attention to their input but their input isn't really
sane anymore.

I did some poking and I found that I could generate bogus interrupts
in the system both when muxing away from GPIO mode and also when
muxing back to GPIO mode.  When configured to use the PDC path for
generating interrupts I found that if the external input on the GPIO
was low that I'd get what looked like a rising edge when unmuxing and
a falling edge when muxing.  When configured away from the PDC path I
got slightly different glitch interrupts when changing muxing.

These glitches when remuxing matter in reality, not just in theory.
To be concrete, let's take the special "wakeup_irq" in
qcom_geni_serial.c as an example.  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.  When we switch
between these two states we can generate the glitches talked about
above because we're configured to look for edges but the transition
from the gated input (which is bogus) to the real input can look like
an edge.

Historically the UART case above was handled by the fact that the
"enable" function in the MSM GPIO driver did an "unmask and clear".
This relied on the fact that the system happens to have the interrupt
disabled until suspend time and that it would enable it after the
pinmux change happened, thus clearing the interrupt.

The historical solution, however, had a few problems.  The first
problem (that nobody seemed to have tripped) is that we can still get
bogus interrupts if we remux when the interrupt isn't disabled during
the muxing and re-enabled after.  The second problem is that it
violates how I believe that the interrupt enable path is supposed to
work.

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
didn't work for two reasons:
* It only tried to address the problem for interrupts that had parents
  (like the PDC).
* It regressed the problem that the original clearing was trying to
  solve.

I think we can safely assume that if someone muxes a pin to be
something other than a GPIO and then muxes it back that we can clear
any interrupts that were pending on it without violating any
assumptions that client drivers are making.  Presumably the client
drivers are intentionally remuxing the pin away from a dedicated
purpose to be a plain GPIO so they don't care what the pin state was
before the mux switch and they don't expect to see the pin change
level during this switch.  Let's move the clearing of the IRQ to the
pin muxing routine so that we'll clear a pending IRQ if we're muxing
from some non-GPIO mode to a GPIO mode.

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 in the series, but not #1.  #1 can land on
its own and then #2/#3 can land together even without #1.  The only
reason patch #1 and #2/#3 are together in one series is because they
address similar issues.

IMPORTANT NOTE: Maulik has requested to wait for his Ack on patch #3
before landing [1].  As per above, this need not stop patch #1 from
landing.

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.

[1] https://lore.kernel.org/r/603c691f-3614-d87b-075a-0889e9ffc453@codeaurora.org

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 | 114 +++++++++++++++++++----------
 1 file changed, 74 insertions(+), 40 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 588df91274e2..89e9579d076d 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -166,14 +166,44 @@ static int msm_get_function_groups(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static void msm_pinctrl_clear_pending_irq(struct msm_pinctrl *pctrl,
+					  unsigned int group,
+					  unsigned int irq)
+{
+	struct irq_data *d = irq_get_irq_data(irq);
+	const struct msm_pingroup *g;
+	unsigned long flags;
+	u32 val;
+
+	if (!d)
+		return;
+
+	if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs)) {
+		irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
+		return;
+	}
+
+	g = &pctrl->soc->groups[group];
+
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	val = msm_readl_intr_status(pctrl, g);
+	val &= ~BIT(g->intr_status_bit);
+	msm_writel_intr_status(val, pctrl, g);
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+}
+
 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 +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);
+
+	enable_irq(irq);
+
 	return 0;
 }
 
@@ -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);
+
+	enable_irq(irq);
+
 	return 0;
 }
 
 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);
 	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);
+
+	enable_irq(irq);
+
 	return 0;
 }
 
@@ -774,7 +841,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 +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);
-	}
-
 	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);
 }
 
 static void msm_gpio_irq_disable(struct irq_data *d)
@@ -837,11 +889,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.
@@ -1097,19 +1144,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.454.gaff20da3a2-goog

Re: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs

[Maulik Shah]

Hi Doug,

On 11/24/2020 11:17 PM, Douglas Anderson wrote:
> Conceptually, we can envision the input on Qualcomm SoCs to pass
> through a bunch of blocks between coming into the chip and becoming a
> GPIO interrupt.  From guessing and running a handful of tests, I
> believe that we can represent the state of the world with a drawing
> that looks something like this:
>
>   +-----------------+       +-----------------+       +-----------------+
>   |      INPUT      |  -->  |      PINMUX     |       |    IS_INPUT     |
>   +-----------------+       |                 |  -->  |                 |
>                             | output bogus (?)|       | output bogus (?)|
>                             | if not muxed    |       | if input disab. |
>                             +-----------------+       +-----------------+
>                                                                |
>            +---------------------------------------------------+--> to PDC
>            |
>            V
>   +-----------------+       +-----------------+       +-----------------+
>   | INTR RAW ENABLE |       | DETECTION LOGIC |       | STATUS REGISTER |
>   |                 |       |                 |       |                 |
>   | output bogus (?)|  -->  | maybe handles   |       | latches inputs  |
>   | if disabled     |       | polarity diffs  |  -->  | that are high   |
>   +-----------------+       |                 |       |                 |
>                             | maybe debounces |       | write 1 to clr  |
>                             | level irqs      |       +-----------------+
>                             +-----------------+                |
>                                                                |
>            +---------------------------------------------------+
>            |
>            V
>   +-----------------+
>   |      ENABLE     |
>   |                 |       +-----------------+
>   | nothing passes  |  -->  |   SUMMARY IRQ   |
>   | through if      |       +-----------------+
>   | disabled        |
>   +-----------------+
>
> The above might not be 100% exact, but for the purpose of this
> discussion, the point is that there are a whole bunch of gates and
> transformations on the input before it gets to the circuitry that
> generates interrupts.
>
> As you might guess, if you reconfigure one of the gates in the above
> diagram while the system is configured to detect interrupts things get
> a little wacky.  Specifically it appears that if you gate the input at
> any step it can cause various glitches in the later steps because they
> are still paying attention to their input but their input isn't really
> sane anymore.
>
> I did some poking and I found that I could generate bogus interrupts
> in the system both when muxing away from GPIO mode and also when
> muxing back to GPIO mode.  When configured to use the PDC path for
> generating interrupts I found that if the external input on the GPIO
> was low that I'd get what looked like a rising edge when unmuxing and
> a falling edge when muxing.  When configured away from the PDC path I
> got slightly different glitch interrupts when changing muxing.
>
> These glitches when remuxing matter in reality, not just in theory.
> To be concrete, let's take the special "wakeup_irq" in
> qcom_geni_serial.c as an example.  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.  When we switch
> between these two states we can generate the glitches talked about
> above because we're configured to look for edges but the transition
> from the gated input (which is bogus) to the real input can look like
> an edge.
>
> Historically the UART case above was handled by the fact that the
> "enable" function in the MSM GPIO driver did an "unmask and clear".
> This relied on the fact that the system happens to have the interrupt
> disabled until suspend time and that it would enable it after the
> pinmux change happened, thus clearing the interrupt.
>
> The historical solution, however, had a few problems.  The first
> problem (that nobody seemed to have tripped) is that we can still get
> bogus interrupts if we remux when the interrupt isn't disabled during
> the muxing and re-enabled after.  The second problem is that it
> violates how I believe that the interrupt enable path is supposed to
> work.
>
> 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
> didn't work for two reasons:
> * It only tried to address the problem for interrupts that had parents
>    (like the PDC).
> * It regressed the problem that the original clearing was trying to
>    solve.
>
> I think we can safely assume that if someone muxes a pin to be
> something other than a GPIO and then muxes it back that we can clear
> any interrupts that were pending on it without violating any
> assumptions that client drivers are making.  Presumably the client
> drivers are intentionally remuxing the pin away from a dedicated
> purpose to be a plain GPIO so they don't care what the pin state was
> before the mux switch and they don't expect to see the pin change
> level during this switch.  Let's move the clearing of the IRQ to the
> pin muxing routine so that we'll clear a pending IRQ if we're muxing
> from some non-GPIO mode to a GPIO mode.
>
> 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 in the series, but not #1.  #1 can land on
> its own and then #2/#3 can land together even without #1.  The only
> reason patch #1 and #2/#3 are together in one series is because they
> address similar issues.
>
> IMPORTANT NOTE: Maulik has requested to wait for his Ack on patch #3
> before landing [1].  As per above, this need not stop patch #1 from
> landing.
>
> 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.
>
> [1] https://lore.kernel.org/r/603c691f-3614-d87b-075a-0889e9ffc453@codeaurora.org
Please wait to land [1] before i confirm with HW team if this is indeed 
valid case.
>
> 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 | 114 +++++++++++++++++++----------
>   1 file changed, 74 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 588df91274e2..89e9579d076d 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -166,14 +166,44 @@ static int msm_get_function_groups(struct pinctrl_dev *pctldev,
>   	return 0;
>   }
>   
> +static void msm_pinctrl_clear_pending_irq(struct msm_pinctrl *pctrl,
> +					  unsigned int group,
> +					  unsigned int irq)
> +{
> +	struct irq_data *d = irq_get_irq_data(irq);
> +	const struct msm_pingroup *g;
> +	unsigned long flags;
> +	u32 val;
> +
> +	if (!d)
> +		return;
> +
> +	if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs)) {
> +		irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
> +		return;
> +	}
> +
> +	g = &pctrl->soc->groups[group];
> +
> +	raw_spin_lock_irqsave(&pctrl->lock, flags);
> +	val = msm_readl_intr_status(pctrl, g);
> +	val &= ~BIT(g->intr_status_bit);
> +	msm_writel_intr_status(val, pctrl, g);
> +	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
>   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 +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) {

even better if you can clear this unconditionally.

> +	enable_irq(irq);
> +
>   	return 0;
>   }
>   
> @@ -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.
> +
> +	enable_irq(irq);
> +
>   	return 0;
>   }
>   
>   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.

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

> +
> +	enable_irq(irq);
> +
>   	return 0;
>   }
>   
> @@ -774,7 +841,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 +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.

>   	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);

Thanks,
Maulik

>   }
>   
>   static void msm_gpio_irq_disable(struct irq_data *d)
> @@ -837,11 +889,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.
> @@ -1097,19 +1144,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);

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

Re: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs

[Doug Anderson]

Hi,

On Mon, Nov 30, 2020 at 2:33 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> > [1] https://lore.kernel.org/r/603c691f-3614-d87b-075a-0889e9ffc453@codeaurora.org
> 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

Re: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs

[Maulik Shah]

Hi Doug,

On 12/1/2020 3:14 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Nov 30, 2020 at 2:33 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>>> [1] https://lore.kernel.org/r/603c691f-3614-d87b-075a-0889e9ffc453@codeaurora.org
>> 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?
Yes
>
> 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.

Probably i was not clear, the phantom irq should be cleared when 
switching gpio to gpio IRQ mode.

When GPIO was used as Rx line in example QUP/UART use case, it can latch 
the phantom IRQ but as long as its IRQ is in disabled/masked state it 
doesn't matter.

its only when the GPIO is again set to IRQ mode with set_mux callback, 
the phantom IRQ needs clear to start as clean.

So we should check only for if (i == pctrl->soc->gpio_func) then clear 
phantom IRQ.

The same is case with .direction_output callback, when GPIO is used as 
output say as clock, need not clear any phantom IRQ,

The reason is with every pulse of clock it can latch as pending IRQ in 
GIC_ISPEND as long as it stay as output mode/clock.

its only when switching back GPIO from output direction to input & IRQ 
function, need to clear the phantom IRQ.

so we do not require clear phantom irq in .direction_output callback.

>
>
>>> @@ -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().

As the clearing phantom irq code in msm_gpio_irq_enable() is moved to 
separate function msm_pinctrl_clear_pending_irq(), it needs invoke from 
at the end of msm_gpio_irq_set_type() too.

Thanks,
Maulik
>
>
>>>        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

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

Re: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs

[Doug Anderson]

Hi,

On Thu, Dec 3, 2020 at 3:22 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> >>> +     /*
> >>> +      * 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?
> Yes

Yes means that you tried switching away from GPIO mode and you
couldn't get a phantom interrupt?  OK, I'll re-test then.

I'll test on the Chrome OS kernel tree since that's easiest for me,
but I can test on mainline if you think it would make a difference...

1. Pick <https://crrev.com/c/2556012> and put that kernel on the device.

2. In Cr50 console, make the WP line low with:
  wp enable

3. In AP console do:
  echo bogus > /sys/module/gpio_keys/parameters/doug_test

4. See bogus interrupt:

localhost ~ # echo bogus > /sys/module/gpio_keys/parameters/doug_test
[   62.006346] DOUG: selecting state bogus
[   62.011813] DOUG: ret 0
[   62.011875] DOUG: in dual edge parent: hwirq=66, type=1
[   62.020300] DOUG: gpio_keys_gpio_isr

Can you try replicating again?


> > 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.
>
> Probably i was not clear, the phantom irq should be cleared when
> switching gpio to gpio IRQ mode.
>
> When GPIO was used as Rx line in example QUP/UART use case, it can latch
> the phantom IRQ

This is where I disagree with you.  I don't think the interrupt is
latching while it's used as an Rx line.  I think it's the pinmux
change that introduces an phantom interrupt.

Specifically, with the same test patch above, AKA
<https://crrev.com/c/2556012>, I can do this:

1. On AP:
  echo bogus > /sys/module/gpio_keys/parameters/doug_test

2. On Cr50 console:
  wp disable
  wp enable
  wp disable
  wp enable
  wp disable
  wp enable

3. Go back and check the AP and see that no interrupts fired.

Said another way: when we're muxed away the interrupts aren't getting
latched.  It's the act of changing the mux that causes the phantom
interrupts.


> but as long as its IRQ is in disabled/masked state it
> doesn't matter.

...but there's no requirement that someone would need to disable/mask
an interrupt while switching the muxing, is there?  So it does matter.


> its only when the GPIO is again set to IRQ mode with set_mux callback,
> the phantom IRQ needs clear to start as clean.
>
> So we should check only for if (i == pctrl->soc->gpio_func) then clear
> phantom IRQ.
>
> The same is case with .direction_output callback, when GPIO is used as
> output say as clock, need not clear any phantom IRQ,
>
> The reason is with every pulse of clock it can latch as pending IRQ in
> GIC_ISPEND as long as it stay as output mode/clock.
>
> its only when switching back GPIO from output direction to input & IRQ
> function, need to clear the phantom IRQ.
>
> so we do not require clear phantom irq in .direction_output callback.

I think all the above explanation is with the model that the interrupt
detection logic is still happening even when muxed away.  I don't
believe that's true.  Please run my test patch or code up something
similar yourself.


> >> 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().
>
> As the clearing phantom irq code in msm_gpio_irq_enable() is moved to
> separate function msm_pinctrl_clear_pending_irq(), it needs invoke from
> at the end of msm_gpio_irq_set_type() too.

Seems reasonable to me.  I'll include this in my next spin.  Still
waiting for us to agree on some of the points above before spinning,
though.

-Doug

Re: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs

[Maulik Shah]

Hi Doug,

On 12/4/2020 2:34 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Dec 3, 2020 at 3:22 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>>>>> +     /*
>>>>> +      * 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?
>> Yes
> Yes means that you tried switching away from GPIO mode and you
> couldn't get a phantom interrupt?  OK, I'll re-test then.
>
> I'll test on the Chrome OS kernel tree since that's easiest for me,
> but I can test on mainline if you think it would make a difference...
>
> 1. Pick <https://crrev.com/c/2556012> and put that kernel on the device.
>
> 2. In Cr50 console, make the WP line low with:
>    wp enable
>
> 3. In AP console do:
>    echo bogus > /sys/module/gpio_keys/parameters/doug_test
>
> 4. See bogus interrupt:
>
> localhost ~ # echo bogus > /sys/module/gpio_keys/parameters/doug_test
> [   62.006346] DOUG: selecting state bogus
> [   62.011813] DOUG: ret 0
> [   62.011875] DOUG: in dual edge parent: hwirq=66, type=1
> [   62.020300] DOUG: gpio_keys_gpio_isr
>
> Can you try replicating again?
>
>
>>> 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.
>> Probably i was not clear, the phantom irq should be cleared when
>> switching gpio to gpio IRQ mode.
>>
>> When GPIO was used as Rx line in example QUP/UART use case, it can latch
>> the phantom IRQ
> This is where I disagree with you.  I don't think the interrupt is
> latching while it's used as an Rx line.  I think it's the pinmux
> change that introduces an phantom interrupt.
>
> Specifically, with the same test patch above, AKA
> <https://crrev.com/c/2556012>, I can do this:
>
> 1. On AP:
>    echo bogus > /sys/module/gpio_keys/parameters/doug_test
>
> 2. On Cr50 console:
>    wp disable
>    wp enable
>    wp disable
>    wp enable
>    wp disable
>    wp enable
>
> 3. Go back and check the AP and see that no interrupts fired.
>
> Said another way: when we're muxed away the interrupts aren't getting
> latched.  It's the act of changing the mux that causes the phantom
> interrupts.
>
>
>> but as long as its IRQ is in disabled/masked state it
>> doesn't matter.
> ...but there's no requirement that someone would need to disable/mask
> an interrupt while switching the muxing, is there?  So it does matter.
>
>
>> its only when the GPIO is again set to IRQ mode with set_mux callback,
>> the phantom IRQ needs clear to start as clean.
>>
>> So we should check only for if (i == pctrl->soc->gpio_func) then clear
>> phantom IRQ.
>>
>> The same is case with .direction_output callback, when GPIO is used as
>> output say as clock, need not clear any phantom IRQ,
>>
>> The reason is with every pulse of clock it can latch as pending IRQ in
>> GIC_ISPEND as long as it stay as output mode/clock.
>>
>> its only when switching back GPIO from output direction to input & IRQ
>> function, need to clear the phantom IRQ.
>>
>> so we do not require clear phantom irq in .direction_output callback.
> I think all the above explanation is with the model that the interrupt
> detection logic is still happening even when muxed away.  I don't
> believe that's true.
Its not the interrupt detection logic that is still happening when muxed 
away, but the GPIO line is routed to GIC from PDC.
The GPIO line get forwarded when the system is active/out of system 
level low power mode to GIC irrespective of whether GPIO is used as 
interrupt or not.
Due to this it can still latch the IRQ at GIC after switching to lets 
say Rx mode, whenever the line has any data recive, the line state 
toggles can be latched as error interrupt at GIC.

As the interrupt is in disabled state it won't be sent to CPU.
Its only when the driver chooses to switch back to interrupt mode we 
want to clear the error interrupt latched to start as clean. same is the 
case when used as output direction.

Hope above is clear.

Thanks,
Maulik
> Please run my test patch or code up something
> similar yourself.
>
>
>>>> 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().
>> As the clearing phantom irq code in msm_gpio_irq_enable() is moved to
>> separate function msm_pinctrl_clear_pending_irq(), it needs invoke from
>> at the end of msm_gpio_irq_set_type() too.
> Seems reasonable to me.  I'll include this in my next spin.  Still
> waiting for us to agree on some of the points above before spinning,
> though.
>
> -Doug

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

Re: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs

[Doug Anderson]

Hi,

On Tue, Dec 8, 2020 at 9:54 PM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> >> but as long as its IRQ is in disabled/masked state it
> >> doesn't matter.
> > ...but there's no requirement that someone would need to disable/mask
> > an interrupt while switching the muxing, is there?  So it does matter.
> >
> >
> >> its only when the GPIO is again set to IRQ mode with set_mux callback,
> >> the phantom IRQ needs clear to start as clean.
> >>
> >> So we should check only for if (i == pctrl->soc->gpio_func) then clear
> >> phantom IRQ.
> >>
> >> The same is case with .direction_output callback, when GPIO is used as
> >> output say as clock, need not clear any phantom IRQ,
> >>
> >> The reason is with every pulse of clock it can latch as pending IRQ in
> >> GIC_ISPEND as long as it stay as output mode/clock.
> >>
> >> its only when switching back GPIO from output direction to input & IRQ
> >> function, need to clear the phantom IRQ.
> >>
> >> so we do not require clear phantom irq in .direction_output callback.
> > I think all the above explanation is with the model that the interrupt
> > detection logic is still happening even when muxed away.  I don't
> > believe that's true.
> Its not the interrupt detection logic that is still happening when muxed
> away, but the GPIO line is routed to GIC from PDC.
> The GPIO line get forwarded when the system is active/out of system
> level low power mode to GIC irrespective of whether GPIO is used as
> interrupt or not.
> Due to this it can still latch the IRQ at GIC after switching to lets
> say Rx mode, whenever the line has any data recive, the line state
> toggles can be latched as error interrupt at GIC.

From my tests, though, I strongly believe that the pin is only visible
to the PDC if it's muxed as GPIO.  Specifically, in my tests I did
this (with all my patches applied so there were no phantom interrupts
when remuxing):

a) Muxed the pin away from GPIO to special function, but _didn't_ mask
the interrupt.

b) Toggled the line a whole bunch.  These caused no interrupts at all.

c) Muxed back to GPIO.

To me this is quite strong evidence that the muxing is "earlier" in
the path than the connection to the PDC.  In other words, if you
change the mux away from GPIO then the PDC stops seeing it and thus
the GIC also stops seeing it.  The GIC can't latch what it can't see.
This means while you're in "Rx mode" it can't be latched.


OK, so just in case this somehow only happens in S3, I also tried this
(with my patch from https://crrev.com/c/2556012):

a) Muxed away from GPIO ("bogus" pinmux)

b) Enter S3.

c) Toggle the GPIO a whole bunch ("wp enable / wp disable" on Cr50).

d) Wake from S3.

e) Check to see if the interrupt fired a bunch.  It didn't fire at all


In my test code the interrupt is not masked, only muxed away.  That
means that if, somehow, the PDC was still observing it then we'd see
the interrupt fire.  We don't.


Unless I messed up in my tests (always possible, though by this point
I've run them a number of times) then it still feels like your mental
model is wrong, or it's always possible I'm still misunderstanding
your model.  Regardless, rather than trying to re-explain your model
can you please confirm that you've written test code to confirm your
mental model?  If so, can you please provide this test code?  I've
provided several test patches proving out my mental model.

> As the interrupt is in disabled state it won't be sent to CPU.
> Its only when the driver chooses to switch back to interrupt mode we
> want to clear the error interrupt latched to start as clean. same is the
> case when used as output direction.
>
> Hope above is clear.

Unfortunately, it's still not.  :(  Can I convince you to provide a
test patch and a set of steps that will demonstrate the problem you're
worried about?  Specifically:

a) Maybe you're talking about the initial switch from a plain GPIO
input to making it an interrupt for the first time?  Are you worried
about a phantom interrupt in this case?  After patch #1 I think we're
safe because pdc_gic_set_type() will always clear the interrupt,
right?


b) You say "switch back to interrupt mode".  Are you imagining that a
driver does something like this:

request_irq();
...
free_irq();
...
request_irq();

If you're worried about that then we can implement irq_shutdown() for
PDC and then make sure we clear on the first enable after a shutdown,
I guess?


c) Maybe when you say "switch back to interrupt mode" you mean
something else?  If you are talking about muxing away and then muxing
back then I think we already have this covered.  If you are talking
about masking/unmasking then the whole point is that we _do_ want
interrupts latched while masked, right?


OK, I'm going to send out a v3 just to get the already-identified
problems fixed and also to allow landing of patch #1 in the series,
which I think is all agreed upon.  My request to you is that if you
think my code misses a specific case to provide some test patches to
demonstrate that case.


-Doug

Re: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs

[Maulik Shah]

Hi Doug,

On 12/10/2020 6:13 AM, Doug Anderson wrote:
> Hi,
>
> On Tue, Dec 8, 2020 at 9:54 PM Maulik Shah <mkshah@codeaurora.org> wrote:
>>>> but as long as its IRQ is in disabled/masked state it
>>>> doesn't matter.
>>> ...but there's no requirement that someone would need to disable/mask
>>> an interrupt while switching the muxing, is there?  So it does matter.
>>>
>>>
>>>> its only when the GPIO is again set to IRQ mode with set_mux callback,
>>>> the phantom IRQ needs clear to start as clean.
>>>>
>>>> So we should check only for if (i == pctrl->soc->gpio_func) then clear
>>>> phantom IRQ.
>>>>
>>>> The same is case with .direction_output callback, when GPIO is used as
>>>> output say as clock, need not clear any phantom IRQ,
>>>>
>>>> The reason is with every pulse of clock it can latch as pending IRQ in
>>>> GIC_ISPEND as long as it stay as output mode/clock.
>>>>
>>>> its only when switching back GPIO from output direction to input & IRQ
>>>> function, need to clear the phantom IRQ.
>>>>
>>>> so we do not require clear phantom irq in .direction_output callback.
>>> I think all the above explanation is with the model that the interrupt
>>> detection logic is still happening even when muxed away.  I don't
>>> believe that's true.
>> Its not the interrupt detection logic that is still happening when muxed
>> away, but the GPIO line is routed to GIC from PDC.
>> The GPIO line get forwarded when the system is active/out of system
>> level low power mode to GIC irrespective of whether GPIO is used as
>> interrupt or not.
>> Due to this it can still latch the IRQ at GIC after switching to lets
>> say Rx mode, whenever the line has any data recive, the line state
>> toggles can be latched as error interrupt at GIC.
>  From my tests, though, I strongly believe that the pin is only visible
> to the PDC if it's muxed as GPIO.  Specifically, in my tests I did
> this (with all my patches applied so there were no phantom interrupts
> when remuxing):
>
> a) Muxed the pin away from GPIO to special function, but _didn't_ mask
> the interrupt.
>
> b) Toggled the line a whole bunch.  These caused no interrupts at all.
>
> c) Muxed back to GPIO.
>
> To me this is quite strong evidence that the muxing is "earlier" in
> the path than the connection to the PDC.  In other words, if you
> change the mux away from GPIO then the PDC stops seeing it and thus
> the GIC also stops seeing it.  The GIC can't latch what it can't see.
> This means while you're in "Rx mode" it can't be latched.
>
>
> OK, so just in case this somehow only happens in S3, I also tried this
> (with my patch from https://crrev.com/c/2556012):
>
> a) Muxed away from GPIO ("bogus" pinmux)
>
> b) Enter S3.
>
> c) Toggle the GPIO a whole bunch ("wp enable / wp disable" on Cr50).
>
> d) Wake from S3.
>
> e) Check to see if the interrupt fired a bunch.  It didn't fire at all
>
>
> In my test code the interrupt is not masked, only muxed away.  That
> means that if, somehow, the PDC was still observing it then we'd see
> the interrupt fire.  We don't.
>
>
> Unless I messed up in my tests (always possible, though by this point
> I've run them a number of times) then it still feels like your mental
> model is wrong, or it's always possible I'm still misunderstanding
> your model.  Regardless, rather than trying to re-explain your model
> can you please confirm that you've written test code to confirm your
> mental model?  If so, can you please provide this test code?  I've
> provided several test patches proving out my mental model.
Its not a mental model, its how the line is connected to GIC.
GPIO follows the path, TLMM to PDC to GIC.
PDC donot know if the line is muxed away from GPIO to some other 
function, so it can stop forwarding to GIC.

I have slightly modified your test case (see at 
https://crrev.com/c/2584729) which is as per what i used in my testing.

Here is what i am doing, setting GPIO to a fixed function (function 2 here)
Note that function 0 is the GPIO (interrupt mode).

1) Pull up the GPIO in function 2
2) Pull down the GPIO in function 2

Repeat above steps, and you will see fake interrupt every time pull down/up.
This proves that if you mux away from GPIO then still PDC sees the line 
and can latch the interrupt at GIC.

Thanks,
Maulik

>> As the interrupt is in disabled state it won't be sent to CPU.
>> Its only when the driver chooses to switch back to interrupt mode we
>> want to clear the error interrupt latched to start as clean. same is the
>> case when used as output direction.
>>
>> Hope above is clear.
> Unfortunately, it's still not.  :(  Can I convince you to provide a
> test patch and a set of steps that will demonstrate the problem you're
> worried about?  Specifically:
>
> a) Maybe you're talking about the initial switch from a plain GPIO
> input to making it an interrupt for the first time?  Are you worried
> about a phantom interrupt in this case?  After patch #1 I think we're
> safe because pdc_gic_set_type() will always clear the interrupt,
> right?
>
>
> b) You say "switch back to interrupt mode".  Are you imagining that a
> driver does something like this:
>
> request_irq();
> ...
> free_irq();
> ...
> request_irq();
>
> If you're worried about that then we can implement irq_shutdown() for
> PDC and then make sure we clear on the first enable after a shutdown,
> I guess?
>
>
> c) Maybe when you say "switch back to interrupt mode" you mean
> something else?  If you are talking about muxing away and then muxing
> back then I think we already have this covered.  If you are talking
> about masking/unmasking then the whole point is that we _do_ want
> interrupts latched while masked, right?
>
>
> OK, I'm going to send out a v3 just to get the already-identified
> problems fixed and also to allow landing of patch #1 in the series,
> which I think is all agreed upon.  My request to you is that if you
> think my code misses a specific case to provide some test patches to
> demonstrate that case.
>
>
> -Doug

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

Re: [PATCH v2 3/3] pinctrl: qcom: Clear possible pending irq when remuxing GPIOs

[Doug Anderson]

Hi,

On Thu, Dec 10, 2020 at 11:07 PM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> I have slightly modified your test case (see at
> https://crrev.com/c/2584729) which is as per what i used in my testing.
>
> Here is what i am doing, setting GPIO to a fixed function (function 2 here)
> Note that function 0 is the GPIO (interrupt mode).
>
> 1) Pull up the GPIO in function 2
> 2) Pull down the GPIO in function 2
>
> Repeat above steps, and you will see fake interrupt every time pull down/up.
> This proves that if you mux away from GPIO then still PDC sees the line
> and can latch the interrupt at GIC.

Ah, super useful example!  Thanks!  Yes, I can replicate your results.

...but this seems to contradict my other test.  Ah, dang, I think I
see the problem with my original test.  The important difference is
that in your test you used the alternate function "mi2s_2" and in mine
I used "qspi_data".  When I selected "qspi_data" it must have been
actively driving the pin and _that's_ why I couldn't affect it.

When I change my test to use "mi2s_2" then my toggles via "wp enable"
and "wp disable" cause phantom interrupts.  That confirms what you're
saying: the PDC _can_ see the twiddles even when muxed away.
Presumably the active driving my "qspi_data" is also what caused my
phantom glitches.

So, as you said, that means my mental model is totally wrong here.
Wow, if I had known that earlier I would have saved a lot of time.
That'll learn me...

OK, v4 being posted and you can see if that handles all the cases?

-Doug

Re: [PATCH v2 1/3] irqchip: qcom-pdc: Fix phantom irq when changing between rising/falling

[Doug Anderson]

Hi,

On Tue, Nov 24, 2020 at 9:47 AM Douglas Anderson <dianders@chromium.org> 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.
>
> 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>

NOTE: even though this has Maulik's tags, he has requested [1] that we
should wait before landing while he talks with HW folks.

[1] https://lore.kernel.org/linux-arm-msm/603c691f-3614-d87b-075a-0889e9ffc453@codeaurora.org/


-Doug

Re: [PATCH v2 1/3] irqchip: qcom-pdc: Fix phantom irq when changing between rising/falling

[Linus Walleij]

On Mon, Nov 30, 2020 at 10:32 PM Doug Anderson <dianders@chromium.org> wrote:

> NOTE: even though this has Maulik's tags, he has requested [1] that we
> should wait before landing while he talks with HW folks.

OK I'm holding this series back until you have confirmation.
When you know you want it applied, poke me (or send a new
iteration).

Yours,
Linus Walleij

Re: [PATCH v2 1/3] irqchip: qcom-pdc: Fix phantom irq when changing between rising/falling

[Maulik Shah]

Hi Doug,

On 11/24/2020 11:17 PM, 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.
>
> 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>
> ---
> 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.
>
> 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..f91e7d5aea25 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 if a line is already
> +	 * high and we change to rising or if a line is already low and we
> +	 * change to falling 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.
> +	 */
Got confirmation that the phantom can show up when we change the 
polarity of the interrupt without changing the state of the signal.

Can you please change above comment to include above when you spin v3.

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

Thanks,
Maulik
> +	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 = {

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