Skip to content

[RFC] [Do not merge] ipc: support for sending ipc payload using registers added #1339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lbetlej wants to merge 2 commits into from
Closed

[RFC] [Do not merge] ipc: support for sending ipc payload using registers added #1339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee32227
ipc: pm_state_set/get commands added
lbetlej Feb 1, 2019
851ff9f
ipc: support for sending ipc payload using registers added
lbetlej Apr 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/drivers/intel/baytrail/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ void ipc_platform_do_cmd(struct ipc *ipc)

// TODO: signal audio work to enter D3 in normal context
/* are we about to enter D3 ? */
if (iipc->pm_prepare_D3) {
if (iipc->pm_target_state == SOF_PM_STATE_D3) {
while (1) {
wait_for_interrupt(0);
}
Expand Down Expand Up @@ -235,7 +235,7 @@ int platform_ipc_init(struct ipc *ipc)
iipc->dmac = dma_get(dir, caps, dev, DMA_ACCESS_SHARED);

/* PM */
iipc->pm_prepare_D3 = 0;
iipc->pm_target_state = SOF_PM_STATE_D0; /* initial value */

/* configure interrupt */
interrupt_register(PLATFORM_IPC_INTERRUPT, IRQ_AUTO_UNMASK,
Expand Down
37 changes: 29 additions & 8 deletions src/drivers/intel/cavs/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,27 +60,33 @@ extern struct ipc *_ipc;
static void ipc_irq_handler(void *arg)
{
uint32_t dipcctl;
struct ipc_data *iipc = ipc_get_drvdata(_ipc);

#if CAVS_VERSION == CAVS_VERSION_1_5
uint32_t dipct;
uint32_t dipcie;
uint32_t dipcte;

dipct = ipc_read(IPC_DIPCT);
dipcie = ipc_read(IPC_DIPCIE);
dipcctl = ipc_read(IPC_DIPCCTL);
dipcte = ipc_read(IPC_DIPCCTE);

tracev_ipc("ipc: irq dipct 0x%x dipcie 0x%x dipcctl 0x%x", dipct,
dipcie, dipcctl);
tracev_ipc("ipc: irq dipct 0x%x dipcie 0x%x dipcctl 0x%x dipcte 0x%x",
dipct, dipcie, dipcctl, dipcte);
#else
uint32_t dipctdr;
uint32_t dipcida;
uint32_t dipctdd;

dipctdr = ipc_read(IPC_DIPCTDR);
dipcida = ipc_read(IPC_DIPCIDA);
dipcctl = ipc_read(IPC_DIPCCTL);
dipctdd = ipc_read(IPC_DIPCTDD);

tracev_ipc("ipc: irq dipctdr 0x%x dipcida 0x%x dipcctl 0x%x", dipctdr,
dipcida, dipcctl);
tracev_ipc("ipc: irq dipctdr 0x%x dipcida 0x%x dipcctl 0x%x",
dipctdr, dipcida, dipcctl);
tracev_ipc("ipc: irq dipctdd 0x%x", dipctdd);
#endif

/* new message from host */
Expand All @@ -106,6 +112,22 @@ static void ipc_irq_handler(void *arg)
ipc_read(IPC_DIPCCTL));
#endif
} else {
#if CAVS_VERSION == CAVS_VERSION_1_5
if (dipct & IPC_DIPCT_MSG_MASK) {
#else
if (dipctdr & IPC_DIPCTDR_MSG_MASK) {
#endif
iipc->ipc_payload = SOF_IPC_PAYLOAD_REGISTER;
#if CAVS_VERSION == CAVS_VERSION_1_5
iipc->reg1 = dipct;
iipc->reg2 = dipcte;
#else
iipc->reg1 = dipctdr;
iipc->reg2 = dipctdd;
#endif
} else {
iipc->ipc_payload = SOF_IPC_PAYLOAD_MEMORY;
}
_ipc->host_pending = 1;
ipc_schedule_process(_ipc);
}
Expand Down Expand Up @@ -145,7 +167,6 @@ void ipc_platform_do_cmd(struct ipc *ipc)

/* perform command and return any error */
err = ipc_cmd();

/* if err > 0, reply created and copied by cmd() */
if (err <= 0) {
/* send std error/ok reply */
Expand All @@ -160,7 +181,7 @@ void ipc_platform_do_cmd(struct ipc *ipc)

/* are we about to enter D3 ? */
#if CAVS_VERSION < CAVS_VERSION_2_0
if (iipc->pm_prepare_D3) {
if (iipc->pm_target_state == SOF_PM_STATE_D3) {
/* no return - memory will be powered off and IPC sent */
platform_pm_runtime_power_off();
}
Expand All @@ -178,7 +199,7 @@ void ipc_platform_do_cmd(struct ipc *ipc)
ipc_write(IPC_DIPCCTL, ipc_read(IPC_DIPCCTL) | IPC_DIPCCTL_IPCTBIE);

#if CAVS_VERSION == CAVS_VERSION_2_0
if (iipc->pm_prepare_D3) {
if (iipc->pm_target_state == SOF_PM_STATE_D3) {
//TODO: add support for Icelake
while (1)
wait_for_interrupt(5);
Expand Down Expand Up @@ -246,7 +267,7 @@ int platform_ipc_init(struct ipc *ipc)
ipc_process_task, _ipc, 0, 0);

/* PM */
iipc->pm_prepare_D3 = 0;
iipc->pm_target_state = SOF_PM_STATE_D0; /* initial value */

/* configure interrupt */
interrupt_register(PLATFORM_IPC_INTERRUPT, IRQ_AUTO_UNMASK,
Expand Down
4 changes: 2 additions & 2 deletions src/drivers/intel/cavs/sue-ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ void ipc_platform_do_cmd(struct ipc *ipc)

// TODO: signal audio work to enter D3 in normal context
/* are we about to enter D3 ? */
if (iipc->pm_prepare_D3) {
if (iipc->pm_target_state == SOF_PM_STATE_D3) {
while (1)
wait_for_interrupt(0);
}
Expand Down Expand Up @@ -137,7 +137,7 @@ int platform_ipc_init(struct ipc *ipc)
ipc_process_task, _ipc, 0, 0);

/* PM */
iipc->pm_prepare_D3 = 0;
iipc->pm_target_state = SOF_PM_STATE_D0; /* initial value */

return 0;
}
4 changes: 2 additions & 2 deletions src/drivers/intel/haswell/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ void ipc_platform_do_cmd(struct ipc *ipc)

// TODO: signal audio work to enter D3 in normal context
/* are we about to enter D3 ? */
if (iipc->pm_prepare_D3) {
if (iipc->pm_target_state == SOF_PM_STATE_D3) {
while (1) {
wait_for_interrupt(0);
}
Expand Down Expand Up @@ -225,7 +225,7 @@ int platform_ipc_init(struct ipc *ipc)
iipc->dmac = dma_get(dir, caps, dev, DMA_ACCESS_SHARED);

/* PM */
iipc->pm_prepare_D3 = 0;
iipc->pm_target_state = SOF_PM_STATE_D0; /* initial value */

/* configure interrupt */
interrupt_register(PLATFORM_IPC_INTERRUPT, IRQ_AUTO_UNMASK,
Expand Down
2 changes: 1 addition & 1 deletion src/host/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ int platform_ipc_init(struct ipc *ipc)
bzero(iipc->page_table, HOST_PAGE_SIZE);

/* PM */
iipc->pm_prepare_D3 = 0;
iipc->pm_target_state = SOF_PM_STATE_D0; /* initial value */

return 0;
}
Expand Down
13 changes: 12 additions & 1 deletion src/include/sof/ipc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
#include <sof/audio/component.h>
#include <sof/dma-trace.h>
#include <sof/preproc.h>
#include <uapi/ipc/pm.h>

struct sof;
struct dai_config;
Expand Down Expand Up @@ -204,14 +205,24 @@ int ipc_dma_trace_send_position(void);
/* get posn offset by pipeline. */
int ipc_get_posn_offset(struct ipc *ipc, struct pipeline *pipe);

enum ipc_payload_location {

@jajanusz jajanusz Apr 30, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@mmaka1 mmaka1 May 10, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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_IPC_PAYLOAD_MEMORY = 0,
SOF_IPC_PAYLOAD_REGISTER
};

/* private data for IPC */
struct ipc_data {
/* DMA */
struct dma *dmac;
uint8_t *page_table;

/* PM */
int pm_prepare_D3; /* do we need to prepare for D3 */
uint32_t pm_target_state; /* SOF_PM_STATE_D.. */

/* register based communication */
uint32_t ipc_payload; /* SOF_IPC_PAYLOAD_... */
uint32_t reg1;
uint32_t reg2;

@plbossart plbossart May 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

int ipc_cmd(void);
Expand Down
2 changes: 1 addition & 1 deletion src/include/uapi/abi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@

/** \brief SOF ABI version major, minor and patch numbers */
#define SOF_ABI_MAJOR 3
#define SOF_ABI_MINOR 6
#define SOF_ABI_MINOR 7
#define SOF_ABI_PATCH 0

/** \brief SOF ABI version number. Format within 32bit word is MMmmmppp */
Expand Down
2 changes: 2 additions & 0 deletions src/include/uapi/ipc/header.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@
#define SOF_IPC_PM_CLK_GET SOF_CMD_TYPE(0x005)
#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)

@plbossart plbossart May 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


/** \name DSP Command: Component runtime config - multiple different types
* @{
Expand Down
45 changes: 45 additions & 0 deletions src/include/uapi/ipc/pm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,49 @@ struct sof_ipc_pm_core_config {
uint32_t enable_mask;
} __attribute__((packed));

/* DSP subsystem power states */
enum sof_pm_state {
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,

@plbossart plbossart May 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mmaka1 mmaka1 May 10, 2019 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@plbossart plbossart May 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lyakh lyakh Aug 28, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

};

/* Flags to allow FW for applying internally clock gating policies */
enum sof_pm_cg {
SOF_PM_CG_ON = 0, /* Clock gating enabled, */
SOF_PM_CG_OFF, /* Clock gating disabled */
};

/* Flags to allow FW for applying power gating policies */
enum sof_pm_pg {
SOF_PM_PG_ON = 0, /* Power gating enabled, */
SOF_PM_PG_OFF, /* Power gating disabled */
};

@plbossart plbossart May 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


/* PM state - SOF_IPC_PM_STATE_SET */
struct sof_ipc_pm_state {
struct sof_ipc_cmd_hdr hdr;

uint32_t pm_state; /**< power state SOF_PM_STATE_D.. */
/**< flag to disable FW logic for autonomous D0ix<->D0 transitions */
uint32_t prevent_power_gating;
/**< flag to disable FW logic for autonomous clock gating */
uint32_t prevent_clock_gating;
/**< reserved for future use */
uint32_t reserved[8];
} __attribute__((packed));

/* PM params info reply - SOF_IPC_PM_STATE_GET */
struct sof_ipc_pm_state_reply {
struct sof_ipc_reply rhdr;
uint32_t pm_state; /**< power state SOF_PM_STATE_D.. */
/**< flag to disable FW logic for autonomous D0ix<->D0 transitions */
uint32_t prevent_power_gating;
/**< flag to disable FW logic for autonomous clock gating */
uint32_t prevent_clock_gating;
/**< reserved for future use */
uint32_t reserved[8];
} __attribute__((packed));

#endif
Loading