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

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

[Yogesh Gaur]

Honour max_data_size for spi-nor writes
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.

Signed-off-by: NeilBrown <neil@brown.name>
Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
---
 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:
-- 
2.7.4

Patch is based on the spi-mem framework[1]
[1]https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/log/?h=for-4.18

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

[Boris Brezillon]

Hi Yogesh,

Unrelated note: no need to send your patches to both
boris.brezillo@free-electrons.com and boris.brezillo@bootlin.com, just
use the latter.

On Mon, 11 Jun 2018 14:48:14 +0530
Yogesh Gaur <yogeshnarayan.gaur@nxp.com> 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.
"

> 
> Signed-off-by: NeilBrown <neil@brown.name>
> Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
> ---
>  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:

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1796 bytes --]

On Mon, Jun 11 2018, Boris Brezillon wrote:
>
> 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.

I no longer think this is good justification for changes to m25p80 or to
anything outside the low-level SPI driver.  The size of the FIFO is not
related to the maximum message size.

An SPI transaction involves:
 - asserting chip-select
 - sending/receiving a bunch of bits
 - deasserting chip-select

That middle part of sending/receiving bits can be done:
 - one bit at a time by bit-banging GPIOs
 - 32 bytes at a time by filling a buffer, running the SPI engine, then
    reading results out of the buffer (assuming a 32-byte buffer)
 - with one single DMA operation if the SPI engine can DMA to/from
   main memory

and there are probably other options.

If your SPI engine only handles (say) 32 bytes at a time, then call it
repeatedly, while holding chip-select asserted the whole time.  The SPI
clock is controlled by the host and in this case it will not have a
stable frequency for the whole transaction (occasionally pauses), but
that doesn't matter.  The slave just watches for the clock transitions
and don't care when they come as long as they don't come too fast.

I recently modified the mt7621 spi driver (in drivers/staging) to work
like this and got much better throughput when reading from spi-nor flash
memory.  It handles writes of full pages (36 bytes at at time!) without
problems.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Boris Brezillon]

Hi Neil,

On Tue, 12 Jun 2018 08:05:13 +1000
NeilBrown <neil@brown.name> wrote:

> On Mon, Jun 11 2018, Boris Brezillon wrote:
> >
> > 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.  
> 
> I no longer think this is good justification for changes to m25p80 or to
> anything outside the low-level SPI driver.  The size of the FIFO is not
> related to the maximum message size.
> 
> An SPI transaction involves:
>  - asserting chip-select
>  - sending/receiving a bunch of bits
>  - deasserting chip-select
> 
> That middle part of sending/receiving bits can be done:
>  - one bit at a time by bit-banging GPIOs
>  - 32 bytes at a time by filling a buffer, running the SPI engine, then
>     reading results out of the buffer (assuming a 32-byte buffer)
>  - with one single DMA operation if the SPI engine can DMA to/from
>    main memory
> 
> and there are probably other options.
> 
> If your SPI engine only handles (say) 32 bytes at a time, then call it
> repeatedly, while holding chip-select asserted the whole time.

Except you're not necessarily in control of the CS signal, and that's
most of the time the case with high-level (Q)SPI mem controllers like
the NXP/FSL QSPI controller (see the datasheet here if you want to check
[1]).

> The SPI
> clock is controlled by the host and in this case it will not have a
> stable frequency for the whole transaction (occasionally pauses), but
> that doesn't matter.  The slave just watches for the clock transitions
> and don't care when they come as long as they don't come too fast.
> 
> I recently modified the mt7621 spi driver (in drivers/staging) to work
> like this and got much better throughput when reading from spi-nor flash
> memory.  It handles writes of full pages (36 bytes at at time!) without
> problems.

Just because you managed to solve the problem in one driver does not
mean the problem does not exist for others. I read this datasheet [1]
several times and couldn't find a way to say 'I want to keep the CS
asserted between 2 transactions', so I think we really need this patch.

Regards,

Boris

[1]https://www.nxp.com/docs/en/reference-manual/VFXXXRM.pdf

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

On Tue, Jun 12 2018, Boris Brezillon wrote:

>
> Just because you managed to solve the problem in one driver does not
> mean the problem does not exist for others. I read this datasheet [1]
> several times and couldn't find a way to say 'I want to keep the CS
> asserted between 2 transactions', so I think we really need this patch.

I agree that my experience doesn't necessarily generalize.  As the patch
carried by signed-off-by (even though I only wrote little parts of it) I
wanted to make it clear that I had no desire to promote the patch -
maybe I stated that too strongly.

Thanks for the link to the data sheet.  I had a bit of a look, but
reading these things must be an art that I haven't fully mastered yet -
it would probably take me a few days to really understand it.
The Programmable Sequence Enginine (Section 10.2.5.3.1) seems
interesting. I wouldn't be surprised that that lets you do interesting
things.

It is obviously quite a powerful unit and it is surprising - to me -
that it might not allow arbitrarily long messages, but I cannot justify
the time to really dig in and see if that is the case.
Maybe you are right.  I have no particular objections to the patch, I
just don't want to be seen as speaking in favour of it.

Thanks,
NeilBrown


>
> Regards,
>
> Boris
>
> [1]https://www.nxp.com/docs/en/reference-manual/VFXXXRM.pdf

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Boris Brezillon]

On Wed, 13 Jun 2018 08:24:26 +1000
NeilBrown <neil@brown.name> wrote:

> On Tue, Jun 12 2018, Boris Brezillon wrote:
> 
> >
> > Just because you managed to solve the problem in one driver does not
> > mean the problem does not exist for others. I read this datasheet [1]
> > several times and couldn't find a way to say 'I want to keep the CS
> > asserted between 2 transactions', so I think we really need this patch.  
> 
> I agree that my experience doesn't necessarily generalize.  As the patch
> carried by signed-off-by (even though I only wrote little parts of it) I
> wanted to make it clear that I had no desire to promote the patch -
> maybe I stated that too strongly.

If that's a problem, we can drop your SoB.

> 
> Thanks for the link to the data sheet.  I had a bit of a look, but
> reading these things must be an art that I haven't fully mastered yet -
> it would probably take me a few days to really understand it.
> The Programmable Sequence Enginine (Section 10.2.5.3.1) seems
> interesting. I wouldn't be surprised that that lets you do interesting
> things.

It does.

> 
> It is obviously quite a powerful unit and it is surprising - to me -
> that it might not allow arbitrarily long messages, but I cannot justify
> the time to really dig in and see if that is the case.

Unfortunately it does not allow you to manually control the CS signal,
and when it comes to data transfers you're limited by the TX/RX FIFO
size.

> Maybe you are right.  I have no particular objections to the patch, I
> just don't want to be seen as speaking in favour of it.

Okay.

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@bootlin.com] 
Sent: Monday, June 11, 2018 3:19 PM
To: Yogesh Narayan Gaur <yogeshnarayan.gaur@nxp.com>
Cc: linux-mtd@lists.infradead.org; boris.brezillon@free-electrons.com; frieder.schrempf@exceet.de; computersforpeace@gmail.com; David Wolfe <david.wolfe@nxp.com>; Han Xu <han.xu@nxp.com>; festevam@gmail.com; marek.vasut@gmail.com; Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>; linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; NeilBrown <neil@brown.name>
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@free-electrons.com and boris.brezillo@bootlin.com, just use the latter.

Ok, Sure.

On Mon, 11 Jun 2018 14:48:14 +0530
Yogesh Gaur <yogeshnarayan.gaur@nxp.com> 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@brown.name>
> Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
> ---
>  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/

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

[Yogesh Gaur]

Honour max_data_size for spi-nor writes
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.

Signed-off-by: NeilBrown <neil@brown.name>
Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
---
 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:
-- 
2.7.4

Patch is based on the spi-mem framework[1]
[1]https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/log/?h=for-4.18