| Message ID | 20200609172841.22541-2-dmurphy@ti.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | TAS2563 DSP Firmware Loader | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
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().
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
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(+)