Skip to content

ASoC: SOF: Intel: only detect codecs when HDA DSP probe #5492

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

Merged
bardliao merged 2 commits into from
Jul 31, 2025
Merged

ASoC: SOF: Intel: only detect codecs when HDA DSP probe #5492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc36524
ASoC: SOF: Intel: split codec mask detect and programming UNSOL
bardliao Jul 29, 2025
a54bc8c
ASoC: SOF: Intel: only detect codecs when HDA DSP probe
bardliao Jul 29, 2025
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
3 changes: 0 additions & 3 deletions sound/soc/sof/intel/hda-codec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,9 +260,6 @@ void hda_codec_detect_mask(struct snd_sof_dev *sdev)
sof_debug_check_flag(SOF_DBG_FORCE_NOCODEC))
return;

/* Accept unsolicited responses */
snd_hdac_chip_updatel(bus, GCTL, AZX_GCTL_UNSOL, AZX_GCTL_UNSOL);

/* detect codecs */
if (!bus->codec_mask) {
bus->codec_mask = snd_hdac_chip_readw(bus, STATESTS);
Expand Down
8 changes: 6 additions & 2 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 @@ -183,7 +183,7 @@ int hda_dsp_ctrl_clock_power_gating(struct snd_sof_dev *sdev, bool enable)
}
Copy link
Collaborator

@kv2019i kv2019i Jul 23, 2025

Choose a reason for hiding this comment

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

typo in commit: "waken event" -> "wake event"

EXPORT_SYMBOL_NS(hda_dsp_ctrl_clock_power_gating, "SND_SOC_SOF_INTEL_HDA_COMMON");

int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev)
int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev, bool detect_codec)
{
struct hdac_bus *bus = sof_to_bus(sdev);
struct hdac_stream *stream;
Expand Down Expand Up @@ -220,7 +220,11 @@ int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev)
}
usleep_range(1000, 1200);

hda_codec_detect_mask(sdev);
/* Accept unsolicited responses */
snd_hdac_chip_updatel(bus, GCTL, AZX_GCTL_UNSOL, AZX_GCTL_UNSOL);

if (detect_codec)
hda_codec_detect_mask(sdev);
Copy link
Collaborator

@kv2019i kv2019i Jul 23, 2025

Choose a reason for hiding this comment

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

Hmm, we had a discussion on the need for this in #5458 . If the display runtime-pm link is removed, the display codec might not be fully up by the time we do resume, but in the end, consensus was that this is harmless, the initial detect_mask at probe should be sufficient.

This is departing from the HDA legacy driver flow though. Aside setting codec_mask, hda_codec_detect_mask() also programs GCTL_UNSOL . With this patch, we drop this programming at resume. Not an issue for HDMI codecs, but for regular HDA jack/speaker codecs this can be a real issue.

Could we instead just look at the HDA bits in hda_codec_detect_mask() and ignore the SDW codec bits?

Copy link
Collaborator Author

@bardliao bardliao Jul 24, 2025

Choose a reason for hiding this comment

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

@kv2019i Yes, initially, my commit is 9a56dbf
But I don't know if (BIT(HDA_EXT_ADDR) | BIT(HDA_IDISP_ADDR)) can represent all HDA codecs. What do you think?

Copy link
Collaborator

@kv2019i kv2019i Jul 24, 2025

Choose a reason for hiding this comment

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

@bardliao Let me think and return to this tomorrow. I think that limitation to SDI0/2 might actually be most practical as SOF doesn't really support anything else (not now and not expected we need this in the future for HDA). But let me still think of other options.

Copy link
Collaborator

@kv2019i kv2019i Jul 25, 2025

Choose a reason for hiding this comment

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

@bardliao I think we need to keep UNSOL programming anyways. The reset value is 0 so it must be programmed on resume when we have normal cases with some HDA codecs.

But yeah, I think I'd be ok with both 1) commit 9a56dbf

And also ok would be 2)
split out UNSOL programming from detect_mask(), call detect mask only from probe and unsol from both probe and from resume. This would not reset the codec_mask upon resume on SDW-only machines (i.e. fix the bug this PR is addressing).

Not sure which (1 or 2) is cleaner...

Copy link
Collaborator Author

@bardliao bardliao Jul 29, 2025

Choose a reason for hiding this comment

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

@kv2019i I chose option 2 :).
Do we need the

        if (IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC_DEBUG_SUPPORT) &&
            sof_debug_check_flag(SOF_DBG_FORCE_NOCODEC))

test before programing UNSOL?


/* clear stream status */
list_for_each_entry(stream, &bus->stream_list, list) {
Expand Down
2 changes: 1 addition & 1 deletion sound/soc/sof/intel/hda-dsp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -881,7 +881,7 @@ static int hda_resume(struct snd_sof_dev *sdev, bool runtime_resume)
snd_sof_pci_update_bits(sdev, PCI_TCSEL, 0x07, 0);

/* reset and start hda controller */
ret = hda_dsp_ctrl_init_chip(sdev);
ret = hda_dsp_ctrl_init_chip(sdev, false);
if (ret < 0) {
dev_err(sdev->dev,
"error: failed to start controller after resume\n");
Expand Down
2 changes: 1 addition & 1 deletion sound/soc/sof/intel/hda.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ static int hda_init_caps(struct snd_sof_dev *sdev)
dev_dbg(sdev->dev, "PP capability, will probe DSP later.\n");

/* Init HDA controller after i915 init */
ret = hda_dsp_ctrl_init_chip(sdev);
ret = hda_dsp_ctrl_init_chip(sdev, true);
if (ret < 0) {
dev_err(bus->dev, "error: init chip failed with ret: %d\n",
ret);
Expand Down
2 changes: 1 addition & 1 deletion sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,7 +757,7 @@ void hda_dsp_ctrl_ppcap_int_enable(struct snd_sof_dev *sdev, bool enable);
int hda_dsp_ctrl_link_reset(struct snd_sof_dev *sdev, bool reset);
void hda_dsp_ctrl_misc_clock_gating(struct snd_sof_dev *sdev, bool enable);
int hda_dsp_ctrl_clock_power_gating(struct snd_sof_dev *sdev, bool enable);
int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev);
int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev, bool detect_codec);
void hda_dsp_ctrl_stop_chip(struct snd_sof_dev *sdev);
/*
* HDA bus operations.
Expand Down
Loading