Re: [PATCH v5 2/3] OPP: Add support for bandwidth OPP tables
On 20-08-19, 15:36, Saravana Kannan wrote: > On Tue, Aug 20, 2019 at 3:27 PM Saravana Kannan wrote: > > > > On Mon, Aug 19, 2019 at 11:13 PM Viresh Kumar > > wrote: > > > > > > On 07-08-19, 15:31, Saravana Kannan wrote: > > > > > + ret = of_property_read_u32(np, "opp-peak-kBps", ); > > > > + if (ret) > > > > + return ret; > > > > + new_opp->rate = (unsigned long) bw; > > > > + > > > > + ret = of_property_read_u32(np, "opp-avg-kBps", ); > > > > + if (!ret) > > > > + new_opp->avg_bw = (unsigned long) bw; Why is this casting required ? If you really want a 64 bit value for bw, then make it 64 bit in bindings as well, like opp-hz. And then you can simply do: of_property_read_u32(np, "opp-avg-kBps", _opp->avg_bw); > > > > > > If none of opp-hz/level/peak-kBps are available, print error message here > > > itself.. > > > > But you don't print any error for opp-level today. Seems like it's optional? > > > > > > > > > + > > > > + return 0; > > > > > > You are returning 0 on failure as well here. > > > > Thanks. > > Wait, no. This is not actually a failure. opp-avg-kBps is optional. So > returning 0 is the right thing to do. If the mandatory properties > aren't present an error is returned before you get to th end. > > -Saravana -- viresh
Re: [PATCH v5 2/3] OPP: Add support for bandwidth OPP tables
On 20-08-19, 15:36, Saravana Kannan wrote: > On Tue, Aug 20, 2019 at 3:27 PM Saravana Kannan wrote: > > > > On Mon, Aug 19, 2019 at 11:13 PM Viresh Kumar > > wrote: > > > > > > On 07-08-19, 15:31, Saravana Kannan wrote: > > > > > + ret = of_property_read_u32(np, "opp-peak-kBps", ); > > > > + if (ret) > > > > + return ret; > > > > + new_opp->rate = (unsigned long) bw; > > > > + > > > > + ret = of_property_read_u32(np, "opp-avg-kBps", ); > > > > + if (!ret) > > > > + new_opp->avg_bw = (unsigned long) bw; > > > > > > If none of opp-hz/level/peak-kBps are available, print error message here > > > itself.. > > > > But you don't print any error for opp-level today. Seems like it's optional? > > > > > > > > > + > > > > + return 0; > > > > > > You are returning 0 on failure as well here. > > > > Thanks. > > Wait, no. This is not actually a failure. opp-avg-kBps is optional. So > returning 0 is the right thing to do. If the mandatory properties > aren't present an error is returned before you get to th end. You are right :) -- viresh
Re: [PATCH v5 2/3] OPP: Add support for bandwidth OPP tables
On 20-08-19, 15:27, Saravana Kannan wrote:
> On Mon, Aug 19, 2019 at 11:13 PM Viresh Kumar wrote:
> >
> > On 07-08-19, 15:31, 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
> > > ---
> > > 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 1813f5ad5fa2..e1750033fef9 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", );
> > > + 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;
> > > + }
> > > +
> >
> > Please read opp-level also here and do error handling.
>
> Can you please explain what's the reasoning? opp-level doesn't seem to
> be a "key" based on looking at the code.
Because opp-level is the thing that distinguishes OPPs for power domains, those
nodes don't have opp-hz or bw.
> > > + ret = of_property_read_u32(np, "opp-peak-kBps", );
> > > + if (ret)
> > > + return ret;
> > > + new_opp->rate = (unsigned long) bw;
> > > +
> > > + ret = of_property_read_u32(np, "opp-avg-kBps", );
> > > + if (!ret)
> > > + new_opp->avg_bw = (unsigned long) bw;
> >
> > If none of opp-hz/level/peak-kBps are available, print error message here
> > itself..
>
> But you don't print any error for opp-level today. Seems like it's optional?
Yeah, probably it should have been there. It will be better to do it now as we
are creating a separate routine for that.
> >
> > > +
> > > + return 0;
> >
> > You are returning 0 on failure as well here.
>
> Thanks.
>
> > > +}
> > > +
> > > /**
> > > * _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", );
> > > + 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-kBps not
> > > found\n",
> > > + __func__);
> > > goto free_opp;
> > > }
> > >
> > > rate_not_available = true;
> >
> > Move all above as well to read_opp_key().
>
> Ok. I didn't want to print an error at the API level and instead print
> at the caller level. But if that's what you want, that's fine by me.
That would be fine, you can keep the print message here (but a generic one, like
key missing).
> > > - } else {
> > > - /*
> > > - * 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;
> > > }
> > >
> > > of_property_read_u32(np, "opp-level", _opp->level);
> > > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> > > index 01a500e2c40a..6bb238af9cac 100644
> > > --- a/drivers/opp/opp.h
> > > +++ b/drivers/opp/opp.h
> > > @@ -56,7 +56,8 @@ extern struct list_head opp_tables;
> > > * @turbo: true if turbo (boost) OPP
> > > * @suspend: true if suspend OPP
> > > * @pstate: Device's power domain's performance state.
> > > - * @rate:Frequency in hertz
> > > + * @rate:Frequency in hertz OR Peak bandwidth in kilobytes per second
> > > + * @avg_bw: Average bandwidth in kilobytes per second
> >
> > Please add separate entry for peak_bw here.
> >
> > I know you reused rate because you don't want to reimplement the helpers we
> > have. Maybe we can just update them to return
Re: [PATCH v5 2/3] OPP: Add support for bandwidth OPP tables
On Tue, Aug 20, 2019 at 3:27 PM Saravana Kannan wrote: > > On Mon, Aug 19, 2019 at 11:13 PM Viresh Kumar wrote: > > > > On 07-08-19, 15:31, Saravana Kannan wrote: > > > + ret = of_property_read_u32(np, "opp-peak-kBps", ); > > > + if (ret) > > > + return ret; > > > + new_opp->rate = (unsigned long) bw; > > > + > > > + ret = of_property_read_u32(np, "opp-avg-kBps", ); > > > + if (!ret) > > > + new_opp->avg_bw = (unsigned long) bw; > > > > If none of opp-hz/level/peak-kBps are available, print error message here > > itself.. > > But you don't print any error for opp-level today. Seems like it's optional? > > > > > > + > > > + return 0; > > > > You are returning 0 on failure as well here. > > Thanks. Wait, no. This is not actually a failure. opp-avg-kBps is optional. So returning 0 is the right thing to do. If the mandatory properties aren't present an error is returned before you get to th end. -Saravana
Re: [PATCH v5 2/3] OPP: Add support for bandwidth OPP tables
On Fri, Aug 16, 2019 at 11:21 AM Stephen Boyd wrote:
>
> Quoting Saravana Kannan (2019-08-07 15:31:10)
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index 1813f5ad5fa2..e1750033fef9 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", );
> > + 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;
> > + }
> > +
> > + ret = of_property_read_u32(np, "opp-peak-kBps", );
> > + if (ret)
> > + return ret;
> > + new_opp->rate = (unsigned long) bw;
> > +
> > + ret = of_property_read_u32(np, "opp-avg-kBps", );
> > + if (!ret)
>
> I would write this as
>
> if (!of_property_read_u32(np, "opp-avg-kBps", ))
> new_opp->avg_bw = (unsigned long) bw;
>
> because you don't care about the return value.
Sure, will do.
>
> > + 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
> > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> > index 01a500e2c40a..6bb238af9cac 100644
> > --- a/drivers/opp/opp.h
> > +++ b/drivers/opp/opp.h
> > @@ -56,7 +56,8 @@ extern struct list_head opp_tables;
> > * @turbo: true if turbo (boost) OPP
> > * @suspend: true if suspend OPP
> > * @pstate: Device's power domain's performance state.
> > - * @rate: Frequency in hertz
> > + * @rate: Frequency in hertz OR Peak bandwidth in kilobytes per second
>
> Why is Peak capitalized?
Because it's another Key? :)
Just kidding. I'll fix it.
> > + * @avg_bw:Average bandwidth in kilobytes per second
> > * @level: Performance level
> > * @supplies: Power supplies voltage/current values
> > * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
> > @@ -78,6 +79,7 @@ struct dev_pm_opp {
> > bool suspend;
> > unsigned int pstate;
> > unsigned long rate;
>
> If you're trying to save space why not make an anonymous union here of
> 'rate' and 'bandwidth'? Then the code doesn't read all weird.
It's not about saving space. It's about having to rewrite all the
helper functions (see subsequent patch in this series) for different
"keys" with zero difference in the actual comparisons/logic. I'm
proposing I rename this to "key" in another email.
-Saravana
Re: [PATCH v5 2/3] OPP: Add support for bandwidth OPP tables
On Mon, Aug 19, 2019 at 11:13 PM Viresh Kumar wrote:
>
> On 07-08-19, 15:31, 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
> > ---
> > 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 1813f5ad5fa2..e1750033fef9 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", );
> > + 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;
> > + }
> > +
>
> Please read opp-level also here and do error handling.
Can you please explain what's the reasoning? opp-level doesn't seem to
be a "key" based on looking at the code.
>
> > + ret = of_property_read_u32(np, "opp-peak-kBps", );
> > + if (ret)
> > + return ret;
> > + new_opp->rate = (unsigned long) bw;
> > +
> > + ret = of_property_read_u32(np, "opp-avg-kBps", );
> > + if (!ret)
> > + new_opp->avg_bw = (unsigned long) bw;
>
> If none of opp-hz/level/peak-kBps are available, print error message here
> itself..
But you don't print any error for opp-level today. Seems like it's optional?
>
> > +
> > + return 0;
>
> You are returning 0 on failure as well here.
Thanks.
> > +}
> > +
> > /**
> > * _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", );
> > + 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-kBps not
> > found\n",
> > + __func__);
> > goto free_opp;
> > }
> >
> > rate_not_available = true;
>
> Move all above as well to read_opp_key().
Ok. I didn't want to print an error at the API level and instead print
at the caller level. But if that's what you want, that's fine by me.
>
> > - } else {
> > - /*
> > - * 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;
> > }
> >
> > of_property_read_u32(np, "opp-level", _opp->level);
> > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> > index 01a500e2c40a..6bb238af9cac 100644
> > --- a/drivers/opp/opp.h
> > +++ b/drivers/opp/opp.h
> > @@ -56,7 +56,8 @@ extern struct list_head opp_tables;
> > * @turbo: true if turbo (boost) OPP
> > * @suspend: true if suspend OPP
> > * @pstate: Device's power domain's performance state.
> > - * @rate:Frequency in hertz
> > + * @rate:Frequency in hertz OR Peak bandwidth in kilobytes per second
> > + * @avg_bw: Average bandwidth in kilobytes per second
>
> Please add separate entry for peak_bw here.
>
> I know you reused rate because you don't want to reimplement the helpers we
> have. Maybe we can just update them to return peak_bw when opp-hz isn't
> present.
How about I just rename this to "key"? That makes a lot more sense
than trying to save 3 different keys and going through them one at a
time.
> > * @level: Performance level
> > * @supplies:Power supplies voltage/current values
> > * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
> > @@ -78,6 +79,7 @@ struct dev_pm_opp {
> > bool suspend;
> > unsigned int pstate;
> > unsigned long rate;
> > + unsigned long avg_bw;
> > unsigned int level;
> >
> > struct dev_pm_opp_supply *supplies;
> > --
> >
Re: [PATCH v5 2/3] OPP: Add support for bandwidth OPP tables
On 07-08-19, 15:31, 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
> ---
> 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 1813f5ad5fa2..e1750033fef9 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", );
> + 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;
> + }
> +
Please read opp-level also here and do error handling.
> + ret = of_property_read_u32(np, "opp-peak-kBps", );
> + if (ret)
> + return ret;
> + new_opp->rate = (unsigned long) bw;
> +
> + ret = of_property_read_u32(np, "opp-avg-kBps", );
> + if (!ret)
> + new_opp->avg_bw = (unsigned long) bw;
If none of opp-hz/level/peak-kBps are available, print error message here
itself..
> +
> + return 0;
You are returning 0 on failure as well here.
> +}
> +
> /**
> * _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", );
> + 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-kBps not found\n",
> + __func__);
> goto free_opp;
> }
>
> rate_not_available = true;
Move all above as well to read_opp_key().
> - } else {
> - /*
> - * 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;
> }
>
> of_property_read_u32(np, "opp-level", _opp->level);
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 01a500e2c40a..6bb238af9cac 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -56,7 +56,8 @@ extern struct list_head opp_tables;
> * @turbo: true if turbo (boost) OPP
> * @suspend: true if suspend OPP
> * @pstate: Device's power domain's performance state.
> - * @rate:Frequency in hertz
> + * @rate:Frequency in hertz OR Peak bandwidth in kilobytes per second
> + * @avg_bw: Average bandwidth in kilobytes per second
Please add separate entry for peak_bw here.
I know you reused rate because you don't want to reimplement the helpers we
have. Maybe we can just update them to return peak_bw when opp-hz isn't present.
> * @level: Performance level
> * @supplies:Power supplies voltage/current values
> * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
> @@ -78,6 +79,7 @@ struct dev_pm_opp {
> bool suspend;
> unsigned int pstate;
> unsigned long rate;
> + unsigned long avg_bw;
> unsigned int level;
>
> struct dev_pm_opp_supply *supplies;
> --
> 2.23.0.rc1.153.gdeed80330f-goog
--
viresh
Re: [PATCH v5 2/3] OPP: Add support for bandwidth OPP tables
Quoting Saravana Kannan (2019-08-07 15:31:10)
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 1813f5ad5fa2..e1750033fef9 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", );
> + 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;
> + }
> +
> + ret = of_property_read_u32(np, "opp-peak-kBps", );
> + if (ret)
> + return ret;
> + new_opp->rate = (unsigned long) bw;
> +
> + ret = of_property_read_u32(np, "opp-avg-kBps", );
> + if (!ret)
I would write this as
if (!of_property_read_u32(np, "opp-avg-kBps", ))
new_opp->avg_bw = (unsigned long) bw;
because you don't care about the return value.
> + 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
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 01a500e2c40a..6bb238af9cac 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -56,7 +56,8 @@ extern struct list_head opp_tables;
> * @turbo: true if turbo (boost) OPP
> * @suspend: true if suspend OPP
> * @pstate: Device's power domain's performance state.
> - * @rate: Frequency in hertz
> + * @rate: Frequency in hertz OR Peak bandwidth in kilobytes per second
Why is Peak capitalized?
> + * @avg_bw:Average bandwidth in kilobytes per second
> * @level: Performance level
> * @supplies: Power supplies voltage/current values
> * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
> @@ -78,6 +79,7 @@ struct dev_pm_opp {
> bool suspend;
> unsigned int pstate;
> unsigned long rate;
If you're trying to save space why not make an anonymous union here of
'rate' and 'bandwidth'? Then the code doesn't read all weird.
> + unsigned long avg_bw;
> unsigned int level;
>
> struct dev_pm_opp_supply *supplies;
[PATCH v5 2/3] OPP: Add support for bandwidth OPP tables
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
---
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 1813f5ad5fa2..e1750033fef9 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", );
+ 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;
+ }
+
+ ret = of_property_read_u32(np, "opp-peak-kBps", );
+ if (ret)
+ return ret;
+ new_opp->rate = (unsigned long) bw;
+
+ ret = of_property_read_u32(np, "opp-avg-kBps", );
+ 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", );
+ 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-kBps not found\n",
+ __func__);
goto free_opp;
}
rate_not_available = true;
- } else {
- /*
-* 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;
}
of_property_read_u32(np, "opp-level", _opp->level);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 01a500e2c40a..6bb238af9cac 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -56,7 +56,8 @@ extern struct list_head opp_tables;
* @turbo: true if turbo (boost) OPP
* @suspend: true if suspend OPP
* @pstate: Device's power domain's performance state.
- * @rate: Frequency in hertz
+ * @rate: Frequency in hertz OR Peak bandwidth in kilobytes per second
+ * @avg_bw:Average bandwidth in kilobytes per second
* @level: Performance level
* @supplies: Power supplies voltage/current values
* @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
@@ -78,6 +79,7 @@ struct dev_pm_opp {
bool suspend;
unsigned int pstate;
unsigned long rate;
+ unsigned long avg_bw;
unsigned int level;
struct dev_pm_opp_supply *supplies;
--
2.23.0.rc1.153.gdeed80330f-goog
