diff mbox series

spi: spi-cadence: Interleave write of TX and read of RX FIFO

Message ID 20230517163157.639974-1-ckeepax@opensource.cirrus.com
State Accepted
Commit 6afe2ae8dc48e643cb9f52e86494b96942440bc6
Headers show
Series spi: spi-cadence: Interleave write of TX and read of RX FIFO | expand

Commit Message

Charles Keepax May 17, 2023, 4:31 p.m. UTC 
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>
---

This patch puts the interleaving back it tests out fine even on
longer transfers on my master setup. We should probably wait for a
tested-by from Srinivas before we merge it. I can't test slave mode,
and it sounds like the timing is exceedingly tight on his system.

Thanks,
Charles

 drivers/spi/spi-cadence.c | 60 ++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 33 deletions(-)

Comments

kernel test robot May 18, 2023, 1:55 a.m. UTC | #1 
Hi Charles,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-spi/for-next]
[also build test WARNING on next-20230517]
[cannot apply to linus/master v6.4-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Charles-Keepax/spi-spi-cadence-Interleave-write-of-TX-and-read-of-RX-FIFO/20230518-005845
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
patch link:    https://lore.kernel.org/r/20230517163157.639974-1-ckeepax%40opensource.cirrus.com
patch subject: [PATCH] spi: spi-cadence: Interleave write of TX and read of RX FIFO
config: x86_64-randconfig-a013
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/46aae5c89e1222b9b10ecd164089d05fbeda2dca
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Charles-Keepax/spi-spi-cadence-Interleave-write-of-TX-and-read-of-RX-FIFO/20230518-005845
        git checkout 46aae5c89e1222b9b10ecd164089d05fbeda2dca
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/spi/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305180824.qIPSXIQQ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/spi/spi-cadence.c:127: warning: Function parameter or member 'clk_rate' not described in 'cdns_spi'
   drivers/spi/spi-cadence.c:309: warning: Function parameter or member 'ntx' not described in 'cdns_spi_process_fifo'
   drivers/spi/spi-cadence.c:309: warning: Function parameter or member 'nrx' not described in 'cdns_spi_process_fifo'
>> drivers/spi/spi-cadence.c:309: warning: expecting prototype for cdns_spi_fill_tx_fifo(). Prototype was for cdns_spi_process_fifo() instead


vim +309 drivers/spi/spi-cadence.c

c474b38665463d Harini Katakam     2014-04-14  303  
c474b38665463d Harini Katakam     2014-04-14  304  /**
c474b38665463d Harini Katakam     2014-04-14  305   * cdns_spi_fill_tx_fifo - Fills the TX FIFO with as many bytes as possible
c474b38665463d Harini Katakam     2014-04-14  306   * @xspi:	Pointer to the cdns_spi structure
c474b38665463d Harini Katakam     2014-04-14  307   */
46aae5c89e1222 Charles Keepax     2023-05-17  308  static void cdns_spi_process_fifo(struct cdns_spi *xspi, int ntx, int nrx)
c474b38665463d Harini Katakam     2014-04-14 @309  {
46aae5c89e1222 Charles Keepax     2023-05-17  310  	ntx = clamp(ntx, 0, xspi->tx_bytes);
46aae5c89e1222 Charles Keepax     2023-05-17  311  	nrx = clamp(nrx, 0, xspi->rx_bytes);
c474b38665463d Harini Katakam     2014-04-14  312  
46aae5c89e1222 Charles Keepax     2023-05-17  313  	xspi->tx_bytes -= ntx;
46aae5c89e1222 Charles Keepax     2023-05-17  314  	xspi->rx_bytes -= nrx;
46aae5c89e1222 Charles Keepax     2023-05-17  315  
46aae5c89e1222 Charles Keepax     2023-05-17  316  	while (ntx || nrx) {
49530e6411789c sxauwsk            2018-04-17  317  		/* When xspi in busy condition, bytes may send failed,
49530e6411789c sxauwsk            2018-04-17  318  		 * then spi control did't work thoroughly, add one byte delay
49530e6411789c sxauwsk            2018-04-17  319  		 */
46aae5c89e1222 Charles Keepax     2023-05-17  320  		if (cdns_spi_read(xspi, CDNS_SPI_ISR) & CDNS_SPI_IXR_TXFULL)
931c4e9a72ae91 Janek Kotas        2018-06-04  321  			udelay(10);
49530e6411789c sxauwsk            2018-04-17  322  
46aae5c89e1222 Charles Keepax     2023-05-17  323  		if (ntx) {
c474b38665463d Harini Katakam     2014-04-14  324  			if (xspi->txbuf)
24746675fbc8dc Shubhrajyoti Datta 2016-04-05  325  				cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi->txbuf++);
c474b38665463d Harini Katakam     2014-04-14  326  			else
24746675fbc8dc Shubhrajyoti Datta 2016-04-05  327  				cdns_spi_write(xspi, CDNS_SPI_TXD, 0);
c474b38665463d Harini Katakam     2014-04-14  328  
46aae5c89e1222 Charles Keepax     2023-05-17  329  			ntx--;
c474b38665463d Harini Katakam     2014-04-14  330  		}
c474b38665463d Harini Katakam     2014-04-14  331  
46aae5c89e1222 Charles Keepax     2023-05-17  332  		if (nrx) {
46aae5c89e1222 Charles Keepax     2023-05-17  333  			u8 data = cdns_spi_read(xspi, CDNS_SPI_RXD);
b1b90514eaa345 Srinivas Goud      2023-04-18  334  
b1b90514eaa345 Srinivas Goud      2023-04-18  335  			if (xspi->rxbuf)
b1b90514eaa345 Srinivas Goud      2023-04-18  336  				*xspi->rxbuf++ = data;
46aae5c89e1222 Charles Keepax     2023-05-17  337  
46aae5c89e1222 Charles Keepax     2023-05-17  338  			nrx--;
46aae5c89e1222 Charles Keepax     2023-05-17  339  		}
b1b90514eaa345 Srinivas Goud      2023-04-18  340  	}
b1b90514eaa345 Srinivas Goud      2023-04-18  341  }
b1b90514eaa345 Srinivas Goud      2023-04-18  342
diff mbox series

Patch

diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
index ff02d81041319..08136bbb34030 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>
@@ -304,44 +305,38 @@  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, 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 +386,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 +442,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);