Re: [PATCH 0/3] Allow specifying an S2RAM sleep on pre-SYSTEM_SUSPEND PSCI impls

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 20.12.2024 2:58 PM, Sudeep Holla wrote:
> On Fri, Dec 20, 2024 at 01:42:04PM +0100, Konrad Dybcio wrote:
>> On 20.12.2024 12:39 PM, Sudeep Holla wrote:
>>> On Thu, Dec 19, 2024 at 08:26:51PM +0100, Konrad Dybcio wrote:
>>>> On 14.11.2024 2:10 AM, Elliot Berman wrote:
>>>>
>>>>> I'm not sure why you'd like to support s2ram. Is it *only* that you'd
>>>>> like to be able to set pm_set_supend/resume_via_firmware()? I hope this
>>>>> doesn't sound silly: what if you register a platform_s2idle_ops for the
>>>>> relevant SoCs which calls pm_set_suspend/resume_via_firwmare()?
>>>>
>>>> S2RAM is what you get after entering a certain state, but currently
>>>> it's presented as just another (s2idle) idle state.
>>>>
>>>
>>> Just to be clear, I assume you mean CPU_SUSPEND idle state. There is
>>> no special or different s2idle idle states IIUC.
>>
>> Yeah, right.
>>
>>>> That means some hardware that may need to be reinitialized, isn't as
>>>> Linux has no clue it might have lost power.
>>>>
>>>
>>> Interesting, so this means firmware doesn't automatically save and restore
>>> states yet exposes it as CPU_SUSPEND idle state.
>>
>> Reading the spec, I'm pretty sure PSCI calls should only mess with the
>> power state of the cores, core-adjacent peripherals and GIC.
>>
>> Reading section 5.20.1 (SYSTEM_SUSPEND / Intended use) I think it says
>> mostly what I'm trying to convey:
>>
>>
>> "In a typical implementation, the semantics are equivalent to a
>> CPU_SUSPEND to the deepest low-power state. However, it is possible that
>> an implementation might reserve a deeper state for SYSTEM_SUSPEND than
>> those used with CPU_SUSPEND."
>>
> 
> Yes these text help to understand the interface easily. If they were same,
> do you think we would have defined 2 different interfaces.

I would happen to think that, yes. Especially since the reference firmware
implementation does *exactly this*:

https://github.com/ARM-software/arm-trusted-firmware/blob/master/lib/psci/psci_main.c#L179-L221

PSCI_SYSTEM_SUSPEND seems to be simply meant as a wrapper around a specific
CPU_SUSPEND state (which may or may not be only callable from inside the
firmware when SYSTEM_SUSPEND specifically is requested, for reasons),
in a platform-agnostic way, so that the OS can enter suspend without
providing that magic StateID on all supported platforms.
But since it already requires more elbow grease on the peripheral IP side,
I'm not really convinced it's that much useful.

Plus, the optional bit of doing more work behind the scenes doesn't seem
to be very wildly used across TF-A supported platforms.

So please, stop making the argument that it's any different. The firmware
I'm dealing with simply didn't expose the same thing twice, in perfect
accordance with the spec.

Konrad