[PATCH 0/7] OPP: Introduce OPP bindings V2 and supporting code

[PATCH 0/7] OPP: Introduce OPP bindings V2 and supporting code

[Viresh Kumar]

Now that I have received an verbal Ack from Rob Herring (in a personal
conversation) about the bindings, I am showing how the code looks like with
these new bindings.

Some part is still now done:
- Interface for adding new detailed OPPs from platform code instead of DT
- Providing cpufreq helpers for the next OPPs
- Providing regulator helpers for the target/min/max ranges

Please provide feedback on how this looks like..

--
viresh

Viresh Kumar (7):
  OPP: Redefine bindings to overcome shortcomings
  opp: Relocate few routines
  OPP: Break _opp_add_dynamic() into smaller functions
  opp: Parse new (v2) bindings
  opp: convert device_opp->dev to a list of devices
  opp: Add helpers for initializing CPU opps
  cpufreq-dt: Use DT to set policy->cpus/related_cpus

 Documentation/devicetree/bindings/power/opp.txt |  407 ++++++++-
 drivers/base/power/opp.c                        | 1041 +++++++++++++++++------
 drivers/cpufreq/cpufreq-dt.c                    |   16 +-
 include/linux/pm_opp.h                          |   23 +
 4 files changed, 1232 insertions(+), 255 deletions(-)

-- 
2.3.0.rc0.44.ga94655d

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

[Viresh Kumar]

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

There had been multiple band-aid approaches to get them fixed (The latest one
being: http://www.mail-archive.com/devicetree@vger.kernel.org/msg53398.html).
For obvious reasons Rob rejected them and shown the right path forward.

The shortcomings we are trying to solve here:

- Getting clock sharing information between CPUs. Single shared clock vs
  independent clock per core vs shared clock per cluster.

- Support for turbo modes

- Support for intermediate frequencies or OPPs we can switch to.

- 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 | 407 +++++++++++++++++++++++-
 1 file changed, 403 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
index 74499e5033fc..a64621819d7c 100644
--- a/Documentation/devicetree/bindings/power/opp.txt
+++ b/Documentation/devicetree/bindings/power/opp.txt
@@ -1,8 +1,407 @@
-* 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-frequency pairs and some implementations have the
+liberty of choosing these pairs. These pairs 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.
+
+
+* 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 frequency-voltage pairs. Their
+  name isn't significant but their phandles can be used to reference an OPP.
+
+Optional properties:
+- shared-opp: Indicates that device nodes using this OPP descriptor's phandle
+  switch their DVFS state together, i.e. they share clock lines. Missing
+  property means devices have independent clock lines, but they share OPPs.
+
+
+* OPP Node
+
+This defines frequency-voltage pairs along with other related properties.
+
+Required properties:
+- opp-khz: Frequency in kHz
+
+Optional properties:
+- opp-microvolt: voltage in micro Volts. Its an array with size one or three.
+  Single entry is for target voltage and three entries are for (in the specified
+  order) <target min max> voltage.
+- clock-latency-ns: Specifies the maximum possible transition latency (in
+  nanoseconds) for switching to this OPP from any other OPP.
+- opp-next: It contains a list of phandles of other OPPs, to which we can switch
+  directly from this OPP (Explained later with examples). Missing property means
+  no restriction on switching to other OPPs.
+- turbo-mode: Marks the OPP to be used only for turbo modes.
+- 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";
+		shared-opp;
+
+		entry00 {
+			opp-khz = <1000000>;
+			opp-microvolt = <970000 975000 985000>;
+			clock-latency-ns = <300000>;
+		};
+		entry01 {
+			opp-khz = <1100000>;
+			opp-microvolt = <980000 1000000 1010000>;
+			clock-latency-ns = <310000>;
+		};
+		entry02 {
+			opp-khz = <1200000>;
+			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 shared-opp property means CPUs switch DVFS states
+		 * independently.
+		 */
+
+		entry00 {
+			opp-khz = <1000000>;
+			opp-microvolt = <970000 975000 985000>;
+			clock-latency-ns = <300000>;
+		};
+		entry01 {
+			opp-khz = <1100000>;
+			opp-microvolt = <980000 1000000 1010000>;
+			clock-latency-ns = <310000>;
+		};
+		entry02 {
+			opp-khz = <1200000>;
+			opp-microvolt = <1025000>;
+			clock-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";
+		shared-opp;
+
+		entry00 {
+			opp-khz = <1000000>;
+			opp-microvolt = <970000 975000 985000>;
+			clock-latency-ns = <300000>;
+		};
+		entry01 {
+			opp-khz = <1100000>;
+			opp-microvolt = <980000 1000000 1010000>;
+			clock-latency-ns = <310000>;
+		};
+		entry02 {
+			opp-khz = <1200000>;
+			opp-microvolt = <1025000>;
+			clock-latency-ns = <290000>;
+			turbo-mode;
+		};
+	};
+
+	cluster1_opp: opp1 {
+		compatible = "operating-points-v2";
+		shared-opp;
+
+		entry10 {
+			opp-khz = <1300000>;
+			opp-microvolt = <1045000 1050000 1055000>;
+			clock-latency-ns = <400000>;
+		};
+		entry11 {
+			opp-khz = <1400000>;
+			opp-microvolt = <1075000>;
+			clock-latency-ns = <400000>;
+		};
+		entry12 {
+			opp-khz = <1500000>;
+			opp-microvolt = <1010000 1100000 1110000>;
+			clock-latency-ns = <400000>;
+			turbo-mode;
+		};
+	};
+};
+
+Example 4: How to use "opp-next" property ?
+
+1.) Switch to a intermediate OPP (entry00) before switching to any other OPP.
+
+/ {
+	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 = <&cpu0_opp>;
+		};
+	};
+
+	cpu0_opp: opp0 {
+		compatible = "operating-points-v2";
+		shared-opp;
+
+		opp_next: entry00 {
+			opp-khz = <500000>;
+			opp-microvolt = <800000>;
+			clock-latency-ns = <300000>;
+			/* Can switch to any OPP from here */
+		};
+		entry01 {
+			opp-khz = <600000>;
+			opp-microvolt = <800000>;
+			clock-latency-ns = <300000>;
+			opp-next = <&opp_next>;
+		};
+		entry02 {
+			opp-khz = <900000>;
+			opp-microvolt = <970000 975000 985000>;
+			clock-latency-ns = <300000>;
+			opp-next = <&opp_next>;
+		};
+		entry03 {
+			opp-khz = <1000000>;
+			opp-microvolt = <970000 975000 985000>;
+			clock-latency-ns = <300000>;
+			opp-next = <&opp_next>;
+		};
+		entry04 {
+			opp-khz = <1100000>;
+			opp-microvolt = <980000 1000000 1010000>;
+			clock-latency-ns = <310000>;
+			opp-next = <&opp_next>;
+		};
+		entry05 {
+			opp-khz = <1200000>;
+			opp-microvolt = <1025000>;
+			clock-latency-ns = <290000>;
+			opp-next = <&opp_next>;
+			turbo-mode;
+		};
+	};
+};
+
+2.) Can only switch to the next or previous OPP directly.
+
+/ {
+	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 = <&cpu0_opp>;
+		};
+	};
+
+	cpu0_opp: opp0 {
+		compatible = "operating-points-v2";
+		shared-opp;
+
+		opp_next0: entry00 {
+			opp-khz = <500000>;
+			opp-microvolt = <800000>;
+			clock-latency-ns = <300000>;
+			opp-next = <&opp_next1>;
+		};
+		opp_next1: entry01 {
+			opp-khz = <600000>;
+			opp-microvolt = <800000>;
+			clock-latency-ns = <300000>;
+			opp-next = <&opp_next0>, <&opp_next2>;
+		};
+		opp_next2: entry02 {
+			opp-khz = <900000>;
+			opp-microvolt = <970000 975000 985000>;
+			clock-latency-ns = <300000>;
+			opp-next = <&opp_next1>, <&opp_next3>;
+		};
+		opp_next3: entry03 {
+			opp-khz = <1000000>;
+			opp-microvolt = <970000 975000 985000>;
+			clock-latency-ns = <300000>;
+			opp-next = <&opp_next2>, <&opp_next4>;
+		};
+		opp_next4: entry04 {
+			opp-khz = <1100000>;
+			opp-microvolt = <980000 1000000 1010000>;
+			clock-latency-ns = <310000>;
+			opp-next = <&opp_next3>, <&opp_next5>;
+		};
+		opp_next5: entry05 {
+			opp-khz = <1200000>;
+			opp-microvolt = <1025000>;
+			clock-latency-ns = <290000>;
+			opp-next = <&opp_next4>;
+			turbo-mode;
+		};
+	};
+};
+
+
+
+Deprecated Bindings
+-------------------
 
 Properties:
 - operating-points: An array of 2-tuples items, and each item consists
-- 
2.3.0.rc0.44.ga94655d

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

[Kevin Hilman]

Viresh Kumar <viresh.kumar@linaro.org> writes:

> Current OPP (Operating performance point) DT bindings are proven to be
> insufficient at multiple instances.
>
> There had been multiple band-aid approaches to get them fixed (The latest one
> being: http://www.mail-archive.com/devicetree@vger.kernel.org/msg53398.html).
> For obvious reasons Rob rejected them and shown the right path forward.
>
> The shortcomings we are trying to solve here:
>
> - Getting clock sharing information between CPUs. Single shared clock vs
>   independent clock per core vs shared clock per cluster.
>
> - Support for turbo modes
>
> - Support for intermediate frequencies or OPPs we can switch to.
>
> - 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>

[...]

> +* 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 frequency-voltage pairs. Their
> +  name isn't significant but their phandles can be used to reference an OPP.
> +
> +Optional properties:
> +- shared-opp: Indicates that device nodes using this OPP descriptor's phandle
> +  switch their DVFS state together, i.e. they share clock lines. 

... or shared voltage rail?

> +  Missing property means devices have independent clock lines, but they share OPPs.

huh?  missing 'shared-opp' property means they share OPPs?  -ECONFUSED.

Maybe I missed some of the discussion of why this property is needed,
but I'm left wondering why it's needed explicitly.  With the OPPs as
part of the CPU nodes, shouldnt' the "shared" nature of the OPP be
easily derived from the clock and or regulator (opp-supply) property of
the CPU nodes?  IOW, if two CPU nodes share a clock and/or a regulator,
the framework should know it's "shared".

Or, were there other reasons besides clocks/regulators why this property
was needed (if so, the documentation doesn't describe them.)

Kevin

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

[Viresh Kumar]

On 24 February 2015 at 04:06, Kevin Hilman <khilman@kernel.org> wrote:
> Viresh Kumar <viresh.kumar@linaro.org> writes:

>> +Optional properties:
>> +- shared-opp: Indicates that device nodes using this OPP descriptor's phandle
>> +  switch their DVFS state together, i.e. they share clock lines.
>
> ... or shared voltage rail?

The point is that they switch their frequencies together. Which means,
sharing voltage
rail + PLLs .. How do we write it properly ?

>> +  Missing property means devices have independent clock lines, but they share OPPs.
>
> huh?  missing 'shared-opp' property means they share OPPs?  -ECONFUSED.

Right. s/OPPs/OPP tables ..

Makes sense now ?

> Maybe I missed some of the discussion of why this property is needed,
> but I'm left wondering why it's needed explicitly.  With the OPPs as

So that same OPP tables can be shared across CPUs which don't share
voltage rails..
For example, Krait. We only need to define single set of tables and
all CPUs will point
to it. But this property would be missing in that case as CPUs don't
change their DVFS
state together.

> part of the CPU nodes, shouldnt' the "shared" nature of the OPP be
> easily derived from the clock and or regulator (opp-supply) property of
> the CPU nodes?  IOW, if two CPU nodes share a clock and/or a regulator,
> the framework should know it's "shared".

So you missed all earlier discussions :), there were lots of concerns
around that.
And the best solution we found out is to do it this way..

- There can be multiple clocks/regulators present in CPU's DT node and
that makes
it complex.
- There are cases where immediate clock parents of CPUs are different
but somewhere
higher in the hierarchy they have a common ancestor, which is
responsible for rate
change. And so it would be difficult to find out if they share
clock/regulator or not..
- People wanted it to be some static data instead of trying to find
with help of some
algorithms..

> Or, were there other reasons besides clocks/regulators why this property
> was needed (if so, the documentation doesn't describe them.)

I am not sure if we need to mention anything else.. But yes, please let me know
if it can be improved a bit.

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

[Kevin Hilman]

Viresh Kumar <viresh.kumar@linaro.org> writes:

> On 24 February 2015 at 04:06, Kevin Hilman <khilman@kernel.org> wrote:
>> Viresh Kumar <viresh.kumar@linaro.org> writes:
>
>>> +Optional properties:
>>> +- shared-opp: Indicates that device nodes using this OPP descriptor's phandle
>>> +  switch their DVFS state together, i.e. they share clock lines.
>>
>> ... or shared voltage rail?
>
> The point is that they switch their frequencies together. Which means,
> sharing voltage rail + PLLs .. How do we write it properly ?

s/they share clock lines/they share clock and/or voltage lines/

>>> +  Missing property means devices have independent clock lines, but they share OPPs.
>>
>> huh?  missing 'shared-opp' property means they share OPPs?  -ECONFUSED.
>
> Right. s/OPPs/OPP tables ..
>
> Makes sense now ?

Yes.

>> Maybe I missed some of the discussion of why this property is needed,
>> but I'm left wondering why it's needed explicitly.  With the OPPs as
>
> So that same OPP tables can be shared across CPUs which don't share
> voltage rails..  For example, Krait. We only need to define single set
> of tables and all CPUs will point to it. But this property would be
> missing in that case as CPUs don't change their DVFS state together.


>> part of the CPU nodes, shouldnt' the "shared" nature of the OPP be
>> easily derived from the clock and or regulator (opp-supply) property of
>> the CPU nodes?  IOW, if two CPU nodes share a clock and/or a regulator,
>> the framework should know it's "shared".
>
> So you missed all earlier discussions :), there were lots of concerns
> around that.  And the best solution we found out is to do it this
> way..
>
> - There can be multiple clocks/regulators present in CPU's DT node and
> that makes it complex.
>
> - There are cases where immediate clock parents of CPUs are different
> but somewhere higher in the hierarchy they have a common ancestor,
> which is responsible for rate change. And so it would be difficult to
> find out if they share clock/regulator or not..
>
> - People wanted it to be some static data instead of trying to find
> with help of some algorithms..

OK, fair enough.  Looks like it's been thought through.  

However, in the end, I don't think it's going to avoid the "help of some
algorithms."  The flag will tell you it's shared, but not how, and that
will likely still need to be determined.

Kevin

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

[viresh kumar]

On Tuesday 24 February 2015 10:42 PM, Kevin Hilman wrote:
> Viresh Kumar <viresh.kumar@linaro.org> writes:

> s/they share clock lines/they share clock and/or voltage lines/

Hmm, will update that.

> OK, fair enough.  Looks like it's been thought through.  
> 
> However, in the end, I don't think it's going to avoid the "help of some
> algorithms."  The flag will tell you it's shared, but not how, and that
> will likely still need to be determined.

What did you mean by 'how' here? You wanted to say which devices share the
OPP? Yeah, a small piece of code is required and that's present in my patchset.

[PATCH 2/7] opp: Relocate few routines

[Viresh Kumar]

In order to prepare for the later commits, this relocates few routines towards
the top as they will be used earlier in the code.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 192 +++++++++++++++++++++++------------------------
 1 file changed, 96 insertions(+), 96 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 15bf29974c31..9e2118284f02 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -408,6 +408,102 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
 /**
+ * _kfree_opp_rcu() - Free OPP RCU handler
+ * @head:	RCU head
+ */
+static void _kfree_opp_rcu(struct rcu_head *head)
+{
+	struct dev_pm_opp *opp = container_of(head, struct dev_pm_opp, rcu_head);
+
+	kfree_rcu(opp, rcu_head);
+}
+
+/**
+ * _kfree_device_rcu() - Free device_opp RCU handler
+ * @head:	RCU head
+ */
+static void _kfree_device_rcu(struct rcu_head *head)
+{
+	struct device_opp *device_opp = container_of(head, struct device_opp, rcu_head);
+
+	kfree_rcu(device_opp, rcu_head);
+}
+
+/**
+ * _opp_remove()  - Remove an OPP from a table definition
+ * @dev_opp:	points back to the device_opp struct this opp belongs to
+ * @opp:	pointer to the OPP to remove
+ *
+ * This function removes an opp definition from the opp list.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * It is assumed that the caller holds required mutex for an RCU updater
+ * strategy.
+ */
+static void _opp_remove(struct device_opp *dev_opp,
+			struct dev_pm_opp *opp)
+{
+	/*
+	 * Notify the changes in the availability of the operable
+	 * frequency/voltage list.
+	 */
+	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
+	list_del_rcu(&opp->node);
+	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
+
+	if (list_empty(&dev_opp->opp_list)) {
+		list_del_rcu(&dev_opp->node);
+		call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,
+			  _kfree_device_rcu);
+	}
+}
+
+/**
+ * dev_pm_opp_remove()  - Remove an OPP from OPP list
+ * @dev:	device for which we do this operation
+ * @freq:	OPP to remove with matching 'freq'
+ *
+ * This function removes an opp from the opp list.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ */
+void dev_pm_opp_remove(struct device *dev, unsigned long freq)
+{
+	struct dev_pm_opp *opp;
+	struct device_opp *dev_opp;
+	bool found = false;
+
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp))
+		goto unlock;
+
+	list_for_each_entry(opp, &dev_opp->opp_list, node) {
+		if (opp->rate == freq) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		dev_warn(dev, "%s: Couldn't find OPP with freq: %lu\n",
+			 __func__, freq);
+		goto unlock;
+	}
+
+	_opp_remove(dev_opp, opp);
+unlock:
+	mutex_unlock(&dev_opp_list_lock);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
+
+/**
  * _add_device_opp() - Allocate a new device OPP table
  * @dev:	device for which we do this operation
  *
@@ -572,102 +668,6 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 EXPORT_SYMBOL_GPL(dev_pm_opp_add);
 
 /**
- * _kfree_opp_rcu() - Free OPP RCU handler
- * @head:	RCU head
- */
-static void _kfree_opp_rcu(struct rcu_head *head)
-{
-	struct dev_pm_opp *opp = container_of(head, struct dev_pm_opp, rcu_head);
-
-	kfree_rcu(opp, rcu_head);
-}
-
-/**
- * _kfree_device_rcu() - Free device_opp RCU handler
- * @head:	RCU head
- */
-static void _kfree_device_rcu(struct rcu_head *head)
-{
-	struct device_opp *device_opp = container_of(head, struct device_opp, rcu_head);
-
-	kfree_rcu(device_opp, rcu_head);
-}
-
-/**
- * _opp_remove()  - Remove an OPP from a table definition
- * @dev_opp:	points back to the device_opp struct this opp belongs to
- * @opp:	pointer to the OPP to remove
- *
- * This function removes an opp definition from the opp list.
- *
- * Locking: The internal device_opp and opp structures are RCU protected.
- * It is assumed that the caller holds required mutex for an RCU updater
- * strategy.
- */
-static void _opp_remove(struct device_opp *dev_opp,
-			struct dev_pm_opp *opp)
-{
-	/*
-	 * Notify the changes in the availability of the operable
-	 * frequency/voltage list.
-	 */
-	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
-	list_del_rcu(&opp->node);
-	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
-
-	if (list_empty(&dev_opp->opp_list)) {
-		list_del_rcu(&dev_opp->node);
-		call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,
-			  _kfree_device_rcu);
-	}
-}
-
-/**
- * dev_pm_opp_remove()  - Remove an OPP from OPP list
- * @dev:	device for which we do this operation
- * @freq:	OPP to remove with matching 'freq'
- *
- * This function removes an opp from the opp list.
- *
- * Locking: The internal device_opp and opp structures are RCU protected.
- * Hence this function internally uses RCU updater strategy with mutex locks
- * to keep the integrity of the internal data structures. Callers should ensure
- * that this function is *NOT* called under RCU protection or in contexts where
- * mutex cannot be locked.
- */
-void dev_pm_opp_remove(struct device *dev, unsigned long freq)
-{
-	struct dev_pm_opp *opp;
-	struct device_opp *dev_opp;
-	bool found = false;
-
-	/* Hold our list modification lock here */
-	mutex_lock(&dev_opp_list_lock);
-
-	dev_opp = _find_device_opp(dev);
-	if (IS_ERR(dev_opp))
-		goto unlock;
-
-	list_for_each_entry(opp, &dev_opp->opp_list, node) {
-		if (opp->rate == freq) {
-			found = true;
-			break;
-		}
-	}
-
-	if (!found) {
-		dev_warn(dev, "%s: Couldn't find OPP with freq: %lu\n",
-			 __func__, freq);
-		goto unlock;
-	}
-
-	_opp_remove(dev_opp, opp);
-unlock:
-	mutex_unlock(&dev_opp_list_lock);
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
-
-/**
  * _opp_set_availability() - helper to set the availability of an opp
  * @dev:		device for which we do this operation
  * @freq:		OPP frequency to modify availability
-- 
2.3.0.rc0.44.ga94655d

[PATCH 3/7] OPP: Break _opp_add_dynamic() into smaller functions

[Viresh Kumar]

Later commits would add support for new OPP bindings and this would be required
then. So, lets do it in a separate patch to make it reviewing easy.

Another change worth noticing is INIT_LIST_HEAD(&opp->node). We weren't doing it
earlier as we never tried to delete a list node before it is added to list. But
this wouldn't be the case anymore. We might try to delete a node (just to reuse
the same code paths), without it being getting added to the list.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 127 ++++++++++++++++++++++++++++-------------------
 1 file changed, 75 insertions(+), 52 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 9e2118284f02..904dcf386747 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -433,6 +433,7 @@ static void _kfree_device_rcu(struct rcu_head *head)
  * _opp_remove()  - Remove an OPP from a table definition
  * @dev_opp:	points back to the device_opp struct this opp belongs to
  * @opp:	pointer to the OPP to remove
+ * @notify:	OPP_EVENT_REMOVE notification should be sent or not
  *
  * This function removes an opp definition from the opp list.
  *
@@ -441,13 +442,14 @@ static void _kfree_device_rcu(struct rcu_head *head)
  * strategy.
  */
 static void _opp_remove(struct device_opp *dev_opp,
-			struct dev_pm_opp *opp)
+			struct dev_pm_opp *opp, bool notify)
 {
 	/*
 	 * Notify the changes in the availability of the operable
 	 * frequency/voltage list.
 	 */
-	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
+	if (notify)
+		srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
 	list_del_rcu(&opp->node);
 	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
 
@@ -497,7 +499,7 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq)
 		goto unlock;
 	}
 
-	_opp_remove(dev_opp, opp);
+	_opp_remove(dev_opp, opp, true);
 unlock:
 	mutex_unlock(&dev_opp_list_lock);
 }
@@ -533,6 +535,63 @@ static struct device_opp *_add_device_opp(struct device *dev)
 	return dev_opp;
 }
 
+static struct dev_pm_opp *_allocate_opp(struct device *dev,
+					struct device_opp **dev_opp)
+{
+	struct dev_pm_opp *opp;
+
+	/* allocate new OPP node */
+	opp = kzalloc(sizeof(*opp), GFP_KERNEL);
+	if (!opp)
+		return NULL;
+
+	INIT_LIST_HEAD(&opp->node);
+
+	/* Check for existing list for 'dev' */
+	*dev_opp = _find_device_opp(dev);
+	if (IS_ERR(*dev_opp)) {
+		*dev_opp = _add_device_opp(dev);
+		if (!*dev_opp) {
+			kfree(opp);
+			return NULL;
+		}
+	}
+
+	return opp;
+}
+
+static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp)
+{
+	struct dev_pm_opp *opp = NULL;
+	struct list_head *head = &dev_opp->opp_list;
+
+	/*
+	 * Insert new OPP in order of increasing frequency
+	 * and discard if already present
+	 */
+	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
+		if (new_opp->rate <= opp->rate)
+			break;
+
+		head = &opp->node;
+	}
+
+	/* Duplicate OPPs ? */
+	if (opp && new_opp->rate == opp->rate) {
+		dev_warn(dev_opp->dev,
+			 "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
+			 __func__, opp->rate, opp->u_volt, opp->available,
+			 new_opp->rate, new_opp->u_volt, new_opp->available);
+		return opp->available && new_opp->u_volt == opp->u_volt ?
+			0 : -EEXIST;
+	}
+
+	new_opp->dev_opp = dev_opp;
+	list_add_rcu(&new_opp->node, head);
+
+	return 0;
+}
+
 /**
  * _opp_add_dynamic() - Allocate a dynamic OPP.
  * @dev:	device for which we do this operation
@@ -563,66 +622,29 @@ static struct device_opp *_add_device_opp(struct device *dev)
 static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 			    long u_volt, bool dynamic)
 {
-	struct device_opp *dev_opp = NULL;
-	struct dev_pm_opp *opp, *new_opp;
-	struct list_head *head;
+	struct device_opp *dev_opp;
+	struct dev_pm_opp *new_opp;
 	int ret;
 
-	/* allocate new OPP node */
-	new_opp = kzalloc(sizeof(*new_opp), GFP_KERNEL);
-	if (!new_opp) {
-		dev_warn(dev, "%s: Unable to create new OPP node\n", __func__);
-		return -ENOMEM;
-	}
-
 	/* Hold our list modification lock here */
 	mutex_lock(&dev_opp_list_lock);
 
+	new_opp = _allocate_opp(dev, &dev_opp);
+	if (!new_opp) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
 	/* populate the opp table */
 	new_opp->rate = freq;
 	new_opp->u_volt = u_volt;
 	new_opp->available = true;
 	new_opp->dynamic = dynamic;
 
-	/* Check for existing list for 'dev' */
-	dev_opp = _find_device_opp(dev);
-	if (IS_ERR(dev_opp)) {
-		dev_opp = _add_device_opp(dev);
-		if (!dev_opp) {
-			ret = -ENOMEM;
-			goto free_opp;
-		}
-
-		head = &dev_opp->opp_list;
-		goto list_add;
-	}
-
-	/*
-	 * Insert new OPP in order of increasing frequency
-	 * and discard if already present
-	 */
-	head = &dev_opp->opp_list;
-	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
-		if (new_opp->rate <= opp->rate)
-			break;
-		else
-			head = &opp->node;
-	}
-
-	/* Duplicate OPPs ? */
-	if (new_opp->rate == opp->rate) {
-		ret = opp->available && new_opp->u_volt == opp->u_volt ?
-			0 : -EEXIST;
-
-		dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
-			 __func__, opp->rate, opp->u_volt, opp->available,
-			 new_opp->rate, new_opp->u_volt, new_opp->available);
+	ret = _opp_add(new_opp, dev_opp);
+	if (ret)
 		goto free_opp;
-	}
 
-list_add:
-	new_opp->dev_opp = dev_opp;
-	list_add_rcu(&new_opp->node, head);
 	mutex_unlock(&dev_opp_list_lock);
 
 	/*
@@ -633,8 +655,9 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	return 0;
 
 free_opp:
+	_opp_remove(dev_opp, new_opp, false);
+unlock:
 	mutex_unlock(&dev_opp_list_lock);
-	kfree(new_opp);
 	return ret;
 }
 
@@ -922,7 +945,7 @@ void of_free_opp_table(struct device *dev)
 	/* Free static OPPs */
 	list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) {
 		if (!opp->dynamic)
-			_opp_remove(dev_opp, opp);
+			_opp_remove(dev_opp, opp, true);
 	}
 
 	mutex_unlock(&dev_opp_list_lock);
-- 
2.3.0.rc0.44.ga94655d

[PATCH 4/7] opp: Parse new (v2) bindings

[Viresh Kumar]

This adds support in OPP library to parse and create list of OPPs from
operating-points-v2 bindings. It takes care of most of the properties of new
bindings (except shared-opp, which will be handled separately).

For backward compatibility, we keep supporting earlier bindings. We try to
search for the new bindings first, in case they aren't present we look for the
old deprecated ones.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 342 +++++++++++++++++++++++++++++++++++++++++++----
 include/linux/pm_opp.h   |   6 +
 2 files changed, 323 insertions(+), 25 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 904dcf386747..b7b9c33fbb65 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -49,12 +49,21 @@
  *		are protected by the dev_opp_list_lock for integrity.
  *		IMPORTANT: the opp nodes should be maintained in increasing
  *		order.
- * @dynamic:	not-created from static DT entries.
  * @available:	true/false - marks if this OPP as available or not
+ * @dynamic:	not-created from static DT entries.
+ * @turbo:	true if turbo (boost) OPP
  * @rate:	Frequency in hertz
- * @u_volt:	Nominal voltage in microvolts corresponding to this OPP
+ * @u_volt:	Target voltage in microvolts corresponding to this OPP
+ * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
+ * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
+ * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
+ *		frequency from any other OPP's frequency.
  * @dev_opp:	points back to the device_opp struct this opp belongs to
  * @rcu_head:	RCU callback head used for deferred freeing
+ * @np:		OPP's device node.
+ * @next:	Array of OPPs we can switch to from this OPP.
+ * @next_np:	Array of device nodes of OPP to which we can switch from this OPP.
+ * @count:	count of 'next' array.
  *
  * This structure stores the OPP information for a given device.
  */
@@ -63,11 +72,20 @@ struct dev_pm_opp {
 
 	bool available;
 	bool dynamic;
+	bool turbo;
 	unsigned long rate;
 	unsigned long u_volt;
+	unsigned long u_volt_min;
+	unsigned long u_volt_max;
+	unsigned long clock_latency_ns;
 
 	struct device_opp *dev_opp;
 	struct rcu_head rcu_head;
+
+	struct device_node *np;
+	struct dev_pm_opp **next;
+	struct device_node **next_np;
+	unsigned int next_count;
 };
 
 /**
@@ -444,15 +462,21 @@ static void _kfree_device_rcu(struct rcu_head *head)
 static void _opp_remove(struct device_opp *dev_opp,
 			struct dev_pm_opp *opp, bool notify)
 {
+	/* Free any next OPP nodes */
+	kfree(opp->next);
+
 	/*
 	 * Notify the changes in the availability of the operable
 	 * frequency/voltage list.
 	 */
 	if (notify)
 		srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
+
+	/* Free OPP */
 	list_del_rcu(&opp->node);
 	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
 
+	/* Free device if all OPPs are freed */
 	if (list_empty(&dev_opp->opp_list)) {
 		list_del_rcu(&dev_opp->node);
 		call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,
@@ -662,6 +686,170 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 }
 
 /**
+ * _opp_add_dynamic_v2() - Allocate a dynamic OPP according to 'v2' DT bindings.
+ * @dev:	device for which we do this operation
+ * @np:		device node
+ * @dynamic:	Dynamically added OPPs.
+ *
+ * This function adds an opp definition to the opp list and returns status. The
+ * opp can be controlled using dev_pm_opp_enable/disable functions and may be
+ * removed by dev_pm_opp_remove.
+ *
+ * NOTE: "dynamic" parameter impacts OPPs added by the of_init_opp_table and
+ * freed by of_free_opp_table.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ *
+ * Return:
+ * 0		On success OR
+ *		Duplicate OPPs (both freq and volt are same) and opp->available
+ * -EEXIST	Freq are same and volt are different OR
+ *		Duplicate OPPs (both freq and volt are same) and !opp->available
+ * -ENOMEM	Memory allocation failure
+ * -EINVAL	Failed parsing the OPP node
+ */
+static int _opp_add_dynamic_v2(struct device *dev, struct device_node *np,
+			       bool dynamic)
+{
+	struct device_opp *dev_opp;
+	struct dev_pm_opp *new_opp;
+	int ret;
+
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
+	new_opp = _allocate_opp(dev, &dev_opp);
+	if (!new_opp) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	ret = of_property_read_u32(np, "opp-khz", (u32 *)&new_opp->rate);
+	if (ret < 0) {
+		dev_err(dev, "%s: opp-khz not found\n", __func__);
+		goto free_opp;
+	}
+	new_opp->rate *= 1000;
+
+	if (of_get_property(np, "turbo-mode", NULL))
+		new_opp->turbo = true;
+
+	new_opp->np = np;
+	new_opp->dynamic = dynamic;
+	new_opp->available = of_device_is_available(np);
+	of_property_read_u32(np, "clock-latency-ns",
+			     (u32 *)&new_opp->clock_latency_ns);
+
+	/* read opp-microvolt array */
+	ret = of_property_count_u32_elems(np, "opp-microvolt");
+	if (ret == 1 || ret == 3) {
+		/* There can be one or three elements here */
+		ret = of_property_read_u32_array(np, "opp-microvolt",
+						 (u32 *)&new_opp->u_volt, ret);
+		if (ret) {
+			dev_err(dev, "%s: error parsing opp-microvolt: %d\n",
+				__func__, ret);
+			goto free_opp;
+		}
+	}
+
+	/* Parse OPP phandles */
+	ret = of_property_count_u32_elems(np, "opp-next");
+	if (ret > 0) {
+		int i, size = sizeof(*new_opp->next) + sizeof(*new_opp->next_np);
+		struct device_node **next_np;
+
+		/* Allocate memory for both 'next' and 'next_np' */
+		new_opp->next = kmalloc(size * ret, GFP_KERNEL);
+		if (!new_opp->next) {
+			ret = -ENOMEM;
+			goto free_opp;
+		}
+
+		new_opp->next_count = ret;
+		new_opp->next_np = (struct device_node **)(new_opp->next + ret);
+		next_np = new_opp->next_np;
+
+		/*
+		 * Parse OPP phandles for now, we will create the list of OPPs
+		 * once all are available.
+		 */
+		for (i = 0; i < ret; i++) {
+			next_np[i] = of_parse_phandle(np, "opp-next", i);
+			if (!next_np[i]) {
+				dev_err(dev, "%s: Failed to get opp phandle\n",
+					__func__);
+				ret = -EINVAL;
+				goto free_opp;
+			}
+		}
+	}
+
+	ret = _opp_add(new_opp, dev_opp);
+	if (ret)
+		goto free_opp;
+
+	pr_debug("%s: dynamic:%d turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu next-count:%u\n",
+		 __func__, new_opp->dynamic, new_opp->turbo, new_opp->rate,
+		 new_opp->u_volt, new_opp->u_volt_min, new_opp->u_volt_max,
+		 new_opp->clock_latency_ns, new_opp->next_count);
+
+	mutex_unlock(&dev_opp_list_lock);
+
+	/*
+	 * Notify the changes in the availability of the operable
+	 * frequency/voltage list.
+	 */
+	srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp);
+	return 0;
+
+free_opp:
+	_opp_remove(dev_opp, new_opp, false);
+unlock:
+	mutex_unlock(&dev_opp_list_lock);
+	return ret;
+}
+
+static struct dev_pm_opp *_get_opp_from_np(struct device_opp *dev_opp,
+					   struct device_node *np)
+{
+	struct dev_pm_opp *opp;
+
+	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node)
+		if (opp->np == np)
+			return opp;
+
+	return NULL;
+}
+
+static void _opp_fill_opp_next(struct device *dev)
+{
+	struct device_opp *dev_opp;
+	struct dev_pm_opp *opp;
+	int i;
+
+	dev_opp = _find_device_opp(dev);
+	if (WARN_ON(!dev_opp))
+		return;
+
+	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
+		if (!opp->next)
+			continue;
+
+		for (i = 0; i < opp->next_count; i++) {
+			opp->next[i] = _get_opp_from_np(dev_opp, opp->next_np[i]);
+			if (!opp->next[i])
+				dev_err(dev, "%s: Have np but no OPP: %d\n",
+					__func__, i);
+		}
+	}
+}
+
+/**
  * dev_pm_opp_add()  - Add an OPP table from a table definitions
  * @dev:	device for which we do this operation
  * @freq:	Frequency in Hz for this OPP
@@ -851,29 +1039,89 @@ struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev)
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_notifier);
 
 #ifdef CONFIG_OF
-/**
- * of_init_opp_table() - Initialize opp table from device tree
- * @dev:	device pointer used to lookup device OPPs.
- *
- * Register the initial OPP table with the OPP library for given device.
- *
- * Locking: The internal device_opp and opp structures are RCU protected.
- * Hence this function indirectly uses RCU updater strategy with mutex locks
- * to keep the integrity of the internal data structures. Callers should ensure
- * that this function is *NOT* called under RCU protection or in contexts where
- * mutex cannot be locked.
- *
- * Return:
- * 0		On success OR
- *		Duplicate OPPs (both freq and volt are same) and opp->available
- * -EEXIST	Freq are same and volt are different OR
- *		Duplicate OPPs (both freq and volt are same) and !opp->available
- * -ENOMEM	Memory allocation failure
- * -ENODEV	when 'operating-points' property is not found or is invalid data
- *		in device node.
- * -ENODATA	when empty 'operating-points' property is found
- */
-int of_init_opp_table(struct device *dev)
+void of_free_opp_table(struct device *dev);
+
+/* Returns opp descriptor node from its phandle. Caller must do of_node_put() */
+static struct device_node *
+_of_get_opp_desc_node_from_prop(struct device *dev, const struct property *prop)
+{
+	struct device_node *opp_np;
+
+	opp_np = of_find_node_by_phandle(be32_to_cpup(prop->value));
+	if (!opp_np) {
+		dev_err(dev, "%s: Prop: %s contains invalid opp desc phandle\n",
+			__func__, prop->name);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return opp_np;
+}
+
+/* Returns opp descriptor node for a device. Caller must do of_node_put() */
+struct device_node *of_get_opp_desc_node(struct device *dev)
+{
+	const struct property *prop;
+
+	prop = of_find_property(dev->of_node, "operating-points-v2", NULL);
+	if (!prop)
+		return ERR_PTR(-ENODEV);
+	if (!prop->value)
+		return ERR_PTR(-ENODATA);
+
+	/*
+	 * There should be only ONE phandle present in "operating-points-v2"
+	 * property.
+	 */
+	if (prop->length != sizeof(__be32)) {
+		dev_err(dev, "%s: Invalid opp desc phandle\n", __func__);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return _of_get_opp_desc_node_from_prop(dev, prop);
+}
+EXPORT_SYMBOL_GPL(of_get_opp_desc_node);
+
+/* Initializes OPP tables based on new bindings */
+static int _of_init_opp_table_v2(struct device *dev,
+				 const struct property *prop)
+{
+	struct device_node *opp_np, *np;
+	int ret = 0, count = 0;
+
+	/* Get opp node */
+	opp_np = _of_get_opp_desc_node_from_prop(dev, prop);
+	if (IS_ERR(opp_np))
+		return PTR_ERR(opp_np);
+
+	/* We have opp-list node now, iterate over it and add OPPs */
+	for_each_available_child_of_node(opp_np, np) {
+		count++;
+
+		ret = _opp_add_dynamic_v2(dev, np, false);
+		if (ret) {
+			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
+				ret);
+			break;
+		}
+	}
+
+	/* There should be one of more OPP defined */
+	if (WARN_ON(!count))
+		goto put_opp_np;
+
+	if (ret)
+		of_free_opp_table(dev);
+	else
+		_opp_fill_opp_next(dev);
+
+put_opp_np:
+	of_node_put(opp_np);
+
+	return ret;
+}
+
+/* Initializes OPP tables based on old-deprecated bindings */
+static int _of_init_opp_table_v1(struct device *dev)
 {
 	const struct property *prop;
 	const __be32 *val;
@@ -908,6 +1156,50 @@ int of_init_opp_table(struct device *dev)
 
 	return 0;
 }
+
+/**
+ * of_init_opp_table() - Initialize opp table from device tree
+ * @dev:	device pointer used to lookup device OPPs.
+ *
+ * Register the initial OPP table with the OPP library for given device.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function indirectly uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ *
+ * Return:
+ * 0		On success OR
+ *		Duplicate OPPs (both freq and volt are same) and opp->available
+ * -EEXIST	Freq are same and volt are different OR
+ *		Duplicate OPPs (both freq and volt are same) and !opp->available
+ * -ENOMEM	Memory allocation failure
+ * -ENODEV	when 'operating-points' property is not found or is invalid data
+ *		in device node.
+ * -ENODATA	when empty 'operating-points' property is found
+ */
+int of_init_opp_table(struct device *dev)
+{
+	const struct property *prop;
+
+	/*
+	 * OPPs have two version of bindings now. The older one is deprecated,
+	 * try for the new binding first.
+	 */
+	prop = of_find_property(dev->of_node, "operating-points-v2", NULL);
+	if (!prop) {
+		/*
+		 * Try old-deprecated bindings for backward compatibility with
+		 * older dtbs.
+		 */
+		return _of_init_opp_table_v1(dev);
+	}
+
+	if (!prop->value)
+		return -ENODATA;
+	return _of_init_opp_table_v2(dev, prop);
+}
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 
 /**
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index cec2d4540914..9949d07a93f9 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -115,6 +115,7 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
 int of_init_opp_table(struct device *dev);
 void of_free_opp_table(struct device *dev);
+struct device_node *of_get_opp_desc_node(struct device *dev);
 #else
 static inline int of_init_opp_table(struct device *dev)
 {
@@ -124,6 +125,11 @@ static inline int of_init_opp_table(struct device *dev)
 static inline void of_free_opp_table(struct device *dev)
 {
 }
+
+static inline struct device_node *of_get_opp_desc_node(struct device *dev)
+{
+	return ERR_PTR(-EINVAL);
+}
 #endif
 
 #endif		/* __LINUX_OPP_H__ */
-- 
2.3.0.rc0.44.ga94655d

[PATCH 6/7] opp: Add helpers for initializing CPU opps

[Viresh Kumar]

This provides helpers to find which CPUs share OPPs and initialize OPPs for
them.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h   |  17 +++++++
 2 files changed, 130 insertions(+)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 24a014b7a68a..751f56f323bf 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -11,6 +11,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/cpu.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/err.h>
@@ -1321,6 +1322,44 @@ int of_init_opp_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 
+int of_cpumask_init_opp_table(cpumask_var_t cpumask)
+{
+	struct device *cpu_dev;
+	int cpu, tcpu, ret = 0;
+
+	if (cpumask_empty(cpumask))
+		return -EINVAL;
+
+	for_each_cpu(cpu, cpumask) {
+		cpu_dev = get_cpu_device(cpu);
+		if (!cpu_dev) {
+			pr_err("%s: failed to get cpu%d device\n", __func__,
+			       cpu);
+			continue;
+		}
+
+		ret = of_init_opp_table(cpu_dev);
+		if (ret) {
+			pr_err("%s: couldn't find opp table for cpu:%d, %d\n",
+			       __func__, cpu, ret);
+
+			/* Free all other OPPs */
+			for_each_cpu(tcpu, cpumask) {
+				if (cpu == tcpu)
+					break;
+
+				cpu_dev = get_cpu_device(tcpu);
+				if (cpu_dev)
+					of_free_opp_table(cpu_dev);
+			}
+			break;
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_cpumask_init_opp_table);
+
 /**
  * of_free_opp_table() - Free OPP table entries created from static DT entries
  * @dev:	device pointer used to lookup device OPPs.
@@ -1368,4 +1407,78 @@ void of_free_opp_table(struct device *dev)
 	mutex_unlock(&dev_opp_list_lock);
 }
 EXPORT_SYMBOL_GPL(of_free_opp_table);
+
+void of_cpumask_free_opp_table(cpumask_var_t cpumask)
+{
+	struct device *cpu_dev;
+	int cpu;
+
+	WARN_ON(cpumask_empty(cpumask));
+
+	for_each_cpu(cpu, cpumask) {
+		cpu_dev = get_cpu_device(cpu);
+		if (!cpu_dev) {
+			pr_err("%s: failed to get cpu%d device\n", __func__,
+			       cpu);
+			continue;
+		}
+
+		of_free_opp_table(cpu_dev);
+	}
+}
+EXPORT_SYMBOL_GPL(of_cpumask_free_opp_table);
+
+/* Works only for OPP v2 bindings */
+int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
+{
+	struct device_node *np, *tmp_np;
+	struct device *tcpu_dev;
+	int cpu, ret = 0;
+
+	cpumask_set_cpu(cpu_dev->id, cpumask);
+
+	/* Get OPP descriptor node */
+	np = of_get_opp_desc_node(cpu_dev);
+	if (IS_ERR(np)) {
+		dev_info(cpu_dev, "%s: Couldn't find opp node: %ld\n", __func__,
+			 PTR_ERR(np));
+		return -EINVAL;
+	}
+
+	/* OPPs are shared ? */
+	if (!of_get_property(np, "shared-opp", NULL))
+		goto put_cpu_node;
+
+	for_each_possible_cpu(cpu) {
+		if (cpu == cpu_dev->id)
+			continue;
+
+		tcpu_dev = get_cpu_device(cpu);
+		if (!tcpu_dev) {
+			pr_err("failed to get cpu%d device\n", cpu);
+			ret = -ENODEV;
+			goto put_cpu_node;
+		}
+
+		/* Get OPP descriptor node */
+		tmp_np = of_get_opp_desc_node(tcpu_dev);
+		if (IS_ERR(tmp_np)) {
+			dev_info(tcpu_dev, "%s: Couldn't find opp node: %ld\n",
+				 __func__, PTR_ERR(np));
+			ret = -EINVAL;
+			goto put_cpu_node;
+		}
+
+		/* CPUs are sharing opp node */
+		if (np == tmp_np)
+			cpumask_set_cpu(cpu, cpumask);
+
+		of_node_put(tmp_np);
+	}
+
+put_cpu_node:
+	of_node_put(np);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_get_cpus_sharing_opps);
 #endif
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 9949d07a93f9..41cbc7469b5a 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -115,6 +115,9 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
 int of_init_opp_table(struct device *dev);
 void of_free_opp_table(struct device *dev);
+int of_cpumask_init_opp_table(cpumask_var_t cpumask);
+void of_cpumask_free_opp_table(cpumask_var_t cpumask);
+int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask);
 struct device_node *of_get_opp_desc_node(struct device *dev);
 #else
 static inline int of_init_opp_table(struct device *dev)
@@ -126,6 +129,20 @@ static inline void of_free_opp_table(struct device *dev)
 {
 }
 
+static inline int of_cpumask_init_opp_table(cpumask_var_t cpumask)
+{
+	return -EINVAL;
+}
+
+static inline void of_cpumask_free_opp_table(cpumask_var_t cpumask)
+{
+}
+
+static inline int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
+{
+	return -EINVAL;
+}
+
 static inline struct device_node *of_get_opp_desc_node(struct device *dev)
 {
 	return ERR_PTR(-EINVAL);
-- 
2.3.0.rc0.44.ga94655d

[PATCH 7/7] cpufreq-dt: Use DT to set policy->cpus/related_cpus

[Viresh Kumar]

This updates cpufreq-dt driver to read clock sharing information from new OPP
bindings. And then initialize OPPs for CPUs with help of new bindings.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index bab67db54b7e..6f307897e17a 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -208,8 +208,14 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		goto out_put_reg_clk;
 	}
 
+	pd = cpufreq_get_driver_data();
+	if (!pd || !pd->independent_clocks) {
+		if (of_get_cpus_sharing_opps(cpu_dev, policy->cpus))
+			cpumask_setall(policy->cpus);
+	}
+
 	/* OPPs might be populated at runtime, don't check for error here */
-	of_init_opp_table(cpu_dev);
+	of_cpumask_init_opp_table(policy->cpus);
 
 	/*
 	 * But we need OPP table to function so if it is not there let's
@@ -293,10 +299,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 
 	policy->cpuinfo.transition_latency = transition_latency;
 
-	pd = cpufreq_get_driver_data();
-	if (!pd || !pd->independent_clocks)
-		cpumask_setall(policy->cpus);
-
 	of_node_put(np);
 
 	return 0;
@@ -306,7 +308,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 out_free_priv:
 	kfree(priv);
 out_free_opp:
-	of_free_opp_table(cpu_dev);
+	of_cpumask_free_opp_table(policy->cpus);
 	of_node_put(np);
 out_put_reg_clk:
 	clk_put(cpu_clk);
@@ -322,7 +324,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 
 	cpufreq_cooling_unregister(priv->cdev);
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
-	of_free_opp_table(priv->cpu_dev);
+	of_cpumask_free_opp_table(policy->related_cpus);
 	clk_put(policy->clk);
 	if (!IS_ERR(priv->cpu_reg))
 		regulator_put(priv->cpu_reg);
-- 
2.3.0.rc0.44.ga94655d

Re: [PATCH 0/7] OPP: Introduce OPP bindings V2 and supporting code

[Stephen Boyd]

On 02/11, Viresh Kumar wrote:
> Now that I have received an verbal Ack from Rob Herring (in a personal
> conversation) about the bindings, I am showing how the code looks like with
> these new bindings.
> 
> Some part is still now done:
> - Interface for adding new detailed OPPs from platform code instead of DT
> - Providing cpufreq helpers for the next OPPs
> - Providing regulator helpers for the target/min/max ranges
> 
> Please provide feedback on how this looks like..
> 

Here's some feedback on how we can't use OPPs (and OPPs in DT) on
qcom platforms.

On these platforms the OPPs are not always frequency voltage
pairs. Sometimes they're a frequency voltage voltage triplet, or
frequency voltage current triplet. I know that the OPP framework
in the kernel doesn't support more than frequency voltage pairs,
but that doesn't mean it can't in the future, and if it needs to
do so the DT bindings shouldn't need a v3 revision.

Furthermore, we have a large number of OPP sets that apply to
different speed bins and silicon characteristics of the SoC. In
our systems we read some efuses (an eeprom of sorts) that tell us
to use a certain set of OPPs because the silicon is so fast or
has these certain characteristics. The bootloader is *not*
reading these fuses and populating OPPs in DT. So right now we
just put all these custom OPPish tables in DT and then pick the
right one based on a node name match constructed from the bits we
read in the efuses. How can we express this in DT with these
bindings?

For example, on msm8974 we have a frequency voltage current
triplet and there are 18 different sets of these triplets, each
with 23 triplets per set. We could encode all of these tables as
a bunch of nodes with compatible = "operating-points-v2" but how
do we pick the right one to assign and populate for the CPU
devices? Do we need some sort of opp-fuse-map table that encodes
the information we want somewhere?

 qcom,opp-fuse-map = <speedX binY versionZ &oppN>,
                     ...

but where do we put it? In the cpus node? Or maybe we can keep
doing the node name matching? That would required dropping the
oppN convention.

Or take msm8916 as another example. On this device the voltage
for a few frequencies comes from the efuses and then we
interpolate the rest of the frequency voltage pairs. The speed
bins are picked from another set of efuses so we can do the
interpolation. Unfortunately we don't encode the frequency in the
fuses, so we rely on a handful of tables being defined somewhere
so that we know speed bin 0 means this set of frequencies and
speed bin 1 means this set of frequencies. How do we encode this
in DT?  Should we have the frequencies as OPPs and leave the
voltage part out, filling it in at runtime based on what we read
out of the efuses? I assume it's desirable to have the frequency
tables in DT but we could also have them in the driver and if we
did that there wouldn't be any shared-opp property to set and
have the cpufreq-dt driver use to figure out clock sharing.

Also sometimes we need to correlate OPPs between each other. For
example on msm8960/apq8064 if the CPU is running at a frequency
and voltage, the L2 needs to be running at another frequency,
voltage, and voltage (triplet). The L2 is in two power domains
but it only has one clock. Can/should this be expressed in DT? It
certainly seems that it's at least easier to add it on as a
feature because OPPs are nodes instead of an array. But we need
to make sure we can support multiple regulators somehow, either
through correlated OPPs and multiple OPPs for a single device or
by being able to say opp-0-microvolt, opp-1-microvolt. I would
guess something similar could happen if there were two clocks and
one regulator although I've never seen such a scenario in
practice.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 0/7] OPP: Introduce OPP bindings V2 and supporting code

[Viresh Kumar]

On 12 February 2015 at 08:52, Stephen Boyd <sboyd@codeaurora.org> wrote:
> Here's some feedback on how we can't use OPPs (and OPPs in DT) on
> qcom platforms.
>
> On these platforms the OPPs are not always frequency voltage
> pairs. Sometimes they're a frequency voltage voltage triplet, or

So, making opp-microvolt an array of <target/min/max>, values should fix this?
Do we also need a opp-microvolt-names array as well? or can we reused
similar ones from the CPU node where regulator are defined.

> frequency voltage current triplet. I know that the OPP framework

so we do need opp-milliamp here..

> in the kernel doesn't support more than frequency voltage pairs,
> but that doesn't mean it can't in the future, and if it needs to
> do so the DT bindings shouldn't need a v3 revision.

Sure.

> Furthermore, we have a large number of OPP sets that apply to
> different speed bins and silicon characteristics of the SoC. In
> our systems we read some efuses (an eeprom of sorts) that tell us
> to use a certain set of OPPs because the silicon is so fast or
> has these certain characteristics. The bootloader is *not*
> reading these fuses and populating OPPs in DT. So right now we
> just put all these custom OPPish tables in DT and then pick the
> right one based on a node name match constructed from the bits we
> read in the efuses. How can we express this in DT with these
> bindings?

What about keeping things as is in DT and disabling the OPPs which
you don't support at boot? And only keep enabled the set of OPPs
that you want to use based on these efuses ?

> For example, on msm8974 we have a frequency voltage current
> triplet and there are 18 different sets of these triplets, each
> with 23 triplets per set. We could encode all of these tables as
> a bunch of nodes with compatible = "operating-points-v2" but how
> do we pick the right one to assign and populate for the CPU
> devices? Do we need some sort of opp-fuse-map table that encodes
> the information we want somewhere?

Probably add all these groups in a single OPP descriptor and
enable/disable them at runtime..

>  qcom,opp-fuse-map = <speedX binY versionZ &oppN>,
>                      ...
>
> but where do we put it? In the cpus node? Or maybe we can keep
> doing the node name matching? That would required dropping the
> oppN convention.

Probably this might not be required then ?

> Or take msm8916 as another example. On this device the voltage
> for a few frequencies comes from the efuses and then we
> interpolate the rest of the frequency voltage pairs. The speed
> bins are picked from another set of efuses so we can do the
> interpolation. Unfortunately we don't encode the frequency in the
> fuses, so we rely on a handful of tables being defined somewhere
> so that we know speed bin 0 means this set of frequencies and
> speed bin 1 means this set of frequencies. How do we encode this
> in DT?  Should we have the frequencies as OPPs and leave the
> voltage part out, filling it in at runtime based on what we read

Maybe yes.

> out of the efuses? I assume it's desirable to have the frequency
> tables in DT but we could also have them in the driver and if we
> did that there wouldn't be any shared-opp property to set and
> have the cpufreq-dt driver use to figure out clock sharing.

Probably better keep them in DT. But for platforms with dynamic OPPs
only, we will surely provide some API to make OPPs shared at runtime
too..

> Also sometimes we need to correlate OPPs between each other. For
> example on msm8960/apq8064 if the CPU is running at a frequency
> and voltage, the L2 needs to be running at another frequency,
> voltage, and voltage (triplet). The L2 is in two power domains
> but it only has one clock. Can/should this be expressed in DT? It

Not sure.

> certainly seems that it's at least easier to add it on as a
> feature because OPPs are nodes instead of an array. But we need

It wouldn't be a great idea to make these nodes too large just to support
some corner cases, and we better try to find the best way out. But if
we need it this way, we need it this way :)

> to make sure we can support multiple regulators somehow, either
> through correlated OPPs and multiple OPPs for a single device or
> by being able to say opp-0-microvolt, opp-1-microvolt. I would

an array would be better.

> guess something similar could happen if there were two clocks and
> one regulator although I've never seen such a scenario in
> practice.

Isn't this common? A single regulator voltage supporting multiple clock
rates? Or did I misunderstood it :)

We can have separate OPP nodes in this case.

Re: [PATCH 0/7] OPP: Introduce OPP bindings V2 and supporting code

[Stephen Boyd]

On 02/12, Viresh Kumar wrote:
> On 12 February 2015 at 08:52, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > Here's some feedback on how we can't use OPPs (and OPPs in DT) on
> > qcom platforms.
> >
> > On these platforms the OPPs are not always frequency voltage
> > pairs. Sometimes they're a frequency voltage voltage triplet, or
> 
> So, making opp-microvolt an array of <target/min/max>, values should fix this?
> Do we also need a opp-microvolt-names array as well? or can we reused
> similar ones from the CPU node where regulator are defined.
> 

I don't follow how target/min/max does anything for two different
voltages. I suppose something like opp-microvolt-names would work
though but I don't know how software would correlate that to the
regulator it uses because that information is elsewhere in the
device's node. Why not put the information about which clock and
regulator is used into the opp node?

> 
> > Furthermore, we have a large number of OPP sets that apply to
> > different speed bins and silicon characteristics of the SoC. In
> > our systems we read some efuses (an eeprom of sorts) that tell us
> > to use a certain set of OPPs because the silicon is so fast or
> > has these certain characteristics. The bootloader is *not*
> > reading these fuses and populating OPPs in DT. So right now we
> > just put all these custom OPPish tables in DT and then pick the
> > right one based on a node name match constructed from the bits we
> > read in the efuses. How can we express this in DT with these
> > bindings?
> 
> What about keeping things as is in DT and disabling the OPPs which
> you don't support at boot? And only keep enabled the set of OPPs
> that you want to use based on these efuses ?

Let's look at an example:


	speed0 bin0 version0 = /* Hz      uV  uA */
                        <  300000000  815000  73 >,
                        <  345600000  825000  85 >,
                        <  422400000  835000 104 >,
			....

	speed0 bin1 version0 =
                        <  300000000  800000  73 >,
                        <  345600000  810000  85 >,
                        <  422400000  820000 104 >,
			...

Each set of fuses has the exact same frequency (as long as the
speed is the same) but the bin changes the voltage and sometimes
current. Maybe we could make this into:

 speed0_bin0_version0_opp: opp0 {
	 compatible = "qcom,speed0-bin0-version0-opp", "operating-points-v2";
	 entry0 {
		 opp-khz = <300000>;
		 opp-microvolt = <815000>;
		 opp-milliamp = <73>;
	 };
	 
	 entry1 {
		 opp-khz = <345600>;
		 opp-microvolt = <825000>;
		 opp-milliamp = <85>;
	 };
	 ...
 };
 
 speed0_bin1_version0_opp: opp0 {
	 compatible = "qcom,speed0-bin1-version0-opp", "operating-points-v2";
	 entry0 {
		 opp-khz = <300000>;
		 opp-microvolt = <800000>;
		 opp-milliamp = <73>;
	 };
	 
	 entry1 {
		 opp-khz = <345600>;
		 opp-microvolt = <810000>;
		 opp-milliamp = <85>;
	 };
	 ...
 };

And then we can construct a compatible string to search the cpus
node for. I wonder if we shouldn't put all this into an opps node
under the cpus node?

 cpus {
  opps { 
   opp0 {
     entry0 { }
     ...
   }
   opp1 {
    entry0 { }
     ...
   }
  }
 }


> 
> > Or take msm8916 as another example. On this device the voltage
> > for a few frequencies comes from the efuses and then we
> > interpolate the rest of the frequency voltage pairs. The speed
> > bins are picked from another set of efuses so we can do the
> > interpolation. Unfortunately we don't encode the frequency in the
> > fuses, so we rely on a handful of tables being defined somewhere
> > so that we know speed bin 0 means this set of frequencies and
> > speed bin 1 means this set of frequencies. How do we encode this
> > in DT?  Should we have the frequencies as OPPs and leave the
> > voltage part out, filling it in at runtime based on what we read
> 
> Maybe yes.
> 
> > out of the efuses? I assume it's desirable to have the frequency
> > tables in DT but we could also have them in the driver and if we
> > did that there wouldn't be any shared-opp property to set and
> > have the cpufreq-dt driver use to figure out clock sharing.
> 
> Probably better keep them in DT. But for platforms with dynamic OPPs
> only, we will surely provide some API to make OPPs shared at runtime
> too..

That would be good. Hopefully we can do that so that the
cpufreq-dt driver doesn't have to open code DT probing to figure
out that the OPPs are shared between CPUs.

> 
> > Also sometimes we need to correlate OPPs between each other. For
> > example on msm8960/apq8064 if the CPU is running at a frequency
> > and voltage, the L2 needs to be running at another frequency,
> > voltage, and voltage (triplet). The L2 is in two power domains
> > but it only has one clock. Can/should this be expressed in DT? It
> 
> Not sure.
> 
> > certainly seems that it's at least easier to add it on as a
> > feature because OPPs are nodes instead of an array. But we need
> 
> It wouldn't be a great idea to make these nodes too large just to support
> some corner cases, and we better try to find the best way out. But if
> we need it this way, we need it this way :)
> 
> > to make sure we can support multiple regulators somehow, either
> > through correlated OPPs and multiple OPPs for a single device or
> > by being able to say opp-0-microvolt, opp-1-microvolt. I would
> 
> an array would be better.

Sure.

> 
> > guess something similar could happen if there were two clocks and
> > one regulator although I've never seen such a scenario in
> > practice.
> 
> Isn't this common? A single regulator voltage supporting multiple clock
> rates? Or did I misunderstood it :)
> 
> We can have separate OPP nodes in this case.
> 

I was thinking the same device has two clocks that share the same
voltage and these two clocks can run at different rates. If two
opp nodes under the same device works then it sounds like it will
be ok. We probably still need some way to correlate the two so
that if one clock is at some freq, the other clock should be at
the corresponding freq in the OPP, assuming that the OPP is
really two clocks and a voltage (i.e. the clocks need to scale
together).

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 0/7] OPP: Introduce OPP bindings V2 and supporting code

[Viresh Kumar]

On 12 February 2015 at 16:20, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 02/12, Viresh Kumar wrote:
>> On 12 February 2015 at 08:52, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > Here's some feedback on how we can't use OPPs (and OPPs in DT) on
>> > qcom platforms.
>> >
>> > On these platforms the OPPs are not always frequency voltage
>> > pairs. Sometimes they're a frequency voltage voltage triplet, or
>>
>> So, making opp-microvolt an array of <target/min/max>, values should fix this?
>> Do we also need a opp-microvolt-names array as well? or can we reused
>> similar ones from the CPU node where regulator are defined.
>>
>
> I don't follow how target/min/max does anything for two different

<target/min/max> represents a single regulators voltage levels for any OPP, this
replaces the existing target+voltage-tolerance stuff..

Now to support multiple regulators, we can have an array here.

> voltages. I suppose something like opp-microvolt-names would work
> though but I don't know how software would correlate that to the
> regulator it uses because that information is elsewhere in the

I hope the CPU node will have an array of supplies to support multiple
regulators, right? If yes, then we can just keep the array of <t/min/max>
in the same order. And software should be able to correlate then?

> device's node. Why not put the information about which clock and
> regulator is used into the opp node?

Don't know. I have been asked specifically to keem them out of the OPPs,
as they belong to the CPU or device instead.

>> > Furthermore, we have a large number of OPP sets that apply to
>> > different speed bins and silicon characteristics of the SoC. In
>> > our systems we read some efuses (an eeprom of sorts) that tell us
>> > to use a certain set of OPPs because the silicon is so fast or
>> > has these certain characteristics. The bootloader is *not*
>> > reading these fuses and populating OPPs in DT. So right now we
>> > just put all these custom OPPish tables in DT and then pick the
>> > right one based on a node name match constructed from the bits we
>> > read in the efuses. How can we express this in DT with these
>> > bindings?
>>
>> What about keeping things as is in DT and disabling the OPPs which
>> you don't support at boot? And only keep enabled the set of OPPs
>> that you want to use based on these efuses ?
>
> Let's look at an example:
>
>
>         speed0 bin0 version0 = /* Hz      uV  uA */
>                         <  300000000  815000  73 >,
>                         <  345600000  825000  85 >,
>                         <  422400000  835000 104 >,
>                         ....
>
>         speed0 bin1 version0 =
>                         <  300000000  800000  73 >,
>                         <  345600000  810000  85 >,
>                         <  422400000  820000 104 >,
>                         ...
>
> Each set of fuses has the exact same frequency (as long as the
> speed is the same) but the bin changes the voltage and sometimes
> current. Maybe we could make this into:
>
>  speed0_bin0_version0_opp: opp0 {
>          compatible = "qcom,speed0-bin0-version0-opp", "operating-points-v2";
>          entry0 {
>                  opp-khz = <300000>;
>                  opp-microvolt = <815000>;
>                  opp-milliamp = <73>;
>          };
>
>          entry1 {
>                  opp-khz = <345600>;
>                  opp-microvolt = <825000>;
>                  opp-milliamp = <85>;
>          };
>          ...
>  };
>
>  speed0_bin1_version0_opp: opp0 {
>          compatible = "qcom,speed0-bin1-version0-opp", "operating-points-v2";
>          entry0 {
>                  opp-khz = <300000>;
>                  opp-microvolt = <800000>;
>                  opp-milliamp = <73>;
>          };
>
>          entry1 {
>                  opp-khz = <345600>;
>                  opp-microvolt = <810000>;
>                  opp-milliamp = <85>;
>          };
>          ...
>  };
>
> And then we can construct a compatible string to search the cpus

So we can make:

                       operating-points-v2 = <&cpu0_opp>;

an array instead, and that btw is expandable in future as well. So we may
leave it as is right now, and update it later.


> node for. I wonder if we shouldn't put all this into an opps node
> under the cpus node?

Nodes in 'cpus' node are thought of as CPUs and so was asked to put
this out at the top.

>> > guess something similar could happen if there were two clocks and
>> > one regulator although I've never seen such a scenario in
>> > practice.
>>
>> Isn't this common? A single regulator voltage supporting multiple clock
>> rates? Or did I misunderstood it :)
>>
>> We can have separate OPP nodes in this case.
>>
>
> I was thinking the same device has two clocks that share the same
> voltage and these two clocks can run at different rates. If two
> opp nodes under the same device works then it sounds like it will
> be ok. We probably still need some way to correlate the two so
> that if one clock is at some freq, the other clock should be at
> the corresponding freq in the OPP, assuming that the OPP is
> really two clocks and a voltage (i.e. the clocks need to scale
> together).

Oh, I got it wrong. So what this requires is an array of clock-rates
like what I am proposing for regulators.

Over that we can make these entries arrays later on as well, as this
wouldn't break anything. So, keep life simple for now.

Re: [PATCH 0/7] OPP: Introduce OPP bindings V2 and supporting code

[Mark Brown]

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

On Tue, Feb 17, 2015 at 01:16:25PM +0530, Viresh Kumar wrote:
> On 12 February 2015 at 16:20, Stephen Boyd <sboyd@codeaurora.org> wrote:

> > voltages. I suppose something like opp-microvolt-names would work
> > though but I don't know how software would correlate that to the
> > regulator it uses because that information is elsewhere in the

> I hope the CPU node will have an array of supplies to support multiple
> regulators, right? If yes, then we can just keep the array of <t/min/max>
> in the same order. And software should be able to correlate then?

It's probably not *too* bad for small numbers of supplies but bindings
that require that indexes into two different arrays match up
(particularly with different step sizes as here) do have legibility
issues - it's easy to miscount when reading things.

> > device's node. Why not put the information about which clock and
> > regulator is used into the opp node?

> Don't know. I have been asked specifically to keem them out of the OPPs,
> as they belong to the CPU or device instead.

The DT should describe the hardware and how it's connected.  No power
supplies or clocks are connected to an OPP, an OPP is a property of
something else.

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

[PATCH 5/7] opp: convert device_opp->dev to a list of devices

[Viresh Kumar]

An opp can be shared by multiple devices, for example its very common for CPUs
to share the OPPs, i.e. when they share clock lines.

Until now, the OPPs are bound to a single device and there is no way to mark
them shared.

This patch adds support to manage a list of devices sharing OPPs. It also senses
if the device (we are trying to initialize OPPs for) shares OPPs with an device
added earlier and in that case we just update the list of devices managed by
OPPs instead of initializing them again.

Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/base/power/opp.c | 189 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 157 insertions(+), 32 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index b7b9c33fbb65..24a014b7a68a 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -89,16 +89,33 @@ struct dev_pm_opp {
 };
 
 /**
+ * struct device_list_opp - devices managed by 'struct device_opp'
+ * @node:	list node
+ * @dev:	device to which the struct object belongs
+ * @rcu_head:	RCU callback head used for deferred freeing
+ *
+ * This is an internal data structure maintaining the list of devices that are
+ * managed by 'struct device_opp'.
+ */
+struct device_list_opp {
+	struct list_head node;
+	struct device *dev;
+	struct rcu_head rcu_head;
+};
+
+/**
  * struct device_opp - Device opp structure
  * @node:	list node - contains the devices with OPPs that
  *		have been registered. Nodes once added are not modified in this
  *		list.
  *		RCU usage: nodes are not modified in the list of device_opp,
  *		however addition is possible and is secured by dev_opp_list_lock
- * @dev:	device pointer
  * @srcu_head:	notifier head to notify the OPP availability changes.
  * @rcu_head:	RCU callback head used for deferred freeing
+ * @dev_list:	list of devices that share these OPPs
  * @opp_list:	list of opps
+ * @np:		struct device_node pointer for opp's DT node.
+ * @shared_opp: OPP is shared between multiple devices.
  *
  * This is an internal data structure maintaining the link to opps attached to
  * a device. This structure is not meant to be shared to users as it is
@@ -111,10 +128,12 @@ struct dev_pm_opp {
 struct device_opp {
 	struct list_head node;
 
-	struct device *dev;
 	struct srcu_notifier_head srcu_head;
 	struct rcu_head rcu_head;
+	struct list_head dev_list;
 	struct list_head opp_list;
+	struct device_node *np;
+	bool shared_opp;
 };
 
 /*
@@ -134,6 +153,45 @@ do {									\
 			   "dev_opp_list_lock protection");		\
 } while (0)
 
+static struct device_list_opp *_find_list_dev(struct device *dev,
+					      struct device_opp *dev_opp)
+{
+	struct device_list_opp *list_dev;
+
+	list_for_each_entry(list_dev, &dev_opp->dev_list, node)
+		if (list_dev->dev == dev)
+			return list_dev;
+
+	return NULL;
+}
+
+static int _list_dev_count(struct device_opp *dev_opp)
+{
+	struct device_list_opp *list_dev;
+	int count = 0;
+
+	list_for_each_entry(list_dev, &dev_opp->dev_list, node)
+		count++;
+
+	return count;
+}
+
+static struct device_opp *_managed_opp(struct device_node *np)
+{
+	struct device_opp *dev_opp;
+
+	list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
+		if (dev_opp->np == np) {
+			/* Managed only if the OPPs are shared */
+			if (dev_opp->shared_opp)
+				return dev_opp;
+			else
+				return NULL;
+		}
+
+	return NULL;
+}
+
 /**
  * _find_device_opp() - find device_opp struct using device pointer
  * @dev:	device pointer used to lookup device OPPs
@@ -150,21 +208,18 @@ do {									\
  */
 static struct device_opp *_find_device_opp(struct device *dev)
 {
-	struct device_opp *tmp_dev_opp, *dev_opp = ERR_PTR(-ENODEV);
+	struct device_opp *dev_opp;
 
 	if (unlikely(IS_ERR_OR_NULL(dev))) {
 		pr_err("%s: Invalid parameters\n", __func__);
 		return ERR_PTR(-EINVAL);
 	}
 
-	list_for_each_entry_rcu(tmp_dev_opp, &dev_opp_list, node) {
-		if (tmp_dev_opp->dev == dev) {
-			dev_opp = tmp_dev_opp;
-			break;
-		}
-	}
+	list_for_each_entry_rcu(dev_opp, &dev_opp_list, node)
+		if (_find_list_dev(dev, dev_opp))
+			return dev_opp;
 
-	return dev_opp;
+	return ERR_PTR(-ENODEV);
 }
 
 /**
@@ -425,6 +480,38 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
+static struct device_list_opp *_add_list_dev(struct device *dev,
+					     struct device_opp *dev_opp)
+{
+	struct device_list_opp *list_dev;
+
+	list_dev = kzalloc(sizeof(*list_dev), GFP_KERNEL);
+	if (!list_dev)
+		return NULL;
+
+	/* Initialize list-dev */
+	list_add_rcu(&list_dev->node, &dev_opp->dev_list);
+	list_dev->dev = dev;
+
+	return list_dev;
+}
+
+static void _kfree_list_dev_rcu(struct rcu_head *head)
+{
+	struct device_list_opp *list_dev;
+
+	list_dev = container_of(head, struct device_list_opp, rcu_head);
+	kfree_rcu(list_dev, rcu_head);
+}
+
+static void _remove_list_dev(struct device_list_opp *list_dev,
+			     struct device_opp *dev_opp)
+{
+	list_del(&list_dev->node);
+	call_srcu(&dev_opp->srcu_head.srcu, &list_dev->rcu_head,
+		  _kfree_list_dev_rcu);
+}
+
 /**
  * _kfree_opp_rcu() - Free OPP RCU handler
  * @head:	RCU head
@@ -478,6 +565,17 @@ static void _opp_remove(struct device_opp *dev_opp,
 
 	/* Free device if all OPPs are freed */
 	if (list_empty(&dev_opp->opp_list)) {
+		struct device_list_opp *list_dev;
+
+		list_dev = list_first_entry(&dev_opp->dev_list,
+					    struct device_list_opp, node);
+
+		_remove_list_dev(list_dev, dev_opp);
+
+		/* dev_list must be empty now */
+		WARN_ON(!list_empty(&dev_opp->dev_list));
+
+		/* Free dev-opp */
 		list_del_rcu(&dev_opp->node);
 		call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,
 			  _kfree_device_rcu);
@@ -541,6 +639,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
 static struct device_opp *_add_device_opp(struct device *dev)
 {
 	struct device_opp *dev_opp;
+	struct device_list_opp *list_dev;
 
 	/*
 	 * Allocate a new device OPP table. In the infrequent case where a new
@@ -550,7 +649,13 @@ static struct device_opp *_add_device_opp(struct device *dev)
 	if (!dev_opp)
 		return NULL;
 
-	dev_opp->dev = dev;
+	INIT_LIST_HEAD(&dev_opp->dev_list);
+	list_dev = _add_list_dev(dev, dev_opp);
+	if (!list_dev) {
+		kfree(dev_opp);
+		return NULL;
+	}
+
 	srcu_init_notifier_head(&dev_opp->srcu_head);
 	INIT_LIST_HEAD(&dev_opp->opp_list);
 
@@ -584,7 +689,8 @@ static struct dev_pm_opp *_allocate_opp(struct device *dev,
 	return opp;
 }
 
-static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp)
+static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
+		    struct device_opp *dev_opp)
 {
 	struct dev_pm_opp *opp = NULL;
 	struct list_head *head = &dev_opp->opp_list;
@@ -602,7 +708,7 @@ static int _opp_add(struct dev_pm_opp *new_opp, struct device_opp *dev_opp)
 
 	/* Duplicate OPPs ? */
 	if (opp && new_opp->rate == opp->rate) {
-		dev_warn(dev_opp->dev,
+		dev_warn(dev,
 			 "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
 			 __func__, opp->rate, opp->u_volt, opp->available,
 			 new_opp->rate, new_opp->u_volt, new_opp->available);
@@ -665,7 +771,7 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
 	new_opp->available = true;
 	new_opp->dynamic = dynamic;
 
-	ret = _opp_add(new_opp, dev_opp);
+	ret = _opp_add(dev, new_opp, dev_opp);
 	if (ret)
 		goto free_opp;
 
@@ -789,7 +895,7 @@ static int _opp_add_dynamic_v2(struct device *dev, struct device_node *np,
 		}
 	}
 
-	ret = _opp_add(new_opp, dev_opp);
+	ret = _opp_add(dev, new_opp, dev_opp);
 	if (ret)
 		goto free_opp;
 
@@ -826,16 +932,11 @@ static struct dev_pm_opp *_get_opp_from_np(struct device_opp *dev_opp,
 	return NULL;
 }
 
-static void _opp_fill_opp_next(struct device *dev)
+static void _opp_fill_opp_next(struct device_opp *dev_opp, struct device *dev)
 {
-	struct device_opp *dev_opp;
 	struct dev_pm_opp *opp;
 	int i;
 
-	dev_opp = _find_device_opp(dev);
-	if (WARN_ON(!dev_opp))
-		return;
-
 	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
 		if (!opp->next)
 			continue;
@@ -1086,6 +1187,7 @@ static int _of_init_opp_table_v2(struct device *dev,
 				 const struct property *prop)
 {
 	struct device_node *opp_np, *np;
+	struct device_opp *dev_opp;
 	int ret = 0, count = 0;
 
 	/* Get opp node */
@@ -1093,6 +1195,14 @@ static int _of_init_opp_table_v2(struct device *dev,
 	if (IS_ERR(opp_np))
 		return PTR_ERR(opp_np);
 
+	dev_opp = _managed_opp(opp_np);
+	if (dev_opp) {
+		/* OPPs are already managed */
+		if (!_add_list_dev(dev, dev_opp))
+			ret = -ENOMEM;
+		goto put_opp_np;
+	}
+
 	/* We have opp-list node now, iterate over it and add OPPs */
 	for_each_available_child_of_node(opp_np, np) {
 		count++;
@@ -1109,10 +1219,19 @@ static int _of_init_opp_table_v2(struct device *dev,
 	if (WARN_ON(!count))
 		goto put_opp_np;
 
-	if (ret)
+	if (ret) {
 		of_free_opp_table(dev);
-	else
-		_opp_fill_opp_next(dev);
+	} else {
+		/* Update np and shared_opp */
+		dev_opp = _find_device_opp(dev);
+		if (WARN_ON(!dev_opp))
+			goto put_opp_np;
+
+		dev_opp->np = opp_np;
+		if (of_get_property(opp_np, "shared-opp", NULL))
+			dev_opp->shared_opp = true;
+		_opp_fill_opp_next(dev_opp, dev);
+	}
 
 put_opp_np:
 	of_node_put(opp_np);
@@ -1219,6 +1338,9 @@ void of_free_opp_table(struct device *dev)
 	struct device_opp *dev_opp;
 	struct dev_pm_opp *opp, *tmp;
 
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
 	/* Check for existing list for 'dev' */
 	dev_opp = _find_device_opp(dev);
 	if (IS_ERR(dev_opp)) {
@@ -1228,18 +1350,21 @@ void of_free_opp_table(struct device *dev)
 			     IS_ERR_OR_NULL(dev) ?
 					"Invalid device" : dev_name(dev),
 			     error);
-		return;
+		goto unlock;
 	}
 
-	/* Hold our list modification lock here */
-	mutex_lock(&dev_opp_list_lock);
-
-	/* Free static OPPs */
-	list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) {
-		if (!opp->dynamic)
-			_opp_remove(dev_opp, opp, true);
+	/* Find if dev_opp manages a single device */
+	if (_list_dev_count(dev_opp) == 1) {
+		/* Free static OPPs */
+		list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) {
+			if (!opp->dynamic)
+				_opp_remove(dev_opp, opp, true);
+		}
+	} else {
+		_remove_list_dev(_find_list_dev(dev, dev_opp), dev_opp);
 	}
 
+unlock:
 	mutex_unlock(&dev_opp_list_lock);
 }
 EXPORT_SYMBOL_GPL(of_free_opp_table);
-- 
2.3.0.rc0.44.ga94655d

--
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 0/7] OPP: Introduce OPP bindings V2 and supporting code

[Viresh Kumar]

On 11 February 2015 at 13:46, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Now that I have received an verbal Ack from Rob Herring (in a personal
> conversation) about the bindings, I am showing how the code looks like with
> these new bindings.
>
> Some part is still now done:

s/now/not

> - Interface for adding new detailed OPPs from platform code instead of DT
> - Providing cpufreq helpers for the next OPPs
> - Providing regulator helpers for the target/min/max ranges
>
> Please provide feedback on how this looks like..

Can we have some reviews of the code changes please ?

--
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 0/7] OPP: Introduce OPP bindings V2 and supporting code

[Viresh Kumar]

On 27 February 2015 at 10:55, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 11 February 2015 at 13:46, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> Now that I have received an verbal Ack from Rob Herring (in a personal
>> conversation) about the bindings, I am showing how the code looks like with
>> these new bindings.
>>
>> Some part is still now done:
>
> s/now/not
>
>> - Interface for adding new detailed OPPs from platform code instead of DT
>> - Providing cpufreq helpers for the next OPPs
>> - Providing regulator helpers for the target/min/max ranges
>>
>> Please provide feedback on how this looks like..
>
> Can we have some reviews of the code changes please ?

Rob/Rafael/Arnd/Nishanth,

Reviews please? This had been hanging since a long time, can we get this merged
in next merge window please?

Bindings are already OK'd by Rob (in personal conversation) and its
all about code
now. Which can be updated later on as well if we find an issue..

@Rob: Can you please Ack whatever stuff you are okay with ?

--
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 0/7] OPP: Introduce OPP bindings V2 and supporting code

[Viresh Kumar]

On 11 February 2015 at 13:46, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Now that I have received an verbal Ack from Rob Herring (in a personal
> conversation) about the bindings, I am showing how the code looks like with
> these new bindings.
>
> Some part is still now done:
> - Interface for adding new detailed OPPs from platform code instead of DT
> - Providing cpufreq helpers for the next OPPs
> - Providing regulator helpers for the target/min/max ranges
>
> Please provide feedback on how this looks like..

@Rafael: Its been two months now since this was posted. Can we get this
merged please? If someone finds issues later, we can go fix them ..

And this *should* not break any existing stuff as this is mostly new stuff..

--
viresh

Re: [PATCH 0/7] OPP: Introduce OPP bindings V2 and supporting code

[Rob Herring]

On Wed, Apr 1, 2015 at 1:22 AM, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 11 February 2015 at 13:46, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> Now that I have received an verbal Ack from Rob Herring (in a personal
>> conversation) about the bindings, I am showing how the code looks like with
>> these new bindings.

I have also told you multiple times I want to see others' inputs and
acks. So yes it looks fine to me, but I am not a user of it.

>> Some part is still now done:
>> - Interface for adding new detailed OPPs from platform code instead of DT
>> - Providing cpufreq helpers for the next OPPs
>> - Providing regulator helpers for the target/min/max ranges
>>
>> Please provide feedback on how this looks like..
>
> @Rafael: Its been two months now since this was posted. Can we get this
> merged please? If someone finds issues later, we can go fix them ..

If there are users waiting for this, then get their acks first. As you
already know, I spoke to Stephen at ELC and he still has concerns
about it. As Qualcomm is probably the most complicated case, it needs
to work for them (or we accept that it can't and they do everything
their own way).

Rob
--
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 0/7] OPP: Introduce OPP bindings V2 and supporting code

[Viresh Kumar]

On 1 April 2015 at 22:13, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> I have also told you multiple times I want to see others' inputs and
> acks. So yes it looks fine to me, but I am not a user of it.

Right :) and so I am trying hard to get those people to review it. Probably
Stephen will respond now and I have also managed to convince Nishanth
to review it :)

>> @Rafael: Its been two months now since this was posted. Can we get this
>> merged please? If someone finds issues later, we can go fix them ..

So, atleast I got the required attention :)

> If there are users waiting for this, then get their acks first. As you

I am one of those for my cpufreq-dt driver. :)

> already know, I spoke to Stephen at ELC and he still has concerns
> about it. As Qualcomm is probably the most complicated case, it needs
> to work for them (or we accept that it can't and they do everything
> their own way).

Yeah, so we can't wait for them for ever. They have to respond now if
they want to use it. Or we go ahead ...
--
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