ASoC: SOF: Intel: hda: refine suspend with stop chip by zhuyingjiang · Pull Request #975 · thesofproject/linux

zhuyingjiang · 2019-05-23T07:30:27Z

Define a SOF hda_dsp_ctrl_stop_chip and use this in hda_suspend, so the disable and clear HDA irq and stream irqs steps all called during the suspend. Codec and HDA link related registers are write if CONFIG_SND_SOC_SOF_HDA is true.
Use the SOF defined ppcap functions in hda_suspend/hda_resume.

plbossart

comments below

plbossart · 2019-05-23T13:37:46Z

sound/soc/sof/intel/hda-ctrl.c

+	snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTCTL,
+				SOF_HDA_INT_ALL_STREAM,	0);
+
+	/* disable hda bus irq */


disable controller CIE and GIE

OK, thanks!

no, not disable CIE and GIE, please check this commit torvalds@7d4f606.
Codec can send out INT or wake up controller depending on whether CIE or GIE enabled.(Figure 4, Interupt structure).

@RanderWang Please check
azx_stop_chip
=>snd_hdac_bus_stop_chip
==>azx_int_disable

yes, I tried this PR with my 983 PR for headphone detection on whiskylake. Headphone detection worked with it.

plbossart · 2019-05-23T13:41:25Z

sound/soc/sof/intel/hda-ctrl.c

+	/* disable CORB/RIRB */
+	snd_hdac_bus_stop_cmd_io(bus);
+
+	/* disable position buffer */


is this really dependent on SOF_HDA? This is the position buffer that we can use even for NOCODEC for DPIB.

@plbossart OK, thanks for confirm, I will still follow a PR with change the same in hda_dsp_ctrl_init_chip.

It was a question, have you confirmed if yes/no this code is needed for NOCODEC?
And if yes, then the fix is incorrect since you are using snd_hdac_chip_writel() so introduced a dependency on hdac.

@plbossart Yes, I am confirm that this is needed for NOCODEC. for position buffer is according to HDA stream. What ever codec is using, the position buffer need to be set at beginning and reset at end. It is only that NOCODEC will not cause a issue if not set.
I will update the snd_hdac_chip_writel with SOF functions, thanks!

zhuyingjiang · 2019-05-24T07:54:02Z

@plbossart updated

zhuyingjiang · 2019-05-29T08:58:08Z

@plbossart I think this can be merged?

RanderWang

LGTM

plbossart · 2019-05-29T15:23:15Z

this is going to conflict with @RanderWang changes, and I'd like his PR to go in first.

kv2019i

The first commit message doesn't explain why the change is needed.

The description of third commit (for example "If CONFIG_SND_SOC_SOF_HDA is
defined, also disable the CORB/RIRB, and stop i/o.") should go to the second commit as that's where the new stop i/o is added.

In general this looks good and helps with readability.

plbossart

same comment as @kv2019i the code looks well structured but the commit messages need more work for upstream. Thanks!

zhuyingjiang · 2019-06-04T02:34:39Z

@plbossart @kv2019i Updated, thanks for your suggestions!

kv2019i

Thanks, the commits are now better, but e.g. the last commit still needs work. How about

First:
Unify ppcap function setup by using SOF common functions for both
HDA and non-HDA cases.

Second:
Add common hda_dsp_ctrl_stop_chip() function to stop controller with
the same function handling both HDA and non-HDA cases. This function
disables IRQs and clears status masks. When CONFIG_SND_SOC_SOF_HDA
is defined, also disables the CORB/RIRB, and stops i/o.

Third:
Unify suspend code by using SOF common function
hda_dsp_ctrl_stop_chip() which can handle both HDA and non-HDA cases.

Unify ppcap function setup by using SOF common functions for both HDA and non-HDA cases. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

Add common hda_dsp_ctrl_stop_chip() function to stop controller with the same function handling both HDA and non-HDA cases. This function disables IRQs and clears status masks. When CONFIG_SND_SOC_SOF_HDA is defined, also disables the CORB/RIRB, and stops i/o. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

Unify suspend code by using SOF common function hda_dsp_ctrl_stop_chip() which can handle both HDA and non-HDA cases. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

zhuyingjiang · 2019-06-05T02:10:24Z

@kv2019i Updated, thanks for your suggestion!

kv2019i

Thank you, looks good now!

plbossart

Thanks @zhuyingjiang

zhuyingjiang requested review from lgirdwood and plbossart as code owners May 23, 2019 07:30

zhuyingjiang mentioned this pull request May 23, 2019

[RFC] ASoC: SOF: Intel: hda: refine init/stop chip in hda_suspend/hda_resume #965

Closed

plbossart requested changes May 23, 2019

View reviewed changes

zhuyingjiang force-pushed the topic/sof-dev-refine-suspend-with-stop-chip branch from dd69928 to f1fff35 Compare May 24, 2019 07:52

zhuyingjiang requested a review from ranj063 May 24, 2019 07:54

zhuyingjiang mentioned this pull request May 27, 2019

[APL][GLK] harden: delays after programming registers (and/or loops to check status changes) #777

Closed

zhuyingjiang force-pushed the topic/sof-dev-refine-suspend-with-stop-chip branch from f1fff35 to f19fe55 Compare May 27, 2019 05:04

zhuyingjiang requested review from RanderWang and plbossart May 27, 2019 05:08

RanderWang previously approved these changes May 29, 2019

View reviewed changes

kv2019i requested changes Jun 3, 2019

View reviewed changes

plbossart requested changes Jun 3, 2019

View reviewed changes

zhuyingjiang dismissed RanderWang’s stale review via ab34e87 June 4, 2019 02:32

zhuyingjiang force-pushed the topic/sof-dev-refine-suspend-with-stop-chip branch from f19fe55 to ab34e87 Compare June 4, 2019 02:32

zhuyingjiang requested review from RanderWang, kv2019i and plbossart June 4, 2019 02:34

kv2019i requested changes Jun 4, 2019

View reviewed changes

zhuyingjiang added 3 commits June 5, 2019 09:56

ASoC: SOF: Intel: hda: use the SOF defined ppcap functions

026c06a

Unify ppcap function setup by using SOF common functions for both HDA and non-HDA cases. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

ASoC: SOF: Intel: hda: use the defined stop chip in suspend

6322320

Unify suspend code by using SOF common function hda_dsp_ctrl_stop_chip() which can handle both HDA and non-HDA cases. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

zhuyingjiang force-pushed the topic/sof-dev-refine-suspend-with-stop-chip branch from ab34e87 to 6322320 Compare June 5, 2019 02:09

zhuyingjiang requested a review from kv2019i June 5, 2019 04:22

kv2019i approved these changes Jun 5, 2019

View reviewed changes

plbossart approved these changes Jun 5, 2019

View reviewed changes

plbossart merged commit 7f300c7 into thesofproject:topic/sof-dev Jun 5, 2019

Conversation

zhuyingjiang commented May 23, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang commented May 24, 2019

Uh oh!

zhuyingjiang commented May 29, 2019

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart commented May 29, 2019

Uh oh!

kv2019i left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang commented Jun 4, 2019

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang commented Jun 5, 2019

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kv2019i left a comment •

edited

Loading