RE: [PATCH 1/2] spi: spi-cadence: Avoid read of RX FIFO before its ready

[Goud, Srinivas]

Hi,

>-----Original Message-----
>From: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
>Sent: Tuesday, May 9, 2023 10:12 PM
>To: broonie@xxxxxxxxxx
>Cc: Goud, Srinivas <srinivas.goud@xxxxxxx>; linux-spi@xxxxxxxxxxxxxxx;
>linux-kernel@xxxxxxxxxxxxxxx; patches@xxxxxxxxxxxxxxxxxxxxx
>Subject: [PATCH 1/2] spi: spi-cadence: Avoid read of RX FIFO before its ready
>
>Recent changes to cdns_spi_irq introduced some issues.
>
>Firstly, when writing the end of a longer transaction, the code in cdns_spi_irq
>will write data into the TX FIFO, then immediately fall into the if (!xspi-
>>tx_bytes) path and attempt to read data from the RX FIFO. However this
>required waiting for the TX FIFO to empty before the RX data was ready.
>
>Secondly, the variable trans_cnt is now rather inaccurately named since in
>cases, where the watermark is set to 1, trans_cnt will be
>1 but the count of bytes transferred would be much longer.
>
>Finally, when setting up the transaction we set the watermark to 50% of the
>FIFO if the transaction is great than 50% of the FIFO. However, there is no need
>to split a tranaction that is smaller than the whole FIFO, so anything up to the
>FIFO size can be done in a single transaction.
>
>Tidy up the code a little, to avoid repeatedly calling cdns_spi_read_rx_fifo with
>a count of 1, and correct the three issues noted above.
>
>Fixes: b1b90514eaa3 ("spi: spi-cadence: Add support for Slave mode")
>Signed-off-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
>---
> drivers/spi/spi-cadence.c | 42 ++++++++++++++-------------------------
> 1 file changed, 15 insertions(+), 27 deletions(-)
>
>diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c index
>ac85d55622127..b0ccb138e3566 100644
>--- a/drivers/spi/spi-cadence.c
>+++ b/drivers/spi/spi-cadence.c
>@@ -304,13 +304,11 @@ 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
>  * @xspi:	Pointer to the cdns_spi structure
>  */
>-static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi)
>+static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi, unsigned int
>+avail)
> {
> 	unsigned long trans_cnt = 0;
>
>-	while ((trans_cnt < xspi->tx_fifo_depth) &&
>-	       (xspi->tx_bytes > 0)) {
>-
>+	while ((trans_cnt < avail) && (xspi->tx_bytes > 0)) {
> 		/* When xspi in busy condition, bytes may send failed,
> 		 * then spi control did't work thoroughly, add one byte delay
> 		 */
>@@ -381,33 +379,23 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_id)
> 		spi_finalize_current_transfer(ctlr);
> 		status = IRQ_HANDLED;
> 	} else if (intr_status & CDNS_SPI_IXR_TXOW) {
>-		int trans_cnt = cdns_spi_read(xspi, CDNS_SPI_THLD);
>+		int threshold = cdns_spi_read(xspi, CDNS_SPI_THLD);
>+		int trans_cnt = xspi->rx_bytes - xspi->tx_bytes;
>+
>+		if (threshold > 1)
>+			trans_cnt -= threshold;
>+
> 		/* Set threshold to one if number of pending are
> 		 * less than half fifo
> 		 */
> 		if (xspi->tx_bytes < xspi->tx_fifo_depth >> 1)
> 			cdns_spi_write(xspi, CDNS_SPI_THLD, 1);
>
>-		while (trans_cnt) {
>-			cdns_spi_read_rx_fifo(xspi, 1);
>-
>-			if (xspi->tx_bytes) {
>-				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--;
>-		}
>-		if (!xspi->tx_bytes) {
>-			/* Fixed delay due to controller limitation with
>-			 * RX_NEMPTY incorrect status
>-			 * Xilinx AR:65885 contains more details
>-			 */
>-			udelay(10);
>-			cdns_spi_read_rx_fifo(xspi, xspi->rx_bytes);
>+		cdns_spi_read_rx_fifo(xspi, trans_cnt);
Cadence SPI configured in Slave mode,  when threshold is half of FIFO depth cdns_spi_read_rx_fifo() function continuously in read mode, 
due to this we see incorrect data received on the Master side as Slave is failed to update the TX FIFO on time.

>+
>+		if (xspi->tx_bytes) {
>+			cdns_spi_fill_tx_fifo(xspi, trans_cnt);
>+		} else {
> 			cdns_spi_write(xspi, CDNS_SPI_IDR,
> 				       CDNS_SPI_IXR_DEFAULT);
> 			spi_finalize_current_transfer(ctlr);
>@@ -456,10 +444,10 @@ static int cdns_transfer_one(struct spi_controller
>*ctlr,
> 	/* Set TX empty threshold to half of FIFO depth
> 	 * only if TX bytes are more than half FIFO depth.
> 	 */
>-	if (xspi->tx_bytes > (xspi->tx_fifo_depth >> 1))
>+	if (xspi->tx_bytes > xspi->tx_fifo_depth)
> 		cdns_spi_write(xspi, CDNS_SPI_THLD, xspi->tx_fifo_depth >>
>1);
>
>-	cdns_spi_fill_tx_fifo(xspi);
>+	cdns_spi_fill_tx_fifo(xspi, xspi->tx_fifo_depth);
> 	spi_transfer_delay_exec(transfer);
>
> 	cdns_spi_write(xspi, CDNS_SPI_IER, CDNS_SPI_IXR_DEFAULT);
>--
>2.30.2

Regards
Srinivas