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 <[email protected]>
>> ---
>> v7:
>> * Addressed some review comments from Viresh and Sibi.
>> * Various other changes.
>>
>> v2:
>> https://lore.kernel.org/linux-arm-msm/[email protected]/
>>
>> 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
