[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 at 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 at 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ž