[RFC PATCH 0/2] TAS2563 DSP Firmware Loader

[RFC PATCH 0/2] TAS2563 DSP Firmware Loader

[Dan Murphy]

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

-- 
2.26.2

[RFC PATCH 1/2] dt-bindings: tas2562: Add firmware support for tas2563

[Dan Murphy]

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
-- 
2.26.2

Re: [RFC PATCH 1/2] dt-bindings: tas2562: Add firmware support for tas2563

[Mark Brown]

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

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.

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

Re: [RFC PATCH 1/2] dt-bindings: tas2562: Add firmware support for tas2563

[Dan Murphy]

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

Re: [RFC PATCH 1/2] dt-bindings: tas2562: Add firmware support for tas2563

[Mark Brown]

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

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

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

Re: [RFC PATCH 1/2] dt-bindings: tas2562: Add firmware support for tas2563

[Dan Murphy]

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

Re: [RFC PATCH 1/2] dt-bindings: tas2562: Add firmware support for tas2563

[Mark Brown]

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

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?

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

Re: [RFC PATCH 1/2] dt-bindings: tas2562: Add firmware support for tas2563

[Dan Murphy]

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

Re: [RFC PATCH 1/2] dt-bindings: tas2562: Add firmware support for tas2563

[Mark Brown]

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

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.

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

Re: [RFC PATCH 1/2] dt-bindings: tas2562: Add firmware support for tas2563

[Dan Murphy]

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.

Re: [RFC PATCH 1/2] dt-bindings: tas2562: Add firmware support for tas2563

[Mark Brown]

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

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.

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

Re: [RFC PATCH 1/2] dt-bindings: tas2562: Add firmware support for tas2563

[Rob Herring]

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

Re: [RFC PATCH 1/2] dt-bindings: tas2562: Add firmware support for tas2563

[Mark Brown]

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

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().

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

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

[Dan Murphy]

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 */
-- 
2.26.2

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

[Mark Brown]

[-- 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 --]

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

[Dan Murphy]

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

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

[Mark Brown]

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

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.

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

Re: [RFC PATCH 0/2] TAS2563 DSP Firmware Loader

[Mark Brown]

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

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.

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

Re: [RFC PATCH 0/2] TAS2563 DSP Firmware Loader

[Dan Murphy]

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

Re: [RFC PATCH 0/2] TAS2563 DSP Firmware Loader

[Mark Brown]

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

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.

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