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

[Manivannan Sadhasivam]

On Mon, Apr 06, 2026 at 08:48:44AM -0500, Bjorn Andersson wrote:
> On Tue, Mar 31, 2026 at 12:09:38PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Mar 30, 2026 at 03:48:05PM -0500, Bjorn Andersson wrote:
> > > On Wed, Dec 31, 2025 at 09:51:26PM +0530, Manivannan Sadhasivam wrote:
> > > > From: Konrad Dybcio <konradybcio@xxxxxxxxxx>
> > > > 
> > > > 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?
> > > 
> > 
> > All devices except the ones backing volatile memory (DDR).
> > 
> 
> I do not agree that this should be the interpretation on a Qualcomm
> platform.
> 
> 
> We do have the corner case of DeepSleep, where this indeed is what will
> happen (not sure if other resource votes in the system are ignored
> though?). For all typical targets making the PSCI jump might result in
> CX being turned off (unless something else votes for it...). 
> 
> State is still retained on a case-by-case basis and some parts of the
> system are still functional when we enter such state - and that is the
> system state we desire.
> 
> Making the interpretation that all SoC resources will be disabled will
> result in a large amount of unnecessary save/restore work, in addition
> to breaking many use cases.
> 

I think adding these kind of per-device power state might be cumbersome as it
may vary from SoC to SoC. But I do agree on the pain point of context
save/restore work which will happen majority of the times since DeepSleep
platforms are limited as of now.

> I do however not think that such interpretation is necessary, the
> pm_suspend_via_firmware() kernel-doc describes that the firmware might
> perform actions to power down things, it isn't specific about the
> extent, so I think that's fine - while equating this to DeepSleep (SoC
> fully powered down) is too extreme.
> 
> 
> What bothers me with this patch in itself is that the behavior in
> relation to PCIe does match the description of pm_suspend_via_firmware()
> - the firmware does things which causes PCIe to "break", but IMHO this
> is merely because PCIe operates without voting for the resources that it
> depends on. But you keep telling me that this can't be solved in the PCI
> layer...
> 

We do not want to vote for any resources during system suspend from the PCI
layer (host controller driver in specific), and that's the problem (only a few
platforms are exceptions).

Let's say if we have connected a NVMe to the PCIe bus. Since this is a passive
device i.e, not supporting wakeup, we can safely put the PCIe bus into low power
mode (turning off resources) to save power during system suspend (let's
ignore the NVMe driver behavior now). Now the suspend (s2ram) behavior will be:

CXPC - Controller context will be retained by switching the power domain to
always ON domain

DeepSleep - Controller context will be lost once the SoC enters CXPC, since CXPC
is the pre-requisite for entering DeepSleep

So only if the NVMe driver had prepared for the power loss, the driver will
resume successfully, if the platform used DeepSleep.

> If we can agree that pm_suspend_via_firmware() relates to the state of
> CX - and merely the implicit PCSI-owned vote thereof - then I think we
> should merge this patch.
> 

Sure. It makes logical sense to relate this API behavior with the state of CX.
I'll send v2 with the updated commit message.

> 
> But regardless of this interpretation. If PCI/NVMe relies on the PSCI
> implementation's implicit vote for CX and its absence breaks NVMe during
> suspend, then we're faced with exactly the same problem if the user
> chooses s2idle as their means of suspending the system.
> 

>From the PCIe controller driver, we are going to keep the votes in s2idle, but
just let go in s2ram.

> I.e. on a Qualcomm platform, we should flag PM_SUSPEND_FLAG_FW_SUSPEND
> in s2idle as well - because from PCI/NVMe's point of view, the relevant
> resources will be gone in either configuration.
> 

No they don't, as we do not plan to drop votes in s2idle. So
PM_SUSPEND_FLAG_FW_SUSPEND only applies to s2ram.

- Mani

-- 
மணிவண்ணன் சதாசிவம்