[PATCH V3 2/4] AC97 driver for mpc5200

[Grant Likely]

On Mon, May 25, 2009 at 9:15 AM, Jon Smirl <jonsmirl at gmail.com> wrote:
> On Mon, May 25, 2009 at 2:16 AM, Grant Likely <grant.likely at secretlab.ca> wrote:
>> On Sun, May 24, 2009 at 7:38 PM, Jon Smirl <jonsmirl at gmail.com> wrote:
>>> +static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
>>> +{
>>> +       int timeout;
>>> +       unsigned int val;
>>> +
>>> +       spin_lock(&psc_dma->lock);
>>> +
>>> +       /* Wait for it to be ready */
>>> +       timeout = 1000;
>>> +       while ((--timeout) && (in_be16(&psc_dma->psc_regs->sr_csr.status) &
>>> +                                               MPC52xx_PSC_SR_CMDSEND))
>>> +               udelay(10);
>>
>> Holy unbounded latency Batman!  This code waits up to 10ms for a register read!
>>
>> I hate spinning, but if it must be done; I'd like to see it small.
>> What is the worst case latency? 125us for 8000Hz bus speed?  If you
>> must spin; can a cpu_relax() be used instead of the udelay() while
>> watch the timebase?  Timur recently posted a patch which makes this
>> easier.
>>
>> http://patchwork.ozlabs.org/patch/27414/
>>
>> They *should* be appearing in Ben's -next branch soon.
>
>
> The link always runs at 12.288Mhz. Each frame is 256 bits. Worst case
> you wait for two frames, 42us. If it doesn't respond in 42us the codec
> clock is not ticking ( a recurring problem I am running into). These
> codecs may be going into a sleep mode I don't understand, but this is
> not the right place to try and wake them up. I'll lower the retry
> counts to 10 instead of 1000.

That still leaves the problem of unecessarily burning time.  udelay
shouldn't be passed any value larger than 1.  In fact, I think udelay
itself is too coarse grained.  Plus, I'd rather see the timebase used
as the exit condition (as mentioned in previous email).

> I played around with implementing this on a kernel thread with
> interrupts. It can be done but the code is a lot more complex.

A kernel thread is definitely the wrong approach.  However, if this is
non-atomic context and IRQs are available, then a wait queue can be
used.  42us is about 16k processor clocks.  I'm not sure what the IRQ
and scheduling overhead is so I don't know whether it would be a net
gain or loss in performance.  However, it would be a net gain in worst
case latency.

> BTW, 8000Hz is implemented by slot stuffing. The link always runs at
> 12.288Mhz. The DACs are double buffered. When a sample is transfered
> between buffers it sets a bit on the link back to the host, and the
> host sends the next sample in the appropriate slot.

ok.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.