[PATCH 1/2] ALSA: hda: fix the missing ptr initialization

[PATCH 1/2] ALSA: hda: fix the missing ptr initialization

[Vinod Koul]

ebus is a member of extended device and was never initialized, so
do this at device creation.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/hda/ext/hdac_ext_bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
index 2433f7c81472..64de0a3d6d93 100644
--- a/sound/hda/ext/hdac_ext_bus.c
+++ b/sound/hda/ext/hdac_ext_bus.c
@@ -144,6 +144,7 @@ int snd_hdac_ext_bus_device_init(struct hdac_ext_bus *ebus, int addr)
 	if (!edev)
 		return -ENOMEM;
 	hdev = &edev->hdac;
+	edev->ebus = ebus;
 
 	snprintf(name, sizeof(name), "ehdaudio%dD%d", ebus->idx, addr);
 
-- 
1.9.1

[PATCH 2/2] ALSA: hda: fix to wait for RIRB & CORB DMA to set

[Vinod Koul]

From: Jeeja KP <jeeja.kp@intel.com>

After setting the stop bit of RIRB/CORB DMA, we should wait for
stop bit to be set.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/hda/hdac_controller.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index 8c486235c905..cc020c1706e7 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -80,6 +80,35 @@ void snd_hdac_bus_init_cmd_io(struct hdac_bus *bus)
 }
 EXPORT_SYMBOL_GPL(snd_hdac_bus_init_cmd_io);
 
+
+/* Stop CORB DMA and poll till they are stopped */
+static void hdac_stop_corb_dma(struct hdac_bus *bus)
+{
+	unsigned long timeout;
+
+	snd_hdac_chip_writew(bus, CORBCTL, 0);
+
+	udelay(3);
+	timeout = jiffies + msecs_to_jiffies(100);
+	while ((snd_hdac_chip_readb(bus, CORBCTL) & AZX_CORBCTL_RUN)
+		&& time_before(jiffies, timeout))
+		udelay(10);
+}
+
+/* Stop RIRB DMA and poll till they are stopped */
+static void hdac_stop_rirb_dma(struct hdac_bus *bus)
+{
+	unsigned long timeout;
+
+	snd_hdac_chip_writew(bus, RIRBCTL, 0);
+
+	udelay(3);
+	timeout = jiffies + msecs_to_jiffies(100);
+	while ((snd_hdac_chip_readb(bus, RIRBCTL) & AZX_RBCTL_DMA_EN)
+		&& time_before(jiffies, timeout))
+		udelay(10);
+}
+
 /**
  * snd_hdac_bus_stop_cmd_io - clean up CORB/RIRB buffers
  * @bus: HD-audio core bus
@@ -88,8 +117,8 @@ void snd_hdac_bus_stop_cmd_io(struct hdac_bus *bus)
 {
 	spin_lock_irq(&bus->reg_lock);
 	/* disable ringbuffer DMAs */
-	snd_hdac_chip_writeb(bus, RIRBCTL, 0);
-	snd_hdac_chip_writeb(bus, CORBCTL, 0);
+	hdac_stop_rirb_dma(bus);
+	hdac_stop_corb_dma(bus);
 	/* disable unsolicited responses */
 	snd_hdac_chip_updatel(bus, GCTL, AZX_GCTL_UNSOL, 0);
 	spin_unlock_irq(&bus->reg_lock);
-- 
1.9.1

Re: [PATCH 2/2] ALSA: hda: fix to wait for RIRB & CORB DMA to set

[Takashi Iwai]

On Thu, 28 Apr 2016 15:06:05 +0200,
Vinod Koul wrote:
> 
> From: Jeeja KP <jeeja.kp@intel.com>
> 
> After setting the stop bit of RIRB/CORB DMA, we should wait for
> stop bit to be set.

What does this actually fix?

 
> Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  sound/hda/hdac_controller.c | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
> index 8c486235c905..cc020c1706e7 100644
> --- a/sound/hda/hdac_controller.c
> +++ b/sound/hda/hdac_controller.c
> @@ -80,6 +80,35 @@ void snd_hdac_bus_init_cmd_io(struct hdac_bus *bus)
>  }
>  EXPORT_SYMBOL_GPL(snd_hdac_bus_init_cmd_io);
>  
> +
> +/* Stop CORB DMA and poll till they are stopped */
> +static void hdac_stop_corb_dma(struct hdac_bus *bus)
> +{
> +	unsigned long timeout;
> +
> +	snd_hdac_chip_writew(bus, CORBCTL, 0);
> +
> +	udelay(3);
> +	timeout = jiffies + msecs_to_jiffies(100);
> +	while ((snd_hdac_chip_readb(bus, CORBCTL) & AZX_CORBCTL_RUN)
> +		&& time_before(jiffies, timeout))
> +		udelay(10);
> +}
> +
> +/* Stop RIRB DMA and poll till they are stopped */
> +static void hdac_stop_rirb_dma(struct hdac_bus *bus)
> +{
> +	unsigned long timeout;
> +
> +	snd_hdac_chip_writew(bus, RIRBCTL, 0);
> +
> +	udelay(3);
> +	timeout = jiffies + msecs_to_jiffies(100);
> +	while ((snd_hdac_chip_readb(bus, RIRBCTL) & AZX_RBCTL_DMA_EN)
> +		&& time_before(jiffies, timeout))
> +		udelay(10);
> +}
> +
>  /**
>   * snd_hdac_bus_stop_cmd_io - clean up CORB/RIRB buffers
>   * @bus: HD-audio core bus
> @@ -88,8 +117,8 @@ void snd_hdac_bus_stop_cmd_io(struct hdac_bus *bus)
>  {
>  	spin_lock_irq(&bus->reg_lock);
>  	/* disable ringbuffer DMAs */
> -	snd_hdac_chip_writeb(bus, RIRBCTL, 0);
> -	snd_hdac_chip_writeb(bus, CORBCTL, 0);
> +	hdac_stop_rirb_dma(bus);
> +	hdac_stop_corb_dma(bus);

Doing these one after another sequentially is a waste of time.
Clear two once, then sync them.


thanks,

Takashi

Re: [PATCH 2/2] ALSA: hda: fix to wait for RIRB & CORB DMA to set

[Takashi Iwai]

On Thu, 28 Apr 2016 15:55:54 +0200,
Vinod Koul wrote:
> 
> On Thu, Apr 28, 2016 at 03:16:24PM +0200, Takashi Iwai wrote:
> > On Thu, 28 Apr 2016 15:06:05 +0200,
> > Vinod Koul wrote:
> > > 
> > > From: Jeeja KP <jeeja.kp@intel.com>
> > > 
> > > After setting the stop bit of RIRB/CORB DMA, we should wait for
> > > stop bit to be set.
> > 
> > What does this actually fix?
> 
> We some some stablity issues on SKL that were attributed to DMAs not being
> quisced properly so recommendation was to to wait till DMAs are stopped

Then please write it up in the changelog.

And, did it actually improve the stability?  That's the biggest
question :)


> > > @@ -88,8 +117,8 @@ void snd_hdac_bus_stop_cmd_io(struct hdac_bus *bus)
> > >  {
> > >  	spin_lock_irq(&bus->reg_lock);
> > >  	/* disable ringbuffer DMAs */
> > > -	snd_hdac_chip_writeb(bus, RIRBCTL, 0);
> > > -	snd_hdac_chip_writeb(bus, CORBCTL, 0);
> > > +	hdac_stop_rirb_dma(bus);
> > > +	hdac_stop_corb_dma(bus);
> > 
> > Doing these one after another sequentially is a waste of time.
> > Clear two once, then sync them.
> 
> Yes agreed, that sounds better, something like adding a new wait function:
> 
> 	hdac_stop_rirb_dma(bus);
> 	hdac_stop_corb_dma(bus);
> 	hdac_wait_for_cmd_dmas(bus);

Yep, that looks better.


thanks,

Takashi

Re: [PATCH 2/2] ALSA: hda: fix to wait for RIRB & CORB DMA to set

[Vinod Koul]

On Thu, Apr 28, 2016 at 03:16:24PM +0200, Takashi Iwai wrote:
> On Thu, 28 Apr 2016 15:06:05 +0200,
> Vinod Koul wrote:
> > 
> > From: Jeeja KP <jeeja.kp@intel.com>
> > 
> > After setting the stop bit of RIRB/CORB DMA, we should wait for
> > stop bit to be set.
> 
> What does this actually fix?

We some some stablity issues on SKL that were attributed to DMAs not being
quisced properly so recommendation was to to wait till DMAs are stopped

> > @@ -88,8 +117,8 @@ void snd_hdac_bus_stop_cmd_io(struct hdac_bus *bus)
> >  {
> >  	spin_lock_irq(&bus->reg_lock);
> >  	/* disable ringbuffer DMAs */
> > -	snd_hdac_chip_writeb(bus, RIRBCTL, 0);
> > -	snd_hdac_chip_writeb(bus, CORBCTL, 0);
> > +	hdac_stop_rirb_dma(bus);
> > +	hdac_stop_corb_dma(bus);
> 
> Doing these one after another sequentially is a waste of time.
> Clear two once, then sync them.

Yes agreed, that sounds better, something like adding a new wait function:

	hdac_stop_rirb_dma(bus);
	hdac_stop_corb_dma(bus);
	hdac_wait_for_cmd_dmas(bus);


Thanks
-- 
~Vinod

Re: [PATCH 2/2] ALSA: hda: fix to wait for RIRB & CORB DMA to set

[Vinod Koul]

On Thu, Apr 28, 2016 at 03:53:16PM +0200, Takashi Iwai wrote:
> On Thu, 28 Apr 2016 15:55:54 +0200,
> Vinod Koul wrote:
> > 
> > On Thu, Apr 28, 2016 at 03:16:24PM +0200, Takashi Iwai wrote:
> > > On Thu, 28 Apr 2016 15:06:05 +0200,
> > > Vinod Koul wrote:
> > > > 
> > > > From: Jeeja KP <jeeja.kp@intel.com>
> > > > 
> > > > After setting the stop bit of RIRB/CORB DMA, we should wait for
> > > > stop bit to be set.
> > > 
> > > What does this actually fix?
> > 
> > We some some stablity issues on SKL that were attributed to DMAs not being
> > quisced properly so recommendation was to to wait till DMAs are stopped
> 
> Then please write it up in the changelog.

Sure thing

> 
> And, did it actually improve the stability?  That's the biggest
> question :)

Yes it did :)

> > > > @@ -88,8 +117,8 @@ void snd_hdac_bus_stop_cmd_io(struct hdac_bus *bus)
> > > >  {
> > > >  	spin_lock_irq(&bus->reg_lock);
> > > >  	/* disable ringbuffer DMAs */
> > > > -	snd_hdac_chip_writeb(bus, RIRBCTL, 0);
> > > > -	snd_hdac_chip_writeb(bus, CORBCTL, 0);
> > > > +	hdac_stop_rirb_dma(bus);
> > > > +	hdac_stop_corb_dma(bus);
> > > 
> > > Doing these one after another sequentially is a waste of time.
> > > Clear two once, then sync them.
> > 
> > Yes agreed, that sounds better, something like adding a new wait function:
> > 
> > 	hdac_stop_rirb_dma(bus);
> > 	hdac_stop_corb_dma(bus);
> > 	hdac_wait_for_cmd_dmas(bus);
> 
> Yep, that looks better.

Okay will update and post v2

-- 
~Vinod