[PATCH RFC 0/2] ARM: DT: add bindings for system suspend

[PATCH RFC 0/2] ARM: DT: add bindings for system suspend

[Sudeep Holla]

Hi,

There are various attempts on the list to implement system suspend
on ARM64 using CPU Idle. Here is an attempt to standardise it by 
defining DT bindings for the same and hence provide a generic solution
without associating it with CPUIdle.

Currently only PSCI is supported as entry method in this series.

Regards,
Sudeep

P.S: I am on vacation till Jan 31st and will have limited access to
emails, so there might be delays in my response.

Sudeep Holla (2):
  Documentation: arm: define DT bindings for system suspend
  ARM64: psci: implement system suspend using PSCI v0.2 CPU SUSPEND

 Documentation/devicetree/bindings/arm/psci.txt     | 11 +++
 .../devicetree/bindings/arm/system-suspend.txt     | 93 ++++++++++++++++++++++
 arch/arm64/kernel/psci.c                           | 83 ++++++++++++++++---
 3 files changed, 178 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/system-suspend.txt

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 1/2] Documentation: arm: define DT bindings for system suspend

[Sudeep Holla]

ARM based platforms implement unique ways to enter system suspend
(i.e. Suspend to RAM). The mechanism and the parameters defining the
system state vary on a per-platform basis forcing the OS to handle it
in very platform specific way.

Since ARM 32-bit systems had machine specific code, no attempts to
standardize are being made as it provides easy way to implement suspend
operations in a platform specific manner. However, this approach not
only makes maintainance more difficult as the number of platforms
supported increases but also not feasible for ARM64.

This DT binding aims at standardizing the system suspend for ARM
platforms. ARM64 platforms mandates entry-method property in DT for
this system suspend node.

On system implementing PSCI as an enable-method to enter system suspend,
the PSCI CPU suspend method is used on versions upto v0.2 and requires
the power_state parameter to be passed to the PSCI CPU suspend function.

This parameter is platform specific, therefore must be provided by
firmware to the OS in order to enable proper call sequence.

This ARM system suspend DT bindings rely on a property
(i.e. arm,psci-suspend-param) in the PSCI DT bindings that describes
how the PSCI CPU suspend power_state parameter should be defined in DT.

Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/psci.txt     | 11 +++
 .../devicetree/bindings/arm/system-suspend.txt     | 93 ++++++++++++++++++++++
 2 files changed, 104 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/system-suspend.txt

diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
index 5aa40ede0e99..bd3977a2a333 100644
--- a/Documentation/devicetree/bindings/arm/psci.txt
+++ b/Documentation/devicetree/bindings/arm/psci.txt
@@ -61,6 +61,14 @@ Device tree nodes that require usage of PSCI CPU_SUSPEND function (ie idle
 		Definition: power_state parameter to pass to the PSCI
 			    suspend call.
 
+PSCI v0.2 and earlier versions don't have explicit operation for system
+suspend. However, one can implement system suspend using CPU_SUSPEND by
+ensuring every other core except the one executing the CPU_SUSPEND call
+has called into PSCI through a CPU_OFF call.
+
+In such cases, device tree nodes representing system suspend as per the
+bindings in [2] must specify the above "arm,psci-suspend-param" property.
+
 Example:
 
 Case 1: PSCI v0.1 only.
@@ -100,3 +108,6 @@ Case 3: PSCI v0.2 and PSCI v0.1.
 
 [1] Kernel documentation - ARM idle states bindings
     Documentation/devicetree/bindings/arm/idle-states.txt
+
+[2] Kernel documentation - ARM system suspend bindings
+    Documentation/devicetree/bindings/arm/system-suspend.txt
diff --git a/Documentation/devicetree/bindings/arm/system-suspend.txt b/Documentation/devicetree/bindings/arm/system-suspend.txt
new file mode 100644
index 000000000000..15cac4c7fe92
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/system-suspend.txt
@@ -0,0 +1,93 @@
+==========================================
+ARM system suspend binding description
+==========================================
+
+==========================================
+1 - Introduction
+==========================================
+
+System Suspend(commonly known as Suspend to RAM) is a method to remove
+power from most parts of the machine aside from the RAM, which is required
+to restore the machine's state. Because of the large power savings, it is
+widely used in mobile systems like laptops, tablets and smartphones.
+
+Usually most mobile systems enter system suspend state aggressively when
+they are idle even for short time(in seconds) while others systems like
+laptops automatically enter this mode when they running on batteries and
+the lid is closed (and/or the user is inactive for some time(in minutes)).
+
+Most of the devices in the system are deactivated. Non-volatile memory
+(like disk drives, flash, memory card), graphics chips and even the CPU
+are usually deactivated. Only the RAM is powered to keep its contents. On
+resume, only those individual devices/CPUs need to be reinitialized and
+work continues relatively fast.
+
+It is highly hardware specific especially on ARM platforms. Hence we need
+device tree binding definition for ARM system suspend state which is the
+subject of this document in order to provide generic solution.
+
+===========================================
+2 - system suspend node
+===========================================
+
+The system suspend node represents the description of the mechanism to
+enter system suspend state and must be defined as follows:
+
+	- compatible
+		Usage: Required
+		Value type: <stringlist>
+		Definition: Must be "arm,system-suspend";
+
+	- entry-method
+		Value type: <stringlist>
+		Usage and definition depend on ARM architecture version.
+			# On ARM v8 64-bit this property is required and must
+			  be one of:
+			   - "arm,psci" (see bindings in [1])
+
+	- status:
+		Usage: Optional
+		Value type: <string>
+		Definition: A standard device tree property [2] that indicates
+			    the operational status of system suspend.
+			    If present, it shall be:
+			    "okay": to indicate it is operational.
+			    "disabled": to indicate that it has been disabled
+			                in firmware so it is not operational.
+			    By default, it's always enabled if not explicitly
+			    disabled.
+
+	In addition to the properties listed above, it may require additional
+	properties specifics to the entry-method defined, please refer to the
+	corresponding entry-method bindings documentation for details.
+	In the below example using "arm,psci" entry method,
+	"arm,psci-suspend-param" is a PSCI specific property.
+
+	The system suspend node's parent node must be the 'cpus' node.
+
+===========================================
+3 - Examples
+===========================================
+
+Example:
+cpus {
+	/* cpu-map, cpu and idle-states nodes */
+	....
+
+	system-suspend {
+		compatible = "arm,system-suspend";
+		entry-method = "arm,psci";
+		arm,psci-suspend-param = <0x1010000>;
+	};
+
+	....
+};
+===========================================
+4 - References
+===========================================
+
+[1] ARM Linux Kernel documentation - PSCI bindings
+    Documentation/devicetree/bindings/arm/psci.txt
+
+[2] ePAPR standard
+    https://www.power.org/documentation/epapr-version-1-1/
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/2] Documentation: arm: define DT bindings for system suspend

[Jisheng Zhang]

Dear Sudeep,

On Wed, 21 Jan 2015 03:35:54 -0800
Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:

> ARM based platforms implement unique ways to enter system suspend
> (i.e. Suspend to RAM). The mechanism and the parameters defining the
> system state vary on a per-platform basis forcing the OS to handle it
> in very platform specific way.
> 
> Since ARM 32-bit systems had machine specific code, no attempts to
> standardize are being made as it provides easy way to implement suspend
> operations in a platform specific manner. However, this approach not
> only makes maintainance more difficult as the number of platforms
> supported increases but also not feasible for ARM64.
> 
> This DT binding aims at standardizing the system suspend for ARM
> platforms. ARM64 platforms mandates entry-method property in DT for
> this system suspend node.
> 
> On system implementing PSCI as an enable-method to enter system suspend,
> the PSCI CPU suspend method is used on versions upto v0.2 and requires
> the power_state parameter to be passed to the PSCI CPU suspend function.
> 
> This parameter is platform specific, therefore must be provided by
> firmware to the OS in order to enable proper call sequence.
> 
> This ARM system suspend DT bindings rely on a property
> (i.e. arm,psci-suspend-param) in the PSCI DT bindings that describes
> how the PSCI CPU suspend power_state parameter should be defined in DT.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/psci.txt     | 11 +++
>  .../devicetree/bindings/arm/system-suspend.txt     | 93
> ++++++++++++++++++++++ 2 files changed, 104 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/system-suspend.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/psci.txt
> b/Documentation/devicetree/bindings/arm/psci.txt index
> 5aa40ede0e99..bd3977a2a333 100644 ---
> a/Documentation/devicetree/bindings/arm/psci.txt +++
> b/Documentation/devicetree/bindings/arm/psci.txt @@ -61,6 +61,14 @@ Device
> tree nodes that require usage of PSCI CPU_SUSPEND function (ie idle
> Definition: power_state parameter to pass to the PSCI suspend call.
>  
> +PSCI v0.2 and earlier versions don't have explicit operation for system
> +suspend. However, one can implement system suspend using CPU_SUSPEND by
> +ensuring every other core except the one executing the CPU_SUSPEND call
> +has called into PSCI through a CPU_OFF call.

If users explicitly hot-unplug other cores when system load is low to save
power, then we want to suspend at some point, how does the firmware know this
case?

In my private tree, I extend the PSCI spec to tell firmware we want to suspend
the system.

Thanks,
Jisheng
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/2] Documentation: arm: define DT bindings for system suspend

[Jisheng Zhang]

Dear Sudeep,

On Wed, 21 Jan 2015 05:21:39 -0800
Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> wrote:

> Dear Sudeep,
> 
> On Wed, 21 Jan 2015 03:35:54 -0800
> Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
> 
> > ARM based platforms implement unique ways to enter system suspend
> > (i.e. Suspend to RAM). The mechanism and the parameters defining the
> > system state vary on a per-platform basis forcing the OS to handle it
> > in very platform specific way.
> > 
> > Since ARM 32-bit systems had machine specific code, no attempts to
> > standardize are being made as it provides easy way to implement suspend
> > operations in a platform specific manner. However, this approach not
> > only makes maintainance more difficult as the number of platforms
> > supported increases but also not feasible for ARM64.
> > 
> > This DT binding aims at standardizing the system suspend for ARM
> > platforms. ARM64 platforms mandates entry-method property in DT for
> > this system suspend node.
> > 
> > On system implementing PSCI as an enable-method to enter system suspend,
> > the PSCI CPU suspend method is used on versions upto v0.2 and requires
> > the power_state parameter to be passed to the PSCI CPU suspend function.
> > 
> > This parameter is platform specific, therefore must be provided by
> > firmware to the OS in order to enable proper call sequence.
> > 
> > This ARM system suspend DT bindings rely on a property
> > (i.e. arm,psci-suspend-param) in the PSCI DT bindings that describes
> > how the PSCI CPU suspend power_state parameter should be defined in DT.
> > 
> > Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> > ---
> >  Documentation/devicetree/bindings/arm/psci.txt     | 11 +++
> >  .../devicetree/bindings/arm/system-suspend.txt     | 93
> > ++++++++++++++++++++++ 2 files changed, 104 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/arm/system-suspend.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/psci.txt
> > b/Documentation/devicetree/bindings/arm/psci.txt index
> > 5aa40ede0e99..bd3977a2a333 100644 ---
> > a/Documentation/devicetree/bindings/arm/psci.txt +++
> > b/Documentation/devicetree/bindings/arm/psci.txt @@ -61,6 +61,14 @@ Device
> > tree nodes that require usage of PSCI CPU_SUSPEND function (ie idle
> > Definition: power_state parameter to pass to the PSCI suspend call.
> >  
> > +PSCI v0.2 and earlier versions don't have explicit operation for system
> > +suspend. However, one can implement system suspend using CPU_SUSPEND by
> > +ensuring every other core except the one executing the CPU_SUSPEND call
> > +has called into PSCI through a CPU_OFF call.
> 
> If users explicitly hot-unplug other cores when system load is low to save
> power, then we want to suspend at some point, how does the firmware know
> this case?

Sorry for confusion. I mean 

If users explicitly hot-unplug other cores when system load is low to save
power, then at some point cpuidle want to suspend the cluster, how does the
distinguish this case with suspend the system to ram.

> 
> In my private tree, I extend the PSCI spec to tell firmware we want to
> suspend the system.

I extend the PSCI suspend function to help firmware distinguish suspend system
to ram and other suspend cores/clusters.

> 
> Thanks,
> Jisheng
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/2] Documentation: arm: define DT bindings for system suspend

[Lorenzo Pieralisi]

On Wed, Jan 21, 2015 at 01:35:07PM +0000, Jisheng Zhang wrote:
> Dear Sudeep,
> 
> On Wed, 21 Jan 2015 05:21:39 -0800
> Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> > Dear Sudeep,
> > 
> > On Wed, 21 Jan 2015 03:35:54 -0800
> > Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
> > 
> > > ARM based platforms implement unique ways to enter system suspend
> > > (i.e. Suspend to RAM). The mechanism and the parameters defining the
> > > system state vary on a per-platform basis forcing the OS to handle it
> > > in very platform specific way.
> > > 
> > > Since ARM 32-bit systems had machine specific code, no attempts to
> > > standardize are being made as it provides easy way to implement suspend
> > > operations in a platform specific manner. However, this approach not
> > > only makes maintainance more difficult as the number of platforms
> > > supported increases but also not feasible for ARM64.
> > > 
> > > This DT binding aims at standardizing the system suspend for ARM
> > > platforms. ARM64 platforms mandates entry-method property in DT for
> > > this system suspend node.
> > > 
> > > On system implementing PSCI as an enable-method to enter system suspend,
> > > the PSCI CPU suspend method is used on versions upto v0.2 and requires
> > > the power_state parameter to be passed to the PSCI CPU suspend function.
> > > 
> > > This parameter is platform specific, therefore must be provided by
> > > firmware to the OS in order to enable proper call sequence.
> > > 
> > > This ARM system suspend DT bindings rely on a property
> > > (i.e. arm,psci-suspend-param) in the PSCI DT bindings that describes
> > > how the PSCI CPU suspend power_state parameter should be defined in DT.
> > > 
> > > Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> > > ---
> > >  Documentation/devicetree/bindings/arm/psci.txt     | 11 +++
> > >  .../devicetree/bindings/arm/system-suspend.txt     | 93
> > > ++++++++++++++++++++++ 2 files changed, 104 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/arm/system-suspend.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/psci.txt
> > > b/Documentation/devicetree/bindings/arm/psci.txt index
> > > 5aa40ede0e99..bd3977a2a333 100644 ---
> > > a/Documentation/devicetree/bindings/arm/psci.txt +++
> > > b/Documentation/devicetree/bindings/arm/psci.txt @@ -61,6 +61,14 @@ Device
> > > tree nodes that require usage of PSCI CPU_SUSPEND function (ie idle
> > > Definition: power_state parameter to pass to the PSCI suspend call.
> > >  
> > > +PSCI v0.2 and earlier versions don't have explicit operation for system
> > > +suspend. However, one can implement system suspend using CPU_SUSPEND by
> > > +ensuring every other core except the one executing the CPU_SUSPEND call
> > > +has called into PSCI through a CPU_OFF call.
> > 
> > If users explicitly hot-unplug other cores when system load is low to save
> > power, then we want to suspend at some point, how does the firmware know
> > this case?
> 
> Sorry for confusion. I mean 
> 
> If users explicitly hot-unplug other cores when system load is low to save
> power, then at some point cpuidle want to suspend the cluster, how does the
> distinguish this case with suspend the system to ram.

Through the arm,psci-suspend-param DT property, ie PSCI CPU_SUSPEND
power_state parameter.

Did you read the patch :) ?

> > In my private tree, I extend the PSCI spec to tell firmware we want to
> > suspend the system.
> 
> I extend the PSCI suspend function to help firmware distinguish suspend system
> to ram and other suspend cores/clusters.

See above.

Thanks,
Lorenzo

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/2] Documentation: arm: define DT bindings for system suspend

[Jisheng Zhang]

Dear Lorenzo,

On Wed, 21 Jan 2015 05:56:11 -0800
Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:

> On Wed, Jan 21, 2015 at 01:35:07PM +0000, Jisheng Zhang wrote:
> > Dear Sudeep,
> > 
> > On Wed, 21 Jan 2015 05:21:39 -0800
> > Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> wrote:
> > 
> > > Dear Sudeep,
> > > 
> > > On Wed, 21 Jan 2015 03:35:54 -0800
> > > Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
> > > 
> > > > ARM based platforms implement unique ways to enter system suspend
> > > > (i.e. Suspend to RAM). The mechanism and the parameters defining the
> > > > system state vary on a per-platform basis forcing the OS to handle it
> > > > in very platform specific way.
> > > > 
> > > > Since ARM 32-bit systems had machine specific code, no attempts to
> > > > standardize are being made as it provides easy way to implement
> > > > suspend operations in a platform specific manner. However, this
> > > > approach not only makes maintainance more difficult as the number of
> > > > platforms supported increases but also not feasible for ARM64.
> > > > 
> > > > This DT binding aims at standardizing the system suspend for ARM
> > > > platforms. ARM64 platforms mandates entry-method property in DT for
> > > > this system suspend node.
> > > > 
> > > > On system implementing PSCI as an enable-method to enter system
> > > > suspend, the PSCI CPU suspend method is used on versions upto v0.2
> > > > and requires the power_state parameter to be passed to the PSCI CPU
> > > > suspend function.
> > > > 
> > > > This parameter is platform specific, therefore must be provided by
> > > > firmware to the OS in order to enable proper call sequence.
> > > > 
> > > > This ARM system suspend DT bindings rely on a property
> > > > (i.e. arm,psci-suspend-param) in the PSCI DT bindings that describes
> > > > how the PSCI CPU suspend power_state parameter should be defined in
> > > > DT.
> > > > 
> > > > Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/arm/psci.txt     | 11 +++
> > > >  .../devicetree/bindings/arm/system-suspend.txt     | 93
> > > > ++++++++++++++++++++++ 2 files changed, 104 insertions(+)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/arm/system-suspend.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/arm/psci.txt
> > > > b/Documentation/devicetree/bindings/arm/psci.txt index
> > > > 5aa40ede0e99..bd3977a2a333 100644 ---
> > > > a/Documentation/devicetree/bindings/arm/psci.txt +++
> > > > b/Documentation/devicetree/bindings/arm/psci.txt @@ -61,6 +61,14 @@
> > > > Device tree nodes that require usage of PSCI CPU_SUSPEND function (ie
> > > > idle Definition: power_state parameter to pass to the PSCI suspend
> > > > call. 
> > > > +PSCI v0.2 and earlier versions don't have explicit operation for
> > > > system +suspend. However, one can implement system suspend using
> > > > CPU_SUSPEND by +ensuring every other core except the one executing
> > > > the CPU_SUSPEND call +has called into PSCI through a CPU_OFF call.
> > > 
> > > If users explicitly hot-unplug other cores when system load is low to
> > > save power, then we want to suspend at some point, how does the
> > > firmware know this case?
> > 
> > Sorry for confusion. I mean 
> > 
> > If users explicitly hot-unplug other cores when system load is low to save
> > power, then at some point cpuidle want to suspend the cluster, how does
> > the distinguish this case with suspend the system to ram.
> 
> Through the arm,psci-suspend-param DT property, ie PSCI CPU_SUSPEND
> power_state parameter.
> 
> Did you read the patch :) ?

Yep, I do read the patch ;) To be honest, I implemented the s2ram similar as
the patch does. But according to PSCI v0.2, "arm,psci-suspend-param = <0x1010000>"
means suspend the cluster. I'm not sure I understand it correctly, "can implement
system suspend using CPU_SUSPEND by ensuring every other core except the one
executing the CPU_SUSPEND call has called into PSCI through a CPU_OFF call" intend
to ask firmware to 

suspend the system if other cores has called into PSCI through a CPU_OFF

or

suspend the cpu cluster if other cores are not CPU_OFF.


I extend the PSCI CPU_SUSPEND function's to use power_state bit[26] to tell firmware
whether suspend to ram or not.

Could you please correct me if I misunderstand something?

Thank you very much,
Jisheng

> 
> > > In my private tree, I extend the PSCI spec to tell firmware we want to
> > > suspend the system.
> > 
> > I extend the PSCI suspend function to help firmware distinguish suspend
> > system to ram and other suspend cores/clusters.
> 
> See above.
> 
> Thanks,
> Lorenzo
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/2] Documentation: arm: define DT bindings for system suspend

[Jisheng Zhang]

Dear Lorenzo and Sudeep,

On Wed, 21 Jan 2015 20:33:14 -0800
Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> wrote:

> Dear Lorenzo,
> 
> On Wed, 21 Jan 2015 05:56:11 -0800
> Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
> 
> > On Wed, Jan 21, 2015 at 01:35:07PM +0000, Jisheng Zhang wrote:
> > > Dear Sudeep,
> > > 
> > > On Wed, 21 Jan 2015 05:21:39 -0800
> > > Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> wrote:
> > > 
> > > > Dear Sudeep,
> > > > 
> > > > On Wed, 21 Jan 2015 03:35:54 -0800
> > > > Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
> > > > 
> > > > > ARM based platforms implement unique ways to enter system suspend
> > > > > (i.e. Suspend to RAM). The mechanism and the parameters defining the
> > > > > system state vary on a per-platform basis forcing the OS to handle
> > > > > it in very platform specific way.
> > > > > 
> > > > > Since ARM 32-bit systems had machine specific code, no attempts to
> > > > > standardize are being made as it provides easy way to implement
> > > > > suspend operations in a platform specific manner. However, this
> > > > > approach not only makes maintainance more difficult as the number of
> > > > > platforms supported increases but also not feasible for ARM64.
> > > > > 
> > > > > This DT binding aims at standardizing the system suspend for ARM
> > > > > platforms. ARM64 platforms mandates entry-method property in DT for
> > > > > this system suspend node.
> > > > > 
> > > > > On system implementing PSCI as an enable-method to enter system
> > > > > suspend, the PSCI CPU suspend method is used on versions upto v0.2
> > > > > and requires the power_state parameter to be passed to the PSCI CPU
> > > > > suspend function.
> > > > > 
> > > > > This parameter is platform specific, therefore must be provided by
> > > > > firmware to the OS in order to enable proper call sequence.
> > > > > 
> > > > > This ARM system suspend DT bindings rely on a property
> > > > > (i.e. arm,psci-suspend-param) in the PSCI DT bindings that describes
> > > > > how the PSCI CPU suspend power_state parameter should be defined in
> > > > > DT.
> > > > > 
> > > > > Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/arm/psci.txt     | 11 +++
> > > > >  .../devicetree/bindings/arm/system-suspend.txt     | 93
> > > > > ++++++++++++++++++++++ 2 files changed, 104 insertions(+)
> > > > >  create mode 100644
> > > > > Documentation/devicetree/bindings/arm/system-suspend.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.txt
> > > > > b/Documentation/devicetree/bindings/arm/psci.txt index
> > > > > 5aa40ede0e99..bd3977a2a333 100644 ---
> > > > > a/Documentation/devicetree/bindings/arm/psci.txt +++
> > > > > b/Documentation/devicetree/bindings/arm/psci.txt @@ -61,6 +61,14 @@
> > > > > Device tree nodes that require usage of PSCI CPU_SUSPEND function
> > > > > (ie idle Definition: power_state parameter to pass to the PSCI
> > > > > suspend call. 
> > > > > +PSCI v0.2 and earlier versions don't have explicit operation for
> > > > > system +suspend. However, one can implement system suspend using
> > > > > CPU_SUSPEND by +ensuring every other core except the one executing
> > > > > the CPU_SUSPEND call +has called into PSCI through a CPU_OFF call.
> > > > 
> > > > If users explicitly hot-unplug other cores when system load is low to
> > > > save power, then we want to suspend at some point, how does the
> > > > firmware know this case?
> > > 
> > > Sorry for confusion. I mean 
> > > 
> > > If users explicitly hot-unplug other cores when system load is low to
> > > save power, then at some point cpuidle want to suspend the cluster, how
> > > does the distinguish this case with suspend the system to ram.
> > 
> > Through the arm,psci-suspend-param DT property, ie PSCI CPU_SUSPEND
> > power_state parameter.
> > 
> > Did you read the patch :) ?
> 
> Yep, I do read the patch ;) To be honest, I implemented the s2ram similar as
> the patch does. But according to PSCI v0.2, "arm,psci-suspend-param =
> <0x1010000>" means suspend the cluster. I'm not sure I understand it
> correctly, "can implement system suspend using CPU_SUSPEND by ensuring
> every other core except the one executing the CPU_SUSPEND call has called
> into PSCI through a CPU_OFF call" intend to ask firmware to 
> 
> suspend the system if other cores has called into PSCI through a CPU_OFF
> 
> or
> 
> suspend the cpu cluster if other cores are not CPU_OFF.
> 
> 
> I extend the PSCI CPU_SUSPEND function's to use power_state bit[26] to tell
> firmware whether suspend to ram or not.
> 

I read the PSCI spec again, power_state bit[0:15] is "platform specific ID",
Is one of these bits used for suspend system?

Thanks,
Jisheng
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/2] Documentation: arm: define DT bindings for system suspend

[Lorenzo Pieralisi]

On Thu, Jan 22, 2015 at 06:29:49AM +0000, Jisheng Zhang wrote:
> Dear Lorenzo and Sudeep,
> 
> On Wed, 21 Jan 2015 20:33:14 -0800
> Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> > Dear Lorenzo,
> > 
> > On Wed, 21 Jan 2015 05:56:11 -0800
> > Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
> > 
> > > On Wed, Jan 21, 2015 at 01:35:07PM +0000, Jisheng Zhang wrote:
> > > > Dear Sudeep,
> > > > 
> > > > On Wed, 21 Jan 2015 05:21:39 -0800
> > > > Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> wrote:
> > > > 
> > > > > Dear Sudeep,
> > > > > 
> > > > > On Wed, 21 Jan 2015 03:35:54 -0800
> > > > > Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
> > > > > 
> > > > > > ARM based platforms implement unique ways to enter system suspend
> > > > > > (i.e. Suspend to RAM). The mechanism and the parameters defining the
> > > > > > system state vary on a per-platform basis forcing the OS to handle
> > > > > > it in very platform specific way.
> > > > > > 
> > > > > > Since ARM 32-bit systems had machine specific code, no attempts to
> > > > > > standardize are being made as it provides easy way to implement
> > > > > > suspend operations in a platform specific manner. However, this
> > > > > > approach not only makes maintainance more difficult as the number of
> > > > > > platforms supported increases but also not feasible for ARM64.
> > > > > > 
> > > > > > This DT binding aims at standardizing the system suspend for ARM
> > > > > > platforms. ARM64 platforms mandates entry-method property in DT for
> > > > > > this system suspend node.
> > > > > > 
> > > > > > On system implementing PSCI as an enable-method to enter system
> > > > > > suspend, the PSCI CPU suspend method is used on versions upto v0.2
> > > > > > and requires the power_state parameter to be passed to the PSCI CPU
> > > > > > suspend function.
> > > > > > 
> > > > > > This parameter is platform specific, therefore must be provided by
> > > > > > firmware to the OS in order to enable proper call sequence.
> > > > > > 
> > > > > > This ARM system suspend DT bindings rely on a property
> > > > > > (i.e. arm,psci-suspend-param) in the PSCI DT bindings that describes
> > > > > > how the PSCI CPU suspend power_state parameter should be defined in
> > > > > > DT.
> > > > > > 
> > > > > > Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/arm/psci.txt     | 11 +++
> > > > > >  .../devicetree/bindings/arm/system-suspend.txt     | 93
> > > > > > ++++++++++++++++++++++ 2 files changed, 104 insertions(+)
> > > > > >  create mode 100644
> > > > > > Documentation/devicetree/bindings/arm/system-suspend.txt
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.txt
> > > > > > b/Documentation/devicetree/bindings/arm/psci.txt index
> > > > > > 5aa40ede0e99..bd3977a2a333 100644 ---
> > > > > > a/Documentation/devicetree/bindings/arm/psci.txt +++
> > > > > > b/Documentation/devicetree/bindings/arm/psci.txt @@ -61,6 +61,14 @@
> > > > > > Device tree nodes that require usage of PSCI CPU_SUSPEND function
> > > > > > (ie idle Definition: power_state parameter to pass to the PSCI
> > > > > > suspend call. 
> > > > > > +PSCI v0.2 and earlier versions don't have explicit operation for
> > > > > > system +suspend. However, one can implement system suspend using
> > > > > > CPU_SUSPEND by +ensuring every other core except the one executing
> > > > > > the CPU_SUSPEND call +has called into PSCI through a CPU_OFF call.
> > > > > 
> > > > > If users explicitly hot-unplug other cores when system load is low to
> > > > > save power, then we want to suspend at some point, how does the
> > > > > firmware know this case?
> > > > 
> > > > Sorry for confusion. I mean 
> > > > 
> > > > If users explicitly hot-unplug other cores when system load is low to
> > > > save power, then at some point cpuidle want to suspend the cluster, how
> > > > does the distinguish this case with suspend the system to ram.
> > > 
> > > Through the arm,psci-suspend-param DT property, ie PSCI CPU_SUSPEND
> > > power_state parameter.
> > > 
> > > Did you read the patch :) ?
> > 
> > Yep, I do read the patch ;) To be honest, I implemented the s2ram similar as
> > the patch does. But according to PSCI v0.2, "arm,psci-suspend-param =
> > <0x1010000>" means suspend the cluster. I'm not sure I understand it
> > correctly, "can implement system suspend using CPU_SUSPEND by ensuring
> > every other core except the one executing the CPU_SUSPEND call has called
> > into PSCI through a CPU_OFF call" intend to ask firmware to 
> > 
> > suspend the system if other cores has called into PSCI through a CPU_OFF
> > 
> > or
> > 
> > suspend the cpu cluster if other cores are not CPU_OFF.
> > 
> > 
> > I extend the PSCI CPU_SUSPEND function's to use power_state bit[26] to tell
> > firmware whether suspend to ram or not.
> > 

And that's what the arm,psci-suspend-param stands for in the
system-state node.

Since system-suspend corresponds supposedly to the highest level of
affinity in the system, I would rather say power_state = 0x3010000
can be used for system suspend (affinity bits[25:24] = 0x3), but we did
not want to force it, probably that's what we should do.

Yes, there is also a platform specific component in power_state
param and you can use that too, we wanted to leave flexibility
to platforms.

PSCI v1.0 will introduce a different separate call for system
suspend, this patch copes with "legacy" versions, as the patch
logs describe.

I agree that the value 0x1010000 was a bad choice for the example, it
is confusing, but it does not mean you _have_ to use that value, is it
clear ?

> I read the PSCI spec again, power_state bit[0:15] is "platform specific ID",
> Is one of these bits used for suspend system?

It is platform specific, you define that :) ! That's the reason why
firmware has to tell the OS what parameter triggers the system-state,
it is platform specific, and we provide a binding to define it and provide
the OS with the correct value to use.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/2] Documentation: arm: define DT bindings for system suspend

[Jisheng Zhang]

Dear Lorenzo,

On Thu, 22 Jan 2015 03:59:06 -0800
Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:

> On Thu, Jan 22, 2015 at 06:29:49AM +0000, Jisheng Zhang wrote:
> > Dear Lorenzo and Sudeep,
> > 
> > On Wed, 21 Jan 2015 20:33:14 -0800
> > Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> wrote:
> > 
> > > Dear Lorenzo,
> > > 
> > > On Wed, 21 Jan 2015 05:56:11 -0800
> > > Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
> > > 
> > > > On Wed, Jan 21, 2015 at 01:35:07PM +0000, Jisheng Zhang wrote:
> > > > > Dear Sudeep,
> > > > > 
> > > > > On Wed, 21 Jan 2015 05:21:39 -0800
> > > > > Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> wrote:
> > > > > 
> > > > > > Dear Sudeep,
> > > > > > 
> > > > > > On Wed, 21 Jan 2015 03:35:54 -0800
> > > > > > Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
> > > > > > 
> > > > > > > ARM based platforms implement unique ways to enter system
> > > > > > > suspend (i.e. Suspend to RAM). The mechanism and the parameters
> > > > > > > defining the system state vary on a per-platform basis forcing
> > > > > > > the OS to handle it in very platform specific way.
> > > > > > > 
> > > > > > > Since ARM 32-bit systems had machine specific code, no attempts
> > > > > > > to standardize are being made as it provides easy way to
> > > > > > > implement suspend operations in a platform specific manner.
> > > > > > > However, this approach not only makes maintainance more
> > > > > > > difficult as the number of platforms supported increases but
> > > > > > > also not feasible for ARM64.
> > > > > > > 
> > > > > > > This DT binding aims at standardizing the system suspend for ARM
> > > > > > > platforms. ARM64 platforms mandates entry-method property in DT
> > > > > > > for this system suspend node.
> > > > > > > 
> > > > > > > On system implementing PSCI as an enable-method to enter system
> > > > > > > suspend, the PSCI CPU suspend method is used on versions upto
> > > > > > > v0.2 and requires the power_state parameter to be passed to the
> > > > > > > PSCI CPU suspend function.
> > > > > > > 
> > > > > > > This parameter is platform specific, therefore must be provided
> > > > > > > by firmware to the OS in order to enable proper call sequence.
> > > > > > > 
> > > > > > > This ARM system suspend DT bindings rely on a property
> > > > > > > (i.e. arm,psci-suspend-param) in the PSCI DT bindings that
> > > > > > > describes how the PSCI CPU suspend power_state parameter should
> > > > > > > be defined in DT.
> > > > > > > 
> > > > > > > Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> > > > > > > ---
> > > > > > >  Documentation/devicetree/bindings/arm/psci.txt     | 11 +++
> > > > > > >  .../devicetree/bindings/arm/system-suspend.txt     | 93
> > > > > > > ++++++++++++++++++++++ 2 files changed, 104 insertions(+)
> > > > > > >  create mode 100644
> > > > > > > Documentation/devicetree/bindings/arm/system-suspend.txt
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/arm/psci.txt
> > > > > > > b/Documentation/devicetree/bindings/arm/psci.txt index
> > > > > > > 5aa40ede0e99..bd3977a2a333 100644 ---
> > > > > > > a/Documentation/devicetree/bindings/arm/psci.txt +++
> > > > > > > b/Documentation/devicetree/bindings/arm/psci.txt @@ -61,6
> > > > > > > +61,14 @@ Device tree nodes that require usage of PSCI
> > > > > > > CPU_SUSPEND function (ie idle Definition: power_state parameter
> > > > > > > to pass to the PSCI suspend call. 
> > > > > > > +PSCI v0.2 and earlier versions don't have explicit operation
> > > > > > > for system +suspend. However, one can implement system suspend
> > > > > > > using CPU_SUSPEND by +ensuring every other core except the one
> > > > > > > executing the CPU_SUSPEND call +has called into PSCI through a
> > > > > > > CPU_OFF call.
> > > > > > 
> > > > > > If users explicitly hot-unplug other cores when system load is
> > > > > > low to save power, then we want to suspend at some point, how
> > > > > > does the firmware know this case?
> > > > > 
> > > > > Sorry for confusion. I mean 
> > > > > 
> > > > > If users explicitly hot-unplug other cores when system load is low
> > > > > to save power, then at some point cpuidle want to suspend the
> > > > > cluster, how does the distinguish this case with suspend the system
> > > > > to ram.
> > > > 
> > > > Through the arm,psci-suspend-param DT property, ie PSCI CPU_SUSPEND
> > > > power_state parameter.
> > > > 
> > > > Did you read the patch :) ?
> > > 
> > > Yep, I do read the patch ;) To be honest, I implemented the s2ram
> > > similar as the patch does. But according to PSCI v0.2,
> > > "arm,psci-suspend-param = <0x1010000>" means suspend the cluster. I'm
> > > not sure I understand it correctly, "can implement system suspend using
> > > CPU_SUSPEND by ensuring every other core except the one executing the
> > > CPU_SUSPEND call has called into PSCI through a CPU_OFF call" intend to
> > > ask firmware to 
> > > 
> > > suspend the system if other cores has called into PSCI through a CPU_OFF
> > > 
> > > or
> > > 
> > > suspend the cpu cluster if other cores are not CPU_OFF.
> > > 
> > > 
> > > I extend the PSCI CPU_SUSPEND function's to use power_state bit[26] to
> > > tell firmware whether suspend to ram or not.
> > > 
> 
> And that's what the arm,psci-suspend-param stands for in the
> system-state node.
> 
> Since system-suspend corresponds supposedly to the highest level of
> affinity in the system, I would rather say power_state = 0x3010000
> can be used for system suspend (affinity bits[25:24] = 0x3), but we did
> not want to force it, probably that's what we should do.
> 
> Yes, there is also a platform specific component in power_state
> param and you can use that too, we wanted to leave flexibility
> to platforms.
> 
> PSCI v1.0 will introduce a different separate call for system
> suspend, this patch copes with "legacy" versions, as the patch
> logs describe.
> 
> I agree that the value 0x1010000 was a bad choice for the example, it
> is confusing, but it does not mean you _have_ to use that value, is it
> clear ?
> 
> > I read the PSCI spec again, power_state bit[0:15] is "platform specific
> > ID", Is one of these bits used for suspend system?
> 
> It is platform specific, you define that :) ! That's the reason why
> firmware has to tell the OS what parameter triggers the system-state,
> it is platform specific, and we provide a binding to define it and provide
> the OS with the correct value to use.
> 
> Lorenzo

Thank you for detailed explanations. Now I got your and the patches' points.
I were just confused by the 0x1010000.

I'll reuse this patch for arm64 suspend system.

Thanks,
Jisheng
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/2] Documentation: arm: define DT bindings for system suspend

[Mark Rutland]

Hi Sudeep,

On Wed, Jan 21, 2015 at 11:35:54AM +0000, Sudeep Holla wrote:
> ARM based platforms implement unique ways to enter system suspend
> (i.e. Suspend to RAM). The mechanism and the parameters defining the
> system state vary on a per-platform basis forcing the OS to handle it
> in very platform specific way.
> 
> Since ARM 32-bit systems had machine specific code, no attempts to
> standardize are being made as it provides easy way to implement suspend
> operations in a platform specific manner. However, this approach not
> only makes maintainance more difficult as the number of platforms
> supported increases but also not feasible for ARM64.
> 
> This DT binding aims at standardizing the system suspend for ARM
> platforms. ARM64 platforms mandates entry-method property in DT for
> this system suspend node.
> 
> On system implementing PSCI as an enable-method to enter system suspend,
> the PSCI CPU suspend method is used on versions upto v0.2 and requires
> the power_state parameter to be passed to the PSCI CPU suspend function.
> 
> This parameter is platform specific, therefore must be provided by
> firmware to the OS in order to enable proper call sequence.
> 
> This ARM system suspend DT bindings rely on a property
> (i.e. arm,psci-suspend-param) in the PSCI DT bindings that describes
> how the PSCI CPU suspend power_state parameter should be defined in DT.

A short while ago (after this posting), the PSCI 1.0 spec [1] was
released, featuring the new (optional) SYSTEM_SUSPEND mechanism intended
for suspend to RAM. This has a standard ID, and its presence can be
detected via the new standard PSCI_FEATURES call.

The fundamental mechanism is identical. We would hot unplug all but one
CPU, and from this remaining CPU we would make a SYSTEM_SUSPEND call.
The major differences are that SYSTEM_SUSPEND can be detected via
PSCI_FEATURES, and doesn't need a state parameter.

Given that the only mandatory addition in PSCI 1.0 over PSCI 0.2 is the
PSCI_FEATURES call (used to detect the presence of SYSTEM_SUSPEND), I do
not believe that implementing this should be a signficant overhead
compared to implementing the CPU_SUSPEND based approach with PSCI 0.2.

So I'd very much prefer that we require a minimal PSCI 1.0 with
SYSTEM_SUSPEND rather than extending CPU_SUSPEND in this manner. Is
anyone attempting to implement suspend to RAM with PSCI 0.1?

Thanks,
Mark.

[1] http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/index.html

> 
> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/psci.txt     | 11 +++
>  .../devicetree/bindings/arm/system-suspend.txt     | 93 ++++++++++++++++++++++
>  2 files changed, 104 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/system-suspend.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
> index 5aa40ede0e99..bd3977a2a333 100644
> --- a/Documentation/devicetree/bindings/arm/psci.txt
> +++ b/Documentation/devicetree/bindings/arm/psci.txt
> @@ -61,6 +61,14 @@ Device tree nodes that require usage of PSCI CPU_SUSPEND function (ie idle
>  		Definition: power_state parameter to pass to the PSCI
>  			    suspend call.
>  
> +PSCI v0.2 and earlier versions don't have explicit operation for system
> +suspend. However, one can implement system suspend using CPU_SUSPEND by
> +ensuring every other core except the one executing the CPU_SUSPEND call
> +has called into PSCI through a CPU_OFF call.
> +
> +In such cases, device tree nodes representing system suspend as per the
> +bindings in [2] must specify the above "arm,psci-suspend-param" property.
> +
>  Example:
>  
>  Case 1: PSCI v0.1 only.
> @@ -100,3 +108,6 @@ Case 3: PSCI v0.2 and PSCI v0.1.
>  
>  [1] Kernel documentation - ARM idle states bindings
>      Documentation/devicetree/bindings/arm/idle-states.txt
> +
> +[2] Kernel documentation - ARM system suspend bindings
> +    Documentation/devicetree/bindings/arm/system-suspend.txt
> diff --git a/Documentation/devicetree/bindings/arm/system-suspend.txt b/Documentation/devicetree/bindings/arm/system-suspend.txt
> new file mode 100644
> index 000000000000..15cac4c7fe92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/system-suspend.txt
> @@ -0,0 +1,93 @@
> +==========================================
> +ARM system suspend binding description
> +==========================================
> +
> +==========================================
> +1 - Introduction
> +==========================================
> +
> +System Suspend(commonly known as Suspend to RAM) is a method to remove
> +power from most parts of the machine aside from the RAM, which is required
> +to restore the machine's state. Because of the large power savings, it is
> +widely used in mobile systems like laptops, tablets and smartphones.
> +
> +Usually most mobile systems enter system suspend state aggressively when
> +they are idle even for short time(in seconds) while others systems like
> +laptops automatically enter this mode when they running on batteries and
> +the lid is closed (and/or the user is inactive for some time(in minutes)).
> +
> +Most of the devices in the system are deactivated. Non-volatile memory
> +(like disk drives, flash, memory card), graphics chips and even the CPU
> +are usually deactivated. Only the RAM is powered to keep its contents. On
> +resume, only those individual devices/CPUs need to be reinitialized and
> +work continues relatively fast.
> +
> +It is highly hardware specific especially on ARM platforms. Hence we need
> +device tree binding definition for ARM system suspend state which is the
> +subject of this document in order to provide generic solution.
> +
> +===========================================
> +2 - system suspend node
> +===========================================
> +
> +The system suspend node represents the description of the mechanism to
> +enter system suspend state and must be defined as follows:
> +
> +	- compatible
> +		Usage: Required
> +		Value type: <stringlist>
> +		Definition: Must be "arm,system-suspend";
> +
> +	- entry-method
> +		Value type: <stringlist>
> +		Usage and definition depend on ARM architecture version.
> +			# On ARM v8 64-bit this property is required and must
> +			  be one of:
> +			   - "arm,psci" (see bindings in [1])
> +
> +	- status:
> +		Usage: Optional
> +		Value type: <string>
> +		Definition: A standard device tree property [2] that indicates
> +			    the operational status of system suspend.
> +			    If present, it shall be:
> +			    "okay": to indicate it is operational.
> +			    "disabled": to indicate that it has been disabled
> +			                in firmware so it is not operational.
> +			    By default, it's always enabled if not explicitly
> +			    disabled.
> +
> +	In addition to the properties listed above, it may require additional
> +	properties specifics to the entry-method defined, please refer to the
> +	corresponding entry-method bindings documentation for details.
> +	In the below example using "arm,psci" entry method,
> +	"arm,psci-suspend-param" is a PSCI specific property.
> +
> +	The system suspend node's parent node must be the 'cpus' node.
> +
> +===========================================
> +3 - Examples
> +===========================================
> +
> +Example:
> +cpus {
> +	/* cpu-map, cpu and idle-states nodes */
> +	....
> +
> +	system-suspend {
> +		compatible = "arm,system-suspend";
> +		entry-method = "arm,psci";
> +		arm,psci-suspend-param = <0x1010000>;
> +	};
> +
> +	....
> +};
> +===========================================
> +4 - References
> +===========================================
> +
> +[1] ARM Linux Kernel documentation - PSCI bindings
> +    Documentation/devicetree/bindings/arm/psci.txt
> +
> +[2] ePAPR standard
> +    https://www.power.org/documentation/epapr-version-1-1/
> -- 
> 1.9.1
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/2] Documentation: arm: define DT bindings for system suspend

[Sudeep Holla]

On 04/02/15 16:10, Mark Rutland wrote:
> Hi Sudeep,
>
> On Wed, Jan 21, 2015 at 11:35:54AM +0000, Sudeep Holla wrote:
>> ARM based platforms implement unique ways to enter system suspend
>> (i.e. Suspend to RAM). The mechanism and the parameters defining the
>> system state vary on a per-platform basis forcing the OS to handle it
>> in very platform specific way.
>>
>> Since ARM 32-bit systems had machine specific code, no attempts to
>> standardize are being made as it provides easy way to implement suspend
>> operations in a platform specific manner. However, this approach not
>> only makes maintainance more difficult as the number of platforms
>> supported increases but also not feasible for ARM64.
>>
>> This DT binding aims at standardizing the system suspend for ARM
>> platforms. ARM64 platforms mandates entry-method property in DT for
>> this system suspend node.
>>
>> On system implementing PSCI as an enable-method to enter system suspend,
>> the PSCI CPU suspend method is used on versions upto v0.2 and requires
>> the power_state parameter to be passed to the PSCI CPU suspend function.
>>
>> This parameter is platform specific, therefore must be provided by
>> firmware to the OS in order to enable proper call sequence.
>>
>> This ARM system suspend DT bindings rely on a property
>> (i.e. arm,psci-suspend-param) in the PSCI DT bindings that describes
>> how the PSCI CPU suspend power_state parameter should be defined in DT.
>
> A short while ago (after this posting), the PSCI 1.0 spec [1] was
> released, featuring the new (optional) SYSTEM_SUSPEND mechanism intended
> for suspend to RAM. This has a standard ID, and its presence can be
> detected via the new standard PSCI_FEATURES call.
>
> The fundamental mechanism is identical. We would hot unplug all but one
> CPU, and from this remaining CPU we would make a SYSTEM_SUSPEND call.
> The major differences are that SYSTEM_SUSPEND can be detected via
> PSCI_FEATURES, and doesn't need a state parameter.
>
> Given that the only mandatory addition in PSCI 1.0 over PSCI 0.2 is the
> PSCI_FEATURES call (used to detect the presence of SYSTEM_SUSPEND), I do
> not believe that implementing this should be a signficant overhead
> compared to implementing the CPU_SUSPEND based approach with PSCI 0.2.
>
> So I'd very much prefer that we require a minimal PSCI 1.0 with
> SYSTEM_SUSPEND rather than extending CPU_SUSPEND in this manner. Is
> anyone attempting to implement suspend to RAM with PSCI 0.1?
>

I too prefer have PSCI v1.0 for SYSTEM SUSPEND support, which eliminates
the need for this binding. But the question is: are silicon vendors
ready to upgrade their firmware to PSCI v1.0 for system system feature
especially since it's one of the fundamental feature needed in Android
systems.

Regards,
Sudeep

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/2] Documentation: arm: define DT bindings for system suspend

[Mark Rutland]

On Thu, Feb 05, 2015 at 01:28:32PM +0000, Sudeep Holla wrote:
> 
> 
> On 04/02/15 16:10, Mark Rutland wrote:
> > Hi Sudeep,
> >
> > On Wed, Jan 21, 2015 at 11:35:54AM +0000, Sudeep Holla wrote:
> >> ARM based platforms implement unique ways to enter system suspend
> >> (i.e. Suspend to RAM). The mechanism and the parameters defining the
> >> system state vary on a per-platform basis forcing the OS to handle it
> >> in very platform specific way.
> >>
> >> Since ARM 32-bit systems had machine specific code, no attempts to
> >> standardize are being made as it provides easy way to implement suspend
> >> operations in a platform specific manner. However, this approach not
> >> only makes maintainance more difficult as the number of platforms
> >> supported increases but also not feasible for ARM64.
> >>
> >> This DT binding aims at standardizing the system suspend for ARM
> >> platforms. ARM64 platforms mandates entry-method property in DT for
> >> this system suspend node.
> >>
> >> On system implementing PSCI as an enable-method to enter system suspend,
> >> the PSCI CPU suspend method is used on versions upto v0.2 and requires
> >> the power_state parameter to be passed to the PSCI CPU suspend function.
> >>
> >> This parameter is platform specific, therefore must be provided by
> >> firmware to the OS in order to enable proper call sequence.
> >>
> >> This ARM system suspend DT bindings rely on a property
> >> (i.e. arm,psci-suspend-param) in the PSCI DT bindings that describes
> >> how the PSCI CPU suspend power_state parameter should be defined in DT.
> >
> > A short while ago (after this posting), the PSCI 1.0 spec [1] was
> > released, featuring the new (optional) SYSTEM_SUSPEND mechanism intended
> > for suspend to RAM. This has a standard ID, and its presence can be
> > detected via the new standard PSCI_FEATURES call.
> >
> > The fundamental mechanism is identical. We would hot unplug all but one
> > CPU, and from this remaining CPU we would make a SYSTEM_SUSPEND call.
> > The major differences are that SYSTEM_SUSPEND can be detected via
> > PSCI_FEATURES, and doesn't need a state parameter.
> >
> > Given that the only mandatory addition in PSCI 1.0 over PSCI 0.2 is the
> > PSCI_FEATURES call (used to detect the presence of SYSTEM_SUSPEND), I do
> > not believe that implementing this should be a signficant overhead
> > compared to implementing the CPU_SUSPEND based approach with PSCI 0.2.
> >
> > So I'd very much prefer that we require a minimal PSCI 1.0 with
> > SYSTEM_SUSPEND rather than extending CPU_SUSPEND in this manner. Is
> > anyone attempting to implement suspend to RAM with PSCI 0.1?
> >
> 
> I too prefer have PSCI v1.0 for SYSTEM SUSPEND support, which eliminates
> the need for this binding. But the question is: are silicon vendors
> ready to upgrade their firmware to PSCI v1.0 for system system feature
> especially since it's one of the fundamental feature needed in Android
> systems.

Sure. That's the question I'd like to know the answer to.

If they're bringing up PSCIv0.2 now, the delta to PSCIv0.1 is not large.

If they already have an implementation baked, then that's a different
scenario.

Regardless, what constitutes a wakeup device is a fundamental question
to answer, along with what in the system (e.g.  peripherals) the kernel
must save/restore the state of.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/2] Documentation: arm: define DT bindings for system suspend

[Sudeep Holla]

On 05/02/15 13:32, Mark Rutland wrote:
> On Thu, Feb 05, 2015 at 01:28:32PM +0000, Sudeep Holla wrote:
>>
>>
>> On 04/02/15 16:10, Mark Rutland wrote:
>>> Hi Sudeep,
>>>
>>> On Wed, Jan 21, 2015 at 11:35:54AM +0000, Sudeep Holla wrote:
>>>> ARM based platforms implement unique ways to enter system suspend
>>>> (i.e. Suspend to RAM). The mechanism and the parameters defining the
>>>> system state vary on a per-platform basis forcing the OS to handle it
>>>> in very platform specific way.
>>>>
>>>> Since ARM 32-bit systems had machine specific code, no attempts to
>>>> standardize are being made as it provides easy way to implement suspend
>>>> operations in a platform specific manner. However, this approach not
>>>> only makes maintainance more difficult as the number of platforms
>>>> supported increases but also not feasible for ARM64.
>>>>
>>>> This DT binding aims at standardizing the system suspend for ARM
>>>> platforms. ARM64 platforms mandates entry-method property in DT for
>>>> this system suspend node.
>>>>
>>>> On system implementing PSCI as an enable-method to enter system suspend,
>>>> the PSCI CPU suspend method is used on versions upto v0.2 and requires
>>>> the power_state parameter to be passed to the PSCI CPU suspend function.
>>>>
>>>> This parameter is platform specific, therefore must be provided by
>>>> firmware to the OS in order to enable proper call sequence.
>>>>
>>>> This ARM system suspend DT bindings rely on a property
>>>> (i.e. arm,psci-suspend-param) in the PSCI DT bindings that describes
>>>> how the PSCI CPU suspend power_state parameter should be defined in DT.
>>>
>>> A short while ago (after this posting), the PSCI 1.0 spec [1] was
>>> released, featuring the new (optional) SYSTEM_SUSPEND mechanism intended
>>> for suspend to RAM. This has a standard ID, and its presence can be
>>> detected via the new standard PSCI_FEATURES call.
>>>
>>> The fundamental mechanism is identical. We would hot unplug all but one
>>> CPU, and from this remaining CPU we would make a SYSTEM_SUSPEND call.
>>> The major differences are that SYSTEM_SUSPEND can be detected via
>>> PSCI_FEATURES, and doesn't need a state parameter.
>>>
>>> Given that the only mandatory addition in PSCI 1.0 over PSCI 0.2 is the
>>> PSCI_FEATURES call (used to detect the presence of SYSTEM_SUSPEND), I do
>>> not believe that implementing this should be a signficant overhead
>>> compared to implementing the CPU_SUSPEND based approach with PSCI 0.2.
>>>
>>> So I'd very much prefer that we require a minimal PSCI 1.0 with
>>> SYSTEM_SUSPEND rather than extending CPU_SUSPEND in this manner. Is
>>> anyone attempting to implement suspend to RAM with PSCI 0.1?
>>>
>>
>> I too prefer have PSCI v1.0 for SYSTEM SUSPEND support, which eliminates
>> the need for this binding. But the question is: are silicon vendors
>> ready to upgrade their firmware to PSCI v1.0 for system system feature
>> especially since it's one of the fundamental feature needed in Android
>> systems.
>
> Sure. That's the question I'd like to know the answer to.
>

Right, I too hope to get response here.

> If they're bringing up PSCIv0.2 now, the delta to PSCIv0.1 is not large.
>

That's true.

> If they already have an implementation baked, then that's a different
> scenario.
>
> Regardless, what constitutes a wakeup device is a fundamental question

IIUC individual devices can expose if they are wake-up capable. E.g.
RTC can be wakeup capable and if it's enabled(sysfs entry exists, I did
use it to test on Juno), it's interrupt is kept unmasked in suspend path
while other devices keep their interrupts masked(can be both at GIC and
source).

> to answer, along with what in the system (e.g.  peripherals) the kernel
> must save/restore the state of.
>

Ideally individual drivers need to take care of saving and restoring 
their state. However there will be always exceptions :)

Regards,
Sudeep

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 2/2] ARM64: psci: implement system suspend using PSCI v0.2 CPU SUSPEND

[Sudeep Holla]

PSCI specifications upto v0.2 and the related kernel back-end
implementation lack a method to enter system wide suspend state.

This patch implements suspend to RAM support for all ARM64 systems
with PSCIv0.2 support using CPU SUSPEND and the new system state DT
bindings.

Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm64/kernel/psci.c | 83 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 74 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index f1dbca7d5c96..8be464747dd6 100644
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -22,6 +22,7 @@
 #include <linux/pm.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#include <linux/suspend.h>
 #include <uapi/linux/psci.h>
 
 #include <asm/compiler.h>
@@ -304,6 +305,75 @@ static void psci_sys_poweroff(void)
 	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
 }
 
+static int psci_suspend_finisher(unsigned long arg)
+{
+	return psci_ops.cpu_suspend(*(struct psci_power_state *)arg,
+				    virt_to_phys(cpu_resume));
+}
+
+#ifdef CONFIG_SUSPEND
+static struct psci_power_state psci_system_suspend_state;
+
+static int psci_system_suspend_enter(suspend_state_t state)
+{
+	if (state != PM_SUSPEND_MEM)
+		return 0;
+
+	/*
+	 * TODO remove pack/unpacking of power_state to do away with
+	 * these ugly type conversions
+	 */
+	return __cpu_suspend((unsigned long)&psci_system_suspend_state,
+			     psci_suspend_finisher);
+}
+
+static const struct platform_suspend_ops psci_suspend_ops = {
+	.valid		= suspend_valid_only_mem,
+	.enter		= psci_system_suspend_enter,
+};
+
+static void __init psci_0_2_system_suspend_init(void)
+{
+	int ret;
+	u32 psci_power_state;
+	const char *entry_method;
+	struct device_node *node;
+
+	if (!psci_ops.cpu_suspend)
+		return; /* -EOPNOTSUPP */
+
+	node = of_find_compatible_node(NULL, NULL, "arm,system-suspend");
+	if (!node || !of_device_is_available(node))
+		return; /* -EOPNOTSUPP */
+
+	if (of_property_read_string(node, "entry-method", &entry_method)) {
+		pr_warn(" * %s missing entry-method property\n", node->full_name);
+		goto exit;
+	}
+
+	if (strcmp(entry_method, "arm,psci"))
+		goto exit; /* out of PSCI scope ignore */
+
+	ret = of_property_read_u32(node, "arm,psci-suspend-param",
+				   &psci_power_state);
+	if (ret) {
+		pr_warn(" * %s missing arm,psci-suspend-param property\n",
+			node->full_name);
+		goto exit;
+	}
+
+	pr_debug("psci-power-state for system suspend %#x\n", psci_power_state);
+
+	psci_power_state_unpack(psci_power_state, &psci_system_suspend_state);
+
+	suspend_set_ops(&psci_suspend_ops);
+exit:
+	of_node_put(node);
+}
+#else
+static void __init psci_0_2_system_suspend_init(void) { }
+#endif
+
 /*
  * PSCI Function IDs for v0.2+ are well defined so use
  * standard values.
@@ -361,6 +431,8 @@ static int __init psci_0_2_init(struct device_node *np)
 
 	pm_power_off = psci_sys_poweroff;
 
+	psci_0_2_system_suspend_init();
+
 out_put_node:
 	of_node_put(np);
 	return err;
@@ -509,14 +581,6 @@ static int cpu_psci_cpu_kill(unsigned int cpu)
 #endif
 #endif
 
-static int psci_suspend_finisher(unsigned long index)
-{
-	struct psci_power_state *state = __this_cpu_read(psci_power_state);
-
-	return psci_ops.cpu_suspend(state[index - 1],
-				    virt_to_phys(cpu_resume));
-}
-
 static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
 {
 	int ret;
@@ -531,7 +595,8 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
 	if (state[index - 1].type == PSCI_POWER_STATE_TYPE_STANDBY)
 		ret = psci_ops.cpu_suspend(state[index - 1], 0);
 	else
-		ret = __cpu_suspend(index, psci_suspend_finisher);
+		ret = __cpu_suspend((unsigned long)&state[index - 1],
+				    psci_suspend_finisher);
 
 	return ret;
 }
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 2/2] ARM64: psci: implement system suspend using PSCI v0.2 CPU SUSPEND

[Leo Yan]

On Wed, Jan 21, 2015 at 11:35:55AM +0000, Sudeep Holla wrote:
> PSCI specifications upto v0.2 and the related kernel back-end
> implementation lack a method to enter system wide suspend state.
> 
> This patch implements suspend to RAM support for all ARM64 systems
> with PSCIv0.2 support using CPU SUSPEND and the new system state DT
> bindings.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> ---
>  arch/arm64/kernel/psci.c | 83 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 74 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
> index f1dbca7d5c96..8be464747dd6 100644
> --- a/arch/arm64/kernel/psci.c
> +++ b/arch/arm64/kernel/psci.c
> @@ -22,6 +22,7 @@
>  #include <linux/pm.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
> +#include <linux/suspend.h>
>  #include <uapi/linux/psci.h>
>  
>  #include <asm/compiler.h>
> @@ -304,6 +305,75 @@ static void psci_sys_poweroff(void)
>  	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
>  }
>  
> +static int psci_suspend_finisher(unsigned long arg)
> +{
> +	return psci_ops.cpu_suspend(*(struct psci_power_state *)arg,
> +				    virt_to_phys(cpu_resume));
> +}
> +
> +#ifdef CONFIG_SUSPEND
> +static struct psci_power_state psci_system_suspend_state;
> +
> +static int psci_system_suspend_enter(suspend_state_t state)
> +{
> +	if (state != PM_SUSPEND_MEM)
> +		return 0;
> +
> +	/*
> +	 * TODO remove pack/unpacking of power_state to do away with
> +	 * these ugly type conversions
> +	 */
> +	return __cpu_suspend((unsigned long)&psci_system_suspend_state,
> +			     psci_suspend_finisher);
> +}
> +
> +static const struct platform_suspend_ops psci_suspend_ops = {
> +	.valid		= suspend_valid_only_mem,
> +	.enter		= psci_system_suspend_enter,
> +};
> +
> +static void __init psci_0_2_system_suspend_init(void)
> +{
> +	int ret;
> +	u32 psci_power_state;
> +	const char *entry_method;
> +	struct device_node *node;
> +
> +	if (!psci_ops.cpu_suspend)
> +		return; /* -EOPNOTSUPP */
> +
> +	node = of_find_compatible_node(NULL, NULL, "arm,system-suspend");
> +	if (!node || !of_device_is_available(node))
> +		return; /* -EOPNOTSUPP */
> +
> +	if (of_property_read_string(node, "entry-method", &entry_method)) {
> +		pr_warn(" * %s missing entry-method property\n", node->full_name);
> +		goto exit;
> +	}
> +
> +	if (strcmp(entry_method, "arm,psci"))
> +		goto exit; /* out of PSCI scope ignore */
> +
> +	ret = of_property_read_u32(node, "arm,psci-suspend-param",
> +				   &psci_power_state);
> +	if (ret) {
> +		pr_warn(" * %s missing arm,psci-suspend-param property\n",
> +			node->full_name);
> +		goto exit;
> +	}
> +
> +	pr_debug("psci-power-state for system suspend %#x\n", psci_power_state);
> +
> +	psci_power_state_unpack(psci_power_state, &psci_system_suspend_state);
> +

How about unify the power states passing for cpu idle and suspend?

below is a example for dts which place all power state into psci
entry, so that idle-states and system suspend both can reference
the power state.

psci {
	compatible = "arm,psci-0.2";
	method = "smc";

        power_state {
                CPU_POWER_OFF: cpu_power_off {
                        state = <0x00010000>;
                };

                CLUSTER_POWER_OFF: cluster_power_off {
                        state = <0x01010000>;
                };

                SOC_SUSPEND: soc_suspend {
                        state = <0x01010001>;
                };
        };
};

> +	suspend_set_ops(&psci_suspend_ops);
> +exit:
> +	of_node_put(node);
> +}
> +#else
> +static void __init psci_0_2_system_suspend_init(void) { }
> +#endif
> +
>  /*
>   * PSCI Function IDs for v0.2+ are well defined so use
>   * standard values.
> @@ -361,6 +431,8 @@ static int __init psci_0_2_init(struct device_node *np)
>  
>  	pm_power_off = psci_sys_poweroff;
>  
> +	psci_0_2_system_suspend_init();
> +
>  out_put_node:
>  	of_node_put(np);
>  	return err;
> @@ -509,14 +581,6 @@ static int cpu_psci_cpu_kill(unsigned int cpu)
>  #endif
>  #endif
>  
> -static int psci_suspend_finisher(unsigned long index)
> -{
> -	struct psci_power_state *state = __this_cpu_read(psci_power_state);
> -
> -	return psci_ops.cpu_suspend(state[index - 1],
> -				    virt_to_phys(cpu_resume));
> -}
> -
>  static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
>  {
>  	int ret;
> @@ -531,7 +595,8 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
>  	if (state[index - 1].type == PSCI_POWER_STATE_TYPE_STANDBY)
>  		ret = psci_ops.cpu_suspend(state[index - 1], 0);
>  	else
> -		ret = __cpu_suspend(index, psci_suspend_finisher);
> +		ret = __cpu_suspend((unsigned long)&state[index - 1],
> +				    psci_suspend_finisher);
>  
>  	return ret;
>  }
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 2/2] ARM64: psci: implement system suspend using PSCI v0.2 CPU SUSPEND

[Lorenzo Pieralisi]

On Thu, Jan 22, 2015 at 06:18:12AM +0000, Leo Yan wrote:

[...]

> How about unify the power states passing for cpu idle and suspend?
> 
> below is a example for dts which place all power state into psci
> entry, so that idle-states and system suspend both can reference
> the power state.
> 
> psci {
> 	compatible = "arm,psci-0.2";
> 	method = "smc";
> 
>         power_state {
>                 CPU_POWER_OFF: cpu_power_off {
>                         state = <0x00010000>;
>                 };
> 
>                 CLUSTER_POWER_OFF: cluster_power_off {
>                         state = <0x01010000>;
>                 };
> 
>                 SOC_SUSPEND: soc_suspend {
>                         state = <0x01010001>;
>                 };
>         };
> };

I do not see why this would help. We would end up with phandles to
the nodes above to get the parameter from idle-states, to me it
looks like churn honestly, I do not see where the improvement is.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 2/2] ARM64: psci: implement system suspend using PSCI v0.2 CPU SUSPEND

[Leo Yan]

hi Lorenzo,

On Thu, Jan 22, 2015 at 12:08:50PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Jan 22, 2015 at 06:18:12AM +0000, Leo Yan wrote:
> 
> [...]
> 
> > How about unify the power states passing for cpu idle and suspend?
> > 
> > below is a example for dts which place all power state into psci
> > entry, so that idle-states and system suspend both can reference
> > the power state.
> > 
> > psci {
> > 	compatible = "arm,psci-0.2";
> > 	method = "smc";
> > 
> >         power_state {
> >                 CPU_POWER_OFF: cpu_power_off {
> >                         state = <0x00010000>;
> >                 };
> > 
> >                 CLUSTER_POWER_OFF: cluster_power_off {
> >                         state = <0x01010000>;
> >                 };
> > 
> >                 SOC_SUSPEND: soc_suspend {
> >                         state = <0x01010001>;
> >                 };
> >         };
> > };
> 
> I do not see why this would help. We would end up with phandles to
> the nodes above to get the parameter from idle-states, to me it
> looks like churn honestly, I do not see where the improvement is.

My original thought is these power states actually are mainly used by
the arm trusted firmware; in kernel, it only need maintain such power
state ids and pass these power state to firmware according to the
requirement from cpuidle or suspend flows.

For cpuidle and suspend, they only need to know the index which
should use and simply pass this index to the function *cpu_suspend(idx)*
will be enough. So finally we also can simplize the code to parse
these power state.

Following upper idea, the dts can be written like this way:

        psci {
        	compatible = "arm,psci-0.2";
        	method = "smc";

                power_state {
                        CPU_POWER_OFF: cpu_power_off {
                                state = <0x00010000>;
                        };

                        CLUSTER_POWER_OFF: cluster_power_off {
                                state = <0x01010000>;
                        };

                        SOC_SUSPEND: soc_suspend {
                                state = <0x01010001>;
                        };
                };
        };

	idle-states {
		entry-method = "arm,psci";

		C1: cpu-power-down {
			compatible = "arm,idle-state";
			arm,psci-state-idx = 0;
			entry-latency-us = <20>;
			exit-latency-us = <40>;
			min-residency-us = <80>;
		};

		MP: cluster-power-down {
			compatible = "arm,idle-state";
			local-timer-stop;
			arm,psci-state-idx = 1;
			entry-latency-us = <50>;
			exit-latency-us = <100>;
			min-residency-us = <250>;
			wakeup-latency-us = <130>;
		};
        };

        system-suspend {
                compatible = "arm,system-suspend";
                entry-method = "arm,psci";
                arm,psci-state-idx = 2;
        };

Before i have not followed up the discussion tightly, so if i miss something
introduce noise for this topic, sorry about that.

Thanks,
Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 2/2] ARM64: psci: implement system suspend using PSCI v0.2 CPU SUSPEND

[Mark Rutland]

On Thu, Jan 22, 2015 at 02:34:43PM +0000, Leo Yan wrote:
> hi Lorenzo,
> 
> On Thu, Jan 22, 2015 at 12:08:50PM +0000, Lorenzo Pieralisi wrote:
> > On Thu, Jan 22, 2015 at 06:18:12AM +0000, Leo Yan wrote:
> > 
> > [...]
> > 
> > > How about unify the power states passing for cpu idle and suspend?
> > > 
> > > below is a example for dts which place all power state into psci
> > > entry, so that idle-states and system suspend both can reference
> > > the power state.
> > > 
> > > psci {
> > > 	compatible = "arm,psci-0.2";
> > > 	method = "smc";
> > > 
> > >         power_state {
> > >                 CPU_POWER_OFF: cpu_power_off {
> > >                         state = <0x00010000>;
> > >                 };
> > > 
> > >                 CLUSTER_POWER_OFF: cluster_power_off {
> > >                         state = <0x01010000>;
> > >                 };
> > > 
> > >                 SOC_SUSPEND: soc_suspend {
> > >                         state = <0x01010001>;
> > >                 };
> > >         };
> > > };
> > 
> > I do not see why this would help. We would end up with phandles to
> > the nodes above to get the parameter from idle-states, to me it
> > looks like churn honestly, I do not see where the improvement is.
> 
> My original thought is these power states actually are mainly used by
> the arm trusted firmware; in kernel, it only need maintain such power
> state ids and pass these power state to firmware according to the
> requirement from cpuidle or suspend flows.
> 
> For cpuidle and suspend, they only need to know the index which
> should use and simply pass this index to the function *cpu_suspend(idx)*
> will be enough. So finally we also can simplize the code to parse
> these power state.
> 
> Following upper idea, the dts can be written like this way:
> 
>         psci {
>         	compatible = "arm,psci-0.2";
>         	method = "smc";
> 
>                 power_state {
>                         CPU_POWER_OFF: cpu_power_off {
>                                 state = <0x00010000>;
>                         };
> 
>                         CLUSTER_POWER_OFF: cluster_power_off {
>                                 state = <0x01010000>;
>                         };
> 
>                         SOC_SUSPEND: soc_suspend {
>                                 state = <0x01010001>;
>                         };
>                 };
>         };

These are only glorified containers for single ID values, and all the
information which could potentially have been shared (e.g.
local-timer-stop) is not.

> 
> 	idle-states {
> 		entry-method = "arm,psci";
> 
> 		C1: cpu-power-down {
> 			compatible = "arm,idle-state";
> 			arm,psci-state-idx = 0;

This implicitly relies on the ordering of nodes above, which is not a
very good idea. As Lorenzo mentioned, we would have to use phandles to
explicitly refer to nodes in this matter.

So all that we've achieved here is replacing the raw state ID with an
indirection to the state ID, and haven't gained any uniformity across
idle and suspend states anyway.

I don't see the point in just indirecting in this manner unless some
additional information is shared.

Mark.

> 			entry-latency-us = <20>;
> 			exit-latency-us = <40>;
> 			min-residency-us = <80>;
> 		};
> 
> 		MP: cluster-power-down {
> 			compatible = "arm,idle-state";
> 			local-timer-stop;
> 			arm,psci-state-idx = 1;
> 			entry-latency-us = <50>;
> 			exit-latency-us = <100>;
> 			min-residency-us = <250>;
> 			wakeup-latency-us = <130>;
> 		};
>         };
> 
>         system-suspend {
>                 compatible = "arm,system-suspend";
>                 entry-method = "arm,psci";
>                 arm,psci-state-idx = 2;
>         };
> 
> Before i have not followed up the discussion tightly, so if i miss something
> introduce noise for this topic, sorry about that.
> 
> Thanks,
> Leo Yan
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html