diff mbox

[v2,3/3] drivers: firmware: psci: add system suspend support

Message ID 1434638494-514-4-git-send-email-sudeep.holla@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sudeep Holla June 18, 2015, 2:41 p.m. UTC
PSCI v1.0 introduces a new API called PSCI_SYSTEM_SUSPEND. This API
provides the mechanism by which the calling OS can request entry into
the deepest possible system sleep state.

It meets all the necessary preconditions for entering suspend to RAM
state in Linux. This patch adds support for PSCI_SYSTEM_SUSPEND in psci
firmware and registers a psci system suspend operation to implement the
suspend-to-RAM(s2r) in a generic way on all the platforms implementing
PSCI.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/psci.c   | 31 +++++++++++++++++++++++++++++++
 include/uapi/linux/psci.h |  3 +++
 2 files changed, 34 insertions(+)

Comments

Jisheng Zhang July 14, 2015, 6:17 a.m. UTC | #1
Hi Sudeep,

I'm sorry to being late. Just have some comments below.

On Thu, 18 Jun 2015 15:41:34 +0100
Sudeep Holla <sudeep.holla@arm.com> wrote:

> PSCI v1.0 introduces a new API called PSCI_SYSTEM_SUSPEND. This API
> provides the mechanism by which the calling OS can request entry into
> the deepest possible system sleep state.
> 
> It meets all the necessary preconditions for entering suspend to RAM
> state in Linux. This patch adds support for PSCI_SYSTEM_SUSPEND in psci
> firmware and registers a psci system suspend operation to implement the
> suspend-to-RAM(s2r) in a generic way on all the platforms implementing
> PSCI.
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/psci.c   | 31 +++++++++++++++++++++++++++++++
>  include/uapi/linux/psci.h |  3 +++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index 752ca7c9eb97..f2b2b3e1c6e4 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -20,11 +20,13 @@
>  #include <linux/printk.h>
>  #include <linux/psci.h>
>  #include <linux/reboot.h>
> +#include <linux/suspend.h>
>  
>  #include <uapi/linux/psci.h>
>  
>  #include <asm/system_misc.h>
>  #include <asm/smp_plat.h>
> +#include <asm/suspend.h>
>  
>  /*
>   * While a 64-bit OS can make calls with SMC32 calling conventions, for some
> @@ -222,6 +224,33 @@ static int __init psci_features(u32 psci_func_id)
>  			      psci_func_id, 0, 0);
>  }
>  
> +static int psci_system_suspend(unsigned long unused)
> +{
> +	return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
> +			      virt_to_phys(cpu_resume), 0, 0);
> +}
> +
> +static int psci_system_suspend_enter(suspend_state_t state)
> +{
> +	return cpu_suspend(0, psci_system_suspend);
> +}
> +
> +static const struct platform_suspend_ops psci_suspend_ops = {
> +	.valid          = suspend_valid_only_mem,
> +	.enter          = psci_system_suspend_enter,
> +};
> +
> +static void __init psci_init_system_suspend(void)
> +{
> +	if (!IS_ENABLED(CONFIG_SUSPEND))
> +		return;
> +
> +	if (psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND)))
> +		return;

So this requires the firmware implements SYSTEM_SUSPEND which doesn't exist
until PSCI 1.0, and even in PSCI 1.0 SYSTEM_SUSPEND is optional, we also want
suspend to ram feature on PSCI 0.2 or PSCI 1.0 w/o SYSTEM_SUSPEND, is there
any possibility to bring your previous RFC patch[1] back? Or ARM expect all
PSCI users must have SYSTEM_SUSPEND implemented if they want s2ram?

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318541.html

Thank you,
Jisheng

> +
> +	suspend_set_ops(&psci_suspend_ops);
> +}
> +
>  static void __init psci_init_cpu_suspend(void)
>  {
>  	int feature = psci_features(psci_function_id[PSCI_FN_CPU_SUSPEND]);
> @@ -311,6 +340,8 @@ static int __init psci_probe(void)
>  
>  	psci_init_cpu_suspend();
>  
> +	psci_init_system_suspend();
> +
>  	return 0;
>  }
>  
> diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
> index 2598d7c9bf20..a1a2d85a91b9 100644
> --- a/include/uapi/linux/psci.h
> +++ b/include/uapi/linux/psci.h
> @@ -47,6 +47,9 @@
>  #define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU	PSCI_0_2_FN64(7)
>  
>  #define PSCI_1_0_FN_PSCI_FEATURES		PSCI_0_2_FN(10)
> +#define PSCI_1_0_FN_SYSTEM_SUSPEND		PSCI_0_2_FN(14)
> +
> +#define PSCI_1_0_FN64_SYSTEM_SUSPEND		PSCI_0_2_FN64(14)
>  
>  /* PSCI v0.2 power state encoding for CPU_SUSPEND function */
>  #define PSCI_0_2_POWER_STATE_ID_MASK		0xffff
Sudeep Holla July 14, 2015, 9:14 a.m. UTC | #2
On 14/07/15 07:17, Jisheng Zhang wrote:
> Hi Sudeep,
>
> I'm sorry to being late. Just have some comments below.
>
> On Thu, 18 Jun 2015 15:41:34 +0100
> Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>> PSCI v1.0 introduces a new API called PSCI_SYSTEM_SUSPEND. This API
>> provides the mechanism by which the calling OS can request entry into
>> the deepest possible system sleep state.
>>
>> It meets all the necessary preconditions for entering suspend to RAM
>> state in Linux. This patch adds support for PSCI_SYSTEM_SUSPEND in psci
>> firmware and registers a psci system suspend operation to implement the
>> suspend-to-RAM(s2r) in a generic way on all the platforms implementing
>> PSCI.
>>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>   drivers/firmware/psci.c   | 31 +++++++++++++++++++++++++++++++
>>   include/uapi/linux/psci.h |  3 +++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
>> index 752ca7c9eb97..f2b2b3e1c6e4 100644
>> --- a/drivers/firmware/psci.c
>> +++ b/drivers/firmware/psci.c
>> @@ -20,11 +20,13 @@
>>   #include <linux/printk.h>
>>   #include <linux/psci.h>
>>   #include <linux/reboot.h>
>> +#include <linux/suspend.h>
>>
>>   #include <uapi/linux/psci.h>
>>
>>   #include <asm/system_misc.h>
>>   #include <asm/smp_plat.h>
>> +#include <asm/suspend.h>
>>
>>   /*
>>    * While a 64-bit OS can make calls with SMC32 calling conventions, for some
>> @@ -222,6 +224,33 @@ static int __init psci_features(u32 psci_func_id)
>>   			      psci_func_id, 0, 0);
>>   }
>>
>> +static int psci_system_suspend(unsigned long unused)
>> +{
>> +	return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
>> +			      virt_to_phys(cpu_resume), 0, 0);
>> +}
>> +
>> +static int psci_system_suspend_enter(suspend_state_t state)
>> +{
>> +	return cpu_suspend(0, psci_system_suspend);
>> +}
>> +
>> +static const struct platform_suspend_ops psci_suspend_ops = {
>> +	.valid          = suspend_valid_only_mem,
>> +	.enter          = psci_system_suspend_enter,
>> +};
>> +
>> +static void __init psci_init_system_suspend(void)
>> +{
>> +	if (!IS_ENABLED(CONFIG_SUSPEND))
>> +		return;
>> +
>> +	if (psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND)))
>> +		return;
>
> So this requires the firmware implements SYSTEM_SUSPEND which doesn't exist
> until PSCI 1.0,

Correct, if you need System to RAM in Linux on a platform with PSCI
firmware, firmware *must implement* SYSTEM_SUSPEND.

> and even in PSCI 1.0 SYSTEM_SUSPEND is optional,

Optional here means, that you may chose to implement or not in the
firmware. It doesn't mean we can implement S2R in Linux using other
PSCI methods. SYSTEM_SUSPEND was added mainly to avoid since workarounds
in the kernel.

>we also want
> suspend to ram feature on PSCI 0.2 or PSCI 1.0 w/o SYSTEM_SUSPEND,

Why do you choose to have PSCIv1.0 w/o SYSTEM_SUSPEND when you need that
feature in Linux. Is it just because we can manage with workarounds ?
Sorry, that's not an option.

> is there
> any possibility to bring your previous RFC patch[1] back? Or ARM expect all
> PSCI users must have SYSTEM_SUSPEND implemented if they want s2ram?

OK, we did expect response back then[1] when we clearly asked if the
firmware can be upgraded to v1.0 from v0.2 as the delta is not much and
helps up to avoid special DT bindings for that case. Since nobody
responded for last 6 months, it's assumed that it's no longer required.

So to answer you, no the RFC is dead and it's expected to have PSCI
firmware support for SYSTEM_SUSPEND if S2R is desired feature in Linux.

Regards,
Sudeep

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/322278.html
Jisheng Zhang July 14, 2015, 9:50 a.m. UTC | #3
Dear Sudeep,

On Tue, 14 Jul 2015 10:14:25 +0100
Sudeep Holla <sudeep.holla@arm.com> wrote:

> 
> 
> On 14/07/15 07:17, Jisheng Zhang wrote:
> > Hi Sudeep,
> >
> > I'm sorry to being late. Just have some comments below.
> >
> > On Thu, 18 Jun 2015 15:41:34 +0100
> > Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> >> PSCI v1.0 introduces a new API called PSCI_SYSTEM_SUSPEND. This API
> >> provides the mechanism by which the calling OS can request entry into
> >> the deepest possible system sleep state.
> >>
> >> It meets all the necessary preconditions for entering suspend to RAM
> >> state in Linux. This patch adds support for PSCI_SYSTEM_SUSPEND in psci
> >> firmware and registers a psci system suspend operation to implement the
> >> suspend-to-RAM(s2r) in a generic way on all the platforms implementing
> >> PSCI.
> >>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >> ---
> >>   drivers/firmware/psci.c   | 31 +++++++++++++++++++++++++++++++
> >>   include/uapi/linux/psci.h |  3 +++
> >>   2 files changed, 34 insertions(+)
> >>
> >> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> >> index 752ca7c9eb97..f2b2b3e1c6e4 100644
> >> --- a/drivers/firmware/psci.c
> >> +++ b/drivers/firmware/psci.c
> >> @@ -20,11 +20,13 @@
> >>   #include <linux/printk.h>
> >>   #include <linux/psci.h>
> >>   #include <linux/reboot.h>
> >> +#include <linux/suspend.h>
> >>
> >>   #include <uapi/linux/psci.h>
> >>
> >>   #include <asm/system_misc.h>
> >>   #include <asm/smp_plat.h>
> >> +#include <asm/suspend.h>
> >>
> >>   /*
> >>    * While a 64-bit OS can make calls with SMC32 calling conventions, for some
> >> @@ -222,6 +224,33 @@ static int __init psci_features(u32 psci_func_id)
> >>   			      psci_func_id, 0, 0);
> >>   }
> >>
> >> +static int psci_system_suspend(unsigned long unused)
> >> +{
> >> +	return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
> >> +			      virt_to_phys(cpu_resume), 0, 0);
> >> +}
> >> +
> >> +static int psci_system_suspend_enter(suspend_state_t state)
> >> +{
> >> +	return cpu_suspend(0, psci_system_suspend);
> >> +}
> >> +
> >> +static const struct platform_suspend_ops psci_suspend_ops = {
> >> +	.valid          = suspend_valid_only_mem,
> >> +	.enter          = psci_system_suspend_enter,
> >> +};
> >> +
> >> +static void __init psci_init_system_suspend(void)
> >> +{
> >> +	if (!IS_ENABLED(CONFIG_SUSPEND))
> >> +		return;
> >> +
> >> +	if (psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND)))
> >> +		return;
> >
> > So this requires the firmware implements SYSTEM_SUSPEND which doesn't exist
> > until PSCI 1.0,
> 
> Correct, if you need System to RAM in Linux on a platform with PSCI
> firmware, firmware *must implement* SYSTEM_SUSPEND.
> 
> > and even in PSCI 1.0 SYSTEM_SUSPEND is optional,
> 
> Optional here means, that you may chose to implement or not in the
> firmware. It doesn't mean we can implement S2R in Linux using other
> PSCI methods. SYSTEM_SUSPEND was added mainly to avoid since workarounds
> in the kernel.
> 
> >we also want
> > suspend to ram feature on PSCI 0.2 or PSCI 1.0 w/o SYSTEM_SUSPEND,
> 
> Why do you choose to have PSCIv1.0 w/o SYSTEM_SUSPEND when you need that
> feature in Linux. Is it just because we can manage with workarounds ?
> Sorry, that's not an option.

The problem is the PSCI 1.0 SYSTEM_SUSPEND definition: we can't pass something
like stateid as in CPU_SUSPEND to firmware. The stateid is used to tell
firmware to go to different Sleep state, for example S2 or S3.

With your RFC patch, it's easy to extend to support this feature, but if we
are limited to SYSTEM_SUSPEND in PSCI 1.0, we have to use a different code
path for non-S3 sleep state.

Is there any elegant solution?

Thanks,
Jisheng

> 
> > is there
> > any possibility to bring your previous RFC patch[1] back? Or ARM expect all
> > PSCI users must have SYSTEM_SUSPEND implemented if they want s2ram?
> 
> OK, we did expect response back then[1] when we clearly asked if the
> firmware can be upgraded to v1.0 from v0.2 as the delta is not much and
> helps up to avoid special DT bindings for that case. Since nobody
> responded for last 6 months, it's assumed that it's no longer required.
> 
> So to answer you, no the RFC is dead and it's expected to have PSCI
> firmware support for SYSTEM_SUSPEND if S2R is desired feature in Linux.
> 
> Regards,
> Sudeep
> 
> [1] 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/322278.html
Sudeep Holla July 14, 2015, 11:02 a.m. UTC | #4
On 14/07/15 10:50, Jisheng Zhang wrote:
> Dear Sudeep,
>
> On Tue, 14 Jul 2015 10:14:25 +0100
> Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>>
>>
>> On 14/07/15 07:17, Jisheng Zhang wrote:
>>> Hi Sudeep,
>>>
>>> I'm sorry to being late. Just have some comments below.
>>>
>>> On Thu, 18 Jun 2015 15:41:34 +0100
>>> Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>>> PSCI v1.0 introduces a new API called PSCI_SYSTEM_SUSPEND. This API
>>>> provides the mechanism by which the calling OS can request entry into
>>>> the deepest possible system sleep state.
>>>>
>>>> It meets all the necessary preconditions for entering suspend to RAM
>>>> state in Linux. This patch adds support for PSCI_SYSTEM_SUSPEND in psci
>>>> firmware and registers a psci system suspend operation to implement the
>>>> suspend-to-RAM(s2r) in a generic way on all the platforms implementing
>>>> PSCI.
>>>>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>> ---
>>>>    drivers/firmware/psci.c   | 31 +++++++++++++++++++++++++++++++
>>>>    include/uapi/linux/psci.h |  3 +++
>>>>    2 files changed, 34 insertions(+)
>>>>
>>>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
>>>> index 752ca7c9eb97..f2b2b3e1c6e4 100644
>>>> --- a/drivers/firmware/psci.c
>>>> +++ b/drivers/firmware/psci.c
>>>> @@ -20,11 +20,13 @@
>>>>    #include <linux/printk.h>
>>>>    #include <linux/psci.h>
>>>>    #include <linux/reboot.h>
>>>> +#include <linux/suspend.h>
>>>>
>>>>    #include <uapi/linux/psci.h>
>>>>
>>>>    #include <asm/system_misc.h>
>>>>    #include <asm/smp_plat.h>
>>>> +#include <asm/suspend.h>
>>>>
>>>>    /*
>>>>     * While a 64-bit OS can make calls with SMC32 calling conventions, for some
>>>> @@ -222,6 +224,33 @@ static int __init psci_features(u32 psci_func_id)
>>>>    			      psci_func_id, 0, 0);
>>>>    }
>>>>
>>>> +static int psci_system_suspend(unsigned long unused)
>>>> +{
>>>> +	return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
>>>> +			      virt_to_phys(cpu_resume), 0, 0);
>>>> +}
>>>> +
>>>> +static int psci_system_suspend_enter(suspend_state_t state)
>>>> +{
>>>> +	return cpu_suspend(0, psci_system_suspend);
>>>> +}
>>>> +
>>>> +static const struct platform_suspend_ops psci_suspend_ops = {
>>>> +	.valid          = suspend_valid_only_mem,
>>>> +	.enter          = psci_system_suspend_enter,
>>>> +};
>>>> +
>>>> +static void __init psci_init_system_suspend(void)
>>>> +{
>>>> +	if (!IS_ENABLED(CONFIG_SUSPEND))
>>>> +		return;
>>>> +
>>>> +	if (psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND)))
>>>> +		return;
>>>
>>> So this requires the firmware implements SYSTEM_SUSPEND which doesn't exist
>>> until PSCI 1.0,
>>
>> Correct, if you need System to RAM in Linux on a platform with PSCI
>> firmware, firmware *must implement* SYSTEM_SUSPEND.
>>
>>> and even in PSCI 1.0 SYSTEM_SUSPEND is optional,
>>
>> Optional here means, that you may chose to implement or not in the
>> firmware. It doesn't mean we can implement S2R in Linux using other
>> PSCI methods. SYSTEM_SUSPEND was added mainly to avoid since workarounds
>> in the kernel.
>>
>>> we also want
>>> suspend to ram feature on PSCI 0.2 or PSCI 1.0 w/o SYSTEM_SUSPEND,
>>
>> Why do you choose to have PSCIv1.0 w/o SYSTEM_SUSPEND when you need that
>> feature in Linux. Is it just because we can manage with workarounds ?
>> Sorry, that's not an option.
>
> The problem is the PSCI 1.0 SYSTEM_SUSPEND definition: we can't pass something
> like stateid as in CPU_SUSPEND to firmware. The stateid is used to tell
> firmware to go to different Sleep state, for example S2 or S3.
>

Can you elaborate on S2 here with an example ? You are using ACPI
Sleep State definitions here which should not be mixed with PSCI.

*Let's refrain ourselves from using ACPI S-states here* until it's well
defined on ARM systems. The current definition is x86 based and doesn't
suit ARM at all.

It's difficult to distinguish between the intermediate system suspend
sleep states you are referring and the composite cpu idle states. That's
the reason I am asking for the example to see if the sleep states you
referring can be considered as composite cpu idle states.
I strongly believe that's the case.

I would consider any other states as composite CPU idle states. In
order to utilize Linux PM features, we would use PSCI functions as
below.

/-----------------------------------\
| PSCI Feature   | Linux PM feature |
|-----------------------------------|
| SYSTEM_SUSPEND |  Suspend-to-RAM  |
|-----------------------------------|
| SYSTEM_SHUTDOWN|  Suspend-to-Disk |
\-----------------------------------/

> With your RFC patch, it's easy to extend to support this feature, but if we
> are limited to SYSTEM_SUSPEND in PSCI 1.0, we have to use a different code
> path for non-S3 sleep state.
>

As I said above can you provide examples for such states.

Regards,
Sudeep
Jisheng Zhang July 14, 2015, 11:40 a.m. UTC | #5
Dear Sudeep,

On Tue, 14 Jul 2015 12:02:15 +0100
Sudeep Holla <sudeep.holla@arm.com> wrote:

> 
> 
> On 14/07/15 10:50, Jisheng Zhang wrote:
> > Dear Sudeep,
> >
> > On Tue, 14 Jul 2015 10:14:25 +0100
> > Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> >>
> >>
> >> On 14/07/15 07:17, Jisheng Zhang wrote:
> >>> Hi Sudeep,
> >>>
> >>> I'm sorry to being late. Just have some comments below.
> >>>
> >>> On Thu, 18 Jun 2015 15:41:34 +0100
> >>> Sudeep Holla <sudeep.holla@arm.com> wrote:
> >>>
> >>>> PSCI v1.0 introduces a new API called PSCI_SYSTEM_SUSPEND. This API
> >>>> provides the mechanism by which the calling OS can request entry into
> >>>> the deepest possible system sleep state.
> >>>>
> >>>> It meets all the necessary preconditions for entering suspend to RAM
> >>>> state in Linux. This patch adds support for PSCI_SYSTEM_SUSPEND in psci
> >>>> firmware and registers a psci system suspend operation to implement the
> >>>> suspend-to-RAM(s2r) in a generic way on all the platforms implementing
> >>>> PSCI.
> >>>>
> >>>> Cc: Mark Rutland <mark.rutland@arm.com>
> >>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >>>> ---
> >>>>    drivers/firmware/psci.c   | 31 +++++++++++++++++++++++++++++++
> >>>>    include/uapi/linux/psci.h |  3 +++
> >>>>    2 files changed, 34 insertions(+)
> >>>>
> >>>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> >>>> index 752ca7c9eb97..f2b2b3e1c6e4 100644
> >>>> --- a/drivers/firmware/psci.c
> >>>> +++ b/drivers/firmware/psci.c
> >>>> @@ -20,11 +20,13 @@
> >>>>    #include <linux/printk.h>
> >>>>    #include <linux/psci.h>
> >>>>    #include <linux/reboot.h>
> >>>> +#include <linux/suspend.h>
> >>>>
> >>>>    #include <uapi/linux/psci.h>
> >>>>
> >>>>    #include <asm/system_misc.h>
> >>>>    #include <asm/smp_plat.h>
> >>>> +#include <asm/suspend.h>
> >>>>
> >>>>    /*
> >>>>     * While a 64-bit OS can make calls with SMC32 calling conventions, for some
> >>>> @@ -222,6 +224,33 @@ static int __init psci_features(u32 psci_func_id)
> >>>>    			      psci_func_id, 0, 0);
> >>>>    }
> >>>>
> >>>> +static int psci_system_suspend(unsigned long unused)
> >>>> +{
> >>>> +	return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
> >>>> +			      virt_to_phys(cpu_resume), 0, 0);
> >>>> +}
> >>>> +
> >>>> +static int psci_system_suspend_enter(suspend_state_t state)
> >>>> +{
> >>>> +	return cpu_suspend(0, psci_system_suspend);
> >>>> +}
> >>>> +
> >>>> +static const struct platform_suspend_ops psci_suspend_ops = {
> >>>> +	.valid          = suspend_valid_only_mem,
> >>>> +	.enter          = psci_system_suspend_enter,
> >>>> +};
> >>>> +
> >>>> +static void __init psci_init_system_suspend(void)
> >>>> +{
> >>>> +	if (!IS_ENABLED(CONFIG_SUSPEND))
> >>>> +		return;
> >>>> +
> >>>> +	if (psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND)))
> >>>> +		return;
> >>>
> >>> So this requires the firmware implements SYSTEM_SUSPEND which doesn't exist
> >>> until PSCI 1.0,
> >>
> >> Correct, if you need System to RAM in Linux on a platform with PSCI
> >> firmware, firmware *must implement* SYSTEM_SUSPEND.
> >>
> >>> and even in PSCI 1.0 SYSTEM_SUSPEND is optional,
> >>
> >> Optional here means, that you may chose to implement or not in the
> >> firmware. It doesn't mean we can implement S2R in Linux using other
> >> PSCI methods. SYSTEM_SUSPEND was added mainly to avoid since workarounds
> >> in the kernel.
> >>
> >>> we also want
> >>> suspend to ram feature on PSCI 0.2 or PSCI 1.0 w/o SYSTEM_SUSPEND,
> >>
> >> Why do you choose to have PSCIv1.0 w/o SYSTEM_SUSPEND when you need that
> >> feature in Linux. Is it just because we can manage with workarounds ?
> >> Sorry, that's not an option.
> >
> > The problem is the PSCI 1.0 SYSTEM_SUSPEND definition: we can't pass something
> > like stateid as in CPU_SUSPEND to firmware. The stateid is used to tell
> > firmware to go to different Sleep state, for example S2 or S3.
> >
> 
> Can you elaborate on S2 here with an example ? You are using ACPI
> Sleep State definitions here which should not be mixed with PSCI.

Yes, it's ACPI concept. Here "S2" can be the "S2" mentioned in PSCI 1.0 spec.

PSCI 1.0 SPEC says:

"While ACPI distinguishes between two system level suspend to RAM states,
S2 and S3, PSCI provides a single API. In practice, operating systems use
only one suspend to RAM state, so this is not seen as a limitation. Note
that this specification is using the state definitions of S2 and S3 provided
by ACPI, but does not limit use of this PSCI function to ACPI based systems.
Semantics such as those described for sleep states S2 and S3 are used in
non-ACPI based systems."

Here is an example:

S3 is the state where All devices are suspended and power down,  memory is
placed in self-refresh mode. Usually this is the suspend to ram state.

S2 is the state where devices except cpus are suspended and put in a
low-power state(not power off), if the deivce doesn't support lower power mode,
the device is left on. memory is placed in self-refresh mode. secondary CPUs
are powered off via. PSCI_CPU_OFF, boot cpu is put into WFI state and runs at
low freq.

Thanks,
Jisheng

> 
> *Let's refrain ourselves from using ACPI S-states here* until it's well
> defined on ARM systems. The current definition is x86 based and doesn't
> suit ARM at all.
> 
> It's difficult to distinguish between the intermediate system suspend
> sleep states you are referring and the composite cpu idle states. That's
> the reason I am asking for the example to see if the sleep states you
> referring can be considered as composite cpu idle states.
> I strongly believe that's the case.
> 
> I would consider any other states as composite CPU idle states. In
> order to utilize Linux PM features, we would use PSCI functions as
> below.
> 
> /-----------------------------------\
> | PSCI Feature   | Linux PM feature |
> |-----------------------------------|
> | SYSTEM_SUSPEND |  Suspend-to-RAM  |
> |-----------------------------------|
> | SYSTEM_SHUTDOWN|  Suspend-to-Disk |
> \-----------------------------------/
> 
> > With your RFC patch, it's easy to extend to support this feature, but if we
> > are limited to SYSTEM_SUSPEND in PSCI 1.0, we have to use a different code
> > path for non-S3 sleep state.
> >
> 
> As I said above can you provide examples for such states.
> 
> Regards,
> Sudeep
Sudeep Holla July 14, 2015, 1:18 p.m. UTC | #6
On 14/07/15 12:40, Jisheng Zhang wrote:
> Dear Sudeep,
>
> On Tue, 14 Jul 2015 12:02:15 +0100
> Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>>
>>
>> On 14/07/15 10:50, Jisheng Zhang wrote:
>>> Dear Sudeep,
>>>
>>> On Tue, 14 Jul 2015 10:14:25 +0100
>>> Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>>>
>>>>
>>>> On 14/07/15 07:17, Jisheng Zhang wrote:
>>>>> Hi Sudeep,
>>>>>
>>>>> I'm sorry to being late. Just have some comments below.
>>>>>
>>>>> On Thu, 18 Jun 2015 15:41:34 +0100
>>>>> Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>>>
>>>>>> PSCI v1.0 introduces a new API called PSCI_SYSTEM_SUSPEND. This API
>>>>>> provides the mechanism by which the calling OS can request entry into
>>>>>> the deepest possible system sleep state.
>>>>>>
>>>>>> It meets all the necessary preconditions for entering suspend to RAM
>>>>>> state in Linux. This patch adds support for PSCI_SYSTEM_SUSPEND in psci
>>>>>> firmware and registers a psci system suspend operation to implement the
>>>>>> suspend-to-RAM(s2r) in a generic way on all the platforms implementing
>>>>>> PSCI.
>>>>>>
>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>>>> ---
>>>>>>     drivers/firmware/psci.c   | 31 +++++++++++++++++++++++++++++++
>>>>>>     include/uapi/linux/psci.h |  3 +++
>>>>>>     2 files changed, 34 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
>>>>>> index 752ca7c9eb97..f2b2b3e1c6e4 100644
>>>>>> --- a/drivers/firmware/psci.c
>>>>>> +++ b/drivers/firmware/psci.c
>>>>>> @@ -20,11 +20,13 @@
>>>>>>     #include <linux/printk.h>
>>>>>>     #include <linux/psci.h>
>>>>>>     #include <linux/reboot.h>
>>>>>> +#include <linux/suspend.h>
>>>>>>
>>>>>>     #include <uapi/linux/psci.h>
>>>>>>
>>>>>>     #include <asm/system_misc.h>
>>>>>>     #include <asm/smp_plat.h>
>>>>>> +#include <asm/suspend.h>
>>>>>>
>>>>>>     /*
>>>>>>      * While a 64-bit OS can make calls with SMC32 calling conventions, for some
>>>>>> @@ -222,6 +224,33 @@ static int __init psci_features(u32 psci_func_id)
>>>>>>     			      psci_func_id, 0, 0);
>>>>>>     }
>>>>>>
>>>>>> +static int psci_system_suspend(unsigned long unused)
>>>>>> +{
>>>>>> +	return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
>>>>>> +			      virt_to_phys(cpu_resume), 0, 0);
>>>>>> +}
>>>>>> +
>>>>>> +static int psci_system_suspend_enter(suspend_state_t state)
>>>>>> +{
>>>>>> +	return cpu_suspend(0, psci_system_suspend);
>>>>>> +}
>>>>>> +
>>>>>> +static const struct platform_suspend_ops psci_suspend_ops = {
>>>>>> +	.valid          = suspend_valid_only_mem,
>>>>>> +	.enter          = psci_system_suspend_enter,
>>>>>> +};
>>>>>> +
>>>>>> +static void __init psci_init_system_suspend(void)
>>>>>> +{
>>>>>> +	if (!IS_ENABLED(CONFIG_SUSPEND))
>>>>>> +		return;
>>>>>> +
>>>>>> +	if (psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND)))
>>>>>> +		return;
>>>>>
>>>>> So this requires the firmware implements SYSTEM_SUSPEND which doesn't exist
>>>>> until PSCI 1.0,
>>>>
>>>> Correct, if you need System to RAM in Linux on a platform with PSCI
>>>> firmware, firmware *must implement* SYSTEM_SUSPEND.
>>>>
>>>>> and even in PSCI 1.0 SYSTEM_SUSPEND is optional,
>>>>
>>>> Optional here means, that you may chose to implement or not in the
>>>> firmware. It doesn't mean we can implement S2R in Linux using other
>>>> PSCI methods. SYSTEM_SUSPEND was added mainly to avoid since workarounds
>>>> in the kernel.
>>>>
>>>>> we also want
>>>>> suspend to ram feature on PSCI 0.2 or PSCI 1.0 w/o SYSTEM_SUSPEND,
>>>>
>>>> Why do you choose to have PSCIv1.0 w/o SYSTEM_SUSPEND when you need that
>>>> feature in Linux. Is it just because we can manage with workarounds ?
>>>> Sorry, that's not an option.
>>>
>>> The problem is the PSCI 1.0 SYSTEM_SUSPEND definition: we can't pass something
>>> like stateid as in CPU_SUSPEND to firmware. The stateid is used to tell
>>> firmware to go to different Sleep state, for example S2 or S3.
>>>
>>
>> Can you elaborate on S2 here with an example ? You are using ACPI
>> Sleep State definitions here which should not be mixed with PSCI.
>
> Yes, it's ACPI concept. Here "S2" can be the "S2" mentioned in PSCI 1.0 spec.
>
> PSCI 1.0 SPEC says:
>
> "While ACPI distinguishes between two system level suspend to RAM states,
> S2 and S3, PSCI provides a single API. In practice, operating systems use
> only one suspend to RAM state, so this is not seen as a limitation. Note
> that this specification is using the state definitions of S2 and S3 provided
> by ACPI, but does not limit use of this PSCI function to ACPI based systems.
> Semantics such as those described for sleep states S2 and S3 are used in
> non-ACPI based systems."
>

OK, though similar semantics may be used else where, we need to define
the state similar to the way ACPI does it for x86 systems.

> Here is an example:
>
> S3 is the state where All devices are suspended and power down,  memory is
> placed in self-refresh mode. Usually this is the suspend to ram state.
>

Agreed.

> S2 is the state where devices except cpus are suspended and put in a
> low-power state(not power off), if the deivce doesn't support lower power mode,
> the device is left on. memory is placed in self-refresh mode. secondary CPUs
> are powered off via. PSCI_CPU_OFF, boot cpu is put into WFI state and runs at
> low freq.
>

Is this a standard state or custom tailored for some power optimization
on particular system ? Anyway I am not sure if I quite understand the
exact definition of this state. When and how often do you use this
use this state ?

Have you looked at PM_SUSPEND_FREEZE state implemented in Linux ?
For me this S2 you explained sounds similar to PM_SUSPEND_FREEZE state.
In short, PM_SUSPEND_FREEZE = all the processes are frozen + all the
devices are suspended + all the processors enter deepest idle state
possible.

Though I don't understand why you need to power-off secondary cpus
and stay in WFI on boot cpu as that would have increased latency
defeating the purpose of S2 state. Also why you are mixing DVFS here ?
PSCI deals with just low power states and has nothing to do with DVFS.

Further IMO we can also achieve a low power state almost close to
PM_SUSPEND_FREEZE having run-time PM for all the devices and cpuidle.

Regards,
Sudeep
Jisheng Zhang July 15, 2015, 2:34 a.m. UTC | #7
Dear Sudeep,

On Tue, 14 Jul 2015 14:18:10 +0100
Sudeep Holla wrote:

> >>>>>> +
> >>>>>> +static void __init psci_init_system_suspend(void)
> >>>>>> +{
> >>>>>> +	if (!IS_ENABLED(CONFIG_SUSPEND))
> >>>>>> +		return;
> >>>>>> +
> >>>>>> +	if (psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND)))
> >>>>>> +		return;
> >>>>>
> >>>>> So this requires the firmware implements SYSTEM_SUSPEND which doesn't exist
> >>>>> until PSCI 1.0,
> >>>>
> >>>> Correct, if you need System to RAM in Linux on a platform with PSCI
> >>>> firmware, firmware *must implement* SYSTEM_SUSPEND.
> >>>>
> >>>>> and even in PSCI 1.0 SYSTEM_SUSPEND is optional,
> >>>>
> >>>> Optional here means, that you may chose to implement or not in the
> >>>> firmware. It doesn't mean we can implement S2R in Linux using other
> >>>> PSCI methods. SYSTEM_SUSPEND was added mainly to avoid since workarounds
> >>>> in the kernel.
> >>>>
> >>>>> we also want
> >>>>> suspend to ram feature on PSCI 0.2 or PSCI 1.0 w/o SYSTEM_SUSPEND,
> >>>>
> >>>> Why do you choose to have PSCIv1.0 w/o SYSTEM_SUSPEND when you need that
> >>>> feature in Linux. Is it just because we can manage with workarounds ?
> >>>> Sorry, that's not an option.
> >>>
> >>> The problem is the PSCI 1.0 SYSTEM_SUSPEND definition: we can't pass something
> >>> like stateid as in CPU_SUSPEND to firmware. The stateid is used to tell
> >>> firmware to go to different Sleep state, for example S2 or S3.
> >>>
> >>
> >> Can you elaborate on S2 here with an example ? You are using ACPI
> >> Sleep State definitions here which should not be mixed with PSCI.
> >
> > Yes, it's ACPI concept. Here "S2" can be the "S2" mentioned in PSCI 1.0 spec.
> >
> > PSCI 1.0 SPEC says:
> >
> > "While ACPI distinguishes between two system level suspend to RAM states,
> > S2 and S3, PSCI provides a single API. In practice, operating systems use
> > only one suspend to RAM state, so this is not seen as a limitation. Note
> > that this specification is using the state definitions of S2 and S3 provided
> > by ACPI, but does not limit use of this PSCI function to ACPI based systems.
> > Semantics such as those described for sleep states S2 and S3 are used in
> > non-ACPI based systems."
> >
> 
> OK, though similar semantics may be used else where, we need to define
> the state similar to the way ACPI does it for x86 systems.
> 
> > Here is an example:
> >
> > S3 is the state where All devices are suspended and power down,  memory is
> > placed in self-refresh mode. Usually this is the suspend to ram state.
> >
> 
> Agreed.
> 
> > S2 is the state where devices except cpus are suspended and put in a
> > low-power state(not power off), if the deivce doesn't support lower power mode,
> > the device is left on. memory is placed in self-refresh mode. secondary CPUs
> > are powered off via. PSCI_CPU_OFF, boot cpu is put into WFI state and runs at
> > low freq.
> >
> 
> Is this a standard state or custom tailored for some power optimization
> on particular system ? Anyway I am not sure if I quite understand the

This is customized state.

> exact definition of this state. When and how often do you use this
> use this state ?

Very often. When we want s2ram similar state but low-latency back transition,
we want to use this state.

> 
> Have you looked at PM_SUSPEND_FREEZE state implemented in Linux ?
> For me this S2 you explained sounds similar to PM_SUSPEND_FREEZE state.
> In short, PM_SUSPEND_FREEZE = all the processes are frozen + all the
> devices are suspended + all the processors enter deepest idle state
> possible.

The S2 in my example is a bit different with freeze. It's looks more like
the S1 or standby in Documentation/power/states.txt. The biggest difference
with freeze is whether putting memory in self-refresh state or not.

> 
> Though I don't understand why you need to power-off secondary cpus
> and stay in WFI on boot cpu as that would have increased latency

we want save more power, the increased latency is affordable.

> defeating the purpose of S2 state. Also why you are mixing DVFS here ?
> PSCI deals with just low power states and has nothing to do with DVFS.

It's not DVFS related, but to put the boot cpu in a low power state while
still keeping its power.

> 
> Further IMO we can also achieve a low power state almost close to
> PM_SUSPEND_FREEZE having run-time PM for all the devices and cpuidle.

we want one more sleep state which saves more power than freeze.

Or let's change the question: how to implement "standby" or "s1" in
Documentation/power/states.txt with the help of PSCI? Seems the PSCI spec
expects "operating systems use only one suspend to RAM state", but this
is a limitation in the long run.

Thanks,
Jisheng
Sudeep Holla July 15, 2015, 10:20 a.m. UTC | #8
Hi Jisheng,

On 15/07/15 03:34, Jisheng Zhang wrote:
> Dear Sudeep,
>
> On Tue, 14 Jul 2015 14:18:10 +0100
> Sudeep Holla wrote:

[...]

>>
>> OK, though similar semantics may be used else where, we need to define
>> the state similar to the way ACPI does it for x86 systems.
>>
>>> Here is an example:
>>>
>>> S3 is the state where All devices are suspended and power down,  memory is
>>> placed in self-refresh mode. Usually this is the suspend to ram state.
>>>
>>
>> Agreed.
>>
>>> S2 is the state where devices except cpus are suspended and put in a
>>> low-power state(not power off), if the deivce doesn't support lower power mode,
>>> the device is left on. memory is placed in self-refresh mode. secondary CPUs
>>> are powered off via. PSCI_CPU_OFF, boot cpu is put into WFI state and runs at
>>> low freq.
>>>
>>
>> Is this a standard state or custom tailored for some power optimization
>> on particular system ? Anyway I am not sure if I quite understand the
>
> This is customized state.
>

OK, but I do have concern as you mention that you leave certain device
on, what if that device initiate some DMA operation which might not be
possible when DDR is in self-refresh mode. IMO this is highly customized
state.

>> exact definition of this state. When and how often do you use this
>> use this state ?
>
> Very often. When we want s2ram similar state but low-latency back transition,
> we want to use this state.
>

I don't understand this. You want secondaries hot-plugged out which is
quite expensive yet you mention this is low-latency. That could be
possible as you leave last CPU in WFI but again highly customized state.

>>
>> Have you looked at PM_SUSPEND_FREEZE state implemented in Linux ?
>> For me this S2 you explained sounds similar to PM_SUSPEND_FREEZE state.
>> In short, PM_SUSPEND_FREEZE = all the processes are frozen + all the
>> devices are suspended + all the processors enter deepest idle state
>> possible.
>
> The S2 in my example is a bit different with freeze. It's looks more like
> the S1 or standby in Documentation/power/states.txt. The biggest difference
> with freeze is whether putting memory in self-refresh state or not.
>

OK I am still struggling to map these(S1/S2) to ARM systems as they
designed and map well for x86 systems. Since the idle states on include
state where CPU power is lost, I wonder if the state you are explaining
provides any more power save compared to system freeze.

>>
>> Though I don't understand why you need to power-off secondary cpus
>> and stay in WFI on boot cpu as that would have increased latency
>
> we want save more power, the increased latency is affordable.
>

Which clearly contradicts what you said above.

>> defeating the purpose of S2 state. Also why you are mixing DVFS here ?
>> PSCI deals with just low power states and has nothing to do with DVFS.
>
> It's not DVFS related, but to put the boot cpu in a low power state while
> still keeping its power.
>

If there's nothing for CPU to do, some CPUFreq governors(e.g.
interactive) will ensure lowest OPP is chosen so you need not do
anything extra I believe.

>>
>> Further IMO we can also achieve a low power state almost close to
>> PM_SUSPEND_FREEZE having run-time PM for all the devices and cpuidle.
>
> we want one more sleep state which saves more power than freeze.
>

Do you have any power/latency number that indicates freeze is not that
power efficient ? Also I imagine you S2 state has higher latency as you
are doing CPU hot-plug.

> Or let's change the question: how to implement "standby" or "s1" in
> Documentation/power/states.txt with the help of PSCI? Seems the PSCI spec
> expects "operating systems use only one suspend to RAM state", but this
> is a limitation in the long run.
>

OK, this one I leave it for you to take up discussion in PSCI forum with
justification(power/latency numbers to prove an extra system state is
really worthy)

Regards,
Sudeep
Lorenzo Pieralisi Sept. 14, 2015, 1:23 p.m. UTC | #9
On Thu, Jun 18, 2015 at 03:41:34PM +0100, Sudeep Holla wrote:
> PSCI v1.0 introduces a new API called PSCI_SYSTEM_SUSPEND. This API
> provides the mechanism by which the calling OS can request entry into
> the deepest possible system sleep state.
> 
> It meets all the necessary preconditions for entering suspend to RAM
> state in Linux. This patch adds support for PSCI_SYSTEM_SUSPEND in psci
> firmware and registers a psci system suspend operation to implement the
> suspend-to-RAM(s2r) in a generic way on all the platforms implementing
> PSCI.
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/psci.c   | 31 +++++++++++++++++++++++++++++++
>  include/uapi/linux/psci.h |  3 +++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index 752ca7c9eb97..f2b2b3e1c6e4 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -20,11 +20,13 @@
>  #include <linux/printk.h>
>  #include <linux/psci.h>
>  #include <linux/reboot.h>
> +#include <linux/suspend.h>
>  
>  #include <uapi/linux/psci.h>
>  
>  #include <asm/system_misc.h>
>  #include <asm/smp_plat.h>
> +#include <asm/suspend.h>
>  
>  /*
>   * While a 64-bit OS can make calls with SMC32 calling conventions, for some
> @@ -222,6 +224,33 @@ static int __init psci_features(u32 psci_func_id)
>  			      psci_func_id, 0, 0);
>  }
>  
> +static int psci_system_suspend(unsigned long unused)
> +{
> +	return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
> +			      virt_to_phys(cpu_resume), 0, 0);
> +}
> +
> +static int psci_system_suspend_enter(suspend_state_t state)
> +{
> +	return cpu_suspend(0, psci_system_suspend);
> +}
> +
> +static const struct platform_suspend_ops psci_suspend_ops = {
> +	.valid          = suspend_valid_only_mem,
> +	.enter          = psci_system_suspend_enter,
> +};
> +
> +static void __init psci_init_system_suspend(void)
> +{
> +	if (!IS_ENABLED(CONFIG_SUSPEND))
> +		return;
> +
> +	if (psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND)))
> +		return;
> +
> +	suspend_set_ops(&psci_suspend_ops);

For consistency with psci_init_cpu_suspend I rewrote it like this:

static void __init psci_init_system_suspend(void)
{
	int ret;

	if (!IS_ENABLED(CONFIG_SUSPEND))
		return;

	ret = psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND));

	if (ret != PSCI_RET_NOT_SUPPORTED)
		suspend_set_ops(&psci_suspend_ops);
}

Let me know if it is ok with you. Other than that:

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Sudeep Holla Sept. 14, 2015, 1:32 p.m. UTC | #10
On 14/09/15 14:23, Lorenzo Pieralisi wrote:
> On Thu, Jun 18, 2015 at 03:41:34PM +0100, Sudeep Holla wrote:
>> PSCI v1.0 introduces a new API called PSCI_SYSTEM_SUSPEND. This API
>> provides the mechanism by which the calling OS can request entry into
>> the deepest possible system sleep state.
>>
>> It meets all the necessary preconditions for entering suspend to RAM
>> state in Linux. This patch adds support for PSCI_SYSTEM_SUSPEND in psci
>> firmware and registers a psci system suspend operation to implement the
>> suspend-to-RAM(s2r) in a generic way on all the platforms implementing
>> PSCI.
>>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>   drivers/firmware/psci.c   | 31 +++++++++++++++++++++++++++++++
>>   include/uapi/linux/psci.h |  3 +++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
>> index 752ca7c9eb97..f2b2b3e1c6e4 100644
>> --- a/drivers/firmware/psci.c
>> +++ b/drivers/firmware/psci.c
>> @@ -20,11 +20,13 @@
>>   #include <linux/printk.h>
>>   #include <linux/psci.h>
>>   #include <linux/reboot.h>
>> +#include <linux/suspend.h>
>>
>>   #include <uapi/linux/psci.h>
>>
>>   #include <asm/system_misc.h>
>>   #include <asm/smp_plat.h>
>> +#include <asm/suspend.h>
>>
>>   /*
>>    * While a 64-bit OS can make calls with SMC32 calling conventions, for some
>> @@ -222,6 +224,33 @@ static int __init psci_features(u32 psci_func_id)
>>   			      psci_func_id, 0, 0);
>>   }
>>
>> +static int psci_system_suspend(unsigned long unused)
>> +{
>> +	return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
>> +			      virt_to_phys(cpu_resume), 0, 0);
>> +}
>> +
>> +static int psci_system_suspend_enter(suspend_state_t state)
>> +{
>> +	return cpu_suspend(0, psci_system_suspend);
>> +}
>> +
>> +static const struct platform_suspend_ops psci_suspend_ops = {
>> +	.valid          = suspend_valid_only_mem,
>> +	.enter          = psci_system_suspend_enter,
>> +};
>> +
>> +static void __init psci_init_system_suspend(void)
>> +{
>> +	if (!IS_ENABLED(CONFIG_SUSPEND))
>> +		return;
>> +
>> +	if (psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND)))
>> +		return;
>> +
>> +	suspend_set_ops(&psci_suspend_ops);
>
> For consistency with psci_init_cpu_suspend I rewrote it like this:
>
> static void __init psci_init_system_suspend(void)
> {
> 	int ret;
>
> 	if (!IS_ENABLED(CONFIG_SUSPEND))
> 		return;
>
> 	ret = psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND));
>
> 	if (ret != PSCI_RET_NOT_SUPPORTED)
> 		suspend_set_ops(&psci_suspend_ops);
> }
>
> Let me know if it is ok with you. Other than that:
>

Looks fine to me. Thanks for the fixup.

> Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>

Thanks.

Regards,
Sudeep
diff mbox

Patch

diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 752ca7c9eb97..f2b2b3e1c6e4 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -20,11 +20,13 @@ 
 #include <linux/printk.h>
 #include <linux/psci.h>
 #include <linux/reboot.h>
+#include <linux/suspend.h>
 
 #include <uapi/linux/psci.h>
 
 #include <asm/system_misc.h>
 #include <asm/smp_plat.h>
+#include <asm/suspend.h>
 
 /*
  * While a 64-bit OS can make calls with SMC32 calling conventions, for some
@@ -222,6 +224,33 @@  static int __init psci_features(u32 psci_func_id)
 			      psci_func_id, 0, 0);
 }
 
+static int psci_system_suspend(unsigned long unused)
+{
+	return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
+			      virt_to_phys(cpu_resume), 0, 0);
+}
+
+static int psci_system_suspend_enter(suspend_state_t state)
+{
+	return cpu_suspend(0, psci_system_suspend);
+}
+
+static const struct platform_suspend_ops psci_suspend_ops = {
+	.valid          = suspend_valid_only_mem,
+	.enter          = psci_system_suspend_enter,
+};
+
+static void __init psci_init_system_suspend(void)
+{
+	if (!IS_ENABLED(CONFIG_SUSPEND))
+		return;
+
+	if (psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND)))
+		return;
+
+	suspend_set_ops(&psci_suspend_ops);
+}
+
 static void __init psci_init_cpu_suspend(void)
 {
 	int feature = psci_features(psci_function_id[PSCI_FN_CPU_SUSPEND]);
@@ -311,6 +340,8 @@  static int __init psci_probe(void)
 
 	psci_init_cpu_suspend();
 
+	psci_init_system_suspend();
+
 	return 0;
 }
 
diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
index 2598d7c9bf20..a1a2d85a91b9 100644
--- a/include/uapi/linux/psci.h
+++ b/include/uapi/linux/psci.h
@@ -47,6 +47,9 @@ 
 #define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU	PSCI_0_2_FN64(7)
 
 #define PSCI_1_0_FN_PSCI_FEATURES		PSCI_0_2_FN(10)
+#define PSCI_1_0_FN_SYSTEM_SUSPEND		PSCI_0_2_FN(14)
+
+#define PSCI_1_0_FN64_SYSTEM_SUSPEND		PSCI_0_2_FN64(14)
 
 /* PSCI v0.2 power state encoding for CPU_SUSPEND function */
 #define PSCI_0_2_POWER_STATE_ID_MASK		0xffff