Re: [PATCH 3/5] ASoC: Intel: Add Intel SST audio DSP Firmware loader.

[Liam Girdwood <liam.r.girdwood@linux.intel.com>]

On Fri, 2014-02-14 at 09:55 +0100, Takashi Iwai wrote:
> At Thu, 13 Feb 2014 19:15:28 +0000,
> Liam Girdwood wrote:
> > 
> > +
> > +/* create new generic firmware object */
> > +struct sst_fw *sst_fw_new(struct sst_dsp *dsp,
> > +	const struct firmware *fw, void *private)
> > +{
> > +	struct sst_fw *sst_fw;
> > +	int err;
> > +
> > +	if (!dsp->ops->parse_fw)
> > +		return NULL;
> > +
> > +	sst_fw = kzalloc(sizeof(*sst_fw), GFP_KERNEL);
> > +	if (sst_fw == NULL)
> > +		return NULL;
> > +
> > +	sst_fw->dsp = dsp;
> > +	sst_fw->private = private;
> > +	sst_fw->size = fw->size;
> > +
> > +	err = dma_coerce_mask_and_coherent(dsp->dev, DMA_BIT_MASK(32));
> > +	if (err < 0) {
> > +		kfree(sst_fw);
> > +		return NULL;
> > +	}
> > +
> > +	/* allocate DMA buffer to store FW data */
> > +	sst_fw->dma_buf = dma_alloc_coherent(dsp->dev, sst_fw->size,
> > +				&sst_fw->dmable_fw_paddr, GFP_DMA);
> > +	if (!sst_fw->dma_buf) {
> > +		dev_err(dsp->dev, "error: DMA alloc failed\n");
> > +		kfree(sst_fw);
> > +		return NULL;
> > +	}
> > +
> > +	/* copy FW data to DMA-able memory */
> > +	memcpy((void *)sst_fw->dma_buf, (void *)fw->data, fw->size);
> > +	release_firmware(fw);
> 
> Should fw object be really released here?
> For example, if this function returns NULL after this point, the
> caller doesn't know whether fw was released or not.  It'd be more
> consistent to release it in the caller side, or always release no
> matter whether the function succeeds or not.

Yep, your right. I'll fix this in V2. Fwiw, we only need to take a copy
of the FW and store that copy in DMA-able memory (32bit addressable). So
the DMA memory will be used for context restore (from resume), DMA
firmware loading etc.

> 

> > +
> > +/* allocate contiguous free DSP blocks - callers hold locks */
> > +static int block_alloc_contiguous(struct sst_module *module,
> > +	struct sst_module_data *data, u32 next_offset, int size)
> > +{
> > +	struct sst_dsp *dsp = module->dsp;
> > +	struct sst_mem_block *block, *tmp;
> > +	int ret;
> > +
> > +	/* find first free blocks that can hold module */
> > +	list_for_each_entry_safe(block, tmp, &dsp->free_block_list, list) {
> > +
> > +		/* ignore blocks that dont match type */
> > +		if (block->type != data->type)
> > +			continue;
> > +
> > +		/* is block next after parent ? */
> > +		if (next_offset == block->offset) {
> > +
> > +			/* do we need more blocks */
> > +			if (size > block->size) {
> > +				ret = block_alloc_contiguous(module,
> > +					data, block->offset + block->size,
> > +					size - block->size);
> > +				if (ret < 0)
> > +					return ret;
> 
> How many contiguous blocks can be?

In theory, the whole DSP memory can be allocated as a contiguous block,
but in practice it's only really a few blocks contiguous per module atm.

> The recursive call for each one block doesn't look scaling.
> 

It seems to work atm, but I'll update this too for V2.

Liam