Re: [PATCH v5 1/2] spi: support inter-word delay requirement for devices

[Geert Uytterhoeven]

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