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

[PATCH v5 1/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 e051aecf95c4..1d2a78452c2d 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.729.g45daf8777d-goog

[PATCH v5 2/4] pinctrl: qcom: No need to read-modify-write the interrupt status

[Douglas Anderson]

When the Qualcomm pinctrl driver wants to Ack an interrupt, it does a
read-modify-write on the interrupt status register.  On some SoCs it
makes sure that the status bit is 1 to "Ack" and on others it makes
sure that the bit is 0 to "Ack".  Presumably the first type of
interrupt controller is a "write 1 to clear" type register and the
second just let you directly set the interrupt status register.

As far as I can tell from scanning structure definitions, the
interrupt status bit is always in a register by itself.  Thus with
both types of interrupt controllers it is safe to "Ack" interrupts
without doing a read-modify-write.  We can do a simple write.

It should be noted that if the interrupt status bit _was_ ever in a
register with other things (like maybe status bits for other GPIOs):
a) For "write 1 clear" type controllers then read-modify-write would
   be totally wrong because we'd accidentally end up clearing
   interrupts we weren't looking at.
b) For "direct set" type controllers then read-modify-write would also
   be wrong because someone setting one of the other bits in the
   register might accidentally clear (or set) our interrupt.
I say this simply to show that the current read-modify-write doesn't
provide any sort of "future proofing" of the code.  In fact (for
"write 1 clear" controllers) the new code is slightly more "future
proof" since it would allow more than one interrupt status bits to
share a register.

NOTE: this code fixes no bugs--it simply avoids an extra register
read.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v5:
- ("pinctrl: qcom: No need to read-modify-write the ...") new for v5.

 drivers/pinctrl/qcom/pinctrl-msm.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 1d2a78452c2d..1787ada6bfab 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -792,16 +792,13 @@ 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);
-	}
+	/*
+	 * clear the interrupt status bit before unmask to avoid
+	 * any erroneous interrupts that would have got latched
+	 * when the interrupt is not in use.
+	 */
+	if (status_clear)
+		msm_writel_intr_status(0, pctrl, g);
 
 	val = msm_readl_intr_cfg(pctrl, g);
 	val |= BIT(g->intr_raw_status_bit);
@@ -906,11 +903,7 @@ static void msm_gpio_irq_ack(struct irq_data *d)
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
-	val = msm_readl_intr_status(pctrl, g);
-	if (g->intr_ack_high)
-		val |= BIT(g->intr_status_bit);
-	else
-		val &= ~BIT(g->intr_status_bit);
+	val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
 	msm_writel_intr_status(val, pctrl, g);
 
 	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
-- 
2.29.2.729.g45daf8777d-goog

[PATCH v5 3/4] pinctrl: qcom: Properly clear "intr_ack_high" interrupts when unmasking

[Douglas Anderson]

In commit 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for
msm gpio") we tried to Ack interrupts during unmask.  However, that
patch forgot to check "intr_ack_high" so, presumably, it only worked
for a certain subset of SoCs.

Let's add a small accessor so we don't need to open-code the logic in
both places.

This was found by code inspection.  I don't have any access to the
hardware in question nor software that needs the Ack during unmask.

Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
It should be noted that this code will be moved in the next patch.  In
theory this could be squashed into the next patch but it seems more
documenting to have this as a separate patch.

Changes in v5:
- ("pinctrl: qcom: Properly clear "intr_ack_high" interrupts...") new for v5.

 drivers/pinctrl/qcom/pinctrl-msm.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 1787ada6bfab..a6b0c17e2f78 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -96,6 +96,14 @@ MSM_ACCESSOR(intr_cfg)
 MSM_ACCESSOR(intr_status)
 MSM_ACCESSOR(intr_target)
 
+static void msm_ack_intr_status(struct msm_pinctrl *pctrl,
+				const struct msm_pingroup *g)
+{
+	u32 val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
+
+	msm_writel_intr_status(val, pctrl, g);
+}
+
 static int msm_get_groups_count(struct pinctrl_dev *pctldev)
 {
 	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
@@ -798,7 +806,7 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
 	 * when the interrupt is not in use.
 	 */
 	if (status_clear)
-		msm_writel_intr_status(0, pctrl, g);
+		msm_ack_intr_status(pctrl, g);
 
 	val = msm_readl_intr_cfg(pctrl, g);
 	val |= BIT(g->intr_raw_status_bit);
@@ -891,7 +899,6 @@ static void msm_gpio_irq_ack(struct irq_data *d)
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 	const struct msm_pingroup *g;
 	unsigned long flags;
-	u32 val;
 
 	if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) {
 		if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
@@ -903,8 +910,7 @@ static void msm_gpio_irq_ack(struct irq_data *d)
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
-	val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
-	msm_writel_intr_status(val, pctrl, g);
+	msm_ack_intr_status(pctrl, g);
 
 	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
 		msm_gpio_update_dual_edge_pos(pctrl, g, d);
-- 
2.29.2.729.g45daf8777d-goog

[PATCH v5 4/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 and it had problems (it caused
sc7180-trogdor devices to fail to suspend).  Let's fix.

From my understanding the source of the phantom interrupt in the
were these two things:
1. One that could have been introduced in msm_gpio_irq_set_type()
   (only for the non-PDC case).
2. Edges could have been detected when a GPIO was muxed away.

Fixing case #1 is easy.  We can just add a clear in
msm_gpio_irq_set_type().

Fixing case #2 is harder.  Let's use a concrete 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.

As per Qualcomm, when we mux the pin over to the UART function the PDC
(or the non-PDC interrupt detection logic) 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 interrupt.  This isn't necessary in
  the above case since the client already masked us, but it's a good
  idea in general.
* When we mux back will clear any interrupts and unmask.

Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
Fixes: 71266d9d3936 ("pinctrl: qcom: Move clearing pending IRQ to .irq_request_resources callback")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Note that patch #1 of v4 has now landed so it's dropped from the v5
post.  Also note that there is no dependency of this series on the v4
patch #1 so no special coordination need happen for that.

I didn't add Rajendra's and Stephen's tags from v4 since there were
enough changes from v4 to v5 to warrant a re-review.

My tests here for the non-PDC case are mostly synthetic and I don't
have any good way to test the case that the original patch was added
for.  Hopefully it's all good?

Changes in v5:
- Re-combined PDC and non-PDC since non-PDC their issues are similar.
- "it" => "the interrupt" in comment.
- Handle 2nd case where edges came when muxed away.
- Handle controllers where you write 1 to Ack.

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

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

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index a6b0c17e2f78..d5d1f3430c6c 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -51,6 +51,7 @@
  * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
  *                  detection.
  * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt controller
+ * @disabled_for_mux: These IRQs were disabled because we muxed away.
  * @soc:            Reference to soc_data of platform specific data.
  * @regs:           Base addresses for the TLMM tiles.
  * @phys_base:      Physical base address
@@ -72,6 +73,7 @@ struct msm_pinctrl {
 	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
 	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
 	DECLARE_BITMAP(skip_wake_irqs, MAX_NR_GPIO);
+	DECLARE_BITMAP(disabled_for_mux, MAX_NR_GPIO);
 
 	const struct msm_pinctrl_soc_data *soc;
 	void __iomem *regs[MAX_NR_TILES];
@@ -179,6 +181,10 @@ 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;
 	unsigned long flags;
 	u32 val, mask;
@@ -195,6 +201,20 @@ 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 then we need special
+	 * handling.  Specifically interrupt detection logic will still see
+	 * the pin twiddle even when we're muxed away.
+	 *
+	 * When we see a pin with an interrupt setup on it then we'll disable
+	 * (mask) interrupts on it when we mux away until we mux back.  Note
+	 * that disable_irq() refcounts and interrupts are disabled as long as
+	 * at least one disable_irq() has been called.
+	 */
+	if (d && i != gpio_func &&
+	    !test_and_set_bit(d->hwirq, pctrl->disabled_for_mux))
+		disable_irq(irq);
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = msm_readl_ctl(pctrl, g);
@@ -204,6 +224,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
+	if (d && i == gpio_func &&
+	    test_and_clear_bit(d->hwirq, pctrl->disabled_for_mux)) {
+		/*
+		 * Clear interrupts detected while not GPIO since we only
+		 * masked things.
+		 */
+		if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
+			irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
+		else
+			msm_ack_intr_status(pctrl, g);
+
+		enable_irq(irq);
+	}
+
 	return 0;
 }
 
@@ -782,7 +816,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);
@@ -800,14 +834,6 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
-	/*
-	 * clear the interrupt status bit before unmask to avoid
-	 * any erroneous interrupts that would have got latched
-	 * when the interrupt is not in use.
-	 */
-	if (status_clear)
-		msm_ack_intr_status(pctrl, g);
-
 	val = msm_readl_intr_cfg(pctrl, g);
 	val |= BIT(g->intr_raw_status_bit);
 	val |= BIT(g->intr_enable_bit);
@@ -827,7 +853,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)
@@ -842,11 +868,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.
@@ -935,6 +956,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)) {
@@ -996,6 +1018,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);
@@ -1045,6 +1068,14 @@ 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 the interrupt.  This is safe because we have
+	 * IRQCHIP_SET_TYPE_MASKED.
+	 */
+	if (!was_enabled)
+		msm_ack_intr_status(pctrl, g);
+
 	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
 		msm_gpio_update_dual_edge_pos(pctrl, g, d);
 
@@ -1096,19 +1127,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.729.g45daf8777d-goog

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

[Linus Walleij]

Hi Doug,

this is an impressive patch.

We definitely need to touch base with Bjorn on this, preferably also
Sboyd.

On Fri, Jan 8, 2021 at 6:35 PM Douglas Anderson <dianders@chromium.org> wrote:

> Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> Fixes: 71266d9d3936 ("pinctrl: qcom: Move clearing pending IRQ to .irq_request_resources callback")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Some mechanics:
1. Does this need to go into stable? Or is current (non-urgent) fine? Or fixes
   for v5.10? I.e. required destination.
2. If it does, should patches 1-3 also go into stable? And are they
prerequisites?

Yours,
Linus Walleij

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

[Maulik Shah]

Hi Doug,

Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
Tested-by: Maulik Shah <mkshah@codeaurora.org>

Thanks,
Maulik

On 1/8/2021 11:05 PM, Douglas Anderson wrote:
> 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 e051aecf95c4..1d2a78452c2d 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;

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

Re: [PATCH v5 2/4] pinctrl: qcom: No need to read-modify-write the interrupt status

[Maulik Shah]

Hi Doug,

Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
Tested-by: Maulik Shah <mkshah@codeaurora.org>

Thanks,
Maulik

On 1/8/2021 11:05 PM, Douglas Anderson wrote:
> When the Qualcomm pinctrl driver wants to Ack an interrupt, it does a
> read-modify-write on the interrupt status register.  On some SoCs it
> makes sure that the status bit is 1 to "Ack" and on others it makes
> sure that the bit is 0 to "Ack".  Presumably the first type of
> interrupt controller is a "write 1 to clear" type register and the
> second just let you directly set the interrupt status register.
>
> As far as I can tell from scanning structure definitions, the
> interrupt status bit is always in a register by itself.  Thus with
> both types of interrupt controllers it is safe to "Ack" interrupts
> without doing a read-modify-write.  We can do a simple write.
>
> It should be noted that if the interrupt status bit _was_ ever in a
> register with other things (like maybe status bits for other GPIOs):
> a) For "write 1 clear" type controllers then read-modify-write would
>     be totally wrong because we'd accidentally end up clearing
>     interrupts we weren't looking at.
> b) For "direct set" type controllers then read-modify-write would also
>     be wrong because someone setting one of the other bits in the
>     register might accidentally clear (or set) our interrupt.
> I say this simply to show that the current read-modify-write doesn't
> provide any sort of "future proofing" of the code.  In fact (for
> "write 1 clear" controllers) the new code is slightly more "future
> proof" since it would allow more than one interrupt status bits to
> share a register.
>
> NOTE: this code fixes no bugs--it simply avoids an extra register
> read.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v5:
> - ("pinctrl: qcom: No need to read-modify-write the ...") new for v5.
>
>   drivers/pinctrl/qcom/pinctrl-msm.c | 23 ++++++++---------------
>   1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 1d2a78452c2d..1787ada6bfab 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -792,16 +792,13 @@ 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);
> -	}
> +	/*
> +	 * clear the interrupt status bit before unmask to avoid
> +	 * any erroneous interrupts that would have got latched
> +	 * when the interrupt is not in use.
> +	 */
> +	if (status_clear)
> +		msm_writel_intr_status(0, pctrl, g);
>   
>   	val = msm_readl_intr_cfg(pctrl, g);
>   	val |= BIT(g->intr_raw_status_bit);
> @@ -906,11 +903,7 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>   
>   	raw_spin_lock_irqsave(&pctrl->lock, flags);
>   
> -	val = msm_readl_intr_status(pctrl, g);
> -	if (g->intr_ack_high)
> -		val |= BIT(g->intr_status_bit);
> -	else
> -		val &= ~BIT(g->intr_status_bit);
> +	val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
>   	msm_writel_intr_status(val, pctrl, g);
>   
>   	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))

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

Re: [PATCH v5 3/4] pinctrl: qcom: Properly clear "intr_ack_high" interrupts when unmasking

[Maulik Shah]

Hi Doug,

Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
Tested-by: Maulik Shah <mkshah@codeaurora.org>

Thanks,
Maulik

On 1/8/2021 11:05 PM, Douglas Anderson wrote:
> In commit 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for
> msm gpio") we tried to Ack interrupts during unmask.  However, that
> patch forgot to check "intr_ack_high" so, presumably, it only worked
> for a certain subset of SoCs.
>
> Let's add a small accessor so we don't need to open-code the logic in
> both places.
>
> This was found by code inspection.  I don't have any access to the
> hardware in question nor software that needs the Ack during unmask.
>
> Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> It should be noted that this code will be moved in the next patch.  In
> theory this could be squashed into the next patch but it seems more
> documenting to have this as a separate patch.
>
> Changes in v5:
> - ("pinctrl: qcom: Properly clear "intr_ack_high" interrupts...") new for v5.
>
>   drivers/pinctrl/qcom/pinctrl-msm.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 1787ada6bfab..a6b0c17e2f78 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -96,6 +96,14 @@ MSM_ACCESSOR(intr_cfg)
>   MSM_ACCESSOR(intr_status)
>   MSM_ACCESSOR(intr_target)
>   
> +static void msm_ack_intr_status(struct msm_pinctrl *pctrl,
> +				const struct msm_pingroup *g)
> +{
> +	u32 val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
> +
> +	msm_writel_intr_status(val, pctrl, g);
> +}
> +
>   static int msm_get_groups_count(struct pinctrl_dev *pctldev)
>   {
>   	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> @@ -798,7 +806,7 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
>   	 * when the interrupt is not in use.
>   	 */
>   	if (status_clear)
> -		msm_writel_intr_status(0, pctrl, g);
> +		msm_ack_intr_status(pctrl, g);
>   
>   	val = msm_readl_intr_cfg(pctrl, g);
>   	val |= BIT(g->intr_raw_status_bit);
> @@ -891,7 +899,6 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>   	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>   	const struct msm_pingroup *g;
>   	unsigned long flags;
> -	u32 val;
>   
>   	if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) {
>   		if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
> @@ -903,8 +910,7 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>   
>   	raw_spin_lock_irqsave(&pctrl->lock, flags);
>   
> -	val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
> -	msm_writel_intr_status(val, pctrl, g);
> +	msm_ack_intr_status(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 v5 4/4] pinctrl: qcom: Don't clear pending interrupts when enabling

[Maulik Shah]

Hi Doug,

Thanks for the patch. Looks good to me and tested.

Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
Tested-by: Maulik Shah <mkshah@codeaurora.org>

Thanks,
Maulik

On 1/8/2021 11:05 PM, 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 and it had problems (it caused
> sc7180-trogdor devices to fail to suspend).  Let's fix.
>
>  From my understanding the source of the phantom interrupt in the
> were these two things:
> 1. One that could have been introduced in msm_gpio_irq_set_type()
>     (only for the non-PDC case).
> 2. Edges could have been detected when a GPIO was muxed away.
>
> Fixing case #1 is easy.  We can just add a clear in
> msm_gpio_irq_set_type().
>
> Fixing case #2 is harder.  Let's use a concrete 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.
>
> As per Qualcomm, when we mux the pin over to the UART function the PDC
> (or the non-PDC interrupt detection logic) 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 interrupt.  This isn't necessary in
>    the above case since the client already masked us, but it's a good
>    idea in general.
> * When we mux back will clear any interrupts and unmask.
>
> Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> Fixes: 71266d9d3936 ("pinctrl: qcom: Move clearing pending IRQ to .irq_request_resources callback")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Note that patch #1 of v4 has now landed so it's dropped from the v5
> post.  Also note that there is no dependency of this series on the v4
> patch #1 so no special coordination need happen for that.
>
> I didn't add Rajendra's and Stephen's tags from v4 since there were
> enough changes from v4 to v5 to warrant a re-review.
>
> My tests here for the non-PDC case are mostly synthetic and I don't
> have any good way to test the case that the original patch was added
> for.  Hopefully it's all good?
>
> Changes in v5:
> - Re-combined PDC and non-PDC since non-PDC their issues are similar.
> - "it" => "the interrupt" in comment.
> - Handle 2nd case where edges came when muxed away.
> - Handle controllers where you write 1 to Ack.
>
> Changes in v4:
> - Split non-PDC fix and PDC fix in two.
> - Totally rewrote again with my new understanding of the world.
>
> 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 | 74 +++++++++++++++++++-----------
>   1 file changed, 46 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index a6b0c17e2f78..d5d1f3430c6c 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -51,6 +51,7 @@
>    * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
>    *                  detection.
>    * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt controller
> + * @disabled_for_mux: These IRQs were disabled because we muxed away.
>    * @soc:            Reference to soc_data of platform specific data.
>    * @regs:           Base addresses for the TLMM tiles.
>    * @phys_base:      Physical base address
> @@ -72,6 +73,7 @@ struct msm_pinctrl {
>   	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
>   	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
>   	DECLARE_BITMAP(skip_wake_irqs, MAX_NR_GPIO);
> +	DECLARE_BITMAP(disabled_for_mux, MAX_NR_GPIO);
>   
>   	const struct msm_pinctrl_soc_data *soc;
>   	void __iomem *regs[MAX_NR_TILES];
> @@ -179,6 +181,10 @@ 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;
>   	unsigned long flags;
>   	u32 val, mask;
> @@ -195,6 +201,20 @@ 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 then we need special
> +	 * handling.  Specifically interrupt detection logic will still see
> +	 * the pin twiddle even when we're muxed away.
> +	 *
> +	 * When we see a pin with an interrupt setup on it then we'll disable
> +	 * (mask) interrupts on it when we mux away until we mux back.  Note
> +	 * that disable_irq() refcounts and interrupts are disabled as long as
> +	 * at least one disable_irq() has been called.
> +	 */
> +	if (d && i != gpio_func &&
> +	    !test_and_set_bit(d->hwirq, pctrl->disabled_for_mux))
> +		disable_irq(irq);
> +
>   	raw_spin_lock_irqsave(&pctrl->lock, flags);
>   
>   	val = msm_readl_ctl(pctrl, g);
> @@ -204,6 +224,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>   
>   	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>   
> +	if (d && i == gpio_func &&
> +	    test_and_clear_bit(d->hwirq, pctrl->disabled_for_mux)) {
> +		/*
> +		 * Clear interrupts detected while not GPIO since we only
> +		 * masked things.
> +		 */
> +		if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
> +			irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
> +		else
> +			msm_ack_intr_status(pctrl, g);
> +
> +		enable_irq(irq);
> +	}
> +
>   	return 0;
>   }
>   
> @@ -782,7 +816,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);
> @@ -800,14 +834,6 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
>   
>   	raw_spin_lock_irqsave(&pctrl->lock, flags);
>   
> -	/*
> -	 * clear the interrupt status bit before unmask to avoid
> -	 * any erroneous interrupts that would have got latched
> -	 * when the interrupt is not in use.
> -	 */
> -	if (status_clear)
> -		msm_ack_intr_status(pctrl, g);
> -
>   	val = msm_readl_intr_cfg(pctrl, g);
>   	val |= BIT(g->intr_raw_status_bit);
>   	val |= BIT(g->intr_enable_bit);
> @@ -827,7 +853,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)
> @@ -842,11 +868,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.
> @@ -935,6 +956,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)) {
> @@ -996,6 +1018,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);
> @@ -1045,6 +1068,14 @@ 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 the interrupt.  This is safe because we have
> +	 * IRQCHIP_SET_TYPE_MASKED.
> +	 */
> +	if (!was_enabled)
> +		msm_ack_intr_status(pctrl, g);
> +
>   	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
>   		msm_gpio_update_dual_edge_pos(pctrl, g, d);
>   
> @@ -1096,19 +1127,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 v5 2/4] pinctrl: qcom: No need to read-modify-write the interrupt status

[Stephen Boyd]

Quoting Douglas Anderson (2021-01-08 09:35:14)
> When the Qualcomm pinctrl driver wants to Ack an interrupt, it does a
> read-modify-write on the interrupt status register.  On some SoCs it
> makes sure that the status bit is 1 to "Ack" and on others it makes
> sure that the bit is 0 to "Ack".  Presumably the first type of
> interrupt controller is a "write 1 to clear" type register and the
> second just let you directly set the interrupt status register.
> 
> As far as I can tell from scanning structure definitions, the
> interrupt status bit is always in a register by itself.  Thus with
> both types of interrupt controllers it is safe to "Ack" interrupts
> without doing a read-modify-write.  We can do a simple write.
> 
> It should be noted that if the interrupt status bit _was_ ever in a
> register with other things (like maybe status bits for other GPIOs):
> a) For "write 1 clear" type controllers then read-modify-write would
>    be totally wrong because we'd accidentally end up clearing
>    interrupts we weren't looking at.
> b) For "direct set" type controllers then read-modify-write would also
>    be wrong because someone setting one of the other bits in the
>    register might accidentally clear (or set) our interrupt.
> I say this simply to show that the current read-modify-write doesn't
> provide any sort of "future proofing" of the code.  In fact (for
> "write 1 clear" controllers) the new code is slightly more "future
> proof" since it would allow more than one interrupt status bits to
> share a register.
> 
> NOTE: this code fixes no bugs--it simply avoids an extra register
> read.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

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

Re: [PATCH v5 3/4] pinctrl: qcom: Properly clear "intr_ack_high" interrupts when unmasking

[Stephen Boyd]

Quoting Douglas Anderson (2021-01-08 09:35:15)
> In commit 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for
> msm gpio") we tried to Ack interrupts during unmask.  However, that
> patch forgot to check "intr_ack_high" so, presumably, it only worked
> for a certain subset of SoCs.
> 
> Let's add a small accessor so we don't need to open-code the logic in
> both places.
> 
> This was found by code inspection.  I don't have any access to the
> hardware in question nor software that needs the Ack during unmask.
> 
> 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 minor nit below.

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 1787ada6bfab..a6b0c17e2f78 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -96,6 +96,14 @@ MSM_ACCESSOR(intr_cfg)
>  MSM_ACCESSOR(intr_status)
>  MSM_ACCESSOR(intr_target)
>  
> +static void msm_ack_intr_status(struct msm_pinctrl *pctrl,
> +                               const struct msm_pingroup *g)
> +{
> +       u32 val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;

Would be nice to remove that extra parenthesis too.

> +
> +       msm_writel_intr_status(val, pctrl, g);
> +}
> +
>  static int msm_get_groups_count(struct pinctrl_dev *pctldev)
>  {
>         struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> @@ -903,8 +910,7 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>  
>         raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
> -       val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;

Even though it is here.

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

[Stephen Boyd]

Quoting Douglas Anderson (2021-01-08 09:35:16)
> Let's deal with the problem like this:
> * When we mux away, we'll mask our interrupt.  This isn't necessary in
>   the above case since the client already masked us, but it's a good
>   idea in general.
> * When we mux back will clear any interrupts and unmask.

I'm on board!

> 
> Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> Fixes: 71266d9d3936 ("pinctrl: qcom: Move clearing pending IRQ to .irq_request_resources callback")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index a6b0c17e2f78..d5d1f3430c6c 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -51,6 +51,7 @@
>   * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
>   *                  detection.
>   * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt controller
> + * @disabled_for_mux: These IRQs were disabled because we muxed away.
>   * @soc:            Reference to soc_data of platform specific data.
>   * @regs:           Base addresses for the TLMM tiles.
>   * @phys_base:      Physical base address
> @@ -72,6 +73,7 @@ struct msm_pinctrl {
>         DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
>         DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
>         DECLARE_BITMAP(skip_wake_irqs, MAX_NR_GPIO);
> +       DECLARE_BITMAP(disabled_for_mux, MAX_NR_GPIO);
>  
>         const struct msm_pinctrl_soc_data *soc;
>         void __iomem *regs[MAX_NR_TILES];
> @@ -179,6 +181,10 @@ 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;
>         unsigned long flags;
>         u32 val, mask;
> @@ -195,6 +201,20 @@ 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 then we need special
> +        * handling.  Specifically interrupt detection logic will still see
> +        * the pin twiddle even when we're muxed away.
> +        *
> +        * When we see a pin with an interrupt setup on it then we'll disable
> +        * (mask) interrupts on it when we mux away until we mux back.  Note
> +        * that disable_irq() refcounts and interrupts are disabled as long as
> +        * at least one disable_irq() has been called.
> +        */
> +       if (d && i != gpio_func &&
> +           !test_and_set_bit(d->hwirq, pctrl->disabled_for_mux))
> +               disable_irq(irq);

Does it need to be forced non-lazy so that it is actually disabled at
the GIC? I'm trying to understand how the lazy irq disabling plays into
this. I think it's a don't care situation because if the line twiddles
and triggers an irq then we'll actually disable it at the GIC in the
genirq core and mark it pending for resend. I wonder if we wouldn't have
to undo the pending state if we actually ignored it at the GIC
forcefully. And I also worry that it may cause a random wakeup if the
line twiddles, becomes pending at GIC and thus blocks the CPU from
running a WFI but it isn't an irq that Linux cares about because it's
muxed to UART, and then lazy handling runs and shuts it down. Is that
possible?

> +
>         raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
>         val = msm_readl_ctl(pctrl, g);
> @@ -204,6 +224,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>  
>         raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>  
> +       if (d && i == gpio_func &&
> +           test_and_clear_bit(d->hwirq, pctrl->disabled_for_mux)) {
> +               /*
> +                * Clear interrupts detected while not GPIO since we only
> +                * masked things.
> +                */
> +               if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
> +                       irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);

So if not lazy this could go away? Although I think this is to clear out
the pending state in the GIC and not the PDC which is the parent.

> +               else
> +                       msm_ack_intr_status(pctrl, g);
> +
> +               enable_irq(irq);
> +       }
> +

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

[Bjorn Andersson]

On Fri 08 Jan 11:35 CST 2021, Douglas Anderson wrote:

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

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.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 e051aecf95c4..1d2a78452c2d 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);

Although I would have preferred this line not be wrapped.

Regards,
Bjorn

>  }
>  
>  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.729.g45daf8777d-goog
> 

Re: [PATCH v5 2/4] pinctrl: qcom: No need to read-modify-write the interrupt status

[Bjorn Andersson]

On Fri 08 Jan 11:35 CST 2021, Douglas Anderson wrote:

> When the Qualcomm pinctrl driver wants to Ack an interrupt, it does a
> read-modify-write on the interrupt status register.  On some SoCs it
> makes sure that the status bit is 1 to "Ack" and on others it makes
> sure that the bit is 0 to "Ack".  Presumably the first type of
> interrupt controller is a "write 1 to clear" type register and the
> second just let you directly set the interrupt status register.
> 
> As far as I can tell from scanning structure definitions, the
> interrupt status bit is always in a register by itself.  Thus with
> both types of interrupt controllers it is safe to "Ack" interrupts
> without doing a read-modify-write.  We can do a simple write.
> 
> It should be noted that if the interrupt status bit _was_ ever in a
> register with other things (like maybe status bits for other GPIOs):
> a) For "write 1 clear" type controllers then read-modify-write would
>    be totally wrong because we'd accidentally end up clearing
>    interrupts we weren't looking at.
> b) For "direct set" type controllers then read-modify-write would also
>    be wrong because someone setting one of the other bits in the
>    register might accidentally clear (or set) our interrupt.
> I say this simply to show that the current read-modify-write doesn't
> provide any sort of "future proofing" of the code.  In fact (for
> "write 1 clear" controllers) the new code is slightly more "future
> proof" since it would allow more than one interrupt status bits to
> share a register.
> 
> NOTE: this code fixes no bugs--it simply avoids an extra register
> read.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> 
> Changes in v5:
> - ("pinctrl: qcom: No need to read-modify-write the ...") new for v5.
> 
>  drivers/pinctrl/qcom/pinctrl-msm.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 1d2a78452c2d..1787ada6bfab 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -792,16 +792,13 @@ 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);
> -	}
> +	/*
> +	 * clear the interrupt status bit before unmask to avoid
> +	 * any erroneous interrupts that would have got latched
> +	 * when the interrupt is not in use.
> +	 */
> +	if (status_clear)
> +		msm_writel_intr_status(0, pctrl, g);
>  
>  	val = msm_readl_intr_cfg(pctrl, g);
>  	val |= BIT(g->intr_raw_status_bit);
> @@ -906,11 +903,7 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>  
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
> -	val = msm_readl_intr_status(pctrl, g);
> -	if (g->intr_ack_high)
> -		val |= BIT(g->intr_status_bit);
> -	else
> -		val &= ~BIT(g->intr_status_bit);
> +	val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
>  	msm_writel_intr_status(val, pctrl, g);
>  
>  	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
> -- 
> 2.29.2.729.g45daf8777d-goog
> 

Re: [PATCH v5 3/4] pinctrl: qcom: Properly clear "intr_ack_high" interrupts when unmasking

[Bjorn Andersson]

On Fri 08 Jan 11:35 CST 2021, Douglas Anderson wrote:

> In commit 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for
> msm gpio") we tried to Ack interrupts during unmask.  However, that
> patch forgot to check "intr_ack_high" so, presumably, it only worked
> for a certain subset of SoCs.
> 
> Let's add a small accessor so we don't need to open-code the logic in
> both places.
> 
> This was found by code inspection.  I don't have any access to the
> hardware in question nor software that needs the Ack during unmask.
> 
> Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

And I agree with Stephen's comment about the unnecessary parenthesis
around g->intr_ack_high.

Regards,
Bjorn

> ---
> It should be noted that this code will be moved in the next patch.  In
> theory this could be squashed into the next patch but it seems more
> documenting to have this as a separate patch.
> 
> Changes in v5:
> - ("pinctrl: qcom: Properly clear "intr_ack_high" interrupts...") new for v5.
> 
>  drivers/pinctrl/qcom/pinctrl-msm.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 1787ada6bfab..a6b0c17e2f78 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -96,6 +96,14 @@ MSM_ACCESSOR(intr_cfg)
>  MSM_ACCESSOR(intr_status)
>  MSM_ACCESSOR(intr_target)
>  
> +static void msm_ack_intr_status(struct msm_pinctrl *pctrl,
> +				const struct msm_pingroup *g)
> +{
> +	u32 val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
> +
> +	msm_writel_intr_status(val, pctrl, g);
> +}
> +
>  static int msm_get_groups_count(struct pinctrl_dev *pctldev)
>  {
>  	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> @@ -798,7 +806,7 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
>  	 * when the interrupt is not in use.
>  	 */
>  	if (status_clear)
> -		msm_writel_intr_status(0, pctrl, g);
> +		msm_ack_intr_status(pctrl, g);
>  
>  	val = msm_readl_intr_cfg(pctrl, g);
>  	val |= BIT(g->intr_raw_status_bit);
> @@ -891,7 +899,6 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>  	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>  	const struct msm_pingroup *g;
>  	unsigned long flags;
> -	u32 val;
>  
>  	if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) {
>  		if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
> @@ -903,8 +910,7 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>  
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
> -	val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
> -	msm_writel_intr_status(val, pctrl, g);
> +	msm_ack_intr_status(pctrl, g);
>  
>  	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
>  		msm_gpio_update_dual_edge_pos(pctrl, g, d);
> -- 
> 2.29.2.729.g45daf8777d-goog
> 

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

[Bjorn Andersson]

On Thu 14 Jan 01:14 CST 2021, Stephen Boyd wrote:

> Quoting Douglas Anderson (2021-01-08 09:35:16)
> > Let's deal with the problem like this:
> > * When we mux away, we'll mask our interrupt.  This isn't necessary in
> >   the above case since the client already masked us, but it's a good
> >   idea in general.
> > * When we mux back will clear any interrupts and unmask.
> 
> I'm on board!
> 
> > 
> > Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> > Fixes: 71266d9d3936 ("pinctrl: qcom: Move clearing pending IRQ to .irq_request_resources callback")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index a6b0c17e2f78..d5d1f3430c6c 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -51,6 +51,7 @@
> >   * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
> >   *                  detection.
> >   * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt controller
> > + * @disabled_for_mux: These IRQs were disabled because we muxed away.
> >   * @soc:            Reference to soc_data of platform specific data.
> >   * @regs:           Base addresses for the TLMM tiles.
> >   * @phys_base:      Physical base address
> > @@ -72,6 +73,7 @@ struct msm_pinctrl {
> >         DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
> >         DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
> >         DECLARE_BITMAP(skip_wake_irqs, MAX_NR_GPIO);
> > +       DECLARE_BITMAP(disabled_for_mux, MAX_NR_GPIO);
> >  
> >         const struct msm_pinctrl_soc_data *soc;
> >         void __iomem *regs[MAX_NR_TILES];
> > @@ -179,6 +181,10 @@ 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;
> >         unsigned long flags;
> >         u32 val, mask;
> > @@ -195,6 +201,20 @@ 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 then we need special
> > +        * handling.  Specifically interrupt detection logic will still see
> > +        * the pin twiddle even when we're muxed away.
> > +        *
> > +        * When we see a pin with an interrupt setup on it then we'll disable
> > +        * (mask) interrupts on it when we mux away until we mux back.  Note
> > +        * that disable_irq() refcounts and interrupts are disabled as long as
> > +        * at least one disable_irq() has been called.
> > +        */
> > +       if (d && i != gpio_func &&
> > +           !test_and_set_bit(d->hwirq, pctrl->disabled_for_mux))
> > +               disable_irq(irq);
> 
> Does it need to be forced non-lazy so that it is actually disabled at
> the GIC? I'm trying to understand how the lazy irq disabling plays into
> this. I think it's a don't care situation because if the line twiddles
> and triggers an irq then we'll actually disable it at the GIC in the
> genirq core and mark it pending for resend. I wonder if we wouldn't have
> to undo the pending state if we actually ignored it at the GIC
> forcefully. And I also worry that it may cause a random wakeup if the
> line twiddles, becomes pending at GIC and thus blocks the CPU from
> running a WFI but it isn't an irq that Linux cares about because it's
> muxed to UART, and then lazy handling runs and shuts it down. Is that
> possible?
> 
> > +
> >         raw_spin_lock_irqsave(&pctrl->lock, flags);
> >  
> >         val = msm_readl_ctl(pctrl, g);
> > @@ -204,6 +224,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> >  
> >         raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >  
> > +       if (d && i == gpio_func &&
> > +           test_and_clear_bit(d->hwirq, pctrl->disabled_for_mux)) {
> > +               /*
> > +                * Clear interrupts detected while not GPIO since we only
> > +                * masked things.
> > +                */
> > +               if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
> > +                       irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
> 
> So if not lazy this could go away? Although I think this is to clear out
> the pending state in the GIC and not the PDC which is the parent.
> 

Isn't this the PDC line after all, because the GIC only has the summary
line while the PDC (if we have d->parent_data) has a dedicated line for
this irq?

Regards,
Bjorn

> > +               else
> > +                       msm_ack_intr_status(pctrl, g);
> > +
> > +               enable_irq(irq);
> > +       }
> > +

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

[Bjorn Andersson]

On Thu 14 Jan 01:14 CST 2021, Stephen Boyd wrote:

> Quoting Douglas Anderson (2021-01-08 09:35:16)
> > Let's deal with the problem like this:
> > * When we mux away, we'll mask our interrupt.  This isn't necessary in
> >   the above case since the client already masked us, but it's a good
> >   idea in general.
> > * When we mux back will clear any interrupts and unmask.
> 
> I'm on board!
> 
> > 
> > Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> > Fixes: 71266d9d3936 ("pinctrl: qcom: Move clearing pending IRQ to .irq_request_resources callback")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index a6b0c17e2f78..d5d1f3430c6c 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -51,6 +51,7 @@
> >   * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
> >   *                  detection.
> >   * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt controller
> > + * @disabled_for_mux: These IRQs were disabled because we muxed away.
> >   * @soc:            Reference to soc_data of platform specific data.
> >   * @regs:           Base addresses for the TLMM tiles.
> >   * @phys_base:      Physical base address
> > @@ -72,6 +73,7 @@ struct msm_pinctrl {
> >         DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
> >         DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
> >         DECLARE_BITMAP(skip_wake_irqs, MAX_NR_GPIO);
> > +       DECLARE_BITMAP(disabled_for_mux, MAX_NR_GPIO);
> >  
> >         const struct msm_pinctrl_soc_data *soc;
> >         void __iomem *regs[MAX_NR_TILES];
> > @@ -179,6 +181,10 @@ 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;
> >         unsigned long flags;
> >         u32 val, mask;
> > @@ -195,6 +201,20 @@ 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 then we need special
> > +        * handling.  Specifically interrupt detection logic will still see
> > +        * the pin twiddle even when we're muxed away.
> > +        *
> > +        * When we see a pin with an interrupt setup on it then we'll disable
> > +        * (mask) interrupts on it when we mux away until we mux back.  Note
> > +        * that disable_irq() refcounts and interrupts are disabled as long as
> > +        * at least one disable_irq() has been called.
> > +        */
> > +       if (d && i != gpio_func &&
> > +           !test_and_set_bit(d->hwirq, pctrl->disabled_for_mux))
> > +               disable_irq(irq);
> 
> Does it need to be forced non-lazy so that it is actually disabled at
> the GIC? I'm trying to understand how the lazy irq disabling plays into
> this. I think it's a don't care situation because if the line twiddles
> and triggers an irq then we'll actually disable it at the GIC in the
> genirq core and mark it pending for resend. I wonder if we wouldn't have
> to undo the pending state if we actually ignored it at the GIC
> forcefully. And I also worry that it may cause a random wakeup if the
> line twiddles, becomes pending at GIC and thus blocks the CPU from
> running a WFI but it isn't an irq that Linux cares about because it's
> muxed to UART, and then lazy handling runs and shuts it down. Is that
> possible?
> 

I was about to write a question about why we should disable the IRQ
through the irqchip framework, rather than just do it in the hardware
directly.

Which I think means that I came to the same conclusion as you, that if
we have a pin masked to non-gpio, it will still wake the system up, just
to actually disable the IRQ lazily.

Is there a problem with leaving the irq framework to believe the IRQ is
enabled while we disable the delivery in hardware?

Regards,
Bjorn

> > +
> >         raw_spin_lock_irqsave(&pctrl->lock, flags);
> >  
> >         val = msm_readl_ctl(pctrl, g);
> > @@ -204,6 +224,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> >  
> >         raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >  
> > +       if (d && i == gpio_func &&
> > +           test_and_clear_bit(d->hwirq, pctrl->disabled_for_mux)) {
> > +               /*
> > +                * Clear interrupts detected while not GPIO since we only
> > +                * masked things.
> > +                */
> > +               if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
> > +                       irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
> 
> So if not lazy this could go away? Although I think this is to clear out
> the pending state in the GIC and not the PDC which is the parent.
> 
> > +               else
> > +                       msm_ack_intr_status(pctrl, g);
> > +
> > +               enable_irq(irq);
> > +       }
> > +

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

[Doug Anderson]

Hi,

On Thu, Jan 14, 2021 at 9:15 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> > > @@ -195,6 +201,20 @@ 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 then we need special
> > > +        * handling.  Specifically interrupt detection logic will still see
> > > +        * the pin twiddle even when we're muxed away.
> > > +        *
> > > +        * When we see a pin with an interrupt setup on it then we'll disable
> > > +        * (mask) interrupts on it when we mux away until we mux back.  Note
> > > +        * that disable_irq() refcounts and interrupts are disabled as long as
> > > +        * at least one disable_irq() has been called.
> > > +        */
> > > +       if (d && i != gpio_func &&
> > > +           !test_and_set_bit(d->hwirq, pctrl->disabled_for_mux))
> > > +               disable_irq(irq);
> >
> > Does it need to be forced non-lazy so that it is actually disabled at
> > the GIC? I'm trying to understand how the lazy irq disabling plays into
> > this. I think it's a don't care situation because if the line twiddles
> > and triggers an irq then we'll actually disable it at the GIC in the
> > genirq core and mark it pending for resend. I wonder if we wouldn't have
> > to undo the pending state if we actually ignored it at the GIC
> > forcefully. And I also worry that it may cause a random wakeup if the
> > line twiddles, becomes pending at GIC and thus blocks the CPU from
> > running a WFI but it isn't an irq that Linux cares about because it's
> > muxed to UART, and then lazy handling runs and shuts it down. Is that
> > possible?
> >
>
> I was about to write a question about why we should disable the IRQ
> through the irqchip framework, rather than just do it in the hardware
> directly.
>
> Which I think means that I came to the same conclusion as you, that if
> we have a pin masked to non-gpio, it will still wake the system up, just
> to actually disable the IRQ lazily.
>
> Is there a problem with leaving the irq framework to believe the IRQ is
> enabled while we disable the delivery in hardware?

Earlier I had it disabling in hardware, but doing it through the IRQ
framework has the advantage of doing refcounting for us and that saves
us complexity.  If we tweaked the hardware directly we'd have to worry
about this case:

a) Client muxes away the pin: we disable in hardware
b) Client tries to disable/mask the interrupt themselves.
c) Client tries to enable/unmask the interrupt themselves

...when we got the call for c) we'd have to realize that we're still
muxed away and we'd have to ignore their request.  Also, if the mux
back happened in step b) we'd have to know _not_ to unmask the
interrupt.  Trying to solve those corner cases adds complexity.  If we
just rely on the refcounting the complexity goes away.


-Doug

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

[Doug Anderson]

Hi,

On Wed, Jan 13, 2021 at 11:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> > @@ -195,6 +201,20 @@ 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 then we need special
> > +        * handling.  Specifically interrupt detection logic will still see
> > +        * the pin twiddle even when we're muxed away.
> > +        *
> > +        * When we see a pin with an interrupt setup on it then we'll disable
> > +        * (mask) interrupts on it when we mux away until we mux back.  Note
> > +        * that disable_irq() refcounts and interrupts are disabled as long as
> > +        * at least one disable_irq() has been called.
> > +        */
> > +       if (d && i != gpio_func &&
> > +           !test_and_set_bit(d->hwirq, pctrl->disabled_for_mux))
> > +               disable_irq(irq);
>
> Does it need to be forced non-lazy so that it is actually disabled at
> the GIC?

Yes, I think non-lazy is important.  Specifically at the end I assume
that I can clear the interrupt in hardware and it will go away and
Linux never saw it.  If it was lazy, it's possible Linux saw the
interrupt and has it marked with IRQS_PENDING.

Right now we get non-lazy because we have "disable" implemented, so it
works fine.  I can be explicit.  Do I add a call to
msm_gpio_irq_reqres() like:

  irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY);

I'll wait for feedback if you think this is the right way to go before
sending the next version.


> I'm trying to understand how the lazy irq disabling plays into
> this. I think it's a don't care situation because if the line twiddles
> and triggers an irq then we'll actually disable it at the GIC in the
> genirq core and mark it pending for resend.

I think the marking as pending is a problem.  When we finally mux back
to GPIO we want to clear out anything that showed up while it was
muxed away and I'm not aware of a way to clear "IRQS_PENDING".


> I wonder if we wouldn't have
> to undo the pending state if we actually ignored it at the GIC
> forcefully. And I also worry that it may cause a random wakeup if the
> line twiddles, becomes pending at GIC and thus blocks the CPU from
> running a WFI but it isn't an irq that Linux cares about because it's
> muxed to UART, and then lazy handling runs and shuts it down. Is that
> possible?

I believe if the interrupt is masked at the GIC then it won't cause
wakeups.  Specifically to get wakeup stuff working we had to unmask
the interrupt at the GIC level.


> > +
> >         raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> >         val = msm_readl_ctl(pctrl, g);
> > @@ -204,6 +224,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> >
> >         raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >
> > +       if (d && i == gpio_func &&
> > +           test_and_clear_bit(d->hwirq, pctrl->disabled_for_mux)) {
> > +               /*
> > +                * Clear interrupts detected while not GPIO since we only
> > +                * masked things.
> > +                */
> > +               if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
> > +                       irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
>
> So if not lazy this could go away?

I don't think so.  If lazy we could have a pending interrupt tracked
in two places: in Linux and in the parent if this happened:
* disable_irq() - do nothing except mark that IRQ is disalbed.
* IRQ happened - track in Linux and actually disable (mask) the interrupt
* IRQ happened again - still tracked in Linux but now also latched in
hardware (but masked)

...so if it was lazy we'd need to clear the interrupt in two places.
With non-lazy we only have to clear the latch that happened in
hardware, right?

> Although I think this is to clear out
> the pending state in the GIC and not the PDC which is the parent.

Yeah, it clears the state that was latched in the GIC.  It just passes
through the PDC code on the way there.

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

[Stephen Boyd]

Quoting Doug Anderson (2021-01-14 09:58:55)
> Hi,
> 
> On Wed, Jan 13, 2021 at 11:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > @@ -195,6 +201,20 @@ 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 then we need special
> > > +        * handling.  Specifically interrupt detection logic will still see
> > > +        * the pin twiddle even when we're muxed away.
> > > +        *
> > > +        * When we see a pin with an interrupt setup on it then we'll disable
> > > +        * (mask) interrupts on it when we mux away until we mux back.  Note
> > > +        * that disable_irq() refcounts and interrupts are disabled as long as
> > > +        * at least one disable_irq() has been called.
> > > +        */
> > > +       if (d && i != gpio_func &&
> > > +           !test_and_set_bit(d->hwirq, pctrl->disabled_for_mux))
> > > +               disable_irq(irq);
> >
> > Does it need to be forced non-lazy so that it is actually disabled at
> > the GIC?
> 
> Yes, I think non-lazy is important.  Specifically at the end I assume
> that I can clear the interrupt in hardware and it will go away and
> Linux never saw it.  If it was lazy, it's possible Linux saw the
> interrupt and has it marked with IRQS_PENDING.
> 
> Right now we get non-lazy because we have "disable" implemented, so it
> works fine.  I can be explicit.  Do I add a call to
> msm_gpio_irq_reqres() like:
> 
>   irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY);

Yes that should be done explicitly. I suppose when the irq is requested
the first time is when we should do it. Or we can add a flag to gpiolib
to do that all the time for the irq domain? Basically make it so
gpiochip_hierarchy_irq_domain_alloc() sets the flag.

> 
> I'll wait for feedback if you think this is the right way to go before
> sending the next version.
> 
> 
> > I'm trying to understand how the lazy irq disabling plays into
> > this. I think it's a don't care situation because if the line twiddles
> > and triggers an irq then we'll actually disable it at the GIC in the
> > genirq core and mark it pending for resend.
> 
> I think the marking as pending is a problem.  When we finally mux back
> to GPIO we want to clear out anything that showed up while it was
> muxed away and I'm not aware of a way to clear "IRQS_PENDING".

Ok.

> 
> 
> > I wonder if we wouldn't have
> > to undo the pending state if we actually ignored it at the GIC
> > forcefully. And I also worry that it may cause a random wakeup if the
> > line twiddles, becomes pending at GIC and thus blocks the CPU from
> > running a WFI but it isn't an irq that Linux cares about because it's
> > muxed to UART, and then lazy handling runs and shuts it down. Is that
> > possible?
> 
> I believe if the interrupt is masked at the GIC then it won't cause
> wakeups.  Specifically to get wakeup stuff working we had to unmask
> the interrupt at the GIC level.

If I understand correctly, the lazy and non-lazy cases will masked at
the GIC either after the line twiddles or immediately here respectfully.
So either way we should be OK because it will be masked, but I'm still
unsure about the lazy case where we are in the process of suspending and
then the line twiddles. That would cause the irq to block suspend,
possibly really late if the CPU is running trusted firmware. So we
really need to make sure that it is non-lazy so this can't happen.

> 
> 
> > > +
> > >         raw_spin_lock_irqsave(&pctrl->lock, flags);
> > >
> > >         val = msm_readl_ctl(pctrl, g);
> > > @@ -204,6 +224,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> > >
> > >         raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > >
> > > +       if (d && i == gpio_func &&
> > > +           test_and_clear_bit(d->hwirq, pctrl->disabled_for_mux)) {
> > > +               /*
> > > +                * Clear interrupts detected while not GPIO since we only
> > > +                * masked things.
> > > +                */
> > > +               if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
> > > +                       irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
> >
> > So if not lazy this could go away?
> 
> I don't think so.  If lazy we could have a pending interrupt tracked
> in two places: in Linux and in the parent if this happened:
> * disable_irq() - do nothing except mark that IRQ is disalbed.
> * IRQ happened - track in Linux and actually disable (mask) the interrupt
> * IRQ happened again - still tracked in Linux but now also latched in
> hardware (but masked)
> 
> ...so if it was lazy we'd need to clear the interrupt in two places.
> With non-lazy we only have to clear the latch that happened in
> hardware, right?

Yes makes sense.

> 
> > Although I think this is to clear out
> > the pending state in the GIC and not the PDC which is the parent.
> 
> Yeah, it clears the state that was latched in the GIC.  It just passes
> through the PDC code on the way there.

Got it. I'm happy with this patch once it explicitly disables the lazy
mode of the gpio irqs.

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

[Doug Anderson]

Hi,

On Fri, Jan 8, 2021 at 4:36 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Doug,
>
> this is an impressive patch.
>
> We definitely need to touch base with Bjorn on this, preferably also
> Sboyd.
>
> On Fri, Jan 8, 2021 at 6:35 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> > Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> > Fixes: 71266d9d3936 ("pinctrl: qcom: Move clearing pending IRQ to .irq_request_resources callback")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> Some mechanics:

I just realized that I addressed everyone's comments but yours.  Doh!


> 1. Does this need to go into stable? Or is current (non-urgent) fine? Or fixes
>    for v5.10? I.e. required destination.

It probably ought to go into stable, but I'll leave it up to you which
version of Linux it lands in.  I don't personally know if anyone is
criticall waiting on this to land upstream.  For Chrome OS we're not
desperate for it because we've already landed a temporary revert of
Maulik's previous patch and the extra clearing of the masked
interrupts isn't causing any really visible problems for us.


> 2. If it does, should patches 1-3 also go into stable? And are they
> prerequisites?

Yeah, the last patch requires the previous ones, so they would all
need to go into stable together.

-Doug