Chunk splitting of spi transfers

drivers/spi/spi-bcm2835.c +0 -6

@@ -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...
-	 */

Review

Quoted 4 lines

(quoted diff) +0 -3

-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.
-Remove automatic byte swapping in tinydrm_spi_transfer as it doesn't have users.
-Remove the upper bound check on dma transfer length in bcm2835_spi_can_dma().

AFAICS you need to reverse the order of the two patches, so that the splitting is added to spi-bcm2835 before you remove it from the DRM drivers. Otherwise there's no splitting at all in-between the patches, which may cause issues for someone hitting this patch when bisecting.

It would be good if you could explain the rationale of the patch (the "why") in more detail. You've provided the rationale in the cover letter but the cover letter isn't recorded in the git history. Right now the commit message only summarizes the "what".

Also, please wrap the commit message at 72 chars.

Quoted 12 lines

drivers/spi/spi-bcm2835.c +0 -6

@@ -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...
-	 */

drivers/spi/spi-bcm2835.c static bool bcm2835_spi_can_dma(struct spi_master *master, · line 370

Quoted 10 lines

drivers/spi/spi-bcm2835.c +0 -7

-	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))

Move this hunk to the other patch so that it's together with the changes that remove the 65535 limitation. It would probably good to retain a portion of the comment to explain why the splitting is necessary.

drivers/spi/spi-bcm2835.c static void bcm2835_dma_init(struct spi_master *master, struct device *dev) · line 451

Quoted 6 lines

drivers/spi/spi-bcm2835.c +1 -1

@@ -461,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 */

Spurious unexplained change.

Thanks,

Lukas

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.

[cc += [email protected]]

Quoted 7 lines · 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 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.

[cc += [email protected]]

Quoted 7 lines · 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 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

Quoted 10 lines

[cc += [email protected]]

Quoted 7 lines · 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 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.

Quoted 7 lines

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.