[PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware
The PCI device registers contain a subsystem ID (SSID), that is separate from the silicon ID. The PCI specification defines it thus:
"They provide a mechanism for board vendors to distiguish their boards from one another even thought the boards may have the same PCI controller on them."
This allows the driver for the silicon part to apply board-speficic settings based on this SSID.
The CS35L56 driver uses this to select the correct firmware file for the board. The actual ID is part of the PCI register set of the host audio interface so this set of patches includes extracting the SSID from the Intel audio controller and passing it to the machine driver and then to ASoC components. Other PCI audio controllers will have the same SSID registers, so can use the same mechanism to pass the SSID.
Richard Fitzgerald (4): ASoC: soc-card: Add storage for PCI SSID ASoC: SOF: Pass PCI SSID to machine driver ASoC: Intel: sof_sdw: Copy PCI SSID to struct snd_soc_card ASoC: cs35l56: Use PCI SSID as the firmware UID
include/sound/soc-acpi.h | 7 ++++++ include/sound/soc-card.h | 37 ++++++++++++++++++++++++++++++++ include/sound/soc.h | 11 ++++++++++ include/sound/sof.h | 8 +++++++ sound/soc/codecs/cs35l56.c | 11 ++++++++++ sound/soc/intel/boards/sof_sdw.c | 6 ++++++ sound/soc/sof/sof-audio.c | 7 ++++++ sound/soc/sof/sof-pci-dev.c | 8 +++++++ 8 files changed, 95 insertions(+)
Add members to struct snd_soc_card to store the PCI subsystem ID (SSID) of the soundcard.
The PCI specification provides two registers to store a vendor-specific SSID that can be read by drivers to uniquely identify a particular "soundcard". This is defined in the PCI specification to distinguish products that use the same silicon (and therefore have the same silicon ID) so that product-specific differences can be applied.
PCI only defines 0xFFFF as an invalid value. 0x0000 is not defined as invalid. So the usual pattern of zero-filling the struct and then assuming a zero value unset will not work. A flag is included to indicate when the SSID information has been filled in.
Unlike DMI information, which has a free-format entirely up to the vendor, the PCI SSID has a strictly defined format and a registry of vendor IDs.
It is usual in Windows drivers that the SSID is used as the sole identifier of the specific end-product and the Windows driver contains tables mapping that to information about the hardware setup, rather than using ACPI properties.
This SSID is important information for ASoC components that need to apply hardware-specific configuration on PCI-based systems.
As the SSID is a generic part of the PCI specification and is treated as identifying the "soundcard", it is reasonable to include this information in struct snd_soc_card, instead of components inventing their own custom ways to pass this information around.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- include/sound/soc-card.h | 37 +++++++++++++++++++++++++++++++++++++ include/sound/soc.h | 11 +++++++++++ 2 files changed, 48 insertions(+)
diff --git a/include/sound/soc-card.h b/include/sound/soc-card.h index fc94dfb0021f..e8ff2e089cd0 100644 --- a/include/sound/soc-card.h +++ b/include/sound/soc-card.h @@ -59,6 +59,43 @@ int snd_soc_card_add_dai_link(struct snd_soc_card *card, void snd_soc_card_remove_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link);
+#ifdef CONFIG_PCI +static inline void snd_soc_card_set_pci_ssid(struct snd_soc_card *card, + unsigned short vendor, + unsigned short device) +{ + card->pci_subsystem_vendor = vendor; + card->pci_subsystem_device = device; + card->pci_subsystem_set = true; +} + +static inline int snd_soc_card_get_pci_ssid(struct snd_soc_card *card, + unsigned short *vendor, + unsigned short *device) +{ + if (!card->pci_subsystem_set) + return -ENOENT; + + *vendor = card->pci_subsystem_vendor; + *device = card->pci_subsystem_device; + + return 0; +} +#else /* !CONFIG_PCI */ +static inline void snd_soc_card_set_pci_ssid(struct snd_soc_card *card, + unsigned short vendor, + unsigned short device) +{ +} + +static inline int snd_soc_card_get_pci_ssid(struct snd_soc_card *card, + unsigned short *vendor, + unsigned short *device) +{ + return -ENOENT; +} +#endif /* CONFIG_PCI */ + /* device driver data */ static inline void snd_soc_card_set_drvdata(struct snd_soc_card *card, void *data) diff --git a/include/sound/soc.h b/include/sound/soc.h index 509386ff5212..81ed08c5c67d 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -929,6 +929,17 @@ struct snd_soc_card { #ifdef CONFIG_DMI char dmi_longname[80]; #endif /* CONFIG_DMI */ + +#ifdef CONFIG_PCI + /* + * PCI does not define 0 as invalid, so pci_subsystem_set indicates + * whether a value has been written to these fields. + */ + unsigned short pci_subsystem_vendor; + unsigned short pci_subsystem_device; + bool pci_subsystem_set; +#endif /* CONFIG_PCI */ + char topology_shortname[32];
struct device *dev;
On 9/12/2023 6:32 PM, Richard Fitzgerald wrote:
Add members to struct snd_soc_card to store the PCI subsystem ID (SSID) of the soundcard.
The PCI specification provides two registers to store a vendor-specific SSID that can be read by drivers to uniquely identify a particular "soundcard". This is defined in the PCI specification to distinguish products that use the same silicon (and therefore have the same silicon ID) so that product-specific differences can be applied.
PCI only defines 0xFFFF as an invalid value. 0x0000 is not defined as invalid. So the usual pattern of zero-filling the struct and then assuming a zero value unset will not work. A flag is included to indicate when the SSID information has been filled in.
Unlike DMI information, which has a free-format entirely up to the vendor, the PCI SSID has a strictly defined format and a registry of vendor IDs.
It is usual in Windows drivers that the SSID is used as the sole identifier of the specific end-product and the Windows driver contains tables mapping that to information about the hardware setup, rather than using ACPI properties.
This SSID is important information for ASoC components that need to apply hardware-specific configuration on PCI-based systems.
As the SSID is a generic part of the PCI specification and is treated as identifying the "soundcard", it is reasonable to include this information in struct snd_soc_card, instead of components inventing their own custom ways to pass this information around.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com
include/sound/soc-card.h | 37 +++++++++++++++++++++++++++++++++++++ include/sound/soc.h | 11 +++++++++++ 2 files changed, 48 insertions(+)
diff --git a/include/sound/soc-card.h b/include/sound/soc-card.h index fc94dfb0021f..e8ff2e089cd0 100644 --- a/include/sound/soc-card.h +++ b/include/sound/soc-card.h
...
diff --git a/include/sound/soc.h b/include/sound/soc.h index 509386ff5212..81ed08c5c67d 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -929,6 +929,17 @@ struct snd_soc_card { #ifdef CONFIG_DMI char dmi_longname[80]; #endif /* CONFIG_DMI */
+#ifdef CONFIG_PCI
- /*
* PCI does not define 0 as invalid, so pci_subsystem_set indicates* whether a value has been written to these fields.*/- unsigned short pci_subsystem_vendor;
- unsigned short pci_subsystem_device;
- bool pci_subsystem_set;
+#endif /* CONFIG_PCI */
char topology_shortname[32];
struct device *dev;
This looks bit weird to me, snd_soc_card is _generic_ struct which is not device specific in any way, and now you add fields for PCI, can't this somehow be done using drvdata or something?
On Wed, Sep 13, 2023 at 12:56:03PM +0200, Amadeusz Sławiński wrote:
On 9/12/2023 6:32 PM, Richard Fitzgerald wrote:
+#ifdef CONFIG_PCI
- /*
* PCI does not define 0 as invalid, so pci_subsystem_set indicates* whether a value has been written to these fields.*/- unsigned short pci_subsystem_vendor;
- unsigned short pci_subsystem_device;
- bool pci_subsystem_set;
+#endif /* CONFIG_PCI */
- char topology_shortname[32]; struct device *dev;
This looks bit weird to me, snd_soc_card is _generic_ struct which is not device specific in any way, and now you add fields for PCI, can't this somehow be done using drvdata or something?
You're right that it's a bit messy but if we use driver data then it becomes specific to a particular driver and there's a goal here to share with subfunction drivers. I was thinking we could refactor to a union or otherwise extend if we find other users with a similar need.
On 13/09/2023 13:58, Mark Brown wrote:
On Wed, Sep 13, 2023 at 12:56:03PM +0200, Amadeusz Sławiński wrote:
On 9/12/2023 6:32 PM, Richard Fitzgerald wrote:
+#ifdef CONFIG_PCI
- /*
* PCI does not define 0 as invalid, so pci_subsystem_set indicates* whether a value has been written to these fields.*/- unsigned short pci_subsystem_vendor;
- unsigned short pci_subsystem_device;
- bool pci_subsystem_set;
+#endif /* CONFIG_PCI */
- char topology_shortname[32]; struct device *dev;
This looks bit weird to me, snd_soc_card is _generic_ struct which is not device specific in any way, and now you add fields for PCI, can't this somehow be done using drvdata or something?
You're right that it's a bit messy but if we use driver data then it becomes specific to a particular driver and there's a goal here to share with subfunction drivers. I was thinking we could refactor to a union or otherwise extend if we find other users with a similar need.
Yes, I was trying to avoid multiple custom ways of passing the same data around. A significant advantage of explicitly passing the SSID (if it's available) rather than a more abstract identifier (such as a char *) is that's it's very well defined exactly what a PCI SSID is so we know we can use it verbatim. A more abstract identifier creates an implied trust (or mistrust) between the machine driver and the component receiving it whether it's unique and in a useful format.
I could de-ugly it a bit by moving it out into a separate struct/union and having just a member of that struct type in snd_soc_card.
An alternative was to add a function like the existing snd_soc_component_set_whatever() family but that means adding a callback pointer to struct snd_soc_component_driver, which is creating more space overhead than one value in the snd_soc_card.
Pass the PCI SSID of the audio interface through to the machine driver. This allows the machine driver to use the SSID to uniquely identify the specific hardware configuration and apply any platform-specific configuration.
struct snd_sof_pdata is passed around inside the SOF code, but it then passes configuration information to the machine driver through struct snd_soc_acpi_mach and struct snd_soc_acpi_mach_params. So SSID information has been added to both snd_sof_pdata and snd_soc_acpi_mach_params.
PCI does not define 0x0000 as an invalid value so we can't use zero to indicate that the struct member was not written. Instead a flag is included to indicate that a value has been written to the subsystem_vendor and subsystem_device members.
sof_pci_probe() creates the struct snd_sof_pdata. It is passed a struct pci_dev so it can fill in the SSID value.
sof_machine_check() finds the appropriate struct snd_soc_acpi_mach. It copies the SSID information across to the struct snd_soc_acpi_mach_params. This done before calling any custom set_mach_params() so that it could be used by the set_mach_params() callback to apply variant params.
The machine driver receives the struct snd_soc_acpi_mach as its platform_data.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- include/sound/soc-acpi.h | 7 +++++++ include/sound/sof.h | 8 ++++++++ sound/soc/sof/sof-audio.c | 7 +++++++ sound/soc/sof/sof-pci-dev.c | 8 ++++++++ 4 files changed, 30 insertions(+)
diff --git a/include/sound/soc-acpi.h b/include/sound/soc-acpi.h index 6d31d535e8f6..23d6d6bfb073 100644 --- a/include/sound/soc-acpi.h +++ b/include/sound/soc-acpi.h @@ -68,6 +68,10 @@ static inline struct snd_soc_acpi_mach *snd_soc_acpi_codec_list(void *arg) * @i2s_link_mask: I2S/TDM links enabled on the board * @num_dai_drivers: number of elements in @dai_drivers * @dai_drivers: pointer to dai_drivers, used e.g. in nocodec mode + * @subsystem_vendor: optional PCI SSID vendor value + * @subsystem_device: optional PCI SSID device value + * @subsystem_id_set: true if a value has been written to + * subsystem_vendor and subsystem_device. */ struct snd_soc_acpi_mach_params { u32 acpi_ipc_irq_index; @@ -80,6 +84,9 @@ struct snd_soc_acpi_mach_params { u32 i2s_link_mask; u32 num_dai_drivers; struct snd_soc_dai_driver *dai_drivers; + unsigned short subsystem_vendor; + unsigned short subsystem_device; + bool subsystem_id_set; };
/** diff --git a/include/sound/sof.h b/include/sound/sof.h index d3c41f87ac31..51294f2ba302 100644 --- a/include/sound/sof.h +++ b/include/sound/sof.h @@ -64,6 +64,14 @@ struct snd_sof_pdata { const char *name; const char *platform;
+ /* + * PCI SSID. As PCI does not define 0 as invalid, the subsystem_id_set + * flag indicates that a value has been written to these members. + */ + unsigned short subsystem_vendor; + unsigned short subsystem_device; + bool subsystem_id_set; + struct device *dev;
/* diff --git a/sound/soc/sof/sof-audio.c b/sound/soc/sof/sof-audio.c index e7ef77012c35..9c2359d10ecf 100644 --- a/sound/soc/sof/sof-audio.c +++ b/sound/soc/sof/sof-audio.c @@ -1031,6 +1031,13 @@ int sof_machine_check(struct snd_sof_dev *sdev) mach = snd_sof_machine_select(sdev); if (mach) { sof_pdata->machine = mach; + + if (sof_pdata->subsystem_id_set) { + mach->mach_params.subsystem_vendor = sof_pdata->subsystem_vendor; + mach->mach_params.subsystem_device = sof_pdata->subsystem_device; + mach->mach_params.subsystem_id_set = true; + } + snd_sof_set_mach_params(mach, sdev); return 0; } diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c index f5ece43d0ec2..146d25983b08 100644 --- a/sound/soc/sof/sof-pci-dev.c +++ b/sound/soc/sof/sof-pci-dev.c @@ -214,6 +214,14 @@ int sof_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id) return ret;
sof_pdata->name = pci_name(pci); + + /* PCI defines a vendor ID of 0xFFFF as invalid. */ + if (pci->subsystem_vendor != 0xFFFF) { + sof_pdata->subsystem_vendor = pci->subsystem_vendor; + sof_pdata->subsystem_device = pci->subsystem_device; + sof_pdata->subsystem_id_set = true; + } + sof_pdata->desc = desc; sof_pdata->dev = dev;
If the PCI SSID has been set in the struct snd_soc_acpi_mach_params, copy this to struct snd_soc_card so that it can be used by other ASoC components.
This is important for components that must apply system-specific configuration.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/soc/intel/boards/sof_sdw.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c index 5a1c750e6ae6..961241100012 100644 --- a/sound/soc/intel/boards/sof_sdw.c +++ b/sound/soc/intel/boards/sof_sdw.c @@ -1924,6 +1924,12 @@ static int mc_probe(struct platform_device *pdev) for (i = 0; i < ARRAY_SIZE(codec_info_list); i++) codec_info_list[i].amp_num = 0;
+ if (mach->mach_params.subsystem_id_set) { + snd_soc_card_set_pci_ssid(card, + mach->mach_params.subsystem_vendor, + mach->mach_params.subsystem_device); + } + ret = sof_card_dai_links_create(card); if (ret < 0) return ret;
If the driver properties do not define a cirrus,firmware-uid try to get the PCI SSID as the UID.
On PCI-based systems the PCI SSID is used to uniquely identify the specific sound hardware. This is the standard mechanism for x86 systems and is the way to get a unique system identifier for systems that use the CS35L56 on SoundWire.
For non-SoundWire systems there is no Windows equivalent of the ASoC driver in I2C/SPI mode. These would be:
1. HDA systems, which are handled by the HDA subsystem. 2. Linux-specific systems. 3. Composite devices where the cs35l56 is not present in ACPI and is configured using software nodes.
Case 2 can use the firmware-uid property, though the PCI SSID is supported as an alternative, as it is the standard PCI mechanism.
Case 3 is a SoundWire system where some other codec is the SoundWire bridge device and CS35L56 is not listed in ACPI. As these are SoundWire systems they will normally use the PCI SSID.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/soc/codecs/cs35l56.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c index f2e7c6d0be46..e6e366333a47 100644 --- a/sound/soc/codecs/cs35l56.c +++ b/sound/soc/codecs/cs35l56.c @@ -772,9 +772,20 @@ static int cs35l56_component_probe(struct snd_soc_component *component) { struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(component); struct dentry *debugfs_root = component->debugfs_root; + unsigned short vendor, device;
BUILD_BUG_ON(ARRAY_SIZE(cs35l56_tx_input_texts) != ARRAY_SIZE(cs35l56_tx_input_values));
+ if (!cs35l56->dsp.system_name && + (snd_soc_card_get_pci_ssid(component->card, &vendor, &device) == 0)) { + cs35l56->dsp.system_name = devm_kasprintf(cs35l56->base.dev, + GFP_KERNEL, + "%04x%04x", + vendor, device); + if (!cs35l56->dsp.system_name) + return -ENOMEM; + } + if (!wait_for_completion_timeout(&cs35l56->init_completion, msecs_to_jiffies(5000))) { dev_err(cs35l56->base.dev, "%s: init_completion timed out\n", __func__);
On 9/12/23 12:32, Richard Fitzgerald wrote:
The PCI device registers contain a subsystem ID (SSID), that is separate from the silicon ID. The PCI specification defines it thus:
"They provide a mechanism for board vendors to distiguish their boards from one another even thought the boards may have the same PCI controller on them."
This allows the driver for the silicon part to apply board-speficic settings based on this SSID.
The CS35L56 driver uses this to select the correct firmware file for the board. The actual ID is part of the PCI register set of the host audio interface so this set of patches includes extracting the SSID from the Intel audio controller and passing it to the machine driver and then to ASoC components. Other PCI audio controllers will have the same SSID registers, so can use the same mechanism to pass the SSID.
Richard Fitzgerald (4): ASoC: soc-card: Add storage for PCI SSID ASoC: SOF: Pass PCI SSID to machine driver ASoC: Intel: sof_sdw: Copy PCI SSID to struct snd_soc_card ASoC: cs35l56: Use PCI SSID as the firmware UID
for the series
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
include/sound/soc-acpi.h | 7 ++++++ include/sound/soc-card.h | 37 ++++++++++++++++++++++++++++++++ include/sound/soc.h | 11 ++++++++++ include/sound/sof.h | 8 +++++++ sound/soc/codecs/cs35l56.c | 11 ++++++++++ sound/soc/intel/boards/sof_sdw.c | 6 ++++++ sound/soc/sof/sof-audio.c | 7 ++++++ sound/soc/sof/sof-pci-dev.c | 8 +++++++ 8 files changed, 95 insertions(+)
On Tue, 12 Sep 2023 17:32:03 +0100, Richard Fitzgerald wrote:
The PCI device registers contain a subsystem ID (SSID), that is separate from the silicon ID. The PCI specification defines it thus:
"They provide a mechanism for board vendors to distiguish their boards from one another even thought the boards may have the same PCI controller on them."
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/4] ASoC: soc-card: Add storage for PCI SSID commit: 47f56e38a199bd45514b8e0142399cba4feeaf1a [2/4] ASoC: SOF: Pass PCI SSID to machine driver commit: ba2de401d32625fe538d3f2c00ca73740dd2d516 [3/4] ASoC: Intel: sof_sdw: Copy PCI SSID to struct snd_soc_card commit: d8b387544ff4d02eda1d1839a0c601de4b037c33 [4/4] ASoC: cs35l56: Use PCI SSID as the firmware UID commit: 1a1c3d794ef65ef2978c5e65e1aed3fe6f014e90
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (4)
-
Amadeusz Sławiński -
Mark Brown -
Pierre-Louis Bossart -
Richard Fitzgerald