RE: [PATCH] mtd: spi-nor: honour max_data_size for spi-nor writes

[Yogesh Narayan Gaur]

Hi Boris,


-----Original Message-----
From: Boris Brezillon [mailto:boris.brezillon@xxxxxxxxxxx] 
Sent: Monday, June 11, 2018 3:19 PM
To: Yogesh Narayan Gaur <yogeshnarayan.gaur@xxxxxxx>
Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx; boris.brezillon@xxxxxxxxxxxxxxxxxx; frieder.schrempf@xxxxxxxxx; computersforpeace@xxxxxxxxx; David Wolfe <david.wolfe@xxxxxxx>; Han Xu <han.xu@xxxxxxx>; festevam@xxxxxxxxx; marek.vasut@xxxxxxxxx; Prabhakar Kushwaha <prabhakar.kushwaha@xxxxxxx>; linux-spi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; NeilBrown <neil@xxxxxxxxxx>
Subject: Re: [PATCH] mtd: spi-nor: honour max_data_size for spi-nor writes

Hi Yogesh,

Unrelated note: no need to send your patches to both boris.brezillo@xxxxxxxxxxxxxxxxxx and boris.brezillo@xxxxxxxxxxx, just use the latter.

Ok, Sure.

On Mon, 11 Jun 2018 14:48:14 +0530
Yogesh Gaur <yogeshnarayan.gaur@xxxxxxx> wrote:

> Honour max_data_size for spi-nor writes

^ This is no longer relevant.

> In new spi-mem framework, data size to be written is being calculated 
> using spi_mem_adjust_op_size().
> This can return value less than nor->page_size.
> 
> Add check value of data size return from API spi_mem_adjust_op_size() 
> with the actual requested data size and write, max, only supported 
> data size.

This part is not clear.

Also, I'd prefer to have this patch split in 2:
1/ one patch removing the check in spi_nor_write() 2/ and the second patch removing the while() loop in m25p80_write()

How about the following commit messages for those 2 patches:

1:
"
mtd: spi-nor: Support controllers with limited TX FIFO size

Some SPI controllers can't write nor->page_size bytes in a single step because their TX FIFO is too small.

Allow nor->write() to return a size that is smaller than the requested write size to gracefully handle this case.
"

2:
"
mtd: devices: m25p80: Make sure WRITE_EN is issued before each write

Some SPI controllers can't write nor->page_size bytes in a single step because their TX FIFO is too small, but when that happens we should make sure a WRITE_EN command is issued before each write access.

The core is already taking care of that, so all we have to do here is return the actual number of bytes that were written during the
spi_mem_exec_op() operation.
"

Both patches send [1][2]

--
Regards
Yogesh Gaur.

> 
> Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@xxxxxxx>
> ---
>  drivers/mtd/devices/m25p80.c  | 23 ++++++++---------------  
> drivers/mtd/spi-nor/spi-nor.c |  7 -------
>  2 files changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c 
> b/drivers/mtd/devices/m25p80.c index e84563d..60224fe 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -72,7 +72,6 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>  				   SPI_MEM_OP_ADDR(nor->addr_width, to, 1),
>  				   SPI_MEM_OP_DUMMY(0, 1),
>  				   SPI_MEM_OP_DATA_OUT(len, buf, 1));
> -	size_t remaining = len;
>  	int ret;
>  
>  	/* get transfer protocols. */
> @@ -84,22 +83,16 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>  	if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second)
>  		op.addr.nbytes = 0;
>  
> -	while (remaining) {
> -		op.data.nbytes = remaining < UINT_MAX ? remaining : UINT_MAX;
> -		ret = spi_mem_adjust_op_size(flash->spimem, &op);
> -		if (ret)
> -			return ret;
> -
> -		ret = spi_mem_exec_op(flash->spimem, &op);
> -		if (ret)
> -			return ret;
> +	ret = spi_mem_adjust_op_size(flash->spimem, &op);
> +	if (ret)
> +		return ret;
> +	op.data.nbytes = len < op.data.nbytes ? len : op.data.nbytes;
>  
> -		op.addr.val += op.data.nbytes;
> -		remaining -= op.data.nbytes;
> -		op.data.buf.out += op.data.nbytes;
> -	}
> +	ret = spi_mem_exec_op(flash->spimem, &op);
> +	if (ret)
> +		return ret;
>  
> -	return len;
> +	return op.data.nbytes;
>  }
>  
>  /*
> diff --git a/drivers/mtd/spi-nor/spi-nor.c 
> b/drivers/mtd/spi-nor/spi-nor.c index 5bfa36e..3e63543 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1431,13 +1431,6 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  			goto write_err;
>  		*retlen += written;
>  		i += written;
> -		if (written != page_remain) {
> -			dev_err(nor->dev,
> -				"While writing %zu bytes written %zd bytes\n",
> -				page_remain, written);
> -			ret = -EIO;
> -			goto write_err;
> -		}
>  	}
>  
>  write_err:

[1] https://patchwork.ozlabs.org/patch/928677/
[2] https://patchwork.ozlabs.org/patch/928678/