Strange construct in binman description
Tim Harvey
tharvey at gateworks.com
Wed Aug 9 02:19:24 CEST 2023
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?
Best regards,
Tim
More information about the U-Boot
mailing list