Re: [PATCH v6 3/3] OPP: Add helper function for bandwidth OPP tables

[Saravana Kannan <saravanak@google.com>]

On Wed, Jan 8, 2020 at 8:40 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 08-01-20, 16:58, Saravana Kannan wrote:
> > On Wed, Jan 8, 2020 at 3:19 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >  /**
> > > >   * dev_pm_opp_get_level() - Gets the level corresponding to an available opp
> > > >   * @opp:     opp for which level value has to be returned for
> > > > @@ -299,6 +322,34 @@ unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_freq);
> > > >
> > > > +/**
> > > > + * dev_pm_opp_get_suspend_opp_bw() - Get peak bandwidth of suspend opp in kBps
> > >
> > > Hmm, I wasn't expecting this. So the interconnects will also have a
> > > suspend OPP ?
> >
> > Yes, device voting for interconnect paths might want to lower the
> > bandwidth to a suspend bandwidth when they suspend.
>
> That's exactly what I was saying, the request for a change during
> suspend should come from the device

Agree. And this tells the device requesting for bandwidth, what
bandwidth to vote for when it's suspending. Keep in mind that these
bandwidth OPP tables are used by the consumers of the interconnects to
pick from a list of bandwidths to vote for.

> and you can't do it here, i.e.
> they should lower their frequency requirement, which should lead to a
> low bandwidth automatically.

Agree, the OPP framework itself shouldn't be responsible. And I'm not
doing anything here? Just giving those devices a way to look up what
their suspend bandwidth is? So they can vote for it when they suspend?

> > > > + * @dev:     device for which we do this operation
> > > > + * @avg_bw:  Pointer where the corresponding average bandwidth is stored.
> > > > + *           Can be NULL.
> > > > + *
> > > > + * Return: This function returns the peak bandwidth of the OPP marked as
> > > > + * suspend_opp if one is available, else returns 0;
> > > > + */
> > > > +unsigned long dev_pm_opp_get_suspend_opp_bw(struct device *dev,
> > > > +                                         unsigned long *avg_bw)
> > > > +{
> > > > +     struct opp_table *opp_table;
> > > > +     unsigned long peak_bw = 0;
> > > > +
> > > > +     opp_table = _find_opp_table(dev);
> > > > +     if (IS_ERR(opp_table))
> > > > +             return 0;
> > > > +
> > > > +     if (opp_table->suspend_opp && opp_table->suspend_opp->available)
> > > > +             peak_bw = dev_pm_opp_get_bw(opp_table->suspend_opp, avg_bw);
> > > > +
> > > > +     dev_pm_opp_put_opp_table(opp_table);
> > > > +
> > > > +     return peak_bw;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_bw);
> > > > +
> > > >  int _get_opp_count(struct opp_table *opp_table)
> > > >  {
> > > >       struct dev_pm_opp *opp;
> > > > @@ -343,6 +394,40 @@ int dev_pm_opp_get_opp_count(struct device *dev)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_count);
> > > >
> > >
> > > I think we should add function header here instead of the helpers
> > > which get exact match for freq, bw or level. And then pass a enum
> > > value to it, which tells what we are looking to compare. After that
> > > rest of the routines will be just one liners, make them macros in
> > > header file itself.
> >
> > Not sure I understand what you are saying here.
>
> Okay, lemme try again with proper example.
>
> enum opp_key_type {
>         OPP_KEY_FREQ = 0x1,
>         OPP_KEY_LEVEL= 0x2,
>         OPP_KEY_BW   = 0x4,
>         OPP_KEY_ALL  = 0x7,
> }
>
> /**
>  * Add function header here..
>  */
> struct dev_pm_opp *dev_pm_opp_find_opp_exact(struct device *dev,
>                                              enum opp_key_type key,
>                                              unsigned long key_value,
>                                              bool available)
> {
>        struct opp_table *opp_table;
>        struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
>
>        opp_table = _find_opp_table(dev);
>        if (IS_ERR(opp_table)) {
>                int r = PTR_ERR(opp_table);
>
>                dev_err(dev, "%s: OPP table not found (%d)\n", __func__, r);
>                return ERR_PTR(r);
>        }
>
>        mutex_lock(&opp_table->lock);
>
>        list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
>                if (temp_opp->available == available &&
>                    !opp_compare_key(temp_opp, key, key_value)) {
>                        opp = temp_opp;
>
>                        /* Increment the reference count of OPP */
>                        dev_pm_opp_get(opp);
>                        break;
>                }
>        }
>
>        mutex_unlock(&opp_table->lock);
>        dev_pm_opp_put_opp_table(opp_table);
>
>        return opp;
> }
>
> //Now in header file
>
> #define dev_pm_opp_find_freq_exact(dev, freq, available)        dev_pm_opp_find_opp_exact(dev, OPP_KEY_FREQ, freq, available);
> #define dev_pm_opp_find_level_exact(dev, level, available)      dev_pm_opp_find_opp_exact(dev, OPP_KEY_LEVEL, level, available);
> #define dev_pm_opp_find_bw_exact(dev, bw, available)            dev_pm_opp_find_opp_exact(dev, OPP_KEY_BW, bw, available);

Ok, but you want this done only for "exact" or for all the other
helpers too? Also, this means that I'll have to implement a
_opp_compare_key2() or whatever because the generic one that
automatically picks the key is still needed for the generic code. Is
that fine by you?

Also, ack to your other response.

-Saravana