[alsa-devel] [RFC PATCH] ALSA: hda - Fix CORB reset to follow specification

[Takashi Iwai]

At Fri, 28 Feb 2014 07:56:58 +0100,
David Henningsson wrote:
> 
> According to the HDA spec, we must write 1 to bit 15 on a CORBRP
> reset, read back 1, then write 0, then read back 0. This must be
> done while the DMA is not running.
> 
> We accidentaly ended up writing back the 0 by using a writel
> instead of a writew to CORBWP.
> 
> This caused occasional controller failure on Bay Trail hardware.
> 
> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> ---
>  sound/pci/hda/hda_intel.c |   24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> Hi Takashi,
> 
> A while ago I researched a spurious response on some Bay Trail hardware,
> so I proofread some of our CORB/RIRB code and found the below.
> 
> When applied, the spurious responses disappeared but other problems might
> have appeared instead. I don't have the hardware here so I'm not 100% sure.
> So I'm hesitant to actually apply it, especially as the existing non-compliant
> way seems to work for all other chips. Still wanted to show the issue to you.
> 
> Also the timeout (1 ms) is completely arbitrarily as there are no time limits
> written in the HDA specification.

OK, the changes look good to me.  I'm going to apply it once after
checking it with some other chips.


thanks,

Takashi

> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index d8d9bf3..59156f1 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -775,6 +775,8 @@ static int azx_alloc_cmd_io(struct azx *chip)
>  
>  static void azx_init_cmd_io(struct azx *chip)
>  {
> +	int timeout;
> +
>  	spin_lock_irq(&chip->reg_lock);
>  	/* CORB set up */
>  	chip->corb.addr = chip->rb.addr;
> @@ -786,8 +788,28 @@ static void azx_init_cmd_io(struct azx *chip)
>  	azx_writeb(chip, CORBSIZE, 0x02);
>  	/* set the corb write pointer to 0 */
>  	azx_writew(chip, CORBWP, 0);
> +
>  	/* reset the corb hw read pointer */
>  	azx_writew(chip, CORBRP, ICH6_CORBRP_RST);
> +	for (timeout = 1000; timeout > 0; timeout--) {
> +		if ((azx_readw(chip, CORBRP) & ICH6_CORBRP_RST) == ICH6_CORBRP_RST)
> +			break;
> +		udelay(1);
> +	}
> +	if (timeout <= 0)
> +		snd_printk(KERN_ERR SFX "%s: CORB reset timeout - CORBRP = %d\n",
> +			   pci_name(chip->pci), (int) azx_readw(chip, CORBRP));
> +
> +	azx_writew(chip, CORBRP, 0);
> +	for (timeout = 1000; timeout > 0; timeout--) {
> +		if (azx_readw(chip, CORBRP) == 0)
> +			break;
> +		udelay(1);
> +	}
> +	if (timeout <= 0)
> +		snd_printk(KERN_ERR SFX "%s: CORB reset timeout, CORBRP = %d\n",
> +			   pci_name(chip->pci), azx_readw(chip, CORBRP));
> +
>  	/* enable corb dma */
>  	azx_writeb(chip, CORBCTL, ICH6_CORBCTL_RUN);
>  
> @@ -862,7 +884,7 @@ static int azx_corb_send_cmd(struct hda_bus *bus, u32 val)
>  
>  	chip->rirb.cmds[addr]++;
>  	chip->corb.buf[wp] = cpu_to_le32(val);
> -	azx_writel(chip, CORBWP, wp);
> +	azx_writew(chip, CORBWP, wp);
>  
>  	spin_unlock_irq(&chip->reg_lock);
>  
> -- 
> 1.7.9.5
>