| Message ID | 20180921070628.35153-2-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 2/2] spi: spi-fsl-dspi: Fix support for XSPI transport mode > > This patch fixes the problem that the XSPI mode of the dspi controller cannot > transfer data properly. > In XSPI mode, cmd_fifo is written before tx_fifo, which transforms the byte > order of sending and receiving data. > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > --- > drivers/spi/spi-fsl-dspi.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c index > 3082e72e4f6c..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; > @@ -243,15 +249,18 @@ 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) > - *(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,16 +602,16 @@ 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); > 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); > } > - cmd_fifo_write(dspi); > } else { > /* Write one entry to both TX FIFO and CMD FIFO > * simultaneously. > -- > 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:27 +0800 Chuanhua Han <chuanhua.han@nxp.com> wrote: > This patch fixes the problem that the XSPI mode of the dspi controller > cannot transfer data properly. > In XSPI mode, cmd_fifo is written before tx_fifo, which transforms the > byte order of sending and receiving data. Again, I have a hard time understanding what the problem is. It doesn't seem related to the ->bits_per_word aspect, and I have the feeling you're actually abusing this field to get your problem fixed. Regards, Boris > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > --- > drivers/spi/spi-fsl-dspi.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c > index 3082e72e4f6c..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; > @@ -243,15 +249,18 @@ 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) > - *(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,16 +602,16 @@ 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); > 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); > } > - cmd_fifo_write(dspi); > } else { > /* Write one entry to both TX FIFO and CMD FIFO > * simultaneously.
Chuanhua Han <chuanhua.han@nxp.com> writes: > This patch fixes the problem that the XSPI mode of the dspi controller > cannot transfer data properly. > In XSPI mode, cmd_fifo is written before tx_fifo, which transforms the > byte order of sending and receiving data. Did you find documentation on proper ordering of writes to related TX FIFO and CMD FIFO entries? I have failed to find such information, and thus opted for what I believed would be the safe approach, writing to TX FIFO first, so that when CMD FIFO is written, it will already have data in place. And it seems to work. But, I now see that SPIx_SR[TFIWF] hints that it should be done the other way around. Tranmit FIFO Invalid Write Flag - Indicates Data Write on TX FIFO while CMD FIFO is empty. Without a Command, the Data entries present in TXFIFO are invalid. But I fail to see how that should be related to byte ordering. So I believe this patch is doing two things. 1. Changing write ordering of TX FIFO and CMD FIFO. 2. Handling byte ordering based on SPIx_CTARn[LSBFE] flag. It would be nice if we could get clarification from NXP on what is the right way to do the TX FIFO and CMD FIFO write ordering. But as for the byte ordering changes, I don't think it looks write. The meaning of SPIx_CTARn[LSBFE] is according to the documentaiton the bit ordering on the wire, and should not be related to register byte ordering. You should probably split this patch in two, so they can be reviewed and merged independently. /Esben
Hi Esben, On Sat, 29 Sep 2018 16:56:17 +0200 Esben Haabendal <esben.haabendal@gmail.com> wrote: > Chuanhua Han <chuanhua.han@nxp.com> writes: > > > This patch fixes the problem that the XSPI mode of the dspi controller > > cannot transfer data properly. > > In XSPI mode, cmd_fifo is written before tx_fifo, which transforms the > > byte order of sending and receiving data. > > Did you find documentation on proper ordering of writes to related > TX FIFO and CMD FIFO entries? > > I have failed to find such information, and thus opted for what I > believed would be the safe approach, writing to TX FIFO first, so that > when CMD FIFO is written, it will already have data in place. And it > seems to work. > > But, I now see that SPIx_SR[TFIWF] hints that it should be done the > other way around. > > Tranmit FIFO Invalid Write Flag - Indicates Data Write on TX FIFO > while CMD FIFO is empty. Without a Command, the Data entries present > in TXFIFO are invalid. > > But I fail to see how that should be related to byte ordering. > > So I believe this patch is doing two things. > > 1. Changing write ordering of TX FIFO and CMD FIFO. > 2. Handling byte ordering based on SPIx_CTARn[LSBFE] flag. > > It would be nice if we could get clarification from NXP on > what is the right way to do the TX FIFO and CMD FIFO write ordering. > > But as for the byte ordering changes, I don't think it looks write. The > meaning of SPIx_CTARn[LSBFE] is according to the documentaiton the bit > ordering on the wire, and should not be related to register byte > ordering. > > You should probably split this patch in two, so they can be reviewed and > merged independently. Yes, I forgot to mention that, but this patch should definitely be split in at least 2 patches. Regards, Boris
> -----Original Message----- > From: Esben Haabendal <esbenhaabendal@gmail.com> On Behalf Of Esben > Haabendal > Sent: 2018年9月29日 22:56 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: broonie@kernel.org; linux-spi@vger.kernel.org; > linux-kernel@vger.kernel.org; boris.brezillon@bootlin.com > Subject: Re: [PATCH 2/2] spi: spi-fsl-dspi: Fix support for XSPI transport mode > > Chuanhua Han <chuanhua.han@nxp.com> writes: > > > This patch fixes the problem that the XSPI mode of the dspi controller > > cannot transfer data properly. > > In XSPI mode, cmd_fifo is written before tx_fifo, which transforms the > > byte order of sending and receiving data. > > Did you find documentation on proper ordering of writes to related TX FIFO > and CMD FIFO entries? > > I have failed to find such information, and thus opted for what I believed would > be the safe approach, writing to TX FIFO first, so that when CMD FIFO is > written, it will already have data in place. And it seems to work. > > But, I now see that SPIx_SR[TFIWF] hints that it should be done the other way > around. > > Tranmit FIFO Invalid Write Flag - Indicates Data Write on TX FIFO > while CMD FIFO is empty. Without a Command, the Data entries present > in TXFIFO are invalid. > > But I fail to see how that should be related to byte ordering. > > So I believe this patch is doing two things. > > 1. Changing write ordering of TX FIFO and CMD FIFO. > 2. Handling byte ordering based on SPIx_CTARn[LSBFE] flag. > > It would be nice if we could get clarification from NXP on what is the right way > to do the TX FIFO and CMD FIFO write ordering. > > But as for the byte ordering changes, I don't think it looks write. The meaning > of SPIx_CTARn[LSBFE] is according to the documentaiton the bit ordering on > the wire, and should not be related to register byte ordering. > > You should probably split this patch in two, so they can be reviewed and > merged independently. > > /Esben Hi, Esben First of all, thank you for your valuable advice. I'm going to do two things: 1. Divide this patch into multiple patches. 2. Verify (if you can) from the relevant NXP documentation. Second, let me mention the issues I found on fixing fsl_dspi's XSPI pattern not working: 1. 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). 2. When I read and write data without converting data byte order, and read and write data directly, I tested spi-flash connected by fsl_dspi controller, and found that data byte order was reversed with the correct byte order. When I changed the byte order according to the SPIx_CTARn[LSBFE] flag, the correct data was obtained (not explained in this data manual). 3. In the dspi_push_rx function, if you add your "rxdata &= (1 << dspi->bits_per_word) -1" statement, the rxdata=0 is obtained when xspi is transmitted, and the correct data cannot be transmitted at this time (I don't quite understand why you added this statement). Thanks, Chuanhua
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c index 3082e72e4f6c..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; @@ -243,15 +249,18 @@ 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) - *(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,16 +602,16 @@ 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); 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); } - cmd_fifo_write(dspi); } else { /* Write one entry to both TX FIFO and CMD FIFO * simultaneously.
This patch fixes the problem that the XSPI mode of the dspi controller cannot transfer data properly. In XSPI mode, cmd_fifo is written before tx_fifo, which transforms the byte order of sending and receiving data. Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> --- drivers/spi/spi-fsl-dspi.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)