Hi,
On 17/05/18 12:05, Siarhei Siamashka wrote:
> On Thu, 17 May 2018 09:16:59 +0100
> Andre Przywara <andre.przyw...@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.
>
>> 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
Ah, sorry, that predates my sunxi involvement ;-)
> 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.przyw...@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.
Well, I understand that it *could* be everywhere, but I wanted to give a
heads up to the reader that the actual mksunxiboot implementation fills
that area, so it's not really vacant anymore. That may become of
importance if we need more fields.
> 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.
For FEL booting we might trigger this. My personal use case is to have
FEL capable 32-bit SPL binaries for the ARMv8 SoCs lying around[2], and
using them with any version of U-Boot proper I want to test/debug.
So I hit this there. I see that this is quite a stretch, but this "<"
relation is what the code actually requires, especially now with the
semantic versioning, so I changed that.
Cheers,
Andre.
[2] https://github.com/apritzel/pine64/tree/master/binaries
> 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)
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
