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

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

On 2022-03-04 5:59 PM, Ranjani Sridharan wrote:
> On Fri, 2022-03-04 at 15:57 +0100, Cezary Rojewski 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?

>> of HDAudio streams during boot procedure causing CLDMA to become
>> obsolete. Once transferred, given stream is returned to pool
>> available
>> for audio streaming.
>>
>> 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).

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

>> +
>> +	return 0;
>> +
>> +err:
>> +	avs_dsp_core_disable(adev, spec->core_init_mask);
>> +	return ret;
>> +}
>> +

...

>> +int avs_hda_load_library(struct avs_dev *adev, struct firmware *lib,
>> u32 id)
>> +{
>> +	struct snd_pcm_substream substream;
>> +	struct snd_dma_buffer dmab;
>> +	struct hdac_ext_stream *estream;
>> +	struct hdac_stream *stream;
>> +	struct hdac_bus *bus = &adev->base.core;
>> +	unsigned int sdfmt;
>> +	int ret;
>> +
>> +	/* configure hda dma */
>> +	memset(&substream, 0, sizeof(substream));
>> +	substream.stream = SNDRV_PCM_STREAM_PLAYBACK;
>> +	estream = snd_hdac_ext_stream_assign(bus, &substream,
>> +					     HDAC_EXT_STREAM_TYPE_HOST)
>> ;
>> +	if (!estream)
>> +		return -ENODEV;
>> +	stream = hdac_stream(estream);
>> +
>> +	/* code loading performed with default format */
>> +	sdfmt = snd_hdac_calc_stream_format(48000, 1,
>> SNDRV_PCM_FORMAT_S32_LE, 32, 0);
>> +	ret = snd_hdac_dsp_prepare(stream, sdfmt, lib->size, &dmab);
>> +	if (ret < 0)
>> +		goto release_stream;
>> +
>> +	/* enable SPIB for hda stream */
>> +	snd_hdac_ext_stream_spbcap_enable(bus, true, stream->index);
>> +	snd_hdac_ext_stream_set_spib(bus, estream, lib->size);
>> +
>> +	memcpy(dmab.area, lib->data, lib->size);
>> +
>> +	/* transfer firmware */
>> +	snd_hdac_dsp_trigger(stream, true);
>> +	ret = avs_ipc_load_library(adev, stream->stream_tag - 1, id);
>> +	snd_hdac_dsp_trigger(stream, false);
>> +	if (ret) {
>> +		dev_err(adev->dev, "transfer lib %d failed: %d\n", id,
>> ret);
>> +		ret = AVS_IPC_RET(ret);
>> +	}
>> +
>> +	/* disable SPIB for hda stream */
>> +	snd_hdac_ext_stream_spbcap_enable(bus, false, stream->index);
>> +	snd_hdac_ext_stream_set_spib(bus, estream, 0);
>> +
>> +	snd_hdac_dsp_cleanup(stream, &dmab);
>> +release_stream:
>> +	snd_hdac_ext_stream_release(estream,
>> HDAC_EXT_STREAM_TYPE_HOST);
>> +
>> +	return ret;
>> +}
>> +
>> +int avs_hda_transfer_modules(struct avs_dev *adev, bool load,
>> +			     struct avs_module_entry *mods, u32
>> num_mods)
> What is the difference between transfer_modules and load_library?


Libraries targeting SKL/KBL/AML platforms *are just manifests* - the 
actual module code is found in the separate file(s) and thus the need 
for separate handlers. The driver has to load both the manifest and the 
module code to have a working streaming scenario requiring module type 
exposed by said library.


Regards,
Czarek