Message-ID: <049171d7-d4ee-4499-8414-4b66b2c6e5b2@ti.com>
Date: Thu, 3 Jul 2025 14:31:10 -0500
From: Kendall Willis <k-willis@...com>
To: Ulf Hansson <ulf.hansson@...aro.org>
CC: <nm@...com>, <kristo@...nel.org>, <ssantosh@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <linux-pm@...r.kernel.org>, <d-gole@...com>, <vishalm@...com>,
        <sebin.francis@...com>, <msp@...libre.com>, <khilman@...libre.com>
Subject: Re: [PATCH 2/2] pmdomain: ti_sci: Add LPM abort sequence to suspend
 path

On 7/2/25 07:06, Ulf Hansson wrote:
> On Fri, 27 Jun 2025 at 22:49, Kendall Willis <k-willis@...com> wrote:
>>
>> Create ->suspend_late() and ->suspend_noirq() hooks to add abort sequence
>> to any driver within the PM domain with either of those hooks. If a driver
>> fails to suspend with those hooks, add a call to the DM to abort entering
>> the LPM. The suspend hooks of the PM domains driver inserts itself into
>> the suspend path of all devices connected to the TI SCI PM domain. So if
>> any device in the PM domain with either a ->suspend_late() or
>> ->suspend_noirq hook fails to suspend, the PM domain drivers suspend hook
>> will send the abort call to the DM.
>>
>> Adding an abort call in the ->suspend() hook is not necessary. TI SCI
>> suspend sends the message to the DM to prepare to enter a low power mode.
>> TI SCI suspend ALWAYS occurs after the ->suspend() hook for all TI SCI
>> devices has been called.
>>
>> Signed-off-by: Kendall Willis <k-willis@...com>
>> ---
>>   drivers/pmdomain/ti/ti_sci_pm_domains.c | 46 ++++++++++++++++++++++++-
>>   1 file changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
>> index 82df7e44250bb..975defc16271d 100644
>> --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
>> +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
>> @@ -2,7 +2,7 @@
>>   /*
>>    * TI SCI Generic Power Domain Driver
>>    *
>> - * Copyright (C) 2015-2017 Texas Instruments Incorporated - http://www.ti.com/
>> + * Copyright (C) 2015-2025 Texas Instruments Incorporated - http://www.ti.com/
>>    *     J Keerthy <j-keerthy@...com>
>>    *     Dave Gerlach <d-gerlach@...com>
>>    */
>> @@ -149,8 +149,47 @@ static int ti_sci_pd_suspend(struct device *dev)
>>
>>          return 0;
>>   }
>> +
>> +static int ti_sci_pd_suspend_late(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;
>> +       int ret;
>> +
>> +       ret = pm_generic_suspend_late(dev);
>> +       if (ret) {
>> +               dev_err(dev, "%s: Failed to suspend. Abort entering low power mode.\n", __func__);
>> +               if (ti_sci->ops.pm_ops.lpm_abort(ti_sci))
>> +                       dev_err(dev, "%s: Failed to abort.\n", __func__);
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int ti_sci_pd_suspend_noirq(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;
>> +       int ret;
>> +
>> +       ret = pm_generic_suspend_noirq(dev);
>> +       if (ret) {
>> +               dev_err(dev, "%s: Failed to suspend. Abort entering low power mode.\n", __func__);
>> +               if (ti_sci->ops.pm_ops.lpm_abort(ti_sci))
>> +                       dev_err(dev, "%s: Failed to abort.\n", __func__);
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   #else
>>   #define ti_sci_pd_suspend              NULL
>> +#define ti_sci_pd_suspend_late         NULL
>> +#define ti_sci_pd_suspend_noirq                NULL
>>   #endif
>>
>>   /*
>> @@ -264,6 +303,11 @@ static int ti_sci_pm_domain_probe(struct platform_device *pdev)
>>                                      pd_provider->ti_sci->ops.pm_ops.set_latency_constraint)
>>                                          pd->pd.domain.ops.suspend = ti_sci_pd_suspend;
>>
>> +                               if (pd_provider->ti_sci->ops.pm_ops.lpm_abort) {
>> +                                       pd->pd.domain.ops.suspend_late = ti_sci_pd_suspend_late;
>> +                                       pd->pd.domain.ops.suspend_noirq = ti_sci_pd_suspend_noirq;
> 
> This doesn't work as pm_genpd_init() is assigning the
> ->suspend_noirq() callback, hence overriding the callback.
> 
> Moreover I am not sure this is the correct thing to do, as having your
> own ->suspend_noirq() callback would mean that you will be by-passing
> genpd's internal reference counting to understand when it's fine to
> power-off the PM domain, vi a the ->power_off() callback.
> 
> Can the LPM abort be handled from a ->complete() callback instead?

Handling the abort from a ->complete() callback does look like a much 
better solution than adding the ->suspend_late() and ->suspend_noirq() 
callbacks. The only problem with it is that there doesn't seem to be a 
way to tell if system suspend failed in order to call the abort sequence.

However, the abort sequence just clears the LPM selection in the DM 
which the DM already does upon successful suspend/resume. Clearing it 
multiple times shouldn't be an issue, but I will have to look into this 
more.

Thanks,
Kendall

> 
>> +                               }
>> +
>>                                  pm_genpd_init(&pd->pd, NULL, true);
>>
>>                                  list_add(&pd->node, &pd_provider->pd_list);
>> --
>> 2.34.1
>>
> 
> Kind regards
> Uffe

Powered by blists - more mailing lists