Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators

[Viresh Kumar <viresh.kumar@linaro.org>]

On 21-10-16, 17:32, Dave Gerlach wrote:
> 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?

Yes.

> > +		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.

Sure.

-- 
viresh