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

[Quentin Schulz <quentin.schulz@cherry.de>]

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