On Tue, Jul 04, 2023 at 11:39:59AM -0300, Fabio Estevam wrote:
> Hi Francesco,
>
> On Mon, Jul 3, 2023 at 5:49 PM Francesco Dolcini <france...@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@30800000/usb@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@30800000/usb@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().I'm a little leary of generic changes here having an unexpected size / performance impact. The API specifically does not "just" handle this case like it does for some others. We should update the docs around fdt_set_node_status and fdt_status_disabled to reference the return codes of fdt_setprop_string itself and check for anyone else that hasn't been considering this possible failure case. -- Tom
signature.asc
Description: PGP signature
