* [U-Boot] [PATCH 0/3] vexpress64 flash support
@ 2015-10-21 9:54 Ryan Harkin
2015-10-21 9:54 ` [U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword Ryan Harkin
` (2 more replies)
0 siblings, 3 replies; 24+ messages in thread
From: Ryan Harkin @ 2015-10-21 9:54 UTC (permalink / raw)
To: u-boot
Whilst adding NOR support for Juno and FVP models, I encountered a
problem in cfi_flash.c. It is using "unsigned long" to represent a
32-bit value. This does not work on 64-bit systems such as vexpress64.
I fixed it up in patch 1/3, however, I'm not convinced that my mod is
the correct one. I'd appreciate some help with that one and will respin
after feedback.
Patch 2/3 may also be unsuitable. Once I'd added the CFI flash support,
the build failed because tools/envcrc.c includes include/config.h, which
in turn includes include/configs/vexpress_aemv8a.h, but without a board
defined. Perhaps there is something missing from the global config that
means the #errors should not be triggered? Again, feedback would be
appreciated here.
The last patch relies on the two previous ones to build and work
successfully. I believe it to be the correct approach but welcome
suggested improvements.
I tested it on Juno R0, R1 and FVP AEMv8 models version "Fast Models
[0.8.6302 (Feb 4 2015)]". I also tested on Foundation Model version
"ARM V8 Foundation Model r0p0 (model build 9.2.28)", where it failed
gracefully.
[PATCH 1/3] cfi_flash: use specific length types for cword
[PATCH 2/3] vexpress64: remove #error
[PATCH 3/3] vexpress64: store env in flash
configs/vexpress_aemv8a_dram_defconfig | 1 +
configs/vexpress_aemv8a_semi_defconfig | 1 +
drivers/mtd/cfi_flash.c | 4 ++--
include/configs/vexpress_aemv8a.h | 43 +++++++++++++++++++++++--------------------
include/mtd/cfi_flash.h | 8 ++++----
5 files changed, 31 insertions(+), 26 deletions(-)
This series is also available in a public GIT tree:
https://git.linaro.org/landing-teams/working/arm/u-boot.git/shortlog/refs/tags/vexpress64-nor-upstream-v1
^ permalink raw reply [flat|nested] 24+ messages in thread* [U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword 2015-10-21 9:54 [U-Boot] [PATCH 0/3] vexpress64 flash support Ryan Harkin @ 2015-10-21 9:54 ` Ryan Harkin 2015-10-21 10:24 ` Stefan Roese 2015-10-21 9:54 ` [U-Boot] [PATCH 2/3] vexpress64: remove #error Ryan Harkin 2015-10-21 9:54 ` [U-Boot] [PATCH 3/3] vexpress64: store env in flash Ryan Harkin 2 siblings, 1 reply; 24+ messages in thread From: Ryan Harkin @ 2015-10-21 9:54 UTC (permalink / raw) To: u-boot 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(-) 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 */ -- 2.1.4 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword 2015-10-21 9:54 ` [U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword Ryan Harkin @ 2015-10-21 10:24 ` Stefan Roese 2015-10-21 11:34 ` Ryan Harkin 0 siblings, 1 reply; 24+ messages in thread From: Stefan Roese @ 2015-10-21 10:24 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword 2015-10-21 10:24 ` Stefan Roese @ 2015-10-21 11:34 ` Ryan Harkin 2015-10-21 12:40 ` Stefan Roese 0 siblings, 1 reply; 24+ messages in thread From: Ryan Harkin @ 2015-10-21 11:34 UTC (permalink / raw) To: u-boot 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 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword 2015-10-21 11:34 ` Ryan Harkin @ 2015-10-21 12:40 ` Stefan Roese 2015-10-21 12:49 ` Ryan Harkin 0 siblings, 1 reply; 24+ messages in thread From: Stefan Roese @ 2015-10-21 12:40 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword 2015-10-21 12:40 ` Stefan Roese @ 2015-10-21 12:49 ` Ryan Harkin 2015-10-21 13:48 ` Ryan Harkin 0 siblings, 1 reply; 24+ messages in thread From: Ryan Harkin @ 2015-10-21 12:49 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword 2015-10-21 12:49 ` Ryan Harkin @ 2015-10-21 13:48 ` Ryan Harkin 2015-10-21 14:00 ` Stefan Roese 0 siblings, 1 reply; 24+ messages in thread From: Ryan Harkin @ 2015-10-21 13:48 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword 2015-10-21 13:48 ` Ryan Harkin @ 2015-10-21 14:00 ` Stefan Roese 0 siblings, 0 replies; 24+ messages in thread From: Stefan Roese @ 2015-10-21 14:00 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 2/3] vexpress64: remove #error 2015-10-21 9:54 [U-Boot] [PATCH 0/3] vexpress64 flash support Ryan Harkin 2015-10-21 9:54 ` [U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword Ryan Harkin @ 2015-10-21 9:54 ` Ryan Harkin 2015-10-27 11:36 ` Linus Walleij 2015-10-21 9:54 ` [U-Boot] [PATCH 3/3] vexpress64: store env in flash Ryan Harkin 2 siblings, 1 reply; 24+ messages in thread From: Ryan Harkin @ 2015-10-21 9:54 UTC (permalink / raw) To: u-boot This patch allows vexpress64 targets to be compiled when CONFIG_SYS_FLASH_CFI is enabled. I considered using #warning instead of #error, but this just clutters up the build output and hides real warnings. Without this patch, you see errors during compilation like this: include/configs/vexpress_aemv8a.h:42:2: error: #error "Unknown board variant" #error "Unknown board variant" include/configs/vexpress_aemv8a.h:115:2: error: #error "Unknown board variant" #error "Unknown board variant" include/configs/vexpress_aemv8a.h:280:2: error: #error "Unknown board variant" #error "Unknown board variant" make[1]: *** [tools/envcrc.o] Error 1 make: *** [tools] Error 2 In file included from include/config.h:5:0, from tools/envcrc.c:19: Signed-off-by: Ryan Harkin <ryan.harkin@linaro.org> --- include/configs/vexpress_aemv8a.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/include/configs/vexpress_aemv8a.h b/include/configs/vexpress_aemv8a.h index 0f2f1a3..c014c14 100644 --- a/include/configs/vexpress_aemv8a.h +++ b/include/configs/vexpress_aemv8a.h @@ -38,8 +38,6 @@ #elif CONFIG_TARGET_VEXPRESS64_JUNO #define CONFIG_SYS_TEXT_BASE 0xe0000000 #define CONFIG_SYS_INIT_SP_ADDR (CONFIG_SYS_SDRAM_BASE + 0x7fff0) -#else -#error "Unknown board variant" #endif #define CONFIG_SYS_BOOTM_LEN (64 << 20) /* Increase max gunzip size */ @@ -111,8 +109,6 @@ #elif CONFIG_TARGET_VEXPRESS64_JUNO #define GICD_BASE (0x2C010000) #define GICC_BASE (0x2C02f000) -#else -#error "Unknown board variant" #endif #endif /* !CONFIG_GICV3 */ @@ -276,8 +272,6 @@ #define CONFIG_BOOTDELAY 1 -#else -#error "Unknown board variant" #endif /* Do not preserve environment */ -- 2.1.4 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 2/3] vexpress64: remove #error 2015-10-21 9:54 ` [U-Boot] [PATCH 2/3] vexpress64: remove #error Ryan Harkin @ 2015-10-27 11:36 ` Linus Walleij 2015-10-27 12:29 ` Ryan Harkin 0 siblings, 1 reply; 24+ messages in thread From: Linus Walleij @ 2015-10-27 11:36 UTC (permalink / raw) To: u-boot On Wed, Oct 21, 2015 at 11:54 AM, Ryan Harkin <ryan.harkin@linaro.org> wrote: > This patch allows vexpress64 targets to be compiled when > CONFIG_SYS_FLASH_CFI is enabled. I can't see what this has to do with enabling CFI? > I considered using #warning instead of #error, but this just clutters up > the build output and hides real warnings. > > Without this patch, you see errors during compilation like this: > > include/configs/vexpress_aemv8a.h:42:2: error: #error "Unknown board > variant" > #error "Unknown board variant" The #error is there because noone could answer the question I posed: what AEMv8 boards are there that are neither FVP nor Juno? So what board variant are you compiling for here? I suspect a non-upstream thingofabob? Maybe there is a better way to cater for that than this magic catch-all? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 2/3] vexpress64: remove #error 2015-10-27 11:36 ` Linus Walleij @ 2015-10-27 12:29 ` Ryan Harkin 2015-11-11 16:11 ` Ryan Harkin 0 siblings, 1 reply; 24+ messages in thread From: Ryan Harkin @ 2015-10-27 12:29 UTC (permalink / raw) To: u-boot On 27 October 2015 at 11:36, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Oct 21, 2015 at 11:54 AM, Ryan Harkin <ryan.harkin@linaro.org> wrote: > >> This patch allows vexpress64 targets to be compiled when >> CONFIG_SYS_FLASH_CFI is enabled. > > I can't see what this has to do with enabling CFI? The errors happen when I enable CFI on Juno or FVP. Enabling CFI support includes envcrc.c into the build, which then includes include/config.h, which in turn includes include/configs/vexpress_aemv8a.h, but without a board defined, despite building a Juno or FVP configuration. I don't really know why the board isn't defined at that point. Looking deeper into why envcrc is included into the build, it was another #define that included it. I can see that tools/Makefile adds envcrc if CONFIG_BUILD_ENVCRC is defined, which in turn is enabled if ENVCRC-y is defined, which seems to happen if CONFIG_ENV_IS_IN_FLASH is defined. So my comment is wrong. > >> I considered using #warning instead of #error, but this just clutters up >> the build output and hides real warnings. >> >> Without this patch, you see errors during compilation like this: >> >> include/configs/vexpress_aemv8a.h:42:2: error: #error "Unknown board >> variant" >> #error "Unknown board variant" > > The #error is there because noone could answer the question I > posed: what AEMv8 boards are there that are neither FVP nor > Juno? AEMv8 may be the wrong term anyway. I *think* AEMv8 really only refers to one flavour of the FVP: AEMv8 is an representation of the complete ARMv8 architecture, not of a specific CPU variant such as Cortex A57, which omits some of the arch. Everything we're including in this vexpress_aemv8a file is really more like just vexpress64, with variants inside it for Juno and FVP. And with a bit of code to detect and handle the platform differences, we could probably create a single binary that works on everything covered by this config. Although I think the #define for the CFI base address could be a sticking point there. Currently, ARM have: Models: - FVP Foundation models - FVP AEMv8 models - FVP Cortex A57/A53 models ^ the same binary runs on all three, although I never test on Cortex models. There is a semihosting variant and I've created a DRAM variant. Real boards: - Juno R0 - Juno R1 ^ the same binary runs on both There are no other public platforms, to my knowledge. ARM have some internal models that have different peripheral sets, CPU configurations, etc... but these aren't covered by the public code at all. > > So what board variant are you compiling for here? I suspect a > non-upstream thingofabob? Maybe there is a better way to cater > for that than this magic catch-all? I was building for Juno. > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 2/3] vexpress64: remove #error 2015-10-27 12:29 ` Ryan Harkin @ 2015-11-11 16:11 ` Ryan Harkin 2015-11-17 13:36 ` Linus Walleij 0 siblings, 1 reply; 24+ messages in thread From: Ryan Harkin @ 2015-11-11 16:11 UTC (permalink / raw) To: u-boot Hi, This is a ping to revive this patch and the next one in the series: "[PATCH 3/3] vexpress64: store env in flash" Patch 1/3 was resent separately and has been merged. This patch (and the subsequent one in the series) hasn't been Reviewed-by or Acked-by anyone, but in a thread for patch 3/3 in this series, Tom Rini and I had this conversation: Tom> The host tools are not board-independent as they include a copy of the Tom> target board env. Keep that in mind. Me> So that means we can't use #error in the target board include file Me> (eg. vexpress_aemv8a.h) to indicate that no board was set, correct? Tom> Correct. So if Juno wants to use the NOR flash and therefore, the host tools, then first we need this patch to remove the #error statements. Thanks, Ryan. On 27 October 2015 at 12:29, Ryan Harkin <ryan.harkin@linaro.org> wrote: > On 27 October 2015 at 11:36, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Wed, Oct 21, 2015 at 11:54 AM, Ryan Harkin <ryan.harkin@linaro.org> wrote: >> >>> This patch allows vexpress64 targets to be compiled when >>> CONFIG_SYS_FLASH_CFI is enabled. >> >> I can't see what this has to do with enabling CFI? > > The errors happen when I enable CFI on Juno or FVP. Enabling CFI > support includes envcrc.c into the build, which then includes > include/config.h, which in turn includes > include/configs/vexpress_aemv8a.h, but without a board defined, > despite building a Juno or FVP configuration. I don't really know why > the board isn't defined at that point. > > Looking deeper into why envcrc is included into the build, it was > another #define that included it. > > I can see that tools/Makefile adds envcrc if CONFIG_BUILD_ENVCRC is > defined, which in turn is enabled if ENVCRC-y is defined, which seems > to happen if CONFIG_ENV_IS_IN_FLASH is defined. > > So my comment is wrong. > >> >>> I considered using #warning instead of #error, but this just clutters up >>> the build output and hides real warnings. >>> >>> Without this patch, you see errors during compilation like this: >>> >>> include/configs/vexpress_aemv8a.h:42:2: error: #error "Unknown board >>> variant" >>> #error "Unknown board variant" >> >> The #error is there because noone could answer the question I >> posed: what AEMv8 boards are there that are neither FVP nor >> Juno? > > AEMv8 may be the wrong term anyway. I *think* AEMv8 really only > refers to one flavour of the FVP: AEMv8 is an representation of the > complete ARMv8 architecture, not of a specific CPU variant such as > Cortex A57, which omits some of the arch. > > Everything we're including in this vexpress_aemv8a file is really more > like just vexpress64, with variants inside it for Juno and FVP. > > And with a bit of code to detect and handle the platform differences, > we could probably create a single binary that works on everything > covered by this config. Although I think the #define for the CFI base > address could be a sticking point there. > > Currently, ARM have: > > Models: > - FVP Foundation models > - FVP AEMv8 models > - FVP Cortex A57/A53 models > > ^ the same binary runs on all three, although I never test on Cortex > models. There is a semihosting variant and I've created a DRAM > variant. > > Real boards: > - Juno R0 > - Juno R1 > > ^ the same binary runs on both > > There are no other public platforms, to my knowledge. ARM have some > internal models that have different peripheral sets, CPU > configurations, etc... but these aren't covered by the public code at > all. > >> >> So what board variant are you compiling for here? I suspect a >> non-upstream thingofabob? Maybe there is a better way to cater >> for that than this magic catch-all? > > I was building for Juno. > > >> >> Yours, >> Linus Walleij ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 2/3] vexpress64: remove #error 2015-11-11 16:11 ` Ryan Harkin @ 2015-11-17 13:36 ` Linus Walleij 0 siblings, 0 replies; 24+ messages in thread From: Linus Walleij @ 2015-11-17 13:36 UTC (permalink / raw) To: u-boot On Wed, Nov 11, 2015 at 5:11 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote: > This patch (and the subsequent one in the series) hasn't been > Reviewed-by or Acked-by anyone, but in a thread for patch 3/3 in this > series, Tom Rini and I had this conversation: > > Tom> The host tools are not board-independent as they include a copy of the > Tom> target board env. Keep that in mind. > > Me> So that means we can't use #error in the target board include file > Me> (eg. vexpress_aemv8a.h) to indicate that no board was set, correct? > > Tom> Correct. > > So if Juno wants to use the NOR flash and therefore, the host tools, > then first we need this patch to remove the #error statements. Sorry, been busy. Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 3/3] vexpress64: store env in flash 2015-10-21 9:54 [U-Boot] [PATCH 0/3] vexpress64 flash support Ryan Harkin 2015-10-21 9:54 ` [U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword Ryan Harkin 2015-10-21 9:54 ` [U-Boot] [PATCH 2/3] vexpress64: remove #error Ryan Harkin @ 2015-10-21 9:54 ` Ryan Harkin 2015-10-27 11:49 ` Linus Walleij 2 siblings, 1 reply; 24+ messages in thread From: Ryan Harkin @ 2015-10-21 9:54 UTC (permalink / raw) To: u-boot Add support for storing the environment in CFI NOR flash on Juno and FVP models. I also removed some config values that are not used by CFI flash parts. Juno has 1 flash part with 259 sectors. The first 255 sectors are 0x40000 (256kb) and are followed by 4 sectors of 0x10000 (64KB). FVP models simulate a 64MB NOR flash part at base address 0x0FFC0000. This part has 256 x 256kb sectors. We use the last sector to store the environment. To save the NOR flash to a file, the following parameters should be passed to the model: -C bp.flashloader1.fname=${FILENAME} -C bp.flashloader1.fnameWrite=${FILENAME} Foundation models don't simulate the NOR flash, but having NOR support in the u-boot binary does not harm: attempting to write to the NOR will fail gracefully. Signed-off-by: Ryan Harkin <ryan.harkin@linaro.org> --- configs/vexpress_aemv8a_dram_defconfig | 1 - configs/vexpress_aemv8a_semi_defconfig | 1 - include/configs/vexpress_aemv8a.h | 37 ++++++++++++++++++---------------- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/configs/vexpress_aemv8a_dram_defconfig b/configs/vexpress_aemv8a_dram_defconfig index e9fc870..a8e4daa 100644 --- a/configs/vexpress_aemv8a_dram_defconfig +++ b/configs/vexpress_aemv8a_dram_defconfig @@ -8,7 +8,6 @@ CONFIG_DEFAULT_DEVICE_TREE="vexpress64" # CONFIG_CMD_EDITENV is not set # CONFIG_CMD_ENV_EXISTS is not set # CONFIG_CMD_LOADS is not set -# CONFIG_CMD_FLASH is not set # CONFIG_CMD_FPGA is not set # CONFIG_CMD_ITEST is not set # CONFIG_CMD_SETEXPR is not set diff --git a/configs/vexpress_aemv8a_semi_defconfig b/configs/vexpress_aemv8a_semi_defconfig index a082d27..e899b90 100644 --- a/configs/vexpress_aemv8a_semi_defconfig +++ b/configs/vexpress_aemv8a_semi_defconfig @@ -10,7 +10,6 @@ CONFIG_SYS_PROMPT="VExpress64# " # CONFIG_CMD_EDITENV is not set # CONFIG_CMD_ENV_EXISTS is not set # CONFIG_CMD_LOADS is not set -# CONFIG_CMD_FLASH is not set # CONFIG_CMD_FPGA is not set # CONFIG_CMD_ITEST is not set # CONFIG_CMD_SETEXPR is not set diff --git a/include/configs/vexpress_aemv8a.h b/include/configs/vexpress_aemv8a.h index c014c14..e795806 100644 --- a/include/configs/vexpress_aemv8a.h +++ b/include/configs/vexpress_aemv8a.h @@ -274,10 +274,6 @@ #endif -/* Do not preserve environment */ -#define CONFIG_ENV_IS_NOWHERE 1 -#define CONFIG_ENV_SIZE 0x1000 - /* Monitor Command Prompt */ #define CONFIG_SYS_CBSIZE 512 /* Console I/O Buffer Size */ #define CONFIG_SYS_PBSIZE (CONFIG_SYS_CBSIZE + \ @@ -288,28 +284,35 @@ #define CONFIG_CMDLINE_EDITING #define CONFIG_SYS_MAXARGS 64 /* max command args */ -/* Flash memory is available on the Juno board only */ -#ifndef CONFIG_TARGET_VEXPRESS64_JUNO -#define CONFIG_SYS_NO_FLASH +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO +#define CONFIG_SYS_FLASH_BASE 0x08000000 +/* 255 x 256KiB sectors + 4 x 64KiB sectors at the end = 259 */ +#define CONFIG_SYS_MAX_FLASH_SECT 259 +/* Store environment at top of flash in the same location as blank.img */ +/* in the Juno firmware. */ +#define CONFIG_ENV_ADDR 0x0BFC0000 +#define CONFIG_ENV_SECT_SIZE 0x00010000 #else +#define CONFIG_SYS_FLASH_BASE 0x0C000000 +/* 256 x 256KiB sectors */ +#define CONFIG_SYS_MAX_FLASH_SECT 256 +/* Store environment at top of flash */ +#define CONFIG_ENV_ADDR 0x0FFC0000 +#define CONFIG_ENV_SECT_SIZE 0x00040000 +#endif + #define CONFIG_CMD_ARMFLASH #define CONFIG_SYS_FLASH_CFI 1 #define CONFIG_FLASH_CFI_DRIVER 1 #define CONFIG_SYS_FLASH_CFI_WIDTH FLASH_CFI_32BIT -#define CONFIG_SYS_FLASH_BASE 0x08000000 -#define CONFIG_SYS_FLASH_SIZE 0x04000000 /* 64 MiB */ -#define CONFIG_SYS_MAX_FLASH_BANKS 2 +#define CONFIG_SYS_MAX_FLASH_BANKS 1 -/* Timeout values in ticks */ -#define CONFIG_SYS_FLASH_ERASE_TOUT (2 * CONFIG_SYS_HZ) /* Erase Timeout */ -#define CONFIG_SYS_FLASH_WRITE_TOUT (2 * CONFIG_SYS_HZ) /* Write Timeout */ - -/* 255 0x40000 sectors + first or last sector may have 4 erase regions = 259 */ -#define CONFIG_SYS_MAX_FLASH_SECT 259 /* Max sectors */ #define CONFIG_SYS_FLASH_USE_BUFFER_WRITE /* use buffered writes */ #define CONFIG_SYS_FLASH_PROTECTION /* The devices have real protection */ #define CONFIG_SYS_FLASH_EMPTY_INFO /* flinfo indicates empty blocks */ +#define FLASH_MAX_SECTOR_SIZE 0x00040000 +#define CONFIG_ENV_SIZE CONFIG_ENV_SECT_SIZE +#define CONFIG_ENV_IS_IN_FLASH 1 -#endif #endif /* __VEXPRESS_AEMV8A_H */ -- 2.1.4 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 3/3] vexpress64: store env in flash 2015-10-21 9:54 ` [U-Boot] [PATCH 3/3] vexpress64: store env in flash Ryan Harkin @ 2015-10-27 11:49 ` Linus Walleij 2015-10-27 12:49 ` Ryan Harkin 0 siblings, 1 reply; 24+ messages in thread From: Linus Walleij @ 2015-10-27 11:49 UTC (permalink / raw) To: u-boot On Wed, Oct 21, 2015 at 11:54 AM, Ryan Harkin <ryan.harkin@linaro.org> wrote: > Juno has 1 flash part with 259 sectors. The first 255 sectors are > 0x40000 (256kb) and are followed by 4 sectors of 0x10000 (64KB). > > FVP models simulate a 64MB NOR flash part at base address 0x0FFC0000. > This part has 256 x 256kb sectors. We use the last sector to store the > environment. Does this mean it appears in a AFS partition so that we see it sitting around there in the flash with afs (no arguments)? I'm asking because that seems to be how it work on the RealView PB11MPCore which was based on some out-of-tree U-Boot that Peter Pearse was maintaining at the time. If there is not a AFS partition for the U-Boot environment, we should take measures to create one, since the flash is handled in AFS partitions on these machines. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 3/3] vexpress64: store env in flash 2015-10-27 11:49 ` Linus Walleij @ 2015-10-27 12:49 ` Ryan Harkin 2015-10-27 21:39 ` Linus Walleij 0 siblings, 1 reply; 24+ messages in thread From: Ryan Harkin @ 2015-10-27 12:49 UTC (permalink / raw) To: u-boot Hi Linus, On 27 October 2015 at 11:49, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Oct 21, 2015 at 11:54 AM, Ryan Harkin <ryan.harkin@linaro.org> wrote: > >> Juno has 1 flash part with 259 sectors. The first 255 sectors are >> 0x40000 (256kb) and are followed by 4 sectors of 0x10000 (64KB). >> >> FVP models simulate a 64MB NOR flash part at base address 0x0FFC0000. >> This part has 256 x 256kb sectors. We use the last sector to store the >> environment. > > Does this mean it appears in a AFS partition so that we see it > sitting around there in the flash with afs (no arguments)? Yes and no. But mostly no. > > I'm asking because that seems to be how it work on the > RealView PB11MPCore which was based on some out-of-tree > U-Boot that Peter Pearse was maintaining at the time. > That seems like a nice feature. > If there is not a AFS partition for the U-Boot environment, > we should take measures to create one, since the flash > is handled in AFS partitions on these machines. > On FVP, afs does nothing because the simulated NOR flash will come up empty each time. On Juno, I have created an entry in the motherboard firmware's images.txt for a file called blank.img that lives in the same region as the config. The config current consumes 1 x 64kb sector, but can consume all 4 x 64kb sectors; blank.img spans 4 x 64kb sectors. The main idea for blank.img is two-fold: 1) let other people who edit images.txt know that we are using that region 2) allow users to erase the config by mounting the motherboard's mass storage device and touching blank.img. On reboot, the firmware will see the timestamp update and re-write the blank.img file, this erasing the config. This is mostly useful because both EDK2 and U-Boot use the same area of flash to store their config and EDK2 gets quite upset if the config isn't what it expected it to be. A potential problem: If the config expands to consume all 4 x 64kb partitions, then the CFI code's sector erase will wipe the footer that afs uses to signal the blank.img file. Of course, the afs command showing the area as "blank.img" is not representative of what u-boot has done with it. Either way, AFAIK, the existing CFI code does not handle AFS support. I think the CFI code would need extending to write the footer if we wanted the afs command to show the config in another way. Could that be what Peter Pearse did? Thanks, Ryan. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 3/3] vexpress64: store env in flash 2015-10-27 12:49 ` Ryan Harkin @ 2015-10-27 21:39 ` Linus Walleij 2015-10-28 15:07 ` Ryan Harkin 0 siblings, 1 reply; 24+ messages in thread From: Linus Walleij @ 2015-10-27 21:39 UTC (permalink / raw) To: u-boot On Tue, Oct 27, 2015 at 1:49 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote: > Either way, AFAIK, the existing CFI code does not handle AFS support. > I think the CFI code would need extending to write the footer if we > wanted the afs command to show the config in another way. Could that > be what Peter Pearse did? Uh, I'd have to dig up the code ... actually that's a very good question. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 3/3] vexpress64: store env in flash 2015-10-27 21:39 ` Linus Walleij @ 2015-10-28 15:07 ` Ryan Harkin 2015-10-28 15:10 ` Linus Walleij 0 siblings, 1 reply; 24+ messages in thread From: Ryan Harkin @ 2015-10-28 15:07 UTC (permalink / raw) To: u-boot On 27 October 2015 at 21:39, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Oct 27, 2015 at 1:49 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote: > >> Either way, AFAIK, the existing CFI code does not handle AFS support. >> I think the CFI code would need extending to write the footer if we >> wanted the afs command to show the config in another way. Could that >> be what Peter Pearse did? > > Uh, I'd have to dig up the code ... actually that's a very good question. > In the meantime, are you happy with my two patches? One to remove the #error lines and this one? ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 3/3] vexpress64: store env in flash 2015-10-28 15:07 ` Ryan Harkin @ 2015-10-28 15:10 ` Linus Walleij 2015-10-28 16:06 ` Ryan Harkin 0 siblings, 1 reply; 24+ messages in thread From: Linus Walleij @ 2015-10-28 15:10 UTC (permalink / raw) To: u-boot On Wed, Oct 28, 2015 at 4:07 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote: > On 27 October 2015 at 21:39, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Tue, Oct 27, 2015 at 1:49 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote: >> >>> Either way, AFAIK, the existing CFI code does not handle AFS support. >>> I think the CFI code would need extending to write the footer if we >>> wanted the afs command to show the config in another way. Could that >>> be what Peter Pearse did? >> >> Uh, I'd have to dig up the code ... actually that's a very good question. >> > > In the meantime, are you happy with my two patches? One to remove the > #error lines and this one? This one: Acked-by: Linus Walleij <linus.walleij@linaro.org> The other one I need to look at again, I'm not sure I understand it or why it's needed.... was there a v2 coming? Linus ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 3/3] vexpress64: store env in flash 2015-10-28 15:10 ` Linus Walleij @ 2015-10-28 16:06 ` Ryan Harkin 2015-10-28 21:43 ` Linus Walleij 0 siblings, 1 reply; 24+ messages in thread From: Ryan Harkin @ 2015-10-28 16:06 UTC (permalink / raw) To: u-boot On 28 October 2015 at 15:10, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Oct 28, 2015 at 4:07 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote: >> On 27 October 2015 at 21:39, Linus Walleij <linus.walleij@linaro.org> wrote: >>> On Tue, Oct 27, 2015 at 1:49 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote: >>> >>>> Either way, AFAIK, the existing CFI code does not handle AFS support. >>>> I think the CFI code would need extending to write the footer if we >>>> wanted the afs command to show the config in another way. Could that >>>> be what Peter Pearse did? >>> >>> Uh, I'd have to dig up the code ... actually that's a very good question. >>> >> >> In the meantime, are you happy with my two patches? One to remove the >> #error lines and this one? > > This one: > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > The other one I need to look at again, I'm not sure I understand > it or why it's needed.... Yeah, have a look again. I hoped my last reply explained that the #error stuff happens when we include envcrc.c in the build. It's nothing to do with Juno r0/1/2 or FVP. For reasons I don't understand, envcrc.c includes config.h which includes vexpress64_aemv8a.h without the board #defines set at all, no matter which board you are. So the #error gets hit no matter if you're building for FVP or Juno. > was there a v2 coming? Yes. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 3/3] vexpress64: store env in flash 2015-10-28 16:06 ` Ryan Harkin @ 2015-10-28 21:43 ` Linus Walleij 2015-10-28 22:07 ` Tom Rini 0 siblings, 1 reply; 24+ messages in thread From: Linus Walleij @ 2015-10-28 21:43 UTC (permalink / raw) To: u-boot On Wed, Oct 28, 2015 at 5:06 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote: > For reasons I don't understand, envcrc.c includes config.h which > includes vexpress64_aemv8a.h without the board #defines set at all, no > matter which board you are. So the #error gets hit no matter if > you're building for FVP or Juno. This is what we should try to avoid, we need to know why this happens :/ Yours, Linus Walleij ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 3/3] vexpress64: store env in flash 2015-10-28 21:43 ` Linus Walleij @ 2015-10-28 22:07 ` Tom Rini 2015-10-29 9:17 ` Ryan Harkin 0 siblings, 1 reply; 24+ messages in thread From: Tom Rini @ 2015-10-28 22:07 UTC (permalink / raw) To: u-boot On Wed, Oct 28, 2015 at 10:43:41PM +0100, Linus Walleij wrote: > On Wed, Oct 28, 2015 at 5:06 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote: > > > For reasons I don't understand, envcrc.c includes config.h which > > includes vexpress64_aemv8a.h without the board #defines set at all, no > > matter which board you are. So the #error gets hit no matter if > > you're building for FVP or Juno. > > This is what we should try to avoid, we need to know why this happens :/ The host tools are not board-independent as they include a copy of the target board env. Keep that in mind. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151028/57cca370/attachment.sig> ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 3/3] vexpress64: store env in flash 2015-10-28 22:07 ` Tom Rini @ 2015-10-29 9:17 ` Ryan Harkin 2015-10-29 14:50 ` Tom Rini 0 siblings, 1 reply; 24+ messages in thread From: Ryan Harkin @ 2015-10-29 9:17 UTC (permalink / raw) To: u-boot Hi Tom, On 28 October 2015 at 22:07, Tom Rini <trini@konsulko.com> wrote: > On Wed, Oct 28, 2015 at 10:43:41PM +0100, Linus Walleij wrote: >> On Wed, Oct 28, 2015 at 5:06 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote: >> >> > For reasons I don't understand, envcrc.c includes config.h which >> > includes vexpress64_aemv8a.h without the board #defines set at all, no >> > matter which board you are. So the #error gets hit no matter if >> > you're building for FVP or Juno. >> >> This is what we should try to avoid, we need to know why this happens :/ > > The host tools are not board-independent as they include a copy of the > target board env. Keep that in mind. So that means we can't use #error in the target board include file (eg. vexpress_aemv8a.h) to indicate that no board was set, correct? ^ permalink raw reply [flat|nested] 24+ messages in thread
* [U-Boot] [PATCH 3/3] vexpress64: store env in flash 2015-10-29 9:17 ` Ryan Harkin @ 2015-10-29 14:50 ` Tom Rini 0 siblings, 0 replies; 24+ messages in thread From: Tom Rini @ 2015-10-29 14:50 UTC (permalink / raw) To: u-boot On Thu, Oct 29, 2015 at 09:17:04AM +0000, Ryan Harkin wrote: > Hi Tom, > > On 28 October 2015 at 22:07, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Oct 28, 2015 at 10:43:41PM +0100, Linus Walleij wrote: > >> On Wed, Oct 28, 2015 at 5:06 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote: > >> > >> > For reasons I don't understand, envcrc.c includes config.h which > >> > includes vexpress64_aemv8a.h without the board #defines set at all, no > >> > matter which board you are. So the #error gets hit no matter if > >> > you're building for FVP or Juno. > >> > >> This is what we should try to avoid, we need to know why this happens :/ > > > > The host tools are not board-independent as they include a copy of the > > target board env. Keep that in mind. > > So that means we can't use #error in the target board include file > (eg. vexpress_aemv8a.h) to indicate that no board was set, correct? Correct. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151029/c145d3e0/attachment.sig> ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2015-11-17 13:36 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-21 9:54 [U-Boot] [PATCH 0/3] vexpress64 flash support Ryan Harkin 2015-10-21 9:54 ` [U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword Ryan Harkin 2015-10-21 10:24 ` Stefan Roese 2015-10-21 11:34 ` Ryan Harkin 2015-10-21 12:40 ` Stefan Roese 2015-10-21 12:49 ` Ryan Harkin 2015-10-21 13:48 ` Ryan Harkin 2015-10-21 14:00 ` Stefan Roese 2015-10-21 9:54 ` [U-Boot] [PATCH 2/3] vexpress64: remove #error Ryan Harkin 2015-10-27 11:36 ` Linus Walleij 2015-10-27 12:29 ` Ryan Harkin 2015-11-11 16:11 ` Ryan Harkin 2015-11-17 13:36 ` Linus Walleij 2015-10-21 9:54 ` [U-Boot] [PATCH 3/3] vexpress64: store env in flash Ryan Harkin 2015-10-27 11:49 ` Linus Walleij 2015-10-27 12:49 ` Ryan Harkin 2015-10-27 21:39 ` Linus Walleij 2015-10-28 15:07 ` Ryan Harkin 2015-10-28 15:10 ` Linus Walleij 2015-10-28 16:06 ` Ryan Harkin 2015-10-28 21:43 ` Linus Walleij 2015-10-28 22:07 ` Tom Rini 2015-10-29 9:17 ` Ryan Harkin 2015-10-29 14:50 ` Tom Rini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.