[PATCH RFC v3 3/3] Documentation: arm: define DT idle states bindings

[Lorenzo Pieralisi]

On Sun, Feb 16, 2014 at 01:39:56AM +0000, Mark Brown wrote:
> On Tue, Feb 11, 2014 at 02:17:53PM +0000, Lorenzo Pieralisi wrote:
> 
> > +According to the Server Base System Architecture document (SBSA, [3]), the
> > +power states an ARM CPU can be put into are identified by the following list:
> 
> > +1 - Running
> > +2 - Idle_standby
> > +3 - Idle_retention
> > +4 - Sleep
> > +5 - Off
> 
> > +ARM platforms implement the power states specified in the SBSA through power
> > +management schemes that allow an OS PM implementation to put the processor in
> > +different idle states (which include states 1 to 4 above; "off" state is not
> > +an idle state since it does not have wake-up capabilities, hence it is not
> > +considered in this document).
> 
> This is explicitly talking about SBSA - is there any restriction with
> regard to non-SBSA systems?  I can't think of any right now and this
> seems purely informational but it might be worth mentioning that
> non-SBSA systems might potentially have other states if the intention is
> to allow that.

It is informational, for nomenclature reasons. I do not think we have a
problem here, I will check if rewording is necessary.

> > +- state node
> > +
> > +	Description: must be child of either the cpu-idle-states node or
> > +		     a state node.
> > +
> > +	The state node name shall be "stateN", where N = {0, 1, ...} is
> > +	the node number; state nodes which are siblings within a single common
> > +	parent node must be given a unique and sequential N value, starting
> > +	from 0.
> 
> This came up with the CPU bindings as well but I'm really not convinced
> that making the naming of the nodes important is great - it's not normal
> for DT and it makes the usual enumeration code not work.  Can we not
> just have a property for state number in the node instead and allow the
> name to be anything?  It seems we even have the index property
> already...

Name can be anything now, acked.
 
> > +	- compatible
> > +		Usage: Required
> > +		Value type: <stringlist>
> > +		Definition: Must be "arm,cpu-idle-state".
> 
> Do we really need this given that the location in the tree is fixed -
> what would we extend it with in future?

I do not think it hurts either, honestly. Unless there is a strong
reason to remove it I would leave it there.

> > +	- index
> > +		Usage: Required
> > +		Value type: <u32>
> > +		Definition: It represents an idle state index, starting from 2.
> > +			    Index 0 represents the processor state "running"
> > +			    and index 1 represents processor mode
> > +			    "idle_standby", entered by executing a wfi
> > +			    instruction (SBSA,[3]); indexes 0 and 1 are
> > +			    standard ARM states that need not be described.
> 
> ...but other numbers are valid.

Now it is sequential {0,1,....} and I defined what it means in terms of
power consumption (ordering).

> > +	- entry-method
> > +		Usage: Required
> > +		Value type: <stringlist>
> > +		Definition: Describes the method by which a CPU enters the
> > +			    idle state. This property is required and must be
> > +			    one of:
> > +
> > +			    - "arm,psci-cpu-suspend"
> > +			      ARM PSCI firmware interface, CPU suspend
> > +			      method[2].
> > +
> > +			    - "[vendor],[method]"
> > +			      An implementation dependent string with
> > +			      format "vendor,method", where vendor is a string
> > +			      denoting the name of the manufacturer and
> > +			      method is a string specifying the mechanism
> > +			      used to enter the idle state.
> 
> Might be worth reversing these - arm,psci-cpu-suspend is just an example
> of a (hopefully very widely used) vendor method.  Given that everyone is
> supposed to be using PSCI might it even be worth making it the default
> and the property optional?

I think it is ok as it is, honestly. It is certainly not through this
property that psci will be mandated, it is just a way to describe an
entry method and it seems fine to me.

> > +	- power-state
> > +		Usage: See definition.
> > +		Value type: <u32>
> > +		Definition: Depends on the entry-method property value.
> > +			    If entry-method is "arm,psci-cpu-suspend":
> > +				# Property is required and represents
> > +				  psci-power-state parameter. Please refer to
> > +				  [2] for PSCI bindings definition.
> 
> Should we just have the entry method nodes define their own properties
> for this stuff?  I guess this goes back to the comment I made on some
> other binding that it might be cleaner to have an explicit PSCI binding
> rather than put PSCI explicitly in indiidual bindings.

I added to the PSCI bindings in this series. OK, I can add something like:
"Refer to entry-method bindings for the property value definition".

> > +	- entry-latency
> > +		Usage: Required
> > +		Value type: <prop-encoded-array>
> > +		Definition: u32 value representing worst case latency
> > +			    in microseconds required to enter the idle state.
> 
> Why is this defined as an array?

For possible extensions.

> > +	- cache-state-retained
> > +		Usage: See definition
> > +		Value type: <none>
> > +		Definition: if present cache memory is retained on power down,
> > +			    otherwise it is lost.
> 
> Might be better to define which caches?
> 
> > +		STATE1: state1 {
> > +			compatible = "arm,cpu-idle-state";
> 
> Even if we stick with the node name being meaningful it'd be nice to
> replace the STATEN here with a meaningful state name to improve
> legibility.  

Ok I will add this to v4.

Thanks,
Lorenzo