On 2022-02-25 9:46 PM, Pierre-Louis Bossart wrote:
+ 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.
again please do not assume that anyone reviewing this code has access to the firmware specification.
Provided simple description for the SET_D0IX message.
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.
it's not about judging, just explaining what is expected on the firmware side and what the driver needs to do.
Dropped all the "cavs < 2.0" unclear comments.
'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.
Provided a short comments instead.