[RFC] Staging: imx-drm: Do not use fractional part of divider

[RFC] Staging: imx-drm: Do not use fractional part of divider

[Alexander Shiyan]

Hello.

Analysis of driver imx-drm led me to believe that the use fractional part of the divider is not always a good idea.
For example, for a parallel display bus connected to LVDS converter chip (DS90C363), in this case the use of
fractional part, clock will unstable and the on-chip PLL is not working properly, or rather, does not work at all.

Let me give a specific example.
ipu_crtc_mode_set 0x36314752
imx-ipuv3 40000000.ipu: clk_di_round_rate: inrate: 133000000 div: 0x00000035 outrate: 40150928 wanted: 40000000
imx-ipuv3 40000000.ipu: clk_di_round_rate: inrate: 133000000 div: 0x00000035 outrate: 40150928 wanted: 40150928
imx-ipuv3 40000000.ipu: clk_di_set_rate: inrate: 133000000 desired: 40150928 div: 0x00000035

In this case the divider is 3.5, that result to clock is incorrect. See an attached oscillogram F0000TEK.jpg.

After a patch the clocks is OK. Patch just uncomment some FSL code.
imx-ipuv3 40000000.ipu: clk_di_round_rate: inrate: 133000000 div: 0x00000040 outrate: 33250000 wanted: 40000000
imx-ipuv3 40000000.ipu: clk_di_round_rate: inrate: 133000000 div: 0x00000040 outrate: 33250000 wanted: 33250000
imx-ipuv3 40000000.ipu: clk_di_set_rate: inrate: 133000000 desired: 33250000 div: 0x00000040

See an attached oscillogram F0001TEK.jpg.

So, I want to review this from developers and wait for comments.


diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-di.c b/drivers/staging/imx-drm/ipu-v3/ipu-di.c
index 19d777e..d424c22 100644
--- a/drivers/staging/imx-drm/ipu-v3/ipu-di.c
+++ b/drivers/staging/imx-drm/ipu-v3/ipu-di.c
@@ -154,22 +154,15 @@ static int ipu_di_clk_calc_div(unsigned long inrate, unsigned long outrate)
 
 	if (div < 0x10)
 		div = 0x10;
-
-#ifdef WTF_IS_THIS
-	/*
-	 * Freescale has this in their Kernel. It is neither clear what
-	 * it does nor why it does it
-	 */
-	if (div & 0x10)
-		div &= ~0x7;
 	else {
 		/* Round up divider if it gets us closer to desired pix clk */
-		if ((div & 0xC) == 0xC) {
+		if (div & 0x0f) {
 			div += 0x10;
-			div &= ~0xF;
+			/* Strip fractional part of divider */
+			div &= ~0x0f;
 		}
 	}
-#endif
+
 	return div;
 }
 
-- 
1.8.1.5

---
-------------- next part --------------
A non-text attachment was scrubbed...
Name: F0001TEK.jpg
Type: image/jpeg
Size: 23677 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130614/ed347d72/attachment-0002.jpg>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: F0000TEK.jpg
Type: image/jpeg
Size: 22554 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130614/ed347d72/attachment-0003.jpg>

[RFC] Staging: imx-drm: Do not use fractional part of divider

[Jiada Wang]

Hello Alexander


Alexander Shiyan wrote:
> Hello.
>
> Analysis of driver imx-drm led me to believe that the use fractional part of the divider is not always a good idea.
> For example, for a parallel display bus connected to LVDS converter chip (DS90C363), in this case the use of
> fractional part, clock will unstable and the on-chip PLL is not working properly, or rather, does not work at all.
>
> Let me give a specific example.
> ipu_crtc_mode_set 0x36314752
> imx-ipuv3 40000000.ipu: clk_di_round_rate: inrate: 133000000 div: 0x00000035 outrate: 40150928 wanted: 40000000
> imx-ipuv3 40000000.ipu: clk_di_round_rate: inrate: 133000000 div: 0x00000035 outrate: 40150928 wanted: 40150928
> imx-ipuv3 40000000.ipu: clk_di_set_rate: inrate: 133000000 desired: 40150928 div: 0x00000035
>
> In this case the divider is 3.5, that result to clock is incorrect. See an attached oscillogram F0000TEK.jpg.
>
> After a patch the clocks is OK. Patch just uncomment some FSL code.
> imx-ipuv3 40000000.ipu: clk_di_round_rate: inrate: 133000000 div: 0x00000040 outrate: 33250000 wanted: 40000000
> imx-ipuv3 40000000.ipu: clk_di_round_rate: inrate: 133000000 div: 0x00000040 outrate: 33250000 wanted: 33250000
> imx-ipuv3 40000000.ipu: clk_di_set_rate: inrate: 133000000 desired: 33250000 div: 0x00000040
>
> See an attached oscillogram F0001TEK.jpg.
>
> So, I want to review this from developers and wait for comments.
>
>

Recently I am also looking at use of fractional part of CLKGEN0,
and want to discuss with you some information I found.

in our code we are using external PLL to drive DI pixel clock, although 
I haven't checked with oscilloscope, but it's apparently fractional part 
of CLKGEN0 doesn't work properly as it is described in reference manual. 
After some investigation I found 0x8 (0.5) seems works fine.

Our solution is try to set DI pixel clock's root clock to integer times 
of clk_di_pixel as close as possible, so that we can avoid using 
fractional part to get desired clock. if Pll -> ipu_di_podf could not 
provide the clock close enough, then try to set it to X.5 times of DI 
pixel clock, then only the "proved" 0x8 of fractional part will be used.

> diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-di.c b/drivers/stagineg/imx-drm/ipu-v3/ipu-di.c
> index 19d777e..d424c22 100644
> --- a/drivers/staging/imx-drm/ipu-v3/ipu-di.c
> +++ b/drivers/staging/imx-drm/ipu-v3/ipu-di.c
> @@ -154,22 +154,15 @@ static int ipu_di_clk_calc_div(unsigned long inrate, unsigned long outrate)
>
>   	if (div < 0x10)
>   		div = 0x10;
> -
> -#ifdef WTF_IS_THIS
> -	/*
> -	 * Freescale has this in their Kernel. It is neither clear what
> -	 * it does nor why it does it
> -	 */
> -	if (div & 0x10)
> -		div &= ~0x7;
>   	else {
>   		/* Round up divider if it gets us closer to desired pix clk */
> -		if ((div & 0xC) == 0xC) {
> +		if (div & 0x0f) {
>   			div += 0x10;
> -			div &= ~0xF;
> +			/* Strip fractional part of divider */
> +			div &= ~0x0f;
If div = 0x11, and the display is not forgiving enough, the pixel clock 
will probably not be accepted by it.

thanks,
jiada
>   		}
>   	}
> -#endif
> +
>   	return div;
>   }
>
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[RFC] Staging: imx-drm: Do not use fractional part of divider

[Sascha Hauer]

On Fri, Jun 14, 2013 at 11:41:10AM +0400, Alexander Shiyan wrote:
> Hello.
> 
> Analysis of driver imx-drm led me to believe that the use fractional part of the divider is not always a good idea.
> For example, for a parallel display bus connected to LVDS converter chip (DS90C363), in this case the use of
> fractional part, clock will unstable and the on-chip PLL is not working properly, or rather, does not work at all.
> 
> Let me give a specific example.
> ipu_crtc_mode_set 0x36314752
> imx-ipuv3 40000000.ipu: clk_di_round_rate: inrate: 133000000 div: 0x00000035 outrate: 40150928 wanted: 40000000
> imx-ipuv3 40000000.ipu: clk_di_round_rate: inrate: 133000000 div: 0x00000035 outrate: 40150928 wanted: 40150928
> imx-ipuv3 40000000.ipu: clk_di_set_rate: inrate: 133000000 desired: 40150928 div: 0x00000035
> 
> In this case the divider is 3.5, that result to clock is incorrect. See an attached oscillogram F0000TEK.jpg.
> 
> After a patch the clocks is OK. Patch just uncomment some FSL code.
> imx-ipuv3 40000000.ipu: clk_di_round_rate: inrate: 133000000 div: 0x00000040 outrate: 33250000 wanted: 40000000
> imx-ipuv3 40000000.ipu: clk_di_round_rate: inrate: 133000000 div: 0x00000040 outrate: 33250000 wanted: 33250000
> imx-ipuv3 40000000.ipu: clk_di_set_rate: inrate: 133000000 desired: 33250000 div: 0x00000040
> 
> See an attached oscillogram F0001TEK.jpg.
> 
> So, I want to review this from developers and wait for comments.
> 

We may need some flag in the devicetree to specify whether the
fractional divider can be used or not.

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 |