[U-Boot] [RFC PATCH 1/3] sunxi: Extend SPL header versioning

Thu May 17 11:06:52 UTC 2018

于 2018年5月17日 GMT+08:00 下午7:05:15, Siarhei Siamashka <siarhei.siamashka at gmail.com> 写到:
>On Thu, 17 May 2018 09:16:59 +0100
>Andre Przywara <andre.przywara at arm.com> wrote:
>
>> On Allwinner SoCs we use some free bytes at the beginning of the SPL
>image
>> to store various information. We have a version byte to allow
>updates,
>> but changing this always requires all tools to be updated as well.
>
>The tools do not need to be updated together with U-Boot even now.
>
>U-Boot may freely increment the SPL version number as long as the new
>header is a superset of the old one.

But now sunxi-fel will work when SPL ver not recognized.

>
>> Introduce the concept of semantic versioning [1] to the SPL header:
>> The major part of the version number only changes on incompatible
>> updates, a minor number bump indicates backward compatibility.
>> This patch just documents the major/minor split, adds some comments
>> to the header file and uses the versioning information for the
>existing
>> users.
>> 
>> [1] https://semver.org
>
>So basically you are implementing the versioning scheme that I proposed
>back in 2015:
>    https://lists.denx.de/pipermail/u-boot/2015-September/228727.html
>
>Hans de Goede thought that the major/minor versioning was too complex
>and unnecessary (if I remember correctly, we had several discussion
>threads which concluded in the same way), so we did not implement it
>explicitly back then. But a potential non-compatible SPL header upgrade
>still could be handled, albeit less gracefully:
>
>    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".
>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>
>In general, improvements in this area are welcome. Just some
>comments below.
>
>> ---
>>  arch/arm/include/asm/arch-sunxi/spl.h | 19 ++++++++++++++-----
>>  board/sunxi/board.c                   |  4 ++--
>>  2 files changed, 16 insertions(+), 7 deletions(-)
>> 
>> diff --git a/arch/arm/include/asm/arch-sunxi/spl.h
>b/arch/arm/include/asm/arch-sunxi/spl.h
>> index 4277d836e5..7cf89c8db2 100644
>> --- a/arch/arm/include/asm/arch-sunxi/spl.h
>> +++ b/arch/arm/include/asm/arch-sunxi/spl.h
>> @@ -9,7 +9,16 @@
>>  
>>  #define BOOT0_MAGIC		"eGON.BT0"
>>  #define SPL_SIGNATURE		"SPL" /* marks "sunxi" SPL header */
>> -#define SPL_HEADER_VERSION	2
>> +#define SPL_MAJOR_BITS		3
>> +#define SPL_MINOR_BITS		5
>> +#define SPL_VERSION(maj, min)						\
>> +	((((maj) & ((1U << SPL_MAJOR_BITS) - 1)) << SPL_MINOR_BITS) | \
>> +	((min) & ((1U << SPL_MINOR_BITS) - 1)))
>> +
>> +#define SPL_HEADER_VERSION	SPL_VERSION(0, 2)
>> +
>> +#define SPL_ENV_HEADER_VERSION	SPL_VERSION(0, 1)
>> +#define SPL_DT_HEADER_VERSION	SPL_VERSION(0, 2)
>>  
>>  #ifdef CONFIG_SUNXI_HIGH_SRAM
>>  #define SPL_ADDR		0x10000
>> @@ -49,14 +58,14 @@ struct boot_file_head {
>>  		uint32_t pub_head_size;
>>  		uint8_t spl_signature[4];
>>  	};
>> -	uint32_t fel_script_address;
>> +	uint32_t fel_script_address;	/* since v0.1, set by sunxi-fel */
>
>Thanks, it's nice to have these comments about the versions where these
>features were introduced.
>
>>  	/*
>>  	 * 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 fel_uEnv_length;	/* since v0.1, set by sunxi-fel */
>>  	/*
>>  	 * Offset of an ASCIIZ string (relative to the SPL header), which
>>  	 * contains the default device tree name
>(CONFIG_DEFAULT_DEVICE_TREE).
>> @@ -64,11 +73,11 @@ struct boot_file_head {
>>  	 * by flash programming tools for providing nice informative
>messages
>>  	 * to the users.
>>  	 */
>> -	uint32_t dt_name_offset;
>> +	uint32_t dt_name_offset;	/* since v0.2, set by mksunxiboot */
>>  	uint32_t reserved1;
>>  	uint32_t boot_media;		/* written here by the boot ROM */
>>  	/* A padding area (may be used for storing text strings) */
>> -	uint32_t string_pool[13];
>> +	uint32_t string_pool[13];	/* since v0.2, filled by mksunxiboot */
>
>The 0.2 version of the SPL header does not specify the exact
>format of this 'string_pool' padding area. So I think that this
>comment is unnecessary.
>
>In principle, the device tree name string can be stored even
>somewhere in the const data section of the SPL binary and
>referenced from the SPL header. Not that it makes any sense to
>do this, but it is technically possible.
>
>>  	/* The header must be a multiple of 32 bytes (for VBAR alignment)
>*/
>>  };
>>  
>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>> index 3d364c6db5..a105d0a5ab 100644
>> --- a/board/sunxi/board.c
>> +++ b/board/sunxi/board.c
>> @@ -631,9 +631,9 @@ static void parse_spl_header(const uint32_t
>spl_addr)
>>  		return; /* signature mismatch, no usable header */
>>  
>>  	uint8_t spl_header_version = spl->spl_signature[3];
>> -	if (spl_header_version != SPL_HEADER_VERSION) {
>> +	if (spl_header_version < SPL_ENV_HEADER_VERSION) {
>
>We have already discussed this particular part of code earlier:
>    https://lists.denx.de/pipermail/u-boot/2015-September/227999.html
>
>Essentially, unless we have a real use case for mixing the SPL and the
>main U-Boot parts built from different U-Boot versions, I don't see any
>purpose for doing anything other than just exact version check here.
>
>If this particular check fails (the SPL part does not match the main
>U-Boot part), then something is already very wrong.
>
>>  		printf("sunxi SPL version mismatch: expected %u, got %u\n",
>> -		       SPL_HEADER_VERSION, spl_header_version);
>> +		       SPL_ENV_HEADER_VERSION, spl_header_version);
>>  		return;
>>  	}
>>  	if (!spl->fel_script_address)