Hi Tim, On Mon, 7 Aug 2023 at 14:03, Tim Harvey <thar...@gateworks.com> wrote: > > On Sun, Jul 30, 2023 at 7:50 PM Simon Glass <s...@chromium.org> wrote: > > > > Hi Tim, > > > > On Sun, 30 Jul 2023 at 17:42, Tim Harvey <thar...@gateworks.com> wrote: > > > > > > > > > On Wed, Jul 26, 2023, 5:55 PM Simon Glass <s...@chromium.org> wrote: > > >> > > >> Hi Tim, > > >> > > >> On Mon, 24 Jul 2023 at 08:53, Simon Glass <s...@chromium.org> wrote: > > >> > > > >> > Hi, > > >> > > > >> > On Sun, 23 Jul 2023 at 03:00, Marcel Ziswiler > > >> > <marcel.ziswi...@toradex.com> wrote: > > >> > > > > >> > > Hi Simon > > >> > > > > >> > > On Jul 23, 2023 05:48, Simon Glass <s...@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