| Message ID | 20180921070628.35153-1-chuanhua.han@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] spi: spi-mem: Add the spi_set_xfer_bpw function | expand |
> -----Original Message----- > From: Chuanhua Han <chuanhua.han@nxp.com> > Sent: 2018年9月21日 15:06 > To: broonie@kernel.org > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; eha@deif.com; > boris.brezillon@bootlin.com; Chuanhua Han <chuanhua.han@nxp.com> > Subject: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function > > 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> > --- > 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; > -- > 2.17.1 Hi,all Could you please help me to see the fix of this patch? What changes need to be made? Looking forward to your valuable comments and criticism, thank you very much!!! Thanks, Chuanhua
Hi Chuanhua, On Fri, 21 Sep 2018 15:06:26 +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. It's not clear to me what you're trying to fix/improve. Can you give more details on what the problem is? > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > --- > 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); Why is this function placed in spi-mem.c, and more importantly, why is it exported? > + > /** > * 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]); It's still unclear why you need to specify a bits_per_word value, but if this is needed, it's probably something you want to add to spi.c, when a message is queued. > 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;
HI,Boris, > -----Original Message----- > From: Boris Brezillon <boris.brezillon@bootlin.com> > Sent: 2018年9月28日 14:45 > 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 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function > > Hi Chuanhua, > > On Fri, 21 Sep 2018 15:06:26 +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. > > It's not clear to me what you're trying to fix/improve. Can you give more > details on what the problem is? > > > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > > --- > > 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); > > Why is this function placed in spi-mem.c, and more importantly, why is it > exported? Since bits_per_word is not judged by spi every time it is transmitted, the code defaults to bits_per_word=8 so that bits_per_word cannot be implemented if it wants to transfer a specific spi controller, so it is flexible to assign bits_per_word according to the spi controller's bits_per_word_mask before each transfer spi. Ah, for which export, there is really no need to deliberately remove. > > > + > > /** > > * 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]); > > It's still unclear why you need to specify a bits_per_word value, but if this is > needed, it's probably something you want to add to spi.c, when a message is > queued. To specify a specific bits_per_word to be able to use the xspi (32bit) mode of the fsl_dspi module to transfer data, you can look at my PATCH 2/2. Do not add a value in spis.c that takes into account that the value assigned to bits_per_word is decided before the transfer. Thanks for your check and reply! > > > 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 Fri, 28 Sep 2018 06:59:58 +0000 Chuanhua Han <chuanhua.han@nxp.com> wrote: > > > > It's still unclear why you need to specify a bits_per_word value, > > but if this is needed, it's probably something you want to add to > > spi.c, when a message is queued. > To specify a specific bits_per_word to be able to use the xspi > (32bit) mode of the fsl_dspi module to transfer data, you can look at > my PATCH 2/2. Do not add a value in spis.c that takes into account > that the value assigned to bits_per_word is decided before the > transfer. Thanks for your check and reply! I might be wrong, but that's not my understanding of ->bits_per_word. To me, it's something that you can use when your *device* (not controller) expects non-byte aligned words [1]. The spi-mem protocol is definitely designed to work with 1byte large words, so, as I said, I suspect you're abusing xfer->bits_per_word to address a controller driver issue. [1]https://elixir.bootlin.com/linux/latest/source/include/linux/spi/spi.h#L114
> -----Original Message----- > From: Boris Brezillon <boris.brezillon@bootlin.com> > Sent: 2018年9月28日 15:19 > 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 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function > > On Fri, 28 Sep 2018 06:59:58 +0000 > Chuanhua Han <chuanhua.han@nxp.com> wrote: > > > > > > > It's still unclear why you need to specify a bits_per_word value, > > > but if this is needed, it's probably something you want to add to > > > spi.c, when a message is queued. > > To specify a specific bits_per_word to be able to use the xspi > > (32bit) mode of the fsl_dspi module to transfer data, you can look at > > my PATCH 2/2. Do not add a value in spis.c that takes into account > > that the value assigned to bits_per_word is decided before the > > transfer. Thanks for your check and reply! > > I might be wrong, but that's not my understanding of ->bits_per_word. > To me, it's something that you can use when your *device* (not > controller) expects non-byte aligned words [1]. The spi-mem protocol is > definitely designed to work with 1byte large words, so, as I said, I suspect > you're abusing xfer->bits_per_word to address a controller driver issue. > > [1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Feli > xir.bootlin.com%2Flinux%2Flatest%2Fsource%2Finclude%2Flinux%2Fspi%2Fspi > .h%23L114&data=02%7C01%7Cchuanhua.han%40nxp.com%7C7ec61d6d > 7ef741aba84408d62512a9e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 > %7C0%7C636737159420820050&sdata=iwFgXG3yFvH7Ruac7MGCoaP8h > l2M916m9%2BZeV7nksTg%3D&reserved=0 Ah, I'll study it. Thank you！
On Fri, Sep 28, 2018 at 09:18:33AM +0200, Boris Brezillon wrote: > I might be wrong, but that's not my understanding of ->bits_per_word. > To me, it's something that you can use when your *device* (not > controller) expects non-byte aligned words [1]. The spi-mem protocol is Yes, bits per word is a device setting. SPI devices can have control protocols that have words larger than a byte with a fixed byte order, many controllers do the required byte swapping.
> -----Original Message----- > From: Boris Brezillon <boris.brezillon@bootlin.com> > Sent: 2018年9月28日 15:19 > 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 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function > > On Fri, 28 Sep 2018 06:59:58 +0000 > Chuanhua Han <chuanhua.han@nxp.com> wrote: > > > > > > > It's still unclear why you need to specify a bits_per_word value, > > > but if this is needed, it's probably something you want to add to > > > spi.c, when a message is queued. > > To specify a specific bits_per_word to be able to use the xspi > > (32bit) mode of the fsl_dspi module to transfer data, you can look at > > my PATCH 2/2. Do not add a value in spis.c that takes into account > > that the value assigned to bits_per_word is decided before the > > transfer. Thanks for your check and reply! > > I might be wrong, but that's not my understanding of ->bits_per_word. > To me, it's something that you can use when your *device* (not > controller) expects non-byte aligned words [1]. The spi-mem protocol is > definitely designed to work with 1byte large words, so, as I said, I suspect > you're abusing xfer->bits_per_word to address a controller driver issue. > > [1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Feli > xir.bootlin.com%2Flinux%2Flatest%2Fsource%2Finclude%2Flinux%2Fspi%2Fspi > .h%23L114&data=02%7C01%7Cchuanhua.han%40nxp.com%7C7ec61d6d > 7ef741aba84408d62512a9e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 > %7C0%7C636737159420820050&sdata=iwFgXG3yFvH7Ruac7MGCoaP8h > l2M916m9%2BZeV7nksTg%3D&reserved=0 1. In the dspi driver (spi controller), bits_per_word (dspi->bits_per_word = transfer->bits_per_word) passed from the upper layer (spi-mem.c) is used. In this way, I can only assign the appropriate value of transfer->bits_per_word before passing to the controller, that is, the controller driver does not know the value of bits_per_word, and it will use this value when the upper level sets what value is passed. 2. As I understand, bits_per_word does not exist for non-byte alignment, but for the need to reserve non-byte transmission mode that meets the controller. 3. In addition, now the XSPI of dspi cannot transfer data normally, so this problem needs to be solved. As for the DMA transfer mode, some colleagues will study it.
On Tue, 9 Oct 2018 09:52:23 +0000 Chuanhua Han <chuanhua.han@nxp.com> wrote: > 1. In the dspi driver (spi controller), bits_per_word > (dspi->bits_per_word = transfer->bits_per_word) passed from the upper > layer (spi-mem.c) is used. In this way, I can only assign the > appropriate value of transfer->bits_per_word before passing to the > controller, that is, the controller driver does not know the value of > bits_per_word, and it will use this value when the upper level sets > what value is passed. I think you're missing my point: ->bits_per_word is not what you're looking for if what you're trying to do is use 32-bits accesses when things are properly aligned. > 2. As I understand, bits_per_word does not > exist for non-byte alignment, but for the need to reserve non-byte > transmission mode that meets the controller. Exactly. It's an optimization you have to take care of inside your driver. The core cannot help you with that. > 3. In addition, now the > XSPI of dspi cannot transfer data normally, so this problem needs to > be solved. I still don't understand what the problem is. > As for the DMA transfer mode, some colleagues will study > it.
> -----Original Message----- > From: Boris Brezillon <boris.brezillon@bootlin.com> > Sent: 2018年10月9日 18:05 > 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 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function > > On Tue, 9 Oct 2018 09:52:23 +0000 > Chuanhua Han <chuanhua.han@nxp.com> wrote: > > > 1. In the dspi driver (spi controller), bits_per_word > > (dspi->bits_per_word = transfer->bits_per_word) passed from the upper > > layer (spi-mem.c) is used. In this way, I can only assign the > > appropriate value of transfer->bits_per_word before passing to the > > controller, that is, the controller driver does not know the value of > > bits_per_word, and it will use this value when the upper level sets > > what value is passed. > > I think you're missing my point: ->bits_per_word is not what you're looking for > if what you're trying to do is use 32-bits accesses when things are properly > aligned. > In the dspi driver (spi controller driver), it is based on whether ->bits_per_word is larger than 16 to decide whether to use the XSPI mode (32bit) to transfer data. If ->bits_per_word is not set and the default bits_per_word =8 is passed to the dspi driver, the XSPI mode (32bit) is not used for data transfer in the dspi driver > > 2. As I understand, bits_per_word does not exist for non-byte > > alignment, but for the need to reserve non-byte transmission mode that > > meets the controller. > > Exactly. It's an optimization you have to take care of inside your driver. The core > cannot help you with that. > The core layer is the upper layer. If you don't set ->bits_per_word, bits_per_word will use the default value of 8, so that the controller's specific mode for transferring data cannot be used (eg: XSPI mode). > > 3. In addition, now the > > XSPI of dspi cannot transfer data normally, so this problem needs to > > be solved. > > I still don't understand what the problem is. > The problem is that I tested the XSPI mode and could not work, that is, the data could not be transmitted normally. I used spi flash connected on dspi to conduct the test. In any case, the controller is independent of connected slave devices, and the data should be transmitted by spi-flash devices and other spi devices. > > As for the DMA transfer mode, some colleagues will study it.
On Tue, Oct 09, 2018 at 12:05:22PM +0200, Boris Brezillon wrote: > On Tue, 9 Oct 2018 09:52:23 +0000 > Chuanhua Han <chuanhua.han@nxp.com> wrote: > > 1. In the dspi driver (spi controller), bits_per_word > > (dspi->bits_per_word = transfer->bits_per_word) passed from the upper > > layer (spi-mem.c) is used. In this way, I can only assign the > > appropriate value of transfer->bits_per_word before passing to the > > controller, that is, the controller driver does not know the value of > > bits_per_word, and it will use this value when the upper level sets > > what value is passed. > I think you're missing my point: ->bits_per_word is not what you're > looking for if what you're trying to do is use 32-bits accesses when > things are properly aligned. To be clear: bits_per_word affects what goes out on the SPI bus (4 byte words swapped to be in MSB first order), it needn't have any effect on on what goes on inside the SoC - many controllers fill their FIFO in 32 bit blocks even when sending 8 bit SPI words.
Chuanhua Han <chuanhua.han@nxp.com> writes: > 1. In the dspi driver (spi controller), bits_per_word (dspi->bits_per_word = > transfer->bits_per_word) passed from the upper layer (spi-mem.c) is used. > In this way, I can only assign the appropriate value of > transfer->bits_per_word before passing to the controller, that is, the > controller driver does not > know the value of bits_per_word, and it will use this value when the upper > level sets what value is passed. Yes, the upper layer (spi-mem.c) should set ->bits_per_word according to how the SPI data is to be transfered on the wire. In this case (I haven't looked at spi-mem.c myself), it sounds like the desired value is 8. So it is set to 8, and that should definitely not be changed by dspi driver. > 2. As I understand, bits_per_word does not exist for non-byte alignment, but > for the need to reserve non-byte transmission mode that meets the > controller. Where did you get that understanding from? The bits_per_word value defines the word size in bits for all transfers. > 3. In addition, now the XSPI of dspi cannot transfer data normally, so this > problem needs to be solved. As for the DMA transfer mode, some colleagues will > study it. What do you mean that "XSPI of dspi cannot tranffer data normally"? What specific problems do you see (with unpatched mainline kernel)? /Esben
Chuanhua Han <chuanhua.han@nxp.com> writes: >> -----Original Message----- >> From: Boris Brezillon <boris.brezillon@bootlin.com> >> Sent: 2018年10月9日 18:05 >> 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 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function >> >> On Tue, 9 Oct 2018 09:52:23 +0000 >> Chuanhua Han <chuanhua.han@nxp.com> wrote: >> >> > 1. In the dspi driver (spi controller), bits_per_word >> > (dspi->bits_per_word = transfer->bits_per_word) passed from the upper >> > layer (spi-mem.c) is used. In this way, I can only assign the >> > appropriate value of transfer->bits_per_word before passing to the >> > controller, that is, the controller driver does not know the value of >> > bits_per_word, and it will use this value when the upper level sets >> > what value is passed. >> >> I think you're missing my point: ->bits_per_word is not what you're looking >> for >> if what you're trying to do is use 32-bits accesses when things are properly >> aligned. >> > In the dspi driver (spi controller driver), it is based on whether > ->bits_per_word is > larger than 16 to decide whether to use the XSPI mode (32bit) to transfer data. Not completely true. XSPI mode is enabled, also for words smaller than or equal to 16 bits. But TX FIFO and CMD FIFO is written together, just as for non-XSPI mode. > If ->bits_per_word is not set and the default bits_per_word =8 is passed to the > dspi driver, the XSPI mode (32bit) is not used for data transfer in the dspi > driver Not true. XSPI mode is unconditionally enabled in dspi_init(). But XSPI mode does not overrule the value of transfer->bits_per_word. The meaning of XSPI mode is the following: 1. Frame (word) size is max 32 bits (with normal SPI mode, max is 16 bits). 2. For frames (words) with more than 16 bits per frame (word), each frame transfer results in 2 TX FIFO pop operations. 3. Command cycling is possible, enabled when SPI_CTARE[DTCP] > 1. Command cycling is currently not implemented. If implemented, it would be possible to send multiple frames (words) by writing one time to CMD FIFO and multiple times to TX FIFO. This could possibly improve performance Another possibility would be to use EOQ mode, if supported by the DSPI controller in the CPU. It allows for filling TX FIFO, and getting IRQ only after last TX FIFO entry is sent. This is also a likely performance improvement. >> > 2. As I understand, bits_per_word does not exist for non-byte >> > alignment, but for the need to reserve non-byte transmission mode that >> > meets the controller. >> >> Exactly. It's an optimization you have to take care of inside your >> driver. The core >> cannot help you with that. >> > The core layer is the upper layer. If you don't set ->bits_per_word, > bits_per_word will use the default value of 8, so that the > controller's specific mode for transferring data cannot be used (eg: > XSPI mode). XSPI mode is independent of bits_per_word. In XSPI mode, you can send frames as small as 4 bits, and up to 32 bits. In normal SPI mode, you can send frames from 4 to 16 bits. >> > 3. In addition, now the >> > XSPI of dspi cannot transfer data normally, so this problem needs to >> > be solved. >> >> I still don't understand what the problem is. >> > The problem is that I tested the XSPI mode and could not work, that > is, the data could not be transmitted normally. What does "could not be transmitted normally" mean? I am using XSPI mode on LS1021A, talking to a lot of different SPI devices. And they all work, and I believe everything is quite "normal". /Esben
Mark Brown <broonie@kernel.org> writes: > On Tue, Oct 09, 2018 at 12:05:22PM +0200, Boris Brezillon wrote: >> On Tue, 9 Oct 2018 09:52:23 +0000 >> Chuanhua Han <chuanhua.han@nxp.com> wrote: > >> > 1. In the dspi driver (spi controller), bits_per_word >> > (dspi->bits_per_word = transfer->bits_per_word) passed from the upper >> > layer (spi-mem.c) is used. In this way, I can only assign the >> > appropriate value of transfer->bits_per_word before passing to the >> > controller, that is, the controller driver does not know the value of >> > bits_per_word, and it will use this value when the upper level sets >> > what value is passed. > >> I think you're missing my point: ->bits_per_word is not what you're >> looking for if what you're trying to do is use 32-bits accesses when >> things are properly aligned. > > To be clear: bits_per_word affects what goes out on the SPI bus (4 byte > words swapped to be in MSB first order), it needn't have any effect on > on what goes on inside the SoC - many controllers fill their FIFO in 32 > bit blocks even when sending 8 bit SPI words. Exactly. I believe behind all the confusion here, is the fact that the current spi-fsl-dspi.c driver does not utilize all the possible features of the DSPI controller. 1. DMA support requires a nasty workaround before it can be enabled for LS1021A. 2. EOQ mode is not enabled (and not tested) for some platforms which might actually support it. 3. Cyclic command transfer is not implemented. So in order to improve performance on LS1021A, I propose to do work on above issues. I believe the performance improvement that can be gained is likely to be biggest for 1 (DMA support), and smallest for 3 (cyclic command transfer). I don't have access to any coldfire boards, so I haven't tested EOQ mode (it is only enabled for coldfire currently), so I cannot really know if there are some bugs hiding there. /Esben
> -----Original Message----- > From: Esben Haabendal <esbenhaabendal@gmail.com> On Behalf Of Esben > Haabendal > Sent: 2018年10月9日 19:21 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: Boris Brezillon <boris.brezillon@bootlin.com>; broonie@kernel.org; > linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function > > Chuanhua Han <chuanhua.han@nxp.com> writes: > > >> -----Original Message----- > >> From: Boris Brezillon <boris.brezillon@bootlin.com> > >> Sent: 2018年10月9日 18:05 > >> 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 1/2] spi: spi-mem: Add the spi_set_xfer_bpw > >> function > >> > >> On Tue, 9 Oct 2018 09:52:23 +0000 > >> Chuanhua Han <chuanhua.han@nxp.com> wrote: > >> > >> > 1. In the dspi driver (spi controller), bits_per_word > >> > (dspi->bits_per_word = transfer->bits_per_word) passed from the > >> > upper layer (spi-mem.c) is used. In this way, I can only assign the > >> > appropriate value of transfer->bits_per_word before passing to the > >> > controller, that is, the controller driver does not know the value > >> > of bits_per_word, and it will use this value when the upper level > >> > sets what value is passed. > >> > >> I think you're missing my point: ->bits_per_word is not what you're > >> looking for if what you're trying to do is use 32-bits accesses when > >> things are properly aligned. > >> > > In the dspi driver (spi controller driver), it is based on whether > > ->bits_per_word is > > larger than 16 to decide whether to use the XSPI mode (32bit) to transfer > data. > > Not completely true. XSPI mode is enabled, also for words smaller than or > equal to 16 bits. But TX FIFO and CMD FIFO is written together, just as for > non-XSPI mode. > > > If ->bits_per_word is not set and the default bits_per_word =8 is > > passed to the dspi driver, the XSPI mode (32bit) is not used for data > > transfer in the dspi driver > > Not true. XSPI mode is unconditionally enabled in dspi_init(). But XSPI > mode does not overrule the value of transfer->bits_per_word. > The meaning of XSPI mode is the following: > > 1. Frame (word) size is max 32 bits (with normal SPI mode, max is 16 bits). > 2. For frames (words) with more than 16 bits per frame (word), each frame > transfer results in 2 TX FIFO pop operations. > 3. Command cycling is possible, enabled when SPI_CTARE[DTCP] > 1. > > Command cycling is currently not implemented. If implemented, it would be > possible to send multiple frames (words) by writing one time to CMD FIFO and > multiple times to TX FIFO. This could possibly improve performance > > Another possibility would be to use EOQ mode, if supported by the DSPI > controller in the CPU. It allows for filling TX FIFO, and getting IRQ only after > last TX FIFO entry is sent. This is also a likely performance improvement. > > >> > 2. As I understand, bits_per_word does not exist for non-byte > >> > alignment, but for the need to reserve non-byte transmission mode > >> > that meets the controller. > >> > >> Exactly. It's an optimization you have to take care of inside your > >> driver. The core cannot help you with that. > >> > > The core layer is the upper layer. If you don't set ->bits_per_word, > > bits_per_word will use the default value of 8, so that the > > controller's specific mode for transferring data cannot be used (eg: > > XSPI mode). > > XSPI mode is independent of bits_per_word. In XSPI mode, you can send > frames as small as 4 bits, and up to 32 bits. In normal SPI mode, you can > send frames from 4 to 16 bits. > > >> > 3. In addition, now the > >> > XSPI of dspi cannot transfer data normally, so this problem needs > >> > to be solved. > >> > >> I still don't understand what the problem is. > >> > > The problem is that I tested the XSPI mode and could not work, that > > is, the data could not be transmitted normally. > > What does "could not be transmitted normally" mean? > > I am using XSPI mode on LS1021A, talking to a lot of different SPI devices. > And they all work, and I believe everything is quite "normal". > Since I don't have the board of LS1021, I can't test it. I use other boards with DSPI (such as LS1043, LS2088, etc.), and I test sp-flash connected on DSPI. If XSPI mode is used at this time, data cannot be transmitted normally. > /Esben
Chuanhua Han <chuanhua.han@nxp.com> writes: >> I am using XSPI mode on LS1021A, talking to a lot of different SPI devices. >> And they all work, and I believe everything is quite "normal". >> > Since I don't have the board of LS1021, I can't test it. I use other boards > with DSPI (such as LS1043, LS2088, etc.), > and I test sp-flash connected on DSPI. If XSPI mode is used at this time, data > cannot be transmitted normally. Please describe what happens in much more detailed terms that "cannot be transmitted normally". It is simply impossible to assist you when you report problems in such general terms. /Esben
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> --- drivers/spi/spi-mem.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)