Re: spi-gpio too fast for some devices

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Christian,

On Sat, Jun 29, 2019 at 11:44 AM Christian Mauderer <oss@xxxxxxxxxxxxx> wrote:
> On 24/06/2019 20:58, Christian Mauderer wrote:
> > On 24/06/2019 15:23, Mark Brown wrote:
> >> On Sat, Jun 22, 2019 at 07:45:50AM +0200, Christian Mauderer wrote:
> >>> On 10/06/2019 18:56, Christian Mauderer wrote:
> >>
> >>>> I have a problem with the spi-gpio driver: It's too fast for one of my
> >>>> devices. Now I'm searching for a good solution that could be
> >>>> acceptable as a patch for the Linux kernel.
> >>
> >>>> Currently there is the following comment and implementation for the
> >>>> spidelay(...) function in spi-gpio.c:
> >>
> >>>>> /*
> >>>>>  * NOTE:  this clocks "as fast as we can".  It "should" be a function of the
> >>>>>  * requested device clock.  Software overhead means we usually have trouble
> >>>>>  * reaching even one Mbit/sec (except when we can inline bitops), so for now
> >>>>>  * we'll just assume we never need additional per-bit slowdowns.
> >>>>>  */
> >>>>> #define spidelay(nsecs)   do {} while (0)
> >>
> >>>>> #define spidelay(nsecs)   ndelay(nsecs)
> >>
> >>>> which basically works. But with that the maximum rate drops to 1.6MHz.
> >>>> I assume that such a drastic performance decrease isn't acceptable for
> >>>> the kernel?
> >>
> >> Yes, I can't imagine that other users are going to be happy with a
> >> performance reduction like that.
> >>
> >>>> Any directions for how an acceptable implementation could look like?
> >>
> >> Off the top of my head you probably need to build a second copy of the
> >> code with the delays included and then select that copy depending on the
> >> speed that's been requested for the device and the speed of the system
> >> somehow.  The actual bitbanging is in a header so that makes it a bit
> >> easier to build two copies than it might otherwise be.
> >>
> >
> > Hello Mark,
> >
> > thanks for the answer and the direction. I'll have a look at the driver
> > and try to create a rough plan which function I can replace without
> > creating too much of copy and paste code and without loosing too much
> > performance. I'm not sure yet where a good location would be to decide
> > which function should be used depending on the speed but I'll try to
> > find one.
> >
> > I'll report back as soon as I have a plan and (maybe) at least a sketch
> > for a patch. Most likely that will need some time because I only do that
> > in my free time.
> >
> > Best regards
> >
> > Christian
> >
>
> Hello Mark,
>
> I had a look at my options for slowing down the spi-gpio in some cases:
>
> == Approach 1:
>
> With some preprocessor magic I could create two sets of bitbang
> functions (bitbang_txrx_be_cpha0/1 and bitbang_txrx_be_cpha0/1_slow). I
> can then decide which of the functions to use in
> spi_gpio_txrx_word_mode0 (and similar functions) based on the nsecs
> value. For example if (nsecs > 1000) use bitbang_txrx_be_cpha0_slow()
> function otherwise use bitbang_txrx_be_cpha0().
>
> This approach would add a minimal delay between the bytes but not in the
> bits. It would allow to have fast and slow devices on the same bus. But
> it's a little intransparent from a user perspective because there is
> some special (hard coded) speed where the behaviour changes.
>
> == Approach 2:
>
> Add a kernel config option to allow the user to select whether he wants
> maximum speed or the possibility to slow down the SPI bus. Default would
> be the same as it is now.
>
> That is very simple to implement. It doesn't allow to mix slow and fast
> devices but it is quite clear from a user perspective and can be
> documented via a kernel option. Due to that I would slightly prefer that
> approach.
>
> What do you think: Would one of these be acceptable as a solution?

A third approach would be to set up the spi_bitbang.txrx_word[]
function pointers at runtime, to point to "fast" or "slow" versions,
depending on the runtime-detected speed of the system you're
running on.

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