[PATCH v3 0/2] thermal, OPP: move the CPU power estimation to the OPP library

[PATCH v3 0/2] thermal, OPP: move the CPU power estimation to the OPP library

[Quentin Perret]

Hi,

Currently, IPA estimates the power dissipated by a CPU at each available OPP
using its capacitance (the dynamic-power-coefficient DT binding). This series
relocates this per-OPP power model in the OPP core as a preparation for
future changes. More specifically:

   1. The current DT-based approach for power estimation will need deep
      changes to support SCMI-provided power values. While the thermal
      subsystem is not necessarily the best place to hide multiple power
      estimation methods, the OPP library appears to be a good candidate to
      implement the required platform abstraction and respect the existing
      design (the thermal subsystem already relies on the OPP library to
      provide voltages no matter where they come from -- DT or SCPI).

   2. The energy models of CPUs will be needed by other clients in the future
      (such as the task scheduler or CPUFreq governors for example) in order
      to make energy-aware decisions. The relocation to the OPP library will
      enable code re-use and all clients will benefit form the platform
      abstraction mentioned previously.

As mentionned during the review of V1, this series will have a chance to get
merged only when the clients (the scheduler, or the SCMI driver) depending
on it are ready to be merged as well. This V3 is here for reference and
addresses the main changes requested on the previous versions.

Changes in v2:
 - Changed the OPP power estimation to be done upon OPP creation (V. Kumar)
 - Updated the cover letter to motivate the choice of PM OPP (E. Valentin)

Changes in v3
 - Removed the has_power logic to simplify the thermal code (V. Kumar)
 - Moved _opp_estimate_power() call to _opp_add() to avoid duplication (V. Kumar)

Quentin Perret (2):
  PM / OPP: introduce an OPP power estimation helper
  thermal: cpu_cooling: use power models from the OPP library

 drivers/opp/core.c            | 32 +++++++++++++++++++++++++
 drivers/opp/of.c              | 11 +++++----
 drivers/opp/opp.h             |  7 ++++++
 drivers/thermal/cpu_cooling.c | 56 ++++++++++++++++---------------------------
 include/linux/pm_opp.h        |  7 ++++++
 5 files changed, 74 insertions(+), 39 deletions(-)

-- 
2.15.1

[PATCH v3 1/2] PM / OPP: introduce an OPP power estimation helper

[Quentin Perret]

The power dissipated by a CPU at a specific OPP is currently estimated by
the thermal subsystem as P = C * V^2 * f, with P the power, C the CPU's
capacitance, V the OPP's voltage and f the OPP's frequency. As this
feature can be useful for other clients requiring energy models of CPUs,
this commit introduces an equivalent power estimator directly into the OPP
library, hence enabling code re-use.

Signed-off-by: Quentin Perret <quentin.perret@arm.com>
---
 drivers/opp/core.c     | 32 ++++++++++++++++++++++++++++++++
 drivers/opp/of.c       | 11 +++++++----
 drivers/opp/opp.h      |  7 +++++++
 include/linux/pm_opp.h |  7 +++++++
 4 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 92fa94a6dcc1..81bf18554b34 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -127,6 +127,24 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
 
+/**
+ * dev_pm_opp_get_power() - Gets the estimated power corresponding to an opp
+ * @opp:	opp for which power has to be returned for
+ *
+ * Return: estimated power in micro-watts corresponding to the opp, else
+ * return 0
+ */
+unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp)
+{
+	if (IS_ERR_OR_NULL(opp)) {
+		pr_err("%s: Invalid parameters\n", __func__);
+		return 0;
+	}
+
+	return opp->power_estimate_uw;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_power);
+
 /**
  * dev_pm_opp_is_turbo() - Returns if opp is turbo OPP or not
  * @opp: opp for which turbo mode is being verified
@@ -985,6 +1003,18 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 	return true;
 }
 
+/**
+ * Estimate the power of an OPP as P = C * V^2 * f, with C the device's
+ * capacitance, V the OPP's voltage and f the OPP's frequency.
+ */
+static void _opp_estimate_power(struct dev_pm_opp *opp, unsigned long cap)
+{
+	unsigned long mV = opp->supplies[0].u_volt / 1000;
+	unsigned long KHz = opp->rate / 1000;
+
+	opp->power_estimate_uw = cap * mV * mV * KHz / 1000000000;
+}
+
 /*
  * Returns:
  * 0: On success. And appropriate error message for duplicate OPPs.
@@ -1039,6 +1069,8 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 	if (opp_table->get_pstate)
 		new_opp->pstate = opp_table->get_pstate(dev, new_opp->rate);
 
+	_opp_estimate_power(new_opp, opp_table->capacitance);
+
 	list_add(&new_opp->node, head);
 	mutex_unlock(&opp_table->lock);
 
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index cb716aa2f44b..d28d9dbb5272 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -55,14 +55,17 @@ void _of_init_opp_table(struct opp_table *opp_table, struct device *dev)
 {
 	struct device_node *np;
 
-	/*
-	 * Only required for backward compatibility with v1 bindings, but isn't
-	 * harmful for other cases. And so we do it unconditionally.
-	 */
 	np = of_node_get(dev->of_node);
 	if (np) {
 		u32 val;
 
+		of_property_read_u32(np, "dynamic-power-coefficient",
+				     &opp_table->capacitance);
+
+		/*
+		 * Required for backward compatibility with v1 bindings, but
+		 * isn't harmful for other cases so we do it unconditionally.
+		 */
 		if (!of_property_read_u32(np, "clock-latency", &val))
 			opp_table->clock_latency_ns_max = val;
 		of_property_read_u32(np, "voltage-tolerance",
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 4d00061648a3..8815b51dd117 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -63,6 +63,8 @@ extern struct list_head opp_tables;
  * @supplies:	Power supplies voltage/current values
  * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
  *		frequency from any other OPP's frequency.
+ * @power_estimate_uw: Estimation of the power (in micro-watts) dissipated by
+ *		the device at this OPP.
  * @opp_table:	points back to the opp_table struct this opp belongs to
  * @np:		OPP's device node.
  * @dentry:	debugfs dentry pointer (per opp)
@@ -84,6 +86,8 @@ struct dev_pm_opp {
 
 	unsigned long clock_latency_ns;
 
+	unsigned long power_estimate_uw;
+
 	struct opp_table *opp_table;
 
 	struct device_node *np;
@@ -129,6 +133,7 @@ enum opp_table_access {
  * @lock:	mutex protecting the opp_list.
  * @np:		struct device_node pointer for opp's DT node.
  * @clock_latency_ns_max: Max clock latency in nanoseconds.
+ * @capacitance: Device's capacitance, used to estimate its power.
  * @shared_opp: OPP is shared between multiple devices.
  * @suspend_opp: Pointer to OPP to be used during device suspend.
  * @supported_hw: Array of version number to support.
@@ -165,6 +170,8 @@ struct opp_table {
 	/* For backward compatibility with v1 bindings */
 	unsigned int voltage_tolerance_v1;
 
+	unsigned int capacitance;
+
 	enum opp_table_access shared_opp;
 	struct dev_pm_opp *suspend_opp;
 
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 6c2d2e88f066..8c73d8b61726 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -85,6 +85,8 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
 
 unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp);
 
+unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp);
+
 bool dev_pm_opp_is_turbo(struct dev_pm_opp *opp);
 
 int dev_pm_opp_get_opp_count(struct device *dev);
@@ -150,6 +152,11 @@ static inline unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
 	return 0;
 }
 
+static inline unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp)
+{
+	return 0;
+}
+
 static inline bool dev_pm_opp_is_turbo(struct dev_pm_opp *opp)
 {
 	return false;
-- 
2.15.1

Re: [PATCH v3 1/2] PM / OPP: introduce an OPP power estimation helper

[Joel Fernandes]

On Fri, Jan 19, 2018 at 1:45 AM, Quentin Perret <quentin.perret@arm.com> wrote:
> The power dissipated by a CPU at a specific OPP is currently estimated by
> the thermal subsystem as P = C * V^2 * f, with P the power, C the CPU's
> capacitance, V the OPP's voltage and f the OPP's frequency. As this
> feature can be useful for other clients requiring energy models of CPUs,
> this commit introduces an equivalent power estimator directly into the OPP
> library, hence enabling code re-use.
>
> Signed-off-by: Quentin Perret <quentin.perret@arm.com>
> ---
>  drivers/opp/core.c     | 32 ++++++++++++++++++++++++++++++++
>  drivers/opp/of.c       | 11 +++++++----
>  drivers/opp/opp.h      |  7 +++++++
>  include/linux/pm_opp.h |  7 +++++++
>  4 files changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 92fa94a6dcc1..81bf18554b34 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -127,6 +127,24 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
>
> +/**
> + * dev_pm_opp_get_power() - Gets the estimated power corresponding to an opp
> + * @opp:       opp for which power has to be returned for
> + *
> + * Return: estimated power in micro-watts corresponding to the opp, else
> + * return 0
> + */
> +unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp)
> +{
> +       if (IS_ERR_OR_NULL(opp)) {
> +               pr_err("%s: Invalid parameters\n", __func__);
> +               return 0;
> +       }
> +
> +       return opp->power_estimate_uw;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_get_power);
> +
>  /**
>   * dev_pm_opp_is_turbo() - Returns if opp is turbo OPP or not
>   * @opp: opp for which turbo mode is being verified
> @@ -985,6 +1003,18 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
>         return true;
>  }
>
> +/**
> + * Estimate the power of an OPP as P = C * V^2 * f, with C the device's
> + * capacitance, V the OPP's voltage and f the OPP's frequency.
> + */
> +static void _opp_estimate_power(struct dev_pm_opp *opp, unsigned long cap)
> +{
> +       unsigned long mV = opp->supplies[0].u_volt / 1000;
> +       unsigned long KHz = opp->rate / 1000;
> +
> +       opp->power_estimate_uw = cap * mV * mV * KHz / 1000000000;
> +}
> +
>  /*
>   * Returns:
>   * 0: On success. And appropriate error message for duplicate OPPs.
> @@ -1039,6 +1069,8 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
>         if (opp_table->get_pstate)
>                 new_opp->pstate = opp_table->get_pstate(dev, new_opp->rate);
>
> +       _opp_estimate_power(new_opp, opp_table->capacitance);
> +

Does this need to be a separate static function? There's only one user
of the function in the C file and it adds more LOC.

Otherwise, feel free to add FWIW:
Reviewed-by: Joel Fernandes <joelaf@google.com>

thanks,

- Joel

Re: [PATCH v3 1/2] PM / OPP: introduce an OPP power estimation helper

[Viresh Kumar]

On 19-01-18, 15:36, Joel Fernandes wrote:
> Does this need to be a separate static function? There's only one user
> of the function in the C file and it adds more LOC.

I would prefer it that way too, i.e. a separate routine for this stuff.

-- 
viresh

Re: [PATCH v3 1/2] PM / OPP: introduce an OPP power estimation helper

[Quentin Perret]

On Friday 19 Jan 2018 at 15:36:42 (-0800), Joel Fernandes wrote:
> > +/**
> > + * Estimate the power of an OPP as P = C * V^2 * f, with C the device's
> > + * capacitance, V the OPP's voltage and f the OPP's frequency.
> > + */
> > +static void _opp_estimate_power(struct dev_pm_opp *opp, unsigned long cap)
> > +{
> > +       unsigned long mV = opp->supplies[0].u_volt / 1000;
> > +       unsigned long KHz = opp->rate / 1000;
> > +
> > +       opp->power_estimate_uw = cap * mV * mV * KHz / 1000000000;
> > +}
> > +
> >  /*
> >   * Returns:
> >   * 0: On success. And appropriate error message for duplicate OPPs.
> > @@ -1039,6 +1069,8 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> >         if (opp_table->get_pstate)
> >                 new_opp->pstate = opp_table->get_pstate(dev, new_opp->rate);
> >
> > +       _opp_estimate_power(new_opp, opp_table->capacitance);
> > +
> 
> Does this need to be a separate static function? There's only one user
> of the function in the C file and it adds more LOC.

True, but I guess that makes sense conceptually to factor out this
feature. Maybe it's just my personal taste :)

> 
> Otherwise, feel free to add FWIW:
> Reviewed-by: Joel Fernandes <joelaf@google.com>

Cool, thank you very much !

Quentin

Re: [PATCH v3 1/2] PM / OPP: introduce an OPP power estimation helper

[Joel Fernandes]

On Fri, Jan 19, 2018 at 1:45 AM, Quentin Perret <quentin.perret@arm.com> wrote:
> The power dissipated by a CPU at a specific OPP is currently estimated by
> the thermal subsystem as P = C * V^2 * f, with P the power, C the CPU's
> capacitance, V the OPP's voltage and f the OPP's frequency. As this
> feature can be useful for other clients requiring energy models of CPUs,
> this commit introduces an equivalent power estimator directly into the OPP
> library, hence enabling code re-use.
>
> Signed-off-by: Quentin Perret <quentin.perret@arm.com>
> ---
>  drivers/opp/core.c     | 32 ++++++++++++++++++++++++++++++++
>  drivers/opp/of.c       | 11 +++++++----
>  drivers/opp/opp.h      |  7 +++++++
>  include/linux/pm_opp.h |  7 +++++++
>  4 files changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 92fa94a6dcc1..81bf18554b34 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -127,6 +127,24 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
>
> +/**
> + * dev_pm_opp_get_power() - Gets the estimated power corresponding to an opp
> + * @opp:       opp for which power has to be returned for
> + *
> + * Return: estimated power in micro-watts corresponding to the opp, else
> + * return 0
> + */
> +unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp)
> +{
> +       if (IS_ERR_OR_NULL(opp)) {
> +               pr_err("%s: Invalid parameters\n", __func__);
> +               return 0;
> +       }
> +
> +       return opp->power_estimate_uw;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_get_power);
> +
>  /**
>   * dev_pm_opp_is_turbo() - Returns if opp is turbo OPP or not
>   * @opp: opp for which turbo mode is being verified
> @@ -985,6 +1003,18 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
>         return true;
>  }
>
> +/**
> + * Estimate the power of an OPP as P = C * V^2 * f, with C the device's
> + * capacitance, V the OPP's voltage and f the OPP's frequency.
> + */
> +static void _opp_estimate_power(struct dev_pm_opp *opp, unsigned long cap)
> +{
> +       unsigned long mV = opp->supplies[0].u_volt / 1000;
> +       unsigned long KHz = opp->rate / 1000;
> +
> +       opp->power_estimate_uw = cap * mV * mV * KHz / 1000000000;

Also, here you can do the multiples first and then the divides to get
better accuracy with lesser rounding.

- Joel

Re: [PATCH v3 1/2] PM / OPP: introduce an OPP power estimation helper

[Quentin Perret]

Hi Joel,

Thanks for the feedback !

On Friday 19 Jan 2018 at 20:10:46 (-0800), Joel Fernandes wrote:
> > +/**
> > + * Estimate the power of an OPP as P = C * V^2 * f, with C the device's
> > + * capacitance, V the OPP's voltage and f the OPP's frequency.
> > + */
> > +static void _opp_estimate_power(struct dev_pm_opp *opp, unsigned long cap)
> > +{
> > +       unsigned long mV = opp->supplies[0].u_volt / 1000;
> > +       unsigned long KHz = opp->rate / 1000;
> > +
> > +       opp->power_estimate_uw = cap * mV * mV * KHz / 1000000000;
> 
> Also, here you can do the multiples first and then the divides to get
> better accuracy with lesser rounding.

Yeah that's what I wanted to do in the first place but there is a risk
that an intermediate result won't fit in 64 bits ... If you take the
example of the highest OPP on the big cluster of the Hikey 960, the
parameters are the following:
 - uV = 1500000
 - Hz = 2362000000
 - C =  550

In this case, P = C * uV^2 * Hz ~ 2.9230e+24 which doesn't fit in 64
bits ...

Re: [PATCH v3 1/2] PM / OPP: introduce an OPP power estimation helper

[Joel Fernandes]

On Mon, Jan 22, 2018 at 1:56 AM, Quentin Perret <quentin.perret@arm.com> wrote:
> Hi Joel,
>
> Thanks for the feedback !
>
> On Friday 19 Jan 2018 at 20:10:46 (-0800), Joel Fernandes wrote:
>> > +/**
>> > + * Estimate the power of an OPP as P = C * V^2 * f, with C the device's
>> > + * capacitance, V the OPP's voltage and f the OPP's frequency.
>> > + */
>> > +static void _opp_estimate_power(struct dev_pm_opp *opp, unsigned long cap)
>> > +{
>> > +       unsigned long mV = opp->supplies[0].u_volt / 1000;
>> > +       unsigned long KHz = opp->rate / 1000;
>> > +
>> > +       opp->power_estimate_uw = cap * mV * mV * KHz / 1000000000;
>>
>> Also, here you can do the multiples first and then the divides to get
>> better accuracy with lesser rounding.
>
> Yeah that's what I wanted to do in the first place but there is a risk
> that an intermediate result won't fit in 64 bits ... If you take the
> example of the highest OPP on the big cluster of the Hikey 960, the
> parameters are the following:
>  - uV = 1500000
>  - Hz = 2362000000
>  - C =  550
>
> In this case, P = C * uV^2 * Hz ~ 2.9230e+24 which doesn't fit in 64
> bits ...

True, if these numbers are typical it looks like there isn't any loss
of accuracy so it looks like it should be fine anyway. Thanks for
indulging.

- Joel

[PATCH v3 2/2] thermal: cpu_cooling: use power models from the OPP library

[Quentin Perret]

Now that the OPP library features a power estimator, the existing code
in IPA can be modified to rely only on dev_pm_opp_get_power() without
having to care about the dynamic-power-coefficient DT binding.

Signed-off-by: Quentin Perret <quentin.perret@arm.com>
---
 drivers/thermal/cpu_cooling.c | 56 ++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 35 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index dfd23245f778..8eda2de2a20d 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -187,7 +187,8 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
 /**
  * update_freq_table() - Update the freq table with power numbers
  * @cpufreq_cdev:	the cpufreq cooling device in which to update the table
- * @capacitance: dynamic power coefficient for these cpus
+ * @found_power:	boolean indicating if a power model was available for
+ *			the cooling device
  *
  * Update the freq table with power numbers.  This table will be used in
  * cpu_power_to_freq() and cpu_freq_to_power() to convert between power and
@@ -198,7 +199,7 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
  * or -ENOMEM if we run out of memory.
  */
 static int update_freq_table(struct cpufreq_cooling_device *cpufreq_cdev,
-			     u32 capacitance)
+			     bool *found_power)
 {
 	struct freq_table *freq_table = cpufreq_cdev->freq_table;
 	struct dev_pm_opp *opp;
@@ -225,11 +226,10 @@ static int update_freq_table(struct cpufreq_cooling_device *cpufreq_cdev,
 		return -EINVAL;
 	}
 
+	*found_power = true;
+
 	for (i = 0; i <= cpufreq_cdev->max_level; i++) {
-		unsigned long freq = freq_table[i].frequency * 1000;
-		u32 freq_mhz = freq_table[i].frequency / 1000;
-		u64 power;
-		u32 voltage_mv;
+		unsigned long power, freq = freq_table[i].frequency * 1000;
 
 		/*
 		 * Find ceil frequency as 'freq' may be slightly lower than OPP
@@ -242,18 +242,12 @@ static int update_freq_table(struct cpufreq_cooling_device *cpufreq_cdev,
 			return -EINVAL;
 		}
 
-		voltage_mv = dev_pm_opp_get_voltage(opp) / 1000;
-		dev_pm_opp_put(opp);
-
-		/*
-		 * Do the multiplication with MHz and millivolt so as
-		 * to not overflow.
-		 */
-		power = (u64)capacitance * freq_mhz * voltage_mv * voltage_mv;
-		do_div(power, 1000000000);
-
 		/* power is stored in mW */
+		power = dev_pm_opp_get_power(opp) / 1000;
+		dev_pm_opp_put(opp);
 		freq_table[i].power = power;
+		if (!power)
+			*found_power = false;
 	}
 
 	return 0;
@@ -594,7 +588,6 @@ static unsigned int find_next_max(struct cpufreq_frequency_table *table,
  * @np: a valid struct device_node to the cooling device device tree node
  * @policy: cpufreq policy
  * Normally this should be same as cpufreq policy->related_cpus.
- * @capacitance: dynamic power coefficient for these cpus
  *
  * This interface function registers the cpufreq cooling device with the name
  * "thermal-cpufreq-%x". This api can support multiple instances of cpufreq
@@ -606,7 +599,7 @@ static unsigned int find_next_max(struct cpufreq_frequency_table *table,
  */
 static struct thermal_cooling_device *
 __cpufreq_cooling_register(struct device_node *np,
-			struct cpufreq_policy *policy, u32 capacitance)
+			struct cpufreq_policy *policy)
 {
 	struct thermal_cooling_device *cdev;
 	struct cpufreq_cooling_device *cpufreq_cdev;
@@ -614,7 +607,7 @@ __cpufreq_cooling_register(struct device_node *np,
 	unsigned int freq, i, num_cpus;
 	int ret;
 	struct thermal_cooling_device_ops *cooling_ops;
-	bool first;
+	bool first, found_power;
 
 	if (IS_ERR_OR_NULL(policy)) {
 		pr_err("%s: cpufreq policy isn't valid: %p\n", __func__, policy);
@@ -675,18 +668,15 @@ __cpufreq_cooling_register(struct device_node *np,
 			pr_debug("%s: freq:%u KHz\n", __func__, freq);
 	}
 
-	if (capacitance) {
-		ret = update_freq_table(cpufreq_cdev, capacitance);
-		if (ret) {
-			cdev = ERR_PTR(ret);
-			goto remove_ida;
-		}
-
-		cooling_ops = &cpufreq_power_cooling_ops;
-	} else {
-		cooling_ops = &cpufreq_cooling_ops;
+	ret = update_freq_table(cpufreq_cdev, &found_power);
+	if (ret) {
+		cdev = ERR_PTR(ret);
+		goto remove_ida;
 	}
 
+	cooling_ops = found_power ? &cpufreq_power_cooling_ops :
+				    &cpufreq_cooling_ops;
+
 	cdev = thermal_of_cooling_device_register(np, dev_name, cpufreq_cdev,
 						  cooling_ops);
 	if (IS_ERR(cdev))
@@ -732,7 +722,7 @@ __cpufreq_cooling_register(struct device_node *np,
 struct thermal_cooling_device *
 cpufreq_cooling_register(struct cpufreq_policy *policy)
 {
-	return __cpufreq_cooling_register(NULL, policy, 0);
+	return __cpufreq_cooling_register(NULL, policy);
 }
 EXPORT_SYMBOL_GPL(cpufreq_cooling_register);
 
@@ -760,7 +750,6 @@ of_cpufreq_cooling_register(struct cpufreq_policy *policy)
 {
 	struct device_node *np = of_get_cpu_node(policy->cpu, NULL);
 	struct thermal_cooling_device *cdev = NULL;
-	u32 capacitance = 0;
 
 	if (!np) {
 		pr_err("cpu_cooling: OF node not available for cpu%d\n",
@@ -769,10 +758,7 @@ of_cpufreq_cooling_register(struct cpufreq_policy *policy)
 	}
 
 	if (of_find_property(np, "#cooling-cells", NULL)) {
-		of_property_read_u32(np, "dynamic-power-coefficient",
-				     &capacitance);
-
-		cdev = __cpufreq_cooling_register(np, policy, capacitance);
+		cdev = __cpufreq_cooling_register(np, policy);
 		if (IS_ERR(cdev)) {
 			pr_err("cpu_cooling: cpu%d is not running as cooling device: %ld\n",
 			       policy->cpu, PTR_ERR(cdev));
-- 
2.15.1

Re: [PATCH v3 2/2] thermal: cpu_cooling: use power models from the OPP library

[Joel Fernandes]

Hi,

On Fri, Jan 19, 2018 at 1:45 AM, Quentin Perret <quentin.perret@arm.com> wrote:
> Now that the OPP library features a power estimator, the existing code
> in IPA can be modified to rely only on dev_pm_opp_get_power() without
> having to care about the dynamic-power-coefficient DT binding.
>
> Signed-off-by: Quentin Perret <quentin.perret@arm.com>
> ---
[..]
>   */
>  static int update_freq_table(struct cpufreq_cooling_device *cpufreq_cdev,
> -                            u32 capacitance)
> +                            bool *found_power)
>  {
>         struct freq_table *freq_table = cpufreq_cdev->freq_table;
>         struct dev_pm_opp *opp;
> @@ -225,11 +226,10 @@ static int update_freq_table(struct cpufreq_cooling_device *cpufreq_cdev,
>                 return -EINVAL;
>         }
>
> +       *found_power = true;
> +
>         for (i = 0; i <= cpufreq_cdev->max_level; i++) {
> -               unsigned long freq = freq_table[i].frequency * 1000;
> -               u32 freq_mhz = freq_table[i].frequency / 1000;
> -               u64 power;
> -               u32 voltage_mv;
> +               unsigned long power, freq = freq_table[i].frequency * 1000;
>
>                 /*
>                  * Find ceil frequency as 'freq' may be slightly lower than OPP
> @@ -242,18 +242,12 @@ static int update_freq_table(struct cpufreq_cooling_device *cpufreq_cdev,
>                         return -EINVAL;
>                 }
>
> -               voltage_mv = dev_pm_opp_get_voltage(opp) / 1000;
> -               dev_pm_opp_put(opp);
> -
> -               /*
> -                * Do the multiplication with MHz and millivolt so as
> -                * to not overflow.
> -                */
> -               power = (u64)capacitance * freq_mhz * voltage_mv * voltage_mv;
> -               do_div(power, 1000000000);
> -
>                 /* power is stored in mW */
> +               power = dev_pm_opp_get_power(opp) / 1000;
> +               dev_pm_opp_put(opp);
>                 freq_table[i].power = power;
> +               if (!power)
> +                       *found_power = false;
>         }
>
>         return 0;
> @@ -594,7 +588,6 @@ static unsigned int find_next_max(struct cpufreq_frequency_table *table,
>   * @np: a valid struct device_node to the cooling device device tree node
>   * @policy: cpufreq policy
>   * Normally this should be same as cpufreq policy->related_cpus.
> - * @capacitance: dynamic power coefficient for these cpus
>   *
>   * This interface function registers the cpufreq cooling device with the name
>   * "thermal-cpufreq-%x". This api can support multiple instances of cpufreq
> @@ -606,7 +599,7 @@ static unsigned int find_next_max(struct cpufreq_frequency_table *table,
>   */
>  static struct thermal_cooling_device *
>  __cpufreq_cooling_register(struct device_node *np,
> -                       struct cpufreq_policy *policy, u32 capacitance)
> +                       struct cpufreq_policy *policy)
>  {
>         struct thermal_cooling_device *cdev;
>         struct cpufreq_cooling_device *cpufreq_cdev;
> @@ -614,7 +607,7 @@ __cpufreq_cooling_register(struct device_node *np,
>         unsigned int freq, i, num_cpus;
>         int ret;
>         struct thermal_cooling_device_ops *cooling_ops;
> -       bool first;
> +       bool first, found_power;
>
>         if (IS_ERR_OR_NULL(policy)) {
>                 pr_err("%s: cpufreq policy isn't valid: %p\n", __func__, policy);
> @@ -675,18 +668,15 @@ __cpufreq_cooling_register(struct device_node *np,
>                         pr_debug("%s: freq:%u KHz\n", __func__, freq);
>         }
>
> -       if (capacitance) {
> -               ret = update_freq_table(cpufreq_cdev, capacitance);
> -               if (ret) {
> -                       cdev = ERR_PTR(ret);
> -                       goto remove_ida;
> -               }
> -
> -               cooling_ops = &cpufreq_power_cooling_ops;
> -       } else {
> -               cooling_ops = &cpufreq_cooling_ops;

Instead of this, is it better to add an API to OPP library to query it
if it has power data for all OPPs? Then you don't need to have a
'found_power' variable and also don't unnecessarily call
dev_pm_opp_find_freq_ceil in update_freq_table.

> +       ret = update_freq_table(cpufreq_cdev, &found_power);
> +       if (ret) {
> +               cdev = ERR_PTR(ret);
> +               goto remove_ida;
>         }
[..]

Thanks,

- Joel

Re: [PATCH v3 2/2] thermal: cpu_cooling: use power models from the OPP library

[Viresh Kumar]

On 19-01-18, 15:15, Joel Fernandes wrote:
> > -       if (capacitance) {
> > -               ret = update_freq_table(cpufreq_cdev, capacitance);
> > -               if (ret) {
> > -                       cdev = ERR_PTR(ret);
> > -                       goto remove_ida;
> > -               }
> > -
> > -               cooling_ops = &cpufreq_power_cooling_ops;
> > -       } else {
> > -               cooling_ops = &cpufreq_cooling_ops;

> also don't unnecessarily call
> dev_pm_opp_find_freq_ceil in update_freq_table.

He would need that anyway as he needs to get the real power numbers.

-- 
viresh

Re: [PATCH v3 2/2] thermal: cpu_cooling: use power models from the OPP library

[Joel Fernandes]

On Sun, Jan 21, 2018 at 9:11 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 19-01-18, 15:15, Joel Fernandes wrote:
>> > -       if (capacitance) {
>> > -               ret = update_freq_table(cpufreq_cdev, capacitance);
>> > -               if (ret) {
>> > -                       cdev = ERR_PTR(ret);
>> > -                       goto remove_ida;
>> > -               }
>> > -
>> > -               cooling_ops = &cpufreq_power_cooling_ops;
>> > -       } else {
>> > -               cooling_ops = &cpufreq_cooling_ops;
>
>> also don't unnecessarily call
>> dev_pm_opp_find_freq_ceil in update_freq_table.
>
> He would need that anyway as he needs to get the real power numbers.
>

I think I wasn't articulating well enough what I meant but sorry if I
missed something...

In the current code, if capacitance is not available, then
update_freq_table -> dev_pm_opp_find_freq_ceil isn't called. However
in the new code, with this patch, you're calling update_freq_table
anyway and later discovering through the new 'found_power' variable
that capacitance isn't possibly available. It seems the new code will
be slower than the older one.

Instead of doing that, if you queried that capacitance isn't available
in advance, then you could avoid finding that out later. Admittedly, I
am new to these code paths so forgive me if I'm noisy, just trying to
help. And thanks for explaining anything I missed.

Thanks,

- Joel

Re: [PATCH v3 2/2] thermal: cpu_cooling: use power models from the OPP library

[Viresh Kumar]

On 22-01-18, 13:30, Joel Fernandes wrote:
> I think I wasn't articulating well enough what I meant but sorry if I
> missed something...
> 
> In the current code, if capacitance is not available, then
> update_freq_table -> dev_pm_opp_find_freq_ceil isn't called. However
> in the new code, with this patch, you're calling update_freq_table
> anyway and later discovering through the new 'found_power' variable
> that capacitance isn't possibly available. It seems the new code will
> be slower than the older one.
> 
> Instead of doing that, if you queried that capacitance isn't available
> in advance, then you could avoid finding that out later. Admittedly, I
> am new to these code paths so forgive me if I'm noisy, just trying to
> help. And thanks for explaining anything I missed.

No, you are correct. But this is init code and executes only once per cooling
device and so speed isn't a big issue for us right now. We may eventually have a
helper like that, just that we wanted to add it only after more than one user
needs it.

-- 
viresh

Re: [PATCH v3 2/2] thermal: cpu_cooling: use power models from the OPP library

[Quentin Perret]

On Friday 19 Jan 2018 at 15:15:41 (-0800), Joel Fernandes wrote:
> > @@ -675,18 +668,15 @@ __cpufreq_cooling_register(struct device_node *np,
> >                         pr_debug("%s: freq:%u KHz\n", __func__, freq);
> >         }
> >
> > -       if (capacitance) {
> > -               ret = update_freq_table(cpufreq_cdev, capacitance);
> > -               if (ret) {
> > -                       cdev = ERR_PTR(ret);
> > -                       goto remove_ida;
> > -               }
> > -
> > -               cooling_ops = &cpufreq_power_cooling_ops;
> > -       } else {
> > -               cooling_ops = &cpufreq_cooling_ops;
> 
> Instead of this, is it better to add an API to OPP library to query it
> if it has power data for all OPPs? Then you don't need to have a
> 'found_power' variable and also don't unnecessarily call
> dev_pm_opp_find_freq_ceil in update_freq_table.

I had an API that looked like that in v1 and v2 (not used here though)
but we agreed with Viresh that it wasn't absolutely necessary so I got
rid of it. I'm not sure how I can avoid to call dev_pm_opp_find_freq_ceil
though. What did you mean by that ?

Thanks,
Quentin

Re: [PATCH v3 0/2] thermal, OPP: move the CPU power estimation to the OPP library

[Viresh Kumar]

On 19-01-18, 09:45, Quentin Perret wrote:
> Hi,
> 
> Currently, IPA estimates the power dissipated by a CPU at each available OPP
> using its capacitance (the dynamic-power-coefficient DT binding). This series
> relocates this per-OPP power model in the OPP core as a preparation for
> future changes. More specifically:
> 
>    1. The current DT-based approach for power estimation will need deep
>       changes to support SCMI-provided power values. While the thermal
>       subsystem is not necessarily the best place to hide multiple power
>       estimation methods, the OPP library appears to be a good candidate to
>       implement the required platform abstraction and respect the existing
>       design (the thermal subsystem already relies on the OPP library to
>       provide voltages no matter where they come from -- DT or SCPI).
> 
>    2. The energy models of CPUs will be needed by other clients in the future
>       (such as the task scheduler or CPUFreq governors for example) in order
>       to make energy-aware decisions. The relocation to the OPP library will
>       enable code re-use and all clients will benefit form the platform
>       abstraction mentioned previously.
> 
> As mentionned during the review of V1, this series will have a chance to get
> merged only when the clients (the scheduler, or the SCMI driver) depending
> on it are ready to be merged as well. This V3 is here for reference and
> addresses the main changes requested on the previous versions.
> 
> Changes in v2:
>  - Changed the OPP power estimation to be done upon OPP creation (V. Kumar)
>  - Updated the cover letter to motivate the choice of PM OPP (E. Valentin)
> 
> Changes in v3
>  - Removed the has_power logic to simplify the thermal code (V. Kumar)
>  - Moved _opp_estimate_power() call to _opp_add() to avoid duplication (V. Kumar)

So I am mostly fine with the changes here and can apply them to the OPP tree as
soon as Eduardo Acks them (as he had some concerns about the users).

Thanks.

-- 
viresh