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