[RFC PATCH 0/2] TAS2563 DSP Firmware Loader
Hello
The TAS2563 amplifier has a DSP that can run programs and configurations to produce alternate audio experiences. The DSP firmware is not a typical firmware as the binary may contain various programs and configurations that are selectable during run time.
These programs and configurations are selectable via files under the I2C dev node. There may be a better way to select this through ALSA controls but I was unable to find a good example of this. This is why this is an RFC patchset.
Dan
Dan Murphy (2): dt-bindings: tas2562: Add firmware support for tas2563 ASoc: tas2563: DSP Firmware loading support
.../devicetree/bindings/sound/tas2562.yaml | 4 + sound/soc/codecs/Makefile | 2 +- sound/soc/codecs/tas2562.c | 48 ++- sound/soc/codecs/tas2562.h | 25 ++ sound/soc/codecs/tas25xx_dsp_loader.c | 377 ++++++++++++++++++ sound/soc/codecs/tas25xx_dsp_loader.h | 93 +++++ 6 files changed, 530 insertions(+), 19 deletions(-) create mode 100644 sound/soc/codecs/tas25xx_dsp_loader.c create mode 100644 sound/soc/codecs/tas25xx_dsp_loader.h
Add a property called firmware-name that will be the name of the firmware that will reside in the file system or built into the kernel.
Signed-off-by: Dan Murphy dmurphy@ti.com --- Documentation/devicetree/bindings/sound/tas2562.yaml | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/tas2562.yaml b/Documentation/devicetree/bindings/sound/tas2562.yaml index c6168aa32954..874f91f32d7f 100644 --- a/Documentation/devicetree/bindings/sound/tas2562.yaml +++ b/Documentation/devicetree/bindings/sound/tas2562.yaml @@ -40,6 +40,10 @@ properties: '#sound-dai-cells': const: 1
+ firmware-name: + $ref: /schemas/types.yaml#/definitions/string + description: Name of the firmware to be loaded to the DSP. TAS2563 only. + required: - compatible - reg
On Tue, Jun 09, 2020 at 12:28:40PM -0500, Dan Murphy wrote:
Add a property called firmware-name that will be the name of the firmware that will reside in the file system or built into the kernel.
Why not just use a standard name for the firmware? If the firmwares vary per-board then building it using the machine compatible (or DMI info) could handle that, with a fallback to a standard name for a default setup.
Mark
On 6/9/20 12:31 PM, Mark Brown wrote:
On Tue, Jun 09, 2020 at 12:28:40PM -0500, Dan Murphy wrote:
Add a property called firmware-name that will be the name of the firmware that will reside in the file system or built into the kernel.
Why not just use a standard name for the firmware? If the firmwares vary per-board then building it using the machine compatible (or DMI info) could handle that, with a fallback to a standard name for a default setup.
The number of firmwares can vary per IC on the board itself. So you may have X number of firmware files all with different names all targets for different TAS2563 ICs.
Also TI will not be providing the individual binaries to the customer. There is a customer tool that the user uses to create the binaries.
So the output names are arbitrary.
I was going to mention this in the cover letter but did not think mentioning the user tool had any value
Dan
On Tue, Jun 09, 2020 at 12:35:50PM -0500, Dan Murphy wrote:
On 6/9/20 12:31 PM, Mark Brown wrote:
Why not just use a standard name for the firmware? If the firmwares vary per-board then building it using the machine compatible (or DMI info) could handle that, with a fallback to a standard name for a default setup.
The number of firmwares can vary per IC on the board itself. So you may have X number of firmware files all with different names all targets for different TAS2563 ICs.
Also TI will not be providing the individual binaries to the customer. There is a customer tool that the user uses to create the binaries.
So the output names are arbitrary.
I was going to mention this in the cover letter but did not think mentioning the user tool had any value
That's all fairly standard for this sort of device. You could still cope with this by including the I2C address in the default name requested - do something like tas2562/myboard-addr.fw or whatever. The concern here is that someone shouldn't have to replace their DT if they decide they want to start using the DSP, and someone making a distro shouldn't be stuck dealing with what happens if multiple vendors decide to just reuse the same name (eg, just calling everything tas2562 regardless of plastics).
Mark
On 6/9/20 12:58 PM, Mark Brown wrote:
On Tue, Jun 09, 2020 at 12:35:50PM -0500, Dan Murphy wrote:
On 6/9/20 12:31 PM, Mark Brown wrote:
Why not just use a standard name for the firmware? If the firmwares vary per-board then building it using the machine compatible (or DMI info) could handle that, with a fallback to a standard name for a default setup.
The number of firmwares can vary per IC on the board itself. So you may have X number of firmware files all with different names all targets for different TAS2563 ICs. Also TI will not be providing the individual binaries to the customer. There is a customer tool that the user uses to create the binaries. So the output names are arbitrary. I was going to mention this in the cover letter but did not think mentioning the user tool had any value
That's all fairly standard for this sort of device. You could still cope with this by including the I2C address in the default name requested - do something like tas2562/myboard-addr.fw or whatever. The concern here is that someone shouldn't have to replace their DT if they decide they want to start using the DSP, and someone making a distro shouldn't be stuck dealing with what happens if multiple vendors decide to just reuse the same name (eg, just calling everything tas2562 regardless of plastics).
I could make a default as you suggested to include i2c address and bus in the name. But the TAS2563 does not need the firmware to operate and the 2562 does not have a DSP.
What if there was an ALSA control instead that passed in the firmware name from the user space instead of using the DT?
Then the control can load and parse the firmware and wait for the user to select the program.
This would solve a user from having ot update the DT to use a firmware.
Dan
On Tue, Jun 09, 2020 at 01:06:50PM -0500, Dan Murphy wrote:
I could make a default as you suggested to include i2c address and bus in the name. But the TAS2563 does not need the firmware to operate and the 2562 does not have a DSP.
That's fine, the driver can just use the compatible string to check this and not offer any of the DSP related stuff (it should do this regardless of the method used here). I'm guessing the regmap configs should also be different.
What if there was an ALSA control instead that passed in the firmware name from the user space instead of using the DT?
Then the control can load and parse the firmware and wait for the user to select the program.
This would solve a user from having ot update the DT to use a firmware.
That's really not very idiomatic for how Linux does stuff and seems to pretty much guarantee issues with hotplugging controls and ordering - you'd need special userspace to start up even if it was just a really simple DSP config doing only speaker correction or something. I'm not sure what the advantage would be - what problem is this solving over static names?
Mark
On 6/9/20 1:47 PM, Mark Brown wrote:
On Tue, Jun 09, 2020 at 01:06:50PM -0500, Dan Murphy wrote:
I could make a default as you suggested to include i2c address and bus in the name. But the TAS2563 does not need the firmware to operate and the 2562 does not have a DSP.
That's fine, the driver can just use the compatible string to check this and not offer any of the DSP related stuff (it should do this regardless of the method used here). I'm guessing the regmap configs should also be different.
The driver does check the compatible to determine if DSP loading is available for the device.
The driver also checks to see if the firmware file is declared in the DT.
So it has to pass 2 checks to even load and parse the firmware to present the controls for the programs and configs.
What if there was an ALSA control instead that passed in the firmware name from the user space instead of using the DT? Then the control can load and parse the firmware and wait for the user to select the program. This would solve a user from having ot update the DT to use a firmware.
That's really not very idiomatic for how Linux does stuff and seems to pretty much guarantee issues with hotplugging controls and ordering - you'd need special userspace to start up even if it was just a really simple DSP config doing only speaker correction or something. I'm not sure what the advantage would be - what problem is this solving over static names?
IMO having a static name is the problem. It is an inflexible design. Besides the firmware-name property seems to be used in other drivers to declare firmwares for the boards.
But if no one is complaining or submitting patches within the codecs to be more flexible with firmware then I can just hard code the name like other drivers do.
Dan
On Tue, Jun 09, 2020 at 02:20:29PM -0500, Dan Murphy wrote:
On 6/9/20 1:47 PM, Mark Brown wrote:
That's really not very idiomatic for how Linux does stuff and seems to pretty much guarantee issues with hotplugging controls and ordering - you'd need special userspace to start up even if it was just a really simple DSP config doing only speaker correction or something. I'm not sure what the advantage would be - what problem is this solving over static names?
IMO having a static name is the problem. It is an inflexible design. Besides the firmware-name property seems to be used in other drivers to declare firmwares for the boards.
But if no one is complaining or submitting patches within the codecs to be more flexible with firmware then I can just hard code the name like other drivers do.
I'm not *completely* opposed to having the ability to suggest a name in firmware, the big problem is making use of the DSP completely dependent on having a DT property or doing some non-standard dance in userspace.
Mark
On 6/10/20 5:29 AM, Mark Brown wrote:
On Tue, Jun 09, 2020 at 02:20:29PM -0500, Dan Murphy wrote:
On 6/9/20 1:47 PM, Mark Brown wrote:
That's really not very idiomatic for how Linux does stuff and seems to pretty much guarantee issues with hotplugging controls and ordering - you'd need special userspace to start up even if it was just a really simple DSP config doing only speaker correction or something. I'm not sure what the advantage would be - what problem is this solving over static names?
IMO having a static name is the problem. It is an inflexible design. Besides the firmware-name property seems to be used in other drivers to declare firmwares for the boards. But if no one is complaining or submitting patches within the codecs to be more flexible with firmware then I can just hard code the name like other drivers do.
I'm not *completely* opposed to having the ability to suggest a name in firmware, the big problem is making use of the DSP completely dependent on having a DT property or doing some non-standard dance in userspace.
Well from what I see we have 4 options.
1. We can have a DT node like RFC'd (Need Rob's comments here)
2. We can have a defconfig flag that hard codes the name (This will probably be met with some resistance if not some really bad reactions and I don't prefer to do it this way)
3. We can hard code the name of the firmware in the c file.
4. Dynamically derive a file name based on the I2C bus-address-device so it would be expected to be "2_4c_tas2563.bin". Just need to figure out how to get the bus number.
I don't see the user space as a viable option because the codec will have to load and then the user space would have to request to load the firmware and then more mixer controls will appear.
Again only option 1 allows us to have different firmware binaries per IC instance and also denotes the use of the DSP. The DSP is not programmed until the user space selects the program or configuration from the binary. So special audio handling is very explicit in the user space. More then likely most standard distributions will not even use the DSP for this device it is more of a specialized use case for each product.
On Wed, Jun 10, 2020 at 09:12:15AM -0500, Dan Murphy wrote:
On 6/10/20 5:29 AM, Mark Brown wrote:
I'm not *completely* opposed to having the ability to suggest a name in firmware, the big problem is making use of the DSP completely dependent on having a DT property or doing some non-standard dance in userspace.
Well from what I see we have 4 options.
These are not mutually exclusive approaches.
1. We can have a DT node like RFC'd (Need Rob's comments here)
This is compatible with any hardcoding option.
2. We can have a defconfig flag that hard codes the name (This will probably be met with some resistance if not some really bad reactions and I don't prefer to do it this way)
This is even worse than the ALSA control suggestion.
3. We can hard code the name of the firmware in the c file.
4. Dynamically derive a file name based on the I2C bus-address-device so it would be expected to be "2_4c_tas2563.bin". Just need to figure out how to get the bus number.
Again only option 1 allows us to have different firmware binaries per IC instance and also denotes the use of the DSP. The DSP is not programmed
No, this is not the case at all - a per-device generated file allows this just as well.
So special audio handling is very explicit in the user space. More then likely most standard distributions will not even use the DSP for this device it is more of a specialized use case for each product.
People do things like make AOSP derived distributions for phones.
On Wed, Jun 10, 2020 at 09:12:15AM -0500, Dan Murphy wrote:
Mark
On 6/10/20 5:29 AM, Mark Brown wrote:
On Tue, Jun 09, 2020 at 02:20:29PM -0500, Dan Murphy wrote:
On 6/9/20 1:47 PM, Mark Brown wrote:
That's really not very idiomatic for how Linux does stuff and seems to pretty much guarantee issues with hotplugging controls and ordering - you'd need special userspace to start up even if it was just a really simple DSP config doing only speaker correction or something. I'm not sure what the advantage would be - what problem is this solving over static names?
IMO having a static name is the problem. It is an inflexible design. Besides the firmware-name property seems to be used in other drivers to declare firmwares for the boards. But if no one is complaining or submitting patches within the codecs to be more flexible with firmware then I can just hard code the name like other drivers do.
I'm not *completely* opposed to having the ability to suggest a name in firmware, the big problem is making use of the DSP completely dependent on having a DT property or doing some non-standard dance in userspace.
Well from what I see we have 4 options.
1. We can have a DT node like RFC'd (Need Rob's comments here)
We've obviously already allowed 'firmware-name', but the preference is still not putting into DT. It's really a Linux userspace interface.
2. We can have a defconfig flag that hard codes the name (This will probably be met with some resistance if not some really bad reactions and I don't prefer to do it this way)
3. We can hard code the name of the firmware in the c file.
4. Dynamically derive a file name based on the I2C bus-address-device so it would be expected to be "2_4c_tas2563.bin". Just need to figure out how to get the bus number.
Given bus numbering may not be constant, that seems like not the best way to match up devices. I'd assume that userspace needs some way to identify which instance is which already, so maybe there's other data you can use already.
I don't see the user space as a viable option because the codec will have to load and then the user space would have to request to load the firmware and then more mixer controls will appear.
Again only option 1 allows us to have different firmware binaries per IC instance and also denotes the use of the DSP. The DSP is not programmed until the user space selects the program or configuration from the binary. So special audio handling is very explicit in the user space. More then likely most standard distributions will not even use the DSP for this device it is more of a specialized use case for each product.
On Wed, Jun 17, 2020 at 04:04:59PM -0600, Rob Herring wrote:
Given bus numbering may not be constant, that seems like not the best way to match up devices. I'd assume that userspace needs some way to identify which instance is which already, so maybe there's other data you can use already.
There isn't really, you're putting stuff in the DT for that - usually as part of the card binding. I guess we could use that string rather than the dev_name().
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.
Signed-off-by: Dan Murphy dmurphy@ti.com --- sound/soc/codecs/Makefile | 2 +- sound/soc/codecs/tas2562.c | 48 ++-- sound/soc/codecs/tas2562.h | 25 ++ sound/soc/codecs/tas25xx_dsp_loader.c | 377 ++++++++++++++++++++++++++ sound/soc/codecs/tas25xx_dsp_loader.h | 93 +++++++ 5 files changed, 526 insertions(+), 19 deletions(-) create mode 100644 sound/soc/codecs/tas25xx_dsp_loader.c create mode 100644 sound/soc/codecs/tas25xx_dsp_loader.h
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index 47ae3cebb61e..c7a4e567b623 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -298,7 +298,7 @@ snd-soc-max98504-objs := max98504.o snd-soc-simple-amplifier-objs := simple-amplifier.o snd-soc-tpa6130a2-objs := tpa6130a2.o snd-soc-tas2552-objs := tas2552.o -snd-soc-tas2562-objs := tas2562.o +snd-soc-tas2562-objs := tas2562.o tas25xx_dsp_loader.o
obj-$(CONFIG_SND_SOC_88PM860X) += snd-soc-88pm860x.o obj-$(CONFIG_SND_SOC_AB8500_CODEC) += snd-soc-ab8500-codec.o diff --git a/sound/soc/codecs/tas2562.c b/sound/soc/codecs/tas2562.c index 7fae88655a0f..3e60fdec4cfd 100644 --- a/sound/soc/codecs/tas2562.c +++ b/sound/soc/codecs/tas2562.c @@ -7,6 +7,7 @@ #include <linux/module.h> #include <linux/errno.h> #include <linux/device.h> +#include <linux/firmware.h> #include <linux/i2c.h> #include <linux/pm_runtime.h> #include <linux/regmap.h> @@ -22,6 +23,7 @@ #include <sound/tlv.h>
#include "tas2562.h" +#include "tas25xx_dsp_loader.h"
#define TAS2562_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE |\ SNDRV_PCM_FORMAT_S32_LE) @@ -44,22 +46,6 @@ static const unsigned int float_vol_db_lookup[] = { 0x197a967f, 0x2013739e, 0x28619ae9, 0x32d64617, 0x40000000 };
-struct tas2562_data { - struct snd_soc_component *component; - struct gpio_desc *sdz_gpio; - struct regmap *regmap; - struct device *dev; - struct i2c_client *client; - int v_sense_slot; - int i_sense_slot; - int volume_lvl; -}; - -enum tas256x_model { - TAS2562, - TAS2563, -}; - static int tas2562_set_bias_level(struct snd_soc_component *component, enum snd_soc_bias_level level) { @@ -343,6 +329,17 @@ static int tas2562_mute(struct snd_soc_dai *dai, int mute) mute ? TAS2562_MUTE : 0); }
+static void tas2562_fw_loaded(const struct firmware *fw, void *context) +{ + struct snd_soc_component *component = context; + struct tas2562_data *tas2562 = snd_soc_component_get_drvdata(component); + int ret; + + ret = tas25xx_init_fw(tas2562, fw); + if (ret) + dev_err(tas2562->dev, "Firmware failed to initialize\n"); +} + static int tas2562_codec_probe(struct snd_soc_component *component) { struct tas2562_data *tas2562 = snd_soc_component_get_drvdata(component); @@ -358,6 +355,12 @@ static int tas2562_codec_probe(struct snd_soc_component *component) if (ret < 0) return ret;
+ if (tas2562->load_firmware == 0 && tas2562->model_id == TAS2563) + request_firmware_nowait(THIS_MODULE, FW_ACTION_HOTPLUG, + tas2562->firmware_name, component->dev, + GFP_KERNEL, component, + tas2562_fw_loaded); + return 0; }
@@ -580,7 +583,7 @@ static struct snd_soc_dai_driver tas2562_dai[] = { static const struct regmap_range_cfg tas2562_ranges[] = { { .range_min = 0, - .range_max = 5 * 128, + .range_max = 255 * 128, .selector_reg = TAS2562_PAGE_CTRL, .selector_mask = 0xff, .selector_shift = 0, @@ -606,7 +609,7 @@ static const struct regmap_config tas2562_regmap_config = { .reg_bits = 8, .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), @@ -634,6 +637,14 @@ static int tas2562_parse_dt(struct tas2562_data *tas2562) dev_err(dev, "Looking up %s property failed %d\n", "ti,imon-slot-no", ret);
+ if (tas2562->model_id != TAS2562) { + tas2562->load_firmware = fwnode_property_read_string(dev->fwnode, + "firmware-name", + &tas2562->firmware_name); + if (tas2562->load_firmware) + dev_info(dev, "No firmware file to request\n"); + } + return ret; }
@@ -650,6 +661,7 @@ static int tas2562_probe(struct i2c_client *client,
data->client = client; data->dev = &client->dev; + data->model_id = id->driver_data;
tas2562_parse_dt(data);
diff --git a/sound/soc/codecs/tas2562.h b/sound/soc/codecs/tas2562.h index 28e75fc431d0..66c15b05cd35 100644 --- a/sound/soc/codecs/tas2562.h +++ b/sound/soc/codecs/tas2562.h @@ -11,6 +11,7 @@ #define __TAS2562_H__
#define TAS2562_PAGE_CTRL 0x00 +#define TAS2562_BOOK_CTRL 0x7f
#define TAS2562_REG(page, reg) ((page * 128) + reg)
@@ -40,6 +41,8 @@ #define TAS2562_DVC_CFG3 TAS2562_REG(2, 0x0e) #define TAS2562_DVC_CFG4 TAS2562_REG(2, 0x0f)
+#define TAS25XX_DSP_MODE TAS2562_REG(1, 2) + #define TAS2562_RESET BIT(0)
#define TAS2562_MODE_MASK GENMASK(1,0) @@ -84,4 +87,26 @@ #define TAS2562_TDM_CFG6_ISNS_EN BIT(6) #define TAS2562_TDM_CFG6_ISNS_SLOT_MASK GENMASK(5, 0)
+#define TAS2563_FW_HDR_OFFSET 134 + +struct tas2562_data { + struct snd_soc_component *component; + struct gpio_desc *sdz_gpio; + struct regmap *regmap; + struct device *dev; + struct i2c_client *client; + struct tas25xx_fw_data *fw_data; + const char *firmware_name; + int load_firmware; + int model_id; + int v_sense_slot; + int i_sense_slot; + int volume_lvl; +}; + +enum tas2562_id { + TAS2562, + TAS2563, +}; + #endif /* __TAS2562_H__ */ diff --git a/sound/soc/codecs/tas25xx_dsp_loader.c b/sound/soc/codecs/tas25xx_dsp_loader.c new file mode 100644 index 000000000000..45cc5a253898 --- /dev/null +++ b/sound/soc/codecs/tas25xx_dsp_loader.c @@ -0,0 +1,377 @@ +// 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> + +#include "tas25xx_dsp_loader.h" + +static void tas25xx_process_fw_delay(struct tas2562_data *tas2562, u8 *fw_out) +{ + u16 fw_delay = fw_out[2] << 8 | fw_out[3]; + + mdelay(fw_delay); +} + +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; + + 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; + + write_reg = TAS2562_REG(cmd_data->page, cmd_data->offset); + ret = regmap_write(tas2562->regmap, write_reg, reg_data); + if (ret) + return ret; + } + + return 0; +} + +static int tas25xx_process_fw_burst(struct tas2562_data *tas2562, + struct tas25xx_cmd_data *cmd_data, + u8 *fw_out, int size) +{ + int hdr_size = sizeof(struct tas25xx_cmd_data); + u8 *data = &fw_out[hdr_size]; + int ret; + int reg_data = TAS2562_REG(cmd_data->page, cmd_data->offset); + + /* Reset Page to 0 */ + ret = regmap_write(tas2562->regmap, TAS2562_PAGE_CTRL, 0); + if (ret) + return ret; + + ret = regmap_write(tas2562->regmap, TAS2562_BOOK_CTRL, cmd_data->book); + if (ret) + return ret; + + ret = regmap_bulk_write(tas2562->regmap, reg_data, data, + cpu_to_be16(cmd_data->length)); + if (ret) + return ret; + + return 0; +} + +static int tas25xx_process_block(struct tas2562_data *tas2562, u8 *data) +{ + struct tas25xx_cmd_data *cmd_data = (struct tas25xx_cmd_data *)data; + int ret; + int total_written; + int hdr_size = sizeof(struct tas25xx_cmd_data); + + switch (cpu_to_be16(cmd_data->cmd_type)) { + case TAS25XX_CMD_SING_W: + ret = tas25xx_process_fw_single(tas2562, cmd_data, data); + if (ret) { + dev_err(tas2562->dev, + "Failed to process single write %d\n", ret); + return ret; + } + + hdr_size -= 4; + total_written = cpu_to_be16(cmd_data->length) * 4 + hdr_size; + break; + case TAS25XX_CMD_BURST: + ret = tas25xx_process_fw_burst(tas2562, cmd_data, data, + cpu_to_be16(cmd_data->length)); + if (ret) { + dev_err(tas2562->dev, + "Failed to process burst write %d\n", ret); + return ret; + } + total_written = cpu_to_be16(cmd_data->length) + hdr_size; + break; + case TAS25XX_CMD_DELAY: + tas25xx_process_fw_delay(tas2562, data); + total_written = 4; + break; + default: + total_written = 0; + break; + }; + + return total_written; +} + + +static int tas25xx_write_program(struct tas2562_data *tas2562, int prog_num) +{ + struct tas25xx_fw_hdr *fw_hdr = tas2562->fw_data->fw_hdr; + struct tas25xx_program_info *prog_info; + int offset = sizeof(struct tas25xx_program_info); + int prog_offset = 0; + int i; + int length = 0; + + if (prog_num > fw_hdr->num_programs) + return -EINVAL; + + if (prog_num) { + for (i = 0; i < prog_num; i++) + prog_offset += cpu_to_be32(fw_hdr->prog_size[i]); + } + + offset += prog_offset; + prog_info = (struct tas25xx_program_info *)&tas2562->fw_data->prog_info[prog_offset]; + + for (i = 0; i < cpu_to_be32(prog_info->blk_data.num_subblocks); i++) { + length = tas25xx_process_block(tas2562, + &tas2562->fw_data->prog_info[offset]); + if (length < 0) + return length; + + offset += length; + } + + /* Reset Book to 0 */ + regmap_write(tas2562->regmap, TAS2562_BOOK_CTRL, 0); + + return 0; +} + +static int tas25xx_write_config(struct tas2562_data *tas2562, int config_num) +{ + struct tas25xx_fw_hdr *fw_hdr = tas2562->fw_data->fw_hdr; + struct tas25xx_config_info *cfg_info; + int config_offset = 0; + int i; + int offset = sizeof(struct tas25xx_config_info); + int length = 0; + + if (config_num > fw_hdr->num_configs) + return -EINVAL; + + if (config_num) + for (i = 0; i < config_num; i++) + config_offset += cpu_to_be32(fw_hdr->config_size[i]); + + cfg_info = (struct tas25xx_config_info *)&tas2562->fw_data->config_info[config_offset]; + offset += config_offset; + + for (i = 0; i < cpu_to_be32(cfg_info->blk_data.num_subblocks); i++) { + length = tas25xx_process_block(tas2562, + &tas2562->fw_data->config_info[offset]); + if (length < 0) + return length; + + offset += length; + } + + /* Reset Book to 0 */ + regmap_write(tas2562->regmap, TAS2562_BOOK_CTRL, 0); + + return 0; +} + +static ssize_t write_config_store(struct device *dev, + struct device_attribute *tas25xx_attr, + const char *buf, size_t size) +{ + struct tas2562_data *tas25xx = dev_get_drvdata(dev); + struct tas25xx_fw_hdr *fw_hdr = tas25xx->fw_data->fw_hdr; + long configs; + int ret; + + ret = kstrtol(buf, 10, &configs); + if (ret != 0) + return ret; + + if (configs < 1 || configs > cpu_to_be32(fw_hdr->num_configs)) + return -EINVAL; + + configs--; + ret = tas25xx_write_config(tas25xx, configs); + if (!ret) + ret = size; + + return ret; +} +static DEVICE_ATTR_WO(write_config); + +static ssize_t write_program_store(struct device *dev, + struct device_attribute *tas25xx_attr, + const char *buf, size_t size) +{ + struct tas2562_data *tas25xx = dev_get_drvdata(dev); + struct tas25xx_fw_hdr *fw_hdr = tas25xx->fw_data->fw_hdr; + long program; + int ret; + + ret = kstrtol(buf, 10, &program); + if (ret != 0) + return ret; + + if (program < 1 || program > cpu_to_be32(fw_hdr->num_programs)) + return -EINVAL; + + program--; + ret = tas25xx_write_program(tas25xx, program); + if (!ret) + ret = size; + + return ret; +} +static DEVICE_ATTR_WO(write_program); + +static ssize_t enable_program_show(struct device *dev, + struct device_attribute *tas25xx_attr, + char *buf) +{ + struct tas2562_data *tas25xx = dev_get_drvdata(dev); + int mode; + + regmap_read(tas25xx->regmap, TAS25XX_DSP_MODE, &mode); + + return sprintf(buf, "0x%X\n", mode); +} + +static ssize_t enable_program_store(struct device *dev, + struct device_attribute *tas25xx_attr, + const char *buf, size_t size) +{ + struct tas2562_data *tas25xx = dev_get_drvdata(dev); + long mode; + int ret; + + ret = kstrtol(buf, 10, &mode); + if (ret != 0) + return ret; + + if (mode) + ret = regmap_write(tas25xx->regmap, TAS25XX_DSP_MODE, mode); + else + ret = regmap_write(tas25xx->regmap, TAS25XX_DSP_MODE, 0); + + if (!ret) + ret = size; + + return ret; + +} +static DEVICE_ATTR_RW(enable_program); + +static ssize_t num_configs_show(struct device *dev, + struct device_attribute *tas25xx_attr, + char *buf) +{ + struct tas2562_data *tas25xx = dev_get_drvdata(dev); + struct tas25xx_fw_hdr *fw_hdr = tas25xx->fw_data->fw_hdr; + + return sprintf(buf, "%d\n", cpu_to_be32(fw_hdr->num_configs)); +} +static DEVICE_ATTR_RO(num_configs); + +static ssize_t num_programs_show(struct device *dev, + struct device_attribute *tas25xx_attr, + char *buf) +{ + struct tas2562_data *tas25xx = dev_get_drvdata(dev); + struct tas25xx_fw_hdr *fw_hdr = tas25xx->fw_data->fw_hdr; + + return sprintf(buf, "%d\n", cpu_to_be32(fw_hdr->num_programs)); +} +static DEVICE_ATTR_RO(num_programs); + +static struct attribute *tas25xx_fw_attrs[] = { + &dev_attr_num_programs.attr, + &dev_attr_num_configs.attr, + &dev_attr_write_config.attr, + &dev_attr_write_program.attr, + &dev_attr_enable_program.attr, + NULL, +}; + +static const struct attribute_group tas25xx_fw_attr_group = { + .attrs = tas25xx_fw_attrs, +}; + +int tas25xx_init_fw(struct tas2562_data *tas2562, const struct firmware *fw) +{ + u32 total_prog_sz = 0; + u32 total_config_sz = 0; + int hdr_size = sizeof(struct tas25xx_fw_hdr); + int i; + u8 *out; + int ret; + + if (!fw->size) + return -ENODATA; + + out = devm_kzalloc(tas2562->dev, fw->size, GFP_KERNEL); + if (!out) + return -ENOMEM; + + memcpy(out, &fw->data[0], fw->size); + tas2562->fw_data = (struct tas25xx_fw_data *)out; + + 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); + + for (i = 0; i < cpu_to_be32(tas2562->fw_data->fw_hdr->num_programs); i++) + total_prog_sz += cpu_to_be32(tas2562->fw_data->fw_hdr->prog_size[i]); + + for (i = 0; i < cpu_to_be32(tas2562->fw_data->fw_hdr->num_configs); i++) + total_config_sz += cpu_to_be32(tas2562->fw_data->fw_hdr->config_size[i]); + + tas2562->fw_data->prog_info = devm_kzalloc(tas2562->dev, total_prog_sz, + GFP_KERNEL); + if (!tas2562->fw_data->prog_info) + return -ENOMEM; + + memcpy(tas2562->fw_data->prog_info, &fw->data[hdr_size], total_prog_sz); + + tas2562->fw_data->config_info = devm_kzalloc(tas2562->dev, + total_config_sz, + GFP_KERNEL); + if (!tas2562->fw_data->config_info) + return -ENOMEM; + + hdr_size += total_prog_sz; + memcpy(tas2562->fw_data->config_info, &fw->data[hdr_size], + total_config_sz); + + /* Create SysFS files */ + ret = device_add_group(tas2562->dev, &tas25xx_fw_attr_group); + if (ret) + dev_info(tas2562->dev, "error creating sysfs files\n"); + + release_firmware(fw); + + return 0; +} + diff --git a/sound/soc/codecs/tas25xx_dsp_loader.h b/sound/soc/codecs/tas25xx_dsp_loader.h new file mode 100644 index 000000000000..01d002da928f --- /dev/null +++ b/sound/soc/codecs/tas25xx_dsp_loader.h @@ -0,0 +1,93 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * tas25xx_dsp_loader.h - Texas Instruments TAS25xx Mono Audio Amplifier + * + * Copyright (C) 2020 Texas Instruments Incorporated - http://www.ti.com + * + * Author: Dan Murphy dmurphy@ti.com + */ + +#ifndef _TAS25XX_DSP_LOADER_H +#define _TAS25XX_DSP_LOADER_H + +#include "tas2562.h" + +#define TAS25XX_NAME_SIZE 64 +#define TAS25XX_PROG_SIZE 5 +#define TAS25XX_CONFIG_SIZE 10 + +#define TAS25XX_SINGLE_COMMAND 0x7f +#define TAS25XX_OFFSET_SHIFT 3 + +#define TAS25XX_BLK_HDR_SZ 4 + +#define TAS25XX_CMD_SING_W 0x1 +#define TAS25XX_CMD_BURST 0x2 +#define TAS25XX_CMD_DELAY 0x3 + +struct tas25xx_cmd_data { + u16 cmd_type; + u16 length; + u8 book; + u8 page; + u8 offset; +}; + +struct tas25xx_block_data { + u32 block_type; + u16 pram_checksum; + u16 yram_checksum; + u32 block_size; + u32 num_subblocks; +}; + +struct tas25xx_program_info { + char name[TAS25XX_NAME_SIZE]; + u8 app_mode; + u8 pdm_i2s_mode; + u8 isns_pd; + u8 vsns_pd; + u8 reserved_1; + u16 reserved_2; + u8 ldg_power_up; + struct tas25xx_block_data blk_data; +}; + +struct tas25xx_config_info { + char name[TAS25XX_NAME_SIZE]; + u16 devices; + u16 program; + u32 samp_rate; + u16 clk_src; + u16 sbclk_fs_ratio; + u32 clk_freq; + u32 num_blocks; + struct tas25xx_block_data blk_data; +}; + +struct tas25xx_fw_hdr { + u32 magic_num; + u32 image_size; + u32 checksum; + u32 version_num; + u32 dsp_version; + u32 drv_fw_version; + u32 timestamp; + char firmware_name[TAS25XX_NAME_SIZE]; + u16 dev_family; + u16 dev_subfamily; + u32 num_programs; + u32 prog_size[TAS25XX_PROG_SIZE]; + u32 num_configs; + u32 config_size[TAS25XX_CONFIG_SIZE]; +}; + +struct tas25xx_fw_data { + struct tas25xx_fw_hdr *fw_hdr; + u8 *config_info; + u8 *prog_info; +}; + +int tas25xx_init_fw(struct tas2562_data *tas2562, const struct firmware *fw); + +#endif /* _TAS25XX_DSP_LOADER_H */
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.
Mark
On 6/9/20 12:50 PM, Mark Brown wrote:
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.
Well the only values I see that change that regmap should care about are in first page of the register map.
After reverse engineering a binary I found that its contents modify page 0 registers of the device.
Not a fan of this as it causes un-wanted changes that may have been setup.
#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. :/
Yes I agree
+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?
Well the reason to set this back to page 0 is that is where the book register is.
So setting this back to page 0 set the Book register appropriately.
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).
Yeah. After reading this and thinking about this for a couple days. This actually has contention issues with the ALSA controls.
There needs to also be some locks put into place.
I prefer to disable the cache. Not sure how many other devices use Books and pages for register maps besides TI.
Adding that to regmap might be to specific to our devices.
+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?
No you are right. The issue with using enums is that the binary is not parsed until after codec_probe and the device is registered.
So the controls would appear later which could be a race condition for the user space.
- 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.
I will have to look into doing this. I blindly copy this data because there is really not a viable and reliable way to check sizes against the structures.
It'd also be good to double check that the number of configs and programs is within bounds.
This I can check once the data is copied.
Dan
On Fri, Jun 12, 2020 at 12:30:29PM -0500, Dan Murphy wrote:
On 6/9/20 12:50 PM, Mark Brown wrote:
On Tue, Jun 09, 2020 at 12:28:41PM -0500, Dan Murphy wrote:
/* Reset Page to 0 */ret = regmap_write(tas2562->regmap, TAS2562_PAGE_CTRL, 0);if (ret)return ret;
Why?
Well the reason to set this back to page 0 is that is where the book register is.
So setting this back to page 0 set the Book register appropriately.
Oh dear, usually the paging register is always visible :/
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).
Yeah. After reading this and thinking about this for a couple days. This actually has contention issues with the ALSA controls.
There needs to also be some locks put into place.
That's not too surprising for something like this.
I prefer to disable the cache. Not sure how many other devices use Books and pages for register maps besides TI.
Adding that to regmap might be to specific to our devices.
Single level pages are in there already, TI is a big user but I've seen this from other vendors too. I do remember some discussion of multi level paging before, I think with a TI part, and I *think* it already happens to work without needing to do anything but I'd need to go back and check and it's 7pm on a Friday night. IIRC if one paging register is within another paged region the code manages to recurse and sort things out, but I could be wrong. I agree that it's a bit specialist if it needs anything non-trivial and something driver local would be reasonable.
- 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.
I will have to look into doing this. I blindly copy this data because there is really not a viable and reliable way to check sizes against the structures.
Yeah, that's reasonable - I was just thinking about checking it against the size of the file to make sure it doesn't actually overflow the buffer and corrupt things or crash. Obviously sanity checking is good but there's limits on what's sensible.
On Tue, Jun 09, 2020 at 12:28:39PM -0500, Dan Murphy wrote:
These programs and configurations are selectable via files under the I2C dev node. There may be a better way to select this through ALSA controls but I was unable to find a good example of this. This is why this is an RFC patchset.
I think you can just use enums for most of this - what you want to do I think is parse the firmware, build templates for the controls and then add them with snd_soc_add_component_controls(). Userspace *should* cope with controls being hotplugged.
Mark
On 6/9/20 12:52 PM, Mark Brown wrote:
On Tue, Jun 09, 2020 at 12:28:39PM -0500, Dan Murphy wrote:
These programs and configurations are selectable via files under the I2C dev node. There may be a better way to select this through ALSA controls but I was unable to find a good example of this. This is why this is an RFC patchset.
I think you can just use enums for most of this - what you want to do I think is parse the firmware, build templates for the controls and then add them with snd_soc_add_component_controls(). Userspace *should* cope with controls being hotplugged.
Yes this was my concern if userspace could cope with dynamic controls.
Dan
On Tue, Jun 09, 2020 at 01:07:50PM -0500, Dan Murphy wrote:
On 6/9/20 12:52 PM, Mark Brown wrote:
I think you can just use enums for most of this - what you want to do I think is parse the firmware, build templates for the controls and then add them with snd_soc_add_component_controls(). Userspace *should* cope with controls being hotplugged.
Yes this was my concern if userspace could cope with dynamic controls.
Things like alsactl definitely do, and obviously anything that starts after the firmware loads will be fine too.
participants (3)
-
Dan Murphy -
Mark Brown -
Rob Herring