Re: [PATCH 1/2] OPP: Add API to update EM after adjustment of voltage for OPPs

[Lukasz Luba]

On 12/21/23 07:28, Xuewen Yan wrote:
> On Wed, Dec 20, 2023 at 7:02 PM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
> > There are device drivers which can modify voltage values for OPPs. It
> > could be due to the chip binning and those drivers have specific chip
> > knowledge about this. This adjustment can happen after Energy Model is
> > registered, thus EM can have stale data about power.
> >
> > Introduce new API function which can be used by device driver which
> > adjusted the voltage for OPPs. The implementation takes care about
> > calculating needed internal details in the new EM table ('cost' field).
> > It plugs in the new EM table to the framework so other subsystems would
> > use the correct data.
> >
> > Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> > ---
> >   drivers/opp/of.c       | 69 ++++++++++++++++++++++++++++++++++++++++++
> >   include/linux/pm_opp.h |  6 ++++
> >   2 files changed, 75 insertions(+)
> >
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index 81fa27599d58..992434c0b711 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -1596,3 +1596,72 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
> >          return ret;
> >   }
> >   EXPORT_SYMBOL_GPL(dev_pm_opp_of_register_em);
> > +
> > +/**
> > + * dev_pm_opp_of_update_em() - Update Energy Model with new power values
> > + * @dev                : Device for which an Energy Model has to be registered
> > + *
> > + * This uses the "dynamic-power-coefficient" devicetree property to calculate
> > + * power values for EM. It uses the new adjusted voltage values known for OPPs
> > + * which have changed after boot.
> > + */
> > +int dev_pm_opp_of_update_em(struct device *dev)
> > +{
> > +       struct em_perf_table __rcu *runtime_table;
> > +       struct em_perf_state *table, *new_table;
> > +       struct em_perf_domain *pd;
> > +       int ret, table_size, i;
> > +
> > +       if (IS_ERR_OR_NULL(dev))
> > +               return -EINVAL;
> > +
> > +       pd = em_pd_get(dev);
> > +       if (!pd) {
> > +               dev_warn(dev, "Couldn't find Energy Model %d\n", ret);
> > +               return -EINVAL;
> > +       }
> > +
> > +       runtime_table = em_allocate_table(pd);
> > +       if (!runtime_table) {
> > +               dev_warn(dev, "new EM allocation failed\n");
> > +               return -ENOMEM;
> > +       }
> > +
> > +       new_table = runtime_table->state;
> > +
> > +       table = em_get_table(pd);
> > +       /* Initialize data based on older EM table */
> > +       table_size = sizeof(struct em_perf_state) * pd->nr_perf_states;
> > +       memcpy(new_table, table, table_size);
> > +
> > +       em_put_table();
> > +
> > +       /* Update power values which might change due to new voltage in OPPs */
> > +       for (i = 0; i < pd->nr_perf_states; i++) {
> > +               unsigned long freq = new_table[i].frequency;
> > +               unsigned long power;
> > +
> > +               ret = _get_power(dev, &power, &freq);
> > +               if (ret)
> > +                       goto failed;
>
> Need we use the EM_SET_ACTIVE_POWER_CB(em_cb, _get_power) and call
> em_cb->active_power?

No, not in this case. It's not like registration of EM, when there
is a need to also pass the callback function. As you can see this code
operates locally and the call _get_power() just simply gets the
power in straight way. Later the whole 'runtime_table' is passed to the
EM framework to 'swap' the pointers under RCU.

Thanks Xuewen for having a look at this!