Hi Quentin, On 8/6/2026 5:25 PM, Quentin Schulz wrote: > 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?
It looks like your new review comments closely matches your old review remarks so probably no reason to do double/triple work :-) I have tried to address all your old review remarks and some of Simons comments in my local tree, will look at your new comments and see if there is anything to pick up from them before sending a v2. Regards, Jonas > > Cheers, > Quentin
