Skip to content

[DNM] Random attempts to fix controller reset issue #4894

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
plbossart wants to merge 3 commits into from
Closed

[DNM] Random attempts to fix controller reset issue #4894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e83f0b3
fixup! ALSA: hda: hdac_controller: Implement support for use_pio_for_…
plbossart Apr 1, 2024
e53d312
[HACK] ASoC: SOF: Intel: disable interrupts last
plbossart Apr 1, 2024
fb332ed
ASoC: SOF: Intel: hda-ctrl: add missing WAKE_STS clear
plbossart Apr 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 24 additions & 19 deletions sound/hda/hdac_controller.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,25 +45,26 @@ void snd_hdac_bus_init_cmd_io(struct hdac_bus *bus)
WARN_ON_ONCE(!bus->rb.area);

spin_lock_irq(&bus->reg_lock);
/* CORB set up */
bus->corb.addr = bus->rb.addr;
bus->corb.buf = (__le32 *)bus->rb.area;
snd_hdac_chip_writel(bus, CORBLBASE, (u32)bus->corb.addr);
snd_hdac_chip_writel(bus, CORBUBASE, upper_32_bits(bus->corb.addr));

/* set the corb size to 256 entries (ULI requires explicitly) */
snd_hdac_chip_writeb(bus, CORBSIZE, 0x02);
/* set the corb write pointer to 0 */
snd_hdac_chip_writew(bus, CORBWP, 0);

/* reset the corb hw read pointer */
snd_hdac_chip_writew(bus, CORBRP, AZX_CORBRP_RST);
if (!bus->corbrp_self_clear)
azx_clear_corbrp(bus);

/* enable corb dma */
if (!bus->use_pio_for_commands)
if (!bus->use_pio_for_commands) {
/* CORB set up */
bus->corb.addr = bus->rb.addr;
bus->corb.buf = (__le32 *)bus->rb.area;
snd_hdac_chip_writel(bus, CORBLBASE, (u32)bus->corb.addr);
snd_hdac_chip_writel(bus, CORBUBASE, upper_32_bits(bus->corb.addr));

/* set the corb size to 256 entries (ULI requires explicitly) */
snd_hdac_chip_writeb(bus, CORBSIZE, 0x02);
/* set the corb write pointer to 0 */
snd_hdac_chip_writew(bus, CORBWP, 0);

/* reset the corb hw read pointer */
snd_hdac_chip_writew(bus, CORBRP, AZX_CORBRP_RST);
if (!bus->corbrp_self_clear)
azx_clear_corbrp(bus);

/* enable corb dma */
snd_hdac_chip_writeb(bus, CORBCTL, AZX_CORBCTL_RUN);
}

/* RIRB set up */
bus->rirb.addr = bus->rb.addr + 2048;
Expand Down Expand Up @@ -100,6 +101,9 @@ static void hdac_wait_for_cmd_dmas(struct hdac_bus *bus)
&& time_before(jiffies, timeout))
udelay(10);

if (bus->use_pio_for_commands)
return;

timeout = jiffies + msecs_to_jiffies(100);
while ((snd_hdac_chip_readb(bus, CORBCTL) & AZX_CORBCTL_RUN)
&& time_before(jiffies, timeout))
Expand All @@ -115,7 +119,8 @@ void snd_hdac_bus_stop_cmd_io(struct hdac_bus *bus)
spin_lock_irq(&bus->reg_lock);
/* disable ringbuffer DMAs */
snd_hdac_chip_writeb(bus, RIRBCTL, 0);
snd_hdac_chip_writeb(bus, CORBCTL, 0);
if (!bus->use_pio_for_commands)
snd_hdac_chip_writeb(bus, CORBCTL, 0);
spin_unlock_irq(&bus->reg_lock);

hdac_wait_for_cmd_dmas(bus);
Expand Down
16 changes: 12 additions & 4 deletions sound/soc/sof/intel/hda-ctrl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev)
struct hdac_bus *bus = sof_to_bus(sdev);
struct hdac_stream *stream;
int sd_offset, ret = 0;
u32 gctl;

if (bus->chip_init)
return 0;
Expand All @@ -196,6 +197,12 @@ int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev)

hda_dsp_ctrl_misc_clock_gating(sdev, false);

/* clear WAKE_STS if not in reset */
gctl = snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR, SOF_HDA_GCTL);
if (gctl & SOF_HDA_GCTL_RESET)
snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR,
SOF_HDA_WAKESTS, SOF_HDA_WAKESTS_INT_MASK);
Copy link
Collaborator

@kv2019i kv2019i Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart @ujfalusi This is really curious. HDA-spec wise this clearing is not needed, the state will be reset with controller reset. But indeed in snd-hda-intel, this has been historically done to avoid bugs, but until now, we've not needed this in SOF.

Copy link
Collaborator

@bardliao bardliao Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, based on my test, the commit does help on updating GCTL value in hda_dsp_ctrl_link_reset(). gctl will be always 1 without this commit. But I don't get the reason why clear WAKE_STS helps on this. Looks like something is blocked?


/* reset HDA controller */
ret = hda_dsp_ctrl_link_reset(sdev, true);
if (ret < 0) {
Expand Down Expand Up @@ -284,10 +291,6 @@ void hda_dsp_ctrl_stop_chip(struct snd_sof_dev *sdev)
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTCTL,
SOF_HDA_INT_ALL_STREAM, 0);

/* disable controller CIE and GIE */
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTCTL,
SOF_HDA_INT_CTRL_EN | SOF_HDA_INT_GLOBAL_EN,
0);
Copy link
Collaborator

@bardliao bardliao Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question is does snd_sof_dsp_write need the interrupts?

Copy link
Member Author

@plbossart plbossart Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is useful, what I was thinking is that disabling the interrupts and then stopping the commands makes no sense in general. If there was an in-flight command it would be blocked, be it with the CORB/RIRB or PIO mode.

For now this doesn't have any effect so we can drop it. Still it'd be good to double-check this.

Copy link
Collaborator

@ujfalusi ujfalusi Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PIO mode is polling on the IC registers, it is not using interrupts, CORB/RIRB uses interrupts.


/* clear stream status */
list_for_each_entry(stream, &bus->stream_list, list) {
Expand Down Expand Up @@ -317,6 +320,11 @@ void hda_dsp_ctrl_stop_chip(struct snd_sof_dev *sdev)
SOF_HDA_ADSP_DPUBASE, 0);
}

/* disable controller CIE and GIE */
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTCTL,
SOF_HDA_INT_CTRL_EN | SOF_HDA_INT_GLOBAL_EN,
0);

bus->chip_init = false;
}

Expand Down