v2023.07-rc5 regression: Image overlaps SPL

[Fabio Estevam]

Hi Francesco,

On Mon, Jul 3, 2023 at 5:49 PM Francesco Dolcini <francesco at dolcini.it> wrote:

> If I do this small partial revert
>
> --- a/arch/arm/dts/imx7d-colibri-eval-v3-u-boot.dtsi
> +++ b/arch/arm/dts/imx7d-colibri-eval-v3-u-boot.dtsi
> @@ -15,7 +15,8 @@
>         pinctrl-0 = <&pinctrl_lcdif_dat
>                      &pinctrl_lcdif_ctrl>;
>         display = <&display0>;
> -       u-boot,dm-pre-reloc;
> +       bootph-all;

I managed to reproduce the behavior on a imx7d-sabresd by doing:

--- a/board/freescale/mx7dsabresd/mx7dsabresd.c
+++ b/board/freescale/mx7dsabresd/mx7dsabresd.c
@@ -25,6 +25,7 @@
 #include <i2c.h>
 #include <asm/mach-imx/mxc_i2c.h>
 #include <asm/arch/crm_regs.h>
+#include <fdt_support.h>

 DECLARE_GLOBAL_DATA_PTR;

@@ -289,6 +290,20 @@ int power_init_board(void)
 }
 #endif

+int board_fix_fdt(void *rw_fdt_blob)
+{
+       int ret;
+
+       int offset = fdt_path_offset(rw_fdt_blob,
"/soc/bus at 30800000/usb at 30b20000");
+
+       ret = fdt_status_disabled(rw_fdt_blob, offset);
+
+       printf("******** offset is 0x%x\n", offset);
+       printf("******** ret is %d\n", ret);
+
+       return 0;
+}
+
 int board_late_init(void)

--- a/configs/mx7dsabresd_defconfig
+++ b/configs/mx7dsabresd_defconfig
@@ -86,3 +86,4 @@ CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5
 CONFIG_CI_UDC=y
 CONFIG_USB_GADGET_DOWNLOAD=y
 CONFIG_ERRNO_STR=y
+CONFIG_OF_BOARD_FIXUP=y

With top of tree U-Boot it fails with:

******** offset is 0x7ba4
******** ret is -3

If u-boot.dtsi gets smaller, for example, by reverting 0aea5dda2928
then it succeeds:

******** offset is 0x7988
******** ret is 0

> fdt_status_disabled() returns 0 again.
>
> With the current master, fdt_status_disabled() returns -3,
> -FDT_ERR_NOSPACE, and I assume I could just change my code to call
> fdt_increase_size() and call it done.
>
> However, what the reason for this different behavior triggered by that
> change in the *-u-boot.dtsi ? Is this expected?
>
> From a quick check multiple place in the code just disable/enable nodes
> or set properties without taking care of this, are those going to be
> affected by that commit that created the regression? Are those all
> buggy?
>
> $ git grep fdt_setprop |wc -l
> 461
>
> We have some helper around, for example
> arch/arm/mach-imx/imx8/fdt.c:disable_fdt_node(), but this is for example
> just specific on that file.

Here is my suggestion: let's increase the fdt size locally on your
board for now, just like turris_omnia.c:

--- a/board/toradex/colibri_imx7/colibri_imx7.c
+++ b/board/toradex/colibri_imx7/colibri_imx7.c
@@ -315,6 +315,15 @@ int board_fix_fdt(void *rw_fdt_blob)
        if (is_cpu_type(MXC_CPU_MX7S)) {
                int offset = fdt_path_offset(rw_fdt_blob,
"/soc/bus at 30800000/usb at 30b20000");

+       /*
+        * We're either adding status = "disabled" property, or changing
+        * status = "okay" to status = "disabled". In both cases we'll need more
+        * space. Increase the size a little.
+        */
+       if (fdt_increase_size(rw_fdt_blob, 32) < 0) {
+               printf("Cannot increase FDT size.\n");
+               return -ENOMEM;
+       }
                return fdt_status_disabled(rw_fdt_blob, offset);
        }

Then for the next cycle, we should plan on adding this
fdt_increase_size() into the common fdt_status_disabled().

Does it work?