sunxi: Increase SPL header size to 64 bytes to avoid code corruption

The current SPL header, created by the 'mksunxiboot' tool, has size 32 bytes. But the code in the boot ROM stores the information about the boot media at the offset 0x28 before passing control to the SPL. For example, when booting from the SD card, the magic number written by the boot ROM is 0. And when booting from the SPI flash, the magic number is 3. NAND and eMMC probably have their own special magic numbers too.

Currently the corrupted byte is a part of one of the instructions in the reset vectors table:

    b     reset
    ldr   pc, _undefined_instruction
    ldr   pc, _software_interrupt      <- Corruption happens here
    ldr   pc, _prefetch_abort
    ldr   pc, _data_abort
    ldr   pc, _not_used
    ldr   pc, _irq
    ldr   pc, _fiq

In practice this does not cause any visible problems, but it's still better to fix it. As a bonus, the reported boot media type can be later used in the 'spl_boot_device' function, but this is out of the scope of this patch.

Signed-off-by Siarhei Siamashka <[email protected]>

---

Changes in v2:
- Increase the header size to 64 bytes instead of 48 bytes in order to satisfy the VBAR alignment requirements.

 arch/arm/include/asm/arch-sunxi/spl.h |  8 +++++++-
 include/configs/sunxi-common.h        | 12 ++++++------
 2 files changed, 13 insertions(+), 7 deletions(-)

arch/arm/include/asm/arch-sunxi/spl.h +7 -1

index ca9a4f9..a0f33b0 100644
@@ -18,6 +18,10 @@
 #define SPL_ADDR		0x0
 #endif
 
+/* The low 8-bits of the 'boot_media' field in the SPL header */
+#define SUNXI_BOOTED_FROM_MMC0	0
+#define SUNXI_BOOTED_FROM_SPI	3
+
 /* boot head definition from sun4i boot code */
 struct boot_file_head {
 	uint32_t b_instruction;	/* one intruction jumping to real code */

@@ -45,7 +49,9 @@ struct boot_file_head {
 		uint8_t spl_signature[4];
 	};
 	uint32_t fel_script_address;
-	uint32_t reserved;		/* padding, align to 32 bytes */
+	uint32_t reserved1[3];
+	uint32_t boot_media;		/* written here by the boot ROM */
+	uint32_t reserved2[5];		/* padding, align to 64 bytes */
 };
 
 #define is_boot0_magic(addr)	(memcmp((void *)addr, BOOT0_MAGIC, 8) == 0)

include/configs/sunxi-common.h +6 -6

index 2406115..e2ee908 100644
@@ -189,14 +189,14 @@
 #define CONFIG_SPL_BOARD_LOAD_IMAGE
 
 #if defined(CONFIG_MACH_SUN9I)
-#define CONFIG_SPL_TEXT_BASE		0x10020		/* sram start+header */
-#define CONFIG_SPL_MAX_SIZE		0x5fe0		/* ? KiB on sun9i */
+#define CONFIG_SPL_TEXT_BASE		0x10040		/* sram start+header */
+#define CONFIG_SPL_MAX_SIZE		0x5fc0		/* ? KiB on sun9i */
 #elif defined(CONFIG_MACH_SUN50I)
-#define CONFIG_SPL_TEXT_BASE		0x10020		/* sram start+header */
-#define CONFIG_SPL_MAX_SIZE		0x7fe0		/* 32 KiB on sun50i */
+#define CONFIG_SPL_TEXT_BASE		0x10040		/* sram start+header */
+#define CONFIG_SPL_MAX_SIZE		0x7fc0		/* 32 KiB on sun50i */
 #else
-#define CONFIG_SPL_TEXT_BASE		0x20		/* sram start+header */
-#define CONFIG_SPL_MAX_SIZE		0x5fe0		/* 24KB on sun4i/sun7i */
+#define CONFIG_SPL_TEXT_BASE		0x40		/* sram start+header */
+#define CONFIG_SPL_MAX_SIZE		0x5fc0		/* 24KB on sun4i/sun7i */
 #endif
 
 #define CONFIG_SPL_LIBDISK_SUPPORT

Given that there now are quite a few additional "reserved" entries, and while we're still at SPL_HEADER_VERSION 1, I'd like to renew my request of dedicating one of these fields to the script length - which would enable us to set the U-Boot ${filesize} accordingly.

i.e.

arch-arm-include-asm-arch-sunxi-spl.new.h renamed from arch-arm-include-asm-arch-sunxi-spl.h +2 -1

@@ -49,7 +49,8 @@
                uint8_t spl_signature[4];
        };
        uint32_t fel_script_address;
-       uint32_t reserved1[3];
+       uint32_t fel_script_length;
+       uint32_t reserved1[2];
        uint32_t boot_media;            /* written here by the boot ROM */
        uint32_t reserved2[5];          /* padding, align to 64 bytes */
 };

I do not intend to further push my specific use cases, however I still consider the (then somewhat theoretical) ability to do "import -t ${fel_script_addr} ${filesize}" useful. For reference, the previous discussion related to this was somewhere around http://lists.denx.de/pipermail/u-boot/2015-September/227454.html

Hi,

Quoted 17 lines · Bernhard Nortmann

Given that there now are quite a few additional "reserved" entries, and while we're still at SPL_HEADER_VERSION 1, I'd like to renew my request of dedicating one of these fields to the script length - which would enable us to set the U-Boot ${filesize} accordingly.

i.e.

arch-arm-include-asm-arch-sunxi-spl.new.h renamed from arch-arm-include-asm-arch-sunxi-spl.h +2 -1

@@ -49,7 +49,8 @@
                uint8_t spl_signature[4];
        };
        uint32_t fel_script_address;
-       uint32_t reserved1[3];
+       uint32_t fel_script_length;
+       uint32_t reserved1[2];
        uint32_t boot_media;            /* written here by the boot ROM */
        uint32_t reserved2[5];          /* padding, align to 64 bytes */
 };

Quoted 1 line

I do not intend to further push my specific use cases, however I still consider the (then somewhat theoretical) ability to do "import -t ${fel_script_addr} ${filesize}" useful. For reference, the previous discussion related to this was somewhere around http://lists.denx.de/pipermail/u-boot/2015-September/227454.html

Hmm, given that the boot-rom touches some of these, I wonder if we should be putting anything here at all.

Other then that worry, I see no problem with adding a fel_script_length, Siarhei what is your opinion on this ?

Quoted 3 lines · On Mon, 16 May 2016 19:52:33 +0200 Hans de Goede <[email protected]> wrote:

[...]
Other then that worry, I see no problem with adding a fel_script_length, Siarhei what is your opinion on this ?

I personally have no objections.

Am 02.06.2016 um 16:57 schrieb Siarhei Siamashka:

Quoted 6 lines

Quoted 3 lines · On Mon, 16 May 2016 19:52:33 +0200 Hans de Goede <[email protected]> wrote:

[...]
Other then that worry, I see no problem with adding a fel_script_length, Siarhei what is your opinion on this ?

Quoted 2 lines

I personally have no objections.

Does it make sense if I submit a patch for this, or should we simply squash that one-line change into your "Store the device tree name in the SPL header" series?

Convert one of the "reserved" fields in the sunxi SPL header to a fel_script_length entry. That enables the sunxi-fel utility to pass the script size when booting over USB ("FEL mode").

If board.c encounters a non-zero value in this header field, it will set U-Boot's "filesize" environment variable accordingly.

sunxi-fel currently doesn't use this field (i.e. fel_script_length will remain 0), but it would allow for new use cases, e.g. passing tweaked/additional settings via a text file (uEnv.txt style), and then using "import -t ${fel_script_addr} ${filesize}" on them.

Signed-off-by Bernhard Nortmann <[email protected]>

---

 arch/arm/include/asm/arch-sunxi/spl.h | 3 ++-
 board/sunxi/board.c                   | 8 +++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

arch/arm/include/asm/arch-sunxi/spl.h +2 -1

index a0f33b0..0e18438 100644
@@ -49,7 +49,8 @@ struct boot_file_head {
 		uint8_t spl_signature[4];
 	};
 	uint32_t fel_script_address;
-	uint32_t reserved1[3];
+	uint32_t fel_script_length;
+	uint32_t reserved1[2];
 	uint32_t boot_media;		/* written here by the boot ROM */
 	uint32_t reserved2[5];		/* padding, align to 64 bytes */
 };

board/sunxi/board.c +0 -0

index d09cf6d..cf0ff33 100644
@@ -585,9 +585,15 @@ static void parse_spl_header(const uint32_t spl_addr)
 	if (memcmp(spl->spl_signature, SPL_SIGNATURE, 3) == 0) {
 		uint8_t spl_header_version = spl->spl_signature[3];

Hello Bernhard,

[...]
How does this work in general with "boot.scr" and "uEnv.txt" use cases? Could you provide a bit more detailed description?

I mean, who is going to do "import -t ${fel_script_addr} ${filesize}" invocation?

This particular FEL feature in the SPL header is used to allow running a boot script provided by the user (boot.scr). Without it, we only have the default U-Boot environment. And the default U-Boot environment does not have the "import -t ${fel_script_addr} ${filesize}" statement yet. This looks a bit like a chicken/egg problem or am I missing something?

I have no real opinion about this, but "filesize" looks like a rather generic name for this environment variable. Are there some advantages/disadvantages picking this particular name instead of something like "fel_scriptsize"?

That said, I have no objections to supporting "uEnv.txt" for FEL boot, as long as it works correctly and does not regress the existing "boot.scr" support.

Hi Siarhei!

Am 05.06.2016 um 13:44 schrieb Siarhei Siamashka:

Quoted 9 lines

Hello Bernhard,

[...]
How does this work in general with "boot.scr" and "uEnv.txt" use cases? Could you provide a bit more detailed description?

I mean, who is going to do "import -t ${fel_script_addr} ${filesize}" invocation?

Quoted 6 lines

This particular FEL feature in the SPL header is used to allow running a boot script provided by the user (boot.scr). Without it, we only have the default U-Boot environment. And the default U-Boot environment does not have the "import -t ${fel_script_addr} ${filesize}" statement yet. This looks a bit like a chicken/egg problem or am I missing something?

No, you're right and not missing anything. Setting ${filesize} alone doesn't achieve much, and would require further customization to do the actual import. Since this whole idea of having the script length didn't go down too well when I initially proposed it (back in September 2015), I stayed away from adding additional U-Boot modifications this time.

One approach would be to have a modified "bootcmd_fel" that either tests for a non-empty ${filesize}, or tries to import the script data as a fallback ("source ${fel_scriptaddr} || import -t <...>;"). Another way is to always assume "uEnv.txt style" whenever fel_script_length is set, and do a himport_r() of the script data right away (the programmatic equivalent of "import -t"). I'm doing this in an experimental branch of mine, as it allows overriding everything from the default environment - including the "bootcmd" itself.

Of course, all of this also requires support from the sunxi-fel side of things (i.e. fel_script_length getting set in the first place). A quick-and-dirty solution I'm currently using is to assume a uEnv.txt script (and set fel_script_length) whenever sunxi-fel detects uploads of pure ASCII data.

This could also be done easily with a dedicated command,
and I can even image sunxi-fel building the uEnv.txt string itself from given ("env") key/value pairs.

Quoted 4 lines

I have no real opinion about this, but "filesize" looks like a rather generic name for this environment variable. Are there some advantages/disadvantages picking this particular name instead of something like "fel_scriptsize"?

Well... this _is_ the generic name that U-Boot itself tends to use for data transfers. E.g. "load" or "tftp" commands set the ${filesize} too.

Quoted 5 lines

That said, I have no objections to supporting "uEnv.txt" for FEL boot, as long as it works correctly and does not regress the existing "boot.scr" support.

Our sunxi-fel utility is already testing for the script.bin format and can preserve the existing functionality by simply forcing fel_script_length to 0 in that case. And additional safeguards might be placed before "actively" setting that value to anything non-zero.

Quoted 4 lines · On Sun, 5 Jun 2016 15:01:30 +0200 Bernhard Nortmann <[email protected]>…

Hi Siarhei!

[...]

Quoted 5 lines

No, you're right and not missing anything. Setting ${filesize} alone doesn't achieve much, and would require further customization to do the actual import. Since this whole idea of having the script length didn't go down too well when I initially proposed it (back in September 2015), I stayed away from adding additional U-Boot modifications this time.

Maybe you can add the necessary changes to the U-Boot default environment in the v2 patch? Either way, we are not going to make any progress until it is feature complete and usable.

Quoted 10 lines

One approach would be to have a modified "bootcmd_fel" that either tests for a non-empty ${filesize}, or tries to import the script data as a fallback ("source ${fel_scriptaddr} || import -t <...>;"). Another way is to always assume "uEnv.txt style" whenever fel_script_length is set, and do a himport_r() of the script data right away (the programmatic equivalent of "import -t"). I'm doing this in an experimental branch of mine, as it allows overriding everything from the default environment - including the "bootcmd" itself.

Quoted 5 lines

Of course, all of this also requires support from the sunxi-fel side of things (i.e. fel_script_length getting set in the first place). A quick-and-dirty solution I'm currently using is to assume a uEnv.txt script (and set fel_script_length) whenever sunxi-fel detects uploads of pure ASCII data.

The boot.scr file is nice because it has its own format and a magic signature. The uEnv.txt is difficult to recognize automatically, but maybe we can borrow the https://en.wikipedia.org/wiki/Shebang_%28Unix%29
approach used by scripting languages?

I mean, we can require that the first line of uEnv.txt is a comment in a special format. This can be described in the sunxi-tools documentation. And also the sunxi-fel tool could print a warning if it detects upload of pure ASCII data from a file with "uEnv" part in the name and ".txt" as the extension.

Quoted 9 lines

This could also be done easily with a dedicated command,
and I can even image sunxi-fel building the uEnv.txt string itself from given ("env") key/value pairs.

Quoted 4 lines

I have no real opinion about this, but "filesize" looks like a rather generic name for this environment variable. Are there some advantages/disadvantages picking this particular name instead of something like "fel_scriptsize"?

Quoted 2 lines

Well... this _is_ the generic name that U-Boot itself tends to use for data transfers. E.g. "load" or "tftp" commands set the ${filesize} too.

So you are suggesting to pretend that there was a "load" command executed for "uEnv.txt" right before running the code from the default environment? This seems to be rather fragile and does not look like it offers any real advantages over "fel_scriptsize".

Quoted 5 lines

Quoted 5 lines

That said, I have no objections to supporting "uEnv.txt" for FEL boot, as long as it works correctly and does not regress the existing "boot.scr" support.

Quoted 4 lines

Our sunxi-fel utility is already testing for the script.bin format and can preserve the existing functionality by simply forcing fel_script_length to 0 in that case. And additional safeguards might be placed before "actively" setting that value to anything non-zero.

So it would serve both as the uEnv.txt length and also as the format type indicator (boot.scr or uEnv.txt)? This is probably okay if it is documented properly.

Hello Siarhei!

Am 06.06.2016 um 11:20 schrieb Siarhei Siamashka:

Quoted 7 lines

Quoted 4 lines · On Sun, 5 Jun 2016 15:01:30 +0200 Bernhard Nortmann <[email protected]>…

Hi Siarhei!

[...]

Quoted 8 lines

Quoted 5 lines

No, you're right and not missing anything. Setting ${filesize} alone doesn't achieve much, and would require further customization to do the actual import. Since this whole idea of having the script length didn't go down too well when I initially proposed it (back in September 2015), I stayed away from adding additional U-Boot modifications this time.

Maybe you can add the necessary changes to the U-Boot default environment in the v2 patch? Either way, we are not going to make any progress until it is feature complete and usable.

Back in 2015 Hans expressed concerns about overcomplicating what is already an exotic use-case. That is why I wanted to keep v1 as simple and 'generic' as possible - working from the assumption that if a user is sufficiently advanced to get fel_script_length set, (s)he'd also be proficient enough to tailor U-Boot to make actual use of ${filesize} according to his/her needs.

Changing the default environment to use the existing "import -t" command was just one example of what might be done - maybe there could be other future uses, which I currently haven't thought of. My basic goal was and is a way to pass a payload from sunxi-fel to U-Boot in a format-agnostic way. This requires a "length" / file size in addition to the memory address.

The discussion here seems to narrow down on "uEnv.txt style" usage now. That's okay for me - I can create a v2 which focuses on that, and fleshes out the needed details.

Quoted 19 lines

Quoted 9 lines

One approach would be to have a modified "bootcmd_fel" that either tests for a non-empty ${filesize}, or tries to import the script data as a fallback ("source ${fel_scriptaddr} || import -t <...>;"). Another way is to always assume "uEnv.txt style" whenever fel_script_length is set, and do a himport_r() of the script data right away (the programmatic equivalent of "import -t"). I'm doing this in an experimental branch of mine, as it allows overriding everything from the default environment - including the "bootcmd" itself.

Quoted 5 lines

Of course, all of this also requires support from the sunxi-fel side of things (i.e. fel_script_length getting set in the first place). A quick-and-dirty solution I'm currently using is to assume a uEnv.txt script (and set fel_script_length) whenever sunxi-fel detects uploads of pure ASCII data.

The boot.scr file is nice because it has its own format and a magic signature. The uEnv.txt is difficult to recognize automatically, but maybe we can borrow the https://en.wikipedia.org/wiki/Shebang_%28Unix%29
approach used by scripting languages?

Quoted 5 lines

I mean, we can require that the first line of uEnv.txt is a comment in a special format. This can be described in the sunxi-tools documentation. And also the sunxi-fel tool could print a warning if it detects upload of pure ASCII data from a file with "uEnv" part in the name and ".txt" as the extension.

I dislike involving filename conventions here, and I'd also prefer not having to "tag" the uEnv-style files (which are basically <key>=<value> pairs) with a special marker.

If sunxi-fel requires 'extra work' to
recognise these files, we might just as well use a dedicated command for uploading them - e.g. "env" instead of "write". This command could also do sanity checks, like issue a warning if the file contains non-ASCII data or fails to meet the expected syntax.

Quoted 8 lines

Quoted 8 lines

This could also be done easily with a dedicated command,
and I can even image sunxi-fel building the uEnv.txt string itself from given ("env") key/value pairs.

Quoted 4 lines

I have no real opinion about this, but "filesize" looks like a rather generic name for this environment variable. Are there some advantages/disadvantages picking this particular name instead of something like "fel_scriptsize"?

Quoted 6 lines

Quoted 2 lines

Well... this _is_ the generic name that U-Boot itself tends to use for data transfers. E.g. "load" or "tftp" commands set the ${filesize} too.

So you are suggesting to pretend that there was a "load" command executed for "uEnv.txt" right before running the code from the default environment? This seems to be rather fragile and does not look like it offers any real advantages over "fel_scriptsize".

It's based on the paradigm that any kind of data transfer (might) provide exactly this kind of information in ${filesize}. U-Boot users know this from a variety of "load" commands (Ymodem anyone?) - so if you like: yes, I'm pretending that something similar happened. Of course I can rename it to anything that you prefer. But if we're taking the uEnv route, we might easily do away with setting any dedicated environment variable at all (see below).

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

That said, I have no objections to supporting "uEnv.txt" for FEL boot, as long as it works correctly and does not regress the existing "boot.scr" support.

Our sunxi-fel utility is already testing for the script.bin format and can preserve the existing functionality by simply forcing fel_script_length to 0 in that case. And additional safeguards might be placed before "actively" setting that value to anything non-zero.

Quoted 4 lines

So it would serve both as the uEnv.txt length and also as the format type indicator (boot.scr or uEnv.txt)? This is probably okay if it is documented properly.

It would be trival for sunxi-fel to pass the size of .scr files, too - there's just no point in doing so, since this information is already redundant (available from the header anyway).

Setting the field to 0 seemed natural to me, and it also reflects what existing versions of the sunxi-fel utility would use (as they only know about setting fel_script_addr).

Actually that might be the right idea to take the whole thing forward, if I modify some of my working assumptions accordingly:

* Redefine "fel_script_length" as "fel_env_size", and associate a 'format type indicator' meaning with it. Any non-zero value will also require the fel_script_addr to be specified, and signals uEnv-style data suitable for "import -t" (i.e. ASCII text with <key>=<value> lines).

* (fel_env_size == 0 && fel_script_addr != 0) means that U-Boot is safe to assume a .scr format was passed. This reflects and preserves the previous use case and existing sunxi-fel implementations. New sunxi-fel versions will make sure to pass (fel_env_size == 0) whenever they detect the transfers of a mkimage-type script.

* sunxi-fel will pass a non-zero fel_env_size if (and only if) requirements and/or safety checks are met, in a way that makes it safe for U-Boot to rely on the assumption of uEnv-style format. This may range from simple user request ("env" command) to actual syntax validation.

* If both fel_script_addr and fel_env_size are non-zero, U-Boot will auto-import the data right away upon start, and afterwards present a modified environment (merge of the default environment with anything the 'script' sets/overrides). This eliminates the need to further modify default env settings (e.g. sneak in "import -t" to some bootcmd) and also avoids setting dedicated environment variables just for this purpose.

The patch converts one of the "reserved" fields in the sunxi SPL header to a fel_uEnv_length entry. When booting over USB ("FEL mode"), this enables the sunxi-fel utility to pass the string length of uEnv.txt compatible data; at the same time requesting that this data be imported into the U-Boot environment.

If parse_spl_header() in the sunxi board.c encounters a non-zero value in this header field, it will therefore call himport_r() to merge the string (lines) passed via FEL into the default settings. Environment vars can be changed this way even before U-Boot will attempt to autoboot - specifically, this also allows overriding "bootcmd".

With fel_script_addr set and a zero fel_uEnv_length, U-Boot is safe to assume that data in .scr format (a mkimage-type script) was passed at fel_script_addr, and will handle it using the existing mechanism ("bootcmd_fel").

Signed-off-by Bernhard Nortmann <[email protected]>

---
A corresponding proof-of-concept version of sunxi-fel is available from my https://github.com/n1tehawk/sunxi-tools/tree/20160608_uEnv-magic
branch. I've picked up the suggestion to use a "#=uEnv" magic string to request that a file be treated as uEnv.txt-style data.

For example, use your text editor to save a my.env file with

#=uEnv
myvar=world
bootcmd=echo "Hello $myvar."

and then test it with
./sunxi-fel uboot u-boot-sunxi-with-spl.bin write 0x43100000 my.env

You should see U-Boot's autoboot print the corresponding message and drop you to the prompt, proving that you have successfully overwritten the "bootcmd".

Changes in v2:
- Patch renamed to something more suitable (was "sunxi: Add the ability to pass (script) filesize in the SPL header")
- The data field is now named fel_uEnv_length, and comes with a corresponding description in spl.h
- Instead of simply passing file size, fel_uEnv_length is now associated with uEnv.txt format. The patch modifies U-Boot's sunxi parse_spl_header() to auto-import such data.

 arch/arm/include/asm/arch-sunxi/spl.h |  9 ++++++++-
 board/sunxi/board.c                   | 30 ++++++++++++++++++++++--------
 2 files changed, 30 insertions(+), 9 deletions(-)

arch/arm/include/asm/arch-sunxi/spl.h +8 -1

index a0f33b0..a966a88 100644
@@ -49,7 +49,14 @@ struct boot_file_head {
 		uint8_t spl_signature[4];
 	};
 	uint32_t fel_script_address;
-	uint32_t reserved1[3];
+	/*
+	 * If the fel_uEnv_length member below is set to a non-zero value,
+	 * it specifies the size (byte count) of data at fel_script_address.
+	 * At the same time this indicates that the data is in uEnv.txt
+	 * compatible format, ready to be imported via "env import -t".
+	 */
+	uint32_t fel_uEnv_length;
+	uint32_t reserved1[2];
 	uint32_t boot_media;		/* written here by the boot ROM */
 	uint32_t reserved2[5];		/* padding, align to 64 bytes */
 };

board/sunxi/board.c +22 -8

index d09cf6d..fc57e60 100644
@@ -573,6 +573,7 @@ void get_board_serial(struct tag_serialnr *serialnr)
 
 #if !defined(CONFIG_SPL_BUILD)
 #include <asm/arch/spl.h>
+#include <environment.h>
 
 /*
  * Check the SPL header for the "sunxi" variant. If found: parse values

@@ -582,17 +583,30 @@ void get_board_serial(struct tag_serialnr *serialnr)
 static void parse_spl_header(const uint32_t spl_addr)
 {
 	struct boot_file_head *spl = (void *)(ulong)spl_addr;
-	if (memcmp(spl->spl_signature, SPL_SIGNATURE, 3) == 0) {
-		uint8_t spl_header_version = spl->spl_signature[3];
-		if (spl_header_version == SPL_HEADER_VERSION) {
-			if (spl->fel_script_address)
-				setenv_hex("fel_scriptaddr",
-					   spl->fel_script_address);
-			return;
-		}
+	if (memcmp(spl->spl_signature, SPL_SIGNATURE, 3) != 0)
+		return; /* signature mismatch, no usable header */
+
+	uint8_t spl_header_version = spl->spl_signature[3];
+	if (spl_header_version != SPL_HEADER_VERSION) {
 		printf("sunxi SPL version mismatch: expected %u, got %u\n",
 		       SPL_HEADER_VERSION, spl_header_version);
+		return;
+	}
+	if (!spl->fel_script_address)
+		return;
+
+	if (spl->fel_uEnv_length != 0) {
+		/*
+		 * data is expected in uEnv.txt compatible format, so "env
+		 * import -t" the string(s) at fel_script_address right away.
+		 */
+		himport_r(&env_htab, (char *)spl->fel_script_address,
+			  spl->fel_uEnv_length, '\n', H_NOCLEAR, 0, 0, NULL);
+		return;
 	}
+	/* otherwise assume .scr format (mkimage-type script) */
+	setenv_hex("fel_scriptaddr", spl->fel_script_address);
+	return;
 }
 #endif
 

Hi,
[...]

This patch looks good to me.

Siarhei any comments from your side ? If not then I'll add this to u-boot-sunxi/next.

Regards,

Hans

Hi,

Quoted 18 lines · Bernhard Nortmann

The patch converts one of the "reserved" fields in the sunxi SPL header to a fel_uEnv_length entry. When booting over USB ("FEL mode"), this enables the sunxi-fel utility to pass the string length of uEnv.txt compatible data; at the same time requesting that this data be imported into the U-Boot environment.

If parse_spl_header() in the sunxi board.c encounters a non-zero value in this header field, it will therefore call himport_r() to merge the string (lines) passed via FEL into the default settings. Environment vars can be changed this way even before U-Boot will attempt to autoboot - specifically, this also allows overriding "bootcmd".

With fel_script_addr set and a zero fel_uEnv_length, U-Boot is safe to assume that data in .scr format (a mkimage-type script) was passed at fel_script_addr, and will handle it using the existing mechanism ("bootcmd_fel").

Quoted 1 line

Signed-off-by Bernhard Nortmann <[email protected]>

This patch looks good to me.

Siarhei any comments from your side ? If not then I'll add this to u-boot-sunxi/next.

Quoted 103 lines

---
A corresponding proof-of-concept version of sunxi-fel is available from my https://github.com/n1tehawk/sunxi-tools/tree/20160608_uEnv-magic
branch. I've picked up the suggestion to use a "#=uEnv" magic string to request that a file be treated as uEnv.txt-style data.

For example, use your text editor to save a my.env file with

#=uEnv
myvar=world
bootcmd=echo "Hello $myvar."

and then test it with
./sunxi-fel uboot u-boot-sunxi-with-spl.bin write 0x43100000 my.env

You should see U-Boot's autoboot print the corresponding message and drop you to the prompt, proving that you have successfully overwritten the "bootcmd".

Changes in v2:
- Patch renamed to something more suitable (was "sunxi: Add the ability to pass (script) filesize in the SPL header")
- The data field is now named fel_uEnv_length, and comes with a corresponding description in spl.h
- Instead of simply passing file size, fel_uEnv_length is now associated with uEnv.txt format. The patch modifies U-Boot's sunxi parse_spl_header() to auto-import such data.

 arch/arm/include/asm/arch-sunxi/spl.h |  9 ++++++++-
 board/sunxi/board.c                   | 30 ++++++++++++++++++++++--------
 2 files changed, 30 insertions(+), 9 deletions(-)

arch/arm/include/asm/arch-sunxi/spl.h +8 -1

index a0f33b0..a966a88 100644
@@ -49,7 +49,14 @@ struct boot_file_head {
 		uint8_t spl_signature[4];
 	};
 	uint32_t fel_script_address;
-	uint32_t reserved1[3];
+	/*
+	 * If the fel_uEnv_length member below is set to a non-zero value,
+	 * it specifies the size (byte count) of data at fel_script_address.
+	 * At the same time this indicates that the data is in uEnv.txt
+	 * compatible format, ready to be imported via "env import -t".
+	 */
+	uint32_t fel_uEnv_length;
+	uint32_t reserved1[2];
 	uint32_t boot_media;		/* written here by the boot ROM */
 	uint32_t reserved2[5];		/* padding, align to 64 bytes */
 };

board/sunxi/board.c +22 -8

index d09cf6d..fc57e60 100644
@@ -573,6 +573,7 @@ void get_board_serial(struct tag_serialnr *serialnr)
 
 #if !defined(CONFIG_SPL_BUILD)
 #include <asm/arch/spl.h>
+#include <environment.h>
 
 /*
  * Check the SPL header for the "sunxi" variant. If found: parse values

@@ -582,17 +583,30 @@ void get_board_serial(struct tag_serialnr *serialnr)
 static void parse_spl_header(const uint32_t spl_addr)
 {
 	struct boot_file_head *spl = (void *)(ulong)spl_addr;
-	if (memcmp(spl->spl_signature, SPL_SIGNATURE, 3) == 0) {
-		uint8_t spl_header_version = spl->spl_signature[3];
-		if (spl_header_version == SPL_HEADER_VERSION) {
-			if (spl->fel_script_address)
-				setenv_hex("fel_scriptaddr",
-					   spl->fel_script_address);
-			return;
-		}
+	if (memcmp(spl->spl_signature, SPL_SIGNATURE, 3) != 0)
+		return; /* signature mismatch, no usable header */
+
+	uint8_t spl_header_version = spl->spl_signature[3];
+	if (spl_header_version != SPL_HEADER_VERSION) {
 		printf("sunxi SPL version mismatch: expected %u, got %u\n",
 		       SPL_HEADER_VERSION, spl_header_version);
+		return;
+	}
+	if (!spl->fel_script_address)
+		return;
+
+	if (spl->fel_uEnv_length != 0) {
+		/*
+		 * data is expected in uEnv.txt compatible format, so "env
+		 * import -t" the string(s) at fel_script_address right away.
+		 */
+		himport_r(&env_htab, (char *)spl->fel_script_address,
+			  spl->fel_uEnv_length, '\n', H_NOCLEAR, 0, 0, NULL);
+		return;
 	}
+	/* otherwise assume .scr format (mkimage-type script) */
+	setenv_hex("fel_scriptaddr", spl->fel_script_address);
+	return;
 }
 #endif

The patch converts one of the "reserved" fields in the sunxi SPL header to a fel_uEnv_length entry. When booting over USB ("FEL mode"), this enables the sunxi-fel utility to pass the string length of uEnv.txt compatible data; at the same time requesting that this data be imported into the U-Boot environment.

If parse_spl_header() in the sunxi board.c encounters a non-zero value in this header field, it will therefore call himport_r() to merge the string (lines) passed via FEL into the default settings. Environment vars can be changed this way even before U-Boot will attempt to autoboot - specifically, this also allows overriding "bootcmd".

With fel_script_addr set and a zero fel_uEnv_length, U-Boot is safe to assume that data in .scr format (a mkimage-type script) was passed at fel_script_addr, and will handle it using the existing mechanism ("bootcmd_fel").

Signed-off-by Bernhard Nortmann <[email protected]>

Acked-by Siarhei Siamashka <[email protected]>

---
A corresponding proof-of-concept version of sunxi-fel is available from my https://github.com/n1tehawk/sunxi-tools/tree/20160608_uEnv-magic
branch. I've picked up the suggestion to use a "#=uEnv" magic string to request that a file be treated as uEnv.txt-style data.

For example, use your text editor to save a my.env file with

#=uEnv
myvar=world
bootcmd=echo "Hello $myvar."

and then test it with
./sunxi-fel uboot u-boot-sunxi-with-spl.bin write 0x43100000 my.env

You should see U-Boot's autoboot print the corresponding message and drop you to the prompt, proving that you have successfully overwritten the "bootcmd".

Changes in v3:
- Dropped a surplus "return;" at the end of parse_spl_header()
- Have "FEL" in the subject line to clarify the scope
- Added "Acked-by" line

Changes in v2:
- Patch renamed to something more suitable (was "sunxi: Add the ability to pass (script) filesize in the SPL header")
- The data field is now named fel_uEnv_length, and comes with a corresponding description in spl.h
- Instead of simply passing file size, fel_uEnv_length is now associated with uEnv.txt format. The patch modifies U-Boot's sunxi parse_spl_header() to auto-import such data.

 arch/arm/include/asm/arch-sunxi/spl.h |  9 ++++++++-
 board/sunxi/board.c                   | 29 +++++++++++++++++++++--------
 2 files changed, 29 insertions(+), 9 deletions(-)

arch/arm/include/asm/arch-sunxi/spl.h +8 -1

index a0f33b0..a966a88 100644
@@ -49,7 +49,14 @@ struct boot_file_head {
 		uint8_t spl_signature[4];
 	};
 	uint32_t fel_script_address;
-	uint32_t reserved1[3];
+	/*
+	 * If the fel_uEnv_length member below is set to a non-zero value,
+	 * it specifies the size (byte count) of data at fel_script_address.
+	 * At the same time this indicates that the data is in uEnv.txt
+	 * compatible format, ready to be imported via "env import -t".
+	 */
+	uint32_t fel_uEnv_length;
+	uint32_t reserved1[2];
 	uint32_t boot_media;		/* written here by the boot ROM */
 	uint32_t reserved2[5];		/* padding, align to 64 bytes */
 };

board/sunxi/board.c +21 -8

index d09cf6d..b5a50f4 100644
@@ -573,6 +573,7 @@ void get_board_serial(struct tag_serialnr *serialnr)
 
 #if !defined(CONFIG_SPL_BUILD)
 #include <asm/arch/spl.h>
+#include <environment.h>
 
 /*
  * Check the SPL header for the "sunxi" variant. If found: parse values

@@ -582,17 +583,29 @@ void get_board_serial(struct tag_serialnr *serialnr)
 static void parse_spl_header(const uint32_t spl_addr)
 {
 	struct boot_file_head *spl = (void *)(ulong)spl_addr;
-	if (memcmp(spl->spl_signature, SPL_SIGNATURE, 3) == 0) {
-		uint8_t spl_header_version = spl->spl_signature[3];
-		if (spl_header_version == SPL_HEADER_VERSION) {
-			if (spl->fel_script_address)
-				setenv_hex("fel_scriptaddr",
-					   spl->fel_script_address);
-			return;
-		}
+	if (memcmp(spl->spl_signature, SPL_SIGNATURE, 3) != 0)
+		return; /* signature mismatch, no usable header */
+
+	uint8_t spl_header_version = spl->spl_signature[3];
+	if (spl_header_version != SPL_HEADER_VERSION) {
 		printf("sunxi SPL version mismatch: expected %u, got %u\n",
 		       SPL_HEADER_VERSION, spl_header_version);
+		return;
+	}
+	if (!spl->fel_script_address)
+		return;
+
+	if (spl->fel_uEnv_length != 0) {
+		/*
+		 * data is expected in uEnv.txt compatible format, so "env
+		 * import -t" the string(s) at fel_script_address right away.
+		 */
+		himport_r(&env_htab, (char *)spl->fel_script_address,
+			  spl->fel_uEnv_length, '\n', H_NOCLEAR, 0, 0, NULL);
+		return;
 	}
+	/* otherwise assume .scr format (mkimage-type script) */
+	setenv_hex("fel_scriptaddr", spl->fel_script_address);
 }
 #endif