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

[Cezary Rojewski]

On 2022-02-25 3:02 AM, 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.


>> +		 */
>> +		return 0;
>> +
>> +	ret = avs_ipc_set_dx(adev, mask, true);
>> +	return AVS_IPC_RET(ret);
>> +}
>> +
>> +static int avs_dsp_disable(struct avs_dev *adev, u32 core_mask)
>> +{
>> +	int ret;
>> +
>> +	ret = avs_ipc_set_dx(adev, core_mask, false);
>> +	if (ret)
>> +		return AVS_IPC_RET(ret);
>> +
>> +	return avs_dsp_core_disable(adev, core_mask);
>> +}
>> +
>> +static int avs_dsp_get_core(struct avs_dev *adev, u32 core_id)
>> +{
>> +	u32 mask;
>> +	int ret;
>> +
>> +	mask = BIT_MASK(core_id);
>> +	if (mask == AVS_MAIN_CORE_MASK)
>> +		/* nothing to do for main core */
>> +		return 0;
>> +	if (core_id >= adev->hw_cfg.dsp_cores) {
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +	adev->core_refs[core_id]++;
>> +	if (adev->core_refs[core_id] == 1) {
>> +		ret = avs_dsp_enable(adev, mask);
>> +		if (ret)
>> +			goto err_enable_dsp;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_enable_dsp:
>> +	adev->core_refs[core_id]--;
>> +err:
>> +	dev_err(adev->dev, "get core failed: %d\n", ret);
> 
> you should log which core could not be enabled


Good catch! Ack.

>> +	return ret;
>> +}
>> +
>> +static int avs_dsp_put_core(struct avs_dev *adev, u32 core_id)
>> +{
>> +	u32 mask;
>> +	int ret;
>> +
>> +	mask = BIT_MASK(core_id);
>> +	if (mask == AVS_MAIN_CORE_MASK)
>> +		/* nothing to do for main core */
>> +		return 0;
>> +	if (core_id >= adev->hw_cfg.dsp_cores) {
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +	adev->core_refs[core_id]--;
>> +	if (!adev->core_refs[core_id]) {
>> +		ret = avs_dsp_disable(adev, mask);
>> +		if (ret)
>> +			goto err;
>> +	}
>> +
>> +	return 0;
>> +err:
>> +	dev_err(adev->dev, "put core failed: %d\n", ret);
> 
> put core %d


Ack.

>> +	return ret;
>> +}
> 
>>   MODULE_LICENSE("GPL v2");
> 
> "GPL"


Ack.