diff mbox

ARM: video: mxs: Fix mxsfb misconfiguring VDCTRL0

Message ID 1363471581-10132-1-git-send-email-marex@denx.de (mailing list archive)
State New, archived
Headers show

Commit Message

Marek Vasut March 16, 2013, 10:06 p.m. UTC
The issue fixed by this patch manifests only then using X11
with mxsfb driver. The X11 will display either shifted image
or otherwise distorted image on the LCD.

The problem is that the X11 tries to reconfigure the framebuffer
and along the way call fb_ops.fb_set_par() with it's configuration
values. The field of particular interest is fb_info->var.sync which
contains non-standard values if configured by kernel. These are
FB_SYNC_DATA_ENABLE_HIGH_ACT and FB_SYNC_DOTCLK_FAILING_ACT defined
in include/linux/mxsfb.h . The driver interprets those and configures
the LCD controller accordingly. Yet X11 only has access to standard
values for this field defined in include/uapi/linux/fb.h and thus
omits these special values. This results in distorted image on the
LCD.

This patch moves these non-standard values into new field of the
mxsfb_platform_data structure so the driver can in turn check this
field instead of the video mode field for these specific portions.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estavem@freescale.com>
Cc: Linux ARM <linux-arm-kernel@lists.infradead.org>
Cc: Linux FBDEV <linux-fbdev@vger.kernel.org>
Cc: Lothar Waßmann <LW@karo-electronics.de>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/mach-mxs/mach-mxs.c |   22 +++++++++++-----------
 drivers/video/mxsfb.c        |    7 +++++--
 include/linux/mxsfb.h        |    7 +++++--
 3 files changed, 21 insertions(+), 15 deletions(-)

NOTE: We ought to get rid of this platform data goo, but we can't do
      it in here. This is a bugfix.

Comments

Fabio Estevam March 16, 2013, 10:38 p.m. UTC | #1
On Sat, Mar 16, 2013 at 7:06 PM, Marek Vasut <marex@denx.de> wrote:
> The issue fixed by this patch manifests only then using X11
> with mxsfb driver. The X11 will display either shifted image
> or otherwise distorted image on the LCD.
>
> The problem is that the X11 tries to reconfigure the framebuffer
> and along the way call fb_ops.fb_set_par() with it's configuration
> values. The field of particular interest is fb_info->var.sync which
> contains non-standard values if configured by kernel. These are
> FB_SYNC_DATA_ENABLE_HIGH_ACT and FB_SYNC_DOTCLK_FAILING_ACT defined
> in include/linux/mxsfb.h . The driver interprets those and configures
> the LCD controller accordingly. Yet X11 only has access to standard
> values for this field defined in include/uapi/linux/fb.h and thus
> omits these special values. This results in distorted image on the
> LCD.
>
> This patch moves these non-standard values into new field of the
> mxsfb_platform_data structure so the driver can in turn check this
> field instead of the video mode field for these specific portions.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estavem@freescale.com>
> Cc: Linux ARM <linux-arm-kernel@lists.infradead.org>
> Cc: Linux FBDEV <linux-fbdev@vger.kernel.org>
> Cc: Lothar Waßmann <LW@karo-electronics.de>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Shawn Guo <shawn.guo@linaro.org>

This fixes the X11 offset issue on my mx28evk, thanks!

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shawn Guo March 18, 2013, 12:44 p.m. UTC | #2
On Sat, Mar 16, 2013 at 11:06:21PM +0100, Marek Vasut wrote:
> The issue fixed by this patch manifests only then using X11
> with mxsfb driver. The X11 will display either shifted image
> or otherwise distorted image on the LCD.
> 
> The problem is that the X11 tries to reconfigure the framebuffer
> and along the way call fb_ops.fb_set_par() with it's configuration
> values. The field of particular interest is fb_info->var.sync which
> contains non-standard values if configured by kernel. These are
> FB_SYNC_DATA_ENABLE_HIGH_ACT and FB_SYNC_DOTCLK_FAILING_ACT defined
> in include/linux/mxsfb.h . The driver interprets those and configures
> the LCD controller accordingly. Yet X11 only has access to standard
> values for this field defined in include/uapi/linux/fb.h and thus
> omits these special values. This results in distorted image on the
> LCD.
> 
> This patch moves these non-standard values into new field of the
> mxsfb_platform_data structure so the driver can in turn check this
> field instead of the video mode field for these specific portions.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estavem@freescale.com>
> Cc: Linux ARM <linux-arm-kernel@lists.infradead.org>
> Cc: Linux FBDEV <linux-fbdev@vger.kernel.org>
> Cc: Lothar Waßmann <LW@karo-electronics.de>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm/mach-mxs/mach-mxs.c |   22 +++++++++++-----------
>  drivers/video/mxsfb.c        |    7 +++++--
>  include/linux/mxsfb.h        |    7 +++++--
>  3 files changed, 21 insertions(+), 15 deletions(-)

Since it touches mach-mxs as well, let me send it through arm-soc tree
as fix for 3.9.

Shawn

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shawn Guo March 18, 2013, 1:01 p.m. UTC | #3
On Mon, Mar 18, 2013 at 10:11:47AM -0300, Fabio Estevam wrote:
> Shawn Guo wrote:
> 
> > Since it touches mach-mxs as well, let me send it through arm-soc tree
> > as fix for 3.9.
> 
> Please CC stable as well, so that we can have X11 functional with 3.8.

Ok, done.

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam March 18, 2013, 1:11 p.m. UTC | #4
Shawn Guo wrote:

> Since it touches mach-mxs as well, let me send it through arm-soc tree
> as fix for 3.9.

Please CC stable as well, so that we can have X11 functional with 3.8.


--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Vasut March 18, 2013, 1:58 p.m. UTC | #5
Dear Shawn Guo,

> On Mon, Mar 18, 2013 at 10:11:47AM -0300, Fabio Estevam wrote:
> > Shawn Guo wrote:
> > > Since it touches mach-mxs as well, let me send it through arm-soc tree
> > > as fix for 3.9.
> > 
> > Please CC stable as well, so that we can have X11 functional with 3.8.
> 
> Ok, done.

Shawn, I think this patch applies to 3.9-rc but there's one more platform in 
mach-mxs.c in 3.9-rc thus this will have compile issue. Just renaming those two 
FB_SYNC_ values to MXSFB_SYNC_ fixes the compile issue on 3.9-rc. Also the 
commit log doesn't look all that cool .... I probably should have marked the 
patch as RFC.

Shall I send you one for 3.9-rc and one for stable maybe ?

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shawn Guo March 18, 2013, 2:06 p.m. UTC | #6
On Mon, Mar 18, 2013 at 02:58:17PM +0100, Marek Vasut wrote:
> Dear Shawn Guo,
> 
> > On Mon, Mar 18, 2013 at 10:11:47AM -0300, Fabio Estevam wrote:
> > > Shawn Guo wrote:
> > > > Since it touches mach-mxs as well, let me send it through arm-soc tree
> > > > as fix for 3.9.
> > > 
> > > Please CC stable as well, so that we can have X11 functional with 3.8.
> > 
> > Ok, done.
> 
> Shawn, I think this patch applies to 3.9-rc but there's one more platform in 
> mach-mxs.c in 3.9-rc thus this will have compile issue. Just renaming those two 
> FB_SYNC_ values to MXSFB_SYNC_ fixes the compile issue on 3.9-rc.

Yes, I noticed that and fixed it when applying the patch.

> Also the 
> commit log doesn't look all that cool .... I probably should have marked the 
> patch as RFC.
> 
> Shall I send you one for 3.9-rc and one for stable maybe ?

Yeah, improved patch is welcomed.  So I'm dropping the current one and
waiting for the new one.

Shawn

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Vasut March 18, 2013, 2:18 p.m. UTC | #7
Dear Shawn Guo,

> On Mon, Mar 18, 2013 at 02:58:17PM +0100, Marek Vasut wrote:
> > Dear Shawn Guo,
> > 
> > > On Mon, Mar 18, 2013 at 10:11:47AM -0300, Fabio Estevam wrote:
> > > > Shawn Guo wrote:
> > > > > Since it touches mach-mxs as well, let me send it through arm-soc
> > > > > tree as fix for 3.9.
> > > > 
> > > > Please CC stable as well, so that we can have X11 functional with
> > > > 3.8.
> > > 
> > > Ok, done.
> > 
> > Shawn, I think this patch applies to 3.9-rc but there's one more platform
> > in mach-mxs.c in 3.9-rc thus this will have compile issue. Just renaming
> > those two FB_SYNC_ values to MXSFB_SYNC_ fixes the compile issue on
> > 3.9-rc.
> 
> Yes, I noticed that and fixed it when applying the patch.
> 
> > Also the
> > commit log doesn't look all that cool .... I probably should have marked
> > the patch as RFC.
> > 
> > Shall I send you one for 3.9-rc and one for stable maybe ?
> 
> Yeah, improved patch is welcomed.  So I'm dropping the current one and
> waiting for the new one.

Perfect, latest tomorrow then.

Did you manage to test it please?

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shawn Guo March 18, 2013, 2:31 p.m. UTC | #8
On Mon, Mar 18, 2013 at 03:18:13PM +0100, Marek Vasut wrote:
> Did you manage to test it please?

I do not have X11 environment handy to test, but framework console
seems still working.

Shawn

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Vasut March 18, 2013, 3:19 p.m. UTC | #9
Dear Shawn Guo,

> On Mon, Mar 18, 2013 at 03:18:13PM +0100, Marek Vasut wrote:
> > Did you manage to test it please?
> 
> I do not have X11 environment handy to test, but framework console
> seems still working.

Even after me poking in it, lol :-)

You can grab [1], extract it onto card or NFS and just use it as root 
filesystem, the X11 will come up automagically.

[1] ftp://ftp.denx.de/pub/eldk/5.3/targets/armv5te/core-image-sato-sdk-generic-
armv5te.tar.gz

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shawn Guo March 19, 2013, 5:48 a.m. UTC | #10
On Mon, Mar 18, 2013 at 04:19:52PM +0100, Marek Vasut wrote:
> Dear Shawn Guo,
> 
> > On Mon, Mar 18, 2013 at 03:18:13PM +0100, Marek Vasut wrote:
> > > Did you manage to test it please?
> > 
> > I do not have X11 environment handy to test, but framework console
> > seems still working.
> 
> Even after me poking in it, lol :-)
> 
> You can grab [1], extract it onto card or NFS and just use it as root 
> filesystem, the X11 will come up automagically.
> 
Thanks, Marek.  Yeah, now I can see the problem and your patch does fix
it.

Shawn

> [1] ftp://ftp.denx.de/pub/eldk/5.3/targets/armv5te/core-image-sato-sdk-generic-
> armv5te.tar.gz

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Vasut March 19, 2013, 10:33 a.m. UTC | #11
Dear Shawn Guo,

> On Mon, Mar 18, 2013 at 04:19:52PM +0100, Marek Vasut wrote:
> > Dear Shawn Guo,
> > 
> > > On Mon, Mar 18, 2013 at 03:18:13PM +0100, Marek Vasut wrote:
> > > > Did you manage to test it please?
> > > 
> > > I do not have X11 environment handy to test, but framework console
> > > seems still working.
> > 
> > Even after me poking in it, lol :-)
> > 
> > You can grab [1], extract it onto card or NFS and just use it as root
> > filesystem, the X11 will come up automagically.
> 
> Thanks, Marek.  Yeah, now I can see the problem and your patch does fix
> it.

Whew, cool. Can you please apply it and push mainline so I can re-send the one 
for -stable then?

Thank you!

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c
index c66129b..ebf592c 100644
--- a/arch/arm/mach-mxs/mach-mxs.c
+++ b/arch/arm/mach-mxs/mach-mxs.c
@@ -41,8 +41,6 @@  static struct fb_videomode mx23evk_video_modes[] = {
 		.lower_margin	= 4,
 		.hsync_len	= 1,
 		.vsync_len	= 1,
-		.sync		= FB_SYNC_DATA_ENABLE_HIGH_ACT |
-				  FB_SYNC_DOTCLK_FAILING_ACT,
 	},
 };
 
@@ -59,8 +57,6 @@  static struct fb_videomode mx28evk_video_modes[] = {
 		.lower_margin	= 10,
 		.hsync_len	= 10,
 		.vsync_len	= 10,
-		.sync		= FB_SYNC_DATA_ENABLE_HIGH_ACT |
-				  FB_SYNC_DOTCLK_FAILING_ACT,
 	},
 };
 
@@ -77,7 +73,6 @@  static struct fb_videomode m28evk_video_modes[] = {
 		.lower_margin	= 45,
 		.hsync_len	= 1,
 		.vsync_len	= 1,
-		.sync		= FB_SYNC_DATA_ENABLE_HIGH_ACT,
 	},
 };
 
@@ -94,9 +89,7 @@  static struct fb_videomode apx4devkit_video_modes[] = {
 		.lower_margin	= 13,
 		.hsync_len	= 48,
 		.vsync_len	= 3,
-		.sync		= FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT |
-				  FB_SYNC_DATA_ENABLE_HIGH_ACT |
-				  FB_SYNC_DOTCLK_FAILING_ACT,
+		.sync		= FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
 	},
 };
 
@@ -113,9 +106,7 @@  static struct fb_videomode apf28dev_video_modes[] = {
 		.lower_margin = 0x15,
 		.hsync_len = 64,
 		.vsync_len = 4,
-		.sync = FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT |
-				FB_SYNC_DATA_ENABLE_HIGH_ACT |
-				FB_SYNC_DOTCLK_FAILING_ACT,
+		.sync = FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
 	},
 };
 
@@ -250,6 +241,8 @@  static void __init imx23_evk_init(void)
 	mxsfb_pdata.mode_count = ARRAY_SIZE(mx23evk_video_modes);
 	mxsfb_pdata.default_bpp = 32;
 	mxsfb_pdata.ld_intf_width = STMLCDIF_24BIT;
+	mxsfb_pdata.sync = MXSFB_SYNC_DATA_ENABLE_HIGH_ACT |
+				MXSFB_SYNC_DOTCLK_FAILING_ACT;
 }
 
 static inline void enable_clk_enet_out(void)
@@ -269,6 +262,8 @@  static void __init imx28_evk_init(void)
 	mxsfb_pdata.mode_count = ARRAY_SIZE(mx28evk_video_modes);
 	mxsfb_pdata.default_bpp = 32;
 	mxsfb_pdata.ld_intf_width = STMLCDIF_24BIT;
+	mxsfb_pdata.sync = MXSFB_SYNC_DATA_ENABLE_HIGH_ACT |
+				MXSFB_SYNC_DOTCLK_FAILING_ACT;
 
 	mxs_saif_clkmux_select(MXS_DIGCTL_SAIF_CLKMUX_EXTMSTR0);
 }
@@ -288,6 +283,7 @@  static void __init m28evk_init(void)
 	mxsfb_pdata.mode_count = ARRAY_SIZE(m28evk_video_modes);
 	mxsfb_pdata.default_bpp = 16;
 	mxsfb_pdata.ld_intf_width = STMLCDIF_18BIT;
+	mxsfb_pdata.sync = MXSFB_SYNC_DATA_ENABLE_HIGH_ACT;
 }
 
 static void __init sc_sps1_init(void)
@@ -313,6 +309,8 @@  static void __init apx4devkit_init(void)
 	mxsfb_pdata.mode_count = ARRAY_SIZE(apx4devkit_video_modes);
 	mxsfb_pdata.default_bpp = 32;
 	mxsfb_pdata.ld_intf_width = STMLCDIF_24BIT;
+	mxsfb_pdata.sync = MXSFB_SYNC_DATA_ENABLE_HIGH_ACT |
+				MXSFB_SYNC_DOTCLK_FAILING_ACT;
 }
 
 #define ENET0_MDC__GPIO_4_0	MXS_GPIO_NR(4, 0)
@@ -403,6 +401,8 @@  static void __init apf28_init(void)
 	mxsfb_pdata.mode_count = ARRAY_SIZE(apf28dev_video_modes);
 	mxsfb_pdata.default_bpp = 16;
 	mxsfb_pdata.ld_intf_width = STMLCDIF_16BIT;
+	mxsfb_pdata.sync = MXSFB_SYNC_DATA_ENABLE_HIGH_ACT |
+				MXSFB_SYNC_DOTCLK_FAILING_ACT;
 }
 
 static void __init mxs_machine_init(void)
diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
index 755556c..45169cb 100644
--- a/drivers/video/mxsfb.c
+++ b/drivers/video/mxsfb.c
@@ -169,6 +169,7 @@  struct mxsfb_info {
 	unsigned dotclk_delay;
 	const struct mxsfb_devdata *devdata;
 	int mapped;
+	u32 sync;
 };
 
 #define mxsfb_is_v3(host) (host->devdata->ipversion == 3)
@@ -456,9 +457,9 @@  static int mxsfb_set_par(struct fb_info *fb_info)
 		vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH;
 	if (fb_info->var.sync & FB_SYNC_VERT_HIGH_ACT)
 		vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH;
-	if (fb_info->var.sync & FB_SYNC_DATA_ENABLE_HIGH_ACT)
+	if (host->sync & MXSFB_SYNC_DATA_ENABLE_HIGH_ACT)
 		vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH;
-	if (fb_info->var.sync & FB_SYNC_DOTCLK_FAILING_ACT)
+	if (host->sync & MXSFB_SYNC_DOTCLK_FAILING_ACT)
 		vdctrl0 |= VDCTRL0_DOTCLK_ACT_FAILING;
 
 	writel(vdctrl0, host->base + LCDC_VDCTRL0);
@@ -861,6 +862,8 @@  static int mxsfb_probe(struct platform_device *pdev)
 
 	INIT_LIST_HEAD(&fb_info->modelist);
 
+	host->sync = pdata->sync;
+
 	ret = mxsfb_init_fbinfo(host);
 	if (ret != 0)
 		goto error_init_fb;
diff --git a/include/linux/mxsfb.h b/include/linux/mxsfb.h
index f14943d..f80af86 100644
--- a/include/linux/mxsfb.h
+++ b/include/linux/mxsfb.h
@@ -24,8 +24,8 @@ 
 #define STMLCDIF_18BIT 2 /** pixel data bus to the display is of 18 bit width */
 #define STMLCDIF_24BIT 3 /** pixel data bus to the display is of 24 bit width */
 
-#define FB_SYNC_DATA_ENABLE_HIGH_ACT	(1 << 6)
-#define FB_SYNC_DOTCLK_FAILING_ACT	(1 << 7) /* failing/negtive edge sampling */
+#define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT	(1 << 6)
+#define MXSFB_SYNC_DOTCLK_FAILING_ACT	(1 << 7) /* failing/negtive edge sampling */
 
 struct mxsfb_platform_data {
 	struct fb_videomode *mode_list;
@@ -44,6 +44,9 @@  struct mxsfb_platform_data {
 				 * allocated. If specified,fb_size must also be specified.
 				 * fb_phys must be unused by Linux.
 				 */
+	u32 sync;		/* sync mask, contains MXSFB specifics not
+				 * carried in fb_info->var.sync
+				 */
 };
 
 #endif /* __LINUX_MXSFB_H */