Re: [PATCH 1/7] rockchip: mkimage: Split size_and_off and size_and_nimage

Hi Jonas,

On 8/6/26 3:09 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 8/6/2026 1:15 PM, Quentin Schulz wrote:
>> Hi Alexey,
>>
>> On 7/13/26 12:02 PM, Alexey Charkov wrote:
>>> From: Jonas Karlman <[email protected]>
>>>
>>> Split 32-bit size_and_off and size_and_nimage fields of the v2 image
>>> format header into their own 16-bit size, offset and num_images fields.
>>>
>>> Set num_images based on number of images passed by the datafile
>>> parameter and size based on the offset to the hash field to fix using a
>>> single init data file and no boot data file for the v2 image format.
>>>
>>
>> When you need to start listing things you do in a commit, it means it
>> needs to be split into multiple individual commits doing one thing at a
>> time. I appreciate this is taken from Jonas's tree but it's fine to
>> modify (well, from a maintainer's perspective at least :) ). Please list
>> between Jonas's and your Signed-off-by what you changed since Jonas's
>> version, in square brackets.
>>
>>> Signed-off-by: Jonas Karlman <[email protected]>
>>> Signed-off-by: Alexey Charkov <[email protected]>
>>> ---
>>>    tools/rkcommon.c | 44 ++++++++++++++++++++++++--------------------
>>>    1 file changed, 24 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
>>> index b39777fc0607..034896d57f80 100644
>>> --- a/tools/rkcommon.c
>>> +++ b/tools/rkcommon.c
>>> @@ -34,15 +34,16 @@ enum hash_type {
>>>    /**
>>>     * struct image_entry
>>>     *
>>> - * @size_and_off:	[31:16]image size;[15:0]image offset
>>> - * @address:	default as 0xFFFFFFFF
>>> + * @offset:	image offset (unit as 512 byte blocks)
>>> + * @size:	image size (unit as 512 byte blocks)
>>> + * @address:	load address (default as 0xFFFFFFFF)
>>
>> Can it be anything but 0xFFFFFFFF? It's not even configurable currently.
>>
>>>     * @flag:	no use
>>>     * @counter:	no use
>>>     * @hash:	hash of image
>>> - *
>>>     */
>>>    struct image_entry {
>>> -	uint32_t size_and_off;
>>> +	uint16_t offset;
>>> +	uint16_t size;
>>
>> Do we need to start __attribute__ ((__packed__))'ing the structure to
>> make sure there's no padding involved ever?
>>
>>>    	uint32_t address;
>>>    	uint32_t flag;
>>>    	uint32_t counter;
>>> @@ -56,16 +57,17 @@ struct image_entry {
>>>     * This is stored at SD card block 64 (where each block is 512 bytes)
>>>     *
>>>     * @magic:	Magic (must be RK_MAGIC_V2)
>>> - * @size_and_nimage:	[31:16]number of images;[15:0]
>>> - *			offset to hash field of header(unit as 4Byte)
>>> - * @boot_flag:	[3:0]hash type(0:none,1:sha256,2:sha512)
>>> - * @signature:	hash or signature for header info
>>> - *
>>> + * @size:	offset to hash field of header (unit as 4 bytes)
>>
>> I'm assuming we mean "unit as multiples of 4 bytes"? Is that correct?
>> Can we say that instead, I find it clearer.
>>
>>> + * @num_images:	number of images
>>
>> Can we improve this documentation as well? I'm assuming this is the
>> number of images stored in idbloader.img (so typically SPL + optionally
>> TPL + optionally VPL/boost?)
>>
>>> + * @boot_flag:	[3:0] hash type (0:none, 1:sha256, 2:sha512)
>>> + * @images:	images
>>
>> Not sure this is helping, either specify what those are or simply don't
>> comment.
>>
>>> + * @hash:	hash or signature for header info
>>>     */
>>>    struct header0_info_v2 {
>>>    	uint32_t magic;
>>>    	uint8_t reserved[4];
>>> -	uint32_t size_and_nimage;
>>> +	uint16_t size;
>>> +	uint16_t num_images;
>>
>> Do we need to start __attribute__ ((__packed__))'ing the structure to
>> make sure there's no padding involved ever?
>>
>> Side question, should we start using the proper expected endianness
>> here? e.g. __le16/__le32 instead of uint32_t and uint16_t? What do you
>> think?
>>
>>>    	uint32_t boot_flag;
>>>    	uint8_t reserved1[104];
>>>    	struct image_entry images[4];
>>> @@ -351,17 +353,18 @@ static void rkcommon_set_header0_v2(void *buf, struct image_tool_params *params)
>>>    	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->size_and_nimage = cpu_to_le32((2 << 16) + 384);
>>> +	hdr->magic = cpu_to_le32(RK_MAGIC_V2);
>>
>> Please don't mix cosmetic changes as those and logic changes, in the
>> same commit. The whitespaces removal makes it harder to identify quickly
>> what is actually changed.
>>
>>>    	hdr->boot_flag = cpu_to_le32(HASH_SHA256);
>>>    	sector_offset = 4;
>>>    	image_size_array[0] = spl_params.init_size;
>>>    	image_size_array[1] = spl_params.boot_size;
>>>    
>>>    	for (i = 0; i < 2; i++) {
>>> +		if (!image_size_array[i])
>>> +			break;
>>>    		image_sector_count = image_size_array[i] / RK_BLK_SIZE;
>>> -		hdr->images[i].size_and_off = cpu_to_le32((image_sector_count
>>> -							<< 16) + sector_offset);
>>> +		hdr->images[i].offset = cpu_to_le16(sector_offset);
>>> +		hdr->images[i].size = cpu_to_le16(image_sector_count);
>>>    		hdr->images[i].address = 0xFFFFFFFF;
>>>    		hdr->images[i].counter = cpu_to_le32(i + 1);
>>>    		image_ptr = buf + sector_offset * RK_BLK_SIZE;
>>> @@ -370,6 +373,8 @@ static void rkcommon_set_header0_v2(void *buf, struct image_tool_params *params)
>>>    		sector_offset = sector_offset + image_sector_count;
>>>    	}
>>>    
>>> +	hdr->num_images = cpu_to_le16(i);
>>
>> Is it though? if image_size_array[i] is 0, it means nothing was passed
>> as the i+1th image to mkimage -T rksd/rkspi -d no?
>>
>>> +	hdr->size = cpu_to_le16(offsetof(typeof(*hdr), hash) / sizeof(uint32_t));
>>>    	do_sha256_hash(buf, (void *)hdr->hash - buf, hdr->hash);
>>>    }
>>>    
>>> @@ -516,10 +521,8 @@ void rkcommon_print_header(const void *buf, struct image_tool_params *params)
>>>    			return;
>>>    		}
>>>    
>>> -		init_size = header0_v2.images[0].size_and_off >> 16;
>>> -		init_size = init_size * RK_BLK_SIZE;
>>> -		boot_size = header0_v2.images[1].size_and_off >> 16;
>>> -		boot_size = boot_size * RK_BLK_SIZE;
>>> +		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;
>>>    	} else {
>>>    		ret = rkcommon_parse_header(buf, &header0, &spl_info);
>>>    
>>> @@ -533,8 +536,9 @@ void rkcommon_print_header(const void *buf, struct image_tool_params *params)
>>>    		}
>>>    
>>>    		image_type = ret;
>>> -		init_size = header0.init_size * RK_BLK_SIZE;
>>> -		boot_size = header0.init_boot_size * RK_BLK_SIZE - init_size;
>>> +		init_size = le16_to_cpu(header0.init_size) * RK_BLK_SIZE;
>>> +		boot_size = le16_to_cpu(header0.init_boot_size) * RK_BLK_SIZE -
>>> +			    init_size;
>>>    
>>
>> I could identify multiple commits to be split out of this one:
>> - update documentation,
>> - whitespace around ->magic assignment,
>> - break the loop when image_size_array[i] is 0,
>> - properly translate init_size/boot_size into CPU endianness,
>> - split size_and_nimage u32 into two u16,
>> - split size_and_off u32 into two u16,
>> - programmatically set ->size,
>> - programmatically set ->num_images,
> 
> As stated to Alexey previously I have started re-working my old series
> to address your old review comments, current local version have:
> 
> b86f1ed33d9d WIP: rockchip: mkimage: Compare full string when matching imagename
> d849f5710a62 WIP: rockchip: mkimage: Print image-type for the v2 format
> 690bd184c62f WIP: rockchip: mkimage: use le16/le32_to_cpu when reading header fields
> 01e1ce9ccab4 WIP: rockchip: mkimage: static assert
> d5b2191eae34 WIP: rockchip: mkimage: split size_and_off in struct image_entry
> 0bbb7bf20348 WIP: rockchip: mkimage: split size_and_nimage in struct header0_info_v2
> 4f1cc2100120 WIP: rockchip: mkimage: Set header size based on offset to hash
> 2031ef00ddaa rockchip: mkimage: Split size_and_off and size_and_nimage
> deae586379d8 rockchip: mkimage: Print image information for all embedded images
> 15867436697d rockchip: mkimage: Print image information for all embedded images
> c5f38fe9aa85 rockchip: mkimage: Print boot0 and boot1 parameters
> c03eda77d517 WIP: rockchip: mkimage: Print hash algo and image hash value
> c28f75ab3d22 WIP: rockchip: mkimage: Print signature type
> 744b76c3e716 WIP: rockchip: mkimage: Extract and use rkcommon_get_aligned_filesize()
> e667ac1ff977 WIP: rockchip: mkimage: Extract and use rkcommon_get_header_size()
> 776f4b84625f rockchip: mkimage: Add option to change image offset alignment
> 403e40c9fe80 rockchip: mkimage: Add support for up to 4 input files
> 
> But I have not completed addressing all review comments yet, will be
> sending a v2 of my original series in a few days.
> 

Should I stop reviewing the rest of this series and wait for your v2 then?

Cheers,
Quentin