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

-- 
viresh