First two patches are small refactoring of code to use poll macros instead of open coding register checks. Following two patches remove unused defines from code.
v2: * Fix includes
Amadeusz Sławiński (4): ALSA: hda: Move stream-register polling macros ALSA: hda: Rework snd_hdac_stream_reset() to use macros ALSA: hda: Remove unused MAX_PIN_CONFIGS constant ALSA: hda: Remove unused defines
include/sound/hdaudio.h | 7 +++++++ include/sound/hdaudio_ext.h | 6 ------ sound/hda/hdac_stream.c | 26 ++++++-------------------- sound/pci/hda/hda_intel.c | 7 ------- sound/pci/hda/hda_sysfs.c | 2 -- 5 files changed, 13 insertions(+), 35 deletions(-)
Polling stream registers doesn't really have anything to do with extended HDA registers, so move it to basic HDA header. This will allow for use in HDA framework.
Reviewed-by: Cezary Rojewski cezary.rojewski@intel.com Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- include/sound/hdaudio.h | 7 +++++++ include/sound/hdaudio_ext.h | 6 ------ 2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 797bf67a164d..6e74aeafeda4 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -10,6 +10,7 @@ #include <linux/interrupt.h> #include <linux/io.h> #include <linux/io-64-nonatomic-lo-hi.h> +#include <linux/iopoll.h> #include <linux/pm_runtime.h> #include <linux/timecounter.h> #include <sound/core.h> @@ -589,6 +590,12 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, snd_hdac_reg_readw((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readb(dev, reg) \ snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) +#define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \ + readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ + delay_us, timeout_us) +#define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, timeout_us) \ + readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ + delay_us, timeout_us)
/* update a register, pass without AZX_REG_ prefix */ #define snd_hdac_stream_updatel(dev, reg, mask, val) \ diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h index d26234f9ee46..03634ea198d0 100644 --- a/include/sound/hdaudio_ext.h +++ b/include/sound/hdaudio_ext.h @@ -188,12 +188,6 @@ void snd_hdac_ext_bus_link_power(struct hdac_device *codec, bool enable); #define snd_hdac_adsp_readq_poll(chip, reg, val, cond, delay_us, timeout_us) \ readq_poll_timeout((chip)->dsp_ba + (reg), val, cond, \ delay_us, timeout_us) -#define snd_hdac_stream_readb_poll(strm, reg, val, cond, delay_us, timeout_us) \ - readb_poll_timeout((strm)->sd_addr + AZX_REG_ ## reg, val, cond, \ - delay_us, timeout_us) -#define snd_hdac_stream_readl_poll(strm, reg, val, cond, delay_us, timeout_us) \ - readl_poll_timeout((strm)->sd_addr + AZX_REG_ ## reg, val, cond, \ - delay_us, timeout_us)
struct hdac_ext_device;
We can use existing macros to poll and update register values instead of open coding the functionality.
Reviewed-by: Cezary Rojewski cezary.rojewski@intel.com Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/hda/hdac_stream.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index f3582012d22f..bdf6d4db6769 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -165,7 +165,6 @@ EXPORT_SYMBOL_GPL(snd_hdac_stop_streams_and_chip); void snd_hdac_stream_reset(struct hdac_stream *azx_dev) { unsigned char val; - int timeout; int dma_run_state;
snd_hdac_stream_clear(azx_dev); @@ -173,30 +172,17 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) dma_run_state = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START;
snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET); - udelay(3); - timeout = 300; - do { - val = snd_hdac_stream_readb(azx_dev, SD_CTL) & - SD_CTL_STREAM_RESET; - if (val) - break; - } while (--timeout); + + /* wait for hardware to report that the stream entered reset */ + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & SD_CTL_STREAM_RESET), 3, 300);
if (azx_dev->bus->dma_stop_delay && dma_run_state) udelay(azx_dev->bus->dma_stop_delay);
- val &= ~SD_CTL_STREAM_RESET; - snd_hdac_stream_writeb(azx_dev, SD_CTL, val); - udelay(3); + snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_STREAM_RESET, 0);
- timeout = 300; - /* waiting for hardware to report that the stream is out of reset */ - do { - val = snd_hdac_stream_readb(azx_dev, SD_CTL) & - SD_CTL_STREAM_RESET; - if (!val) - break; - } while (--timeout); + /* wait for hardware to report that the stream is out of reset */ + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & SD_CTL_STREAM_RESET), 3, 300);
/* reset first position - may not be synced with hw at this time */ if (azx_dev->posbuf)
On 18/08/2022 15:15, Amadeusz Sławiński wrote:
We can use existing macros to poll and update register values instead of open coding the functionality.
Reviewed-by: Cezary Rojewski cezary.rojewski@intel.com Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
sound/hda/hdac_stream.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index f3582012d22f..bdf6d4db6769 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -165,7 +165,6 @@ EXPORT_SYMBOL_GPL(snd_hdac_stop_streams_and_chip); void snd_hdac_stream_reset(struct hdac_stream *azx_dev) { unsigned char val;
int timeout; int dma_run_state;
snd_hdac_stream_clear(azx_dev);
@@ -173,30 +172,17 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) dma_run_state = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START;
snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET);
- udelay(3);
- timeout = 300;
- do {
val = snd_hdac_stream_readb(azx_dev, SD_CTL) &SD_CTL_STREAM_RESET;if (val)break;- } while (--timeout);
/* wait for hardware to report that the stream entered reset */
snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & SD_CTL_STREAM_RESET), 3, 300);
if (azx_dev->bus->dma_stop_delay && dma_run_state) udelay(azx_dev->bus->dma_stop_delay);
- val &= ~SD_CTL_STREAM_RESET;
- snd_hdac_stream_writeb(azx_dev, SD_CTL, val);
- udelay(3);
- snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_STREAM_RESET, 0);
- timeout = 300;
- /* waiting for hardware to report that the stream is out of reset */
- do {
val = snd_hdac_stream_readb(azx_dev, SD_CTL) &SD_CTL_STREAM_RESET;if (!val)break;- } while (--timeout);
/* wait for hardware to report that the stream is out of reset */
snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & SD_CTL_STREAM_RESET), 3, 300);
/* reset first position - may not be synced with hw at this time */ if (azx_dev->posbuf)
HDA playback is failing on -next for various Tegra boards. Bisect is point to this commit and reverting it fixes the problem. I was a bit puzzled why this change is causing a problem, but looking closer there is a difference between the previous code that was calling snd_hdac_stream_readb() and the new code that is calling snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO which Tegra does and then would call snd_hdac_aligned_read(). However, now the code always call readb() and this is breaking Tegra.
So it is either necessary to update snd_hdac_stream_readb_poll() to handle this or revert this change.
Cheers Jon
On Wed, 05 Oct 2022 14:10:04 +0200, Jon Hunter wrote:
On 18/08/2022 15:15, Amadeusz Sławiński wrote:
We can use existing macros to poll and update register values instead of open coding the functionality.
Reviewed-by: Cezary Rojewski cezary.rojewski@intel.com Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
sound/hda/hdac_stream.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index f3582012d22f..bdf6d4db6769 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -165,7 +165,6 @@ EXPORT_SYMBOL_GPL(snd_hdac_stop_streams_and_chip); void snd_hdac_stream_reset(struct hdac_stream *azx_dev) { unsigned char val;
- int timeout; int dma_run_state; snd_hdac_stream_clear(azx_dev);
@@ -173,30 +172,17 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) dma_run_state = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START; snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET);
- udelay(3);
- timeout = 300;
- do {
val = snd_hdac_stream_readb(azx_dev, SD_CTL) &SD_CTL_STREAM_RESET;if (val)break;- } while (--timeout);
- /* wait for hardware to report that the stream entered reset */
- snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & SD_CTL_STREAM_RESET), 3, 300); if (azx_dev->bus->dma_stop_delay && dma_run_state) udelay(azx_dev->bus->dma_stop_delay);
- val &= ~SD_CTL_STREAM_RESET;
- snd_hdac_stream_writeb(azx_dev, SD_CTL, val);
- udelay(3);
- snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_STREAM_RESET, 0);
- timeout = 300;
- /* waiting for hardware to report that the stream is out of reset */
- do {
val = snd_hdac_stream_readb(azx_dev, SD_CTL) &SD_CTL_STREAM_RESET;if (!val)break;- } while (--timeout);
- /* wait for hardware to report that the stream is out of reset */
- snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & SD_CTL_STREAM_RESET), 3, 300); /* reset first position - may not be synced with hw at this
time */ if (azx_dev->posbuf)
HDA playback is failing on -next for various Tegra boards. Bisect is point to this commit and reverting it fixes the problem. I was a bit puzzled why this change is causing a problem, but looking closer there is a difference between the previous code that was calling snd_hdac_stream_readb() and the new code that is calling snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO which Tegra does and then would call snd_hdac_aligned_read(). However, now the code always call readb() and this is breaking Tegra.
So it is either necessary to update snd_hdac_stream_readb_poll() to handle this or revert this change.
Does the patch below work?
thanks,
Takashi
-- 8< -- --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, #define snd_hdac_stream_readb(dev, reg) \ snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \ - readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ - delay_us, timeout_us) + read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\ + false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, timeout_us) \ readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ delay_us, timeout_us)
On 05/10/2022 13:29, Takashi Iwai wrote:
...
HDA playback is failing on -next for various Tegra boards. Bisect is point to this commit and reverting it fixes the problem. I was a bit puzzled why this change is causing a problem, but looking closer there is a difference between the previous code that was calling snd_hdac_stream_readb() and the new code that is calling snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO which Tegra does and then would call snd_hdac_aligned_read(). However, now the code always call readb() and this is breaking Tegra.
So it is either necessary to update snd_hdac_stream_readb_poll() to handle this or revert this change.
Does the patch below work?
thanks,
Takashi
-- 8< -- --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, #define snd_hdac_stream_readb(dev, reg) \ snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \
- readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \
delay_us, timeout_us)
- read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\
false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg)
Amazingly it does not work. I would have thought that would, but it does not. I am a bit puzzled by that?
Jon
On Wed, 05 Oct 2022 15:52:01 +0200, Jon Hunter wrote:
On 05/10/2022 13:29, Takashi Iwai wrote:
...
HDA playback is failing on -next for various Tegra boards. Bisect is point to this commit and reverting it fixes the problem. I was a bit puzzled why this change is causing a problem, but looking closer there is a difference between the previous code that was calling snd_hdac_stream_readb() and the new code that is calling snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO which Tegra does and then would call snd_hdac_aligned_read(). However, now the code always call readb() and this is breaking Tegra.
So it is either necessary to update snd_hdac_stream_readb_poll() to handle this or revert this change.
Does the patch below work?
thanks,
Takashi
-- 8< -- --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, #define snd_hdac_stream_readb(dev, reg) \ snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \
- readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \
delay_us, timeout_us)
- read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\
false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg)Amazingly it does not work. I would have thought that would, but it does not. I am a bit puzzled by that?
Interesting, it must be a subtle difference. What about passing true? It seems that the original code has the udelay(3) before the loop.
Takashi
-- 8< -- --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, #define snd_hdac_stream_readb(dev, reg) \ snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \ - readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ - delay_us, timeout_us) + read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\ + true, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, timeout_us) \ readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ delay_us, timeout_us)
On 05/10/2022 15:07, Takashi Iwai wrote:
On Wed, 05 Oct 2022 15:52:01 +0200, Jon Hunter wrote:
On 05/10/2022 13:29, Takashi Iwai wrote:
...
HDA playback is failing on -next for various Tegra boards. Bisect is point to this commit and reverting it fixes the problem. I was a bit puzzled why this change is causing a problem, but looking closer there is a difference between the previous code that was calling snd_hdac_stream_readb() and the new code that is calling snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO which Tegra does and then would call snd_hdac_aligned_read(). However, now the code always call readb() and this is breaking Tegra.
So it is either necessary to update snd_hdac_stream_readb_poll() to handle this or revert this change.
Does the patch below work?
thanks,
Takashi
-- 8< -- --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, #define snd_hdac_stream_readb(dev, reg) \ snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \
- readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \
delay_us, timeout_us)
- read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\
false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg)Amazingly it does not work. I would have thought that would, but it does not. I am a bit puzzled by that?
Interesting, it must be a subtle difference. What about passing true? It seems that the original code has the udelay(3) before the loop.
I wondered the same and tried that, but still not working.
Jon
On 10/5/2022 4:26 PM, Jon Hunter wrote:
On 05/10/2022 15:07, Takashi Iwai wrote:
On Wed, 05 Oct 2022 15:52:01 +0200, Jon Hunter wrote:
On 05/10/2022 13:29, Takashi Iwai wrote:
...
HDA playback is failing on -next for various Tegra boards. Bisect is point to this commit and reverting it fixes the problem. I was a bit puzzled why this change is causing a problem, but looking closer there is a difference between the previous code that was calling snd_hdac_stream_readb() and the new code that is calling snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO which Tegra does and then would call snd_hdac_aligned_read(). However, now the code always call readb() and this is breaking Tegra.
So it is either necessary to update snd_hdac_stream_readb_poll() to handle this or revert this change.
Does the patch below work?
thanks,
Takashi
-- 8< -- --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, #define snd_hdac_stream_readb(dev, reg) \ snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \ - readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ - delay_us, timeout_us) + read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\ + false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, timeout_us) \ readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ delay_us, timeout_us)
Amazingly it does not work. I would have thought that would, but it does not. I am a bit puzzled by that?
Interesting, it must be a subtle difference. What about passing true? It seems that the original code has the udelay(3) before the loop.
I wondered the same and tried that, but still not working.
Jon
Well in worse case we can revert the patch in question, but I would like to get it working...
Maybe also try to raise timeout to 1000, as what original code called timeout, was actually number of retries? So 300 * udelay(3) which is more or less 900us, so we can round it up for test?
I mean, something like:
--- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -176,7 +176,7 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET);
/* wait for hardware to report that the stream entered reset */ - snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & SD_CTL_STREAM_RESET), 3, 300); + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & SD_CTL_STREAM_RESET), 3, 1000);
if (azx_dev->bus->dma_stop_delay && dma_run_state) udelay(azx_dev->bus->dma_stop_delay); @@ -184,7 +184,7 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_STREAM_RESET, 0);
/* wait for hardware to report that the stream is out of reset */ - snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & SD_CTL_STREAM_RESET), 3, 300); + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & SD_CTL_STREAM_RESET), 3, 1000);
/* reset first position - may not be synced with hw at this time */ if (azx_dev->posbuf)
in addition to Takashi suggestion?
On 05/10/2022 15:47, Amadeusz Sławiński wrote:
...
Well in worse case we can revert the patch in question, but I would like to get it working...
Maybe also try to raise timeout to 1000, as what original code called timeout, was actually number of retries? So 300 * udelay(3) which is more or less 900us, so we can round it up for test?
I mean, something like:
--- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -176,7 +176,7 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET);
/* wait for hardware to report that the stream entered reset */ - snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & SD_CTL_STREAM_RESET), 3, 300); + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & SD_CTL_STREAM_RESET), 3, 1000);
if (azx_dev->bus->dma_stop_delay && dma_run_state) udelay(azx_dev->bus->dma_stop_delay); @@ -184,7 +184,7 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_STREAM_RESET, 0);
/* wait for hardware to report that the stream is out of reset */ - snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & SD_CTL_STREAM_RESET), 3, 300); + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & SD_CTL_STREAM_RESET), 3, 1000);
/* reset first position - may not be synced with hw at this time */ if (azx_dev->posbuf)
in addition to Takashi suggestion?
Thanks. Tried that on top of Takaski's patch but still not working :-(
Jon
On 05/10/2022 13:29, Takashi Iwai wrote:
...
HDA playback is failing on -next for various Tegra boards. Bisect is point to this commit and reverting it fixes the problem. I was a bit puzzled why this change is causing a problem, but looking closer there is a difference between the previous code that was calling snd_hdac_stream_readb() and the new code that is calling snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO which Tegra does and then would call snd_hdac_aligned_read(). However, now the code always call readb() and this is breaking Tegra.
So it is either necessary to update snd_hdac_stream_readb_poll() to handle this or revert this change.
Does the patch below work?
thanks,
Takashi
-- 8< -- --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, #define snd_hdac_stream_readb(dev, reg) \ snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \
- readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \
delay_us, timeout_us)
- read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\
false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg)
So looking at this a bit more I see ...
[ 199.422773] bad: scheduling from the idle thread! [ 199.427610] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D C 6.0.0-rc7-next-20220930-00007-gd6ae4ed0a78f-dirty #23 [ 199.438880] Hardware name: NVIDIA Jetson Nano Developer Kit (DT) [ 199.444899] Call trace: [ 199.447357] dump_backtrace.part.7+0xe8/0xf8 [ 199.451680] show_stack+0x14/0x38 [ 199.455024] dump_stack_lvl+0x64/0x7c [ 199.458715] dump_stack+0x14/0x2c [ 199.462067] dequeue_task_idle+0x2c/0x58 [ 199.466038] dequeue_task+0x38/0x98 [ 199.469565] __schedule+0x4a4/0x6d8 [ 199.473104] schedule+0x58/0xc0 [ 199.476292] schedule_hrtimeout_range_clock+0x98/0x108 [ 199.481472] schedule_hrtimeout_range+0x10/0x18 [ 199.486039] usleep_range_state+0x7c/0xb0 [ 199.490084] snd_hdac_stream_reset+0xe8/0x328 [snd_hda_core] [ 199.495913] snd_hdac_stream_sync+0xe4/0x190 [snd_hda_core] [ 199.501627] azx_pcm_trigger+0x1d8/0x250 [snd_hda_codec] [ 199.507109] snd_pcm_do_stop+0x54/0x70 [ 199.510904] snd_pcm_action_single+0x44/0xa0 [ 199.515215] snd_pcm_drain_done+0x20/0x28 [ 199.519267] snd_pcm_update_state+0x114/0x128 [ 199.523670] snd_pcm_update_hw_ptr0+0x22c/0x3a0 [ 199.528246] snd_pcm_period_elapsed_under_stream_lock+0x44/0x88 [ 199.534216] snd_pcm_period_elapsed+0x24/0x48 [ 199.538617] stream_update+0x3c/0x50 [snd_hda_codec] [ 199.543737] snd_hdac_bus_handle_stream_irq+0xe8/0x150 [snd_hda_core] [ 199.550320] azx_interrupt+0xb4/0x1b0 [snd_hda_codec] [ 199.555524] __handle_irq_event_percpu+0x74/0x140 [ 199.560281] handle_irq_event_percpu+0x14/0x48 [ 199.564772] handle_irq_event+0x44/0x88 [ 199.568653] handle_fasteoi_irq+0xa8/0x130 [ 199.572788] generic_handle_domain_irq+0x28/0x40 [ 199.577452] gic_handle_irq+0x9c/0xb8 [ 199.581168] call_on_irq_stack+0x2c/0x40 [ 199.585129] do_interrupt_handler+0x7c/0x80 [ 199.589355] el1_interrupt+0x34/0x68 [ 199.592969] el1h_64_irq_handler+0x14/0x20 [ 199.597107] el1h_64_irq+0x64/0x68 [ 199.600540] cpuidle_enter_state+0x130/0x2f8 [ 199.604853] cpuidle_enter+0x38/0x50 [ 199.608463] call_cpuidle+0x18/0x38 [ 199.611991] do_idle+0x1f8/0x248 [ 199.615259] cpu_startup_entry+0x20/0x28 [ 199.619224] kernel_init+0x0/0x128 [ 199.622669] arch_post_acpi_subsys_init+0x0/0x8 [ 199.627240] start_kernel+0x630/0x668 [ 199.630933] __primary_switched+0xb4/0xbc
If I change your patch to be read_poll_timeout_atomic, then it works \o/
Can we make that update?
Jon
On 10/6/2022 10:45 AM, Jon Hunter wrote:
On 05/10/2022 13:29, Takashi Iwai wrote:
...
HDA playback is failing on -next for various Tegra boards. Bisect is point to this commit and reverting it fixes the problem. I was a bit puzzled why this change is causing a problem, but looking closer there is a difference between the previous code that was calling snd_hdac_stream_readb() and the new code that is calling snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO which Tegra does and then would call snd_hdac_aligned_read(). However, now the code always call readb() and this is breaking Tegra.
So it is either necessary to update snd_hdac_stream_readb_poll() to handle this or revert this change.
Does the patch below work?
thanks,
Takashi
-- 8< -- --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, #define snd_hdac_stream_readb(dev, reg) \ snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \ - readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ - delay_us, timeout_us) + read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\ + false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, timeout_us) \ readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ delay_us, timeout_us)
So looking at this a bit more I see ...
[ 199.422773] bad: scheduling from the idle thread! [ 199.427610] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D C 6.0.0-rc7-next-20220930-00007-gd6ae4ed0a78f-dirty #23 [ 199.438880] Hardware name: NVIDIA Jetson Nano Developer Kit (DT) [ 199.444899] Call trace: [ 199.447357] dump_backtrace.part.7+0xe8/0xf8 [ 199.451680] show_stack+0x14/0x38 [ 199.455024] dump_stack_lvl+0x64/0x7c [ 199.458715] dump_stack+0x14/0x2c [ 199.462067] dequeue_task_idle+0x2c/0x58 [ 199.466038] dequeue_task+0x38/0x98 [ 199.469565] __schedule+0x4a4/0x6d8 [ 199.473104] schedule+0x58/0xc0 [ 199.476292] schedule_hrtimeout_range_clock+0x98/0x108 [ 199.481472] schedule_hrtimeout_range+0x10/0x18 [ 199.486039] usleep_range_state+0x7c/0xb0 [ 199.490084] snd_hdac_stream_reset+0xe8/0x328 [snd_hda_core] [ 199.495913] snd_hdac_stream_sync+0xe4/0x190 [snd_hda_core] [ 199.501627] azx_pcm_trigger+0x1d8/0x250 [snd_hda_codec] [ 199.507109] snd_pcm_do_stop+0x54/0x70 [ 199.510904] snd_pcm_action_single+0x44/0xa0 [ 199.515215] snd_pcm_drain_done+0x20/0x28 [ 199.519267] snd_pcm_update_state+0x114/0x128 [ 199.523670] snd_pcm_update_hw_ptr0+0x22c/0x3a0 [ 199.528246] snd_pcm_period_elapsed_under_stream_lock+0x44/0x88 [ 199.534216] snd_pcm_period_elapsed+0x24/0x48 [ 199.538617] stream_update+0x3c/0x50 [snd_hda_codec] [ 199.543737] snd_hdac_bus_handle_stream_irq+0xe8/0x150 [snd_hda_core] [ 199.550320] azx_interrupt+0xb4/0x1b0 [snd_hda_codec] [ 199.555524] __handle_irq_event_percpu+0x74/0x140 [ 199.560281] handle_irq_event_percpu+0x14/0x48 [ 199.564772] handle_irq_event+0x44/0x88 [ 199.568653] handle_fasteoi_irq+0xa8/0x130 [ 199.572788] generic_handle_domain_irq+0x28/0x40 [ 199.577452] gic_handle_irq+0x9c/0xb8 [ 199.581168] call_on_irq_stack+0x2c/0x40 [ 199.585129] do_interrupt_handler+0x7c/0x80 [ 199.589355] el1_interrupt+0x34/0x68 [ 199.592969] el1h_64_irq_handler+0x14/0x20 [ 199.597107] el1h_64_irq+0x64/0x68 [ 199.600540] cpuidle_enter_state+0x130/0x2f8 [ 199.604853] cpuidle_enter+0x38/0x50 [ 199.608463] call_cpuidle+0x18/0x38 [ 199.611991] do_idle+0x1f8/0x248 [ 199.615259] cpu_startup_entry+0x20/0x28 [ 199.619224] kernel_init+0x0/0x128 [ 199.622669] arch_post_acpi_subsys_init+0x0/0x8 [ 199.627240] start_kernel+0x630/0x668 [ 199.630933] __primary_switched+0xb4/0xbc
If I change your patch to be read_poll_timeout_atomic, then it works \o/
Can we make that update?
Jon
Yes, it makes sense, as it uses udelay instead of usleep, same as original code.
I've send patch which updates the macros. It passed validation on our side.
Thanks for report!
Amadeusz
Since it was introduced around v2.6.30 it was never used. Also HDA specification does not mention any limitation on number of PIN configurations.
Reviewed-by: Cezary Rojewski cezary.rojewski@intel.com Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/pci/hda/hda_sysfs.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/sound/pci/hda/hda_sysfs.c b/sound/pci/hda/hda_sysfs.c index bf951c10ae61..69ebc37a4d6f 100644 --- a/sound/pci/hda/hda_sysfs.c +++ b/sound/pci/hda/hda_sysfs.c @@ -375,8 +375,6 @@ static ssize_t user_pin_configs_show(struct device *dev, return pin_configs_show(codec, &codec->user_pins, buf); }
-#define MAX_PIN_CONFIGS 32 - static int parse_user_pin_configs(struct hda_codec *codec, const char *buf) { int nid, cfg, err;
There is no need to keep unused defines in file.
Reviewed-by: Cezary Rojewski cezary.rojewski@intel.com Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/pci/hda/hda_intel.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index a77165bd92a9..7720978dc132 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -86,9 +86,6 @@ enum { #define INTEL_SCH_HDA_DEVC 0x78 #define INTEL_SCH_HDA_DEVC_NOSNOOP (0x1<<11)
-/* Define VIA HD Audio Device ID*/ -#define VIA_HDAC_DEVICE_ID 0x3288 - /* max number of SDs */ /* ICH, ATI and VIA have 4 playback and 4 capture */ #define ICH6_NUM_CAPTURE 4 @@ -102,10 +99,6 @@ enum { #define ATIHDMI_NUM_CAPTURE 0 #define ATIHDMI_NUM_PLAYBACK 8
-/* TERA has 4 playback and 3 capture */ -#define TERA_NUM_CAPTURE 3 -#define TERA_NUM_PLAYBACK 4 -
static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX; static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;
On Thu, 18 Aug 2022 16:15:13 +0200, Amadeusz Sławiński wrote:
First two patches are small refactoring of code to use poll macros instead of open coding register checks. Following two patches remove unused defines from code.
v2:
- Fix includes
Amadeusz Sławiński (4): ALSA: hda: Move stream-register polling macros ALSA: hda: Rework snd_hdac_stream_reset() to use macros ALSA: hda: Remove unused MAX_PIN_CONFIGS constant ALSA: hda: Remove unused defines
Thanks, applied now to for-next branch.
Takashi
participants (3)
-
Amadeusz Sławiński -
Jon Hunter -
Takashi Iwai