[1/2] ASoc: tas2563: DSP Firmware loading support

The TAS2563 device has a DSP that can run firmware.
The firmware can contain up to 5 programs and 10 configurations to
support alternate audio processing.

Firmware loading is done dymacally and the program and configuration
selection is done by the user.

The binary itself contains a list of instructions for either a single
mode write or a burst write.  The single mode write is list of register
writes to different books and pages within the register map.
The burst writes is a block of data that is written to a specific
location in memory.

The firmware loader must parse and load the blocks in real time as the
binary may contain different audio profiles.

If the DSP is not needed to do audio preocessing then the DSP program
can be turned off and the device will effectively turn off the DSP.

RFC patch:
https://lore.kernel.org/lkml/6d6aaed3-dac8-e1ec-436c-9b04273df2b3@ti.com/T/

Co-developed-by: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
 {sound/soc/codecs => include/sound}/tas2562.h |   3 +
 include/sound/tas25xx-dsp.h                   | 100 +++++++
 sound/soc/codecs/Kconfig                      |   7 +
 sound/soc/codecs/Makefile                     |   2 +
 sound/soc/codecs/tas2562.c                    |   2 +-
 sound/soc/codecs/tas25xx-dsp.c                | 282 ++++++++++++++++++
 6 files changed, 395 insertions(+), 1 deletion(-)
 rename {sound/soc/codecs => include/sound}/tas2562.h (97%)
 create mode 100644 include/sound/tas25xx-dsp.h
 create mode 100644 sound/soc/codecs/tas25xx-dsp.c

> Firmware loading is done dymacally and the program and configuration

dynamically

> selection is done by the user.
> 
> The binary itself contains a list of instructions for either a single
> mode write or a burst write.  The single mode write is list of register
> writes to different books and pages within the register map.
> The burst writes is a block of data that is written to a specific
> location in memory.
> 
> The firmware loader must parse and load the blocks in real time as the
> binary may contain different audio profiles.
> 
> If the DSP is not needed to do audio preocessing then the DSP program

preprocessing

> can be turned off and the device will effectively turn off the DSP.

> ---
>  {sound/soc/codecs => include/sound}/tas2562.h |   3 +
>  include/sound/tas25xx-dsp.h                   | 100 +++++++
>  sound/soc/codecs/Kconfig                      |   7 +
>  sound/soc/codecs/Makefile                     |   2 +
>  sound/soc/codecs/tas2562.c                    |   2 +-

are tas2562 and tas2563 (from commit subject) the same?

>  sound/soc/codecs/tas25xx-dsp.c                | 282 ++++++++++++++++++
>  6 files changed, 395 insertions(+), 1 deletion(-)
>  rename {sound/soc/codecs => include/sound}/tas2562.h (97%)
>  create mode 100644 include/sound/tas25xx-dsp.h
>  create mode 100644 sound/soc/codecs/tas25xx-dsp.c
> 
> diff --git a/sound/soc/codecs/tas2562.h b/include/sound/tas2562.h
> similarity index 97%
> rename from sound/soc/codecs/tas2562.h
> rename to include/sound/tas2562.h

> diff --git a/sound/soc/codecs/tas25xx-dsp.c b/sound/soc/codecs/tas25xx-dsp.c
> new file mode 100644
> index 000000000000..d5081fa01441
> --- /dev/null
> +++ b/sound/soc/codecs/tas25xx-dsp.c
> @@ -0,0 +1,282 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Firmware loader for the Texas Instruments TAS25XX DSP
> +// Copyright (C) 2020 Texas Instruments Inc.
> +
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/device.h>
> +#include <linux/firmware.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>

nit-pick: alphabetical order?

> +
> +#include <sound/tas2562.h>
> +#include <sound/tas25xx-dsp.h>
> +
> +
> +static void tas25xx_process_fw_delay(struct tas25xx_cmd *cmd)
> +{
> +	mdelay(cpu_to_be16(cmd->hdr.length));

is this the length of the header, or the duration of the delay?

Someone will get it wrong with this naming...

> +}
> +
> +static int tas25xx_process_fw_single(struct regmap *regmap,
> +	struct tas25xx_cmd *cmd)
> +{
> +	int ret;
> +	int num_writes = cpu_to_be16(cmd->hdr.length);
> +	struct tas25xx_cmd_reg *reg = &cmd->reg;

reverse x-mas style recommended, e.g. move 'int ret;' last

> +
> +	for (int i = 0; i < num_writes; i++, reg++) {
> +		/* Reset Page to 0 to access BOOK_CTRL */
> +		ret = regmap_write(regmap, TAS2562_PAGE_CTRL, 0);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(regmap, TAS2562_BOOK_CTRL, reg->book);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(regmap, TAS2562_REG(reg->page, reg->offset),
> +			reg->data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tas25xx_process_fw_burst(struct regmap *regmap,
> +	struct tas25xx_cmd *cmd)
> +{
> +	int ret;
> +
> +	/* Reset Page to 0 to access BOOK_CTRL */
> +	ret = regmap_write(regmap, TAS2562_PAGE_CTRL, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(regmap, TAS2562_BOOK_CTRL, cmd->reg.book);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_bulk_write(regmap, TAS2562_REG(cmd->reg.page, cmd->reg.offset), &cmd[1],
> +				cpu_to_be16(cmd->hdr.length));
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int tas25xx_process_block(struct device *dev, struct regmap *regmap,
> +	struct tas25xx_cmd *cmd, int max_block_size)
> +{
> +	int ret;
> +	int block_read;
> +
> +	const int hdr_size = sizeof(struct tas25xx_cmd_hdr);
> +	const int reg_size = sizeof(struct tas25xx_cmd_reg);
> +	const int cmd_size = sizeof(struct tas25xx_cmd);
> +
> +	switch (cpu_to_be16(cmd->hdr.cmd_type)) {
> +	case TAS25XX_CMD_SING_W:
> +		block_read = cpu_to_be16(cmd->hdr.length) * reg_size + hdr_size;
> +		break;
> +	case TAS25XX_CMD_BURST:
> +		block_read = cpu_to_be16(cmd->hdr.length) + cmd_size;
> +		break;
> +	case TAS25XX_CMD_DELAY:
> +		block_read = 4;
> +		break;
> +	default:
> +		block_read = 0;
> +	}
> +
> +	if (block_read > max_block_size) {
> +		dev_err(dev,
> +			"Corrupt firmware: block_read > max_block_size %d %d\n",
> +			block_read, max_block_size);
> +		return -EINVAL;
> +	}
> +
> +	switch (cpu_to_be16(cmd->hdr.cmd_type)) {
> +	case TAS25XX_CMD_SING_W:
> +		ret = tas25xx_process_fw_single(regmap, cmd);
> +		if (ret) {
> +			dev_err(dev, "Failed to process single write %d\n",
> +				ret);
> +			return ret;
> +		}
> +		break;
> +	case TAS25XX_CMD_BURST:
> +		ret = tas25xx_process_fw_burst(regmap, cmd);
> +		if (ret) {
> +			dev_err(dev, "Failed to process burst write %d\n", ret);
> +			return ret;
> +		}
> +		break;
> +	case TAS25XX_CMD_DELAY:
> +		tas25xx_process_fw_delay(cmd);
> +		break;
> +	default:
> +		dev_warn(dev, "Unknown cmd type %d\n",
> +			cpu_to_be16(cmd->hdr.cmd_type));
> +		break;
> +	};
> +
> +	return block_read;
> +}
> +
> +
> +int tas25xx_write_program(struct device *dev, struct regmap *regmap,
> +	struct tas25xx_fw_data *fw_data, int prog_num)
> +{
> +	int offset = 0;
> +	int length = 0;

useless init

> +	struct tas25xx_program_info *prog_info;
> +	struct tas25xx_fw_hdr *hdr = fw_data->hdr;
> +	struct tas25xx_cmd *cmd;

again reverse x-mas style would look much better

> +
> +	if (prog_num > cpu_to_be32(hdr->num_programs))
> +		return -EINVAL;
> +
> +	for (int i = 0; i < prog_num; i++)
> +		offset += cpu_to_be32(hdr->prog_size[i]);
> +
> +	prog_info = (struct tas25xx_program_info *)&fw_data->prog_data[offset];
> +	dev_info(dev, "Write program %d: %s\n", prog_num, prog_info->name);
> +
> +	int max_offset = offset + cpu_to_be32(hdr->prog_size[prog_num]);
> +	int num_subblocks = cpu_to_be32(prog_info->blk_data.num_subblocks);

It's not illegal but not consistent to declare variables in the middle
of code for no good reason.

> +	offset += sizeof(struct tas25xx_program_info);
> +
> +	for (int i = 0; i < num_subblocks; i++) {
> +		cmd = (struct tas25xx_cmd *)&fw_data->prog_data[offset];
> +		length = tas25xx_process_block(dev, regmap, cmd,
> +			max_offset - offset);
> +		if (length < 0)
> +			return length;
> +
> +		offset += length;
> +	}
> +
> +	/* Reset Book to 0 */
> +	regmap_write(regmap, TAS2562_PAGE_CTRL, 0);
> +	regmap_write(regmap, TAS2562_BOOK_CTRL, 0);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tas25xx_write_program);
> +
> +int tas25xx_write_config(struct device *dev, struct regmap *regmap,
> +	struct tas25xx_fw_data *fw_data, int config_num)
> +{
> +	int offset = 0;
> +	int length = 0;

useless init again

> +	struct tas25xx_config_info *cfg_info;
> +	struct tas25xx_fw_hdr *hdr = fw_data->hdr;
> +	struct tas25xx_cmd *cmd;
> +
> +	if (config_num > cpu_to_be32(hdr->num_configs))
> +		return -EINVAL;
> +
> +	for (int i = 0; i < config_num; i++)
> +		offset += cpu_to_be32(hdr->config_size[i]);
> +
> +	cfg_info = (struct tas25xx_config_info *)&fw_data->config_data[offset];
> +	dev_info(dev, "Write config %d: %s\n", config_num, cfg_info->name);
> +
> +	int max_offset = offset + cpu_to_be32(hdr->config_size[config_num]);
> +	int num_subblocks = cpu_to_be32(cfg_info->blk_data.num_subblocks);
> +
> +	offset += sizeof(struct tas25xx_config_info);
> +
> +	for (int i = 0; i < num_subblocks; i++) {
> +		cmd = (struct tas25xx_cmd *)&fw_data->config_data[offset];
> +		length = tas25xx_process_block(dev, regmap, cmd,
> +			max_offset - offset);
> +		if (length < 0)
> +			return length;
> +
> +		offset += length;
> +	}
> +
> +	/* Reset Book to 0 */
> +	regmap_write(regmap, TAS2562_PAGE_CTRL, 0);
> +	regmap_write(regmap, TAS2562_BOOK_CTRL, 0);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tas25xx_write_config);
> +
> +
> +struct tas25xx_fw_data *tas25xx_parse_fw(struct device *dev,
> +	const struct firmware *fw)
> +{
> +	u32 total_prog_sz = 0;
> +	u32 total_config_sz = 0;
> +	u32 prog_num = 0;
> +	u32 config_num = 0;

last two inits are useless

> +	int hdr_size = sizeof(struct tas25xx_fw_hdr);
> +	struct tas25xx_fw_data *fw_data = NULL;
> +
> +	fw_data = devm_kzalloc(dev, sizeof(struct tas25xx_fw_data), GFP_KERNEL);
> +	if (!fw_data)
> +		goto err_fw;
> +
> +	if (fw->size < hdr_size)
> +		goto err_data;
> +
> +	fw_data->hdr = devm_kzalloc(dev, hdr_size, GFP_KERNEL);
> +	if (!fw_data->hdr)
> +		goto err_data;
> +
> +	memcpy(fw_data->hdr, &fw->data[0], hdr_size);
> +
> +	for (int i = 0; i < cpu_to_be32(fw_data->hdr->num_programs); i++)
> +		total_prog_sz += cpu_to_be32(fw_data->hdr->prog_size[i]);
> +
> +	for (int i = 0; i < cpu_to_be32(fw_data->hdr->num_configs); i++)
> +		total_config_sz += cpu_to_be32(fw_data->hdr->config_size[i]);
> +
> +	if (fw->size < hdr_size + total_prog_sz + total_config_sz)
> +		goto err_hdr;
> +
> +	fw_data->prog_data = devm_kzalloc(dev, total_prog_sz, GFP_KERNEL);
> +	if (!fw_data->prog_data)
> +		goto err_hdr;
> +
> +	memcpy(fw_data->prog_data, &fw->data[hdr_size], total_prog_sz);
> +
> +	fw_data->config_data = devm_kzalloc(dev, total_config_sz, GFP_KERNEL);
> +	if (!fw_data->config_data)
> +		goto err_prog;
> +
> +	memcpy(fw_data->config_data, &fw->data[hdr_size + total_prog_sz],
> +		total_config_sz);
> +
> +	prog_num = cpu_to_be32(fw_data->hdr->num_programs);
> +	config_num = cpu_to_be32(fw_data->hdr->num_configs);
> +	dev_info(dev, "Firmware loaded: programs %d, configs %d\n",
> +		prog_num, config_num);
> +
> +	return fw_data;
> +
> +err_prog:
> +	devm_kfree(dev, fw_data->prog_data);
> +err_hdr:
> +	devm_kfree(dev, fw_data->hdr);
> +err_data:
> +	devm_kfree(dev, fw_data);
> +err_fw:
> +	release_firmware(fw);
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(tas25xx_parse_fw);
> +
> +MODULE_DESCRIPTION("TAS25xx DSP library");
> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
> +MODULE_LICENSE("GPL");

Hi Pierre-Louis,

Thank you for your review.

On Mon, 2023-12-04 at 18:05 -0600, Pierre-Louis Bossart wrote:

> > 
> >  sound/soc/codecs/tas2562.c                    |   2 +-
> 
> are tas2562 and tas2563 (from commit subject) the same?
> 
No, tas2563 is register compatible with tas2562.

> > 
> > +#include <linux/slab.h>
> > +#include <linux/delay.h>
> 
> nit-pick: alphabetical order?
> 
Good idea, I'll fix it and the others in the next version.

> > +
> > +#include <sound/tas2562.h>
> > +#include <sound/tas25xx-dsp.h>
> > +
> > +
> > +static void tas25xx_process_fw_delay(struct tas25xx_cmd *cmd)
> > +{
> > +	mdelay(cpu_to_be16(cmd->hdr.length));
> 
> is this the length of the header, or the duration of the delay?
> 
> Someone will get it wrong with this naming...
> 
I think this is because of the format.
If cmd_type is TAS25XX_CMD_DELAY, then hdr.length is the length of the
delay.
At least I think so, based on
https://lore.kernel.org/lkml/6d6aaed3-dac8-e1ec-436c-9b04273df2b3@ti.com/T/
https://github.com/torvalds/linux/blob/bee0e7762ad2c6025b9f5245c040fcc36ef2bde8/sound/soc/codecs/tas2781-fmwlib.c#L769

But I don't see any TAS25XX_CMD_DELAY command in the firmware I'm
using.

> > 
> > +	prog_num = cpu_to_be32(fw_data->hdr->num_programs);
> > +	config_num = cpu_to_be32(fw_data->hdr->num_configs);
> > +	dev_info(dev, "Firmware loaded: programs %d, configs %d\n",
> > +		prog_num, config_num);
> > +

For me:
release_firmware(fw) is missing from here

> > +	return fw_data;
> > +
> > +err_prog:
> > +	devm_kfree(dev, fw_data->prog_data);
> > +err_hdr:
> > +	devm_kfree(dev, fw_data->hdr);
> > +err_data:
> > +	devm_kfree(dev, fw_data);
> > +err_fw:
> > +	release_firmware(fw);
> > +
> > +	return NULL;
> > +}
> > 

Regards,

Gergo