-----Original Message----- From: Rajat Srivastava Sent: Friday, April 5, 2019 2:24 PM To: u-boot@lists.denx.de; vigneshr@ti.com; trini@konsulko.com; marek.vasut@gmail.com; marek.vasut+renesas@gmail.com; jagan@openedev.com Cc: Ashish Kumar ashish.kumar@nxp.com; Rajat Srivastava rajat.srivastava@nxp.com Subject: [PATCH] mtd: spi: Improve data write functionality

Incorporate write enable and status check in the write data function itself.

Formerly, Write data function used to break the data to be written into smaller chunks and used to send the smaller chunks without write enable or status check for every iteration.

Signed-off-by: Rajat Srivastava rajat.srivastava@nxp.com

Hi Jagan, Vignesh, Marek,

Did you get some time to look into this Patch, the current data write is also broken ?

Regards Ashish

---

While writing any data to a SPI flash every write transaction shall be preceded by a WRITE_ENABLE command and shall be followed by a READ_STATUS process (to check if the flash is not busy). This sequence can be roughly represented as:

1. write_enable //issue write enable command 2. execute_write_operation

//write data to flash or register 3. spi_nor_wait_till_ready //read status of flash

The current framework has two types of write operation:

1. write to register (nor->write_reg)
2. write data to flash memory (nor->write)

There seems to be an issue in writing data to flash memory for which the framework uses spi_nor_write_data() function. Before every call to spi_nor_write_data() function the framework sends a WRITE_ENABLE command and later checks if the flash is busy. However, the spi_nor_write_data() function which executes the data write to flash, breaks the data into smaller chunks. For all of these small transactions there is only a single WRITE_ENABLE command issued and a single check made for status, which breaks the write operation. Only the first chunk of the whole data is successfully written on to the flash.

This patch fixes the bug making the spi_nor_write_data() function issue WRITE_ENABLE command and status checks with every write transactions.

Without this patch write in fsl_qspi.c driver is broken.

drivers/mtd/spi/spi-nor-core.c | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index c4e2f6a08f..757163369b 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -23,6 +23,9 @@

#include "sf_internal.h"

+static int spi_nor_wait_till_ready(struct spi_nor *nor); static int +write_enable(struct spi_nor *nor);

/* Define max times to check status register before we give up. */

/* @@ -128,6 +131,8 @@ static ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len, op.addr.nbytes = 0;

while (remaining) {

• write_enable(nor);
  

• op.data.nbytes = remaining < UINT_MAX ? remaining :

UINT_MAX; ret = spi_mem_adjust_op_size(nor->spi, &op); if (ret) @@ -137,6 +142,10 @@ static ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len, if (ret) return ret;

• ret = spi_nor_wait_till_ready(nor);
  

• if (ret)
  

• 	return ret;
  

• op.addr.val += op.data.nbytes; remaining -= op.data.nbytes; op.data.buf.out += op.data.nbytes;

@@ -961,14 +970,10 @@ static int sst_write_byteprogram(struct spi_nor *nor, loff_t to, size_t len, for (actual = 0; actual < len; actual++) { nor->program_opcode = SPINOR_OP_BP;

• write_enable(nor);
  

  /* write one byte. */ ret = nor->write(nor, to, 1, buf + actual); if (ret < 0) goto sst_write_err;

• ret = spi_nor_wait_till_ready(nor);
  

• if (ret)
  

• 	goto sst_write_err;
  

  to++; }

@@ -989,8 +994,6 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, if (spi->mode & SPI_TX_BYTE) return sst_write_byteprogram(nor, to, len, retlen, buf);

• write_enable(nor);

• nor->sst_write_second = false;

  actual = to % 2;

@@ -1002,9 +1005,6 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, ret = nor->write(nor, to, 1, buf); if (ret < 0) goto sst_write_err;

• ret = spi_nor_wait_till_ready(nor);
  

• if (ret)
  

• 	goto sst_write_err;
  

  } to += actual;

@@ -1016,9 +1016,6 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, ret = nor->write(nor, to, 2, buf + actual); if (ret < 0) goto sst_write_err;

• ret = spi_nor_wait_till_ready(nor);
  

• if (ret)
  

• 	goto sst_write_err;
  

  to += 2; nor->sst_write_second = true; }

@@ -1031,15 +1028,10 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,

/* Write out trailing byte if it exists. */ if (actual != len) {

• write_enable(nor);
  

• nor->program_opcode = SPINOR_OP_BP; ret = nor->write(nor, to, 1, buf + actual); if (ret < 0) goto sst_write_err;

• ret = spi_nor_wait_till_ready(nor);
  

• if (ret)
  

• 	goto sst_write_err;
  

  write_disable(nor); actual += 1; }

@@ -1090,15 +1082,11 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, if (ret < 0) return ret; #endif

• write_enable(nor);
  

  ret = nor->write(nor, addr, page_remain, buf + i); if (ret < 0) goto write_err; written = ret;

• ret = spi_nor_wait_till_ready(nor);
  

• if (ret)
  

• 	goto write_err;
  

  *retlen += written; i += written; if (written != page_remain) {

-- 2.17.1

On 05/04/19 2:24 PM, Rajat Srivastava wrote:

Incorporate write enable and status check in the write data function itself.

Formerly, Write data function used to break the data to be written into smaller chunks and used to send the smaller chunks without write enable or status check for every iteration.

Signed-off-by: Rajat Srivastava rajat.srivastava@nxp.com

While writing any data to a SPI flash every write transaction shall be preceded by a WRITE_ENABLE command and shall be followed by a READ_STATUS process (to check if the flash is not busy). This sequence can be roughly represented as:

1. write_enable //issue write enable command
2. execute_write_operation //write data to flash or register
3. spi_nor_wait_till_ready //read status of flash

The current framework has two types of write operation:

1. write to register (nor->write_reg)
2. write data to flash memory (nor->write)

There seems to be an issue in writing data to flash memory for which the framework uses spi_nor_write_data() function. Before every call to spi_nor_write_data() function the framework sends a WRITE_ENABLE command and later checks if the flash is busy. However, the spi_nor_write_data() function which executes the data write to flash, breaks the data into smaller chunks. For all of these small transactions there is only a single WRITE_ENABLE command issued and a single check made for status, which breaks the write operation. Only the first chunk of the whole data is successfully written on to the flash.

This patch fixes the bug making the spi_nor_write_data() function issue WRITE_ENABLE command and status checks with every write transactions.

Without this patch write in fsl_qspi.c driver is broken.

I see this is mainly because fsl-qspi uses slave->max_write_size to restrict max write size which leads to fragmentation of traffic as part of spi_mem_adjust_op_size(). So, could you please remove while() loop in spi_nor_write_data(), return actual number of data bytes that is written and make spi_nor_write() loop around until all the data has been written?

Regards Vignesh

drivers/mtd/spi/spi-nor-core.c | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index c4e2f6a08f..757163369b 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -23,6 +23,9 @@

#include "sf_internal.h"

+static int spi_nor_wait_till_ready(struct spi_nor *nor); +static int write_enable(struct spi_nor *nor);

/* Define max times to check status register before we give up. */

/* @@ -128,6 +131,8 @@ static ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len, op.addr.nbytes = 0;

while (remaining) {

• write_enable(nor);
  

• op.data.nbytes = remaining < UINT_MAX ? remaining : UINT_MAX; ret = spi_mem_adjust_op_size(nor->spi, &op); if (ret)

@@ -137,6 +142,10 @@ static ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len, if (ret) return ret;

• ret = spi_nor_wait_till_ready(nor);
  

• if (ret)
  

• 	return ret;
  

• op.addr.val += op.data.nbytes; remaining -= op.data.nbytes; op.data.buf.out += op.data.nbytes;

@@ -961,14 +970,10 @@ static int sst_write_byteprogram(struct spi_nor *nor, loff_t to, size_t len, for (actual = 0; actual < len; actual++) { nor->program_opcode = SPINOR_OP_BP;

• write_enable(nor);
  

  /* write one byte. */ ret = nor->write(nor, to, 1, buf + actual); if (ret < 0) goto sst_write_err;

• ret = spi_nor_wait_till_ready(nor);
  

• if (ret)
  

• 	goto sst_write_err;
  

  to++; }

@@ -989,8 +994,6 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, if (spi->mode & SPI_TX_BYTE) return sst_write_byteprogram(nor, to, len, retlen, buf);

• write_enable(nor);

• nor->sst_write_second = false;

  actual = to % 2;

@@ -1002,9 +1005,6 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, ret = nor->write(nor, to, 1, buf); if (ret < 0) goto sst_write_err;

• ret = spi_nor_wait_till_ready(nor);
  

• if (ret)
  

• 	goto sst_write_err;
  

  } to += actual;

@@ -1016,9 +1016,6 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, ret = nor->write(nor, to, 2, buf + actual); if (ret < 0) goto sst_write_err;

• ret = spi_nor_wait_till_ready(nor);
  

• if (ret)
  

• 	goto sst_write_err;
  

  to += 2; nor->sst_write_second = true; }

@@ -1031,15 +1028,10 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,

/* Write out trailing byte if it exists. */ if (actual != len) {

• write_enable(nor);
  

• nor->program_opcode = SPINOR_OP_BP; ret = nor->write(nor, to, 1, buf + actual); if (ret < 0) goto sst_write_err;

• ret = spi_nor_wait_till_ready(nor);
  

• if (ret)
  

• 	goto sst_write_err;
  

  write_disable(nor); actual += 1; }

@@ -1090,15 +1082,11 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, if (ret < 0) return ret; #endif

• write_enable(nor);
  

  ret = nor->write(nor, addr, page_remain, buf + i); if (ret < 0) goto write_err; written = ret;

• ret = spi_nor_wait_till_ready(nor);
  

• if (ret)
  

• 	goto write_err;
  

  *retlen += written; i += written; if (written != page_remain) {