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

[Bernhard Nortmann]

Hi!

Am 16.09.2015 um 03:00 schrieb Siarhei Siamashka:
> On Mon, 14 Sep 2015 15:15:29 +0200
> Bernhard Nortmann <bernhard.nortmann at web.de> 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 a non-zero value
>> for the boot.scr address, then the corresponding environment
>> variable fel_scriptaddr gets 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 at web.de>
>> ---
>>
>> Changes in v2:
>> - renamed fel_data_addr to fel_script_addr, discarded fel_data_size
>> - make sure that FEL-related environment vars are always cleared first
>> - support minimum and maximum SPL (header) version, more verbose error messages
>>
>>   board/sunxi/board.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 56 insertions(+)
>>
>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>> index 9c855f6..aa26e57 100644
>> --- a/board/sunxi/board.c
>> +++ b/board/sunxi/board.c
>> @@ -516,6 +516,52 @@ 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);
> It is probably better to just use memcmp() instead of this loop.
>
> Admittedly, I'm responsible for abusing readl() in the old code,
> because it allowed to fit the FEL mode check into just a single line
> (the pointer casts would make it exceed the 80 characters limit):
>
>       if (readl(4) != 0x4E4F4765 || readl(8) != 0x3054422E) { /* eGON.BT0 */
>
> However now this readb() usage seems to do exactly the opposite and only
> inflates the code unnecessarily.

That answers my questions from the introductory message. I'm glad we can
do away with that function, as it really was just introduced to 'squeeze
in' that readb() logic there. I think a nice way of dealing with the
signature checks (and their line length) is to introduce appropriate
macro wrappers in a reworked asm/arch/spl.h - e.g. have_boot0_magic()
and have_sunxi_spl().

>
>> +	return 1;
>> +}
>>
>> +#define SPL_SIGNATURE			"SPL" /* marks "sunxi" header */
>> +#define SPL_MIN_VERSION			1
>> +#define SPL_MAX_VERSION			1
> Can we have a way to share these defines between "board/sunxi/board.c"
> and "tools/mksunxiboot.c"?

Yes, a sane(r) way of dealing with these definitions and the
boot_file_head structure shared by that code is to place them in a common
spl.h include file. As that is sunxi platform-specific I've targetted
asm/arch/spl.h - that file's content seems superseded / currently unused
anyway.

>
>> +/*
>> + * 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_script_addr;
>> +
>> +	if (check_signature(0x14, SPL_SIGNATURE, 3)) {
>> +		spl_header_version = readb(0x17);
>> +		if (spl_header_version < SPL_MIN_VERSION) {
>> +			printf("sunxi SPL version mismatch: found 0x%02X < required minimum 0x%02X\n",
>> +			       spl_header_version, SPL_MIN_VERSION);
>> +			return;
>> +		}
>> +		if (spl_header_version > SPL_MAX_VERSION) {
>> +			printf("sunxi SPL version mismatch: found 0x%02X > maximum supported 0x%02X\n",
>> +			       spl_header_version, SPL_MAX_VERSION);
>> +			return;
>> +		}
> Yes, having this signature check before extracting the information from
> the SPL header is a good idea. Because we can have Allwinner's boot0
> used together with the main U-Boot binary (for the SoC variants, which
> do not have SPL support yet).
>
> But here it is probably better to just expect the exact SPL header
> version match? The SPL and the main U-Boot binary are usually both
> built together from the same sources and combined into a single
> u-boot-sunxi-with-spl.bin file.

Agreed. I was following the fel.c logic from sunxi-tools, but here it's
quite reasonable to expect an exact SPL_HEADER_VERSION originating from
the same U-Boot version / build.

>
>> +		fel_script_addr = readl(0x18);
>> +		if (fel_script_addr)
>> +			setenv_hex("fel_scriptaddr", fel_script_addr);
>> +	}
>> +}
>> +#endif /* !defined(CONFIG_SPL_BUILD) */
>> +
>>   #ifdef CONFIG_MISC_INIT_R
>>   int misc_init_r(void)
>>   {
>> @@ -524,6 +570,16 @@ int misc_init_r(void)
>>   	uint8_t mac_addr[6];
>>   	int ret;
>>   
>> +#if !defined(CONFIG_SPL_BUILD)
>> +	setenv("fel_booted", NULL);
>> +	setenv("fel_scriptaddr", NULL);
>> +	/* determine if we are running in FEL mode */
>> +	if (readl(4) != 0x4E4F4765 || readl(8) != 0x3054422E) { /* eGON.BT0 */
>> +		setenv("fel_booted", "1");
>> +		parse_spl_header();
>> +	}
>> +#endif
>> +
>>   	ret = sunxi_get_sid(sid);
>>   	if (ret == 0 && sid[0] != 0 && sid[3] != 0) {
>>   		if (!getenv("ethaddr")) {

I think following your suggestions improves quite a bit on the readability
of this patch series. I'll thus prepare and submit a v3 soon.

Regards, B. Nortmann