[PATCH 0/6] rockchip: mkimage: Improve support for v2 image format

[PATCH 0/6] rockchip: mkimage: Improve support for v2 image format

[Jonas Karlman]

The Rockchip idblock v2 image format header embeds much more information
than the older format. E.g. it can embed up to 4 images that the BootROM
can load, image load address, flag, checksum and more.

This series improves mkimage with support for some of these features of
the v2 image format. Features that are likely to be needed to have
working SD-card boot on RK3576.

Jonas Karlman (6):
  rockchip: mkimage: Split size_and_off and size_and_nimage
  rockchip: mkimage: Print image information for all embedded images
  rockchip: mkimage: Print boot0 and boot1 parameters
  rockchip: mkimage: Add option to change image offset alignment
  rockchip: mkimage: Add support for up to 4 input files
  rockchip: mkimage: Add option for image load address and flag

 tools/rkcommon.c | 268 +++++++++++++++++++++++++++++------------------
 tools/rkcommon.h |   2 -
 2 files changed, 166 insertions(+), 104 deletions(-)

-- 
2.48.1

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

[Jonas Karlman]

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.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/rkcommon.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/tools/rkcommon.c b/tools/rkcommon.c
index 3e52236b15a8..de3fd2d3f3c2 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)
  * @flag:	no use
  * @counter:	no use
  * @hash:	hash of image
- *
  */
 struct image_entry {
-	uint32_t size_and_off;
+	uint16_t offset;
+	uint16_t size;
 	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)
+ * @num_images:	number of images
+ * @boot_flag:	[3:0] hash type (0:none, 1:sha256, 2:sha512)
+ * @images:	images
+ * @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;
 	uint32_t boot_flag;
 	uint8_t reserved1[104];
 	struct image_entry images[4];
@@ -332,17 +334,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);
 	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;
@@ -351,6 +354,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);
+	hdr->size = cpu_to_le16(offsetof(typeof(*hdr), hash) / sizeof(uint32_t));
 	do_sha256_hash(buf, (void *)hdr->hash - buf, hdr->hash);
 }
 
@@ -497,10 +502,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);
 
@@ -514,8 +517,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;
 
 		printf("Image Type:   Rockchip %s (%s) boot image\n",
 		       spl_info->spl_hdr,
-- 
2.48.1

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

[Quentin Schulz]

Hi Jonas,

On 1/29/25 11:36 PM, Jonas Karlman wrote:
> 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.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>   tools/rkcommon.c | 44 ++++++++++++++++++++++++--------------------
>   1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
> index 3e52236b15a8..de3fd2d3f3c2 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)
>    * @flag:	no use
>    * @counter:	no use
>    * @hash:	hash of image
> - *
>    */
>   struct image_entry {
> -	uint32_t size_and_off;
> +	uint16_t offset;
> +	uint16_t size;
>   	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)
> + * @num_images:	number of images
> + * @boot_flag:	[3:0] hash type (0:none, 1:sha256, 2:sha512)
> + * @images:	images
> + * @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;
>   	uint32_t boot_flag;
>   	uint8_t reserved1[104];
>   	struct image_entry images[4];
> @@ -332,17 +334,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);
>   	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;

This isn't related to this change I believe, can you please make it its 
own commit so it doesn't get lost in the diff and has its own individual 
commit log?

>   		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;
> @@ -351,6 +354,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);
> +	hdr->size = cpu_to_le16(offsetof(typeof(*hdr), hash) / sizeof(uint32_t));

Same here. Just do a migration commit (possibly one for struct 
image_entry and another one for struct header0_info_v2) first and then 
adapt so it handles image_size_array[1] = 0. We don't today so a 
separate patch explaining the usecase would be nice.

>   	do_sha256_hash(buf, (void *)hdr->hash - buf, hdr->hash);
>   }
>   
> @@ -497,10 +502,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;

Ditto. Separate patch for the le16_to_cpu would be nice as I assume this 
is not a side-effect of switching to two u16 instead of one u32. This 
likely fixes a bug :)

I was wondering if we shouldn't have CI to generate a handful of 
Rockchip dummy binaries with the header on different endianness so we 
can catch those. I remember we had someone fix those for v1 already.

>   	} else {
>   		ret = rkcommon_parse_header(buf, &header0, &spl_info);
>   
> @@ -514,8 +517,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;
>   

Ditto, separate patch for le16_to_cpu.

Looks good otherwise!

Cheers,
Quentin

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

[Jonas Karlman]

Hi Quentin,

On 2025-02-05 16:40, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 1/29/25 11:36 PM, Jonas Karlman wrote:
>> 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.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>>   tools/rkcommon.c | 44 ++++++++++++++++++++++++--------------------
>>   1 file changed, 24 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
>> index 3e52236b15a8..de3fd2d3f3c2 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)
>>    * @flag:	no use
>>    * @counter:	no use
>>    * @hash:	hash of image
>> - *
>>    */
>>   struct image_entry {
>> -	uint32_t size_and_off;
>> +	uint16_t offset;
>> +	uint16_t size;
>>   	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)
>> + * @num_images:	number of images
>> + * @boot_flag:	[3:0] hash type (0:none, 1:sha256, 2:sha512)
>> + * @images:	images
>> + * @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;
>>   	uint32_t boot_flag;
>>   	uint8_t reserved1[104];
>>   	struct image_entry images[4];
>> @@ -332,17 +334,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);
>>   	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;
> 
> This isn't related to this change I believe, can you please make it its 
> own commit so it doesn't get lost in the diff and has its own individual 
> commit log?

Yeah, I mostly wanted/needed a test to stop the loop on the "last"
supplied input file/image in a later patch. Adding it here made the
later diff little bit smaller :-)

> 
>>   		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;
>> @@ -351,6 +354,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);
>> +	hdr->size = cpu_to_le16(offsetof(typeof(*hdr), hash) / sizeof(uint32_t));
> 
> Same here. Just do a migration commit (possibly one for struct 
> image_entry and another one for struct header0_info_v2) first and then 
> adapt so it handles image_size_array[1] = 0. We don't today so a 
> separate patch explaining the usecase would be nice.

I was trying to avoid having to create too many commits, and mostly
cared about the end result, guess I can split this even further in a v2.

All in-tree use from binman will always supply two input files/images
for v2 image format, so has not really been an issue or need to handle
image_size_array[1] = 0, not sure what would happen if you try to run
mkimage manually without the second boot image.

> 
>>   	do_sha256_hash(buf, (void *)hdr->hash - buf, hdr->hash);
>>   }
>>   
>> @@ -497,10 +502,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;
> 
> Ditto. Separate patch for the le16_to_cpu would be nice as I assume this 
> is not a side-effect of switching to two u16 instead of one u32. This 
> likely fixes a bug :)

I have no idea if any of this was or is really working on big-endian
prior to (or after), there was some bit manipulation and change to
use leXX_to_cpu seemed appropriate, will try to split it out in v2.
Could possible try to use qemu for some big-endian testing.

> 
> I was wondering if we shouldn't have CI to generate a handful of 
> Rockchip dummy binaries with the header on different endianness so we 
> can catch those. I remember we had someone fix those for v1 already.

Sound like a nice task for someone else :-)

> 
>>   	} else {
>>   		ret = rkcommon_parse_header(buf, &header0, &spl_info);
>>   
>> @@ -514,8 +517,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;
>>   
> 
> Ditto, separate patch for le16_to_cpu.

Sure, will split out to a few more patches for v2.

Regards,
Jonas

> 
> Looks good otherwise!
> 
> Cheers,
> Quentin

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

[Jonas Karlman]

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));
 	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)
+			printf("- Load address: 0x%x\n", val);
+
+		val = le32_to_cpu(hdr->images[i].flag);
+		if (val)
+			printf("- Flag: 0x%x\n", val);
+	}
+}
+
 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");
-	}
 
-	printf("Init Data Size: %d bytes\n", init_size);
+		printf("Init Data: %d @ 0x%x\n", init_size,
+		       le16_to_cpu(header0.init_offset) * RK_BLK_SIZE);
 
-	if (boot_size != RK_MAX_BOOT_SIZE)
-		printf("Boot Data Size: %d bytes\n", boot_size);
+		if (boot_size != RK_MAX_BOOT_SIZE)
+			printf("Boot Data: %d @ 0x%x\n", boot_size, init_size +
+			       le16_to_cpu(header0.init_offset) * RK_BLK_SIZE);
+	}
 }
 
 void rkcommon_rc4_encode_spl(void *buf, unsigned int offset, unsigned int size)
-- 
2.48.1

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

[Quentin Schulz]

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

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

[Jonas Karlman]

Hi Quentin,

On 2025-02-05 16:57, Quentin Schulz wrote:
> 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.

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

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

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

Will see what I can do, initially I indented to align with the size @
offset and changed to the dash just before sending.

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

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.

[1] https://patchwork.ozlabs.org/project/uboot/patch/20250103215904.2590769-2-jonas@kwiboo.se/

Regards,
Jonas

> 
> Looking good otherwise,
> Cheers,
> Quentin

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

[Quentin Schulz]

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

[PATCH 3/6] rockchip: mkimage: Print boot0 and boot1 parameters

[Jonas Karlman]

The v2 image format embeds boot0 and boot1 parameters, the vendor tool
boot_merger may write these parameters based on the rkboot miniall.ini
files.

E.g. a RK3576 boot image may contain a boot1 parameter that signals
BootROM or vendor blobs to use 1 GHz instead of the regular 24 MHz rate
for the high precision timer.

Add support for printing boot0 and boot1 parameters, e.g.:

  > tools/mkimage -l rk3576_idblock_v1.09.107.img
  Rockchip Boot Image (v2)
  Boot1 2: 0x100
  Image 1: 4096 @ 0x1000
  - Load address: 0x3ffc0000
  Image 2: 77824 @ 0x2000
  - Load address: 0x3ff81000
  Image 3: 262144 @ 0x15000

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/rkcommon.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/tools/rkcommon.c b/tools/rkcommon.c
index ad239917d2bd..324820717663 100644
--- a/tools/rkcommon.c
+++ b/tools/rkcommon.c
@@ -62,6 +62,8 @@ struct image_entry {
  * @boot_flag:	[3:0] hash type (0:none, 1:sha256, 2:sha512)
  * @images:	images
  * @hash:	hash or signature for header info
+ *
+ * Other fields are not used by U-Boot
  */
 struct header0_info_v2 {
 	uint32_t magic;
@@ -69,7 +71,9 @@ struct header0_info_v2 {
 	uint16_t size;
 	uint16_t num_images;
 	uint32_t boot_flag;
-	uint8_t reserved1[104];
+	uint8_t reserved1[32];
+	uint32_t boot0_param[10];
+	uint32_t boot1_param[8];
 	struct image_entry images[4];
 	uint8_t reserved2[1064];
 	uint8_t hash[512];
@@ -491,6 +495,18 @@ static void rkcommon_print_header_v2(const struct header0_info_v2 *hdr)
 
 	printf("Rockchip Boot Image (v2)\n");
 
+	for (i = 0; i < ARRAY_SIZE(hdr->boot0_param); i++) {
+		val = le32_to_cpu(hdr->boot0_param[i]);
+		if (val)
+			printf("Boot0 %d: 0x%x\n", i, val);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(hdr->boot1_param); i++) {
+		val = le32_to_cpu(hdr->boot1_param[i]);
+		if (val)
+			printf("Boot1 %d: 0x%x\n", i, val);
+	}
+
 	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),
-- 
2.48.1

Re: [PATCH 3/6] rockchip: mkimage: Print boot0 and boot1 parameters

[Quentin Schulz]

Hi Jonas,

On 1/29/25 11:36 PM, Jonas Karlman wrote:
> The v2 image format embeds boot0 and boot1 parameters, the vendor tool
> boot_merger may write these parameters based on the rkboot miniall.ini
> files.
> 
> E.g. a RK3576 boot image may contain a boot1 parameter that signals
> BootROM or vendor blobs to use 1 GHz instead of the regular 24 MHz rate
> for the high precision timer.
> 
> Add support for printing boot0 and boot1 parameters, e.g.:
> 
>    > tools/mkimage -l rk3576_idblock_v1.09.107.img
>    Rockchip Boot Image (v2)
>    Boot1 2: 0x100
>    Image 1: 4096 @ 0x1000
>    - Load address: 0x3ffc0000
>    Image 2: 77824 @ 0x2000
>    - Load address: 0x3ff81000
>    Image 3: 262144 @ 0x15000
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>   tools/rkcommon.c | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
> index ad239917d2bd..324820717663 100644
> --- a/tools/rkcommon.c
> +++ b/tools/rkcommon.c
> @@ -62,6 +62,8 @@ struct image_entry {
>    * @boot_flag:	[3:0] hash type (0:none, 1:sha256, 2:sha512)
>    * @images:	images
>    * @hash:	hash or signature for header info
> + *
> + * Other fields are not used by U-Boot
>    */
>   struct header0_info_v2 {
>   	uint32_t magic;
> @@ -69,7 +71,9 @@ struct header0_info_v2 {
>   	uint16_t size;
>   	uint16_t num_images;
>   	uint32_t boot_flag;
> -	uint8_t reserved1[104];
> +	uint8_t reserved1[32];
> +	uint32_t boot0_param[10];
> +	uint32_t boot1_param[8];
>   	struct image_entry images[4];
>   	uint8_t reserved2[1064];
>   	uint8_t hash[512];
> @@ -491,6 +495,18 @@ static void rkcommon_print_header_v2(const struct header0_info_v2 *hdr)
>   
>   	printf("Rockchip Boot Image (v2)\n");
>   
> +	for (i = 0; i < ARRAY_SIZE(hdr->boot0_param); i++) {
> +		val = le32_to_cpu(hdr->boot0_param[i]);
> +		if (val)
> +			printf("Boot0 %d: 0x%x\n", i, val);
> +	}
> +

This seems to indicate that there are 10 4B params for boot0, is that 
correct? If that's the case I would at least add "param" before %d, the 
output looked odd to me at first glance.

If they aren't guaranteed to be individual 4B params, what about just 
printing the whole boot0_param in hex format?

> +	for (i = 0; i < ARRAY_SIZE(hdr->boot1_param); i++) {
> +		val = le32_to_cpu(hdr->boot1_param[i]);
> +		if (val)
> +			printf("Boot1 %d: 0x%x\n", i, val);
> +	}
> +

Same remark as for boot0 params instead with 8 4B params for boot1.

Cheers,
Quentin

Re: [PATCH 3/6] rockchip: mkimage: Print boot0 and boot1 parameters

[Jonas Karlman]

Hi Quentin,

On 2025-02-05 17:04, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 1/29/25 11:36 PM, Jonas Karlman wrote:
>> The v2 image format embeds boot0 and boot1 parameters, the vendor tool
>> boot_merger may write these parameters based on the rkboot miniall.ini
>> files.
>>
>> E.g. a RK3576 boot image may contain a boot1 parameter that signals
>> BootROM or vendor blobs to use 1 GHz instead of the regular 24 MHz rate
>> for the high precision timer.
>>
>> Add support for printing boot0 and boot1 parameters, e.g.:
>>
>>    > tools/mkimage -l rk3576_idblock_v1.09.107.img
>>    Rockchip Boot Image (v2)
>>    Boot1 2: 0x100
>>    Image 1: 4096 @ 0x1000
>>    - Load address: 0x3ffc0000
>>    Image 2: 77824 @ 0x2000
>>    - Load address: 0x3ff81000
>>    Image 3: 262144 @ 0x15000
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>>   tools/rkcommon.c | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
>> index ad239917d2bd..324820717663 100644
>> --- a/tools/rkcommon.c
>> +++ b/tools/rkcommon.c
>> @@ -62,6 +62,8 @@ struct image_entry {
>>    * @boot_flag:	[3:0] hash type (0:none, 1:sha256, 2:sha512)
>>    * @images:	images
>>    * @hash:	hash or signature for header info
>> + *
>> + * Other fields are not used by U-Boot
>>    */
>>   struct header0_info_v2 {
>>   	uint32_t magic;
>> @@ -69,7 +71,9 @@ struct header0_info_v2 {
>>   	uint16_t size;
>>   	uint16_t num_images;
>>   	uint32_t boot_flag;
>> -	uint8_t reserved1[104];
>> +	uint8_t reserved1[32];
>> +	uint32_t boot0_param[10];
>> +	uint32_t boot1_param[8];
>>   	struct image_entry images[4];
>>   	uint8_t reserved2[1064];
>>   	uint8_t hash[512];
>> @@ -491,6 +495,18 @@ static void rkcommon_print_header_v2(const struct header0_info_v2 *hdr)
>>   
>>   	printf("Rockchip Boot Image (v2)\n");
>>   
>> +	for (i = 0; i < ARRAY_SIZE(hdr->boot0_param); i++) {
>> +		val = le32_to_cpu(hdr->boot0_param[i]);
>> +		if (val)
>> +			printf("Boot0 %d: 0x%x\n", i, val);
>> +	}
>> +
> 
> This seems to indicate that there are 10 4B params for boot0, is that 
> correct? If that's the case I would at least add "param" before %d, the 
> output looked odd to me at first glance.

That should be correct and is what boot_merger can embed based on the
[BOOT0_PARAM] section and WORD_n (n=0-9) values from MINIALL.ini.

The only reason I skipped "param" was because I thought it looked
prettier to align the "BootX %d" and "Image %d" in the output, can
change to include "param" :-)

> 
> If they aren't guaranteed to be individual 4B params, what about just 
> printing the whole boot0_param in hex format?

There is only very few WORD_ values in use in linux-6.1-stan-rkr5 rkbin
MINIALL.ini files. So I opted to only print out the params that have a
value different from the default 0x0.

> 
>> +	for (i = 0; i < ARRAY_SIZE(hdr->boot1_param); i++) {
>> +		val = le32_to_cpu(hdr->boot1_param[i]);
>> +		if (val)
>> +			printf("Boot1 %d: 0x%x\n", i, val);
>> +	}
>> +
> 
> Same remark as for boot0 params instead with 8 4B params for boot1.

Correct, boot_merger embed WORD_n (n=0-7) from the [BOOT1_PARAM] section
and also only very few are in use.

My main test to validate this was to add different values to WORD_n
under BOOT0/BOOT1_PARAM and then compare with the generated idblock.img.

Regards,
Jonas

> 
> Cheers,
> Quentin

Re: [PATCH 3/6] rockchip: mkimage: Print boot0 and boot1 parameters

[Quentin Schulz]

Hi Jonas,

On 2/5/25 5:42 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2025-02-05 17:04, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 1/29/25 11:36 PM, Jonas Karlman wrote:
>>> The v2 image format embeds boot0 and boot1 parameters, the vendor tool
>>> boot_merger may write these parameters based on the rkboot miniall.ini
>>> files.
>>>
>>> E.g. a RK3576 boot image may contain a boot1 parameter that signals
>>> BootROM or vendor blobs to use 1 GHz instead of the regular 24 MHz rate
>>> for the high precision timer.
>>>
>>> Add support for printing boot0 and boot1 parameters, e.g.:
>>>
>>>     > tools/mkimage -l rk3576_idblock_v1.09.107.img
>>>     Rockchip Boot Image (v2)
>>>     Boot1 2: 0x100
>>>     Image 1: 4096 @ 0x1000
>>>     - Load address: 0x3ffc0000
>>>     Image 2: 77824 @ 0x2000
>>>     - Load address: 0x3ff81000
>>>     Image 3: 262144 @ 0x15000
>>>
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>> ---
>>>    tools/rkcommon.c | 18 +++++++++++++++++-
>>>    1 file changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
>>> index ad239917d2bd..324820717663 100644
>>> --- a/tools/rkcommon.c
>>> +++ b/tools/rkcommon.c
>>> @@ -62,6 +62,8 @@ struct image_entry {
>>>     * @boot_flag:	[3:0] hash type (0:none, 1:sha256, 2:sha512)
>>>     * @images:	images
>>>     * @hash:	hash or signature for header info
>>> + *
>>> + * Other fields are not used by U-Boot
>>>     */
>>>    struct header0_info_v2 {
>>>    	uint32_t magic;
>>> @@ -69,7 +71,9 @@ struct header0_info_v2 {
>>>    	uint16_t size;
>>>    	uint16_t num_images;
>>>    	uint32_t boot_flag;
>>> -	uint8_t reserved1[104];
>>> +	uint8_t reserved1[32];
>>> +	uint32_t boot0_param[10];
>>> +	uint32_t boot1_param[8];
>>>    	struct image_entry images[4];
>>>    	uint8_t reserved2[1064];
>>>    	uint8_t hash[512];
>>> @@ -491,6 +495,18 @@ static void rkcommon_print_header_v2(const struct header0_info_v2 *hdr)
>>>    
>>>    	printf("Rockchip Boot Image (v2)\n");
>>>    
>>> +	for (i = 0; i < ARRAY_SIZE(hdr->boot0_param); i++) {
>>> +		val = le32_to_cpu(hdr->boot0_param[i]);
>>> +		if (val)
>>> +			printf("Boot0 %d: 0x%x\n", i, val);
>>> +	}
>>> +
>>
>> This seems to indicate that there are 10 4B params for boot0, is that
>> correct? If that's the case I would at least add "param" before %d, the
>> output looked odd to me at first glance.
> 
> That should be correct and is what boot_merger can embed based on the
> [BOOT0_PARAM] section and WORD_n (n=0-9) values from MINIALL.ini.
> 
> The only reason I skipped "param" was because I thought it looked
> prettier to align the "BootX %d" and "Image %d" in the output, can
> change to include "param" :-)
> 

Hehe, could have seen:
Boot1:
- param2: 0x100
- param3: 0x500

as well. I am no UX expert so whatever works best for you :)

>>
>> If they aren't guaranteed to be individual 4B params, what about just
>> printing the whole boot0_param in hex format?
> 
> There is only very few WORD_ values in use in linux-6.1-stan-rkr5 rkbin
> MINIALL.ini files. So I opted to only print out the params that have a
> value different from the default 0x0.
> 
>>
>>> +	for (i = 0; i < ARRAY_SIZE(hdr->boot1_param); i++) {
>>> +		val = le32_to_cpu(hdr->boot1_param[i]);
>>> +		if (val)
>>> +			printf("Boot1 %d: 0x%x\n", i, val);
>>> +	}
>>> +
>>
>> Same remark as for boot0 params instead with 8 4B params for boot1.
> 
> Correct, boot_merger embed WORD_n (n=0-7) from the [BOOT1_PARAM] section
> and also only very few are in use.
> 
> My main test to validate this was to add different values to WORD_n
> under BOOT0/BOOT1_PARAM and then compare with the generated idblock.img.
> 

Cool. Do you know if there's some public sources with appropriate 
licensing for boot_merger for RK35xx? There used to be code in the 
U-Boot fork from Rockchip but they stopped updating boot_merger source a 
few years ago last time I checked.

Could be nice to have it in U-Boot too :)

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH 3/6] rockchip: mkimage: Print boot0 and boot1 parameters

[Jonas Karlman]

Hi Quentin,

On 2025-02-05 17:48, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 2/5/25 5:42 PM, Jonas Karlman wrote:
>> Hi Quentin,
>>
>> On 2025-02-05 17:04, Quentin Schulz wrote:
>>> Hi Jonas,
>>>
>>> On 1/29/25 11:36 PM, Jonas Karlman wrote:
>>>> The v2 image format embeds boot0 and boot1 parameters, the vendor tool
>>>> boot_merger may write these parameters based on the rkboot miniall.ini
>>>> files.
>>>>
>>>> E.g. a RK3576 boot image may contain a boot1 parameter that signals
>>>> BootROM or vendor blobs to use 1 GHz instead of the regular 24 MHz rate
>>>> for the high precision timer.
>>>>
>>>> Add support for printing boot0 and boot1 parameters, e.g.:
>>>>
>>>>     > tools/mkimage -l rk3576_idblock_v1.09.107.img
>>>>     Rockchip Boot Image (v2)
>>>>     Boot1 2: 0x100
>>>>     Image 1: 4096 @ 0x1000
>>>>     - Load address: 0x3ffc0000
>>>>     Image 2: 77824 @ 0x2000
>>>>     - Load address: 0x3ff81000
>>>>     Image 3: 262144 @ 0x15000
>>>>
>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>> ---
>>>>    tools/rkcommon.c | 18 +++++++++++++++++-
>>>>    1 file changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
>>>> index ad239917d2bd..324820717663 100644
>>>> --- a/tools/rkcommon.c
>>>> +++ b/tools/rkcommon.c
>>>> @@ -62,6 +62,8 @@ struct image_entry {
>>>>     * @boot_flag:	[3:0] hash type (0:none, 1:sha256, 2:sha512)
>>>>     * @images:	images
>>>>     * @hash:	hash or signature for header info
>>>> + *
>>>> + * Other fields are not used by U-Boot
>>>>     */
>>>>    struct header0_info_v2 {
>>>>    	uint32_t magic;
>>>> @@ -69,7 +71,9 @@ struct header0_info_v2 {
>>>>    	uint16_t size;
>>>>    	uint16_t num_images;
>>>>    	uint32_t boot_flag;
>>>> -	uint8_t reserved1[104];
>>>> +	uint8_t reserved1[32];
>>>> +	uint32_t boot0_param[10];
>>>> +	uint32_t boot1_param[8];
>>>>    	struct image_entry images[4];
>>>>    	uint8_t reserved2[1064];
>>>>    	uint8_t hash[512];
>>>> @@ -491,6 +495,18 @@ static void rkcommon_print_header_v2(const struct header0_info_v2 *hdr)
>>>>    
>>>>    	printf("Rockchip Boot Image (v2)\n");
>>>>    
>>>> +	for (i = 0; i < ARRAY_SIZE(hdr->boot0_param); i++) {
>>>> +		val = le32_to_cpu(hdr->boot0_param[i]);
>>>> +		if (val)
>>>> +			printf("Boot0 %d: 0x%x\n", i, val);
>>>> +	}
>>>> +
>>>
>>> This seems to indicate that there are 10 4B params for boot0, is that
>>> correct? If that's the case I would at least add "param" before %d, the
>>> output looked odd to me at first glance.
>>
>> That should be correct and is what boot_merger can embed based on the
>> [BOOT0_PARAM] section and WORD_n (n=0-9) values from MINIALL.ini.
>>
>> The only reason I skipped "param" was because I thought it looked
>> prettier to align the "BootX %d" and "Image %d" in the output, can
>> change to include "param" :-)
>>
> 
> Hehe, could have seen:
> Boot1:
> - param2: 0x100
> - param3: 0x500
> 
> as well. I am no UX expert so whatever works best for you :)

I will see what I can do for v2.

> 
>>>
>>> If they aren't guaranteed to be individual 4B params, what about just
>>> printing the whole boot0_param in hex format?
>>
>> There is only very few WORD_ values in use in linux-6.1-stan-rkr5 rkbin
>> MINIALL.ini files. So I opted to only print out the params that have a
>> value different from the default 0x0.
>>
>>>
>>>> +	for (i = 0; i < ARRAY_SIZE(hdr->boot1_param); i++) {
>>>> +		val = le32_to_cpu(hdr->boot1_param[i]);
>>>> +		if (val)
>>>> +			printf("Boot1 %d: 0x%x\n", i, val);
>>>> +	}
>>>> +
>>>
>>> Same remark as for boot0 params instead with 8 4B params for boot1.
>>
>> Correct, boot_merger embed WORD_n (n=0-7) from the [BOOT1_PARAM] section
>> and also only very few are in use.
>>
>> My main test to validate this was to add different values to WORD_n
>> under BOOT0/BOOT1_PARAM and then compare with the generated idblock.img.
>>
> 
> Cool. Do you know if there's some public sources with appropriate 
> licensing for boot_merger for RK35xx? There used to be code in the 
> U-Boot fork from Rockchip but they stopped updating boot_merger source a 
> few years ago last time I checked.

Unfortunately I do not, only started looking into these extra parameters
while experimenting with RK3576 because I found following in
RK3576MINIALL.ini:

  [BOOT1_PARAM]
  WORD_2=0x100

Turned out that this value is used to signal that BootROM, TPL or SPL
should use 1 GHz rate for the high-precision timer. This did not work
well with mainline U-Boot because mainline expect a hard-coded value of
24 MHz or you start seeing a lot of quick timeouts.

Including them in output at least meant is was not fully a waste of time.

Regards,
Jonas

> 
> Could be nice to have it in U-Boot too :)
> 
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> 
> Thanks!
> Quentin

[PATCH 4/6] rockchip: mkimage: Add option to change image offset alignment

[Jonas Karlman]

The vendor boot_merger tool support a ALIGN parameter that is used to
define offset alignment of the embedded images.

Vendor use this for RK3576 to change offset alignment from the common
2 KiB to 4 KiB, presumably it may have something to do with UFS.
Testing with eMMC has shown that using a 512-byte alignment also work.

Add support for overriding offset alignment in case this is needed for
e.g. RK3576 in the future.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/rkcommon.c | 75 +++++++++++++++++++++++++++++++-----------------
 tools/rkcommon.h |  2 --
 2 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/tools/rkcommon.c b/tools/rkcommon.c
index 324820717663..542aca931693 100644
--- a/tools/rkcommon.c
+++ b/tools/rkcommon.c
@@ -124,6 +124,7 @@ struct spl_info {
 	const uint32_t spl_size;
 	const bool spl_rc4;
 	const uint32_t header_ver;
+	const uint32_t align;
 };
 
 static struct spl_info spl_infos[] = {
@@ -181,14 +182,19 @@ static struct spl_info *rkcommon_get_spl_info(char *imagename)
 	return NULL;
 }
 
-static int rkcommon_get_aligned_size(struct image_tool_params *params,
-				     const char *fname)
+static bool rkcommon_is_header_v2(struct image_tool_params *params)
 {
-	int size;
+	struct spl_info *info = rkcommon_get_spl_info(params->imagename);
 
-	size = imagetool_get_filesize(params, fname);
-	if (size < 0)
-		return -1;
+	return (info->header_ver == RK_HEADER_V2);
+}
+
+static int rkcommon_get_aligned_size(struct image_tool_params *params, int size)
+{
+	struct spl_info *info = rkcommon_get_spl_info(params->imagename);
+
+	if (info->align)
+		return ROUND(size, info->align * RK_BLK_SIZE);
 
 	/*
 	 * Pad to a 2KB alignment, as required for init/boot size by the ROM
@@ -197,6 +203,27 @@ static int rkcommon_get_aligned_size(struct image_tool_params *params,
 	return ROUND(size, RK_SIZE_ALIGN);
 }
 
+static int rkcommon_get_header_size(struct image_tool_params *params)
+{
+	int header_size = rkcommon_is_header_v2(params) ?
+			  sizeof(struct header0_info_v2) :
+			  sizeof(struct header0_info);
+
+	return rkcommon_get_aligned_size(params, header_size);
+}
+
+static int rkcommon_get_aligned_filesize(struct image_tool_params *params,
+					 const char *fname)
+{
+	int size;
+
+	size = imagetool_get_filesize(params, fname);
+	if (size < 0)
+		return -1;
+
+	return rkcommon_get_aligned_size(params, size);
+}
+
 int rkcommon_check_params(struct image_tool_params *params)
 {
 	int i, size;
@@ -219,14 +246,14 @@ int rkcommon_check_params(struct image_tool_params *params)
 		spl_params.boot_file += 1;
 	}
 
-	size = rkcommon_get_aligned_size(params, spl_params.init_file);
+	size = rkcommon_get_aligned_filesize(params, spl_params.init_file);
 	if (size < 0)
 		return EXIT_FAILURE;
 	spl_params.init_size = size;
 
 	/* Boot file is optional, and only for back-to-bootrom functionality. */
 	if (spl_params.boot_file) {
-		size = rkcommon_get_aligned_size(params, spl_params.boot_file);
+		size = rkcommon_get_aligned_filesize(params, spl_params.boot_file);
 		if (size < 0)
 			return EXIT_FAILURE;
 		spl_params.boot_size = size;
@@ -283,13 +310,6 @@ bool rkcommon_need_rc4_spl(struct image_tool_params *params)
 	return info->spl_rc4;
 }
 
-bool rkcommon_is_header_v2(struct image_tool_params *params)
-{
-	struct spl_info *info = rkcommon_get_spl_info(params->imagename);
-
-	return (info->header_ver == RK_HEADER_V2);
-}
-
 static void do_sha256_hash(uint8_t *buf, uint32_t size, uint8_t *out)
 {
 	sha256_context ctx;
@@ -302,12 +322,13 @@ static void do_sha256_hash(uint8_t *buf, uint32_t size, uint8_t *out)
 static void rkcommon_set_header0(void *buf, struct image_tool_params *params)
 {
 	struct header0_info *hdr = buf;
-	uint32_t init_boot_size;
+	uint32_t init_boot_size, init_offset;
 
-	memset(buf, '\0', RK_INIT_OFFSET * RK_BLK_SIZE);
+	init_offset = rkcommon_get_header_size(params) / RK_BLK_SIZE;
+	memset(buf, '\0', init_offset * RK_BLK_SIZE);
 	hdr->magic = cpu_to_le32(RK_MAGIC);
 	hdr->disable_rc4 = cpu_to_le32(!rkcommon_need_rc4_spl(params));
-	hdr->init_offset = cpu_to_le16(RK_INIT_OFFSET);
+	hdr->init_offset = cpu_to_le16(init_offset);
 	hdr->init_size   = cpu_to_le16(spl_params.init_size / RK_BLK_SIZE);
 
 	/*
@@ -335,10 +356,10 @@ static void rkcommon_set_header0_v2(void *buf, struct image_tool_params *params)
 	uint8_t *image_ptr = NULL;
 	int i;
 
-	memset(buf, '\0', RK_INIT_OFFSET * RK_BLK_SIZE);
+	sector_offset = rkcommon_get_header_size(params) / RK_BLK_SIZE;
+	memset(buf, '\0', sector_offset * RK_BLK_SIZE);
 	hdr->magic = cpu_to_le32(RK_MAGIC_V2);
 	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;
 
@@ -364,11 +385,12 @@ static void rkcommon_set_header0_v2(void *buf, struct image_tool_params *params)
 void rkcommon_set_header(void *buf,  struct stat *sbuf,  int ifd,
 			 struct image_tool_params *params)
 {
-	struct header1_info *hdr = buf + RK_SPL_HDR_START;
-
 	if (rkcommon_is_header_v2(params)) {
 		rkcommon_set_header0_v2(buf, params);
 	} else {
+		int header_size = rkcommon_get_header_size(params);
+		struct header1_info *hdr = buf + header_size;
+
 		rkcommon_set_header0(buf, params);
 
 		/* Set up the SPL name (i.e. copy spl_hdr over) */
@@ -376,12 +398,12 @@ void rkcommon_set_header(void *buf,  struct stat *sbuf,  int ifd,
 			memcpy(&hdr->magic, rkcommon_get_spl_hdr(params), RK_SPL_HDR_SIZE);
 
 		if (rkcommon_need_rc4_spl(params))
-			rkcommon_rc4_encode_spl(buf, RK_SPL_HDR_START,
+			rkcommon_rc4_encode_spl(buf, header_size,
 						spl_params.init_size);
 
 		if (spl_params.boot_file) {
 			if (rkcommon_need_rc4_spl(params))
-				rkcommon_rc4_encode_spl(buf + RK_SPL_HDR_START,
+				rkcommon_rc4_encode_spl(buf + header_size,
 							spl_params.init_size,
 							spl_params.boot_size);
 		}
@@ -606,7 +628,7 @@ int rkcommon_vrec_header(struct image_tool_params *params,
 	 * 4 bytes of these images can safely be overwritten using the
 	 * boot magic.
 	 */
-	tparams->header_size = RK_SPL_HDR_START;
+	tparams->header_size = rkcommon_get_header_size(params);
 
 	/* Allocate, clear and install the header */
 	tparams->hdr = malloc(tparams->header_size);
@@ -624,7 +646,8 @@ int rkcommon_vrec_header(struct image_tool_params *params,
 	params->orig_file_size = tparams->header_size +
 		spl_params.init_size + spl_params.boot_size;
 
-	params->file_size = ROUND(params->orig_file_size, RK_SIZE_ALIGN);
+	params->file_size = rkcommon_get_aligned_size(params,
+						      params->orig_file_size);
 
 	/* Ignoring pad len, since we are using our own copy_image() */
 	return 0;
diff --git a/tools/rkcommon.h b/tools/rkcommon.h
index 5d2770a80f1c..c887a659a953 100644
--- a/tools/rkcommon.h
+++ b/tools/rkcommon.h
@@ -10,9 +10,7 @@
 enum {
 	RK_BLK_SIZE		= 512,
 	RK_SIZE_ALIGN		= 2048,
-	RK_INIT_OFFSET		= 4,
 	RK_MAX_BOOT_SIZE	= 512 << 10,
-	RK_SPL_HDR_START	= RK_INIT_OFFSET * RK_BLK_SIZE,
 	RK_SPL_HDR_SIZE		= 4,
 };
 
-- 
2.48.1

Re: [PATCH 4/6] rockchip: mkimage: Add option to change image offset alignment

[Quentin Schulz]

Hi Jonas,

On 1/29/25 11:36 PM, Jonas Karlman wrote:
> The vendor boot_merger tool support a ALIGN parameter that is used to
> define offset alignment of the embedded images.
> 
> Vendor use this for RK3576 to change offset alignment from the common
> 2 KiB to 4 KiB, presumably it may have something to do with UFS.
> Testing with eMMC has shown that using a 512-byte alignment also work.
> 
> Add support for overriding offset alignment in case this is needed for
> e.g. RK3576 in the future.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>   tools/rkcommon.c | 75 +++++++++++++++++++++++++++++++-----------------
>   tools/rkcommon.h |  2 --
>   2 files changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
> index 324820717663..542aca931693 100644
> --- a/tools/rkcommon.c
> +++ b/tools/rkcommon.c
> @@ -124,6 +124,7 @@ struct spl_info {
>   	const uint32_t spl_size;
>   	const bool spl_rc4;
>   	const uint32_t header_ver;
> +	const uint32_t align;

Missing documentation update above the struct definition.

>   };
>   
>   static struct spl_info spl_infos[] = {
> @@ -181,14 +182,19 @@ static struct spl_info *rkcommon_get_spl_info(char *imagename)
>   	return NULL;
>   }
>   
> -static int rkcommon_get_aligned_size(struct image_tool_params *params,
> -				     const char *fname)
> +static bool rkcommon_is_header_v2(struct image_tool_params *params)
>   {
> -	int size;
> +	struct spl_info *info = rkcommon_get_spl_info(params->imagename);
>   
> -	size = imagetool_get_filesize(params, fname);
> -	if (size < 0)
> -		return -1;
> +	return (info->header_ver == RK_HEADER_V2);
> +}
> +
> +static int rkcommon_get_aligned_size(struct image_tool_params *params, int size)

Maybe use an unsigned type here as a size will be guaranteed to be positive?

> +{
> +	struct spl_info *info = rkcommon_get_spl_info(params->imagename);
> +
> +	if (info->align)
> +		return ROUND(size, info->align * RK_BLK_SIZE);
>   

Why not make info->align be 4 (RK_SIZE_ALIGN / RK_BLK_SIZE) if unset?

I like the change, though I felt splitting in more commits would have 
made the review easier, e.g.:

- split part of get_aligned_size into get_aligned_filesize
- migrate hardcoded RK_INIT_OFFSET to get_aligned_size
- migrate hardcoded RK_SPL_HDR_START to get_aligned_size
- add align to spl_info + handling in get_aligned_size

Looks good to me otherwise!

Cheers,
Quentin

Re: [PATCH 4/6] rockchip: mkimage: Add option to change image offset alignment

[Jonas Karlman]

Hi Quentin,

On 2025-02-05 17:29, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 1/29/25 11:36 PM, Jonas Karlman wrote:
>> The vendor boot_merger tool support a ALIGN parameter that is used to
>> define offset alignment of the embedded images.
>>
>> Vendor use this for RK3576 to change offset alignment from the common
>> 2 KiB to 4 KiB, presumably it may have something to do with UFS.
>> Testing with eMMC has shown that using a 512-byte alignment also work.
>>
>> Add support for overriding offset alignment in case this is needed for
>> e.g. RK3576 in the future.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>>   tools/rkcommon.c | 75 +++++++++++++++++++++++++++++++-----------------
>>   tools/rkcommon.h |  2 --
>>   2 files changed, 49 insertions(+), 28 deletions(-)
>>
>> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
>> index 324820717663..542aca931693 100644
>> --- a/tools/rkcommon.c
>> +++ b/tools/rkcommon.c
>> @@ -124,6 +124,7 @@ struct spl_info {
>>   	const uint32_t spl_size;
>>   	const bool spl_rc4;
>>   	const uint32_t header_ver;
>> +	const uint32_t align;
> 
> Missing documentation update above the struct definition.

Will fix in v2.

> 
>>   };
>>   
>>   static struct spl_info spl_infos[] = {
>> @@ -181,14 +182,19 @@ static struct spl_info *rkcommon_get_spl_info(char *imagename)
>>   	return NULL;
>>   }
>>   
>> -static int rkcommon_get_aligned_size(struct image_tool_params *params,
>> -				     const char *fname)
>> +static bool rkcommon_is_header_v2(struct image_tool_params *params)
>>   {
>> -	int size;
>> +	struct spl_info *info = rkcommon_get_spl_info(params->imagename);
>>   
>> -	size = imagetool_get_filesize(params, fname);
>> -	if (size < 0)
>> -		return -1;
>> +	return (info->header_ver == RK_HEADER_V2);
>> +}
>> +
>> +static int rkcommon_get_aligned_size(struct image_tool_params *params, int size)
> 
> Maybe use an unsigned type here as a size will be guaranteed to be positive?

Use of unsigned may be more correct, can adjust in v2.

> 
>> +{
>> +	struct spl_info *info = rkcommon_get_spl_info(params->imagename);
>> +
>> +	if (info->align)
>> +		return ROUND(size, info->align * RK_BLK_SIZE);
>>   
> 
> Why not make info->align be 4 (RK_SIZE_ALIGN / RK_BLK_SIZE) if unset?

Can probably change to something similar in v2, spl_infos should
probably be a const but can introduce a local align value (what I used
in an early version).

> 
> I like the change, though I felt splitting in more commits would have 
> made the review easier, e.g.:
> 
> - split part of get_aligned_size into get_aligned_filesize
> - migrate hardcoded RK_INIT_OFFSET to get_aligned_size
> - migrate hardcoded RK_SPL_HDR_START to get_aligned_size
> - add align to spl_info + handling in get_aligned_size

Hehe, during development I had everything in this series + rk3576
sd-card workaround as a single commit, and I thought current split into
6+1 patches was more than enough ;-)

Will see if I can split this further, it will be with a cost of some
quick/bad commit messages.

Regards,
Jonas

> 
> Looks good to me otherwise!
> 
> Cheers,
> Quentin

[PATCH 5/6] rockchip: mkimage: Add support for up to 4 input files

[Jonas Karlman]

The v2 image format can support up to 4 embedded images that can be
loaded by the BootROM using the back-to-bootrom method.

Currently two input files can be passed in using the datafile parameter,
separated by a colon (":").

Extend the datafile parameter parsing to support up to 4 input files
separated by a colon (":") for use with the v2 image format.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/rkcommon.c | 93 +++++++++++++++++++++++-------------------------
 1 file changed, 44 insertions(+), 49 deletions(-)

diff --git a/tools/rkcommon.c b/tools/rkcommon.c
index 542aca931693..4ff48e81a636 100644
--- a/tools/rkcommon.c
+++ b/tools/rkcommon.c
@@ -148,17 +148,15 @@ static struct spl_info spl_infos[] = {
 /**
  * struct spl_params - spl params parsed in check_params()
  *
- * @init_file:		Init data file path
- * @init_size:		Aligned size of init data in bytes
- * @boot_file:		Boot data file path
- * @boot_size:		Aligned size of boot data in bytes
+ * @file:	image file path
+ * @size:	aligned size of image in bytes
  */
 
 struct spl_params {
-	char *init_file;
-	uint32_t init_size;
-	char *boot_file;
-	uint32_t boot_size;
+	struct {
+		char *file;
+		uint32_t size;
+	} images[4];
 };
 
 static struct spl_params spl_params = { 0 };
@@ -238,31 +236,32 @@ int rkcommon_check_params(struct image_tool_params *params)
 	if (!rkcommon_get_spl_info(params->imagename))
 		goto err_spl_info;
 
-	spl_params.init_file = params->datafile;
+	spl_params.images[0].file = params->datafile;
+	for (i = 1; i < ARRAY_SIZE(spl_params.images); i++) {
+		spl_params.images[i].file =
+				strchr(spl_params.images[i - 1].file, ':');
+		if (!spl_params.images[i].file)
+			break;
 
-	spl_params.boot_file = strchr(spl_params.init_file, ':');
-	if (spl_params.boot_file) {
-		*spl_params.boot_file = '\0';
-		spl_params.boot_file += 1;
+		*spl_params.images[i].file = '\0';
+		spl_params.images[i].file += 1;
 	}
 
-	size = rkcommon_get_aligned_filesize(params, spl_params.init_file);
-	if (size < 0)
-		return EXIT_FAILURE;
-	spl_params.init_size = size;
+	for (i = 0; i < ARRAY_SIZE(spl_params.images); i++) {
+		if (!spl_params.images[i].file)
+			break;
 
-	/* Boot file is optional, and only for back-to-bootrom functionality. */
-	if (spl_params.boot_file) {
-		size = rkcommon_get_aligned_filesize(params, spl_params.boot_file);
+		size = rkcommon_get_aligned_filesize(params,
+						     spl_params.images[i].file);
 		if (size < 0)
 			return EXIT_FAILURE;
-		spl_params.boot_size = size;
+		spl_params.images[i].size = size;
 	}
 
-	if (spl_params.init_size > rkcommon_get_spl_size(params)) {
+	if (spl_params.images[0].size > rkcommon_get_spl_size(params)) {
 		fprintf(stderr,
 			"Error: SPL image is too large (size %#x than %#x)\n",
-			spl_params.init_size, rkcommon_get_spl_size(params));
+			spl_params.images[0].size, rkcommon_get_spl_size(params));
 		return EXIT_FAILURE;
 	}
 
@@ -329,7 +328,7 @@ static void rkcommon_set_header0(void *buf, struct image_tool_params *params)
 	hdr->magic = cpu_to_le32(RK_MAGIC);
 	hdr->disable_rc4 = cpu_to_le32(!rkcommon_need_rc4_spl(params));
 	hdr->init_offset = cpu_to_le16(init_offset);
-	hdr->init_size   = cpu_to_le16(spl_params.init_size / RK_BLK_SIZE);
+	hdr->init_size = cpu_to_le16(spl_params.images[0].size / RK_BLK_SIZE);
 
 	/*
 	 * init_boot_size needs to be set, as it is read by the BootROM
@@ -339,10 +338,11 @@ static void rkcommon_set_header0(void *buf, struct image_tool_params *params)
 	 * see https://lists.denx.de/pipermail/u-boot/2017-May/293267.html
 	 * for a more detailed explanation by Andy Yan
 	 */
-	if (spl_params.boot_file)
-		init_boot_size = spl_params.init_size + spl_params.boot_size;
+	if (spl_params.images[1].file)
+		init_boot_size = spl_params.images[0].size +
+				 spl_params.images[1].size;
 	else
-		init_boot_size = spl_params.init_size + RK_MAX_BOOT_SIZE;
+		init_boot_size = spl_params.images[0].size + RK_MAX_BOOT_SIZE;
 	hdr->init_boot_size = cpu_to_le16(init_boot_size / RK_BLK_SIZE);
 
 	rc4_encode(buf, RK_BLK_SIZE, rc4_key);
@@ -352,7 +352,6 @@ static void rkcommon_set_header0_v2(void *buf, struct image_tool_params *params)
 {
 	struct header0_info_v2 *hdr = buf;
 	uint32_t sector_offset, image_sector_count;
-	uint32_t image_size_array[2];
 	uint8_t *image_ptr = NULL;
 	int i;
 
@@ -360,19 +359,17 @@ static void rkcommon_set_header0_v2(void *buf, struct image_tool_params *params)
 	memset(buf, '\0', sector_offset * RK_BLK_SIZE);
 	hdr->magic = cpu_to_le32(RK_MAGIC_V2);
 	hdr->boot_flag = cpu_to_le32(HASH_SHA256);
-	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])
+	for (i = 0; i < ARRAY_SIZE(spl_params.images); i++) {
+		if (!spl_params.images[i].size)
 			break;
-		image_sector_count = image_size_array[i] / RK_BLK_SIZE;
+		image_sector_count = spl_params.images[i].size / RK_BLK_SIZE;
 		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;
-		do_sha256_hash(image_ptr, image_size_array[i],
+		do_sha256_hash(image_ptr, spl_params.images[i].size,
 			       hdr->images[i].hash);
 		sector_offset = sector_offset + image_sector_count;
 	}
@@ -399,13 +396,13 @@ void rkcommon_set_header(void *buf,  struct stat *sbuf,  int ifd,
 
 		if (rkcommon_need_rc4_spl(params))
 			rkcommon_rc4_encode_spl(buf, header_size,
-						spl_params.init_size);
+						spl_params.images[0].size);
 
-		if (spl_params.boot_file) {
+		if (spl_params.images[1].file) {
 			if (rkcommon_need_rc4_spl(params))
 				rkcommon_rc4_encode_spl(buf + header_size,
-							spl_params.init_size,
-							spl_params.boot_size);
+							spl_params.images[0].size,
+							spl_params.images[1].size);
 		}
 	}
 }
@@ -643,8 +640,9 @@ int rkcommon_vrec_header(struct image_tool_params *params,
 	 * We need to store the original file-size (i.e. before padding), as
 	 * imagetool does not set this during its adjustment of file_size.
 	 */
-	params->orig_file_size = tparams->header_size +
-		spl_params.init_size + spl_params.boot_size;
+	params->orig_file_size = tparams->header_size;
+	for (int i = 0; i < ARRAY_SIZE(spl_params.images); i++)
+		params->orig_file_size += spl_params.images[i].size;
 
 	params->file_size = rkcommon_get_aligned_size(params,
 						      params->orig_file_size);
@@ -731,16 +729,13 @@ err_close:
 
 int rockchip_copy_image(int ifd, struct image_tool_params *params)
 {
-	int ret;
-
-	ret = copy_file(params, ifd, spl_params.init_file,
-			spl_params.init_size);
-	if (ret)
-		return ret;
+	int i, ret;
 
-	if (spl_params.boot_file) {
-		ret = copy_file(params, ifd, spl_params.boot_file,
-				spl_params.boot_size);
+	for (i = 0; i < ARRAY_SIZE(spl_params.images); i++) {
+		if (!spl_params.images[i].size)
+			break;
+		ret = copy_file(params, ifd, spl_params.images[i].file,
+				spl_params.images[i].size);
 		if (ret)
 			return ret;
 	}
-- 
2.48.1

Re: [PATCH 5/6] rockchip: mkimage: Add support for up to 4 input files

[Quentin Schulz]

Hi Jonas,

On 1/29/25 11:36 PM, Jonas Karlman wrote:
> The v2 image format can support up to 4 embedded images that can be
> loaded by the BootROM using the back-to-bootrom method.
> 
> Currently two input files can be passed in using the datafile parameter,
> separated by a colon (":").
> 
> Extend the datafile parameter parsing to support up to 4 input files
> separated by a colon (":") for use with the v2 image format.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>   tools/rkcommon.c | 93 +++++++++++++++++++++++-------------------------
>   1 file changed, 44 insertions(+), 49 deletions(-)
> 
> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
> index 542aca931693..4ff48e81a636 100644
> --- a/tools/rkcommon.c
> +++ b/tools/rkcommon.c
> @@ -148,17 +148,15 @@ static struct spl_info spl_infos[] = {
>   /**
>    * struct spl_params - spl params parsed in check_params()
>    *
> - * @init_file:		Init data file path
> - * @init_size:		Aligned size of init data in bytes
> - * @boot_file:		Boot data file path
> - * @boot_size:		Aligned size of boot data in bytes
> + * @file:	image file path
> + * @size:	aligned size of image in bytes

Not really matching reality though. Could make it easier maybe to have 
an intermediary

struct spl_params_image {
     char *file;
     uint32_t size;
};

and then have

struct spl_params {
     struct spl_params_image images[4];
};

?

>    */
>   
>   struct spl_params {
> -	char *init_file;
> -	uint32_t init_size;
> -	char *boot_file;
> -	uint32_t boot_size;
> +	struct {
> +		char *file;
> +		uint32_t size;
> +	} images[4];
>   };
>   
>   static struct spl_params spl_params = { 0 };
> @@ -238,31 +236,32 @@ int rkcommon_check_params(struct image_tool_params *params)
>   	if (!rkcommon_get_spl_info(params->imagename))
>   		goto err_spl_info;
>   
> -	spl_params.init_file = params->datafile;
> +	spl_params.images[0].file = params->datafile;
> +	for (i = 1; i < ARRAY_SIZE(spl_params.images); i++) {
> +		spl_params.images[i].file =
> +				strchr(spl_params.images[i - 1].file, ':');
> +		if (!spl_params.images[i].file)
> +			break;
>   
> -	spl_params.boot_file = strchr(spl_params.init_file, ':');
> -	if (spl_params.boot_file) {
> -		*spl_params.boot_file = '\0';
> -		spl_params.boot_file += 1;
> +		*spl_params.images[i].file = '\0';
> +		spl_params.images[i].file += 1;
>   	}
>   
> -	size = rkcommon_get_aligned_filesize(params, spl_params.init_file);
> -	if (size < 0)
> -		return EXIT_FAILURE;
> -	spl_params.init_size = size;
> +	for (i = 0; i < ARRAY_SIZE(spl_params.images); i++) {
> +		if (!spl_params.images[i].file)
> +			break;
>   
> -	/* Boot file is optional, and only for back-to-bootrom functionality. */
> -	if (spl_params.boot_file) {
> -		size = rkcommon_get_aligned_filesize(params, spl_params.boot_file);
> +		size = rkcommon_get_aligned_filesize(params,
> +						     spl_params.images[i].file);
>   		if (size < 0)
>   			return EXIT_FAILURE;
> -		spl_params.boot_size = size;
> +		spl_params.images[i].size = size;
>   	}
>   

Can't we merge the two for-loops?

The patch diff makes sense to me :)

Cheers,
Quentin

Re: [PATCH 5/6] rockchip: mkimage: Add support for up to 4 input files

[Jonas Karlman]

Hi Quentin,

On 2025-02-05 17:43, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 1/29/25 11:36 PM, Jonas Karlman wrote:
>> The v2 image format can support up to 4 embedded images that can be
>> loaded by the BootROM using the back-to-bootrom method.
>>
>> Currently two input files can be passed in using the datafile parameter,
>> separated by a colon (":").
>>
>> Extend the datafile parameter parsing to support up to 4 input files
>> separated by a colon (":") for use with the v2 image format.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>>   tools/rkcommon.c | 93 +++++++++++++++++++++++-------------------------
>>   1 file changed, 44 insertions(+), 49 deletions(-)
>>
>> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
>> index 542aca931693..4ff48e81a636 100644
>> --- a/tools/rkcommon.c
>> +++ b/tools/rkcommon.c
>> @@ -148,17 +148,15 @@ static struct spl_info spl_infos[] = {
>>   /**
>>    * struct spl_params - spl params parsed in check_params()
>>    *
>> - * @init_file:		Init data file path
>> - * @init_size:		Aligned size of init data in bytes
>> - * @boot_file:		Boot data file path
>> - * @boot_size:		Aligned size of boot data in bytes
>> + * @file:	image file path
>> + * @size:	aligned size of image in bytes
> 
> Not really matching reality though. Could make it easier maybe to have 
> an intermediary
> 
> struct spl_params_image {
>      char *file;
>      uint32_t size;
> };
> 
> and then have
> 
> struct spl_params {
>      struct spl_params_image images[4];
> };
> 
> ?

Sound good, will use in v2.

> 
>>    */
>>   
>>   struct spl_params {
>> -	char *init_file;
>> -	uint32_t init_size;
>> -	char *boot_file;
>> -	uint32_t boot_size;
>> +	struct {
>> +		char *file;
>> +		uint32_t size;
>> +	} images[4];
>>   };
>>   
>>   static struct spl_params spl_params = { 0 };
>> @@ -238,31 +236,32 @@ int rkcommon_check_params(struct image_tool_params *params)
>>   	if (!rkcommon_get_spl_info(params->imagename))
>>   		goto err_spl_info;
>>   
>> -	spl_params.init_file = params->datafile;
>> +	spl_params.images[0].file = params->datafile;
>> +	for (i = 1; i < ARRAY_SIZE(spl_params.images); i++) {
>> +		spl_params.images[i].file =
>> +				strchr(spl_params.images[i - 1].file, ':');
>> +		if (!spl_params.images[i].file)
>> +			break;
>>   
>> -	spl_params.boot_file = strchr(spl_params.init_file, ':');
>> -	if (spl_params.boot_file) {
>> -		*spl_params.boot_file = '\0';
>> -		spl_params.boot_file += 1;
>> +		*spl_params.images[i].file = '\0';
>> +		spl_params.images[i].file += 1;
>>   	}
>>   
>> -	size = rkcommon_get_aligned_filesize(params, spl_params.init_file);
>> -	if (size < 0)
>> -		return EXIT_FAILURE;
>> -	spl_params.init_size = size;
>> +	for (i = 0; i < ARRAY_SIZE(spl_params.images); i++) {
>> +		if (!spl_params.images[i].file)
>> +			break;
>>   
>> -	/* Boot file is optional, and only for back-to-bootrom functionality. */
>> -	if (spl_params.boot_file) {
>> -		size = rkcommon_get_aligned_filesize(params, spl_params.boot_file);
>> +		size = rkcommon_get_aligned_filesize(params,
>> +						     spl_params.images[i].file);
>>   		if (size < 0)
>>   			return EXIT_FAILURE;
>> -		spl_params.boot_size = size;
>> +		spl_params.images[i].size = size;
>>   	}
>>   
> 
> Can't we merge the two for-loops?

Possible, suspect I kept it as two loops to avoid having to work on [i]
and [i - 1] too much in same loop. Do you have any suggestion on how to
merge the two for-loops to simplify this?

Regards,
Jonas

> 
> The patch diff makes sense to me :)
> 
> Cheers,
> Quentin

Re: [PATCH 5/6] rockchip: mkimage: Add support for up to 4 input files

[Quentin Schulz]

On 2/5/25 8:00 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2025-02-05 17:43, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 1/29/25 11:36 PM, Jonas Karlman wrote:
>>> The v2 image format can support up to 4 embedded images that can be
>>> loaded by the BootROM using the back-to-bootrom method.
>>>
>>> Currently two input files can be passed in using the datafile parameter,
>>> separated by a colon (":").
>>>
>>> Extend the datafile parameter parsing to support up to 4 input files
>>> separated by a colon (":") for use with the v2 image format.
>>>
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>> ---
>>>    tools/rkcommon.c | 93 +++++++++++++++++++++++-------------------------
>>>    1 file changed, 44 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
>>> index 542aca931693..4ff48e81a636 100644
>>> --- a/tools/rkcommon.c
>>> +++ b/tools/rkcommon.c
>>> @@ -148,17 +148,15 @@ static struct spl_info spl_infos[] = {
>>>    /**
>>>     * struct spl_params - spl params parsed in check_params()
>>>     *
>>> - * @init_file:		Init data file path
>>> - * @init_size:		Aligned size of init data in bytes
>>> - * @boot_file:		Boot data file path
>>> - * @boot_size:		Aligned size of boot data in bytes
>>> + * @file:	image file path
>>> + * @size:	aligned size of image in bytes
>>
>> Not really matching reality though. Could make it easier maybe to have
>> an intermediary
>>
>> struct spl_params_image {
>>       char *file;
>>       uint32_t size;
>> };
>>
>> and then have
>>
>> struct spl_params {
>>       struct spl_params_image images[4];
>> };
>>
>> ?
> 
> Sound good, will use in v2.
> 
>>
>>>     */
>>>    
>>>    struct spl_params {
>>> -	char *init_file;
>>> -	uint32_t init_size;
>>> -	char *boot_file;
>>> -	uint32_t boot_size;
>>> +	struct {
>>> +		char *file;
>>> +		uint32_t size;
>>> +	} images[4];
>>>    };
>>>    
>>>    static struct spl_params spl_params = { 0 };
>>> @@ -238,31 +236,32 @@ int rkcommon_check_params(struct image_tool_params *params)
>>>    	if (!rkcommon_get_spl_info(params->imagename))
>>>    		goto err_spl_info;
>>>    
>>> -	spl_params.init_file = params->datafile;
>>> +	spl_params.images[0].file = params->datafile;
>>> +	for (i = 1; i < ARRAY_SIZE(spl_params.images); i++) {
>>> +		spl_params.images[i].file =
>>> +				strchr(spl_params.images[i - 1].file, ':');
>>> +		if (!spl_params.images[i].file)
>>> +			break;
>>>    
>>> -	spl_params.boot_file = strchr(spl_params.init_file, ':');
>>> -	if (spl_params.boot_file) {
>>> -		*spl_params.boot_file = '\0';
>>> -		spl_params.boot_file += 1;
>>> +		*spl_params.images[i].file = '\0';
>>> +		spl_params.images[i].file += 1;
>>>    	}
>>>    
>>> -	size = rkcommon_get_aligned_filesize(params, spl_params.init_file);
>>> -	if (size < 0)
>>> -		return EXIT_FAILURE;
>>> -	spl_params.init_size = size;
>>> +	for (i = 0; i < ARRAY_SIZE(spl_params.images); i++) {
>>> +		if (!spl_params.images[i].file)
>>> +			break;
>>>    
>>> -	/* Boot file is optional, and only for back-to-bootrom functionality. */
>>> -	if (spl_params.boot_file) {
>>> -		size = rkcommon_get_aligned_filesize(params, spl_params.boot_file);
>>> +		size = rkcommon_get_aligned_filesize(params,
>>> +						     spl_params.images[i].file);
>>>    		if (size < 0)
>>>    			return EXIT_FAILURE;
>>> -		spl_params.boot_size = size;
>>> +		spl_params.images[i].size = size;
>>>    	}
>>>    
>>
>> Can't we merge the two for-loops?
> 
> Possible, suspect I kept it as two loops to avoid having to work on [i]
> and [i - 1] too much in same loop. Do you have any suggestion on how to
> merge the two for-loops to simplify this?
> 

Something like:

"""
	for (i = 0; i < ARRAY_SIZE(spl_params.images); i++) {
		int size;

		if (i == 0)
			spl_params.images[i].file = params->datafile;
		else
			spl_params.images[i].file =
					strchr(spl_params.images[i - 1].file, ':');

		if (!spl_params.images[i].file)
			break;

		*spl_params.images[i].file = '\0';
		spl_params.images[i].file += 1;

		size = rkcommon_get_aligned_filesize(params,
						     spl_params.images[i].file);
		if (size < 0)
			return EXIT_FAILURE;

		spl_params.images[i].size = size;
	}
"""

maybe?

**NOT TESTED**

Cheers,
Quentin

[PATCH 6/6] rockchip: mkimage: Add option for image load address and flag

[Jonas Karlman]

The v2 image format supports defining a load address and flag for each
embedded image.

Add initial support for writing the image load address and flag to the
v2 image format header.

This may later be used for RK3576 to embed a minimal initial image that
if required to fix booting from SD-card due to a BootROM issue.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/rkcommon.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/rkcommon.c b/tools/rkcommon.c
index 4ff48e81a636..952b76297dd9 100644
--- a/tools/rkcommon.c
+++ b/tools/rkcommon.c
@@ -150,12 +150,16 @@ static struct spl_info spl_infos[] = {
  *
  * @file:	image file path
  * @size:	aligned size of image in bytes
+ * @address:	image load address
+ * @flag:	no use
  */
 
 struct spl_params {
 	struct {
 		char *file;
 		uint32_t size;
+		uint32_t address;
+		uint32_t flag;
 	} images[4];
 };
 
@@ -366,7 +370,8 @@ static void rkcommon_set_header0_v2(void *buf, struct image_tool_params *params)
 		image_sector_count = spl_params.images[i].size / RK_BLK_SIZE;
 		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].address = spl_params.images[i].address ?: 0xFFFFFFFF;
+		hdr->images[i].flag = spl_params.images[i].flag;
 		hdr->images[i].counter = cpu_to_le32(i + 1);
 		image_ptr = buf + sector_offset * RK_BLK_SIZE;
 		do_sha256_hash(image_ptr, spl_params.images[i].size,
-- 
2.48.1

Re: [PATCH 6/6] rockchip: mkimage: Add option for image load address and flag

[Quentin Schulz]

Hi Jonas,

On 1/29/25 11:36 PM, Jonas Karlman wrote:
> The v2 image format supports defining a load address and flag for each
> embedded image.
> 
> Add initial support for writing the image load address and flag to the
> v2 image format header.
> 
> This may later be used for RK3576 to embed a minimal initial image that
> if required to fix booting from SD-card due to a BootROM issue.
> 

Would have been better with RK3576 support so we can see how it will be 
used. Especially, the flag member is very obscure. If we do nothing with 
it and document it as "no use", should we really add code for it?

The change itself seems fine though.

Cheers,
Quentin

Re: [PATCH 6/6] rockchip: mkimage: Add option for image load address and flag

[Jonas Karlman]

Hi Quentin,

On 2025-02-05 17:51, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 1/29/25 11:36 PM, Jonas Karlman wrote:
>> The v2 image format supports defining a load address and flag for each
>> embedded image.
>>
>> Add initial support for writing the image load address and flag to the
>> v2 image format header.
>>
>> This may later be used for RK3576 to embed a minimal initial image that
>> if required to fix booting from SD-card due to a BootROM issue.
>>
> 
> Would have been better with RK3576 support so we can see how it will be 
> used. Especially, the flag member is very obscure. If we do nothing with 
> it and document it as "no use", should we really add code for it?

I fully agree that this patch should possible be dropped from this
series and instead be included in a future rk3576 sd-card workaround
series.

I can only find FLAG=0x10007 for RV1106 in rkbin/RKBOOT, i.e. "no use"
in current state for mainline. However, a few SoCs seem to have use for
a LOAD_ADDR= different from the BootROM default.

Below is what I am playing with. I am not happy with current state and
would instead like to embed the binary code in some way, similar to [1].
See my rk3576-2025.04-wip branch at [2] for the full commit.

[1] https://patchwork.ozlabs.org/project/uboot/patch/20250103215904.2590769-3-jonas@kwiboo.se/
[2] https://github.com/Kwiboo/u-boot-rockchip/commits/rk3576-2025.04-wip/

commit e431562260a6313f765dbea9ed4f696fa97c5abc
Author: Jonas Karlman <jonas@kwiboo.se>
Date:   Tue Jan 28 01:30:12 2025 +0000

    WIP: rockchip: mkimage: Add rk3576 align and sd-card workaround
    
    The BootROM on RK3576 has an issue loading boot images from an SD-card.
    This issue can be worked around by injecting an initial boot image
    before TPL that:
    
      writel(0x3ffff800, 0x3ff803b0)
    
    Prepend an image containing binary code that does this and return to
    BootROM to load next image, TPL.
    
    TODO: embed the binary code into rkcommon.c
    
    Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

diff --git a/tools/rkcommon.c b/tools/rkcommon.c
index 8b57ba69cde6..7125b1de9fe9 100644
--- a/tools/rkcommon.c
+++ b/tools/rkcommon.c
@@ -143,7 +143,7 @@ static struct spl_info spl_infos[] = {
 	{ "rv1126", "110B", 0x10000 - 0x1000, false, RK_HEADER_V1 },
 	{ "rk3528", "RK35", 0x10000 - 0x1000, false, RK_HEADER_V2 },
 	{ "rk3568", "RK35", 0x10000 - 0x1000, false, RK_HEADER_V2 },
-	{ "rk3576", "RK35", 0x80000 - 0x1000, false, RK_HEADER_V2 },
+	{ "rk3576", "RK35", 0x80000 - 0x1000, false, RK_HEADER_V2, 8 },
 	{ "rk3588", "RK35", 0x100000 - 0x1000, false, RK_HEADER_V2 },
 };
 
@@ -271,6 +271,22 @@ int rkcommon_check_params(struct image_tool_params *params)
 		return EXIT_FAILURE;
 	}
 
+	if (!strcmp(params->imagename, "rk3576")) {
+		size = rkcommon_get_aligned_filesize(params, "rk3576-boost.bin");
+		if (size < 0)
+			return EXIT_SUCCESS;
+
+		for (i = ARRAY_SIZE(spl_params.images) - 1; i > 0; i--) {
+			spl_params.images[i] = spl_params.images[i - 1];
+		}
+
+		spl_params.images[0].file = "rk3576-boost.bin";
+		spl_params.images[0].size = size;
+
+		spl_params.images[0].address = 0x3ffc0000;
+		spl_params.images[1].address = 0x3ff81000;
+	}
+
 	return EXIT_SUCCESS;
 
 err_spl_info:

Regards,
Jonas

> 
> The change itself seems fine though.
> 
> Cheers,
> Quentin

Re: [PATCH 6/6] rockchip: mkimage: Add option for image load address and flag

[Quentin Schulz]

Hi Jonas,

On 2/5/25 8:54 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2025-02-05 17:51, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 1/29/25 11:36 PM, Jonas Karlman wrote:
>>> The v2 image format supports defining a load address and flag for each
>>> embedded image.
>>>
>>> Add initial support for writing the image load address and flag to the
>>> v2 image format header.
>>>
>>> This may later be used for RK3576 to embed a minimal initial image that
>>> if required to fix booting from SD-card due to a BootROM issue.
>>>
>>
>> Would have been better with RK3576 support so we can see how it will be
>> used. Especially, the flag member is very obscure. If we do nothing with
>> it and document it as "no use", should we really add code for it?
> 
> I fully agree that this patch should possible be dropped from this
> series and instead be included in a future rk3576 sd-card workaround
> series.
> 
> I can only find FLAG=0x10007 for RV1106 in rkbin/RKBOOT, i.e. "no use"
> in current state for mainline. However, a few SoCs seem to have use for
> a LOAD_ADDR= different from the BootROM default.
> 
> Below is what I am playing with. I am not happy with current state and
> would instead like to embed the binary code in some way, similar to [1].
> See my rk3576-2025.04-wip branch at [2] for the full commit.
> 
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20250103215904.2590769-3-jonas@kwiboo.se/
> [2] https://github.com/Kwiboo/u-boot-rockchip/commits/rk3576-2025.04-wip/
> 
> commit e431562260a6313f765dbea9ed4f696fa97c5abc
> Author: Jonas Karlman <jonas@kwiboo.se>
> Date:   Tue Jan 28 01:30:12 2025 +0000
> 
>      WIP: rockchip: mkimage: Add rk3576 align and sd-card workaround
>      
>      The BootROM on RK3576 has an issue loading boot images from an SD-card.
>      This issue can be worked around by injecting an initial boot image
>      before TPL that:
>      
>        writel(0x3ffff800, 0x3ff803b0)
>      

Extrapolating here, but I guess it works good enough to load this small 
boost.bin but not enough for the full TPL (which is the DRAM init blob 
from Rockchip) and thus adding boost.bin before Rockchip's DRAM init 
blob makes it the whole thing able to boot from SD?

Shouldn't that rather be fixed by Rockchip in their blob? Ideally we 
shouldn't need this trick if and once we get an open-source DRAM init.

>      Prepend an image containing binary code that does this and return to
>      BootROM to load next image, TPL.
>      
>      TODO: embed the binary code into rkcommon.c
>      
>      Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> 
> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
> index 8b57ba69cde6..7125b1de9fe9 100644
> --- a/tools/rkcommon.c
> +++ b/tools/rkcommon.c
> @@ -143,7 +143,7 @@ static struct spl_info spl_infos[] = {
>   	{ "rv1126", "110B", 0x10000 - 0x1000, false, RK_HEADER_V1 },
>   	{ "rk3528", "RK35", 0x10000 - 0x1000, false, RK_HEADER_V2 },
>   	{ "rk3568", "RK35", 0x10000 - 0x1000, false, RK_HEADER_V2 },
> -	{ "rk3576", "RK35", 0x80000 - 0x1000, false, RK_HEADER_V2 },
> +	{ "rk3576", "RK35", 0x80000 - 0x1000, false, RK_HEADER_V2, 8 },
>   	{ "rk3588", "RK35", 0x100000 - 0x1000, false, RK_HEADER_V2 },
>   };
>   
> @@ -271,6 +271,22 @@ int rkcommon_check_params(struct image_tool_params *params)
>   		return EXIT_FAILURE;
>   	}
>   
> +	if (!strcmp(params->imagename, "rk3576")) {
> +		size = rkcommon_get_aligned_filesize(params, "rk3576-boost.bin");
> +		if (size < 0)
> +			return EXIT_SUCCESS;
> +
> +		for (i = ARRAY_SIZE(spl_params.images) - 1; i > 0; i--) {
> +			spl_params.images[i] = spl_params.images[i - 1];
> +		}
> +
> +		spl_params.images[0].file = "rk3576-boost.bin";
> +		spl_params.images[0].size = size;
> +
> +		spl_params.images[0].address = 0x3ffc0000;
> +		spl_params.images[1].address = 0x3ff81000;
> +	}
> +

Can't we add a new node to the rk3576-u-boot.dtsi in 
/binman/simple-bin/mkimage which is before rockchip-tpl which generates 
this new binary?

Cheers,
Quentin

Re: [PATCH 0/6] rockchip: mkimage: Improve support for v2 image format

[Kever Yang]

Hi Jonas,

     Do you have a new version for this patch set?

     The main change in this patch set looks good to me and it needs for 
rk3576 which has land for next release.


Thanks,

- Kever

On 2025/1/30 06:36, Jonas Karlman wrote:
> The Rockchip idblock v2 image format header embeds much more information
> than the older format. E.g. it can embed up to 4 images that the BootROM
> can load, image load address, flag, checksum and more.
>
> This series improves mkimage with support for some of these features of
> the v2 image format. Features that are likely to be needed to have
> working SD-card boot on RK3576.
>
> Jonas Karlman (6):
>    rockchip: mkimage: Split size_and_off and size_and_nimage
>    rockchip: mkimage: Print image information for all embedded images
>    rockchip: mkimage: Print boot0 and boot1 parameters
>    rockchip: mkimage: Add option to change image offset alignment
>    rockchip: mkimage: Add support for up to 4 input files
>    rockchip: mkimage: Add option for image load address and flag
>
>   tools/rkcommon.c | 268 +++++++++++++++++++++++++++++------------------
>   tools/rkcommon.h |   2 -
>   2 files changed, 166 insertions(+), 104 deletions(-)
>