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

[Ulf Hansson]

On Wed, 14 Jun 2023 at 12:37, Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>

Again, thanks for improving this code!

Only a minor thing below and after that, feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>

> ---
>  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