Re: [PATCH 08/17] ASoC: Intel: avs: Add power management requests

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

> Audio DSP supports low power states i.e.: transitions between D0 and D3
> and D0-substates in form of D0i3. That process is a combination of core

D0i0 and D0i3?

> and IPC operations. Here, Dx and D0ix IPC handlers are added.
> 
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  sound/soc/intel/avs/messages.c | 43 ++++++++++++++++++++++++++++++++++
>  sound/soc/intel/avs/messages.h | 16 +++++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/sound/soc/intel/avs/messages.c b/sound/soc/intel/avs/messages.c
> index e870d5792a77..1b589689410f 100644
> --- a/sound/soc/intel/avs/messages.c
> +++ b/sound/soc/intel/avs/messages.c
> @@ -347,3 +347,46 @@ int avs_ipc_get_large_config(struct avs_dev *adev, u16 module_id, u8 instance_id
>  
>  	return 0;
>  }
> +
> +int avs_ipc_set_dx(struct avs_dev *adev, u32 core_mask, bool powerup)
> +{
> +	union avs_module_msg msg = AVS_MODULE_REQUEST(SET_DX);
> +	struct avs_ipc_msg request;
> +	struct avs_dxstate_info dx;
> +	int ret;
> +
> +	dx.core_mask = core_mask;
> +	dx.dx_mask = powerup ? core_mask : 0;
> +	request.header = msg.val;
> +	request.data = &dx;
> +	request.size = sizeof(dx);
> +
> +	/*
> +	 * SET_D0 is sent for non-main cores only while SET_D3 is used to
> +	 * suspend for all of them. Both cases prevent any D0I3 transitions.

The asymmetry in the comment isn't clear. Did you mean that the main
code is in D0 when powered?

> +	 */
> +	ret = avs_dsp_send_pm_msg(adev, &request, NULL, true);
> +	if (ret)
> +		avs_ipc_err(adev, &request, "set dx", ret);
> +
> +	return ret;
> +}
> +
> +int avs_ipc_set_d0ix(struct avs_dev *adev, bool enable_pg, bool streaming)
> +{
> +	union avs_module_msg msg = AVS_MODULE_REQUEST(SET_D0IX);
> +	struct avs_ipc_msg request = {0};
> +	int ret;
> +
> +	/* Wake & streaming for < cAVS 2.0 */

I don't how anyone outside of Intel could understand this comment.
Consider explaining what the two terms refer to.

> +	msg.ext.set_d0ix.wake = enable_pg;

simplify the argument? Not sure anyone could understand what wake and
enable_pg mean.

int avs_ipc_set_d0ix(struct avs_dev *adev, bool wake, bool streaming)

> +	msg.ext.set_d0ix.streaming = streaming;
> +
> +	request.header = msg.val;
> +
> +	ret = avs_dsp_send_pm_msg(adev, &request, NULL, false);
> +	if (ret)
> +		avs_ipc_err(adev, &request, "set d0ix", ret);
> +
> +	return ret;
> +}
> diff --git a/sound/soc/intel/avs/messages.h b/sound/soc/intel/avs/messages.h
> index 1dabd1005327..bbdba4631b1f 100644
> --- a/sound/soc/intel/avs/messages.h
> +++ b/sound/soc/intel/avs/messages.h
> @@ -101,6 +101,8 @@ enum avs_module_msg_type {
>  	AVS_MOD_LARGE_CONFIG_SET = 4,
>  	AVS_MOD_BIND = 5,
>  	AVS_MOD_UNBIND = 6,
> +	AVS_MOD_SET_DX = 7,
> +	AVS_MOD_SET_D0IX = 8,
>  	AVS_MOD_DELETE_INSTANCE = 11,
>  };
>  
> @@ -137,6 +139,11 @@ union avs_module_msg {
>  				u32 dst_queue:3;
>  				u32 src_queue:3;
>  			} bind_unbind;
> +			struct {
> +				/* cAVS < 2.0 */
> +				u32 wake:1;
> +				u32 streaming:1;

you probably want to explain how a 'streaming' flag is set at the module
level? One would think all modules connected in a pipeline would need to
use the same flag value.

> +			} set_d0ix;
>  		} ext;
>  	};
>  } __packed;
> @@ -298,4 +305,13 @@ int avs_ipc_get_large_config(struct avs_dev *adev, u16 module_id, u8 instance_id
>  			     u8 param_id, u8 *request_data, size_t request_size,
>  			     u8 **reply_data, size_t *reply_size);
>  
> +/* DSP cores and domains power management messages */
> +struct avs_dxstate_info {
> +	u32 core_mask;
> +	u32 dx_mask;

what is the convention for D0 and D3 in the mask ? which one is 0 or 1?

Is this also handled in a hierarchical way where only the bits set in
core_mask matter?

> +} __packed;
> +
> +int avs_ipc_set_dx(struct avs_dev *adev, u32 core_mask, bool powerup);
> +int avs_ipc_set_d0ix(struct avs_dev *adev, bool enable_pg, bool streaming);
> +
>  #endif /* __SOUND_SOC_INTEL_AVS_MSGS_H */