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

Hi Alexey,

On 7/13/26 12:02 PM, Alexey Charkov wrote:
> From: Jonas Karlman <[email protected]>
> 
> 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.
> 

So this is lacking some information. Reading this commit log I thought 
we just implement this "because we can", which I really don't like, but 
we actually make use of this in patch 7 (without justification).

> Signed-off-by: Jonas Karlman <[email protected]>
> Signed-off-by: Alexey Charkov <[email protected]>
> ---
>   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 992c90791ffc..b413cd3f7e49 100644
> --- a/tools/rkcommon.c
> +++ b/tools/rkcommon.c
> @@ -140,6 +140,7 @@ struct spl_info {
>   	const uint32_t spl_size;
>   	const bool spl_rc4;
>   	const uint32_t header_ver;
> +	const uint32_t align;

Please add documentation to the structure so we know what this stores, 
in which unit, etc...

>   };
>   
>   static struct spl_info spl_infos[] = {
> @@ -200,14 +201,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)

Do the rename from rkcommon_get_aligned_size to 
rkcommon_get_aligned_filesize in a separate patch, it should make this 
diff here easier to review then.

> +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);

Please always set align to a valid value so we don't have to do this 
check. The default seems to be 4 (RK_SIZE_ALIGN/RK_BLK_SIZE) so set it 
for all currently supported SoC to that value.

>   
>   	/*
>   	 * Pad to a 2KB alignment, as required for init/boot size by the ROM
> @@ -216,6 +222,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)
> +{

Can we be consistent here and rename this to 
rkcommon_get_aligned_headersize (you can add an underscore between 
header and size, I just would like "aligned_" to be part of the function 
name to match rkcommon_get_aligned_filesize and user expectations).

Cheers,
Quentin