[PATCH v2 0/5] PM / OPP: updates to enable sharing OPPs info

[PATCH v2 0/5] PM / OPP: updates to enable sharing OPPs info

[Sudeep KarkadaNagesha]

From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>

Hi,

These are few updates to existing device tree binding and the PM/OPP
library to support sharing of OPPs between different device nodes.

Currently all the cpu nodes are parsed until the OPPs are found. This
is essential to support cpuhotplug without having to replicate OPP
information in all the cpu nodes.

However in systems with multiple cpu power domain, its better to have
OPP entry for each cpu. To avoid replication, phandle can be specified
to the node which contains the full OPP information.

The table node containing all the OPPs and device nodes pointing to an
entry into it through phandle might also solve the problem of describing
multiple OPP profiles.

Regards,
Sudeep

Changes v1->v2:
	- Updated binding document as per Mark's suggestions
	- Added of_node_put for phandle as of_parse_phandle increments
	  the refcount
	- Updated user of OPP library(cpufreq/arm_big_little_dt) affected
	  by binding changes

Sudeep KarkadaNagesha (5):
  PM / OPP: extend DT binding to specify phandle of another node for OPP
  PM / OPP: add support to specify phandle of another node for OPP
  PM / OPP: check for existing OPP list when initialising from device
    tree
  cpufreq: arm_big_little_dt: enhance OPP error checking
  cpufreq: arm_big_little_dt: return success if OPP list already exists

 Documentation/devicetree/bindings/power/opp.txt | 161 ++++++++++++++++++++++--
 drivers/base/power/opp.c                        |  21 +++-
 drivers/cpufreq/arm_big_little_dt.c             |   5 +-
 3 files changed, 172 insertions(+), 15 deletions(-)

-- 
1.8.1.2

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

[PATCH v2 1/5] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Sudeep KarkadaNagesha]

From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>

If more than one similar devices share the same operating points(OPPs)
being in the same clock domain, currently we need to replicate the
OPP entries in all the nodes.

This patch extends existing binding by adding a new property named
'operating-points-phandle' to specify the phandle in any device node
pointing to another node which contains the actual OPP tuples.
This helps to avoid replication if multiple devices share the OPPs.

Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
Cc: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/devicetree/bindings/power/opp.txt | 161 ++++++++++++++++++++++--
 1 file changed, 149 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
index 74499e5..f59b878 100644
--- a/Documentation/devicetree/bindings/power/opp.txt
+++ b/Documentation/devicetree/bindings/power/opp.txt
@@ -4,22 +4,159 @@ 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.
 
-Properties:
+Required Properties:
 - operating-points: An array of 2-tuples items, and each item consists
   of frequency and voltage like <freq-kHz vol-uV>.
 	freq: clock frequency in kHz
 	vol: voltage in microvolt
 
+- operating-points-phandle: phandle to the device tree node which contains
+	the operating points tuples(recommended to be used if multiple
+	devices are in the same clock domain and hence share OPPs, as it
+	avoids replication of OPPs)
+
+  operating-points and operating-points-phandle are mutually exclusive, only
+  one of them can be present in any device node.
+
 Examples:
 
-cpu@0 {
-	compatible = "arm,cortex-a9";
-	reg = <0>;
-	next-level-cache = <&L2>;
-	operating-points = <
-		/* kHz    uV */
-		792000  1100000
-		396000  950000
-		198000  850000
-	>;
-};
+1. A uniprocessor system (phandle not required)
+
+	cpu0: cpu@0 {
+		compatible = "arm,cortex-a9";
+		reg = <0>;
+		operating-points = <
+			/* kHz    uV */
+			792000  1100000
+			396000  950000
+			198000  850000
+		>;
+	};
+
+2a. Consider a SMP system with 4 CPUs in the same clock domain(no phandle)
+    Some existing DTs describe homogenous SMP systems by only listing the
+    OPPs in the cpu@0 node. For compatiblity with existing DTs, an
+    operating system may handle this case specially.
+
+	cpu0: cpu@0 {
+		compatible = "arm,cortex-a9";
+		reg = <0>;
+		operating-points = <
+			/* kHz    uV */
+			792000  1100000
+			396000  950000
+			198000  850000
+		>;
+	};
+
+	cpu1: cpu@1 {
+		compatible = "arm,cortex-a9";
+		reg = <1>;
+	};
+
+	cpu2: cpu@2 {
+		compatible = "arm,cortex-a9";
+		reg = <2>;
+	};
+
+	cpu3: cpu@3 {
+		compatible = "arm,cortex-a9";
+		reg = <3>;
+	};
+
+2b. Consider a SMP system with 4 CPUs in the same clock domain(with phandle)
+    If more than one device of same type share the same OPPs, for example
+    all the CPUs on a SoC or in a single cluster on a SoC, then we can avoid
+    replicating the OPPs in all the nodes. We can specify the phandle of
+    the node which contains the OPP tuples instead.
+
+	cpu0: cpu@0 {
+		compatible = "arm,cortex-a9";
+		reg = <0>;
+		operating-points-phandle = <&cpu_opp>;
+	};
+
+	cpu1: cpu@1 {
+		compatible = "arm,cortex-a9";
+		reg = <1>;
+		operating-points-phandle = <&cpu_opp>;
+	};
+
+	cpu2: cpu@2 {
+		compatible = "arm,cortex-a9";
+		reg = <2>;
+		operating-points-phandle = <&cpu_opp>;
+	};
+
+	cpu3: cpu@3 {
+		compatible = "arm,cortex-a9";
+		reg = <3>;
+		operating-points-phandle = <&cpu_opp>;
+	};
+
+	opps-table {
+		cpu_opp: cpu_opp {
+			operating-points = <
+				/* kHz    uV */
+				792000  1100000
+				396000  950000
+				198000  850000
+			>;
+		};
+		... /* other device OPP nodes */
+	}
+
+4. Consider an AMP(asymmetric multi-processor) sytem with 2 clusters of
+   CPUs. Each cluster has 2 CPUs and all the CPUs within the cluster share
+   the clock domain.
+
+	cpu0: cpu@0 {
+		compatible = "arm,cortex-a15";
+		reg = <0>;
+		operating-points-phandle = <&cluster0_opp>;
+	};
+
+	cpu1: cpu@1 {
+		compatible = "arm,cortex-a15";
+		reg = <1>;
+		operating-points-phandle = <&cluster0_opp>;
+	};
+
+	cpu2: cpu@100 {
+		compatible = "arm,cortex-a7";
+		reg = <100>;
+		operating-points-phandle = <&cluster1_opp>;
+	};
+
+	cpu3: cpu@101 {
+		compatible = "arm,cortex-a7";
+		reg = <101>;
+		operating-points-phandle = <&cluster1_opp>;
+	};
+
+	opps-table {
+		cluster0_opp: cluster0_opp {
+			operating-points = <
+				/* kHz    uV */
+				792000  1100000
+				396000  950000
+				198000  850000
+			>;
+		};
+		cluster1_opp: cluster1_opp {
+			operating-points = <
+				/* kHz    uV */
+				792000  950000
+				396000  750000
+				198000  450000
+			>;
+		};
+		... /* other device OPP nodes */
+	}
+
+Container Node
+--------------
+	- It's highly recommended to place all the shared OPPs under single
+	  node for consistency and better readability
+	- It's quite similar to clocks or pinmux container nodes
+	- In the above examples, "opps-table" is the container node
-- 
1.8.1.2

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

[PATCH v2 2/5] PM / OPP: add support to specify phandle of another node for OPP

[Sudeep KarkadaNagesha]

From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>

Currently we need to replicate the OPP entries in all the nodes even
though they share OPPs being in the same clock domain.

Few drivers like cpufreq depend on physical cpu0 node to specify the
OPPs and only that node is referred irrespective of the logical cpu
accessing it. Alternatively to support cpuhotplug path, few drivers
parse all the cpu nodes for OPPs. Instead we can specify the phandle
of the node which contains the OPP tuples.

This patch adds support to the new property 'operating-points-phandle'
which specifies the phandle pointing to another node which contains the
actual OPP tuples.

Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Nishanth Menon <nm@ti.com>
Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
---
 drivers/base/power/opp.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index ef89897..bb6ff64 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -708,12 +708,20 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
 int of_init_opp_table(struct device *dev)
 {
 	const struct property *prop;
+	struct device_node *opp_node;
 	const __be32 *val;
 	int nr;
 
-	prop = of_find_property(dev->of_node, "operating-points", NULL);
-	if (!prop)
+	opp_node = of_parse_phandle(dev->of_node,
+					"operating-points-phandle", 0);
+	if (!opp_node) /* if no OPP phandle, search for OPPs in current node */
+		opp_node = dev->of_node;
+	prop = of_find_property(opp_node, "operating-points", NULL);
+	if (!prop) {
+		dev_warn(dev, "node %s missing operating-points property\n",
+							opp_node->full_name);
 		return -ENODEV;
+	}
 	if (!prop->value)
 		return -ENODATA;
 
@@ -740,6 +748,9 @@ int of_init_opp_table(struct device *dev)
 		nr -= 2;
 	}
 
+	if (opp_node != dev->of_node)
+		of_node_put(opp_node);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
-- 
1.8.1.2

[PATCH v2 3/5] PM / OPP: check for existing OPP list when initialising from device tree

[Sudeep KarkadaNagesha]

From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>

CPUs are registered as devices and their OPPs can be initialised from
the device tree. Whenever CPUs are hotplugged out, the corresponding
cpu devices are not removed. As a result all their OPPs remain intact
even when they are offlined.

But when they are hotplugged back-in, the cpufreq along with other cpu
related subsystem gets re-initialised. Since its almost same as secondary
cpu being brought up, no special consideration is taken in the hotplug
path. This may result in cpufreq trying to initialise the OPPs again though
the cpu device already contains the OPPs.

This patch checks if there exist an OPP list associated with the device,
before attempting to initialise it.

Cc: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
Cc: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
---
 drivers/base/power/opp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index bb6ff64..05b2ebf 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -709,9 +709,15 @@ int of_init_opp_table(struct device *dev)
 {
 	const struct property *prop;
 	struct device_node *opp_node;
+	struct device_opp *dev_opp;
 	const __be32 *val;
 	int nr;
 
+	/* Check for existing list for 'dev' */
+	dev_opp = find_device_opp(dev);
+	if (!IS_ERR(dev_opp))
+		return -EEXIST; /* Device OPP already initialized */
+
 	opp_node = of_parse_phandle(dev->of_node,
 					"operating-points-phandle", 0);
 	if (!opp_node) /* if no OPP phandle, search for OPPs in current node */
-- 
1.8.1.2

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

[PATCH v2 4/5] cpufreq: arm_big_little_dt: enhance OPP error checking

[Sudeep KarkadaNagesha]

From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>

Since the OPPs can be specified either through 'operating-points'
as tuples or through 'operating-points-phandle' as phandle to the
node containing tuples, we need to check for both the properties.

Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
Cc: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
---
 drivers/cpufreq/arm_big_little_dt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/arm_big_little_dt.c b/drivers/cpufreq/arm_big_little_dt.c
index 480c0bd..b03d4fe 100644
--- a/drivers/cpufreq/arm_big_little_dt.c
+++ b/drivers/cpufreq/arm_big_little_dt.c
@@ -35,7 +35,8 @@ static struct device_node *get_cpu_node_with_valid_op(int cpu)
 {
 	struct device_node *np = of_cpu_device_node_get(cpu);
 
-	if (!of_get_property(np, "operating-points", NULL)) {
+	if (!of_get_property(np, "operating-points", NULL) &&
+		!of_get_property(np, "operating-points-phandle", NULL)) {
 		of_node_put(np);
 		np = NULL;
 	}
-- 
1.8.1.2

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

[PATCH v2 5/5] cpufreq: arm_big_little_dt: return success if OPP list already exists

[Sudeep KarkadaNagesha]

From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>

of_init_opp_table returns -EEXIST if an OPP list is already associated
with the device, before attempting to re-initialise it. In such cases,
dt_init_opp_table must return success as OPP table is initialised.

Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
---
 drivers/cpufreq/arm_big_little_dt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cpufreq/arm_big_little_dt.c b/drivers/cpufreq/arm_big_little_dt.c
index b03d4fe..cc5d92e 100644
--- a/drivers/cpufreq/arm_big_little_dt.c
+++ b/drivers/cpufreq/arm_big_little_dt.c
@@ -56,6 +56,8 @@ static int dt_init_opp_table(struct device *cpu_dev)
 	}
 
 	ret = of_init_opp_table(cpu_dev);
+	if (ret == -EEXIST)
+		ret = 0;
 	of_node_put(np);
 
 	return ret;
-- 
1.8.1.2

Re: [PATCH v2 2/5] PM / OPP: add support to specify phandle of another node for OPP

[Sudeep KarkadaNagesha]

On 01/10/13 14:32, Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
> 
> Currently we need to replicate the OPP entries in all the nodes even
> though they share OPPs being in the same clock domain.
> 
> Few drivers like cpufreq depend on physical cpu0 node to specify the
> OPPs and only that node is referred irrespective of the logical cpu
> accessing it. Alternatively to support cpuhotplug path, few drivers
> parse all the cpu nodes for OPPs. Instead we can specify the phandle
> of the node which contains the OPP tuples.
> 
> This patch adds support to the new property 'operating-points-phandle'
> which specifies the phandle pointing to another node which contains the
> actual OPP tuples.
> 
> Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
> Cc: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/base/power/opp.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index ef89897..bb6ff64 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -708,12 +708,20 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
>  int of_init_opp_table(struct device *dev)
>  {
>  	const struct property *prop;
> +	struct device_node *opp_node;
>  	const __be32 *val;
>  	int nr;
>  
> -	prop = of_find_property(dev->of_node, "operating-points", NULL);
> -	if (!prop)
> +	opp_node = of_parse_phandle(dev->of_node,
> +					"operating-points-phandle", 0);
> +	if (!opp_node) /* if no OPP phandle, search for OPPs in current node */
> +		opp_node = dev->of_node;
> +	prop = of_find_property(opp_node, "operating-points", NULL);
> +	if (!prop) {
> +		dev_warn(dev, "node %s missing operating-points property\n",
> +							opp_node->full_name);
>  		return -ENODEV;
> +	}
>  	if (!prop->value)
>  		return -ENODATA;
>  
> @@ -740,6 +748,9 @@ int of_init_opp_table(struct device *dev)
>  		nr -= 2;
>  	}
>  
> +	if (opp_node != dev->of_node)
> +		of_node_put(opp_node);
> +
There are several exit paths on error conditions where we need to do this.
I missed them all. Here is the updated patch:

-->8--

From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>

Currently we need to replicate the OPP entries in all the nodes even
though they share OPPs being in the same clock domain.

Few drivers like cpufreq depend on physical cpu0 node to specify the
OPPs and only that node is referred irrespective of the logical cpu
accessing it. Alternatively to support cpuhotplug path, few drivers
parse all the cpu nodes for OPPs. Instead we can specify the phandle
of the node which contains the OPP tuples.

This patch adds support to the new property 'operating-points-phandle'
which specifies the phandle pointing to another node which contains the
actual OPP tuples.

Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
Cc: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
---
 drivers/base/power/opp.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index ef89897..cd4dbb3 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -708,14 +708,25 @@ struct srcu_notifier_head *opp_get_notifier(struct device
*dev)
 int of_init_opp_table(struct device *dev)
 {
 	const struct property *prop;
+	struct device_node *opp_node;
 	const __be32 *val;
-	int nr;
-
-	prop = of_find_property(dev->of_node, "operating-points", NULL);
-	if (!prop)
-		return -ENODEV;
-	if (!prop->value)
-		return -ENODATA;
+	int nr, ret = 0;
+
+	opp_node = of_parse_phandle(dev->of_node,
+					"operating-points-phandle", 0);
+	if (!opp_node) /* if no OPP phandle, search for OPPs in current node */
+		opp_node = dev->of_node;
+	prop = of_find_property(opp_node, "operating-points", NULL);
+	if (!prop) {
+		dev_warn(dev, "node %s missing operating-points property\n",
+							opp_node->full_name);
+		ret = -ENODEV;
+		goto put_node;
+	}
+	if (!prop->value) {
+		ret = -ENODATA;
+		goto put_node;
+	}

 	/*
 	 * Each OPP is a set of tuples consisting of frequency and
@@ -724,7 +735,8 @@ int of_init_opp_table(struct device *dev)
 	nr = prop->length / sizeof(u32);
 	if (nr % 2) {
 		dev_err(dev, "%s: Invalid OPP list\n", __func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto put_node;
 	}

 	val = prop->value;
@@ -740,7 +752,11 @@ int of_init_opp_table(struct device *dev)
 		nr -= 2;
 	}

-	return 0;
+put_node:
+	if (opp_node != dev->of_node)
+		of_node_put(opp_node);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 #endif
-- 
1.8.1.2






--
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 v2 5/5] cpufreq: arm_big_little_dt: return success if OPP list already exists

[Viresh Kumar]

On 1 October 2013 19:03, Sudeep KarkadaNagesha
<Sudeep.KarkadaNagesha-5wv7dgnIgG8@public.gmane.org> wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
>
> of_init_opp_table returns -EEXIST if an OPP list is already associated
> with the device, before attempting to re-initialise it. In such cases,
> dt_init_opp_table must return success as OPP table is initialised.
>
> Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
> Cc: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/cpufreq/arm_big_little_dt.c | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
--
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 v2 3/5] PM / OPP: check for existing OPP list when initialising from device tree

[Viresh Kumar]

On 1 October 2013 19:03, Sudeep KarkadaNagesha
<Sudeep.KarkadaNagesha@arm.com> wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>
> CPUs are registered as devices and their OPPs can be initialised from
> the device tree. Whenever CPUs are hotplugged out, the corresponding
> cpu devices are not removed. As a result all their OPPs remain intact
> even when they are offlined.
>
> But when they are hotplugged back-in, the cpufreq along with other cpu
> related subsystem gets re-initialised. Since its almost same as secondary
> cpu being brought up, no special consideration is taken in the hotplug
> path. This may result in cpufreq trying to initialise the OPPs again though
> the cpu device already contains the OPPs.
>
> This patch checks if there exist an OPP list associated with the device,
> before attempting to initialise it.
>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> ---
>  drivers/base/power/opp.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH v2 4/5] cpufreq: arm_big_little_dt: enhance OPP error checking

[Viresh Kumar]

On 1 October 2013 19:03, Sudeep KarkadaNagesha
<Sudeep.KarkadaNagesha@arm.com> wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>
> Since the OPPs can be specified either through 'operating-points'
> as tuples or through 'operating-points-phandle' as phandle to the
> node containing tuples, we need to check for both the properties.
>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> ---
>  drivers/cpufreq/arm_big_little_dt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH v2 1/5] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Nishanth Menon]

On 10/01/2013 08:32 AM, Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> 
> If more than one similar devices share the same operating points(OPPs)
> being in the same clock domain, currently we need to replicate the
> OPP entries in all the nodes.
> 
> This patch extends existing binding by adding a new property named
> 'operating-points-phandle' to specify the phandle in any device node
> pointing to another node which contains the actual OPP tuples.
> This helps to avoid replication if multiple devices share the OPPs.
> 
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: Nishanth Menon <nm@ti.com>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> ---
>  Documentation/devicetree/bindings/power/opp.txt | 161 ++++++++++++++++++++++--
>  1 file changed, 149 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
> index 74499e5..f59b878 100644
> --- a/Documentation/devicetree/bindings/power/opp.txt
> +++ b/Documentation/devicetree/bindings/power/opp.txt
> @@ -4,22 +4,159 @@ 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.
>  
> -Properties:
> +Required Properties:
>  - operating-points: An array of 2-tuples items, and each item consists
>    of frequency and voltage like <freq-kHz vol-uV>.
>  	freq: clock frequency in kHz
>  	vol: voltage in microvolt
>  
> +- operating-points-phandle: phandle to the device tree node which contains
> +	the operating points tuples(recommended to be used if multiple
> +	devices are in the same clock domain and hence share OPPs, as it
> +	avoids replication of OPPs)
> +
> +  operating-points and operating-points-phandle are mutually exclusive, only
> +  one of them can be present in any device node.
> +
>  Examples:
>  
> -cpu@0 {
> -	compatible = "arm,cortex-a9";
> -	reg = <0>;
> -	next-level-cache = <&L2>;
> -	operating-points = <
> -		/* kHz    uV */
> -		792000  1100000
> -		396000  950000
> -		198000  850000
> -	>;
> -};
> +1. A uniprocessor system (phandle not required)
> +
> +	cpu0: cpu@0 {
> +		compatible = "arm,cortex-a9";
> +		reg = <0>;
> +		operating-points = <
> +			/* kHz    uV */
> +			792000  1100000
> +			396000  950000
> +			198000  850000
> +		>;
> +	};
> +
> +2a. Consider a SMP system with 4 CPUs in the same clock domain(no phandle)
> +    Some existing DTs describe homogenous SMP systems by only listing the
> +    OPPs in the cpu@0 node. For compatiblity with existing DTs, an
> +    operating system may handle this case specially.
> +
> +	cpu0: cpu@0 {
> +		compatible = "arm,cortex-a9";
> +		reg = <0>;
> +		operating-points = <
> +			/* kHz    uV */
> +			792000  1100000
> +			396000  950000
> +			198000  850000
> +		>;
> +	};
> +
> +	cpu1: cpu@1 {
> +		compatible = "arm,cortex-a9";
> +		reg = <1>;
> +	};
> +
> +	cpu2: cpu@2 {
> +		compatible = "arm,cortex-a9";
> +		reg = <2>;
> +	};
> +
> +	cpu3: cpu@3 {
> +		compatible = "arm,cortex-a9";
> +		reg = <3>;
> +	};
> +
> +2b. Consider a SMP system with 4 CPUs in the same clock domain(with phandle)
> +    If more than one device of same type share the same OPPs, for example
> +    all the CPUs on a SoC or in a single cluster on a SoC, then we can avoid
> +    replicating the OPPs in all the nodes. We can specify the phandle of
> +    the node which contains the OPP tuples instead.
> +
> +	cpu0: cpu@0 {
> +		compatible = "arm,cortex-a9";
> +		reg = <0>;
> +		operating-points-phandle = <&cpu_opp>;
> +	};
> +
> +	cpu1: cpu@1 {
> +		compatible = "arm,cortex-a9";
> +		reg = <1>;
> +		operating-points-phandle = <&cpu_opp>;
> +	};
> +
> +	cpu2: cpu@2 {
> +		compatible = "arm,cortex-a9";
> +		reg = <2>;
> +		operating-points-phandle = <&cpu_opp>;
> +	};
> +
> +	cpu3: cpu@3 {
> +		compatible = "arm,cortex-a9";
> +		reg = <3>;
> +		operating-points-phandle = <&cpu_opp>;
> +	};
> +
> +	opps-table {
> +		cpu_opp: cpu_opp {
> +			operating-points = <
> +				/* kHz    uV */
> +				792000  1100000
> +				396000  950000
> +				198000  850000
> +			>;
> +		};
> +		... /* other device OPP nodes */
> +	}
> +
> +4. Consider an AMP(asymmetric multi-processor) sytem with 2 clusters of
> +   CPUs. Each cluster has 2 CPUs and all the CPUs within the cluster share
> +   the clock domain.
> +
> +	cpu0: cpu@0 {
> +		compatible = "arm,cortex-a15";
> +		reg = <0>;
> +		operating-points-phandle = <&cluster0_opp>;
> +	};
> +
> +	cpu1: cpu@1 {
> +		compatible = "arm,cortex-a15";
> +		reg = <1>;
> +		operating-points-phandle = <&cluster0_opp>;
> +	};
> +
> +	cpu2: cpu@100 {
> +		compatible = "arm,cortex-a7";
> +		reg = <100>;
> +		operating-points-phandle = <&cluster1_opp>;
> +	};
> +
> +	cpu3: cpu@101 {
> +		compatible = "arm,cortex-a7";
> +		reg = <101>;
> +		operating-points-phandle = <&cluster1_opp>;
> +	};
> +
> +	opps-table {
> +		cluster0_opp: cluster0_opp {
> +			operating-points = <
> +				/* kHz    uV */
> +				792000  1100000
> +				396000  950000
> +				198000  850000
> +			>;
> +		};
Style comment - add an EOL
> +		cluster1_opp: cluster1_opp {
> +			operating-points = <
> +				/* kHz    uV */
> +				792000  950000
> +				396000  750000
> +				198000  450000
> +			>;
> +		};
> +		... /* other device OPP nodes */
> +	}
> +
> +Container Node
> +--------------
> +	- It's highly recommended to place all the shared OPPs under single
> +	  node for consistency and better readability
> +	- It's quite similar to clocks or pinmux container nodes
> +	- In the above examples, "opps-table" is the container node
> 

in short, I love this - thanks for doing this.

However, could you squash this to patch #2 -> having implementation
and binding together is better for git log history.

-- 
Regards,
Nishanth Menon

Re: [PATCH v2 1/5] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Sudeep KarkadaNagesha]

Hi Nishanth,

Thanks for reviewing it.

On 03/10/13 13:40, Nishanth Menon wrote:
> On 10/01/2013 08:32 AM, Sudeep KarkadaNagesha wrote:
>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
[...]
>> +4. Consider an AMP(asymmetric multi-processor) sytem with 2 clusters of
>> +   CPUs. Each cluster has 2 CPUs and all the CPUs within the cluster share
>> +   the clock domain.
>> +
>> +	cpu0: cpu@0 {
>> +		compatible = "arm,cortex-a15";
>> +		reg = <0>;
>> +		operating-points-phandle = <&cluster0_opp>;
>> +	};
>> +
>> +	cpu1: cpu@1 {
>> +		compatible = "arm,cortex-a15";
>> +		reg = <1>;
>> +		operating-points-phandle = <&cluster0_opp>;
>> +	};
>> +
>> +	cpu2: cpu@100 {
>> +		compatible = "arm,cortex-a7";
>> +		reg = <100>;
>> +		operating-points-phandle = <&cluster1_opp>;
>> +	};
>> +
>> +	cpu3: cpu@101 {
>> +		compatible = "arm,cortex-a7";
>> +		reg = <101>;
>> +		operating-points-phandle = <&cluster1_opp>;
>> +	};
>> +
>> +	opps-table {
>> +		cluster0_opp: cluster0_opp {
>> +			operating-points = <
>> +				/* kHz    uV */
>> +				792000  1100000
>> +				396000  950000
>> +				198000  850000
>> +			>;
>> +		};
> Style comment - add an EOL
Ok will fix up.

>> +		cluster1_opp: cluster1_opp {
>> +			operating-points = <
>> +				/* kHz    uV */
>> +				792000  950000
>> +				396000  750000
>> +				198000  450000
>> +			>;
>> +		};
>> +		... /* other device OPP nodes */
>> +	}
>> +
>> +Container Node
>> +--------------
>> +	- It's highly recommended to place all the shared OPPs under single
>> +	  node for consistency and better readability
>> +	- It's quite similar to clocks or pinmux container nodes
>> +	- In the above examples, "opps-table" is the container node
>>
> 
> in short, I love this - thanks for doing this.
> 
> However, could you squash this to patch #2 -> having implementation
> and binding together is better for git log history.
> 
Based on some arguments on the other threads[1] on devicetree list, I thought
having separate patches for binding and driver changes is preferred. Hence the
split, I am OK either way.

Can I add your ACK/Reviewed-by otherwise ?

Regards,
Sudeep

[1] http://www.spinics.net/lists/devicetree/msg04855.html


--
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 v2 2/5] PM / OPP: add support to specify phandle of another node for OPP

[Nishanth Menon]

On 10/01/2013 11:46 AM, Sudeep KarkadaNagesha wrote:
> On 01/10/13 14:32, Sudeep KarkadaNagesha wrote:
>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
>>
>> Currently we need to replicate the OPP entries in all the nodes even
>> though they share OPPs being in the same clock domain.
>>
>> Few drivers like cpufreq depend on physical cpu0 node to specify the
>> OPPs and only that node is referred irrespective of the logical cpu
>> accessing it. Alternatively to support cpuhotplug path, few drivers
>> parse all the cpu nodes for OPPs. Instead we can specify the phandle
>> of the node which contains the OPP tuples.
>>
>> This patch adds support to the new property 'operating-points-phandle'
>> which specifies the phandle pointing to another node which contains the
>> actual OPP tuples.
>>
>> Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>> Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
>> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
>> Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
>> Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
>> Cc: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
>> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
>> ---
>>  drivers/base/power/opp.c | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index ef89897..bb6ff64 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -708,12 +708,20 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
>>  int of_init_opp_table(struct device *dev)
>>  {
>>  	const struct property *prop;
>> +	struct device_node *opp_node;
>>  	const __be32 *val;
>>  	int nr;
>>  
>> -	prop = of_find_property(dev->of_node, "operating-points", NULL);
>> -	if (!prop)
>> +	opp_node = of_parse_phandle(dev->of_node,
>> +					"operating-points-phandle", 0);
>> +	if (!opp_node) /* if no OPP phandle, search for OPPs in current node */
>> +		opp_node = dev->of_node;
>> +	prop = of_find_property(opp_node, "operating-points", NULL);
>> +	if (!prop) {
>> +		dev_warn(dev, "node %s missing operating-points property\n",
>> +							opp_node->full_name);
>>  		return -ENODEV;
>> +	}
>>  	if (!prop->value)
>>  		return -ENODATA;
>>  
>> @@ -740,6 +748,9 @@ int of_init_opp_table(struct device *dev)
>>  		nr -= 2;
>>  	}
>>  
>> +	if (opp_node != dev->of_node)
>> +		of_node_put(opp_node);
>> +
> There are several exit paths on error conditions where we need to do this.
> I missed them all. Here is the updated patch:

> 
> -->8--
> 
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
> 
> Currently we need to replicate the OPP entries in all the nodes even
> though they share OPPs being in the same clock domain.
> 
> Few drivers like cpufreq depend on physical cpu0 node to specify the
> OPPs and only that node is referred irrespective of the logical cpu
> accessing it. Alternatively to support cpuhotplug path, few drivers
> parse all the cpu nodes for OPPs. Instead we can specify the phandle
> of the node which contains the OPP tuples.
> 
> This patch adds support to the new property 'operating-points-phandle'
> which specifies the phandle pointing to another node which contains the
> actual OPP tuples.
> 
> Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
> Cc: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/base/power/opp.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index ef89897..cd4dbb3 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -708,14 +708,25 @@ struct srcu_notifier_head *opp_get_notifier(struct device
> *dev)
^^
<minor crib>
When reposting, could you avoid having word wrap? - kinda hard to get
https://patchwork.kernel.org/patch/2971091/ to apply clean :(

wget -O a.patch 'https://patchwork.kernel.org/patch/2971091/mbox/'
patch -p1< a.patch --dry-run
patching file drivers/base/power/opp.c
patch: **** malformed patch at line 143: *dev)

also when i look at the mbox patch, I see it split up.. did not dig in
why.. manually fixed it up.
</minor crib>

minor comments follow (other than squashing it with patch #1)
>  int of_init_opp_table(struct device *dev)
>  {
>  	const struct property *prop;
> +	struct device_node *opp_node;
>  	const __be32 *val;
> -	int nr;
> -
> -	prop = of_find_property(dev->of_node, "operating-points", NULL);
> -	if (!prop)
> -		return -ENODEV;
> -	if (!prop->value)
> -		return -ENODATA;
> +	int nr, ret = 0;
> +
> +	opp_node = of_parse_phandle(dev->of_node,
> +					"operating-points-phandle", 0);
> +	if (!opp_node) /* if no OPP phandle, search for OPPs in current node */
If you do not have a strong opinion, could you move the comment above
the if?
> +		opp_node = dev->of_node;
add an eol here.
> +	prop = of_find_property(opp_node, "operating-points", NULL);
> +	if (!prop) {
> +		dev_warn(dev, "node %s missing operating-points property\n",
> +							opp_node->full_name);
^^ align the opp_node->full_name to the d in dev in dev_warn(dev?
Checkpatch.sh --strict reports
CHECK: Alignment should match open parenthesis
#57: FILE: drivers/base/power/opp.c:722:
+		dev_warn(dev, "node %s missing operating-points property\n",
+							opp_node->full_name);

total: 0 errors, 0 warnings, 1 checks, 53 lines checked


> +		ret = -ENODEV;
> +		goto put_node;
> +	}
> +	if (!prop->value) {
> +		ret = -ENODATA;
> +		goto put_node;
> +	}
> 
>  	/*
>  	 * Each OPP is a set of tuples consisting of frequency and
> @@ -724,7 +735,8 @@ int of_init_opp_table(struct device *dev)
>  	nr = prop->length / sizeof(u32);
>  	if (nr % 2) {
>  		dev_err(dev, "%s: Invalid OPP list\n", __func__);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto put_node;
>  	}
> 
>  	val = prop->value;
> @@ -740,7 +752,11 @@ int of_init_opp_table(struct device *dev)
>  		nr -= 2;
>  	}
> 
> -	return 0;
> +put_node:
> +	if (opp_node != dev->of_node)
> +		of_node_put(opp_node);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>  #endif
> 
other than that, please feel free to add
Acked-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>

-- 
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] PM / OPP: check for existing OPP list when initialising from device tree

[Nishanth Menon]

On 10/01/2013 08:33 AM, Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
> 
> CPUs are registered as devices and their OPPs can be initialised from
> the device tree. Whenever CPUs are hotplugged out, the corresponding
> cpu devices are not removed. As a result all their OPPs remain intact
> even when they are offlined.
> 
> But when they are hotplugged back-in, the cpufreq along with other cpu
> related subsystem gets re-initialised. Since its almost same as secondary
> cpu being brought up, no special consideration is taken in the hotplug
> path. This may result in cpufreq trying to initialise the OPPs again though
> the cpu device already contains the OPPs.
> 
> This patch checks if there exist an OPP list associated with the device,
> before attempting to initialise it.
> 
> Cc: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
> Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
> Cc: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/base/power/opp.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index bb6ff64..05b2ebf 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -709,9 +709,15 @@ int of_init_opp_table(struct device *dev)
>  {
>  	const struct property *prop;
>  	struct device_node *opp_node;
> +	struct device_opp *dev_opp;
>  	const __be32 *val;
>  	int nr;
>  
> +	/* Check for existing list for 'dev' */
> +	dev_opp = find_device_opp(dev);
> +	if (!IS_ERR(dev_opp))
> +		return -EEXIST; /* Device OPP already initialized */
in the interest of remaining code, Prefer comments not inline with
code -> example if additional logic gets added later on in the if
condition, someone'd have to move that comment over, instead, prefer
it embedded before the if check as a line of it's own.
> +
>  	opp_node = of_parse_phandle(dev->of_node,
>  					"operating-points-phandle", 0);
>  	if (!opp_node) /* if no OPP phandle, search for OPPs in current node */
> 

other than that,
Acked-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>

-- 
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/5] cpufreq: arm_big_little_dt: enhance OPP error checking

[Nishanth Menon]

On 10/01/2013 08:33 AM, Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
> 
> Since the OPPs can be specified either through 'operating-points'
> as tuples or through 'operating-points-phandle' as phandle to the
> node containing tuples, we need to check for both the properties.
> 
> Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
> Cc: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/cpufreq/arm_big_little_dt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/arm_big_little_dt.c b/drivers/cpufreq/arm_big_little_dt.c
> index 480c0bd..b03d4fe 100644
> --- a/drivers/cpufreq/arm_big_little_dt.c
> +++ b/drivers/cpufreq/arm_big_little_dt.c
> @@ -35,7 +35,8 @@ static struct device_node *get_cpu_node_with_valid_op(int cpu)
>  {
>  	struct device_node *np = of_cpu_device_node_get(cpu);
>  
> -	if (!of_get_property(np, "operating-points", NULL)) {
> +	if (!of_get_property(np, "operating-points", NULL) &&
> +		!of_get_property(np, "operating-points-phandle", NULL)) {
>  		of_node_put(np);
>  		np = NULL;
>  	}
> 
generated checkpatch --strict warning:
CHECK: Alignment should match open parenthesis
#28: FILE: drivers/cpufreq/arm_big_little_dt.c:39:
+	if (!of_get_property(np, "operating-points", NULL) &&
+		!of_get_property(np, "operating-points-phandle", NULL)) {

otherwise,
Reviewed-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>

-- 
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Nishanth Menon]

On 10/03/2013 08:05 AM, Sudeep KarkadaNagesha wrote:
[...]
>> However, could you squash this to patch #2 -> having implementation
>> and binding together is better for git log history.
>>
> Based on some arguments on the other threads[1] on devicetree list, I thought
> having separate patches for binding and driver changes is preferred. Hence the
> split, I am OK either way.
Thanks for pointing the discussion out.

/me might rant about this ;) -> if someone has a strong opinion about
this, they should probably propose a change to submitting patches
guideline.. Grr..

I leave this to Rafael as to how he'd like this to be squashed/split

> 
> Can I add your ACK/Reviewed-by otherwise ?
> 
> Regards,
> Sudeep
> 
> [1] http://www.spinics.net/lists/devicetree/msg04855.html
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH v2 2/5] PM / OPP: add support to specify phandle of another node for OPP

[Sudeep KarkadaNagesha]

On 03/10/13 15:20, Nishanth Menon wrote:
> On 10/01/2013 11:46 AM, Sudeep KarkadaNagesha wrote:
>> On 01/10/13 14:32, Sudeep KarkadaNagesha wrote:
[...]
>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>>
>> Currently we need to replicate the OPP entries in all the nodes even
>> though they share OPPs being in the same clock domain.
>>
>> Few drivers like cpufreq depend on physical cpu0 node to specify the
>> OPPs and only that node is referred irrespective of the logical cpu
>> accessing it. Alternatively to support cpuhotplug path, few drivers
>> parse all the cpu nodes for OPPs. Instead we can specify the phandle
>> of the node which contains the OPP tuples.
>>
>> This patch adds support to the new property 'operating-points-phandle'
>> which specifies the phandle pointing to another node which contains the
>> actual OPP tuples.
>>
>> Cc: Rob Herring <rob.herring@calxeda.com>
>> Cc: Pawel Moll <pawel.moll@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Stephen Warren <swarren@wwwdotorg.org>
>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
>> Cc: Nishanth Menon <nm@ti.com>
>> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>> ---
>>  drivers/base/power/opp.c | 34 +++++++++++++++++++++++++---------
>>  1 file changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index ef89897..cd4dbb3 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -708,14 +708,25 @@ struct srcu_notifier_head *opp_get_notifier(struct device
>> *dev)
> ^^
> <minor crib>
> When reposting, could you avoid having word wrap? - kinda hard to get
> https://patchwork.kernel.org/patch/2971091/ to apply clean :(
> 
> wget -O a.patch 'https://patchwork.kernel.org/patch/2971091/mbox/'
> patch -p1< a.patch --dry-run
> patching file drivers/base/power/opp.c
> patch: **** malformed patch at line 143: *dev)
> 
> also when i look at the mbox patch, I see it split up.. did not dig in
> why.. manually fixed it up.

Sorry for this, since I replied over my original patch(didn't use git mail)
I messed it up. It won't happen again :)

> </minor crib>
> 
> minor comments follow (other than squashing it with patch #1)
>>  int of_init_opp_table(struct device *dev)
>>  {
>>  	const struct property *prop;
>> +	struct device_node *opp_node;
>>  	const __be32 *val;
>> -	int nr;
>> -
>> -	prop = of_find_property(dev->of_node, "operating-points", NULL);
>> -	if (!prop)
>> -		return -ENODEV;
>> -	if (!prop->value)
>> -		return -ENODATA;
>> +	int nr, ret = 0;
>> +
>> +	opp_node = of_parse_phandle(dev->of_node,
>> +					"operating-points-phandle", 0);
>> +	if (!opp_node) /* if no OPP phandle, search for OPPs in current node */
> If you do not have a strong opinion, could you move the comment above
> the if?
>> +		opp_node = dev->of_node;
> add an eol here.
>> +	prop = of_find_property(opp_node, "operating-points", NULL);
>> +	if (!prop) {
>> +		dev_warn(dev, "node %s missing operating-points property\n",
>> +							opp_node->full_name);
> ^^ align the opp_node->full_name to the d in dev in dev_warn(dev?
> Checkpatch.sh --strict reports
> CHECK: Alignment should match open parenthesis
> #57: FILE: drivers/base/power/opp.c:722:
> +		dev_warn(dev, "node %s missing operating-points property\n",
> +							opp_node->full_name);
> 
> total: 0 errors, 0 warnings, 1 checks, 53 lines checked
> 
All the other coding style comments and checkpatch errors reported in all the
patches are now fixed. I will post the next version once I get response for DT
binding updates from DT maintainers.

> 
>> +		ret = -ENODEV;
>> +		goto put_node;
>> +	}
>> +	if (!prop->value) {
>> +		ret = -ENODATA;
>> +		goto put_node;
>> +	}
>>
>>  	/*
>>  	 * Each OPP is a set of tuples consisting of frequency and
>> @@ -724,7 +735,8 @@ int of_init_opp_table(struct device *dev)
>>  	nr = prop->length / sizeof(u32);
>>  	if (nr % 2) {
>>  		dev_err(dev, "%s: Invalid OPP list\n", __func__);
>> -		return -EINVAL;
>> +		ret = -EINVAL;
>> +		goto put_node;
>>  	}
>>
>>  	val = prop->value;
>> @@ -740,7 +752,11 @@ int of_init_opp_table(struct device *dev)
>>  		nr -= 2;
>>  	}
>>
>> -	return 0;
>> +put_node:
>> +	if (opp_node != dev->of_node)
>> +		of_node_put(opp_node);
>> +
>> +	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>>  #endif
>>
> other than that, please feel free to add
> Acked-by: Nishanth Menon <nm@ti.com>
> 
Thanks for the review.

Regards,
Sudeep

Re: [PATCH v2 1/5] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Sudeep KarkadaNagesha]

Hi Mark, Stephen,

On 01/10/13 14:32, Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> 
> If more than one similar devices share the same operating points(OPPs)
> being in the same clock domain, currently we need to replicate the
> OPP entries in all the nodes.
> 
> This patch extends existing binding by adding a new property named
> 'operating-points-phandle' to specify the phandle in any device node
> pointing to another node which contains the actual OPP tuples.
> This helps to avoid replication if multiple devices share the OPPs.
> 

Can you review this version ?

Regards,
Sudeep

Re: [PATCH v2 1/5] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Rob Herring]

On 10/01/2013 08:32 AM, Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> 
> If more than one similar devices share the same operating points(OPPs)
> being in the same clock domain, currently we need to replicate the
> OPP entries in all the nodes.
> 
> This patch extends existing binding by adding a new property named
> 'operating-points-phandle' to specify the phandle in any device node
> pointing to another node which contains the actual OPP tuples.
> This helps to avoid replication if multiple devices share the OPPs.
> 
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: Nishanth Menon <nm@ti.com>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> ---
>  Documentation/devicetree/bindings/power/opp.txt | 161 ++++++++++++++++++++++--
>  1 file changed, 149 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
> index 74499e5..f59b878 100644
> --- a/Documentation/devicetree/bindings/power/opp.txt
> +++ b/Documentation/devicetree/bindings/power/opp.txt
> @@ -4,22 +4,159 @@ 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.
>  
> -Properties:
> +Required Properties:
>  - operating-points: An array of 2-tuples items, and each item consists
>    of frequency and voltage like <freq-kHz vol-uV>.
>  	freq: clock frequency in kHz
>  	vol: voltage in microvolt
>  
> +- operating-points-phandle: phandle to the device tree node which contains
> +	the operating points tuples(recommended to be used if multiple
> +	devices are in the same clock domain and hence share OPPs, as it
> +	avoids replication of OPPs)
> +
> +  operating-points and operating-points-phandle are mutually exclusive, only
> +  one of them can be present in any device node.
> +
>  Examples:
>  
> -cpu@0 {
> -	compatible = "arm,cortex-a9";
> -	reg = <0>;
> -	next-level-cache = <&L2>;
> -	operating-points = <
> -		/* kHz    uV */
> -		792000  1100000
> -		396000  950000
> -		198000  850000
> -	>;
> -};
> +1. A uniprocessor system (phandle not required)
> +
> +	cpu0: cpu@0 {
> +		compatible = "arm,cortex-a9";
> +		reg = <0>;
> +		operating-points = <
> +			/* kHz    uV */
> +			792000  1100000
> +			396000  950000
> +			198000  850000
> +		>;
> +	};
> +
> +2a. Consider a SMP system with 4 CPUs in the same clock domain(no phandle)
> +    Some existing DTs describe homogenous SMP systems by only listing the
> +    OPPs in the cpu@0 node. For compatiblity with existing DTs, an
> +    operating system may handle this case specially.
> +
> +	cpu0: cpu@0 {
> +		compatible = "arm,cortex-a9";
> +		reg = <0>;
> +		operating-points = <
> +			/* kHz    uV */
> +			792000  1100000
> +			396000  950000
> +			198000  850000
> +		>;
> +	};
> +
> +	cpu1: cpu@1 {
> +		compatible = "arm,cortex-a9";
> +		reg = <1>;
> +	};
> +
> +	cpu2: cpu@2 {
> +		compatible = "arm,cortex-a9";
> +		reg = <2>;
> +	};
> +
> +	cpu3: cpu@3 {
> +		compatible = "arm,cortex-a9";
> +		reg = <3>;
> +	};
> +
> +2b. Consider a SMP system with 4 CPUs in the same clock domain(with phandle)
> +    If more than one device of same type share the same OPPs, for example
> +    all the CPUs on a SoC or in a single cluster on a SoC, then we can avoid
> +    replicating the OPPs in all the nodes. We can specify the phandle of
> +    the node which contains the OPP tuples instead.
> +
> +	cpu0: cpu@0 {
> +		compatible = "arm,cortex-a9";
> +		reg = <0>;
> +		operating-points-phandle = <&cpu_opp>;
> +	};
> +
> +	cpu1: cpu@1 {
> +		compatible = "arm,cortex-a9";
> +		reg = <1>;
> +		operating-points-phandle = <&cpu_opp>;
> +	};
> +
> +	cpu2: cpu@2 {
> +		compatible = "arm,cortex-a9";
> +		reg = <2>;
> +		operating-points-phandle = <&cpu_opp>;
> +	};
> +
> +	cpu3: cpu@3 {
> +		compatible = "arm,cortex-a9";
> +		reg = <3>;
> +		operating-points-phandle = <&cpu_opp>;
> +	};
> +
> +	opps-table {
> +		cpu_opp: cpu_opp {
> +			operating-points = <
> +				/* kHz    uV */
> +				792000  1100000
> +				396000  950000
> +				198000  850000
> +			>;
> +		};
> +		... /* other device OPP nodes */

But this is a subnode of /cpus. IMO, OPPs should be located near what
they control.


> +	}
> +
> +4. Consider an AMP(asymmetric multi-processor) sytem with 2 clusters of
> +   CPUs. Each cluster has 2 CPUs and all the CPUs within the cluster share
> +   the clock domain.
> +
> +	cpu0: cpu@0 {
> +		compatible = "arm,cortex-a15";
> +		reg = <0>;
> +		operating-points-phandle = <&cluster0_opp>;
> +	};
> +
> +	cpu1: cpu@1 {
> +		compatible = "arm,cortex-a15";
> +		reg = <1>;
> +		operating-points-phandle = <&cluster0_opp>;
> +	};
> +
> +	cpu2: cpu@100 {
> +		compatible = "arm,cortex-a7";
> +		reg = <100>;
> +		operating-points-phandle = <&cluster1_opp>;
> +	};
> +
> +	cpu3: cpu@101 {
> +		compatible = "arm,cortex-a7";
> +		reg = <101>;
> +		operating-points-phandle = <&cluster1_opp>;
> +	};
> +
> +	opps-table {
> +		cluster0_opp: cluster0_opp {

Why not use the cpu topology? Then the operating point can apply to
cores based on the position in the topology. You don't even need a
phandle in that case. You can look for OPPs in either a cpu node or in
the topology.


> +			operating-points = <
> +				/* kHz    uV */
> +				792000  1100000
> +				396000  950000
> +				198000  850000
> +			>;
> +		};
> +		cluster1_opp: cluster1_opp {
> +			operating-points = <
> +				/* kHz    uV */
> +				792000  950000
> +				396000  750000
> +				198000  450000
> +			>;
> +		};
> +		... /* other device OPP nodes */
> +	}
> +
> +Container Node
> +--------------
> +	- It's highly recommended to place all the shared OPPs under single
> +	  node for consistency and better readability
> +	- It's quite similar to clocks or pinmux container nodes
> +	- In the above examples, "opps-table" is the container node
> 

Re: [PATCH v2 1/5] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Sudeep KarkadaNagesha]

On 07/10/13 17:01, Rob Herring wrote:
> On 10/01/2013 08:32 AM, Sudeep KarkadaNagesha wrote:
>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>>
>> If more than one similar devices share the same operating points(OPPs)
>> being in the same clock domain, currently we need to replicate the
>> OPP entries in all the nodes.
>>
>> This patch extends existing binding by adding a new property named
>> 'operating-points-phandle' to specify the phandle in any device node
>> pointing to another node which contains the actual OPP tuples.
>> This helps to avoid replication if multiple devices share the OPPs.
>>
>> Cc: Rob Herring <rob.herring@calxeda.com>
>> Cc: Pawel Moll <pawel.moll@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Stephen Warren <swarren@wwwdotorg.org>
>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
>> Cc: Nishanth Menon <nm@ti.com>
>> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>> ---
>>  Documentation/devicetree/bindings/power/opp.txt | 161 ++++++++++++++++++++++--
>>  1 file changed, 149 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
>> index 74499e5..f59b878 100644
>> --- a/Documentation/devicetree/bindings/power/opp.txt
>> +++ b/Documentation/devicetree/bindings/power/opp.txt
>> @@ -4,22 +4,159 @@ 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.
>>  
>> -Properties:
>> +Required Properties:
>>  - operating-points: An array of 2-tuples items, and each item consists
>>    of frequency and voltage like <freq-kHz vol-uV>.
>>  	freq: clock frequency in kHz
>>  	vol: voltage in microvolt
>>  
>> +- operating-points-phandle: phandle to the device tree node which contains
>> +	the operating points tuples(recommended to be used if multiple
>> +	devices are in the same clock domain and hence share OPPs, as it
>> +	avoids replication of OPPs)
>> +
>> +  operating-points and operating-points-phandle are mutually exclusive, only
>> +  one of them can be present in any device node.
>> +
>>  Examples:
>>  
>> -cpu@0 {
>> -	compatible = "arm,cortex-a9";
>> -	reg = <0>;
>> -	next-level-cache = <&L2>;
>> -	operating-points = <
>> -		/* kHz    uV */
>> -		792000  1100000
>> -		396000  950000
>> -		198000  850000
>> -	>;
>> -};
>> +1. A uniprocessor system (phandle not required)
>> +
>> +	cpu0: cpu@0 {
>> +		compatible = "arm,cortex-a9";
>> +		reg = <0>;
>> +		operating-points = <
>> +			/* kHz    uV */
>> +			792000  1100000
>> +			396000  950000
>> +			198000  850000
>> +		>;
>> +	};
>> +
>> +2a. Consider a SMP system with 4 CPUs in the same clock domain(no phandle)
>> +    Some existing DTs describe homogenous SMP systems by only listing the
>> +    OPPs in the cpu@0 node. For compatiblity with existing DTs, an
>> +    operating system may handle this case specially.
>> +
>> +	cpu0: cpu@0 {
>> +		compatible = "arm,cortex-a9";
>> +		reg = <0>;
>> +		operating-points = <
>> +			/* kHz    uV */
>> +			792000  1100000
>> +			396000  950000
>> +			198000  850000
>> +		>;
>> +	};
>> +
>> +	cpu1: cpu@1 {
>> +		compatible = "arm,cortex-a9";
>> +		reg = <1>;
>> +	};
>> +
>> +	cpu2: cpu@2 {
>> +		compatible = "arm,cortex-a9";
>> +		reg = <2>;
>> +	};
>> +
>> +	cpu3: cpu@3 {
>> +		compatible = "arm,cortex-a9";
>> +		reg = <3>;
>> +	};
>> +
>> +2b. Consider a SMP system with 4 CPUs in the same clock domain(with phandle)
>> +    If more than one device of same type share the same OPPs, for example
>> +    all the CPUs on a SoC or in a single cluster on a SoC, then we can avoid
>> +    replicating the OPPs in all the nodes. We can specify the phandle of
>> +    the node which contains the OPP tuples instead.
>> +
>> +	cpu0: cpu@0 {
>> +		compatible = "arm,cortex-a9";
>> +		reg = <0>;
>> +		operating-points-phandle = <&cpu_opp>;
>> +	};
>> +
>> +	cpu1: cpu@1 {
>> +		compatible = "arm,cortex-a9";
>> +		reg = <1>;
>> +		operating-points-phandle = <&cpu_opp>;
>> +	};
>> +
>> +	cpu2: cpu@2 {
>> +		compatible = "arm,cortex-a9";
>> +		reg = <2>;
>> +		operating-points-phandle = <&cpu_opp>;
>> +	};
>> +
>> +	cpu3: cpu@3 {
>> +		compatible = "arm,cortex-a9";
>> +		reg = <3>;
>> +		operating-points-phandle = <&cpu_opp>;
>> +	};
>> +
>> +	opps-table {
>> +		cpu_opp: cpu_opp {
>> +			operating-points = <
>> +				/* kHz    uV */
>> +				792000  1100000
>> +				396000  950000
>> +				198000  850000
>> +			>;
>> +		};
>> +		... /* other device OPP nodes */
> 
> But this is a subnode of /cpus. IMO, OPPs should be located near what
> they control.
> 
No, the idea was to group all the shared OPPs in a container node like clocks or
pinmux. So opps-table in above example need not be subnode of /cpus.

> 
>> +	}
>> +
>> +4. Consider an AMP(asymmetric multi-processor) sytem with 2 clusters of
>> +   CPUs. Each cluster has 2 CPUs and all the CPUs within the cluster share
>> +   the clock domain.
>> +
>> +	cpu0: cpu@0 {
>> +		compatible = "arm,cortex-a15";
>> +		reg = <0>;
>> +		operating-points-phandle = <&cluster0_opp>;
>> +	};
>> +
>> +	cpu1: cpu@1 {
>> +		compatible = "arm,cortex-a15";
>> +		reg = <1>;
>> +		operating-points-phandle = <&cluster0_opp>;
>> +	};
>> +
>> +	cpu2: cpu@100 {
>> +		compatible = "arm,cortex-a7";
>> +		reg = <100>;
>> +		operating-points-phandle = <&cluster1_opp>;
>> +	};
>> +
>> +	cpu3: cpu@101 {
>> +		compatible = "arm,cortex-a7";
>> +		reg = <101>;
>> +		operating-points-phandle = <&cluster1_opp>;
>> +	};
>> +
>> +	opps-table {
>> +		cluster0_opp: cluster0_opp {
> 
> Why not use the cpu topology? Then the operating point can apply to
> cores based on the position in the topology. You don't even need a
> phandle in that case. You can look for OPPs in either a cpu node or in
> the topology.
> 
Agreed but few thoughts behind this binding:

1. OPPs are not just cpu specific:
   How do we share OPPs for 2 devices in the same clock domain ?
   Also moving the OPP into cpu-topo makes parsing specific to cpu.
   Currently the OPP library fetches the of_node from the device struct
   which is applicable to any devices.

2. Should cpu topology(i.e. cpu-map) just contain the topology info ? and
   phandles to these nodes should be used to setup any affinity ?

3. As part of RFC[1][2], it was also discussed that on some SoCs we need
   multiple OPP profiles/options which it provides but only one of them can
   be used based on the board design. phandle to OPP will also help resolve
   that issue.

Let me know your thoughts.

Regards,
Sudeep

[1] http://www.spinics.net/lists/cpufreq/msg06556.html
[2] http://www.spinics.net/lists/cpufreq/msg06685.html

Re: [PATCH v2 0/5] PM / OPP: updates to enable sharing OPPs info

[Rafael J. Wysocki]

On Tuesday, October 01, 2013 02:32:57 PM Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> 
> Hi,
> 
> These are few updates to existing device tree binding and the PM/OPP
> library to support sharing of OPPs between different device nodes.
> 
> Currently all the cpu nodes are parsed until the OPPs are found. This
> is essential to support cpuhotplug without having to replicate OPP
> information in all the cpu nodes.
> 
> However in systems with multiple cpu power domain, its better to have
> OPP entry for each cpu. To avoid replication, phandle can be specified
> to the node which contains the full OPP information.
> 
> The table node containing all the OPPs and device nodes pointing to an
> entry into it through phandle might also solve the problem of describing
> multiple OPP profiles.

It looks like this still is under discussion, isn't it?

If not, care to obtain ACKs from the people involved and resubmit?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2 1/5] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Sudeep KarkadaNagesha]

Hi DT-folks,

On 08/10/13 13:55, Sudeep KarkadaNagesha wrote:
> On 07/10/13 17:01, Rob Herring wrote:
>> On 10/01/2013 08:32 AM, Sudeep KarkadaNagesha wrote:
>>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>>>
>>> If more than one similar devices share the same operating points(OPPs)
>>> being in the same clock domain, currently we need to replicate the
>>> OPP entries in all the nodes.
>>>
>>> This patch extends existing binding by adding a new property named
>>> 'operating-points-phandle' to specify the phandle in any device node
>>> pointing to another node which contains the actual OPP tuples.
>>> This helps to avoid replication if multiple devices share the OPPs.
>>>
>>> Cc: Rob Herring <rob.herring@calxeda.com>
>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Stephen Warren <swarren@wwwdotorg.org>
>>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
>>> Cc: Nishanth Menon <nm@ti.com>
>>> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>>> ---
>>>  Documentation/devicetree/bindings/power/opp.txt | 161 ++++++++++++++++++++++--
>>>  1 file changed, 149 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
>>> index 74499e5..f59b878 100644
>>> --- a/Documentation/devicetree/bindings/power/opp.txt
>>> +++ b/Documentation/devicetree/bindings/power/opp.txt
>>> @@ -4,22 +4,159 @@ 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.
>>>  
>>> -Properties:
>>> +Required Properties:
>>>  - operating-points: An array of 2-tuples items, and each item consists
>>>    of frequency and voltage like <freq-kHz vol-uV>.
>>>  	freq: clock frequency in kHz
>>>  	vol: voltage in microvolt
>>>  
>>> +- operating-points-phandle: phandle to the device tree node which contains
>>> +	the operating points tuples(recommended to be used if multiple
>>> +	devices are in the same clock domain and hence share OPPs, as it
>>> +	avoids replication of OPPs)
>>> +
>>> +  operating-points and operating-points-phandle are mutually exclusive, only
>>> +  one of them can be present in any device node.
>>> +
>>>  Examples:
>>>  
>>> -cpu@0 {
>>> -	compatible = "arm,cortex-a9";
>>> -	reg = <0>;
>>> -	next-level-cache = <&L2>;
>>> -	operating-points = <
>>> -		/* kHz    uV */
>>> -		792000  1100000
>>> -		396000  950000
>>> -		198000  850000
>>> -	>;
>>> -};
>>> +1. A uniprocessor system (phandle not required)
>>> +
>>> +	cpu0: cpu@0 {
>>> +		compatible = "arm,cortex-a9";
>>> +		reg = <0>;
>>> +		operating-points = <
>>> +			/* kHz    uV */
>>> +			792000  1100000
>>> +			396000  950000
>>> +			198000  850000
>>> +		>;
>>> +	};
>>> +
>>> +2a. Consider a SMP system with 4 CPUs in the same clock domain(no phandle)
>>> +    Some existing DTs describe homogenous SMP systems by only listing the
>>> +    OPPs in the cpu@0 node. For compatiblity with existing DTs, an
>>> +    operating system may handle this case specially.
>>> +
>>> +	cpu0: cpu@0 {
>>> +		compatible = "arm,cortex-a9";
>>> +		reg = <0>;
>>> +		operating-points = <
>>> +			/* kHz    uV */
>>> +			792000  1100000
>>> +			396000  950000
>>> +			198000  850000
>>> +		>;
>>> +	};
>>> +
>>> +	cpu1: cpu@1 {
>>> +		compatible = "arm,cortex-a9";
>>> +		reg = <1>;
>>> +	};
>>> +
>>> +	cpu2: cpu@2 {
>>> +		compatible = "arm,cortex-a9";
>>> +		reg = <2>;
>>> +	};
>>> +
>>> +	cpu3: cpu@3 {
>>> +		compatible = "arm,cortex-a9";
>>> +		reg = <3>;
>>> +	};
>>> +
>>> +2b. Consider a SMP system with 4 CPUs in the same clock domain(with phandle)
>>> +    If more than one device of same type share the same OPPs, for example
>>> +    all the CPUs on a SoC or in a single cluster on a SoC, then we can avoid
>>> +    replicating the OPPs in all the nodes. We can specify the phandle of
>>> +    the node which contains the OPP tuples instead.
>>> +
>>> +	cpu0: cpu@0 {
>>> +		compatible = "arm,cortex-a9";
>>> +		reg = <0>;
>>> +		operating-points-phandle = <&cpu_opp>;
>>> +	};
>>> +
>>> +	cpu1: cpu@1 {
>>> +		compatible = "arm,cortex-a9";
>>> +		reg = <1>;
>>> +		operating-points-phandle = <&cpu_opp>;
>>> +	};
>>> +
>>> +	cpu2: cpu@2 {
>>> +		compatible = "arm,cortex-a9";
>>> +		reg = <2>;
>>> +		operating-points-phandle = <&cpu_opp>;
>>> +	};
>>> +
>>> +	cpu3: cpu@3 {
>>> +		compatible = "arm,cortex-a9";
>>> +		reg = <3>;
>>> +		operating-points-phandle = <&cpu_opp>;
>>> +	};
>>> +
>>> +	opps-table {
>>> +		cpu_opp: cpu_opp {
>>> +			operating-points = <
>>> +				/* kHz    uV */
>>> +				792000  1100000
>>> +				396000  950000
>>> +				198000  850000
>>> +			>;
>>> +		};
>>> +		... /* other device OPP nodes */
>>
>> But this is a subnode of /cpus. IMO, OPPs should be located near what
>> they control.
>>
> No, the idea was to group all the shared OPPs in a container node like clocks or
> pinmux. So opps-table in above example need not be subnode of /cpus.
> 
>>
>>> +	}
>>> +
>>> +4. Consider an AMP(asymmetric multi-processor) sytem with 2 clusters of
>>> +   CPUs. Each cluster has 2 CPUs and all the CPUs within the cluster share
>>> +   the clock domain.
>>> +
>>> +	cpu0: cpu@0 {
>>> +		compatible = "arm,cortex-a15";
>>> +		reg = <0>;
>>> +		operating-points-phandle = <&cluster0_opp>;
>>> +	};
>>> +
>>> +	cpu1: cpu@1 {
>>> +		compatible = "arm,cortex-a15";
>>> +		reg = <1>;
>>> +		operating-points-phandle = <&cluster0_opp>;
>>> +	};
>>> +
>>> +	cpu2: cpu@100 {
>>> +		compatible = "arm,cortex-a7";
>>> +		reg = <100>;
>>> +		operating-points-phandle = <&cluster1_opp>;
>>> +	};
>>> +
>>> +	cpu3: cpu@101 {
>>> +		compatible = "arm,cortex-a7";
>>> +		reg = <101>;
>>> +		operating-points-phandle = <&cluster1_opp>;
>>> +	};
>>> +
>>> +	opps-table {
>>> +		cluster0_opp: cluster0_opp {
>>
>> Why not use the cpu topology? Then the operating point can apply to
>> cores based on the position in the topology. You don't even need a
>> phandle in that case. You can look for OPPs in either a cpu node or in
>> the topology.
>>
> Agreed but few thoughts behind this binding:
> 
> 1. OPPs are not just cpu specific:
>    How do we share OPPs for 2 devices in the same clock domain ?
>    Also moving the OPP into cpu-topo makes parsing specific to cpu.
>    Currently the OPP library fetches the of_node from the device struct
>    which is applicable to any devices.
> 
> 2. Should cpu topology(i.e. cpu-map) just contain the topology info ? and
>    phandles to these nodes should be used to setup any affinity ?
> 
> 3. As part of RFC[1][2], it was also discussed that on some SoCs we need
>    multiple OPP profiles/options which it provides but only one of them can
>    be used based on the board design. phandle to OPP will also help resolve
>    that issue.
> 
> Let me know your thoughts.
> 

(pinging to get this discussion alive again)
Is this extension to the binding to support shared OPPs acceptable ?
If not, any ideas to progress on this is helpful.

Regards,
Sudeep

> 
> [1] http://www.spinics.net/lists/cpufreq/msg06556.html
> [2] http://www.spinics.net/lists/cpufreq/msg06685.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 1/5] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Rob Herring]

On Tue, Oct 8, 2013 at 7:55 AM, Sudeep KarkadaNagesha
<Sudeep.KarkadaNagesha@arm.com> wrote:
> On 07/10/13 17:01, Rob Herring wrote:
>> On 10/01/2013 08:32 AM, Sudeep KarkadaNagesha wrote:
>>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>>>
>>> If more than one similar devices share the same operating points(OPPs)
>>> being in the same clock domain, currently we need to replicate the
>>> OPP entries in all the nodes.
>>>
>>> This patch extends existing binding by adding a new property named
>>> 'operating-points-phandle' to specify the phandle in any device node
>>> pointing to another node which contains the actual OPP tuples.
>>> This helps to avoid replication if multiple devices share the OPPs.
>>>

[snip]

>>> +    opps-table {
>>> +            cpu_opp: cpu_opp {
>>> +                    operating-points = <
>>> +                            /* kHz    uV */
>>> +                            792000  1100000
>>> +                            396000  950000
>>> +                            198000  850000
>>> +                    >;
>>> +            };
>>> +            ... /* other device OPP nodes */
>>
>> But this is a subnode of /cpus. IMO, OPPs should be located near what
>> they control.
>>
> No, the idea was to group all the shared OPPs in a container node like clocks or
> pinmux. So opps-table in above example need not be subnode of /cpus.

Clocks are typically defined in a clock controller node. pinmux
doesn't fit anywhere well so it is an exception. We don't want to
expand on that. OPP's at least for cpu's would typically follow the
topology, so put them there.

>>> +    cpu3: cpu@101 {
>>> +            compatible = "arm,cortex-a7";
>>> +            reg = <101>;
>>> +            operating-points-phandle = <&cluster1_opp>;
>>> +    };
>>> +
>>> +    opps-table {
>>> +            cluster0_opp: cluster0_opp {
>>
>> Why not use the cpu topology? Then the operating point can apply to
>> cores based on the position in the topology. You don't even need a
>> phandle in that case. You can look for OPPs in either a cpu node or in
>> the topology.
>>
> Agreed but few thoughts behind this binding:
>
> 1. OPPs are not just cpu specific:
>    How do we share OPPs for 2 devices in the same clock domain ?
>    Also moving the OPP into cpu-topo makes parsing specific to cpu.
>    Currently the OPP library fetches the of_node from the device struct
>    which is applicable to any devices.

But an OPP for a cpu is cpu specific, so put it there. For devices, we
may need something different. Perhaps it should be part of the clock
binding in some way.

> 2. Should cpu topology(i.e. cpu-map) just contain the topology info ? and
>    phandles to these nodes should be used to setup any affinity ?

Can't the OPP just be in the topology nodes at the appropriate level.
Then the kernel can look for the OPPs in either place. Perhaps Lorenzo
has thoughts on this.

> 3. As part of RFC[1][2], it was also discussed that on some SoCs we need
>    multiple OPP profiles/options which it provides but only one of them can
>    be used based on the board design. phandle to OPP will also help resolve
>    that issue.

Then include the OPP required for that board design. You could have
dtsi files of OPPs that can be included based on the board.

Rob

Re: [PATCH v2 1/5] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Sudeep KarkadaNagesha]

On 17/10/13 14:22, Rob Herring wrote:
> On Tue, Oct 8, 2013 at 7:55 AM, Sudeep KarkadaNagesha
> <Sudeep.KarkadaNagesha@arm.com> wrote:
>> On 07/10/13 17:01, Rob Herring wrote:
>>> On 10/01/2013 08:32 AM, Sudeep KarkadaNagesha wrote:
>>>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>>>>
>>>> If more than one similar devices share the same operating points(OPPs)
>>>> being in the same clock domain, currently we need to replicate the
>>>> OPP entries in all the nodes.
>>>>
>>>> This patch extends existing binding by adding a new property named
>>>> 'operating-points-phandle' to specify the phandle in any device node
>>>> pointing to another node which contains the actual OPP tuples.
>>>> This helps to avoid replication if multiple devices share the OPPs.
>>>>
> 
> [snip]
> 
>>>> +    opps-table {
>>>> +            cpu_opp: cpu_opp {
>>>> +                    operating-points = <
>>>> +                            /* kHz    uV */
>>>> +                            792000  1100000
>>>> +                            396000  950000
>>>> +                            198000  850000
>>>> +                    >;
>>>> +            };
>>>> +            ... /* other device OPP nodes */
>>>
>>> But this is a subnode of /cpus. IMO, OPPs should be located near what
>>> they control.
>>>
>> No, the idea was to group all the shared OPPs in a container node like clocks or
>> pinmux. So opps-table in above example need not be subnode of /cpus.
> 
> Clocks are typically defined in a clock controller node. pinmux
> doesn't fit anywhere well so it is an exception. We don't want to
> expand on that. OPP's at least for cpu's would typically follow the
> topology, so put them there.
> 
I agree we don't want OPP to be another exception like pin-mux.

>>>> +    cpu3: cpu@101 {
>>>> +            compatible = "arm,cortex-a7";
>>>> +            reg = <101>;
>>>> +            operating-points-phandle = <&cluster1_opp>;
>>>> +    };
>>>> +
>>>> +    opps-table {
>>>> +            cluster0_opp: cluster0_opp {
>>>
>>> Why not use the cpu topology? Then the operating point can apply to
>>> cores based on the position in the topology. You don't even need a
>>> phandle in that case. You can look for OPPs in either a cpu node or in
>>> the topology.
>>>
>> Agreed but few thoughts behind this binding:
>>
>> 1. OPPs are not just cpu specific:
>>    How do we share OPPs for 2 devices in the same clock domain ?
>>    Also moving the OPP into cpu-topo makes parsing specific to cpu.
>>    Currently the OPP library fetches the of_node from the device struct
>>    which is applicable to any devices.
> 
> But an OPP for a cpu is cpu specific, so put it there. For devices, we
> may need something different. Perhaps it should be part of the clock
> binding in some way.
> 

I still don't understand why we need to handle cpu and other devices
differently. If you think that the devices sharing OPPs can be handled through
clock binding in some way, it should be equally applicable for CPUs too ?
In fact when I started this thread, even I had a similar thought and has asked
on the list[1]

>> 2. Should cpu topology(i.e. cpu-map) just contain the topology info ? and
>>    phandles to these nodes should be used to setup any affinity ?
> 
> Can't the OPP just be in the topology nodes at the appropriate level.
> Then the kernel can look for the OPPs in either place. Perhaps Lorenzo
> has thoughts on this.
> 

Ok will check with Lorenzo. I agree that we can extend topology binding so that
we add OPPs there. But I would like to avoid that if we think we can solve it
through clock bindings as mentioned above.

>> 3. As part of RFC[1][2], it was also discussed that on some SoCs we need
>>    multiple OPP profiles/options which it provides but only one of them can
>>    be used based on the board design. phandle to OPP will also help resolve
>>    that issue.
> 
> Then include the OPP required for that board design. You could have
> dtsi files of OPPs that can be included based on the board.
> 

I will let Nishanth comment on this.

Regards,
Sudeep

[1] http://marc.info/?l=linux-pm&m=137449807414601&w=4

Re: [PATCH v2 0/5] PM / OPP: updates to enable sharing OPPs info

[Sudeep KarkadaNagesha]

Hi Rafael,

On 17/10/13 00:12, Rafael J. Wysocki wrote:
> On Tuesday, October 01, 2013 02:32:57 PM Sudeep KarkadaNagesha wrote:
>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>>
>> Hi,
>>
>> These are few updates to existing device tree binding and the PM/OPP
>> library to support sharing of OPPs between different device nodes.
>>
>> Currently all the cpu nodes are parsed until the OPPs are found. This
>> is essential to support cpuhotplug without having to replicate OPP
>> information in all the cpu nodes.
>>
>> However in systems with multiple cpu power domain, its better to have
>> OPP entry for each cpu. To avoid replication, phandle can be specified
>> to the node which contains the full OPP information.
>>
>> The table node containing all the OPPs and device nodes pointing to an
>> entry into it through phandle might also solve the problem of describing
>> multiple OPP profiles.
> 
> It looks like this still is under discussion, isn't it?
> 
> If not, care to obtain ACKs from the people involved and resubmit?
> 

Thanks for following up on this patches. Since the entire series is based on the
DT binding in PATCH 1, which is still in discussion and subject to change, I
don't think its ready yet.

Regards,
Sudeep

Re: [PATCH v2 1/5] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Nishanth Menon]

On 10/17/2013 12:22 PM, Sudeep KarkadaNagesha wrote:
> On 17/10/13 14:22, Rob Herring wrote:
>> On Tue, Oct 8, 2013 at 7:55 AM, Sudeep KarkadaNagesha
>> <Sudeep.KarkadaNagesha@arm.com> wrote:
>>> On 07/10/13 17:01, Rob Herring wrote:
>>>> On 10/01/2013 08:32 AM, Sudeep KarkadaNagesha wrote:
[...]
>>> 3. As part of RFC[1][2], it was also discussed that on some SoCs we need
>>>    multiple OPP profiles/options which it provides but only one of them can
>>>    be used based on the board design. phandle to OPP will also help resolve
>>>    that issue.
>>
>> Then include the OPP required for that board design. You could have
>> dtsi files of OPPs that can be included based on the board.
>>
> 
> I will let Nishanth comment on this.

there are couple of angles to this[1] that Sudeep already pointed at:

A) SoCs like OMAP3430, 3630, 3530, 3730, omap4430, omap4460, omap4470,
omap5430, omap5432, there will at least be 2 dtsi *per* OPPset dtsi
per chip - and we have OPP sets per device inside the SoC - one for
CPU, one for GPU, one for IVA(accelerator), one for l3 (interconnect).
Further, when we go to newer variants like AM335x,AM437x,DRA7 - the
opp sets can increase to around 4/5 per domain per chip. Most of these
are deterministic based on Efused bit fields.
-> On this angle, arch/arm/mach-imx/mach-imx6q.c is a good example of
a similar challenge - here OPP enable/disable of hardcoded frequency
is kept in kernel, but data is picked up from SoC dts - not a good
story when we want to maintain old dtb compatibility - if frequency
changes in either dts/kernel, we wont function.
-> We are working internally to a potentially generic solution to
address this - not yet in a stage even for RFC -> if that is possible
to be done, then we dont need profiles to handle things, instead an
alternative constraint description is provided in dts. or we could
force folks to use specific dtsis OR allow generic handling based on
profiles.

B) now we can have a chip which can support a high frequency (efuse
will say so), however board characteristics(Power Distribution Network
requirements) may not be capable of allowing the chip to perform. We
can argue saying that "why would anyone put a higher performing chip
on a not-capable board?" - but we all know the reality of marketting
folks and cost of chip story.. but this is a real constraint when we
try to support our SoCs on as many platforms as possible and enabling
Linux on all those platforms.
-> We could use profiles to deal with this.
-> we could use dtsi override with OPP to deal with this
OR
-> we could use custom properties (something like
ti,non-optimal-board-design ;) ).

My personal preference is to state hardware capability solely in dts
and kernel is just operations on the dts data. OPP profiles did seem
to me to be the intutive way to do that job as it does describe
exactly wha the hardware capabilities were. Creating n different dtsis
is possible per device, but it essentially does describe the same
information.

[1] http://www.spinics.net/lists/cpufreq/msg06685.html
-- 
Regards,
Nishanth Menon

Re: [PATCH v2 1/5] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Lorenzo Pieralisi]

On Thu, Oct 17, 2013 at 02:22:44PM +0100, Rob Herring wrote:

[...]

> >>> +    cpu3: cpu@101 {
> >>> +            compatible = "arm,cortex-a7";
> >>> +            reg = <101>;
> >>> +            operating-points-phandle = <&cluster1_opp>;
> >>> +    };
> >>> +
> >>> +    opps-table {
> >>> +            cluster0_opp: cluster0_opp {
> >>
> >> Why not use the cpu topology? Then the operating point can apply to
> >> cores based on the position in the topology. You don't even need a
> >> phandle in that case. You can look for OPPs in either a cpu node or in
> >> the topology.
> >>
> > Agreed but few thoughts behind this binding:
> >
> > 1. OPPs are not just cpu specific:
> >    How do we share OPPs for 2 devices in the same clock domain ?
> >    Also moving the OPP into cpu-topo makes parsing specific to cpu.
> >    Currently the OPP library fetches the of_node from the device struct
> >    which is applicable to any devices.
> 
> But an OPP for a cpu is cpu specific, so put it there. For devices, we
> may need something different. Perhaps it should be part of the clock
> binding in some way.
> 
> > 2. Should cpu topology(i.e. cpu-map) just contain the topology info ? and
> >    phandles to these nodes should be used to setup any affinity ?
> 
> Can't the OPP just be in the topology nodes at the appropriate level.
> Then the kernel can look for the OPPs in either place. Perhaps Lorenzo
> has thoughts on this.

The reason we introduced the topology nodes (ie cpu-map) was to provide
a topology description of the system with leaf nodes linked to cpu
nodes; nodes in the topology do not map to HW entities.
Put it differently, IMHO we should attach no HW meaning to nodes in the
cpu-map so we should not add eg OPPs under cluster nodes because those
nodes do not represent HW entities, I would like to keep cpu-map a pure
DT representation of the topology, no more, but debate is open.

Lorenzo