diff mbox series

[V2,5/6] spi: bcm2835: make the lower limit for dma mode configurable

Message ID 20190423201513.8073-6-kernel@martin.sperl.org (mailing list archive)
State New, archived
Headers show
Series spi: bcm2835: improvements | expand

Commit Message

Martin Sperl April 23, 2019, 8:15 p.m. UTC 
From: Martin Sperl <kernel@martin.sperl.org>

Under some circumstances the default 96 byte limit is not optimal.

The polling and interrupt mode of the spi-bcm2835 controller exhibit a
1 idle clock cycle which is missing when using DMA.

So when transferring a longer spi_transfer to some low spec MCU devices
without SPI FIFOs like a ATMega it means that the SPI has to get reduced
by a factor of 2 or more when using DMA mode compared to using PIO modes
to give the MCU enough time to handle the incoming spi byte and prepare
the next byte to get delivered - these extra 4+ CPU cycles that the idle
spi clock is providing allows bigger transfers to work at faster speeds
(assuming highly optimized MCU code).
So forcing the transfer to use PIO mode to geth this behaviour for
longer transfers is of advantage.

The oposite is true when (ab)using the spi MOSI line alone to control a
WS2812B RGB LED stripe, where the DMA mode makes it possible to use the
MOSI lines alone (with correct encoding) to control the LED.

So both extremes have there use cases and with this patch we allow to
control the policy on a controller wide basis.

Right now we only allow to control the limit on a controller wide basis,
but in the future it may be possible to configure this on a per spi_device
basis.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/spi/spi-bcm2835.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Lukas Wunner April 24, 2019, 7:07 a.m. UTC | #1 
On Tue, Apr 23, 2019 at 08:15:12PM +0000, kernel@martin.sperl.org wrote:
> Under some circumstances the default 96 byte limit is not optimal.
> 
> The polling and interrupt mode of the spi-bcm2835 controller exhibit a
> 1 idle clock cycle which is missing when using DMA.
> 
> So when transferring a longer spi_transfer to some low spec MCU devices
> without SPI FIFOs like a ATMega it means that the SPI has to get reduced
> by a factor of 2 or more when using DMA mode compared to using PIO modes
> to give the MCU enough time to handle the incoming spi byte and prepare
> the next byte to get delivered - these extra 4+ CPU cycles that the idle
> spi clock is providing allows bigger transfers to work at faster speeds
> (assuming highly optimized MCU code).
> So forcing the transfer to use PIO mode to geth this behaviour for
> longer transfers is of advantage.
> 
> The oposite is true when (ab)using the spi MOSI line alone to control a
> WS2812B RGB LED stripe, where the DMA mode makes it possible to use the
> MOSI lines alone (with correct encoding) to control the LED.
> 
> So both extremes have there use cases and with this patch we allow to
> control the policy on a controller wide basis.
> 
> Right now we only allow to control the limit on a controller wide basis,
> but in the future it may be possible to configure this on a per spi_device
> basis.

Indeed I'd prefer if another bit is added to "mode" in struct device
to represent the need for another clock cycle in-between bytes.
The SPI core could then reduce the clock speed based on this flag
and the problem would be solved for everyone.

Influencing this behavior with a module parameter feels a bit like a
kludge and I fear may stay indefinitely even if a better solution
is implemented later.

Thanks,

Lukas
Mark Brown May 8, 2019, 8:42 a.m. UTC | #2 
On Wed, Apr 24, 2019 at 09:07:12AM +0200, Lukas Wunner wrote:

> Indeed I'd prefer if another bit is added to "mode" in struct device
> to represent the need for another clock cycle in-between bytes.
> The SPI core could then reduce the clock speed based on this flag
> and the problem would be solved for everyone.

> Influencing this behavior with a module parameter feels a bit like a
> kludge and I fear may stay indefinitely even if a better solution
> is implemented later.

This does feel like we know enough to have a more advanced function in
the driver given a bit of information about the client device
requireemnts.  Though it's going to be complex to express them,
especially with the ATMega case where we want fast individual clocks but
lots of dead space in between bytes (is the controller capable of adding
that dead space itself in DMA mode BTW?).
Martin Sperl May 8, 2019, 10:55 a.m. UTC | #3 
> On 08.05.2019, at 10:42, Mark Brown <broonie@kernel.org> wrote:
> 
> especially with the ATMega case where we want fast individual clocks but
> lots of dead space in between bytes (is the controller capable of adding
> that dead space itself in DMA mode BTW?).

No it is not - at least not so far as I know of (and I just went over the
available documentation again).

Martin
diff mbox series

Patch

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 2d702db8689c..755e1e6d2252 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -73,7 +73,6 @@ 
 
 #define BCM2835_SPI_FIFO_SIZE		64
 #define BCM2835_SPI_FIFO_SIZE_3_4	48
-#define BCM2835_SPI_DMA_MIN_LENGTH	96
 #define BCM2835_SPI_MODE_BITS	(SPI_CPOL | SPI_CPHA | SPI_CS_HIGH \
 				| SPI_NO_CS | SPI_3WIRE)
 
@@ -85,6 +84,12 @@  module_param(polling_limit_us, uint, 0664);
 MODULE_PARM_DESC(polling_limit_us,
 		 "time in us to run a transfer in polling mode\n");
 
+/* define minimum transfer length for which we may use DMA */
+unsigned int dma_min_bytes_limit = 96;
+module_param(dma_min_bytes_limit, uint, 0664);
+MODULE_PARM_DESC(dma_min_bytes_limit,
+		 "minimum number of bytes to run a transfer in dma mode - 0 disables dma-mode completely\n");
+
 /**
  * struct bcm2835_spi - BCM2835 SPI controller
  * @regs: base address of register map
@@ -631,7 +636,7 @@  static bool bcm2835_spi_can_dma(struct spi_master *master,
 				struct spi_transfer *tfr)
 {
 	/* we start DMA efforts only on bigger transfers */
-	if (tfr->len < BCM2835_SPI_DMA_MIN_LENGTH)
+	if (!dma_min_bytes_limit || tfr->len < dma_min_bytes_limit)
 		return false;
 
 	/* BCM2835_SPI_DLEN has defined a max transfer size as