Using SPI NOR flah on sunxi.

The size of written data was added to user supplied value rather than written at the provided address.

Signed-off-by Michal Suchanek <[email protected]>

---
 drivers/mtd/devices/m25p80.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mtd/devices/m25p80.c +1 -1

index 7c8b169..0b2bc21 100644
@@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t
 to, size_t len,
 
 	spi_sync(spi, &m);
 
-	*retlen += m.actual_length - cmd_sz;
+	*retlen = m.actual_length - cmd_sz;
 }
 

static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

Quoted 2 lines · Michal Suchanek

The size of written data was added to user supplied value rather than written at the provided address.

You might want to work on the commit message a little, something like the following, but feel free to reword as seen fit.

The 'retlen' points to a variable representing the number of data bytes written/read (see include/linux/mtd/mtd.h) by the current invocation of the function. This variable must be set, not incremented.

Otherwise, the patch is correct I believe:

Acked-by Marek Vasut <[email protected]>

My SPI controller driver does not support DMA so writes are truncated to FIFO size.

Check the amount of data actually written by the driver.

Signed-off-by Michal Suchanek <[email protected]>

---
 drivers/mtd/spi-nor/spi-nor.c | 42 +++++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

drivers/mtd/spi-nor/spi-nor.c +17 -25

index 14a5d23..75904b5 100644
@@ -807,7 +807,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 	size_t *retlen, const u_char *buf)
 {
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
-	u32 page_offset, page_size, i;
+	u32 page_offset, page_remainder, page_size, i;
 	int ret;
 
 	dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len);

@@ -816,36 +816,28 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 	if (ret)
 		return ret;
 
-	write_enable(nor);
-
-	page_offset = to & (nor->page_size - 1);
-
-	/* do all the bytes fit onto one page? */
-	if (page_offset + len <= nor->page_size) {
-		nor->write(nor, to, len, retlen, buf);
-	} else {
-		/* the size of data remaining on the first page */
-		page_size = nor->page_size - page_offset;
-		nor->write(nor, to, page_size, retlen, buf);
+	/* write everything in nor->page_size chunks */
+	/* check the size of data actually written */
+	for (i = 0; i < len; i += *retlen) {
+		page_size = len - i;
+		page_offset = (to + i) & (nor->page_size - 1);
+		page_remainder = nor->page_size - page_offset;
+		if (page_size > nor->page_size)
+			page_size = nor->page_size;
+		if (page_remainder && (page_size > page_remainder))
+			page_size = page_remainder;
 
-		/* write everything in nor->page_size chunks */
-		for (i = page_size; i < len; i += page_size) {
-			page_size = len - i;
-			if (page_size > nor->page_size)
-				page_size = nor->page_size;
-
-			ret = spi_nor_wait_till_ready(nor);
-			if (ret)
-				goto write_err;
+		write_enable(nor);
 
-			write_enable(nor);
+		nor->write(nor, to + i, page_size, retlen, buf + i);
 
-			nor->write(nor, to + i, page_size, retlen, buf + i);
-		}
+		ret = spi_nor_wait_till_ready(nor);
+		if (ret)
+			goto write_err;
 	}
 
-	ret = spi_nor_wait_till_ready(nor);
 write_err:
+	*retlen = i;
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE);
 	return ret;
 }

+ linux-spi, Mark

Quoted 2 lines · Michal Suchanek

My SPI controller driver does not support DMA so writes are truncated to FIFO size.

Which SPI master driver?

Quoted 2 lines

Check the amount of data actually written by the driver.

I'm not sure if we should just reactively use the retlen, or if we should be communicating such a limitation via the SPI API. Thoughts?

On 21 May 2015 at 01:38, Brian Norris <[email protected]> wrote:

Quoted 4 lines

Quoted 1 line

Check the amount of data actually written by the driver.

I'm not sure if we should just reactively use the retlen, or if we should be communicating such a limitation via the SPI API. Thoughts?

Is there any driver that would break if the SPI master truncated writes when the message is too long rather than returning an error an refusing the transfer?

Quoted 1 line · Michal Suchanek

On 21 May 2015 at 01:38, Brian Norris <[email protected]> wrote:

Why is this thread about SPI error handling CCed to quite so many people?

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Check the amount of data actually written by the driver.

I'm not sure if we should just reactively use the retlen, or if we should be communicating such a limitation via the SPI API. Thoughts?

Is there any driver that would break if the SPI master truncated writes when the message is too long rather than returning an error an refusing the transfer?

Any such driver is buggy, the actual length transferred has always been a part of the SPI API.

We should probably expose limitations so clients
can query in advance (we don't at the minute) but it'd be a while before clients could rely on that information being useful and it's still possible things can be truncated by error.

Admittedly, as he's using an out-of-tree driver, I'm not sure I know exactly what failure modes he is hitting yet.

Quoted 2 lines · Brian Norris

Admittedly, as he's using an out-of-tree driver, I'm not sure I know exactly what failure modes he is hitting yet.

Sorry, I realized I misread here. He's using spi-sunxi. Given that...

... is this code even valid?

static int sun6i_spi_transfer_one(struct spi_master *master,
struct spi_device *spi, struct spi_transfer *tfr)
{
...

	/* We don't support transfer larger than the FIFO */
	if (tfr->len > SUN6I_FIFO_DEPTH)
		return -EINVAL;

Seems like it should be looping over the transfer in multiple chunks instead.

My SPI driver does not support DMA so sizes larger than 64 bytes return an error. Add an option to limit transfer size so m25p80 can be used with reduced speed with such controller.

Quoted 3 lines · Michal Suchanek

My SPI driver does not support DMA so sizes larger than 64 bytes return an error. Add an option to limit transfer size so m25p80 can be used with reduced speed with such controller.

I suspect that this will be rejected as the maximum transfer size is a property of the SPI controller, not the device and should be exported to the device driver from the controller, not specified in the device's binding.

I tried to connect a SPI NOR flash to my sunxi board and due to the

sunxi SPI driver limitations it does not work.

The SPI driver returns an error when more than 64 bytes are transferred at once
due to lack of DMA support.

I tried to connect a SPI NOR flash to my sunxi board and due to the

sunxi SPI driver limitations it does not work.

The SPI driver returns an error when more than 64 bytes are transferred at once
due to lack of DMA support.

Hello Michal:

Quoted 1 line

I tried to connect a SPI NOR flash to my sunxi board and due to the

current

Quoted 2 lines

sunxi SPI driver limitations it does not work.

Quoted 3 lines

The SPI driver returns an error when more than 64 bytes are transferred at once
due to lack of DMA support.

Wouldn't it be easier to fix the SPI driver to handle transfers larger than 64 bytes, filling and draining the FIFO multiple times if neccessary? (As far as I can tell, most SPI drivers do this.)

Quoted 7 lines · On 30 April 2015 at 18:30, <Thomas.Betker-Bf/A/FSCP0w3s4ca2cGeAgC/G2K4zDHf@pub…

Hello Michal:

Quoted 1 line

I tried to connect a SPI NOR flash to my sunxi board and due to the

current

Quoted 2 lines

sunxi SPI driver limitations it does not work.

Quoted 7 lines

Quoted 3 lines

The SPI driver returns an error when more than 64 bytes are transferred at once
due to lack of DMA support.

Wouldn't it be easier to fix the SPI driver to handle transfers larger than 64 bytes, filling and draining the FIFO multiple times if neccessary? (As far as I can tell, most SPI drivers do this.)

Yes, the intent is to fix this by adding dma support to the driver, eventually.

The patch might be still useful for other hardware with developing SPI support.

Quoted 16 lines · Michal Suchanek

Quoted 14 lines · On 30 April 2015 at 18:30, <[email protected]> wrote:

Hello Michal:

Quoted 1 line

I tried to connect a SPI NOR flash to my sunxi board and due to the

current

Quoted 5 lines

sunxi SPI driver limitations it does not work.

The SPI driver returns an error when more than 64 bytes are transferred at once
due to lack of DMA support.

Wouldn't it be easier to fix the SPI driver to handle transfers larger than 64 bytes, filling and draining the FIFO multiple times if neccessary? (As far as I can tell, most SPI drivers do this.)

Quoted 5 lines

Yes, the intent is to fix this by adding dma support to the driver, eventually.

The patch might be still useful for other hardware with developing SPI support.

Please just fix the controller driver to correctly handle arbitrary transfer lengths.

I tried to connect a SPI NOR flash to my sunxi board and due to the

sunxi SPI driver limitations it does not work.

The SPI driver returns an error when more than 64 bytes are transferred at once
due to lack of DMA support.

I tried to connect a SPI NOR flash to my sunxi board and due to the

sunxi SPI driver limitations it does not work.

The SPI driver returns an error when more than 64 bytes are transferred at once
due to lack of DMA support.

The SPI driver returns an error when more than 64 bytes are transferred at once due to lack of DMA support.

Quoted 2 lines

The SPI driver returns an error when more than 64 bytes are transferred at once due to lack of DMA support.

Have you tried the dmaengine patch and make the SPI driver use it?