RE: [PATCH v2 1/2] spi: spi-cadence: Interleave write of TX and read of RX FIFO

[Goud, Srinivas]

Hi Charles,

>-----Original Message-----
>From: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
>Sent: Thursday, May 18, 2023 3:09 PM
>To: broonie@xxxxxxxxxx
>Cc: Goud, Srinivas <srinivas.goud@xxxxxxx>; linux-spi@xxxxxxxxxxxxxxx;
>linux-kernel@xxxxxxxxxxxxxxx; patches@xxxxxxxxxxxxxxxxxxxxx
>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 <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
>---
>
>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

Attachment: 0001-spi-spi-cadence-Fix-data-corruption-issues-in-slave-.patch
Description: 0001-spi-spi-cadence-Fix-data-corruption-issues-in-slave-.patch