Hi Yixun,
Thanks for your comments, I will address them.
On Thu Sep 10, 2026 at 6:02 PM CST, Yixun Lan wrote:
> Hi Junhui,
>
> The patch looks good to me, I only have a few minor comments.
>
> On 23:51 Wed 09 Sep , Junhui Liu wrote:
[...]
>> +#define SMT_AUTH_SIZE 0x100
>> +
> ..
>> +#define SMT_FLAG_CRC32 (1U << 0)
> is this a local flag? how about defining as BIT(0)? which is more readable
> IMO
The BIT() macro is only available when building U-Boot itself and is not
exposed to host tools, so I'll keep the current expression.
>
>> +
>> +/**
>> + * struct smtimage_header - metadata header within a SpacemiT boot image
>> header
>> + *
>> + * @magic: Magic (must be SMT_MAGIC)
>> + * @version: Image version used when anti-rollback is enabled by
>> eFuse
>> + * @secure: Version slot used when anti-rollback is enabled by eFuse
>> + * @reserved: Reserved
>> + * @image_size: Header0 stores the image header size
>> + * Header1 stores the aligned payload size
>> + * @load_addr: Unused
>> + * @header_crc: Header0 and Header1 store the CRC32 of the preceding 24
>> bytes on K3
>> + * @image_crc: Header1 stores the CRC32 of the aligned payload on K3
>> + */
>> +struct smtimage_header {
>> + uint8_t magic[4];
>> + uint8_t version;
>> + uint8_t secure;
>> + uint16_t reserved;
>> + uint64_t image_size;
>> + uint64_t load_addr;
>> + uint32_t header_crc;
>> + uint32_t image_crc;
>> +} __packed;
>> +
>> +/**
>> + * struct smtimage_image_header - fixed header area of a SpacemiT boot image
>> + *
>> + * @root_key: Root public key
>> + * @header0: Image layout and load information
>> + * @key_data: Key metadata
>> + * @oem_keys: OEM public-key area
>> + * @signature0: Signature of the image configuration
>> + * @reserved: Reserved
>> + * @header1: SPL payload information
>> + */
>> +struct smtimage_image_header {
>> + uint8_t root_key[0x100];
>> + struct smtimage_header header0;
>> + uint8_t key_data[0x1e0];
>> + uint8_t oem_keys[0x800];
>> + uint8_t signature0[SMT_AUTH_SIZE];
>> + uint8_t reserved[0x3e0];
>> + struct smtimage_header header1;
>> +} __packed;
>> +
>> +/**
>> + * struct smtimage_variant - SoC-specific image properties
>> + *
>> + * @name: SoC name passed to mkimage with -n
>> + * @flags: SoC-specific image flags
>> + */
>> +struct smtimage_variant {
>> + const char *name;
>> + unsigned int flags;
>> +};
>> +
>> +static const struct smtimage_variant smtimage_variants[] = {
>> + {
>> + .name = "k3",
>> + .flags = SMT_FLAG_CRC32,
>> + },
> you may want to define a macro if adding too many more variants which will
> make code more compact..
> #define VARIANT(name, flag) \
> { \
> .name = "k3", \
> .flags = SMT_FLAG_CRC32, \
> }
> but I'm fine with current version if you're relunctant to change
I'll keep the current form, as introducing a macro wouldn't simplify
much and would make the code less readable.
> [...]
--
Best regards,
Junhui Liu
