[PATCH 2/6] rockchip: mkimage: Print image information for all embedded images

[Jonas Karlman]

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 <jonas at 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-jonas@kwiboo.se/

Regards,
Jonas

> 
> Looking good otherwise,
> Cheers,
> Quentin