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

["Maulik Shah (mkshah)" <maulik.shah@oss.qualcomm.com>]

On 3/31/2026 2:18 AM, Bjorn Andersson wrote:
> 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.

The "power might be removed" not necessarily indicate that devices/ peripherals
should remove the wake up capability of interrupts in order to prevent waking up
from sleep.

The hibernation + suspend-to-ram is one such example, where "power might
be removed" but devices may still leave the wake up interrupts enabled.

Pasting from commit 62c552ccc3ed ("PM / Hibernate: Enable suspend to both for in-kernel hibernation.")

 |   It is often useful to suspend to memory after hibernation image has been
 |   written to disk. If the battery runs out or power is otherwise lost, the
 |   computer will resume from the hibernated image. If not, it will resume
 |   from memory

Which can be interpreted as wake up interrupt should be able to wake from suspend-to-ram state
"to the point until the power is cut off or battery runs out".

And in order to do so, wake up capable interrupts should be left enabled in HW,
for interrupt controllers like GIC (and PDC in Qualcomm SoC cases) to support wake up.

Adding Cc: linux-arm-msm for better visibility.

Thanks,
Maulik

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