Date:   Tue, 25 Oct 2016 09:49:53 -0700
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Rafael Wysocki <rjw@...ysocki.net>, nm@...com,
        Viresh Kumar <vireshk@...nel.org>,
        linaro-kernel@...ts.linaro.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Vincent Guittot <vincent.guittot@...aro.org>, robh@...nel.org,
        d-gerlach@...com, broonie@...nel.org
Subject: Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple
 regulators

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);

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

Powered by blists - more mailing lists