alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] hda: Minor cleanups
@ 2022-08-18 14:15 Amadeusz Sławiński
  2022-08-18 14:15 ` [PATCH v2 1/4] ALSA: hda: Move stream-register polling macros Amadeusz Sławiński
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Amadeusz Sławiński @ 2022-08-18 14:15 UTC (permalink / raw)
  To: Takashi Iwai, alsa-devel
  Cc: Cezary Rojewski, linux-kernel, Amadeusz Sławiński,
	Pierre-Louis Bossart

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/4] ALSA: hda: Move stream-register polling macros
  2022-08-18 14:15 [PATCH v2 0/4] hda: Minor cleanups Amadeusz Sławiński
@ 2022-08-18 14:15 ` Amadeusz Sławiński
  2022-08-18 14:15 ` [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros Amadeusz Sławiński
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Amadeusz Sławiński @ 2022-08-18 14:15 UTC (permalink / raw)
  To: Takashi Iwai, alsa-devel
  Cc: Cezary Rojewski, linux-kernel, Amadeusz Sławiński,
	Pierre-Louis Bossart

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros
  2022-08-18 14:15 [PATCH v2 0/4] hda: Minor cleanups Amadeusz Sławiński
  2022-08-18 14:15 ` [PATCH v2 1/4] ALSA: hda: Move stream-register polling macros Amadeusz Sławiński
@ 2022-08-18 14:15 ` Amadeusz Sławiński
  2022-10-05 12:10   ` Jon Hunter
  2022-08-18 14:15 ` [PATCH v2 3/4] ALSA: hda: Remove unused MAX_PIN_CONFIGS constant Amadeusz Sławiński
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Amadeusz Sławiński @ 2022-08-18 14:15 UTC (permalink / raw)
  To: Takashi Iwai, alsa-devel
  Cc: Cezary Rojewski, linux-kernel, Amadeusz Sławiński,
	Pierre-Louis Bossart

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 3/4] ALSA: hda: Remove unused MAX_PIN_CONFIGS constant
  2022-08-18 14:15 [PATCH v2 0/4] hda: Minor cleanups Amadeusz Sławiński
  2022-08-18 14:15 ` [PATCH v2 1/4] ALSA: hda: Move stream-register polling macros Amadeusz Sławiński
  2022-08-18 14:15 ` [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros Amadeusz Sławiński
@ 2022-08-18 14:15 ` Amadeusz Sławiński
  2022-08-18 14:15 ` [PATCH v2 4/4] ALSA: hda: Remove unused defines Amadeusz Sławiński
  2022-08-19 12:05 ` [PATCH v2 0/4] hda: Minor cleanups Takashi Iwai
  4 siblings, 0 replies; 15+ messages in thread
From: Amadeusz Sławiński @ 2022-08-18 14:15 UTC (permalink / raw)
  To: Takashi Iwai, alsa-devel
  Cc: Cezary Rojewski, linux-kernel, Amadeusz Sławiński,
	Pierre-Louis Bossart

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 4/4] ALSA: hda: Remove unused defines
  2022-08-18 14:15 [PATCH v2 0/4] hda: Minor cleanups Amadeusz Sławiński
                   ` (2 preceding siblings ...)
  2022-08-18 14:15 ` [PATCH v2 3/4] ALSA: hda: Remove unused MAX_PIN_CONFIGS constant Amadeusz Sławiński
@ 2022-08-18 14:15 ` Amadeusz Sławiński
  2022-08-19 12:05 ` [PATCH v2 0/4] hda: Minor cleanups Takashi Iwai
  4 siblings, 0 replies; 15+ messages in thread
From: Amadeusz Sławiński @ 2022-08-18 14:15 UTC (permalink / raw)
  To: Takashi Iwai, alsa-devel
  Cc: Cezary Rojewski, linux-kernel, Amadeusz Sławiński,
	Pierre-Louis Bossart

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/4] hda: Minor cleanups
  2022-08-18 14:15 [PATCH v2 0/4] hda: Minor cleanups Amadeusz Sławiński
                   ` (3 preceding siblings ...)
  2022-08-18 14:15 ` [PATCH v2 4/4] ALSA: hda: Remove unused defines Amadeusz Sławiński
@ 2022-08-19 12:05 ` Takashi Iwai
  4 siblings, 0 replies; 15+ messages in thread
From: Takashi Iwai @ 2022-08-19 12:05 UTC (permalink / raw)
  To: Amadeusz Sławiński
  Cc: Pierre-Louis Bossart, Cezary Rojewski, linux-kernel,
	Takashi Iwai, alsa-devel

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros
  2022-08-18 14:15 ` [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros Amadeusz Sławiński
@ 2022-10-05 12:10   ` Jon Hunter
  2022-10-05 12:29     ` Takashi Iwai
  0 siblings, 1 reply; 15+ messages in thread
From: Jon Hunter @ 2022-10-05 12:10 UTC (permalink / raw)
  To: Amadeusz Sławiński, Takashi Iwai, alsa-devel
  Cc: Pierre-Louis Bossart, Cezary Rojewski, linux-kernel, linux-tegra,
	Mohan Kumar D


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros
  2022-10-05 12:10   ` Jon Hunter
@ 2022-10-05 12:29     ` Takashi Iwai
  2022-10-05 13:52       ` Jon Hunter
  2022-10-06  8:45       ` Jon Hunter
  0 siblings, 2 replies; 15+ messages in thread
From: Takashi Iwai @ 2022-10-05 12:29 UTC (permalink / raw)
  To: Jon Hunter
  Cc: alsa-devel, Cezary Rojewski, Takashi Iwai, linux-kernel,
	Pierre-Louis Bossart, Amadeusz Sławiński, linux-tegra,
	Mohan Kumar D

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)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros
  2022-10-05 12:29     ` Takashi Iwai
@ 2022-10-05 13:52       ` Jon Hunter
  2022-10-05 14:07         ` Takashi Iwai
  2022-10-06  8:45       ` Jon Hunter
  1 sibling, 1 reply; 15+ messages in thread
From: Jon Hunter @ 2022-10-05 13:52 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: alsa-devel, Cezary Rojewski, Takashi Iwai, linux-kernel,
	Pierre-Louis Bossart, Amadeusz Sławiński, linux-tegra,
	Mohan Kumar D


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros
  2022-10-05 13:52       ` Jon Hunter
@ 2022-10-05 14:07         ` Takashi Iwai
  2022-10-05 14:26           ` Jon Hunter
  0 siblings, 1 reply; 15+ messages in thread
From: Takashi Iwai @ 2022-10-05 14:07 UTC (permalink / raw)
  To: Jon Hunter
  Cc: alsa-devel, Cezary Rojewski, Takashi Iwai, linux-kernel,
	Pierre-Louis Bossart, Amadeusz Sławiński, linux-tegra,
	Mohan Kumar D

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)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros
  2022-10-05 14:07         ` Takashi Iwai
@ 2022-10-05 14:26           ` Jon Hunter
  2022-10-05 14:47             ` Amadeusz Sławiński
  0 siblings, 1 reply; 15+ messages in thread
From: Jon Hunter @ 2022-10-05 14:26 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: alsa-devel, Cezary Rojewski, Takashi Iwai, linux-kernel,
	Pierre-Louis Bossart, Amadeusz Sławiński, linux-tegra,
	Mohan Kumar D



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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros
  2022-10-05 14:26           ` Jon Hunter
@ 2022-10-05 14:47             ` Amadeusz Sławiński
  2022-10-05 20:16               ` Jon Hunter
  0 siblings, 1 reply; 15+ messages in thread
From: Amadeusz Sławiński @ 2022-10-05 14:47 UTC (permalink / raw)
  To: Jon Hunter, Takashi Iwai
  Cc: alsa-devel, Cezary Rojewski, Takashi Iwai, linux-kernel,
	Pierre-Louis Bossart, linux-tegra, Mohan Kumar D

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?


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros
  2022-10-05 14:47             ` Amadeusz Sławiński
@ 2022-10-05 20:16               ` Jon Hunter
  0 siblings, 0 replies; 15+ messages in thread
From: Jon Hunter @ 2022-10-05 20:16 UTC (permalink / raw)
  To: Amadeusz Sławiński, Takashi Iwai
  Cc: alsa-devel, Cezary Rojewski, Takashi Iwai, linux-kernel,
	Pierre-Louis Bossart, linux-tegra, Mohan Kumar D



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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros
  2022-10-05 12:29     ` Takashi Iwai
  2022-10-05 13:52       ` Jon Hunter
@ 2022-10-06  8:45       ` Jon Hunter
  2022-10-07  8:49         ` Amadeusz Sławiński
  1 sibling, 1 reply; 15+ messages in thread
From: Jon Hunter @ 2022-10-06  8:45 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: alsa-devel, Cezary Rojewski, Takashi Iwai, linux-kernel,
	Pierre-Louis Bossart, Amadeusz Sławiński, linux-tegra,
	Mohan Kumar D


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros
  2022-10-06  8:45       ` Jon Hunter
@ 2022-10-07  8:49         ` Amadeusz Sławiński
  0 siblings, 0 replies; 15+ messages in thread
From: Amadeusz Sławiński @ 2022-10-07  8:49 UTC (permalink / raw)
  To: Jon Hunter, Takashi Iwai
  Cc: alsa-devel, Cezary Rojewski, Takashi Iwai, linux-kernel,
	Pierre-Louis Bossart, linux-tegra, Mohan Kumar D

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-10-07  8:51 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-18 14:15 [PATCH v2 0/4] hda: Minor cleanups Amadeusz Sławiński
2022-08-18 14:15 ` [PATCH v2 1/4] ALSA: hda: Move stream-register polling macros Amadeusz Sławiński
2022-08-18 14:15 ` [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros Amadeusz Sławiński
2022-10-05 12:10   ` Jon Hunter
2022-10-05 12:29     ` Takashi Iwai
2022-10-05 13:52       ` Jon Hunter
2022-10-05 14:07         ` Takashi Iwai
2022-10-05 14:26           ` Jon Hunter
2022-10-05 14:47             ` Amadeusz Sławiński
2022-10-05 20:16               ` Jon Hunter
2022-10-06  8:45       ` Jon Hunter
2022-10-07  8:49         ` Amadeusz Sławiński
2022-08-18 14:15 ` [PATCH v2 3/4] ALSA: hda: Remove unused MAX_PIN_CONFIGS constant Amadeusz Sławiński
2022-08-18 14:15 ` [PATCH v2 4/4] ALSA: hda: Remove unused defines Amadeusz Sławiński
2022-08-19 12:05 ` [PATCH v2 0/4] hda: Minor cleanups Takashi Iwai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).