[PATCH v3 0/6] spi: spi-fsl-dspi: Target mode improvements

[PATCH v3 0/6] spi: spi-fsl-dspi: Target mode improvements

[James Clark]

Improve usability of target mode by reporting FIFO errors and increasing
the buffer size when DMA is used. While we're touching DMA stuff also
switch to non-coherent memory, although this is unrelated to target
mode.

The first commit is marked as a fix because it can fix intermittent
issues with existing transfers, rather than the later fixes which
improve larger than FIFO target mode transfers which would have never
worked.

With the combination of the commit to increase the DMA buffer size and
the commit to use non-coherent memory, the host mode performance figures
are as follows on S32G3:

  # spidev_test --device /dev/spidev1.0 --bpw 8 --size <test_size> --cpha --iter 10000000 --speed 10000000

  Coherent (4096 byte transfers): 6534 kbps
  Non-coherent:                   7347 kbps

  Coherent (16 byte transfers):    447 kbps
  Non-coherent:                    448 kbps

Just for comparison running the same test in XSPI mode:

  4096 byte transfers:            2143 kbps
  16 byte transfers:               637 kbps

These tests required hacking S32G3 to use DMA in host mode, although
the figures should be representative of target mode too where DMA is
used. And the other devices that use DMA in host mode should see similar
improvements.

Signed-off-by: James Clark <james.clark@linaro.org>
---
Changes in v3:
- Stub out DMA functions in the driver so no-DMA builds work
- Link to v2: https://lore.kernel.org/r/20250613-james-nxp-spi-dma-v2-0-017eecf24aab@linaro.org

Changes in v2:
- Store status in cur_msg->status rather than adding xfer_status
- Show exact underflow/overflow flags in error message
- Rate limit error messages
- Add a comment about resetting the completion counter prior to transfer
- Rename dspi_is_fifo_overflow() -> dspi_fifo_error()
- Add performance figures to cover letter
- Rebase onto https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/for-next
  to avoid some conflicts
- Link to v1: https://lore.kernel.org/r/20250609-james-nxp-spi-dma-v1-0-2b831e714be2@linaro.org

---
James Clark (5):
      spi: spi-fsl-dspi: Clear completion counter before initiating transfer
      spi: spi-fsl-dspi: Store status directly in cur_msg->status
      spi: spi-fsl-dspi: Stub out DMA functions
      spi: spi-fsl-dspi: Use non-coherent memory for DMA
      spi: spi-fsl-dspi: Report FIFO overflows as errors

Larisa Grigore (1):
      spi: spi-fsl-dspi: Increase DMA buffer size

 drivers/spi/spi-fsl-dspi.c | 221 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 160 insertions(+), 61 deletions(-)
---
base-commit: 4f326fa6236787ca516ea6eab8e5e9dc5c236f03
change-id: 20250522-james-nxp-spi-dma-a997ebebfb6b

Best regards,
-- 
James Clark <james.clark@linaro.org>

[PATCH v3 1/6] spi: spi-fsl-dspi: Clear completion counter before initiating transfer

[James Clark]

In target mode, extra interrupts can be received between the end of a
transfer and halting the module if the host continues sending more data.
If the interrupt from this occurs after the reinit_completion() then the
completion counter is left at a non-zero value. The next unrelated
transfer initiated by userspace will then complete immediately without
waiting for the interrupt or writing to the RX buffer.

Fix it by resetting the counter before the transfer so that lingering
values are cleared. This is done after clearing the FIFOs and the
status register but before the transfer is initiated, so no interrupts
should be received at this point resulting in other race conditions.

Fixes: 4f5ee75ea171 ("spi: spi-fsl-dspi: Replace interruptible wait queue with a simple completion")
Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-dspi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 04c88d090c4d..744dfc561db2 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -1122,11 +1122,19 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 		if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
 			status = dspi_dma_xfer(dspi);
 		} else {
+			/*
+			 * Reset completion counter to clear any extra
+			 * complete()s from spurious interrupts that may have
+			 * happened after the last message's completion but
+			 * before the module was fully in stop mode.
+			 */
+			if (dspi->irq)
+				reinit_completion(&dspi->xfer_done);
+
 			dspi_fifo_write(dspi);
 
 			if (dspi->irq) {
 				wait_for_completion(&dspi->xfer_done);
-				reinit_completion(&dspi->xfer_done);
 			} else {
 				do {
 					status = dspi_poll(dspi);

-- 
2.34.1

[PATCH v3 2/6] spi: spi-fsl-dspi: Store status directly in cur_msg->status

[James Clark]

This will allow us to return a status from the interrupt handler in a
later commit and avoids copying it at the end of
dspi_transfer_one_message(). For consistency make polling and DMA modes
use the same mechanism.

Refactor dspi_rxtx() and dspi_poll() to not return -EINPROGRESS because
this isn't actually a status that was ever returned to the core layer
but some internal state. Wherever that was used we can look at dspi->len
instead.

No functional changes intended.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-dspi.c | 68 ++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 33 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 744dfc561db2..feb29bb92a77 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -591,11 +591,10 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 
 static void dspi_setup_accel(struct fsl_dspi *dspi);
 
-static int dspi_dma_xfer(struct fsl_dspi *dspi)
+static void dspi_dma_xfer(struct fsl_dspi *dspi)
 {
 	struct spi_message *message = dspi->cur_msg;
 	struct device *dev = &dspi->pdev->dev;
-	int ret = 0;
 
 	/*
 	 * dspi->len gets decremented by dspi_pop_tx_pushr in
@@ -612,14 +611,12 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
 		message->actual_length += dspi->words_in_flight *
 					  dspi->oper_word_size;
 
-		ret = dspi_next_xfer_dma_submit(dspi);
-		if (ret) {
+		message->status = dspi_next_xfer_dma_submit(dspi);
+		if (message->status) {
 			dev_err(dev, "DMA transfer failed\n");
 			break;
 		}
 	}
-
-	return ret;
 }
 
 static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
@@ -986,36 +983,40 @@ static void dspi_fifo_write(struct fsl_dspi *dspi)
 				dspi->progress, !dspi->irq);
 }
 
-static int dspi_rxtx(struct fsl_dspi *dspi)
+static void dspi_rxtx(struct fsl_dspi *dspi)
 {
 	dspi_fifo_read(dspi);
 
 	if (!dspi->len)
 		/* Success! */
-		return 0;
+		return;
 
 	dspi_fifo_write(dspi);
-
-	return -EINPROGRESS;
 }
 
-static int dspi_poll(struct fsl_dspi *dspi)
+static void dspi_poll(struct fsl_dspi *dspi)
 {
 	int tries = 1000;
 	u32 spi_sr;
 
-	do {
-		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
-		regmap_write(dspi->regmap, SPI_SR, spi_sr);
+	while (dspi->len) {
+		do {
+			regmap_read(dspi->regmap, SPI_SR, &spi_sr);
+			regmap_write(dspi->regmap, SPI_SR, spi_sr);
 
-		if (spi_sr & SPI_SR_CMDTCF)
-			break;
-	} while (--tries);
+			if (spi_sr & SPI_SR_CMDTCF)
+				break;
+		} while (--tries);
 
-	if (!tries)
-		return -ETIMEDOUT;
+		if (!tries) {
+			dspi->cur_msg->status = -ETIMEDOUT;
+			return;
+		}
 
-	return dspi_rxtx(dspi);
+		dspi_rxtx(dspi);
+	}
+
+	dspi->cur_msg->status = 0;
 }
 
 static irqreturn_t dspi_interrupt(int irq, void *dev_id)
@@ -1029,8 +1030,13 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
 	if (!(spi_sr & SPI_SR_CMDTCF))
 		return IRQ_NONE;
 
-	if (dspi_rxtx(dspi) == 0)
+	dspi_rxtx(dspi);
+
+	if (!dspi->len) {
+		if (dspi->cur_msg)
+			WRITE_ONCE(dspi->cur_msg->status, 0);
 		complete(&dspi->xfer_done);
+	}
 
 	return IRQ_HANDLED;
 }
@@ -1060,7 +1066,6 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 	struct spi_device *spi = message->spi;
 	struct spi_transfer *transfer;
 	bool cs = false;
-	int status = 0;
 	u32 val = 0;
 	bool cs_change = false;
 
@@ -1120,7 +1125,7 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 				       dspi->progress, !dspi->irq);
 
 		if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
-			status = dspi_dma_xfer(dspi);
+			dspi_dma_xfer(dspi);
 		} else {
 			/*
 			 * Reset completion counter to clear any extra
@@ -1133,15 +1138,12 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 
 			dspi_fifo_write(dspi);
 
-			if (dspi->irq) {
+			if (dspi->irq)
 				wait_for_completion(&dspi->xfer_done);
-			} else {
-				do {
-					status = dspi_poll(dspi);
-				} while (status == -EINPROGRESS);
-			}
+			else
+				dspi_poll(dspi);
 		}
-		if (status)
+		if (READ_ONCE(message->status))
 			break;
 
 		spi_transfer_delay_exec(transfer);
@@ -1150,7 +1152,8 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 			dspi_deassert_cs(spi, &cs);
 	}
 
-	if (status || !cs_change) {
+	dspi->cur_msg = NULL;
+	if (message->status || !cs_change) {
 		/* Put DSPI in stop mode */
 		regmap_update_bits(dspi->regmap, SPI_MCR,
 				   SPI_MCR_HALT, SPI_MCR_HALT);
@@ -1159,10 +1162,9 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 			;
 	}
 
-	message->status = status;
 	spi_finalize_current_message(ctlr);
 
-	return status;
+	return message->status;
 }
 
 static int dspi_set_mtf(struct fsl_dspi *dspi)

-- 
2.34.1

[PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions

[James Clark]

This will allow the build to succeed with !CONFIG_HAS_DMA, either due to
a randconfig build test or when the target only uses one of the non-DMA
transfer modes which this driver supports.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-dspi.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index feb29bb92a77..8212c4193536 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -373,6 +373,8 @@ struct fsl_dspi {
 	void (*dev_to_host)(struct fsl_dspi *dspi, u32 rxdata);
 };
 
+static void dspi_setup_accel(struct fsl_dspi *dspi);
+
 static bool is_s32g_dspi(struct fsl_dspi *data)
 {
 	return data->devtype_data == &devtype_data[S32G] ||
@@ -489,6 +491,7 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
 	dspi->dev_to_host(dspi, rxdata);
 }
 
+#if IS_ENABLED(CONFIG_HAS_DMA)
 static void dspi_tx_dma_callback(void *arg)
 {
 	struct fsl_dspi *dspi = arg;
@@ -589,8 +592,6 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 	return 0;
 }
 
-static void dspi_setup_accel(struct fsl_dspi *dspi);
-
 static void dspi_dma_xfer(struct fsl_dspi *dspi)
 {
 	struct spi_message *message = dspi->cur_msg;
@@ -722,6 +723,18 @@ static void dspi_release_dma(struct fsl_dspi *dspi)
 		dma_release_channel(dma->chan_rx);
 	}
 }
+#else
+static void dspi_dma_xfer(struct fsl_dspi *dspi)
+{
+	sdpi->cur_msg->status = -EINVAL;
+}
+static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
+{
+	dev_err(&dspi->pdev->dev, "DMA support not enabled in kernel\n");
+	return -EINVAL;
+}
+static void dspi_release_dma(struct fsl_dspi *dspi) {}
+#endif
 
 static void hz_to_spi_baud(char *pbr, char *br, int speed_hz,
 			   unsigned long clkrate, bool mtf_enabled)

-- 
2.34.1

[PATCH v3 4/6] spi: spi-fsl-dspi: Use non-coherent memory for DMA

[James Clark]

Using coherent memory here isn't functionally necessary. Because the
change to use non-coherent memory isn't overly complex and only a few
synchronization points are required, we might as well do it while fixing
up some other DMA issues.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-dspi.c | 56 +++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 8212c4193536..172eb9929de1 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -492,11 +492,20 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
 }
 
 #if IS_ENABLED(CONFIG_HAS_DMA)
+
+static int dspi_dma_transfer_size(struct fsl_dspi *dspi)
+{
+	return dspi->words_in_flight * DMA_SLAVE_BUSWIDTH_4_BYTES;
+}
+
 static void dspi_tx_dma_callback(void *arg)
 {
 	struct fsl_dspi *dspi = arg;
 	struct fsl_dspi_dma *dma = dspi->dma;
+	struct device *dev = &dspi->pdev->dev;
 
+	dma_sync_single_for_cpu(dev, dma->tx_dma_phys,
+				dspi_dma_transfer_size(dspi), DMA_TO_DEVICE);
 	complete(&dma->cmd_tx_complete);
 }
 
@@ -504,9 +513,13 @@ static void dspi_rx_dma_callback(void *arg)
 {
 	struct fsl_dspi *dspi = arg;
 	struct fsl_dspi_dma *dma = dspi->dma;
+	struct device *dev = &dspi->pdev->dev;
 	int i;
 
 	if (dspi->rx) {
+		dma_sync_single_for_cpu(dev, dma->rx_dma_phys,
+					dspi_dma_transfer_size(dspi),
+					DMA_FROM_DEVICE);
 		for (i = 0; i < dspi->words_in_flight; i++)
 			dspi_push_rx(dspi, dspi->dma->rx_dma_buf[i]);
 	}
@@ -516,6 +529,7 @@ static void dspi_rx_dma_callback(void *arg)
 
 static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 {
+	size_t size = dspi_dma_transfer_size(dspi);
 	struct device *dev = &dspi->pdev->dev;
 	struct fsl_dspi_dma *dma = dspi->dma;
 	int time_left;
@@ -524,10 +538,9 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 	for (i = 0; i < dspi->words_in_flight; i++)
 		dspi->dma->tx_dma_buf[i] = dspi_pop_tx_pushr(dspi);
 
+	dma_sync_single_for_device(dev, dma->tx_dma_phys, size, DMA_TO_DEVICE);
 	dma->tx_desc = dmaengine_prep_slave_single(dma->chan_tx,
-					dma->tx_dma_phys,
-					dspi->words_in_flight *
-					DMA_SLAVE_BUSWIDTH_4_BYTES,
+					dma->tx_dma_phys, size,
 					DMA_MEM_TO_DEV,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 	if (!dma->tx_desc) {
@@ -542,10 +555,10 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 		return -EINVAL;
 	}
 
+	dma_sync_single_for_device(dev, dma->rx_dma_phys, size,
+				   DMA_FROM_DEVICE);
 	dma->rx_desc = dmaengine_prep_slave_single(dma->chan_rx,
-					dma->rx_dma_phys,
-					dspi->words_in_flight *
-					DMA_SLAVE_BUSWIDTH_4_BYTES,
+					dma->rx_dma_phys, size,
 					DMA_DEV_TO_MEM,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 	if (!dma->rx_desc) {
@@ -642,17 +655,17 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
 		goto err_tx_channel;
 	}
 
-	dma->tx_dma_buf = dma_alloc_coherent(dma->chan_tx->device->dev,
-					     dma_bufsize, &dma->tx_dma_phys,
-					     GFP_KERNEL);
+	dma->tx_dma_buf = dma_alloc_noncoherent(dma->chan_tx->device->dev,
+						dma_bufsize, &dma->tx_dma_phys,
+						DMA_TO_DEVICE, GFP_KERNEL);
 	if (!dma->tx_dma_buf) {
 		ret = -ENOMEM;
 		goto err_tx_dma_buf;
 	}
 
-	dma->rx_dma_buf = dma_alloc_coherent(dma->chan_rx->device->dev,
-					     dma_bufsize, &dma->rx_dma_phys,
-					     GFP_KERNEL);
+	dma->rx_dma_buf = dma_alloc_noncoherent(dma->chan_rx->device->dev,
+						dma_bufsize, &dma->rx_dma_phys,
+						DMA_FROM_DEVICE, GFP_KERNEL);
 	if (!dma->rx_dma_buf) {
 		ret = -ENOMEM;
 		goto err_rx_dma_buf;
@@ -687,11 +700,12 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
 	return 0;
 
 err_slave_config:
-	dma_free_coherent(dma->chan_rx->device->dev,
-			  dma_bufsize, dma->rx_dma_buf, dma->rx_dma_phys);
+	dma_free_noncoherent(dma->chan_rx->device->dev, dma_bufsize,
+			     dma->rx_dma_buf, dma->rx_dma_phys,
+			     DMA_FROM_DEVICE);
 err_rx_dma_buf:
-	dma_free_coherent(dma->chan_tx->device->dev,
-			  dma_bufsize, dma->tx_dma_buf, dma->tx_dma_phys);
+	dma_free_noncoherent(dma->chan_tx->device->dev, dma_bufsize,
+			     dma->tx_dma_buf, dma->tx_dma_phys, DMA_TO_DEVICE);
 err_tx_dma_buf:
 	dma_release_channel(dma->chan_tx);
 err_tx_channel:
@@ -712,14 +726,16 @@ static void dspi_release_dma(struct fsl_dspi *dspi)
 		return;
 
 	if (dma->chan_tx) {
-		dma_free_coherent(dma->chan_tx->device->dev, dma_bufsize,
-				  dma->tx_dma_buf, dma->tx_dma_phys);
+		dma_free_noncoherent(dma->chan_tx->device->dev, dma_bufsize,
+				     dma->tx_dma_buf, dma->tx_dma_phys,
+				     DMA_TO_DEVICE);
 		dma_release_channel(dma->chan_tx);
 	}
 
 	if (dma->chan_rx) {
-		dma_free_coherent(dma->chan_rx->device->dev, dma_bufsize,
-				  dma->rx_dma_buf, dma->rx_dma_phys);
+		dma_free_noncoherent(dma->chan_rx->device->dev, dma_bufsize,
+				     dma->rx_dma_buf, dma->rx_dma_phys,
+				     DMA_FROM_DEVICE);
 		dma_release_channel(dma->chan_rx);
 	}
 }

-- 
2.34.1

[PATCH v3 5/6] spi: spi-fsl-dspi: Increase DMA buffer size

[James Clark]

From: Larisa Grigore <larisa.grigore@nxp.com>

When the device is configured as a target, the host won't stop sending
data while we're draining the buffer which leads to FIFO underflows
and corruption.

Increase the DMA buffer size to the maximum words that edma can
transfer once to reduce the chance of this happening.

While we're here, also change the buffer size for host mode back to a
page as it was before commit a957499bd437 ("spi: spi-fsl-dspi: Fix
bits-per-word acceleration in DMA mode"). dma_alloc_noncoherent()
allocations are backed by a full page anyway, so we might as well use it
all.

Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-dspi.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 172eb9929de1..58881911e74a 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -493,6 +493,39 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
 
 #if IS_ENABLED(CONFIG_HAS_DMA)
 
+static int dspi_dma_bufsize(struct fsl_dspi *dspi)
+{
+	if (spi_controller_is_target(dspi->ctlr)) {
+		/*
+		 * In target mode we have to be ready to receive the maximum
+		 * that can possibly be transferred at once by EDMA without any
+		 * FIFO underflows. This is CITER * SSIZE, where SSIZE is a max
+		 * of 4 when transferring to a peripheral.
+		 */
+		return GENMASK(14, 0) * DMA_SLAVE_BUSWIDTH_4_BYTES;
+	}
+
+	return PAGE_SIZE;
+}
+
+static int dspi_dma_max_datawords(struct fsl_dspi *dspi)
+{
+	/*
+	 * Transfers look like this so we always use a full DMA word regardless
+	 * of SPI word size:
+	 *
+	 * 31              16 15                   0
+	 * -----------------------------------------
+	 * |   CONTROL WORD  |     16-bit DATA     |
+	 * -----------------------------------------
+	 * or
+	 * -----------------------------------------
+	 * |   CONTROL WORD  | UNUSED | 8-bit DATA |
+	 * -----------------------------------------
+	 */
+	return dspi_dma_bufsize(dspi) / DMA_SLAVE_BUSWIDTH_4_BYTES;
+}
+
 static int dspi_dma_transfer_size(struct fsl_dspi *dspi)
 {
 	return dspi->words_in_flight * DMA_SLAVE_BUSWIDTH_4_BYTES;
@@ -608,6 +641,7 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 static void dspi_dma_xfer(struct fsl_dspi *dspi)
 {
 	struct spi_message *message = dspi->cur_msg;
+	int max_words = dspi_dma_max_datawords(dspi);
 	struct device *dev = &dspi->pdev->dev;
 
 	/*
@@ -619,8 +653,8 @@ static void dspi_dma_xfer(struct fsl_dspi *dspi)
 		dspi_setup_accel(dspi);
 
 		dspi->words_in_flight = dspi->len / dspi->oper_word_size;
-		if (dspi->words_in_flight > dspi->devtype_data->fifo_size)
-			dspi->words_in_flight = dspi->devtype_data->fifo_size;
+		if (dspi->words_in_flight > max_words)
+			dspi->words_in_flight = max_words;
 
 		message->actual_length += dspi->words_in_flight *
 					  dspi->oper_word_size;
@@ -635,7 +669,7 @@ static void dspi_dma_xfer(struct fsl_dspi *dspi)
 
 static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
 {
-	int dma_bufsize = dspi->devtype_data->fifo_size * 2;
+	int dma_bufsize = dspi_dma_bufsize(dspi);
 	struct device *dev = &dspi->pdev->dev;
 	struct dma_slave_config cfg;
 	struct fsl_dspi_dma *dma;
@@ -719,7 +753,7 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
 
 static void dspi_release_dma(struct fsl_dspi *dspi)
 {
-	int dma_bufsize = dspi->devtype_data->fifo_size * 2;
+	int dma_bufsize = dspi_dma_bufsize(dspi);
 	struct fsl_dspi_dma *dma = dspi->dma;
 
 	if (!dma)

-- 
2.34.1

[PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors

[James Clark]

In target mode, the host sending more data than can be consumed would be
a common problem for any message exceeding the FIFO or DMA buffer size.
Cancel the whole message as soon as this condition is hit as the message
will be corrupted.

Only do this for target mode in a DMA transfer because we need to add a
register read. In IRQ and polling modes always do it because SPI_SR was
already read and it might catch some host mode programming/buffer
management errors too.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-dspi.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 58881911e74a..16a9769f518d 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -560,12 +560,24 @@ static void dspi_rx_dma_callback(void *arg)
 	complete(&dma->cmd_rx_complete);
 }
 
+static int dspi_fifo_error(struct fsl_dspi *dspi, u32 spi_sr)
+{
+	if (spi_sr & (SPI_SR_TFUF | SPI_SR_RFOF)) {
+		dev_err_ratelimited(&dspi->pdev->dev, "FIFO errors:%s%s\n",
+				    spi_sr & SPI_SR_TFUF ? " TX underflow," : "",
+				    spi_sr & SPI_SR_RFOF ? " RX overflow," : "");
+		return -EIO;
+	}
+	return 0;
+}
+
 static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 {
 	size_t size = dspi_dma_transfer_size(dspi);
 	struct device *dev = &dspi->pdev->dev;
 	struct fsl_dspi_dma *dma = dspi->dma;
 	int time_left;
+	u32 spi_sr;
 	int i;
 
 	for (i = 0; i < dspi->words_in_flight; i++)
@@ -614,7 +626,8 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 
 	if (spi_controller_is_target(dspi->ctlr)) {
 		wait_for_completion_interruptible(&dspi->dma->cmd_rx_complete);
-		return 0;
+		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
+		return dspi_fifo_error(dspi, spi_sr);
 	}
 
 	time_left = wait_for_completion_timeout(&dspi->dma->cmd_tx_complete,
@@ -1069,6 +1082,10 @@ static void dspi_poll(struct fsl_dspi *dspi)
 
 			if (spi_sr & SPI_SR_CMDTCF)
 				break;
+
+			dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
+			if (dspi->cur_msg->status)
+				return;
 		} while (--tries);
 
 		if (!tries) {
@@ -1085,6 +1102,7 @@ static void dspi_poll(struct fsl_dspi *dspi)
 static irqreturn_t dspi_interrupt(int irq, void *dev_id)
 {
 	struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
+	int status;
 	u32 spi_sr;
 
 	regmap_read(dspi->regmap, SPI_SR, &spi_sr);
@@ -1093,6 +1111,14 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
 	if (!(spi_sr & SPI_SR_CMDTCF))
 		return IRQ_NONE;
 
+	status = dspi_fifo_error(dspi, spi_sr);
+	if (status) {
+		if (dspi->cur_msg)
+			WRITE_ONCE(dspi->cur_msg->status, status);
+		complete(&dspi->xfer_done);
+		return IRQ_HANDLED;
+	}
+
 	dspi_rxtx(dspi);
 
 	if (!dspi->len) {

-- 
2.34.1

Re: [PATCH v3 1/6] spi: spi-fsl-dspi: Clear completion counter before initiating transfer

[Frank Li]

On Tue, Jun 24, 2025 at 11:35:31AM +0100, James Clark wrote:
> In target mode, extra interrupts can be received between the end of a
> transfer and halting the module if the host continues sending more data.
> If the interrupt from this occurs after the reinit_completion() then the
> completion counter is left at a non-zero value. The next unrelated
> transfer initiated by userspace will then complete immediately without
> waiting for the interrupt or writing to the RX buffer.
>
> Fix it by resetting the counter before the transfer so that lingering
> values are cleared. This is done after clearing the FIFOs and the
> status register but before the transfer is initiated, so no interrupts
> should be received at this point resulting in other race conditions.
>
> Fixes: 4f5ee75ea171 ("spi: spi-fsl-dspi: Replace interruptible wait queue with a simple completion")
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-dspi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 04c88d090c4d..744dfc561db2 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -1122,11 +1122,19 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  		if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
>  			status = dspi_dma_xfer(dspi);
>  		} else {
> +			/*
> +			 * Reset completion counter to clear any extra
> +			 * complete()s from spurious interrupts that may have
> +			 * happened after the last message's completion but
> +			 * before the module was fully in stop mode.
> +			 */

I think you change is correct. reinit_completion() should be called before
dspi_fifo_write().

comments is quite confused. how about below comments?

/*
 * Reinitialize the completion before transferring data to avoid the case where
 * it might remain in the done state due to a spurious interrupt from a previous
 * transfer. This could falsely signal that the current transfer has completed.
 */

Frank
> +			if (dspi->irq)
> +				reinit_completion(&dspi->xfer_done);
> +
>  			dspi_fifo_write(dspi);
>
>  			if (dspi->irq) {
>  				wait_for_completion(&dspi->xfer_done);
> -				reinit_completion(&dspi->xfer_done);
>  			} else {
>  				do {
>  					status = dspi_poll(dspi);
>
> --
> 2.34.1
>

Re: [PATCH v3 2/6] spi: spi-fsl-dspi: Store status directly in cur_msg->status

[Frank Li]

On Tue, Jun 24, 2025 at 11:35:32AM +0100, James Clark wrote:
> This will allow us to return a status from the interrupt handler in a
> later commit and avoids copying it at the end of
> dspi_transfer_one_message(). For consistency make polling and DMA modes
> use the same mechanism.
>
> Refactor dspi_rxtx() and dspi_poll() to not return -EINPROGRESS because
> this isn't actually a status that was ever returned to the core layer
> but some internal state. Wherever that was used we can look at dspi->len
> instead.
>
> No functional changes intended.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-dspi.c | 68 ++++++++++++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 744dfc561db2..feb29bb92a77 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -591,11 +591,10 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>
>  static void dspi_setup_accel(struct fsl_dspi *dspi);
>
> -static int dspi_dma_xfer(struct fsl_dspi *dspi)
> +static void dspi_dma_xfer(struct fsl_dspi *dspi)
>  {
>  	struct spi_message *message = dspi->cur_msg;
>  	struct device *dev = &dspi->pdev->dev;
> -	int ret = 0;
>
>  	/*
>  	 * dspi->len gets decremented by dspi_pop_tx_pushr in
> @@ -612,14 +611,12 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
>  		message->actual_length += dspi->words_in_flight *
>  					  dspi->oper_word_size;
>
> -		ret = dspi_next_xfer_dma_submit(dspi);
> -		if (ret) {
> +		message->status = dspi_next_xfer_dma_submit(dspi);
> +		if (message->status) {
>  			dev_err(dev, "DMA transfer failed\n");
>  			break;
>  		}
>  	}
> -
> -	return ret;
>  }
>
>  static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
> @@ -986,36 +983,40 @@ static void dspi_fifo_write(struct fsl_dspi *dspi)
>  				dspi->progress, !dspi->irq);
>  }
>
> -static int dspi_rxtx(struct fsl_dspi *dspi)
> +static void dspi_rxtx(struct fsl_dspi *dspi)
>  {
>  	dspi_fifo_read(dspi);
>
>  	if (!dspi->len)
>  		/* Success! */
> -		return 0;
> +		return;
>
>  	dspi_fifo_write(dspi);
> -
> -	return -EINPROGRESS;
>  }
>
> -static int dspi_poll(struct fsl_dspi *dspi)
> +static void dspi_poll(struct fsl_dspi *dspi)
>  {
>  	int tries = 1000;
>  	u32 spi_sr;
>
> -	do {
> -		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> -		regmap_write(dspi->regmap, SPI_SR, spi_sr);
> +	while (dspi->len) {

Preivous have not checked dspi->len.

Not sure if it is logical equivalence

> +		do {
> +			regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> +			regmap_write(dspi->regmap, SPI_SR, spi_sr);
>
> -		if (spi_sr & SPI_SR_CMDTCF)
> -			break;
> -	} while (--tries);
> +			if (spi_sr & SPI_SR_CMDTCF)
> +				break;
> +		} while (--tries);
>
> -	if (!tries)
> -		return -ETIMEDOUT;
> +		if (!tries) {
> +			dspi->cur_msg->status = -ETIMEDOUT;
> +			return;
> +		}
>
> -	return dspi_rxtx(dspi);
> +		dspi_rxtx(dspi);
> +	}
> +
> +	dspi->cur_msg->status = 0;
>  }
>
>  static irqreturn_t dspi_interrupt(int irq, void *dev_id)
> @@ -1029,8 +1030,13 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>  	if (!(spi_sr & SPI_SR_CMDTCF))
>  		return IRQ_NONE;
>
> -	if (dspi_rxtx(dspi) == 0)
> +	dspi_rxtx(dspi);
> +
> +	if (!dspi->len) {
> +		if (dspi->cur_msg)
> +			WRITE_ONCE(dspi->cur_msg->status, 0);
>  		complete(&dspi->xfer_done);
> +	}
>
>  	return IRQ_HANDLED;
>  }
> @@ -1060,7 +1066,6 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  	struct spi_device *spi = message->spi;
>  	struct spi_transfer *transfer;
>  	bool cs = false;
> -	int status = 0;
>  	u32 val = 0;
>  	bool cs_change = false;
>
> @@ -1120,7 +1125,7 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  				       dspi->progress, !dspi->irq);
>
>  		if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
> -			status = dspi_dma_xfer(dspi);
> +			dspi_dma_xfer(dspi);
>  		} else {
>  			/*
>  			 * Reset completion counter to clear any extra
> @@ -1133,15 +1138,12 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>
>  			dspi_fifo_write(dspi);
>
> -			if (dspi->irq) {
> +			if (dspi->irq)
>  				wait_for_completion(&dspi->xfer_done);
> -			} else {
> -				do {
> -					status = dspi_poll(dspi);
> -				} while (status == -EINPROGRESS);
> -			}
> +			else
> +				dspi_poll(dspi);
>  		}
> -		if (status)
> +		if (READ_ONCE(message->status))

Why need READ_ONCE()? Does any hardware (DMA) set status?

where update message->status at pio mode.

Frank
>  			break;
>
>  		spi_transfer_delay_exec(transfer);
> @@ -1150,7 +1152,8 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  			dspi_deassert_cs(spi, &cs);
>  	}
>
> -	if (status || !cs_change) {
> +	dspi->cur_msg = NULL;
> +	if (message->status || !cs_change) {
>  		/* Put DSPI in stop mode */
>  		regmap_update_bits(dspi->regmap, SPI_MCR,
>  				   SPI_MCR_HALT, SPI_MCR_HALT);
> @@ -1159,10 +1162,9 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  			;
>  	}
>
> -	message->status = status;
>  	spi_finalize_current_message(ctlr);
>
> -	return status;
> +	return message->status;
>  }
>
>  static int dspi_set_mtf(struct fsl_dspi *dspi)
>
> --
> 2.34.1
>

Re: [PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions

[Frank Li]

On Tue, Jun 24, 2025 at 11:35:33AM +0100, James Clark wrote:
> This will allow the build to succeed with !CONFIG_HAS_DMA, either due to
> a randconfig build test or when the target only uses one of the non-DMA

I supposed you met kbuild error. If yes, can you add kbuild build report
tags.

Frank
> transfer modes which this driver supports.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-dspi.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index feb29bb92a77..8212c4193536 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -373,6 +373,8 @@ struct fsl_dspi {
>  	void (*dev_to_host)(struct fsl_dspi *dspi, u32 rxdata);
>  };
>
> +static void dspi_setup_accel(struct fsl_dspi *dspi);
> +
>  static bool is_s32g_dspi(struct fsl_dspi *data)
>  {
>  	return data->devtype_data == &devtype_data[S32G] ||
> @@ -489,6 +491,7 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
>  	dspi->dev_to_host(dspi, rxdata);
>  }
>
> +#if IS_ENABLED(CONFIG_HAS_DMA)
>  static void dspi_tx_dma_callback(void *arg)
>  {
>  	struct fsl_dspi *dspi = arg;
> @@ -589,8 +592,6 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>  	return 0;
>  }
>
> -static void dspi_setup_accel(struct fsl_dspi *dspi);
> -
>  static void dspi_dma_xfer(struct fsl_dspi *dspi)
>  {
>  	struct spi_message *message = dspi->cur_msg;
> @@ -722,6 +723,18 @@ static void dspi_release_dma(struct fsl_dspi *dspi)
>  		dma_release_channel(dma->chan_rx);
>  	}
>  }
> +#else
> +static void dspi_dma_xfer(struct fsl_dspi *dspi)
> +{
> +	sdpi->cur_msg->status = -EINVAL;
> +}
> +static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
> +{
> +	dev_err(&dspi->pdev->dev, "DMA support not enabled in kernel\n");
> +	return -EINVAL;
> +}
> +static void dspi_release_dma(struct fsl_dspi *dspi) {}
> +#endif
>
>  static void hz_to_spi_baud(char *pbr, char *br, int speed_hz,
>  			   unsigned long clkrate, bool mtf_enabled)
>
> --
> 2.34.1
>

Re: [PATCH v3 4/6] spi: spi-fsl-dspi: Use non-coherent memory for DMA

[Frank Li]

On Tue, Jun 24, 2025 at 11:35:34AM +0100, James Clark wrote:
> Using coherent memory here isn't functionally necessary. Because the
> change to use non-coherent memory isn't overly complex and only a few
> synchronization points are required, we might as well do it while fixing
> up some other DMA issues.
>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: James Clark <james.clark@linaro.org>

In https://lore.kernel.org/imx/c65c752a-5b60-4f30-8d51-9a903ddd55a6@linaro.org/

look like less performance, why need this patch.

In https://lore.kernel.org/imx/ad7e9aa7-74a3-449d-8ed9-cb270fd5c718@linaro.org/
look like better.

Any conclusion?

Need performance gain here if it is better.

Frank

> ---
>  drivers/spi/spi-fsl-dspi.c | 56 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 8212c4193536..172eb9929de1 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -492,11 +492,20 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
>  }
>
>  #if IS_ENABLED(CONFIG_HAS_DMA)
> +
> +static int dspi_dma_transfer_size(struct fsl_dspi *dspi)
> +{
> +	return dspi->words_in_flight * DMA_SLAVE_BUSWIDTH_4_BYTES;
> +}
> +
>  static void dspi_tx_dma_callback(void *arg)
>  {
>  	struct fsl_dspi *dspi = arg;
>  	struct fsl_dspi_dma *dma = dspi->dma;
> +	struct device *dev = &dspi->pdev->dev;
>
> +	dma_sync_single_for_cpu(dev, dma->tx_dma_phys,
> +				dspi_dma_transfer_size(dspi), DMA_TO_DEVICE);
>  	complete(&dma->cmd_tx_complete);
>  }
>
> @@ -504,9 +513,13 @@ static void dspi_rx_dma_callback(void *arg)
>  {
>  	struct fsl_dspi *dspi = arg;
>  	struct fsl_dspi_dma *dma = dspi->dma;
> +	struct device *dev = &dspi->pdev->dev;
>  	int i;
>
>  	if (dspi->rx) {
> +		dma_sync_single_for_cpu(dev, dma->rx_dma_phys,
> +					dspi_dma_transfer_size(dspi),
> +					DMA_FROM_DEVICE);
>  		for (i = 0; i < dspi->words_in_flight; i++)
>  			dspi_push_rx(dspi, dspi->dma->rx_dma_buf[i]);
>  	}
> @@ -516,6 +529,7 @@ static void dspi_rx_dma_callback(void *arg)
>
>  static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>  {
> +	size_t size = dspi_dma_transfer_size(dspi);
>  	struct device *dev = &dspi->pdev->dev;
>  	struct fsl_dspi_dma *dma = dspi->dma;
>  	int time_left;
> @@ -524,10 +538,9 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>  	for (i = 0; i < dspi->words_in_flight; i++)
>  		dspi->dma->tx_dma_buf[i] = dspi_pop_tx_pushr(dspi);
>
> +	dma_sync_single_for_device(dev, dma->tx_dma_phys, size, DMA_TO_DEVICE);
>  	dma->tx_desc = dmaengine_prep_slave_single(dma->chan_tx,
> -					dma->tx_dma_phys,
> -					dspi->words_in_flight *
> -					DMA_SLAVE_BUSWIDTH_4_BYTES,
> +					dma->tx_dma_phys, size,
>  					DMA_MEM_TO_DEV,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  	if (!dma->tx_desc) {
> @@ -542,10 +555,10 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>  		return -EINVAL;
>  	}
>
> +	dma_sync_single_for_device(dev, dma->rx_dma_phys, size,
> +				   DMA_FROM_DEVICE);
>  	dma->rx_desc = dmaengine_prep_slave_single(dma->chan_rx,
> -					dma->rx_dma_phys,
> -					dspi->words_in_flight *
> -					DMA_SLAVE_BUSWIDTH_4_BYTES,
> +					dma->rx_dma_phys, size,
>  					DMA_DEV_TO_MEM,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  	if (!dma->rx_desc) {
> @@ -642,17 +655,17 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
>  		goto err_tx_channel;
>  	}
>
> -	dma->tx_dma_buf = dma_alloc_coherent(dma->chan_tx->device->dev,
> -					     dma_bufsize, &dma->tx_dma_phys,
> -					     GFP_KERNEL);
> +	dma->tx_dma_buf = dma_alloc_noncoherent(dma->chan_tx->device->dev,
> +						dma_bufsize, &dma->tx_dma_phys,
> +						DMA_TO_DEVICE, GFP_KERNEL);
>  	if (!dma->tx_dma_buf) {
>  		ret = -ENOMEM;
>  		goto err_tx_dma_buf;
>  	}
>
> -	dma->rx_dma_buf = dma_alloc_coherent(dma->chan_rx->device->dev,
> -					     dma_bufsize, &dma->rx_dma_phys,
> -					     GFP_KERNEL);
> +	dma->rx_dma_buf = dma_alloc_noncoherent(dma->chan_rx->device->dev,
> +						dma_bufsize, &dma->rx_dma_phys,
> +						DMA_FROM_DEVICE, GFP_KERNEL);
>  	if (!dma->rx_dma_buf) {
>  		ret = -ENOMEM;
>  		goto err_rx_dma_buf;
> @@ -687,11 +700,12 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
>  	return 0;
>
>  err_slave_config:
> -	dma_free_coherent(dma->chan_rx->device->dev,
> -			  dma_bufsize, dma->rx_dma_buf, dma->rx_dma_phys);
> +	dma_free_noncoherent(dma->chan_rx->device->dev, dma_bufsize,
> +			     dma->rx_dma_buf, dma->rx_dma_phys,
> +			     DMA_FROM_DEVICE);
>  err_rx_dma_buf:
> -	dma_free_coherent(dma->chan_tx->device->dev,
> -			  dma_bufsize, dma->tx_dma_buf, dma->tx_dma_phys);
> +	dma_free_noncoherent(dma->chan_tx->device->dev, dma_bufsize,
> +			     dma->tx_dma_buf, dma->tx_dma_phys, DMA_TO_DEVICE);
>  err_tx_dma_buf:
>  	dma_release_channel(dma->chan_tx);
>  err_tx_channel:
> @@ -712,14 +726,16 @@ static void dspi_release_dma(struct fsl_dspi *dspi)
>  		return;
>
>  	if (dma->chan_tx) {
> -		dma_free_coherent(dma->chan_tx->device->dev, dma_bufsize,
> -				  dma->tx_dma_buf, dma->tx_dma_phys);
> +		dma_free_noncoherent(dma->chan_tx->device->dev, dma_bufsize,
> +				     dma->tx_dma_buf, dma->tx_dma_phys,
> +				     DMA_TO_DEVICE);
>  		dma_release_channel(dma->chan_tx);
>  	}
>
>  	if (dma->chan_rx) {
> -		dma_free_coherent(dma->chan_rx->device->dev, dma_bufsize,
> -				  dma->rx_dma_buf, dma->rx_dma_phys);
> +		dma_free_noncoherent(dma->chan_rx->device->dev, dma_bufsize,
> +				     dma->rx_dma_buf, dma->rx_dma_phys,
> +				     DMA_FROM_DEVICE);
>  		dma_release_channel(dma->chan_rx);
>  	}
>  }
>
> --
> 2.34.1
>

Re: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors

[Frank Li]

On Tue, Jun 24, 2025 at 11:35:36AM +0100, James Clark wrote:
> In target mode, the host sending more data than can be consumed would be
> a common problem for any message exceeding the FIFO or DMA buffer size.
> Cancel the whole message as soon as this condition is hit as the message
> will be corrupted.
>
> Only do this for target mode in a DMA transfer because we need to add a
> register read.

"We need to add a register read" is not reason.

Add checking FIFO error status at target mode in a DMA transfer since PIO
mode already do it. It help catch some host mode ...

> In IRQ and polling modes always do it because SPI_SR was
> already read and it might catch some host mode programming/buffer
> management errors too.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-dspi.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 58881911e74a..16a9769f518d 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -560,12 +560,24 @@ static void dspi_rx_dma_callback(void *arg)
>  	complete(&dma->cmd_rx_complete);
>  }
>
> +static int dspi_fifo_error(struct fsl_dspi *dspi, u32 spi_sr)
> +{
> +	if (spi_sr & (SPI_SR_TFUF | SPI_SR_RFOF)) {
> +		dev_err_ratelimited(&dspi->pdev->dev, "FIFO errors:%s%s\n",
> +				    spi_sr & SPI_SR_TFUF ? " TX underflow," : "",
> +				    spi_sr & SPI_SR_RFOF ? " RX overflow," : "");
> +		return -EIO;
> +	}
> +	return 0;
> +}
> +
>  static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>  {
>  	size_t size = dspi_dma_transfer_size(dspi);
>  	struct device *dev = &dspi->pdev->dev;
>  	struct fsl_dspi_dma *dma = dspi->dma;
>  	int time_left;
> +	u32 spi_sr;
>  	int i;
>
>  	for (i = 0; i < dspi->words_in_flight; i++)
> @@ -614,7 +626,8 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>
>  	if (spi_controller_is_target(dspi->ctlr)) {
>  		wait_for_completion_interruptible(&dspi->dma->cmd_rx_complete);
> -		return 0;
> +		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> +		return dspi_fifo_error(dspi, spi_sr);
>  	}
>
>  	time_left = wait_for_completion_timeout(&dspi->dma->cmd_tx_complete,
> @@ -1069,6 +1082,10 @@ static void dspi_poll(struct fsl_dspi *dspi)
>
>  			if (spi_sr & SPI_SR_CMDTCF)
>  				break;
> +
> +			dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
> +			if (dspi->cur_msg->status)
> +				return;


Although fifo error may happen after you check, it may reduce some possilbity
and catch some problems.

Frak

>  		} while (--tries);
>
>  		if (!tries) {
> @@ -1085,6 +1102,7 @@ static void dspi_poll(struct fsl_dspi *dspi)
>  static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>  {
>  	struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
> +	int status;
>  	u32 spi_sr;
>
>  	regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> @@ -1093,6 +1111,14 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>  	if (!(spi_sr & SPI_SR_CMDTCF))
>  		return IRQ_NONE;
>
> +	status = dspi_fifo_error(dspi, spi_sr);
> +	if (status) {
> +		if (dspi->cur_msg)
> +			WRITE_ONCE(dspi->cur_msg->status, status);
> +		complete(&dspi->xfer_done);
> +		return IRQ_HANDLED;
> +	}
> +
>  	dspi_rxtx(dspi);
>
>  	if (!dspi->len) {
>
> --
> 2.34.1
>

Re: [PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions

[Arnd Bergmann]

On Tue, Jun 24, 2025, at 18:29, Frank Li wrote:
> On Tue, Jun 24, 2025 at 11:35:33AM +0100, James Clark wrote:
>> This will allow the build to succeed with !CONFIG_HAS_DMA, either due to
>> a randconfig build test or when the target only uses one of the non-DMA
>
> I supposed you met kbuild error. If yes, can you add kbuild build report
> tags.

Actually I would suggest making it a dependency on CONFIG_DMA_ENGINE
instead of CONFIG_HAS_DMA, since that is the more relevant symbol.

It would also be simpler to enforce this in Kconfig if we only
care about users that use the DMA support. 

      Arnd

Re: [PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions

[kernel test robot]

Hi James,

kernel test robot noticed the following build errors:

[auto build test ERROR on 4f326fa6236787ca516ea6eab8e5e9dc5c236f03]

url:    https://github.com/intel-lab-lkp/linux/commits/James-Clark/spi-spi-fsl-dspi-Clear-completion-counter-before-initiating-transfer/20250624-183952
base:   4f326fa6236787ca516ea6eab8e5e9dc5c236f03
patch link:    https://lore.kernel.org/r/20250624-james-nxp-spi-dma-v3-3-e7d574f5f62c%40linaro.org
patch subject: [PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250625/202506251332.thYB4ced-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250625/202506251332.thYB4ced-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506251332.thYB4ced-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/spi/spi-fsl-dspi.c: In function 'dspi_dma_xfer':
>> drivers/spi/spi-fsl-dspi.c:729:9: error: 'sdpi' undeclared (first use in this function); did you mean 'dspi'?
     729 |         sdpi->cur_msg->status = -EINVAL;
         |         ^~~~
         |         dspi
   drivers/spi/spi-fsl-dspi.c:729:9: note: each undeclared identifier is reported only once for each function it appears in
   drivers/spi/spi-fsl-dspi.c: At top level:
>> drivers/spi/spi-fsl-dspi.c:474:12: warning: 'dspi_pop_tx_pushr' defined but not used [-Wunused-function]
     474 | static u32 dspi_pop_tx_pushr(struct fsl_dspi *dspi)
         |            ^~~~~~~~~~~~~~~~~


vim +729 drivers/spi/spi-fsl-dspi.c

   705	
   706	static void dspi_release_dma(struct fsl_dspi *dspi)
   707	{
   708		int dma_bufsize = dspi->devtype_data->fifo_size * 2;
   709		struct fsl_dspi_dma *dma = dspi->dma;
   710	
   711		if (!dma)
   712			return;
   713	
   714		if (dma->chan_tx) {
   715			dma_free_coherent(dma->chan_tx->device->dev, dma_bufsize,
   716					  dma->tx_dma_buf, dma->tx_dma_phys);
   717			dma_release_channel(dma->chan_tx);
   718		}
   719	
   720		if (dma->chan_rx) {
   721			dma_free_coherent(dma->chan_rx->device->dev, dma_bufsize,
   722					  dma->rx_dma_buf, dma->rx_dma_phys);
   723			dma_release_channel(dma->chan_rx);
   724		}
   725	}
   726	#else
   727	static void dspi_dma_xfer(struct fsl_dspi *dspi)
   728	{
 > 729		sdpi->cur_msg->status = -EINVAL;
   730	}
   731	static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
   732	{
   733		dev_err(&dspi->pdev->dev, "DMA support not enabled in kernel\n");
   734		return -EINVAL;
   735	}
   736	static void dspi_release_dma(struct fsl_dspi *dspi) {}
   737	#endif
   738	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors

[kernel test robot]

Hi James,

kernel test robot noticed the following build errors:

[auto build test ERROR on 4f326fa6236787ca516ea6eab8e5e9dc5c236f03]

url:    https://github.com/intel-lab-lkp/linux/commits/James-Clark/spi-spi-fsl-dspi-Clear-completion-counter-before-initiating-transfer/20250624-183952
base:   4f326fa6236787ca516ea6eab8e5e9dc5c236f03
patch link:    https://lore.kernel.org/r/20250624-james-nxp-spi-dma-v3-6-e7d574f5f62c%40linaro.org
patch subject: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250625/202506251415.Cj026uIP-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250625/202506251415.Cj026uIP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506251415.Cj026uIP-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/spi/spi-fsl-dspi.c: In function 'dspi_dma_xfer':
   drivers/spi/spi-fsl-dspi.c:792:9: error: 'sdpi' undeclared (first use in this function); did you mean 'dspi'?
     792 |         sdpi->cur_msg->status = -EINVAL;
         |         ^~~~
         |         dspi
   drivers/spi/spi-fsl-dspi.c:792:9: note: each undeclared identifier is reported only once for each function it appears in
   drivers/spi/spi-fsl-dspi.c: In function 'dspi_poll':
>> drivers/spi/spi-fsl-dspi.c:1086:49: error: implicit declaration of function 'dspi_fifo_error'; did you mean 'dspi_fifo_write'? [-Wimplicit-function-declaration]
    1086 |                         dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
         |                                                 ^~~~~~~~~~~~~~~
         |                                                 dspi_fifo_write
   drivers/spi/spi-fsl-dspi.c: At top level:
   drivers/spi/spi-fsl-dspi.c:474:12: warning: 'dspi_pop_tx_pushr' defined but not used [-Wunused-function]
     474 | static u32 dspi_pop_tx_pushr(struct fsl_dspi *dspi)
         |            ^~~~~~~~~~~~~~~~~


vim +1086 drivers/spi/spi-fsl-dspi.c

  1072	
  1073	static void dspi_poll(struct fsl_dspi *dspi)
  1074	{
  1075		int tries = 1000;
  1076		u32 spi_sr;
  1077	
  1078		while (dspi->len) {
  1079			do {
  1080				regmap_read(dspi->regmap, SPI_SR, &spi_sr);
  1081				regmap_write(dspi->regmap, SPI_SR, spi_sr);
  1082	
  1083				if (spi_sr & SPI_SR_CMDTCF)
  1084					break;
  1085	
> 1086				dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
  1087				if (dspi->cur_msg->status)
  1088					return;
  1089			} while (--tries);
  1090	
  1091			if (!tries) {
  1092				dspi->cur_msg->status = -ETIMEDOUT;
  1093				return;
  1094			}
  1095	
  1096			dspi_rxtx(dspi);
  1097		}
  1098	
  1099		dspi->cur_msg->status = 0;
  1100	}
  1101	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 4/6] spi: spi-fsl-dspi: Use non-coherent memory for DMA

[James Clark]

On 24/06/2025 5:39 pm, Frank Li wrote:
> On Tue, Jun 24, 2025 at 11:35:34AM +0100, James Clark wrote:
>> Using coherent memory here isn't functionally necessary. Because the
>> change to use non-coherent memory isn't overly complex and only a few
>> synchronization points are required, we might as well do it while fixing
>> up some other DMA issues.
>>
>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: James Clark <james.clark@linaro.org>
> 
> In https://lore.kernel.org/imx/c65c752a-5b60-4f30-8d51-9a903ddd55a6@linaro.org/
> 
> look like less performance, why need this patch.
> 
> In https://lore.kernel.org/imx/ad7e9aa7-74a3-449d-8ed9-cb270fd5c718@linaro.org/
> look like better.
> 
> Any conclusion?
> 
> Need performance gain here if it is better.
> 
> Frank
> 

Hi Frank,

The performance figures for this set are in the cover letter. It's 
either the same or faster, there is no evidence of worse performance. 
The one you linked was a bad result from not testing it in DMA mode, but 
it's corrected later in that thread.

The reason the figures aren't in this commit is because it requires this 
change and the one to increase the size of the buffer.

Thanks
James

Re: [PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions

[James Clark]

On 24/06/2025 6:16 pm, Arnd Bergmann wrote:
> On Tue, Jun 24, 2025, at 18:29, Frank Li wrote:
>> On Tue, Jun 24, 2025 at 11:35:33AM +0100, James Clark wrote:
>>> This will allow the build to succeed with !CONFIG_HAS_DMA, either due to
>>> a randconfig build test or when the target only uses one of the non-DMA
>>
>> I supposed you met kbuild error. If yes, can you add kbuild build report
>> tags.
> 
> Actually I would suggest making it a dependency on CONFIG_DMA_ENGINE
> instead of CONFIG_HAS_DMA, since that is the more relevant symbol.
> 

Makes sense.

> It would also be simpler to enforce this in Kconfig if we only
> care about users that use the DMA support.
> 
>        Arnd

But most of the devices supported by the driver don't do any DMA. That 
was the reason to stub them out rather than add the Kconfig depends.

Re: [PATCH v3 1/6] spi: spi-fsl-dspi: Clear completion counter before initiating transfer

[James Clark]

On 24/06/2025 4:58 pm, Frank Li wrote:
> On Tue, Jun 24, 2025 at 11:35:31AM +0100, James Clark wrote:
>> In target mode, extra interrupts can be received between the end of a
>> transfer and halting the module if the host continues sending more data.
>> If the interrupt from this occurs after the reinit_completion() then the
>> completion counter is left at a non-zero value. The next unrelated
>> transfer initiated by userspace will then complete immediately without
>> waiting for the interrupt or writing to the RX buffer.
>>
>> Fix it by resetting the counter before the transfer so that lingering
>> values are cleared. This is done after clearing the FIFOs and the
>> status register but before the transfer is initiated, so no interrupts
>> should be received at this point resulting in other race conditions.
>>
>> Fixes: 4f5ee75ea171 ("spi: spi-fsl-dspi: Replace interruptible wait queue with a simple completion")
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-dspi.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>> index 04c88d090c4d..744dfc561db2 100644
>> --- a/drivers/spi/spi-fsl-dspi.c
>> +++ b/drivers/spi/spi-fsl-dspi.c
>> @@ -1122,11 +1122,19 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>>   		if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
>>   			status = dspi_dma_xfer(dspi);
>>   		} else {
>> +			/*
>> +			 * Reset completion counter to clear any extra
>> +			 * complete()s from spurious interrupts that may have
>> +			 * happened after the last message's completion but
>> +			 * before the module was fully in stop mode.
>> +			 */
> 
> I think you change is correct. reinit_completion() should be called before
> dspi_fifo_write().
> 
> comments is quite confused. how about below comments?
> 
> /*
>   * Reinitialize the completion before transferring data to avoid the case where
>   * it might remain in the done state due to a spurious interrupt from a previous
>   * transfer. This could falsely signal that the current transfer has completed.
>   */
> 
> Frank

Will do

>> +			if (dspi->irq)
>> +				reinit_completion(&dspi->xfer_done);
>> +
>>   			dspi_fifo_write(dspi);
>>
>>   			if (dspi->irq) {
>>   				wait_for_completion(&dspi->xfer_done);
>> -				reinit_completion(&dspi->xfer_done);
>>   			} else {
>>   				do {
>>   					status = dspi_poll(dspi);
>>
>> --
>> 2.34.1
>>

Re: [PATCH v3 2/6] spi: spi-fsl-dspi: Store status directly in cur_msg->status

[James Clark]

On 24/06/2025 5:18 pm, Frank Li wrote:
> On Tue, Jun 24, 2025 at 11:35:32AM +0100, James Clark wrote:
>> This will allow us to return a status from the interrupt handler in a
>> later commit and avoids copying it at the end of
>> dspi_transfer_one_message(). For consistency make polling and DMA modes
>> use the same mechanism.
>>
>> Refactor dspi_rxtx() and dspi_poll() to not return -EINPROGRESS because
>> this isn't actually a status that was ever returned to the core layer
>> but some internal state. Wherever that was used we can look at dspi->len
>> instead.
>>
>> No functional changes intended.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-dspi.c | 68 ++++++++++++++++++++++++----------------------
>>   1 file changed, 35 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>> index 744dfc561db2..feb29bb92a77 100644
>> --- a/drivers/spi/spi-fsl-dspi.c
>> +++ b/drivers/spi/spi-fsl-dspi.c
>> @@ -591,11 +591,10 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>>
>>   static void dspi_setup_accel(struct fsl_dspi *dspi);
>>
>> -static int dspi_dma_xfer(struct fsl_dspi *dspi)
>> +static void dspi_dma_xfer(struct fsl_dspi *dspi)
>>   {
>>   	struct spi_message *message = dspi->cur_msg;
>>   	struct device *dev = &dspi->pdev->dev;
>> -	int ret = 0;
>>
>>   	/*
>>   	 * dspi->len gets decremented by dspi_pop_tx_pushr in
>> @@ -612,14 +611,12 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
>>   		message->actual_length += dspi->words_in_flight *
>>   					  dspi->oper_word_size;
>>
>> -		ret = dspi_next_xfer_dma_submit(dspi);
>> -		if (ret) {
>> +		message->status = dspi_next_xfer_dma_submit(dspi);
>> +		if (message->status) {
>>   			dev_err(dev, "DMA transfer failed\n");
>>   			break;
>>   		}
>>   	}
>> -
>> -	return ret;
>>   }
>>
>>   static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
>> @@ -986,36 +983,40 @@ static void dspi_fifo_write(struct fsl_dspi *dspi)
>>   				dspi->progress, !dspi->irq);
>>   }
>>
>> -static int dspi_rxtx(struct fsl_dspi *dspi)
>> +static void dspi_rxtx(struct fsl_dspi *dspi)
>>   {
>>   	dspi_fifo_read(dspi);
>>
>>   	if (!dspi->len)
>>   		/* Success! */
>> -		return 0;
>> +		return;
>>
>>   	dspi_fifo_write(dspi);
>> -
>> -	return -EINPROGRESS;
>>   }
>>
>> -static int dspi_poll(struct fsl_dspi *dspi)
>> +static void dspi_poll(struct fsl_dspi *dspi)
>>   {
>>   	int tries = 1000;
>>   	u32 spi_sr;
>>
>> -	do {
>> -		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>> -		regmap_write(dspi->regmap, SPI_SR, spi_sr);
>> +	while (dspi->len) {
> 
> Preivous have not checked dspi->len.
> 
> Not sure if it is logical equivalence
> 

It used to be this:

    status = dspi_poll(dspi);
  } while (status == -EINPROGRESS);

Where checking status for -EINPROGRESS is equivalent to checking dspi->len.

>> +		do {
>> +			regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>> +			regmap_write(dspi->regmap, SPI_SR, spi_sr);
>>
>> -		if (spi_sr & SPI_SR_CMDTCF)
>> -			break;
>> -	} while (--tries);
>> +			if (spi_sr & SPI_SR_CMDTCF)
>> +				break;
>> +		} while (--tries);
>>
>> -	if (!tries)
>> -		return -ETIMEDOUT;
>> +		if (!tries) {
>> +			dspi->cur_msg->status = -ETIMEDOUT;
>> +			return;
>> +		}
>>
>> -	return dspi_rxtx(dspi);
>> +		dspi_rxtx(dspi);
>> +	}
>> +
>> +	dspi->cur_msg->status = 0;
>>   }
>>
>>   static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>> @@ -1029,8 +1030,13 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>>   	if (!(spi_sr & SPI_SR_CMDTCF))
>>   		return IRQ_NONE;
>>
>> -	if (dspi_rxtx(dspi) == 0)
>> +	dspi_rxtx(dspi);
>> +
>> +	if (!dspi->len) {
>> +		if (dspi->cur_msg)
>> +			WRITE_ONCE(dspi->cur_msg->status, 0);
>>   		complete(&dspi->xfer_done);
>> +	}
>>
>>   	return IRQ_HANDLED;
>>   }
>> @@ -1060,7 +1066,6 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>>   	struct spi_device *spi = message->spi;
>>   	struct spi_transfer *transfer;
>>   	bool cs = false;
>> -	int status = 0;
>>   	u32 val = 0;
>>   	bool cs_change = false;
>>
>> @@ -1120,7 +1125,7 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>>   				       dspi->progress, !dspi->irq);
>>
>>   		if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
>> -			status = dspi_dma_xfer(dspi);
>> +			dspi_dma_xfer(dspi);
>>   		} else {
>>   			/*
>>   			 * Reset completion counter to clear any extra
>> @@ -1133,15 +1138,12 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>>
>>   			dspi_fifo_write(dspi);
>>
>> -			if (dspi->irq) {
>> +			if (dspi->irq)
>>   				wait_for_completion(&dspi->xfer_done);
>> -			} else {
>> -				do {
>> -					status = dspi_poll(dspi);
>> -				} while (status == -EINPROGRESS);
>> -			}
>> +			else
>> +				dspi_poll(dspi);
>>   		}
>> -		if (status)
>> +		if (READ_ONCE(message->status))
> 
> Why need READ_ONCE()? Does any hardware (DMA) set status?
> 
> where update message->status at pio mode.
> 
> Frank

Because message->status is set from dspi_interrupt(), so the compiler 
wouldn't be aware that this variable can be changed from elsewhere. 
Otherwise it could assume that it's never written to in 
dspi_transfer_one_message() and not bother to read it again.

>>   			break;
>>
>>   		spi_transfer_delay_exec(transfer);
>> @@ -1150,7 +1152,8 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>>   			dspi_deassert_cs(spi, &cs);
>>   	}
>>
>> -	if (status || !cs_change) {
>> +	dspi->cur_msg = NULL;
>> +	if (message->status || !cs_change) {
>>   		/* Put DSPI in stop mode */
>>   		regmap_update_bits(dspi->regmap, SPI_MCR,
>>   				   SPI_MCR_HALT, SPI_MCR_HALT);
>> @@ -1159,10 +1162,9 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>>   			;
>>   	}
>>
>> -	message->status = status;
>>   	spi_finalize_current_message(ctlr);
>>
>> -	return status;
>> +	return message->status;
>>   }
>>
>>   static int dspi_set_mtf(struct fsl_dspi *dspi)
>>
>> --
>> 2.34.1
>>

Re: [PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions

[Arnd Bergmann]

On Wed, Jun 25, 2025, at 11:19, James Clark wrote:
> On 24/06/2025 6:16 pm, Arnd Bergmann wrote:
>> On Tue, Jun 24, 2025, at 18:29, Frank Li wrote:
>
>> It would also be simpler to enforce this in Kconfig if we only
>> care about users that use the DMA support.
>
> But most of the devices supported by the driver don't do any DMA. That 
> was the reason to stub them out rather than add the Kconfig depends.

Ah right. So even when running on SoCs that have a DMA engine,
you can end up not using it?

In this case you could have an extra Kconfig symbol to configure
DMA support for this driver and use that in the source code:

config SPI_FSL_DSPI_DMA
      bool "Use DMA engine for offloading Freescale DSPI transfers"
      depends on SPI_FSL_DSPI && DMA_ENGINE
      help
         ....


     Arnd

Re: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors

[James Clark]

On 24/06/2025 5:50 pm, Frank Li wrote:
> On Tue, Jun 24, 2025 at 11:35:36AM +0100, James Clark wrote:
>> In target mode, the host sending more data than can be consumed would be
>> a common problem for any message exceeding the FIFO or DMA buffer size.
>> Cancel the whole message as soon as this condition is hit as the message
>> will be corrupted.
>>
>> Only do this for target mode in a DMA transfer because we need to add a
>> register read.
> 
> "We need to add a register read" is not reason.
> 

Maybe: "It's not likely to catch any errors in host mode so optimize by 
avoiding an extra register read"?

> Add checking FIFO error status at target mode in a DMA transfer since PIO
> mode already do it. It help catch some host mode ...
> 

Are you suggesting that we check for FIFO errors in host mode too? It 
requires an extra read and check in dspi_tx_dma_callback(), but I'm not 
sure what it could catch. Realistically as long as everything is setup 
correctly then neither of those flags will be set. It will either always 
work or never work.

It might be better to add it later if a use becomes apparent otherwise 
it's extra noise in the code.

>> In IRQ and polling modes always do it because SPI_SR was
>> already read and it might catch some host mode programming/buffer
>> management errors too.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-dspi.c | 28 +++++++++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>> index 58881911e74a..16a9769f518d 100644
>> --- a/drivers/spi/spi-fsl-dspi.c
>> +++ b/drivers/spi/spi-fsl-dspi.c
>> @@ -560,12 +560,24 @@ static void dspi_rx_dma_callback(void *arg)
>>   	complete(&dma->cmd_rx_complete);
>>   }
>>
>> +static int dspi_fifo_error(struct fsl_dspi *dspi, u32 spi_sr)
>> +{
>> +	if (spi_sr & (SPI_SR_TFUF | SPI_SR_RFOF)) {
>> +		dev_err_ratelimited(&dspi->pdev->dev, "FIFO errors:%s%s\n",
>> +				    spi_sr & SPI_SR_TFUF ? " TX underflow," : "",
>> +				    spi_sr & SPI_SR_RFOF ? " RX overflow," : "");
>> +		return -EIO;
>> +	}
>> +	return 0;
>> +}
>> +
>>   static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>>   {
>>   	size_t size = dspi_dma_transfer_size(dspi);
>>   	struct device *dev = &dspi->pdev->dev;
>>   	struct fsl_dspi_dma *dma = dspi->dma;
>>   	int time_left;
>> +	u32 spi_sr;
>>   	int i;
>>
>>   	for (i = 0; i < dspi->words_in_flight; i++)
>> @@ -614,7 +626,8 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>>
>>   	if (spi_controller_is_target(dspi->ctlr)) {
>>   		wait_for_completion_interruptible(&dspi->dma->cmd_rx_complete);
>> -		return 0;
>> +		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>> +		return dspi_fifo_error(dspi, spi_sr);
>>   	}
>>
>>   	time_left = wait_for_completion_timeout(&dspi->dma->cmd_tx_complete,
>> @@ -1069,6 +1082,10 @@ static void dspi_poll(struct fsl_dspi *dspi)
>>
>>   			if (spi_sr & SPI_SR_CMDTCF)
>>   				break;
>> +
>> +			dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
>> +			if (dspi->cur_msg->status)
>> +				return;
> 
> 
> Although fifo error may happen after you check, it may reduce some possilbity
> and catch some problems.
> 
> Frak
> 

Not sure what you mean by this one. But I've seen a few small errors now 
that I look again. The error check should be before the transfer 
complete break. And tries should be reset for each part of the message.

>>   		} while (--tries);
>>
>>   		if (!tries) {
>> @@ -1085,6 +1102,7 @@ static void dspi_poll(struct fsl_dspi *dspi)
>>   static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>>   {
>>   	struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
>> +	int status;
>>   	u32 spi_sr;
>>
>>   	regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>> @@ -1093,6 +1111,14 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>>   	if (!(spi_sr & SPI_SR_CMDTCF))
>>   		return IRQ_NONE;
>>
>> +	status = dspi_fifo_error(dspi, spi_sr);
>> +	if (status) {
>> +		if (dspi->cur_msg)
>> +			WRITE_ONCE(dspi->cur_msg->status, status);
>> +		complete(&dspi->xfer_done);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>>   	dspi_rxtx(dspi);
>>
>>   	if (!dspi->len) {
>>
>> --
>> 2.34.1
>>

Re: [PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions

[James Clark]

On 25/06/2025 11:00 am, Arnd Bergmann wrote:
> On Wed, Jun 25, 2025, at 11:19, James Clark wrote:
>> On 24/06/2025 6:16 pm, Arnd Bergmann wrote:
>>> On Tue, Jun 24, 2025, at 18:29, Frank Li wrote:
>>
>>> It would also be simpler to enforce this in Kconfig if we only
>>> care about users that use the DMA support.
>>
>> But most of the devices supported by the driver don't do any DMA. That
>> was the reason to stub them out rather than add the Kconfig depends.
> 
> Ah right. So even when running on SoCs that have a DMA engine,
> you can end up not using it?
> 

Yes

> In this case you could have an extra Kconfig symbol to configure
> DMA support for this driver and use that in the source code:
> 
> config SPI_FSL_DSPI_DMA
>        bool "Use DMA engine for offloading Freescale DSPI transfers"
>        depends on SPI_FSL_DSPI && DMA_ENGINE
>        help
>           ....
> 
> 
>       Arnd

Wouldn't that allow someone to break it by disabling (or not enabling) 
that option? The driver won't fall back to the other modes if DMA isn't 
configured, it just won't work. In this case it seems like it's better 
to depend directly on DMA_ENGINE because that fixes the randconfig 
issues, which is the whole reason for the discussion.

Would someone really want an option to disable compilation of two 
functions if their DSPI device is one that doesn't use DMA? Seems like 
this option would always be on anyway.

Re: [PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions

[Arnd Bergmann]

On Wed, Jun 25, 2025, at 12:19, James Clark wrote:
> On 25/06/2025 11:00 am, Arnd Bergmann wrote:
>> On Wed, Jun 25, 2025, at 11:19, James Clark wrote:
>>> On 24/06/2025 6:16 pm, Arnd Bergmann wrote:
>
> Wouldn't that allow someone to break it by disabling (or not enabling) 
> that option? The driver won't fall back to the other modes if DMA isn't 
> configured, it just won't work. In this case it seems like it's better 
> to depend directly on DMA_ENGINE because that fixes the randconfig 
> issues, which is the whole reason for the discussion.

It would be the same as disabling DMA_ENGINE today when running on an
SoC that supports DMA mode in DSPI. Ideally that should fall back
to non-accelerated mode. I see a lot of checks for
trans_mode == DSPI_DMA_MODE, and I it's probably best to change
them to a function call like

static inline bool dsp_dma_mode(struct fsl_dspi *dspi)
{
     if (!IS_ENABLED(CONFIG_DMA_ENGINE)) // or CONFIG_FSL_DSPI_USE_DMA
          return false;

     return dspi->devtype_data->trans_mode == DSPI_DMA_MODE;
}

> Would someone really want an option to disable compilation of two 
> functions if their DSPI device is one that doesn't use DMA? Seems like 
> this option would always be on anyway.

It's probably mainly relevant if they want to completely turn off
CONFIG_DMA_ENGINE, which is substantially bigger. Using a check
for that symbol in the driver is certainly simpler for the user,
as they can't accidentally turn it off the custom symbol.

In theory you may also want to turn off DMA mode for testing,
which is supported by at least the DW_DMA driver.

I see that SPI_TEGRA114 has a dependency on TEGRA20_APB_DMA,
which is yet another variation. This is clearly done for
usability purposes since that SPI driver only ever works with
the specific DMA driver in practice, but it seems worse
conceptually.

     Arnd

Re: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors

[Frank Li]

On Wed, Jun 25, 2025 at 11:09:57AM +0100, James Clark wrote:
>
>
> On 24/06/2025 5:50 pm, Frank Li wrote:
> > On Tue, Jun 24, 2025 at 11:35:36AM +0100, James Clark wrote:
> > > In target mode, the host sending more data than can be consumed would be
> > > a common problem for any message exceeding the FIFO or DMA buffer size.
> > > Cancel the whole message as soon as this condition is hit as the message
> > > will be corrupted.
> > >
> > > Only do this for target mode in a DMA transfer because we need to add a
> > > register read.
> >
> > "We need to add a register read" is not reason.
> >
>
> Maybe: "It's not likely to catch any errors in host mode so optimize by
> avoiding an extra register read"?
>
> > Add checking FIFO error status at target mode in a DMA transfer since PIO
> > mode already do it. It help catch some host mode ...
> >
>
> Are you suggesting that we check for FIFO errors in host mode too? It
> requires an extra read and check in dspi_tx_dma_callback(), but I'm not sure
> what it could catch. Realistically as long as everything is setup correctly
> then neither of those flags will be set. It will either always work or never
> work.
>
> It might be better to add it later if a use becomes apparent otherwise it's
> extra noise in the code.

I think your origial last phrase is not good enough. You may rephrase it
to make it clear.

for example: according to your patch

"Only do this for target mode in a DMA transfer because we need to add a register read."

"add a register read" is result, not a reason. the reason should be "you want
host side capture such error."

Frank


>
> > > In IRQ and polling modes always do it because SPI_SR was
> > > already read and it might catch some host mode programming/buffer
> > > management errors too.
> > >
> > > Signed-off-by: James Clark <james.clark@linaro.org>
> > > ---
> > >   drivers/spi/spi-fsl-dspi.c | 28 +++++++++++++++++++++++++++-
> > >   1 file changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> > > index 58881911e74a..16a9769f518d 100644
> > > --- a/drivers/spi/spi-fsl-dspi.c
> > > +++ b/drivers/spi/spi-fsl-dspi.c
> > > @@ -560,12 +560,24 @@ static void dspi_rx_dma_callback(void *arg)
> > >   	complete(&dma->cmd_rx_complete);
> > >   }
> > >
> > > +static int dspi_fifo_error(struct fsl_dspi *dspi, u32 spi_sr)
> > > +{
> > > +	if (spi_sr & (SPI_SR_TFUF | SPI_SR_RFOF)) {
> > > +		dev_err_ratelimited(&dspi->pdev->dev, "FIFO errors:%s%s\n",
> > > +				    spi_sr & SPI_SR_TFUF ? " TX underflow," : "",
> > > +				    spi_sr & SPI_SR_RFOF ? " RX overflow," : "");
> > > +		return -EIO;
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > >   static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
> > >   {
> > >   	size_t size = dspi_dma_transfer_size(dspi);
> > >   	struct device *dev = &dspi->pdev->dev;
> > >   	struct fsl_dspi_dma *dma = dspi->dma;
> > >   	int time_left;
> > > +	u32 spi_sr;
> > >   	int i;
> > >
> > >   	for (i = 0; i < dspi->words_in_flight; i++)
> > > @@ -614,7 +626,8 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
> > >
> > >   	if (spi_controller_is_target(dspi->ctlr)) {
> > >   		wait_for_completion_interruptible(&dspi->dma->cmd_rx_complete);
> > > -		return 0;
> > > +		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> > > +		return dspi_fifo_error(dspi, spi_sr);
> > >   	}
> > >
> > >   	time_left = wait_for_completion_timeout(&dspi->dma->cmd_tx_complete,
> > > @@ -1069,6 +1082,10 @@ static void dspi_poll(struct fsl_dspi *dspi)
> > >
> > >   			if (spi_sr & SPI_SR_CMDTCF)
> > >   				break;
> > > +
> > > +			dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
> > > +			if (dspi->cur_msg->status)
> > > +				return;
> >
> >
> > Although fifo error may happen after you check, it may reduce some possilbity
> > and catch some problems.
> >
> > Frak
> >
>
> Not sure what you mean by this one. But I've seen a few small errors now
> that I look again. The error check should be before the transfer complete
> break. And tries should be reset for each part of the message.
>
> > >   		} while (--tries);
> > >
> > >   		if (!tries) {
> > > @@ -1085,6 +1102,7 @@ static void dspi_poll(struct fsl_dspi *dspi)
> > >   static irqreturn_t dspi_interrupt(int irq, void *dev_id)
> > >   {
> > >   	struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
> > > +	int status;
> > >   	u32 spi_sr;
> > >
> > >   	regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> > > @@ -1093,6 +1111,14 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
> > >   	if (!(spi_sr & SPI_SR_CMDTCF))
> > >   		return IRQ_NONE;
> > >
> > > +	status = dspi_fifo_error(dspi, spi_sr);
> > > +	if (status) {
> > > +		if (dspi->cur_msg)
> > > +			WRITE_ONCE(dspi->cur_msg->status, status);
> > > +		complete(&dspi->xfer_done);
> > > +		return IRQ_HANDLED;
> > > +	}
> > > +
> > >   	dspi_rxtx(dspi);
> > >
> > >   	if (!dspi->len) {
> > >
> > > --
> > > 2.34.1
> > >
>

Re: [PATCH v3 4/6] spi: spi-fsl-dspi: Use non-coherent memory for DMA

[Frank Li]

On Wed, Jun 25, 2025 at 10:00:41AM +0100, James Clark wrote:
>
>
> On 24/06/2025 5:39 pm, Frank Li wrote:
> > On Tue, Jun 24, 2025 at 11:35:34AM +0100, James Clark wrote:
> > > Using coherent memory here isn't functionally necessary. Because the
> > > change to use non-coherent memory isn't overly complex and only a few
> > > synchronization points are required, we might as well do it while fixing
> > > up some other DMA issues.
> > >
> > > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > > Signed-off-by: James Clark <james.clark@linaro.org>
> >
> > In https://lore.kernel.org/imx/c65c752a-5b60-4f30-8d51-9a903ddd55a6@linaro.org/
> >
> > look like less performance, why need this patch.
> >
> > In https://lore.kernel.org/imx/ad7e9aa7-74a3-449d-8ed9-cb270fd5c718@linaro.org/
> > look like better.
> >
> > Any conclusion?
> >
> > Need performance gain here if it is better.
> >
> > Frank
> >
>
> Hi Frank,
>
> The performance figures for this set are in the cover letter. It's either
> the same or faster, there is no evidence of worse performance. The one you
> linked was a bad result from not testing it in DMA mode, but it's corrected
> later in that thread.

Okay! you need mention why need this change, why non-coherent better than
coherent at your case.

You descript what you already done, but not descript why need it.

for example:

"fixing up some other DMA issues", what issues exactly?

some benefit, like memcpy from/to non-coherent is faster then from/to
coherenct memory ...

of put test data here will be appreciated.

The cover letter will be lost after patch merge. When someone run git log
after some year later, they need know why need this change , what purpose ...

Frank


>
> The reason the figures aren't in this commit is because it requires this
> change and the one to increase the size of the buffer.


>
> Thanks
> James
>

Re: [PATCH v3 4/6] spi: spi-fsl-dspi: Use non-coherent memory for DMA

[James Clark]

On 25/06/2025 4:04 pm, Frank Li wrote:
> On Wed, Jun 25, 2025 at 10:00:41AM +0100, James Clark wrote:
>>
>>
>> On 24/06/2025 5:39 pm, Frank Li wrote:
>>> On Tue, Jun 24, 2025 at 11:35:34AM +0100, James Clark wrote:
>>>> Using coherent memory here isn't functionally necessary. Because the
>>>> change to use non-coherent memory isn't overly complex and only a few
>>>> synchronization points are required, we might as well do it while fixing
>>>> up some other DMA issues.
>>>>
>>>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>>
>>> In https://lore.kernel.org/imx/c65c752a-5b60-4f30-8d51-9a903ddd55a6@linaro.org/
>>>
>>> look like less performance, why need this patch.
>>>
>>> In https://lore.kernel.org/imx/ad7e9aa7-74a3-449d-8ed9-cb270fd5c718@linaro.org/
>>> look like better.
>>>
>>> Any conclusion?
>>>
>>> Need performance gain here if it is better.
>>>
>>> Frank
>>>
>>
>> Hi Frank,
>>
>> The performance figures for this set are in the cover letter. It's either
>> the same or faster, there is no evidence of worse performance. The one you
>> linked was a bad result from not testing it in DMA mode, but it's corrected
>> later in that thread.
> 
> Okay! you need mention why need this change, why non-coherent better than
> coherent at your case.
> 
> You descript what you already done, but not descript why need it.
> 
> for example:
> 
> "fixing up some other DMA issues", what issues exactly?

I can remove that line, it might be more appropriate to add in the cover 
letter as it's relating to other commits in this set.

> 
> some benefit, like memcpy from/to non-coherent is faster then from/to
> coherenct memory ...
> 

Yes I can mention that it's expected to be and was measured to be 
faster. That should help people running git log in the future to see why 
we did it.

> of put test data here will be appreciated.
> 
> The cover letter will be lost after patch merge. When someone run git log
> after some year later, they need know why need this change , what purpose ...
> 
> Frank
> 

I somewhat disagree with this. Usually maintainers add a 'Link:' to the 
mailing list when applying patches, so the cover letter shouldn't be 
lost. And these particular performance test results are short lived, in 
several years time other things may have changed. The performance is 
related to a specific device and the state of the rest of the kernel at 
this time. Additionally, I mentioned that it's the combination of two 
commits. In order to put figures on this commit message I would have to 
run another set of tests with only this commit and not the one to 
increase the buffer size which comes after. I did consider reversing the 
order of them to do this, but it wasn't straightforward, and I really 
didn't think it was worth the effort when I can just put the figures on 
the cover letter.

We only need the figures to judge whether to merge it right now, readers 
in the future will want to read the commit message to see what was done 
and why. I'm sure that they can trust that we measured some improvement 
if for some reason the cover letter is lost.

It's very common in the kernel to put figures relating to an entire 
patchset on the cover letter of it, rather than on the last commit message.

> 
>>
>> The reason the figures aren't in this commit is because it requires this
>> change and the one to increase the size of the buffer.
> 
> 
>>
>> Thanks
>> James
>>

Re: [PATCH v3 4/6] spi: spi-fsl-dspi: Use non-coherent memory for DMA

[Arnd Bergmann]

On Thu, Jun 26, 2025, at 11:14, James Clark wrote:
> On 25/06/2025 4:04 pm, Frank Li wrote:
>>
>> The cover letter will be lost after patch merge. When someone run git log
>> after some year later, they need know why need this change , what purpose ...
>> 
> I somewhat disagree with this. Usually maintainers add a 'Link:' to the 
> mailing list when applying patches, so the cover letter shouldn't be 
> lost. And these particular performance test results are short lived, in 
> several years time other things may have changed. The performance is 
> related to a specific device and the state of the rest of the kernel at 
> this time. Additionally, I mentioned that it's the combination of two 
> commits. In order to put figures on this commit message I would have to 
> run another set of tests with only this commit and not the one to 
> increase the buffer size which comes after. I did consider reversing the 
> order of them to do this, but it wasn't straightforward, and I really 
> didn't think it was worth the effort when I can just put the figures on 
> the cover letter.

If you submit your changes as a pull request for inclusion, the
common solution is to put the cover letter into the signed tag,
which then gets used as both the description in 'git request-pull'
and in the merge commit.

See e.g. commit 5b31d2d81a4b ("spi: sh-msiof: Transfer size
improvements and I2S") for how a similar series on another driver
shows up in the git log.

    Arnd

Re: [PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions

[James Clark]

On 25/06/2025 11:54 am, Arnd Bergmann wrote:
> On Wed, Jun 25, 2025, at 12:19, James Clark wrote:
>> On 25/06/2025 11:00 am, Arnd Bergmann wrote:
>>> On Wed, Jun 25, 2025, at 11:19, James Clark wrote:
>>>> On 24/06/2025 6:16 pm, Arnd Bergmann wrote:
>>
>> Wouldn't that allow someone to break it by disabling (or not enabling)
>> that option? The driver won't fall back to the other modes if DMA isn't
>> configured, it just won't work. In this case it seems like it's better
>> to depend directly on DMA_ENGINE because that fixes the randconfig
>> issues, which is the whole reason for the discussion.
> 
> It would be the same as disabling DMA_ENGINE today when running on an
> SoC that supports DMA mode in DSPI. Ideally that should fall back
> to non-accelerated mode. I see a lot of checks for
> trans_mode == DSPI_DMA_MODE, and I it's probably best to change
> them to a function call like
> 
> static inline bool dsp_dma_mode(struct fsl_dspi *dspi)
> {
>       if (!IS_ENABLED(CONFIG_DMA_ENGINE)) // or CONFIG_FSL_DSPI_USE_DMA
>            return false;
> 
>       return dspi->devtype_data->trans_mode == DSPI_DMA_MODE;
> }
> 

I think we'd have to check with Vladimir on that. He said that he didn't 
change some devices that use DMA to use XSPI because he couldn't verify 
that they would still work [1]. So by adding this fallback we would be 
making that decision now, which I'm not sure we can do. It would 
probably be better to add the stubs that make the probe fail rather than 
register a driver with a fallback that we don't know works.

>> Would someone really want an option to disable compilation of two
>> functions if their DSPI device is one that doesn't use DMA? Seems like
>> this option would always be on anyway.
> 
> It's probably mainly relevant if they want to completely turn off
> CONFIG_DMA_ENGINE, which is substantially bigger. Using a check
> for that symbol in the driver is certainly simpler for the user,
> as they can't accidentally turn it off the custom symbol.

That was my thinking.

> 
> In theory you may also want to turn off DMA mode for testing,
> which is supported by at least the DW_DMA driver.
> 
> I see that SPI_TEGRA114 has a dependency on TEGRA20_APB_DMA,
> which is yet another variation. This is clearly done for
> usability purposes since that SPI driver only ever works with
> the specific DMA driver in practice, but it seems worse
> conceptually.
> 
>       Arnd

I can add it, I'm just questioning the usefulness of adding a new 
config option that's unlikely to be used in reality. It only achieves 
the same thing as doing #if CONFIG_DMA_ENGINE in one place in the code.

Ultimately we're just trying to fix some randconfig error, if we start 
adding new knobs for people to turn just do that, there's a risk they 
won't get used and we'll make configuring more complicated for people.

Testing might be a partial justification, but that config wouldn't allow 
you to force DMA mode where XSPI is used. You would almost certainly be 
doing that too if you are doing that kind of testing, and you'd have to 
hack the driver anyway to do that.

[1]: https://lkml.org/lkml/2025/6/12/1385

Re: [PATCH v3 4/6] spi: spi-fsl-dspi: Use non-coherent memory for DMA

[Mark Brown]

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

On Thu, Jun 26, 2025 at 11:38:46AM +0200, Arnd Bergmann wrote:

> If you submit your changes as a pull request for inclusion, the
> common solution is to put the cover letter into the signed tag,
> which then gets used as both the description in 'git request-pull'
> and in the merge commit.

That does rely on the maintainer taking the changes as a pull request
which you probably shouldn't expect without some prior discussion.

> See e.g. commit 5b31d2d81a4b ("spi: sh-msiof: Transfer size
> improvements and I2S") for how a similar series on another driver
> shows up in the git log.

That's not a pull request - for my CI I generally apply serieses on a
separate branch then if they pass merge them into my branch.  If they
need a merge commit I use the cover letter to generate content for that.

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

Re: [PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions

[Mark Brown]

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

On Thu, Jun 26, 2025 at 11:04:50AM +0100, James Clark wrote:
> On 25/06/2025 11:54 am, Arnd Bergmann wrote:

> > In theory you may also want to turn off DMA mode for testing,
> > which is supported by at least the DW_DMA driver.

> > I see that SPI_TEGRA114 has a dependency on TEGRA20_APB_DMA,
> > which is yet another variation. This is clearly done for
> > usability purposes since that SPI driver only ever works with
> > the specific DMA driver in practice, but it seems worse
> > conceptually.

> I can add it, I'm just questioning the usefulness of adding a new config
> option that's unlikely to be used in reality. It only achieves the same
> thing as doing #if CONFIG_DMA_ENGINE in one place in the code.

> Ultimately we're just trying to fix some randconfig error, if we start
> adding new knobs for people to turn just do that, there's a risk they won't
> get used and we'll make configuring more complicated for people.

Please just keep things simple, as you say nobody is likely to be
actually trying to run such a configuration practically.

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

Re: [PATCH v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors

[James Clark]

On 25/06/2025 3:55 pm, Frank Li wrote:
> On Wed, Jun 25, 2025 at 11:09:57AM +0100, James Clark wrote:
>>
>>
>> On 24/06/2025 5:50 pm, Frank Li wrote:
>>> On Tue, Jun 24, 2025 at 11:35:36AM +0100, James Clark wrote:
>>>> In target mode, the host sending more data than can be consumed would be
>>>> a common problem for any message exceeding the FIFO or DMA buffer size.
>>>> Cancel the whole message as soon as this condition is hit as the message
>>>> will be corrupted.
>>>>
>>>> Only do this for target mode in a DMA transfer because we need to add a
>>>> register read.
>>>
>>> "We need to add a register read" is not reason.
>>>
>>
>> Maybe: "It's not likely to catch any errors in host mode so optimize by
>> avoiding an extra register read"?
>>
>>> Add checking FIFO error status at target mode in a DMA transfer since PIO
>>> mode already do it. It help catch some host mode ...
>>>
>>
>> Are you suggesting that we check for FIFO errors in host mode too? It
>> requires an extra read and check in dspi_tx_dma_callback(), but I'm not sure
>> what it could catch. Realistically as long as everything is setup correctly
>> then neither of those flags will be set. It will either always work or never
>> work.
>>
>> It might be better to add it later if a use becomes apparent otherwise it's
>> extra noise in the code.
> 
> I think your origial last phrase is not good enough. You may rephrase it
> to make it clear.
> 
> for example: according to your patch
> 
> "Only do this for target mode in a DMA transfer because we need to add a register read."
> 
> "add a register read" is result, not a reason. the reason should be "you want
> host side capture such error."
> 
> Frank
> 
> 

Got it, thanks

>>
>>>> In IRQ and polling modes always do it because SPI_SR was
>>>> already read and it might catch some host mode programming/buffer
>>>> management errors too.
>>>>
>>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>>> ---
>>>>    drivers/spi/spi-fsl-dspi.c | 28 +++++++++++++++++++++++++++-
>>>>    1 file changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>>>> index 58881911e74a..16a9769f518d 100644
>>>> --- a/drivers/spi/spi-fsl-dspi.c
>>>> +++ b/drivers/spi/spi-fsl-dspi.c
>>>> @@ -560,12 +560,24 @@ static void dspi_rx_dma_callback(void *arg)
>>>>    	complete(&dma->cmd_rx_complete);
>>>>    }
>>>>
>>>> +static int dspi_fifo_error(struct fsl_dspi *dspi, u32 spi_sr)
>>>> +{
>>>> +	if (spi_sr & (SPI_SR_TFUF | SPI_SR_RFOF)) {
>>>> +		dev_err_ratelimited(&dspi->pdev->dev, "FIFO errors:%s%s\n",
>>>> +				    spi_sr & SPI_SR_TFUF ? " TX underflow," : "",
>>>> +				    spi_sr & SPI_SR_RFOF ? " RX overflow," : "");
>>>> +		return -EIO;
>>>> +	}
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>>>>    {
>>>>    	size_t size = dspi_dma_transfer_size(dspi);
>>>>    	struct device *dev = &dspi->pdev->dev;
>>>>    	struct fsl_dspi_dma *dma = dspi->dma;
>>>>    	int time_left;
>>>> +	u32 spi_sr;
>>>>    	int i;
>>>>
>>>>    	for (i = 0; i < dspi->words_in_flight; i++)
>>>> @@ -614,7 +626,8 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>>>>
>>>>    	if (spi_controller_is_target(dspi->ctlr)) {
>>>>    		wait_for_completion_interruptible(&dspi->dma->cmd_rx_complete);
>>>> -		return 0;
>>>> +		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>>>> +		return dspi_fifo_error(dspi, spi_sr);
>>>>    	}
>>>>
>>>>    	time_left = wait_for_completion_timeout(&dspi->dma->cmd_tx_complete,
>>>> @@ -1069,6 +1082,10 @@ static void dspi_poll(struct fsl_dspi *dspi)
>>>>
>>>>    			if (spi_sr & SPI_SR_CMDTCF)
>>>>    				break;
>>>> +
>>>> +			dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
>>>> +			if (dspi->cur_msg->status)
>>>> +				return;
>>>
>>>
>>> Although fifo error may happen after you check, it may reduce some possilbity
>>> and catch some problems.
>>>
>>> Frak
>>>
>>
>> Not sure what you mean by this one. But I've seen a few small errors now
>> that I look again. The error check should be before the transfer complete
>> break. And tries should be reset for each part of the message.
>>
>>>>    		} while (--tries);
>>>>
>>>>    		if (!tries) {
>>>> @@ -1085,6 +1102,7 @@ static void dspi_poll(struct fsl_dspi *dspi)
>>>>    static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>>>>    {
>>>>    	struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
>>>> +	int status;
>>>>    	u32 spi_sr;
>>>>
>>>>    	regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>>>> @@ -1093,6 +1111,14 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>>>>    	if (!(spi_sr & SPI_SR_CMDTCF))
>>>>    		return IRQ_NONE;
>>>>
>>>> +	status = dspi_fifo_error(dspi, spi_sr);
>>>> +	if (status) {
>>>> +		if (dspi->cur_msg)
>>>> +			WRITE_ONCE(dspi->cur_msg->status, status);
>>>> +		complete(&dspi->xfer_done);
>>>> +		return IRQ_HANDLED;
>>>> +	}
>>>> +
>>>>    	dspi_rxtx(dspi);
>>>>
>>>>    	if (!dspi->len) {
>>>>
>>>> --
>>>> 2.34.1
>>>>
>>