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
There is no need to keep unused defines in file.
Reviewed-by: Cezary Rojewski <[email protected]>
Signed-off-by: Amadeusz Sławiński <[email protected]>
---
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
We can use existing macros to poll and update register values instead of
open coding the functionality.
Reviewed-by: Cezary Rojewski <[email protected]>
Signed-off-by: Amadeusz Sławiński <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Amadeusz Sławiński <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Amadeusz Sławiński <[email protected]>
---
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
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
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 <[email protected]>
> Signed-off-by: Amadeusz Sławiński <[email protected]>
> ---
> 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
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 <[email protected]>
> > Signed-off-by: Amadeusz S?awi?ski <[email protected]>
> > ---
> > 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)
> #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
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)
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
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
--
nvpublic
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
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