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 <Peter.Hoyes@arm.com>
> > 
> > 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 <Peter.Hoyes@arm.com>
> > ---
> >   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.

-- 
Tom