[PATCH v4 08/10] rockchip: binman: Use the skip-at-start prop in simple-bin image

[Jonas Karlman]

Hi Quentin,

On 2025-04-09 12:57, Quentin Schulz wrote:
> Hi Jonas, Simon,
> 
> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>> From: Simon Glass <sjg at chromium.org>
>>
>> The simple-bin image is normally written to MMC media at block 64, which
>> is a 32K offset from start of storage media.
>>
>> Set the skip-at-start property to 0x8000 (32 KiB) so that fdtmap and
>> other embedded binman symbols in the output binary is referencing image
>> offsets correctly.
>>
> 
> Shouldn't we have this commit BEFORE we add the `fdtmap` node since we 
> know it's wrong before this commit?

Sure, this series could/should probably be reordered into, patches with:
- fixes
- no functional change
- functional change
- new features

My understanding is that the binman symbols is not really used by
Rockchip in current configuration, yet symbols is embedded into the
binary utput and possible also with the fdtmap. So the order has no
practical change/diff.

Will reorder patches in a v5.

> 
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>> ---
>> Changes in v4:
>> - Drop defconfig changes
>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>>
>> Changes in v2:
>> - Move this patch to the end of the series
>> - Drop 0x8000 offset for SPI
>> ---
>>   arch/arm/dts/rockchip-u-boot.dtsi | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
>> index fb38b7b80c43..65b81bf58626 100644
>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
>> @@ -154,6 +154,7 @@
>>   	simple-bin {
>>   		filename = "u-boot-rockchip.bin";
>>   		pad-byte = <0xff>;
>> +		skip-at-start = <0x8000>;
>>   
>>   		mkimage {
>>   			filename = "idbloader.img";
>> @@ -178,7 +179,7 @@
>>   #else
>>   		u-boot-img {
>>   #endif
>> -			offset = <CONFIG_SPL_PAD_TO>;
>> +			offset = <(CONFIG_SPL_PAD_TO + 0x8000)>;
> 
> This is confusing. The documentation states:
> 
> """
> offset:
>      This sets the offset of an entry within the image or section containing
>      it.
> """
> 
> My understanding is that it should be relative to the beginning of the 
> image but this now needs the knowledge of where it will be stored on the 
> MMC device (via the value in skip-at-start).

This was also my understanding but Simon has a different view and for
the simple-bin image that is mainly targeted MMC storage this could
possible be okay.

If there is any slight objection of this I would rather just drop this
patch for now and it can be part of Simon's next VBE part series.

Regards,
Jonas

> 
> Why is skip-at-start automatically deducted from offset?
> 
> Cheers,
> Quentin