[PATCH 0/5] OPP: Replace custom set_opp() with config_regulators()

[PATCH 0/5] OPP: Replace custom set_opp() with config_regulators()

[Viresh Kumar]

Hi,

This series is in continuation to OPP cleanups [1] posted earlier and is rebased over them.

Currently the custom set_opp() helper, which is implemented only for omap, is responsible to set
both clock and regulators for the device and may end up doing tricky stuff behind the scene. This
makes the OPP core contain special code to support it.

This patch series tries to streamline the code path in _set_opp() in the OPP core and minimize the
platform specific code within it. The platforms provide a config_regulators() callback now, from
which they should only program the regulators in their preferred sequence. Rest of the code sequence
to program clk, bw, required-opps, etc is common across all device and platform types and is present
in the OPP core.

Keerthy/Dave: I couldn't test it on omap, can any of you do that please ? It builds just fine
though. Also maybe you can simplify the OPP driver to drop all restoration logic on failures, as the
OPP core doesn't do any of it as well. I can add a patch for that if you guys are fine with it.

This is pushed here along with other dependencies:

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

Thanks.

--
Viresh

[1] https://lore.kernel.org/lkml/cover.1653564321.git.viresh.kumar@linaro.org/

Viresh Kumar (5):
  OPP: Add support for config_regulators() helper
  OPP: Make _generic_set_opp_regulator() a config_regulators() interface
  OPP: Add dev_pm_opp_get_supplies()
  OPP: ti: Migrate to config_regulators()
  OPP: Remove custom OPP helper support

 drivers/opp/core.c          | 204 +++++++++++++-----------------------
 drivers/opp/opp.h           |   9 +-
 drivers/opp/ti-opp-supply.c |  74 ++++++-------
 include/linux/pm_opp.h      |  47 +++------
 4 files changed, 118 insertions(+), 216 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[PATCH 1/5] OPP: Add support for config_regulators() helper

[Viresh Kumar]

Extend the dev_pm_opp_set_config() interface to allow adding
config_regulators() helpers. This helper will be called to set the
voltages of the regulators from the regular path in _set_opp(), while we
are trying to change the OPP.

This will eventually replace the custom set_opp() helper.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 63 ++++++++++++++++++++++++++++++++++++++++++
 drivers/opp/opp.h      |  2 ++
 include/linux/pm_opp.h |  5 ++++
 3 files changed, 70 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 62c5d4308e01..bac91d078cbb 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1181,6 +1181,15 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 			dev_err(dev, "Failed to set bw: %d\n", ret);
 			return ret;
 		}
+
+		if (opp_table->config_regulators) {
+			ret = opp_table->config_regulators(dev, old_opp, opp, opp_table->regulators,
+							   opp_table->regulator_count);
+			if (ret) {
+				dev_err(dev, "Failed to set regulator voltages: %d\n", ret);
+				return ret;
+			}
+		}
 	}
 
 	if (opp_table->set_opp) {
@@ -1198,6 +1207,15 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 
 	/* Scaling down? Configure required OPPs after frequency */
 	if (scaling_down) {
+		if (opp_table->config_regulators) {
+			ret = opp_table->config_regulators(dev, old_opp, opp, opp_table->regulators,
+							   opp_table->regulator_count);
+			if (ret) {
+				dev_err(dev, "Failed to set regulator voltages: %d\n", ret);
+				return ret;
+			}
+		}
+
 		ret = _set_opp_bw(opp_table, opp, dev);
 		if (ret) {
 			dev_err(dev, "Failed to set bw: %d\n", ret);
@@ -2237,6 +2255,43 @@ static void _opp_unregister_set_opp_helper(struct opp_table *opp_table)
 	}
 }
 
+/**
+ * _opp_set_config_regulators_helper() - Register custom set regulator helper.
+ * @dev: Device for which the helper is getting registered.
+ * @config_regulators: Custom set regulator helper.
+ *
+ * This is useful to support platforms with multiple regulators per device.
+ *
+ * This must be called before any OPPs are initialized for the device.
+ */
+static int _opp_set_config_regulators_helper(struct opp_table *opp_table, struct device *dev,
+					     config_regulators_t config_regulators)
+{
+	/* Another CPU that shares the OPP table has set the helper ? */
+	if (!opp_table->config_regulators) {
+		mutex_lock(&opp_table->lock);
+		opp_table->config_regulators = config_regulators;
+		mutex_unlock(&opp_table->lock);
+	}
+
+	return 0;
+}
+
+/**
+ * _opp_put_config_regulators_helper() - Releases resources blocked for config_regulators helper.
+ * @opp_table: OPP table returned from _opp_set_config_regulators_helper().
+ *
+ * Release resources blocked for platform specific config_regulators helper.
+ */
+static void _opp_put_config_regulators_helper(struct opp_table *opp_table)
+{
+	if (opp_table->config_regulators) {
+		mutex_lock(&opp_table->lock);
+		opp_table->config_regulators = NULL;
+		mutex_unlock(&opp_table->lock);
+	}
+}
+
 static void _detach_genpd(struct opp_table *opp_table)
 {
 	int index;
@@ -2404,6 +2459,13 @@ struct opp_table *dev_pm_opp_set_config(struct device *dev,
 			goto err;
 	}
 
+	/* Configure config_regulators helper */
+	if (config->config_regulators) {
+		ret = _opp_set_config_regulators_helper(opp_table, dev, config->config_regulators);
+		if (ret)
+			goto err;
+	}
+
 	/* Configure supported hardware */
 	if (config->supported_hw) {
 		ret = _opp_set_supported_hw(opp_table, config->supported_hw,
@@ -2458,6 +2520,7 @@ void dev_pm_opp_clear_config(struct opp_table *opp_table)
 	_opp_detach_genpd(opp_table);
 	_opp_put_regulators(opp_table);
 	_opp_put_supported_hw(opp_table);
+	_opp_put_config_regulators_helper(opp_table);
 	_opp_unregister_set_opp_helper(opp_table);
 	_opp_put_prop_name(opp_table);
 	_opp_put_clkname(opp_table);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 9e1cfcb0ea98..4695d315e7f9 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -151,6 +151,7 @@ enum opp_table_access {
  * @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_regulators: Platform specific config_regulators() callback.
  * @regulators: Supply regulators
  * @regulator_count: Number of power supply regulators. Its value can be -1
  * (uninitialized), 0 (no opp-microvolt property) or > 0 (has opp-microvolt
@@ -203,6 +204,7 @@ struct opp_table {
 	const char *prop_name;
 	bool clk_configured;
 	struct clk *clk;
+	config_regulators_t config_regulators;
 	struct regulator **regulators;
 	int regulator_count;
 	struct icc_path **paths;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 6cf1bbc71ed2..7e7986106274 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -90,11 +90,15 @@ struct dev_pm_set_opp_data {
 	struct device *dev;
 };
 
+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);
 /**
  * struct dev_pm_opp_config - Device OPP configuration values
  * @clk_name: Clk name.
  * @prop_name: Name to postfix to properties.
  * @set_opp: Custom set OPP helper.
+ * @config_regulators: Custom set regulator helper.
  * @supported_hw: Array of hierarchy of versions to match.
  * @supported_hw_count: Number of elements in the array.
  * @regulator_names: Array of pointers to the names of the regulator.
@@ -108,6 +112,7 @@ struct dev_pm_opp_config {
 	const char *clk_name;
 	const char *prop_name;
 	int (*set_opp)(struct dev_pm_set_opp_data *data);
+	config_regulators_t config_regulators;
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
 	const char * const *regulator_names;
-- 
2.31.1.272.g89b43f80a514

[PATCH 2/5] OPP: Make _generic_set_opp_regulator() a config_regulators() interface

[Viresh Kumar]

In order to reuse the same code path, i.e. clk_set_rate() from
_set_opp(), migrate _generic_set_opp_regulator() to implement a
config_regulators() interface.

It is renamed to _opp_config_regulator_single() and is set as the
preferred config_regulators() interface whenever we have a single
regulator available.

Note that this also drops code responsible for restoring the
voltage/freq in case of errors. We aren't handling that properly
currently, restoring only some of the resources while leaving others out
(like bandwidth and required OPPs). It is better to drop all of it
instead of partial restoration.

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

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index bac91d078cbb..2f76442845be 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -892,62 +892,34 @@ static inline int _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
 	return ret;
 }
 
-static int _generic_set_opp_regulator(struct opp_table *opp_table,
-				      struct device *dev,
-				      struct dev_pm_opp *opp,
-				      unsigned long freq,
-				      int scaling_down)
+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)
 {
-	struct regulator *reg = opp_table->regulators[0];
-	struct dev_pm_opp *old_opp = opp_table->current_opp;
+	struct regulator *reg = regulators[0];
 	int ret;
 
 	/* This function only supports single regulator per device */
-	if (WARN_ON(opp_table->regulator_count > 1)) {
+	if (WARN_ON(count > 1)) {
 		dev_err(dev, "multiple regulators are not supported\n");
 		return -EINVAL;
 	}
 
-	/* Scaling up? Scale voltage before frequency */
-	if (!scaling_down) {
-		ret = _set_opp_voltage(dev, reg, opp->supplies);
-		if (ret)
-			goto restore_voltage;
-	}
-
-	/* Change frequency */
-	ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
+	ret = _set_opp_voltage(dev, reg, new_opp->supplies);
 	if (ret)
-		goto restore_voltage;
-
-	/* Scaling down? Scale voltage after frequency */
-	if (scaling_down) {
-		ret = _set_opp_voltage(dev, reg, opp->supplies);
-		if (ret)
-			goto restore_freq;
-	}
+		return ret;
 
 	/*
 	 * Enable the regulator after setting its voltages, otherwise it breaks
 	 * some boot-enabled regulators.
 	 */
-	if (unlikely(!opp_table->enabled)) {
+	if (unlikely(!new_opp->opp_table->enabled)) {
 		ret = regulator_enable(reg);
 		if (ret < 0)
 			dev_warn(dev, "Failed to enable regulator: %d", ret);
 	}
 
 	return 0;
-
-restore_freq:
-	if (_generic_set_opp_clk_only(dev, opp_table->clk, old_opp->rate))
-		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
-			__func__, old_opp->rate);
-restore_voltage:
-	/* This shouldn't harm even if the voltages weren't updated earlier */
-	_set_opp_voltage(dev, reg, old_opp->supplies);
-
-	return ret;
 }
 
 static int _set_opp_bw(const struct opp_table *opp_table,
@@ -1194,9 +1166,6 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 
 	if (opp_table->set_opp) {
 		ret = _set_opp_custom(opp_table, dev, opp, freq);
-	} else if (opp_table->regulators) {
-		ret = _generic_set_opp_regulator(opp_table, dev, opp, freq,
-						 scaling_down);
 	} else {
 		/* Only frequency scaling */
 		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
@@ -2103,6 +2072,10 @@ static int _opp_set_regulators(struct opp_table *opp_table, struct device *dev,
 		opp_table->set_opp_data->old_opp.supplies = supplies;
 		opp_table->set_opp_data->new_opp.supplies = supplies + count;
 	}
+
+	/* Set generic config_regulators() for single regulators here */
+	if (count == 1)
+		opp_table->config_regulators = _opp_config_regulator_single;
 	mutex_unlock(&opp_table->lock);
 
 	return 0;
-- 
2.31.1.272.g89b43f80a514

[PATCH 3/5] OPP: Add dev_pm_opp_get_supplies()

[Viresh Kumar]

We already have an API for getting voltage information for a single
regulator, dev_pm_opp_get_voltage(), but there is nothing available for
multiple regulator case.

This patch adds a new API, dev_pm_opp_get_supplies(), to get all
information related to the supplies for an OPP.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 22 ++++++++++++++++++++++
 include/linux/pm_opp.h |  7 +++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 2f76442845be..58ce1240e808 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -113,6 +113,28 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_voltage);
 
+/**
+ * dev_pm_opp_get_supplies() - Gets the supply information corresponding to an opp
+ * @opp:	opp for which voltage has to be returned for
+ * @supplies:	Placeholder for copying the supply information.
+ *
+ * Return: negative error number on failure, 0 otherwise on success after setting @supplies.
+ *
+ * This can be used for devices with any number of power supplies. The caller must
+ * ensure the @supplies array must contain space for each regulator.
+ */
+int dev_pm_opp_get_supplies(struct dev_pm_opp *opp, struct dev_pm_opp_supply *supplies)
+{
+	if (IS_ERR_OR_NULL(opp) || !supplies) {
+		pr_err("%s: Invalid parameters\n", __func__);
+		return -EINVAL;
+	}
+
+	memcpy(supplies, opp->supplies, sizeof(*supplies) * opp->opp_table->regulator_count);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_supplies);
+
 /**
  * dev_pm_opp_get_power() - Gets the power corresponding to an opp
  * @opp:	opp for which power has to be returned for
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 7e7986106274..185503aab64d 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -128,6 +128,8 @@ void dev_pm_opp_put_opp_table(struct opp_table *opp_table);
 
 unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
 
+int dev_pm_opp_get_supplies(struct dev_pm_opp *opp, struct dev_pm_opp_supply *supplies);
+
 unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp);
 
 unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp);
@@ -216,6 +218,11 @@ static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 	return 0;
 }
 
+static inline int dev_pm_opp_get_supplies(struct dev_pm_opp *opp, struct dev_pm_opp_supply *supplies)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp)
 {
 	return 0;
-- 
2.31.1.272.g89b43f80a514

[PATCH 4/5] OPP: ti: Migrate to config_regulators()

[Viresh Kumar]

The OPP core now provides config_regulators() interface, which needs the
platforms to just set the OPP voltages instead of both clk and voltage.
The clock is set by the OPP core instead and hence reduces code
redundancy.

Migrate the only user of the custom set_opp() interface to
config_regulators().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/ti-opp-supply.c | 74 +++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 40 deletions(-)

diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c
index a30825dc30cf..2aeb084fc379 100644
--- a/drivers/opp/ti-opp-supply.c
+++ b/drivers/opp/ti-opp-supply.c
@@ -36,11 +36,15 @@ struct ti_opp_supply_optimum_voltage_table {
  * @vdd_table:	Optimized voltage mapping table
  * @num_vdd_table: number of entries in vdd_table
  * @vdd_absolute_max_voltage_uv: absolute maximum voltage in UV for the supply
+ * @old_supplies: Placeholder for supplies information for old OPP.
+ * @new_supplies: Placeholder for supplies information for new OPP.
  */
 struct ti_opp_supply_data {
 	struct ti_opp_supply_optimum_voltage_table *vdd_table;
 	u32 num_vdd_table;
 	u32 vdd_absolute_max_voltage_uv;
+	struct dev_pm_opp_supply old_supplies[2];
+	struct dev_pm_opp_supply new_supplies[2];
 };
 
 static struct ti_opp_supply_data opp_data;
@@ -266,27 +270,32 @@ static int _opp_set_voltage(struct device *dev,
 	return 0;
 }
 
-/**
- * ti_opp_supply_set_opp() - do the opp supply transition
- * @data:	information on regulators and new and old opps provided by
- *		opp core to use in transition
- *
- * Return: If successful, 0, else appropriate error value.
- */
-static int ti_opp_supply_set_opp(struct dev_pm_set_opp_data *data)
+/* Do the opp supply transition */
+static int ti_opp_config_regulators(struct device *dev, struct dev_pm_opp *old_opp,
+				    struct dev_pm_opp *new_opp, struct regulator **regulators,
+				    unsigned int count)
 {
-	struct dev_pm_opp_supply *old_supply_vdd = &data->old_opp.supplies[0];
-	struct dev_pm_opp_supply *old_supply_vbb = &data->old_opp.supplies[1];
-	struct dev_pm_opp_supply *new_supply_vdd = &data->new_opp.supplies[0];
-	struct dev_pm_opp_supply *new_supply_vbb = &data->new_opp.supplies[1];
-	struct device *dev = data->dev;
-	unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate;
-	struct clk *clk = data->clk;
-	struct regulator *vdd_reg = data->regulators[0];
-	struct regulator *vbb_reg = data->regulators[1];
+	struct dev_pm_opp_supply *old_supply_vdd = &opp_data.old_supplies[0];
+	struct dev_pm_opp_supply *old_supply_vbb = &opp_data.old_supplies[1];
+	struct dev_pm_opp_supply *new_supply_vdd = &opp_data.new_supplies[0];
+	struct dev_pm_opp_supply *new_supply_vbb = &opp_data.new_supplies[1];
+	struct regulator *vdd_reg = regulators[0];
+	struct regulator *vbb_reg = regulators[1];
+	unsigned long old_freq, freq;
 	int vdd_uv;
 	int ret;
 
+	/* We must have two regulators here */
+	WARN_ON(count != 2);
+
+	/* Fetch supplies and freq information from OPP core */
+	ret = dev_pm_opp_get_supplies(new_opp, opp_data.new_supplies);
+	WARN_ON(ret);
+
+	old_freq = dev_pm_opp_get_freq(old_opp);
+	freq = dev_pm_opp_get_freq(new_opp);
+	WARN_ON(!old_freq || !freq);
+
 	vdd_uv = _get_optimal_vdd_voltage(dev, &opp_data,
 					  new_supply_vdd->u_volt);
 
@@ -303,39 +312,24 @@ static int ti_opp_supply_set_opp(struct dev_pm_set_opp_data *data)
 		ret = _opp_set_voltage(dev, new_supply_vbb, 0, vbb_reg, "vbb");
 		if (ret)
 			goto restore_voltage;
-	}
-
-	/* Change frequency */
-	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",
-		__func__, old_freq, freq);
-
-	ret = clk_set_rate(clk, freq);
-	if (ret) {
-		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
-			ret);
-		goto restore_voltage;
-	}
-
-	/* Scaling down? Scale voltage after frequency */
-	if (freq < old_freq) {
+	} else {
 		ret = _opp_set_voltage(dev, new_supply_vbb, 0, vbb_reg, "vbb");
 		if (ret)
-			goto restore_freq;
+			goto restore_voltage;
 
 		ret = _opp_set_voltage(dev, new_supply_vdd, vdd_uv, vdd_reg,
 				       "vdd");
 		if (ret)
-			goto restore_freq;
+			goto restore_voltage;
 	}
 
 	return 0;
 
-restore_freq:
-	ret = clk_set_rate(clk, old_freq);
-	if (ret)
-		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
-			__func__, old_freq);
 restore_voltage:
+	/* Fetch old supplies information only if required */
+	ret = dev_pm_opp_get_supplies(old_opp, opp_data.old_supplies);
+	WARN_ON(ret);
+
 	/* This shouldn't harm even if the voltages weren't updated earlier */
 	if (old_supply_vdd->u_volt) {
 		ret = _opp_set_voltage(dev, old_supply_vbb, 0, vbb_reg, "vbb");
@@ -383,7 +377,7 @@ static int ti_opp_supply_probe(struct platform_device *pdev)
 	const struct ti_opp_supply_of_data *of_data;
 	int ret = 0;
 	struct dev_pm_opp_config config = {
-		.set_opp = ti_opp_supply_set_opp,
+		.config_regulators = ti_opp_config_regulators,
 	};
 
 	match = of_match_device(ti_opp_supply_of_match, dev);
-- 
2.31.1.272.g89b43f80a514

[PATCH 5/5] OPP: Remove custom OPP helper support

[Viresh Kumar]

The only user of the custom helper is migrated to use
config_regulators() interface. Remove the now unused custom OPP helper
support.

This cleans up _set_opp() and leaves a single code path to be used by
all users.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 126 +----------------------------------------
 drivers/opp/opp.h      |   7 ---
 include/linux/pm_opp.h |  35 ------------
 3 files changed, 2 insertions(+), 166 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 58ce1240e808..554a043bc225 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -972,36 +972,6 @@ static int _set_opp_bw(const struct opp_table *opp_table,
 	return 0;
 }
 
-static int _set_opp_custom(const struct opp_table *opp_table,
-			   struct device *dev, struct dev_pm_opp *opp,
-			   unsigned long freq)
-{
-	struct dev_pm_set_opp_data *data = opp_table->set_opp_data;
-	struct dev_pm_opp *old_opp = opp_table->current_opp;
-	int size;
-
-	/*
-	 * We support this only if dev_pm_opp_set_config() was called
-	 * earlier to set regulators.
-	 */
-	if (opp_table->sod_supplies) {
-		size = sizeof(*old_opp->supplies) * opp_table->regulator_count;
-		memcpy(data->old_opp.supplies, old_opp->supplies, size);
-		memcpy(data->new_opp.supplies, opp->supplies, size);
-		data->regulator_count = opp_table->regulator_count;
-	} else {
-		data->regulator_count = 0;
-	}
-
-	data->regulators = opp_table->regulators;
-	data->clk = opp_table->clk;
-	data->dev = dev;
-	data->old_opp.rate = old_opp->rate;
-	data->new_opp.rate = freq;
-
-	return opp_table->set_opp(data);
-}
-
 static int _set_required_opp(struct device *dev, struct device *pd_dev,
 			     struct dev_pm_opp *opp, int i)
 {
@@ -1186,13 +1156,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		}
 	}
 
-	if (opp_table->set_opp) {
-		ret = _set_opp_custom(opp_table, dev, opp, freq);
-	} else {
-		/* Only frequency scaling */
-		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
-	}
-
+	ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
 	if (ret)
 		return ret;
 
@@ -2054,7 +2018,6 @@ static void _opp_put_prop_name(struct opp_table *opp_table)
 static int _opp_set_regulators(struct opp_table *opp_table, struct device *dev,
 			       const char * const names[], unsigned int count)
 {
-	struct dev_pm_opp_supply *supplies;
 	struct regulator *reg;
 	int ret, i;
 
@@ -2082,20 +2045,8 @@ static int _opp_set_regulators(struct opp_table *opp_table, struct device *dev,
 
 	opp_table->regulator_count = count;
 
-	supplies = kmalloc_array(count * 2, sizeof(*supplies), GFP_KERNEL);
-	if (!supplies) {
-		ret = -ENOMEM;
-		goto free_regulators;
-	}
-
-	mutex_lock(&opp_table->lock);
-	opp_table->sod_supplies = supplies;
-	if (opp_table->set_opp_data) {
-		opp_table->set_opp_data->old_opp.supplies = supplies;
-		opp_table->set_opp_data->new_opp.supplies = supplies + count;
-	}
-
 	/* Set generic config_regulators() for single regulators here */
+	mutex_lock(&opp_table->lock);
 	if (count == 1)
 		opp_table->config_regulators = _opp_config_regulator_single;
 	mutex_unlock(&opp_table->lock);
@@ -2132,16 +2083,6 @@ static void _opp_put_regulators(struct opp_table *opp_table)
 	for (i = opp_table->regulator_count - 1; i >= 0; i--)
 		regulator_put(opp_table->regulators[i]);
 
-	mutex_lock(&opp_table->lock);
-	if (opp_table->set_opp_data) {
-		opp_table->set_opp_data->old_opp.supplies = NULL;
-		opp_table->set_opp_data->new_opp.supplies = NULL;
-	}
-
-	kfree(opp_table->sod_supplies);
-	opp_table->sod_supplies = NULL;
-	mutex_unlock(&opp_table->lock);
-
 	kfree(opp_table->regulators);
 	opp_table->regulators = NULL;
 	opp_table->regulator_count = -1;
@@ -2195,61 +2136,6 @@ static void _opp_put_clkname(struct opp_table *opp_table)
 	}
 }
 
-/**
- * _opp_register_set_opp_helper() - Register custom set OPP helper
- * @dev: Device for which the helper is getting registered.
- * @set_opp: Custom set OPP helper.
- *
- * This is useful to support complex platforms (like platforms with multiple
- * regulators per device), instead of the generic OPP set rate helper.
- *
- * This must be called before any OPPs are initialized for the device.
- */
-static int _opp_register_set_opp_helper(struct opp_table *opp_table,
-	struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data))
-{
-	struct dev_pm_set_opp_data *data;
-
-	/* Another CPU that shares the OPP table has set the helper ? */
-	if (opp_table->set_opp)
-		return 0;
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	mutex_lock(&opp_table->lock);
-	opp_table->set_opp_data = data;
-	if (opp_table->sod_supplies) {
-		data->old_opp.supplies = opp_table->sod_supplies;
-		data->new_opp.supplies = opp_table->sod_supplies +
-					 opp_table->regulator_count;
-	}
-	mutex_unlock(&opp_table->lock);
-
-	opp_table->set_opp = set_opp;
-
-	return 0;
-}
-
-/**
- * _opp_unregister_set_opp_helper() - Releases resources blocked for set_opp helper
- * @opp_table: OPP table returned from _opp_register_set_opp_helper().
- *
- * Release resources blocked for platform specific set_opp helper.
- */
-static void _opp_unregister_set_opp_helper(struct opp_table *opp_table)
-{
-	if (opp_table->set_opp) {
-		opp_table->set_opp = NULL;
-
-		mutex_lock(&opp_table->lock);
-		kfree(opp_table->set_opp_data);
-		opp_table->set_opp_data = NULL;
-		mutex_unlock(&opp_table->lock);
-	}
-}
-
 /**
  * _opp_set_config_regulators_helper() - Register custom set regulator helper.
  * @dev: Device for which the helper is getting registered.
@@ -2447,13 +2333,6 @@ struct opp_table *dev_pm_opp_set_config(struct device *dev,
 			goto err;
 	}
 
-	/* Configure opp helper */
-	if (config->set_opp) {
-		ret = _opp_register_set_opp_helper(opp_table, dev, config->set_opp);
-		if (ret)
-			goto err;
-	}
-
 	/* Configure config_regulators helper */
 	if (config->config_regulators) {
 		ret = _opp_set_config_regulators_helper(opp_table, dev, config->config_regulators);
@@ -2516,7 +2395,6 @@ void dev_pm_opp_clear_config(struct opp_table *opp_table)
 	_opp_put_regulators(opp_table);
 	_opp_put_supported_hw(opp_table);
 	_opp_put_config_regulators_helper(opp_table);
-	_opp_unregister_set_opp_helper(opp_table);
 	_opp_put_prop_name(opp_table);
 	_opp_put_clkname(opp_table);
 
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 4695d315e7f9..407eee9f10ab 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -161,9 +161,6 @@ enum opp_table_access {
  * @enabled: Set to true if the device's resources are enabled/configured.
  * @genpd_performance_state: Device's power domain support performance state.
  * @is_genpd: Marks if the OPP table belongs to a genpd.
- * @set_opp: Platform specific set_opp callback
- * @sod_supplies: Set opp data supplies
- * @set_opp_data: Data to be passed to set_opp callback
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
@@ -213,10 +210,6 @@ struct opp_table {
 	bool genpd_performance_state;
 	bool is_genpd;
 
-	int (*set_opp)(struct dev_pm_set_opp_data *data);
-	struct dev_pm_opp_supply *sod_supplies;
-	struct dev_pm_set_opp_data *set_opp_data;
-
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
 	char dentry_name[NAME_MAX];
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 185503aab64d..c6d8f01ef9fd 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -57,39 +57,6 @@ struct dev_pm_opp_icc_bw {
 	u32 peak;
 };
 
-/**
- * struct dev_pm_opp_info - OPP freq/voltage/current values
- * @rate:	Target clk rate in hz
- * @supplies:	Array of voltage/current values for all power supplies
- *
- * This structure stores the freq/voltage/current values for a single OPP.
- */
-struct dev_pm_opp_info {
-	unsigned long rate;
-	struct dev_pm_opp_supply *supplies;
-};
-
-/**
- * struct dev_pm_set_opp_data - Set OPP data
- * @old_opp:	Old OPP info
- * @new_opp:	New OPP info
- * @regulators:	Array of regulator pointers
- * @regulator_count: Number of regulators
- * @clk:	Pointer to clk
- * @dev:	Pointer to the struct device
- *
- * This structure contains all information required for setting an OPP.
- */
-struct dev_pm_set_opp_data {
-	struct dev_pm_opp_info old_opp;
-	struct dev_pm_opp_info new_opp;
-
-	struct regulator **regulators;
-	unsigned int regulator_count;
-	struct clk *clk;
-	struct device *dev;
-};
-
 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);
@@ -97,7 +64,6 @@ typedef int (*config_regulators_t)(struct device *dev, struct dev_pm_opp *old_op
  * struct dev_pm_opp_config - Device OPP configuration values
  * @clk_name: Clk name.
  * @prop_name: Name to postfix to properties.
- * @set_opp: Custom set OPP helper.
  * @config_regulators: Custom set regulator helper.
  * @supported_hw: Array of hierarchy of versions to match.
  * @supported_hw_count: Number of elements in the array.
@@ -111,7 +77,6 @@ typedef int (*config_regulators_t)(struct device *dev, struct dev_pm_opp *old_op
 struct dev_pm_opp_config {
 	const char *clk_name;
 	const char *prop_name;
-	int (*set_opp)(struct dev_pm_set_opp_data *data);
 	config_regulators_t config_regulators;
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Dmitry Osipenko]

31.05.2022 13:10, Viresh Kumar пишет:
> The only user of the custom helper is migrated to use
> config_regulators() interface. Remove the now unused custom OPP helper
> support.
> 
> This cleans up _set_opp() and leaves a single code path to be used by
> all users.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c     | 126 +----------------------------------------
>  drivers/opp/opp.h      |   7 ---
>  include/linux/pm_opp.h |  35 ------------
>  3 files changed, 2 insertions(+), 166 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 58ce1240e808..554a043bc225 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -972,36 +972,6 @@ static int _set_opp_bw(const struct opp_table *opp_table,
>  	return 0;
>  }
>  
> -static int _set_opp_custom(const struct opp_table *opp_table,
> -			   struct device *dev, struct dev_pm_opp *opp,
> -			   unsigned long freq)
> -{
> -	struct dev_pm_set_opp_data *data = opp_table->set_opp_data;
> -	struct dev_pm_opp *old_opp = opp_table->current_opp;
> -	int size;
> -
> -	/*
> -	 * We support this only if dev_pm_opp_set_config() was called
> -	 * earlier to set regulators.
> -	 */
> -	if (opp_table->sod_supplies) {
> -		size = sizeof(*old_opp->supplies) * opp_table->regulator_count;
> -		memcpy(data->old_opp.supplies, old_opp->supplies, size);
> -		memcpy(data->new_opp.supplies, opp->supplies, size);
> -		data->regulator_count = opp_table->regulator_count;
> -	} else {
> -		data->regulator_count = 0;
> -	}
> -
> -	data->regulators = opp_table->regulators;
> -	data->clk = opp_table->clk;
> -	data->dev = dev;
> -	data->old_opp.rate = old_opp->rate;
> -	data->new_opp.rate = freq;
> -
> -	return opp_table->set_opp(data);
> -}
> -
>  static int _set_required_opp(struct device *dev, struct device *pd_dev,
>  			     struct dev_pm_opp *opp, int i)
>  {
> @@ -1186,13 +1156,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
>  		}
>  	}
>  
> -	if (opp_table->set_opp) {
> -		ret = _set_opp_custom(opp_table, dev, opp, freq);
> -	} else {
> -		/* Only frequency scaling */
> -		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
> -	}
> -
> +	ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
>  	if (ret)
>  		return ret;
>  
> @@ -2054,7 +2018,6 @@ static void _opp_put_prop_name(struct opp_table *opp_table)
>  static int _opp_set_regulators(struct opp_table *opp_table, struct device *dev,
>  			       const char * const names[], unsigned int count)
>  {
> -	struct dev_pm_opp_supply *supplies;
>  	struct regulator *reg;
>  	int ret, i;
>  
> @@ -2082,20 +2045,8 @@ static int _opp_set_regulators(struct opp_table *opp_table, struct device *dev,
>  
>  	opp_table->regulator_count = count;
>  
> -	supplies = kmalloc_array(count * 2, sizeof(*supplies), GFP_KERNEL);
> -	if (!supplies) {
> -		ret = -ENOMEM;
> -		goto free_regulators;
> -	}
> -
> -	mutex_lock(&opp_table->lock);
> -	opp_table->sod_supplies = supplies;
> -	if (opp_table->set_opp_data) {
> -		opp_table->set_opp_data->old_opp.supplies = supplies;
> -		opp_table->set_opp_data->new_opp.supplies = supplies + count;
> -	}
> -
>  	/* Set generic config_regulators() for single regulators here */
> +	mutex_lock(&opp_table->lock);
>  	if (count == 1)
>  		opp_table->config_regulators = _opp_config_regulator_single;
>  	mutex_unlock(&opp_table->lock);
> @@ -2132,16 +2083,6 @@ static void _opp_put_regulators(struct opp_table *opp_table)
>  	for (i = opp_table->regulator_count - 1; i >= 0; i--)
>  		regulator_put(opp_table->regulators[i]);
>  
> -	mutex_lock(&opp_table->lock);
> -	if (opp_table->set_opp_data) {
> -		opp_table->set_opp_data->old_opp.supplies = NULL;
> -		opp_table->set_opp_data->new_opp.supplies = NULL;
> -	}
> -
> -	kfree(opp_table->sod_supplies);
> -	opp_table->sod_supplies = NULL;
> -	mutex_unlock(&opp_table->lock);
> -
>  	kfree(opp_table->regulators);
>  	opp_table->regulators = NULL;
>  	opp_table->regulator_count = -1;
> @@ -2195,61 +2136,6 @@ static void _opp_put_clkname(struct opp_table *opp_table)
>  	}
>  }
>  
> -/**
> - * _opp_register_set_opp_helper() - Register custom set OPP helper
> - * @dev: Device for which the helper is getting registered.
> - * @set_opp: Custom set OPP helper.
> - *
> - * This is useful to support complex platforms (like platforms with multiple
> - * regulators per device), instead of the generic OPP set rate helper.
> - *
> - * This must be called before any OPPs are initialized for the device.
> - */
> -static int _opp_register_set_opp_helper(struct opp_table *opp_table,
> -	struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data))
> -{
> -	struct dev_pm_set_opp_data *data;
> -
> -	/* Another CPU that shares the OPP table has set the helper ? */
> -	if (opp_table->set_opp)
> -		return 0;
> -
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -
> -	mutex_lock(&opp_table->lock);
> -	opp_table->set_opp_data = data;
> -	if (opp_table->sod_supplies) {
> -		data->old_opp.supplies = opp_table->sod_supplies;
> -		data->new_opp.supplies = opp_table->sod_supplies +
> -					 opp_table->regulator_count;
> -	}
> -	mutex_unlock(&opp_table->lock);
> -
> -	opp_table->set_opp = set_opp;
> -
> -	return 0;
> -}
> -
> -/**
> - * _opp_unregister_set_opp_helper() - Releases resources blocked for set_opp helper
> - * @opp_table: OPP table returned from _opp_register_set_opp_helper().
> - *
> - * Release resources blocked for platform specific set_opp helper.
> - */
> -static void _opp_unregister_set_opp_helper(struct opp_table *opp_table)
> -{
> -	if (opp_table->set_opp) {
> -		opp_table->set_opp = NULL;
> -
> -		mutex_lock(&opp_table->lock);
> -		kfree(opp_table->set_opp_data);
> -		opp_table->set_opp_data = NULL;
> -		mutex_unlock(&opp_table->lock);
> -	}
> -}
> -
>  /**
>   * _opp_set_config_regulators_helper() - Register custom set regulator helper.
>   * @dev: Device for which the helper is getting registered.
> @@ -2447,13 +2333,6 @@ struct opp_table *dev_pm_opp_set_config(struct device *dev,
>  			goto err;
>  	}
>  
> -	/* Configure opp helper */
> -	if (config->set_opp) {
> -		ret = _opp_register_set_opp_helper(opp_table, dev, config->set_opp);
> -		if (ret)
> -			goto err;
> -	}
> -
>  	/* Configure config_regulators helper */
>  	if (config->config_regulators) {
>  		ret = _opp_set_config_regulators_helper(opp_table, dev, config->config_regulators);
> @@ -2516,7 +2395,6 @@ void dev_pm_opp_clear_config(struct opp_table *opp_table)
>  	_opp_put_regulators(opp_table);
>  	_opp_put_supported_hw(opp_table);
>  	_opp_put_config_regulators_helper(opp_table);
> -	_opp_unregister_set_opp_helper(opp_table);
>  	_opp_put_prop_name(opp_table);
>  	_opp_put_clkname(opp_table);
>  
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 4695d315e7f9..407eee9f10ab 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -161,9 +161,6 @@ enum opp_table_access {
>   * @enabled: Set to true if the device's resources are enabled/configured.
>   * @genpd_performance_state: Device's power domain support performance state.
>   * @is_genpd: Marks if the OPP table belongs to a genpd.
> - * @set_opp: Platform specific set_opp callback
> - * @sod_supplies: Set opp data supplies
> - * @set_opp_data: Data to be passed to set_opp callback
>   * @dentry:	debugfs dentry pointer of the real device directory (not links).
>   * @dentry_name: Name of the real dentry.
>   *
> @@ -213,10 +210,6 @@ struct opp_table {
>  	bool genpd_performance_state;
>  	bool is_genpd;
>  
> -	int (*set_opp)(struct dev_pm_set_opp_data *data);
> -	struct dev_pm_opp_supply *sod_supplies;
> -	struct dev_pm_set_opp_data *set_opp_data;
> -
>  #ifdef CONFIG_DEBUG_FS
>  	struct dentry *dentry;
>  	char dentry_name[NAME_MAX];
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 185503aab64d..c6d8f01ef9fd 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -57,39 +57,6 @@ struct dev_pm_opp_icc_bw {
>  	u32 peak;
>  };
>  
> -/**
> - * struct dev_pm_opp_info - OPP freq/voltage/current values
> - * @rate:	Target clk rate in hz
> - * @supplies:	Array of voltage/current values for all power supplies
> - *
> - * This structure stores the freq/voltage/current values for a single OPP.
> - */
> -struct dev_pm_opp_info {
> -	unsigned long rate;
> -	struct dev_pm_opp_supply *supplies;
> -};
> -
> -/**
> - * struct dev_pm_set_opp_data - Set OPP data
> - * @old_opp:	Old OPP info
> - * @new_opp:	New OPP info
> - * @regulators:	Array of regulator pointers
> - * @regulator_count: Number of regulators
> - * @clk:	Pointer to clk
> - * @dev:	Pointer to the struct device
> - *
> - * This structure contains all information required for setting an OPP.
> - */
> -struct dev_pm_set_opp_data {
> -	struct dev_pm_opp_info old_opp;
> -	struct dev_pm_opp_info new_opp;
> -
> -	struct regulator **regulators;
> -	unsigned int regulator_count;
> -	struct clk *clk;
> -	struct device *dev;
> -};
> -
>  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);
> @@ -97,7 +64,6 @@ typedef int (*config_regulators_t)(struct device *dev, struct dev_pm_opp *old_op
>   * struct dev_pm_opp_config - Device OPP configuration values
>   * @clk_name: Clk name.
>   * @prop_name: Name to postfix to properties.
> - * @set_opp: Custom set OPP helper.
>   * @config_regulators: Custom set regulator helper.
>   * @supported_hw: Array of hierarchy of versions to match.
>   * @supported_hw_count: Number of elements in the array.
> @@ -111,7 +77,6 @@ typedef int (*config_regulators_t)(struct device *dev, struct dev_pm_opp *old_op
>  struct dev_pm_opp_config {
>  	const char *clk_name;
>  	const char *prop_name;
> -	int (*set_opp)(struct dev_pm_set_opp_data *data);
>  	config_regulators_t config_regulators;
>  	unsigned int *supported_hw;
>  	unsigned int supported_hw_count;

Hello Viresh,

Unfortunately we can't remove the set_opp_helper(). It's terrible that
this function is unused by Tegra 3d driver because it should be used.

The patch that supposed to use the devm_pm_opp_register_set_opp_helper()
[1] was merged a half year ago and just today I noticed that the merged
code doesn't have devm_pm_opp_register_set_opp_helper() [2]. I think
Thierry edited my patch before applying it, perhaps there was a merge
conflict :/ This needs to be fixed now.

[1] https://lore.kernel.org/all/20210817012754.8710-16-digetx@gmail.com/

[2]
https://elixir.bootlin.com/linux/v5.19-rc3/source/drivers/gpu/drm/tegra/gr3d.c#L535

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Viresh Kumar]

Hi Dmitry,

On 25-06-22, 14:42, Dmitry Osipenko wrote:
> 31.05.2022 13:10, Viresh Kumar пишет:
> > The only user of the custom helper is migrated to use
> > config_regulators() interface. Remove the now unused custom OPP helper
> > support.
> > 
> > This cleans up _set_opp() and leaves a single code path to be used by
> > all users.

> Unfortunately we can't remove the set_opp_helper(). It's terrible that
> this function is unused by Tegra 3d driver because it should be used.
> 
> The patch that supposed to use the devm_pm_opp_register_set_opp_helper()
> [1] was merged a half year ago and just today I noticed that the merged
> code doesn't have devm_pm_opp_register_set_opp_helper() [2]. I think
> Thierry edited my patch before applying it, perhaps there was a merge
> conflict :/ This needs to be fixed now.

As the commit log above says, we aren't removing the feature, but just
changing the interface to cnofig_regulators(). That was the only
special handling the drivers were required to do earlier as well, for
which the helper interface was added.

-- 
viresh

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Dmitry Osipenko]

27.06.2022 09:06, Viresh Kumar пишет:
> Hi Dmitry,
> 
> On 25-06-22, 14:42, Dmitry Osipenko wrote:
>> 31.05.2022 13:10, Viresh Kumar пишет:
>>> The only user of the custom helper is migrated to use
>>> config_regulators() interface. Remove the now unused custom OPP helper
>>> support.
>>>
>>> This cleans up _set_opp() and leaves a single code path to be used by
>>> all users.
> 
>> Unfortunately we can't remove the set_opp_helper(). It's terrible that
>> this function is unused by Tegra 3d driver because it should be used.
>>
>> The patch that supposed to use the devm_pm_opp_register_set_opp_helper()
>> [1] was merged a half year ago and just today I noticed that the merged
>> code doesn't have devm_pm_opp_register_set_opp_helper() [2]. I think
>> Thierry edited my patch before applying it, perhaps there was a merge
>> conflict :/ This needs to be fixed now.
> 
> As the commit log above says, we aren't removing the feature, but just
> changing the interface to cnofig_regulators(). That was the only
> special handling the drivers were required to do earlier as well, for
> which the helper interface was added.
> 

Okay, but Tegra 3d driver doesn't need config_regulators(), it needs
customized set_opp() to set clock rate for both 3d engines.

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Viresh Kumar]

On 27-06-22, 09:10, Dmitry Osipenko wrote:
> Okay, but Tegra 3d driver doesn't need config_regulators(), it needs
> customized set_opp() to set clock rate for both 3d engines.

What does that mean, you need to set two clocks ? There is separate
support added for that already [1].

-- 
viresh

[1] https://lore.kernel.org/lkml/cover.1654849214.git.viresh.kumar@linaro.org/

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Dmitry Osipenko]

27.06.2022 09:41, Viresh Kumar пишет:
> On 27-06-22, 09:10, Dmitry Osipenko wrote:
>> Okay, but Tegra 3d driver doesn't need config_regulators(), it needs
>> customized set_opp() to set clock rate for both 3d engines.
> 
> What does that mean, you need to set two clocks ? There is separate
> support added for that already [1].
> 

Yes, I missed that multi-clock OPP patch, thanks.

Seems _opp_compare_key() won't work properly for the multi-clocks since
Tegra doesn't have bandwidth nor level for the 3d OPPs. Why does it need
to check opp_table->clk_count == 1? Shouldn't it be opp_table->clk_count
> 0?

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Viresh Kumar]

On 27-06-22, 10:09, Dmitry Osipenko wrote:
> Yes, I missed that multi-clock OPP patch, thanks.
> 
> Seems _opp_compare_key() won't work properly for the multi-clocks since
> Tegra doesn't have bandwidth nor level for the 3d OPPs. Why does it need
> to check opp_table->clk_count == 1? Shouldn't it be opp_table->clk_count
> > 0?

The problem is that when we have multiple clocks, we can't assume any
of them as primary. Its the combination of the clock frequencies that
make them unique. Otherwise, what will happen if we have same
frequency of the first clock in two OPPs, but different frequency of
the second clock.

Because of this, we won't also support multiple clocks in all freq
finder APIs, like dev_pm_opp_find_freq_exact(). We can't do that from
just one frequency.

Ideally, the drivers should now be calling dev_pm_opp_set_opp() to set
the OPP now.

For your case, I think you can just add levels (like index) in the OPP
table. So we can uniquely identify each OPP.

-- 
viresh

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Dmitry Osipenko]

On 6/27/22 10:19, Viresh Kumar wrote:
> On 27-06-22, 10:09, Dmitry Osipenko wrote:
>> Yes, I missed that multi-clock OPP patch, thanks.
>>
>> Seems _opp_compare_key() won't work properly for the multi-clocks since
>> Tegra doesn't have bandwidth nor level for the 3d OPPs. Why does it need
>> to check opp_table->clk_count == 1? Shouldn't it be opp_table->clk_count
>>> 0?
> 
> The problem is that when we have multiple clocks, we can't assume any
> of them as primary. Its the combination of the clock frequencies that
> make them unique. Otherwise, what will happen if we have same
> frequency of the first clock in two OPPs, but different frequency of
> the second clock.
> 
> Because of this, we won't also support multiple clocks in all freq
> finder APIs, like dev_pm_opp_find_freq_exact(). We can't do that from
> just one frequency.
> 
> Ideally, the drivers should now be calling dev_pm_opp_set_opp() to set
> the OPP now.
> 
> For your case, I think you can just add levels (like index) in the OPP
> table. So we can uniquely identify each OPP.
> 

What about to bump the "by-level" sorting priority, making it above the
"by-rate" sorting and then always use the first clock for the "by-rate"
sorting? Then the multi-clock will work for Tegra without breaking dtbs
and those for whom this sorting option won't be appropriate will have to
add levels to the DT.

-- 
Best regards,
Dmitry

Re: [PATCH 5/5] OPP: Remove custom OPP helper support

[Viresh Kumar]

On 28-06-22, 13:04, Dmitry Osipenko wrote:
> What about to bump the "by-level" sorting priority, making it above the
> "by-rate" sorting and then always use the first clock for the "by-rate"
> sorting?

The order doesn't matter much really. If there are multiple clocks,
then we can't compare just one of them. If we don't want the level to
be introduced, which is fine, then we need to compare all the clocks.

> Then the multi-clock will work for Tegra without breaking dtbs
> and those for whom this sorting option won't be appropriate will have to
> add levels to the DT.

There was a recent discussion [1] around this, where using level was
considered sensible for such devices, like Qcom UFS.

-- 
viresh

[1] https://lore.kernel.org/lkml/65a4c28d-6702-3a9f-f837-1ea69a428777@linaro.org/