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 <[email protected]>
---
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(-)
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);
--
2.30.2
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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Thu, May 18, 2023 at 09:55:11AM +0800, kernel test robot wrote:
> 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
oops... yeah kernel doc should have been updated as well. I will
respin, functionally the patch should be fine.
Thanks,
Charles