diff mbox series

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

Message ID 20200609172841.22541-2-dmurphy@ti.com
State New
Headers show
Series TAS2563 DSP Firmware Loader | expand

Commit Message

Dan Murphy June 9, 2020, 5:28 p.m. UTC
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(+)

-- 
2.26.2

Comments

Mark Brown June 9, 2020, 5:31 p.m. UTC | #1
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.
Dan Murphy June 9, 2020, 5:35 p.m. UTC | #2
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
Mark Brown June 9, 2020, 5:58 p.m. UTC | #3
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).
Dan Murphy June 9, 2020, 6:06 p.m. UTC | #4
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
Mark Brown June 9, 2020, 6:47 p.m. UTC | #5
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?
Dan Murphy June 9, 2020, 7:20 p.m. UTC | #6
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
Mark Brown June 10, 2020, 10:29 a.m. UTC | #7
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.
Dan Murphy June 10, 2020, 2:12 p.m. UTC | #8
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.
Mark Brown June 10, 2020, 2:28 p.m. UTC | #9
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.
Rob Herring (Arm) June 17, 2020, 10:04 p.m. UTC | #10
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.

> 

> 

>
Mark Brown June 18, 2020, 10:57 a.m. UTC | #11
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().
diff mbox series

Patch

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