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

[zhuyingjiang]

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.
Use hda_dsp_ctrl_init_chip as common code for hda_resume.
fix part of #777

[plbossart]

I cannot approve this PR. While it's good to add clean-ups, it contains too many changes without justifications that seem very scary to merge. Please double-check your changes and make sure they don't change the programming sequences. If you do change the programming sequence, then you need an explanation for why it didn't work before.

[plbossart]

that's not an iso-feature change for the case where CONFIG_SND_SOC_SOF_HDA is not defined. You are doing a lot more and you'd need to explain a bit more the rationale for the changes.

[zhuyingjiang]

@plbossart The added steps are :
1 disable interrupts in stream descriptor
2 disable SIE for all streams
3 clear stream status
4 clear WAKESTS
5 clear interrupt status register
So all these are disable interrupts and clear registers of the streams, these are needed when shutdown/suspend to D3. Even with no HDA codecs.
So the previous code less them, although it seems still no problem with the DSP power off and on.
The PR has been tested with APL/WHL with HDA/nocodec mode.

[plbossart]

That one I could live with, but I would need a separate PR that adds those steps with the existing code. Once it's proven there is no impact we can do the code optimization. Doing all the changes at once is too risky and difficult to bisect.

[zhuyingjiang]

OK, #975 is the separate PR for this. I will modify the last commit and send a separate PR for the misc clock gating with init chip.

[plbossart]

thats seems wrong. You are changing the programming sequence here by resetting things without taking care of the clock gating. Why?

[zhuyingjiang]

Within @lbetlej 's email:
"CGCTL.MISCBDCGE needs to be disabled during controller reset "
So hda_dsp_ctrl_misc_clock_gating are disabled before controller reset and enabled after that.
with regard to "Reset stream-to-link mapping", there is no need to disable CGCTL.MISCBDCGE before that and then enable after.
So there are 2 solution:
1). move "Reset stream-to-link mapping" into hda_dsp_ctrl_init_chip and before enable CGCTL.MISCBDCGE
2). don't need CGCTL.MISCBDCGE operations before and after "Reset stream-to-link mapping".
With testing 2) on APL/WHL with HDA mode, it seems there is no difference if don't do that.
@lbetlej do we need disable and enable CGCTL.MISCBDCGE for "Reset stream-to-link mapping"?which solution is right in your opinion?

[plbossart]

@zhuyingjiang you are playing with fire here. The sequence has been used for several years, and it'll take architectural pointers and a lot more experimental evidence to convince me to change something which is not broken.

[zhuyingjiang]

@zhuyingjiang you are playing with fire here. The sequence has been used for several years, and it'll take architectural pointers and a lot more experimental evidence to convince me to change something which is not broken.

@plbossart let me explain more here, the first attempt to change here is because the code difference with SST,
In SST the code is:
static int skl_init_chip(struct hdac_bus *bus, bool full_reset)
{
struct hdac_ext_link hlink;
int ret;
skl_enable_miscbdcge(bus->dev, false);
ret = snd_hdac_bus_init_chip(bus, full_reset);
/ Reset stream-to-link mapping */
list_for_each_entry(hlink, &bus->hlink_list, list)
bus->io_ops->reg_writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV);
skl_enable_miscbdcge(bus->dev, true);
return ret;
}

But in our code it is:

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
/* reset and start hda controller /
ret = hda_dsp_ctrl_init_chip(sdev, true);
==>hda_dsp_ctrl_misc_clock_gating(sdev, false);
==>hda_dsp_ctrl_link_reset
==>...
==>hda_dsp_ctrl_misc_clock_gating(sdev, true);
hda_dsp_ctrl_misc_clock_gating(sdev, false);
/ Reset stream-to-link mapping /
list_for_each_entry(hlink, &bus->hlink_list, list)
bus->io_ops->reg_writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV);
hda_dsp_ctrl_misc_clock_gating(sdev, true);
/ enable ppcap interrupt */
snd_hdac_ext_bus_ppcap_enable(bus, true);
snd_hdac_ext_bus_ppcap_int_enable(bus, true);
#else

So in our code there will be disable and enable clock gating twice.

And a more important difference is in probe,
The SST skl_probe_work which called skl_init_chip still do the below steps:
/* Reset stream-to-link mapping */
list_for_each_entry(hlink, &bus->hlink_list, list)
bus->io_ops->reg_writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV);

but in our code, there is no such step in probe.

So in conclusion, I think the solution 1) I mentioned above may be better (move "Reset stream-to-link mapping" into hda_dsp_ctrl_init_chip and before enable CGCTL.MISCBDCGE inside. the programming sequence goes the same steps with SST),
and for 2) , it changed a little much and may leads to uncertain results.

I will change the PR according to solution 1) and request for comments.

[zhuyingjiang]

@plbossart So this is request for comments, and are tested for APL/WHL with HDA/nocodec all 4 cases.
Some are register programming sequences better have.(disable irq and clear registers for stream before shutdown)
Others are from @lbetlej 's email and compare to SST driver steps.

[zhuyingjiang]

@plbossart #975 is the separate PR for refine suspend with stop chip, and ppcap. I modified the last commit with solution 1) I mentioned above, and for RFC. Will submit it after #975 is merged.

[zhuyingjiang]

Close this since this is separated as:
#975 for stop chip in suspend.
#1020 for init chip in resume.