[PATCH V2 00/13] OPP: Add support for multiple clocks*

[PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

Hello,

This patchset adds support for devices with multiple clocks. None of the clocks
is considered primary in this case and all are handled equally.

The drivers, for multiple clock case, are expected to call dev_pm_opp_set_opp()
to set the specific OPP. Though how they find the target OPP is left for the
users to handle. For some, we may have another unique OPP property, like level,
which can be used to find the OPP. While in case of others, we may want to
implement freq-based OPP finder APIs for multiple clock rates. I have decided
not to implement them in advance, and add them only someone wants to use them.

This is rebased over a lot of other OPP changes and is pushed here:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

V1->V2:

- Fix broken git bisect for:
  OPP: Reuse _opp_compare_key() in _opp_add_static_v2()

- Include binding changes written by Krzysztof earlier.

- Check config_clks before calling it, it isn't always set.

- Add config_clks for Tegra30's devfreq to handle its corner case.

- _opp_compare_key() supports multi-clk case now, earlier it skipped freq
  comparison for such a case.

- New patch to compare all bandwidth values as well in _opp_compare_key().

- New patch to remove *_noclk() interface.

- Various other minor fixes.

--
Viresh

Krzysztof Kozlowski (1):
  dt-bindings: opp: accept array of frequencies

Viresh Kumar (12):
  OPP: Use consistent names for OPP table instances
  OPP: Remove rate_not_available parameter to _opp_add()
  OPP: Reuse _opp_compare_key() in _opp_add_static_v2()
  OPP: Make dev_pm_opp_set_opp() independent of frequency
  OPP: Allow multiple clocks for a device
  OPP: Compare bandwidths for all paths in _opp_compare_key()
  OPP: Add key specific assert() method to key finding helpers
  OPP: Assert clk_count == 1 for single clk helpers
  OPP: Provide a simple implementation to configure multiple clocks
  OPP: Allow config_clks helper for single clk case
  PM / devfreq: tegra30: Register config_clks helper
  OPP: Remove dev{m}_pm_opp_of_add_table_noclk()

 .../devicetree/bindings/opp/opp-v2-base.yaml  |  10 +
 drivers/devfreq/tegra30-devfreq.c             |  22 +-
 drivers/opp/core.c                            | 404 +++++++++++++-----
 drivers/opp/cpu.c                             |  12 +-
 drivers/opp/debugfs.c                         |  27 +-
 drivers/opp/of.c                              | 139 +++---
 drivers/opp/opp.h                             |  24 +-
 include/linux/pm_opp.h                        |  29 +-
 8 files changed, 466 insertions(+), 201 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[PATCH V2 01/13] OPP: Use consistent names for OPP table instances

[Viresh Kumar]

The OPP table is called "opp_table" at most of the places and "table" at
few. Make all of them follow the same naming convention, "opp_table".

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c |  7 ++++---
 drivers/opp/cpu.c  | 12 ++++++------
 drivers/opp/of.c   | 12 ++++++------
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 5ad43dbfd87f..e836d3043d22 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1585,15 +1585,16 @@ void dev_pm_opp_remove_all_dynamic(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_remove_all_dynamic);
 
-struct dev_pm_opp *_opp_allocate(struct opp_table *table)
+struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table)
 {
 	struct dev_pm_opp *opp;
 	int supply_count, supply_size, icc_size;
 
 	/* Allocate space for at least one supply */
-	supply_count = table->regulator_count > 0 ? table->regulator_count : 1;
+	supply_count = opp_table->regulator_count > 0 ?
+			opp_table->regulator_count : 1;
 	supply_size = sizeof(*opp->supplies) * supply_count;
-	icc_size = sizeof(*opp->bandwidth) * table->path_count;
+	icc_size = sizeof(*opp->bandwidth) * opp_table->path_count;
 
 	/* allocate new OPP node and supplies structures */
 	opp = kzalloc(sizeof(*opp) + supply_size + icc_size, GFP_KERNEL);
diff --git a/drivers/opp/cpu.c b/drivers/opp/cpu.c
index 5004335cf0de..3c3506021501 100644
--- a/drivers/opp/cpu.c
+++ b/drivers/opp/cpu.c
@@ -41,7 +41,7 @@
  * the table if any of the mentioned functions have been invoked in the interim.
  */
 int dev_pm_opp_init_cpufreq_table(struct device *dev,
-				  struct cpufreq_frequency_table **table)
+				  struct cpufreq_frequency_table **opp_table)
 {
 	struct dev_pm_opp *opp;
 	struct cpufreq_frequency_table *freq_table = NULL;
@@ -76,7 +76,7 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev,
 	freq_table[i].driver_data = i;
 	freq_table[i].frequency = CPUFREQ_TABLE_END;
 
-	*table = &freq_table[0];
+	*opp_table = &freq_table[0];
 
 out:
 	if (ret)
@@ -94,13 +94,13 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_init_cpufreq_table);
  * Free up the table allocated by dev_pm_opp_init_cpufreq_table
  */
 void dev_pm_opp_free_cpufreq_table(struct device *dev,
-				   struct cpufreq_frequency_table **table)
+				   struct cpufreq_frequency_table **opp_table)
 {
-	if (!table)
+	if (!opp_table)
 		return;
 
-	kfree(*table);
-	*table = NULL;
+	kfree(*opp_table);
+	*opp_table = NULL;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_free_cpufreq_table);
 #endif	/* CONFIG_CPU_FREQ */
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 30394929d700..e07fc31de416 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -767,7 +767,7 @@ void dev_pm_opp_of_remove_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
 
-static int _read_bw(struct dev_pm_opp *new_opp, struct opp_table *table,
+static int _read_bw(struct dev_pm_opp *new_opp, struct opp_table *opp_table,
 		    struct device_node *np, bool peak)
 {
 	const char *name = peak ? "opp-peak-kBps" : "opp-avg-kBps";
@@ -780,9 +780,9 @@ static int _read_bw(struct dev_pm_opp *new_opp, struct opp_table *table,
 		return -ENODEV;
 
 	count = prop->length / sizeof(u32);
-	if (table->path_count != count) {
+	if (opp_table->path_count != count) {
 		pr_err("%s: Mismatch between %s and paths (%d %d)\n",
-				__func__, name, count, table->path_count);
+				__func__, name, count, opp_table->path_count);
 		return -EINVAL;
 	}
 
@@ -808,7 +808,7 @@ static int _read_bw(struct dev_pm_opp *new_opp, struct opp_table *table,
 	return ret;
 }
 
-static int _read_opp_key(struct dev_pm_opp *new_opp, struct opp_table *table,
+static int _read_opp_key(struct dev_pm_opp *new_opp, struct opp_table *opp_table,
 			 struct device_node *np, bool *rate_not_available)
 {
 	bool found = false;
@@ -832,10 +832,10 @@ static int _read_opp_key(struct dev_pm_opp *new_opp, struct opp_table *table,
 	 * opp-peak-kBps = <path1_value path2_value>;
 	 * opp-avg-kBps = <path1_value path2_value>;
 	 */
-	ret = _read_bw(new_opp, table, np, true);
+	ret = _read_bw(new_opp, opp_table, np, true);
 	if (!ret) {
 		found = true;
-		ret = _read_bw(new_opp, table, np, false);
+		ret = _read_bw(new_opp, opp_table, np, false);
 	}
 
 	/* The properties were found but we failed to parse them */
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 02/13] OPP: Remove rate_not_available parameter to _opp_add()

[Viresh Kumar]

commit 32715be4fe95 ("opp: Fix adding OPP entries in a wrong order if
rate is unavailable") removed the only user of this field, get rid of
rest of it now.

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c |  4 ++--
 drivers/opp/of.c   | 10 ++++------
 drivers/opp/opp.h  |  2 +-
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e836d3043d22..ae5949656d77 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1713,7 +1713,7 @@ void _required_opps_available(struct dev_pm_opp *opp, int count)
  *  should be considered an error by the callers of _opp_add().
  */
 int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
-	     struct opp_table *opp_table, bool rate_not_available)
+	     struct opp_table *opp_table)
 {
 	struct list_head *head;
 	int ret;
@@ -1792,7 +1792,7 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
 	new_opp->available = true;
 	new_opp->dynamic = dynamic;
 
-	ret = _opp_add(dev, new_opp, opp_table, false);
+	ret = _opp_add(dev, new_opp, opp_table);
 	if (ret) {
 		/* Don't return error for duplicate OPPs */
 		if (ret == -EBUSY)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index e07fc31de416..bec9644a7260 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -808,8 +808,8 @@ static int _read_bw(struct dev_pm_opp *new_opp, struct opp_table *opp_table,
 	return ret;
 }
 
-static int _read_opp_key(struct dev_pm_opp *new_opp, struct opp_table *opp_table,
-			 struct device_node *np, bool *rate_not_available)
+static int _read_opp_key(struct dev_pm_opp *new_opp,
+			 struct opp_table *opp_table, struct device_node *np)
 {
 	bool found = false;
 	u64 rate;
@@ -825,7 +825,6 @@ static int _read_opp_key(struct dev_pm_opp *new_opp, struct opp_table *opp_table
 		new_opp->rate = (unsigned long)rate;
 		found = true;
 	}
-	*rate_not_available = !!ret;
 
 	/*
 	 * Bandwidth consists of peak and average (optional) values:
@@ -881,13 +880,12 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	struct dev_pm_opp *new_opp;
 	u32 val;
 	int ret;
-	bool rate_not_available = false;
 
 	new_opp = _opp_allocate(opp_table);
 	if (!new_opp)
 		return ERR_PTR(-ENOMEM);
 
-	ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
+	ret = _read_opp_key(new_opp, opp_table, np);
 	if (ret < 0) {
 		dev_err(dev, "%s: opp key field not found\n", __func__);
 		goto free_opp;
@@ -920,7 +918,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	if (opp_table->is_genpd)
 		new_opp->pstate = pm_genpd_opp_to_performance_state(dev, new_opp);
 
-	ret = _opp_add(dev, new_opp, opp_table, rate_not_available);
+	ret = _opp_add(dev, new_opp, opp_table);
 	if (ret) {
 		/* Don't return error for duplicate OPPs */
 		if (ret == -EBUSY)
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 13abe991e811..e449828ffbf4 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -247,7 +247,7 @@ struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_
 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(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table);
 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);
 struct opp_table *_add_opp_table_indexed(struct device *dev, int index, bool getclk);
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 03/13] OPP: Reuse _opp_compare_key() in _opp_add_static_v2()

[Viresh Kumar]

Reuse _opp_compare_key() in _opp_add_static_v2() instead of just
comparing frequency while finding suspend frequency. Also add a comment
over _opp_compare_key() explaining its return values.

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 6 ++++++
 drivers/opp/of.c   | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index ae5949656d77..00d5911b20f8 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1636,6 +1636,12 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 	return true;
 }
 
+/*
+ * Returns
+ * 0: opp1 == opp2
+ * 1: opp1 > opp2
+ * -1: opp1 < opp2
+ */
 int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
 {
 	if (opp1->rate != opp2->rate)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index bec9644a7260..bb49057cb1fc 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -929,8 +929,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	/* OPP to select on device suspend */
 	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) {
+			/* Pick the OPP with higher rate/bw/level as suspend OPP */
+			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;
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 04/13] OPP: Make dev_pm_opp_set_opp() independent of frequency

[Viresh Kumar]

dev_pm_opp_set_opp() can be called for any device, it may or may not
have a frequency value associated with it.

If a frequency value isn't available, we pass 0 to _set_opp(). Make it
optional instead by making _set_opp() accept a pointer instead, as the
frequency value is anyway available in the OPP. This makes
dev_pm_opp_set_opp() and _set_opp() completely independent of any
special key value.

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 52 +++++++++++++++++++++++++++++++++-------------
 drivers/opp/opp.h  |  4 ++--
 2 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 00d5911b20f8..daabc810a1f9 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -784,19 +784,33 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 	return ret;
 }
 
-static inline int _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
-					    unsigned long freq)
+static inline int _generic_set_opp_clk_only(struct device *dev,
+		struct opp_table *opp_table, struct dev_pm_opp *opp, void *data)
 {
+	unsigned long *target = data;
+	unsigned long freq;
 	int ret;
 
 	/* We may reach here for devices which don't change frequency */
-	if (IS_ERR(clk))
+	if (IS_ERR(opp_table->clk))
 		return 0;
 
-	ret = clk_set_rate(clk, freq);
+	/* One of target and opp must be available */
+	if (target) {
+		freq = *target;
+	} else if (opp) {
+		freq = opp->rate;
+	} else {
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	ret = clk_set_rate(opp_table->clk, freq);
 	if (ret) {
 		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
 			ret);
+	} else {
+		opp_table->rate_clk_single = freq;
 	}
 
 	return ret;
@@ -990,7 +1004,7 @@ static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
 }
 
 static int _set_opp(struct device *dev, struct opp_table *opp_table,
-		    struct dev_pm_opp *opp, unsigned long freq)
+		    struct dev_pm_opp *opp, void *clk_data, bool forced)
 {
 	struct dev_pm_opp *old_opp;
 	int scaling_down, ret;
@@ -1005,15 +1019,14 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	old_opp = opp_table->current_opp;
 
 	/* Return early if nothing to do */
-	if (old_opp == opp && opp_table->current_rate == freq &&
-	    opp_table->enabled) {
+	if (!forced && old_opp == opp && opp_table->enabled) {
 		dev_dbg(dev, "%s: OPPs are same, nothing to do\n", __func__);
 		return 0;
 	}
 
 	dev_dbg(dev, "%s: switching OPP: Freq %lu -> %lu Hz, Level %u -> %u, Bw %u -> %u\n",
-		__func__, opp_table->current_rate, freq, old_opp->level,
-		opp->level, old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
+		__func__, old_opp->rate, opp->rate, old_opp->level, opp->level,
+		old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
 		opp->bandwidth ? opp->bandwidth[0].peak : 0);
 
 	scaling_down = _opp_compare_key(old_opp, opp);
@@ -1046,7 +1059,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		}
 	}
 
-	ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
+	ret = _generic_set_opp_clk_only(dev, opp_table, opp, clk_data);
 	if (ret)
 		return ret;
 
@@ -1082,7 +1095,6 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	/* Make sure current_opp doesn't get freed */
 	dev_pm_opp_get(opp);
 	opp_table->current_opp = opp;
-	opp_table->current_rate = freq;
 
 	return ret;
 }
@@ -1103,6 +1115,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	struct opp_table *opp_table;
 	unsigned long freq = 0, temp_freq;
 	struct dev_pm_opp *opp = NULL;
+	bool forced = false;
 	int ret;
 
 	opp_table = _find_opp_table(dev);
@@ -1120,7 +1133,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		 * equivalent to a clk_set_rate()
 		 */
 		if (!_get_opp_count(opp_table)) {
-			ret = _generic_set_opp_clk_only(dev, opp_table->clk, target_freq);
+			ret = _generic_set_opp_clk_only(dev, opp_table, NULL,
+							&target_freq);
 			goto put_opp_table;
 		}
 
@@ -1141,12 +1155,22 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 				__func__, freq, ret);
 			goto put_opp_table;
 		}
+
+		/*
+		 * An OPP entry specifies the highest frequency at which other
+		 * properties of the OPP entry apply. Even if the new OPP is
+		 * same as the old one, we may still reach here for a different
+		 * value of the frequency. In such a case, do not abort but
+		 * configure the hardware to the desired frequency forcefully.
+		 */
+		forced = opp_table->rate_clk_single != target_freq;
 	}
 
-	ret = _set_opp(dev, opp_table, opp, freq);
+	ret = _set_opp(dev, opp_table, opp, &target_freq, forced);
 
 	if (target_freq)
 		dev_pm_opp_put(opp);
+
 put_opp_table:
 	dev_pm_opp_put_opp_table(opp_table);
 	return ret;
@@ -1174,7 +1198,7 @@ int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
 		return PTR_ERR(opp_table);
 	}
 
-	ret = _set_opp(dev, opp_table, opp, opp ? opp->rate : 0);
+	ret = _set_opp(dev, opp_table, opp, NULL, false);
 	dev_pm_opp_put_opp_table(opp_table);
 
 	return ret;
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index e449828ffbf4..e481bdb59499 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -159,7 +159,7 @@ enum opp_table_access {
  * @clock_latency_ns_max: Max clock latency in nanoseconds.
  * @parsed_static_opps: Count of devices for which OPPs are initialized from DT.
  * @shared_opp: OPP is shared between multiple devices.
- * @current_rate: Currently configured frequency.
+ * @rate_clk_single: Currently configured frequency for single clk.
  * @current_opp: Currently configured OPP for the table.
  * @suspend_opp: Pointer to OPP to be used during device suspend.
  * @genpd_virt_dev_lock: Mutex protecting the genpd virtual device pointers.
@@ -208,7 +208,7 @@ struct opp_table {
 
 	unsigned int parsed_static_opps;
 	enum opp_table_access shared_opp;
-	unsigned long current_rate;
+	unsigned long rate_clk_single;
 	struct dev_pm_opp *current_opp;
 	struct dev_pm_opp *suspend_opp;
 
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 05/13] dt-bindings: opp: accept array of frequencies

[Viresh Kumar]

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Devices might need to control several clocks when scaling the frequency
and voltage.  Allow passing array of clock frequencies, similarly to the
voltages.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
index 76c8acd981b3..66d0ec763f0b 100644
--- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
+++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
@@ -50,6 +50,16 @@ select: false
           property to uniquely identify the OPP nodes exists. Devices like power
           domains must have another (implementation dependent) property.
 
+          Entries for multiple clocks shall be provided in the same field, as
+          array of frequencies.  The OPP binding doesn't provide any provisions
+          to relate the values to their clocks or the order in which the clocks
+          need to be configured and that is left for the implementation
+          specific binding.
+        minItems: 1
+        maxItems: 16
+        items:
+          maxItems: 1
+
       opp-microvolt:
         description: |
           Voltage for the OPP
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 06/13] OPP: Allow multiple clocks for a device

[Viresh Kumar]

This patch adds support to allow multiple clocks for a device.

The design is pretty much similar to how this is done for regulators,
and platforms can supply their own version of the config_clks() callback
if they have multiple clocks for their device. The core manages the
calls via opp_table->config_clks() eventually.

We have kept both "clk" and "clks" fields in the OPP table structure and
the reason is provided as a comment in _opp_set_clknames(). The same
isn't done for "rates" though and we use rates[0] at most of the places
now.

Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Tested-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 199 +++++++++++++++++++++++++++++------------
 drivers/opp/debugfs.c  |  27 +++++-
 drivers/opp/of.c       |  69 +++++++++++---
 drivers/opp/opp.h      |  16 ++--
 include/linux/pm_opp.h |   7 +-
 5 files changed, 235 insertions(+), 83 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index daabc810a1f9..003cd48123d7 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -181,7 +181,7 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
 		return 0;
 	}
 
-	return opp->rate;
+	return opp->rates[0];
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
 
@@ -430,7 +430,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_count);
 /* Helpers to read keys */
 static unsigned long _read_freq(struct dev_pm_opp *opp, int index)
 {
-	return opp->rate;
+	return opp->rates[0];
 }
 
 static unsigned long _read_level(struct dev_pm_opp *opp, int index)
@@ -784,22 +784,19 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 	return ret;
 }
 
-static inline int _generic_set_opp_clk_only(struct device *dev,
-		struct opp_table *opp_table, struct dev_pm_opp *opp, void *data)
+static int
+_opp_config_clk_single(struct device *dev, struct opp_table *opp_table,
+		       struct dev_pm_opp *opp, void *data, bool scaling_down)
 {
 	unsigned long *target = data;
 	unsigned long freq;
 	int ret;
 
-	/* We may reach here for devices which don't change frequency */
-	if (IS_ERR(opp_table->clk))
-		return 0;
-
 	/* One of target and opp must be available */
 	if (target) {
 		freq = *target;
 	} else if (opp) {
-		freq = opp->rate;
+		freq = opp->rates[0];
 	} else {
 		WARN_ON(1);
 		return -EINVAL;
@@ -1025,11 +1022,11 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	}
 
 	dev_dbg(dev, "%s: switching OPP: Freq %lu -> %lu Hz, Level %u -> %u, Bw %u -> %u\n",
-		__func__, old_opp->rate, opp->rate, old_opp->level, opp->level,
-		old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
+		__func__, old_opp->rates[0], opp->rates[0], old_opp->level,
+		opp->level, old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
 		opp->bandwidth ? opp->bandwidth[0].peak : 0);
 
-	scaling_down = _opp_compare_key(old_opp, opp);
+	scaling_down = _opp_compare_key(opp_table, old_opp, opp);
 	if (scaling_down == -1)
 		scaling_down = 0;
 
@@ -1059,9 +1056,11 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		}
 	}
 
-	ret = _generic_set_opp_clk_only(dev, opp_table, opp, clk_data);
-	if (ret)
-		return ret;
+	if (opp_table->config_clks) {
+		ret = opp_table->config_clks(dev, opp_table, opp, clk_data, scaling_down);
+		if (ret)
+			return ret;
+	}
 
 	/* Scaling down? Configure required OPPs after frequency */
 	if (scaling_down) {
@@ -1133,8 +1132,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		 * equivalent to a clk_set_rate()
 		 */
 		if (!_get_opp_count(opp_table)) {
-			ret = _generic_set_opp_clk_only(dev, opp_table, NULL,
-							&target_freq);
+			ret = opp_table->config_clks(dev, opp_table, NULL,
+						     &target_freq, false);
 			goto put_opp_table;
 		}
 
@@ -1255,6 +1254,8 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 	INIT_LIST_HEAD(&opp_table->dev_list);
 	INIT_LIST_HEAD(&opp_table->lazy);
 
+	opp_table->clk = ERR_PTR(-ENODEV);
+
 	/* Mark regulator count uninitialized */
 	opp_table->regulator_count = -1;
 
@@ -1301,18 +1302,22 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
 	int ret;
 
 	/*
-	 * Return early if we don't need to get clk or we have already tried it
+	 * Return early if we don't need to get clk or we have already done it
 	 * earlier.
 	 */
-	if (!getclk || IS_ERR(opp_table) || opp_table->clk)
+	if (!getclk || IS_ERR(opp_table) || !IS_ERR(opp_table->clk) ||
+	    opp_table->clks)
 		return opp_table;
 
 	/* Find clk for the device */
 	opp_table->clk = clk_get(dev, NULL);
 
 	ret = PTR_ERR_OR_ZERO(opp_table->clk);
-	if (!ret)
+	if (!ret) {
+		opp_table->config_clks = _opp_config_clk_single;
+		opp_table->clk_count = 1;
 		return opp_table;
+	}
 
 	if (ret == -ENOENT) {
 		dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
@@ -1417,7 +1422,7 @@ static void _opp_table_kref_release(struct kref *kref)
 
 	_of_clear_opp_table(opp_table);
 
-	/* Release clk */
+	/* Release automatically acquired single clk */
 	if (!IS_ERR(opp_table->clk))
 		clk_put(opp_table->clk);
 
@@ -1505,7 +1510,7 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq)
 	mutex_lock(&opp_table->lock);
 
 	list_for_each_entry(iter, &opp_table->opp_list, node) {
-		if (iter->rate == freq) {
+		if (iter->rates[0] == freq) {
 			opp = iter;
 			break;
 		}
@@ -1612,24 +1617,28 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove_all_dynamic);
 struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table)
 {
 	struct dev_pm_opp *opp;
-	int supply_count, supply_size, icc_size;
+	int supply_count, supply_size, icc_size, clk_size;
 
 	/* Allocate space for at least one supply */
 	supply_count = opp_table->regulator_count > 0 ?
 			opp_table->regulator_count : 1;
 	supply_size = sizeof(*opp->supplies) * supply_count;
+	clk_size = sizeof(*opp->rates) * opp_table->clk_count;
 	icc_size = sizeof(*opp->bandwidth) * opp_table->path_count;
 
 	/* allocate new OPP node and supplies structures */
-	opp = kzalloc(sizeof(*opp) + supply_size + icc_size, GFP_KERNEL);
-
+	opp = kzalloc(sizeof(*opp) + supply_size + clk_size + icc_size, GFP_KERNEL);
 	if (!opp)
 		return NULL;
 
-	/* Put the supplies at the end of the OPP structure as an empty array */
+	/* Put the supplies, bw and clock at the end of the OPP structure */
 	opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
+
+	opp->rates = (unsigned long *)(opp->supplies + supply_count);
+
 	if (icc_size)
-		opp->bandwidth = (struct dev_pm_opp_icc_bw *)(opp->supplies + supply_count);
+		opp->bandwidth = (struct dev_pm_opp_icc_bw *)(opp->rates + opp_table->clk_count);
+
 	INIT_LIST_HEAD(&opp->node);
 
 	return opp;
@@ -1660,21 +1669,43 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 	return true;
 }
 
+static int _opp_compare_rate(struct opp_table *opp_table,
+			     struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
+{
+	int i;
+
+	for (i = 0; i < opp_table->clk_count; i++) {
+		if (opp1->rates[i] != opp2->rates[i])
+			return opp1->rates[i] < opp2->rates[i] ? -1 : 1;
+	}
+
+	/* Same rates for both OPPs */
+	return 0;
+}
+
 /*
  * Returns
  * 0: opp1 == opp2
  * 1: opp1 > opp2
  * -1: opp1 < opp2
  */
-int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
+int _opp_compare_key(struct opp_table *opp_table, struct dev_pm_opp *opp1,
+		     struct dev_pm_opp *opp2)
 {
-	if (opp1->rate != opp2->rate)
-		return opp1->rate < opp2->rate ? -1 : 1;
+	int ret;
+
+	ret = _opp_compare_rate(opp_table, opp1, opp2);
+	if (ret)
+		return ret;
+
 	if (opp1->bandwidth && opp2->bandwidth &&
 	    opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
 		return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1;
+
 	if (opp1->level != opp2->level)
 		return opp1->level < opp2->level ? -1 : 1;
+
+	/* Duplicate OPPs */
 	return 0;
 }
 
@@ -1694,7 +1725,7 @@ 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) {
-		opp_cmp = _opp_compare_key(new_opp, opp);
+		opp_cmp = _opp_compare_key(opp_table, new_opp, opp);
 		if (opp_cmp > 0) {
 			*head = &opp->node;
 			continue;
@@ -1705,8 +1736,8 @@ static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,
 
 		/* Duplicate OPPs */
 		dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
-			 __func__, opp->rate, opp->supplies[0].u_volt,
-			 opp->available, new_opp->rate,
+			 __func__, opp->rates[0], opp->supplies[0].u_volt,
+			 opp->available, new_opp->rates[0],
 			 new_opp->supplies[0].u_volt, new_opp->available);
 
 		/* Should we compare voltages for all regulators here ? */
@@ -1727,7 +1758,7 @@ void _required_opps_available(struct dev_pm_opp *opp, int count)
 
 		opp->available = false;
 		pr_warn("%s: OPP not supported by required OPP %pOF (%lu)\n",
-			 __func__, opp->required_opps[i]->np, opp->rate);
+			 __func__, opp->required_opps[i]->np, opp->rates[0]);
 		return;
 	}
 }
@@ -1768,7 +1799,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 	if (!_opp_supported_by_regulators(new_opp, opp_table)) {
 		new_opp->available = false;
 		dev_warn(dev, "%s: OPP not supported by regulators (%lu)\n",
-			 __func__, new_opp->rate);
+			 __func__, new_opp->rates[0]);
 	}
 
 	/* required-opps not fully initialized yet */
@@ -1814,7 +1845,7 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
 		return -ENOMEM;
 
 	/* populate the opp table */
-	new_opp->rate = freq;
+	new_opp->rates[0] = freq;
 	tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
 	new_opp->supplies[0].u_volt = u_volt;
 	new_opp->supplies[0].u_volt_min = u_volt - tol;
@@ -2017,6 +2048,17 @@ static void _opp_put_regulators(struct opp_table *opp_table)
 	opp_table->regulator_count = -1;
 }
 
+static void _put_clks(struct opp_table *opp_table, int count)
+{
+	int i;
+
+	for (i = count - 1; i >= 0; i--)
+		clk_put(opp_table->clks[i]);
+
+	kfree(opp_table->clks);
+	opp_table->clks = NULL;
+}
+
 /**
  * _opp_set_clknames() - Set clk names for the device
  * @dev: Device for which clk names is being set.
@@ -2031,10 +2073,12 @@ static void _opp_put_regulators(struct opp_table *opp_table)
  * This must be called before any OPPs are initialized for the device.
  */
 static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev,
-			     const char * const names[])
+			     const char * const names[],
+			     config_clks_t config_clks)
 {
 	const char * const *temp = names;
-	int count = 0;
+	int count = 0, ret, i;
+	struct clk *clk;
 
 	/* Count number of clks */
 	while (*temp++)
@@ -2047,28 +2091,60 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev,
 	if (!count && !names[1])
 		count = 1;
 
-	/* We support only one clock name for now */
-	if (count != 1)
+	/* Fail early for invalid configurations */
+	if (!count || (config_clks && count == 1) || (!config_clks && count > 1))
 		return -EINVAL;
 
 	/* Another CPU that shares the OPP table has set the clkname ? */
-	if (opp_table->clk_configured)
+	if (opp_table->clks)
 		return 0;
 
-	/* clk shouldn't be initialized at this point */
-	if (WARN_ON(opp_table->clk))
-		return -EBUSY;
+	opp_table->clks = kmalloc_array(count, sizeof(*opp_table->clks),
+					GFP_KERNEL);
+	if (!opp_table->clks)
+		return -ENOMEM;
 
-	/* Find clk for the device */
-	opp_table->clk = clk_get(dev, names[0]);
-	if (IS_ERR(opp_table->clk)) {
-		return dev_err_probe(dev, PTR_ERR(opp_table->clk),
-				    "%s: Couldn't find clock\n", __func__);
+	/* Find clks for the device */
+	for (i = 0; i < count; i++) {
+		clk = clk_get(dev, names[i]);
+		if (IS_ERR(clk)) {
+			ret = dev_err_probe(dev, PTR_ERR(clk),
+					    "%s: Couldn't find clock with name: %s\n",
+					    __func__, names[i]);
+			goto free_clks;
+		}
+
+		opp_table->clks[i] = clk;
 	}
 
-	opp_table->clk_configured = true;
+	opp_table->clk_count = count;
+
+	/* Set generic single clk set here */
+	if (count == 1) {
+		opp_table->config_clks = _opp_config_clk_single;
+
+		/*
+		 * We could have just dropped the "clk" field and used "clks"
+		 * everywhere. Instead we kept the "clk" field around for
+		 * following reasons:
+		 *
+		 * - avoiding clks[0] everywhere else.
+		 * - not running single clk helpers for multiple clk usecase by
+		 *   mistake.
+		 *
+		 * Since this is single-clk case, just update the clk pointer
+		 * too.
+		 */
+		opp_table->clk = opp_table->clks[0];
+	} else {
+		opp_table->config_clks = config_clks;
+	}
 
 	return 0;
+
+free_clks:
+	_put_clks(opp_table, i);
+	return ret;
 }
 
 /**
@@ -2077,11 +2153,13 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev,
  */
 static void _opp_put_clknames(struct opp_table *opp_table)
 {
-	if (opp_table->clk_configured) {
-		clk_put(opp_table->clk);
-		opp_table->clk = ERR_PTR(-EINVAL);
-		opp_table->clk_configured = false;
-	}
+	if (!opp_table->clks)
+		return;
+
+	opp_table->config_clks = NULL;
+	opp_table->clk = ERR_PTR(-ENODEV);
+
+	_put_clks(opp_table, opp_table->clk_count);
 }
 
 /**
@@ -2298,11 +2376,16 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
 
 	/* Configure clocks */
 	if (config->clk_names) {
-		ret = _opp_set_clknames(opp_table, dev, config->clk_names);
+		ret = _opp_set_clknames(opp_table, dev, config->clk_names,
+					config->config_clks);
 		if (ret)
 			goto err;
 
 		data->flags |= OPP_CONFIG_CLK;
+	} else if (config->config_clks) {
+		/* Don't allow config callback without clocks */
+		ret = -EINVAL;
+		goto err;
 	}
 
 	/* Configure property names */
@@ -2614,7 +2697,7 @@ static int _opp_set_availability(struct device *dev, unsigned long freq,
 
 	/* Do we have the frequency? */
 	list_for_each_entry(tmp_opp, &opp_table->opp_list, node) {
-		if (tmp_opp->rate == freq) {
+		if (tmp_opp->rates[0] == freq) {
 			opp = tmp_opp;
 			break;
 		}
@@ -2685,7 +2768,7 @@ int dev_pm_opp_adjust_voltage(struct device *dev, unsigned long freq,
 
 	/* Do we have the frequency? */
 	list_for_each_entry(tmp_opp, &opp_table->opp_list, node) {
-		if (tmp_opp->rate == freq) {
+		if (tmp_opp->rates[0] == freq) {
 			opp = tmp_opp;
 			break;
 		}
diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
index 1b6e5c55c3ed..402c507edac7 100644
--- a/drivers/opp/debugfs.c
+++ b/drivers/opp/debugfs.c
@@ -74,6 +74,24 @@ static void opp_debug_create_bw(struct dev_pm_opp *opp,
 	}
 }
 
+static void opp_debug_create_clks(struct dev_pm_opp *opp,
+				  struct opp_table *opp_table,
+				  struct dentry *pdentry)
+{
+	char name[12];
+	int i;
+
+	if (opp_table->clk_count == 1) {
+		debugfs_create_ulong("rate_hz", S_IRUGO, pdentry, &opp->rates[0]);
+		return;
+	}
+
+	for (i = 0; i < opp_table->clk_count; i++) {
+		snprintf(name, sizeof(name), "rate_hz_%d", i);
+		debugfs_create_ulong(name, S_IRUGO, pdentry, &opp->rates[i]);
+	}
+}
+
 static void opp_debug_create_supplies(struct dev_pm_opp *opp,
 				      struct opp_table *opp_table,
 				      struct dentry *pdentry)
@@ -117,10 +135,11 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
 	 * Get directory name for OPP.
 	 *
 	 * - Normally rate is unique to each OPP, use it to get unique opp-name.
-	 * - For some devices rate isn't available, use index instead.
+	 * - For some devices rate isn't available or there are multiple, use
+	 *   index instead for them.
 	 */
-	if (likely(opp->rate))
-		id = opp->rate;
+	if (likely(opp_table->clk_count == 1))
+		id = opp->rates[0];
 	else
 		id = _get_opp_count(opp_table);
 
@@ -134,7 +153,6 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
 	debugfs_create_bool("turbo", S_IRUGO, d, &opp->turbo);
 	debugfs_create_bool("suspend", S_IRUGO, d, &opp->suspend);
 	debugfs_create_u32("performance_state", S_IRUGO, d, &opp->pstate);
-	debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate);
 	debugfs_create_u32("level", S_IRUGO, d, &opp->level);
 	debugfs_create_ulong("clock_latency_ns", S_IRUGO, d,
 			     &opp->clock_latency_ns);
@@ -142,6 +160,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
 	opp->of_name = of_node_full_name(opp->np);
 	debugfs_create_str("of_name", S_IRUGO, d, (char **)&opp->of_name);
 
+	opp_debug_create_clks(opp, opp_table, d);
 	opp_debug_create_supplies(opp, opp_table, d);
 	opp_debug_create_bw(opp, opp_table, d);
 
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index bb49057cb1fc..080481a05223 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -767,6 +767,53 @@ void dev_pm_opp_of_remove_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
 
+static int _read_rate(struct dev_pm_opp *new_opp, struct opp_table *opp_table,
+		      struct device_node *np)
+{
+	struct property *prop;
+	int i, count, ret;
+	u64 *rates;
+
+	if (!opp_table->clk_count)
+		return -ENODEV;
+
+	prop = of_find_property(np, "opp-hz", NULL);
+	if (!prop)
+		return -ENODEV;
+
+	count = prop->length / sizeof(u64);
+	if (opp_table->clk_count != count) {
+		pr_err("%s: Count mismatch between opp-hz and clk_count (%d %d)\n",
+		       __func__, count, opp_table->clk_count);
+		return -EINVAL;
+	}
+
+	rates = kmalloc_array(count, sizeof(*rates), GFP_KERNEL);
+	if (!rates)
+		return -ENOMEM;
+
+	ret = of_property_read_u64_array(np, "opp-hz", rates, count);
+	if (ret) {
+		pr_err("%s: Error parsing opp-hz: %d\n", __func__, ret);
+	} 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.
+		 */
+		for (i = 0; i < count; i++) {
+			new_opp->rates[i] = (unsigned long)rates[i];
+
+			/* This will happen for frequencies > 4.29 GHz */
+			WARN_ON(new_opp->rates[i] != rates[i]);
+		}
+	}
+
+	kfree(rates);
+
+	return ret;
+}
+
 static int _read_bw(struct dev_pm_opp *new_opp, struct opp_table *opp_table,
 		    struct device_node *np, bool peak)
 {
@@ -812,19 +859,13 @@ static int _read_opp_key(struct dev_pm_opp *new_opp,
 			 struct opp_table *opp_table, struct device_node *np)
 {
 	bool found = false;
-	u64 rate;
 	int ret;
 
-	ret = of_property_read_u64(np, "opp-hz", &rate);
-	if (!ret) {
-		/*
-		 * Rate is defined as an unsigned long in clk API, and so
-		 * casting explicitly to its type. Must be fixed once rate is 64
-		 * bit guaranteed in clk API.
-		 */
-		new_opp->rate = (unsigned long)rate;
+	ret = _read_rate(new_opp, opp_table, np);
+	if (!ret)
 		found = true;
-	}
+	else if (ret != -ENODEV)
+		return ret;
 
 	/*
 	 * Bandwidth consists of peak and average (optional) values:
@@ -893,8 +934,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 
 	/* 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: %lu\n",
-			new_opp->rate);
+		dev_dbg(dev, "OPP not supported by hardware: %s\n",
+			of_node_full_name(np));
 		goto free_opp;
 	}
 
@@ -930,7 +971,7 @@ 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/bw/level as suspend OPP */
-			if (_opp_compare_key(new_opp, opp_table->suspend_opp) == 1) {
+			if (_opp_compare_key(opp_table, new_opp, opp_table->suspend_opp) == 1) {
 				opp_table->suspend_opp->suspend = false;
 				new_opp->suspend = true;
 				opp_table->suspend_opp = new_opp;
@@ -945,7 +986,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 		opp_table->clock_latency_ns_max = new_opp->clock_latency_ns;
 
 	pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu level:%u\n",
-		 __func__, new_opp->turbo, new_opp->rate,
+		 __func__, new_opp->turbo, new_opp->rates[0],
 		 new_opp->supplies[0].u_volt, new_opp->supplies[0].u_volt_min,
 		 new_opp->supplies[0].u_volt_max, new_opp->clock_latency_ns,
 		 new_opp->level);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index e481bdb59499..816009eaafee 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -79,7 +79,7 @@ struct opp_config_data {
  * @suspend:	true if suspend OPP
  * @removed:	flag indicating that OPP's reference is dropped by OPP core.
  * @pstate: Device's power domain's performance state.
- * @rate:	Frequency in hertz
+ * @rates:	Frequencies in hertz
  * @level:	Performance level
  * @supplies:	Power supplies voltage/current values
  * @bandwidth:	Interconnect bandwidth values
@@ -102,7 +102,7 @@ struct dev_pm_opp {
 	bool suspend;
 	bool removed;
 	unsigned int pstate;
-	unsigned long rate;
+	unsigned long *rates;
 	unsigned int level;
 
 	struct dev_pm_opp_supply *supplies;
@@ -170,8 +170,10 @@ enum opp_table_access {
  * @supported_hw: Array of version number to support.
  * @supported_hw_count: Number of elements in supported_hw array.
  * @prop_name: A name to postfix to many DT properties, while parsing them.
- * @clk_configured: Clock name is configured by the platform.
- * @clk: Device's clock handle
+ * @config_clks: Platform specific config_clks() callback.
+ * @clks: Device's clock handles, for multiple clocks.
+ * @clk: Device's clock handle, for single clock.
+ * @clk_count: Number of clocks.
  * @config_regulators: Platform specific config_regulators() callback.
  * @regulators: Supply regulators
  * @regulator_count: Number of power supply regulators. Its value can be -1
@@ -220,8 +222,10 @@ struct opp_table {
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
 	const char *prop_name;
-	bool clk_configured;
+	config_clks_t config_clks;
+	struct clk **clks;
 	struct clk *clk;
+	int clk_count;
 	config_regulators_t config_regulators;
 	struct regulator **regulators;
 	int regulator_count;
@@ -246,7 +250,7 @@ struct opp_table *_find_opp_table(struct device *dev);
 struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table);
 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_compare_key(struct opp_table *opp_table, 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);
 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);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 50cbc75bef71..104151dfe46c 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -61,9 +61,13 @@ typedef int (*config_regulators_t)(struct device *dev,
 			struct dev_pm_opp *old_opp, struct dev_pm_opp *new_opp,
 			struct regulator **regulators, unsigned int count);
 
+typedef int (*config_clks_t)(struct device *dev, struct opp_table *opp_table,
+			struct dev_pm_opp *opp, void *data, bool scaling_down);
+
 /**
  * struct dev_pm_opp_config - Device OPP configuration values
- * @clk_names: Clk names, NULL terminated array, max 1 clock for now.
+ * @clk_names: Clk names, NULL terminated array.
+ * @config_clks: Custom set clk helper.
  * @prop_name: Name to postfix to properties.
  * @config_regulators: Custom set regulator helper.
  * @supported_hw: Array of hierarchy of versions to match.
@@ -78,6 +82,7 @@ typedef int (*config_regulators_t)(struct device *dev,
 struct dev_pm_opp_config {
 	/* NULL terminated */
 	const char * const *clk_names;
+	config_clks_t config_clks;
 	const char *prop_name;
 	config_regulators_t config_regulators;
 	const unsigned int *supported_hw;
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 07/13] OPP: Compare bandwidths for all paths in _opp_compare_key()

[Viresh Kumar]

Replicate the same behavior as "rates" here and compare all values
instead of relying on the first entry alone.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 003cd48123d7..261f5e1abfe1 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1683,6 +1683,20 @@ static int _opp_compare_rate(struct opp_table *opp_table,
 	return 0;
 }
 
+static int _opp_compare_bw(struct opp_table *opp_table, struct dev_pm_opp *opp1,
+			   struct dev_pm_opp *opp2)
+{
+	int i;
+
+	for (i = 0; i < opp_table->path_count; i++) {
+		if (opp1->bandwidth[i].peak != opp2->bandwidth[i].peak)
+			return opp1->bandwidth[i].peak < opp2->bandwidth[i].peak ? -1 : 1;
+	}
+
+	/* Same bw for both OPPs */
+	return 0;
+}
+
 /*
  * Returns
  * 0: opp1 == opp2
@@ -1698,9 +1712,9 @@ int _opp_compare_key(struct opp_table *opp_table, struct dev_pm_opp *opp1,
 	if (ret)
 		return ret;
 
-	if (opp1->bandwidth && opp2->bandwidth &&
-	    opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
-		return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1;
+	ret = _opp_compare_bw(opp_table, opp1, opp2);
+	if (ret)
+		return ret;
 
 	if (opp1->level != opp2->level)
 		return opp1->level < opp2->level ? -1 : 1;
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 08/13] OPP: Add key specific assert() method to key finding helpers

[Viresh Kumar]

The helpers for the clock key, at least, would need to assert that the
helpers are called only for single clock case. Prepare for that by
adding an argument to the key finding helpers.

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 52 +++++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 261f5e1abfe1..e1696cf63409 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -481,10 +481,15 @@ static struct dev_pm_opp *_opp_table_find_key(struct opp_table *opp_table,
 		unsigned long *key, int index, bool available,
 		unsigned long (*read)(struct dev_pm_opp *opp, int index),
 		bool (*compare)(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
-				unsigned long opp_key, unsigned long key))
+				unsigned long opp_key, unsigned long key),
+		bool (*assert)(struct opp_table *opp_table))
 {
 	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
 
+	/* Assert that the requirement is met */
+	if (assert && !assert(opp_table))
+		return ERR_PTR(-EINVAL);
+
 	mutex_lock(&opp_table->lock);
 
 	list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
@@ -509,7 +514,8 @@ static struct dev_pm_opp *
 _find_key(struct device *dev, unsigned long *key, int index, bool available,
 	  unsigned long (*read)(struct dev_pm_opp *opp, int index),
 	  bool (*compare)(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
-			  unsigned long opp_key, unsigned long key))
+			  unsigned long opp_key, unsigned long key),
+	  bool (*assert)(struct opp_table *opp_table))
 {
 	struct opp_table *opp_table;
 	struct dev_pm_opp *opp;
@@ -522,7 +528,7 @@ _find_key(struct device *dev, unsigned long *key, int index, bool available,
 	}
 
 	opp = _opp_table_find_key(opp_table, key, index, available, read,
-				  compare);
+				  compare, assert);
 
 	dev_pm_opp_put_opp_table(opp_table);
 
@@ -531,35 +537,42 @@ _find_key(struct device *dev, unsigned long *key, int index, bool available,
 
 static struct dev_pm_opp *_find_key_exact(struct device *dev,
 		unsigned long key, int index, bool available,
-		unsigned long (*read)(struct dev_pm_opp *opp, int index))
+		unsigned long (*read)(struct dev_pm_opp *opp, int index),
+		bool (*assert)(struct opp_table *opp_table))
 {
 	/*
 	 * The value of key will be updated here, but will be ignored as the
 	 * caller doesn't need it.
 	 */
-	return _find_key(dev, &key, index, available, read, _compare_exact);
+	return _find_key(dev, &key, index, available, read, _compare_exact,
+			 assert);
 }
 
 static struct dev_pm_opp *_opp_table_find_key_ceil(struct opp_table *opp_table,
 		unsigned long *key, int index, bool available,
-		unsigned long (*read)(struct dev_pm_opp *opp, int index))
+		unsigned long (*read)(struct dev_pm_opp *opp, int index),
+		bool (*assert)(struct opp_table *opp_table))
 {
 	return _opp_table_find_key(opp_table, key, index, available, read,
-				   _compare_ceil);
+				   _compare_ceil, assert);
 }
 
 static struct dev_pm_opp *_find_key_ceil(struct device *dev, unsigned long *key,
 		int index, bool available,
-		unsigned long (*read)(struct dev_pm_opp *opp, int index))
+		unsigned long (*read)(struct dev_pm_opp *opp, int index),
+		bool (*assert)(struct opp_table *opp_table))
 {
-	return _find_key(dev, key, index, available, read, _compare_ceil);
+	return _find_key(dev, key, index, available, read, _compare_ceil,
+			 assert);
 }
 
 static struct dev_pm_opp *_find_key_floor(struct device *dev,
 		unsigned long *key, int index, bool available,
-		unsigned long (*read)(struct dev_pm_opp *opp, int index))
+		unsigned long (*read)(struct dev_pm_opp *opp, int index),
+		bool (*assert)(struct opp_table *opp_table))
 {
-	return _find_key(dev, key, index, available, read, _compare_floor);
+	return _find_key(dev, key, index, available, read, _compare_floor,
+			 assert);
 }
 
 /**
@@ -588,14 +601,15 @@ static struct dev_pm_opp *_find_key_floor(struct device *dev,
 struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 		unsigned long freq, bool available)
 {
-	return _find_key_exact(dev, freq, 0, available, _read_freq);
+	return _find_key_exact(dev, freq, 0, available, _read_freq, NULL);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_exact);
 
 static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,
 						   unsigned long *freq)
 {
-	return _opp_table_find_key_ceil(opp_table, freq, 0, true, _read_freq);
+	return _opp_table_find_key_ceil(opp_table, freq, 0, true, _read_freq,
+					NULL);
 }
 
 /**
@@ -619,7 +633,7 @@ static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,
 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 					     unsigned long *freq)
 {
-	return _find_key_ceil(dev, freq, 0, true, _read_freq);
+	return _find_key_ceil(dev, freq, 0, true, _read_freq, NULL);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil);
 
@@ -644,7 +658,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil);
 struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 					      unsigned long *freq)
 {
-	return _find_key_floor(dev, freq, 0, true, _read_freq);
+	return _find_key_floor(dev, freq, 0, true, _read_freq, NULL);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
@@ -666,7 +680,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 struct dev_pm_opp *dev_pm_opp_find_level_exact(struct device *dev,
 					       unsigned int level)
 {
-	return _find_key_exact(dev, level, 0, true, _read_level);
+	return _find_key_exact(dev, level, 0, true, _read_level, NULL);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_level_exact);
 
@@ -691,7 +705,7 @@ struct dev_pm_opp *dev_pm_opp_find_level_ceil(struct device *dev,
 	unsigned long temp = *level;
 	struct dev_pm_opp *opp;
 
-	opp = _find_key_ceil(dev, &temp, 0, true, _read_level);
+	opp = _find_key_ceil(dev, &temp, 0, true, _read_level, NULL);
 	*level = temp;
 	return opp;
 }
@@ -722,7 +736,7 @@ struct dev_pm_opp *dev_pm_opp_find_bw_ceil(struct device *dev, unsigned int *bw,
 	unsigned long temp = *bw;
 	struct dev_pm_opp *opp;
 
-	opp = _find_key_ceil(dev, &temp, index, true, _read_bw);
+	opp = _find_key_ceil(dev, &temp, index, true, _read_bw, NULL);
 	*bw = temp;
 	return opp;
 }
@@ -753,7 +767,7 @@ struct dev_pm_opp *dev_pm_opp_find_bw_floor(struct device *dev,
 	unsigned long temp = *bw;
 	struct dev_pm_opp *opp;
 
-	opp = _find_key_floor(dev, &temp, index, true, _read_bw);
+	opp = _find_key_floor(dev, &temp, index, true, _read_bw, NULL);
 	*bw = temp;
 	return opp;
 }
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 09/13] OPP: Assert clk_count == 1 for single clk helpers

[Viresh Kumar]

Many helpers can be safely called only for devices that have a single
clk associated with them. Assert the same for those routines.

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 45 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e1696cf63409..5b3542557f72 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -97,6 +97,18 @@ struct opp_table *_find_opp_table(struct device *dev)
 	return opp_table;
 }
 
+/*
+ * Returns true if multiple clocks aren't there, else returns false with WARN.
+ *
+ * We don't force clk_count == 1 here as there are users who don't have a clock
+ * representation in the OPP table and manage the clock configuration themselves
+ * in an platform specific way.
+ */
+static bool assert_single_clk(struct opp_table *opp_table)
+{
+	return !WARN_ON(opp_table->clk_count > 1);
+}
+
 /**
  * dev_pm_opp_get_voltage() - Gets the voltage corresponding to an opp
  * @opp:	opp for which voltage has to be returned for
@@ -181,6 +193,9 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
 		return 0;
 	}
 
+	if (!assert_single_clk(opp->opp_table))
+		return 0;
+
 	return opp->rates[0];
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
@@ -601,7 +616,8 @@ static struct dev_pm_opp *_find_key_floor(struct device *dev,
 struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 		unsigned long freq, bool available)
 {
-	return _find_key_exact(dev, freq, 0, available, _read_freq, NULL);
+	return _find_key_exact(dev, freq, 0, available, _read_freq,
+			       assert_single_clk);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_exact);
 
@@ -609,7 +625,7 @@ static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,
 						   unsigned long *freq)
 {
 	return _opp_table_find_key_ceil(opp_table, freq, 0, true, _read_freq,
-					NULL);
+					assert_single_clk);
 }
 
 /**
@@ -633,7 +649,7 @@ static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,
 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 					     unsigned long *freq)
 {
-	return _find_key_ceil(dev, freq, 0, true, _read_freq, NULL);
+	return _find_key_ceil(dev, freq, 0, true, _read_freq, assert_single_clk);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil);
 
@@ -658,7 +674,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil);
 struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 					      unsigned long *freq)
 {
-	return _find_key_floor(dev, freq, 0, true, _read_freq, NULL);
+	return _find_key_floor(dev, freq, 0, true, _read_freq, assert_single_clk);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
@@ -1521,6 +1537,9 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq)
 	if (IS_ERR(opp_table))
 		return;
 
+	if (!assert_single_clk(opp_table))
+		goto put_table;
+
 	mutex_lock(&opp_table->lock);
 
 	list_for_each_entry(iter, &opp_table->opp_list, node) {
@@ -1542,6 +1561,7 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq)
 			 __func__, freq);
 	}
 
+put_table:
 	/* Drop the reference taken by _find_opp_table() */
 	dev_pm_opp_put_opp_table(opp_table);
 }
@@ -1868,6 +1888,9 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
 	unsigned long tol;
 	int ret;
 
+	if (!assert_single_clk(opp_table))
+		return -EINVAL;
+
 	new_opp = _opp_allocate(opp_table);
 	if (!new_opp)
 		return -ENOMEM;
@@ -2721,6 +2744,11 @@ static int _opp_set_availability(struct device *dev, unsigned long freq,
 		return r;
 	}
 
+	if (!assert_single_clk(opp_table)) {
+		r = -EINVAL;
+		goto put_table;
+	}
+
 	mutex_lock(&opp_table->lock);
 
 	/* Do we have the frequency? */
@@ -2792,6 +2820,11 @@ int dev_pm_opp_adjust_voltage(struct device *dev, unsigned long freq,
 		return r;
 	}
 
+	if (!assert_single_clk(opp_table)) {
+		r = -EINVAL;
+		goto put_table;
+	}
+
 	mutex_lock(&opp_table->lock);
 
 	/* Do we have the frequency? */
@@ -2823,11 +2856,11 @@ int dev_pm_opp_adjust_voltage(struct device *dev, unsigned long freq,
 				     opp);
 
 	dev_pm_opp_put(opp);
-	goto adjust_put_table;
+	goto put_table;
 
 adjust_unlock:
 	mutex_unlock(&opp_table->lock);
-adjust_put_table:
+put_table:
 	dev_pm_opp_put_opp_table(opp_table);
 	return r;
 }
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 10/13] OPP: Provide a simple implementation to configure multiple clocks

[Viresh Kumar]

This provides a simple implementation to configure multiple clocks for a
device.

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 34 ++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h | 10 ++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 5b3542557f72..597f7df3e375 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -843,6 +843,40 @@ _opp_config_clk_single(struct device *dev, struct opp_table *opp_table,
 	return ret;
 }
 
+/*
+ * Simple implementation for configuring multiple clocks. Configure clocks in
+ * the order in which they are present in the array while scaling up.
+ */
+int dev_pm_opp_config_clks_simple(struct device *dev,
+		struct opp_table *opp_table, struct dev_pm_opp *opp, void *data,
+		bool scaling_down)
+{
+	int ret, i;
+
+	if (scaling_down) {
+		for (i = opp_table->clk_count - 1; i >= 0; i--) {
+			ret = clk_set_rate(opp_table->clks[i], opp->rates[i]);
+			if (ret) {
+				dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
+					ret);
+				return ret;
+			}
+		}
+	} else {
+		for (i = 0; i < opp_table->clk_count; i++) {
+			ret = clk_set_rate(opp_table->clks[i], opp->rates[i]);
+			if (ret) {
+				dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
+					ret);
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_config_clks_simple);
+
 static int _opp_config_regulator_single(struct device *dev,
 			struct dev_pm_opp *old_opp, struct dev_pm_opp *new_opp,
 			struct regulator **regulators, unsigned int count)
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 104151dfe46c..683e6baf9618 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -159,6 +159,9 @@ int dev_pm_opp_unregister_notifier(struct device *dev, struct notifier_block *nb
 int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config);
 int devm_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config);
 void dev_pm_opp_clear_config(int token);
+int dev_pm_opp_config_clks_simple(struct device *dev,
+		struct opp_table *opp_table, struct dev_pm_opp *opp, void *data,
+		bool scaling_down);
 
 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);
@@ -342,6 +345,13 @@ static inline int devm_pm_opp_set_config(struct device *dev, struct dev_pm_opp_c
 
 static inline void dev_pm_opp_clear_config(int token) {}
 
+static inline int dev_pm_opp_config_clks_simple(struct device *dev,
+		struct opp_table *opp_table, struct dev_pm_opp *opp, void *data,
+		bool scaling_down)
+{
+	return -EOPNOTSUPP;
+}
+
 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)
 {
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 11/13] OPP: Allow config_clks helper for single clk case

[Viresh Kumar]

There is a corner case with Tegra30, where we want to skip clk
configuration via dev_pm_opp_set_opp(), but still want the OPP core to
read the "opp-hz" property so we can find the right OPP via freq finding
helpers.

This is the easiest of the ways to make it work, without any special
hacks in the OPP core. Allow config_clks to be passed for single clk
case.

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 597f7df3e375..666e1ebf91d1 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2177,7 +2177,7 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev,
 		count = 1;
 
 	/* Fail early for invalid configurations */
-	if (!count || (config_clks && count == 1) || (!config_clks && count > 1))
+	if (!count || (!config_clks && count > 1))
 		return -EINVAL;
 
 	/* Another CPU that shares the OPP table has set the clkname ? */
@@ -2203,10 +2203,12 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev,
 	}
 
 	opp_table->clk_count = count;
+	opp_table->config_clks = config_clks;
 
 	/* Set generic single clk set here */
 	if (count == 1) {
-		opp_table->config_clks = _opp_config_clk_single;
+		if (!opp_table->config_clks)
+			opp_table->config_clks = _opp_config_clk_single;
 
 		/*
 		 * We could have just dropped the "clk" field and used "clks"
@@ -2221,8 +2223,6 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev,
 		 * too.
 		 */
 		opp_table->clk = opp_table->clks[0];
-	} else {
-		opp_table->config_clks = config_clks;
 	}
 
 	return 0;
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 12/13] PM / devfreq: tegra30: Register config_clks helper

[Viresh Kumar]

There is a corner case with Tegra30, where we want to skip clk
configuration that happens via dev_pm_opp_set_opp(), but still want the
OPP core to read the "opp-hz" property so we can find the right OPP via
freq finding helpers.

The OPP core provides support for the platforms to provide config_clks
helpers now, lets use that instead of devm_pm_opp_of_add_table_noclk()
to achieve the same result, as the OPP core won't parse the DT's
"opp-hz" property anymore if the clock isn't provided.

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/devfreq/tegra30-devfreq.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 65ecf17a36f4..0e0a4058f45c 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -821,6 +821,15 @@ static int devm_tegra_devfreq_init_hw(struct device *dev,
 	return err;
 }
 
+static int tegra_devfreq_config_clks_nop(struct device *dev,
+					 struct opp_table *opp_table,
+					 struct dev_pm_opp *opp, void *data,
+					 bool scaling_down)
+{
+	/* We want to skip clk configuration via dev_pm_opp_set_opp() */
+	return 0;
+}
+
 static int tegra_devfreq_probe(struct platform_device *pdev)
 {
 	u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
@@ -830,6 +839,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	unsigned int i;
 	long rate;
 	int err;
+	const char *clk_names[] = { "actmon", NULL };
+	struct dev_pm_opp_config config = {
+		.supported_hw = &hw_version,
+		.supported_hw_count = 1,
+		.clk_names = clk_names,
+		.config_clks = tegra_devfreq_config_clks_nop,
+	};
 
 	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
 	if (!tegra)
@@ -874,13 +890,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	err = devm_pm_opp_set_supported_hw(&pdev->dev, &hw_version, 1);
+	err = devm_pm_opp_set_config(&pdev->dev, &config);
 	if (err) {
-		dev_err(&pdev->dev, "Failed to set supported HW: %d\n", err);
+		dev_err(&pdev->dev, "Failed to set OPP config: %d\n", err);
 		return err;
 	}
 
-	err = devm_pm_opp_of_add_table_noclk(&pdev->dev, 0);
+	err = devm_pm_opp_of_add_table_indexed(&pdev->dev, 0);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to add OPP table: %d\n", err);
 		return err;
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 13/13] OPP: Remove dev{m}_pm_opp_of_add_table_noclk()

[Viresh Kumar]

Remove the now unused variants and the extra "getclk" parameter from few
routines, which is always "true" now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 19 +++++++----------
 drivers/opp/of.c       | 48 +++++++-----------------------------------
 drivers/opp/opp.h      |  2 +-
 include/linux/pm_opp.h | 12 -----------
 4 files changed, 17 insertions(+), 64 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 666e1ebf91d1..0205b83e1c02 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1360,8 +1360,7 @@ void _get_opp_table_kref(struct opp_table *opp_table)
 }
 
 static struct opp_table *_update_opp_table_clk(struct device *dev,
-					       struct opp_table *opp_table,
-					       bool getclk)
+					       struct opp_table *opp_table)
 {
 	int ret;
 
@@ -1369,8 +1368,7 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
 	 * Return early if we don't need to get clk or we have already done it
 	 * earlier.
 	 */
-	if (!getclk || IS_ERR(opp_table) || !IS_ERR(opp_table->clk) ||
-	    opp_table->clks)
+	if (IS_ERR(opp_table) || !IS_ERR(opp_table->clk) || opp_table->clks)
 		return opp_table;
 
 	/* Find clk for the device */
@@ -1409,8 +1407,7 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
  * uses the opp_tables_busy flag to indicate if another creator is in the middle
  * of adding an OPP table and others should wait for it to finish.
  */
-struct opp_table *_add_opp_table_indexed(struct device *dev, int index,
-					 bool getclk)
+struct opp_table *_add_opp_table_indexed(struct device *dev, int index)
 {
 	struct opp_table *opp_table;
 
@@ -1457,12 +1454,12 @@ struct opp_table *_add_opp_table_indexed(struct device *dev, int index,
 unlock:
 	mutex_unlock(&opp_table_lock);
 
-	return _update_opp_table_clk(dev, opp_table, getclk);
+	return _update_opp_table_clk(dev, opp_table);
 }
 
-static struct opp_table *_add_opp_table(struct device *dev, bool getclk)
+static struct opp_table *_add_opp_table(struct device *dev)
 {
-	return _add_opp_table_indexed(dev, 0, getclk);
+	return _add_opp_table_indexed(dev, 0);
 }
 
 struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
@@ -2444,7 +2441,7 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
 	if (!data)
 		return -ENOMEM;
 
-	opp_table = _add_opp_table(dev, false);
+	opp_table = _add_opp_table(dev);
 	if (IS_ERR(opp_table)) {
 		kfree(data);
 		return PTR_ERR(opp_table);
@@ -2735,7 +2732,7 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 	struct opp_table *opp_table;
 	int ret;
 
-	opp_table = _add_opp_table(dev, true);
+	opp_table = _add_opp_table(dev);
 	if (IS_ERR(opp_table))
 		return PTR_ERR(opp_table);
 
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 080481a05223..6b19764a3897 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -1123,7 +1123,7 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
 	return ret;
 }
 
-static int _of_add_table_indexed(struct device *dev, int index, bool getclk)
+static int _of_add_table_indexed(struct device *dev, int index)
 {
 	struct opp_table *opp_table;
 	int ret, count;
@@ -1139,7 +1139,7 @@ static int _of_add_table_indexed(struct device *dev, int index, bool getclk)
 			index = 0;
 	}
 
-	opp_table = _add_opp_table_indexed(dev, index, getclk);
+	opp_table = _add_opp_table_indexed(dev, index);
 	if (IS_ERR(opp_table))
 		return PTR_ERR(opp_table);
 
@@ -1163,11 +1163,11 @@ static void devm_pm_opp_of_table_release(void *data)
 	dev_pm_opp_of_remove_table(data);
 }
 
-static int _devm_of_add_table_indexed(struct device *dev, int index, bool getclk)
+static int _devm_of_add_table_indexed(struct device *dev, int index)
 {
 	int ret;
 
-	ret = _of_add_table_indexed(dev, index, getclk);
+	ret = _of_add_table_indexed(dev, index);
 	if (ret)
 		return ret;
 
@@ -1195,7 +1195,7 @@ static int _devm_of_add_table_indexed(struct device *dev, int index, bool getclk
  */
 int devm_pm_opp_of_add_table(struct device *dev)
 {
-	return _devm_of_add_table_indexed(dev, 0, true);
+	return _devm_of_add_table_indexed(dev, 0);
 }
 EXPORT_SYMBOL_GPL(devm_pm_opp_of_add_table);
 
@@ -1218,7 +1218,7 @@ EXPORT_SYMBOL_GPL(devm_pm_opp_of_add_table);
  */
 int dev_pm_opp_of_add_table(struct device *dev)
 {
-	return _of_add_table_indexed(dev, 0, true);
+	return _of_add_table_indexed(dev, 0);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table);
 
@@ -1234,7 +1234,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table);
  */
 int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
 {
-	return _of_add_table_indexed(dev, index, true);
+	return _of_add_table_indexed(dev, index);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table_indexed);
 
@@ -1247,42 +1247,10 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table_indexed);
  */
 int devm_pm_opp_of_add_table_indexed(struct device *dev, int index)
 {
-	return _devm_of_add_table_indexed(dev, index, true);
+	return _devm_of_add_table_indexed(dev, index);
 }
 EXPORT_SYMBOL_GPL(devm_pm_opp_of_add_table_indexed);
 
-/**
- * dev_pm_opp_of_add_table_noclk() - Initialize indexed opp table from device
- *		tree without getting clk for device.
- * @dev:	device pointer used to lookup OPP table.
- * @index:	Index number.
- *
- * Register the initial OPP table with the OPP library for given device only
- * using the "operating-points-v2" property. Do not try to get the clk for the
- * device.
- *
- * Return: Refer to dev_pm_opp_of_add_table() for return values.
- */
-int dev_pm_opp_of_add_table_noclk(struct device *dev, int index)
-{
-	return _of_add_table_indexed(dev, index, false);
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table_noclk);
-
-/**
- * devm_pm_opp_of_add_table_noclk() - Initialize indexed opp table from device
- *		tree without getting clk for device.
- * @dev:	device pointer used to lookup OPP table.
- * @index:	Index number.
- *
- * This is a resource-managed variant of dev_pm_opp_of_add_table_noclk().
- */
-int devm_pm_opp_of_add_table_noclk(struct device *dev, int index)
-{
-	return _devm_of_add_table_indexed(dev, index, false);
-}
-EXPORT_SYMBOL_GPL(devm_pm_opp_of_add_table_noclk);
-
 /* CPU device specific helpers */
 
 /**
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 816009eaafee..5e089651c91f 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -254,7 +254,7 @@ int _opp_compare_key(struct opp_table *opp_table, struct dev_pm_opp *opp1, struc
 int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table);
 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);
-struct opp_table *_add_opp_table_indexed(struct device *dev, int index, bool getclk);
+struct opp_table *_add_opp_table_indexed(struct device *dev, int index);
 void _put_opp_list_kref(struct opp_table *opp_table);
 void _required_opps_available(struct dev_pm_opp *opp, int count);
 
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 683e6baf9618..dc1fb5890792 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -402,8 +402,6 @@ static inline int dev_pm_opp_sync_regulators(struct device *dev)
 int dev_pm_opp_of_add_table(struct device *dev);
 int dev_pm_opp_of_add_table_indexed(struct device *dev, int index);
 int devm_pm_opp_of_add_table_indexed(struct device *dev, int index);
-int dev_pm_opp_of_add_table_noclk(struct device *dev, int index);
-int devm_pm_opp_of_add_table_noclk(struct device *dev, int index);
 void dev_pm_opp_of_remove_table(struct device *dev);
 int devm_pm_opp_of_add_table(struct device *dev);
 int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask);
@@ -434,16 +432,6 @@ static inline int devm_pm_opp_of_add_table_indexed(struct device *dev, int index
 	return -EOPNOTSUPP;
 }
 
-static inline int dev_pm_opp_of_add_table_noclk(struct device *dev, int index)
-{
-	return -EOPNOTSUPP;
-}
-
-static inline int devm_pm_opp_of_add_table_noclk(struct device *dev, int index)
-{
-	return -EOPNOTSUPP;
-}
-
 static inline void dev_pm_opp_of_remove_table(struct device *dev)
 {
 }
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V2 09/13] OPP: Assert clk_count == 1 for single clk helpers

[Krzysztof Kozlowski]

On 05/07/2022 09:00, Viresh Kumar wrote:
> Many helpers can be safely called only for devices that have a single
> clk associated with them. Assert the same for those routines.
> 

Where is the safety problem with multiple-clocks case? And how users of
PM OPP API are supposed to iterate/find OPPs they want if the API now
throws WARN?

Best regards,
Krzysztof

Re: [PATCH V2 09/13] OPP: Assert clk_count == 1 for single clk helpers

[Viresh Kumar]

On 05-07-22, 19:21, Krzysztof Kozlowski wrote:
> Where is the safety problem with multiple-clocks case?

All these APIs, which I have added the assert to now, are designed
with a single clk/freq in mind. They simply take a clock frequency
value as input and do something based on it. It only works fine with
single clk case, as more information is required for multiple clock
case, a freq value isn't sufficient here. In order to avoid abuse or
accidental use of the wrong API, these WARNs were required.

> And how users of PM OPP API are supposed to iterate/find OPPs they
> want if the API now throws WARN?

We need to provide new APIs for that, as I mentioned in the cover
letter and the other mail I sent you yesterday.

Specifically, if we want to have OPP finder API based on freq value,
then it needs to either have index + freq as input, or an array of
frequencies (one for each clk) as input.

Though I am not sure what you would need at the moment, as you can
also use opp-level for OPPs as they match UFS gears, that was my
understanding from earlier discussion.

-- 
viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Dmitry Osipenko]

On 7/5/22 10:00, Viresh Kumar wrote:
> Hello,
> 
> This patchset adds support for devices with multiple clocks. None of the clocks
> is considered primary in this case and all are handled equally.
> 
> The drivers, for multiple clock case, are expected to call dev_pm_opp_set_opp()
> to set the specific OPP. Though how they find the target OPP is left for the
> users to handle. For some, we may have another unique OPP property, like level,
> which can be used to find the OPP. While in case of others, we may want to
> implement freq-based OPP finder APIs for multiple clock rates. I have decided
> not to implement them in advance, and add them only someone wants to use them.
> 
> This is rebased over a lot of other OPP changes and is pushed here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next
> 
> V1->V2:
> 
> - Fix broken git bisect for:
>   OPP: Reuse _opp_compare_key() in _opp_add_static_v2()
> 
> - Include binding changes written by Krzysztof earlier.
> 
> - Check config_clks before calling it, it isn't always set.
> 
> - Add config_clks for Tegra30's devfreq to handle its corner case.
> 
> - _opp_compare_key() supports multi-clk case now, earlier it skipped freq
>   comparison for such a case.
> 
> - New patch to compare all bandwidth values as well in _opp_compare_key().
> 
> - New patch to remove *_noclk() interface.
> 
> - Various other minor fixes.
> 
> --
> Viresh
> 
> Krzysztof Kozlowski (1):
>   dt-bindings: opp: accept array of frequencies
> 
> Viresh Kumar (12):
>   OPP: Use consistent names for OPP table instances
>   OPP: Remove rate_not_available parameter to _opp_add()
>   OPP: Reuse _opp_compare_key() in _opp_add_static_v2()
>   OPP: Make dev_pm_opp_set_opp() independent of frequency
>   OPP: Allow multiple clocks for a device
>   OPP: Compare bandwidths for all paths in _opp_compare_key()
>   OPP: Add key specific assert() method to key finding helpers
>   OPP: Assert clk_count == 1 for single clk helpers
>   OPP: Provide a simple implementation to configure multiple clocks
>   OPP: Allow config_clks helper for single clk case
>   PM / devfreq: tegra30: Register config_clks helper

Hello Viresh,

This patch breaks Tegra again, please take a look:

   OPP: Remove dev{m}_pm_opp_of_add_table_noclk()

 8<--- cut here ---
 Unable to handle kernel paging request at virtual address ffffffff
 [ffffffff] *pgd=9effd861, *pte=00000000, *ppte=00000000
 Internal error: Oops: 37 [#1] PREEMPT SMP ARM
 Modules linked in:
 CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted
5.19.0-rc1-00040-g30b62d123f4f #82
 Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
 Workqueue: events_unbound deferred_probe_work_func
 PC is at _opp_compare_key+0x40/0xc4
 LR is at 0xfffffffb
 pc : [<c0b91b54>]    lr : [<fffffffb>]    psr: 20000113
 sp : df831b08  ip : c33cd4d0  fp : df831b24
 r10: c2586078  r9 : c258606c  r8 : 00000000
 r7 : 00000000  r6 : 00000001  r5 : c33cd480  r4 : c2586000
 r3 : 00000000  r2 : c33cd480  r1 : c258606c  r0 : c2586000
 Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
 Control: 10c5387d  Table: 8000404a  DAC: 00000051
...
 Backtrace:
  _opp_compare_key from _set_opp+0x80/0x408
  r7:00000000 r6:c27c0010 r5:c33cd480 r4:c2586000
  _set_opp from dev_pm_opp_set_opp+0x74/0xdc
  r10:00000001 r9:000f4240 r8:c33b6840 r7:c33b6840 r6:c33cd480 r5:c27c0010
  r4:c2586000
  dev_pm_opp_set_opp from tegra_pmc_core_pd_set_performance_state+0x54/0xbc
  r6:c33b6840 r5:c1a21760 r4:c33cd480
  tegra_pmc_core_pd_set_performance_state from
_genpd_set_performance_state+0x1f0/0x280
  r6:c33b6b58 r5:c33b6b48 r4:000f4240
  _genpd_set_performance_state from _genpd_set_performance_state+0xb4/0x280
  r10:00000001 r9:000f4240 r8:c33b9800 r7:c33b6840 r6:c33b9b18 r5:c33d0040
  r4:000f4240
  _genpd_set_performance_state from genpd_set_performance_state+0xb8/0xd4
  r10:c33b9a98 r9:c33d0400 r8:00000000 r7:00000000 r6:c33b9800 r5:00000000
  r4:c33d0400
  genpd_set_performance_state from genpd_runtime_resume+0x22c/0x240
  r5:00000000 r4:c27c0810
  genpd_runtime_resume from __rpm_callback+0x4c/0x1ac
  r10:c27ba8bc r9:00000000 r8:c27c0960 r7:c27c08cc r6:c27ba800 r5:c09611b8
  r4:c27c0810
  __rpm_callback from rpm_callback+0x60/0x64
  r9:df831ce4 r8:c27c0960 r7:00000004 r6:c27ba800 r5:c09611b8 r4:c27c0810
  rpm_callback from rpm_resume+0x480/0x7e0
  r7:00000004 r6:c27ba800 r5:c09611b8 r4:c27c0810
  rpm_resume from __pm_runtime_resume+0x58/0xb0
  r10:00000000 r9:c2587194 r8:c2587210 r7:c27c0810 r6:c27c08cc r5:60000113
  r4:c27c0810
  __pm_runtime_resume from host1x_probe+0x3d4/0x6d4
  r7:c27c0810 r6:c27c0800 r5:00000000 r4:c2587040
  host1x_probe from platform_probe+0x6c/0xc0
  r10:c191438c r9:c1aa8e20 r8:0000000d r7:c27c0810 r6:c1a2d354 r5:c27c0810
  r4:00000000
  platform_probe from really_probe.part.0+0xac/0x2c0
  r7:c27c0810 r6:c1a2d354 r5:c27c0810 r4:00000000
  really_probe.part.0 from __driver_probe_device+0xb8/0x14c
  r7:c27c0810 r6:c1a2d354 r5:00000000 r4:c27c0810
  __driver_probe_device from driver_probe_device+0x44/0x11c
  r7:c27c0810 r6:c27c0810 r5:c2137c58 r4:c2137c54
  driver_probe_device from __device_attach_driver+0xc8/0x10c
  r9:c1aa8e20 r8:c242c000 r7:00000000 r6:c27c0810 r5:df831e6c r4:c1a2d354
  __device_attach_driver from bus_for_each_drv+0x90/0xdc
  r7:00000000 r6:c09440f8 r5:df831e6c r4:00000000
  bus_for_each_drv from __device_attach+0xbc/0x1d4
  r6:c27c0854 r5:00000001 r4:c27c0810
  __device_attach from device_initial_probe+0x1c/0x20
  r6:c1a30df8 r5:c27c0810 r4:c27c0810
  device_initial_probe from bus_probe_device+0x98/0xa0
  bus_probe_device from deferred_probe_work_func+0x8c/0xbc
  r7:00000000 r6:c1a309e8 r5:c1a3099c r4:c27c0810
  deferred_probe_work_func from process_one_work+0x2b8/0x774
  r7:c25c8000 r6:c2407000 r5:c2557480 r4:c1a309f8
  process_one_work from worker_thread+0x17c/0x56c
  r10:00000088 r9:c25c8000 r8:c1905d40 r7:c240703c r6:c2557498 r5:c2407000
  r4:c2557480
  worker_thread from kthread+0x108/0x13c
  r10:00000000 r9:df815e2c r8:c2557500 r7:c2557480 r6:c0151924 r5:c25c8000
  r4:c2554540
  kthread from ret_from_fork+0x14/0x28
 Exception stack(0xdf831fb0 to 0xdf831ff8)
 1fa0:                                     00000000 00000000 00000000
00000000
 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
  r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c015ba68 r4:c2554540
 Code: e24cc004 ea000001 e1530006 0a000007 (e5be5004)
 ---[ end trace 0000000000000000 ]---


-- 
Best regards,
Dmitry

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

On 07-07-22, 22:43, Dmitry Osipenko wrote:
> This patch breaks Tegra again, please take a look:

Damn, not again :(

>    OPP: Remove dev{m}_pm_opp_of_add_table_noclk()

Why did you mention this patch ? This just removed an unused API,
Tegra should have broke because of something else, isn't it ?

>  8<--- cut here ---
>  Unable to handle kernel paging request at virtual address ffffffff
>  [ffffffff] *pgd=9effd861, *pte=00000000, *ppte=00000000
>  Internal error: Oops: 37 [#1] PREEMPT SMP ARM
>  Modules linked in:
>  CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted
> 5.19.0-rc1-00040-g30b62d123f4f #82
>  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>  Workqueue: events_unbound deferred_probe_work_func
>  PC is at _opp_compare_key+0x40/0xc4
>  LR is at 0xfffffffb

How is LR set to such an address ?

>  pc : [<c0b91b54>]    lr : [<fffffffb>]    psr: 20000113
>  sp : df831b08  ip : c33cd4d0  fp : df831b24
>  r10: c2586078  r9 : c258606c  r8 : 00000000
>  r7 : 00000000  r6 : 00000001  r5 : c33cd480  r4 : c2586000
>  r3 : 00000000  r2 : c33cd480  r1 : c258606c  r0 : c2586000
>  Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>  Control: 10c5387d  Table: 8000404a  DAC: 00000051
> ...
>  Backtrace:
>   _opp_compare_key from _set_opp+0x80/0x408

Whatever happened, happened from _opp_compare_key() and I tried to
look at it many times, couldn't figure out what's wrong there.

For the device in question, pmc I think, we don't have any "opp-hz"
property in the DT, but still the OPP core will fetch its clock and
set clk_count to 1. But this was working earlier too, we were
comparing the rate anyways. I think one of _opp_compare_rate() or
_opp_compare_bw() is broken here, but I just couldn't figure out. The
rate one should run one loop and bw one should just return. I don't
see why a crash should come out eventually.

Can you help debug this a bit ? Also see what are the values of
opp_table->path_count and opp_table->clk_count, should be 0 and 1
AFAICT.

Sorry about this Dmitry, I think we are all settled and again went
into crap.

-- 
viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Dmitry Osipenko]

On 7/8/22 10:19, Viresh Kumar wrote:
> On 07-07-22, 22:43, Dmitry Osipenko wrote:
>> This patch breaks Tegra again, please take a look:
> 
> Damn, not again :(
> 
>>    OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
> 
> Why did you mention this patch ? This just removed an unused API,
> Tegra should have broke because of something else, isn't it ?

This patch is the cause.

>>  8<--- cut here ---
>>  Unable to handle kernel paging request at virtual address ffffffff
>>  [ffffffff] *pgd=9effd861, *pte=00000000, *ppte=00000000
>>  Internal error: Oops: 37 [#1] PREEMPT SMP ARM
>>  Modules linked in:
>>  CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted
>> 5.19.0-rc1-00040-g30b62d123f4f #82
>>  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>>  Workqueue: events_unbound deferred_probe_work_func
>>  PC is at _opp_compare_key+0x40/0xc4
>>  LR is at 0xfffffffb
> 
> How is LR set to such an address ?
> 
>>  pc : [<c0b91b54>]    lr : [<fffffffb>]    psr: 20000113
>>  sp : df831b08  ip : c33cd4d0  fp : df831b24
>>  r10: c2586078  r9 : c258606c  r8 : 00000000
>>  r7 : 00000000  r6 : 00000001  r5 : c33cd480  r4 : c2586000
>>  r3 : 00000000  r2 : c33cd480  r1 : c258606c  r0 : c2586000
>>  Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>>  Control: 10c5387d  Table: 8000404a  DAC: 00000051
>> ...
>>  Backtrace:
>>   _opp_compare_key from _set_opp+0x80/0x408
> 
> Whatever happened, happened from _opp_compare_key() and I tried to
> look at it many times, couldn't figure out what's wrong there.
> 
> For the device in question, pmc I think, we don't have any "opp-hz"
> property in the DT, but still the OPP core will fetch its clock and
> set clk_count to 1. But this was working earlier too, we were
> comparing the rate anyways. I think one of _opp_compare_rate() or
> _opp_compare_bw() is broken here, but I just couldn't figure out. The
> rate one should run one loop and bw one should just return. I don't
> see why a crash should come out eventually.
> 
> Can you help debug this a bit ? Also see what are the values of
> opp_table->path_count and opp_table->clk_count, should be 0 and 1
> AFAICT.

I see that previously dev_pm_opp_set_config() had "_add_opp_table(dev,
false)", now it's "_add_opp_table(dev, true)".

Will take a closer look later on.

> Sorry about this Dmitry, I think we are all settled and again went
> into crap.

No problems :)

-- 
Best regards,
Dmitry

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Dmitry Osipenko]

On 7/8/22 10:26, Dmitry Osipenko wrote:
> On 7/8/22 10:19, Viresh Kumar wrote:
>> On 07-07-22, 22:43, Dmitry Osipenko wrote:
>>> This patch breaks Tegra again, please take a look:
>>
>> Damn, not again :(
>>
>>>    OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
>>
>> Why did you mention this patch ? This just removed an unused API,
>> Tegra should have broke because of something else, isn't it ?
> 
> This patch is the cause.
> 
>>>  8<--- cut here ---
>>>  Unable to handle kernel paging request at virtual address ffffffff
>>>  [ffffffff] *pgd=9effd861, *pte=00000000, *ppte=00000000
>>>  Internal error: Oops: 37 [#1] PREEMPT SMP ARM
>>>  Modules linked in:
>>>  CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted
>>> 5.19.0-rc1-00040-g30b62d123f4f #82
>>>  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>>>  Workqueue: events_unbound deferred_probe_work_func
>>>  PC is at _opp_compare_key+0x40/0xc4
>>>  LR is at 0xfffffffb
>>
>> How is LR set to such an address ?
>>
>>>  pc : [<c0b91b54>]    lr : [<fffffffb>]    psr: 20000113
>>>  sp : df831b08  ip : c33cd4d0  fp : df831b24
>>>  r10: c2586078  r9 : c258606c  r8 : 00000000
>>>  r7 : 00000000  r6 : 00000001  r5 : c33cd480  r4 : c2586000
>>>  r3 : 00000000  r2 : c33cd480  r1 : c258606c  r0 : c2586000
>>>  Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>>>  Control: 10c5387d  Table: 8000404a  DAC: 00000051
>>> ...
>>>  Backtrace:
>>>   _opp_compare_key from _set_opp+0x80/0x408
>>
>> Whatever happened, happened from _opp_compare_key() and I tried to
>> look at it many times, couldn't figure out what's wrong there.
>>
>> For the device in question, pmc I think, we don't have any "opp-hz"
>> property in the DT, but still the OPP core will fetch its clock and
>> set clk_count to 1. But this was working earlier too, we were
>> comparing the rate anyways. I think one of _opp_compare_rate() or
>> _opp_compare_bw() is broken here, but I just couldn't figure out. The
>> rate one should run one loop and bw one should just return. I don't
>> see why a crash should come out eventually.
>>
>> Can you help debug this a bit ? Also see what are the values of
>> opp_table->path_count and opp_table->clk_count, should be 0 and 1
>> AFAICT.
> 
> I see that previously dev_pm_opp_set_config() had "_add_opp_table(dev,
> false)", now it's "_add_opp_table(dev, true)".
> 
> Will take a closer look later on.
> 
>> Sorry about this Dmitry, I think we are all settled and again went
>> into crap.
> 
> No problems :)

BTW, maybe we should consider to start adding kselftests for OPP, like
clk framework did. That will be handy to have given that it's not easy
to test the whole OPP core without having specific devices.

-- 
Best regards,
Dmitry

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

On 08-07-22, 10:26, Dmitry Osipenko wrote:
> On 7/8/22 10:19, Viresh Kumar wrote:
> > On 07-07-22, 22:43, Dmitry Osipenko wrote:
> >> This patch breaks Tegra again, please take a look:
> > 
> > Damn, not again :(
> > 
> >>    OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
> > 
> > Why did you mention this patch ? This just removed an unused API,
> > Tegra should have broke because of something else, isn't it ?
> 
> This patch is the cause.

I was tracking the crash too closely it seems. :(

> I see that previously dev_pm_opp_set_config() had "_add_opp_table(dev,
> false)", now it's "_add_opp_table(dev, true)".

That's definitely a mistake, I still don't understand though how it
can lead to the crash we got.

I have fixed this in my tree now, can you check again please.

-- 
viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

On 08-07-22, 10:30, Dmitry Osipenko wrote:
> BTW, maybe we should consider to start adding kselftests for OPP, like
> clk framework did. That will be handy to have given that it's not easy
> to test the whole OPP core without having specific devices.

After being regularly bitten by such issues, I added some for cpufreq
earlier. Its time that I invest some time for OPP core too I think :)

I don't know though when I will be able to find time for that :(

-- 
viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Dmitry Osipenko]

On 7/8/22 11:12, Viresh Kumar wrote:
> On 08-07-22, 10:26, Dmitry Osipenko wrote:
>> On 7/8/22 10:19, Viresh Kumar wrote:
>>> On 07-07-22, 22:43, Dmitry Osipenko wrote:
>>>> This patch breaks Tegra again, please take a look:
>>>
>>> Damn, not again :(
>>>
>>>>    OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
>>>
>>> Why did you mention this patch ? This just removed an unused API,
>>> Tegra should have broke because of something else, isn't it ?
>>
>> This patch is the cause.
> 
> I was tracking the crash too closely it seems. :(
> 
>> I see that previously dev_pm_opp_set_config() had "_add_opp_table(dev,
>> false)", now it's "_add_opp_table(dev, true)".
> 
> That's definitely a mistake, I still don't understand though how it
> can lead to the crash we got.

I'll investigate it.

> I have fixed this in my tree now, can you check again please.
> 

Yours tree works, thank you.

-- 
Best regards,
Dmitry

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Johan Hovold]

On Tue, Jul 05, 2022 at 12:30:03PM +0530, Viresh Kumar wrote:
> Hello,
> 
> This patchset adds support for devices with multiple clocks. None of the clocks
> is considered primary in this case and all are handled equally.
> 
> The drivers, for multiple clock case, are expected to call dev_pm_opp_set_opp()
> to set the specific OPP. Though how they find the target OPP is left for the
> users to handle. For some, we may have another unique OPP property, like level,
> which can be used to find the OPP. While in case of others, we may want to
> implement freq-based OPP finder APIs for multiple clock rates. I have decided
> not to implement them in advance, and add them only someone wants to use them.

This break OPP parsing on SC8280XP and hence cpufreq and other things:

[  +0.010890] cpu cpu0: _opp_add_static_v2: opp key field not found
[  +0.000019] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -19
[  +0.000060] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 403200000, volt: 576000, enabled: 1
[  +0.000030] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 499200000, volt: 576000, enabled: 1
...

I just did a rebase on next-20220708 and hit this.

I've narrowed it down to _read_rate() now returning -ENODEV since
opp_table->clk_count is zero.

Similar to what was reported for tegra for v1:

	https://lore.kernel.org/all/58cc8e3c-74d4-e432-8502-299312a1f15e@collabora.com/

I don't have time to look at this any more today, but it would we nice
if you could unbreak linux-next.

Perhaps Bjorn or Mani can help with further details, but this doesn't
look like something that is specific to SC8280XP.

Johan

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

On 11-07-22, 18:40, Johan Hovold wrote:
> This break OPP parsing on SC8280XP and hence cpufreq and other things:
> 
> [  +0.010890] cpu cpu0: _opp_add_static_v2: opp key field not found
> [  +0.000019] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -19
> [  +0.000060] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 403200000, volt: 576000, enabled: 1
> [  +0.000030] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 499200000, volt: 576000, enabled: 1
> ...
> 
> I just did a rebase on next-20220708 and hit this.
> 
> I've narrowed it down to _read_rate() now returning -ENODEV since
> opp_table->clk_count is zero.
> 
> Similar to what was reported for tegra for v1:
> 
> 	https://lore.kernel.org/all/58cc8e3c-74d4-e432-8502-299312a1f15e@collabora.com/
> 
> I don't have time to look at this any more today, but it would we nice
> if you could unbreak linux-next.
> 
> Perhaps Bjorn or Mani can help with further details, but this doesn't
> look like something that is specific to SC8280XP.

It is actually. This is yet another corner case, Tegra had one as
well.

I have tried to understand the Qcom code / setup to best of my
abilities, and the problem as per me is that qcom-cpufreq-hw doesn't
provide a clk to the OPP core, which breaks it after the new updates
to the OPP core. I believe following will solve it. Can someone please
try this ? I will then merge it with the right commit.

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 666e1ebf91d1..4f4a285886fa 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
        }

        if (ret == -ENOENT) {
+               /*
+                * There are few platforms which don't want the OPP core to
+                * manage device's clock settings. In such cases neither the
+                * platform provides the clks explicitly to us, nor the DT
+                * contains a valid clk entry. The OPP nodes in DT may still
+                * contain "opp-hz" property though, which we need to parse and
+                * allow the platform to find an OPP based on freq later on.
+                *
+                * This is a simple solution to take care of such corner cases,
+                * i.e. make the clk_count 1, which lets us allocate space for
+                * frequency in opp->rates and also parse the entries in DT.
+                */
+               opp_table->clk_count = 1;
+
                dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
                return opp_table;
        }

-- 
viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Manivannan Sadhasivam]

On Tue, Jul 12, 2022 at 01:22:40PM +0530, Viresh Kumar wrote:
> On 11-07-22, 18:40, Johan Hovold wrote:
> > This break OPP parsing on SC8280XP and hence cpufreq and other things:
> > 
> > [  +0.010890] cpu cpu0: _opp_add_static_v2: opp key field not found
> > [  +0.000019] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -19
> > [  +0.000060] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 403200000, volt: 576000, enabled: 1
> > [  +0.000030] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 499200000, volt: 576000, enabled: 1
> > ...
> > 
> > I just did a rebase on next-20220708 and hit this.
> > 
> > I've narrowed it down to _read_rate() now returning -ENODEV since
> > opp_table->clk_count is zero.
> > 
> > Similar to what was reported for tegra for v1:
> > 
> > 	https://lore.kernel.org/all/58cc8e3c-74d4-e432-8502-299312a1f15e@collabora.com/
> > 
> > I don't have time to look at this any more today, but it would we nice
> > if you could unbreak linux-next.
> > 
> > Perhaps Bjorn or Mani can help with further details, but this doesn't
> > look like something that is specific to SC8280XP.
> 
> It is actually. This is yet another corner case, Tegra had one as
> well.
> 
> I have tried to understand the Qcom code / setup to best of my
> abilities, and the problem as per me is that qcom-cpufreq-hw doesn't
> provide a clk to the OPP core, which breaks it after the new updates
> to the OPP core. I believe following will solve it. Can someone please
> try this ? I will then merge it with the right commit.
> 

I gave it a shot on Lenovo X13s based on SC8280 SoC and it fixes the OPP
issue. So you can add,

Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 666e1ebf91d1..4f4a285886fa 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
>         }
> 
>         if (ret == -ENOENT) {
> +               /*
> +                * There are few platforms which don't want the OPP core to
> +                * manage device's clock settings. In such cases neither the
> +                * platform provides the clks explicitly to us, nor the DT
> +                * contains a valid clk entry. The OPP nodes in DT may still
> +                * contain "opp-hz" property though, which we need to parse and
> +                * allow the platform to find an OPP based on freq later on.
> +                *
> +                * This is a simple solution to take care of such corner cases,
> +                * i.e. make the clk_count 1, which lets us allocate space for
> +                * frequency in opp->rates and also parse the entries in DT.
> +                */
> +               opp_table->clk_count = 1;
> +
>                 dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
>                 return opp_table;
>         }
> 
> -- 
> viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Johan Hovold]

On Tue, Jul 12, 2022 at 01:22:40PM +0530, Viresh Kumar wrote:
> On 11-07-22, 18:40, Johan Hovold wrote:
> > This break OPP parsing on SC8280XP and hence cpufreq and other things:
> > 
> > [  +0.010890] cpu cpu0: _opp_add_static_v2: opp key field not found
> > [  +0.000019] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -19
> > [  +0.000060] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 403200000, volt: 576000, enabled: 1
> > [  +0.000030] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 499200000, volt: 576000, enabled: 1
> > ...
> > 
> > I just did a rebase on next-20220708 and hit this.
> > 
> > I've narrowed it down to _read_rate() now returning -ENODEV since
> > opp_table->clk_count is zero.
> > 
> > Similar to what was reported for tegra for v1:
> > 
> > 	https://lore.kernel.org/all/58cc8e3c-74d4-e432-8502-299312a1f15e@collabora.com/
> > 
> > I don't have time to look at this any more today, but it would we nice
> > if you could unbreak linux-next.
> > 
> > Perhaps Bjorn or Mani can help with further details, but this doesn't
> > look like something that is specific to SC8280XP.
> 
> It is actually. This is yet another corner case, Tegra had one as
> well.

I literally meant that it does not appear to be SC8280XP specific. Bjorn
reported seeing similar problems on multiple Qualcomm SoCs.

> I have tried to understand the Qcom code / setup to best of my
> abilities, and the problem as per me is that qcom-cpufreq-hw doesn't
> provide a clk to the OPP core, which breaks it after the new updates
> to the OPP core. I believe following will solve it. Can someone please
> try this ? I will then merge it with the right commit.
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 666e1ebf91d1..4f4a285886fa 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
>         }
> 
>         if (ret == -ENOENT) {
> +               /*
> +                * There are few platforms which don't want the OPP core to
> +                * manage device's clock settings. In such cases neither the
> +                * platform provides the clks explicitly to us, nor the DT
> +                * contains a valid clk entry. The OPP nodes in DT may still
> +                * contain "opp-hz" property though, which we need to parse and
> +                * allow the platform to find an OPP based on freq later on.
> +                *
> +                * This is a simple solution to take care of such corner cases,
> +                * i.e. make the clk_count 1, which lets us allocate space for
> +                * frequency in opp->rates and also parse the entries in DT.
> +                */
> +               opp_table->clk_count = 1;
> +
>                 dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
>                 return opp_table;
>         }

This looks like a hack. And it also triggers a bunch of new warning when
opp is trying to create debugfs entries for an entirely different table
which now gets clk_count set to 1:

[  +0.000979]  cx: _update_opp_table_clk: Couldn't find clock: -2
[  +0.000022] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
[  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!

This is for the rpmhpd whose opp table does not have either opp-hz or
clocks (just opp-level).

The above unbreaks cpufreq though.

Johan

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

On 12-07-22, 16:29, Johan Hovold wrote:
> On Tue, Jul 12, 2022 at 01:22:40PM +0530, Viresh Kumar wrote:
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 666e1ebf91d1..4f4a285886fa 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
> >         }
> > 
> >         if (ret == -ENOENT) {
> > +               /*
> > +                * There are few platforms which don't want the OPP core to
> > +                * manage device's clock settings. In such cases neither the
> > +                * platform provides the clks explicitly to us, nor the DT
> > +                * contains a valid clk entry. The OPP nodes in DT may still
> > +                * contain "opp-hz" property though, which we need to parse and
> > +                * allow the platform to find an OPP based on freq later on.
> > +                *
> > +                * This is a simple solution to take care of such corner cases,
> > +                * i.e. make the clk_count 1, which lets us allocate space for
> > +                * frequency in opp->rates and also parse the entries in DT.
> > +                */
> > +               opp_table->clk_count = 1;
> > +
> >                 dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
> >                 return opp_table;
> >         }
> 
> This looks like a hack.

Yeah, a bit. Initially I wanted to solve it in a cleaner way, like it
is done for Tegra, where you will pass the right clock name to the OPP
core, so it can verify that the clk is there and parse the table. And
then tell the OPP core not to configure the clk from
dev_pm_opp_set_opp(), which is possible now. This would have done the
things in the right way.

The problem with Qcom's DT is that the CPU node have the OPP table but
doesn't contain the clocks, which are available with the
qcom,cpufreq-hw node instead. Because of this, I couldn't pass the
real clocks name to the OPP core, "xo", for the CPU device.

I really tried to avoid adding the above code for Tegra and found a
better and cleaner way out. But I couldn't do the same here and
figured it may be more generic of a problem, which is fine as well.

The OPP core does two things currently:

1) Parse the DT and provide helpers to find the right OPP, etc.

2) Provide generic helper to configure all resources related to the
   OPP.

It is fine if some platforms only want to have the first and not the
second. To have the second though, you need to have the first as well.

The clk is required only for the second case, and the OPP core should
parse the DT anyways, irrespective of the availability of the clock.
Because of this reason, making the above change looked reasonable
(this is what was happening before my new patches came in anyway). The
clock isn't there, but there is "opp-hz" present in the DT, which
needs to be parsed.

> And it also triggers a bunch of new warning when
> opp is trying to create debugfs entries for an entirely different table
> which now gets clk_count set to 1:
> 
> [  +0.000979]  cx: _update_opp_table_clk: Couldn't find clock: -2
> [  +0.000022] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> 
> This is for the rpmhpd whose opp table does not have either opp-hz or
> clocks (just opp-level).

Ahh, I missed switching back to the earlier code here. i.e. not use
the frequency for OPP directory's name, when it is 0.

This will fix it.

diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
index 402c507edac7..96a30a032c5f 100644
--- a/drivers/opp/debugfs.c
+++ b/drivers/opp/debugfs.c
@@ -138,7 +138,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
         * - For some devices rate isn't available or there are multiple, use
         *   index instead for them.
         */
-       if (likely(opp_table->clk_count == 1))
+       if (likely(opp_table->clk_count == 1 && opp->rates[0]))
                id = opp->rates[0];
        else
                id = _get_opp_count(opp_table);

I have merged this into:

commit 341df9889277 ("OPP: Allow multiple clocks for a device")

and pushed out for linux-next.


Bjorn, Mani,

It would be really good if we can find a way to make following work on
Qcom:

        clk_get(cpu_dev, NULL or "xo")

If that happens, we can handle the special case just at the consumer
driver (qcom-cpufreq-hw) and not in the core.

-- 
viresh

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Johan Hovold]

On Tue, Jul 12, 2022 at 08:40:45PM +0530, Viresh Kumar wrote:
> On 12-07-22, 16:29, Johan Hovold wrote:
> > On Tue, Jul 12, 2022 at 01:22:40PM +0530, Viresh Kumar wrote:
> > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > > index 666e1ebf91d1..4f4a285886fa 100644
> > > --- a/drivers/opp/core.c
> > > +++ b/drivers/opp/core.c
> > > @@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
> > >         }
> > > 
> > >         if (ret == -ENOENT) {
> > > +               /*
> > > +                * There are few platforms which don't want the OPP core to
> > > +                * manage device's clock settings. In such cases neither the
> > > +                * platform provides the clks explicitly to us, nor the DT
> > > +                * contains a valid clk entry. The OPP nodes in DT may still
> > > +                * contain "opp-hz" property though, which we need to parse and
> > > +                * allow the platform to find an OPP based on freq later on.
> > > +                *
> > > +                * This is a simple solution to take care of such corner cases,
> > > +                * i.e. make the clk_count 1, which lets us allocate space for
> > > +                * frequency in opp->rates and also parse the entries in DT.
> > > +                */
> > > +               opp_table->clk_count = 1;
> > > +
> > >                 dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
> > >                 return opp_table;
> > >         }
> > 
> > This looks like a hack.
> 
> Yeah, a bit. Initially I wanted to solve it in a cleaner way, like it
> is done for Tegra, where you will pass the right clock name to the OPP
> core, so it can verify that the clk is there and parse the table. And
> then tell the OPP core not to configure the clk from
> dev_pm_opp_set_opp(), which is possible now. This would have done the
> things in the right way.
> 
> The problem with Qcom's DT is that the CPU node have the OPP table but
> doesn't contain the clocks, which are available with the
> qcom,cpufreq-hw node instead. Because of this, I couldn't pass the
> real clocks name to the OPP core, "xo", for the CPU device.
> 
> I really tried to avoid adding the above code for Tegra and found a
> better and cleaner way out. But I couldn't do the same here and
> figured it may be more generic of a problem, which is fine as well.
> 
> The OPP core does two things currently:
> 
> 1) Parse the DT and provide helpers to find the right OPP, etc.
> 
> 2) Provide generic helper to configure all resources related to the
>    OPP.
> 
> It is fine if some platforms only want to have the first and not the
> second. To have the second though, you need to have the first as well.
> 
> The clk is required only for the second case, and the OPP core should
> parse the DT anyways, irrespective of the availability of the clock.
> Because of this reason, making the above change looked reasonable
> (this is what was happening before my new patches came in anyway). The
> clock isn't there, but there is "opp-hz" present in the DT, which
> needs to be parsed.

Ok, thanks for the details. I'd still look into if there's some way to
avoid setting clk_count when there are no clocks as it sounds like an
anti-pattern that will just make the code harder to understand and
maintain.

> > And it also triggers a bunch of new warning when
> > opp is trying to create debugfs entries for an entirely different table
> > which now gets clk_count set to 1:
> > 
> > [  +0.000979]  cx: _update_opp_table_clk: Couldn't find clock: -2
> > [  +0.000022] debugfs: Directory 'opp:0' with parent 'cx' already present!

> > This is for the rpmhpd whose opp table does not have either opp-hz or
> > clocks (just opp-level).
> 
> Ahh, I missed switching back to the earlier code here. i.e. not use
> the frequency for OPP directory's name, when it is 0.
> 
> This will fix it.
> 
> diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
> index 402c507edac7..96a30a032c5f 100644
> --- a/drivers/opp/debugfs.c
> +++ b/drivers/opp/debugfs.c
> @@ -138,7 +138,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
>          * - For some devices rate isn't available or there are multiple, use
>          *   index instead for them.
>          */
> -       if (likely(opp_table->clk_count == 1))
> +       if (likely(opp_table->clk_count == 1 && opp->rates[0]))
>                 id = opp->rates[0];
>         else
>                 id = _get_opp_count(opp_table);

Indeed it does, thanks.

> I have merged this into:
> 
> commit 341df9889277 ("OPP: Allow multiple clocks for a device")
> 
> and pushed out for linux-next.

Thanks for addressing this quickly. With the two patches above applied,
the issues I noticed are gone.

Johan

Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

[Viresh Kumar]

On 12-07-22, 17:55, Johan Hovold wrote:
> Ok, thanks for the details. I'd still look into if there's some way to
> avoid setting clk_count when there are no clocks as it sounds like an
> anti-pattern that will just make the code harder to understand and
> maintain.

Here is an attempt from me :)

https://lore.kernel.org/lkml/cover.1657695140.git.viresh.kumar@linaro.org/

-- 
viresh