diff mbox series

[01/26] ASoC: SOF: add a field to store the current D0 substate of DSP

Message ID 20191025224122.7718-2-pierre-louis.bossart@linux.intel.com (mailing list archive)
State Accepted
Commit 4c19030c511fd6eab029bae838f736256d2f43cd
Headers show
Series ASoC: SOF: enable S0ix support for Intel platforms | expand

Commit Message

Pierre-Louis Bossart Oct. 25, 2019, 10:40 p.m. UTC
From: Keyon Jie <yang.jie@linux.intel.com>

Add field d0_substate to struct snd_sof_dev to store the current DSP
D0 sub-state(only meaningful when DSP in D0), which could be D0I0 or
D0I3.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/sof-priv.h | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Cezary Rojewski Oct. 29, 2019, 10:20 a.m. UTC | #1
On 2019-10-26 00:40, Pierre-Louis Bossart wrote:
> +/* DSP D0ix sub-state */
> +enum sof_d0_substate {
> +	SOF_DSP_D0I0 = 0,	/* DSP default D0 substate */
> +	SOF_DSP_D0I3,		/* DSP D0i3(low power) substate*/
> +};

Name of the type states: "d0 substate" while description "D0ix 
sub-state". Why was not this named D0ix from the get-go? Goes into the 
same the same naming bucket as S0ix.

On the further note, adding D0ix patch within "enable S0ix support for 
Intel platforms" is misleading. S-states != D-states. D0ix is especially 
orthogonal. It is these to further reduce power consumption when system 
and device are in S0 and D0 respectively and idle time between IPC 
communication is long enough for DSP to be power gated.

Czarek
Pierre-Louis Bossart Oct. 29, 2019, 2:11 p.m. UTC | #2
On 10/29/19 5:20 AM, Cezary Rojewski wrote:
> On 2019-10-26 00:40, Pierre-Louis Bossart wrote:
>> +/* DSP D0ix sub-state */
>> +enum sof_d0_substate {
>> +    SOF_DSP_D0I0 = 0,    /* DSP default D0 substate */
>> +    SOF_DSP_D0I3,        /* DSP D0i3(low power) substate*/
>> +};
> 
> Name of the type states: "d0 substate" while description "D0ix 
> sub-state". Why was not this named D0ix from the get-go? Goes into the 
> same the same naming bucket as S0ix.

The definition is correct, from the pm_runtime perspective the device is 
'active' i.e. D0. D0ix is a substate of D0.

> 
> On the further note, adding D0ix patch within "enable S0ix support for 
> Intel platforms" is misleading. S-states != D-states. D0ix is especially 
> orthogonal. It is these to further reduce power consumption when system 
> and device are in S0 and D0 respectively and idle time between IPC 
> communication is long enough for DSP to be power gated.

there are well-defined requirements and dependencies between S and D states:

S0: device can be in D0, D0ix, D3
S0ix: device can be in D0ix, D3
S3: device can be in D3

That's hardly orthogonal.
diff mbox series

Patch

diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 2d40de5ee285..481dfe4ee2d0 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -68,6 +68,12 @@  extern int sof_core_debug;
 
 #define DMA_CHAN_INVALID	0xFFFFFFFF
 
+/* DSP D0ix sub-state */
+enum sof_d0_substate {
+	SOF_DSP_D0I0 = 0,	/* DSP default D0 substate */
+	SOF_DSP_D0I3,		/* DSP D0i3(low power) substate*/
+};
+
 struct snd_sof_dev;
 struct snd_sof_ipc_msg;
 struct snd_sof_ipc;
@@ -387,6 +393,9 @@  struct snd_sof_dev {
 	 */
 	struct snd_soc_component_driver plat_drv;
 
+	/* power states related */
+	enum sof_d0_substate d0_substate;
+
 	/* DSP firmware boot */
 	wait_queue_head_t boot_wait;
 	u32 boot_complete;