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

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

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 <srinivas.goud@xxxxxxx>
> 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 <srinivas.goud@xxxxxxx>
> ---
>  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