ASoC: SOF: Intel: only detect codecs when HDA DSP probe#5492
ASoC: SOF: Intel: only detect codecs when HDA DSP probe#5492bardliao merged 2 commits intothesofproject:topic/sof-devfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the HDA codec detection logic to filter the codec mask to only include HDA and IDISP codecs, excluding SDW (SoundWire) codecs. This change prevents interference with SDW codec wake handling since LNL (Lunar Lake) platforms where SDW codecs use the global HDaudio WAKEEN/STS mechanism but have their wake events handled separately by the SDW driver.
- Filters
bus->codec_maskto only include HDA and IDISP codecs using a bitmask - Adds explanatory comment about SDW codec wake handling separation
- Prevents clearing wake status before SDW driver can handle it
bardliao
changed the title
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Does this patch need to be a fix for back porting ?
|
@bardliao Thank you for your full support! I have verified it in non-HDMI case. |
@lgirdwood Not sure if it is needed. No one reported the issue and I think all Intel SDW devices have HDMI supported. And I don't know which commit should be the fix tag target to. https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=ab9182441ee5a94dd6f47743ed1b7b6b07b63cb2 could be a good candidate. However, the patch can't apply to the commit. But there is no issue if I cherry pick the patch on top of the "ASoC: SOF: Intel: lnl: add helper to detect SoundWire wakes" commit. Can we add a fix tag with this commit in this case? |
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Some comments inline, please check.
| @@ -183,7 +183,7 @@ int hda_dsp_ctrl_clock_power_gating(struct snd_sof_dev *sdev, bool enable) | |||
| } | |||
There was a problem hiding this comment.
typo in commit: "waken event" -> "wake event"
|
|
||
| hda_codec_detect_mask(sdev); | ||
| if (detect_codec) | ||
| hda_codec_detect_mask(sdev); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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...
There was a problem hiding this comment.
@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?
We only need to detect codec mask in probe, but need to program UNSOL in probe and resume. We will detect codec mask and program UNSOL separately in the follow up commit. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
SDW codecs use the global HDaudio WAKEEN/STS to detect wakes since LNL. But the wake event is handled in the SDW driver. We should only clear WAKESTS for the HDA and IDISP codecs that was detected when HDA DSP probe. The SoundWire codec will be included in the codec_mask if we read the STATESTS register when a SoundWire codec wake event happens. The commit avoid updating bus->codec_mask in resume to not clear WAKESTS of SoundWire codecs. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
SDW codecs use the global HDaudio WAKEEN/STS to detect wakes since LNL. But the wakeen is handled in the SDW driver. We should filter the bus->codec_mask to only include HDA and IDISP codecs to avoid clearing the wakeen status before it is handled.