[PATCH 0/2] pinctrl: qcom: Allow arming the wakeup_enable bit from pin config

[PATCH 0/2] pinctrl: qcom: Allow arming the wakeup_enable bit from pin config

[Mohammad Rafi Shaik]

The TLMM keeps a per-pin wakeup_enable bit that gates whether a toggle of
the pad is transferred to the wake up controller (PDC/MPM). Despite the
name, that transfer is what delivers the interrupt event as well as what
wakes the system from a low power mode, as the comment in
msm_gpio_irq_reqres() already notes.

Today the bit can only be armed by requesting the pin as a GPIO interrupt:
msm_gpio_irq_reqres() sets it and msm_gpio_irq_relres() clears it. That
path calls msm_pinmux_request_gpio() and so takes over the pin mux, which
makes it unusable for a pin that has to keep a peripheral function and is
still toggled by an external device to request a wake up.

Such a pin exists on boards with a SoundWire codec. While the bus is clock
stopped, the codec asks to be read out by toggling the SoundWire data line,
which stays muxed to the SoundWire function and is never requested as a
GPIO. With the pad not armed the toggle is dropped, the controller is not
resumed, and the codec's interrupt - a headset insertion, for example - is
only seen if something else happens to wake the bus first.

This series adds a "wakeup-enable" pin configuration parameter so the bit
can be armed from the pin's own configuration, without turning the pin into
a GPIO.

A pin the SoC data does not describe as wakeup capable - no
intr_wakeup_present_bit, or the present bit not set in the live register -
is rejected with -EOPNOTSUPP and an error message, rather than silently
accepting a configuration that cannot take effect. The GPIO interrupt path
keeps its existing best effort behaviour and ignores that return value, so
its behaviour is unchanged.

There is no functional change for any existing device tree: the new
parameter only does something where a board asks for it.

Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

---
Mohammad Rafi Shaik (2):
      dt-bindings: pinctrl: qcom,tlmm-common: Add wakeup-enable
      pinctrl: qcom: Allow arming the wakeup_enable bit from pin config

 .../bindings/pinctrl/qcom,tlmm-common.yaml         |  12 +++
 drivers/pinctrl/qcom/pinctrl-msm.c                 | 101 +++++++++++++++------
 2 files changed, 85 insertions(+), 28 deletions(-)
---
base-commit: d419d682257d813f6cb7a28e42b35d0edf4ad038
change-id: 20261015-tlmm_wakeup_gpio_support-9cf2c08af51f

Best regards,
--  
Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

[PATCH v1 1/2] dt-bindings: pinctrl: qcom,tlmm-common: Add wakeup-enable

[Mohammad Rafi Shaik]

The TLMM can gate whether a toggle of a pad is transferred to the wake up
controller (PDC/MPM) while the system is in a low power mode. The pinctrl
driver arms this for pins it requests as GPIO interrupts, but that path
takes over the pin mux, so it cannot be used for a pin that has to stay
muxed to a peripheral function.

Such a pin exists on boards with a SoundWire codec: while the bus is clock
stopped the codec requests a wake up by toggling the SoundWire data line,
which is muxed to the SoundWire function and not to GPIO. Unless the pad is
armed, the toggle never reaches the wake up controller, the controller is
never resumed and the event - a headset insertion, for instance - is lost
until something else happens to wake the bus.

Add a pin configuration property so such a pin can be armed from its own
pin configuration, without being turned into a GPIO.

Assisted-by: Claude Opus 5
Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
---
 .../devicetree/bindings/pinctrl/qcom,tlmm-common.yaml        | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
index aec72e8c0..95f539611 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
@@ -87,6 +87,18 @@ $defs:
       output-high: true
       output-low: true
 
+      wakeup-enable:
+        type: boolean
+        description:
+          Allow a toggle of the specified pins to be transferred to the wake up
+          controller while the system is in a low power mode. The driver arms
+          this on its own for pins requested as GPIO interrupts, so it is only
+          needed for pins that stay muxed to a peripheral function and are
+          toggled by an external device to request a wake up, such as a
+          SoundWire data line driven by a codec while the bus is clock stopped.
+          Only supported on pins that are marked as wakeup capable by the
+          hardware.
+
     allOf:
       - $ref: pincfg-node.yaml#
       - $ref: pinmux-node.yaml#

-- 
2.34.1

[PATCH v1 2/2] pinctrl: qcom: Allow arming the wakeup_enable bit from pin config

[Mohammad Rafi Shaik]

A pin's wakeup_enable bit controls whether a toggle of the pad is
transferred to the PDC/MPM HW, which is what both wakes the system from a
low power mode and, as the existing comment in msm_gpio_irq_reqres() notes,
delivers the interrupt event. The driver sets it when a pin is requested as
a GPIO interrupt and clears it when that interrupt is released.

That is the only way the bit can be armed today, and it is not usable for
every pin that needs it, because msm_gpio_irq_reqres() calls
msm_pinmux_request_gpio() and so takes over the pin mux.

A pin that must keep a peripheral function and is still toggled by an
external device to request a wake up therefore has no way to get the bit
armed. On boards with a SoundWire codec this loses interrupts: while the
bus is clock stopped the codec asks to be read out by toggling the
SoundWire data line, which stays muxed to the SoundWire function. With the
pad not armed the toggle is dropped, the controller is not resumed, and the
codec's interrupt - a headset insertion, for example - is only seen if
something else wakes the bus first.

Add a "wakeup-enable" pin configuration parameter so the bit can be armed
from the pin's own configuration, and fold the existing open coded accesses
in msm_gpio_irq_reqres() and msm_gpio_irq_relres() into the helper it
shares. Pins the SoC does not describe as wakeup capable are rejected
rather than silently ignored.

Assisted-by: Claude Opus 5
Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 101 +++++++++++++++++++++++++++----------
 1 file changed, 73 insertions(+), 28 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 27f06bbc1..cb12f8b90 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -286,6 +286,54 @@ static const struct pinmux_ops msm_pinmux_ops = {
 	.strict			= true,
 };
 
+/*
+ * "wakeup-enable" arms the wakeup_enable bit of a pin from its pin
+ * configuration. The driver already does this for pins requested as GPIO
+ * interrupts, but that path takes over the pin mux. A pin that has to stay
+ * muxed to a peripheral function and is toggled by an external device to
+ * request a wake up needs the bit armed without being turned into a GPIO.
+ */
+#define MSM_PINCONF_WAKEUP_ENABLE	(PIN_CONFIG_END + 1)
+
+static const struct pinconf_generic_params msm_gpio_bindings[] = {
+	{ "wakeup-enable", MSM_PINCONF_WAKEUP_ENABLE, 1 },
+};
+
+/*
+ * Set or clear the wakeup_enable bit of @g, which allows a pad event to be
+ * transferred to the PDC/MPM HW. While the name implies only the wakeup event,
+ * it's also required for the interrupt event.
+ *
+ * Returns -EOPNOTSUPP if the SoC does not describe the bit or the pin is not
+ * marked as wakeup capable.
+ */
+static int msm_pin_set_wakeup_enable(struct msm_pinctrl *pctrl,
+				     const struct msm_pingroup *g, bool enable)
+{
+	unsigned long flags;
+	int ret = -EOPNOTSUPP;
+	u32 intr_cfg;
+
+	if (!g->intr_wakeup_present_bit)
+		return ret;
+
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
+
+	intr_cfg = msm_readl_intr_cfg(pctrl, g);
+	if (intr_cfg & BIT(g->intr_wakeup_present_bit)) {
+		if (enable)
+			intr_cfg |= BIT(g->intr_wakeup_enable_bit);
+		else
+			intr_cfg &= ~BIT(g->intr_wakeup_enable_bit);
+		msm_writel_intr_cfg(intr_cfg, pctrl, g);
+		ret = 0;
+	}
+
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	return ret;
+}
+
 static int msm_config_reg(struct msm_pinctrl *pctrl,
 			  const struct msm_pingroup *g,
 			  unsigned param,
@@ -354,6 +402,16 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev,
 
 	g = &pctrl->soc->groups[group];
 
+	if (param == MSM_PINCONF_WAKEUP_ENABLE) {
+		if (!g->intr_wakeup_present_bit)
+			return -EOPNOTSUPP;
+
+		arg = msm_readl_intr_cfg(pctrl, g) & BIT(g->intr_wakeup_enable_bit);
+		*config = pinconf_to_config_packed(param, !!arg);
+
+		return 0;
+	}
+
 	ret = msm_config_reg(pctrl, g, param, &mask, &bit);
 	if (ret < 0)
 		return ret;
@@ -443,6 +501,17 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
 		param = pinconf_to_config_param(configs[i]);
 		arg = pinconf_to_config_argument(configs[i]);
 
+		if (param == MSM_PINCONF_WAKEUP_ENABLE) {
+			ret = msm_pin_set_wakeup_enable(pctrl, g, arg);
+			if (ret < 0) {
+				dev_err(pctrl->dev, "pin %u is not wakeup capable\n",
+					group);
+				return ret;
+			}
+
+			continue;
+		}
+
 		ret = msm_config_reg(pctrl, g, param, &mask, &bit);
 		if (ret < 0)
 			return ret;
@@ -1221,7 +1290,6 @@ static int msm_gpio_irq_reqres(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 	const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
-	unsigned long flags;
 	int ret;
 
 	if (!try_module_get(gc->owner))
@@ -1255,19 +1323,7 @@ static int msm_gpio_irq_reqres(struct irq_data *d)
 	 * While the name implies only the wakeup event, it's also required for
 	 * the interrupt event.
 	 */
-	if (g->intr_wakeup_present_bit) {
-		u32 intr_cfg;
-
-		raw_spin_lock_irqsave(&pctrl->lock, flags);
-
-		intr_cfg = msm_readl_intr_cfg(pctrl, g);
-		if (intr_cfg & BIT(g->intr_wakeup_present_bit)) {
-			intr_cfg |= BIT(g->intr_wakeup_enable_bit);
-			msm_writel_intr_cfg(intr_cfg, pctrl, g);
-		}
-
-		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
-	}
+	msm_pin_set_wakeup_enable(pctrl, g, true);
 
 	return 0;
 out:
@@ -1280,22 +1336,9 @@ static void msm_gpio_irq_relres(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 	const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
-	unsigned long flags;
 
 	/* Disable the wakeup_enable bit if it has been set in msm_gpio_irq_reqres() */
-	if (g->intr_wakeup_present_bit) {
-		u32 intr_cfg;
-
-		raw_spin_lock_irqsave(&pctrl->lock, flags);
-
-		intr_cfg = msm_readl_intr_cfg(pctrl, g);
-		if (intr_cfg & BIT(g->intr_wakeup_present_bit)) {
-			intr_cfg &= ~BIT(g->intr_wakeup_enable_bit);
-			msm_writel_intr_cfg(intr_cfg, pctrl, g);
-		}
-
-		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
-	}
+	msm_pin_set_wakeup_enable(pctrl, g, false);
 
 	gpiochip_unlock_as_irq(gc, d->hwirq);
 	module_put(gc->owner);
@@ -1639,6 +1682,8 @@ int msm_pinctrl_probe(struct platform_device *pdev,
 	pctrl->desc.name = dev_name(&pdev->dev);
 	pctrl->desc.pins = pctrl->soc->pins;
 	pctrl->desc.npins = pctrl->soc->npins;
+	pctrl->desc.num_custom_params = ARRAY_SIZE(msm_gpio_bindings);
+	pctrl->desc.custom_params = msm_gpio_bindings;
 
 	ret = devm_pinctrl_register_and_init(&pdev->dev, &pctrl->desc,
 					     pctrl, &pctrl->pctrl);

-- 
2.34.1

Re: [PATCH v1 2/2] pinctrl: qcom: Allow arming the wakeup_enable bit from pin config

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] pinctrl: qcom: missing hardware capability check in wakeup-enable get()
--

commit b3082e25a0d303b051061eac8a2191e7673c4106
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

pinctrl: qcom: Allow arming the wakeup_enable bit from pin config

This commit adds a "wakeup-enable" pin configuration parameter to arm the
wakeup_enable bit without taking over the pin mux. It folds open-coded
accesses into a helper shared between GPIO IRQ requests and pin config. Pins
not described as wakeup-capable by the SoC are rejected.

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 27f06bbc113c9..cb12f8b906007 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
[ ... ]
> @@ -354,6 +402,16 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev,
>  
>  	g = &pctrl->soc->groups[group];
>  
> +	if (param == MSM_PINCONF_WAKEUP_ENABLE) {
> +		if (!g->intr_wakeup_present_bit)
> +			return -EOPNOTSUPP;
> +
> +		arg = msm_readl_intr_cfg(pctrl, g) & BIT(g->intr_wakeup_enable_bit);
> +		*config = pinconf_to_config_packed(param, !!arg);

[Severity: Low]
Does this code miss a hardware capability check before returning the
configuration?

In the setter msm_pin_set_wakeup_enable(), the code correctly verifies that
the specific pin has the capability by checking the dynamic hardware presence
bit:

    intr_cfg = msm_readl_intr_cfg(pctrl, g);
    if (intr_cfg & BIT(g->intr_wakeup_present_bit)) {

However, this getter msm_config_group_get() only checks the static bit offset
g->intr_wakeup_present_bit from the SoC struct and fails to read the register
to check the actual dynamic hardware capability.

Can querying wakeup-enable on a pin that is dynamically not wakeup-capable
succeed and return unverified, meaningless register state from the enable_bit
offset, exposing inconsistent capabilities to userspace?

> +
> +		return 0;
> +	}
> +
>  	ret = msm_config_reg(pctrl, g, param, &mask, &bit);
>  	if (ret < 0)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-tlmm_wakeup_gpio_support-v1-0-c6d6f37a7e51@oss.qualcomm.com?part=2

Re: [PATCH v1 1/2] dt-bindings: pinctrl: qcom,tlmm-common: Add wakeup-enable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dt-bindings: custom property wakeup-enable lacks vendor prefix
--

commit 1b3617dbc4a7e1c4b006d9d99d89f2ff25a551bc
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

dt-bindings: pinctrl: qcom,tlmm-common: Add wakeup-enable

The TLMM can gate whether a toggle of a pad is transferred to the wake up
controller (PDC/MPM) while the system is in a low power mode. This commit
adds a pin configuration property so a pin can be armed from its own pin
configuration without being turned into a GPIO.

> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
> index aec72e8c06214..95f5396116916 100644
> --- a/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
> @@ -87,6 +87,18 @@ $defs:
>        output-high: true
>        output-low: true
>  
> +      wakeup-enable:

[Severity: Low]
Does this hardware-specific property need a vendor prefix?

Since this controls Qualcomm's specific interaction with the PDC/MPM wakeup
controllers, defining it without a qcom, prefix risks namespace collisions
if the pinctrl subsystem later standardizes a generic wakeup-enable property
with different constraints.

If this is intended to be a generic capability, shouldn't it be added to the
core pincfg-node.yaml bindings instead?

> +        type: boolean
> +        description:
> +          Allow a toggle of the specified pins to be transferred to the wake up
> +          controller while the system is in a low power mode. The driver arms
> +          this on its own for pins requested as GPIO interrupts, so it is only
> +          needed for pins that stay muxed to a peripheral function and are
> +          toggled by an external device to request a wake up, such as a
> +          SoundWire data line driven by a codec while the bus is clock stopped.
> +          Only supported on pins that are marked as wakeup capable by the
> +          hardware.
> +
>      allOf:
>        - $ref: pincfg-node.yaml#
>        - $ref: pinmux-node.yaml#

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-tlmm_wakeup_gpio_support-v1-0-c6d6f37a7e51@oss.qualcomm.com?part=1