| Message ID | af0fd0fd64f33809875335a9cc2761085c3bd66f.1686739018.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | [1/2] OPP: pstate is only valid for genpd OPP tables | expand |
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: Reviewed-by: Ulf Hansson <ulf.hansson@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(-) > > 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); I think this should be dropped altogether. The "performance_state" node is just confusing - and we already have a node for "level" a few lines above. > > 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; > Kind regards Uffe
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 --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;
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(-)