PM / OPP: Introduce APIs to transition OPPs

PATCH Superseded Viresh Kumar 2 participants 38 messages Jan 28, 2016 – Feb 9, 2016

Subsystems (6)

Driver core CPU frequency scaling framework Suspend to RAM Hibernation +2 more subsystems

lore

Share & tools

lore

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #0 lore raw

v2 cover PM / OPP: Introduce APIs to transition OPPs

Hi Guys,

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 7 patches update the OPP core to introduce the new APIs and the next Nine patches update cpufreq-dt for the same.

11 out of 17 are already Reviewed by Stephen, only few are left :)

@Rafael: I will send a pull request once that is done.

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

V1->V2:
- Locking fixes as suggested by Stephen
- Made dev_pm_opp_put_regulator(), as we can't rely on dev_name() to get name of the supply (Stephen/Mark B.)
- Other minor modifications

Viresh Kumar (16):
PM / OPP: get/put regulators from OPP core
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_debug/err() calls to dev_dbg/err()
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
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 | 420 ++++++++++++++++++++++++++++++++++++++++++
 drivers/base/power/opp/opp.h  |  13 ++
 drivers/cpufreq/cpufreq-dt.c  | 290 ++++++++++-------------------
 include/linux/pm_opp.h        |  27 +++
 4 files changed, 555 insertions(+), 195 deletions(-)

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #1 lore raw

PATCH v2 1/16 PM / OPP: get/put regulators from OPP core

This allows the OPP core to request/free the regulator resource, attached to a device OPP. The regulator device is fetched using the name provided by the driver, while calling: dev_pm_opp_set_regulator().

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

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

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/base/power/opp/core.c | 111 ++++++++++++++++++++++++++++++++++++++++++
 drivers/base/power/opp/opp.h  |   4 ++
 include/linux/pm_opp.h        |   9 ++++
 3 files changed, 124 insertions(+)

drivers/base/power/opp/core.c +111 -0

index cf351d3dab1c..1e22b71abf1e 100644
@@ -19,6 +19,7 @@
 #include <linux/device.h>
 #include <linux/of.h>
 #include <linux/export.h>
+#include <linux/regulator/consumer.h>
 
 #include "opp.h"

@@ -565,6 +566,9 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (dev_opp->prop_name)
 		return;
 
+	if (!IS_ERR_OR_NULL(dev_opp->regulator))
+		return;
+
 	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
 				    node);

@@ -1085,6 +1089,113 @@ 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.
+ *
+ * In order to support OPP switching, OPP layer needs to know the name of the
+ * device's regulator, as the core would be required to switch voltages as well.
+ *
+ * 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;
+
+	mutex_lock(&dev_opp_list_lock);
+
+	dev_opp = _add_device_opp(dev);
+	if (!dev_opp) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	/* This should be called before OPPs are initialized */
+	if (WARN_ON(!list_empty(&dev_opp->opp_list))) {
+		ret = -EBUSY;
+		goto err;
+	}
+
+	/* Already have a regulator set */
+	if (WARN_ON(!IS_ERR_OR_NULL(dev_opp->regulator))) {
+		ret = -EBUSY;
+		goto err;
+	}
+	/* 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);
+		goto err;
+	}
+
+	dev_opp->regulator = reg;
+
+	mutex_unlock(&dev_opp_list_lock);
+	return 0;
+
+err:
+	_remove_device_opp(dev_opp);
+unlock:
+	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;
+	}
+
+	if (IS_ERR_OR_NULL(dev_opp->regulator)) {
+		dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
+		goto unlock;
+	}
+
+	/* Make sure there are no concurrent readers while updating dev_opp */
+	WARN_ON(!list_empty(&dev_opp->opp_list));
+
+	regulator_put(dev_opp->regulator);
+	dev_opp->regulator = ERR_PTR(-EINVAL);
+
+	/* 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)
 {

drivers/base/power/opp/opp.h +4 -0

index 690638ef36ee..416293b7da23 100644
@@ -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;

include/linux/pm_opp.h +9 -0

index 95403d2ccaf5..c70a18ac9c8a 100644
@@ -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)

Signature · 1 line

2.7.0.79.gdc08a19

Stephen Boyd

2016-02-02 02:29 UTC #2 lore raw

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

Quoted 32 lines · Viresh Kumar

drivers/base/power/opp/core.c +32 -0

+
+/**
+ * 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;
+	}
+
+	if (IS_ERR_OR_NULL(dev_opp->regulator)) {
+		dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
+		goto unlock;
+	}
+
+	/* Make sure there are no concurrent readers while updating dev_opp */
+	WARN_ON(!list_empty(&dev_opp->opp_list));
+

drivers/base/power/opp/core.c void dev_pm_opp_put_prop_name(struct device *dev) · line 1197

Quoted 10 lines

drivers/base/power/opp/core.c +10 -0

+	regulator_put(dev_opp->regulator);
+	dev_opp->regulator = ERR_PTR(-EINVAL);
+
+	/* 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);

I'm still lost why we need this API. When the OPP is torn down we can call regulator_put there instead. The same style seems to be done for supported hw, and prop_name, which doesn't make any sense either. Just tear everything down when there aren't any more OPPs in the table.

Signature · 2 lines

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

Viresh Kumar

author maintainer

2016-02-02 03:23 UTC #3 lore raw

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

Quoted 5 lines · Stephen Boyd

I'm still lost why we need this API. When the OPP is torn down we can call regulator_put there instead. The same style seems to be done for supported hw, and prop_name, which doesn't make any sense either. Just tear everything down when there aren't any more OPPs in the table.

I explained that earlier as well, but you never replied to that :) Let me paste that again here:

Consider this case:
- Platform code sets regulator for cpuX (Create OPP-table struct and set regulator)
- insmod cpufreq-dt.ko (Fill OPP table)
- rmmod cpufreq-dt.ko (Remove OPP table and struct, according to your suggestion)
- insmod cpufreq-dt.ko (No regulator found).

The platform code is supposed to set regulator, supported-hw, prop-name only once from some init-code. And it should just work out of the box after that. And so these calls are really required.

Signature · 1 line

viresh

Stephen Boyd

2016-02-08 22:52 UTC #4 lore raw

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

Quoted 18 lines · Viresh Kumar

Quoted 5 lines · Stephen Boyd
I'm still lost why we need this API. When the OPP is torn down we can call regulator_put there instead. The same style seems to be done for supported hw, and prop_name, which doesn't make any sense either. Just tear everything down when there aren't any more OPPs in the table.
I explained that earlier as well, but you never replied to that :) Let me paste that again here:
Consider this case:
- Platform code sets regulator for cpuX (Create OPP-table struct and set regulator)
- insmod cpufreq-dt.ko (Fill OPP table)
- rmmod cpufreq-dt.ko (Remove OPP table and struct, according to your suggestion)
- insmod cpufreq-dt.ko (No regulator found).

Quoted 4 lines

The platform code is supposed to set regulator, supported-hw, prop-name only once from some init-code. And it should just work out of the box after that. And so these calls are really required.

Ok the sequence makes sense now that it's clearly explained. I wonder if we should create and destroy OPP tables when a device is created and destroyed instead of triggering that from a driver. I suppose not creating the tables until they're used is good for saving memory though?

Signature · 2 lines

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

Viresh Kumar

author maintainer

2016-02-09 03:53 UTC #5 lore raw

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

Quoted 5 lines · Stephen Boyd

Ok the sequence makes sense now that it's clearly explained. I wonder if we should create and destroy OPP tables when a device is created and destroyed instead of triggering that from a driver. I suppose not creating the tables until they're used is good for saving memory though?

That is one of the aspects, over that, there are cases where we want to do few things from platform code before initializing the OPP tables. For examples, setting the hw-version or '-name', as done in ST's case.

Signature · 1 line

viresh

Viresh Kumar

author maintainer

2016-02-09 03:54 UTC #6 lore raw

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

In reply to Stephen Boyd

Quoted 1 line · Stephen Boyd

Ok the sequence makes sense now that it's clearly explained.

Should I consider it as a Reviewed-by ?

Signature · 1 line

viresh

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #7 lore raw

PATCH v2 2/16 PM / OPP: Disable OPPs that aren't supported by the regulator

In reply to Viresh Kumar

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

Signed-off-by Viresh Kumar <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

---
 drivers/base/power/opp/core.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/base/power/opp/core.c +22 -0

index 1e22b71abf1e..71545becfca1 100644
@@ -687,6 +687,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)
 {

@@ -728,6 +744,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;
 }

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-01-28 08:21 UTC #8 lore raw

PATCH v2 3/16 PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()

In reply to 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 <[email protected]>

---
 drivers/base/power/opp/core.c | 59 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h        |  6 +++++
 2 files changed, 65 insertions(+)

drivers/base/power/opp/core.c +59 -0

index 71545becfca1..ffe2406af882 100644
@@ -231,6 +231,65 @@ 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)) {
+		rcu_read_unlock();
+		return 0;
+	}
+
+	reg = dev_opp->regulator;
+	if (IS_ERR_OR_NULL(reg)) {
+		/* Regulator may not be required for device */
+		if (reg)
+			dev_err(dev, "%s: Invalid regulator (%ld)\n", __func__,
+				PTR_ERR(reg));
+		rcu_read_unlock();
+		return 0;
+	}
+
+	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;
+	}
+
+	rcu_read_unlock();
+
+	/*
+	 * The caller needs to ensure that dev_opp (and hence the regulator)
+	 * isn't freed, while we are executing this routine.
+	 */
+	ret = regulator_set_voltage_time(reg, min_uV, max_uV);
+	if (ret > 0)
+		latency_ns = ret * 1000;
+
+	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
  *

include/linux/pm_opp.h +6 -0

index c70a18ac9c8a..5daa43058ac1 100644
@@ -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;

Signature · 1 line

2.7.0.79.gdc08a19

Stephen Boyd

Reviewed-by on v2 3/16

2016-02-02 02:30 UTC #9 lore raw

Re: [PATCH V2 03/16] PM / OPP: Introduce dev_pm_opp_get_max_volt_latency()

Quoted 5 lines · 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 <[email protected]>
---

Reviewed-by Stephen Boyd <[email protected]>

Signature · 2 lines

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

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #10 lore raw

PATCH v2 4/16 PM / OPP: Introduce dev_pm_opp_get_max_transition_latency()

In reply to 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 <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

---
 drivers/base/power/opp/core.c | 17 +++++++++++++++++
 include/linux/pm_opp.h        |  6 ++++++
 2 files changed, 23 insertions(+)

drivers/base/power/opp/core.c +17 -0

index ffe2406af882..b0f5c72f0fc3 100644
@@ -290,6 +290,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
  *

include/linux/pm_opp.h +6 -0

index 5daa43058ac1..59da3d9e11ea 100644
@@ -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;

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-01-28 08:21 UTC #11 lore raw

PATCH v2 5/16 PM / OPP: Parse clock-latency and voltage-tolerance for v1 bindings

In reply to 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 <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

---
 drivers/base/power/opp/core.c | 20 ++++++++++++++++++++
 drivers/base/power/opp/opp.h  |  6 ++++++
 2 files changed, 26 insertions(+)

drivers/base/power/opp/core.c +20 -0

index b0f5c72f0fc3..4fafa733a1c7 100644
@@ -582,6 +582,7 @@ static struct device_opp *_add_device_opp(struct device *dev)
 {
 	struct device_opp *dev_opp;
 	struct device_list_opp *list_dev;
+	struct device_node *np;
 
 	/* Check for existing list for 'dev' first */
 	dev_opp = _find_device_opp(dev);

@@ -604,6 +605,21 @@ static struct device_opp *_add_device_opp(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);
+	}
+
 	srcu_init_notifier_head(&dev_opp->srcu_head);
 	INIT_LIST_HEAD(&dev_opp->opp_list);

@@ -861,6 +877,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 */

@@ -874,7 +891,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;

drivers/base/power/opp/opp.h +6 -0

index 416293b7da23..fe44beb404ba 100644
@@ -138,6 +138,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.

@@ -156,6 +158,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;

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #12 lore raw

PATCH v2 6/16 PM / OPP: Manage device clk

In reply to 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 <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

---
 drivers/base/power/opp/core.c | 15 +++++++++++++++
 drivers/base/power/opp/opp.h  |  3 +++
 2 files changed, 18 insertions(+)

drivers/base/power/opp/core.c +15 -0

index 4fafa733a1c7..7d7749ce1ce4 100644
@@ -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>

@@ -583,6 +584,7 @@ static struct device_opp *_add_device_opp(struct device *dev)
 	struct device_opp *dev_opp;
 	struct device_list_opp *list_dev;
 	struct device_node *np;
+	int ret;
 
 	/* Check for existing list for 'dev' first */
 	dev_opp = _find_device_opp(dev);

@@ -620,6 +622,15 @@ static struct device_opp *_add_device_opp(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);
+	}
+
 	srcu_init_notifier_head(&dev_opp->srcu_head);
 	INIT_LIST_HEAD(&dev_opp->opp_list);

@@ -661,6 +672,10 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (!IS_ERR_OR_NULL(dev_opp->regulator))
 		return;
 
+	/* Release clk */
+	if (!IS_ERR(dev_opp->clk))
+		clk_put(dev_opp->clk);
+
 	list_dev = list_first_entry(&dev_opp->dev_list, struct device_list_opp,
 				    node);

drivers/base/power/opp/opp.h +3 -0

index fe44beb404ba..4f1bdfc7da03 100644
@@ -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
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.

@@ -168,6 +170,7 @@ struct device_opp {
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
 	const char *prop_name;
+	struct clk *clk;
 	struct regulator *regulator;
 
 #ifdef CONFIG_DEBUG_FS

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #13 lore raw

PATCH v2 7/16 PM / OPP: Add dev_pm_opp_set_rate()

In reply to 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 <[email protected]>

---
 drivers/base/power/opp/core.c | 176 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h        |   6 ++
 2 files changed, 182 insertions(+)

drivers/base/power/opp/core.c +176 -0

index 7d7749ce1ce4..2672f4bfec41 100644
@@ -529,6 +529,182 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
+/*
+ * The caller needs to ensure that device_opp (and hence the clk) isn't freed,
+ * while clk returned here is used.
+ */
+static struct clk *_get_opp_clk(struct device *dev)
+{
+	struct device_opp *dev_opp;
+	struct clk *clk;
+
+	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__);
+		clk = (struct clk *)dev_opp;
+		goto unlock;
+	}
+
+	clk = dev_opp->clk;
+	if (IS_ERR(clk))
+		dev_err(dev, "%s: No clock available for the device\n",
+			__func__);
+
+unlock:
+	rcu_read_unlock();
+	return clk;
+}
+
+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 takes rcu_read_lock().
+ */
+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;
+	}
+
+	clk = _get_opp_clk(dev);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	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);
+		return 0;
+	}
+
+	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;
+	}
+
+	old_opp = dev_pm_opp_find_freq_ceil(dev, &old_freq);
+	if (!IS_ERR(old_opp)) {
+		ou_volt = old_opp->u_volt;
+		ou_volt_min = old_opp->u_volt_min;
+		ou_volt_max = old_opp->u_volt_max;
+	} else {
+		dev_err(dev, "%s: failed to find Current OPP for freq %lu (%ld)\n",
+			__func__, old_freq, PTR_ERR(old_opp));
+	}
+
+	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;
+
+	reg = dev_opp->regulator;
+
+	rcu_read_unlock();
+
+	/* 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;
+	}
+
+	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 even if the voltages weren't updated earlier */
+	if (!IS_ERR(old_opp))
+		_set_opp_voltage(dev, reg, ou_volt, ou_volt_min, ou_volt_max);
+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)
 {

include/linux/pm_opp.h +6 -0

index 59da3d9e11ea..cccaf4a29e9f 100644
@@ -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)

Signature · 1 line

2.7.0.79.gdc08a19

Stephen Boyd

2016-02-02 02:10 UTC #14 lore raw

Re: [PATCH V2 07/16] PM / OPP: Add dev_pm_opp_set_rate()

Quoted 13 lines · Viresh Kumar

drivers/base/power/opp/core.c +5 -0

index 7d7749ce1ce4..2672f4bfec41 100644
@@ -529,6 +529,182 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
+/*
+ * The caller needs to ensure that device_opp (and hence the clk) isn't freed,
+ * while clk returned here is used.
+ */
+static struct clk *_get_opp_clk(struct device *dev)

drivers/base/power/opp/core.c struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, · line 546

Quoted 10 lines

drivers/base/power/opp/core.c +10 -0

+{
+	struct device_opp *dev_opp;
+	struct clk *clk;
+
+	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__);
+		clk = (struct clk *)dev_opp;

clk = ERR_CAST(dev_opp);

Quoted 10 lines

drivers/base/power/opp/core.c +10 -0

+		goto unlock;
+	}
+
+	clk = dev_opp->clk;
+	if (IS_ERR(clk))
+		dev_err(dev, "%s: No clock available for the device\n",
+			__func__);
+
+unlock:
+	rcu_read_unlock();

drivers/base/power/opp/core.c struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, · line 566

Quoted 10 lines

drivers/base/power/opp/core.c +10 -0

+	return clk;
+}
+
+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 */

not available?

Quoted 68 lines

drivers/base/power/opp/core.c +68 -0

+	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 takes rcu_read_lock().
+ */
+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;
+	}
+
+	clk = _get_opp_clk(dev);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	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);
+		return 0;
+	}
+
+	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);

drivers/base/power/opp/core.c struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, · line 644

Quoted 10 lines

drivers/base/power/opp/core.c +10 -0

+		goto unlock;
+	}
+
+	old_opp = dev_pm_opp_find_freq_ceil(dev, &old_freq);
+	if (!IS_ERR(old_opp)) {
+		ou_volt = old_opp->u_volt;
+		ou_volt_min = old_opp->u_volt_min;
+		ou_volt_max = old_opp->u_volt_max;
+	} else {
+		dev_err(dev, "%s: failed to find Current OPP for freq %lu (%ld)\n",

Why is current capitalized?

Quoted 49 lines

drivers/base/power/opp/core.c +49 -0

+			__func__, old_freq, PTR_ERR(old_opp));
+	}
+
+	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;
+
+	reg = dev_opp->regulator;
+
+	rcu_read_unlock();
+
+	/* 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;
+	}
+
+	return 0;
+

drivers/base/power/opp/core.c struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, · line 703

Quoted 10 lines

drivers/base/power/opp/core.c +10 -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 even if the voltages weren't updated earlier */
+	if (!IS_ERR(old_opp))
+		_set_opp_voltage(dev, reg, ou_volt, ou_volt_min, ou_volt_max);
+unlock:
+	rcu_read_unlock();

The error gotos above this don't have rcu lock held, so this is sometimes wrong to do.

Signature · 2 lines

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

Viresh Kumar

author maintainer

2016-02-02 03:39 UTC #15 lore raw

Re: [PATCH V2 07/16] PM / OPP: Add dev_pm_opp_set_rate()

Quoted 8 lines · Stephen Boyd

clk = ERR_CAST(dev_opp);
not available?
Why is current capitalized?
The error gotos above this don't have rcu lock held, so this is sometimes wrong to do.

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

From: Viresh Kumar <[email protected]>
Date: Fri, 4 Sep 2015 12:28:39 +0530
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 <[email protected]>

---
 drivers/base/power/opp/core.c | 176 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h        |   6 ++
 2 files changed, 182 insertions(+)

drivers/base/power/opp/core.c +176 -0

index 7d7749ce1ce4..ab711c2c3e00 100644
@@ -529,6 +529,182 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
+/*
+ * The caller needs to ensure that device_opp (and hence the clk) isn't freed,
+ * while clk returned here is used.
+ */
+static struct clk *_get_opp_clk(struct device *dev)
+{
+	struct device_opp *dev_opp;
+	struct clk *clk;
+
+	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__);
+		clk = ERR_CAST(dev_opp);
+		goto unlock;
+	}
+
+	clk = dev_opp->clk;
+	if (IS_ERR(clk))
+		dev_err(dev, "%s: No clock available for the device\n",
+			__func__);
+
+unlock:
+	rcu_read_unlock();
+	return clk;
+}
+
+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 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 takes rcu_read_lock().
+ */
+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;
+	}
+
+	clk = _get_opp_clk(dev);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	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);
+		return 0;
+	}
+
+	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__);
+		rcu_read_unlock();
+		return PTR_ERR(dev_opp);
+	}
+
+	old_opp = dev_pm_opp_find_freq_ceil(dev, &old_freq);
+	if (!IS_ERR(old_opp)) {
+		ou_volt = old_opp->u_volt;
+		ou_volt_min = old_opp->u_volt_min;
+		ou_volt_max = old_opp->u_volt_max;
+	} else {
+		dev_err(dev, "%s: failed to find current OPP for freq %lu (%ld)\n",
+			__func__, old_freq, PTR_ERR(old_opp));
+	}
+
+	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);
+		rcu_read_unlock();
+		return ret;
+	}
+
+	u_volt = opp->u_volt;
+	u_volt_min = opp->u_volt_min;
+	u_volt_max = opp->u_volt_max;
+
+	reg = dev_opp->regulator;
+
+	rcu_read_unlock();
+
+	/* 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;
+	}
+
+	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 even if the voltages weren't updated earlier */
+	if (!IS_ERR(old_opp))
+		_set_opp_voltage(dev, reg, ou_volt, ou_volt_min, ou_volt_max);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
+
 /* List-dev Helpers */
 static void _kfree_list_dev_rcu(struct rcu_head *head)
 {

include/linux/pm_opp.h +6 -0

index 59da3d9e11ea..cccaf4a29e9f 100644
@@ -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)

Stephen Boyd

Reviewed-by

2016-02-02 20:46 UTC #16 lore raw

Re: [PATCH V2 07/16] PM / OPP: Add dev_pm_opp_set_rate()

Quoted 9 lines · Viresh Kumar

On 01-02-16, 18:10, Stephen Boyd wrote:
From: Viresh Kumar <[email protected]>
Date: Fri, 4 Sep 2015 12:28:39 +0530
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.

Quoted 6 lines

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 <[email protected]>
---

Reviewed-by Stephen Boyd <[email protected]>

Signature · 2 lines

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

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #17 lore raw

PATCH v2 8/16 cpufreq: dt: Convert few pr_debug/err() calls to dev_dbg/err()

In reply to Viresh Kumar

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

Signed-off-by Viresh Kumar <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

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

drivers/cpufreq/cpufreq-dt.c +2 -2

index 0ca74d070058..ace0168274d4 100644
@@ -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;
 	}

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-01-28 08:21 UTC #18 lore raw

PATCH v2 9/16 cpufreq: dt: Rename 'need_update' to 'opp_v1'

In reply to 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 <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

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

drivers/cpufreq/cpufreq-dt.c +3 -3

index ace0168274d4..0047d20803db 100644
@@ -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)

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #19 lore raw

PATCH v2 10/16 cpufreq: dt: OPP layers handles clock-latency for V1 bindings as well

In reply to 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 <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

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

drivers/cpufreq/cpufreq-dt.c +1 -4

index 0047d20803db..4c9f8a828f6f 100644
@@ -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;

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #20 lore raw

PATCH v2 11/16 cpufreq: dt: Pass regulator name to the OPP core

In reply to Viresh Kumar

OPP core can handle the regulators by itself, and but it needs to know the name of the regulator to fetch. Add support for that.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq-dt.c +70 -0

index 4c9f8a828f6f..15637734986a 100644
@@ -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[] = {

@@ -119,6 +120,49 @@ 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
+ * "cpu0-supply", we still need to handle that for backwards compatibility.
+ */
+static const char *find_supply_name(struct device *dev)
+{
+	struct regulator *cpu_reg;
+	char *reg_cpu0 = "cpu0", *reg_cpu = "cpu", *reg;
+	int cpu = dev->id, ret;
+
+	/* Try "cpu0" for older DTs */
+	if (!cpu)
+		reg = reg_cpu0;
+	else
+		reg = reg_cpu;
+
+try_again:
+	cpu_reg = regulator_get_optional(dev, reg);
+	ret = PTR_ERR_OR_ZERO(cpu_reg);
+	if (!ret) {
+		regulator_put(cpu_reg);
+		return reg;
+	}
+
+	/*
+	 * If cpu's regulator supply node is present, but regulator is not yet
+	 * registered, we should try defering probe.
+	 */
+	if (ret == -EPROBE_DEFER) {
+		dev_dbg(dev, "cpu%d regulator not ready, retry\n", cpu);
+		return ERR_PTR(ret);
+	}
+
+	/* Try with "cpu-supply" */
+	if (reg == reg_cpu0) {
+		reg = reg_cpu;
+		goto try_again;
+	}
+
+	dev_dbg(dev, "no regulator for cpu%d: %d\n", cpu, ret);
+	return NULL;
+}
+
 static int allocate_resources(int cpu, struct device **cdev,
 			      struct regulator **creg, struct clk **cclk)
 {

@@ -200,6 +244,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 +274,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 first.
+	 */
+	name = find_supply_name(cpu_dev);
+	if (IS_ERR(name)) {
+		ret = PTR_ERR(name);
+		goto out_node_put;
+	}
+
+	if (name) {
+		ret = dev_pm_opp_set_regulator(cpu_dev, name);
+		if (ret) {
+			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
+				policy->cpu, ret);
+			goto out_node_put;
+		}
+	}
+
+	/*
 	 * 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 +337,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 +431,8 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	kfree(priv);
 out_free_opp:
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
+	if (name)
+		dev_pm_opp_put_regulator(cpu_dev);
 out_node_put:
 	of_node_put(np);
 out_put_reg_clk:

@@ -383,6 +450,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);

Signature · 1 line

2.7.0.79.gdc08a19

Stephen Boyd

2016-02-02 02:34 UTC #21 lore raw

Re: [PATCH V2 11/16] cpufreq: dt: Pass regulator name to the OPP core

Quoted 15 lines · Viresh Kumar

drivers/cpufreq/cpufreq-dt.c +11 -0

@@ -119,6 +120,49 @@ 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
+ * "cpu0-supply", we still need to handle that for backwards compatibility.
+ */
+static const char *find_supply_name(struct device *dev)
+{
+	struct regulator *cpu_reg;
+	char *reg_cpu0 = "cpu0", *reg_cpu = "cpu", *reg;
+	int cpu = dev->id, ret;
+
+	/* Try "cpu0" for older DTs */

drivers/cpufreq/cpufreq-dt.c static int set_target(struct cpufreq_policy *policy, unsigned int index) · line 143

Quoted 10 lines

drivers/cpufreq/cpufreq-dt.c +10 -0

+	if (!cpu)
+		reg = reg_cpu0;
+	else
+		reg = reg_cpu;
+
+try_again:
+	cpu_reg = regulator_get_optional(dev, reg);
+	ret = PTR_ERR_OR_ZERO(cpu_reg);
+	if (!ret) {
+		regulator_put(cpu_reg);

What's the point of creating a regulator just to find the name? It seems like we should just look in the DT node of the CPU for cpu-supply vs cpu0-supply. Then we don't need to involve the regulator framework at all.

Quoted 26 lines

drivers/cpufreq/cpufreq-dt.c +22 -0

+		return reg;
+	}
+
+	/*
+	 * If cpu's regulator supply node is present, but regulator is not yet
+	 * registered, we should try defering probe.
+	 */
+	if (ret == -EPROBE_DEFER) {
+		dev_dbg(dev, "cpu%d regulator not ready, retry\n", cpu);
+		return ERR_PTR(ret);
+	}
+
+	/* Try with "cpu-supply" */
+	if (reg == reg_cpu0) {
+		reg = reg_cpu;
+		goto try_again;
+	}
+
+	dev_dbg(dev, "no regulator for cpu%d: %d\n", cpu, ret);
+	return NULL;
+}
+
 static int allocate_resources(int cpu, struct device **cdev,
 			      struct regulator **creg, struct clk **cclk)
 {

drivers/cpufreq/cpufreq-dt.c static int cpufreq_exit(struct cpufreq_policy *policy) · line 454

Quoted 5 lines

drivers/cpufreq/cpufreq-dt.c +2 -0

@@ -383,6 +450,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);

Let's hope this goes away because it's always right next to dev_pm_opp_of_cpumask_remove_table() anyway. Same for reg_name.

Signature · 2 lines

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

Viresh Kumar

author maintainer

2016-02-02 06:10 UTC #22 lore raw

Re: [PATCH V2 11/16] cpufreq: dt: Pass regulator name to the OPP core

Quoted 10 lines · Stephen Boyd

Review
Quoted 4 lines · Viresh Kumar
(quoted diff) +4 -0
+	cpu_reg = regulator_get_optional(dev, reg);
+	ret = PTR_ERR_OR_ZERO(cpu_reg);
+	if (!ret) {
+		regulator_put(cpu_reg);
What's the point of creating a regulator just to find the name? It seems like we should just look in the DT node of the CPU for cpu-supply vs cpu0-supply. Then we don't need to involve the regulator framework at all.

Yeah, this can be simplified a bit..

Quoted 4 lines

Quoted 4 lines
static int allocate_resources(int cpu, struct device **cdev,
(quoted diff) +0 -0
 			      struct regulator **creg, struct clk **cclk)
 {
@@ -383,6 +450,9 @@ static int cpufreq_exit(struct cpufreq_policy *policy)

Quoted 8 lines

Review static int cpufreq_exit(struct cpufreq_policy *policy) · line 454
Quoted 5 lines
(quoted diff) +2 -0
 	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);
Let's hope this goes away because it's always right next to dev_pm_opp_of_cpumask_remove_table() anyway. Same for reg_name.

We are calling it from cpufreq-dt in this particular case, but it can be called from platform code as well.

-------------------------8<-------------------------
From: Viresh Kumar <[email protected]>
Date: Tue, 8 Sep 2015 17:16:46 +0530
Subject: [PATCH 04/20] cpufreq: dt: Pass regulator name to the OPP core

OPP core can handle the regulators by itself, and but it needs to know the name of the regulator to fetch. Add support for that.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq-dt.c +51 -0

index 4c9f8a828f6f..d4651d684f11 100644
@@ -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[] = {

@@ -119,6 +120,30 @@ 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
+ * "cpu0-supply", we still need to handle that for backwards compatibility.
+ */
+static const char *find_supply_name(struct device *dev, struct device_node *np)
+{
+	struct property *pp;
+	int cpu = dev->id;
+
+	/* Try "cpu0" for older DTs */
+	if (!cpu) {
+		pp = of_find_property(np, "cpu0-supply", NULL);
+		if (pp)
+			return "cpu0";
+	}
+
+	pp = of_find_property(np, "cpu-supply", NULL);
+	if (pp)
+		return "cpu";
+
+	dev_dbg(dev, "no regulator for cpu%d\n", cpu);
+	return NULL;
+}
+
 static int allocate_resources(int cpu, struct device **cdev,
 			      struct regulator **creg, struct clk **cclk)
 {

@@ -200,6 +225,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 +255,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 first.
+	 */
+	name = find_supply_name(cpu_dev, np);
+	if (IS_ERR(name)) {
+		ret = PTR_ERR(name);
+		goto out_node_put;
+	}
+
+	if (name) {
+		ret = dev_pm_opp_set_regulator(cpu_dev, name);
+		if (ret) {
+			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
+				policy->cpu, ret);
+			goto out_node_put;
+		}
+	}
+
+	/*
 	 * 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 +318,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 +412,8 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	kfree(priv);
 out_free_opp:
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
+	if (name)
+		dev_pm_opp_put_regulator(cpu_dev);
 out_node_put:
 	of_node_put(np);
 out_put_reg_clk:

@@ -383,6 +431,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);

Stephen Boyd

2016-02-08 22:55 UTC #23 lore raw

Re: [PATCH V2 11/16] cpufreq: dt: Pass regulator name to the OPP core

drivers/cpufreq/cpufreq-dt.c static int cpufreq_init(struct cpufreq_policy *policy) · line 228

Quoted 8 lines · Viresh Kumar

static int allocate_resources(int cpu, struct device **cdev,

drivers/cpufreq/cpufreq-dt.c +1 -0

 			      struct regulator **creg, struct clk **cclk)
 {

@@ -200,6 +225,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;

Is this initialization necessary?

drivers/cpufreq/cpufreq-dt.c static int cpufreq_init(struct cpufreq_policy *policy) · line 262

Quoted 12 lines

drivers/cpufreq/cpufreq-dt.c +5 -0

 	int ret;
 
 	ret = allocate_resources(policy->cpu, &cpu_dev, &cpu_reg, &cpu_clk);

@@ -229,6 +255,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 first.
+	 */
+	name = find_supply_name(cpu_dev, np);
+	if (IS_ERR(name)) {

This looks to never happen?

Quoted 6 lines

drivers/cpufreq/cpufreq-dt.c +6 -0

+		ret = PTR_ERR(name);
+		goto out_node_put;
+	}
+
+	if (name) {
+		ret = dev_pm_opp_set_regulator(cpu_dev, name);

Signature · 2 lines

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

Viresh Kumar

author maintainer

2016-02-09 04:10 UTC #24 lore raw

Re: [PATCH V2 11/16] cpufreq: dt: Pass regulator name to the OPP core

Quoted 18 lines · Stephen Boyd

Review static int cpufreq_init(struct cpufreq_policy *policy) · line 228
Quoted 8 lines · Viresh Kumar
static int allocate_resources(int cpu, struct device **cdev,
(quoted diff) +1 -0
 			      struct regulator **creg, struct clk **cclk)
 {
@@ -200,6 +225,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;
Is this initialization necessary?
Quoted 6 lines
(quoted diff) +0 -0
 	int ret;
 
 	ret = allocate_resources(policy->cpu, &cpu_dev, &cpu_reg, &cpu_clk);
@@ -229,6 +255,25 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	}
 

Quoted 8 lines

Review static int cpufreq_init(struct cpufreq_policy *policy) · line 262
Quoted 6 lines
(quoted diff) +5 -0
 	/*
+	 * OPP layer will be taking care of regulators now, but it needs to know
+	 * the name of the regulator first.
+	 */
+	name = find_supply_name(cpu_dev, np);
+	if (IS_ERR(name)) {
This looks to never happen?

Stupid me :(

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

From: Viresh Kumar <[email protected]>
Date: Tue, 8 Sep 2015 17:16:46 +0530
Subject: [PATCH] cpufreq: dt: Pass regulator name to the OPP core

OPP core can handle the regulators by itself, and but it needs to know the name of the regulator to fetch. Add support for that.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq-dt.c +46 -0

index 4c9f8a828f6f..2af75f8088bb 100644
@@ -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[] = {

@@ -119,6 +120,30 @@ 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
+ * "cpu0-supply", we still need to handle that for backwards compatibility.
+ */
+static const char *find_supply_name(struct device *dev, struct device_node *np)
+{
+	struct property *pp;
+	int cpu = dev->id;
+
+	/* Try "cpu0" for older DTs */
+	if (!cpu) {
+		pp = of_find_property(np, "cpu0-supply", NULL);
+		if (pp)
+			return "cpu0";
+	}
+
+	pp = of_find_property(np, "cpu-supply", NULL);
+	if (pp)
+		return "cpu";
+
+	dev_dbg(dev, "no regulator for cpu%d\n", cpu);
+	return NULL;
+}
+
 static int allocate_resources(int cpu, struct device **cdev,
 			      struct regulator **creg, struct clk **cclk)
 {

@@ -200,6 +225,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;
 	int ret;
 
 	ret = allocate_resources(policy->cpu, &cpu_dev, &cpu_reg, &cpu_clk);

@@ -229,6 +255,20 @@ 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 first.
+	 */
+	name = find_supply_name(cpu_dev, np);
+	if (name) {
+		ret = dev_pm_opp_set_regulator(cpu_dev, name);
+		if (ret) {
+			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
+				policy->cpu, ret);
+			goto out_node_put;
+		}
+	}
+
+	/*
 	 * 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 +313,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 +407,8 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	kfree(priv);
 out_free_opp:
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
+	if (name)
+		dev_pm_opp_put_regulator(cpu_dev);
 out_node_put:
 	of_node_put(np);
 out_put_reg_clk:

@@ -383,6 +426,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);

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #25 lore raw

PATCH v2 12/16 cpufreq: dt: Unsupported OPPs are already disabled

In reply to 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 <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

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

drivers/cpufreq/cpufreq-dt.c +0 -2

index 15637734986a..ca7a930ea283 100644
@@ -373,8 +373,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++;

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #26 lore raw

PATCH v2 13/16 cpufreq: dt: Reuse dev_pm_opp_get_max_transition_latency()

In reply to 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 <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

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

drivers/cpufreq/cpufreq-dt.c +4 ~~-44~~

index ca7a930ea283..214612c473d4 100644
@@ -241,7 +241,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;

@@ -340,49 +339,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);

@@ -417,6 +373,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);

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #27 lore raw

PATCH v2 14/16 cpufreq: dt: Use dev_pm_opp_set_rate() to switch frequency

In reply to 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 <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

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

drivers/cpufreq/cpufreq-dt.c +2 ~~-71~~

index 214612c473d4..13338365f24e 100644
@@ -45,79 +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, tol = 0;
-	int volt_old = 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, %d 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);
 }
 
 /*

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #28 lore raw

PATCH v2 15/16 cpufreq: dt: drop references to DT node

In reply to Viresh Kumar

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

Signed-off-by Viresh Kumar <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

---
 drivers/cpufreq/cpufreq-dt.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

drivers/cpufreq/cpufreq-dt.c +3 ~~-17~~

index 13338365f24e..5ea41518df63 100644
@@ -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;
 };

@@ -166,7 +165,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;

@@ -183,13 +181,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) {

@@ -200,7 +191,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;
 	}
 
 	/*

@@ -210,7 +201,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	name = find_supply_name(cpu_dev);
 	if (IS_ERR(name)) {
 		ret = PTR_ERR(name);
-		goto out_node_put;
+		goto out_put_reg_clk;
 	}
 
 	if (name) {

@@ -218,7 +209,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		if (ret) {
 			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
 				policy->cpu, ret);
-			goto out_node_put;
+			goto out_put_reg_clk;
 		}
 	}

@@ -268,7 +259,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) {

@@ -310,8 +300,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:

@@ -322,8 +310,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
 	if (name)
 		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))

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-02-02 06:11 UTC #29 lore raw

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

Quoted 4 lines · Viresh Kumar

We don't need to get reference to DT node now, lets drop it.
Signed-off-by Viresh Kumar <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>

And because of changes in 11/16, this got updated as well:

-------------------------8<-------------------------
From: Viresh Kumar <[email protected]>
Date: Tue, 8 Sep 2015 17:34:26 +0530
Subject: [PATCH 08/20] cpufreq: dt: No need to fetch voltage-tolerance

Its already done by core and we don't need to get it anymore. And so, we don't need to get of node in cpufreq_init() anymore, move that to find_supply_name() instead.

Signed-off-by Viresh Kumar <[email protected]>

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

drivers/cpufreq/cpufreq-dt.c +24 ~~-24~~

index 35c5ec3b333c..3c43fda48bdf 100644
@@ -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;
 };

@@ -55,24 +54,38 @@ static int set_target(struct cpufreq_policy *policy, unsigned int index)
  * An earlier version of opp-v1 bindings used to name the regulator
  * "cpu0-supply", we still need to handle that for backwards compatibility.
  */
-static const char *find_supply_name(struct device *dev, struct device_node *np)
+static const char *find_supply_name(struct device *dev)
 {
+	struct device_node *np;
 	struct property *pp;
 	int cpu = dev->id;
+	const char *name = NULL;
+
+	np = of_node_get(dev->of_node);
+	if (!np) {
+		dev_err(dev, "failed to find cpu%d node\n", cpu);
+		return ERR_PTR(-ENOENT);
+	}
 
 	/* Try "cpu0" for older DTs */
 	if (!cpu) {
 		pp = of_find_property(np, "cpu0-supply", NULL);
-		if (pp)
-			return "cpu0";
+		if (pp) {
+			name = "cpu0";
+			goto node_put;
+		}
 	}
 
 	pp = of_find_property(np, "cpu-supply", NULL);
-	if (pp)
-		return "cpu";
+	if (pp) {
+		name = "cpu";
+		goto node_put;
+	}
 
 	dev_dbg(dev, "no regulator for cpu%d\n", cpu);
-	return NULL;
+node_put:
+	of_node_put(np);
+	return name;
 }
 
 static int allocate_resources(int cpu, struct device **cdev,

@@ -147,7 +160,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;

@@ -164,13 +176,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) {

@@ -181,17 +186,17 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		if (ret == -ENOENT)
 			opp_v1 = true;
 		else
-			goto out_node_put;
+			goto out_put_reg_clk;
 	}
 
 	/*
 	 * OPP layer will be taking care of regulators now, but it needs to know
 	 * the name of the regulator first.
 	 */
-	name = find_supply_name(cpu_dev, np);
+	name = find_supply_name(cpu_dev);
 	if (IS_ERR(name)) {
 		ret = PTR_ERR(name);
-		goto out_node_put;
+		goto out_put_reg_clk;
 	}
 
 	if (name) {

@@ -199,7 +204,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		if (ret) {
 			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
 				policy->cpu, ret);
-			goto out_node_put;
+			goto out_put_reg_clk;
 		}
 	}

@@ -249,7 +254,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) {

@@ -291,8 +295,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:

@@ -303,8 +305,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
 	if (name)
 		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))

Stephen Boyd

Reviewed-by

2016-02-08 22:56 UTC #30 lore raw

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

Quoted 10 lines · Viresh Kumar

From: Viresh Kumar <[email protected]>
Date: Tue, 8 Sep 2015 17:34:26 +0530
Subject: [PATCH 08/20] cpufreq: dt: No need to fetch voltage-tolerance
Its already done by core and we don't need to get it anymore. And so, we don't need to get of node in cpufreq_init() anymore, move that to find_supply_name() instead.
Signed-off-by Viresh Kumar <[email protected]>
---

Reviewed-by Stephen Boyd <[email protected]>

Signature · 2 lines

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

Viresh Kumar

author maintainer

2016-02-09 04:22 UTC #31 lore raw

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

In reply to Viresh Kumar

drivers/cpufreq/cpufreq-dt.c static int set_target(struct cpufreq_policy *policy, unsigned int index) · line 68

Quoted 13 lines · Viresh Kumar

drivers/cpufreq/cpufreq-dt.c +9 -0

+static const char *find_supply_name(struct device *dev)
 {
+	struct device_node *np;
 	struct property *pp;
 	int cpu = dev->id;
+	const char *name = NULL;
+
+	np = of_node_get(dev->of_node);
+	if (!np) {
+		dev_err(dev, "failed to find cpu%d node\n", cpu);
+		return ERR_PTR(-ENOENT);
+	}

I have updated that as:

+ np = of_node_get(dev->of_node);
+
+ /* This must be valid for sure */
+ if (WARN_ON(!np))
+ return NULL;

so that we don't have to check return value of find_supply_name() for errors, as you pointed out in the other patch.

Will keep your RBY tag :)

Signature · 1 line

viresh

Viresh Kumar

author maintainer

2016-01-28 08:20 UTC #32 lore raw

PATCH v2 16/16 cpufreq: dt: No need to allocate resources anymore

In reply to 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 <[email protected]>

---
 drivers/cpufreq/cpufreq-dt.c | 94 ++++++++++++--------------------------------
 1 file changed, 26 insertions(+), 68 deletions(-)

drivers/cpufreq/cpufreq-dt.c +26 ~~-68~~

index 5ea41518df63..82d23288c70f 100644
@@ -31,7 +31,6 @@
 
 struct private_data {
 	struct device *cpu_dev;
-	struct regulator *cpu_reg;
 	struct thermal_cooling_device *cdev;
 	const char *reg_name;
 };

@@ -93,70 +92,36 @@ 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;
 	struct clk *cpu_clk;
 	int ret = 0;
-	char *reg_cpu0 = "cpu0", *reg_cpu = "cpu", *reg;
+	const char *name;
 
-	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;
-
-try_again:
-	cpu_reg = regulator_get_optional(cpu_dev, reg);
-	ret = PTR_ERR_OR_ZERO(cpu_reg);
-	if (ret) {
-		/*
-		 * If cpu's regulator supply node is present, but regulator is
-		 * not yet registered, we should try defering probe.
-		 */
-		if (ret == -EPROBE_DEFER) {
-			dev_dbg(cpu_dev, "cpu%d regulator not ready, retry\n",
-				cpu);
-			return ret;
-		}
-
-		/* Try with "cpu-supply" */
-		if (reg == reg_cpu0) {
-			reg = reg_cpu;
-			goto try_again;
-		}
-
-		dev_dbg(cpu_dev, "no regulator for cpu%d: %d\n", cpu, ret);
-	}
+	name = find_supply_name(cpu_dev);
+	if (IS_ERR(name))
+		return PTR_ERR(name);
 
 	cpu_clk = clk_get(cpu_dev, NULL);
 	ret = PTR_ERR_OR_ZERO(cpu_clk);
 	if (ret) {
-		/* put regulator */
-		if (!IS_ERR(cpu_reg))
-			regulator_put(cpu_reg);
-
 		/*
 		 * If cpu's clk node is present, but clock is not yet
 		 * 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, "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 clock: %d\n", ret);
 	} else {
-		*cdev = cpu_dev;
-		*creg = cpu_reg;
-		*cclk = cpu_clk;
+		clk_put(cpu_clk);
 	}
 
 	return ret;

@@ -167,7 +132,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;

@@ -175,9 +139,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;
 	}

@@ -191,7 +162,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;
 	}
 
 	/*

@@ -201,7 +172,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	name = find_supply_name(cpu_dev);
 	if (IS_ERR(name)) {
 		ret = PTR_ERR(name);
-		goto out_put_reg_clk;
+		goto out_put_clk;
 	}
 
 	if (name) {

@@ -209,7 +180,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		if (ret) {
 			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
 				policy->cpu, ret);
-			goto out_put_reg_clk;
+			goto out_put_clk;
 		}
 	}

@@ -267,9 +238,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();

@@ -310,10 +279,8 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
 	if (name)
 		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;
 }

@@ -329,8 +296,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;

@@ -383,9 +348,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;
 
 	/*

@@ -395,19 +357,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;
 }

Signature · 1 line

2.7.0.79.gdc08a19

Viresh Kumar

author maintainer

2016-02-02 06:12 UTC #33 lore raw

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

Quoted 4 lines · 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 <[email protected]>

Got updated due to 11/16 ..

-------------------------8<-------------------------
From: Viresh Kumar <[email protected]>
Date: Wed, 9 Sep 2015 10:41:26 +0530
Subject: [PATCH 09/20] cpufreq: dt: No need to allocate resources anymore

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 <[email protected]>

---
 drivers/cpufreq/cpufreq-dt.c | 116 ++++++++++++++++++-------------------------
 1 file changed, 48 insertions(+), 68 deletions(-)

drivers/cpufreq/cpufreq-dt.c +48 ~~-68~~

index 3c43fda48bdf..1beb484b4bbe 100644
@@ -31,7 +31,6 @@
 
 struct private_data {
 	struct device *cpu_dev;
-	struct regulator *cpu_reg;
 	struct thermal_cooling_device *cdev;
 	const char *reg_name;
 };

@@ -88,73 +87,61 @@ static const char *find_supply_name(struct device *dev)
 	return name;
 }
 
-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;
 	struct clk *cpu_clk;
 	int ret = 0;
-	char *reg_cpu0 = "cpu0", *reg_cpu = "cpu", *reg;
+	const char *name;
 
-	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;
-
-try_again:
-	cpu_reg = regulator_get_optional(cpu_dev, reg);
-	ret = PTR_ERR_OR_ZERO(cpu_reg);
+	cpu_clk = clk_get(cpu_dev, NULL);
+	ret = PTR_ERR_OR_ZERO(cpu_clk);
 	if (ret) {
 		/*
-		 * If cpu's regulator supply node is present, but regulator is
-		 * not yet registered, we should try defering probe.
+		 * If cpu's clk node is present, but clock is not yet
+		 * registered, we should try defering probe.
 		 */
-		if (ret == -EPROBE_DEFER) {
-			dev_dbg(cpu_dev, "cpu%d regulator not ready, retry\n",
-				cpu);
-			return ret;
-		}
-
-		/* Try with "cpu-supply" */
-		if (reg == reg_cpu0) {
-			reg = reg_cpu;
-			goto try_again;
-		}
+		if (ret == -EPROBE_DEFER)
+			dev_dbg(cpu_dev, "clock not ready, retry\n");
+		else
+			dev_err(cpu_dev, "failed to get clock: %d\n", ret);
 
-		dev_dbg(cpu_dev, "no regulator for cpu%d: %d\n", cpu, ret);
+		return ret;
 	}
 
-	cpu_clk = clk_get(cpu_dev, NULL);
-	ret = PTR_ERR_OR_ZERO(cpu_clk);
-	if (ret) {
-		/* put regulator */
-		if (!IS_ERR(cpu_reg))
-			regulator_put(cpu_reg);
+	clk_put(cpu_clk);
+
+	name = find_supply_name(cpu_dev);
+	if (IS_ERR(name))
+		return PTR_ERR(name);
+
+	if (!name)
+		return 0;
 
+	cpu_reg = regulator_get_optional(cpu_dev, name);
+	ret = PTR_ERR_OR_ZERO(cpu_reg);
+	if (ret) {
 		/*
-		 * If cpu's clk node is present, but clock is not yet
-		 * registered, we should try defering probe.
+		 * If cpu's regulator supply node is present, but regulator is
+		 * not yet 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 regulator not ready, retry\n");
 		else
-			dev_err(cpu_dev, "failed to get cpu%d clock: %d\n", cpu,
-				ret);
-	} else {
-		*cdev = cpu_dev;
-		*creg = cpu_reg;
-		*cclk = cpu_clk;
+			dev_dbg(cpu_dev, "no regulator for cpu0: %d\n", ret);
+
+		return ret;
 	}
 
-	return ret;
+	regulator_put(cpu_reg);
+	return 0;
 }
 
 static int cpufreq_init(struct cpufreq_policy *policy)

@@ -162,7 +149,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;

@@ -170,9 +156,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;
 	}

@@ -186,7 +179,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;
 	}
 
 	/*

@@ -196,7 +189,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	name = find_supply_name(cpu_dev);
 	if (IS_ERR(name)) {
 		ret = PTR_ERR(name);
-		goto out_put_reg_clk;
+		goto out_put_clk;
 	}
 
 	if (name) {

@@ -204,7 +197,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		if (ret) {
 			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
 				policy->cpu, ret);
-			goto out_put_reg_clk;
+			goto out_put_clk;
 		}
 	}

@@ -262,9 +255,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();

@@ -305,10 +296,8 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
 	if (name)
 		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;
 }

@@ -324,8 +313,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;

@@ -378,9 +365,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;
 
 	/*

@@ -390,19 +374,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;
 }

Stephen Boyd

2016-02-08 22:58 UTC #34 lore raw

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

drivers/cpufreq/cpufreq-dt.c static const char *find_supply_name(struct device *dev) · line 124

Quoted 4 lines · Viresh Kumar

drivers/cpufreq/cpufreq-dt.c +4 -0

+	name = find_supply_name(cpu_dev);
+	if (IS_ERR(name))
+		return PTR_ERR(name);
+

Same IS_ERR question as before. Otherwise the patch looks ok.

Quoted 3 lines

(quoted diff) +2 -0
+	if (!name)
+		return 0;

Signature · 2 lines

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

Stephen Boyd

2016-01-30 01:48 UTC #35 lore raw

In reply to Viresh Kumar

Quoted 11 lines · Viresh Kumar

Hi Guys,
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 7 patches update the OPP core to introduce the new APIs and the next Nine patches update cpufreq-dt for the same.
11 out of 17 are already Reviewed by Stephen, only few are left :)

I'll look at this early next week.

Just a note for future work, I think we're going to need to add some sort of enable/disable into the OPP layer. At least in qcom designs, if a clock is off we don't want the voltage requirement for that clock to factor into the final voltage on the regulator. Furthermore, we want to disable the regulator with regulator_disable() if all the clocks are off.

This is also a problem with cpufreq-dt. The regulators and clocks are assumed to be enabled out of the bootloader, which may not even be true. Now that OPP layer is managing all the clocks and regulators here we're going to need to do something to make sure they're on and controllable.

Signature · 2 lines

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

Viresh Kumar

author maintainer

2016-02-01 04:09 UTC #36 lore raw

Quoted 1 line · Stephen Boyd

I'll look at this early next week.

Thanks.

Quoted 6 lines

Just a note for future work, I think we're going to need to add some sort of enable/disable into the OPP layer. At least in qcom designs, if a clock is off we don't want the voltage requirement for that clock to factor into the final voltage on the regulator. Furthermore, we want to disable the regulator with regulator_disable() if all the clocks are off.

Hmm, we need to discuss more on this, maybe in a HO sometime or at connect :)

Quoted 5 lines

This is also a problem with cpufreq-dt. The regulators and clocks are assumed to be enabled out of the bootloader, which may not even be true. Now that OPP layer is managing all the clocks and regulators here we're going to need to do something to make sure they're on and controllable.

Also, I fail to understand, how the clock of a 'online' CPU can be off?

Signature · 1 line

viresh

Stephen Boyd

2016-02-01 19:41 UTC #37 lore raw

Quoted 10 lines · Viresh Kumar

Quoted 6 lines · Stephen Boyd
Just a note for future work, I think we're going to need to add some sort of enable/disable into the OPP layer. At least in qcom designs, if a clock is off we don't want the voltage requirement for that clock to factor into the final voltage on the regulator. Furthermore, we want to disable the regulator with regulator_disable() if all the clocks are off.
Hmm, we need to discuss more on this, maybe in a HO sometime or at connect :)

Sure.

Quoted 10 lines

Quoted 6 lines
This is also a problem with cpufreq-dt. The regulators and clocks are assumed to be enabled out of the bootloader, which may not even be true. Now that OPP layer is managing all the clocks and regulators here we're going to need to do something to make sure they're on and controllable.
Also, I fail to understand, how the clock of a 'online' CPU can be off?

Hmm right. The problem is the clock and regulator frameworks aren't aware that they're enabled already. In the case of the clocks, if we switch a mux from one source to another while frequency switching and the framework doesn't know the clock is on, it won't turn on the new source for the CPU, effectively killing the CPU.

In the regulator case I ran into a problem with qcom's RPM regulator implementation where the voltage isn't sent when the regulator is disabled (the driver has some check for that case).

I suppose in both cases these could be fixed in the frameworks if they detected on/off state at boot. That's fine, but it doesn't seem like that will work for things like GPU where the clock may not be on in the first place. If we don't want to clk_get() and regulator_get() in two places (OPP and drivers) then we'll need on/off in OPP code.

In the CPU case, we've turned clocks on and off when CPUs are hotplugged in and out on qcom platforms so that we can drop the clock and voltage requirements of a particular CPU. If we had that CPU driver on ARM platforms it would be similar to the GPU case then. We could get the clocks and regulators in two places and we could put the clock/regulator on/off code in the CPU driver instead of the OPP layer.

Signature · 2 lines

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