[PATCH v2 0/5] spi: spi-fsl-dspi: Target mode improvements

[PATCH v2 0/5] 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 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 (4):
      spi: spi-fsl-dspi: Clear completion counter before initiating transfer
      spi: spi-fsl-dspi: Use non-coherent memory for DMA
      spi: spi-fsl-dspi: Store status directly in cur_msg->status
      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 | 203 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 144 insertions(+), 59 deletions(-)
---
base-commit: 3adf5ba9ad767e33db2d6aab01bbca396bcb614b
change-id: 20250522-james-nxp-spi-dma-a997ebebfb6b

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

[PATCH v2 1/5] 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 v2 2/5] 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 | 55 +++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 744dfc561db2..f19404e10c92 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -379,6 +379,11 @@ static bool is_s32g_dspi(struct fsl_dspi *data)
 	       data->devtype_data == &devtype_data[S32G_TARGET];
 }
 
+static int dspi_dma_transfer_size(struct fsl_dspi *dspi)
+{
+	return dspi->words_in_flight * DMA_SLAVE_BUSWIDTH_4_BYTES;
+}
+
 static void dspi_native_host_to_dev(struct fsl_dspi *dspi, u32 *txdata)
 {
 	switch (dspi->oper_word_size) {
@@ -493,7 +498,10 @@ 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);
 }
 
@@ -501,9 +509,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]);
 	}
@@ -513,6 +525,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;
@@ -521,10 +534,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) {
@@ -539,10 +551,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) {
@@ -644,17 +656,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;
@@ -689,11 +701,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:
@@ -714,14 +727,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 v2 3/5] 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 f19404e10c92..48c2ebefcd4a 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -384,6 +384,39 @@ static int dspi_dma_transfer_size(struct fsl_dspi *dspi)
 	return dspi->words_in_flight * DMA_SLAVE_BUSWIDTH_4_BYTES;
 }
 
+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 void dspi_native_host_to_dev(struct fsl_dspi *dspi, u32 *txdata)
 {
 	switch (dspi->oper_word_size) {
@@ -606,6 +639,7 @@ static void dspi_setup_accel(struct fsl_dspi *dspi);
 static int 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;
 	int ret = 0;
 
@@ -618,8 +652,8 @@ static int 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;
@@ -636,7 +670,7 @@ static int 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;
@@ -720,7 +754,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 v2 4/5] 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 48c2ebefcd4a..31432d533dea 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -636,12 +636,11 @@ 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;
 	int max_words = dspi_dma_max_datawords(dspi);
 	struct device *dev = &dspi->pdev->dev;
-	int ret = 0;
 
 	/*
 	 * dspi->len gets decremented by dspi_pop_tx_pushr in
@@ -658,14 +657,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)
@@ -1035,36 +1032,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)
@@ -1078,8 +1079,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;
 }
@@ -1109,7 +1115,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;
 
@@ -1169,7 +1174,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
@@ -1182,15 +1187,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);
@@ -1199,7 +1201,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);
@@ -1208,10 +1211,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 v2 5/5] 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 31432d533dea..f62f99f272b2 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -556,12 +556,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++)
@@ -610,7 +622,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,
@@ -1055,6 +1068,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) {
@@ -1071,6 +1088,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);
@@ -1079,6 +1097,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 v2 2/5] spi: spi-fsl-dspi: Use non-coherent memory for DMA

[kernel test robot]

Hi James,

kernel test robot noticed the following build errors:

[auto build test ERROR on 3adf5ba9ad767e33db2d6aab01bbca396bcb614b]

url:    https://github.com/intel-lab-lkp/linux/commits/James-Clark/spi-spi-fsl-dspi-Clear-completion-counter-before-initiating-transfer/20250613-173429
base:   3adf5ba9ad767e33db2d6aab01bbca396bcb614b
patch link:    https://lore.kernel.org/r/20250613-james-nxp-spi-dma-v2-2-017eecf24aab%40linaro.org
patch subject: [PATCH v2 2/5] spi: spi-fsl-dspi: Use non-coherent memory for DMA
config: m68k-randconfig-r113-20250615 (https://download.01.org/0day-ci/archive/20250616/202506160036.t9VDxF6p-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.3.0
reproduce: (https://download.01.org/0day-ci/archive/20250616/202506160036.t9VDxF6p-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/202506160036.t9VDxF6p-lkp@intel.com/

All errors (new ones prefixed by >>):

   m68k-linux-ld: drivers/spi/spi-fsl-dspi.o: in function `dspi_release_dma.isra.0':
>> spi-fsl-dspi.c:(.text+0x644): undefined reference to `dma_free_pages'
>> m68k-linux-ld: spi-fsl-dspi.c:(.text+0x67a): undefined reference to `dma_free_pages'

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

[PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[James Clark]

The implementations are in mapping.c which requires HAS_DMA so stub them
out if not present. This is required for some drivers to pass randconfig
builds.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202506160036.t9VDxF6p-lkp@intel.com/
Signed-off-by: James Clark <james.clark@linaro.org>
---
 include/linux/dma-mapping.h | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 55c03e5fe8cb..766f28a0e11f 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -161,6 +161,12 @@ void *dma_vmap_noncontiguous(struct device *dev, size_t size,
 void dma_vunmap_noncontiguous(struct device *dev, void *vaddr);
 int dma_mmap_noncontiguous(struct device *dev, struct vm_area_struct *vma,
 		size_t size, struct sg_table *sgt);
+struct page *dma_alloc_pages(struct device *dev, size_t size,
+		dma_addr_t *dma_handle, enum dma_data_direction dir, gfp_t gfp);
+void dma_free_pages(struct device *dev, size_t size, struct page *page,
+		dma_addr_t dma_handle, enum dma_data_direction dir);
+int dma_mmap_pages(struct device *dev, struct vm_area_struct *vma,
+		size_t size, struct page *page);
 #else /* CONFIG_HAS_DMA */
 static inline dma_addr_t dma_map_page_attrs(struct device *dev,
 		struct page *page, size_t offset, size_t size,
@@ -291,6 +297,21 @@ static inline int dma_mmap_noncontiguous(struct device *dev,
 {
 	return -EINVAL;
 }
+static inline struct page *dma_alloc_pages(struct device *dev, size_t size,
+		dma_addr_t *dma_handle, enum dma_data_direction dir, gfp_t gfp)
+{
+	return NULL;
+}
+static inline void dma_free_pages(struct device *dev, size_t size,
+		struct page *page, dma_addr_t dma_handle,
+		enum dma_data_direction dir)
+{
+}
+static inline int dma_mmap_pages(struct device *dev, struct vm_area_struct *vma,
+				 size_t size, struct page *page)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_HAS_DMA */
 
 #ifdef CONFIG_IOMMU_DMA
@@ -438,13 +459,6 @@ static inline bool dma_need_unmap(struct device *dev)
 }
 #endif /* !CONFIG_HAS_DMA || !CONFIG_DMA_NEED_SYNC */
 
-struct page *dma_alloc_pages(struct device *dev, size_t size,
-		dma_addr_t *dma_handle, enum dma_data_direction dir, gfp_t gfp);
-void dma_free_pages(struct device *dev, size_t size, struct page *page,
-		dma_addr_t dma_handle, enum dma_data_direction dir);
-int dma_mmap_pages(struct device *dev, struct vm_area_struct *vma,
-		size_t size, struct page *page);
-
 static inline void *dma_alloc_noncoherent(struct device *dev, size_t size,
 		dma_addr_t *dma_handle, enum dma_data_direction dir, gfp_t gfp)
 {
-- 
2.34.1

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[James Clark]

On 16/06/2025 12:17 pm, James Clark wrote:
> The implementations are in mapping.c which requires HAS_DMA so stub them
> out if not present. This is required for some drivers to pass randconfig
> builds.
> 

So the commit message makes it seem like this fixes an existing issue 
and it would need a fixes: tag. But I didn't add one because it seems 
like it would only hit new users like in the linked patchset. It might 
be better to add a tag but it was changed 5 years ago and nobody hit it 
until now so I'm not sure.

> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202506160036.t9VDxF6p-lkp@intel.com/
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>   include/linux/dma-mapping.h | 28 +++++++++++++++++++++-------
>   1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 55c03e5fe8cb..766f28a0e11f 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -161,6 +161,12 @@ void *dma_vmap_noncontiguous(struct device *dev, size_t size,
>   void dma_vunmap_noncontiguous(struct device *dev, void *vaddr);
>   int dma_mmap_noncontiguous(struct device *dev, struct vm_area_struct *vma,
>   		size_t size, struct sg_table *sgt);
> +struct page *dma_alloc_pages(struct device *dev, size_t size,
> +		dma_addr_t *dma_handle, enum dma_data_direction dir, gfp_t gfp);
> +void dma_free_pages(struct device *dev, size_t size, struct page *page,
> +		dma_addr_t dma_handle, enum dma_data_direction dir);
> +int dma_mmap_pages(struct device *dev, struct vm_area_struct *vma,
> +		size_t size, struct page *page);
>   #else /* CONFIG_HAS_DMA */
>   static inline dma_addr_t dma_map_page_attrs(struct device *dev,
>   		struct page *page, size_t offset, size_t size,
> @@ -291,6 +297,21 @@ static inline int dma_mmap_noncontiguous(struct device *dev,
>   {
>   	return -EINVAL;
>   }
> +static inline struct page *dma_alloc_pages(struct device *dev, size_t size,
> +		dma_addr_t *dma_handle, enum dma_data_direction dir, gfp_t gfp)
> +{
> +	return NULL;
> +}
> +static inline void dma_free_pages(struct device *dev, size_t size,
> +		struct page *page, dma_addr_t dma_handle,
> +		enum dma_data_direction dir)
> +{
> +}
> +static inline int dma_mmap_pages(struct device *dev, struct vm_area_struct *vma,
> +				 size_t size, struct page *page)
> +{
> +	return -EINVAL;
> +}
>   #endif /* CONFIG_HAS_DMA */
>   
>   #ifdef CONFIG_IOMMU_DMA
> @@ -438,13 +459,6 @@ static inline bool dma_need_unmap(struct device *dev)
>   }
>   #endif /* !CONFIG_HAS_DMA || !CONFIG_DMA_NEED_SYNC */
>   
> -struct page *dma_alloc_pages(struct device *dev, size_t size,
> -		dma_addr_t *dma_handle, enum dma_data_direction dir, gfp_t gfp);
> -void dma_free_pages(struct device *dev, size_t size, struct page *page,
> -		dma_addr_t dma_handle, enum dma_data_direction dir);
> -int dma_mmap_pages(struct device *dev, struct vm_area_struct *vma,
> -		size_t size, struct page *page);
> -
>   static inline void *dma_alloc_noncoherent(struct device *dev, size_t size,
>   		dma_addr_t *dma_handle, enum dma_data_direction dir, gfp_t gfp)
>   {

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Arnd Bergmann]

On Mon, Jun 16, 2025, at 13:17, James Clark wrote:
> The implementations are in mapping.c which requires HAS_DMA so stub them
> out if not present. This is required for some drivers to pass randconfig
> builds.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: 
> https://lore.kernel.org/oe-kbuild-all/202506160036.t9VDxF6p-lkp@intel.com/
> Signed-off-by: James Clark <james.clark@linaro.org>

Looks good to me

Acked-by: Arnd Bergmann <arnd@arndb.de>

It may be worth adding here that HAS_DMA is set on almost
all configurations, this only showed up as an error in
a randconfig build for m68k/dragonball, which is barely
supported at all.

The other two architectures without DMA support are sh-nommu
and uml.

     Arnd

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Christoph Hellwig]

On Mon, Jun 16, 2025 at 12:17:49PM +0100, James Clark wrote:
> The implementations are in mapping.c which requires HAS_DMA so stub them
> out if not present. This is required for some drivers to pass randconfig
> builds.

No.  Just add the proper IS_ENABLED checks in the callers.  While these
kinds of stubs used to be popular they are really nasty in that the
calls unexpectedly just fail without the right depends.

Re: [PATCH v2 2/5] spi: spi-fsl-dspi: Use non-coherent memory for DMA

[Robin Murphy]

On 2025-06-13 10:28 am, 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.

If it doesn't need coherent memory then does the driver really need to 
do its own bounce-buffering at all? Could you not simplify the whole lot 
even more by getting rid of {tx,rx}_dma_buf altogether and relying on 
the SPI core helpers to DMA-map the messages in-place?

Thanks,
Robin.

> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>   drivers/spi/spi-fsl-dspi.c | 55 +++++++++++++++++++++++++++++-----------------
>   1 file changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 744dfc561db2..f19404e10c92 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -379,6 +379,11 @@ static bool is_s32g_dspi(struct fsl_dspi *data)
>   	       data->devtype_data == &devtype_data[S32G_TARGET];
>   }
>   
> +static int dspi_dma_transfer_size(struct fsl_dspi *dspi)
> +{
> +	return dspi->words_in_flight * DMA_SLAVE_BUSWIDTH_4_BYTES;
> +}
> +
>   static void dspi_native_host_to_dev(struct fsl_dspi *dspi, u32 *txdata)
>   {
>   	switch (dspi->oper_word_size) {
> @@ -493,7 +498,10 @@ 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);
>   }
>   
> @@ -501,9 +509,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]);
>   	}
> @@ -513,6 +525,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;
> @@ -521,10 +534,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) {
> @@ -539,10 +551,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) {
> @@ -644,17 +656,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;
> @@ -689,11 +701,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:
> @@ -714,14 +727,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);
>   	}
>   }
> 

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Mark Brown]

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

On Mon, Jun 16, 2025 at 01:29:27PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 16, 2025 at 12:17:49PM +0100, James Clark wrote:

> > The implementations are in mapping.c which requires HAS_DMA so stub them
> > out if not present. This is required for some drivers to pass randconfig
> > builds.

> No.  Just add the proper IS_ENABLED checks in the callers.  While these
> kinds of stubs used to be popular they are really nasty in that the
> calls unexpectedly just fail without the right depends.

The issue with HAS_DMA is that essentially all platforms have and rely
on DMA.  This ends up just being painful noise from the buildbots when
they do randconfigs rather than something useful.

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

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Christoph Hellwig]

On Mon, Jun 16, 2025 at 01:06:00PM +0100, Mark Brown wrote:
> On Mon, Jun 16, 2025 at 01:29:27PM +0200, Christoph Hellwig wrote:
> > On Mon, Jun 16, 2025 at 12:17:49PM +0100, James Clark wrote:
> 
> > > The implementations are in mapping.c which requires HAS_DMA so stub them
> > > out if not present. This is required for some drivers to pass randconfig
> > > builds.
> 
> > No.  Just add the proper IS_ENABLED checks in the callers.  While these
> > kinds of stubs used to be popular they are really nasty in that the
> > calls unexpectedly just fail without the right depends.
> 
> The issue with HAS_DMA is that essentially all platforms have and rely
> on DMA.  This ends up just being painful noise from the buildbots when
> they do randconfigs rather than something useful.

In most case the driver really does depend on DMA to work, so just
depend on HAS_DMA.  If it can work without DMA, you can use IS_ENABLED.

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Mark Brown]

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

On Mon, Jun 16, 2025 at 02:08:32PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 16, 2025 at 01:06:00PM +0100, Mark Brown wrote:

> > The issue with HAS_DMA is that essentially all platforms have and rely
> > on DMA.  This ends up just being painful noise from the buildbots when
> > they do randconfigs rather than something useful.

> In most case the driver really does depend on DMA to work, so just
> depend on HAS_DMA.  If it can work without DMA, you can use IS_ENABLED.

Right, so you just end up with essentially every driver that isn't
already tied to a platform that needs DMA needing to add the dependency
which nobody is going to notice without doing build testing for
randconfigs or similar non-useful configs - it's not a productive use of
time.

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

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Christoph Hellwig]

On Mon, Jun 16, 2025 at 01:11:49PM +0100, Mark Brown wrote:
> already tied to a platform that needs DMA needing to add the dependency
> which nobody is going to notice without doing build testing for
> randconfigs or similar non-useful configs - it's not a productive use of
> time.

Stop your unproductive whining and just fix your dependencies.

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Mark Brown]

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

On Mon, Jun 16, 2025 at 02:14:44PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 16, 2025 at 01:11:49PM +0100, Mark Brown wrote:
> > already tied to a platform that needs DMA needing to add the dependency
> > which nobody is going to notice without doing build testing for
> > randconfigs or similar non-useful configs - it's not a productive use of
> > time.

> Stop your unproductive whining and just fix your dependencies.

That is not a constructive way to talk to people, especially not when
misdirected.

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

Re: [PATCH v2 2/5] spi: spi-fsl-dspi: Use non-coherent memory for DMA

[James Clark]

On 16/06/2025 12:56 pm, Robin Murphy wrote:
> On 2025-06-13 10:28 am, 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.
> 
> If it doesn't need coherent memory then does the driver really need to 
> do its own bounce-buffering at all? Could you not simplify the whole lot 
> even more by getting rid of {tx,rx}_dma_buf altogether and relying on 
> the SPI core helpers to DMA-map the messages in-place?
> 
> Thanks,
> Robin.
> 

In this case the device needs a control word to be written into the 
buffer for every SPI word to be sent, so it didn't fit into what was in 
the core layer. But we did look into doing it that way.

James

>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-dspi.c | 55 ++++++++++++++++++++++++++++ 
>> +-----------------
>>   1 file changed, 35 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>> index 744dfc561db2..f19404e10c92 100644
>> --- a/drivers/spi/spi-fsl-dspi.c
>> +++ b/drivers/spi/spi-fsl-dspi.c
>> @@ -379,6 +379,11 @@ static bool is_s32g_dspi(struct fsl_dspi *data)
>>              data->devtype_data == &devtype_data[S32G_TARGET];
>>   }
>> +static int dspi_dma_transfer_size(struct fsl_dspi *dspi)
>> +{
>> +    return dspi->words_in_flight * DMA_SLAVE_BUSWIDTH_4_BYTES;
>> +}
>> +
>>   static void dspi_native_host_to_dev(struct fsl_dspi *dspi, u32 *txdata)
>>   {
>>       switch (dspi->oper_word_size) {
>> @@ -493,7 +498,10 @@ 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);
>>   }
>> @@ -501,9 +509,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]);
>>       }
>> @@ -513,6 +525,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;
>> @@ -521,10 +534,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) {
>> @@ -539,10 +551,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) {
>> @@ -644,17 +656,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;
>> @@ -689,11 +701,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:
>> @@ -714,14 +727,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);
>>       }
>>   }
>>
> 

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[James Clark]

On 16/06/2025 1:14 pm, Christoph Hellwig wrote:
> On Mon, Jun 16, 2025 at 01:11:49PM +0100, Mark Brown wrote:
>> already tied to a platform that needs DMA needing to add the dependency
>> which nobody is going to notice without doing build testing for
>> randconfigs or similar non-useful configs - it's not a productive use of
>> time.
> 
> Stop your unproductive whining and just fix your dependencies.

The change introduces consistency with the existing declarations in 
dma-mapping.h. Surely there is value in consistency and it doesn't do 
any harm to define new ones with stubs the same as the other ones. That 
way when you change an existing device that has DMA stuff to use a new 
part of the API you don't have to predict that it will behave 
differently to another part of the API.

I suppose it is possible to #ifdef out the DMA stuff in this driver, but 
IMO it would be quite messy, and I don't think randomly not stubbing out 
some functions is the right way to move towards fixing all the 
dependencies in all drivers. We should continue with the stubs for now 
and fix whole drivers one by one as a proper effort.

Thanks
James

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Christoph Hellwig]

On Mon, Jun 16, 2025 at 02:05:22PM +0100, Mark Brown wrote:
> On Mon, Jun 16, 2025 at 02:14:44PM +0200, Christoph Hellwig wrote:
> > On Mon, Jun 16, 2025 at 01:11:49PM +0100, Mark Brown wrote:
> > > already tied to a platform that needs DMA needing to add the dependency
> > > which nobody is going to notice without doing build testing for
> > > randconfigs or similar non-useful configs - it's not a productive use of
> > > time.
> 
> > Stop your unproductive whining and just fix your dependencies.
> 
> That is not a constructive way to talk to people, especially not when
> misdirected.

In case you didn't notice, I found your answer extremely unconstrutive
if not actively malicious to start with.

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Christoph Hellwig]

On Mon, Jun 16, 2025 at 02:10:40PM +0100, James Clark wrote:
> The change introduces consistency with the existing declarations in 
> dma-mapping.h. Surely there is value in consistency and it doesn't do any 
> harm to define new ones with stubs the same as the other ones. That way 
> when you change an existing device that has DMA stuff to use a new part of 
> the API you don't have to predict that it will behave differently to 
> another part of the API.

Well, redoing the rest would definitively be nice, but so far no one
has signed up to that.

> I suppose it is possible to #ifdef out the DMA stuff in this driver, but 
> IMO it would be quite messy, and I don't think randomly not stubbing out 
> some functions is the right way to move towards fixing all the dependencies 
> in all drivers. We should continue with the stubs for now and fix whole 
> drivers one by one as a proper effort.

Does the driver even work at all without DMA support?

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[James Clark]

On 16/06/2025 2:13 pm, Christoph Hellwig wrote:
> On Mon, Jun 16, 2025 at 02:10:40PM +0100, James Clark wrote:
>> The change introduces consistency with the existing declarations in
>> dma-mapping.h. Surely there is value in consistency and it doesn't do any
>> harm to define new ones with stubs the same as the other ones. That way
>> when you change an existing device that has DMA stuff to use a new part of
>> the API you don't have to predict that it will behave differently to
>> another part of the API.
> 
> Well, redoing the rest would definitively be nice, but so far no one
> has signed up to that.
> 
>> I suppose it is possible to #ifdef out the DMA stuff in this driver, but
>> IMO it would be quite messy, and I don't think randomly not stubbing out
>> some functions is the right way to move towards fixing all the dependencies
>> in all drivers. We should continue with the stubs for now and fix whole
>> drivers one by one as a proper effort.
> 
> Does the driver even work at all without DMA support?
> 

Yes it does, it has a few modes that don't require it. Presumably we 
can't just add a depends into the kconfig for all devices because they 
might not be using DMA.

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[James Clark]

On 16/06/2025 2:14 pm, James Clark wrote:
> 
> 
> On 16/06/2025 2:13 pm, Christoph Hellwig wrote:
>> On Mon, Jun 16, 2025 at 02:10:40PM +0100, James Clark wrote:
>>> The change introduces consistency with the existing declarations in
>>> dma-mapping.h. Surely there is value in consistency and it doesn't do 
>>> any
>>> harm to define new ones with stubs the same as the other ones. That way
>>> when you change an existing device that has DMA stuff to use a new 
>>> part of
>>> the API you don't have to predict that it will behave differently to
>>> another part of the API.
>>
>> Well, redoing the rest would definitively be nice, but so far no one
>> has signed up to that.
>>
>>> I suppose it is possible to #ifdef out the DMA stuff in this driver, but
>>> IMO it would be quite messy, and I don't think randomly not stubbing out
>>> some functions is the right way to move towards fixing all the 
>>> dependencies
>>> in all drivers. We should continue with the stubs for now and fix whole
>>> drivers one by one as a proper effort.
>>
>> Does the driver even work at all without DMA support?
>>
> 
> Yes it does, it has a few modes that don't require it. Presumably we 
> can't just add a depends into the kconfig for all devices because they 
> might not be using DMA.

*for all the different variants of spi-fsl-dpsi devices I mean

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Christoph Hellwig]

On Mon, Jun 16, 2025 at 02:15:56PM +0100, James Clark wrote:
>> Yes it does, it has a few modes that don't require it. Presumably we can't 
>> just add a depends into the kconfig for all devices because they might not 
>> be using DMA.
>
> *for all the different variants of spi-fsl-dpsi devices I mean

This is drivers/spi/spi-fsl-dspi.c?

Yes, looks like it is one of those rare devices supporting a DMA and
non-DMA mode.  But everything seems nicely guarded off using
"dspi->devtype_data->trans_mode == DSPI_DMA_MODE" checks there.  So
wrap them into a little helper using IS_ENABLED(CONFIG_HAS_DMA) and
everything should be sorted out.

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[James Clark]

On 16/06/2025 2:19 pm, Christoph Hellwig wrote:
> On Mon, Jun 16, 2025 at 02:15:56PM +0100, James Clark wrote:
>>> Yes it does, it has a few modes that don't require it. Presumably we can't
>>> just add a depends into the kconfig for all devices because they might not
>>> be using DMA.
>>
>> *for all the different variants of spi-fsl-dpsi devices I mean
> 
> This is drivers/spi/spi-fsl-dspi.c?
> 
> Yes, looks like it is one of those rare devices supporting a DMA and
> non-DMA mode.  But everything seems nicely guarded off using
> "dspi->devtype_data->trans_mode == DSPI_DMA_MODE" checks there.  So
> wrap them into a little helper using IS_ENABLED(CONFIG_HAS_DMA) and
> everything should be sorted out.

Sure, I don't mind doing it.

But separately to that, I still think making the stubs consistent would 
save people a lot of time diagnosing build failures if they switch 
existing code to any of those 3 functions. Principle of Least 
Astonishment and all that.

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Arnd Bergmann]

On Mon, Jun 16, 2025, at 15:23, James Clark wrote:
> On 16/06/2025 2:19 pm, Christoph Hellwig wrote:
>> On Mon, Jun 16, 2025 at 02:15:56PM +0100, James Clark wrote:
>>>> Yes it does, it has a few modes that don't require it. Presumably we can't
>>>> just add a depends into the kconfig for all devices because they might not
>>>> be using DMA.
>>>
>>> *for all the different variants of spi-fsl-dpsi devices I mean
>> 
>> This is drivers/spi/spi-fsl-dspi.c?
>> 
>> Yes, looks like it is one of those rare devices supporting a DMA and
>> non-DMA mode.  But everything seems nicely guarded off using
>> "dspi->devtype_data->trans_mode == DSPI_DMA_MODE" checks there.  So
>> wrap them into a little helper using IS_ENABLED(CONFIG_HAS_DMA) and
>> everything should be sorted out.
>
> Sure, I don't mind doing it.
>
> But separately to that, I still think making the stubs consistent would 
> save people a lot of time diagnosing build failures if they switch 
> existing code to any of those 3 functions. Principle of Least 
> Astonishment and all that.

As far as I can tell, the difference here is that the
dma_alloc_coherent()/dma_free_coherent() calls all get stubbed
out, so the 827 drivers using those can all build cleanly on
mk68knommu, shnommu and UML, while dma_alloc_noncoherent()/
dma_free_noncoherent() are only used on 15 files that are all
guarded by some other Kconfig dependency at the moment and won't
build on the those platforms.

I agree that it would be best to treat the coherent/noncoherent
cases the same, and I also think the existing stubs are a bit
silly, but just removing them would likely require fixing
hundreds of drivers with added Kconfig or IS_ENABLED() checks.

Maybe we can actually remove CONFIG_NO_DMA/CONFIG_HAS_DMA
entirely and remove all the checks for CONFIG_HAS_DMA? 
My guess is that this would only lead to a small code size
increase on the affected targets, but since they are not
actually trying to do DMA, and they all have a very limited
set of drivers they actually use, it won't break existing
code.

    Arnd

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Arnd Bergmann]

On Mon, Jun 16, 2025, at 15:48, Arnd Bergmann wrote:
> On Mon, Jun 16, 2025, at 15:23, James Clark wrote:
>> On 16/06/2025 2:19 pm, Christoph Hellwig wrote:
>
> Maybe we can actually remove CONFIG_NO_DMA/CONFIG_HAS_DMA
> entirely and remove all the checks for CONFIG_HAS_DMA? 
> My guess is that this would only lead to a small code size
> increase on the affected targets, but since they are not
> actually trying to do DMA, and they all have a very limited
> set of drivers they actually use, it won't break existing
> code.

I tried removing the stubs on sh/j2 allmodconfig and found this:

ERROR: modpost: "dma_unmap_page_attrs" [drivers/bus/mhi/host/mhi.ko] undefined!
ERROR: modpost: "dma_map_page_attrs" [drivers/bus/mhi/host/mhi.ko] undefined!
ERROR: modpost: "dma_alloc_attrs" [drivers/bus/mhi/host/mhi.ko] undefined!
ERROR: modpost: "dma_free_attrs" [drivers/bus/mhi/host/mhi.ko] undefined!
ERROR: modpost: "dma_unmap_page_attrs" [drivers/soc/mediatek/mtk-cmdq-helper.ko] undefined!
ERROR: modpost: "dma_map_page_attrs" [drivers/soc/mediatek/mtk-cmdq-helper.ko] undefined!
ERROR: modpost: "dma_unmap_page_attrs" [drivers/soc/qcom/qcom-geni-se.ko] undefined!
ERROR: modpost: "dma_map_page_attrs" [drivers/soc/qcom/qcom-geni-se.ko] undefined!
ERROR: modpost: "dma_set_coherent_mask" [drivers/soc/ti/pruss.ko] undefined!
ERROR: modpost: "dma_unmap_page_attrs" [drivers/virtio/virtio_ring.ko] undefined!
WARNING: modpost: suppressed 733 unresolved symbol warnings because there were too many)

Enabling HAS_DMA unconditionally all all the platforms builds:

--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -64,7 +64,6 @@ config SUPERH
        select LOCK_MM_AND_FIND_VMA
        select MODULES_USE_ELF_RELA
        select NEED_SG_DMA_LENGTH
-       select NO_DMA if !MMU && !DMA_COHERENT
        select NO_GENERIC_PCI_IOPORT_MAP if PCI
        select OLD_SIGACTION
        select OLD_SIGSUSPEND
@@ -134,7 +133,7 @@ config SWAP_IO_SPACE
        bool
 
 config DMA_COHERENT
-       bool
+       def_bool !MMU
 
 config DMA_NONCOHERENT
        def_bool !NO_DMA && !DMA_COHERENT
--- a/arch/um/Kconfig
+++ b/arch/um/Kconfig
@@ -22,7 +22,6 @@ config UML
        select HAVE_DEBUG_KMEMLEAK
        select HAVE_DEBUG_BUGVERBOSE
        select HAVE_PAGE_SIZE_4KB
-       select NO_DMA if !UML_DMA_EMULATION
        select OF_EARLY_FLATTREE if OF
        select GENERIC_IRQ_SHOW
        select GENERIC_CPU_DEVICES
--- a/arch/m68k/Kconfig
+++ b/arch/m68k/Kconfig
@@ -37,7 +37,6 @@ config M68K
        select MMU_GATHER_NO_RANGE if MMU
        select MODULES_USE_ELF_REL
        select MODULES_USE_ELF_RELA
-       select NO_DMA if !MMU && !COLDFIRE
        select OLD_SIGACTION
        select OLD_SIGSUSPEND3
        select UACCESS_MEMCPY if !MMU
diff --git a/arch/m68k/Kconfig.cpu b/arch/m68k/Kconfig.cpu
index c9a7e602d8a4..a1123e2fecdf 100644
--- a/arch/m68k/Kconfig.cpu
+++ b/arch/m68k/Kconfig.cpu
@@ -38,7 +38,6 @@ config SUN3
        depends on MMU
        select HAVE_ARCH_PFN_VALID
        select LEGACY_TIMER_TICK
-       select NO_DMA
        select M68020
        help
          This option enables support for the Sun 3 series of workstations
@@ -558,4 +557,4 @@ config COLDFIRE_COHERENT_DMA
 config M68K_NONCOHERENT_DMA
        bool
        default y
-       depends on HAS_DMA && !COLDFIRE_COHERENT_DMA
+       depends on HAS_DMA && !COLDFIRE_COHERENT_DMA && !SUN3
--- a/arch/m68k/kernel/dma.c
+++ b/arch/m68k/kernel/dma.c
@@ -8,7 +8,7 @@
 #include <linux/kernel.h>
 #include <asm/cacheflush.h>
 
-#ifndef CONFIG_COLDFIRE
+#if !defined(CONFIG_COLDFIRE) && !defined(CONFIG_SUN3)
 void arch_dma_prep_coherent(struct page *page, size_t size)
 {
        cache_push(page_to_phys(page), size);

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Christoph Hellwig]

On Mon, Jun 16, 2025 at 03:48:50PM +0200, Arnd Bergmann wrote:
> As far as I can tell, the difference here is that the
> dma_alloc_coherent()/dma_free_coherent() calls all get stubbed
> out, so the 827 drivers using those can all build cleanly on
> mk68knommu, shnommu and UML, while dma_alloc_noncoherent()/
> dma_free_noncoherent() are only used on 15 files that are all
> guarded by some other Kconfig dependency at the moment and won't
> build on the those platforms.

Yes, dma_alloc_coherent is from a time where stubbing out was
still very common.

> I agree that it would be best to treat the coherent/noncoherent
> cases the same, and I also think the existing stubs are a bit
> silly, but just removing them would likely require fixing
> hundreds of drivers with added Kconfig or IS_ENABLED() checks.

I doubt it's that many, as most drivers and even subsystems simply
depend on DMA.  There's probably at most a few dozen drivers
supporting DMA but not requiring it.

> Maybe we can actually remove CONFIG_NO_DMA/CONFIG_HAS_DMA
> entirely and remove all the checks for CONFIG_HAS_DMA? 
> My guess is that this would only lead to a small code size
> increase on the affected targets, but since they are not
> actually trying to do DMA, and they all have a very limited
> set of drivers they actually use, it won't break existing
> code.

Except for uml, the CONFIG_NO_DMA configs are usually very resource
constraint, so I don't think that's a good idea.

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Arnd Bergmann]

On Tue, Jun 17, 2025, at 06:48, Christoph Hellwig wrote:
> On Mon, Jun 16, 2025 at 03:48:50PM +0200, Arnd Bergmann wrote:
>> As far as I can tell, the difference here is that the
>> dma_alloc_coherent()/dma_free_coherent() calls all get stubbed
>> out, so the 827 drivers using those can all build cleanly on
>> mk68knommu, shnommu and UML, while dma_alloc_noncoherent()/
>> dma_free_noncoherent() are only used on 15 files that are all
>> guarded by some other Kconfig dependency at the moment and won't
>> build on the those platforms.
>
> Yes, dma_alloc_coherent is from a time where stubbing out was
> still very common.
>
>> I agree that it would be best to treat the coherent/noncoherent
>> cases the same, and I also think the existing stubs are a bit
>> silly, but just removing them would likely require fixing
>> hundreds of drivers with added Kconfig or IS_ENABLED() checks.
>
> I doubt it's that many, as most drivers and even subsystems simply
> depend on DMA.  There's probably at most a few dozen drivers
> supporting DMA but not requiring it.

I checked again, and the actual number is 263 modules for a j2
allmodconfig with the DMA stubs removed, see
https://pastebin.com/raw/4PFcEe04

This is helped a lot by PCI being unavailable on all four
targets without DMA. If I force-enable PCI, I see 610 modules
in allmodconfig that link to one of the dma-mapping helper
functions but are missing a dependency on CONFIG_HAS_DMA.

>> Maybe we can actually remove CONFIG_NO_DMA/CONFIG_HAS_DMA
>> entirely and remove all the checks for CONFIG_HAS_DMA? 
>> My guess is that this would only lead to a small code size
>> increase on the affected targets, but since they are not
>> actually trying to do DMA, and they all have a very limited
>> set of drivers they actually use, it won't break existing
>> code.
>
> Except for uml, the CONFIG_NO_DMA configs are usually very resource
> constraint, so I don't think that's a good idea.

The J2 Turtleboard has 256MB of RAM, which is not too
constrained either.

Between SH72xx/SH76xx, SUN3 and M68328, I believe the
supported machines are all limited to between 1MB and 32MB in
the maximum configuration, which is obviously extremely
tight. On the other hand, I really don't think anyone cares
any more: all three platforms are between 25 and 40 years
old, kernel support is very minimal and nobody has really
spent time improving them in at least 15 years. It also
appears that all three do support DMA in some form, so if
anyone was serious about using them, they would probably
want to use the dma-mapping interface for it as well,
like we do for similarly constrained nommu platforms
(coldfire, cortex-m3, xtensa)

      Arnd

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Arnd Bergmann]

On Tue, Jun 17, 2025, at 09:53, Arnd Bergmann wrote:

> Between SH72xx/SH76xx, SUN3 and M68328, I believe the
> supported machines are all limited to between 1MB and 32MB in
> the maximum configuration, which is obviously extremely
> tight.

I checked the exact numbers we're talking about here: enabling
CONFIG_HAS_DMA on rsk7269_defconfig adds 10KB of extra vmlinux
size, which doesn't seem too bad:

   text	   data	    bss	    dec	    hex	filename
3295084	1111396	 112264	4518744	 44f358	vmlinux-before
3302836	1113652	 112264	4528752	 451a70	vmlinux-after

      Arnd

Re: [PATCH] dma-mapping: Stub out dma_{alloc,free,map}_pages() API

[Jason Gunthorpe]

On Tue, Jun 17, 2025 at 10:26:51AM +0200, Arnd Bergmann wrote:
> On Tue, Jun 17, 2025, at 09:53, Arnd Bergmann wrote:
> 
> > Between SH72xx/SH76xx, SUN3 and M68328, I believe the
> > supported machines are all limited to between 1MB and 32MB in
> > the maximum configuration, which is obviously extremely
> > tight.
> 
> I checked the exact numbers we're talking about here: enabling
> CONFIG_HAS_DMA on rsk7269_defconfig adds 10KB of extra vmlinux
> size, which doesn't seem too bad:
> 
>    text	   data	    bss	    dec	    hex	filename
> 3295084	1111396	 112264	4518744	 44f358	vmlinux-before
> 3302836	1113652	 112264	4528752	 451a70	vmlinux-after

Long ago I ran some numbers for an ancient PPC system:

https://lore.kernel.org/all/20121119214922.GA5636@obsidianresearch.com/

The base smallest kernel was growing .text and a stripped down initrd
at a rate of 1MB evey 6 years.

Somehow I doubt that system (with 16MB ram I think it was) would even
fit a v6.x kernel. v3.6 was already challenging.

Even back then Greg was incredulous that an embedded system would run
a 6 year newer kernel. Here we are contemplating a 20 year newer
kernel?

I think you have the right direction, we just removed !SMP support,
removing !DMA also seems logical to me.

Jason