Hi Stefan, On 21 October 2015 at 11:24, Stefan Roese <[email protected]> wrote: > Hi Ryan, > > On 21.10.2015 11:54, Ryan Harkin wrote: >> >> cword.l is an unsigned long int, but it is intended to be 32 bits long. >> >> Unfortunately, it's 64-bits long on a 64-bit system, meaning that a >> 64-bit system cannot use 32-bit wide CFI flash parts. >> >> Similar problems also exist with the other cword sizes. >> >> Also, this patch introduced compiler warnings: >> >> drivers/mtd/cfi_flash.c: In function 'flash_write_cmd': >> drivers/mtd/cfi_flash.c:348:3: warning: format '%lx' expects argument of >> type 'long unsigned int', but argument 4 has type '__u32' [-Wformat=] >> debug ("fwc addr %p cmd %x %8.8lx 32bit x %d bit\n", addr, >> ^ >> drivers/mtd/cfi_flash.c: In function 'flash_isequal': >> drivers/mtd/cfi_flash.c:404:3: warning: format '%lx' expects argument of >> type 'long unsigned int', but argument 3 has type '__u32' [-Wformat=] >> debug ("is= %8.8x %8.8lx\n", flash_read32(addr), cword.l); >> ^ >> >> I masked these warnings in this patch, however, I am quite sure this is >> the wrong approach. > > > Why do you think this is the wrong approach?
That comment was more referring to the change from %lx to %x because I moved from "unsigned long" to "u32". > Changing from "long" to > "u32" seems correct to me. Ok, good. Perhaps also the "c", "w", "l" and "ll" in "cword" should be renamed to "u8", "u16", "u32" and "u64"? We aren't actually looking for a "char" in this file, but an 8 bit value, for example. > > >> Signed-off-by: Ryan Harkin <[email protected]> >> --- >> drivers/mtd/cfi_flash.c | 4 ++-- >> include/mtd/cfi_flash.h | 8 ++++---- >> 2 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c >> index 50983b8..cee85a9 100644 >> --- a/drivers/mtd/cfi_flash.c >> +++ b/drivers/mtd/cfi_flash.c >> @@ -345,7 +345,7 @@ void flash_write_cmd (flash_info_t * info, >> flash_sect_t sect, >> flash_write16(cword.w, addr); >> break; >> case FLASH_CFI_32BIT: >> - debug ("fwc addr %p cmd %x %8.8lx 32bit x %d bit\n", addr, >> + debug ("fwc addr %p cmd %x %8.8x 32bit x %d bit\n", addr, >> cmd, cword.l, >> info->chipwidth << CFI_FLASH_SHIFT_WIDTH); >> flash_write32(cword.l, addr); >> @@ -401,7 +401,7 @@ static int flash_isequal (flash_info_t * info, >> flash_sect_t sect, >> retval = (flash_read16(addr) == cword.w); >> break; >> case FLASH_CFI_32BIT: >> - debug ("is= %8.8x %8.8lx\n", flash_read32(addr), cword.l); >> + debug ("is= %8.8x %8.8x\n", flash_read32(addr), cword.l); >> retval = (flash_read32(addr) == cword.l); >> break; >> case FLASH_CFI_64BIT: >> diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h >> index 048b477..cd30619 100644 >> --- a/include/mtd/cfi_flash.h >> +++ b/include/mtd/cfi_flash.h >> @@ -105,10 +105,10 @@ >> #define NUM_ERASE_REGIONS 4 /* max. number of erase regions */ >> >> typedef union { >> - unsigned char c; >> - unsigned short w; >> - unsigned long l; >> - unsigned long long ll; >> + __u8 c; >> + __u16 w; >> + __u32 l; >> + __u64 ll; >> } cfiword_t; > > > Please use the "uX" types and not the "__uX" types here. Those are > already widely used in this header. Yes, I'll make that change for v2. Thanks, Ryan. > > Thanks, > Stefan > _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
