Re: [PATCH v3 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Martin Guy <martinwguy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 4/19/10, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>  >> > +static int ep93xx_spi_flush(const struct ep93xx_spi *espi, unsigned long msecs)
>  >> > +{
>  >> > +       unsigned long timeout = jiffies + msecs_to_jiffies(msecs);
>  >> > +
>  >> > +       while (ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_RNE) {
>  >> > +               if (time_after(jiffies, timeout))
>  >> > +                       return -ETIMEDOUT;
>  >>
>  >> Ouch.  With the current code the driver could busy wait for 100ms!  If
>  >> flushing takes time, then don't busywait.  Bail out to some form of
>  >> deferred work.  Let the kernel get on with something else.
>  >
>  > I've never witnessed flushing to take this whole 100ms. Timeout is
>  > there just to make sure that this ends eventually. I can change it
>  > to some lower value (say few msecs or something like that).
>
>  Even with a lower value, the driver is still busywaiting which is bad
>  for latency.  If I see a lot of busywaiting, then I look very closely
>  at refactoring the driver into a state machine that can easily defer
>  work.

This function and all busywaiting should disappear now that the
interrupt-driven mode only interrupts once per four words instead of
doing a whole block transfer inside a single interrupts call.

>  >> > + * ep93xx_spi_calc_divisors() - calculates SPI clock divisors
>  >> > +
>  >> > +       /*
>  >> > +        * Calculate divisors so that we can get speed according the
>  >> > +        * following formula:
>  >> > +        *      rate = spi_clock_rate / (cpsr * (1 + scr))
>  >> > +        *
>  >> > +        * cpsr must be even number and starts from 2, scr can be any number
>  >> > +        * between 0 and 255.
>  >> > +        */
>  >> > +       for (cpsr = 2; cpsr <= 254; cpsr += 2) {
>  >> > +               for (scr = 0; scr <= 255; scr++) {
>  >>
>  >> Non-deterministic.
Eh? Some new meaning of "non-deterministic" that I wasn'f familiar with? :)

Yes, it does look gross, but max clock rate is 3.7Mhz (7.4 in rev E2
silicon), so if your slowest device is 400kHz, in practice it only
runs the inner loop from 0 to less than 10.
How slow do SPI devices go in reality? The 254*256 divisor gives 100Hz.

>  >> algebraically calculate the factors instead of an iterative loop.

>  > FIFO size is max 8 frames so there should be 8 iterations when the
>  > FIFO is full. So is it enought to to add check that we only read
>  > max 8 frames from the FIFO?
>
>  If you do it right, FIFO size shouldn't matter.  The driver should
>  easily be able to read however many are available and defer until more
>  is ready.

Again, with the new interrupt-handling code you have to know the FIFO
size because if you regulate the TX filling with the FIFO_NOT_FULL_YET
status bit you end up with 8 words in the TX FIFO plus one word being
transmitted already and that makes 9, which can  overflow the receive
buffer (I've seen this happening in practice).

    M

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev