Re: [PATCH V3 2/4] AC97 driver for mpc5200

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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(&regs->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.