| Message ID | 20180930092535.24544-1-chuanhua.han@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/4] spi: spi-mem: Add the spi_set_xfer_bpw function | expand |
Hi Chuanhua, On Sun, 30 Sep 2018 17:25:32 +0800 Chuanhua Han <chuanhua.han@nxp.com> wrote: > Before we add this spi_transfer to the spi_message chain table, we need > bits_per_word_mask based on spi_control to set the bits_per_word of > this spi_transfer. Let's make it clearer: this is wrong. The spi-mem protocol is just using bytes, not custom size words. Fix the fsl-dspi driver if needed, but don't try to adjust xfer->bits_per_word in spi-mem.c, because this is inappropriate. Regards, Boris > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > --- > Changes in v2: > -The original patch is divided into multiple patches(the original > patch theme is "spi: spi-fsl-dspi: Fix support for XSPI transport > mode"),one of which is segmented. > > drivers/spi/spi-mem.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > index eb72dba71d83..717e711c0952 100644 > --- a/drivers/spi/spi-mem.c > +++ b/drivers/spi/spi-mem.c > @@ -175,6 +175,41 @@ bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op) > } > EXPORT_SYMBOL_GPL(spi_mem_supports_op); > > +/** > + * spi_set_xfer_bpw() - Set the bits_per_word for each transfer based on > + * the bits_per_word_mask of the spi controller > + * @ctrl: the spi controller > + * @xfer: the spi transfer > + * > + * This function sets the bits_per_word for each transfer based on the spi > + * controller's bits_per_word_mask to improve the efficiency of spi transport. > + * > + * Return: 0 in case of success, a negative error code otherwise. > + */ > +int spi_set_xfer_bpw(struct spi_controller *ctlr, struct spi_transfer *xfer) > +{ > + if (!ctlr || !xfer) { > + dev_err(&ctlr->dev, > + "Fail to set bits_per_word for spi transfer\n"); > + return -EINVAL; > + } > + > + if (ctlr->bits_per_word_mask) { > + if (!(xfer->len % 4)) { > + if (ctlr->bits_per_word_mask & SPI_BPW_MASK(32)) > + xfer->bits_per_word = 32; > + } else if (!(xfer->len % 2)) { > + if (ctlr->bits_per_word_mask & SPI_BPW_MASK(16)) > + xfer->bits_per_word = 16; > + } else { > + xfer->bits_per_word = 8; > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(spi_set_xfer_bpw); > + > /** > * spi_mem_exec_op() - Execute a memory operation > * @mem: the SPI memory > @@ -252,6 +287,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > xfers[xferpos].tx_buf = tmpbuf; > xfers[xferpos].len = sizeof(op->cmd.opcode); > xfers[xferpos].tx_nbits = op->cmd.buswidth; > + spi_set_xfer_bpw(ctlr, &xfers[xferpos]); > spi_message_add_tail(&xfers[xferpos], &msg); > xferpos++; > totalxferlen++; > @@ -266,6 +302,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > xfers[xferpos].tx_buf = tmpbuf + 1; > xfers[xferpos].len = op->addr.nbytes; > xfers[xferpos].tx_nbits = op->addr.buswidth; > + spi_set_xfer_bpw(ctlr, &xfers[xferpos]); > spi_message_add_tail(&xfers[xferpos], &msg); > xferpos++; > totalxferlen += op->addr.nbytes; > @@ -276,6 +313,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > xfers[xferpos].tx_buf = tmpbuf + op->addr.nbytes + 1; > xfers[xferpos].len = op->dummy.nbytes; > xfers[xferpos].tx_nbits = op->dummy.buswidth; > + spi_set_xfer_bpw(ctlr, &xfers[xferpos]); > spi_message_add_tail(&xfers[xferpos], &msg); > xferpos++; > totalxferlen += op->dummy.nbytes; > @@ -291,6 +329,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > } > > xfers[xferpos].len = op->data.nbytes; > + spi_set_xfer_bpw(ctlr, &xfers[xferpos]); > spi_message_add_tail(&xfers[xferpos], &msg); > xferpos++; > totalxferlen += op->data.nbytes;
Boris Brezillon <boris.brezillon@bootlin.com> writes: > Hi Chuanhua, > > On Sun, 30 Sep 2018 17:25:32 +0800 > Chuanhua Han <chuanhua.han@nxp.com> wrote: > >> Before we add this spi_transfer to the spi_message chain table, we need >> bits_per_word_mask based on spi_control to set the bits_per_word of >> this spi_transfer. > > Let's make it clearer: this is wrong. The spi-mem protocol is just > using bytes, not custom size words. Fix the fsl-dspi driver if needed, > but don't try to adjust xfer->bits_per_word in spi-mem.c, because this > is inappropriate. I don't think there is a "fix" needed in fsl-dspi driver for this. I am not sure, but I think that what Han is trying to achieve here is better performance. And wile the XSPI mode does provide better performance for sending one 32 bit word, than normal mode providees for sending 4 x 8 bit words. But as you say, this is wrong. To improve performance, the fsl-dspi driver should be fixed to work in DMA mode. Implementation of erratum A-011218 is necessary in order to use DSPI DMA mode on LS1021A. I was planning to work on that, but haven't had the time for it. So if you want better performance for spi-mem on LS1021A DSPI, please work on this. /Esben
> -----Original Message----- > From: Boris Brezillon <boris.brezillon@bootlin.com> > Sent: 2018年9月30日 18:04 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: broonie@kernel.org; linux-spi@vger.kernel.org; > linux-kernel@vger.kernel.org; eha@deif.com > Subject: Re: [PATCH v2 1/4] spi: spi-mem: Add the spi_set_xfer_bpw function > > Hi Chuanhua, > > On Sun, 30 Sep 2018 17:25:32 +0800 > Chuanhua Han <chuanhua.han@nxp.com> wrote: > > > Before we add this spi_transfer to the spi_message chain table, we > > need bits_per_word_mask based on spi_control to set the bits_per_word > > of this spi_transfer. > > Let's make it clearer: this is wrong. The spi-mem protocol is just using bytes, > not custom size words. Fix the fsl-dspi driver if needed, but don't try to adjust > xfer->bits_per_word in spi-mem.c, because this is inappropriate. The value of bits_per_word is only known before the spi_message_add_tail function is called, and dspi controllers only decide which mode (8bit, 16bit, or 32bit) to use for data transfer based on the value of the transfer->bits_per_word. > > Regards, > > Boris > > > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > > --- > > Changes in v2: > > -The original patch is divided into multiple patches(the original > > patch theme is "spi: spi-fsl-dspi: Fix support for XSPI transport > > mode"),one of which is segmented. > > > > drivers/spi/spi-mem.c | 39 > +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index > > eb72dba71d83..717e711c0952 100644 > > --- a/drivers/spi/spi-mem.c > > +++ b/drivers/spi/spi-mem.c > > @@ -175,6 +175,41 @@ bool spi_mem_supports_op(struct spi_mem > *mem, > > const struct spi_mem_op *op) } > > EXPORT_SYMBOL_GPL(spi_mem_supports_op); > > > > +/** > > + * spi_set_xfer_bpw() - Set the bits_per_word for each transfer based on > > + * the bits_per_word_mask of the spi controller > > + * @ctrl: the spi controller > > + * @xfer: the spi transfer > > + * > > + * This function sets the bits_per_word for each transfer based on > > +the spi > > + * controller's bits_per_word_mask to improve the efficiency of spi > transport. > > + * > > + * Return: 0 in case of success, a negative error code otherwise. > > + */ > > +int spi_set_xfer_bpw(struct spi_controller *ctlr, struct spi_transfer > > +*xfer) { > > + if (!ctlr || !xfer) { > > + dev_err(&ctlr->dev, > > + "Fail to set bits_per_word for spi transfer\n"); > > + return -EINVAL; > > + } > > + > > + if (ctlr->bits_per_word_mask) { > > + if (!(xfer->len % 4)) { > > + if (ctlr->bits_per_word_mask & SPI_BPW_MASK(32)) > > + xfer->bits_per_word = 32; > > + } else if (!(xfer->len % 2)) { > > + if (ctlr->bits_per_word_mask & SPI_BPW_MASK(16)) > > + xfer->bits_per_word = 16; > > + } else { > > + xfer->bits_per_word = 8; > > + } > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(spi_set_xfer_bpw); > > + > > /** > > * spi_mem_exec_op() - Execute a memory operation > > * @mem: the SPI memory > > @@ -252,6 +287,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const > struct spi_mem_op *op) > > xfers[xferpos].tx_buf = tmpbuf; > > xfers[xferpos].len = sizeof(op->cmd.opcode); > > xfers[xferpos].tx_nbits = op->cmd.buswidth; > > + spi_set_xfer_bpw(ctlr, &xfers[xferpos]); > > spi_message_add_tail(&xfers[xferpos], &msg); > > xferpos++; > > totalxferlen++; > > @@ -266,6 +302,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const > struct spi_mem_op *op) > > xfers[xferpos].tx_buf = tmpbuf + 1; > > xfers[xferpos].len = op->addr.nbytes; > > xfers[xferpos].tx_nbits = op->addr.buswidth; > > + spi_set_xfer_bpw(ctlr, &xfers[xferpos]); > > spi_message_add_tail(&xfers[xferpos], &msg); > > xferpos++; > > totalxferlen += op->addr.nbytes; > > @@ -276,6 +313,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const > struct spi_mem_op *op) > > xfers[xferpos].tx_buf = tmpbuf + op->addr.nbytes + 1; > > xfers[xferpos].len = op->dummy.nbytes; > > xfers[xferpos].tx_nbits = op->dummy.buswidth; > > + spi_set_xfer_bpw(ctlr, &xfers[xferpos]); > > spi_message_add_tail(&xfers[xferpos], &msg); > > xferpos++; > > totalxferlen += op->dummy.nbytes; > > @@ -291,6 +329,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const > struct spi_mem_op *op) > > } > > > > xfers[xferpos].len = op->data.nbytes; > > + spi_set_xfer_bpw(ctlr, &xfers[xferpos]); > > spi_message_add_tail(&xfers[xferpos], &msg); > > xferpos++; > > totalxferlen += op->data.nbytes;
On Sun, 30 Sep 2018 10:18:18 +0000 Chuanhua Han <chuanhua.han@nxp.com> wrote: > > -----Original Message----- > > From: Boris Brezillon <boris.brezillon@bootlin.com> > > Sent: 2018年9月30日 18:04 > > To: Chuanhua Han <chuanhua.han@nxp.com> > > Cc: broonie@kernel.org; linux-spi@vger.kernel.org; > > linux-kernel@vger.kernel.org; eha@deif.com > > Subject: Re: [PATCH v2 1/4] spi: spi-mem: Add the spi_set_xfer_bpw function > > > > Hi Chuanhua, > > > > On Sun, 30 Sep 2018 17:25:32 +0800 > > Chuanhua Han <chuanhua.han@nxp.com> wrote: > > > > > Before we add this spi_transfer to the spi_message chain table, we > > > need bits_per_word_mask based on spi_control to set the bits_per_word > > > of this spi_transfer. > > > > Let's make it clearer: this is wrong. The spi-mem protocol is just using bytes, > > not custom size words. Fix the fsl-dspi driver if needed, but don't try to adjust > > xfer->bits_per_word in spi-mem.c, because this is inappropriate. > The value of bits_per_word is only known before the spi_message_add_tail function is called, No, it's not. It's known from the beginning, and spi_setup() defaults to 8 when spidev->bits_per_word is 0, which is exactly what we want. Then, when you send a message through, spi_sync(), spi_validate() makes sure that each transfer in the message has a xfer->bits_per_word != 0 and when that's not the case, it sets it to spi->bits_per_word [2]. Really, there's nothing to fix in spi-mem.c, because it's already doing the right thing (leaving ->bits_per_word to 0 so that it's set to spi->bits_per_word, which should be 8). Maybe we have a bug somewhere else though. [1]https://elixir.bootlin.com/linux/v4.19-rc5/source/drivers/spi/spi.c#L2803 [2]https://elixir.bootlin.com/linux/v4.19-rc5/source/drivers/spi/spi.c#L2869
> -----Original Message----- > From: Esben Haabendal <esbenhaabendal@gmail.com> On Behalf Of Esben > Haabendal > Sent: 2018年9月30日 18:18 > To: Boris Brezillon <boris.brezillon@bootlin.com> > Cc: Chuanhua Han <chuanhua.han@nxp.com>; broonie@kernel.org; > linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2 1/4] spi: spi-mem: Add the spi_set_xfer_bpw function > > Boris Brezillon <boris.brezillon@bootlin.com> writes: > > > Hi Chuanhua, > > > > On Sun, 30 Sep 2018 17:25:32 +0800 > > Chuanhua Han <chuanhua.han@nxp.com> wrote: > > > >> Before we add this spi_transfer to the spi_message chain table, we > >> need bits_per_word_mask based on spi_control to set the bits_per_word > >> of this spi_transfer. > > > > Let's make it clearer: this is wrong. The spi-mem protocol is just > > using bytes, not custom size words. Fix the fsl-dspi driver if needed, > > but don't try to adjust xfer->bits_per_word in spi-mem.c, because this > > is inappropriate. > > I don't think there is a "fix" needed in fsl-dspi driver for this. > > I am not sure, but I think that what Han is trying to achieve here is better > performance. > And wile the XSPI mode does provide better performance for sending one > 32 bit word, than normal mode providees for sending 4 x 8 bit words. > But as you say, this is wrong. > > To improve performance, the fsl-dspi driver should be fixed to work in DMA > mode. Implementation of erratum A-011218 is necessary in order to use > DSPI DMA mode on LS1021A. > I was planning to work on that, but haven't had the time for it. > So if you want better performance for spi-mem on LS1021A DSPI, please work > on this. > > /Esben Hi， Another colleague is responsible for the DMA transmission of dspi. I am not clear about it. I just fix the transmission of XSPI mode with several patches. Thank you for your comments.
> -----Original Message----- > From: Boris Brezillon <boris.brezillon@bootlin.com> > Sent: 2018年9月30日 18:40 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: broonie@kernel.org; linux-spi@vger.kernel.org; > linux-kernel@vger.kernel.org; eha@deif.com > Subject: Re: [PATCH v2 1/4] spi: spi-mem: Add the spi_set_xfer_bpw function > > On Sun, 30 Sep 2018 10:18:18 +0000 > Chuanhua Han <chuanhua.han@nxp.com> wrote: > > > > -----Original Message----- > > > From: Boris Brezillon <boris.brezillon@bootlin.com> > > > Sent: 2018年9月30日 18:04 > > > To: Chuanhua Han <chuanhua.han@nxp.com> > > > Cc: broonie@kernel.org; linux-spi@vger.kernel.org; > > > linux-kernel@vger.kernel.org; eha@deif.com > > > Subject: Re: [PATCH v2 1/4] spi: spi-mem: Add the spi_set_xfer_bpw > > > function > > > > > > Hi Chuanhua, > > > > > > On Sun, 30 Sep 2018 17:25:32 +0800 > > > Chuanhua Han <chuanhua.han@nxp.com> wrote: > > > > > > > Before we add this spi_transfer to the spi_message chain table, we > > > > need bits_per_word_mask based on spi_control to set the > > > > bits_per_word of this spi_transfer. > > > > > > Let's make it clearer: this is wrong. The spi-mem protocol is just > > > using bytes, not custom size words. Fix the fsl-dspi driver if > > > needed, but don't try to adjust > > > xfer->bits_per_word in spi-mem.c, because this is inappropriate. > > The value of bits_per_word is only known before the > > spi_message_add_tail function is called, > > No, it's not. It's known from the beginning, and spi_setup() defaults to 8 when > spidev->bits_per_word is 0, which is exactly what we want. Then, when you > send a message through, spi_sync(), spi_validate() makes sure that each > transfer in the message has a > xfer->bits_per_word != 0 and when that's not the case, it sets it to > spi->bits_per_word [2]. > > Really, there's nothing to fix in spi-mem.c, because it's already doing the right > thing (leaving ->bits_per_word to 0 so that it's set to > spi->bits_per_word, which should be 8). Maybe we have a bug somewhere > else though. > > [1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Feli > xir.bootlin.com%2Flinux%2Fv4.19-rc5%2Fsource%2Fdrivers%2Fspi%2Fspi.c%23 > L2803&data=02%7C01%7Cchuanhua.han%40nxp.com%7C48694d5d7cd > a460b3e0b08d626c11e3f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7 > C0%7C636739008212287814&sdata=5wYyFaZjk9kkbZtR0w0uS2YFlfNjC > Gz3SN80Ws599j0%3D&reserved=0 > [2]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Feli > xir.bootlin.com%2Flinux%2Fv4.19-rc5%2Fsource%2Fdrivers%2Fspi%2Fspi.c%23 > L2869&data=02%7C01%7Cchuanhua.han%40nxp.com%7C48694d5d7cd > a460b3e0b08d626c11e3f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7 > C0%7C636739008212287814&sdata=ibwl%2BZuk%2FMLagsudNetwgda > hR1VVKBqI2ByL6225H50%3D&reserved=0 I'll take the time to study this. October 1 to October 7 is the National Day of our country. I need to take a vacation. I may not reply to the email in time during this period. Thank you very much for your valuable advice!!!
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index eb72dba71d83..717e711c0952 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -175,6 +175,41 @@ bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op) } EXPORT_SYMBOL_GPL(spi_mem_supports_op); +/** + * spi_set_xfer_bpw() - Set the bits_per_word for each transfer based on + * the bits_per_word_mask of the spi controller + * @ctrl: the spi controller + * @xfer: the spi transfer + * + * This function sets the bits_per_word for each transfer based on the spi + * controller's bits_per_word_mask to improve the efficiency of spi transport. + * + * Return: 0 in case of success, a negative error code otherwise. + */ +int spi_set_xfer_bpw(struct spi_controller *ctlr, struct spi_transfer *xfer) +{ + if (!ctlr || !xfer) { + dev_err(&ctlr->dev, + "Fail to set bits_per_word for spi transfer\n"); + return -EINVAL; + } + + if (ctlr->bits_per_word_mask) { + if (!(xfer->len % 4)) { + if (ctlr->bits_per_word_mask & SPI_BPW_MASK(32)) + xfer->bits_per_word = 32; + } else if (!(xfer->len % 2)) { + if (ctlr->bits_per_word_mask & SPI_BPW_MASK(16)) + xfer->bits_per_word = 16; + } else { + xfer->bits_per_word = 8; + } + } + + return 0; +} +EXPORT_SYMBOL_GPL(spi_set_xfer_bpw); + /** * spi_mem_exec_op() - Execute a memory operation * @mem: the SPI memory @@ -252,6 +287,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) xfers[xferpos].tx_buf = tmpbuf; xfers[xferpos].len = sizeof(op->cmd.opcode); xfers[xferpos].tx_nbits = op->cmd.buswidth; + spi_set_xfer_bpw(ctlr, &xfers[xferpos]); spi_message_add_tail(&xfers[xferpos], &msg); xferpos++; totalxferlen++; @@ -266,6 +302,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) xfers[xferpos].tx_buf = tmpbuf + 1; xfers[xferpos].len = op->addr.nbytes; xfers[xferpos].tx_nbits = op->addr.buswidth; + spi_set_xfer_bpw(ctlr, &xfers[xferpos]); spi_message_add_tail(&xfers[xferpos], &msg); xferpos++; totalxferlen += op->addr.nbytes; @@ -276,6 +313,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) xfers[xferpos].tx_buf = tmpbuf + op->addr.nbytes + 1; xfers[xferpos].len = op->dummy.nbytes; xfers[xferpos].tx_nbits = op->dummy.buswidth; + spi_set_xfer_bpw(ctlr, &xfers[xferpos]); spi_message_add_tail(&xfers[xferpos], &msg); xferpos++; totalxferlen += op->dummy.nbytes; @@ -291,6 +329,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) } xfers[xferpos].len = op->data.nbytes; + spi_set_xfer_bpw(ctlr, &xfers[xferpos]); spi_message_add_tail(&xfers[xferpos], &msg); xferpos++; totalxferlen += op->data.nbytes;
Before we add this spi_transfer to the spi_message chain table, we need bits_per_word_mask based on spi_control to set the bits_per_word of this spi_transfer. Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> --- Changes in v2: -The original patch is divided into multiple patches(the original patch theme is "spi: spi-fsl-dspi: Fix support for XSPI transport mode"),one of which is segmented. drivers/spi/spi-mem.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)