[PATCH 00/17] PM / OPP: Introduce APIs to transition OPPs

[PATCH 00/17] PM / OPP: Introduce APIs to transition OPPs

[Viresh Kumar]

This patchset add APIs in OPP layer to allow OPPs transitioning from
within OPP layer. Currently all OPP users need to replicate the same
code to switch between OPPs. While the same can be handled easily by
OPP-core.

The first Eight patches update the OPP core to introduce the new APIs
and the next Nine patches update cpufreq-dt for the same.

Testing:
- Tested on exynos 5250-arndale (dual-cortex-A15)
- Tested for both Old-V1 bindings and New V2 bindings
- Tested with regulator names as: 'cpu-supply' and 'cpu0-supply'
- Tested with Unsupported supply ranges as well, to check the
  opp-disable logic

Viresh Kumar (17):
  PM / OPP: get/put regulators from OPP core
  PM / OPP: Add APIs to set regulator-name
  PM / OPP: Disable OPPs that aren't supported by the regulator
  PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()
  PM / OPP: Introduce dev_pm_opp_get_max_transition_latency()
  PM / OPP: Parse clock-latency and voltage-tolerance for v1 bindings
  PM / OPP: Manage device clk
  PM / OPP: Add dev_pm_opp_set_rate()
  cpufreq: dt: Convert few pr_XXX() calls to dev_XXX()
  cpufreq: dt: Rename 'need_update' to 'opp_v1'
  cpufreq: dt: OPP layers handles clock-latency for V1 bindings as well
  cpufreq: dt: Pass regulator name to the OPP core for V1 bindings
  cpufreq: dt: Unsupported OPPs are already disabled
  cpufreq: dt: Reuse dev_pm_opp_get_max_transition_latency()
  cpufreq: dt: Use dev_pm_opp_set_rate() to switch frequency
  cpufreq: dt: drop references to DT node
  cpufreq: dt: No need to allocate resources anymore

 drivers/base/power/opp/core.c | 428 ++++++++++++++++++++++++++++++++++++++++--
 drivers/base/power/opp/opp.h  |  15 ++
 drivers/cpufreq/cpufreq-dt.c  | 254 ++++++++-----------------
 include/linux/pm_opp.h        |  27 +++
 4 files changed, 530 insertions(+), 194 deletions(-)

-- 
2.7.0.rc1.186.g94414c4

[PATCH 01/17] PM / OPP: get/put regulators from OPP core

[Viresh Kumar]

The allows the OPP core to request/free the regulator resource, attached
to a device OPP. The regulator device is fetched using device node and
name of the device.

For example, a cpu0 device node needs to look like this:
		cpu0: cpu@900 {
			device_type = "cpu";
			compatible = "arm,cortex-a9";
			reg = <0x900>;
			cpu0-supply = <&varm>;
			...
		};

This will work for both OPP-v1 and v2 bindings.

This is a preliminary step in moving the OPP switching logic into the
OPP core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 8 ++++++++
 drivers/base/power/opp/opp.h  | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index cd230c63aee6..2e49f5e9daa3 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -505,6 +505,7 @@ static struct device_opp *_add_device_opp(struct device *dev)
 {
 	struct device_opp *dev_opp;
 	struct device_list_opp *list_dev;
+	const char *name = dev_name(dev);
 
 	/* Check for existing list for 'dev' first */
 	dev_opp = _find_device_opp(dev);
@@ -527,6 +528,11 @@ static struct device_opp *_add_device_opp(struct device *dev)
 		return NULL;
 	}
 
+	dev_opp->regulator = regulator_get_optional(dev, name);
+	if (IS_ERR(dev_opp->regulator))
+		dev_info(dev, "%s: no regulator (%s) found: %ld\n", __func__,
+			 name, PTR_ERR(dev_opp->regulator));
+
 	srcu_init_notifier_head(&dev_opp->srcu_head);
 	INIT_LIST_HEAD(&dev_opp->opp_list);
 
@@ -565,6 +571,8 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (dev_opp->prop_name)
 		return;
 
+	regulator_put(dev_opp->regulator);
+
 	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
 				    node);
 
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 690638ef36ee..c4a03ad34100 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -18,6 +18,7 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/limits.h>
+#include <linux/regulator/consumer.h>
 #include <linux/pm_opp.h>
 #include <linux/rculist.h>
 #include <linux/rcupdate.h>
@@ -132,6 +133,8 @@ struct device_list_opp {
  * @supported_hw: Array of version number to support.
  * @supported_hw_count: Number of elements in supported_hw array.
  * @prop_name: A name to postfix to many DT properties, while parsing them.
+ * @regulator: Supply Regulator
+ *
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
@@ -159,6 +162,7 @@ struct device_opp {
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
 	const char *prop_name;
+	struct regulator *regulator;
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
-- 
2.7.0.rc1.186.g94414c4

[PATCH 02/17] PM / OPP: Add APIs to set regulator-name

[Viresh Kumar]

This is required mostly for backward compatibility of DT users. The OPP
layer automatically gets the regulator device currently, but the name of
the device needs to be same as that of the device. But existing DT
entries may not be following that and might have used generic names
instead. For example, they might have used 'cpu-supply' instead of
'cpu0-supply'.

The APIs allow such platforms to pass a supply 'name' to OPP core, so
that the correct regulator supply can be allocated by the OPP core.

This must be called before any OPPs are initialized for the device.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 163 +++++++++++++++++++++++++++++++++++++-----
 drivers/base/power/opp/opp.h  |   2 +
 include/linux/pm_opp.h        |   9 +++
 3 files changed, 158 insertions(+), 16 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 2e49f5e9daa3..76232ee04cc6 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -492,25 +492,11 @@ struct device_list_opp *_add_list_dev(const struct device *dev,
 	return list_dev;
 }
 
-/**
- * _add_device_opp() - Find device OPP table or allocate a new one
- * @dev:	device for which we do this operation
- *
- * It tries to find an existing table first, if it couldn't find one, it
- * allocates a new OPP table and returns that.
- *
- * Return: valid device_opp pointer if success, else NULL.
- */
-static struct device_opp *_add_device_opp(struct device *dev)
+static struct device_opp *_add_device_opp_reg(struct device *dev,
+					      const char *name)
 {
 	struct device_opp *dev_opp;
 	struct device_list_opp *list_dev;
-	const char *name = dev_name(dev);
-
-	/* Check for existing list for 'dev' first */
-	dev_opp = _find_device_opp(dev);
-	if (!IS_ERR(dev_opp))
-		return dev_opp;
 
 	/*
 	 * Allocate a new device OPP table. In the infrequent case where a new
@@ -542,6 +528,27 @@ static struct device_opp *_add_device_opp(struct device *dev)
 }
 
 /**
+ * _add_device_opp() - Find device OPP table or allocate a new one
+ * @dev:	device for which we do this operation
+ *
+ * It tries to find an existing table first, if it couldn't find one, it
+ * allocates a new OPP table and returns that.
+ *
+ * Return: valid device_opp pointer if success, else NULL.
+ */
+static struct device_opp *_add_device_opp(struct device *dev)
+{
+	struct device_opp *dev_opp;
+
+	/* Check for existing list for 'dev' first */
+	dev_opp = _find_device_opp(dev);
+	if (!IS_ERR(dev_opp))
+		return dev_opp;
+
+	return _add_device_opp_reg(dev, dev_name(dev));
+}
+
+/**
  * _kfree_device_rcu() - Free device_opp RCU handler
  * @head:	RCU head
  */
@@ -571,6 +578,9 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (dev_opp->prop_name)
 		return;
 
+	if (dev_opp->regulator_set)
+		return;
+
 	regulator_put(dev_opp->regulator);
 
 	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
@@ -1091,6 +1101,127 @@ void dev_pm_opp_put_prop_name(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
 
+/**
+ * dev_pm_opp_set_regulator() - Set regulator name for the device
+ * @dev: Device for which regulator name is being set.
+ * @name: Name of the regulator.
+ *
+ * This is required mostly for backward compatibility of DT users. The OPP layer
+ * automatically gets the regulator device currently, but the name of the device
+ * needs to be same as that of the device. But existing DT entries may not be
+ * following that and might have used generic names instead. For example, they
+ * might have used 'cpu-supply' instead of 'cpu0-supply'.
+ *
+ * This must be called before any OPPs are initialized for the device.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ */
+int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+{
+	struct device_opp *dev_opp;
+	struct regulator *reg;
+	int ret = 0;
+
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
+	dev_opp = _find_device_opp(dev);
+	/* We already have a dev-opp */
+	if (!IS_ERR(dev_opp)) {
+		/* This should be called before OPPs are initialized */
+		if (WARN_ON(!list_empty(&dev_opp->opp_list))) {
+			ret = -EBUSY;
+			goto unlock;
+		}
+
+		/* Already have a regulator set? Free it and re-allocate */
+		if (!IS_ERR(dev_opp->regulator))
+			regulator_put(dev_opp->regulator);
+
+		/* Allocate the regulator */
+		reg = regulator_get_optional(dev, name);
+		if (IS_ERR(reg)) {
+			ret = PTR_ERR(reg);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "%s: no regulator (%s) found: %d\n",
+					__func__, name, ret);
+		}
+
+		dev_opp->regulator = reg;
+		goto unlock;
+	}
+
+	dev_opp = _add_device_opp_reg(dev, name);
+	if (!dev_opp) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	reg = dev_opp->regulator;
+	if (IS_ERR(reg)) {
+		ret = PTR_ERR(reg);
+		_remove_device_opp(dev_opp);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "%s: no regulator (%s) found: %d\n",
+				__func__, name, ret);
+	}
+
+unlock:
+	if (!ret)
+		dev_opp->regulator_set = true;
+
+	mutex_unlock(&dev_opp_list_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
+
+/**
+ * dev_pm_opp_put_regulator() - Releases resources blocked for regulator
+ * @dev: Device for which regulator was set.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ */
+void dev_pm_opp_put_regulator(struct device *dev)
+{
+	struct device_opp *dev_opp;
+
+	/* Hold our list modification lock here */
+	mutex_lock(&dev_opp_list_lock);
+
+	/* Check for existing list for 'dev' first */
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp)) {
+		dev_err(dev, "Failed to find dev_opp: %ld\n", PTR_ERR(dev_opp));
+		goto unlock;
+	}
+
+	/* Make sure there are no concurrent readers while updating dev_opp */
+	WARN_ON(!list_empty(&dev_opp->opp_list));
+
+	if (!dev_opp->regulator_set) {
+		dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
+		goto unlock;
+	}
+
+	dev_opp->regulator_set = false;
+
+	/* Try freeing device_opp if this was the last blocking resource */
+	_remove_device_opp(dev_opp);
+
+unlock:
+	mutex_unlock(&dev_opp_list_lock);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulator);
+
 static bool _opp_is_supported(struct device *dev, struct device_opp *dev_opp,
 			      struct device_node *np)
 {
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index c4a03ad34100..08aae35ab8e0 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -134,6 +134,7 @@ struct device_list_opp {
  * @supported_hw_count: Number of elements in supported_hw array.
  * @prop_name: A name to postfix to many DT properties, while parsing them.
  * @regulator: Supply Regulator
+ * @regulator_set: Regulator's name is explicitly set by platform.
  *
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
@@ -163,6 +164,7 @@ struct device_opp {
 	unsigned int supported_hw_count;
 	const char *prop_name;
 	struct regulator *regulator;
+	bool regulator_set;
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 95403d2ccaf5..c70a18ac9c8a 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -60,6 +60,8 @@ int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
 void dev_pm_opp_put_supported_hw(struct device *dev);
 int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
 void dev_pm_opp_put_prop_name(struct device *dev);
+int dev_pm_opp_set_regulator(struct device *dev, const char *name);
+void dev_pm_opp_put_regulator(struct device *dev);
 #else
 static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 {
@@ -151,6 +153,13 @@ static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
 
 static inline void dev_pm_opp_put_prop_name(struct device *dev) {}
 
+static inline int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+{
+	return -EINVAL;
+}
+
+static inline void dev_pm_opp_put_regulator(struct device *dev) {}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
-- 
2.7.0.rc1.186.g94414c4

[PATCH 03/17] PM / OPP: Disable OPPs that aren't supported by the regulator

[Viresh Kumar]

Disable any OPPs where the connected regulator isn't able to provide the
specified voltage.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 76232ee04cc6..12066d3def38 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -701,6 +701,22 @@ static struct dev_pm_opp *_allocate_opp(struct device *dev,
 	return opp;
 }
 
+static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
+					 struct device_opp *dev_opp)
+{
+	struct regulator *reg = dev_opp->regulator;
+
+	if (!IS_ERR(reg) &&
+	    !regulator_is_supported_voltage(reg, opp->u_volt_min,
+					    opp->u_volt_max)) {
+		pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
+			__func__, opp->u_volt_min, opp->u_volt_max);
+		return false;
+	}
+
+	return true;
+}
+
 static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 		    struct device_opp *dev_opp)
 {
@@ -742,6 +758,12 @@ static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 		dev_err(dev, "%s: Failed to register opp to debugfs (%d)\n",
 			__func__, ret);
 
+	if (!_opp_supported_by_regulators(new_opp, dev_opp)) {
+		new_opp->available = false;
+		dev_warn(dev, "%s: OPP not supported by regulators (%lu)\n",
+			 __func__, new_opp->rate);
+	}
+
 	return 0;
 }
 
-- 
2.7.0.rc1.186.g94414c4

[PATCH 04/17] PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()

[Viresh Kumar]

In few use cases (like: cpufreq), it is desired to get the maximum
voltage latency for changing OPPs. Add support for that.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 49 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h        |  6 ++++++
 2 files changed, 55 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 12066d3def38..bf296c459e06 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -230,6 +230,55 @@ unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev)
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_clock_latency);
 
 /**
+ * dev_pm_opp_get_max_volt_latency() - Get max voltage latency in nanoseconds
+ * @dev:	device for which we do this operation
+ *
+ * Return: This function returns the max voltage latency in nanoseconds.
+ *
+ * Locking: This function takes rcu_read_lock().
+ */
+unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
+{
+	struct device_opp *dev_opp;
+	struct dev_pm_opp *opp;
+	struct regulator *reg;
+	unsigned long latency_ns = 0;
+	unsigned long min_uV = ~0, max_uV = 0;
+	int ret;
+
+	rcu_read_lock();
+
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp))
+		goto unlock;
+
+	reg = dev_opp->regulator;
+	/* Regulator may not be available for device */
+	if (IS_ERR(reg))
+		goto unlock;
+
+	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
+		if (!opp->available)
+			continue;
+
+		if (opp->u_volt_min < min_uV)
+			min_uV = opp->u_volt_min;
+		if (opp->u_volt_max > max_uV)
+			max_uV = opp->u_volt_max;
+	}
+
+	ret = regulator_set_voltage_time(reg, min_uV, max_uV);
+	if (ret > 0)
+		latency_ns += ret * 1000;
+
+unlock:
+	rcu_read_unlock();
+
+	return latency_ns;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_volt_latency);
+
+/**
  * dev_pm_opp_get_suspend_opp() - Get suspend opp
  * @dev:	device for which we do this operation
  *
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index c70a18ac9c8a..5daa43058ac1 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -34,6 +34,7 @@ bool dev_pm_opp_is_turbo(struct dev_pm_opp *opp);
 
 int dev_pm_opp_get_opp_count(struct device *dev);
 unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev);
+unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev);
 struct dev_pm_opp *dev_pm_opp_get_suspend_opp(struct device *dev);
 
 struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
@@ -88,6 +89,11 @@ static inline unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev)
 	return 0;
 }
 
+static inline unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
+{
+	return 0;
+}
+
 static inline struct dev_pm_opp *dev_pm_opp_get_suspend_opp(struct device *dev)
 {
 	return NULL;
-- 
2.7.0.rc1.186.g94414c4

[PATCH 05/17] PM / OPP: Introduce dev_pm_opp_get_max_transition_latency()

[Viresh Kumar]

In few use cases (like: cpufreq), it is desired to get the maximum
latency for changing OPPs. Add support for that.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 17 +++++++++++++++++
 include/linux/pm_opp.h        |  6 ++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index bf296c459e06..5746993aa71d 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -279,6 +279,23 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_volt_latency);
 
 /**
+ * dev_pm_opp_get_max_transition_latency() - Get max transition latency in
+ *					     nanoseconds
+ * @dev:	device for which we do this operation
+ *
+ * Return: This function returns the max transition latency, in nanoseconds, to
+ * switch from one OPP to other.
+ *
+ * Locking: This function takes rcu_read_lock().
+ */
+unsigned long dev_pm_opp_get_max_transition_latency(struct device *dev)
+{
+	return dev_pm_opp_get_max_volt_latency(dev) +
+		dev_pm_opp_get_max_clock_latency(dev);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_transition_latency);
+
+/**
  * dev_pm_opp_get_suspend_opp() - Get suspend opp
  * @dev:	device for which we do this operation
  *
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 5daa43058ac1..59da3d9e11ea 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -35,6 +35,7 @@ bool dev_pm_opp_is_turbo(struct dev_pm_opp *opp);
 int dev_pm_opp_get_opp_count(struct device *dev);
 unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev);
 unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev);
+unsigned long dev_pm_opp_get_max_transition_latency(struct device *dev);
 struct dev_pm_opp *dev_pm_opp_get_suspend_opp(struct device *dev);
 
 struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
@@ -94,6 +95,11 @@ static inline unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
 	return 0;
 }
 
+static inline unsigned long dev_pm_opp_get_max_transition_latency(struct device *dev)
+{
+	return 0;
+}
+
 static inline struct dev_pm_opp *dev_pm_opp_get_suspend_opp(struct device *dev)
 {
 	return NULL;
-- 
2.7.0.rc1.186.g94414c4

[PATCH 06/17] PM / OPP: Parse clock-latency and voltage-tolerance for v1 bindings

[Viresh Kumar]

V2 bindings have better support for clock-latency and voltage-tolerance
and doesn't need special care. To use callbacks, like
dev_pm_opp_get_max_{transition|volt}_latency(), irrespective of the
bindings, the core needs to know clock-latency/voltage-tolerance for the
earlier bindings.

This patch reads clock-latency/voltage-tolerance from the device node,
irrespective of the bindings (to keep it simple) and use them only for
V1 bindings.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 20 ++++++++++++++++++++
 drivers/base/power/opp/opp.h  |  6 ++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 5746993aa71d..1bb09138cdae 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -563,6 +563,7 @@ static struct device_opp *_add_device_opp_reg(struct device *dev,
 {
 	struct device_opp *dev_opp;
 	struct device_list_opp *list_dev;
+	struct device_node *np;
 
 	/*
 	 * Allocate a new device OPP table. In the infrequent case where a new
@@ -580,6 +581,21 @@ static struct device_opp *_add_device_opp_reg(struct device *dev,
 		return NULL;
 	}
 
+	/*
+	 * 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;
+
+		if (!of_property_read_u32(np, "clock-latency", &val))
+			dev_opp->clock_latency_ns_max = val;
+		of_property_read_u32(np, "voltage-tolerance",
+				     &dev_opp->voltage_tolerance_v1);
+		of_node_put(np);
+	}
+
 	dev_opp->regulator = regulator_get_optional(dev, name);
 	if (IS_ERR(dev_opp->regulator))
 		dev_info(dev, "%s: no regulator (%s) found: %ld\n", __func__,
@@ -865,6 +881,7 @@ static int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
 {
 	struct device_opp *dev_opp;
 	struct dev_pm_opp *new_opp;
+	unsigned long tol;
 	int ret;
 
 	/* Hold our list modification lock here */
@@ -878,7 +895,10 @@ static int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
 
 	/* populate the opp table */
 	new_opp->rate = freq;
+	tol = u_volt * dev_opp->voltage_tolerance_v1 / 100;
 	new_opp->u_volt = u_volt;
+	new_opp->u_volt_min = u_volt - tol;
+	new_opp->u_volt_max = u_volt + tol;
 	new_opp->available = true;
 	new_opp->dynamic = dynamic;
 
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 08aae35ab8e0..a39347d8693f 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -139,6 +139,8 @@ struct device_list_opp {
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
+ * @voltage_tolerance_v1: In percentage, for v1 bindings only.
+ *
  * This is an internal data structure maintaining the link to opps attached to
  * a device. This structure is not meant to be shared to users as it is
  * meant for book keeping and private to OPP library.
@@ -157,6 +159,10 @@ struct device_opp {
 
 	struct device_node *np;
 	unsigned long clock_latency_ns_max;
+
+	/* For backward compatibility with v1 bindings */
+	unsigned int voltage_tolerance_v1;
+
 	bool shared_opp;
 	struct dev_pm_opp *suspend_opp;
 
-- 
2.7.0.rc1.186.g94414c4

[PATCH 07/17] PM / OPP: Manage device clk

[Viresh Kumar]

OPP core has got almost everything now to manage device's OPP
transitions, the only thing left is device's clk. Get that as well.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 9 +++++++++
 drivers/base/power/opp/opp.h  | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 1bb09138cdae..1d1b0faa825d 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -596,6 +596,11 @@ static struct device_opp *_add_device_opp_reg(struct device *dev,
 		of_node_put(np);
 	}
 
+	/* Find clk for the device */
+	dev_opp->clk = clk_get(dev, NULL);
+	if (IS_ERR(dev_opp->clk))
+		dev_dbg(dev, "%s: Couldn't find clock\n", __func__);
+
 	dev_opp->regulator = regulator_get_optional(dev, name);
 	if (IS_ERR(dev_opp->regulator))
 		dev_info(dev, "%s: no regulator (%s) found: %ld\n", __func__,
@@ -663,6 +668,10 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (dev_opp->regulator_set)
 		return;
 
+	/* Release clk */
+	if (!IS_ERR(dev_opp->clk))
+		clk_put(dev_opp->clk);
+
 	regulator_put(dev_opp->regulator);
 
 	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index a39347d8693f..42759f025ef1 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -14,6 +14,7 @@
 #ifndef __DRIVER_OPP_H__
 #define __DRIVER_OPP_H__
 
+#include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -133,6 +134,7 @@ struct device_list_opp {
  * @supported_hw: Array of version number to support.
  * @supported_hw_count: Number of elements in supported_hw array.
  * @prop_name: A name to postfix to many DT properties, while parsing them.
+ * @clk:	struct clk pointer
  * @regulator: Supply Regulator
  * @regulator_set: Regulator's name is explicitly set by platform.
  *
@@ -169,6 +171,7 @@ struct device_opp {
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
 	const char *prop_name;
+	struct clk *clk;
 	struct regulator *regulator;
 	bool regulator_set;
 
-- 
2.7.0.rc1.186.g94414c4

[PATCH 08/17] PM / OPP: Add dev_pm_opp_set_rate()

[Viresh Kumar]

This adds a routine, dev_pm_opp_set_rate(), responsible for configuring
power-supply and clock source for an OPP.

The OPP is found by matching against the target_freq passed to the
routine. This shall replace similar code present in most of the OPP
users and help simplify them a lot.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 142 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h        |   6 ++
 2 files changed, 148 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 1d1b0faa825d..c76818f69f14 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -517,6 +517,148 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
+/*
+ * _set_opp_voltage() - Configure device supply for an OPP
+ * @dev:	device for which we do this operation
+ * @dev_opp:	dev_opp for the device
+ * @opp:	opp for getting supply voltage levels
+ *
+ * This is used to configure the power supply, used by the device, to the
+ * voltage levels specified by a particular OPP.
+ */
+static int _set_opp_voltage(struct device *dev, struct device_opp *dev_opp,
+			    struct dev_pm_opp *opp)
+{
+	struct regulator *reg = dev_opp->regulator;
+	int ret;
+
+	/* Regulator not be available for device */
+	if (IS_ERR(reg)) {
+		dev_dbg(dev, "%s: regulator not available: %ld\n", __func__,
+			PTR_ERR(reg));
+		return 0;
+	}
+
+	dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
+		opp->u_volt_min, opp->u_volt, opp->u_volt_max);
+
+	ret = regulator_set_voltage_triplet(reg, opp->u_volt_min, opp->u_volt,
+					    opp->u_volt_max);
+	if (ret)
+		dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n",
+			__func__, opp->u_volt_min, opp->u_volt, opp->u_volt_max,
+			ret);
+
+	return ret;
+}
+
+/**
+ * dev_pm_opp_set_rate() - Configure new OPP based on frequency
+ * @dev:	 device for which we do this operation
+ * @target_freq: frequency to achieve
+ *
+ * This configures the power-supplies and clock source to the levels specified
+ * by the OPP corresponding to the target_freq. It takes all necessary locks
+ * required for the operation and the caller doesn't need to care about the rcu
+ * locks.
+ */
+int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
+{
+	struct device_opp *dev_opp;
+	struct dev_pm_opp *old_opp, *opp;
+	struct clk *clk;
+	unsigned long freq, old_freq;
+	int ret;
+
+	if (unlikely(!target_freq)) {
+		dev_err(dev, "%s: Invalid target frequemncy %lu\n", __func__,
+			target_freq);
+		return -EINVAL;
+	}
+
+	rcu_read_lock();
+
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp)) {
+		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
+		ret = PTR_ERR(dev_opp);
+		goto unlock;
+	}
+
+	clk = dev_opp->clk;
+	if (IS_ERR(clk)) {
+		dev_err(dev, "%s: No clock available for the device\n",
+			__func__);
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	freq = clk_round_rate(clk, target_freq);
+	if ((long)freq <= 0)
+		freq = target_freq;
+
+	old_freq = clk_get_rate(clk);
+
+	/* Return early if nothing to do */
+	if (old_freq == freq) {
+		dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
+			__func__, freq);
+		ret = 0;
+		goto unlock;
+	}
+
+	old_opp = dev_pm_opp_find_freq_ceil(dev, &old_freq);
+	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+
+	if (IS_ERR(opp)) {
+		ret = PTR_ERR(opp);
+		dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
+			__func__, freq, ret);
+		goto unlock;
+	}
+
+	/* Scaling up? Scale voltage before frequency */
+	if (freq > old_freq) {
+		ret = _set_opp_voltage(dev, dev_opp, opp);
+		if (ret)
+			goto restore_voltage;
+	}
+
+	/* Change frequency */
+
+	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",
+		__func__, old_freq, freq);
+
+	ret = clk_set_rate(clk, freq);
+	if (ret) {
+		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
+			ret);
+		goto restore_voltage;
+	}
+
+	/* Scaling down? Scale voltage after frequency */
+	if (freq < old_freq) {
+		ret = _set_opp_voltage(dev, dev_opp, opp);
+		if (ret)
+			goto restore_freq;
+	}
+
+	goto unlock;
+
+restore_freq:
+	if (clk_set_rate(clk, old_freq))
+		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
+			__func__, old_freq);
+restore_voltage:
+	/* This shouldn't harm if the voltages weren't updated earlier */
+	_set_opp_voltage(dev, dev_opp, old_opp);
+unlock:
+	rcu_read_unlock();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
+
 /* List-dev Helpers */
 static void _kfree_list_dev_rcu(struct rcu_head *head)
 {
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 59da3d9e11ea..cccaf4a29e9f 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -64,6 +64,7 @@ int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
 void dev_pm_opp_put_prop_name(struct device *dev);
 int dev_pm_opp_set_regulator(struct device *dev, const char *name);
 void dev_pm_opp_put_regulator(struct device *dev);
+int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 #else
 static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 {
@@ -172,6 +173,11 @@ static inline int dev_pm_opp_set_regulator(struct device *dev, const char *name)
 
 static inline void dev_pm_opp_put_regulator(struct device *dev) {}
 
+static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
+{
+	return -EINVAL;
+}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
-- 
2.7.0.rc1.186.g94414c4

[PATCH 10/17] cpufreq: dt: Rename 'need_update' to 'opp_v1'

[Viresh Kumar]

That's the real purpose of this field, i.e. to take special care of old
OPP V1 bindings. Lets name it accordingly, so that it can be used
elsewhere.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 45ff7671a883..7af6e466205f 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -199,7 +199,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	struct dev_pm_opp *suspend_opp;
 	unsigned long min_uV = ~0, max_uV = 0;
 	unsigned int transition_latency;
-	bool need_update = false;
+	bool opp_v1 = false;
 	int ret;
 
 	ret = allocate_resources(policy->cpu, &cpu_dev, &cpu_reg, &cpu_clk);
@@ -223,7 +223,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		 * finding shared-OPPs for backward compatibility.
 		 */
 		if (ret == -ENOENT)
-			need_update = true;
+			opp_v1 = true;
 		else
 			goto out_node_put;
 	}
@@ -251,7 +251,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		goto out_free_opp;
 	}
 
-	if (need_update) {
+	if (opp_v1) {
 		struct cpufreq_dt_platform_data *pd = cpufreq_get_driver_data();
 
 		if (!pd || !pd->independent_clocks)
-- 
2.7.0.rc1.186.g94414c4

[PATCH 11/17] cpufreq: dt: OPP layers handles clock-latency for V1 bindings as well

[Viresh Kumar]

"clock-latency" is handled by OPP layer for all bindings and so there is
no need to make special calls for V1 bindings. Use
dev_pm_opp_get_max_clock_latency() for both the cases.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 7af6e466205f..a6f91da7283e 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -265,10 +265,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		if (ret)
 			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
 				__func__, ret);
-
-		of_property_read_u32(np, "clock-latency", &transition_latency);
-	} else {
-		transition_latency = dev_pm_opp_get_max_clock_latency(cpu_dev);
 	}
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
@@ -279,6 +275,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 
 	of_property_read_u32(np, "voltage-tolerance", &priv->voltage_tolerance);
 
+	transition_latency = dev_pm_opp_get_max_clock_latency(cpu_dev);
 	if (!transition_latency)
 		transition_latency = CPUFREQ_ETERNAL;
 
-- 
2.7.0.rc1.186.g94414c4

[PATCH 12/17] cpufreq: dt: Pass regulator name to the OPP core for V1 bindings

[Viresh Kumar]

OPP core can handle the regulators by itself, and it allocates the
regulator based on device's name. But for older V1 bindings, many DT
files have used names like 'cpu-supply' instead of 'cpu0-supply'.

The cpufreq-dt driver needs to tell the right name of the regulator in
this case to the OPP core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index a6f91da7283e..39df4f1a06d2 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -34,6 +34,7 @@ struct private_data {
 	struct regulator *cpu_reg;
 	struct thermal_cooling_device *cdev;
 	unsigned int voltage_tolerance; /* in percentage */
+	const char *reg_name;
 };
 
 static struct freq_attr *cpufreq_dt_attr[] = {
@@ -118,6 +119,22 @@ static int set_target(struct cpufreq_policy *policy, unsigned int index)
 	return ret;
 }
 
+/*
+ * An earlier version of opp-v1 bindings used to name the regulator
+ * "cpu-supply", we still need to handle that for backwards compatibility.
+ */
+static const char *find_supply_name(struct device *dev)
+{
+	struct regulator *cpu_reg;
+
+	cpu_reg = regulator_get_optional(dev, dev_name(dev));
+	if (IS_ERR(cpu_reg))
+		return "cpu";
+
+	regulator_put(cpu_reg);
+	return NULL;
+}
+
 static int allocate_resources(int cpu, struct device **cdev,
 			      struct regulator **creg, struct clk **cclk)
 {
@@ -200,6 +217,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	unsigned long min_uV = ~0, max_uV = 0;
 	unsigned int transition_latency;
 	bool opp_v1 = false;
+	const char *name = NULL;
 	int ret;
 
 	ret = allocate_resources(policy->cpu, &cpu_dev, &cpu_reg, &cpu_clk);
@@ -229,6 +247,25 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	}
 
 	/*
+	 * OPP layer will be taking care of regulators now, but it needs to know
+	 * the name of the regulator for v1 bindings.
+	 */
+	if (opp_v1) {
+		name = find_supply_name(cpu_dev);
+		if (name) {
+			ret = dev_pm_opp_set_regulator(cpu_dev, name);
+			if (ret) {
+				/*
+				 * Regulators aren't compulsory on few
+				 * platforms, don't error out for them.
+				 */
+				dev_dbg(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
+					policy->cpu, ret);
+			}
+		}
+	}
+
+	/*
 	 * Initialize OPP tables for all policy->cpus. They will be shared by
 	 * all CPUs which have marked their CPUs shared with OPP bindings.
 	 *
@@ -273,6 +310,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		goto out_free_opp;
 	}
 
+	priv->reg_name = name;
 	of_property_read_u32(np, "voltage-tolerance", &priv->voltage_tolerance);
 
 	transition_latency = dev_pm_opp_get_max_clock_latency(cpu_dev);
@@ -366,6 +404,8 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	kfree(priv);
 out_free_opp:
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
+	if (opp_v1)
+		dev_pm_opp_put_regulator(cpu_dev);
 out_node_put:
 	of_node_put(np);
 out_put_reg_clk:
@@ -383,6 +423,9 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 	cpufreq_cooling_unregister(priv->cdev);
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
 	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
+	if (priv->reg_name)
+		dev_pm_opp_put_regulator(priv->cpu_dev);
+
 	clk_put(policy->clk);
 	if (!IS_ERR(priv->cpu_reg))
 		regulator_put(priv->cpu_reg);
-- 
2.7.0.rc1.186.g94414c4

[PATCH 13/17] cpufreq: dt: Unsupported OPPs are already disabled

[Viresh Kumar]

The core already have a valid regulator set for the device opp and the
unsupported OPPs are already disabled by the core. There is no need to
repeat that in the user drivers, get rid of it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 39df4f1a06d2..8b6e55f62852 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -346,8 +346,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 					min_uV = opp_uV;
 				if (opp_uV > max_uV)
 					max_uV = opp_uV;
-			} else {
-				dev_pm_opp_disable(cpu_dev, opp_freq);
 			}
 
 			opp_freq++;
-- 
2.7.0.rc1.186.g94414c4

[PATCH 14/17] cpufreq: dt: Reuse dev_pm_opp_get_max_transition_latency()

[Viresh Kumar]

OPP layer has all the information now to calculate transition latency
(clock_latency + voltage_latency). Lets reuse the OPP layer helper
dev_pm_opp_get_max_transition_latency() instead of open coding the same
in cpufreq-dt driver.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 48 ++++----------------------------------------
 1 file changed, 4 insertions(+), 44 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 8b6e55f62852..01a7354b1ada 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -214,7 +214,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	struct regulator *cpu_reg;
 	struct clk *cpu_clk;
 	struct dev_pm_opp *suspend_opp;
-	unsigned long min_uV = ~0, max_uV = 0;
 	unsigned int transition_latency;
 	bool opp_v1 = false;
 	const char *name = NULL;
@@ -313,49 +312,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	priv->reg_name = name;
 	of_property_read_u32(np, "voltage-tolerance", &priv->voltage_tolerance);
 
-	transition_latency = dev_pm_opp_get_max_clock_latency(cpu_dev);
-	if (!transition_latency)
-		transition_latency = CPUFREQ_ETERNAL;
-
-	if (!IS_ERR(cpu_reg)) {
-		unsigned long opp_freq = 0;
-
-		/*
-		 * Disable any OPPs where the connected regulator isn't able to
-		 * provide the specified voltage and record minimum and maximum
-		 * voltage levels.
-		 */
-		while (1) {
-			struct dev_pm_opp *opp;
-			unsigned long opp_uV, tol_uV;
-
-			rcu_read_lock();
-			opp = dev_pm_opp_find_freq_ceil(cpu_dev, &opp_freq);
-			if (IS_ERR(opp)) {
-				rcu_read_unlock();
-				break;
-			}
-			opp_uV = dev_pm_opp_get_voltage(opp);
-			rcu_read_unlock();
-
-			tol_uV = opp_uV * priv->voltage_tolerance / 100;
-			if (regulator_is_supported_voltage(cpu_reg,
-							   opp_uV - tol_uV,
-							   opp_uV + tol_uV)) {
-				if (opp_uV < min_uV)
-					min_uV = opp_uV;
-				if (opp_uV > max_uV)
-					max_uV = opp_uV;
-			}
-
-			opp_freq++;
-		}
-
-		ret = regulator_set_voltage_time(cpu_reg, min_uV, max_uV);
-		if (ret > 0)
-			transition_latency += ret * 1000;
-	}
-
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {
 		dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
@@ -390,6 +346,10 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		cpufreq_dt_attr[1] = &cpufreq_freq_attr_scaling_boost_freqs;
 	}
 
+	transition_latency = dev_pm_opp_get_max_transition_latency(cpu_dev);
+	if (!transition_latency)
+		transition_latency = CPUFREQ_ETERNAL;
+
 	policy->cpuinfo.transition_latency = transition_latency;
 
 	of_node_put(np);
-- 
2.7.0.rc1.186.g94414c4

[PATCH 15/17] cpufreq: dt: Use dev_pm_opp_set_rate() to switch frequency

[Viresh Kumar]

OPP core supports frequency/voltage changes based on the target
frequency now, use that instead of open coding the same in cpufreq-dt
driver.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 72 ++------------------------------------------
 1 file changed, 2 insertions(+), 70 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 01a7354b1ada..89c89dfab237 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -45,78 +45,10 @@ static struct freq_attr *cpufreq_dt_attr[] = {
 
 static int set_target(struct cpufreq_policy *policy, unsigned int index)
 {
-	struct dev_pm_opp *opp;
-	struct cpufreq_frequency_table *freq_table = policy->freq_table;
-	struct clk *cpu_clk = policy->clk;
 	struct private_data *priv = policy->driver_data;
-	struct device *cpu_dev = priv->cpu_dev;
-	struct regulator *cpu_reg = priv->cpu_reg;
-	unsigned long volt = 0, volt_old = 0, tol = 0;
-	unsigned int old_freq, new_freq;
-	long freq_Hz, freq_exact;
-	int ret;
-
-	freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
-	if (freq_Hz <= 0)
-		freq_Hz = freq_table[index].frequency * 1000;
-
-	freq_exact = freq_Hz;
-	new_freq = freq_Hz / 1000;
-	old_freq = clk_get_rate(cpu_clk) / 1000;
 
-	if (!IS_ERR(cpu_reg)) {
-		unsigned long opp_freq;
-
-		rcu_read_lock();
-		opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_Hz);
-		if (IS_ERR(opp)) {
-			rcu_read_unlock();
-			dev_err(cpu_dev, "failed to find OPP for %ld\n",
-				freq_Hz);
-			return PTR_ERR(opp);
-		}
-		volt = dev_pm_opp_get_voltage(opp);
-		opp_freq = dev_pm_opp_get_freq(opp);
-		rcu_read_unlock();
-		tol = volt * priv->voltage_tolerance / 100;
-		volt_old = regulator_get_voltage(cpu_reg);
-		dev_dbg(cpu_dev, "Found OPP: %ld kHz, %ld uV\n",
-			opp_freq / 1000, volt);
-	}
-
-	dev_dbg(cpu_dev, "%u MHz, %ld mV --> %u MHz, %ld mV\n",
-		old_freq / 1000, (volt_old > 0) ? volt_old / 1000 : -1,
-		new_freq / 1000, volt ? volt / 1000 : -1);
-
-	/* scaling up?  scale voltage before frequency */
-	if (!IS_ERR(cpu_reg) && new_freq > old_freq) {
-		ret = regulator_set_voltage_tol(cpu_reg, volt, tol);
-		if (ret) {
-			dev_err(cpu_dev, "failed to scale voltage up: %d\n",
-				ret);
-			return ret;
-		}
-	}
-
-	ret = clk_set_rate(cpu_clk, freq_exact);
-	if (ret) {
-		dev_err(cpu_dev, "failed to set clock rate: %d\n", ret);
-		if (!IS_ERR(cpu_reg) && volt_old > 0)
-			regulator_set_voltage_tol(cpu_reg, volt_old, tol);
-		return ret;
-	}
-
-	/* scaling down?  scale voltage after frequency */
-	if (!IS_ERR(cpu_reg) && new_freq < old_freq) {
-		ret = regulator_set_voltage_tol(cpu_reg, volt, tol);
-		if (ret) {
-			dev_err(cpu_dev, "failed to scale voltage down: %d\n",
-				ret);
-			clk_set_rate(cpu_clk, old_freq * 1000);
-		}
-	}
-
-	return ret;
+	return dev_pm_opp_set_rate(priv->cpu_dev,
+				   policy->freq_table[index].frequency * 1000);
 }
 
 /*
-- 
2.7.0.rc1.186.g94414c4

[PATCH 16/17] cpufreq: dt: drop references to DT node

[Viresh Kumar]

We don't need to get reference to DT node now, lets drop it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 89c89dfab237..c9b7dafd986d 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -33,7 +33,6 @@ struct private_data {
 	struct device *cpu_dev;
 	struct regulator *cpu_reg;
 	struct thermal_cooling_device *cdev;
-	unsigned int voltage_tolerance; /* in percentage */
 	const char *reg_name;
 };
 
@@ -140,7 +139,6 @@ static int allocate_resources(int cpu, struct device **cdev,
 static int cpufreq_init(struct cpufreq_policy *policy)
 {
 	struct cpufreq_frequency_table *freq_table;
-	struct device_node *np;
 	struct private_data *priv;
 	struct device *cpu_dev;
 	struct regulator *cpu_reg;
@@ -157,13 +155,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		return ret;
 	}
 
-	np = of_node_get(cpu_dev->of_node);
-	if (!np) {
-		dev_err(cpu_dev, "failed to find cpu%d node\n", policy->cpu);
-		ret = -ENOENT;
-		goto out_put_reg_clk;
-	}
-
 	/* Get OPP-sharing information from "operating-points-v2" bindings */
 	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, policy->cpus);
 	if (ret) {
@@ -174,7 +165,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		if (ret == -ENOENT)
 			opp_v1 = true;
 		else
-			goto out_node_put;
+			goto out_put_reg_clk;
 	}
 
 	/*
@@ -242,7 +233,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	}
 
 	priv->reg_name = name;
-	of_property_read_u32(np, "voltage-tolerance", &priv->voltage_tolerance);
 
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {
@@ -284,8 +274,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 
 	policy->cpuinfo.transition_latency = transition_latency;
 
-	of_node_put(np);
-
 	return 0;
 
 out_free_cpufreq_table:
@@ -296,8 +284,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
 	if (opp_v1)
 		dev_pm_opp_put_regulator(cpu_dev);
-out_node_put:
-	of_node_put(np);
 out_put_reg_clk:
 	clk_put(cpu_clk);
 	if (!IS_ERR(cpu_reg))
-- 
2.7.0.rc1.186.g94414c4

[PATCH 17/17] cpufreq: dt: No need to allocate resources anymore

[Viresh Kumar]

OPP layer manages it now and cpufreq-dt driver doesn't need it. But, we
still need to check for availability of resources for deferred probing.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 70 ++++++++++++++++----------------------------
 1 file changed, 25 insertions(+), 45 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index c9b7dafd986d..c778d20f8c37 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -31,7 +31,6 @@
 
 struct private_data {
 	struct device *cpu_dev;
-	struct regulator *cpu_reg;
 	struct thermal_cooling_device *cdev;
 	const char *reg_name;
 };
@@ -66,8 +65,7 @@ static const char *find_supply_name(struct device *dev)
 	return NULL;
 }
 
-static int allocate_resources(int cpu, struct device **cdev,
-			      struct regulator **creg, struct clk **cclk)
+static int resources_available(void)
 {
 	struct device *cpu_dev;
 	struct regulator *cpu_reg;
@@ -75,17 +73,14 @@ static int allocate_resources(int cpu, struct device **cdev,
 	int ret = 0;
 	char *reg_cpu0 = "cpu0", *reg_cpu = "cpu", *reg;
 
-	cpu_dev = get_cpu_device(cpu);
+	cpu_dev = get_cpu_device(0);
 	if (!cpu_dev) {
-		pr_err("failed to get cpu%d device\n", cpu);
+		pr_err("failed to get cpu0 device\n");
 		return -ENODEV;
 	}
 
 	/* Try "cpu0" for older DTs */
-	if (!cpu)
-		reg = reg_cpu0;
-	else
-		reg = reg_cpu;
+	reg = reg_cpu0;
 
 try_again:
 	cpu_reg = regulator_get_optional(cpu_dev, reg);
@@ -95,8 +90,8 @@ static int allocate_resources(int cpu, struct device **cdev,
 		 * not yet registered, we should try defering probe.
 		 */
 		if (PTR_ERR(cpu_reg) == -EPROBE_DEFER) {
-			dev_dbg(cpu_dev, "cpu%d regulator not ready, retry\n",
-				cpu);
+			dev_dbg(cpu_dev, "cpu%s regulator not ready, retry\n",
+				reg);
 			return -EPROBE_DEFER;
 		}
 
@@ -105,17 +100,12 @@ static int allocate_resources(int cpu, struct device **cdev,
 			reg = reg_cpu;
 			goto try_again;
 		}
-
-		dev_dbg(cpu_dev, "no regulator for cpu%d: %ld\n",
-			cpu, PTR_ERR(cpu_reg));
+	} else {
+		regulator_put(cpu_reg);
 	}
 
 	cpu_clk = clk_get(cpu_dev, NULL);
 	if (IS_ERR(cpu_clk)) {
-		/* put regulator */
-		if (!IS_ERR(cpu_reg))
-			regulator_put(cpu_reg);
-
 		ret = PTR_ERR(cpu_clk);
 
 		/*
@@ -123,14 +113,11 @@ static int allocate_resources(int cpu, struct device **cdev,
 		 * registered, we should try defering probe.
 		 */
 		if (ret == -EPROBE_DEFER)
-			dev_dbg(cpu_dev, "cpu%d clock not ready, retry\n", cpu);
+			dev_dbg(cpu_dev, "cpu0 clock not ready, retry\n");
 		else
-			dev_err(cpu_dev, "failed to get cpu%d clock: %d\n", cpu,
-				ret);
+			dev_err(cpu_dev, "failed to get cpu0 clock: %d\n", ret);
 	} else {
-		*cdev = cpu_dev;
-		*creg = cpu_reg;
-		*cclk = cpu_clk;
+		clk_put(cpu_clk);
 	}
 
 	return ret;
@@ -141,7 +128,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	struct cpufreq_frequency_table *freq_table;
 	struct private_data *priv;
 	struct device *cpu_dev;
-	struct regulator *cpu_reg;
 	struct clk *cpu_clk;
 	struct dev_pm_opp *suspend_opp;
 	unsigned int transition_latency;
@@ -149,9 +135,16 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	const char *name = NULL;
 	int ret;
 
-	ret = allocate_resources(policy->cpu, &cpu_dev, &cpu_reg, &cpu_clk);
-	if (ret) {
-		pr_err("%s: Failed to allocate resources: %d\n", __func__, ret);
+	cpu_dev = get_cpu_device(policy->cpu);
+	if (!cpu_dev) {
+		pr_err("failed to get cpu%d device\n", policy->cpu);
+		return -ENODEV;
+	}
+
+	cpu_clk = clk_get(cpu_dev, NULL);
+	if (IS_ERR(cpu_clk)) {
+		ret = PTR_ERR(cpu_clk);
+		dev_err(cpu_dev, "%s: failed to get clk: %d\n", __func__, ret);
 		return ret;
 	}
 
@@ -165,7 +158,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		if (ret == -ENOENT)
 			opp_v1 = true;
 		else
-			goto out_put_reg_clk;
+			goto out_put_clk;
 	}
 
 	/*
@@ -241,9 +234,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	}
 
 	priv->cpu_dev = cpu_dev;
-	priv->cpu_reg = cpu_reg;
 	policy->driver_data = priv;
-
 	policy->clk = cpu_clk;
 
 	rcu_read_lock();
@@ -284,10 +275,8 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
 	if (opp_v1)
 		dev_pm_opp_put_regulator(cpu_dev);
-out_put_reg_clk:
+out_put_clk:
 	clk_put(cpu_clk);
-	if (!IS_ERR(cpu_reg))
-		regulator_put(cpu_reg);
 
 	return ret;
 }
@@ -303,8 +292,6 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 		dev_pm_opp_put_regulator(priv->cpu_dev);
 
 	clk_put(policy->clk);
-	if (!IS_ERR(priv->cpu_reg))
-		regulator_put(priv->cpu_reg);
 	kfree(priv);
 
 	return 0;
@@ -357,9 +344,6 @@ static struct cpufreq_driver dt_cpufreq_driver = {
 
 static int dt_cpufreq_probe(struct platform_device *pdev)
 {
-	struct device *cpu_dev;
-	struct regulator *cpu_reg;
-	struct clk *cpu_clk;
 	int ret;
 
 	/*
@@ -369,19 +353,15 @@ static int dt_cpufreq_probe(struct platform_device *pdev)
 	 *
 	 * FIXME: Is checking this only for CPU0 sufficient ?
 	 */
-	ret = allocate_resources(0, &cpu_dev, &cpu_reg, &cpu_clk);
+	ret = resources_available();
 	if (ret)
 		return ret;
 
-	clk_put(cpu_clk);
-	if (!IS_ERR(cpu_reg))
-		regulator_put(cpu_reg);
-
 	dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);
 
 	ret = cpufreq_register_driver(&dt_cpufreq_driver);
 	if (ret)
-		dev_err(cpu_dev, "failed register driver: %d\n", ret);
+		dev_err(&pdev->dev, "failed register driver: %d\n", ret);
 
 	return ret;
 }
-- 
2.7.0.rc1.186.g94414c4

[PATCH 09/17] cpufreq: dt: Convert few pr_debug/err() calls to dev_dbg/err()

[Viresh Kumar]

We have the device structure available now, lets use it for better print
messages.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Resend:
- Mail was blocked by linux-pm due to XXX in $Subject :)

 drivers/cpufreq/cpufreq-dt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 1ceece9d6711..45ff7671a883 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -246,7 +246,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	 */
 	ret = dev_pm_opp_get_opp_count(cpu_dev);
 	if (ret <= 0) {
-		pr_debug("OPP table is not ready, deferring probe\n");
+		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
 		ret = -EPROBE_DEFER;
 		goto out_free_opp;
 	}
@@ -325,7 +325,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {
-		pr_err("failed to init cpufreq table: %d\n", ret);
+		dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
 		goto out_free_priv;
 	}
 
-- 
2.7.0.rc1.186.g94414c4

Re: [PATCH 00/17] PM / OPP: Introduce APIs to transition OPPs

[Viresh Kumar]

On 23-12-15, 04:12, Rafael J. Wysocki wrote:
> On Tuesday, December 22, 2015 03:46:01 PM Viresh Kumar wrote:
> > This patchset add APIs in OPP layer to allow OPPs transitioning from
> > within OPP layer. Currently all OPP users need to replicate the same
> > code to switch between OPPs. While the same can be handled easily by
> > OPP-core.
> > 
> > The first Eight patches update the OPP core to introduce the new APIs
> > and the next Nine patches update cpufreq-dt for the same.
> > 
> > Testing:
> > - Tested on exynos 5250-arndale (dual-cortex-A15)
> > - Tested for both Old-V1 bindings and New V2 bindings
> > - Tested with regulator names as: 'cpu-supply' and 'cpu0-supply'
> > - Tested with Unsupported supply ranges as well, to check the
> >   opp-disable logic
> > 
> > Viresh Kumar (17):
> >   PM / OPP: get/put regulators from OPP core
> >   PM / OPP: Add APIs to set regulator-name
> >   PM / OPP: Disable OPPs that aren't supported by the regulator
> >   PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()
> >   PM / OPP: Introduce dev_pm_opp_get_max_transition_latency()
> >   PM / OPP: Parse clock-latency and voltage-tolerance for v1 bindings
> >   PM / OPP: Manage device clk
> >   PM / OPP: Add dev_pm_opp_set_rate()
> >   cpufreq: dt: Convert few pr_XXX() calls to dev_XXX()
> >   cpufreq: dt: Rename 'need_update' to 'opp_v1'
> >   cpufreq: dt: OPP layers handles clock-latency for V1 bindings as well
> >   cpufreq: dt: Pass regulator name to the OPP core for V1 bindings
> >   cpufreq: dt: Unsupported OPPs are already disabled
> >   cpufreq: dt: Reuse dev_pm_opp_get_max_transition_latency()
> >   cpufreq: dt: Use dev_pm_opp_set_rate() to switch frequency
> >   cpufreq: dt: drop references to DT node
> >   cpufreq: dt: No need to allocate resources anymore
> > 
> >  drivers/base/power/opp/core.c | 428 ++++++++++++++++++++++++++++++++++++++++--
> >  drivers/base/power/opp/opp.h  |  15 ++
> >  drivers/cpufreq/cpufreq-dt.c  | 254 ++++++++-----------------
> >  include/linux/pm_opp.h        |  27 +++
> >  4 files changed, 530 insertions(+), 194 deletions(-)
> 
> As per our earlier deal regarding the OPP maintenance, I'll be waiting for ACKs
> from other OPP developers for this series.

Yeah, I was sitting on them for a long time now (based on the
multi-regulator support, which never went anywhere). And so wanted to
get them out..

I will wait for Stephen to come and look at them, Nishanth seems to be
too busy to do reviews here :)

-- 
viresh

Re: [PATCH 00/17] PM / OPP: Introduce APIs to transition OPPs

[Rafael J. Wysocki]

On Tuesday, December 22, 2015 03:46:01 PM Viresh Kumar wrote:
> This patchset add APIs in OPP layer to allow OPPs transitioning from
> within OPP layer. Currently all OPP users need to replicate the same
> code to switch between OPPs. While the same can be handled easily by
> OPP-core.
> 
> The first Eight patches update the OPP core to introduce the new APIs
> and the next Nine patches update cpufreq-dt for the same.
> 
> Testing:
> - Tested on exynos 5250-arndale (dual-cortex-A15)
> - Tested for both Old-V1 bindings and New V2 bindings
> - Tested with regulator names as: 'cpu-supply' and 'cpu0-supply'
> - Tested with Unsupported supply ranges as well, to check the
>   opp-disable logic
> 
> Viresh Kumar (17):
>   PM / OPP: get/put regulators from OPP core
>   PM / OPP: Add APIs to set regulator-name
>   PM / OPP: Disable OPPs that aren't supported by the regulator
>   PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()
>   PM / OPP: Introduce dev_pm_opp_get_max_transition_latency()
>   PM / OPP: Parse clock-latency and voltage-tolerance for v1 bindings
>   PM / OPP: Manage device clk
>   PM / OPP: Add dev_pm_opp_set_rate()
>   cpufreq: dt: Convert few pr_XXX() calls to dev_XXX()
>   cpufreq: dt: Rename 'need_update' to 'opp_v1'
>   cpufreq: dt: OPP layers handles clock-latency for V1 bindings as well
>   cpufreq: dt: Pass regulator name to the OPP core for V1 bindings
>   cpufreq: dt: Unsupported OPPs are already disabled
>   cpufreq: dt: Reuse dev_pm_opp_get_max_transition_latency()
>   cpufreq: dt: Use dev_pm_opp_set_rate() to switch frequency
>   cpufreq: dt: drop references to DT node
>   cpufreq: dt: No need to allocate resources anymore
> 
>  drivers/base/power/opp/core.c | 428 ++++++++++++++++++++++++++++++++++++++++--
>  drivers/base/power/opp/opp.h  |  15 ++
>  drivers/cpufreq/cpufreq-dt.c  | 254 ++++++++-----------------
>  include/linux/pm_opp.h        |  27 +++
>  4 files changed, 530 insertions(+), 194 deletions(-)

As per our earlier deal regarding the OPP maintenance, I'll be waiting for ACKs
from other OPP developers for this series.

Thanks,
Rafael

Re: [PATCH 00/17] PM / OPP: Introduce APIs to transition OPPs

[Viresh Kumar]

On 22-12-15, 15:46, Viresh Kumar wrote:
> This patchset add APIs in OPP layer to allow OPPs transitioning from
> within OPP layer. Currently all OPP users need to replicate the same
> code to switch between OPPs. While the same can be handled easily by
> OPP-core.
> 
> The first Eight patches update the OPP core to introduce the new APIs
> and the next Nine patches update cpufreq-dt for the same.

Stephen,

Ping !!

(In case you missed this thread)

-- 
viresh

Re: [PATCH 01/17] PM / OPP: get/put regulators from OPP core

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
> index 690638ef36ee..c4a03ad34100 100644
> --- a/drivers/base/power/opp/opp.h
> +++ b/drivers/base/power/opp/opp.h
> @@ -18,6 +18,7 @@
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/limits.h>
> +#include <linux/regulator/consumer.h>

This include doesn't seem necessary. Forward declare struct
regulator instead?

>  #include <linux/pm_opp.h>
>  #include <linux/rculist.h>
>  #include <linux/rcupdate.h>
> @@ -132,6 +133,8 @@ struct device_list_opp {
>   * @supported_hw: Array of version number to support.
>   * @supported_hw_count: Number of elements in supported_hw array.
>   * @prop_name: A name to postfix to many DT properties, while parsing them.
> + * @regulator: Supply Regulator

Is there a reason we capitalize regulator?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 02/17] PM / OPP: Add APIs to set regulator-name

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> This is required mostly for backward compatibility of DT users. The OPP
> layer automatically gets the regulator device currently, but the name of
> the device needs to be same as that of the device. But existing DT

The name of the device needs to be the same as that of the
device? What does that mean? Perhaps this should say the name of
the regulator/supply needs to be the same as that of the device?

The same words are in the kernel doc too.

> entries may not be following that and might have used generic names
> instead. For example, they might have used 'cpu-supply' instead of
> 'cpu0-supply'.
> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index 2e49f5e9daa3..76232ee04cc6 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -492,25 +492,11 @@ struct device_list_opp *_add_list_dev(const struct device *dev,
>  	return list_dev;
>  }
>  
> -/**
> - * _add_device_opp() - Find device OPP table or allocate a new one
> - * @dev:	device for which we do this operation
> - *
> - * It tries to find an existing table first, if it couldn't find one, it
> - * allocates a new OPP table and returns that.
> - *
> - * Return: valid device_opp pointer if success, else NULL.
> - */
> -static struct device_opp *_add_device_opp(struct device *dev)
> +static struct device_opp *_add_device_opp_reg(struct device *dev,
> +					      const char *name)
>  {
>  	struct device_opp *dev_opp;
>  	struct device_list_opp *list_dev;
> -	const char *name = dev_name(dev);
> -
> -	/* Check for existing list for 'dev' first */
> -	dev_opp = _find_device_opp(dev);
> -	if (!IS_ERR(dev_opp))
> -		return dev_opp;
>  
>  	/*
>  	 * Allocate a new device OPP table. In the infrequent case where a new
> @@ -542,6 +528,27 @@ static struct device_opp *_add_device_opp(struct device *dev)
>  }
>  
>  /**
> + * _add_device_opp() - Find device OPP table or allocate a new one
> + * @dev:	device for which we do this operation

Why the tab? I know copy/paste, but still.

> + *
> + * It tries to find an existing table first, if it couldn't find one, it
> + * allocates a new OPP table and returns that.
> + *
> + * Return: valid device_opp pointer if success, else NULL.
> + */
> @@ -1091,6 +1101,127 @@ void dev_pm_opp_put_prop_name(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
>  
> +/**
> + * dev_pm_opp_set_regulator() - Set regulator name for the device
> + * @dev: Device for which regulator name is being set.
> + * @name: Name of the regulator.
> + *
> + * This is required mostly for backward compatibility of DT users. The OPP layer
> + * automatically gets the regulator device currently, but the name of the device
> + * needs to be same as that of the device. But existing DT entries may not be
> + * following that and might have used generic names instead. For example, they
> + * might have used 'cpu-supply' instead of 'cpu0-supply'.

The new v2 OPP bindings are using cpu-supply instead of
cpu0-supply. This makes it sound like cpu-supply is the old style
and we've added this API to handle it, when in reality, this is
the preferred way to do this and so we've added this API because
we almost always need it.

> + *
> + * This must be called before any OPPs are initialized for the device.
> + *
> + * Locking: The internal device_opp and opp structures are RCU protected.
> + * Hence this function internally uses RCU updater strategy with mutex locks
> + * to keep the integrity of the internal data structures. Callers should ensure
> + * that this function is *NOT* called under RCU protection or in contexts where
> + * mutex cannot be locked.
> + */
> +int dev_pm_opp_set_regulator(struct device *dev, const char *name)
> +{
> +	struct device_opp *dev_opp;
> +	struct regulator *reg;
> +	int ret = 0;
> +
> +	/* Hold our list modification lock here */
> +	mutex_lock(&dev_opp_list_lock);
> +
> +	dev_opp = _find_device_opp(dev);
> +	/* We already have a dev-opp */
> +	if (!IS_ERR(dev_opp)) {
> +		/* This should be called before OPPs are initialized */
> +		if (WARN_ON(!list_empty(&dev_opp->opp_list))) {
> +			ret = -EBUSY;
> +			goto unlock;
> +		}
> +
> +		/* Already have a regulator set? Free it and re-allocate */
> +		if (!IS_ERR(dev_opp->regulator))
> +			regulator_put(dev_opp->regulator);

Is this used in practice? It seems odd that users would be using
one regulator, and then change that to another regulator later.

> +
> +		/* Allocate the regulator */
> +		reg = regulator_get_optional(dev, name);
> +		if (IS_ERR(reg)) {
> +			ret = PTR_ERR(reg);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "%s: no regulator (%s) found: %d\n",
> +					__func__, name, ret);
> +		}
> +
> +		dev_opp->regulator = reg;
> +		goto unlock;
> +	}
> +
[...]
> +
> +/**
> + * dev_pm_opp_put_regulator() - Releases resources blocked for regulator
> + * @dev: Device for which regulator was set.
> + *
> + * Locking: The internal device_opp and opp structures are RCU protected.
> + * Hence this function internally uses RCU updater strategy with mutex locks
> + * to keep the integrity of the internal data structures. Callers should ensure
> + * that this function is *NOT* called under RCU protection or in contexts where
> + * mutex cannot be locked.
> + */
> +void dev_pm_opp_put_regulator(struct device *dev)
> +{
> +	struct device_opp *dev_opp;
> +
> +	/* Hold our list modification lock here */

Yeah, that's obvious from the code.

> +	mutex_lock(&dev_opp_list_lock);
> +
> +	/* Check for existing list for 'dev' first */
> +	dev_opp = _find_device_opp(dev);
> +	if (IS_ERR(dev_opp)) {
> +		dev_err(dev, "Failed to find dev_opp: %ld\n", PTR_ERR(dev_opp));
> +		goto unlock;
> +	}
> +
> +	/* Make sure there are no concurrent readers while updating dev_opp */
> +	WARN_ON(!list_empty(&dev_opp->opp_list));
> +
> +	if (!dev_opp->regulator_set) {
> +		dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
> +		goto unlock;
> +	}
> +
> +	dev_opp->regulator_set = false;
> +
> +	/* Try freeing device_opp if this was the last blocking resource */
> +	_remove_device_opp(dev_opp);

It looks like this whole function exists to set the boolean to
false so that _remove_device_opp() can continue. What's the point
of that? From what I can tell all we're getting is symmetry in
the API by having this function, so why have this function at
all?


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 03/17] PM / OPP: Disable OPPs that aren't supported by the regulator

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> Disable any OPPs where the connected regulator isn't able to provide the
> specified voltage.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 04/17] PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> @@ -230,6 +230,55 @@ unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev)
>  EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_clock_latency);
>  
>  /**
> + * dev_pm_opp_get_max_volt_latency() - Get max voltage latency in nanoseconds
> + * @dev:	device for which we do this operation

Weird tab again.

> + *
> + * Return: This function returns the max voltage latency in nanoseconds.
> + *
> + * Locking: This function takes rcu_read_lock().
> + */
> +unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
> +{
> +	struct device_opp *dev_opp;
> +	struct dev_pm_opp *opp;
> +	struct regulator *reg;
> +	unsigned long latency_ns = 0;
> +	unsigned long min_uV = ~0, max_uV = 0;
> +	int ret;
> +
> +	rcu_read_lock();
> +
> +	dev_opp = _find_device_opp(dev);
> +	if (IS_ERR(dev_opp))
> +		goto unlock;
> +
> +	reg = dev_opp->regulator;
> +	/* Regulator may not be available for device */
> +	if (IS_ERR(reg))
> +		goto unlock;
> +
> +	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
> +		if (!opp->available)
> +			continue;
> +
> +		if (opp->u_volt_min < min_uV)
> +			min_uV = opp->u_volt_min;
> +		if (opp->u_volt_max > max_uV)
> +			max_uV = opp->u_volt_max;
> +	}
> +
> +	ret = regulator_set_voltage_time(reg, min_uV, max_uV);

Doesn't rcu_read_lock() disable preemption sometimes? I don't
think we can call regulator ops with preemption disabled because
regulators use mutex locking. It looks like
regulator_list_voltage() may take a mutex anyway.

> +	if (ret > 0)
> +		latency_ns += ret * 1000;

Why add to 0? Perhaps this should just be an assignment?

> +
> +unlock:
> +	rcu_read_unlock();
> +
> +	return latency_ns;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_volt_latency);
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 06/17] PM / OPP: Parse clock-latency and voltage-tolerance for v1 bindings

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> @@ -580,6 +581,21 @@ static struct device_opp *_add_device_opp_reg(struct device *dev,
>  		return NULL;
>  	}
>  
> +	/*
> +	 * 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;
> +
> +		if (!of_property_read_u32(np, "clock-latency", &val))
> +			dev_opp->clock_latency_ns_max = val;
> +		of_property_read_u32(np, "voltage-tolerance",
> +				     &dev_opp->voltage_tolerance_v1);

Why do we conditionalize the assignment for clock latency but not
for voltage tolerance?

> +		of_node_put(np);
> +	}
> +
>  	dev_opp->regulator = regulator_get_optional(dev, name);
>  	if (IS_ERR(dev_opp->regulator))
>  		dev_info(dev, "%s: no regulator (%s) found: %ld\n", __func__,

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 07/17] PM / OPP: Manage device clk

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index 1bb09138cdae..1d1b0faa825d 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -596,6 +596,11 @@ static struct device_opp *_add_device_opp_reg(struct device *dev,
>  		of_node_put(np);
>  	}
>  
> +	/* Find clk for the device */
> +	dev_opp->clk = clk_get(dev, NULL);
> +	if (IS_ERR(dev_opp->clk))

We'll need the same probe defer check here so we don't print
anything. I know common clock framework doesn't return probe
defer pointers yet, but we're working on it so it would be nice
to handle it.

> +		dev_dbg(dev, "%s: Couldn't find clock\n", __func__);
> +
>  	dev_opp->regulator = regulator_get_optional(dev, name);
>  	if (IS_ERR(dev_opp->regulator))
>  		dev_info(dev, "%s: no regulator (%s) found: %ld\n", __func__,
> diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
> index a39347d8693f..42759f025ef1 100644
> --- a/drivers/base/power/opp/opp.h
> +++ b/drivers/base/power/opp/opp.h
> @@ -14,6 +14,7 @@
>  #ifndef __DRIVER_OPP_H__
>  #define __DRIVER_OPP_H__
>  
> +#include <linux/clk.h>

Again, forward declare struct clk instead of including the
header.

>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -133,6 +134,7 @@ struct device_list_opp {
>   * @supported_hw: Array of version number to support.
>   * @supported_hw_count: Number of elements in supported_hw array.
>   * @prop_name: A name to postfix to many DT properties, while parsing them.
> + * @clk:	struct clk pointer

Why tab? And perhaps it should say "clock handle" or "supply
clock" or something instead of rewording the type "struct clk *".

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 08/17] PM / OPP: Add dev_pm_opp_set_rate()

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index 1d1b0faa825d..c76818f69f14 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -517,6 +517,148 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>  
> +/*
> + * _set_opp_voltage() - Configure device supply for an OPP
> + * @dev:	device for which we do this operation
> + * @dev_opp:	dev_opp for the device
> + * @opp:	opp for getting supply voltage levels
> + *
> + * This is used to configure the power supply, used by the device, to the
> + * voltage levels specified by a particular OPP.
> + */
> +static int _set_opp_voltage(struct device *dev, struct device_opp *dev_opp,
> +			    struct dev_pm_opp *opp)
> +{
> +	struct regulator *reg = dev_opp->regulator;
> +	int ret;
> +
> +	/* Regulator not be available for device */
> +	if (IS_ERR(reg)) {
> +		dev_dbg(dev, "%s: regulator not available: %ld\n", __func__,
> +			PTR_ERR(reg));
> +		return 0;
> +	}
> +
> +	dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
> +		opp->u_volt_min, opp->u_volt, opp->u_volt_max);
> +
> +	ret = regulator_set_voltage_triplet(reg, opp->u_volt_min, opp->u_volt,

This can't be called under an RCU read lock.

> +					    opp->u_volt_max);
> +	if (ret)
> +		dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n",
> +			__func__, opp->u_volt_min, opp->u_volt, opp->u_volt_max,
> +			ret);
> +
> +	return ret;
> +}
> +
> +/**
> + * dev_pm_opp_set_rate() - Configure new OPP based on frequency
> + * @dev:	 device for which we do this operation
> + * @target_freq: frequency to achieve
> + *
> + * This configures the power-supplies and clock source to the levels specified
> + * by the OPP corresponding to the target_freq. It takes all necessary locks
> + * required for the operation and the caller doesn't need to care about the rcu
> + * locks.
> + */
> +int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> +{
> +	struct device_opp *dev_opp;
> +	struct dev_pm_opp *old_opp, *opp;
> +	struct clk *clk;
> +	unsigned long freq, old_freq;
> +	int ret;
> +
> +	if (unlikely(!target_freq)) {
> +		dev_err(dev, "%s: Invalid target frequemncy %lu\n", __func__,

s/frequemncy/frequency/

> +			target_freq);
> +		return -EINVAL;
> +	}
> +
> +	rcu_read_lock();

This lock is held way too long.

> +
> +	dev_opp = _find_device_opp(dev);
> +	if (IS_ERR(dev_opp)) {
> +		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
> +		ret = PTR_ERR(dev_opp);
> +		goto unlock;
> +	}
> +
> +	clk = dev_opp->clk;
> +	if (IS_ERR(clk)) {
> +		dev_err(dev, "%s: No clock available for the device\n",
> +			__func__);
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	freq = clk_round_rate(clk, target_freq);

This can't be called under RCU.

> +	if ((long)freq <= 0)
> +		freq = target_freq;
> +
> +	old_freq = clk_get_rate(clk);

Same for this.

> +
> +	/* Return early if nothing to do */
> +	if (old_freq == freq) {
> +		dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
> +			__func__, freq);
> +		ret = 0;
> +		goto unlock;
> +	}
> +
> +	old_opp = dev_pm_opp_find_freq_ceil(dev, &old_freq);
> +	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> +
> +	if (IS_ERR(opp)) {
> +		ret = PTR_ERR(opp);
> +		dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
> +			__func__, freq, ret);
> +		goto unlock;
> +	}
> +
> +	/* Scaling up? Scale voltage before frequency */
> +	if (freq > old_freq) {
> +		ret = _set_opp_voltage(dev, dev_opp, opp);
> +		if (ret)
> +			goto restore_voltage;
> +	}
> +
> +	/* Change frequency */
> +
> +	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",
> +		__func__, old_freq, freq);
> +
> +	ret = clk_set_rate(clk, freq);

And same for this.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 09/17] cpufreq: dt: Convert few pr_debug/err() calls to dev_dbg/err()

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> We have the device structure available now, lets use it for better print
> messages.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 10/17] cpufreq: dt: Rename 'need_update' to 'opp_v1'

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> That's the real purpose of this field, i.e. to take special care of old
> OPP V1 bindings. Lets name it accordingly, so that it can be used
> elsewhere.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Ok...

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 11/17] cpufreq: dt: OPP layers handles clock-latency for V1 bindings as well

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> "clock-latency" is handled by OPP layer for all bindings and so there is
> no need to make special calls for V1 bindings. Use
> dev_pm_opp_get_max_clock_latency() for both the cases.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 12/17] cpufreq: dt: Pass regulator name to the OPP core for V1 bindings

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> OPP core can handle the regulators by itself, and it allocates the
> regulator based on device's name. But for older V1 bindings, many DT
> files have used names like 'cpu-supply' instead of 'cpu0-supply'.
> 
> The cpufreq-dt driver needs to tell the right name of the regulator in
> this case to the OPP core.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

This whole patch is confusing to me because old style was to be
cpu0-supply, and new style is cpu-supply.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 13/17] cpufreq: dt: Unsupported OPPs are already disabled

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> The core already have a valid regulator set for the device opp and the
> unsupported OPPs are already disabled by the core. There is no need to
> repeat that in the user drivers, get rid of it.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 14/17] cpufreq: dt: Reuse dev_pm_opp_get_max_transition_latency()

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> OPP layer has all the information now to calculate transition latency
> (clock_latency + voltage_latency). Lets reuse the OPP layer helper
> dev_pm_opp_get_max_transition_latency() instead of open coding the same
> in cpufreq-dt driver.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Maybe this can be combined with patch 13. I don't seem much need
to distinguish between the two.

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 15/17] cpufreq: dt: Use dev_pm_opp_set_rate() to switch frequency

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> OPP core supports frequency/voltage changes based on the target
> frequency now, use that instead of open coding the same in cpufreq-dt
> driver.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Nice, assuming that dev_pm_opp_set_rate() is fixed:

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 16/17] cpufreq: dt: drop references to DT node

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> We don't need to get reference to DT node now, lets drop it.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 17/17] cpufreq: dt: No need to allocate resources anymore

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> OPP layer manages it now and cpufreq-dt driver doesn't need it. But, we
> still need to check for availability of resources for deferred probing.

Why? It seems cleaner to let OPP layer return an error indicating
probe defer or failure when we try to initialize it. That way we
aren't duplicating the same logic in two places to figure out if
a regulator or clock is ready.

> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> @@ -95,8 +90,8 @@ static int allocate_resources(int cpu, struct device **cdev,
>  		 * not yet registered, we should try defering probe.
>  		 */
>  		if (PTR_ERR(cpu_reg) == -EPROBE_DEFER) {
> -			dev_dbg(cpu_dev, "cpu%d regulator not ready, retry\n",
> -				cpu);
> +			dev_dbg(cpu_dev, "cpu%s regulator not ready, retry\n",

Should that just be "%s regulator not ready"?

> +				reg);
>  			return -EPROBE_DEFER;
>  		}
> @@ -241,9 +234,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>  	}
>  
>  	priv->cpu_dev = cpu_dev;
> -	priv->cpu_reg = cpu_reg;
>  	policy->driver_data = priv;
> -
>  	policy->clk = cpu_clk;

Maybe we can have an dev_pm_opp_get_rate() API and a
cpufreq_generic_opp_get() so we can get rid of policy->clk usage
in this driver?

>  
>  	rcu_read_lock();
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 05/17] PM / OPP: Introduce dev_pm_opp_get_max_transition_latency()

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> In few use cases (like: cpufreq), it is desired to get the maximum
> latency for changing OPPs. Add support for that.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 01/17] PM / OPP: get/put regulators from OPP core

[Viresh Kumar]

On 11-01-16, 15:21, Stephen Boyd wrote:
> Is there a reason we capitalize regulator?

-------------------------8<-------------------------

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index cf351d3dab1c..9e437416e155 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -19,6 +19,7 @@
 #include <linux/device.h>
 #include <linux/of.h>
 #include <linux/export.h>
+#include <linux/regulator/consumer.h>
 
 #include "opp.h"
 
@@ -505,6 +506,7 @@ static struct device_opp *_add_device_opp(struct device *dev)
 {
 	struct device_opp *dev_opp;
 	struct device_list_opp *list_dev;
+	const char *name = dev_name(dev);
 
 	/* Check for existing list for 'dev' first */
 	dev_opp = _find_device_opp(dev);
@@ -527,6 +529,11 @@ static struct device_opp *_add_device_opp(struct device *dev)
 		return NULL;
 	}
 
+	dev_opp->regulator = regulator_get_optional(dev, name);
+	if (IS_ERR(dev_opp->regulator))
+		dev_info(dev, "%s: no regulator (%s) found: %ld\n", __func__,
+			 name, PTR_ERR(dev_opp->regulator));
+
 	srcu_init_notifier_head(&dev_opp->srcu_head);
 	INIT_LIST_HEAD(&dev_opp->opp_list);
 
@@ -565,6 +572,8 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (dev_opp->prop_name)
 		return;
 
+	regulator_put(dev_opp->regulator);
+
 	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
 				    node);
 
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 690638ef36ee..b9555f28f216 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -132,6 +132,8 @@ struct device_list_opp {
  * @supported_hw: Array of version number to support.
  * @supported_hw_count: Number of elements in supported_hw array.
  * @prop_name: A name to postfix to many DT properties, while parsing them.
+ * @regulator: Supply Regulator
+ *
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
@@ -159,6 +161,7 @@ struct device_opp {
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
 	const char *prop_name;
+	struct regulator *regulator;
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
-- 
viresh

Re: [PATCH 02/17] PM / OPP: Add APIs to set regulator-name

[Viresh Kumar]

On 11-01-16, 17:18, Stephen Boyd wrote:
> On 12/22, Viresh Kumar wrote:
> > This is required mostly for backward compatibility of DT users. The OPP
> > layer automatically gets the regulator device currently, but the name of
> > the device needs to be same as that of the device. But existing DT
> 
> The name of the device needs to be the same as that of the
> device? What does that mean? Perhaps this should say the name of
> the regulator/supply needs to be the same as that of the device?
>
> The same words are in the kernel doc too.

Absolutely.

> > +/**
> > + * dev_pm_opp_set_regulator() - Set regulator name for the device
> > + * @dev: Device for which regulator name is being set.
> > + * @name: Name of the regulator.
> > + *
> > + * This is required mostly for backward compatibility of DT users. The OPP layer
> > + * automatically gets the regulator device currently, but the name of the device
> > + * needs to be same as that of the device. But existing DT entries may not be
> > + * following that and might have used generic names instead. For example, they
> > + * might have used 'cpu-supply' instead of 'cpu0-supply'.
> 
> The new v2 OPP bindings are using cpu-supply instead of
> cpu0-supply.

I will say the examples are written that way, instead of the bindings.
And I will update the bindings doc to update that. Though we will keep
supporting cpu-supply (for backward compatibility), but
<devname>-supply is the way forward I believe. We can't really get the
device's generic name to use here for all kind of devices. Remember it
is not just about CPUs and so I went for it.

I will add another patch to update the bindings first and then rest of
the stuff, if that looks fine to you.

> This makes it sound like cpu-supply is the old style
> and we've added this API to handle it, when in reality, this is
> the preferred way to do this and so we've added this API because
> we almost always need it.

> > +int dev_pm_opp_set_regulator(struct device *dev, const char *name)
> > +{
> > +	struct device_opp *dev_opp;
> > +	struct regulator *reg;
> > +	int ret = 0;
> > +
> > +	/* Hold our list modification lock here */
> > +	mutex_lock(&dev_opp_list_lock);
> > +
> > +	dev_opp = _find_device_opp(dev);
> > +	/* We already have a dev-opp */
> > +	if (!IS_ERR(dev_opp)) {
> > +		/* This should be called before OPPs are initialized */
> > +		if (WARN_ON(!list_empty(&dev_opp->opp_list))) {
> > +			ret = -EBUSY;
> > +			goto unlock;
> > +		}
> > +
> > +		/* Already have a regulator set? Free it and re-allocate */
> > +		if (!IS_ERR(dev_opp->regulator))
> > +			regulator_put(dev_opp->regulator);
> 
> Is this used in practice? It seems odd that users would be using
> one regulator, and then change that to another regulator later.

Its required in the following possible scenario:
- The CPU0 DT node has two supplies, cpu-supply and cpu0-supply
- The dev_opp is added prior to this routine is called, which will
  actually allocate the 'cpu0-supply'.
- But the platform wanted to use cpu-supply instead ..

Yeah, this is kind of a corner case, but I think it is required.

> > +void dev_pm_opp_put_regulator(struct device *dev)
> > +{
> > +	struct device_opp *dev_opp;
> > +
> > +	/* Hold our list modification lock here */
> > +	mutex_lock(&dev_opp_list_lock);
> > +
> > +	/* Check for existing list for 'dev' first */
> > +	dev_opp = _find_device_opp(dev);
> > +	if (IS_ERR(dev_opp)) {
> > +		dev_err(dev, "Failed to find dev_opp: %ld\n", PTR_ERR(dev_opp));
> > +		goto unlock;
> > +	}
> > +
> > +	/* Make sure there are no concurrent readers while updating dev_opp */
> > +	WARN_ON(!list_empty(&dev_opp->opp_list));
> > +
> > +	if (!dev_opp->regulator_set) {
> > +		dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
> > +		goto unlock;
> > +	}
> > +
> > +	dev_opp->regulator_set = false;
> > +
> > +	/* Try freeing device_opp if this was the last blocking resource */
> > +	_remove_device_opp(dev_opp);
> 
> It looks like this whole function exists to set the boolean to
> false so that _remove_device_opp() can continue. What's the point
> of that? From what I can tell all we're getting is symmetry in
> the API by having this function, so why have this function at
> all?

No. Consider this case:
- Platform code sets regulator for cpuX
- insmod cpufreq-dt.ko
- rmmod cpufreq-dt.ko
- insmod cpufreq-dt.ko

If we don't have that boolean and the put_regulator routine, the
second insmod wouldn't work.
-------------------------8<-------------------------

Subject: [PATCH] PM / OPP: Add APIs to set regulator-name

This is required mostly for backward compatibility of DT users. The OPP
layer automatically gets the regulator device currently, but the name of
the regulator needs to be same as that of the device. But existing DT
entries may not be following that and might have used generic names
instead. For example, they might have used 'cpu-supply' instead of
'cpu0-supply'.

The APIs allow such platforms to pass a supply 'name' to OPP core, so
that the correct regulator supply can be allocated by the OPP core.

This must be called before any OPPs are initialized for the device.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 161 +++++++++++++++++++++++++++++++++++++-----
 drivers/base/power/opp/opp.h  |   2 +
 include/linux/pm_opp.h        |   9 +++
 3 files changed, 156 insertions(+), 16 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 9e437416e155..2b9663d573b0 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -493,25 +493,11 @@ struct device_list_opp *_add_list_dev(const struct device *dev,
 	return list_dev;
 }
 
-/**
- * _add_device_opp() - Find device OPP table or allocate a new one
- * @dev:	device for which we do this operation
- *
- * It tries to find an existing table first, if it couldn't find one, it
- * allocates a new OPP table and returns that.
- *
- * Return: valid device_opp pointer if success, else NULL.
- */
-static struct device_opp *_add_device_opp(struct device *dev)
+static struct device_opp *_add_device_opp_reg(struct device *dev,
+					      const char *name)
 {
 	struct device_opp *dev_opp;
 	struct device_list_opp *list_dev;
-	const char *name = dev_name(dev);
-
-	/* Check for existing list for 'dev' first */
-	dev_opp = _find_device_opp(dev);
-	if (!IS_ERR(dev_opp))
-		return dev_opp;
 
 	/*
 	 * Allocate a new device OPP table. In the infrequent case where a new
@@ -543,6 +529,27 @@ static struct device_opp *_add_device_opp(struct device *dev)
 }
 
 /**
+ * _add_device_opp() - Find device OPP table or allocate a new one
+ * @dev: device for which we do this operation
+ *
+ * It tries to find an existing table first, if it couldn't find one, it
+ * allocates a new OPP table and returns that.
+ *
+ * Return: valid device_opp pointer if success, else NULL.
+ */
+static struct device_opp *_add_device_opp(struct device *dev)
+{
+	struct device_opp *dev_opp;
+
+	/* Check for existing list for 'dev' first */
+	dev_opp = _find_device_opp(dev);
+	if (!IS_ERR(dev_opp))
+		return dev_opp;
+
+	return _add_device_opp_reg(dev, dev_name(dev));
+}
+
+/**
  * _kfree_device_rcu() - Free device_opp RCU handler
  * @head:	RCU head
  */
@@ -572,6 +579,9 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (dev_opp->prop_name)
 		return;
 
+	if (dev_opp->regulator_set)
+		return;
+
 	regulator_put(dev_opp->regulator);
 
 	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
@@ -1094,6 +1104,125 @@ void dev_pm_opp_put_prop_name(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
 
+/**
+ * dev_pm_opp_set_regulator() - Set regulator name for the device
+ * @dev: Device for which regulator name is being set.
+ * @name: Name of the regulator.
+ *
+ * This is required mostly for backward compatibility of DT users. The OPP layer
+ * automatically gets the regulator device currently, but the name of the
+ * regulator needs to be same as that of the device. But existing DT entries may
+ * not be following that and might have used generic names instead. For example,
+ * they might have used 'cpu-supply' instead of 'cpu0-supply'.
+ *
+ * This must be called before any OPPs are initialized for the device.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ */
+int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+{
+	struct device_opp *dev_opp;
+	struct regulator *reg;
+	int ret = 0;
+
+	mutex_lock(&dev_opp_list_lock);
+
+	dev_opp = _find_device_opp(dev);
+	/* We already have a dev-opp */
+	if (!IS_ERR(dev_opp)) {
+		/* This should be called before OPPs are initialized */
+		if (WARN_ON(!list_empty(&dev_opp->opp_list))) {
+			ret = -EBUSY;
+			goto unlock;
+		}
+
+		/* Already have a regulator set? Free it and re-allocate */
+		if (!IS_ERR(dev_opp->regulator))
+			regulator_put(dev_opp->regulator);
+
+		/* Allocate the regulator */
+		reg = regulator_get_optional(dev, name);
+		if (IS_ERR(reg)) {
+			ret = PTR_ERR(reg);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "%s: no regulator (%s) found: %d\n",
+					__func__, name, ret);
+		}
+
+		dev_opp->regulator = reg;
+		goto unlock;
+	}
+
+	dev_opp = _add_device_opp_reg(dev, name);
+	if (!dev_opp) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	reg = dev_opp->regulator;
+	if (IS_ERR(reg)) {
+		ret = PTR_ERR(reg);
+		_remove_device_opp(dev_opp);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "%s: no regulator (%s) found: %d\n",
+				__func__, name, ret);
+	}
+
+unlock:
+	if (!ret)
+		dev_opp->regulator_set = true;
+
+	mutex_unlock(&dev_opp_list_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
+
+/**
+ * dev_pm_opp_put_regulator() - Releases resources blocked for regulator
+ * @dev: Device for which regulator was set.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ */
+void dev_pm_opp_put_regulator(struct device *dev)
+{
+	struct device_opp *dev_opp;
+
+	mutex_lock(&dev_opp_list_lock);
+
+	/* Check for existing list for 'dev' first */
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp)) {
+		dev_err(dev, "Failed to find dev_opp: %ld\n", PTR_ERR(dev_opp));
+		goto unlock;
+	}
+
+	/* Make sure there are no concurrent readers while updating dev_opp */
+	WARN_ON(!list_empty(&dev_opp->opp_list));
+
+	if (!dev_opp->regulator_set) {
+		dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
+		goto unlock;
+	}
+
+	dev_opp->regulator_set = false;
+
+	/* Try freeing device_opp if this was the last blocking resource */
+	_remove_device_opp(dev_opp);
+
+unlock:
+	mutex_unlock(&dev_opp_list_lock);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulator);
+
 static bool _opp_is_supported(struct device *dev, struct device_opp *dev_opp,
 			      struct device_node *np)
 {
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index b9555f28f216..aca423caebf3 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -133,6 +133,7 @@ struct device_list_opp {
  * @supported_hw_count: Number of elements in supported_hw array.
  * @prop_name: A name to postfix to many DT properties, while parsing them.
  * @regulator: Supply Regulator
+ * @regulator_set: Regulator's name is explicitly set by platform.
  *
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
@@ -162,6 +163,7 @@ struct device_opp {
 	unsigned int supported_hw_count;
 	const char *prop_name;
 	struct regulator *regulator;
+	bool regulator_set;
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 95403d2ccaf5..c70a18ac9c8a 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -60,6 +60,8 @@ int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
 void dev_pm_opp_put_supported_hw(struct device *dev);
 int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
 void dev_pm_opp_put_prop_name(struct device *dev);
+int dev_pm_opp_set_regulator(struct device *dev, const char *name);
+void dev_pm_opp_put_regulator(struct device *dev);
 #else
 static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 {
@@ -151,6 +153,13 @@ static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
 
 static inline void dev_pm_opp_put_prop_name(struct device *dev) {}
 
+static inline int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+{
+	return -EINVAL;
+}
+
+static inline void dev_pm_opp_put_regulator(struct device *dev) {}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)


-- 
viresh

Re: [PATCH 04/17] PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()

[Viresh Kumar]

On 11-01-16, 17:25, Stephen Boyd wrote:
> Doesn't rcu_read_lock() disable preemption sometimes? I don't
> think we can call regulator ops with preemption disabled because
> regulators use mutex locking. It looks like
> regulator_list_voltage() may take a mutex anyway.

-------------------------8<-------------------------

Subject: [PATCH] PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()

In few use cases (like: cpufreq), it is desired to get the maximum
voltage latency for changing OPPs. Add support for that.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 56 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/pm_opp.h        |  6 +++++
 2 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 052fc6b78dc3..62976d0bd61c 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -231,8 +231,62 @@ unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev)
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_clock_latency);
 
 /**
+ * dev_pm_opp_get_max_volt_latency() - Get max voltage latency in nanoseconds
+ * @dev: device for which we do this operation
+ *
+ * Return: This function returns the max voltage latency in nanoseconds.
+ *
+ * Locking: This function takes rcu_read_lock().
+ */
+unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
+{
+	struct device_opp *dev_opp;
+	struct dev_pm_opp *opp;
+	struct regulator *reg;
+	unsigned long latency_ns = 0;
+	unsigned long min_uV = ~0, max_uV = 0;
+	int ret;
+
+	/*
+	 * Hold our list modification lock here as regulator_set_voltage_time()
+	 * can possibly take another mutex, which isn't allowed within rcu
+	 * locks.
+	 */
+	mutex_lock(&dev_opp_list_lock);
+
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp))
+		goto unlock;
+
+	reg = dev_opp->regulator;
+	/* Regulator may not be available for device */
+	if (IS_ERR(reg))
+		goto unlock;
+
+	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
+		if (!opp->available)
+			continue;
+
+		if (opp->u_volt_min < min_uV)
+			min_uV = opp->u_volt_min;
+		if (opp->u_volt_max > max_uV)
+			max_uV = opp->u_volt_max;
+	}
+
+	ret = regulator_set_voltage_time(reg, min_uV, max_uV);
+	if (ret > 0)
+		latency_ns = ret * 1000;
+
+unlock:
+	mutex_unlock(&dev_opp_list_lock);
+
+	return latency_ns;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_volt_latency);
+
+/**
  * dev_pm_opp_get_suspend_opp() - Get suspend opp
- * @dev:	device for which we do this operation
+ * @dev: device for which we do this operation
  *
  * Return: This function returns pointer to the suspend opp if it is
  * defined and available, otherwise it returns NULL.
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index c70a18ac9c8a..5daa43058ac1 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -34,6 +34,7 @@ bool dev_pm_opp_is_turbo(struct dev_pm_opp *opp);
 
 int dev_pm_opp_get_opp_count(struct device *dev);
 unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev);
+unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev);
 struct dev_pm_opp *dev_pm_opp_get_suspend_opp(struct device *dev);
 
 struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
@@ -88,6 +89,11 @@ static inline unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev)
 	return 0;
 }
 
+static inline unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
+{
+	return 0;
+}
+
 static inline struct dev_pm_opp *dev_pm_opp_get_suspend_opp(struct device *dev)
 {
 	return NULL;

-- 
viresh

Re: [PATCH 06/17] PM / OPP: Parse clock-latency and voltage-tolerance for v1 bindings

[Viresh Kumar]

On 11-01-16, 17:29, Stephen Boyd wrote:
> On 12/22, Viresh Kumar wrote:
> > @@ -580,6 +581,21 @@ static struct device_opp *_add_device_opp_reg(struct device *dev,
> >  		return NULL;
> >  	}
> >  
> > +	/*
> > +	 * 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;
> > +
> > +		if (!of_property_read_u32(np, "clock-latency", &val))
> > +			dev_opp->clock_latency_ns_max = val;

This is u64 type variable, but we are reading a 32 bit value from DT
and so wrote it that way.

> > +		of_property_read_u32(np, "voltage-tolerance",
> > +				     &dev_opp->voltage_tolerance_v1);

And this is u32 type.

> Why do we conditionalize the assignment for clock latency but not
> for voltage tolerance?

Nothing more than what I described earlier.

-- 
viresh

Re: [PATCH 01/17] PM / OPP: get/put regulators from OPP core

[Viresh Kumar]

On 12-01-16, 08:35, Viresh Kumar wrote:
> On 11-01-16, 15:21, Stephen Boyd wrote:
> > Is there a reason we capitalize regulator?
> 

Something went wrong, I have updated the code correctly. Let me paste
it again..

-------------------------8<-------------------------

Subject: [PATCH] PM / OPP: get/put regulators from OPP core

The allows the OPP core to request/free the regulator resource, attached
to a device OPP. The regulator device is fetched using device node and
name of the device.

For example, a cpu0 device node needs to look like this:
		cpu0: cpu@900 {
			device_type = "cpu";
			compatible = "arm,cortex-a9";
			reg = <0x900>;
			cpu0-supply = <&varm>;
			...
		};

This will work for both OPP-v1 and v2 bindings.

This is a preliminary step in moving the OPP switching logic into the
OPP core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 9 +++++++++
 drivers/base/power/opp/opp.h  | 4 ++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index cf351d3dab1c..9e437416e155 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -19,6 +19,7 @@
 #include <linux/device.h>
 #include <linux/of.h>
 #include <linux/export.h>
+#include <linux/regulator/consumer.h>
 
 #include "opp.h"
 
@@ -505,6 +506,7 @@ static struct device_opp *_add_device_opp(struct device *dev)
 {
 	struct device_opp *dev_opp;
 	struct device_list_opp *list_dev;
+	const char *name = dev_name(dev);
 
 	/* Check for existing list for 'dev' first */
 	dev_opp = _find_device_opp(dev);
@@ -527,6 +529,11 @@ static struct device_opp *_add_device_opp(struct device *dev)
 		return NULL;
 	}
 
+	dev_opp->regulator = regulator_get_optional(dev, name);
+	if (IS_ERR(dev_opp->regulator))
+		dev_info(dev, "%s: no regulator (%s) found: %ld\n", __func__,
+			 name, PTR_ERR(dev_opp->regulator));
+
 	srcu_init_notifier_head(&dev_opp->srcu_head);
 	INIT_LIST_HEAD(&dev_opp->opp_list);
 
@@ -565,6 +572,8 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (dev_opp->prop_name)
 		return;
 
+	regulator_put(dev_opp->regulator);
+
 	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
 				    node);
 
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 690638ef36ee..416293b7da23 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -22,6 +22,8 @@
 #include <linux/rculist.h>
 #include <linux/rcupdate.h>
 
+struct regulator;
+
 /* Lock to allow exclusive modification to the device and opp lists */
 extern struct mutex dev_opp_list_lock;
 
@@ -132,6 +134,7 @@ struct device_list_opp {
  * @supported_hw: Array of version number to support.
  * @supported_hw_count: Number of elements in supported_hw array.
  * @prop_name: A name to postfix to many DT properties, while parsing them.
+ * @regulator: Supply regulator
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
@@ -159,6 +162,7 @@ struct device_opp {
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
 	const char *prop_name;
+	struct regulator *regulator;
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
-- 
viresh

Re: [PATCH 07/17] PM / OPP: Manage device clk

[Viresh Kumar]

On 11-01-16, 17:32, Stephen Boyd wrote:
> > + * @clk:	struct clk pointer
> 
> Why tab? And perhaps it should say "clock handle" or "supply
> clock" or something instead of rewording the type "struct clk *".

-------------------------8<-------------------------

Subject: [PATCH] PM / OPP: Manage device clk

OPP core has got almost everything now to manage device's OPP
transitions, the only thing left is device's clk. Get that as well.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 15 +++++++++++++++
 drivers/base/power/opp/opp.h  |  3 +++
 2 files changed, 18 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 72d9fdf9ff0c..e7c9ae0c4c09 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/clk.h>
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/slab.h>
@@ -570,6 +571,7 @@ static struct device_opp *_add_device_opp_reg(struct device *dev,
 	struct device_opp *dev_opp;
 	struct device_list_opp *list_dev;
 	struct device_node *np;
+	int ret;
 
 	/*
 	 * Allocate a new device OPP table. In the infrequent case where a new
@@ -602,6 +604,15 @@ static struct device_opp *_add_device_opp_reg(struct device *dev,
 		of_node_put(np);
 	}
 
+	/* Find clk for the device */
+	dev_opp->clk = clk_get(dev, NULL);
+	if (IS_ERR(dev_opp->clk)) {
+		ret = PTR_ERR(dev_opp->clk);
+		if (ret != -EPROBE_DEFER)
+			dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__,
+				ret);
+	}
+
 	dev_opp->regulator = regulator_get_optional(dev, name);
 	if (IS_ERR(dev_opp->regulator))
 		dev_info(dev, "%s: no regulator (%s) found: %ld\n", __func__,
@@ -669,6 +680,10 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (dev_opp->regulator_set)
 		return;
 
+	/* Release clk */
+	if (!IS_ERR(dev_opp->clk))
+		clk_put(dev_opp->clk);
+
 	regulator_put(dev_opp->regulator);
 
 	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index d45570458f89..777ec70488bc 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -22,6 +22,7 @@
 #include <linux/rculist.h>
 #include <linux/rcupdate.h>
 
+struct clk;
 struct regulator;
 
 /* Lock to allow exclusive modification to the device and opp lists */
@@ -134,6 +135,7 @@ struct device_list_opp {
  * @supported_hw: Array of version number to support.
  * @supported_hw_count: Number of elements in supported_hw array.
  * @prop_name: A name to postfix to many DT properties, while parsing them.
+ * @clk: Device's clock handle
  * @regulator: Supply regulator
  * @regulator_set: Regulator's name is explicitly set by platform.
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
@@ -169,6 +171,7 @@ struct device_opp {
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
 	const char *prop_name;
+	struct clk *clk;
 	struct regulator *regulator;
 	bool regulator_set;
 
-- 
viresh

Re: [PATCH 08/17] PM / OPP: Add dev_pm_opp_set_rate()

[Viresh Kumar]

On 11-01-16, 17:40, Stephen Boyd wrote:
> > +	ret = regulator_set_voltage_triplet(reg, opp->u_volt_min, opp->u_volt,
> 
> This can't be called under an RCU read lock.
> 

> > +	rcu_read_lock();
> 
> This lock is held way too long.
> 

> > +	freq = clk_round_rate(clk, target_freq);
> 
> This can't be called under RCU.
> 
> > +	if ((long)freq <= 0)
> > +		freq = target_freq;
> > +
> > +	old_freq = clk_get_rate(clk);
> 
> Same for this.

Some of these I figured out earlier (after sending the series), and
fixed them by dropping rcu lock at certain points. But the problem is
that we will be using opp/clk/reg here for almost the complete routine
and the lock must be acquired for all the time.

(Untested for now)

-------------------------8<-------------------------

Subject: [PATCH] PM / OPP: Add dev_pm_opp_set_rate()

This adds a routine, dev_pm_opp_set_rate(), responsible for configuring
power-supply and clock source for an OPP.

The OPP is found by matching against the target_freq passed to the
routine. This shall replace similar code present in most of the OPP
users and help simplify them a lot.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 155 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h        |   6 ++
 2 files changed, 161 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 2b1e8cb3897f..ca6b31ab5c87 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -527,6 +527,161 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
+static int _set_opp_voltage(struct device *dev, struct regulator *reg,
+			    unsigned long u_volt, unsigned long u_volt_min,
+			    unsigned long u_volt_max)
+{
+	int ret;
+
+	/* Regulator not be available for device */
+	if (IS_ERR(reg)) {
+		dev_dbg(dev, "%s: regulator not available: %ld\n", __func__,
+			PTR_ERR(reg));
+		return 0;
+	}
+
+	dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, u_volt_min,
+		u_volt, u_volt_max);
+
+	ret = regulator_set_voltage_triplet(reg, u_volt_min, u_volt,
+					    u_volt_max);
+	if (ret)
+		dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n",
+			__func__, u_volt_min, u_volt, u_volt_max, ret);
+
+	return ret;
+}
+
+/**
+ * dev_pm_opp_set_rate() - Configure new OPP based on frequency
+ * @dev:	 device for which we do this operation
+ * @target_freq: frequency to achieve
+ *
+ * This configures the power-supplies and clock source to the levels specified
+ * by the OPP corresponding to the target_freq.
+ *
+ * Locking: This function internally uses mutex locks to keep the integrity of
+ * the internal data structures. Callers should ensure that this function is
+ * *NOT* called under RCU protection or in contexts where mutex cannot be
+ * locked.
+ */
+int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
+{
+	struct device_opp *dev_opp;
+	struct dev_pm_opp *old_opp, *opp;
+	struct regulator *reg;
+	struct clk *clk;
+	unsigned long freq, old_freq;
+	unsigned long u_volt, u_volt_min, u_volt_max;
+	unsigned long ou_volt, ou_volt_min, ou_volt_max;
+	int ret;
+
+	if (unlikely(!target_freq)) {
+		dev_err(dev, "%s: Invalid target frequency %lu\n", __func__,
+			target_freq);
+		return -EINVAL;
+	}
+
+	/*
+	 * Hold our list modification lock here as clk/regulator routines called
+	 * below can possibly take another mutex, which isn't allowed within rcu
+	 * locks.
+	 */
+	mutex_lock(&dev_opp_list_lock);
+
+	dev_opp = _find_device_opp(dev);
+	if (IS_ERR(dev_opp)) {
+		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
+		ret = PTR_ERR(dev_opp);
+		goto unlock;
+	}
+
+	clk = dev_opp->clk;
+	if (IS_ERR(clk)) {
+		dev_err(dev, "%s: No clock available for the device\n",
+			__func__);
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	freq = clk_round_rate(clk, target_freq);
+	if ((long)freq <= 0)
+		freq = target_freq;
+
+	old_freq = clk_get_rate(clk);
+
+	/* Return early if nothing to do */
+	if (old_freq == freq) {
+		dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
+			__func__, freq);
+		ret = 0;
+		goto unlock;
+	}
+
+	reg = dev_opp->regulator;
+	old_opp = dev_pm_opp_find_freq_ceil(dev, &old_freq);
+	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+
+	if (IS_ERR(opp)) {
+		ret = PTR_ERR(opp);
+		dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
+			__func__, freq, ret);
+		goto unlock;
+	}
+
+	u_volt = opp->u_volt;
+	u_volt_min = opp->u_volt_min;
+	u_volt_max = opp->u_volt_max;
+
+	ou_volt = old_opp->u_volt;
+	ou_volt_min = old_opp->u_volt_min;
+	ou_volt_max = old_opp->u_volt_max;
+
+	/* Scaling up? Scale voltage before frequency */
+	if (freq > old_freq) {
+		ret = _set_opp_voltage(dev, reg, u_volt, u_volt_min,
+				       u_volt_max);
+		if (ret)
+			goto restore_voltage;
+	}
+
+	/* Change frequency */
+
+	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",
+		__func__, old_freq, freq);
+
+	ret = clk_set_rate(clk, freq);
+	if (ret) {
+		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
+			ret);
+		goto restore_voltage;
+	}
+
+	/* Scaling down? Scale voltage after frequency */
+	if (freq < old_freq) {
+		ret = _set_opp_voltage(dev, reg, u_volt, u_volt_min,
+				       u_volt_max);
+		if (ret)
+			goto restore_freq;
+	}
+
+	mutex_unlock(&dev_opp_list_lock);
+
+	return 0;
+
+restore_freq:
+	if (clk_set_rate(clk, old_freq))
+		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
+			__func__, old_freq);
+restore_voltage:
+	/* This shouldn't harm if the voltages weren't updated earlier */
+	_set_opp_voltage(dev, reg, ou_volt, ou_volt_min, ou_volt_max);
+unlock:
+	mutex_unlock(&dev_opp_list_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
+
 /* List-dev Helpers */
 static void _kfree_list_dev_rcu(struct rcu_head *head)
 {
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 59da3d9e11ea..cccaf4a29e9f 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -64,6 +64,7 @@ int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
 void dev_pm_opp_put_prop_name(struct device *dev);
 int dev_pm_opp_set_regulator(struct device *dev, const char *name);
 void dev_pm_opp_put_regulator(struct device *dev);
+int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 #else
 static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 {
@@ -172,6 +173,11 @@ static inline int dev_pm_opp_set_regulator(struct device *dev, const char *name)
 
 static inline void dev_pm_opp_put_regulator(struct device *dev) {}
 
+static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
+{
+	return -EINVAL;
+}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)


-- 
viresh

Re: [PATCH 12/17] cpufreq: dt: Pass regulator name to the OPP core for V1 bindings

[Viresh Kumar]

On 11-01-16, 17:53, Stephen Boyd wrote:
> On 12/22, Viresh Kumar wrote:
> > OPP core can handle the regulators by itself, and it allocates the
> > regulator based on device's name. But for older V1 bindings, many DT
> > files have used names like 'cpu-supply' instead of 'cpu0-supply'.
> > 
> > The cpufreq-dt driver needs to tell the right name of the regulator in
> > this case to the OPP core.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> This whole patch is confusing to me because old style was to be
> cpu0-supply, and new style is cpu-supply.

Long back we used to have cpu0-supply, then while I converted
cpufreq-dt to support multiple clusters, you asked me to name it to
cpu-supply, which I did.

Now, looking at the implementation into the generic OPP layer, it
looks like <device>-name is a far better and reasonable choice.

And so I am moving back to cpu0-supply, will udpate binding doc as
well.

-- 
viresh

Re: [PATCH 17/17] cpufreq: dt: No need to allocate resources anymore

[Viresh Kumar]

On 11-01-16, 18:20, Stephen Boyd wrote:
> On 12/22, Viresh Kumar wrote:
> > OPP layer manages it now and cpufreq-dt driver doesn't need it. But, we
> > still need to check for availability of resources for deferred probing.
> 
> Why? It seems cleaner to let OPP layer return an error indicating
> probe defer or failure when we try to initialize it. That way we
> aren't duplicating the same logic in two places to figure out if
> a regulator or clock is ready.

cpufreq driver's ->init() callback doesn't return the error value
properly to the probe() function, and so it was done this way in the
first place. The problem is in subsys framework. I tried to fix it but
it was rejected and we need to fix it some other way:

http://marc.info/?l=linux-kernel&m=143530948918819&w=2

> >  	policy->clk = cpu_clk;
> 
> Maybe we can have an dev_pm_opp_get_rate() API and a
> cpufreq_generic_opp_get() so we can get rid of policy->clk usage
> in this driver?

Okay, will do.

-- 
viresh

Re: [PATCH 01/17] PM / OPP: get/put regulators from OPP core

[Stephen Boyd]

On 01/12, Viresh Kumar wrote:
> On 12-01-16, 08:35, Viresh Kumar wrote:
> > On 11-01-16, 15:21, Stephen Boyd wrote:
> > > Is there a reason we capitalize regulator?
> > 
> 
> Something went wrong, I have updated the code correctly. Let me paste
> it again..
> 
> -------------------------8<-------------------------
> 
> Subject: [PATCH] PM / OPP: get/put regulators from OPP core
> 
> The allows the OPP core to request/free the regulator resource, attached
> to a device OPP. The regulator device is fetched using device node and
> name of the device.
> 
> For example, a cpu0 device node needs to look like this:
> 		cpu0: cpu@900 {
> 			device_type = "cpu";
> 			compatible = "arm,cortex-a9";
> 			reg = <0x900>;
> 			cpu0-supply = <&varm>;
> 			...
> 		};
> 
> This will work for both OPP-v1 and v2 bindings.
> 
> This is a preliminary step in moving the OPP switching logic into the
> OPP core.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 04/17] PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()

[Stephen Boyd]

On 01/12, Viresh Kumar wrote:
> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index 052fc6b78dc3..62976d0bd61c 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -231,8 +231,62 @@ unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev)
>  EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_clock_latency);
>  
>  /**
> + * dev_pm_opp_get_max_volt_latency() - Get max voltage latency in nanoseconds
> + * @dev: device for which we do this operation
> + *
> + * Return: This function returns the max voltage latency in nanoseconds.
> + *
> + * Locking: This function takes rcu_read_lock().

False.

> + */
> +unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
> +{
> +	struct device_opp *dev_opp;
> +	struct dev_pm_opp *opp;
> +	struct regulator *reg;
> +	unsigned long latency_ns = 0;
> +	unsigned long min_uV = ~0, max_uV = 0;
> +	int ret;
> +
> +	/*
> +	 * Hold our list modification lock here as regulator_set_voltage_time()
> +	 * can possibly take another mutex, which isn't allowed within rcu
> +	 * locks.
> +	 */
> +	mutex_lock(&dev_opp_list_lock);

So now we take the list modification mutex. Why can't we
rcu_read_lock(), find the min and max voltage, and then release
the read lock and ask regulator framework for the voltage time?
>From what I can tell we're just trying to make sure that the list
is stable when iterating through it to find the min/max voltage.

> +
> +	dev_opp = _find_device_opp(dev);
> +	if (IS_ERR(dev_opp))
> +		goto unlock;
> +
> +	reg = dev_opp->regulator;
> +	/* Regulator may not be available for device */
> +	if (IS_ERR(reg))
> +		goto unlock;
> +
> +	list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) {
> +		if (!opp->available)
> +			continue;
> +
> +		if (opp->u_volt_min < min_uV)
> +			min_uV = opp->u_volt_min;
> +		if (opp->u_volt_max > max_uV)
> +			max_uV = opp->u_volt_max;
> +	}
> +
> +	ret = regulator_set_voltage_time(reg, min_uV, max_uV);
> +	if (ret > 0)
> +		latency_ns = ret * 1000;
> +
> +unlock:
> +	mutex_unlock(&dev_opp_list_lock);
> +
> +	return latency_ns;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_volt_latency);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 07/17] PM / OPP: Manage device clk

[Stephen Boyd]

On 01/12, Viresh Kumar wrote:
> Subject: [PATCH] PM / OPP: Manage device clk
> 
> OPP core has got almost everything now to manage device's OPP
> transitions, the only thing left is device's clk. Get that as well.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 06/17] PM / OPP: Parse clock-latency and voltage-tolerance for v1 bindings

[Stephen Boyd]

On 01/12, Viresh Kumar wrote:
> On 11-01-16, 17:29, Stephen Boyd wrote:
> > On 12/22, Viresh Kumar wrote:
> > > @@ -580,6 +581,21 @@ static struct device_opp *_add_device_opp_reg(struct device *dev,
> > >  		return NULL;
> > >  	}
> > >  
> > > +	/*
> > > +	 * 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;
> > > +
> > > +		if (!of_property_read_u32(np, "clock-latency", &val))
> > > +			dev_opp->clock_latency_ns_max = val;
> 
> This is u64 type variable, but we are reading a 32 bit value from DT
> and so wrote it that way.
> 
> > > +		of_property_read_u32(np, "voltage-tolerance",
> > > +				     &dev_opp->voltage_tolerance_v1);
> 
> And this is u32 type.
> 
> > Why do we conditionalize the assignment for clock latency but not
> > for voltage tolerance?
> 
> Nothing more than what I described earlier.
> 

Ok.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 12/17] cpufreq: dt: Pass regulator name to the OPP core for V1 bindings

[Stephen Boyd]

On 01/12, Viresh Kumar wrote:
> On 11-01-16, 17:53, Stephen Boyd wrote:
> > On 12/22, Viresh Kumar wrote:
> > > OPP core can handle the regulators by itself, and it allocates the
> > > regulator based on device's name. But for older V1 bindings, many DT
> > > files have used names like 'cpu-supply' instead of 'cpu0-supply'.
> > > 
> > > The cpufreq-dt driver needs to tell the right name of the regulator in
> > > this case to the OPP core.
> > > 
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > 
> > This whole patch is confusing to me because old style was to be
> > cpu0-supply, and new style is cpu-supply.
> 
> Long back we used to have cpu0-supply, then while I converted
> cpufreq-dt to support multiple clusters, you asked me to name it to
> cpu-supply, which I did.
> 
> Now, looking at the implementation into the generic OPP layer, it
> looks like <device>-name is a far better and reasonable choice.

It's far easier to implement, but not far better. In most designs
the pin is not called <device_name>-supply, but something more
mundane like vdd-supply, vddio-supply, vcc-supply, etc. In the
case of CPUs, there's probably nothing in the datasheets, so cpu
vs cpu0 is not too important to distinguish here. But for things
like a GPU, DSP, video encoder, etc. I doubt it's going to be
called <device_name>-supply, so making that the norm is
misguided.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 08/17] PM / OPP: Add dev_pm_opp_set_rate()

[Stephen Boyd]

On 01/12, Viresh Kumar wrote:
> 
> Some of these I figured out earlier (after sending the series), and
> fixed them by dropping rcu lock at certain points. But the problem is
> that we will be using opp/clk/reg here for almost the complete routine
> and the lock must be acquired for all the time.
> 
> (Untested for now)
[...]
> +int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> +{
> +	struct device_opp *dev_opp;
> +	struct dev_pm_opp *old_opp, *opp;
> +	struct regulator *reg;
> +	struct clk *clk;
> +	unsigned long freq, old_freq;
> +	unsigned long u_volt, u_volt_min, u_volt_max;
> +	unsigned long ou_volt, ou_volt_min, ou_volt_max;
> +	int ret;
> +
> +	if (unlikely(!target_freq)) {
> +		dev_err(dev, "%s: Invalid target frequency %lu\n", __func__,
> +			target_freq);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Hold our list modification lock here as clk/regulator routines called
> +	 * below can possibly take another mutex, which isn't allowed within rcu
> +	 * locks.
> +	 */
> +	mutex_lock(&dev_opp_list_lock);
> +

Nak

Having a single mutex around the clock and voltage setting
makes cpu frequency switching scalability drop to zero for
devices that have independent clock and voltage supplies for each
CPU. We need to get the voltage and frequency settings under rcu
and then release rcu and change the hardware. This is already how
cpufreq-dt is doing it anyway, so I'm lost how it can't be copied
here into OPP framework.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 04/17] PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()

[Viresh Kumar]

On 12-01-16, 11:45, Stephen Boyd wrote:
> > + * Locking: This function takes rcu_read_lock().
> 
> False.

Yeah, I realized it and fixed it right after sending it:

+ * Locking: This function internally uses mutex locks to keep the integrity of
+ * the internal data structures. Callers should ensure that this function is
+ * *NOT* called under RCU protection or in contexts where mutex cannot be
+ * locked.

> > +	/*
> > +	 * Hold our list modification lock here as regulator_set_voltage_time()
> > +	 * can possibly take another mutex, which isn't allowed within rcu
> > +	 * locks.
> > +	 */
> > +	mutex_lock(&dev_opp_list_lock);
> 
> So now we take the list modification mutex. Why can't we
> rcu_read_lock(), find the min and max voltage, and then release
> the read lock and ask regulator framework for the voltage time?

That was possible before this series came in..

> From what I can tell we're just trying to make sure that the list
> is stable when iterating through it to find the min/max voltage.

Hmm, we have pointer to regulator within the dev-opp struct. If we
drop the lock, the dev-opp struct can get freed and regulator will be
put just before that. So, we may be using an already freed resource.

How do you suggest we fix it ?

-- 
viresh

Re: [PATCH 06/17] PM / OPP: Parse clock-latency and voltage-tolerance for v1 bindings

[Viresh Kumar]

On 12-01-16, 16:36, Stephen Boyd wrote:
> On 01/12, Viresh Kumar wrote:
> > On 11-01-16, 17:29, Stephen Boyd wrote:
> > > On 12/22, Viresh Kumar wrote:
> > > > @@ -580,6 +581,21 @@ static struct device_opp *_add_device_opp_reg(struct device *dev,
> > > >  		return NULL;
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * 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;
> > > > +
> > > > +		if (!of_property_read_u32(np, "clock-latency", &val))
> > > > +			dev_opp->clock_latency_ns_max = val;
> > 
> > This is u64 type variable, but we are reading a 32 bit value from DT
> > and so wrote it that way.
> > 
> > > > +		of_property_read_u32(np, "voltage-tolerance",
> > > > +				     &dev_opp->voltage_tolerance_v1);
> > 
> > And this is u32 type.
> > 
> > > Why do we conditionalize the assignment for clock latency but not
> > > for voltage tolerance?
> > 
> > Nothing more than what I described earlier.
> > 
> 
> Ok.

Should I consider that an Reviewed-by ?

-- 
viresh

Re: [PATCH 12/17] cpufreq: dt: Pass regulator name to the OPP core for V1 bindings

[Viresh Kumar]

On 12-01-16, 16:43, Stephen Boyd wrote:
> It's far easier to implement, but not far better. In most designs
> the pin is not called <device_name>-supply, but something more
> mundane like vdd-supply, vddio-supply, vcc-supply, etc. In the
> case of CPUs, there's probably nothing in the datasheets, so cpu
> vs cpu0 is not too important to distinguish here. But for things
> like a GPU, DSP, video encoder, etc. I doubt it's going to be
> called <device_name>-supply, so making that the norm is
> misguided.

I completely agree with that, but here is the usecase:
- A OPP-user driver doesn't need to call any special OPP API and OPP
  layer can allocate the regulator for it using <device>-name. This
  will make all drivers (that follow this nomenclature in DT) very
  simple and straight-forward.
- But then there are drivers, which need special supply-name. They can
  always call opp-set-regulator API to mention that, no one is
  stopping them from that.

And so I still believe, OPP layer has only this option to do it
generically.

Comments ?

-- 
viresh

Re: [PATCH 08/17] PM / OPP: Add dev_pm_opp_set_rate()

[Viresh Kumar]

On 12-01-16, 16:49, Stephen Boyd wrote:
> Having a single mutex around the clock and voltage setting
> makes cpu frequency switching scalability drop to zero for
> devices that have independent clock and voltage supplies for each
> CPU. We need to get the voltage and frequency settings under rcu
> and then release rcu and change the hardware. This is already how
> cpufreq-dt is doing it anyway, so I'm lost how it can't be copied
> here into OPP framework.

Hmm. So what should we do about the clk/regulator issue I mentioned in
the other thread? How do we guarantee that the OPP doesn't get freed?

Should we implement a get/put regulator and clk within the OPP layer
using some sort of refcount ?

-- 
viresh

Re: [PATCH 12/17] cpufreq: dt: Pass regulator name to the OPP core for V1 bindings

[Mark Brown]

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

On Wed, Jan 13, 2016 at 11:17:50AM +0530, Viresh Kumar wrote:

> - A OPP-user driver doesn't need to call any special OPP API and OPP
>   layer can allocate the regulator for it using <device>-name. This
>   will make all drivers (that follow this nomenclature in DT) very
>   simple and straight-forward.

Viresh, as we've been through multiple times supplies should always be
requested with the name of the supply on the physical device.  Please
stop trying to create ABIs around whatever Linux internals you're
currently working with, this is getting repetitive.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 06/17] PM / OPP: Parse clock-latency and voltage-tolerance for v1 bindings

[Stephen Boyd]

On 12/22, Viresh Kumar wrote:
> V2 bindings have better support for clock-latency and voltage-tolerance
> and doesn't need special care. To use callbacks, like
> dev_pm_opp_get_max_{transition|volt}_latency(), irrespective of the
> bindings, the core needs to know clock-latency/voltage-tolerance for the
> earlier bindings.
> 
> This patch reads clock-latency/voltage-tolerance from the device node,
> irrespective of the bindings (to keep it simple) and use them only for
> V1 bindings.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 06/17] PM / OPP: Parse clock-latency and voltage-tolerance for v1 bindings

[Stephen Boyd]

On 01/13, Viresh Kumar wrote:
> On 12-01-16, 16:36, Stephen Boyd wrote:
> > On 01/12, Viresh Kumar wrote:
> > > 
> > > Nothing more than what I described earlier.
> > > 
> > 
> > Ok.
> 
> Should I consider that an Reviewed-by ?
> 

Yep.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 04/17] PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()

[Viresh Kumar]

On 13-01-16, 11:04, Viresh Kumar wrote:
> On 12-01-16, 11:45, Stephen Boyd wrote:
> > > + * Locking: This function takes rcu_read_lock().
> > 
> > False.
> 
> Yeah, I realized it and fixed it right after sending it:
> 
> + * Locking: This function internally uses mutex locks to keep the integrity of
> + * the internal data structures. Callers should ensure that this function is
> + * *NOT* called under RCU protection or in contexts where mutex cannot be
> + * locked.
> 
> > > +	/*
> > > +	 * Hold our list modification lock here as regulator_set_voltage_time()
> > > +	 * can possibly take another mutex, which isn't allowed within rcu
> > > +	 * locks.
> > > +	 */
> > > +	mutex_lock(&dev_opp_list_lock);
> > 
> > So now we take the list modification mutex. Why can't we
> > rcu_read_lock(), find the min and max voltage, and then release
> > the read lock and ask regulator framework for the voltage time?
> 
> That was possible before this series came in..
> 
> > From what I can tell we're just trying to make sure that the list
> > is stable when iterating through it to find the min/max voltage.
> 
> Hmm, we have pointer to regulator within the dev-opp struct. If we
> drop the lock, the dev-opp struct can get freed and regulator will be
> put just before that. So, we may be using an already freed resource.
> 
> How do you suggest we fix it ?

Ping..

-- 
viresh

Re: [PATCH 04/17] PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()

[Stephen Boyd]

On 01/13, Viresh Kumar wrote:
> On 12-01-16, 11:45, Stephen Boyd wrote:
> > > +	/*
> > > +	 * Hold our list modification lock here as regulator_set_voltage_time()
> > > +	 * can possibly take another mutex, which isn't allowed within rcu
> > > +	 * locks.
> > > +	 */
> > > +	mutex_lock(&dev_opp_list_lock);
> > 
> > So now we take the list modification mutex. Why can't we
> > rcu_read_lock(), find the min and max voltage, and then release
> > the read lock and ask regulator framework for the voltage time?
> 
> That was possible before this series came in..
> 
> > From what I can tell we're just trying to make sure that the list
> > is stable when iterating through it to find the min/max voltage.
> 
> Hmm, we have pointer to regulator within the dev-opp struct. If we
> drop the lock, the dev-opp struct can get freed and regulator will be
> put just before that. So, we may be using an already freed resource.
> 
> How do you suggest we fix it ?
> 

Ok, first off, I don't understand why the regulator and clock
pointers are in the struct device_opp instead of the struct
device_list_opp. I thought we wanted to make it possible for two
devices to share the same OPP table (device_opp), but have
physically different clocks and regulators (non opp-shared case)?
If we put the clock and regulator handles in the device_opp then
the opp table is limited to one device or a set of devices that
all share the same clock and regulators.

BTW, these structure names confuse me all the time. I have to
rename dev_pm_opp to opp_entry, device_opp to opp_table, and
device_list_opp to opp_device_list in my head for anything to
make sense.

Gripes aside, the clock and regulator pointers should never be
'put' and go away until the device driver that's using the
dev_pm_opp_set_rate() API has called dev_pm_opp_remove(). So any
concerns about that happening during an OPP switch aren't the
concern of the OPP framework, but the concern of the consumer
drivers having the proper locking and tear down sequences.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 17/17] cpufreq: dt: No need to allocate resources anymore

[Stephen Boyd]

On 01/12, Viresh Kumar wrote:
> On 11-01-16, 18:20, Stephen Boyd wrote:
> > On 12/22, Viresh Kumar wrote:
> > > OPP layer manages it now and cpufreq-dt driver doesn't need it. But, we
> > > still need to check for availability of resources for deferred probing.
> > 
> > Why? It seems cleaner to let OPP layer return an error indicating
> > probe defer or failure when we try to initialize it. That way we
> > aren't duplicating the same logic in two places to figure out if
> > a regulator or clock is ready.
> 
> cpufreq driver's ->init() callback doesn't return the error value
> properly to the probe() function, and so it was done this way in the
> first place. The problem is in subsys framework. I tried to fix it but
> it was rejected and we need to fix it some other way:
> 
> http://marc.info/?l=linux-kernel&m=143530948918819&w=2

Yeah I don't understand why we at least can't populate the OPP
structures and get the clocks and regulators for all the CPU
devices before we register the dt_cpufreq_driver structure. The
CPU devices should exist at that point, and we can wait to do
CPUfreq transitions until the regulators/clocks for all the CPUs
are registered. Sure we'd need to find the OPPs that are being
shared in the cpufreq_init callback and populate the cpu
frequency tables, etc., but that's not a big deal.

Making a CPU processor driver on ARM/non-ACPI systems would be to
solve the problem of all these random things like cpuidle-arm and
cpufreq-dt going through the list of cpu devices and hooking up
stuff to the cpuidle, cpufreq, and thermal frameworks. That's a
separate but related problem to probe defering the cpufreq-dt
driver.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 04/17] PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()

[Viresh Kumar]

On 20-01-16, 16:20, Stephen Boyd wrote:
> Ok, first off, I don't understand why the regulator and clock
> pointers are in the struct device_opp instead of the struct
> device_list_opp. I thought we wanted to make it possible for two
> devices to share the same OPP table (device_opp), but have
> physically different clocks and regulators (non opp-shared case)?
> If we put the clock and regulator handles in the device_opp then
> the opp table is limited to one device or a set of devices that
> all share the same clock and regulators.

Not exactly. We wanted devices (with separate rails) to share the OPP
tables ONLY IN DT. So that the same thing isn't required to be copied
multiple times in DT, for example in case of Krait.

But the code isn't supposed to shared device_opp structure at all.
There is one device_opp structure for a groups of devices that share
their OPPs and their clock/voltage rails. opp_device_list is
representing individual devices that share the same device_opp thing.

And this works pretty well and I don't think we should touch it now.

> BTW, these structure names confuse me all the time. I have to
> rename dev_pm_opp to opp_entry, device_opp to opp_table, and
> device_list_opp to opp_device_list in my head for anything to
> make sense.

Sure, we can (should) rename them after this series goes in.

> Gripes aside, the clock and regulator pointers should never be
> 'put' and go away until the device driver that's using the
> dev_pm_opp_set_rate() API has called dev_pm_opp_remove().

Right.

> So any
> concerns about that happening during an OPP switch aren't the
> concern of the OPP framework, but the concern of the consumer
> drivers having the proper locking and tear down sequences.

I am fine with that view as well.. It simplifies lots of things for me
:)

-- 
viresh

Re: [PATCH 17/17] cpufreq: dt: No need to allocate resources anymore

[Viresh Kumar]

On 20-01-16, 17:18, Stephen Boyd wrote:
> Yeah I don't understand why we at least can't populate the OPP
> structures and get the clocks and regulators for all the CPU
> devices before we register the dt_cpufreq_driver structure. The
> CPU devices should exist at that point, and we can wait to do
> CPUfreq transitions until the regulators/clocks for all the CPUs
> are registered. Sure we'd need to find the OPPs that are being
> shared in the cpufreq_init callback and populate the cpu
> frequency tables, etc., but that's not a big deal.

We can do this, yes. But ->init() was really the right place to fix
that, we aren't able to do it properly because we lack a cpu processor
driver for ARM.

> Making a CPU processor driver on ARM/non-ACPI systems would be to
> solve the problem of all these random things like cpuidle-arm and
> cpufreq-dt going through the list of cpu devices and hooking up
> stuff to the cpuidle, cpufreq, and thermal frameworks. That's a
> separate but related problem to probe defering the cpufreq-dt
> driver.

Yeah, I wanted to work on that sometime :)

-- 
viresh

Re: [PATCH 04/17] PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()

[Stephen Boyd]

On 01/20/2016 06:32 PM, Viresh Kumar wrote:
> On 20-01-16, 16:20, Stephen Boyd wrote:
>> Ok, first off, I don't understand why the regulator and clock
>> pointers are in the struct device_opp instead of the struct
>> device_list_opp. I thought we wanted to make it possible for two
>> devices to share the same OPP table (device_opp), but have
>> physically different clocks and regulators (non opp-shared case)?
>> If we put the clock and regulator handles in the device_opp then
>> the opp table is limited to one device or a set of devices that
>> all share the same clock and regulators.
> Not exactly. We wanted devices (with separate rails) to share the OPP
> tables ONLY IN DT. So that the same thing isn't required to be copied
> multiple times in DT, for example in case of Krait.
>
> But the code isn't supposed to shared device_opp structure at all.
> There is one device_opp structure for a groups of devices that share
> their OPPs and their clock/voltage rails. opp_device_list is
> representing individual devices that share the same device_opp thing.
>
> And this works pretty well and I don't think we should touch it now.
>
>

Ok. Thanks for clarifying. I see the code matches that description.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 17/17] cpufreq: dt: No need to allocate resources anymore

[Stephen Boyd]

On 01/20/2016 06:36 PM, Viresh Kumar wrote:
> On 20-01-16, 17:18, Stephen Boyd wrote:
>> Yeah I don't understand why we at least can't populate the OPP
>> structures and get the clocks and regulators for all the CPU
>> devices before we register the dt_cpufreq_driver structure. The
>> CPU devices should exist at that point, and we can wait to do
>> CPUfreq transitions until the regulators/clocks for all the CPUs
>> are registered. Sure we'd need to find the OPPs that are being
>> shared in the cpufreq_init callback and populate the cpu
>> frequency tables, etc., but that's not a big deal.
> We can do this, yes. But ->init() was really the right place to fix
> that, we aren't able to do it properly because we lack a cpu processor
> driver for ARM.

Sorry I don't understand why ->init() is so important here. It seems
like ->init() is being used because we lack a proper processor driver on
ARM, but it isn't required and I don't see how it could be the right
place to handle any probe defer stuff because that's already been
rejected by Rafael and Greg.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 17/17] cpufreq: dt: No need to allocate resources anymore

[Viresh Kumar]

On 11-01-16, 18:20, Stephen Boyd wrote:
> > @@ -241,9 +234,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> >  	}
> >  
> >  	priv->cpu_dev = cpu_dev;
> > -	priv->cpu_reg = cpu_reg;
> >  	policy->driver_data = priv;
> > -
> >  	policy->clk = cpu_clk;
> 
> Maybe we can have an dev_pm_opp_get_rate() API and a
> cpufreq_generic_opp_get() so we can get rid of policy->clk usage
> in this driver?

I thought about this today and couldn't get to a sane solution.
If we implement dev_pm_opp_get_rate(), then it should return the
cached value of the currently programmed OPP. If the OPP isn't set
yet, it can call clk_get_rate() and return that instead. But it
doesn't make any sense to always call clk_get_rate() from
dev_pm_opp_get_rate(), as that doesn't match its name.

OTOH, cpufreq really really needs a clk_get_rate() to happen, there
can be cases where the clk is changed by some other entity, specially
during suspend/resume and there are special checks in cpufreq about
that.

We can surely add a patch later if we get answer to this, but for now,
I will keep this patch as is.

-- 
viresh