Re: [PATCH v2 14/15] mtd: rawnand: Get rid of chip->numchips

From: Boris Brezillon <boris.brezillon@collabora.com>
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Mason Yang <masonccyang@mxic.com.tw>, Vignesh R <vigneshr@ti.com>,
	Boris Brezillon <bbrezillon@kernel.org>,
	Julien Su <juliensu@mxic.com.tw>,
	Richard Weinberger <richard@nod.at>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Schrempf Frieder <frieder.schrempf@kontron.de>,
	Marek Vasut <marek.vasut@gmail.com>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	linux-mtd@lists.infradead.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Brian Norris <computersforpeace@gmail.com>,
	David Woodhouse <dwmw2@infradead.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 14/15] mtd: rawnand: Get rid of chip->numchips
Date: Tue, 21 May 2019 09:43:05 +0200	[thread overview]
Message-ID: <20190521094305.3769f1a2@collabora.com> (raw)
In-Reply-To: <20190521093302.079f5470@collabora.com>

On Tue, 21 May 2019 09:33:02 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Tue, 21 May 2019 08:59:48 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > Hi,
> > 
> > On Mon, Mar 04, 2019 at 09:15:21PM +0100, Miquel Raynal wrote:  
> > > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> > > index fbf6ca015cd7..a204f9d7e123 100644
> > > --- a/drivers/mtd/nand/raw/internals.h
> > > +++ b/drivers/mtd/nand/raw/internals.h
> > > @@ -110,7 +110,7 @@ static inline int nand_exec_op(struct nand_chip *chip,
> > >  	if (!nand_has_exec_op(chip))
> > >  		return -ENOTSUPP;
> > >  
> > > -	if (WARN_ON(op->cs >= chip->numchips))
> > > +	if (WARN_ON(op->cs >= nanddev_ntargets(&chip->base)))
> > >  		return -EINVAL;    
> > 
> > This warning triggers when I apply my gpmi nand exec_op series.
> > 
> > The gpmi driver calls:
> > 
> > 	ret = nand_scan(chip, GPMI_IS_MX6(this) ? 2 : 1);
> > 
> > This ends up in nand_scan_ident() with maxchips = 2. Here nand_detect()
> > is called which sets memorg->ntargets = 1; Later in nand_scan_ident() we
> > have:
> > 
> > 	for (i = 1; i < maxchips; i++) {  
> 
> This loop should be fixed to test against nanddev_ntargets() instead of
> maxchips.
> 
> > 		u8 id[2];
> > 
> > 		/* See comment in nand_get_flash_type for reset */
> > 		ret = nand_reset(chip, i);
> > 		if (ret)
> > 			break;
> > 		....
> > 
> > this nand_reset() calls nand_exec_op() with op->cs = 1, nanddev_ntargets() = 1
> > and boom.
> > 
> > I can't see how this can work with anything else but maxchips = 1. Do you
> > have an idea how this is supposed to work?

Forgot to reply to that one. ->ntargets is set to the number of
dies/tartgets actually detected here [1], so it's not always 1 (can
also be extracted from the ONFI table IIRC).
Note that I've never been a big fan of this maxchip param, and I've
asked that new drivers pass the actual number of CS connected to the
NAND chip being initialized (which should be part of the HW desc, be it
DT based or board-file based). So, ideally this argument should be named
num_dies or num_targets and the function should return an error when
one of the die returns a different ID. Unfortunately, that's not
something we can do, because a lot of drivers rely on the old
semantic...

[1]https://elixir.bootlin.com/linux/v5.2-rc1/source/drivers/mtd/nand/raw/nand_base.c#L5073

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel