| Message ID | 1528708694-19407-1-git-send-email-yogeshnarayan.gaur@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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: -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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/ -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: