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, August 10, 2023 3:40 PM
>To: Goud, Srinivas <srinivas.goud@xxxxxxx>
>Cc: broonie@xxxxxxxxxx; linux-spi@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; patches@xxxxxxxxxxxxxxxxxxxxx
>Subject: Re: [PATCH v2 1/2] spi: spi-cadence: Interleave write of TX and read of
>RX FIFO
>
>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.
Here AR link which contains the issue description
https://support.xilinx.com/s/article/65885?language=en_US

Thanks,
Srinivas
>
>> +			 */
>> +			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