[U-Boot] [PATCH] mtd: spi: Improve data write functionality

[Vignesh Raghavendra]

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 at 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) {
> 

-- 
Regards
Vignesh