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.
--
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".
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