[PATCH v2 0/4] hda: Minor cleanups

[PATCH v2 0/4] hda: Minor cleanups

[Amadeusz Sławiński]

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(-)

-- 
2.25.1

[PATCH v2 1/4] ALSA: hda: Move stream-register polling macros

[Amadeusz Sławiński]

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;
 
-- 
2.25.1

[PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros

[Amadeusz Sławiński]

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)
-- 
2.25.1

Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros

[Takashi Iwai]

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)

Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros

[Jon Hunter]

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?

Jon

-- 
nvpublic

Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros

[Takashi Iwai]

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.


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)

Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros

[Amadeusz Sławiński]

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?

Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros

[Amadeusz Sławiński]

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

[PATCH v2 3/4] ALSA: hda: Remove unused MAX_PIN_CONFIGS constant

[Amadeusz Sławiński]

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;
-- 
2.25.1

[PATCH v2 4/4] ALSA: hda: Remove unused defines

[Amadeusz Sławiński]

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;
-- 
2.25.1

Re: [PATCH v2 0/4] hda: Minor cleanups

[Takashi Iwai]

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