[alsa-devel] [PATCH 0/4] ALSA: hda: Bug fixes
The series contains few bug fixes and debug updates
Pardha Saradhi K (4): ALSA: hda: Make sure DMA is stopped by reading back the RUN bit ALSA: hda: Make sure DMA is started by reading back the RUN bit ALSA: hda: Log HDA Hardware related errors ALSA: hda: check if stream is stopped in snd_hdac_stream_clear
sound/hda/hdac_stream.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
From: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com
As per HW recommendation, after clearing the RUN bit, software must read a 0 from the RUN bit, before modifying related control registers or re-starting the DMA engine.
Signed-off-by: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com Signed-off-by: Sriram Periyasamy sriramx.periyasamy@intel.com --- sound/hda/hdac_stream.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index e1472c7ab6c1..2000ea6f48fa 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -70,8 +70,23 @@ EXPORT_SYMBOL_GPL(snd_hdac_stream_start); */ void snd_hdac_stream_clear(struct hdac_stream *azx_dev) { + int timeout = 300; + unsigned char val; + snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_DMA_START | SD_INT_MASK, 0); + + do { + udelay(3); + val = snd_hdac_stream_readb(azx_dev, SD_CTL) & + SD_CTL_DMA_START; + if (!val) + break; + } while (--timeout); + + if (!timeout) + dev_err(azx_dev->bus->dev, "unable to stop the stream\n"); + snd_hdac_stream_writeb(azx_dev, SD_STS, SD_INT_MASK); /* to be sure */ azx_dev->running = false; }
On Tue, 15 May 2018 08:40:53 +0200, Sriram Periyasamy wrote:
From: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com
As per HW recommendation, after clearing the RUN bit, software must read a 0 from the RUN bit, before modifying related control registers or re-starting the DMA engine.
This is already done in snd_hdac_stream_sync().
Doing this in snd_hdac_stream_clear() may delay the sync'ed operations unnecessarily.
thanks,
Takashi
Signed-off-by: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com Signed-off-by: Sriram Periyasamy sriramx.periyasamy@intel.com
sound/hda/hdac_stream.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index e1472c7ab6c1..2000ea6f48fa 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -70,8 +70,23 @@ EXPORT_SYMBOL_GPL(snd_hdac_stream_start); */ void snd_hdac_stream_clear(struct hdac_stream *azx_dev) {
- int timeout = 300;
- unsigned char val;
- snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_DMA_START | SD_INT_MASK, 0);
- do {
udelay(3);val = snd_hdac_stream_readb(azx_dev, SD_CTL) &SD_CTL_DMA_START;if (!val)break;- } while (--timeout);
- if (!timeout)
dev_err(azx_dev->bus->dev, "unable to stop the stream\n");- snd_hdac_stream_writeb(azx_dev, SD_STS, SD_INT_MASK); /* to be sure */ azx_dev->running = false;
}
2.7.4
From: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com
As per HW recommendation, after setting the RUN bit, software must read a 1 from the RUN bit, before modifying related control registers/re-starting the DMA engine.
Signed-off-by: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com Signed-off-by: Sriram Periyasamy sriramx.periyasamy@intel.com --- sound/hda/hdac_stream.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index 2000ea6f48fa..33c8ced528f6 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -48,6 +48,8 @@ EXPORT_SYMBOL_GPL(snd_hdac_stream_init); void snd_hdac_stream_start(struct hdac_stream *azx_dev, bool fresh_start) { struct hdac_bus *bus = azx_dev->bus; + int timeout = 300; + unsigned char val;
trace_snd_hdac_stream_start(bus, azx_dev);
@@ -60,6 +62,20 @@ void snd_hdac_stream_start(struct hdac_stream *azx_dev, bool fresh_start) /* set DMA start and interrupt mask */ snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_DMA_START | SD_INT_MASK); + + do { + udelay(3); + val = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START; + if (val) + break; + } while (--timeout); + + if (!timeout) { + azx_dev->running = false; + dev_err(azx_dev->bus->dev, "unable to start the stream\n"); + return; + } + azx_dev->running = true; } EXPORT_SYMBOL_GPL(snd_hdac_stream_start);
On Tue, 15 May 2018 08:40:54 +0200, Sriram Periyasamy wrote:
From: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com
As per HW recommendation, after setting the RUN bit, software must read a 1 from the RUN bit, before modifying related control registers/re-starting the DMA engine.
Rather FIFO is checked in snd_hdac_stream_sync(), so I guess it already suffices. But we can add this sanity check there, too, if it really matters.
thanks,
Takashi
From: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com
Detect the timeout while modifying HDA DMA related Registers for stream reset and print them to console for user information
Signed-off-by: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com Signed-off-by: Sriram Periyasamy sriramx.periyasamy@intel.com --- sound/hda/hdac_stream.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index 33c8ced528f6..aa8a2884289f 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -144,6 +144,10 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) if (val) break; } while (--timeout); + + if (!timeout) + dev_err(azx_dev->bus->dev, "timeout on stream reset entry\n"); + val &= ~SD_CTL_STREAM_RESET; snd_hdac_stream_writeb(azx_dev, SD_CTL, val); udelay(3); @@ -157,6 +161,9 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) break; } while (--timeout);
+ if (!timeout) + dev_err(azx_dev->bus->dev, "timeout on stream reset exit\n"); + /* reset first position - may not be synced with hw at this time */ if (azx_dev->posbuf) *azx_dev->posbuf = 0;
On Tue, 15 May 2018 08:40:55 +0200, Sriram Periyasamy wrote:
From: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com
Detect the timeout while modifying HDA DMA related Registers for stream reset and print them to console for user information
Signed-off-by: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com Signed-off-by: Sriram Periyasamy sriramx.periyasamy@intel.com
sound/hda/hdac_stream.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index 33c8ced528f6..aa8a2884289f 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -144,6 +144,10 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) if (val) break; } while (--timeout);
- if (!timeout)
dev_err(azx_dev->bus->dev, "timeout on stream reset entry\n");- val &= ~SD_CTL_STREAM_RESET; snd_hdac_stream_writeb(azx_dev, SD_CTL, val); udelay(3);
@@ -157,6 +161,9 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) break; } while (--timeout);
- if (!timeout)
dev_err(azx_dev->bus->dev, "timeout on stream reset exit\n");
Just from curiosity: did you ever hit these? Or just because of consistency?
Takashi
From: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com
Check if the DMA Channel is already stopped. There is no need to stop it again if stopped.
Signed-off-by: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com Signed-off-by: Sriram Periyasamy sriramx.periyasamy@intel.com --- sound/hda/hdac_stream.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index aa8a2884289f..c1810eb7212b 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -89,6 +89,11 @@ void snd_hdac_stream_clear(struct hdac_stream *azx_dev) int timeout = 300; unsigned char val;
+ /* check if the DMA is already stopped */ + val = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START; + if (!val) + return; + snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_DMA_START | SD_INT_MASK, 0);
On Tue, 15 May 2018 08:40:56 +0200, Sriram Periyasamy wrote:
From: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com
Check if the DMA Channel is already stopped. There is no need to stop it again if stopped.
Signed-off-by: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com Signed-off-by: Sriram Periyasamy sriramx.periyasamy@intel.com
sound/hda/hdac_stream.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index aa8a2884289f..c1810eb7212b 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -89,6 +89,11 @@ void snd_hdac_stream_clear(struct hdac_stream *azx_dev) int timeout = 300; unsigned char val;
- /* check if the DMA is already stopped */
- val = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START;
- if (!val)
return;- snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_DMA_START | SD_INT_MASK, 0);
Here we call snd_hdac_stream_updateb(), so we don't write unnecessarily.
And, if any shortcut is wanted, rather check azx_dev->running flag instead of the h/w register. The former is significantly cheaper.
thanks,
Takashi
participants (2)
-
Sriram Periyasamy -
Takashi Iwai