[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

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

[Meghana Madhyastha]

I've added bcm2835_spi_transfer_one_message in spi-bcm2835. This calls
spi_split_transfers_maxsize to split large chunks for spi dma transfers. 
I then removed chunk splitting in the tinydrm spi helper (as now the core is
handling the chunk splitting). However, although the SPI HW should be able to
accomodate upto 65535 bytes for dma transfers, the splitting of chunks to 65535
bytes results in a dma transfer time out error. However, when the chunks are
split to <64  bytes it seems to work fine.

Changes in v2:
-Patch 2 did not exist in v1.

Meghana Madhyastha (2):
  drm/tinydrm: Remove chunk splitting in tinydrm_spi_transfer
  spi/spi-bcm2835: Add bcm2835_spi_transfer_one_message in spi-bcm2835.c

 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 48 ++++----------------------
 drivers/gpu/drm/tinydrm/mipi-dbi.c             | 10 ++----
 drivers/spi/spi-bcm2835.c                      | 29 ++++++++--------
 drivers/spi/spi.c                              |  5 ++-
 include/linux/spi/spi.h                        |  2 ++
 5 files changed, 27 insertions(+), 67 deletions(-)

-- 
2.11.0

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

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

[Lukas Wunner]

[cc += linux-rpi-kernel@lists.infradead.org]

On Sat, Feb 24, 2018 at 06:15:59PM +0000, Meghana Madhyastha wrote:
> I've added bcm2835_spi_transfer_one_message in spi-bcm2835. This calls
> spi_split_transfers_maxsize to split large chunks for spi dma transfers. 
> I then removed chunk splitting in the tinydrm spi helper (as now the core
> is handling the chunk splitting). However, although the SPI HW should be
> able to accomodate up to 65535 bytes for dma transfers, the splitting of
> chunks to 65535 bytes results in a dma transfer time out error. However,
> when the chunks are split to < 64 bytes it seems to work fine.

Hm, that is really odd, how did you test this exactly, what did you
use as SPI slave?  It contradicts our own experience, we're using
Micrel KSZ8851 Ethernet chips as SPI slave on spi0 of a BCM2837
and can send/receive messages via DMA to the tune of several hundred
bytes without any issues.  In fact, for messages < 96 bytes, DMA is
not used at all, so you've probably been using interrupt mode,
see the BCM2835_SPI_DMA_MIN_LENGTH macro in spi-bcm2835.c.

Thanks,

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

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

[Noralf Trønnes]

Den 25.02.2018 14.19, skrev Lukas Wunner:
> [cc += linux-rpi-kernel@lists.infradead.org]
>
> On Sat, Feb 24, 2018 at 06:15:59PM +0000, Meghana Madhyastha wrote:
>> I've added bcm2835_spi_transfer_one_message in spi-bcm2835. This calls
>> spi_split_transfers_maxsize to split large chunks for spi dma transfers.
>> I then removed chunk splitting in the tinydrm spi helper (as now the core
>> is handling the chunk splitting). However, although the SPI HW should be
>> able to accomodate up to 65535 bytes for dma transfers, the splitting of
>> chunks to 65535 bytes results in a dma transfer time out error. However,
>> when the chunks are split to < 64 bytes it seems to work fine.
> Hm, that is really odd, how did you test this exactly, what did you
> use as SPI slave?  It contradicts our own experience, we're using
> Micrel KSZ8851 Ethernet chips as SPI slave on spi0 of a BCM2837
> and can send/receive messages via DMA to the tune of several hundred
> bytes without any issues.  In fact, for messages < 96 bytes, DMA is
> not used at all, so you've probably been using interrupt mode,
> see the BCM2835_SPI_DMA_MIN_LENGTH macro in spi-bcm2835.c.

Meghana,
I suggest using modetest with page flipping to check that you don't
regress performance wise:

$ ./libdrm/tests/modetest/modetest -M mi0283qt -s 28:320x240@RG16 -v
setting mode 320x240-0Hz@RG16 on connectors 28, crtc 30
freq: 29.88Hz
freq: 29.96Hz
freq: 29.99Hz
^C

Noralf.

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

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

[Meghana Madhyastha]

On Sun, Feb 25, 2018 at 02:19:10PM +0100, Lukas Wunner wrote:
> [cc += linux-rpi-kernel@lists.infradead.org]
> 
> On Sat, Feb 24, 2018 at 06:15:59PM +0000, Meghana Madhyastha wrote:
> > I've added bcm2835_spi_transfer_one_message in spi-bcm2835. This calls
> > spi_split_transfers_maxsize to split large chunks for spi dma transfers. 
> > I then removed chunk splitting in the tinydrm spi helper (as now the core
> > is handling the chunk splitting). However, although the SPI HW should be
> > able to accomodate up to 65535 bytes for dma transfers, the splitting of
> > chunks to 65535 bytes results in a dma transfer time out error. However,
> > when the chunks are split to < 64 bytes it seems to work fine.
> 
> Hm, that is really odd, how did you test this exactly, what did you
> use as SPI slave?  It contradicts our own experience, we're using
> Micrel KSZ8851 Ethernet chips as SPI slave on spi0 of a BCM2837
> and can send/receive messages via DMA to the tune of several hundred
> bytes without any issues.  In fact, for messages < 96 bytes, DMA is
> not used at all, so you've probably been using interrupt mode,
> see the BCM2835_SPI_DMA_MIN_LENGTH macro in spi-bcm2835.c.

Hi Lukas,

I think you are right. I checked it and its not using the DMA mode which
is why its working with 64 bytes.
Noralf, that leaves us back to the
initial time out problem. I've tried doing the message splitting in
spi_sync as well as spi_pump_messages. Martin had explained that DMA
will wait for
the SPI HW to set the send_more_data line, but the SPI-HW itself will
stop triggering it when SPI_LEN is 0 causing DMA to wait forever. I
thought if we split it before itself, the SPI_LEN will not go to zero
thus preventing this problem, however it didn't work and started
hanging. So I'm a little uncertain as to how to proceed and debug what
exactly has caused the time out due to the asynchronous methods.

Thanks and regards,
Meghana

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

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

[Noralf Trønnes]

Den 02.03.2018 12.11, skrev Meghana Madhyastha:
> On Sun, Feb 25, 2018 at 02:19:10PM +0100, Lukas Wunner wrote:
>> [cc += linux-rpi-kernel@lists.infradead.org]
>>
>> On Sat, Feb 24, 2018 at 06:15:59PM +0000, Meghana Madhyastha wrote:
>>> I've added bcm2835_spi_transfer_one_message in spi-bcm2835. This calls
>>> spi_split_transfers_maxsize to split large chunks for spi dma transfers.
>>> I then removed chunk splitting in the tinydrm spi helper (as now the core
>>> is handling the chunk splitting). However, although the SPI HW should be
>>> able to accomodate up to 65535 bytes for dma transfers, the splitting of
>>> chunks to 65535 bytes results in a dma transfer time out error. However,
>>> when the chunks are split to < 64 bytes it seems to work fine.
>> Hm, that is really odd, how did you test this exactly, what did you
>> use as SPI slave?  It contradicts our own experience, we're using
>> Micrel KSZ8851 Ethernet chips as SPI slave on spi0 of a BCM2837
>> and can send/receive messages via DMA to the tune of several hundred
>> bytes without any issues.  In fact, for messages < 96 bytes, DMA is
>> not used at all, so you've probably been using interrupt mode,
>> see the BCM2835_SPI_DMA_MIN_LENGTH macro in spi-bcm2835.c.
> Hi Lukas,
>
> I think you are right. I checked it and its not using the DMA mode which
> is why its working with 64 bytes.
> Noralf, that leaves us back to the
> initial time out problem. I've tried doing the message splitting in
> spi_sync as well as spi_pump_messages. Martin had explained that DMA
> will wait for
> the SPI HW to set the send_more_data line, but the SPI-HW itself will
> stop triggering it when SPI_LEN is 0 causing DMA to wait forever. I
> thought if we split it before itself, the SPI_LEN will not go to zero
> thus preventing this problem, however it didn't work and started
> hanging. So I'm a little uncertain as to how to proceed and debug what
> exactly has caused the time out due to the asynchronous methods.

I did a quick test and at least this is working:

int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
              struct spi_transfer *header, u8 bpw, const void *buf,
              size_t len)
{
     struct spi_transfer tr = {
         .bits_per_word = bpw,
         .speed_hz = speed_hz,
         .tx_buf = buf,
         .len = len,
     };
     struct spi_message m;
     size_t maxsize;
     int ret;

     maxsize = tinydrm_spi_max_transfer_size(spi, 0);

     if (drm_debug & DRM_UT_DRIVER)
         pr_debug("[drm:%s] bpw=%u, maxsize=%zu, transfers:\n",
              __func__, bpw, maxsize);

     spi_message_init(&m);
     m.spi = spi;
     if (header)
         spi_message_add_tail(header, &m);
     spi_message_add_tail(&tr, &m);

     ret = spi_split_transfers_maxsize(spi->controller, &m, maxsize, 
GFP_KERNEL);
     if (ret)
         return ret;

     tinydrm_dbg_spi_message(spi, &m);

     return spi_sync(spi, &m);
}
EXPORT_SYMBOL(tinydrm_spi_transfer);


Log:
[   39.015644] [drm:mipi_dbi_fb_dirty [mipi_dbi]] Flushing [FB:36] x1=0, 
x2=320, y1=0, y2=240

[   39.018079] [drm:mipi_dbi_typec3_command [mipi_dbi]] cmd=2a, par=00 
00 01 3f
[   39.018129] [drm:tinydrm_spi_transfer] bpw=8, maxsize=65532, transfers:
[   39.018152]     tr(0): speed=10MHz, bpw=8, len=1, tx_buf=[2a]
[   39.018231] [drm:tinydrm_spi_transfer] bpw=8, maxsize=65532, transfers:
[   39.018248]     tr(0): speed=10MHz, bpw=8, len=4, tx_buf=[00 00 01 3f]

[   39.018330] [drm:mipi_dbi_typec3_command [mipi_dbi]] cmd=2b, par=00 
00 00 ef
[   39.018347] [drm:tinydrm_spi_transfer] bpw=8, maxsize=65532, transfers:
[   39.018362]     tr(0): speed=10MHz, bpw=8, len=1, tx_buf=[2b]
[   39.018396] [drm:tinydrm_spi_transfer] bpw=8, maxsize=65532, transfers:
[   39.018428]     tr(0): speed=10MHz, bpw=8, len=4, tx_buf=[00 00 00 ef]

[   39.018487] [drm:mipi_dbi_typec3_command [mipi_dbi]] cmd=2c, len=153600
[   39.018502] [drm:tinydrm_spi_transfer] bpw=8, maxsize=65532, transfers:
[   39.018517]     tr(0): speed=10MHz, bpw=8, len=1, tx_buf=[2c]
[   39.018565] [drm:tinydrm_spi_transfer] bpw=8, maxsize=65532, transfers:
[   39.018594]     tr(0): speed=48MHz, bpw=8, len=65532, tx_buf=[c6 18 
c6 18 c6 18 c6 18 c6 18 c6 18 c6 18 c6 18 ...]
[   39.018608]     tr(1): speed=48MHz, bpw=8, len=65532, tx_buf=[06 18 
06 18 06 18 06 18 06 18 06 18 06 18 06 18 ...]
[   39.018621]     tr(2): speed=48MHz, bpw=8, len=22536, tx_buf=[10 82 
10 82 10 82 10 82 10 82 10 82 18 e3 18 e3 ...]


Noralf.


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