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 francesco@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.