Rounding issue in drivers/clk/clk-divider.c

Rounding issue in drivers/clk/clk-divider.c

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

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.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: Rounding issue in drivers/clk/clk-divider.c

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3631 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: Rounding issue in drivers/clk/clk-divider.c

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3894 bytes --]

Hi,

Ping.

 Tomi

On 09/10/13 15:43, Tomi Valkeinen wrote:
> 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
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]