On Wed, Mar 28, 2012 at 3:25 PM, Saravana Kannan skannan@codeaurora.org wrote:
On 03/28/2012 10:08 AM, Turquette, Mike wrote:
On Tue, Mar 27, 2012 at 8:06 PM, Saravana Kannanskannan@codeaurora.org wrote:
I think there is still a problem with not being able to differentiate between pre-change recalc and post-change recalc. This applies for any set parent and set rate on a clock that has children.
Consider this simple example:
- Divider clock B is fed from clock A.
- Clock B can never output> 120 MHz due to downstream
HW/clock limitations.
- Clock A is running at 200 MHz
- Clock B divider is set to 2.
Now, say the rate of clock A is changing from 200 MHz to 300 MHz (due to set rate or set parent). In this case, the clock B divider should be set to 3 pre-rate change to guarantee that the output of clock B is never> 120 MHz. So the rate of clock B will go from 100 MHz (200/2) to 66 MHz (200/3) to 100 MHz (300/3) and everything is good.
Assume we somehow managed to do the above. So, now clock A is at 300 MHz, clock B divider is at 3 and the clock B output is 100 MHz.
Now, say the rate of clock A changes from 300 MHz to 100 MHz. In this case the clock B divider should only be changed post rate change. If we do it pre rate change, then the output will go from 100 MHz (300/3) to 150 MHz (300/1) to 100 MHz (100/1). We went past the 120 MHz limit of clock B's output rate.
If we do this post rate change, we can do this without exceeding the max output limit of clock B. It will go from 100 MHz (300/3) to 33 MHz (100/3) to 100 MHz (100/1). We never went past the 120 MHz limit.
So, at least for this reason above, I think we need to pass a pre/post parameter to the recalc ops.
Sorry if I wasn't clear. But the case above is a separate issue from what I mention below. What are your thoughts on handling this? Pass "msg" to recalc_rates?
I'm OK passing the msg to in .recalc_rates for your hardware which samples frequency.
While we are at it, we should probably just add a failure option for recalc to make it easy to reject unacceptable rate changes. To keep the clock framework code simpler, you could decide to allow errors only for the pre-change recalc calls. That way, the error case roll back code won't be crazy.
We do need a rate validation mechanism, which neither .round_rate or .recalc_rates provide. But I don't want to bolt one on right now because the proper solution to that problem is not a quick fix. Coupled to this issue are lingering topics that we've discussed tentatively in the past:
1) rate ranges (min/max)
2) clock dependencies; these should enforce functional dependencies between clocks
3) intended rates; this strays into the scary arena of policy, which has traditionally fallen outside the purview of the clock framework. But if we want a better clock framework we probably need a way for downstream clocks to make intelligent decisions about their rates when parent rates change. We could overload .round_rate to do this for clk_set_rate, but that doesn't help out clk_set_parent. And I'm against overloading the functionality of these callbacks any more than I already have.
I'm not sure that just overloading .recalc_rates is the best approach. I like that .recalc_rates only recalculates the rate! For clocks that need to make an intelligent decision about their rates when parent rates change we might need to introduce a new callback and/or refactor the clk_set_rate and clk_set_parent paths to use some common code.
recalc is too late to catch this. I think you mean round_rate. We want to determine which rate changes are out-of-spec during clk_calc_new_rates (for clk_set_rate) which involves round_rate being a bit smarter about what it can and cannot do.
The case I'm referring to is set_parent(). set_rate() and set_parent() have a lot of common implications and it doesn't look like the clock framework is handling the common cases the same way for both set_parent() and set_rate().
Agreed that there needs to be some unification here. Certainly a common rate validation mechanism is missing and the downstream notification diverged in the last set of patches.
It almost feels like set_parent() and set_rate() should just be wrappers around some common function that handles most of the work. After all, set_parent() is just a slimmed down version of set_rate(). Set rate is just a combination of set parent and set divider.
I mostly agree. A big caveat is that clk_set_parent will never propagate a request up the tree, but that doesn't mean the implementation details of the two functions are mutually exclusive.
Anyways I'm looking at ways to do this in my clk-dependencies branch.
Are you also looking into the pre/post rate change divider handling case I mentioned above?
I'll admit that my initial focus on clk-deps was more along the lines of:
"Clock A wants to go faster, and has a functional dependency to scale Clock B to a faster rate first"
That sort of situation is often necessary to keep certain timing requirements valid. But your example reinforces that clocks need to have some clue about rate ranges, and it resets my opinion on writing a clk-dep mechanism which is just focused on raising rates. The real solution should be generic and able to handle lowering and NACKing rates also.
In short: for now I'll just pass the msg into .recalc_rates. The other stuff is a design activity which is out-of-scope for the upcoming -rc bugfix windows.
Regards, Mike