[2/2] firmware/psci: Set pm_set_resume/suspend_via_firmware() on qcom

  Most Qualcomm platforms implementing PSCI (ab)use CPU_SUSPEND for
entering various stages of suspend, across the SoC. These range from a
simple WFI to a full-fledged power collapse of the entire chip
(mostly, anyway).

Some device drivers are curious to know whether "the firmware" (which is
often assumed to be ACPI) takes care of suspending or resuming the
platform. Set the flag that reports this behavior on the aforementioned
chips.

Some newer Qualcomm chips ship with firmware that actually advertises
PSCI SYSTEM_SUSPEND, so the compatible list should only grow slightly.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/firmware/psci/psci.c | 57 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)
  

On Wed, Dec 27, 2023 at 11:15:31PM +0100, Konrad Dybcio wrote:
> Most Qualcomm platforms implementing PSCI (ab)use CPU_SUSPEND for
> entering various stages of suspend, across the SoC. These range from a
> simple WFI to a full-fledged power collapse of the entire chip
> (mostly, anyway).
> 
> Some device drivers are curious to know whether "the firmware" (which is
> often assumed to be ACPI) takes care of suspending or resuming the
> platform. Set the flag that reports this behavior on the aforementioned
> chips.
> 
> Some newer Qualcomm chips ship with firmware that actually advertises
> PSCI SYSTEM_SUSPEND, so the compatible list should only grow slightly.
>

NACK, just use suspend-to-idle if SYSTEM_SUSPEND is not advertised. It is
designed for such platforms especially on x86/ACPI which don't advertise
Sx states. I see no reason why that doesn't work on ARM platforms as well.
  

On 28.12.2023 11:28, Sudeep Holla wrote:
> On Wed, Dec 27, 2023 at 11:15:31PM +0100, Konrad Dybcio wrote:
>> Most Qualcomm platforms implementing PSCI (ab)use CPU_SUSPEND for
>> entering various stages of suspend, across the SoC. These range from a
>> simple WFI to a full-fledged power collapse of the entire chip
>> (mostly, anyway).
>>
>> Some device drivers are curious to know whether "the firmware" (which is
>> often assumed to be ACPI) takes care of suspending or resuming the
>> platform. Set the flag that reports this behavior on the aforementioned
>> chips.
>>
>> Some newer Qualcomm chips ship with firmware that actually advertises
>> PSCI SYSTEM_SUSPEND, so the compatible list should only grow slightly.
>>
> 
> NACK, just use suspend-to-idle if SYSTEM_SUSPEND is not advertised. It is
> designed for such platforms especially on x86/ACPI which don't advertise
> Sx states. I see no reason why that doesn't work on ARM platforms as well.
Not sure if I got the message through well, but the bottom line is, on
Qualcomm platforms the "idle" states aren't actually just "idle" (read:
they're not like S0ix). All but the most shallow ones shut down quite a
chunk of the entire SoC, with the lowest ones being essentially S3 with
power being cut off from the entire chip, except for the memory rail.

Konrad
  

On Thu, Dec 28, 2023 at 12:47:51PM +0100, Konrad Dybcio wrote:
> On 28.12.2023 11:28, Sudeep Holla wrote:
> > On Wed, Dec 27, 2023 at 11:15:31PM +0100, Konrad Dybcio wrote:
> >> Most Qualcomm platforms implementing PSCI (ab)use CPU_SUSPEND for
> >> entering various stages of suspend, across the SoC. These range from a
> >> simple WFI to a full-fledged power collapse of the entire chip
> >> (mostly, anyway).
> >>
> >> Some device drivers are curious to know whether "the firmware" (which is
> >> often assumed to be ACPI) takes care of suspending or resuming the
> >> platform. Set the flag that reports this behavior on the aforementioned
> >> chips.
> >>
> >> Some newer Qualcomm chips ship with firmware that actually advertises
> >> PSCI SYSTEM_SUSPEND, so the compatible list should only grow slightly.
> >>
> > 
> > NACK, just use suspend-to-idle if SYSTEM_SUSPEND is not advertised. It is
> > designed for such platforms especially on x86/ACPI which don't advertise
> > Sx states. I see no reason why that doesn't work on ARM platforms as well.
>
> Not sure if I got the message through well, but the bottom line is, on
> Qualcomm platforms the "idle" states aren't actually just "idle" (read:
> they're not like S0ix). All but the most shallow ones shut down quite a
> chunk of the entire SoC, with the lowest ones being essentially S3 with
> power being cut off from the entire chip, except for the memory rail.
>

No I understood that and S2I is exactly what you need.
Have you checked if S2I already works as intended on these platforms ?
What extra do you achieve with this hack by advertising fake S2R ?
S2I will have less latency compared to S2R and the mem_sleep will be
automatically set to S2I if S2R is not supported, so no userspace impact
as well.

So please let us know what this change provide extra over S2I ? Until
then my NACK still stands.

--
Regards,
Sudeep
  

On 28.12.2023 12:50, Sudeep Holla wrote:
> On Thu, Dec 28, 2023 at 12:47:51PM +0100, Konrad Dybcio wrote:
>> On 28.12.2023 11:28, Sudeep Holla wrote:
>>> On Wed, Dec 27, 2023 at 11:15:31PM +0100, Konrad Dybcio wrote:
>>>> Most Qualcomm platforms implementing PSCI (ab)use CPU_SUSPEND for
>>>> entering various stages of suspend, across the SoC. These range from a
>>>> simple WFI to a full-fledged power collapse of the entire chip
>>>> (mostly, anyway).
>>>>
>>>> Some device drivers are curious to know whether "the firmware" (which is
>>>> often assumed to be ACPI) takes care of suspending or resuming the
>>>> platform. Set the flag that reports this behavior on the aforementioned
>>>> chips.
>>>>
>>>> Some newer Qualcomm chips ship with firmware that actually advertises
>>>> PSCI SYSTEM_SUSPEND, so the compatible list should only grow slightly.
>>>>
>>>
>>> NACK, just use suspend-to-idle if SYSTEM_SUSPEND is not advertised. It is
>>> designed for such platforms especially on x86/ACPI which don't advertise
>>> Sx states. I see no reason why that doesn't work on ARM platforms as well.
>>
>> Not sure if I got the message through well, but the bottom line is, on
>> Qualcomm platforms the "idle" states aren't actually just "idle" (read:
>> they're not like S0ix). All but the most shallow ones shut down quite a
>> chunk of the entire SoC, with the lowest ones being essentially S3 with
>> power being cut off from the entire chip, except for the memory rail.
>>
> 
> No I understood that and S2I is exactly what you need.
> Have you checked if S2I already works as intended on these platforms ?
Yes, simple CPU idling works OOTB and the SoC power collapse only works
given the developer doesn't cut corners when bringing up the platform
(read: works on some of the ones we support, trying hard to expand that
group!)

> What extra do you achieve with this hack by advertising fake S2R ?
Uh.. unless I misunderstood something, I'm not advertising s2ram..
Quite the opposite, I'm making sure only s2idle is allowed.

Admittedly, my mistake, I managed to misname the function which I
didn't spot before wrapping this patch up for sending..

> S2I will have less latency compared to S2R and the mem_sleep will be
> automatically set to S2I if S2R is not supported, so no userspace impact
> as well.
Yes, everything that differs them is abstracted in TZ or through the
power management coprocessor.

> 
> So please let us know what this change provide extra over S2I ? Until
> then my NACK still stands.
The main point here is to advertise pm_resume/suspend_via_firmware,
which represents that Qualcomm does a lot of not-very-obvious power
wire plumbing within their PSCI impl.

Konrad
  

On Thu, Dec 28, 2023 at 01:16:28PM +0100, Konrad Dybcio wrote:
> On 28.12.2023 12:50, Sudeep Holla wrote:
> > On Thu, Dec 28, 2023 at 12:47:51PM +0100, Konrad Dybcio wrote:
> >> On 28.12.2023 11:28, Sudeep Holla wrote:
> >>> On Wed, Dec 27, 2023 at 11:15:31PM +0100, Konrad Dybcio wrote:
> >>>> Most Qualcomm platforms implementing PSCI (ab)use CPU_SUSPEND for
> >>>> entering various stages of suspend, across the SoC. These range from a
> >>>> simple WFI to a full-fledged power collapse of the entire chip
> >>>> (mostly, anyway).
> >>>>
> >>>> Some device drivers are curious to know whether "the firmware" (which is
> >>>> often assumed to be ACPI) takes care of suspending or resuming the
> >>>> platform. Set the flag that reports this behavior on the aforementioned
> >>>> chips.
> >>>>
> >>>> Some newer Qualcomm chips ship with firmware that actually advertises
> >>>> PSCI SYSTEM_SUSPEND, so the compatible list should only grow slightly.
> >>>>
> >>>
> >>> NACK, just use suspend-to-idle if SYSTEM_SUSPEND is not advertised. It is
> >>> designed for such platforms especially on x86/ACPI which don't advertise
> >>> Sx states. I see no reason why that doesn't work on ARM platforms as well.
> >>
> >> Not sure if I got the message through well, but the bottom line is, on
> >> Qualcomm platforms the "idle" states aren't actually just "idle" (read:
> >> they're not like S0ix). All but the most shallow ones shut down quite a
> >> chunk of the entire SoC, with the lowest ones being essentially S3 with
> >> power being cut off from the entire chip, except for the memory rail.
> >>
> > 
> > No I understood that and S2I is exactly what you need.
> > Have you checked if S2I already works as intended on these platforms ?
> Yes, simple CPU idling works OOTB and the SoC power collapse only works
> given the developer doesn't cut corners when bringing up the platform
> (read: works on some of the ones we support, trying hard to expand that
> group!)
> 
> > What extra do you achieve with this hack by advertising fake S2R ?
> Uh.. unless I misunderstood something, I'm not advertising s2ram..
> Quite the opposite, I'm making sure only s2idle is allowed.
>

Right, I didn't notice that in suspend_valid_all(), it can be rename
suspend_valid_s2i or something. "All" indicates all state is valid.

Anyways that is not the main point. IIUC S2I must still work if there is
at-least one CPU idle state other than WFI without these changes. Right ?

If all these changes is to support S2I wih WFI only, then we can look at
some generic solution as there were previous attempts to do something
similar on other platforms IIRC.
  

On 28.12.2023 13:43, Sudeep Holla wrote:
> On Thu, Dec 28, 2023 at 01:16:28PM +0100, Konrad Dybcio wrote:
>> On 28.12.2023 12:50, Sudeep Holla wrote:
>>> On Thu, Dec 28, 2023 at 12:47:51PM +0100, Konrad Dybcio wrote:
>>>> On 28.12.2023 11:28, Sudeep Holla wrote:
>>>>> On Wed, Dec 27, 2023 at 11:15:31PM +0100, Konrad Dybcio wrote:
>>>>>> Most Qualcomm platforms implementing PSCI (ab)use CPU_SUSPEND for
>>>>>> entering various stages of suspend, across the SoC. These range from a
>>>>>> simple WFI to a full-fledged power collapse of the entire chip
>>>>>> (mostly, anyway).
>>>>>>
>>>>>> Some device drivers are curious to know whether "the firmware" (which is
>>>>>> often assumed to be ACPI) takes care of suspending or resuming the
>>>>>> platform. Set the flag that reports this behavior on the aforementioned
>>>>>> chips.
>>>>>>
>>>>>> Some newer Qualcomm chips ship with firmware that actually advertises
>>>>>> PSCI SYSTEM_SUSPEND, so the compatible list should only grow slightly.
>>>>>>
>>>>>
>>>>> NACK, just use suspend-to-idle if SYSTEM_SUSPEND is not advertised. It is
>>>>> designed for such platforms especially on x86/ACPI which don't advertise
>>>>> Sx states. I see no reason why that doesn't work on ARM platforms as well.
>>>>
>>>> Not sure if I got the message through well, but the bottom line is, on
>>>> Qualcomm platforms the "idle" states aren't actually just "idle" (read:
>>>> they're not like S0ix). All but the most shallow ones shut down quite a
>>>> chunk of the entire SoC, with the lowest ones being essentially S3 with
>>>> power being cut off from the entire chip, except for the memory rail.
>>>>
>>>
>>> No I understood that and S2I is exactly what you need.
>>> Have you checked if S2I already works as intended on these platforms ?
>> Yes, simple CPU idling works OOTB and the SoC power collapse only works
>> given the developer doesn't cut corners when bringing up the platform
>> (read: works on some of the ones we support, trying hard to expand that
>> group!)
>>
>>> What extra do you achieve with this hack by advertising fake S2R ?
>> Uh.. unless I misunderstood something, I'm not advertising s2ram..
>> Quite the opposite, I'm making sure only s2idle is allowed.
>>
> 
> Right, I didn't notice that in suspend_valid_all(), it can be rename
> suspend_valid_s2i or something. "All" indicates all state is valid.
> 
> Anyways that is not the main point. IIUC S2I must still work if there is
> at-least one CPU idle state other than WFI without these changes. Right ?

Yes, and it does

> 
> If all these changes is to support S2I wih WFI only, then we can look at
> some generic solution as there were previous attempts to do something
> similar on other platforms IIRC.

No, I'm just interested in setting the resume/suspend_via_firmware()
markers and the PSCI driver seems like a good place for it, be it on
all platforms with SYSTEM_SUSPEND and Qualcomm which sneaked equivalent
functionality into the CPU_SUSPEND call.

Konrad
  

On Tue, Jan 02, 2024 at 07:17:50PM +0100, Konrad Dybcio wrote:
> On 28.12.2023 13:43, Sudeep Holla wrote:
> > On Thu, Dec 28, 2023 at 01:16:28PM +0100, Konrad Dybcio wrote:
> >> On 28.12.2023 12:50, Sudeep Holla wrote:
> >>> On Thu, Dec 28, 2023 at 12:47:51PM +0100, Konrad Dybcio wrote:
> >>>> On 28.12.2023 11:28, Sudeep Holla wrote:
> >>>>> On Wed, Dec 27, 2023 at 11:15:31PM +0100, Konrad Dybcio wrote:
> >>>>>> Most Qualcomm platforms implementing PSCI (ab)use CPU_SUSPEND for
> >>>>>> entering various stages of suspend, across the SoC. These range from a
> >>>>>> simple WFI to a full-fledged power collapse of the entire chip
> >>>>>> (mostly, anyway).
> >>>>>>
> >>>>>> Some device drivers are curious to know whether "the firmware" (which is
> >>>>>> often assumed to be ACPI) takes care of suspending or resuming the
> >>>>>> platform. Set the flag that reports this behavior on the aforementioned
> >>>>>> chips.
> >>>>>>
> >>>>>> Some newer Qualcomm chips ship with firmware that actually advertises
> >>>>>> PSCI SYSTEM_SUSPEND, so the compatible list should only grow slightly.
> >>>>>>
> >>>>>
> >>>>> NACK, just use suspend-to-idle if SYSTEM_SUSPEND is not advertised. It is
> >>>>> designed for such platforms especially on x86/ACPI which don't advertise
> >>>>> Sx states. I see no reason why that doesn't work on ARM platforms as well.
> >>>>
> >>>> Not sure if I got the message through well, but the bottom line is, on
> >>>> Qualcomm platforms the "idle" states aren't actually just "idle" (read:
> >>>> they're not like S0ix). All but the most shallow ones shut down quite a
> >>>> chunk of the entire SoC, with the lowest ones being essentially S3 with
> >>>> power being cut off from the entire chip, except for the memory rail.
> >>>>
> >>>
> >>> No I understood that and S2I is exactly what you need.
> >>> Have you checked if S2I already works as intended on these platforms ?
> >> Yes, simple CPU idling works OOTB and the SoC power collapse only works
> >> given the developer doesn't cut corners when bringing up the platform
> >> (read: works on some of the ones we support, trying hard to expand that
> >> group!)
> >>
> >>> What extra do you achieve with this hack by advertising fake S2R ?
> >> Uh.. unless I misunderstood something, I'm not advertising s2ram..
> >> Quite the opposite, I'm making sure only s2idle is allowed.
> >>
> >
> > Right, I didn't notice that in suspend_valid_all(), it can be rename
> > suspend_valid_s2i or something. "All" indicates all state is valid.
> >
> > Anyways that is not the main point. IIUC S2I must still work if there is
> > at-least one CPU idle state other than WFI without these changes. Right ?
>
> Yes, and it does
>

Thanks for the confirmation.

> >
> > If all these changes is to support S2I wih WFI only, then we can look at
> > some generic solution as there were previous attempts to do something
> > similar on other platforms IIRC.
>
> No, I'm just interested in setting the resume/suspend_via_firmware()
> markers and the PSCI driver seems like a good place for it, be it on

Agreed and your PATCH 1/2 does that exactly and hence I have acked it
already.

> all platforms with SYSTEM_SUSPEND and Qualcomm which sneaked equivalent
> functionality into the CPU_SUSPEND call.
>

But I don't like the Qualcomm specific changes.

--
Regards,
Sudeep
  

On 3.01.2024 10:44, Sudeep Holla wrote:
> On Tue, Jan 02, 2024 at 07:17:50PM +0100, Konrad Dybcio wrote:
>> On 28.12.2023 13:43, Sudeep Holla wrote:
>>> On Thu, Dec 28, 2023 at 01:16:28PM +0100, Konrad Dybcio wrote:
>>>> On 28.12.2023 12:50, Sudeep Holla wrote:
>>>>> On Thu, Dec 28, 2023 at 12:47:51PM +0100, Konrad Dybcio wrote:
>>>>>> On 28.12.2023 11:28, Sudeep Holla wrote:
>>>>>>> On Wed, Dec 27, 2023 at 11:15:31PM +0100, Konrad Dybcio wrote:
>>>>>>>> Most Qualcomm platforms implementing PSCI (ab)use CPU_SUSPEND for
>>>>>>>> entering various stages of suspend, across the SoC. These range from a
>>>>>>>> simple WFI to a full-fledged power collapse of the entire chip
>>>>>>>> (mostly, anyway).
>>>>>>>>
>>>>>>>> Some device drivers are curious to know whether "the firmware" (which is
>>>>>>>> often assumed to be ACPI) takes care of suspending or resuming the
>>>>>>>> platform. Set the flag that reports this behavior on the aforementioned
>>>>>>>> chips.
>>>>>>>>
>>>>>>>> Some newer Qualcomm chips ship with firmware that actually advertises
>>>>>>>> PSCI SYSTEM_SUSPEND, so the compatible list should only grow slightly.
>>>>>>>>
>>>>>>>
>>>>>>> NACK, just use suspend-to-idle if SYSTEM_SUSPEND is not advertised. It is
>>>>>>> designed for such platforms especially on x86/ACPI which don't advertise
>>>>>>> Sx states. I see no reason why that doesn't work on ARM platforms as well.
>>>>>>
>>>>>> Not sure if I got the message through well, but the bottom line is, on
>>>>>> Qualcomm platforms the "idle" states aren't actually just "idle" (read:
>>>>>> they're not like S0ix). All but the most shallow ones shut down quite a
>>>>>> chunk of the entire SoC, with the lowest ones being essentially S3 with
>>>>>> power being cut off from the entire chip, except for the memory rail.
>>>>>>
>>>>>
>>>>> No I understood that and S2I is exactly what you need.
>>>>> Have you checked if S2I already works as intended on these platforms ?
>>>> Yes, simple CPU idling works OOTB and the SoC power collapse only works
>>>> given the developer doesn't cut corners when bringing up the platform
>>>> (read: works on some of the ones we support, trying hard to expand that
>>>> group!)
>>>>
>>>>> What extra do you achieve with this hack by advertising fake S2R ?
>>>> Uh.. unless I misunderstood something, I'm not advertising s2ram..
>>>> Quite the opposite, I'm making sure only s2idle is allowed.
>>>>
>>>
>>> Right, I didn't notice that in suspend_valid_all(), it can be rename
>>> suspend_valid_s2i or something. "All" indicates all state is valid.
>>>
>>> Anyways that is not the main point. IIUC S2I must still work if there is
>>> at-least one CPU idle state other than WFI without these changes. Right ?
>>
>> Yes, and it does
>>
> 
> Thanks for the confirmation.
> 
>>>
>>> If all these changes is to support S2I wih WFI only, then we can look at
>>> some generic solution as there were previous attempts to do something
>>> similar on other platforms IIRC.
>>
>> No, I'm just interested in setting the resume/suspend_via_firmware()
>> markers and the PSCI driver seems like a good place for it, be it on
> 
> Agreed and your PATCH 1/2 does that exactly and hence I have acked it
> already.
> 
>> all platforms with SYSTEM_SUSPEND and Qualcomm which sneaked equivalent
>> functionality into the CPU_SUSPEND call.
>>
> 
> But I don't like the Qualcomm specific changes.

Is that because of the matching table, or due to the slightly more
convoluted way of suspending the platform through CPU_SUSPEND?

If the former, I can think of other solutions. If the latter, I'm
open to keep convincing you :D There are probably things I skipped
over when explaining how it's wired up..

Konrad
  

On Mon, Jan 08, 2024 at 04:47:05PM +0100, Konrad Dybcio wrote:
> On 3.01.2024 10:44, Sudeep Holla wrote:
> >
> > But I don't like the Qualcomm specific changes.
>
> Is that because of the matching table, or due to the slightly more
> convoluted way of suspending the platform through CPU_SUSPEND?
>

I would say both. I don't like this to be Qualcomm platform specific
feature. Also advertising absence of system suspend on those platforms
as presence of some special system suspend. It simply is not system
suspend. The sysfs hides/abstracts and provides s2idle even when user
request s2r on such platforms.

We should advertise it as s2idle. I would do something like below patch,
just a rough idea, not compiled or tested. This avoids any misleading
or confusion IMO.

However I am interested in knowing which are these drivers that rely on
the pm_suspend_global_flags ? The reason I ask the x86 ACPI doesn't set
the flags for s2idle. Also the core code explicitly calls
pm_set_suspend_no_platform() in suspend_devices_and_enter(). What you
want conflicts with both the above observations. I would like to involve
Rafael and check what is the correct/expected way to use those flags.

Regards,
Sudeep

--->8
diff --git i/drivers/firmware/psci/psci.c w/drivers/firmware/psci/psci.c
index 0e622aa5ad58..b2559ae7668a 100644
--- i/drivers/firmware/psci/psci.c
+++ w/drivers/firmware/psci/psci.c
@@ -505,26 +505,42 @@ static int psci_system_suspend(unsigned long unused)
        return psci_to_linux_errno(err);
 }

-static int psci_system_suspend_enter(suspend_state_t state)
+static int psci_system_idle_prepare_late(void)
 {
        pm_set_resume_via_firmware();
+       return 0;
+}
+#define psci_system_system_prepare_late        psci_system_idle_prepare_late
+
+static int psci_system_suspend_enter(suspend_state_t state)
+{
+       psci_system_system_prepare_late();

        return cpu_suspend(0, psci_system_suspend);
 }

-static int psci_system_suspend_begin(suspend_state_t state)
+static int psci_system_idle_begin(void)
 {
        pm_set_suspend_via_firmware();
-
        return 0;
 }

+static int psci_system_suspend_begin(suspend_state_t state)
+{
+       return psci_system_idle_begin();
+}
+
 static const struct platform_suspend_ops psci_suspend_ops = {
        .valid          = suspend_valid_only_mem,
        .enter          = psci_system_suspend_enter,
        .begin          = psci_system_suspend_begin,
 };

+static const struct platform_s2idle_ops psci_s2idle_ops = {
+       .begin = psci_system_idle_begin,
+       .prepare_late = psci_system_idle_prepare_late,
+};
+
 static void __init psci_init_system_reset2(void)
 {
        int ret;
@@ -546,6 +562,8 @@ static void __init psci_init_system_suspend(void)

        if (ret != PSCI_RET_NOT_SUPPORTED)
                suspend_set_ops(&psci_suspend_ops);
+
+       s2idle_set_ops(&psci_s2idle_ops);
 }

 static void __init psci_init_cpu_suspend(void)