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

Hi Alexey,

On 7/13/26 12:02 PM, Alexey Charkov wrote:
> From: Jonas Karlman <[email protected]>
> 
> 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 <[email protected]>
> Signed-off-by: Alexey Charkov <[email protected]>
> ---
>   tools/rkcommon.c | 41 +++++++++++++++++++++++++++++++----------
>   1 file changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
> index 034896d57f80..fe0afc5629b3 100644
> --- a/tools/rkcommon.c
> +++ b/tools/rkcommon.c
> @@ -350,8 +350,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));
>   	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);
> @@ -505,6 +503,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;

Scope can be reduced by moving this into the for-loop below.

> +	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)
> +			printf("- Load address: 0x%x\n", val);

Why? Isn't it always useful info?

> +
> +		val = le32_to_cpu(hdr->images[i].flag);
> +		if (val)
> +			printf("- Flag: 0x%x\n", val);

Considering we never set the flag member, I don't think this makes sense 
until we add code that sets it. I see you also print something in the 
next commit that U-Boot does not set, so just make the flag printing its 
own commit and justify as coming from binaries generated by boot_merger.

> +	}
> +}
> +
>   void rkcommon_print_header(const void *buf, struct image_tool_params *params)
>   {
>   	struct header0_info header0;
> @@ -521,8 +542,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);
>   
> @@ -540,15 +560,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,

Please keep printing "Image Type: ", from a quick glance most other 
image types also print that prefix (same remark for v2).

>   		       (image_type == IH_TYPE_RKSD) ? "SD/MMC" : "SPI");
> -	}
>   
> -	printf("Init Data Size: %d bytes\n", init_size);
> +		printf("Init Data: %d @ 0x%x\n", init_size,
> +		       le16_to_cpu(header0.init_offset) * RK_BLK_SIZE);
>   
> -	if (boot_size != RK_MAX_BOOT_SIZE)
> -		printf("Boot Data Size: %d bytes\n", boot_size);
> +		if (boot_size != RK_MAX_BOOT_SIZE)
> +			printf("Boot Data: %d @ 0x%x\n", boot_size, init_size +
> +			       le16_to_cpu(header0.init_offset) * RK_BLK_SIZE);
> +	}

Just split the logic for v1 into a function like you did for v2 so we're 
consistent. A single commit for moving both

Do *NOT* reword/capitalize/rephrase/change what's printed for the v1. 
Even though we don't specify this is stable or part of an API, I'm sure 
some people out there are parsing mkimage -l's output so let's not break 
them unless we have to. For v2 I guess it's fine since it'll be heavily 
modified and I never was a fan of the Init Data and Boot Data (what does 
this actually mean....).

Cheers,
Quentin