[PATCH v3 0/3] Dynamic power model from device tree

[PATCH v3 0/3] Dynamic power model from device tree

[Punit Agrawal]

Hi,

This patchset adds support to build a single-coefficient dynamic power
model for a CPU. The model is used by the CPU cooling device to
provide an estimate of power consumption and also translate allocated
power to performance cap.

Patch 1 extends the CPU nodes binding to provide an optional dynamic
power coefficient which can be used to create a dynamic power model
for the CPUs. This model is used to constrain device power consumption
(using power_allocator governor) when the system is thermally
constrained.

Patches 2-3 extends the cpufreq-dt and arm_big_little driver to
register cpu cooling devices with the dynamic coefficient when
provided.

The patches were previously posted as part of [0][1]. Since the last
posting Mediatek platform 8173 is also building on these bindings to
build the power model.

Feedback on the bindings would be particularly appreciated.

Thanks,
Punit

[0] http://thread.gmane.org/gmane.linux.kernel/2002152
[1] http://thread.gmane.org/gmane.linux.kernel/2011466

Punit Agrawal (3):
  devicetree: bindings: Add optional dynamic-power-coefficient property
  cpufreq-dt: Supply power coefficient when registering cooling devices
  cpufreq: arm_big_little: Add support to register a cpufreq cooling
    device

 Documentation/devicetree/bindings/arm/cpus.txt | 17 +++++++++
 drivers/cpufreq/arm_big_little.c               | 52 +++++++++++++++++++++++++-
 drivers/cpufreq/cpufreq-dt.c                   |  9 ++++-
 3 files changed, 74 insertions(+), 4 deletions(-)

-- 
2.5.3

[PATCH v3 1/3] devicetree: bindings: Add optional dynamic-power-coefficient property

[Punit Agrawal]

The dynamic power consumption of a device is proportional to the
square of voltage (V) and the clock frequency (f). It can be expressed as

Pdyn = dynamic-power-coefficient * V^2 * f.

The coefficient represents the running time dynamic power consumption in
units of mw/MHz/uVolt^2 and can be used in the above formula to
calculate the dynamic power in mW.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index 91e6e5c..c33362d 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -240,6 +240,23 @@ nodes to be present and contain the properties described below.
 		Definition: Specifies the syscon node controlling the cpu core
 			    power domains.
 
+	- dynamic-power-coefficient
+		Usage: optional
+		Value type: <prop-encoded-array>
+		Definition: A u32 value that represents the running time dynamic
+			    power coefficient in units of mW/MHz/uVolt^2. The
+			    coefficient can either be calculated from power
+			    measurements or derived by analysis.
+
+			    The dynamic power consumption of the CPU  is
+			    proportional to the square of the Voltage (V) and
+			    the clock frequency (f). The coefficient is used to
+			    calculate the dynamic power as below -
+
+			    Pdyn = dynamic-power-coefficient * V^2 * f
+
+			    where voltage is in uV, frequency is in MHz.
+
 Example 1 (dual-cluster big.LITTLE system 32-bit):
 
 	cpus {
-- 
2.5.3

Re: [PATCH v3 1/3] devicetree: bindings: Add optional dynamic-power-coefficient property

[Rob Herring]

On Mon, Nov 09, 2015 at 05:29:21PM +0000, Punit Agrawal wrote:
> The dynamic power consumption of a device is proportional to the
> square of voltage (V) and the clock frequency (f). It can be expressed as
> 
> Pdyn = dynamic-power-coefficient * V^2 * f.
> 
> The coefficient represents the running time dynamic power consumption in
> units of mw/MHz/uVolt^2 and can be used in the above formula to
> calculate the dynamic power in mW.

I have no issue with the binding, but I wonder if a single value is 
really sufficient to model this. Don't you also need a static component? 

Rob

> 
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index 91e6e5c..c33362d 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -240,6 +240,23 @@ nodes to be present and contain the properties described below.
>  		Definition: Specifies the syscon node controlling the cpu core
>  			    power domains.
>  
> +	- dynamic-power-coefficient
> +		Usage: optional
> +		Value type: <prop-encoded-array>
> +		Definition: A u32 value that represents the running time dynamic
> +			    power coefficient in units of mW/MHz/uVolt^2. The
> +			    coefficient can either be calculated from power
> +			    measurements or derived by analysis.
> +
> +			    The dynamic power consumption of the CPU  is
> +			    proportional to the square of the Voltage (V) and
> +			    the clock frequency (f). The coefficient is used to
> +			    calculate the dynamic power as below -
> +
> +			    Pdyn = dynamic-power-coefficient * V^2 * f
> +
> +			    where voltage is in uV, frequency is in MHz.
> +
>  Example 1 (dual-cluster big.LITTLE system 32-bit):
>  
>  	cpus {
> -- 
> 2.5.3
> 

Re: [PATCH v3 1/3] devicetree: bindings: Add optional dynamic-power-coefficient property

[Punit Agrawal]

Rob Herring <robh@kernel.org> writes:

> On Mon, Nov 09, 2015 at 05:29:21PM +0000, Punit Agrawal wrote:
>> The dynamic power consumption of a device is proportional to the
>> square of voltage (V) and the clock frequency (f). It can be expressed as
>> 
>> Pdyn = dynamic-power-coefficient * V^2 * f.
>> 
>> The coefficient represents the running time dynamic power consumption in
>> units of mw/MHz/uVolt^2 and can be used in the above formula to
>> calculate the dynamic power in mW.
>
> I have no issue with the binding, but I wonder if a single value is 
> really sufficient to model this. Don't you also need a static
> component?

In general, power consumption does consist of the static and dynamic
power. Here we are only modelling the dynamic component.

For CPUs, we found that a single-coefficient model using frequency and
voltages gives a reasonable estimation of dynamic power consumption. For
thermal management, because of using closed loop control providing the
dynamic consumption is good enough as long as static power for your
platform is not significant.

On the other hand, there isn't a generally applicable model for static
power - it has complex relationships with temperature, voltages, etc
which make it hard to express as a simple formula. For platforms where
static power is significant, providing it via platform code is the only
option for now.

This patchset enables platforms where dynamic power works well to setup
their power model for thermal management via device tree.

>
> Rob
>
>> 
>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> ---
>>  Documentation/devicetree/bindings/arm/cpus.txt | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
>> index 91e6e5c..c33362d 100644
>> --- a/Documentation/devicetree/bindings/arm/cpus.txt
>> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
>> @@ -240,6 +240,23 @@ nodes to be present and contain the properties described below.
>>  		Definition: Specifies the syscon node controlling the cpu core
>>  			    power domains.
>>  
>> +	- dynamic-power-coefficient
>> +		Usage: optional
>> +		Value type: <prop-encoded-array>
>> +		Definition: A u32 value that represents the running time dynamic
>> +			    power coefficient in units of mW/MHz/uVolt^2. The
>> +			    coefficient can either be calculated from power
>> +			    measurements or derived by analysis.
>> +
>> +			    The dynamic power consumption of the CPU  is
>> +			    proportional to the square of the Voltage (V) and
>> +			    the clock frequency (f). The coefficient is used to
>> +			    calculate the dynamic power as below -
>> +
>> +			    Pdyn = dynamic-power-coefficient * V^2 * f
>> +
>> +			    where voltage is in uV, frequency is in MHz.
>> +
>>  Example 1 (dual-cluster big.LITTLE system 32-bit):
>>  
>>  	cpus {
>> -- 
>> 2.5.3
>> 
> --
> 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

[PATCH v3 2/3] cpufreq-dt: Supply power coefficient when registering cooling devices

[Punit Agrawal]

Support registering cooling devices with dynamic power coefficient
where provided by the device tree. This allows OF registered cooling
devices driver to be used with the power_allocator thermal governor.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/cpufreq/cpufreq-dt.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 7c0d70e..4434e45 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -407,8 +407,13 @@ static void cpufreq_ready(struct cpufreq_policy *policy)
 	 * thermal DT code takes care of matching them.
 	 */
 	if (of_find_property(np, "#cooling-cells", NULL)) {
-		priv->cdev = of_cpufreq_cooling_register(np,
-							 policy->related_cpus);
+		u32 power_coefficient = 0;
+
+		of_property_read_u32(np, "dynamic-power-coefficient",
+				     &power_coefficient);
+
+		priv->cdev = of_cpufreq_power_cooling_register(np,
+				policy->related_cpus, power_coefficient, NULL);
 		if (IS_ERR(priv->cdev)) {
 			dev_err(priv->cpu_dev,
 				"running cpufreq without cooling device: %ld\n",
-- 
2.5.3

Re: [PATCH v3 2/3] cpufreq-dt: Supply power coefficient when registering cooling devices

[Javi Merino]

On Mon, Nov 09, 2015 at 05:29:22PM +0000, Punit Agrawal wrote:
> Support registering cooling devices with dynamic power coefficient
> where provided by the device tree. This allows OF registered cooling
> devices driver to be used with the power_allocator thermal governor.
> 
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> ---
>  drivers/cpufreq/cpufreq-dt.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

FWIW,

Reviewed-by: Javi Merino <javi.merino@arm.com>

[PATCH v3 3/3] cpufreq: arm_big_little: Add support to register a cpufreq cooling device

[Punit Agrawal]

Register passive cooling devices when initialising cpufreq on
big.LITTLE systems. If the device tree provides a dynamic power
coefficient for the CPUs then the bound cooling device will support
the extensions that allow it to be used with all the existing thermal
governors including the power allocator governor.

A cooling device will be created per individual frequency domain and
can be bound to thermal zones via the thermal DT bindings.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/cpufreq/arm_big_little.c | 52 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
index f1e42f8..72a2777 100644
--- a/drivers/cpufreq/arm_big_little.c
+++ b/drivers/cpufreq/arm_big_little.c
@@ -23,6 +23,7 @@
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
 #include <linux/cpumask.h>
+#include <linux/cpu_cooling.h>
 #include <linux/export.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -55,6 +56,10 @@ static bool bL_switching_enabled;
 #define ACTUAL_FREQ(cluster, freq)  ((cluster == A7_CLUSTER) ? freq << 1 : freq)
 #define VIRT_FREQ(cluster, freq)    ((cluster == A7_CLUSTER) ? freq >> 1 : freq)
 
+struct private_data {
+	struct thermal_cooling_device *cdev;
+};
+
 static struct cpufreq_arm_bL_ops *arm_bL_ops;
 static struct clk *clk[MAX_CLUSTERS];
 static struct cpufreq_frequency_table *freq_table[MAX_CLUSTERS + 1];
@@ -440,6 +445,7 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy)
 {
 	u32 cur_cluster = cpu_to_cluster(policy->cpu);
 	struct device *cpu_dev;
+	struct private_data *priv;
 	int ret;
 
 	cpu_dev = get_cpu_device(policy->cpu);
@@ -457,9 +463,15 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy)
 	if (ret) {
 		dev_err(cpu_dev, "CPU %d, cluster: %d invalid freq table\n",
 				policy->cpu, cur_cluster);
-		put_cluster_clk_and_freq_table(cpu_dev);
-		return ret;
+		goto out_free_cpufreq_table;
+	}
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		ret = -ENOMEM;
+		goto out_free_cpufreq_table;
 	}
+	policy->driver_data = priv;
 
 	if (cur_cluster < MAX_CLUSTERS) {
 		int cpu;
@@ -484,12 +496,19 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy)
 
 	dev_info(cpu_dev, "%s: CPU %d initialized\n", __func__, policy->cpu);
 	return 0;
+
+out_free_cpufreq_table:
+	put_cluster_clk_and_freq_table(cpu_dev);
+
+	return ret;
 }
 
 static int bL_cpufreq_exit(struct cpufreq_policy *policy)
 {
 	struct device *cpu_dev;
+	struct private_data *priv = policy->driver_data;
 
+	cpufreq_cooling_unregister(priv->cdev);
 	cpu_dev = get_cpu_device(policy->cpu);
 	if (!cpu_dev) {
 		pr_err("%s: failed to get cpu%d device\n", __func__,
@@ -498,11 +517,39 @@ static int bL_cpufreq_exit(struct cpufreq_policy *policy)
 	}
 
 	put_cluster_clk_and_freq_table(cpu_dev);
+	kfree(priv);
 	dev_dbg(cpu_dev, "%s: Exited, cpu: %d\n", __func__, policy->cpu);
 
 	return 0;
 }
 
+static void bL_cpufreq_ready(struct cpufreq_policy *policy)
+{
+	struct device *cpu_dev = get_cpu_device(policy->cpu);
+	struct device_node *np = of_node_get(cpu_dev->of_node);
+	struct private_data *priv = policy->driver_data;
+
+	if (WARN_ON(!np))
+		return;
+
+	if (of_find_property(np, "#cooling-cells", NULL)) {
+		u32 power_coefficient = 0;
+
+		of_property_read_u32(np, "dynamic-power-coefficient",
+				     &power_coefficient);
+
+		priv->cdev = of_cpufreq_power_cooling_register(np,
+				policy->related_cpus, power_coefficient, NULL);
+		if (IS_ERR(priv->cdev)) {
+			dev_err(cpu_dev,
+				"running cpufreq without cooling device: %ld\n",
+				PTR_ERR(priv->cdev));
+			priv->cdev = NULL;
+		}
+	}
+	of_node_put(np);
+}
+
 static struct cpufreq_driver bL_cpufreq_driver = {
 	.name			= "arm-big-little",
 	.flags			= CPUFREQ_STICKY |
@@ -513,6 +560,7 @@ static struct cpufreq_driver bL_cpufreq_driver = {
 	.get			= bL_cpufreq_get_rate,
 	.init			= bL_cpufreq_init,
 	.exit			= bL_cpufreq_exit,
+	.ready			= bL_cpufreq_ready,
 	.attr			= cpufreq_generic_attr,
 };
 
-- 
2.5.3

Re: [PATCH v3 3/3] cpufreq: arm_big_little: Add support to register a cpufreq cooling device

[Javi Merino]

On Mon, Nov 09, 2015 at 05:29:23PM +0000, Punit Agrawal wrote:
> Register passive cooling devices when initialising cpufreq on
> big.LITTLE systems. If the device tree provides a dynamic power
> coefficient for the CPUs then the bound cooling device will support
> the extensions that allow it to be used with all the existing thermal
> governors including the power allocator governor.
> 
> A cooling device will be created per individual frequency domain and
> can be bound to thermal zones via the thermal DT bindings.
> 
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> ---
>  drivers/cpufreq/arm_big_little.c | 52 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
> index f1e42f8..72a2777 100644
> --- a/drivers/cpufreq/arm_big_little.c
> +++ b/drivers/cpufreq/arm_big_little.c
> @@ -23,6 +23,7 @@
>  #include <linux/cpu.h>
>  #include <linux/cpufreq.h>
>  #include <linux/cpumask.h>
> +#include <linux/cpu_cooling.h>
>  #include <linux/export.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -55,6 +56,10 @@ static bool bL_switching_enabled;
>  #define ACTUAL_FREQ(cluster, freq)  ((cluster == A7_CLUSTER) ? freq << 1 : freq)
>  #define VIRT_FREQ(cluster, freq)    ((cluster == A7_CLUSTER) ? freq >> 1 : freq)
>  
> +struct private_data {
> +	struct thermal_cooling_device *cdev;
> +};
> +

My first impression was that this is redundant and you could store the
pointer to cdev in driver_data itself.  But seeing that it's mirroring
the structure in cpufreq-dt and it's simpler to do it this way I guess
it's ok to create this struct with only one pointer.  You can add my

Reviewed-by: Javi Merino <javi.merino@arm.com>

>  static struct cpufreq_arm_bL_ops *arm_bL_ops;
>  static struct clk *clk[MAX_CLUSTERS];
>  static struct cpufreq_frequency_table *freq_table[MAX_CLUSTERS + 1];
> @@ -440,6 +445,7 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy)
>  {
>  	u32 cur_cluster = cpu_to_cluster(policy->cpu);
>  	struct device *cpu_dev;
> +	struct private_data *priv;
>  	int ret;
>  
>  	cpu_dev = get_cpu_device(policy->cpu);
> @@ -457,9 +463,15 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy)
>  	if (ret) {
>  		dev_err(cpu_dev, "CPU %d, cluster: %d invalid freq table\n",
>  				policy->cpu, cur_cluster);
> -		put_cluster_clk_and_freq_table(cpu_dev);
> -		return ret;
> +		goto out_free_cpufreq_table;
> +	}
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		ret = -ENOMEM;
> +		goto out_free_cpufreq_table;
>  	}
> +	policy->driver_data = priv;
>  
>  	if (cur_cluster < MAX_CLUSTERS) {
>  		int cpu;
> @@ -484,12 +496,19 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy)
>  
>  	dev_info(cpu_dev, "%s: CPU %d initialized\n", __func__, policy->cpu);
>  	return 0;
> +
> +out_free_cpufreq_table:
> +	put_cluster_clk_and_freq_table(cpu_dev);
> +
> +	return ret;
>  }
>  
>  static int bL_cpufreq_exit(struct cpufreq_policy *policy)
>  {
>  	struct device *cpu_dev;
> +	struct private_data *priv = policy->driver_data;
>  
> +	cpufreq_cooling_unregister(priv->cdev);
>  	cpu_dev = get_cpu_device(policy->cpu);
>  	if (!cpu_dev) {
>  		pr_err("%s: failed to get cpu%d device\n", __func__,
> @@ -498,11 +517,39 @@ static int bL_cpufreq_exit(struct cpufreq_policy *policy)
>  	}
>  
>  	put_cluster_clk_and_freq_table(cpu_dev);
> +	kfree(priv);
>  	dev_dbg(cpu_dev, "%s: Exited, cpu: %d\n", __func__, policy->cpu);
>  
>  	return 0;
>  }
>  
> +static void bL_cpufreq_ready(struct cpufreq_policy *policy)
> +{
> +	struct device *cpu_dev = get_cpu_device(policy->cpu);
> +	struct device_node *np = of_node_get(cpu_dev->of_node);
> +	struct private_data *priv = policy->driver_data;
> +
> +	if (WARN_ON(!np))
> +		return;
> +
> +	if (of_find_property(np, "#cooling-cells", NULL)) {
> +		u32 power_coefficient = 0;
> +
> +		of_property_read_u32(np, "dynamic-power-coefficient",
> +				     &power_coefficient);
> +
> +		priv->cdev = of_cpufreq_power_cooling_register(np,
> +				policy->related_cpus, power_coefficient, NULL);
> +		if (IS_ERR(priv->cdev)) {
> +			dev_err(cpu_dev,
> +				"running cpufreq without cooling device: %ld\n",
> +				PTR_ERR(priv->cdev));
> +			priv->cdev = NULL;
> +		}
> +	}
> +	of_node_put(np);
> +}
> +
>  static struct cpufreq_driver bL_cpufreq_driver = {
>  	.name			= "arm-big-little",
>  	.flags			= CPUFREQ_STICKY |
> @@ -513,6 +560,7 @@ static struct cpufreq_driver bL_cpufreq_driver = {
>  	.get			= bL_cpufreq_get_rate,
>  	.init			= bL_cpufreq_init,
>  	.exit			= bL_cpufreq_exit,
> +	.ready			= bL_cpufreq_ready,
>  	.attr			= cpufreq_generic_attr,
>  };
>  
> -- 
> 2.5.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH v3 3/3] cpufreq: arm_big_little: Add support to register a cpufreq cooling device

[Viresh Kumar]

On 09-11-15, 17:29, Punit Agrawal wrote:
> Register passive cooling devices when initialising cpufreq on
> big.LITTLE systems. If the device tree provides a dynamic power
> coefficient for the CPUs then the bound cooling device will support
> the extensions that allow it to be used with all the existing thermal
> governors including the power allocator governor.
> 
> A cooling device will be created per individual frequency domain and
> can be bound to thermal zones via the thermal DT bindings.
> 
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> ---
>  drivers/cpufreq/arm_big_little.c | 52 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
> index f1e42f8..72a2777 100644
> --- a/drivers/cpufreq/arm_big_little.c
> +++ b/drivers/cpufreq/arm_big_little.c
> @@ -23,6 +23,7 @@
>  #include <linux/cpu.h>
>  #include <linux/cpufreq.h>
>  #include <linux/cpumask.h>
> +#include <linux/cpu_cooling.h>
>  #include <linux/export.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -55,6 +56,10 @@ static bool bL_switching_enabled;
>  #define ACTUAL_FREQ(cluster, freq)  ((cluster == A7_CLUSTER) ? freq << 1 : freq)
>  #define VIRT_FREQ(cluster, freq)    ((cluster == A7_CLUSTER) ? freq >> 1 : freq)
>  
> +struct private_data {
> +	struct thermal_cooling_device *cdev;
> +};

I think we need to be consistent within the driver, and so this must
be stored in a similar way to what we do for other structures. We have
static arrays for them, please do it that way only OR first change all
of them to be part of a bigger private_data structure.

-- 
viresh

Re: [PATCH v3 3/3] cpufreq: arm_big_little: Add support to register a cpufreq cooling device

[Punit Agrawal]

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

> On 09-11-15, 17:29, Punit Agrawal wrote:
>> Register passive cooling devices when initialising cpufreq on
>> big.LITTLE systems. If the device tree provides a dynamic power
>> coefficient for the CPUs then the bound cooling device will support
>> the extensions that allow it to be used with all the existing thermal
>> governors including the power allocator governor.
>> 
>> A cooling device will be created per individual frequency domain and
>> can be bound to thermal zones via the thermal DT bindings.
>> 
>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>> Cc: Eduardo Valentin <edubezval@gmail.com>
>> ---
>>  drivers/cpufreq/arm_big_little.c | 52 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 50 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
>> index f1e42f8..72a2777 100644
>> --- a/drivers/cpufreq/arm_big_little.c
>> +++ b/drivers/cpufreq/arm_big_little.c
>> @@ -23,6 +23,7 @@
>>  #include <linux/cpu.h>
>>  #include <linux/cpufreq.h>
>>  #include <linux/cpumask.h>
>> +#include <linux/cpu_cooling.h>
>>  #include <linux/export.h>
>>  #include <linux/module.h>
>>  #include <linux/mutex.h>
>> @@ -55,6 +56,10 @@ static bool bL_switching_enabled;
>>  #define ACTUAL_FREQ(cluster, freq)  ((cluster == A7_CLUSTER) ? freq << 1 : freq)
>>  #define VIRT_FREQ(cluster, freq)    ((cluster == A7_CLUSTER) ? freq >> 1 : freq)
>>  
>> +struct private_data {
>> +	struct thermal_cooling_device *cdev;
>> +};
>
> I think we need to be consistent within the driver, and so this must
> be stored in a similar way to what we do for other structures. We have
> static arrays for them, please do it that way only OR first change all
> of them to be part of a bigger private_data structure.

Sure, I'll switch to using static arrays. There seem to be a lot of
assumptions that might be easily broken in moving all the data into a
single structure. I'll leave that as a later change.