[PATCH v6 0/3] Introduce Bandwidth OPPs for interconnects

[PATCH v6 0/3] Introduce Bandwidth OPPs for interconnects

[Saravana Kannan]

Viresh/Stephen,

I don't think all the additional code/diff in this v6 series is worth it
to avoid using the rate field to store peak bandwidth. However, since folks
weren't too happy about it, here it is. I prefer the v5 series, but not
too strongly tied to it. Let me know what you think Viresh/Stephen.

Btw, I wasn't sure of opp-hz = 0 or opp-level = 0 were allowed. Also,
it's not clear why the duplicate check isn't done for opp-level when
_opp_add() is called. Based on that, we could add opp-level comparison
to opp_compare_key(). That's why you'll see a few spurious
opp_key.level = 0 lines. Let me know how you want to go with that.

I could also add a opp.key_type enum field to store what key type the
opp entry is. But looks like I can get away without adding an
unnecessary variable. So, I've skipped that for now.

------

Interconnects and interconnect paths quantify their performance levels in
terms of bandwidth and not in terms of frequency. So similar to how we have
frequency based OPP tables in DT and in the OPP framework, we need
bandwidth OPP table support in DT and in the OPP framework.

So with the DT bindings added in this patch series, the DT for a GPU
that does bandwidth voting from GPU to Cache and GPU to DDR would look
something like this:

gpu_cache_opp_table: gpu_cache_opp_table {
	compatible = "operating-points-v2";

	gpu_cache_3000: opp-3000 {
		opp-peak-KBps = <3000000>;
		opp-avg-KBps = <1000000>;
	};
	gpu_cache_6000: opp-6000 {
		opp-peak-KBps = <6000000>;
		opp-avg-KBps = <2000000>;
	};
	gpu_cache_9000: opp-9000 {
		opp-peak-KBps = <9000000>;
		opp-avg-KBps = <9000000>;
	};
};

gpu_ddr_opp_table: gpu_ddr_opp_table {
	compatible = "operating-points-v2";

	gpu_ddr_1525: opp-1525 {
		opp-peak-KBps = <1525000>;
		opp-avg-KBps = <452000>;
	};
	gpu_ddr_3051: opp-3051 {
		opp-peak-KBps = <3051000>;
		opp-avg-KBps = <915000>;
	};
	gpu_ddr_7500: opp-7500 {
		opp-peak-KBps = <7500000>;
		opp-avg-KBps = <3000000>;
	};
};

gpu_opp_table: gpu_opp_table {
	compatible = "operating-points-v2";
	opp-shared;

	opp-200000000 {
		opp-hz = /bits/ 64 <200000000>;
	};
	opp-400000000 {
		opp-hz = /bits/ 64 <400000000>;
	};
};

gpu@7864000 {
	...
	operating-points-v2 = <&gpu_opp_table>, <&gpu_cache_opp_table>, <&gpu_ddr_opp_table>;
	...
};

v1 -> v3:
- Lots of patch additions that were later dropped
v3 -> v4:
- Fixed typo bugs pointed out by Sibi.
- Fixed bug that incorrectly reset rate to 0 all the time
- Added units documentation
- Dropped interconnect-opp-table property and related changes
v4->v5:
- Replaced KBps with kBps
- Minor documentation fix
v5->v6:
- Added Rob's reviewed-by for the DT patch
- Rewrote OPP patches to use separate field for peak_bw instead of
  reusing rate field.
- Pulled in opp-level parsing into _read_opp_key
- Addressed minor code style and typo comments

Cheers,
Saravana

Saravana Kannan (3):
  dt-bindings: opp: Introduce opp-peak-kBps and opp-avg-kBps bindings
  OPP: Add support for bandwidth OPP tables
  OPP: Add helper function for bandwidth OPP tables

 Documentation/devicetree/bindings/opp/opp.txt |  15 +-
 .../devicetree/bindings/property-units.txt    |   4 +
 drivers/opp/core.c                            | 316 +++++++++++++++---
 drivers/opp/of.c                              |  63 ++--
 drivers/opp/opp.h                             |   5 +
 include/linux/pm_opp.h                        |  43 +++
 6 files changed, 383 insertions(+), 63 deletions(-)

-- 
2.24.0.393.g34dc348eaf-goog

[PATCH v6 1/3] dt-bindings: opp: Introduce opp-peak-kBps and opp-avg-kBps bindings

[Saravana Kannan]

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: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/opp/opp.txt     | 15 ++++++++++++---
 .../devicetree/bindings/property-units.txt        |  4 ++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index 68592271461f..dbad8eb6c746 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 except for devices like power domains
+  or bandwidth opp tables. The power domain nodes must have another
+  (implementation dependent) property which uniquely identifies the OPP nodes.
+  The interconnect opps are required to have the opp-peak-kBps property.
+
+- opp-peak-kBps: Peak bandwidth in kilobytes per second, expressed as a 32-bit
+  big-endian integer. This is a required property for all devices that don't
+  have opp-hz. For example, bandwidth OPP tables for interconnect paths.
 
 Optional properties:
 - opp-microvolt: voltage in micro Volts.
@@ -132,6 +137,10 @@ 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 a
+  32-bit big-endian integer. 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
-- 
2.24.0.393.g34dc348eaf-goog

[PATCH v6 2/3] OPP: Add support for bandwidth OPP tables

[Saravana Kannan]

Not all devices quantify their performance points in terms of frequency.
Devices like interconnects quantify their performance points in terms of
bandwidth. We need a way to represent these bandwidth levels in OPP. So,
add support for parsing bandwidth OPPs from DT.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/opp/core.c | 15 +++++++++--
 drivers/opp/of.c   | 63 ++++++++++++++++++++++++++++++++--------------
 drivers/opp/opp.h  |  5 ++++
 3 files changed, 62 insertions(+), 21 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index be7a7d332332..c79bbfac7289 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1282,11 +1282,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->peak_bw != opp2->peak_bw)
+		return opp1->peak_bw < opp2->peak_bw ? -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
@@ -1297,12 +1307,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 1cbb58240b80..b565da5a2b1f 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -521,6 +521,44 @@ 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)
+{
+	int ret;
+	u64 rate;
+	u32 bw;
+
+	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;
+		goto out;
+	}
+
+	ret = of_property_read_u32(np, "opp-peak-kBps", &bw);
+	if (!ret) {
+		new_opp->peak_bw = bw;
+
+		if (!of_property_read_u32(np, "opp-avg-kBps", &bw))
+			new_opp->avg_bw = bw;
+	}
+
+out:
+	*rate_not_available = !!ret;
+	/*
+	 * If ret is 0 at this point, we have already found a key. If we
+	 * haven't found a key yet, then ret already has an error value. In
+	 * either case, we don't need to update 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 +596,12 @@ 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);
+	if (ret) {
+		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);
@@ -616,7 +640,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	if (of_property_read_bool(np, "opp-suspend")) {
 		if (opp_table->suspend_opp) {
 			/* Pick the OPP with higher rate as suspend OPP */
-			if (new_opp->rate > opp_table->suspend_opp->rate) {
+			if (opp_compare_key(new_opp,
+					    opp_table->suspend_opp) > 1) {
 				opp_table->suspend_opp->suspend = false;
 				new_opp->suspend = true;
 				opp_table->suspend_opp = new_opp;
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 01a500e2c40a..0def3154d07b 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -57,6 +57,8 @@ extern struct list_head opp_tables;
  * @suspend:	true if suspend OPP
  * @pstate: Device's power domain's performance state.
  * @rate:	Frequency in hertz
+ * @peak_bw:	Peak bandwidth in kilobytes per second
+ * @avg_bw:	Average bandwidth in kilobytes per second
  * @level:	Performance level
  * @supplies:	Power supplies voltage/current values
  * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
@@ -78,6 +80,8 @@ struct dev_pm_opp {
 	bool suspend;
 	unsigned int pstate;
 	unsigned long rate;
+	unsigned int peak_bw;
+	unsigned int avg_bw;
 	unsigned int level;
 
 	struct dev_pm_opp_supply *supplies;
@@ -213,6 +217,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);
-- 
2.24.0.393.g34dc348eaf-goog

[PATCH v6 3/3] OPP: Add helper function for bandwidth OPP tables

[Saravana Kannan]

The frequency OPP tables have helper functions to search for entries in the
table based on frequency and get the frequency values for a given (or
suspend) OPP entry.

Add similar helper functions for bandwidth OPP tables to search for entries
in the table based on peak bandwidth and to get the peak and average
bandwidth for a given (or suspend) OPP entry.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/opp/core.c     | 301 +++++++++++++++++++++++++++++++++++------
 include/linux/pm_opp.h |  43 ++++++
 2 files changed, 305 insertions(+), 39 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c79bbfac7289..3ff33a08198e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -127,6 +127,29 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
 
+/**
+ * dev_pm_opp_get_bw() - Gets the bandwidth corresponding to an available opp
+ * @opp:	opp for which peak bandwidth has to be returned for
+ * @avg_bw:	Pointer where the corresponding average bandwidth is stored.
+ *		Can be NULL.
+ *
+ * Return: Peak bandwidth in kBps corresponding to the opp, else
+ * return 0
+ */
+unsigned long dev_pm_opp_get_bw(struct dev_pm_opp *opp, unsigned long *avg_bw)
+{
+	if (IS_ERR_OR_NULL(opp) || !opp->available) {
+		pr_err("%s: Invalid parameters\n", __func__);
+		return 0;
+	}
+
+	if (avg_bw)
+		*avg_bw = opp->avg_bw;
+
+	return opp->peak_bw;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_bw);
+
 /**
  * dev_pm_opp_get_level() - Gets the level corresponding to an available opp
  * @opp:	opp for which level value has to be returned for
@@ -299,6 +322,34 @@ unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_freq);
 
+/**
+ * dev_pm_opp_get_suspend_opp_bw() - Get peak bandwidth of suspend opp in kBps
+ * @dev:	device for which we do this operation
+ * @avg_bw:	Pointer where the corresponding average bandwidth is stored.
+ *		Can be NULL.
+ *
+ * Return: This function returns the peak bandwidth of the OPP marked as
+ * suspend_opp if one is available, else returns 0;
+ */
+unsigned long dev_pm_opp_get_suspend_opp_bw(struct device *dev,
+					    unsigned long *avg_bw)
+{
+	struct opp_table *opp_table;
+	unsigned long peak_bw = 0;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return 0;
+
+	if (opp_table->suspend_opp && opp_table->suspend_opp->available)
+		peak_bw = dev_pm_opp_get_bw(opp_table->suspend_opp, avg_bw);
+
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return peak_bw;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_bw);
+
 int _get_opp_count(struct opp_table *opp_table)
 {
 	struct dev_pm_opp *opp;
@@ -343,6 +394,40 @@ int dev_pm_opp_get_opp_count(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_count);
 
+struct dev_pm_opp *dev_pm_opp_find_opp_exact(struct device *dev,
+					      struct dev_pm_opp *opp_key,
+					      bool available)
+{
+	struct opp_table *opp_table;
+	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table)) {
+		int r = PTR_ERR(opp_table);
+
+		dev_err(dev, "%s: OPP table not found (%d)\n", __func__, r);
+		return ERR_PTR(r);
+	}
+
+	mutex_lock(&opp_table->lock);
+
+	list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
+		if (temp_opp->available == available &&
+		    !opp_compare_key(temp_opp, opp_key)) {
+			opp = temp_opp;
+
+			/* Increment the reference count of OPP */
+			dev_pm_opp_get(opp);
+			break;
+		}
+	}
+
+	mutex_unlock(&opp_table->lock);
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return opp;
+}
+
 /**
  * dev_pm_opp_find_freq_exact() - search for an exact frequency
  * @dev:		device for which we do this operation
@@ -370,36 +455,53 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 					      unsigned long freq,
 					      bool available)
 {
-	struct opp_table *opp_table;
-	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
-
-	opp_table = _find_opp_table(dev);
-	if (IS_ERR(opp_table)) {
-		int r = PTR_ERR(opp_table);
+	struct dev_pm_opp opp_key;
 
-		dev_err(dev, "%s: OPP table not found (%d)\n", __func__, r);
-		return ERR_PTR(r);
-	}
+	opp_key.rate = freq;
+	opp_key.peak_bw = 0;
+	opp_key.level = 0;
 
-	mutex_lock(&opp_table->lock);
+	return dev_pm_opp_find_opp_exact(dev, &opp_key, available);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_exact);
 
-	list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
-		if (temp_opp->available == available &&
-				temp_opp->rate == freq) {
-			opp = temp_opp;
+/**
+ * dev_pm_opp_find_peak_bw_exact() - search for an exact peak bandwidth
+ * @dev:		device for which we do this operation
+ * @peak_bw:		peak bandwidth to search for
+ * @available:		true/false - match for available opp
+ *
+ * Return: Searches for exact match in the opp table and returns pointer to the
+ * matching opp if found, else returns ERR_PTR in case of error and should
+ * be handled using IS_ERR. Error return values can be:
+ * EINVAL:	for bad pointer
+ * ERANGE:	no match found for search
+ * ENODEV:	if device not found in list of registered devices
+ *
+ * Note: available is a modifier for the search. if available=true, then the
+ * match is for exact matching peak bandwidth and is available in the stored
+ * OPP table. if false, the match is for exact peak bandwidth which is not
+ * available.
+ *
+ * This provides a mechanism to enable an opp which is not available currently
+ * or the opposite as well.
+ *
+ * The callers are required to call dev_pm_opp_put() for the returned OPP after
+ * use.
+ */
+struct dev_pm_opp *dev_pm_opp_find_peak_bw_exact(struct device *dev,
+						 unsigned int peak_bw,
+						 bool available)
+{
+	struct dev_pm_opp opp_key;
 
-			/* Increment the reference count of OPP */
-			dev_pm_opp_get(opp);
-			break;
-		}
-	}
+	opp_key.rate = 0;
+	opp_key.peak_bw = peak_bw;
+	opp_key.level = 0;
 
-	mutex_unlock(&opp_table->lock);
-	dev_pm_opp_put_opp_table(opp_table);
-
-	return opp;
+	return dev_pm_opp_find_opp_exact(dev, &opp_key, available);
 }
-EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_exact);
+EXPORT_SYMBOL_GPL(dev_pm_opp_find_peak_bw_exact);
 
 /**
  * dev_pm_opp_find_level_exact() - search for an exact level
@@ -449,18 +551,17 @@ struct dev_pm_opp *dev_pm_opp_find_level_exact(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_level_exact);
 
-static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,
-						   unsigned long *freq)
+static struct dev_pm_opp *_find_opp_ceil(struct opp_table *opp_table,
+					 struct dev_pm_opp *opp_key)
 {
 	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
 
 	mutex_lock(&opp_table->lock);
 
 	list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
-		if (temp_opp->available && temp_opp->rate >= *freq) {
+		if (temp_opp->available &&
+		    opp_compare_key(temp_opp, opp_key) >= 0) {
 			opp = temp_opp;
-			*freq = opp->rate;
-
 			/* Increment the reference count of OPP */
 			dev_pm_opp_get(opp);
 			break;
@@ -472,6 +573,23 @@ static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,
 	return opp;
 }
 
+static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,
+						   unsigned long *freq)
+{
+	struct dev_pm_opp opp_key, *opp;
+
+	opp_key.rate = *freq;
+	opp_key.peak_bw = 0;
+	opp_key.level = 0;
+
+	opp = _find_opp_ceil(opp_table, &opp_key);
+
+	if (!IS_ERR(opp))
+		*freq = opp->rate;
+
+	return opp;
+}
+
 /**
  * dev_pm_opp_find_freq_ceil() - Search for an rounded ceil freq
  * @dev:	device for which we do this operation
@@ -514,14 +632,14 @@ struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil);
 
 /**
- * dev_pm_opp_find_freq_floor() - Search for a rounded floor freq
+ * dev_pm_opp_find_peak_bw_ceil() - Search for an rounded ceil peak bandwidth
  * @dev:	device for which we do this operation
- * @freq:	Start frequency
+ * @peak_bw:	Start peak bandwidth
  *
- * Search for the matching floor *available* OPP from a starting freq
+ * Search for the matching ceil *available* OPP from a starting peak bandwidth
  * for a device.
  *
- * Return: matching *opp and refreshes *freq accordingly, else returns
+ * Return: matching *opp and refreshes *peak_bw accordingly, else returns
  * ERR_PTR in case of error and should be handled using IS_ERR. Error return
  * values can be:
  * EINVAL:	for bad pointer
@@ -531,17 +649,43 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil);
  * The callers are required to call dev_pm_opp_put() for the returned OPP after
  * use.
  */
-struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
-					      unsigned long *freq)
+struct dev_pm_opp *dev_pm_opp_find_peak_bw_ceil(struct device *dev,
+						unsigned long *peak_bw)
 {
 	struct opp_table *opp_table;
-	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
+	struct dev_pm_opp *opp;
+	struct dev_pm_opp opp_key;
 
-	if (!dev || !freq) {
-		dev_err(dev, "%s: Invalid argument freq=%p\n", __func__, freq);
+	if (!dev || !peak_bw) {
+		dev_err(dev, "%s: Invalid argument peak_bw=%p\n", __func__,
+			peak_bw);
 		return ERR_PTR(-EINVAL);
 	}
 
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return ERR_CAST(opp_table);
+
+	opp_key.rate = 0;
+	opp_key.peak_bw = *peak_bw;
+	opp_key.level = 0;
+	opp = _find_opp_ceil(opp_table, &opp_key);
+
+	if (!IS_ERR(opp))
+		*peak_bw = opp->rate;
+
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return opp;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_find_peak_bw_ceil);
+
+struct dev_pm_opp *dev_pm_opp_find_opp_floor(struct device *dev,
+					     struct dev_pm_opp *opp_key)
+{
+	struct opp_table *opp_table;
+	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
+
 	opp_table = _find_opp_table(dev);
 	if (IS_ERR(opp_table))
 		return ERR_CAST(opp_table);
@@ -551,7 +695,7 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 	list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
 		if (temp_opp->available) {
 			/* go to the next node, before choosing prev */
-			if (temp_opp->rate > *freq)
+			if (opp_compare_key(temp_opp, opp_key) > 0)
 				break;
 			else
 				opp = temp_opp;
@@ -564,6 +708,43 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 	mutex_unlock(&opp_table->lock);
 	dev_pm_opp_put_opp_table(opp_table);
 
+	return opp;
+}
+
+/**
+ * dev_pm_opp_find_freq_floor() - Search for a rounded floor freq
+ * @dev:	device for which we do this operation
+ * @freq:	Start frequency
+ *
+ * Search for the matching floor *available* OPP from a starting freq
+ * for a device.
+ *
+ * Return: matching *opp and refreshes *freq accordingly, else returns
+ * ERR_PTR in case of error and should be handled using IS_ERR. Error return
+ * values can be:
+ * EINVAL:	for bad pointer
+ * ERANGE:	no match found for search
+ * ENODEV:	if device not found in list of registered devices
+ *
+ * The callers are required to call dev_pm_opp_put() for the returned OPP after
+ * use.
+ */
+struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
+					      unsigned long *freq)
+{
+	struct dev_pm_opp opp_key, *opp;
+
+	if (!dev || !freq) {
+		dev_err(dev, "%s: Invalid argument freq=%p\n", __func__, freq);
+		return ERR_PTR(-EINVAL);
+	}
+
+	opp_key.rate = *freq;
+	opp_key.peak_bw = 0;
+	opp_key.level = 0;
+
+	opp = dev_pm_opp_find_opp_floor(dev, &opp_key);
+
 	if (!IS_ERR(opp))
 		*freq = opp->rate;
 
@@ -571,6 +752,48 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
+/**
+ * dev_pm_opp_find_peak_bw_floor() - Search for a rounded floor peak bandwidth
+ * @dev:	device for which we do this operation
+ * @peak_bw:	Start peak bandwidth
+ *
+ * Search for the matching floor *available* OPP from a starting peak bandwidth
+ * for a device.
+ *
+ * Return: matching *opp and refreshes *peak_bw accordingly, else returns
+ * ERR_PTR in case of error and should be handled using IS_ERR. Error return
+ * values can be:
+ * EINVAL:	for bad pointer
+ * ERANGE:	no match found for search
+ * ENODEV:	if device not found in list of registered devices
+ *
+ * The callers are required to call dev_pm_opp_put() for the returned OPP after
+ * use.
+ */
+struct dev_pm_opp *dev_pm_opp_find_peak_bw_floor(struct device *dev,
+						 unsigned int *peak_bw)
+{
+	struct dev_pm_opp opp_key, *opp;
+
+	if (!dev || !peak_bw) {
+		dev_err(dev, "%s: Invalid argument peak_bw=%p\n", __func__,
+			peak_bw);
+		return ERR_PTR(-EINVAL);
+	}
+
+	opp_key.rate = 0;
+	opp_key.peak_bw = *peak_bw;
+	opp_key.level = 0;
+
+	opp = dev_pm_opp_find_opp_floor(dev, &opp_key);
+
+	if (!IS_ERR(opp))
+		*peak_bw = opp->peak_bw;
+
+	return opp;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_find_peak_bw_floor);
+
 /**
  * dev_pm_opp_find_freq_ceil_by_volt() - Find OPP with highest frequency for
  *					 target voltage.
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 747861816f4f..aab1b9c143ac 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -83,6 +83,7 @@ void dev_pm_opp_put_opp_table(struct opp_table *opp_table);
 unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
 
 unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp);
+unsigned long dev_pm_opp_get_bw(struct dev_pm_opp *opp, unsigned long *avg_bw);
 
 unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp);
 
@@ -93,20 +94,29 @@ unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev);
 unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev);
 unsigned long dev_pm_opp_get_max_transition_latency(struct device *dev);
 unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev);
+unsigned long dev_pm_opp_get_suspend_opp_bw(struct device *dev,
+					    unsigned long *avg_bw);
 
 struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 					      unsigned long freq,
 					      bool available);
+struct dev_pm_opp *dev_pm_opp_find_peak_bw_exact(struct device *dev,
+						 unsigned int peak_bw,
+						 bool available);
 struct dev_pm_opp *dev_pm_opp_find_level_exact(struct device *dev,
 					       unsigned int level);
 
 struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 					      unsigned long *freq);
+struct dev_pm_opp *dev_pm_opp_find_peak_bw_floor(struct device *dev,
+						 unsigned int *peak_bw);
 struct dev_pm_opp *dev_pm_opp_find_freq_ceil_by_volt(struct device *dev,
 						     unsigned long u_volt);
 
 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 					     unsigned long *freq);
+struct dev_pm_opp *dev_pm_opp_find_peak_bw_ceil(struct device *dev,
+						unsigned long *peak_bw);
 void dev_pm_opp_put(struct dev_pm_opp *opp);
 
 int dev_pm_opp_add(struct device *dev, unsigned long freq,
@@ -165,6 +175,11 @@ static inline unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
 {
 	return 0;
 }
+static inline unsigned long dev_pm_opp_get_bw(struct dev_pm_opp *opp,
+					      unsigned long *avg_bw)
+{
+	return 0;
+}
 
 static inline unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp)
 {
@@ -201,12 +216,26 @@ static inline unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev)
 	return 0;
 }
 
+static inline unsigned long dev_pm_opp_get_suspend_opp_bw(struct device *dev,
+							  unsigned long *avg_bw)
+{
+	return 0;
+}
+
 static inline struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 					unsigned long freq, bool available)
 {
 	return ERR_PTR(-ENOTSUPP);
 }
 
+static inline struct dev_pm_opp *dev_pm_opp_find_peak_bw_exact(
+							struct device *dev,
+							unsigned int peak_bw,
+							bool available)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
 static inline struct dev_pm_opp *dev_pm_opp_find_level_exact(struct device *dev,
 					unsigned int level)
 {
@@ -219,6 +248,13 @@ static inline struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 	return ERR_PTR(-ENOTSUPP);
 }
 
+static inline struct dev_pm_opp *dev_pm_opp_find_peak_bw_floor(
+							struct device *dev,
+							unsigned int *peak_bw)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
 static inline struct dev_pm_opp *dev_pm_opp_find_freq_ceil_by_volt(struct device *dev,
 					unsigned long u_volt)
 {
@@ -231,6 +267,13 @@ static inline struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 	return ERR_PTR(-ENOTSUPP);
 }
 
+static inline struct dev_pm_opp *dev_pm_opp_find_peak_bw_ceil(
+							struct device *dev,
+							unsigned long *peak_bw)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
 static inline void dev_pm_opp_put(struct dev_pm_opp *opp) {}
 
 static inline int dev_pm_opp_add(struct device *dev, unsigned long freq,
-- 
2.24.0.393.g34dc348eaf-goog

Re: [PATCH v6 2/3] OPP: Add support for bandwidth OPP tables

[Sibi Sankar]

Hey Saravana,

Spent some time testing this series while
trying out dcvs on SDM845/SC7180. Apart from
the few minor issues it works quite well!

On 2019-12-07 05:54, Saravana Kannan wrote:
> Not all devices quantify their performance points in terms of 
> frequency.
> Devices like interconnects quantify their performance points in terms 
> of
> bandwidth. We need a way to represent these bandwidth levels in OPP. 
> So,
> add support for parsing bandwidth OPPs from DT.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/opp/core.c | 15 +++++++++--
>  drivers/opp/of.c   | 63 ++++++++++++++++++++++++++++++++--------------
>  drivers/opp/opp.h  |  5 ++++
>  3 files changed, 62 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index be7a7d332332..c79bbfac7289 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1282,11 +1282,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->peak_bw != opp2->peak_bw)
> +		return opp1->peak_bw < opp2->peak_bw ? -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
> @@ -1297,12 +1307,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 1cbb58240b80..b565da5a2b1f 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -521,6 +521,44 @@ 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)
> +{
> +	int ret;
> +	u64 rate;
> +	u32 bw;
> +
> +	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;
> +		goto out;
> +	}
> +
> +	ret = of_property_read_u32(np, "opp-peak-kBps", &bw);
> +	if (!ret) {
> +		new_opp->peak_bw = bw;
> +
> +		if (!of_property_read_u32(np, "opp-avg-kBps", &bw))
> +			new_opp->avg_bw = bw;
> +	}
> +
> +out:
> +	*rate_not_available = !!ret;
> +	/*
> +	 * If ret is 0 at this point, we have already found a key. If we
> +	 * haven't found a key yet, then ret already has an error value. In
> +	 * either case, we don't need to update 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 +596,12 @@ 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);
> +	if (ret) {

if (!opp_table->is_genpd) {

_read_opp_key returns an error for genpd
opps so please check if it is a genpd
opp_table before erroring out here.

> +		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);
> @@ -616,7 +640,8 @@ static struct dev_pm_opp
> *_opp_add_static_v2(struct opp_table *opp_table,
>  	if (of_property_read_bool(np, "opp-suspend")) {
>  		if (opp_table->suspend_opp) {
>  			/* Pick the OPP with higher rate as suspend OPP */
> -			if (new_opp->rate > opp_table->suspend_opp->rate) {
> +			if (opp_compare_key(new_opp,
> +					    opp_table->suspend_opp) > 1) {

shouldn't the condition be > 0?

>  				opp_table->suspend_opp->suspend = false;
>  				new_opp->suspend = true;
>  				opp_table->suspend_opp = new_opp;
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 01a500e2c40a..0def3154d07b 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -57,6 +57,8 @@ extern struct list_head opp_tables;
>   * @suspend:	true if suspend OPP
>   * @pstate: Device's power domain's performance state.
>   * @rate:	Frequency in hertz
> + * @peak_bw:	Peak bandwidth in kilobytes per second
> + * @avg_bw:	Average bandwidth in kilobytes per second
>   * @level:	Performance level
>   * @supplies:	Power supplies voltage/current values
>   * @clock_latency_ns: Latency (in nanoseconds) of switching to this 
> OPP's
> @@ -78,6 +80,8 @@ struct dev_pm_opp {
>  	bool suspend;
>  	unsigned int pstate;
>  	unsigned long rate;
> +	unsigned int peak_bw;
> +	unsigned int avg_bw;
>  	unsigned int level;
> 
>  	struct dev_pm_opp_supply *supplies;
> @@ -213,6 +217,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);

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

Re: [PATCH v6 2/3] OPP: Add support for bandwidth OPP tables

[Saravana Kannan]

On Tue, Jan 7, 2020 at 11:28 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Saravana,
>
> Spent some time testing this series while
> trying out dcvs on SDM845/SC7180. Apart from
> the few minor issues it works quite well!

Thanks a lot for testing Sibi. Can you give a tested-by? Glad to hear
it works well.

> On 2019-12-07 05:54, Saravana Kannan wrote:
> > Not all devices quantify their performance points in terms of
> > frequency.
> > Devices like interconnects quantify their performance points in terms
> > of
> > bandwidth. We need a way to represent these bandwidth levels in OPP.
> > So,
> > add support for parsing bandwidth OPPs from DT.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/opp/core.c | 15 +++++++++--
> >  drivers/opp/of.c   | 63 ++++++++++++++++++++++++++++++++--------------
> >  drivers/opp/opp.h  |  5 ++++
> >  3 files changed, 62 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index be7a7d332332..c79bbfac7289 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1282,11 +1282,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->peak_bw != opp2->peak_bw)
> > +             return opp1->peak_bw < opp2->peak_bw ? -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
> > @@ -1297,12 +1307,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 1cbb58240b80..b565da5a2b1f 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -521,6 +521,44 @@ 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)
> > +{
> > +     int ret;
> > +     u64 rate;
> > +     u32 bw;
> > +
> > +     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;
> > +             goto out;
> > +     }
> > +
> > +     ret = of_property_read_u32(np, "opp-peak-kBps", &bw);
> > +     if (!ret) {
> > +             new_opp->peak_bw = bw;
> > +
> > +             if (!of_property_read_u32(np, "opp-avg-kBps", &bw))
> > +                     new_opp->avg_bw = bw;
> > +     }
> > +
> > +out:
> > +     *rate_not_available = !!ret;
> > +     /*
> > +      * If ret is 0 at this point, we have already found a key. If we
> > +      * haven't found a key yet, then ret already has an error value. In
> > +      * either case, we don't need to update 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 +596,12 @@ 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);
> > +     if (ret) {
>
> if (!opp_table->is_genpd) {
>
> _read_opp_key returns an error for genpd
> opps so please check if it is a genpd
> opp_table before erroring out here.

Thanks. I'll fix it in the next version.

> > +             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);
> > @@ -616,7 +640,8 @@ static struct dev_pm_opp
> > *_opp_add_static_v2(struct opp_table *opp_table,
> >       if (of_property_read_bool(np, "opp-suspend")) {
> >               if (opp_table->suspend_opp) {
> >                       /* Pick the OPP with higher rate as suspend OPP */
> > -                     if (new_opp->rate > opp_table->suspend_opp->rate) {
> > +                     if (opp_compare_key(new_opp,
> > +                                         opp_table->suspend_opp) > 1) {
>
> shouldn't the condition be > 0?

Duh. Thanks. I'll fix it in the next version.

I'm guessing you tested with the fixes you pointed out?

-Saravana

Re: [PATCH v6 1/3] dt-bindings: opp: Introduce opp-peak-kBps and opp-avg-kBps bindings

[Viresh Kumar]

On 06-12-19, 16:24, Saravana Kannan wrote:
> 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: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/opp/opp.txt     | 15 ++++++++++++---
>  .../devicetree/bindings/property-units.txt        |  4 ++++
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index 68592271461f..dbad8eb6c746 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 except for devices like power domains
> +  or bandwidth opp tables.

Fine until here.

> The power domain nodes must have another
> +  (implementation dependent) property which uniquely identifies the OPP nodes.
> +  The interconnect opps are required to have the opp-peak-kBps property.

Maybe rewrite it as:

The devices which don't have this property must have another
(implementation dependent) property which uniquely identifies the OPP
nodes.

So we won't be required to update this again for another property.

> +
> +- opp-peak-kBps: Peak bandwidth in kilobytes per second, expressed as a 32-bit
> +  big-endian integer.

> This is a required property for all devices that don't
> +  have opp-hz.

This statement is surely incorrect, isn't it ? What about power-domain
tables ?

Suggest rewriting it as:

This is a required property for bandwidth OPP tables.

> For example, bandwidth OPP tables for interconnect paths.
>  
>  Optional properties:
>  - opp-microvolt: voltage in micro Volts.
> @@ -132,6 +137,10 @@ 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 a
> +  32-bit big-endian integer. 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
> -- 
> 2.24.0.393.g34dc348eaf-goog

-- 
viresh

Re: [PATCH v6 2/3] OPP: Add support for bandwidth OPP tables

[Viresh Kumar]

On 06-12-19, 16:24, Saravana Kannan wrote:
> Not all devices quantify their performance points in terms of frequency.
> Devices like interconnects quantify their performance points in terms of
> bandwidth. We need a way to represent these bandwidth levels in OPP. So,
> add support for parsing bandwidth OPPs from DT.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/opp/core.c | 15 +++++++++--
>  drivers/opp/of.c   | 63 ++++++++++++++++++++++++++++++++--------------
>  drivers/opp/opp.h  |  5 ++++
>  3 files changed, 62 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index be7a7d332332..c79bbfac7289 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1282,11 +1282,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->peak_bw != opp2->peak_bw)
> +		return opp1->peak_bw < opp2->peak_bw ? -1 : 1;

Please also add level here.

> +	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
> @@ -1297,12 +1307,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 1cbb58240b80..b565da5a2b1f 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -521,6 +521,44 @@ 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)
> +{
> +	int ret;
> +	u64 rate;
> +	u32 bw;
> +
> +	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;
> +		goto out;
> +	}
> +
> +	ret = of_property_read_u32(np, "opp-peak-kBps", &bw);
> +	if (!ret) {
> +		new_opp->peak_bw = bw;
> +
> +		if (!of_property_read_u32(np, "opp-avg-kBps", &bw))
> +			new_opp->avg_bw = bw;

Maybe
		of_property_read_u32(np, "opp-avg-kBps", &new_opp->avg_bw);

and same for opp-peak-kbps as well.

> +	}
> +
> +out:
> +	*rate_not_available = !!ret;
> +	/*
> +	 * If ret is 0 at this point, we have already found a key. If we
> +	 * haven't found a key yet, then ret already has an error value. In
> +	 * either case, we don't need to update ret.
> +	 */
> +	of_property_read_u32(np, "opp-level", &new_opp->level);

Yes, it wasn't done earlier but we should do it now. Check level as
well and treat it as any other key.

I think add a preparatory patch first which does all the cleanup
before bandwidth thing is added.

> +
> +	return ret;
> +}
> +
>  /**
>   * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
>   * @opp_table:	OPP table
> @@ -558,26 +596,12 @@ 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);
> +	if (ret) {
> +		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);
> @@ -616,7 +640,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
>  	if (of_property_read_bool(np, "opp-suspend")) {
>  		if (opp_table->suspend_opp) {
>  			/* Pick the OPP with higher rate as suspend OPP */
> -			if (new_opp->rate > opp_table->suspend_opp->rate) {
> +			if (opp_compare_key(new_opp,
> +					    opp_table->suspend_opp) > 1) {

Maybe leave this place as is as we never want to compare anything else
but rate.

>  				opp_table->suspend_opp->suspend = false;
>  				new_opp->suspend = true;
>  				opp_table->suspend_opp = new_opp;
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 01a500e2c40a..0def3154d07b 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -57,6 +57,8 @@ extern struct list_head opp_tables;
>   * @suspend:	true if suspend OPP
>   * @pstate: Device's power domain's performance state.
>   * @rate:	Frequency in hertz
> + * @peak_bw:	Peak bandwidth in kilobytes per second
> + * @avg_bw:	Average bandwidth in kilobytes per second
>   * @level:	Performance level
>   * @supplies:	Power supplies voltage/current values
>   * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
> @@ -78,6 +80,8 @@ struct dev_pm_opp {
>  	bool suspend;
>  	unsigned int pstate;
>  	unsigned long rate;
> +	unsigned int peak_bw;
> +	unsigned int avg_bw;
>  	unsigned int level;
>  
>  	struct dev_pm_opp_supply *supplies;
> @@ -213,6 +217,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);

make it _opp_compare_key() instead.

>  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);
> -- 
> 2.24.0.393.g34dc348eaf-goog

-- 
viresh

Re: [PATCH v6 3/3] OPP: Add helper function for bandwidth OPP tables

[Viresh Kumar]

On 06-12-19, 16:24, Saravana Kannan wrote:
> The frequency OPP tables have helper functions to search for entries in the
> table based on frequency and get the frequency values for a given (or
> suspend) OPP entry.
> 
> Add similar helper functions for bandwidth OPP tables to search for entries
> in the table based on peak bandwidth and to get the peak and average
> bandwidth for a given (or suspend) OPP entry.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/opp/core.c     | 301 +++++++++++++++++++++++++++++++++++------
>  include/linux/pm_opp.h |  43 ++++++
>  2 files changed, 305 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index c79bbfac7289..3ff33a08198e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -127,6 +127,29 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
>  
> +/**
> + * dev_pm_opp_get_bw() - Gets the bandwidth corresponding to an available opp
> + * @opp:	opp for which peak bandwidth has to be returned for

s/peak //

> + * @avg_bw:	Pointer where the corresponding average bandwidth is stored.
> + *		Can be NULL.
> + *
> + * Return: Peak bandwidth in kBps corresponding to the opp, else
> + * return 0
> + */
> +unsigned long dev_pm_opp_get_bw(struct dev_pm_opp *opp, unsigned long *avg_bw)
> +{
> +	if (IS_ERR_OR_NULL(opp) || !opp->available) {
> +		pr_err("%s: Invalid parameters\n", __func__);
> +		return 0;
> +	}
> +
> +	if (avg_bw)

Do you see this being NULL in practice ? If no, then we can make it
mandatory for now ?

> +		*avg_bw = opp->avg_bw;
> +
> +	return opp->peak_bw;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_get_bw);
> +
>  /**
>   * dev_pm_opp_get_level() - Gets the level corresponding to an available opp
>   * @opp:	opp for which level value has to be returned for
> @@ -299,6 +322,34 @@ unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_freq);
>  
> +/**
> + * dev_pm_opp_get_suspend_opp_bw() - Get peak bandwidth of suspend opp in kBps

Hmm, I wasn't expecting this. So the interconnects will also have a
suspend OPP ?

> + * @dev:	device for which we do this operation
> + * @avg_bw:	Pointer where the corresponding average bandwidth is stored.
> + *		Can be NULL.
> + *
> + * Return: This function returns the peak bandwidth of the OPP marked as
> + * suspend_opp if one is available, else returns 0;
> + */
> +unsigned long dev_pm_opp_get_suspend_opp_bw(struct device *dev,
> +					    unsigned long *avg_bw)
> +{
> +	struct opp_table *opp_table;
> +	unsigned long peak_bw = 0;
> +
> +	opp_table = _find_opp_table(dev);
> +	if (IS_ERR(opp_table))
> +		return 0;
> +
> +	if (opp_table->suspend_opp && opp_table->suspend_opp->available)
> +		peak_bw = dev_pm_opp_get_bw(opp_table->suspend_opp, avg_bw);
> +
> +	dev_pm_opp_put_opp_table(opp_table);
> +
> +	return peak_bw;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_bw);
> +
>  int _get_opp_count(struct opp_table *opp_table)
>  {
>  	struct dev_pm_opp *opp;
> @@ -343,6 +394,40 @@ int dev_pm_opp_get_opp_count(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_count);
>  

I think we should add function header here instead of the helpers
which get exact match for freq, bw or level. And then pass a enum
value to it, which tells what we are looking to compare. After that
rest of the routines will be just one liners, make them macros in
header file itself.

> +struct dev_pm_opp *dev_pm_opp_find_opp_exact(struct device *dev,
> +					      struct dev_pm_opp *opp_key,
> +					      bool available)

-- 
viresh

Re: [PATCH v6 0/3] Introduce Bandwidth OPPs for interconnects

[Viresh Kumar]

On 06-12-19, 16:24, Saravana Kannan wrote:
> Viresh/Stephen,
> 
> I don't think all the additional code/diff in this v6 series is worth it
> to avoid using the rate field to store peak bandwidth. However, since folks
> weren't too happy about it, here it is. I prefer the v5 series, but not
> too strongly tied to it. Let me know what you think Viresh/Stephen.
> 
> Btw, I wasn't sure of opp-hz = 0

I am not sure either ;)

> or opp-level = 0 were allowed. Also,

I think this is allowed.

> it's not clear why the duplicate check isn't done for opp-level when
> _opp_add() is called. Based on that, we could add opp-level comparison

This should be done. Please do that in the first patch as I suggested
in the code as well.

> to opp_compare_key(). That's why you'll see a few spurious
> opp_key.level = 0 lines. Let me know how you want to go with that.
> 
> I could also add a opp.key_type enum field to store what key type the
> opp entry is. But looks like I can get away without adding an
> unnecessary variable. So, I've skipped that for now.

Not in the OPP struct, but such an enum can be used for helper
routines as I commented.

-- 
viresh

Re: [PATCH v6 1/3] dt-bindings: opp: Introduce opp-peak-kBps and opp-avg-kBps bindings

[Saravana Kannan]

On Wed, Jan 8, 2020 at 2:32 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 06-12-19, 16:24, Saravana Kannan wrote:
> > 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: Rob Herring <robh@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/opp/opp.txt     | 15 ++++++++++++---
> >  .../devicetree/bindings/property-units.txt        |  4 ++++
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> > index 68592271461f..dbad8eb6c746 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 except for devices like power domains
> > +  or bandwidth opp tables.
>
> Fine until here.
>
> > The power domain nodes must have another
> > +  (implementation dependent) property which uniquely identifies the OPP nodes.
> > +  The interconnect opps are required to have the opp-peak-kBps property.
>
> Maybe rewrite it as:
>
> The devices which don't have this property must have another
> (implementation dependent) property which uniquely identifies the OPP
> nodes.
>
> So we won't be required to update this again for another property.
>
> > +
> > +- opp-peak-kBps: Peak bandwidth in kilobytes per second, expressed as a 32-bit
> > +  big-endian integer.
>
> > This is a required property for all devices that don't
> > +  have opp-hz.
>
> This statement is surely incorrect, isn't it ? What about power-domain
> tables ?
>
> Suggest rewriting it as:
>
> This is a required property for bandwidth OPP tables.
>

Agree with all the suggestions. Will fix in the next version.

-Saravana

Re: [PATCH v6 2/3] OPP: Add support for bandwidth OPP tables

[Saravana Kannan]

On Wed, Jan 8, 2020 at 2:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 06-12-19, 16:24, Saravana Kannan wrote:
> > Not all devices quantify their performance points in terms of frequency.
> > Devices like interconnects quantify their performance points in terms of
> > bandwidth. We need a way to represent these bandwidth levels in OPP. So,
> > add support for parsing bandwidth OPPs from DT.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/opp/core.c | 15 +++++++++--
> >  drivers/opp/of.c   | 63 ++++++++++++++++++++++++++++++++--------------
> >  drivers/opp/opp.h  |  5 ++++
> >  3 files changed, 62 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index be7a7d332332..c79bbfac7289 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1282,11 +1282,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->peak_bw != opp2->peak_bw)
> > +             return opp1->peak_bw < opp2->peak_bw ? -1 : 1;
>
> Please also add level here.

I can, but I vaguely remember finding opp-levels could have
duplicates? Am I wrong? If so I can add the opp-level comparison too.

> > +     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
> > @@ -1297,12 +1307,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 1cbb58240b80..b565da5a2b1f 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -521,6 +521,44 @@ 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)
> > +{
> > +     int ret;
> > +     u64 rate;
> > +     u32 bw;
> > +
> > +     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;
> > +             goto out;
> > +     }
> > +
> > +     ret = of_property_read_u32(np, "opp-peak-kBps", &bw);
> > +     if (!ret) {
> > +             new_opp->peak_bw = bw;
> > +
> > +             if (!of_property_read_u32(np, "opp-avg-kBps", &bw))
> > +                     new_opp->avg_bw = bw;
>
> Maybe
>                 of_property_read_u32(np, "opp-avg-kBps", &new_opp->avg_bw);
>
> and same for opp-peak-kbps as well.

But those are not u32. Is it always safe to directly read into it
across all endian-ness and unsigned int sizes? I get tripped up by
that occasionally.

> > +     }
> > +
> > +out:
> > +     *rate_not_available = !!ret;
> > +     /*
> > +      * If ret is 0 at this point, we have already found a key. If we
> > +      * haven't found a key yet, then ret already has an error value. In
> > +      * either case, we don't need to update ret.
> > +      */
> > +     of_property_read_u32(np, "opp-level", &new_opp->level);
>
> Yes, it wasn't done earlier but we should do it now. Check level as
> well and treat it as any other key.
>
> I think add a preparatory patch first which does all the cleanup
> before bandwidth thing is added.

Ah come on man! You are making this too painful. It's okay to add a
few more error checks as part of implementing a new feature. Please
don't make me add more patches before this.

> > +
> > +     return ret;
> > +}
> > +
> >  /**
> >   * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> >   * @opp_table:       OPP table
> > @@ -558,26 +596,12 @@ 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);
> > +     if (ret) {
> > +             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);
> > @@ -616,7 +640,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> >       if (of_property_read_bool(np, "opp-suspend")) {
> >               if (opp_table->suspend_opp) {
> >                       /* Pick the OPP with higher rate as suspend OPP */
> > -                     if (new_opp->rate > opp_table->suspend_opp->rate) {
> > +                     if (opp_compare_key(new_opp,
> > +                                         opp_table->suspend_opp) > 1) {
>
> Maybe leave this place as is as we never want to compare anything else
> but rate.

We do want to support suspend bandwidth. So I think I should have this
fix here so it works in general. Also, why not suspend opp-level?

> >                               opp_table->suspend_opp->suspend = false;
> >                               new_opp->suspend = true;
> >                               opp_table->suspend_opp = new_opp;
> > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> > index 01a500e2c40a..0def3154d07b 100644
> > --- a/drivers/opp/opp.h
> > +++ b/drivers/opp/opp.h
> > @@ -57,6 +57,8 @@ extern struct list_head opp_tables;
> >   * @suspend: true if suspend OPP
> >   * @pstate: Device's power domain's performance state.
> >   * @rate:    Frequency in hertz
> > + * @peak_bw: Peak bandwidth in kilobytes per second
> > + * @avg_bw:  Average bandwidth in kilobytes per second
> >   * @level:   Performance level
> >   * @supplies:        Power supplies voltage/current values
> >   * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
> > @@ -78,6 +80,8 @@ struct dev_pm_opp {
> >       bool suspend;
> >       unsigned int pstate;
> >       unsigned long rate;
> > +     unsigned int peak_bw;
> > +     unsigned int avg_bw;
> >       unsigned int level;
> >
> >       struct dev_pm_opp_supply *supplies;
> > @@ -213,6 +217,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);
>
> make it _opp_compare_key() instead.

Ack.

-Saravana

Re: [PATCH v6 3/3] OPP: Add helper function for bandwidth OPP tables

[Saravana Kannan]

On Wed, Jan 8, 2020 at 3:19 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 06-12-19, 16:24, Saravana Kannan wrote:
> > The frequency OPP tables have helper functions to search for entries in the
> > table based on frequency and get the frequency values for a given (or
> > suspend) OPP entry.
> >
> > Add similar helper functions for bandwidth OPP tables to search for entries
> > in the table based on peak bandwidth and to get the peak and average
> > bandwidth for a given (or suspend) OPP entry.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/opp/core.c     | 301 +++++++++++++++++++++++++++++++++++------
> >  include/linux/pm_opp.h |  43 ++++++
> >  2 files changed, 305 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index c79bbfac7289..3ff33a08198e 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -127,6 +127,29 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
> >
> > +/**
> > + * dev_pm_opp_get_bw() - Gets the bandwidth corresponding to an available opp
> > + * @opp:     opp for which peak bandwidth has to be returned for
>
> s/peak //

Ack

>
> > + * @avg_bw:  Pointer where the corresponding average bandwidth is stored.
> > + *           Can be NULL.
> > + *
> > + * Return: Peak bandwidth in kBps corresponding to the opp, else
> > + * return 0
> > + */
> > +unsigned long dev_pm_opp_get_bw(struct dev_pm_opp *opp, unsigned long *avg_bw)
> > +{
> > +     if (IS_ERR_OR_NULL(opp) || !opp->available) {
> > +             pr_err("%s: Invalid parameters\n", __func__);
> > +             return 0;
> > +     }
> > +
> > +     if (avg_bw)
>
> Do you see this being NULL in practice ? If no, then we can make it
> mandatory for now ?

Yes, very likely. A lot of OPP tables might not have avg bandwidth listed.

> > +             *avg_bw = opp->avg_bw;
> > +
> > +     return opp->peak_bw;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_pm_opp_get_bw);
> > +
> >  /**
> >   * dev_pm_opp_get_level() - Gets the level corresponding to an available opp
> >   * @opp:     opp for which level value has to be returned for
> > @@ -299,6 +322,34 @@ unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_freq);
> >
> > +/**
> > + * dev_pm_opp_get_suspend_opp_bw() - Get peak bandwidth of suspend opp in kBps
>
> Hmm, I wasn't expecting this. So the interconnects will also have a
> suspend OPP ?

Yes, device voting for interconnect paths might want to lower the
bandwidth to a suspend bandwidth when they suspend.

> > + * @dev:     device for which we do this operation
> > + * @avg_bw:  Pointer where the corresponding average bandwidth is stored.
> > + *           Can be NULL.
> > + *
> > + * Return: This function returns the peak bandwidth of the OPP marked as
> > + * suspend_opp if one is available, else returns 0;
> > + */
> > +unsigned long dev_pm_opp_get_suspend_opp_bw(struct device *dev,
> > +                                         unsigned long *avg_bw)
> > +{
> > +     struct opp_table *opp_table;
> > +     unsigned long peak_bw = 0;
> > +
> > +     opp_table = _find_opp_table(dev);
> > +     if (IS_ERR(opp_table))
> > +             return 0;
> > +
> > +     if (opp_table->suspend_opp && opp_table->suspend_opp->available)
> > +             peak_bw = dev_pm_opp_get_bw(opp_table->suspend_opp, avg_bw);
> > +
> > +     dev_pm_opp_put_opp_table(opp_table);
> > +
> > +     return peak_bw;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_bw);
> > +
> >  int _get_opp_count(struct opp_table *opp_table)
> >  {
> >       struct dev_pm_opp *opp;
> > @@ -343,6 +394,40 @@ int dev_pm_opp_get_opp_count(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_count);
> >
>
> I think we should add function header here instead of the helpers
> which get exact match for freq, bw or level. And then pass a enum
> value to it, which tells what we are looking to compare. After that
> rest of the routines will be just one liners, make them macros in
> header file itself.

Not sure I understand what you are saying here.


> > +struct dev_pm_opp *dev_pm_opp_find_opp_exact(struct device *dev,
> > +                                           struct dev_pm_opp *opp_key,
> > +                                           bool available)
>

-Saravana

Re: [PATCH v6 3/3] OPP: Add helper function for bandwidth OPP tables

[Saravana Kannan]

On Wed, Jan 8, 2020 at 4:58 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Jan 8, 2020 at 3:19 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 06-12-19, 16:24, Saravana Kannan wrote:
> > > The frequency OPP tables have helper functions to search for entries in the
> > > table based on frequency and get the frequency values for a given (or
> > > suspend) OPP entry.
> > >
> > > Add similar helper functions for bandwidth OPP tables to search for entries
> > > in the table based on peak bandwidth and to get the peak and average
> > > bandwidth for a given (or suspend) OPP entry.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/opp/core.c     | 301 +++++++++++++++++++++++++++++++++++------
> > >  include/linux/pm_opp.h |  43 ++++++
> > >  2 files changed, 305 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > > index c79bbfac7289..3ff33a08198e 100644
> > > --- a/drivers/opp/core.c
> > > +++ b/drivers/opp/core.c
> > > @@ -127,6 +127,29 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
> > >  }
> > >  EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
> > >
> > > +/**
> > > + * dev_pm_opp_get_bw() - Gets the bandwidth corresponding to an available opp
> > > + * @opp:     opp for which peak bandwidth has to be returned for
> >
> > s/peak //
>
> Ack

Actually, isn't this correct as is? peak bandwidth is "returned".
Average bandwidth is updated through the pointer.

-Saravana

Re: [PATCH v6 2/3] OPP: Add support for bandwidth OPP tables

[Viresh Kumar]

On 08-01-20, 16:58, Saravana Kannan wrote:
> On Wed, Jan 8, 2020 at 2:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 06-12-19, 16:24, Saravana Kannan wrote:
> > > Not all devices quantify their performance points in terms of frequency.
> > > Devices like interconnects quantify their performance points in terms of
> > > bandwidth. We need a way to represent these bandwidth levels in OPP. So,
> > > add support for parsing bandwidth OPPs from DT.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/opp/core.c | 15 +++++++++--
> > >  drivers/opp/of.c   | 63 ++++++++++++++++++++++++++++++++--------------
> > >  drivers/opp/opp.h  |  5 ++++
> > >  3 files changed, 62 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > > index be7a7d332332..c79bbfac7289 100644
> > > --- a/drivers/opp/core.c
> > > +++ b/drivers/opp/core.c
> > > @@ -1282,11 +1282,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->peak_bw != opp2->peak_bw)
> > > +             return opp1->peak_bw < opp2->peak_bw ? -1 : 1;
> >
> > Please also add level here.
> 
> I can, but I vaguely remember finding opp-levels could have
> duplicates? Am I wrong? If so I can add the opp-level comparison too.

No they can't have duplicates.

> > > +     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
> > > @@ -1297,12 +1307,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 1cbb58240b80..b565da5a2b1f 100644
> > > --- a/drivers/opp/of.c
> > > +++ b/drivers/opp/of.c
> > > @@ -521,6 +521,44 @@ 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)
> > > +{
> > > +     int ret;
> > > +     u64 rate;
> > > +     u32 bw;
> > > +
> > > +     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;
> > > +             goto out;
> > > +     }
> > > +
> > > +     ret = of_property_read_u32(np, "opp-peak-kBps", &bw);
> > > +     if (!ret) {
> > > +             new_opp->peak_bw = bw;
> > > +
> > > +             if (!of_property_read_u32(np, "opp-avg-kBps", &bw))
> > > +                     new_opp->avg_bw = bw;
> >
> > Maybe
> >                 of_property_read_u32(np, "opp-avg-kBps", &new_opp->avg_bw);
> >
> > and same for opp-peak-kbps as well.
> 
> But those are not u32. Is it always safe to directly read into it
> across all endian-ness and unsigned int sizes? I get tripped up by
> that occasionally.

It may not be safe.

> > > +     }
> > > +
> > > +out:
> > > +     *rate_not_available = !!ret;
> > > +     /*
> > > +      * If ret is 0 at this point, we have already found a key. If we
> > > +      * haven't found a key yet, then ret already has an error value. In
> > > +      * either case, we don't need to update ret.
> > > +      */
> > > +     of_property_read_u32(np, "opp-level", &new_opp->level);
> >
> > Yes, it wasn't done earlier but we should do it now. Check level as
> > well and treat it as any other key.
> >
> > I think add a preparatory patch first which does all the cleanup
> > before bandwidth thing is added.
> 
> Ah come on man! You are making this too painful. It's okay to add a
> few more error checks as part of implementing a new feature. Please
> don't make me add more patches before this.

It will only make your life easier and not painful in my opinion as
the reviews are getting mixed/confused between the new things and the
old fields right now. With a separate patch introducing just the
bandwidth part, it will get reviewed in maximum 1-2 versions, else you
will keep updating the unrelated patch and I will keep reviewing it as
it is all a single patch.

It is always suggested to break patches into the smallest possible
meaningful separate things you want to achieve. You are introducing
something here and adding cleanup to that.

> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  /**
> > >   * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> > >   * @opp_table:       OPP table
> > > @@ -558,26 +596,12 @@ 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);
> > > +     if (ret) {
> > > +             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);
> > > @@ -616,7 +640,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> > >       if (of_property_read_bool(np, "opp-suspend")) {
> > >               if (opp_table->suspend_opp) {
> > >                       /* Pick the OPP with higher rate as suspend OPP */
> > > -                     if (new_opp->rate > opp_table->suspend_opp->rate) {
> > > +                     if (opp_compare_key(new_opp,
> > > +                                         opp_table->suspend_opp) > 1) {
> >
> > Maybe leave this place as is as we never want to compare anything else
> > but rate.
> 
> We do want to support suspend bandwidth.

Yeah, I understood that from a later patch.

> So I think I should have this
> fix here so it works in general. Also, why not suspend opp-level?

Because we don't want/need to set a specific value to the
voltage-corners during suspend directly from the PM domain driver.
This should happen automatically via a call from the underlying
cpufreq or device driver.

And that's what I expected out of the interconnect thing. For example,
say during suspend you put the interconnect or PM domains to a low
bandwidth/level value, while the underlying device driver (which uses
the interconnect or domain) never requested for it. Who will be
responsible to restore the value during resume as we would be out of
sync here.

-- 
viresh

Re: [PATCH v6 3/3] OPP: Add helper function for bandwidth OPP tables

[Viresh Kumar]

On 08-01-20, 16:58, Saravana Kannan wrote:
> On Wed, Jan 8, 2020 at 3:19 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >  /**
> > >   * dev_pm_opp_get_level() - Gets the level corresponding to an available opp
> > >   * @opp:     opp for which level value has to be returned for
> > > @@ -299,6 +322,34 @@ unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_freq);
> > >
> > > +/**
> > > + * dev_pm_opp_get_suspend_opp_bw() - Get peak bandwidth of suspend opp in kBps
> >
> > Hmm, I wasn't expecting this. So the interconnects will also have a
> > suspend OPP ?
> 
> Yes, device voting for interconnect paths might want to lower the
> bandwidth to a suspend bandwidth when they suspend.

That's exactly what I was saying, the request for a change during
suspend should come from the device and you can't do it here, i.e.
they should lower their frequency requirement, which should lead to a
low bandwidth automatically.

> > > + * @dev:     device for which we do this operation
> > > + * @avg_bw:  Pointer where the corresponding average bandwidth is stored.
> > > + *           Can be NULL.
> > > + *
> > > + * Return: This function returns the peak bandwidth of the OPP marked as
> > > + * suspend_opp if one is available, else returns 0;
> > > + */
> > > +unsigned long dev_pm_opp_get_suspend_opp_bw(struct device *dev,
> > > +                                         unsigned long *avg_bw)
> > > +{
> > > +     struct opp_table *opp_table;
> > > +     unsigned long peak_bw = 0;
> > > +
> > > +     opp_table = _find_opp_table(dev);
> > > +     if (IS_ERR(opp_table))
> > > +             return 0;
> > > +
> > > +     if (opp_table->suspend_opp && opp_table->suspend_opp->available)
> > > +             peak_bw = dev_pm_opp_get_bw(opp_table->suspend_opp, avg_bw);
> > > +
> > > +     dev_pm_opp_put_opp_table(opp_table);
> > > +
> > > +     return peak_bw;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_bw);
> > > +
> > >  int _get_opp_count(struct opp_table *opp_table)
> > >  {
> > >       struct dev_pm_opp *opp;
> > > @@ -343,6 +394,40 @@ int dev_pm_opp_get_opp_count(struct device *dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_count);
> > >
> >
> > I think we should add function header here instead of the helpers
> > which get exact match for freq, bw or level. And then pass a enum
> > value to it, which tells what we are looking to compare. After that
> > rest of the routines will be just one liners, make them macros in
> > header file itself.
> 
> Not sure I understand what you are saying here.

Okay, lemme try again with proper example.

enum opp_key_type {
        OPP_KEY_FREQ = 0x1,
        OPP_KEY_LEVEL= 0x2,
        OPP_KEY_BW   = 0x4,
        OPP_KEY_ALL  = 0x7,
}

/**
 * Add function header here..
 */
struct dev_pm_opp *dev_pm_opp_find_opp_exact(struct device *dev,
                                             enum opp_key_type key,
                                             unsigned long key_value,
                                             bool available)
{
       struct opp_table *opp_table;
       struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);

       opp_table = _find_opp_table(dev);
       if (IS_ERR(opp_table)) {
               int r = PTR_ERR(opp_table);

               dev_err(dev, "%s: OPP table not found (%d)\n", __func__, r);
               return ERR_PTR(r);
       }

       mutex_lock(&opp_table->lock);

       list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
               if (temp_opp->available == available &&
                   !opp_compare_key(temp_opp, key, key_value)) {
                       opp = temp_opp;

                       /* Increment the reference count of OPP */
                       dev_pm_opp_get(opp);
                       break;
               }
       }

       mutex_unlock(&opp_table->lock);
       dev_pm_opp_put_opp_table(opp_table);

       return opp;
}

//Now in header file

#define dev_pm_opp_find_freq_exact(dev, freq, available)        dev_pm_opp_find_opp_exact(dev, OPP_KEY_FREQ, freq, available);
#define dev_pm_opp_find_level_exact(dev, level, available)      dev_pm_opp_find_opp_exact(dev, OPP_KEY_LEVEL, level, available);
#define dev_pm_opp_find_bw_exact(dev, bw, available)            dev_pm_opp_find_opp_exact(dev, OPP_KEY_BW, bw, available);

-- 
viresh

Re: [PATCH v6 3/3] OPP: Add helper function for bandwidth OPP tables

[Viresh Kumar]

On 08-01-20, 19:36, Saravana Kannan wrote:
> On Wed, Jan 8, 2020 at 4:58 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Wed, Jan 8, 2020 at 3:19 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 06-12-19, 16:24, Saravana Kannan wrote:
> > > > The frequency OPP tables have helper functions to search for entries in the
> > > > table based on frequency and get the frequency values for a given (or
> > > > suspend) OPP entry.
> > > >
> > > > Add similar helper functions for bandwidth OPP tables to search for entries
> > > > in the table based on peak bandwidth and to get the peak and average
> > > > bandwidth for a given (or suspend) OPP entry.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/opp/core.c     | 301 +++++++++++++++++++++++++++++++++++------
> > > >  include/linux/pm_opp.h |  43 ++++++
> > > >  2 files changed, 305 insertions(+), 39 deletions(-)
> > > >
> > > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > > > index c79bbfac7289..3ff33a08198e 100644
> > > > --- a/drivers/opp/core.c
> > > > +++ b/drivers/opp/core.c
> > > > @@ -127,6 +127,29 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
> > > >
> > > > +/**
> > > > + * dev_pm_opp_get_bw() - Gets the bandwidth corresponding to an available opp
> > > > + * @opp:     opp for which peak bandwidth has to be returned for
> > >
> > > s/peak //
> >
> > Ack
> 
> Actually, isn't this correct as is? peak bandwidth is "returned".
> Average bandwidth is updated through the pointer.

I think we return two values here, peak and avg bw. Just that we can't
return two values from a routine, and so one is returned using a
pointer. And so I though writing just bw may be better.

-- 
viresh

Re: [PATCH v6 2/3] OPP: Add support for bandwidth OPP tables

[Saravana Kannan]

On Wed, Jan 8, 2020 at 8:31 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 08-01-20, 16:58, Saravana Kannan wrote:
> > On Wed, Jan 8, 2020 at 2:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 06-12-19, 16:24, Saravana Kannan wrote:
> > > > Not all devices quantify their performance points in terms of frequency.
> > > > Devices like interconnects quantify their performance points in terms of
> > > > bandwidth. We need a way to represent these bandwidth levels in OPP. So,
> > > > add support for parsing bandwidth OPPs from DT.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/opp/core.c | 15 +++++++++--
> > > >  drivers/opp/of.c   | 63 ++++++++++++++++++++++++++++++++--------------
> > > >  drivers/opp/opp.h  |  5 ++++
> > > >  3 files changed, 62 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > > > index be7a7d332332..c79bbfac7289 100644
> > > > --- a/drivers/opp/core.c
> > > > +++ b/drivers/opp/core.c
> > > > @@ -1282,11 +1282,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->peak_bw != opp2->peak_bw)
> > > > +             return opp1->peak_bw < opp2->peak_bw ? -1 : 1;
> > >
> > > Please also add level here.
> >
> > I can, but I vaguely remember finding opp-levels could have
> > duplicates? Am I wrong? If so I can add the opp-level comparison too.
>
> No they can't have duplicates.
>
> > > > +     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
> > > > @@ -1297,12 +1307,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 1cbb58240b80..b565da5a2b1f 100644
> > > > --- a/drivers/opp/of.c
> > > > +++ b/drivers/opp/of.c
> > > > @@ -521,6 +521,44 @@ 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)
> > > > +{
> > > > +     int ret;
> > > > +     u64 rate;
> > > > +     u32 bw;
> > > > +
> > > > +     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;
> > > > +             goto out;
> > > > +     }
> > > > +
> > > > +     ret = of_property_read_u32(np, "opp-peak-kBps", &bw);
> > > > +     if (!ret) {
> > > > +             new_opp->peak_bw = bw;
> > > > +
> > > > +             if (!of_property_read_u32(np, "opp-avg-kBps", &bw))
> > > > +                     new_opp->avg_bw = bw;
> > >
> > > Maybe
> > >                 of_property_read_u32(np, "opp-avg-kBps", &new_opp->avg_bw);
> > >
> > > and same for opp-peak-kbps as well.
> >
> > But those are not u32. Is it always safe to directly read into it
> > across all endian-ness and unsigned int sizes? I get tripped up by
> > that occasionally.
>
> It may not be safe.

Ok, so I'll leave it as is then.

>
> > > > +     }
> > > > +
> > > > +out:
> > > > +     *rate_not_available = !!ret;
> > > > +     /*
> > > > +      * If ret is 0 at this point, we have already found a key. If we
> > > > +      * haven't found a key yet, then ret already has an error value. In
> > > > +      * either case, we don't need to update ret.
> > > > +      */
> > > > +     of_property_read_u32(np, "opp-level", &new_opp->level);
> > >
> > > Yes, it wasn't done earlier but we should do it now. Check level as
> > > well and treat it as any other key.
> > >
> > > I think add a preparatory patch first which does all the cleanup
> > > before bandwidth thing is added.
> >
> > Ah come on man! You are making this too painful. It's okay to add a
> > few more error checks as part of implementing a new feature. Please
> > don't make me add more patches before this.
>
> It will only make your life easier and not painful in my opinion as
> the reviews are getting mixed/confused between the new things and the
> old fields right now. With a separate patch introducing just the
> bandwidth part, it will get reviewed in maximum 1-2 versions, else you
> will keep updating the unrelated patch and I will keep reviewing it as
> it is all a single patch.
>
> It is always suggested to break patches into the smallest possible
> meaningful separate things you want to achieve. You are introducing
> something here and adding cleanup to that.

Ok I completely misunderstood (in a stupid way) what you were asking
me to do. I don't mind breaking it up into smaller patches at all.
Will do so.

> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > >  /**
> > > >   * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> > > >   * @opp_table:       OPP table
> > > > @@ -558,26 +596,12 @@ 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);
> > > > +     if (ret) {
> > > > +             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);
> > > > @@ -616,7 +640,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> > > >       if (of_property_read_bool(np, "opp-suspend")) {
> > > >               if (opp_table->suspend_opp) {
> > > >                       /* Pick the OPP with higher rate as suspend OPP */
> > > > -                     if (new_opp->rate > opp_table->suspend_opp->rate) {
> > > > +                     if (opp_compare_key(new_opp,
> > > > +                                         opp_table->suspend_opp) > 1) {
> > >
> > > Maybe leave this place as is as we never want to compare anything else
> > > but rate.
> >
> > We do want to support suspend bandwidth.
>
> Yeah, I understood that from a later patch.
>
> > So I think I should have this
> > fix here so it works in general. Also, why not suspend opp-level?
>
> Because we don't want/need to set a specific value to the
> voltage-corners during suspend directly from the PM domain driver.
> This should happen automatically via a call from the underlying
> cpufreq or device driver.

Agreed for the example you are giving where PM domains/voltages are
dropped automatically when dropping the device freq to suspend freq.
I'm just wondering about a different scenario where if some power
domain needed to be at say 0.5v when it's suspended (no consumer using
it) to not lose state, or to come back up without brownouts, etc then
suspend OPP for PM domains might be useful. But I don't know enough
about that to speak with authority, so I'll leave it at this.

> And that's what I expected out of the interconnect thing. For example,
> say during suspend you put the interconnect or PM domains to a low
> bandwidth/level value, while the underlying device driver (which uses
> the interconnect or domain) never requested for it. Who will be
> responsible to restore the value during resume as we would be out of
> sync here.

I see this suspend-opp as a way to mark to what level the bandwidth
needs to be dropped to/brought back up from during suspend/resume by
the driver making interconnect bandwidth requests. For example, what
if the CPU -> DDR needed to be at some level to avoid suspend/resume
issues (say CPU bug with respect to timing/latencies)? In this
example, the CPU driver would be the one making bandwidth requests for
CPU -> DDR bandwidth during normal operation and during
suspend/resume. So it's basically exactly the same way it would treat
CPU freq OPP.

Btw, I don't have a strong opinion on this. But, even if we do only a
rate comparison, what does it even mean to compare rates for genpd or
BW opp tables? It's just weird. So I think it's nicer to just allow
marking any OPP as suspend OPP in any OPP table type. If there's a
need people can use it, if not, nothing is lost.

-Saravana

Re: [PATCH v6 3/3] OPP: Add helper function for bandwidth OPP tables

[Saravana Kannan]

On Wed, Jan 8, 2020 at 8:40 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 08-01-20, 16:58, Saravana Kannan wrote:
> > On Wed, Jan 8, 2020 at 3:19 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >  /**
> > > >   * dev_pm_opp_get_level() - Gets the level corresponding to an available opp
> > > >   * @opp:     opp for which level value has to be returned for
> > > > @@ -299,6 +322,34 @@ unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_freq);
> > > >
> > > > +/**
> > > > + * dev_pm_opp_get_suspend_opp_bw() - Get peak bandwidth of suspend opp in kBps
> > >
> > > Hmm, I wasn't expecting this. So the interconnects will also have a
> > > suspend OPP ?
> >
> > Yes, device voting for interconnect paths might want to lower the
> > bandwidth to a suspend bandwidth when they suspend.
>
> That's exactly what I was saying, the request for a change during
> suspend should come from the device

Agree. And this tells the device requesting for bandwidth, what
bandwidth to vote for when it's suspending. Keep in mind that these
bandwidth OPP tables are used by the consumers of the interconnects to
pick from a list of bandwidths to vote for.

> and you can't do it here, i.e.
> they should lower their frequency requirement, which should lead to a
> low bandwidth automatically.

Agree, the OPP framework itself shouldn't be responsible. And I'm not
doing anything here? Just giving those devices a way to look up what
their suspend bandwidth is? So they can vote for it when they suspend?

> > > > + * @dev:     device for which we do this operation
> > > > + * @avg_bw:  Pointer where the corresponding average bandwidth is stored.
> > > > + *           Can be NULL.
> > > > + *
> > > > + * Return: This function returns the peak bandwidth of the OPP marked as
> > > > + * suspend_opp if one is available, else returns 0;
> > > > + */
> > > > +unsigned long dev_pm_opp_get_suspend_opp_bw(struct device *dev,
> > > > +                                         unsigned long *avg_bw)
> > > > +{
> > > > +     struct opp_table *opp_table;
> > > > +     unsigned long peak_bw = 0;
> > > > +
> > > > +     opp_table = _find_opp_table(dev);
> > > > +     if (IS_ERR(opp_table))
> > > > +             return 0;
> > > > +
> > > > +     if (opp_table->suspend_opp && opp_table->suspend_opp->available)
> > > > +             peak_bw = dev_pm_opp_get_bw(opp_table->suspend_opp, avg_bw);
> > > > +
> > > > +     dev_pm_opp_put_opp_table(opp_table);
> > > > +
> > > > +     return peak_bw;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_bw);
> > > > +
> > > >  int _get_opp_count(struct opp_table *opp_table)
> > > >  {
> > > >       struct dev_pm_opp *opp;
> > > > @@ -343,6 +394,40 @@ int dev_pm_opp_get_opp_count(struct device *dev)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_count);
> > > >
> > >
> > > I think we should add function header here instead of the helpers
> > > which get exact match for freq, bw or level. And then pass a enum
> > > value to it, which tells what we are looking to compare. After that
> > > rest of the routines will be just one liners, make them macros in
> > > header file itself.
> >
> > Not sure I understand what you are saying here.
>
> Okay, lemme try again with proper example.
>
> enum opp_key_type {
>         OPP_KEY_FREQ = 0x1,
>         OPP_KEY_LEVEL= 0x2,
>         OPP_KEY_BW   = 0x4,
>         OPP_KEY_ALL  = 0x7,
> }
>
> /**
>  * Add function header here..
>  */
> struct dev_pm_opp *dev_pm_opp_find_opp_exact(struct device *dev,
>                                              enum opp_key_type key,
>                                              unsigned long key_value,
>                                              bool available)
> {
>        struct opp_table *opp_table;
>        struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
>
>        opp_table = _find_opp_table(dev);
>        if (IS_ERR(opp_table)) {
>                int r = PTR_ERR(opp_table);
>
>                dev_err(dev, "%s: OPP table not found (%d)\n", __func__, r);
>                return ERR_PTR(r);
>        }
>
>        mutex_lock(&opp_table->lock);
>
>        list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
>                if (temp_opp->available == available &&
>                    !opp_compare_key(temp_opp, key, key_value)) {
>                        opp = temp_opp;
>
>                        /* Increment the reference count of OPP */
>                        dev_pm_opp_get(opp);
>                        break;
>                }
>        }
>
>        mutex_unlock(&opp_table->lock);
>        dev_pm_opp_put_opp_table(opp_table);
>
>        return opp;
> }
>
> //Now in header file
>
> #define dev_pm_opp_find_freq_exact(dev, freq, available)        dev_pm_opp_find_opp_exact(dev, OPP_KEY_FREQ, freq, available);
> #define dev_pm_opp_find_level_exact(dev, level, available)      dev_pm_opp_find_opp_exact(dev, OPP_KEY_LEVEL, level, available);
> #define dev_pm_opp_find_bw_exact(dev, bw, available)            dev_pm_opp_find_opp_exact(dev, OPP_KEY_BW, bw, available);

Ok, but you want this done only for "exact" or for all the other
helpers too? Also, this means that I'll have to implement a
_opp_compare_key2() or whatever because the generic one that
automatically picks the key is still needed for the generic code. Is
that fine by you?

Also, ack to your other response.

-Saravana

Re: [PATCH v6 2/3] OPP: Add support for bandwidth OPP tables

[Viresh Kumar]

On 09-01-20, 10:35, Saravana Kannan wrote:
> Agreed for the example you are giving where PM domains/voltages are
> dropped automatically when dropping the device freq to suspend freq.
> I'm just wondering about a different scenario where if some power
> domain needed to be at say 0.5v when it's suspended (no consumer using
> it)

The domain should be powered off in this case I think.

> to not lose state, or to come back up without brownouts, etc then
> suspend OPP for PM domains might be useful. But I don't know enough
> about that to speak with authority, so I'll leave it at this.
> 
> I see this suspend-opp as a way to mark to what level the bandwidth
> needs to be dropped to/brought back up from during suspend/resume by
> the driver making interconnect bandwidth requests. For example, what
> if the CPU -> DDR needed to be at some level to avoid suspend/resume
> issues (say CPU bug with respect to timing/latencies)? In this
> example, the CPU driver would be the one making bandwidth requests for
> CPU -> DDR bandwidth during normal operation and during
> suspend/resume. So it's basically exactly the same way it would treat
> CPU freq OPP.

I understand your concerns but to me it all looks hypothetical right
now. I am not saying we won't support suspend-opp for interconnect or
domains, but that we should do it only if it is required.

> Btw, I don't have a strong opinion on this. But, even if we do only a
> rate comparison, what does it even mean to compare rates for genpd or
> BW opp tables?

We will never do the comparison because those tables will never have
the suspend OPP in the respective tables.

-- 
viresh

Re: [PATCH v6 3/3] OPP: Add helper function for bandwidth OPP tables

[Viresh Kumar]

On 09-01-20, 10:44, Saravana Kannan wrote:
> Agree, the OPP framework itself shouldn't be responsible. And I'm not
> doing anything here? Just giving those devices a way to look up what
> their suspend bandwidth is? So they can vote for it when they suspend?

I think this will originate by itself from the device in case of
interconnects as well and you don't need to separately have that for
interconnects.

For example, the device (lets say GPU) will have one of its OPP (and
frequency, maybe the lowest one) marked as suspend-OPP. Then the
driver which is doing the co-relation normally between GPU/DDR/Cache
OPPs should be able to do this conversion as well without any extra
help from the interconnect table.

If the minimum freq of the device correspond to the minimum freq of
the DDR/Cache during normal operation, that should still work during
suspend times, isn't it ?

> Ok, but you want this done only for "exact" or for all the other
> helpers too?

All helpers that you need for PM domains and interconnects.

> Also, this means that I'll have to implement a
> _opp_compare_key2() or whatever because the generic one that
> automatically picks the key is still needed for the generic code. Is
> that fine by you?

I am not concerned about the number of helpers but their optimization.
I will leave it for you to do that and review it when I see how you
have done it :)

-- 
viresh

Re: [PATCH v6 0/3] Introduce Bandwidth OPPs for interconnects

[Viresh Kumar]

On 06-12-19, 16:24, Saravana Kannan wrote:
> gpu_cache_opp_table: gpu_cache_opp_table {
> 	compatible = "operating-points-v2";
> 
> 	gpu_cache_3000: opp-3000 {
> 		opp-peak-KBps = <3000000>;
> 		opp-avg-KBps = <1000000>;
> 	};
> 	gpu_cache_6000: opp-6000 {
> 		opp-peak-KBps = <6000000>;
> 		opp-avg-KBps = <2000000>;
> 	};
> 	gpu_cache_9000: opp-9000 {
> 		opp-peak-KBps = <9000000>;
> 		opp-avg-KBps = <9000000>;
> 	};
> };
> 
> gpu_ddr_opp_table: gpu_ddr_opp_table {
> 	compatible = "operating-points-v2";
> 
> 	gpu_ddr_1525: opp-1525 {
> 		opp-peak-KBps = <1525000>;
> 		opp-avg-KBps = <452000>;
> 	};
> 	gpu_ddr_3051: opp-3051 {
> 		opp-peak-KBps = <3051000>;
> 		opp-avg-KBps = <915000>;
> 	};
> 	gpu_ddr_7500: opp-7500 {
> 		opp-peak-KBps = <7500000>;
> 		opp-avg-KBps = <3000000>;
> 	};
> };
> 
> gpu_opp_table: gpu_opp_table {
> 	compatible = "operating-points-v2";
> 	opp-shared;
> 
> 	opp-200000000 {
> 		opp-hz = /bits/ 64 <200000000>;
> 	};
> 	opp-400000000 {
> 		opp-hz = /bits/ 64 <400000000>;
> 	};
> };
> 
> gpu@7864000 {
> 	...
> 	operating-points-v2 = <&gpu_opp_table>, <&gpu_cache_opp_table>, <&gpu_ddr_opp_table>;

Okay, I got confused a bit again after some interaction with Sibi
today. The multiple phandle thing in the operating-points-v2 property
is there specifically for nodes that can provide multiple devices,
like PM domains where the provider may end up providing multiple
domains.

But I am not sure what you are going to do with the list of phandles
you have set for the GPU here.

We can not add multiple OPP tables for a single device right now.

-- 
viresh

Re: [PATCH v6 2/3] OPP: Add support for bandwidth OPP tables

[Sibi Sankar]

Hey Saravana,

On 2020-01-08 11:46, Saravana Kannan wrote:
> On Tue, Jan 7, 2020 at 11:28 AM Sibi Sankar <sibis@codeaurora.org> 
> wrote:
>> 
>> Hey Saravana,
>> 
>> Spent some time testing this series while
>> trying out dcvs on SDM845/SC7180. Apart from
>> the few minor issues it works quite well!
> 
> Thanks a lot for testing Sibi. Can you give a tested-by? Glad to hear
> it works well.

sry missed this mail. Sure will
add Tested-by in the next revision.

> 
>> On 2019-12-07 05:54, Saravana Kannan wrote:
>> > Not all devices quantify their performance points in terms of
>> > frequency.
>> > Devices like interconnects quantify their performance points in terms
>> > of
>> > bandwidth. We need a way to represent these bandwidth levels in OPP.
>> > So,
>> > add support for parsing bandwidth OPPs from DT.
>> >
>> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>> > ---
>> >  drivers/opp/core.c | 15 +++++++++--
>> >  drivers/opp/of.c   | 63 ++++++++++++++++++++++++++++++++--------------
>> >  drivers/opp/opp.h  |  5 ++++
>> >  3 files changed, 62 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> > index be7a7d332332..c79bbfac7289 100644
>> > --- a/drivers/opp/core.c
>> > +++ b/drivers/opp/core.c
>> > @@ -1282,11 +1282,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->peak_bw != opp2->peak_bw)
>> > +             return opp1->peak_bw < opp2->peak_bw ? -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
>> > @@ -1297,12 +1307,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 1cbb58240b80..b565da5a2b1f 100644
>> > --- a/drivers/opp/of.c
>> > +++ b/drivers/opp/of.c
>> > @@ -521,6 +521,44 @@ 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)
>> > +{
>> > +     int ret;
>> > +     u64 rate;
>> > +     u32 bw;
>> > +
>> > +     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;
>> > +             goto out;
>> > +     }
>> > +
>> > +     ret = of_property_read_u32(np, "opp-peak-kBps", &bw);
>> > +     if (!ret) {
>> > +             new_opp->peak_bw = bw;
>> > +
>> > +             if (!of_property_read_u32(np, "opp-avg-kBps", &bw))
>> > +                     new_opp->avg_bw = bw;
>> > +     }
>> > +
>> > +out:
>> > +     *rate_not_available = !!ret;
>> > +     /*
>> > +      * If ret is 0 at this point, we have already found a key. If we
>> > +      * haven't found a key yet, then ret already has an error value. In
>> > +      * either case, we don't need to update 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 +596,12 @@ 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);
>> > +     if (ret) {
>> 
>> if (!opp_table->is_genpd) {
>> 
>> _read_opp_key returns an error for genpd
>> opps so please check if it is a genpd
>> opp_table before erroring out here.
> 
> Thanks. I'll fix it in the next version.
> 
>> > +             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);
>> > @@ -616,7 +640,8 @@ static struct dev_pm_opp
>> > *_opp_add_static_v2(struct opp_table *opp_table,
>> >       if (of_property_read_bool(np, "opp-suspend")) {
>> >               if (opp_table->suspend_opp) {
>> >                       /* Pick the OPP with higher rate as suspend OPP */
>> > -                     if (new_opp->rate > opp_table->suspend_opp->rate) {
>> > +                     if (opp_compare_key(new_opp,
>> > +                                         opp_table->suspend_opp) > 1) {
>> 
>> shouldn't the condition be > 0?
> 
> Duh. Thanks. I'll fix it in the next version.
> 
> I'm guessing you tested with the fixes you pointed out?

yes

> 
> -Saravana

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