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
