| Message ID | 20131106161911.GA16735@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
--- 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);