Re: [PATCH] firmware: psci: Set pm_set_resume/suspend_via_firmware() for SYSTEM_SUSPEND

[Bjorn Andersson <andersson@kernel.org>]

On Wed, Dec 31, 2025 at 09:51:26PM +0530, Manivannan Sadhasivam wrote:
> From: Konrad Dybcio <konradybcio@kernel.org>
> 
> PSCI specification defines the SYSTEM_SUSPEND feature which enables the
> firmware to implement the suspend to RAM (S2RAM) functionality by
> transitioning the system to a deeper low power state. When the system
> enters such state, the power to the peripheral devices might be removed.

What is the actual extent of "peripheral devices" in this definition?

> So
> the respective device drivers must prepare for the possible removal of the
> power by performing actions such as shutting down or resetting the device
> in their system suspend callbacks.
> 

Our typical interpretation of this state is that IP-blocks might be
non-functional during this time, but typically some state is retained.

Will the acceptance of this patch result in that we in the Qualcomm case
should start accepting/writing patches that implement save/restore of
state that is generally retained throughout the drivers - in the name of
"power might be removed"?

> The Linux PM framework allows the platform drivers to convey this info to
> device drivers by calling the pm_set_suspend_via_firmware() and
> pm_set_resume_via_firmware() APIs.
> 
> Hence, if the PSCI firmware supports SYSTEM_SUSPEND feature, call the above
> mentioned APIs in the psci_system_suspend_begin() and
> psci_system_suspend_enter() callbacks.
> 

With the right interpretation of what this means for us, I think this
patch looks good - but as we've discussed, I'm a bit worried about how
to deal with the alternative interpretations.

Specifically, if we fully adopt "power might be removed", we should to
take actions which will prevent a typical Qualcomm system from waking up
from sleep again.

Regards,
Bjorn

> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> [mani: reworded the description to be more elaborative]
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
> 
> This patch was part of an old series that didn't make it to mainline due to
> objections in the binding and exposing CPU_SUSPEND as S2RAM patches:
> https://lore.kernel.org/all/20241028-topic-cpu_suspend_s2ram-v1-0-9fdd9a04b75c@oss.qualcomm.com/
> 
> But this patch on its own is useful for platforms implementing the S2RAM
> feature in PSCI firmware. So I picked it up, tested on Qcom X1E T14s and
> resending it.
> 
>  drivers/firmware/psci/psci.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 38ca190d4a22..e73bae6cb23a 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -539,12 +539,22 @@ static int psci_system_suspend(unsigned long unused)
>  
>  static int psci_system_suspend_enter(suspend_state_t state)
>  {
> +	pm_set_resume_via_firmware();
> +
>  	return cpu_suspend(0, psci_system_suspend);
>  }
>  
> +static int psci_system_suspend_begin(suspend_state_t state)
> +{
> +	pm_set_suspend_via_firmware();
> +
> +	return 0;
> +}
> +
>  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 void __init psci_init_system_reset2(void)
> -- 
> 2.48.1
>