devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC v2 0/2] ARM: defining power states DT bindings
@ 2014-01-20 17:47 Lorenzo Pieralisi
  2014-01-20 17:47 ` [PATCH RFC v2 1/2] Documentation: arm: add cache " Lorenzo Pieralisi
  2014-01-20 17:47 ` [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings Lorenzo Pieralisi
  0 siblings, 2 replies; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-20 17:47 UTC (permalink / raw)
  To: devicetree
  Cc: linux-arm-kernel, linux-pm, Lorenzo Pieralisi, Dave Martin,
	Mark Rutland, Sudeep Holla, Charles Garcia Tobin, Nicolas Pitre,
	Rob Herring, Peter De Schrijver, Grant Likely, Kumar Gala,
	Santosh Shilimkar, Mark Hambleton, Hanjun Guo, Daniel Lezcano,
	Amit Kucheria, Vincent Guittot, Antti Miettinen, Stephen Boyd,
	Tomasz Figa, Kevin Hilman

This is v2 of a previous posting:

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/215544.html

This patchset depends on the following bindings to be approved and augmented
to cater for hierarchical power domains in DT:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/224928.html

Changes in v2:

- Updated cache bindings according to review
- Added power domain phandle to cache bindings
- Added power domains to C-states bindings
- Removed useless reg property from C-states bindings
- Removed cpu-map references from C-states bindings
- Added dependency on OPP in C-states parameters
- Added C-state state hierarchy

ARM based systems embed power management HW that allows SW to enter
low-power states according to run-time criteria based on parameters (eg
power state entry/exit latency) that define how a power state has to be
managed and its respective properties. ARM partners implement HW power
management schemes through custom HW, with power controllers and relative
control mechanisms differing on both HW implementations and the way SW can
control them. This differentiation forces PM software in the kernel to cope
with states differences in power management drivers, which cause code
fragmentation and duplication of functionality.

Most of the power control scheme HW parameters are not probeable on ARM
platforms from a SW point of view, hence, in order to tackle the drivers
fragmentation problem, this patch series defines device tree bindings to
describe power states parameters on ARM platforms.

Device tree bindings for power states also require the introduction of device
tree bindings for processor caches, since power states entry/exit require
SW cache maintainance; in some ARM systems, where firmware does not
support power down interfaces, cache maintainance must be carried out in the
OS power management layer, which then requires a description of the cache
topology through device tree nodes.

The power states on ARM are described as "C-states" in this patchset,
borrowing the nomenclature from ACPI power states bindings which have by now
been widely adopted on both x86 and ARM world as power states names.

C-states device tree standardization shares most of the concepts and
definitions with the ongoing ACPI ARM C-state bindings proposal so that
both standards can contain a coherent set of parameters, simplifying the
way SW will have to handle the respective device drivers.

Lorenzo Pieralisi (2):
  Documentation: arm: add cache DT bindings
  Documentation: arm: define DT C-states bindings

 Documentation/devicetree/bindings/arm/c-states.txt | 774 +++++++++++++++++++++
 Documentation/devicetree/bindings/arm/cache.txt    | 187 +++++
 Documentation/devicetree/bindings/arm/cpus.txt     |  10 +
 3 files changed, 971 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/c-states.txt
 create mode 100644 Documentation/devicetree/bindings/arm/cache.txt

-- 
1.8.4



^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH RFC v2 1/2] Documentation: arm: add cache DT bindings
  2014-01-20 17:47 [PATCH RFC v2 0/2] ARM: defining power states DT bindings Lorenzo Pieralisi
@ 2014-01-20 17:47 ` Lorenzo Pieralisi
  2014-01-21 11:49   ` Dave Martin
  2014-01-20 17:47 ` [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings Lorenzo Pieralisi
  1 sibling, 1 reply; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-20 17:47 UTC (permalink / raw)
  To: devicetree
  Cc: linux-arm-kernel, linux-pm, Lorenzo Pieralisi, Dave Martin,
	Mark Rutland, Sudeep Holla, Charles Garcia Tobin, Nicolas Pitre,
	Rob Herring, Peter De Schrijver, Grant Likely, Kumar Gala,
	Santosh Shilimkar, Mark Hambleton, Hanjun Guo, Daniel Lezcano,
	Amit Kucheria, Vincent Guittot, Antti Miettinen, Stephen Boyd,
	Tomasz Figa, Kevin Hilman

On ARM systems the cache topology cannot be probed at runtime, in
particular, it is impossible to probe which CPUs share a given cache
level. Power management software requires this knowledge to implement
optimized power down sequences, hence this patch adds a document that
defines the DT cache bindings for ARM systems. The bindings are compliant
with ePAPR (PowerPC bindings), even though most of the cache nodes
properties requirements are overriden, because caches geometry for
architected caches is probeable on ARM systems. This patch also adds
properties that are specific to ARM architected caches to the existing ones
defined in the ePAPR v1.1, as bindings extensions.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 Documentation/devicetree/bindings/arm/cache.txt | 187 ++++++++++++++++++++++++
 1 file changed, 187 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/cache.txt

diff --git a/Documentation/devicetree/bindings/arm/cache.txt b/Documentation/devicetree/bindings/arm/cache.txt
new file mode 100644
index 0000000..b27cedf
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/cache.txt
@@ -0,0 +1,187 @@
+==========================================
+ARM processors cache binding description
+==========================================
+
+Device tree bindings for ARM processor caches adhere to the cache bindings
+described in [3], in section 3.8 for multi-level and shared caches.
+On ARM based systems most of the cache properties related to cache
+geometry are probeable in HW, hence, unless otherwise stated, the properties
+defined in ePAPR for multi-level and shared caches are to be considered
+optional by default.
+
+On ARM, caches are either architected (directly controlled by the processor
+through coprocessor instructions and tightly coupled with the processor
+implementation) or unarchitected (controlled through a memory mapped
+interface, implemented as a stand-alone IP external to the processor
+implementation).
+
+This document provides the device tree bindings for ARM architected caches.
+
+- ARM architected cache node
+
+	Description: must be a direct child of the cpu node. A system
+		     can contain multiple architected cache nodes per cpu node,
+		     linked through the next-level-cache phandle. The
+		     next-level-cache property in the cpu node points to
+		     the first level of architected cache for the CPU.
+		     The next-level-cache property in architected cache nodes
+		     points to the respective next level of caching in the
+		     hierarchy. An architected cache node with an empty or
+		     missing next-level-cache property represents the last
+		     architected cache level for the CPU.
+		     On ARM v7 and v8 architectures, the order in which cache
+		     nodes are linked through the next-level-cache phandle must
+		     follow the ordering specified in the processors CLIDR (v7)
+		     and CLIDR_EL1 (v8) registers, as described in [1][2],
+		     implying that a cache node pointed at by a
+		     next-level-cache phandle must correspond to a level
+		     defined in CLIDR (v7) and CLIDR_EL1 (v8) greater than the
+		     one the cache node containing the next-level-cache
+		     phandle corresponds to.
+
+	Since on ARM most of the cache properties are probeable in HW the
+	properties described in [3] - section 3.8 multi-level and shared
+	caches - shall be considered optional, with the following properties
+	updates, specific for the ARM architected cache node.
+
+	- compatible
+		Usage: Required
+		Value type: <string>
+		Definition: value shall be "arm,arch-cache".
+
+	- interrupts
+		Usage: Optional
+		Value type: See definition
+		Definition: standard device tree property [3] that defines
+			    the interrupt line associated with the cache.
+			    The property can be accompanied by an
+			    interrupt-names property, as described in [4].
+
+	- power-domain
+		Usage: Optional
+		Value type: phandle
+		Definition: A phandle and power domain specifier as defined by
+			    bindings of power controller specified by the
+			    phandle [5].
+
+Example(dual-cluster big.LITTLE system 32-bit)
+
+	cpus {
+		#size-cells = <0>;
+		#address-cells = <1>;
+
+		cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x0>;
+			next-level-cache = <&L1_0>;
+
+			L1_0: l1-cache {
+				compatible = "arm,arch-cache";
+				next-level-cache = <&L2_0>;
+			};
+
+			L2_0: l2-cache {
+				compatible = "arm,arch-cache";
+			};
+		};
+
+		cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x1>;
+			next-level-cache = <&L1_1>;
+
+			L1_1: l1-cache {
+				compatible = "arm,arch-cache";
+				next-level-cache = <&L2_0>;
+			};
+		};
+
+		cpu@2 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x2>;
+			next-level-cache = <&L1_2>;
+
+			L1_2: l1-cache {
+				compatible = "arm,arch-cache";
+				next-level-cache = <&L2_0>;
+			};
+		};
+
+		cpu@3 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x3>;
+			next-level-cache = <&L1_3>;
+
+			L1_3: l1-cache {
+				compatible = "arm,arch-cache";
+				next-level-cache = <&L2_0>;
+			};
+		};
+
+		cpu@100 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a7";
+			reg = <0x100>;
+			next-level-cache = <&L1_4>;
+
+			L1_4: l1-cache {
+				compatible = "arm,arch-cache";
+				next-level-cache = <&L2_1>;
+			};
+
+			L2_1: l2-cache {
+				compatible = "arm,arch-cache";
+			};
+		};
+
+		cpu@101 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a7";
+			reg = <0x101>;
+			next-level-cache = <&L1_5>;
+
+			L1_5: l1-cache {
+				compatible = "arm,arch-cache";
+				next-level-cache = <&L2_1>;
+			};
+		};
+
+		cpu@102 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a7";
+			reg = <0x102>;
+			next-level-cache = <&L1_6>;
+
+			L1_6: l1-cache {
+				compatible = "arm,arch-cache";
+				next-level-cache = <&L2_1>;
+			};
+		};
+
+		cpu@103 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a7";
+			reg = <0x103>;
+			next-level-cache = <&L1_7>;
+
+			L1_7: l1-cache {
+				compatible = "arm,arch-cache";
+				next-level-cache = <&L2_1>;
+			};
+		};
+	};
+
+[1] ARMv7-AR Reference Manual
+    http://infocenter.arm.com/help/index.jsp
+[2] ARMv8-A Reference Manual
+    http://infocenter.arm.com/help/index.jsp
+[3] ePAPR standard
+    https://www.power.org/documentation/epapr-version-1-1/
+[4] Kernel documentation - resource property bindings
+    Documentation/devicetree/bindings/resource-names.txt
+[5] Kernel documentation - power domain bindings
+    Documentation/devicetree/bindings/power/power_domain.txt
-- 
1.8.4



^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-20 17:47 [PATCH RFC v2 0/2] ARM: defining power states DT bindings Lorenzo Pieralisi
  2014-01-20 17:47 ` [PATCH RFC v2 1/2] Documentation: arm: add cache " Lorenzo Pieralisi
@ 2014-01-20 17:47 ` Lorenzo Pieralisi
  2014-01-21 11:16   ` Vincent Guittot
  2014-01-25  8:15   ` Antti P Miettinen
  1 sibling, 2 replies; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-20 17:47 UTC (permalink / raw)
  To: devicetree
  Cc: linux-arm-kernel, linux-pm, Lorenzo Pieralisi, Dave Martin,
	Mark Rutland, Sudeep Holla, Charles Garcia Tobin, Nicolas Pitre,
	Rob Herring, Peter De Schrijver, Grant Likely, Kumar Gala,
	Santosh Shilimkar, Mark Hambleton, Hanjun Guo, Daniel Lezcano,
	Amit Kucheria, Vincent Guittot, Antti Miettinen, Stephen Boyd,
	Tomasz Figa, Kevin Hilman

ARM based platforms implement a variety of power management schemes that
allow processors to enter at run-time low-power states, aka C-states
in ACPI jargon. The parameters defining these C-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 C-state device tree bindings for
ARM platforms. Bindings define C-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 power states and removing the need for static data in current
and previous kernel versions.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 Documentation/devicetree/bindings/arm/c-states.txt | 774 +++++++++++++++++++++
 Documentation/devicetree/bindings/arm/cpus.txt     |  10 +
 2 files changed, 784 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/c-states.txt

diff --git a/Documentation/devicetree/bindings/arm/c-states.txt b/Documentation/devicetree/bindings/arm/c-states.txt
new file mode 100644
index 0000000..0b5617b
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/c-states.txt
@@ -0,0 +1,774 @@
+==========================================
+ARM C-states binding description
+==========================================
+
+==========================================
+1 - Introduction
+==========================================
+
+ARM systems contain HW capable of managing power consumption dynamically,
+where cores can be put in different low-power states (ranging from simple
+wfi to power gating) according to OSPM policies. Borrowing concepts
+from the ACPI specification[1], the CPU states representing the range of
+dynamic states that a processor can enter at run-time, aka C-state, can be
+specified through device tree bindings representing the parameters required to
+enter/exit specific C-states on a given processor.
+
+The state an ARM CPU can be put into is loosely identified by one of the
+following operating modes:
+
+- Running:
+	 # Processor core is executing instructions
+
+- Wait for Interrupt:
+	# An ARM processor enters wait for interrupt (WFI) low power
+	  state by executing a wfi instruction. When a processor enters
+	  wfi state it disables most of the clocks while keeping the processor
+	  powered up. This state is standard on all ARM processors and it is
+	  defined as C1 in the remainder of this document.
+
+- Dormant:
+	# Dormant mode is entered by executing wfi instructions and by sending
+	  platform specific commands to the platform power controller (coupled
+	  with processor specific SW/HW control sequences).
+	  In dormant mode, most of the processor control and debug logic is
+	  powered up but cache RAM can be put in retention state, providing
+	  additional power savings.
+
+- Sleep:
+	# Sleep mode is entered by executing the wfi instruction and by sending
+	  platform specific commands to the platform power controller (coupled
+	  with processor specific SW/HW control sequences). In sleep mode, a
+	  processor and its caches are shutdown, the entire processor state is
+	  lost.
+
+Building on top of the previous processor modes, ARM platforms implement power
+management schemes that allow an OS PM implementation to put the processor in
+different CPU states (C-states). C-states parameters (eg latency) are
+platform specific and need to be characterized with bindings that provide the
+required information to OSPM code so that it can build the required tables and
+use them at runtime.
+
+The device tree binding definition for ARM C-states is the subject of this
+document.
+
+===========================================
+2 - cpu-power-states node
+===========================================
+
+ARM processor C-states are defined within the cpu-power-states node, which is
+a direct child of the cpus node and provides a container where the processor
+states, defined as device tree nodes, are listed.
+
+- cpu-power-states node
+
+	Usage: Optional - On ARM systems, is a container of processor C-state
+			  nodes. If the system does not provide CPU power
+			  management capabilities or the processor just
+			  supports WFI (C1 state) a cpu-power-states node is
+			  not required.
+
+	Description: cpu-power-states node is a container node, where its
+		     subnodes describe the CPU low-power C-states.
+
+	Node name must be "cpu-power-states".
+
+	The cpu-power-states node's parent node must be cpus node.
+
+	The cpu-power-states node's child nodes can be:
+
+	- one or more state nodes
+
+	Any other configuration is considered invalid.
+
+The nodes describing the C-states (state) can only be defined within the
+cpu-power-states node.
+
+Any other configuration is consider invalid and therefore must be ignored.
+
+===========================================
+2 - state node
+===========================================
+
+A state node represents a C-state description and must be defined as follows:
+
+- state node
+
+	Description: must be child of either the cpu-power-states node or
+		     a state node.
+
+	The state node name shall be "stateN", where N = {0, 1, ...} is
+	the node number; state nodes which are siblings within a single common
+	parent node must be given a unique and sequential N value, starting
+	from 0.
+
+	A state node can contain state child nodes. Child nodes inherit
+	properties from the parent state nodes that work as state
+	properties aggregators (ie contain properties valid on all state
+	nodes children).
+
+	A state node defines the following properties (either explicitly
+	or by inheriting them from a parent node):
+
+	- compatible
+		Usage: Required
+		Value type: <stringlist>
+		Definition: Must be "arm,cpu-power-state".
+
+	- index
+		Usage: Required
+		Value type: <u32>
+		Definition: It represents C-state index, starting from 2 (index
+			    0 represents the processor state "running" and
+			    index 1 represents processor mode "WFI"; indexes 0
+			    and 1 are standard ARM states that need not be
+			    described).
+
+	- power-domain
+		Usage: Required
+		Value type: <prop-encoded-array>
+		Definition: List of phandle and power domain specifiers
+			    as defined by bindings of power controller
+			    specified by the phandle [3]. It represents the
+			    power domains associated with the C-state. The
+			    power domains list can be used by OSPM to
+			    retrieve the devices belonging to the power
+			    domains and carry out corresponding actions to
+			    preserve functionality across power cycles
+			    (ie context save/restore, cache flushing).
+
+	- entry-method
+		Usage: Required
+		Value type: <stringlist>
+		Definition: Describes the method by which a CPU enters the
+			    C-state. This property is required and must be one
+			    of:
+
+			    - "psci"
+			      ARM Standard firmware interface
+
+			    - "[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 C-state.
+
+	- psci-power-state
+		Usage: Required if entry-method property value is set to
+		       "psci".
+		Value type: <u32>
+		Definition: power_state parameter to pass to the PSCI
+			    suspend call to enter the C-state.
+
+	- latency
+		Usage: Required
+		Value type: <prop-encoded-array>
+		Definition: List of u32 values representing worst case latency
+			    in microseconds required to enter and exit the
+			    C-state, one value per OPP [2]. The list should
+			    be specified in the same order as the operating
+			    points property list of the cpu this state is
+			    valid on.
+			    If no OPP bindings are present, the latency value
+			    is associated with the current OPP of CPUs in the
+			    system.
+
+	- min-residency
+		Usage: Required
+		Value type: <prop-encoded-array>
+		Definition: List of u32 values representing time in
+			    microseconds required for the CPU to be in
+			    the C-state to make up for the dynamic power
+			    consumed to enter/exit the C-state in order to
+			    break even in terms of power consumption compared
+			    to C1 state (wfi), one value per-OPP [2].
+			    This parameter depends on the operating conditions
+			    (HW state) and must assume worst case scenario.
+			    The list should be specified in the same order as
+			    the operating points property list of the cpu this
+			    state is valid on.
+			    If no OPP bindings are present the min-residency
+			    value is associated with the current OPP of CPUs
+			    in the system.
+
+===========================================
+3 - Examples
+===========================================
+
+Example 1 (ARM 64-bit, 16-cpu system, two clusters of clusters):
+
+pd_clusters: power-domain-clusters@80002000 {
+	compatible = "arm,power-controller";
+	reg = <0x0 0x80002000 0x0 0x1000>;
+	#power-domain-cells = <1>;
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	pd_cores: power-domain-cores@80000000 {
+		compatible = "arm,power-controller";
+		reg = <0x0 0x80000000 0x0 0x1000>;
+		#power-domain-cells = <1>;
+	};
+};
+
+cpus {
+	#size-cells = <0>;
+	#address-cells = <2>;
+
+	cpu-power-states {
+
+		state0 {
+			compatible = "arm,cpu-power-state";
+			index = <2>;
+			entry-method = "psci";
+			psci-power-state = <0x1010000>;
+			latency = <400>;
+			min-residency = <300>;
+			STATE0_0: state0 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 0>;
+			};
+			STATE0_1: state1 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 1>;
+			};
+			STATE0_2: state2 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 2>;
+			};
+			STATE0_3: state3 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 3>;
+			};
+			STATE0_4: state4 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 4>;
+			};
+			STATE0_5: state5 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 5>;
+			};
+			STATE0_6: state6 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 6>;
+			};
+			STATE0_7: state7 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 7>;
+			};
+		};
+
+		state1 {
+			compatible = "arm,cpu-power-state";
+			index = <2>;
+			entry-method = "psci";
+			psci-power-state = <0x1010000>;
+			latency = <400>;
+			min-residency = <500>;
+			STATE1_0: state0 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 8>;
+			};
+			STATE1_1: state1 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 9>;
+			};
+			STATE1_2: state2 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 10>;
+			};
+			STATE1_3: state3 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 11>;
+			};
+			STATE1_4: state4 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 12>;
+			};
+			STATE1_5: state5 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 13>;
+			};
+			STATE1_6: state6 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 14>;
+			};
+			STATE1_7: state7 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 15>;
+			};
+		};
+
+		STATE2: state2 {
+			compatible = "arm,cpu-power-state";
+			index = <3>;
+			entry-method = "psci";
+			psci-power-state = <0x3010000>;
+			latency = <1000>;
+			min-residency = <2500>;
+			power-domain = <&pd_clusters 0>;
+		};
+
+		STATE3: state3 {
+			compatible = "arm,cpu-power-state";
+			index = <3>;
+			entry-method = "psci";
+			psci-power-state = <0x3010000>;
+			latency = <4500>;
+			min-residency = <6500>;
+			power-domain = <&pd_clusters 1>;
+		};
+	};
+
+	CPU0: cpu@0 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x0>;
+		enable-method = "psci";
+		next-level-cache = <&L1_0>;
+		cpu-power-states = <&STATE0_0 &STATE2>;
+		L1_0: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_0>;
+			power-domain = <&pd_cores 0>;
+		};
+		L2_0: l2-cache {
+			compatible = "arm,arch-cache";
+			power-domain = <&pd_clusters 0>;
+		};
+	};
+
+	CPU1: cpu@1 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x1>;
+		enable-method = "psci";
+		next-level-cache = <&L1_1>;
+		cpu-power-states = <&STATE0_1 &STATE2>;
+		L1_1: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_0>;
+			power-domain = <&pd_cores 1>;
+		};
+	};
+
+	CPU2: cpu@100 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x100>;
+		enable-method = "psci";
+		next-level-cache = <&L1_2>;
+		cpu-power-states = <&STATE0_2 &STATE2>;
+		L1_2: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_0>;
+			power-domain = <&pd_cores 2>;
+		};
+	};
+
+	CPU3: cpu@101 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x101>;
+		enable-method = "psci";
+		next-level-cache = <&L1_3>;
+		cpu-power-states = <&STATE0_3 &STATE2>;
+		L1_3: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_0>;
+			power-domain = <&pd_cores 3>;
+		};
+	};
+
+	CPU4: cpu@10000 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x10000>;
+		enable-method = "psci";
+		next-level-cache = <&L1_4>;
+		cpu-power-states = <&STATE0_4 &STATE2>;
+		L1_4: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_0>;
+			power-domain = <&pd_cores 4>;
+		};
+	};
+
+	CPU5: cpu@10001 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x10001>;
+		enable-method = "psci";
+		next-level-cache = <&L1_5>;
+		cpu-power-states = <&STATE0_5 &STATE2>;
+		L1_5: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_0>;
+			power-domain = <&pd_cores 5>;
+		};
+	};
+
+	CPU6: cpu@10100 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x10100>;
+		enable-method = "psci";
+		next-level-cache = <&L1_6>;
+		cpu-power-states = <&STATE0_6 &STATE2>;
+		L1_6: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_0>;
+			power-domain = <&pd_cores 6>;
+		};
+	};
+
+	CPU7: cpu@10101 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a57";
+		reg = <0x0 0x10101>;
+		enable-method = "psci";
+		next-level-cache = <&L1_7>;
+		cpu-power-states = <&STATE0_7 &STATE2>;
+		L1_7: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_0>;
+			power-domain = <&pd_cores 7>;
+		};
+	};
+
+	CPU8: cpu@100000000 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x0>;
+		enable-method = "psci";
+		next-level-cache = <&L1_8>;
+		cpu-power-states = <&STATE1_0 &STATE3>;
+		L1_8: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_1>;
+			power-domain = <&pd_cores 8>;
+		};
+		L2_1: l2-cache {
+			compatible = "arm,arch-cache";
+			power-domain = <&pd_clusters 1>;
+		};
+	};
+
+	CPU9: cpu@100000001 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x1>;
+		enable-method = "psci";
+		next-level-cache = <&L1_9>;
+		cpu-power-states = <&STATE1_1 &STATE3>;
+		L1_9: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_1>;
+			power-domain = <&pd_cores 9>;
+		};
+	};
+
+	CPU10: cpu@100000100 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x100>;
+		enable-method = "psci";
+		next-level-cache = <&L1_10>;
+		cpu-power-states = <&STATE1_2 &STATE3>;
+		L1_10: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_1>;
+			power-domain = <&pd_cores 10>;
+		};
+	};
+
+	CPU11: cpu@100000101 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x101>;
+		enable-method = "psci";
+		next-level-cache = <&L1_11>;
+		cpu-power-states = <&STATE1_3 &STATE3>;
+		L1_11: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_1>;
+			power-domain = <&pd_cores 11>;
+		};
+	};
+
+	CPU12: cpu@100010000 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x10000>;
+		enable-method = "psci";
+		next-level-cache = <&L1_12>;
+		cpu-power-states = <&STATE1_4 &STATE3>;
+		L1_12: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_1>;
+			power-domain = <&pd_cores 12>;
+		};
+	};
+
+	CPU13: cpu@100010001 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x10001>;
+		enable-method = "psci";
+		next-level-cache = <&L1_13>;
+		cpu-power-states = <&STATE1_5 &STATE3>;
+		L1_13: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_1>;
+			power-domain = <&pd_cores 13>;
+		};
+	};
+
+	CPU14: cpu@100010100 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x10100>;
+		enable-method = "psci";
+		next-level-cache = <&L1_14>;
+		cpu-power-states = <&STATE1_6 &STATE3>;
+		L1_14: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_1>;
+			power-domain = <&pd_cores 14>;
+		};
+	};
+
+	CPU15: cpu@100010101 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a53";
+		reg = <0x1 0x10101>;
+		enable-method = "psci";
+		next-level-cache = <&L1_15>;
+		cpu-power-states = <&STATE1_7 &STATE3>;
+		L1_15: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_1>;
+			power-domain = <&pd_cores 15>;
+		};
+	};
+};
+
+Example 2 (ARM 32-bit, 8-cpu system, two clusters):
+
+pd_clusters: power-domain-clusters@80002000 {
+	compatible = "arm,power-controller";
+	reg = <0x80002000 0x1000>;
+	#power-domain-cells = <1>;
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	pd_cores: power-domain-cores@80000000 {
+		compatible = "arm,power-controller";
+		reg = <0x80000000 0x1000>;
+		#power-domain-cells = <1>;
+	};
+};
+
+cpus {
+	#size-cells = <0>;
+	#address-cells = <1>;
+
+	cpu-power-states {
+
+		state0 {
+			compatible = "arm,cpu-power-state";
+			index = <2>;
+			entry-method = "psci";
+			psci-power-state = <0x1010000>;
+			latency = <400>;
+			min-residency = <300>;
+			STATE0_0: state0 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 0>;
+			};
+			STATE0_1: state1 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 1>;
+			};
+			STATE0_2: state2 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 2>;
+			};
+			STATE0_3: state3 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 3>;
+			};
+		};
+
+		state1 {
+			compatible = "arm,cpu-power-state";
+			index = <2>;
+			entry-method = "psci";
+			psci-power-state = <0x1010000>;
+			latency = <400>;
+			min-residency = <500>;
+			STATE1_0: state0 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 4>;
+			};
+			STATE1_1: state1 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 5>;
+			};
+			STATE1_2: state2 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 6>;
+			};
+			STATE1_3: state3 {
+				compatible = "arm,cpu-power-state";
+				power-domain = <&pd_cores 7>;
+			};
+		};
+
+		STATE2: state2 {
+			compatible = "arm,cpu-power-state";
+			index = <3>;
+			entry-method = "psci";
+			psci-power-state = <0x3010000>;
+			latency = <1000>;
+			min-residency = <1500>;
+			power-domain = <&pd_clusters 0>;
+		};
+
+		STATE3: state3 {
+			compatible = "arm,cpu-power-state";
+			index = <3>;
+			entry-method = "psci";
+			psci-power-state = <0x3010000>;
+			latency = <4500>;
+			min-residency = <6500>;
+			power-domain = <&pd_clusters 1>;
+		};
+	};
+
+	CPU0: cpu@0 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a15";
+		reg = <0x0>;
+		next-level-cache = <&L1_0>;
+		cpu-power-states = <&STATE0_0 &STATE2>;
+		L1_0: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_0>;
+			power-domain = <&pd_cores 0>;
+		};
+		L2_0: l2-cache {
+			compatible = "arm,arch-cache";
+			power-domain = <&pd_clusters 0>;
+		};
+	};
+
+	CPU1: cpu@1 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a15";
+		reg = <0x1>;
+		next-level-cache = <&L1_1>;
+		cpu-power-states = <&STATE0_1 &STATE2>;
+		L1_1: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_0>;
+			power-domain = <&pd_cores 1>;
+		};
+	};
+
+	CPU2: cpu@2 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a15";
+		reg = <0x2>;
+		next-level-cache = <&L1_2>;
+		cpu-power-states = <&STATE0_2 &STATE2>;
+		L1_2: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_0>;
+			power-domain = <&pd_cores 2>;
+		};
+	};
+
+	CPU3: cpu@3 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a15";
+		reg = <0x3>;
+		next-level-cache = <&L1_3>;
+		cpu-power-states = <&STATE0_3 &STATE2>;
+		L1_3: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_0>;
+			power-domain = <&pd_cores 3>;
+		};
+	};
+
+	CPU4: cpu@100 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a7";
+		reg = <0x100>;
+		next-level-cache = <&L1_4>;
+		cpu-power-states = <&STATE1_0 &STATE3>;
+		L1_4: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_1>;
+			power-domain = <&pd_cores 4>;
+		};
+		L2_1: l2-cache {
+			compatible = "arm,arch-cache";
+			power-domain = <&pd_clusters 1>;
+		};
+	};
+
+	CPU5: cpu@101 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a7";
+		reg = <0x101>;
+		next-level-cache = <&L1_5>;
+		cpu-power-states = <&STATE1_1 &STATE3>;
+		L1_5: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_1>;
+			power-domain = <&pd_cores 5>;
+		};
+	};
+
+	CPU6: cpu@102 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a7";
+		reg = <0x102>;
+		next-level-cache = <&L1_6>;
+		cpu-power-states = <&STATE1_2 &STATE3>;
+		L1_6: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_1>;
+			power-domain = <&pd_cores 6>;
+		};
+	};
+
+	CPU7: cpu@103 {
+		device_type = "cpu";
+		compatible = "arm,cortex-a7";
+		reg = <0x103>;
+		next-level-cache = <&L1_7>;
+		cpu-power-states = <&STATE1_3 &STATE3>;
+		L1_7: l1-cache {
+			compatible = "arm,arch-cache";
+			next-level-cache = <&L2_1>;
+			power-domain = <&pd_cores 7>;
+		};
+	};
+};
+
+===========================================
+4 - References
+===========================================
+
+[1] ACPI v5.0 specification
+    http://www.acpi.info/spec50.htm
+
+[2] ARM Linux kernel documentation - OPP bindings
+    Documentation/devicetree/bindings/power/opp.txt
+
+[3] ARM Linux Kernel documentation - power domain bindings
+    Documentation/devicetree/bindings/power/power_domain.txt
diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index 9130435..a3c9193 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -191,6 +191,13 @@ nodes to be present and contain the properties described below.
 			  property identifying a 64-bit zero-initialised
 			  memory location.
 
+	- cpu-power-states
+		Usage: Optional
+		Value type: <prop-encoded-array>
+		Definition:
+			# List of phandles to cpu power state nodes supported
+			  by this cpu [1].
+
 Example 1 (dual-cluster big.LITTLE system 32-bit):
 
 	cpus {
@@ -382,3 +389,6 @@ cpus {
 		cpu-release-addr = <0 0x20000000>;
 	};
 };
+
+[1] ARM Linux kernel documentation - C-state bindings
+    Documentation/devicetree/bindings/arm/c-states.txt
-- 
1.8.4



^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-20 17:47 ` [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings Lorenzo Pieralisi
@ 2014-01-21 11:16   ` Vincent Guittot
  2014-01-21 13:31     ` Lorenzo Pieralisi
  2014-01-22 19:20     ` Lorenzo Pieralisi
  2014-01-25  8:15   ` Antti P Miettinen
  1 sibling, 2 replies; 27+ messages in thread
From: Vincent Guittot @ 2014-01-21 11:16 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: devicetree@vger.kernel.org, LAK, linux-pm@vger.kernel.org,
	Dave Martin, Mark Rutland, Sudeep Holla, Charles Garcia Tobin,
	Nicolas Pitre, Rob Herring, Peter De Schrijver, Grant Likely,
	Kumar Gala, Santosh Shilimkar, Mark Hambleton, Hanjun Guo,
	Daniel Lezcano, Amit Kucheria, Antti Miettinen, Stephen Boyd,
	Tomasz Figa, Kevin Hilman

Hi Lorenzo,

On 20 January 2014 18:47, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> ARM based platforms implement a variety of power management schemes that
> allow processors to enter at run-time low-power states, aka C-states
> in ACPI jargon. The parameters defining these C-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 C-state device tree bindings for
> ARM platforms. Bindings define C-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 power states and removing the need for static data in current
> and previous kernel versions.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>  Documentation/devicetree/bindings/arm/c-states.txt | 774 +++++++++++++++++++++
>  Documentation/devicetree/bindings/arm/cpus.txt     |  10 +
>  2 files changed, 784 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/c-states.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/c-states.txt b/Documentation/devicetree/bindings/arm/c-states.txt
> new file mode 100644
> index 0000000..0b5617b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/c-states.txt
> @@ -0,0 +1,774 @@
> +==========================================
> +ARM C-states binding description
> +==========================================
> +
> +==========================================
> +1 - Introduction
> +==========================================
> +
> +ARM systems contain HW capable of managing power consumption dynamically,
> +where cores can be put in different low-power states (ranging from simple
> +wfi to power gating) according to OSPM policies. Borrowing concepts
> +from the ACPI specification[1], the CPU states representing the range of
> +dynamic states that a processor can enter at run-time, aka C-state, can be
> +specified through device tree bindings representing the parameters required to
> +enter/exit specific C-states on a given processor.
> +
> +The state an ARM CPU can be put into is loosely identified by one of the
> +following operating modes:
> +
> +- Running:
> +        # Processor core is executing instructions
> +
> +- Wait for Interrupt:
> +       # An ARM processor enters wait for interrupt (WFI) low power
> +         state by executing a wfi instruction. When a processor enters
> +         wfi state it disables most of the clocks while keeping the processor
> +         powered up. This state is standard on all ARM processors and it is
> +         defined as C1 in the remainder of this document.
> +

> +- Dormant:
> +       # Dormant mode is entered by executing wfi instructions and by sending
> +         platform specific commands to the platform power controller (coupled
> +         with processor specific SW/HW control sequences).
> +         In dormant mode, most of the processor control and debug logic is
> +         powered up but cache RAM can be put in retention state, providing

Base on your description, it's not clear for me what is on, what is
lost and what is power down ?
My understand of the dormant mode that you described above is : the
cache is preserved (and especially the cache RAM) but the processor
state is lost (registers ...). Do I understand correctly ?

What about retention mode where the contents of processor and cache
are preserved but the power consumption is reduced ? it can be seen as
a special wfi mode which need specific SW/HW control sequences but i'm
not sure to understand how to describe such state with your proposal.

> +         additional power savings.
> +
> +- Sleep:
> +       # Sleep mode is entered by executing the wfi instruction and by sending
> +         platform specific commands to the platform power controller (coupled
> +         with processor specific SW/HW control sequences). In sleep mode, a
> +         processor and its caches are shutdown, the entire processor state is
> +         lost.
> +
> +Building on top of the previous processor modes, ARM platforms implement power
> +management schemes that allow an OS PM implementation to put the processor in
> +different CPU states (C-states). C-states parameters (eg latency) are
> +platform specific and need to be characterized with bindings that provide the
> +required information to OSPM code so that it can build the required tables and
> +use them at runtime.
> +
> +The device tree binding definition for ARM C-states is the subject of this
> +document.
> +

[snip]

> +
> +       - psci-power-state
> +               Usage: Required if entry-method property value is set to
> +                      "psci".
> +               Value type: <u32>
> +               Definition: power_state parameter to pass to the PSCI
> +                           suspend call to enter the C-state.

Why psci has got a dedicated field and not vendor methods ? can't you
make that more generic ?

> +
> +       - latency
> +               Usage: Required
> +               Value type: <prop-encoded-array>
> +               Definition: List of u32 values representing worst case latency
> +                           in microseconds required to enter and exit the
> +                           C-state, one value per OPP [2]. The list should
> +                           be specified in the same order as the operating
> +                           points property list of the cpu this state is
> +                           valid on.
> +                           If no OPP bindings are present, the latency value
> +                           is associated with the current OPP of CPUs in the
> +                           system.
> +

[snip]

Thanks
Vincent

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 1/2] Documentation: arm: add cache DT bindings
  2014-01-20 17:47 ` [PATCH RFC v2 1/2] Documentation: arm: add cache " Lorenzo Pieralisi
@ 2014-01-21 11:49   ` Dave Martin
  2014-01-21 14:47     ` Lorenzo Pieralisi
       [not found]     ` <20140121114845.GA2598-M5GwZQ6tE7x5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org>
  0 siblings, 2 replies; 27+ messages in thread
From: Dave Martin @ 2014-01-21 11:49 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Mark Rutland, devicetree, Kevin Hilman, Vincent Guittot,
	Nicolas Pitre, Amit Kucheria, Tomasz Figa, Daniel Lezcano,
	Stephen Boyd, Rob Herring, Sudeep Holla, Kumar Gala,
	Santosh Shilimkar, Antti Miettinen, Mark Hambleton, linux-pm,
	Grant Likely, Hanjun Guo, Peter De Schrijver, linux-arm-kernel,
	Charles Garcia Tobin

On Mon, Jan 20, 2014 at 05:47:58PM +0000, Lorenzo Pieralisi wrote:
> On ARM systems the cache topology cannot be probed at runtime, in
> particular, it is impossible to probe which CPUs share a given cache
> level. Power management software requires this knowledge to implement
> optimized power down sequences, hence this patch adds a document that
> defines the DT cache bindings for ARM systems. The bindings are compliant
> with ePAPR (PowerPC bindings), even though most of the cache nodes
> properties requirements are overriden, because caches geometry for
> architected caches is probeable on ARM systems. This patch also adds
> properties that are specific to ARM architected caches to the existing ones
> defined in the ePAPR v1.1, as bindings extensions.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>  Documentation/devicetree/bindings/arm/cache.txt | 187 ++++++++++++++++++++++++
>  1 file changed, 187 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/cache.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/cache.txt b/Documentation/devicetree/bindings/arm/cache.txt
> new file mode 100644
> index 0000000..b27cedf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/cache.txt
> @@ -0,0 +1,187 @@
> +==========================================
> +ARM processors cache binding description
> +==========================================
> +
> +Device tree bindings for ARM processor caches adhere to the cache bindings
> +described in [3], in section 3.8 for multi-level and shared caches.
> +On ARM based systems most of the cache properties related to cache
> +geometry are probeable in HW, hence, unless otherwise stated, the properties
> +defined in ePAPR for multi-level and shared caches are to be considered
> +optional by default.

This point should be highlighted for discussion.

I do have a worry that because the kernel won't normally use this
information, by default it will get pasted between .dts files, won't get
tested and will be wrong rather often.  It also violates the DT principle
that probeable information should not be present in the DT -- ePAPR
obviously envisages systems where cache geometry information is not
probeable, but that's not the case for architected caches on ARM, except
in rare cases where the CLIDR is wrong.

But deviating needlessly from ePAPR is arguably a bad thing too.


There is one good argument in favour of making these properties
mandatory: it gives people a way to get (possibly wrong or unvalidated)
information about cache geometry and topology into userspace without the
kernel having to care.  This remains a contraversial issue though.


If we decide to allow or mandate these properties, the kernel should
validate them for consistency with the hardware and BUG() on boot if they
are inconsistent.  This is the correct approach until/unless the kernel
grows a proper mechanism for using this info from the DT.

> +
> +On ARM, caches are either architected (directly controlled by the processor
> +through coprocessor instructions and tightly coupled with the processor
> +implementation) or unarchitected (controlled through a memory mapped
> +interface, implemented as a stand-alone IP external to the processor
> +implementation).
> +
> +This document provides the device tree bindings for ARM architected caches.
> +
> +- ARM architected cache node
> +
> +	Description: must be a direct child of the cpu node. A system


For this paragraph as a whole:

Can we break this paragraph up, and move the background information (e.g.
description of the ARM Architecture) outside?

It's long and hard to read at present.

I think we only need the fundamental rules, which are basically that
the next-level-cache properties must be consistent with the hardware
cache topology.  We could try to be more precise, but ePAPR is pretty
vague too.

> +		     can contain multiple architected cache nodes per cpu node,
> +		     linked through the next-level-cache phandle. The
> +		     next-level-cache property in the cpu node points to
> +		     the first level of architected cache for the CPU.
> +		     The next-level-cache property in architected cache nodes
> +		     points to the respective next level of caching in the
> +		     hierarchy. An architected cache node with an empty or
> +		     missing next-level-cache property represents the last
> +		     architected cache level for the CPU.
> +		     On ARM v7 and v8 architectures, the order in which cache
> +		     nodes are linked through the next-level-cache phandle must
> +		     follow the ordering specified in the processors CLIDR (v7)

We shouldn't describe the ARM Architecture in the binding.  That's
background information that could move outside.

> +		     and CLIDR_EL1 (v8) registers, as described in [1][2],
> +		     implying that a cache node pointed at by a
> +		     next-level-cache phandle must correspond to a level
> +		     defined in CLIDR (v7) and CLIDR_EL1 (v8) greater than the
> +		     one the cache node containing the next-level-cache
> +		     phandle corresponds to.
> +
> +	Since on ARM most of the cache properties are probeable in HW the
> +	properties described in [3] - section 3.8 multi-level and shared
> +	caches - shall be considered optional, with the following properties
> +	updates, specific for the ARM architected cache node.
> +
> +	- compatible
> +		Usage: Required
> +		Value type: <string>
> +		Definition: value shall be "arm,arch-cache".
> +
> +	- interrupts
> +		Usage: Optional
> +		Value type: See definition
> +		Definition: standard device tree property [3] that defines
> +			    the interrupt line associated with the cache.
> +			    The property can be accompanied by an
> +			    interrupt-names property, as described in [4].

Do ARM Architectured caches ever have interrupts?  (Just my ignorance
here.)

> +
> +	- power-domain
> +		Usage: Optional
> +		Value type: phandle
> +		Definition: A phandle and power domain specifier as defined by
> +			    bindings of power controller specified by the
> +			    phandle [5].
> +
> +Example(dual-cluster big.LITTLE system 32-bit)
> +
> +	cpus {
> +		#size-cells = <0>;
> +		#address-cells = <1>;
> +
> +		cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a15";
> +			reg = <0x0>;
> +			next-level-cache = <&L1_0>;

ePAPR puts the L1 cache properties in the cpu node directly;
there's no "L1" node as such.  That geometry description is also
mandated by ePAPR.

The ARM Architecture does not force a CPU to have any non-shared
first level(s) of cache, and ePAPR does not permit next-level-cache
to point to a cpu node, so there are possible ARM Architecture
implementations that cannot be described without violating ePAPR.
However, for practical reasons, such systems are unlikely -- I don't
know if any exist today.

We should decide whether to deviate explicitly from ePAPR on this
point (your examples provide on way), or whether to follow ePAPR and
bodge things later for L1-less systems if they appear.

Cheers
---Dave

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-21 11:16   ` Vincent Guittot
@ 2014-01-21 13:31     ` Lorenzo Pieralisi
  2014-01-21 14:35       ` Amit Kucheria
  2014-01-22 11:42       ` Mark Brown
  2014-01-22 19:20     ` Lorenzo Pieralisi
  1 sibling, 2 replies; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-21 13:31 UTC (permalink / raw)
  To: Vincent Guittot
  Cc: devicetree@vger.kernel.org, LAK, linux-pm@vger.kernel.org,
	Dave P Martin, Mark Rutland, Sudeep Holla, Charles Garcia-Tobin,
	Nicolas Pitre, Rob Herring, Peter De Schrijver,
	grant.likely@linaro.org, Kumar Gala, Santosh Shilimkar,
	Mark Hambleton, Hanjun Guo, Daniel Lezcano, Amit Kucheria,
	Antti Miettinen, Stephen Boyd, Tomasz Figa, Kevin Hilman

On Tue, Jan 21, 2014 at 11:16:46AM +0000, Vincent Guittot wrote:

[...]

> > +- Dormant:
> > +       # Dormant mode is entered by executing wfi instructions and by sending
> > +         platform specific commands to the platform power controller (coupled
> > +         with processor specific SW/HW control sequences).
> > +         In dormant mode, most of the processor control and debug logic is
> > +         powered up but cache RAM can be put in retention state, providing
> 
> Base on your description, it's not clear for me what is on, what is
> lost and what is power down ?

Sorry, typo, "powered down", not powered up.

> My understand of the dormant mode that you described above is : the
> cache is preserved (and especially the cache RAM) but the processor
> state is lost (registers ...). Do I understand correctly ?

Yes.

> What about retention mode where the contents of processor and cache
> are preserved but the power consumption is reduced ? it can be seen as
> a special wfi mode which need specific SW/HW control sequences but i'm
> not sure to understand how to describe such state with your proposal.

True, and I omitted that on purpose so that it can be debated and to
keep it simple (well, so to speak) thanks for pointing that out.

The bindings allow a C-state to link to a power domain. Each device can
link itself to a power domain. Hence at least now we know what devices
are affected by a C-state (and by device I also mean arch timers, PMUs,
GIC, etc).

Now, retention vs. off. In theory we could link a device to a C-state
and define what mode would be that device on C-state entry, but honestly
it starts becoming looooots of data in the DT.

For instance, we could define for every device the max C-state index allowed
for the device context to be powered-up (or retained).

Or, find a way to describe it through the power domain specifier:

cache {
	power-domain = <&foo 0 &foo 1>:
	power-state = <1 0>;
};

which means that for the pair <&foo 0> cache is retained (1 == retained,
0 == lost) and for power domain <&foo 1> cache is lost.

I have no complete answer, certainly this adds complexity (but it is a very
complex problem, so..) and it is a bit horrible, ideas welcome.

[...]

> > +       - psci-power-state
> > +               Usage: Required if entry-method property value is set to
> > +                      "psci".
> > +               Value type: <u32>
> > +               Definition: power_state parameter to pass to the PSCI
> > +                           suspend call to enter the C-state.
> 
> Why psci has got a dedicated field and not vendor methods ? can't you
> make that more generic ?

If anyone provides me with an example usage why not, for now I know I
need that parameter for PSCI, I can call it differently, define it for PSCI
and leave it as optional for other methods.

Thanks,
Lorenzo


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-21 13:31     ` Lorenzo Pieralisi
@ 2014-01-21 14:35       ` Amit Kucheria
  2014-01-21 15:23         ` Lorenzo Pieralisi
  2014-01-22 11:42       ` Mark Brown
  1 sibling, 1 reply; 27+ messages in thread
From: Amit Kucheria @ 2014-01-21 14:35 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: devicetree, Lists LAKML, Linux PM list, Dave Martin, Mark Rutland,
	Sudeep Holla, Charles Garcia Tobin, Nicolas Pitre, Rob Herring,
	Peter De Schrijver, Grant Likely, Kumar Gala, Santosh Shilimkar,
	Mark Hambleton, Hanjun Guo, Daniel Lezcano, Vincent Guittot,
	Antti Miettinen, Stephen Boyd, Tomasz Figa, Kevin Hilman

Hi Lorenzo,

On Mon, Jan 20, 2014 at 11:17 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> ARM based platforms implement a variety of power management schemes that
> allow processors to enter at run-time low-power states, aka C-states
> in ACPI jargon. The parameters defining these C-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 C-state device tree bindings for
> ARM platforms. Bindings define C-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 power states and removing the need for static data in current
> and previous kernel versions.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>  Documentation/devicetree/bindings/arm/c-states.txt | 774 +++++++++++++++++++++
>  Documentation/devicetree/bindings/arm/cpus.txt     |  10 +
>  2 files changed, 784 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/c-states.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/c-states.txt b/Documentation/devicetree/bindings/arm/c-states.txt

s/c-states/idle-states?

While C-states are widely used when talking about idle-states as
you've noted, idle-states are still the correct generic term for them.

> new file mode 100644
> index 0000000..0b5617b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/c-states.txt
> @@ -0,0 +1,774 @@
> +==========================================
> +ARM C-states binding description
> +==========================================
> +
> +==========================================
> +1 - Introduction
> +==========================================
> +
> +ARM systems contain HW capable of managing power consumption dynamically,
> +where cores can be put in different low-power states (ranging from simple
> +wfi to power gating) according to OSPM policies. Borrowing concepts
> +from the ACPI specification[1], the CPU states representing the range of
> +dynamic states that a processor can enter at run-time, aka C-state, can be
> +specified through device tree bindings representing the parameters required to
> +enter/exit specific C-states on a given processor.
> +
> +The state an ARM CPU can be put into is loosely identified by one of the
> +following operating modes:
> +
> +- Running:
> +        # Processor core is executing instructions
> +
> +- Wait for Interrupt:
> +       # An ARM processor enters wait for interrupt (WFI) low power
> +         state by executing a wfi instruction. When a processor enters
> +         wfi state it disables most of the clocks while keeping the processor
> +         powered up. This state is standard on all ARM processors and it is
> +         defined as C1 in the remainder of this document.
> +
> +- Dormant:
> +       # Dormant mode is entered by executing wfi instructions and by sending
> +         platform specific commands to the platform power controller (coupled
> +         with processor specific SW/HW control sequences).
> +         In dormant mode, most of the processor control and debug logic is
> +         powered up but cache RAM can be put in retention state, providing
> +         additional power savings.
> +
> +- Sleep:
> +       # Sleep mode is entered by executing the wfi instruction and by sending
> +         platform specific commands to the platform power controller (coupled
> +         with processor specific SW/HW control sequences). In sleep mode, a
> +         processor and its caches are shutdown, the entire processor state is
> +         lost.
> +
> +Building on top of the previous processor modes, ARM platforms implement power

Nitpick: s/previous/above

> +management schemes that allow an OS PM implementation to put the processor in
> +different CPU states (C-states). C-states parameters (eg latency) are
> +platform specific and need to be characterized with bindings that provide the
> +required information to OSPM code so that it can build the required tables and
> +use them at runtime.
> +
> +The device tree binding definition for ARM C-states is the subject of this
> +document.
> +
> +===========================================
> +2 - cpu-power-states node
> +===========================================
> +
> +ARM processor C-states are defined within the cpu-power-states node, which is
> +a direct child of the cpus node and provides a container where the processor
> +states, defined as device tree nodes, are listed.
> +
> +- cpu-power-states node

What do you think of s/cpu-power-states/cpu-idle-states?

CPU Power management is more than just idle. Unless you have plans to
add more properties to the cpu-power-states node later.

> +
> +       Usage: Optional - On ARM systems, is a container of processor C-state
> +                         nodes. If the system does not provide CPU power
> +                         management capabilities or the processor just
> +                         supports WFI (C1 state) a cpu-power-states node is
> +                         not required.
> +
> +       Description: cpu-power-states node is a container node, where its
> +                    subnodes describe the CPU low-power C-states.
> +
> +       Node name must be "cpu-power-states".
> +
> +       The cpu-power-states node's parent node must be cpus node.
> +
> +       The cpu-power-states node's child nodes can be:
> +
> +       - one or more state nodes
> +
> +       Any other configuration is considered invalid.
> +
> +The nodes describing the C-states (state) can only be defined within the
> +cpu-power-states node.
> +
> +Any other configuration is consider invalid and therefore must be ignored.
> +
> +===========================================
> +2 - state node
> +===========================================
> +
> +A state node represents a C-state description and must be defined as follows:
> +
> +- state node
> +
> +       Description: must be child of either the cpu-power-states node or
> +                    a state node.
> +
> +       The state node name shall be "stateN", where N = {0, 1, ...} is
> +       the node number; state nodes which are siblings within a single common
> +       parent node must be given a unique and sequential N value, starting
> +       from 0.
> +
> +       A state node can contain state child nodes. Child nodes inherit
> +       properties from the parent state nodes that work as state
> +       properties aggregators (ie contain properties valid on all state
> +       nodes children).
> +
> +       A state node defines the following properties (either explicitly
> +       or by inheriting them from a parent node):
> +
> +       - compatible
> +               Usage: Required
> +               Value type: <stringlist>
> +               Definition: Must be "arm,cpu-power-state".
> +
> +       - index
> +               Usage: Required
> +               Value type: <u32>
> +               Definition: It represents C-state index, starting from 2 (index
> +                           0 represents the processor state "running" and
> +                           index 1 represents processor mode "WFI"; indexes 0
> +                           and 1 are standard ARM states that need not be
> +                           described).
> +
> +       - power-domain
> +               Usage: Required
> +               Value type: <prop-encoded-array>
> +               Definition: List of phandle and power domain specifiers
> +                           as defined by bindings of power controller
> +                           specified by the phandle [3]. It represents the
> +                           power domains associated with the C-state. The
> +                           power domains list can be used by OSPM to
> +                           retrieve the devices belonging to the power
> +                           domains and carry out corresponding actions to
> +                           preserve functionality across power cycles
> +                           (ie context save/restore, cache flushing).
> +
> +       - entry-method
> +               Usage: Required
> +               Value type: <stringlist>
> +               Definition: Describes the method by which a CPU enters the
> +                           C-state. This property is required and must be one
> +                           of:
> +
> +                           - "psci"
> +                             ARM Standard firmware interface
> +
> +                           - "[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 C-state.
> +
> +       - psci-power-state
> +               Usage: Required if entry-method property value is set to
> +                      "psci".
> +               Value type: <u32>
> +               Definition: power_state parameter to pass to the PSCI
> +                           suspend call to enter the C-state.
> +
> +       - latency
> +               Usage: Required
> +               Value type: <prop-encoded-array>
> +               Definition: List of u32 values representing worst case latency
> +                           in microseconds required to enter and exit the
> +                           C-state, one value per OPP [2]. The list should
> +                           be specified in the same order as the operating
> +                           points property list of the cpu this state is
> +                           valid on.
> +                           If no OPP bindings are present, the latency value
> +                           is associated with the current OPP of CPUs in the
> +                           system.

DT-newbie here. What would happen if a vendor does not characterise
the latency at each OPP? IOW, the table only contains latency values
for a subset of the OPPs.

> +
> +       - min-residency
> +               Usage: Required
> +               Value type: <prop-encoded-array>
> +               Definition: List of u32 values representing time in
> +                           microseconds required for the CPU to be in
> +                           the C-state to make up for the dynamic power
> +                           consumed to enter/exit the C-state in order to
> +                           break even in terms of power consumption compared
> +                           to C1 state (wfi), one value per-OPP [2].
> +                           This parameter depends on the operating conditions
> +                           (HW state) and must assume worst case scenario.
> +                           The list should be specified in the same order as
> +                           the operating points property list of the cpu this
> +                           state is valid on.
> +                           If no OPP bindings are present the min-residency
> +                           value is associated with the current OPP of CPUs
> +                           in the system.

Same question as latency above.

> +
> +===========================================
> +3 - Examples
> +===========================================
> +
> +Example 1 (ARM 64-bit, 16-cpu system, two clusters of clusters):


     ^^^^^^^^^^^^^
                                                         clusters of cpus?
> +
> +pd_clusters: power-domain-clusters@80002000 {
> +       compatible = "arm,power-controller";
> +       reg = <0x0 0x80002000 0x0 0x1000>;
> +       #power-domain-cells = <1>;
> +       #address-cells = <2>;
> +       #size-cells = <2>;
> +
> +       pd_cores: power-domain-cores@80000000 {
> +               compatible = "arm,power-controller";
> +               reg = <0x0 0x80000000 0x0 0x1000>;
> +               #power-domain-cells = <1>;
> +       };
> +};
> +

<snip>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 1/2] Documentation: arm: add cache DT bindings
  2014-01-21 11:49   ` Dave Martin
@ 2014-01-21 14:47     ` Lorenzo Pieralisi
       [not found]     ` <20140121114845.GA2598-M5GwZQ6tE7x5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org>
  1 sibling, 0 replies; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-21 14:47 UTC (permalink / raw)
  To: Dave Martin
  Cc: devicetree@vger.kernel.org, Mark Rutland, Tomasz Figa,
	Mark Hambleton, Vincent Guittot, Nicolas Pitre, Daniel Lezcano,
	linux-arm-kernel@lists.infradead.org, grant.likely@linaro.org,
	Charles Garcia-Tobin, Kevin Hilman, linux-pm@vger.kernel.org,
	Kumar Gala, Rob Herring, Antti Miettinen, Peter De Schrijver,
	Stephen Boyd, Amit Kucheria, Santosh Shilimkar, Hanjun Guo

On Tue, Jan 21, 2014 at 11:49:01AM +0000, Dave Martin wrote:
> On Mon, Jan 20, 2014 at 05:47:58PM +0000, Lorenzo Pieralisi wrote:
> > On ARM systems the cache topology cannot be probed at runtime, in
> > particular, it is impossible to probe which CPUs share a given cache
> > level. Power management software requires this knowledge to implement
> > optimized power down sequences, hence this patch adds a document that
> > defines the DT cache bindings for ARM systems. The bindings are compliant
> > with ePAPR (PowerPC bindings), even though most of the cache nodes
> > properties requirements are overriden, because caches geometry for
> > architected caches is probeable on ARM systems. This patch also adds
> > properties that are specific to ARM architected caches to the existing ones
> > defined in the ePAPR v1.1, as bindings extensions.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> >  Documentation/devicetree/bindings/arm/cache.txt | 187 ++++++++++++++++++++++++
> >  1 file changed, 187 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/cache.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/cache.txt b/Documentation/devicetree/bindings/arm/cache.txt
> > new file mode 100644
> > index 0000000..b27cedf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/cache.txt
> > @@ -0,0 +1,187 @@
> > +==========================================
> > +ARM processors cache binding description
> > +==========================================
> > +
> > +Device tree bindings for ARM processor caches adhere to the cache bindings
> > +described in [3], in section 3.8 for multi-level and shared caches.
> > +On ARM based systems most of the cache properties related to cache
> > +geometry are probeable in HW, hence, unless otherwise stated, the properties
> > +defined in ePAPR for multi-level and shared caches are to be considered
> > +optional by default.
> 
> This point should be highlighted for discussion.
> 
> I do have a worry that because the kernel won't normally use this
> information, by default it will get pasted between .dts files, won't get
> tested and will be wrong rather often.  It also violates the DT principle
> that probeable information should not be present in the DT -- ePAPR
> obviously envisages systems where cache geometry information is not
> probeable, but that's not the case for architected caches on ARM, except
> in rare cases where the CLIDR is wrong.
> 
> But deviating needlessly from ePAPR is arguably a bad thing too.
> 
> 
> There is one good argument in favour of making these properties
> mandatory: it gives people a way to get (possibly wrong or unvalidated)
> information about cache geometry and topology into userspace without the
> kernel having to care.  This remains a contraversial issue though.
>

I am not sure it is a good argument, and to be honest there are already
properties in the DT that we do not validate but still are exposed to
users (eg cpu nodes compatible strings). I'd rather forbid all cache
geometry DT properties on ARM.

> If we decide to allow or mandate these properties, the kernel should
> validate them for consistency with the hardware and BUG() on boot if they
> are inconsistent.  This is the correct approach until/unless the kernel
> grows a proper mechanism for using this info from the DT.

I think we should forbid them, so that, if people cut'n'paste those
properties into DT, those properties are to be considered outright junk.

A wrong CLIDR is a HW bug, and should be fixed as an errata, not through DT.

> > +On ARM, caches are either architected (directly controlled by the processor
> > +through coprocessor instructions and tightly coupled with the processor
> > +implementation) or unarchitected (controlled through a memory mapped
> > +interface, implemented as a stand-alone IP external to the processor
> > +implementation).
> > +
> > +This document provides the device tree bindings for ARM architected caches.
> > +
> > +- ARM architected cache node
> > +
> > +	Description: must be a direct child of the cpu node. A system
> 
> 
> For this paragraph as a whole:
> 
> Can we break this paragraph up, and move the background information (e.g.
> description of the ARM Architecture) outside?
> 
> It's long and hard to read at present.

Yes, I noticed too.

> I think we only need the fundamental rules, which are basically that
> the next-level-cache properties must be consistent with the hardware
> cache topology.  We could try to be more precise, but ePAPR is pretty
> vague too.

I agree.

> > +		     can contain multiple architected cache nodes per cpu node,
> > +		     linked through the next-level-cache phandle. The
> > +		     next-level-cache property in the cpu node points to
> > +		     the first level of architected cache for the CPU.
> > +		     The next-level-cache property in architected cache nodes
> > +		     points to the respective next level of caching in the
> > +		     hierarchy. An architected cache node with an empty or
> > +		     missing next-level-cache property represents the last
> > +		     architected cache level for the CPU.
> > +		     On ARM v7 and v8 architectures, the order in which cache
> > +		     nodes are linked through the next-level-cache phandle must
> > +		     follow the ordering specified in the processors CLIDR (v7)
> 
> We shouldn't describe the ARM Architecture in the binding.  That's
> background information that could move outside.

Ok, I will try to reword it.

> > +		     and CLIDR_EL1 (v8) registers, as described in [1][2],
> > +		     implying that a cache node pointed at by a
> > +		     next-level-cache phandle must correspond to a level
> > +		     defined in CLIDR (v7) and CLIDR_EL1 (v8) greater than the
> > +		     one the cache node containing the next-level-cache
> > +		     phandle corresponds to.
> > +
> > +	Since on ARM most of the cache properties are probeable in HW the
> > +	properties described in [3] - section 3.8 multi-level and shared
> > +	caches - shall be considered optional, with the following properties
> > +	updates, specific for the ARM architected cache node.
> > +
> > +	- compatible
> > +		Usage: Required
> > +		Value type: <string>
> > +		Definition: value shall be "arm,arch-cache".
> > +
> > +	- interrupts
> > +		Usage: Optional
> > +		Value type: See definition
> > +		Definition: standard device tree property [3] that defines
> > +			    the interrupt line associated with the cache.
> > +			    The property can be accompanied by an
> > +			    interrupt-names property, as described in [4].
> 
> Do ARM Architectured caches ever have interrupts?  (Just my ignorance
> here.)

I added the property in preparation for Stephen's bindings on krait, but
since those caches are not architected caches, I was a bit overzealous.

Certainly it is a bit of a shame to redefine another binding for caches
on krait that basically have the SAME properties, and just add interrupts.

Thoughts appreciated.

> > +	- power-domain
> > +		Usage: Optional
> > +		Value type: phandle
> > +		Definition: A phandle and power domain specifier as defined by
> > +			    bindings of power controller specified by the
> > +			    phandle [5].
> > +
> > +Example(dual-cluster big.LITTLE system 32-bit)
> > +
> > +	cpus {
> > +		#size-cells = <0>;
> > +		#address-cells = <1>;
> > +
> > +		cpu@0 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a15";
> > +			reg = <0x0>;
> > +			next-level-cache = <&L1_0>;
> 
> ePAPR puts the L1 cache properties in the cpu node directly;
> there's no "L1" node as such.  That geometry description is also
> mandated by ePAPR.

I mentioned that all cache properties from ePAPR should be considered
optional. Now I will probably forbid them :)

> The ARM Architecture does not force a CPU to have any non-shared
> first level(s) of cache, and ePAPR does not permit next-level-cache
> to point to a cpu node, so there are possible ARM Architecture
> implementations that cannot be described without violating ePAPR.
> However, for practical reasons, such systems are unlikely -- I don't
> know if any exist today.
> 
> We should decide whether to deviate explicitly from ePAPR on this
> point (your examples provide on way), or whether to follow ePAPR and
> bodge things later for L1-less systems if they appear.

I defined the way it is so that I can always distinguish between a CPU
and its L1, that for power domain reasons, through phandles.

I might want to define a L1 that is retained and a CPU that loses context, if
I lump them together I need to cook up something else to distinguish them.

I think the way I defined it is cleaner, opinions appreciated.

Thanks for the review,
Lorenzo


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-21 14:35       ` Amit Kucheria
@ 2014-01-21 15:23         ` Lorenzo Pieralisi
  2014-01-22 11:52           ` Mark Brown
  0 siblings, 1 reply; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-21 15:23 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: devicetree@vger.kernel.org, Lists LAKML, Linux PM list,
	Dave P Martin, Mark Rutland, Sudeep Holla, Charles Garcia-Tobin,
	Nicolas Pitre, Rob Herring, Peter De Schrijver,
	grant.likely@linaro.org, Kumar Gala, Santosh Shilimkar,
	Mark Hambleton, Hanjun Guo, Daniel Lezcano, Vincent Guittot,
	Antti Miettinen, Stephen Boyd, Tomasz Figa, Kevin Hilman

Hi Amit,

On Tue, Jan 21, 2014 at 02:35:11PM +0000, Amit Kucheria wrote:
> Hi Lorenzo,
> 
> On Mon, Jan 20, 2014 at 11:17 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > ARM based platforms implement a variety of power management schemes that
> > allow processors to enter at run-time low-power states, aka C-states
> > in ACPI jargon. The parameters defining these C-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 C-state device tree bindings for
> > ARM platforms. Bindings define C-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 power states and removing the need for static data in current
> > and previous kernel versions.
> >
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> >  Documentation/devicetree/bindings/arm/c-states.txt | 774 +++++++++++++++++++++
> >  Documentation/devicetree/bindings/arm/cpus.txt     |  10 +
> >  2 files changed, 784 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/c-states.txt
> >
> > diff --git a/Documentation/devicetree/bindings/arm/c-states.txt b/Documentation/devicetree/bindings/arm/c-states.txt
> 
> s/c-states/idle-states?
> 
> While C-states are widely used when talking about idle-states as
> you've noted, idle-states are still the correct generic term for them.

C-states on ACPI are processor power states, I think we can keep the
same nomenclature. I do not mind changing it though, more comments
below.

> > new file mode 100644
> > index 0000000..0b5617b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/c-states.txt
> > @@ -0,0 +1,774 @@
> > +==========================================
> > +ARM C-states binding description
> > +==========================================
> > +
> > +==========================================
> > +1 - Introduction
> > +==========================================
> > +
> > +ARM systems contain HW capable of managing power consumption dynamically,
> > +where cores can be put in different low-power states (ranging from simple
> > +wfi to power gating) according to OSPM policies. Borrowing concepts
> > +from the ACPI specification[1], the CPU states representing the range of
> > +dynamic states that a processor can enter at run-time, aka C-state, can be
> > +specified through device tree bindings representing the parameters required to
> > +enter/exit specific C-states on a given processor.
> > +
> > +The state an ARM CPU can be put into is loosely identified by one of the
> > +following operating modes:
> > +
> > +- Running:
> > +        # Processor core is executing instructions
> > +
> > +- Wait for Interrupt:
> > +       # An ARM processor enters wait for interrupt (WFI) low power
> > +         state by executing a wfi instruction. When a processor enters
> > +         wfi state it disables most of the clocks while keeping the processor
> > +         powered up. This state is standard on all ARM processors and it is
> > +         defined as C1 in the remainder of this document.
> > +
> > +- Dormant:
> > +       # Dormant mode is entered by executing wfi instructions and by sending
> > +         platform specific commands to the platform power controller (coupled
> > +         with processor specific SW/HW control sequences).
> > +         In dormant mode, most of the processor control and debug logic is
> > +         powered up but cache RAM can be put in retention state, providing
> > +         additional power savings.
> > +
> > +- Sleep:
> > +       # Sleep mode is entered by executing the wfi instruction and by sending
> > +         platform specific commands to the platform power controller (coupled
> > +         with processor specific SW/HW control sequences). In sleep mode, a
> > +         processor and its caches are shutdown, the entire processor state is
> > +         lost.
> > +
> > +Building on top of the previous processor modes, ARM platforms implement power
> 
> Nitpick: s/previous/above

Ok.

> > +management schemes that allow an OS PM implementation to put the processor in
> > +different CPU states (C-states). C-states parameters (eg latency) are
> > +platform specific and need to be characterized with bindings that provide the
> > +required information to OSPM code so that it can build the required tables and
> > +use them at runtime.
> > +
> > +The device tree binding definition for ARM C-states is the subject of this
> > +document.
> > +
> > +===========================================
> > +2 - cpu-power-states node
> > +===========================================
> > +
> > +ARM processor C-states are defined within the cpu-power-states node, which is
> > +a direct child of the cpus node and provides a container where the processor
> > +states, defined as device tree nodes, are listed.
> > +
> > +- cpu-power-states node
> 
> What do you think of s/cpu-power-states/cpu-idle-states?
> 
> CPU Power management is more than just idle. Unless you have plans to
> add more properties to the cpu-power-states node later.

Ok, if by saying that CPU power management is more than just idle you
also mean managing power while processor is running (ie DVFS) I think
you have a point. Again, I do not mind changing it, keeping in mind that
names stick so if we think we require more info from these bindings,
and that's probably the case, we'd better stick to the current naming
scheme.

[...]

> > +       - latency
> > +               Usage: Required
> > +               Value type: <prop-encoded-array>
> > +               Definition: List of u32 values representing worst case latency
> > +                           in microseconds required to enter and exit the
> > +                           C-state, one value per OPP [2]. The list should
> > +                           be specified in the same order as the operating
> > +                           points property list of the cpu this state is
> > +                           valid on.
> > +                           If no OPP bindings are present, the latency value
> > +                           is associated with the current OPP of CPUs in the
> > +                           system.
> 
> DT-newbie here. What would happen if a vendor does not characterise
> the latency at each OPP? IOW, the table only contains latency values
> for a subset of the OPPs.

The bindings are explicit, so the kernel will barf. Adding a LUT to map
latencies to OPPs make me cringe, so I would not change the current
bindings.

> > +       - min-residency
> > +               Usage: Required
> > +               Value type: <prop-encoded-array>
> > +               Definition: List of u32 values representing time in
> > +                           microseconds required for the CPU to be in
> > +                           the C-state to make up for the dynamic power
> > +                           consumed to enter/exit the C-state in order to
> > +                           break even in terms of power consumption compared
> > +                           to C1 state (wfi), one value per-OPP [2].
> > +                           This parameter depends on the operating conditions
> > +                           (HW state) and must assume worst case scenario.
> > +                           The list should be specified in the same order as
> > +                           the operating points property list of the cpu this
> > +                           state is valid on.
> > +                           If no OPP bindings are present the min-residency
> > +                           value is associated with the current OPP of CPUs
> > +                           in the system.
> 
> Same question as latency above.

Same opinion, I am not keen on adding further complexity, after all if
some operating points are not characterized either they should be
disabled or people do not care, hence they can add estimated values just
as well to the respective latencies in the DT.

> > +
> > +===========================================
> > +3 - Examples
> > +===========================================
> > +
> > +Example 1 (ARM 64-bit, 16-cpu system, two clusters of clusters):
> 
> 
>      ^^^^^^^^^^^^^
>                                                          clusters of cpus?

Well I took it from an example where topology was clusters of clusters
but I removed the topology node and honestly it does not add anything to
the discussion so I will reword it.

Thanks for having a look,
Lorenzo


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-21 13:31     ` Lorenzo Pieralisi
  2014-01-21 14:35       ` Amit Kucheria
@ 2014-01-22 11:42       ` Mark Brown
  2014-01-22 16:33         ` Lorenzo Pieralisi
  1 sibling, 1 reply; 27+ messages in thread
From: Mark Brown @ 2014-01-22 11:42 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Vincent Guittot, Mark Rutland, devicetree@vger.kernel.org,
	Daniel Lezcano, Kevin Hilman, linux-pm@vger.kernel.org,
	Peter De Schrijver, Nicolas Pitre, Stephen Boyd, Antti Miettinen,
	Amit Kucheria, Tomasz Figa, Rob Herring, Santosh Shilimkar,
	Hanjun Guo, Mark Hambleton, Sudeep Holla, grant.likely@linaro.org,
	Kumar Gala, Dave P Martin, LAK, Charl

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

On Tue, Jan 21, 2014 at 01:31:48PM +0000, Lorenzo Pieralisi wrote:
> On Tue, Jan 21, 2014 at 11:16:46AM +0000, Vincent Guittot wrote:

> > > +       - psci-power-state
> > > +               Usage: Required if entry-method property value is set to
> > > +                      "psci".
> > > +               Value type: <u32>
> > > +               Definition: power_state parameter to pass to the PSCI
> > > +                           suspend call to enter the C-state.

> > Why psci has got a dedicated field and not vendor methods ? can't you
> > make that more generic ?

> If anyone provides me with an example usage why not, for now I know I
> need that parameter for PSCI, I can call it differently, define it for PSCI
> and leave it as optional for other methods.

Would it not be sensible to define a PSCI binding that extends this and
other bindings - ISTR some other properties getting scattered into
bindings for it?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-21 15:23         ` Lorenzo Pieralisi
@ 2014-01-22 11:52           ` Mark Brown
  2014-01-22 16:23             ` Lorenzo Pieralisi
  0 siblings, 1 reply; 27+ messages in thread
From: Mark Brown @ 2014-01-22 11:52 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Amit Kucheria, Mark Rutland, devicetree@vger.kernel.org,
	Daniel Lezcano, Vincent Guittot, Kevin Hilman, Linux PM list,
	Peter De Schrijver, Nicolas Pitre, Stephen Boyd, Antti Miettinen,
	Tomasz Figa, Rob Herring, Santosh Shilimkar, Hanjun Guo,
	Mark Hambleton, Sudeep Holla, grant.likely@linaro.org, Kumar Gala,
	Dave P Martin, Lists LAKML, Charles

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

On Tue, Jan 21, 2014 at 03:23:59PM +0000, Lorenzo Pieralisi wrote:
> On Tue, Jan 21, 2014 at 02:35:11PM +0000, Amit Kucheria wrote:

> > DT-newbie here. What would happen if a vendor does not characterise
> > the latency at each OPP? IOW, the table only contains latency values
> > for a subset of the OPPs.

> The bindings are explicit, so the kernel will barf. Adding a LUT to map
> latencies to OPPs make me cringe, so I would not change the current
> bindings.

Actually looking at the OPP binding I do wonder if it might not be
better to have a v2/rich binding for them which is extensible - the fact
that it's not possible to add additional information seems like an
issue, this can't be the only thing anyone might want to add and lining
up multiple tables is never fun.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-22 11:52           ` Mark Brown
@ 2014-01-22 16:23             ` Lorenzo Pieralisi
  2014-01-22 18:17               ` Mark Brown
  0 siblings, 1 reply; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-22 16:23 UTC (permalink / raw)
  To: Mark Brown
  Cc: Amit Kucheria, Mark Rutland, devicetree@vger.kernel.org,
	Daniel Lezcano, Vincent Guittot, Kevin Hilman, Linux PM list,
	Peter De Schrijver, Nicolas Pitre, Stephen Boyd, Antti Miettinen,
	Tomasz Figa, Rob Herring, Santosh Shilimkar, Hanjun Guo,
	Mark Hambleton, Sudeep Holla, grant.likely@linaro.org, Kumar Gala,
	Dave P Martin, Lists LAKML, Charles

Hi Mark,

On Wed, Jan 22, 2014 at 11:52:14AM +0000, Mark Brown wrote:
> On Tue, Jan 21, 2014 at 03:23:59PM +0000, Lorenzo Pieralisi wrote:
> > On Tue, Jan 21, 2014 at 02:35:11PM +0000, Amit Kucheria wrote:
> 
> > > DT-newbie here. What would happen if a vendor does not characterise
> > > the latency at each OPP? IOW, the table only contains latency values
> > > for a subset of the OPPs.
> 
> > The bindings are explicit, so the kernel will barf. Adding a LUT to map
> > latencies to OPPs make me cringe, so I would not change the current
> > bindings.
> 
> Actually looking at the OPP binding I do wonder if it might not be
> better to have a v2/rich binding for them which is extensible - the fact
> that it's not possible to add additional information seems like an
> issue, this can't be the only thing anyone might want to add and lining
> up multiple tables is never fun.

On one hand OPP bindings need improvement and that's on the cards. I am not
really following what you mean by "extensible", I only want to make sure
that the C-state bindings do not become too complex.

Do you mean extending OPP bindings to add eg C-state information there
(or whatever piece of information that is OPP dependent) ?
It seems a bit of a stretch but I can think about that.

I think that C-state properties are better defined in the C-state bindings
that was the idea but I am open to suggestions.

Thank you !
Lorenzo


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-22 11:42       ` Mark Brown
@ 2014-01-22 16:33         ` Lorenzo Pieralisi
  2014-01-22 18:11           ` Mark Brown
  0 siblings, 1 reply; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-22 16:33 UTC (permalink / raw)
  To: Mark Brown
  Cc: Vincent Guittot, Mark Rutland, devicetree@vger.kernel.org,
	Daniel Lezcano, Kevin Hilman, linux-pm@vger.kernel.org,
	Peter De Schrijver, Nicolas Pitre, Stephen Boyd, Antti Miettinen,
	Amit Kucheria, Tomasz Figa, Rob Herring, Santosh Shilimkar,
	Hanjun Guo, Mark Hambleton, Sudeep Holla, grant.likely@linaro.org,
	Kumar Gala, Dave P Martin, LAK, Charl

On Wed, Jan 22, 2014 at 11:42:32AM +0000, Mark Brown wrote:
> On Tue, Jan 21, 2014 at 01:31:48PM +0000, Lorenzo Pieralisi wrote:
> > On Tue, Jan 21, 2014 at 11:16:46AM +0000, Vincent Guittot wrote:
> 
> > > > +       - psci-power-state
> > > > +               Usage: Required if entry-method property value is set to
> > > > +                      "psci".
> > > > +               Value type: <u32>
> > > > +               Definition: power_state parameter to pass to the PSCI
> > > > +                           suspend call to enter the C-state.
> 
> > > Why psci has got a dedicated field and not vendor methods ? can't you
> > > make that more generic ?
> 
> > If anyone provides me with an example usage why not, for now I know I
> > need that parameter for PSCI, I can call it differently, define it for PSCI
> > and leave it as optional for other methods.
> 
> Would it not be sensible to define a PSCI binding that extends this and
> other bindings - ISTR some other properties getting scattered into
> bindings for it?

You mean adding the properties in the PSCI bindings instead of defining
them here ? Let me think about this, I really reckon these are C-state
specific properties that belong in here (but actually I have to add
a statement related to PSCI - ie bindings require a PSCI node to be
present and valid), I will look into this.

Thank you,
Lorenzo


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-22 16:33         ` Lorenzo Pieralisi
@ 2014-01-22 18:11           ` Mark Brown
  0 siblings, 0 replies; 27+ messages in thread
From: Mark Brown @ 2014-01-22 18:11 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Vincent Guittot, Mark Rutland, devicetree@vger.kernel.org,
	Daniel Lezcano, Kevin Hilman, linux-pm@vger.kernel.org,
	Peter De Schrijver, Nicolas Pitre, Stephen Boyd, Antti Miettinen,
	Amit Kucheria, Tomasz Figa, Rob Herring, Santosh Shilimkar,
	Hanjun Guo, Mark Hambleton, Sudeep Holla, grant.likely@linaro.org,
	Kumar Gala, Dave P Martin, LAK, Charl

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]

On Wed, Jan 22, 2014 at 04:33:22PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Jan 22, 2014 at 11:42:32AM +0000, Mark Brown wrote:

> > Would it not be sensible to define a PSCI binding that extends this and
> > other bindings - ISTR some other properties getting scattered into
> > bindings for it?

> You mean adding the properties in the PSCI bindings instead of defining
> them here ? Let me think about this, I really reckon these are C-state

Yes.

> specific properties that belong in here (but actually I have to add
> a statement related to PSCI - ie bindings require a PSCI node to be
> present and valid), I will look into this.

It depends how you think about it - something like PSCI is going to to
define properties used in a bunch of different parts of the low level
code, it seems reasonable for things like this to say that the user
should refer to the binding for the firmware to find out what options
that firmware requires and can support rather than having to refer to
each individual place and have those places enumerate the options for
each firmware.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-22 16:23             ` Lorenzo Pieralisi
@ 2014-01-22 18:17               ` Mark Brown
  0 siblings, 0 replies; 27+ messages in thread
From: Mark Brown @ 2014-01-22 18:17 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Amit Kucheria, Mark Rutland, devicetree@vger.kernel.org,
	Daniel Lezcano, Vincent Guittot, Kevin Hilman, Linux PM list,
	Peter De Schrijver, Nicolas Pitre, Stephen Boyd, Antti Miettinen,
	Tomasz Figa, Rob Herring, Santosh Shilimkar, Hanjun Guo,
	Mark Hambleton, Sudeep Holla, grant.likely@linaro.org, Kumar Gala,
	Dave P Martin, Lists LAKML, Charles

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On Wed, Jan 22, 2014 at 04:23:14PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Jan 22, 2014 at 11:52:14AM +0000, Mark Brown wrote:

> > Actually looking at the OPP binding I do wonder if it might not be
> > better to have a v2/rich binding for them which is extensible - the fact
> > that it's not possible to add additional information seems like an
> > issue, this can't be the only thing anyone might want to add and lining
> > up multiple tables is never fun.

> On one hand OPP bindings need improvement and that's on the cards. I am not
> really following what you mean by "extensible", I only want to make sure
> that the C-state bindings do not become too complex.

By extensible I guess I mostly mean not just a list of numbers we can't
add additional information to without breaking compatibility.  Having to
look through tables and make sure they all use the same indexes gets
error prone and generally miserable.

> Do you mean extending OPP bindings to add eg C-state information there
> (or whatever piece of information that is OPP dependent) ?
> It seems a bit of a stretch but I can think about that.

> I think that C-state properties are better defined in the C-state bindings
> that was the idea but I am open to suggestions.

I think defining in these bindings makes total sense, it's just
providing an extension to the OPP binding (which is already being looked
at anyway).  The exensibility issue I see is that with the current OPP
binding it's inelegant for another binding to add extra information
about an operating point relevant to that binding.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-21 11:16   ` Vincent Guittot
  2014-01-21 13:31     ` Lorenzo Pieralisi
@ 2014-01-22 19:20     ` Lorenzo Pieralisi
  2014-01-24  8:40       ` Vincent Guittot
  1 sibling, 1 reply; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-22 19:20 UTC (permalink / raw)
  To: Vincent Guittot
  Cc: devicetree@vger.kernel.org, LAK, linux-pm@vger.kernel.org,
	Dave P Martin, Mark Rutland, Sudeep Holla, Charles Garcia-Tobin,
	Nicolas Pitre, Rob Herring, Peter De Schrijver,
	grant.likely@linaro.org, Kumar Gala, Santosh Shilimkar,
	Mark Hambleton, Hanjun Guo, Daniel Lezcano, Amit Kucheria,
	Antti Miettinen, Stephen Boyd, Tomasz Figa, Kevin Hilman

Hi Vincent,

On Tue, Jan 21, 2014 at 11:16:46AM +0000, Vincent Guittot wrote:
> Hi Lorenzo,
> 
> On 20 January 2014 18:47, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > ARM based platforms implement a variety of power management schemes that
> > allow processors to enter at run-time low-power states, aka C-states
> > in ACPI jargon. The parameters defining these C-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 C-state device tree bindings for
> > ARM platforms. Bindings define C-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 power states and removing the need for static data in current
> > and previous kernel versions.
> >
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> >  Documentation/devicetree/bindings/arm/c-states.txt | 774 +++++++++++++++++++++
> >  Documentation/devicetree/bindings/arm/cpus.txt     |  10 +
> >  2 files changed, 784 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/c-states.txt
> >
> > diff --git a/Documentation/devicetree/bindings/arm/c-states.txt b/Documentation/devicetree/bindings/arm/c-states.txt
> > new file mode 100644
> > index 0000000..0b5617b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/c-states.txt
> > @@ -0,0 +1,774 @@
> > +==========================================
> > +ARM C-states binding description
> > +==========================================
> > +
> > +==========================================
> > +1 - Introduction
> > +==========================================
> > +
> > +ARM systems contain HW capable of managing power consumption dynamically,
> > +where cores can be put in different low-power states (ranging from simple
> > +wfi to power gating) according to OSPM policies. Borrowing concepts
> > +from the ACPI specification[1], the CPU states representing the range of
> > +dynamic states that a processor can enter at run-time, aka C-state, can be
> > +specified through device tree bindings representing the parameters required to
> > +enter/exit specific C-states on a given processor.
> > +
> > +The state an ARM CPU can be put into is loosely identified by one of the
> > +following operating modes:
> > +
> > +- Running:
> > +        # Processor core is executing instructions
> > +
> > +- Wait for Interrupt:
> > +       # An ARM processor enters wait for interrupt (WFI) low power
> > +         state by executing a wfi instruction. When a processor enters
> > +         wfi state it disables most of the clocks while keeping the processor
> > +         powered up. This state is standard on all ARM processors and it is
> > +         defined as C1 in the remainder of this document.
> > +
> 
> > +- Dormant:
> > +       # Dormant mode is entered by executing wfi instructions and by sending
> > +         platform specific commands to the platform power controller (coupled
> > +         with processor specific SW/HW control sequences).
> > +         In dormant mode, most of the processor control and debug logic is
> > +         powered up but cache RAM can be put in retention state, providing
> 
> Base on your description, it's not clear for me what is on, what is
> lost and what is power down ?
> My understand of the dormant mode that you described above is : the
> cache is preserved (and especially the cache RAM) but the processor
> state is lost (registers ...). Do I understand correctly ?
> 
> What about retention mode where the contents of processor and cache
> are preserved but the power consumption is reduced ? it can be seen as
> a special wfi mode which need specific SW/HW control sequences but i'm
> not sure to understand how to describe such state with your proposal.

I had an idea. To simplify things, I think that one possibility is to
add a parameter to the power domain specifier (platform specific, see
Tomasz bindings):

Documentation/devicetree/bindings/power/power_domain.txt

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/224928.html

to represent, when that state is entered the behavior of the power
controller (ie cache RAM retention or cache shutdown or in general any
substate within a power domain). Since it is platform specific, and since
we are able to link caches to the power domain, the power controller will
actually define what happens to the cache when that state is entered
(basically we use the power domain specifier additional parameter to define
a "substate" in that power domain e.g.:

Example:

foo_power_controller {
	[...]
	/*
	 * first cell is register index, second one is the state index
	 * that in turn implies the state behavior - eg cache lost or
	 * retained
	 */
	#power-domain-cells = <2>;
};

l1-cache {
	[...]
	/*
	 * syntax: power-domains = list of power domain specifiers
		<[&power_domain_phandle register-index state],[&power_domain_phandle register-index state]>;
		The syntax is defined by the power controller du jour
		as described by Tomasz bindings
	*/
	power-domains =<&foo_power_controller 0 0 &foo_power_controller 0 1>;

}:

and then

state0 {
	index = <2>;
	compatible = "arm,cpu-power-state";
	latency = <...>;
	/*
	 * This means that when the state is entered, the power
	 * controller should use register index 0 and state 0,
	 * whose meaning is power controller specific. Since we
	 * know all components affected (for every component
	 * we declare its power domain(s) and states so we
	 * know what components are affected by the state entry.
	 * Given the cache node above and this phandle, the state
	 * implies that the cache is retained, register index == 0 state == 0
	 /*
	power-domain =<&foo_power_controller 0 0>;
};

state1 {
	index = <3>;
	compatible = "arm,cpu-power-state";
	latency = <...>;
	/*
	 * This means that when the state is entered, the power
	 * controller should use register index 0 and state 1,
	 * whose meaning is power controller specific. Since we
	 * know all components affected (for every component
	 * we declare its power domain(s) and states so we
	 * know what components are affected by the state entry.
	 * Given the cache node above and this phandle, the state
	 * implies that the cache is lost, register index == 0 state == 1
	 /*
	power-domain =<&foo_power_controller 0 1>;
};

It is complex but it is probably the cleanest way. And it leaves complexity
to power controller implementations (if managed in the kernel....), which
actually makes sense because it is up to power controller to define the
behavior of certain states.

All in all it is just an idea, feel free to scotch it, it is complex but
we have to sort it out, one way or another.

Vincent, Tomasz, anyone, thoughts ?
Lorenzo


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-22 19:20     ` Lorenzo Pieralisi
@ 2014-01-24  8:40       ` Vincent Guittot
  2014-01-24 17:58         ` Lorenzo Pieralisi
  0 siblings, 1 reply; 27+ messages in thread
From: Vincent Guittot @ 2014-01-24  8:40 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: devicetree@vger.kernel.org, LAK, linux-pm@vger.kernel.org,
	Dave P Martin, Mark Rutland, Sudeep Holla, Charles Garcia-Tobin,
	Nicolas Pitre, Rob Herring, Peter De Schrijver,
	grant.likely@linaro.org, Kumar Gala, Santosh Shilimkar,
	Mark Hambleton, Hanjun Guo, Daniel Lezcano, Amit Kucheria,
	Antti Miettinen, Stephen Boyd, Tomasz Figa, Kevin Hilman

On 22 January 2014 20:20, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> Hi Vincent,
>
> On Tue, Jan 21, 2014 at 11:16:46AM +0000, Vincent Guittot wrote:
>> Hi Lorenzo,
>>
>> On 20 January 2014 18:47, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> > ARM based platforms implement a variety of power management schemes that
>> > allow processors to enter at run-time low-power states, aka C-states
>> > in ACPI jargon. The parameters defining these C-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 C-state device tree bindings for
>> > ARM platforms. Bindings define C-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 power states and removing the need for static data in current
>> > and previous kernel versions.
>> >
>> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> > ---
>> >  Documentation/devicetree/bindings/arm/c-states.txt | 774 +++++++++++++++++++++
>> >  Documentation/devicetree/bindings/arm/cpus.txt     |  10 +
>> >  2 files changed, 784 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/arm/c-states.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/arm/c-states.txt b/Documentation/devicetree/bindings/arm/c-states.txt
>> > new file mode 100644
>> > index 0000000..0b5617b
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/arm/c-states.txt
>> > @@ -0,0 +1,774 @@
>> > +==========================================
>> > +ARM C-states binding description
>> > +==========================================
>> > +
>> > +==========================================
>> > +1 - Introduction
>> > +==========================================
>> > +
>> > +ARM systems contain HW capable of managing power consumption dynamically,
>> > +where cores can be put in different low-power states (ranging from simple
>> > +wfi to power gating) according to OSPM policies. Borrowing concepts
>> > +from the ACPI specification[1], the CPU states representing the range of
>> > +dynamic states that a processor can enter at run-time, aka C-state, can be
>> > +specified through device tree bindings representing the parameters required to
>> > +enter/exit specific C-states on a given processor.
>> > +
>> > +The state an ARM CPU can be put into is loosely identified by one of the
>> > +following operating modes:
>> > +
>> > +- Running:
>> > +        # Processor core is executing instructions
>> > +
>> > +- Wait for Interrupt:
>> > +       # An ARM processor enters wait for interrupt (WFI) low power
>> > +         state by executing a wfi instruction. When a processor enters
>> > +         wfi state it disables most of the clocks while keeping the processor
>> > +         powered up. This state is standard on all ARM processors and it is
>> > +         defined as C1 in the remainder of this document.
>> > +
>>
>> > +- Dormant:
>> > +       # Dormant mode is entered by executing wfi instructions and by sending
>> > +         platform specific commands to the platform power controller (coupled
>> > +         with processor specific SW/HW control sequences).
>> > +         In dormant mode, most of the processor control and debug logic is
>> > +         powered up but cache RAM can be put in retention state, providing
>>
>> Base on your description, it's not clear for me what is on, what is
>> lost and what is power down ?
>> My understand of the dormant mode that you described above is : the
>> cache is preserved (and especially the cache RAM) but the processor
>> state is lost (registers ...). Do I understand correctly ?
>>
>> What about retention mode where the contents of processor and cache
>> are preserved but the power consumption is reduced ? it can be seen as
>> a special wfi mode which need specific SW/HW control sequences but i'm
>> not sure to understand how to describe such state with your proposal.
>

Hi Lorenzo,

Sorry for the late reply,


> I had an idea. To simplify things, I think that one possibility is to
> add a parameter to the power domain specifier (platform specific, see
> Tomasz bindings):

We can't use a simple boolean state (on/off) for defining the
powerdomain state associated to a c-state so your proposal of being
able to add a parameter that will define the power domain state is
interesting.

>
> Documentation/devicetree/bindings/power/power_domain.txt
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/224928.html
>
> to represent, when that state is entered the behavior of the power
> controller (ie cache RAM retention or cache shutdown or in general any
> substate within a power domain). Since it is platform specific, and since
> we are able to link caches to the power domain, the power controller will
> actually define what happens to the cache when that state is entered
> (basically we use the power domain specifier additional parameter to define
> a "substate" in that power domain e.g.:
>
> Example:
>
> foo_power_controller {
>         [...]
>         /*
>          * first cell is register index, second one is the state index
>          * that in turn implies the state behavior - eg cache lost or
>          * retained
>          */
>         #power-domain-cells = <2>;
> };
>
> l1-cache {
>         [...]
>         /*
>          * syntax: power-domains = list of power domain specifiers
>                 <[&power_domain_phandle register-index state],[&power_domain_phandle register-index state]>;
>                 The syntax is defined by the power controller du jour
>                 as described by Tomasz bindings
>         */
>         power-domains =<&foo_power_controller 0 0 &foo_power_controller 0 1>;

Normally, power-domains describes a list of power domain specifiers
that are necessary for the l1-cache to at least retain its state so
i'm not sure understand your example above

If we take the example of system that support running, retention and
powerdown state described as state 0, 1 and 2 for the power domain, i
would have set the l1-cache like:
       power-domains =<&foo_power_controller 0 1>;

for saying that the state is retained up to state 1

Please look below, i have modified the rest of your example accordingly

>
> }:
>
> and then
>
> state0 {
>         index = <2>;
>         compatible = "arm,cpu-power-state";
>         latency = <...>;
>         /*
>          * This means that when the state is entered, the power
>          * controller should use register index 0 and state 0,
>          * whose meaning is power controller specific. Since we
>          * know all components affected (for every component
>          * we declare its power domain(s) and states so we
>          * know what components are affected by the state entry.
>          * Given the cache node above and this phandle, the state
>          * implies that the cache is retained, register index == 0 state == 0
>          /*
>         power-domain =<&foo_power_controller 0 0>;

for retention state we need to set the power domain in state 1
        power-domain =<&foo_power_controller 0 1>;

> };
>
> state1 {
>         index = <3>;
>         compatible = "arm,cpu-power-state";
>         latency = <...>;
>         /*
>          * This means that when the state is entered, the power
>          * controller should use register index 0 and state 1,
>          * whose meaning is power controller specific. Since we
>          * know all components affected (for every component
>          * we declare its power domain(s) and states so we
>          * know what components are affected by the state entry.
>          * Given the cache node above and this phandle, the state
>          * implies that the cache is lost, register index == 0 state == 1
>          /*
>         power-domain =<&foo_power_controller 0 1>;

for power down mode, we need to set thge power domain in state 2
        power-domain =<&foo_power_controller 0 2>;


Vincent

> };
>
> It is complex but it is probably the cleanest way. And it leaves complexity
> to power controller implementations (if managed in the kernel....), which
> actually makes sense because it is up to power controller to define the
> behavior of certain states.
>
> All in all it is just an idea, feel free to scotch it, it is complex but
> we have to sort it out, one way or another.
>
> Vincent, Tomasz, anyone, thoughts ?
> Lorenzo
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-24  8:40       ` Vincent Guittot
@ 2014-01-24 17:58         ` Lorenzo Pieralisi
  2014-01-28  8:24           ` Vincent Guittot
  0 siblings, 1 reply; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-24 17:58 UTC (permalink / raw)
  To: Vincent Guittot
  Cc: devicetree@vger.kernel.org, LAK, linux-pm@vger.kernel.org,
	Dave P Martin, Mark Rutland, Sudeep Holla, Charles Garcia-Tobin,
	Nicolas Pitre, Rob Herring, Peter De Schrijver,
	grant.likely@linaro.org, Kumar Gala, Santosh Shilimkar,
	Mark Hambleton, Hanjun Guo, Daniel Lezcano, Amit Kucheria,
	Antti Miettinen, Stephen Boyd, Tomasz Figa, Kevin Hilman

Hi Vincent,

On Fri, Jan 24, 2014 at 08:40:40AM +0000, Vincent Guittot wrote:

[...]

> Hi Lorenzo,
> 
> Sorry for the late reply,
> 
> 
> > I had an idea. To simplify things, I think that one possibility is to
> > add a parameter to the power domain specifier (platform specific, see
> > Tomasz bindings):
> 
> We can't use a simple boolean state (on/off) for defining the
> powerdomain state associated to a c-state so your proposal of being
> able to add a parameter that will define the power domain state is
> interesting.
> 
> >
> > Documentation/devicetree/bindings/power/power_domain.txt
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/224928.html
> >
> > to represent, when that state is entered the behavior of the power
> > controller (ie cache RAM retention or cache shutdown or in general any
> > substate within a power domain). Since it is platform specific, and since
> > we are able to link caches to the power domain, the power controller will
> > actually define what happens to the cache when that state is entered
> > (basically we use the power domain specifier additional parameter to define
> > a "substate" in that power domain e.g.:
> >
> > Example:
> >
> > foo_power_controller {
> >         [...]
> >         /*
> >          * first cell is register index, second one is the state index
> >          * that in turn implies the state behavior - eg cache lost or
> >          * retained
> >          */
> >         #power-domain-cells = <2>;
> > };
> >
> > l1-cache {
> >         [...]
> >         /*
> >          * syntax: power-domains = list of power domain specifiers
> >                 <[&power_domain_phandle register-index state],[&power_domain_phandle register-index state]>;
> >                 The syntax is defined by the power controller du jour
> >                 as described by Tomasz bindings
> >         */
> >         power-domains =<&foo_power_controller 0 0 &foo_power_controller 0 1>;
> 
> Normally, power-domains describes a list of power domain specifiers
> that are necessary for the l1-cache to at least retain its state so
> i'm not sure understand your example above

> 
> If we take the example of system that support running, retention and
> powerdown state described as state 0, 1 and 2 for the power domain, i
> would have set the l1-cache like:
>        power-domains =<&foo_power_controller 0 1>;
> 
> for saying that the state is retained up to state 1
> 
> Please look below, i have modified the rest of your example accordingly
> 
> >
> > }:
> >
> > and then
> >
> > state0 {
> >         index = <2>;
> >         compatible = "arm,cpu-power-state";
> >         latency = <...>;
> >         /*
> >          * This means that when the state is entered, the power
> >          * controller should use register index 0 and state 0,
> >          * whose meaning is power controller specific. Since we
> >          * know all components affected (for every component
> >          * we declare its power domain(s) and states so we
> >          * know what components are affected by the state entry.
> >          * Given the cache node above and this phandle, the state
> >          * implies that the cache is retained, register index == 0 state == 0
> >          /*
> >         power-domain =<&foo_power_controller 0 0>;
> 
> for retention state we need to set the power domain in state 1
>         power-domain =<&foo_power_controller 0 1>;
> 
> > };
> >
> > state1 {
> >         index = <3>;
> >         compatible = "arm,cpu-power-state";
> >         latency = <...>;
> >         /*
> >          * This means that when the state is entered, the power
> >          * controller should use register index 0 and state 1,
> >          * whose meaning is power controller specific. Since we
> >          * know all components affected (for every component
> >          * we declare its power domain(s) and states so we
> >          * know what components are affected by the state entry.
> >          * Given the cache node above and this phandle, the state
> >          * implies that the cache is lost, register index == 0 state == 1
> >          /*
> >         power-domain =<&foo_power_controller 0 1>;
> 
> for power down mode, we need to set thge power domain in state 2
>         power-domain =<&foo_power_controller 0 2>;

Ok, what I meant was not what you got, but your approach looks sensible
too. What I do not like is that the power-domain specifier is power
controller specific (that was true even for my example). In theory
we can achieve something identical by forcing every component in a power
domain to specify the max C-state index that allows it to retain its
state (through a specific property). Same logic to your example applies.
Nice thing is that we do not change the power domain specifiers, bad thing
is that it adds two properties to each device (c-state index and
power-domain-specifier - but we can make it hierarchical so that device
nodes can inherit the maximum operating C-state by inheriting the value
from a parent node providing a common value).

In my example the third parameter was just a number that the power
controller would decode (eg 0 = cache retained, 1 = cache lost)
according to its implementation, it was not a "state index". The
power controller would know what to do with eg a cache component (that
declares to be in that power domain) when a C-state with that power
domain specifier was entered.

Not very different from what you are saying, let's get to the nub:

- Either we define it in a platform specific way through the power
  domain specifier
- Or we force a max-c-state-supported property for every device,
  possibly hierarchical

Thoughts ?

Thank you !
Lorenzo


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-20 17:47 ` [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings Lorenzo Pieralisi
  2014-01-21 11:16   ` Vincent Guittot
@ 2014-01-25  8:15   ` Antti P Miettinen
  2014-01-27 11:41     ` Lorenzo Pieralisi
  1 sibling, 1 reply; 27+ messages in thread
From: Antti P Miettinen @ 2014-01-25  8:15 UTC (permalink / raw)
  To: lorenzo.pieralisi
  Cc: mark.rutland, devicetree, daniel.lezcano, vincent.guittot,
	khilman, linux-pm, pdeschrijver, nico, sboyd, amit.kucheria,
	t.figa, robh+dt, santosh.shilimkar, hanjun.guo, mark.hambleton,
	sudeep.holla, grant.likely, galak, dave.martin, linux-arm-kernel,
	Charles.Garcia-Tobin

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Subject: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
Date: Mon, 20 Jan 2014 17:47:59 +0000
> +	- latency
> +		Usage: Required
> +		Value type: <prop-encoded-array>
> +		Definition: List of u32 values representing worst case latency
> +			    in microseconds required to enter and exit the
> +			    C-state, one value per OPP [2]. The list should
> +			    be specified in the same order as the operating
> +			    points property list of the cpu this state is
> +			    valid on.
> +			    If no OPP bindings are present, the latency value
> +			    is associated with the current OPP of CPUs in the
> +			    system.

I'm afraid the CPU OPP is not enough to capture the variance in
latencies. Especially memory frequency affects some of the latencies
very stronly.

	--Antti

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-25  8:15   ` Antti P Miettinen
@ 2014-01-27 11:41     ` Lorenzo Pieralisi
  2014-01-27 12:48       ` Antti P Miettinen
  0 siblings, 1 reply; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-27 11:41 UTC (permalink / raw)
  To: Antti P Miettinen
  Cc: Mark Rutland, devicetree@vger.kernel.org,
	daniel.lezcano@linaro.org, vincent.guittot@linaro.org,
	khilman@linaro.org, linux-pm@vger.kernel.org,
	pdeschrijver@nvidia.com, nico@linaro.org, sboyd@codeaurora.org,
	amit.kucheria@linaro.org, t.figa@samsung.com, robh+dt@kernel.org,
	santosh.shilimkar@ti.com, hanjun.guo@linaro.org,
	mark.hambleton@broadcom.com, Sudeep Holla,
	grant.likely@linaro.org, galak@codeaurora.org

On Sat, Jan 25, 2014 at 08:15:46AM +0000, Antti P Miettinen wrote:
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Subject: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
> Date: Mon, 20 Jan 2014 17:47:59 +0000
> > +	- latency
> > +		Usage: Required
> > +		Value type: <prop-encoded-array>
> > +		Definition: List of u32 values representing worst case latency
> > +			    in microseconds required to enter and exit the
> > +			    C-state, one value per OPP [2]. The list should
> > +			    be specified in the same order as the operating
> > +			    points property list of the cpu this state is
> > +			    valid on.
> > +			    If no OPP bindings are present, the latency value
> > +			    is associated with the current OPP of CPUs in the
> > +			    system.
> 
> I'm afraid the CPU OPP is not enough to capture the variance in
> latencies. Especially memory frequency affects some of the latencies
> very stronly.

That's why I defined the worst case. How did you implemented it in your
idle drivers ? That would help generalize it, after all these bindings
are there to simplify drivers upstreaming, feedback welcome.

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-27 11:41     ` Lorenzo Pieralisi
@ 2014-01-27 12:48       ` Antti P Miettinen
  2014-01-27 18:22         ` Lorenzo Pieralisi
  0 siblings, 1 reply; 27+ messages in thread
From: Antti P Miettinen @ 2014-01-27 12:48 UTC (permalink / raw)
  To: lorenzo.pieralisi
  Cc: devicetree, linux-arm-kernel, linux-pm, Dave.Martin, Mark.Rutland,
	Sudeep.Holla, Charles.Garcia-Tobin, nico, robh+dt, pdeschrijver,
	grant.likely, galak, santosh.shilimkar, mark.hambleton,
	hanjun.guo, daniel.lezcano, amit.kucheria, vincent.guittot, sboyd,
	t.figa, khilman

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> That's why I defined the worst case. How did you implemented it in your
> idle drivers ? That would help generalize it, after all these bindings
> are there to simplify drivers upstreaming, feedback welcome.

Currently we do not handle this well downstream either. The problem
with worst case is that the absolute worst case can be really bad and
probability of it might be very low. Sorry - no ready answer :-)

	--Antti

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 1/2] Documentation: arm: add cache DT bindings
       [not found]     ` <20140121114845.GA2598-M5GwZQ6tE7x5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org>
@ 2014-01-27 12:58       ` Russell King - ARM Linux
  2014-01-27 18:10         ` Lorenzo Pieralisi
  0 siblings, 1 reply; 27+ messages in thread
From: Russell King - ARM Linux @ 2014-01-27 12:58 UTC (permalink / raw)
  To: Dave Martin
  Cc: Lorenzo Pieralisi, Mark Rutland,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Kevin Hilman, Vincent Guittot,
	Nicolas Pitre, Amit Kucheria, Tomasz Figa, Daniel Lezcano,
	Stephen Boyd, Rob Herring, Sudeep Holla, Kumar Gala,
	Santosh Shilimkar, Antti Miettinen, Mark Hambleton,
	linux-pm-u79uwXL29TY76Z2rM5mHXA, Grant Likely, Hanjun Guo,
	Peter De Schrijver,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Charles Garcia Tobin

On Tue, Jan 21, 2014 at 11:49:01AM +0000, Dave Martin wrote:
> I do have a worry that because the kernel won't normally use this
> information, by default it will get pasted between .dts files, won't get
> tested and will be wrong rather often.  It also violates the DT principle
> that probeable information should not be present in the DT -- ePAPR
> obviously envisages systems where cache geometry information is not
> probeable, but that's not the case for architected caches on ARM, except
> in rare cases where the CLIDR is wrong.

That statement is wrong.  There are caches on ARM CPUs where there is no
CLIDR register.  I suggest reading the earlier DDI0100 revisions.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 1/2] Documentation: arm: add cache DT bindings
  2014-01-27 12:58       ` Russell King - ARM Linux
@ 2014-01-27 18:10         ` Lorenzo Pieralisi
  0 siblings, 0 replies; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-27 18:10 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Dave P Martin, Mark Rutland, devicetree@vger.kernel.org,
	Kevin Hilman, Vincent Guittot, Nicolas Pitre, Amit Kucheria,
	Tomasz Figa, Daniel Lezcano, Stephen Boyd, Rob Herring,
	Sudeep Holla, Kumar Gala, Santosh Shilimkar, Antti Miettinen,
	Mark Hambleton, linux-pm@vger.kernel.org, grant.likely@linaro.org,
	Hanjun Guo, Peter De Schrijver,
	"linux-arm-kernel@lists.infradead.org" <linux-ar>

On Mon, Jan 27, 2014 at 12:58:39PM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 21, 2014 at 11:49:01AM +0000, Dave Martin wrote:
> > I do have a worry that because the kernel won't normally use this
> > information, by default it will get pasted between .dts files, won't get
> > tested and will be wrong rather often.  It also violates the DT principle
> > that probeable information should not be present in the DT -- ePAPR
> > obviously envisages systems where cache geometry information is not
> > probeable, but that's not the case for architected caches on ARM, except
> > in rare cases where the CLIDR is wrong.
> 
> That statement is wrong.  There are caches on ARM CPUs where there is no
> CLIDR register.  I suggest reading the earlier DDI0100 revisions.

You are right, Dave was referring to the cache geometry properties in the
ePAPRv1.1, and the question on whether to ignore them for ARM. True, some
earlier ARM processors would need DT properties to define cache geometry owing
to the lack of cache type/id registers, but I guess we can work around that
and safely rule cache geometry properties out for ARM (better that than
having people rely on dts files containing wrong copy'n'pasted cache
geometry properties, that's the reasoning). The only reason we are defining
these bindings is to make sure we are able to detect which CPUs share what
caches, we can work around the lack of cache type/id registers to probe
geometry in earlier processors in the kernel.

Thanks,
Lorenzo


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 1/2] Documentation: arm: add cache DT bindings
@ 2014-01-27 18:11 Dave Martin
  0 siblings, 0 replies; 27+ messages in thread
From: Dave Martin @ 2014-01-27 18:11 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Mark Rutland, Tomasz Figa, Mark Hambleton, Lorenzo Pieralisi,
	Vincent Guittot, Nicolas Pitre, Daniel Lezcano, Antti Miettinen,
	Grant Likely, Charles Garcia Tobin, devicetree, Kevin Hilman,
	linux-pm, Kumar Gala, Rob Herring, linux-arm-kernel,
	Peter De Schrijver, Stephen Boyd, Amit Kucheria,
	Santosh Shilimkar, Hanjun Guo, Sudeep Holla

On Mon, Jan 27, 2014 at 12:58:39PM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 21, 2014 at 11:49:01AM +0000, Dave Martin wrote:
> > I do have a worry that because the kernel won't normally use this
> > information, by default it will get pasted between .dts files, won't get
> > tested and will be wrong rather often.  It also violates the DT principle
> > that probeable information should not be present in the DT -- ePAPR
> > obviously envisages systems where cache geometry information is not
> > probeable, but that's not the case for architected caches on ARM, except
> > in rare cases where the CLIDR is wrong.
> 
> That statement is wrong.  There are caches on ARM CPUs where there is no
> CLIDR register.  I suggest reading the earlier DDI0100 revisions.

You're right; I should have qualified that statement with the proviso
that there _is_ a CLIDR (or some other reliably way to discover the
cache geometry directly from the hardware).  In particular, this applies
to all v7-[AR] CPUs, but not to <=v5.  Not sure about v6 -- it may be
a mixture.

My worry was about duplication of information; so if there is no discovery
mechanism then there is no duplication and no problem -- on those older
CPUs, the corresponding information could be placed in the DT, or where
it doesn't cause a problem it can remain hard-coded into the kernel, as
at present.  If we _allow_ inclusion of that information in the DT for
pre-v7, that still raises questions about what information gets used if
there is hard-coded geometry in the kernel too.

I can't see a sufficient reason to advocate churn for pre-v7 platforms,
but other people's views may differ.

Cheers
---Dave

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-27 12:48       ` Antti P Miettinen
@ 2014-01-27 18:22         ` Lorenzo Pieralisi
  0 siblings, 0 replies; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-27 18:22 UTC (permalink / raw)
  To: Antti P Miettinen
  Cc: Mark Rutland, devicetree@vger.kernel.org,
	daniel.lezcano@linaro.org, vincent.guittot@linaro.org,
	khilman@linaro.org, linux-pm@vger.kernel.org,
	pdeschrijver@nvidia.com, nico@linaro.org, sboyd@codeaurora.org,
	amit.kucheria@linaro.org, t.figa@samsung.com, robh+dt@kernel.org,
	santosh.shilimkar@ti.com, hanjun.guo@linaro.org,
	mark.hambleton@broadcom.com, Sudeep Holla,
	grant.likely@linaro.org, galak@codeaurora.org

On Mon, Jan 27, 2014 at 12:48:15PM +0000, Antti P Miettinen wrote:
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > That's why I defined the worst case. How did you implemented it in your
> > idle drivers ? That would help generalize it, after all these bindings
> > are there to simplify drivers upstreaming, feedback welcome.
> 
> Currently we do not handle this well downstream either. The problem
> with worst case is that the absolute worst case can be really bad and
> probability of it might be very low. Sorry - no ready answer :-)

Point taken, but these bindings still get us to a point that is much
better than today situation. After all, if the worst case can happen
either we design for worst case or we update parameters at runtime in
the kernel (which is not happening as of now) according to a notification
mechanism.

It is certainly worth investigating, probably we can define OPPs as
generic (ie not tied to the CPU), as performance point or system
operating points. I will think about this.

In the meantime if you have further pieces of feedback please keep them
coming.

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-24 17:58         ` Lorenzo Pieralisi
@ 2014-01-28  8:24           ` Vincent Guittot
  2014-01-29 12:42             ` Lorenzo Pieralisi
  0 siblings, 1 reply; 27+ messages in thread
From: Vincent Guittot @ 2014-01-28  8:24 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: devicetree@vger.kernel.org, LAK, linux-pm@vger.kernel.org,
	Dave P Martin, Mark Rutland, Sudeep Holla, Charles Garcia-Tobin,
	Nicolas Pitre, Rob Herring, Peter De Schrijver,
	grant.likely@linaro.org, Kumar Gala, Santosh Shilimkar,
	Mark Hambleton, Hanjun Guo, Daniel Lezcano, Amit Kucheria,
	Antti Miettinen, Stephen Boyd, Tomasz Figa, Kevin Hilman

On 24 January 2014 18:58, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> Hi Vincent,
>
> On Fri, Jan 24, 2014 at 08:40:40AM +0000, Vincent Guittot wrote:
>
> [...]
>
>> Hi Lorenzo,
>>
>> Sorry for the late reply,
>>
>>
>> > I had an idea. To simplify things, I think that one possibility is to
>> > add a parameter to the power domain specifier (platform specific, see
>> > Tomasz bindings):
>>
>> We can't use a simple boolean state (on/off) for defining the
>> powerdomain state associated to a c-state so your proposal of being
>> able to add a parameter that will define the power domain state is
>> interesting.
>>
>> >
>> > Documentation/devicetree/bindings/power/power_domain.txt
>> >
>> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/224928.html
>> >
>> > to represent, when that state is entered the behavior of the power
>> > controller (ie cache RAM retention or cache shutdown or in general any
>> > substate within a power domain). Since it is platform specific, and since
>> > we are able to link caches to the power domain, the power controller will
>> > actually define what happens to the cache when that state is entered
>> > (basically we use the power domain specifier additional parameter to define
>> > a "substate" in that power domain e.g.:
>> >
>> > Example:
>> >
>> > foo_power_controller {
>> >         [...]
>> >         /*
>> >          * first cell is register index, second one is the state index
>> >          * that in turn implies the state behavior - eg cache lost or
>> >          * retained
>> >          */
>> >         #power-domain-cells = <2>;
>> > };
>> >
>> > l1-cache {
>> >         [...]
>> >         /*
>> >          * syntax: power-domains = list of power domain specifiers
>> >                 <[&power_domain_phandle register-index state],[&power_domain_phandle register-index state]>;
>> >                 The syntax is defined by the power controller du jour
>> >                 as described by Tomasz bindings
>> >         */
>> >         power-domains =<&foo_power_controller 0 0 &foo_power_controller 0 1>;
>>
>> Normally, power-domains describes a list of power domain specifiers
>> that are necessary for the l1-cache to at least retain its state so
>> i'm not sure understand your example above
>
>>
>> If we take the example of system that support running, retention and
>> powerdown state described as state 0, 1 and 2 for the power domain, i
>> would have set the l1-cache like:
>>        power-domains =<&foo_power_controller 0 1>;
>>
>> for saying that the state is retained up to state 1
>>
>> Please look below, i have modified the rest of your example accordingly
>>
>> >
>> > }:
>> >
>> > and then
>> >
>> > state0 {
>> >         index = <2>;
>> >         compatible = "arm,cpu-power-state";
>> >         latency = <...>;
>> >         /*
>> >          * This means that when the state is entered, the power
>> >          * controller should use register index 0 and state 0,
>> >          * whose meaning is power controller specific. Since we
>> >          * know all components affected (for every component
>> >          * we declare its power domain(s) and states so we
>> >          * know what components are affected by the state entry.
>> >          * Given the cache node above and this phandle, the state
>> >          * implies that the cache is retained, register index == 0 state == 0
>> >          /*
>> >         power-domain =<&foo_power_controller 0 0>;
>>
>> for retention state we need to set the power domain in state 1
>>         power-domain =<&foo_power_controller 0 1>;
>>
>> > };
>> >
>> > state1 {
>> >         index = <3>;
>> >         compatible = "arm,cpu-power-state";
>> >         latency = <...>;
>> >         /*
>> >          * This means that when the state is entered, the power
>> >          * controller should use register index 0 and state 1,
>> >          * whose meaning is power controller specific. Since we
>> >          * know all components affected (for every component
>> >          * we declare its power domain(s) and states so we
>> >          * know what components are affected by the state entry.
>> >          * Given the cache node above and this phandle, the state
>> >          * implies that the cache is lost, register index == 0 state == 1
>> >          /*
>> >         power-domain =<&foo_power_controller 0 1>;
>>
>> for power down mode, we need to set thge power domain in state 2
>>         power-domain =<&foo_power_controller 0 2>;
>
> Ok, what I meant was not what you got, but your approach looks sensible
> too. What I do not like is that the power-domain specifier is power

sorry for the misconception of your example

> controller specific (that was true even for my example). In theory
> we can achieve something identical by forcing every component in a power
> domain to specify the max C-state index that allows it to retain its

I'm not sure that we should force a component to set an opaque (for
the component) max c-state. The device should describe its power
domain requirements and the correlation of the latter with the
description of the c-state binding should be enough to deduct the max
c-state.

> state (through a specific property). Same logic to your example applies.
> Nice thing is that we do not change the power domain specifiers, bad thing
> is that it adds two properties to each device (c-state index and
> power-domain-specifier - but we can make it hierarchical so that device
> nodes can inherit the maximum operating C-state by inheriting the value
> from a parent node providing a common value).
>
> In my example the third parameter was just a number that the power
> controller would decode (eg 0 = cache retained, 1 = cache lost)
> according to its implementation, it was not a "state index". The
> power controller would know what to do with eg a cache component (that
> declares to be in that power domain) when a C-state with that power
> domain specifier was entered.
>
> Not very different from what you are saying, let's get to the nub:
>
> - Either we define it in a platform specific way through the power
>   domain specifier
> - Or we force a max-c-state-supported property for every device,
>   possibly hierarchical

As explained above, adding a max-cstate property for a device that
only know the power-domain is not a good thing IMHO.

Vincent

>
> Thoughts ?
>
> Thank you !
> Lorenzo
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
  2014-01-28  8:24           ` Vincent Guittot
@ 2014-01-29 12:42             ` Lorenzo Pieralisi
  0 siblings, 0 replies; 27+ messages in thread
From: Lorenzo Pieralisi @ 2014-01-29 12:42 UTC (permalink / raw)
  To: Vincent Guittot
  Cc: devicetree@vger.kernel.org, LAK, linux-pm@vger.kernel.org,
	Dave P Martin, Mark Rutland, Sudeep Holla, Charles Garcia-Tobin,
	Nicolas Pitre, Rob Herring, Peter De Schrijver,
	grant.likely@linaro.org, Kumar Gala, Santosh Shilimkar,
	Mark Hambleton, Hanjun Guo, Daniel Lezcano, Amit Kucheria,
	Antti Miettinen, Stephen Boyd, Tomasz Figa, Kevin Hilman

On Tue, Jan 28, 2014 at 08:24:54AM +0000, Vincent Guittot wrote:
> On 24 January 2014 18:58, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
[...]

> >> Please look below, i have modified the rest of your example accordingly
> >>
> >> >
> >> > }:
> >> >
> >> > and then
> >> >
> >> > state0 {
> >> >         index = <2>;
> >> >         compatible = "arm,cpu-power-state";
> >> >         latency = <...>;
> >> >         /*
> >> >          * This means that when the state is entered, the power
> >> >          * controller should use register index 0 and state 0,
> >> >          * whose meaning is power controller specific. Since we
> >> >          * know all components affected (for every component
> >> >          * we declare its power domain(s) and states so we
> >> >          * know what components are affected by the state entry.
> >> >          * Given the cache node above and this phandle, the state
> >> >          * implies that the cache is retained, register index == 0 state == 0
> >> >          /*
> >> >         power-domain =<&foo_power_controller 0 0>;
> >>
> >> for retention state we need to set the power domain in state 1
> >>         power-domain =<&foo_power_controller 0 1>;
> >>
> >> > };
> >> >
> >> > state1 {
> >> >         index = <3>;
> >> >         compatible = "arm,cpu-power-state";
> >> >         latency = <...>;
> >> >         /*
> >> >          * This means that when the state is entered, the power
> >> >          * controller should use register index 0 and state 1,
> >> >          * whose meaning is power controller specific. Since we
> >> >          * know all components affected (for every component
> >> >          * we declare its power domain(s) and states so we
> >> >          * know what components are affected by the state entry.
> >> >          * Given the cache node above and this phandle, the state
> >> >          * implies that the cache is lost, register index == 0 state == 1
> >> >          /*
> >> >         power-domain =<&foo_power_controller 0 1>;
> >>
> >> for power down mode, we need to set thge power domain in state 2
> >>         power-domain =<&foo_power_controller 0 2>;
> >
> > Ok, what I meant was not what you got, but your approach looks sensible
> > too. What I do not like is that the power-domain specifier is power
> 
> sorry for the misconception of your example
> 
> > controller specific (that was true even for my example). In theory
> > we can achieve something identical by forcing every component in a power
> > domain to specify the max C-state index that allows it to retain its
> 
> I'm not sure that we should force a component to set an opaque (for
> the component) max c-state. The device should describe its power
> domain requirements and the correlation of the latter with the
> description of the c-state binding should be enough to deduct the max
> c-state.

I agree, that was an option, I just loathe the idea of implementing it.
Using power domain specifiers is ways cleaner IMHO, the only drawback is
that, it is up to the power domain documentation to define what a state
means in terms of save/restore and cache behavior. I think that makes
perfect sense, at least for me.

> > state (through a specific property). Same logic to your example applies.
> > Nice thing is that we do not change the power domain specifiers, bad thing
> > is that it adds two properties to each device (c-state index and
> > power-domain-specifier - but we can make it hierarchical so that device
> > nodes can inherit the maximum operating C-state by inheriting the value
> > from a parent node providing a common value).
> >
> > In my example the third parameter was just a number that the power
> > controller would decode (eg 0 = cache retained, 1 = cache lost)
> > according to its implementation, it was not a "state index". The
> > power controller would know what to do with eg a cache component (that
> > declares to be in that power domain) when a C-state with that power
> > domain specifier was entered.
> >
> > Not very different from what you are saying, let's get to the nub:
> >
> > - Either we define it in a platform specific way through the power
> >   domain specifier
> > - Or we force a max-c-state-supported property for every device,
> >   possibly hierarchical
> 
> As explained above, adding a max-cstate property for a device that
> only know the power-domain is not a good thing IMHO.

I agree, if nobody complains that's the way I will define the bindings.

Thank you,
Lorenzo


^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2014-01-29 12:42 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-20 17:47 [PATCH RFC v2 0/2] ARM: defining power states DT bindings Lorenzo Pieralisi
2014-01-20 17:47 ` [PATCH RFC v2 1/2] Documentation: arm: add cache " Lorenzo Pieralisi
2014-01-21 11:49   ` Dave Martin
2014-01-21 14:47     ` Lorenzo Pieralisi
     [not found]     ` <20140121114845.GA2598-M5GwZQ6tE7x5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org>
2014-01-27 12:58       ` Russell King - ARM Linux
2014-01-27 18:10         ` Lorenzo Pieralisi
2014-01-20 17:47 ` [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings Lorenzo Pieralisi
2014-01-21 11:16   ` Vincent Guittot
2014-01-21 13:31     ` Lorenzo Pieralisi
2014-01-21 14:35       ` Amit Kucheria
2014-01-21 15:23         ` Lorenzo Pieralisi
2014-01-22 11:52           ` Mark Brown
2014-01-22 16:23             ` Lorenzo Pieralisi
2014-01-22 18:17               ` Mark Brown
2014-01-22 11:42       ` Mark Brown
2014-01-22 16:33         ` Lorenzo Pieralisi
2014-01-22 18:11           ` Mark Brown
2014-01-22 19:20     ` Lorenzo Pieralisi
2014-01-24  8:40       ` Vincent Guittot
2014-01-24 17:58         ` Lorenzo Pieralisi
2014-01-28  8:24           ` Vincent Guittot
2014-01-29 12:42             ` Lorenzo Pieralisi
2014-01-25  8:15   ` Antti P Miettinen
2014-01-27 11:41     ` Lorenzo Pieralisi
2014-01-27 12:48       ` Antti P Miettinen
2014-01-27 18:22         ` Lorenzo Pieralisi
  -- strict thread matches above, loose matches on Subject: below --
2014-01-27 18:11 [PATCH RFC v2 1/2] Documentation: arm: add cache DT bindings Dave Martin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).