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

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

[David Henningsson]

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@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.

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

Re: [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@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
>