Skip to content

ASoC: SOF: Intel: hda-ctrl: add missing WAKE_STS clear#4896

Merged
plbossart merged 1 commit intothesofproject:topic/sof-devfrom
plbossart:fix/lnl-reset
Apr 4, 2024
Merged

ASoC: SOF: Intel: hda-ctrl: add missing WAKE_STS clear#4896
plbossart merged 1 commit intothesofproject:topic/sof-devfrom
plbossart:fix/lnl-reset

Conversation

@plbossart
Copy link
Member

For some reason, the programming sequences in the SOF driver do not include a clear of the WAKE_STS bits before resetting the controller.

This clear is not formally required by the HDaudio specification, but was added to harden the snd-hda-reset back in 2007. Adding this sequence back avoids an issue reported by the Intel CI.

Closes: #4889

For some reason, the programming sequences in the SOF driver do not
include a clear of the WAKE_STS bits before resetting the controller.

This clear is not formally required by the HDaudio specification, but
was added to harden the snd-hda-reset back in 2007. Adding this
sequence back avoids an issue reported by the Intel CI.

Closes: thesofproject#4889
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@marc-hb
Copy link
Collaborator

marc-hb commented Apr 2, 2024

In a comment in #4894 you posted more information and SHA1s which really seem to belong to this commit message:
#4894 (comment)

FWIW the 3rd commit partially reverts 6ec3295 ("ASoC: SOF: Intel: hda: remove duplicated clear WAKESTS")

I have no idea why this was removed.

It could also be that there's an interference from ae5ff22 ("ASoC: SOF: Intel: hda-ctrl: only clear WAKESTS for HDaudio codecs"), where in the end we didn't clear all the WAKE_STS fields.

Wow, my brain is officially fried haha.

OK: maybe the last line about your brain is not necessary.

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 2, 2024

I have no idea why this was removed.

This was discussed in

@plbossart
Copy link
Member Author

I don't want to add more details on this patch.
We don't really know what the correlation/causation is on LNL, adding more details would likely mislead reviewers.

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 2, 2024

I wonder why even the purely factual, past SHA1s could confuse reviewers but I imagine they can be found with some well placed git blame commands so I hope omitting them only adds a bit overhead.


/* 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to test gctl? Can we clear WAKE_STS unconditionally?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry @bardliao I don't understand the questions. This is similar to the code in b37a151

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @plbossart , I got the reason why if (gctl & SOF_HDA_GCTL_RESET) is needed after reading b37a151.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LNL/SDW AOIC: major regression with PIO commands

4 participants

Comments