On Thu, Mar 15, 2012 at 2:43 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Wed, Mar 14, 2012 at 05:51:48PM -0700, Turquette, Mike wrote:
@@ -84,9 +78,9 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
for (i = 1; i <= maxdiv; i++) { parent_rate = __clk_round_rate(__clk_get_parent(hw->clk),
- MULT_ROUND_UP(rate, i));
- (rate * i));
I think MULT_ROUND_UP is the right thing to use here (not sure if this is a good name though) Consider we want to have an output rate of 33Hz. Now acceptable input rates for a divider value of 3 would be 99, 100 and 101Hz, so we have to call round_rate for the parent with 101Hz which includes 100 and 99Hz.
We're back to the point Rob brought up about .round_rate rounding up or down. We really need a least-upper-bounds or greatest-lower-bounds flag, similar to how CPUfreq selects target frequencies today. I'm freezing features for this patchset now, so I'll keep your MULT_ROUND_UP approach and some day I'll fix .round_rate for good.
If you have problems with your PLL than most likely because it does something different on clk_round_rate than it does in clk_set_rate, for example clk_round_rate(10000) returns 10000, but clk_set_rate then sets the rate 9999 due to some rounding error. Being consistent between round_rate and set_rate is very important for this mechanism to work properly. It did cost me some nerves to get it right for the divider (and even more nerves to figure out why it is correct the way it works)
What I'd like to do is request the *exact* frequency I want when passing in a rate to my PLLs .round_rate. Due to the way that my MN dividers are calculated, and due to some jitter avoidance code, it is bad for me to request 600000001 Hz when I really want 600MHz exactly. Anyways I fixed this up on my end enough to work with MULT_ROUND_UP, so that can stay for the immediate future.
now = parent_rate / i;
- if (now <= rate && now >= best) {
- if (now <= rate && now > best) {
This change is an optimization, but should be unrelated to your PLL problem, right?
MULT_ROUND_UP yields multiples of 'rate' plus an incrementing value (m - 1). Without that incrementing value added to the rate passed into __clk_round_rate the for loop above will always max out the divider. To illustrate:
The rate we want is 300MHz. Without MULT_ROUND_UP the for loop will start yielding these combinations:
__clk_round_rate(parent, 300MHz), divide-by-1 __clk_round_rate(parent, 600MHz), divide-by-2 __clk_round_rate(parent, 900MHz), divide-by-3 __clk_round_rate(parent, 1200MHz), divide-by-4 ...etc...
These all yield the desired 300MHz for our divider so it just keeps going until you max out the divider and requests some crazy rate for the parent. On most hardware that I am aware of it is desirable to keep the divider as low as possible so the change to the conditional prevents us from overwriting best every single time while keeping the divider low. So yes, it is an optimization, but it is also quite necessary without MULT_ROUND_UP.
Anyways I've kept MULT_ROUND_UP exactly as-is with the exception that I've added that small optimization to keep dividers low. I hope there are no objections to that.
Regards, Mike
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |