[PATCH v2] clk: si5351: fix .round_rate for multisynth 6-7

[PATCH v2] clk: si5351: fix .round_rate for multisynth 6-7

[Sergej Sawazki]

The divider calculation for multisynth 6 and 7 differs from the
calculation for multisynth 0-5.

For MS6 and MS7, set MSx_P1 directly, MSx_P1=divide value
[AN619, p. 6].

Referenced document:
[AN619] Manually Generating an Si5351 Register Map, Rev. 0.4

Signed-off-by: Sergej Sawazki <ce3a@gmx.de>
---
 drivers/clk/clk-si5351.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index 44ea107..1029bf7 100644
--- a/drivers/clk/clk-si5351.c
+++ b/drivers/clk/clk-si5351.c
@@ -552,7 +552,8 @@ static const struct clk_ops si5351_pll_ops = {
  * MSx_P2[19:0] = 128 * b - c * floor(128 * b/c) = (128*b) mod c
  * MSx_P3[19:0] = c
  *
- * MS[6,7] are integer (P1) divide only, P2 = 0, P3 = 0
+ * MS[6,7] are integer (P1) divide only, P1 = divide value,
+ * P2 and P3 are not applicable
  *
  * for 150MHz < fOUT <= 160MHz:
  *
@@ -645,7 +646,7 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
 	struct si5351_hw_data *hwdata =
 		container_of(hw, struct si5351_hw_data, hw);
 	unsigned long long lltmp;
-	unsigned long a, b, c;
+	unsigned long a, b = 0, c = 1;
 	int divby4;
 
 	/* multisync6-7 can only handle freqencies < 150MHz */
@@ -675,9 +676,6 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
 		} else
 			a = 4;
 
-		b = 0;
-		c = 1;
-
 		*parent_rate = a * rate;
 	} else {
 		unsigned long rfrac, denom;
@@ -698,18 +696,19 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
 			a = SI5351_MULTISYNTH_A_MAX;
 
 		/* find best approximation for b/c = fVCO mod fOUT */
-		denom = 1000 * 1000;
-		lltmp = (*parent_rate) % rate;
-		lltmp *= denom;
-		do_div(lltmp, rate);
-		rfrac = (unsigned long)lltmp;
-
-		b = 0;
-		c = 1;
-		if (rfrac)
-			rational_best_approximation(rfrac, denom,
-			    SI5351_MULTISYNTH_B_MAX, SI5351_MULTISYNTH_C_MAX,
-			    &b, &c);
+		if (hwdata->num <= 5) {
+			denom = 1000 * 1000;
+			lltmp = (*parent_rate) % rate;
+			lltmp *= denom;
+			do_div(lltmp, rate);
+			rfrac = (unsigned long)lltmp;
+
+			if (rfrac)
+				rational_best_approximation(rfrac, denom,
+					SI5351_MULTISYNTH_B_MAX,
+					SI5351_MULTISYNTH_C_MAX,
+					&b, &c);
+		}
 	}
 
 	/* recalculate rate by fOUT = fIN / (a + b/c) */
@@ -723,6 +722,10 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
 		hwdata->params.p3 = 1;
 		hwdata->params.p2 = 0;
 		hwdata->params.p1 = 0;
+	} else if (hwdata->num >= 6) {
+		hwdata->params.p3 = c;
+		hwdata->params.p2 = b;
+		hwdata->params.p1 = a;
 	} else {
 		hwdata->params.p3  = c;
 		hwdata->params.p2  = (128 * b) % c;
-- 
1.9.1

Re: [PATCH v2] clk: si5351: fix .round_rate for multisynth 6-7

[Sebastian Hesselbarth]

On 07.04.2015 21:11, Sergej Sawazki wrote:
> The divider calculation for multisynth 6 and 7 differs from the
> calculation for multisynth 0-5.
>
> For MS6 and MS7, set MSx_P1 directly, MSx_P1=divide value
> [AN619, p. 6].
>
> Referenced document:
> [AN619] Manually Generating an Si5351 Register Map, Rev. 0.4
>
> Signed-off-by: Sergej Sawazki <ce3a@gmx.de>
> ---
>   drivers/clk/clk-si5351.c | 37 ++++++++++++++++++++-----------------
>   1 file changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
> index 44ea107..1029bf7 100644
> --- a/drivers/clk/clk-si5351.c
> +++ b/drivers/clk/clk-si5351.c
> @@ -552,7 +552,8 @@ static const struct clk_ops si5351_pll_ops = {
>    * MSx_P2[19:0] = 128 * b - c * floor(128 * b/c) = (128*b) mod c
>    * MSx_P3[19:0] = c
>    *
> - * MS[6,7] are integer (P1) divide only, P2 = 0, P3 = 0
> + * MS[6,7] are integer (P1) divide only, P1 = divide value,
> + * P2 and P3 are not applicable
>    *
>    * for 150MHz < fOUT <= 160MHz:
>    *
> @@ -645,7 +646,7 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
>   	struct si5351_hw_data *hwdata =
>   		container_of(hw, struct si5351_hw_data, hw);
>   	unsigned long long lltmp;
> -	unsigned long a, b, c;
> +	unsigned long a, b = 0, c = 1;

Actually, moving b,c initialization up here is neither related
to the patch itself nor is it mentioned in the commit log.

Please do not mix different patches.

>   	int divby4;
>
>   	/* multisync6-7 can only handle freqencies < 150MHz */
> @@ -675,9 +676,6 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
>   		} else
>   			a = 4;
>
> -		b = 0;
> -		c = 1;
> -
>   		*parent_rate = a * rate;
>   	} else {
>   		unsigned long rfrac, denom;
> @@ -698,18 +696,19 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
>   			a = SI5351_MULTISYNTH_A_MAX;
>
>   		/* find best approximation for b/c = fVCO mod fOUT */
> -		denom = 1000 * 1000;
> -		lltmp = (*parent_rate) % rate;
> -		lltmp *= denom;
> -		do_div(lltmp, rate);
> -		rfrac = (unsigned long)lltmp;
> -
> -		b = 0;
> -		c = 1;
> -		if (rfrac)
> -			rational_best_approximation(rfrac, denom,
> -			    SI5351_MULTISYNTH_B_MAX, SI5351_MULTISYNTH_C_MAX,
> -			    &b, &c);
> +		if (hwdata->num <= 5) {
> +			denom = 1000 * 1000;
> +			lltmp = (*parent_rate) % rate;
> +			lltmp *= denom;
> +			do_div(lltmp, rate);
> +			rfrac = (unsigned long)lltmp;
> +
> +			if (rfrac)
> +				rational_best_approximation(rfrac, denom,
> +					SI5351_MULTISYNTH_B_MAX,
> +					SI5351_MULTISYNTH_C_MAX,
> +					&b, &c);
> +		}

I am still not convinced that this is the right way to calculate the
_best_ integer divider for ms6,7.

The code above is written to (a) find the _largest_ integer "a" that
will match

a * rate <= parent_rate

and then (b) determines the fractional part of the divider.

As you correctly stated, ms6,7 do not support (b) but if we use (a) for
those msynths, we will determine an "a" so that "a * rate" is always
smaller than parent_rate.

What we actually want is the smallest error between generated and
requested rate, so we have to find the closest integer with

a = DIV_ROUND_CLOSEST(parent_rate, rate)

IMHO, the special divider calculation for ms6,7 is best dealt with
in an extra else-if branch after the check for CLK_SET_RATE_PARENT
flag.

>   	}
>
>   	/* recalculate rate by fOUT = fIN / (a + b/c) */
> @@ -723,6 +722,10 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
>   		hwdata->params.p3 = 1;
>   		hwdata->params.p2 = 0;
>   		hwdata->params.p1 = 0;
> +	} else if (hwdata->num >= 6) {
> +		hwdata->params.p3 = c;
> +		hwdata->params.p2 = b;

hwdata->params.p3 = 0;
hwdata->params.p2 = 0;

> +		hwdata->params.p1 = a;
>   	} else {
>   		hwdata->params.p3  = c;
>   		hwdata->params.p2  = (128 * b) % c;
>

Out of curiosity, do you actually have a device that uses ms6,7 and can
you measure the generated frequency - or did you just read the code as
a bedtime story? ;)

Sebastian

Aw: Re: [PATCH v2] clk: si5351: fix .round_rate for multisynth 6-7

[S. S.]

>> @@ -645,7 +646,7 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
>> struct si5351_hw_data *hwdata =
>> container_of(hw, struct si5351_hw_data, hw);
>> unsigned long long lltmp;
>> - unsigned long a, b, c;
>> + unsigned long a, b = 0, c = 1;
>
> Actually, moving b,c initialization up here is neither related
> to the patch itself nor is it mentioned in the commit log.
>
> Please do not mix different patches.

I agree. I just thought we could avoid repeating the b,c initialization
in the code, but You are right, that would be a different patch, if any 
at all.

>> *parent_rate = a * rate;
>
>> + if (rfrac)
>> + rational_best_approximation(rfrac, denom,
>> + SI5351_MULTISYNTH_B_MAX,
>> + SI5351_MULTISYNTH_C_MAX,
>> + &b, &c);
>> + }
>
> I am still not convinced that this is the right way to calculate the
> _best_ integer divider for ms6,7.
>
> The code above is written to (a) find the _largest_ integer "a" that
> will match
>
> a * rate <= parent_rate
>
> and then (b) determines the fractional part of the divider.
>
> As you correctly stated, ms6,7 do not support (b) but if we use (a) for
> those msynths, we will determine an "a" so that "a * rate" is always
> smaller than parent_rate.
> 
> What we actually want is the smallest error between generated and
> requested rate, so we have to find the closest integer with
> 
> a = DIV_ROUND_CLOSEST(parent_rate, rate)

Agreed, that's probably better.

> IMHO, the special divider calculation for ms6,7 is best dealt with
> in an extra else-if branch after the check for CLK_SET_RATE_PARENT
> flag.

Agreed.

> hwdata->params.p3 = 0;
> hwdata->params.p2 = 0;

Agreed.

> Out of curiosity, do you actually have a device that uses ms6,7 and can
> you measure the generated frequency - or did you just read the code as
> a bedtime story? ;)

:) I do have a device and have to use ms6,7, those are connected to pllb.
I have tested some integer pll dividers in a range from 30 to 130.
Measured frequencies and the frequencies shown in <debugfs>/clk/clk_summery
are the same.

I am going to send a patch v3.
Thanks for your feedback.

Sergej