[Sound-open-firmware] [PATCH V3 1/2] ASoc: SOF: Add memory window for all platform
Pan, Xiuli
xiuli.pan at linux.intel.com
Tue Mar 6 05:56:22 CET 2018
On 3/5/2018 22:54, Pierre-Louis Bossart wrote:
>
>>>>
>>>> ---
>>>> 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.
>
> if there is an error then there should be a way to inform the caller
> than something went wrong - or add a dev_err.
I will add a dev_err here. This function is designed to parse the
message from FW. It expected that DSP give the right things, but may
need to debug if anything goes wrong.
>>
>>>
>>>> +
>>>> + 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.
>
> in the default case, all local variables remain set to zero, so that
> case is likely a major issue where you would need to return an error,
> not continue with a simple break.
>
> the same comments for the other platforms apply, error cases are not
> handled.
>
Will add a dev_err here too to inform that we got some illegal memory
window from the DSP. Will need to check the FW if we have send something
wrong.
Will also make some change to the hw-apl.c function.
May have some refine about the BAR value and memory window hard code,
then we may merge these functions into one function.
Thanks
Xiuli
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
More information about the Sound-open-firmware
mailing list