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 018C9C64EC4 for ; Fri, 3 Feb 2023 15:12:15 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id E049385F53; Fri, 3 Feb 2023 16:12:13 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=bootlin.com header.i=@bootlin.com header.b="VU3LhwPO"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 00F8085D17; Fri, 3 Feb 2023 16:12:12 +0100 (CET) Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) (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 24F1A8545E for ; Fri, 3 Feb 2023 16:12:07 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=miquel.raynal@bootlin.com Received: (Authenticated sender: miquel.raynal@bootlin.com) by mail.gandi.net (Postfix) with ESMTPSA id BACBF4000B; Fri, 3 Feb 2023 15:12:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1675437126; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=fgonYy5kf0HaJcOsNAlLRss28D6PMmH9gqHFpX6dh2A=; b=VU3LhwPORdLdOo9J3/3gGJ1WgBCWKGJsOTr2bJDho2mIByMUvrpRzWC0dXrrcHgR4sgt1U GJhFbRe8kilQk9HqxH2uODDKDrOsuaCm+AHvkHlCOL07/eKGPY4FgghklIQCr8BkW1nztu NV8TpHq69bVdmEh20BM7g0GEkYZv38YL+A3YYw8OdQW90/3ZS/duUXIWMBNMyG6Z/pobEz mxb1Eucf+he1UNIVzko/estx/OBrAxiN8g3Mgkk+za+g0DCdPjPBpp8I26/PTq9rAeyQiA kBpkFymDRH2SPOsT4dl1OsR9bwkoYpHxSv3hRK1nUMqH2bmdY8U4qPReTFRHAA== Date: Fri, 3 Feb 2023 16:12:02 +0100 From: Miquel Raynal To: Francesco Dolcini Cc: Greg Kroah-Hartman , linux-mtd@lists.infradead.org, Richard Weinberger , Vignesh Raghavendra , Marek Vasut , Francesco Dolcini , u-boot@lists.denx.de Subject: Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0 Message-ID: <20230203161202.76f3b057@xps-13> In-Reply-To: References: <20230124104444.330913-1-francesco@dolcini.it> <20230126101204.245ace0d@xps-13> Organization: Bootlin X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 Hi Francesco, francesco@dolcini.it wrote on Thu, 2 Feb 2023 12:33:34 +0100: > On Thu, Jan 26, 2023 at 10:12:04AM +0100, Miquel Raynal wrote: > > gregkh@linuxfoundation.org wrote on Thu, 26 Jan 2023 10:01:02 +0100: > > =20 > > > On Wed, Jan 25, 2023 at 10:06:57PM +0100, Francesco Dolcini wrote: =20 > > > > Hello Miquel, Greg and all > > > >=20 > > > > On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:= =20 > > > > > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote= : =20 > > > > > > From: Francesco Dolcini > > > > > >=20 > > > > > > Add a mechanism to handle the case in which partitions are pres= ent as > > > > > > direct child of the nand controller node and #size-cells is set= to <0>. > > > > > >=20 > > > > > > This could happen if the nand-controller node in the DTS is sup= posed to > > > > > > have #size-cells set to 0, but for some historical reason/bug i= t was set > > > > > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the = partition > > > > > > as direct children of the nand-controller defaulting to #size-c= ells > > > > > > being to 1. > > > > > >=20 > > > > > > This prevents a real boot failure on colibri-imx7 that happened= during v6.1 > > > > > > development cycles. > > > > > >=20 > > > > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb= .int.toradex.com/ > > > > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-fran= cesco@dolcini.it/ > > > > > > Signed-off-by: Francesco Dolcini > > > > > > Reviewed-by: Greg Kroah-Hartman > > > > > > --- > > > > > > I do not expect this patch to be backported to stable, however = I would expect > > > > > > that we do not backport nand-controller dts cleanups neither. > > > > > >=20 > > > > > > v4: > > > > > > fixed wrong English spelling in the comment > > > > > >=20 > > > > > > v3: > > > > > > minor formatting change, removed not needed new-line and space= .=20 > > > > > >=20 > > > > > > v2: > > > > > > fixup size-cells only when partitions are direct children of t= he nand-controller > > > > > > completely revised commit message, comments and warning print > > > > > > use pr_warn instead of pr_warn_once > > > > > > added Reviewed-by Greg > > > > > > removed cc:stable@ and fixes tag, since the problematic commit= was reverted > > > > > > --- > > > > > > drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++ > > > > > > 1 file changed, 19 insertions(+) > > > > > >=20 > > > > > > diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/pa= rsers/ofpart_core.c > > > > > > index 192190c42fc8..e7b8e9d0a910 100644 > > > > > > --- a/drivers/mtd/parsers/ofpart_core.c > > > > > > +++ b/drivers/mtd/parsers/ofpart_core.c > > > > > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct m= td_info *master, > > > > > > =20 > > > > > > a_cells =3D of_n_addr_cells(pp); > > > > > > s_cells =3D of_n_size_cells(pp); > > > > > > + if (!dedicated && s_cells =3D=3D 0) { > > > > > > + /* > > > > > > + * This is a ugly workaround to not create > > > > > > + * regression on devices that are still creating > > > > > > + * partitions as direct children of the nand controller. > > > > > > + * This can happen in case the nand controller node has > > > > > > + * #size-cells equal to 0 and the firmware (e.g. > > > > > > + * U-Boot) just add the partitions there assuming > > > > > > + * 32-bit addressing. > > > > > > + * > > > > > > + * If you get this warning your firmware and/or DTS > > > > > > + * should be really fixed. > > > > > > + * > > > > > > + * This is working only for devices smaller than 4GiB. > > > > > > + */ > > > > > > + pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wr= ongly set to <0>, assuming <1> for parsing partitions.\n", > > > > > > + master->name, pp, mtd_node); =20 > > > > >=20 > > > > > This is a driver, always use dev_*() calls, not pr_*() calls so t= hat we > > > > > know what is being referred to exactly. =20 > > > >=20 > > > > Is this reasonable here? Where can I get the struct device? =20 > > >=20 > > > Walk back up the call chain, there has to be a device somewhere > > > controlling this, right? > > > =20 > > > > In general this file uses only pr_* debug API and messages are abou= t OF > > > > nodes/properties, not about a device. =20 > > >=20 > > > OF nodes and properties are part of a device's properties :) =20 > >=20 > > Yes but the warning comes from a wrong DT description, hence it felt > > better suited to warn against the node name which is easily identifiable > > in a text file and must be fixed rather than the device which is a pure > > software component. > >=20 > > Anyway, Francesco, please show us the resultant line and if it feels > > meaningful enough we'll take the dev_warn approach. =20 >=20 > So, I tried, but I guess I failed. >=20 > Both >=20 > dev_warn(&mtd_get_master(master)->dev, ...); >=20 > and >=20 > dev_warn(&master->dev, ...); >=20 > are NULL. mtd->dev (in raw NAND) is populated by the controller drivers, so the master mtd device is pointing to the bus "struct device" in its dev.parent field. This happens at the end of the probe of the controller, after setting the dev entry, so we expect the name of the controller to appear. > (null): gpmi-nand: ofpart partition /soc/nand-controller@33002000/partiti= on@0 (/soc/nand-controller@33002000) #size-cells is wrongly set to <0>, ass= uming <1> for parsing partitions. Second field looks right, first field does not (bus or class id?) I have no idea why it has not been populated at this point (end of the controller probe). But it's not a big deal, at least we have the device name, so it's ok for me. > while the current v4 is just: >=20 > gpmi-nand: ofpart partition /soc/nand-controller@33002000/partition@0 (/s= oc/nand-controller@33002000) #size-cells is wrongly set to <0>, assuming <1= > for parsing partitions. >=20 > on a colibri-imx7. >=20 > Any advice? >=20 > Francesco >=20 Thanks, Miqu=C3=A8l