[U-Boot] [PATCH v2] sunxi: Increase SPL header size to 64 bytes to avoid code corruption

[U-Boot] [PATCH v2] sunxi: Increase SPL header size to 64 bytes to avoid code corruption

[Siarhei Siamashka]

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 <siarhei.siamashka@gmail.com>
---

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(-)

diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
index ca9a4f9..a0f33b0 100644
--- a/arch/arm/include/asm/arch-sunxi/spl.h
+++ b/arch/arm/include/asm/arch-sunxi/spl.h
@@ -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)
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 2406115..e2ee908 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -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
-- 
2.7.3

[U-Boot] [PATCH v2] sunxi: Increase SPL header size to 64 bytes to avoid code corruption

[Hans de Goede]

Hi,

On 14-05-16 03:13, Siarhei Siamashka wrote:
> 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 <siarhei.siamashka@gmail.com>

Thanks, looks good to me applied to:

http://git.denx.de/?p=u-boot/u-boot-sunxi.git;a=shortlog;h=refs/heads/next

This will be part of my first pull-req for u-boot v2016.07.

Regards,

Hans



> ---
>
> 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(-)
>
> diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
> index ca9a4f9..a0f33b0 100644
> --- a/arch/arm/include/asm/arch-sunxi/spl.h
> +++ b/arch/arm/include/asm/arch-sunxi/spl.h
> @@ -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)
> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index 2406115..e2ee908 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -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
>

[U-Boot] [PATCH v2] sunxi: Increase SPL header size to 64 bytes to avoid code corruption

[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.h
+++ arch-arm-include-asm-arch-sunxi-spl.new.h
@@ -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

Regards, B. Nortmann

[U-Boot] [PATCH v2] sunxi: Increase SPL header size to 64 bytes to avoid code corruption

[Hans de Goede]

Hi,

On 16-05-16 11:56, Bernhard Nortmann wrote:
> 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.h
> +++ arch-arm-include-asm-arch-sunxi-spl.new.h
> @@ -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

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 ?

Regards,

Hans

[U-Boot] [PATCH v2] sunxi: Increase SPL header size to 64 bytes to avoid code corruption

[Siarhei Siamashka]

On Mon, 16 May 2016 19:52:33 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 16-05-16 11:56, Bernhard Nortmann wrote:
> > 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.h
> > +++ arch-arm-include-asm-arch-sunxi-spl.new.h
> > @@ -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  
> 
> Hmm, given that the boot-rom touches some of these, I wonder if
> we should be putting anything here at all.

Yes, this came as a bit of surprise because this was not clearly
documented anywhere. Still it looks like that's just a single
byte getting modified, albeit at a bit strange location.

BTW, do you remember what I said earlier about not always being in
perfect control?

    http://lists.denx.de/pipermail/u-boot/2015-September/228727.html

This particular issue just serves as a very nice demonstration :-)

Anyway, I think that we are already reasonably well prepared to handle
it. The worst thing that can happen is that the boot ROM in the future
Allwinner SoCs starts patching even more bytes in the header or moves
this boot device id variable to some other address. If/when this
happens, we can always update the SPL header format (do the "major"
version change trick).

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

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH v2] sunxi: Increase SPL header size to 64 bytes to avoid code corruption

[Bernhard Nortmann]

Am 02.06.2016 um 16:57 schrieb Siarhei Siamashka:
> On Mon, 16 May 2016 19:52:33 +0200
> Hans de Goede <hdegoede@redhat.com> 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.
>

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?

Regards, B. Nortmann

[U-Boot] [PATCH v2] sunxi: Increase SPL header size to 64 bytes to avoid code corruption

[Hans de Goede]

Hi,

On 03-06-16 09:30, Bernhard Nortmann wrote:
> Am 02.06.2016 um 16:57 schrieb Siarhei Siamashka:
>> On Mon, 16 May 2016 19:52:33 +0200
>> Hans de Goede <hdegoede@redhat.com> 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.
>>
>
> 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?

This seems like an unrelated change to me and as such should be
a separate patch.

Regards,

Hans

[U-Boot] [PATCH] sunxi: Add the ability to pass (script) filesize in the SPL header

[Bernhard Nortmann]

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 <bernhard.nortmann@web.de>

---

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

diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
index a0f33b0..0e18438 100644
--- a/arch/arm/include/asm/arch-sunxi/spl.h
+++ b/arch/arm/include/asm/arch-sunxi/spl.h
@@ -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 */
 };
diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index d09cf6d..cf0ff33 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -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];
 		if (spl_header_version == SPL_HEADER_VERSION) {
-			if (spl->fel_script_address)
+			if (spl->fel_script_address) {
 				setenv_hex("fel_scriptaddr",
 					   spl->fel_script_address);
+				if (spl->fel_script_length)
+					setenv_hex("filesize",
+						   spl->fel_script_length);
+				else
+					setenv("filesize", NULL);
+			}
 			return;
 		}
 		printf("sunxi SPL version mismatch: expected %u, got %u\n",
-- 
2.7.3

[U-Boot] [PATCH] sunxi: Add the ability to pass (script) filesize in the SPL header

[Siarhei Siamashka]

Hello Bernhard,

On Sat,  4 Jun 2016 19:12:20 +0200
Bernhard Nortmann <bernhard.nortmann@web.de> wrote:

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

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?

> 
> Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
> 
> ---
> 
>  arch/arm/include/asm/arch-sunxi/spl.h | 3 ++-
>  board/sunxi/board.c                   | 8 +++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
> index a0f33b0..0e18438 100644
> --- a/arch/arm/include/asm/arch-sunxi/spl.h
> +++ b/arch/arm/include/asm/arch-sunxi/spl.h
> @@ -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 */
>  };
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index d09cf6d..cf0ff33 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -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];
>  		if (spl_header_version == SPL_HEADER_VERSION) {
> -			if (spl->fel_script_address)
> +			if (spl->fel_script_address) {
>  				setenv_hex("fel_scriptaddr",
>  					   spl->fel_script_address);
> +				if (spl->fel_script_length)
> +					setenv_hex("filesize",
> +						   spl->fel_script_length);
> +				else
> +					setenv("filesize", NULL);

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"?

> +			}
>  			return;
>  		}
>  		printf("sunxi SPL version mismatch: expected %u, got %u\n",

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.

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH] sunxi: Add the ability to pass (script) filesize in the SPL header

[Bernhard Nortmann]

Hi Siarhei!

Am 05.06.2016 um 13:44 schrieb Siarhei Siamashka:
> 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?

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.

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

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

Regards, B. Nortmann

[U-Boot] [PATCH] sunxi: Add the ability to pass (script) filesize in the SPL header

[Siarhei Siamashka]

On Sun, 5 Jun 2016 15:01:30 +0200
Bernhard Nortmann <bernhard.nortmann@web.de> wrote:

> Hi Siarhei!
> 
> Am 05.06.2016 um 13:44 schrieb Siarhei Siamashka:
> > 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?  
> 
> 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.

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

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.

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

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".

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

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.

-- 
Regards,
Sier?

[U-Boot] [PATCH] sunxi: Add the ability to pass (script) filesize in the SPL header

[Bernhard Nortmann]

Hello Siarhei!

Am 06.06.2016 um 11:20 schrieb Siarhei Siamashka:
> On Sun, 5 Jun 2016 15:01:30 +0200
> Bernhard Nortmann <bernhard.nortmann@web.de> wrote:
>
>> Hi Siarhei!
>>
>> [...]
>>
>> 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.

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

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.

>
>> 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.
>>
>>> 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.
> 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).

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

Regards, B. Nortmann

[U-Boot] [PATCH] sunxi: Add the ability to pass (script) filesize in the SPL header

[Siarhei Siamashka]

Hello,

On Tue, 7 Jun 2016 16:09:58 +0200
Bernhard Nortmann <bernhard.nortmann@web.de> wrote:

> Hello Siarhei!
> 
> Am 06.06.2016 um 11:20 schrieb Siarhei Siamashka:
> > On Sun, 5 Jun 2016 15:01:30 +0200
> > Bernhard Nortmann <bernhard.nortmann@web.de> wrote:
> >  
> >> Hi Siarhei!
> >>
> >> [...]
> >>
> >> 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.

Out of curiosity, what kind of other payload are you trying to pass to
U-Boot?

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

For the record, I'm personally not very much interested in uEnv.txt
support myself. I was just assuming that it was the purpose of your
patch. But now I'm a bit confused.

Again, I have no objections. But if people really want to have uEnv.txt
support (or something else), then it's better to say so instead of
beating around the bush :-)

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

Still that's not something that I invented myself. As I said, scripting
languages are using it quite successfully. For example, you can find
the line "#!/usr/bin/env python" in the beginning of many python
source files.

And uEnv.txt files could use something similar to make them easier to
detect. Maybe something like "#=uEnv" could be a reasonable choice?

As for the filename conventions, I only proposed this as an optional
hint for the user. As a way to help people to make the transition. If
such heuristics is correct and helpful even in 50% of cases, then it's
good enough.

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

Yes, this is also a reasonable solution.
 
> >> 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.
> >>  
> >>> 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.  
> > 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).

I don't have any preference. I was just wondering if other people
(Hans?) are fine with your choice, which may potentially clash with
the "load" commands in a rather obscure way.

> >  
> >>> 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.  
> > 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).

Yes, it just needs to be clearly documented, so that nothing can be
easily misinterpreted. Right now your current patch looks like this:

--- a/arch/arm/include/asm/arch-sunxi/spl.h
+++ b/arch/arm/include/asm/arch-sunxi/spl.h
@@ -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 */

> 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).

Natural or not, it still needs to be documented. Because different
things may seem to be natural/obvious for different people. See my
comment above.

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

OK. Please just try to formulate it as concise, but comprehensive
comments for the spl.h file.

-- 
Regards,
Sier?

[U-Boot] [PATCH v2] sunxi: Add the ability to recognize and auto-import uEnv-style data

[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").

Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>

---
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(-)

diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
index a0f33b0..a966a88 100644
--- a/arch/arm/include/asm/arch-sunxi/spl.h
+++ b/arch/arm/include/asm/arch-sunxi/spl.h
@@ -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 */
 };
diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index d09cf6d..fc57e60 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -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
 
-- 
2.7.3

[U-Boot] [PATCH v2] sunxi: Add the ability to recognize and auto-import uEnv-style data

[Hans de Goede]

Hi,

On 08-06-16 20:23, Bernhard Nortmann wrote:
> 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 <bernhard.nortmann@web.de>

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



>
> ---
> 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(-)
>
> diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
> index a0f33b0..a966a88 100644
> --- a/arch/arm/include/asm/arch-sunxi/spl.h
> +++ b/arch/arm/include/asm/arch-sunxi/spl.h
> @@ -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 */
>  };
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index d09cf6d..fc57e60 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -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
>
>

[U-Boot] [PATCH v2] sunxi: Add the ability to recognize and auto-import uEnv-style data

[Bernhard Nortmann]

Hi Hans!

Am 08.06.2016 um 22:13 schrieb Hans de Goede:
> 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

Thanks for looking into it. One small thing I only noticed after posting
the patch: The last "return;" at the end of parse_spl_header() is unneeded
and may safely be dropped.

Regards, B. Nortmann

[U-Boot] [PATCH v2] sunxi: Add the ability to recognize and auto-import uEnv-style data

[Siarhei Siamashka]

Hi,

On Wed, 8 Jun 2016 22:13:50 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 08-06-16 20:23, Bernhard Nortmann wrote:
> > 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 <bernhard.nortmann@web.de>  
> 
> This patch looks good to me.
> 
> Siarhei any comments from your side ? If not then I'll add this to
> u-boot-sunxi/next.

Yes, it also looks good to me:
Acked-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>

The only nitpick is about the subject line. It does not mention
FEL and may be a bit misleading.


Regarding the uEnv.txt support in general, I tried to grep U-Boot
sources and found that it's use is very much inconsistent on
different platforms. For example, there seems to be some sort
of leftover junk in sunxi-common.h:

http://git.denx.de/?p=u-boot.git;a=blob;f=include/configs/sunxi-common.h;h=b33cfb86f82e0831f5d19b1e473205f65efb5a96;hb=b104b3dc1dd90cdbf67ccf3c51b06e4f1592fe91#l481

Hardcoding the partition number and the file system type is hardly
something that the users may reasonably expect.

It also might be a good idea to do more or less uniform handling of
the uEnv.txt for both FEL boot and normal SD card boot. If Bernhard
wants to improve the uEnv.txt support in general, then could this
be possibly addressed later (not in this patch)?

Still the "doc/README.distro" mentions boot.scr but has no references
to uEnv.txt (which seems to imply that the uEnv.txt is a second class
citizen).


> >
> > ---
> > 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(-)
> >
> > diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
> > index a0f33b0..a966a88 100644
> > --- a/arch/arm/include/asm/arch-sunxi/spl.h
> > +++ b/arch/arm/include/asm/arch-sunxi/spl.h
> > @@ -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 */
> >  };
> > diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> > index d09cf6d..fc57e60 100644
> > --- a/board/sunxi/board.c
> > +++ b/board/sunxi/board.c
> > @@ -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

[U-Boot] [PATCH v3] sunxi: FEL - Add the ability to recognize and auto-import uEnv-style data

[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").

Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
Acked-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>

---
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(-)

diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
index a0f33b0..a966a88 100644
--- a/arch/arm/include/asm/arch-sunxi/spl.h
+++ b/arch/arm/include/asm/arch-sunxi/spl.h
@@ -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 */
 };
diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index d09cf6d..b5a50f4 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -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
 
-- 
2.7.3

[U-Boot] [U-Boot, v3] sunxi: FEL - Add the ability to recognize and auto-import uEnv-style data

[Hans de Goede]

Hi,

On 09-06-16 07:37, Bernhard Nortmann wrote:
> 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 <bernhard.nortmann@web.de>
> Acked-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>

Thanks, I've added this to u-boot-sunxi/next for merging into
v2016.09 as soon as the merge window for that opens.

Regards,

Hans



> ---
> 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(-)
>
> diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
> index a0f33b0..a966a88 100644
> --- a/arch/arm/include/asm/arch-sunxi/spl.h
> +++ b/arch/arm/include/asm/arch-sunxi/spl.h
> @@ -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 */
>  };
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index d09cf6d..b5a50f4 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -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
>
>