[U-Boot] [EXT] Re: [PATCH] mtd: spi: Improve data write functionality
Rajat Srivastava
rajat.srivastava at nxp.com
Tue Apr 16 12:01:05 UTC 2019
> -----Original Message-----
> From: Vignesh Raghavendra <vigneshr at ti.com>
> Sent: Friday, April 12, 2019 1:01 PM
> To: Rajat Srivastava <rajat.srivastava at nxp.com>; u-boot at lists.denx.de;
> trini at konsulko.com; marek.vasut at gmail.com;
> marek.vasut+renesas at gmail.com; jagan at openedev.com
> Cc: Ashish Kumar <ashish.kumar at nxp.com>
> Subject: [EXT] Re: [PATCH] mtd: spi: Improve data write functionality
>
> WARNING: This email was created outside of NXP. DO NOT CLICK links or
> attachments unless you recognize the sender and know the content is safe.
>
>
>
> 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?
>
Please find the v2 of this patch at https://patchwork.ozlabs.org/patch/1086262/
Regards
Rajat
> 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
More information about the U-Boot
mailing list