[PATCH v5 0/5] Devfreq cooling device

[PATCH v5 0/5] Devfreq cooling device

[Javi Merino]

This series introduces a devfreq cooling device in the thermal
framework.  Devfreq is used for DVFS for devices other than the CPUs.
With a devfreq cooling device, the thermal framework can throttle them
to control temperature.  The cooling device has the power extensions,
so it can be used by all governors in the thermal framework, including
the power allocator governor.

Changes since v4:
  - Don't introduce a new function in the OPP library and instead fix
    dev_pm_opp_get_voltage() as suggested by Viresh Kumar
  - Don't allocate memory under RCU
  - Don't call dev_pm_opp_enable/disable under RCU
  - Generate the frequency table even if the power extensions were
    not provided

Changes since v3:
  - Made devfreq_update_stats() a static inline function
  - Add dev_pm_get_voltage_always() to get the voltage even for
    disabled OPPs
  - Don't rely on freq_table from the devfreq->profile being present
    and create our own
  - Don't use devm_k* to allocate memory
  - Move struct devfreq_cooling_register to devfreq_cooling.c

Javi Merino (4):
  PM / devfreq: cache the last call to get_dev_status()
  PM / OPP: get the voltage for all OPPs
  devfreq_cooling: add trace information
  PM / devfreq: drop comment about thermal setting max_freq

Ørjan Eide (1):
  thermal: Add devfreq cooling

 drivers/base/power/opp.c                  |   4 +-
 drivers/devfreq/devfreq.c                 |   6 +-
 drivers/devfreq/governor_simpleondemand.c |  33 +-
 drivers/thermal/Kconfig                   |  11 +
 drivers/thermal/Makefile                  |   3 +
 drivers/thermal/devfreq_cooling.c         | 552 ++++++++++++++++++++++++++++++
 include/linux/devfreq.h                   |  15 +
 include/linux/devfreq_cooling.h           |  72 ++++
 include/trace/events/thermal.h            |  53 +++
 9 files changed, 729 insertions(+), 20 deletions(-)
 create mode 100644 drivers/thermal/devfreq_cooling.c
 create mode 100644 include/linux/devfreq_cooling.h

-- 
1.9.1

[PATCH v5 1/5] PM / devfreq: cache the last call to get_dev_status()

[Javi Merino]

The return value of get_dev_status() can be reused.  Cache it so that
other parts of the kernel can reuse it instead of having to call the
same function again.

Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
 drivers/devfreq/governor_simpleondemand.c | 33 +++++++++++++++++--------------
 include/linux/devfreq.h                   | 15 ++++++++++++++
 2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
index 0720ba84ca92..ae72ba5e78df 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -21,17 +21,20 @@
 static int devfreq_simple_ondemand_func(struct devfreq *df,
 					unsigned long *freq)
 {
-	struct devfreq_dev_status stat;
-	int err = df->profile->get_dev_status(df->dev.parent, &stat);
+	int err;
+	struct devfreq_dev_status *stat;
 	unsigned long long a, b;
 	unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD;
 	unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL;
 	struct devfreq_simple_ondemand_data *data = df->data;
 	unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX;
 
+	err = devfreq_update_stats(df);
 	if (err)
 		return err;
 
+	stat = &df->last_status;
+
 	if (data) {
 		if (data->upthreshold)
 			dfso_upthreshold = data->upthreshold;
@@ -43,41 +46,41 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
 		return -EINVAL;
 
 	/* Assume MAX if it is going to be divided by zero */
-	if (stat.total_time == 0) {
+	if (stat->total_time == 0) {
 		*freq = max;
 		return 0;
 	}
 
 	/* Prevent overflow */
-	if (stat.busy_time >= (1 << 24) || stat.total_time >= (1 << 24)) {
-		stat.busy_time >>= 7;
-		stat.total_time >>= 7;
+	if (stat->busy_time >= (1 << 24) || stat->total_time >= (1 << 24)) {
+		stat->busy_time >>= 7;
+		stat->total_time >>= 7;
 	}
 
 	/* Set MAX if it's busy enough */
-	if (stat.busy_time * 100 >
-	    stat.total_time * dfso_upthreshold) {
+	if (stat->busy_time * 100 >
+	    stat->total_time * dfso_upthreshold) {
 		*freq = max;
 		return 0;
 	}
 
 	/* Set MAX if we do not know the initial frequency */
-	if (stat.current_frequency == 0) {
+	if (stat->current_frequency == 0) {
 		*freq = max;
 		return 0;
 	}
 
 	/* Keep the current frequency */
-	if (stat.busy_time * 100 >
-	    stat.total_time * (dfso_upthreshold - dfso_downdifferential)) {
-		*freq = stat.current_frequency;
+	if (stat->busy_time * 100 >
+	    stat->total_time * (dfso_upthreshold - dfso_downdifferential)) {
+		*freq = stat->current_frequency;
 		return 0;
 	}
 
 	/* Set the desired frequency based on the load */
-	a = stat.busy_time;
-	a *= stat.current_frequency;
-	b = div_u64(a, stat.total_time);
+	a = stat->busy_time;
+	a *= stat->current_frequency;
+	b = div_u64(a, stat->total_time);
 	b *= 100;
 	b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2));
 	*freq = (unsigned long) b;
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index ce447f0f1bad..70a1c60ddda4 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -161,6 +161,7 @@ struct devfreq {
 	struct delayed_work work;
 
 	unsigned long previous_freq;
+	struct devfreq_dev_status last_status;
 
 	void *data; /* private data for governors */
 
@@ -204,6 +205,15 @@ extern int devm_devfreq_register_opp_notifier(struct device *dev,
 extern void devm_devfreq_unregister_opp_notifier(struct device *dev,
 						struct devfreq *devfreq);
 
+/**
+ * devfreq_update_stats() - update the last_status pointer in struct devfreq
+ * @df:		the devfreq instance whose status needs updating
+ */
+static inline int devfreq_update_stats(struct devfreq *df)
+{
+	return df->profile->get_dev_status(df->dev.parent, &df->last_status);
+}
+
 #if IS_ENABLED(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND)
 /**
  * struct devfreq_simple_ondemand_data - void *data fed to struct devfreq
@@ -289,6 +299,11 @@ static inline void devm_devfreq_unregister_opp_notifier(struct device *dev,
 							struct devfreq *devfreq)
 {
 }
+
+static inline int devfreq_update_stats(struct devfreq *df)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_PM_DEVFREQ */
 
 #endif /* __LINUX_DEVFREQ_H__ */
-- 
1.9.1

[PATCH v5 2/5] PM / OPP: get the voltage for all OPPs

[Javi Merino]

The OPP library is now used for power models to calculate the power
that a device would consume at a specific OPP.  To do that, we use a
simple power model which takes frequency and voltage as inputs.  We get
the voltage and frequency from the OPP library.

The devfreq cooling device for the thermal framework controls
temperature by disabling OPPs.  The power model needs to calculate the
power that would be consumed if we reenabled the OPP.  Therefore, let
dev_pm_opp_get_voltage() work for disabled OPPs.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
 drivers/base/power/opp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 677fb2843553..3e5f7ae29ef9 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -150,7 +150,7 @@ static struct device_opp *_find_device_opp(struct device *dev)
 }
 
 /**
- * dev_pm_opp_get_voltage() - Gets the voltage corresponding to an available opp
+ * dev_pm_opp_get_voltage() - Gets the voltage corresponding to an opp
  * @opp:	opp for which voltage has to be returned for
  *
  * Return: voltage in micro volt corresponding to the opp, else
@@ -172,7 +172,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 	opp_rcu_lockdep_assert();
 
 	tmp_opp = rcu_dereference(opp);
-	if (unlikely(IS_ERR_OR_NULL(tmp_opp)) || !tmp_opp->available)
+	if (unlikely(IS_ERR_OR_NULL(tmp_opp)))
 		pr_err("%s: Invalid parameters\n", __func__);
 	else
 		v = tmp_opp->u_volt;
-- 
1.9.1

Re: [PATCH v5 2/5] PM / OPP: get the voltage for all OPPs

[Viresh Kumar]

On 27-08-15, 11:55, Javi Merino wrote:
> The OPP library is now used for power models to calculate the power
> that a device would consume at a specific OPP.  To do that, we use a
> simple power model which takes frequency and voltage as inputs.  We get
> the voltage and frequency from the OPP library.
> 
> The devfreq cooling device for the thermal framework controls
> temperature by disabling OPPs.  The power model needs to calculate the
> power that would be consumed if we reenabled the OPP.  Therefore, let
> dev_pm_opp_get_voltage() work for disabled OPPs.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
>  drivers/base/power/opp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 677fb2843553..3e5f7ae29ef9 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -150,7 +150,7 @@ static struct device_opp *_find_device_opp(struct device *dev)
>  }
>  
>  /**
> - * dev_pm_opp_get_voltage() - Gets the voltage corresponding to an available opp
> + * dev_pm_opp_get_voltage() - Gets the voltage corresponding to an opp
>   * @opp:	opp for which voltage has to be returned for
>   *
>   * Return: voltage in micro volt corresponding to the opp, else
> @@ -172,7 +172,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
>  	opp_rcu_lockdep_assert();
>  
>  	tmp_opp = rcu_dereference(opp);
> -	if (unlikely(IS_ERR_OR_NULL(tmp_opp)) || !tmp_opp->available)
> +	if (unlikely(IS_ERR_OR_NULL(tmp_opp)))
>  		pr_err("%s: Invalid parameters\n", __func__);
>  	else
>  		v = tmp_opp->u_volt;

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

-- 
viresh

[PATCH v5 3/5] thermal: Add devfreq cooling

[Javi Merino]

From: Ørjan Eide <orjan.eide@arm.com>

Add a generic thermal cooling device for devfreq, that is similar to
cpu_cooling.

The device must use devfreq.  In order to use the power extension of the
cooling device, it must have registered its OPPs using the OPP library.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
Signed-off-by: Ørjan Eide <orjan.eide@arm.com>
---

I had a look at 02373d7c69b4 ("thermal: cpu_cooling: fix lockdep
problems in cpu_cooling").  It doesn't affect devfreq cooling because
we don't have notifiers, we only use locking for idr.

 drivers/thermal/Kconfig           |  11 +
 drivers/thermal/Makefile          |   3 +
 drivers/thermal/devfreq_cooling.c | 546 ++++++++++++++++++++++++++++++++++++++
 include/linux/devfreq_cooling.h   |  72 +++++
 4 files changed, 632 insertions(+)
 create mode 100644 drivers/thermal/devfreq_cooling.c
 create mode 100644 include/linux/devfreq_cooling.h

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 118938ee8552..a2c6a6497804 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -147,6 +147,17 @@ config CLOCK_THERMAL
 	  device that is configured to use this cooling mechanism will be
 	  controlled to reduce clock frequency whenever temperature is high.
 
+config DEVFREQ_THERMAL
+	bool "Generic device cooling support"
+	depends on PM_DEVFREQ
+	depends on PM_OPP
+	help
+	  This implements the generic devfreq cooling mechanism through
+	  frequency reduction for devices using devfreq.
+
+	  This will throttle the device by limiting the maximum allowed DVFS
+	  frequency corresponding to the cooling level.
+
 	  If you want this support, you should say Y here.
 
 config THERMAL_EMULATION
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 535dfee1496f..45f26978ff74 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -22,6 +22,9 @@ thermal_sys-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
 # clock cooling
 thermal_sys-$(CONFIG_CLOCK_THERMAL)	+= clock_cooling.o
 
+# devfreq cooling
+thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
+
 # platform thermal drivers
 obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
 obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
new file mode 100644
index 000000000000..3d4abc746099
--- /dev/null
+++ b/drivers/thermal/devfreq_cooling.c
@@ -0,0 +1,546 @@
+/*
+ * devfreq_cooling: Thermal cooling device implementation for devices using
+ *                  devfreq
+ *
+ * Copyright (C) 2014-2015 ARM Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * TODO:
+ *    - If OPPs are added or removed after devfreq cooling has
+ *      registered, the devfreq cooling won't react to it.
+ */
+
+#include <linux/devfreq.h>
+#include <linux/devfreq_cooling.h>
+#include <linux/export.h>
+#include <linux/slab.h>
+#include <linux/pm_opp.h>
+#include <linux/thermal.h>
+
+static DEFINE_MUTEX(devfreq_lock);
+static DEFINE_IDR(devfreq_idr);
+
+/**
+ * struct devfreq_cooling_device - Devfreq cooling device
+ * @id:		unique integer value corresponding to each
+ *		devfreq_cooling_device registered.
+ * @cdev:	Pointer to associated thermal cooling device.
+ * @devfreq:	Pointer to associated devfreq device.
+ * @cooling_state:	Current cooling state.
+ * @power_table:	Pointer to table with maximum power draw for each
+ *			cooling state. State is the index into the table, and
+ *			the power is in mW.
+ * @freq_table:	Pointer to a table with the frequencies sorted in descending
+ *		order.  You can index the table by cooling device state
+ * @freq_table_size:	size of the @freq_table and @power_table
+ * @power_ops:	Pointer to power operations, used to generate @power_table.
+ */
+struct devfreq_cooling_device {
+	int id;
+	struct thermal_cooling_device *cdev;
+	struct devfreq *devfreq;
+	unsigned long cooling_state;
+	u32 *power_table;
+	u32 *freq_table;
+	size_t freq_table_size;
+	struct devfreq_cooling_ops *power_ops;
+};
+
+/**
+ * get_idr - function to get a unique id.
+ * @idr: struct idr * handle used to create a id.
+ * @id: int * value generated by this function.
+ *
+ * This function will populate @id with an unique
+ * id, using the idr API.
+ *
+ * Return: 0 on success, an error code on failure.
+ */
+static int get_idr(struct idr *idr, int *id)
+{
+	int ret;
+
+	mutex_lock(&devfreq_lock);
+	ret = idr_alloc(idr, NULL, 0, 0, GFP_KERNEL);
+	mutex_unlock(&devfreq_lock);
+	if (unlikely(ret < 0))
+		return ret;
+	*id = ret;
+
+	return 0;
+}
+
+/**
+ * release_idr - function to free the unique id.
+ * @idr: struct idr * handle used for creating the id.
+ * @id: int value representing the unique id.
+ */
+static void release_idr(struct idr *idr, int id)
+{
+	mutex_lock(&devfreq_lock);
+	idr_remove(idr, id);
+	mutex_unlock(&devfreq_lock);
+}
+
+/**
+ * enable_disable_opps() - disable all opps above a given state
+ * @dfc:	Pointer to devfreq we are operating on
+ * @cdev_state:	cooling device state we're setting
+ *
+ * Go through the OPPs of the device, enabling all OPPs until
+ * @cdev_state and disabling those frequencies above it.
+ */
+static int enable_disable_opps(struct devfreq_cooling_device *dfc,
+			       unsigned long cdev_state)
+{
+	int i;
+	struct device *dev = dfc->devfreq->dev.parent;
+
+	for (i = 0; i < dfc->freq_table_size; i++) {
+		struct dev_pm_opp *opp;
+		int ret = 0;
+		unsigned int freq = dfc->freq_table[i];
+		bool want_enable = i >= cdev_state ? true : false;
+
+		rcu_read_lock();
+		opp = dev_pm_opp_find_freq_exact(dev, freq, !want_enable);
+		rcu_read_unlock();
+
+		if (PTR_ERR(opp) == -ERANGE)
+			continue;
+		else if (IS_ERR(opp))
+			return PTR_ERR(opp);
+
+		if (want_enable)
+			ret = dev_pm_opp_enable(dev, freq);
+		else
+			ret = dev_pm_opp_disable(dev, freq);
+
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int devfreq_cooling_get_max_state(struct thermal_cooling_device *cdev,
+					 unsigned long *state)
+{
+	struct devfreq_cooling_device *dfc = cdev->devdata;
+
+	*state = dfc->freq_table_size - 1;
+
+	return 0;
+}
+
+static int devfreq_cooling_get_cur_state(struct thermal_cooling_device *cdev,
+					 unsigned long *state)
+{
+	struct devfreq_cooling_device *dfc = cdev->devdata;
+
+	*state = dfc->cooling_state;
+
+	return 0;
+}
+
+static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
+					 unsigned long state)
+{
+	struct devfreq_cooling_device *dfc = cdev->devdata;
+	struct devfreq *df = dfc->devfreq;
+	struct device *dev = df->dev.parent;
+	int ret;
+
+	if (state == dfc->cooling_state)
+		return 0;
+
+	dev_dbg(dev, "Setting cooling state %lu\n", state);
+
+	if (state >= dfc->freq_table_size)
+		return -EINVAL;
+
+	ret = enable_disable_opps(dfc, state);
+	if (ret)
+		return ret;
+
+	dfc->cooling_state = state;
+
+	return 0;
+}
+
+/**
+ * freq_get_state() - get the cooling state corresponding to a frequency
+ * @dfc:	Pointer to devfreq cooling device
+ * @freq:	frequency in Hz
+ *
+ * Return: the cooling state associated with the @freq, or
+ * THERMAL_CSTATE_INVALID if it wasn't found.
+ */
+static unsigned long
+freq_get_state(struct devfreq_cooling_device *dfc, unsigned long freq)
+{
+	int i;
+
+	for (i = 0; i < dfc->freq_table_size; i++) {
+		if (dfc->freq_table[i] == freq)
+			return i;
+	}
+
+	return THERMAL_CSTATE_INVALID;
+}
+
+/**
+ * state_get_freq() - get the frequency corresponding to a cooling state
+ * @dfc:	Pointer to devfreq cooling device
+ * @state:	cooling device state
+ *
+ * Return: the frequency for this cooling device state or 0 for
+ * invalid states.
+ */
+static unsigned long
+state_get_freq(struct devfreq_cooling_device *dfc, unsigned long state)
+{
+	if (state >= dfc->freq_table_size) {
+		dev_warn(dfc->devfreq->dev.parent,
+			 "State %lu bigger than frequency table\n", state);
+		return 0;
+	}
+
+	return dfc->freq_table[state];
+}
+
+static unsigned long
+get_static_power(struct devfreq_cooling_device *dfc, unsigned long freq)
+{
+	struct devfreq *df = dfc->devfreq;
+	struct device *dev = df->dev.parent;
+	unsigned long voltage;
+	struct dev_pm_opp *opp;
+
+	rcu_read_lock();
+
+	opp = dev_pm_opp_find_freq_exact(dev, freq, true);
+	if (IS_ERR(opp) && (PTR_ERR(opp) == -ERANGE))
+		opp = dev_pm_opp_find_freq_exact(dev, freq, false);
+
+	voltage = dev_pm_opp_get_voltage(opp) / 1000; /* mV */
+
+	rcu_read_unlock();
+
+	if (voltage == 0) {
+		dev_warn_ratelimited(dev,
+				     "Failed to get voltage for frequency %lu: %ld\n",
+				     freq, IS_ERR(opp) ? PTR_ERR(opp) : 0);
+		return 0;
+	}
+
+	return dfc->power_ops->get_static_power(voltage);
+}
+
+static int devfreq_cooling_get_requested_power(struct thermal_cooling_device *cdev,
+					       struct thermal_zone_device *tz,
+					       u32 *power)
+{
+	struct devfreq_cooling_device *dfc = cdev->devdata;
+	struct devfreq *df = dfc->devfreq;
+	struct devfreq_dev_status *status = &df->last_status;
+	unsigned long state;
+	unsigned long freq = status->current_frequency;
+	u32 dyn_power, static_power;
+
+	/* Get dynamic power for state */
+	state = freq_get_state(dfc, freq);
+	if (state == THERMAL_CSTATE_INVALID)
+		return -EAGAIN;
+
+	dyn_power = dfc->power_table[state];
+
+	/* Scale dynamic power for utilization */
+	dyn_power = (dyn_power * status->busy_time) / status->total_time;
+
+	/* Get static power */
+	static_power = get_static_power(dfc, freq);
+
+	*power = dyn_power + static_power;
+
+	return 0;
+}
+
+static int devfreq_cooling_state2power(struct thermal_cooling_device *cdev,
+				       struct thermal_zone_device *tz,
+				       unsigned long state,
+				       u32 *power)
+{
+	struct devfreq_cooling_device *dfc = cdev->devdata;
+	unsigned long freq;
+	u32 static_power;
+
+	freq = state_get_freq(dfc, state);
+	static_power = get_static_power(dfc, freq);
+
+	*power = dfc->power_table[state] + static_power;
+	return 0;
+}
+
+static int devfreq_cooling_power2state(struct thermal_cooling_device *cdev,
+				       struct thermal_zone_device *tz,
+				       u32 power, unsigned long *state)
+{
+	struct devfreq_cooling_device *dfc = cdev->devdata;
+	struct devfreq *df = dfc->devfreq;
+	struct devfreq_dev_status *status = &df->last_status;
+	unsigned long freq = status->current_frequency;
+	unsigned long busy_time;
+	s32 dyn_power;
+	u32 static_power;
+	int i;
+
+	static_power = get_static_power(dfc, freq);
+
+	dyn_power = power - static_power;
+	dyn_power = dyn_power > 0 ? dyn_power : 0;
+
+	/* Scale dynamic power for utilization */
+	busy_time = status->busy_time ?: 1;
+	dyn_power = (dyn_power * status->total_time) / busy_time;
+
+	/*
+	 * Find the first cooling state that is within the power
+	 * budget for dynamic power.
+	 */
+	for (i = 0; i < dfc->freq_table_size - 1; i++)
+		if (dyn_power >= dfc->power_table[i])
+			break;
+
+	*state = i;
+	return 0;
+}
+
+static struct thermal_cooling_device_ops devfreq_cooling_ops = {
+	.get_max_state = devfreq_cooling_get_max_state,
+	.get_cur_state = devfreq_cooling_get_cur_state,
+	.set_cur_state = devfreq_cooling_set_cur_state,
+};
+
+/**
+ * devfreq_cooling_gen_tables() - Generate power and freq tables.
+ * @dfc: Pointer to devfreq cooling device.
+ *
+ * Generate power and frequency tables: the power table hold the
+ * device's maximum power usage at each cooling state (OPP).  The
+ * static and dynamic power using the appropriate voltage and
+ * frequency for the state, is acquired from the struct
+ * devfreq_cooling_ops, and summed to make the maximum power draw.
+ *
+ * The frequency table holds the frequencies in descending order.
+ * That way its indexed by cooling device state.
+ *
+ * The tables are malloced, and pointers put in dfc.  They must be
+ * freed when unregistering the devfreq cooling device.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+static int devfreq_cooling_gen_tables(struct devfreq_cooling_device *dfc)
+{
+	struct devfreq *df = dfc->devfreq;
+	struct device *dev = df->dev.parent;
+	int ret, num_opps;
+	struct devfreq_cooling_ops *callbacks = dfc->power_ops;
+	unsigned long freq;
+	u32 *power_table = NULL;
+	u32 *freq_table;
+	int i;
+
+	num_opps = dev_pm_opp_get_opp_count(dev);
+
+	if (callbacks) {
+		power_table = kcalloc(num_opps, sizeof(*power_table),
+				      GFP_KERNEL);
+		if (!power_table)
+			ret = -ENOMEM;
+	}
+
+	freq_table = kcalloc(num_opps, sizeof(*freq_table),
+			     GFP_KERNEL);
+	if (!freq_table) {
+		ret = -ENOMEM;
+		goto free_power_table;
+	}
+
+	for (i = 0, freq = ULONG_MAX; i < num_opps; i++, freq--) {
+		unsigned long power_dyn, voltage;
+		struct dev_pm_opp *opp;
+
+		rcu_read_lock();
+
+		opp = dev_pm_opp_find_freq_floor(dev, &freq);
+		if (IS_ERR(opp)) {
+			rcu_read_unlock();
+			ret = PTR_ERR(opp);
+			goto free_tables;
+		}
+
+		voltage = dev_pm_opp_get_voltage(opp) / 1000; /* mV */
+
+		rcu_read_unlock();
+
+		if (callbacks) {
+			power_dyn = callbacks->get_dynamic_power(freq, voltage);
+
+			dev_info(dev, "Dynamic power table: %lu MHz @ %lu mV: %lu = %lu mW\n",
+				 freq / 1000000, voltage, power_dyn, power_dyn);
+
+			power_table[i] = power_dyn;
+		}
+
+		freq_table[i] = freq;
+	}
+
+	if (callbacks)
+		dfc->power_table = power_table;
+
+	dfc->freq_table = freq_table;
+	dfc->freq_table_size = num_opps;
+
+	return 0;
+
+free_tables:
+	kfree(freq_table);
+free_power_table:
+	kfree(power_table);
+
+	return ret;
+}
+
+/**
+ * of_devfreq_cooling_register_power() - Register devfreq cooling device,
+ *                                      with OF and power information.
+ * @np: Pointer to OF device_node.
+ * @df: Pointer to devfreq device.
+ * @ops: Pointer to power ops.
+ *
+ * Register a devfreq cooling device.  The available OPPs must be
+ * registered on the device.
+ *
+ * If @ops is provided, the cooling device is registered with the
+ * power extensions.  For the power extensions to work correctly,
+ * devfreq should use the simple_ondemand governor, other governors
+ * are not currently supported.
+ */
+struct devfreq_cooling_device *
+of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
+				  struct devfreq_cooling_ops *ops)
+{
+	struct thermal_cooling_device *cdev;
+	struct devfreq_cooling_device *dfc;
+	char dev_name[THERMAL_NAME_LENGTH];
+	int err;
+
+	dfc = kzalloc(sizeof(*dfc), GFP_KERNEL);
+	if (!dfc)
+		return ERR_PTR(-ENOMEM);
+
+	dfc->devfreq = df;
+
+	if (ops) {
+		if (strncmp(df->governor_name, "simple_ondemand",
+			    sizeof("simple_ondemand")))
+			dev_warn(df->dev.parent,
+				 "Warning: Registering devfreq cooling device with power extensions but devfreq governor is not simple_ondemand\n");
+
+		if (!ops->get_static_power || !ops->get_dynamic_power) {
+			err = -EINVAL;
+			goto free_dfc;
+		}
+		dfc->power_ops = ops;
+
+		devfreq_cooling_ops.get_requested_power =
+			devfreq_cooling_get_requested_power;
+		devfreq_cooling_ops.state2power = devfreq_cooling_state2power;
+		devfreq_cooling_ops.power2state = devfreq_cooling_power2state;
+	}
+
+	err = devfreq_cooling_gen_tables(dfc);
+	if (err)
+		goto free_dfc;
+
+	err = get_idr(&devfreq_idr, &dfc->id);
+	if (err)
+		goto free_tables;
+
+	snprintf(dev_name, sizeof(dev_name), "devfreq-%d", dfc->id);
+
+	cdev = thermal_of_cooling_device_register(np, dev_name, dfc,
+						  &devfreq_cooling_ops);
+	if (IS_ERR(cdev)) {
+		err = PTR_ERR(cdev);
+		dev_err(df->dev.parent,
+			"Failed to register devfreq cooling device (%d)\n",
+			err);
+		goto release_idr;
+	}
+
+	dfc->cdev = cdev;
+
+	return dfc;
+
+release_idr:
+	release_idr(&devfreq_idr, dfc->id);
+free_tables:
+	kfree(dfc->power_table);
+	kfree(dfc->freq_table);
+free_dfc:
+	kfree(dfc);
+
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL(of_devfreq_cooling_register_power);
+
+/**
+ * of_devfreq_cooling_register() - Register devfreq cooling device,
+ *                                with OF information.
+ * @np: Pointer to OF device_node.
+ * @df: Pointer to devfreq device.
+ */
+struct devfreq_cooling_device *
+of_devfreq_cooling_register(struct device_node *np, struct devfreq *df)
+{
+	return of_devfreq_cooling_register_power(np, df, NULL);
+}
+EXPORT_SYMBOL(of_devfreq_cooling_register);
+
+/**
+ * devfreq_cooling_register() - Register devfreq cooling device.
+ * @df: Pointer to devfreq device.
+ */
+struct devfreq_cooling_device *devfreq_cooling_register(struct devfreq *df)
+{
+	return of_devfreq_cooling_register(NULL, df);
+}
+EXPORT_SYMBOL(devfreq_cooling_register);
+
+/**
+ * devfreq_cooling_unregister() - Unregister devfreq cooling device.
+ * @dfc: Pointer to devfreq cooling device to unregister.
+ */
+void devfreq_cooling_unregister(struct devfreq_cooling_device *dfc)
+{
+	if (!dfc)
+		return;
+
+	thermal_cooling_device_unregister(dfc->cdev);
+	release_idr(&devfreq_idr, dfc->id);
+	kfree(dfc->power_table);
+	kfree(dfc->freq_table);
+
+	kfree(dfc);
+}
+EXPORT_SYMBOL(devfreq_cooling_unregister);
diff --git a/include/linux/devfreq_cooling.h b/include/linux/devfreq_cooling.h
new file mode 100644
index 000000000000..de866200efe0
--- /dev/null
+++ b/include/linux/devfreq_cooling.h
@@ -0,0 +1,72 @@
+/*
+ * devfreq_cooling: Thermal cooling device implementation for devices using
+ *                  devfreq
+ *
+ * Copyright (C) 2014-2015 ARM Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __DEVFREQ_COOLING_H__
+#define __DEVFREQ_COOLING_H__
+
+#include <linux/devfreq.h>
+#include <linux/thermal.h>
+
+#ifdef CONFIG_DEVFREQ_THERMAL
+
+/**
+ * struct devfreq_cooling_ops - Devfreq cooling power ops
+ * @get_static_power: Take voltage, in mV, and return the static power in mW.
+ * @get_dynamic_power: Take voltage, in mV, and frequency, in HZ, and return
+ * the dynamic power draw in mW.
+ */
+struct devfreq_cooling_ops {
+	unsigned long (*get_static_power)(unsigned long voltage);
+	unsigned long (*get_dynamic_power)(unsigned long freq,
+					   unsigned long voltage);
+};
+
+struct devfreq_cooling_device *
+of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
+				  struct devfreq_cooling_ops *ops);
+struct devfreq_cooling_device *
+of_devfreq_cooling_register(struct device_node *np, struct devfreq *df);
+struct devfreq_cooling_device *devfreq_cooling_register(struct devfreq *df);
+void devfreq_cooling_unregister(struct devfreq_cooling_device *dfc);
+
+#else /* !CONFIG_DEVFREQ_THERMAL */
+
+struct devfreq_cooling_device *
+of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
+				  struct devfreq_cooling_ops *ops)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline struct devfreq_cooling_device *
+of_devfreq_cooling_register(struct device_node *np, struct devfreq *df)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline struct devfreq_cooling_device *
+devfreq_cooling_register(struct devfreq *df)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline void
+devfreq_cooling_unregister(struct devfreq_cooling_device *dfc)
+{
+}
+
+#endif /* CONFIG_DEVFREQ_THERMAL */
+#endif /* __DEVFREQ_COOLING_H__ */
-- 
1.9.1

Re: [PATCH v5 3/5] thermal: Add devfreq cooling

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 23215 bytes --]

Hi

On Thu, Aug 27, 2015 at 11:55:49AM +0100, Javi Merino wrote:
> From: Ørjan Eide <orjan.eide@arm.com>
> 
> Add a generic thermal cooling device for devfreq, that is similar to
> cpu_cooling.
> 
> The device must use devfreq.  In order to use the power extension of the
> cooling device, it must have registered its OPPs using the OPP library.
> 
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> Signed-off-by: Ørjan Eide <orjan.eide@arm.com>

Thanks for taking this to upstream kernel.

Just minor comments as follows.

> ---
> 
> I had a look at 02373d7c69b4 ("thermal: cpu_cooling: fix lockdep
> problems in cpu_cooling").  It doesn't affect devfreq cooling because
> we don't have notifiers, we only use locking for idr.
> 

Thanks once again for checking it.

>  drivers/thermal/Kconfig           |  11 +
>  drivers/thermal/Makefile          |   3 +
>  drivers/thermal/devfreq_cooling.c | 546 ++++++++++++++++++++++++++++++++++++++
>  include/linux/devfreq_cooling.h   |  72 +++++
>  4 files changed, 632 insertions(+)
>  create mode 100644 drivers/thermal/devfreq_cooling.c
>  create mode 100644 include/linux/devfreq_cooling.h
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 118938ee8552..a2c6a6497804 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -147,6 +147,17 @@ config CLOCK_THERMAL
>  	  device that is configured to use this cooling mechanism will be
>  	  controlled to reduce clock frequency whenever temperature is high.
>  
> +config DEVFREQ_THERMAL
> +	bool "Generic device cooling support"
> +	depends on PM_DEVFREQ
> +	depends on PM_OPP
> +	help
> +	  This implements the generic devfreq cooling mechanism through
> +	  frequency reduction for devices using devfreq.
> +
> +	  This will throttle the device by limiting the maximum allowed DVFS
> +	  frequency corresponding to the cooling level.
> +
>  	  If you want this support, you should say Y here.
>  
>  config THERMAL_EMULATION
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 535dfee1496f..45f26978ff74 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -22,6 +22,9 @@ thermal_sys-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
>  # clock cooling
>  thermal_sys-$(CONFIG_CLOCK_THERMAL)	+= clock_cooling.o
>  
> +# devfreq cooling
> +thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
> +
>  # platform thermal drivers
>  obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
>  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
> diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
> new file mode 100644
> index 000000000000..3d4abc746099
> --- /dev/null
> +++ b/drivers/thermal/devfreq_cooling.c
> @@ -0,0 +1,546 @@
> +/*
> + * devfreq_cooling: Thermal cooling device implementation for devices using
> + *                  devfreq
> + *
> + * Copyright (C) 2014-2015 ARM Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * TODO:
> + *    - If OPPs are added or removed after devfreq cooling has
> + *      registered, the devfreq cooling won't react to it.
> + */
> +
> +#include <linux/devfreq.h>
> +#include <linux/devfreq_cooling.h>
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +#include <linux/pm_opp.h>
> +#include <linux/thermal.h>
> +
> +static DEFINE_MUTEX(devfreq_lock);
> +static DEFINE_IDR(devfreq_idr);
> +
> +/**
> + * struct devfreq_cooling_device - Devfreq cooling device
> + * @id:		unique integer value corresponding to each
> + *		devfreq_cooling_device registered.
> + * @cdev:	Pointer to associated thermal cooling device.
> + * @devfreq:	Pointer to associated devfreq device.
> + * @cooling_state:	Current cooling state.
> + * @power_table:	Pointer to table with maximum power draw for each
> + *			cooling state. State is the index into the table, and
> + *			the power is in mW.
> + * @freq_table:	Pointer to a table with the frequencies sorted in descending
> + *		order.  You can index the table by cooling device state
> + * @freq_table_size:	size of the @freq_table and @power_table
> + * @power_ops:	Pointer to power operations, used to generate @power_table.
> + */
> +struct devfreq_cooling_device {
> +	int id;
> +	struct thermal_cooling_device *cdev;
> +	struct devfreq *devfreq;
> +	unsigned long cooling_state;
> +	u32 *power_table;
> +	u32 *freq_table;
> +	size_t freq_table_size;
> +	struct devfreq_cooling_ops *power_ops;
> +};
> +
> +/**
> + * get_idr - function to get a unique id.
> + * @idr: struct idr * handle used to create a id.
> + * @id: int * value generated by this function.
> + *
> + * This function will populate @id with an unique
> + * id, using the idr API.
> + *
> + * Return: 0 on success, an error code on failure.
> + */
> +static int get_idr(struct idr *idr, int *id)
> +{
> +	int ret;
> +
> +	mutex_lock(&devfreq_lock);
> +	ret = idr_alloc(idr, NULL, 0, 0, GFP_KERNEL);
> +	mutex_unlock(&devfreq_lock);
> +	if (unlikely(ret < 0))
> +		return ret;
> +	*id = ret;
> +
> +	return 0;
> +}
> +
> +/**
> + * release_idr - function to free the unique id.
> + * @idr: struct idr * handle used for creating the id.
> + * @id: int value representing the unique id.
> + */
> +static void release_idr(struct idr *idr, int id)
> +{
> +	mutex_lock(&devfreq_lock);
> +	idr_remove(idr, id);
> +	mutex_unlock(&devfreq_lock);
> +}
> +
> +/**
> + * enable_disable_opps() - disable all opps above a given state

This function name is confusing. But, at the same time, I cannot think
of a name for it right now.

maybe partition_enable_opps()?

> + * @dfc:	Pointer to devfreq we are operating on
> + * @cdev_state:	cooling device state we're setting
> + *
> + * Go through the OPPs of the device, enabling all OPPs until
> + * @cdev_state and disabling those frequencies above it.
> + */
> +static int enable_disable_opps(struct devfreq_cooling_device *dfc,
> +			       unsigned long cdev_state)
> +{
> +	int i;
> +	struct device *dev = dfc->devfreq->dev.parent;
> +
> +	for (i = 0; i < dfc->freq_table_size; i++) {
> +		struct dev_pm_opp *opp;
> +		int ret = 0;
> +		unsigned int freq = dfc->freq_table[i];
> +		bool want_enable = i >= cdev_state ? true : false;
> +
> +		rcu_read_lock();
> +		opp = dev_pm_opp_find_freq_exact(dev, freq, !want_enable);
> +		rcu_read_unlock();
> +
> +		if (PTR_ERR(opp) == -ERANGE)
> +			continue;
> +		else if (IS_ERR(opp))
> +			return PTR_ERR(opp);
> +
> +		if (want_enable)
> +			ret = dev_pm_opp_enable(dev, freq);
> +		else
> +			ret = dev_pm_opp_disable(dev, freq);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int devfreq_cooling_get_max_state(struct thermal_cooling_device *cdev,
> +					 unsigned long *state)
> +{
> +	struct devfreq_cooling_device *dfc = cdev->devdata;
> +
> +	*state = dfc->freq_table_size - 1;
> +
> +	return 0;
> +}
> +
> +static int devfreq_cooling_get_cur_state(struct thermal_cooling_device *cdev,
> +					 unsigned long *state)
> +{
> +	struct devfreq_cooling_device *dfc = cdev->devdata;
> +
> +	*state = dfc->cooling_state;
> +
> +	return 0;
> +}
> +
> +static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
> +					 unsigned long state)
> +{
> +	struct devfreq_cooling_device *dfc = cdev->devdata;
> +	struct devfreq *df = dfc->devfreq;
> +	struct device *dev = df->dev.parent;
> +	int ret;
> +
> +	if (state == dfc->cooling_state)
> +		return 0;
> +
> +	dev_dbg(dev, "Setting cooling state %lu\n", state);
> +
> +	if (state >= dfc->freq_table_size)
> +		return -EINVAL;
> +
> +	ret = enable_disable_opps(dfc, state);
> +	if (ret)
> +		return ret;
> +
> +	dfc->cooling_state = state;
> +
> +	return 0;
> +}
> +
> +/**
> + * freq_get_state() - get the cooling state corresponding to a frequency
> + * @dfc:	Pointer to devfreq cooling device
> + * @freq:	frequency in Hz
> + *
> + * Return: the cooling state associated with the @freq, or
> + * THERMAL_CSTATE_INVALID if it wasn't found.
> + */
> +static unsigned long
> +freq_get_state(struct devfreq_cooling_device *dfc, unsigned long freq)
> +{
> +	int i;
> +
> +	for (i = 0; i < dfc->freq_table_size; i++) {
> +		if (dfc->freq_table[i] == freq)
> +			return i;
> +	}
> +
> +	return THERMAL_CSTATE_INVALID;
> +}
> +
> +/**
> + * state_get_freq() - get the frequency corresponding to a cooling state
> + * @dfc:	Pointer to devfreq cooling device
> + * @state:	cooling device state
> + *
> + * Return: the frequency for this cooling device state or 0 for
> + * invalid states.
> + */
> +static unsigned long
> +state_get_freq(struct devfreq_cooling_device *dfc, unsigned long state)
> +{
> +	if (state >= dfc->freq_table_size) {
> +		dev_warn(dfc->devfreq->dev.parent,
> +			 "State %lu bigger than frequency table\n", state);
> +		return 0;
> +	}
> +
> +	return dfc->freq_table[state];
> +}
> +
> +static unsigned long
> +get_static_power(struct devfreq_cooling_device *dfc, unsigned long freq)
> +{
> +	struct devfreq *df = dfc->devfreq;
> +	struct device *dev = df->dev.parent;
> +	unsigned long voltage;
> +	struct dev_pm_opp *opp;
> +
> +	rcu_read_lock();
> +
> +	opp = dev_pm_opp_find_freq_exact(dev, freq, true);
> +	if (IS_ERR(opp) && (PTR_ERR(opp) == -ERANGE))
> +		opp = dev_pm_opp_find_freq_exact(dev, freq, false);
> +
> +	voltage = dev_pm_opp_get_voltage(opp) / 1000; /* mV */
> +
> +	rcu_read_unlock();
> +
> +	if (voltage == 0) {
> +		dev_warn_ratelimited(dev,
> +				     "Failed to get voltage for frequency %lu: %ld\n",
> +				     freq, IS_ERR(opp) ? PTR_ERR(opp) : 0);
> +		return 0;
> +	}
> +
> +	return dfc->power_ops->get_static_power(voltage);
> +}
> +
> +static int devfreq_cooling_get_requested_power(struct thermal_cooling_device *cdev,
> +					       struct thermal_zone_device *tz,
> +					       u32 *power)
> +{
> +	struct devfreq_cooling_device *dfc = cdev->devdata;
> +	struct devfreq *df = dfc->devfreq;
> +	struct devfreq_dev_status *status = &df->last_status;
> +	unsigned long state;
> +	unsigned long freq = status->current_frequency;
> +	u32 dyn_power, static_power;
> +
> +	/* Get dynamic power for state */
> +	state = freq_get_state(dfc, freq);
> +	if (state == THERMAL_CSTATE_INVALID)
> +		return -EAGAIN;
> +
> +	dyn_power = dfc->power_table[state];
> +
> +	/* Scale dynamic power for utilization */
> +	dyn_power = (dyn_power * status->busy_time) / status->total_time;
> +
> +	/* Get static power */
> +	static_power = get_static_power(dfc, freq);
> +
> +	*power = dyn_power + static_power;
> +
> +	return 0;
> +}
> +
> +static int devfreq_cooling_state2power(struct thermal_cooling_device *cdev,
> +				       struct thermal_zone_device *tz,
> +				       unsigned long state,
> +				       u32 *power)
> +{
> +	struct devfreq_cooling_device *dfc = cdev->devdata;
> +	unsigned long freq;
> +	u32 static_power;
> +
> +	freq = state_get_freq(dfc, state);
> +	static_power = get_static_power(dfc, freq);
> +
> +	*power = dfc->power_table[state] + static_power;

should we have a boundaries check here on state?

> +	return 0;
> +}
> +
> +static int devfreq_cooling_power2state(struct thermal_cooling_device *cdev,
> +				       struct thermal_zone_device *tz,
> +				       u32 power, unsigned long *state)
> +{
> +	struct devfreq_cooling_device *dfc = cdev->devdata;
> +	struct devfreq *df = dfc->devfreq;
> +	struct devfreq_dev_status *status = &df->last_status;
> +	unsigned long freq = status->current_frequency;
> +	unsigned long busy_time;
> +	s32 dyn_power;
> +	u32 static_power;
> +	int i;
> +
> +	static_power = get_static_power(dfc, freq);
> +
> +	dyn_power = power - static_power;
> +	dyn_power = dyn_power > 0 ? dyn_power : 0;
> +
> +	/* Scale dynamic power for utilization */
> +	busy_time = status->busy_time ?: 1;
> +	dyn_power = (dyn_power * status->total_time) / busy_time;
> +
> +	/*
> +	 * Find the first cooling state that is within the power
> +	 * budget for dynamic power.
> +	 */
> +	for (i = 0; i < dfc->freq_table_size - 1; i++)
> +		if (dyn_power >= dfc->power_table[i])
> +			break;
> +
> +	*state = i;
> +	return 0;
> +}
> +
> +static struct thermal_cooling_device_ops devfreq_cooling_ops = {
> +	.get_max_state = devfreq_cooling_get_max_state,
> +	.get_cur_state = devfreq_cooling_get_cur_state,
> +	.set_cur_state = devfreq_cooling_set_cur_state,
> +};
> +
> +/**
> + * devfreq_cooling_gen_tables() - Generate power and freq tables.
> + * @dfc: Pointer to devfreq cooling device.
> + *
> + * Generate power and frequency tables: the power table hold the
> + * device's maximum power usage at each cooling state (OPP).  The
> + * static and dynamic power using the appropriate voltage and
> + * frequency for the state, is acquired from the struct
> + * devfreq_cooling_ops, and summed to make the maximum power draw.
> + *
> + * The frequency table holds the frequencies in descending order.
> + * That way its indexed by cooling device state.
> + *
> + * The tables are malloced, and pointers put in dfc.  They must be
> + * freed when unregistering the devfreq cooling device.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +static int devfreq_cooling_gen_tables(struct devfreq_cooling_device *dfc)
> +{
> +	struct devfreq *df = dfc->devfreq;
> +	struct device *dev = df->dev.parent;
> +	int ret, num_opps;
> +	struct devfreq_cooling_ops *callbacks = dfc->power_ops;
> +	unsigned long freq;
> +	u32 *power_table = NULL;
> +	u32 *freq_table;
> +	int i;
> +
> +	num_opps = dev_pm_opp_get_opp_count(dev);
> +
> +	if (callbacks) {
> +		power_table = kcalloc(num_opps, sizeof(*power_table),
> +				      GFP_KERNEL);
> +		if (!power_table)
> +			ret = -ENOMEM;
> +	}
> +
> +	freq_table = kcalloc(num_opps, sizeof(*freq_table),
> +			     GFP_KERNEL);
> +	if (!freq_table) {
> +		ret = -ENOMEM;
> +		goto free_power_table;
> +	}
> +
> +	for (i = 0, freq = ULONG_MAX; i < num_opps; i++, freq--) {
> +		unsigned long power_dyn, voltage;
> +		struct dev_pm_opp *opp;
> +
> +		rcu_read_lock();
> +
> +		opp = dev_pm_opp_find_freq_floor(dev, &freq);
> +		if (IS_ERR(opp)) {
> +			rcu_read_unlock();
> +			ret = PTR_ERR(opp);
> +			goto free_tables;
> +		}
> +
> +		voltage = dev_pm_opp_get_voltage(opp) / 1000; /* mV */
> +
> +		rcu_read_unlock();
> +
> +		if (callbacks) {
> +			power_dyn = callbacks->get_dynamic_power(freq, voltage);
> +
> +			dev_info(dev, "Dynamic power table: %lu MHz @ %lu mV: %lu = %lu mW\n",

I would prefer this would not be dev_info though..

> +				 freq / 1000000, voltage, power_dyn, power_dyn);
> +
> +			power_table[i] = power_dyn;
> +		}
> +
> +		freq_table[i] = freq;
> +	}
> +
> +	if (callbacks)
> +		dfc->power_table = power_table;
> +
> +	dfc->freq_table = freq_table;
> +	dfc->freq_table_size = num_opps;
> +
> +	return 0;
> +
> +free_tables:
> +	kfree(freq_table);
> +free_power_table:
> +	kfree(power_table);
> +
> +	return ret;
> +}
> +
> +/**
> + * of_devfreq_cooling_register_power() - Register devfreq cooling device,
> + *                                      with OF and power information.
> + * @np: Pointer to OF device_node.
> + * @df: Pointer to devfreq device.
> + * @ops: Pointer to power ops.
> + *
> + * Register a devfreq cooling device.  The available OPPs must be
> + * registered on the device.
> + *
> + * If @ops is provided, the cooling device is registered with the
> + * power extensions.  For the power extensions to work correctly,
> + * devfreq should use the simple_ondemand governor, other governors
> + * are not currently supported.

Fine, but this should not be cooling device concern.

> + */
> +struct devfreq_cooling_device *
> +of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
> +				  struct devfreq_cooling_ops *ops)
> +{
> +	struct thermal_cooling_device *cdev;
> +	struct devfreq_cooling_device *dfc;
> +	char dev_name[THERMAL_NAME_LENGTH];
> +	int err;
> +
> +	dfc = kzalloc(sizeof(*dfc), GFP_KERNEL);
> +	if (!dfc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	dfc->devfreq = df;
> +
> +	if (ops) {
> +		if (strncmp(df->governor_name, "simple_ondemand",
> +			    sizeof("simple_ondemand")))
> +			dev_warn(df->dev.parent,
> +				 "Warning: Registering devfreq cooling device with power extensions but devfreq governor is not simple_ondemand\n");

I don't think this check should actually be in the devfreq cooling
device. I would prefer cooling devices still just provide the
functionality for the governor do what they need to. This is a policy
check, which I don't think should be done here. What if we add another
governor with 'power' extensions? should we come back here and remove
the warn? or add the governor to the check list?

> +
> +		if (!ops->get_static_power || !ops->get_dynamic_power) {

What is the reason we cannot use P=alpha \times f \times V^2 as a
default?

> +			err = -EINVAL;
> +			goto free_dfc;
> +		}
> +		dfc->power_ops = ops;
> +
> +		devfreq_cooling_ops.get_requested_power =
> +			devfreq_cooling_get_requested_power;
> +		devfreq_cooling_ops.state2power = devfreq_cooling_state2power;
> +		devfreq_cooling_ops.power2state = devfreq_cooling_power2state;
> +	}
> +
> +	err = devfreq_cooling_gen_tables(dfc);
> +	if (err)
> +		goto free_dfc;
> +
> +	err = get_idr(&devfreq_idr, &dfc->id);
> +	if (err)
> +		goto free_tables;
> +
> +	snprintf(dev_name, sizeof(dev_name), "devfreq-%d", dfc->id);

the name does not seam to mention anything about thermal or cooling, any
particular reason?

> +
> +	cdev = thermal_of_cooling_device_register(np, dev_name, dfc,
> +						  &devfreq_cooling_ops);
> +	if (IS_ERR(cdev)) {
> +		err = PTR_ERR(cdev);
> +		dev_err(df->dev.parent,
> +			"Failed to register devfreq cooling device (%d)\n",
> +			err);
> +		goto release_idr;
> +	}
> +
> +	dfc->cdev = cdev;
> +
> +	return dfc;
> +
> +release_idr:
> +	release_idr(&devfreq_idr, dfc->id);
> +free_tables:
> +	kfree(dfc->power_table);
> +	kfree(dfc->freq_table);
> +free_dfc:
> +	kfree(dfc);
> +
> +	return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL(of_devfreq_cooling_register_power);
> +
> +/**
> + * of_devfreq_cooling_register() - Register devfreq cooling device,
> + *                                with OF information.
> + * @np: Pointer to OF device_node.
> + * @df: Pointer to devfreq device.
> + */
> +struct devfreq_cooling_device *
> +of_devfreq_cooling_register(struct device_node *np, struct devfreq *df)
> +{
> +	return of_devfreq_cooling_register_power(np, df, NULL);
> +}
> +EXPORT_SYMBOL(of_devfreq_cooling_register);
> +
> +/**
> + * devfreq_cooling_register() - Register devfreq cooling device.
> + * @df: Pointer to devfreq device.
> + */
> +struct devfreq_cooling_device *devfreq_cooling_register(struct devfreq *df)
> +{
> +	return of_devfreq_cooling_register(NULL, df);
> +}
> +EXPORT_SYMBOL(devfreq_cooling_register);
> +
> +/**
> + * devfreq_cooling_unregister() - Unregister devfreq cooling device.
> + * @dfc: Pointer to devfreq cooling device to unregister.
> + */
> +void devfreq_cooling_unregister(struct devfreq_cooling_device *dfc)
> +{
> +	if (!dfc)
> +		return;
> +
> +	thermal_cooling_device_unregister(dfc->cdev);
> +	release_idr(&devfreq_idr, dfc->id);
> +	kfree(dfc->power_table);
> +	kfree(dfc->freq_table);
> +
> +	kfree(dfc);
> +}
> +EXPORT_SYMBOL(devfreq_cooling_unregister);

Any particular reason why this cannot be EXPORT_SYMBOL_GPL (same for all
exports above)?

> diff --git a/include/linux/devfreq_cooling.h b/include/linux/devfreq_cooling.h
> new file mode 100644
> index 000000000000..de866200efe0
> --- /dev/null
> +++ b/include/linux/devfreq_cooling.h
> @@ -0,0 +1,72 @@
> +/*
> + * devfreq_cooling: Thermal cooling device implementation for devices using
> + *                  devfreq
> + *
> + * Copyright (C) 2014-2015 ARM Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __DEVFREQ_COOLING_H__
> +#define __DEVFREQ_COOLING_H__
> +
> +#include <linux/devfreq.h>
> +#include <linux/thermal.h>
> +
> +#ifdef CONFIG_DEVFREQ_THERMAL
> +
> +/**
> + * struct devfreq_cooling_ops - Devfreq cooling power ops
> + * @get_static_power: Take voltage, in mV, and return the static power in mW.
> + * @get_dynamic_power: Take voltage, in mV, and frequency, in HZ, and return
> + * the dynamic power draw in mW.

If you are not providing a default, you must mention that all fields are
required.

> + */
> +struct devfreq_cooling_ops {
> +	unsigned long (*get_static_power)(unsigned long voltage);
> +	unsigned long (*get_dynamic_power)(unsigned long freq,
> +					   unsigned long voltage);
> +};
> +
> +struct devfreq_cooling_device *
> +of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
> +				  struct devfreq_cooling_ops *ops);
> +struct devfreq_cooling_device *
> +of_devfreq_cooling_register(struct device_node *np, struct devfreq *df);
> +struct devfreq_cooling_device *devfreq_cooling_register(struct devfreq *df);
> +void devfreq_cooling_unregister(struct devfreq_cooling_device *dfc);
> +
> +#else /* !CONFIG_DEVFREQ_THERMAL */
> +
> +struct devfreq_cooling_device *
> +of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
> +				  struct devfreq_cooling_ops *ops)
> +{
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static inline struct devfreq_cooling_device *
> +of_devfreq_cooling_register(struct device_node *np, struct devfreq *df)
> +{
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static inline struct devfreq_cooling_device *
> +devfreq_cooling_register(struct devfreq *df)
> +{
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static inline void
> +devfreq_cooling_unregister(struct devfreq_cooling_device *dfc)
> +{
> +}
> +
> +#endif /* CONFIG_DEVFREQ_THERMAL */
> +#endif /* __DEVFREQ_COOLING_H__ */
> -- 
> 1.9.1
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH v5 3/5] thermal: Add devfreq cooling

[Javi Merino]

On Wed, Sep 09, 2015 at 06:10:22AM +0100, Eduardo Valentin wrote:
> Hi

Hi Eduardo,

> On Thu, Aug 27, 2015 at 11:55:49AM +0100, Javi Merino wrote:
> > From: Ørjan Eide <orjan.eide@arm.com>
> > 
> > Add a generic thermal cooling device for devfreq, that is similar to
> > cpu_cooling.
> > 
> > The device must use devfreq.  In order to use the power extension of the
> > cooling device, it must have registered its OPPs using the OPP library.
> > 
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > Cc: Eduardo Valentin <edubezval@gmail.com>
> > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > Signed-off-by: Ørjan Eide <orjan.eide@arm.com>
> 
> Thanks for taking this to upstream kernel.
> 
> Just minor comments as follows.
> 
> > ---
> > 
> > I had a look at 02373d7c69b4 ("thermal: cpu_cooling: fix lockdep
> > problems in cpu_cooling").  It doesn't affect devfreq cooling because
> > we don't have notifiers, we only use locking for idr.
> 
> Thanks once again for checking it.
> 
> >  drivers/thermal/Kconfig           |  11 +
> >  drivers/thermal/Makefile          |   3 +
> >  drivers/thermal/devfreq_cooling.c | 546 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/devfreq_cooling.h   |  72 +++++
> >  4 files changed, 632 insertions(+)
> >  create mode 100644 drivers/thermal/devfreq_cooling.c
> >  create mode 100644 include/linux/devfreq_cooling.h
> > 
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index 118938ee8552..a2c6a6497804 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -147,6 +147,17 @@ config CLOCK_THERMAL
> >  	  device that is configured to use this cooling mechanism will be
> >  	  controlled to reduce clock frequency whenever temperature is high.
> >  
> > +config DEVFREQ_THERMAL
> > +	bool "Generic device cooling support"
> > +	depends on PM_DEVFREQ
> > +	depends on PM_OPP
> > +	help
> > +	  This implements the generic devfreq cooling mechanism through
> > +	  frequency reduction for devices using devfreq.
> > +
> > +	  This will throttle the device by limiting the maximum allowed DVFS
> > +	  frequency corresponding to the cooling level.
> > +
> >  	  If you want this support, you should say Y here.
> >  
> >  config THERMAL_EMULATION
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > index 535dfee1496f..45f26978ff74 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -22,6 +22,9 @@ thermal_sys-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
> >  # clock cooling
> >  thermal_sys-$(CONFIG_CLOCK_THERMAL)	+= clock_cooling.o
> >  
> > +# devfreq cooling
> > +thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
> > +
> >  # platform thermal drivers
> >  obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
> >  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
> > diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
> > new file mode 100644
> > index 000000000000..3d4abc746099
> > --- /dev/null
> > +++ b/drivers/thermal/devfreq_cooling.c
> > @@ -0,0 +1,546 @@
> > +/*
> > + * devfreq_cooling: Thermal cooling device implementation for devices using
> > + *                  devfreq
> > + *
> > + * Copyright (C) 2014-2015 ARM Limited
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * TODO:
> > + *    - If OPPs are added or removed after devfreq cooling has
> > + *      registered, the devfreq cooling won't react to it.
> > + */
> > +
> > +#include <linux/devfreq.h>
> > +#include <linux/devfreq_cooling.h>
> > +#include <linux/export.h>
> > +#include <linux/slab.h>
> > +#include <linux/pm_opp.h>
> > +#include <linux/thermal.h>
> > +
> > +static DEFINE_MUTEX(devfreq_lock);
> > +static DEFINE_IDR(devfreq_idr);
> > +
> > +/**
> > + * struct devfreq_cooling_device - Devfreq cooling device
> > + * @id:		unique integer value corresponding to each
> > + *		devfreq_cooling_device registered.
> > + * @cdev:	Pointer to associated thermal cooling device.
> > + * @devfreq:	Pointer to associated devfreq device.
> > + * @cooling_state:	Current cooling state.
> > + * @power_table:	Pointer to table with maximum power draw for each
> > + *			cooling state. State is the index into the table, and
> > + *			the power is in mW.
> > + * @freq_table:	Pointer to a table with the frequencies sorted in descending
> > + *		order.  You can index the table by cooling device state
> > + * @freq_table_size:	size of the @freq_table and @power_table
> > + * @power_ops:	Pointer to power operations, used to generate @power_table.
> > + */
> > +struct devfreq_cooling_device {
> > +	int id;
> > +	struct thermal_cooling_device *cdev;
> > +	struct devfreq *devfreq;
> > +	unsigned long cooling_state;
> > +	u32 *power_table;
> > +	u32 *freq_table;
> > +	size_t freq_table_size;
> > +	struct devfreq_cooling_ops *power_ops;
> > +};
> > +
> > +/**
> > + * get_idr - function to get a unique id.
> > + * @idr: struct idr * handle used to create a id.
> > + * @id: int * value generated by this function.
> > + *
> > + * This function will populate @id with an unique
> > + * id, using the idr API.
> > + *
> > + * Return: 0 on success, an error code on failure.
> > + */
> > +static int get_idr(struct idr *idr, int *id)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&devfreq_lock);
> > +	ret = idr_alloc(idr, NULL, 0, 0, GFP_KERNEL);
> > +	mutex_unlock(&devfreq_lock);
> > +	if (unlikely(ret < 0))
> > +		return ret;
> > +	*id = ret;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * release_idr - function to free the unique id.
> > + * @idr: struct idr * handle used for creating the id.
> > + * @id: int value representing the unique id.
> > + */
> > +static void release_idr(struct idr *idr, int id)
> > +{
> > +	mutex_lock(&devfreq_lock);
> > +	idr_remove(idr, id);
> > +	mutex_unlock(&devfreq_lock);
> > +}
> > +
> > +/**
> > + * enable_disable_opps() - disable all opps above a given state
> 
> This function name is confusing. But, at the same time, I cannot think
> of a name for it right now.

Yes, it is meant to say enable/disable opps, but it doesn't work.

> maybe partition_enable_opps()?

Sure, I'll change it.

> > + * @dfc:	Pointer to devfreq we are operating on
> > + * @cdev_state:	cooling device state we're setting
> > + *
> > + * Go through the OPPs of the device, enabling all OPPs until
> > + * @cdev_state and disabling those frequencies above it.
> > + */
> > +static int enable_disable_opps(struct devfreq_cooling_device *dfc,
> > +			       unsigned long cdev_state)
> > +{
> > +	int i;
> > +	struct device *dev = dfc->devfreq->dev.parent;
> > +
> > +	for (i = 0; i < dfc->freq_table_size; i++) {
> > +		struct dev_pm_opp *opp;
> > +		int ret = 0;
> > +		unsigned int freq = dfc->freq_table[i];
> > +		bool want_enable = i >= cdev_state ? true : false;
> > +
> > +		rcu_read_lock();
> > +		opp = dev_pm_opp_find_freq_exact(dev, freq, !want_enable);
> > +		rcu_read_unlock();
> > +
> > +		if (PTR_ERR(opp) == -ERANGE)
> > +			continue;
> > +		else if (IS_ERR(opp))
> > +			return PTR_ERR(opp);
> > +
> > +		if (want_enable)
> > +			ret = dev_pm_opp_enable(dev, freq);
> > +		else
> > +			ret = dev_pm_opp_disable(dev, freq);
> > +
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int devfreq_cooling_get_max_state(struct thermal_cooling_device *cdev,
> > +					 unsigned long *state)
> > +{
> > +	struct devfreq_cooling_device *dfc = cdev->devdata;
> > +
> > +	*state = dfc->freq_table_size - 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static int devfreq_cooling_get_cur_state(struct thermal_cooling_device *cdev,
> > +					 unsigned long *state)
> > +{
> > +	struct devfreq_cooling_device *dfc = cdev->devdata;
> > +
> > +	*state = dfc->cooling_state;
> > +
> > +	return 0;
> > +}
> > +
> > +static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
> > +					 unsigned long state)
> > +{
> > +	struct devfreq_cooling_device *dfc = cdev->devdata;
> > +	struct devfreq *df = dfc->devfreq;
> > +	struct device *dev = df->dev.parent;
> > +	int ret;
> > +
> > +	if (state == dfc->cooling_state)
> > +		return 0;
> > +
> > +	dev_dbg(dev, "Setting cooling state %lu\n", state);
> > +
> > +	if (state >= dfc->freq_table_size)
> > +		return -EINVAL;
> > +
> > +	ret = enable_disable_opps(dfc, state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dfc->cooling_state = state;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * freq_get_state() - get the cooling state corresponding to a frequency
> > + * @dfc:	Pointer to devfreq cooling device
> > + * @freq:	frequency in Hz
> > + *
> > + * Return: the cooling state associated with the @freq, or
> > + * THERMAL_CSTATE_INVALID if it wasn't found.
> > + */
> > +static unsigned long
> > +freq_get_state(struct devfreq_cooling_device *dfc, unsigned long freq)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < dfc->freq_table_size; i++) {
> > +		if (dfc->freq_table[i] == freq)
> > +			return i;
> > +	}
> > +
> > +	return THERMAL_CSTATE_INVALID;
> > +}
> > +
> > +/**
> > + * state_get_freq() - get the frequency corresponding to a cooling state
> > + * @dfc:	Pointer to devfreq cooling device
> > + * @state:	cooling device state
> > + *
> > + * Return: the frequency for this cooling device state or 0 for
> > + * invalid states.
> > + */
> > +static unsigned long
> > +state_get_freq(struct devfreq_cooling_device *dfc, unsigned long state)
> > +{
> > +	if (state >= dfc->freq_table_size) {
> > +		dev_warn(dfc->devfreq->dev.parent,
> > +			 "State %lu bigger than frequency table\n", state);
> > +		return 0;
> > +	}
> > +
> > +	return dfc->freq_table[state];
> > +}
> > +
> > +static unsigned long
> > +get_static_power(struct devfreq_cooling_device *dfc, unsigned long freq)
> > +{
> > +	struct devfreq *df = dfc->devfreq;
> > +	struct device *dev = df->dev.parent;
> > +	unsigned long voltage;
> > +	struct dev_pm_opp *opp;
> > +
> > +	rcu_read_lock();
> > +
> > +	opp = dev_pm_opp_find_freq_exact(dev, freq, true);
> > +	if (IS_ERR(opp) && (PTR_ERR(opp) == -ERANGE))
> > +		opp = dev_pm_opp_find_freq_exact(dev, freq, false);
> > +
> > +	voltage = dev_pm_opp_get_voltage(opp) / 1000; /* mV */
> > +
> > +	rcu_read_unlock();
> > +
> > +	if (voltage == 0) {
> > +		dev_warn_ratelimited(dev,
> > +				     "Failed to get voltage for frequency %lu: %ld\n",
> > +				     freq, IS_ERR(opp) ? PTR_ERR(opp) : 0);
> > +		return 0;
> > +	}
> > +
> > +	return dfc->power_ops->get_static_power(voltage);
> > +}
> > +
> > +static int devfreq_cooling_get_requested_power(struct thermal_cooling_device *cdev,
> > +					       struct thermal_zone_device *tz,
> > +					       u32 *power)
> > +{
> > +	struct devfreq_cooling_device *dfc = cdev->devdata;
> > +	struct devfreq *df = dfc->devfreq;
> > +	struct devfreq_dev_status *status = &df->last_status;
> > +	unsigned long state;
> > +	unsigned long freq = status->current_frequency;
> > +	u32 dyn_power, static_power;
> > +
> > +	/* Get dynamic power for state */
> > +	state = freq_get_state(dfc, freq);
> > +	if (state == THERMAL_CSTATE_INVALID)
> > +		return -EAGAIN;
> > +
> > +	dyn_power = dfc->power_table[state];
> > +
> > +	/* Scale dynamic power for utilization */
> > +	dyn_power = (dyn_power * status->busy_time) / status->total_time;
> > +
> > +	/* Get static power */
> > +	static_power = get_static_power(dfc, freq);
> > +
> > +	*power = dyn_power + static_power;
> > +
> > +	return 0;
> > +}
> > +
> > +static int devfreq_cooling_state2power(struct thermal_cooling_device *cdev,
> > +				       struct thermal_zone_device *tz,
> > +				       unsigned long state,
> > +				       u32 *power)
> > +{
> > +	struct devfreq_cooling_device *dfc = cdev->devdata;
> > +	unsigned long freq;
> > +	u32 static_power;
> > +
> > +	freq = state_get_freq(dfc, state);
> > +	static_power = get_static_power(dfc, freq);
> > +
> > +	*power = dfc->power_table[state] + static_power;
> 
> should we have a boundaries check here on state?

Definitely, I'll add them.

> > +	return 0;
> > +}
> > +
> > +static int devfreq_cooling_power2state(struct thermal_cooling_device *cdev,
> > +				       struct thermal_zone_device *tz,
> > +				       u32 power, unsigned long *state)
> > +{
> > +	struct devfreq_cooling_device *dfc = cdev->devdata;
> > +	struct devfreq *df = dfc->devfreq;
> > +	struct devfreq_dev_status *status = &df->last_status;
> > +	unsigned long freq = status->current_frequency;
> > +	unsigned long busy_time;
> > +	s32 dyn_power;
> > +	u32 static_power;
> > +	int i;
> > +
> > +	static_power = get_static_power(dfc, freq);
> > +
> > +	dyn_power = power - static_power;
> > +	dyn_power = dyn_power > 0 ? dyn_power : 0;
> > +
> > +	/* Scale dynamic power for utilization */
> > +	busy_time = status->busy_time ?: 1;
> > +	dyn_power = (dyn_power * status->total_time) / busy_time;
> > +
> > +	/*
> > +	 * Find the first cooling state that is within the power
> > +	 * budget for dynamic power.
> > +	 */
> > +	for (i = 0; i < dfc->freq_table_size - 1; i++)
> > +		if (dyn_power >= dfc->power_table[i])
> > +			break;
> > +
> > +	*state = i;
> > +	return 0;
> > +}
> > +
> > +static struct thermal_cooling_device_ops devfreq_cooling_ops = {
> > +	.get_max_state = devfreq_cooling_get_max_state,
> > +	.get_cur_state = devfreq_cooling_get_cur_state,
> > +	.set_cur_state = devfreq_cooling_set_cur_state,
> > +};
> > +
> > +/**
> > + * devfreq_cooling_gen_tables() - Generate power and freq tables.
> > + * @dfc: Pointer to devfreq cooling device.
> > + *
> > + * Generate power and frequency tables: the power table hold the
> > + * device's maximum power usage at each cooling state (OPP).  The
> > + * static and dynamic power using the appropriate voltage and
> > + * frequency for the state, is acquired from the struct
> > + * devfreq_cooling_ops, and summed to make the maximum power draw.
> > + *
> > + * The frequency table holds the frequencies in descending order.
> > + * That way its indexed by cooling device state.
> > + *
> > + * The tables are malloced, and pointers put in dfc.  They must be
> > + * freed when unregistering the devfreq cooling device.
> > + *
> > + * Return: 0 on success, negative error code on failure.
> > + */
> > +static int devfreq_cooling_gen_tables(struct devfreq_cooling_device *dfc)
> > +{
> > +	struct devfreq *df = dfc->devfreq;
> > +	struct device *dev = df->dev.parent;
> > +	int ret, num_opps;
> > +	struct devfreq_cooling_ops *callbacks = dfc->power_ops;
> > +	unsigned long freq;
> > +	u32 *power_table = NULL;
> > +	u32 *freq_table;
> > +	int i;
> > +
> > +	num_opps = dev_pm_opp_get_opp_count(dev);
> > +
> > +	if (callbacks) {
> > +		power_table = kcalloc(num_opps, sizeof(*power_table),
> > +				      GFP_KERNEL);
> > +		if (!power_table)
> > +			ret = -ENOMEM;
> > +	}
> > +
> > +	freq_table = kcalloc(num_opps, sizeof(*freq_table),
> > +			     GFP_KERNEL);
> > +	if (!freq_table) {
> > +		ret = -ENOMEM;
> > +		goto free_power_table;
> > +	}
> > +
> > +	for (i = 0, freq = ULONG_MAX; i < num_opps; i++, freq--) {
> > +		unsigned long power_dyn, voltage;
> > +		struct dev_pm_opp *opp;
> > +
> > +		rcu_read_lock();
> > +
> > +		opp = dev_pm_opp_find_freq_floor(dev, &freq);
> > +		if (IS_ERR(opp)) {
> > +			rcu_read_unlock();
> > +			ret = PTR_ERR(opp);
> > +			goto free_tables;
> > +		}
> > +
> > +		voltage = dev_pm_opp_get_voltage(opp) / 1000; /* mV */
> > +
> > +		rcu_read_unlock();
> > +
> > +		if (callbacks) {
> > +			power_dyn = callbacks->get_dynamic_power(freq, voltage);
> > +
> > +			dev_info(dev, "Dynamic power table: %lu MHz @ %lu mV: %lu = %lu mW\n",
> 
> I would prefer this would not be dev_info though..

dev_dbg() then?

> > +				 freq / 1000000, voltage, power_dyn, power_dyn);
> > +
> > +			power_table[i] = power_dyn;
> > +		}
> > +
> > +		freq_table[i] = freq;
> > +	}
> > +
> > +	if (callbacks)
> > +		dfc->power_table = power_table;
> > +
> > +	dfc->freq_table = freq_table;
> > +	dfc->freq_table_size = num_opps;
> > +
> > +	return 0;
> > +
> > +free_tables:
> > +	kfree(freq_table);
> > +free_power_table:
> > +	kfree(power_table);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * of_devfreq_cooling_register_power() - Register devfreq cooling device,
> > + *                                      with OF and power information.
> > + * @np: Pointer to OF device_node.
> > + * @df: Pointer to devfreq device.
> > + * @ops: Pointer to power ops.
> > + *
> > + * Register a devfreq cooling device.  The available OPPs must be
> > + * registered on the device.
> > + *
> > + * If @ops is provided, the cooling device is registered with the
> > + * power extensions.  For the power extensions to work correctly,
> > + * devfreq should use the simple_ondemand governor, other governors
> > + * are not currently supported.
> 
> Fine, but this should not be cooling device concern.
> 
> > + */
> > +struct devfreq_cooling_device *
> > +of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
> > +				  struct devfreq_cooling_ops *ops)
> > +{
> > +	struct thermal_cooling_device *cdev;
> > +	struct devfreq_cooling_device *dfc;
> > +	char dev_name[THERMAL_NAME_LENGTH];
> > +	int err;
> > +
> > +	dfc = kzalloc(sizeof(*dfc), GFP_KERNEL);
> > +	if (!dfc)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	dfc->devfreq = df;
> > +
> > +	if (ops) {
> > +		if (strncmp(df->governor_name, "simple_ondemand",
> > +			    sizeof("simple_ondemand")))
> > +			dev_warn(df->dev.parent,
> > +				 "Warning: Registering devfreq cooling device with power extensions but devfreq governor is not simple_ondemand\n");
> 
> I don't think this check should actually be in the devfreq cooling
> device. I would prefer cooling devices still just provide the
> functionality for the governor do what they need to. This is a policy
> check, which I don't think should be done here. What if we add another
> governor with 'power' extensions? should we come back here and remove
> the warn? or add the governor to the check list?

Ok, I'll drop the check here.  We will document it in the
kerneldoc of the function and in the Kconfig option.

> > +
> > +		if (!ops->get_static_power || !ops->get_dynamic_power) {
> 
> What is the reason we cannot use P=alpha \times f \times V^2 as a
> default?

Yes, we should provide the simple power model as a default if no
get_dynamic_power() is supplied.  I'll add a dynamic_power_coefficient
and use that simple model if get_dynamic_power() is NULL.

> > +			err = -EINVAL;
> > +			goto free_dfc;
> > +		}
> > +		dfc->power_ops = ops;
> > +
> > +		devfreq_cooling_ops.get_requested_power =
> > +			devfreq_cooling_get_requested_power;
> > +		devfreq_cooling_ops.state2power = devfreq_cooling_state2power;
> > +		devfreq_cooling_ops.power2state = devfreq_cooling_power2state;
> > +	}
> > +
> > +	err = devfreq_cooling_gen_tables(dfc);
> > +	if (err)
> > +		goto free_dfc;
> > +
> > +	err = get_idr(&devfreq_idr, &dfc->id);
> > +	if (err)
> > +		goto free_tables;
> > +
> > +	snprintf(dev_name, sizeof(dev_name), "devfreq-%d", dfc->id);
> 
> the name does not seam to mention anything about thermal or cooling, any
> particular reason?

No reason.  I'll call it devfreq-cooling-%d

> > +
> > +	cdev = thermal_of_cooling_device_register(np, dev_name, dfc,
> > +						  &devfreq_cooling_ops);
> > +	if (IS_ERR(cdev)) {
> > +		err = PTR_ERR(cdev);
> > +		dev_err(df->dev.parent,
> > +			"Failed to register devfreq cooling device (%d)\n",
> > +			err);
> > +		goto release_idr;
> > +	}
> > +
> > +	dfc->cdev = cdev;
> > +
> > +	return dfc;
> > +
> > +release_idr:
> > +	release_idr(&devfreq_idr, dfc->id);
> > +free_tables:
> > +	kfree(dfc->power_table);
> > +	kfree(dfc->freq_table);
> > +free_dfc:
> > +	kfree(dfc);
> > +
> > +	return ERR_PTR(err);
> > +}
> > +EXPORT_SYMBOL(of_devfreq_cooling_register_power);
> > +
> > +/**
> > + * of_devfreq_cooling_register() - Register devfreq cooling device,
> > + *                                with OF information.
> > + * @np: Pointer to OF device_node.
> > + * @df: Pointer to devfreq device.
> > + */
> > +struct devfreq_cooling_device *
> > +of_devfreq_cooling_register(struct device_node *np, struct devfreq *df)
> > +{
> > +	return of_devfreq_cooling_register_power(np, df, NULL);
> > +}
> > +EXPORT_SYMBOL(of_devfreq_cooling_register);
> > +
> > +/**
> > + * devfreq_cooling_register() - Register devfreq cooling device.
> > + * @df: Pointer to devfreq device.
> > + */
> > +struct devfreq_cooling_device *devfreq_cooling_register(struct devfreq *df)
> > +{
> > +	return of_devfreq_cooling_register(NULL, df);
> > +}
> > +EXPORT_SYMBOL(devfreq_cooling_register);
> > +
> > +/**
> > + * devfreq_cooling_unregister() - Unregister devfreq cooling device.
> > + * @dfc: Pointer to devfreq cooling device to unregister.
> > + */
> > +void devfreq_cooling_unregister(struct devfreq_cooling_device *dfc)
> > +{
> > +	if (!dfc)
> > +		return;
> > +
> > +	thermal_cooling_device_unregister(dfc->cdev);
> > +	release_idr(&devfreq_idr, dfc->id);
> > +	kfree(dfc->power_table);
> > +	kfree(dfc->freq_table);
> > +
> > +	kfree(dfc);
> > +}
> > +EXPORT_SYMBOL(devfreq_cooling_unregister);
> 
> Any particular reason why this cannot be EXPORT_SYMBOL_GPL (same for all
> exports above)?
> 
> > diff --git a/include/linux/devfreq_cooling.h b/include/linux/devfreq_cooling.h
> > new file mode 100644
> > index 000000000000..de866200efe0
> > --- /dev/null
> > +++ b/include/linux/devfreq_cooling.h
> > @@ -0,0 +1,72 @@
> > +/*
> > + * devfreq_cooling: Thermal cooling device implementation for devices using
> > + *                  devfreq
> > + *
> > + * Copyright (C) 2014-2015 ARM Limited
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#ifndef __DEVFREQ_COOLING_H__
> > +#define __DEVFREQ_COOLING_H__
> > +
> > +#include <linux/devfreq.h>
> > +#include <linux/thermal.h>
> > +
> > +#ifdef CONFIG_DEVFREQ_THERMAL
> > +
> > +/**
> > + * struct devfreq_cooling_ops - Devfreq cooling power ops
> > + * @get_static_power: Take voltage, in mV, and return the static power in mW.
> > + * @get_dynamic_power: Take voltage, in mV, and frequency, in HZ, and return
> > + * the dynamic power draw in mW.
> 
> If you are not providing a default, you must mention that all fields are
> required.

Ok, I'll clarify which fields are required.  As discussed above, the
get_dynamic_power() can be optional if we have a
dynamic_power_coefficient .  If no get_dynamic_power() is supplied,
the default dynamic power model will be used.

Cheers,
Javi

[PATCH v5 4/5] devfreq_cooling: add trace information

[Javi Merino]

Tracing is useful for debugging and performance tuning.  Add similar
traces to what's present in the cpu cooling device.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
 drivers/thermal/devfreq_cooling.c |  6 +++++
 include/trace/events/thermal.h    | 53 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
index 3d4abc746099..14498d61969e 100644
--- a/drivers/thermal/devfreq_cooling.c
+++ b/drivers/thermal/devfreq_cooling.c
@@ -25,6 +25,8 @@
 #include <linux/pm_opp.h>
 #include <linux/thermal.h>
 
+#include <trace/events/thermal.h>
+
 static DEFINE_MUTEX(devfreq_lock);
 static DEFINE_IDR(devfreq_idr);
 
@@ -269,6 +271,9 @@ static int devfreq_cooling_get_requested_power(struct thermal_cooling_device *cd
 	/* Get static power */
 	static_power = get_static_power(dfc, freq);
 
+	trace_thermal_power_devfreq_get_power(cdev, status, freq, dyn_power,
+					      static_power);
+
 	*power = dyn_power + static_power;
 
 	return 0;
@@ -321,6 +326,7 @@ static int devfreq_cooling_power2state(struct thermal_cooling_device *cdev,
 			break;
 
 	*state = i;
+	trace_thermal_power_devfreq_limit(cdev, freq, *state, power);
 	return 0;
 }
 
diff --git a/include/trace/events/thermal.h b/include/trace/events/thermal.h
index 8b1f80682b80..5738bb3e2343 100644
--- a/include/trace/events/thermal.h
+++ b/include/trace/events/thermal.h
@@ -4,6 +4,7 @@
 #if !defined(_TRACE_THERMAL_H) || defined(TRACE_HEADER_MULTI_READ)
 #define _TRACE_THERMAL_H
 
+#include <linux/devfreq.h>
 #include <linux/thermal.h>
 #include <linux/tracepoint.h>
 
@@ -135,6 +136,58 @@ TRACE_EVENT(thermal_power_cpu_limit,
 		__entry->power)
 );
 
+TRACE_EVENT(thermal_power_devfreq_get_power,
+	TP_PROTO(struct thermal_cooling_device *cdev,
+		 struct devfreq_dev_status *status, unsigned long freq,
+		u32 dynamic_power, u32 static_power),
+
+	TP_ARGS(cdev, status,  freq, dynamic_power, static_power),
+
+	TP_STRUCT__entry(
+		__string(type,         cdev->type    )
+		__field(unsigned long, freq          )
+		__field(u32,           load          )
+		__field(u32,           dynamic_power )
+		__field(u32,           static_power  )
+	),
+
+	TP_fast_assign(
+		__assign_str(type, cdev->type);
+		__entry->freq = freq;
+		__entry->load = (100 * status->busy_time) / status->total_time;
+		__entry->dynamic_power = dynamic_power;
+		__entry->static_power = static_power;
+	),
+
+	TP_printk("type=%s freq=%lu load=%u dynamic_power=%u static_power=%u",
+		__get_str(type), __entry->freq,
+		__entry->load, __entry->dynamic_power, __entry->static_power)
+);
+
+TRACE_EVENT(thermal_power_devfreq_limit,
+	TP_PROTO(struct thermal_cooling_device *cdev, unsigned long freq,
+		unsigned long cdev_state, u32 power),
+
+	TP_ARGS(cdev, freq, cdev_state, power),
+
+	TP_STRUCT__entry(
+		__string(type,         cdev->type)
+		__field(unsigned int,  freq      )
+		__field(unsigned long, cdev_state)
+		__field(u32,           power     )
+	),
+
+	TP_fast_assign(
+		__assign_str(type, cdev->type);
+		__entry->freq = freq;
+		__entry->cdev_state = cdev_state;
+		__entry->power = power;
+	),
+
+	TP_printk("type=%s freq=%u cdev_state=%lu power=%u",
+		__get_str(type), __entry->freq, __entry->cdev_state,
+		__entry->power)
+);
 #endif /* _TRACE_THERMAL_H */
 
 /* This part must be outside protection */
-- 
1.9.1

[PATCH v5 5/5] PM / devfreq: drop comment about thermal setting max_freq

[Javi Merino]

The thermal infrastructure should use the devfreq cooling device, which
uses the OPP library to disable OPPs as necessary.

Fix a couple of typos in the same comment while we are at it.

Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
 drivers/devfreq/devfreq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index ca1b362d77e2..aed1137b2173 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -177,10 +177,10 @@ int update_devfreq(struct devfreq *devfreq)
 		return err;
 
 	/*
-	 * Adjust the freuqency with user freq and QoS.
+	 * Adjust the frequency with user freq and QoS.
 	 *
-	 * List from the highest proiority
-	 * max_freq (probably called by thermal when it's too hot)
+	 * List from the highest priority
+	 * max_freq
 	 * min_freq
 	 */
 
-- 
1.9.1