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

[Cezary Rojewski]

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.