Message-ID: <48ad21a2-6562-4568-aace-68d163116ddd@ti.com>
Date: Mon, 8 Sep 2025 15:19:26 -0500
From: Kendall Willis <k-willis@...com>
To: Dhruva Gole <d-gole@...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

Hi Dhruva,

On 9/5/25 13:39, Dhruva Gole wrote:
> 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.

In v2, I will add references to commit b06bc47279919 ("pmdomain: ti_sci: 
handle wake IRQs for IO daisy chain wakeups") and I will remove the 
reference to the UART driver.

Thanks,
Kendall

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

Powered by blists - more mailing lists