[PATCH v8 00/10] Introduce OPP bandwidth bindings

[PATCH v8 00/10] Introduce OPP bandwidth bindings

[Georgi Djakov]

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/20200424155404.10746-1-georgi.djakov@linaro.org
* 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/20190313090010.20534-1-georgi.djakov@linaro.org/
[3] https://lore.kernel.org/r/20190423132823.7915-1-georgi.djakov@linaro.org/
[4] https://lore.kernel.org/r/20190608044339.115026-1-saravanak@google.com
[5] https://lore.kernel.org/r/20190614041733.120807-1-saravanak@google.com
[6] https://lore.kernel.org/r/20190703011020.151615-1-saravanak@google.com
[7] https://lore.kernel.org/r/20190726231558.175130-1-saravanak@google.com
[8] https://lore.kernel.org/r/20190807223111.230846-1-saravanak@google.com
[9] https://lore.kernel.org/r/20191207002424.201796-1-saravanak@google.com

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(-)

[PATCH v8 01/10] dt-bindings: opp: Introduce opp-peak-kBps and opp-avg-kBps bindings

[Georgi Djakov]

From: Saravana Kannan <saravanak@google.com>

Interconnects often quantify their performance points in terms of
bandwidth. So, add opp-peak-kBps (required) and opp-avg-kBps (optional) to
allow specifying Bandwidth OPP tables in DT.

opp-peak-kBps is a required property that replaces opp-hz for Bandwidth OPP
tables.

opp-avg-kBps is an optional property that can be used in Bandwidth OPP
tables.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Reviewed-by: Sibi Sankar <sibis@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
v8:
* Picked reviewed-by tags.
* Changes on wording.

 Documentation/devicetree/bindings/opp/opp.txt   | 17 ++++++++++++++---
 .../devicetree/bindings/property-units.txt      |  4 ++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index 68592271461f..9d16d417e9be 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -83,9 +83,14 @@ properties.
 
 Required properties:
 - opp-hz: Frequency in Hz, expressed as a 64-bit big-endian integer. This is a
-  required property for all device nodes but devices like power domains. The
-  power domain nodes must have another (implementation dependent) property which
-  uniquely identifies the OPP nodes.
+  required property for all device nodes, unless another "required" property to
+  uniquely identify the OPP nodes exists. Devices like power domains must have
+  another (implementation dependent) property.
+
+- opp-peak-kBps: Peak bandwidth in kilobytes per second, expressed as an array
+  of 32-bit big-endian integers. Each element of the array represents the
+  peak bandwidth value of each interconnect path. The number of elements should
+  match the number of interconnect paths.
 
 Optional properties:
 - opp-microvolt: voltage in micro Volts.
@@ -132,6 +137,12 @@ Optional properties:
 - opp-level: A value representing the performance level of the device,
   expressed as a 32-bit integer.
 
+- opp-avg-kBps: Average bandwidth in kilobytes per second, expressed as an array
+  of 32-bit big-endian integers. Each element of the array represents the
+  average bandwidth value of each interconnect path. The number of elements
+  should match the number of interconnect paths. This property is only
+  meaningful in OPP tables where opp-peak-kBps is present.
+
 - clock-latency-ns: Specifies the maximum possible transition latency (in
   nanoseconds) for switching to this OPP from any other OPP.
 
diff --git a/Documentation/devicetree/bindings/property-units.txt b/Documentation/devicetree/bindings/property-units.txt
index e9b8360b3288..c80a110c1e26 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -41,3 +41,7 @@ Temperature
 Pressure
 ----------------------------------------
 -kpascal	: kilopascal
+
+Throughput
+----------------------------------------
+-kBps		: kilobytes per second

[PATCH v8 02/10] OPP: Add helpers for reading the binding properties

[Georgi Djakov]

From: Saravana Kannan <saravanak@google.com>

The opp-hz DT property is not mandatory and we may use another property
as a key in the OPP table. Add helper functions to simplify the reading
and comparing the keys.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Sibi Sankar <sibis@codeaurora.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
v8:
* Keep the existing behavior: goto free_opp only if !is_genpd
* Picked reviewed-by tags.

 drivers/opp/core.c | 15 +++++++++++++--
 drivers/opp/of.c   | 45 +++++++++++++++++++++++++++------------------
 drivers/opp/opp.h  |  1 +
 3 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e4f01e7771a2..ce7e4103ec09 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1286,11 +1286,21 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 	return true;
 }
 
+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->level != opp2->level)
+		return opp1->level < opp2->level ? -1 : 1;
+	return 0;
+}
+
 static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,
 			     struct opp_table *opp_table,
 			     struct list_head **head)
 {
 	struct dev_pm_opp *opp;
+	int opp_cmp;
 
 	/*
 	 * Insert new OPP in order of increasing frequency and discard if
@@ -1301,12 +1311,13 @@ static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,
 	 * loop.
 	 */
 	list_for_each_entry(opp, &opp_table->opp_list, node) {
-		if (new_opp->rate > opp->rate) {
+		opp_cmp = _opp_compare_key(new_opp, opp);
+		if (opp_cmp > 0) {
 			*head = &opp->node;
 			continue;
 		}
 
-		if (new_opp->rate < opp->rate)
+		if (opp_cmp < 0)
 			return 0;
 
 		/* Duplicate OPPs */
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 9cd8f0adacae..5179f034751a 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -521,6 +521,28 @@ void dev_pm_opp_of_remove_table(struct device *dev)
 }
 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)
+{
+	u64 rate;
+	int ret;
+
+	ret = of_property_read_u64(np, "opp-hz", &rate);
+	if (!ret) {
+		/*
+		 * Rate is defined as an unsigned long in clk API, and so
+		 * casting explicitly to its type. Must be fixed once rate is 64
+		 * bit guaranteed in clk API.
+		 */
+		new_opp->rate = (unsigned long)rate;
+	}
+	*rate_not_available = !!ret;
+
+	of_property_read_u32(np, "opp-level", &new_opp->level);
+
+	return ret;
+}
+
 /**
  * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
  * @opp_table:	OPP table
@@ -558,26 +580,13 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	if (!new_opp)
 		return ERR_PTR(-ENOMEM);
 
-	ret = of_property_read_u64(np, "opp-hz", &rate);
-	if (ret < 0) {
-		/* "opp-hz" is optional for devices like power domains. */
-		if (!opp_table->is_genpd) {
-			dev_err(dev, "%s: opp-hz not found\n", __func__);
-			goto free_opp;
-		}
-
-		rate_not_available = true;
-	} else {
-		/*
-		 * Rate is defined as an unsigned long in clk API, and so
-		 * casting explicitly to its type. Must be fixed once rate is 64
-		 * bit guaranteed in clk API.
-		 */
-		new_opp->rate = (unsigned long)rate;
+	ret = _read_opp_key(new_opp, np, &rate_not_available);
+	/* The key is optional for devices like power domains. */
+	if (ret < 0 && !opp_table->is_genpd) {
+		dev_err(dev, "%s: opp key field not found\n", __func__);
+		goto free_opp;
 	}
 
-	of_property_read_u32(np, "opp-level", &new_opp->level);
-
 	/* Check if the OPP supports hardware's hierarchy of versions or not */
 	if (!_opp_is_supported(dev, opp_table, np)) {
 		dev_dbg(dev, "OPP not supported by hardware: %llu\n", rate);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index d14e27102730..bcadb1e328a4 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -211,6 +211,7 @@ struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_
 void _dev_pm_opp_find_and_remove_table(struct device *dev);
 struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table);
 void _opp_free(struct dev_pm_opp *opp);
+int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
 int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, bool rate_not_available);
 int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic);
 void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cpu);

Re: [PATCH v8 02/10] OPP: Add helpers for reading the binding properties

[Viresh Kumar]

On 12-05-20, 15:53, Georgi Djakov wrote:
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c

> @@ -558,26 +580,13 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
>  	if (!new_opp)
>  		return ERR_PTR(-ENOMEM);
>  
> -	ret = of_property_read_u64(np, "opp-hz", &rate);
> -	if (ret < 0) {
> -		/* "opp-hz" is optional for devices like power domains. */
> -		if (!opp_table->is_genpd) {
> -			dev_err(dev, "%s: opp-hz not found\n", __func__);
> -			goto free_opp;
> -		}
> -
> -		rate_not_available = true;
> -	} else {
> -		/*
> -		 * Rate is defined as an unsigned long in clk API, and so
> -		 * casting explicitly to its type. Must be fixed once rate is 64
> -		 * bit guaranteed in clk API.
> -		 */
> -		new_opp->rate = (unsigned long)rate;
> +	ret = _read_opp_key(new_opp, np, &rate_not_available);
> +	/* The key is optional for devices like power domains. */

Dropped this comment as key isn't optional for genpd as well as you
handled that in a later patch..

> +	if (ret < 0 && !opp_table->is_genpd) {
> +		dev_err(dev, "%s: opp key field not found\n", __func__);
> +		goto free_opp;
>  	}

-- 
viresh

[PATCH v8 03/10] interconnect: Add of_icc_get_by_index() helper function

[Georgi Djakov]

This is the same as the traditional of_icc_get() function, but the
difference is that it takes index as an argument, instead of name.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Sibi Sankar <sibis@codeaurora.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
v8:
* Dropped unneeded initializations.
* Picked reviewed-by tags.

 drivers/interconnect/core.c  | 72 +++++++++++++++++++++++++++---------
 include/linux/interconnect.h |  6 +++
 2 files changed, 60 insertions(+), 18 deletions(-)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 2c6515e3ecf1..aba5d38ea9d1 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -351,9 +351,9 @@ static struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
 }
 
 /**
- * of_icc_get() - get a path handle from a DT node based on name
+ * of_icc_get_by_index() - get a path handle from a DT node based on index
  * @dev: device pointer for the consumer device
- * @name: interconnect path name
+ * @idx: interconnect path index
  *
  * This function will search for a path between two endpoints and return an
  * icc_path handle on success. Use icc_put() to release constraints when they
@@ -365,13 +365,12 @@ static struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
  * Return: icc_path pointer on success or ERR_PTR() on error. NULL is returned
  * when the API is disabled or the "interconnects" DT property is missing.
  */
-struct icc_path *of_icc_get(struct device *dev, const char *name)
+struct icc_path *of_icc_get_by_index(struct device *dev, int idx)
 {
-	struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
+	struct icc_path *path;
 	struct icc_node *src_node, *dst_node;
-	struct device_node *np = NULL;
+	struct device_node *np;
 	struct of_phandle_args src_args, dst_args;
-	int idx = 0;
 	int ret;
 
 	if (!dev || !dev->of_node)
@@ -391,12 +390,6 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
 	 * lets support only global ids and extend this in the future if needed
 	 * without breaking DT compatibility.
 	 */
-	if (name) {
-		idx = of_property_match_string(np, "interconnect-names", name);
-		if (idx < 0)
-			return ERR_PTR(idx);
-	}
-
 	ret = of_parse_phandle_with_args(np, "interconnects",
 					 "#interconnect-cells", idx * 2,
 					 &src_args);
@@ -439,12 +432,8 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
 		return path;
 	}
 
-	if (name)
-		path->name = kstrdup_const(name, GFP_KERNEL);
-	else
-		path->name = kasprintf(GFP_KERNEL, "%s-%s",
-				       src_node->name, dst_node->name);
-
+	path->name = kasprintf(GFP_KERNEL, "%s-%s",
+			       src_node->name, dst_node->name);
 	if (!path->name) {
 		kfree(path);
 		return ERR_PTR(-ENOMEM);
@@ -452,6 +441,53 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
 
 	return path;
 }
+EXPORT_SYMBOL_GPL(of_icc_get_by_index);
+
+/**
+ * of_icc_get() - get a path handle from a DT node based on name
+ * @dev: device pointer for the consumer device
+ * @name: interconnect path name
+ *
+ * This function will search for a path between two endpoints and return an
+ * icc_path handle on success. Use icc_put() to release constraints when they
+ * are not needed anymore.
+ * If the interconnect API is disabled, NULL is returned and the consumer
+ * drivers will still build. Drivers are free to handle this specifically,
+ * but they don't have to.
+ *
+ * Return: icc_path pointer on success or ERR_PTR() on error. NULL is returned
+ * when the API is disabled or the "interconnects" DT property is missing.
+ */
+struct icc_path *of_icc_get(struct device *dev, const char *name)
+{
+	struct device_node *np;
+	int idx = 0;
+
+	if (!dev || !dev->of_node)
+		return ERR_PTR(-ENODEV);
+
+	np = dev->of_node;
+
+	/*
+	 * When the consumer DT node do not have "interconnects" property
+	 * return a NULL path to skip setting constraints.
+	 */
+	if (!of_find_property(np, "interconnects", NULL))
+		return NULL;
+
+	/*
+	 * We use a combination of phandle and specifier for endpoint. For now
+	 * lets support only global ids and extend this in the future if needed
+	 * without breaking DT compatibility.
+	 */
+	if (name) {
+		idx = of_property_match_string(np, "interconnect-names", name);
+		if (idx < 0)
+			return ERR_PTR(idx);
+	}
+
+	return of_icc_get_by_index(dev, idx);
+}
 EXPORT_SYMBOL_GPL(of_icc_get);
 
 /**
diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
index d70a914cba11..34e97231a6ab 100644
--- a/include/linux/interconnect.h
+++ b/include/linux/interconnect.h
@@ -28,6 +28,7 @@ struct device;
 struct icc_path *icc_get(struct device *dev, const int src_id,
 			 const int dst_id);
 struct icc_path *of_icc_get(struct device *dev, const char *name);
+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);
@@ -46,6 +47,11 @@ static inline struct icc_path *of_icc_get(struct device *dev,
 	return NULL;
 }
 
+static inline struct icc_path *of_icc_get_by_index(struct device *dev, int idx)
+{
+	return NULL;
+}
+
 static inline void icc_put(struct icc_path *path)
 {
 }

[PATCH v8 04/10] OPP: Add support for parsing interconnect bandwidth

[Georgi Djakov]

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>
---
v8:
* Drop bandwidth requests and free memory in _opp_table_kref_release.
* Take into account the supply_count in struct size calculations.
* Free the temporary arrays for peak and average bandwidth.
* Fix the check for opp-level.
* Use dev_warn insted of dev_dbg.
* Rename _of_find_icc_paths to _of_find_icc_paths.
* Rename the variable count to supply_count.

 drivers/opp/Kconfig    |   1 +
 drivers/opp/core.c     |  27 +++++++--
 drivers/opp/of.c       | 121 ++++++++++++++++++++++++++++++++++++++++-
 drivers/opp/opp.h      |   9 +++
 include/linux/pm_opp.h |  12 ++++
 5 files changed, 164 insertions(+), 6 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 ce7e4103ec09..a3dd0bc9b9f6 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -999,6 +999,12 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 				ret);
 	}
 
+	/* Find interconnect path(s) for the device */
+	ret = _of_find_icc_paths(opp_table, dev);
+	if (ret)
+		dev_warn(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);
@@ -1057,6 +1063,7 @@ static void _opp_table_kref_release(struct kref *kref)
 {
 	struct opp_table *opp_table = container_of(kref, struct opp_table, kref);
 	struct opp_device *opp_dev, *temp;
+	int i;
 
 	_of_clear_opp_table(opp_table);
 
@@ -1064,6 +1071,12 @@ static void _opp_table_kref_release(struct kref *kref)
 	if (!IS_ERR(opp_table->clk))
 		clk_put(opp_table->clk);
 
+	if (opp_table->paths) {
+		for (i = 0; i < opp_table->path_count; i++)
+			icc_put(opp_table->paths[i]);
+		kfree(opp_table->paths);
+	}
+
 	WARN_ON(!list_empty(&opp_table->opp_list));
 
 	list_for_each_entry_safe(opp_dev, temp, &opp_table->dev_list, node) {
@@ -1243,19 +1256,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 supply_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;
+	supply_count = table->regulator_count > 0 ? table->regulator_count : 1;
+	supply_size = sizeof(*opp->supplies) * supply_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 + supply_count);
 	INIT_LIST_HEAD(&opp->node);
 
 	return opp;
@@ -1290,6 +1306,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 5179f034751a..d139ad8c8f4f 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -332,6 +332,58 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 	return ret;
 }
 
+int _of_find_icc_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 +576,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 +590,72 @@ 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);
+	}
+
+	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);
+	}
+
+	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..17d45119d9bc 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_icc_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_icc_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

Re: [PATCH v8 04/10] OPP: Add support for parsing interconnect bandwidth

[Matthias Kaehlcke]

On Tue, May 12, 2020 at 03:53:21PM +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@linaro.org>
> ---
> v8:
> * Drop bandwidth requests and free memory in _opp_table_kref_release.
> * Take into account the supply_count in struct size calculations.
> * Free the temporary arrays for peak and average bandwidth.
> * Fix the check for opp-level.
> * Use dev_warn insted of dev_dbg.
> * Rename _of_find_icc_paths to _of_find_icc_paths.
> * Rename the variable count to supply_count.
> 
>  drivers/opp/Kconfig    |   1 +
>  drivers/opp/core.c     |  27 +++++++--
>  drivers/opp/of.c       | 121 ++++++++++++++++++++++++++++++++++++++++-
>  drivers/opp/opp.h      |   9 +++
>  include/linux/pm_opp.h |  12 ++++
>  5 files changed, 164 insertions(+), 6 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 ce7e4103ec09..a3dd0bc9b9f6 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -999,6 +999,12 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>  				ret);
>  	}
>  
> +	/* Find interconnect path(s) for the device */
> +	ret = _of_find_icc_paths(opp_table, dev);
> +	if (ret)
> +		dev_warn(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);
> @@ -1057,6 +1063,7 @@ static void _opp_table_kref_release(struct kref *kref)
>  {
>  	struct opp_table *opp_table = container_of(kref, struct opp_table, kref);
>  	struct opp_device *opp_dev, *temp;
> +	int i;
>  
>  	_of_clear_opp_table(opp_table);
>  
> @@ -1064,6 +1071,12 @@ static void _opp_table_kref_release(struct kref *kref)
>  	if (!IS_ERR(opp_table->clk))
>  		clk_put(opp_table->clk);
>  
> +	if (opp_table->paths) {
> +		for (i = 0; i < opp_table->path_count; i++)
> +			icc_put(opp_table->paths[i]);
> +		kfree(opp_table->paths);
> +	}
> +
>  	WARN_ON(!list_empty(&opp_table->opp_list));
>  
>  	list_for_each_entry_safe(opp_dev, temp, &opp_table->dev_list, node) {
> @@ -1243,19 +1256,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 supply_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;
> +	supply_count = table->regulator_count > 0 ? table->regulator_count : 1;
> +	supply_size = sizeof(*opp->supplies) * supply_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 + supply_count);
>  	INIT_LIST_HEAD(&opp->node);
>  
>  	return opp;
> @@ -1290,6 +1306,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 5179f034751a..d139ad8c8f4f 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -332,6 +332,58 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
>  	return ret;
>  }
>  
> +int _of_find_icc_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 +576,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 +590,72 @@ 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);
> +	}
> +
> +	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);
> +	}
> +
> +	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..17d45119d9bc 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_icc_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_icc_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

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH v8 04/10] OPP: Add support for parsing interconnect bandwidth

[Viresh Kumar]

On 12-05-20, 15:53, 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>
> ---
> v8:
> * Drop bandwidth requests and free memory in _opp_table_kref_release.
> * Take into account the supply_count in struct size calculations.
> * Free the temporary arrays for peak and average bandwidth.
> * Fix the check for opp-level.
> * Use dev_warn insted of dev_dbg.
> * Rename _of_find_icc_paths to _of_find_icc_paths.
> * Rename the variable count to supply_count.

Added this delta to this patch:

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index a3dd0bc9b9f6..8b640eac86d5 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1000,7 +1000,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 	}
 
 	/* Find interconnect path(s) for the device */
-	ret = _of_find_icc_paths(opp_table, dev);
+	ret = dev_pm_opp_of_find_icc_paths(dev, opp_table);
 	if (ret)
 		dev_warn(dev, "%s: Error finding interconnect paths: %d\n",
 			 __func__, ret);
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index a41c0dc2ac04..15f30ed70bbc 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -332,10 +332,12 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 	return ret;
 }
 
-int _of_find_icc_paths(struct opp_table *opp_table, struct device *dev)
+int dev_pm_opp_of_find_icc_paths(struct device *dev,
+				 struct opp_table *opp_table)
 {
 	struct device_node *np;
 	int ret, i, count, num_paths;
+	struct icc_path **paths;
 
 	np = of_node_get(dev->of_node);
 	if (!np)
@@ -354,15 +356,14 @@ int _of_find_icc_paths(struct opp_table *opp_table, struct device *dev)
 	}
 
 	num_paths = count / 2;
-	opp_table->paths = kcalloc(num_paths, sizeof(*opp_table->paths),
-				   GFP_KERNEL);
-	if (!opp_table->paths)
+	paths = kcalloc(num_paths, sizeof(*paths), GFP_KERNEL);
+	if (!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]);
+		paths[i] = of_icc_get_by_index(dev, i);
+		if (IS_ERR(paths[i])) {
+			ret = PTR_ERR(paths[i]);
 			if (ret != -EPROBE_DEFER) {
 				dev_err(dev, "%s: Unable to get path%d: %d\n",
 					__func__, i, ret);
@@ -370,19 +371,23 @@ int _of_find_icc_paths(struct opp_table *opp_table, struct device *dev)
 			goto err;
 		}
 	}
-	opp_table->path_count = num_paths;
+
+	if (opp_table) {
+		opp_table->paths = paths;
+		opp_table->path_count = num_paths;
+	}
 
 	return 0;
 
 err:
 	while (i--)
-		icc_put(opp_table->paths[i]);
+		icc_put(paths[i]);
 
-	kfree(opp_table->paths);
-	opp_table->paths = NULL;
+	kfree(paths);
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(dev_pm_opp_of_find_icc_paths);
 
 static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
 			      struct device_node *np)
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 17d45119d9bc..2b81ffef1ba4 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -231,14 +231,12 @@ 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_icc_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_icc_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 cfceb0290401..d5c4a329321d 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -372,6 +372,7 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpuma
 struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev);
 struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp);
 int of_get_required_opp_performance_state(struct device_node *np, int index);
+int dev_pm_opp_of_find_icc_paths(struct device *dev, struct opp_table *opp_table);
 void dev_pm_opp_of_register_em(struct cpumask *cpus);
 #else
 static inline int dev_pm_opp_of_add_table(struct device *dev)
@@ -420,6 +421,11 @@ static inline int of_get_required_opp_performance_state(struct device_node *np,
 {
 	return -ENOTSUPP;
 }
+
+static inline int dev_pm_opp_of_find_icc_paths(struct device *dev, struct opp_table *opp_table)
+{
+	return -ENOTSUPP;
+}
 #endif
 
 #endif		/* __LINUX_OPP_H__ */

-- 
viresh

Re: [PATCH v8 04/10] OPP: Add support for parsing interconnect bandwidth

[Viresh Kumar]

On 13-05-20, 12:11, Viresh Kumar wrote:
> On 12-05-20, 15:53, 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>
> > ---
> > v8:
> > * Drop bandwidth requests and free memory in _opp_table_kref_release.
> > * Take into account the supply_count in struct size calculations.
> > * Free the temporary arrays for peak and average bandwidth.
> > * Fix the check for opp-level.
> > * Use dev_warn insted of dev_dbg.
> > * Rename _of_find_icc_paths to _of_find_icc_paths.
> > * Rename the variable count to supply_count.
> 
> Added this delta to this patch:

+this :)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 15f30ed70bbc..06e38f95116c 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -336,7 +336,7 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev,
                                 struct opp_table *opp_table)
 {
        struct device_node *np;
-       int ret, i, count, num_paths;
+       int ret = 0, i, count, num_paths;
        struct icc_path **paths;
 
        np = of_node_get(dev->of_node);
@@ -375,10 +375,9 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev,
        if (opp_table) {
                opp_table->paths = paths;
                opp_table->path_count = num_paths;
+               return 0;
        }
 
-       return 0;
-
 err:
        while (i--)
                icc_put(paths[i]);

-- 
viresh

Re: [PATCH v8 04/10] OPP: Add support for parsing interconnect bandwidth

[Viresh Kumar]

On 12-05-20, 15:53, Georgi Djakov wrote:
>  struct dev_pm_opp *_opp_allocate(struct opp_table *table)
>  {
>  	struct dev_pm_opp *opp;
> -	int count, supply_size;
> +	int supply_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;
> +	supply_count = table->regulator_count > 0 ? table->regulator_count : 1;
> +	supply_size = sizeof(*opp->supplies) * supply_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 + supply_count);
>  	INIT_LIST_HEAD(&opp->node);

Added this delta here.

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 7302f2631f8d..dfbd3d10410c 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1330,7 +1330,8 @@ struct dev_pm_opp *_opp_allocate(struct opp_table *table)
 
        /* 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 + supply_count);
+       if (icc_size)
+               opp->bandwidth = (struct dev_pm_opp_icc_bw *)(opp->supplies + supply_count);
        INIT_LIST_HEAD(&opp->node);
 
        return opp;


-- 
viresh

Re: [PATCH v8 04/10] OPP: Add support for parsing interconnect bandwidth

[Sibi Sankar]

On 2020-05-29 10:14, Viresh Kumar wrote:
> On 12-05-20, 15:53, Georgi Djakov wrote:
>>  struct dev_pm_opp *_opp_allocate(struct opp_table *table)
>>  {
>>  	struct dev_pm_opp *opp;
>> -	int count, supply_size;
>> +	int supply_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;
>> +	supply_count = table->regulator_count > 0 ? table->regulator_count : 
>> 1;
>> +	supply_size = sizeof(*opp->supplies) * supply_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 + 
>> supply_count);
>>  	INIT_LIST_HEAD(&opp->node);
> 
> Added this delta here.
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 7302f2631f8d..dfbd3d10410c 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1330,7 +1330,8 @@ struct dev_pm_opp *_opp_allocate(struct opp_table 
> *table)
> 
>         /* 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 +
> supply_count);
> +       if (icc_size)
> +               opp->bandwidth = (struct dev_pm_opp_icc_bw
> *)(opp->supplies + supply_count);

nice catch!
Reviewed-by: Sibi Sankar <sibis@codeaurora.org>

>         INIT_LIST_HEAD(&opp->node);
> 
>         return opp;

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

[PATCH v8 05/10] OPP: Add sanity checks in _read_opp_key()

[Georgi Djakov]

When we read the OPP keys, it would be nice to do some sanity checks
of the values we get from DT and see if they match with the information
that is populated in the OPP table. Let's pass a pointer of the table,
so that we can do some validation.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Sibi Sankar <sibis@codeaurora.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
v8:
* Picked reviewed-by tags.

 drivers/opp/of.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index d139ad8c8f4f..3a64f2aa0f86 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -573,8 +573,8 @@ void dev_pm_opp_of_remove_table(struct device *dev)
 }
 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)
+static int _read_opp_key(struct dev_pm_opp *new_opp, struct opp_table *table,
+			 struct device_node *np, bool *rate_not_available)
 {
 	struct property *peak, *avg;
 	u32 *peak_bw, *avg_bw;
@@ -602,6 +602,12 @@ static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np,
 		 * opp-avg-kBps = <path1_value path2_value>;
 		 */
 		count = peak->length / sizeof(u32);
+		if (table->path_count != count) {
+			pr_err("%s: Mismatch between opp-peak-kBps and paths (%d %d)\n",
+			       __func__, count, table->path_count);
+			return -EINVAL;
+		}
+
 		peak_bw = kmalloc_array(count, sizeof(*peak_bw), GFP_KERNEL);
 		if (!peak_bw)
 			return -ENOMEM;
@@ -624,6 +630,13 @@ static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np,
 	avg = of_find_property(np, "opp-avg-kBps", NULL);
 	if (peak && avg) {
 		count = avg->length / sizeof(u32);
+		if (table->path_count != count) {
+			pr_err("%s: Mismatch between opp-avg-kBps and paths (%d %d)\n",
+			       __func__, count, table->path_count);
+			ret = -EINVAL;
+			goto free_peak_bw;
+		}
+
 		avg_bw = kmalloc_array(count, sizeof(*avg_bw), GFP_KERNEL);
 		if (!avg_bw) {
 			ret = -ENOMEM;
@@ -697,7 +710,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	if (!new_opp)
 		return ERR_PTR(-ENOMEM);
 
-	ret = _read_opp_key(new_opp, np, &rate_not_available);
+	ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
 	/* The key is optional for devices like power domains. */
 	if (ret < 0 && !opp_table->is_genpd) {
 		dev_err(dev, "%s: opp key field not found\n", __func__);

[PATCH v8 06/10] OPP: Update the bandwidth on OPP frequency changes

[Georgi Djakov]

If the OPP bandwidth values are populated, we want to switch also the
interconnect bandwidth in addition to frequency and voltage.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Sibi Sankar <sibis@codeaurora.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
v8:
* Picked reviewed-by tags.

 drivers/opp/core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index a3dd0bc9b9f6..b4cc4b12d57b 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -808,7 +808,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;
+	int ret, i;
 
 	opp_table = _find_opp_table(dev);
 	if (IS_ERR(opp_table)) {
@@ -909,6 +909,17 @@ 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);
+		}
+	}
+
 put_opp:
 	dev_pm_opp_put(opp);
 put_old_opp:

[PATCH v8 07/10] cpufreq: dt: Add support for interconnect bandwidth scaling

[Georgi Djakov]

In addition to clocks and regulators, some devices can scale the bandwidth
of their on-chip interconnect - for example between CPU and DDR memory. Add
support for that, so that platforms which support it can make use of it.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
v8:
* Picked reviewed-by tag.
* Added a separate patch to check the number of paths in DT and validate
  each one of them.

 drivers/cpufreq/Kconfig      |  1 +
 drivers/cpufreq/cpufreq-dt.c | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index c3e6bd59e920..db2ad54ee67f 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -217,6 +217,7 @@ config CPUFREQ_DT
 
 config CPUFREQ_DT_PLATDEV
 	bool
+	depends on INTERCONNECT || !INTERCONNECT
 	help
 	  This adds a generic DT based cpufreq platdev driver for frequency
 	  management.  This creates a 'cpufreq-dt' platform device, on the
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 26fe8dfb9ce6..4ecef3257532 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -13,6 +13,7 @@
 #include <linux/cpufreq.h>
 #include <linux/cpumask.h>
 #include <linux/err.h>
+#include <linux/interconnect.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pm_opp.h>
@@ -95,6 +96,7 @@ static int resources_available(void)
 	struct device *cpu_dev;
 	struct regulator *cpu_reg;
 	struct clk *cpu_clk;
+	struct icc_path *cpu_path;
 	int ret = 0;
 	const char *name;
 
@@ -121,6 +123,19 @@ static int resources_available(void)
 
 	clk_put(cpu_clk);
 
+	cpu_path = of_icc_get(cpu_dev, NULL);
+	ret = PTR_ERR_OR_ZERO(cpu_path);
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
+			dev_dbg(cpu_dev, "defer icc path: %d\n", ret);
+		else
+			dev_err(cpu_dev, "failed to get icc path: %d\n", ret);
+
+		return ret;
+	}
+
+	icc_put(cpu_path);
+
 	name = find_supply_name(cpu_dev);
 	/* Platform doesn't require regulator */
 	if (!name)

[PATCH v8 08/10] cpufreq: dt: Validate all interconnect paths

[Georgi Djakov]

Currently when we check for the available resources, we assume that there
is only one interconnect path, but in fact it could be more than one. Do
some validation to determine the number of paths and verify if each one
of them is available.

Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
v8:
* New patch.

 drivers/cpufreq/cpufreq-dt.c | 49 ++++++++++++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 4ecef3257532..3dd28c2c1633 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -91,12 +91,54 @@ static const char *find_supply_name(struct device *dev)
 	return name;
 }
 
+static int find_icc_paths(struct device *dev)
+{
+	struct device_node *np;
+	struct icc_path **paths;
+	int i, count, num_paths;
+	int ret = 0;
+
+	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;
+	paths = kcalloc(num_paths, sizeof(*paths), GFP_KERNEL);
+	if (!paths)
+		return -ENOMEM;
+
+	for (i = 0; i < num_paths; i++) {
+		paths[i] = of_icc_get_by_index(dev, i);
+		ret = PTR_ERR_OR_ZERO(paths[i]);
+		if (ret)
+			break;
+	}
+
+	while (i--)
+		icc_put(paths[i]);
+
+	kfree(paths);
+
+	return ret;
+}
+
 static int resources_available(void)
 {
 	struct device *cpu_dev;
 	struct regulator *cpu_reg;
 	struct clk *cpu_clk;
-	struct icc_path *cpu_path;
 	int ret = 0;
 	const char *name;
 
@@ -123,8 +165,7 @@ static int resources_available(void)
 
 	clk_put(cpu_clk);
 
-	cpu_path = of_icc_get(cpu_dev, NULL);
-	ret = PTR_ERR_OR_ZERO(cpu_path);
+	ret = find_icc_paths(cpu_dev);
 	if (ret) {
 		if (ret == -EPROBE_DEFER)
 			dev_dbg(cpu_dev, "defer icc path: %d\n", ret);
@@ -134,8 +175,6 @@ static int resources_available(void)
 		return ret;
 	}
 
-	icc_put(cpu_path);
-
 	name = find_supply_name(cpu_dev);
 	/* Platform doesn't require regulator */
 	if (!name)

Re: [PATCH v8 08/10] cpufreq: dt: Validate all interconnect paths

[Matthias Kaehlcke]

Hi Georgi,

On Tue, May 12, 2020 at 03:53:25PM +0300, Georgi Djakov wrote:
> Currently when we check for the available resources, we assume that there
> is only one interconnect path, but in fact it could be more than one. Do
> some validation to determine the number of paths and verify if each one
> of them is available.
> 
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
> v8:
> * New patch.
> 
>  drivers/cpufreq/cpufreq-dt.c | 49 ++++++++++++++++++++++++++++++++----
>  1 file changed, 44 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index 4ecef3257532..3dd28c2c1633 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -91,12 +91,54 @@ static const char *find_supply_name(struct device *dev)
>  	return name;
>  }
>  
> +static int find_icc_paths(struct device *dev)
> +{
> +	struct device_node *np;
> +	struct icc_path **paths;
> +	int i, count, num_paths;
> +	int ret = 0;
> +
> +	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;
> +	paths = kcalloc(num_paths, sizeof(*paths), GFP_KERNEL);
> +	if (!paths)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_paths; i++) {
> +		paths[i] = of_icc_get_by_index(dev, i);
> +		ret = PTR_ERR_OR_ZERO(paths[i]);
> +		if (ret)
> +			break;
> +	}
> +
> +	while (i--)
> +		icc_put(paths[i]);

Since the function only does a validation and throws the paths away
afterwards you don't really need the dynamic allocation and 'icc_put'
loop. Just have a single 'struct icc_path' pointer and call icc_put()
inside the for loop.

Re: [PATCH v8 08/10] cpufreq: dt: Validate all interconnect paths

[Viresh Kumar]

On 12-05-20, 15:53, Georgi Djakov wrote:
> Currently when we check for the available resources, we assume that there
> is only one interconnect path, but in fact it could be more than one. Do
> some validation to determine the number of paths and verify if each one
> of them is available.
> 
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
> v8:
> * New patch.

Merged this with the 7th patch and applied this delta:

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 15d70112454c..79742bbd221f 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -13,7 +13,6 @@
 #include <linux/cpufreq.h>
 #include <linux/cpumask.h>
 #include <linux/err.h>
-#include <linux/interconnect.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pm_opp.h>
@@ -91,49 +90,6 @@ static const char *find_supply_name(struct device *dev)
 	return name;
 }
 
-static int find_icc_paths(struct device *dev)
-{
-	struct device_node *np;
-	struct icc_path **paths;
-	int i, count, num_paths;
-	int ret = 0;
-
-	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;
-	paths = kcalloc(num_paths, sizeof(*paths), GFP_KERNEL);
-	if (!paths)
-		return -ENOMEM;
-
-	for (i = 0; i < num_paths; i++) {
-		paths[i] = of_icc_get_by_index(dev, i);
-		ret = PTR_ERR_OR_ZERO(paths[i]);
-		if (ret)
-			break;
-	}
-
-	while (i--)
-		icc_put(paths[i]);
-
-	kfree(paths);
-
-	return ret;
-}
-
 static int resources_available(void)
 {
 	struct device *cpu_dev;
@@ -165,15 +121,9 @@ static int resources_available(void)
 
 	clk_put(cpu_clk);
 
-	ret = find_icc_paths(cpu_dev);
-	if (ret) {
-		if (ret == -EPROBE_DEFER)
-			dev_dbg(cpu_dev, "defer icc path: %d\n", ret);
-		else
-			dev_err(cpu_dev, "failed to get icc path: %d\n", ret);
-
+	ret = dev_pm_opp_of_find_icc_paths(cpu_dev, NULL);
+	if (ret)
 		return ret;
-	}
 
 	name = find_supply_name(cpu_dev);
 	/* Platform doesn't require regulator */

-- 
viresh

[PATCH v8 09/10] dt-bindings: interconnect: Add interconnect-tags bindings

[Georgi Djakov]

From: Sibi Sankar <sibis@codeaurora.org>

Add interconnect-tags bindings to enable passing of optional
tag information to the interconnect framework.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
v8:
* New patch, picked from here:
  https://lore.kernel.org/r/20200504202243.5476-10-sibis@codeaurora.org

 .../devicetree/bindings/interconnect/interconnect.txt        | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt
index 6f5d23a605b7..c1a226a934e5 100644
--- a/Documentation/devicetree/bindings/interconnect/interconnect.txt
+++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt
@@ -55,6 +55,11 @@ interconnect-names : List of interconnect path name strings sorted in the same
 			 * dma-mem: Path from the device to the main memory of
 			            the system
 
+interconnect-tags : List of interconnect path tags sorted in the same order as the
+		    interconnects property. Consumers can append a specific tag to
+		    the path and pass this information to the interconnect framework
+		    to do aggregation based on the attached tag.
+
 Example:
 
 	sdhci@7864000 {

Re: [PATCH v8 09/10] dt-bindings: interconnect: Add interconnect-tags bindings

[Viresh Kumar]

On 12-05-20, 15:53, Georgi Djakov wrote:
> From: Sibi Sankar <sibis@codeaurora.org>
> 
> Add interconnect-tags bindings to enable passing of optional
> tag information to the interconnect framework.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
> v8:
> * New patch, picked from here:
>   https://lore.kernel.org/r/20200504202243.5476-10-sibis@codeaurora.org
> 
>  .../devicetree/bindings/interconnect/interconnect.txt        | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt
> index 6f5d23a605b7..c1a226a934e5 100644
> --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt
> +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt
> @@ -55,6 +55,11 @@ interconnect-names : List of interconnect path name strings sorted in the same
>  			 * dma-mem: Path from the device to the main memory of
>  			            the system
>  
> +interconnect-tags : List of interconnect path tags sorted in the same order as the
> +		    interconnects property. Consumers can append a specific tag to
> +		    the path and pass this information to the interconnect framework
> +		    to do aggregation based on the attached tag.
> +
>  Example:
>  
>  	sdhci@7864000 {

@Rob: Though I have applied the patch to my branch for now, I can
revert it just fine if you aren't okay with the bindings. Please lemme
know about your feedback on this (sorry for missing that earlier).

-- 
viresh

Re: [PATCH v8 09/10] dt-bindings: interconnect: Add interconnect-tags bindings

[Rob Herring]

On Tue, May 12, 2020 at 03:53:26PM +0300, Georgi Djakov wrote:
> From: Sibi Sankar <sibis@codeaurora.org>
> 
> Add interconnect-tags bindings to enable passing of optional
> tag information to the interconnect framework.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
> v8:
> * New patch, picked from here:
>   https://lore.kernel.org/r/20200504202243.5476-10-sibis@codeaurora.org
> 
>  .../devicetree/bindings/interconnect/interconnect.txt        | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt
> index 6f5d23a605b7..c1a226a934e5 100644
> --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt
> +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt
> @@ -55,6 +55,11 @@ interconnect-names : List of interconnect path name strings sorted in the same
>  			 * dma-mem: Path from the device to the main memory of
>  			            the system
>  
> +interconnect-tags : List of interconnect path tags sorted in the same order as the
> +		    interconnects property. Consumers can append a specific tag to
> +		    the path and pass this information to the interconnect framework
> +		    to do aggregation based on the attached tag.

Why isn't this information in the 'interconnect' arg cells?

We have 'interconnect-names' because strings don't mix with cells. An 
expanding list of 'interconnect-.*' is not a good pattern IMO.

Rob

Re: [PATCH v8 09/10] dt-bindings: interconnect: Add interconnect-tags bindings

[Saravana Kannan]

On Tue, May 19, 2020 at 11:58 AM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, May 12, 2020 at 03:53:26PM +0300, Georgi Djakov wrote:
> > From: Sibi Sankar <sibis@codeaurora.org>
> >
> > Add interconnect-tags bindings to enable passing of optional
> > tag information to the interconnect framework.
> >
> > Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> > ---
> > v8:
> > * New patch, picked from here:
> >   https://lore.kernel.org/r/20200504202243.5476-10-sibis@codeaurora.org
> >
> >  .../devicetree/bindings/interconnect/interconnect.txt        | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt
> > index 6f5d23a605b7..c1a226a934e5 100644
> > --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt
> > +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt
> > @@ -55,6 +55,11 @@ interconnect-names : List of interconnect path name strings sorted in the same
> >                        * dma-mem: Path from the device to the main memory of
> >                                   the system
> >
> > +interconnect-tags : List of interconnect path tags sorted in the same order as the
> > +                 interconnects property. Consumers can append a specific tag to
> > +                 the path and pass this information to the interconnect framework
> > +                 to do aggregation based on the attached tag.
>
> Why isn't this information in the 'interconnect' arg cells?
>
> We have 'interconnect-names' because strings don't mix with cells. An
> expanding list of 'interconnect-.*' is not a good pattern IMO.

Also, is there an example for interconnect-tags that I missed? Is it a
list of strings, numbers, etc?

-Saravana

Re: [PATCH v8 09/10] dt-bindings: interconnect: Add interconnect-tags bindings

[Sibi Sankar]

On 2020-05-20 01:27, Saravana Kannan wrote:
> On Tue, May 19, 2020 at 11:58 AM Rob Herring <robh@kernel.org> wrote:
>> 
>> On Tue, May 12, 2020 at 03:53:26PM +0300, Georgi Djakov wrote:
>> > From: Sibi Sankar <sibis@codeaurora.org>
>> >
>> > Add interconnect-tags bindings to enable passing of optional
>> > tag information to the interconnect framework.
>> >
>> > Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> > ---
>> > v8:
>> > * New patch, picked from here:
>> >   https://lore.kernel.org/r/20200504202243.5476-10-sibis@codeaurora.org
>> >
>> >  .../devicetree/bindings/interconnect/interconnect.txt        | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt
>> > index 6f5d23a605b7..c1a226a934e5 100644
>> > --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt
>> > +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt
>> > @@ -55,6 +55,11 @@ interconnect-names : List of interconnect path name strings sorted in the same
>> >                        * dma-mem: Path from the device to the main memory of
>> >                                   the system
>> >
>> > +interconnect-tags : List of interconnect path tags sorted in the same order as the
>> > +                 interconnects property. Consumers can append a specific tag to
>> > +                 the path and pass this information to the interconnect framework
>> > +                 to do aggregation based on the attached tag.
>> 
>> Why isn't this information in the 'interconnect' arg cells?
>> 
>> We have 'interconnect-names' because strings don't mix with cells. An
>> expanding list of 'interconnect-.*' is not a good pattern IMO.

Rob,
Currently the interconnect paths
assume a default tag and only few
icc paths require tags that differ
from the default ones. Encoding the
tags in the interconnect arg cells
would force all paths to specify
the tags. I guess that's okay.


> 
> Also, is there an example for interconnect-tags that I missed? Is it a
> list of strings, numbers, etc?

Saravana,
https://patchwork.kernel.org/patch/11527589/
^^ is an example of interconnect-tag useage.

> 
> -Saravana

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

Re: [PATCH v8 09/10] dt-bindings: interconnect: Add interconnect-tags bindings

[Saravana Kannan]

On Wed, May 20, 2020 at 11:51 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> On 2020-05-20 01:27, Saravana Kannan wrote:
> > On Tue, May 19, 2020 at 11:58 AM Rob Herring <robh@kernel.org> wrote:
> >>
> >> On Tue, May 12, 2020 at 03:53:26PM +0300, Georgi Djakov wrote:
> >> > From: Sibi Sankar <sibis@codeaurora.org>
> >> >
> >> > Add interconnect-tags bindings to enable passing of optional
> >> > tag information to the interconnect framework.
> >> >
> >> > Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> >> > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> >> > ---
> >> > v8:
> >> > * New patch, picked from here:
> >> >   https://lore.kernel.org/r/20200504202243.5476-10-sibis@codeaurora.org
> >> >
> >> >  .../devicetree/bindings/interconnect/interconnect.txt        | 5 +++++
> >> >  1 file changed, 5 insertions(+)
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt
> >> > index 6f5d23a605b7..c1a226a934e5 100644
> >> > --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt
> >> > +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt
> >> > @@ -55,6 +55,11 @@ interconnect-names : List of interconnect path name strings sorted in the same
> >> >                        * dma-mem: Path from the device to the main memory of
> >> >                                   the system
> >> >
> >> > +interconnect-tags : List of interconnect path tags sorted in the same order as the
> >> > +                 interconnects property. Consumers can append a specific tag to
> >> > +                 the path and pass this information to the interconnect framework
> >> > +                 to do aggregation based on the attached tag.
> >>
> >> Why isn't this information in the 'interconnect' arg cells?
> >>
> >> We have 'interconnect-names' because strings don't mix with cells. An
> >> expanding list of 'interconnect-.*' is not a good pattern IMO.
>
> Rob,
> Currently the interconnect paths
> assume a default tag and only few
> icc paths require tags that differ
> from the default ones. Encoding the
> tags in the interconnect arg cells
> would force all paths to specify
> the tags. I guess that's okay.

I think that's the right thing. Those cells are meant to be "args" to
the provider.

> >
> > Also, is there an example for interconnect-tags that I missed? Is it a
> > list of strings, numbers, etc?
>
> Saravana,
> https://patchwork.kernel.org/patch/11527589/
> ^^ is an example of interconnect-tag useage.

If we actually merge interconnect-tags, I think the doc should be
updated. Instead of having to grep around.

-Saravana

Re: [PATCH v8 09/10] dt-bindings: interconnect: Add interconnect-tags bindings

[Sibi Sankar]

On 5/21/20 12:43 AM, Saravana Kannan wrote:
> On Wed, May 20, 2020 at 11:51 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>>
>> On 2020-05-20 01:27, Saravana Kannan wrote:
>>> On Tue, May 19, 2020 at 11:58 AM Rob Herring <robh@kernel.org> wrote:
>>>>
>>>> On Tue, May 12, 2020 at 03:53:26PM +0300, Georgi Djakov wrote:
>>>>> From: Sibi Sankar <sibis@codeaurora.org>
>>>>>
>>>>> Add interconnect-tags bindings to enable passing of optional
>>>>> tag information to the interconnect framework.
>>>>>
>>>>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>>>>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>>>>> ---
>>>>> v8:
>>>>> * New patch, picked from here:
>>>>>    https://lore.kernel.org/r/20200504202243.5476-10-sibis@codeaurora.org
>>>>>
>>>>>   .../devicetree/bindings/interconnect/interconnect.txt        | 5 +++++
>>>>>   1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt
>>>>> index 6f5d23a605b7..c1a226a934e5 100644
>>>>> --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt
>>>>> +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt
>>>>> @@ -55,6 +55,11 @@ interconnect-names : List of interconnect path name strings sorted in the same
>>>>>                         * dma-mem: Path from the device to the main memory of
>>>>>                                    the system
>>>>>
>>>>> +interconnect-tags : List of interconnect path tags sorted in the same order as the
>>>>> +                 interconnects property. Consumers can append a specific tag to
>>>>> +                 the path and pass this information to the interconnect framework
>>>>> +                 to do aggregation based on the attached tag.
>>>>
>>>> Why isn't this information in the 'interconnect' arg cells?
>>>>
>>>> We have 'interconnect-names' because strings don't mix with cells. An
>>>> expanding list of 'interconnect-.*' is not a good pattern IMO.
>>
>> Rob,
>> Currently the interconnect paths
>> assume a default tag and only few
>> icc paths require tags that differ
>> from the default ones. Encoding the
>> tags in the interconnect arg cells
>> would force all paths to specify
>> the tags. I guess that's okay.
> 
> I think that's the right thing. Those cells are meant to be "args" to
> the provider.

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi 
b/arch/arm64/boot/dts/qcom/sc7180.dtsi

index ea4764f06a901..b34f024d4ab63 100644

--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi

+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi

@@ -132,9 +132,8 @@ &LITTLE_CPU_SLEEP_1

                         capacity-dmips-mhz = <1024>;

                         dynamic-power-coefficient = <100>;

                         operating-points-v2 = <&cpu0_opp_table>;

-                       interconnects = <&gem_noc MASTER_APPSS_PROC 
&mc_virt SLAVE_EBI1>,

+                       interconnects = <&gem_noc MASTER_APPSS_PROC 3 
&mc_virt SLAVE_EBI1 3>,

                                         <&osm_l3 MASTER_OSM_L3_APPS 
&osm_l3 SLAVE_OSM_L3>;

                         next-level-cache = <&L2_0>;

                         #cooling-cells = <2>;

                         qcom,freq-domain = <&cpufreq_hw 0>;



....

                 mc_virt: interconnect@1638000 {

                         compatible = "qcom,sc7180-mc-virt";

                         reg = <0 0x01638000 0 0x1000>;

-                       #interconnect-cells = <1>;

+                       #interconnect-cells = <2>;

                         qcom,bcm-voters = <&apps_bcm_voter>;

                 };

....



@@ -2216,14 +2208,14 @@ system-cache-controller@9200000 {

                 gem_noc: interconnect@9680000 {

                         compatible = "qcom,sc7180-gem-noc";

                         reg = <0 0x09680000 0 0x3e200>;

-                       #interconnect-cells = <1>;

+                       #interconnect-cells = <2>;

                         qcom,bcm-voters = <&apps_bcm_voter>;

                 };

....



diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c

index 294e9c58565bb..6a7a785bd90b9 100644

--- a/drivers/interconnect/core.c

+++ b/drivers/interconnect/core.c

@@ -340,7 +340,7 @@ static struct icc_node 
*of_icc_get_from_provider(struct of_phandle_args *spec)

         struct icc_node *node = ERR_PTR(-EPROBE_DEFER);

         struct icc_provider *provider;



-       if (!spec || spec->args_count != 1)

+       if (!spec || spec->args_count < 1)

                 return ERR_PTR(-EINVAL);



         mutex_lock(&icc_lock);

@@ -469,6 +469,9 @@ struct icc_path *of_icc_get_by_index(struct device 
*dev, int idx)

                 return ERR_PTR(-ENOMEM);

         }



+       if (src_args.args_count == 2)

+               icc_set_tag(path, src_args.args[1]);

diff: https://paste.ubuntu.com/p/sRRYhxQjsV/

Saravana/Georgi,
A few concerns here, I feel tag info as the second arg to the provider
may not be true for all socs. Does introducing soc specific of_icc_get
functions make sense?

> 
>>>
>>> Also, is there an example for interconnect-tags that I missed? Is it a
>>> list of strings, numbers, etc?
>>
>> Saravana,
>> https://patchwork.kernel.org/patch/11527589/
>> ^^ is an example of interconnect-tag useage.
> 
> If we actually merge interconnect-tags, I think the doc should be
> updated. Instead of having to grep around.
> 
> -Saravana
> 

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

[PATCH v8 10/10] OPP: Add support for setting interconnect-tags

[Georgi Djakov]

From: Sibi Sankar <sibis@codeaurora.org>

Add support for setting tags on icc paths associated with
the opp_table.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
v8:
* New patch, picked from here:
  https://lore.kernel.org/r/20200504202243.5476-11-sibis@codeaurora.org

 drivers/opp/of.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 3a64f2aa0f86..fd148d54022f 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -336,6 +336,7 @@ int _of_find_icc_paths(struct opp_table *opp_table, struct device *dev)
 {
 	struct device_node *np;
 	int ret, i, count, num_paths;
+	u32 tag;
 
 	np = of_node_get(dev->of_node);
 	if (!np)
@@ -344,20 +345,25 @@ int _of_find_icc_paths(struct opp_table *opp_table, struct device *dev)
 	count = of_count_phandle_with_args(np, "interconnects",
 					   "#interconnect-cells");
 	of_node_put(np);
-	if (count < 0)
-		return 0;
+	if (count < 0) {
+		ret = 0;
+		goto put_np;
+	}
 
 	/* two phandles when #interconnect-cells = <1> */
 	if (count % 2) {
 		dev_err(dev, "%s: Invalid interconnects values\n", __func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto put_np;
 	}
 
 	num_paths = count / 2;
 	opp_table->paths = kcalloc(num_paths, sizeof(*opp_table->paths),
 				   GFP_KERNEL);
-	if (!opp_table->paths)
-		return -ENOMEM;
+	if (!opp_table->paths) {
+		ret = -ENOMEM;
+		goto put_np;
+	}
 
 	for (i = 0; i < num_paths; i++) {
 		opp_table->paths[i] = of_icc_get_by_index(dev, i);
@@ -369,8 +375,14 @@ int _of_find_icc_paths(struct opp_table *opp_table, struct device *dev)
 			}
 			goto err;
 		}
+
+		/* Set tag if present */
+		if (!of_property_read_u32_index(np, "interconnect-tags",
+						i, &tag))
+			icc_set_tag(opp_table->paths[i], tag);
 	}
 	opp_table->path_count = num_paths;
+	of_node_put(np);
 
 	return 0;
 
@@ -380,6 +392,8 @@ int _of_find_icc_paths(struct opp_table *opp_table, struct device *dev)
 
 	kfree(opp_table->paths);
 	opp_table->paths = NULL;
+put_np:
+	of_node_put(np);
 
 	return ret;
 }

Re: [PATCH v8 10/10] OPP: Add support for setting interconnect-tags

[Viresh Kumar]

On 12-05-20, 15:53, Georgi Djakov wrote:
> From: Sibi Sankar <sibis@codeaurora.org>
> 
> Add support for setting tags on icc paths associated with
> the opp_table.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---

Applied this delta:

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index f71dc5eb0eba..a4972c94f428 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -346,7 +346,6 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev,
 
 	count = of_count_phandle_with_args(np, "interconnects",
 					   "#interconnect-cells");
-	of_node_put(np);
 	if (count < 0) {
 		ret = 0;
 		goto put_np;
@@ -380,7 +379,7 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev,
 		/* Set tag if present */
 		if (!of_property_read_u32_index(np, "interconnect-tags",
 						i, &tag))
-			icc_set_tag(opp_table->paths[i], tag);
+			icc_set_tag(paths[i], tag);
 	}
 
 	if (opp_table) {

-- 
viresh

Re: [PATCH v8 00/10] Introduce OPP bandwidth bindings

[Viresh Kumar]

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

Re: [PATCH v8 00/10] Introduce OPP bandwidth bindings

[Georgi Djakov]

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

Re: [PATCH v8 00/10] Introduce OPP bandwidth bindings

[Viresh Kumar]

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

[PATCH] opp: Expose bandwidth information via debugfs

[Viresh Kumar]

Expose the bandwidth information as well via debugfs.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
@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

[PATCH] opp: Remove bandwidth votes when target_freq is zero

[Viresh Kumar]

We already drop several votes when target_freq is set to zero, drop
bandwidth votes as well.

Reported-by: Sibi Sankar <sibis@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
@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

[PATCH V2] opp: Remove bandwidth votes when target_freq is zero

[Viresh Kumar]

We already drop several votes when target_freq is set to zero, drop
bandwidth votes as well.

Reported-by: Sibi Sankar <sibis@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
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

Re: [PATCH V2] opp: Remove bandwidth votes when target_freq is zero

[Georgi Djakov]

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 <sibis@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Georgi Djakov <georgi.djakov@linaro.org>
Tested-by: Georgi Djakov <georgi.djakov@linaro.org>

Thanks!
Georgi

Re: [PATCH V2] opp: Remove bandwidth votes when target_freq is zero

[Sibi Sankar]

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 <sibis@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2: Some changes left uncommited in my tree by mistake.

Viresh,
Thanks for the patch :)

Tested-by: Sibi Sankar <sibis@codeaurora.org>
Reviewed-by: Sibi Sankar <sibis@codeaurora.org>

> 
>  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.