On Sun, May 24, 2009 at 09:38:49PM -0400, Jon Smirl wrote:
> I've implemented retries for when the AC97 hardware doesn't reset on
> first try. About 10% of the time both the Efika and pcm030 AC97 codecs
> don't reset on first try and need to be poked multiple times. Failure
> is indicated by not having the link clock start ticking. Every once in
> a while even five pokes won't get the link started and I have to power
> cycle.
This smells like either a very broken board or some issue with starting
the master clock for the CODEC - if the CODEC is clocked by the AC97
controller you may need to do something to ensure that it has finished
starting up before initiating the reset.
> +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.
> + */
The AC97 standard allows CODECs to come out of cold reset with the link
disabled. With those CODECs this is going fail every time - they need a
warm reset to come on-line.
If this really is a general issue with the AC97 controller here you'll
need to do a warm reset in here. It's not ideal but extra warm resets
will cause less harm than completely failing to come on-line.
> +static int psc_ac97_trigger(struct snd_pcm_substream *substream, int cmd,
> + struct snd_soc_dai *dai)
I keep mentioning the indentation issues with your code without seeing
any response from you. If you run checkpatch over your code you'll also
see a bunch of complaints about using spaces instead of tabs for
indentation. It looks for all the world like you're using 4 character
tabs instead of the 8 character tabs which are the kernel standard.
> +
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_STOP:
> + if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
> + psc_dma->slots &= 0xFFFF0000;
> + else
> + psc_dma->slots &= 0x0000FFFF;
> +
> + spin_lock(&psc_dma->lock);
> + out_be32(&psc_dma->psc_regs->ac97_slots, psc_dma->slots);
> + spin_unlock(&psc_dma->lock);
> + break;
This locking looks wrong - I'd expect it to also cover the modification
of psc_dma->slots? Otherwise it's hard to see what it buys you.
> + /* AC97 clock is generated by the codec.
> + * Ensure that it starts ticking after codec reset.
> + */
> + rc = psc_ac97_cold_reset_check(&ac97);
> + if (rc != 0) {
> + dev_err(&op->dev, "AC97 codec failed to reset\n");
> + mpc5200_audio_dma_destroy(op);
> + return rc;
> + }
Your AC97 driver should not be doing this - leave it to the card and
CODEC driver to bring things on line.
> +
> + /* Go */
> + out_8(®s->command, MPC52xx_PSC_TX_ENABLE | MPC52xx_PSC_RX_ENABLE);
As I said last time I'd expect this to be deferred to the ASoC device
probe.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
