[PATCH v1 1/3] fdt: validate/fix cells count on mtdpart fixup
Francesco Dolcini
francesco at dolcini.it
Mon Jan 16 10:00:16 PST 2023
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 at 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 at denx.de>
> > > > Cc: Miquel Raynal <miquel.raynal at bootlin.com>
> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini at 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 at 33002000 {
> > compatible = "fsl,imx7d-gpmi-nand";
> >
> > #address-cells = <0x01>;
> > #size-cells = <0x01>;
> >
> > [...snip...]
> >
> > partition at 0 {
> > label = "mx7-bcb";
> > reg = <0x00 0x80000>;
> > };
> >
> > partition at 400000 {
> > label = "ubi";
> > reg = <0x400000 0x1fc00000>;
> > };
> >
> > partition at 80000 {
> > read_only;
> > label = "u-boot1";
> > reg = <0x80000 0x180000>;
> > };
> >
> > partition at 380000 {
> > label = "u-boot-env";
> > reg = <0x380000 0x80000>;
> > };
> >
> > partition at 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
More information about the linux-mtd
mailing list