[U-Boot] [PATCH 0/3] vexpress64 flash support

[U-Boot] [PATCH 0/3] vexpress64 flash support

[Ryan Harkin]

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

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

[Ryan Harkin]

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

[U-Boot] [PATCH 2/3] vexpress64: remove #error

[Ryan Harkin]

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

[U-Boot] [PATCH 3/3] vexpress64: store env in flash

[Ryan Harkin]

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

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

[Stefan Roese]

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

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

[Ryan Harkin]

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
>

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

[Stefan Roese]

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

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

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

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

[Stefan Roese]

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

[U-Boot] [PATCH 2/3] vexpress64: remove #error

[Linus Walleij]

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

[U-Boot] [PATCH 3/3] vexpress64: store env in flash

[Linus Walleij]

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

[U-Boot] [PATCH 2/3] vexpress64: remove #error

[Ryan Harkin]

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

[U-Boot] [PATCH 3/3] vexpress64: store env in flash

[Ryan Harkin]

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.

[U-Boot] [PATCH 3/3] vexpress64: store env in flash

[Linus Walleij]

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

[U-Boot] [PATCH 3/3] vexpress64: store env in flash

[Ryan Harkin]

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?

[U-Boot] [PATCH 3/3] vexpress64: store env in flash

[Linus Walleij]

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

[U-Boot] [PATCH 3/3] vexpress64: store env in flash

[Ryan Harkin]

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.

[U-Boot] [PATCH 3/3] vexpress64: store env in flash

[Linus Walleij]

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

[U-Boot] [PATCH 3/3] vexpress64: store env in flash

[Tom Rini]

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>

[U-Boot] [PATCH 3/3] vexpress64: store env in flash

[Ryan Harkin]

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?

[U-Boot] [PATCH 3/3] vexpress64: store env in flash

[Tom Rini]

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>

[U-Boot] [PATCH 2/3] vexpress64: remove #error

[Ryan Harkin]

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

[U-Boot] [PATCH 2/3] vexpress64: remove #error

[Linus Walleij]

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