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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux