| Message ID | 1393570618-13442-1-git-send-email-david.henningsson@canonical.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 >
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);
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.