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

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

Hi Simon,

On 4/9/25 4:30 PM, Simon Glass wrote:
> Hi Quentin,
> 
> On Wed, 9 Apr 2025 at 07:35, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Simon,
>>
>> On 4/9/25 3:33 PM, Simon Glass wrote:
>>> Hi Quentin,
>>>
>>> On Wed, 9 Apr 2025 at 07:32, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 4/9/25 3:22 PM, Simon Glass wrote:
>>>>> Hi Quentin,
>>>>>
>>>>> On Wed, 9 Apr 2025 at 04:57, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>>>>
>>>>>> Hi Jonas, Simon,
>>>>>>
>>>>>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>>>>>>> From: Simon Glass <sjg@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?
>>>>>>
>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>> Signed-off-by: Jonas Karlman <jonas@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).
>>>>>>
>>>>>> Why is skip-at-start automatically deducted from offset?
>>>>>
>>>>> This is how binman works[1]. We are trying to use the feature designs
>>>>
>>>> Why is it deducted?
>>>
>>> Are you asking why skip-at-start is deducted from the offset?
>>>
>>
>> Yes
> 
> It is confusing, unfortunately.
> 
> When you use an offset (say 0x78000) then normally the entry will
> start at that offset in the image.
> 
> When you use skip-at-start 0x8000, its value is added to all top-level
> offsets in the image, so the offset becomes 0x80000
> 
> BUT the image built by binman does not contain the first 0x8000 bytes.
> It is expected that the image is written to offset 0x8000 so that the
> offsets will be correct when used within U-Boot itself.
> 

I would assume all offsets are relative to the beginning of the image in 
the binary? Adding skip-at-start doesn't add 0x8000 bytes at the 
beginning of the binary file, so why would the offsets need to be modified?

Also, while 0x8000 is the typical address the image can be flashed, it 
is not necessarily where it will be as the BootROM tries a few other 
offsets if it cannot find one at 32KiB offset in the storage medium. 
This seems to me to be a case of "somewhat helps in one case, but makes 
things more confusing in others"? Will we need different offsets 
depending on where the FIT is flashed? What happens for A/B updates then?

I understand that we currently essentially have skip-at-start = 0; and 
that it is bad because it doesn't reflect the actual address, but how is 
that worse than hardcoding a different offset?

Cheers,
Quentin