[PATCH 1/2] OPP: pstate is only valid for genpd OPP tables

[PATCH 1/2] OPP: pstate is only valid for genpd OPP tables

[Viresh Kumar]

It is not very clear right now that the `pstate` field is only valid for
genpd OPP tables and not consumer tables. And there is no checking for
the same at various places.

Add checks in place to verify that and make it clear to the reader.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c    | 18 ++++++++++++++++--
 drivers/opp/debugfs.c |  4 +++-
 drivers/opp/of.c      |  6 ++++++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 7290168ec806..bfb012f5383c 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -227,16 +227,24 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_level);
 unsigned int dev_pm_opp_get_required_pstate(struct dev_pm_opp *opp,
 					    unsigned int index)
 {
+	struct opp_table *opp_table = opp->opp_table;
+
 	if (IS_ERR_OR_NULL(opp) || !opp->available ||
-	    index >= opp->opp_table->required_opp_count) {
+	    index >= opp_table->required_opp_count) {
 		pr_err("%s: Invalid parameters\n", __func__);
 		return 0;
 	}
 
 	/* required-opps not fully initialized yet */
-	if (lazy_linking_pending(opp->opp_table))
+	if (lazy_linking_pending(opp_table))
 		return 0;
 
+	/* The required OPP table must belong to a genpd */
+	if (unlikely(!opp_table->required_opp_tables[index]->is_genpd)) {
+		pr_err("%s: Performance state is only valid for genpds.\n", __func__);
+		return 0;
+	}
+
 	return opp->required_opps[index]->pstate;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_required_pstate);
@@ -2686,6 +2694,12 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
 	int dest_pstate = -EINVAL;
 	int i;
 
+	/* Both OPP tables must belong to genpds */
+	if (unlikely(!src_table->is_genpd || !dst_table->is_genpd)) {
+		pr_err("%s: Performance state is only valid for genpds.\n", __func__);
+		return -EINVAL;
+	}
+
 	/*
 	 * Normally the src_table will have the "required_opps" property set to
 	 * point to one of the OPPs in the dst_table, but in some cases the
diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
index 2c7fb683441e..0cc21e2b42ff 100644
--- a/drivers/opp/debugfs.c
+++ b/drivers/opp/debugfs.c
@@ -152,11 +152,13 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
 	debugfs_create_bool("dynamic", S_IRUGO, d, &opp->dynamic);
 	debugfs_create_bool("turbo", S_IRUGO, d, &opp->turbo);
 	debugfs_create_bool("suspend", S_IRUGO, d, &opp->suspend);
-	debugfs_create_u32("performance_state", S_IRUGO, d, &opp->pstate);
 	debugfs_create_u32("level", S_IRUGO, d, &opp->level);
 	debugfs_create_ulong("clock_latency_ns", S_IRUGO, d,
 			     &opp->clock_latency_ns);
 
+	if (opp_table->is_genpd)
+		debugfs_create_u32("performance_state", S_IRUGO, d, &opp->pstate);
+
 	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 943c7fb7402b..e23ce6e78eb6 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -1392,6 +1392,12 @@ int of_get_required_opp_performance_state(struct device_node *np, int index)
 		goto put_required_np;
 	}
 
+	/* The OPP tables must belong to a genpd */
+	if (unlikely(!opp_table->is_genpd)) {
+		pr_err("%s: Performance state is only valid for genpds.\n", __func__);
+		goto put_required_np;
+	}
+
 	opp = _find_opp_of_np(opp_table, required_np);
 	if (opp) {
 		pstate = opp->pstate;
-- 
2.31.1.272.g89b43f80a514

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

[Viresh Kumar]

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

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;
 
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables

[Ulf Hansson]

On Wed, 14 Jun 2023 at 12:37, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> It is not very clear right now that the `pstate` field is only valid for
> genpd OPP tables and not consumer tables. And there is no checking for
> the same at various places.
>
> Add checks in place to verify that and make it clear to the reader.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Yes, this makes the code more clear!

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

Kind regards
Uffe

> ---
>  drivers/opp/core.c    | 18 ++++++++++++++++--
>  drivers/opp/debugfs.c |  4 +++-
>  drivers/opp/of.c      |  6 ++++++
>  3 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 7290168ec806..bfb012f5383c 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -227,16 +227,24 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_level);
>  unsigned int dev_pm_opp_get_required_pstate(struct dev_pm_opp *opp,
>                                             unsigned int index)
>  {
> +       struct opp_table *opp_table = opp->opp_table;
> +
>         if (IS_ERR_OR_NULL(opp) || !opp->available ||
> -           index >= opp->opp_table->required_opp_count) {
> +           index >= opp_table->required_opp_count) {
>                 pr_err("%s: Invalid parameters\n", __func__);
>                 return 0;
>         }
>
>         /* required-opps not fully initialized yet */
> -       if (lazy_linking_pending(opp->opp_table))
> +       if (lazy_linking_pending(opp_table))
>                 return 0;
>
> +       /* The required OPP table must belong to a genpd */
> +       if (unlikely(!opp_table->required_opp_tables[index]->is_genpd)) {
> +               pr_err("%s: Performance state is only valid for genpds.\n", __func__);
> +               return 0;
> +       }
> +
>         return opp->required_opps[index]->pstate;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_get_required_pstate);
> @@ -2686,6 +2694,12 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
>         int dest_pstate = -EINVAL;
>         int i;
>
> +       /* Both OPP tables must belong to genpds */
> +       if (unlikely(!src_table->is_genpd || !dst_table->is_genpd)) {
> +               pr_err("%s: Performance state is only valid for genpds.\n", __func__);
> +               return -EINVAL;
> +       }
> +
>         /*
>          * Normally the src_table will have the "required_opps" property set to
>          * point to one of the OPPs in the dst_table, but in some cases the
> diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
> index 2c7fb683441e..0cc21e2b42ff 100644
> --- a/drivers/opp/debugfs.c
> +++ b/drivers/opp/debugfs.c
> @@ -152,11 +152,13 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
>         debugfs_create_bool("dynamic", S_IRUGO, d, &opp->dynamic);
>         debugfs_create_bool("turbo", S_IRUGO, d, &opp->turbo);
>         debugfs_create_bool("suspend", S_IRUGO, d, &opp->suspend);
> -       debugfs_create_u32("performance_state", S_IRUGO, d, &opp->pstate);
>         debugfs_create_u32("level", S_IRUGO, d, &opp->level);
>         debugfs_create_ulong("clock_latency_ns", S_IRUGO, d,
>                              &opp->clock_latency_ns);
>
> +       if (opp_table->is_genpd)
> +               debugfs_create_u32("performance_state", S_IRUGO, d, &opp->pstate);
> +
>         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 943c7fb7402b..e23ce6e78eb6 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -1392,6 +1392,12 @@ int of_get_required_opp_performance_state(struct device_node *np, int index)
>                 goto put_required_np;
>         }
>
> +       /* The OPP tables must belong to a genpd */
> +       if (unlikely(!opp_table->is_genpd)) {
> +               pr_err("%s: Performance state is only valid for genpds.\n", __func__);
> +               goto put_required_np;
> +       }
> +
>         opp = _find_opp_of_np(opp_table, required_np);
>         if (opp) {
>                 pstate = opp->pstate;
> --
> 2.31.1.272.g89b43f80a514
>

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

Re: [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables

[Bjorn Andersson]

On Wed, Jun 14, 2023 at 04:07:25PM +0530, Viresh Kumar wrote:
> It is not very clear right now that the `pstate` field is only valid for
> genpd OPP tables and not consumer tables. And there is no checking for
> the same at various places.
> 
> Add checks in place to verify that and make it clear to the reader.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c    | 18 ++++++++++++++++--
>  drivers/opp/debugfs.c |  4 +++-
>  drivers/opp/of.c      |  6 ++++++
>  3 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
[..]
> @@ -2686,6 +2694,12 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
>  	int dest_pstate = -EINVAL;
>  	int i;
>  
> +	/* Both OPP tables must belong to genpds */
> +	if (unlikely(!src_table->is_genpd || !dst_table->is_genpd)) {

I have a platform_device, with a required-opps and with an associated
genpd which does not implement set_performance_state(), but its parent
genpd does.

This results in me arriving here with src_table of NULL, and boom...


Looking at the very next statement in this function, it seems arriving
here without src_table was valid up until this change.

Regards,
Bjorn

Re: [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables

[Viresh Kumar]

On 15-06-23, 18:35, Bjorn Andersson wrote:
> On Wed, Jun 14, 2023 at 04:07:25PM +0530, Viresh Kumar wrote:
> > It is not very clear right now that the `pstate` field is only valid for
> > genpd OPP tables and not consumer tables. And there is no checking for
> > the same at various places.
> > 
> > Add checks in place to verify that and make it clear to the reader.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/opp/core.c    | 18 ++++++++++++++++--
> >  drivers/opp/debugfs.c |  4 +++-
> >  drivers/opp/of.c      |  6 ++++++
> >  3 files changed, 25 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> [..]
> > @@ -2686,6 +2694,12 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
> >  	int dest_pstate = -EINVAL;
> >  	int i;
> >  
> > +	/* Both OPP tables must belong to genpds */
> > +	if (unlikely(!src_table->is_genpd || !dst_table->is_genpd)) {
> 
> I have a platform_device, with a required-opps and with an associated
> genpd which does not implement set_performance_state(), but its parent
> genpd does.
> 
> This results in me arriving here with src_table of NULL, and boom...
> 
> 
> Looking at the very next statement in this function, it seems arriving
> here without src_table was valid up until this change.

Fixed as:

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 48ddd72d2c71..3f46e499d615 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2694,12 +2694,6 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
        int dest_pstate = -EINVAL;
        int i;

-       /* Both OPP tables must belong to genpds */
-       if (unlikely(!src_table->is_genpd || !dst_table->is_genpd)) {
-               pr_err("%s: Performance state is only valid for genpds.\n", __func__);
-               return -EINVAL;
-       }
-
        /*
         * Normally the src_table will have the "required_opps" property set to
         * point to one of the OPPs in the dst_table, but in some cases the
@@ -2710,6 +2704,12 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
        if (!src_table || !src_table->required_opp_count)
                return pstate;

+       /* Both OPP tables must belong to genpds */
+       if (unlikely(!src_table->is_genpd || !dst_table->is_genpd)) {
+               pr_err("%s: Performance state is only valid for genpds.\n", __func__);
+               return -EINVAL;
+       }
+
        /* required-opps not fully initialized yet */
        if (lazy_linking_pending(src_table))
                return -EBUSY;

Thanks.

-- 
viresh

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

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables

[Bjorn Andersson]

On Fri, Jun 16, 2023 at 11:48:30AM +0530, Viresh Kumar wrote:
> On 15-06-23, 18:35, Bjorn Andersson wrote:
> > On Wed, Jun 14, 2023 at 04:07:25PM +0530, Viresh Kumar wrote:
> > > It is not very clear right now that the `pstate` field is only valid for
> > > genpd OPP tables and not consumer tables. And there is no checking for
> > > the same at various places.
> > > 
> > > Add checks in place to verify that and make it clear to the reader.
> > > 
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > >  drivers/opp/core.c    | 18 ++++++++++++++++--
> > >  drivers/opp/debugfs.c |  4 +++-
> > >  drivers/opp/of.c      |  6 ++++++
> > >  3 files changed, 25 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > [..]
> > > @@ -2686,6 +2694,12 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
> > >  	int dest_pstate = -EINVAL;
> > >  	int i;
> > >  
> > > +	/* Both OPP tables must belong to genpds */
> > > +	if (unlikely(!src_table->is_genpd || !dst_table->is_genpd)) {
> > 
> > I have a platform_device, with a required-opps and with an associated
> > genpd which does not implement set_performance_state(), but its parent
> > genpd does.
> > 
> > This results in me arriving here with src_table of NULL, and boom...
> > 
> > 
> > Looking at the very next statement in this function, it seems arriving
> > here without src_table was valid up until this change.
> 
> Fixed as:
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 48ddd72d2c71..3f46e499d615 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2694,12 +2694,6 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
>         int dest_pstate = -EINVAL;
>         int i;
> 
> -       /* Both OPP tables must belong to genpds */
> -       if (unlikely(!src_table->is_genpd || !dst_table->is_genpd)) {
> -               pr_err("%s: Performance state is only valid for genpds.\n", __func__);
> -               return -EINVAL;
> -       }
> -
>         /*
>          * Normally the src_table will have the "required_opps" property set to
>          * point to one of the OPPs in the dst_table, but in some cases the
> @@ -2710,6 +2704,12 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
>         if (!src_table || !src_table->required_opp_count)
>                 return pstate;
> 
> +       /* Both OPP tables must belong to genpds */
> +       if (unlikely(!src_table->is_genpd || !dst_table->is_genpd)) {
> +               pr_err("%s: Performance state is only valid for genpds.\n", __func__);
> +               return -EINVAL;
> +       }
> +
>         /* required-opps not fully initialized yet */
>         if (lazy_linking_pending(src_table))
>                 return -EBUSY;
> 
> Thanks.
> 

Looks good, thanks for the quick response.

Reviewed-by: Bjorn Andersson <quic_bjorande@quicinc.com>
Tested-by: Bjorn Andersson <quic_bjorande@quicinc.com>

Regards,
Bjorn