[RFT PATCH 0/2] OPP: Simplify required-opp handling

[RFT PATCH 0/2] OPP: Simplify required-opp handling

[Viresh Kumar]

Hello,

I wasn't able to test this locally (despite trying to hack it around) and need
help from someone who is `virt_devs` field of `struct dev_pm_opp_config`.

I asked Mani earlier to test this, which he did, but unfortunately he could only
half test it as the platform didn't have `virt_devs` configured.

Pushed here:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/required-opps

-------------------------8<-------------------------

Configuring the required OPP was never properly implemented, we just
took an exception for genpds and configured them directly, while leaving
out all other required OPP types.

Now that a standard call to dev_pm_opp_set_opp() takes care of
configuring the opp->level too, the special handling for genpds can be
avoided by simply calling dev_pm_opp_set_opp() for the required OPPs,
which shall eventually configure the corresponding level for genpds.

This also makes it possible for us to configure other type of required
OPPs (no concrete users yet though), via the same path. This is how
other frameworks take care of parent nodes, like clock, regulators, etc,
where we recursively call the same helper.

--
Viresh

Viresh Kumar (2):
  OPP: Use _set_opp_level() for single genpd case
  OPP: Call dev_pm_opp_set_opp() for required OPPs

 drivers/opp/core.c     | 144 ++++++++++++++++++-----------------------
 drivers/opp/of.c       |  35 +++++++---
 drivers/opp/opp.h      |   8 +--
 include/linux/pm_opp.h |   7 +-
 4 files changed, 98 insertions(+), 96 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Viresh Kumar]

There are two genpd (as required-opp) cases that we need to handle,
devices with a single genpd and ones with multiple genpds.

The multiple genpds case is clear, where the OPP core calls
dev_pm_domain_attach_by_name() for them and uses the virtual devices
returned by this helper to call dev_pm_domain_set_performance_state()
later to change the performance state.

The single genpd case however requires special handling as we need to
use the same `dev` structure (instead of a virtual one provided by genpd
core) for setting the performance state via
dev_pm_domain_set_performance_state().

As we move towards more generic code to take care of the required OPPs,
where we will recursively call dev_pm_opp_set_opp() for all the required
OPPs, the above special case becomes a problem.

Eventually we want to handle all performance state changes via
_set_opp_level(), so lets move the single genpd case to that right away.

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

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 84f345c69ea5..aab8c8e79146 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1074,10 +1074,12 @@ static int _opp_set_required_opps_generic(struct device *dev,
 static int _opp_set_required_opps_genpd(struct device *dev,
 	struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down)
 {
-	struct device **genpd_virt_devs =
-		opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev;
+	struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
 	int index, target, delta, ret;
 
+	if (!genpd_virt_devs)
+		return 0;
+
 	/* Scaling up? Set required OPPs in normal order, else reverse */
 	if (!scaling_down) {
 		index = 0;
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 81fa27599d58..e056f31a48b5 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -296,7 +296,7 @@ void _of_clear_opp(struct opp_table *opp_table, struct dev_pm_opp *opp)
 	of_node_put(opp->np);
 }
 
-static int _link_required_opps(struct dev_pm_opp *opp,
+static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table,
 			       struct opp_table *required_table, int index)
 {
 	struct device_node *np;
@@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp,
 		return -ENODEV;
 	}
 
+	/*
+	 * There are two genpd (as required-opp) cases that we need to handle,
+	 * devices with a single genpd and ones with multiple genpds.
+	 *
+	 * The single genpd case requires special handling as we need to use the
+	 * same `dev` structure (instead of a virtual one provided by genpd
+	 * core) for setting the performance state. Lets treat this as a case
+	 * where the OPP's level is directly available without required genpd
+	 * link in the DT.
+	 *
+	 * Just update the `level` with the right value, which
+	 * dev_pm_opp_set_opp() will take care of in the normal path itself.
+	 */
+	if (required_table->is_genpd && opp_table->required_opp_count == 1 &&
+	    !opp_table->genpd_virt_devs) {
+		if (!WARN_ON(opp->level))
+			opp->level = opp->required_opps[0]->level;
+	}
+
 	return 0;
 }
 
@@ -338,7 +357,7 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 		if (IS_ERR_OR_NULL(required_table))
 			continue;
 
-		ret = _link_required_opps(opp, required_table, i);
+		ret = _link_required_opps(opp, opp_table, required_table, i);
 		if (ret)
 			goto free_required_opps;
 	}
@@ -359,7 +378,7 @@ static int lazy_link_required_opps(struct opp_table *opp_table,
 	int ret;
 
 	list_for_each_entry(opp, &opp_table->opp_list, node) {
-		ret = _link_required_opps(opp, new_table, index);
+		ret = _link_required_opps(opp, opp_table, new_table, index);
 		if (ret)
 			return ret;
 	}
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Ulf Hansson]

On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> There are two genpd (as required-opp) cases that we need to handle,
> devices with a single genpd and ones with multiple genpds.
>
> The multiple genpds case is clear, where the OPP core calls
> dev_pm_domain_attach_by_name() for them and uses the virtual devices
> returned by this helper to call dev_pm_domain_set_performance_state()
> later to change the performance state.
>
> The single genpd case however requires special handling as we need to
> use the same `dev` structure (instead of a virtual one provided by genpd
> core) for setting the performance state via
> dev_pm_domain_set_performance_state().
>
> As we move towards more generic code to take care of the required OPPs,
> where we will recursively call dev_pm_opp_set_opp() for all the required
> OPPs, the above special case becomes a problem.
>
> Eventually we want to handle all performance state changes via
> _set_opp_level(), so lets move the single genpd case to that right away.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c |  6 ++++--
>  drivers/opp/of.c   | 25 ++++++++++++++++++++++---
>  2 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 84f345c69ea5..aab8c8e79146 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1074,10 +1074,12 @@ static int _opp_set_required_opps_generic(struct device *dev,
>  static int _opp_set_required_opps_genpd(struct device *dev,
>         struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down)
>  {
> -       struct device **genpd_virt_devs =
> -               opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev;
> +       struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
>         int index, target, delta, ret;
>
> +       if (!genpd_virt_devs)
> +               return 0;
> +
>         /* Scaling up? Set required OPPs in normal order, else reverse */
>         if (!scaling_down) {
>                 index = 0;
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 81fa27599d58..e056f31a48b5 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -296,7 +296,7 @@ void _of_clear_opp(struct opp_table *opp_table, struct dev_pm_opp *opp)
>         of_node_put(opp->np);
>  }
>
> -static int _link_required_opps(struct dev_pm_opp *opp,
> +static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table,
>                                struct opp_table *required_table, int index)
>  {
>         struct device_node *np;
> @@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp,
>                 return -ENODEV;
>         }
>
> +       /*
> +        * There are two genpd (as required-opp) cases that we need to handle,
> +        * devices with a single genpd and ones with multiple genpds.
> +        *
> +        * The single genpd case requires special handling as we need to use the
> +        * same `dev` structure (instead of a virtual one provided by genpd
> +        * core) for setting the performance state. Lets treat this as a case
> +        * where the OPP's level is directly available without required genpd
> +        * link in the DT.
> +        *
> +        * Just update the `level` with the right value, which
> +        * dev_pm_opp_set_opp() will take care of in the normal path itself.
> +        */
> +       if (required_table->is_genpd && opp_table->required_opp_count == 1 &&
> +           !opp_table->genpd_virt_devs) {
> +               if (!WARN_ON(opp->level))

Hmm. Doesn't this introduce an unnecessary limitation?

An opp node that has a required-opps phande, may have "opp-hz",
"opp-microvolt", etc. Why would we not allow the "opp-level" to be
used too?

> +                       opp->level = opp->required_opps[0]->level;
> +       }
> +
>         return 0;
>  }
>
> @@ -338,7 +357,7 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
>                 if (IS_ERR_OR_NULL(required_table))
>                         continue;
>
> -               ret = _link_required_opps(opp, required_table, i);
> +               ret = _link_required_opps(opp, opp_table, required_table, i);
>                 if (ret)
>                         goto free_required_opps;
>         }
> @@ -359,7 +378,7 @@ static int lazy_link_required_opps(struct opp_table *opp_table,
>         int ret;
>
>         list_for_each_entry(opp, &opp_table->opp_list, node) {
> -               ret = _link_required_opps(opp, new_table, index);
> +               ret = _link_required_opps(opp, opp_table, new_table, index);
>                 if (ret)
>                         return ret;
>         }

Kind regards
Uffe

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Viresh Kumar]

On 19-10-23, 13:16, Ulf Hansson wrote:
> On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > +       /*
> > +        * There are two genpd (as required-opp) cases that we need to handle,
> > +        * devices with a single genpd and ones with multiple genpds.
> > +        *
> > +        * The single genpd case requires special handling as we need to use the
> > +        * same `dev` structure (instead of a virtual one provided by genpd
> > +        * core) for setting the performance state. Lets treat this as a case
> > +        * where the OPP's level is directly available without required genpd
> > +        * link in the DT.
> > +        *
> > +        * Just update the `level` with the right value, which
> > +        * dev_pm_opp_set_opp() will take care of in the normal path itself.
> > +        */
> > +       if (required_table->is_genpd && opp_table->required_opp_count == 1 &&
> > +           !opp_table->genpd_virt_devs) {
> > +               if (!WARN_ON(opp->level))
> 
> Hmm. Doesn't this introduce an unnecessary limitation?
> 
> An opp node that has a required-opps phande, may have "opp-hz",
> "opp-microvolt", etc. Why would we not allow the "opp-level" to be
> used too?

Such platforms need to call dev_pm_opp_set_config() with genpd names
and it should all work just fine. The point is that we can't use the
same `dev` pointer with another OPP table, i.e. device's dev pointer
for the genpd's table here.

-- 
viresh

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Ulf Hansson]

On Fri, 20 Oct 2023 at 05:45, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 19-10-23, 13:16, Ulf Hansson wrote:
> > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > +       /*
> > > +        * There are two genpd (as required-opp) cases that we need to handle,
> > > +        * devices with a single genpd and ones with multiple genpds.
> > > +        *
> > > +        * The single genpd case requires special handling as we need to use the
> > > +        * same `dev` structure (instead of a virtual one provided by genpd
> > > +        * core) for setting the performance state. Lets treat this as a case
> > > +        * where the OPP's level is directly available without required genpd
> > > +        * link in the DT.
> > > +        *
> > > +        * Just update the `level` with the right value, which
> > > +        * dev_pm_opp_set_opp() will take care of in the normal path itself.
> > > +        */
> > > +       if (required_table->is_genpd && opp_table->required_opp_count == 1 &&
> > > +           !opp_table->genpd_virt_devs) {
> > > +               if (!WARN_ON(opp->level))
> >
> > Hmm. Doesn't this introduce an unnecessary limitation?
> >
> > An opp node that has a required-opps phande, may have "opp-hz",
> > "opp-microvolt", etc. Why would we not allow the "opp-level" to be
> > used too?
>
> Such platforms need to call dev_pm_opp_set_config() with genpd names
> and it should all work just fine. The point is that we can't use the
> same `dev` pointer with another OPP table, i.e. device's dev pointer
> for the genpd's table here.

For the single PM domain case, consumer drivers are often not able to
use dev_pm_opp_set_config(). That's because the PM domain has already
been attached from some of the generic buses, through
dev_pm_domain_attach().

In this case, as dev_pm_opp_set_config() ends up trying to attach
again, via dev_pm_domain_attach_by_name() it would receive
"ERR_PTR(-EEXIST)".

Or maybe I didn't quite understand your point?

Kind regards
Uffe

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Viresh Kumar]

On 20-10-23, 12:02, Ulf Hansson wrote:
> For the single PM domain case, consumer drivers are often not able to
> use dev_pm_opp_set_config(). That's because the PM domain has already
> been attached from some of the generic buses, through
> dev_pm_domain_attach().
> 
> In this case, as dev_pm_opp_set_config() ends up trying to attach
> again, via dev_pm_domain_attach_by_name() it would receive
> "ERR_PTR(-EEXIST)".
> 
> Or maybe I didn't quite understand your point?

So the thing is that I _really_ want to call dev_pm_opp_set_opp() for
each OPP we want to configure, primary or required. For example, the
required OPP may want to do more than just performance state and we
aren't touching them right now.

Now, in order to call dev_pm_opp_set_opp() for any device, we need a
device pointer and an OPP table associated with it.

I can take care of it for the multi genpd case as there are extra
device structures (which we get from dev_pm_domain_attach_by_name()),
but there is no clean way out for single PM domain devices, unless
they also call dev_pm_opp_set_config() to get a virtual structure.

This is why I had to get this hackish code in place to make it work
with the recursive calls to dev_pm_opp_set_opp(), where I could just
reuse the opp-level thing for the primary device.

How do you suggest we take care of this now ?

-- 
viresh

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Ulf Hansson]

On Fri, 20 Oct 2023 at 12:57, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 20-10-23, 12:02, Ulf Hansson wrote:
> > For the single PM domain case, consumer drivers are often not able to
> > use dev_pm_opp_set_config(). That's because the PM domain has already
> > been attached from some of the generic buses, through
> > dev_pm_domain_attach().
> >
> > In this case, as dev_pm_opp_set_config() ends up trying to attach
> > again, via dev_pm_domain_attach_by_name() it would receive
> > "ERR_PTR(-EEXIST)".
> >
> > Or maybe I didn't quite understand your point?
>
> So the thing is that I _really_ want to call dev_pm_opp_set_opp() for
> each OPP we want to configure, primary or required. For example, the
> required OPP may want to do more than just performance state and we
> aren't touching them right now.

I understand - and it makes perfect sense to me too!

>
> Now, in order to call dev_pm_opp_set_opp() for any device, we need a
> device pointer and an OPP table associated with it.
>
> I can take care of it for the multi genpd case as there are extra
> device structures (which we get from dev_pm_domain_attach_by_name()),
> but there is no clean way out for single PM domain devices, unless
> they also call dev_pm_opp_set_config() to get a virtual structure.
>
> This is why I had to get this hackish code in place to make it work
> with the recursive calls to dev_pm_opp_set_opp(), where I could just
> reuse the opp-level thing for the primary device.
>
> How do you suggest we take care of this now ?

Honestly, I don't know yet. But I am certainly willing to help. Allow
me to have a closer look and see if I can propose a way forward.

Kind regards
Uffe

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Viresh Kumar]

On 19-10-23, 13:16, Ulf Hansson wrote:
> On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > +static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table,
> >                                struct opp_table *required_table, int index)
> >  {
> >         struct device_node *np;
> > @@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp,
> >                 return -ENODEV;
> >         }
> >
> > +       /*
> > +        * There are two genpd (as required-opp) cases that we need to handle,
> > +        * devices with a single genpd and ones with multiple genpds.
> > +        *
> > +        * The single genpd case requires special handling as we need to use the
> > +        * same `dev` structure (instead of a virtual one provided by genpd
> > +        * core) for setting the performance state. Lets treat this as a case
> > +        * where the OPP's level is directly available without required genpd
> > +        * link in the DT.
> > +        *
> > +        * Just update the `level` with the right value, which
> > +        * dev_pm_opp_set_opp() will take care of in the normal path itself.
> > +        */
> > +       if (required_table->is_genpd && opp_table->required_opp_count == 1 &&
> > +           !opp_table->genpd_virt_devs) {
> > +               if (!WARN_ON(opp->level))
> 
> Hmm. Doesn't this introduce an unnecessary limitation?
> 
> An opp node that has a required-opps phande, may have "opp-hz",
> "opp-microvolt", etc. Why would we not allow the "opp-level" to be
> used too?

Coming back to this, why would we ever want a device to have "opp-level" and
"required-opp" (set to genpd's table) ? That would mean we will call:

dev_pm_domain_set_performance_state() twice to set different level values.

And so it should be safe to force that if required-opp table is set to a genpd,
then opp-level shouldn't be set. Maybe we should fail in that case, which isn't
happening currently.

-- 
viresh

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Ulf Hansson]

On Wed, 25 Oct 2023 at 08:55, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 19-10-23, 13:16, Ulf Hansson wrote:
> > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > +static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table,
> > >                                struct opp_table *required_table, int index)
> > >  {
> > >         struct device_node *np;
> > > @@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp,
> > >                 return -ENODEV;
> > >         }
> > >
> > > +       /*
> > > +        * There are two genpd (as required-opp) cases that we need to handle,
> > > +        * devices with a single genpd and ones with multiple genpds.
> > > +        *
> > > +        * The single genpd case requires special handling as we need to use the
> > > +        * same `dev` structure (instead of a virtual one provided by genpd
> > > +        * core) for setting the performance state. Lets treat this as a case
> > > +        * where the OPP's level is directly available without required genpd
> > > +        * link in the DT.
> > > +        *
> > > +        * Just update the `level` with the right value, which
> > > +        * dev_pm_opp_set_opp() will take care of in the normal path itself.
> > > +        */
> > > +       if (required_table->is_genpd && opp_table->required_opp_count == 1 &&
> > > +           !opp_table->genpd_virt_devs) {
> > > +               if (!WARN_ON(opp->level))
> >
> > Hmm. Doesn't this introduce an unnecessary limitation?
> >
> > An opp node that has a required-opps phande, may have "opp-hz",
> > "opp-microvolt", etc. Why would we not allow the "opp-level" to be
> > used too?
>
> Coming back to this, why would we ever want a device to have "opp-level" and
> "required-opp" (set to genpd's table) ? That would mean we will call:
>
> dev_pm_domain_set_performance_state() twice to set different level values.

Yes - and that would be weird, especially since the PM domain (genpd)
is already managing the aggregation and propagation to parent domains.

I guess I got a bit confused by the commit message for patch2/2, where
it sounded like you were striving towards introducing recursive calls
to set OPPs. Having a closer look, I realize that isn't the case,
which I think makes sense.

>
> And so it should be safe to force that if required-opp table is set to a genpd,
> then opp-level shouldn't be set. Maybe we should fail in that case, which isn't
> happening currently.

Yes, it seems better to fail earlier during the OF parsing of the
required-opps or when adding an OPP dynamically. In that way, the
WARN_ON above could be removed.

That said, sorry for the noise and either way, feel free to add (for
$subject patch):

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

Kind regards
Uffe

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Viresh Kumar]

On 25-10-23, 12:40, Ulf Hansson wrote:
> On Wed, 25 Oct 2023 at 08:55, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 19-10-23, 13:16, Ulf Hansson wrote:
> > > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > +       if (required_table->is_genpd && opp_table->required_opp_count == 1 &&
> > > > +           !opp_table->genpd_virt_devs) {
> > > > +               if (!WARN_ON(opp->level))
> > >
> > > Hmm. Doesn't this introduce an unnecessary limitation?
> > >
> > > An opp node that has a required-opps phande, may have "opp-hz",
> > > "opp-microvolt", etc. Why would we not allow the "opp-level" to be
> > > used too?
> >
> > Coming back to this, why would we ever want a device to have "opp-level" and
> > "required-opp" (set to genpd's table) ? That would mean we will call:
> >
> > dev_pm_domain_set_performance_state() twice to set different level values.
> 
> Yes - and that would be weird, especially since the PM domain (genpd)
> is already managing the aggregation and propagation to parent domains.
> 
> I guess I got a bit confused by the commit message for patch2/2, where
> it sounded like you were striving towards introducing recursive calls
> to set OPPs. Having a closer look, I realize that isn't the case,
> which I think makes sense.
> 
> >
> > And so it should be safe to force that if required-opp table is set to a genpd,
> > then opp-level shouldn't be set. Maybe we should fail in that case, which isn't
> > happening currently.
> 
> Yes, it seems better to fail earlier during the OF parsing of the
> required-opps or when adding an OPP dynamically. In that way, the
> WARN_ON above could be removed.

For now I will leave the WARN as is, will reconsider if it makes more
sense to fail and return early. And send a separate patch for that.

> That said, sorry for the noise and either way, feel free to add (for
> $subject patch):
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Thanks.

-- 
viresh

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Stephan Gerhold]

On Wed, Oct 25, 2023 at 12:40:26PM +0200, Ulf Hansson wrote:
> On Wed, 25 Oct 2023 at 08:55, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 19-10-23, 13:16, Ulf Hansson wrote:
> > > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > +static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table,
> > > >                                struct opp_table *required_table, int index)
> > > >  {
> > > >         struct device_node *np;
> > > > @@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp,
> > > >                 return -ENODEV;
> > > >         }
> > > >
> > > > +       /*
> > > > +        * There are two genpd (as required-opp) cases that we need to handle,
> > > > +        * devices with a single genpd and ones with multiple genpds.
> > > > +        *
> > > > +        * The single genpd case requires special handling as we need to use the
> > > > +        * same `dev` structure (instead of a virtual one provided by genpd
> > > > +        * core) for setting the performance state. Lets treat this as a case
> > > > +        * where the OPP's level is directly available without required genpd
> > > > +        * link in the DT.
> > > > +        *
> > > > +        * Just update the `level` with the right value, which
> > > > +        * dev_pm_opp_set_opp() will take care of in the normal path itself.
> > > > +        */
> > > > +       if (required_table->is_genpd && opp_table->required_opp_count == 1 &&
> > > > +           !opp_table->genpd_virt_devs) {
> > > > +               if (!WARN_ON(opp->level))
> > >
> > > Hmm. Doesn't this introduce an unnecessary limitation?
> > >
> > > An opp node that has a required-opps phande, may have "opp-hz",
> > > "opp-microvolt", etc. Why would we not allow the "opp-level" to be
> > > used too?
> >
> > Coming back to this, why would we ever want a device to have "opp-level" and
> > "required-opp" (set to genpd's table) ? That would mean we will call:
> >
> > dev_pm_domain_set_performance_state() twice to set different level values.
> 
> Yes - and that would be weird, especially since the PM domain (genpd)
> is already managing the aggregation and propagation to parent domains.
> 

FWIW I'm hitting this WARNing when trying to set up the parent domain
setup for CPR->RPMPD(MX) on MSM8916 that I discussed with Uffe recently
[1]. I know, me and all my weird OPP setups. :'D

Basically, I have cpufreq voting for performance states of the CPR genpd
(via required-opps). CPR is supposed to have <&rpmpd MSM8916_VDDMX_AO>
as parent genpd and translates to the parent performance state using the
"required-opps" in the *CPR* OPP table:

	cpr: power-controller@b018000 {
		compatible = "qcom,msm8916-cpr", "qcom,cpr";
		reg = <0x0b018000 0x1000>;
		/* ... */
		#power-domain-cells = <0>;
		operating-points-v2 = <&cpr_opp_table>;
		/* Supposed to be parent domain, not consumer */
		power-domains = <&rpmpd MSM8916_VDDMX_AO>;

		cpr_opp_table: opp-table {
			compatible = "operating-points-v2-qcom-level";

			cpr_opp1: opp1 {
				opp-level = <1>;
				qcom,opp-fuse-level = <1>;
				required-opps = <&rpmpd_opp_svs_soc>;
			};
			cpr_opp2: opp2 {
				opp-level = <2>;
				qcom,opp-fuse-level = <2>;
				required-opps = <&rpmpd_opp_nom>;
			};
			cpr_opp3: opp3 {
				opp-level = <3>;
				qcom,opp-fuse-level = <3>;
				required-opps = <&rpmpd_opp_super_turbo>;
			};
		};
	};

There are two problems with this:

 1. (Unrelated to $subject patch)
    Since there is only a single entry in "power-domains", the genpd
    core code automatically attaches the CPR platform device as consumer
    of the VDDMX_AO power domain. I don't want this, I want it to become
    child of the VDDMX_AO genpd.

    I added some hacky code to workaround this. One option that works is
    to add a second dummy entry to "power-domains", which will prevent
    the genpd core from attaching the power domain:

    	power-domains = <&rpmpd MSM8916_VDDMX_AO>, <0>;

    The other option is detaching the power domain again in probe(),
    after setting it up as parent domain:

	struct of_phandle_args parent, child;

	child.np = dev->of_node;
	child.args_count = 0;

	of_parse_phandle_with_args(dev->of_node, "power-domains",
 			           "#power-domain-cells", 0, &parent));
	of_genpd_add_subdomain(&parent, &child);

	/* Detach power domain since it's managed via the subdomain */
	dev_pm_domain_detach(dev, false);

    Is there a cleaner way to handle this? Mainly a question for Uffe.

 2. The OPP WARNing triggers with both variants because it just checks
    if "required-opps" has a single entry. I guess we need extra checks
    to exclude the "parent genpd" case compared to the "OPP" case.

	[    1.116244] WARNING: CPU: 2 PID: 36 at drivers/opp/of.c:331 _link_required_opps+0x180/0x1cc
	[    1.125897] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
	[    1.146887] pc : _link_required_opps+0x180/0x1cc
	[    1.146902] lr : _link_required_opps+0xdc/0x1cc
	[    1.276408] Call trace:
	[    1.283519]  _link_required_opps+0x180/0x1cc
	[    1.285779]  _of_add_table_indexed+0x61c/0xd40
	[    1.290292]  dev_pm_opp_of_add_table+0x10/0x18
	[    1.294546]  of_genpd_add_provider_simple+0x80/0x160
	[    1.298974]  cpr_probe+0x6a0/0x97c
	[    1.304092]  platform_probe+0x64/0xbc

It does seem to work correctly, with and without this patch. So I guess
another option might be to simply silence this WARN_ON(). :')

Thanks,
Stephan

[1]: https://lore.kernel.org/linux-pm/CAPDyKFoH5EOvRRKy-Bgp_B9B3rf=PUKK5N45s5PNgfBi55PaOQ@mail.gmail.com/

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Viresh Kumar]

On 25-10-23, 15:47, Stephan Gerhold wrote:
> FWIW I'm hitting this WARNing when trying to set up the parent domain
> setup for CPR->RPMPD(MX) on MSM8916 that I discussed with Uffe recently
> [1]. I know, me and all my weird OPP setups. :'D
> 
> Basically, I have cpufreq voting for performance states of the CPR genpd
> (via required-opps). CPR is supposed to have <&rpmpd MSM8916_VDDMX_AO>
> as parent genpd and translates to the parent performance state using the
> "required-opps" in the *CPR* OPP table:
> 
> 	cpr: power-controller@b018000 {
> 		compatible = "qcom,msm8916-cpr", "qcom,cpr";
> 		reg = <0x0b018000 0x1000>;
> 		/* ... */
> 		#power-domain-cells = <0>;
> 		operating-points-v2 = <&cpr_opp_table>;
> 		/* Supposed to be parent domain, not consumer */
> 		power-domains = <&rpmpd MSM8916_VDDMX_AO>;
> 
> 		cpr_opp_table: opp-table {
> 			compatible = "operating-points-v2-qcom-level";
> 
> 			cpr_opp1: opp1 {
> 				opp-level = <1>;
> 				qcom,opp-fuse-level = <1>;
> 				required-opps = <&rpmpd_opp_svs_soc>;
> 			};
> 			cpr_opp2: opp2 {
> 				opp-level = <2>;
> 				qcom,opp-fuse-level = <2>;
> 				required-opps = <&rpmpd_opp_nom>;
> 			};
> 			cpr_opp3: opp3 {
> 				opp-level = <3>;
> 				qcom,opp-fuse-level = <3>;
> 				required-opps = <&rpmpd_opp_super_turbo>;
> 			};
> 		};
> 	};

I have forgotten a bit about this usecase. How exactly does the
configurations work currently for this ? I mean genpd core must be
setting the vote finally for only one of them or something else ?

-- 
viresh

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Stephan Gerhold]

On Wed, Oct 25, 2023 at 08:54:31PM +0530, Viresh Kumar wrote:
> On 25-10-23, 15:47, Stephan Gerhold wrote:
> > FWIW I'm hitting this WARNing when trying to set up the parent domain
> > setup for CPR->RPMPD(MX) on MSM8916 that I discussed with Uffe recently
> > [1]. I know, me and all my weird OPP setups. :'D
> > 
> > Basically, I have cpufreq voting for performance states of the CPR genpd
> > (via required-opps). CPR is supposed to have <&rpmpd MSM8916_VDDMX_AO>
> > as parent genpd and translates to the parent performance state using the
> > "required-opps" in the *CPR* OPP table:
> > 
> > 	cpr: power-controller@b018000 {
> > 		compatible = "qcom,msm8916-cpr", "qcom,cpr";
> > 		reg = <0x0b018000 0x1000>;
> > 		/* ... */
> > 		#power-domain-cells = <0>;
> > 		operating-points-v2 = <&cpr_opp_table>;
> > 		/* Supposed to be parent domain, not consumer */
> > 		power-domains = <&rpmpd MSM8916_VDDMX_AO>;
> > 
> > 		cpr_opp_table: opp-table {
> > 			compatible = "operating-points-v2-qcom-level";
> > 
> > 			cpr_opp1: opp1 {
> > 				opp-level = <1>;
> > 				qcom,opp-fuse-level = <1>;
> > 				required-opps = <&rpmpd_opp_svs_soc>;
> > 			};
> > 			cpr_opp2: opp2 {
> > 				opp-level = <2>;
> > 				qcom,opp-fuse-level = <2>;
> > 				required-opps = <&rpmpd_opp_nom>;
> > 			};
> > 			cpr_opp3: opp3 {
> > 				opp-level = <3>;
> > 				qcom,opp-fuse-level = <3>;
> > 				required-opps = <&rpmpd_opp_super_turbo>;
> > 			};
> > 		};
> > 	};
> 
> I have forgotten a bit about this usecase. How exactly does the
> configurations work currently for this ? I mean genpd core must be
> setting the vote finally for only one of them or something else ?
> 

I'm not sure if I understand your question correctly. Basically, setting
<&rpmpd MSM8916_VDDMX_AO> as "parent genpd" of <&cpr> is supposed to
describe that there is a direct relationship between the performance
states of CPR and VDDMX. When changing the CPR performance state, VDDMX
should also be adjusted accordingly.

This is implemented in the genpd core in _genpd_set_performance_state().
It loops over the parent genpds, and re-evaluates the performance states
of each of them. Translation happens using genpd_xlate_performance_state()
which is just a direct call to dev_pm_opp_xlate_performance_state().
This will look up the required-opps from the OPP table above. However,
the genpd core calls ->set_performance_state() on the parent genpd
directly, so dev_pm_opp_set_opp() isn't involved in this case.

Overall the call sequence for a CPUfreq switch will look something like:

 - cpu0: dev_pm_opp_set_rate(998.4 MHz)
  - cpu0: _set_required_opps(opp-998400000)
   - genpd:1:cpu0: dev_pm_opp_set_opp(&cpr_opp3)
    - genpd:1:cpu0: _set_opp_level(&cpr_opp3)
     - cpr: _genpd_set_performance_state(3)

      # genpd: translate & adjust parent performance states
      - cpr: genpd_xlate_performance_state(parent=VDDMX_AO)
             => &rpmpd_opp_super_turbo = 6
       - VDDMX_AO: _genpd_set_performance_state(6)
        - rpmpd: ->set_performance_state(VDDMX_AO, 6)

      # genpd: change actual performance state
      - cpr: ->set_performance_state(cpr, 3)

Before the discussion with Uffe I did not describe this relationship
between CPR<->VDDMX as parent-child, I just had them as two separate
power domains in the CPU OPP table. That worked fine too but Uffe
suggested the parent-child representation might be better.
   
Does that help or were you looking for something else? :D

Thanks,
Stephan

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Ulf Hansson]

On Wed, 25 Oct 2023 at 15:49, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Wed, Oct 25, 2023 at 12:40:26PM +0200, Ulf Hansson wrote:
> > On Wed, 25 Oct 2023 at 08:55, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 19-10-23, 13:16, Ulf Hansson wrote:
> > > > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > > +static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table,
> > > > >                                struct opp_table *required_table, int index)
> > > > >  {
> > > > >         struct device_node *np;
> > > > > @@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp,
> > > > >                 return -ENODEV;
> > > > >         }
> > > > >
> > > > > +       /*
> > > > > +        * There are two genpd (as required-opp) cases that we need to handle,
> > > > > +        * devices with a single genpd and ones with multiple genpds.
> > > > > +        *
> > > > > +        * The single genpd case requires special handling as we need to use the
> > > > > +        * same `dev` structure (instead of a virtual one provided by genpd
> > > > > +        * core) for setting the performance state. Lets treat this as a case
> > > > > +        * where the OPP's level is directly available without required genpd
> > > > > +        * link in the DT.
> > > > > +        *
> > > > > +        * Just update the `level` with the right value, which
> > > > > +        * dev_pm_opp_set_opp() will take care of in the normal path itself.
> > > > > +        */
> > > > > +       if (required_table->is_genpd && opp_table->required_opp_count == 1 &&
> > > > > +           !opp_table->genpd_virt_devs) {
> > > > > +               if (!WARN_ON(opp->level))
> > > >
> > > > Hmm. Doesn't this introduce an unnecessary limitation?
> > > >
> > > > An opp node that has a required-opps phande, may have "opp-hz",
> > > > "opp-microvolt", etc. Why would we not allow the "opp-level" to be
> > > > used too?
> > >
> > > Coming back to this, why would we ever want a device to have "opp-level" and
> > > "required-opp" (set to genpd's table) ? That would mean we will call:
> > >
> > > dev_pm_domain_set_performance_state() twice to set different level values.
> >
> > Yes - and that would be weird, especially since the PM domain (genpd)
> > is already managing the aggregation and propagation to parent domains.
> >
>
> FWIW I'm hitting this WARNing when trying to set up the parent domain
> setup for CPR->RPMPD(MX) on MSM8916 that I discussed with Uffe recently
> [1]. I know, me and all my weird OPP setups. :'D
>
> Basically, I have cpufreq voting for performance states of the CPR genpd
> (via required-opps). CPR is supposed to have <&rpmpd MSM8916_VDDMX_AO>
> as parent genpd and translates to the parent performance state using the
> "required-opps" in the *CPR* OPP table:
>
>         cpr: power-controller@b018000 {
>                 compatible = "qcom,msm8916-cpr", "qcom,cpr";
>                 reg = <0x0b018000 0x1000>;
>                 /* ... */
>                 #power-domain-cells = <0>;
>                 operating-points-v2 = <&cpr_opp_table>;
>                 /* Supposed to be parent domain, not consumer */
>                 power-domains = <&rpmpd MSM8916_VDDMX_AO>;
>
>                 cpr_opp_table: opp-table {
>                         compatible = "operating-points-v2-qcom-level";
>
>                         cpr_opp1: opp1 {
>                                 opp-level = <1>;
>                                 qcom,opp-fuse-level = <1>;
>                                 required-opps = <&rpmpd_opp_svs_soc>;
>                         };
>                         cpr_opp2: opp2 {
>                                 opp-level = <2>;
>                                 qcom,opp-fuse-level = <2>;
>                                 required-opps = <&rpmpd_opp_nom>;
>                         };
>                         cpr_opp3: opp3 {
>                                 opp-level = <3>;
>                                 qcom,opp-fuse-level = <3>;
>                                 required-opps = <&rpmpd_opp_super_turbo>;
>                         };
>                 };
>         };
>
> There are two problems with this:
>
>  1. (Unrelated to $subject patch)
>     Since there is only a single entry in "power-domains", the genpd
>     core code automatically attaches the CPR platform device as consumer
>     of the VDDMX_AO power domain. I don't want this, I want it to become
>     child of the VDDMX_AO genpd.
>
>     I added some hacky code to workaround this. One option that works is
>     to add a second dummy entry to "power-domains", which will prevent
>     the genpd core from attaching the power domain:
>
>         power-domains = <&rpmpd MSM8916_VDDMX_AO>, <0>;

Hmm, looks a bit hackish to me.

>
>     The other option is detaching the power domain again in probe(),
>     after setting it up as parent domain:

Yes, if needed.

>
>         struct of_phandle_args parent, child;
>
>         child.np = dev->of_node;
>         child.args_count = 0;
>
>         of_parse_phandle_with_args(dev->of_node, "power-domains",
>                                    "#power-domain-cells", 0, &parent));
>         of_genpd_add_subdomain(&parent, &child);
>
>         /* Detach power domain since it's managed via the subdomain */
>         dev_pm_domain_detach(dev, false);
>
>     Is there a cleaner way to handle this? Mainly a question for Uffe.

At the moment, I don't think so. In fact, we have situations when the
attachment is really useful.

For example, during ->probe(), one can do a pm_runtime_get_sync() to
power-on the "parent" domain. This may be needed to synchronize the
power-states between the child/parent-domains, before calling
of_genpd_add_subdomain().

>
>  2. The OPP WARNing triggers with both variants because it just checks
>     if "required-opps" has a single entry. I guess we need extra checks
>     to exclude the "parent genpd" case compared to the "OPP" case.
>
>         [    1.116244] WARNING: CPU: 2 PID: 36 at drivers/opp/of.c:331 _link_required_opps+0x180/0x1cc
>         [    1.125897] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
>         [    1.146887] pc : _link_required_opps+0x180/0x1cc
>         [    1.146902] lr : _link_required_opps+0xdc/0x1cc
>         [    1.276408] Call trace:
>         [    1.283519]  _link_required_opps+0x180/0x1cc
>         [    1.285779]  _of_add_table_indexed+0x61c/0xd40
>         [    1.290292]  dev_pm_opp_of_add_table+0x10/0x18
>         [    1.294546]  of_genpd_add_provider_simple+0x80/0x160
>         [    1.298974]  cpr_probe+0x6a0/0x97c
>         [    1.304092]  platform_probe+0x64/0xbc
>
> It does seem to work correctly, with and without this patch. So I guess
> another option might be to simply silence this WARN_ON(). :')

Oh, thanks for pointing this out! This case haven't crossed my mind yet!

Allow me to think a bit more about it. I will get back to you again
with a suggestion soon, unless Viresh comes back first. :-)

[...]

Kind regards
Uffe

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Viresh Kumar]

On 26-10-23, 11:53, Ulf Hansson wrote:
> On Wed, 25 Oct 2023 at 15:49, Stephan Gerhold <stephan@gerhold.net> wrote:
> >  2. The OPP WARNing triggers with both variants because it just checks
> >     if "required-opps" has a single entry. I guess we need extra checks
> >     to exclude the "parent genpd" case compared to the "OPP" case.
> >
> >         [    1.116244] WARNING: CPU: 2 PID: 36 at drivers/opp/of.c:331 _link_required_opps+0x180/0x1cc
> >         [    1.125897] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
> >         [    1.146887] pc : _link_required_opps+0x180/0x1cc
> >         [    1.146902] lr : _link_required_opps+0xdc/0x1cc
> >         [    1.276408] Call trace:
> >         [    1.283519]  _link_required_opps+0x180/0x1cc
> >         [    1.285779]  _of_add_table_indexed+0x61c/0xd40
> >         [    1.290292]  dev_pm_opp_of_add_table+0x10/0x18
> >         [    1.294546]  of_genpd_add_provider_simple+0x80/0x160
> >         [    1.298974]  cpr_probe+0x6a0/0x97c
> >         [    1.304092]  platform_probe+0x64/0xbc
> >
> > It does seem to work correctly, with and without this patch. So I guess
> > another option might be to simply silence this WARN_ON(). :')
> 
> Oh, thanks for pointing this out! This case haven't crossed my mind yet!
> 
> Allow me to think a bit more about it. I will get back to you again
> with a suggestion soon, unless Viresh comes back first. :-)

I have resent the series now.

Stephan, please give it a try again. Thanks.

Regarding this case where a genpd's table points to a parent genpd's table via
the required-opps, it is a bit tricky to solve and the only way around that I
could think of is that someone needs to call dev_pm_opp_set_config() with the
right device pointer, with that we won't hit the warning anymore and things will
work as expected.

In this case the OPP core needs to call dev_pm_domain_set_performance_state()
for device and then its genpd. We need the right device pointers :(

Ulf, also another important thing here is that maybe we would want the genpd
core to not propagate the voting anymore to the parent genpd's ? The
dev_pm_opp_set_opp() call is better placed at handling all things and not just
the performance state, like clk, regulator, bandwidth and so the recursion
should happen at OPP level only. For now my series shouldn't break anything,
just that we will try to set performance state twice for the parent genpd, the
second call should silently return as the target state should be equal to
current state.

-- 
viresh

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Ulf Hansson]

On Mon, 30 Oct 2023 at 11:29, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 26-10-23, 11:53, Ulf Hansson wrote:
> > On Wed, 25 Oct 2023 at 15:49, Stephan Gerhold <stephan@gerhold.net> wrote:
> > >  2. The OPP WARNing triggers with both variants because it just checks
> > >     if "required-opps" has a single entry. I guess we need extra checks
> > >     to exclude the "parent genpd" case compared to the "OPP" case.
> > >
> > >         [    1.116244] WARNING: CPU: 2 PID: 36 at drivers/opp/of.c:331 _link_required_opps+0x180/0x1cc
> > >         [    1.125897] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
> > >         [    1.146887] pc : _link_required_opps+0x180/0x1cc
> > >         [    1.146902] lr : _link_required_opps+0xdc/0x1cc
> > >         [    1.276408] Call trace:
> > >         [    1.283519]  _link_required_opps+0x180/0x1cc
> > >         [    1.285779]  _of_add_table_indexed+0x61c/0xd40
> > >         [    1.290292]  dev_pm_opp_of_add_table+0x10/0x18
> > >         [    1.294546]  of_genpd_add_provider_simple+0x80/0x160
> > >         [    1.298974]  cpr_probe+0x6a0/0x97c
> > >         [    1.304092]  platform_probe+0x64/0xbc
> > >
> > > It does seem to work correctly, with and without this patch. So I guess
> > > another option might be to simply silence this WARN_ON(). :')
> >
> > Oh, thanks for pointing this out! This case haven't crossed my mind yet!
> >
> > Allow me to think a bit more about it. I will get back to you again
> > with a suggestion soon, unless Viresh comes back first. :-)
>
> I have resent the series now.
>
> Stephan, please give it a try again. Thanks.
>
> Regarding this case where a genpd's table points to a parent genpd's table via
> the required-opps, it is a bit tricky to solve and the only way around that I
> could think of is that someone needs to call dev_pm_opp_set_config() with the
> right device pointer, with that we won't hit the warning anymore and things will
> work as expected.
>
> In this case the OPP core needs to call dev_pm_domain_set_performance_state()
> for device and then its genpd. We need the right device pointers :(
>
> Ulf, also another important thing here is that maybe we would want the genpd
> core to not propagate the voting anymore to the parent genpd's ? The
> dev_pm_opp_set_opp() call is better placed at handling all things and not just
> the performance state, like clk, regulator, bandwidth and so the recursion
> should happen at OPP level only.

Are you saying that the OPP library should be capable of managing the
parent-clock-rates too, when there is a new rate being requested for a
clock that belongs to an OPP? To me, that sounds like replicating
framework specific knowledge into the OPP library, no? Why do we want
this?

Unless I totally misunderstood your suggestion, I think it would be
better if the OPP library remained simple and didn't run recursive
calls, but instead relied on each framework to manage the aggregation
and propagation to parents.

> For now my series shouldn't break anything,
> just that we will try to set performance state twice for the parent genpd, the
> second call should silently return as the target state should be equal to
> current state.
>
> --
> viresh

Kind regards
Uffe

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Viresh Kumar]

On 03-11-23, 12:58, Ulf Hansson wrote:
> Are you saying that the OPP library should be capable of managing the
> parent-clock-rates too, when there is a new rate being requested for a
> clock that belongs to an OPP? To me, that sounds like replicating
> framework specific knowledge into the OPP library, no? Why do we want
> this?

I am surely not touching clocks or any other framework :)

> Unless I totally misunderstood your suggestion, I think it would be
> better if the OPP library remained simple and didn't run recursive
> calls, but instead relied on each framework to manage the aggregation
> and propagation to parents.

I see your point and agree with it.

Here is the problem and I am not very sure what's the way forward for this then:

- Devices can have other devices (like caches) or genpds mentioned via
  required-opps.

- Same is true for genpds, they can also have required-opps, which may or may not
  be other genpds.

- When OPP core is asked to set a device's OPP, it isn't only about performance
  level, but clk, level, regulator, bw, etc. And so a full call to
  dev_pm_opp_set_opp() is required.

- The OPP core is going to run the helper recursively only for required-opps and
  hence it won't affect clock or regulators.

- But it currently affects genpds as they are mentioned in required-opps.

- Skipping the recursive call to a parent genpd will require a special hack,
  maybe we should add it, I am just discussing it if we should or if there is
  another way around this.

-- 
viresh

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Ulf Hansson]

On Mon, 6 Nov 2023 at 08:08, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 03-11-23, 12:58, Ulf Hansson wrote:
> > Are you saying that the OPP library should be capable of managing the
> > parent-clock-rates too, when there is a new rate being requested for a
> > clock that belongs to an OPP? To me, that sounds like replicating
> > framework specific knowledge into the OPP library, no? Why do we want
> > this?
>
> I am surely not touching clocks or any other framework :)
>
> > Unless I totally misunderstood your suggestion, I think it would be
> > better if the OPP library remained simple and didn't run recursive
> > calls, but instead relied on each framework to manage the aggregation
> > and propagation to parents.
>
> I see your point and agree with it.

Okay!

>
> Here is the problem and I am not very sure what's the way forward for this then:
>
> - Devices can have other devices (like caches) or genpds mentioned via
>   required-opps.
>
> - Same is true for genpds, they can also have required-opps, which may or may not
>   be other genpds.
>
> - When OPP core is asked to set a device's OPP, it isn't only about performance
>   level, but clk, level, regulator, bw, etc. And so a full call to
>   dev_pm_opp_set_opp() is required.
>
> - The OPP core is going to run the helper recursively only for required-opps and
>   hence it won't affect clock or regulators.

What if a required-opps has a clock or regulator? Doesn't that mean
that clocks/regulators could be called too?

>
> - But it currently affects genpds as they are mentioned in required-opps.
>
> - Skipping the recursive call to a parent genpd will require a special hack,
>   maybe we should add it, I am just discussing it if we should or if there is
>   another way around this.

Right, I see.

If this is only for required-opps and devices being hooked up to a PM
domain (genpd), my suggestion would be to keep avoiding doing the
propagation to required-opps-parents. For the similar reasons to why
we don't do it for clock/regulators, the propagation and aggregation,
seems to me, to belong better in genpd.

Did that make sense?

Kind regards
Uffe

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Viresh Kumar]

On 10-11-23, 14:50, Ulf Hansson wrote:
> If this is only for required-opps and devices being hooked up to a PM
> domain (genpd), my suggestion would be to keep avoiding doing the
> propagation to required-opps-parents. For the similar reasons to why
> we don't do it for clock/regulators, the propagation and aggregation,
> seems to me, to belong better in genpd.
> 
> Did that make sense?

Hmm, it does. Let me see what I can do on this..

-- 
viresh

Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

[Viresh Kumar]

On 15-11-23, 11:02, Viresh Kumar wrote:
> On 10-11-23, 14:50, Ulf Hansson wrote:
> > If this is only for required-opps and devices being hooked up to a PM
> > domain (genpd), my suggestion would be to keep avoiding doing the
> > propagation to required-opps-parents. For the similar reasons to why
> > we don't do it for clock/regulators, the propagation and aggregation,
> > seems to me, to belong better in genpd.
> > 
> > Did that make sense?
> 
> Hmm, it does. Let me see what I can do on this..

Hi Ulf,

I have sent V3 with a new commit at the end to take care of this.

-- 
viresh

[PATCH 2/2] OPP: Call dev_pm_opp_set_opp() for required OPPs

[Viresh Kumar]

Configuring the required OPP was never properly implemented, we just
took an exception for genpds and configured them directly, while leaving
out all other required OPP types.

Now that a standard call to dev_pm_opp_set_opp() takes care of
configuring the opp->level too, the special handling for genpds can be
avoided by simply calling dev_pm_opp_set_opp() for the required OPPs,
which shall eventually configure the corresponding level for genpds.

This also makes it possible for us to configure other type of required
OPPs (no concrete users yet though), via the same path. This is how
other frameworks take care of parent nodes, like clock, regulators, etc,
where we recursively call the same helper.

In order to call dev_pm_opp_set_opp() for the virtual genpd devices,
they must share the OPP table of the genpd. Call _add_opp_dev() for them
to get that done.

This commit also extends the struct dev_pm_opp_config to pass required
devices, for non-genpd cases, which can be used to call
dev_pm_opp_set_opp() for the non-genpd required devices.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 144 ++++++++++++++++++-----------------------
 drivers/opp/of.c       |  12 ++--
 drivers/opp/opp.h      |   8 +--
 include/linux/pm_opp.h |   7 +-
 4 files changed, 76 insertions(+), 95 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index aab8c8e79146..056b51abc501 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1046,42 +1046,19 @@ static int _set_opp_bw(const struct opp_table *opp_table,
 	return 0;
 }
 
-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]->level: 0;
-	int ret;
-
-	if (!pd_dev)
-		return 0;
-
-	ret = dev_pm_domain_set_performance_state(pd_dev, pstate);
-	if (ret) {
-		dev_err(dev, "Failed to set performance state of %s: %d (%d)\n",
-			dev_name(pd_dev), pstate, ret);
-	}
-
-	return ret;
-}
-
-static int _opp_set_required_opps_generic(struct device *dev,
-	struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down)
-{
-	dev_err(dev, "setting required-opps isn't supported for non-genpd devices\n");
-	return -ENOENT;
-}
-
-static int _opp_set_required_opps_genpd(struct device *dev,
-	struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down)
+/* This is only called for PM domain for now */
+static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
+			      struct dev_pm_opp *opp, bool up)
 {
-	struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
+	struct device **devs = opp_table->required_devs;
 	int index, target, delta, ret;
 
-	if (!genpd_virt_devs)
-		return 0;
+	/* required-opps not fully initialized yet */
+	if (lazy_linking_pending(opp_table))
+		return -EBUSY;
 
 	/* Scaling up? Set required OPPs in normal order, else reverse */
-	if (!scaling_down) {
+	if (up) {
 		index = 0;
 		target = opp_table->required_opp_count;
 		delta = 1;
@@ -1092,9 +1069,11 @@ static int _opp_set_required_opps_genpd(struct device *dev,
 	}
 
 	while (index != target) {
-		ret = _set_performance_state(dev, genpd_virt_devs[index], opp, index);
-		if (ret)
-			return ret;
+		if (devs[index]) {
+			ret = dev_pm_opp_set_opp(devs[index], opp);
+			if (ret)
+				return ret;
+		}
 
 		index += delta;
 	}
@@ -1102,34 +1081,6 @@ static int _opp_set_required_opps_genpd(struct device *dev,
 	return 0;
 }
 
-/* This is only called for PM domain for now */
-static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
-			      struct dev_pm_opp *opp, bool up)
-{
-	/* required-opps not fully initialized yet */
-	if (lazy_linking_pending(opp_table))
-		return -EBUSY;
-
-	if (opp_table->set_required_opps)
-		return opp_table->set_required_opps(dev, opp_table, opp, up);
-
-	return 0;
-}
-
-/* Update set_required_opps handler */
-void _update_set_required_opps(struct opp_table *opp_table)
-{
-	/* Already set */
-	if (opp_table->set_required_opps)
-		return;
-
-	/* All required OPPs will belong to genpd or none */
-	if (opp_table->required_opp_tables[0]->is_genpd)
-		opp_table->set_required_opps = _opp_set_required_opps_genpd;
-	else
-		opp_table->set_required_opps = _opp_set_required_opps_generic;
-}
-
 static int _set_opp_level(struct device *dev, struct opp_table *opp_table,
 			  struct dev_pm_opp *opp)
 {
@@ -2390,19 +2341,13 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
 {
 	int index;
 
-	if (!opp_table->genpd_virt_devs)
-		return;
-
 	for (index = 0; index < opp_table->required_opp_count; index++) {
-		if (!opp_table->genpd_virt_devs[index])
+		if (!opp_table->required_devs[index])
 			continue;
 
-		dev_pm_domain_detach(opp_table->genpd_virt_devs[index], false);
-		opp_table->genpd_virt_devs[index] = NULL;
+		dev_pm_domain_detach(opp_table->required_devs[index], false);
+		opp_table->required_devs[index] = NULL;
 	}
-
-	kfree(opp_table->genpd_virt_devs);
-	opp_table->genpd_virt_devs = NULL;
 }
 
 /*
@@ -2429,15 +2374,10 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
 	int index = 0, ret = -EINVAL;
 	const char * const *name = names;
 
-	if (opp_table->genpd_virt_devs)
+	/* Checking only the first one is enough ? */
+	if (opp_table->required_devs[0])
 		return 0;
 
-	opp_table->genpd_virt_devs = kcalloc(opp_table->required_opp_count,
-					     sizeof(*opp_table->genpd_virt_devs),
-					     GFP_KERNEL);
-	if (!opp_table->genpd_virt_devs)
-		return -ENOMEM;
-
 	while (*name) {
 		if (index >= opp_table->required_opp_count) {
 			dev_err(dev, "Index can't be greater than required-opp-count - 1, %s (%d : %d)\n",
@@ -2452,13 +2392,25 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
 			goto err;
 		}
 
-		opp_table->genpd_virt_devs[index] = virt_dev;
+		/*
+		 * Add the virtual genpd device as a user of the OPP table, so
+		 * we can call dev_pm_opp_set_opp() on it directly.
+		 *
+		 * This will be automatically removed when the OPP table is
+		 * removed, don't need to handle that here.
+		 */
+		if (!_add_opp_dev(virt_dev, opp_table->required_opp_tables[index])) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		opp_table->required_devs[index] = virt_dev;
 		index++;
 		name++;
 	}
 
 	if (virt_devs)
-		*virt_devs = opp_table->genpd_virt_devs;
+		*virt_devs = opp_table->required_devs;
 
 	return 0;
 
@@ -2468,10 +2420,34 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
 
 }
 
+static void _opp_set_required_devs(struct opp_table *opp_table,
+				   struct device **required_devs)
+{
+	int i;
+
+	/* Another CPU that shares the OPP table has set the required devs ? */
+	if (opp_table->required_devs[0])
+		return;
+
+	for (i = 0; i < opp_table->required_opp_count; i++)
+		opp_table->required_devs[i] = required_devs[i];
+}
+
+static void _opp_put_required_devs(struct opp_table *opp_table)
+{
+	int i;
+
+	for (i = 0; i < opp_table->required_opp_count; i++)
+		opp_table->required_devs[i] = NULL;
+}
+
 static void _opp_clear_config(struct opp_config_data *data)
 {
-	if (data->flags & OPP_CONFIG_GENPD)
+	if (data->flags & OPP_CONFIG_REQUIRED_DEVS)
+		_opp_put_required_devs(data->opp_table);
+	else if (data->flags & OPP_CONFIG_GENPD)
 		_opp_detach_genpd(data->opp_table);
+
 	if (data->flags & OPP_CONFIG_REGULATOR)
 		_opp_put_regulators(data->opp_table);
 	if (data->flags & OPP_CONFIG_SUPPORTED_HW)
@@ -2585,12 +2561,18 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
 
 	/* Attach genpds */
 	if (config->genpd_names) {
+		if (config->required_devs)
+			goto err;
+
 		ret = _opp_attach_genpd(opp_table, dev, config->genpd_names,
 					config->virt_devs);
 		if (ret)
 			goto err;
 
 		data->flags |= OPP_CONFIG_GENPD;
+	} else if (config->required_devs) {
+		_opp_set_required_devs(opp_table, config->required_devs);
+		data->flags |= OPP_CONFIG_REQUIRED_DEVS;
 	}
 
 	ret = xa_alloc(&opp_configs, &id, data, XA_LIMIT(1, INT_MAX),
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index e056f31a48b5..27659d23d54d 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -165,7 +165,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 	struct opp_table **required_opp_tables;
 	struct device_node *required_np, *np;
 	bool lazy = false;
-	int count, i;
+	int count, i, size;
 
 	/* Traversing the first OPP node is all we need */
 	np = of_get_next_available_child(opp_np, NULL);
@@ -179,12 +179,13 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 	if (count <= 0)
 		goto put_np;
 
-	required_opp_tables = kcalloc(count, sizeof(*required_opp_tables),
-				      GFP_KERNEL);
+	size = sizeof(*required_opp_tables) + sizeof(*opp_table->required_devs);
+	required_opp_tables = kcalloc(count, size, GFP_KERNEL);
 	if (!required_opp_tables)
 		goto put_np;
 
 	opp_table->required_opp_tables = required_opp_tables;
+	opp_table->required_devs = (void *)(required_opp_tables + count);
 	opp_table->required_opp_count = count;
 
 	for (i = 0; i < count; i++) {
@@ -208,8 +209,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 		mutex_lock(&opp_table_lock);
 		list_add(&opp_table->lazy, &lazy_opp_tables);
 		mutex_unlock(&opp_table_lock);
-	} else {
-		_update_set_required_opps(opp_table);
 	}
 
 	goto put_np;
@@ -328,7 +327,7 @@ static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_tab
 	 * dev_pm_opp_set_opp() will take care of in the normal path itself.
 	 */
 	if (required_table->is_genpd && opp_table->required_opp_count == 1 &&
-	    !opp_table->genpd_virt_devs) {
+	    !opp_table->required_devs[0]) {
 		if (!WARN_ON(opp->level))
 			opp->level = opp->required_opps[0]->level;
 	}
@@ -441,7 +440,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
 
 		/* All required opp-tables found, remove from lazy list */
 		if (!lazy) {
-			_update_set_required_opps(opp_table);
 			list_del_init(&opp_table->lazy);
 
 			list_for_each_entry(opp, &opp_table->opp_list, node)
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 08366f90f16b..23dcb2fbf8c3 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -35,6 +35,7 @@ extern struct list_head opp_tables;
 #define OPP_CONFIG_PROP_NAME		BIT(3)
 #define OPP_CONFIG_SUPPORTED_HW		BIT(4)
 #define OPP_CONFIG_GENPD		BIT(5)
+#define OPP_CONFIG_REQUIRED_DEVS	BIT(6)
 
 /**
  * struct opp_config_data - data for set config operations
@@ -160,9 +161,9 @@ enum opp_table_access {
  * @rate_clk_single: Currently configured frequency for single clk.
  * @current_opp: Currently configured OPP for the table.
  * @suspend_opp: Pointer to OPP to be used during device suspend.
- * @genpd_virt_devs: List of virtual devices for multiple genpd support.
  * @required_opp_tables: List of device OPP tables that are required by OPPs in
  *		this table.
+ * @required_devs: List of devices for required OPP tables.
  * @required_opp_count: Number of required devices.
  * @supported_hw: Array of version number to support.
  * @supported_hw_count: Number of elements in supported_hw array.
@@ -180,7 +181,6 @@ enum opp_table_access {
  * @path_count: Number of interconnect paths
  * @enabled: Set to true if the device's resources are enabled/configured.
  * @is_genpd: Marks if the OPP table belongs to a genpd.
- * @set_required_opps: Helper responsible to set required OPPs.
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
@@ -211,8 +211,8 @@ struct opp_table {
 	struct dev_pm_opp *current_opp;
 	struct dev_pm_opp *suspend_opp;
 
-	struct device **genpd_virt_devs;
 	struct opp_table **required_opp_tables;
+	struct device **required_devs;
 	unsigned int required_opp_count;
 
 	unsigned int *supported_hw;
@@ -229,8 +229,6 @@ struct opp_table {
 	unsigned int path_count;
 	bool enabled;
 	bool is_genpd;
-	int (*set_required_opps)(struct device *dev,
-		struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down);
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index ccd97bcef269..25f7bcbea7ab 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -74,8 +74,10 @@ typedef int (*config_clks_t)(struct device *dev, struct opp_table *opp_table,
  * @supported_hw_count: Number of elements in the array.
  * @regulator_names: Array of pointers to the names of the regulator, NULL terminated.
  * @genpd_names: Null terminated array of pointers containing names of genpd to
- *		 attach.
- * @virt_devs: Pointer to return the array of virtual devices.
+ *		attach. Mutually exclusive with required_devs.
+ * @virt_devs: Pointer to return the array of genpd virtual devices. Mutually
+ *		exclusive with required_devs.
+ * @required_devs: Required OPP devices. Mutually exclusive with genpd_names/virt_devs.
  *
  * This structure contains platform specific OPP configurations for the device.
  */
@@ -90,6 +92,7 @@ struct dev_pm_opp_config {
 	const char * const *regulator_names;
 	const char * const *genpd_names;
 	struct device ***virt_devs;
+	struct device **required_devs;
 };
 
 /**
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH 2/2] OPP: Call dev_pm_opp_set_opp() for required OPPs

[Stephan Gerhold]

On Thu, Oct 19, 2023 at 03:52:01PM +0530, Viresh Kumar wrote:
> Configuring the required OPP was never properly implemented, we just
> took an exception for genpds and configured them directly, while leaving
> out all other required OPP types.
> 
> Now that a standard call to dev_pm_opp_set_opp() takes care of
> configuring the opp->level too, the special handling for genpds can be
> avoided by simply calling dev_pm_opp_set_opp() for the required OPPs,
> which shall eventually configure the corresponding level for genpds.
> 
> This also makes it possible for us to configure other type of required
> OPPs (no concrete users yet though), via the same path. This is how
> other frameworks take care of parent nodes, like clock, regulators, etc,
> where we recursively call the same helper.
> 
> In order to call dev_pm_opp_set_opp() for the virtual genpd devices,
> they must share the OPP table of the genpd. Call _add_opp_dev() for them
> to get that done.
> 
> This commit also extends the struct dev_pm_opp_config to pass required
> devices, for non-genpd cases, which can be used to call
> dev_pm_opp_set_opp() for the non-genpd required devices.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c     | 144 ++++++++++++++++++-----------------------
>  drivers/opp/of.c       |  12 ++--
>  drivers/opp/opp.h      |   8 +--
>  include/linux/pm_opp.h |   7 +-
>  4 files changed, 76 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index aab8c8e79146..056b51abc501 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1046,42 +1046,19 @@ static int _set_opp_bw(const struct opp_table *opp_table,
>  	return 0;
>  }
>  
> -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]->level: 0;

Unfortunately this patch breaks scaling the performance state of the
virtual genpd devices completely. As far as I can tell it just keeps
setting level = 0 for every OPP switch (this is on MSM8909 with [1],
a single "perf" power domain attached to the CPU):

[  457.252255] cpu cpu0: _set_opp: switching OPP: Freq 998400000 -> 799999804 Hz, Level 0 -> 0, Bw 3200000 -> 1600000
[  457.253739] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 998400000 -> 799999804 Hz, Level 0 -> 0, Bw 3200000 -> 1600000
[  457.304581] cpu cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000
[  457.306091] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000
[  457.323489] cpu cpu0: _set_opp: switching OPP: Freq 533333202 -> 399999902 Hz, Level 0 -> 0, Bw 1600000 -> 800000
[  457.352792] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 533333202 -> 399999902 Hz, Level 0 -> 0, Bw 1600000 -> 800000
[  457.353056] cpu cpu0: _set_opp: switching OPP: Freq 399999902 -> 199999951 Hz, Level 0 -> 0, Bw 800000 -> 800000
[  457.355285] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 399999902 -> 199999951 Hz, Level 0 -> 0, Bw 800000 -> 800000

Where do you handle setting the pstate specified in the "required-opps"
of the OPP table with this patch? I've looked at your changes for some
time but must admit I haven't really understood how it is supposed to
work. :-)

Thanks,
Stephan

[1]: https://lore.kernel.org/linux-arm-msm/20231018-msm8909-cpufreq-v2-0-0962df95f654@kernkonzept.com/

> -	int ret;
> -
> -	if (!pd_dev)
> -		return 0;
> -
> -	ret = dev_pm_domain_set_performance_state(pd_dev, pstate);
> -	if (ret) {
> -		dev_err(dev, "Failed to set performance state of %s: %d (%d)\n",
> -			dev_name(pd_dev), pstate, ret);
> -	}
> -
> -	return ret;
> -}
> -
> -static int _opp_set_required_opps_generic(struct device *dev,
> -	struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down)
> -{
> -	dev_err(dev, "setting required-opps isn't supported for non-genpd devices\n");
> -	return -ENOENT;
> -}
> -
> -static int _opp_set_required_opps_genpd(struct device *dev,
> -	struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down)
> +/* This is only called for PM domain for now */
> +static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
> +			      struct dev_pm_opp *opp, bool up)
>  {
> -	struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
> +	struct device **devs = opp_table->required_devs;
>  	int index, target, delta, ret;
>  
> -	if (!genpd_virt_devs)
> -		return 0;
> +	/* required-opps not fully initialized yet */
> +	if (lazy_linking_pending(opp_table))
> +		return -EBUSY;
>  
>  	/* Scaling up? Set required OPPs in normal order, else reverse */
> -	if (!scaling_down) {
> +	if (up) {
>  		index = 0;
>  		target = opp_table->required_opp_count;
>  		delta = 1;
> @@ -1092,9 +1069,11 @@ static int _opp_set_required_opps_genpd(struct device *dev,
>  	}
>  
>  	while (index != target) {
> -		ret = _set_performance_state(dev, genpd_virt_devs[index], opp, index);
> -		if (ret)
> -			return ret;
> +		if (devs[index]) {
> +			ret = dev_pm_opp_set_opp(devs[index], opp);
> +			if (ret)
> +				return ret;
> +		}
>  
>  		index += delta;
>  	}
> @@ -1102,34 +1081,6 @@ static int _opp_set_required_opps_genpd(struct device *dev,
>  	return 0;
>  }
>  
> -/* This is only called for PM domain for now */
> -static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
> -			      struct dev_pm_opp *opp, bool up)
> -{
> -	/* required-opps not fully initialized yet */
> -	if (lazy_linking_pending(opp_table))
> -		return -EBUSY;
> -
> -	if (opp_table->set_required_opps)
> -		return opp_table->set_required_opps(dev, opp_table, opp, up);
> -
> -	return 0;
> -}
> -
> -/* Update set_required_opps handler */
> -void _update_set_required_opps(struct opp_table *opp_table)
> -{
> -	/* Already set */
> -	if (opp_table->set_required_opps)
> -		return;
> -
> -	/* All required OPPs will belong to genpd or none */
> -	if (opp_table->required_opp_tables[0]->is_genpd)
> -		opp_table->set_required_opps = _opp_set_required_opps_genpd;
> -	else
> -		opp_table->set_required_opps = _opp_set_required_opps_generic;
> -}
> -
>  static int _set_opp_level(struct device *dev, struct opp_table *opp_table,
>  			  struct dev_pm_opp *opp)
>  {
> @@ -2390,19 +2341,13 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
>  {
>  	int index;
>  
> -	if (!opp_table->genpd_virt_devs)
> -		return;
> -
>  	for (index = 0; index < opp_table->required_opp_count; index++) {
> -		if (!opp_table->genpd_virt_devs[index])
> +		if (!opp_table->required_devs[index])
>  			continue;
>  
> -		dev_pm_domain_detach(opp_table->genpd_virt_devs[index], false);
> -		opp_table->genpd_virt_devs[index] = NULL;
> +		dev_pm_domain_detach(opp_table->required_devs[index], false);
> +		opp_table->required_devs[index] = NULL;
>  	}
> -
> -	kfree(opp_table->genpd_virt_devs);
> -	opp_table->genpd_virt_devs = NULL;
>  }
>  
>  /*
> @@ -2429,15 +2374,10 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
>  	int index = 0, ret = -EINVAL;
>  	const char * const *name = names;
>  
> -	if (opp_table->genpd_virt_devs)
> +	/* Checking only the first one is enough ? */
> +	if (opp_table->required_devs[0])
>  		return 0;
>  
> -	opp_table->genpd_virt_devs = kcalloc(opp_table->required_opp_count,
> -					     sizeof(*opp_table->genpd_virt_devs),
> -					     GFP_KERNEL);
> -	if (!opp_table->genpd_virt_devs)
> -		return -ENOMEM;
> -
>  	while (*name) {
>  		if (index >= opp_table->required_opp_count) {
>  			dev_err(dev, "Index can't be greater than required-opp-count - 1, %s (%d : %d)\n",
> @@ -2452,13 +2392,25 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
>  			goto err;
>  		}
>  
> -		opp_table->genpd_virt_devs[index] = virt_dev;
> +		/*
> +		 * Add the virtual genpd device as a user of the OPP table, so
> +		 * we can call dev_pm_opp_set_opp() on it directly.
> +		 *
> +		 * This will be automatically removed when the OPP table is
> +		 * removed, don't need to handle that here.
> +		 */
> +		if (!_add_opp_dev(virt_dev, opp_table->required_opp_tables[index])) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
> +
> +		opp_table->required_devs[index] = virt_dev;
>  		index++;
>  		name++;
>  	}
>  
>  	if (virt_devs)
> -		*virt_devs = opp_table->genpd_virt_devs;
> +		*virt_devs = opp_table->required_devs;
>  
>  	return 0;
>  
> @@ -2468,10 +2420,34 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
>  
>  }
>  
> +static void _opp_set_required_devs(struct opp_table *opp_table,
> +				   struct device **required_devs)
> +{
> +	int i;
> +
> +	/* Another CPU that shares the OPP table has set the required devs ? */
> +	if (opp_table->required_devs[0])
> +		return;
> +
> +	for (i = 0; i < opp_table->required_opp_count; i++)
> +		opp_table->required_devs[i] = required_devs[i];
> +}
> +
> +static void _opp_put_required_devs(struct opp_table *opp_table)
> +{
> +	int i;
> +
> +	for (i = 0; i < opp_table->required_opp_count; i++)
> +		opp_table->required_devs[i] = NULL;
> +}
> +
>  static void _opp_clear_config(struct opp_config_data *data)
>  {
> -	if (data->flags & OPP_CONFIG_GENPD)
> +	if (data->flags & OPP_CONFIG_REQUIRED_DEVS)
> +		_opp_put_required_devs(data->opp_table);
> +	else if (data->flags & OPP_CONFIG_GENPD)
>  		_opp_detach_genpd(data->opp_table);
> +
>  	if (data->flags & OPP_CONFIG_REGULATOR)
>  		_opp_put_regulators(data->opp_table);
>  	if (data->flags & OPP_CONFIG_SUPPORTED_HW)
> @@ -2585,12 +2561,18 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
>  
>  	/* Attach genpds */
>  	if (config->genpd_names) {
> +		if (config->required_devs)
> +			goto err;
> +
>  		ret = _opp_attach_genpd(opp_table, dev, config->genpd_names,
>  					config->virt_devs);
>  		if (ret)
>  			goto err;
>  
>  		data->flags |= OPP_CONFIG_GENPD;
> +	} else if (config->required_devs) {
> +		_opp_set_required_devs(opp_table, config->required_devs);
> +		data->flags |= OPP_CONFIG_REQUIRED_DEVS;
>  	}
>  
>  	ret = xa_alloc(&opp_configs, &id, data, XA_LIMIT(1, INT_MAX),
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index e056f31a48b5..27659d23d54d 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -165,7 +165,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>  	struct opp_table **required_opp_tables;
>  	struct device_node *required_np, *np;
>  	bool lazy = false;
> -	int count, i;
> +	int count, i, size;
>  
>  	/* Traversing the first OPP node is all we need */
>  	np = of_get_next_available_child(opp_np, NULL);
> @@ -179,12 +179,13 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>  	if (count <= 0)
>  		goto put_np;
>  
> -	required_opp_tables = kcalloc(count, sizeof(*required_opp_tables),
> -				      GFP_KERNEL);
> +	size = sizeof(*required_opp_tables) + sizeof(*opp_table->required_devs);
> +	required_opp_tables = kcalloc(count, size, GFP_KERNEL);
>  	if (!required_opp_tables)
>  		goto put_np;
>  
>  	opp_table->required_opp_tables = required_opp_tables;
> +	opp_table->required_devs = (void *)(required_opp_tables + count);
>  	opp_table->required_opp_count = count;
>  
>  	for (i = 0; i < count; i++) {
> @@ -208,8 +209,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>  		mutex_lock(&opp_table_lock);
>  		list_add(&opp_table->lazy, &lazy_opp_tables);
>  		mutex_unlock(&opp_table_lock);
> -	} else {
> -		_update_set_required_opps(opp_table);
>  	}
>  
>  	goto put_np;
> @@ -328,7 +327,7 @@ static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_tab
>  	 * dev_pm_opp_set_opp() will take care of in the normal path itself.
>  	 */
>  	if (required_table->is_genpd && opp_table->required_opp_count == 1 &&
> -	    !opp_table->genpd_virt_devs) {
> +	    !opp_table->required_devs[0]) {
>  		if (!WARN_ON(opp->level))
>  			opp->level = opp->required_opps[0]->level;
>  	}
> @@ -441,7 +440,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
>  
>  		/* All required opp-tables found, remove from lazy list */
>  		if (!lazy) {
> -			_update_set_required_opps(opp_table);
>  			list_del_init(&opp_table->lazy);
>  
>  			list_for_each_entry(opp, &opp_table->opp_list, node)
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 08366f90f16b..23dcb2fbf8c3 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -35,6 +35,7 @@ extern struct list_head opp_tables;
>  #define OPP_CONFIG_PROP_NAME		BIT(3)
>  #define OPP_CONFIG_SUPPORTED_HW		BIT(4)
>  #define OPP_CONFIG_GENPD		BIT(5)
> +#define OPP_CONFIG_REQUIRED_DEVS	BIT(6)
>  
>  /**
>   * struct opp_config_data - data for set config operations
> @@ -160,9 +161,9 @@ enum opp_table_access {
>   * @rate_clk_single: Currently configured frequency for single clk.
>   * @current_opp: Currently configured OPP for the table.
>   * @suspend_opp: Pointer to OPP to be used during device suspend.
> - * @genpd_virt_devs: List of virtual devices for multiple genpd support.
>   * @required_opp_tables: List of device OPP tables that are required by OPPs in
>   *		this table.
> + * @required_devs: List of devices for required OPP tables.
>   * @required_opp_count: Number of required devices.
>   * @supported_hw: Array of version number to support.
>   * @supported_hw_count: Number of elements in supported_hw array.
> @@ -180,7 +181,6 @@ enum opp_table_access {
>   * @path_count: Number of interconnect paths
>   * @enabled: Set to true if the device's resources are enabled/configured.
>   * @is_genpd: Marks if the OPP table belongs to a genpd.
> - * @set_required_opps: Helper responsible to set required OPPs.
>   * @dentry:	debugfs dentry pointer of the real device directory (not links).
>   * @dentry_name: Name of the real dentry.
>   *
> @@ -211,8 +211,8 @@ struct opp_table {
>  	struct dev_pm_opp *current_opp;
>  	struct dev_pm_opp *suspend_opp;
>  
> -	struct device **genpd_virt_devs;
>  	struct opp_table **required_opp_tables;
> +	struct device **required_devs;
>  	unsigned int required_opp_count;
>  
>  	unsigned int *supported_hw;
> @@ -229,8 +229,6 @@ struct opp_table {
>  	unsigned int path_count;
>  	bool enabled;
>  	bool is_genpd;
> -	int (*set_required_opps)(struct device *dev,
> -		struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down);
>  
>  #ifdef CONFIG_DEBUG_FS
>  	struct dentry *dentry;
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index ccd97bcef269..25f7bcbea7ab 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -74,8 +74,10 @@ typedef int (*config_clks_t)(struct device *dev, struct opp_table *opp_table,
>   * @supported_hw_count: Number of elements in the array.
>   * @regulator_names: Array of pointers to the names of the regulator, NULL terminated.
>   * @genpd_names: Null terminated array of pointers containing names of genpd to
> - *		 attach.
> - * @virt_devs: Pointer to return the array of virtual devices.
> + *		attach. Mutually exclusive with required_devs.
> + * @virt_devs: Pointer to return the array of genpd virtual devices. Mutually
> + *		exclusive with required_devs.
> + * @required_devs: Required OPP devices. Mutually exclusive with genpd_names/virt_devs.
>   *
>   * This structure contains platform specific OPP configurations for the device.
>   */
> @@ -90,6 +92,7 @@ struct dev_pm_opp_config {
>  	const char * const *regulator_names;
>  	const char * const *genpd_names;
>  	struct device ***virt_devs;
> +	struct device **required_devs;
>  };
>  
>  /**
> -- 
> 2.31.1.272.g89b43f80a514
> 

-- 
Stephan Gerhold <stephan.gerhold@kernkonzept.com>
Kernkonzept GmbH at Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael Hohmuth

Re: [PATCH 2/2] OPP: Call dev_pm_opp_set_opp() for required OPPs

[Viresh Kumar]

Thanks a lot for taking this up, really appreciate it Stephan.

On 24-10-23, 13:18, Stephan Gerhold wrote:
> Unfortunately this patch breaks scaling the performance state of the
> virtual genpd devices completely. As far as I can tell it just keeps
> setting level = 0 for every OPP switch (this is on MSM8909 with [1],
> a single "perf" power domain attached to the CPU):
> 
> [  457.252255] cpu cpu0: _set_opp: switching OPP: Freq 998400000 -> 799999804 Hz, Level 0 -> 0, Bw 3200000 -> 1600000
> [  457.253739] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 998400000 -> 799999804 Hz, Level 0 -> 0, Bw 3200000 -> 1600000

The thing I was more worried about worked fine it seems (recursively calling
dev_pm_opp_set_opp() i.e.).

> [  457.304581] cpu cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000
> [  457.306091] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000
> [  457.323489] cpu cpu0: _set_opp: switching OPP: Freq 533333202 -> 399999902 Hz, Level 0 -> 0, Bw 1600000 -> 800000
> [  457.352792] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 533333202 -> 399999902 Hz, Level 0 -> 0, Bw 1600000 -> 800000
> [  457.353056] cpu cpu0: _set_opp: switching OPP: Freq 399999902 -> 199999951 Hz, Level 0 -> 0, Bw 800000 -> 800000
> [  457.355285] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 399999902 -> 199999951 Hz, Level 0 -> 0, Bw 800000 -> 800000
> 
> Where do you handle setting the pstate specified in the "required-opps"
> of the OPP table with this patch? I've looked at your changes for some
> time but must admit I haven't really understood how it is supposed to
> work. :-)

Thanks for the debug print, they helped me find the issue.

> [  457.304581] cpu cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000
> [  457.306091] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000

The second line shouldn't have had freq/bw/etc, but just level. Hopefully this
will fix it. Pushed to my branch too. Thanks. Please try again.

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 056b51abc501..cb2b353129c6 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1070,7 +1070,7 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,

        while (index != target) {
                if (devs[index]) {
-                       ret = dev_pm_opp_set_opp(devs[index], opp);
+                       ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]);
                        if (ret)
                                return ret;
                }

-- 
viresh

Re: [PATCH 2/2] OPP: Call dev_pm_opp_set_opp() for required OPPs

[Stephan Gerhold]

On Wed, Oct 25, 2023 at 01:06:34PM +0530, Viresh Kumar wrote:
> Thanks a lot for taking this up, really appreciate it Stephan.
> 
> On 24-10-23, 13:18, Stephan Gerhold wrote:
> > Unfortunately this patch breaks scaling the performance state of the
> > virtual genpd devices completely. As far as I can tell it just keeps
> > setting level = 0 for every OPP switch (this is on MSM8909 with [1],
> > a single "perf" power domain attached to the CPU):
> > 
> > [  457.252255] cpu cpu0: _set_opp: switching OPP: Freq 998400000 -> 799999804 Hz, Level 0 -> 0, Bw 3200000 -> 1600000
> > [  457.253739] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 998400000 -> 799999804 Hz, Level 0 -> 0, Bw 3200000 -> 1600000
> 
> The thing I was more worried about worked fine it seems (recursively calling
> dev_pm_opp_set_opp() i.e.).
> 
> > [  457.304581] cpu cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000
> > [  457.306091] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000
> > [  457.323489] cpu cpu0: _set_opp: switching OPP: Freq 533333202 -> 399999902 Hz, Level 0 -> 0, Bw 1600000 -> 800000
> > [  457.352792] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 533333202 -> 399999902 Hz, Level 0 -> 0, Bw 1600000 -> 800000
> > [  457.353056] cpu cpu0: _set_opp: switching OPP: Freq 399999902 -> 199999951 Hz, Level 0 -> 0, Bw 800000 -> 800000
> > [  457.355285] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 399999902 -> 199999951 Hz, Level 0 -> 0, Bw 800000 -> 800000
> > 
> > Where do you handle setting the pstate specified in the "required-opps"
> > of the OPP table with this patch? I've looked at your changes for some
> > time but must admit I haven't really understood how it is supposed to
> > work. :-)
> 
> Thanks for the debug print, they helped me find the issue.
> 
> > [  457.304581] cpu cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000
> > [  457.306091] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000
> 
> The second line shouldn't have had freq/bw/etc, but just level. Hopefully this
> will fix it. Pushed to my branch too. Thanks. Please try again.
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 056b51abc501..cb2b353129c6 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1070,7 +1070,7 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
> 
>         while (index != target) {
>                 if (devs[index]) {
> -                       ret = dev_pm_opp_set_opp(devs[index], opp);
> +                       ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]);
>                         if (ret)
>                                 return ret;
>                 }
> 

Thanks, this seems to work fine.

I found another small problem: In my OPP setup for MSM8916, some of the
required-opps point to an OPP with "opp-level = <0>" (see [1], the
<&rpmpd_opp_none> in the cpu-opp-table). This is because the vote for
the "CX" domain is for the CPU PLL clock source, which is only used for
the higher CPU frequencies (>= 998.4 MHz). With the previous code you
made it possible for me to vote for opp-level = <0> in commit
a5663c9b1e31 ("opp: Allow opp-level to be set to 0", discussion in [2]).
I think now it's broken because the _set_opp_level() added by Uffe
checks for if (!opp->level) as a sign that there is no opp-level defined
at all.

Based on my longer discussion with Uffe recently [3] it's not entirely
clear yet if I will still have the reference to &rpmpd_opp_none in the
future. Alternatively, we discussed describing this differently, e.g. as
a parent power domain (which would bypass most of the OPP code), or
moving it directly to an OPP table of CPU PLL device node (which would
only describe the actual "active" required-opps).

I'm not sure if anyone else has a reasonable use case for pointing to a
required-opp with opp-level = <0>, so we could potentially also postpone
solving this to later.

Thanks,
Stephan

[1]: https://github.com/msm8916-mainline/linux/commit/8880f39108206d7a60a0a8351c0373bddf58657c
[2]: https://lore.kernel.org/linux-pm/20200911092538.jexqm6joww67d4yv@vireshk-i7/
[3]: https://lore.kernel.org/linux-arm-msm/ZQGqfMigCFZP_HLA@gerhold.net/

Re: [PATCH 2/2] OPP: Call dev_pm_opp_set_opp() for required OPPs

[Viresh Kumar]

On 25-10-23, 14:17, Stephan Gerhold wrote:
> Thanks, this seems to work fine.

Thanks a lot.

> I found another small problem: In my OPP setup for MSM8916, some of the
> required-opps point to an OPP with "opp-level = <0>" (see [1], the
> <&rpmpd_opp_none> in the cpu-opp-table). This is because the vote for
> the "CX" domain is for the CPU PLL clock source, which is only used for
> the higher CPU frequencies (>= 998.4 MHz). With the previous code you
> made it possible for me to vote for opp-level = <0> in commit
> a5663c9b1e31 ("opp: Allow opp-level to be set to 0", discussion in [2]).
> I think now it's broken because the _set_opp_level() added by Uffe
> checks for if (!opp->level) as a sign that there is no opp-level defined
> at all.

Yes, we broke that. I think a simple fix is to initialize the level
with an impossible value, like -1 and then 0 becomes valid.

> Based on my longer discussion with Uffe recently [3] it's not entirely
> clear yet if I will still have the reference to &rpmpd_opp_none in the
> future. Alternatively, we discussed describing this differently, e.g. as
> a parent power domain (which would bypass most of the OPP code), or
> moving it directly to an OPP table of CPU PLL device node (which would
> only describe the actual "active" required-opps).
> 
> I'm not sure if anyone else has a reasonable use case for pointing to a
> required-opp with opp-level = <0>, so we could potentially also postpone
> solving this to later.

I would like to fix this respectively. Thanks for bringing this up.

-- 
viresh

Re: [PATCH 2/2] OPP: Call dev_pm_opp_set_opp() for required OPPs

[Ulf Hansson]

On Wed, 25 Oct 2023 at 17:20, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 25-10-23, 14:17, Stephan Gerhold wrote:
> > Thanks, this seems to work fine.
>
> Thanks a lot.
>
> > I found another small problem: In my OPP setup for MSM8916, some of the
> > required-opps point to an OPP with "opp-level = <0>" (see [1], the
> > <&rpmpd_opp_none> in the cpu-opp-table). This is because the vote for
> > the "CX" domain is for the CPU PLL clock source, which is only used for
> > the higher CPU frequencies (>= 998.4 MHz). With the previous code you
> > made it possible for me to vote for opp-level = <0> in commit
> > a5663c9b1e31 ("opp: Allow opp-level to be set to 0", discussion in [2]).
> > I think now it's broken because the _set_opp_level() added by Uffe
> > checks for if (!opp->level) as a sign that there is no opp-level defined
> > at all.

I don't think my patch broke it, I just followed the similar semantics
of how we treated the OPPs for the required opps.

As far as I understood it, the only way we could request the
performance-level to be zero, was if the consumer driver would call
dev_pm_opp_set_opp(dev, NULL);

So, unless I am overlooking something, things can have been screwed up
earlier too?

>
> Yes, we broke that. I think a simple fix is to initialize the level
> with an impossible value, like -1 and then 0 becomes valid.

Yep, make sense, let's fix it!

Are you sending a patch?

[...]

Kind regards
Uffe

Re: [PATCH 2/2] OPP: Call dev_pm_opp_set_opp() for required OPPs

[Viresh Kumar]

On 25-10-23, 18:03, Ulf Hansson wrote:
> I don't think my patch broke it, I just followed the similar semantics
> of how we treated the OPPs for the required opps.

Yes, your commit didn't break it as it didn't touch the required-opps part. So
it is my commit that moved over to the generic solution that breaks it.

> Are you sending a patch?

Yes.

-- 
viresh

Re: [PATCH 2/2] OPP: Call dev_pm_opp_set_opp() for required OPPs

[Ulf Hansson]

On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Configuring the required OPP was never properly implemented, we just
> took an exception for genpds and configured them directly, while leaving
> out all other required OPP types.
>
> Now that a standard call to dev_pm_opp_set_opp() takes care of
> configuring the opp->level too, the special handling for genpds can be
> avoided by simply calling dev_pm_opp_set_opp() for the required OPPs,
> which shall eventually configure the corresponding level for genpds.
>
> This also makes it possible for us to configure other type of required
> OPPs (no concrete users yet though), via the same path. This is how
> other frameworks take care of parent nodes, like clock, regulators, etc,
> where we recursively call the same helper.
>
> In order to call dev_pm_opp_set_opp() for the virtual genpd devices,
> they must share the OPP table of the genpd. Call _add_opp_dev() for them
> to get that done.
>
> This commit also extends the struct dev_pm_opp_config to pass required
> devices, for non-genpd cases, which can be used to call
> dev_pm_opp_set_opp() for the non-genpd required devices.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c     | 144 ++++++++++++++++++-----------------------
>  drivers/opp/of.c       |  12 ++--
>  drivers/opp/opp.h      |   8 +--
>  include/linux/pm_opp.h |   7 +-
>  4 files changed, 76 insertions(+), 95 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index aab8c8e79146..056b51abc501 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c

[...]

> -static int _opp_set_required_opps_genpd(struct device *dev,
> -       struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down)
> +/* This is only called for PM domain for now */
> +static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
> +                             struct dev_pm_opp *opp, bool up)
>  {
> -       struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
> +       struct device **devs = opp_table->required_devs;
>         int index, target, delta, ret;
>
> -       if (!genpd_virt_devs)
> -               return 0;

Rather than continue the path below, wouldn't it be better to return 0
"if (!devs)" here?

If I understand correctly, the code below does manage this condition,
so it's not strictly needed though.

> +       /* required-opps not fully initialized yet */
> +       if (lazy_linking_pending(opp_table))
> +               return -EBUSY;
>
>         /* Scaling up? Set required OPPs in normal order, else reverse */
> -       if (!scaling_down) {
> +       if (up) {
>                 index = 0;
>                 target = opp_table->required_opp_count;
>                 delta = 1;
> @@ -1092,9 +1069,11 @@ static int _opp_set_required_opps_genpd(struct device *dev,
>         }
>
>         while (index != target) {
> -               ret = _set_performance_state(dev, genpd_virt_devs[index], opp, index);
> -               if (ret)
> -                       return ret;
> +               if (devs[index]) {
> +                       ret = dev_pm_opp_set_opp(devs[index], opp);
> +                       if (ret)
> +                               return ret;
> +               }
>
>                 index += delta;
>         }

[...]

>
>  /*
> @@ -2429,15 +2374,10 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
>         int index = 0, ret = -EINVAL;
>         const char * const *name = names;
>
> -       if (opp_table->genpd_virt_devs)
> +       /* Checking only the first one is enough ? */
> +       if (opp_table->required_devs[0])

The allocation of opp_table->required_devs is being done from
_opp_table_alloc_required_tables(), which doesn't necessarily
allocate/assign the data for it.

Maybe check "opp_table->required_devs" instead, to make that clear?

>                 return 0;
>
> -       opp_table->genpd_virt_devs = kcalloc(opp_table->required_opp_count,
> -                                            sizeof(*opp_table->genpd_virt_devs),
> -                                            GFP_KERNEL);
> -       if (!opp_table->genpd_virt_devs)
> -               return -ENOMEM;
> -
>         while (*name) {
>                 if (index >= opp_table->required_opp_count) {
>                         dev_err(dev, "Index can't be greater than required-opp-count - 1, %s (%d : %d)\n",
> @@ -2452,13 +2392,25 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
>                         goto err;
>                 }
>
> -               opp_table->genpd_virt_devs[index] = virt_dev;
> +               /*
> +                * Add the virtual genpd device as a user of the OPP table, so
> +                * we can call dev_pm_opp_set_opp() on it directly.
> +                *
> +                * This will be automatically removed when the OPP table is
> +                * removed, don't need to handle that here.
> +                */
> +               if (!_add_opp_dev(virt_dev, opp_table->required_opp_tables[index])) {
> +                       ret = -ENOMEM;
> +                       goto err;
> +               }
> +
> +               opp_table->required_devs[index] = virt_dev;
>                 index++;
>                 name++;
>         }
>
>         if (virt_devs)
> -               *virt_devs = opp_table->genpd_virt_devs;
> +               *virt_devs = opp_table->required_devs;
>
>         return 0;
>
> @@ -2468,10 +2420,34 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
>
>  }
>
> +static void _opp_set_required_devs(struct opp_table *opp_table,
> +                                  struct device **required_devs)
> +{
> +       int i;
> +
> +       /* Another CPU that shares the OPP table has set the required devs ? */

Not sure I fully understand the above comment. Is this the only
relevant use-case or could there be others too?

> +       if (opp_table->required_devs[0])

Maybe check opp_table->required_devs instead?

> +               return;
> +
> +       for (i = 0; i < opp_table->required_opp_count; i++)
> +               opp_table->required_devs[i] = required_devs[i];

To be safe, don't we need to check the in-parameter required_devs?

Or we should simply rely on the callers of dev_pm_opp_set_config() to
do the right thing?

[...]

Besides the minor things above, this looks really great to me! Feel free to add:

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

Kind regards
Uffe

Re: [PATCH 2/2] OPP: Call dev_pm_opp_set_opp() for required OPPs

[Viresh Kumar]

On 25-10-23, 15:51, Ulf Hansson wrote:
> On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > -static int _opp_set_required_opps_genpd(struct device *dev,
> > -       struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down)
> > +/* This is only called for PM domain for now */
> > +static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
> > +                             struct dev_pm_opp *opp, bool up)
> >  {
> > -       struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
> > +       struct device **devs = opp_table->required_devs;
> >         int index, target, delta, ret;
> >
> > -       if (!genpd_virt_devs)
> > -               return 0;
> 
> Rather than continue the path below, wouldn't it be better to return 0
> "if (!devs)" here?

I can add that optimization, moreover it would make the code simpler
to read.

> > @@ -2429,15 +2374,10 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
> >         int index = 0, ret = -EINVAL;
> >         const char * const *name = names;
> >
> > -       if (opp_table->genpd_virt_devs)
> > +       /* Checking only the first one is enough ? */
> > +       if (opp_table->required_devs[0])
> 
> The allocation of opp_table->required_devs is being done from
> _opp_table_alloc_required_tables(), which doesn't necessarily
> allocate/assign the data for it.
> 
> Maybe check "opp_table->required_devs" instead, to make that clear?

Hmm, the expectation here is that if someone has called the config
routine with genpd option, then required OPPs must be present and so
required_devs won't be NULL.

What instead we are looking to check here, and later in
_opp_set_required_devs(), is if this operation is already done for a
group of devices.

The OPP table is shared, for example, between CPUs of the same cluster
and the OPP core supports the config routine getting called for all of
them, in a loop. In that case, we just increase the ref count and
return without redoing stuff. That's why we were checking for
genpd_virt_devs earlier.

Though interfaces and things have changed several times, I may need to
check it again to make sure it all works as expected.

> > +static void _opp_set_required_devs(struct opp_table *opp_table,
> > +                                  struct device **required_devs)
> > +{
> > +       int i;
> > +
> > +       /* Another CPU that shares the OPP table has set the required devs ? */
> 
> Not sure I fully understand the above comment. Is this the only
> relevant use-case or could there be others too?

Answered above, but I shouldn't write CPU here anymore. We support all
device types now.

> > +       if (opp_table->required_devs[0])
> 
> Maybe check opp_table->required_devs instead?
> 
> > +               return;
> > +
> > +       for (i = 0; i < opp_table->required_opp_count; i++)
> > +               opp_table->required_devs[i] = required_devs[i];
> 
> To be safe, don't we need to check the in-parameter required_devs?

I left it like that intentionally, in case someone wants to skip the
required dev for some required OPP, while make all others change. But
maybe I will keep it simpler and check it all the time.

> Or we should simply rely on the callers of dev_pm_opp_set_config() to
> do the right thing?
> 
> [...]
> 
> Besides the minor things above, this looks really great to me! Feel free to add:

Thanks.

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

-- 
viresh