[U-Boot] [PATCH] fsl-diu: Using I/O accessor to CCSR space

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; } }

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.

@@ -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?

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

- 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?

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

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

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

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

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.

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
participants (6)
-
Dave Liu
-
Kumar Gala
-
Liu Dave-R63238
-
Scott Wood
-
Timur Tabi
-
Wolfgang Denk