Re: [PATCH v7 4/7] OPP: Add support for parsing interconnect bandwidth

[Georgi Djakov]

Hi Matthias,

On 4/24/20 22:20, Matthias Kaehlcke wrote:
> Hi,
> 
> On Fri, Apr 24, 2020 at 06:54:01PM +0300, Georgi Djakov wrote:
>> The OPP bindings now support bandwidth values, so add support to parse it
>> from device tree and store it into the new dev_pm_opp_icc_bw struct, which
>> is part of the dev_pm_opp.
>>
>> Signed-off-by: Georgi Djakov <georgi.djakov@xxxxxxxxxx>
>> ---
>> v7:
>> * Addressed some review comments from Viresh and Sibi.
>> * Various other changes.
>>
>> v2: https://lore.kernel.org/linux-arm-msm/20190423132823.7915-4-georgi.djakov@xxxxxxxxxx/
>>
>>  drivers/opp/Kconfig    |   1 +
>>  drivers/opp/core.c     |  16 +++++-
>>  drivers/opp/of.c       | 119 ++++++++++++++++++++++++++++++++++++++++-
>>  drivers/opp/opp.h      |   9 ++++
>>  include/linux/pm_opp.h |  12 +++++
>>  5 files changed, 153 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/opp/Kconfig b/drivers/opp/Kconfig
>> index 35dfc7e80f92..230d2b84436c 100644
>> --- a/drivers/opp/Kconfig
>> +++ b/drivers/opp/Kconfig
>> @@ -2,6 +2,7 @@
>>  config PM_OPP
>>  	bool
>>  	select SRCU
>> +	depends on INTERCONNECT || !INTERCONNECT
> 
> huh?

Yeah, PM_OPP can be built-in only, but interconnect can be a module and in this
case i expect the linker to complain.

> 
>>  	---help---
>>  	  SOCs have a standard set of tuples consisting of frequency and
>>  	  voltage pairs that the device will support per voltage domain. This
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index c9c1bbe6ae27..8e86811eb7b2 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -985,6 +985,12 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>>  				ret);
>>  	}
>>  
>> +	/* Find interconnect path(s) for the device */
>> +	ret = _of_find_paths(opp_table, dev);
>> +	if (ret)
>> +		dev_dbg(dev, "%s: Error finding interconnect paths: %d\n",
>> +			__func__, ret);
> 
> why dev_dbg and not dev_warn?

Will make it dev_warn. Thanks!

>> +
>>  	BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
>>  	INIT_LIST_HEAD(&opp_table->opp_list);
>>  	kref_init(&opp_table->kref);
>> @@ -1229,19 +1235,22 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove_all_dynamic);
>>  struct dev_pm_opp *_opp_allocate(struct opp_table *table)
>>  {
>>  	struct dev_pm_opp *opp;
>> -	int count, supply_size;
>> +	int count, supply_size, icc_size;
>>  
>>  	/* Allocate space for at least one supply */
>>  	count = table->regulator_count > 0 ? table->regulator_count : 1;
>>  	supply_size = sizeof(*opp->supplies) * count;
>> +	icc_size = sizeof(*opp->bandwidth) * table->path_count;
>>  
>>  	/* allocate new OPP node and supplies structures */
>> -	opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL);
>> +	opp = kzalloc(sizeof(*opp) + supply_size + icc_size, GFP_KERNEL);
>> +
>>  	if (!opp)
>>  		return NULL;
>>  
>>  	/* Put the supplies at the end of the OPP structure as an empty array */
>>  	opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
>> +	opp->bandwidth = (struct dev_pm_opp_icc_bw *)(opp->supplies + 1);
> 
> IIUC this needs to be:
> 
> 	opp->bandwidth = (struct dev_pm_opp_icc_bw *)(opp->supplies + count);
> 
> maybe s/count/supply_count/

Right, thank you!

> 
>>  	INIT_LIST_HEAD(&opp->node);
>>  
>>  	return opp;
>> @@ -1276,6 +1285,9 @@ int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
>>  {
>>  	if (opp1->rate != opp2->rate)
>>  		return opp1->rate < opp2->rate ? -1 : 1;
>> +	if (opp1->bandwidth && opp2->bandwidth &&
>> +	    opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
>> +		return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1;
>>  	if (opp1->level != opp2->level)
>>  		return opp1->level < opp2->level ? -1 : 1;
>>  	return 0;
>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>> index e33169c7e045..978e445b0cdb 100644
>> --- a/drivers/opp/of.c
>> +++ b/drivers/opp/of.c
>> @@ -332,6 +332,59 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
>>  	return ret;
>>  }
>>  
>> +int _of_find_paths(struct opp_table *opp_table, struct device *dev)
> 
> nit: _of_find_icc_paths() to be more concise?

Ok!

> 
>> +{
>> +	struct device_node *np;
>> +	int ret, i, count, num_paths;
>> +
>> +	np = of_node_get(dev->of_node);
>> +	if (!np)
>> +		return 0;
>> +
>> +	count = of_count_phandle_with_args(np, "interconnects",
>> +					   "#interconnect-cells");
>> +	of_node_put(np);
>> +	if (count < 0)
>> +		return 0;
>> +
>> +	/* two phandles when #interconnect-cells = <1> */
>> +	if (count % 2) {
>> +		dev_err(dev, "%s: Invalid interconnects values\n",
>> +			__func__);
> 
> nit: no need for separate line

Ok!

> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	num_paths = count / 2;
>> +	opp_table->paths = kcalloc(num_paths, sizeof(*opp_table->paths),
>> +				   GFP_KERNEL);
> 
> Add kfree(opp_table->paths) to _opp_table_kref_release() ?

Yes, sure.

>> +	if (!opp_table->paths)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < num_paths; i++) {
>> +		opp_table->paths[i] = of_icc_get_by_index(dev, i);
>> +		if (IS_ERR(opp_table->paths[i])) {
>> +			ret = PTR_ERR(opp_table->paths[i]);
>> +			if (ret != -EPROBE_DEFER) {
>> +				dev_err(dev, "%s: Unable to get path%d: %d\n",
>> +					__func__, i, ret);
>> +			}
> 
> nit: curly braces not needed

Ok!

[..]
>> +		for (i = 0; i < count; i++)
>> +			new_opp->bandwidth[i].peak = kBps_to_icc(peak_bw[i]);
>> +
>> +		found = true;
> 
> 		kfree(peak_bw);
> 
> or re-arrange the kfree()'s below to be in the common code path
> 
>> +	}
>> +
>> +	avg = of_find_property(np, "opp-avg-kBps", NULL);
>> +	if (peak && avg) {
>> +		count = avg->length / sizeof(u32);
>> +		avg_bw = kmalloc_array(count, sizeof(*avg_bw), GFP_KERNEL);
>> +		if (!avg_bw) {
>> +			ret = -ENOMEM;
>> +			goto free_peak_bw;
>> +		}
>> +
>> +		ret = of_property_read_u32_array(np, "opp-avg-kBps", avg_bw,
>> +						 count);
>> +		if (ret) {
>> +			pr_err("%s: Error parsing opp-avg-kBps: %d\n",
>> +			       __func__, ret);
>> +			goto free_avg_bw;
>> +		}
>> +
>> +		for (i = 0; i < count; i++)
>> +			new_opp->bandwidth[i].avg = kBps_to_icc(avg_bw[i]);
> 
> 		kfree(avg_bw);
> 
>> +	}
> 
> nit: the two code blocks for peak and average bandwidth are mostly redundant.
> If it weren't for the assignment of 'new_opp->bandwidth[i].avg' vs
> 'new_opp->bandwidth[i].peak' the above could easily be outsourced into a
> helper function. With some pointer hacks you could still do this, but not
> sure if it's worth the effort.

Yeah, i didn't really like this part. I'll see if i can improve it a bit.

Thanks a lot for reviewing!

BR,
Georgi