[alsa-devel] [PATCH V3 2/4] AC97 driver for mpc5200

[Jon Smirl]

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.

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

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.


>
>> +
>> +       if (!timeout) {
>> +               pr_err("timeout on ac97 bus (rdy)\n");
>> +               return 0xffff;
>> +       }
>> +
>> +       /* Do the read */
>> +       out_be32(&psc_dma->psc_regs->ac97_cmd, (1<<31) | ((reg & 0x7f) << 24));
>> +
>> +       /* Wait for the answer */
>> +       timeout = 1000;
>> +       while ((--timeout) && !(in_be16(&psc_dma->psc_regs->sr_csr.status) &
>> +                                               MPC52xx_PSC_SR_DATA_VAL))
>> +               udelay(10);
>
> ditto.
>
>> +static int psc_ac97_cold_reset_check(struct snd_ac97 *ac97)
>> +{
>> +       int max_reset, timeout;
>> +       struct mpc52xx_psc __iomem *regs = psc_dma->psc_regs;
>> +
>> +       /* AC97 clock is generated by the codec.
>> +        * Ensure that it starts ticking after codec reset.
>> +        */
>> +       for (max_reset = 0; max_reset < 5; max_reset++) {
>> +
>> +               /* Do a cold reset */
>> +               out_8(&regs->op1, MPC52xx_PSC_OP_RES);
>> +               udelay(10);
>> +               out_8(&regs->op0, MPC52xx_PSC_OP_RES);
>> +               udelay(50);
>
> :-/  Don't like, but don't know if there is an alternative.
>
>> +
>> +               /* PSC recover from cold reset
>> +                * (cfr user manual, not sure if useful)
>> +                */
>> +               out_be32(&regs->sicr, in_be32(&regs->sicr));
>> +
>> +               psc_ac97_warm_reset(ac97);
>> +
>> +               /* first make sure AC97 clock is low */
>> +               for (timeout = 0; ((in_8(&regs->ipcr_acr.ipcr) & 0x80) != 0) &&
>> +                               (timeout < 100); timeout++)
>> +                       udelay(10);
>> +               if (timeout == 100)
>> +                       continue;
>> +
>> +               /* then wait for the transition to high */
>> +               for (timeout = 0; ((in_8(&regs->ipcr_acr.ipcr) & 0x80) == 0) &&
>> +                               (timeout < 100); timeout++)
>> +                       udelay(10);
>> +               if (timeout == 100)
>> +                       continue;
>
> Using udelay makes this less accurate.  Only possible reason to use a
> udelay is if the register cannot be polled at full speed (which is
> possibly the case if it adds bus contention; but I don't think it is
> an issue here).
>
> g.
>
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>



-- 
Jon Smirl
jonsmirl at gmail.com