[PATCH] clk: divider: fix rate calculation for fractional rates

[PATCH] clk: divider: fix rate calculation for fractional rates

[Tomi Valkeinen]

clk-divider.c does not calculate the rates consistently at the moment.

As an example, on OMAP3 we have a clock divider with a source clock of
864000000 Hz. With dividers 6, 7 and 8 the theoretical rates are:

6: 144000000
7: 123428571.428571...
8: 108000000

Calling clk_round_rate() with the rate in the first column will give the
rate in the second column:

144000000 -> 144000000
143999999 -> 123428571
123428572 -> 123428571
123428571 -> 108000000

Note how clk_round_rate() returns 123428571 for rates from 123428572 to
143999999, which is mathematically correct, but when clk_round_rate() is
called with 123428571, the returned value is surprisingly 108000000.

This means that the following code works a bit oddly:

rate = clk_round_rate(clk, 123428572);
clk_set_rate(clk, rate);

As clk_set_rate() also does clock rate rounding, the result is that the
clock is set to the rate of 108000000, not 123428571 returned by the
clk_round_rate.

This patch changes the clk-divider.c to use DIV_ROUND_UP when
calculating the rate. This gives the following behavior which fixes the
inconsistency:

144000000 -> 144000000
143999999 -> 123428572
123428572 -> 123428572
123428571 -> 108000000

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Mike Turquette <mturquette@linaro.org>
---
 drivers/clk/clk-divider.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 8d3009e..43348f3 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -24,7 +24,7 @@
  * Traits of this clock:
  * prepare - clk_prepare only ensures that parents are prepared
  * enable - clk_enable only ensures that parents are enabled
- * rate - rate is adjustable.  clk->rate = parent->rate / divisor
+ * rate - rate is adjustable.  clk->rate = DIV_ROUND_UP(parent->rate / divisor)
  * parent - fixed parent.  No clk_set_parent support
  */
 
@@ -115,7 +115,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
 		return parent_rate;
 	}
 
-	return parent_rate / div;
+	return DIV_ROUND_UP(parent_rate, div);
 }
 
 /*
@@ -185,7 +185,7 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
 		}
 		parent_rate = __clk_round_rate(__clk_get_parent(hw->clk),
 				MULT_ROUND_UP(rate, i));
-		now = parent_rate / i;
+		now = DIV_ROUND_UP(parent_rate, i);
 		if (now <= rate && now > best) {
 			bestdiv = i;
 			best = now;
@@ -207,7 +207,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 	int div;
 	div = clk_divider_bestdiv(hw, rate, prate);
 
-	return *prate / div;
+	return DIV_ROUND_UP(*prate, div);
 }
 
 static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
@@ -218,7 +218,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long flags = 0;
 	u32 val;
 
-	div = parent_rate / rate;
+	div = DIV_ROUND_UP(parent_rate, rate);
 	value = _get_val(divider, div);
 
 	if (value > div_mask(divider))
-- 
1.8.3.2

[PATCH] clk: divider: fix rate calculation for fractional rates

[Tomi Valkeinen]

clk-divider.c does not calculate the rates consistently at the moment.

As an example, on OMAP3 we have a clock divider with a source clock of
864000000 Hz. With dividers 6, 7 and 8 the theoretical rates are:

6: 144000000
7: 123428571.428571...
8: 108000000

Calling clk_round_rate() with the rate in the first column will give the
rate in the second column:

144000000 -> 144000000
143999999 -> 123428571
123428572 -> 123428571
123428571 -> 108000000

Note how clk_round_rate() returns 123428571 for rates from 123428572 to
143999999, which is mathematically correct, but when clk_round_rate() is
called with 123428571, the returned value is surprisingly 108000000.

This means that the following code works a bit oddly:

rate = clk_round_rate(clk, 123428572);
clk_set_rate(clk, rate);

As clk_set_rate() also does clock rate rounding, the result is that the
clock is set to the rate of 108000000, not 123428571 returned by the
clk_round_rate.

This patch changes the clk-divider.c to use DIV_ROUND_UP when
calculating the rate. This gives the following behavior which fixes the
inconsistency:

144000000 -> 144000000
143999999 -> 123428572
123428572 -> 123428572
123428571 -> 108000000

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Mike Turquette <mturquette@linaro.org>
---
 drivers/clk/clk-divider.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 8d3009e..43348f3 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -24,7 +24,7 @@
  * Traits of this clock:
  * prepare - clk_prepare only ensures that parents are prepared
  * enable - clk_enable only ensures that parents are enabled
- * rate - rate is adjustable.  clk->rate = parent->rate / divisor
+ * rate - rate is adjustable.  clk->rate = DIV_ROUND_UP(parent->rate / divisor)
  * parent - fixed parent.  No clk_set_parent support
  */
 
@@ -115,7 +115,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
 		return parent_rate;
 	}
 
-	return parent_rate / div;
+	return DIV_ROUND_UP(parent_rate, div);
 }
 
 /*
@@ -185,7 +185,7 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
 		}
 		parent_rate = __clk_round_rate(__clk_get_parent(hw->clk),
 				MULT_ROUND_UP(rate, i));
-		now = parent_rate / i;
+		now = DIV_ROUND_UP(parent_rate, i);
 		if (now <= rate && now > best) {
 			bestdiv = i;
 			best = now;
@@ -207,7 +207,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 	int div;
 	div = clk_divider_bestdiv(hw, rate, prate);
 
-	return *prate / div;
+	return DIV_ROUND_UP(*prate, div);
 }
 
 static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
@@ -218,7 +218,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long flags = 0;
 	u32 val;
 
-	div = parent_rate / rate;
+	div = DIV_ROUND_UP(parent_rate, rate);
 	value = _get_val(divider, div);
 
 	if (value > div_mask(divider))
-- 
1.8.3.2

Re: [PATCH] clk: divider: fix rate calculation for fractional rates

[Russell King - ARM Linux]

On Wed, Nov 06, 2013 at 01:06:48PM +0200, Tomi Valkeinen wrote:
> This means that the following code works a bit oddly:
> 
> rate = clk_round_rate(clk, 123428572);
> clk_set_rate(clk, rate);

You're right, but the above sequence is quite a crass thing to do.  Why?

clk_round_rate() returns the clock rate that clk_set_rate() would give
you if you were to use this sequence:

	clk_rate_rate(clk, 123428572);
	rate = clk_get_rate(clk);

The difference is that it doesn't change the actual clock rate itself;
clk_round_rate() is meant to answer the question:

	"If I were to set _this_ rate, what clock rate would
	 the clock give me?"

thereby providing a method for drivers to inquire what the effect would
be when changing such a clock without actually affecting it.

So please, don't use clk_round_rate() followed by clk_set_rate().

[PATCH] clk: divider: fix rate calculation for fractional rates

[Russell King - ARM Linux]

On Wed, Nov 06, 2013 at 01:06:48PM +0200, Tomi Valkeinen wrote:
> This means that the following code works a bit oddly:
> 
> rate = clk_round_rate(clk, 123428572);
> clk_set_rate(clk, rate);

You're right, but the above sequence is quite a crass thing to do.  Why?

clk_round_rate() returns the clock rate that clk_set_rate() would give
you if you were to use this sequence:

	clk_rate_rate(clk, 123428572);
	rate = clk_get_rate(clk);

The difference is that it doesn't change the actual clock rate itself;
clk_round_rate() is meant to answer the question:

	"If I were to set _this_ rate, what clock rate would
	 the clock give me?"

thereby providing a method for drivers to inquire what the effect would
be when changing such a clock without actually affecting it.

So please, don't use clk_round_rate() followed by clk_set_rate().

Re: [PATCH] clk: divider: fix rate calculation for fractional rates

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1899 bytes --]

On 2013-11-06 13:15, Russell King - ARM Linux wrote:
> On Wed, Nov 06, 2013 at 01:06:48PM +0200, Tomi Valkeinen wrote:
>> This means that the following code works a bit oddly:
>>
>> rate = clk_round_rate(clk, 123428572);
>> clk_set_rate(clk, rate);
> 
> You're right, but the above sequence is quite a crass thing to do.  Why?

Do you mean that you think the fix is right, but the above example
sequence is silly, or that the fix is not needed either?

> clk_round_rate() returns the clock rate that clk_set_rate() would give
> you if you were to use this sequence:
> 
> 	clk_rate_rate(clk, 123428572);
> 	rate = clk_get_rate(clk);
> 
> The difference is that it doesn't change the actual clock rate itself;
> clk_round_rate() is meant to answer the question:
> 
> 	"If I were to set _this_ rate, what clock rate would
> 	 the clock give me?"
> 
> thereby providing a method for drivers to inquire what the effect would
> be when changing such a clock without actually affecting it.
> 
> So please, don't use clk_round_rate() followed by clk_set_rate().

Ok, if defined like that, then the current behavior is logical.

The comment in clk.h says "adjust a rate to the exact rate a clock can
provide", which does not contradict with what you said, but doesn't
really confirm it either. If I get "the exact rate a clock can provide"
I don't see why I can't use that exact clock rate for clk_set_rate.
Maybe the comment should be improved to state explicitly what it does.

However, how about the following sequence:

	clk_set_rate(clk, 123428572);
	rate = clk_get_rate(clk);
	clk_set_rate(clk, rate);

I didn't test that but it should result in the clock first set to
123428571, and then to 108000000. Obviously pointless if done exactly
like that, but I don't see why the above code sequence is wrong, yet it
gives a bit surprising result.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[PATCH] clk: divider: fix rate calculation for fractional rates

[Tomi Valkeinen]

On 2013-11-06 13:15, Russell King - ARM Linux wrote:
> On Wed, Nov 06, 2013 at 01:06:48PM +0200, Tomi Valkeinen wrote:
>> This means that the following code works a bit oddly:
>>
>> rate = clk_round_rate(clk, 123428572);
>> clk_set_rate(clk, rate);
> 
> You're right, but the above sequence is quite a crass thing to do.  Why?

Do you mean that you think the fix is right, but the above example
sequence is silly, or that the fix is not needed either?

> clk_round_rate() returns the clock rate that clk_set_rate() would give
> you if you were to use this sequence:
> 
> 	clk_rate_rate(clk, 123428572);
> 	rate = clk_get_rate(clk);
> 
> The difference is that it doesn't change the actual clock rate itself;
> clk_round_rate() is meant to answer the question:
> 
> 	"If I were to set _this_ rate, what clock rate would
> 	 the clock give me?"
> 
> thereby providing a method for drivers to inquire what the effect would
> be when changing such a clock without actually affecting it.
> 
> So please, don't use clk_round_rate() followed by clk_set_rate().

Ok, if defined like that, then the current behavior is logical.

The comment in clk.h says "adjust a rate to the exact rate a clock can
provide", which does not contradict with what you said, but doesn't
really confirm it either. If I get "the exact rate a clock can provide"
I don't see why I can't use that exact clock rate for clk_set_rate.
Maybe the comment should be improved to state explicitly what it does.

However, how about the following sequence:

	clk_set_rate(clk, 123428572);
	rate = clk_get_rate(clk);
	clk_set_rate(clk, rate);

I didn't test that but it should result in the clock first set to
123428571, and then to 108000000. Obviously pointless if done exactly
like that, but I don't see why the above code sequence is wrong, yet it
gives a bit surprising result.

 Tomi


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131106/8ae3124c/attachment.sig>

Re: [PATCH] clk: divider: fix rate calculation for fractional rates

[Russell King - ARM Linux]

On Wed, Nov 06, 2013 at 01:48:44PM +0200, Tomi Valkeinen wrote:
> On 2013-11-06 13:15, Russell King - ARM Linux wrote:
> > On Wed, Nov 06, 2013 at 01:06:48PM +0200, Tomi Valkeinen wrote:
> >> This means that the following code works a bit oddly:
> >>
> >> rate = clk_round_rate(clk, 123428572);
> >> clk_set_rate(clk, rate);
> > 
> > You're right, but the above sequence is quite a crass thing to do.  Why?
> 
> Do you mean that you think the fix is right, but the above example
> sequence is silly, or that the fix is not needed either?

I think a fix _is) required, because:

	rate = clk_get_rate(clk);
	clk_set_rate(clk, rate);
	assert(clk_get_rate(clk) == rate);

If not, there's something quite wrong.  However, I am saying that the
sequence you provided was nevertheless silly - I've seen it in real code
in the kernel, which is why I've commented about it.

> Ok, if defined like that, then the current behavior is logical.
> 
> The comment in clk.h says "adjust a rate to the exact rate a clock can
> provide", which does not contradict with what you said, but doesn't
> really confirm it either. If I get "the exact rate a clock can provide"
> I don't see why I can't use that exact clock rate for clk_set_rate.
> Maybe the comment should be improved to state explicitly what it does.
> 
> However, how about the following sequence:
> 
> 	clk_set_rate(clk, 123428572);
> 	rate = clk_get_rate(clk);
> 	clk_set_rate(clk, rate);
> 
> I didn't test that but it should result in the clock first set to
> 123428571, and then to 108000000. Obviously pointless if done exactly
> like that, but I don't see why the above code sequence is wrong, yet it
> gives a bit surprising result.

Does this help clarify it?

--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -228,6 +228,20 @@
  * @clk: clock source
  * @rate: desired clock rate in Hz
  *
+ * This answers the question "if I were to pass @rate to clk_set_rate(),
+ * what clock rate would I end up with?" without changing the hardware
+ * in any way.  In other words:
+ *
+ *   rate = clk_round_rate(clk, r);
+ *
+ * and:
+ *
+ *   clk_set_rate(clk, r);
+ *   rate = clk_get_rate(clk);
+ *
+ * are equivalent except the former does not modify the clock hardware
+ * in any way.
+ *
  * Returns rounded clock rate in Hz, or negative errno.
  */
 long clk_round_rate(struct clk *clk, unsigned long rate);

[PATCH] clk: divider: fix rate calculation for fractional rates

[Russell King - ARM Linux]

On Wed, Nov 06, 2013 at 01:48:44PM +0200, Tomi Valkeinen wrote:
> On 2013-11-06 13:15, Russell King - ARM Linux wrote:
> > On Wed, Nov 06, 2013 at 01:06:48PM +0200, Tomi Valkeinen wrote:
> >> This means that the following code works a bit oddly:
> >>
> >> rate = clk_round_rate(clk, 123428572);
> >> clk_set_rate(clk, rate);
> > 
> > You're right, but the above sequence is quite a crass thing to do.  Why?
> 
> Do you mean that you think the fix is right, but the above example
> sequence is silly, or that the fix is not needed either?

I think a fix _is) required, because:

	rate = clk_get_rate(clk);
	clk_set_rate(clk, rate);
	assert(clk_get_rate(clk) == rate);

If not, there's something quite wrong.  However, I am saying that the
sequence you provided was nevertheless silly - I've seen it in real code
in the kernel, which is why I've commented about it.

> Ok, if defined like that, then the current behavior is logical.
> 
> The comment in clk.h says "adjust a rate to the exact rate a clock can
> provide", which does not contradict with what you said, but doesn't
> really confirm it either. If I get "the exact rate a clock can provide"
> I don't see why I can't use that exact clock rate for clk_set_rate.
> Maybe the comment should be improved to state explicitly what it does.
> 
> However, how about the following sequence:
> 
> 	clk_set_rate(clk, 123428572);
> 	rate = clk_get_rate(clk);
> 	clk_set_rate(clk, rate);
> 
> I didn't test that but it should result in the clock first set to
> 123428571, and then to 108000000. Obviously pointless if done exactly
> like that, but I don't see why the above code sequence is wrong, yet it
> gives a bit surprising result.

Does this help clarify it?

--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -228,6 +228,20 @@
  * @clk: clock source
  * @rate: desired clock rate in Hz
  *
+ * This answers the question "if I were to pass @rate to clk_set_rate(),
+ * what clock rate would I end up with?" without changing the hardware
+ * in any way.  In other words:
+ *
+ *   rate = clk_round_rate(clk, r);
+ *
+ * and:
+ *
+ *   clk_set_rate(clk, r);
+ *   rate = clk_get_rate(clk);
+ *
+ * are equivalent except the former does not modify the clock hardware
+ * in any way.
+ *
  * Returns rounded clock rate in Hz, or negative errno.
  */
 long clk_round_rate(struct clk *clk, unsigned long rate);

Re: [PATCH] clk: divider: fix rate calculation for fractional rates

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]

Hi Mike, Russell,

On 2013-11-06 18:19, Russell King - ARM Linux wrote:
> On Wed, Nov 06, 2013 at 01:48:44PM +0200, Tomi Valkeinen wrote:
>> On 2013-11-06 13:15, Russell King - ARM Linux wrote:
>>> On Wed, Nov 06, 2013 at 01:06:48PM +0200, Tomi Valkeinen wrote:
>>>> This means that the following code works a bit oddly:
>>>>
>>>> rate = clk_round_rate(clk, 123428572);
>>>> clk_set_rate(clk, rate);
>>>
>>> You're right, but the above sequence is quite a crass thing to do.  Why?
>>
>> Do you mean that you think the fix is right, but the above example
>> sequence is silly, or that the fix is not needed either?
> 
> I think a fix _is) required, because:
> 
> 	rate = clk_get_rate(clk);
> 	clk_set_rate(clk, rate);
> 	assert(clk_get_rate(clk) == rate);
> 
> If not, there's something quite wrong.  However, I am saying that the
> sequence you provided was nevertheless silly - I've seen it in real code
> in the kernel, which is why I've commented about it.

I just ran into this issue again with omap3, and so I'm resurrecting the
thread.

Mike, can you review the patch?

Russell, I'd like to understand why you think the original example is bad:

	rate = clk_round_rate(clk, rate);
	clk_set_rate(clk, rate);

If the definition of clk_round_rate is basically "clk_set_rate without
actually setting the rate", I agree that the above code is not good as
it might not work correctly.

However, if  the following code you gave should work:

	rate = clk_get_rate(clk);
	clk_set_rate(clk, rate);
	assert(clk_get_rate(clk) == rate);

then the original example should also always work, as it's almost the
same as:

	/* this is the "round" part */
	clk_set_rate(clk, rate);
	rate = clk_get_rate(clk);

	clk_set_rate(clk, rate);
	assert(clk_get_rate(clk) == rate);

Why I'm asking this is that for me (and probably for others also if
you've seen it used in the kernel code) it feels natural to have code like:

	rate = clk_round_rate(clk, rate);
	
	/* Verify the rounded rate here to see it's ok for the IP etc */

	/* The rate is ok, so set it */
	clk_set_rate(clk, rate);

This could be rewritten as:

	rounded_rate = clk_round_rate(clk, rate);
	
	/* Verify the rounded rate here to see it's ok for the IP etc */

	/* The rounded rate is ok, so set the original rate */
	clk_set_rate(clk, rate);

But it just feels unnecessary complication to keep both the original
rate and the rounded rate around.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[PATCH] clk: divider: fix rate calculation for fractional rates

[Tomi Valkeinen]

Hi Mike, Russell,

On 2013-11-06 18:19, Russell King - ARM Linux wrote:
> On Wed, Nov 06, 2013 at 01:48:44PM +0200, Tomi Valkeinen wrote:
>> On 2013-11-06 13:15, Russell King - ARM Linux wrote:
>>> On Wed, Nov 06, 2013 at 01:06:48PM +0200, Tomi Valkeinen wrote:
>>>> This means that the following code works a bit oddly:
>>>>
>>>> rate = clk_round_rate(clk, 123428572);
>>>> clk_set_rate(clk, rate);
>>>
>>> You're right, but the above sequence is quite a crass thing to do.  Why?
>>
>> Do you mean that you think the fix is right, but the above example
>> sequence is silly, or that the fix is not needed either?
> 
> I think a fix _is) required, because:
> 
> 	rate = clk_get_rate(clk);
> 	clk_set_rate(clk, rate);
> 	assert(clk_get_rate(clk) == rate);
> 
> If not, there's something quite wrong.  However, I am saying that the
> sequence you provided was nevertheless silly - I've seen it in real code
> in the kernel, which is why I've commented about it.

I just ran into this issue again with omap3, and so I'm resurrecting the
thread.

Mike, can you review the patch?

Russell, I'd like to understand why you think the original example is bad:

	rate = clk_round_rate(clk, rate);
	clk_set_rate(clk, rate);

If the definition of clk_round_rate is basically "clk_set_rate without
actually setting the rate", I agree that the above code is not good as
it might not work correctly.

However, if  the following code you gave should work:

	rate = clk_get_rate(clk);
	clk_set_rate(clk, rate);
	assert(clk_get_rate(clk) == rate);

then the original example should also always work, as it's almost the
same as:

	/* this is the "round" part */
	clk_set_rate(clk, rate);
	rate = clk_get_rate(clk);

	clk_set_rate(clk, rate);
	assert(clk_get_rate(clk) == rate);

Why I'm asking this is that for me (and probably for others also if
you've seen it used in the kernel code) it feels natural to have code like:

	rate = clk_round_rate(clk, rate);
	
	/* Verify the rounded rate here to see it's ok for the IP etc */

	/* The rate is ok, so set it */
	clk_set_rate(clk, rate);

This could be rewritten as:

	rounded_rate = clk_round_rate(clk, rate);
	
	/* Verify the rounded rate here to see it's ok for the IP etc */

	/* The rounded rate is ok, so set the original rate */
	clk_set_rate(clk, rate);

But it just feels unnecessary complication to keep both the original
rate and the rounded rate around.

 Tomi


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140128/56c20d63/attachment-0001.sig>

Re: [PATCH] clk: divider: fix rate calculation for fractional rates

[Russell King - ARM Linux]

On Tue, Jan 28, 2014 at 10:45:46AM +0200, Tomi Valkeinen wrote:
> Russell, I'd like to understand why you think the original example is bad:
> 
> 	rate = clk_round_rate(clk, rate);
> 	clk_set_rate(clk, rate);

It's needlessly wasteful.  All the processing for setting the rate is
repeated.

> If the definition of clk_round_rate is basically "clk_set_rate without
> actually setting the rate", I agree that the above code is not good as
> it might not work correctly.
> 
> However, if  the following code you gave should work:
> 
> 	rate = clk_get_rate(clk);
> 	clk_set_rate(clk, rate);
> 	assert(clk_get_rate(clk) == rate);
> 
> then the original example should also always work, as it's almost the
> same as:
> 
> 	/* this is the "round" part */
> 	clk_set_rate(clk, rate);
> 	rate = clk_get_rate(clk);
> 
> 	clk_set_rate(clk, rate);
> 	assert(clk_get_rate(clk) == rate);

Okay, now ask yourself this - would you code the above into your driver
with no processing between the two?  It seems that some people would!

> Why I'm asking this is that for me (and probably for others also if
> you've seen it used in the kernel code) it feels natural to have code like:
> 
> 	rate = clk_round_rate(clk, rate);
> 	
> 	/* Verify the rounded rate here to see it's ok for the IP etc */
> 
> 	/* The rate is ok, so set it */
> 	clk_set_rate(clk, rate);

If you want to do something with the rounded rate, then that's fine,
you have a reason to do it this way.  However, what I was referring to
are drivers which literally do this:

	clk_set_rate(clk, clk_round_rate(clk, rate));

In other words, they think that they must always round the rate before
passing it into clk_set_rate() even though they make no other use of
the rounded rate.  That is completely wasteful and unnecessary.  It
might as well have clk_round_rate() replaced by a udelay() to waste
some CPU cycles just for the hell of it.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH] clk: divider: fix rate calculation for fractional rates

[Russell King - ARM Linux]

On Tue, Jan 28, 2014 at 10:45:46AM +0200, Tomi Valkeinen wrote:
> Russell, I'd like to understand why you think the original example is bad:
> 
> 	rate = clk_round_rate(clk, rate);
> 	clk_set_rate(clk, rate);

It's needlessly wasteful.  All the processing for setting the rate is
repeated.

> If the definition of clk_round_rate is basically "clk_set_rate without
> actually setting the rate", I agree that the above code is not good as
> it might not work correctly.
> 
> However, if  the following code you gave should work:
> 
> 	rate = clk_get_rate(clk);
> 	clk_set_rate(clk, rate);
> 	assert(clk_get_rate(clk) == rate);
> 
> then the original example should also always work, as it's almost the
> same as:
> 
> 	/* this is the "round" part */
> 	clk_set_rate(clk, rate);
> 	rate = clk_get_rate(clk);
> 
> 	clk_set_rate(clk, rate);
> 	assert(clk_get_rate(clk) == rate);

Okay, now ask yourself this - would you code the above into your driver
with no processing between the two?  It seems that some people would!

> Why I'm asking this is that for me (and probably for others also if
> you've seen it used in the kernel code) it feels natural to have code like:
> 
> 	rate = clk_round_rate(clk, rate);
> 	
> 	/* Verify the rounded rate here to see it's ok for the IP etc */
> 
> 	/* The rate is ok, so set it */
> 	clk_set_rate(clk, rate);

If you want to do something with the rounded rate, then that's fine,
you have a reason to do it this way.  However, what I was referring to
are drivers which literally do this:

	clk_set_rate(clk, clk_round_rate(clk, rate));

In other words, they think that they must always round the rate before
passing it into clk_set_rate() even though they make no other use of
the rounded rate.  That is completely wasteful and unnecessary.  It
might as well have clk_round_rate() replaced by a udelay() to waste
some CPU cycles just for the hell of it.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCH] clk: divider: fix rate calculation for fractional rates

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On 2014-01-28 12:32, Russell King - ARM Linux wrote:

>> Why I'm asking this is that for me (and probably for others also if
>> you've seen it used in the kernel code) it feels natural to have code like:
>>
>> 	rate = clk_round_rate(clk, rate);
>> 	
>> 	/* Verify the rounded rate here to see it's ok for the IP etc */
>>
>> 	/* The rate is ok, so set it */
>> 	clk_set_rate(clk, rate);
> 
> If you want to do something with the rounded rate, then that's fine,
> you have a reason to do it this way.  However, what I was referring to
> are drivers which literally do this:
> 
> 	clk_set_rate(clk, clk_round_rate(clk, rate));

Thanks for clarification. Agreed, that's pointless. I gave the sequence
in the patch description just as an example for the sake of discussion
about the bug.

I didn't realize people actually do that in real code =).

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[PATCH] clk: divider: fix rate calculation for fractional rates

[Tomi Valkeinen]

On 2014-01-28 12:32, Russell King - ARM Linux wrote:

>> Why I'm asking this is that for me (and probably for others also if
>> you've seen it used in the kernel code) it feels natural to have code like:
>>
>> 	rate = clk_round_rate(clk, rate);
>> 	
>> 	/* Verify the rounded rate here to see it's ok for the IP etc */
>>
>> 	/* The rate is ok, so set it */
>> 	clk_set_rate(clk, rate);
> 
> If you want to do something with the rounded rate, then that's fine,
> you have a reason to do it this way.  However, what I was referring to
> are drivers which literally do this:
> 
> 	clk_set_rate(clk, clk_round_rate(clk, rate));

Thanks for clarification. Agreed, that's pointless. I gave the sequence
in the patch description just as an example for the sake of discussion
about the bug.

I didn't realize people actually do that in real code =).

 Tomi


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140128/97c9c93c/attachment.sig>

Re: [PATCH] clk: divider: fix rate calculation for fractional rates

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3350 bytes --]

Mike, ping.

 Tomi

On 06/11/13 13:06, Tomi Valkeinen wrote:
> clk-divider.c does not calculate the rates consistently at the moment.
> 
> As an example, on OMAP3 we have a clock divider with a source clock of
> 864000000 Hz. With dividers 6, 7 and 8 the theoretical rates are:
> 
> 6: 144000000
> 7: 123428571.428571...
> 8: 108000000
> 
> Calling clk_round_rate() with the rate in the first column will give the
> rate in the second column:
> 
> 144000000 -> 144000000
> 143999999 -> 123428571
> 123428572 -> 123428571
> 123428571 -> 108000000
> 
> Note how clk_round_rate() returns 123428571 for rates from 123428572 to
> 143999999, which is mathematically correct, but when clk_round_rate() is
> called with 123428571, the returned value is surprisingly 108000000.
> 
> This means that the following code works a bit oddly:
> 
> rate = clk_round_rate(clk, 123428572);
> clk_set_rate(clk, rate);
> 
> As clk_set_rate() also does clock rate rounding, the result is that the
> clock is set to the rate of 108000000, not 123428571 returned by the
> clk_round_rate.
> 
> This patch changes the clk-divider.c to use DIV_ROUND_UP when
> calculating the rate. This gives the following behavior which fixes the
> inconsistency:
> 
> 144000000 -> 144000000
> 143999999 -> 123428572
> 123428572 -> 123428572
> 123428571 -> 108000000
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Mike Turquette <mturquette@linaro.org>
> ---
>  drivers/clk/clk-divider.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index 8d3009e..43348f3 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -24,7 +24,7 @@
>   * Traits of this clock:
>   * prepare - clk_prepare only ensures that parents are prepared
>   * enable - clk_enable only ensures that parents are enabled
> - * rate - rate is adjustable.  clk->rate = parent->rate / divisor
> + * rate - rate is adjustable.  clk->rate = DIV_ROUND_UP(parent->rate / divisor)
>   * parent - fixed parent.  No clk_set_parent support
>   */
>  
> @@ -115,7 +115,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
>  		return parent_rate;
>  	}
>  
> -	return parent_rate / div;
> +	return DIV_ROUND_UP(parent_rate, div);
>  }
>  
>  /*
> @@ -185,7 +185,7 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
>  		}
>  		parent_rate = __clk_round_rate(__clk_get_parent(hw->clk),
>  				MULT_ROUND_UP(rate, i));
> -		now = parent_rate / i;
> +		now = DIV_ROUND_UP(parent_rate, i);
>  		if (now <= rate && now > best) {
>  			bestdiv = i;
>  			best = now;
> @@ -207,7 +207,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
>  	int div;
>  	div = clk_divider_bestdiv(hw, rate, prate);
>  
> -	return *prate / div;
> +	return DIV_ROUND_UP(*prate, div);
>  }
>  
>  static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
> @@ -218,7 +218,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
>  	unsigned long flags = 0;
>  	u32 val;
>  
> -	div = parent_rate / rate;
> +	div = DIV_ROUND_UP(parent_rate, rate);
>  	value = _get_val(divider, div);
>  
>  	if (value > div_mask(divider))
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[PATCH] clk: divider: fix rate calculation for fractional rates

[Tomi Valkeinen]

Mike, ping.

 Tomi

On 06/11/13 13:06, Tomi Valkeinen wrote:
> clk-divider.c does not calculate the rates consistently at the moment.
> 
> As an example, on OMAP3 we have a clock divider with a source clock of
> 864000000 Hz. With dividers 6, 7 and 8 the theoretical rates are:
> 
> 6: 144000000
> 7: 123428571.428571...
> 8: 108000000
> 
> Calling clk_round_rate() with the rate in the first column will give the
> rate in the second column:
> 
> 144000000 -> 144000000
> 143999999 -> 123428571
> 123428572 -> 123428571
> 123428571 -> 108000000
> 
> Note how clk_round_rate() returns 123428571 for rates from 123428572 to
> 143999999, which is mathematically correct, but when clk_round_rate() is
> called with 123428571, the returned value is surprisingly 108000000.
> 
> This means that the following code works a bit oddly:
> 
> rate = clk_round_rate(clk, 123428572);
> clk_set_rate(clk, rate);
> 
> As clk_set_rate() also does clock rate rounding, the result is that the
> clock is set to the rate of 108000000, not 123428571 returned by the
> clk_round_rate.
> 
> This patch changes the clk-divider.c to use DIV_ROUND_UP when
> calculating the rate. This gives the following behavior which fixes the
> inconsistency:
> 
> 144000000 -> 144000000
> 143999999 -> 123428572
> 123428572 -> 123428572
> 123428571 -> 108000000
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Mike Turquette <mturquette@linaro.org>
> ---
>  drivers/clk/clk-divider.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index 8d3009e..43348f3 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -24,7 +24,7 @@
>   * Traits of this clock:
>   * prepare - clk_prepare only ensures that parents are prepared
>   * enable - clk_enable only ensures that parents are enabled
> - * rate - rate is adjustable.  clk->rate = parent->rate / divisor
> + * rate - rate is adjustable.  clk->rate = DIV_ROUND_UP(parent->rate / divisor)
>   * parent - fixed parent.  No clk_set_parent support
>   */
>  
> @@ -115,7 +115,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
>  		return parent_rate;
>  	}
>  
> -	return parent_rate / div;
> +	return DIV_ROUND_UP(parent_rate, div);
>  }
>  
>  /*
> @@ -185,7 +185,7 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
>  		}
>  		parent_rate = __clk_round_rate(__clk_get_parent(hw->clk),
>  				MULT_ROUND_UP(rate, i));
> -		now = parent_rate / i;
> +		now = DIV_ROUND_UP(parent_rate, i);
>  		if (now <= rate && now > best) {
>  			bestdiv = i;
>  			best = now;
> @@ -207,7 +207,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
>  	int div;
>  	div = clk_divider_bestdiv(hw, rate, prate);
>  
> -	return *prate / div;
> +	return DIV_ROUND_UP(*prate, div);
>  }
>  
>  static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
> @@ -218,7 +218,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
>  	unsigned long flags = 0;
>  	u32 val;
>  
> -	div = parent_rate / rate;
> +	div = DIV_ROUND_UP(parent_rate, rate);
>  	value = _get_val(divider, div);
>  
>  	if (value > div_mask(divider))
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140211/7fadb1e8/attachment.sig>