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

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

[Charles Keepax]

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@opensource.cirrus.com>
---

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

[PATCH 2/2] spi: spi-cadence: Add missing kernel doc for clk_rate in cdns_spi

[Charles Keepax]

Add the missing kernel documentation to silence the build warning.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

New since v1 of the series, but might as well fix this build warning
too.

Thanks,
Charles

 drivers/spi/spi-cadence.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
index 26e6633693196..de8fe3c5becbb 100644
--- a/drivers/spi/spi-cadence.c
+++ b/drivers/spi/spi-cadence.c
@@ -102,6 +102,7 @@
  * @regs:		Virtual address of the SPI controller registers
  * @ref_clk:		Pointer to the peripheral clock
  * @pclk:		Pointer to the APB clock
+ * @clk_rate:		Reference clock frequency, taken from @ref_clk
  * @speed_hz:		Current SPI bus clock speed in Hz
  * @txbuf:		Pointer	to the TX buffer
  * @rxbuf:		Pointer to the RX buffer
-- 
2.30.2

Re: [PATCH 2/2] spi: spi-cadence: Add missing kernel doc for clk_rate in cdns_spi

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

On Thu, May 18, 2023 at 10:39:27AM +0100, Charles Keepax wrote:
> Add the missing kernel documentation to silence the build warning.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> 
> New since v1 of the series, but might as well fix this build warning
> too.

Sending this without the "v2" breaks threading and makes it hard to
handle with tooling, versioning applies to the patch series, not to
individual patches.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] spi: spi-cadence: Add missing kernel doc for clk_rate in cdns_spi

[Charles Keepax]

On Mon, May 22, 2023 at 10:52:17AM +0100, Mark Brown wrote:
> On Thu, May 18, 2023 at 10:39:27AM +0100, Charles Keepax wrote:
> > Add the missing kernel documentation to silence the build warning.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > ---
> > 
> > New since v1 of the series, but might as well fix this build warning
> > too.
> 
> Sending this without the "v2" breaks threading and makes it hard to
> handle with tooling, versioning applies to the patch series, not to
> individual patches.

Apologies do you want me to resend?

Thanks,
Charles

Re: [PATCH 2/2] spi: spi-cadence: Add missing kernel doc for clk_rate in cdns_spi

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]

On Mon, May 22, 2023 at 02:21:06PM +0000, Charles Keepax wrote:
> On Mon, May 22, 2023 at 10:52:17AM +0100, Mark Brown wrote:

> > Sending this without the "v2" breaks threading and makes it hard to
> > handle with tooling, versioning applies to the patch series, not to
> > individual patches.

> Apologies do you want me to resend?

Just this patch please, the first patch is still going through CI but I
didn't figure out how to push this into the tooling.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

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

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

[Goud, Srinivas]

[-- Attachment #1: Type: text/plain, Size: 5241 bytes --]

Hi Charles,

>-----Original Message-----
>From: Charles Keepax <ckeepax@opensource.cirrus.com>
>Sent: Thursday, May 18, 2023 3:09 PM
>To: broonie@kernel.org
>Cc: Goud, Srinivas <srinivas.goud@amd.com>; linux-spi@vger.kernel.org;
>linux-kernel@vger.kernel.org; patches@opensource.cirrus.com
>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@opensource.cirrus.com>
>---
>
>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 #2: 0001-spi-spi-cadence-Fix-data-corruption-issues-in-slave-.patch --]
[-- Type: application/octet-stream, Size: 2497 bytes --]

From 40154932ac7486c99e339bbc0b85b3cfe382286c Mon Sep 17 00:00:00 2001
From: Srinivas Goud <srinivas.goud@amd.com>
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.

Signed-off-by: Srinivas Goud <srinivas.goud@amd.com>
---
 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
+			 */
+			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
+	 */
+	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

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

[Charles Keepax]

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@amd.com>
> 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@amd.com>
> ---
>  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

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@opensource.cirrus.com>
>Sent: Thursday, August 10, 2023 3:40 PM
>To: Goud, Srinivas <srinivas.goud@amd.com>
>Cc: broonie@kernel.org; linux-spi@vger.kernel.org; linux-
>kernel@vger.kernel.org; patches@opensource.cirrus.com
>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@amd.com>
>> 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@amd.com>
>> ---
>>  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

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

[Charles Keepax]

On Mon, Aug 21, 2023 at 07:26:57AM +0000, Goud, Srinivas wrote:
> >
> >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

Apologies for the delay, the patch tests out fine for me. You
probably want to send it properly as a patch rather than just an
attachment. But when you do feel free to add my:

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Tested-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles