On 3/3/2018 09:38, Pierre-Louis Bossart wrote:
On 03/02/2018 03:21 AM, Xiuli Pan wrote:
From: Pan Xiuli xiuli.pan@linux.intel.com
Add memory window handler for BYT, HSW and BDW.
Signed-off-by: Pan Xiuli xiuli.pan@linux.intel.com
Can you please run checkpatch --strict and resubmit. Nothing personal, it's the same for everyone. I have no appetite for fixing this sort of formatting issues myself. See also comments in the code below.
CHECK: Comparison to NULL could be written "!sdev->info_window" #67: FILE: sound/soc/sof/hw-bdw.c:402: + if (sdev->info_window == NULL)
CHECK: Blank lines aren't necessary after an open brace '{' #71: FILE: sound/soc/sof/hw-bdw.c:406: + for (i = 0; i < sdev->info_window->num_windows; i++) {
CHECK: Alignment should match open parenthesis #79: FILE: sound/soc/sof/hw-bdw.c:414: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BDW_DSP_BAR] + inbox_offset,
CHECK: Alignment should match open parenthesis #86: FILE: sound/soc/sof/hw-bdw.c:421: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BDW_DSP_BAR] + outbox_offset,
CHECK: Alignment should match open parenthesis #91: FILE: sound/soc/sof/hw-bdw.c:426: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BDW_DSP_BAR] + elem->offset +
CHECK: Alignment should match open parenthesis #96: FILE: sound/soc/sof/hw-bdw.c:431: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BDW_DSP_BAR] + elem->offset +
CHECK: Alignment should match open parenthesis #103: FILE: sound/soc/sof/hw-bdw.c:438: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BDW_DSP_BAR] + stream_offset,
CHECK: Alignment should match open parenthesis #108: FILE: sound/soc/sof/hw-bdw.c:443: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BDW_DSP_BAR] + elem->offset +
CHECK: Alignment should match open parenthesis #117: FILE: sound/soc/sof/hw-bdw.c:452: + snd_sof_dsp_mailbox_init(sdev, inbox_offset, inbox_size, + outbox_offset, outbox_size);
CHECK: Alignment should match open parenthesis #147: FILE: sound/soc/sof/hw-bdw.c:480: + snd_sof_fw_parse_ext_data(sdev, + MBOX_OFFSET + sizeof(struct sof_ipc_fw_ready));
CHECK: Comparison to NULL could be written "!sdev->info_window" #189: FILE: sound/soc/sof/hw-byt.c:172: + if (sdev->info_window == NULL)
CHECK: Blank lines aren't necessary after an open brace '{' #193: FILE: sound/soc/sof/hw-byt.c:176: + for (i = 0; i < sdev->info_window->num_windows; i++) {
CHECK: Alignment should match open parenthesis #201: FILE: sound/soc/sof/hw-byt.c:184: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BYT_DSP_BAR] + inbox_offset,
CHECK: Alignment should match open parenthesis #208: FILE: sound/soc/sof/hw-byt.c:191: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BYT_DSP_BAR] + outbox_offset,
CHECK: Alignment should match open parenthesis #213: FILE: sound/soc/sof/hw-byt.c:196: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BYT_DSP_BAR] + elem->offset +
CHECK: Alignment should match open parenthesis #218: FILE: sound/soc/sof/hw-byt.c:201: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BYT_DSP_BAR] + elem->offset +
CHECK: Alignment should match open parenthesis #225: FILE: sound/soc/sof/hw-byt.c:208: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BYT_DSP_BAR] + stream_offset,
CHECK: Alignment should match open parenthesis #230: FILE: sound/soc/sof/hw-byt.c:213: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BYT_DSP_BAR] + elem->offset +
CHECK: Alignment should match open parenthesis #239: FILE: sound/soc/sof/hw-byt.c:222: + snd_sof_dsp_mailbox_init(sdev, inbox_offset, inbox_size, + outbox_offset, outbox_size);
CHECK: Alignment should match open parenthesis #269: FILE: sound/soc/sof/hw-byt.c:250: + snd_sof_fw_parse_ext_data(sdev, + MBOX_OFFSET + sizeof(struct sof_ipc_fw_ready));
CHECK: Comparison to NULL could be written "!sdev->info_window" #303: FILE: sound/soc/sof/hw-hsw.c:402: + if (sdev->info_window == NULL)
CHECK: Blank lines aren't necessary after an open brace '{' #307: FILE: sound/soc/sof/hw-hsw.c:406: + for (i = 0; i < sdev->info_window->num_windows; i++) {
CHECK: Alignment should match open parenthesis #315: FILE: sound/soc/sof/hw-hsw.c:414: + snd_sof_debugfs_create_item(sdev, + sdev->bar[HSW_DSP_BAR] + inbox_offset,
CHECK: Alignment should match open parenthesis #322: FILE: sound/soc/sof/hw-hsw.c:421: + snd_sof_debugfs_create_item(sdev, + sdev->bar[HSW_DSP_BAR] + outbox_offset,
CHECK: Alignment should match open parenthesis #327: FILE: sound/soc/sof/hw-hsw.c:426: + snd_sof_debugfs_create_item(sdev, + sdev->bar[HSW_DSP_BAR] + elem->offset +
CHECK: Alignment should match open parenthesis #332: FILE: sound/soc/sof/hw-hsw.c:431: + snd_sof_debugfs_create_item(sdev, + sdev->bar[HSW_DSP_BAR] + elem->offset +
CHECK: Alignment should match open parenthesis #339: FILE: sound/soc/sof/hw-hsw.c:438: + snd_sof_debugfs_create_item(sdev, + sdev->bar[HSW_DSP_BAR] + stream_offset,
CHECK: Alignment should match open parenthesis #344: FILE: sound/soc/sof/hw-hsw.c:443: + snd_sof_debugfs_create_item(sdev, + sdev->bar[HSW_DSP_BAR] + elem->offset +
CHECK: Alignment should match open parenthesis #353: FILE: sound/soc/sof/hw-hsw.c:452: + snd_sof_dsp_mailbox_init(sdev, inbox_offset, inbox_size, + outbox_offset, outbox_size);
CHECK: Alignment should match open parenthesis #382: FILE: sound/soc/sof/hw-hsw.c:479: + snd_sof_fw_parse_ext_data(sdev, + MBOX_OFFSET + sizeof(struct sof_ipc_fw_ready));
Test with: Mininow max rt5651 GP-MRB nocodec SOF master: 1693b66bb1d804ded975767cc1e5911e6ff9c93c SOF-Tool master: a02abb799405d0e4ad0d6bb46eacf6fbe958c06e https://github.com/plbossart/sound/tree/topic/sof-v4.14: 9513a73b981bc1917705671ec54402a7e21672eb
sound/soc/sof/hw-apl.c | 11 +++++-- sound/soc/sof/hw-bdw.c | 86 ++++++++++++++++++++++++++++++++++++++++++------- sound/soc/sof/hw-byt.c | 87 +++++++++++++++++++++++++++++++++++++++++++------- sound/soc/sof/hw-hsw.c | 85 +++++++++++++++++++++++++++++++++++++++++------- 4 files changed, 233 insertions(+), 36 deletions(-)
diff --git a/sound/soc/sof/hw-apl.c b/sound/soc/sof/hw-apl.c index bfbdd915..de0efe1 100644 --- a/sound/soc/sof/hw-apl.c +++ b/sound/soc/sof/hw-apl.c @@ -1089,9 +1089,13 @@ static int apl_prepare(struct snd_sof_dev *sdev, unsigned int format, static void apl_get_windows(struct snd_sof_dev *sdev) { struct sof_ipc_window_elem *elem; + u32 outbox_offset = 0; + u32 stream_offset = 0; + u32 inbox_offset = 0; + u32 outbox_size = 0; + u32 stream_size = 0; + u32 inbox_size = 0; int i; - u32 inbox_offset = 0, outbox_offset = 0; - u32 inbox_size = 0, outbox_size = 0; if (!sdev->info_window) return; @@ -1135,6 +1139,9 @@ static void apl_get_windows(struct snd_sof_dev *sdev) elem->size, "debug"); break; case SOF_IPC_REGION_STREAM: + stream_offset = + elem->offset + SRAM_WINDOW_OFFSET(elem->id); + stream_size = elem->size; snd_sof_debugfs_create_item(sdev, sdev->bar[APL_DSP_BAR] + elem->offset + diff --git a/sound/soc/sof/hw-bdw.c b/sound/soc/sof/hw-bdw.c index bc3141f..5ec0c5b 100644 --- a/sound/soc/sof/hw-bdw.c +++ b/sound/soc/sof/hw-bdw.c @@ -63,7 +63,6 @@ static const struct snd_sof_debugfs_map bdw_debugfs[] = { {"iram", BDW_DSP_BAR, IRAM_OFFSET, BDW_IRAM_SIZE}, {"dram", BDW_DSP_BAR, DRAM_OFFSET, BDW_DRAM_SIZE}, {"shim", BDW_DSP_BAR, SHIM_OFFSET, SHIM_SIZE}, - {"mbox", BDW_DSP_BAR, MBOX_OFFSET, MBOX_SIZE}, }; /* @@ -379,6 +378,75 @@ static irqreturn_t bdw_irq_thread(int irq, void *context) /* * IPC Firmware ready. */ +static void bdw_get_windows(struct snd_sof_dev *sdev) +{ + struct sof_ipc_window_elem *elem; + u32 outbox_offset = 0; + u32 stream_offset = 0; + u32 inbox_offset = 0; + u32 outbox_size = 0; + u32 stream_size = 0; + u32 inbox_size = 0; + int i;
+ if (sdev->info_window == NULL) + return;
with no error?
Sorry about this, copy the function base from the hw-apl.c. The function has no return type. What should we handle the error here.
+ for (i = 0; i < sdev->info_window->num_windows; i++) {
+ elem = &sdev->info_window->window[i];
+ switch (elem->type) { + case SOF_IPC_REGION_UPBOX: + inbox_offset = elem->offset + MBOX_OFFSET; + inbox_size = elem->size; + snd_sof_debugfs_create_item(sdev, + sdev->bar[BDW_DSP_BAR] + inbox_offset, + elem->size, "inbox"); + break; + case SOF_IPC_REGION_DOWNBOX: + outbox_offset = elem->offset + MBOX_OFFSET; + outbox_size = elem->size; + snd_sof_debugfs_create_item(sdev, + sdev->bar[BDW_DSP_BAR] + outbox_offset, + elem->size, "outbox"); + break; + case SOF_IPC_REGION_TRACE: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BDW_DSP_BAR] + elem->offset + + MBOX_OFFSET, elem->size, "etrace"); + break; + case SOF_IPC_REGION_DEBUG: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BDW_DSP_BAR] + elem->offset + + MBOX_OFFSET, elem->size, "debug"); + break; + case SOF_IPC_REGION_STREAM: + stream_offset = elem->offset + MBOX_OFFSET; + stream_size = elem->size; + snd_sof_debugfs_create_item(sdev, + sdev->bar[BDW_DSP_BAR] + stream_offset, + elem->size, "stream"); + break; + case SOF_IPC_REGION_REGS: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BDW_DSP_BAR] + elem->offset + + MBOX_OFFSET, elem->size, "regs"); + break; + default: + break;
so here you will init a mailbox of size zero? This looks like an error, why continue with the execution flow.
What do you mean size zero here? This is a dynamic parse for the memory window info passed from the DSP. We may change the memory window setting in the FW and here we just get and create the memory window what the DSP send to the host.
+ } + }
+ snd_sof_dsp_mailbox_init(sdev, inbox_offset, inbox_size, + outbox_offset, outbox_size);
+ dev_dbg(sdev->dev, " mailbox upstream 0x%x - size 0x%x\n", + inbox_offset, inbox_size); + dev_dbg(sdev->dev, " mailbox downstream 0x%x - size 0x%x\n", + outbox_offset, outbox_size); +}
static int bdw_fw_ready(struct snd_sof_dev *sdev, u32 msg_id) { struct sof_ipc_fw_ready *fw_ready = &sdev->fw_ready; @@ -394,19 +462,15 @@ static int bdw_fw_ready(struct snd_sof_dev *sdev, u32 msg_id) /* copy data from the DSP FW ready offset */ bdw_block_read(sdev, offset, fw_ready, sizeof(*fw_ready)); - snd_sof_dsp_mailbox_init(sdev, fw_ready->dspbox_offset, - fw_ready->dspbox_size, - fw_ready->hostbox_offset, - fw_ready->hostbox_size);
- dev_dbg(sdev->dev, " mailbox DSP initiated 0x%x - size 0x%x\n", - fw_ready->dspbox_offset, fw_ready->dspbox_size); - dev_dbg(sdev->dev, " mailbox Host initiated 0x%x - size 0x%x\n", - fw_ready->hostbox_offset, fw_ready->hostbox_size);
dev_info(sdev->dev, " Firmware info: version %d:%d-%s build %d on %s:%s\n", v->major, v->minor, v->tag, v->build, v->date, v->time); + /* now check for extended data */ + snd_sof_fw_parse_ext_data(sdev, + MBOX_OFFSET + sizeof(struct sof_ipc_fw_ready));
+ bdw_get_windows(sdev);
return 0; } diff --git a/sound/soc/sof/hw-byt.c b/sound/soc/sof/hw-byt.c index 209f72a..031cec6 100644 --- a/sound/soc/sof/hw-byt.c +++ b/sound/soc/sof/hw-byt.c @@ -73,7 +73,6 @@ static const struct snd_sof_debugfs_map byt_debugfs[] = { {"iram", BYT_DSP_BAR, IRAM_OFFSET, IRAM_SIZE}, {"dram", BYT_DSP_BAR, DRAM_OFFSET, DRAM_SIZE}, {"shim", BYT_DSP_BAR, SHIM_OFFSET, SHIM_SIZE}, - {"mbox", BYT_DSP_BAR, MBOX_OFFSET, MBOX_SIZE}, }; static const struct snd_sof_debugfs_map cht_debugfs[] = { @@ -89,7 +88,6 @@ static const struct snd_sof_debugfs_map cht_debugfs[] = { {"iram", BYT_DSP_BAR, IRAM_OFFSET, IRAM_SIZE}, {"dram", BYT_DSP_BAR, DRAM_OFFSET, DRAM_SIZE}, {"shim", BYT_DSP_BAR, SHIM_OFFSET, SHIM_SIZE}, - {"mbox", BYT_DSP_BAR, MBOX_OFFSET, MBOX_SIZE}, }; static void byt_dump(struct snd_sof_dev *sdev, u32 flags) @@ -197,6 +195,75 @@ static void byt_block_read(struct snd_sof_dev *sdev, u32 offset, void *dest, /* * IPC Firmware ready. */ +static void byt_get_windows(struct snd_sof_dev *sdev) +{ + struct sof_ipc_window_elem *elem; + u32 outbox_offset = 0; + u32 stream_offset = 0; + u32 inbox_offset = 0; + u32 outbox_size = 0; + u32 stream_size = 0; + u32 inbox_size = 0; + int i;
+ if (sdev->info_window == NULL) + return;
no error?
+ for (i = 0; i < sdev->info_window->num_windows; i++) {
+ elem = &sdev->info_window->window[i];
+ switch (elem->type) { + case SOF_IPC_REGION_UPBOX: + inbox_offset = elem->offset + MBOX_OFFSET; + inbox_size = elem->size; + snd_sof_debugfs_create_item(sdev, + sdev->bar[BYT_DSP_BAR] + inbox_offset, + elem->size, "inbox"); + break; + case SOF_IPC_REGION_DOWNBOX: + outbox_offset = elem->offset + MBOX_OFFSET; + outbox_size = elem->size; + snd_sof_debugfs_create_item(sdev, + sdev->bar[BYT_DSP_BAR] + outbox_offset, + elem->size, "outbox"); + break; + case SOF_IPC_REGION_TRACE: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BYT_DSP_BAR] + elem->offset + + MBOX_OFFSET, elem->size, "etrace"); + break; + case SOF_IPC_REGION_DEBUG: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BYT_DSP_BAR] + elem->offset + + MBOX_OFFSET, elem->size, "debug"); + break; + case SOF_IPC_REGION_STREAM: + stream_offset = elem->offset + MBOX_OFFSET; + stream_size = elem->size; + snd_sof_debugfs_create_item(sdev, + sdev->bar[BYT_DSP_BAR] + stream_offset, + elem->size, "stream"); + break; + case SOF_IPC_REGION_REGS: + snd_sof_debugfs_create_item(sdev, + sdev->bar[BYT_DSP_BAR] + elem->offset + + MBOX_OFFSET, elem->size, "regs"); + break; + default: + break;
same, this looks wrong.
+ } + }
+ snd_sof_dsp_mailbox_init(sdev, inbox_offset, inbox_size, + outbox_offset, outbox_size);
+ dev_dbg(sdev->dev, " mailbox upstream 0x%x - size 0x%x\n", + inbox_offset, inbox_size); + dev_dbg(sdev->dev, " mailbox downstream 0x%x - size 0x%x\n", + outbox_offset, outbox_size); +}
static int byt_fw_ready(struct snd_sof_dev *sdev, u32 msg_id) { struct sof_ipc_fw_ready *fw_ready = &sdev->fw_ready; @@ -212,19 +279,15 @@ static int byt_fw_ready(struct snd_sof_dev *sdev, u32 msg_id) /* copy data from the DSP FW ready offset */ byt_block_read(sdev, offset, fw_ready, sizeof(*fw_ready)); - snd_sof_dsp_mailbox_init(sdev, fw_ready->dspbox_offset, - fw_ready->dspbox_size, - fw_ready->hostbox_offset, - fw_ready->hostbox_size);
- dev_dbg(sdev->dev, " mailbox DSP initiated 0x%x - size 0x%x\n", - fw_ready->dspbox_offset, fw_ready->dspbox_size); - dev_dbg(sdev->dev, " mailbox Host initiated 0x%x - size 0x%x\n", - fw_ready->hostbox_offset, fw_ready->hostbox_size);
dev_info(sdev->dev, " Firmware info: version %d:%d-%s build %d on %s:%s\n", v->major, v->minor, v->tag, v->build, v->date, v->time); + /* now check for extended data */ + snd_sof_fw_parse_ext_data(sdev, + MBOX_OFFSET + sizeof(struct sof_ipc_fw_ready));
+ byt_get_windows(sdev);
return 0; } diff --git a/sound/soc/sof/hw-hsw.c b/sound/soc/sof/hw-hsw.c index c6366bb..2f36216 100644 --- a/sound/soc/sof/hw-hsw.c +++ b/sound/soc/sof/hw-hsw.c @@ -63,7 +63,6 @@ static const struct snd_sof_debugfs_map hsw_debugfs[] = { {"iram", HSW_DSP_BAR, IRAM_OFFSET, HSW_IRAM_SIZE}, {"dram", HSW_DSP_BAR, DRAM_OFFSET, HSW_DRAM_SIZE}, {"shim", HSW_DSP_BAR, SHIM_OFFSET, SHIM_SIZE}, - {"mbox", HSW_DSP_BAR, MBOX_OFFSET, MBOX_SIZE}, }; /* @@ -380,6 +379,75 @@ static irqreturn_t hsw_irq_thread(int irq, void *context) /* * IPC Firmware ready. */ +static void hsw_get_windows(struct snd_sof_dev *sdev) +{ + struct sof_ipc_window_elem *elem; + u32 outbox_offset = 0; + u32 stream_offset = 0; + u32 inbox_offset = 0; + u32 outbox_size = 0; + u32 stream_size = 0; + u32 inbox_size = 0; + int i;
+ if (sdev->info_window == NULL) + return;
no error?
+ for (i = 0; i < sdev->info_window->num_windows; i++) {
+ elem = &sdev->info_window->window[i];
+ switch (elem->type) { + case SOF_IPC_REGION_UPBOX: + inbox_offset = elem->offset + MBOX_OFFSET; + inbox_size = elem->size; + snd_sof_debugfs_create_item(sdev, + sdev->bar[HSW_DSP_BAR] + inbox_offset, + elem->size, "inbox"); + break; + case SOF_IPC_REGION_DOWNBOX: + outbox_offset = elem->offset + MBOX_OFFSET; + outbox_size = elem->size; + snd_sof_debugfs_create_item(sdev, + sdev->bar[HSW_DSP_BAR] + outbox_offset, + elem->size, "outbox"); + break; + case SOF_IPC_REGION_TRACE: + snd_sof_debugfs_create_item(sdev, + sdev->bar[HSW_DSP_BAR] + elem->offset + + MBOX_OFFSET, elem->size, "etrace"); + break; + case SOF_IPC_REGION_DEBUG: + snd_sof_debugfs_create_item(sdev, + sdev->bar[HSW_DSP_BAR] + elem->offset + + MBOX_OFFSET, elem->size, "debug"); + break; + case SOF_IPC_REGION_STREAM: + stream_offset = elem->offset + MBOX_OFFSET; + stream_size = elem->size; + snd_sof_debugfs_create_item(sdev, + sdev->bar[HSW_DSP_BAR] + stream_offset, + elem->size, "stream"); + break; + case SOF_IPC_REGION_REGS: + snd_sof_debugfs_create_item(sdev, + sdev->bar[HSW_DSP_BAR] + elem->offset + + MBOX_OFFSET, elem->size, "regs"); + break; + default: + break;
same here, this looks wrong.
+ } + }
+ snd_sof_dsp_mailbox_init(sdev, inbox_offset, inbox_size, + outbox_offset, outbox_size);
+ dev_dbg(sdev->dev, " mailbox upstream 0x%x - size 0x%x\n", + inbox_offset, inbox_size); + dev_dbg(sdev->dev, " mailbox downstream 0x%x - size 0x%x\n", + outbox_offset, outbox_size); +}
static int hsw_fw_ready(struct snd_sof_dev *sdev, u32 msg_id) { struct sof_ipc_fw_ready *fw_ready = &sdev->fw_ready; @@ -395,18 +463,13 @@ static int hsw_fw_ready(struct snd_sof_dev *sdev, u32 msg_id) /* copy data from the DSP FW ready offset */ hsw_block_read(sdev, offset, fw_ready, sizeof(*fw_ready)); - snd_sof_dsp_mailbox_init(sdev, fw_ready->dspbox_offset, - fw_ready->dspbox_size, - fw_ready->hostbox_offset, - fw_ready->hostbox_size);
- dev_dbg(sdev->dev, " mailbox DSP initiated 0x%x - size 0x%x\n", - fw_ready->dspbox_offset, fw_ready->dspbox_size); - dev_dbg(sdev->dev, " mailbox Host initiated 0x%x - size 0x%x\n", - fw_ready->hostbox_offset, fw_ready->hostbox_size);
dev_info(sdev->dev, " Firmware info: version %d:%d-%s build %d on %s:%s\n", v->major, v->minor, v->tag, v->build, v->date, v->time); + /* now check for extended data */ + snd_sof_fw_parse_ext_data(sdev, + MBOX_OFFSET + sizeof(struct sof_ipc_fw_ready));
+ hsw_get_windows(sdev); return 0; }