| Message ID | 20231127133448.18449-7-peter.ujfalusi@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Intel: Soundwire related board and match updates | expand |
On 27/11/2023 13:34, Peter Ujfalusi wrote: > From: Bard Liao <yung-chuan.liao@linux.intel.com> > > This is a test configuration for UpExtreme. > > The codec layout is configured as: > - Link3: CS42L43 Jack > - Link0: 2x CS35L56 Speaker > - Link1: 2x CS35L56 Speaker > > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> > --- > .../intel/common/soc-acpi-intel-tgl-match.c | 78 +++++++++++++++++++ > 1 file changed, 78 insertions(+) > > diff --git a/sound/soc/intel/common/soc-acpi-intel-tgl-match.c b/sound/soc/intel/common/soc-acpi-intel-tgl-match.c > index 5804926c8b56..49834bffa50c 100644 > --- a/sound/soc/intel/common/soc-acpi-intel-tgl-match.c > +++ b/sound/soc/intel/common/soc-acpi-intel-tgl-match.c > @@ -41,6 +41,20 @@ static const struct snd_soc_acpi_endpoint spk_r_endpoint = { > .group_id = 1, > }; > > +static const struct snd_soc_acpi_endpoint spk_2_endpoint = { > + .num = 0, > + .aggregated = 1, > + .group_position = 2, > + .group_id = 1, > +}; > + > +static const struct snd_soc_acpi_endpoint spk_3_endpoint = { > + .num = 0, > + .aggregated = 1, > + .group_position = 3, > + .group_id = 1, > +}; > + > static const struct snd_soc_acpi_endpoint rt712_endpoints[] = { > { > .num = 0, > @@ -400,6 +414,64 @@ static const struct snd_soc_acpi_link_adr tgl_712_only[] = { > {} > }; > > +static const struct snd_soc_acpi_adr_device cs42l43_3_adr[] = { > + { > + .adr = 0x00033001FA424301ull, > + .num_endpoints = 1, > + .endpoints = &single_endpoint, > + .name_prefix = "cs42l43" > + } > +}; > + > +static const struct snd_soc_acpi_adr_device cs35l56_0_adr[] = { > + { > + .adr = 0x00003301FA355601ull, > + .num_endpoints = 1, > + .endpoints = &spk_r_endpoint, Assigning CS35L56 to "left" or "right" endpoints might be confusing. All CS35L56 in a system receive both left and right channels and by default they output a mono-mix of left+right. The left/right of an amp is determined by the firmware file (.bin) that is loaded and the current settings of the "Posture" ALSA control. So this amp might be the left channel after a .bin is loaded. It would be better to have generic names for the endpoint that don't imply position, for example: group1_spk1_endpoint group1_spk2_endpoint group1_spk3_endpoint group1_spk4_endpoint. > + .name_prefix = "cs35l56-8" Can these prefixes be "AMPn" to match the CS35L41, CS35L51 and CS35L56-hda driver? This prefix is used to find the matching firmware files and our naming convention for these has been cs35lxx-xxxx-ampn Is there anything that depends on the prefixes being "cs35l56-n" ? > + }, > + { > + .adr = 0x00003201FA355601ull, > + .num_endpoints = 1, > + .endpoints = &spk_3_endpoint, > + .name_prefix = "cs35l56-7" > + } > +}; > + > +static const struct snd_soc_acpi_adr_device cs35l56_1_adr[] = { > + { > + .adr = 0x00013701FA355601ull, > + .num_endpoints = 1, > + .endpoints = &spk_l_endpoint, > + .name_prefix = "cs35l56-1" > + }, > + { > + .adr = 0x00013601FA355601ull, > + .num_endpoints = 1, > + .endpoints = &spk_2_endpoint, > + .name_prefix = "cs35l56-2" > + } > +}; > + > +static const struct snd_soc_acpi_link_adr tgl_cs42l43_cs35l56[] = { > + { > + .mask = BIT(3), > + .num_adr = ARRAY_SIZE(cs42l43_3_adr), > + .adr_d = cs42l43_3_adr, > + }, > + { > + .mask = BIT(0), > + .num_adr = ARRAY_SIZE(cs35l56_0_adr), > + .adr_d = cs35l56_0_adr, > + }, > + { > + .mask = BIT(1), > + .num_adr = ARRAY_SIZE(cs35l56_1_adr), > + .adr_d = cs35l56_1_adr, > + }, > + {} > +}; > +
>> +static const struct snd_soc_acpi_adr_device cs35l56_0_adr[] = { >> + { >> + .adr = 0x00003301FA355601ull, >> + .num_endpoints = 1, >> + .endpoints = &spk_r_endpoint, > > Assigning CS35L56 to "left" or "right" endpoints might be confusing. > All CS35L56 in a system receive both left and right channels and by > default they output a mono-mix of left+right. > > The left/right of an amp is determined by the firmware file (.bin) that > is loaded and the current settings of the "Posture" ALSA control. So > this amp might be the left channel after a .bin is loaded. That's a problem if the kernel does not know which amplifier is on which side, no? How would one change the balance if this information is known only within a binary/opaque firmware? > It would be better to have generic names for the endpoint that don't > imply position, for example: > > group1_spk1_endpoint > group1_spk2_endpoint > group1_spk3_endpoint > group1_spk4_endpoint. The notion of endpoint is completely half-baked today and the settings used have no bearing on the behavior and user-experience. I am inches away from sending a patch that removes all of the endpoint definitions, we can re-add them if/when we can get the information from platform firmware. >> + .name_prefix = "cs35l56-8" > > Can these prefixes be "AMPn" to match the CS35L41, CS35L51 and > CS35L56-hda driver? This prefix is used to find the matching firmware > files and our naming convention for these has been cs35lxx-xxxx-ampn > > Is there anything that depends on the prefixes being "cs35l56-n" ? IIRC this name_prefix is just used for the codec_conf and hence for control names/UCM. At some point userspace/driver need to know if amp5 is left or right. We can certainly align on conventions but the values set in this ACPI match table will not be used for firmware download - different scope. >> + }, >> + { >> + .adr = 0x00003201FA355601ull, >> + .num_endpoints = 1, >> + .endpoints = &spk_3_endpoint, >> + .name_prefix = "cs35l56-7" >> + } >> +}; >> + >> +static const struct snd_soc_acpi_adr_device cs35l56_1_adr[] = { >> + { >> + .adr = 0x00013701FA355601ull, >> + .num_endpoints = 1, >> + .endpoints = &spk_l_endpoint, >> + .name_prefix = "cs35l56-1" >> + }, >> + { >> + .adr = 0x00013601FA355601ull, >> + .num_endpoints = 1, >> + .endpoints = &spk_2_endpoint, >> + .name_prefix = "cs35l56-2" >> + } >> +}; >> + >> +static const struct snd_soc_acpi_link_adr tgl_cs42l43_cs35l56[] = { >> + { >> + .mask = BIT(3), >> + .num_adr = ARRAY_SIZE(cs42l43_3_adr), >> + .adr_d = cs42l43_3_adr, >> + }, >> + { >> + .mask = BIT(0), >> + .num_adr = ARRAY_SIZE(cs35l56_0_adr), >> + .adr_d = cs35l56_0_adr, >> + }, >> + { >> + .mask = BIT(1), >> + .num_adr = ARRAY_SIZE(cs35l56_1_adr), >> + .adr_d = cs35l56_1_adr, >> + }, >> + {} >> +}; >> +
On 27/11/2023 17:36, Pierre-Louis Bossart wrote: > >>> +static const struct snd_soc_acpi_adr_device cs35l56_0_adr[] = { >>> + { >>> + .adr = 0x00003301FA355601ull, >>> + .num_endpoints = 1, >>> + .endpoints = &spk_r_endpoint, >> >> Assigning CS35L56 to "left" or "right" endpoints might be confusing. >> All CS35L56 in a system receive both left and right channels and by >> default they output a mono-mix of left+right. >> >> The left/right of an amp is determined by the firmware file (.bin) that >> is loaded and the current settings of the "Posture" ALSA control. So >> this amp might be the left channel after a .bin is loaded. > > That's a problem if the kernel does not know which amplifier is on which > side, no? How would one change the balance if this information is known > only within a binary/opaque firmware? > SDCA allows the posture (orientation) of amplifiers to be changed at runtime. CS35L56 is designed as a SDCA device so it doesn't have any hardwired position. SDCA doesn't define what the posture numbers mean, they are an integer that is system-specific. Because SDCA doesn't define the meaning of postures, and an SDCA device should work with a generic SDCA driver (which obviously wouldn't have hardcoded knowledge of the system) the speaker positions and postures are coded into the firmware It's difficult to say what is "default". For example, if you say that the default for a tablet is left/right/top/bottom, assuming it is used in portrait orientation, that would be wrong if the user always uses it in landscape. Matching by what amp is on what bus doesn't work well here because two systems could have the same arrangement of CS35L56 on each bus but use them for different purposes. So they could both match the "2 on bus 0, 2 on bus 1" table entry, but could be left/right/top/bottom on one device and left woofer/right woofer/left tweeter/right tweeter on another device. I assume that if the system supports rotation there should be something in the UCM or other userland that manages this. At least, it seems like it's a UCM problem to decide which speakers are doing what audio. If Linux-based distros don't have something like that - well, that just means Linux is behind Windows. >> It would be better to have generic names for the endpoint that don't >> imply position, for example: >> >> group1_spk1_endpoint >> group1_spk2_endpoint >> group1_spk3_endpoint >> group1_spk4_endpoint. > > The notion of endpoint is completely half-baked today and the settings > used have no bearing on the behavior and user-experience. I am inches > away from sending a patch that removes all of the endpoint definitions, > we can re-add them if/when we can get the information from platform > firmware. > >>> + .name_prefix = "cs35l56-8" >> >> Can these prefixes be "AMPn" to match the CS35L41, CS35L51 and >> CS35L56-hda driver? This prefix is used to find the matching firmware >> files and our naming convention for these has been cs35lxx-xxxx-ampn >> >> Is there anything that depends on the prefixes being "cs35l56-n" ? > > IIRC this name_prefix is just used for the codec_conf and hence for > control names/UCM. At some point userspace/driver need to know if amp5 > is left or right. > > We can certainly align on conventions but the values set in this ACPI > match table will not be used for firmware download - different scope. > They are used for our firmware download. Each amp can have its own unique firmware file. The ALSA prefix is used to identify which firmware file to load to which amp. >>> + }, >>> + { >>> + .adr = 0x00003201FA355601ull, >>> + .num_endpoints = 1, >>> + .endpoints = &spk_3_endpoint, >>> + .name_prefix = "cs35l56-7" >>> + } >>> +}; >>> + >>> +static const struct snd_soc_acpi_adr_device cs35l56_1_adr[] = { >>> + { >>> + .adr = 0x00013701FA355601ull, >>> + .num_endpoints = 1, >>> + .endpoints = &spk_l_endpoint, >>> + .name_prefix = "cs35l56-1" >>> + }, >>> + { >>> + .adr = 0x00013601FA355601ull, >>> + .num_endpoints = 1, >>> + .endpoints = &spk_2_endpoint, >>> + .name_prefix = "cs35l56-2" >>> + } >>> +}; >>> + >>> +static const struct snd_soc_acpi_link_adr tgl_cs42l43_cs35l56[] = { >>> + { >>> + .mask = BIT(3), >>> + .num_adr = ARRAY_SIZE(cs42l43_3_adr), >>> + .adr_d = cs42l43_3_adr, >>> + }, >>> + { >>> + .mask = BIT(0), >>> + .num_adr = ARRAY_SIZE(cs35l56_0_adr), >>> + .adr_d = cs35l56_0_adr, >>> + }, >>> + { >>> + .mask = BIT(1), >>> + .num_adr = ARRAY_SIZE(cs35l56_1_adr), >>> + .adr_d = cs35l56_1_adr, >>> + }, >>> + {} >>> +}; >>> +
On 11/28/23 04:31, Richard Fitzgerald wrote: > On 27/11/2023 17:36, Pierre-Louis Bossart wrote: >> >>>> +static const struct snd_soc_acpi_adr_device cs35l56_0_adr[] = { >>>> + { >>>> + .adr = 0x00003301FA355601ull, >>>> + .num_endpoints = 1, >>>> + .endpoints = &spk_r_endpoint, >>> >>> Assigning CS35L56 to "left" or "right" endpoints might be confusing. >>> All CS35L56 in a system receive both left and right channels and by >>> default they output a mono-mix of left+right. >>> >>> The left/right of an amp is determined by the firmware file (.bin) that >>> is loaded and the current settings of the "Posture" ALSA control. So >>> this amp might be the left channel after a .bin is loaded. >> >> That's a problem if the kernel does not know which amplifier is on which >> side, no? How would one change the balance if this information is known >> only within a binary/opaque firmware? >> > > SDCA allows the posture (orientation) of amplifiers to be changed at > runtime. CS35L56 is designed as a SDCA device so it doesn't have any > hardwired position. SDCA doesn't define what the posture numbers mean, > they are an integer that is system-specific. > > Because SDCA doesn't define the meaning of postures, and an SDCA device > should work with a generic SDCA driver (which obviously wouldn't have > hardcoded knowledge of the system) the speaker positions and postures > are coded into the firmware > > It's difficult to say what is "default". For example, if you say that > the default for a tablet is left/right/top/bottom, assuming it is > used in portrait orientation, that would be wrong if the user always > uses it in landscape. > > Matching by what amp is on what bus doesn't work well here because two > systems could have the same arrangement of CS35L56 on each bus but use > them for different purposes. So they could both match the "2 on bus 0, 2 > on bus 1" table entry, but could be left/right/top/bottom on one device > and left woofer/right woofer/left tweeter/right tweeter on another > device. In the absence of any platform firmware information, I am not sure how we can deal with such systems. The match tables are already hard to support given that a number of OEMs get the _ADR wrong, the speaker position is the next-level... Or did you just volunteer to maintain a DMI quirk table for Cirrus-based systems :-) I also bet that at some point the wrong firmware will be loaded on the wrong amplifiers, that could be fun as well. > I assume that if the system supports rotation there should be something > in the UCM or other userland that manages this. At least, it seems like > it's a UCM problem to decide which speakers are doing what audio. > If Linux-based distros don't have something like that - well, that just > means Linux is behind Windows. SDCA has lots of fancy concepts, posture is one. Last time I checked we don't have any reports of the hinge angle in Linux so the best we can do is landscape/portrait, and even that is questionable given that tablets or detachables have not reached any developers so far. CI automation is another fun issue, we'll need robotic arms to move the device around and intelligent alsa-bat-v2 to verify sound levels... The notion of which speakers do what is something that will clearly take years to figure out. For now the main issue is to get all parts connected and basic "loud enough" sound working. >>> It would be better to have generic names for the endpoint that don't >>> imply position, for example: >>> >>> group1_spk1_endpoint >>> group1_spk2_endpoint >>> group1_spk3_endpoint >>> group1_spk4_endpoint. >> >> The notion of endpoint is completely half-baked today and the settings >> used have no bearing on the behavior and user-experience. I am inches >> away from sending a patch that removes all of the endpoint definitions, >> we can re-add them if/when we can get the information from platform >> firmware. >> >>>> + .name_prefix = "cs35l56-8" >>> >>> Can these prefixes be "AMPn" to match the CS35L41, CS35L51 and >>> CS35L56-hda driver? This prefix is used to find the matching firmware >>> files and our naming convention for these has been cs35lxx-xxxx-ampn >>> >>> Is there anything that depends on the prefixes being "cs35l56-n" ? >> >> IIRC this name_prefix is just used for the codec_conf and hence for >> control names/UCM. At some point userspace/driver need to know if amp5 >> is left or right. >> >> We can certainly align on conventions but the values set in this ACPI >> match table will not be used for firmware download - different scope. >> > > They are used for our firmware download. Each amp can have its own > unique firmware file. The ALSA prefix is used to identify which firmware > file to load to which amp. The prefix will only be used when the card is created, specifically for control names. The firmware should be selected and downloaded when the device shows up on the bus. Card creation and device enumeration/initialization happen on different timelines, if the machine driver is "blacklisted" or unbound I am not sure what happens. There is a dependency between machine driver probe and codec firmware download that I am not able to follow, can you please elaborate?
On 28/11/2023 15:23, Pierre-Louis Bossart wrote: > > > On 11/28/23 04:31, Richard Fitzgerald wrote: >> On 27/11/2023 17:36, Pierre-Louis Bossart wrote: >>> >>>>> +static const struct snd_soc_acpi_adr_device cs35l56_0_adr[] = { >>>>> + { >>>>> + .adr = 0x00003301FA355601ull, >>>>> + .num_endpoints = 1, >>>>> + .endpoints = &spk_r_endpoint, >>>> >>>> Assigning CS35L56 to "left" or "right" endpoints might be confusing. >>>> All CS35L56 in a system receive both left and right channels and by >>>> default they output a mono-mix of left+right. >>>> >>>> The left/right of an amp is determined by the firmware file (.bin) that >>>> is loaded and the current settings of the "Posture" ALSA control. So >>>> this amp might be the left channel after a .bin is loaded. >>> >>> That's a problem if the kernel does not know which amplifier is on which >>> side, no? How would one change the balance if this information is known >>> only within a binary/opaque firmware? >>> >> >> SDCA allows the posture (orientation) of amplifiers to be changed at >> runtime. CS35L56 is designed as a SDCA device so it doesn't have any >> hardwired position. SDCA doesn't define what the posture numbers mean, >> they are an integer that is system-specific. >> >> Because SDCA doesn't define the meaning of postures, and an SDCA device >> should work with a generic SDCA driver (which obviously wouldn't have >> hardcoded knowledge of the system) the speaker positions and postures >> are coded into the firmware >> >> It's difficult to say what is "default". For example, if you say that >> the default for a tablet is left/right/top/bottom, assuming it is >> used in portrait orientation, that would be wrong if the user always >> uses it in landscape. >> >> Matching by what amp is on what bus doesn't work well here because two >> systems could have the same arrangement of CS35L56 on each bus but use >> them for different purposes. So they could both match the "2 on bus 0, 2 >> on bus 1" table entry, but could be left/right/top/bottom on one device >> and left woofer/right woofer/left tweeter/right tweeter on another >> device. > > In the absence of any platform firmware information, I am not sure how > we can deal with such systems. The match tables are already hard to > support given that a number of OEMs get the _ADR wrong, the speaker > position is the next-level... > > Or did you just volunteer to maintain a DMI quirk table for Cirrus-based > systems :-) > Short answer: "That's SDCA." I don't think a quirk table is needed. It's just that we can't hardcode "this speaker is left, that speaker is right". SDCA defers orientation changes to the amp through the posture control. If you have a daemon to handle rotation, everything will be fine and left audio is on your left. Let's say you have a tablet and you hold it in portrait with left and right correct. You then rotate it 180 degrees, if the daemon updates the posture control, the amps will swap channels so left audio is still on your left, and right is still on your right. If Linux distros don't have any daemon that can handle rotation, then rotating the tablet 180 degrees is going to give you left and right audio on the wrong sides. But that's what you'd expect if nothing is handling rotation, and you'd get the same problem if it was all done by changing the routing in ALSA controls but there was nothing to change that routing. Getting back to my original comment about endpoints. It really doesn't matter what the endpoint structs are called because all they do is declare the aggregation. I was only suggesting to maybe avoid names that imply a specific function. When I said "Confusing" that was overstating things. A bit misleading perhaps. > I also bet that at some point the wrong firmware will be loaded on the > wrong amplifiers, that could be fun as well. > Hence using the SSDI + ALSA prefix to qualify the firmware files. We aim to push out all the firmware to linux-firmware for products we know about. So far it's worked ok for CS35L41 and CS35L51 - problems with those have been with incorrect ACPI. >> I assume that if the system supports rotation there should be something >> in the UCM or other userland that manages this. At least, it seems like >> it's a UCM problem to decide which speakers are doing what audio. >> If Linux-based distros don't have something like that - well, that just >> means Linux is behind Windows. > > SDCA has lots of fancy concepts, posture is one. Last time I checked we > don't have any reports of the hinge angle in Linux so the best we can do > is landscape/portrait, and even that is questionable given that tablets > or detachables have not reached any developers so far. CI automation is > another fun issue, we'll need robotic arms to move the device around and > intelligent alsa-bat-v2 to verify sound levels... > > The notion of which speakers do what is something that will clearly take > years to figure out. For now the main issue is to get all parts > connected and basic "loud enough" sound working. > It's still the case that shiny new things on x86 platforms will be designed around Windows and made to work there. Then Linux has to catch up with that. >>>> It would be better to have generic names for the endpoint that don't >>>> imply position, for example: >>>> >>>> group1_spk1_endpoint >>>> group1_spk2_endpoint >>>> group1_spk3_endpoint >>>> group1_spk4_endpoint. >>> >>> The notion of endpoint is completely half-baked today and the settings >>> used have no bearing on the behavior and user-experience. I am inches >>> away from sending a patch that removes all of the endpoint definitions, >>> we can re-add them if/when we can get the information from platform >>> firmware. >>> >>>>> + .name_prefix = "cs35l56-8" >>>> >>>> Can these prefixes be "AMPn" to match the CS35L41, CS35L51 and >>>> CS35L56-hda driver? This prefix is used to find the matching firmware >>>> files and our naming convention for these has been cs35lxx-xxxx-ampn >>>> >>>> Is there anything that depends on the prefixes being "cs35l56-n" ? >>> >>> IIRC this name_prefix is just used for the codec_conf and hence for >>> control names/UCM. At some point userspace/driver need to know if amp5 >>> is left or right. >>> >>> We can certainly align on conventions but the values set in this ACPI >>> match table will not be used for firmware download - different scope. >>> >> >> They are used for our firmware download. Each amp can have its own >> unique firmware file. The ALSA prefix is used to identify which firmware >> file to load to which amp. > > The prefix will only be used when the card is created, specifically for > control names. > The firmware should be selected and downloaded when the device shows up > on the bus. > Card creation and device enumeration/initialization happen on different > timelines, if the machine driver is "blacklisted" or unbound I am not > sure what happens. > > There is a dependency between machine driver probe and codec firmware > download that I am not able to follow, can you please elaborate? > The codec driver has to choose which firmware to load from under /lib/firmware. It does this using a combination of SSID (to identify the target product), the ALSA prefix string (to identify which amp) and in some systems a GPIO on the motherboard to select between different models of speaker when they have multiple suppliers. This results in a firmware name like: cs35l56-<silicon rev>-dsp1-misc-<SSID>[-<SPEAKER MODEL>]-<ALSA PREFIX> You can see this if you look in the linux-firmware repo under cirrus/ for cs35l41 firmware files (though the ALSA PREFIX section in those cases is not "AMPn" because they are not SDCA parts with rotation, they have a fixed left/right assignment.) We have to be careful of the length of the prefix. The 44 characters of an ALSA control name get eaten up very quickly when we start creating fully-qualified names for controls published by the firmware. So "AMPn" was nice because it was descriptive enough but only uses 5 characters of the 44. Having said that, I've calculated that we have enough characters (just) to use a prefix of "cs35l56-n". If there's a reason why that is necessary/desirable for SOF or SoundWire then we could do that. But we'd intended to use "AMPn" prefixes. We just need to decide whether to go with "AMPn". Or switch to using "cs35l56-n" for the ALSA prefix (the therefore the qualifier at the end of the firmware filename).
>>>>>> + .name_prefix = "cs35l56-8" >>>>> >>>>> Can these prefixes be "AMPn" to match the CS35L41, CS35L51 and >>>>> CS35L56-hda driver? This prefix is used to find the matching firmware >>>>> files and our naming convention for these has been cs35lxx-xxxx-ampn >>>>> >>>>> Is there anything that depends on the prefixes being "cs35l56-n" ? >>>> >>>> IIRC this name_prefix is just used for the codec_conf and hence for >>>> control names/UCM. At some point userspace/driver need to know if amp5 >>>> is left or right. >>>> >>>> We can certainly align on conventions but the values set in this ACPI >>>> match table will not be used for firmware download - different scope. >>>> >>> >>> They are used for our firmware download. Each amp can have its own >>> unique firmware file. The ALSA prefix is used to identify which firmware >>> file to load to which amp. >> >> The prefix will only be used when the card is created, specifically for >> control names. >> The firmware should be selected and downloaded when the device shows up >> on the bus. >> Card creation and device enumeration/initialization happen on different >> timelines, if the machine driver is "blacklisted" or unbound I am not >> sure what happens. >> >> There is a dependency between machine driver probe and codec firmware >> download that I am not able to follow, can you please elaborate? >> > > The codec driver has to choose which firmware to load from under > /lib/firmware. It does this using a combination of SSID (to identify the > target product), the ALSA prefix string (to identify which amp) and > in some systems a GPIO on the motherboard to select between different > models of speaker when they have multiple suppliers. This results in a > firmware name like: > > cs35l56-<silicon rev>-dsp1-misc-<SSID>[-<SPEAKER MODEL>]-<ALSA PREFIX> > > You can see this if you look in the linux-firmware repo under cirrus/ > for cs35l41 firmware files (though the ALSA PREFIX section in those > cases is not "AMPn" because they are not SDCA parts with rotation, > they have a fixed left/right assignment.) > > We have to be careful of the length of the prefix. The 44 characters of > an ALSA control name get eaten up very quickly when we start creating > fully-qualified names for controls published by the firmware. So "AMPn" > was nice because it was descriptive enough but only uses 5 characters > of the 44. > > Having said that, I've calculated that we have enough characters (just) > to use a prefix of "cs35l56-n". If there's a reason why that is > necessary/desirable for SOF or SoundWire then we could do that. But we'd > intended to use "AMPn" prefixes. > > We just need to decide whether to go with "AMPn". Or switch to using > "cs35l56-n" for the ALSA prefix (the therefore the qualifier at the end > of the firmware filename). Yes we have similar issues with control names in topology, the limit is hit very quickly. I think you missed my point though that the ALSA prefix is only set when the card is created, which can be sometime after the firmware needs to be downloaded. I guess you could pick the firmware in the component probe, which happens during the card creation, but that could be sub-optimal. Given the download times you want the download to proceed as early as possible.
On 29/11/23 16:39, Pierre-Louis Bossart wrote: > >>>>>>> + .name_prefix = "cs35l56-8" >>>>>> >>>>>> Can these prefixes be "AMPn" to match the CS35L41, CS35L51 and >>>>>> CS35L56-hda driver? This prefix is used to find the matching firmware >>>>>> files and our naming convention for these has been cs35lxx-xxxx-ampn >>>>>> >>>>>> Is there anything that depends on the prefixes being "cs35l56-n" ? >>>>> >>>>> IIRC this name_prefix is just used for the codec_conf and hence for >>>>> control names/UCM. At some point userspace/driver need to know if amp5 >>>>> is left or right. >>>>> >>>>> We can certainly align on conventions but the values set in this ACPI >>>>> match table will not be used for firmware download - different scope. >>>>> >>>> >>>> They are used for our firmware download. Each amp can have its own >>>> unique firmware file. The ALSA prefix is used to identify which firmware >>>> file to load to which amp. >>> >>> The prefix will only be used when the card is created, specifically for >>> control names. >>> The firmware should be selected and downloaded when the device shows up >>> on the bus. >>> Card creation and device enumeration/initialization happen on different >>> timelines, if the machine driver is "blacklisted" or unbound I am not >>> sure what happens. >>> >>> There is a dependency between machine driver probe and codec firmware >>> download that I am not able to follow, can you please elaborate? >>> >> >> The codec driver has to choose which firmware to load from under >> /lib/firmware. It does this using a combination of SSID (to identify the >> target product), the ALSA prefix string (to identify which amp) and >> in some systems a GPIO on the motherboard to select between different >> models of speaker when they have multiple suppliers. This results in a >> firmware name like: >> >> cs35l56-<silicon rev>-dsp1-misc-<SSID>[-<SPEAKER MODEL>]-<ALSA PREFIX> >> >> You can see this if you look in the linux-firmware repo under cirrus/ >> for cs35l41 firmware files (though the ALSA PREFIX section in those >> cases is not "AMPn" because they are not SDCA parts with rotation, >> they have a fixed left/right assignment.) >> >> We have to be careful of the length of the prefix. The 44 characters of >> an ALSA control name get eaten up very quickly when we start creating >> fully-qualified names for controls published by the firmware. So "AMPn" >> was nice because it was descriptive enough but only uses 5 characters >> of the 44. >> >> Having said that, I've calculated that we have enough characters (just) >> to use a prefix of "cs35l56-n". If there's a reason why that is >> necessary/desirable for SOF or SoundWire then we could do that. But we'd >> intended to use "AMPn" prefixes. >> >> We just need to decide whether to go with "AMPn". Or switch to using >> "cs35l56-n" for the ALSA prefix (the therefore the qualifier at the end >> of the firmware filename). > > Yes we have similar issues with control names in topology, the limit is > hit very quickly. > > I think you missed my point though that the ALSA prefix is only set when > the card is created, which can be sometime after the firmware needs to > be downloaded. I guess you could pick the firmware in the component > probe, which happens during the card creation, but that could be > sub-optimal. Given the download times you want the download to proceed > as early as possible. We kick off a background task from our component_probe() to do the firmware download. We need the ALSA prefix, and also the wm_adsp library that actually handles the DSP is ASoC code so it needs a probed component. Doing it in a background work means it doesn't block probe(). And the download to multiple amps can proceed in parallel - obviously that's constrained by bus bandwidth but we are seeing that they interleave.
On 11/30/23 04:15, Richard Fitzgerald wrote: > On 29/11/23 16:39, Pierre-Louis Bossart wrote: >> >>>>>>>> + .name_prefix = "cs35l56-8" >>>>>>> >>>>>>> Can these prefixes be "AMPn" to match the CS35L41, CS35L51 and >>>>>>> CS35L56-hda driver? This prefix is used to find the matching >>>>>>> firmware >>>>>>> files and our naming convention for these has been cs35lxx-xxxx-ampn >>>>>>> >>>>>>> Is there anything that depends on the prefixes being "cs35l56-n" ? >>>>>> >>>>>> IIRC this name_prefix is just used for the codec_conf and hence for >>>>>> control names/UCM. At some point userspace/driver need to know if >>>>>> amp5 >>>>>> is left or right. >>>>>> >>>>>> We can certainly align on conventions but the values set in this ACPI >>>>>> match table will not be used for firmware download - different scope. >>>>>> >>>>> >>>>> They are used for our firmware download. Each amp can have its own >>>>> unique firmware file. The ALSA prefix is used to identify which >>>>> firmware >>>>> file to load to which amp. >>>> >>>> The prefix will only be used when the card is created, specifically for >>>> control names. >>>> The firmware should be selected and downloaded when the device shows up >>>> on the bus. >>>> Card creation and device enumeration/initialization happen on different >>>> timelines, if the machine driver is "blacklisted" or unbound I am not >>>> sure what happens. >>>> >>>> There is a dependency between machine driver probe and codec firmware >>>> download that I am not able to follow, can you please elaborate? >>>> >>> >>> The codec driver has to choose which firmware to load from under >>> /lib/firmware. It does this using a combination of SSID (to identify the >>> target product), the ALSA prefix string (to identify which amp) and >>> in some systems a GPIO on the motherboard to select between different >>> models of speaker when they have multiple suppliers. This results in a >>> firmware name like: >>> >>> cs35l56-<silicon rev>-dsp1-misc-<SSID>[-<SPEAKER MODEL>]-<ALSA PREFIX> >>> >>> You can see this if you look in the linux-firmware repo under cirrus/ >>> for cs35l41 firmware files (though the ALSA PREFIX section in those >>> cases is not "AMPn" because they are not SDCA parts with rotation, >>> they have a fixed left/right assignment.) >>> >>> We have to be careful of the length of the prefix. The 44 characters of >>> an ALSA control name get eaten up very quickly when we start creating >>> fully-qualified names for controls published by the firmware. So "AMPn" >>> was nice because it was descriptive enough but only uses 5 characters >>> of the 44. >>> >>> Having said that, I've calculated that we have enough characters (just) >>> to use a prefix of "cs35l56-n". If there's a reason why that is >>> necessary/desirable for SOF or SoundWire then we could do that. But we'd >>> intended to use "AMPn" prefixes. >>> >>> We just need to decide whether to go with "AMPn". Or switch to using >>> "cs35l56-n" for the ALSA prefix (the therefore the qualifier at the end >>> of the firmware filename). >> >> Yes we have similar issues with control names in topology, the limit is >> hit very quickly. >> >> I think you missed my point though that the ALSA prefix is only set when >> the card is created, which can be sometime after the firmware needs to >> be downloaded. I guess you could pick the firmware in the component >> probe, which happens during the card creation, but that could be >> sub-optimal. Given the download times you want the download to proceed >> as early as possible. > > We kick off a background task from our component_probe() to do the > firmware download. We need the ALSA prefix, and also the wm_adsp library > that actually handles the DSP is ASoC code so it needs a probed > component. Doing it in a background work means it doesn't block probe(). > And the download to multiple amps can proceed in parallel - obviously > that's constrained by bus bandwidth but we are seeing that they > interleave. ah ok, that makes sense. In practice the delta between codec enumeration and component probe is probably negligible, and in a multi-amplifier setup the download times are much larger. So to circle back to the initial feedback, do you mind submitting a patch with the exact naming you'd want for the prefix?
On 27/11/23 14:40, Richard Fitzgerald wrote: > > >> + .name_prefix = "cs35l56-8" > > Can these prefixes be "AMPn" to match the CS35L41, CS35L51 and > CS35L56-hda driver? This prefix is used to find the matching firmware > files and our naming convention for these has been cs35lxx-xxxx-ampn > > Is there anything that depends on the prefixes being "cs35l56-n" ? > We're thinking about whether to change to using "cs35l56-n" prefix. Hold on this while we make a decision...
diff --git a/sound/soc/intel/common/soc-acpi-intel-tgl-match.c b/sound/soc/intel/common/soc-acpi-intel-tgl-match.c index 5804926c8b56..49834bffa50c 100644 --- a/sound/soc/intel/common/soc-acpi-intel-tgl-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-tgl-match.c @@ -41,6 +41,20 @@ static const struct snd_soc_acpi_endpoint spk_r_endpoint = { .group_id = 1, }; +static const struct snd_soc_acpi_endpoint spk_2_endpoint = { + .num = 0, + .aggregated = 1, + .group_position = 2, + .group_id = 1, +}; + +static const struct snd_soc_acpi_endpoint spk_3_endpoint = { + .num = 0, + .aggregated = 1, + .group_position = 3, + .group_id = 1, +}; + static const struct snd_soc_acpi_endpoint rt712_endpoints[] = { { .num = 0, @@ -400,6 +414,64 @@ static const struct snd_soc_acpi_link_adr tgl_712_only[] = { {} }; +static const struct snd_soc_acpi_adr_device cs42l43_3_adr[] = { + { + .adr = 0x00033001FA424301ull, + .num_endpoints = 1, + .endpoints = &single_endpoint, + .name_prefix = "cs42l43" + } +}; + +static const struct snd_soc_acpi_adr_device cs35l56_0_adr[] = { + { + .adr = 0x00003301FA355601ull, + .num_endpoints = 1, + .endpoints = &spk_r_endpoint, + .name_prefix = "cs35l56-8" + }, + { + .adr = 0x00003201FA355601ull, + .num_endpoints = 1, + .endpoints = &spk_3_endpoint, + .name_prefix = "cs35l56-7" + } +}; + +static const struct snd_soc_acpi_adr_device cs35l56_1_adr[] = { + { + .adr = 0x00013701FA355601ull, + .num_endpoints = 1, + .endpoints = &spk_l_endpoint, + .name_prefix = "cs35l56-1" + }, + { + .adr = 0x00013601FA355601ull, + .num_endpoints = 1, + .endpoints = &spk_2_endpoint, + .name_prefix = "cs35l56-2" + } +}; + +static const struct snd_soc_acpi_link_adr tgl_cs42l43_cs35l56[] = { + { + .mask = BIT(3), + .num_adr = ARRAY_SIZE(cs42l43_3_adr), + .adr_d = cs42l43_3_adr, + }, + { + .mask = BIT(0), + .num_adr = ARRAY_SIZE(cs35l56_0_adr), + .adr_d = cs35l56_0_adr, + }, + { + .mask = BIT(1), + .num_adr = ARRAY_SIZE(cs35l56_1_adr), + .adr_d = cs35l56_1_adr, + }, + {} +}; + static const struct snd_soc_acpi_codecs tgl_max98373_amp = { .num_codecs = 1, .codecs = {"MX98373"} @@ -494,6 +566,12 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_sdw_machines[] = { .drv_name = "sof_sdw", .sof_tplg_filename = "sof-tgl-rt715-rt711-rt1308-mono.tplg", }, + { + .link_mask = 0xB, + .links = tgl_cs42l43_cs35l56, + .drv_name = "sof_sdw", + .sof_tplg_filename = "sof-tgl-cs42l43-l3-cs35l56-l01.tplg", + }, { .link_mask = 0xF, /* 4 active links required */ .links = tgl_3_in_1_default,