[PATCH v2 0/3] fdt: Fix mtparts fixup

[PATCH v2 0/3] fdt: Fix mtparts fixup

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Recently we had a boot regression on colibri-imx7 because of a cleanup change
on Linux imx7.dtsi setting nand controller node #size-cells from 1 to 0.

Because of that Linux partition parser was no longer able to properly
parse the OF partitions leading to a boot failure, the above change was
reverted in the meantime as an immediate workaround, but some improvement
is required on both Linux and U-Boot.

This change improve the U-Boot part of it, #size-cell is set to 1 when
it has an invalid value. This has the limitation to work only with devices
smaller than 4GiB. In general the suggestion from the Linux MTD maintainer would
be to not edit the DT partitions from U-Boot unless some dynamic tweaking is
required.

This series therefore convert colibri-imx6ull and colibri-imx7 to pass the
partition list from the command line instead of fixing up the DT.

Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/

v2:
 - removed dead code from colibri imx7/imx6ull
 - rewrote and clarify commit messages

Francesco Dolcini (3):
  fdt: validate/fix cells count on mtdpart fixup
  colibri-imx7: specify MTD partitions on command line
  colibri-imx6ull: specify MTD partitions on command line

 .../toradex/colibri-imx6ull/colibri-imx6ull.c | 11 -----
 board/toradex/colibri_imx7/colibri_imx7.c     | 10 -----
 common/fdt_support.c                          | 45 ++++++++++++++-----
 configs/colibri-imx6ull_defconfig             |  1 -
 configs/colibri_imx7_defconfig                |  1 -
 include/configs/colibri-imx6ull.h             |  2 +-
 include/configs/colibri_imx7.h                |  2 +-
 7 files changed, 37 insertions(+), 35 deletions(-)

-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 0/3] fdt: Fix mtparts fixup

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Recently we had a boot regression on colibri-imx7 because of a cleanup change
on Linux imx7.dtsi setting nand controller node #size-cells from 1 to 0.

Because of that Linux partition parser was no longer able to properly
parse the OF partitions leading to a boot failure, the above change was
reverted in the meantime as an immediate workaround, but some improvement
is required on both Linux and U-Boot.

This change improve the U-Boot part of it, #size-cell is set to 1 when
it has an invalid value. This has the limitation to work only with devices
smaller than 4GiB. In general the suggestion from the Linux MTD maintainer would
be to not edit the DT partitions from U-Boot unless some dynamic tweaking is
required.

This series therefore convert colibri-imx6ull and colibri-imx7 to pass the
partition list from the command line instead of fixing up the DT.

Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/

v2:
 - removed dead code from colibri imx7/imx6ull
 - rewrote and clarify commit messages

Francesco Dolcini (3):
  fdt: validate/fix cells count on mtdpart fixup
  colibri-imx7: specify MTD partitions on command line
  colibri-imx6ull: specify MTD partitions on command line

 .../toradex/colibri-imx6ull/colibri-imx6ull.c | 11 -----
 board/toradex/colibri_imx7/colibri_imx7.c     | 10 -----
 common/fdt_support.c                          | 45 ++++++++++++++-----
 configs/colibri-imx6ull_defconfig             |  1 -
 configs/colibri_imx7_defconfig                |  1 -
 include/configs/colibri-imx6ull.h             |  2 +-
 include/configs/colibri_imx7.h                |  2 +-
 7 files changed, 37 insertions(+), 35 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] fdt: validate/fix cells count on mtdpart fixup

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Fixup #size-cells value when updating the MTD partitions, this is
required to prevent issues in case the MTD parent set #size-cells to
zero.
This could happen for example in the legacy case in which the partitions
are created as direct child of the mtd node and that specific node has
no children. Recent clean-up on Linux device tree files created a boot
regression on colibri-imx7 [0].

This fixup has the limitation to assume 32-bit (#size-cells=1)
addressing, therefore it will not work with device bigger than 4GiB.

This change also enforce #address-cells to be the same as #size-cells,
this was already silently enforced by fdt_node_set_part_info(), now this
is checked explicitly and partition fixup will just fail in such case.

When the partition list is static the preferred way to pass the mtd
partition list to the kernel is to either define those in the source DTS
file or use mtdparts= in the command line.
Tweaking the DT from U-Boot should be avoided, unless some dynamic
changes are required, since it proved to be problematic when not
following the evolution of the "standard".

Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
Cc: Marek Vasut <marex@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
v2: improved commit message
---
 common/fdt_support.c | 45 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index dbceec6f2dcc..3aee826e60cf 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -877,27 +877,20 @@ static int fdt_del_partitions(void *blob, int parent_offset)
 	return 0;
 }
 
-static int fdt_node_set_part_info(void *blob, int parent_offset,
+/* This expects #address-cells and #size-cells to have same value */
+static int fdt_node_set_part_info(void *blob, int parent_offset, int sizecell,
 				  struct mtd_device *dev)
 {
 	struct list_head *pentry;
 	struct part_info *part;
 	int off, ndepth = 0;
 	int part_num, ret;
-	int sizecell;
 	char buf[64];
 
 	ret = fdt_del_partitions(blob, parent_offset);
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * Check if size/address is 1 or 2 cells.
-	 * We assume #address-cells and #size-cells have same value.
-	 */
-	sizecell = fdt_getprop_u32_default_node(blob, parent_offset,
-						0, "#size-cells", 1);
-
 	/*
 	 * Check if it is nand {}; subnode, adjust
 	 * the offset in this case
@@ -992,6 +985,31 @@ err_prop:
 	return ret;
 }
 
+static int fdt_mtdparts_cell_cnt(void *fdt, int off)
+{
+	int sizecell, addrcell;
+
+	sizecell = fdt_getprop_u32_default_node(fdt, off, 0, "#size-cells", 0);
+	if (sizecell != 1 && sizecell != 2) {
+		printf("%s: Invalid or missing #size-cells %d value, assuming 1\n",
+		       __func__, sizecell);
+
+		sizecell = 1;
+		if (fdt_setprop_u32(fdt, off, "#size-cells", sizecell))
+			return -1;
+	}
+
+	addrcell = fdt_getprop_u32_default_node(fdt, off, 0,
+						"#address-cells", 0);
+	if (addrcell != sizecell) {
+		printf("%s: Invalid #address-cells %d != #size-cells %d, aborting\n",
+		       __func__, addrcell, sizecell);
+		return -1;
+	}
+
+	return sizecell;
+}
+
 /*
  * Update partitions in nor/nand nodes using info from
  * mtdparts environment variable. The nodes to update are
@@ -1037,12 +1055,19 @@ void fdt_fixup_mtdparts(void *blob, const struct node_info *node_info,
 
 			dev = device_find(node_info[i].type, idx++);
 			if (dev) {
+				int cell;
+
 				parts = fdt_subnode_offset(blob, noff,
 							   "partitions");
 				if (parts < 0)
 					parts = noff;
 
-				if (fdt_node_set_part_info(blob, parts, dev))
+				cell = fdt_mtdparts_cell_cnt(blob, parts);
+				if (cell < 0)
+					return;
+
+				if (fdt_node_set_part_info(blob, parts,
+							   cell, dev))
 					return; /* return on error */
 			}
 		}
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 1/3] fdt: validate/fix cells count on mtdpart fixup

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Fixup #size-cells value when updating the MTD partitions, this is
required to prevent issues in case the MTD parent set #size-cells to
zero.
This could happen for example in the legacy case in which the partitions
are created as direct child of the mtd node and that specific node has
no children. Recent clean-up on Linux device tree files created a boot
regression on colibri-imx7 [0].

This fixup has the limitation to assume 32-bit (#size-cells=1)
addressing, therefore it will not work with device bigger than 4GiB.

This change also enforce #address-cells to be the same as #size-cells,
this was already silently enforced by fdt_node_set_part_info(), now this
is checked explicitly and partition fixup will just fail in such case.

When the partition list is static the preferred way to pass the mtd
partition list to the kernel is to either define those in the source DTS
file or use mtdparts= in the command line.
Tweaking the DT from U-Boot should be avoided, unless some dynamic
changes are required, since it proved to be problematic when not
following the evolution of the "standard".

Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
Cc: Marek Vasut <marex@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
v2: improved commit message
---
 common/fdt_support.c | 45 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index dbceec6f2dcc..3aee826e60cf 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -877,27 +877,20 @@ static int fdt_del_partitions(void *blob, int parent_offset)
 	return 0;
 }
 
-static int fdt_node_set_part_info(void *blob, int parent_offset,
+/* This expects #address-cells and #size-cells to have same value */
+static int fdt_node_set_part_info(void *blob, int parent_offset, int sizecell,
 				  struct mtd_device *dev)
 {
 	struct list_head *pentry;
 	struct part_info *part;
 	int off, ndepth = 0;
 	int part_num, ret;
-	int sizecell;
 	char buf[64];
 
 	ret = fdt_del_partitions(blob, parent_offset);
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * Check if size/address is 1 or 2 cells.
-	 * We assume #address-cells and #size-cells have same value.
-	 */
-	sizecell = fdt_getprop_u32_default_node(blob, parent_offset,
-						0, "#size-cells", 1);
-
 	/*
 	 * Check if it is nand {}; subnode, adjust
 	 * the offset in this case
@@ -992,6 +985,31 @@ err_prop:
 	return ret;
 }
 
+static int fdt_mtdparts_cell_cnt(void *fdt, int off)
+{
+	int sizecell, addrcell;
+
+	sizecell = fdt_getprop_u32_default_node(fdt, off, 0, "#size-cells", 0);
+	if (sizecell != 1 && sizecell != 2) {
+		printf("%s: Invalid or missing #size-cells %d value, assuming 1\n",
+		       __func__, sizecell);
+
+		sizecell = 1;
+		if (fdt_setprop_u32(fdt, off, "#size-cells", sizecell))
+			return -1;
+	}
+
+	addrcell = fdt_getprop_u32_default_node(fdt, off, 0,
+						"#address-cells", 0);
+	if (addrcell != sizecell) {
+		printf("%s: Invalid #address-cells %d != #size-cells %d, aborting\n",
+		       __func__, addrcell, sizecell);
+		return -1;
+	}
+
+	return sizecell;
+}
+
 /*
  * Update partitions in nor/nand nodes using info from
  * mtdparts environment variable. The nodes to update are
@@ -1037,12 +1055,19 @@ void fdt_fixup_mtdparts(void *blob, const struct node_info *node_info,
 
 			dev = device_find(node_info[i].type, idx++);
 			if (dev) {
+				int cell;
+
 				parts = fdt_subnode_offset(blob, noff,
 							   "partitions");
 				if (parts < 0)
 					parts = noff;
 
-				if (fdt_node_set_part_info(blob, parts, dev))
+				cell = fdt_mtdparts_cell_cnt(blob, parts);
+				if (cell < 0)
+					return;
+
+				if (fdt_node_set_part_info(blob, parts,
+							   cell, dev))
 					return; /* return on error */
 			}
 		}
-- 
2.25.1

[PATCH v2 2/3] colibri-imx7: specify MTD partitions on command line

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Disable fdt_fixup_mtdparts() and pass MTD partition on the command
line. Dynamically editing the fdt with a static partitions configuration
is not required and there is no advantages compared to using the command
line. This change should prevent boot failures as the one in [0].

Cc: Marek Vasut <marex@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/ [0]
Link: https://lore.kernel.org/all/20230105123334.7f90c289@xps-13/
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
v2:
 - improved commit message
 - removed dead code
---
 board/toradex/colibri_imx7/colibri_imx7.c | 10 ----------
 configs/colibri_imx7_defconfig            |  1 -
 include/configs/colibri_imx7.h            |  2 +-
 3 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/board/toradex/colibri_imx7/colibri_imx7.c b/board/toradex/colibri_imx7/colibri_imx7.c
index 6ce4fa376ac0..3e79ab93a982 100644
--- a/board/toradex/colibri_imx7/colibri_imx7.c
+++ b/board/toradex/colibri_imx7/colibri_imx7.c
@@ -303,16 +303,6 @@ int ft_board_setup(void *blob, struct bd_info *bd)
 			fdt_status_disabled(blob, off);
 	}
 #endif
-#if defined(CONFIG_FDT_FIXUP_PARTITIONS)
-	static const struct node_info nodes[] = {
-		{ "fsl,imx7d-gpmi-nand", MTD_DEV_TYPE_NAND, }, /* NAND flash */
-		{ "fsl,imx6q-gpmi-nand", MTD_DEV_TYPE_NAND, },
-	};
-
-	/* Update partition nodes using info from mtdparts env var */
-	puts("   Updating MTD partitions...\n");
-	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
-#endif
 
 	return ft_common_board_setup(blob, bd);
 }
diff --git a/configs/colibri_imx7_defconfig b/configs/colibri_imx7_defconfig
index 7e24dcd40090..5cc22a481dd9 100644
--- a/configs/colibri_imx7_defconfig
+++ b/configs/colibri_imx7_defconfig
@@ -101,4 +101,3 @@ CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
 CONFIG_CI_UDC=y
 CONFIG_USB_GADGET_DOWNLOAD=y
 CONFIG_OF_LIBFDT_OVERLAY=y
-CONFIG_FDT_FIXUP_PARTITIONS=y
diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h
index c568643977ca..03f8ed14787a 100644
--- a/include/configs/colibri_imx7.h
+++ b/include/configs/colibri_imx7.h
@@ -141,7 +141,7 @@
 		"${board}/flash_blk.img && source ${loadaddr}\0" \
 	"setup=setenv setupargs " \
 		"console=tty1 console=${console}" \
-		",${baudrate}n8 ${memargs} consoleblank=0\0" \
+		",${baudrate}n8 ${memargs} ${mtdparts} consoleblank=0\0" \
 	"setupdate=run setsdupdate || run setusbupdate || run setethupdate\0" \
 	"setusbupdate=usb start && setenv interface usb && " \
 		"fatload ${interface} 0:1 ${loadaddr} " \
-- 
2.25.1

[PATCH v2 3/3] colibri-imx6ull: specify MTD partitions on command line

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Disable fdt_fixup_mtdparts() and pass MTD partition on the command
line. Dynamically editing the fdt with a static partitions configuration
is not required and there is no advantages compared to using the command
line. This change should prevent boot failures as the one in [0].

Cc: Marek Vasut <marex@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/ [0]
Link: https://lore.kernel.org/all/20230105123334.7f90c289@xps-13/
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
v2:
 - removed dead code
 - improved commit message
---
 board/toradex/colibri-imx6ull/colibri-imx6ull.c | 11 -----------
 configs/colibri-imx6ull_defconfig               |  1 -
 include/configs/colibri-imx6ull.h               |  2 +-
 3 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/board/toradex/colibri-imx6ull/colibri-imx6ull.c b/board/toradex/colibri-imx6ull/colibri-imx6ull.c
index 6007f110e4ba..48fdb1e09712 100644
--- a/board/toradex/colibri-imx6ull/colibri-imx6ull.c
+++ b/board/toradex/colibri-imx6ull/colibri-imx6ull.c
@@ -212,17 +212,6 @@ int checkboard(void)
 #if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP)
 int ft_board_setup(void *blob, struct bd_info *bd)
 {
-#if defined(CONFIG_FDT_FIXUP_PARTITIONS)
-	static struct node_info nodes[] = {
-		{ "fsl,imx6ull-gpmi-nand", MTD_DEV_TYPE_NAND, },
-		{ "fsl,imx6q-gpmi-nand", MTD_DEV_TYPE_NAND, },
-	};
-
-	/* Update partition nodes using info from mtdparts env var */
-	puts("   Updating MTD partitions...\n");
-	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
-#endif
-
 	return ft_common_board_setup(blob, bd);
 }
 #endif
diff --git a/configs/colibri-imx6ull_defconfig b/configs/colibri-imx6ull_defconfig
index f2a0d79ccca9..fbab687f5ab8 100644
--- a/configs/colibri-imx6ull_defconfig
+++ b/configs/colibri-imx6ull_defconfig
@@ -104,4 +104,3 @@ CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
 CONFIG_CI_UDC=y
 CONFIG_USB_GADGET_DOWNLOAD=y
 CONFIG_OF_LIBFDT_OVERLAY=y
-CONFIG_FDT_FIXUP_PARTITIONS=y
diff --git a/include/configs/colibri-imx6ull.h b/include/configs/colibri-imx6ull.h
index ba45ee4efd37..561a61ebc03c 100644
--- a/include/configs/colibri-imx6ull.h
+++ b/include/configs/colibri-imx6ull.h
@@ -99,7 +99,7 @@
 		"${board}/flash_blk.img && source ${loadaddr}\0" \
 	"setup=setenv setupargs " \
 		"console=tty1 console=${console}" \
-		",${baudrate}n8 ${memargs} consoleblank=0\0" \
+		",${baudrate}n8 ${memargs} ${mtdparts} consoleblank=0\0" \
 	"setupdate=run setsdupdate || run setusbupdate || run setethupdate\0" \
 	"setusbupdate=usb start && setenv interface usb && " \
 		"fatload ${interface} 0:1 ${loadaddr} " \
-- 
2.25.1

Re: [PATCH v2 1/3] fdt: validate/fix cells count on mtdpart fixup

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2426 bytes --]

On Mon, Feb 06, 2023 at 11:48:36PM +0100, Francesco Dolcini wrote:

> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Fixup #size-cells value when updating the MTD partitions, this is
> required to prevent issues in case the MTD parent set #size-cells to
> zero.
> This could happen for example in the legacy case in which the partitions
> are created as direct child of the mtd node and that specific node has
> no children. Recent clean-up on Linux device tree files created a boot
> regression on colibri-imx7 [0].
> 
> This fixup has the limitation to assume 32-bit (#size-cells=1)
> addressing, therefore it will not work with device bigger than 4GiB.
> 
> This change also enforce #address-cells to be the same as #size-cells,
> this was already silently enforced by fdt_node_set_part_info(), now this
> is checked explicitly and partition fixup will just fail in such case.
> 
> When the partition list is static the preferred way to pass the mtd
> partition list to the kernel is to either define those in the source DTS
> file or use mtdparts= in the command line.
> Tweaking the DT from U-Boot should be avoided, unless some dynamic
> changes are required, since it proved to be problematic when not
> following the evolution of the "standard".
> 
> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> Cc: Marek Vasut <marex@denx.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> v2: improved commit message
> ---
>  common/fdt_support.c | 45 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 10 deletions(-)

I'm dropping the linux-mtd list here and adding a bunch more platform
maintainers. In general, calling fdt_fixup_mtdparts is the wrong choice.
I see we do have a few cases in U-Boot where we're clearly doing
something dynamic to the partition table, but it looks like at first
glance most callers are using this hook when they should either be
having the partition map in the device tree properly (and using one of
the appropriate bindings) or passing the map in via the kernel command
line. I would like to ask everyone I've added to the list here to
please audit your platform and update it as needed. Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/3] fdt: validate/fix cells count on mtdpart fixup

[Francesco Dolcini]

+Stefano

On Mon, Feb 06, 2023 at 06:17:31PM -0500, Tom Rini wrote:
> On Mon, Feb 06, 2023 at 11:48:36PM +0100, Francesco Dolcini wrote:
> 
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > Fixup #size-cells value when updating the MTD partitions, this is
> > required to prevent issues in case the MTD parent set #size-cells to
> > zero.
> > This could happen for example in the legacy case in which the partitions
> > are created as direct child of the mtd node and that specific node has
> > no children. Recent clean-up on Linux device tree files created a boot
> > regression on colibri-imx7 [0].
> > 
> > This fixup has the limitation to assume 32-bit (#size-cells=1)
> > addressing, therefore it will not work with device bigger than 4GiB.
> > 
> > This change also enforce #address-cells to be the same as #size-cells,
> > this was already silently enforced by fdt_node_set_part_info(), now this
> > is checked explicitly and partition fixup will just fail in such case.
> > 
> > When the partition list is static the preferred way to pass the mtd
> > partition list to the kernel is to either define those in the source DTS
> > file or use mtdparts= in the command line.
> > Tweaking the DT from U-Boot should be avoided, unless some dynamic
> > changes are required, since it proved to be problematic when not
> > following the evolution of the "standard".
> > 
> > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > ---
> > v2: improved commit message
> > ---
> >  common/fdt_support.c | 45 ++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 35 insertions(+), 10 deletions(-)
> 
> I'm dropping the linux-mtd list here and adding a bunch more platform
> maintainers. In general, calling fdt_fixup_mtdparts is the wrong choice.
> I see we do have a few cases in U-Boot where we're clearly doing
> something dynamic to the partition table, but it looks like at first
> glance most callers are using this hook when they should either be
> having the partition map in the device tree properly (and using one of
> the appropriate bindings) or passing the map in via the kernel command
> line. I would like to ask everyone I've added to the list here to
> please audit your platform and update it as needed. Thanks!

Hello Tom,
what should we do with this patch? No feedback so far, apart this email
from you.

Stefano: maybe you can pick patches 2 and 3 ?

Francesco

Re: [PATCH v2 1/3] fdt: validate/fix cells count on mtdpart fixup

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3022 bytes --]

On Tue, Apr 04, 2023 at 06:18:03PM +0200, Francesco Dolcini wrote:
> +Stefano
> 
> On Mon, Feb 06, 2023 at 06:17:31PM -0500, Tom Rini wrote:
> > On Mon, Feb 06, 2023 at 11:48:36PM +0100, Francesco Dolcini wrote:
> > 
> > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > 
> > > Fixup #size-cells value when updating the MTD partitions, this is
> > > required to prevent issues in case the MTD parent set #size-cells to
> > > zero.
> > > This could happen for example in the legacy case in which the partitions
> > > are created as direct child of the mtd node and that specific node has
> > > no children. Recent clean-up on Linux device tree files created a boot
> > > regression on colibri-imx7 [0].
> > > 
> > > This fixup has the limitation to assume 32-bit (#size-cells=1)
> > > addressing, therefore it will not work with device bigger than 4GiB.
> > > 
> > > This change also enforce #address-cells to be the same as #size-cells,
> > > this was already silently enforced by fdt_node_set_part_info(), now this
> > > is checked explicitly and partition fixup will just fail in such case.
> > > 
> > > When the partition list is static the preferred way to pass the mtd
> > > partition list to the kernel is to either define those in the source DTS
> > > file or use mtdparts= in the command line.
> > > Tweaking the DT from U-Boot should be avoided, unless some dynamic
> > > changes are required, since it proved to be problematic when not
> > > following the evolution of the "standard".
> > > 
> > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > > Cc: Marek Vasut <marex@denx.de>
> > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > ---
> > > v2: improved commit message
> > > ---
> > >  common/fdt_support.c | 45 ++++++++++++++++++++++++++++++++++----------
> > >  1 file changed, 35 insertions(+), 10 deletions(-)
> > 
> > I'm dropping the linux-mtd list here and adding a bunch more platform
> > maintainers. In general, calling fdt_fixup_mtdparts is the wrong choice.
> > I see we do have a few cases in U-Boot where we're clearly doing
> > something dynamic to the partition table, but it looks like at first
> > glance most callers are using this hook when they should either be
> > having the partition map in the device tree properly (and using one of
> > the appropriate bindings) or passing the map in via the kernel command
> > line. I would like to ask everyone I've added to the list here to
> > please audit your platform and update it as needed. Thanks!
> 
> Hello Tom,
> what should we do with this patch? No feedback so far, apart this email
> from you.
> 
> Stefano: maybe you can pick patches 2 and 3 ?

I thought someone chimed in for the STM32 side? But yes, patches 2 and 3
should get picked up for sure.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/3] fdt: validate/fix cells count on mtdpart fixup

[Francesco Dolcini]

On Thu, Apr 06, 2023 at 11:25:27AM -0400, Tom Rini wrote:
> On Tue, Apr 04, 2023 at 06:18:03PM +0200, Francesco Dolcini wrote:
> > +Stefano
> > 
> > On Mon, Feb 06, 2023 at 06:17:31PM -0500, Tom Rini wrote:
> > > On Mon, Feb 06, 2023 at 11:48:36PM +0100, Francesco Dolcini wrote:
> > > 
> > > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > 
> > > > Fixup #size-cells value when updating the MTD partitions, this is
> > > > required to prevent issues in case the MTD parent set #size-cells to
> > > > zero.
> > > > This could happen for example in the legacy case in which the partitions
> > > > are created as direct child of the mtd node and that specific node has
> > > > no children. Recent clean-up on Linux device tree files created a boot
> > > > regression on colibri-imx7 [0].
> > > > 
> > > > This fixup has the limitation to assume 32-bit (#size-cells=1)
> > > > addressing, therefore it will not work with device bigger than 4GiB.
> > > > 
> > > > This change also enforce #address-cells to be the same as #size-cells,
> > > > this was already silently enforced by fdt_node_set_part_info(), now this
> > > > is checked explicitly and partition fixup will just fail in such case.
> > > > 
> > > > When the partition list is static the preferred way to pass the mtd
> > > > partition list to the kernel is to either define those in the source DTS
> > > > file or use mtdparts= in the command line.
> > > > Tweaking the DT from U-Boot should be avoided, unless some dynamic
> > > > changes are required, since it proved to be problematic when not
> > > > following the evolution of the "standard".
> > > > 
> > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > > > Cc: Marek Vasut <marex@denx.de>
> > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > ---
> > > > v2: improved commit message
> > > > ---
> > > >  common/fdt_support.c | 45 ++++++++++++++++++++++++++++++++++----------
> > > >  1 file changed, 35 insertions(+), 10 deletions(-)
> > > 
> > > I'm dropping the linux-mtd list here and adding a bunch more platform
> > > maintainers. In general, calling fdt_fixup_mtdparts is the wrong choice.
> > > I see we do have a few cases in U-Boot where we're clearly doing
> > > something dynamic to the partition table, but it looks like at first
> > > glance most callers are using this hook when they should either be
> > > having the partition map in the device tree properly (and using one of
> > > the appropriate bindings) or passing the map in via the kernel command
> > > line. I would like to ask everyone I've added to the list here to
> > > please audit your platform and update it as needed. Thanks!
> > 
> > Hello Tom,
> > what should we do with this patch? No feedback so far, apart this email
> > from you.
> > 
> > Stefano: maybe you can pick patches 2 and 3 ?
> 
> I thought someone chimed in for the STM32 side?
Whoops, yes, I remember the same.

However, what about this patch, do we want to merge it? It solves a
potential issue and from what I can tell it does not introduce new one.

Francesco

Re: [PATCH v2 2/3] colibri-imx7: specify MTD partitions on command line

[Francesco Dolcini]

Hello Stefano,

On Mon, Feb 06, 2023 at 11:48:37PM +0100, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Disable fdt_fixup_mtdparts() and pass MTD partition on the command
> line. Dynamically editing the fdt with a static partitions configuration
> is not required and there is no advantages compared to using the command
> line. This change should prevent boot failures as the one in [0].
> 
> Cc: Marek Vasut <marex@denx.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/ [0]
> Link: https://lore.kernel.org/all/20230105123334.7f90c289@xps-13/
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> v2:
>  - improved commit message
>  - removed dead code
> ---
>  board/toradex/colibri_imx7/colibri_imx7.c | 10 ----------
>  configs/colibri_imx7_defconfig            |  1 -
>  include/configs/colibri_imx7.h            |  2 +-
>  3 files changed, 1 insertion(+), 12 deletions(-)

On Mon, Feb 06, 2023 at 11:48:38PM +0100, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Disable fdt_fixup_mtdparts() and pass MTD partition on the command
> line. Dynamically editing the fdt with a static partitions configuration
> is not required and there is no advantages compared to using the command
> line. This change should prevent boot failures as the one in [0].
> 
> Cc: Marek Vasut <marex@denx.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/ [0]
> Link: https://lore.kernel.org/all/20230105123334.7f90c289@xps-13/
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>

Stefano: Can you pick those two patches? Or someone else should pick those?

Thanks,
Francesco

Re: [PATCH v2 2/3] colibri-imx7: specify MTD partitions on command line

[Dario Binacchi]

> Disable fdt_fixup_mtdparts() and pass MTD partition on the command
> line. Dynamically editing the fdt with a static partitions configuration
> is not required and there is no advantages compared to using the command
> line. This change should prevent boot failures as the one in [0].
>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/ [0]
> Link: https://lore.kernel.org/all/20230105123334.7f90c289@xps-13/
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> v2:
>  - improved commit message
>  - removed dead code
> ---
>  board/toradex/colibri_imx7/colibri_imx7.c | 10 ----------
>  configs/colibri_imx7_defconfig            |  1 -
>  include/configs/colibri_imx7.h            |  2 +-
>  3 files changed, 1 insertion(+), 12 deletions(-)

> diff --git a/board/toradex/colibri_imx7/colibri_imx7.c b/board/toradex/colibri_imx7/colibri_imx7.c
> index 6ce4fa376ac0..3e79ab93a982 100644
> --- a/board/toradex/colibri_imx7/colibri_imx7.c
> +++ b/board/toradex/colibri_imx7/colibri_imx7.c
> @@ -303,16 +303,6 @@ int ft_board_setup(void *blob, struct bd_info *bd)
>  			fdt_status_disabled(blob, off);
>  	}
>  #endif
> -#if defined(CONFIG_FDT_FIXUP_PARTITIONS)
> -	static const struct node_info nodes[] = {
> -		{ "fsl,imx7d-gpmi-nand", MTD_DEV_TYPE_NAND, }, /* NAND flash */
> -		{ "fsl,imx6q-gpmi-nand", MTD_DEV_TYPE_NAND, },
> -	};
> -
> -	/* Update partition nodes using info from mtdparts env var */
> -	puts("   Updating MTD partitions...\n");
> -	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
> -#endif
 
>  	return ft_common_board_setup(blob, bd);
>  }
> diff --git a/configs/colibri_imx7_defconfig b/configs/colibri_imx7_defconfig
> index 7e24dcd40090..5cc22a481dd9 100644
> --- a/configs/colibri_imx7_defconfig
> +++ b/configs/colibri_imx7_defconfig
> @@ -101,4 +101,3 @@ CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
>  CONFIG_CI_UDC=y
>  CONFIG_USB_GADGET_DOWNLOAD=y
>  CONFIG_OF_LIBFDT_OVERLAY=y
> -CONFIG_FDT_FIXUP_PARTITIONS=y
> diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h
> index c568643977ca..03f8ed14787a 100644
> --- a/include/configs/colibri_imx7.h
> +++ b/include/configs/colibri_imx7.h
> @@ -141,7 +141,7 @@
>  		"${board}/flash_blk.img && source ${loadaddr}\0" \
>  	"setup=setenv setupargs " \
>  		"console=tty1 console=${console}" \
> -		",${baudrate}n8 ${memargs} consoleblank=0\0" \
> +		",${baudrate}n8 ${memargs} ${mtdparts} consoleblank=0\0" \
>  	"setupdate=run setsdupdate || run setusbupdate || run setethupdate\0" \
>  	"setusbupdate=usb start && setenv interface usb && " \
>  		"fatload ${interface} 0:1 ${loadaddr} " \
> -- 
> 2.25.1

Applied to nand-next,
thanks and regards,

Dario Binacchi

Re: [PATCH v2 3/3] colibri-imx6ull: specify MTD partitions on command line

[Dario Binacchi]

> From: Francesco Dolcini <francesco.dolcini@toradex.com>

> Disable fdt_fixup_mtdparts() and pass MTD partition on the command
> line. Dynamically editing the fdt with a static partitions configuration
> is not required and there is no advantages compared to using the command
> line. This change should prevent boot failures as the one in [0].

> Cc: Marek Vasut <marex@denx.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/ [0]
> Link: https://lore.kernel.org/all/20230105123334.7f90c289@xps-13/
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> v2:
>  - removed dead code
>  - improved commit message
> ---
>  board/toradex/colibri-imx6ull/colibri-imx6ull.c | 11 -----------
>  configs/colibri-imx6ull_defconfig               |  1 -
>  include/configs/colibri-imx6ull.h               |  2 +-
>  3 files changed, 1 insertion(+), 13 deletions(-)

> diff --git a/board/toradex/colibri-imx6ull/colibri-imx6ull.c b/board/toradex/colibri-imx6ull/colibri-imx6ull.c
> index 6007f110e4ba..48fdb1e09712 100644
> --- a/board/toradex/colibri-imx6ull/colibri-imx6ull.c
> +++ b/board/toradex/colibri-imx6ull/colibri-imx6ull.c
> @@ -212,17 +212,6 @@ int checkboard(void)
>  #if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP)
>  int ft_board_setup(void *blob, struct bd_info *bd)
>  {
> -#if defined(CONFIG_FDT_FIXUP_PARTITIONS)
> -	static struct node_info nodes[] = {
> -		{ "fsl,imx6ull-gpmi-nand", MTD_DEV_TYPE_NAND, },
> -		{ "fsl,imx6q-gpmi-nand", MTD_DEV_TYPE_NAND, },
> -	};
> -
> -	/* Update partition nodes using info from mtdparts env var */
> -	puts("   Updating MTD partitions...\n");
> -	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
> -#endif
> -
>  	return ft_common_board_setup(blob, bd);
>  }
>  #endif
> diff --git a/configs/colibri-imx6ull_defconfig b/configs/colibri-imx6ull_defconfig
> index f2a0d79ccca9..fbab687f5ab8 100644
> --- a/configs/colibri-imx6ull_defconfig
> +++ b/configs/colibri-imx6ull_defconfig
> @@ -104,4 +104,3 @@ CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
>  CONFIG_CI_UDC=y
>  CONFIG_USB_GADGET_DOWNLOAD=y
>  CONFIG_OF_LIBFDT_OVERLAY=y
> -CONFIG_FDT_FIXUP_PARTITIONS=y
> diff --git a/include/configs/colibri-imx6ull.h b/include/configs/colibri-imx6ull.h
> index ba45ee4efd37..561a61ebc03c 100644
> --- a/include/configs/colibri-imx6ull.h
> +++ b/include/configs/colibri-imx6ull.h
> @@ -99,7 +99,7 @@
>  		"${board}/flash_blk.img && source ${loadaddr}\0" \
>  	"setup=setenv setupargs " \
>  		"console=tty1 console=${console}" \
> -		",${baudrate}n8 ${memargs} consoleblank=0\0" \
> +		",${baudrate}n8 ${memargs} ${mtdparts} consoleblank=0\0" \
>  	"setupdate=run setsdupdate || run setusbupdate || run setethupdate\0" \
>  	"setusbupdate=usb start && setenv interface usb && " \
>  		"fatload ${interface} 0:1 ${loadaddr} " \
> -- 
> 2.25.1

Applied to nand-next,
thanks and regards,

Dario Binacchi