LinuxLists.cc - [PATCH v2 1/4] spi: spi-mem: Add the spi_set_xfer

[permalink] [raw]

Subject: [PATCH v2 4/4] spi: spi-fsl-dspi: Fix adjust the byte order when sending and receiving data

This patch fixes the byte order inversion problem in the XSPI mode of
the dspi controller during data transfer.
In XSPI mode,When I read and write data without converting the byte
order of the data, and read and write the data directly, I tested spi
flash connected by the dspi controller and found that the byte
order of the data was reversed by the correct byte order.
When I changed the byte order according to the SPIx_CTARn[LSBFE] flag,
the correct data was obtained.

Signed-off-by: Chuanhua Han <[email protected]>
---
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-fsl-dspi.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 96e790e90997..44cc2bd0120e 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -220,9 +220,15 @@ static u32 dspi_pop_tx(struct fsl_dspi *dspi)
if (dspi->bytes_per_word == 1)
txdata = *(u8 *)dspi->tx;
else if (dspi->bytes_per_word == 2)
- txdata = *(u16 *)dspi->tx;
+ if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
+ txdata = cpu_to_le16(*(u16 *)dspi->tx);
+ else
+ txdata = cpu_to_be16(*(u16 *)dspi->tx);
else /* dspi->bytes_per_word == 4 */
- txdata = *(u32 *)dspi->tx;
+ if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
+ txdata = cpu_to_le32(*(u32 *)dspi->tx);
+ else
+ txdata = cpu_to_be32(*(u32 *)dspi->tx);
dspi->tx += dspi->bytes_per_word;
}
dspi->len -= dspi->bytes_per_word;
@@ -246,9 +252,15 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
if (dspi->bytes_per_word == 1)
*(u8 *)dspi->rx = rxdata;
else if (dspi->bytes_per_word == 2)
- *(u16 *)dspi->rx = rxdata;
+ if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
+ *(u16 *)dspi->rx = be16_to_cpu(rxdata);
+ else
+ *(u16 *)dspi->rx = be16_to_cpu(rxdata);
else /* dspi->bytes_per_word == 4 */
- *(u32 *)dspi->rx = rxdata;
+ if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
+ *(u32 *)dspi->rx = le32_to_cpu(rxdata);
+ else
+ *(u32 *)dspi->rx = be32_to_cpu(rxdata);
dspi->rx += dspi->bytes_per_word;
}

@@ -593,12 +605,12 @@ static void dspi_tcfq_write(struct fsl_dspi *dspi)
cmd_fifo_write(dspi);
if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1)) {
/* LSB */
- tx_fifo_write(dspi, data & 0xFFFF);
tx_fifo_write(dspi, data >> 16);
+ tx_fifo_write(dspi, data & 0xFFFF);
} else {
/* MSB */
- tx_fifo_write(dspi, data >> 16);
tx_fifo_write(dspi, data & 0xFFFF);
+ tx_fifo_write(dspi, data >> 16);
}
} else {
/* Write one entry to both TX FIFO and CMD FIFO
--
2.17.1

2018-09-30 09:26:13

[permalink] [raw]

Subject: [PATCH v2 3/4] spi: spi-fsl-dspi: Fix cmd_fifo is written before tx_fifo

This patch fixes the problem of invalid data writing during the XSPI
mode transfer of the dspi controller.
In XSPI mode,When I executed TX FIFO first and then CMD FIFO for XSPI
transmission, I found that SPIx_SR[TFIWF]=1(Invalid Data present in TX
FIFO since CMD FIFO is empty).
This is the time when no data can be read or written (all the data
obtained is equal to 0).

Signed-off-by: Chuanhua Han <[email protected]>
---
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-fsl-dspi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 4dc1064bf408..96e790e90997 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -590,6 +590,7 @@ static void dspi_tcfq_write(struct fsl_dspi *dspi)
*/
u32 data = dspi_pop_tx(dspi);

+ cmd_fifo_write(dspi);
if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1)) {
/* LSB */
tx_fifo_write(dspi, data & 0xFFFF);
@@ -599,7 +600,6 @@ static void dspi_tcfq_write(struct fsl_dspi *dspi)
tx_fifo_write(dspi, data >> 16);
tx_fifo_write(dspi, data & 0xFFFF);
}
- cmd_fifo_write(dspi);
} else {
/* Write one entry to both TX FIFO and CMD FIFO
* simultaneously.
--
2.17.1

2018-09-30 09:27:36

[permalink] [raw]

Subject: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of undefined bitmask for rxdata

This patch fixes the problem of rxdata being equal to 0 during the XSPI
mode transfer of the dspi controller.
In XSPI mode, If it is not deleted, the value of rxdata will be equal
to 0, and the data received will not be received correctly, causing the
receiving transfer of the spi to fail.

Signed-off-by: Chuanhua Han <[email protected]>
---
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-fsl-dspi.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 3082e72e4f6c..4dc1064bf408 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
if (!dspi->rx)
return;

- /* Mask of undefined bits */
- rxdata &= (1 << dspi->bits_per_word) - 1;
-
if (dspi->bytes_per_word == 1)
*(u8 *)dspi->rx = rxdata;
else if (dspi->bytes_per_word == 2)
--
2.17.1

2018-09-30 10:05:00

[permalink] [raw]

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 <[email protected]> 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 <[email protected]>
> ---
> 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;

2018-09-30 10:07:22

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of undefined bitmask for rxdata

On Sun, 30 Sep 2018 17:25:33 +0800
Chuanhua Han <[email protected]> wrote:

> This patch fixes the problem of rxdata being equal to 0 during the XSPI
> mode transfer of the dspi controller.
> In XSPI mode, If it is not deleted, the value of rxdata will be equal
> to 0, and the data received will not be received correctly, causing the
> receiving transfer of the spi to fail.
>
> Signed-off-by: Chuanhua Han <[email protected]>
> ---
> 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-fsl-dspi.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 3082e72e4f6c..4dc1064bf408 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
> if (!dspi->rx)
> return;
>
> - /* Mask of undefined bits */
> - rxdata &= (1 << dspi->bits_per_word) - 1;
> -

Why not

if (dspi->bits_per_word)
rxdata &= (1 << dspi->bits_per_word) - 1;

> if (dspi->bytes_per_word == 1)
> *(u8 *)dspi->rx = rxdata;
> else if (dspi->bytes_per_word == 2)

2018-09-30 10:12:19

[permalink] [raw]

Subject: RE: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of undefined bitmask for rxdata

> -----Original Message-----
> From: Boris Brezillon <[email protected]>
> Sent: 2018??9??30?? 18:07
> To: Chuanhua Han <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of
> undefined bitmask for rxdata
>
> On Sun, 30 Sep 2018 17:25:33 +0800
> Chuanhua Han <[email protected]> wrote:
>
> > This patch fixes the problem of rxdata being equal to 0 during the
> > XSPI mode transfer of the dspi controller.
> > In XSPI mode, If it is not deleted, the value of rxdata will be equal
> > to 0, and the data received will not be received correctly, causing
> > the receiving transfer of the spi to fail.
> >
> > Signed-off-by: Chuanhua Han <[email protected]>
> > ---
> > 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-fsl-dspi.c | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> > index 3082e72e4f6c..4dc1064bf408 100644
> > --- a/drivers/spi/spi-fsl-dspi.c
> > +++ b/drivers/spi/spi-fsl-dspi.c
> > @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32
> rxdata)
> > if (!dspi->rx)
> > return;
> >
> > - /* Mask of undefined bits */
> > - rxdata &= (1 << dspi->bits_per_word) - 1;
> > -
>
> Why not
In xspi mode, the value of rxdata after the statement is processed is equal to 0 no matter what data is received.
>
> if (dspi->bits_per_word)
> rxdata &= (1 << dspi->bits_per_word) - 1;
>
> > if (dspi->bytes_per_word == 1)
> > *(u8 *)dspi->rx = rxdata;
> > else if (dspi->bytes_per_word == 2)

2018-09-30 10:17:39

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of undefined bitmask for rxdata

On Sun, 30 Sep 2018 10:10:14 +0000
Chuanhua Han <[email protected]> wrote:

> > -----Original Message-----
> > From: Boris Brezillon <[email protected]>
> > Sent: 2018年9月30日 18:07
> > To: Chuanhua Han <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of
> > undefined bitmask for rxdata
> >
> > On Sun, 30 Sep 2018 17:25:33 +0800
> > Chuanhua Han <[email protected]> wrote:
> >
> > > This patch fixes the problem of rxdata being equal to 0 during the
> > > XSPI mode transfer of the dspi controller.
> > > In XSPI mode, If it is not deleted, the value of rxdata will be equal
> > > to 0, and the data received will not be received correctly, causing
> > > the receiving transfer of the spi to fail.
> > >
> > > Signed-off-by: Chuanhua Han <[email protected]>
> > > ---
> > > 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-fsl-dspi.c | 3 ---
> > > 1 file changed, 3 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> > > index 3082e72e4f6c..4dc1064bf408 100644
> > > --- a/drivers/spi/spi-fsl-dspi.c
> > > +++ b/drivers/spi/spi-fsl-dspi.c
> > > @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32
> > rxdata)
> > > if (!dspi->rx)
> > > return;
> > >
> > > - /* Mask of undefined bits */
> > > - rxdata &= (1 << dspi->bits_per_word) - 1;
> > > -
> >
> > Why not
> In xspi mode, the value of rxdata after the statement is processed is equal to 0 no matter what data is received.

Only if dspi->bits_per_word is 0.

Actually, I just had a look, and xfer->bits_per_word should never be 0
because spi_validate() makes sure it's initialized [1]. Don't know
where dpsi->bits_per_word comes from, but maybe you have a problem
there (dpsi->bits_per_word and xfer->bits_per_word not in sync).

[1]https://elixir.bootlin.com/linux/v4.19-rc5/source/drivers/spi/spi.c#L2869

2018-09-30 10:18:16

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] spi: spi-mem: Add the spi_set_xfer_bpw function

Boris Brezillon <[email protected]> writes:

> Hi Chuanhua,
>
> On Sun, 30 Sep 2018 17:25:32 +0800
> Chuanhua Han <[email protected]> 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

2018-09-30 10:19:03

[permalink] [raw]

Subject: RE: [PATCH v2 1/4] spi: spi-mem: Add the spi_set_xfer_bpw function

> -----Original Message-----
> From: Boris Brezillon <[email protected]>
> Sent: 2018??9??30?? 18:04
> To: Chuanhua Han <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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;

2018-09-30 10:27:49

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] spi: spi-fsl-dspi: Fix adjust the byte order when sending and receiving data

Chuanhua Han <[email protected]> writes:

> This patch fixes the byte order inversion problem in the XSPI mode of
> the dspi controller during data transfer.
> In XSPI mode,When I read and write data without converting the byte
> order of the data, and read and write the data directly, I tested spi
> flash connected by the dspi controller and found that the byte
> order of the data was reversed by the correct byte order.
> When I changed the byte order according to the SPIx_CTARn[LSBFE] flag,
> the correct data was obtained.

I believe this is related to patch 1/4 of this series, and your attempt
on pushing the 8-bit spi-mem data into 32-bit SPI words. The
byte-ordering for that does not belong here, and will likely break
byte-ordering for other (proper) use of XSPI mode.

My advice is that you focus your effort on implementing/fixing DMA mode,
ie. erratum A-011218.
A proper implementation of that will be appreciated, and should give you
much better performance than XSPI mode would be able to give you.

/Esben

2018-09-30 10:29:46

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of undefined bitmask for rxdata

Chuanhua Han <[email protected]> writes:

> This patch fixes the problem of rxdata being equal to 0 during the XSPI
> mode transfer of the dspi controller.
> In XSPI mode, If it is not deleted, the value of rxdata will be equal
> to 0, and the data received will not be received correctly, causing the
> receiving transfer of the spi to fail.
>
> Signed-off-by: Chuanhua Han <[email protected]>
> ---
> 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-fsl-dspi.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 3082e72e4f6c..4dc1064bf408 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
> if (!dspi->rx)
> return;
>
> - /* Mask of undefined bits */
> - rxdata &= (1 << dspi->bits_per_word) - 1;

What is the dspi->bits_per_word value when your rxdata is set equal to
0? Could this perhaps also be related to byte ordering problems?

> if (dspi->bytes_per_word == 1)
> *(u8 *)dspi->rx = rxdata;
> else if (dspi->bytes_per_word == 2)

/Esben

2018-09-30 10:32:07

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] spi: spi-fsl-dspi: Fix cmd_fifo is written before tx_fifo

Chuanhua Han <[email protected]> writes:

> This patch fixes the problem of invalid data writing during the XSPI
> mode transfer of the dspi controller.
> In XSPI mode,When I executed TX FIFO first and then CMD FIFO for XSPI
> transmission, I found that SPIx_SR[TFIWF]=1(Invalid Data present in TX
> FIFO since CMD FIFO is empty).
> This is the time when no data can be read or written (all the data
> obtained is equal to 0).
>
> Signed-off-by: Chuanhua Han <[email protected]>
> ---
> 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-fsl-dspi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 4dc1064bf408..96e790e90997 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -590,6 +590,7 @@ static void dspi_tcfq_write(struct fsl_dspi *dspi)
> */
> u32 data = dspi_pop_tx(dspi);
>
> + cmd_fifo_write(dspi);
> if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1)) {
> /* LSB */
> tx_fifo_write(dspi, data & 0xFFFF);
> @@ -599,7 +600,6 @@ static void dspi_tcfq_write(struct fsl_dspi *dspi)
> tx_fifo_write(dspi, data >> 16);
> tx_fifo_write(dspi, data & 0xFFFF);
> }
> - cmd_fifo_write(dspi);
> } else {
> /* Write one entry to both TX FIFO and CMD FIFO
> * simultaneously.

I will try and find time to test this on our systems at work.

/Esben

2018-09-30 10:37:38

[permalink] [raw]

Subject: RE: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of undefined bitmask for rxdata

> -----Original Message-----
> From: Boris Brezillon <[email protected]>
> Sent: 2018年9月30日 18:17
> To: Chuanhua Han <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of
> undefined bitmask for rxdata
>
> On Sun, 30 Sep 2018 10:10:14 +0000
> Chuanhua Han <[email protected]> wrote:
>
> > > -----Original Message-----
> > > From: Boris Brezillon <[email protected]>
> > > Sent: 2018年9月30日 18:07
> > > To: Chuanhua Han <[email protected]>
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the
> > > processing of undefined bitmask for rxdata
> > >
> > > On Sun, 30 Sep 2018 17:25:33 +0800
> > > Chuanhua Han <[email protected]> wrote:
> > >
> > > > This patch fixes the problem of rxdata being equal to 0 during the
> > > > XSPI mode transfer of the dspi controller.
> > > > In XSPI mode, If it is not deleted, the value of rxdata will be
> > > > equal to 0, and the data received will not be received correctly,
> > > > causing the receiving transfer of the spi to fail.
> > > >
> > > > Signed-off-by: Chuanhua Han <[email protected]>
> > > > ---
> > > > 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-fsl-dspi.c | 3 ---
> > > > 1 file changed, 3 deletions(-)
> > > >
> > > > diff --git a/drivers/spi/spi-fsl-dspi.c
> > > > b/drivers/spi/spi-fsl-dspi.c index 3082e72e4f6c..4dc1064bf408
> > > > 100644
> > > > --- a/drivers/spi/spi-fsl-dspi.c
> > > > +++ b/drivers/spi/spi-fsl-dspi.c
> > > > @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi
> > > > *dspi, u32
> > > rxdata)
> > > > if (!dspi->rx)
> > > > return;
> > > >
> > > > - /* Mask of undefined bits */
> > > > - rxdata &= (1 << dspi->bits_per_word) - 1;
> > > > -
> > >
> > > Why not
> > In xspi mode, the value of rxdata after the statement is processed is equal to
> 0 no matter what data is received.
>
> Only if dspi->bits_per_word is 0.
>
> Actually, I just had a look, and xfer->bits_per_word should never be 0 because
> spi_validate() makes sure it's initialized [1]. Don't know where
> dpsi->bits_per_word comes from, but maybe you have a problem there
> (dpsi->bits_per_word and xfer->bits_per_word not in sync).
>
> [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
> L2869&data=02%7C01%7Cchuanhua.han%40nxp.com%7Cd92a3b54ccf0
> 4d1c1f3208d626bde775%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C636738994411369491&sdata=piLwfBc0kzMhOnI5uubHYJ9tbe%2BR
> KENKbYiLrkY1c30%3D&reserved=0
OK, Let me analyze it again,Thanks!

2018-09-30 10:38:12

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of undefined bitmask for rxdata

Boris Brezillon <[email protected]> writes:

> On Sun, 30 Sep 2018 10:10:14 +0000
> Chuanhua Han <[email protected]> wrote:
>
>> > -----Original Message-----
>> > From: Boris Brezillon <[email protected]>
>> > Sent: 2018年9月30日 18:07
>> > To: Chuanhua Han <[email protected]>
>> > Cc: [email protected]; [email protected];
>> > [email protected]; [email protected]
>> > Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of
>> > undefined bitmask for rxdata
>> >
>> > On Sun, 30 Sep 2018 17:25:33 +0800
>> > Chuanhua Han <[email protected]> wrote:
>> >
>> > > This patch fixes the problem of rxdata being equal to 0 during the
>> > > XSPI mode transfer of the dspi controller.
>> > > In XSPI mode, If it is not deleted, the value of rxdata will be equal
>> > > to 0, and the data received will not be received correctly, causing
>> > > the receiving transfer of the spi to fail.
>> > >
>> > > Signed-off-by: Chuanhua Han <[email protected]>
>> > > ---
>> > > 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-fsl-dspi.c | 3 ---
>> > > 1 file changed, 3 deletions(-)
>> > >
>> > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>> > > index 3082e72e4f6c..4dc1064bf408 100644
>> > > --- a/drivers/spi/spi-fsl-dspi.c
>> > > +++ b/drivers/spi/spi-fsl-dspi.c
>> > > @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32
>> > rxdata)
>> > > if (!dspi->rx)
>> > > return;
>> > >
>> > > - /* Mask of undefined bits */
>> > > - rxdata &= (1 << dspi->bits_per_word) - 1;
>> > > -
>> >
>> > Why not
>> In xspi mode, the value of rxdata after the statement is processed is equal
>> to 0 no matter what data is received.
>
> Only if dspi->bits_per_word is 0.
>
> Actually, I just had a look, and xfer->bits_per_word should never be 0
> because spi_validate() makes sure it's initialized [1]. Don't know
> where dpsi->bits_per_word comes from, but maybe you have a problem
> there (dpsi->bits_per_word and xfer->bits_per_word not in sync).
>
> [1]https://elixir.bootlin.com/linux/v4.19-rc5/source/drivers/spi/spi.c#L2869

dspi->bits_per_word = xfer->bits_per_word

https://elixir.bootlin.com/linux/v4.19-rc5/source/drivers/spi/spi-fsl-dspi.c#L697

So it should never be out of sync, and it should never be 0.

As I mentioned in another mail, I suspect what Han is observing is
caused by byte ordering, so that the mask masks the wrong data.
Maybe related to the byte-ordering fix patch.

/Esben

2018-09-30 10:40:48

[permalink] [raw]

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 <[email protected]> wrote:

> > -----Original Message-----
> > From: Boris Brezillon <[email protected]>
> > Sent: 2018年9月30日 18:04
> > To: Chuanhua Han <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]
> > 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 <[email protected]> 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

2018-09-30 10:41:18

[permalink] [raw]

Subject: RE: [PATCH v2 1/4] spi: spi-mem: Add the spi_set_xfer_bpw function

> -----Original Message-----
> From: Esben Haabendal <[email protected]> On Behalf Of Esben
> Haabendal
> Sent: 2018??9??30?? 18:18
> To: Boris Brezillon <[email protected]>
> Cc: Chuanhua Han <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v2 1/4] spi: spi-mem: Add the spi_set_xfer_bpw function
>
> Boris Brezillon <[email protected]> writes:
>
> > Hi Chuanhua,
> >
> > On Sun, 30 Sep 2018 17:25:32 +0800
> > Chuanhua Han <[email protected]> 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.

2018-09-30 10:42:29

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of undefined bitmask for rxdata

On Sun, 30 Sep 2018 12:37:38 +0200
Esben Haabendal <[email protected]> wrote:

> Boris Brezillon <[email protected]> writes:
>
> > On Sun, 30 Sep 2018 10:10:14 +0000
> > Chuanhua Han <[email protected]> wrote:
> >
> >> > -----Original Message-----
> >> > From: Boris Brezillon <[email protected]>
> >> > Sent: 2018年9月30日 18:07
> >> > To: Chuanhua Han <[email protected]>
> >> > Cc: [email protected]; [email protected];
> >> > [email protected]; [email protected]
> >> > Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of
> >> > undefined bitmask for rxdata
> >> >
> >> > On Sun, 30 Sep 2018 17:25:33 +0800
> >> > Chuanhua Han <[email protected]> wrote:
> >> >
> >> > > This patch fixes the problem of rxdata being equal to 0 during the
> >> > > XSPI mode transfer of the dspi controller.
> >> > > In XSPI mode, If it is not deleted, the value of rxdata will be equal
> >> > > to 0, and the data received will not be received correctly, causing
> >> > > the receiving transfer of the spi to fail.
> >> > >
> >> > > Signed-off-by: Chuanhua Han <[email protected]>
> >> > > ---
> >> > > 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-fsl-dspi.c | 3 ---
> >> > > 1 file changed, 3 deletions(-)
> >> > >
> >> > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> >> > > index 3082e72e4f6c..4dc1064bf408 100644
> >> > > --- a/drivers/spi/spi-fsl-dspi.c
> >> > > +++ b/drivers/spi/spi-fsl-dspi.c
> >> > > @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32
> >> > rxdata)
> >> > > if (!dspi->rx)
> >> > > return;
> >> > >
> >> > > - /* Mask of undefined bits */
> >> > > - rxdata &= (1 << dspi->bits_per_word) - 1;
> >> > > -
> >> >
> >> > Why not
> >> In xspi mode, the value of rxdata after the statement is processed is equal
> >> to 0 no matter what data is received.
> >
> > Only if dspi->bits_per_word is 0.
> >
> > Actually, I just had a look, and xfer->bits_per_word should never be 0
> > because spi_validate() makes sure it's initialized [1]. Don't know
> > where dpsi->bits_per_word comes from, but maybe you have a problem
> > there (dpsi->bits_per_word and xfer->bits_per_word not in sync).
> >
> > [1]https://elixir.bootlin.com/linux/v4.19-rc5/source/drivers/spi/spi.c#L2869
>
> dspi->bits_per_word = xfer->bits_per_word
>
> https://elixir.bootlin.com/linux/v4.19-rc5/source/drivers/spi/spi-fsl-dspi.c#L697
>
> So it should never be out of sync, and it should never be 0.
>
> As I mentioned in another mail, I suspect what Han is observing is
> caused by byte ordering, so that the mask masks the wrong data.
> Maybe related to the byte-ordering fix patch.

Okay. Wait and see then.

2018-09-30 10:48:58