From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qy0-f196.google.com (mail-qy0-f196.google.com [209.85.221.196]) by ozlabs.org (Postfix) with ESMTP id 53074DE105 for ; Tue, 26 May 2009 01:15:36 +1000 (EST) Received: by qyk34 with SMTP id 34so5412762qyk.17 for ; Mon, 25 May 2009 08:15:35 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <20090525013606.3073.86753.stgit@terra> <20090525013849.3073.96729.stgit@terra> Date: Mon, 25 May 2009 11:15:34 -0400 Message-ID: <9e4733910905250815r4a6fa205s26531f08fe99131a@mail.gmail.com> Subject: Re: [PATCH V3 2/4] AC97 driver for mpc5200 From: Jon Smirl To: Grant Likely Content-Type: text/plain; charset=ISO-8859-1 Cc: linuxppc-dev@ozlabs.org, alsa-devel@alsa-project.org, broonie@sirena.org.uk List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Mon, May 25, 2009 at 2:16 AM, Grant Likely w= rote: > On Sun, May 24, 2009 at 7:38 PM, Jon Smirl wrote: >> +static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned sho= rt reg) >> +{ >> + =A0 =A0 =A0 int timeout; >> + =A0 =A0 =A0 unsigned int val; >> + >> + =A0 =A0 =A0 spin_lock(&psc_dma->lock); >> + >> + =A0 =A0 =A0 /* Wait for it to be ready */ >> + =A0 =A0 =A0 timeout =3D 1000; >> + =A0 =A0 =A0 while ((--timeout) && (in_be16(&psc_dma->psc_regs->sr_csr.= status) & >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 MPC52xx_PSC_SR_CMDSEND)) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(10); > > Holy unbounded latency Batman! =A0This code waits up to 10ms for a regist= er 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? =A0If you > must spin; can a cpu_relax() be used instead of the udelay() while > watch the timebase? =A0Timur 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. > >> + >> + =A0 =A0 =A0 if (!timeout) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pr_err("timeout on ac97 bus (rdy)\n"); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0xffff; >> + =A0 =A0 =A0 } >> + >> + =A0 =A0 =A0 /* Do the read */ >> + =A0 =A0 =A0 out_be32(&psc_dma->psc_regs->ac97_cmd, (1<<31) | ((reg & 0= x7f) << 24)); >> + >> + =A0 =A0 =A0 /* Wait for the answer */ >> + =A0 =A0 =A0 timeout =3D 1000; >> + =A0 =A0 =A0 while ((--timeout) && !(in_be16(&psc_dma->psc_regs->sr_csr= .status) & >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 MPC52xx_PSC_SR_DATA_VAL)) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(10); > > ditto. > >> +static int psc_ac97_cold_reset_check(struct snd_ac97 *ac97) >> +{ >> + =A0 =A0 =A0 int max_reset, timeout; >> + =A0 =A0 =A0 struct mpc52xx_psc __iomem *regs =3D psc_dma->psc_regs; >> + >> + =A0 =A0 =A0 /* AC97 clock is generated by the codec. >> + =A0 =A0 =A0 =A0* Ensure that it starts ticking after codec reset. >> + =A0 =A0 =A0 =A0*/ >> + =A0 =A0 =A0 for (max_reset =3D 0; max_reset < 5; max_reset++) { >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Do a cold reset */ >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_8(®s->op1, MPC52xx_PSC_OP_RES); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(10); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_8(®s->op0, MPC52xx_PSC_OP_RES); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(50); > > :-/ =A0Don't like, but don't know if there is an alternative. > >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* PSC recover from cold reset >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* (cfr user manual, not sure if useful) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/ >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_be32(®s->sicr, in_be32(®s->sicr)= ); >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 psc_ac97_warm_reset(ac97); >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* first make sure AC97 clock is low */ >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (timeout =3D 0; ((in_8(®s->ipcr_acr= .ipcr) & 0x80) !=3D 0) && >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (timeout <= 100); timeout++) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(10); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (timeout =3D=3D 100) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue; >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* then wait for the transition to high */ >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (timeout =3D 0; ((in_8(®s->ipcr_acr= .ipcr) & 0x80) =3D=3D 0) && >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (timeout <= 100); timeout++) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(10); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (timeout =3D=3D 100) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue; > > Using udelay makes this less accurate. =A0Only 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. > --=20 Jon Smirl jonsmirl@gmail.com