| Message ID | 20200330231617.17079-3-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | NVIDIA Tegra devfreq drivers improvements | expand |
On Tue, Mar 31, 2020 at 02:16:14AM +0300, Dmitry Osipenko wrote: [...] > +/** > + * clk_round_rate_unboundly - unboundly round the given rate for a clk Just grammar nits: clk_round_rate_unbounded - round the given rate for a clk, ignoring users' min/max constraints > + * @clk: the clk for which we are rounding a rate > + * @rate: the rate which is to be rounded > + * > + * Takes in a rate as input and rounds it to a rate that the clk can use > + * which is then returned. The given rate isn't bounded by clk users min-max s/bounded/limited/ ? > + * rates, unlike in a case of clk_round_rate(). If clk doesn't support "... unlike in clk_round_rate()." > + * round_rate operation then the parent rate is returned. > + */ [...] Best Regards, Michał Mirosław
02.04.2020 03:33, Michał Mirosław пишет: > On Tue, Mar 31, 2020 at 02:16:14AM +0300, Dmitry Osipenko wrote: > [...] >> +/** >> + * clk_round_rate_unboundly - unboundly round the given rate for a clk > > Just grammar nits: > > clk_round_rate_unbounded - round the given rate for a clk, ignoring users' min/max constraints > >> + * @clk: the clk for which we are rounding a rate >> + * @rate: the rate which is to be rounded >> + * >> + * Takes in a rate as input and rounds it to a rate that the clk can use >> + * which is then returned. The given rate isn't bounded by clk users min-max > > s/bounded/limited/ ? > >> + * rates, unlike in a case of clk_round_rate(). If clk doesn't support > > "... unlike in clk_round_rate()." > >> + * round_rate operation then the parent rate is returned. >> + */ > [...] Hello Michał, Thank you very much for the suggestion, I'll take it into account in the next version.
Quoting Dmitry Osipenko (2020-03-30 16:16:14) > In same cases it may be desired to round clock's rate without taking into > account current min/max requests made by the clock's users. One example is > building up OPP table based on a possible clock rates. Shouldn't the OPP table come from firmware/DT? I don't quite understand why we're generating OPP tables on top of the rate rounding API. clk_round_rate() is supposed to tell us what rate we'll get if we call clk_set_rate() with the same arguments. An unboundly version of that doesn't make sense. I wonder if perhaps the clk provider should be populating OPP tables in this case? Or basically anything besides adding another clk consumer API to solve this problem. Who is the caller? Something later in this series?
Quoting Dmitry Osipenko (2020-03-30 16:16:14) > In same cases it may be desired to round clock's rate without taking into > account current min/max requests made by the clock's users. One example is > building up OPP table based on a possible clock rates. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/clk/clk.c | 49 +++++++++++++++++++++++++++++++++++---------- > include/linux/clk.h | 18 +++++++++++++++++ BTW, please run get_maintainers.pl patches. Russell maintains include/linux/clk.h (the clk API). CCF implements it.
27.05.2020 08:55, Stephen Boyd пишет: > Quoting Dmitry Osipenko (2020-03-30 16:16:14) >> In same cases it may be desired to round clock's rate without taking into >> account current min/max requests made by the clock's users. One example is >> building up OPP table based on a possible clock rates. > > Shouldn't the OPP table come from firmware/DT? I don't quite understand > why we're generating OPP tables on top of the rate rounding API. > clk_round_rate() is supposed to tell us what rate we'll get if we call > clk_set_rate() with the same arguments. An unboundly version of that > doesn't make sense. The OPP should come from the DT, but unfortunately DT and Tegra's devfreq driver wasn't designed like that from the start, so it will take some extra effort to re-do it properly now. I wanted to postpone that effort a tad and get at least the basics upstreamed for the starter. > I wonder if perhaps the clk provider should be populating OPP tables in > this case? Or basically anything besides adding another clk consumer API > to solve this problem. Who is the caller? Something later in this > series? I'll try to add a proper OPP table with freqs and voltages, will see how it goes. We will need to do it sooner or later anyways. So perhaps it's fine to drop the current approach with the clk_round_rate_unboundly() and re-focus on a proper OPP implementation. Thank you for getting back and replying to this topic :)
Quoting Dmitry Osipenko (2020-05-27 10:57:01) > 27.05.2020 08:55, Stephen Boyd \u043f\u0438\u0448\u0435\u0442: > > Quoting Dmitry Osipenko (2020-03-30 16:16:14) > >> In same cases it may be desired to round clock's rate without taking into > >> account current min/max requests made by the clock's users. One example is > >> building up OPP table based on a possible clock rates. > > > > Shouldn't the OPP table come from firmware/DT? I don't quite understand > > why we're generating OPP tables on top of the rate rounding API. > > clk_round_rate() is supposed to tell us what rate we'll get if we call > > clk_set_rate() with the same arguments. An unboundly version of that > > doesn't make sense. > > The OPP should come from the DT, but unfortunately DT and Tegra's > devfreq driver wasn't designed like that from the start, so it will take > some extra effort to re-do it properly now. I wanted to postpone that > effort a tad and get at least the basics upstreamed for the starter. > > > I wonder if perhaps the clk provider should be populating OPP tables in > > this case? Or basically anything besides adding another clk consumer API > > to solve this problem. Who is the caller? Something later in this > > series? > > I'll try to add a proper OPP table with freqs and voltages, will see how > it goes. We will need to do it sooner or later anyways. So perhaps it's > fine to drop the current approach with the clk_round_rate_unboundly() > and re-focus on a proper OPP implementation. > > Thank you for getting back and replying to this topic :) Alright, it sounds better to me if we can avoid a one off addition to the clk API in favor of implementing a proper OPP table from the start.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 39c59f063aa0..28c2e1699619 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1412,16 +1412,7 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate) } EXPORT_SYMBOL_GPL(clk_hw_round_rate); -/** - * clk_round_rate - round the given rate for a clk - * @clk: the clk for which we are rounding a rate - * @rate: the rate which is to be rounded - * - * Takes in a rate as input and rounds it to a rate that the clk can actually - * use which is then returned. If clk doesn't support round_rate operation - * then the parent rate is returned. - */ -long clk_round_rate(struct clk *clk, unsigned long rate) +static long __clk_round_rate(struct clk *clk, unsigned long rate, bool bound) { struct clk_rate_request req; int ret; @@ -1434,7 +1425,13 @@ long clk_round_rate(struct clk *clk, unsigned long rate) if (clk->exclusive_count) clk_core_rate_unprotect(clk->core); - clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate); + if (bound) { + clk_core_get_boundaries(clk->core, &req.min_rate, + &req.max_rate); + } else { + req.min_rate = 0; + req.max_rate = ULONG_MAX; + } req.rate = rate; ret = clk_core_round_rate_nolock(clk->core, &req); @@ -1449,8 +1446,38 @@ long clk_round_rate(struct clk *clk, unsigned long rate) return req.rate; } + +/** + * clk_round_rate - round the given rate for a clk + * @clk: the clk for which we are rounding a rate + * @rate: the rate which is to be rounded + * + * Takes in a rate as input and rounds it to a rate that the clk can actually + * use which is then returned. If clk doesn't support round_rate operation + * then the parent rate is returned. + */ +long clk_round_rate(struct clk *clk, unsigned long rate) +{ + return __clk_round_rate(clk, rate, true); +} EXPORT_SYMBOL_GPL(clk_round_rate); +/** + * clk_round_rate_unboundly - unboundly round the given rate for a clk + * @clk: the clk for which we are rounding a rate + * @rate: the rate which is to be rounded + * + * Takes in a rate as input and rounds it to a rate that the clk can use + * which is then returned. The given rate isn't bounded by clk users min-max + * rates, unlike in a case of clk_round_rate(). If clk doesn't support + * round_rate operation then the parent rate is returned. + */ +long clk_round_rate_unboundly(struct clk *clk, unsigned long rate) +{ + return __clk_round_rate(clk, rate, false); +} +EXPORT_SYMBOL_GPL(clk_round_rate_unboundly); + /** * __clk_notify - call clk notifier chain * @core: clk that is changing rate diff --git a/include/linux/clk.h b/include/linux/clk.h index 7fd6a1febcf4..b534643015f5 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -622,6 +622,19 @@ void devm_clk_put(struct device *dev, struct clk *clk); */ long clk_round_rate(struct clk *clk, unsigned long rate); +/** + * clk_round_rate_unboundly - adjust a rate to the rate a clock can provide + * @clk: clock source + * @rate: desired clock rate in Hz + * + * This helper function rounds the given rate to a value that hardware + * could actually accept, without taking into account current min/max + * requests. Useful for cases like OPP table buildup. + * + * Returns rounded clock rate in Hz, or negative errno. + */ +long clk_round_rate_unboundly(struct clk *clk, unsigned long rate); + /** * clk_set_rate - set the clock rate for a clock source * @clk: clock source @@ -857,6 +870,11 @@ static inline long clk_round_rate(struct clk *clk, unsigned long rate) return 0; } +static inline long clk_round_rate_unboundly(struct clk *clk, unsigned long rate) +{ + return 0; +} + static inline bool clk_has_parent(struct clk *clk, struct clk *parent) { return true;
In same cases it may be desired to round clock's rate without taking into account current min/max requests made by the clock's users. One example is building up OPP table based on a possible clock rates. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/clk/clk.c | 49 +++++++++++++++++++++++++++++++++++---------- include/linux/clk.h | 18 +++++++++++++++++ 2 files changed, 56 insertions(+), 11 deletions(-)