ARM generic idle states

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.

+Idle state parameters (eg entry latency) are platform specific and

Documentation/devicetree/bindings/arm/idle-states.txt new file +2 -0

+characterized with bindings that provide the required information to OSPM
+code so that it can build the required tables and use them at runtime.

Documentation/devicetree/bindings/arm/idle-states.txt new file +8 -0

+	- 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.
+
+	- exit-latency-us
+		Usage: Required

Quoted 7 lines · Lorenzo Pieralisi

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.

Quoted 7 lines

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.

Following the offline discussion with Charles, I've some comments.

[...]

Quoted 1 line

+Idle state parameters (eg entry latency) are platform specific and

need to be

Quoted 2 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +2 -0

+characterized with bindings that provide the required information to OSPM
+code so that it can build the required tables and use them at runtime.

[...]

Quoted 8 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +8 -0

+	- 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.
+
+	- exit-latency-us
+		Usage: Required

Documentation/devicetree/bindings/arm/idle-states.txt line 152

Quoted 10 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +10 -0

+		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 duration in microseconds
+			    after which this state becomes more energy
+			    efficient than any shallower states.

I think this would benefit from a clearer definition. For example, should the min-residency-us value include or exclude the entry and exit delays? I think it should since that's what the cpuidle code will have to use when testing against expected delay before next wakeup event in any case. Some of your examples don't assume it is the case though, as the min-residency-us is smaller than entry+exit delays.

Also I think we'd need a 4th value to fully characterize a state: worst case wake-up latency for QoS purposes.

Let's illustrate the different periods on a time line to make it clearer (hmmm let's see how this can be managed on a braille display :-O ):

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, we go 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 power-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).

...__[EXEC]__|__[PREP]--|__[ENTRY]__|__[IDLE]__|___[EXIT]_--|__[EXEC]__...
             |          |           |          |            |

             |<-- entry-latency --->|

                                               |<- exit-  ->|
                                               |  latency   |

             |<-------------- min-residency --------------->|

                        |<----- worst_wakeup_latency ------>|

entry-latency: Worst case latency required to enter the idle state. The exit_latency may be guaranteed only after entry-latency has passed.

min-residency: Minimum period, including preparation, entry and exit, for a given power mode to be worthwhile energy wise. It must be at least equal to entry_latency + exit_latency.

worst_wakeup_latency: Maximum delay between the signaling of a wake-up event and the CPU being able to execute normal code again. If not specified, this is assumed to be entry-latency + exit_latency.

Notes:

The cpuidle code would only care about min-residency to select the most appropriate mode based on the expected delay before the next event.

The scheduler will care about the following in the near future:

wakeup_delay = exit_latency + max(entry_latency - (now - entry_timestamp), 0)

In other words, the scheduler would wake up the CPU with the shortest wake-up latency. This wake-up latency must take into account the entry latency if that period has not expired. Here the abortable nature of the PREP period is ignored on purpose because it cannot be relied upon (e.g. if the cache is mostly clean then the PREP deadline may occur much sooner than expected).

And pmqos would only care about worst_wakeup_latency.

So... I hope this is useful. I think the above ascii art could be part of your documentation to explain it all.

Quoted 60 lines · Nicolas Pitre

Let's illustrate the different periods on a time line to make it clearer (hmmm let's see how this can be managed on a braille display :-O ):

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, we go 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 power-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).

...__[EXEC]__|__[PREP]--|__[ENTRY]__|__[IDLE]__|___[EXIT]_--|__[EXEC]__...
             |          |           |          |            |

             |<-- entry-latency --->|

                                               |<- exit-  ->|
                                               |  latency   |

             |<-------------- min-residency --------------->|

                        |<----- worst_wakeup_latency ------>|

entry-latency: Worst case latency required to enter the idle state. The exit_latency may be guaranteed only after entry-latency has passed.

min-residency: Minimum period, including preparation, entry and exit, for a given power mode to be worthwhile energy wise. It must be at least equal to entry_latency + exit_latency.

worst_wakeup_latency: Maximum delay between the signaling of a wake-up event and the CPU being able to execute normal code again. If not specified, this is assumed to be entry-latency + exit_latency.

Notes:

The cpuidle code would only care about min-residency to select the most appropriate mode based on the expected delay before the next event.

The scheduler will care about the following in the near future:

wakeup_delay = exit_latency + max(entry_latency - (now - entry_timestamp), 0)

In other words, the scheduler would wake up the CPU with the shortest wake-up latency. This wake-up latency must take into account the entry latency if that period has not expired. Here the abortable nature of the PREP period is ignored on purpose because it cannot be relied upon (e.g. if the cache is mostly clean then the PREP deadline may occur much sooner than expected).

Quoted 4 lines

And pmqos would only care about worst_wakeup_latency.

So... I hope this is useful. I think the above ascii art could be part of your documentation to explain it all.

I will, it makes perfect sense, let me point out a couple of things:

1) we need 4 properties, 1 optional (worst_wakeup_latency, if not present defaults to entry+exit)
2) is everyone ok, given these definitions, in sorting idle states using min-residency-us as a rank ?

3) CPUidle:
idle_state.exit_latency = worst-wakeup-latency
idle_state.target_residency = min-residency-us

4) PREP (longest period) can be obtained from the other properties, IF it is needed
PREP = (entry + exit) - worst_wakeup (if worst_wakeup omitted, PREP = 0)

Quoted 66 lines · Lorenzo Pieralisi

Quoted 60 lines · Nicolas Pitre

Let's illustrate the different periods on a time line to make it clearer (hmmm let's see how this can be managed on a braille display :-O ):

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, we go 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 power-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).

...__[EXEC]__|__[PREP]--|__[ENTRY]__|__[IDLE]__|___[EXIT]_--|__[EXEC]__...
             |          |           |          |            |

             |<-- entry-latency --->|

                                               |<- exit-  ->|
                                               |  latency   |

             |<-------------- min-residency --------------->|

                        |<----- worst_wakeup_latency ------>|

entry-latency: Worst case latency required to enter the idle state. The exit_latency may be guaranteed only after entry-latency has passed.

min-residency: Minimum period, including preparation, entry and exit, for a given power mode to be worthwhile energy wise. It must be at least equal to entry_latency + exit_latency.

worst_wakeup_latency: Maximum delay between the signaling of a wake-up event and the CPU being able to execute normal code again. If not specified, this is assumed to be entry-latency + exit_latency.

Notes:

The cpuidle code would only care about min-residency to select the most appropriate mode based on the expected delay before the next event.

The scheduler will care about the following in the near future:

wakeup_delay = exit_latency + max(entry_latency - (now - entry_timestamp), 0)

In other words, the scheduler would wake up the CPU with the shortest wake-up latency. This wake-up latency must take into account the entry latency if that period has not expired. Here the abortable nature of the PREP period is ignored on purpose because it cannot be relied upon (e.g. if the cache is mostly clean then the PREP deadline may occur much sooner than expected).

Quoted 4 lines

And pmqos would only care about worst_wakeup_latency.

So... I hope this is useful. I think the above ascii art could be part of your documentation to explain it all.

Quoted 6 lines

I will, it makes perfect sense, let me point out a couple of things:

1) we need 4 properties, 1 optional (worst_wakeup_latency, if not present defaults to entry+exit)
2) is everyone ok, given these definitions, in sorting idle states using min-residency-us as a rank ?

Yes.

Quoted 3 lines

3) CPUidle:
idle_state.exit_latency = worst-wakeup-latency
idle_state.target_residency = min-residency-us

But exit_latency is not necessarily equal to worst-wakeup-latency. We'll need any of those 4 values depending on the context. So I'd add entry_latency and worst_wakeup_latency to struct cpuidle_state. If a driver doesn't initialize entry_latency then it can be left to 0, and if worst_wakeup_latency is 0 then it should be set to entry_latency + exit_latency by the core code.

Quoted 3 lines

4) PREP (longest period) can be obtained from the other properties, IF it is needed
PREP = (entry + exit) - worst_wakeup (if worst_wakeup omitted, PREP = 0)

Sure. However I'd avoid documenting it. As I said this period cannot be relied upon because it can vary a lot and if you miss its deadline you're up for a much longer delay than expected. It is useful if a wake-up event happens during that period and then the latency can be cut short opportunistically. But if we get to the point we need to rely on this period to improve things then it would be a good idea to question why we need to request and immediately abort a state so often to start with.

Quoted 9 lines · Nicolas Pitre

Quoted 2 lines

idle_state.exit_latency = worst-wakeup-latency
idle_state.target_residency = min-residency-us

But exit_latency is not necessarily equal to worst-wakeup-latency. We'll need any of those 4 values depending on the context. So I'd add entry_latency and worst_wakeup_latency to struct cpuidle_state. If a driver doesn't initialize entry_latency then it can be left to 0, and if worst_wakeup_latency is 0 then it should be set to entry_latency + exit_latency by the core code.

Well, that's why I mentioned idle_state.exit_latency, because in CPUidle today, the struct cpuidle_state.exit_latency field corresponds to our worst-wakeup-latency property, not to the exit_latency property; I know it is confusing but at least by defining proper bindings the kernel structures can be updated with clear semantics (I would not rename them for the time being though).

Fields required by the scheduler (ie entry_latency) can be added in the patches that rely on them, when we agreed on the bindings, adding the variables is no big deal.

Quoted 32 lines · Lorenzo Pieralisi

Quoted 31 lines · Nicolas Pitre

Let's illustrate the different periods on a time line to make it clearer (hmmm let's see how this can be managed on a braille display :-O ):

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, we go 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 power-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).

...__[EXEC]__|__[PREP]--|__[ENTRY]__|__[IDLE]__|___[EXIT]_--|__[EXEC]__...
             |          |           |          |            |

             |<-- entry-latency --->|

                                               |<- exit-  ->|
                                               |  latency   |

             |<-------------- min-residency --------------->|

Quoted 8 lines

Quoted 8 lines

                        |<----- worst_wakeup_latency ------>|

entry-latency: Worst case latency required to enter the idle state. The exit_latency may be guaranteed only after entry-latency has passed.

min-residency: Minimum period, including preparation, entry and exit, for a given power mode to be worthwhile energy wise. It must be at least equal to entry_latency + exit_latency.

Quoted 15 lines · Nicolas Pitre

Quoted 13 lines · Lorenzo Pieralisi

Quoted 12 lines · Nicolas Pitre

...__[EXEC]__|__[PREP]--|__[ENTRY]__|__[IDLE]__|___[EXIT]_--|__[EXEC]__...
             |          |           |          |            |
             |<-- entry-latency --->|
                                               |<- exit-  ->|
                                               |  latency   |
             |<-------------- min-residency --------------->|
                        |<----- worst_wakeup_latency ------>|

entry-latency: Worst case latency required to enter the idle state. The exit_latency may be guaranteed only after entry-latency has passed.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

min-residency: Minimum period, including preparation, entry and exit, for a given power mode to be worthwhile energy wise. It must be at least equal to entry_latency + exit_latency.

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.

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 <[email protected]>

Signed-off-by Lorenzo Pieralisi <[email protected]>

---

 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

Documentation/devicetree/bindings/arm/cpus.txt +8 -0

index 1fe72a0..a44d4fd 100644
@@ -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

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

new file mode 100644
index 0000000..c9e1ec6
@@ -0,0 +1,561 @@
+==========================================

Documentation/devicetree/bindings/arm/idle-states.txt new file +11 -0

+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

Documentation/devicetree/bindings/arm/idle-states.txt new file +6 -0

+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.
+
+===========================================

Documentation/devicetree/bindings/arm/idle-states.txt new file +52 -0

+
+	- 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:
+

Documentation/devicetree/bindings/arm/idle-states.txt new file +70 -0

+	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.

On Wednesday 18 June 2014 01:36 PM, Lorenzo Pieralisi wrote:
[..]

Quoted 12 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +12 -0

+	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 |

Documentation/devicetree/bindings/arm/idle-states.txt line 123

Quoted 3 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +3 -0

+		    |<-------- 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.

Quoted 14 lines · Santosh Shilimkar

On Wednesday 18 June 2014 01:36 PM, Lorenzo Pieralisi wrote:
[..]

Quoted 12 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +12 -0

+	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 |

Quoted 8 lines

Documentation/devicetree/bindings/arm/idle-states.txt line 123

Quoted 3 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +3 -0

+		    |<-------- 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.

I explained the rationale for this parameter in a previous email but Lorenzo didn't carry it over. To be clearer, this should be "worst case wake-up latency". It is of interest for PMQOS. This is the maximum delay that can be expected from the moment a wake-up event is signaled and the moment the CPU is back operational. This is more than just exit latency. By default this is entry_latency + exit_latency but when there is an abortable PREP phase then it may be shorter than that.

Quoted 25 lines · Nicolas Pitre

Quoted 14 lines · Santosh Shilimkar

On Wednesday 18 June 2014 01:36 PM, Lorenzo Pieralisi wrote:
[..]

Quoted 12 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +12 -0

+	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 |

Quoted 8 lines

Documentation/devicetree/bindings/arm/idle-states.txt line 123

Quoted 3 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +3 -0

+		    |<-------- 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.

Quoted 8 lines

I explained the rationale for this parameter in a previous email but Lorenzo didn't carry it over. To be clearer, this should be "worst case wake-up latency". It is of interest for PMQOS. This is the maximum delay that can be expected from the moment a wake-up event is signaled and the moment the CPU is back operational. This is more than just exit latency. By default this is entry_latency + exit_latency but when there is an abortable PREP phase then it may be shorter than that.

PMQOS angle is right. It is just that the idle code is not going to do anything with this value. But I see a value adding it instead of some one doing calculation.

Quoted 34 lines · Santosh Shilimkar

Quoted 25 lines · Nicolas Pitre

Quoted 14 lines · Santosh Shilimkar

On Wednesday 18 June 2014 01:36 PM, Lorenzo Pieralisi wrote:
[..]

Quoted 12 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +12 -0

+	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 |

Quoted 8 lines

Documentation/devicetree/bindings/arm/idle-states.txt line 123

Quoted 3 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +3 -0

+		    |<-------- 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.

Quoted 8 lines

I explained the rationale for this parameter in a previous email but Lorenzo didn't carry it over. To be clearer, this should be "worst case wake-up latency". It is of interest for PMQOS. This is the maximum delay that can be expected from the moment a wake-up event is signaled and the moment the CPU is back operational. This is more than just exit latency. By default this is entry_latency + exit_latency but when there is an abortable PREP phase then it may be shorter than that.

Quoted 3 lines

PMQOS angle is right. It is just that the idle code is not going to do anything with this value. But I see a value adding it instead of some one doing calculation.

The idle code should take it into account when a PMQOS restriction is in effect i.e. avoid using those modes whose worst case wake-up latency is too large.

And cpuidle is being migrated into the scheduler as we speak. So some of the values there, namely entry_latency and exit_latency (taken separately for timing purposes) will be directly used by the scheduler to decide which CPU to wake up for example.

So there is fundamentally 4 parameters if we want to comprehensively support all pertinent use cases.

-----Original Message-----
From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
Sent: 18 June 2014 20:27
To: Lorenzo Pieralisi; Nicolas Pitre
Cc: linux-arm-kernel at lists.infradead.org; linux-pm at vger.kernel.org; devicetree at vger.kernel.org; Mark Rutland; Sudeep Holla; Catalin Marinas; Charles Garcia-Tobin; Rob Herring; grant.likely at linaro.org; Peter De Schrijver; Daniel Lezcano; Amit Kucheria; Vincent Guittot; Antti Miettinen; Stephen Boyd; Kevin Hilman; Sebastian Capella; Tomasz Figa; Mark Brown; Paul Walmsley; Chander Kashyap
Subject: Re: [PATCH v4 1/6] Documentation: arm: define DT idle states bindings

Quoted 1 line · Lorenzo Pieralisi

On Fri, Jun 13, 2014 at 06:33:35PM +0100, Nicolas Pitre wrote:

[..]

Quoted 8 lines

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

Quoted 2 lines

as soon as possible so that we can still integrate changes.

[..]

Quoted 5 lines

-- >8 --
Subject: [PATCH] Documentation: arm: define DT idle states bindings

ARM based platforms implement a variety of power management schemes

that

Quoted 2 lines

allow processors to enter idle states at run-time.
The parameters defining these idle states vary on a per-platform

basis forcing

Quoted 1 line

the OS to hardcode the state parameters in platform specific static

tables

Quoted 1 line

whose size grows as the number of platforms supported in the kernel

increases

Quoted 3 lines

and hampers device drivers standardization.

Therefore, this patch aims at standardizing idle state device tree

bindings for

Quoted 1 line

ARM platforms. Bindings define idle state parameters inclusive of

entry methods

Quoted 1 line

and state latencies, to allow operating systems to retrieve the

configuration

Quoted 1 line

entries from the device tree and initialize the related power

management

Quoted 1 line

drivers, paving the way for common code in the kernel to deal with

idle

Quoted 1 line

states and removing the need for static data in current and previous

kernel

Quoted 5 lines

versions.

Reviewed-by Sebastian Capella <[email protected]>

Signed-off-by Lorenzo Pieralisi <[email protected]>

---

Nice work Lorenzo !!
I have few comments/questions.

Quoted 2 lines

 Documentation/devicetree/bindings/arm/cpus.txt     |   8 +
 .../devicetree/bindings/arm/idle-states.txt        | 561

+++++++++++++++++++++

Quoted 2 lines

 2 files changed, 569 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/idle-

states.txt

Review

Quoted 2 lines

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/arm/cpus.txt

Documentation/devicetree/bindings/arm/cpus.txt nodes to be present and contain the properties

Quoted 4 lines

index 1fe72a0..a44d4fd 100644

Documentation/devicetree/bindings/arm/cpus.txt +0 -0

@@ -215,6 +215,12 @@ nodes to be present and contain the properties

described below.

Documentation/devicetree/bindings/arm/cpus.txt nodes to be present and contain the properties · line 216

Quoted 2 lines

Documentation/devicetree/bindings/arm/cpus.txt +0 -0

 		Value type: <phandle>
 		Definition: Specifies the ACC[2] node associated with this

CPU.

Review

Quoted 17 lines

Documentation/devicetree/bindings/arm/cpus.txt +8 -0

+	- 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

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 5 lines

new file mode 100644
index 0000000..c9e1ec6

Documentation/devicetree/bindings/arm/idle-states.txt line 9

Quoted 9 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +9 -0

@@ -0,0 +1,561 @@
+==========================================
+ARM idle states binding description
+==========================================
+
+==========================================
+1 - Introduction
+==========================================
+
+ARM systems contain HW capable of managing power consumption

dynamically,

Documentation/devicetree/bindings/arm/idle-states.txt line 10

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+where cores can be put in different low-power states (ranging from

simple

Documentation/devicetree/bindings/arm/idle-states.txt line 11

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+wfi to power gating) according to OSPM policies. The CPU states

representing
s/OSPM/OS PM ?

Documentation/devicetree/bindings/arm/idle-states.txt line 12

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+the range of dynamic idle states that a processor can enter at run-

time, can be

Documentation/devicetree/bindings/arm/idle-states.txt line 13

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+specified through device tree bindings representing the parameters

required

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 3 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +3 -0

+to enter/exit specific idle states on a given processor.
+
+According to the Server Base System Architecture document (SBSA,

[3]), the

Documentation/devicetree/bindings/arm/idle-states.txt line 17

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+power states an ARM CPU can be put into are identified by the

following list:

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 8 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +8 -0

+
+- Running
+- Idle_standby
+- Idle_retention
+- Sleep
+- Off
+
+The power states described in the SBSA document define the basic CPU

states on

Documentation/devicetree/bindings/arm/idle-states.txt line 26

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+top of which ARM platforms implement power management schemes that

allow an OS

Documentation/devicetree/bindings/arm/idle-states.txt line 27

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+PM implementation to put the processor in different idle states

(which include

Documentation/devicetree/bindings/arm/idle-states.txt line 28

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+states listed above; "off" state is not an idle state since it does

not have

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 3 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +3 -0

+wake-up capabilities, hence it is not considered in this document).
+
+Idle state parameters (eg entry latency) are platform specific and

need to be

Documentation/devicetree/bindings/arm/idle-states.txt line 32

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+characterized with bindings that provide the required information to

OSPM
Ditto

Documentation/devicetree/bindings/arm/idle-states.txt line 33

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+code so that it can build the required tables and use them at

runtime.

Documentation/devicetree/bindings/arm/idle-states.txt line 35

Quoted 2 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +2 -0

+
+The device tree binding definition for ARM idle states is the

subject of this

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 7 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +7 -0

+document.
+
+===========================================
+2 - idle-states node
+===========================================
+
+ARM processor idle states are defined within the idle-states node,

which is

Documentation/devicetree/bindings/arm/idle-states.txt line 43

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+a direct child of the cpus node [1] and provides a container where

the

Documentation/devicetree/bindings/arm/idle-states.txt line 48

Quoted 5 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +5 -0

+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 ?

Documentation/devicetree/bindings/arm/idle-states.txt line 50

Quoted 2 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +2 -0

+			  states nodes. If the system does not provide CPU
+			  power management capabilities or the processor

Documentation/devicetree/bindings/arm/idle-states.txt line 61

Quoted 11 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +11 -0

+			  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

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 12 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +12 -0

+
+	- 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

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 8 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +8 -0

+			    one of:
+
+			    - "arm,psci"
+			      ARM PSCI firmware interface [2].
+
+			    - "[vendor],[method]"
+			      An implementation dependent string with
+			      format "vendor,method", where vendor is a

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 5 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +5 -0

+			      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

Documentation/devicetree/bindings/arm/idle-states.txt line 87

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+idle-states node, any other configuration is considered invalid and

therefore

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 7 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +7 -0

+must be ignored.
+
+===========================================
+3 - state node
+===========================================
+
+A state node represents an idle state description and must be

defined as

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 9 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +9 -0

+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.

Documentation/devicetree/bindings/arm/idle-states.txt line 105

Quoted 2 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +2 -0

+
+	The idle state entered by executing the wfi instruction

(idle_standby

Documentation/devicetree/bindings/arm/idle-states.txt line 106

Quoted 1 line

Documentation/devicetree/bindings/arm/idle-states.txt new file +1 -0

+	SBSA,[3][4]) is considered standard on all ARM platforms and

therefore

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 3 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +3 -0

+	must not be listed.
+
+	To correctly specify idle states timing and energy related

properties,

Documentation/devicetree/bindings/arm/idle-states.txt line 116

Quoted 5 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +5 -0

+	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]

__..

Documentation/devicetree/bindings/arm/idle-states.txt line 123

Quoted 9 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +9 -0

+		    |          |           |          |          |
+
+		    |<------ 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.

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 3 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +3 -0

+	EXEC:	Normal CPU execution.
+
+	PREP:	Preparation phase before committing the hardware to idle

Documentation/devicetree/bindings/arm/idle-states.txt line 128

Quoted 2 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +2 -0

+		like cache flushing. This is abortable on pending wake-up
+		event conditions. The abort latency is assumed to be

negligible

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 5 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +5 -0

+		(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

Quoted 16 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +16 -0

+		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".
+

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 10 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +10 -0

+	- 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,

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 7 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +7 -0

+			    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

Quoted 12 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +12 -0

+			    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

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 8 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +8 -0

+		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

Quoted 7 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +7 -0

+			    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.

Documentation/devicetree/bindings/arm/idle-states.txt line 203

Quoted 10 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +10 -0

+
+	- 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

Quoted 6 lines

-----Original Message-----
From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
Sent: 18 June 2014 20:27
To: Lorenzo Pieralisi; Nicolas Pitre

Quoted 32 lines

Review

Quoted 5 lines

+===========================================
+3 - state node
+===========================================

(quoted diff) +2 -0

+
+A state node represents an idle state description and must be

defined as

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 9 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +9 -0

+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.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	The idle state entered by executing the wfi instruction

(idle_standby

Review

Quoted 1 line

(quoted diff) +1 -0

+	SBSA,[3][4]) is considered standard on all ARM platforms and

therefore

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 3 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +3 -0

+	must not be listed.
+
+	To correctly specify idle states timing and energy related

properties,

Documentation/devicetree/bindings/arm/idle-states.txt line 116

Quoted 5 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +5 -0

+	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]

__..

Quoted 15 lines

Documentation/devicetree/bindings/arm/idle-states.txt line 123

Quoted 9 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +9 -0

+		    |          |           |          |          |
+
+		    |<------ 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.

Hi Santosh,

To me wake up latency makes up a lot of sense. It is not always the same as exit latency, it will depend on your system, and just how smart it is. In some cases the [ENTRY] period may not be negligible in which case exit latency will be less than the wake up latency.
In addition, it will generally always be shorter than entry+exit which is the default value if omitted, this assumes the PREP time is not abortable, but this is the safer assumption to make.
Wake up latency is really the number that folk have in their head for what you'd stick into the pm_qos to veto entry into states when you are latency constrained.
The one thing that really is an optimisation here is having a separate exit latency, which is being proposed for use in core selection for the scheduler.
So if anything was going to be made optional pending new scheduler patches should that not be entry/exit latency?

-----Original Message-----
From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
Sent: 19 June 2014 15:09
To: Charles Garcia-Tobin; Lorenzo Pieralisi; Nicolas Pitre
Cc: linux-arm-kernel at lists.infradead.org; linux-pm at vger.kernel.org; devicetree at vger.kernel.org; Mark Rutland; Sudeep Holla; Catalin Marinas; Rob Herring; grant.likely at linaro.org; Peter De Schrijver; Daniel Lezcano; Amit Kucheria; Vincent Guittot; Antti Miettinen; Stephen Boyd; Kevin Hilman; Sebastian Capella; Tomasz Figa; Mark Brown; Paul Walmsley; Chander Kashyap
Subject: Re: [PATCH v4 1/6] Documentation: arm: define DT idle states bindings

Charles,

Quoted 6 lines · Charles Garcia-Tobin

Quoted 6 lines

-----Original Message-----
From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
Sent: 18 June 2014 20:27
To: Lorenzo Pieralisi; Nicolas Pitre

[..]

Quoted 48 lines

Quoted 32 lines

Review

Quoted 5 lines

+===========================================
+3 - state node
+===========================================

(quoted diff) +2 -0

+
+A state node represents an idle state description and must be

defined as

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 9 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +9 -0

+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.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	The idle state entered by executing the wfi instruction

(idle_standby

Review

Quoted 1 line

(quoted diff) +1 -0

+	SBSA,[3][4]) is considered standard on all ARM platforms and

therefore

Documentation/devicetree/bindings/arm/idle-states.txt

Quoted 3 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +3 -0

+	must not be listed.
+
+	To correctly specify idle states timing and energy related

properties,

Documentation/devicetree/bindings/arm/idle-states.txt line 116

Quoted 5 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +5 -0

+	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]

__..

Quoted 15 lines

Documentation/devicetree/bindings/arm/idle-states.txt line 123

Quoted 9 lines

Documentation/devicetree/bindings/arm/idle-states.txt new file +9 -0

+		    |          |           |          |          |
+
+		    |<------ 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.

Quoted 3 lines

Hi Santosh,

To me wake up latency makes up a lot of sense. It is not always the

same as

Quoted 1 line

exit latency, it will depend on your system, and just how smart it

is. In

Quoted 1 line

some cases the [ENTRY] period may not be negligible in which case

exit

Quoted 2 lines

latency will be less than the wake up latency.
In addition, it will generally always be shorter than entry+exit

which is

Quoted 1 line

the default value if omitted, this assumes the PREP time is not

abortable,

Quoted 2 lines

but this is the safer assumption to make.
Wake up latency is really the number that folk have in their head for

what

Quoted 1 line

you'd stick into the pm_qos to veto entry into states when you are

latency

Quoted 2 lines

constrained.
The one thing that really is an optimisation here is having a

separate exit

Quoted 3 lines

latency, which is being proposed for use in core selection for the scheduler.
So if anything was going to be made optional pending new scheduler

patches

Quoted 2 lines

should that not be entry/exit latency?

PM QOS angle Nico pointed out and its clear. The wakeup latency as such is a
worst case wakeup latency from QOS perspective so considering the aborted idle
case it makes sense to have conservative number which includes entry + exit.

If you look at current idle governors, only exit latency and target residency
is being used. No matter how we represent it, as long idle governor or idle
C-state selection logic gets that information, things should be fine. So
from that view your point of entry/exit optional makes sense considering
wakeup latency can convey that information indirectly.

Regards,
Santosh

Quoted 39 lines · Nicolas Pitre

Quoted 37 lines · Lorenzo Pieralisi

Quoted 36 lines · Nicolas Pitre

Let's illustrate the different periods on a time line to make it clearer (hmmm let's see how this can be managed on a braille display :-O ):

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, we go 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 power-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).

...__[EXEC]__|__[PREP]--|__[ENTRY]__|__[IDLE]__|___[EXIT]_--|__[EXEC]__...
             |          |           |          |            |

             |<-- entry-latency --->|

                                               |<- exit-  ->|
                                               |  latency   |

             |<-------------- min-residency --------------->|

                        |<----- worst_wakeup_latency ------>|

entry-latency: Worst case latency required to enter the idle state. The exit_latency may be guaranteed only after entry-latency has passed.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

min-residency: Minimum period, including preparation, entry and exit, for a given power mode to be worthwhile energy wise. It must be at least equal to entry_latency + exit_latency.

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.

+- state node

Documentation/devicetree/bindings/arm/idle-states.txt new file +36 -0

+
+	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 ------->|
+
+	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

drivers/cpuidle/Kconfig config CPU_IDLE_GOV_MENU · line 36

Quoted 4 lines · Lorenzo Pieralisi

drivers/cpuidle/Kconfig +4 -0

+config OF_IDLE_STATES
+        bool "Idle states DT support"
+	depends on ARM || ARM64
+	default n

The default for default is n already, so you don't have to default to the default's default.

On most common ARM systems, the low-power states a CPU can be put into are not discoverable in HW and require device tree bindings to describe power down suspend operations and idle states parameters.

In order to enable DT based idle states and configure idle drivers, this patch implements the bulk infrastructure required to parse the device tree idle states bindings and initialize the corresponding CPUidle driver states data.

Code that initializes idle states checks the CPU idle driver cpumask so that multiple CPU idle drivers can be initialized through it in the kernel. The CPU idle driver cpumask defines which idle states should be considered valid for the driver, ie idle states that are valid on a set of cpus the idle driver manages.

Signed-off-by Lorenzo Pieralisi <[email protected]>

---
 drivers/cpuidle/Kconfig          |   9 ++
 drivers/cpuidle/Makefile         |   1 +
 drivers/cpuidle/of_idle_states.c | 282 +++++++++++++++++++++++++++++++++++++++
 drivers/cpuidle/of_idle_states.h |   8 ++
 4 files changed, 300 insertions(+)
 create mode 100644 drivers/cpuidle/of_idle_states.c
 create mode 100644 drivers/cpuidle/of_idle_states.h

drivers/cpuidle/Kconfig +0 -0

index 1b96fb9..760ce20 100644

Quoted 33 lines · Lorenzo Pieralisi

On most common ARM systems, the low-power states a CPU can be put into are not discoverable in HW and require device tree bindings to describe power down suspend operations and idle states parameters.

In order to enable DT based idle states and configure idle drivers, this patch implements the bulk infrastructure required to parse the device tree idle states bindings and initialize the corresponding CPUidle driver states data.

Code that initializes idle states checks the CPU idle driver cpumask so that multiple CPU idle drivers can be initialized through it in the kernel. The CPU idle driver cpumask defines which idle states should be considered valid for the driver, ie idle states that are valid on a set of cpus the idle driver manages.

Signed-off-by Lorenzo Pieralisi <[email protected]>

---
 drivers/cpuidle/Kconfig          |   9 ++
 drivers/cpuidle/Makefile         |   1 +
 drivers/cpuidle/of_idle_states.c | 282 +++++++++++++++++++++++++++++++++++++++
 drivers/cpuidle/of_idle_states.h |   8 ++
 4 files changed, 300 insertions(+)
 create mode 100644 drivers/cpuidle/of_idle_states.c
 create mode 100644 drivers/cpuidle/of_idle_states.h

drivers/cpuidle/Kconfig +0 -0

index 1b96fb9..760ce20 100644
@@ -30,6 +30,15 @@ config CPU_IDLE_GOV_MENU
        bool "Menu governor (for tickless system)"
        default y
 

drivers/cpuidle/Kconfig config CPU_IDLE_GOV_MENU · line 33

Quoted 1 line

drivers/cpuidle/Kconfig +1 -0

+config OF_IDLE_STATES

One question here.

Do you want this to be generally useful or is it just ARM-specific?

[CC'ing Preeti and Paul to check their opinions]

Hi Rafael,

Quoted 35 lines · Rafael J. Wysocki

Quoted 33 lines · Lorenzo Pieralisi

On most common ARM systems, the low-power states a CPU can be put into are not discoverable in HW and require device tree bindings to describe power down suspend operations and idle states parameters.

In order to enable DT based idle states and configure idle drivers, this patch implements the bulk infrastructure required to parse the device tree idle states bindings and initialize the corresponding CPUidle driver states data.

Code that initializes idle states checks the CPU idle driver cpumask so that multiple CPU idle drivers can be initialized through it in the kernel. The CPU idle driver cpumask defines which idle states should be considered valid for the driver, ie idle states that are valid on a set of cpus the idle driver manages.

Signed-off-by Lorenzo Pieralisi <[email protected]>

---
 drivers/cpuidle/Kconfig          |   9 ++
 drivers/cpuidle/Makefile         |   1 +
 drivers/cpuidle/of_idle_states.c | 282 +++++++++++++++++++++++++++++++++++++++
 drivers/cpuidle/of_idle_states.h |   8 ++
 4 files changed, 300 insertions(+)
 create mode 100644 drivers/cpuidle/of_idle_states.c
 create mode 100644 drivers/cpuidle/of_idle_states.h

drivers/cpuidle/Kconfig +0 -0

index 1b96fb9..760ce20 100644
@@ -30,6 +30,15 @@ config CPU_IDLE_GOV_MENU
        bool "Menu governor (for tickless system)"
        default y
 

Quoted 5 lines

drivers/cpuidle/Kconfig config CPU_IDLE_GOV_MENU · line 33

Quoted 1 line

drivers/cpuidle/Kconfig +1 -0

+config OF_IDLE_STATES

One question here.

Do you want this to be generally useful or is it just ARM-specific?

The first series was targeting ARM64, then I noticed that it might be used for ARM too (Daniel is working on that). Actually, I discovered that Power and MIPS can reuse at least the code that initializes the states data too, but I have to point out three things:

1) state enter function method: in my bindings it is common for all idle states, need to check if it applies to Power and MIPS too.

2) CPUIDLE_FLAG_TIMER_STOP and how to set it. It is non-trivial to add code that detects what idle states lose the tick device context. At the moment I am adding the flag by default to all idle states apart from standbywfi on ARM, but that can be optimised. Unless we resort to power domains (but that's not trivial), we can add a flag to the idle states in DT (ie local-timer-stop or suchlike) to support that. I think that it will be frowned upon but it is worth trying, would like to know what other people think about this.

3) idle states bindings should be reviewed, I expect them to be valid on other architectures too, but I need acknowledgments.

I think this series is not far from being ready to be upstreamed, I would be certainly happy if it can be reused for other archs too so just let me know.

Thanks !
Lorenzo

Quoted 7 lines

Quoted 1 line

Do you want this to be generally useful or is it just ARM-specific?

The first series was targeting ARM64, then I noticed that it might be used for ARM too (Daniel is working on that). Actually, I discovered that Power and MIPS can reuse at least the code that initializes the states data too, but I have to point out three things:

Quoted 2 lines

1) state enter function method: in my bindings it is common for all idle states, need to check if it applies to Power and MIPS too.

On PowerPC, we have a state enter function for each idle state. It will not be too difficult to consolidate them into one, but that is not on the cards right now.

Quoted 8 lines

2) CPUIDLE_FLAG_TIMER_STOP and how to set it. It is non-trivial to add code that detects what idle states lose the tick device context. At the moment I am adding the flag by default to all idle states apart from standbywfi on ARM, but that can be optimised. Unless we resort to power domains (but that's not trivial), we can add a flag to the idle states in DT (ie local-timer-stop or suchlike) to support that. I think that it will be frowned upon but it is worth trying, would like to know what other people think about this.

On PowerPC we have a bit in flag property of the idle state device node, which determines if timers will stop. Yes, maybe we can fix it at a specific bit but it may be messy.

Quoted 2 lines

3) idle states bindings should be reviewed, I expect them to be valid on other architectures too, but I need acknowledgments.

The major difference as I see it is the idle state bindings. In your patch there is a device node for each idle state. On PowerPC however, currently we have a single node with the property values of this node determining the idle states' name, desc etc..

Besides this, the names of the device tree nodes for idle states could be arch specific to meet some hierarchical requirements in the device tree. This would make it difficult for this driver to parse the idle states based on a generic idle state node name.

Quoted 4 lines

I think this series is not far from being ready to be upstreamed, I would be certainly happy if it can be reused for other archs too so just let me know.

On PowerPC there are a couple of other sanity checks that we ought to do before initializing the driver.

So IMO, although we can press out the above mentioned differences in one way or the other to make way for a generic idle driver which reads from the device tree, I am not in favour of it since it has to concern itself with quite a bit of arch-specific stuff. This would anyway make it less and less of a generic idle driver. So its best to push this patch to be ARM specific.

[CC'ing Preeti and Paul to check their opinions]

Hi Rafael,

Quoted 3 lines

One question here.

Do you want this to be generally useful or is it just ARM-specific?

The first series was targeting ARM64, then I noticed that it might be used for ARM too (Daniel is working on that). Actually, I discovered that Power and MIPS can reuse at least the code that initializes the states data too, but I have to point out three things:

1) state enter function method: in my bindings it is common for all idle states, need to check if it applies to Power and MIPS too.
2) CPUIDLE_FLAG_TIMER_STOP and how to set it. It is non-trivial to add code that detects what idle states lose the tick device context. At the moment I am adding the flag by default to all idle states apart from standbywfi on ARM, but that can be optimised. Unless we resort to power domains (but that's not trivial), we can add a flag to the idle states in DT (ie local-timer-stop or suchlike) to support that. I think that it will be frowned upon but it is worth trying, would like to know what other people think about this.
3) idle states bindings should be reviewed, I expect them to be valid on other architectures too, but I need acknowledgments.

I think this series is not far from being ready to be upstreamed, I would be certainly happy if it can be reused for other archs too so just let me know.

Thanks !
Lorenzo

On most common ARM systems, the low-power states a CPU can be put into are not discoverable in HW and require device tree bindings to describe power down suspend operations and idle states parameters.

In order to enable DT based idle states and configure idle drivers, this patch implements the bulk infrastructure required to parse the device tree idle states bindings and initialize the corresponding CPUidle driver states data.

 create mode 100644 drivers/cpuidle/of_idle_states.c
 create mode 100644 drivers/cpuidle/of_idle_states.h

Quoted 8 lines · Lorenzo Pieralisi

On most common ARM systems, the low-power states a CPU can be put into are not discoverable in HW and require device tree bindings to describe power down suspend operations and idle states parameters.

In order to enable DT based idle states and configure idle drivers, this patch implements the bulk infrastructure required to parse the device tree idle states bindings and initialize the corresponding CPUidle driver states data.

Oh and another pet peeve of mine: given we always talk about "device tree" all the time, could you s/OF/DT/ in the subject? It's been a while that DT has outgrown its OF origins.

Quoted 2 lines

 create mode 100644 drivers/cpuidle/of_idle_states.c
 create mode 100644 drivers/cpuidle/of_idle_states.h

Ditto here, including any new symbols you introduced.

This patch implements a generic CPU idle driver for ARM64 machines.

It relies on the DT idle states infrastructure to initialize idle states count and respective parameters. Current code assumes the driver is managing idle states on all possible CPUs but can be easily generalized to support heterogenous systems and build cpumasks at runtime using MIDRs or DT cpu nodes compatible properties.

Suspend back-ends (eg PSCI) must register a suspend initializer with the CPU idle driver so that the suspend backend call can be detected, and the driver code can call the back-end infrastructure to complete the suspend backend initialization.

Idle state index 0 is always initialized as a simple wfi state, ie always considered present and functional on all ARM64 platforms.

Signed-off-by Lorenzo Pieralisi <[email protected]>

---
  drivers/cpuidle/Kconfig         |   5 ++
  drivers/cpuidle/Kconfig.arm64   |  13 ++++
  drivers/cpuidle/Makefile        |   4 +
  drivers/cpuidle/cpuidle-arm64.c | 168 ++++++++++++++++++++++++++++++++++++++++
  4 files changed, 190 insertions(+)

create mode 100644 drivers/cpuidle/Kconfig.arm64 create mode 100644 drivers/cpuidle/cpuidle-arm64.c

drivers/cpuidle/Kconfig +5 -0

index 760ce20..360c086 100644
@@ -44,6 +44,11 @@ depends on ARM
  source "drivers/cpuidle/Kconfig.arm"
  endmenu
 
+menu "ARM64 CPU Idle Drivers"
+depends on ARM64
+source "drivers/cpuidle/Kconfig.arm64"
+endmenu
+
  menu "MIPS CPU Idle Drivers"
  depends on MIPS
  source "drivers/cpuidle/Kconfig.mips"

drivers/cpuidle/Kconfig.arm64 new file +13 -0

new file mode 100644
index 0000000..b83612c
@@ -0,0 +1,13 @@
+#
+# ARM64 CPU Idle drivers
+#
+
+config ARM64_CPUIDLE
+	bool "Generic ARM64 CPU idle Driver"
+	select OF_IDLE_STATES
+	help
+	  Select this to enable generic cpuidle driver for ARM v8.
+	  It provides a generic idle driver whose idle states are configured
+	  at run-time through DT nodes. The CPUidle suspend backend is
+	  initialized by the device tree parsing code on matching the entry
+	  method to the respective CPU operations.

drivers/cpuidle/Makefile +4 -0

index d5ebf4b..e496242 100644
@@ -23,6 +23,10 @@ obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
  obj-$(CONFIG_MIPS_CPS_CPUIDLE)		+= cpuidle-cps.o
 
  ###############################################################################
+# ARM64 drivers
+obj-$(CONFIG_ARM64_CPUIDLE)		+= cpuidle-arm64.o
+
+###############################################################################
  # POWERPC drivers
  obj-$(CONFIG_PSERIES_CPUIDLE)		+= cpuidle-pseries.o
  obj-$(CONFIG_POWERNV_CPUIDLE)		+= cpuidle-powernv.o

drivers/cpuidle/cpuidle-arm64.c new file +78 -0

new file mode 100644
index 0000000..4c932f8
@@ -0,0 +1,168 @@
+/*
+ * ARM64 generic CPU idle driver.
+ *
+ * Copyright (C) 2014 ARM Ltd.
+ * Author: Lorenzo Pieralisi <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) "CPUidle arm64: " fmt
+
+#include <linux/cpuidle.h>
+#include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include <asm/psci.h>
+#include <asm/suspend.h>
+
+#include "of_idle_states.h"
+
+typedef int (*suspend_init_fn)(struct cpuidle_driver *,
+			       struct device_node *[]);
+
+struct cpu_suspend_ops {
+	const char *id;
+	suspend_init_fn init_fn;
+};
+
+static const struct cpu_suspend_ops suspend_operations[] __initconst = {
+	{"arm,psci", psci_dt_register_idle_states},
+	{}
+};
+
+static __init const struct cpu_suspend_ops *get_suspend_ops(const char *str)
+{
+	int i;
+
+	if (!str)
+		return NULL;
+
+	for (i = 0; suspend_operations[i].id; i++)
+		if (!strcmp(suspend_operations[i].id, str))
+			return &suspend_operations[i];
+
+	return NULL;
+}
+
+/*
+ * arm_enter_idle_state - Programs CPU to enter the specified state
+ *
+ * dev: cpuidle device
+ * drv: cpuidle driver
+ * idx: state index
+ *
+ * Called from the CPUidle framework to program the device to the
+ * specified target state selected by the governor.
+ */
+static int arm_enter_idle_state(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int idx)
+{
+	int ret;
+
+	if (!idx) {
+		cpu_do_idle();
+		return idx;
+	}
+
+	cpu_pm_enter();
+	/*
+	 * Pass idle state index to cpu_suspend which in turn will call
+	 * the CPU ops suspend protocol with idle index as a parameter.
+	 *
+	 * Some states would not require context to be saved and flushed

drivers/cpuidle/cpuidle-arm64.c new file +46 -0

+}
+
+struct cpuidle_driver arm64_idle_driver = {
+	.name = "arm64_idle",
+	.owner = THIS_MODULE,
+};
+
+static struct device_node *state_nodes[CPUIDLE_STATE_MAX] __initdata;
+
+/*
+ * arm64_idle_init
+ *
+ * Registers the arm64 specific cpuidle driver with the cpuidle
+ * framework. It relies on core code to parse the idle states
+ * and initialize them using driver data structures accordingly.
+ */
+static int __init arm64_idle_init(void)
+{
+	int i, ret;
+	const char *entry_method;
+	struct device_node *idle_states_node;
+	const struct cpu_suspend_ops *suspend_init;
+	struct cpuidle_driver *drv = &arm64_idle_driver;
+
+	idle_states_node = of_find_node_by_path("/cpus/idle-states");
+	if (!idle_states_node)
+		return -ENOENT;
+
+	if (of_property_read_string(idle_states_node, "entry-method",
+				    &entry_method)) {
+		pr_warn(" * %s missing entry-method property\n",
+			    idle_states_node->full_name);
+		of_node_put(idle_states_node);
+		ret = -EOPNOTSUPP;
+		goto put_node;
+	}
+
+	suspend_init = get_suspend_ops(entry_method);
+	if (!suspend_init) {
+		pr_warn("Missing suspend initializer\n");
+		ret = -EOPNOTSUPP;
+		goto put_node;
+	}
+
+	/*
+	 * State at index 0 is standby wfi and considered standard

drivers/cpuidle/cpuidle-arm64.c new file +5 -0

+	drv->cpumask = (struct cpumask *) cpu_possible_mask;
+	/*
+	 * Start at index 1, request idle state nodes to be filled
+	 */
+	ret = of_init_idle_driver(drv, state_nodes, 1, true);

drivers/cpuidle/cpuidle-arm64.c new file +8 -0

+
+	ret = cpuidle_register(drv, NULL);
+
+put_node:
+	of_node_put(idle_states_node);
+	return ret;
+}
+device_initcall(arm64_idle_init);

Quoted 30 lines

drivers/cpuidle/cpuidle-arm64.c new file +30 -0

+/*
+ * arm_enter_idle_state - Programs CPU to enter the specified state
+ *
+ * dev: cpuidle device
+ * drv: cpuidle driver
+ * idx: state index
+ *
+ * Called from the CPUidle framework to program the device to the
+ * specified target state selected by the governor.
+ */
+static int arm_enter_idle_state(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int idx)
+{
+	int ret;
+
+	if (!idx) {
+		cpu_do_idle();
+		return idx;
+	}
+
+	cpu_pm_enter();
+	/*
+	 * Pass idle state index to cpu_suspend which in turn will call
+	 * the CPU ops suspend protocol with idle index as a parameter.
+	 *
+	 * Some states would not require context to be saved and flushed
+	 * to DRAM, so calling cpu_suspend would not be stricly necessary.
+	 * When power domains specifications for ARM CPUs are finalized then
+	 * this code can be optimized to prevent saving registers if not
+	 * needed.

drivers/cpuidle/cpuidle-arm64.c line 88

Quoted 6 lines

drivers/cpuidle/cpuidle-arm64.c new file +6 -0

+	 */
+	ret = cpu_suspend(idx);
+
+	cpu_pm_exit();
+
+	return ret ? -1 : idx;

Is it sure cpu_suspend will return always 0 on success ?

Quoted 51 lines

drivers/cpuidle/cpuidle-arm64.c new file +51 -0

+}
+
+struct cpuidle_driver arm64_idle_driver = {
+	.name = "arm64_idle",
+	.owner = THIS_MODULE,
+};
+
+static struct device_node *state_nodes[CPUIDLE_STATE_MAX] __initdata;
+
+/*
+ * arm64_idle_init
+ *
+ * Registers the arm64 specific cpuidle driver with the cpuidle
+ * framework. It relies on core code to parse the idle states
+ * and initialize them using driver data structures accordingly.
+ */
+static int __init arm64_idle_init(void)
+{
+	int i, ret;
+	const char *entry_method;
+	struct device_node *idle_states_node;
+	const struct cpu_suspend_ops *suspend_init;
+	struct cpuidle_driver *drv = &arm64_idle_driver;
+
+	idle_states_node = of_find_node_by_path("/cpus/idle-states");
+	if (!idle_states_node)
+		return -ENOENT;
+
+	if (of_property_read_string(idle_states_node, "entry-method",
+				    &entry_method)) {
+		pr_warn(" * %s missing entry-method property\n",
+			    idle_states_node->full_name);
+		of_node_put(idle_states_node);
+		ret = -EOPNOTSUPP;
+		goto put_node;
+	}
+
+	suspend_init = get_suspend_ops(entry_method);
+	if (!suspend_init) {
+		pr_warn("Missing suspend initializer\n");
+		ret = -EOPNOTSUPP;
+		goto put_node;
+	}
+
+	/*
+	 * State at index 0 is standby wfi and considered standard
+	 * on all ARM platforms. If in some platforms simple wfi
+	 * can't be used as "state 0", DT bindings must be implemented
+	 * to work around this issue and allow installing a special
+	 * handler for idle state index 0.
+	 */

drivers/cpuidle/cpuidle-arm64.c line 144

Quoted 5 lines

drivers/cpuidle/cpuidle-arm64.c new file +5 -0

+	drv->states[0].exit_latency = 1;
+	drv->states[0].target_residency = 1;
+	drv->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
+	strncpy(drv->states[0].name, "ARM WFI", CPUIDLE_NAME_LEN);
+	strncpy(drv->states[0].desc, "ARM WFI", CPUIDLE_DESC_LEN);

Please do not copy the state name and desc strings, they will be converted to 'const char *'.

Quoted 9 lines

drivers/cpuidle/cpuidle-arm64.c new file +9 -0

+	drv->cpumask = (struct cpumask *) cpu_possible_mask;
+	/*
+	 * Start at index 1, request idle state nodes to be filled
+	 */
+	ret = of_init_idle_driver(drv, state_nodes, 1, true);
+	if (ret)
+		goto put_node;
+
+	if (suspend_init->init_fn(drv, state_nodes)) {

drivers/cpuidle/cpuidle-arm64.c line 160

Quoted 6 lines

drivers/cpuidle/cpuidle-arm64.c new file +6 -0

+		ret = -EOPNOTSUPP;
+		goto put_node;
+	}
+
+	for (i = 0; i < drv->state_count; i++)
+		drv->states[i].enter = arm_enter_idle_state;

May be s/arm/arm64/ ?

This patch implements a generic CPU idle driver for ARM64 machines.

It relies on the DT idle states infrastructure to initialize idle states count and respective parameters. Current code assumes the driver is managing idle states on all possible CPUs but can be easily generalized to support heterogenous systems and build cpumasks at runtime using MIDRs or DT cpu nodes compatible properties.

Suspend back-ends (eg PSCI) must register a suspend initializer with the CPU idle driver so that the suspend backend call can be detected, and the driver code can call the back-end infrastructure to complete the suspend backend initialization.

Idle state index 0 is always initialized as a simple wfi state, ie always considered present and functional on all ARM64 platforms.

Signed-off-by Lorenzo Pieralisi <[email protected]>

---
 drivers/cpuidle/Kconfig         |   5 ++
 drivers/cpuidle/Kconfig.arm64   |  13 ++++
 drivers/cpuidle/Makefile        |   4 +
 drivers/cpuidle/cpuidle-arm64.c | 168 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 190 insertions(+)
 create mode 100644 drivers/cpuidle/Kconfig.arm64
 create mode 100644 drivers/cpuidle/cpuidle-arm64.c

drivers/cpuidle/Kconfig +5 -0

index 760ce20..360c086 100644
@@ -44,6 +44,11 @@ depends on ARM
 source "drivers/cpuidle/Kconfig.arm"
 endmenu
 
+menu "ARM64 CPU Idle Drivers"
+depends on ARM64
+source "drivers/cpuidle/Kconfig.arm64"
+endmenu
+
 menu "MIPS CPU Idle Drivers"
 depends on MIPS
 source "drivers/cpuidle/Kconfig.mips"

drivers/cpuidle/Kconfig.arm64 new file +13 -0

new file mode 100644
index 0000000..b83612c
@@ -0,0 +1,13 @@
+#
+# ARM64 CPU Idle drivers
+#
+
+config ARM64_CPUIDLE
+       bool "Generic ARM64 CPU idle Driver"
+       select OF_IDLE_STATES
+       help
+         Select this to enable generic cpuidle driver for ARM v8.
+         It provides a generic idle driver whose idle states are configured
+         at run-time through DT nodes. The CPUidle suspend backend is
+         initialized by the device tree parsing code on matching the entry
+         method to the respective CPU operations.

drivers/cpuidle/Makefile +4 -0

index d5ebf4b..e496242 100644
@@ -23,6 +23,10 @@ obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
 obj-$(CONFIG_MIPS_CPS_CPUIDLE)         += cpuidle-cps.o
 
 ###############################################################################
+# ARM64 drivers
+obj-$(CONFIG_ARM64_CPUIDLE)            += cpuidle-arm64.o
+
+###############################################################################
 # POWERPC drivers
 obj-$(CONFIG_PSERIES_CPUIDLE)          += cpuidle-pseries.o
 obj-$(CONFIG_POWERNV_CPUIDLE)          += cpuidle-powernv.o

drivers/cpuidle/cpuidle-arm64.c new file +168 -1

new file mode 100644
index 0000000..4c932f8
@@ -0,0 +1,168 @@
+/*
+ * ARM64 generic CPU idle driver.
+ *
+ * Copyright (C) 2014 ARM Ltd.
+ * Author: Lorenzo Pieralisi <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) "CPUidle arm64: " fmt
+
+#include <linux/cpuidle.h>
+#include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include <asm/psci.h>
+#include <asm/suspend.h>
+
+#include "of_idle_states.h"
+
+typedef int (*suspend_init_fn)(struct cpuidle_driver *,
+                              struct device_node *[]);
+
+struct cpu_suspend_ops {
+       const char *id;
+       suspend_init_fn init_fn;
+};
+
+static const struct cpu_suspend_ops suspend_operations[] __initconst = {
+       {"arm,psci", psci_dt_register_idle_states},
+       {}
+};
+
+static __init const struct cpu_suspend_ops *get_suspend_ops(const char *str)
+{
+       int i;
+
+       if (!str)
+               return NULL;
+
+       for (i = 0; suspend_operations[i].id; i++)
+               if (!strcmp(suspend_operations[i].id, str))
+                       return &suspend_operations[i];
+
+       return NULL;
+}
+
+/*
+ * arm_enter_idle_state - Programs CPU to enter the specified state
+ *
+ * dev: cpuidle device
+ * drv: cpuidle driver
+ * idx: state index
+ *
+ * Called from the CPUidle framework to program the device to the
+ * specified target state selected by the governor.
+ */
+static int arm_enter_idle_state(struct cpuidle_device *dev,
+                               struct cpuidle_driver *drv, int idx)
+{
+       int ret;
+
+       if (!idx) {
+               cpu_do_idle();
+               return idx;
+       }
+
+       cpu_pm_enter();
+       /*
+        * Pass idle state index to cpu_suspend which in turn will call
+        * the CPU ops suspend protocol with idle index as a parameter.
+        *
+        * Some states would not require context to be saved and flushed
+        * to DRAM, so calling cpu_suspend would not be stricly necessary.
+        * When power domains specifications for ARM CPUs are finalized then
+        * this code can be optimized to prevent saving registers if not
+        * needed.
+        */
+       ret = cpu_suspend(idx);
+
+       cpu_pm_exit();
+
+       return ret ? -1 : idx;
+}
+
+struct cpuidle_driver arm64_idle_driver = {
+       .name = "arm64_idle",
+       .owner = THIS_MODULE,
+};
+
+static struct device_node *state_nodes[CPUIDLE_STATE_MAX] __initdata;
+
+/*
+ * arm64_idle_init
+ *
+ * Registers the arm64 specific cpuidle driver with the cpuidle
+ * framework. It relies on core code to parse the idle states
+ * and initialize them using driver data structures accordingly.
+ */
+static int __init arm64_idle_init(void)
+{
+       int i, ret;
+       const char *entry_method;
+       struct device_node *idle_states_node;
+       const struct cpu_suspend_ops *suspend_init;
+       struct cpuidle_driver *drv = &arm64_idle_driver;
+
+       idle_states_node = of_find_node_by_path("/cpus/idle-states");
+       if (!idle_states_node)
+               return -ENOENT;
+
+       if (of_property_read_string(idle_states_node, "entry-method",
+                                   &entry_method)) {
+               pr_warn(" * %s missing entry-method property\n",
+                           idle_states_node->full_name);
+               of_node_put(idle_states_node);
+               ret = -EOPNOTSUPP;
+               goto put_node;
+       }
+
+       suspend_init = get_suspend_ops(entry_method);
+       if (!suspend_init) {
+               pr_warn("Missing suspend initializer\n");
+               ret = -EOPNOTSUPP;
+               goto put_node;
+       }
+
+       /*
+        * State at index 0 is standby wfi and considered standard
+        * on all ARM platforms. If in some platforms simple wfi
+        * can't be used as "state 0", DT bindings must be implemented
+        * to work around this issue and allow installing a special
+        * handler for idle state index 0.
+        */
+       drv->states[0].exit_latency = 1;
+       drv->states[0].target_residency = 1;
+       drv->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
+       strncpy(drv->states[0].name, "ARM WFI", CPUIDLE_NAME_LEN);
+       strncpy(drv->states[0].desc, "ARM WFI", CPUIDLE_DESC_LEN);
+
+       drv->cpumask = (struct cpumask *) cpu_possible_mask;
+       /*
+        * Start at index 1, request idle state nodes to be filled
+        */
+       ret = of_init_idle_driver(drv, state_nodes, 1, true);
+       if (ret)
+               goto put_node;
+
+       if (suspend_init->init_fn(drv, state_nodes)) {
+               ret = -EOPNOTSUPP;
+               goto put_node;
+       }
+
+       for (i = 0; i < drv->state_count; i++)
+               drv->states[i].enter = arm_enter_idle_state;
+
+       ret = cpuidle_register(drv, NULL);
+
+put_node:
+       of_node_put(idle_states_node);
+       return ret;
+}
+device_initcall(arm64_idle_init);
--

1.8.4

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

Quoted 1 line · Lorenzo Pieralisi

This patch implements a generic CPU idle driver for ARM64 machines.

I fail to see anything arm64 specific here. The idle states binding is for both arm32 and arm64, right? If not, please make it for both. Otherwise, I'm okay with the binding for the most part. I need to take another pass at it though.

Rob

Quoted 252 lines

It relies on the DT idle states infrastructure to initialize idle states count and respective parameters. Current code assumes the driver is managing idle states on all possible CPUs but can be easily generalized to support heterogenous systems and build cpumasks at runtime using MIDRs or DT cpu nodes compatible properties.

Suspend back-ends (eg PSCI) must register a suspend initializer with the CPU idle driver so that the suspend backend call can be detected, and the driver code can call the back-end infrastructure to complete the suspend backend initialization.

Idle state index 0 is always initialized as a simple wfi state, ie always considered present and functional on all ARM64 platforms.

Signed-off-by Lorenzo Pieralisi <[email protected]>

---
 drivers/cpuidle/Kconfig         |   5 ++
 drivers/cpuidle/Kconfig.arm64   |  13 ++++
 drivers/cpuidle/Makefile        |   4 +
 drivers/cpuidle/cpuidle-arm64.c | 168 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 190 insertions(+)
 create mode 100644 drivers/cpuidle/Kconfig.arm64
 create mode 100644 drivers/cpuidle/cpuidle-arm64.c

drivers/cpuidle/Kconfig +5 -0

index 760ce20..360c086 100644
@@ -44,6 +44,11 @@ depends on ARM
 source "drivers/cpuidle/Kconfig.arm"
 endmenu
 
+menu "ARM64 CPU Idle Drivers"
+depends on ARM64
+source "drivers/cpuidle/Kconfig.arm64"
+endmenu
+
 menu "MIPS CPU Idle Drivers"
 depends on MIPS
 source "drivers/cpuidle/Kconfig.mips"

drivers/cpuidle/Kconfig.arm64 new file +13 -0

new file mode 100644
index 0000000..b83612c
@@ -0,0 +1,13 @@
+#
+# ARM64 CPU Idle drivers
+#
+
+config ARM64_CPUIDLE
+       bool "Generic ARM64 CPU idle Driver"
+       select OF_IDLE_STATES
+       help
+         Select this to enable generic cpuidle driver for ARM v8.
+         It provides a generic idle driver whose idle states are configured
+         at run-time through DT nodes. The CPUidle suspend backend is
+         initialized by the device tree parsing code on matching the entry
+         method to the respective CPU operations.

drivers/cpuidle/Makefile +4 -0

index d5ebf4b..e496242 100644
@@ -23,6 +23,10 @@ obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
 obj-$(CONFIG_MIPS_CPS_CPUIDLE)         += cpuidle-cps.o
 
 ###############################################################################
+# ARM64 drivers
+obj-$(CONFIG_ARM64_CPUIDLE)            += cpuidle-arm64.o
+
+###############################################################################
 # POWERPC drivers
 obj-$(CONFIG_PSERIES_CPUIDLE)          += cpuidle-pseries.o
 obj-$(CONFIG_POWERNV_CPUIDLE)          += cpuidle-powernv.o

drivers/cpuidle/cpuidle-arm64.c new file +168 -1

new file mode 100644
index 0000000..4c932f8
@@ -0,0 +1,168 @@
+/*
+ * ARM64 generic CPU idle driver.
+ *
+ * Copyright (C) 2014 ARM Ltd.
+ * Author: Lorenzo Pieralisi <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) "CPUidle arm64: " fmt
+
+#include <linux/cpuidle.h>
+#include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include <asm/psci.h>
+#include <asm/suspend.h>
+
+#include "of_idle_states.h"
+
+typedef int (*suspend_init_fn)(struct cpuidle_driver *,
+                              struct device_node *[]);
+
+struct cpu_suspend_ops {
+       const char *id;
+       suspend_init_fn init_fn;
+};
+
+static const struct cpu_suspend_ops suspend_operations[] __initconst = {
+       {"arm,psci", psci_dt_register_idle_states},
+       {}
+};
+
+static __init const struct cpu_suspend_ops *get_suspend_ops(const char *str)
+{
+       int i;
+
+       if (!str)
+               return NULL;
+
+       for (i = 0; suspend_operations[i].id; i++)
+               if (!strcmp(suspend_operations[i].id, str))
+                       return &suspend_operations[i];
+
+       return NULL;
+}
+
+/*
+ * arm_enter_idle_state - Programs CPU to enter the specified state
+ *
+ * dev: cpuidle device
+ * drv: cpuidle driver
+ * idx: state index
+ *
+ * Called from the CPUidle framework to program the device to the
+ * specified target state selected by the governor.
+ */
+static int arm_enter_idle_state(struct cpuidle_device *dev,
+                               struct cpuidle_driver *drv, int idx)
+{
+       int ret;
+
+       if (!idx) {
+               cpu_do_idle();
+               return idx;
+       }
+
+       cpu_pm_enter();
+       /*
+        * Pass idle state index to cpu_suspend which in turn will call
+        * the CPU ops suspend protocol with idle index as a parameter.
+        *
+        * Some states would not require context to be saved and flushed
+        * to DRAM, so calling cpu_suspend would not be stricly necessary.
+        * When power domains specifications for ARM CPUs are finalized then
+        * this code can be optimized to prevent saving registers if not
+        * needed.
+        */
+       ret = cpu_suspend(idx);
+
+       cpu_pm_exit();
+
+       return ret ? -1 : idx;
+}
+
+struct cpuidle_driver arm64_idle_driver = {
+       .name = "arm64_idle",
+       .owner = THIS_MODULE,
+};
+
+static struct device_node *state_nodes[CPUIDLE_STATE_MAX] __initdata;
+
+/*
+ * arm64_idle_init
+ *
+ * Registers the arm64 specific cpuidle driver with the cpuidle
+ * framework. It relies on core code to parse the idle states
+ * and initialize them using driver data structures accordingly.
+ */
+static int __init arm64_idle_init(void)
+{
+       int i, ret;
+       const char *entry_method;
+       struct device_node *idle_states_node;
+       const struct cpu_suspend_ops *suspend_init;
+       struct cpuidle_driver *drv = &arm64_idle_driver;
+
+       idle_states_node = of_find_node_by_path("/cpus/idle-states");
+       if (!idle_states_node)
+               return -ENOENT;
+
+       if (of_property_read_string(idle_states_node, "entry-method",
+                                   &entry_method)) {
+               pr_warn(" * %s missing entry-method property\n",
+                           idle_states_node->full_name);
+               of_node_put(idle_states_node);
+               ret = -EOPNOTSUPP;
+               goto put_node;
+       }
+
+       suspend_init = get_suspend_ops(entry_method);
+       if (!suspend_init) {
+               pr_warn("Missing suspend initializer\n");
+               ret = -EOPNOTSUPP;
+               goto put_node;
+       }
+
+       /*
+        * State at index 0 is standby wfi and considered standard
+        * on all ARM platforms. If in some platforms simple wfi
+        * can't be used as "state 0", DT bindings must be implemented
+        * to work around this issue and allow installing a special
+        * handler for idle state index 0.
+        */
+       drv->states[0].exit_latency = 1;
+       drv->states[0].target_residency = 1;
+       drv->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
+       strncpy(drv->states[0].name, "ARM WFI", CPUIDLE_NAME_LEN);
+       strncpy(drv->states[0].desc, "ARM WFI", CPUIDLE_DESC_LEN);
+
+       drv->cpumask = (struct cpumask *) cpu_possible_mask;
+       /*
+        * Start at index 1, request idle state nodes to be filled
+        */
+       ret = of_init_idle_driver(drv, state_nodes, 1, true);
+       if (ret)
+               goto put_node;
+
+       if (suspend_init->init_fn(drv, state_nodes)) {
+               ret = -EOPNOTSUPP;
+               goto put_node;
+       }
+
+       for (i = 0; i < drv->state_count; i++)
+               drv->states[i].enter = arm_enter_idle_state;
+
+       ret = cpuidle_register(drv, NULL);
+
+put_node:
+       of_node_put(idle_states_node);
+       return ret;
+}
+device_initcall(arm64_idle_init);
--

1.8.4

Quoted 4 lines

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel