From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Certain firmware implementations (such as the ones found on Qualcomm
SoCs between roughly 2015 and 2023) expose an S3-like S2RAM state
through the CPU_SUSPEND call.
This works exactly like SYSTEM_SUSPEND. The PSCI spec describes that
call as optional (and only introduced in PSCIv1.0), so not all
platforms expose it.
Marking a DT-described "domain-idle-state" as such isn't currently
well accounted for in the PSCI idle topology infrastructure: the
cpuidle and genpd framework are deeply intertwined, and trying to
separate them would cause more havoc than good.
Instead, allow the specifying of a single CPU_SUSPEND sleep param
under the /psci node that shall be treated exactly like SYSTEM_SUSPEND
from Linux's POV. As a bonus, this way we also don't have to fight
with the genpd idle governor to avoid taking the S3-like state into
consideration.
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
drivers/firmware/psci/psci.c | 36 +++++++++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index 0e622aa5ad58bbe69dfc3a71bced597618e73f15..20ae6a6d59a9f276db75260b6ca1a5827e443782 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -78,6 +78,7 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void)
static u32 psci_cpu_suspend_feature;
static bool psci_system_reset2_supported;
+static u32 psci_s2ram_suspend_param;
static inline bool psci_has_ext_power_state(void)
{
@@ -519,10 +520,10 @@ static int psci_system_suspend_begin(suspend_state_t state)
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 const struct platform_suspend_ops psci_system_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)
@@ -545,7 +546,7 @@ static void __init psci_init_system_suspend(void)
ret = psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND));
if (ret != PSCI_RET_NOT_SUPPORTED)
- suspend_set_ops(&psci_suspend_ops);
+ suspend_set_ops(&psci_system_suspend_ops);
}
static void __init psci_init_cpu_suspend(void)
@@ -673,6 +674,17 @@ static int __init psci_probe(void)
typedef int (*psci_initcall_t)(const struct device_node *);
+static int psci_cpu_suspend_s2ram_enter(suspend_state_t state)
+{
+ return psci_cpu_suspend_enter(psci_s2ram_suspend_param);
+}
+
+static const struct platform_suspend_ops psci_cpu_suspend_s2ram_ops = {
+ .valid = suspend_valid_only_mem,
+ .enter = psci_cpu_suspend_s2ram_enter,
+ .begin = psci_system_suspend_begin,
+};
+
/*
* PSCI init function for PSCI versions >=0.2
*
@@ -686,6 +698,20 @@ static int __init psci_0_2_init(const struct device_node *np)
if (err)
return err;
+ /*
+ * Some firmwares expose S2RAM entry through a custom suspend param.
+ *
+ * If found, register a suspend handler instead of registering the
+ * idle state with cpuidle.
+ */
+ err = of_property_read_u32(np, "arm,psci-s2ram-param", &psci_s2ram_suspend_param);
+ if (!err) {
+ suspend_set_ops(&psci_cpu_suspend_s2ram_ops);
+ } else if (err != -EINVAL) {
+ pr_err("Couldn't read the S2RAM PSCI suspend param: %d\n",
+ psci_s2ram_suspend_param);
+ }
+
/*
* Starting with v0.2, the PSCI specification introduced a call
* (PSCI_VERSION) that allows probing the firmware version, so
--
2.47.0On Mon, Oct 28, 2024 at 03:22:59PM +0100, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> Certain firmware implementations (such as the ones found on Qualcomm
> SoCs between roughly 2015 and 2023) expose an S3-like S2RAM state
> through the CPU_SUSPEND call.
>
> This works exactly like SYSTEM_SUSPEND. The PSCI spec describes that
> call as optional (and only introduced in PSCIv1.0), so not all
> platforms expose it.
>
> Marking a DT-described "domain-idle-state" as such isn't currently
> well accounted for in the PSCI idle topology infrastructure: the
> cpuidle and genpd framework are deeply intertwined, and trying to
> separate them would cause more havoc than good.
I don't understand what you mean here please elaborate.
The part of the story I understand is that you have a system (well,
firmware for an extended set of systems) that does not implement
SYSTEM_SUSPEND but can reach a S2R like system state through the
CPU_SUSPEND call. Firmware works in OS-initiated mode, idle-states
should allow you to define idle states that allow the system to
enter the S2R state through CPUidle.
Please explain to me what's missing.
> Instead, allow the specifying of a single CPU_SUSPEND sleep param
> under the /psci node that shall be treated exactly like SYSTEM_SUSPEND
> from Linux's POV. As a bonus, this way we also don't have to fight
> with the genpd idle governor to avoid taking the S3-like state into
> consideration.
That's not acceptable. I want to understand what's preventing this
system to enter that state through suspend2idle and the mainline code.
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
> drivers/firmware/psci/psci.c | 36 +++++++++++++++++++++++++++++++-----
> 1 file changed, 31 insertions(+), 5 deletions(-)
NACK
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 0e622aa5ad58bbe69dfc3a71bced597618e73f15..20ae6a6d59a9f276db75260b6ca1a5827e443782 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -78,6 +78,7 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void)
>
> static u32 psci_cpu_suspend_feature;
> static bool psci_system_reset2_supported;
> +static u32 psci_s2ram_suspend_param;
>
> static inline bool psci_has_ext_power_state(void)
> {
> @@ -519,10 +520,10 @@ static int psci_system_suspend_begin(suspend_state_t state)
> 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 const struct platform_suspend_ops psci_system_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)
> @@ -545,7 +546,7 @@ static void __init psci_init_system_suspend(void)
> ret = psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND));
>
> if (ret != PSCI_RET_NOT_SUPPORTED)
> - suspend_set_ops(&psci_suspend_ops);
> + suspend_set_ops(&psci_system_suspend_ops);
> }
>
> static void __init psci_init_cpu_suspend(void)
> @@ -673,6 +674,17 @@ static int __init psci_probe(void)
>
> typedef int (*psci_initcall_t)(const struct device_node *);
>
> +static int psci_cpu_suspend_s2ram_enter(suspend_state_t state)
> +{
> + return psci_cpu_suspend_enter(psci_s2ram_suspend_param);
> +}
> +
> +static const struct platform_suspend_ops psci_cpu_suspend_s2ram_ops = {
> + .valid = suspend_valid_only_mem,
> + .enter = psci_cpu_suspend_s2ram_enter,
> + .begin = psci_system_suspend_begin,
> +};
> +
> /*
> * PSCI init function for PSCI versions >=0.2
> *
> @@ -686,6 +698,20 @@ static int __init psci_0_2_init(const struct device_node *np)
> if (err)
> return err;
>
> + /*
> + * Some firmwares expose S2RAM entry through a custom suspend param.
> + *
> + * If found, register a suspend handler instead of registering the
> + * idle state with cpuidle.
> + */
> + err = of_property_read_u32(np, "arm,psci-s2ram-param", &psci_s2ram_suspend_param);
> + if (!err) {
> + suspend_set_ops(&psci_cpu_suspend_s2ram_ops);
> + } else if (err != -EINVAL) {
> + pr_err("Couldn't read the S2RAM PSCI suspend param: %d\n",
> + psci_s2ram_suspend_param);
> + }
> +
> /*
> * Starting with v0.2, the PSCI specification introduced a call
> * (PSCI_VERSION) that allows probing the firmware version, so
>
> --
> 2.47.0
>
On 13.11.2024 1:57 PM, Lorenzo Pieralisi wrote: > On Mon, Oct 28, 2024 at 03:22:59PM +0100, Konrad Dybcio wrote: >> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >> >> Certain firmware implementations (such as the ones found on Qualcomm >> SoCs between roughly 2015 and 2023) expose an S3-like S2RAM state >> through the CPU_SUSPEND call. >> >> This works exactly like SYSTEM_SUSPEND. The PSCI spec describes that >> call as optional (and only introduced in PSCIv1.0), so not all >> platforms expose it. >> >> Marking a DT-described "domain-idle-state" as such isn't currently >> well accounted for in the PSCI idle topology infrastructure: the >> cpuidle and genpd framework are deeply intertwined, and trying to >> separate them would cause more havoc than good. > > I don't understand what you mean here please elaborate. > > The part of the story I understand is that you have a system (well, > firmware for an extended set of systems) that does not implement > SYSTEM_SUSPEND but can reach a S2R like system state through the > CPU_SUSPEND call. Firmware works in OS-initiated mode, idle-states > should allow you to define idle states that allow the system to > enter the S2R state through CPUidle. > > Please explain to me what's missing. > >> Instead, allow the specifying of a single CPU_SUSPEND sleep param >> under the /psci node that shall be treated exactly like SYSTEM_SUSPEND >> from Linux's POV. As a bonus, this way we also don't have to fight >> with the genpd idle governor to avoid taking the S3-like state into >> consideration. > > That's not acceptable. I want to understand what's preventing this > system to enter that state through suspend2idle and the mainline code. The answer to both is: Linux doesn't get to know we're entering a S2RAM state and can't make different decisions based on that, if said low power state is exposed though cpuidle. We e.g. can't expect all hardware to come back up functional after entering such state. Konrad
On Wed, Nov 13, 2024 at 01:57:23PM +0100, Lorenzo Pieralisi wrote: > On Mon, Oct 28, 2024 at 03:22:59PM +0100, Konrad Dybcio wrote: > > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > > > Certain firmware implementations (such as the ones found on Qualcomm > > SoCs between roughly 2015 and 2023) expose an S3-like S2RAM state > > through the CPU_SUSPEND call. > > > > This works exactly like SYSTEM_SUSPEND. The PSCI spec describes that > > call as optional (and only introduced in PSCIv1.0), so not all > > platforms expose it. > > > > Marking a DT-described "domain-idle-state" as such isn't currently > > well accounted for in the PSCI idle topology infrastructure: the > > cpuidle and genpd framework are deeply intertwined, and trying to > > separate them would cause more havoc than good. > > I don't understand what you mean here please elaborate. > > The part of the story I understand is that you have a system (well, > firmware for an extended set of systems) that does not implement > SYSTEM_SUSPEND but can reach a S2R like system state through the > CPU_SUSPEND call. Firmware works in OS-initiated mode, idle-states > should allow you to define idle states that allow the system to > enter the S2R state through CPUidle. > > Please explain to me what's missing. > > > Instead, allow the specifying of a single CPU_SUSPEND sleep param > > under the /psci node that shall be treated exactly like SYSTEM_SUSPEND > > from Linux's POV. As a bonus, this way we also don't have to fight > > with the genpd idle governor to avoid taking the S3-like state into > > consideration. > > That's not acceptable. I want to understand what's preventing this > system to enter that state through suspend2idle and the mainline code. > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > --- > > drivers/firmware/psci/psci.c | 36 +++++++++++++++++++++++++++++++----- > > 1 file changed, 31 insertions(+), 5 deletions(-) > > NACK > +1, will wait for the response here before adding any more questions that may lead to more confusion or discussion churn. -- Regards, Sudeep
© 2016 - 2026 Red Hat, Inc.