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

[Quentin Schulz <quentin.schulz@cherry.de>]

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@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.

>   	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?

> +			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.

> +	}
> +}
> +
>   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.

Looking good otherwise,
Cheers,
Quentin