Uh oh!
There was an error while loading. Please reload this page.
Enable WAKEEN feature in sof hda driver#983
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -10,6 +10,7 @@ | ||
| #include <linux/module.h> | ||
| #include <sound/hdaudio_ext.h> | ||
| #include <sound/hda_register.h> | ||
| #include <sound/hda_codec.h> | ||
| #include <sound/hda_i915.h> | ||
| #include <sound/sof.h> | ||
| @@ -37,16 +38,55 @@ static void hda_codec_load_module(struct hda_codec *codec) | ||
| static void hda_codec_load_module(struct hda_codec *codec) {} | ||
| #endif | ||
| /* check jack status after resuming from suspend mode */ | ||
| void hda_codec_jack_check(struct snd_sof_dev *sdev, int status) | ||
| { | ||
| struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata; | ||
| struct hda_bus *hbus = sof_to_hbus(sdev); | ||
| struct hdac_bus *bus = sof_to_bus(sdev); | ||
| struct hda_codec *codec; | ||
| int mask; | ||
| /* | ||
| * there are two reasons for runtime resume | ||
| * (1) waken up by interrupt triggered by WAKEEN feature | ||
| * (2) waken up by pm get functions for some audio operations | ||
| * For case (1), the bits in status mean which codec triggers | ||
| * the interrupt and jacks will be checked on these codecs. | ||
| * For case (2), we need to check all the non-hdmi codecs for some | ||
| * cases like playback with HDMI or capture with DMIC. In these | ||
| * cases, only controller is active and codecs are suspended, so | ||
| * codecs can't send unsolicited event to controller. The jack polling | ||
| * operation will activate codecs and unsolicited event can work | ||
| * even codecs become suspended later. | ||
| */ | ||
| mask = status ? status : hda->hda_codec_mask; | ||
| list_for_each_codec(codec, hbus) | ||
| if (mask & BIT(codec->core.addr)) | ||
| schedule_delayed_work(&codec->jackpoll_work, | ||
| codec->jackpoll_interval); | ||
Collaborator There was a problem hiding this comment. This doesn't work, it only produces kernel warnings, since the jackpool_work isn't initialised in SOF. Further, even though with some tricking that work can be abused for our purpose, its initial purpose is codec polling when the codec cannot send unsolicited events. Author There was a problem hiding this comment. I didn't make some tricking thing and both I and QA tested on a few devices. Author There was a problem hiding this comment.
@lyakh Can you share me your test device info so that I can debug on it. Thanks! Author There was a problem hiding this comment. @lyakh jackpoo_work is initialized by hdac_hda codec. I checked kernel document and didn't find any limitation about this case. Could do share with us more information ? thanks! Collaborator There was a problem hiding this comment. @RanderWang I was getting these warnings, here from a Wasp log: I'm trying to recover the state, when I was getting those warnings to study them in a bit more detail. Collaborator There was a problem hiding this comment. It cost me about half a day, but now I have it. Basically you need something like: Collaborator There was a problem hiding this comment. @RanderWang I actually think it would be better to avoid setting WAKEEN bits for codecs like HDMI. Feel free to use this diff instead of the previous one: Author There was a problem hiding this comment. Thanks! These warning message is caused by interrupt issue and I will validate it again on WASP with patch for interrupt. Thanks for your reference! | ||
| /* disable controller Wake Up event*/ | ||
| snd_hdac_chip_writew(bus, WAKEEN, | ||
| snd_hdac_chip_readw(bus, WAKEEN) & | ||
| ~hda->hda_codec_mask); | ||
| } | ||
| #else | ||
| void hda_codec_jack_check(struct snd_sof_dev *sdev, int status) {} | ||
| #endif /* CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC */ | ||
| EXPORT_SYMBOL(hda_codec_jack_check); | ||
| /* probe individual codec */ | ||
| static int hda_codec_probe(struct snd_sof_dev *sdev, int address) | ||
| { | ||
| #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC) | ||
| struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata; | ||
| struct hdac_hda_priv *hda_priv; | ||
| #endif | ||
| struct hda_bus *hbus = sof_to_hbus(sdev); | ||
| struct hdac_device *hdev; | ||
| u32 hda_cmd = (address << 28) | (AC_NODE_ROOT << 20) | | ||
| (AC_VERB_PARAMETERS << 8) | AC_PAR_VENDOR_ID; | ||
| u32 resp = -1; | ||
| @@ -76,6 +116,7 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address) | ||
| /* use legacy bus only for HDA codecs, idisp uses ext bus */ | ||
| if ((resp & 0xFFFF0000) != IDISP_VID_INTEL) { | ||
| hdev->type = HDA_DEV_LEGACY; | ||
| hda->hda_codec_mask |= BIT(address); | ||
| hda_codec_load_module(&hda_priv->codec); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -282,7 +282,8 @@ void hda_dsp_ipc_int_disable(struct snd_sof_dev *sdev) | ||
| HDA_DSP_REG_HIPCCTL_BUSY | HDA_DSP_REG_HIPCCTL_DONE, 0); | ||
| } | ||
| static int hda_suspend(struct snd_sof_dev *sdev, int state, | ||
| bool runtime_suspend) | ||
| { | ||
| struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata; | ||
| const struct sof_intel_dsp_desc *chip = hda->desc; | ||
| @@ -295,6 +296,12 @@ static int hda_suspend(struct snd_sof_dev *sdev, int state) | ||
| hda_dsp_ipc_int_disable(sdev); | ||
| #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) | ||
| if (IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC) && runtime_suspend) | ||
| /* enable controller wake up event */ | ||
| snd_hdac_chip_writew(bus, WAKEEN, | ||
| snd_hdac_chip_readw(bus, WAKEEN) | | ||
| hda->hda_codec_mask); | ||
| /* power down all hda link */ | ||
| snd_hdac_ext_bus_link_power_down_all(bus); | ||
| #endif | ||
| @@ -329,11 +336,12 @@ static int hda_suspend(struct snd_sof_dev *sdev, int state) | ||
| return 0; | ||
| } | ||
| static int hda_resume(struct snd_sof_dev *sdev, bool runtime_resume) | ||
| { | ||
| #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) | ||
| struct hdac_bus *bus = sof_to_bus(sdev); | ||
| struct hdac_ext_link *hlink = NULL; | ||
| int status; | ||
| #endif | ||
| int ret; | ||
| @@ -344,6 +352,11 @@ static int hda_resume(struct snd_sof_dev *sdev) | ||
| snd_sof_pci_update_bits(sdev, PCI_TCSEL, 0x07, 0); | ||
| #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) | ||
| if (runtime_resume) { | ||
| /* read STATESTS before controller reset */ | ||
| status = snd_hdac_chip_readw(bus, STATESTS); | ||
plbossart marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. plbossart marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| /* reset and start hda controller */ | ||
| ret = hda_dsp_ctrl_init_chip(sdev, true); | ||
| if (ret < 0) { | ||
| @@ -359,6 +372,20 @@ static int hda_resume(struct snd_sof_dev *sdev) | ||
| bus->io_ops->reg_writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV); | ||
| hda_dsp_ctrl_misc_clock_gating(sdev, true); | ||
| /* check jack status based on controller status */ | ||
| if (runtime_resume) | ||
| hda_codec_jack_check(sdev, status); | ||
| /* turn off the links that were off before suspend */ | ||
| list_for_each_entry(hlink, &bus->hlink_list, list) { | ||
| if (!hlink->ref_count) | ||
| snd_hdac_ext_bus_link_power_down(hlink); | ||
| } | ||
| /* check dma status and clean up CORB/RIRB buffers */ | ||
| if (!bus->cmd_dma_state) | ||
| snd_hdac_bus_stop_cmd_io(bus); | ||
| #else | ||
| hda_dsp_ctrl_misc_clock_gating(sdev, false); | ||
| @@ -391,37 +418,25 @@ static int hda_resume(struct snd_sof_dev *sdev) | ||
| hda_dsp_ctrl_ppcap_enable(sdev, true); | ||
| hda_dsp_ctrl_ppcap_int_enable(sdev, true); | ||
| return 0; | ||
| } | ||
| int hda_dsp_resume(struct snd_sof_dev *sdev) | ||
| { | ||
| /* init hda controller. DSP cores will be powered up during fw boot */ | ||
| return hda_resume(sdev, false); | ||
| } | ||
| int hda_dsp_runtime_resume(struct snd_sof_dev *sdev) | ||
| { | ||
| /* init hda controller. DSP cores will be powered up during fw boot */ | ||
| return hda_resume(sdev, true); | ||
| } | ||
| int hda_dsp_runtime_suspend(struct snd_sof_dev *sdev, int state) | ||
| { | ||
| /* stop hda controller and power dsp off */ | ||
| return hda_suspend(sdev, state, true); | ||
| } | ||
| int hda_dsp_suspend(struct snd_sof_dev *sdev, int state) | ||
| @@ -430,7 +445,7 @@ int hda_dsp_suspend(struct snd_sof_dev *sdev, int state) | ||
| int ret; | ||
| /* stop hda controller and power dsp off */ | ||
| ret = hda_suspend(sdev, state, false); | ||
| if (ret < 0) { | ||
| dev_err(bus->dev, "error: suspending dsp\n"); | ||
| return ret; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -392,6 +392,9 @@ struct sof_intel_hda_dev { | ||
| /* DMIC device */ | ||
| struct platform_device *dmic_dev; | ||
| /* hda codec mask excluding hdmi */ | ||
| u32 hda_codec_mask; | ||
Collaborator There was a problem hiding this comment. @RanderWang@plbossart@ranj063 Is this really needed? Wouldn't it be enough to check codec->jacktbl.used? Author There was a problem hiding this comment. hda_codec_mask is used by enable & disable WAKEEN, please check other change in the PR. codec->jacktbl.used is not available. | ||
| }; | ||
| static inline struct hdac_bus *sof_to_bus(struct snd_sof_dev *s) | ||
| @@ -550,6 +553,7 @@ void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev, | ||
| * HDA Codec operations. | ||
| */ | ||
| int hda_codec_probe_bus(struct snd_sof_dev *sdev); | ||
| void hda_codec_jack_check(struct snd_sof_dev *sdev, int status); | ||
| #endif /* CONFIG_SND_SOC_SOF_HDA */ | ||
Collaborator There was a problem hiding this comment. Please leave the empty line between prototype and #endif | ||
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RanderWang@plbossart Sorry, I don't understand this. Now you check jack-status of every codec on every runtime-resume?.. This seems wrong to me. Jack status change should be detected when the change happens, not when the next rtpm-resume is happening. This really doesn't look right to me, please, explain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check comments in this function