Re: [PATCH 12/17] mtd: rawnand: cafe: Don't split things when reading/writing a page

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Tue, 28 Apr
2020 08:20:24 +0200:

> On Mon, 27 Apr 2020 21:53:53 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Boris,
> > 
> > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Apr
> > 2020 10:20:22 +0200:
> >   
> > > Calling nand_read_page_op(pagesize)/nand_prog_page_begin_op(pagesize)
> > > and expecting to get a pagesize+oobsize read from/written to the
> > > read/write buffer is fragile and only works because of hacks done
> > > in cmdfunc(). Let's read/write the page in one go, using the page
> > > cache buffer as a bounce buffer.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > >  drivers/mtd/nand/raw/cafe_nand.c | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/cafe_nand.c b/drivers/mtd/nand/raw/cafe_nand.c
> > > index 31493a201a02..edf65197604b 100644
> > > --- a/drivers/mtd/nand/raw/cafe_nand.c
> > > +++ b/drivers/mtd/nand/raw/cafe_nand.c
> > > @@ -472,6 +472,7 @@ static int cafe_nand_read_page(struct nand_chip *chip, uint8_t *buf,
> > >  {
> > >  	struct mtd_info *mtd = nand_to_mtd(chip);
> > >  	struct cafe_priv *cafe = nand_get_controller_data(chip);
> > > +	void *pagebuf = nand_get_data_buf(chip);
> > >  	unsigned int max_bitflips = 0;
> > >  	u32 ecc_result, status;
> > >  
> > > @@ -479,8 +480,11 @@ static int cafe_nand_read_page(struct nand_chip *chip, uint8_t *buf,
> > >  		cafe_readl(cafe, NAND_ECC_RESULT),
> > >  		cafe_readl(cafe, NAND_ECC_SYN_REG(0)));
> > >  
> > > -	nand_read_page_op(chip, page, 0, buf, mtd->writesize);
> > > -	chip->legacy.read_buf(chip, chip->oob_poi, mtd->oobsize);
> > > +	nand_read_page_op(chip, page, 0, pagebuf,
> > > +			  mtd->writesize + mtd->oobsize);
> > > +
> > > +	if (buf != pagebuf)
> > > +		memcpy(buf, pagebuf, mtd->writesize);
> > >  
> > >  	ecc_result = cafe_readl(cafe, NAND_ECC_RESULT);
> > >  	status = CAFE_FIELD_GET(NAND_ECC_RESULT, STATUS, ecc_result);
> > > @@ -642,15 +646,17 @@ static int cafe_nand_write_page(struct nand_chip *chip,
> > >  {
> > >  	struct mtd_info *mtd = nand_to_mtd(chip);
> > >  	struct cafe_priv *cafe = nand_get_controller_data(chip);
> > > +	void *pagebuf = nand_get_data_buf(chip);
> > >  	int ret;
> > >  
> > > -	nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize);
> > > -	chip->legacy.write_buf(chip, chip->oob_poi, mtd->oobsize);
> > > +	if (pagebuf != buf)
> > > +		memcpy(pagebuf, buf, mtd->writesize);
> > >  
> > >  	/* Set up ECC autogeneration */
> > >  	cafe->ctl2 |= CAFE_NAND_CTRL2_AUTO_WRITE_ECC;
> > >  
> > > -	ret = nand_prog_page_end_op(chip);
> > > +	ret = nand_prog_page_op(chip, page, 0, pagebuf,
> > > +				mtd->writesize + mtd->oobsize);
> > >  
> > >  	/*
> > >  	 * And clear it before returning so that following write operations    
> > 
> > 
> > You are replacing ->read/write_buf() calls into memcpy() calls,
> > shouldn't this be cleaned before? or at least mentioned?  
> 
> Actually, those read/write_buf are still there, they're just hidden in
> the nand_{prog,read}_page_op() call now (to be accurate, the read/write
> buf in there now covers the data and oob portions). It's really what
> should be done, the reason this worked so far is because cmdfunc()
> guesses that the full page will be read/written and issues a read/write
> of the data+oob portion. The extra memcpy that's added here is done to
> account for the fact that the core might pass 2 different buffers for
> OOB and data, but we want things to be done in one step, so we're using
> the bounce buffer to do the transfer.

Now that you mention it I fully understand.

Thanks for the clarification, maybe you can add these details to the
commit log.


Thanks,
Miquèl

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