All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space
@ 2010-04-07  6:51 Dave Liu
  2010-04-07 14:35 ` Timur Tabi
  0 siblings, 1 reply; 11+ messages in thread
From: Dave Liu @ 2010-04-07  6:51 UTC (permalink / raw)
  To: u-boot

From: Jerry Huang <Chang-Ming.Huang@freescale.com>

Using PPC I/O accessor to DIU I/O space instead of directly
read/write. It will prevent the dozen of compiler order issue
and PPC hardware order issue for accessing I/O space.

Using the toolchain(tc-fsl-x86lnx-e500-dp-4.3.74-2.i386.rpm)
can show up the order issue of DIU driver.

Signed-off-by: Dave Liu <daveliu@freescale.com>
Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
---
 board/freescale/common/fsl_diu_fb.c |   55 ++++++++++++++++++-----------------
 1 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/board/freescale/common/fsl_diu_fb.c b/board/freescale/common/fsl_diu_fb.c
index 2fc878b..36c5d3d 100644
--- a/board/freescale/common/fsl_diu_fb.c
+++ b/board/freescale/common/fsl_diu_fb.c
@@ -1,5 +1,5 @@
 /*
- * Copyright 2007 Freescale Semiconductor, Inc.
+ * Copyright 2007, 2010 Freescale Semiconductor, Inc.
  * York Sun <yorksun@freescale.com>
  *
  * FSL DIU Framebuffer driver
@@ -26,6 +26,7 @@
 #include <common.h>
 #include <i2c.h>
 #include <malloc.h>
+#include <asm/io.h>
 
 #include "fsl_diu_fb.h"
 
@@ -246,9 +247,9 @@ int fsl_diu_init(int xres,
 
 	memset(info->screen_base, 0, info->smem_len);
 
-	dr.diu_reg->desc[0] = (unsigned int) &dummy_ad;
-	dr.diu_reg->desc[1] = (unsigned int) &dummy_ad;
-	dr.diu_reg->desc[2] = (unsigned int) &dummy_ad;
+	out_be32(&dr.diu_reg->desc[0], &dummy_ad);
+	out_be32(&dr.diu_reg->desc[1], &dummy_ad);
+	out_be32(&dr.diu_reg->desc[2], &dummy_ad);
 	debug("dummy dr.diu_reg->desc[0] = 0x%x\n", dr.diu_reg->desc[0]);
 	debug("dummy desc[0] = 0x%x\n", hw->desc[0]);
 
@@ -310,26 +311,26 @@ int fsl_diu_init(int xres,
 
 	/* Program DIU registers */
 
-	hw->gamma = (unsigned int) gamma.paddr;
-	hw->cursor= (unsigned int) cursor.paddr;
-	hw->bgnd = 0x007F7F7F;				/* BGND */
-	hw->bgnd_wb = 0;				/* BGND_WB */
-	hw->disp_size = var->yres << 16 | var->xres;	/* DISP SIZE */
-	hw->wb_size = 0;				/* WB SIZE */
-	hw->wb_mem_addr = 0;				/* WB MEM ADDR */
-	hw->hsyn_para = var->left_margin << 22 |	/* BP_H */
+	out_be32(&hw->gamma, gamma.paddr);
+	out_be32(&hw->cursor, cursor.paddr);
+	out_be32(&hw->bgnd, 0x007F7F7F);
+	out_be32(&hw->bgnd_wb, 0);				/* BGND_WB */
+	out_be32(&hw->disp_size, var->yres << 16 | var->xres);	/* DISP SIZE */
+	out_be32(&hw->wb_size, 0);				/* WB SIZE */
+	out_be32(&hw->wb_mem_addr, 0);				/* WB MEM ADDR */
+	out_be32(&hw->hsyn_para, var->left_margin << 22 |	/* BP_H */
 			var->hsync_len << 11   |	/* PW_H */
-			var->right_margin;		/* FP_H */
-	hw->vsyn_para = var->upper_margin << 22 |	/* BP_V */
-			var->vsync_len << 11    |	/* PW_V  */
-			var->lower_margin;		/* FP_V  */
+			var->right_margin);		/* FP_H */
 
-	hw->syn_pol = 0;			/* SYNC SIGNALS POLARITY */
-	hw->thresholds = 0x00037800;		/* The Thresholds */
-	hw->int_status = 0;			/* INTERRUPT STATUS */
-	hw->int_mask = 0;			/* INT MASK */
-	hw->plut = 0x01F5F666;
+	out_be32(&hw->vsyn_para, var->upper_margin << 22 |	/* BP_V */
+			var->vsync_len << 11    |	/* PW_V  */
+			var->lower_margin);		/* FP_V  */
 
+	out_be32(&hw->syn_pol, 0);			/* SYNC SIGNALS POLARITY */
+	out_be32(&hw->thresholds, 0x00037800);		/* The Thresholds */
+	out_be32(&hw->int_status, 0);			/* INTERRUPT STATUS */
+	out_be32(&hw->int_mask, 0);			/* INT MASK */
+	out_be32(&hw->plut, 0x01F5F666);
 	/* Pixel Clock configuration */
 	debug("DIU pixclock in ps - %d\n", var->pixclock);
 	diu_set_pixel_clock(var->pixclock);
@@ -369,8 +370,8 @@ static int fsl_diu_enable_panel(struct fb_info *info)
 	struct diu_ad *ad = &fsl_diu_fb_ad;
 
 	debug("Entered: enable_panel\n");
-	if (hw->desc[0] != (unsigned int)ad)
-		hw->desc[0] = (unsigned int)ad;
+	if (in_be32(&hw->desc[0]) != (unsigned int)ad)
+		out_be32(&hw->desc[0], ad);
 	debug("desc[0] = 0x%x\n", hw->desc[0]);
 	return 0;
 }
@@ -380,8 +381,8 @@ static int fsl_diu_disable_panel(struct fb_info *info)
 	struct diu *hw = dr.diu_reg;
 
 	debug("Entered: disable_panel\n");
-	if (hw->desc[0] != (unsigned int)&dummy_ad)
-		hw->desc[0] = (unsigned int)&dummy_ad;
+	if (in_be32(&hw->desc[0]) != (unsigned int)&dummy_ad)
+		out_be32(&hw->desc[0], &dummy_ad);
 	return 0;
 }
 
@@ -422,7 +423,7 @@ static void enable_lcdc(void)
 
 	debug("Entered: enable_lcdc, fb_enabled = %d\n", fb_enabled);
 	if (!fb_enabled) {
-		hw->diu_mode = dr.mode;
+		out_be32(&hw->diu_mode, dr.mode);
 		fb_enabled++;
 	}
 	debug("diu_mode = %d\n", hw->diu_mode);
@@ -434,7 +435,7 @@ static void disable_lcdc(void)
 
 	debug("Entered: disable_lcdc, fb_enabled = %d\n", fb_enabled);
 	if (fb_enabled) {
-		hw->diu_mode = 0;
+		out_be32(&hw->diu_mode, 0);
 		fb_enabled = 0;
 	}
 }
-- 
1.6.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space
  2010-04-07  6:51 [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space Dave Liu
@ 2010-04-07 14:35 ` Timur Tabi
  2010-04-08  6:14   ` Liu Dave-R63238
  0 siblings, 1 reply; 11+ messages in thread
From: Timur Tabi @ 2010-04-07 14:35 UTC (permalink / raw)
  To: u-boot

Dave Liu wrote:

> @@ -369,8 +370,8 @@ static int fsl_diu_enable_panel(struct fb_info *info)
>  	struct diu_ad *ad = &fsl_diu_fb_ad;
>
>  	debug("Entered: enable_panel\n");
> -	if (hw->desc[0] != (unsigned int)ad)
> -		hw->desc[0] = (unsigned int)ad;
> +	if (in_be32(&hw->desc[0]) != (unsigned int)ad)
> +		out_be32(&hw->desc[0], ad);

'ad' should be cast to a u32, not an "unsigned int".  You shouldn't
compare a sized type with an unsized type.

>  	debug("desc[0] = 0x%x\n", hw->desc[0]);
>  	return 0;
>  }
> @@ -380,8 +381,8 @@ static int fsl_diu_disable_panel(struct fb_info *info)
>  	struct diu *hw = dr.diu_reg;
>
>  	debug("Entered: disable_panel\n");
> -	if (hw->desc[0] != (unsigned int)&dummy_ad)
> -		hw->desc[0] = (unsigned int)&dummy_ad;
> +	if (in_be32(&hw->desc[0]) != (unsigned int)&dummy_ad)
> +		out_be32(&hw->desc[0], &dummy_ad);

Same thing here.

-- 
Timur Tabi
Linux kernel developer at Freescale

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space
  2010-04-07 14:35 ` Timur Tabi
@ 2010-04-08  6:14   ` Liu Dave-R63238
  2010-04-08  7:27     ` Kumar Gala
  2010-04-08 10:11     ` Wolfgang Denk
  0 siblings, 2 replies; 11+ messages in thread
From: Liu Dave-R63238 @ 2010-04-08  6:14 UTC (permalink / raw)
  To: u-boot

> > @@ -369,8 +370,8 @@ static int fsl_diu_enable_panel(struct 
> fb_info *info)
> >  	struct diu_ad *ad = &fsl_diu_fb_ad;
> >
> >  	debug("Entered: enable_panel\n");
> > -	if (hw->desc[0] != (unsigned int)ad)
> > -		hw->desc[0] = (unsigned int)ad;
> > +	if (in_be32(&hw->desc[0]) != (unsigned int)ad)
> > +		out_be32(&hw->desc[0], ad);
 
> 'ad' should be cast to a u32, not an "unsigned int".  You 
> shouldn't compare a sized type with an unsized type.

Grep the include/asm-ppc/types.h, I got typedef unsigned int u32;
The u32 is same as "unsigned int".

Kumar, any comments?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space
  2010-04-08  6:14   ` Liu Dave-R63238
@ 2010-04-08  7:27     ` Kumar Gala
  2010-04-08  7:32       ` Liu Dave-R63238
  2010-04-08 17:15       ` Scott Wood
  2010-04-08 10:11     ` Wolfgang Denk
  1 sibling, 2 replies; 11+ messages in thread
From: Kumar Gala @ 2010-04-08  7:27 UTC (permalink / raw)
  To: u-boot


On Apr 8, 2010, at 1:14 AM, Liu Dave-R63238 wrote:

>>> @@ -369,8 +370,8 @@ static int fsl_diu_enable_panel(struct 
>> fb_info *info)
>>> 	struct diu_ad *ad = &fsl_diu_fb_ad;
>>> 
>>> 	debug("Entered: enable_panel\n");
>>> -	if (hw->desc[0] != (unsigned int)ad)
>>> -		hw->desc[0] = (unsigned int)ad;
>>> +	if (in_be32(&hw->desc[0]) != (unsigned int)ad)
>>> +		out_be32(&hw->desc[0], ad);
> 
>> 'ad' should be cast to a u32, not an "unsigned int".  You 
>> shouldn't compare a sized type with an unsized type.
> 
> Grep the include/asm-ppc/types.h, I got typedef unsigned int u32;
> The u32 is same as "unsigned int".
> 
> Kumar, any comments?

Timur is correct, while u32 is typedef to 'unsigned int' its cleaner in the code to use u32 and if we have a 64-bit port of u-boot on PPC in the future it makes things cleaner.

- k

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space
  2010-04-08  7:27     ` Kumar Gala
@ 2010-04-08  7:32       ` Liu Dave-R63238
  2010-04-08  7:58         ` Kumar Gala
  2010-04-08 17:15       ` Scott Wood
  1 sibling, 1 reply; 11+ messages in thread
From: Liu Dave-R63238 @ 2010-04-08  7:32 UTC (permalink / raw)
  To: u-boot

> >>> -	if (hw->desc[0] != (unsigned int)ad)
> >>> -		hw->desc[0] = (unsigned int)ad;
> >>> +	if (in_be32(&hw->desc[0]) != (unsigned int)ad)
> >>> +		out_be32(&hw->desc[0], ad);
> > 
> >> 'ad' should be cast to a u32, not an "unsigned int".  You 
> >> shouldn't compare a sized type with an unsized type.
> > 
> > Grep the include/asm-ppc/types.h, I got typedef unsigned int u32;
> > The u32 is same as "unsigned int".
> > 
> > Kumar, any comments?
> 
> Timur is correct, while u32 is typedef to 'unsigned int' its 
> cleaner in the code to use u32 and if we have a 64-bit port 
> of u-boot on PPC in the future it makes things cleaner.

So, I need to change the (unsigned int)ad to (u32)ad?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space
  2010-04-08  7:32       ` Liu Dave-R63238
@ 2010-04-08  7:58         ` Kumar Gala
  0 siblings, 0 replies; 11+ messages in thread
From: Kumar Gala @ 2010-04-08  7:58 UTC (permalink / raw)
  To: u-boot


On Apr 8, 2010, at 2:32 AM, Liu Dave-R63238 wrote:

>>>>> -	if (hw->desc[0] != (unsigned int)ad)
>>>>> -		hw->desc[0] = (unsigned int)ad;
>>>>> +	if (in_be32(&hw->desc[0]) != (unsigned int)ad)
>>>>> +		out_be32(&hw->desc[0], ad);
>>> 
>>>> 'ad' should be cast to a u32, not an "unsigned int".  You 
>>>> shouldn't compare a sized type with an unsized type.
>>> 
>>> Grep the include/asm-ppc/types.h, I got typedef unsigned int u32;
>>> The u32 is same as "unsigned int".
>>> 
>>> Kumar, any comments?
>> 
>> Timur is correct, while u32 is typedef to 'unsigned int' its 
>> cleaner in the code to use u32 and if we have a 64-bit port 
>> of u-boot on PPC in the future it makes things cleaner.
> 
> So, I need to change the (unsigned int)ad to (u32)ad?

yes, please do.  Otherwise patch looked good.

- k

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space
  2010-04-08  6:14   ` Liu Dave-R63238
  2010-04-08  7:27     ` Kumar Gala
@ 2010-04-08 10:11     ` Wolfgang Denk
  2010-04-08 15:50       ` Kumar Gala
  1 sibling, 1 reply; 11+ messages in thread
From: Wolfgang Denk @ 2010-04-08 10:11 UTC (permalink / raw)
  To: u-boot

Dear "Liu Dave-R63238",

In message <D7CCA83BB0796C49BC0BB53B6AB12089A9C4E3@zch01exm21.fsl.freescale.net> you wrote:
>
> > 'ad' should be cast to a u32, not an "unsigned int".  You 
> > shouldn't compare a sized type with an unsized type.
...

> The u32 is same as "unsigned int".

Maybe, maybe not. If you want to be safe, then compare u32 with u32
only.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
When you say "I wrote a program that crashed  Windows",  people  just
stare  at you blankly and say "Hey, I got those with the system, *for
free*".        - Linus Torvalds in <3itc77$9lj@ninurta.fer.uni-lj.si>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space
  2010-04-08 10:11     ` Wolfgang Denk
@ 2010-04-08 15:50       ` Kumar Gala
  2010-04-08 16:19         ` Timur Tabi
  0 siblings, 1 reply; 11+ messages in thread
From: Kumar Gala @ 2010-04-08 15:50 UTC (permalink / raw)
  To: u-boot


On Apr 8, 2010, at 5:11 AM, Wolfgang Denk wrote:

> Dear "Liu Dave-R63238",
> 
> In message <D7CCA83BB0796C49BC0BB53B6AB12089A9C4E3@zch01exm21.fsl.freescale.net> you wrote:
>> 
>>> 'ad' should be cast to a u32, not an "unsigned int".  You 
>>> shouldn't compare a sized type with an unsized type.
> ...
> 
>> The u32 is same as "unsigned int".
> 
> Maybe, maybe not. If you want to be safe, then compare u32 with u32
> only.

Good point, I wasn't look at what the src info was.  We should probably fix the struct diu_ad, diu to use 'u32' etc.

- k

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space
  2010-04-08 15:50       ` Kumar Gala
@ 2010-04-08 16:19         ` Timur Tabi
  2010-04-10 20:24           ` Wolfgang Denk
  0 siblings, 1 reply; 11+ messages in thread
From: Timur Tabi @ 2010-04-08 16:19 UTC (permalink / raw)
  To: u-boot

On Thu, Apr 8, 2010 at 10:50 AM, Kumar Gala <galak@kernel.crashing.org> wrote:

> Good point, I wasn't look at what the src info was. ?We should probably fix the struct diu_ad, diu to use 'u32' etc.

Or __be32.  I think the consensus is that struct fields for hardware
registers should use endian-specific sized integers.

-- 
Timur Tabi
Linux kernel developer at Freescale

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space
  2010-04-08  7:27     ` Kumar Gala
  2010-04-08  7:32       ` Liu Dave-R63238
@ 2010-04-08 17:15       ` Scott Wood
  1 sibling, 0 replies; 11+ messages in thread
From: Scott Wood @ 2010-04-08 17:15 UTC (permalink / raw)
  To: u-boot

On Thu, Apr 08, 2010 at 02:27:01AM -0500, Kumar Gala wrote:
> 
> On Apr 8, 2010, at 1:14 AM, Liu Dave-R63238 wrote:
> 
> >>> @@ -369,8 +370,8 @@ static int fsl_diu_enable_panel(struct 
> >> fb_info *info)
> >>> 	struct diu_ad *ad = &fsl_diu_fb_ad;
> >>> 
> >>> 	debug("Entered: enable_panel\n");
> >>> -	if (hw->desc[0] != (unsigned int)ad)
> >>> -		hw->desc[0] = (unsigned int)ad;
> >>> +	if (in_be32(&hw->desc[0]) != (unsigned int)ad)
> >>> +		out_be32(&hw->desc[0], ad);
> > 
> >> 'ad' should be cast to a u32, not an "unsigned int".  You 
> >> shouldn't compare a sized type with an unsized type.
> > 
> > Grep the include/asm-ppc/types.h, I got typedef unsigned int u32;
> > The u32 is same as "unsigned int".
> > 
> > Kumar, any comments?
> 
> Timur is correct, while u32 is typedef to 'unsigned int' its cleaner in
> the code to use u32 and if we have a 64-bit port of u-boot on PPC in the
> future it makes things cleaner.

If we have a 64-bit port of u-boot, then casting a pointer to u32 will still
be broken...

-Scott

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space
  2010-04-08 16:19         ` Timur Tabi
@ 2010-04-10 20:24           ` Wolfgang Denk
  0 siblings, 0 replies; 11+ messages in thread
From: Wolfgang Denk @ 2010-04-10 20:24 UTC (permalink / raw)
  To: u-boot

Dear Timur Tabi,

In message <i2xed82fe3e1004080919p5e6a472ek388dbf21a3bd899f@mail.gmail.com> you wrote:
> On Thu, Apr 8, 2010 at 10:50 AM, Kumar Gala <galak@kernel.crashing.org> wro=
> te:
> 
> > Good point, I wasn't look at what the src info was. =A0We should probably=
>  fix the struct diu_ad, diu to use 'u32' etc.
> 
> Or __be32.  I think the consensus is that struct fields for hardware
> registers should use endian-specific sized integers.

<sarcasm>
Is there such consensus?  Does this include chip designers?

I mean, can we be sure that __be32 will also work if this IP core
shows up in a little-endian ARM system?
</sarcasm>

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Our business is run on trust.  We trust you will pay in advance.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2010-04-10 20:24 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-07  6:51 [U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space Dave Liu
2010-04-07 14:35 ` Timur Tabi
2010-04-08  6:14   ` Liu Dave-R63238
2010-04-08  7:27     ` Kumar Gala
2010-04-08  7:32       ` Liu Dave-R63238
2010-04-08  7:58         ` Kumar Gala
2010-04-08 17:15       ` Scott Wood
2010-04-08 10:11     ` Wolfgang Denk
2010-04-08 15:50       ` Kumar Gala
2010-04-08 16:19         ` Timur Tabi
2010-04-10 20:24           ` Wolfgang Denk

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.