Hi Jonas,
On Wed, Jan 30, 2019 at 9:10 AM Jonas Bonn <jonas@xxxxxxxxxxx> wrote:
> On 30/01/2019 08:35, Geert Uytterhoeven wrote:
> > 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.
I'll defer that to the maintainer (Mark).
> >> --- 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.
Personally, I prefer "us", too. Unfortunately not everyone has been
converted to metric, SI, and EURO yet ;-)
However, it looks really odd if the next line uses "usecs".
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
