| Message ID | 20230113184547.487322-2-francesco@dolcini.it |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fdt: Fix mtparts fixup | expand |
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 ?
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>; }; };
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 ?
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
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.
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
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
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
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
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 */ } }