diff mbox series

[2/2] OPP: Simplify the over-designed pstate <-> level dance

Message ID af0fd0fd64f33809875335a9cc2761085c3bd66f.1686739018.git.viresh.kumar@linaro.org
State Accepted
Commit 7c41cdcd3bbee5d49de9d4821b15e49d155ff22b
Headers show
Series None | expand

Commit Message

Viresh Kumar June 14, 2023, 10:37 a.m. UTC 
While adding support for "performance states" in the OPP and genpd core,
it was decided to set the `pstate` field via genpd's
pm_genpd_opp_to_performance_state() helper, to allow platforms to set
`pstate` even if they don't have a corresponding `level` field in the DT
OPP tables (More details are present in commit 6e41766a6a50 ("PM /
Domain: Implement of_genpd_opp_to_performance_state()")).

Revisiting that five years later clearly suggests that it was
over-designed as all current users are eventually using the `level`
value only.

The previous commit already added necessary checks to make sure pstate
is only used for genpd tables. Lets now simplify this a little, and use
`level` directly and remove `pstate` field altogether.

Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c    | 8 ++++----
 drivers/opp/debugfs.c | 2 +-
 drivers/opp/of.c      | 5 +----
 drivers/opp/opp.h     | 2 --
 4 files changed, 6 insertions(+), 11 deletions(-)

Comments

Viresh Kumar June 16, 2023, 6:20 a.m. UTC | #1 
On 14-06-23, 14:37, Ulf Hansson wrote:
> On Wed, 14 Jun 2023 at 12:37, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > While adding support for "performance states" in the OPP and genpd core,
> > it was decided to set the `pstate` field via genpd's
> > pm_genpd_opp_to_performance_state() helper, to allow platforms to set
> > `pstate` even if they don't have a corresponding `level` field in the DT
> > OPP tables (More details are present in commit 6e41766a6a50 ("PM /
> > Domain: Implement of_genpd_opp_to_performance_state()")).
> >
> > Revisiting that five years later clearly suggests that it was
> > over-designed as all current users are eventually using the `level`
> > value only.
> >
> > The previous commit already added necessary checks to make sure pstate
> > is only used for genpd tables. Lets now simplify this a little, and use
> > `level` directly and remove `pstate` field altogether.
> >
> > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Again, thanks for improving this code!
> 
> Only a minor thing below and after that, feel free to add:

Done.
diff mbox series

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index bfb012f5383c..48ddd72d2c71 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -245,7 +245,7 @@  unsigned int dev_pm_opp_get_required_pstate(struct dev_pm_opp *opp,
 		return 0;
 	}
 
-	return opp->required_opps[index]->pstate;
+	return opp->required_opps[index]->level;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_required_pstate);
 
@@ -943,7 +943,7 @@  static int _set_opp_bw(const struct opp_table *opp_table,
 static int _set_performance_state(struct device *dev, struct device *pd_dev,
 				  struct dev_pm_opp *opp, int i)
 {
-	unsigned int pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
+	unsigned int pstate = likely(opp) ? opp->required_opps[i]->level: 0;
 	int ret;
 
 	if (!pd_dev)
@@ -2728,8 +2728,8 @@  int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
 	mutex_lock(&src_table->lock);
 
 	list_for_each_entry(opp, &src_table->opp_list, node) {
-		if (opp->pstate == pstate) {
-			dest_pstate = opp->required_opps[i]->pstate;
+		if (opp->level == pstate) {
+			dest_pstate = opp->required_opps[i]->level;
 			goto unlock;
 		}
 	}
diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
index 0cc21e2b42ff..954ea31a2ff3 100644
--- a/drivers/opp/debugfs.c
+++ b/drivers/opp/debugfs.c
@@ -157,7 +157,7 @@  void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
 			     &opp->clock_latency_ns);
 
 	if (opp_table->is_genpd)
-		debugfs_create_u32("performance_state", S_IRUGO, d, &opp->pstate);
+		debugfs_create_u32("performance_state", S_IRUGO, d, &opp->level);
 
 	opp->of_name = of_node_full_name(opp->np);
 	debugfs_create_str("of_name", S_IRUGO, d, (char **)&opp->of_name);
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index e23ce6e78eb6..e6d1155d0990 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -945,9 +945,6 @@  static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	if (ret)
 		goto free_required_opps;
 
-	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);
 	if (ret) {
 		/* Don't return error for duplicate OPPs */
@@ -1400,7 +1397,7 @@  int of_get_required_opp_performance_state(struct device_node *np, int index)
 
 	opp = _find_opp_of_np(opp_table, required_np);
 	if (opp) {
-		pstate = opp->pstate;
+		pstate = opp->level;
 		dev_pm_opp_put(opp);
 	}
 
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 3805b92a6100..8a5ea38f3a3d 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -78,7 +78,6 @@  struct opp_config_data {
  * @turbo:	true if turbo (boost) OPP
  * @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.
  * @rates:	Frequencies in hertz
  * @level:	Performance level
  * @supplies:	Power supplies voltage/current values
@@ -101,7 +100,6 @@  struct dev_pm_opp {
 	bool turbo;
 	bool suspend;
 	bool removed;
-	unsigned int pstate;
 	unsigned long *rates;
 	unsigned int level;