Does clk_core_round_rate_nolock() really do the right thing?

Does clk_core_round_rate_nolock() really do the right thing?

[Jeffrey Hugo]

"clk: rework calls to round and determine rate callbacks" [1] added 
clk_core_round_rate_nolock().

I question that it does the right thing in a specific scenario.

Lets assume we have a clock that is currently running at 5Mhz, but we 
want to have it run at 400Khz, and the clock supports both rates. 
Perhaps the clock is for a hardware block, where in to switch 
operational modes, the clock frequency is expected to run at the lower 
frequency.

The driver may call clk_set_rate() to do so, resulting in the following 
call flow (based on 4.20-rc2)-

clk_set_rate()
clk_core_set_rate_nolock()
clk_core_req_round_rate_nolock()
clk_core_round_rate_nolock()

In clk_core_round_rate_nolock(), lets assume the conditionals do not 
apply (clk core cannot round, and set rate parent flag isn't set).  In 
that case, the requested rate is modified by this line -

req->rate = core->rate;

This transforms the request from 400Khz to 5Mhz (the current clock rate).

What will then end up happening is we return from 
clk_core_round_rate_nolock(), through clk_core_req_round_rate_nolock() 
back to clk_core_set_rate_nolock() where we check to see if the rate 
returned from the "rounding" is the same as the current clock rate (5Mhz 
== 5Mhz).  Thus we conclude there is nothing to be done, and we return 
success back up to the caller having not actually touched the clock or 
set the requested rate.

On the face of it, this seems incorrect to me, although I am a neophyte 
to the clock framework.  Is this really the expected behavior, to leave 
the clock at the current rate even though the requested rate is 
different?  If so, why?


[1] - 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0f6cc2b8e94da5400528c0ba7fd910392ec598a2

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: Does clk_core_round_rate_nolock() really do the right thing?

[Stephen Boyd]

Quoting Jeffrey Hugo (2018-11-13 14:37:54)
> "clk: rework calls to round and determine rate callbacks" [1] added 
> clk_core_round_rate_nolock().
> 
> I question that it does the right thing in a specific scenario.
> 
> Lets assume we have a clock that is currently running at 5Mhz, but we 
> want to have it run at 400Khz, and the clock supports both rates. 
> Perhaps the clock is for a hardware block, where in to switch 
> operational modes, the clock frequency is expected to run at the lower 
> frequency.
> 
> The driver may call clk_set_rate() to do so, resulting in the following 
> call flow (based on 4.20-rc2)-
> 
> clk_set_rate()
> clk_core_set_rate_nolock()
> clk_core_req_round_rate_nolock()
> clk_core_round_rate_nolock()
> 
> In clk_core_round_rate_nolock(), lets assume the conditionals do not 
> apply (clk core cannot round, and set rate parent flag isn't set).  In 
> that case, the requested rate is modified by this line -
> 
> req->rate = core->rate;
> 
> This transforms the request from 400Khz to 5Mhz (the current clock rate).
> 
> What will then end up happening is we return from 
> clk_core_round_rate_nolock(), through clk_core_req_round_rate_nolock() 
> back to clk_core_set_rate_nolock() where we check to see if the rate 
> returned from the "rounding" is the same as the current clock rate (5Mhz 
> == 5Mhz).  Thus we conclude there is nothing to be done, and we return 
> success back up to the caller having not actually touched the clock or 
> set the requested rate.
> 
> On the face of it, this seems incorrect to me, although I am a neophyte 
> to the clock framework.  Is this really the expected behavior, to leave 
> the clock at the current rate even though the requested rate is 
> different?  If so, why?
> 
> 

I think you're getting hung up on how clk_set_rate() works.
clk_set_rate() sets the rate to be what the clk can support based on the
requested rate. I suppose we could have named it better to be
clk_request_something_close_to_rate(). The assumption is that a clk
consumer will request the rate it wants, and the clk provider, in this
case the fixed 5MHz clk, will set the rate to what it can support. If
the consumer had called clk_round_rate() before calling clk_set_rate()
to figure out what rate it would get if it called clk_set_rate() with
the same arguments, then it would be informed about what rate is
supported, i.e. 5MHz. It could also call clk_set_rate(4kHz) and then
call clk_get_rate() and see 5MHz returned.

Long story short, clk_set_rate() isn't clk_set_exact_rate(). There's
rounding involved and that rounding is implementation specific.

In your example, it sounds like the clk needs to support rounding so it
can pick between the two different frequencies supported. Then
clk_set_rate() can be called with some frequency and the implementation
can round however it likes to achieve the two supported frequencies.