vexpress64 flash support
cover vexpress64 flash support
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
PATCH 1/3 cfi_flash: use specific length types for cword
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.
--- drivers/mtd/cfi_flash.c | 4 ++-- include/mtd/cfi_flash.h | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-)
drivers/mtd/cfi_flash.c +2 -2
@@ -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 :
include/mtd/cfi_flash.h +4 -4
@@ -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 */
Signature · 1 line
2.1.4
PATCH 1/3 cfi_flash: use specific length types for cword
Hi Ryan,
Quoted 20 lines · 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); ^
Quoted 2 lines
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.
Quoted 35 lines
Signed-off-by Ryan Harkin <[email protected]>--- drivers/mtd/cfi_flash.c | 4 ++-- include/mtd/cfi_flash.h | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-)drivers/mtd/cfi_flash.c +2
-2@@ -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 :include/mtd/cfi_flash.h +0
-0@@ -105,10 +105,10 @@#define NUM_ERASE_REGIONS 4/* max. number of erase regions */
include/mtd/cfi_flash.h line 112
Quoted 10 lines
include/mtd/cfi_flash.h +4
-4typedef 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
PATCH 1/3 cfi_flash: use specific length types for cword
Hi Stefan,
Quoted 24 lines · Stefan Roese
Hi Ryan,
Quoted 21 lines · 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); ^
Quoted 5 lines
Quoted 2 lines
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".
Quoted 2 lines
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.
Quoted 39 lines
Quoted 39 lines
Signed-off-by Ryan Harkin <[email protected]>--- drivers/mtd/cfi_flash.c | 4 ++-- include/mtd/cfi_flash.h | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-)drivers/mtd/cfi_flash.c +1
-1@@ -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);drivers/mtd/cfi_flash.c +1
-1@@ -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:include/mtd/cfi_flash.h +0
-0@@ -105,10 +105,10 @@#define NUM_ERASE_REGIONS 4/* max. number of erase regions */
Quoted 14 lines
include/mtd/cfi_flash.h line 112
Quoted 10 lines
include/mtd/cfi_flash.h +4
-4typedef 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.
Quoted 4 lines
Thanks,
Stefan
PATCH 1/3 cfi_flash: use specific length types for cword
Hi Ryan,
Quoted 25 lines · Ryan Harkin
Quoted 24 lines · Stefan Roese
Hi Ryan,
Quoted 21 lines · 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); ^
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
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...
Quoted 54 lines
Quoted 39 lines
Quoted 39 lines
Signed-off-by Ryan Harkin <[email protected]>--- drivers/mtd/cfi_flash.c | 4 ++-- include/mtd/cfi_flash.h | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-)drivers/mtd/cfi_flash.c +1
-1@@ -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);drivers/mtd/cfi_flash.c +1
-1@@ -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:include/mtd/cfi_flash.h +0
-0@@ -105,10 +105,10 @@#define NUM_ERASE_REGIONS 4/* max. number of erase regions */ Quoted 14 lines
include/mtd/cfi_flash.h line 112
Quoted 10 lines
include/mtd/cfi_flash.h +4
-4typedef 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.
Quoted 1 line
Yes, I'll make that change for v2.
Thanks. You can add my:
to this version then.
Thanks,
Stefan
PATCH 1/3 cfi_flash: use specific length types for cword
Quoted 39 lines · Stefan Roese
Hi Ryan,
Quoted 36 lines · Ryan Harkin
Quoted 32 lines · Stefan Roese
Hi Ryan,
Quoted 22 lines · 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); ^Quoted 2 lines
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?
Quoted 6 lines
Quoted 2 lines
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.
Quoted 12 lines
Quoted 10 lines
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 62 lines
Quoted 62 lines
Quoted 60 lines
Quoted 45 lines
Signed-off-by Ryan Harkin <[email protected]>--- drivers/mtd/cfi_flash.c | 4 ++-- include/mtd/cfi_flash.h | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-)drivers/mtd/cfi_flash.c +1
-1@@ -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);drivers/mtd/cfi_flash.c +1
-1@@ -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:include/mtd/cfi_flash.h +0
-0@@ -105,10 +105,10 @@#define NUM_ERASE_REGIONS 4/* max. number of erase regions */ include/mtd/cfi_flash.h line 112
Quoted 10 lines
include/mtd/cfi_flash.h +4
-4typedef 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.
Quoted 8 lines
Quoted 1 line
Yes, I'll make that change for v2.
Thanks. You can add my:
Acked-by Stefan Roese <[email protected]>to this version then.
Thanks,
Ryan.
PATCH 1/3 cfi_flash: use specific length types for cword
Quoted 61 lines · Ryan Harkin
Quoted 39 lines · Stefan Roese
Hi Ryan,
Quoted 36 lines · Ryan Harkin
Quoted 32 lines · Stefan Roese
Hi Ryan,
Quoted 22 lines · 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); ^Quoted 2 lines
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?
Quoted 6 lines
Quoted 2 lines
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.
Quoted 12 lines
Quoted 10 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 4 lines
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?
Quoted 75 lines
Quoted 63 lines
Quoted 63 lines
Quoted 61 lines
Quoted 46 lines
Signed-off-by Ryan Harkin <[email protected]>--- drivers/mtd/cfi_flash.c | 4 ++-- include/mtd/cfi_flash.h | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-)drivers/mtd/cfi_flash.c +1
-1@@ -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);drivers/mtd/cfi_flash.c +1
-1@@ -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:include/mtd/cfi_flash.h +0
-0@@ -105,10 +105,10 @@#define NUM_ERASE_REGIONS 4/* max. number of erase regions */ include/mtd/cfi_flash.h line 112
Quoted 10 lines
include/mtd/cfi_flash.h +4
-4typedef 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.Quoted 8 lines
Quoted 1 line
Yes, I'll make that change for v2.
Thanks. You can add my:
Acked-by Stefan Roese <[email protected]>to this version then.
Thanks,
Ryan.
PATCH 1/3 cfi_flash: use specific length types for cword
Hi Ryan,
On 21.10.2015 15:48, Ryan Harkin wrote:
<snip>
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
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...
Quoted 5 lines
Quoted 3 lines
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.
Quoted 9 lines
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];
Quoted 5 lines
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_tininclude/flash.hand 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
PATCH 2/3 vexpress64: remove #error
In reply to Ryan HarkinThis 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:
--- include/configs/vexpress_aemv8a.h | 6 ------ 1 file changed, 6 deletions(-)
include/configs/vexpress_aemv8a.h +0 -6
@@ -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 */
Signature · 1 line
2.1.4
PATCH 2/3 vexpress64: remove #error
Quoted 2 lines · Ryan Harkin
This patch allows vexpress64 targets to be compiled when
CONFIG_SYS_FLASH_CFIis enabled.
I can't see what this has to do with enabling CFI?
Quoted 8 lines
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
PATCH 2/3 vexpress64: remove #error
Quoted 6 lines · Linus Walleij
Quoted 2 lines · Ryan Harkin
This patch allows vexpress64 targets to be compiled when
CONFIG_SYS_FLASH_CFIis 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.
Quoted 6 lines
Quoted 6 lines
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:
Quoted 7 lines
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 3 lines
Yours,
Linus Walleij
PATCH 2/3 vexpress64: remove #error
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.
Quoted 84 lines · Ryan Harkin
Quoted 6 lines · Linus Walleij
Quoted 2 lines · Ryan Harkin
This patch allows vexpress64 targets to be compiled when
CONFIG_SYS_FLASH_CFIis 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.cinto the build, which then includesinclude/config.h, which in turn includesinclude/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/Makefileadds envcrc ifCONFIG_BUILD_ENVCRCis defined, which in turn is enabled if ENVCRC-y is defined, which seems to happen ifCONFIG_ENV_IS_IN_FLASHis defined.So my comment is wrong.
Quoted 6 lines
Quoted 6 lines
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:
Quoted 7 lines
Quoted 3 lines
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_aemv8afile 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.
Quoted 4 lines
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.
Quoted 3 lines
Yours,
Linus Walleij
PATCH 2/3 vexpress64: remove #error
Quoted 7 lines · Ryan Harkin
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.
Quoted 7 lines
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.
Yours,
Linus Walleij
PATCH 3/3 vexpress64: store env in flash
In reply to Ryan HarkinAdd 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.
--- 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(-)
configs/vexpress_aemv8a_dram_defconfig +0 -1
@@ -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
configs/vexpress_aemv8a_semi_defconfig +0 -1
@@ -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
include/configs/vexpress_aemv8a.h +20 -17
@@ -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 */
Signature · 1 line
2.1.4
PATCH 3/3 vexpress64: store env in flash
Quoted 6 lines · Ryan Harkin
Juno has 1 flash part with 259 sectors. The first 255 sectors are
0x40000(256kb) and are followed by 4 sectors of0x10000(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
PATCH 3/3 vexpress64: store env in flash
Hi Linus,
Quoted 11 lines · Linus Walleij
Quoted 6 lines · Ryan Harkin
Juno has 1 flash part with 259 sectors. The first 255 sectors are
0x40000(256kb) and are followed by 4 sectors of0x10000(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.
Quoted 5 lines
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.
Quoted 4 lines
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.
PATCH 3/3 vexpress64: store env in flash
Quoted 4 lines · Ryan Harkin
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
PATCH 3/3 vexpress64: store env in flash
Quoted 9 lines · Linus Walleij
Quoted 4 lines · Ryan Harkin
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?
PATCH 3/3 vexpress64: store env in flash
Quoted 8 lines · Ryan Harkin
Quoted 7 lines · Linus Walleij
Quoted 4 lines · Ryan Harkin
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?
Quoted 5 lines
Quoted 2 lines
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:
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
PATCH 3/3 vexpress64: store env in flash
Quoted 12 lines · Linus Walleij
Quoted 11 lines · Ryan Harkin
Quoted 9 lines · Linus Walleij
Quoted 4 lines · Ryan Harkin
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.
Quoted 8 lines
Quoted 2 lines
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 <[email protected]>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.
Quoted 1 line
was there a v2 coming?
Yes.
PATCH 3/3 vexpress64: store env in flash
Quoted 4 lines · Ryan Harkin
For reasons I don't understand,
envcrc.cincludesconfig.hwhich includesvexpress64_aemv8a.hwithout 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
PATCH 3/3 vexpress64: store env in flash
Quoted 8 lines · Linus Walleij
Quoted 4 lines · Ryan Harkin
For reasons I don't understand,
envcrc.cincludesconfig.hwhich includesvexpress64_aemv8a.hwithout 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.
Signature · 8 lines
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>
PATCH 3/3 vexpress64: store env in flash
Hi Tom,
Quoted 8 lines · Tom Rini
Quoted 7 lines · Linus Walleij
Quoted 4 lines · Ryan Harkin
For reasons I don't understand,
envcrc.cincludesconfig.hwhich includesvexpress64_aemv8a.hwithout 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.
Quoted 4 lines
Quoted 1 line
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?
PATCH 3/3 vexpress64: store env in flash
Quoted 11 lines · Ryan Harkin
Hi Tom,
Quoted 8 lines · Tom Rini
Quoted 7 lines · Linus Walleij
Quoted 4 lines · Ryan Harkin
For reasons I don't understand,
envcrc.cincludesconfig.hwhich includesvexpress64_aemv8a.hwithout 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.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.
Signature · 8 lines
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>