[PATCH v5 0/4] Chunk splitting of spi transfers

[PATCH v5 0/4] Chunk splitting of spi transfers

[Noralf Trønnes]

spi-bcm2835 has a ~64kB upper limit on DMA transfers. Drivers in
drivers/gpu/drm/tinydrm work around this limitation by splitting the
buffer into multiple transfers. This patchset lifts this driver
limitation by splitting affected transfers using
spi_split_transfers_maxsize().

Based on the feedback on the previous version, I now understand that
->max_dma_len is used to make scatter gather entries that the DMA engine
can handle. For a BCM2835 Lite DMA channel this is 65535 bytes, the same
limitation that the SPI block has on the combined DMA transfer length
through its DLEN register. The SPI block in DMA mode accesses the FIFO 4
bytes at a time, so the aligned maximum length is thus 65532 bytes.

Since this is a BCM2835 SPI block limitation and not a common DMA
limitation that drivers face, I've moved the splitting to the driver.

I also found out why buffer unmapping happened on the original transfer
instead of the split one. spi_res_release() restored the original
transfer before the message was finalized. AFAICT reordering this
shouldn't cause any problems.

Noralf.

Meghana Madhyastha (2):
  spi/spi-bcm2835: Split transfers that exceed DLEN
  drm/tinydrm: Remove chunk splitting in tinydrm_spi_transfer

Noralf Trønnes (2):
  spi: Remove warning in spi_split_transfers_maxsize()
  spi: Release spi_res after finalizing message

 .../gpu/drm/tinydrm/core/tinydrm-helpers.c    | 83 ++-----------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c            | 10 +--
 drivers/spi/spi-bcm2835.c                     | 39 +++------
 drivers/spi/spi.c                             |  9 +-
 4 files changed, 20 insertions(+), 121 deletions(-)

-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 1/4] spi: Remove warning in spi_split_transfers_maxsize()

[Noralf Trønnes]

Don't warn about splitting transfers, the info is available in the
statistics if needed.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/spi/spi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9a7def7c3237..05875e63be43 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2692,11 +2692,6 @@ static int __spi_split_transfer_maxsize(struct spi_controller *ctlr,
 	size_t offset;
 	size_t count, i;
 
-	/* warn once about this fact that we are splitting a transfer */
-	dev_warn_once(&msg->spi->dev,
-		      "spi_transfer of length %i exceed max length of %zu - needed to split transfers\n",
-		      xfer->len, maxsize);
-
 	/* calculate how many we have to replace */
 	count = DIV_ROUND_UP(xfer->len, maxsize);
 
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 2/4] spi: Release spi_res after finalizing message

[Noralf Trønnes]

spi_split_transfers_maxsize() can be used to split a transfer. This
function uses spi_res to lifetime manage the added transfer structures.
So in order to finalize the current message while it contains the split
transfers, spi_res_release() must be called after finalizing.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/spi/spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 05875e63be43..35939ffdeebe 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1162,10 +1162,10 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
 	if (msg->status && ctlr->handle_err)
 		ctlr->handle_err(ctlr, msg);
 
-	spi_res_release(ctlr, msg);
-
 	spi_finalize_current_message(ctlr);
 
+	spi_res_release(ctlr, msg);
+
 	return ret;
 }
 
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 3/4] spi/spi-bcm2835: Split transfers that exceed DLEN

[Noralf Trønnes]

From: Meghana Madhyastha <meghana.madhyastha@gmail.com>

Split spi transfers into chunks of <=65532 to enable the driver to
perform DMA transfer on big buffers. The DLEN register specifies the
number of bytes to transfer in DMA mode. It is 16-bit wide and thus the
maximum DMA transfer is 65535 bytes. Set the maximum to 65532 (4 byte
aligned) since the FIFO in DMA mode is accessed in 4 byte chunks.

->max_dma_len is the value the spi core uses when splitting the buffer
into scatter gather entries.
The BCM2835 DMA block has 2 types of channels/engines:
- Normal: Max length: 1G
- Lite: Max length: 65535

Even when using a Lite channel we will not exceed the max length, so
let's drop setting ->max_dma_len.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/spi/spi-bcm2835.c | 39 +++++++++++----------------------------
 1 file changed, 11 insertions(+), 28 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 35aebdfd3b4e..8aa22713c483 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -335,20 +335,6 @@ static int bcm2835_spi_transfer_one_irq(struct spi_master *master,
 	return 1;
 }
 
-/*
- * DMA support
- *
- * this implementation has currently a few issues in so far as it does
- * not work arrount limitations of the HW.
- *
- * the main one being that DMA transfers are limited to 16 bit
- * (so 0 to 65535 bytes) by the SPI HW due to BCM2835_SPI_DLEN
- *
- * there may be a few more border-cases we may need to address as well
- * but unfortunately this would mean splitting up the scatter-gather
- * list making it slightly unpractical...
- */
-
 /**
  * bcm2835_spi_transfer_prologue() - transfer first few bytes without DMA
  * @master: SPI master
@@ -630,19 +616,6 @@ static bool bcm2835_spi_can_dma(struct spi_master *master,
 	if (tfr->len < BCM2835_SPI_DMA_MIN_LENGTH)
 		return false;
 
-	/* BCM2835_SPI_DLEN has defined a max transfer size as
-	 * 16 bit, so max is 65535
-	 * we can revisit this by using an alternative transfer
-	 * method - ideally this would get done without any more
-	 * interaction...
-	 */
-	if (tfr->len > 65535) {
-		dev_warn_once(&spi->dev,
-			      "transfer size of %d too big for dma-transfer\n",
-			      tfr->len);
-		return false;
-	}
-
 	/* return OK */
 	return true;
 }
@@ -707,7 +680,6 @@ static void bcm2835_dma_init(struct spi_master *master, struct device *dev)
 
 	/* all went well, so set can_dma */
 	master->can_dma = bcm2835_spi_can_dma;
-	master->max_dma_len = 65535; /* limitation by BCM2835_SPI_DLEN */
 	/* need to do TX AND RX DMA, so we need dummy buffers */
 	master->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;
 
@@ -844,6 +816,17 @@ static int bcm2835_spi_prepare_message(struct spi_master *master,
 	struct spi_device *spi = msg->spi;
 	struct bcm2835_spi *bs = spi_master_get_devdata(master);
 	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
+	int ret;
+
+	/*
+	 * DMA transfers are limited to 16 bit (0 to 65535 bytes) by the SPI HW
+	 * due to DLEN. Split up transfers (32-bit FIFO aligned) if the limit is
+	 * exceeded.
+	 */
+	ret = spi_split_transfers_maxsize(master, msg, 65532,
+					  GFP_KERNEL | GFP_DMA);
+	if (ret)
+		return ret;
 
 	cs &= ~(BCM2835_SPI_CS_CPOL | BCM2835_SPI_CS_CPHA);
 
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 4/4] drm/tinydrm: Remove chunk splitting in tinydrm_spi_transfer

[Noralf Trønnes]

From: Meghana Madhyastha <meghana.madhyastha@gmail.com>

Remove chunk splitting in tinydrm_spi_transfer in tinydrm-helpers as
spi-bcm2835 now can handle large buffers using DMA, automatic byte
swapping in tinydrm_spi_transfer as it doesn't have users.

Remove the spi_max module argument that now has lost its cause.

The 16kB buffer size for Type C Option 1 (9-bit) interface is somewhat
arbitrary, but a bigger buffer will have a miniscule impact on transfer
speed, so it's probably fine.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 .../gpu/drm/tinydrm/core/tinydrm-helpers.c    | 83 ++-----------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c            | 10 +--
 2 files changed, 7 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index 6d540d93758f..a32dc859a9c1 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -22,41 +22,8 @@
 #include <drm/drm_rect.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 
-static unsigned int spi_max;
-module_param(spi_max, uint, 0400);
-MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
-
 #if IS_ENABLED(CONFIG_SPI)
 
-/**
- * tinydrm_spi_max_transfer_size - Determine max SPI transfer size
- * @spi: SPI device
- * @max_len: Maximum buffer size needed (optional)
- *
- * This function returns the maximum size to use for SPI transfers. It checks
- * the SPI master, the optional @max_len and the module parameter spi_max and
- * returns the smallest.
- *
- * Returns:
- * Maximum size for SPI transfers
- */
-size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len)
-{
-	size_t ret;
-
-	ret = min(spi_max_transfer_size(spi), spi->master->max_dma_len);
-	if (max_len)
-		ret = min(ret, max_len);
-	if (spi_max)
-		ret = min_t(size_t, ret, spi_max);
-	ret &= ~0x3;
-	if (ret < 4)
-		ret = 4;
-
-	return ret;
-}
-EXPORT_SYMBOL(tinydrm_spi_max_transfer_size);
-
 /**
  * tinydrm_spi_bpw_supported - Check if bits per word is supported
  * @spi: SPI device
@@ -147,62 +114,22 @@ int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
 	struct spi_transfer tr = {
 		.bits_per_word = bpw,
 		.speed_hz = speed_hz,
+		.tx_buf = buf,
+		.len = len
 	};
 	struct spi_message m;
-	u16 *swap_buf = NULL;
-	size_t max_chunk;
-	size_t chunk;
-	int ret = 0;
-
-	if (WARN_ON_ONCE(bpw != 8 && bpw != 16))
-		return -EINVAL;
-
-	max_chunk = tinydrm_spi_max_transfer_size(spi, 0);
 
 	if (drm_debug & DRM_UT_DRIVER)
-		pr_debug("[drm:%s] bpw=%u, max_chunk=%zu, transfers:\n",
-			 __func__, bpw, max_chunk);
-
-	if (bpw == 16 && !tinydrm_spi_bpw_supported(spi, 16)) {
-		tr.bits_per_word = 8;
-		if (tinydrm_machine_little_endian()) {
-			swap_buf = kmalloc(min(len, max_chunk), GFP_KERNEL);
-			if (!swap_buf)
-				return -ENOMEM;
-		}
-	}
+		pr_debug("[drm:%s] bpw=%u, transfers:\n", __func__, bpw);
 
 	spi_message_init(&m);
 	if (header)
 		spi_message_add_tail(header, &m);
 	spi_message_add_tail(&tr, &m);
 
-	while (len) {
-		chunk = min(len, max_chunk);
+	tinydrm_dbg_spi_message(spi, &m);
 
-		tr.tx_buf = buf;
-		tr.len = chunk;
-
-		if (swap_buf) {
-			const u16 *buf16 = buf;
-			unsigned int i;
-
-			for (i = 0; i < chunk / 2; i++)
-				swap_buf[i] = swab16(buf16[i]);
-
-			tr.tx_buf = swap_buf;
-		}
-
-		buf += chunk;
-		len -= chunk;
-
-		tinydrm_dbg_spi_message(spi, &m);
-		ret = spi_sync(spi, &m);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
+	return spi_sync(spi, &m);
 }
 EXPORT_SYMBOL(tinydrm_spi_transfer);
 
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index 85761b4abb83..0bcf6c764893 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -975,15 +975,9 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd,
 int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
 		      struct gpio_desc *dc)
 {
-	size_t tx_size = tinydrm_spi_max_transfer_size(spi, 0);
 	struct device *dev = &spi->dev;
 	int ret;
 
-	if (tx_size < 16) {
-		DRM_ERROR("SPI transmit buffer too small: %zu\n", tx_size);
-		return -EINVAL;
-	}
-
 	/*
 	 * Even though it's not the SPI device that does DMA (the master does),
 	 * the dma mask is necessary for the dma_alloc_wc() in
@@ -1013,8 +1007,8 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
 			mipi->swap_bytes = true;
 	} else {
 		mipi->command = mipi_dbi_typec1_command;
-		mipi->tx_buf9_len = tx_size;
-		mipi->tx_buf9 = devm_kmalloc(dev, tx_size, GFP_KERNEL);
+		mipi->tx_buf9_len = SZ_16K;
+		mipi->tx_buf9 = devm_kmalloc(dev, mipi->tx_buf9_len, GFP_KERNEL);
 		if (!mipi->tx_buf9)
 			return -ENOMEM;
 	}
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 3/4] spi/spi-bcm2835: Split transfers that exceed DLEN

[Lukas Wunner]

On Sat, Apr 13, 2019 at 08:24:14PM +0200, Noralf Trønnes wrote:
> @@ -844,6 +816,17 @@ static int bcm2835_spi_prepare_message(struct spi_master *master,
>  	struct spi_device *spi = msg->spi;
>  	struct bcm2835_spi *bs = spi_master_get_devdata(master);
>  	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
> +	int ret;
> +
> +	/*
> +	 * DMA transfers are limited to 16 bit (0 to 65535 bytes) by the SPI HW
> +	 * due to DLEN. Split up transfers (32-bit FIFO aligned) if the limit is
> +	 * exceeded.
> +	 */
> +	ret = spi_split_transfers_maxsize(master, msg, 65532,
> +					  GFP_KERNEL | GFP_DMA);
> +	if (ret)
> +		return ret;

This looks much better than the previous version because
spi_split_transfers_maxsize() is now used as a library function
by an individual driver, rather than something that is inflicted
on *all* drivers (midlayer fallacy).

Of course the performance is suboptimal compared to an approach
which transmits the sglist in portions which are < 65535 each,
but we can move to that later.

In case it helps,
Reviewed-by: Lukas Wunner <lukas@wunner.de>

Thanks,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Applied "spi: Release spi_res after finalizing message" to the spi tree

[Mark Brown]

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

The patch

   spi: Release spi_res after finalizing message

has been applied to the spi tree at

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

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 8ed2e1a50e74a08adce3fe0207be1649b2b13a83 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Noralf=20Tr=C3=B8nnes?= <noralf@tronnes.org>
Date: Sat, 13 Apr 2019 20:24:13 +0200
Subject: [PATCH] spi: Release spi_res after finalizing message
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

spi_split_transfers_maxsize() can be used to split a transfer. This
function uses spi_res to lifetime manage the added transfer structures.
So in order to finalize the current message while it contains the split
transfers, spi_res_release() must be called after finalizing.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 3c6c6101b611..2195fa265aef 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1181,10 +1181,10 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
 	if (msg->status && ctlr->handle_err)
 		ctlr->handle_err(ctlr, msg);
 
-	spi_res_release(ctlr, msg);
-
 	spi_finalize_current_message(ctlr);
 
+	spi_res_release(ctlr, msg);
+
 	return ret;
 }
 
-- 
2.20.1


[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Applied "spi: Remove warning in spi_split_transfers_maxsize()" to the spi tree

[Mark Brown]

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

The patch

   spi: Remove warning in spi_split_transfers_maxsize()

has been applied to the spi tree at

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

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 179f7949c0663f1923564acf0e626d459ea80047 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Noralf=20Tr=C3=B8nnes?= <noralf@tronnes.org>
Date: Sat, 13 Apr 2019 20:24:12 +0200
Subject: [PATCH] spi: Remove warning in spi_split_transfers_maxsize()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Don't warn about splitting transfers, the info is available in the
statistics if needed.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 86a31340ad03..3c6c6101b611 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2788,11 +2788,6 @@ static int __spi_split_transfer_maxsize(struct spi_controller *ctlr,
 	size_t offset;
 	size_t count, i;
 
-	/* warn once about this fact that we are splitting a transfer */
-	dev_warn_once(&msg->spi->dev,
-		      "spi_transfer of length %i exceed max length of %zu - needed to split transfers\n",
-		      xfer->len, maxsize);
-
 	/* calculate how many we have to replace */
 	count = DIV_ROUND_UP(xfer->len, maxsize);
 
-- 
2.20.1


[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel