[Question] TI's u-boot.img is built twice

[Question] TI's u-boot.img is built twice

[Masahiro Yamada]

Hi.

Since the TI platform migrated to binman,
u-boot.img is built twice.

It is created by "mkimage -E",
then overwritten by binman.


So, the data are embedded in the FIT structure
instead of being appended.

Is this intentional?

To me, it looks weird.




To confirm it, apply the following hack.

Since u-boot.img is overwritten by binman,
copy it to u-boot.img.backup.




diff --git a/Makefile b/Makefile
index 87f9fc786e..4cffa8a061 100644
--- a/Makefile
+++ b/Makefile
@@ -1112,6 +1112,7 @@ endef
 # Timestamp file to make sure that binman always runs
 .binman_stamp: $(INPUTS-y) FORCE
 ifeq ($(CONFIG_BINMAN),y)
+       cp u-boot.img u-boot.img.backup
        $(call if_changed,binman)
 endif
        @touch $@



Then, build it for the main core.


make -j$(nproc) CROSS_COMPILE=aarch64-linux-gnu-
   am64x_evm_a53_defconfig all
   TEE=~/ref/OP-TEE/optee_os/out/arm-plat-k3/core/tee-raw.bin
   BL31=~/ref/trusted-firmware-a/build/k3/lite/release/bl31.bin
   BINMAN_INDIRS=~/ref/ti-linux-firmware




Compare the two files.
Run fdtdump to see what happened to them.


$ diff -u u-boot.img  u-boot.img.backup
Binary files u-boot.img and u-boot.img.backup differ


$ fdtdump u-boot.img
    => u-boot and dt are embedded.

$ fdtdump u-boot.img.backup
    => u-boot and dt are appended after the
       FIT structure



-- 
Best Regards
Masahiro Yamada

Re: [Question] TI's u-boot.img is built twice

[Neha Malcom Francis]

Hi Masahiro

On 21/09/23 21:06, Masahiro Yamada wrote:
> Hi.
> 
> Since the TI platform migrated to binman,
> u-boot.img is built twice.
> 
> It is created by "mkimage -E",
> then overwritten by binman.
> 
> 
> So, the data are embedded in the FIT structure
> instead of being appended.
> 
> Is this intentional?
> 
> To me, it looks weird.
> 
> 

I haven't added the fit,external-offset property in the binman.dtsi so it was 
not appended as external data and I did not find reason to. Is there any benefit 
in having the data appended than embedded?

> 
> 
> To confirm it, apply the following hack.
> 
> Since u-boot.img is overwritten by binman,
> copy it to u-boot.img.backup.
> 
> 
> 
> 
> diff --git a/Makefile b/Makefile
> index 87f9fc786e..4cffa8a061 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1112,6 +1112,7 @@ endef
>   # Timestamp file to make sure that binman always runs
>   .binman_stamp: $(INPUTS-y) FORCE
>   ifeq ($(CONFIG_BINMAN),y)
> +       cp u-boot.img u-boot.img.backup
>          $(call if_changed,binman)
>   endif
>          @touch $@
> 
> 
> 
> Then, build it for the main core.
> 
> 
> make -j$(nproc) CROSS_COMPILE=aarch64-linux-gnu-
>     am64x_evm_a53_defconfig all
>     TEE=~/ref/OP-TEE/optee_os/out/arm-plat-k3/core/tee-raw.bin
>     BL31=~/ref/trusted-firmware-a/build/k3/lite/release/bl31.bin
>     BINMAN_INDIRS=~/ref/ti-linux-firmware
> 
> 
> 
> 
> Compare the two files.
> Run fdtdump to see what happened to them.
> 
> 
> $ diff -u u-boot.img  u-boot.img.backup
> Binary files u-boot.img and u-boot.img.backup differ
> 
> 
> $ fdtdump u-boot.img
>      => u-boot and dt are embedded.
> 
> $ fdtdump u-boot.img.backup
>      => u-boot and dt are appended after the
>         FIT structure
> 
> 
> 

-- 
Thanking You
Neha Malcom Francis

Re: [Question] TI's u-boot.img is built twice

[Masahiro Yamada]

On Fri, Sep 22, 2023 at 2:27 PM Neha Malcom Francis <n-francis@ti.com> wrote:
>
> Hi Masahiro
>
> On 21/09/23 21:06, Masahiro Yamada wrote:
> > Hi.
> >
> > Since the TI platform migrated to binman,
> > u-boot.img is built twice.
> >
> > It is created by "mkimage -E",
> > then overwritten by binman.
> >
> >
> > So, the data are embedded in the FIT structure
> > instead of being appended.
> >
> > Is this intentional?
> >
> > To me, it looks weird.
> >
> >
>
> I haven't added the fit,external-offset property in the binman.dtsi so it was
> not appended as external data and I did not find reason to. Is there any benefit
> in having the data appended than embedded?



Placing payload data outside the FIT structure is a U-Boot hack.


The motivation was explained in the commit log of
722ebc8f84d5bccd2e70fad1079a0dd40cceddec



Before TI migrated to binman,
u-boot.img was the "payload outside" structure
but it is not any more.

I do not mind the implementation details as long as
U-Boot is able to boot the Linux kernel.


I was just suffering from the AM64-SK boot failure
(as I asked in another thread) and just noticed
something odd when I was poking the SPL code.


At least, .u-boot.img.cmd is not telling the truth.

Usually, the build command is saved in a *.cmd file
but this does not reflect the reality, because
it is binman that created the final u-boot.img


$ cat .u-boot.img.cmd
cmd_u-boot.img := ./tools/mkimage -f auto -A arm -T firmware -C none
-O u-boot -a 0x80800000 -e 0x80800000 -p 0x0 -n "U-Boot
2023.10-rc4-00047-gb9b83a86f0-dirty for am64x board" -E  -b
arch/arm/dts/k3-am642-evm.dtb -b arch/arm/dts/k3-am642-sk.dtb  -d
u-boot-nodtb.bin u-boot.img >/dev/null


I will not track it down any further, though.
It is too complicated.






> >
> >
> > To confirm it, apply the following hack.
> >
> > Since u-boot.img is overwritten by binman,
> > copy it to u-boot.img.backup.
> >
> >
> >
> >
> > diff --git a/Makefile b/Makefile
> > index 87f9fc786e..4cffa8a061 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1112,6 +1112,7 @@ endef
> >   # Timestamp file to make sure that binman always runs
> >   .binman_stamp: $(INPUTS-y) FORCE
> >   ifeq ($(CONFIG_BINMAN),y)
> > +       cp u-boot.img u-boot.img.backup
> >          $(call if_changed,binman)
> >   endif
> >          @touch $@
> >
> >
> >
> > Then, build it for the main core.
> >
> >
> > make -j$(nproc) CROSS_COMPILE=aarch64-linux-gnu-
> >     am64x_evm_a53_defconfig all
> >     TEE=~/ref/OP-TEE/optee_os/out/arm-plat-k3/core/tee-raw.bin
> >     BL31=~/ref/trusted-firmware-a/build/k3/lite/release/bl31.bin
> >     BINMAN_INDIRS=~/ref/ti-linux-firmware
> >
> >
> >
> >
> > Compare the two files.
> > Run fdtdump to see what happened to them.
> >
> >
> > $ diff -u u-boot.img  u-boot.img.backup
> > Binary files u-boot.img and u-boot.img.backup differ
> >
> >
> > $ fdtdump u-boot.img
> >      => u-boot and dt are embedded.
> >
> > $ fdtdump u-boot.img.backup
> >      => u-boot and dt are appended after the
> >         FIT structure
> >
> >
> >
>
> --
> Thanking You
> Neha Malcom Francis



-- 
Best Regards
Masahiro Yamada

Re: [Question] TI's u-boot.img is built twice

[Neha Malcom Francis]

Hi Masahiro

On 22/09/23 12:48, Masahiro Yamada wrote:
> On Fri, Sep 22, 2023 at 2:27 PM Neha Malcom Francis <n-francis@ti.com> wrote:
>>
>> Hi Masahiro
>>
>> On 21/09/23 21:06, Masahiro Yamada wrote:
>>> Hi.
>>>
>>> Since the TI platform migrated to binman,
>>> u-boot.img is built twice.
>>>
>>> It is created by "mkimage -E",
>>> then overwritten by binman.
>>>
>>>
>>> So, the data are embedded in the FIT structure
>>> instead of being appended.
>>>
>>> Is this intentional?
>>>
>>> To me, it looks weird.
>>>
>>>
>>
>> I haven't added the fit,external-offset property in the binman.dtsi so it was
>> not appended as external data and I did not find reason to. Is there any benefit
>> in having the data appended than embedded?
> 
> 
> 
> Placing payload data outside the FIT structure is a U-Boot hack.
> 
> 
> The motivation was explained in the commit log of
> 722ebc8f84d5bccd2e70fad1079a0dd40cceddec
> 
> 

Thanks for this! Makes sense, I think we should make it appended again in 
binman. Can reduce boot time.

> 
> Before TI migrated to binman,
> u-boot.img was the "payload outside" structure
> but it is not any more.
> 
> I do not mind the implementation details as long as
> U-Boot is able to boot the Linux kernel.
> 
> 
> I was just suffering from the AM64-SK boot failure
> (as I asked in another thread) and just noticed
> something odd when I was poking the SPL code.
> 
> 
> At least, .u-boot.img.cmd is not telling the truth.
> 
> Usually, the build command is saved in a *.cmd file
> but this does not reflect the reality, because
> it is binman that created the final u-boot.img
> 
> 
> $ cat .u-boot.img.cmd
> cmd_u-boot.img := ./tools/mkimage -f auto -A arm -T firmware -C none
> -O u-boot -a 0x80800000 -e 0x80800000 -p 0x0 -n "U-Boot
> 2023.10-rc4-00047-gb9b83a86f0-dirty for am64x board" -E  -b
> arch/arm/dts/k3-am642-evm.dtb -b arch/arm/dts/k3-am642-sk.dtb  -d
> u-boot-nodtb.bin u-boot.img >/dev/null
> 
> 
> I will not track it down any further, though.
> It is too complicated.
> 
> 
> 

I am not too sure about the .cmd files but looks like its misleading probably 
because the replacing of the binman generated image takes under cmd_binman and 
not directly from the Makefile (just a guess).
> 
> 
>>>
>>>
>>> To confirm it, apply the following hack.
>>>
>>> Since u-boot.img is overwritten by binman,
>>> copy it to u-boot.img.backup.
>>>
>>>
>>>
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 87f9fc786e..4cffa8a061 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -1112,6 +1112,7 @@ endef
>>>    # Timestamp file to make sure that binman always runs
>>>    .binman_stamp: $(INPUTS-y) FORCE
>>>    ifeq ($(CONFIG_BINMAN),y)
>>> +       cp u-boot.img u-boot.img.backup
>>>           $(call if_changed,binman)
>>>    endif
>>>           @touch $@
>>>
>>>
>>>
>>> Then, build it for the main core.
>>>
>>>
>>> make -j$(nproc) CROSS_COMPILE=aarch64-linux-gnu-
>>>      am64x_evm_a53_defconfig all
>>>      TEE=~/ref/OP-TEE/optee_os/out/arm-plat-k3/core/tee-raw.bin
>>>      BL31=~/ref/trusted-firmware-a/build/k3/lite/release/bl31.bin
>>>      BINMAN_INDIRS=~/ref/ti-linux-firmware
>>>
>>>
>>>
>>>
>>> Compare the two files.
>>> Run fdtdump to see what happened to them.
>>>
>>>
>>> $ diff -u u-boot.img  u-boot.img.backup
>>> Binary files u-boot.img and u-boot.img.backup differ
>>>
>>>
>>> $ fdtdump u-boot.img
>>>       => u-boot and dt are embedded.
>>>
>>> $ fdtdump u-boot.img.backup
>>>       => u-boot and dt are appended after the
>>>          FIT structure
>>>
>>>
>>>
>>
>> --
>> Thanking You
>> Neha Malcom Francis
> 
> 
> 

-- 
Thanking You
Neha Malcom Francis

Re: [Question] TI's u-boot.img is built twice

[Masahiro Yamada]

On Fri, Sep 22, 2023 at 5:04 PM Neha Malcom Francis <n-francis@ti.com> wrote:
>
> Hi Masahiro
>
> On 22/09/23 12:48, Masahiro Yamada wrote:
> > On Fri, Sep 22, 2023 at 2:27 PM Neha Malcom Francis <n-francis@ti.com> wrote:
> >>
> >> Hi Masahiro
> >>
> >> On 21/09/23 21:06, Masahiro Yamada wrote:
> >>> Hi.
> >>>
> >>> Since the TI platform migrated to binman,
> >>> u-boot.img is built twice.
> >>>
> >>> It is created by "mkimage -E",
> >>> then overwritten by binman.
> >>>
> >>>
> >>> So, the data are embedded in the FIT structure
> >>> instead of being appended.
> >>>
> >>> Is this intentional?
> >>>
> >>> To me, it looks weird.
> >>>
> >>>
> >>
> >> I haven't added the fit,external-offset property in the binman.dtsi so it was
> >> not appended as external data and I did not find reason to. Is there any benefit
> >> in having the data appended than embedded?
> >
> >
> >
> > Placing payload data outside the FIT structure is a U-Boot hack.
> >
> >
> > The motivation was explained in the commit log of
> > 722ebc8f84d5bccd2e70fad1079a0dd40cceddec
> >
> >
>
> Thanks for this! Makes sense, I think we should make it appended again in
> binman. Can reduce boot time.
> >
> > Before TI migrated to binman,
> > u-boot.img was the "payload outside" structure
> > but it is not any more.
> >
> > I do not mind the implementation details as long as
> > U-Boot is able to boot the Linux kernel.
> >
> >
> > I was just suffering from the AM64-SK boot failure
> > (as I asked in another thread) and just noticed
> > something odd when I was poking the SPL code.
> >
> >
> > At least, .u-boot.img.cmd is not telling the truth.
> >
> > Usually, the build command is saved in a *.cmd file
> > but this does not reflect the reality, because
> > it is binman that created the final u-boot.img
> >
> >
> > $ cat .u-boot.img.cmd
> > cmd_u-boot.img := ./tools/mkimage -f auto -A arm -T firmware -C none
> > -O u-boot -a 0x80800000 -e 0x80800000 -p 0x0 -n "U-Boot
> > 2023.10-rc4-00047-gb9b83a86f0-dirty for am64x board" -E  -b
> > arch/arm/dts/k3-am642-evm.dtb -b arch/arm/dts/k3-am642-sk.dtb  -d
> > u-boot-nodtb.bin u-boot.img >/dev/null
> >
> >
> > I will not track it down any further, though.
> > It is too complicated.
> >
> >
> >
>
> I am not too sure about the .cmd files but looks like its misleading probably
> because the replacing of the binman generated image takes under cmd_binman and
> not directly from the Makefile (just a guess).



From my view as a user, building images without k3-image-gen
seems like a benefit, but binman is not mandatory for u-boot.img.

With line 255-339 of arch/arm/dts/k3-am64x-binman.dtsi deleted,
u-boot.img was still generated in the same structure as before.
(payload appended)


The confusion came from u-boot.img being first created by
line 1439 of the top Makefile  (mkimage),
then overwritten by line 1113 (binman).

If you use binman for u-boot.img, the line 1439 does not need to run,
but it runs because u-boot.img is added to INPUTS-y.


Perhaps, you may want to hack line 978 because
IMX is doing something there.


ifeq ($(CONFIG_MX7)$(CONFIG_IMX_HAB), yy)
INPUTS-$(CONFIG_SPL_FRAMEWORK) += u-boot-ivt.img
else
INPUTS-$(CONFIG_SPL_FRAMEWORK) += u-boot.img
endif





For the *.cmd files, the mkimage command is
saved in .u-boot.img.cmd and the binman command
is saved in ..binman_stamp.cmd




$ cat  .u-boot.img.cmd
cmd_u-boot.img := ./tools/mkimage -f auto -A arm -T firmware -C none
-O u-boot -a 0x80800000 -e 0x80800000 -p 0x0 -n "U-Boot
2023.10-rc4-00047-gb9b83a86f0-dirty for am64x board" -E  -b
arch/arm/dts/k3-am642-evm.dtb -b arch/arm/dts/k3-am642-sk.dtb  -d
u-boot-nodtb.bin u-boot.img >/dev/null

$ cat ..binman_stamp.cmd
cmd_.binman_stamp := ./tools/binman/binman   --toolpath ./tools  build
-u -d u-boot.dtb -O . -m --allow-missing  -I . -I . -I
./board/ti/am64x -I arch/arm/dts -a of-list="k3-am642-evm k3-am642-sk"
-I /home/masahiro/ref/ti-linux-firmware -a
atf-bl31-path=/home/masahiro/ref/trusted-firmware-a/build/k3/lite/release/bl31.bin
-a tee-os-path=/home/masahiro/ref/OP-TEE/optee_os/out/arm-plat-k3/core/tee-raw.bin
-a opensbi-path= -a default-dt="k3-am642-evm" -a scp-path= -a
rockchip-tpl-path= -a spl-bss-pad= -a tpl-bss-pad=1 -a spl-dtb=y -a
tpl-dtb= -a pre-load-key-path=






--
Best Regards
Masahiro Yamada

Re: [Question] TI's u-boot.img is built twice

[Simon Glass]

Hi Masahiro,

On Thu, 21 Sept 2023 at 09:36, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Hi.
>
> Since the TI platform migrated to binman,
> u-boot.img is built twice.
>
> It is created by "mkimage -E",
> then overwritten by binman.
>
>
> So, the data are embedded in the FIT structure
> instead of being appended.
>
> Is this intentional?
>
> To me, it looks weird.
>
>
>
>
> To confirm it, apply the following hack.
>
> Since u-boot.img is overwritten by binman,
> copy it to u-boot.img.backup.
>
>
>
>
> diff --git a/Makefile b/Makefile
> index 87f9fc786e..4cffa8a061 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1112,6 +1112,7 @@ endef
>  # Timestamp file to make sure that binman always runs
>  .binman_stamp: $(INPUTS-y) FORCE
>  ifeq ($(CONFIG_BINMAN),y)
> +       cp u-boot.img u-boot.img.backup
>         $(call if_changed,binman)
>  endif
>         @touch $@
>
>
>
> Then, build it for the main core.
>
>
> make -j$(nproc) CROSS_COMPILE=aarch64-linux-gnu-
>    am64x_evm_a53_defconfig all
>    TEE=~/ref/OP-TEE/optee_os/out/arm-plat-k3/core/tee-raw.bin
>    BL31=~/ref/trusted-firmware-a/build/k3/lite/release/bl31.bin
>    BINMAN_INDIRS=~/ref/ti-linux-firmware
>
>
>
>
> Compare the two files.
> Run fdtdump to see what happened to them.
>
>
> $ diff -u u-boot.img  u-boot.img.backup
> Binary files u-boot.img and u-boot.img.backup differ
>
>
> $ fdtdump u-boot.img
>     => u-boot and dt are embedded.
>
> $ fdtdump u-boot.img.backup
>     => u-boot and dt are appended after the
>        FIT structure

That seems like a bug to me...at some point we might consider building
u-boot.img with Binman, but for now it is built by the Makefile.

>
>
>
> --
> Best Regards
> Masahiro Yamada

Regards,
Simon

Re: [Question] TI's u-boot.img is built twice

[Simon Glass]

Hi Masahiro,

On Fri, 22 Sept 2023 at 01:19, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Fri, Sep 22, 2023 at 2:27 PM Neha Malcom Francis <n-francis@ti.com> wrote:
> >
> > Hi Masahiro
> >
> > On 21/09/23 21:06, Masahiro Yamada wrote:
> > > Hi.
> > >
> > > Since the TI platform migrated to binman,
> > > u-boot.img is built twice.
> > >
> > > It is created by "mkimage -E",
> > > then overwritten by binman.
> > >
> > >
> > > So, the data are embedded in the FIT structure
> > > instead of being appended.
> > >
> > > Is this intentional?
> > >
> > > To me, it looks weird.
> > >
> > >
> >
> > I haven't added the fit,external-offset property in the binman.dtsi so it was
> > not appended as external data and I did not find reason to. Is there any benefit
> > in having the data appended than embedded?
>
>
>
> Placing payload data outside the FIT structure is a U-Boot hack.

No, it isn't a hack. It is part of the specification[1].

[..]

Regards,
Simon

[1] https://github.com/open-source-firmware/flat-image-tree/releases/tag/v0.8

Re: [Question] TI's u-boot.img is built twice

[Andrew Davis]

On 9/22/23 10:01 AM, Simon Glass wrote:
> Hi Masahiro,
> 
> On Thu, 21 Sept 2023 at 09:36, Masahiro Yamada <masahiroy@kernel.org> wrote:
>>
>> Hi.
>>
>> Since the TI platform migrated to binman,
>> u-boot.img is built twice.
>>
>> It is created by "mkimage -E",
>> then overwritten by binman.
>>
>>
>> So, the data are embedded in the FIT structure
>> instead of being appended.
>>
>> Is this intentional?
>>
>> To me, it looks weird.
>>
>>
>>
>>
>> To confirm it, apply the following hack.
>>
>> Since u-boot.img is overwritten by binman,
>> copy it to u-boot.img.backup.
>>
>>
>>
>>
>> diff --git a/Makefile b/Makefile
>> index 87f9fc786e..4cffa8a061 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1112,6 +1112,7 @@ endef
>>   # Timestamp file to make sure that binman always runs
>>   .binman_stamp: $(INPUTS-y) FORCE
>>   ifeq ($(CONFIG_BINMAN),y)
>> +       cp u-boot.img u-boot.img.backup
>>          $(call if_changed,binman)
>>   endif
>>          @touch $@
>>
>>
>>
>> Then, build it for the main core.
>>
>>
>> make -j$(nproc) CROSS_COMPILE=aarch64-linux-gnu-
>>     am64x_evm_a53_defconfig all
>>     TEE=~/ref/OP-TEE/optee_os/out/arm-plat-k3/core/tee-raw.bin
>>     BL31=~/ref/trusted-firmware-a/build/k3/lite/release/bl31.bin
>>     BINMAN_INDIRS=~/ref/ti-linux-firmware
>>
>>
>>
>>
>> Compare the two files.
>> Run fdtdump to see what happened to them.
>>
>>
>> $ diff -u u-boot.img  u-boot.img.backup
>> Binary files u-boot.img and u-boot.img.backup differ
>>
>>
>> $ fdtdump u-boot.img
>>      => u-boot and dt are embedded.
>>
>> $ fdtdump u-boot.img.backup
>>      => u-boot and dt are appended after the
>>         FIT structure
> 
> That seems like a bug to me...at some point we might consider building
> u-boot.img with Binman, but for now it is built by the Makefile.
> 

This is not true for K3 as we now use Binman for u-boot.img generation
(we need this as we have a signing step injected at this point).

So as Masahiro suggested in the other branch of this thread, we need
to disable the Makefile generation of u-boot.img for K3.

Neha,

Can you take this action? I assume you can add it as part of your
work in making the data external again, which would make the u-boot.img
identical to before and hence Makefile version could be disabled at
the same time/series.

Andrew

>>
>>
>>
>> --
>> Best Regards
>> Masahiro Yamada
> 
> Regards,
> Simon

Re: [Question] TI's u-boot.img is built twice

[Simon Glass]

Hi Andrew,

On Fri, 22 Sept 2023 at 10:35, Andrew Davis <afd@ti.com> wrote:
>
> On 9/22/23 10:01 AM, Simon Glass wrote:
> > Hi Masahiro,
> >
> > On Thu, 21 Sept 2023 at 09:36, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >>
> >> Hi.
> >>
> >> Since the TI platform migrated to binman,
> >> u-boot.img is built twice.
> >>
> >> It is created by "mkimage -E",
> >> then overwritten by binman.
> >>
> >>
> >> So, the data are embedded in the FIT structure
> >> instead of being appended.
> >>
> >> Is this intentional?
> >>
> >> To me, it looks weird.
> >>
> >>
> >>
> >>
> >> To confirm it, apply the following hack.
> >>
> >> Since u-boot.img is overwritten by binman,
> >> copy it to u-boot.img.backup.
> >>
> >>
> >>
> >>
> >> diff --git a/Makefile b/Makefile
> >> index 87f9fc786e..4cffa8a061 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -1112,6 +1112,7 @@ endef
> >>   # Timestamp file to make sure that binman always runs
> >>   .binman_stamp: $(INPUTS-y) FORCE
> >>   ifeq ($(CONFIG_BINMAN),y)
> >> +       cp u-boot.img u-boot.img.backup
> >>          $(call if_changed,binman)
> >>   endif
> >>          @touch $@
> >>
> >>
> >>
> >> Then, build it for the main core.
> >>
> >>
> >> make -j$(nproc) CROSS_COMPILE=aarch64-linux-gnu-
> >>     am64x_evm_a53_defconfig all
> >>     TEE=~/ref/OP-TEE/optee_os/out/arm-plat-k3/core/tee-raw.bin
> >>     BL31=~/ref/trusted-firmware-a/build/k3/lite/release/bl31.bin
> >>     BINMAN_INDIRS=~/ref/ti-linux-firmware
> >>
> >>
> >>
> >>
> >> Compare the two files.
> >> Run fdtdump to see what happened to them.
> >>
> >>
> >> $ diff -u u-boot.img  u-boot.img.backup
> >> Binary files u-boot.img and u-boot.img.backup differ
> >>
> >>
> >> $ fdtdump u-boot.img
> >>      => u-boot and dt are embedded.
> >>
> >> $ fdtdump u-boot.img.backup
> >>      => u-boot and dt are appended after the
> >>         FIT structure
> >
> > That seems like a bug to me...at some point we might consider building
> > u-boot.img with Binman, but for now it is built by the Makefile.
> >
>
> This is not true for K3 as we now use Binman for u-boot.img generation
> (we need this as we have a signing step injected at this point).

No, no.

You must not mess with the outputs of Makefile - create a new file
with what you need, e.g. u-boot-ti.img

While we could use Binman to produce the .img we are quite a way from
doing that as not that many boards have been converted to binman.

>
> So as Masahiro suggested in the other branch of this thread, we need
> to disable the Makefile generation of u-boot.img for K3.
>
> Neha,
>
> Can you take this action? I assume you can add it as part of your
> work in making the data external again, which would make the u-boot.img
> identical to before and hence Makefile version could be disabled at
> the same time/series.

If you are suggesting that we slowly migrate boards away from
generating the .img using Makefile...perhaps that makes sense? I'm not
really sure. But in that case we need another Kconfig option.

Regards,
Simon

Re: [Question] TI's u-boot.img is built twice

[Andrew Davis]

On 9/22/23 11:40 AM, Simon Glass wrote:
> Hi Andrew,
> 
> On Fri, 22 Sept 2023 at 10:35, Andrew Davis <afd@ti.com> wrote:
>>
>> On 9/22/23 10:01 AM, Simon Glass wrote:
>>> Hi Masahiro,
>>>
>>> On Thu, 21 Sept 2023 at 09:36, Masahiro Yamada <masahiroy@kernel.org> wrote:
>>>>
>>>> Hi.
>>>>
>>>> Since the TI platform migrated to binman,
>>>> u-boot.img is built twice.
>>>>
>>>> It is created by "mkimage -E",
>>>> then overwritten by binman.
>>>>
>>>>
>>>> So, the data are embedded in the FIT structure
>>>> instead of being appended.
>>>>
>>>> Is this intentional?
>>>>
>>>> To me, it looks weird.
>>>>
>>>>
>>>>
>>>>
>>>> To confirm it, apply the following hack.
>>>>
>>>> Since u-boot.img is overwritten by binman,
>>>> copy it to u-boot.img.backup.
>>>>
>>>>
>>>>
>>>>
>>>> diff --git a/Makefile b/Makefile
>>>> index 87f9fc786e..4cffa8a061 100644
>>>> --- a/Makefile
>>>> +++ b/Makefile
>>>> @@ -1112,6 +1112,7 @@ endef
>>>>    # Timestamp file to make sure that binman always runs
>>>>    .binman_stamp: $(INPUTS-y) FORCE
>>>>    ifeq ($(CONFIG_BINMAN),y)
>>>> +       cp u-boot.img u-boot.img.backup
>>>>           $(call if_changed,binman)
>>>>    endif
>>>>           @touch $@
>>>>
>>>>
>>>>
>>>> Then, build it for the main core.
>>>>
>>>>
>>>> make -j$(nproc) CROSS_COMPILE=aarch64-linux-gnu-
>>>>      am64x_evm_a53_defconfig all
>>>>      TEE=~/ref/OP-TEE/optee_os/out/arm-plat-k3/core/tee-raw.bin
>>>>      BL31=~/ref/trusted-firmware-a/build/k3/lite/release/bl31.bin
>>>>      BINMAN_INDIRS=~/ref/ti-linux-firmware
>>>>
>>>>
>>>>
>>>>
>>>> Compare the two files.
>>>> Run fdtdump to see what happened to them.
>>>>
>>>>
>>>> $ diff -u u-boot.img  u-boot.img.backup
>>>> Binary files u-boot.img and u-boot.img.backup differ
>>>>
>>>>
>>>> $ fdtdump u-boot.img
>>>>       => u-boot and dt are embedded.
>>>>
>>>> $ fdtdump u-boot.img.backup
>>>>       => u-boot and dt are appended after the
>>>>          FIT structure
>>>
>>> That seems like a bug to me...at some point we might consider building
>>> u-boot.img with Binman, but for now it is built by the Makefile.
>>>
>>
>> This is not true for K3 as we now use Binman for u-boot.img generation
>> (we need this as we have a signing step injected at this point).
> 
> No, no.
> 
> You must not mess with the outputs of Makefile - create a new file
> with what you need, e.g. u-boot-ti.img
> 
> While we could use Binman to produce the .img we are quite a way from
> doing that as not that many boards have been converted to binman.
> 

Too late, we already use Binman to generate u-boot.img on K3, why
would we want to go back to using Makefile?

Let's go with what you suggest below and add a Kconfig that can
be used for boards that have already switched to binman that disables
the Makefile generator for u-boot.img.

Once all boards have switched to Binman, then we could refactor
the implementations into something more common.

Andrew

>>
>> So as Masahiro suggested in the other branch of this thread, we need
>> to disable the Makefile generation of u-boot.img for K3.
>>
>> Neha,
>>
>> Can you take this action? I assume you can add it as part of your
>> work in making the data external again, which would make the u-boot.img
>> identical to before and hence Makefile version could be disabled at
>> the same time/series.
> 
> If you are suggesting that we slowly migrate boards away from
> generating the .img using Makefile...perhaps that makes sense? I'm not
> really sure. But in that case we need another Kconfig option.
> 
> Regards,
> Simon

Re: [Question] TI's u-boot.img is built twice

[Simon Glass]

Hi Andrew,

On Fri, 22 Sept 2023 at 12:14, Andrew Davis <afd@ti.com> wrote:
>
> On 9/22/23 11:40 AM, Simon Glass wrote:
> > Hi Andrew,
> >
> > On Fri, 22 Sept 2023 at 10:35, Andrew Davis <afd@ti.com> wrote:
> >>
> >> On 9/22/23 10:01 AM, Simon Glass wrote:
> >>> Hi Masahiro,
> >>>
> >>> On Thu, 21 Sept 2023 at 09:36, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >>>>
> >>>> Hi.
> >>>>
> >>>> Since the TI platform migrated to binman,
> >>>> u-boot.img is built twice.
> >>>>
> >>>> It is created by "mkimage -E",
> >>>> then overwritten by binman.
> >>>>
> >>>>
> >>>> So, the data are embedded in the FIT structure
> >>>> instead of being appended.
> >>>>
> >>>> Is this intentional?
> >>>>
> >>>> To me, it looks weird.
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> To confirm it, apply the following hack.
> >>>>
> >>>> Since u-boot.img is overwritten by binman,
> >>>> copy it to u-boot.img.backup.
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> diff --git a/Makefile b/Makefile
> >>>> index 87f9fc786e..4cffa8a061 100644
> >>>> --- a/Makefile
> >>>> +++ b/Makefile
> >>>> @@ -1112,6 +1112,7 @@ endef
> >>>>    # Timestamp file to make sure that binman always runs
> >>>>    .binman_stamp: $(INPUTS-y) FORCE
> >>>>    ifeq ($(CONFIG_BINMAN),y)
> >>>> +       cp u-boot.img u-boot.img.backup
> >>>>           $(call if_changed,binman)
> >>>>    endif
> >>>>           @touch $@
> >>>>
> >>>>
> >>>>
> >>>> Then, build it for the main core.
> >>>>
> >>>>
> >>>> make -j$(nproc) CROSS_COMPILE=aarch64-linux-gnu-
> >>>>      am64x_evm_a53_defconfig all
> >>>>      TEE=~/ref/OP-TEE/optee_os/out/arm-plat-k3/core/tee-raw.bin
> >>>>      BL31=~/ref/trusted-firmware-a/build/k3/lite/release/bl31.bin
> >>>>      BINMAN_INDIRS=~/ref/ti-linux-firmware
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> Compare the two files.
> >>>> Run fdtdump to see what happened to them.
> >>>>
> >>>>
> >>>> $ diff -u u-boot.img  u-boot.img.backup
> >>>> Binary files u-boot.img and u-boot.img.backup differ
> >>>>
> >>>>
> >>>> $ fdtdump u-boot.img
> >>>>       => u-boot and dt are embedded.
> >>>>
> >>>> $ fdtdump u-boot.img.backup
> >>>>       => u-boot and dt are appended after the
> >>>>          FIT structure
> >>>
> >>> That seems like a bug to me...at some point we might consider building
> >>> u-boot.img with Binman, but for now it is built by the Makefile.
> >>>
> >>
> >> This is not true for K3 as we now use Binman for u-boot.img generation
> >> (we need this as we have a signing step injected at this point).
> >
> > No, no.
> >
> > You must not mess with the outputs of Makefile - create a new file
> > with what you need, e.g. u-boot-ti.img
> >
> > While we could use Binman to produce the .img we are quite a way from
> > doing that as not that many boards have been converted to binman.
> >
>
> Too late, we already use Binman to generate u-boot.img on K3, why
> would we want to go back to using Makefile?

I don't mean that, I mean give it another name. The u-boot.img file is
intended to be a certain format, and it seems this is being changed.

Really, this is a problem with binman, as it should have refused to
creating u-boot.img...?

>
> Let's go with what you suggest below and add a Kconfig that can
> be used for boards that have already switched to binman that disables
> the Makefile generator for u-boot.img.
>
> Once all boards have switched to Binman, then we could refactor
> the implementations into something more common.

That seems OK to me.

Regards,
Simon

>
> Andrew
>
> >>
> >> So as Masahiro suggested in the other branch of this thread, we need
> >> to disable the Makefile generation of u-boot.img for K3.
> >>
> >> Neha,
> >>
> >> Can you take this action? I assume you can add it as part of your
> >> work in making the data external again, which would make the u-boot.img
> >> identical to before and hence Makefile version could be disabled at
> >> the same time/series.
> >
> > If you are suggesting that we slowly migrate boards away from
> > generating the .img using Makefile...perhaps that makes sense? I'm not
> > really sure. But in that case we need another Kconfig option.
> >
> > Regards,
> > Simon

Re: [Question] TI's u-boot.img is built twice

[Andrew Davis]

On 9/22/23 1:29 PM, Simon Glass wrote:
> Hi Andrew,
> 
> On Fri, 22 Sept 2023 at 12:14, Andrew Davis <afd@ti.com> wrote:
>>
>> On 9/22/23 11:40 AM, Simon Glass wrote:
>>> Hi Andrew,
>>>
>>> On Fri, 22 Sept 2023 at 10:35, Andrew Davis <afd@ti.com> wrote:
>>>>
>>>> On 9/22/23 10:01 AM, Simon Glass wrote:
>>>>> Hi Masahiro,
>>>>>
>>>>> On Thu, 21 Sept 2023 at 09:36, Masahiro Yamada <masahiroy@kernel.org> wrote:
>>>>>>
>>>>>> Hi.
>>>>>>
>>>>>> Since the TI platform migrated to binman,
>>>>>> u-boot.img is built twice.
>>>>>>
>>>>>> It is created by "mkimage -E",
>>>>>> then overwritten by binman.
>>>>>>
>>>>>>
>>>>>> So, the data are embedded in the FIT structure
>>>>>> instead of being appended.
>>>>>>
>>>>>> Is this intentional?
>>>>>>
>>>>>> To me, it looks weird.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> To confirm it, apply the following hack.
>>>>>>
>>>>>> Since u-boot.img is overwritten by binman,
>>>>>> copy it to u-boot.img.backup.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> diff --git a/Makefile b/Makefile
>>>>>> index 87f9fc786e..4cffa8a061 100644
>>>>>> --- a/Makefile
>>>>>> +++ b/Makefile
>>>>>> @@ -1112,6 +1112,7 @@ endef
>>>>>>     # Timestamp file to make sure that binman always runs
>>>>>>     .binman_stamp: $(INPUTS-y) FORCE
>>>>>>     ifeq ($(CONFIG_BINMAN),y)
>>>>>> +       cp u-boot.img u-boot.img.backup
>>>>>>            $(call if_changed,binman)
>>>>>>     endif
>>>>>>            @touch $@
>>>>>>
>>>>>>
>>>>>>
>>>>>> Then, build it for the main core.
>>>>>>
>>>>>>
>>>>>> make -j$(nproc) CROSS_COMPILE=aarch64-linux-gnu-
>>>>>>       am64x_evm_a53_defconfig all
>>>>>>       TEE=~/ref/OP-TEE/optee_os/out/arm-plat-k3/core/tee-raw.bin
>>>>>>       BL31=~/ref/trusted-firmware-a/build/k3/lite/release/bl31.bin
>>>>>>       BINMAN_INDIRS=~/ref/ti-linux-firmware
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Compare the two files.
>>>>>> Run fdtdump to see what happened to them.
>>>>>>
>>>>>>
>>>>>> $ diff -u u-boot.img  u-boot.img.backup
>>>>>> Binary files u-boot.img and u-boot.img.backup differ
>>>>>>
>>>>>>
>>>>>> $ fdtdump u-boot.img
>>>>>>        => u-boot and dt are embedded.
>>>>>>
>>>>>> $ fdtdump u-boot.img.backup
>>>>>>        => u-boot and dt are appended after the
>>>>>>           FIT structure
>>>>>
>>>>> That seems like a bug to me...at some point we might consider building
>>>>> u-boot.img with Binman, but for now it is built by the Makefile.
>>>>>
>>>>
>>>> This is not true for K3 as we now use Binman for u-boot.img generation
>>>> (we need this as we have a signing step injected at this point).
>>>
>>> No, no.
>>>
>>> You must not mess with the outputs of Makefile - create a new file
>>> with what you need, e.g. u-boot-ti.img
>>>
>>> While we could use Binman to produce the .img we are quite a way from
>>> doing that as not that many boards have been converted to binman.
>>>
>>
>> Too late, we already use Binman to generate u-boot.img on K3, why
>> would we want to go back to using Makefile?
> 
> I don't mean that, I mean give it another name. The u-boot.img file is
> intended to be a certain format, and it seems this is being changed.
> 

Well we need u-boot.img, and it needs to be called that, and
it needs to be the one generated by Binman (the one generated by
Makefile can only boot on GP devices, but not HS-SE).

The format produced by Makefile and Binman will be identical after
we fix Binman to put the data at the end. At which point why not
drop the Makefile made one?

Andrew

> Really, this is a problem with binman, as it should have refused to
> creating u-boot.img...?
> 
>>
>> Let's go with what you suggest below and add a Kconfig that can
>> be used for boards that have already switched to binman that disables
>> the Makefile generator for u-boot.img.
>>
>> Once all boards have switched to Binman, then we could refactor
>> the implementations into something more common.
> 
> That seems OK to me.
> 
> Regards,
> Simon
> 
>>
>> Andrew
>>
>>>>
>>>> So as Masahiro suggested in the other branch of this thread, we need
>>>> to disable the Makefile generation of u-boot.img for K3.
>>>>
>>>> Neha,
>>>>
>>>> Can you take this action? I assume you can add it as part of your
>>>> work in making the data external again, which would make the u-boot.img
>>>> identical to before and hence Makefile version could be disabled at
>>>> the same time/series.
>>>
>>> If you are suggesting that we slowly migrate boards away from
>>> generating the .img using Makefile...perhaps that makes sense? I'm not
>>> really sure. But in that case we need another Kconfig option.
>>>
>>> Regards,
>>> Simon

Re: [Question] TI's u-boot.img is built twice

[Simon Glass]

Hi Andrew,

On Fri, 22 Sept 2023 at 12:44, Andrew Davis <afd@ti.com> wrote:
>
> On 9/22/23 1:29 PM, Simon Glass wrote:
> > Hi Andrew,
> >
> > On Fri, 22 Sept 2023 at 12:14, Andrew Davis <afd@ti.com> wrote:
> >>
> >> On 9/22/23 11:40 AM, Simon Glass wrote:
> >>> Hi Andrew,
> >>>
> >>> On Fri, 22 Sept 2023 at 10:35, Andrew Davis <afd@ti.com> wrote:
> >>>>
> >>>> On 9/22/23 10:01 AM, Simon Glass wrote:
> >>>>> Hi Masahiro,
> >>>>>
> >>>>> On Thu, 21 Sept 2023 at 09:36, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >>>>>>
> >>>>>> Hi.
> >>>>>>
> >>>>>> Since the TI platform migrated to binman,
> >>>>>> u-boot.img is built twice.
> >>>>>>
> >>>>>> It is created by "mkimage -E",
> >>>>>> then overwritten by binman.
> >>>>>>
> >>>>>>
> >>>>>> So, the data are embedded in the FIT structure
> >>>>>> instead of being appended.
> >>>>>>
> >>>>>> Is this intentional?
> >>>>>>
> >>>>>> To me, it looks weird.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> To confirm it, apply the following hack.
> >>>>>>
> >>>>>> Since u-boot.img is overwritten by binman,
> >>>>>> copy it to u-boot.img.backup.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> diff --git a/Makefile b/Makefile
> >>>>>> index 87f9fc786e..4cffa8a061 100644
> >>>>>> --- a/Makefile
> >>>>>> +++ b/Makefile
> >>>>>> @@ -1112,6 +1112,7 @@ endef
> >>>>>>     # Timestamp file to make sure that binman always runs
> >>>>>>     .binman_stamp: $(INPUTS-y) FORCE
> >>>>>>     ifeq ($(CONFIG_BINMAN),y)
> >>>>>> +       cp u-boot.img u-boot.img.backup
> >>>>>>            $(call if_changed,binman)
> >>>>>>     endif
> >>>>>>            @touch $@
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Then, build it for the main core.
> >>>>>>
> >>>>>>
> >>>>>> make -j$(nproc) CROSS_COMPILE=aarch64-linux-gnu-
> >>>>>>       am64x_evm_a53_defconfig all
> >>>>>>       TEE=~/ref/OP-TEE/optee_os/out/arm-plat-k3/core/tee-raw.bin
> >>>>>>       BL31=~/ref/trusted-firmware-a/build/k3/lite/release/bl31.bin
> >>>>>>       BINMAN_INDIRS=~/ref/ti-linux-firmware
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Compare the two files.
> >>>>>> Run fdtdump to see what happened to them.
> >>>>>>
> >>>>>>
> >>>>>> $ diff -u u-boot.img  u-boot.img.backup
> >>>>>> Binary files u-boot.img and u-boot.img.backup differ
> >>>>>>
> >>>>>>
> >>>>>> $ fdtdump u-boot.img
> >>>>>>        => u-boot and dt are embedded.
> >>>>>>
> >>>>>> $ fdtdump u-boot.img.backup
> >>>>>>        => u-boot and dt are appended after the
> >>>>>>           FIT structure
> >>>>>
> >>>>> That seems like a bug to me...at some point we might consider building
> >>>>> u-boot.img with Binman, but for now it is built by the Makefile.
> >>>>>
> >>>>
> >>>> This is not true for K3 as we now use Binman for u-boot.img generation
> >>>> (we need this as we have a signing step injected at this point).
> >>>
> >>> No, no.
> >>>
> >>> You must not mess with the outputs of Makefile - create a new file
> >>> with what you need, e.g. u-boot-ti.img
> >>>
> >>> While we could use Binman to produce the .img we are quite a way from
> >>> doing that as not that many boards have been converted to binman.
> >>>
> >>
> >> Too late, we already use Binman to generate u-boot.img on K3, why
> >> would we want to go back to using Makefile?
> >
> > I don't mean that, I mean give it another name. The u-boot.img file is
> > intended to be a certain format, and it seems this is being changed.
> >
>
> Well we need u-boot.img, and it needs to be called that, and
> it needs to be the one generated by Binman (the one generated by
> Makefile can only boot on GP devices, but not HS-SE).
>
> The format produced by Makefile and Binman will be identical after
> we fix Binman to put the data at the end. At which point why not
> drop the Makefile made one?

I think you understand my concern, which is really around overwriting
an existing file with Binman. So long as you can resolve that in a
deterministic way (i.e. Kconfig) then I am happy.

Regards,
Simon

>
> Andrew
>
> > Really, this is a problem with binman, as it should have refused to
> > creating u-boot.img...?
> >
> >>
> >> Let's go with what you suggest below and add a Kconfig that can
> >> be used for boards that have already switched to binman that disables
> >> the Makefile generator for u-boot.img.
> >>
> >> Once all boards have switched to Binman, then we could refactor
> >> the implementations into something more common.
> >
> > That seems OK to me.
> >
> > Regards,
> > Simon
> >
> >>
> >> Andrew
> >>
> >>>>
> >>>> So as Masahiro suggested in the other branch of this thread, we need
> >>>> to disable the Makefile generation of u-boot.img for K3.
> >>>>
> >>>> Neha,
> >>>>
> >>>> Can you take this action? I assume you can add it as part of your
> >>>> work in making the data external again, which would make the u-boot.img
> >>>> identical to before and hence Makefile version could be disabled at
> >>>> the same time/series.
> >>>
> >>> If you are suggesting that we slowly migrate boards away from
> >>> generating the .img using Makefile...perhaps that makes sense? I'm not
> >>> really sure. But in that case we need another Kconfig option.
> >>>
> >>> Regards,
> >>> Simon