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

[PATCH v1 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 just deprecate using this U-Boot function and pass the MTD partitions
from the command line, unless they are statically defined in the DTS file
in the first place.

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/

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

 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 +-
 5 files changed, 37 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH v1 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 just deprecate using this U-Boot function and pass the MTD partitions
from the command line, unless they are statically defined in the DTS file
in the first place.

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/

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

 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 +-
 5 files changed, 37 insertions(+), 14 deletions(-)

-- 
2.25.1


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

[PATCH v1 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.

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.

In general board should not generally need nor use this functionality
and should be just deprecated, passing mtdparts= in the kernel command
line is the preferred way according to Linux MTD subsystem maintainer.

Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
Cc: Marek Vasut <marex@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 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 v1 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.

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.

In general board should not generally need nor use this functionality
and should be just deprecated, passing mtdparts= in the kernel command
line is the preferred way according to Linux MTD subsystem maintainer.

Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
Cc: Marek Vasut <marex@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 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 v1 2/3] colibri-imx7: specify MTD partitions on command line

[Francesco Dolcini]

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

Disable fdt_fixup_mtdparts(), instead pass MTD partition on the command
line. This is the preferred method to be used when partitions are not
statically defined into the DTS according the Linux MTD subsystem
maintainer.

Cc: Marek Vasut <marex@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Link: https://lore.kernel.org/all/20230105123334.7f90c289@xps-13/
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 configs/colibri_imx7_defconfig | 1 -
 include/configs/colibri_imx7.h | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

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 v1 3/3] colibri-imx6ull: specify MTD partitions on command line

[Francesco Dolcini]

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

Disable fdt_fixup_mtdparts(), instead pass MTD partition on the command
line. This is the preferred method to be used when partitions are not
statically defined into the DTS according the Linux MTD subsystem
maintainer.

Cc: Marek Vasut <marex@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Link: https://lore.kernel.org/all/20230105123334.7f90c289@xps-13/
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 configs/colibri-imx6ull_defconfig | 1 -
 include/configs/colibri-imx6ull.h | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

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 v1 0/3] fdt: Fix mtparts fixup

[Tom Rini]

[-- Attachment #1.1: Type: text/plain, Size: 1609 bytes --]

On Fri, Jan 13, 2023 at 07:45:44PM +0100, Francesco Dolcini wrote:

> 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 just deprecate using this U-Boot function and pass the MTD partitions
> from the command line, unless they are statically defined in the DTS file
> in the first place.
> 
> 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/

My higher level question / concern here is, is using one of the dts
partition schemes still valid / preferred, or should everyone now have
reverted to passing via the kernel command line?  If device tree still,
is mtd/partitions/fixed-partitions.yaml the one to follow or something
else?

-- 
Tom

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

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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

Re: [PATCH v1 0/3] fdt: Fix mtparts fixup

[Tom Rini]

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

On Fri, Jan 13, 2023 at 07:45:44PM +0100, Francesco Dolcini wrote:

> 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 just deprecate using this U-Boot function and pass the MTD partitions
> from the command line, unless they are statically defined in the DTS file
> in the first place.
> 
> 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/

My higher level question / concern here is, is using one of the dts
partition schemes still valid / preferred, or should everyone now have
reverted to passing via the kernel command line?  If device tree still,
is mtd/partitions/fixed-partitions.yaml the one to follow or something
else?

-- 
Tom

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

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

[Marek Vasut]

On 1/13/23 19:45, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Disable fdt_fixup_mtdparts(), instead pass MTD partition on the command
> line. This is the preferred method to be used when partitions are not
> statically defined into the DTS according the Linux MTD subsystem
> maintainer.

Do we really want to do this ?
I thought passing the mtdparts via DT is the preferred way ?

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

[Marek Vasut]

On 1/13/23 19:45, 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.
> 
> 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.
> 
> In general board should not generally need nor use this functionality
> and should be just deprecated, passing mtdparts= in the kernel command
> line is the preferred way according to Linux MTD subsystem maintainer.
> 
> Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> Cc: Marek Vasut <marex@denx.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>   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 */
>   			}
>   		}

Can you please include the resulting gpmi node content with this fixup 
applied in the commit message , so it can be validated ?

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

[Marek Vasut]

On 1/13/23 19:45, 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.
> 
> 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.
> 
> In general board should not generally need nor use this functionality
> and should be just deprecated, passing mtdparts= in the kernel command
> line is the preferred way according to Linux MTD subsystem maintainer.
> 
> Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> Cc: Marek Vasut <marex@denx.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>   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 */
>   			}
>   		}

Can you please include the resulting gpmi node content with this fixup 
applied in the commit message , so it can be validated ?

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

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

[Francesco Dolcini]

On Sun, Jan 15, 2023 at 03:33:25PM +0100, Marek Vasut wrote:
> On 1/13/23 19:45, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > Disable fdt_fixup_mtdparts(), instead pass MTD partition on the command
> > line. This is the preferred method to be used when partitions are not
> > statically defined into the DTS according the Linux MTD subsystem
> > maintainer.
> 
> Do we really want to do this ?
> I thought passing the mtdparts via DT is the preferred way ?

Yes, to me it seems like the most robust approach to minimize chances
of regressions and boot issues.

With the current dts schema, that expects the partitions to not be
direct children of the nand controller node, but expecting having a
nand-chip, and partitions as a child of the nand-chip this is the most
likely approach to just works.

For example if I add a partition node to my DTS, older U-Boot will just
ignore it and add the partitions as children of the nand controller.
In this case the current parser will just fails leading to another
flavor of boot failure on our board.

In reality I cannot really tell which u-boot/kernel combination is going
to be used on our SOMs.

Said that, what would be the disadvantages of having the partitions on
the command line? Miquel?

Francesco

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

[Francesco Dolcini]

On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:
> On 1/13/23 19:45, 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.
> > 
> > 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.
> > 
> > In general board should not generally need nor use this functionality
> > and should be just deprecated, passing mtdparts= in the kernel command
> > line is the preferred way according to Linux MTD subsystem maintainer.
> > 
> > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > ---
> >   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 */
> >   			}
> >   		}
> 
> Can you please include the resulting gpmi node content with this fixup
> applied in the commit message , so it can be validated ?

I will add it to v2, I would wait a little bit more time to get
additional feedback sending it however.

In the meantime here the output, but nothing really changed!
What this change is doing is just
 - setting #size-cells to <1> when it is invalid
 - skip generation at all when #size-cells != #address-cells. Former
   code was just generating a broken table without any error
   message.

Here what is generated for colibri-imx7

nand-controller@33002000 {
	compatible = "fsl,imx7d-gpmi-nand";

	#address-cells = <0x01>;
	#size-cells = <0x01>;

[...snip...]

	partition@0 {
		label = "mx7-bcb";
		reg = <0x00 0x80000>;
	};

	partition@400000 {
		label = "ubi";
		reg = <0x400000 0x1fc00000>;
	};

	partition@80000 {
		read_only;
		label = "u-boot1";
		reg = <0x80000 0x180000>;
	};

	partition@380000 {
		label = "u-boot-env";
		reg = <0x380000 0x80000>;
	};

	partition@200000 {
		read_only;
		label = "u-boot2";
		reg = <0x200000 0x180000>;
	};
};

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

[Francesco Dolcini]

On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:
> On 1/13/23 19:45, 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.
> > 
> > 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.
> > 
> > In general board should not generally need nor use this functionality
> > and should be just deprecated, passing mtdparts= in the kernel command
> > line is the preferred way according to Linux MTD subsystem maintainer.
> > 
> > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > ---
> >   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 */
> >   			}
> >   		}
> 
> Can you please include the resulting gpmi node content with this fixup
> applied in the commit message , so it can be validated ?

I will add it to v2, I would wait a little bit more time to get
additional feedback sending it however.

In the meantime here the output, but nothing really changed!
What this change is doing is just
 - setting #size-cells to <1> when it is invalid
 - skip generation at all when #size-cells != #address-cells. Former
   code was just generating a broken table without any error
   message.

Here what is generated for colibri-imx7

nand-controller@33002000 {
	compatible = "fsl,imx7d-gpmi-nand";

	#address-cells = <0x01>;
	#size-cells = <0x01>;

[...snip...]

	partition@0 {
		label = "mx7-bcb";
		reg = <0x00 0x80000>;
	};

	partition@400000 {
		label = "ubi";
		reg = <0x400000 0x1fc00000>;
	};

	partition@80000 {
		read_only;
		label = "u-boot1";
		reg = <0x80000 0x180000>;
	};

	partition@380000 {
		label = "u-boot-env";
		reg = <0x380000 0x80000>;
	};

	partition@200000 {
		read_only;
		label = "u-boot2";
		reg = <0x200000 0x180000>;
	};
};


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

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

[Marek Vasut]

On 1/16/23 15:20, Francesco Dolcini wrote:
> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:
>> On 1/13/23 19:45, 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.
>>>
>>> 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.
>>>
>>> In general board should not generally need nor use this functionality
>>> and should be just deprecated, passing mtdparts= in the kernel command
>>> line is the preferred way according to Linux MTD subsystem maintainer.
>>>
>>> Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
>>> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
>>> Cc: Marek Vasut <marex@denx.de>
>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>>> ---
>>>    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 */
>>>    			}
>>>    		}
>>
>> Can you please include the resulting gpmi node content with this fixup
>> applied in the commit message , so it can be validated ?
> 
> I will add it to v2, I would wait a little bit more time to get
> additional feedback sending it however.
> 
> In the meantime here the output, but nothing really changed!
> What this change is doing is just
>   - setting #size-cells to <1> when it is invalid
>   - skip generation at all when #size-cells != #address-cells. Former
>     code was just generating a broken table without any error
>     message.
> 
> Here what is generated for colibri-imx7
> 
> nand-controller@33002000 {
> 	compatible = "fsl,imx7d-gpmi-nand";
> 
> 	#address-cells = <0x01>;
> 	#size-cells = <0x01>;
> 
> [...snip...]
> 
> 	partition@0 {
> 		label = "mx7-bcb";
> 		reg = <0x00 0x80000>;
> 	};
> 
> 	partition@400000 {
> 		label = "ubi";
> 		reg = <0x400000 0x1fc00000>;
> 	};
> 
> 	partition@80000 {
> 		read_only;
> 		label = "u-boot1";
> 		reg = <0x80000 0x180000>;
> 	};
> 
> 	partition@380000 {
> 		label = "u-boot-env";
> 		reg = <0x380000 0x80000>;
> 	};
> 
> 	partition@200000 {
> 		read_only;
> 		label = "u-boot2";
> 		reg = <0x200000 0x180000>;
> 	};
> };

This is what I was afraid of, shouldn't this contain the partitions in 
per-chipselect sub-node instead of directly in the GPMI node ?

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

[Marek Vasut]

On 1/16/23 15:20, Francesco Dolcini wrote:
> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:
>> On 1/13/23 19:45, 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.
>>>
>>> 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.
>>>
>>> In general board should not generally need nor use this functionality
>>> and should be just deprecated, passing mtdparts= in the kernel command
>>> line is the preferred way according to Linux MTD subsystem maintainer.
>>>
>>> Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
>>> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
>>> Cc: Marek Vasut <marex@denx.de>
>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>>> ---
>>>    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 */
>>>    			}
>>>    		}
>>
>> Can you please include the resulting gpmi node content with this fixup
>> applied in the commit message , so it can be validated ?
> 
> I will add it to v2, I would wait a little bit more time to get
> additional feedback sending it however.
> 
> In the meantime here the output, but nothing really changed!
> What this change is doing is just
>   - setting #size-cells to <1> when it is invalid
>   - skip generation at all when #size-cells != #address-cells. Former
>     code was just generating a broken table without any error
>     message.
> 
> Here what is generated for colibri-imx7
> 
> nand-controller@33002000 {
> 	compatible = "fsl,imx7d-gpmi-nand";
> 
> 	#address-cells = <0x01>;
> 	#size-cells = <0x01>;
> 
> [...snip...]
> 
> 	partition@0 {
> 		label = "mx7-bcb";
> 		reg = <0x00 0x80000>;
> 	};
> 
> 	partition@400000 {
> 		label = "ubi";
> 		reg = <0x400000 0x1fc00000>;
> 	};
> 
> 	partition@80000 {
> 		read_only;
> 		label = "u-boot1";
> 		reg = <0x80000 0x180000>;
> 	};
> 
> 	partition@380000 {
> 		label = "u-boot-env";
> 		reg = <0x380000 0x80000>;
> 	};
> 
> 	partition@200000 {
> 		read_only;
> 		label = "u-boot2";
> 		reg = <0x200000 0x180000>;
> 	};
> };

This is what I was afraid of, shouldn't this contain the partitions in 
per-chipselect sub-node instead of directly in the GPMI node ?

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

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

[Francesco Dolcini]

On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:
> On 1/16/23 15:20, Francesco Dolcini wrote:
> > On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:
> > > On 1/13/23 19:45, 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.
> > > > 
> > > > 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.
> > > > 
> > > > In general board should not generally need nor use this functionality
> > > > and should be just deprecated, passing mtdparts= in the kernel command
> > > > line is the preferred way according to Linux MTD subsystem maintainer.
> > > > 
> > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > > > Cc: Marek Vasut <marex@denx.de>
> > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > ---
> > > >    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 */
> > > >    			}
> > > >    		}
> > > 
> > > Can you please include the resulting gpmi node content with this fixup
> > > applied in the commit message , so it can be validated ?
> > 
> > I will add it to v2, I would wait a little bit more time to get
> > additional feedback sending it however.
> > 
> > In the meantime here the output, but nothing really changed!
> > What this change is doing is just
> >   - setting #size-cells to <1> when it is invalid
> >   - skip generation at all when #size-cells != #address-cells. Former
> >     code was just generating a broken table without any error
> >     message.
> > 
> > Here what is generated for colibri-imx7
> > 
> > nand-controller@33002000 {
> > 	compatible = "fsl,imx7d-gpmi-nand";
> > 
> > 	#address-cells = <0x01>;
> > 	#size-cells = <0x01>;
> > 
> > [...snip...]
> > 
> > 	partition@0 {
> > 		label = "mx7-bcb";
> > 		reg = <0x00 0x80000>;
> > 	};
> > 
> > 	partition@400000 {
> > 		label = "ubi";
> > 		reg = <0x400000 0x1fc00000>;
> > 	};
> > 
> > 	partition@80000 {
> > 		read_only;
> > 		label = "u-boot1";
> > 		reg = <0x80000 0x180000>;
> > 	};
> > 
> > 	partition@380000 {
> > 		label = "u-boot-env";
> > 		reg = <0x380000 0x80000>;
> > 	};
> > 
> > 	partition@200000 {
> > 		read_only;
> > 		label = "u-boot2";
> > 		reg = <0x200000 0x180000>;
> > 	};
> > };
> 
> This is what I was afraid of, shouldn't this contain the partitions in
> per-chipselect sub-node instead of directly in the GPMI node ?

That does not exists in my source DTS, this function just look for a
partitions node and update it when it exists.
I do not have a nand chip, and I do not want to add.

The reason is what I wrote in my other email, if I would do something
like older U-Boot's would ignore it and just generate the partitions
as direct children of the nand-controller.

Commit 36fee2f7621e ("common: fdt_support: add support for "partitions"
subnode to fdt_fixup_mtdparts()") was introduced only in v2022.04.

Francesco

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

[Francesco Dolcini]

On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:
> On 1/16/23 15:20, Francesco Dolcini wrote:
> > On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:
> > > On 1/13/23 19:45, 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.
> > > > 
> > > > 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.
> > > > 
> > > > In general board should not generally need nor use this functionality
> > > > and should be just deprecated, passing mtdparts= in the kernel command
> > > > line is the preferred way according to Linux MTD subsystem maintainer.
> > > > 
> > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > > > Cc: Marek Vasut <marex@denx.de>
> > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > ---
> > > >    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 */
> > > >    			}
> > > >    		}
> > > 
> > > Can you please include the resulting gpmi node content with this fixup
> > > applied in the commit message , so it can be validated ?
> > 
> > I will add it to v2, I would wait a little bit more time to get
> > additional feedback sending it however.
> > 
> > In the meantime here the output, but nothing really changed!
> > What this change is doing is just
> >   - setting #size-cells to <1> when it is invalid
> >   - skip generation at all when #size-cells != #address-cells. Former
> >     code was just generating a broken table without any error
> >     message.
> > 
> > Here what is generated for colibri-imx7
> > 
> > nand-controller@33002000 {
> > 	compatible = "fsl,imx7d-gpmi-nand";
> > 
> > 	#address-cells = <0x01>;
> > 	#size-cells = <0x01>;
> > 
> > [...snip...]
> > 
> > 	partition@0 {
> > 		label = "mx7-bcb";
> > 		reg = <0x00 0x80000>;
> > 	};
> > 
> > 	partition@400000 {
> > 		label = "ubi";
> > 		reg = <0x400000 0x1fc00000>;
> > 	};
> > 
> > 	partition@80000 {
> > 		read_only;
> > 		label = "u-boot1";
> > 		reg = <0x80000 0x180000>;
> > 	};
> > 
> > 	partition@380000 {
> > 		label = "u-boot-env";
> > 		reg = <0x380000 0x80000>;
> > 	};
> > 
> > 	partition@200000 {
> > 		read_only;
> > 		label = "u-boot2";
> > 		reg = <0x200000 0x180000>;
> > 	};
> > };
> 
> This is what I was afraid of, shouldn't this contain the partitions in
> per-chipselect sub-node instead of directly in the GPMI node ?

That does not exists in my source DTS, this function just look for a
partitions node and update it when it exists.
I do not have a nand chip, and I do not want to add.

The reason is what I wrote in my other email, if I would do something
like older U-Boot's would ignore it and just generate the partitions
as direct children of the nand-controller.

Commit 36fee2f7621e ("common: fdt_support: add support for "partitions"
subnode to fdt_fixup_mtdparts()") was introduced only in v2022.04.

Francesco


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

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

[Marek Vasut]

On 1/16/23 19:00, Francesco Dolcini wrote:
> On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:
>> On 1/16/23 15:20, Francesco Dolcini wrote:
>>> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:
>>>> On 1/13/23 19:45, 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.
>>>>>
>>>>> 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.
>>>>>
>>>>> In general board should not generally need nor use this functionality
>>>>> and should be just deprecated, passing mtdparts= in the kernel command
>>>>> line is the preferred way according to Linux MTD subsystem maintainer.
>>>>>
>>>>> Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
>>>>> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>>>>> ---
>>>>>     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 */
>>>>>     			}
>>>>>     		}
>>>>
>>>> Can you please include the resulting gpmi node content with this fixup
>>>> applied in the commit message , so it can be validated ?
>>>
>>> I will add it to v2, I would wait a little bit more time to get
>>> additional feedback sending it however.
>>>
>>> In the meantime here the output, but nothing really changed!
>>> What this change is doing is just
>>>    - setting #size-cells to <1> when it is invalid
>>>    - skip generation at all when #size-cells != #address-cells. Former
>>>      code was just generating a broken table without any error
>>>      message.
>>>
>>> Here what is generated for colibri-imx7
>>>
>>> nand-controller@33002000 {
>>> 	compatible = "fsl,imx7d-gpmi-nand";
>>>
>>> 	#address-cells = <0x01>;
>>> 	#size-cells = <0x01>;
>>>
>>> [...snip...]
>>>
>>> 	partition@0 {
>>> 		label = "mx7-bcb";
>>> 		reg = <0x00 0x80000>;
>>> 	};
>>>
>>> 	partition@400000 {
>>> 		label = "ubi";
>>> 		reg = <0x400000 0x1fc00000>;
>>> 	};
>>>
>>> 	partition@80000 {
>>> 		read_only;
>>> 		label = "u-boot1";
>>> 		reg = <0x80000 0x180000>;
>>> 	};
>>>
>>> 	partition@380000 {
>>> 		label = "u-boot-env";
>>> 		reg = <0x380000 0x80000>;
>>> 	};
>>>
>>> 	partition@200000 {
>>> 		read_only;
>>> 		label = "u-boot2";
>>> 		reg = <0x200000 0x180000>;
>>> 	};
>>> };
>>
>> This is what I was afraid of, shouldn't this contain the partitions in
>> per-chipselect sub-node instead of directly in the GPMI node ?
> 
> That does not exists in my source DTS, this function just look for a
> partitions node and update it when it exists.
> I do not have a nand chip, and I do not want to add.

I know. I wonder if the function should convert the bindings to latest 
greatest, but I am starting to feel like hard-coding this kind of 
complex logic into bootloader is not a great idea .

> The reason is what I wrote in my other email, if I would do something
> like older U-Boot's would ignore it and just generate the partitions
> as direct children of the nand-controller.
> 
> Commit 36fee2f7621e ("common: fdt_support: add support for "partitions"
> subnode to fdt_fixup_mtdparts()") was introduced only in v2022.04.

There is the Linux BOOT_CONFIG , that might be some sort of alternative 
to complex kernel command line, but that's a bit more work. I figured 
I'd mention it here.

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

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

[Marek Vasut]

On 1/16/23 19:00, Francesco Dolcini wrote:
> On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:
>> On 1/16/23 15:20, Francesco Dolcini wrote:
>>> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:
>>>> On 1/13/23 19:45, 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.
>>>>>
>>>>> 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.
>>>>>
>>>>> In general board should not generally need nor use this functionality
>>>>> and should be just deprecated, passing mtdparts= in the kernel command
>>>>> line is the preferred way according to Linux MTD subsystem maintainer.
>>>>>
>>>>> Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
>>>>> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>>>>> ---
>>>>>     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 */
>>>>>     			}
>>>>>     		}
>>>>
>>>> Can you please include the resulting gpmi node content with this fixup
>>>> applied in the commit message , so it can be validated ?
>>>
>>> I will add it to v2, I would wait a little bit more time to get
>>> additional feedback sending it however.
>>>
>>> In the meantime here the output, but nothing really changed!
>>> What this change is doing is just
>>>    - setting #size-cells to <1> when it is invalid
>>>    - skip generation at all when #size-cells != #address-cells. Former
>>>      code was just generating a broken table without any error
>>>      message.
>>>
>>> Here what is generated for colibri-imx7
>>>
>>> nand-controller@33002000 {
>>> 	compatible = "fsl,imx7d-gpmi-nand";
>>>
>>> 	#address-cells = <0x01>;
>>> 	#size-cells = <0x01>;
>>>
>>> [...snip...]
>>>
>>> 	partition@0 {
>>> 		label = "mx7-bcb";
>>> 		reg = <0x00 0x80000>;
>>> 	};
>>>
>>> 	partition@400000 {
>>> 		label = "ubi";
>>> 		reg = <0x400000 0x1fc00000>;
>>> 	};
>>>
>>> 	partition@80000 {
>>> 		read_only;
>>> 		label = "u-boot1";
>>> 		reg = <0x80000 0x180000>;
>>> 	};
>>>
>>> 	partition@380000 {
>>> 		label = "u-boot-env";
>>> 		reg = <0x380000 0x80000>;
>>> 	};
>>>
>>> 	partition@200000 {
>>> 		read_only;
>>> 		label = "u-boot2";
>>> 		reg = <0x200000 0x180000>;
>>> 	};
>>> };
>>
>> This is what I was afraid of, shouldn't this contain the partitions in
>> per-chipselect sub-node instead of directly in the GPMI node ?
> 
> That does not exists in my source DTS, this function just look for a
> partitions node and update it when it exists.
> I do not have a nand chip, and I do not want to add.

I know. I wonder if the function should convert the bindings to latest 
greatest, but I am starting to feel like hard-coding this kind of 
complex logic into bootloader is not a great idea .

> The reason is what I wrote in my other email, if I would do something
> like older U-Boot's would ignore it and just generate the partitions
> as direct children of the nand-controller.
> 
> Commit 36fee2f7621e ("common: fdt_support: add support for "partitions"
> subnode to fdt_fixup_mtdparts()") was introduced only in v2022.04.

There is the Linux BOOT_CONFIG , that might be some sort of alternative 
to complex kernel command line, but that's a bit more work. I figured 
I'd mention it here.

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

[Miquel Raynal]

Hi Francesco,

Thanks for your work.

marex@denx.de wrote on Tue, 17 Jan 2023 01:59:54 +0100:

> On 1/16/23 19:00, Francesco Dolcini wrote:
> > On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:  
> >> On 1/16/23 15:20, Francesco Dolcini wrote:  
> >>> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:  
> >>>> On 1/13/23 19:45, 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.
> >>>>>
> >>>>> 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.
> >>>>>
> >>>>> In general board should not generally need nor use this functionality
> >>>>> and should be just deprecated, passing mtdparts= in the kernel command
> >>>>> line is the preferred way according to Linux MTD subsystem maintainer.

Just to be clear, I'm perfectly fine with the partitions being either
on the cmdline or in the DT without preference (well, the cmdline only
works with the simple generic parser, but whatever). What I really
meant is that tweaking the DT from U-Boot should be rare and in no case
the "right thing to add partitions".

> >>>>>
> >>>>> Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> >>>>> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> >>>>> Cc: Marek Vasut <marex@denx.de>
> >>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> >>>>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> >>>>> ---
> >>>>>     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 */
> >>>>>     			}
> >>>>>     		}  
> >>>>
> >>>> Can you please include the resulting gpmi node content with this fixup
> >>>> applied in the commit message , so it can be validated ?  
> >>>
> >>> I will add it to v2, I would wait a little bit more time to get
> >>> additional feedback sending it however.
> >>>
> >>> In the meantime here the output, but nothing really changed!
> >>> What this change is doing is just
> >>>    - setting #size-cells to <1> when it is invalid
> >>>    - skip generation at all when #size-cells != #address-cells. Former
> >>>      code was just generating a broken table without any error
> >>>      message.
> >>>
> >>> Here what is generated for colibri-imx7
> >>>
> >>> nand-controller@33002000 {
> >>> 	compatible = "fsl,imx7d-gpmi-nand";
> >>>
> >>> 	#address-cells = <0x01>;
> >>> 	#size-cells = <0x01>;
> >>>
> >>> [...snip...]
> >>>
> >>> 	partition@0 {
> >>> 		label = "mx7-bcb";
> >>> 		reg = <0x00 0x80000>;
> >>> 	};
> >>>
> >>> 	partition@400000 {
> >>> 		label = "ubi";
> >>> 		reg = <0x400000 0x1fc00000>;
> >>> 	};
> >>>
> >>> 	partition@80000 {
> >>> 		read_only;
> >>> 		label = "u-boot1";
> >>> 		reg = <0x80000 0x180000>;
> >>> 	};
> >>>
> >>> 	partition@380000 {
> >>> 		label = "u-boot-env";
> >>> 		reg = <0x380000 0x80000>;
> >>> 	};
> >>>
> >>> 	partition@200000 {
> >>> 		read_only;
> >>> 		label = "u-boot2";
> >>> 		reg = <0x200000 0x180000>;
> >>> 	};
> >>> };  
> >>
> >> This is what I was afraid of, shouldn't this contain the partitions in
> >> per-chipselect sub-node instead of directly in the GPMI node ?  
> > 
> > That does not exists in my source DTS, this function just look for a
> > partitions node and update it when it exists.
> > I do not have a nand chip, and I do not want to add.  
> 
> I know. I wonder if the function should convert the bindings to latest greatest, but I am starting to feel like hard-coding this kind of complex logic into bootloader is not a great idea .

As long as the function is deprecated (because that is not a pertinent
way of declaring partitions on the long run) I'm fine with keeping the
partitions in the root controller node, there is little we can do about
it now anyway.

> > The reason is what I wrote in my other email, if I would do something
> > like older U-Boot's would ignore it and just generate the partitions
> > as direct children of the nand-controller.
> > 
> > Commit 36fee2f7621e ("common: fdt_support: add support for "partitions"
> > subnode to fdt_fixup_mtdparts()") was introduced only in v2022.04.  
> 
> There is the Linux BOOT_CONFIG , that might be some sort of alternative to complex kernel command line, but that's a bit more work. I figured I'd mention it here.


Thanks,
Miquèl

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

[Miquel Raynal]

Hi Francesco,

Thanks for your work.

marex@denx.de wrote on Tue, 17 Jan 2023 01:59:54 +0100:

> On 1/16/23 19:00, Francesco Dolcini wrote:
> > On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:  
> >> On 1/16/23 15:20, Francesco Dolcini wrote:  
> >>> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:  
> >>>> On 1/13/23 19:45, 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.
> >>>>>
> >>>>> 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.
> >>>>>
> >>>>> In general board should not generally need nor use this functionality
> >>>>> and should be just deprecated, passing mtdparts= in the kernel command
> >>>>> line is the preferred way according to Linux MTD subsystem maintainer.

Just to be clear, I'm perfectly fine with the partitions being either
on the cmdline or in the DT without preference (well, the cmdline only
works with the simple generic parser, but whatever). What I really
meant is that tweaking the DT from U-Boot should be rare and in no case
the "right thing to add partitions".

> >>>>>
> >>>>> Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> >>>>> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> >>>>> Cc: Marek Vasut <marex@denx.de>
> >>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> >>>>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> >>>>> ---
> >>>>>     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 */
> >>>>>     			}
> >>>>>     		}  
> >>>>
> >>>> Can you please include the resulting gpmi node content with this fixup
> >>>> applied in the commit message , so it can be validated ?  
> >>>
> >>> I will add it to v2, I would wait a little bit more time to get
> >>> additional feedback sending it however.
> >>>
> >>> In the meantime here the output, but nothing really changed!
> >>> What this change is doing is just
> >>>    - setting #size-cells to <1> when it is invalid
> >>>    - skip generation at all when #size-cells != #address-cells. Former
> >>>      code was just generating a broken table without any error
> >>>      message.
> >>>
> >>> Here what is generated for colibri-imx7
> >>>
> >>> nand-controller@33002000 {
> >>> 	compatible = "fsl,imx7d-gpmi-nand";
> >>>
> >>> 	#address-cells = <0x01>;
> >>> 	#size-cells = <0x01>;
> >>>
> >>> [...snip...]
> >>>
> >>> 	partition@0 {
> >>> 		label = "mx7-bcb";
> >>> 		reg = <0x00 0x80000>;
> >>> 	};
> >>>
> >>> 	partition@400000 {
> >>> 		label = "ubi";
> >>> 		reg = <0x400000 0x1fc00000>;
> >>> 	};
> >>>
> >>> 	partition@80000 {
> >>> 		read_only;
> >>> 		label = "u-boot1";
> >>> 		reg = <0x80000 0x180000>;
> >>> 	};
> >>>
> >>> 	partition@380000 {
> >>> 		label = "u-boot-env";
> >>> 		reg = <0x380000 0x80000>;
> >>> 	};
> >>>
> >>> 	partition@200000 {
> >>> 		read_only;
> >>> 		label = "u-boot2";
> >>> 		reg = <0x200000 0x180000>;
> >>> 	};
> >>> };  
> >>
> >> This is what I was afraid of, shouldn't this contain the partitions in
> >> per-chipselect sub-node instead of directly in the GPMI node ?  
> > 
> > That does not exists in my source DTS, this function just look for a
> > partitions node and update it when it exists.
> > I do not have a nand chip, and I do not want to add.  
> 
> I know. I wonder if the function should convert the bindings to latest greatest, but I am starting to feel like hard-coding this kind of complex logic into bootloader is not a great idea .

As long as the function is deprecated (because that is not a pertinent
way of declaring partitions on the long run) I'm fine with keeping the
partitions in the root controller node, there is little we can do about
it now anyway.

> > The reason is what I wrote in my other email, if I would do something
> > like older U-Boot's would ignore it and just generate the partitions
> > as direct children of the nand-controller.
> > 
> > Commit 36fee2f7621e ("common: fdt_support: add support for "partitions"
> > subnode to fdt_fixup_mtdparts()") was introduced only in v2022.04.  
> 
> There is the Linux BOOT_CONFIG , that might be some sort of alternative to complex kernel command line, but that's a bit more work. I figured I'd mention it here.


Thanks,
Miquèl

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

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

[Miquel Raynal]

Hi Francesco,

francesco@dolcini.it wrote on Mon, 16 Jan 2023 14:58:28 +0100:

> On Sun, Jan 15, 2023 at 03:33:25PM +0100, Marek Vasut wrote:
> > On 1/13/23 19:45, Francesco Dolcini wrote:  
> > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > 
> > > Disable fdt_fixup_mtdparts(), instead pass MTD partition on the command
> > > line. This is the preferred method to be used when partitions are not
> > > statically defined into the DTS according the Linux MTD subsystem
> > > maintainer.  
> > 
> > Do we really want to do this ?
> > I thought passing the mtdparts via DT is the preferred way ?  
> 
> Yes, to me it seems like the most robust approach to minimize chances
> of regressions and boot issues.
> 
> With the current dts schema, that expects the partitions to not be
> direct children of the nand controller node, but expecting having a
> nand-chip, and partitions as a child of the nand-chip this is the most
> likely approach to just works.
> 
> For example if I add a partition node to my DTS, older U-Boot will just
> ignore it and add the partitions as children of the nand controller.
> In this case the current parser will just fails leading to another
> flavor of boot failure on our board.
> 
> In reality I cannot really tell which u-boot/kernel combination is going
> to be used on our SOMs.
> 
> Said that, what would be the disadvantages of having the partitions on
> the command line? Miquel?

It only works with the simplest parser, but that's anyway the one
implied when tweaking the DT from U-Boot to add the partitions.

If that works, then it looks the simplest approach indeed.

Thanks,
Miquèl

Re: [PATCH v1 0/3] fdt: Fix mtparts fixup

[Miquel Raynal]

Hi Tom,

trini@konsulko.com wrote on Fri, 13 Jan 2023 14:34:11 -0500:

> On Fri, Jan 13, 2023 at 07:45:44PM +0100, Francesco Dolcini wrote:
> 
> > 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 just deprecate using this U-Boot function and pass the MTD partitions
> > from the command line, unless they are statically defined in the DTS file
> > in the first place.
> > 
> > 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/  
> 
> My higher level question / concern here is, is using one of the dts
> partition schemes still valid / preferred, or should everyone now have
> reverted to passing via the kernel command line?  If device tree still,
> is mtd/partitions/fixed-partitions.yaml the one to follow or something
> else?

I don't think we can "prefer" one mode over the other between cmdline
and DTS. Both should work pretty well. Of course on the cmdline you can
only define fixed partitions and many devices require more advanced
parsers, which are only available through DTS, but for simple
partitions, it works totally okay.

The only thing that I would like to avoid is the need to write code in
the bootloaders to tweak the FDT in order to add partitions. That is
clearly not needed, error prone, and do not follow evolution of the
"standard", as we just discovered.

Thanks,
Miquèl

Re: [PATCH v1 0/3] fdt: Fix mtparts fixup

[Miquel Raynal]

Hi Tom,

trini@konsulko.com wrote on Fri, 13 Jan 2023 14:34:11 -0500:

> On Fri, Jan 13, 2023 at 07:45:44PM +0100, Francesco Dolcini wrote:
> 
> > 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 just deprecate using this U-Boot function and pass the MTD partitions
> > from the command line, unless they are statically defined in the DTS file
> > in the first place.
> > 
> > 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/  
> 
> My higher level question / concern here is, is using one of the dts
> partition schemes still valid / preferred, or should everyone now have
> reverted to passing via the kernel command line?  If device tree still,
> is mtd/partitions/fixed-partitions.yaml the one to follow or something
> else?

I don't think we can "prefer" one mode over the other between cmdline
and DTS. Both should work pretty well. Of course on the cmdline you can
only define fixed partitions and many devices require more advanced
parsers, which are only available through DTS, but for simple
partitions, it works totally okay.

The only thing that I would like to avoid is the need to write code in
the bootloaders to tweak the FDT in order to add partitions. That is
clearly not needed, error prone, and do not follow evolution of the
"standard", as we just discovered.

Thanks,
Miquèl

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

Re: [PATCH v1 0/3] fdt: Fix mtparts fixup

[Tom Rini]

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

On Mon, Jan 23, 2023 at 11:06:06AM +0100, Miquel Raynal wrote:
> Hi Tom,
> 
> trini@konsulko.com wrote on Fri, 13 Jan 2023 14:34:11 -0500:
> 
> > On Fri, Jan 13, 2023 at 07:45:44PM +0100, Francesco Dolcini wrote:
> > 
> > > 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 just deprecate using this U-Boot function and pass the MTD partitions
> > > from the command line, unless they are statically defined in the DTS file
> > > in the first place.
> > > 
> > > 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/  
> > 
> > My higher level question / concern here is, is using one of the dts
> > partition schemes still valid / preferred, or should everyone now have
> > reverted to passing via the kernel command line?  If device tree still,
> > is mtd/partitions/fixed-partitions.yaml the one to follow or something
> > else?
> 
> I don't think we can "prefer" one mode over the other between cmdline
> and DTS. Both should work pretty well. Of course on the cmdline you can
> only define fixed partitions and many devices require more advanced
> parsers, which are only available through DTS, but for simple
> partitions, it works totally okay.

When both are present, which one is used?

> The only thing that I would like to avoid is the need to write code in
> the bootloaders to tweak the FDT in order to add partitions. That is
> clearly not needed, error prone, and do not follow evolution of the
> "standard", as we just discovered.

I'm not sure about this. Looking around in U-Boot today, I see two types
of cases. One of which, the colibri case, can clearly be not done and
either passed on the command line, or put in to the device tree as
there's nothing run-time related being tweaked here. That's a fine path
to take on those platforms and Francesco's patches should be updated to
remove the unused C code too from the board code.

But the other cases are doing something dynamic and run-time related.
There's the omap3 igep00x0 family (which yes, legacy) that is doing NAND
or oneNAND and adjusting things at run time.  I don't know how much
anyone has interest in those platforms at this point, nor exactly who to
contact (for Linux or U-Boot). There's also the stm32mp1 family doing
something that's very not obvious at first glance, so I've cc'd the
maintainers there.

-- 
Tom

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

Re: [PATCH v1 0/3] fdt: Fix mtparts fixup

[Tom Rini]

[-- Attachment #1.1: Type: text/plain, Size: 3368 bytes --]

On Mon, Jan 23, 2023 at 11:06:06AM +0100, Miquel Raynal wrote:
> Hi Tom,
> 
> trini@konsulko.com wrote on Fri, 13 Jan 2023 14:34:11 -0500:
> 
> > On Fri, Jan 13, 2023 at 07:45:44PM +0100, Francesco Dolcini wrote:
> > 
> > > 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 just deprecate using this U-Boot function and pass the MTD partitions
> > > from the command line, unless they are statically defined in the DTS file
> > > in the first place.
> > > 
> > > 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/  
> > 
> > My higher level question / concern here is, is using one of the dts
> > partition schemes still valid / preferred, or should everyone now have
> > reverted to passing via the kernel command line?  If device tree still,
> > is mtd/partitions/fixed-partitions.yaml the one to follow or something
> > else?
> 
> I don't think we can "prefer" one mode over the other between cmdline
> and DTS. Both should work pretty well. Of course on the cmdline you can
> only define fixed partitions and many devices require more advanced
> parsers, which are only available through DTS, but for simple
> partitions, it works totally okay.

When both are present, which one is used?

> The only thing that I would like to avoid is the need to write code in
> the bootloaders to tweak the FDT in order to add partitions. That is
> clearly not needed, error prone, and do not follow evolution of the
> "standard", as we just discovered.

I'm not sure about this. Looking around in U-Boot today, I see two types
of cases. One of which, the colibri case, can clearly be not done and
either passed on the command line, or put in to the device tree as
there's nothing run-time related being tweaked here. That's a fine path
to take on those platforms and Francesco's patches should be updated to
remove the unused C code too from the board code.

But the other cases are doing something dynamic and run-time related.
There's the omap3 igep00x0 family (which yes, legacy) that is doing NAND
or oneNAND and adjusting things at run time.  I don't know how much
anyone has interest in those platforms at this point, nor exactly who to
contact (for Linux or U-Boot). There's also the stm32mp1 family doing
something that's very not obvious at first glance, so I've cc'd the
maintainers there.

-- 
Tom

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

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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

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

[Francesco Dolcini]

Hello Miquel,

On Mon, Jan 23, 2023 at 10:56:10AM +0100, Miquel Raynal wrote:
> marex@denx.de wrote on Tue, 17 Jan 2023 01:59:54 +0100:
> 
> > On 1/16/23 19:00, Francesco Dolcini wrote:
> > > On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:  
> > >> On 1/16/23 15:20, Francesco Dolcini wrote:  
> > >>> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:  
> > >>>> On 1/13/23 19:45, 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.
> > >>>>>
> > >>>>> 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.
> > >>>>>
> > >>>>> In general board should not generally need nor use this functionality
> > >>>>> and should be just deprecated, passing mtdparts= in the kernel command
> > >>>>> line is the preferred way according to Linux MTD subsystem maintainer.
> 
> Just to be clear, I'm perfectly fine with the partitions being either
> on the cmdline or in the DT without preference (well, the cmdline only
> works with the simple generic parser, but whatever). What I really
> meant is that tweaking the DT from U-Boot should be rare and in no case
> the "right thing to add partitions".

Miquel, should I send a v2 to clarify the commit message or you do not
mind keeping it as it is?

Francesco


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

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

[Francesco Dolcini]

Hello Miquel,

On Mon, Jan 23, 2023 at 10:56:10AM +0100, Miquel Raynal wrote:
> marex@denx.de wrote on Tue, 17 Jan 2023 01:59:54 +0100:
> 
> > On 1/16/23 19:00, Francesco Dolcini wrote:
> > > On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:  
> > >> On 1/16/23 15:20, Francesco Dolcini wrote:  
> > >>> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:  
> > >>>> On 1/13/23 19:45, 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.
> > >>>>>
> > >>>>> 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.
> > >>>>>
> > >>>>> In general board should not generally need nor use this functionality
> > >>>>> and should be just deprecated, passing mtdparts= in the kernel command
> > >>>>> line is the preferred way according to Linux MTD subsystem maintainer.
> 
> Just to be clear, I'm perfectly fine with the partitions being either
> on the cmdline or in the DT without preference (well, the cmdline only
> works with the simple generic parser, but whatever). What I really
> meant is that tweaking the DT from U-Boot should be rare and in no case
> the "right thing to add partitions".

Miquel, should I send a v2 to clarify the commit message or you do not
mind keeping it as it is?

Francesco

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

[Miquel Raynal]

Hi Francesco,

francesco@dolcini.it wrote on Tue, 24 Jan 2023 09:41:41 +0100:

> Hello Miquel,
> 
> On Mon, Jan 23, 2023 at 10:56:10AM +0100, Miquel Raynal wrote:
> > marex@denx.de wrote on Tue, 17 Jan 2023 01:59:54 +0100:
> >   
> > > On 1/16/23 19:00, Francesco Dolcini wrote:  
> > > > On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:    
> > > >> On 1/16/23 15:20, Francesco Dolcini wrote:    
> > > >>> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:    
> > > >>>> On 1/13/23 19:45, 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.
> > > >>>>>
> > > >>>>> 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.
> > > >>>>>
> > > >>>>> In general board should not generally need nor use this functionality
> > > >>>>> and should be just deprecated, passing mtdparts= in the kernel command
> > > >>>>> line is the preferred way according to Linux MTD subsystem maintainer.  
> > 
> > Just to be clear, I'm perfectly fine with the partitions being either
> > on the cmdline or in the DT without preference (well, the cmdline only
> > works with the simple generic parser, but whatever). What I really
> > meant is that tweaking the DT from U-Boot should be rare and in no case
> > the "right thing to add partitions".  
> 
> Miquel, should I send a v2 to clarify the commit message or you do not
> mind keeping it as it is?

I think it is worth mentioning, also adding Tom's remark:
- Describing the partitions on the cmdline or in the DT is the right
  thing to do.
- Tweaking the DT to add static partitions should be forbidden.
- In some rare legacy cases, dynamic changes to the partitions might be
  tolerated.

Thanks,
Miquèl

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

[Miquel Raynal]

Hi Francesco,

francesco@dolcini.it wrote on Tue, 24 Jan 2023 09:41:41 +0100:

> Hello Miquel,
> 
> On Mon, Jan 23, 2023 at 10:56:10AM +0100, Miquel Raynal wrote:
> > marex@denx.de wrote on Tue, 17 Jan 2023 01:59:54 +0100:
> >   
> > > On 1/16/23 19:00, Francesco Dolcini wrote:  
> > > > On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:    
> > > >> On 1/16/23 15:20, Francesco Dolcini wrote:    
> > > >>> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:    
> > > >>>> On 1/13/23 19:45, 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.
> > > >>>>>
> > > >>>>> 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.
> > > >>>>>
> > > >>>>> In general board should not generally need nor use this functionality
> > > >>>>> and should be just deprecated, passing mtdparts= in the kernel command
> > > >>>>> line is the preferred way according to Linux MTD subsystem maintainer.  
> > 
> > Just to be clear, I'm perfectly fine with the partitions being either
> > on the cmdline or in the DT without preference (well, the cmdline only
> > works with the simple generic parser, but whatever). What I really
> > meant is that tweaking the DT from U-Boot should be rare and in no case
> > the "right thing to add partitions".  
> 
> Miquel, should I send a v2 to clarify the commit message or you do not
> mind keeping it as it is?

I think it is worth mentioning, also adding Tom's remark:
- Describing the partitions on the cmdline or in the DT is the right
  thing to do.
- Tweaking the DT to add static partitions should be forbidden.
- In some rare legacy cases, dynamic changes to the partitions might be
  tolerated.

Thanks,
Miquèl

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

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

[Francesco Dolcini]

On Tue, Jan 24, 2023 at 09:58:15AM +0100, Miquel Raynal wrote:
> francesco@dolcini.it wrote on Tue, 24 Jan 2023 09:41:41 +0100:
> > On Mon, Jan 23, 2023 at 10:56:10AM +0100, Miquel Raynal wrote:
> > > marex@denx.de wrote on Tue, 17 Jan 2023 01:59:54 +0100:
> > >   
> > > > On 1/16/23 19:00, Francesco Dolcini wrote:  
> > > > > On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:    
> > > > >> On 1/16/23 15:20, Francesco Dolcini wrote:    
> > > > >>> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:    
> > > > >>>> On 1/13/23 19:45, 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.
> > > > >>>>>
> > > > >>>>> 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.
> > > > >>>>>
> > > > >>>>> In general board should not generally need nor use this functionality
> > > > >>>>> and should be just deprecated, passing mtdparts= in the kernel command
> > > > >>>>> line is the preferred way according to Linux MTD subsystem maintainer.  
> > > 
> > > Just to be clear, I'm perfectly fine with the partitions being either
> > > on the cmdline or in the DT without preference (well, the cmdline only
> > > works with the simple generic parser, but whatever). What I really
> > > meant is that tweaking the DT from U-Boot should be rare and in no case
> > > the "right thing to add partitions".  
> > 
> > Miquel, should I send a v2 to clarify the commit message or you do not
> > mind keeping it as it is?
> 
> I think it is worth mentioning, also adding Tom's remark:
> - Describing the partitions on the cmdline or in the DT is the right
>   thing to do.
> - Tweaking the DT to add static partitions should be forbidden.
> - In some rare legacy cases, dynamic changes to the partitions might be
>   tolerated.

Agreed, I'll wait a little bit sending a v2 to see how the discussion
progresses and to see if there are concerns on the actual code change.

Francesco

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

[Francesco Dolcini]

On Tue, Jan 24, 2023 at 09:58:15AM +0100, Miquel Raynal wrote:
> francesco@dolcini.it wrote on Tue, 24 Jan 2023 09:41:41 +0100:
> > On Mon, Jan 23, 2023 at 10:56:10AM +0100, Miquel Raynal wrote:
> > > marex@denx.de wrote on Tue, 17 Jan 2023 01:59:54 +0100:
> > >   
> > > > On 1/16/23 19:00, Francesco Dolcini wrote:  
> > > > > On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:    
> > > > >> On 1/16/23 15:20, Francesco Dolcini wrote:    
> > > > >>> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:    
> > > > >>>> On 1/13/23 19:45, 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.
> > > > >>>>>
> > > > >>>>> 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.
> > > > >>>>>
> > > > >>>>> In general board should not generally need nor use this functionality
> > > > >>>>> and should be just deprecated, passing mtdparts= in the kernel command
> > > > >>>>> line is the preferred way according to Linux MTD subsystem maintainer.  
> > > 
> > > Just to be clear, I'm perfectly fine with the partitions being either
> > > on the cmdline or in the DT without preference (well, the cmdline only
> > > works with the simple generic parser, but whatever). What I really
> > > meant is that tweaking the DT from U-Boot should be rare and in no case
> > > the "right thing to add partitions".  
> > 
> > Miquel, should I send a v2 to clarify the commit message or you do not
> > mind keeping it as it is?
> 
> I think it is worth mentioning, also adding Tom's remark:
> - Describing the partitions on the cmdline or in the DT is the right
>   thing to do.
> - Tweaking the DT to add static partitions should be forbidden.
> - In some rare legacy cases, dynamic changes to the partitions might be
>   tolerated.

Agreed, I'll wait a little bit sending a v2 to see how the discussion
progresses and to see if there are concerns on the actual code change.

Francesco


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

Re: [PATCH v1 0/3] fdt: Fix mtparts fixup

[Patrick DELAUNAY]

Hi,

On 1/23/23 21:01, Tom Rini wrote:
> On Mon, Jan 23, 2023 at 11:06:06AM +0100, Miquel Raynal wrote:
>> Hi Tom,
>>
>> trini@konsulko.com wrote on Fri, 13 Jan 2023 14:34:11 -0500:
>>
>>> On Fri, Jan 13, 2023 at 07:45:44PM +0100, Francesco Dolcini wrote:
>>>
>>>> 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 just deprecate using this U-Boot function and pass the MTD partitions
>>>> from the command line, unless they are statically defined in the DTS file
>>>> in the first place.
>>>>
>>>> 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/
>>> My higher level question / concern here is, is using one of the dts
>>> partition schemes still valid / preferred, or should everyone now have
>>> reverted to passing via the kernel command line?  If device tree still,
>>> is mtd/partitions/fixed-partitions.yaml the one to follow or something
>>> else?
>> I don't think we can "prefer" one mode over the other between cmdline
>> and DTS. Both should work pretty well. Of course on the cmdline you can
>> only define fixed partitions and many devices require more advanced
>> parsers, which are only available through DTS, but for simple
>> partitions, it works totally okay.
> When both are present, which one is used?
>
>> The only thing that I would like to avoid is the need to write code in
>> the bootloaders to tweak the FDT in order to add partitions. That is
>> clearly not needed, error prone, and do not follow evolution of the
>> "standard", as we just discovered.
> I'm not sure about this. Looking around in U-Boot today, I see two types
> of cases. One of which, the colibri case, can clearly be not done and
> either passed on the command line, or put in to the device tree as
> there's nothing run-time related being tweaked here. That's a fine path
> to take on those platforms and Francesco's patches should be updated to
> remove the unused C code too from the board code.
>
> But the other cases are doing something dynamic and run-time related.
> There's the omap3 igep00x0 family (which yes, legacy) that is doing NAND
> or oneNAND and adjusting things at run time.  I don't know how much
> anyone has interest in those platforms at this point, nor exactly who to
> contact (for Linux or U-Boot). There's also the stm32mp1 family doing
> something that's very not obvious at first glance, so I've cc'd the
> maintainers there.
>

For information, today for stm32mp1 family we are using the build

  of MTDPARTS and fdt fixup, only for backward compatibility issue

(the MTD partitions change for boot with or without OP-TEE,

with or wihtout FIP, with SPL).


Today we are already plan to remove this dynamic management

and to switch to static MTD partition defined in device tree,

as already proposed by Tom in the serie

  "mtd: spi: nor: force mtd name to "nor%d""

http://patchwork.ozlabs.org/project/uboot/patch/20210916155040.v3.2.Ia461e670c7438478aa8f8939209d45c818ccd284@changeid/


This patchset is already ready, we are currently testing it internally

and it should be pushed when it will be validated in our donwstream.


Regards


Patrick







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

Re: [PATCH v1 0/3] fdt: Fix mtparts fixup

[Patrick DELAUNAY]

Hi,

On 1/23/23 21:01, Tom Rini wrote:
> On Mon, Jan 23, 2023 at 11:06:06AM +0100, Miquel Raynal wrote:
>> Hi Tom,
>>
>> trini@konsulko.com wrote on Fri, 13 Jan 2023 14:34:11 -0500:
>>
>>> On Fri, Jan 13, 2023 at 07:45:44PM +0100, Francesco Dolcini wrote:
>>>
>>>> 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 just deprecate using this U-Boot function and pass the MTD partitions
>>>> from the command line, unless they are statically defined in the DTS file
>>>> in the first place.
>>>>
>>>> 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/
>>> My higher level question / concern here is, is using one of the dts
>>> partition schemes still valid / preferred, or should everyone now have
>>> reverted to passing via the kernel command line?  If device tree still,
>>> is mtd/partitions/fixed-partitions.yaml the one to follow or something
>>> else?
>> I don't think we can "prefer" one mode over the other between cmdline
>> and DTS. Both should work pretty well. Of course on the cmdline you can
>> only define fixed partitions and many devices require more advanced
>> parsers, which are only available through DTS, but for simple
>> partitions, it works totally okay.
> When both are present, which one is used?
>
>> The only thing that I would like to avoid is the need to write code in
>> the bootloaders to tweak the FDT in order to add partitions. That is
>> clearly not needed, error prone, and do not follow evolution of the
>> "standard", as we just discovered.
> I'm not sure about this. Looking around in U-Boot today, I see two types
> of cases. One of which, the colibri case, can clearly be not done and
> either passed on the command line, or put in to the device tree as
> there's nothing run-time related being tweaked here. That's a fine path
> to take on those platforms and Francesco's patches should be updated to
> remove the unused C code too from the board code.
>
> But the other cases are doing something dynamic and run-time related.
> There's the omap3 igep00x0 family (which yes, legacy) that is doing NAND
> or oneNAND and adjusting things at run time.  I don't know how much
> anyone has interest in those platforms at this point, nor exactly who to
> contact (for Linux or U-Boot). There's also the stm32mp1 family doing
> something that's very not obvious at first glance, so I've cc'd the
> maintainers there.
>

For information, today for stm32mp1 family we are using the build

  of MTDPARTS and fdt fixup, only for backward compatibility issue

(the MTD partitions change for boot with or without OP-TEE,

with or wihtout FIP, with SPL).


Today we are already plan to remove this dynamic management

and to switch to static MTD partition defined in device tree,

as already proposed by Tom in the serie

  "mtd: spi: nor: force mtd name to "nor%d""

http://patchwork.ozlabs.org/project/uboot/patch/20210916155040.v3.2.Ia461e670c7438478aa8f8939209d45c818ccd284@changeid/


This patchset is already ready, we are currently testing it internally

and it should be pushed when it will be validated in our donwstream.


Regards


Patrick

Re: [PATCH v1 0/3] fdt: Fix mtparts fixup

[Tom Rini]

[-- Attachment #1.1: Type: text/plain, Size: 4502 bytes --]

On Thu, Feb 23, 2023 at 11:23:41AM +0100, Patrick DELAUNAY wrote:
> Hi,
> 
> On 1/23/23 21:01, Tom Rini wrote:
> > On Mon, Jan 23, 2023 at 11:06:06AM +0100, Miquel Raynal wrote:
> > > Hi Tom,
> > > 
> > > trini@konsulko.com wrote on Fri, 13 Jan 2023 14:34:11 -0500:
> > > 
> > > > On Fri, Jan 13, 2023 at 07:45:44PM +0100, Francesco Dolcini wrote:
> > > > 
> > > > > 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 just deprecate using this U-Boot function and pass the MTD partitions
> > > > > from the command line, unless they are statically defined in the DTS file
> > > > > in the first place.
> > > > > 
> > > > > 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/
> > > > My higher level question / concern here is, is using one of the dts
> > > > partition schemes still valid / preferred, or should everyone now have
> > > > reverted to passing via the kernel command line?  If device tree still,
> > > > is mtd/partitions/fixed-partitions.yaml the one to follow or something
> > > > else?
> > > I don't think we can "prefer" one mode over the other between cmdline
> > > and DTS. Both should work pretty well. Of course on the cmdline you can
> > > only define fixed partitions and many devices require more advanced
> > > parsers, which are only available through DTS, but for simple
> > > partitions, it works totally okay.
> > When both are present, which one is used?
> > 
> > > The only thing that I would like to avoid is the need to write code in
> > > the bootloaders to tweak the FDT in order to add partitions. That is
> > > clearly not needed, error prone, and do not follow evolution of the
> > > "standard", as we just discovered.
> > I'm not sure about this. Looking around in U-Boot today, I see two types
> > of cases. One of which, the colibri case, can clearly be not done and
> > either passed on the command line, or put in to the device tree as
> > there's nothing run-time related being tweaked here. That's a fine path
> > to take on those platforms and Francesco's patches should be updated to
> > remove the unused C code too from the board code.
> > 
> > But the other cases are doing something dynamic and run-time related.
> > There's the omap3 igep00x0 family (which yes, legacy) that is doing NAND
> > or oneNAND and adjusting things at run time.  I don't know how much
> > anyone has interest in those platforms at this point, nor exactly who to
> > contact (for Linux or U-Boot). There's also the stm32mp1 family doing
> > something that's very not obvious at first glance, so I've cc'd the
> > maintainers there.
> > 
> 
> For information, today for stm32mp1 family we are using the build
> 
>  of MTDPARTS and fdt fixup, only for backward compatibility issue
> 
> (the MTD partitions change for boot with or without OP-TEE,
> 
> with or wihtout FIP, with SPL).
> 
> 
> Today we are already plan to remove this dynamic management
> 
> and to switch to static MTD partition defined in device tree,
> 
> as already proposed by Tom in the serie
> 
>  "mtd: spi: nor: force mtd name to "nor%d""
> 
> http://patchwork.ozlabs.org/project/uboot/patch/20210916155040.v3.2.Ia461e670c7438478aa8f8939209d45c818ccd284@changeid/
> 
> 
> This patchset is already ready, we are currently testing it internally
> 
> and it should be pushed when it will be validated in our donwstream.

Great, thanks.

-- 
Tom

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

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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

Re: [PATCH v1 0/3] fdt: Fix mtparts fixup

[Tom Rini]

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

On Thu, Feb 23, 2023 at 11:23:41AM +0100, Patrick DELAUNAY wrote:
> Hi,
> 
> On 1/23/23 21:01, Tom Rini wrote:
> > On Mon, Jan 23, 2023 at 11:06:06AM +0100, Miquel Raynal wrote:
> > > Hi Tom,
> > > 
> > > trini@konsulko.com wrote on Fri, 13 Jan 2023 14:34:11 -0500:
> > > 
> > > > On Fri, Jan 13, 2023 at 07:45:44PM +0100, Francesco Dolcini wrote:
> > > > 
> > > > > 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 just deprecate using this U-Boot function and pass the MTD partitions
> > > > > from the command line, unless they are statically defined in the DTS file
> > > > > in the first place.
> > > > > 
> > > > > 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/
> > > > My higher level question / concern here is, is using one of the dts
> > > > partition schemes still valid / preferred, or should everyone now have
> > > > reverted to passing via the kernel command line?  If device tree still,
> > > > is mtd/partitions/fixed-partitions.yaml the one to follow or something
> > > > else?
> > > I don't think we can "prefer" one mode over the other between cmdline
> > > and DTS. Both should work pretty well. Of course on the cmdline you can
> > > only define fixed partitions and many devices require more advanced
> > > parsers, which are only available through DTS, but for simple
> > > partitions, it works totally okay.
> > When both are present, which one is used?
> > 
> > > The only thing that I would like to avoid is the need to write code in
> > > the bootloaders to tweak the FDT in order to add partitions. That is
> > > clearly not needed, error prone, and do not follow evolution of the
> > > "standard", as we just discovered.
> > I'm not sure about this. Looking around in U-Boot today, I see two types
> > of cases. One of which, the colibri case, can clearly be not done and
> > either passed on the command line, or put in to the device tree as
> > there's nothing run-time related being tweaked here. That's a fine path
> > to take on those platforms and Francesco's patches should be updated to
> > remove the unused C code too from the board code.
> > 
> > But the other cases are doing something dynamic and run-time related.
> > There's the omap3 igep00x0 family (which yes, legacy) that is doing NAND
> > or oneNAND and adjusting things at run time.  I don't know how much
> > anyone has interest in those platforms at this point, nor exactly who to
> > contact (for Linux or U-Boot). There's also the stm32mp1 family doing
> > something that's very not obvious at first glance, so I've cc'd the
> > maintainers there.
> > 
> 
> For information, today for stm32mp1 family we are using the build
> 
>  of MTDPARTS and fdt fixup, only for backward compatibility issue
> 
> (the MTD partitions change for boot with or without OP-TEE,
> 
> with or wihtout FIP, with SPL).
> 
> 
> Today we are already plan to remove this dynamic management
> 
> and to switch to static MTD partition defined in device tree,
> 
> as already proposed by Tom in the serie
> 
>  "mtd: spi: nor: force mtd name to "nor%d""
> 
> http://patchwork.ozlabs.org/project/uboot/patch/20210916155040.v3.2.Ia461e670c7438478aa8f8939209d45c818ccd284@changeid/
> 
> 
> This patchset is already ready, we are currently testing it internally
> 
> and it should be pushed when it will be validated in our donwstream.

Great, thanks.

-- 
Tom

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