On Mon, May 23, 2022 at 09:25:55AM +0200, Neil Armstrong wrote: > Hi, > > On 19/05/2022 14:58, Peter Hoyes wrote: > > On 18/05/2022 19:15, Neil Armstrong wrote: > > > On 18/05/2022 17:52, Tom Rini wrote: > > > > On Wed, May 18, 2022 at 10:40:12AM +0200, Neil Armstrong wrote: > > > > > Hi, > > > > > > > > > > On 14/10/2021 10:40, Peter Hoyes wrote: > > > > > > From: Peter Hoyes > > > > > > > > > > > > If using OF_CONTROL, fdtcontroladdr is set to the fdt used to configure > > > > > > U-Boot. When using PXE, if no fdt is defined in the menu file, and > > > > > > there is no fdt at fdt_addr, add fall back on fdtcontroladdr too. > > > > > > > > > > > > We are developing board support for the Armv8r64 FVP using > > > > > > config_distro_bootcmd. We are also using OF_BOARD and would like the > > > > > > PXE boot option to default to the fdt provided by board_fdt_blob_setup. > > > > > > > > > > > > Signed-off-by: Peter Hoyes > > > > > > --- > > > > > >    cmd/pxe_utils.c | 8 +++++++- > > > > > >    1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c > > > > > > index 067c24e5ff..8f8e69ca97 100644 > > > > > > --- a/cmd/pxe_utils.c > > > > > > +++ b/cmd/pxe_utils.c > > > > > > @@ -556,7 +556,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label) > > > > > >         * Scenario 2: If there is an fdt_addr specified, pass it along to > > > > > >         * bootm, and adjust argc appropriately. > > > > > >         * > > > > > > -     * Scenario 3: fdt blob is not available. > > > > > > +     * Scenario 3: If there is an fdtcontroladdr specified, pass it along to > > > > > > +     * bootm, and adjust argc appropriately. > > > > > > +     * > > > > > > +     * Scenario 4: fdt blob is not available. > > > > > >         */ > > > > > >        bootm_argv[3] = env_get("fdt_addr_r"); > > > > > > @@ -646,6 +649,9 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label) > > > > > >        if (!bootm_argv[3]) > > > > > >            bootm_argv[3] = env_get("fdt_addr"); > > > > > > +    if (!bootm_argv[3]) > > > > > > +        bootm_argv[3] = env_get("fdtcontroladdr"); > > > > > > + > > > > > >        if (bootm_argv[3]) { > > > > > >            if (!bootm_argv[2]) > > > > > >                bootm_argv[2] = "-"; > > > > > > > > > > This change makes a regression when using a FIT image as kernel image within an extlinux.conf > > > > > > > > > > Before this change, when a DT wasn't specified, the DT in the FIT image was selected, > > > > > not the u-boot DT is selected. > > > > > > > > > > While it should work because the U-Boot DT should work in Linux, it's not always the case. > > > > > > > > > > Before: > > > > > Found /extlinux/extlinux.conf > > > > > Retrieving file: /extlinux/extlinux.conf > > > > > 1:    Yocto > > > > > Retrieving file: /fitImage > > > > > append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200 > > > > > ## Loading kernel from FIT Image at 08080000 ... > > > > >     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration > > > > >     Trying 'kernel-1' kernel subimage > > > > >       Description:  Linux kernel > > > > >       Type:         Kernel Image > > > > >       Compression:  gzip compressed > > > > >       Data Start:   0x08080120 > > > > >       Data Size:    9926956 Bytes = 9.5 MiB > > > > >       Architecture: AArch64 > > > > >       OS:           Linux > > > > >       Load Address: 0x01080000 > > > > >       Entry Point:  0x01080000 > > > > >       Hash algo:    sha256 > > > > >       Hash value: 5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e > > > > >     Verifying Hash Integrity ... sha256+ OK > > > > > ## Loading fdt from FIT Image at 08080000 ... > > > > >     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration > > > > >     Trying 'fdt-amlogic_meson-gxl-s905x-libretech-cc.dtb' fdt subimage > > > > >       Description:  Flattened Device Tree blob > > > > >       Type:         Flat Device Tree > > > > >       Compression:  uncompressed > > > > >       Data Start:   0x089f7b78 > > > > >       Data Size:    29092 Bytes = 28.4 KiB > > > > >       Architecture: AArch64 > > > > >       Hash algo:    sha256 > > > > >       Hash value: 72e5e4fcbb4aa59042377720e5636132ba790d85b6c3f6442446acc63f48cf67 > > > > >     Verifying Hash Integrity ... sha256+ OK > > > > >     Booting using the fdt blob at 0x89f7b78 > > > > >     Uncompressing Kernel Image > > > > >     Loading Device Tree to 000000007bf2f000, end 000000007bf391a3 ... OK > > > > > > > > > > > > > > > After: > > > > > Found /extlinux/extlinux.conf > > > > > Retrieving file: /extlinux/extlinux.conf > > > > > 1:    Yocto > > > > > Retrieving file: /fitImage > > > > > append: root=PARTUUID=fd26d57f-02 rootwait console=ttyAML0,115200 > > > > > ## Loading kernel from FIT Image at 08080000 ... > > > > >     Using 'conf-amlogic_meson-gxl-s905x-libretech-cc.dtb' configuration > > > > >     Trying 'kernel-1' kernel subimage > > > > >       Description:  Linux kernel > > > > >       Type:         Kernel Image > > > > >       Compression:  gzip compressed > > > > >       Data Start:   0x08080120 > > > > >       Data Size:    9926956 Bytes = 9.5 MiB > > > > >       Architecture: AArch64 > > > > >       OS:           Linux > > > > >       Load Address: 0x01080000 > > > > >       Entry Point:  0x01080000 > > > > >       Hash algo:    sha256 > > > > >       Hash value: 5181a76e4e7a728e24cd8569f8e48c543ac259bf4d66591a3dc5e166d709429e > > > > >     Verifying Hash Integrity ... sha256+ OK > > > > > ## Flattened Device Tree blob at 7bf40210 > > > > >     Booting using the fdt blob at 0x7bf40210 > > > > >     Uncompressing Kernel Image > > > > >     Loading Device Tree to 000000007bf2c000, end 000000007bf38ba7 ... OK > > > > > > > > > > > > > > > Note this is the "best in town" boot method in Yocto/OE when using WIC, FIT Image and > > > > > generating extlinux.conf from the WIC "bootimg-partition" plugin. > > > > > > > > > > So this change will regress all platforms using this scheme. > > > > > > > > > > In case we only want the FIT image and not DTB, the fdtdir is ommited at: > > > > > https://github.com/openembedded/openembedded-core/blob/9015dec93233c7d45fd0c9885ff5d4ec23ad377d/scripts/lib/wic/plugins/source/bootimg-partition.py#L150 > > > > > > > > Well this is certainly tricky.  Do we be "boring" and say that the > > > > built-in device tree should be used, or "boring" and say that the distro > > > > provided device tree should be used.  I assume that in the OE case we're > > > > not always going to be booting U-Boot that was built, so updating the > > > > logic there to have the correct dtb isn't right either. > > > > > > > > > > I'm not against this logic, it should definitely be aligned on EFI and permit > > > using built-in DT, but the logic breaks fitImage loading via extlinux, because > > > now we have no way to say "use the fitImage DT". > > > > > > This change should have been elsewhere, like in the non-FIT bootm path when > > > no DT was supplied and DT is mandatory for the platform. > > > > > > Or by passing a keyword like fdt-built-in or ftdfile=builtin. > > > > I agree the correct logic should be: > > > >  * Use the extlinux.conf fdt file, if defined > >  * Use a FIT fdt, if available > >  * Use fdtcontroladdr, as a last resort > > > > , so FIT images was an oversight. > > > > > > > > ==><======================================================= > > > diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c > > > index 0c24becae3..22040c2340 100644 > > > --- a/boot/pxe_utils.c > > > +++ b/boot/pxe_utils.c > > > @@ -682,7 +682,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label) > > >                         fdtfile = fdtfilefree; > > >                 } > > > > > > -               if (fdtfile) { > > > +               if (strcmp(fdtfile, "builtin") && fdtfile) { > > >                         int err = get_relfile_envaddr(ctx, fdtfile, > > > "fdt_addr_r", NULL); > > > > > > @@ -724,7 +724,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label) > > >         if (!bootm_argv[3]) > > >                 bootm_argv[3] = env_get("fdt_addr"); > > > > > > -       if (!bootm_argv[3]) > > > +       if (!strcmp(fdtfile, "builtin")) > > >                 bootm_argv[3] = env_get("fdtcontroladdr"); > > > > > >         if (bootm_argv[3]) { > > > ==><======================================================= > > > > > > Or by checking the image format before using fdtcontroladdr like: > > > > > > ==><======================================================= > > > diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c > > > index 0c24becae3..f4f6369de6 100644 > > > --- a/boot/pxe_utils.c > > > +++ b/boot/pxe_utils.c > > > @@ -724,7 +724,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label) > > >         if (!bootm_argv[3]) > > >                 bootm_argv[3] = env_get("fdt_addr"); > > > > > > -       if (!bootm_argv[3]) > > > +       if (genimg_get_format(buf) != IMAGE_FORMAT_FIT && !bootm_argv[3]) > > >                 bootm_argv[3] = env_get("fdtcontroladdr"); > > > > > >         if (bootm_argv[3]) { > > > ==><======================================================= > > > > > > Neil > > > > Your second diff above seems like the least invasive way of achieving this behavior. > > > > I did wonder whether it would make sense to add a global 'fdtcontroladdr' fallback for bootm (i.e. in boot_get_fdt in image-fdt.c), so that: > > > >    bootm $kernel_addr_r > > > > falls back to $fdtcontroladdr (if valid) as a last resort. Then we could remove the branch in pxe_utils, and the FIT logic would happen in the right order. It looks like bootefi already does something similar, but I guess modifying the bootm behavior would affect a lot of existing boards. > > Probably, but I think getting back fit image working would be the priority. > > Tom, should I send the last change ? Yes please, thanks! -- Tom