Re: [PATCH v4 1/6] Documentation: arm: define DT idle states bindings

[Santosh Shilimkar <santosh.shilimkar@ti.com>]

On Wednesday 18 June 2014 01:36 PM, Lorenzo Pieralisi wrote:
> On Fri, Jun 13, 2014 at 06:33:35PM +0100, Nicolas Pitre wrote:

[..]
> Ok, a minor tweak to the diagram above, min-residency should include
> energy costs related to idle entry and exit, but not the exit-latency
> itself, as long as the energy costs implied by exiting the state are
> factored out in the min-residency-us property.
> 
> Hence, to sum it up, I attached below the updated bindings patch:
> 
> I think we are close to an agreement, if anyone disagrees please shout
> as soon as possible so that we can still integrate changes.
> 

[..]

> 
> -- >8 --
> Subject: [PATCH] Documentation: arm: define DT idle states bindings
> 
> ARM based platforms implement a variety of power management schemes that
> allow processors to enter idle states at run-time.
> The parameters defining these idle states vary on a per-platform basis forcing
> the OS to hardcode the state parameters in platform specific static tables
> whose size grows as the number of platforms supported in the kernel increases
> and hampers device drivers standardization.
> 
> Therefore, this patch aims at standardizing idle state device tree bindings for
> ARM platforms. Bindings define idle state parameters inclusive of entry methods
> and state latencies, to allow operating systems to retrieve the configuration
> entries from the device tree and initialize the related power management
> drivers, paving the way for common code in the kernel to deal with idle
> states and removing the need for static data in current and previous kernel
> versions.
> 
> Reviewed-by: Sebastian Capella <sebcape@gmail.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
Nice work Lorenzo !!
I have few comments/questions.

>  Documentation/devicetree/bindings/arm/cpus.txt     |   8 +
>  .../devicetree/bindings/arm/idle-states.txt        | 561 +++++++++++++++++++++
>  2 files changed, 569 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/idle-states.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index 1fe72a0..a44d4fd 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -215,6 +215,12 @@ nodes to be present and contain the properties described below.
>  		Value type: <phandle>
>  		Definition: Specifies the ACC[2] node associated with this CPU.
>  
> +	- cpu-idle-states
> +		Usage: Optional
> +		Value type: <prop-encoded-array>
> +		Definition:
> +			# List of phandles to idle state nodes supported
> +			  by this cpu [3].
>  
>  Example 1 (dual-cluster big.LITTLE system 32-bit):
>  
> @@ -411,3 +417,5 @@ cpus {
>  --
>  [1] arm/msm/qcom,saw2.txt
>  [2] arm/msm/qcom,kpss-acc.txt
> +[3] ARM Linux kernel documentation - idle states bindings
> +    Documentation/devicetree/bindings/arm/idle-states.txt
> diff --git a/Documentation/devicetree/bindings/arm/idle-states.txt b/Documentation/devicetree/bindings/arm/idle-states.txt
> new file mode 100644
> index 0000000..c9e1ec6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/idle-states.txt
> @@ -0,0 +1,561 @@
> +==========================================
> +ARM idle states binding description
> +==========================================
> +
> +==========================================
> +1 - Introduction
> +==========================================
> +
> +ARM systems contain HW capable of managing power consumption dynamically,
> +where cores can be put in different low-power states (ranging from simple
> +wfi to power gating) according to OSPM policies. The CPU states representing
s/OSPM/OS PM ?
> +the range of dynamic idle states that a processor can enter at run-time, can be
> +specified through device tree bindings representing the parameters required
> +to enter/exit specific idle states on a given processor.
> +
> +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:
> +
> +- Running
> +- Idle_standby
> +- Idle_retention
> +- Sleep
> +- Off
> +
> +The power states described in the SBSA document define the basic CPU states on
> +top of which ARM platforms implement power management schemes that allow an OS
> +PM implementation to put the processor in different idle states (which include
> +states listed above; "off" state is not an idle state since it does not have
> +wake-up capabilities, hence it is not considered in this document).
> +
> +Idle state parameters (eg entry latency) are platform specific and need to be
> +characterized with bindings that provide the required information to OSPM
Ditto
> +code so that it can build the required tables and use them at runtime.
> +
> +The device tree binding definition for ARM idle states is the subject of this
> +document.
> +
> +===========================================
> +2 - idle-states node
> +===========================================
> +
> +ARM processor idle states are defined within the idle-states node, which is
> +a direct child of the cpus node [1] and provides a container where the
> +processor idle states, defined as device tree nodes, are listed.
> +
> +- idle-states node
> +
> +	Usage: Optional - On ARM systems, is a container of processor idle
s/is/it is ?
> +			  states nodes. If the system does not provide CPU
> +			  power management capabilities or the processor just
> +			  supports idle_standby an idle-states node is not
> +			  required.
> +
> +	Description: idle-states node is a container node, where its
> +		     subnodes describe the CPU idle states.
> +
> +	Node name must be "idle-states".
> +
> +	The idle-states node's parent node must be the cpus node.
> +
> +	The idle-states node's child nodes can be:
s/idle-states/idle-state
> +
> +	- one or more state nodes
> +
> +	Any other configuration is considered invalid.
> +
> +	An idle-states node defines the following properties:
> +
> +	- entry-method
> +		Usage: Required
> +		Value type: <stringlist>
> +		Definition: Describes the method by which a CPU enters the
> +			    idle states. This property is required and must be
> +			    one of:
> +
> +			    - "arm,psci"
> +			      ARM PSCI firmware interface [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.
> +
> +The nodes describing the idle states (state) can only be defined within the
> +idle-states node, any other configuration is considered invalid and therefore
> +must be ignored.
> +
> +===========================================
> +3 - state node
> +===========================================
> +
> +A state node represents an idle state description and must be defined as
> +follows:
> +
> +- state node
> +
> +	Description: must be child of the idle-states node
> +
> +	The state node name shall follow standard device tree naming
> +	rules ([5], 2.2.1 "Node names"), in particular state nodes which
> +	are siblings within a single common parent must be given a unique name.
> +
> +	The idle state entered by executing the wfi instruction (idle_standby
> +	SBSA,[3][4]) is considered standard on all ARM platforms and therefore
> +	must not be listed.
> +
> +	To correctly specify idle states timing and energy related properties,
> +	the following definitions identify the different execution phases
> +	a CPU goes through to enter and exit idle states and the implied
> +	energy metrics:
> +
> +	..__[EXEC]__|__[PREP]__|__[ENTRY]__|__[IDLE]__|__[EXIT]__|__[EXEC]__..
> +		    |          |           |          |          |
> +
> +		    |<------ entry ------->|
> +		    |       latency        |
> +						      |<- exit ->|
> +						      |  latency |
> +		    |<-------- min-residency -------->|
> +			       |<-------  wakeup-latency ------->|
> +
I don't know the wakeup latency makes much sense and also correct.
Hardware wakeup latency is actually exit latency. Is it for failed
or abort-able ilde case ? We are adding this as a new parameter
at least from idle states perspective. I think we should just
avoid it.

> +	EXEC:	Normal CPU execution.
> +
> +	PREP:	Preparation phase before committing the hardware to idle mode
> +		like cache flushing. This is abortable on pending wake-up
> +		event conditions. The abort latency is assumed to be negligible
> +		(i.e. less than the ENTRY + EXIT duration). If aborted, CPU
> +		goes back to EXEC. This phase is optional. If not abortable,
> +		this should be included in the ENTRY phase instead.
> +
> +	ENTRY:	The hardware is committed to idle mode. This period must run
> +		to completion up to IDLE before anything else can happen.
> +
> +	IDLE:	This is the actual energy-saving idle period. This may last
> +		between 0 and infinite time, until a wake-up event occurs.
> +
> +	EXIT:	Period during which the CPU is brought back to operational
> +		mode (EXEC).
> +
> +	With the definitions provided above, the following list represents
> +	the valid properties for a state node:
> +
> +	- compatible
> +		Usage: Required
> +		Value type: <stringlist>
> +		Definition: Must be "arm,idle-state".
> +
> +	- logic-state-retained
> +		Usage: See definition
> +		Value type: <none>
> +		Definition: if present logic is retained on state entry,
> +			    otherwise it is lost.
> +
> +	- cache-state-retained
> +		Usage: See definition
> +		Value type: <none>
> +		Definition: if present cache memory is retained on state entry,
> +			    otherwise it is lost.
> +
> +	- entry-method-param
> +		Usage: See definition.
> +		Value type: <u32>
> +		Definition: Depends on the idle-states node entry-method
> +			    property value. Refer to the entry-method bindings
> +			    for this property value definition.
> +
> +	- entry-latency-us
> +		Usage: Required
> +		Value type: <prop-encoded-array>
> +		Definition: u32 value representing worst case latency in
> +			    microseconds required to enter the idle state.
> +			    The exit-latency-us duration may be guaranteed
> +			    only after entry-latency-us has passed.
> +
> +	- exit-latency-us
> +		Usage: Required
> +		Value type: <prop-encoded-array>
> +		Definition: u32 value representing worst case latency
> +			    in microseconds required to exit the idle state.
> +
> +	- min-residency-us
> +		Usage: Required
> +		Value type: <prop-encoded-array>
> +		Definition: u32 value representing minimum residency duration
> +			    in microseconds, inclusive of preparation and
> +			    entry, for this idle state to be considered
> +			    worthwhile energy wise.
> +			    The residency time must take into account the
> +			    energy consumed while entering and exiting the
> +			    idle state and is therefore expected to be
> +			    longer than entry-latency-us.
> +
> +	- wakeup-latency-us:
> +		Usage: Optional
> +		Value type: <prop-encoded-array>
> +		Definition: u32 value representing maximum delay between the
> +			    signaling of a wake-up event and the CPU being
> +			    able to execute normal code again. If omitted,
> +			    this is assumed to be equal to:
> +				entry-latency-us + exit-latency-us
> +
Rest of the patch looks fine by to me.

regards,
Santosh