diff mbox series

[2/4] opp: Track device's resources configuration status

Message ID 453b3897507838e95359d891ef967165bd167a4e.1597292833.git.viresh.kumar@linaro.org
State New
Headers show
Series None | expand

Commit Message

Viresh Kumar Aug. 13, 2020, 4:28 a.m. UTC 
The OPP core needs to track if the resources of devices are enabled or
configured or not, as it disables the resources when target_freq is set
to 0.

Handle that with a separate variable to make it easy to maintain.

Also note that we will unconditionally call clk_set_rate() in the case
where the resources are getting enabled again. This shouldn't have any
side effects anyway.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/opp/core.c | 37 ++++++++++++++++++-------------------
 drivers/opp/opp.h  |  2 ++
 2 files changed, 20 insertions(+), 19 deletions(-)

-- 
2.14.1

Comments

Stephen Boyd Aug. 15, 2020, 8:03 a.m. UTC | #1 
Quoting Viresh Kumar (2020-08-12 21:28:59)
> The OPP core needs to track if the resources of devices are enabled or

> configured or not, as it disables the resources when target_freq is set

> to 0.

> 

> Handle that with a separate variable to make it easy to maintain.

> 

> Also note that we will unconditionally call clk_set_rate() in the case

> where the resources are getting enabled again. This shouldn't have any

> side effects anyway.


Any reason to want to do that? We'll have to grab the prepare lock in
the clk framework to figure out that there's nothing to do sometimes. If
anything, a comment may help to indicate that we call clk_set_rate()
again, but don't expect it to matter much.
Viresh Kumar Aug. 17, 2020, 5:05 a.m. UTC | #2 
On 15-08-20, 01:03, Stephen Boyd wrote:
> Quoting Viresh Kumar (2020-08-12 21:28:59)

> > The OPP core needs to track if the resources of devices are enabled or

> > configured or not, as it disables the resources when target_freq is set

> > to 0.

> > 

> > Handle that with a separate variable to make it easy to maintain.

> > 

> > Also note that we will unconditionally call clk_set_rate() in the case

> > where the resources are getting enabled again. This shouldn't have any

> > side effects anyway.

> 

> Any reason to want to do that?


To avoid more flags, code paths and simplicity of the code. And this
should normally happen in a corner case as well, like calling
set-rate(0) from suspend and then reinitializing things again in
resume.

> We'll have to grab the prepare lock in

> the clk framework to figure out that there's nothing to do sometimes. If

> anything, a comment may help to indicate that we call clk_set_rate()

> again, but don't expect it to matter much.


Ok.

-- 
viresh
diff mbox series

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 9668ea04cc80..e8882e7fd8a5 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -888,22 +888,18 @@  int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	}
 
 	if (unlikely(!target_freq)) {
+		ret = 0;
+
+		if (!opp_table->enabled)
+			goto put_opp_table;
+
 		/*
 		 * Some drivers need to support cases where some platforms may
 		 * have OPP table for the device, while others don't and
 		 * opp_set_rate() just needs to behave like clk_set_rate().
 		 */
-		if (!_get_opp_count(opp_table)) {
-			ret = 0;
-			goto put_opp_table;
-		}
-
-		if (!opp_table->required_opp_tables && !opp_table->regulators &&
-		    !opp_table->paths) {
-			dev_err(dev, "target frequency can't be 0\n");
-			ret = -EINVAL;
+		if (!_get_opp_count(opp_table))
 			goto put_opp_table;
-		}
 
 		ret = _set_opp_bw(opp_table, NULL, dev, true);
 		if (ret)
@@ -915,6 +911,9 @@  int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		}
 
 		ret = _set_required_opps(dev, opp_table, NULL);
+		if (!ret)
+			opp_table->enabled = false;
+
 		goto put_opp_table;
 	}
 
@@ -933,14 +932,11 @@  int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	old_freq = clk_get_rate(clk);
 
 	/* Return early if nothing to do */
-	if (old_freq == freq) {
-		if (!opp_table->required_opp_tables && !opp_table->regulators &&
-		    !opp_table->paths) {
-			dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
-				__func__, freq);
-			ret = 0;
-			goto put_opp_table;
-		}
+	if (opp_table->enabled && old_freq == freq) {
+		dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
+			__func__, freq);
+		ret = 0;
+		goto put_opp_table;
 	}
 
 	/*
@@ -1001,8 +997,11 @@  int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 			dev_err(dev, "Failed to set required opps: %d\n", ret);
 	}
 
-	if (!ret)
+	if (!ret) {
 		ret = _set_opp_bw(opp_table, opp, dev, false);
+		if (!ret)
+			opp_table->enabled = true;
+	}
 
 put_opp:
 	dev_pm_opp_put(opp);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index e51646ff279e..bd35802acc6e 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -152,6 +152,7 @@  enum opp_table_access {
  * property).
  * @paths: Interconnect path handles
  * @path_count: Number of interconnect paths
+ * @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
@@ -198,6 +199,7 @@  struct opp_table {
 	bool regulator_enabled;
 	struct icc_path **paths;
 	unsigned int path_count;
+	bool enabled;
 	bool genpd_performance_state;
 	bool is_genpd;