Repository navigation
Conversation
The commit extends IPC interface between a driver and FW to support new power flows to be used with keyword detection feature. A new command is added to set request for power state transition request explicitly & definition of D0ix in addtion to D0 , D3 states is added. Signed-off-by: Lech Betlej <lech.betlej@linux.intel.com>
cAVS HW in a D0ix state does not guarantee access from host CPU to a memory shared with DSP. To enforce D0ix->D0 transtion host has to send the message using IPC registers only. The change enforces that host driver has to control MSG/MSGEXT bits while operating on IPC registers. Signed-off-by: Lech Betlej <lech.betlej@linux.intel.com>
lbetlej
changed the title
| /* get posn offset by pipeline. */ | ||
| int ipc_get_posn_offset(struct ipc *ipc, struct pipeline *pipe); | ||
|
|
||
| enum ipc_payload_location { |
There was a problem hiding this comment.
We add something globally that may not be widely supported. Shouldn't we just guard it with config for low power ipc and use it by default when available and fallback to mailbox on platforms that don't have lp ipc?
There was a problem hiding this comment.
@jajanusz I agree, all those defs are related to how the IPC data is transported on a specific set of platforms (Intel cAVS family). Therefore we should create another "transport" layer first and separate common upper logic, n both endpoints, the FW and the driver. Then, we may start introducing changes to the lower transport for Intel cAVS platform like the one proposed in the patch.
| SOF_PM_STATE_D0 = 0, /* D0 only, also initial state */ | ||
| SOF_PM_STATE_D3, /* D3 DSP subsystem is power gated*/ | ||
| /* internal FW transitions across D0ix substates incl. D0 are allowed */ | ||
| SOF_PM_STATE_D0ix, |
There was a problem hiding this comment.
Could we instead use a bitmap
Bit0: D0
Bit1: D3
Bit2: D0i1
Bit3: D0i2
Bit3: D0i3
This would keep the definitions for D0 and D3 and allow for new additions fairly easily.
There was a problem hiding this comment.
@plbossart I believe there is a similar concern about using bitmaps vs. "enums", part of the recent memory caps discussion (#1394 (comment)). Bitmap enables invalid combinations, not allowed when using enums (not necessarily coded as C enums if this is part of public inter processor API btw).
There was a problem hiding this comment.
@mmaka1 yes, I meant an explicit bitmap with the bit position defined, not an implicit one with the ':1: notation. In other words you have a uint32_t pm_state and its bits define state information.
There was a problem hiding this comment.
@plbossart @mmaka1 is the purpose of using bits to be able to specify supported power states e.g. as (D0 | D3 | D0i1)? In that case, yes, I also think using bits would make sense. However, if there's no plan to use such masks, I'm not sure bits make sense?. Sometimes also both are used like
enum {V1, V2, V3};
#define CAP_V1 BIT(V1)
#define CAP_V2 BIT(V2)
#define CAP_V3 BIT(V3)
| #define SOF_IPC_PM_CLK_REQ SOF_CMD_TYPE(0x006) | ||
| #define SOF_IPC_PM_CORE_ENABLE SOF_CMD_TYPE(0x007) | ||
| #define SOF_IPC_PM_STATE_SET SOF_CMD_TYPE(0x008) | ||
| #define SOF_IPC_PM_STATE_GET SOF_CMD_TYPE(0x009) |
There was a problem hiding this comment.
we would want to deprecated CTX_SAVE and use STATE_SET(D3) instead. And likewise CTX_RESTORE would be STATE_SET(D0).
It's not clear if we need a _GET if there is a reply to SET? In what case would the kernel need to get a state that it didn't set in the first place?
| enum sof_pm_pg { | ||
| SOF_PM_PG_ON = 0, /* Power gating enabled, */ | ||
| SOF_PM_PG_OFF, /* Power gating disabled */ | ||
| }; |
There was a problem hiding this comment.
why do we need these two definitions if we have boolean states for 'prevent_power_gating' and 'prevent_clock_gating'.
Or just like for power state should we have a bitfield that's extensible for power and clock gating?
| /* register based communication */ | ||
| uint32_t ipc_payload; /* SOF_IPC_PAYLOAD_... */ | ||
| uint32_t reg1; | ||
| uint32_t reg2; |
There was a problem hiding this comment.
I don't get why this is needed. I thought we would either
A) get the info directly from the memory windows
B) uncompress the information from the IPC register
In both cases, we'd handle the same structure in the end, so not sure why we would keep this information in a intermediate structure.
| /* get posn offset by pipeline. */ | ||
| int ipc_get_posn_offset(struct ipc *ipc, struct pipeline *pipe); | ||
|
|
||
| enum ipc_payload_location { |
There was a problem hiding this comment.
@jajanusz I agree, all those defs are related to how the IPC data is transported on a specific set of platforms (Intel cAVS family). Therefore we should create another "transport" layer first and separate common upper logic, n both endpoints, the FW and the driver. Then, we may start introducing changes to the lower transport for Intel cAVS platform like the one proposed in the patch.
| SOF_PM_STATE_D0 = 0, /* D0 only, also initial state */ | ||
| SOF_PM_STATE_D3, /* D3 DSP subsystem is power gated*/ | ||
| /* internal FW transitions across D0ix substates incl. D0 are allowed */ | ||
| SOF_PM_STATE_D0ix, |
There was a problem hiding this comment.
@plbossart I believe there is a similar concern about using bitmaps vs. "enums", part of the recent memory caps discussion (#1394 (comment)). Bitmap enables invalid combinations, not allowed when using enums (not necessarily coded as C enums if this is part of public inter processor API btw).
cAVS HW while FW is in a D0ix state does not guarantee access from host CPU to a memory shared with DSP. To enforce D0ix->D0 transtion host has to send the message using IPC registers only. The change enforces that host driver has to control MSG/MSGEXT bits while operating on IPC registers.
Note that this PR is to review the idea to add register based communication for cAVS HW for low power management stat transitions. Specifically implementation of recreating IPC structures from register content is not finalized.