Re: [PATCH 3/4] mtd: spinand: Add support continuous read operation

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

Hello,

zhengxunli.mxic@gmail.com wrote on Fri,  8 Oct 2021 14:57:58 +0800:

> The patch adds a continuous read start flag to support continuous
> read operations. The continuous read operation only issues a page
> read command (13h), issues multiple read commands from the cache
> (03h/0Bh/3Bh/6Bh/BBh/EBh) to read continuous address data, and
> finally issues an exit continuous read command (63h) to terminate
> this continuous read operation.
> 
> Since the continuous read mode can only read the entire page of data
> (2KB)

Remove this size, it is highly unlikely that all SPI NAND devices will
ever be restricted to 2kiB right?

> and cannot read the oob data,

This is something that you seem to skip to check in your series.

> the dynamic change mode is added
> to enable continuous read mode and disable continuous read mode in
> spinand_mtd_read to avoid writing and erasing operation is abnormal.
> 
> Signed-off-by: Zhengxun <zhengxunli.mxic@gmail.com>
> ---
>  drivers/mtd/nand/spi/core.c | 38 +++++++++++++++++++++++++++++---------
>  include/linux/mtd/spinand.h |  2 ++
>  2 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 0d9632f..0369453 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -195,6 +195,8 @@ static int spinand_init_quad_enable(struct spinand_device *spinand)
>  
>  static int spinand_continuous_read_enable(struct spinand_device *spinand)
>  {
> +	spinand->cont_read_start = false;

I really don't like the "= false" in the "read_enable" hook. Why not
just checking directly in mtd_read and drop that boolean ?

> +
>  	if (!(spinand->flags & SPINAND_HAS_CONT_READ_BIT))
>  		return 0;
>  
> @@ -598,16 +600,22 @@ static int spinand_read_page(struct spinand_device *spinand,
>  	if (ret)
>  		return ret;
>  
> -	ret = spinand_load_page_op(spinand, req);
> -	if (ret)
> -		return ret;
> +	if (!spinand->cont_read_start) {

I don't get this check. This condition will always be true. You can
drop it.

>  
> -	ret = spinand_wait(spinand,
> -			   SPINAND_READ_INITIAL_DELAY_US,
> -			   SPINAND_READ_POLL_DELAY_US,
> -			   &status);
> -	if (ret < 0)
> -		return ret;
> +		ret = spinand_load_page_op(spinand, req);
> +		if (ret)
> +			return ret;
> +
> +		ret = spinand_wait(spinand,
> +				   SPINAND_READ_INITIAL_DELAY_US,
> +				   SPINAND_READ_POLL_DELAY_US,
> +				   &status);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (spinand->flags & SPINAND_HAS_CONT_READ_BIT)
> +			spinand->cont_read_start = true;
> +	}
>  
>  	spinand_ondie_ecc_save_status(nand, status);
>  
> @@ -667,6 +675,10 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
>  
>  	mutex_lock(&spinand->lock);
>  
> +	ret = spinand_continuous_read_enable(spinand);
> +	if (ret)
> +		return ret;
> +
>  	nanddev_io_for_each_page(nand, NAND_PAGE_READ, from, ops, &iter) {
>  		if (disable_ecc)
>  			iter.req.mode = MTD_OPS_RAW;
> @@ -689,6 +701,14 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
>  		ops->oobretlen += iter.req.ooblen;
>  	}
>  
> +	ret = spinand_continuous_read_exit(spinand);
> +	if (ret)
> +		return ret;
> +
> +	ret = spinand_continuous_read_disable(spinand);
> +	if (ret)
> +		return ret;

The asymmetry here looks strange. Where do we actually enter the
continuous read mode?

Do you have any indicators that this change improves the performances?
It would be good to share them in the commit log.

> +
>  	mutex_unlock(&spinand->lock);
>  
>  	if (ecc_failed && !ret)
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index e044aba..c2a41a3 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -422,6 +422,7 @@ struct spinand_dirmap {
>   *		because the spi-mem interface explicitly requests that buffers
>   *		passed in spi_mem_op be DMA-able, so we can't based the bufs on
>   *		the stack
> + * @cont_read_start: record the continuous read status
>   * @manufacturer: SPI NAND manufacturer information
>   * @priv: manufacturer private data
>   */
> @@ -450,6 +451,7 @@ struct spinand_device {
>  	u8 *databuf;
>  	u8 *oobbuf;
>  	u8 *scratchbuf;
> +	bool cont_read_start;
>  	const struct spinand_manufacturer *manufacturer;
>  	void *priv;
>  };


Thanks,
Miquèl

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