Re: [PATCH v2 08/11] mtd: rawnand: jedec: Adapt the parameter page read to constraint controllers

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Michal Simek <monstr@monstr.eu>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Naga Sureshkumar Relli <nagasure@xilinx.com>
Subject: Re: [PATCH v2 08/11] mtd: rawnand: jedec: Adapt the parameter page read to constraint controllers
Date: Mon, 4 May 2020 10:02:45 +0200	[thread overview]
Message-ID: <20200504100245.6fa81a31@xps13> (raw)
In-Reply-To: <20200503214403.5f68fb93@collabora.com>

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 3 May
2020 21:44:03 +0200:

> On Sun, 3 May 2020 21:06:23 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Boris,
> > 
> > Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 29 Apr
> > 2020 18:04:05 +0200:
> >   
> > > On Wed, 29 Apr 2020 17:55:37 +0200
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >     
> > > > We already know that there are controllers not able to read the three
> > > > copies of the parameter page in one go. The workaround was to first
> > > > request the controller to assert command and address cycles on the
> > > > NAND bus to trigger a parameter page read, and then do a read
> > > > operation for each page.
> > > > 
> > > > But there are also controllers which are not able to split the
> > > > parameter page read between the command/address cycles and the actual
> > > > data operation.
> > > > 
> > > > All controllers are expected to be able to change the read column
> > > > though. So let's use a regular parameter page read operation for the
> > > > first iteration and use a change read column operation for the
> > > > following copies.
> > > > 
> > > > The extra command and address cycles sent over the NAND bus are
> > > > negligible compared to the amount of data that is being transferred
> > > > anyway.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/mtd/nand/raw/nand_jedec.c | 28 +++++++++++++++-------------
> > > >  1 file changed, 15 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/nand/raw/nand_jedec.c b/drivers/mtd/nand/raw/nand_jedec.c
> > > > index 15937e02c64f..b2be9056759a 100644
> > > > --- a/drivers/mtd/nand/raw/nand_jedec.c
> > > > +++ b/drivers/mtd/nand/raw/nand_jedec.c
> > > > @@ -25,7 +25,7 @@ int nand_jedec_detect(struct nand_chip *chip)
> > > >  {
> > > >  	struct mtd_info *mtd = nand_to_mtd(chip);
> > > >  	struct nand_memory_organization *memorg;
> > > > -	struct nand_jedec_params *p;
> > > > +	struct nand_jedec_params *p = NULL, *pbuf;      
> > > 
> > > Looks like you've merged 2 different commits here. I remember you had a
> > > separate commit adding pbuf to avoid an extra memcpy().    
> > 
> > Actually this was only fixed in the onfi detection routine. The jedec
> > equivalent does not copy the page (the copy came from the 3-way merge
> > that has only been added to onfi detection).
> > 
> > Here pbuf represents the buffer containing the three pages while p now
> > only points to the actual page that is correct.  
> 
> Oh, I see. Looks like the p = pbuf assignment is wrong BTW (should be
> p = &pbuf[i]).

Good catch!

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/