All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Alexander Shiyan" <shc_work@mail.ru>
To: linux-kernel@vger.kernel.org,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	devel@driverdev.osuosl.org,
	"Sascha Hauer" <s.hauer@pengutronix.de>,
	linux-arm-kernel@lists.infradead.org,
	"Shawn Guo" <shawn.guo@linaro.org>,
	"Philipp Zabel" <p.zabel@pengutronix.de>
Subject: [RFC] Staging: imx-drm: Do not use fractional part of divider
Date: Fri, 14 Jun 2013 11:41:10 +0400	[thread overview]
Message-ID: <1371195670.904413971@f400.i.mail.ru> (raw)

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

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

---

[-- Attachment #2: F0001TEK.jpg --]
[-- Type: image/jpeg, Size: 23677 bytes --]

[-- Attachment #3: F0000TEK.jpg --]
[-- Type: image/jpeg, Size: 22554 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: shc_work@mail.ru (Alexander Shiyan)
To: linux-arm-kernel@lists.infradead.org
Subject: [RFC] Staging: imx-drm: Do not use fractional part of divider
Date: Fri, 14 Jun 2013 11:41:10 +0400	[thread overview]
Message-ID: <1371195670.904413971@f400.i.mail.ru> (raw)

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>

             reply	other threads:[~2013-06-14  7:43 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-06-14  7:41 Alexander Shiyan [this message]
2013-06-14  7:41 ` [RFC] Staging: imx-drm: Do not use fractional part of divider Alexander Shiyan
2013-06-14 23:23 ` Jiada Wang
2013-06-14 23:23   ` Jiada Wang
2013-06-17  8:48 ` Sascha Hauer
2013-06-17  8:48   ` Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1371195670.904413971@f400.i.mail.ru \
    --to=shc_work@mail.ru \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=s.hauer@pengutronix.de \
    --cc=shawn.guo@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.