| Message ID | 20210820205230.1369683-2-marcel@ziswiler.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | board: toradex: verdin-imx8mm: target refresh | expand |
Hello Marcel, > -----Original Message----- > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Marcel Ziswiler > Sent: Friday, August 20, 2021 10:52 PM > To: u-boot@lists.denx.de > Cc: Heiko Thiery <heiko.thiery@gmail.com>; Stefano Babic <sbabic@denx.de>; > Fabio Estevam <festevam@gmail.com>; Frieder Schrempf > <frieder.schrempf@kontron.de>; Marcel Ziswiler > <marcel.ziswiler@toradex.com>; NXP i.MX U-Boot Team <uboot- > imx@nxp.com>; Peng Fan <peng.fan@nxp.com> > Subject: [PATCH v1 1/7] imx: mkimage_fit_atf: fix legacy image generation > > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > While most boards meanwhile migrated to using binman a few like the verdin- > imx8mm are still using the legacy image generation. > Unfortunately, the legacy image generation is currently broken which is especially > bad for any kind of bisection attempts. > Anyway, this fixes it even though we will also migrate to using binman shortly. This change has been already proposed in [1], but the discussion went into the direction of monolithic "flash.bin" rather than a migration to use binman. I guess if this change is really needed due to the fact that the migration of some boards is really difficult - the original patch can be taken. However, I've commented out in that thread that there is a warning regarding the usage of scripts and migration notice, so maybe it does make sense to spend extra effort to migrate away from this script at all? > > Fixes: commit cb9faa6f98ae > ("tools: Use a single target-independent config to enable OpenSSL") > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > --- > > arch/arm/mach-imx/mkimage_fit_atf.sh | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/arch/arm/mach-imx/mkimage_fit_atf.sh b/arch/arm/mach- > imx/mkimage_fit_atf.sh > index 2a17968794c..2d6c46633c7 100755 > --- a/arch/arm/mach-imx/mkimage_fit_atf.sh > +++ b/arch/arm/mach-imx/mkimage_fit_atf.sh > @@ -53,7 +53,7 @@ cat << __HEADER_EOF > description = "Configuration to load ATF before U-Boot"; > > images { > - uboot@1 { > + uboot_1 { > description = "U-Boot (64-bit)"; > os = "u-boot"; > data = /incbin/("$BL33"); @@ -68,7 +68,7 @@ cnt=1 for dtname in $* > do > cat << __FDT_IMAGE_EOF > - fdt@$cnt { > + fdt_$cnt { > description = "$(basename $dtname .dtb)"; > data = /incbin/("$dtname"); > type = "flat_dt"; @@ -79,7 +79,7 @@ cnt=$((cnt+1)) done > > cat << __HEADER_EOF > - atf@1 { > + atf_1 { > description = "ARM Trusted Firmware"; > os = "arm-trusted-firmware"; > data = /incbin/("$BL31"); @@ -93,7 +93,7 @@ __HEADER_EOF > > if [ -f $BL32 ]; then > cat << __HEADER_EOF > - tee@1 { > + tee_1 { > description = "TEE firmware"; > data = /incbin/("$BL32"); > type = "firmware"; @@ -108,7 +108,7 @@ fi cat << > __CONF_HEADER_EOF > }; > configurations { > - default = "config@1"; > + default = "config_1"; > > __CONF_HEADER_EOF > > @@ -117,20 +117,20 @@ for dtname in $* > do > if [ -f $BL32 ]; then > cat << __CONF_SECTION_EOF > - config@$cnt { > + config_$cnt { > description = "$(basename $dtname .dtb)"; > - firmware = "uboot@1"; > - loadables = "atf@1", "tee@1"; > - fdt = "fdt@$cnt"; > + firmware = "uboot_1"; > + loadables = "atf_1", "tee_1"; > + fdt = "fdt_$cnt"; > }; > __CONF_SECTION_EOF > else > cat << __CONF_SECTION1_EOF > - config@$cnt { > + config_$cnt { > description = "$(basename $dtname .dtb)"; > - firmware = "uboot@1"; > - loadables = "atf@1"; > - fdt = "fdt@$cnt"; > + firmware = "uboot_1"; > + loadables = "atf_1"; > + fdt = "fdt_$cnt"; > }; > __CONF_SECTION1_EOF > fi > -- > 2.26.2 Link: [1]: https://lore.kernel.org/u-boot/20210505120053.9466-1-oliver.graute@kococonnector.com/ Regards, Andrey
Hi Andrey Long time no see (;-p). On Sun, 2021-08-22 at 10:07 +0000, ZHIZHIKIN Andrey wrote: > Hello Marcel, > > > -----Original Message----- > > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Marcel Ziswiler > > Sent: Friday, August 20, 2021 10:52 PM > > To: u-boot@lists.denx.de > > Cc: Heiko Thiery <heiko.thiery@gmail.com>; Stefano Babic <sbabic@denx.de>; > > Fabio Estevam <festevam@gmail.com>; Frieder Schrempf > > <frieder.schrempf@kontron.de>; Marcel Ziswiler > > <marcel.ziswiler@toradex.com>; NXP i.MX U-Boot Team <uboot- > > imx@nxp.com>; Peng Fan <peng.fan@nxp.com> > > Subject: [PATCH v1 1/7] imx: mkimage_fit_atf: fix legacy image generation > > > > > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > > > While most boards meanwhile migrated to using binman a few like the verdin- > > imx8mm are still using the legacy image generation. > > Unfortunately, the legacy image generation is currently broken which is especially > > bad for any kind of bisection attempts. > > Anyway, this fixes it even though we will also migrate to using binman shortly. > > This change has been already proposed in [1], Well, what I do not get is how one can move forward and leave all kinds of stuff just broken. Fact is, that the legacy image creation has been and still is plain simply broken! > but the discussion went into the direction of monolithic "flash.bin" rather than a migration to use binman. Well, those two do actually not rule each other out. Remember, later in this patch set I am migrating to using binman which I instruct to generate a monolithic "flash.bin" again. > I guess if this change is really needed due to the fact that the migration of some boards is really difficult > - the original patch can be taken. What I found extremely problematic is, as mentioned initially, stuff is currently broken which makes e.g. bisecting other issues extremely cumbersome. But in theory, as I propose now to migrate anyway, we could just not care and leave it broken for anybody else. I just feel this is not really too nice of a gesture! > However, I've commented out in that thread that there is a warning regarding the usage of scripts and > migration notice, so maybe it does make sense to spend extra effort to migrate away from this script at all? Yes, of course, it is the goal to migrate. I just don't get how in IT new stuff gets introduced all the time with leaving past things broken. Just a little bit annoying... > > Fixes: commit cb9faa6f98ae > > ("tools: Use a single target-independent config to enable OpenSSL") > ... > > Link: [1]: https://lore.kernel.org/u-boot/20210505120053.9466-1-oliver.graute@kococonnector.com/ > > Regards, > Andrey Cheers Marcel
Hello Marcel! > -----Original Message----- > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > Sent: Monday, August 23, 2021 8:57 AM > To: u-boot@lists.denx.de; ZHIZHIKIN Andrey <andrey.zhizhikin@leica- > geosystems.com> > Cc: festevam@gmail.com; peng.fan@nxp.com; uboot-imx@nxp.com; > sbabic@denx.de; frieder.schrempf@kontron.de; heiko.thiery@gmail.com > Subject: Re: [PATCH v1 1/7] imx: mkimage_fit_atf: fix legacy image generation > > > Hi Andrey > > Long time no see (;-p). Indeed! :D > > On Sun, 2021-08-22 at 10:07 +0000, ZHIZHIKIN Andrey wrote: > > Hello Marcel, > > > > > -----Original Message----- > > > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Marcel > > > Ziswiler > > > Sent: Friday, August 20, 2021 10:52 PM > > > To: u-boot@lists.denx.de > > > Cc: Heiko Thiery <heiko.thiery@gmail.com>; Stefano Babic > > > <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>; Frieder > > > Schrempf <frieder.schrempf@kontron.de>; Marcel Ziswiler > > > <marcel.ziswiler@toradex.com>; NXP i.MX U-Boot Team <uboot- > > > imx@nxp.com>; Peng Fan <peng.fan@nxp.com> > > > Subject: [PATCH v1 1/7] imx: mkimage_fit_atf: fix legacy image > > > generation > > > > > > > > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > > > > > While most boards meanwhile migrated to using binman a few like the > > > verdin- imx8mm are still using the legacy image generation. > > > Unfortunately, the legacy image generation is currently broken which > > > is especially bad for any kind of bisection attempts. > > > Anyway, this fixes it even though we will also migrate to using binman shortly. > > > > This change has been already proposed in [1], > > Well, what I do not get is how one can move forward and leave all kinds of stuff > just broken. Fact is, that the legacy image creation has been and still is plain > simply broken! > > > but the discussion went into the direction of monolithic "flash.bin" rather than a > migration to use binman. > > Well, those two do actually not rule each other out. Remember, later in this patch > set I am migrating to using binman which I instruct to generate a monolithic > "flash.bin" again. > > > I guess if this change is really needed due to the fact that the > > migration of some boards is really difficult > > - the original patch can be taken. > > What I found extremely problematic is, as mentioned initially, stuff is currently > broken which makes e.g. > bisecting other issues extremely cumbersome. But in theory, as I propose now to > migrate anyway, we could just not care and leave it broken for anybody else. I > just feel this is not really too nice of a gesture! Totally agree, keeping the broken implementation is not a nice thing as people unaware of this might base their work on it and wonder why it does not build on the first place... Actually, I wanted to propose to remove this script from the tree and let derivatives that rely on it to fail to being forcibly converted to binman. But I was humble enough to do it through... There are still those boards that use FIT generator script: 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/imx8mm_venice_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/imx8mq_evk_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh" configs/imx8mq_phanbell_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" configs/phycore-imx8mm_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh" configs/pico-imx8mq_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh" configs/verdin-imx8mm_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh" Verdin gets converted with your other patch, while for all the others I'm not sure what the status is. There is however one board which has been introduced quite recent (Cc'ing Oliver here) and didn't went for binman implementation from the start. This is a clear indication that people are still relying on old FIT generator, even though there is a clear warning about that it is not suppose to be used. I guess at this stage, I'd rather leave this question for maintainers to decide - enforce the conversion to binman via deleting the FIT generator script, or fix the generator script with the patch from Oliver and let board maintainers to either use it or migrate. > > > However, I've commented out in that thread that there is a warning > > regarding the usage of scripts and migration notice, so maybe it does make > sense to spend extra effort to migrate away from this script at all? > > Yes, of course, it is the goal to migrate. I just don't get how in IT new stuff gets > introduced all the time with leaving past things broken. Just a little bit annoying... > > > > Fixes: commit cb9faa6f98ae > > > ("tools: Use a single target-independent config to enable OpenSSL") > > ... > > > > Cheers > > Marcel Cheers, Andrey
Hi Andrey, On Mon, Aug 23, 2021 at 4:50 AM ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com> wrote: > There are still those boards that use FIT generator script: > 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/imx8mm_venice_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/imx8mq_evk_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh" > configs/imx8mq_phanbell_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" > configs/phycore-imx8mm_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh" > configs/pico-imx8mq_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh" > configs/verdin-imx8mm_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh" > > Verdin gets converted with your other patch, while for all the others I'm not sure what the status is. > Peng sent a patch series converting imx8mq_evk, pico-imx8mq and imx8mq_phanbell to binman: https://www.mail-archive.com/u-boot@lists.denx.de/msg414962.html Regards, Fabio Estevam
Hello Fabio, > -----Original Message----- > From: Fabio Estevam <festevam@gmail.com> > Sent: Monday, August 23, 2021 1:00 PM > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com> > Cc: Marcel Ziswiler <marcel.ziswiler@toradex.com>; u-boot@lists.denx.de; > peng.fan@nxp.com; uboot-imx@nxp.com; sbabic@denx.de; > frieder.schrempf@kontron.de; heiko.thiery@gmail.com; Oliver Graute > <oliver.graute@kococonnector.com> > Subject: Re: [PATCH v1 1/7] imx: mkimage_fit_atf: fix legacy image generation > > > Hi Andrey, > > > > > Verdin gets converted with your other patch, while for all the others I'm not > sure what the status is. > > > > Peng sent a patch series converting imx8mq_evk, pico-imx8mq and > imx8mq_phanbell to binman: > https://www.mail-archive.com/u-boot@lists.denx.de/msg414962.html > Thanks for pointing it out! I guess the short list of affected candidates when mkimage_fit_atf.sh FIT generator gets deleted then would be: 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/imx8mm_venice_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" configs/phycore-imx8mm_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh" Should I send out an RFC to have the FIT generator script removed for 'mach-imx'? This would trigger the build failures for those derivatives, but since it is broken anyway - this would need to be solved by maintainers migrating them to binman. > Regards, > > Fabio Estevam Regards, Andrey
Hi Andrey, On Mon, Aug 23, 2021 at 12:53 PM ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com> wrote: > Thanks for pointing it out! > > I guess the short list of affected candidates when mkimage_fit_atf.sh FIT generator gets deleted then would be: > > 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/imx8mm_venice_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" > configs/phycore-imx8mm_defconfig:CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh" > > Should I send out an RFC to have the FIT generator script removed for 'mach-imx'? > > This would trigger the build failures for those derivatives, but since it is broken anyway - this would need to be solved by maintainers migrating them to binman. Yes, please send an RFC for that. Thanks
diff --git a/arch/arm/mach-imx/mkimage_fit_atf.sh b/arch/arm/mach-imx/mkimage_fit_atf.sh index 2a17968794c..2d6c46633c7 100755 --- a/arch/arm/mach-imx/mkimage_fit_atf.sh +++ b/arch/arm/mach-imx/mkimage_fit_atf.sh @@ -53,7 +53,7 @@ cat << __HEADER_EOF description = "Configuration to load ATF before U-Boot"; images { - uboot@1 { + uboot_1 { description = "U-Boot (64-bit)"; os = "u-boot"; data = /incbin/("$BL33"); @@ -68,7 +68,7 @@ cnt=1 for dtname in $* do cat << __FDT_IMAGE_EOF - fdt@$cnt { + fdt_$cnt { description = "$(basename $dtname .dtb)"; data = /incbin/("$dtname"); type = "flat_dt"; @@ -79,7 +79,7 @@ cnt=$((cnt+1)) done cat << __HEADER_EOF - atf@1 { + atf_1 { description = "ARM Trusted Firmware"; os = "arm-trusted-firmware"; data = /incbin/("$BL31"); @@ -93,7 +93,7 @@ __HEADER_EOF if [ -f $BL32 ]; then cat << __HEADER_EOF - tee@1 { + tee_1 { description = "TEE firmware"; data = /incbin/("$BL32"); type = "firmware"; @@ -108,7 +108,7 @@ fi cat << __CONF_HEADER_EOF }; configurations { - default = "config@1"; + default = "config_1"; __CONF_HEADER_EOF @@ -117,20 +117,20 @@ for dtname in $* do if [ -f $BL32 ]; then cat << __CONF_SECTION_EOF - config@$cnt { + config_$cnt { description = "$(basename $dtname .dtb)"; - firmware = "uboot@1"; - loadables = "atf@1", "tee@1"; - fdt = "fdt@$cnt"; + firmware = "uboot_1"; + loadables = "atf_1", "tee_1"; + fdt = "fdt_$cnt"; }; __CONF_SECTION_EOF else cat << __CONF_SECTION1_EOF - config@$cnt { + config_$cnt { description = "$(basename $dtname .dtb)"; - firmware = "uboot@1"; - loadables = "atf@1"; - fdt = "fdt@$cnt"; + firmware = "uboot_1"; + loadables = "atf_1"; + fdt = "fdt_$cnt"; }; __CONF_SECTION1_EOF fi