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

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

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

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

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