[PATCH 00/18] ALSA: Various fixes for Cirrus Logic CS35L56 support
This chain of patches fixes various things that were undocumented, unknown or uncertain when the original driver code was written. And also a few things that were just bugs.
The HDA patches have dependencies on the ASoC patches, except for the final patch that removes a bogus test stub function.
Richard Fitzgerald (18): ASoC: wm_adsp: Fix firmware file search order ASoC: wm_adsp: Don't overwrite fwf_name with the default ASoC: cs35l56: cs35l56_component_remove() must clear cs35l56->component ASoC: cs35l56: cs35l56_component_remove() must clean up wm_adsp ASoC: cs35l56: Don't add the same register patch multiple times ASoC: cs35l56: Remove buggy checks from cs35l56_is_fw_reload_needed() ASoC: cs35l56: Fix to ensure ASP1 registers match cache ASoC: cs35l56: Fix default SDW TX mixer registers ALSA: hda: cs35l56: Initialize all ASP1 registers ASoC: cs35l56: Fix for initializing ASP1 mixer registers ASoC: cs35l56: Fix misuse of wm_adsp 'part' string for silicon revision ASoC: cs35l56: Firmware file must match the version of preloaded firmware ASoC: cs35l56: Load tunings for the correct speaker models ASoC: cs35l56: Allow more time for firmware to boot ALSA: hda: cs35l56: Fix order of searching for firmware files ALSA: hda: cs35l56: Fix filename string field layout ALSA: hda: cs35l56: Firmware file must match the version of preloaded firmware ALSA: hda: cs35l56: Remove unused test stub function
include/sound/cs35l56.h | 7 +- sound/pci/hda/cs35l56_hda.c | 138 ++++++++------ sound/soc/codecs/cs35l56-shared.c | 140 ++++++++++++-- sound/soc/codecs/cs35l56.c | 307 +++++++++++++++++++++++++----- sound/soc/codecs/cs35l56.h | 2 + sound/soc/codecs/wm_adsp.c | 73 +++---- 6 files changed, 498 insertions(+), 169 deletions(-)
Check for the cases of system-specific bin file without a wmfw before falling back to looking for a generic wmfw.
All system-specific options should be tried before falling back to loading a generic wmfw/bin. With the original code, the presence of a fallback generic wmfw on the filesystem would prevent using a system-specific tuning with a ROM firmware.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 0e7d82cbea8b ("ASoC: wm_adsp: Add support for loading bin files without wmfw") --- sound/soc/codecs/wm_adsp.c | 44 ++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 25 deletions(-)
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index c01e31175015..bd60ceebb6a9 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -823,6 +823,23 @@ static int wm_adsp_request_firmware_files(struct wm_adsp *dsp, } }
+ /* Check system-specific bin without wmfw before falling back to generic */ + if (dsp->wmfw_optional && system_name) { + if (asoc_component_prefix) + wm_adsp_request_firmware_file(dsp, coeff_firmware, coeff_filename, + cirrus_dir, system_name, + asoc_component_prefix, "bin"); + + if (!*coeff_firmware) + wm_adsp_request_firmware_file(dsp, coeff_firmware, coeff_filename, + cirrus_dir, system_name, + NULL, "bin"); + + if (*coeff_firmware) + return 0; + } + + /* Check legacy location */ if (!wm_adsp_request_firmware_file(dsp, wmfw_firmware, wmfw_filename, "", NULL, NULL, "wmfw")) { wm_adsp_request_firmware_file(dsp, coeff_firmware, coeff_filename, @@ -830,38 +847,15 @@ static int wm_adsp_request_firmware_files(struct wm_adsp *dsp, return 0; }
+ /* Fall back to generic wmfw and optional matching bin */ ret = wm_adsp_request_firmware_file(dsp, wmfw_firmware, wmfw_filename, cirrus_dir, NULL, NULL, "wmfw"); - if (!ret) { + if (!ret || dsp->wmfw_optional) { wm_adsp_request_firmware_file(dsp, coeff_firmware, coeff_filename, cirrus_dir, NULL, NULL, "bin"); return 0; }
- if (dsp->wmfw_optional) { - if (system_name) { - if (asoc_component_prefix) - wm_adsp_request_firmware_file(dsp, coeff_firmware, coeff_filename, - cirrus_dir, system_name, - asoc_component_prefix, "bin"); - - if (!*coeff_firmware) - wm_adsp_request_firmware_file(dsp, coeff_firmware, coeff_filename, - cirrus_dir, system_name, - NULL, "bin"); - } - - if (!*coeff_firmware) - wm_adsp_request_firmware_file(dsp, coeff_firmware, coeff_filename, - "", NULL, NULL, "bin"); - - if (!*coeff_firmware) - wm_adsp_request_firmware_file(dsp, coeff_firmware, coeff_filename, - cirrus_dir, NULL, NULL, "bin"); - - return 0; - } - adsp_err(dsp, "Failed to request firmware <%s>%s-%s-%s<-%s<%s>>.wmfw\n", cirrus_dir, dsp->part, dsp->fwf_name, wm_adsp_fw[dsp->fw].file, system_name, asoc_component_prefix);
There's no need to overwrite fwf_name with a kstrdup() of the cs_dsp part name. It is trivial to select either fwf_name or cs_dsp.part as the string to use when building the filename in wm_adsp_request_firmware_file().
This leaves fwf_name entirely owned by the codec driver.
It also avoids problems with freeing the pointer. With the original code fwf_name was either a pointer owned by the codec driver, or a kstrdup() created by wm_adsp. This meant wm_adsp must free it if it set it, but not if the codec driver set it. The code was handling this by using devm_kstrdup(). But there is no absolute requirement that wm_adsp_common_init() must be called from probe(), so this was a pseudo-memory leak - each new call to wm_adsp_common_init() would allocate another block of memory but these would only be freed if the owning codec driver was removed.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/soc/codecs/wm_adsp.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index bd60ceebb6a9..36ea0dcdc7ab 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -739,19 +739,25 @@ static int wm_adsp_request_firmware_file(struct wm_adsp *dsp, const char *filetype) { struct cs_dsp *cs_dsp = &dsp->cs_dsp; + const char *fwf; char *s, c; int ret = 0;
+ if (dsp->fwf_name) + fwf = dsp->fwf_name; + else + fwf = dsp->cs_dsp.name; + if (system_name && asoc_component_prefix) *filename = kasprintf(GFP_KERNEL, "%s%s-%s-%s-%s-%s.%s", dir, dsp->part, - dsp->fwf_name, wm_adsp_fw[dsp->fw].file, system_name, + fwf, wm_adsp_fw[dsp->fw].file, system_name, asoc_component_prefix, filetype); else if (system_name) *filename = kasprintf(GFP_KERNEL, "%s%s-%s-%s-%s.%s", dir, dsp->part, - dsp->fwf_name, wm_adsp_fw[dsp->fw].file, system_name, + fwf, wm_adsp_fw[dsp->fw].file, system_name, filetype); else - *filename = kasprintf(GFP_KERNEL, "%s%s-%s-%s.%s", dir, dsp->part, dsp->fwf_name, + *filename = kasprintf(GFP_KERNEL, "%s%s-%s-%s.%s", dir, dsp->part, fwf, wm_adsp_fw[dsp->fw].file, filetype);
if (*filename == NULL) @@ -857,29 +863,18 @@ static int wm_adsp_request_firmware_files(struct wm_adsp *dsp, }
adsp_err(dsp, "Failed to request firmware <%s>%s-%s-%s<-%s<%s>>.wmfw\n", - cirrus_dir, dsp->part, dsp->fwf_name, wm_adsp_fw[dsp->fw].file, - system_name, asoc_component_prefix); + cirrus_dir, dsp->part, + dsp->fwf_name ? dsp->fwf_name : dsp->cs_dsp.name, + wm_adsp_fw[dsp->fw].file, system_name, asoc_component_prefix);
return -ENOENT; }
static int wm_adsp_common_init(struct wm_adsp *dsp) { - char *p; - INIT_LIST_HEAD(&dsp->compr_list); INIT_LIST_HEAD(&dsp->buffer_list);
- if (!dsp->fwf_name) { - p = devm_kstrdup(dsp->cs_dsp.dev, dsp->cs_dsp.name, GFP_KERNEL); - if (!p) - return -ENOMEM; - - dsp->fwf_name = p; - for (; *p != 0; ++p) - *p = tolower(*p); - } - return 0; }
The cs35l56->component pointer is used by the suspend-resume handling to know whether the driver is fully instantiated. This is to prevent it queuing dsp_work which would result in calling wm_adsp when the driver is not an instantiated ASoC component. So this pointer must be cleared by cs35l56_component_remove().
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: e49611252900 ("ASoC: cs35l56: Add driver for Cirrus Logic CS35L56") --- sound/soc/codecs/cs35l56.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c index 45b4de3eff94..09944db4db30 100644 --- a/sound/soc/codecs/cs35l56.c +++ b/sound/soc/codecs/cs35l56.c @@ -809,6 +809,8 @@ static void cs35l56_component_remove(struct snd_soc_component *component) struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(component);
cancel_work_sync(&cs35l56->dsp_work); + + cs35l56->component = NULL; }
static int cs35l56_set_bias_level(struct snd_soc_component *component,
cs35l56_component_remove() must call wm_adsp_power_down() and wm_adsp2_component_remove().
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: e49611252900 ("ASoC: cs35l56: Add driver for Cirrus Logic CS35L56") --- sound/soc/codecs/cs35l56.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c index 09944db4db30..491da77112c3 100644 --- a/sound/soc/codecs/cs35l56.c +++ b/sound/soc/codecs/cs35l56.c @@ -810,6 +810,11 @@ static void cs35l56_component_remove(struct snd_soc_component *component)
cancel_work_sync(&cs35l56->dsp_work);
+ if (cs35l56->dsp.cs_dsp.booted) + wm_adsp_power_down(&cs35l56->dsp); + + wm_adsp2_component_remove(&cs35l56->dsp, component); + cs35l56->component = NULL; }
Move the call to cs35l56_set_patch() earlier in cs35l56_init() so that it only adds the register patch on first-time initialization.
The call was after the post_soft_reset label, so every time this function was run to re-initialize the hardware after a reset it would call regmap_register_patch() and add the same reg_sequence again.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 898673b905b9 ("ASoC: cs35l56: Move shared data into a common data structure") --- sound/soc/codecs/cs35l56.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c index 491da77112c3..ea5d2b2eb82a 100644 --- a/sound/soc/codecs/cs35l56.c +++ b/sound/soc/codecs/cs35l56.c @@ -1159,6 +1159,10 @@ int cs35l56_init(struct cs35l56_private *cs35l56) if (ret < 0) return ret;
+ ret = cs35l56_set_patch(&cs35l56->base); + if (ret) + return ret; + /* Populate the DSP information with the revision and security state */ cs35l56->dsp.part = devm_kasprintf(cs35l56->base.dev, GFP_KERNEL, "cs35l56%s-%02x", cs35l56->base.secured ? "s" : "", cs35l56->base.rev); @@ -1197,10 +1201,6 @@ int cs35l56_init(struct cs35l56_private *cs35l56) if (ret) return ret;
- ret = cs35l56_set_patch(&cs35l56->base); - if (ret) - return ret; - /* Registers could be dirty after soft reset or SoundWire enumeration */ regcache_sync(cs35l56->base.regmap);
Remove the check of fw_patched from cs35l56_is_fw_reload_needed(). Also remove the redundant check for control of the reset GPIO.
The fw_patched flag is set when cs35l56_dsp_work() has completed its steps to download firmware and power-up wm_adsp. There was a check in cs35l56_is_fw_reload_needed() to make a quick exit of 'false' if !fw_patched. The original idea was that the system might be suspended before the driver has ever made any attempt to download firmware, and in that case the driver doesn't need to return to a patched state because it was never in a patched state.
This check of fw_patched is buggy because it prevented ever recovering from a failed patch. If a previous attempt to patch and reboot the silicon had failed it would leave fw_patched==false. This would mean the driver never attempted another download even though the fault may have been cleared (by a hard reset, for example).
It is also a redundant check because the calling code already makes a quick exit if cs35l56_component_probe() has not been called, which deals with the original intent of this check but in a safer way.
The check for reset GPIO is redundant: if the silicon was hard-reset the FIRMWARE_MISSING flag will be 1. But this check created an expectation that the suspend/resume code toggles reset. This can't easily be protected against accidental code breakage. The only reason for the check was to skip runtime-resuming the driver to read the PROTECTION_STATUS register when it already knows it reset the silicon. But in that case the driver will have to be runtime-resumed to do the firmware download. So it created an assumption for no benefit.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 8a731fd37f8b ("ASoC: cs35l56: Move utility functions to shared file") --- sound/soc/codecs/cs35l56-shared.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/sound/soc/codecs/cs35l56-shared.c b/sound/soc/codecs/cs35l56-shared.c index 953ba066bab1..0cd572de73a9 100644 --- a/sound/soc/codecs/cs35l56-shared.c +++ b/sound/soc/codecs/cs35l56-shared.c @@ -400,17 +400,6 @@ int cs35l56_is_fw_reload_needed(struct cs35l56_base *cs35l56_base) unsigned int val; int ret;
- /* Nothing to re-patch if we haven't done any patching yet. */ - if (!cs35l56_base->fw_patched) - return false; - - /* - * If we have control of RESET we will have asserted it so the firmware - * will need re-patching. - */ - if (cs35l56_base->reset_gpio) - return true; - /* * In secure mode FIRMWARE_MISSING is cleared by the BIOS loader so * can't be used here to test for memory retention.
Add a dummy SUPPLY widget connected to the ASP that forces the chip registers to match the regmap cache when the ASP is powered-up.
On a SoundWire system the ASP is free for use as a chip-to-chip interconnect. This can be either for the firmware on multiple CS35L56 to share reference audio; or as a bridge to another device. If it is a firmware interconnect it is owned by the firmware and the Linux driver should avoid writing the registers. However. If it is a bridge then Linux may take over and handle it as a normal codec-to-codec link.
CS35L56 is designed for SDCA and a generic SDCA driver would know nothing about these chip-specific registers. So if the ASP is being used on a SoundWire system the firmware sets up the ASP registers. This means that we can't assume the default state of the ASP registers. But we don't know the initial state that the firmware set them to until after the firmware has been downloaded and booted, which can take several seconds when downloading multiple amps.
To avoid blocking probe() for several seconds waiting for the firmware, the silicon defaults are assumed. This allows the machine driver to setup the ASP configuration during probe() without being blocked. If the ASP is hooked up and used, the SUPPLY widget ensures that the chip registers match what was configured in the regmap cache.
If the machine driver does not hook up the ASP, it is assumed that it won't call any functions to configure the ASP DAI. Therefore the regmap cache will be clean for these registers so a regcache_sync() will not overwrite the chip registers. If the DAI is not hooked up, the dummy SUPPLY widget will not be invoked so it will never force-overwrite the chip registers.
Backport note: This won't apply cleanly to kernels older than v6.6.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: e49611252900 ("ASoC: cs35l56: Add driver for Cirrus Logic CS35L56") --- include/sound/cs35l56.h | 1 + sound/soc/codecs/cs35l56-shared.c | 41 +++++++++++++++++++++++++++++++ sound/soc/codecs/cs35l56.c | 21 ++++++++++++++++ 3 files changed, 63 insertions(+)
diff --git a/include/sound/cs35l56.h b/include/sound/cs35l56.h index 8c18e8b6d27d..4db36c893d9d 100644 --- a/include/sound/cs35l56.h +++ b/include/sound/cs35l56.h @@ -272,6 +272,7 @@ extern const char * const cs35l56_tx_input_texts[CS35L56_NUM_INPUT_SRC]; extern const unsigned int cs35l56_tx_input_values[CS35L56_NUM_INPUT_SRC];
int cs35l56_set_patch(struct cs35l56_base *cs35l56_base); +int cs35l56_force_sync_asp1_registers_from_cache(struct cs35l56_base *cs35l56_base); int cs35l56_mbox_send(struct cs35l56_base *cs35l56_base, unsigned int command); int cs35l56_firmware_shutdown(struct cs35l56_base *cs35l56_base); int cs35l56_wait_for_firmware_boot(struct cs35l56_base *cs35l56_base); diff --git a/sound/soc/codecs/cs35l56-shared.c b/sound/soc/codecs/cs35l56-shared.c index 0cd572de73a9..35789ffc63af 100644 --- a/sound/soc/codecs/cs35l56-shared.c +++ b/sound/soc/codecs/cs35l56-shared.c @@ -195,6 +195,47 @@ static bool cs35l56_volatile_reg(struct device *dev, unsigned int reg) } }
+/* + * The firmware boot sequence can overwrite the ASP1 config registers so that + * they don't match regmap's view of their values. Rewrite the values from the + * regmap cache into the hardware registers. + */ +int cs35l56_force_sync_asp1_registers_from_cache(struct cs35l56_base *cs35l56_base) +{ + struct reg_sequence asp1_regs[] = { + { .reg = CS35L56_ASP1_ENABLES1 }, + { .reg = CS35L56_ASP1_CONTROL1 }, + { .reg = CS35L56_ASP1_CONTROL2 }, + { .reg = CS35L56_ASP1_CONTROL3 }, + { .reg = CS35L56_ASP1_FRAME_CONTROL1 }, + { .reg = CS35L56_ASP1_FRAME_CONTROL5 }, + { .reg = CS35L56_ASP1_DATA_CONTROL1 }, + { .reg = CS35L56_ASP1_DATA_CONTROL5 }, + }; + int i, ret; + + /* Read values from regmap cache into a write sequence */ + for (i = 0; i < ARRAY_SIZE(asp1_regs); ++i) { + ret = regmap_read(cs35l56_base->regmap, asp1_regs[i].reg, &asp1_regs[i].def); + if (ret) + goto err; + } + + /* Write the values cache-bypassed so that they will be written to silicon */ + ret = regmap_multi_reg_write_bypassed(cs35l56_base->regmap, asp1_regs, + ARRAY_SIZE(asp1_regs)); + if (ret) + goto err; + + return 0; + +err: + dev_err(cs35l56_base->dev, "Failed to sync ASP1 registers: %d\n", ret); + + return ret; +} +EXPORT_SYMBOL_NS_GPL(cs35l56_force_sync_asp1_registers_from_cache, SND_SOC_CS35L56_SHARED); + int cs35l56_mbox_send(struct cs35l56_base *cs35l56_base, unsigned int command) { unsigned int val; diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c index ea5d2b2eb82a..41aa79848b15 100644 --- a/sound/soc/codecs/cs35l56.c +++ b/sound/soc/codecs/cs35l56.c @@ -148,6 +148,21 @@ static SOC_VALUE_ENUM_SINGLE_DECL(cs35l56_sdw1tx4_enum, static const struct snd_kcontrol_new sdw1_tx4_mux = SOC_DAPM_ENUM("SDW1TX4 SRC", cs35l56_sdw1tx4_enum);
+static int cs35l56_asp1_cfg_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); + struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(component); + + switch (event) { + case SND_SOC_DAPM_PRE_PMU: + /* Override register values set by firmware boot */ + return cs35l56_force_sync_asp1_registers_from_cache(&cs35l56->base); + default: + return 0; + } +} + static int cs35l56_play_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event) { @@ -184,6 +199,9 @@ static const struct snd_soc_dapm_widget cs35l56_dapm_widgets[] = { SND_SOC_DAPM_REGULATOR_SUPPLY("VDD_B", 0, 0), SND_SOC_DAPM_REGULATOR_SUPPLY("VDD_AMP", 0, 0),
+ SND_SOC_DAPM_SUPPLY("ASP1 CFG", SND_SOC_NOPM, 0, 0, cs35l56_asp1_cfg_event, + SND_SOC_DAPM_PRE_PMU), + SND_SOC_DAPM_SUPPLY("PLAY", SND_SOC_NOPM, 0, 0, cs35l56_play_event, SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
@@ -251,6 +269,9 @@ static const struct snd_soc_dapm_route cs35l56_audio_map[] = { { "AMP", NULL, "VDD_B" }, { "AMP", NULL, "VDD_AMP" },
+ { "ASP1 Playback", NULL, "ASP1 CFG" }, + { "ASP1 Capture", NULL, "ASP1 CFG" }, + { "ASP1 Playback", NULL, "PLAY" }, { "SDW1 Playback", NULL, "PLAY" },
Patch the SDW TX mixer registers to silicon defaults.
CS35L56 is designed for SDCA and a generic SDCA driver would know nothing about these chip-specific registers. So the firmware sets up the SDW TX mixer registers to whatever audio is relevant on a specific system.
This means that the driver cannot assume the initial values of these registers. But Linux has ALSA controls to configure routing, so the registers can be patched to silicon default and the ALSA controls used to select what audio to feed back to the host capture path.
Backport note: This won't apply to kernels older than v6.6.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: e49611252900 ("ASoC: cs35l56: Add driver for Cirrus Logic CS35L56") --- sound/soc/codecs/cs35l56-shared.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/sound/soc/codecs/cs35l56-shared.c b/sound/soc/codecs/cs35l56-shared.c index 35789ffc63af..a812abf90836 100644 --- a/sound/soc/codecs/cs35l56-shared.c +++ b/sound/soc/codecs/cs35l56-shared.c @@ -12,6 +12,15 @@ #include "cs35l56.h"
static const struct reg_sequence cs35l56_patch[] = { + /* + * Firmware can change these to non-defaults to satisfy SDCA. + * Ensure that they are at known defaults. + */ + { CS35L56_SWIRE_DP3_CH1_INPUT, 0x00000018 }, + { CS35L56_SWIRE_DP3_CH2_INPUT, 0x00000019 }, + { CS35L56_SWIRE_DP3_CH3_INPUT, 0x00000029 }, + { CS35L56_SWIRE_DP3_CH4_INPUT, 0x00000028 }, + /* These are not reset by a soft-reset, so patch to defaults. */ { CS35L56_MAIN_RENDER_USER_MUTE, 0x00000000 }, { CS35L56_MAIN_RENDER_USER_VOLUME, 0x00000000 },
On 1/29/24 17:27, Richard Fitzgerald wrote:
Patch the SDW TX mixer registers to silicon defaults.
CS35L56 is designed for SDCA and a generic SDCA driver would know nothing about these chip-specific registers. So the firmware sets up the SDW TX mixer registers to whatever audio is relevant on a specific system.
This means that the driver cannot assume the initial values of these registers. But Linux has ALSA controls to configure routing, so the registers can be patched to silicon default and the ALSA controls used to select what audio to feed back to the host capture path.
humm, which has the precedence then? a) the values set by firmware b) the default values set by the driver?
Also if the firmware touches those registers shouldn't they be marked as 'volatile'?
Backport note: This won't apply to kernels older than v6.6.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: e49611252900 ("ASoC: cs35l56: Add driver for Cirrus Logic CS35L56")
sound/soc/codecs/cs35l56-shared.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/sound/soc/codecs/cs35l56-shared.c b/sound/soc/codecs/cs35l56-shared.c index 35789ffc63af..a812abf90836 100644 --- a/sound/soc/codecs/cs35l56-shared.c +++ b/sound/soc/codecs/cs35l56-shared.c @@ -12,6 +12,15 @@ #include "cs35l56.h"
static const struct reg_sequence cs35l56_patch[] = {
- /*
* Firmware can change these to non-defaults to satisfy SDCA.* Ensure that they are at known defaults.*/- { CS35L56_SWIRE_DP3_CH1_INPUT, 0x00000018 },
- { CS35L56_SWIRE_DP3_CH2_INPUT, 0x00000019 },
- { CS35L56_SWIRE_DP3_CH3_INPUT, 0x00000029 },
- { CS35L56_SWIRE_DP3_CH4_INPUT, 0x00000028 },
- /* These are not reset by a soft-reset, so patch to defaults. */ { CS35L56_MAIN_RENDER_USER_MUTE, 0x00000000 }, { CS35L56_MAIN_RENDER_USER_VOLUME, 0x00000000 },
On 29/01/2024 17:15, Pierre-Louis Bossart wrote:
On 1/29/24 17:27, Richard Fitzgerald wrote:
Patch the SDW TX mixer registers to silicon defaults.
CS35L56 is designed for SDCA and a generic SDCA driver would know nothing about these chip-specific registers. So the firmware sets up the SDW TX mixer registers to whatever audio is relevant on a specific system.
This means that the driver cannot assume the initial values of these registers. But Linux has ALSA controls to configure routing, so the registers can be patched to silicon default and the ALSA controls used to select what audio to feed back to the host capture path.
humm, which has the precedence then? a) the values set by firmware b) the default values set by the driver?
Also if the firmware touches those registers shouldn't they be marked as 'volatile'?
The firmware was designed to work with Windows, so it looks a bit strange if you are coming at it from ALSA. There's not really any defined 'precedence'. The firmware will setup the feedback monitor paths to something that satisfies SDCA and Windows expectations.
We don't care about that in Linux, the firmware on the Intel DSP probably isn't running the same algorithms for Linux, and we have ALSA controls to configure those paths. So we patch the mixers back to their silicon defaults and take over complete control of them.
The firmware only writes them during its power-up sequence so they will only change when we are rebooting the firmware or coming out of low-power standby, which is under the control of the driver. When that happens regmap will re-apply the patch and then sync up the registers again. The firmware won't touch them after boot, so we can avoid having to mark them volatile (which would mean implementing our own manual caching of the settings).
Backport note: This won't apply to kernels older than v6.6.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: e49611252900 ("ASoC: cs35l56: Add driver for Cirrus Logic CS35L56")
sound/soc/codecs/cs35l56-shared.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/sound/soc/codecs/cs35l56-shared.c b/sound/soc/codecs/cs35l56-shared.c index 35789ffc63af..a812abf90836 100644 --- a/sound/soc/codecs/cs35l56-shared.c +++ b/sound/soc/codecs/cs35l56-shared.c @@ -12,6 +12,15 @@ #include "cs35l56.h"
static const struct reg_sequence cs35l56_patch[] = {
- /*
* Firmware can change these to non-defaults to satisfy SDCA.* Ensure that they are at known defaults.*/- { CS35L56_SWIRE_DP3_CH1_INPUT, 0x00000018 },
- { CS35L56_SWIRE_DP3_CH2_INPUT, 0x00000019 },
- { CS35L56_SWIRE_DP3_CH3_INPUT, 0x00000029 },
- { CS35L56_SWIRE_DP3_CH4_INPUT, 0x00000028 },
- /* These are not reset by a soft-reset, so patch to defaults. */ { CS35L56_MAIN_RENDER_USER_MUTE, 0x00000000 }, { CS35L56_MAIN_RENDER_USER_VOLUME, 0x00000000 },
CS35L56 is designed for SDCA and a generic SDCA driver would know nothing about these chip-specific registers. So the firmware sets up the SDW TX mixer registers to whatever audio is relevant on a specific system.
This means that the driver cannot assume the initial values of these registers. But Linux has ALSA controls to configure routing, so the registers can be patched to silicon default and the ALSA controls used to select what audio to feed back to the host capture path.
humm, which has the precedence then? a) the values set by firmware b) the default values set by the driver?
Also if the firmware touches those registers shouldn't they be marked as 'volatile'?
The firmware was designed to work with Windows, so it looks a bit strange if you are coming at it from ALSA. There's not really any defined 'precedence'. The firmware will setup the feedback monitor paths to something that satisfies SDCA and Windows expectations.
We don't care about that in Linux, the firmware on the Intel DSP probably isn't running the same algorithms for Linux, and we have ALSA controls to configure those paths. So we patch the mixers back to their silicon defaults and take over complete control of them.
The firmware only writes them during its power-up sequence so they will only change when we are rebooting the firmware or coming out of low-power standby, which is under the control of the driver. When that happens regmap will re-apply the patch and then sync up the registers again. The firmware won't touch them after boot, so we can avoid having to mark them volatile (which would mean implementing our own manual caching of the settings).
ok, thanks for the explanations.
Add ASP1_FRAME_CONTROL1, ASP1_FRAME_CONTROL5 and the ASP1_TX?_INPUT registers to the sequence used to initialize the ASP configuration. Write this sequence to the cache and directly to the registers to ensure that they match.
A system-specific firmware can patch these registers to values that are not the silicon default, so that the CS35L56 boots already in the configuration used by Windows or by "driverless" Windows setups such as factory tuning.
These may not match how Linux is configuring the HDA codec. And anyway on Linux the ALSA controls are used to configure routing options.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 73cfbfa9caea ("ALSA: hda/cs35l56: Add driver for Cirrus Logic CS35L56 amplifier") --- sound/pci/hda/cs35l56_hda.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index b61e1de8c4bf..f22bcb104a4e 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -30,14 +30,23 @@ * ASP1_RX_WL = 24 bits per sample * ASP1_TX_WL = 24 bits per sample * ASP1_RXn_EN 1..3 and ASP1_TXn_EN 1..4 disabled + * + * Override any Windows-specific mixer settings applied by the firmware. */ static const struct reg_sequence cs35l56_hda_dai_config[] = { { CS35L56_ASP1_CONTROL1, 0x00000021 }, { CS35L56_ASP1_CONTROL2, 0x20200200 }, { CS35L56_ASP1_CONTROL3, 0x00000003 }, + { CS35L56_ASP1_FRAME_CONTROL1, 0x03020100 }, + { CS35L56_ASP1_FRAME_CONTROL5, 0x00020100 }, { CS35L56_ASP1_DATA_CONTROL5, 0x00000018 }, { CS35L56_ASP1_DATA_CONTROL1, 0x00000018 }, { CS35L56_ASP1_ENABLES1, 0x00000000 }, + { CS35L56_ASP1TX1_INPUT, 0x00000018 }, + { CS35L56_ASP1TX2_INPUT, 0x00000019 }, + { CS35L56_ASP1TX3_INPUT, 0x00000020 }, + { CS35L56_ASP1TX4_INPUT, 0x00000028 }, + };
static void cs35l56_hda_play(struct cs35l56_hda *cs35l56) @@ -133,6 +142,10 @@ static int cs35l56_hda_runtime_resume(struct device *dev) } }
+ ret = cs35l56_force_sync_asp1_registers_from_cache(&cs35l56->base); + if (ret) + goto err; + return 0;
err: @@ -976,6 +989,9 @@ int cs35l56_hda_common_probe(struct cs35l56_hda *cs35l56, int id)
regmap_multi_reg_write(cs35l56->base.regmap, cs35l56_hda_dai_config, ARRAY_SIZE(cs35l56_hda_dai_config)); + ret = cs35l56_force_sync_asp1_registers_from_cache(&cs35l56->base); + if (ret) + goto err;
/* * By default only enable one ASP1TXn, where n=amplifier index,
Defer initializing the state of the ASP1 mixer registers until the firmware has been downloaded and rebooted.
On a SoundWire system the ASP is free for use as a chip-to-chip interconnect. This can be either for the firmware on multiple CS35L56 to share reference audio; or as a bridge to another device. If it is a firmware interconnect it is owned by the firmware and the Linux driver should avoid writing the registers. However, if it is a bridge then Linux may take over and handle it as a normal codec-to-codec link. Even if the ASP is used as a firmware-firmware interconnect it is useful to have ALSA controls for the ASP mixer. They are at least useful for debugging.
CS35L56 is designed for SDCA and a generic SDCA driver would know nothing about these chip-specific registers. So if the ASP is being used on a SoundWire system the firmware sets up the ASP mixer registers. This means that we can't assume the default state of these registers. But we don't know the initial state that the firmware set them to until after the firmware has been downloaded and booted, which can take several seconds when downloading multiple amps.
DAPM normally reads the initial state of mux registers during probe() but this would mean blocking probe() for several seconds until the firmware has initialized them. To avoid this, the mixer muxes are set SND_SOC_NOPM to prevent DAPM trying to read the register state. Custom get/set callbacks are implemented for ALSA control access, and these can safely block waiting for the firmware download.
After the firmware download has completed, the state of the mux registers is known so a work job is queued to call snd_soc_dapm_mux_update_power() on each of the mux widgets.
Backport note: This won't apply cleanly to kernels older than v6.6.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: e49611252900 ("ASoC: cs35l56: Add driver for Cirrus Logic CS35L56") --- sound/soc/codecs/cs35l56-shared.c | 7 +- sound/soc/codecs/cs35l56.c | 172 +++++++++++++++++++++++++++--- sound/soc/codecs/cs35l56.h | 1 + 3 files changed, 163 insertions(+), 17 deletions(-)
diff --git a/sound/soc/codecs/cs35l56-shared.c b/sound/soc/codecs/cs35l56-shared.c index a812abf90836..9a70db0fa418 100644 --- a/sound/soc/codecs/cs35l56-shared.c +++ b/sound/soc/codecs/cs35l56-shared.c @@ -43,10 +43,9 @@ static const struct reg_default cs35l56_reg_defaults[] = { { CS35L56_ASP1_FRAME_CONTROL5, 0x00020100 }, { CS35L56_ASP1_DATA_CONTROL1, 0x00000018 }, { CS35L56_ASP1_DATA_CONTROL5, 0x00000018 }, - { CS35L56_ASP1TX1_INPUT, 0x00000018 }, - { CS35L56_ASP1TX2_INPUT, 0x00000019 }, - { CS35L56_ASP1TX3_INPUT, 0x00000020 }, - { CS35L56_ASP1TX4_INPUT, 0x00000028 }, + + /* no defaults for ASP1TX mixer */ + { CS35L56_SWIRE_DP3_CH1_INPUT, 0x00000018 }, { CS35L56_SWIRE_DP3_CH2_INPUT, 0x00000019 }, { CS35L56_SWIRE_DP3_CH3_INPUT, 0x00000029 }, diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c index 41aa79848b15..1b51650a19ff 100644 --- a/sound/soc/codecs/cs35l56.c +++ b/sound/soc/codecs/cs35l56.c @@ -59,6 +59,135 @@ static int cs35l56_dspwait_put_volsw(struct snd_kcontrol *kcontrol, return snd_soc_put_volsw(kcontrol, ucontrol); }
+static const unsigned short cs35l56_asp1_mixer_regs[] = { + CS35L56_ASP1TX1_INPUT, CS35L56_ASP1TX2_INPUT, + CS35L56_ASP1TX3_INPUT, CS35L56_ASP1TX4_INPUT, +}; + +static const char * const cs35l56_asp1_mux_control_names[] = { + "ASP1 TX1 Source", "ASP1 TX2 Source", "ASP1 TX3 Source", "ASP1 TX4 Source" +}; + +static int cs35l56_dspwait_asp1tx_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *component = snd_soc_dapm_kcontrol_component(kcontrol); + struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(component); + struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; + int index = e->shift_l; + unsigned int addr, val; + int ret; + + /* Wait for mux to be initialized */ + cs35l56_wait_dsp_ready(cs35l56); + flush_work(&cs35l56->mux_init_work); + + addr = cs35l56_asp1_mixer_regs[index]; + ret = regmap_read(cs35l56->base.regmap, addr, &val); + if (ret) + return ret; + + val &= CS35L56_ASP_TXn_SRC_MASK; + ucontrol->value.enumerated.item[0] = snd_soc_enum_val_to_item(e, val); + + return 0; +} + +static int cs35l56_dspwait_asp1tx_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *component = snd_soc_dapm_kcontrol_component(kcontrol); + struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kcontrol); + struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(component); + struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; + int item = ucontrol->value.enumerated.item[0]; + int index = e->shift_l; + unsigned int addr, val; + bool changed; + int ret; + + /* Wait for mux to be initialized */ + cs35l56_wait_dsp_ready(cs35l56); + flush_work(&cs35l56->mux_init_work); + + addr = cs35l56_asp1_mixer_regs[index]; + val = snd_soc_enum_item_to_val(e, item); + + ret = regmap_update_bits_check(cs35l56->base.regmap, addr, + CS35L56_ASP_TXn_SRC_MASK, val, &changed); + if (!ret) + return ret; + + if (changed) + snd_soc_dapm_mux_update_power(dapm, kcontrol, item, e, NULL); + + return changed; +} + +static void cs35l56_mark_asp1_mixer_widgets_dirty(struct cs35l56_private *cs35l56) +{ + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(cs35l56->component); + const char *prefix = cs35l56->component->name_prefix; + char full_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN]; + const char *name; + struct snd_kcontrol *kcontrol; + struct soc_enum *e; + unsigned int val[4]; + int i, item, ret; + + /* + * Resume so we can read the registers from silicon if the regmap + * cache has not yet been populated. + */ + ret = pm_runtime_resume_and_get(cs35l56->base.dev); + if (ret < 0) + return; + + ret = regmap_bulk_read(cs35l56->base.regmap, CS35L56_ASP1TX1_INPUT, + val, ARRAY_SIZE(val)); + + pm_runtime_mark_last_busy(cs35l56->base.dev); + pm_runtime_put_autosuspend(cs35l56->base.dev); + + if (ret) { + dev_err(cs35l56->base.dev, "Failed to read ASP1 mixer regs: %d\n", ret); + return; + } + + snd_soc_card_mutex_lock(dapm->card); + WARN_ON(!dapm->card->instantiated); + + for (i = 0; i < ARRAY_SIZE(cs35l56_asp1_mux_control_names); ++i) { + name = cs35l56_asp1_mux_control_names[i]; + + if (prefix) { + snprintf(full_name, sizeof(full_name), "%s %s", prefix, name); + name = full_name; + } + + kcontrol = snd_soc_card_get_kcontrol(dapm->card, name); + if (!kcontrol) { + dev_warn(cs35l56->base.dev, "Could not find control %s\n", name); + continue; + } + + e = (struct soc_enum *)kcontrol->private_value; + item = snd_soc_enum_val_to_item(e, val[i] & CS35L56_ASP_TXn_SRC_MASK); + snd_soc_dapm_mux_update_power(dapm, kcontrol, item, e, NULL); + } + + snd_soc_card_mutex_unlock(dapm->card); +} + +static void cs35l56_mux_init_work(struct work_struct *work) +{ + struct cs35l56_private *cs35l56 = container_of(work, + struct cs35l56_private, + mux_init_work); + + cs35l56_mark_asp1_mixer_widgets_dirty(cs35l56); +} + static DECLARE_TLV_DB_SCALE(vol_tlv, -10000, 25, 0);
static const struct snd_kcontrol_new cs35l56_controls[] = { @@ -77,40 +206,44 @@ static const struct snd_kcontrol_new cs35l56_controls[] = { };
static SOC_VALUE_ENUM_SINGLE_DECL(cs35l56_asp1tx1_enum, - CS35L56_ASP1TX1_INPUT, - 0, CS35L56_ASP_TXn_SRC_MASK, + SND_SOC_NOPM, + 0, 0, cs35l56_tx_input_texts, cs35l56_tx_input_values);
static const struct snd_kcontrol_new asp1_tx1_mux = - SOC_DAPM_ENUM("ASP1TX1 SRC", cs35l56_asp1tx1_enum); + SOC_DAPM_ENUM_EXT("ASP1TX1 SRC", cs35l56_asp1tx1_enum, + cs35l56_dspwait_asp1tx_get, cs35l56_dspwait_asp1tx_put);
static SOC_VALUE_ENUM_SINGLE_DECL(cs35l56_asp1tx2_enum, - CS35L56_ASP1TX2_INPUT, - 0, CS35L56_ASP_TXn_SRC_MASK, + SND_SOC_NOPM, + 1, 0, cs35l56_tx_input_texts, cs35l56_tx_input_values);
static const struct snd_kcontrol_new asp1_tx2_mux = - SOC_DAPM_ENUM("ASP1TX2 SRC", cs35l56_asp1tx2_enum); + SOC_DAPM_ENUM_EXT("ASP1TX2 SRC", cs35l56_asp1tx2_enum, + cs35l56_dspwait_asp1tx_get, cs35l56_dspwait_asp1tx_put);
static SOC_VALUE_ENUM_SINGLE_DECL(cs35l56_asp1tx3_enum, - CS35L56_ASP1TX3_INPUT, - 0, CS35L56_ASP_TXn_SRC_MASK, + SND_SOC_NOPM, + 2, 0, cs35l56_tx_input_texts, cs35l56_tx_input_values);
static const struct snd_kcontrol_new asp1_tx3_mux = - SOC_DAPM_ENUM("ASP1TX3 SRC", cs35l56_asp1tx3_enum); + SOC_DAPM_ENUM_EXT("ASP1TX3 SRC", cs35l56_asp1tx3_enum, + cs35l56_dspwait_asp1tx_get, cs35l56_dspwait_asp1tx_put);
static SOC_VALUE_ENUM_SINGLE_DECL(cs35l56_asp1tx4_enum, - CS35L56_ASP1TX4_INPUT, - 0, CS35L56_ASP_TXn_SRC_MASK, + SND_SOC_NOPM, + 3, 0, cs35l56_tx_input_texts, cs35l56_tx_input_values);
static const struct snd_kcontrol_new asp1_tx4_mux = - SOC_DAPM_ENUM("ASP1TX4 SRC", cs35l56_asp1tx4_enum); + SOC_DAPM_ENUM_EXT("ASP1TX4 SRC", cs35l56_asp1tx4_enum, + cs35l56_dspwait_asp1tx_get, cs35l56_dspwait_asp1tx_put);
static SOC_VALUE_ENUM_SINGLE_DECL(cs35l56_sdw1tx1_enum, CS35L56_SWIRE_DP3_CH1_INPUT, @@ -785,6 +918,15 @@ static void cs35l56_dsp_work(struct work_struct *work) else cs35l56_patch(cs35l56);
+ + /* + * Set starting value of ASP1 mux widgets. Updating a mux takes + * the DAPM mutex. Post this to a separate job so that DAPM + * power-up can wait for dsp_work to complete without deadlocking + * on the DAPM mutex. + */ + queue_work(cs35l56->dsp_wq, &cs35l56->mux_init_work); + pm_runtime_mark_last_busy(cs35l56->base.dev); pm_runtime_put_autosuspend(cs35l56->base.dev); } @@ -830,6 +972,7 @@ static void cs35l56_component_remove(struct snd_soc_component *component) struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(component);
cancel_work_sync(&cs35l56->dsp_work); + cancel_work_sync(&cs35l56->mux_init_work);
if (cs35l56->dsp.cs_dsp.booted) wm_adsp_power_down(&cs35l56->dsp); @@ -897,8 +1040,10 @@ int cs35l56_system_suspend(struct device *dev)
dev_dbg(dev, "system_suspend\n");
- if (cs35l56->component) + if (cs35l56->component) { flush_work(&cs35l56->dsp_work); + cancel_work_sync(&cs35l56->mux_init_work); + }
/* * The interrupt line is normally shared, but after we start suspending @@ -1049,6 +1194,7 @@ static int cs35l56_dsp_init(struct cs35l56_private *cs35l56) return -ENOMEM;
INIT_WORK(&cs35l56->dsp_work, cs35l56_dsp_work); + INIT_WORK(&cs35l56->mux_init_work, cs35l56_mux_init_work);
dsp = &cs35l56->dsp; cs35l56_init_cs_dsp(&cs35l56->base, &dsp->cs_dsp); diff --git a/sound/soc/codecs/cs35l56.h b/sound/soc/codecs/cs35l56.h index 8159c3e217d9..dc2fe4c91e67 100644 --- a/sound/soc/codecs/cs35l56.h +++ b/sound/soc/codecs/cs35l56.h @@ -34,6 +34,7 @@ struct cs35l56_private { struct wm_adsp dsp; /* must be first member */ struct cs35l56_base base; struct work_struct dsp_work; + struct work_struct mux_init_work; struct workqueue_struct *dsp_wq; struct snd_soc_component *component; struct regulator_bulk_data supplies[CS35L56_NUM_BULK_SUPPLIES];
Put the silicon revision and secured flag in the wm_adsp fwf_name string instead of including them in the part string.
This changes the format of the firmware name string from
cs35l56[s]-rev-misc[-system_name]
to cs35l56-rev[-s]-misc[-system_name]
No firmware files have been published, so this doesn't cause a compatibility break.
Silicon revision and secured flag are included in the firmware filename to pick a firmware compatible with the part. These strings were being added to the part string, but that is a misuse of the string. The correct place for these is the fwf_name string, which is specifically intended to select between multiple firmware files for the same part.
Backport note: This won't apply to kernels older than v6.6.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 608f1b0dbdde ("ASoC: cs35l56: Move DSP part string generation so that it is done only once") --- sound/soc/codecs/cs35l56.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c index 1b51650a19ff..8899c02c6dea 100644 --- a/sound/soc/codecs/cs35l56.c +++ b/sound/soc/codecs/cs35l56.c @@ -907,6 +907,18 @@ static void cs35l56_dsp_work(struct work_struct *work)
pm_runtime_get_sync(cs35l56->base.dev);
+ /* Populate fw file qualifier with the revision and security state */ + if (!cs35l56->dsp.fwf_name) { + cs35l56->dsp.fwf_name = kasprintf(GFP_KERNEL, "%02x%s-dsp1", + cs35l56->base.rev, + cs35l56->base.secured ? "-s" : ""); + if (!cs35l56->dsp.fwf_name) + goto err; + } + + dev_dbg(cs35l56->base.dev, "DSP fwf name: '%s' system name: '%s'\n", + cs35l56->dsp.fwf_name, cs35l56->dsp.system_name); + /* * When the device is running in secure mode the firmware files can * only contain insecure tunings and therefore we do not need to @@ -926,7 +938,7 @@ static void cs35l56_dsp_work(struct work_struct *work) * on the DAPM mutex. */ queue_work(cs35l56->dsp_wq, &cs35l56->mux_init_work); - +err: pm_runtime_mark_last_busy(cs35l56->base.dev); pm_runtime_put_autosuspend(cs35l56->base.dev); } @@ -979,6 +991,9 @@ static void cs35l56_component_remove(struct snd_soc_component *component)
wm_adsp2_component_remove(&cs35l56->dsp, component);
+ kfree(cs35l56->dsp.fwf_name); + cs35l56->dsp.fwf_name = NULL; + cs35l56->component = NULL; }
@@ -1330,12 +1345,6 @@ int cs35l56_init(struct cs35l56_private *cs35l56) if (ret) return ret;
- /* Populate the DSP information with the revision and security state */ - cs35l56->dsp.part = devm_kasprintf(cs35l56->base.dev, GFP_KERNEL, "cs35l56%s-%02x", - cs35l56->base.secured ? "s" : "", cs35l56->base.rev); - if (!cs35l56->dsp.part) - return -ENOMEM; - if (!cs35l56->base.reset_gpio) { dev_dbg(cs35l56->base.dev, "No reset gpio: using soft reset\n"); cs35l56->soft_resetting = true;
Check during initialization whether the firmware is already patched. If so, include the firmware version in the wm_adsp fwf_name string.
If the firmware has already been patched by the BIOS the driver can only replace it if it has control of hard RESET.
If the driver cannot replace the firmware, it can still load a wmfw (for ALSA control definitions) and/or a bin (for additional tunings). But these must match the version of firmware that is running on the CS35L56.
The firmware is pre-patched if FIRMWARE_MISSING == 0.
Including the firmware version in the fwf_name string will qualify the firmware file name:
Normal (unpatched or replaceable firmware): cs35l56-rev-dsp1-misc[-system_name].[wmfw|bin]
Preloaded firmware: cs35l56-rev[-s]-VVVVVV-dsp1-misc[-system_name].[wmfw|bin]
Where: [-s] is an optional -s added into the name for a secured CS35L56 VVVVVV is the 24-bit firmware version in hexadecimal.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 608f1b0dbdde ("ASoC: cs35l56: Move DSP part string generation so that it is done only once") --- include/sound/cs35l56.h | 3 ++ sound/soc/codecs/cs35l56-shared.c | 36 +++++++++++++++++++-- sound/soc/codecs/cs35l56.c | 52 +++++++++++++++++-------------- 3 files changed, 65 insertions(+), 26 deletions(-)
diff --git a/include/sound/cs35l56.h b/include/sound/cs35l56.h index 4db36c893d9d..5d6aefc41e64 100644 --- a/include/sound/cs35l56.h +++ b/include/sound/cs35l56.h @@ -75,6 +75,7 @@ #define CS35L56_DSP1_AHBM_WINDOW_DEBUG_0 0x25E2040 #define CS35L56_DSP1_AHBM_WINDOW_DEBUG_1 0x25E2044 #define CS35L56_DSP1_XMEM_UNPACKED24_0 0x2800000 +#define CS35L56_DSP1_FW_VER 0x2800010 #define CS35L56_DSP1_HALO_STATE_A1 0x2801E58 #define CS35L56_DSP1_HALO_STATE 0x28021E0 #define CS35L56_DSP1_PM_CUR_STATE_A1 0x2804000 @@ -285,6 +286,8 @@ int cs35l56_is_fw_reload_needed(struct cs35l56_base *cs35l56_base); int cs35l56_runtime_suspend_common(struct cs35l56_base *cs35l56_base); int cs35l56_runtime_resume_common(struct cs35l56_base *cs35l56_base, bool is_soundwire); void cs35l56_init_cs_dsp(struct cs35l56_base *cs35l56_base, struct cs_dsp *cs_dsp); +int cs35l56_read_prot_status(struct cs35l56_base *cs35l56_base, + bool *fw_missing, unsigned int *fw_version); int cs35l56_hw_init(struct cs35l56_base *cs35l56_base); int cs35l56_get_bclk_freq_id(unsigned int freq); void cs35l56_fill_supply_names(struct regulator_bulk_data *data); diff --git a/sound/soc/codecs/cs35l56-shared.c b/sound/soc/codecs/cs35l56-shared.c index 9a70db0fa418..33835535ef84 100644 --- a/sound/soc/codecs/cs35l56-shared.c +++ b/sound/soc/codecs/cs35l56-shared.c @@ -628,10 +628,35 @@ void cs35l56_init_cs_dsp(struct cs35l56_base *cs35l56_base, struct cs_dsp *cs_ds } EXPORT_SYMBOL_NS_GPL(cs35l56_init_cs_dsp, SND_SOC_CS35L56_SHARED);
+int cs35l56_read_prot_status(struct cs35l56_base *cs35l56_base, + bool *fw_missing, unsigned int *fw_version) +{ + unsigned int prot_status; + int ret; + + ret = regmap_read(cs35l56_base->regmap, CS35L56_PROTECTION_STATUS, &prot_status); + if (ret) { + dev_err(cs35l56_base->dev, "Get PROTECTION_STATUS failed: %d\n", ret); + return ret; + } + + *fw_missing = !!(prot_status & CS35L56_FIRMWARE_MISSING); + + ret = regmap_read(cs35l56_base->regmap, CS35L56_DSP1_FW_VER, fw_version); + if (ret) { + dev_err(cs35l56_base->dev, "Get FW VER failed: %d\n", ret); + return ret; + } + + return 0; +} +EXPORT_SYMBOL_NS_GPL(cs35l56_read_prot_status, SND_SOC_CS35L56_SHARED); + int cs35l56_hw_init(struct cs35l56_base *cs35l56_base) { int ret; - unsigned int devid, revid, otpid, secured; + unsigned int devid, revid, otpid, secured, fw_ver; + bool fw_missing;
/* * When the system is not using a reset_gpio ensure the device is @@ -690,8 +715,13 @@ int cs35l56_hw_init(struct cs35l56_base *cs35l56_base) return ret; }
- dev_info(cs35l56_base->dev, "Cirrus Logic CS35L56%s Rev %02X OTP%d\n", - cs35l56_base->secured ? "s" : "", cs35l56_base->rev, otpid); + ret = cs35l56_read_prot_status(cs35l56_base, &fw_missing, &fw_ver); + if (ret) + return ret; + + dev_info(cs35l56_base->dev, "Cirrus Logic CS35L56%s Rev %02X OTP%d fw:%d.%d.%d (patched=%u)\n", + cs35l56_base->secured ? "s" : "", cs35l56_base->rev, otpid, + fw_ver >> 16, (fw_ver >> 8) & 0xff, fw_ver & 0xff, !fw_missing);
/* Wake source and *_BLOCKED interrupts default to unmasked, so mask them */ regmap_write(cs35l56_base->regmap, CS35L56_IRQ1_MASK_20, 0xffffffff); diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c index 8899c02c6dea..597677422547 100644 --- a/sound/soc/codecs/cs35l56.c +++ b/sound/soc/codecs/cs35l56.c @@ -804,7 +804,7 @@ static struct snd_soc_dai_driver cs35l56_dai[] = { } };
-static void cs35l56_secure_patch(struct cs35l56_private *cs35l56) +static void cs35l56_reinit_patch(struct cs35l56_private *cs35l56) { int ret;
@@ -816,19 +816,10 @@ static void cs35l56_secure_patch(struct cs35l56_private *cs35l56) cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_REINIT); }
-static void cs35l56_patch(struct cs35l56_private *cs35l56) +static void cs35l56_patch(struct cs35l56_private *cs35l56, bool firmware_missing) { - unsigned int firmware_missing; int ret;
- ret = regmap_read(cs35l56->base.regmap, CS35L56_PROTECTION_STATUS, &firmware_missing); - if (ret) { - dev_err(cs35l56->base.dev, "Failed to read PROTECTION_STATUS: %d\n", ret); - return; - } - - firmware_missing &= CS35L56_FIRMWARE_MISSING; - /* * Disable SoundWire interrupts to prevent race with IRQ work. * Setting sdw_irq_no_unmask prevents the handler re-enabling @@ -901,34 +892,49 @@ static void cs35l56_dsp_work(struct work_struct *work) struct cs35l56_private *cs35l56 = container_of(work, struct cs35l56_private, dsp_work); + unsigned int firmware_version; + bool firmware_missing; + int ret;
if (!cs35l56->base.init_done) return;
pm_runtime_get_sync(cs35l56->base.dev);
+ ret = cs35l56_read_prot_status(&cs35l56->base, &firmware_missing, &firmware_version); + if (ret) + goto err; + /* Populate fw file qualifier with the revision and security state */ - if (!cs35l56->dsp.fwf_name) { - cs35l56->dsp.fwf_name = kasprintf(GFP_KERNEL, "%02x%s-dsp1", + kfree(cs35l56->dsp.fwf_name); + if (firmware_missing) { + cs35l56->dsp.fwf_name = kasprintf(GFP_KERNEL, "%02x-dsp1", cs35l56->base.rev); + } else { + /* Firmware files must match the running firmware version */ + cs35l56->dsp.fwf_name = kasprintf(GFP_KERNEL, + "%02x%s-%06x-dsp1", cs35l56->base.rev, - cs35l56->base.secured ? "-s" : ""); - if (!cs35l56->dsp.fwf_name) - goto err; + cs35l56->base.secured ? "-s" : "", + firmware_version); }
+ if (!cs35l56->dsp.fwf_name) + goto err; + dev_dbg(cs35l56->base.dev, "DSP fwf name: '%s' system name: '%s'\n", cs35l56->dsp.fwf_name, cs35l56->dsp.system_name);
/* - * When the device is running in secure mode the firmware files can - * only contain insecure tunings and therefore we do not need to - * shutdown the firmware to apply them and can use the lower cost - * reinit sequence instead. + * The firmware cannot be patched if it is already running from + * patch RAM. In this case the firmware files are versioned to + * match the running firmware version and will only contain + * tunings. We do not need to shutdown the firmware to apply + * tunings so can use the lower cost reinit sequence instead. */ - if (cs35l56->base.secured) - cs35l56_secure_patch(cs35l56); + if (!firmware_missing) + cs35l56_reinit_patch(cs35l56); else - cs35l56_patch(cs35l56); + cs35l56_patch(cs35l56, firmware_missing);
/*
If the "spk-id-gpios" property is present it points to GPIOs whose value must be used to select the correct bin file to match the speakers.
Some manufacturers use multiple sources of speakers, which need different tunings for best performance. On these models the type of speaker fitted is indicated by the values of one or more GPIOs. The number formed by the GPIOs identifies the tuning required.
The speaker ID must be used in combination with the subsystem ID (either from PCI SSID or cirrus,firmware-uid property), because the GPIOs can only indicate variants of a specific model.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 1a1c3d794ef6 ("ASoC: cs35l56: Use PCI SSID as the firmware UID") --- include/sound/cs35l56.h | 1 + sound/soc/codecs/cs35l56-shared.c | 36 +++++++++++++++++++++++++++++++ sound/soc/codecs/cs35l56.c | 32 ++++++++++++++++++++++----- sound/soc/codecs/cs35l56.h | 1 + 4 files changed, 65 insertions(+), 5 deletions(-)
diff --git a/include/sound/cs35l56.h b/include/sound/cs35l56.h index 5d6aefc41e64..23da6298ab37 100644 --- a/include/sound/cs35l56.h +++ b/include/sound/cs35l56.h @@ -289,6 +289,7 @@ void cs35l56_init_cs_dsp(struct cs35l56_base *cs35l56_base, struct cs_dsp *cs_ds int cs35l56_read_prot_status(struct cs35l56_base *cs35l56_base, bool *fw_missing, unsigned int *fw_version); int cs35l56_hw_init(struct cs35l56_base *cs35l56_base); +int cs35l56_get_speaker_id(struct cs35l56_base *cs35l56_base); int cs35l56_get_bclk_freq_id(unsigned int freq); void cs35l56_fill_supply_names(struct regulator_bulk_data *data);
diff --git a/sound/soc/codecs/cs35l56-shared.c b/sound/soc/codecs/cs35l56-shared.c index 33835535ef84..02fba4bc0a14 100644 --- a/sound/soc/codecs/cs35l56-shared.c +++ b/sound/soc/codecs/cs35l56-shared.c @@ -5,6 +5,7 @@ // Copyright (C) 2023 Cirrus Logic, Inc. and // Cirrus Logic International Semiconductor Ltd.
+#include <linux/gpio/consumer.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> #include <linux/types.h> @@ -736,6 +737,41 @@ int cs35l56_hw_init(struct cs35l56_base *cs35l56_base) } EXPORT_SYMBOL_NS_GPL(cs35l56_hw_init, SND_SOC_CS35L56_SHARED);
+int cs35l56_get_speaker_id(struct cs35l56_base *cs35l56_base) +{ + struct gpio_descs *descs; + int speaker_id; + int i, ret; + + /* Read the speaker type qualifier from the motherboard GPIOs */ + descs = gpiod_get_array_optional(cs35l56_base->dev, "spk-id", GPIOD_IN); + if (!descs) { + return -ENOENT; + } else if (IS_ERR(descs)) { + ret = PTR_ERR(descs); + return dev_err_probe(cs35l56_base->dev, ret, "Failed to get spk-id-gpios\n"); + } + + speaker_id = 0; + for (i = 0; i < descs->ndescs; i++) { + ret = gpiod_get_value_cansleep(descs->desc[i]); + if (ret < 0) { + dev_err_probe(cs35l56_base->dev, ret, "Failed to read spk-id[%d]\n", i); + goto err; + } + + speaker_id |= (ret << i); + } + + dev_dbg(cs35l56_base->dev, "Speaker ID = %d\n", speaker_id); + ret = speaker_id; +err: + gpiod_put_array(descs); + + return ret; +} +EXPORT_SYMBOL_NS_GPL(cs35l56_get_speaker_id, SND_SOC_CS35L56_SHARED); + static const u32 cs35l56_bclk_valid_for_pll_freq_table[] = { [0x0C] = 128000, [0x0F] = 256000, diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c index 597677422547..c23e29da4cfb 100644 --- a/sound/soc/codecs/cs35l56.c +++ b/sound/soc/codecs/cs35l56.c @@ -959,10 +959,19 @@ static int cs35l56_component_probe(struct snd_soc_component *component)
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); + /* Append a speaker qualifier if there is a speaker ID */ + if (cs35l56->speaker_id >= 0) { + cs35l56->dsp.system_name = devm_kasprintf(cs35l56->base.dev, + GFP_KERNEL, + "%04x%04x-spkid%d", + vendor, device, + cs35l56->speaker_id); + } else { + cs35l56->dsp.system_name = devm_kasprintf(cs35l56->base.dev, + GFP_KERNEL, + "%04x%04x", + vendor, device); + } if (!cs35l56->dsp.system_name) return -ENOMEM; } @@ -1245,7 +1254,13 @@ static int cs35l56_get_firmware_uid(struct cs35l56_private *cs35l56) if (ret < 0) return 0;
- cs35l56->dsp.system_name = devm_kstrdup(dev, prop, GFP_KERNEL); + /* Append a speaker qualifier if there is a speaker ID */ + if (cs35l56->speaker_id >= 0) + cs35l56->dsp.system_name = devm_kasprintf(dev, GFP_KERNEL, "%s-spkid%d", + prop, cs35l56->speaker_id); + else + cs35l56->dsp.system_name = devm_kstrdup(dev, prop, GFP_KERNEL); + if (cs35l56->dsp.system_name == NULL) return -ENOMEM;
@@ -1260,6 +1275,7 @@ int cs35l56_common_probe(struct cs35l56_private *cs35l56)
init_completion(&cs35l56->init_completion); mutex_init(&cs35l56->base.irq_lock); + cs35l56->speaker_id = -ENOENT;
dev_set_drvdata(cs35l56->base.dev, cs35l56);
@@ -1296,6 +1312,12 @@ int cs35l56_common_probe(struct cs35l56_private *cs35l56) gpiod_set_value_cansleep(cs35l56->base.reset_gpio, 1); }
+ ret = cs35l56_get_speaker_id(&cs35l56->base); + if ((ret < 0) && (ret != -ENOENT)) + goto err; + + cs35l56->speaker_id = ret; + ret = cs35l56_get_firmware_uid(cs35l56); if (ret != 0) goto err; diff --git a/sound/soc/codecs/cs35l56.h b/sound/soc/codecs/cs35l56.h index dc2fe4c91e67..596b141e3f96 100644 --- a/sound/soc/codecs/cs35l56.h +++ b/sound/soc/codecs/cs35l56.h @@ -45,6 +45,7 @@ struct cs35l56_private { bool sdw_attached; struct completion init_completion;
+ int speaker_id; u32 rx_mask; u32 tx_mask; u8 asp_slot_width;
The original 50ms timeout for firmware boot is not long enough for worst-case time to reboot after a firmware download. Increase the timeout to 250ms.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: e49611252900 ("ASoC: cs35l56: Add driver for Cirrus Logic CS35L56") --- include/sound/cs35l56.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/sound/cs35l56.h b/include/sound/cs35l56.h index 23da6298ab37..b24716ab2750 100644 --- a/include/sound/cs35l56.h +++ b/include/sound/cs35l56.h @@ -242,7 +242,7 @@
#define CS35L56_CONTROL_PORT_READY_US 2200 #define CS35L56_HALO_STATE_POLL_US 1000 -#define CS35L56_HALO_STATE_TIMEOUT_US 50000 +#define CS35L56_HALO_STATE_TIMEOUT_US 250000 #define CS35L56_RESET_PULSE_MIN_US 1100 #define CS35L56_WAKE_HOLD_TIME_US 1000
Check for the cases of system-specific bin file without a wmfw before falling back to looking for a generic wmfw.
All system-specific options should be tried before falling back to loading a generic wmfw/bin. With the original code, the presence of a fallback generic wmfw on the filesystem would prevent using a system-specific tuning with a ROM firmware.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 73cfbfa9caea ("ALSA: hda/cs35l56: Add driver for Cirrus Logic CS35L56 amplifier") --- sound/pci/hda/cs35l56_hda.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index f22bcb104a4e..7ba7234d8d9c 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -483,6 +483,20 @@ static void cs35l56_hda_request_firmware_files(struct cs35l56_hda *cs35l56, NULL, "bin"); return; } + + /* + * Check for system-specific bin files without wmfw before + * falling back to generic firmware + */ + if (amp_name) + cs35l56_hda_request_firmware_file(cs35l56, coeff_firmware, coeff_filename, + cirrus_dir, system_name, amp_name, "bin"); + if (!*coeff_firmware) + cs35l56_hda_request_firmware_file(cs35l56, coeff_firmware, coeff_filename, + cirrus_dir, system_name, NULL, "bin"); + + if (*coeff_firmware) + return; }
ret = cs35l56_hda_request_firmware_file(cs35l56, wmfw_firmware, wmfw_filename, @@ -493,16 +507,6 @@ static void cs35l56_hda_request_firmware_files(struct cs35l56_hda *cs35l56, return; }
- /* When a firmware file is not found must still search for the coeff files */ - if (system_name) { - if (amp_name) - cs35l56_hda_request_firmware_file(cs35l56, coeff_firmware, coeff_filename, - cirrus_dir, system_name, amp_name, "bin"); - if (!*coeff_firmware) - cs35l56_hda_request_firmware_file(cs35l56, coeff_firmware, coeff_filename, - cirrus_dir, system_name, NULL, "bin"); - } - if (!*coeff_firmware) cs35l56_hda_request_firmware_file(cs35l56, coeff_firmware, coeff_filename, cirrus_dir, NULL, NULL, "bin");
Change the filename field layout to: cs35l56-rev[-s]-dsp1-misc[-sub].[wmfw|bin]
This is to keep the same firmware file naming scheme as the CS35L56 ASoC driver.
This is not a compatibility break because no firmware files have been published.
The original field layout matched the ASoC driver, but the way the ASoC driver used the wm_adsp driver config to form this filename was bugged. Fixing the ASoC driver to use the correct wm_adsp config strings means that the 's' flag (to indicate a secured part) has to move to somewhere after the first '-'.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 73cfbfa9caea ("ALSA: hda/cs35l56: Add driver for Cirrus Logic CS35L56 amplifier") --- sound/pci/hda/cs35l56_hda.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index 7ba7234d8d9c..081479f65fe7 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -405,16 +405,19 @@ static int cs35l56_hda_request_firmware_file(struct cs35l56_hda *cs35l56, int ret = 0;
if (system_name && amp_name) - *filename = kasprintf(GFP_KERNEL, "%scs35l56%s-%02x-dsp1-misc-%s-%s.%s", dir, - cs35l56->base.secured ? "s" : "", cs35l56->base.rev, + *filename = kasprintf(GFP_KERNEL, "%scs35l56-%02x%s-dsp1-misc-%s-%s.%s", dir, + cs35l56->base.rev, + cs35l56->base.secured ? "-s" : "", system_name, amp_name, filetype); else if (system_name) - *filename = kasprintf(GFP_KERNEL, "%scs35l56%s-%02x-dsp1-misc-%s.%s", dir, - cs35l56->base.secured ? "s" : "", cs35l56->base.rev, + *filename = kasprintf(GFP_KERNEL, "%scs35l56-%02x%s-dsp1-misc-%s.%s", dir, + cs35l56->base.rev, + cs35l56->base.secured ? "-s" : "", system_name, filetype); else - *filename = kasprintf(GFP_KERNEL, "%scs35l56%s-%02x-dsp1-misc.%s", dir, - cs35l56->base.secured ? "s" : "", cs35l56->base.rev, + *filename = kasprintf(GFP_KERNEL, "%scs35l56-%02x%s-dsp1-misc.%s", dir, + cs35l56->base.rev, + cs35l56->base.secured ? "-s" : "", filetype);
if (!*filename)
Check whether the firmware is already patched. If so, include the firmware version in the firmware file name.
If the firmware has already been patched by the BIOS the driver can only replace it if it has control of hard RESET.
If the driver cannot replace the firmware, it can still load a wmfw (for ALSA control definitions) and/or a bin (for additional tunings). But these must match the version of firmware that is running on the CS35L56.
The firmware is pre-patched if either: - FIRMWARE_MISSING == 0, or - it is a secured CS35L56 (which implies that is was already patched),
cs35l56_hw_init() will set preloaded_fw_ver to the (non-zero) firmware version if either of these conditions is true.
Normal (unpatched or replaceable firmware): cs35l56-rev-dsp1-misc[-system_name].[wmfw|bin]
Preloaded firmware: cs35l56-rev[-s]-VVVVVV-dsp1-misc[-system_name].[wmfw|bin]
Where: [-s] is an optional -s added into the name for a secured CS35L56 VVVVVV is the 24-bit firmware version in hexadecimal.
Backport note: This won't apply to kernel versions older than v6.6.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 73cfbfa9caea ("ALSA: hda/cs35l56: Add driver for Cirrus Logic CS35L56 amplifier") --- sound/pci/hda/cs35l56_hda.c | 95 +++++++++++++++++++------------------ 1 file changed, 50 insertions(+), 45 deletions(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index 081479f65fe7..32736d3e45ba 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -397,7 +397,7 @@ static const struct cs_dsp_client_ops cs35l56_hda_client_ops = {
static int cs35l56_hda_request_firmware_file(struct cs35l56_hda *cs35l56, const struct firmware **firmware, char **filename, - const char *dir, const char *system_name, + const char *base_name, const char *system_name, const char *amp_name, const char *filetype) { @@ -405,20 +405,13 @@ static int cs35l56_hda_request_firmware_file(struct cs35l56_hda *cs35l56, int ret = 0;
if (system_name && amp_name) - *filename = kasprintf(GFP_KERNEL, "%scs35l56-%02x%s-dsp1-misc-%s-%s.%s", dir, - cs35l56->base.rev, - cs35l56->base.secured ? "-s" : "", + *filename = kasprintf(GFP_KERNEL, "%s-%s-%s.%s", base_name, system_name, amp_name, filetype); else if (system_name) - *filename = kasprintf(GFP_KERNEL, "%scs35l56-%02x%s-dsp1-misc-%s.%s", dir, - cs35l56->base.rev, - cs35l56->base.secured ? "-s" : "", + *filename = kasprintf(GFP_KERNEL, "%s-%s.%s", base_name, system_name, filetype); else - *filename = kasprintf(GFP_KERNEL, "%scs35l56-%02x%s-dsp1-misc.%s", dir, - cs35l56->base.rev, - cs35l56->base.secured ? "-s" : "", - filetype); + *filename = kasprintf(GFP_KERNEL, "%s.%s", base_name, filetype);
if (!*filename) return -ENOMEM; @@ -451,8 +444,8 @@ static int cs35l56_hda_request_firmware_file(struct cs35l56_hda *cs35l56, return 0; }
-static const char cirrus_dir[] = "cirrus/"; static void cs35l56_hda_request_firmware_files(struct cs35l56_hda *cs35l56, + unsigned int preloaded_fw_ver, const struct firmware **wmfw_firmware, char **wmfw_filename, const struct firmware **coeff_firmware, @@ -460,29 +453,43 @@ static void cs35l56_hda_request_firmware_files(struct cs35l56_hda *cs35l56, { const char *system_name = cs35l56->system_name; const char *amp_name = cs35l56->amp_name; + char base_name[37]; int ret;
+ if (preloaded_fw_ver) { + snprintf(base_name, sizeof(base_name), + "cirrus/cs35l56-%02x%s-%06x-dsp1-misc", + cs35l56->base.rev, + cs35l56->base.secured ? "-s" : "", + preloaded_fw_ver & 0xffffff); + } else { + snprintf(base_name, sizeof(base_name), + "cirrus/cs35l56-%02x%s-dsp1-misc", + cs35l56->base.rev, + cs35l56->base.secured ? "-s" : ""); + } + if (system_name && amp_name) { if (!cs35l56_hda_request_firmware_file(cs35l56, wmfw_firmware, wmfw_filename, - cirrus_dir, system_name, amp_name, "wmfw")) { + base_name, system_name, amp_name, "wmfw")) { cs35l56_hda_request_firmware_file(cs35l56, coeff_firmware, coeff_filename, - cirrus_dir, system_name, amp_name, "bin"); + base_name, system_name, amp_name, "bin"); return; } }
if (system_name) { if (!cs35l56_hda_request_firmware_file(cs35l56, wmfw_firmware, wmfw_filename, - cirrus_dir, system_name, NULL, "wmfw")) { + base_name, system_name, NULL, "wmfw")) { if (amp_name) cs35l56_hda_request_firmware_file(cs35l56, coeff_firmware, coeff_filename, - cirrus_dir, system_name, + base_name, system_name, amp_name, "bin"); if (!*coeff_firmware) cs35l56_hda_request_firmware_file(cs35l56, coeff_firmware, coeff_filename, - cirrus_dir, system_name, + base_name, system_name, NULL, "bin"); return; } @@ -493,26 +500,26 @@ static void cs35l56_hda_request_firmware_files(struct cs35l56_hda *cs35l56, */ if (amp_name) cs35l56_hda_request_firmware_file(cs35l56, coeff_firmware, coeff_filename, - cirrus_dir, system_name, amp_name, "bin"); + base_name, system_name, amp_name, "bin"); if (!*coeff_firmware) cs35l56_hda_request_firmware_file(cs35l56, coeff_firmware, coeff_filename, - cirrus_dir, system_name, NULL, "bin"); + base_name, system_name, NULL, "bin");
if (*coeff_firmware) return; }
ret = cs35l56_hda_request_firmware_file(cs35l56, wmfw_firmware, wmfw_filename, - cirrus_dir, NULL, NULL, "wmfw"); + base_name, NULL, NULL, "wmfw"); if (!ret) { cs35l56_hda_request_firmware_file(cs35l56, coeff_firmware, coeff_filename, - cirrus_dir, NULL, NULL, "bin"); + base_name, NULL, NULL, "bin"); return; }
if (!*coeff_firmware) cs35l56_hda_request_firmware_file(cs35l56, coeff_firmware, coeff_filename, - cirrus_dir, NULL, NULL, "bin"); + base_name, NULL, NULL, "bin"); }
static void cs35l56_hda_release_firmware_files(const struct firmware *wmfw_firmware, @@ -546,7 +553,8 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) const struct firmware *wmfw_firmware = NULL; char *coeff_filename = NULL; char *wmfw_filename = NULL; - unsigned int firmware_missing; + unsigned int preloaded_fw_ver; + bool firmware_missing; int ret = 0;
/* Prepare for a new DSP power-up */ @@ -557,24 +565,21 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
pm_runtime_get_sync(cs35l56->base.dev);
- ret = regmap_read(cs35l56->base.regmap, CS35L56_PROTECTION_STATUS, &firmware_missing); - if (ret) { - dev_err(cs35l56->base.dev, "Failed to read PROTECTION_STATUS: %d\n", ret); - goto err_pm_put; - } - - firmware_missing &= CS35L56_FIRMWARE_MISSING; - /* - * Firmware can only be downloaded if the CS35L56 is secured or is - * running from the built-in ROM. If it is secured the BIOS will have - * downloaded firmware, and the wmfw/bin files will only contain - * tunings that are safe to download with the firmware running. + * The firmware can only be upgraded if it is currently running + * from the built-in ROM. If not, the wmfw/bin must be for the + * version of firmware that is running on the chip. */ - if (cs35l56->base.secured || firmware_missing) { - cs35l56_hda_request_firmware_files(cs35l56, &wmfw_firmware, &wmfw_filename, - &coeff_firmware, &coeff_filename); - } + ret = cs35l56_read_prot_status(&cs35l56->base, &firmware_missing, &preloaded_fw_ver); + if (ret) + goto err_pm_put; + + if (firmware_missing) + preloaded_fw_ver = 0; + + cs35l56_hda_request_firmware_files(cs35l56, preloaded_fw_ver, + &wmfw_firmware, &wmfw_filename, + &coeff_firmware, &coeff_filename);
/* * If the BIOS didn't patch the firmware a bin file is mandatory to @@ -589,12 +594,12 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) mutex_lock(&cs35l56->base.irq_lock);
/* - * When the device is running in secure mode the firmware files can - * only contain insecure tunings and therefore we do not need to - * shutdown the firmware to apply them and can use the lower cost - * reinit sequence instead. + * If the firmware hasn't been patched it must be shutdown before + * doing a full patch and reset afterwards. If it is already + * running a patched version the firmware files only contain + * tunings and we can use the lower cost reinit sequence instead. */ - if (!cs35l56->base.secured && (wmfw_firmware || coeff_firmware)) { + if (firmware_missing && (wmfw_firmware || coeff_firmware)) { ret = cs35l56_firmware_shutdown(&cs35l56->base); if (ret) goto err; @@ -613,7 +618,7 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) if (coeff_filename) dev_dbg(cs35l56->base.dev, "Loaded Coefficients: %s\n", coeff_filename);
- if (cs35l56->base.secured) { + if (!firmware_missing) { ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_REINIT); if (ret) goto err_powered_up;
Remove an unused stub function that calls a non-existant function.
This function was accidentally added as part of commit 2144833e7b41 ("ALSA: hda: cirrus_scodec: Add KUnit test"). It was a relic of an earlier version of the test that should have been removed.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 2144833e7b41 ("ALSA: hda: cirrus_scodec: Add KUnit test") --- sound/pci/hda/cs35l56_hda.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index 32736d3e45ba..75a14ba54fcd 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -1063,16 +1063,6 @@ const struct dev_pm_ops cs35l56_hda_pm_ops = { }; EXPORT_SYMBOL_NS_GPL(cs35l56_hda_pm_ops, SND_HDA_SCODEC_CS35L56);
-#if IS_ENABLED(CONFIG_SND_HDA_SCODEC_CS35L56_KUNIT_TEST) -/* Hooks to export static function to KUnit test */ - -int cs35l56_hda_test_hook_get_speaker_id(struct device *dev, int amp_index, int num_amps) -{ - return cs35l56_hda_get_speaker_id(dev, amp_index, num_amps); -} -EXPORT_SYMBOL_NS_GPL(cs35l56_hda_test_hook_get_speaker_id, SND_HDA_SCODEC_CS35L56); -#endif - MODULE_DESCRIPTION("CS35L56 HDA Driver"); MODULE_IMPORT_NS(SND_HDA_CIRRUS_SCODEC); MODULE_IMPORT_NS(SND_HDA_CS_DSP_CONTROLS);
On Mon, Jan 29, 2024 at 04:27:19PM +0000, Richard Fitzgerald wrote:
This chain of patches fixes various things that were undocumented, unknown or uncertain when the original driver code was written. And also a few things that were just bugs.
The HDA patches have dependencies on the ASoC patches, except for the final patch that removes a bogus test stub function.
Takashi, should I apply the ALSA bits of this via ASoC?
On Thu, 01 Feb 2024 13:47:38 +0100, Mark Brown wrote:
On Mon, Jan 29, 2024 at 04:27:19PM +0000, Richard Fitzgerald wrote:
This chain of patches fixes various things that were undocumented, unknown or uncertain when the original driver code was written. And also a few things that were just bugs.
The HDA patches have dependencies on the ASoC patches, except for the final patch that removes a bogus test stub function.
Takashi, should I apply the ALSA bits of this via ASoC?
Judging from the amount, better to go through your tree. Please go ahead.
thanks,
Takashi
On Mon, 29 Jan 2024 16:27:19 +0000, Richard Fitzgerald wrote:
This chain of patches fixes various things that were undocumented, unknown or uncertain when the original driver code was written. And also a few things that were just bugs.
The HDA patches have dependencies on the ASoC patches, except for the final patch that removes a bogus test stub function.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[01/18] ASoC: wm_adsp: Fix firmware file search order commit: 3657e4cb5a8abd9edf6c944e022fe9ef06989960 [02/18] ASoC: wm_adsp: Don't overwrite fwf_name with the default commit: daf3f0f99cde93a066240462b7a87cdfeedc04c0 [03/18] ASoC: cs35l56: cs35l56_component_remove() must clear cs35l56->component commit: ae861c466ee57e15a29d97629e1c564e3f714a4f [04/18] ASoC: cs35l56: cs35l56_component_remove() must clean up wm_adsp commit: cd38ccbecdace1469b4e0cfb3ddeec72a3fad226 [05/18] ASoC: cs35l56: Don't add the same register patch multiple times commit: 07687cd0539f8185b6ba0c0afba8473517116d6a [06/18] ASoC: cs35l56: Remove buggy checks from cs35l56_is_fw_reload_needed() commit: 3739cc0733ba7eeafc08d4d4208d1f3c2451eabd [07/18] ASoC: cs35l56: Fix to ensure ASP1 registers match cache commit: 72a77d7631c6e392677c0134343cf5edcd3a4572 [08/18] ASoC: cs35l56: Fix default SDW TX mixer registers commit: 782e6c538be43a17e34f552ab49e8c713cac7883 [09/18] ALSA: hda: cs35l56: Initialize all ASP1 registers commit: 856ce8982169acb31a25c5f2ecd2570ab8a6af46 [10/18] ASoC: cs35l56: Fix for initializing ASP1 mixer registers commit: 07f7d6e7a124d3e4de36771e2a4926d0e31c2258 [11/18] ASoC: cs35l56: Fix misuse of wm_adsp 'part' string for silicon revision commit: f6c967941c5d6fa526fdd64733a8d86bf2bfab31 [12/18] ASoC: cs35l56: Firmware file must match the version of preloaded firmware commit: f4ef5149953f2fc04907ca5b34db3df667dcddef [13/18] ASoC: cs35l56: Load tunings for the correct speaker models commit: 245eeff18d7a37693815250ae15979ce98c3d190 [14/18] ASoC: cs35l56: Allow more time for firmware to boot commit: 9e92b77ceb6f362eb2e7995dad6c7f9863053d97 [15/18] ALSA: hda: cs35l56: Fix order of searching for firmware files commit: 77c60722ded7d6739805e045e9648cda82dde5ed [16/18] ALSA: hda: cs35l56: Fix filename string field layout commit: e82bc517c6ef5d5c04b845420406e694c31bdb8a [17/18] ALSA: hda: cs35l56: Firmware file must match the version of preloaded firmware commit: 6f8ad0480d82245961dae4d3280908611633872d [18/18] ALSA: hda: cs35l56: Remove unused test stub function commit: 28876c1ae8b8cd1dacef50bd6c0555824774f0d2
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)
-
Mark Brown -
Pierre-Louis Bossart -
Richard Fitzgerald -
Takashi Iwai