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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep