Messages in this thread Patch in this message | | | Date | Wed, 9 Oct 2013 15:43:18 +0300 | From | Tomi Valkeinen <> | Subject | Re: Rounding issue in drivers/clk/clk-divider.c |
| |
Hi,
On 08/10/13 16:17, Tomi Valkeinen wrote: > Hi, > > I'm seeing the following issue on omap3 with dpll4_m4 clock. dpll4_m4's > parent is a PLL set to 864000000 and dpll4_m4 is a divider, handled by > clk-divider.c. > > Now, if I call clk_round_rate(dpll4_m4, 143999999), I get 123428571 > which is correct. However, if I call clk_round_rate(dpll4_m4, > 123428571), I would presume to get the same answer, 123428571, as that > was already "verified" by the previous clk_round_rate() call. However, I > get 108000000. > > So, if I have the following code: > > rate = clk_round_rate(dpll4_m4, 143999999); > /* rate is 123428571 */ > clk_set_rate(dpll4_m4, rate); > > the resulting rate is 108000000.
I continued testing with this, and with the following RFC patch I get consistent rates:
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index 8d3009e..ba20314 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -115,7 +115,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw, return parent_rate; } - return parent_rate / div; + return DIV_ROUND_UP(parent_rate, div); } /* @@ -207,7 +207,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate, int div; div = clk_divider_bestdiv(hw, rate, prate); - return *prate / div; + return DIV_ROUND_UP(*prate, div); } static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, @@ -218,7 +218,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long flags = 0; u32 val; - div = parent_rate / rate; + div = DIV_ROUND_UP(parent_rate, rate); value = _get_val(divider, div); if (value > div_mask(divider))
Now clk_round_rate for this clock returns the following:
144000000 -> 144000000 143999999 -> 123428572 123428572 -> 123428572 123428571 -> 108000000 So now multiple nested calls to clk_round_rate return consistent values, and calling clk_set_rate with the rate returned by clk_round_rate will not modify the rate.
I believe the patch is missing pieces, at least for clk_divider_bestdiv() for the case when CLK_SET_RATE_PARENT is set. Also, 864000000 / 7 = 123428571.4..., so in reality 123428571 would be a better answer than 123428572. But rounding to 123428572 makes things work consistently. However, even if the patch fixes the issue for me, I'm a bit confused on the clock rate rounding. How should it happen? Is it even defined how the rate is rounded?
In my particular use case I want to iterate the possible clock rates, so that I can find the best one to use. I do it with this kind of code:
/* start with the max rate my IP allows */ rate = max_allowed_fck; while (true) { rate = clk_round_rate(rate); test_rate(rate); /* -1, so that the next round will return the next lowest rate */ rate -= 1; } The code above presumes that the clk_round_rate will round down, but I don't see the rounding explicitly specified in any documentation. Is that kind of code valid?
Another use case I have is to set the clock rate to something which is higher than what I need. I.e. I know that I need at least 100MHz clock so that the IP performs the job quickly enough. If I call clk_round_rate(100M), I'll get a lower clock, not higher. So in this case I'd actually like the rounding to be up. And if the rate is rounded down, I have no idea what rate should I use to get at least 100MHz.
Am I doing something silly here? =) Should there be multiple clk_round_rate versions, for different roundings?
Tomi
[unhandled content-type:application/pgp-signature] | |