[PATCH v2 0/2] Chunk splitting of spi transfers

[PATCH v2 0/2] Chunk splitting of spi transfers

[Meghana Madhyastha]

-Call spi_split_transfers_maxsize in __spi_pump_messages
to split large chunks for spi dma transfers.
-Remove chunk splitting in the tinydrm spi helper (as now the core is
handling the chunk splitting).

Changes in v2:
-Change the order of the two patches in the patchset.
-Undo the spurious blank line deletions
-Remove bcm2835_spi_transfer_one_message and add spi_split_transfers_maxsize
 in __spi_pump_messages. This solves the DMA time out error.

Meghana Madhyastha (2):
  spi: Split spi message into chunks of <65535 in the spi subsystem
  drm/tinydrm: Remove chunk splitting in tinydrm_spi_transfer

 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 48 ++++----------------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c             | 10 ++----
 drivers/spi/spi-bcm2835.c                      | 15 +-------
 drivers/spi/spi.c                              |  8 +++++
 4 files changed, 17 insertions(+), 64 deletions(-)

-- 
2.11.0

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

[PATCH v2 1/2] spi: Split spi message into chunks of <65535 in the spi subsystem

[Meghana Madhyastha]

Split spi messages into chunks of <65535 in the spi subsystem and remove the message
length warning in bcm2835_spi_can_dma. This is so that the messages can be transferred
via dma and that the tinydrm drivers need not split it.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
---
 drivers/spi/spi-bcm2835.c | 13 -------------
 drivers/spi/spi.c         |  8 ++++++++
 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index f35cc10772f6..0dcc45f158b8 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -365,19 +365,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;
-	}
-
 	/* if we run rx/tx_buf with word aligned addresses then we are OK */
 	if ((((size_t)tfr->rx_buf & 3) == 0) &&
 	    (((size_t)tfr->tx_buf & 3) == 0))
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b33a727a0158..e8e2c366a93b 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1242,6 +1242,14 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
 	trace_spi_message_start(ctlr->cur_msg);
 
 	if (ctlr->prepare_message) {
+		gfp_t gfp_flags = GFP_KERNEL | GFP_DMA;
+		size_t max_transfer_size = 32000;
+		ret = spi_split_transfers_maxsize(ctlr, ctlr->cur_msg, max_transfer_size, gfp_flags);
+		if (ret) {
+			dev_err(&ctlr->dev,
+				"failed to split message\n");
+			goto out;
+		}
 		ret = ctlr->prepare_message(ctlr, ctlr->cur_msg);
 		if (ret) {
 			dev_err(&ctlr->dev, "failed to prepare message: %d\n",
-- 
2.11.0

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

Re: [PATCH v2 1/2] spi: Split spi message into chunks of <65535 in the spi subsystem

[Geert Uytterhoeven]

Hi Meghana,

On Sat, Mar 10, 2018 at 4:51 PM, Meghana Madhyastha
<meghana.madhyastha@gmail.com> wrote:
> Split spi messages into chunks of <65535 in the spi subsystem and remove the message
> length warning in bcm2835_spi_can_dma. This is so that the messages can be transferred
> via dma and that the tinydrm drivers need not split it.
>
> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>

Thanks for your patch!

> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1242,6 +1242,14 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
>         trace_spi_message_start(ctlr->cur_msg);
>
>         if (ctlr->prepare_message) {
> +               gfp_t gfp_flags = GFP_KERNEL | GFP_DMA;
> +               size_t max_transfer_size = 32000;

Do you really want to impose this arbitrary limit (which BTW doesn't match
the value in the patch description) to each and every SPI controller driver?

> +               ret = spi_split_transfers_maxsize(ctlr, ctlr->cur_msg, max_transfer_size, gfp_flags);
> +               if (ret) {
> +                       dev_err(&ctlr->dev,
> +                               "failed to split message\n");
> +                       goto out;
> +               }
>                 ret = ctlr->prepare_message(ctlr, ctlr->cur_msg);
>                 if (ret) {
>                         dev_err(&ctlr->dev, "failed to prepare message: %d\n",

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] spi: Split spi message into chunks of <65535 in the spi subsystem

[Noralf Trønnes]

Den 10.03.2018 16.51, skrev Meghana Madhyastha:
> Split spi messages into chunks of <65535 in the spi subsystem and remove the message
> length warning in bcm2835_spi_can_dma. This is so that the messages can be transferred
> via dma and that the tinydrm drivers need not split it.
>
> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> ---
>   drivers/spi/spi-bcm2835.c | 13 -------------
>   drivers/spi/spi.c         |  8 ++++++++
>   2 files changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> index f35cc10772f6..0dcc45f158b8 100644
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c
> @@ -365,19 +365,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;
> -	}
> -
>   	/* if we run rx/tx_buf with word aligned addresses then we are OK */
>   	if ((((size_t)tfr->rx_buf & 3) == 0) &&
>   	    (((size_t)tfr->tx_buf & 3) == 0))
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index b33a727a0158..e8e2c366a93b 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1242,6 +1242,14 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
>   	trace_spi_message_start(ctlr->cur_msg);
>   
>   	if (ctlr->prepare_message) {
> +		gfp_t gfp_flags = GFP_KERNEL | GFP_DMA;
> +		size_t max_transfer_size = 32000;

What number is this?

As Geert says, this shouldn't be in the core but in the driver callback
bcm2835_spi_prepare_message().

Noralf.

> +		ret = spi_split_transfers_maxsize(ctlr, ctlr->cur_msg, max_transfer_size, gfp_flags);
> +		if (ret) {
> +			dev_err(&ctlr->dev,
> +				"failed to split message\n");
> +			goto out;
> +		}
>   		ret = ctlr->prepare_message(ctlr, ctlr->cur_msg);
>   		if (ret) {
>   			dev_err(&ctlr->dev, "failed to prepare message: %d\n",

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

[PATCH v2 2/2] drm/tinydrm: Remove chunk splitting in tinydrm_spi_transfer

[Meghana Madhyastha]

Remove chunk splitting in tinydrm_spi_transfer in tinydrm-helpers as the spi core will split
a buffer into max_dma_len chunks for the spi controller driver to handle, automatic byte
swapping in tinydrm_spi_transfer as it doesn't have users.

Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
---
 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 48 ++++----------------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c             | 10 ++----
 drivers/spi/spi-bcm2835.c                      |  2 +-
 3 files changed, 9 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index bf96072d1b97..6064099e8e63 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -452,62 +452,26 @@ 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);
+	max_chunk = SIZE_MAX;
 
 	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;
-		}
-	}
+			__func__, bpw, max_chunk);
 
 	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);
-
-		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;
-	}
+	tinydrm_dbg_spi_message(spi, &m);
 
-	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 75dd65c57e74..c8af2d65c2ad 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -886,15 +886,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
@@ -924,8 +918,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;
 	}
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 0dcc45f158b8..2fd650891c07 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -448,7 +448,7 @@ 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 */
+	master->max_dma_len = 65528; /* 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;
 
-- 
2.11.0

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

Re: [PATCH v2 2/2] drm/tinydrm: Remove chunk splitting in tinydrm_spi_transfer

[Noralf Trønnes]

Den 10.03.2018 16.52, skrev Meghana Madhyastha:
> Remove chunk splitting in tinydrm_spi_transfer in tinydrm-helpers as the spi core will split
> a buffer into max_dma_len chunks for the spi controller driver to handle, automatic byte
> swapping in tinydrm_spi_transfer as it doesn't have users.
>
> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> ---
>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 48 ++++----------------------
>   drivers/gpu/drm/tinydrm/mipi-dbi.c             | 10 ++----
>   drivers/spi/spi-bcm2835.c                      |  2 +-
>   3 files changed, 9 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> index bf96072d1b97..6064099e8e63 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> @@ -452,62 +452,26 @@ 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);
> +	max_chunk = SIZE_MAX;

max_chunk isn't used so can be removed.
tinydrm_spi_max_transfer_size() can also be remove now.

>   
>   	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;
> -		}
> -	}
> +			__func__, bpw, max_chunk);
>   
>   	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);
> -
> -		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;
> -	}
> +	tinydrm_dbg_spi_message(spi, &m);
>   
> -	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 75dd65c57e74..c8af2d65c2ad 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -886,15 +886,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
> @@ -924,8 +918,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;
>   	}
> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> index 0dcc45f158b8..2fd650891c07 100644
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c
> @@ -448,7 +448,7 @@ 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 */
> +	master->max_dma_len = 65528; /* limitation by BCM2835_SPI_DLEN */

This change belongs in the spi-bcm2835 patch.
You need to tell us why the change is needed.

Noralf.

>   	/* need to do TX AND RX DMA, so we need dummy buffers */
>   	master->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;
>   

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