[Sound-open-firmware] [PATCH V3 1/2] ASoc: SOF: Add memory window for all platform

[Pierre-Louis Bossart]

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

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