Strange construct in binman description
Simon Glass
sjg at chromium.org
Fri Aug 11 00:17:40 CEST 2023
Hi Tim,
On Tue, 8 Aug 2023 at 18:19, Tim Harvey <tharvey at gateworks.com> wrote:
>
> On Tue, Aug 8, 2023 at 10:54 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Tim,
> >
> > On Mon, 7 Aug 2023 at 14:03, Tim Harvey <tharvey at gateworks.com> wrote:
> > >
> > > On Sun, Jul 30, 2023 at 7:50 PM Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Tim,
> > > >
> > > > On Sun, 30 Jul 2023 at 17:42, Tim Harvey <tharvey at gateworks.com> wrote:
> > > > >
> > > > >
> > > > > On Wed, Jul 26, 2023, 5:55 PM Simon Glass <sjg at chromium.org> wrote:
> > > > >>
> > > > >> Hi Tim,
> > > > >>
> > > > >> On Mon, 24 Jul 2023 at 08:53, Simon Glass <sjg at chromium.org> wrote:
> > > > >> >
> > > > >> > Hi,
> > > > >> >
> > > > >> > On Sun, 23 Jul 2023 at 03:00, Marcel Ziswiler
> > > > >> > <marcel.ziswiler at toradex.com> wrote:
> > > > >> > >
> > > > >> > > Hi Simon
> > > > >> > >
> > > > >> > > On Jul 23, 2023 05:48, Simon Glass <sjg at chromium.org> wrote:
> > > > >> > >
> > > > >> > > Hi Marcel,
> > > > >> > >
> > > > >> > > I just noticed this in an imx8 description:
> > > > >> > >
> > > > >> > > binman_configuration: @config-SEQ {
> > > > >> > >
> > > > >> > >
> > > > >> > > I remember having stumbled over this before but I do not remember any exact details, sorry.
> > > > >> > >
> > > > >> > > Since this is a generator node, binman blindly generates a phandle for
> > > > >> > > each not it generates. This means that if there is more than one
> > > > >> > > config node, then they will have duplicate phandles.
> > > > >> > >
> > > > >> > > I have sent a series to correct this, but it fails the build on:
> > > > >> > > imx8mm_venice imx8mn_venice imx8mp_venice
> > > > >> > >
> > > > >> > >
> > > > >> > > Ah, now I get it. You mixed up venice (Gate works) with verdin (Toradex).
> > > > >> > >
> > > > >> > > I am a bit unsure about what this is supposed to do. Could you please
> > > > >> > > take a look?
> > > > >> > >
> > > > >> > >
> > > > >> > > I'm more than happy to do that but you are probably much better off enquiring Tim Harvey from Gateworks about this. I put him on CC.
> > > > >> >
> > > > >> > Tim, any thoughts on this please?
> > > > >>
> > > > >> Can you please weigh in on this one?
> > > > >
> > > > >
> > > > > Simon,
> > > > >
> > > > > I'm away from a computer until next week.
> > > > >
> > > > > Can you point me to the series in reference here?
> > > > >
> > > > > The -seq is for configs that support multiple dtbs which the Venice configs use.
> > > >
> > > > This is referring to code in mainline. it seems to have a phandle in a
> > > > FIT template node, which means that Binman will generate multiple
> > > > nodes with duplicate phandles.
> > > >
> > >
> > > Simon,
> > >
> > > I don't know where the original discussion on this thread is from but
> > > what I found is that your commit d4d97661d255: ("binman: Support
> > > templates containing phandles") broke imx8mm_venice_defconfig,
> > > imx8mn_venice_defconfig, and imx8mp_venice_defconfig. I see that you
> > > merged commit 288ae53cb736: ("binman: Add a temporary hack for
> > > duplicate phandles") while I was away as a temporary workaround for
> > > these three boards instead of reverting the patch so I suppose we need
> > > to figure out how to address it then remove your workaround.
> > >
> > > The imx8mm-u-boot.dtsi contains a configurations node in the FIT image
> > > to build configs for each dtb from CONFIG_OF_LIST.
> > >
> > > configurations {
> > > default = "@config-DEFAULT-SEQ";
> > >
> > > binman_configuration: @config-SEQ {
> > > description = "NAME";
> > > fdt = "fdt-SEQ";
> > > firmware = "uboot";
> > > #ifndef CONFIG_ARMV8_PSCI
> > > loadables = "atf";
> > > #endif
> > > };
> > > };
> > >
> > > These three boards are the only imx8m* boards that support multiple
> > > dtb's. If I look at the fit image (dtc -I dtb -O dts itb.fit.fit -f) I
> > > see the following for imx8mm_venice_defconfig:
> > >
> > > configurations {
> > > default = "config-1";
> > >
> > > config-1 {
> > > description = "imx8mm-venice";
> > > fdt = "fdt-1";
> > > firmware = "uboot";
> > > loadables = "atf";
> > > phandle = <0x7f>;
> > > };
> > >
> > > config-2 {
> > > description = "imx8mm-venice-gw71xx-0x";
> > > fdt = "fdt-2";
> > > firmware = "uboot";
> > > loadables = "atf";
> > > phandle = <0x7f>;
> > > };
> > >
> > > config-3 {
> > > description = "imx8mm-venice-gw72xx-0x";
> > > fdt = "fdt-3";
> > > firmware = "uboot";
> > > loadables = "atf";
> > > phandle = <0x7f>;
> > > };
> > >
> > > config-4 {
> > > description = "imx8mm-venice-gw73xx-0x";
> > > fdt = "fdt-4";
> > > firmware = "uboot";
> > > loadables = "atf";
> > > phandle = <0x7f>;
> > > };
> > >
> > > config-5 {
> > > description = "imx8mm-venice-gw7901";
> > > fdt = "fdt-5";
> > > firmware = "uboot";
> > > loadables = "atf";
> > > phandle = <0x7f>;
> > > };
> > >
> > > config-6 {
> > > description = "imx8mm-venice-gw7902";
> > > fdt = "fdt-6";
> > > firmware = "uboot";
> > > loadables = "atf";
> > > phandle = <0x7f>;
> > > };
> > >
> > > config-7 {
> > > description = "imx8mm-venice-gw7903";
> > > fdt = "fdt-7";
> > > firmware = "uboot";
> > > loadables = "atf";
> > > phandle = <0x7f>;
> > > };
> > > };
> > > };
> > >
> > > So yeah... it doesn't make sense that each config has the same phandle
> > > value but this tells me that U-Boot must not use these phandles as
> > > this has never presented a problem before. The venice
> > > board_fit_config_name_match() function gets called with each dtb to
> > > choose which dtb and that works fine. Does U-Boot even use the
> > > configurations node? Perhaps it uses the configurations node but
> > > doesn't use the phandles?
> >
> > U-Boot doesn't use them. I think it is best to remove the phandle from
> > node. Could you please send a patch to do that and a revert of my
> > work-around?
> >
>
> Simon,
>
> The phandle is not specified in the configurations node in
> imx8mm-u-boot.dtsi so it gets added automatically somewhere by binman.
> Where does it get automatically added and why is it only getting added
> for nodes under configurations?
Scotch mist?
arch/arm/dts/imx8mp-u-boot.dtsi:
@fdt-SEQ {
description = "NAME";
type = "flat_dt";
compression = "none";
uboot_fdt_blob: blob-ext {
^^ phandle
filename = "u-boot.dtb";
};
};
If there is no mysterious need for this then I think it should be removed.
Regards,
Simon
More information about the U-Boot
mailing list