Re: [PATCH 12/14] ASoC: Intel: avs: Power management

[Cezary Rojewski <cezary.rojewski@xxxxxxxxx>]

On 2022-04-27 12:18 AM, Pierre-Louis Bossart wrote:
> On 4/26/22 12:23, Cezary Rojewski wrote:
> > To preserve power during sleep operations, handle suspend (S3),
> > hibernation (S4) and runtime (RTD3) transitions. As flow for all of
> > is shared, define common handlers to reduce code size.
> >
> > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
> > Signed-off-by: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
> > ---
> >   sound/soc/intel/avs/core.c | 125 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 125 insertions(+)
> >
> > diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
> > index 93180c22032d..c2f8fb87cfc2 100644
> > --- a/sound/soc/intel/avs/core.c
> > +++ b/sound/soc/intel/avs/core.c
> > @@ -536,6 +536,128 @@ static void avs_pci_remove(struct pci_dev *pci)
> >   	pm_runtime_get_noresume(&pci->dev);
> >   }
> >   
> > +static int __maybe_unused avs_suspend_common(struct avs_dev *adev, bool low_power)
>
> low_power is not used so....


Indeed, this argument should be part of the patch which introduces its 
usage. Removing in v2!

> > +{
> > +	struct hdac_bus *bus = &adev->base.core;
> > +	int ret;
> > +
> > +	flush_work(&adev->probe_work);
> > +
> > +	snd_hdac_ext_bus_link_power_down_all(bus);
> > +
> > +	ret = avs_ipc_set_dx(adev, AVS_MAIN_CORE_MASK, false);
> > +	/*
> > +	 * pm_runtime is blocked on DSP failure but system-wide suspend is not.
> > +	 * Do not block entire system from suspending if that's the case.
> > +	 */
> > +	if (ret && ret != -EPERM) {
> > +		dev_err(adev->dev, "set dx failed: %d\n", ret);
> > +		return AVS_IPC_RET(ret);
> > +	}
> > +
> > +	avs_dsp_op(adev, int_control, false);
> > +	snd_hdac_ext_bus_ppcap_int_enable(bus, false);
> > +
> > +	ret = avs_dsp_core_disable(adev, AVS_MAIN_CORE_MASK);
> > +	if (ret < 0) {
> > +		dev_err(adev->dev, "core_mask %ld disable failed: %d\n", AVS_MAIN_CORE_MASK, ret);
> > +		return ret;
> > +	}
> > +
> > +	snd_hdac_ext_bus_ppcap_enable(bus, false);
> > +	/* disable LP SRAM retention */
> > +	avs_hda_power_gating_enable(adev, false);
> > +	snd_hdac_bus_stop_chip(bus);
> > +	/* disable CG when putting controller to reset */
> > +	avs_hdac_clock_gating_enable(bus, false);
> > +	snd_hdac_bus_enter_link_reset(bus);
> > +	avs_hdac_clock_gating_enable(bus, true);
> > +
> > +	snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused avs_resume_common(struct avs_dev *adev, bool low_power, bool purge)
> > +{
> > +	struct hdac_bus *bus = &adev->base.core;
> > +	struct hdac_ext_link *hlink;
> > +	int ret;
> > +
> > +	snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
> > +	avs_hdac_bus_init_chip(bus, true);
> > +
> > +	snd_hdac_ext_bus_ppcap_enable(bus, true);
> > +	snd_hdac_ext_bus_ppcap_int_enable(bus, true);
> > +
> > +	ret = avs_dsp_boot_firmware(adev, purge);
> > +	if (ret < 0) {
> > +		dev_err(adev->dev, "firmware boot failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* 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);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused avs_suspend(struct device *dev)
> > +{
> > +	return avs_suspend_common(to_avs_dev(dev), true);
> > +}
> > +
> > +static int __maybe_unused avs_resume(struct device *dev)
> > +{
> > +	return avs_resume_common(to_avs_dev(dev), true, true);
> > +}
> > +
> > +static int __maybe_unused avs_runtime_suspend(struct device *dev)
> > +{
> > +	return avs_suspend_common(to_avs_dev(dev), true);
> > +}
> > +
> > +static int __maybe_unused avs_runtime_resume(struct device *dev)
> > +{
> > +	return avs_resume_common(to_avs_dev(dev), true, false);
> > +}
> > +
> > +static int __maybe_unused avs_freeze(struct device *dev)
> > +{
> > +	return avs_suspend_common(to_avs_dev(dev), false);
> > +}
> > +static int __maybe_unused avs_thaw(struct device *dev)
> > +{
> > +	return avs_resume_common(to_avs_dev(dev), false, true);
> > +}
> > +
> > +static int __maybe_unused avs_poweroff(struct device *dev)
> > +{
> > +	return avs_suspend_common(to_avs_dev(dev), false);
> > +}
> > +
> > +static int __maybe_unused avs_restore(struct device *dev)
> > +{
> > +	return avs_resume_common(to_avs_dev(dev), false, true);
> > +}
> > +
> > +static const struct dev_pm_ops avs_dev_pm = {
> > +	.suspend = avs_suspend,
> > +	.resume = avs_resume,
>
> ... all the 4 functions below seem unnecessary?


If we exclude 'low_power' then this is true. Will move this to the 
'low_power' patch as requested!

> > +	.freeze = avs_freeze,
> > +	.thaw = avs_thaw,
> > +	.poweroff = avs_poweroff,
> > +	.restore = avs_restore,
>
> we've historically never used those, what drives this new usage?
>
> on the SOF side, we've used prepare and complete, along with idle...


No streams may survive S4 what is not the case for S3. That's the main 
difference.

Yes, there are many opses vailable in dev_pm_ops, perhaps a different 
set of them is more appropriate. Let's have this talk once 'low_power' 
patch is here (PCM-power management patches are not part of this 
patchset but depend on it).

> > +	SET_RUNTIME_PM_OPS(avs_runtime_suspend, avs_runtime_resume, NULL)
> > +};
> > +
> >   static const struct pci_device_id avs_ids[] = {
> >   	{ 0 }
> >   };
> > @@ -546,6 +668,9 @@ static struct pci_driver avs_pci_driver = {
> >   	.id_table = avs_ids,
> >   	.probe = avs_pci_probe,
> >   	.remove = avs_pci_remove,
> > +	.driver = {
> > +		.pm = &avs_dev_pm,
> > +	},
> >   };
> >   module_pci_driver(avs_pci_driver);
> >