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

[Cezary Rojewski]

On 2022-02-25 2:37 AM, Pierre-Louis Bossart wrote:
>> 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?


Ack.

>> and IPC operations. Here, Dx and D0ix IPC handlers are added.
>>
>> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski at linux.intel.com>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski at 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?


Yes. There is no putting MAIN_CORE to D0 as we must be in D0 to begin 
with, if we're thinking about sending an IPC to the base firmware.

>> +	 */
>> +	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.


Sure, will improve the wording.

>> +	msg.ext.set_d0ix.wake = enable_pg;
> 
> simplify the argument? Not sure anyone could understand what wake and
> enable_pg mean.


Well, CG and PG are popular shortcuts among Intel audio team and stand 
for clock gating and power gating respectively. 'wake' is firmware 
specific. I can provide a comment, but not all question are going to be 
answered by it. Firmware specification is the place to find the answer 
for most of these.

> 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.


Some of the fields are confusing and I agree, but driver has to adhere 
to FW expectations if it wants to be a working one. I would like to 
avoid judging the firmware architecture here, regardless of how 
confusing we think it is.

'wake' and 'streaming' fields are part of SET_D0ix message is which part 
of MODULE-type message interface. Base firmware is, from architecture 
point of view, a module of type=0 (module_id) and instance id=0 
(instance_id).

>> +			} 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?


Can provide a short kernel-doc for these two, sure.