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

[PATCH] 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 | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index 44ea107..310078d 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:
  *
@@ -718,11 +719,18 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
 	do_div(lltmp, a * c + b);
 	rate  = (unsigned long)lltmp;
 
-	/* calculate parameters */
+	/*
+	 * calculate parameters
+	 * for multisync6-7 set p1 directly, fOUT = fIN / p1
+	 */
 	if (divby4) {
 		hwdata->params.p3 = 1;
 		hwdata->params.p2 = 0;
 		hwdata->params.p1 = 0;
+	} else if (hwdata->num >= 6) {
+		hwdata->params.p3 = 1;
+		hwdata->params.p2 = 0;
+		hwdata->params.p1 = a;
 	} else {
 		hwdata->params.p3  = c;
 		hwdata->params.p2  = (128 * b) % c;
-- 
1.9.1

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

[Sebastian Hesselbarth]

On 06.04.2015 16:25, 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>

Sergej,

thanks for the patch, I do have some remarks though.

> ---
>   drivers/clk/clk-si5351.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
> index 44ea107..310078d 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:
>    *
> @@ -718,11 +719,18 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
>   	do_div(lltmp, a * c + b);
>   	rate  = (unsigned long)lltmp;
>
> -	/* calculate parameters */
> +	/*
> +	 * calculate parameters
> +	 * for multisync6-7 set p1 directly, fOUT = fIN / p1

I'd say the comment about the differences of MS6,7 in the function's
comment block is enough, no need to repeat it again here.

> +	 */
>   	if (divby4) {
>   		hwdata->params.p3 = 1;
>   		hwdata->params.p2 = 0;
>   		hwdata->params.p1 = 0;
> +	} else if (hwdata->num >= 6) {
> +		hwdata->params.p3 = 1;
> +		hwdata->params.p2 = 0;
> +		hwdata->params.p1 = a;

Hmm. Just overruling what will be written in the registers is
definitely not enough. If you look at the first two lines of this
hunk, you'll admit that the visible representation (rate) of the MS
frequency isn't affected by your patch at all.

That basically means that real frequency and frequency in the kernel
will differ, so we have to put this change in the param calculation
above this hunk instead. The extra else-if-path here is ok though.

Sebastian

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

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

[Sergej Sawazki]

Sebastian,

thanks for your reply, please find my comments below:

On 06.04.2015 18:43, Sebastian Hesselbarth wrote:
> On 06.04.2015 16:25, 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>
>
> Sergej,
>
> thanks for the patch, I do have some remarks though.
>
>> ---
>>   drivers/clk/clk-si5351.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
>> index 44ea107..310078d 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:
>>    *
>> @@ -718,11 +719,18 @@ static long si5351_msynth_round_rate(struct
>> clk_hw *hw, unsigned long rate,
>>       do_div(lltmp, a * c + b);
>>       rate  = (unsigned long)lltmp;
>>
>> -    /* calculate parameters */
>> +    /*
>> +     * calculate parameters
>> +     * for multisync6-7 set p1 directly, fOUT = fIN / p1
>
> I'd say the comment about the differences of MS6,7 in the function's
> comment block is enough, no need to repeat it again here.
>
I agree.
>> +     */
>>       if (divby4) {
>>           hwdata->params.p3 = 1;
>>           hwdata->params.p2 = 0;
>>           hwdata->params.p1 = 0;
>> +    } else if (hwdata->num >= 6) {
>> +        hwdata->params.p3 = 1;
>> +        hwdata->params.p2 = 0;
>> +        hwdata->params.p1 = a;
>
> Hmm. Just overruling what will be written in the registers is
> definitely not enough. If you look at the first two lines of this
> hunk, you'll admit that the visible representation (rate) of the MS
> frequency isn't affected by your patch at all.
>
> That basically means that real frequency and frequency in the kernel
> will differ, so we have to put this change in the param calculation
> above this hunk instead. The extra else-if-path here is ok though.
>
You right, I will take a look at that and provide a patch v2.

Sergej