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

[Boris Brezillon <boris.brezillon@collabora.com>]

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.

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