On 21 October 2015 at 13:49, Ryan Harkin <[email protected]> wrote: > On 21 October 2015 at 13:40, Stefan Roese <[email protected]> wrote: >> Hi Ryan, >> >> On 21.10.2015 13:34, Ryan Harkin wrote: >>> >>> 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". >> >> >> Looks okay to me. Please make sure that you don't see any warning >> when compiling this new code for 32bit and 64bit platforms. > > Good point, I haven't built for a 32bit platform with this patch. > >> >>>> 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. >> >> >> Correct. But I suspect that u8 etc will not be possible as variable >> names. As they are already taken. The current names are not perfect >> but still not that bad. Feel free to come up with some better names >> that match the meaning of the variables more closely in v2... > > Yeah, I just noticed that problem too. I'll think of an appropriate > new name. For now, I've just used w8, w16, w32 and w64, meaning > "width 8", etc., but that might not be very clear either.
I'm actually liking the "w" names now. Let me know if it doesn't suit. Now for a separate but related issue, so I thought I'd ask about it here. Looking through the code to make this rename, I noticed a few u32 pointers, eg: 1056: u32 *flash; 1063: flash = (u32 *)info->start[sect]; 1145: u32 *flash; 1151: flash = (u32 *)info->start[i]; Perhaps I'm lucky that my flash part lives in the 32-bit address range, but I think this may need fixing too - with a separate patch. That took me to looking at the definition of flash_info_t in include/flash.h and I see there are quite a few "uchar", "ushort" and "ulong"s in there. Do you think these will need changing too? > >> >> >>>> >>>> >>>>> 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. You can add my: >> >> Acked-by: Stefan Roese <[email protected]> >> >> to this version then. > > > Thanks, > Ryan. _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
