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

[Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>]

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