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

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

Hi Jonas,

On 2/5/25 8:36 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2025-02-05 16:57, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 1/29/25 11:36 PM, Jonas Karlman wrote:
[...]
>>> --- 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>".
> 

Ah, I see, thanks.

So this is a bugfix because it doesn't show with mkimage -l, separate 
commit then.

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

Or maybe a constant to highlight the relation between both. Though one 
would need to give it an appropriate name... Here comes the most 
difficult part of SW development.

[...]

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

Could even have another callback e.g. get_image_type() which returns a 
const char* to print after "Image Type: " and move that to 
tools/mkimage.c to have consistent behavior for that part. Probably 
over-engineering this though :) (I think we'd need to update imagetool too).

Cheers,
Quentin