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

[Ryan Harkin]

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.

>
>
>>>
>>>
>>>> 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.