[U-Boot] [RFC PATCH 0/2] sunxi: support FEL-provided environment vars and "fel" boot target

[U-Boot] [RFC PATCH 0/2] sunxi: support FEL-provided environment vars and "fel" boot target

[Bernhard Nortmann]

This patch series builds upon
http://lists.denx.de/pipermail/u-boot/2015-September/226515.html
and is a first proposal. It will eventually get combined with that
patch for proper submission, but I'd like your thoughts / input on
several topics first:

* Hans de Goede already pointed out that it might be preferable to
make use of the spl_boot_device() function even from the main
(non-SPL) U-Boot binary (e.g. for the "NAND" case). Currently this
function is only available with CONFIG_SPL_BUILD set, and
implemented in arch/arm/cpu/armv7/sunxi/board.c
Would it be safe to enable it for non-SPL builds and use something
like "(spl_boot_device == BOOT_DEVICE_BOARD)" for misc_init_r()?

* The test for FEL boot mode ("eGON.BT0") in spl_boot_device()
explicitly uses the "io" function readl() to access the signature.
Is there a specific reason for this? Marex pointed out (on IRC)
that the SRAM might as well be read directly. For now, I have
followed the existing code, and used readl() and readb() to work
with the SPL header information.

* The check_signature() function somewhat duplicates very similar
code from arch/arm/include/asm/io.h. That particular piece of code
depends on the presence of a "__mem_pci" symbol. However, it seems
'generic' enough - so it may be worth to factor out and reuse it?

* My second patch adds a "fel" boot target to the "distro_bootcmd"
logic. It seems simple enough, but I'm sure it did it 'the right
way'. Any feedback is appreciated.

Regards, B. Nortmann


Bernhard Nortmann (2):
  sunxi: retrieve FEL-provided values to environment variables
  sunxi: add "fel" boot target

 board/sunxi/board.c            | 59 ++++++++++++++++++++++++++++++++++++++++++
 include/configs/sunxi-common.h | 13 ++++++++++
 2 files changed, 72 insertions(+)

-- 
2.4.6

[U-Boot] [RFC PATCH 1/2] sunxi: retrieve FEL-provided values to environment variables

[Bernhard Nortmann]

This patch extends the misc_init_r() function on sunxi boards
to test for the presence of a suitable "sunxi" SPL header. If
found, and the loader ("fel" utility) provided non-zero values
for either data address or size, then corresponding environment
variables get set.

misc_init_r() also sets (or clears) the "fel_booted" variable depending
on the active boot device, using the same logic as spl_boot_device().

The goal is to provide sufficient information (within the U-Boot
environment) to make intelligent decisions on how to continue the boot
process, allowing specific customizations for the "FEL boot" case.

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

 board/sunxi/board.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 9c855f6..ceab1e0 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -516,6 +516,54 @@ void get_board_serial(struct tag_serialnr *serialnr)
 }
 #endif
 
+#if !defined(CONFIG_SPL_BUILD)
+static int check_signature(unsigned long io_addr, const char *signature,
+			   int length)
+{
+	do {
+		if (readb(io_addr) != *signature)
+			return 0;
+		io_addr++;
+		signature++;
+	} while (--length > 0);
+	return 1;
+}
+
+#define SPL_SIGNATURE			"SPL" /* marks "sunxi" header */
+#define SPL_EXPECTED_VERSION		1
+
+/*
+ * Check the SPL header for the "sunxi" variant. If found: parse values
+ * that might have been passed by the loader ("fel" utility), and update
+ * the environment accordingly.
+ */
+static void parse_spl_header(void)
+{
+	uint8_t spl_header_version;
+	uint32_t fel_data_address;
+	uint32_t fel_data_size;
+
+	if (check_signature(0x14, SPL_SIGNATURE, 3)) {
+		spl_header_version = readb(0x17);
+		if (spl_header_version == SPL_EXPECTED_VERSION) {
+			fel_data_address = readl(0x18);
+			fel_data_size = readl(0x1C);
+			if (fel_data_address)
+				setenv_hex("fel_data_addr", fel_data_address);
+			else
+				setenv("fel_data_addr", NULL);
+			if (fel_data_size)
+				setenv_hex("fel_data_size", fel_data_size);
+			else
+				setenv("fel_data_size", NULL);
+		} else {
+			printf("sunxi SPL header version mismatch: expected %u, got %u\n",
+			       SPL_EXPECTED_VERSION, spl_header_version);
+		}
+	}
+}
+#endif /* !defined(CONFIG_SPL_BUILD) */
+
 #ifdef CONFIG_MISC_INIT_R
 int misc_init_r(void)
 {
@@ -524,6 +572,17 @@ int misc_init_r(void)
 	uint8_t mac_addr[6];
 	int ret;
 
+#if !defined(CONFIG_SPL_BUILD)
+	/* determine if we are running in FEL mode */
+	if (readl(4) != 0x4E4F4765 || readl(8) != 0x3054422E) { /* eGON.BT0 */
+		setenv("fel_booted", "1");
+		parse_spl_header();
+	} else {
+		/* not in FEL mode, delete/clear env var (if present) */
+		setenv("fel_booted", NULL);
+	}
+#endif
+
 	ret = sunxi_get_sid(sid);
 	if (ret == 0 && sid[0] != 0 && sid[3] != 0) {
 		if (!getenv("ethaddr")) {
-- 
2.4.6

[U-Boot] [RFC PATCH 1/2] sunxi: retrieve FEL-provided values to environment variables

[Hans de Goede]

Hi,

On 03-09-15 16:11, Bernhard Nortmann wrote:
> This patch extends the misc_init_r() function on sunxi boards
> to test for the presence of a suitable "sunxi" SPL header. If
> found, and the loader ("fel" utility) provided non-zero values
> for either data address or size, then corresponding environment
> variables get set.
>
> misc_init_r() also sets (or clears) the "fel_booted" variable depending
> on the active boot device, using the same logic as spl_boot_device().
>
> The goal is to provide sufficient information (within the U-Boot
> environment) to make intelligent decisions on how to continue the boot
> process, allowing specific customizations for the "FEL boot" case.
>
> Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
> ---
>
>   board/sunxi/board.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 59 insertions(+)
>
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index 9c855f6..ceab1e0 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -516,6 +516,54 @@ void get_board_serial(struct tag_serialnr *serialnr)
>   }
>   #endif
>
> +#if !defined(CONFIG_SPL_BUILD)
> +static int check_signature(unsigned long io_addr, const char *signature,
> +			   int length)
> +{
> +	do {
> +		if (readb(io_addr) != *signature)
> +			return 0;
> +		io_addr++;
> +		signature++;
> +	} while (--length > 0);
> +	return 1;
> +}
> +
> +#define SPL_SIGNATURE			"SPL" /* marks "sunxi" header */
> +#define SPL_EXPECTED_VERSION		1
> +
> +/*
> + * Check the SPL header for the "sunxi" variant. If found: parse values
> + * that might have been passed by the loader ("fel" utility), and update
> + * the environment accordingly.
> + */
> +static void parse_spl_header(void)
> +{
> +	uint8_t spl_header_version;
> +	uint32_t fel_data_address;
> +	uint32_t fel_data_size;
> +
> +	if (check_signature(0x14, SPL_SIGNATURE, 3)) {
> +		spl_header_version = readb(0x17);
> +		if (spl_header_version == SPL_EXPECTED_VERSION) {
> +			fel_data_address = readl(0x18);
> +			fel_data_size = readl(0x1C);
> +			if (fel_data_address)
> +				setenv_hex("fel_data_addr", fel_data_address);
> +			else
> +				setenv("fel_data_addr", NULL);
> +			if (fel_data_size)
> +				setenv_hex("fel_data_size", fel_data_size);
> +			else
> +				setenv("fel_data_size", NULL);
> +		} else {
> +			printf("sunxi SPL header version mismatch: expected %u, got %u\n",
> +			       SPL_EXPECTED_VERSION, spl_header_version);
> +		}
> +	}
> +}

What if the user interrupts auto-boot with a fel provided boot.scr
and then does "saveenv" ?

Then we end up with a fel_data_addr and fel_data_size permanently
in the env.

At a minimum this function must always do:

setenv("fel_data_addr", NULL);
setenv("fel_data_size", NULL);

(rather then only when we've a spl fel header but no addr / size)

So that we do not end up trying to interpret old values ever.

Ideally though we would find another way not involving putting these
in the environment (not sure if that is easily doable).


> +#endif /* !defined(CONFIG_SPL_BUILD) */
> +
>   #ifdef CONFIG_MISC_INIT_R
>   int misc_init_r(void)
>   {
> @@ -524,6 +572,17 @@ int misc_init_r(void)
>   	uint8_t mac_addr[6];
>   	int ret;
>
> +#if !defined(CONFIG_SPL_BUILD)
> +	/* determine if we are running in FEL mode */
> +	if (readl(4) != 0x4E4F4765 || readl(8) != 0x3054422E) { /* eGON.BT0 */
> +		setenv("fel_booted", "1");
> +		parse_spl_header();
> +	} else {
> +		/* not in FEL mode, delete/clear env var (if present) */
> +		setenv("fel_booted", NULL);

Maybe clear all env variables here?

Or even better clear them all unconditionally, and then set
them when the checks succeed ?


> +	}
> +#endif
> +
>   	ret = sunxi_get_sid(sid);
>   	if (ret == 0 && sid[0] != 0 && sid[3] != 0) {
>   		if (!getenv("ethaddr")) {
>

Regards,

Hans

[U-Boot] [RFC PATCH 1/2] sunxi: retrieve FEL-provided values to environment variables

[Bernhard Nortmann]

Hi Hans!

Am 10.09.2015 um 20:34 schrieb Hans de Goede:
>
> What if the user interrupts auto-boot with a fel provided boot.scr
> and then does "saveenv" ?
>
> Then we end up with a fel_data_addr and fel_data_size permanently
> in the env.
>
> At a minimum this function must always do:
>
> setenv("fel_data_addr", NULL);
> setenv("fel_data_size", NULL);
>
> (rather then only when we've a spl fel header but no addr / size)
>
> So that we do not end up trying to interpret old values ever.

Good point.

>
> Ideally though we would find another way not involving putting these
> in the environment (not sure if that is easily doable).

I also have no idea how to achieve that, especially if we want to keep
the FEL case "in line" with other, more conventional ways of booting.

> Maybe clear all env variables here?
>
> Or even better clear them all unconditionally, and then set
> them when the checks succeed ?
>

That's something that may be best aligned somehow with the NAND and
MMC boot handling, if possible? I.e. have some centralized piece of
code that determines the active boot method and retrieves/presets the
environments accordingly. I'll have to re-examine that more closely.

Regards, B. Nortmann

[U-Boot] [RFC PATCH 1/2] sunxi: retrieve FEL-provided values to environment variables

[Ian Campbell]

On Thu, 2015-09-10 at 20:34 +0200, Hans de Goede wrote:
> [...]
> 
> What if the user interrupts auto-boot with a fel provided boot.scr
> and then does "saveenv" ?

This is an interesting question which is more generic than just these
variable, i.e. it applies to some extent to "ipaddr" when someone does
"dhcp ; saveenv" too.

Grepping around to see if there was any special handling for ipaddr I
came across "Vendor Parameter Protection" in the top-level README as
well as "CONFIG_ENV_FLAGS_LIST_DEFAULT" (and _STATIC) and various
default settings in include/env_flags.h.

I think CONFIG_ENV_FLAGS_LIST_* are what we want, and we want fel* to
be flagged "r" for read only and perhaps given an appropriate type
(either "d" or "x" for decimal or hex respectively, I suppose).

Ian.

[U-Boot] [RFC PATCH 1/2] sunxi: retrieve FEL-provided values to environment variables

[Hans de Goede]

Hi,

On 12-09-15 13:58, Ian Campbell wrote:
> On Thu, 2015-09-10 at 20:34 +0200, Hans de Goede wrote:
>> [...]
>>
>> What if the user interrupts auto-boot with a fel provided boot.scr
>> and then does "saveenv" ?
>
> This is an interesting question which is more generic than just these
> variable, i.e. it applies to some extent to "ipaddr" when someone does
> "dhcp ; saveenv" too.
>
> Grepping around to see if there was any special handling for ipaddr I
> came across "Vendor Parameter Protection" in the top-level README as
> well as "CONFIG_ENV_FLAGS_LIST_DEFAULT" (and _STATIC) and various
> default settings in include/env_flags.h.
>
> I think CONFIG_ENV_FLAGS_LIST_* are what we want, and we want fel* to
> be flagged "r" for read only

Ah, yes that sounds exactly what we want, thanks for figuring that
out.

> and perhaps given an appropriate type
> (either "d" or "x" for decimal or hex respectively, I suppose).

Regards,

Hans

[U-Boot] [RFC PATCH 1/2] sunxi: retrieve FEL-provided values to environment variables

[Bernhard Nortmann]

Hi Ian, hello Hans!

That's an interesting find, Ian - thank you.

Unfortunately it seems that flagging our environment vars accordingly
isn't enough (on its own) to prevent them from being written by
"saveenv". I've been testing

#define CONFIG_ENV_FLAGS_LIST_STATIC "fel_booted:bo,fel_scriptaddr:xo"

which achieves the desired flags, but doesn't alter saveenv behaviour.
Setting them to read-only ("r" access flag) doesn't help, as it won't
allow the setenv_*() calls to alter the vars in the first place.

As you have observed, this is a more general U-Boot problem - other
'volatile' information might end up in the environment (and saveenv)
as well, e.g. "ipaddr" and "serverip" retrieved by DHCP.

Regards, B. Nortmann


Am 12.09.2015 um 14:24 schrieb Hans de Goede:
> Hi,
>
> On 12-09-15 13:58, Ian Campbell wrote:
>> On Thu, 2015-09-10 at 20:34 +0200, Hans de Goede wrote:
>>> [...]
>>>
>>> What if the user interrupts auto-boot with a fel provided boot.scr
>>> and then does "saveenv" ?
>>
>> This is an interesting question which is more generic than just these
>> variable, i.e. it applies to some extent to "ipaddr" when someone does
>> "dhcp ; saveenv" too.
>>
>> Grepping around to see if there was any special handling for ipaddr I
>> came across "Vendor Parameter Protection" in the top-level README as
>> well as "CONFIG_ENV_FLAGS_LIST_DEFAULT" (and _STATIC) and various
>> default settings in include/env_flags.h.
>>
>> I think CONFIG_ENV_FLAGS_LIST_* are what we want, and we want fel* to
>> be flagged "r" for read only
>
> Ah, yes that sounds exactly what we want, thanks for figuring that
> out.
>
>> and perhaps given an appropriate type
>> (either "d" or "x" for decimal or hex respectively, I suppose).
>
> Regards,
>
> Hans

[U-Boot] [RFC PATCH 2/2] sunxi: add "fel" boot target

[Bernhard Nortmann]

This patch makes use of the previous changes to add a new "fel" boot
target for sunxi boards.

When booting via FEL, it's often desirable to work around the absence
of other (usable) boot devices - or to be able to override them,
deviating from the standard boot sequence. To achieve this, the "fel"
boot target gets the highest priority, but won't actually do anything
unless certain criteria are met.

The "bootcmd_fel" implementation proposed here first tests if an actual
FEL boot takes place (using the "fel_booted" env var), and secondly
checks that "fel_data_addr" was set (originating from the 'loader',
i.e. the sunxi-tools fel utility). If both checks pass, then it will
try to execute a boot script (boot.scr) at the given address. In case
of an error (e.g. an invalid image), the source command might return
"false", causing "distro_bootcmd" to proceed with the next boot target.

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

---

 include/configs/sunxi-common.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 48cc4ed..750355d 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -423,7 +423,20 @@ extern int soft_i2c_gpio_scl;
 #define BOOT_TARGET_DEVICES_USB(func)
 #endif
 
+/* FEL boot support, auto-execute boot.scr if a script address was provided */
+#define BOOTENV_DEV_FEL(devtypeu, devtypel, instance) \
+	"bootcmd_fel=" \
+		"if test -n ${fel_booted}; then " \
+			"echo '(FEL boot)';" \
+			"if test -n ${fel_data_addr}; then " \
+				"source ${fel_data_addr}; " \
+			"fi; " \
+		"fi\0"
+#define BOOTENV_DEV_NAME_FEL(devtypeu, devtypel, instance) \
+	"fel "
+
 #define BOOT_TARGET_DEVICES(func) \
+	func(FEL, fel, na) \
 	BOOT_TARGET_DEVICES_MMC(func) \
 	BOOT_TARGET_DEVICES_SCSI(func) \
 	BOOT_TARGET_DEVICES_USB(func) \
-- 
2.4.6

[U-Boot] [RFC PATCH 2/2] sunxi: add "fel" boot target

[Hans de Goede]

Hi,

On 03-09-15 16:12, Bernhard Nortmann wrote:
> This patch makes use of the previous changes to add a new "fel" boot
> target for sunxi boards.
>
> When booting via FEL, it's often desirable to work around the absence
> of other (usable) boot devices - or to be able to override them,
> deviating from the standard boot sequence. To achieve this, the "fel"
> boot target gets the highest priority, but won't actually do anything
> unless certain criteria are met.
>
> The "bootcmd_fel" implementation proposed here first tests if an actual
> FEL boot takes place (using the "fel_booted" env var), and secondly
> checks that "fel_data_addr" was set (originating from the 'loader',
> i.e. the sunxi-tools fel utility). If both checks pass, then it will
> try to execute a boot script (boot.scr) at the given address. In case
> of an error (e.g. an invalid image), the source command might return
> "false", causing "distro_bootcmd" to proceed with the next boot target.
>
> Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
>
> ---
>
>   include/configs/sunxi-common.h | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index 48cc4ed..750355d 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -423,7 +423,20 @@ extern int soft_i2c_gpio_scl;
>   #define BOOT_TARGET_DEVICES_USB(func)
>   #endif
>
> +/* FEL boot support, auto-execute boot.scr if a script address was provided */
> +#define BOOTENV_DEV_FEL(devtypeu, devtypel, instance) \
> +	"bootcmd_fel=" \
> +		"if test -n ${fel_booted}; then " \
> +			"echo '(FEL boot)';" \
> +			"if test -n ${fel_data_addr}; then " \
> +				"source ${fel_data_addr}; " \
> +			"fi; " \
> +		"fi\0"

I would prefer to have this like this:

	"bootcmd_fel=" \
		"if test -n ${fel_booted} && test -n ${fel_data_addr}; then " \
			"echo '(FEL boot)';" \
			"source ${fel_data_addr}; " \
		"fi\0"

Also if we are not using fel_data_size, then why do we even
have it ?

Regards,

Hans



> +#define BOOTENV_DEV_NAME_FEL(devtypeu, devtypel, instance) \
> +	"fel "
> +
>   #define BOOT_TARGET_DEVICES(func) \
> +	func(FEL, fel, na) \
>   	BOOT_TARGET_DEVICES_MMC(func) \
>   	BOOT_TARGET_DEVICES_SCSI(func) \
>   	BOOT_TARGET_DEVICES_USB(func) \
>

[U-Boot] [RFC PATCH 2/2] sunxi: add "fel" boot target

[Bernhard Nortmann]

Hi!

Am 10.09.2015 um 20:36 schrieb Hans de Goede:
> Hi,
>
> I would prefer to have this like this:
>
>     "bootcmd_fel=" \
>         "if test -n ${fel_booted} && test -n ${fel_data_addr}; then " \
>             "echo '(FEL boot)';" \
>             "source ${fel_data_addr}; " \
>         "fi\0"
>

Sure, we could do that. I wanted to make clear that ${fel_booted} is
independent of a script being present (and thus ${fel_data_addr} set).
If the user feels inclined to do so, he might e.g. tweak bootcmd_fel
to override some defaults even with no boot.scr involved.

> Also if we are not using fel_data_size, then why do we even
> have it ?
>

I thought it unnecessary to restrict ourselves to not being able to
pass the size information, and kept it optional deliberately.

Admittedly it's pointless in the "standard" case of boot.scr, as that
is expected to be an image with a well-defined header (including data
size). I could imagine other uses, e.g. a customized fel utility
passing uEnv.txt-style data, and integrating that via bootcmd_fel
"import -t ${fel_data_addr} ${fel_data_size}". Personally I like to
do this when testing; I find it easier to simply edit a text file
(without having to go through a mkimage .scr on each cycle).

Regards, B. Nortmann

[U-Boot] [RFC PATCH 2/2] sunxi: add "fel" boot target

[Hans de Goede]

Hi,

On 11-09-15 11:31, Bernhard Nortmann wrote:
> Hi!
>
> Am 10.09.2015 um 20:36 schrieb Hans de Goede:
>> Hi,
>>
>> I would prefer to have this like this:
>>
>>     "bootcmd_fel=" \
>>         "if test -n ${fel_booted} && test -n ${fel_data_addr}; then " \
>>             "echo '(FEL boot)';" \
>>             "source ${fel_data_addr}; " \
>>         "fi\0"
>>
>
> Sure, we could do that. I wanted to make clear that ${fel_booted} is
> independent of a script being present (and thus ${fel_data_addr} set).
> If the user feels inclined to do so, he might e.g. tweak bootcmd_fel
> to override some defaults even with no boot.scr involved.

I think that an user advanced enough to do this can figure this out
anyways and the above is cleaner, so please switch to the above
construct for the next version.

>> Also if we are not using fel_data_size, then why do we even
>> have it ?
>>
>
> I thought it unnecessary to restrict ourselves to not being able to
> pass the size information, and kept it optional deliberately.

Right, but by doing so you are taking our last reserved uint32, making
it unavailable for future use, I'm not sure if that is a good idea.

> Admittedly it's pointless in the "standard" case of boot.scr, as that
> is expected to be an image with a well-defined header (including data
> size). I could imagine other uses, e.g. a customized fel utility
> passing uEnv.txt-style data, and integrating that via bootcmd_fel
> "import -t ${fel_data_addr} ${fel_data_size}". Personally I like to
> do this when testing; I find it easier to simply edit a text file
> (without having to go through a mkimage .scr on each cycle).

But your sunxi-tools patches depends on the header to detect that
a boot.scr is being loaded and set ${fel_data_addr} and ${fel_data_size}
in the spl header, so this requires hacking up things in both u-boot
and sunxi-tools. Given that this is already a pretty exotic use-case
I believe that the user having to have mkimage available (the calling
of it can be scripted away) is not a big deal.

I would rather see an as clean as possible solution focussing on just
the boot.scr use-case, as said that is exotic enough already to not
complicate things further.

Regards,

Hans

[U-Boot] [RFC PATCH 2/2] sunxi: add "fel" boot target

[Ian Campbell]

On Sat, 2015-09-12 at 14:48 +0200, Hans de Goede wrote:
> I believe that the user having to have mkimage available (the calling
> of it can be scripted away) is not a big deal.

The fel tool could even call it automatically, couldn't it?

[U-Boot] [RFC PATCH 2/2] sunxi: add "fel" boot target

[Siarhei Siamashka]

On Fri, 11 Sep 2015 11:31:50 +0200
Bernhard Nortmann <bernhard.nortmann@web.de> wrote:

> Hi!
> 
> Am 10.09.2015 um 20:36 schrieb Hans de Goede:
> > Hi,
> >
> > I would prefer to have this like this:
> >
> >     "bootcmd_fel=" \
> >         "if test -n ${fel_booted} && test -n ${fel_data_addr}; then " \
> >             "echo '(FEL boot)';" \
> >             "source ${fel_data_addr}; " \
> >         "fi\0"
> >
> 
> Sure, we could do that. I wanted to make clear that ${fel_booted} is
> independent of a script being present (and thus ${fel_data_addr} set).
> If the user feels inclined to do so, he might e.g. tweak bootcmd_fel
> to override some defaults even with no boot.scr involved.
> 
> > Also if we are not using fel_data_size, then why do we even
> > have it ?
> >
> 
> I thought it unnecessary to restrict ourselves to not being able to
> pass the size information, and kept it optional deliberately.
> 
> Admittedly it's pointless in the "standard" case of boot.scr, as that
> is expected to be an image with a well-defined header (including data
> size). I could imagine other uses, e.g. a customized fel utility
> passing uEnv.txt-style data, and integrating that via bootcmd_fel
> "import -t ${fel_data_addr} ${fel_data_size}".

We could probably have this data represented in the following way
in the SPL header:

	union {
		struct {
			uint32_t fel_boot_script_address;
			uint32_t must_be_zero;
		};
		struct {
			uint32_t fel_uenv_txt_address;
			uint32_t fel_uenv_txt_size;
		};
	};

So that if 'fel_uenv_txt_size' is non-zero, then we can do
"import -t ${fel_uenv_txt_address} ${fel_uenv_txt_size}".
And if it is zero, then treat this pointer as boot.scr data.

And we don't even need to use the union if we don't want to. This all
data could be also stored in a straightforward way (at the expense
of using additional 4 bytes in the SPL header):

	uint32_t fel_boot_script_address;
	uint32_t fel_uenv_txt_address;
	uint32_t fel_uenv_txt_size;

And because it takes more storage space, we would need to increase
the SPL header size. But this can be easily done.

> Personally I like to do this when testing; I find it easier to
> simply edit a text file (without having to go through a mkimage
> .scr on each cycle).

Supporting both boot.scr and uEnv.txt for FEL boot seems to be
reasonably simple to me. You can even do it in a single patch
series. As Hans suggests, please take care of the boot.scr case
first. Then maybe introduce uEnv.txt support with an additional
patch.

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [RFC PATCH 2/2] sunxi: add "fel" boot target

[Hans de Goede]

Hi,

On 14-09-15 12:33, Siarhei Siamashka wrote:
> On Fri, 11 Sep 2015 11:31:50 +0200
> Bernhard Nortmann <bernhard.nortmann@web.de> wrote:
>
>> Hi!
>>
>> Am 10.09.2015 um 20:36 schrieb Hans de Goede:
>>> Hi,
>>>
>>> I would prefer to have this like this:
>>>
>>>      "bootcmd_fel=" \
>>>          "if test -n ${fel_booted} && test -n ${fel_data_addr}; then " \
>>>              "echo '(FEL boot)';" \
>>>              "source ${fel_data_addr}; " \
>>>          "fi\0"
>>>
>>
>> Sure, we could do that. I wanted to make clear that ${fel_booted} is
>> independent of a script being present (and thus ${fel_data_addr} set).
>> If the user feels inclined to do so, he might e.g. tweak bootcmd_fel
>> to override some defaults even with no boot.scr involved.
>>
>>> Also if we are not using fel_data_size, then why do we even
>>> have it ?
>>>
>>
>> I thought it unnecessary to restrict ourselves to not being able to
>> pass the size information, and kept it optional deliberately.
>>
>> Admittedly it's pointless in the "standard" case of boot.scr, as that
>> is expected to be an image with a well-defined header (including data
>> size). I could imagine other uses, e.g. a customized fel utility
>> passing uEnv.txt-style data, and integrating that via bootcmd_fel
>> "import -t ${fel_data_addr} ${fel_data_size}".
>
> We could probably have this data represented in the following way
> in the SPL header:
>
> 	union {
> 		struct {
> 			uint32_t fel_boot_script_address;
> 			uint32_t must_be_zero;
> 		};
> 		struct {
> 			uint32_t fel_uenv_txt_address;
> 			uint32_t fel_uenv_txt_size;
> 		};
> 	};
>
> So that if 'fel_uenv_txt_size' is non-zero, then we can do
> "import -t ${fel_uenv_txt_address} ${fel_uenv_txt_size}".
> And if it is zero, then treat this pointer as boot.scr data.
>
> And we don't even need to use the union if we don't want to. This all
> data could be also stored in a straightforward way (at the expense
> of using additional 4 bytes in the SPL header):
>
> 	uint32_t fel_boot_script_address;
> 	uint32_t fel_uenv_txt_address;
> 	uint32_t fel_uenv_txt_size;
>
> And because it takes more storage space, we would need to increase
> the SPL header size. But this can be easily done.
>
>> Personally I like to do this when testing; I find it easier to
>> simply edit a text file (without having to go through a mkimage
>> .scr on each cycle).
>
> Supporting both boot.scr and uEnv.txt for FEL boot seems to be
> reasonably simple to me. You can even do it in a single patch
> series. As Hans suggests, please take care of the boot.scr case
> first. Then maybe introduce uEnv.txt support with an additional
> patch.

I'm still not convinced that support uEnv.txt is necessary at all,
but I agree that if we this having this done through extra fields,
rather then through a union would be better.

Regards,

Hans

[U-Boot] [RFC PATCH 2/2] sunxi: add "fel" boot target

[Hans de Goede]

Hi,

On 14-09-15 13:42, Hans de Goede wrote:

>> Supporting both boot.scr and uEnv.txt for FEL boot seems to be
>> reasonably simple to me. You can even do it in a single patch
>> series. As Hans suggests, please take care of the boot.scr case
>> first. Then maybe introduce uEnv.txt support with an additional
>> patch.
>
> I'm still not convinced that support uEnv.txt is necessary at all,
> but I agree that if we this having this done through extra fields,
> rather then through a union would be better.

p.s.

As for the version byte vs minor/major scheme discussion. Why not
simply declare that we will always be backwards compatible, and
only ever add new fields ? Then we can simply add version checks
around those new fields, and have both back and forward compatibility.

In this case the single byte should suffice nicely.

Regards,

Hans

[U-Boot] [RFC PATCH 2/2] sunxi: add "fel" boot target

[Siarhei Siamashka]

On Mon, 14 Sep 2015 13:46:57 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 14-09-15 13:42, Hans de Goede wrote:
> 
> >> Supporting both boot.scr and uEnv.txt for FEL boot seems to be
> >> reasonably simple to me. You can even do it in a single patch
> >> series. As Hans suggests, please take care of the boot.scr case
> >> first. Then maybe introduce uEnv.txt support with an additional
> >> patch.
> >
> > I'm still not convinced that support uEnv.txt is necessary at all,
> > but I agree that if we this having this done through extra fields,
> > rather then through a union would be better.
> 
> p.s.
> 
> As for the version byte vs minor/major scheme discussion. Why not
> simply declare that we will always be backwards compatible, and
> only ever add new fields ? Then we can simply add version checks
> around those new fields, and have both back and forward compatibility.
> 
> In this case the single byte should suffice nicely.

Regarding "Why not simply declare that we will always be backwards
compatible". Certain things are not always in our perfect control.
It's better not to make unnecessary promises, otherwise they can
become a liability :-)

For example, let's suppose that we have added data fields for
both "uEnv.txt" and "boot.scr" in the SPL header. Then suppose
that later U-Boot maintainers decide to deprecate and drop support
of one of these methods in favour of the other.

This situation can be handled by doing the major version increase
and removing the obsolete field from the SPL header. The "fel" tool
would notice that the U-Boot binary is incompatible and complain,
providing the user with all the necessary hints.

But if we don't have the concept of major/minor versions, then the
"fel" tool would be just assuming full backwards compatibility and
happily trying to pass the obsolete data to U-Boot.

Yes, we can also always change the SPL header signature in the case
of introducing incompatible changes. But the down side is that the
"fel" tool would treat this situation as "unknown bootloader"
instead of "incompatible U-Boot".

Anyway, nothing really needs to be changed on the U-Boot side at
this moment and the Bernhard's patches don't need any modifications.
It's up to the "fel" tool how to interpret the version byte. For
example, we can arbitrarily decide that the top 4 bits are used for
the major version part and low 4 bits are used for the minor version
part. If only the minor version number is changed, then assume
backwards compatibility. If the major version number has changed,
then complain to the user.

-- 
Best regards,
Siarhei Siamashka