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

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

[Sudeep.KarkadaNagesha]

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

These are couple of updates to existing 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.

Sudeep KarkadaNagesha (2):
  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 |   41 +++++++++++++++++++++++
 drivers/base/power/opp.c                        |   35 ++++++++++++++-----
 2 files changed, 68 insertions(+), 8 deletions(-)

-- 
1.7.10.4

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

[Sudeep.KarkadaNagesha]

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

If more than one similar devices share the same OPPs, currently we
need to replicate the OPP entries in all the nodes.

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 with which the current node shares the operating points.

This patch adds support to specify the phandle in the operating points
of any device node, where the node specified by the phandle holds the
actual OPPs.

Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
---
 Documentation/devicetree/bindings/power/opp.txt |   41 +++++++++++++++++++++++
 drivers/base/power/opp.c                        |   30 ++++++++++++-----
 2 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
index 74499e5..a659da4 100644
--- a/Documentation/devicetree/bindings/power/opp.txt
+++ b/Documentation/devicetree/bindings/power/opp.txt
@@ -23,3 +23,44 @@ cpu@0 {
 		198000  850000
 	>;
 };
+
+If more than one device of same type share the same OPPs, e.g. 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 with which the
+current node shares the operating points instead.
+
+Examples:
+Consider an SMP with 4 CPUs all sharing the same OPPs.
+
+cpu0: cpu@0 {
+	compatible = "arm,cortex-a9";
+	reg = <0>;
+	next-level-cache = <&L2>;
+	operating-points = <
+		/* kHz    uV */
+		792000  1100000
+		396000  950000
+		198000  850000
+	>;
+};
+
+cpu1: cpu@1 {
+	compatible = "arm,cortex-a9";
+	reg = <1>;
+	next-level-cache = <&L2>;
+	operating-points = <&cpu0>;
+};
+
+cpu2: cpu@2 {
+	compatible = "arm,cortex-a9";
+	reg = <2>;
+	next-level-cache = <&L2>;
+	operating-points = <&cpu0>;
+};
+
+cpu3: cpu@3 {
+	compatible = "arm,cortex-a9";
+	reg = <3>;
+	next-level-cache = <&L2>;
+	operating-points = <&cpu0>;
+};
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index f0077cb..4dfdc01 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -698,19 +698,15 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
 }
 
 #ifdef CONFIG_OF
-/**
- * of_init_opp_table() - Initialize opp table from device tree
- * @dev:	device pointer used to lookup device OPPs.
- *
- * Register the initial OPP table with the OPP library for given device.
- */
-int of_init_opp_table(struct device *dev)
+static int of_init_opp_table_from_ofnode(struct device *dev,
+					struct device_node *of_node)
 {
+	struct device_opp *dev_opp = NULL;
 	const struct property *prop;
 	const __be32 *val;
 	int nr;
 
-	prop = of_find_property(dev->of_node, "operating-points", NULL);
+	prop = of_find_property(of_node, "operating-points", NULL);
 	if (!prop)
 		return -ENODEV;
 	if (!prop->value)
@@ -722,6 +718,14 @@ int of_init_opp_table(struct device *dev)
 	 */
 	nr = prop->length / sizeof(u32);
 	if (nr % 2) {
+		if (nr == 1) {
+			struct device_node *opp_node;
+			opp_node = of_parse_phandle(dev->of_node,
+						"operating-points", 0);
+			if (opp_node)
+				return of_init_opp_table_from_ofnode(dev,
+								opp_node);
+		}
 		dev_err(dev, "%s: Invalid OPP list\n", __func__);
 		return -EINVAL;
 	}
@@ -741,5 +745,15 @@ int of_init_opp_table(struct device *dev)
 
 	return 0;
 }
+/**
+ * of_init_opp_table() - Initialize opp table from device tree
+ * @dev:	device pointer used to lookup device OPPs.
+ *
+ * Register the initial OPP table with the OPP library for given device.
+ */
+int of_init_opp_table(struct device *dev)
+{
+	return of_init_opp_table_from_ofnode(dev, dev->of_node);
+}
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 #endif
-- 
1.7.10.4

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

[Nishanth Menon]

On 12:11-20130501, Sudeep.KarkadaNagesha@arm.com wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> 
> If more than one similar devices share the same OPPs, currently we
> need to replicate the OPP entries in all the nodes.
Nice, thanks.
> 
> Few drivers like cpufreq depend on physical cpu0 node to specify the
cpufreq-cpu0?
> 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 with which the current node shares the operating points.
> 
> This patch adds support to specify the phandle in the operating points
> of any device node, where the node specified by the phandle holds the
> actual OPPs.
> 
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> ---
>  Documentation/devicetree/bindings/power/opp.txt |   41 +++++++++++++++++++++++
>  drivers/base/power/opp.c                        |   30 ++++++++++++-----
>  2 files changed, 63 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
> index 74499e5..a659da4 100644
> --- a/Documentation/devicetree/bindings/power/opp.txt
> +++ b/Documentation/devicetree/bindings/power/opp.txt
> @@ -23,3 +23,44 @@ cpu@0 {
>  		198000  850000
>  	>;
>  };
> +
Definition of operating-points is now a little different in the
original description - it still indicates tuple {freq,voltage}, where
as, this patch allows phandle to a different device's operating-points
to be used. - we might want to rephrase the description.

btw, to device-tree folks, I am not sure if it is OK to have different formats
for the same property like operating-points. At least I don't seem to
quickly be able to find any precedence.

> +If more than one device of same type share the same OPPs, e.g. all the CPUs on
s/e.g/example?
> +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 with which the
> +current node shares the operating points instead.
> +
> +Examples:
> +Consider an SMP with 4 CPUs all sharing the same OPPs.
We might want to descr
> +
> +cpu0: cpu@0 {
> +	compatible = "arm,cortex-a9";
> +	reg = <0>;
> +	next-level-cache = <&L2>;
> +	operating-points = <
> +		/* kHz    uV */
> +		792000  1100000
> +		396000  950000
> +		198000  850000
> +	>;
> +};
> +
> +cpu1: cpu@1 {
> +	compatible = "arm,cortex-a9";
> +	reg = <1>;
> +	next-level-cache = <&L2>;
> +	operating-points = <&cpu0>;
> +};
> +
> +cpu2: cpu@2 {
> +	compatible = "arm,cortex-a9";
> +	reg = <2>;
> +	next-level-cache = <&L2>;
> +	operating-points = <&cpu0>;
> +};
> +
> +cpu3: cpu@3 {
> +	compatible = "arm,cortex-a9";
> +	reg = <3>;
> +	next-level-cache = <&L2>;
> +	operating-points = <&cpu0>;
> +};
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index f0077cb..4dfdc01 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -698,19 +698,15 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
>  }
>  
>  #ifdef CONFIG_OF
> -/**
> - * of_init_opp_table() - Initialize opp table from device tree
> - * @dev:	device pointer used to lookup device OPPs.
> - *
> - * Register the initial OPP table with the OPP library for given device.
> - */
> -int of_init_opp_table(struct device *dev)
> +static int of_init_opp_table_from_ofnode(struct device *dev,
> +					struct device_node *of_node)
please provide kernel-doc documentation for static function as well -
this is inline with the rest of the file.
>  {
> +	struct device_opp *dev_opp = NULL;
dev_opp is not used until patch #2 - please introduce it in that patch.
>  	const struct property *prop;
>  	const __be32 *val;
>  	int nr;
>  
> -	prop = of_find_property(dev->of_node, "operating-points", NULL);
> +	prop = of_find_property(of_node, "operating-points", NULL);
>  	if (!prop)
>  		return -ENODEV;
>  	if (!prop->value)
> @@ -722,6 +718,14 @@ int of_init_opp_table(struct device *dev)
>  	 */
>  	nr = prop->length / sizeof(u32);
>  	if (nr % 2) {
> +		if (nr == 1) {
> +			struct device_node *opp_node;
> +			opp_node = of_parse_phandle(dev->of_node,
> +						"operating-points", 0);
> +			if (opp_node)
> +				return of_init_opp_table_from_ofnode(dev,
> +								opp_node);
> +		}
if operating-points=<100000>, then we return Invalid OPP list
if operating-points=<&uart3>; (some phandle that does not have
operating-points), there is no helpful warning in log except -ENODEV is
returned - we might want to add some info here?
>  		dev_err(dev, "%s: Invalid OPP list\n", __func__);
>  		return -EINVAL;
>  	}
> @@ -741,5 +745,15 @@ int of_init_opp_table(struct device *dev)
>  
>  	return 0;
>  }
missing EOL?
> +/**
> + * of_init_opp_table() - Initialize opp table from device tree
> + * @dev:	device pointer used to lookup device OPPs.
> + *
> + * Register the initial OPP table with the OPP library for given device.
> + */
> +int of_init_opp_table(struct device *dev)
> +{
> +	return of_init_opp_table_from_ofnode(dev, dev->of_node);
> +}
>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>  #endif
> -- 
> 1.7.10.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Regards,
Nishanth Menon

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

[Sudeep KarkadaNagesha]

On 01/05/13 15:41, Nishanth Menon wrote:
> On 12:11-20130501, Sudeep.KarkadaNagesha@arm.com wrote:
>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>>
>> If more than one similar devices share the same OPPs, currently we
>> need to replicate the OPP entries in all the nodes.
> Nice, thanks.
>>
>> Few drivers like cpufreq depend on physical cpu0 node to specify the
> cpufreq-cpu0?
Yes when I originally wrote this patch cpufreq-cpu0 was using cpu0 node.
But later sometimes it was changed to parse all the nodes.

>> 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 with which the current node shares the operating points.
>>
>> This patch adds support to specify the phandle in the operating points
>> of any device node, where the node specified by the phandle holds the
>> actual OPPs.
>>
>> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>> ---
>>  Documentation/devicetree/bindings/power/opp.txt |   41 +++++++++++++++++++++++
>>  drivers/base/power/opp.c                        |   30 ++++++++++++-----
>>  2 files changed, 63 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
>> index 74499e5..a659da4 100644
>> --- a/Documentation/devicetree/bindings/power/opp.txt
>> +++ b/Documentation/devicetree/bindings/power/opp.txt
>> @@ -23,3 +23,44 @@ cpu@0 {
>>  		198000  850000
>>  	>;
>>  };
>> +
> Definition of operating-points is now a little different in the
> original description - it still indicates tuple {freq,voltage}, where
> as, this patch allows phandle to a different device's operating-points
> to be used. - we might want to rephrase the description.
> 
> btw, to device-tree folks, I am not sure if it is OK to have different formats
> for the same property like operating-points. At least I don't seem to
> quickly be able to find any precedence.
> 
>> +If more than one device of same type share the same OPPs, e.g. all the CPUs on
> s/e.g/example?
Ok

>> +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 with which the
>> +current node shares the operating points instead.
>> +
>> +Examples:
>> +Consider an SMP with 4 CPUs all sharing the same OPPs.
> We might want to descr
I could not get what exactly you mean by 'descr', do you mean more
descriptive ? If so, what exactly you would like to add ?

>> +
>> +cpu0: cpu@0 {
>> +	compatible = "arm,cortex-a9";
>> +	reg = <0>;
>> +	next-level-cache = <&L2>;
>> +	operating-points = <
>> +		/* kHz    uV */
>> +		792000  1100000
>> +		396000  950000
>> +		198000  850000
>> +	>;
>> +};
>> +
>> +cpu1: cpu@1 {
>> +	compatible = "arm,cortex-a9";
>> +	reg = <1>;
>> +	next-level-cache = <&L2>;
>> +	operating-points = <&cpu0>;
>> +};
>> +
>> +cpu2: cpu@2 {
>> +	compatible = "arm,cortex-a9";
>> +	reg = <2>;
>> +	next-level-cache = <&L2>;
>> +	operating-points = <&cpu0>;
>> +};
>> +
>> +cpu3: cpu@3 {
>> +	compatible = "arm,cortex-a9";
>> +	reg = <3>;
>> +	next-level-cache = <&L2>;
>> +	operating-points = <&cpu0>;
>> +};
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index f0077cb..4dfdc01 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -698,19 +698,15 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
>>  }
>>  
>>  #ifdef CONFIG_OF
>> -/**
>> - * of_init_opp_table() - Initialize opp table from device tree
>> - * @dev:	device pointer used to lookup device OPPs.
>> - *
>> - * Register the initial OPP table with the OPP library for given device.
>> - */
>> -int of_init_opp_table(struct device *dev)
>> +static int of_init_opp_table_from_ofnode(struct device *dev,
>> +					struct device_node *of_node)
> please provide kernel-doc documentation for static function as well -
> this is inline with the rest of the file.
Ok

>>  {
>> +	struct device_opp *dev_opp = NULL;
> dev_opp is not used until patch #2 - please introduce it in that patch.
Correct it was meant to be in patch#2, will move in next version

>>  	const struct property *prop;
>>  	const __be32 *val;
>>  	int nr;
>>  
>> -	prop = of_find_property(dev->of_node, "operating-points", NULL);
>> +	prop = of_find_property(of_node, "operating-points", NULL);
>>  	if (!prop)
>>  		return -ENODEV;
>>  	if (!prop->value)
>> @@ -722,6 +718,14 @@ int of_init_opp_table(struct device *dev)
>>  	 */
>>  	nr = prop->length / sizeof(u32);
>>  	if (nr % 2) {
>> +		if (nr == 1) {
>> +			struct device_node *opp_node;
>> +			opp_node = of_parse_phandle(dev->of_node,
>> +						"operating-points", 0);
>> +			if (opp_node)
>> +				return of_init_opp_table_from_ofnode(dev,
>> +								opp_node);
>> +		}
> if operating-points=<100000>, then we return Invalid OPP list
> if operating-points=<&uart3>; (some phandle that does not have
> operating-points), there is no helpful warning in log except -ENODEV is
> returned - we might want to add some info here?
Makes sense, will add that in next version.

>>  		dev_err(dev, "%s: Invalid OPP list\n", __func__);
>>  		return -EINVAL;
>>  	}
>> @@ -741,5 +745,15 @@ int of_init_opp_table(struct device *dev)
>>  
>>  	return 0;
>>  }
> missing EOL?
Ok

>> +/**
>> + * of_init_opp_table() - Initialize opp table from device tree
>> + * @dev:	device pointer used to lookup device OPPs.
>> + *
>> + * Register the initial OPP table with the OPP library for given device.
>> + */
>> +int of_init_opp_table(struct device *dev)
>> +{
>> +	return of_init_opp_table_from_ofnode(dev, dev->of_node);
>> +}
>>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>>  #endif
>> -- 
>> 1.7.10.4
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Nishanth Menon]

On 17:28-20130501, Sudeep KarkadaNagesha wrote:
> On 01/05/13 15:41, Nishanth Menon wrote:
> > On 12:11-20130501, Sudeep.KarkadaNagesha@arm.com wrote:
> >> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> >>
> >> If more than one similar devices share the same OPPs, currently we
> >> need to replicate the OPP entries in all the nodes.
> > Nice, thanks.
> >>
> >> Few drivers like cpufreq depend on physical cpu0 node to specify the
> > cpufreq-cpu0?
> Yes when I originally wrote this patch cpufreq-cpu0 was using cpu0 node.
> But later sometimes it was changed to parse all the nodes.
giving an explicit example like cpufreq-cpu0 might be helpful - but we
may have more cases like this else where with co-processors (devfreq
world).
[...]
> >> +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 with which the
> >> +current node shares the operating points instead.
> >> +
> >> +Examples:
> >> +Consider an SMP with 4 CPUs all sharing the same OPPs.
> > We might want to descr
> I could not get what exactly you mean by 'descr', do you mean more
> descriptive ? If so, what exactly you would like to add ?
Arrgh.. Typical of me to forget my reply thread and leave it dangling
when someone interrupts me :( Sigh.. Apologies about that.

I intended the following:
An example of bL might be 4 A15s and 3 a7s? A15s have different
frequencies Vs A7s?
The example below could easily be covered by cpufreq-cpu0 - so an actual
usage illustrated will help.
> 
> >> +
> >> +cpu0: cpu@0 {
> >> +	compatible = "arm,cortex-a9";
> >> +	reg = <0>;
> >> +	next-level-cache = <&L2>;
> >> +	operating-points = <
> >> +		/* kHz    uV */
> >> +		792000  1100000
> >> +		396000  950000
> >> +		198000  850000
> >> +	>;
> >> +};
> >> +
> >> +cpu1: cpu@1 {
> >> +	compatible = "arm,cortex-a9";
> >> +	reg = <1>;
> >> +	next-level-cache = <&L2>;
> >> +	operating-points = <&cpu0>;
> >> +};
[...]
> 
> >>  {
> >> +	struct device_opp *dev_opp = NULL;
> > dev_opp is not used until patch #2 - please introduce it in that patch.
> Correct it was meant to be in patch#2, will move in next version
I'd hold on to the rev 2 until we are clear that we have precedence for
allowing parameters to have two different formats.

I might optionally go with introducing a new parameter to indicate
phandle lists similar to pinctrl.. Just a thought. There may be other
usage for the same in addition to resolving the scenario you mention.
Trivial example:
cpu0 can take three different operating-point sets ->
default - 300MHz, 600MHz, 800MHz
regular performance - 300MHz, 600MHz, 800MHz, 1GHz
performance - 300MHz, 600MHz, 800MHz, 1GHz, 1.7GHz

Choice is made which set it picks up.
making operating-points as a phandle by itself is questionable as it
does not really represent an hardware device - but options for
operation.

-- 
Regards,
Nishanth Menon

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

[Sudeep KarkadaNagesha]

Hi device-tree folks,

On 01/05/13 15:41, Nishanth Menon wrote:
> On 12:11-20130501, Sudeep.KarkadaNagesha@arm.com wrote:
>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>>
>> If more than one similar devices share the same OPPs, currently we
>> need to replicate the OPP entries in all the nodes.
> Nice, thanks.
>>
>> Few drivers like cpufreq depend on physical cpu0 node to specify the
> cpufreq-cpu0?
>> 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 with which the current node shares the operating points.
>>
>> This patch adds support to specify the phandle in the operating points
>> of any device node, where the node specified by the phandle holds the
>> actual OPPs.
>>
>> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>> ---
>>  Documentation/devicetree/bindings/power/opp.txt |   41 +++++++++++++++++++++++
>>  drivers/base/power/opp.c                        |   30 ++++++++++++-----
>>  2 files changed, 63 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
>> index 74499e5..a659da4 100644
>> --- a/Documentation/devicetree/bindings/power/opp.txt
>> +++ b/Documentation/devicetree/bindings/power/opp.txt
>> @@ -23,3 +23,44 @@ cpu@0 {
>>              198000  850000
>>      >;
>>  };
>> +
> Definition of operating-points is now a little different in the
> original description - it still indicates tuple {freq,voltage}, where
> as, this patch allows phandle to a different device's operating-points
> to be used. - we might want to rephrase the description.
>
> btw, to device-tree folks, I am not sure if it is OK to have different formats
> for the same property like operating-points. At least I don't seem to
> quickly be able to find any precedence.
>
Any directions on this to proceed ? Is proposed option of phandle for
OPP acceptable ?

Regards,
Sudeep

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

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

[Sudeep.KarkadaNagesha]

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

CPUs are registered as devices and their OPPs can be initialised from
the device tree. Whenever CPUs can be 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. As a result of this the cpufreq will try 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.

Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
---
 drivers/base/power/opp.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 4dfdc01..66d52d2 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -706,6 +706,11 @@ static int of_init_opp_table_from_ofnode(struct device *dev,
 	const __be32 *val;
 	int nr;
 
+	/* Check for existing list for 'dev' */
+	dev_opp = find_device_opp(dev);
+	if (!IS_ERR(dev_opp))
+		return 0; /* Device OPP already initialized */
+
 	prop = of_find_property(of_node, "operating-points", NULL);
 	if (!prop)
 		return -ENODEV;
-- 
1.7.10.4

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

[Nishanth Menon]

On 12:11-20130501, 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 can be 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. As a result of this the cpufreq will try 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.
> 
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> ---
>  drivers/base/power/opp.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 4dfdc01..66d52d2 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -706,6 +706,11 @@ static int of_init_opp_table_from_ofnode(struct device *dev,
>  	const __be32 *val;
>  	int nr;
>  
> +	/* Check for existing list for 'dev' */
> +	dev_opp = find_device_opp(dev);
> +	if (!IS_ERR(dev_opp))
> +		return 0; /* Device OPP already initialized */
> +
It gets a little touchy here -> the normal expectation is for the OPP
entries to be populated onetime at boot.
For example - driver bug where same device was attempted twice Vs the
usecase you mention here - how'd we differentiate between the two?
>  	prop = of_find_property(of_node, "operating-points", NULL);
>  	if (!prop)
>  		return -ENODEV;
> -- 
> 1.7.10.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Regards,
Nishanth Menon

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

[Sudeep KarkadaNagesha]

On 01/05/13 16:04, Nishanth Menon wrote:
> On 12:11-20130501, 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 can be 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. As a result of this the cpufreq will try 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.
>>
>> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>> ---
>>  drivers/base/power/opp.c |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index 4dfdc01..66d52d2 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -706,6 +706,11 @@ static int of_init_opp_table_from_ofnode(struct device *dev,
>>  	const __be32 *val;
>>  	int nr;
>>  
>> +	/* Check for existing list for 'dev' */
>> +	dev_opp = find_device_opp(dev);
>> +	if (!IS_ERR(dev_opp))
>> +		return 0; /* Device OPP already initialized */
>> +
> It gets a little touchy here -> the normal expectation is for the OPP
> entries to be populated onetime at boot.
> For example - driver bug where same device was attempted twice Vs the
> usecase you mention here - how'd we differentiate between the two?

Do we really need to differentiate ? How about returning -EEXIST ?

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

[Nishanth Menon]

On 17:33-20130501, Sudeep KarkadaNagesha wrote:
> On 01/05/13 16:04, Nishanth Menon wrote:
> > On 12:11-20130501, Sudeep.KarkadaNagesha@arm.com wrote:
> >> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
[...]
> >> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> >> index 4dfdc01..66d52d2 100644
> >> --- a/drivers/base/power/opp.c
> >> +++ b/drivers/base/power/opp.c
> >> @@ -706,6 +706,11 @@ static int of_init_opp_table_from_ofnode(struct device *dev,
> >>  	const __be32 *val;
> >>  	int nr;
> >>  
> >> +	/* Check for existing list for 'dev' */
> >> +	dev_opp = find_device_opp(dev);
> >> +	if (!IS_ERR(dev_opp))
> >> +		return 0; /* Device OPP already initialized */
> >> +
> > It gets a little touchy here -> the normal expectation is for the OPP
> > entries to be populated onetime at boot.
> > For example - driver bug where same device was attempted twice Vs the
> > usecase you mention here - how'd we differentiate between the two?
> 
> Do we really need to differentiate ? How about returning -EEXIST ?
We have tried to provide enough debug information for developers to
detect and fix their mistakes, error value is one part of the story for
callers, error messages emphasis on top of it for the developer.. But,
for some reason I might be led to believe probe and hotplug are separate
usecases - probe of a device indicates it's presence, and hotplug of a
device should ideally be a power state.. But that is just me.
-- 
Regards,
Nishanth Menon

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

[Sudeep KarkadaNagesha]

Hi Rob, Grant,

On 01/05/13 12:11, Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
> 
> These are couple of updates to existing 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.
> 
Is proposed option of phandle for OPP acceptable to avoid replication ?
Any suggestions to proceed on this ? This is needed to support CPU
hotplug on big LITTLE system where current methods like parsing all the
nodes or just CPU0 node will not work.

Regards,
Sudeep

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

[Grant Likely]

On Tue, 21 May 2013 11:00:35 +0100, Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha@arm.com> wrote:
> Hi Rob, Grant,
> 
> On 01/05/13 12:11, Sudeep KarkadaNagesha wrote:
> > From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> > 
> > These are couple of updates to existing 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.
> > 
> Is proposed option of phandle for OPP acceptable to avoid replication ?
> Any suggestions to proceed on this ? This is needed to support CPU
> hotplug on big LITTLE system where current methods like parsing all the
> nodes or just CPU0 node will not work.

Looks fine to me.

g.

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

[Sudeep KarkadaNagesha]

On 20/07/13 06:09, Grant Likely wrote:
> On Tue, 21 May 2013 11:00:35 +0100, Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha@arm.com> wrote:
>> Hi Rob, Grant,
>>
>> On 01/05/13 12:11, Sudeep KarkadaNagesha wrote:
>>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>>>
>>> These are couple of updates to existing 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.
>>>
>> Is proposed option of phandle for OPP acceptable to avoid replication ?
>> Any suggestions to proceed on this ? This is needed to support CPU
>> hotplug on big LITTLE system where current methods like parsing all the
>> nodes or just CPU0 node will not work.
> 
> Looks fine to me.
Hi Grant,

Thanks for the response.

However I had a thought after seeing recent patch series by Mike[1]
Since the OPPs are usually associated with clocks, and multiple devices
sharing clocks will point to same clock node in DT, clk node is more
logical place to specify the OPPs. IMO this will be good alignment for
the consolidation effort by Mike. One issue with this approach is
backward compatibility(using old DT)

Regards,
Sudeep

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/182231.html

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

[Sudeep KarkadaNagesha]

(sorry with new DT mailing list address this time)
On 20/07/13 06:09, Grant Likely wrote:
> On Tue, 21 May 2013 11:00:35 +0100, Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha@arm.com> wrote:
>> Hi Rob, Grant,
>>
>> On 01/05/13 12:11, Sudeep KarkadaNagesha wrote:
>>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>>>
>>> These are couple of updates to existing 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.
>>>
>> Is proposed option of phandle for OPP acceptable to avoid replication ?
>> Any suggestions to proceed on this ? This is needed to support CPU
>> hotplug on big LITTLE system where current methods like parsing all the
>> nodes or just CPU0 node will not work.
> 
> Looks fine to me.
> 
Hi Grant,

Thanks for the response.

However I had a thought after seeing recent patch series by Mike[1]
Since the OPPs are usually associated with clocks, and multiple devices
sharing clocks will point to same clock node in DT, clk node is more
logical place to specify the OPPs. IMO this will be good alignment for
the consolidation effort by Mike. One issue with this approach is
backward compatibility(using old DT)

Regards,
Sudeep

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/182231.html