| Message ID | 1445421247-15013-2-git-send-email-ryan.harkin@linaro.org |
|---|---|
| State | New |
| Headers | show |
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? Changing from "long" to "u32" seems correct to me. > 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; > } cfiword_t; Please use the "uX" types and not the "__uX" types here. Those are already widely used in this header. Thanks, Stefan
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; >> } 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 >
Hi Ryan, On 21.10.2015 13:34, Ryan Harkin wrote: > 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". Looks okay to me. Please make sure that you don't see any warning when compiling this new code for 32bit and 64bit platforms. >> 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... >> >> >>> 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; >>> } 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@denx.de> to this version then. Thanks, Stefan
On 21 October 2015 at 13:40, Stefan Roese <sr@denx.de> wrote: > Hi Ryan, > > On 21.10.2015 13:34, Ryan Harkin wrote: >> >> 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". > > > 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@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@denx.de> > > to this version then. Thanks, Ryan.
On 21 October 2015 at 13:49, Ryan Harkin <ryan.harkin@linaro.org> wrote: > On 21 October 2015 at 13:40, Stefan Roese <sr@denx.de> wrote: >> Hi Ryan, >> >> On 21.10.2015 13:34, Ryan Harkin wrote: >>> >>> 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". >> >> >> 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@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@denx.de> >> >> to this version then. > > > Thanks, > Ryan.
Hi Ryan, On 21.10.2015 15:48, Ryan Harkin wrote: <snip> >>>> 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. I'm fine with the "w" names. > 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? The cfi flash driver unfortunately has not seem much "love" in the last few years. As parallel NOR flash devices are used rarely nowadays. So you will most likely find many places that could use some cleanup / fixes. And yes, some "ulong"s might cause problems on the new 64bit systems. I really appreciate it that you take a look at this. Thanks, Stefan
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; /* CFI standard query structure */
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. 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(-)