diff mbox series

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

Message ID 20230509164153.3907694-1-ckeepax@opensource.cirrus.com
State New
Headers
Series [1/2] spi: spi-cadence: Avoid read of RX FIFO before its ready |

Commit Message

Charles Keepax May 9, 2023, 4:41 p.m. UTC 
  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@opensource.cirrus.com>
---
 drivers/spi/spi-cadence.c | 42 ++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 27 deletions(-)

Comments

Mark Brown May 15, 2023, 11:09 a.m. UTC | #1 
On Tue, 09 May 2023 17:41:52 +0100, Charles Keepax wrote:
> 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.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/2] spi: spi-cadence: Avoid read of RX FIFO before its ready
      commit: a84c11e16dc2cc1faad2e688f8c12beeb369d80c
[2/2] spi: spi-cadence: Only overlap FIFO transactions in slave mode
      commit: a0eb7be22c0f934d1fe7e1131f174ef5bc59d3f9

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
Goud, Srinivas May 15, 2023, 12:04 p.m. UTC | #2 
Hi,

>-----Original Message-----
>From: Charles Keepax <ckeepax@opensource.cirrus.com>
>Sent: Tuesday, May 9, 2023 10:12 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 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@opensource.cirrus.com>
>---
> 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
Charles Keepax May 15, 2023, 12:54 p.m. UTC | #3 
On Mon, May 15, 2023 at 12:04:38PM +0000, Goud, Srinivas wrote:
> >-----Original Message-----
> >From: Charles Keepax <ckeepax@opensource.cirrus.com>
> >Sent: Tuesday, May 9, 2023 10:12 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 1/2] spi: spi-cadence: Avoid read of RX FIFO before its ready
> >-		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.

Apologies I am having a little trouble following this are you
saying this part of the patch cases issues for you running in
slave mode?

Thanks,
Charles
Goud, Srinivas May 17, 2023, 5:24 a.m. UTC | #4 
Hi,

>-----Original Message-----
>From: Charles Keepax <ckeepax@opensource.cirrus.com>
>Sent: Monday, May 15, 2023 6:25 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 1/2] spi: spi-cadence: Avoid read of RX FIFO before its
>ready
>
>On Mon, May 15, 2023 at 12:04:38PM +0000, Goud, Srinivas wrote:
>> >-----Original Message-----
>> >From: Charles Keepax <ckeepax@opensource.cirrus.com>
>> >Sent: Tuesday, May 9, 2023 10:12 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 1/2] spi: spi-cadence: Avoid read of RX FIFO before its ready
>> >-		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.
>
>Apologies I am having a little trouble following this are you saying this part of
>the patch cases issues for you running in slave mode?
Yes, we see issue with this patch when we run in Slave mode.

When any master is in continuous read mode (anything > FIFO depth), 
with updated logic cdns_spi_read_rx_fifo() function in cdns_spi_irq
continuously in read loop to read complete half FIFO data.
due to this Slave failed to write the TX FIFO on time and result in
incorrect data in Master receive. 
Whereas in my previous patch, data read and write happening byte wise,
by which we are making sure data availability in TXFIFO on time.
>
>Thanks,
>Charles

Thanks,
Srinivas
Charles Keepax May 17, 2023, 10:46 a.m. UTC | #5 
On Wed, May 17, 2023 at 05:24:10AM +0000, Goud, Srinivas wrote:
> >On Mon, May 15, 2023 at 12:04:38PM +0000, Goud, Srinivas wrote:
> >> 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.
> >
> >Apologies I am having a little trouble following this are you saying this part of
> >the patch cases issues for you running in slave mode?
> Yes, we see issue with this patch when we run in Slave mode.
> 
> When any master is in continuous read mode (anything > FIFO depth), 
> with updated logic cdns_spi_read_rx_fifo() function in cdns_spi_irq
> continuously in read loop to read complete half FIFO data.
> due to this Slave failed to write the TX FIFO on time and result in
> incorrect data in Master receive. 
> Whereas in my previous patch, data read and write happening byte wise,
> by which we are making sure data availability in TXFIFO on time.

That is a very tight system if reading 64 sequential memory locations
is the timing difference between success and failure, Linux is
not a real-time OS.

But I don't really mind moving back to a byte-wise operation. Although
we need to avoid the issues introduced by the first attempt at that. I
will have a look at doing a patch to put the byte-wise back in.

Thanks,
Charles
diff mbox series

Patch

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);
+
+		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);