[PATCH] Revert "tree: imx: remove old fit generator script"

Tim Harvey tharvey at gateworks.com
Fri Jan 7 22:41:42 CET 2022 

On Fri, Jan 7, 2022 at 1:18 PM Adam Ford <aford173 at gmail.com> wrote:
>
>
>
> On Fri, Jan 7, 2022 at 2:14 PM Tom Rini <trini at konsulko.com> wrote:
>>
>> On Fri, Jan 07, 2022 at 02:08:00PM -0600, Adam Ford wrote:
>> > On Fri, Jan 7, 2022 at 12:25 PM Tom Rini <trini at konsulko.com> wrote:
>> >
>> > > On Fri, Jan 07, 2022 at 12:21:18PM -0600, Adam Ford wrote:
>> > > > On Fri, Jan 7, 2022 at 11:38 AM Tom Rini <trini at konsulko.com> wrote:
>> > > >
>> > > > > On Fri, Jan 07, 2022 at 09:27:05AM -0800, Tim Harvey wrote:
>> > > > > > On Thu, Jan 6, 2022 at 12:27 PM Tim Harvey <tharvey at gateworks.com>
>> > > > > wrote:
>> > > > > > >
>> > > > > > > On Thu, Jan 6, 2022 at 11:18 AM ZHIZHIKIN Andrey
>> > > > > > > <andrey.zhizhikin at leica-geosystems.com> wrote:
>> > > > > > > >
>> > > > > > > > Hello Tom,
>> > > > > > > >
>> > > > > > > > > -----Original Message-----
>> > > > > > > > > From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Tom
>> > > Rini
>> > > > > > > > > Sent: Thursday, January 6, 2022 7:52 PM
>> > > > > > > > > To: u-boot at lists.denx.de
>> > > > > > > > > Cc: Tim Harvey <tharvey at gateworks.com>
>> > > > > > > > > Subject: [PATCH] Revert "tree: imx: remove old fit generator
>> > > > > script"
>> > > > > > > > >
>> > > > > > > > > This reverts commit d9a6f0eed66a39206b13513ec914f14084c3bb73.
>> > > > > > > > >
>> > > > > > > > > For right now, it's too close to the release to merge the
>> > > series
>> > > > > that
>> > > > > > > > > allows for binman to be used to generate the final images, and
>> > > > > also not
>> > > > > > > > > break CI, and then also merge all of the series that convert
>> > > > > currently
>> > > > > > > > > broken platforms to use binman instead.  So, bring back this
>> > > > > script now
>> > > > > > > > > and remove it again for real after the release.
>> > > > > > > >
>> > > > > > > > Please note that this might not work, as the FIT generator script
>> > > > > would
>> > > > > > > > generate ITS with '@' symbols which are not compatible with
>> > > mkimage
>> > > > > due
>> > > > > > > > to CVE-2021-27138. This revert should be complemented with the
>> > > fix to
>> > > > > > > > remove those '@' symbols as well.
>> > > > > > >
>> > > > > > > Correct, the revert is not enough anymore:
>> > > > > > >   MKIMAGE u-boot.itb
>> > > > > > > u-boot.its:7.11-15.5: Warning (unit_address_vs_reg):
>> > > /images/uboot at 1:
>> > > > > > > node has a unit name, but no reg property
>> > > > > > > u-boot.its:16.9-21.5: Warning (unit_address_vs_reg): /images/fdt at 1
>> > > :
>> > > > > > > node has a unit name, but no reg property
>> > > > > > > u-boot.its:22.9-27.5: Warning (unit_address_vs_reg): /images/fdt at 2
>> > > :
>> > > > > > > node has a unit name, but no reg property
>> > > > > > > u-boot.its:28.9-33.5: Warning (unit_address_vs_reg): /images/fdt at 3
>> > > :
>> > > > > > > node has a unit name, but no reg property
>> > > > > > > u-boot.its:34.9-39.5: Warning (unit_address_vs_reg): /images/fdt at 4
>> > > :
>> > > > > > > node has a unit name, but no reg property
>> > > > > > > u-boot.its:40.9-45.5: Warning (unit_address_vs_reg): /images/fdt at 5
>> > > :
>> > > > > > > node has a unit name, but no reg property
>> > > > > > > u-boot.its:46.9-55.5: Warning (unit_address_vs_reg): /images/atf at 1
>> > > :
>> > > > > > > node has a unit name, but no reg property
>> > > > > > > u-boot.its:60.12-65.5: Warning (unit_address_vs_reg):
>> > > > > > > /configurations/config at 1: node has a unit name, but no reg
>> > > property
>> > > > > > > u-boot.its:66.12-71.5: Warning (unit_address_vs_reg):
>> > > > > > > /configurations/config at 2: node has a unit name, but no reg
>> > > property
>> > > > > > > u-boot.its:72.12-77.5: Warning (unit_address_vs_reg):
>> > > > > > > /configurations/config at 3: node has a unit name, but no reg
>> > > property
>> > > > > > > u-boot.its:78.12-83.5: Warning (unit_address_vs_reg):
>> > > > > > > /configurations/config at 4: node has a unit name, but no reg
>> > > property
>> > > > > > > u-boot.its:84.12-89.5: Warning (unit_address_vs_reg):
>> > > > > > > /configurations/config at 5: node has a unit name, but no reg
>> > > property
>> > > > > > > ./tools/mkimage: verify_header failed for FIT Image support with
>> > > exit
>> > > > > code 1
>> > > > > > > Makefile:1433: recipe for target 'u-boot.itb' failed
>> > > > > > > make: *** [u-boot.itb] Error 1
>> > > > > > > make: *** Deleting file 'u-boot.itb'
>> > > > > > > make: *** Waiting for unfinished jobs....
>> > > > > > >
>> > > > > > > I don't know what had changed to cause this or when (again, I
>> > > stopped
>> > > > > > > worrying about it because I thought we were moving to binman for
>> > > this
>> > > > > > > release). There was a patch that resolved this from Oliver at
>> > > > > > > https://lists.denx.de/pipermail/u-boot/2021-August/457997.html
>> > > but I
>> > > > > > > don't think that fully solves anything 'at this point' either.
>> > > > > > >
>> > > > > > > Even with that applied to current master I then end up with:
>> > > > > > >   MKIMAGE flash.bin
>> > > > > > > ./tools/mkimage: Can't open spl/u-boot-spl-ddr.bin: No such file or
>> > > > > directory
>> > > > > > > arch/arm/mach-imx/Makefile:167: recipe for target 'flash.bin'
>> > > failed
>> > > > > > > make[1]: *** [flash.bin] Error 1
>> > > > > > > make[1]: *** Deleting file 'flash.bin'
>> > > > > > > Makefile:1526: recipe for target 'flash.bin' failed
>> > > > > > >
>> > > > > > > At some point over the past couple of months that patch resolved
>> > > the
>> > > > > > > building issue when using the FIT generator but I also don't know
>> > > what
>> > > > > > > else has changed that now causes that to not work.
>> > > > > > >
>> > > > > > > As Tom pointed out in another thread these build failures did not
>> > > get
>> > > > > > > caught by CI apparently because CI does a 'make all' which did not
>> > > > > > > include the FIT images (that was accomplished with the 'flash.bin'
>> > > > > > > target prior to binman conversion).
>> > > > > > >
>> > > > > > > Is it too late to apply the CI fix and the pending binman
>> > > conversions?
>> > > > > > >
>> > > > > > > I know that my series has been reviewed by Marcel [1] and as far
>> > > as I
>> > > > > > > know didn't get merged simply because of the CI issue. It still
>> > > > > > > applies and produces a valid flash.bin image.
>> > > > > > > I was also able to merge Peng's series [2] which converts
>> > > > > > > imx8mq_evk/imx8mq_phanbell/pico-imx8mq to binman and was able to
>> > > build
>> > > > > > > flash.bin images for them
>> > > > > > >
>> > > > > > > I tried to merge Adam's series that moves imx8mm_beacon to binman
>> > > [3]
>> > > > > > > and imx8mn_beacon to binman [4] but they no longer apply due to
>> > > > > > > defconfig/Kconfig changes
>> > > > > > >
>> > > > > > > That still leaves the following unbuildable with
>> > > > > > > CONFIG_SPL_FIT_GENERATOR = "arch/arm/mach-imx/mkimage_fit_atf.sh":
>> > > > > > >
>> > > > >
>> > > configs/cgtqmx8_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
>> > > > > > >
>> > > > >
>> > > configs/imx8mm-icore-mx8mm-ctouch2_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
>> > > > > > >
>> > > > >
>> > > configs/imx8mm-icore-mx8mm-edimm2.2_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
>> > > > > > >
>> > > > >
>> > > configs/imx8mm_beacon_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
>> > > > > > >
>> > > > >
>> > > configs/imx8mn_beacon_2g_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
>> > > > > > >
>> > > > >
>> > > configs/imx8mn_beacon_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
>> > > > > > >
>> > > > >
>> > > configs/imx8qm_rom7720_a1_4G_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
>> > > > > > >
>> > > > > > > Tim
>> > > > > > > [1] https://patchwork.ozlabs.org/project/uboot/list/?series=265765
>> > > > > > > [2] https://patchwork.ozlabs.org/project/uboot/list/?series=268380
>> > > > > > > [3] https://patchwork.ozlabs.org/project/uboot/list/?series=261640
>> > > > > > > [4] https://patchwork.ozlabs.org/project/uboot/list/?series=261822
>> > > > > > >
>> > > > > >
>> > > > > > Tom,
>> > > > > >
>> > > > > > I'm not familiar with the U-boot CI tool.
>> > > > >
>> > > > > It's at
>> > > https://u-boot.readthedocs.io/en/latest/develop/ci_testing.html
>> > > > > (and in-tree under doc/develop/ci_testing.rst).
>> > > > >
>> > > > > > Is it a show-stopper that it
>> > > > > > does not build for boards using binman for release? From what you
>> > > > > > mentioned in another thread it was never building the flash.bin
>> > > target
>> > > > > > for the boards using the FIT generator anyway.
>> > > > >
>> > > > > Yes, breaking CI is a ship-stopper.  That all of these boards were not
>> > > > > previously building the final image as part of "all" is a problem.
>> > > > >
>> > > > > So, here's what I'm at right now.  I've grabbed Heiko's patch.
>> > > > > Everything is currently visible at
>> > > > >
>> > > > >
>> > > https://patchwork.ozlabs.org/bundle/trini/2022-01-07-imx8-and-binman-updates/
>> > > > > and with this, we get some boards building and complaining as expected:
>> > > > >    aarch64:  w+   imx8mn_evk
>> > > > > +(imx8mn_evk) Image 'main-section' is missing external blobs and is
>> > > > > non-functional: blob-ext at 1
>> > > > > blob-ext at 2 blob-ext at 3 blob-ext at 4
>> > > > > +(imx8mn_evk) Image 'main-section' is missing external blobs and is
>> > > > > non-functional: blob-ext
>> > > > > +(imx8mn_evk)
>> > > > > +(imx8mn_evk) Some images are invalid
>> > > > >
>> > > > > But others are:
>> > > > >    aarch64:  +   imx8mn_beacon_2g
>> > > > > +(imx8mn_beacon_2g) ===================== WARNING
>> > > ======================
>> > > > > +(imx8mn_beacon_2g) This board uses CONFIG_SPL_FIT_GENERATOR. Please
>> > > > > migrate
>> > > > > +(imx8mn_beacon_2g) to binman instead, to avoid the proliferation of
>> > > > > +(imx8mn_beacon_2g) arch-specific scripts with no tests.
>> > > > > +(imx8mn_beacon_2g)
>> > > ====================================================
>> > > > > +(imx8mn_beacon_2g) Image 'main-section' is missing external blobs and
>> > > is
>> > > > > non-functional: blob-
>> > > > > ext at 1 blob-ext at 2 blob-ext at 3 blob-ext at 4
>> > > > > +(imx8mn_beacon_2g) binman: Error 1 running 'mkimage -d
>> > > > > ./mkimage.spl.mkimage -n spl/u-boot-spl
>> > > > > .cfgout -T imx8mimage -e 0x912000 ./mkimage-out.spl.mkimage':
>> > > > > spl/u-boot-spl-ddr.bin: Can't ope
>> > > > > n: No such file or directory
>> > > > > +(imx8mn_beacon_2g)
>> > > > > +(imx8mn_beacon_2g) make[1]: *** [Makefile:1088: all] Error 1
>> > > > > +(imx8mn_beacon_2g) make: *** [Makefile:177: sub-make] Error 2
>> > > > >
>> > > >
>> > > > The beacon boards are mine.  I can work on this one today.  Do I just
>> > > grab
>> > > > the binman updates and apply it to master and fix it from that starting
>> > > > point?
>> > >
>> > > So, yes, grabbing the series from the above link and applying it on top
>> > > of master, and making builds work with fake binaries would be very
>> > > helpful.  As best I can tell there's still something missing with making
>> > > fake blobs link and not fail.  For example, imx8mm_venice is part of the
>> > > series above but still fails with 'make BINMAN_FAKE_EXT_BLOBS=1 ...':
>> > > Image 'main-section:u-boot-spl-ddr' has faked external blobs and is
>> > > non-functional: lpddr4_pmu_train_1d_imem.bin lpddr4_pmu_train_1d_dmem.bin
>> > > lpddr4_pmu_train_2d_imem.bin lpddr4_pmu_train_2d_dmem.bin
>> > > Image 'main-section' is missing external blobs and is non-functional:
>> > > atf_blob blob-ext
>> > > Wrote map file './imx-boot.map' to show errors
>> > > binman: Node '/binman/imx-boot/blob-ext at 1': Offset 0x0 (0) overlaps with
>> > > previous entry '/binman/imx-boot/uboot' ending at 0x1aee08 (1764872)
>> > >
>> > >
>> > From what I can tell looking at the imx8mm_venice board is that
>> > the imx8mm-u-boot.dtsi has the binman nodes to build the image,
>> > but arch/arm/dts/imx8mm-venice-u-boot.dtsi duplicates that work, so there
>> > are two copies trying to occupy the same space.  Deleting the &binman node
>> > from the venice-u-boot.dtsi file appears to make the problem go away, and
>> > binman is still making the image.
>> >
>> > $ cat imx-boot.map
>> > ImagePos    Offset      Size  Name
>> > 00000000  00000000  0011d778  main-section
>> > 00000000   00000000  00039000  spl
>> > 00057c00   00057c00  000c5b78  uboot
>> >
>> > $ cat spl.map
>> > ImagePos    Offset      Size  Name
>> > 00000000  00000000  00039000  main-section
>> > 00000000   00000000  00039000  mkimage
>> >
>> > $ cat u-boot-spl-ddr.map
>> > ImagePos    Offset      Size  Name
>> > 00000000  00000000  00036f5c  main-section
>> > 00000000   00000000  0001ef5c  u-boot-spl
>> > 00000000    00000000  0001dc40  u-boot-spl-nodtb
>> > 0001dc40    0001dc40  0000131c  u-boot-spl-dtb
>> > 0001ef5c   0001ef5c  00008000  1d-imem
>> > 00026f5c   00026f5c  00004000  1d_dmem
>> > 0002af5c   0002af5c  00008000  2d_imem
>> > 00032f5c   00032f5c  00004000  2d_dmem
>> >
>> > Tim,
>> >
>> > Can you try deleting the &binman node from your mx8mm-venice-u-boot.dtsi
>> > file and see if the stock binman node in 8mm-u-boot-dtsi works for you?
>> > The new default one does for me on my Beacon 8mm board.
>> >
>> > Tom,
>> >
>> > I will submit another patch for the imx8mn_beacon boards to fix the build
>> > error on the 2g version.  I forgot to update the second defconfig file when
>> > I pushed the patch for the first one.   I have it working, but I need to
>> > clean it up.
>>
>> OK, thanks!  I just confirmed that with the above series it's down to:
>> imx8mn_beacon_2g imx8mm_venice kontron-sl-mx8mm phycore-imx8mm
>> as the fails to build in CI boards:
>
>
> I just submitted two patches against this WIP branch to remove the duplicate binman node from their respective u-boot.dtsi files as well.  The kontron and phycore boards had the same issue as the venice board.
>
> They were build-only tested.
>
>>
>> https://source.denx.de/u-boot/u-boot/-/jobs/372239 as part of
>> https://source.denx.de/u-boot/u-boot/-/pipelines/10497
>>
>> I feel at this point that worst-case I can come up with something to
>> make venice and kontron link still given what you figured out above and
>> shouldn't be any worse off than they are today.  And that can go in
>> v2022.01.
>>

Tom,

I just resent my '[v3] imx8mm_venice: switch to use binman to pack
images' as well (had to re-send as I forgot to include u-boot list
earlier).

Tim

More information about the U-Boot mailing list