Hi Tim,

On Mon, 24 Jul 2023 at 08:53, Simon Glass sjg@chromium.org wrote:

Hi,

On Sun, 23 Jul 2023 at 03:00, Marcel Ziswiler marcel.ziswiler@toradex.com wrote:

...

Hi Simon

On Jul 23, 2023 05:48, Simon Glass sjg@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?

Regards, Simon

Hi Tim,

On Sun, 30 Jul 2023 at 17:42, Tim Harvey tharvey@gateworks.com wrote:

On Wed, Jul 26, 2023, 5:55 PM Simon Glass sjg@chromium.org wrote:

...

Hi Tim,

On Mon, 24 Jul 2023 at 08:53, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Sun, 23 Jul 2023 at 03:00, Marcel Ziswiler marcel.ziswiler@toradex.com wrote:

...

Hi Simon

On Jul 23, 2023 05:48, Simon Glass sjg@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.

Regards, Simon

Hi Tim,

On Mon, 7 Aug 2023 at 14:03, Tim Harvey tharvey@gateworks.com wrote:

On Sun, Jul 30, 2023 at 7:50 PM Simon Glass sjg@chromium.org wrote:

...

Hi Tim,

On Sun, 30 Jul 2023 at 17:42, Tim Harvey tharvey@gateworks.com wrote:

...

On Wed, Jul 26, 2023, 5:55 PM Simon Glass sjg@chromium.org wrote:

...

Hi Tim,

On Mon, 24 Jul 2023 at 08:53, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Sun, 23 Jul 2023 at 03:00, Marcel Ziswiler marcel.ziswiler@toradex.com wrote:

...

Hi Simon

On Jul 23, 2023 05:48, Simon Glass sjg@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?

Regards, Simon

Hi Tim,

On Tue, 8 Aug 2023 at 18:19, Tim Harvey tharvey@gateworks.com wrote:

On Tue, Aug 8, 2023 at 10:54 AM Simon Glass sjg@chromium.org wrote:

...

Hi Tim,

On Mon, 7 Aug 2023 at 14:03, Tim Harvey tharvey@gateworks.com wrote:

...

On Sun, Jul 30, 2023 at 7:50 PM Simon Glass sjg@chromium.org wrote:

...

Hi Tim,

On Sun, 30 Jul 2023 at 17:42, Tim Harvey tharvey@gateworks.com wrote:

...

On Wed, Jul 26, 2023, 5:55 PM Simon Glass sjg@chromium.org wrote:

...

Hi Tim,

On Mon, 24 Jul 2023 at 08:53, Simon Glass sjg@chromium.org wrote: > > Hi, > > On Sun, 23 Jul 2023 at 03:00, Marcel Ziswiler > marcel.ziswiler@toradex.com wrote: > > > > Hi Simon > > > > On Jul 23, 2023 05:48, Simon Glass sjg@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