From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D15EEC46467 for ; Mon, 16 Jan 2023 18:00:31 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C4107855A4; Mon, 16 Jan 2023 19:00:28 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=dolcini.it Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=mailserver.it header.i=@mailserver.it header.b="2ZytoT2h"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 1CE428559B; Mon, 16 Jan 2023 19:00:27 +0100 (CET) Received: from smtp-out-05.comm2000.it (smtp-out-05.comm2000.it [212.97.32.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 19E7F855AC for ; Mon, 16 Jan 2023 19:00:22 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=dolcini.it Authentication-Results: phobos.denx.de; spf=none smtp.mailfrom=francesco@dolcini.it Received: from francesco-nb.int.toradex.com (31-10-206-125.static.upc.ch [31.10.206.125]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: francesco@dolcini.it) by smtp-out-05.comm2000.it (Postfix) with ESMTPSA id F361B823801; Mon, 16 Jan 2023 19:00:17 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mailserver.it; s=mailsrv; t=1673892021; bh=Ht4j5zvsKCpLpenGypyh8flQGV1XZhR+PS4DLmnVTVI=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=2ZytoT2hJlY3+h10c5cQg74E3WMmrs5BxYAocuLVpyMyV4yCkMWBGdbnJd9j72+O0 z7Hnzug3RyUCj9Sxdb8EIlLg67BgOQIrEAPEvbNZlg8t/PI+dQiTbBjF7xmhzK6/C6 YwbXs3VEwMf16kfmdNULaoEng2UaeM7qXHK47rNs6PzbhdFMAXqnQt6NfSsIWn1ZUm e3Q8Vtk00rvW063ZzKLOE6ZLzT8iKK1pQYMpP+mmZ6Elp8jt8xjrQJd/syPtkqOjHE Y456cXMWv03uw4cQ08D9UwSJjTjO+R9g9ovlSD9mZFtt/A3eOwvsUxvg1ZvH0/NDJ+ 8AQiJETU/CNLQ== Date: Mon, 16 Jan 2023 19:00:16 +0100 From: Francesco Dolcini To: Marek Vasut Cc: Francesco Dolcini , Simon Glass , u-boot@lists.denx.de, Marcel Ziswiler , Francesco Dolcini , Miquel Raynal , linux-mtd@lists.infradead.org Subject: Re: [PATCH v1 1/3] fdt: validate/fix cells count on mtdpart fixup Message-ID: References: <20230113184547.487322-1-francesco@dolcini.it> <20230113184547.487322-2-francesco@dolcini.it> <9ed3b4e4-7336-baf3-5a5a-8e77543e210d@denx.de> <06279fe3-a3b3-f3c4-276f-7cd0d4773875@denx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <06279fe3-a3b3-f3c4-276f-7cd0d4773875@denx.de> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean 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 > > > > > > > > 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 > > > > Cc: Miquel Raynal > > > > Signed-off-by: Francesco Dolcini > > > > --- > > > > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id ED867C54EBE for ; Mon, 16 Jan 2023 18:01:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Xu0m3kSPcFZyeY1wuirGjFMIoQMc/GcsZk/vhZaz8WY=; b=1lysSuSME8nfnZ 7DUxs5trFUiOqyStSmzprBV81J+d6ePZ1tWX9QqIwL8FnnmfzPw2ydKYbrvR8W8z1YN6euSbAjjYb oT6IMUOXvRIupIa+yXvIPxgGRS0kGRKE9EfD1p6qkzzOF3t7RkNpKbyNFtPzxvvFnL3gE1n4y1b1g Sf8LdJEduYfZ6vSfvQ5Do+G2Vq6YXDbOBUAwNL9YhvQLYWnBOHFRjmKUHG4XdMF7QEmfjpH5llTqJ g6Lo1qiJlOr99/3XL8Tpfsxexb58G/FU9oaSogoLT4Q/GmhMNBwz9M745ZaWjqySrDgWcn8ljmo8P yqCgMZXIzpaavWq0hEgw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pHTmo-00Bcx2-Iu; Mon, 16 Jan 2023 18:00:46 +0000 Received: from smtp-out-05.comm2000.it ([212.97.32.73]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pHTmk-00BcvF-S5 for linux-mtd@lists.infradead.org; Mon, 16 Jan 2023 18:00:45 +0000 Received: from francesco-nb.int.toradex.com (31-10-206-125.static.upc.ch [31.10.206.125]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: francesco@dolcini.it) by smtp-out-05.comm2000.it (Postfix) with ESMTPSA id F361B823801; Mon, 16 Jan 2023 19:00:17 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mailserver.it; s=mailsrv; t=1673892021; bh=Ht4j5zvsKCpLpenGypyh8flQGV1XZhR+PS4DLmnVTVI=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=2ZytoT2hJlY3+h10c5cQg74E3WMmrs5BxYAocuLVpyMyV4yCkMWBGdbnJd9j72+O0 z7Hnzug3RyUCj9Sxdb8EIlLg67BgOQIrEAPEvbNZlg8t/PI+dQiTbBjF7xmhzK6/C6 YwbXs3VEwMf16kfmdNULaoEng2UaeM7qXHK47rNs6PzbhdFMAXqnQt6NfSsIWn1ZUm e3Q8Vtk00rvW063ZzKLOE6ZLzT8iKK1pQYMpP+mmZ6Elp8jt8xjrQJd/syPtkqOjHE Y456cXMWv03uw4cQ08D9UwSJjTjO+R9g9ovlSD9mZFtt/A3eOwvsUxvg1ZvH0/NDJ+ 8AQiJETU/CNLQ== Date: Mon, 16 Jan 2023 19:00:16 +0100 From: Francesco Dolcini To: Marek Vasut Cc: Francesco Dolcini , Simon Glass , u-boot@lists.denx.de, Marcel Ziswiler , Francesco Dolcini , Miquel Raynal , linux-mtd@lists.infradead.org Subject: Re: [PATCH v1 1/3] fdt: validate/fix cells count on mtdpart fixup Message-ID: References: <20230113184547.487322-1-francesco@dolcini.it> <20230113184547.487322-2-francesco@dolcini.it> <9ed3b4e4-7336-baf3-5a5a-8e77543e210d@denx.de> <06279fe3-a3b3-f3c4-276f-7cd0d4773875@denx.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <06279fe3-a3b3-f3c4-276f-7cd0d4773875@denx.de> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230116_100043_065477_4A82B60E X-CRM114-Status: GOOD ( 48.14 ) X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-mtd" Errors-To: linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org 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 > > > > > > > > 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 > > > > Cc: Miquel Raynal > > > > Signed-off-by: Francesco Dolcini > > > > --- > > > > 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/