[PATCH 13/17] ASoC: Intel: avs: Dynamic firmware resources management

[Pierre-Louis Bossart]

On 2/28/22 09:30, Cezary Rojewski wrote:
> On 2022-02-25 9:21 PM, Pierre-Louis Bossart wrote:
>>>>> +static int avs_dsp_enable(struct avs_dev *adev, u32 core_mask)
>>>>> +{
>>>>> +    u32 mask;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = avs_dsp_core_enable(adev, core_mask);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    mask = core_mask & ~AVS_MAIN_CORE_MASK;
>>>>
>>>> so here BIT(MAIN_CORE) is zero in mask
>>>
>>>
>>> What's wrong with AVS_MAIN_CORE_MASK being used explicitly?
>>>
>>>>> +    if (!mask)
>>>>> +        /*
>>>>> +         * without main core, fw is dead anyway
>>>>> +         * so setting D0 for it is futile.
>>>>
>>>> I don't get the comment, you explicitly discarded the main core with
>>>> your logical AND above, so this test means that all other non-main
>>>> cores
>>>> are disabled.
>>>
>>> There is no setting D0 for MAIN_CORE as firmware is not operational
>>> without it. Firmware needs to be notified about D3 -> D0 transitions
>>> only in case of non-MAIN_COREs.
>>
>> the comment was about 'without main core'.
>>
>> This is difficult to follow, because you've discarded the main code in
>> the if (!mask), so that's an always-true case, which makes the rest of
>> the explanations not so clear.
> 
> 
> I'm afraid, not seeing the problem. It's clearly not always-true
> statement as 'mask' can be non-zero after discarding the MAIN_CORE.

mask = core_mask & ~AVS_MAIN_CORE_MASK;

-> main core is ignored.

comment says "without main core, fw is dead anyway"

since you ignored the main core, is fw dead in all cases?