[PATCH V5 0/3] OPP: Introduce OPP (V2) bindings

[PATCH V5 0/3] OPP: Introduce OPP (V2) bindings

[Viresh Kumar]

Hi Guys,

Finally V4 got some good review comments, Acks, etc.. I have updated the
bindings with all review comments and here is V5.

V4->V5:
- opp-microamp fixed and rewritten as per Mark's suggestions.
- shared-opp renamed as opp-shared, as that's the convention for other
  properties.
- Dropped "[V4 3/3] OPP: Add 'opp-next' in operating-points-v2 bindings" as that
  was NAK'd by Mike T..
- Added [V5 3/3] based on Nishanth's suggestions.
- Added an example for 2/3, multiple OPP nodes.
  @Stephen/Nishanth: I have kept your tags here as I haven't changed the binding
  but only an example. Please let me know if you have some comments.

- Other minor formatting..
- Existing binding: "operating-points" isn't deprecated now as platforms looking
  for simple bindings should be allowed to use them.
- opp-khz is changed to opp-hz, examples updated.
- turbo-mode explained


V3->V4:
- Dropped code changes as we are still concerned about bindings.
- separated out into three patches, some of which might be NAK'd. :)
- The first patch presents basic OPP stuff that was reviewed earlier. It also
  has support for multiple regulators, with values for both current and voltage.
- Second patch is based on a special concern that Stephen had about multiple OPP
  tables, one of which the parsing code will select at runtime.
- Third one separates out 'opp-next' or Intermediate freq support as Mike T. had
  few concerns over it. He wanted the clock driver to take care of this and so
  do not want it to be passed by DT and used by cpufreq. Also, there were
  concerns like the platform may not want to choose intermediate frequency as a
  target frequency for longer runs, which wasn't prevented in earlier bindings.
  And so it is kept separate to be NAK'd quietly, without much disturbances.

  ---------------x-------------------x------------------------

Current OPP (Operating performance point) DT bindings are proven to be
insufficient at multiple instances.

The shortcomings we are trying to solve here:

- Getting clock/voltage/current rails sharing information between CPUs.
  Shared by all cores vs independent clock per core vs shared clock per
  cluster.

- Support for specifying current levels along with voltages.

- Support for multiple regulators.

- Support for turbo modes.

- Other per OPP settings: transition latencies, disabled status, etc.?

- Expandability of OPPs in future.

This patchset tries to solve these shortcomings with a new "operating-points-v2"
binding, which can be easily extended later if required.

Viresh Kumar (3):
  OPP: Redefine bindings to overcome shortcomings
  OPP: Allow multiple OPP tables to be passed via DT
  OPP: Add binding for 'opp-suspend'

 Documentation/devicetree/bindings/power/opp.txt | 437 +++++++++++++++++++++++-
 1 file changed, 433 insertions(+), 4 deletions(-)

-- 
2.4.0

[PATCH V5 1/3] OPP: Redefine bindings to overcome shortcomings

[Viresh Kumar]

Current OPP (Operating performance point) DT bindings are proven to be
insufficient at multiple instances.

The shortcomings we are trying to solve here:

- Getting clock/voltage/current rails sharing information between CPUs.
  Shared by all cores vs independent clock per core vs shared clock per
  cluster.

- Support for specifying current levels along with voltages.

- Support for multiple regulators.

- Support for turbo modes.

- Other per OPP settings: transition latencies, disabled status, etc.?

- Expandability of OPPs in future.

This patch introduces new bindings "operating-points-v2" to get these problems
solved. Refer to the bindings for more details.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/devicetree/bindings/power/opp.txt | 379 +++++++++++++++++++++++-
 1 file changed, 375 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
index 74499e5033fc..d132e2927b21 100644
--- a/Documentation/devicetree/bindings/power/opp.txt
+++ b/Documentation/devicetree/bindings/power/opp.txt
@@ -1,8 +1,19 @@
-* Generic OPP Interface
+Generic OPP (Operating Performance Points) Bindings
+----------------------------------------------------
 
-SoCs have a standard set of tuples consisting of frequency and
-voltage pairs that the device will support per voltage domain. These
-are called Operating Performance Points or OPPs.
+Devices work at voltage-current-frequency combinations and some implementations
+have the liberty of choosing these. These combinations are called Operating
+Performance Points aka OPPs. This document defines bindings for these OPPs
+applicable across wide range of devices. For illustration purpose, this document
+uses CPU as a device.
+
+This document contain multiple versions of OPP binding and only one of them
+should be used per device.
+
+Binding 1: operating-points
+============================
+
+This binding only supports voltage-frequency pairs.
 
 Properties:
 - operating-points: An array of 2-tuples items, and each item consists
@@ -23,3 +34,363 @@ cpu@0 {
 		198000  850000
 	>;
 };
+
+
+
+Binding 2: operating-points-v2
+============================
+
+* Property: operating-points-v2
+
+Devices supporting OPPs must set their "operating-points-v2" property with
+phandle to a OPP descriptor in their DT node. The OPP core will use this phandle
+to find the operating points for the device.
+
+
+* OPP Descriptor Node
+
+This describes the OPPs belonging to a device. This node can have following
+properties:
+
+Required properties:
+- compatible: Allow OPPs to express their compatibility. It should be:
+  "operating-points-v2".
+
+- OPP nodes: One or more OPP nodes describing voltage-current-frequency
+  combinations. Their name isn't significant but their phandle can be used to
+  reference an OPP.
+
+Optional properties:
+- opp-shared: Indicates that device nodes using this OPP descriptor's phandle
+  switch their DVFS state together, i.e. they share clock/voltage/current lines.
+  Missing property means devices have independent clock/voltage/current lines,
+  but they share OPP tables.
+
+
+* OPP Node
+
+This defines voltage-current-frequency combinations along with other related
+properties.
+
+Required properties:
+- opp-hz: Frequency in Hz
+
+Optional properties:
+- opp-microvolt: voltage in micro Volts.
+
+  A single regulator's voltage is specified with an array of size one or three.
+  Single entry is for target voltage and three entries are for <target min max>
+  voltages.
+
+  Entries for multiple regulators must be present in the same order as
+  regulators are specified in device's DT node.
+
+- opp-microamp: The maximum current drawn by the device in microamperes
+  considering system specific parameters (such as transients, process, aging,
+  maximum operating temperature range etc.) as necessary. This may be used to
+  set the most efficient regulator operating mode.
+
+  Should only be set if opp-microvolt is set for the OPP.
+
+  Entries for multiple regulators must be present in the same order as
+  regulators are specified in device's DT node. If this property isn't required
+  for few regulators, then this should be marked as zero for them. If it isn't
+  required for any regulator, then this property need not be present.
+
+- clock-latency-ns: Specifies the maximum possible transition latency (in
+  nanoseconds) for switching to this OPP from any other OPP.
+
+- turbo-mode: Marks the OPP to be used only for turbo modes. Turbo mode is
+  available on some platforms, where the device can run over its operating
+  frequency for a short duration of time limited by the device's power, current
+  and thermal limits.
+
+- status: Marks the node enabled/disabled.
+
+Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.
+
+/ {
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			compatible = "arm,cortex-a9";
+			reg = <0>;
+			next-level-cache = <&L2>;
+			clocks = <&clk_controller 0>;
+			clock-names = "cpu";
+			opp-supply = <&cpu_supply0>;
+			operating-points-v2 = <&cpu0_opp>;
+		};
+
+		cpu@1 {
+			compatible = "arm,cortex-a9";
+			reg = <1>;
+			next-level-cache = <&L2>;
+			clocks = <&clk_controller 0>;
+			clock-names = "cpu";
+			opp-supply = <&cpu_supply0>;
+			operating-points-v2 = <&cpu0_opp>;
+		};
+	};
+
+	cpu0_opp: opp0 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		entry00 {
+			opp-hz = <1000000000>;
+			opp-microvolt = <970000 975000 985000>;
+			opp-microamp = <70000>;
+			clock-latency-ns = <300000>;
+		};
+		entry01 {
+			opp-hz = <1100000000>;
+			opp-microvolt = <980000 1000000 1010000>;
+			opp-microamp = <80000>;
+			clock-latency-ns = <310000>;
+		};
+		entry02 {
+			opp-hz = <1200000000>;
+			opp-microvolt = <1025000>;
+			clock-latency-ns = <290000>;
+			turbo-mode;
+		};
+	};
+};
+
+Example 2: Single cluster, Quad-core Qualcom-krait, switches DVFS states
+independently.
+
+/ {
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			compatible = "qcom,krait";
+			reg = <0>;
+			next-level-cache = <&L2>;
+			clocks = <&clk_controller 0>;
+			clock-names = "cpu";
+			opp-supply = <&cpu_supply0>;
+			operating-points-v2 = <&cpu0_opp>;
+		};
+
+		cpu@1 {
+			compatible = "qcom,krait";
+			reg = <1>;
+			next-level-cache = <&L2>;
+			clocks = <&clk_controller 1>;
+			clock-names = "cpu";
+			opp-supply = <&cpu_supply1>;
+			operating-points-v2 = <&cpu0_opp>;
+		};
+
+		cpu@2 {
+			compatible = "qcom,krait";
+			reg = <2>;
+			next-level-cache = <&L2>;
+			clocks = <&clk_controller 2>;
+			clock-names = "cpu";
+			opp-supply = <&cpu_supply2>;
+			operating-points-v2 = <&cpu0_opp>;
+		};
+
+		cpu@3 {
+			compatible = "qcom,krait";
+			reg = <3>;
+			next-level-cache = <&L2>;
+			clocks = <&clk_controller 3>;
+			clock-names = "cpu";
+			opp-supply = <&cpu_supply3>;
+			operating-points-v2 = <&cpu0_opp>;
+		};
+	};
+
+	cpu0_opp: opp0 {
+		compatible = "operating-points-v2";
+
+		/*
+		 * Missing opp-shared property means CPUs switch DVFS states
+		 * independently.
+		 */
+
+		entry00 {
+			opp-hz = <1000000000>;
+			opp-microvolt = <970000 975000 985000>;
+			opp-microamp = <70000>;
+			clock-latency-ns = <300000>;
+		};
+		entry01 {
+			opp-hz = <1100000000>;
+			opp-microvolt = <980000 1000000 1010000>;
+			opp-microamp = <80000>;
+			clock-latency-ns = <310000>;
+		};
+		entry02 {
+			opp-hz = <1200000000>;
+			opp-microvolt = <1025000>;
+			opp-microamp = <90000;
+			lock-latency-ns = <290000>;
+			turbo-mode;
+		};
+	};
+};
+
+Example 3: Dual-cluster, Dual-core per cluster. CPUs within a cluster switch
+DVFS state together.
+
+/ {
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			compatible = "arm,cortex-a7";
+			reg = <0>;
+			next-level-cache = <&L2>;
+			clocks = <&clk_controller 0>;
+			clock-names = "cpu";
+			opp-supply = <&cpu_supply0>;
+			operating-points-v2 = <&cluster0_opp>;
+		};
+
+		cpu@1 {
+			compatible = "arm,cortex-a7";
+			reg = <1>;
+			next-level-cache = <&L2>;
+			clocks = <&clk_controller 0>;
+			clock-names = "cpu";
+			opp-supply = <&cpu_supply0>;
+			operating-points-v2 = <&cluster0_opp>;
+		};
+
+		cpu@100 {
+			compatible = "arm,cortex-a15";
+			reg = <100>;
+			next-level-cache = <&L2>;
+			clocks = <&clk_controller 1>;
+			clock-names = "cpu";
+			opp-supply = <&cpu_supply1>;
+			operating-points-v2 = <&cluster1_opp>;
+		};
+
+		cpu@101 {
+			compatible = "arm,cortex-a15";
+			reg = <101>;
+			next-level-cache = <&L2>;
+			clocks = <&clk_controller 1>;
+			clock-names = "cpu";
+			opp-supply = <&cpu_supply1>;
+			operating-points-v2 = <&cluster1_opp>;
+		};
+	};
+
+	cluster0_opp: opp0 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		entry00 {
+			opp-hz = <1000000000>;
+			opp-microvolt = <970000 975000 985000>;
+			opp-microamp = <70000>;
+			clock-latency-ns = <300000>;
+		};
+		entry01 {
+			opp-hz = <1100000000>;
+			opp-microvolt = <980000 1000000 1010000>;
+			opp-microamp = <80000>;
+			clock-latency-ns = <310000>;
+		};
+		entry02 {
+			opp-hz = <1200000000>;
+			opp-microvolt = <1025000>;
+			opp-microamp = <90000>;
+			clock-latency-ns = <290000>;
+			turbo-mode;
+		};
+	};
+
+	cluster1_opp: opp1 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		entry10 {
+			opp-hz = <1300000000>;
+			opp-microvolt = <1045000 1050000 1055000>;
+			opp-microamp = <95000>;
+			clock-latency-ns = <400000>;
+		};
+		entry11 {
+			opp-hz = <1400000000>;
+			opp-microvolt = <1075000>;
+			opp-microamp = <100000>;
+			clock-latency-ns = <400000>;
+		};
+		entry12 {
+			opp-hz = <1500000000>;
+			opp-microvolt = <1010000 1100000 1110000>;
+			opp-microamp = <95000>;
+			clock-latency-ns = <400000>;
+			turbo-mode;
+		};
+	};
+};
+
+Example 4: Handling multiple regulators
+
+/ {
+	cpus {
+		cpu@0 {
+			compatible = "arm,cortex-a7";
+			...
+
+			opp-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
+			operating-points-v2 = <&cpu0_opp>;
+		};
+	};
+
+	cpu0_opp: opp0 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		entry00 {
+			opp-hz = <1000000000>;
+			opp-microvolt = <970000>, /* Supply 0 */
+					<960000>, /* Supply 1 */
+					<960000>; /* Supply 2 */
+			opp-microamp =  <70000>,  /* Supply 0 */
+					<70000>,  /* Supply 1 */
+					<70000>;  /* Supply 2 */
+			clock-latency-ns = <300000>;
+		};
+
+		/* OR */
+
+		entry00 {
+			opp-hz = <1000000000>;
+			opp-microvolt = <970000 975000 985000>, /* Supply 0 */
+					<960000 965000 975000>, /* Supply 1 */
+					<960000 965000 975000>; /* Supply 2 */
+			opp-microamp =  <70000>,		/* Supply 0 */
+					<70000>,		/* Supply 1 */
+					<70000>;		/* Supply 2 */
+			clock-latency-ns = <300000>;
+		};
+
+		/* OR */
+
+		entry00 {
+			opp-hz = <1000000000>;
+			opp-microvolt = <970000 975000 985000>, /* Supply 0 */
+					<960000 965000 975000>, /* Supply 1 */
+					<960000 965000 975000>; /* Supply 2 */
+			opp-microamp =  <70000>,		/* Supply 0 */
+					<0>,			/* Supply 1 doesn't need this */
+					<70000>;		/* Supply 2 */
+			clock-latency-ns = <300000>;
+		};
+	};
+};
-- 
2.4.0

[PATCH V5 2/3] OPP: Allow multiple OPP tables to be passed via DT

[Viresh Kumar]

On some platforms (Like Qualcomm's SoCs), it is not decided until
runtime on what OPPs to use. The OPP tables can be fixed at compile
time, but which table to use is found out only after reading some efuses
(sort of an prom) and knowing characteristics of the SoC.

To support such platform we need to pass multiple OPP tables per device
and hardware should be able to choose one and only one table out of
those.

Update OPP-v2 bindings to support that.

Acked-by: Nishanth Menon <nm@ti.com>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/devicetree/bindings/power/opp.txt | 52 +++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
index d132e2927b21..7f30d9b07db7 100644
--- a/Documentation/devicetree/bindings/power/opp.txt
+++ b/Documentation/devicetree/bindings/power/opp.txt
@@ -46,6 +46,9 @@ Devices supporting OPPs must set their "operating-points-v2" property with
 phandle to a OPP descriptor in their DT node. The OPP core will use this phandle
 to find the operating points for the device.
 
+Devices may want to choose OPP tables at runtime and so can provide a list of
+phandles here. But only *one* of them should be chosen at runtime.
+
 
 * OPP Descriptor Node
 
@@ -61,6 +64,9 @@ This describes the OPPs belonging to a device. This node can have following
   reference an OPP.
 
 Optional properties:
+- opp-name: Name of the OPP table, to uniquely identify it if more than one OPP
+  table is supplied in "operating-points-v2" property of device.
+
 - opp-shared: Indicates that device nodes using this OPP descriptor's phandle
   switch their DVFS state together, i.e. they share clock/voltage/current lines.
   Missing property means devices have independent clock/voltage/current lines,
@@ -394,3 +400,49 @@ Example 4: Handling multiple regulators
 		};
 	};
 };
+
+Example 5: Multiple OPP tables
+
+/ {
+	cpus {
+		cpu@0 {
+			compatible = "arm,cortex-a7";
+			...
+
+			opp-supply = <&cpu_supply>
+			operating-points-v2 = <&cpu0_opp0>, <&cpu0_opp1>;
+		};
+	};
+
+	cpu0_opp0: opp0 {
+		compatible = "operating-points-v2";
+		opp-name = "opp-slow";
+		opp-shared;
+
+		entry00 {
+			opp-hz = <600000000>;
+			...
+		};
+
+		entry01 {
+			opp-hz = <800000000>;
+			...
+		};
+	};
+
+	cpu0_opp1: opp1 {
+		compatible = "operating-points-v2";
+		opp-name = "opp-fast";
+		opp-shared;
+
+		entry10 {
+			opp-hz = <1000000000>;
+			...
+		};
+
+		entry11 {
+			opp-hz = <1100000000>;
+			...
+		};
+	};
+};
-- 
2.4.0

[PATCH V5 3/3] OPP: Add binding for 'opp-suspend'

[Viresh Kumar]

On few platforms, for power efficiency, we want the device to be
configured for a specific OPP while we put the device in suspend state.

Add an optional property in operating-points-v2 bindings for that.

Suggested-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/devicetree/bindings/power/opp.txt | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
index 7f30d9b07db7..880f5e14b6e2 100644
--- a/Documentation/devicetree/bindings/power/opp.txt
+++ b/Documentation/devicetree/bindings/power/opp.txt
@@ -72,6 +72,8 @@ This describes the OPPs belonging to a device. This node can have following
   Missing property means devices have independent clock/voltage/current lines,
   but they share OPP tables.
 
+- opp-suspend: Phandle of the OPP to set while device is suspended.
+
 
 * OPP Node
 
@@ -143,9 +145,10 @@ Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.
 
 	cpu0_opp: opp0 {
 		compatible = "operating-points-v2";
+		opp-suspend = <&suspend_opp>;
 		opp-shared;
 
-		entry00 {
+		suspend-opp: entry00 {
 			opp-hz = <1000000000>;
 			opp-microvolt = <970000 975000 985000>;
 			opp-microamp = <70000>;
@@ -217,13 +220,14 @@ independently.
 
 	cpu0_opp: opp0 {
 		compatible = "operating-points-v2";
+		opp-suspend = <&suspend_opp>;
 
 		/*
 		 * Missing opp-shared property means CPUs switch DVFS states
 		 * independently.
 		 */
 
-		entry00 {
+		suspend-opp: entry00 {
 			opp-hz = <1000000000>;
 			opp-microvolt = <970000 975000 985000>;
 			opp-microamp = <70000>;
@@ -296,9 +300,10 @@ DVFS state together.
 
 	cluster0_opp: opp0 {
 		compatible = "operating-points-v2";
+		opp-suspend = <&suspend_opp0>;
 		opp-shared;
 
-		entry00 {
+		suspend-opp0: entry00 {
 			opp-hz = <1000000000>;
 			opp-microvolt = <970000 975000 985000>;
 			opp-microamp = <70000>;
@@ -321,9 +326,10 @@ DVFS state together.
 
 	cluster1_opp: opp1 {
 		compatible = "operating-points-v2";
+		opp-suspend = <&suspend_opp1>;
 		opp-shared;
 
-		entry10 {
+		suspend-opp1: entry10 {
 			opp-hz = <1300000000>;
 			opp-microvolt = <1045000 1050000 1055000>;
 			opp-microamp = <95000>;
-- 
2.4.0

Re: [PATCH V5 1/3] OPP: Redefine bindings to overcome shortcomings

[Rob Herring]

On Tue, May 19, 2015 at 10:41 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Current OPP (Operating performance point) DT bindings are proven to be
> insufficient at multiple instances.
>
> The shortcomings we are trying to solve here:
>
> - Getting clock/voltage/current rails sharing information between CPUs.
>   Shared by all cores vs independent clock per core vs shared clock per
>   cluster.
>
> - Support for specifying current levels along with voltages.
>
> - Support for multiple regulators.
>
> - Support for turbo modes.
>
> - Other per OPP settings: transition latencies, disabled status, etc.?
>
> - Expandability of OPPs in future.
>
> This patch introduces new bindings "operating-points-v2" to get these problems
> solved. Refer to the bindings for more details.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

This looks good to me:

Reviewed-by: Rob Herring <robh@kernel.org>

As I've mentioned before, I would like to see some users' acks on this as well.

Rob

> ---
>  Documentation/devicetree/bindings/power/opp.txt | 379 +++++++++++++++++++++++-
>  1 file changed, 375 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
> index 74499e5033fc..d132e2927b21 100644
> --- a/Documentation/devicetree/bindings/power/opp.txt
> +++ b/Documentation/devicetree/bindings/power/opp.txt
> @@ -1,8 +1,19 @@
> -* Generic OPP Interface
> +Generic OPP (Operating Performance Points) Bindings
> +----------------------------------------------------
>
> -SoCs have a standard set of tuples consisting of frequency and
> -voltage pairs that the device will support per voltage domain. These
> -are called Operating Performance Points or OPPs.
> +Devices work at voltage-current-frequency combinations and some implementations
> +have the liberty of choosing these. These combinations are called Operating
> +Performance Points aka OPPs. This document defines bindings for these OPPs
> +applicable across wide range of devices. For illustration purpose, this document
> +uses CPU as a device.
> +
> +This document contain multiple versions of OPP binding and only one of them
> +should be used per device.
> +
> +Binding 1: operating-points
> +============================
> +
> +This binding only supports voltage-frequency pairs.
>
>  Properties:
>  - operating-points: An array of 2-tuples items, and each item consists
> @@ -23,3 +34,363 @@ cpu@0 {
>                 198000  850000
>         >;
>  };
> +
> +
> +
> +Binding 2: operating-points-v2
> +============================
> +
> +* Property: operating-points-v2
> +
> +Devices supporting OPPs must set their "operating-points-v2" property with
> +phandle to a OPP descriptor in their DT node. The OPP core will use this phandle
> +to find the operating points for the device.
> +
> +
> +* OPP Descriptor Node
> +
> +This describes the OPPs belonging to a device. This node can have following
> +properties:
> +
> +Required properties:
> +- compatible: Allow OPPs to express their compatibility. It should be:
> +  "operating-points-v2".
> +
> +- OPP nodes: One or more OPP nodes describing voltage-current-frequency
> +  combinations. Their name isn't significant but their phandle can be used to
> +  reference an OPP.
> +
> +Optional properties:
> +- opp-shared: Indicates that device nodes using this OPP descriptor's phandle
> +  switch their DVFS state together, i.e. they share clock/voltage/current lines.
> +  Missing property means devices have independent clock/voltage/current lines,
> +  but they share OPP tables.
> +
> +
> +* OPP Node
> +
> +This defines voltage-current-frequency combinations along with other related
> +properties.
> +
> +Required properties:
> +- opp-hz: Frequency in Hz
> +
> +Optional properties:
> +- opp-microvolt: voltage in micro Volts.
> +
> +  A single regulator's voltage is specified with an array of size one or three.
> +  Single entry is for target voltage and three entries are for <target min max>
> +  voltages.
> +
> +  Entries for multiple regulators must be present in the same order as
> +  regulators are specified in device's DT node.
> +
> +- opp-microamp: The maximum current drawn by the device in microamperes
> +  considering system specific parameters (such as transients, process, aging,
> +  maximum operating temperature range etc.) as necessary. This may be used to
> +  set the most efficient regulator operating mode.
> +
> +  Should only be set if opp-microvolt is set for the OPP.
> +
> +  Entries for multiple regulators must be present in the same order as
> +  regulators are specified in device's DT node. If this property isn't required
> +  for few regulators, then this should be marked as zero for them. If it isn't
> +  required for any regulator, then this property need not be present.
> +
> +- clock-latency-ns: Specifies the maximum possible transition latency (in
> +  nanoseconds) for switching to this OPP from any other OPP.
> +
> +- turbo-mode: Marks the OPP to be used only for turbo modes. Turbo mode is
> +  available on some platforms, where the device can run over its operating
> +  frequency for a short duration of time limited by the device's power, current
> +  and thermal limits.
> +
> +- status: Marks the node enabled/disabled.
> +
> +Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.
> +
> +/ {
> +       cpus {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               cpu@0 {
> +                       compatible = "arm,cortex-a9";
> +                       reg = <0>;
> +                       next-level-cache = <&L2>;
> +                       clocks = <&clk_controller 0>;
> +                       clock-names = "cpu";
> +                       opp-supply = <&cpu_supply0>;
> +                       operating-points-v2 = <&cpu0_opp>;
> +               };
> +
> +               cpu@1 {
> +                       compatible = "arm,cortex-a9";
> +                       reg = <1>;
> +                       next-level-cache = <&L2>;
> +                       clocks = <&clk_controller 0>;
> +                       clock-names = "cpu";
> +                       opp-supply = <&cpu_supply0>;
> +                       operating-points-v2 = <&cpu0_opp>;
> +               };
> +       };
> +
> +       cpu0_opp: opp0 {
> +               compatible = "operating-points-v2";
> +               opp-shared;
> +
> +               entry00 {
> +                       opp-hz = <1000000000>;
> +                       opp-microvolt = <970000 975000 985000>;
> +                       opp-microamp = <70000>;
> +                       clock-latency-ns = <300000>;
> +               };
> +               entry01 {
> +                       opp-hz = <1100000000>;
> +                       opp-microvolt = <980000 1000000 1010000>;
> +                       opp-microamp = <80000>;
> +                       clock-latency-ns = <310000>;
> +               };
> +               entry02 {
> +                       opp-hz = <1200000000>;
> +                       opp-microvolt = <1025000>;
> +                       clock-latency-ns = <290000>;
> +                       turbo-mode;
> +               };
> +       };
> +};
> +
> +Example 2: Single cluster, Quad-core Qualcom-krait, switches DVFS states
> +independently.
> +
> +/ {
> +       cpus {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               cpu@0 {
> +                       compatible = "qcom,krait";
> +                       reg = <0>;
> +                       next-level-cache = <&L2>;
> +                       clocks = <&clk_controller 0>;
> +                       clock-names = "cpu";
> +                       opp-supply = <&cpu_supply0>;
> +                       operating-points-v2 = <&cpu0_opp>;
> +               };
> +
> +               cpu@1 {
> +                       compatible = "qcom,krait";
> +                       reg = <1>;
> +                       next-level-cache = <&L2>;
> +                       clocks = <&clk_controller 1>;
> +                       clock-names = "cpu";
> +                       opp-supply = <&cpu_supply1>;
> +                       operating-points-v2 = <&cpu0_opp>;
> +               };
> +
> +               cpu@2 {
> +                       compatible = "qcom,krait";
> +                       reg = <2>;
> +                       next-level-cache = <&L2>;
> +                       clocks = <&clk_controller 2>;
> +                       clock-names = "cpu";
> +                       opp-supply = <&cpu_supply2>;
> +                       operating-points-v2 = <&cpu0_opp>;
> +               };
> +
> +               cpu@3 {
> +                       compatible = "qcom,krait";
> +                       reg = <3>;
> +                       next-level-cache = <&L2>;
> +                       clocks = <&clk_controller 3>;
> +                       clock-names = "cpu";
> +                       opp-supply = <&cpu_supply3>;
> +                       operating-points-v2 = <&cpu0_opp>;
> +               };
> +       };
> +
> +       cpu0_opp: opp0 {
> +               compatible = "operating-points-v2";
> +
> +               /*
> +                * Missing opp-shared property means CPUs switch DVFS states
> +                * independently.
> +                */
> +
> +               entry00 {
> +                       opp-hz = <1000000000>;
> +                       opp-microvolt = <970000 975000 985000>;
> +                       opp-microamp = <70000>;
> +                       clock-latency-ns = <300000>;
> +               };
> +               entry01 {
> +                       opp-hz = <1100000000>;
> +                       opp-microvolt = <980000 1000000 1010000>;
> +                       opp-microamp = <80000>;
> +                       clock-latency-ns = <310000>;
> +               };
> +               entry02 {
> +                       opp-hz = <1200000000>;
> +                       opp-microvolt = <1025000>;
> +                       opp-microamp = <90000;
> +                       lock-latency-ns = <290000>;
> +                       turbo-mode;
> +               };
> +       };
> +};
> +
> +Example 3: Dual-cluster, Dual-core per cluster. CPUs within a cluster switch
> +DVFS state together.
> +
> +/ {
> +       cpus {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               cpu@0 {
> +                       compatible = "arm,cortex-a7";
> +                       reg = <0>;
> +                       next-level-cache = <&L2>;
> +                       clocks = <&clk_controller 0>;
> +                       clock-names = "cpu";
> +                       opp-supply = <&cpu_supply0>;
> +                       operating-points-v2 = <&cluster0_opp>;
> +               };
> +
> +               cpu@1 {
> +                       compatible = "arm,cortex-a7";
> +                       reg = <1>;
> +                       next-level-cache = <&L2>;
> +                       clocks = <&clk_controller 0>;
> +                       clock-names = "cpu";
> +                       opp-supply = <&cpu_supply0>;
> +                       operating-points-v2 = <&cluster0_opp>;
> +               };
> +
> +               cpu@100 {
> +                       compatible = "arm,cortex-a15";
> +                       reg = <100>;
> +                       next-level-cache = <&L2>;
> +                       clocks = <&clk_controller 1>;
> +                       clock-names = "cpu";
> +                       opp-supply = <&cpu_supply1>;
> +                       operating-points-v2 = <&cluster1_opp>;
> +               };
> +
> +               cpu@101 {
> +                       compatible = "arm,cortex-a15";
> +                       reg = <101>;
> +                       next-level-cache = <&L2>;
> +                       clocks = <&clk_controller 1>;
> +                       clock-names = "cpu";
> +                       opp-supply = <&cpu_supply1>;
> +                       operating-points-v2 = <&cluster1_opp>;
> +               };
> +       };
> +
> +       cluster0_opp: opp0 {
> +               compatible = "operating-points-v2";
> +               opp-shared;
> +
> +               entry00 {
> +                       opp-hz = <1000000000>;
> +                       opp-microvolt = <970000 975000 985000>;
> +                       opp-microamp = <70000>;
> +                       clock-latency-ns = <300000>;
> +               };
> +               entry01 {
> +                       opp-hz = <1100000000>;
> +                       opp-microvolt = <980000 1000000 1010000>;
> +                       opp-microamp = <80000>;
> +                       clock-latency-ns = <310000>;
> +               };
> +               entry02 {
> +                       opp-hz = <1200000000>;
> +                       opp-microvolt = <1025000>;
> +                       opp-microamp = <90000>;
> +                       clock-latency-ns = <290000>;
> +                       turbo-mode;
> +               };
> +       };
> +
> +       cluster1_opp: opp1 {
> +               compatible = "operating-points-v2";
> +               opp-shared;
> +
> +               entry10 {
> +                       opp-hz = <1300000000>;
> +                       opp-microvolt = <1045000 1050000 1055000>;
> +                       opp-microamp = <95000>;
> +                       clock-latency-ns = <400000>;
> +               };
> +               entry11 {
> +                       opp-hz = <1400000000>;
> +                       opp-microvolt = <1075000>;
> +                       opp-microamp = <100000>;
> +                       clock-latency-ns = <400000>;
> +               };
> +               entry12 {
> +                       opp-hz = <1500000000>;
> +                       opp-microvolt = <1010000 1100000 1110000>;
> +                       opp-microamp = <95000>;
> +                       clock-latency-ns = <400000>;
> +                       turbo-mode;
> +               };
> +       };
> +};
> +
> +Example 4: Handling multiple regulators
> +
> +/ {
> +       cpus {
> +               cpu@0 {
> +                       compatible = "arm,cortex-a7";
> +                       ...
> +
> +                       opp-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
> +                       operating-points-v2 = <&cpu0_opp>;
> +               };
> +       };
> +
> +       cpu0_opp: opp0 {
> +               compatible = "operating-points-v2";
> +               opp-shared;
> +
> +               entry00 {
> +                       opp-hz = <1000000000>;
> +                       opp-microvolt = <970000>, /* Supply 0 */
> +                                       <960000>, /* Supply 1 */
> +                                       <960000>; /* Supply 2 */
> +                       opp-microamp =  <70000>,  /* Supply 0 */
> +                                       <70000>,  /* Supply 1 */
> +                                       <70000>;  /* Supply 2 */
> +                       clock-latency-ns = <300000>;
> +               };
> +
> +               /* OR */
> +
> +               entry00 {
> +                       opp-hz = <1000000000>;
> +                       opp-microvolt = <970000 975000 985000>, /* Supply 0 */
> +                                       <960000 965000 975000>, /* Supply 1 */
> +                                       <960000 965000 975000>; /* Supply 2 */
> +                       opp-microamp =  <70000>,                /* Supply 0 */
> +                                       <70000>,                /* Supply 1 */
> +                                       <70000>;                /* Supply 2 */
> +                       clock-latency-ns = <300000>;
> +               };
> +
> +               /* OR */
> +
> +               entry00 {
> +                       opp-hz = <1000000000>;
> +                       opp-microvolt = <970000 975000 985000>, /* Supply 0 */
> +                                       <960000 965000 975000>, /* Supply 1 */
> +                                       <960000 965000 975000>; /* Supply 2 */
> +                       opp-microamp =  <70000>,                /* Supply 0 */
> +                                       <0>,                    /* Supply 1 doesn't need this */
> +                                       <70000>;                /* Supply 2 */
> +                       clock-latency-ns = <300000>;
> +               };
> +       };
> +};
> --
> 2.4.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 1/3] OPP: Redefine bindings to overcome shortcomings

[Viresh Kumar]

On 20-05-15, 08:27, Rob Herring wrote:
> This looks good to me:
> 
> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Thanks a lot !!

> As I've mentioned before, I would like to see some users' acks on this as well.

Sure. Stephen and Nishanth have already approved 2/3 and I hope they
will Ack other two as well today.

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

Re: [PATCH V5 1/3] OPP: Redefine bindings to overcome shortcomings

[Nishanth Menon]

$subject: (Rafael usually hand fixes it.. but might be nice of us not to
save him that effort) PM / OPP: Additional binding definition to address ...

we are not really redefining the old definitions..

On 05/19/2015 10:41 PM, Viresh Kumar wrote:
> Current OPP (Operating performance point) DT bindings are proven to be
Nit Pick: s/DT/device tree ; s/are/have been

> insufficient at multiple instances.

insufficient due to the inflexible nature of the original bindings. Over
time, we have realized that Operating Performance Point definitions and
usage is varied depending on the SoC and a "single size (just frequency,
voltage) fits all" model which the original bindings attempted and failed.

> 
> The shortcomings we are trying to solve here:

The proposed next generation of the bindings addresses by providing a
expandable binding for OPPs and introduces the following common
shortcomings seen with the original bindings:

[...]

> ---
>  Documentation/devicetree/bindings/power/opp.txt | 379 +++++++++++++++++++++++-
>  1 file changed, 375 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
> index 74499e5033fc..d132e2927b21 100644
> --- a/Documentation/devicetree/bindings/power/opp.txt
> +++ b/Documentation/devicetree/bindings/power/opp.txt
> @@ -1,8 +1,19 @@
> -* Generic OPP Interface
> +Generic OPP (Operating Performance Points) Bindings
> +----------------------------------------------------
>  
> -SoCs have a standard set of tuples consisting of frequency and
> -voltage pairs that the device will support per voltage domain. These
> -are called Operating Performance Points or OPPs.
> +Devices work at voltage-current-frequency combinations and some implementations
> +have the liberty of choosing these. These combinations are called Operating
> +Performance Points aka OPPs. This document defines bindings for these OPPs
> +applicable across wide range of devices. For illustration purpose, this document
> +uses CPU as a device.
> +
> +This document contain multiple versions of OPP binding and only one of them
> +should be used per device.

Will be nice to repeat this message in the commit message as well..
folks just doing git log should probably not freak out that the current
dtbs will stop working once the implementation is merged in - it might
help deal with some of the concerns of folks not aware of the mailing
thread discussions.

> +
> +Binding 1: operating-points
> +============================
> +
> +This binding only supports voltage-frequency pairs.
>  
>  Properties:
>  - operating-points: An array of 2-tuples items, and each item consists
> @@ -23,3 +34,363 @@ cpu@0 {
>  		198000  850000
>  	>;
>  };
> +
> +

Extra EOLs? maybe just stop at 2 EOLs to separate the sections.?

> +
> +Binding 2: operating-points-v2
> +============================
> +
> +* Property: operating-points-v2
> +
> +Devices supporting OPPs must set their "operating-points-v2" property with
> +phandle to a OPP descriptor in their DT node. The OPP core will use this phandle
> +to find the operating points for the device.
> +

I would suggest adding a link to how future vendor specific extension
docs might look like - maybe this is probably not the time to discuss
this.. but anyways.. we could make some statement to the effect of "SoC
vendor specfic extensions are documented as
Documentation/devicetree/bindings/power/<vendor>,opp.txt and should
clearly indicate that the extensions are permitted only under the
operating-points-v2 compatible description."


> +
> +* OPP Descriptor Node
> +
> +This describes the OPPs belonging to a device. This node can have following
> +properties:
> +
> +Required properties:
> +- compatible: Allow OPPs to express their compatibility. It should be:
> +  "operating-points-v2".
> +
> +- OPP nodes: One or more OPP nodes describing voltage-current-frequency
> +  combinations. Their name isn't significant but their phandle can be used to
> +  reference an OPP.

What if this was generated data (say using an overlay)? does it have to
be "required" or just "optional" :)

> +
> +Optional properties:
> +- opp-shared: Indicates that device nodes using this OPP descriptor's phandle
> +  switch their DVFS state together, i.e. they share clock/voltage/current lines.
> +  Missing property means devices have independent clock/voltage/current lines,
> +  but they share OPP tables.
> +
> +
> +* OPP Node
> +
> +This defines voltage-current-frequency combinations along with other related
> +properties.
> +
> +Required properties:
> +- opp-hz: Frequency in Hz

I am just being nit picky -> should we keep Heinrich Hertz respected[2]
and name it opp-Hz ? No strong opinions either way.

different angle: How about just target-freq-Hz? just drop the "opp"
prefix for properties of an OPP node? No strong feelings here. (some
folks did have variations of a few Hz based on clock tree - example with
a crystal frequency of 19.2MHz you'd probably get 1001MHz exact, with a
26MHz crystal, you'd get 1000MHz -> ofcourse round-rate is supposed to
help with that... but anyways.. why not say we are trying to indicate
target frequency? I do recollect during initial days of OPP
(pre-dt-adoption days) folks did complain about this - we kinda worked
around this with round-rated handling - but we might as well anticipate it.

[...]

> +Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.

Thanks for adding the examples - My customer support team especially
will appreciate having such examples ;).

> +
> +/ {
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu@0 {
> +			compatible = "arm,cortex-a9";
> +			reg = <0>;
> +			next-level-cache = <&L2>;
> +			clocks = <&clk_controller 0>;
> +			clock-names = "cpu";
> +			opp-supply = <&cpu_supply0>;
> +			operating-points-v2 = <&cpu0_opp>;
> +		};
> +
> +		cpu@1 {
> +			compatible = "arm,cortex-a9";
> +			reg = <1>;
> +			next-level-cache = <&L2>;
> +			clocks = <&clk_controller 0>;

I agree with Mike[1] here -> why not move clocks and supply to cpu0_opp?
"
It seems wrong to me that the clock and supply data is owned by the cpu
node, and not the opp descriptor. Everything about the opp transition
should belong to a provider node. Then the cpu simply needs to consume
that via a phandle.
"

I am not sure if we discussed that point further OR if we kinda got
hooked on to the "should it be in kernel" point of debate in V4.

> +			clock-names = "cpu";
> +			opp-supply = <&cpu_supply0>;
> +			operating-points-v2 = <&cpu0_opp>;
> +		};
> +	};
> +

[...]

[1] http://marc.info/?l=linux-pm&m=143146696029140&w=2
[2] http://marc.info/?l=linux-kernel&m=143135047525313&w=2
-- 
Regards,
Nishanth Menon

Re: [PATCH V5 3/3] OPP: Add binding for 'opp-suspend'

[Nishanth Menon]

On 05/19/2015 10:41 PM, Viresh Kumar wrote:
> On few platforms, for power efficiency, we want the device to be
> configured for a specific OPP while we put the device in suspend state.
> 
> Add an optional property in operating-points-v2 bindings for that.

Why not just mark it as a property of an OPP rather than a phandle? just
thinking that this might setup a precedence for future needs like
"shutdown OPP" or "Reboot OPP" or something different that some other
SoC might have..

> 
> Suggested-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  Documentation/devicetree/bindings/power/opp.txt | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
> index 7f30d9b07db7..880f5e14b6e2 100644
> --- a/Documentation/devicetree/bindings/power/opp.txt
> +++ b/Documentation/devicetree/bindings/power/opp.txt
> @@ -72,6 +72,8 @@ This describes the OPPs belonging to a device. This node can have following
>    Missing property means devices have independent clock/voltage/current lines,
>    but they share OPP tables.
>  
> +- opp-suspend: Phandle of the OPP to set while device is suspended.
> +
>  
>  * OPP Node
>  
> @@ -143,9 +145,10 @@ Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.
>  
>  	cpu0_opp: opp0 {
>  		compatible = "operating-points-v2";
> +		opp-suspend = <&suspend_opp>;
>  		opp-shared;
>  
> -		entry00 {
> +		suspend-opp: entry00 {
>  			opp-hz = <1000000000>;
>  			opp-microvolt = <970000 975000 985000>;
>  			opp-microamp = <70000>;
> @@ -217,13 +220,14 @@ independently.
>  
>  	cpu0_opp: opp0 {
>  		compatible = "operating-points-v2";
> +		opp-suspend = <&suspend_opp>;
>  
>  		/*
>  		 * Missing opp-shared property means CPUs switch DVFS states
>  		 * independently.
>  		 */
>  
> -		entry00 {
> +		suspend-opp: entry00 {
>  			opp-hz = <1000000000>;
>  			opp-microvolt = <970000 975000 985000>;
>  			opp-microamp = <70000>;
> @@ -296,9 +300,10 @@ DVFS state together.
>  
>  	cluster0_opp: opp0 {
>  		compatible = "operating-points-v2";
> +		opp-suspend = <&suspend_opp0>;
>  		opp-shared;
>  
> -		entry00 {
> +		suspend-opp0: entry00 {
>  			opp-hz = <1000000000>;
>  			opp-microvolt = <970000 975000 985000>;
>  			opp-microamp = <70000>;
> @@ -321,9 +326,10 @@ DVFS state together.
>  
>  	cluster1_opp: opp1 {
>  		compatible = "operating-points-v2";
> +		opp-suspend = <&suspend_opp1>;
>  		opp-shared;
>  
> -		entry10 {
> +		suspend-opp1: entry10 {
>  			opp-hz = <1300000000>;
>  			opp-microvolt = <1045000 1050000 1055000>;
>  			opp-microamp = <95000>;
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH V5 2/3] OPP: Allow multiple OPP tables to be passed via DT

[Nishanth Menon]

On 05/19/2015 10:41 PM, Viresh Kumar wrote:
Thinking a little more..

> +Example 5: Multiple OPP tables
> +
> +/ {
> +	cpus {
> +		cpu@0 {
> +			compatible = "arm,cortex-a7";
> +			...
> +
> +			opp-supply = <&cpu_supply>
> +			operating-points-v2 = <&cpu0_opp0>, <&cpu0_opp1>;
> +		};
> +	};
> +
> +	cpu0_opp0: opp0 {

Maybe we should rename these as cpu0_opp_table_slow opp_table-fast

> +		compatible = "operating-points-v2";
> +		opp-name = "opp-slow";

just name = "slow" ?

> +		opp-shared;
> +
> +		entry00 {

rename these as opp0, opp01 etc?
these are the actual OPP description, while, what we call "cpu0_opp0" is
actually an OPP table choice we have.

> +			opp-hz = <600000000>;
> +			...
> +		};
> +
> +		entry01 {
> +			opp-hz = <800000000>;
> +			...
> +		};
> +	};
> +
> +	cpu0_opp1: opp1 {
> +		compatible = "operating-points-v2";
> +		opp-name = "opp-fast";
> +		opp-shared;
> +
> +		entry10 {
> +			opp-hz = <1000000000>;
> +			...
> +		};
> +
> +		entry11 {
> +			opp-hz = <1100000000>;
> +			...
> +		};
> +	};
> +};
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH V5 1/3] OPP: Redefine bindings to overcome shortcomings

[Viresh Kumar]

On 21-05-15, 00:27, Nishanth Menon wrote:
> > +This describes the OPPs belonging to a device. This node can have following
> > +properties:
> > +
> > +Required properties:
> > +- compatible: Allow OPPs to express their compatibility. It should be:
> > +  "operating-points-v2".
> > +
> > +- OPP nodes: One or more OPP nodes describing voltage-current-frequency
> > +  combinations. Their name isn't significant but their phandle can be used to
> > +  reference an OPP.
> 
> What if this was generated data (say using an overlay)?

Sorry I am not aware of this, can you explain a bit how this is done ?
Are you talking about something like fdtput here ?

> does it have to
> be "required" or just "optional" :)

This has to be required (by the parser, kernel in our case).

> > +Required properties:
> > +- opp-hz: Frequency in Hz
> 
> I am just being nit picky -> should we keep Heinrich Hertz respected[2]
> and name it opp-Hz ? No strong opinions either way.

Documentation/devicetree/booting-without-of.txt:

4) Note about node and property names and character set
-------------------------------------------------------

While Open Firmware provides more flexible usage of 8859-1, this
specification enforces more strict rules. Nodes and properties should
be comprised only of ASCII characters 'a' to 'z', '0' to
'9', ',', '.', '_', '+', '#', '?', and '-'. Node names additionally
allow uppercase characters 'A' to 'Z' (property names should be
lowercase. The fact that vendors like Apple don't respect this rule is
irrelevant here). Additionally, node and property names should always
begin with a character in the range 'a' to 'z' (or 'A' to 'Z' for node
names).

> 
> different angle: How about just target-freq-Hz? just drop the "opp"
> prefix for properties of an OPP node? No strong feelings here. (some
> folks did have variations of a few Hz based on clock tree - example with
> a crystal frequency of 19.2MHz you'd probably get 1001MHz exact, with a
> 26MHz crystal, you'd get 1000MHz -> ofcourse round-rate is supposed to
> help with that... but anyways.. why not say we are trying to indicate
> target frequency? I do recollect during initial days of OPP
> (pre-dt-adoption days) folks did complain about this - we kinda worked
> around this with round-rated handling - but we might as well anticipate it.

Rob suggested opp- prefix and it looks good to me, lets see what
others have to say :)

> Thanks for adding the examples - My customer support team especially
> will appreciate having such examples ;).

I can understand that :)

> I agree with Mike[1] here -> why not move clocks and supply to cpu0_opp?
> "
> It seems wrong to me that the clock and supply data is owned by the cpu
> node, and not the opp descriptor. Everything about the opp transition
> should belong to a provider node. Then the cpu simply needs to consume
> that via a phandle.
> "
> 
> I am not sure if we discussed that point further OR if we kinda got
> hooked on to the "should it be in kernel" point of debate in V4.

I did send a reply to that, but no one replied after that. Probably
you want to reply on that ?

Thanks for your detailed review.

-- 
viresh

Re: [PATCH V5 3/3] OPP: Add binding for 'opp-suspend'

[Viresh Kumar]

On 21-05-15, 00:32, Nishanth Menon wrote:
> Why not just mark it as a property of an OPP rather than a phandle? just
> thinking that this might setup a precedence for future needs like
> "shutdown OPP" or "Reboot OPP" or something different that some other
> SoC might have..

AFAIU, a property should be present in the OPP if and only if any OPP
can set it. But in this case, only one OPP from the entire list can
set it. So, its more a property of the list rather than every OPP
within it. And then there wouldn't be any need to code that would
check bugs in dtbs where multiple OPPs have it set.

-- 
viresh

Re: [PATCH V5 2/3] OPP: Allow multiple OPP tables to be passed via DT

[Viresh Kumar]

On 21-05-15, 00:34, Nishanth Menon wrote:
> On 05/19/2015 10:41 PM, Viresh Kumar wrote:
> Thinking a little more..

No issues. Will update that.

-- 
viresh

Re: [PATCH V5 1/3] OPP: Redefine bindings to overcome shortcomings

[Nishanth Menon]

On 05/21/2015 12:46 AM, Viresh Kumar wrote:
> On 21-05-15, 00:27, Nishanth Menon wrote:
>>> +This describes the OPPs belonging to a device. This node can have following
>>> +properties:
>>> +
>>> +Required properties:
>>> +- compatible: Allow OPPs to express their compatibility. It should be:
>>> +  "operating-points-v2".
>>> +
>>> +- OPP nodes: One or more OPP nodes describing voltage-current-frequency
>>> +  combinations. Their name isn't significant but their phandle can be used to
>>> +  reference an OPP.
>>
>> What if this was generated data (say using an overlay)?
> 
> Sorry I am not aware of this, can you explain a bit how this is done ?
> Are you talking about something like fdtput here ?

Honestly, have'nt thought out all the details in yet :)... just
thinking.. since overlays can come in later in the system, we could boot
up a very bare minimum kernel, have device tree populated runtime (say
based on data incoming from a power management microcontroller based
description of the capabilities.. I know PSCI or some other form will
eventually deal with it.. just wondering about scaling options)... basic
node description made in original dtb, and rest of the data populated
dynamically(by some magic mechanism yet to be defined) prior to consumer
of that data crunching away.. mostly to deal with paper spins that many
of us in SoC vendor world work with..


> 
>> does it have to
>> be "required" or just "optional" :)
> 
> This has to be required (by the parser, kernel in our case).
> 
>>> +Required properties:
>>> +- opp-hz: Frequency in Hz
>>
>> I am just being nit picky -> should we keep Heinrich Hertz respected[2]
>> and name it opp-Hz ? No strong opinions either way.
> 
> Documentation/devicetree/booting-without-of.txt:
> 

hehe.. thanks on the reminder to RTFM.
[...]

> 
>>
>> different angle: How about just target-freq-Hz? just drop the "opp"
>> prefix for properties of an OPP node? No strong feelings here. (some
>> folks did have variations of a few Hz based on clock tree - example with
>> a crystal frequency of 19.2MHz you'd probably get 1001MHz exact, with a
>> 26MHz crystal, you'd get 1000MHz -> ofcourse round-rate is supposed to
>> help with that... but anyways.. why not say we are trying to indicate
>> target frequency? I do recollect during initial days of OPP
>> (pre-dt-adoption days) folks did complain about this - we kinda worked
>> around this with round-rated handling - but we might as well anticipate it.
> 
> Rob suggested opp- prefix and it looks good to me, lets see what
> others have to say :)

OK.

>> I agree with Mike[1] here -> why not move clocks and supply to cpu0_opp?
>> "
>> It seems wrong to me that the clock and supply data is owned by the cpu
>> node, and not the opp descriptor. Everything about the opp transition
>> should belong to a provider node. Then the cpu simply needs to consume
>> that via a phandle.
>> "
>>
>> I am not sure if we discussed that point further OR if we kinda got
>> hooked on to the "should it be in kernel" point of debate in V4.
> 
> I did send a reply to that, but no one replied after that. Probably
> you want to reply on that ?

I assume you meant [1] which in turn pointed to [2]. Thanks, will do so.


[1] http://marc.info/?l=linux-pm&m=143150734506159&w=2
[2]
https://lists.linaro.org/pipermail/linaro-kernel/2014-December/019505.html

-- 
Regards,
Nishanth Menon

Re: [PATCH V5 3/3] OPP: Add binding for 'opp-suspend'

[Nishanth Menon]

On 05/21/2015 12:49 AM, Viresh Kumar wrote:
> On 21-05-15, 00:32, Nishanth Menon wrote:
>> Why not just mark it as a property of an OPP rather than a phandle? just
>> thinking that this might setup a precedence for future needs like
>> "shutdown OPP" or "Reboot OPP" or something different that some other
>> SoC might have..
> 
> AFAIU, a property should be present in the OPP if and only if any OPP
> can set it. But in this case, only one OPP from the entire list can
> set it. So, its more a property of the list rather than every OPP
> within it. And then there wouldn't be any need to code that would
> check bugs in dtbs where multiple OPPs have it set.
> 
True.. fair enough.

Acked-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>

-- 
Regards,
Nishanth Menon
--
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

Re: [PATCH V5 1/3] OPP: Redefine bindings to overcome shortcomings

[Viresh Kumar]

On 21-05-15, 00:58, Nishanth Menon wrote:
> I assume you meant [1] which in turn pointed to [2]. Thanks, will do so.

Right.

-- 
viresh

Re: [PATCH V5 1/3] OPP: Redefine bindings to overcome shortcomings

[Rob Herring]

On Thu, May 21, 2015 at 12:46 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 21-05-15, 00:27, Nishanth Menon wrote:
>> > +This describes the OPPs belonging to a device. This node can have following
>> > +properties:

[...]

>> different angle: How about just target-freq-Hz? just drop the "opp"
>> prefix for properties of an OPP node? No strong feelings here. (some
>> folks did have variations of a few Hz based on clock tree - example with
>> a crystal frequency of 19.2MHz you'd probably get 1001MHz exact, with a
>> 26MHz crystal, you'd get 1000MHz -> ofcourse round-rate is supposed to
>> help with that... but anyways.. why not say we are trying to indicate
>> target frequency? I do recollect during initial days of OPP
>> (pre-dt-adoption days) folks did complain about this - we kinda worked
>> around this with round-rated handling - but we might as well anticipate it.
>
> Rob suggested opp- prefix and it looks good to me, lets see what
> others have to say :)

You can decide the color of the bike shed.

>
>> Thanks for adding the examples - My customer support team especially
>> will appreciate having such examples ;).
>
> I can understand that :)
>
>> I agree with Mike[1] here -> why not move clocks and supply to cpu0_opp?
>> "
>> It seems wrong to me that the clock and supply data is owned by the cpu
>> node, and not the opp descriptor. Everything about the opp transition
>> should belong to a provider node. Then the cpu simply needs to consume
>> that via a phandle.
>> "
>>
>> I am not sure if we discussed that point further OR if we kinda got
>> hooked on to the "should it be in kernel" point of debate in V4.
>
> I did send a reply to that, but no one replied after that. Probably
> you want to reply on that ?

Clock and regulator bindings describe connections in the h/w. OPPs are
not a h/w block, but rather properties of the h/w. The connections
here are to the cpu's.

Rob

Re: [PATCH V5 2/3] OPP: Allow multiple OPP tables to be passed via DT

[Viresh Kumar]

On 21-05-15, 00:34, Nishanth Menon wrote:
> > +	cpu0_opp0: opp0 {
> 
> Maybe we should rename these as cpu0_opp_table_slow opp_table-fast
> 
> > +		compatible = "operating-points-v2";
> > +		opp-name = "opp-slow";
> 
> just name = "slow" ?
> 
> > +		opp-shared;
> > +
> > +		entry00 {
> 
> rename these as opp0, opp01 etc?
> these are the actual OPP description, while, what we call "cpu0_opp0" is
> actually an OPP table choice we have.

What about this now:
+Example 5: Multiple OPP tables
+
+/ {
+       cpus {
+               cpu@0 {
+                       compatible = "arm,cortex-a7";
+                       ...
+
+                       opp-supply = <&cpu_supply>
+                       operating-points-v2 = <&cpu0_opp_table_slow>, <&cpu0_opp_table_fast>;
+               };
+       };
+
+       cpu0_opp_table_slow: opp_table_slow {
+               compatible = "operating-points-v2";
+               opp-name = "slow";
+               opp-shared;
+
+               opp00 {
+                       opp-hz = <600000000>;
+                       ...
+               };
+
+               opp01 {
+                       opp-hz = <800000000>;
+                       ...
+               };
+       };
+
+       cpu0_opp_table_fast: opp_table_fast {
+               compatible = "operating-points-v2";
+               opp-name = "fast";
+               opp-shared;
+
+               opp10 {
+                       opp-hz = <1000000000>;
+                       ...
+               };
+
+               opp11 {
+                       opp-hz = <1100000000>;
+                       ...
+               };
+       };
+};

May I carry your Ack or you want to give it again?

-- 
viresh

Re: [PATCH V5 1/3] OPP: Redefine bindings to overcome shortcomings

[Viresh Kumar]

On 21-05-15, 00:27, Nishanth Menon wrote:
> I would suggest adding a link to how future vendor specific extension
> docs might look like - maybe this is probably not the time to discuss
> this.. but anyways.. we could make some statement to the effect of "SoC
> vendor specfic extensions are documented as
> Documentation/devicetree/bindings/power/<vendor>,opp.txt and should
> clearly indicate that the extensions are permitted only under the
> operating-points-v2 compatible description."

Will this work ?:

If required, this can be extended for SoC vendor specfic bindings. Such bindings
should be documented as Documentation/devicetree/bindings/power/<vendor>-opp.txt
and should have a compatible description like: "operating-points-v2-<vendor>".

-- 
viresh

Re: [PATCH V5 1/3] OPP: Redefine bindings to overcome shortcomings

[Viresh Kumar]

On 22-05-15, 10:55, Rob Herring wrote:
> > Rob suggested opp- prefix and it looks good to me, lets see what
> > others have to say :)
> 
> You can decide the color of the bike shed.

I like that color more :)

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