lkml.org 
[lkml]   [2020]   [May]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v7 4/7] OPP: Add support for parsing interconnect bandwidth
    Hey Georgi,

    Apart from the Matthias's comments
    ran into a few issues during testing.

    On 2020-04-24 21:24, 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@linaro.org>
    > ---
    > 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@linaro.org/
    >
    > 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
    > ---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);
    > +
    > 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);
    > 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)
    > +{
    > + 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__);
    > + return -EINVAL;
    > + }
    > +
    > + num_paths = count / 2;
    > + opp_table->paths = kcalloc(num_paths, sizeof(*opp_table->paths),
    > + GFP_KERNEL);
    > + if (!opp_table->paths)
    > + return -ENOMEM;
    > +
    > + for (i = 0; i < num_paths; i++) {
    > + opp_table->paths[i] = of_icc_get_by_index(dev, i);

    now that icc_get is part of the
    opp core framework shouldn't we
    do a put on table remove as well?

    > + 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);
    > + }
    > + goto err;
    > + }
    > + }
    > + opp_table->path_count = num_paths;
    > +
    > + return 0;
    > +
    > +err:
    > + while (i--)
    > + icc_put(opp_table->paths[i]);
    > +
    > + kfree(opp_table->paths);
    > + opp_table->paths = NULL;
    > +
    > + return ret;
    > +}
    > +
    > static bool _opp_is_supported(struct device *dev, struct opp_table
    > *opp_table,
    > struct device_node *np)
    > {
    > @@ -524,8 +577,11 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
    > static int _read_opp_key(struct dev_pm_opp *new_opp, struct
    > device_node *np,
    > bool *rate_not_available)
    > {
    > + struct property *peak, *avg;
    > + u32 *peak_bw, *avg_bw;
    > u64 rate;
    > - int ret;
    > + int ret, i, count;
    > + bool found = false;
    >
    > ret = of_property_read_u64(np, "opp-hz", &rate);
    > if (!ret) {
    > @@ -535,10 +591,69 @@ static int _read_opp_key(struct dev_pm_opp
    > *new_opp, struct device_node *np,
    > * bit guaranteed in clk API.
    > */
    > new_opp->rate = (unsigned long)rate;
    > + found = true;
    > }
    > *rate_not_available = !!ret;
    >
    > - of_property_read_u32(np, "opp-level", &new_opp->level);
    > + peak = of_find_property(np, "opp-peak-kBps", NULL);
    > + if (peak) {
    > + /*
    > + * Bandwidth consists of peak and average (optional) values:
    > + * opp-peak-kBps = <path1_value path2_value>;
    > + * opp-avg-kBps = <path1_value path2_value>;
    > + */
    > + count = peak->length / sizeof(u32);
    > + peak_bw = kmalloc_array(count, sizeof(*peak_bw), GFP_KERNEL);
    > + if (!peak_bw)
    > + return -ENOMEM;
    > +
    > + ret = of_property_read_u32_array(np, "opp-peak-kBps", peak_bw,
    > + count);
    > + if (ret) {
    > + pr_err("%s: Error parsing opp-peak-kBps: %d\n",
    > + __func__, ret);
    > + goto free_peak_bw;
    > + }
    > +
    > + for (i = 0; i < count; i++)
    > + new_opp->bandwidth[i].peak = kBps_to_icc(peak_bw[i]);
    > +
    > + found = true;
    > + }
    > +
    > + 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]);
    > + }
    > +
    > + if (of_property_read_u32(np, "opp-level", &new_opp->level))

    of_property_read_u32 returns 0 on
    success so should use logical not
    instead ^^

    > + found = true;
    > +
    > + if (found)
    > + return 0;
    > +
    > + return ret;
    > +
    > +free_avg_bw:
    > + kfree(avg_bw);
    > +free_peak_bw:
    > + kfree(peak_bw);
    >
    > return ret;
    > }
    > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
    > index bcadb1e328a4..2e0113360297 100644
    > --- a/drivers/opp/opp.h
    > +++ b/drivers/opp/opp.h
    > @@ -12,6 +12,7 @@
    > #define __DRIVER_OPP_H__
    >
    > #include <linux/device.h>
    > +#include <linux/interconnect.h>
    > #include <linux/kernel.h>
    > #include <linux/kref.h>
    > #include <linux/list.h>
    > @@ -59,6 +60,7 @@ extern struct list_head opp_tables;
    > * @rate: Frequency in hertz
    > * @level: Performance level
    > * @supplies: Power supplies voltage/current values
    > + * @bandwidth: Interconnect bandwidth values
    > * @clock_latency_ns: Latency (in nanoseconds) of switching to this
    > OPP's
    > * frequency from any other OPP's frequency.
    > * @required_opps: List of OPPs that are required by this OPP.
    > @@ -81,6 +83,7 @@ struct dev_pm_opp {
    > unsigned int level;
    >
    > struct dev_pm_opp_supply *supplies;
    > + struct dev_pm_opp_icc_bw *bandwidth;
    >
    > unsigned long clock_latency_ns;
    >
    > @@ -146,6 +149,8 @@ enum opp_table_access {
    > * @regulator_count: Number of power supply regulators. Its value can
    > be -1
    > * (uninitialized), 0 (no opp-microvolt property) or > 0 (has
    > opp-microvolt
    > * property).
    > + * @paths: Interconnect path handles
    > + * @path_count: Number of interconnect paths
    > * @genpd_performance_state: Device's power domain support performance
    > state.
    > * @is_genpd: Marks if the OPP table belongs to a genpd.
    > * @set_opp: Platform specific set_opp callback
    > @@ -189,6 +194,8 @@ struct opp_table {
    > struct clk *clk;
    > struct regulator **regulators;
    > int regulator_count;
    > + struct icc_path **paths;
    > + unsigned int path_count;
    > bool genpd_performance_state;
    > bool is_genpd;
    >
    > @@ -224,12 +231,14 @@ void _of_clear_opp_table(struct opp_table
    > *opp_table);
    > struct opp_table *_managed_opp(struct device *dev, int index);
    > void _of_opp_free_required_opps(struct opp_table *opp_table,
    > struct dev_pm_opp *opp);
    > +int _of_find_paths(struct opp_table *opp_table, struct device *dev);
    > #else
    > static inline void _of_init_opp_table(struct opp_table *opp_table,
    > struct device *dev, int index) {}
    > static inline void _of_clear_opp_table(struct opp_table *opp_table) {}
    > static inline struct opp_table *_managed_opp(struct device *dev, int
    > index) { return NULL; }
    > static inline void _of_opp_free_required_opps(struct opp_table
    > *opp_table,
    > struct dev_pm_opp *opp) {}
    > +static inline int _of_find_paths(struct opp_table *opp_table, struct
    > device *dev) { return 0; }
    > #endif
    >
    > #ifdef CONFIG_DEBUG_FS
    > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
    > index 747861816f4f..cfceb0290401 100644
    > --- a/include/linux/pm_opp.h
    > +++ b/include/linux/pm_opp.h
    > @@ -41,6 +41,18 @@ struct dev_pm_opp_supply {
    > unsigned long u_amp;
    > };
    >
    > +/**
    > + * struct dev_pm_opp_icc_bw - Interconnect bandwidth values
    > + * @avg: Average bandwidth corresponding to this OPP (in icc units)
    > + * @peak: Peak bandwidth corresponding to this OPP (in icc units)
    > + *
    > + * This structure stores the bandwidth values for a single
    > interconnect path.
    > + */
    > +struct dev_pm_opp_icc_bw {
    > + u32 avg;
    > + u32 peak;
    > +};
    > +
    > /**
    > * struct dev_pm_opp_info - OPP freq/voltage/current values
    > * @rate: Target clk rate in hz

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

    \
     
     \ /
      Last update: 2020-05-04 23:04    [W:3.656 / U:0.916 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site