Re: [PATCH v2 09/11] mtd: rawnand: Expose monolithic read/write_page_raw() helpers

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

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 29 Apr
2020 18:15:09 +0200:

> On Wed, 29 Apr 2020 17:55:38 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > The current nand_read/write_page_raw() helpers are already widely used
> > but do not fit the purpose of "constrained" controllers which cannot,
> > for instance, separate command/address cycles with data cycles.
> > 
> > Workaround this issue by proposing alternative helpers that cannot be
> > used by controller drivers instead.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_base.c | 60 ++++++++++++++++++++++++++++++++
> >  include/linux/mtd/rawnand.h      |  8 +++--
> >  2 files changed, 66 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index 15a9189b2307..2e525cb5a4e4 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -2629,6 +2629,39 @@ int nand_read_page_raw(struct nand_chip *chip, uint8_t *buf, int oob_required,
> >  }
> >  EXPORT_SYMBOL(nand_read_page_raw);
> >  
> > +/**
> > + * nand_monolithic_read_page_raw - Read raw page data without ECC in one go  
> 
> Maybe
> 
> "Read the full page (data + OOB) with ECC engine disabled"
> 
> ?

This is not accurate as we don't enforce OOB read.

Don't you find "in one go" explicit enough?

> 
> > + * @chip: nand chip info structure
> > + * @buf: buffer to store read data
> > + * @oob_required: caller requires OOB data read to chip->oob_poi
> > + * @page: page number to read
> > + */
> > +int nand_monolithic_read_page_raw(struct nand_chip *chip, u8 *buf,
> > +				  int oob_required, int page)
> > +{
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +	unsigned int size = mtd->writesize;
> > +	u8 *read_buf = buf;
> > +	int ret;
> > +
> > +	if (oob_required) {
> > +		size += mtd->oobsize;
> > +
> > +		if (buf != chip->data_buf)
> > +			read_buf = nand_get_data_buf(chip);
> > +	}
> > +
> > +	ret = nand_read_page_op(chip, page, 0, read_buf, size);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (buf != chip->data_buf)
> > +		memcpy(buf, read_buf, mtd->writesize);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(nand_monolithic_read_page_raw);
> > +
> >  /**
> >   * nand_read_page_raw_syndrome - [INTERN] read raw page data without ecc
> >   * @chip: nand chip info structure
> > @@ -3636,6 +3669,33 @@ int nand_write_page_raw(struct nand_chip *chip, const uint8_t *buf,
> >  }
> >  EXPORT_SYMBOL(nand_write_page_raw);
> >  
> > +/**
> > + * nand_monolithic_write_page_raw - Raw page write in one go  
> 
> Maybe we should have a consistent description for those helpers:
> 
> "Write the full page (data + OOB) with ECC engine disabled"
> 
> ?*
> 
> > + * @chip: NAND chip info structure
> > + * @buf: data buffer
> > + * @oob_required: must write chip->oob_poi to OOB
> > + * @page: page number to write
> > + */
> > +int nand_monolithic_write_page_raw(struct nand_chip *chip, const u8 *buf,
> > +				   int oob_required, int page)
> > +{
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +	unsigned int size = mtd->writesize;
> > +	u8 *write_buf = (u8 *)buf;
> > +
> > +	if (oob_required) {
> > +		size += mtd->oobsize;
> > +
> > +		if (buf != chip->data_buf) {
> > +			write_buf = nand_get_data_buf(chip);
> > +			memcpy(write_buf, buf, mtd->writesize);
> > +		}
> > +	}
> > +
> > +	return nand_prog_page_op(chip, page, 0, write_buf, size);
> > +}
> > +EXPORT_SYMBOL(nand_monolithic_write_page_raw);
> > +
> >  /**
> >   * nand_write_page_raw_syndrome - [INTERN] raw page write function
> >   * @chip: nand chip info structure
> > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > index d40ad19ba0f6..0da10e99cf39 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -1328,13 +1328,17 @@ int nand_read_oob_std(struct nand_chip *chip, int page);
> >  int nand_get_set_features_notsupp(struct nand_chip *chip, int addr,
> >  				  u8 *subfeature_param);
> >  
> > -/* Default read_page_raw implementation */
> > +/* Default read_page_raw implementations */  
> 
> Well, nand_monolithic_read_page_raw() is not the default :p. Just drop
> the "Default " and it should be good.

Fair enough :)

> 
> >  int nand_read_page_raw(struct nand_chip *chip, uint8_t *buf, int oob_required,
> >  		       int page);
> > +int nand_monolithic_read_page_raw(struct nand_chip *chip, uint8_t *buf,
> > +				  int oob_required, int page);
> >  
> > -/* Default write_page_raw implementation */
> > +/* Default write_page_raw implementations */  
> 
> Ditto.
> 
> >  int nand_write_page_raw(struct nand_chip *chip, const uint8_t *buf,
> >  			int oob_required, int page);
> > +int nand_monolithic_write_page_raw(struct nand_chip *chip, const uint8_t *buf,
> > +				   int oob_required, int page);
> >  
> >  /* Reset and initialize a NAND device */
> >  int nand_reset(struct nand_chip *chip, int chipnr);  
> 




Thanks,
Miquèl

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