Message-ID: <20250905183931.qfqnnvmwqqvo3emy@lcpd911>
Date: Sat, 6 Sep 2025 00:09:31 +0530
From: Dhruva Gole <d-gole@...com>
To: Kendall Willis <k-willis@...com>
CC: <nm@...com>, <kristo@...nel.org>, <ssantosh@...nel.org>,
        <ulf.hansson@...aro.org>, <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <vishalm@...com>,
        <sebin.francis@...com>, <msp@...libre.com>, <khilman@...libre.com>,
        <a-kaur@...com>
Subject: Re: [PATCH] pmdomain: ti_sci: Handle wakeup constraint if device has
 pinctrl wakeup state

On Sep 04, 2025 at 16:16:07 -0500, Kendall Willis wrote:
> In TI K3 SoCs the PM co-processor (device manager or DM) will decide
> which low power state to suspend into based off of constraints given by
> Linux. If a device is marked as a wakeup source in Linux, Linux will add
> a constraint that the wakeup source has to be on. The DM will enter the
> deepest low power state based off of the constraint.
> 
> In cases like UARTs, IO daisy-chaining can be used to wakeup the system,
> however if the UART is marked as a wakeup source, the system is not able
> to enter any low power mode.
> 
> IO daisy-chain wakeup can use the pinctrl wakeup state instead of using
> wake IRQs. For example, the serial driver on K3 platforms uses a wakeup

I think this statement will only make sense to someone who's worked on
io daisychain and wake IRQs on our SoCs.
I don't think it's coming out very clearly why wake IRQs are needed for
IO Daisychain in the first place.

You could probably just reference commit b06bc47279919 ("pmdomain:
ti_sci: handle wake IRQs for IO daisy chain wakeups") from Kevin where
we explain whats io daisychain and need for wake IRQ. Talk in this patch
with relation to that patch - what you're doing that the other one
missed.

> pinctrl state to be able to resume from suspend.

Perhaps I am missing the order in which the patches are being applied.
But if it's like the 1 -> 2 -> 3 order you mentioned below then this
reference to serial drv won't really make sense right?

If we're going in the order of applying the series that you
specified, there's no guarantee that the UART patch makes it in
along with this patch, so I'd say let's skip mentioning the serial
driver to avoid sending people in search for how the serial driver is
doing this atall.

In Kevin's earlier commit it made sense, because he was talking about
daisychaining, here we're not. So the 8250 driver doesn't actually have
the changes we're talking about in this patch.

> 
> Devices that are marked as a wakeup source and use pinctrl wakeup state
> should not set wakeup constraints since these can happen even from deepest
> low power state, so the DM should not be prevented from picking deep power
> states.
> 
> Detect the pinctrl wakeup state in the suspend path, and if it exists,
> skip sending the constraint.
> 
> Signed-off-by: Kendall Willis <k-willis@...com>
> ---
> This series is intended to be implemented along with the following
> series:
> 
> 1. "pmdomain: ti_sci: Handle wakeup constraint if device has pinctrl
>    wakeup state": (this patch) skips setting constraints for wakeup
>    sources that use pinctrl state 'wakeup'.
> 
> 2. "serial: 8250: omap: Add wakeup support": Implements wakeup from
>    the UARTs for TI K3 SoCs
> 
> 3. "arm64: dts: ti: k3-am62: Support Main UART wakeup": Implements the
>    functionality to wakeup the system from the Main UART
> 
> Testing
> -------
> Tested on a SK-AM62B-P1 board with all series and dependencies
> implemented. Suspend/resume verified with the Main UART wakeup source
> by entering a keypress on the console.
> 
> ---
>  drivers/pmdomain/ti/ti_sci_pm_domains.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
> index 82df7e44250bb..884905fd0686c 100644
> --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
> +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
> @@ -10,6 +10,7 @@
>  #include <linux/err.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
>  #include <linux/pm_qos.h>
> @@ -84,9 +85,24 @@ static inline void ti_sci_pd_set_wkup_constraint(struct device *dev)
>  	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>  	struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(genpd);
>  	const struct ti_sci_handle *ti_sci = pd->parent->ti_sci;
> +	struct pinctrl *pinctrl = devm_pinctrl_get(dev);
> +	struct pinctrl_state *pinctrl_state_wakeup;
>  	int ret;
>  
>  	if (device_may_wakeup(dev)) {
> +		/*
> +		 * If device can wakeup using pinctrl wakeup state,
> +		 * we do not want to set a constraint
> +		 */
> +		if (!IS_ERR_OR_NULL(pinctrl)) {
> +			pinctrl_state_wakeup = pinctrl_lookup_state(pinctrl, "wakeup");
> +			if (!IS_ERR_OR_NULL(pinctrl_state_wakeup)) {
> +				dev_dbg(dev, "%s: has wake pinctrl wakeup state, not setting " \
> +						"constraints\n", __func__);
> +				return;
> +			}
> +		}
> +

Not much objections to the code itself, as it makes sense that we want
to ignore the constraint for wakeup pinctrl similar to how we did for the
wake IRQ one.

-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated

Powered by blists - more mailing lists