When working in slave mode it seems the timing is exceedingly tight.
The TX FIFO can never empty, because the master is driving the clock so
zeros would be sent for those bytes where the FIFO is empty.
Return to interleaving the writing of the TX FIFO and the reading
of the RX FIFO to try to ensure the data is available when required.
Fixes: a84c11e16dc2 ("spi: spi-cadence: Avoid read of RX FIFO before its ready")
Signed-off-by: Charles Keepax <[email protected]>
---
Updates since v1:
- Update the kernel doc to match the changes
Thanks,
Charles
drivers/spi/spi-cadence.c | 64 ++++++++++++++++++---------------------
1 file changed, 30 insertions(+), 34 deletions(-)
diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
index ff02d81041319..26e6633693196 100644
--- a/drivers/spi/spi-cadence.c
+++ b/drivers/spi/spi-cadence.c
@@ -12,6 +12,7 @@
#include <linux/gpio/consumer.h>
#include <linux/interrupt.h>
#include <linux/io.h>
+#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of_irq.h>
#include <linux/of_address.h>
@@ -301,47 +302,43 @@ static int cdns_spi_setup_transfer(struct spi_device *spi,
}
/**
- * cdns_spi_fill_tx_fifo - Fills the TX FIFO with as many bytes as possible
+ * cdns_spi_process_fifo - Fills the TX FIFO, and drain the RX FIFO
* @xspi: Pointer to the cdns_spi structure
+ * @ntx: Number of bytes to pack into the TX FIFO
+ * @nrx: Number of bytes to drain from the RX FIFO
*/
-static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi, unsigned int avail)
+static void cdns_spi_process_fifo(struct cdns_spi *xspi, int ntx, int nrx)
{
- unsigned long trans_cnt = 0;
+ ntx = clamp(ntx, 0, xspi->tx_bytes);
+ nrx = clamp(nrx, 0, xspi->rx_bytes);
- while ((trans_cnt < avail) && (xspi->tx_bytes > 0)) {
+ xspi->tx_bytes -= ntx;
+ xspi->rx_bytes -= nrx;
+
+ while (ntx || nrx) {
/* When xspi in busy condition, bytes may send failed,
* then spi control did't work thoroughly, add one byte delay
*/
- if (cdns_spi_read(xspi, CDNS_SPI_ISR) &
- CDNS_SPI_IXR_TXFULL)
+ if (cdns_spi_read(xspi, CDNS_SPI_ISR) & CDNS_SPI_IXR_TXFULL)
udelay(10);
- if (xspi->txbuf)
- cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi->txbuf++);
- else
- cdns_spi_write(xspi, CDNS_SPI_TXD, 0);
+ if (ntx) {
+ if (xspi->txbuf)
+ cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi->txbuf++);
+ else
+ cdns_spi_write(xspi, CDNS_SPI_TXD, 0);
- xspi->tx_bytes--;
- trans_cnt++;
- }
-}
+ ntx--;
+ }
-/**
- * cdns_spi_read_rx_fifo - Reads the RX FIFO with as many bytes as possible
- * @xspi: Pointer to the cdns_spi structure
- * @count: Read byte count
- */
-static void cdns_spi_read_rx_fifo(struct cdns_spi *xspi, unsigned long count)
-{
- u8 data;
-
- /* Read out the data from the RX FIFO */
- while (count > 0) {
- data = cdns_spi_read(xspi, CDNS_SPI_RXD);
- if (xspi->rxbuf)
- *xspi->rxbuf++ = data;
- xspi->rx_bytes--;
- count--;
+ if (nrx) {
+ u8 data = cdns_spi_read(xspi, CDNS_SPI_RXD);
+
+ if (xspi->rxbuf)
+ *xspi->rxbuf++ = data;
+
+ nrx--;
+ }
}
}
@@ -391,11 +388,10 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_id)
if (xspi->tx_bytes < xspi->tx_fifo_depth >> 1)
cdns_spi_write(xspi, CDNS_SPI_THLD, 1);
- cdns_spi_read_rx_fifo(xspi, trans_cnt);
-
if (xspi->tx_bytes) {
- cdns_spi_fill_tx_fifo(xspi, trans_cnt);
+ cdns_spi_process_fifo(xspi, trans_cnt, trans_cnt);
} else {
+ cdns_spi_process_fifo(xspi, 0, trans_cnt);
cdns_spi_write(xspi, CDNS_SPI_IDR,
CDNS_SPI_IXR_DEFAULT);
spi_finalize_current_transfer(ctlr);
@@ -448,7 +444,7 @@ static int cdns_transfer_one(struct spi_controller *ctlr,
cdns_spi_write(xspi, CDNS_SPI_THLD, xspi->tx_fifo_depth >> 1);
}
- cdns_spi_fill_tx_fifo(xspi, xspi->tx_fifo_depth);
+ cdns_spi_process_fifo(xspi, xspi->tx_fifo_depth, 0);
spi_transfer_delay_exec(transfer);
cdns_spi_write(xspi, CDNS_SPI_IER, CDNS_SPI_IXR_DEFAULT);
--
2.30.2
On Thu, 18 May 2023 10:39:26 +0100, Charles Keepax wrote:
> When working in slave mode it seems the timing is exceedingly tight.
> The TX FIFO can never empty, because the master is driving the clock so
> zeros would be sent for those bytes where the FIFO is empty.
>
> Return to interleaving the writing of the TX FIFO and the reading
> of the RX FIFO to try to ensure the data is available when required.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
Thanks!
[1/2] spi: spi-cadence: Interleave write of TX and read of RX FIFO
commit: 6afe2ae8dc48e643cb9f52e86494b96942440bc6
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
Hi Charles,
>-----Original Message-----
>From: Charles Keepax <[email protected]>
>Sent: Thursday, May 18, 2023 3:09 PM
>To: [email protected]
>Cc: Goud, Srinivas <[email protected]>; [email protected];
>[email protected]; [email protected]
>Subject: [PATCH v2 1/2] spi: spi-cadence: Interleave write of TX and read of RX
>FIFO
>
>When working in slave mode it seems the timing is exceedingly tight.
>The TX FIFO can never empty, because the master is driving the clock so zeros
>would be sent for those bytes where the FIFO is empty.
>
>Return to interleaving the writing of the TX FIFO and the reading of the RX FIFO
>to try to ensure the data is available when required.
>
>Fixes: a84c11e16dc2 ("spi: spi-cadence: Avoid read of RX FIFO before its
>ready")
>Signed-off-by: Charles Keepax <[email protected]>
>---
>
>Updates since v1:
> - Update the kernel doc to match the changes
>
>Thanks,
>Charles
>
> drivers/spi/spi-cadence.c | 64 ++++++++++++++++++---------------------
> 1 file changed, 30 insertions(+), 34 deletions(-)
>
>diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c index
>ff02d81041319..26e6633693196 100644
>--- a/drivers/spi/spi-cadence.c
>+++ b/drivers/spi/spi-cadence.c
>@@ -12,6 +12,7 @@
> #include <linux/gpio/consumer.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
>+#include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of_irq.h>
> #include <linux/of_address.h>
>@@ -301,47 +302,43 @@ static int cdns_spi_setup_transfer(struct spi_device
>*spi, }
>
> /**
>- * cdns_spi_fill_tx_fifo - Fills the TX FIFO with as many bytes as possible
>+ * cdns_spi_process_fifo - Fills the TX FIFO, and drain the RX FIFO
> * @xspi: Pointer to the cdns_spi structure
>+ * @ntx: Number of bytes to pack into the TX FIFO
>+ * @nrx: Number of bytes to drain from the RX FIFO
> */
>-static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi, unsigned int avail)
>+static void cdns_spi_process_fifo(struct cdns_spi *xspi, int ntx, int
>+nrx)
> {
>- unsigned long trans_cnt = 0;
>+ ntx = clamp(ntx, 0, xspi->tx_bytes);
>+ nrx = clamp(nrx, 0, xspi->rx_bytes);
>
>- while ((trans_cnt < avail) && (xspi->tx_bytes > 0)) {
>+ xspi->tx_bytes -= ntx;
>+ xspi->rx_bytes -= nrx;
>+
>+ while (ntx || nrx) {
> /* When xspi in busy condition, bytes may send failed,
> * then spi control did't work thoroughly, add one byte delay
> */
>- if (cdns_spi_read(xspi, CDNS_SPI_ISR) &
>- CDNS_SPI_IXR_TXFULL)
>+ if (cdns_spi_read(xspi, CDNS_SPI_ISR) &
>CDNS_SPI_IXR_TXFULL)
> udelay(10);
Linux driver configured as Slave, due to this above delay we see data corruption issue on Master side.
when Master is continuously reading the data, Slave is failed to prepare the date on time due to above delay.
>
>- if (xspi->txbuf)
>- cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi->txbuf++);
>- else
>- cdns_spi_write(xspi, CDNS_SPI_TXD, 0);
>+ if (ntx) {
>+ if (xspi->txbuf)
>+ cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi-
>>txbuf++);
>+ else
>+ cdns_spi_write(xspi, CDNS_SPI_TXD, 0);
>
>- xspi->tx_bytes--;
>- trans_cnt++;
>- }
>-}
>+ ntx--;
>+ }
>
>-/**
>- * cdns_spi_read_rx_fifo - Reads the RX FIFO with as many bytes as possible
>- * @xspi: Pointer to the cdns_spi structure
>- * @count: Read byte count
>- */
>-static void cdns_spi_read_rx_fifo(struct cdns_spi *xspi, unsigned long count) -{
>- u8 data;
>-
>- /* Read out the data from the RX FIFO */
>- while (count > 0) {
>- data = cdns_spi_read(xspi, CDNS_SPI_RXD);
>- if (xspi->rxbuf)
>- *xspi->rxbuf++ = data;
>- xspi->rx_bytes--;
>- count--;
>+ if (nrx) {
>+ u8 data = cdns_spi_read(xspi, CDNS_SPI_RXD);
>+
>+ if (xspi->rxbuf)
>+ *xspi->rxbuf++ = data;
>+
>+ nrx--;
>+ }
> }
> }
>
>@@ -391,11 +388,10 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_id)
> if (xspi->tx_bytes < xspi->tx_fifo_depth >> 1)
> cdns_spi_write(xspi, CDNS_SPI_THLD, 1);
>
>- cdns_spi_read_rx_fifo(xspi, trans_cnt);
>-
> if (xspi->tx_bytes) {
>- cdns_spi_fill_tx_fifo(xspi, trans_cnt);
>+ cdns_spi_process_fifo(xspi, trans_cnt, trans_cnt);
> } else {
>+ cdns_spi_process_fifo(xspi, 0, trans_cnt);
When Linux driver configured as Slave, we observed data corruption issue with Slave mode read when data length is 400 bytes.
As TX empty doesn't guaranties valid data in RX FIFO, hence we added one byte delay(10us) before RX FIFO read to overcome above issue.
Created patch with above changes, find patch as attachment.
Can you please test and let me know your observations.
Thanks,
Srinivas
> cdns_spi_write(xspi, CDNS_SPI_IDR,
> CDNS_SPI_IXR_DEFAULT);
> spi_finalize_current_transfer(ctlr);
>@@ -448,7 +444,7 @@ static int cdns_transfer_one(struct spi_controller *ctlr,
> cdns_spi_write(xspi, CDNS_SPI_THLD, xspi-
>>tx_fifo_depth >> 1);
> }
>
>- cdns_spi_fill_tx_fifo(xspi, xspi->tx_fifo_depth);
>+ cdns_spi_process_fifo(xspi, xspi->tx_fifo_depth, 0);
> spi_transfer_delay_exec(transfer);
>
> cdns_spi_write(xspi, CDNS_SPI_IER, CDNS_SPI_IXR_DEFAULT);
>--
>2.30.2
On Wed, Aug 09, 2023 at 11:31:24AM +0000, Goud, Srinivas wrote:
> >+ while (ntx || nrx) {
> > /* When xspi in busy condition, bytes may send failed,
> > * then spi control did't work thoroughly, add one byte delay
> > */
> >- if (cdns_spi_read(xspi, CDNS_SPI_ISR) &
> >- CDNS_SPI_IXR_TXFULL)
> >+ if (cdns_spi_read(xspi, CDNS_SPI_ISR) &
> >CDNS_SPI_IXR_TXFULL)
> > udelay(10);
> Linux driver configured as Slave, due to this above delay we see data corruption issue on Master side.
> when Master is continuously reading the data, Slave is failed to prepare the date on time due to above delay.
>
> >@@ -391,11 +388,10 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_id)
> > if (xspi->tx_bytes < xspi->tx_fifo_depth >> 1)
> > cdns_spi_write(xspi, CDNS_SPI_THLD, 1);
> >
> >- cdns_spi_read_rx_fifo(xspi, trans_cnt);
> >-
> > if (xspi->tx_bytes) {
> >- cdns_spi_fill_tx_fifo(xspi, trans_cnt);
> >+ cdns_spi_process_fifo(xspi, trans_cnt, trans_cnt);
> > } else {
> >+ cdns_spi_process_fifo(xspi, 0, trans_cnt);
> When Linux driver configured as Slave, we observed data corruption issue with Slave mode read when data length is 400 bytes.
> As TX empty doesn't guaranties valid data in RX FIFO, hence we added one byte delay(10us) before RX FIFO read to overcome above issue.
> Created patch with above changes, find patch as attachment.
> Can you please test and let me know your observations.
>
Yeah I can test the patch, I am on holiday this week so don't
have access to the hardware, but will do so next week.
> From 40154932ac7486c99e339bbc0b85b3cfe382286c Mon Sep 17 00:00:00 2001
> From: Srinivas Goud <[email protected]>
> Date: Tue, 1 Aug 2023 21:21:09 +0530
> Subject: [PATCH] spi: spi-cadence: Fix data corruption issues in slave mode
>
> Remove 10us delay in cdns_spi_process_fifo() (called from cdns_spi_irq())
> to fix data corruption issue on Master side when this driver
> configured in Slave mode, as Slave is failed to prepare the date
> on time due to above delay.
>
> Add 10us delay before processing the RX FIFO as TX empty doesn't
> guaranty valid data in RX FIFO.
guarantee
>
> Signed-off-by: Srinivas Goud <[email protected]>
> ---
> drivers/spi/spi-cadence.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
> index 42f101d..07a593c 100644
> --- a/drivers/spi/spi-cadence.c
> +++ b/drivers/spi/spi-cadence.c
> @@ -317,12 +317,6 @@ static void cdns_spi_process_fifo(struct cdns_spi *xspi, int ntx, int nrx)
> xspi->rx_bytes -= nrx;
>
> while (ntx || nrx) {
> - /* When xspi in busy condition, bytes may send failed,
> - * then spi control did't work thoroughly, add one byte delay
> - */
> - if (cdns_spi_read(xspi, CDNS_SPI_ISR) & CDNS_SPI_IXR_TXFULL)
> - udelay(10);
> -
> if (ntx) {
> if (xspi->txbuf)
> cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi->txbuf++);
> @@ -392,6 +386,11 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_id)
> if (xspi->tx_bytes) {
> cdns_spi_process_fifo(xspi, trans_cnt, trans_cnt);
> } else {
> + /* Fixed delay due to controller limitation with
> + * RX_NEMPTY incorrect status
> + * Xilinx AR:65885 contains more details
Do you have a web link to this ticket? Would be good to get some
more background.
> + */
> + udelay(10);
> cdns_spi_process_fifo(xspi, 0, trans_cnt);
> cdns_spi_write(xspi, CDNS_SPI_IDR,
> CDNS_SPI_IXR_DEFAULT);
> @@ -439,12 +438,18 @@ static int cdns_transfer_one(struct spi_controller *ctlr,
> cdns_spi_setup_transfer(spi, transfer);
> } else {
> /* Set TX empty threshold to half of FIFO depth
> - * only if TX bytes are more than half FIFO depth.
> + * only if TX bytes are more than FIFO depth.
> */
> if (xspi->tx_bytes > xspi->tx_fifo_depth)
> cdns_spi_write(xspi, CDNS_SPI_THLD, xspi->tx_fifo_depth >> 1);
> }
>
> + /* When xspi in busy condition, bytes may send failed,
> + * then spi control did't work thoroughly, add one byte delay
Just noticing there is an n missing in didn't might as well add
that whilst moving the comment.
> + */
> + if (cdns_spi_read(xspi, CDNS_SPI_ISR) & CDNS_SPI_IXR_TXFULL)
> + udelay(10);
> +
> cdns_spi_process_fifo(xspi, xspi->tx_fifo_depth, 0);
> spi_transfer_delay_exec(transfer);
>
> --
> 2.1.1
Thanks,
Charles