Does clk_core_round_rate_nolock() really do the right thing?
"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?
Signature · 4 lines
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.
Stephen Boyd
maintainerQuoting Jeffrey Hugo (2018-11-13 14:37:54)
Quoted 35 lines
"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(), throughclk_core_req_round_rate_nolock()back toclk_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.
Quoted 6 lines
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.