fsl-diu: Using I/O accessor to CCSR space
PATCH 1/1 fsl-diu: Using I/O accessor to CCSR space
From: Jerry Huang <[email protected]>
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.
--- board/freescale/common/fsl_diu_fb.c | 55 ++++++++++++++++++----------------- 1 files changed, 28 insertions(+), 27 deletions(-)
board/freescale/common/fsl_diu_fb.c +28 -27
@@ -1,5 +1,5 @@/* -* Copyright 2007 Freescale Semiconductor, Inc. +* Copyright 2007, 2010 Freescale Semiconductor, Inc. * York Sun <[email protected]> * * 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 ; } }
Signature · 1 line
1.6.4
PATCH 1/1 fsl-diu: Using I/O accessor to CCSR space
board/freescale/common/fsl_diu_fb.c static int fsl_diu_enable_panel(struct fb_info *info) · line 374
Quoted 8 lines · Dave Liu
board/freescale/common/fsl_diu_fb.c +2
-2@@ -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.
board/freescale/common/fsl_diu_fb.c static int fsl_diu_disable_panel(struct fb_info *info) · line 385
Quoted 11 lines
board/freescale/common/fsl_diu_fb.c +2
-2debug ("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.
Signature · 2 lines
Timur Tabi Linux kernel developer at Freescale
PATCH 1/1 fsl-diu: Using I/O accessor to CCSR space
Quoted 4 lines
Review static int fsl_diu_enable_panel(struct
Quoted 1 line
(quoted diff) +0
-0@@ -369,8 +370,8 @@ static int fsl_diu_enable_panel(struct
fb_info*info)Quoted 2 lines
board/freescale/common/fsl_diu_fb.c +0
-0struct diu_ad *ad = &fsl_diu_fb_ad;
Quoted 8 lines
board/freescale/common/fsl_diu_fb.c static int fsl_diu_enable_panel(struct fb_info *info) · line 374
Quoted 5 lines
board/freescale/common/fsl_diu_fb.c +2
-2debug ("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?
PATCH 1/1 fsl-diu: Using I/O accessor to CCSR space
Quoted 13 lines · On Apr 8, 2010, at 1:14 AM, Liu Dave-R63238 wrote:
Quoted 4 lines
Review static int fsl_diu_enable_panel(struct
Quoted 1 line
(quoted diff) +0
-0@@ -369,8 +370,8 @@ static int fsl_diu_enable_panel(struct
fb_info*info)Quoted 2 lines
board/freescale/common/fsl_diu_fb.c +0
-0struct diu_ad *ad = &fsl_diu_fb_ad;Quoted 8 lines
board/freescale/common/fsl_diu_fb.c static int fsl_diu_enable_panel(struct fb_info *info) · line 374
Quoted 5 lines
board/freescale/common/fsl_diu_fb.c +2
-2debug ("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.
Quoted 4 lines
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
PATCH 1/1 fsl-diu: Using I/O accessor to CCSR space
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
board/freescale/common/fsl_diu_fb.c static int fsl_diu_enable_panel(struct fb_info *info) · line 374
Quoted 4 lines
board/freescale/common/fsl_diu_fb.c +2
-2-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.
Quoted 8 lines
Quoted 4 lines
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?
PATCH 1/1 fsl-diu: Using I/O accessor to CCSR space
Quoted 11 lines · On Apr 8, 2010, at 2:32 AM, Liu Dave-R63238 wrote:
Quoted 11 lines
Quoted 11 lines
Quoted 7 lines
board/freescale/common/fsl_diu_fb.c static int fsl_diu_enable_panel(struct fb_info *info) · line 374
Quoted 4 lines
board/freescale/common/fsl_diu_fb.c +2
-2-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".
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
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
PATCH 1/1 fsl-diu: Using I/O accessor to CCSR space
In reply to Kumar GalaQuoted 16 lines · Kumar Gala
Quoted 13 lines · On Apr 8, 2010, at 1:14 AM, Liu Dave-R63238 wrote:
Quoted 12 lines
Review static int fsl_diu_enable_panel(struct
Quoted 1 line
(quoted diff) +0
-0@@ -369,8 +370,8 @@ static int fsl_diu_enable_panel(struct
fb_info*info)board/freescale/common/fsl_diu_fb.c static int fsl_diu_enable_panel(struct fb_info *info) · line 374
Quoted 7 lines
board/freescale/common/fsl_diu_fb.c +2
-2struct 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.
Quoted 8 lines
Quoted 4 lines
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
PATCH 1/1 fsl-diu: Using I/O accessor to CCSR space
In reply to Liu Dave-R63238Dear "Liu Dave-R63238",
Quoted 3 lines · In message <D7CCA83BB0796C49BC0BB53B6AB12089A9C4E3@zch01exm21.fsl.freescale.net…
Quoted 3 lines
'ad' should be cast to a u32, not an "unsigned int". You shouldn't compare a sized type with an unsized type.
...
Quoted 1 line
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
Signature · 6 lines
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 <[email protected]>
PATCH 1/1 fsl-diu: Using I/O accessor to CCSR space
Quoted 4 lines · Wolfgang Denk
Dear "Liu Dave-R63238",
Quoted 1 line · In message <D7CCA83BB0796C49BC0BB53B6AB12089A9C4E3@zch01exm21.fsl.freescale.net…
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
'ad' should be cast to a u32, not an "unsigned int". You shouldn't compare a sized type with an unsized type.
...
Quoted 1 line
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
PATCH 1/1 fsl-diu: Using I/O accessor to CCSR space
Quoted 1 line · Kumar Gala
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.
Signature · 2 lines
Timur Tabi Linux kernel developer at Freescale
PATCH 1/1 fsl-diu: Using I/O accessor to CCSR space
Dear Timur Tabi,
Quoted 8 lines · In message <[email protected]>…
On Thu, Apr 8, 2010 at 10:50 AM, Kumar Gala <[email protected]> wro= te:
Quoted 1 line
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
Signature · 4 lines
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.