Hi Quentin,
On 2025-02-05 16:57, Quentin Schulz wrote:
> Hi Jonas,
>
> On 1/29/25 11:36 PM, Jonas Karlman wrote:
>> The v2 image format can embed up to 4 data files compared to the two
>> init and boot data files using the older image format.
>>
>> Add support for displaying more of the image header information that
>> exists in the v2 image format, e.g. image load address and flag.
>>
>> Example for v2 image format:
>>
>> > tools/mkimage -l rk3576_idblock_v1.09.107.img
>> Rockchip Boot Image (v2)
>> Image 1: 4096 @ 0x1000
>> - Load address: 0x3ffc0000
>> Image 2: 77824 @ 0x2000
>> - Load address: 0x3ff81000
>> Image 3: 262144 @ 0x15000
>>
>> Example for older image format:
>>
>> > tools/mkimage -l u-boot-rockchip.bin
>> Rockchip RK32 (SD/MMC) Boot Image
>> Init Data: 20480 @ 0x800
>> Boot Data: 112640 @ 0x5800
>>
>> Signed-off-by: Jonas Karlman <jo...@kwiboo.se>
>> ---
>> tools/rkcommon.c | 41 +++++++++++++++++++++++++++++++----------
>> 1 file changed, 31 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
>> index de3fd2d3f3c2..ad239917d2bd 100644
>> --- a/tools/rkcommon.c
>> +++ b/tools/rkcommon.c
>> @@ -331,8 +331,6 @@ static void rkcommon_set_header0_v2(void *buf, struct
>> image_tool_params *params)
>> uint8_t *image_ptr = NULL;
>> int i;
>>
>> - printf("Image Type: Rockchip %s boot image\n",
>> - rkcommon_get_spl_hdr(params));
>
> Not sure this change is related? It's also not replaced by anything if
> I'm not mistaken, hence why I'm wondering why it's in this patch.
Following was meant as a replacement for this, in rkcommon_print_header_v2():
printf("Rockchip Boot Image (v2)\n");
The old printf() was incorrectly done at set_header, not in print_header,
that is called after set_header or when you try to "mkimage -l <file>".
>
>> memset(buf, '\0', RK_INIT_OFFSET * RK_BLK_SIZE);
>> hdr->magic = cpu_to_le32(RK_MAGIC_V2);
>> hdr->boot_flag = cpu_to_le32(HASH_SHA256);
>> @@ -486,6 +484,29 @@ int rkcommon_verify_header(unsigned char *buf, int size,
>> return -ENOENT;
>> }
>>
>> +static void rkcommon_print_header_v2(const struct header0_info_v2 *hdr)
>> +{
>> + uint32_t val;
>> + int i;
>> +
>> + printf("Rockchip Boot Image (v2)\n");
>> +
>> + for (i = 0; i < le16_to_cpu(hdr->num_images); i++) {
>> + printf("Image %u: %u @ 0x%x\n",
>> + le32_to_cpu(hdr->images[i].counter),
>> + le16_to_cpu(hdr->images[i].size) * RK_BLK_SIZE,
>> + le16_to_cpu(hdr->images[i].offset) * RK_BLK_SIZE);
>> +
>> + val = le32_to_cpu(hdr->images[i].address);
>> + if (val != 0xFFFFFFFF)
>
> Can you explain why this value is explicitly excluded? I know this is
> the 4GiB boundary but why does it matter?
It is used as the default value, unknown why, see:
@address: load address (default as 0xFFFFFFFF)
Can probably add a code comment here as well.
>
>> + printf("- Load address: 0x%x\n", val);
>> +
>> + val = le32_to_cpu(hdr->images[i].flag);
>> + if (val)
>> + printf("- Flag: 0x%x\n", val);
>
> Matter of taste but the dashes were bothering me when parsing the output
> with my eyes, two spaces could work better. In any case, not a big deal
> to me.
Will see what I can do, initially I indented to align with the size @
offset and changed to the dash just before sending.
>
>> + }
>> +}
>> +
>> void rkcommon_print_header(const void *buf, struct image_tool_params
>> *params)
>> {
>> struct header0_info header0;
>> @@ -502,8 +523,7 @@ void rkcommon_print_header(const void *buf, struct
>> image_tool_params *params)
>> return;
>> }
>>
>> - init_size = le16_to_cpu(header0_v2.images[0].size) *
>> RK_BLK_SIZE;
>> - boot_size = le16_to_cpu(header0_v2.images[1].size) *
>> RK_BLK_SIZE;
>> + rkcommon_print_header_v2(&header0_v2);
>> } else {
>> ret = rkcommon_parse_header(buf, &header0, &spl_info);
>>
>> @@ -521,15 +541,16 @@ void rkcommon_print_header(const void *buf, struct
>> image_tool_params *params)
>> boot_size = le16_to_cpu(header0.init_boot_size) * RK_BLK_SIZE -
>> init_size;
>>
>> - printf("Image Type: Rockchip %s (%s) boot image\n",
>> - spl_info->spl_hdr,
>> + printf("Rockchip %s (%s) Boot Image\n", spl_info->spl_hdr,
>> (image_type == IH_TYPE_RKSD) ? "SD/MMC" : "SPI");
>
> Please keep "Image Type:" this is what's used for other SoC vendors
> also, I assume some tooling could be parsing it.
Sure, will restore the "Image Type:" prefix here and for "Rockchip Boot
Image (v2)" above. Should probably also adjust "tools: mkimage: Add
Amlogic Boot Image type" [1] to do the same.
[1]
https://patchwork.ozlabs.org/project/uboot/patch/20250103215904.2590769-2-jo...@kwiboo.se/
Regards,
Jonas
>
> Looking good otherwise,
> Cheers,
> Quentin
