[U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword

[Ryan Harkin]

On 21 October 2015 at 13:49, Ryan Harkin <ryan.harkin at linaro.org> wrote:
> On 21 October 2015 at 13:40, Stefan Roese <sr at denx.de> wrote:
>> Hi Ryan,
>>
>> On 21.10.2015 13:34, Ryan Harkin wrote:
>>>
>>> On 21 October 2015 at 11:24, Stefan Roese <sr at 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".
>>
>>
>> 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 <ryan.harkin at 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;
>>>>>    } 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 <sr at denx.de>
>>
>> to this version then.
>
>
> Thanks,
> Ryan.