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

[PATCH 0/3] 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.

An RFC version was posted [1] and based on the discussion it was
preferred to use phandle to a shared OPP table in a separate node
rather than creating indirect linkage to the device with which it share
the clock as it could be problematic. E.g. the bootloaders can remove
cpu nodes if there are not used.

Also I am retaining 'operating-points-phandle' property name, as it makes
more sense now.

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

[1] http://www.spinics.net/lists/cpufreq/msg06524.html

Sudeep KarkadaNagesha (3):
  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

 Documentation/devicetree/bindings/power/opp.txt | 152 ++++++++++++++++++++++--
 drivers/base/power/opp.c                        |  18 ++-
 2 files changed, 156 insertions(+), 14 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 1/3] 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: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@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 | 152 ++++++++++++++++++++++--
 1 file changed, 140 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
index 74499e5..e9fea65 100644
--- a/Documentation/devicetree/bindings/power/opp.txt
+++ b/Documentation/devicetree/bindings/power/opp.txt
@@ -4,22 +4,150 @@ 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
 
+Optional properties:
+- 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)
+
 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
+		>;
+	};
+
+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 need to
+avoid replicating the OPPs in all the nodes. We can specify the phandle
+of the node which contains the OPP tuples
+
+2a. Consider a SMP system with 4 CPUs in the same clock domain
+    (backward compatible style, only CPU0 contains OPP)
+
+	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
+    (using operating-points-phandle)
+
+	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>;
+	};
+
+	operating_points {
+		cpu_opp: cpu_opp {
+			operating-points = <
+				/* kHz    uV */
+				792000  1100000
+				396000  950000
+				198000  850000
+			>;
+		};
+		... /* other device OPP nodes */
+	}
+
+3. 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>;
+	};
+
+	operating_points {
+		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 */
+	}
-- 
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 2/3] PM / OPP: add support to specify phandle of another node for OPP

[Sudeep KarkadaNagesha]

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: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@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 | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index ef89897..a450e2f 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;
 
-- 
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 3/3] 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: "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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index a450e2f..0ebd884 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

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

[Mark Rutland]

Hi Sudeep,

I have a few comments.

On Wed, Sep 18, 2013 at 11:58:11AM +0100, Sudeep KarkadaNagesha wrote:
> 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: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@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 | 152 ++++++++++++++++++++++--
>  1 file changed, 140 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
> index 74499e5..e9fea65 100644
> --- a/Documentation/devicetree/bindings/power/opp.txt
> +++ b/Documentation/devicetree/bindings/power/opp.txt
> @@ -4,22 +4,150 @@ 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
>  
> +Optional properties:
> +- 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)
> +

I assume if you have an operating-points-phandle property,
operating-points it no longer required. That should probably be
described.

>  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
> +		>;
> +	};
> +
> +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 need to
> +avoid replicating the OPPs in all the nodes. We can specify the phandle
> +of the node which contains the OPP tuples

This seems a bit out of place given the example immediately below.

> +
> +2a. Consider a SMP system with 4 CPUs in the same clock domain
> +    (backward compatible style, only CPU0 contains OPP)
> +
> +	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>;
> +	};

This "backward compatible style" doesn't seem to actually be described
anywhere, and the paragraph above about phandles makes it somewhat
confusing.

> +
> +2b. Consider a SMP system with 4 CPUs in the same clock domain
> +    (using operating-points-phandle)
> +
> +	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>;
> +	};
> +
> +	operating_points {
> +		cpu_opp: cpu_opp {
> +			operating-points = <
> +				/* kHz    uV */
> +				792000  1100000
> +				396000  950000
> +				198000  850000
> +			>;
> +		};
> +		... /* other device OPP nodes */
> +	}

Is this all inside the /cpus node?

Is the "operating_points" name important?

Are all OPP tables expected to be in the same "operating_points" node?

Cheers,
Mark.

> +
> +3. 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>;
> +	};
> +
> +	operating_points {
> +		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 */
> +	}
> -- 
> 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 2/3] PM / OPP: add support to specify phandle of another node for OPP

[Mark Rutland]

On Wed, Sep 18, 2013 at 11:58:12AM +0100, 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: Kumar Gala <galak@codeaurora.org>
> 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 | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index ef89897..a450e2f 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);

This will increment the refcount of the node pointed to.

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

>From the looks of mainline the table gets parsed here, and then the node
is never used again.

At the end there should probably be an of_node_put on the opp_node (if
it's from an operating-points-phandle property).

Cheers,
Mark.

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

[Sudeep KarkadaNagesha]

On 30/09/13 09:52, Mark Rutland wrote:
> Hi Sudeep,
> 
> I have a few comments.
> 
> On Wed, Sep 18, 2013 at 11:58:11AM +0100, 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: Kumar Gala <galak@codeaurora.org>
>> 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 | 152 ++++++++++++++++++++++--
>>  1 file changed, 140 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
>> index 74499e5..e9fea65 100644
>> --- a/Documentation/devicetree/bindings/power/opp.txt
>> +++ b/Documentation/devicetree/bindings/power/opp.txt
>> @@ -4,22 +4,150 @@ 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
>>  
>> +Optional properties:
>> +- 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)
>> +
> 
> I assume if you have an operating-points-phandle property,
> operating-points it no longer required. That should probably be
> described.
> 
Correct, infact do it make sense to list both operating-points and
operating-points-phandle as required property but they are mutually exclusive ?

>>  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
>> +		>;
>> +	};
>> +
>> +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 need to
>> +avoid replicating the OPPs in all the nodes. We can specify the phandle
>> +of the node which contains the OPP tuples
> 
> This seems a bit out of place given the example immediately below.
> 
Sorry carried from previous version, will fix it.

>> +
>> +2a. Consider a SMP system with 4 CPUs in the same clock domain
>> +    (backward compatible style, only CPU0 contains OPP)
>> +
>> +	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>;
>> +	};
> 
> This "backward compatible style" doesn't seem to actually be described
> anywhere, and the paragraph above about phandles makes it somewhat
> confusing.
> 
Not sure if this is needed at all here. I must say there's nothing called
backward compatible style, the only reason I added it to tell that existing
cpufreq-cpu0 DTs continue to work.

I would say it's more like some agreement with the existing binding and the
cpufreq-cpu0 driver. Do it make sense to drop it from here ?

>> +
>> +2b. Consider a SMP system with 4 CPUs in the same clock domain
>> +    (using operating-points-phandle)
>> +
>> +	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>;
>> +	};
>> +
>> +	operating_points {
>> +		cpu_opp: cpu_opp {
>> +			operating-points = <
>> +				/* kHz    uV */
>> +				792000  1100000
>> +				396000  950000
>> +				198000  850000
>> +			>;
>> +		};
>> +		... /* other device OPP nodes */
>> +	}
> 
> Is this all inside the /cpus node?
> 
It can be anywhere, if only cpus share OPPs on a system, it can be placed under
/cpus. The idea to put all OPP nodes under one node is just for readibility
purposes. As along as phandle is correct it doesn't matter where the exact nodes
are. Does this need to be mentioned explicity in the binding ?

> Is the "operating_points" name important?
> 
> Are all OPP tables expected to be in the same "operating_points" node?
> 
No for both the above questions, again just for readibilty I chose that name and
placed all OPP nodes under same node in the example, it can be named anything
and can be scattered.
Again does this need to be mentioned explicity in the binding ?

Regards,
Sudeep

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

[Mark Rutland]

On Mon, Sep 30, 2013 at 01:33:48PM +0100, Sudeep KarkadaNagesha wrote:
> On 30/09/13 09:52, Mark Rutland wrote:
> > Hi Sudeep,
> > 
> > I have a few comments.
> > 
> > On Wed, Sep 18, 2013 at 11:58:11AM +0100, Sudeep KarkadaNagesha wrote:
> >> 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: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@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 | 152 ++++++++++++++++++++++--
> >>  1 file changed, 140 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
> >> index 74499e5..e9fea65 100644
> >> --- a/Documentation/devicetree/bindings/power/opp.txt
> >> +++ b/Documentation/devicetree/bindings/power/opp.txt
> >> @@ -4,22 +4,150 @@ 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
> >>  
> >> +Optional properties:
> >> +- 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)
> >> +
> > 
> > I assume if you have an operating-points-phandle property,
> > operating-points it no longer required. That should probably be
> > described.
> > 
> Correct, infact do it make sense to list both operating-points and
> operating-points-phandle as required property but they are mutually exclusive ?

That sounds about right to me.

> 
> >>  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
> >> +		>;
> >> +	};
> >> +
> >> +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 need to
> >> +avoid replicating the OPPs in all the nodes. We can specify the phandle
> >> +of the node which contains the OPP tuples
> > 
> > This seems a bit out of place given the example immediately below.
> > 
> Sorry carried from previous version, will fix it.

Cheers.

> 
> >> +
> >> +2a. Consider a SMP system with 4 CPUs in the same clock domain
> >> +    (backward compatible style, only CPU0 contains OPP)
> >> +
> >> +	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>;
> >> +	};
> > 
> > This "backward compatible style" doesn't seem to actually be described
> > anywhere, and the paragraph above about phandles makes it somewhat
> > confusing.
> > 
> Not sure if this is needed at all here. I must say there's nothing called
> backward compatible style, the only reason I added it to tell that existing
> cpufreq-cpu0 DTs continue to work.
> 
> I would say it's more like some agreement with the existing binding and the
> cpufreq-cpu0 driver. Do it make sense to drop it from here ?

It's probably worth adding a paragraph above the example describing
that, something like:

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.

> 
> >> +
> >> +2b. Consider a SMP system with 4 CPUs in the same clock domain
> >> +    (using operating-points-phandle)
> >> +
> >> +	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>;
> >> +	};
> >> +
> >> +	operating_points {
> >> +		cpu_opp: cpu_opp {
> >> +			operating-points = <
> >> +				/* kHz    uV */
> >> +				792000  1100000
> >> +				396000  950000
> >> +				198000  850000
> >> +			>;
> >> +		};
> >> +		... /* other device OPP nodes */
> >> +	}
> > 
> > Is this all inside the /cpus node?
> > 
> It can be anywhere, if only cpus share OPPs on a system, it can be placed under
> /cpus. The idea to put all OPP nodes under one node is just for readibility
> purposes. As along as phandle is correct it doesn't matter where the exact nodes
> are. Does this need to be mentioned explicity in the binding ?

I'm not sure what the preferred way of handling container nodes is in
general. Do we have any examples of existing container nodes (i.e. those
which aren't devices and don't contain devices) elsewhere?

> 
> > Is the "operating_points" name important?
> > 
> > Are all OPP tables expected to be in the same "operating_points" node?
> > 
> No for both the above questions, again just for readibilty I chose that name and
> placed all OPP nodes under same node in the example, it can be named anything
> and can be scattered.
> Again does this need to be mentioned explicity in the binding ?

This would depend on the preferred way of handling container nodes. If
we want them all in one place, that needs to be documented. If we allow
them anywhere, that should also be documented (though some guidance
should probably be given so as to encourage uniform DTs).

Cheers,
Mark.
--
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 1/3] PM / OPP: extend DT binding to specify phandle of another node for OPP

[Sudeep KarkadaNagesha]

On 30/09/13 16:51, Mark Rutland wrote:
> On Mon, Sep 30, 2013 at 01:33:48PM +0100, Sudeep KarkadaNagesha wrote:
>> On 30/09/13 09:52, Mark Rutland wrote:
>>> Hi Sudeep,
>>>
>>> I have a few comments.
>>>
>>> On Wed, Sep 18, 2013 at 11:58:11AM +0100, Sudeep KarkadaNagesha wrote:
[...]
>>>> +
>>>> +2a. Consider a SMP system with 4 CPUs in the same clock domain
>>>> +    (backward compatible style, only CPU0 contains OPP)
>>>> +
>>>> +	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>;
>>>> +	};
>>>
>>> This "backward compatible style" doesn't seem to actually be described
>>> anywhere, and the paragraph above about phandles makes it somewhat
>>> confusing.
>>>
>> Not sure if this is needed at all here. I must say there's nothing called
>> backward compatible style, the only reason I added it to tell that existing
>> cpufreq-cpu0 DTs continue to work.
>>
>> I would say it's more like some agreement with the existing binding and the
>> cpufreq-cpu0 driver. Do it make sense to drop it from here ?
> 
> It's probably worth adding a paragraph above the example describing
> that, something like:
> 
> 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.
> 
That sounds good, will fix this in next version.

>>
>>>> +
>>>> +2b. Consider a SMP system with 4 CPUs in the same clock domain
>>>> +    (using operating-points-phandle)
>>>> +
>>>> +	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>;
>>>> +	};
>>>> +
>>>> +	operating_points {
>>>> +		cpu_opp: cpu_opp {
>>>> +			operating-points = <
>>>> +				/* kHz    uV */
>>>> +				792000  1100000
>>>> +				396000  950000
>>>> +				198000  850000
>>>> +			>;
>>>> +		};
>>>> +		... /* other device OPP nodes */
>>>> +	}
>>>
>>> Is this all inside the /cpus node?
>>>
>> It can be anywhere, if only cpus share OPPs on a system, it can be placed under
>> /cpus. The idea to put all OPP nodes under one node is just for readibility
>> purposes. As along as phandle is correct it doesn't matter where the exact nodes
>> are. Does this need to be mentioned explicity in the binding ?
> 
> I'm not sure what the preferred way of handling container nodes is in
> general. Do we have any examples of existing container nodes (i.e. those
> which aren't devices and don't contain devices) elsewhere?
> 
The closest match I can think of is clocks or pinmux container node in many DTs.
However I see that clock container nodes can hold generic properties like
#address-cell and #size-cells that are applicable for all the nodes in them.
I could not find any documentation on them either :(

>>
>>> Is the "operating_points" name important?
>>>
>>> Are all OPP tables expected to be in the same "operating_points" node?
>>>
>> No for both the above questions, again just for readibilty I chose that name and
>> placed all OPP nodes under same node in the example, it can be named anything
>> and can be scattered.
>> Again does this need to be mentioned explicity in the binding ?
> 
> This would depend on the preferred way of handling container nodes. If
> we want them all in one place, that needs to be documented. If we allow
> them anywhere, that should also be documented (though some guidance
> should probably be given so as to encourage uniform DTs).
> 
Makes sense. I can document recommending to have all the OPPs in single node.
But it's not mandatory as it doesn't matter for OPPs as long as the phandles are
correct. However if it helps for validating DTs, then can we mandate it?

Regards,
Sudeep

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