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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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

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

> +		 */
> +		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

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

> +	return ret;
> +}

>  MODULE_LICENSE("GPL v2");

"GPL"