Re: [PATCH v3 17/17] ASoC: Intel: avs: Code loading over HDA

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-03-04 7:56 PM, Pierre-Louis Bossart wrote:
> 
>>>> Compared to SKL and KBL, younger cAVS platforms are meant to re-use
>>>> one
>>> Younger? you mean newer?
>>
>>
>> Isn't the meaning of the two quite similar in this context?
> 
> younger doesn't sound quite right to me.
> 
> 'cAVS platforms more recent that SKL and KBL...'


Sure, can reword.

>>>> Module loading handler is dummy as library and module code became
>>>> inseparable in later firmware generations.
>>> replace dummy with "stub" maybe? lets use inclusive terms
>>
>>
>> Is 'dummy' categorized as non-inclusive? We have several usages of 
>> 'dummy' even within /sound (e.g. soc-utils.c).
> 
> Intel and plenty of other companies recommend a different wording. 
> mock-up, stub, placeholder, etc. see e.g.
> 
> https://developers.google.com/style/inclusive-documentation
> 
> Yes we have plenty of existing uses of 'dummy', but that doesn't mean we 
> should add new ones.


No problem with rewording this one in v4.

>>>> +static int
>>>> +avs_hda_init_rom(struct avs_dev *adev, unsigned int dma_id, bool
>>>> purge)
>>>> +{
>>>> +    const struct avs_spec *const spec = adev->spec;
>>>> +    unsigned int corex_mask, reg;
>>>> +    int ret;
>>>> +
>>>> +    corex_mask = spec->core_init_mask & ~AVS_MAIN_CORE_MASK;
>>>> +
>>>> +    ret = avs_dsp_op(adev, power, spec->core_init_mask, true);
>>>> +    if (ret < 0)
>>>> +        goto err;
>>>> +
>>>> +    ret = avs_dsp_op(adev, reset, AVS_MAIN_CORE_MASK, false);
>>>> +    if (ret < 0)
>>>> +        goto err;
>>>> +
>>>> +    reinit_completion(&adev->fw_ready);
>>>> +    avs_dsp_op(adev, int_control, true);
>>>> +
>>>> +    /* set boot config */
>>>> +    ret = avs_ipc_set_boot_config(adev, dma_id, purge);
>>>> +    if (ret) {
>>>> +        ret = AVS_IPC_RET(ret);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    /* await ROM init */
>>>> +    ret = snd_hdac_adsp_readq_poll(adev, spec->rom_status, reg,
>>>> +                       (reg & 0xF) == AVS_ROM_INIT_DONE
>>>> ||
>>>> +                       (reg & 0xF) ==
>>>> APL_ROM_FW_ENTERED,
>>>> +                       AVS_ROM_INIT_POLLING_US,
>>>> APL_ROM_INIT_TIMEOUT_US);
>>>> +    if (ret < 0) {
>>>> +        dev_err(adev->dev, "rom init timeout: %d\n", ret);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    /* power down non-main cores */
>>>> +    if (corex_mask)
>>>> +        avs_dsp_op(adev, power, corex_mask, false);
>>> What if this fails?
>>
>>
>> We are still in happy path, worst thing could happen here is increased 
>> power consumption.
> 
> 'happy path'?

The question is whether we value user experience over error-checking 
this case. I've never seen this step failing and the step itself is 
quite.. extraordinary : ) It's also invoked on very limited number of 
cAVS platforms, for everything else it's just: 'return 0'.


Regards,
Czarek