Here is a proposal to extend the OPP bindings with bandwidth based on
a few previous discussions [1] and patchsets from me [2][3] and Saravana
[4][5][6][7][8][9].
Changes in v8:
* Addressed review comments from Matthias, Sibi and Viresh.
* Picked reviewed-by tags.
* Picked Sibi's interconnect-tag patches into this patchset.
Changes in v7: https://lore.kernel.org/r/[email protected]
* This version is combination of both patchsets by Saravana and me, based
on [3] and [9].
* The latest version of DT bindings from Saravana is used here, with a
minor change of using arrays instead of single integers for opp-peak-kBps
and opp-avg-kBps. This is needed to support multiple interconnect paths.
* The concept of having multiple OPP tables per device has been dropped,
as it was nacked by Viresh.
* Various reviews comments have been addressed and some patches are
split, and there are also some new patches. Thanks to Viresh, Sibi and
others for providing feedback!
With this version of the patchset, the CPU/GPU to DDR bandwidth scaling
will look like this in DT:
One interconnect path (no change from Saravana's v6 patches):
cpu@0 {
operating-points-v2 = <&cpu_opp_table>;
interconnects = <&noc1 MASTER1 &noc2 SLAVE1>,
};
cpu_opp_table: cpu_opp_table {
compatible = "operating-points-v2";
opp-800000000 {
opp-hz = /bits/ 64 <800000000>;
opp-peak-kBps = <1525000>;
opp-avg-kBps = <457000>;
};
opp-998400000 {
opp-hz = /bits/ 64 <998400000>;
opp-peak-kBps = <7614000>;
opp-avg-kBps = <2284000>;
};
};
Two interconnect paths:
cpu@0 {
operating-points-v2 = <&cpu_opp_table>;
interconnects = <&noc1 MASTER1 &noc2 SLAVE1>,
<&noc3 MASTER2 &noc4 SLAVE2>;
};
cpu_opp_table: cpu_opp_table {
compatible = "operating-points-v2";
opp-800000000 {
opp-hz = /bits/ 64 <800000000>;
opp-peak-kBps = <1525000 2000>;
opp-avg-kBps = <457000 1000>;
};
opp-998400000 {
opp-hz = /bits/ 64 <998400000>;
opp-peak-kBps = <7614000 4000>;
opp-avg-kBps = <2284000 2000>;
};
};
------
Every functional block on a SoC can contribute to the system power
efficiency by expressing its own bandwidth needs (to memory or other SoC
modules). This will allow the system to save power when high throughput
is not required (and also provide maximum throughput when needed).
There are at least three ways for a device to determine its bandwidth
needs:
1. The device can dynamically calculate the needed bandwidth
based on some known variable. For example: UART (baud rate), I2C (fast
mode, high-speed mode, etc), USB (specification version, data transfer
type), SDHC (SD standard, clock rate, bus-width), Video Encoder/Decoder
(video format, resolution, frame-rate)
2. There is a hardware specific value. For example: hardware
specific constant value (e.g. for PRNG) or use-case specific value that
is hard-coded.
3. Predefined SoC/board specific bandwidth values. For example:
CPU or GPU bandwidth is related to the current core frequency and both
bandwidth and frequency are scaled together.
This patchset is trying to address point 3 above by extending the OPP
bindings to support predefined SoC/board bandwidth values and adds
support in cpufreq-dt to scale the interconnect between the CPU and the
DDR together with frequency and voltage.
[1] https://patchwork.kernel.org/patch/10577315/
[2] https://lore.kernel.org/r/[email protected]/
[3] https://lore.kernel.org/r/[email protected]/
[4] https://lore.kernel.org/r/[email protected]
[5] https://lore.kernel.org/r/[email protected]
[6] https://lore.kernel.org/r/[email protected]
[7] https://lore.kernel.org/r/[email protected]
[8] https://lore.kernel.org/r/[email protected]
[9] https://lore.kernel.org/r/[email protected]
Georgi Djakov (6):
interconnect: Add of_icc_get_by_index() helper function
OPP: Add support for parsing interconnect bandwidth
OPP: Add sanity checks in _read_opp_key()
OPP: Update the bandwidth on OPP frequency changes
cpufreq: dt: Add support for interconnect bandwidth scaling
cpufreq: dt: Validate all interconnect paths
Saravana Kannan (2):
dt-bindings: opp: Introduce opp-peak-kBps and opp-avg-kBps bindings
OPP: Add helpers for reading the binding properties
Sibi Sankar (2):
dt-bindings: interconnect: Add interconnect-tags bindings
OPP: Add support for setting interconnect-tags
.../bindings/interconnect/interconnect.txt | 5 +
Documentation/devicetree/bindings/opp/opp.txt | 17 +-
.../devicetree/bindings/property-units.txt | 4 +
drivers/cpufreq/Kconfig | 1 +
drivers/cpufreq/cpufreq-dt.c | 54 +++++
drivers/interconnect/core.c | 72 +++++--
drivers/opp/Kconfig | 1 +
drivers/opp/core.c | 55 ++++-
drivers/opp/of.c | 189 ++++++++++++++++--
drivers/opp/opp.h | 10 +
include/linux/interconnect.h | 6 +
include/linux/pm_opp.h | 12 ++
12 files changed, 380 insertions(+), 46 deletions(-)
On 12-05-20, 15:53, Georgi Djakov wrote:
> Here is a proposal to extend the OPP bindings with bandwidth based on
> a few previous discussions [1] and patchsets from me [2][3] and Saravana
> [4][5][6][7][8][9].
>
> Changes in v8:
> * Addressed review comments from Matthias, Sibi and Viresh.
> * Picked reviewed-by tags.
> * Picked Sibi's interconnect-tag patches into this patchset.
I have applied the series with the modifications I replied with
separately.
Please lemme know if any more tags (reviewed/acked) etc need to be
applied or any more changes are required before I send the pull
request to Rafael.
Please give my branch a try as soon as you can.
Thanks.
--
viresh
Hi Viresh,
On 5/13/20 09:55, Viresh Kumar wrote:
> On 12-05-20, 15:53, Georgi Djakov wrote:
>> Here is a proposal to extend the OPP bindings with bandwidth based on
>> a few previous discussions [1] and patchsets from me [2][3] and Saravana
>> [4][5][6][7][8][9].
>>
>> Changes in v8:
>> * Addressed review comments from Matthias, Sibi and Viresh.
>> * Picked reviewed-by tags.
>> * Picked Sibi's interconnect-tag patches into this patchset.
>
> I have applied the series with the modifications I replied with
> separately.
Thanks a lot!
> Please lemme know if any more tags (reviewed/acked) etc need to be
> applied or any more changes are required before I send the pull
> request to Rafael.
>
> Please give my branch a try as soon as you can.
On top of your branch i tested with scaling 3 different test paths (also
tagged with different tags) and it looks good:
node tag avg peak
--------------------------------------------------------------------
slv_ebi_ch0 458824 1525000
cpu0 3 922 911
cpu0 2 902 901
cpu0 1 457000 1525000
Apart from that, i ran memory throughput tests and they also confirm
that it's working as expected.
There will be a minor conflict with my branch when this is merged upstream,
so maybe we will need to report it or use an immutable tag/branch.
Thanks,
Georgi
On 13-05-20, 13:10, Georgi Djakov wrote:
> There will be a minor conflict with my branch when this is merged upstream,
> so maybe we will need to report it or use an immutable tag/branch.
Okay, give me your branch and I will rebase over it then. Or if you
can do that over my branch, that will be better as mine is just based
of rc2.
--
viresh
Expose the bandwidth information as well via debugfs.
Signed-off-by: Viresh Kumar <[email protected]>
Signed-off-by: Georgi Djakov <[email protected]>
---
@Georgi: I am applying this along with your series.
drivers/interconnect/core.c | 18 ++++++++++++++++
drivers/opp/debugfs.c | 42 ++++++++++++++++++++++++++++++++++++
include/linux/interconnect.h | 6 ++++++
3 files changed, 66 insertions(+)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 2d2e49780511..a56349c14985 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -514,6 +514,24 @@ void icc_set_tag(struct icc_path *path, u32 tag)
}
EXPORT_SYMBOL_GPL(icc_set_tag);
+/**
+ * icc_get_name() - Get name of the icc path
+ * @path: reference to the path returned by icc_get()
+ *
+ * This function is used by an interconnect consumer to get the name of the icc
+ * path.
+ *
+ * Returns a valid pointer on success, or NULL otherwise.
+ */
+const char *icc_get_name(struct icc_path *path)
+{
+ if (!path)
+ return NULL;
+
+ return path->name;
+}
+EXPORT_SYMBOL_GPL(icc_get_name);
+
/**
* icc_set_bw() - set bandwidth constraints on an interconnect path
* @path: reference to the path returned by icc_get()
diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
index 609665e339b6..596c185b5dda 100644
--- a/drivers/opp/debugfs.c
+++ b/drivers/opp/debugfs.c
@@ -32,6 +32,47 @@ void opp_debug_remove_one(struct dev_pm_opp *opp)
debugfs_remove_recursive(opp->dentry);
}
+static ssize_t bw_name_read(struct file *fp, char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ struct icc_path *path = fp->private_data;
+ char buf[64];
+ int i;
+
+ i = scnprintf(buf, sizeof(buf), "%.62s\n", icc_get_name(path));
+
+ return simple_read_from_buffer(userbuf, count, ppos, buf, i);
+}
+
+static const struct file_operations bw_name_fops = {
+ .open = simple_open,
+ .read = bw_name_read,
+ .llseek = default_llseek,
+};
+
+static void opp_debug_create_bw(struct dev_pm_opp *opp,
+ struct opp_table *opp_table,
+ struct dentry *pdentry)
+{
+ struct dentry *d;
+ char name[11];
+ int i;
+
+ for (i = 0; i < opp_table->path_count; i++) {
+ snprintf(name, sizeof(name), "icc-path-%.1d", i);
+
+ /* Create per-path directory */
+ d = debugfs_create_dir(name, pdentry);
+
+ debugfs_create_file("name", S_IRUGO, d, opp_table->paths[i],
+ &bw_name_fops);
+ debugfs_create_u32("peak_bw", S_IRUGO, d,
+ &opp->bandwidth[i].peak);
+ debugfs_create_u32("avg_bw", S_IRUGO, d,
+ &opp->bandwidth[i].avg);
+ }
+}
+
static void opp_debug_create_supplies(struct dev_pm_opp *opp,
struct opp_table *opp_table,
struct dentry *pdentry)
@@ -94,6 +135,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
&opp->clock_latency_ns);
opp_debug_create_supplies(opp, opp_table, d);
+ opp_debug_create_bw(opp, opp_table, d);
opp->dentry = d;
}
diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
index 34e97231a6ab..1ad09efd296e 100644
--- a/include/linux/interconnect.h
+++ b/include/linux/interconnect.h
@@ -32,6 +32,7 @@ struct icc_path *of_icc_get_by_index(struct device *dev, int idx);
void icc_put(struct icc_path *path);
int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
void icc_set_tag(struct icc_path *path, u32 tag);
+const char *icc_get_name(struct icc_path *path);
#else
@@ -65,6 +66,11 @@ static inline void icc_set_tag(struct icc_path *path, u32 tag)
{
}
+static inline const char *icc_get_name(struct icc_path *path)
+{
+ return NULL;
+}
+
#endif /* CONFIG_INTERCONNECT */
#endif /* __LINUX_INTERCONNECT_H */
--
2.25.0.rc1.19.g042ed3e048af
We already drop several votes when target_freq is set to zero, drop
bandwidth votes as well.
Reported-by: Sibi Sankar <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
V2: Some changes left uncommited in my tree by mistake.
drivers/opp/core.c | 49 ++++++++++++++++++++++++++++++++++------------
1 file changed, 37 insertions(+), 12 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 56d3022c1ca2..df12c3804533 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -725,6 +725,34 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
return ret;
}
+static int _set_opp_bw(const struct opp_table *opp_table,
+ struct dev_pm_opp *opp, struct device *dev, bool remove)
+{
+ u32 avg, peak;
+ int i, ret;
+
+ if (!opp_table->paths)
+ return 0;
+
+ for (i = 0; i < opp_table->path_count; i++) {
+ if (remove) {
+ avg = 0;
+ peak = 0;
+ } else {
+ avg = opp->bandwidth[i].avg;
+ peak = opp->bandwidth[i].peak;
+ }
+ ret = icc_set_bw(opp_table->paths[i], avg, peak);
+ if (ret) {
+ dev_err(dev, "Failed to %s bandwidth[%d]: %d\n",
+ remove ? "remove" : "set", i, ret);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
static int _set_opp_custom(const struct opp_table *opp_table,
struct device *dev, unsigned long old_freq,
unsigned long freq,
@@ -820,7 +848,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
unsigned long freq, old_freq, temp_freq;
struct dev_pm_opp *old_opp, *opp;
struct clk *clk;
- int ret, i;
+ int ret;
opp_table = _find_opp_table(dev);
if (IS_ERR(opp_table)) {
@@ -837,12 +865,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
if (!_get_opp_count(opp_table))
return 0;
- if (!opp_table->required_opp_tables && !opp_table->regulators) {
+ if (!opp_table->required_opp_tables && !opp_table->regulators &&
+ !opp_table->paths) {
dev_err(dev, "target frequency can't be 0\n");
ret = -EINVAL;
goto put_opp_table;
}
+ ret = _set_opp_bw(opp_table, opp, dev, true);
+ if (ret)
+ return ret;
+
if (opp_table->regulator_enabled) {
regulator_disable(opp_table->regulators[0]);
opp_table->regulator_enabled = false;
@@ -932,16 +965,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
dev_err(dev, "Failed to set required opps: %d\n", ret);
}
- if (!ret && opp_table->paths) {
- for (i = 0; i < opp_table->path_count; i++) {
- ret = icc_set_bw(opp_table->paths[i],
- opp->bandwidth[i].avg,
- opp->bandwidth[i].peak);
- if (ret)
- dev_err(dev, "Failed to set bandwidth[%d]: %d\n",
- i, ret);
- }
- }
+ if (!ret)
+ ret = _set_opp_bw(opp_table, opp, dev, false);
put_opp:
dev_pm_opp_put(opp);
--
2.25.0.rc1.19.g042ed3e048af
We already drop several votes when target_freq is set to zero, drop
bandwidth votes as well.
Reported-by: Sibi Sankar <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
@Georgi/Sibi: Sibi requested this change, please test this out.
drivers/opp/core.c | 47 +++++++++++++++++++++++++++++++++++-----------
1 file changed, 36 insertions(+), 11 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 56d3022c1ca2..0c259d5ed232 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -725,6 +725,34 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
return ret;
}
+static int _set_opp_bw(const struct opp_table *opp_table,
+ struct dev_pm_opp *opp, bool remove)
+{
+ u32 avg, peak;
+ int i, ret;
+
+ if (!opp_table->paths)
+ return 0;
+
+ for (i = 0; i < opp_table->path_count; i++) {
+ if (remove) {
+ avg = 0;
+ peak = 0;
+ } else {
+ avg = opp->bandwidth[i].avg;
+ peak = opp->bandwidth[i].peak;
+ }
+ ret = icc_set_bw(opp_table->paths[i], avg, peak);
+ if (ret) {
+ dev_err(dev, "Failed to %s bandwidth[%d]: %d\n",
+ remove ? "remove" : "set", i, ret);
+ retrun ret;
+ }
+ }
+
+ return 0;
+}
+
static int _set_opp_custom(const struct opp_table *opp_table,
struct device *dev, unsigned long old_freq,
unsigned long freq,
@@ -837,12 +865,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
if (!_get_opp_count(opp_table))
return 0;
- if (!opp_table->required_opp_tables && !opp_table->regulators) {
+ if (!opp_table->required_opp_tables && !opp_table->regulators &&
+ !opp_table->paths) {
dev_err(dev, "target frequency can't be 0\n");
ret = -EINVAL;
goto put_opp_table;
}
+ ret = _set_opp_bw(opp_table, opp, true);
+ if (ret)
+ return ret;
+
if (opp_table->regulator_enabled) {
regulator_disable(opp_table->regulators[0]);
opp_table->regulator_enabled = false;
@@ -932,16 +965,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
dev_err(dev, "Failed to set required opps: %d\n", ret);
}
- if (!ret && opp_table->paths) {
- for (i = 0; i < opp_table->path_count; i++) {
- ret = icc_set_bw(opp_table->paths[i],
- opp->bandwidth[i].avg,
- opp->bandwidth[i].peak);
- if (ret)
- dev_err(dev, "Failed to set bandwidth[%d]: %d\n",
- i, ret);
- }
- }
+ if (!ret)
+ ret = _set_opp_bw(opp_table, opp, false);
put_opp:
dev_pm_opp_put(opp);
--
2.25.0.rc1.19.g042ed3e048af
On 5/27/20 07:13, Viresh Kumar wrote:
> We already drop several votes when target_freq is set to zero, drop
> bandwidth votes as well.
>
> Reported-by: Sibi Sankar <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
Reviewed-by: Georgi Djakov <[email protected]>
Tested-by: Georgi Djakov <[email protected]>
Thanks!
Georgi
On 2020-05-27 09:43, Viresh Kumar wrote:
> We already drop several votes when target_freq is set to zero, drop
> bandwidth votes as well.
>
> Reported-by: Sibi Sankar <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> V2: Some changes left uncommited in my tree by mistake.
Viresh,
Thanks for the patch :)
Tested-by: Sibi Sankar <[email protected]>
Reviewed-by: Sibi Sankar <[email protected]>
>
> drivers/opp/core.c | 49 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 56d3022c1ca2..df12c3804533 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -725,6 +725,34 @@ static int _generic_set_opp_regulator(struct
> opp_table *opp_table,
> return ret;
> }
>
> +static int _set_opp_bw(const struct opp_table *opp_table,
> + struct dev_pm_opp *opp, struct device *dev, bool remove)
> +{
> + u32 avg, peak;
> + int i, ret;
> +
> + if (!opp_table->paths)
> + return 0;
> +
> + for (i = 0; i < opp_table->path_count; i++) {
> + if (remove) {
> + avg = 0;
> + peak = 0;
> + } else {
> + avg = opp->bandwidth[i].avg;
> + peak = opp->bandwidth[i].peak;
> + }
> + ret = icc_set_bw(opp_table->paths[i], avg, peak);
> + if (ret) {
> + dev_err(dev, "Failed to %s bandwidth[%d]: %d\n",
> + remove ? "remove" : "set", i, ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int _set_opp_custom(const struct opp_table *opp_table,
> struct device *dev, unsigned long old_freq,
> unsigned long freq,
> @@ -820,7 +848,7 @@ int dev_pm_opp_set_rate(struct device *dev,
> unsigned long target_freq)
> unsigned long freq, old_freq, temp_freq;
> struct dev_pm_opp *old_opp, *opp;
> struct clk *clk;
> - int ret, i;
> + int ret;
>
> opp_table = _find_opp_table(dev);
> if (IS_ERR(opp_table)) {
> @@ -837,12 +865,17 @@ int dev_pm_opp_set_rate(struct device *dev,
> unsigned long target_freq)
> if (!_get_opp_count(opp_table))
> return 0;
>
> - if (!opp_table->required_opp_tables && !opp_table->regulators) {
> + if (!opp_table->required_opp_tables && !opp_table->regulators &&
> + !opp_table->paths) {
> dev_err(dev, "target frequency can't be 0\n");
> ret = -EINVAL;
> goto put_opp_table;
> }
>
> + ret = _set_opp_bw(opp_table, opp, dev, true);
> + if (ret)
> + return ret;
> +
> if (opp_table->regulator_enabled) {
> regulator_disable(opp_table->regulators[0]);
> opp_table->regulator_enabled = false;
> @@ -932,16 +965,8 @@ int dev_pm_opp_set_rate(struct device *dev,
> unsigned long target_freq)
> dev_err(dev, "Failed to set required opps: %d\n", ret);
> }
>
> - if (!ret && opp_table->paths) {
> - for (i = 0; i < opp_table->path_count; i++) {
> - ret = icc_set_bw(opp_table->paths[i],
> - opp->bandwidth[i].avg,
> - opp->bandwidth[i].peak);
> - if (ret)
> - dev_err(dev, "Failed to set bandwidth[%d]: %d\n",
> - i, ret);
> - }
> - }
> + if (!ret)
> + ret = _set_opp_bw(opp_table, opp, dev, false);
>
> put_opp:
> dev_pm_opp_put(opp);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.