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. Regards, Jonas > > Cheers, > Quentin
