diff mbox series

[v1,2/5] clk: Introduce clk_round_rate_unboundly()

Message ID 20200330231617.17079-3-digetx@gmail.com
State New
Headers show
Series NVIDIA Tegra devfreq drivers improvements | expand

Commit Message

Dmitry Osipenko March 30, 2020, 11:16 p.m. UTC
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(-)

Comments

Michał Mirosław April 2, 2020, 12:33 a.m. UTC | #1
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
Dmitry Osipenko April 2, 2020, 2:21 p.m. UTC | #2
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.
Stephen Boyd May 27, 2020, 5:55 a.m. UTC | #3
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?
Stephen Boyd May 27, 2020, 5:57 a.m. UTC | #4
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.
Dmitry Osipenko May 27, 2020, 5:57 p.m. UTC | #5
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 :)
Stephen Boyd May 28, 2020, 12:38 a.m. UTC | #6
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 mbox series

Patch

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;