Re: [PATCH v4 2/3] OPP: Add support for bandwidth OPP tables

[Saravana Kannan]

On Wed, Aug 7, 2019 at 5:53 AM Georgi Djakov <georgi.djakov@xxxxxxxxxx> wrote:
>
> Hi Saravana,
>
> On 7/27/19 02:15, Saravana Kannan wrote:
> > Not all devices quantify their performance points in terms of frequency.
> > Devices like interconnects quantify their performance points in terms of
> > bandwidth. We need a way to represent these bandwidth levels in OPP. So,
> > add support for parsing bandwidth OPPs from DT.
> >
> > Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> > ---
> >  drivers/opp/of.c  | 41 ++++++++++++++++++++++++++++++++---------
> >  drivers/opp/opp.h |  4 +++-
> >  2 files changed, 35 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index b313aca9894f..ac73512f4416 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -523,6 +523,35 @@ void dev_pm_opp_of_remove_table(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
> >
> > +static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np)
> > +{
> > +     int ret;
> > +     u64 rate;
> > +     u32 bw;
> > +
> > +     ret = of_property_read_u64(np, "opp-hz", &rate);
> > +     if (!ret) {
> > +             /*
> > +              * Rate is defined as an unsigned long in clk API, and so
> > +              * casting explicitly to its type. Must be fixed once rate is 64
> > +              * bit guaranteed in clk API.
> > +              */
> > +             new_opp->rate = (unsigned long)rate;
> > +             return 0;
>
> So we can't have a single OPP table with both frequency and bandwidth?

Right, because we can have only 1 "key" for the OPP table. Having more
than one "key" for an OPP table makes a lot of things pretty messy.
Most of the helper functions need to be rewritten to say which key is
being referred to when searching. A lot of the error checking when
creating OPP tables becomes convoluted -- can we allow more than one
OPP entry with the same frequency just because the opp-peak-kBps is
different? Etc. Seems like a lot of code change for something that I
don't think is a very useful.

Also, an OPP table is either going to represent performance levels of
a clock domain (opp-hz) or the performance levels of an interconnect
path (opp-peak-kBps) or an OPP table for genpd. Mixing them all up is
just going to make it convoluted with not enough benefit or use case
that can't be handled as is (separate BW and freq OPP tables).

> > +     }
> > +
> > +     ret = of_property_read_u32(np, "opp-peak-KBps", &bw);
> > +     if (ret)
> > +             return ret;
> > +     new_opp->rate = (unsigned long) bw;
> > +
> > +     ret = of_property_read_u32(np, "opp-avg-KBps", &bw);
> > +     if (!ret)
> > +             new_opp->avg_bw = (unsigned long) bw;
> > +
> > +     return 0;
> > +}
> > +
> >  /**
> >   * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> >   * @opp_table:       OPP table
> > @@ -560,22 +589,16 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> >       if (!new_opp)
> >               return ERR_PTR(-ENOMEM);
> >
> > -     ret = of_property_read_u64(np, "opp-hz", &rate);
> > +     ret = _read_opp_key(new_opp, np);
> >       if (ret < 0) {
> >               /* "opp-hz" is optional for devices like power domains. */
> >               if (!opp_table->is_genpd) {
> > -                     dev_err(dev, "%s: opp-hz not found\n", __func__);
> > +                     dev_err(dev, "%s: opp-hz or opp-peak-bw not found\n",
>
> s/opp-peak-bw/opp-peak-kBps/

Thanks. Will fix.

-Saravana