[PATCH v4 0/5] Add required-opps support to devfreq passive gov

[PATCH v4 0/5] Add required-opps support to devfreq passive gov

[Saravana Kannan]

The devfreq passive governor scales the frequency of a "child" device based
on the current frequency of a "parent" device (not parent/child in the
sense of device hierarchy). As of today, the passive governor requires one
of the following to work correctly:
1. The parent and child device have the same number of frequencies
2. The child device driver passes a mapping function to translate from
   parent frequency to child frequency.

When (1) is not true, (2) is the only option right now. But often times,
all that is required is a simple mapping from parent's frequency to child's
frequency.

Since OPPs already support pointing to other "required-opps", add support
for using that to map from parent device frequency to child device
frequency. That way, every child device driver doesn't have to implement a
separate mapping function anytime (1) isn't true.

Some common (but not comprehensive) reason for needing a devfreq passive
governor to adjust the frequency of one device based on another are:

1. These were the combination of frequencies that were validated/screened
   during the manufacturing process.
2. These are the sensible performance combinations between two devices
   interacting with each other. So that when one runs fast the other
   doesn't become the bottleneck.
3. Hardware bugs requiring some kind of frequency ratio between devices.

For example, the following mapping can't be captured in DT as it stands
today because the parent and child device have different number of OPPs.
But with this patch series, this mapping can be captured cleanly.

In arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi you have something
like this with the following changes:

	bus_g2d_400: bus0 {
		compatible = "samsung,exynos-bus";
		clocks = <&cmu_top CLK_ACLK_G2D_400>;
		clock-names = "bus";
		operating-points-v2 = <&bus_g2d_400_opp_table>;
		status = "disabled";
	};

	bus_noc2: bus9 {
		compatible = "samsung,exynos-bus";
		clocks = <&cmu_mif CLK_ACLK_BUS2_400>;
		clock-names = "bus";
		operating-points-v2 = <&bus_noc2_opp_table>;
		status = "disabled";
	};

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

		opp-400000000 {
			opp-hz = /bits/ 64 <400000000>;
			opp-microvolt = <1075000>;
			required-opps = <&noc2_400>;
		};
		opp-267000000 {
			opp-hz = /bits/ 64 <267000000>;
			opp-microvolt = <1000000>;
			required-opps = <&noc2_200>;
		};
		opp-200000000 {
			opp-hz = /bits/ 64 <200000000>;
			opp-microvolt = <975000>;
			required-opps = <&noc2_200>;
		};
		opp-160000000 {
			opp-hz = /bits/ 64 <160000000>;
			opp-microvolt = <962500>;
			required-opps = <&noc2_134>;
		};
		opp-134000000 {
			opp-hz = /bits/ 64 <134000000>;
			opp-microvolt = <950000>;
			required-opps = <&noc2_134>;
		};
		opp-100000000 {
			opp-hz = /bits/ 64 <100000000>;
			opp-microvolt = <937500>;
			required-opps = <&noc2_100>;
		};
	};

	bus_noc2_opp_table: opp_table6 {
		compatible = "operating-points-v2";

		noc2_400: opp-400000000 {
			opp-hz = /bits/ 64 <400000000>;
		};
		noc2_200: opp-200000000 {
			opp-hz = /bits/ 64 <200000000>;
		};
		noc2_134: opp-134000000 {
			opp-hz = /bits/ 64 <134000000>;
		};
		noc2_100: opp-100000000 {
			opp-hz = /bits/ 64 <100000000>;
		};
	};

-Saravana

v3 -> v4:
- Fixed documentation comments
- Fixed order of functions in .h file
- Renamed the new xlate API
- Caused _set_required_opps() to fail if all required opps tables aren't
  linked.
v2 -> v3:
- Rebased onto linux-next.
- Added documentation comment for new fields.
- Added support for lazy required-opps linking.
- Updated Ack/Reviewed-bys.
v1 -> v2:
- Cached OPP table reference in devfreq to avoid looking up every time.
- Renamed variable in passive governor to be more intuitive.
- Updated cover letter with examples.


Saravana Kannan (5):
  OPP: Allow required-opps even if the device doesn't have power-domains
  OPP: Add function to look up required OPP's for a given OPP
  OPP: Improve required-opps linking
  PM / devfreq: Cache OPP table reference in devfreq
  PM / devfreq: Add required OPPs support to passive governor

 drivers/devfreq/devfreq.c          |   6 ++
 drivers/devfreq/governor_passive.c |  20 ++++--
 drivers/opp/core.c                 |  83 +++++++++++++++++++---
 drivers/opp/of.c                   | 108 ++++++++++++++---------------
 drivers/opp/opp.h                  |   5 ++
 include/linux/devfreq.h            |   2 +
 include/linux/pm_opp.h             |  11 +++
 7 files changed, 165 insertions(+), 70 deletions(-)

-- 
2.22.0.709.g102302147b-goog

[PATCH v4 1/5] OPP: Allow required-opps even if the device doesn't have power-domains

[Saravana Kannan]

A Device-A can have a (minimum) performance requirement on another
Device-B to be able to function correctly. This performance requirement
on Device-B can also change based on the current performance level of
Device-A.

The existing required-opps feature fits well to describe this need. So,
instead of limiting required-opps to point to only PM-domain devices,
allow it to point to any device.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/opp/core.c |  2 +-
 drivers/opp/of.c   | 11 -----------
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c094d5d20fd7..438fcd134d93 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -707,7 +707,7 @@ static int _set_required_opps(struct device *dev,
 		return 0;
 
 	/* Single genpd case */
-	if (!genpd_virt_devs) {
+	if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
 		pstate = likely(opp) ? opp->required_opps[0]->pstate : 0;
 		ret = dev_pm_genpd_set_performance_state(dev, pstate);
 		if (ret) {
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index b313aca9894f..ff88eaf66b56 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -197,17 +197,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 
 		if (IS_ERR(required_opp_tables[i]))
 			goto free_required_tables;
-
-		/*
-		 * We only support genpd's OPPs in the "required-opps" for now,
-		 * as we don't know how much about other cases. Error out if the
-		 * required OPP doesn't belong to a genpd.
-		 */
-		if (!required_opp_tables[i]->is_genpd) {
-			dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
-				required_np);
-			goto free_required_tables;
-		}
 	}
 
 	goto put_np;
-- 
2.22.0.709.g102302147b-goog

[PATCH v4 2/5] OPP: Add function to look up required OPP's for a given OPP

[Saravana Kannan]

Add a function that allows looking up required OPPs given a source OPP
table, destination OPP table and the source OPP.

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

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 438fcd134d93..5460b7da110e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1883,6 +1883,59 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd);
 
+/**
+ * dev_pm_opp_xlate_required_opp() - Find required OPP for @src_table OPP.
+ * @src_table: OPP table which has @dst_table as one of its required OPP table.
+ * @dst_table: Required OPP table of the @src_table.
+ *
+ * This function returns the OPP (present in @dst_table) pointed out by the
+ * "required-opps" property of the OPP (present in @src_table).
+ *
+ * The callers are required to call dev_pm_opp_put() for the returned OPP after
+ * use.
+ *
+ * Return: destination table OPP on success, otherwise NULL on errors.
+ */
+struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table,
+						 struct opp_table *dst_table,
+						 struct dev_pm_opp *src_opp)
+{
+	struct dev_pm_opp *opp, *dest_opp = NULL;
+	int i;
+
+	if (!src_table || !dst_table || !src_opp)
+		return NULL;
+
+	for (i = 0; i < src_table->required_opp_count; i++) {
+		if (src_table->required_opp_tables[i]->np == dst_table->np)
+			break;
+	}
+
+	if (unlikely(i == src_table->required_opp_count)) {
+		pr_err("%s: Couldn't find matching OPP table (%p: %p)\n",
+		       __func__, src_table, dst_table);
+		return NULL;
+	}
+
+	mutex_lock(&src_table->lock);
+
+	list_for_each_entry(opp, &src_table->opp_list, node) {
+		if (opp == src_opp) {
+			dest_opp = opp->required_opps[i];
+			dev_pm_opp_get(dest_opp);
+			goto unlock;
+		}
+	}
+
+	pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table,
+	       dst_table);
+
+unlock:
+	mutex_unlock(&src_table->lock);
+
+	return dest_opp;
+}
+
 /**
  * dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for src_table.
  * @src_table: OPP table which has dst_table as one of its required OPP table.
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index af5021f27cb7..21d331de98b9 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -130,6 +130,9 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*s
 void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
 struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names);
 void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
+struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table,
+						 struct opp_table *dst_table,
+						 struct dev_pm_opp *src_opp);
 int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
@@ -299,6 +302,14 @@ static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, cons
 
 static inline void dev_pm_opp_detach_genpd(struct opp_table *opp_table) {}
 
+static inline struct dev_pm_opp *dev_pm_opp_xlate_required_opp(
+						struct opp_table *src_table,
+						struct opp_table *dst_table,
+						struct dev_pm_opp *src_opp)
+{
+	return NULL;
+}
+
 static inline int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate)
 {
 	return -ENOTSUPP;
-- 
2.22.0.709.g102302147b-goog

[PATCH v4 3/5] OPP: Improve required-opps linking

[Saravana Kannan]

Currently, the linking of required-opps fails silently if the
destination OPP table hasn't been added before the source OPP table is
added. This puts an unnecessary requirement that the destination table
be added before the source table is added.

In reality, the destination table is needed only when we try to
translate from source OPP to destination OPP. So, instead of
completely failing, retry linking the tables when the translation is
attempted.

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

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 5460b7da110e..01c71d145ec7 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -706,6 +706,9 @@ static int _set_required_opps(struct device *dev,
 	if (!required_opp_tables)
 		return 0;
 
+	if (!_of_lazy_link_required_tables(opp_table))
+		return -EPROBE_DEFER;
+
 	/* Single genpd case */
 	if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
 		pstate = likely(opp) ? opp->required_opps[0]->pstate : 0;
@@ -726,11 +729,16 @@ static int _set_required_opps(struct device *dev,
 	mutex_lock(&opp_table->genpd_virt_dev_lock);
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
-		pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
-
 		if (!genpd_virt_devs[i])
 			continue;
 
+		if (!opp->required_opps[i]) {
+			ret = -ENODEV;
+			break;
+		}
+
+		pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
+
 		ret = dev_pm_genpd_set_performance_state(genpd_virt_devs[i], pstate);
 		if (ret) {
 			dev_err(dev, "Failed to set performance rate of %s: %d (%d)\n",
@@ -1906,8 +1914,11 @@ struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table,
 	if (!src_table || !dst_table || !src_opp)
 		return NULL;
 
+	_of_lazy_link_required_tables(src_table);
+
 	for (i = 0; i < src_table->required_opp_count; i++) {
-		if (src_table->required_opp_tables[i]->np == dst_table->np)
+		if (src_table->required_opp_tables[i]
+		    && src_table->required_opp_tables[i]->np == dst_table->np)
 			break;
 	}
 
@@ -1970,6 +1981,8 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
 	if (!src_table->required_opp_count)
 		return pstate;
 
+	_of_lazy_link_required_tables(src_table);
+
 	for (i = 0; i < src_table->required_opp_count; i++) {
 		if (src_table->required_opp_tables[i]->np == dst_table->np)
 			break;
@@ -1985,15 +1998,16 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
 
 	list_for_each_entry(opp, &src_table->opp_list, node) {
 		if (opp->pstate == pstate) {
-			dest_pstate = opp->required_opps[i]->pstate;
-			goto unlock;
+			if (opp->required_opps[i])
+				dest_pstate = opp->required_opps[i]->pstate;
+			break;
 		}
 	}
 
-	pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table,
-	       dst_table);
+	if (dest_pstate < 0)
+		pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__,
+		       src_table, dst_table);
 
-unlock:
 	mutex_unlock(&src_table->lock);
 
 	return dest_pstate;
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index ff88eaf66b56..1f2860b2d6e5 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -145,7 +145,7 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
 		if (IS_ERR_OR_NULL(required_opp_tables[i]))
-			break;
+			continue;
 
 		dev_pm_opp_put_opp_table(required_opp_tables[i]);
 	}
@@ -165,8 +165,8 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 					     struct device_node *opp_np)
 {
 	struct opp_table **required_opp_tables;
-	struct device_node *required_np, *np;
-	int count, i;
+	struct device_node *np;
+	int count;
 
 	/* Traversing the first OPP node is all we need */
 	np = of_get_next_available_child(opp_np, NULL);
@@ -176,35 +176,65 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 	}
 
 	count = of_count_phandle_with_args(np, "required-opps", NULL);
+	of_node_put(np);
 	if (!count)
-		goto put_np;
+		return;
 
 	required_opp_tables = kcalloc(count, sizeof(*required_opp_tables),
 				      GFP_KERNEL);
 	if (!required_opp_tables)
-		goto put_np;
+		return;
 
 	opp_table->required_opp_tables = required_opp_tables;
 	opp_table->required_opp_count = count;
+}
+
+/*
+ * Try to link all required tables and return true if all of them have been
+ * linked. Otherwise, return false.
+ */
+bool _of_lazy_link_required_tables(struct opp_table *src)
+{
+	struct dev_pm_opp *src_opp, *tmp_opp;
+	struct opp_table *req_table;
+	struct device_node *req_np;
+	int i, num_linked = 0;
 
-	for (i = 0; i < count; i++) {
-		required_np = of_parse_required_opp(np, i);
-		if (!required_np)
-			goto free_required_tables;
+	mutex_lock(&src->lock);
 
-		required_opp_tables[i] = _find_table_of_opp_np(required_np);
-		of_node_put(required_np);
+	if (list_empty(&src->opp_list))
+		goto out;
 
-		if (IS_ERR(required_opp_tables[i]))
-			goto free_required_tables;
-	}
+	src_opp = list_first_entry(&src->opp_list, struct dev_pm_opp, node);
 
-	goto put_np;
+	for (i = 0; i < src->required_opp_count; i++) {
+		if (src->required_opp_tables[i]) {
+			num_linked++;
+			continue;
+		}
 
-free_required_tables:
-	_opp_table_free_required_tables(opp_table);
-put_np:
-	of_node_put(np);
+		req_np = of_parse_required_opp(src_opp->np, i);
+		if (!req_np)
+			continue;
+
+		req_table = _find_table_of_opp_np(req_np);
+		of_node_put(req_np);
+		if (!req_table)
+			continue;
+
+		src->required_opp_tables[i] = req_table;
+		list_for_each_entry(tmp_opp, &src->opp_list, node) {
+			req_np = of_parse_required_opp(tmp_opp->np, i);
+			tmp_opp->required_opps[i] = _find_opp_of_np(req_table,
+								    req_np);
+			of_node_put(req_np);
+		}
+		num_linked++;
+	}
+
+out:
+	mutex_unlock(&src->lock);
+	return num_linked == src->required_opp_count;
 }
 
 void _of_init_opp_table(struct opp_table *opp_table, struct device *dev,
@@ -267,7 +297,7 @@ void _of_opp_free_required_opps(struct opp_table *opp_table,
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
 		if (!required_opps[i])
-			break;
+			continue;
 
 		/* Put the reference back */
 		dev_pm_opp_put(required_opps[i]);
@@ -282,9 +312,7 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 				       struct dev_pm_opp *opp)
 {
 	struct dev_pm_opp **required_opps;
-	struct opp_table *required_table;
-	struct device_node *np;
-	int i, ret, count = opp_table->required_opp_count;
+	int count = opp_table->required_opp_count;
 
 	if (!count)
 		return 0;
@@ -295,32 +323,7 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 
 	opp->required_opps = required_opps;
 
-	for (i = 0; i < count; i++) {
-		required_table = opp_table->required_opp_tables[i];
-
-		np = of_parse_required_opp(opp->np, i);
-		if (unlikely(!np)) {
-			ret = -ENODEV;
-			goto free_required_opps;
-		}
-
-		required_opps[i] = _find_opp_of_np(required_table, np);
-		of_node_put(np);
-
-		if (!required_opps[i]) {
-			pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
-			       __func__, opp->np, i);
-			ret = -ENODEV;
-			goto free_required_opps;
-		}
-	}
-
 	return 0;
-
-free_required_opps:
-	_of_opp_free_required_opps(opp_table, opp);
-
-	return ret;
 }
 
 static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
@@ -687,6 +690,8 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
 	if (pstate_count)
 		opp_table->genpd_performance_state = true;
 
+	_of_lazy_link_required_tables(opp_table);
+
 	opp_table->parsed_static_opps = true;
 
 	return 0;
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 01a500e2c40a..77e8394fd0fe 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -223,12 +223,17 @@ void _put_opp_list_kref(struct opp_table *opp_table);
 void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index);
 void _of_clear_opp_table(struct opp_table *opp_table);
 struct opp_table *_managed_opp(struct device *dev, int index);
+bool _of_lazy_link_required_tables(struct opp_table *src);
 void _of_opp_free_required_opps(struct opp_table *opp_table,
 				struct dev_pm_opp *opp);
 #else
 static inline void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index) {}
 static inline void _of_clear_opp_table(struct opp_table *opp_table) {}
 static inline struct opp_table *_managed_opp(struct device *dev, int index) { return NULL; }
+bool _of_lazy_link_required_tables(struct opp_table *src)
+{
+	return true;
+}
 static inline void _of_opp_free_required_opps(struct opp_table *opp_table,
 					      struct dev_pm_opp *opp) {}
 #endif
-- 
2.22.0.709.g102302147b-goog

Re: [PATCH v4 3/5] OPP: Improve required-opps linking

[Sibi Sankar]

Hey Saravana,

Didn't get time to test v4 will
give it a spin tomorrow. Just
included some issues which should
still remain in v4.

On 2019-07-24 07:12, Saravana Kannan wrote:
> Currently, the linking of required-opps fails silently if the
> destination OPP table hasn't been added before the source OPP table is
> added. This puts an unnecessary requirement that the destination table
> be added before the source table is added.
> 
> In reality, the destination table is needed only when we try to
> translate from source OPP to destination OPP. So, instead of
> completely failing, retry linking the tables when the translation is
> attempted.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/opp/core.c |  30 ++++++++++----
>  drivers/opp/of.c   | 101 ++++++++++++++++++++++++---------------------
>  drivers/opp/opp.h  |   5 +++
>  3 files changed, 80 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 5460b7da110e..01c71d145ec7 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -706,6 +706,9 @@ static int _set_required_opps(struct device *dev,
>  	if (!required_opp_tables)
>  		return 0;
> 
> +	if (!_of_lazy_link_required_tables(opp_table))
> +		return -EPROBE_DEFER;
> +
>  	/* Single genpd case */
>  	if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
>  		pstate = likely(opp) ? opp->required_opps[0]->pstate : 0;
> @@ -726,11 +729,16 @@ static int _set_required_opps(struct device *dev,
>  	mutex_lock(&opp_table->genpd_virt_dev_lock);
> 
>  	for (i = 0; i < opp_table->required_opp_count; i++) {
> -		pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
> -
>  		if (!genpd_virt_devs[i])
>  			continue;
> 
> +		if (!opp->required_opps[i]) {
> +			ret = -ENODEV;
> +			break;
> +		}
> +
> +		pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
> +
>  		ret = dev_pm_genpd_set_performance_state(genpd_virt_devs[i], 
> pstate);
>  		if (ret) {
>  			dev_err(dev, "Failed to set performance rate of %s: %d (%d)\n",
> @@ -1906,8 +1914,11 @@ struct dev_pm_opp
> *dev_pm_opp_xlate_required_opp(struct opp_table *src_table,
>  	if (!src_table || !dst_table || !src_opp)
>  		return NULL;
> 
> +	_of_lazy_link_required_tables(src_table);
> +
>  	for (i = 0; i < src_table->required_opp_count; i++) {
> -		if (src_table->required_opp_tables[i]->np == dst_table->np)
> +		if (src_table->required_opp_tables[i]
> +		    && src_table->required_opp_tables[i]->np == dst_table->np)
>  			break;
>  	}
> 
> @@ -1970,6 +1981,8 @@ int dev_pm_opp_xlate_performance_state(struct
> opp_table *src_table,
>  	if (!src_table->required_opp_count)
>  		return pstate;
> 
> +	_of_lazy_link_required_tables(src_table);
> +
>  	for (i = 0; i < src_table->required_opp_count; i++) {
>  		if (src_table->required_opp_tables[i]->np == dst_table->np)
>  			break;
> @@ -1985,15 +1998,16 @@ int dev_pm_opp_xlate_performance_state(struct
> opp_table *src_table,
> 
>  	list_for_each_entry(opp, &src_table->opp_list, node) {
>  		if (opp->pstate == pstate) {
> -			dest_pstate = opp->required_opps[i]->pstate;
> -			goto unlock;
> +			if (opp->required_opps[i])
> +				dest_pstate = opp->required_opps[i]->pstate;
> +			break;
>  		}
>  	}
> 
> -	pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, 
> src_table,
> -	       dst_table);
> +	if (dest_pstate < 0)
> +		pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__,
> +		       src_table, dst_table);
> 
> -unlock:
>  	mutex_unlock(&src_table->lock);
> 
>  	return dest_pstate;
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index ff88eaf66b56..1f2860b2d6e5 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -145,7 +145,7 @@ static void _opp_table_free_required_tables(struct
> opp_table *opp_table)
> 
>  	for (i = 0; i < opp_table->required_opp_count; i++) {
>  		if (IS_ERR_OR_NULL(required_opp_tables[i]))
> -			break;
> +			continue;
> 
>  		dev_pm_opp_put_opp_table(required_opp_tables[i]);
>  	}
> @@ -165,8 +165,8 @@ static void
> _opp_table_alloc_required_tables(struct opp_table *opp_table,
>  					     struct device_node *opp_np)
>  {
>  	struct opp_table **required_opp_tables;
> -	struct device_node *required_np, *np;
> -	int count, i;
> +	struct device_node *np;
> +	int count;
> 
>  	/* Traversing the first OPP node is all we need */
>  	np = of_get_next_available_child(opp_np, NULL);
> @@ -176,35 +176,65 @@ static void
> _opp_table_alloc_required_tables(struct opp_table *opp_table,
>  	}
> 
>  	count = of_count_phandle_with_args(np, "required-opps", NULL);
> +	of_node_put(np);
>  	if (!count)

should this be count <= 0 instead ?

> -		goto put_np;
> +		return;
> 
>  	required_opp_tables = kcalloc(count, sizeof(*required_opp_tables),
>  				      GFP_KERNEL);
>  	if (!required_opp_tables)
> -		goto put_np;
> +		return;
> 
>  	opp_table->required_opp_tables = required_opp_tables;
>  	opp_table->required_opp_count = count;

error values could be assigned to
unsigned int member required_opp_count

> +}
> +
> +/*
> + * Try to link all required tables and return true if all of them have 
> been
> + * linked. Otherwise, return false.
> + */
> +bool _of_lazy_link_required_tables(struct opp_table *src)
> +{
> +	struct dev_pm_opp *src_opp, *tmp_opp;
> +	struct opp_table *req_table;
> +	struct device_node *req_np;
> +	int i, num_linked = 0;
> 
> -	for (i = 0; i < count; i++) {
> -		required_np = of_parse_required_opp(np, i);
> -		if (!required_np)
> -			goto free_required_tables;
> +	mutex_lock(&src->lock);
> 
> -		required_opp_tables[i] = _find_table_of_opp_np(required_np);
> -		of_node_put(required_np);
> +	if (list_empty(&src->opp_list))
> +		goto out;
> 
> -		if (IS_ERR(required_opp_tables[i]))
> -			goto free_required_tables;
> -	}
> +	src_opp = list_first_entry(&src->opp_list, struct dev_pm_opp, node);
> 
> -	goto put_np;
> +	for (i = 0; i < src->required_opp_count; i++) {
> +		if (src->required_opp_tables[i]) {
> +			num_linked++;
> +			continue;
> +		}
> 
> -free_required_tables:
> -	_opp_table_free_required_tables(opp_table);
> -put_np:
> -	of_node_put(np);
> +		req_np = of_parse_required_opp(src_opp->np, i);
> +		if (!req_np)
> +			continue;
> +
> +		req_table = _find_table_of_opp_np(req_np);
> +		of_node_put(req_np);
> +		if (!req_table)

should be IS_ERR instead since
req_table could be ERR_PTR(-ENODEV)

> +			continue;
> +
> +		src->required_opp_tables[i] = req_table;
> +		list_for_each_entry(tmp_opp, &src->opp_list, node) {
> +			req_np = of_parse_required_opp(tmp_opp->np, i);
> +			tmp_opp->required_opps[i] = _find_opp_of_np(req_table,
> +								    req_np);
> +			of_node_put(req_np);
> +		}
> +		num_linked++;
> +	}
> +
> +out:
> +	mutex_unlock(&src->lock);
> +	return num_linked == src->required_opp_count;
>  }
> 
>  void _of_init_opp_table(struct opp_table *opp_table, struct device 
> *dev,
> @@ -267,7 +297,7 @@ void _of_opp_free_required_opps(struct opp_table 
> *opp_table,
> 
>  	for (i = 0; i < opp_table->required_opp_count; i++) {
>  		if (!required_opps[i])
> -			break;
> +			continue;
> 
>  		/* Put the reference back */
>  		dev_pm_opp_put(required_opps[i]);
> @@ -282,9 +312,7 @@ static int _of_opp_alloc_required_opps(struct
> opp_table *opp_table,
>  				       struct dev_pm_opp *opp)
>  {
>  	struct dev_pm_opp **required_opps;
> -	struct opp_table *required_table;
> -	struct device_node *np;
> -	int i, ret, count = opp_table->required_opp_count;
> +	int count = opp_table->required_opp_count;
> 
>  	if (!count)
>  		return 0;
> @@ -295,32 +323,7 @@ static int _of_opp_alloc_required_opps(struct
> opp_table *opp_table,
> 
>  	opp->required_opps = required_opps;
> 
> -	for (i = 0; i < count; i++) {
> -		required_table = opp_table->required_opp_tables[i];
> -
> -		np = of_parse_required_opp(opp->np, i);
> -		if (unlikely(!np)) {
> -			ret = -ENODEV;
> -			goto free_required_opps;
> -		}
> -
> -		required_opps[i] = _find_opp_of_np(required_table, np);
> -		of_node_put(np);
> -
> -		if (!required_opps[i]) {
> -			pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
> -			       __func__, opp->np, i);
> -			ret = -ENODEV;
> -			goto free_required_opps;
> -		}
> -	}
> -
>  	return 0;
> -
> -free_required_opps:
> -	_of_opp_free_required_opps(opp_table, opp);
> -
> -	return ret;
>  }
> 
>  static bool _opp_is_supported(struct device *dev, struct opp_table 
> *opp_table,
> @@ -687,6 +690,8 @@ static int _of_add_opp_table_v2(struct device
> *dev, struct opp_table *opp_table)
>  	if (pstate_count)
>  		opp_table->genpd_performance_state = true;
> 
> +	_of_lazy_link_required_tables(opp_table);
> +
>  	opp_table->parsed_static_opps = true;
> 
>  	return 0;
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 01a500e2c40a..77e8394fd0fe 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -223,12 +223,17 @@ void _put_opp_list_kref(struct opp_table 
> *opp_table);
>  void _of_init_opp_table(struct opp_table *opp_table, struct device
> *dev, int index);
>  void _of_clear_opp_table(struct opp_table *opp_table);
>  struct opp_table *_managed_opp(struct device *dev, int index);
> +bool _of_lazy_link_required_tables(struct opp_table *src);
>  void _of_opp_free_required_opps(struct opp_table *opp_table,
>  				struct dev_pm_opp *opp);
>  #else
>  static inline void _of_init_opp_table(struct opp_table *opp_table,
> struct device *dev, int index) {}
>  static inline void _of_clear_opp_table(struct opp_table *opp_table) {}
>  static inline struct opp_table *_managed_opp(struct device *dev, int
> index) { return NULL; }
> +bool _of_lazy_link_required_tables(struct opp_table *src)

should be static inline bool

> +{
> +	return true;
> +}
>  static inline void _of_opp_free_required_opps(struct opp_table 
> *opp_table,
>  					      struct dev_pm_opp *opp) {}
>  #endif

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

Re: [PATCH v4 3/5] OPP: Improve required-opps linking

[Saravana Kannan]

:
On Wed, Jul 24, 2019 at 10:59 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Saravana,
>
> Didn't get time to test v4 will
> give it a spin tomorrow. Just
> included some issues which should
> still remain in v4.

Thanks.

>
> On 2019-07-24 07:12, Saravana Kannan wrote:
> > Currently, the linking of required-opps fails silently if the
> > destination OPP table hasn't been added before the source OPP table is
> > added. This puts an unnecessary requirement that the destination table
> > be added before the source table is added.
> >
> > In reality, the destination table is needed only when we try to
> > translate from source OPP to destination OPP. So, instead of
> > completely failing, retry linking the tables when the translation is
> > attempted.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/opp/core.c |  30 ++++++++++----
> >  drivers/opp/of.c   | 101 ++++++++++++++++++++++++---------------------
> >  drivers/opp/opp.h  |   5 +++
> >  3 files changed, 80 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 5460b7da110e..01c71d145ec7 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -706,6 +706,9 @@ static int _set_required_opps(struct device *dev,
> >       if (!required_opp_tables)
> >               return 0;
> >
> > +     if (!_of_lazy_link_required_tables(opp_table))
> > +             return -EPROBE_DEFER;
> > +
> >       /* Single genpd case */
> >       if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
> >               pstate = likely(opp) ? opp->required_opps[0]->pstate : 0;
> > @@ -726,11 +729,16 @@ static int _set_required_opps(struct device *dev,
> >       mutex_lock(&opp_table->genpd_virt_dev_lock);
> >
> >       for (i = 0; i < opp_table->required_opp_count; i++) {
> > -             pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
> > -
> >               if (!genpd_virt_devs[i])
> >                       continue;
> >
> > +             if (!opp->required_opps[i]) {
> > +                     ret = -ENODEV;
> > +                     break;
> > +             }
> > +
> > +             pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
> > +
> >               ret = dev_pm_genpd_set_performance_state(genpd_virt_devs[i],
> > pstate);
> >               if (ret) {
> >                       dev_err(dev, "Failed to set performance rate of %s: %d (%d)\n",
> > @@ -1906,8 +1914,11 @@ struct dev_pm_opp
> > *dev_pm_opp_xlate_required_opp(struct opp_table *src_table,
> >       if (!src_table || !dst_table || !src_opp)
> >               return NULL;
> >
> > +     _of_lazy_link_required_tables(src_table);
> > +
> >       for (i = 0; i < src_table->required_opp_count; i++) {
> > -             if (src_table->required_opp_tables[i]->np == dst_table->np)
> > +             if (src_table->required_opp_tables[i]
> > +                 && src_table->required_opp_tables[i]->np == dst_table->np)
> >                       break;
> >       }
> >
> > @@ -1970,6 +1981,8 @@ int dev_pm_opp_xlate_performance_state(struct
> > opp_table *src_table,
> >       if (!src_table->required_opp_count)
> >               return pstate;
> >
> > +     _of_lazy_link_required_tables(src_table);
> > +
> >       for (i = 0; i < src_table->required_opp_count; i++) {
> >               if (src_table->required_opp_tables[i]->np == dst_table->np)
> >                       break;
> > @@ -1985,15 +1998,16 @@ int dev_pm_opp_xlate_performance_state(struct
> > opp_table *src_table,
> >
> >       list_for_each_entry(opp, &src_table->opp_list, node) {
> >               if (opp->pstate == pstate) {
> > -                     dest_pstate = opp->required_opps[i]->pstate;
> > -                     goto unlock;
> > +                     if (opp->required_opps[i])
> > +                             dest_pstate = opp->required_opps[i]->pstate;
> > +                     break;
> >               }
> >       }
> >
> > -     pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__,
> > src_table,
> > -            dst_table);
> > +     if (dest_pstate < 0)
> > +             pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__,
> > +                    src_table, dst_table);
> >
> > -unlock:
> >       mutex_unlock(&src_table->lock);
> >
> >       return dest_pstate;
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index ff88eaf66b56..1f2860b2d6e5 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -145,7 +145,7 @@ static void _opp_table_free_required_tables(struct
> > opp_table *opp_table)
> >
> >       for (i = 0; i < opp_table->required_opp_count; i++) {
> >               if (IS_ERR_OR_NULL(required_opp_tables[i]))
> > -                     break;
> > +                     continue;
> >
> >               dev_pm_opp_put_opp_table(required_opp_tables[i]);
> >       }
> > @@ -165,8 +165,8 @@ static void
> > _opp_table_alloc_required_tables(struct opp_table *opp_table,
> >                                            struct device_node *opp_np)
> >  {
> >       struct opp_table **required_opp_tables;
> > -     struct device_node *required_np, *np;
> > -     int count, i;
> > +     struct device_node *np;
> > +     int count;
> >
> >       /* Traversing the first OPP node is all we need */
> >       np = of_get_next_available_child(opp_np, NULL);
> > @@ -176,35 +176,65 @@ static void
> > _opp_table_alloc_required_tables(struct opp_table *opp_table,
> >       }
> >
> >       count = of_count_phandle_with_args(np, "required-opps", NULL);
> > +     of_node_put(np);
> >       if (!count)
>
> should this be count <= 0 instead ?

Looks like in the current implementation of
of_count_phandle_with_args() returns 0 if the required-opps property
doesn't exist. It can return errors only if the phandles are invalid
(hopefully DTC would catch that) or if the "cells" param is listed by
the DT node doesn't have it. The later is not a problem here since
cells is NULL.

But I think what you suggested would be safer if the other error cases
aren't caught by the DTC. But this is existing code and I don't want
to fix unrelated stuff in this patch.

> > -             goto put_np;
> > +             return;
> >
> >       required_opp_tables = kcalloc(count, sizeof(*required_opp_tables),
> >                                     GFP_KERNEL);
> >       if (!required_opp_tables)
> > -             goto put_np;
> > +             return;
> >
> >       opp_table->required_opp_tables = required_opp_tables;
> >       opp_table->required_opp_count = count;
>
> error values could be assigned to
> unsigned int member required_opp_count

That's assuming a ~4GB kernel malloc succeeds :) Still a valid point,
but don't want to fix that in this patch series.

> > +}
> > +
> > +/*
> > + * Try to link all required tables and return true if all of them have
> > been
> > + * linked. Otherwise, return false.
> > + */
> > +bool _of_lazy_link_required_tables(struct opp_table *src)
> > +{
> > +     struct dev_pm_opp *src_opp, *tmp_opp;
> > +     struct opp_table *req_table;
> > +     struct device_node *req_np;
> > +     int i, num_linked = 0;
> >
> > -     for (i = 0; i < count; i++) {
> > -             required_np = of_parse_required_opp(np, i);
> > -             if (!required_np)
> > -                     goto free_required_tables;
> > +     mutex_lock(&src->lock);
> >
> > -             required_opp_tables[i] = _find_table_of_opp_np(required_np);
> > -             of_node_put(required_np);
> > +     if (list_empty(&src->opp_list))
> > +             goto out;
> >
> > -             if (IS_ERR(required_opp_tables[i]))
> > -                     goto free_required_tables;
> > -     }
> > +     src_opp = list_first_entry(&src->opp_list, struct dev_pm_opp, node);
> >
> > -     goto put_np;
> > +     for (i = 0; i < src->required_opp_count; i++) {
> > +             if (src->required_opp_tables[i]) {
> > +                     num_linked++;
> > +                     continue;
> > +             }
> >
> > -free_required_tables:
> > -     _opp_table_free_required_tables(opp_table);
> > -put_np:
> > -     of_node_put(np);
> > +             req_np = of_parse_required_opp(src_opp->np, i);
> > +             if (!req_np)
> > +                     continue;
> > +
> > +             req_table = _find_table_of_opp_np(req_np);
> > +             of_node_put(req_np);
> > +             if (!req_table)
>
> should be IS_ERR instead since
> req_table could be ERR_PTR(-ENODEV)

Will fix in the next version.

>
> > +                     continue;
> > +
> > +             src->required_opp_tables[i] = req_table;
> > +             list_for_each_entry(tmp_opp, &src->opp_list, node) {
> > +                     req_np = of_parse_required_opp(tmp_opp->np, i);
> > +                     tmp_opp->required_opps[i] = _find_opp_of_np(req_table,
> > +                                                                 req_np);
> > +                     of_node_put(req_np);
> > +             }
> > +             num_linked++;
> > +     }
> > +
> > +out:
> > +     mutex_unlock(&src->lock);
> > +     return num_linked == src->required_opp_count;
> >  }
> >
> >  void _of_init_opp_table(struct opp_table *opp_table, struct device
> > *dev,
> > @@ -267,7 +297,7 @@ void _of_opp_free_required_opps(struct opp_table
> > *opp_table,
> >
> >       for (i = 0; i < opp_table->required_opp_count; i++) {
> >               if (!required_opps[i])
> > -                     break;
> > +                     continue;
> >
> >               /* Put the reference back */
> >               dev_pm_opp_put(required_opps[i]);
> > @@ -282,9 +312,7 @@ static int _of_opp_alloc_required_opps(struct
> > opp_table *opp_table,
> >                                      struct dev_pm_opp *opp)
> >  {
> >       struct dev_pm_opp **required_opps;
> > -     struct opp_table *required_table;
> > -     struct device_node *np;
> > -     int i, ret, count = opp_table->required_opp_count;
> > +     int count = opp_table->required_opp_count;
> >
> >       if (!count)
> >               return 0;
> > @@ -295,32 +323,7 @@ static int _of_opp_alloc_required_opps(struct
> > opp_table *opp_table,
> >
> >       opp->required_opps = required_opps;
> >
> > -     for (i = 0; i < count; i++) {
> > -             required_table = opp_table->required_opp_tables[i];
> > -
> > -             np = of_parse_required_opp(opp->np, i);
> > -             if (unlikely(!np)) {
> > -                     ret = -ENODEV;
> > -                     goto free_required_opps;
> > -             }
> > -
> > -             required_opps[i] = _find_opp_of_np(required_table, np);
> > -             of_node_put(np);
> > -
> > -             if (!required_opps[i]) {
> > -                     pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
> > -                            __func__, opp->np, i);
> > -                     ret = -ENODEV;
> > -                     goto free_required_opps;
> > -             }
> > -     }
> > -
> >       return 0;
> > -
> > -free_required_opps:
> > -     _of_opp_free_required_opps(opp_table, opp);
> > -
> > -     return ret;
> >  }
> >
> >  static bool _opp_is_supported(struct device *dev, struct opp_table
> > *opp_table,
> > @@ -687,6 +690,8 @@ static int _of_add_opp_table_v2(struct device
> > *dev, struct opp_table *opp_table)
> >       if (pstate_count)
> >               opp_table->genpd_performance_state = true;
> >
> > +     _of_lazy_link_required_tables(opp_table);
> > +
> >       opp_table->parsed_static_opps = true;
> >
> >       return 0;
> > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> > index 01a500e2c40a..77e8394fd0fe 100644
> > --- a/drivers/opp/opp.h
> > +++ b/drivers/opp/opp.h
> > @@ -223,12 +223,17 @@ void _put_opp_list_kref(struct opp_table
> > *opp_table);
> >  void _of_init_opp_table(struct opp_table *opp_table, struct device
> > *dev, int index);
> >  void _of_clear_opp_table(struct opp_table *opp_table);
> >  struct opp_table *_managed_opp(struct device *dev, int index);
> > +bool _of_lazy_link_required_tables(struct opp_table *src);
> >  void _of_opp_free_required_opps(struct opp_table *opp_table,
> >                               struct dev_pm_opp *opp);
> >  #else
> >  static inline void _of_init_opp_table(struct opp_table *opp_table,
> > struct device *dev, int index) {}
> >  static inline void _of_clear_opp_table(struct opp_table *opp_table) {}
> >  static inline struct opp_table *_managed_opp(struct device *dev, int
> > index) { return NULL; }
> > +bool _of_lazy_link_required_tables(struct opp_table *src)
>
> should be static inline bool

Thanks. Good catch.

>
> > +{
> > +     return true;
> > +}
> >  static inline void _of_opp_free_required_opps(struct opp_table
> > *opp_table,
> >                                             struct dev_pm_opp *opp) {}
> >  #endif
>

I'll send out a v5 series after Viresh takes a look at this and
confirms this design is okay.

-Saravana

[PATCH v4 4/5] PM / devfreq: Cache OPP table reference in devfreq

[Saravana Kannan]

The OPP table can be used often in devfreq. Trying to get it each time can
be expensive, so cache it in the devfreq struct.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
---
 drivers/devfreq/devfreq.c | 6 ++++++
 include/linux/devfreq.h   | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 784c08e4f931..7984b01d585d 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -594,6 +594,8 @@ static void devfreq_dev_release(struct device *dev)
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
+	if (devfreq->opp_table)
+		dev_pm_opp_put_opp_table(devfreq->opp_table);
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
 }
@@ -674,6 +676,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->max_freq = devfreq->scaling_max_freq;
 
 	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
+	devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
+	if (IS_ERR(devfreq->opp_table))
+		devfreq->opp_table = NULL;
+
 	atomic_set(&devfreq->suspend_count, 0);
 
 	dev_set_name(&devfreq->dev, "devfreq%d",
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 2bae9ed3c783..1c05129f76c0 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -116,6 +116,7 @@ struct devfreq_dev_profile {
  * @profile:	device-specific devfreq profile
  * @governor:	method how to choose frequency based on the usage.
  * @governor_name:	devfreq governor name for use with this devfreq
+ * @opp_table:	Reference to OPP table of dev.parent, if one exists.
  * @nb:		notifier block used to notify devfreq object that it should
  *		reevaluate operable frequencies. Devfreq users may use
  *		devfreq.nb to the corresponding register notifier call chain.
@@ -153,6 +154,7 @@ struct devfreq {
 	struct devfreq_dev_profile *profile;
 	const struct devfreq_governor *governor;
 	char governor_name[DEVFREQ_NAME_LEN];
+	struct opp_table *opp_table;
 	struct notifier_block nb;
 	struct delayed_work work;
 
-- 
2.22.0.709.g102302147b-goog

[PATCH v4 5/5] PM / devfreq: Add required OPPs support to passive governor

[Saravana Kannan]

Look at the required OPPs of the "parent" device to determine the OPP that
is required from the slave device managed by the passive governor. This
allows having mappings between a parent device and a slave device even when
they don't have the same number of OPPs.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/governor_passive.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 58308948b863..14dc5bb58733 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -19,7 +19,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 			= (struct devfreq_passive_data *)devfreq->data;
 	struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
 	unsigned long child_freq = ULONG_MAX;
-	struct dev_pm_opp *opp;
+	struct dev_pm_opp *opp = NULL, *p_opp = NULL;
 	int i, count, ret = 0;
 
 	/*
@@ -56,13 +56,20 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 	 * list of parent device. Because in this case, *freq is temporary
 	 * value which is decided by ondemand governor.
 	 */
-	opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
-	if (IS_ERR(opp)) {
-		ret = PTR_ERR(opp);
+	p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
+	if (IS_ERR(p_opp)) {
+		ret = PTR_ERR(p_opp);
 		goto out;
 	}
 
-	dev_pm_opp_put(opp);
+	if (devfreq->opp_table && parent_devfreq->opp_table)
+		opp = dev_pm_opp_xlate_required_opp(parent_devfreq->opp_table,
+						    devfreq->opp_table, p_opp);
+	if (opp) {
+		*freq = dev_pm_opp_get_freq(opp);
+		dev_pm_opp_put(opp);
+		goto out;
+	}
 
 	/*
 	 * Get the OPP table's index of decided freqeuncy by governor
@@ -89,6 +96,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 	*freq = child_freq;
 
 out:
+	if (!IS_ERR_OR_NULL(opp))
+		dev_pm_opp_put(p_opp);
+
 	return ret;
 }
 
-- 
2.22.0.709.g102302147b-goog

Re: [PATCH v4 0/5] Add required-opps support to devfreq passive gov

[Viresh Kumar]

On 23-07-19, 18:42, Saravana Kannan wrote:
> The devfreq passive governor scales the frequency of a "child" device based
> on the current frequency of a "parent" device (not parent/child in the
> sense of device hierarchy). As of today, the passive governor requires one
> of the following to work correctly:
> 1. The parent and child device have the same number of frequencies
> 2. The child device driver passes a mapping function to translate from
>    parent frequency to child frequency.

> v3 -> v4:
> - Fixed documentation comments
> - Fixed order of functions in .h file
> - Renamed the new xlate API
> - Caused _set_required_opps() to fail if all required opps tables aren't
>   linked.

We are already in the middle of a discussion for your previous version
and I haven't said yet that I am happy with what you suggested just 2
days back. Why send another version so soon ? The next merge window is
also very far in time from now.

Please wait for a few days before sending newer versions, I will
continue discussion on the previous version only for now.

-- 
viresh

Re: [PATCH v4 0/5] Add required-opps support to devfreq passive gov

[Saravana Kannan]

On Wed, Jul 24, 2019 at 7:30 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 23-07-19, 18:42, Saravana Kannan wrote:
> > The devfreq passive governor scales the frequency of a "child" device based
> > on the current frequency of a "parent" device (not parent/child in the
> > sense of device hierarchy). As of today, the passive governor requires one
> > of the following to work correctly:
> > 1. The parent and child device have the same number of frequencies
> > 2. The child device driver passes a mapping function to translate from
> >    parent frequency to child frequency.
>
> > v3 -> v4:
> > - Fixed documentation comments
> > - Fixed order of functions in .h file
> > - Renamed the new xlate API
> > - Caused _set_required_opps() to fail if all required opps tables aren't
> >   linked.
>
> We are already in the middle of a discussion for your previous version
> and I haven't said yet that I am happy with what you suggested just 2
> days back. Why send another version so soon ?

I wanted you to see how I addressed your comments. It didn't look like
you were going to make more comments on the code.

> The next merge window is
> also very far in time from now.
>
> Please wait for a few days before sending newer versions, I will
> continue discussion on the previous version only for now.

Ok

-Saravana

Re: [PATCH v4 0/5] Add required-opps support to devfreq passive gov

[Viresh Kumar]

On 24-07-19, 20:40, Saravana Kannan wrote:
> On Wed, Jul 24, 2019 at 7:30 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 23-07-19, 18:42, Saravana Kannan wrote:
> > > The devfreq passive governor scales the frequency of a "child" device based
> > > on the current frequency of a "parent" device (not parent/child in the
> > > sense of device hierarchy). As of today, the passive governor requires one
> > > of the following to work correctly:
> > > 1. The parent and child device have the same number of frequencies
> > > 2. The child device driver passes a mapping function to translate from
> > >    parent frequency to child frequency.
> >
> > > v3 -> v4:
> > > - Fixed documentation comments
> > > - Fixed order of functions in .h file
> > > - Renamed the new xlate API
> > > - Caused _set_required_opps() to fail if all required opps tables aren't
> > >   linked.
> >
> > We are already in the middle of a discussion for your previous version
> > and I haven't said yet that I am happy with what you suggested just 2
> > days back. Why send another version so soon ?
> 
> I wanted you to see how I addressed your comments.

Sure, but that is just half the comments.

> It didn't look like
> you were going to make more comments on the code.

I posted some queries and you posted your opinions on them. Now
shouldn't I get a chance to reply again to see if I agree with your
replies or if we can settle to something else ? I only got one day in
between where I was busy with other stuff and so couldn't come back to
it. Please wait a little longer specially when the comments aren't
minor in nature.

Anyway, lets get over it now. Lets continue our discussion on V3 and
then we can have a V5 :)

Have a good day Saravana.

-- 
viresh

Re: [PATCH v4 0/5] Add required-opps support to devfreq passive gov

[Saravana Kannan]

On Wed, Jul 24, 2019 at 10:22 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 24-07-19, 20:40, Saravana Kannan wrote:
> > On Wed, Jul 24, 2019 at 7:30 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 23-07-19, 18:42, Saravana Kannan wrote:
> > > > The devfreq passive governor scales the frequency of a "child" device based
> > > > on the current frequency of a "parent" device (not parent/child in the
> > > > sense of device hierarchy). As of today, the passive governor requires one
> > > > of the following to work correctly:
> > > > 1. The parent and child device have the same number of frequencies
> > > > 2. The child device driver passes a mapping function to translate from
> > > >    parent frequency to child frequency.
> > >
> > > > v3 -> v4:
> > > > - Fixed documentation comments
> > > > - Fixed order of functions in .h file
> > > > - Renamed the new xlate API
> > > > - Caused _set_required_opps() to fail if all required opps tables aren't
> > > >   linked.
> > >
> > > We are already in the middle of a discussion for your previous version
> > > and I haven't said yet that I am happy with what you suggested just 2
> > > days back. Why send another version so soon ?
> >
> > I wanted you to see how I addressed your comments.
>
> Sure, but that is just half the comments.
>
> > It didn't look like
> > you were going to make more comments on the code.
>
> I posted some queries and you posted your opinions on them. Now
> shouldn't I get a chance to reply again to see if I agree with your
> replies or if we can settle to something else ? I only got one day in
> between where I was busy with other stuff and so couldn't come back to
> it. Please wait a little longer specially when the comments aren't
> minor in nature.

Sorry if it came off as trying to rush you. That wasn't the intention.
Just some misunderstanding on my part.

> Anyway, lets get over it now. Lets continue our discussion on V3 and
> then we can have a V5 :)
>
> Have a good day Saravana.

Sounds good. You too Viresh! :)

-Saravana

Re: [PATCH v4 0/5] Add required-opps support to devfreq passive gov

[Chanwoo Choi]

Dear Saravana,

Any other progress of this series?

Regards,
Chanwoo Choi

On 7/24/19 10:42 AM, Saravana Kannan wrote:
> The devfreq passive governor scales the frequency of a "child" device based
> on the current frequency of a "parent" device (not parent/child in the
> sense of device hierarchy). As of today, the passive governor requires one
> of the following to work correctly:
> 1. The parent and child device have the same number of frequencies
> 2. The child device driver passes a mapping function to translate from
>    parent frequency to child frequency.
> 
> When (1) is not true, (2) is the only option right now. But often times,
> all that is required is a simple mapping from parent's frequency to child's
> frequency.
> 
> Since OPPs already support pointing to other "required-opps", add support
> for using that to map from parent device frequency to child device
> frequency. That way, every child device driver doesn't have to implement a
> separate mapping function anytime (1) isn't true.
> 
> Some common (but not comprehensive) reason for needing a devfreq passive
> governor to adjust the frequency of one device based on another are:
> 
> 1. These were the combination of frequencies that were validated/screened
>    during the manufacturing process.
> 2. These are the sensible performance combinations between two devices
>    interacting with each other. So that when one runs fast the other
>    doesn't become the bottleneck.
> 3. Hardware bugs requiring some kind of frequency ratio between devices.
> 
> For example, the following mapping can't be captured in DT as it stands
> today because the parent and child device have different number of OPPs.
> But with this patch series, this mapping can be captured cleanly.
> 
> In arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi you have something
> like this with the following changes:
> 
> 	bus_g2d_400: bus0 {
> 		compatible = "samsung,exynos-bus";
> 		clocks = <&cmu_top CLK_ACLK_G2D_400>;
> 		clock-names = "bus";
> 		operating-points-v2 = <&bus_g2d_400_opp_table>;
> 		status = "disabled";
> 	};
> 
> 	bus_noc2: bus9 {
> 		compatible = "samsung,exynos-bus";
> 		clocks = <&cmu_mif CLK_ACLK_BUS2_400>;
> 		clock-names = "bus";
> 		operating-points-v2 = <&bus_noc2_opp_table>;
> 		status = "disabled";
> 	};
> 
> 	bus_g2d_400_opp_table: opp_table2 {
> 		compatible = "operating-points-v2";
> 		opp-shared;
> 
> 		opp-400000000 {
> 			opp-hz = /bits/ 64 <400000000>;
> 			opp-microvolt = <1075000>;
> 			required-opps = <&noc2_400>;
> 		};
> 		opp-267000000 {
> 			opp-hz = /bits/ 64 <267000000>;
> 			opp-microvolt = <1000000>;
> 			required-opps = <&noc2_200>;
> 		};
> 		opp-200000000 {
> 			opp-hz = /bits/ 64 <200000000>;
> 			opp-microvolt = <975000>;
> 			required-opps = <&noc2_200>;
> 		};
> 		opp-160000000 {
> 			opp-hz = /bits/ 64 <160000000>;
> 			opp-microvolt = <962500>;
> 			required-opps = <&noc2_134>;
> 		};
> 		opp-134000000 {
> 			opp-hz = /bits/ 64 <134000000>;
> 			opp-microvolt = <950000>;
> 			required-opps = <&noc2_134>;
> 		};
> 		opp-100000000 {
> 			opp-hz = /bits/ 64 <100000000>;
> 			opp-microvolt = <937500>;
> 			required-opps = <&noc2_100>;
> 		};
> 	};
> 
> 	bus_noc2_opp_table: opp_table6 {
> 		compatible = "operating-points-v2";
> 
> 		noc2_400: opp-400000000 {
> 			opp-hz = /bits/ 64 <400000000>;
> 		};
> 		noc2_200: opp-200000000 {
> 			opp-hz = /bits/ 64 <200000000>;
> 		};
> 		noc2_134: opp-134000000 {
> 			opp-hz = /bits/ 64 <134000000>;
> 		};
> 		noc2_100: opp-100000000 {
> 			opp-hz = /bits/ 64 <100000000>;
> 		};
> 	};
> 
> -Saravana
> 
> v3 -> v4:
> - Fixed documentation comments
> - Fixed order of functions in .h file
> - Renamed the new xlate API
> - Caused _set_required_opps() to fail if all required opps tables aren't
>   linked.
> v2 -> v3:
> - Rebased onto linux-next.
> - Added documentation comment for new fields.
> - Added support for lazy required-opps linking.
> - Updated Ack/Reviewed-bys.
> v1 -> v2:
> - Cached OPP table reference in devfreq to avoid looking up every time.
> - Renamed variable in passive governor to be more intuitive.
> - Updated cover letter with examples.
> 
> 
> Saravana Kannan (5):
>   OPP: Allow required-opps even if the device doesn't have power-domains
>   OPP: Add function to look up required OPP's for a given OPP
>   OPP: Improve required-opps linking
>   PM / devfreq: Cache OPP table reference in devfreq
>   PM / devfreq: Add required OPPs support to passive governor
> 
>  drivers/devfreq/devfreq.c          |   6 ++
>  drivers/devfreq/governor_passive.c |  20 ++++--
>  drivers/opp/core.c                 |  83 +++++++++++++++++++---
>  drivers/opp/of.c                   | 108 ++++++++++++++---------------
>  drivers/opp/opp.h                  |   5 ++
>  include/linux/devfreq.h            |   2 +
>  include/linux/pm_opp.h             |  11 +++
>  7 files changed, 165 insertions(+), 70 deletions(-)
> 

Re: [PATCH v4 0/5] Add required-opps support to devfreq passive gov

[Saravana Kannan]

On Wed, Nov 13, 2019 at 11:48 PM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>
> Dear Saravana,
>
> Any other progress of this series?

Hi Chanwoo,

Thanks for checking. I haven't abandoned this patch series. This patch
series depends on "lazy linking" of required-opps to avoid a cyclic
dependency between devfreq and OPP table population. But Viresh wasn't
happy with my implementation of the lazy liking for reasonable
reasons.

I had a chat with Viresh during one of the several conferences that I
met him at. To fix the lazy linking in the way he wanted it meant we
had to fix other issues in the OPP framework that arise when OPP
tables are shared in DT but not in memory. So he was kind enough to
sign up to add lazy linking support to OPPs so that I won't have to do
it. So, I'm waiting on that. So once that's added, I should be able to
drop a few patches in this series, do some minor updates and then this
will be good to go.

Thanks,
Saravana

Re: [PATCH v4 0/5] Add required-opps support to devfreq passive gov

[Viresh Kumar]

On 14-11-19, 00:23, Saravana Kannan wrote:
> Thanks for checking. I haven't abandoned this patch series. This patch
> series depends on "lazy linking" of required-opps to avoid a cyclic
> dependency between devfreq and OPP table population. But Viresh wasn't
> happy with my implementation of the lazy liking for reasonable
> reasons.
> 
> I had a chat with Viresh during one of the several conferences that I
> met him at. To fix the lazy linking in the way he wanted it meant we
> had to fix other issues in the OPP framework that arise when OPP
> tables are shared in DT but not in memory. So he was kind enough to
> sign up to add lazy linking support to OPPs so that I won't have to do
> it. So, I'm waiting on that. So once that's added, I should be able to
> drop a few patches in this series, do some minor updates and then this
> will be good to go.

I am fixing few other issues in OPP core right now and lazy linking
is next in the list :)

-- 
viresh

Re: [PATCH v4 0/5] Add required-opps support to devfreq passive gov

[Saravana Kannan]

On Thu, Nov 14, 2019 at 12:35 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 14-11-19, 00:23, Saravana Kannan wrote:
> > Thanks for checking. I haven't abandoned this patch series. This patch
> > series depends on "lazy linking" of required-opps to avoid a cyclic
> > dependency between devfreq and OPP table population. But Viresh wasn't
> > happy with my implementation of the lazy liking for reasonable
> > reasons.
> >
> > I had a chat with Viresh during one of the several conferences that I
> > met him at. To fix the lazy linking in the way he wanted it meant we
> > had to fix other issues in the OPP framework that arise when OPP
> > tables are shared in DT but not in memory. So he was kind enough to
> > sign up to add lazy linking support to OPPs so that I won't have to do
> > it. So, I'm waiting on that. So once that's added, I should be able to
> > drop a few patches in this series, do some minor updates and then this
> > will be good to go.
>
> I am fixing few other issues in OPP core right now and lazy linking
> is next in the list :)

Thanks Viresh! Glad the offer still stands :)

-Saravana

Re: [PATCH v4 0/5] Add required-opps support to devfreq passive gov

[Chanwoo Choi]

Hi Saravana,

On 11/14/19 5:23 PM, Saravana Kannan wrote:
> On Wed, Nov 13, 2019 at 11:48 PM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>
>> Dear Saravana,
>>
>> Any other progress of this series?
> 
> Hi Chanwoo,
> 
> Thanks for checking. I haven't abandoned this patch series. This patch
> series depends on "lazy linking" of required-opps to avoid a cyclic
> dependency between devfreq and OPP table population. But Viresh wasn't
> happy with my implementation of the lazy liking for reasonable
> reasons.
> 
> I had a chat with Viresh during one of the several conferences that I
> met him at. To fix the lazy linking in the way he wanted it meant we
> had to fix other issues in the OPP framework that arise when OPP
> tables are shared in DT but not in memory. So he was kind enough to
> sign up to add lazy linking support to OPPs so that I won't have to do
> it. So, I'm waiting on that. So once that's added, I should be able to
> drop a few patches in this series, do some minor updates and then this
> will be good to go.

Thanks for the detailed explanation. I'll expect the your next version.

Thanks,
Chanwoo Choi

Re: [PATCH v4 0/5] Add required-opps support to devfreq passive gov

[Chanwoo Choi]

Hi Saravana,

2019년 11월 14일 (목) 오후 5:36, Chanwoo Choi <cw00.choi@samsung.com>님이 작성:
>
> Hi Saravana,
>
> On 11/14/19 5:23 PM, Saravana Kannan wrote:
> > On Wed, Nov 13, 2019 at 11:48 PM Chanwoo Choi <cw00.choi@samsung.com> wrote:
> >>
> >> Dear Saravana,
> >>
> >> Any other progress of this series?
> >
> > Hi Chanwoo,
> >
> > Thanks for checking. I haven't abandoned this patch series. This patch
> > series depends on "lazy linking" of required-opps to avoid a cyclic
> > dependency between devfreq and OPP table population. But Viresh wasn't
> > happy with my implementation of the lazy liking for reasonable
> > reasons.
> >
> > I had a chat with Viresh during one of the several conferences that I
> > met him at. To fix the lazy linking in the way he wanted it meant we
> > had to fix other issues in the OPP framework that arise when OPP
> > tables are shared in DT but not in memory. So he was kind enough to
> > sign up to add lazy linking support to OPPs so that I won't have to do
> > it. So, I'm waiting on that. So once that's added, I should be able to
> > drop a few patches in this series, do some minor updates and then this
> > will be good to go.
>
> Thanks for the detailed explanation. I'll expect the your next version.

As I know, the lazy linking issue was fixed by Viresh.
If possible, I want to know your plan about 'required-opp' with
passive governor.
Because I think that the patch[1] is good for devfreq device.

As you mentioned, you have the other idea to implement the 'cpu based
scaling support'
to passive governor without cpu notifier. Actually, if there are no
any other best solution,
I prefer to use cpu notifier for 'cpu based scaling support to
passive_governor'.

[1] https://patchwork.kernel.org/patch/11046147/
- [RFC,v2] PM / devfreq: Add cpu based scaling support to passive_governor

-- 
Best Regards,
Chanwoo Choi