Hi Stefan,
On 21 October 2015 at 11:24, Stefan Roese sr@denx.de 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 ryan.harkin@linaro.org
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;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