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
---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);
+ 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))
+ 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
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?
> ---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?
> +
> 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/
> 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?
> +{
> + 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
> + 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() ?
> + 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
> + 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;
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.
> +
> + if (of_property_read_u32(np, "opp-level", &new_opp->level))
> + found = true;
> +
> + if (found)
> + return 0;
> +
> + return ret;
> +
> +free_avg_bw:
> + kfree(avg_bw);
> +free_peak_bw:
> + kfree(peak_bw);
>
> return ret;
> }
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
On 24-04-20, 12:20, Matthias Kaehlcke wrote:
> On Fri, Apr 24, 2020 at 06:54:01PM +0300, Georgi Djakov wrote:
> > + 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
Again, braces are preferred across multi-line blocks. Please keep it.
--
viresh
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 <[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
> ---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.