Re: [PATCH v5 1/2] spi: support inter-word delay requirement for devices
From: Jonas Bonn
Date: Wed Jan 30 2019 - 03:10:20 EST
Hi Geert,
On 30/01/2019 08:35, Geert Uytterhoeven wrote:
Hi Jonas,
On Tue, Jan 29, 2019 at 9:55 PM Jonas Bonn <jonas@xxxxxxxxxxx> wrote:
Some devices are slow and cannot keep up with the SPI bus and therefore
require a short delay between words of the SPI transfer.
The example of this that I'm looking at is a SAMA5D2 with a minimum SPI
clock of 400kHz talking to an AVR-based SPI slave. The AVR cannot put
bytes on the bus fast enough to keep up with the SoC's SPI controller
even at the lowest bus speed.
This patch introduces the ability to specify a required inter-word
delay for SPI devices. It is up to the controller driver to configure
itself accordingly in order to introduce the requested delay.
Note that, for spi_transfer, there is already a field word_delay that
provides similar functionality. This field, however, is specified in
clock cycles (and worse, SPI controller cycles, not SCK cycles); that
makes this value dependent on the master clock instead of the device
clock for which the delay is intended to provide some relief. This
patch leaves this old word_delay in place and provides a time-based
word_delay_us alongside it; the new field fits in the struct padding
so struct size is constant. There is only one in-kernel user of the
word_delay field and presumably that driver could be reworked to use
the time-based value instead.
Thanks for your patch!
The time-based delay is limited to 8 bits as these delays are intended
to be short. The SAMA5D2 that I've tested this on limits delays to a
maximum of ~100us, which is already many word-transfer periods even at
the minimum transfer speed supported by the controller.
Still, the similar delay_usecs uses a u16.
The delays are not comparable. delay_usecs is the "end of transfer"
delay and represents the processing time to handle a command or a bundle
of data. The inter-word delay is just a stutter to give the slave time
to get the next word set up.
On the Microchip (Atmel... these name changes take a year or two to sink
in!) board that I'm using (SAMA5D2), the inter-word delay is limited to
8192 SPI-controller-clock cycles (not SPI bus clock, input clock). At
83Mhz, that's about 100us, and it only gets shorter as you clock up.
The Spreadtrum board has an upper limit of 130 SPI-controller-clock
cycles which is just a few microseconds.
Aside from that limitation, consider what's reasonable in terms of
delay. More than 5x the word-transfer time and things are pretty
questionable. A 250us delay with a word transfer time of 50us gives an
SPI-bus clock 160kHz. That's already pretty slow in SPI terms and well
below what the SAMA5D2 is capable of driving with it's 83MHz input clock
(max divider is 255 which gives roughtly 400kHz minimum SPI bus clock).
So with that, setting an inter-word delay larger than 255us is barely
reasonable. If somebody finds a concrete use for such a setting, then
the size of the field can be expanded at that time.
Just for info, the inter-word delay that I need to communicate with an
Atmega MCU running at 500kHz is ~20us which is on the order of the word
transfer time itself. This chip is a poor example of how to design an
SPI slave so I suspect it can only get better from here! :)
And with that said, if you insist having a u16 for this, I'll change it.
Just let me know.
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -803,6 +808,7 @@ struct spi_transfer {
#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */
#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */
u8 bits_per_word;
+ u8 word_delay_us;
us for Âs
u16 delay_usecs;
usecs for Âs
Can we please try to be consistent?
I can change it to usecs if you want. Is this a serious request to do
so? _usecs and _us are used pretty interchangeably across the kernel
with a slight advantage to _us.
/Jonas
Gr{oetje,eeting}s,
Geert