On 24-10-16, 15:52, Stephen Boyd wrote:

On 10/20, Viresh Kumar wrote:

...

The OPP structure must not be used out of the rcu protected section. Cache the values to be used in separate variables instead.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

Was this found by visual inspection or through some static checker? Just curious.

Visual inspection :)

...

@@ -633,6 +634,14 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) return ret; }

• if (IS_ERR(old_opp)) {

• old_u_volt = 0;
  

• } else {

• old_u_volt = old_opp->u_volt;
  

• old_u_volt_min = old_opp->u_volt_min;
  

• old_u_volt_max = old_opp->u_volt_max;
  

• }
• u_volt = opp->u_volt; u_volt_min = opp->u_volt_min; u_volt_max = opp->u_volt_max;

@@ -677,9 +686,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) __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, old_opp->u_volt,
  

• 		 old_opp->u_volt_min, old_opp->u_volt_max);
  

• if (old_u_volt) {

What if old_u_volt == 0 is valid?

How can that be valid ?

We could have another variable like 'valid' or something that we use to figure out if we should set values instead. Then this isn't a potential pitfall.

I can do that but just wanted to know if we need that or not.

Hi, On 10/20/2016 03:44 AM, Viresh Kumar wrote:

This patch adds infrastructure to manage multiple regulators and updates the only user (cpufreq-dt) of dev_pm_opp_set{put}_regulator().

This is preparatory work for adding full support for devices with multiple regulators.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

drivers/base/power/opp/core.c | 220 ++++++++++++++++++++++++++------------- drivers/base/power/opp/debugfs.c | 48 +++++++-- drivers/base/power/opp/of.c | 102 +++++++++++++----- drivers/base/power/opp/opp.h | 10 +- drivers/cpufreq/cpufreq-dt.c | 9 +- include/linux/pm_opp.h | 8 +- 6 files changed, 276 insertions(+), 121 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 37fad2eb0f47..45c70ce07864 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -93,6 +93,8 @@ struct opp_table *_find_opp_table(struct device *dev)

• Return: voltage in micro volt corresponding to the opp, else
• return 0

• • This is useful only for devices with single power supply.
• • Locking: This function must be called under rcu_read_lock(). opp is a rcu
  • protected pointer. This means that opp which could have been fetched by
  • opp_find_freq_{exact,ceil,floor} functions is valid as long as we are

@@ -112,7 +114,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp) if (IS_ERR_OR_NULL(tmp_opp)) pr_err("%s: Invalid parameters\n", __func__); else

• v = tmp_opp->supply.u_volt;
  

• v = tmp_opp->supplies[0].u_volt;
  
  

  return v;

} @@ -222,10 +224,10 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) { struct opp_table *opp_table; struct dev_pm_opp *opp;

• struct regulator *reg;

• struct regulator *reg, **regulators; unsigned long latency_ns = 0;

• unsigned long min_uV = ~0, max_uV = 0;
• int ret;

• unsigned long *min_uV, *max_uV;

• int ret, size, i, count;

  rcu_read_lock();

@@ -235,21 +237,44 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) return 0; }

• reg = opp_table->regulator;
• if (IS_ERR(reg)) {

• count = opp_table->regulator_count;
• if (!count) { /* Regulator may not be required for device */ rcu_read_unlock(); return 0; }

• list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {

• if (!opp->available)
  

• 	continue;
  

• size = count * sizeof(*regulators);
• regulators = kmemdup(opp_table->regulators, size, GFP_KERNEL);
• if (!regulators) {

• rcu_read_unlock();
  

• return 0;
  

• }
• min_uV = kmalloc(count * (sizeof(*min_uV) + sizeof(*max_uV)),

• 	 GFP_KERNEL);
  

• if (!min_uV) {

• kfree(regulators);
  

• rcu_read_unlock();
  

• return 0;
  

• }
• max_uV = min_uV + count;
• for (i = 0; i < count; i++) {

• min_uV[i] = ~0;
  

• max_uV[i] = 0;
  

• list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
  

• 	if (!opp->available)
  

• 		continue;
  
  

• if (opp->supply.u_volt_min < min_uV)
  

• 	min_uV = opp->supply.u_volt_min;
  

• if (opp->supply.u_volt_max > max_uV)
  

• 	max_uV = opp->supply.u_volt_max;
  

• 	if (opp->supplies[i].u_volt_min < min_uV[i])
  

• 		min_uV[i] = opp->supplies[i].u_volt_min;
  

• 	if (opp->supplies[i].u_volt_max > max_uV[i])
  

• 		max_uV[i] = opp->supplies[i].u_volt_max;
  

• }
  

  }

  rcu_read_unlock();

@@ -258,9 +283,14 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) * The caller needs to ensure that opp_table (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;
  

• for (i = 0; reg = regulators[i], i < count; i++) {

• ret = regulator_set_voltage_time(reg, min_uV[i], max_uV[i]);
  

• if (ret > 0)
  

• 	latency_ns += ret * 1000;
  

• }

• kfree(min_uV);

• kfree(regulators);

  return latency_ns;

} @@ -580,7 +610,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) { struct opp_table *opp_table; struct dev_pm_opp *old_opp, *opp;

• struct regulator *reg;

• struct regulator *reg = ERR_PTR(-ENXIO); struct clk *clk; unsigned long freq, old_freq; struct dev_pm_opp_supply old_supply, new_supply;

@@ -633,14 +663,23 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) return ret; }

• if (opp_table->regulators) {

• /* This function only supports single regulator per device */
  

• if (WARN_ON(opp_table->regulator_count > 1)) {
  

• 	dev_err(dev, "multiple regulators not supported\n");
  

• 	rcu_read_unlock();
  

• 	return -EINVAL;
  

• }
  

• reg = opp_table->regulators[0];
  

• }
• if (IS_ERR(old_opp)) old_supply.u_volt = 0; else

• memcpy(&old_supply, &old_opp->supply, sizeof(old_supply));
  

• memcpy(&old_supply, old_opp->supplies, sizeof(old_supply));
  
  

• memcpy(&new_supply, &opp->supply, sizeof(new_supply));
• reg = opp_table->regulator;

• memcpy(&new_supply, opp->supplies, sizeof(new_supply));

  rcu_read_unlock();

@@ -764,9 +803,6 @@ static struct opp_table *_add_opp_table(struct device *dev)

_of_init_opp_table(opp_table, dev);

• /* Set regulator to a non-NULL error value */
• opp_table->regulator = ERR_PTR(-ENXIO);
• /* Find clk for the device */ opp_table->clk = clk_get(dev, NULL); if (IS_ERR(opp_table->clk)) {

@@ -815,7 +851,7 @@ static void _remove_opp_table(struct opp_table *opp_table) if (opp_table->prop_name) return;

• if (!IS_ERR(opp_table->regulator))

• if (opp_table->regulators) return;

  /* Release clk */

@@ -924,35 +960,49 @@ struct dev_pm_opp *_allocate_opp(struct device *dev, struct opp_table **opp_table) { struct dev_pm_opp *opp;

• int count, supply_size;
• struct opp_table *table;

• /* allocate new OPP node */
• opp = kzalloc(sizeof(*opp), GFP_KERNEL);
• if (!opp)

• table = _add_opp_table(dev);
• if (!table) return NULL;

• INIT_LIST_HEAD(&opp->node);

• /* Allocate space for at least one supply */
• count = table->regulator_count ? table->regulator_count : 1;
• supply_size = sizeof(*opp->supplies) * count;

• *opp_table = _add_opp_table(dev);
• if (!*opp_table) {

• kfree(opp);
  

• /* allocate new OPP node + and supplies structures */

• opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL);

• if (!opp) {

• kfree(table);
  

  return NULL; }

• opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);

• INIT_LIST_HEAD(&opp->node);

• *opp_table = table;

• return opp;

}

static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, struct opp_table *opp_table) {

• struct regulator *reg = opp_table->regulator;
• if (!IS_ERR(reg) &&

•    !regulator_is_supported_voltage(reg, opp->supply.u_volt_min,
  

• 			    opp->supply.u_volt_max)) {
  

• pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
  

• 	__func__, opp->supply.u_volt_min,
  

• 	opp->supply.u_volt_max);
  

• return false;
  

• struct regulator *reg;

• int i;

• for (i = 0; i < opp_table->regulator_count; i++) {

• reg = opp_table->regulators[i];
  

• if (!regulator_is_supported_voltage(reg,
  

• 			opp->supplies[i].u_volt_min,
  

• 			opp->supplies[i].u_volt_max)) {
  

• 	pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
  

• 		__func__, opp->supplies[i].u_volt_min,
  

• 		opp->supplies[i].u_volt_max);
  

• 	return false;
  

• }
  

  }

  return true;

@@ -984,12 +1034,13 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,

/* Duplicate OPPs */
dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",

• 	 __func__, opp->rate, opp->supply.u_volt,
  

• 	 opp->available, new_opp->rate, new_opp->supply.u_volt,
  

• 	 new_opp->available);
  

• 	 __func__, opp->rate, opp->supplies[0].u_volt,
  

• 	 opp->available, new_opp->rate,
  

• 	 new_opp->supplies[0].u_volt, new_opp->available);
  
  

• /* Should we compare voltages for all regulators here ? */
  

  return opp->available &&

•        new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST;
  

•        new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? 0 : -EEXIST;
  

  }

  new_opp->opp_table = opp_table;

@@ -1056,9 +1107,9 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, /* populate the opp table */ new_opp->rate = freq; tol = u_volt * opp_table->voltage_tolerance_v1 / 100;

• new_opp->supply.u_volt = u_volt;
• new_opp->supply.u_volt_min = u_volt - tol;
• new_opp->supply.u_volt_max = u_volt + tol;

• new_opp->supplies[0].u_volt = u_volt;
• new_opp->supplies[0].u_volt_min = u_volt - tol;
• new_opp->supplies[0].u_volt_max = u_volt + tol; new_opp->available = true; new_opp->dynamic = dynamic;

@@ -1303,12 +1354,14 @@ 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_pm_opp_set_regulators() - Set regulator names for the device
  • @dev: Device for which regulator name is being set.

• • @name: Name of the regulator.

• • @names: Array of pointers to the names of the regulator.
• • @count: Number of regulators.
  • 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.

• • device's regulators, as the core would be required to switch voltages as
• • well.
  • This must be called before any OPPs are initialized for the device.

@@ -1318,11 +1371,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);

• 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) +int dev_pm_opp_set_regulators(struct device *dev, const char *names[],

• 	      unsigned int count)
  

{ struct opp_table *opp_table; struct regulator *reg;

• int ret;

• int ret, i;

  mutex_lock(&opp_table_lock);

@@ -1338,26 +1392,43 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name) goto err; }

• /* Already have a regulator set */
• if (WARN_ON(!IS_ERR(opp_table->regulator))) {

• /* Already have regulators set */
• if (WARN_ON(opp_table->regulators)) { 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);
  

• opp_table->regulators = kmalloc_array(count,

• 			      sizeof(*opp_table->regulators),
  

• 			      GFP_KERNEL);
  

• if (!opp_table->regulators) goto err;
• for (i = 0; i < count; i++) {

• reg = regulator_get_optional(dev, names[i]);
  

• pr_info("%s: %d: %p: %s\n", __func__, __LINE__, reg, names[i]);
  

Think this is leftover debug msg?

• if (IS_ERR(reg)) {
  

• 	ret = PTR_ERR(reg);
  

• 	if (ret != -EPROBE_DEFER)
  

• 		dev_err(dev, "%s: regulator (%s) not found: %d\n",
  

• 			__func__, names[i], ret);
  

• 	goto free_regulators;
  

• }
  

• opp_table->regulators[i] = reg;
  

  }

• opp_table->regulator = reg;

• opp_table->regulator_count = count;

  mutex_unlock(&opp_table_lock); return 0;

+free_regulators:

• while (i != 0)

• regulator_put(opp_table->regulators[--i]);
  

• kfree(opp_table->regulators);
• opp_table->regulators = NULL;

err: _remove_opp_table(opp_table); unlock: @@ -1365,11 +1436,11 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)

return ret; } -EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator); +EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulators);

/**

• • dev_pm_opp_put_regulator() - Releases resources blocked for regulator
• • @dev: Device for which regulator was set.

• • dev_pm_opp_put_regulators() - Releases resources blocked for regulators
• • @dev: Device for which regulators were set.
  • Locking: The internal opp_table and opp structures are RCU protected.
  • Hence this function internally uses RCU updater strategy with mutex locks

@@ -1377,9 +1448,10 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);

• 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) +void dev_pm_opp_put_regulators(struct device *dev) { struct opp_table *opp_table;

• int i;

  mutex_lock(&opp_table_lock);

@@ -1391,16 +1463,20 @@ void dev_pm_opp_put_regulator(struct device *dev) goto unlock; }

• if (IS_ERR(opp_table->regulator)) {

• dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
  

• if (!opp_table->regulators) {

• dev_err(dev, "%s: Doesn't have regulators set\n", __func__);
  

  goto unlock; }

  /* Make sure there are no concurrent readers while updating opp_table */ WARN_ON(!list_empty(&opp_table->opp_list));

• regulator_put(opp_table->regulator);
• opp_table->regulator = ERR_PTR(-ENXIO);

• for (i = opp_table->regulator_count - 1; i >= 0; i--)

• regulator_put(opp_table->regulators[i]);
  

• kfree(opp_table->regulators);

• opp_table->regulators = NULL;

• opp_table->regulator_count = 0;

  /* Try freeing opp_table if this was the last blocking resource */ _remove_opp_table(opp_table);

@@ -1408,7 +1484,7 @@ void dev_pm_opp_put_regulator(struct device *dev) unlock: mutex_unlock(&opp_table_lock); } -EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulator); +EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);

/**

• dev_pm_opp_add() - Add an OPP table from a table definitions

diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c index c897676ca35f..cb5e5fde3d39 100644 --- a/drivers/base/power/opp/debugfs.c +++ b/drivers/base/power/opp/debugfs.c @@ -34,6 +34,43 @@ void opp_debug_remove_one(struct dev_pm_opp *opp) debugfs_remove_recursive(opp->dentry); }

+static bool opp_debug_create_supplies(struct dev_pm_opp *opp,

• 		      struct opp_table *opp_table,
  

• 		      struct dentry *pdentry)
  

+{

• struct dentry *d;
• int i = 0;
• char name[] = "supply-X"; /* support only 0-9 supplies */
• /* Always create at least supply-0 directory */
• do {

• name[7] = i + '0';
  

• /* Create per-opp directory */
  

• d = debugfs_create_dir(name, pdentry);
  

• if (!d)
  

• 	return false;
  

• if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d,
  

• 			  &opp->supplies[i].u_volt))
  

• 	return false;
  

• if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d,
  

• 			  &opp->supplies[i].u_volt_min))
  

• 	return false;
  

• if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d,
  

• 			  &opp->supplies[i].u_volt_max))
  

• 	return false;
  

• if (!debugfs_create_ulong("u_amp", S_IRUGO, d,
  

• 			  &opp->supplies[i].u_amp))
  

• 	return false;
  

• } while (++i < opp_table->regulator_count);
• return true;

+}

int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) { struct dentry *pdentry = opp_table->dentry; @@ -63,16 +100,7 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate)) return -ENOMEM;

• if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt))

• return -ENOMEM;
  

• if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min))

• return -ENOMEM;
  

• if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max))

• return -ENOMEM;
  

• if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp))

• if (!opp_debug_create_supplies(opp, opp_table, d)) return -ENOMEM;

  if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d,

diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c index b7fcd0a1b58b..c857fb07a5bc 100644 --- a/drivers/base/power/opp/of.c +++ b/drivers/base/power/opp/of.c @@ -17,6 +17,7 @@ #include <linux/errno.h> #include <linux/device.h> #include <linux/of.h> +#include <linux/slab.h> #include <linux/export.h>

#include "opp.h" @@ -105,12 +106,13 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,

Though not in the patch there's a comment to

/* TODO: Support multiple regulators */

in the file right above the below opp_parse_supplies function that can probably be removed as part of this patch.

Otherwise this patch looks good to me.

Regards, Dave

static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, struct opp_table *opp_table) {

• u32 microvolt[3] = {0};
• u32 val;
• int count, ret;

• u32 *microvolt, *microamp = NULL;

• int supplies, vcount, icount, ret, i, j; struct property *prop = NULL; char name[NAME_MAX];

• supplies = opp_table->regulator_count ? opp_table->regulator_count : 1;

• /* Search for "opp-microvolt-<name>" */ if (opp_table->prop_name) { snprintf(name, sizeof(name), "opp-microvolt-%s",

@@ -128,34 +130,29 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, return 0; }

• count = of_property_count_u32_elems(opp->np, name);
• if (count < 0) {

• vcount = of_property_count_u32_elems(opp->np, name);
• if (vcount < 0) { dev_err(dev, "%s: Invalid %s property (%d)\n",

• 	__func__, name, count);
  

• return count;
  

• 	__func__, name, vcount);
  

• return vcount;
  

  }

• /* There can be one or three elements here */
• if (count != 1 && count != 3) {

• dev_err(dev, "%s: Invalid number of elements in %s property (%d)\n",
  

• 	__func__, name, count);
  

• /* There can be one or three elements per supply */
• if (vcount != supplies && vcount != supplies * 3) {

• dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n",
  

• 	__func__, name, vcount, supplies);
  

  return -EINVAL; }

• ret = of_property_read_u32_array(opp->np, name, microvolt, count);

• microvolt = kmalloc_array(vcount, sizeof(*microvolt), GFP_KERNEL);
• if (!microvolt)

• return -ENOMEM;
  

• ret = of_property_read_u32_array(opp->np, name, microvolt, vcount); if (ret) { dev_err(dev, "%s: error parsing %s: %d\n", __func__, name, ret);

• return -EINVAL;
  

• }
• opp->supply.u_volt = microvolt[0];
• if (count == 1) {

• opp->supply.u_volt_min = opp->supply.u_volt;
  

• opp->supply.u_volt_max = opp->supply.u_volt;
  

• } else {

• opp->supply.u_volt_min = microvolt[1];
  

• opp->supply.u_volt_max = microvolt[2];
  

• ret = -EINVAL;
  

• goto free_microvolt;
  

  }

  /* Search for "opp-microamp-<name>" */

@@ -172,10 +169,59 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, prop = of_find_property(opp->np, name, NULL); }

• if (prop && !of_property_read_u32(opp->np, name, &val))

• opp->supply.u_amp = val;
  

• if (prop) {

• icount = of_property_count_u32_elems(opp->np, name);
  

• if (icount < 0) {
  

• 	dev_err(dev, "%s: Invalid %s property (%d)\n", __func__,
  

• 		name, icount);
  

• 	ret = icount;
  

• 	goto free_microvolt;
  

• }
  
  

• return 0;

• if (icount != supplies) {
  

• 	dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n",
  

• 		__func__, name, icount, supplies);
  

• 	ret = -EINVAL;
  

• 	goto free_microvolt;
  

• }
  

• microamp = kmalloc_array(icount, sizeof(*microamp), GFP_KERNEL);
  

• if (!microamp) {
  

• 	ret = -EINVAL;
  

• 	goto free_microvolt;
  

• }
  

• ret = of_property_read_u32_array(opp->np, name, microamp,
  

• 				 icount);
  

• if (ret) {
  

• 	dev_err(dev, "%s: error parsing %s: %d\n", __func__,
  

• 		name, ret);
  

• 	ret = -EINVAL;
  

• 	goto free_microamp;
  

• }
  

• }
• for (i = 0, j = 0; i < supplies; i++) {

• opp->supplies[i].u_volt = microvolt[j++];
  

• if (vcount == supplies) {
  

• 	opp->supplies[i].u_volt_min = opp->supplies[i].u_volt;
  

• 	opp->supplies[i].u_volt_max = opp->supplies[i].u_volt;
  

• } else {
  

• 	opp->supplies[i].u_volt_min = microvolt[j++];
  

• 	opp->supplies[i].u_volt_max = microvolt[j++];
  

• }
  

• if (microamp)
  

• 	opp->supplies[i].u_amp = microamp[i];
  

• }

+free_microamp:

• kfree(microamp);

+free_microvolt:

• kfree(microvolt);
• return ret;

}

/** @@ -304,8 +350,8 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)

pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n", __func__, new_opp->turbo, new_opp->rate,

•  new_opp->supply.u_volt, new_opp->supply.u_volt_min,
  

•  new_opp->supply.u_volt_max, new_opp->clock_latency_ns);
  

•  new_opp->supplies[0].u_volt, new_opp->supplies[0].u_volt_min,
  

•  new_opp->supplies[0].u_volt_max, new_opp->clock_latency_ns);
  
  

  /*

  • Notify the changes in the availability of the operable

diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index 1bda0d35c486..d3f0861f9bff 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -77,7 +77,7 @@ struct dev_pm_opp_supply {

• @turbo: true if turbo (boost) OPP
• @suspend: true if suspend OPP
• @rate: Frequency in hertz

• • @supply: Power supply voltage/current values

• • @supplies: Power supplies voltage/current values
  • @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's

  • frequency from any other OPP's frequency.
    

  • @opp_table: points back to the opp_table struct this opp belongs to

@@ -96,7 +96,7 @@ struct dev_pm_opp { bool suspend; unsigned long rate;

• struct dev_pm_opp_supply supply;

• struct dev_pm_opp_supply *supplies;

  unsigned long clock_latency_ns;

@@ -155,7 +155,8 @@ enum opp_table_access {

• @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

• • @regulators: Supply regulators
• • @regulator_count: Number of Power Supply regulators
  • @dentry: debugfs dentry pointer of the real device directory (not links).
  • @dentry_name: Name of the real dentry.

@@ -190,7 +191,8 @@ struct opp_table { unsigned int supported_hw_count; const char *prop_name; struct clk *clk;

• struct regulator *regulator;

• struct regulator **regulators;
• unsigned int regulator_count;

#ifdef CONFIG_DEBUG_FS struct dentry *dentry; diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 5c07ae05d69a..15cb26118dc7 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -186,7 +186,10 @@ static int cpufreq_init(struct cpufreq_policy *policy) */ name = find_supply_name(cpu_dev); if (name) {

• ret = dev_pm_opp_set_regulator(cpu_dev, name);
  

• const char *names[] = {name};
  

• ret = dev_pm_opp_set_regulators(cpu_dev, names,
  

• 				ARRAY_SIZE(names));
  

  if (ret) { dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n", policy->cpu, ret);

@@ -285,7 +288,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) out_free_opp: dev_pm_opp_of_cpumask_remove_table(policy->cpus); if (name)

• dev_pm_opp_put_regulator(cpu_dev);
  

• dev_pm_opp_put_regulators(cpu_dev);
  

out_put_clk: clk_put(cpu_clk);

@@ -300,7 +303,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy) 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);
  

• dev_pm_opp_put_regulators(priv->cpu_dev);
  
  

  clk_put(policy->clk); kfree(priv);

diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index bca26157f5b6..0606b70a8b97 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -62,8 +62,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); +int dev_pm_opp_set_regulators(struct device *dev, const char *names[], unsigned int count); +void dev_pm_opp_put_regulators(struct device *dev); int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq); int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask); int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask); @@ -170,12 +170,12 @@ 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) +static inline int dev_pm_opp_set_regulators(struct device *dev, const char *names[], unsigned int count) { return -ENOTSUPP; }

-static inline void dev_pm_opp_put_regulator(struct device *dev) {} +static inline void dev_pm_opp_put_regulators(struct device *dev) {}

static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) {

On 10/20, Viresh Kumar wrote:

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 37fad2eb0f47..45c70ce07864 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -235,21 +237,44 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) return 0; }

• reg = opp_table->regulator;
• if (IS_ERR(reg)) {

• count = opp_table->regulator_count;
• if (!count) { /* Regulator may not be required for device */ rcu_read_unlock(); return 0; }

• list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {

• if (!opp->available)
  

• 	continue;
  

• size = count * sizeof(*regulators);
• regulators = kmemdup(opp_table->regulators, size, GFP_KERNEL);

How do we allocate under RCU? Doesn't that spit out big warnings?

• if (!regulators) {

• rcu_read_unlock();
  

• return 0;
  

• }
• min_uV = kmalloc(count * (sizeof(*min_uV) + sizeof(*max_uV)),

Do we imagine min_uV is going to be a different size from max_uV? It may be better to have a struct for min/max pair and then stride them. Then the kmalloc() can become a kmalloc_array().

• 	 GFP_KERNEL);
  

• if (!min_uV) {

• kfree(regulators);
  

• rcu_read_unlock();
  

• return 0;
  

• }
• max_uV = min_uV + count;
• for (i = 0; i < count; i++) {

• min_uV[i] = ~0;
  

• max_uV[i] = 0;
  

• list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
  

• 	if (!opp->available)
  

• 		continue;
  

• if (opp->supply.u_volt_min < min_uV)
  

• 	min_uV = opp->supply.u_volt_min;
  

• if (opp->supply.u_volt_max > max_uV)
  

• 	max_uV = opp->supply.u_volt_max;
  

• 	if (opp->supplies[i].u_volt_min < min_uV[i])
  

• 		min_uV[i] = opp->supplies[i].u_volt_min;
  

• 	if (opp->supplies[i].u_volt_max > max_uV[i])
  

• 		max_uV[i] = opp->supplies[i].u_volt_max;
  

• }
  

  }

rcu_read_unlock(); @@ -924,35 +960,49 @@ struct dev_pm_opp *_allocate_opp(struct device *dev, struct opp_table **opp_table) { struct dev_pm_opp *opp;

• int count, supply_size;
• struct opp_table *table;

• /* allocate new OPP node */
• opp = kzalloc(sizeof(*opp), GFP_KERNEL);
• if (!opp)

• table = _add_opp_table(dev);
• if (!table) return NULL;

• INIT_LIST_HEAD(&opp->node);

• /* Allocate space for at least one supply */
• count = table->regulator_count ? table->regulator_count : 1;
• supply_size = sizeof(*opp->supplies) * count;

• *opp_table = _add_opp_table(dev);
• if (!*opp_table) {

• kfree(opp);
  

• /* allocate new OPP node + and supplies structures */
• opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL);
• if (!opp) {

• kfree(table);
  

  return NULL; }

• opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);

So put the supplies at the end of the OPP structure as an empty array?

• INIT_LIST_HEAD(&opp->node);
• *opp_table = table;
• return opp;

} static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, struct opp_table *opp_table) {

• struct regulator *reg = opp_table->regulator;
• if (!IS_ERR(reg) &&

•    !regulator_is_supported_voltage(reg, opp->supply.u_volt_min,
  

• 			    opp->supply.u_volt_max)) {
  

• pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
  

• 	__func__, opp->supply.u_volt_min,
  

• 	opp->supply.u_volt_max);
  

• return false;
  

• struct regulator *reg;
• int i;
• for (i = 0; i < opp_table->regulator_count; i++) {

• reg = opp_table->regulators[i];
  

• if (!regulator_is_supported_voltage(reg,
  

• 			opp->supplies[i].u_volt_min,
  

• 			opp->supplies[i].u_volt_max)) {
  

• 	pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
  

• 		__func__, opp->supplies[i].u_volt_min,
  

• 		opp->supplies[i].u_volt_max);
  

• 	return false;
  

• }
  

  }

return true; @@ -984,12 +1034,13 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, /* Duplicate OPPs */ dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",

• 	 __func__, opp->rate, opp->supply.u_volt,
  

• 	 opp->available, new_opp->rate, new_opp->supply.u_volt,
  

• 	 new_opp->available);
  

• 	 __func__, opp->rate, opp->supplies[0].u_volt,
  

• 	 opp->available, new_opp->rate,
  

• 	 new_opp->supplies[0].u_volt, new_opp->available);
  

• /* Should we compare voltages for all regulators here ? */
  

  return opp->available &&

•        new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST;
  

•        new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? 0 : -EEXIST;
  

  }

new_opp->opp_table = opp_table; @@ -1303,12 +1354,14 @@ 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_pm_opp_set_regulators() - Set regulator names for the device
  • @dev: Device for which regulator name is being set.

• • @name: Name of the regulator.

• • @names: Array of pointers to the names of the regulator.
• • @count: Number of regulators.
  • 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.

• • device's regulators, as the core would be required to switch voltages as
• • well.
  • This must be called before any OPPs are initialized for the device.

@@ -1318,11 +1371,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);

• 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) +int dev_pm_opp_set_regulators(struct device *dev, const char *names[],

Make names a const char * const *? I seem to recall arrays as function arguments has some problem but my C mastery is failing right now.

• 	      unsigned int count)
  

{ struct opp_table *opp_table; struct regulator *reg;

• int ret;

• int ret, i;

mutex_lock(&opp_table_lock); @@ -1338,26 +1392,43 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name) goto err; }

• /* Already have a regulator set */
• if (WARN_ON(!IS_ERR(opp_table->regulator))) {

• /* Already have regulators set */
• if (WARN_ON(opp_table->regulators)) { 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);
  

• opp_table->regulators = kmalloc_array(count,

• 			      sizeof(*opp_table->regulators),
  

• 			      GFP_KERNEL);
  

• if (!opp_table->regulators) goto err;
• for (i = 0; i < count; i++) {

• reg = regulator_get_optional(dev, names[i]);
  

• pr_info("%s: %d: %p: %s\n", __func__, __LINE__, reg, names[i]);
  

Debug noise?

• if (IS_ERR(reg)) {
  

• 	ret = PTR_ERR(reg);
  

• 	if (ret != -EPROBE_DEFER)
  

• 		dev_err(dev, "%s: regulator (%s) not found: %d\n",
  

• 			__func__, names[i], ret);
  

• 	goto free_regulators;
  

• }
  

• opp_table->regulators[i] = reg;
  

  }

• opp_table->regulator = reg;

• opp_table->regulator_count = count;

mutex_unlock(&opp_table_lock); return 0; +free_regulators:

• while (i != 0)

• regulator_put(opp_table->regulators[--i]);
  

• kfree(opp_table->regulators);
• opp_table->regulators = NULL;

err: _remove_opp_table(opp_table); unlock: diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c index c897676ca35f..cb5e5fde3d39 100644 --- a/drivers/base/power/opp/debugfs.c +++ b/drivers/base/power/opp/debugfs.c @@ -34,6 +34,43 @@ void opp_debug_remove_one(struct dev_pm_opp *opp) debugfs_remove_recursive(opp->dentry); } +static bool opp_debug_create_supplies(struct dev_pm_opp *opp,

• 		      struct opp_table *opp_table,
  

• 		      struct dentry *pdentry)
  

+{

• struct dentry *d;
• int i = 0;
• char name[] = "supply-X"; /* support only 0-9 supplies */

But we don't verify that's the case? Why not use kasprintf() and free() and then there isn't any limit?

• /* Always create at least supply-0 directory */
• do {

• name[7] = i + '0';
  

• /* Create per-opp directory */
  

• d = debugfs_create_dir(name, pdentry);
  

• if (!d)
  

• 	return false;
  

• if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d,
  

• 			  &opp->supplies[i].u_volt))
  

• 	return false;
  

• if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d,
  

• 			  &opp->supplies[i].u_volt_min))
  

• 	return false;
  

• if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d,
  

• 			  &opp->supplies[i].u_volt_max))
  

• 	return false;
  

• if (!debugfs_create_ulong("u_amp", S_IRUGO, d,
  

• 			  &opp->supplies[i].u_amp))
  

• 	return false;
  

• } while (++i < opp_table->regulator_count);
• return true;

+}

int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) { struct dentry *pdentry = opp_table->dentry; diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c index b7fcd0a1b58b..c857fb07a5bc 100644 --- a/drivers/base/power/opp/of.c +++ b/drivers/base/power/opp/of.c @@ -105,12 +106,13 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table, static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, struct opp_table *opp_table) {

• u32 microvolt[3] = {0};
• u32 val;
• int count, ret;

• u32 *microvolt, *microamp = NULL;
• int supplies, vcount, icount, ret, i, j; struct property *prop = NULL; char name[NAME_MAX];

• supplies = opp_table->regulator_count ? opp_table->regulator_count : 1;

We can't have regulator_count == 1 by default?

• /* Search for "opp-microvolt-<name>" */ if (opp_table->prop_name) { snprintf(name, sizeof(name), "opp-microvolt-%s",

@@ -155,7 +155,8 @@ enum opp_table_access {

• @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

• • @regulators: Supply regulators
• • @regulator_count: Number of Power Supply regulators

Lowercase Power Supply please.

• @dentry: debugfs dentry pointer of the real device directory (not links).
• @dentry_name: Name of the real dentry.

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 5c07ae05d69a..15cb26118dc7 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -186,7 +186,10 @@ static int cpufreq_init(struct cpufreq_policy *policy) */ name = find_supply_name(cpu_dev); if (name) {

• ret = dev_pm_opp_set_regulator(cpu_dev, name);
  

• const char *names[] = {name};
  

• ret = dev_pm_opp_set_regulators(cpu_dev, names,
  

We can't just do &names instead here? Hmm...

• 				ARRAY_SIZE(names));
  

  if (ret) { dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n", policy->cpu, ret);

On 10/26, Viresh Kumar wrote:

On 25-10-16, 09:49, Stephen Boyd wrote:

...

On 10/20, Viresh Kumar wrote:

...

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 37fad2eb0f47..45c70ce07864 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -235,21 +237,44 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) return 0; }

• reg = opp_table->regulator;
• if (IS_ERR(reg)) {

• count = opp_table->regulator_count;
• if (!count) { /* Regulator may not be required for device */ rcu_read_unlock(); return 0; }

• list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {

• if (!opp->available)
  

• 	continue;
  

• size = count * sizeof(*regulators);
• regulators = kmemdup(opp_table->regulators, size, GFP_KERNEL);

How do we allocate under RCU? Doesn't that spit out big warnings?

That doesn't. I even tried enabling several locking debug config options.

Please read RCU documentation. From rcu_read_lock() function documentation:

In non-preemptible RCU implementations (TREE_RCU and TINY_RCU), it is illegal to block while in an RCU read-side critical section. In preemptible RCU implementations (TREE_PREEMPT_RCU) in CONFIG_PREEMPT kernel builds, RCU read-side critical sections may be preempted, but explicit blocking is illegal. Finally, in preemptible RCU implementations in real-time (with -rt patchset) kernel builds, RCU read-side critical sections may be preempted and they may also block, but only when acquiring spinlocks that are subject to priority inheritance.

I suppose that in certain configurations it will warn and in others it won't. I thought lockdep would always complain though, so that's sad it doesn't. At least in some implementations of RCU rcu_read_lock() is the same as preempt_disable(), which basically means no sleeping calls like GFP_KERNEL allocations.

On 10/20, Viresh Kumar wrote:

Later patches would add support for custom opp_set_rate callbacks. This

I know the OPP consumer function has "rate" in the name, but it makes more sense to call the callback set_opp instead because we could be doing a lot more than setting the opp rate.

patch separates out the code for generic opp_set_rate handler in order to prepare for that.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 45c70ce07864..96f04392daef 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -596,6 +596,73 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg, return ret; } +static inline int +_generic_opp_set_rate_clk_only(struct device *dev, struct clk *clk,

• 	       unsigned long old_freq, unsigned long freq)
  

+{

• int ret;
• /* Change frequency */
• dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",

• __func__, old_freq, freq);
  

Perhaps this should stay at the beginning of OPP transitions? Otherwise it can get confusing when multiple switching OPP messages appear on OPP transition failures.

• ret = clk_set_rate(clk, freq);
• if (ret) {

• dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
  

• 	ret);
  

• }
• return ret;

+}

diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index d3f0861f9bff..6629c53c0aa1 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -47,22 +47,6 @@ extern struct list_head opp_tables; */ /**

• • struct dev_pm_opp_supply - Power supply voltage/current values
• • @u_volt: Target voltage in microvolts corresponding to this OPP
• • @u_volt_min: Minimum voltage in microvolts corresponding to this OPP
• • @u_volt_max: Maximum voltage in microvolts corresponding to this OPP
• • @u_amp: Maximum current drawn by the device in microamperes
• • This structure stores the voltage/current values for a single power supply.
• */

-struct dev_pm_opp_supply {

• unsigned long u_volt;
• unsigned long u_volt_min;
• unsigned long u_volt_max;
• unsigned long u_amp;

-};

-/**

• struct dev_pm_opp - Generic OPP description structure
• @node: opp table node. The nodes are maintained throughout the lifetime

• of boot. It is expected only an optimal set of OPPs are
  

diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 0606b70a8b97..73713a8424b1 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -17,6 +17,7 @@ #include <linux/err.h> #include <linux/notifier.h> +struct clk;

Is struct regulator also forward declared?

struct dev_pm_opp; struct device; @@ -24,6 +25,36 @@ enum dev_pm_opp_event { OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE, }; +/**

• • struct dev_pm_opp_supply - Power supply voltage/current values
• • @u_volt: Target voltage in microvolts corresponding to this OPP
• • @u_volt_min: Minimum voltage in microvolts corresponding to this OPP
• • @u_volt_max: Maximum voltage in microvolts corresponding to this OPP
• • @u_amp: Maximum current drawn by the device in microamperes
• • This structure stores the voltage/current values for a single power supply.
• */

+struct dev_pm_opp_supply {

• unsigned long u_volt;
• unsigned long u_volt_min;
• unsigned long u_volt_max;
• unsigned long u_amp;

+};

This structure moved during this series. Can we avoid that and already have it in the right place to begin with?

+struct dev_pm_opp_info {

• unsigned long rate;
• struct dev_pm_opp_supply *supplies;

+};

+struct dev_pm_set_rate_data {

dev_pm_set_opp_data?

• struct dev_pm_opp_info old_opp;
• struct dev_pm_opp_info new_opp;
• struct regulator **regulators;
• unsigned int regulator_count;
• struct clk *clk;

+};

The above two structures don't get kernel doc?

#if defined(CONFIG_PM_OPP) unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp); -- 2.7.1.410.g6faf27b

On 23-10-16, 20:08, Dave Gerlach wrote:

Overall this series looks good to me apart from a few small things. Most importantly I was able to get a working implementation using two regulators on ti dra7xx platform with proper sequencing built on top of this series. We have cpu regulator and Adaptive body bias (abb) regulator that must be scaled in a certain order before or after clock scaling and I was able to implement a rough custom set_rate to perform this and ran some dvfs stress tests that all worked fine.

Thanks for testing it buddy.

First comment, I think the platform specific set_rate is a good place to hook in for adaptive voltage scaling as well. I was able to implement TI Class0 AVS in the same code by using the requested transition voltage as a reference and programming AVS voltage using that, along with scaling the additional regulators in sequence (the original multi regulator functionality).

Hmm, interesting..

I would think some people would want to use this even with single regulator platforms, no?

Maybe, but I would like to see such user code first. It may be possible to handle much of AVS stuff in core so that everyone isn't required to do it.

This raises some concerns about dependencies/probe sequencing. Right now we just need to make sure the cpufreq-dt driver probes after we have called _set_regulators, but if our platform code fails cpufreq-dt currently will treat this as no regulator needed for the platform and operate without one, which will likely hang the system. Is there a good way to to guarantee this doesn't happen? My main concern is that if we plan to provide a platform specific set-rate function, we should have a way to indicate this and prevent things from progressing if it isn't yet ready.

Again, overall I think it solves the multi regulator problem, and it works well for AVS as well. For the series:

Tested-by: Dave Gerlach d-gerlach@ti.com

Thanks.

For the concern you shared about, does the below patch fix it ? I will include that in V3 then.

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

From: Viresh Kumar viresh.kumar@linaro.org Date: Mon, 24 Oct 2016 09:45:30 +0530 Subject: [PATCH] PM / OPP: Don't assume platform doesn't have regulators

If the regulators aren't set explicitly by the platform, the OPP core assumes that the platform doesn't have any regulator and uses the clk-only callback.

If the platform failed to register a regulator with the core, then this can turn out to be a dangerous assumption as the OPP core will try to change clk without changing regulators.

Handle that properly by making sure that the DT didn't had any entries for supply voltages as well.

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

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index b69908b74ed6..fb4250532180 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -737,7 +737,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)

/* Only frequency scaling */ if (!regulators) { - rcu_read_unlock(); + /* + * DT contained supply ratings? Consider platform failed to set + * regulators. + */ + if (unlikely(opp->supplies[0].u_volt)) { + rcu_read_unlock(); + dev_err(dev, "%s: Regulator not registered with OPP core\n", + __func__); + return -EINVAL; + } + return _generic_opp_set_rate_clk_only(dev, clk, old_freq, freq); }