Re: [RFC PATCH 2/2] ASoc: tas2563: DSP Firmware loading support

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3057 bytes --]

On Tue, Jun 09, 2020 at 12:28:41PM -0500, Dan Murphy wrote:

>  	.val_bits = 8,
>  
> -	.max_register = 5 * 128,
> +	.max_register = 255 * 128,
>  	.cache_type = REGCACHE_RBTREE,
>  	.reg_defaults = tas2562_reg_defaults,
>  	.num_reg_defaults = ARRAY_SIZE(tas2562_reg_defaults),

Should some or all of the DSP memory be marked as volatile?  I guess if
we only write program to it then on reload after power off it should be
fine to just blast everything in again and ignore the fact that some
will have changed, but it might be helpful for debugging to be able to
read the live values back and do something more clever for restore.

>  #define TAS2562_PAGE_CTRL      0x00
> +#define TAS2562_BOOK_CTRL      0x7f

*sigh*  Of course the two levels of paging register are not located
anywhere near each other so we can't easily pretend they're one double
width page address.  :/

> +static int tas25xx_process_fw_single(struct tas2562_data *tas2562,
> +				     struct tas25xx_cmd_data *cmd_data,
> +				     u8 *fw_out)
> +{
> +	int num_writes = cpu_to_be16(cmd_data->length);
> +	int i;
> +	int ret;
> +	int offset = 4;
> +	int reg_data, write_reg;
> +
> +	for (i = 0; i < num_writes; i++) {
> +		/* Reset Page to 0 */
> +		ret = regmap_write(tas2562->regmap, TAS2562_PAGE_CTRL, 0);
> +		if (ret)
> +			return ret;

Why?

> +
> +		cmd_data->book = fw_out[offset];
> +		cmd_data->page = fw_out[offset + 1];
> +		cmd_data->offset = fw_out[offset + 2];
> +		reg_data = fw_out[offset + 3];
> +		offset += 4;
> +
> +		ret = regmap_write(tas2562->regmap, TAS2562_BOOK_CTRL,
> +				   cmd_data->book);
> +		if (ret)
> +			return ret;

This manual paging doesn't fill me with with joy especially with regard
to caching and doing the books behind the back of regmap.  I didn't spot
anything disabling cache or anything in the code.  I think you should
either bypass the cache while doing this or teach regmap about the
books (which may require core updates, I can't remember if the range
code copes with nested levels of paging - I remember thinking about it).

> +static ssize_t write_config_store(struct device *dev,
> +				struct device_attribute *tas25xx_attr,
> +				const char *buf, size_t size)
> +{

This looks like it could just be an enum (it looks like there's names we
could use) or just a simple numbered control?  Same for all the other
controls, they're just small integers so don't look hard to handle.  But
perhaps I'm missing something?

> +	tas2562->fw_data->fw_hdr = devm_kzalloc(tas2562->dev, hdr_size,
> +						GFP_KERNEL);
> +	if (!tas2562->fw_data->fw_hdr)
> +		return -ENOMEM;
> +
> +	memcpy(tas2562->fw_data->fw_hdr, &fw->data[0], hdr_size);

Should validate that the firmware is actually at least hdr_size big, and
similarly for all the other lengths we get from the header we should
check that there's actually enough data in the file.  ATM we just
blindly copy.

It'd also be good to double check that the number of configs and
programs is within bounds.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]