[PATCH 0/9] ALSA: hda/cs35l56: Various bugfixes
A collection of various bugfixes to the cs35l56 driver.
Richard Fitzgerald (9): ALSA: hda/cs35l56: Complete firmware reboot before calling cs_dsp_run() ALSA: hda/cs35l56: Do not mark cache dirty after REINIT ALSA: hda/cs35l56: Call cs_dsp_power_down() before reloading firmware ALSA: hda/cs35l56: Always power-up and start cs_dsp ALSA: hda/cs35l56: Call cs_dsp_power_down() before calling cs_dsp_remove() ALSA: hda/cs35l56: cs_dsp_power_down() on cs35l56_hda_fw_load() error path ALSA: hda/cs35l56: Do not download firmware over existing RAM firmware ALSA: hda/cs35l56: Fail if .bin not found and firmware not patched ALSA: hda/cs35l56: Reject I2C alias addresses
sound/pci/hda/cs35l56_hda.c | 91 ++++++++++++++++++++++++++----------- 1 file changed, 65 insertions(+), 26 deletions(-)
Move the call to cs_dsp_run() in cs35l56_hda_fw_load() so that it is after the CS35L56 has been reset/reinit'd and the regmap cache has been synced.
cs_dsp_run() syncs up ALSA control cache values with the DSP memory so this must not be done until the firmware has reinitialized.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/pci/hda/cs35l56_hda.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index 4c3279f61b94..c3acda2daeeb 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -562,12 +562,6 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) if (coeff_filename) dev_dbg(cs35l56->base.dev, "Loaded Coefficients: %s\n", coeff_filename);
- ret = cs_dsp_run(&cs35l56->cs_dsp); - if (ret) { - dev_dbg(cs35l56->base.dev, "%s: cs_dsp_run ret %d\n", __func__, ret); - goto err; - } - if (cs35l56->base.secured) { ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_REINIT); if (ret) @@ -591,6 +585,11 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) regmap_clear_bits(cs35l56->base.regmap, CS35L56_PROTECTION_STATUS, CS35L56_FIRMWARE_MISSING); cs35l56->base.fw_patched = true; + + ret = cs_dsp_run(&cs35l56->cs_dsp); + if (ret) + dev_dbg(cs35l56->base.dev, "%s: cs_dsp_run ret %d\n", __func__, ret); + err: pm_runtime_put(cs35l56->base.dev); mutex_unlock(&cs35l56->base.irq_lock);
Only call regcache_mark_dirty() in cs35l56_hda_fw_load() if the CS35L56 was SYSTEM_RESET.
recache_mark_dirty() changes the behaviour of regcache_sync() to write out cache values that are not the default value, and skip cache values that are the default.
AUDIO_REINIT does not reset the registers. regcache_mark_dirty() after AUDIO_REINIT could cause the regcache_sync() to sync registers incorrectly because it will assume that all registers have reset to default.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/pci/hda/cs35l56_hda.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index c3acda2daeeb..fda716e0db09 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -569,6 +569,7 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) } else { /* Reset the device and wait for it to boot */ cs35l56_system_reset(&cs35l56->base, false); + regcache_mark_dirty(cs35l56->base.regmap); ret = cs35l56_wait_for_firmware_boot(&cs35l56->base); if (ret) goto err; @@ -579,7 +580,6 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) if (ret) goto err;
- regcache_mark_dirty(cs35l56->base.regmap); regcache_sync(cs35l56->base.regmap);
regmap_clear_bits(cs35l56->base.regmap, CS35L56_PROTECTION_STATUS,
When firmware is reloaded after a system resume cs_dsp_power_down() should be called before calling cs_dsp_power_up().
The fw_patched flag should also be cleared and only set again if the firmware download succeeded.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/pci/hda/cs35l56_hda.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index fda716e0db09..b6b8cb21da75 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -527,6 +527,12 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) char *wmfw_filename = NULL; int ret = 0;
+ /* Prepare for a new DSP power-up */ + if (cs35l56->base.fw_patched) + cs_dsp_power_down(&cs35l56->cs_dsp); + + cs35l56->base.fw_patched = false; + cs35l56_hda_request_firmware_files(cs35l56, &wmfw_firmware, &wmfw_filename, &coeff_firmware, &coeff_filename);
Always call cs_dsp_power_up() and cs_dsp_run() in cs35l56_hda_fw_load() even if there aren't any firmware files to download. Also, if there aren't any firmware files to download there is no need to do cs35l56_firmware_shutdown() and cs35l56_system_reset().
If there aren't any firmware files there's no need to write anything to the CS35L56 registers to make it work - it will already be running the ROM firmware. So it's not strictly necessary to start cs_dsp.
But it's perfectly ok to call cs_dsp_power_up() and cs_dsp_run() without downloading any firmware. This avoids having to support a state where audio is playing but cs_dsp is not running.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/pci/hda/cs35l56_hda.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index b6b8cb21da75..2870f82bfa45 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -536,10 +536,6 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) cs35l56_hda_request_firmware_files(cs35l56, &wmfw_firmware, &wmfw_filename, &coeff_firmware, &coeff_filename);
- /* Nothing to do - no firmware files were found to download */ - if (!wmfw_filename && !coeff_filename) - return 0; - mutex_lock(&cs35l56->base.irq_lock); pm_runtime_get_sync(cs35l56->base.dev);
@@ -549,7 +545,7 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) * shutdown the firmware to apply them and can use the lower cost * reinit sequence instead. */ - if (!cs35l56->base.secured) { + if (!cs35l56->base.secured && (wmfw_firmware || coeff_firmware)) { ret = cs35l56_firmware_shutdown(&cs35l56->base); if (ret) goto err; @@ -572,8 +568,8 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_REINIT); if (ret) goto err; - } else { - /* Reset the device and wait for it to boot */ + } else if (wmfw_firmware || coeff_firmware) { + /* If we downloaded firmware, reset the device and wait for it to boot */ cs35l56_system_reset(&cs35l56->base, false); regcache_mark_dirty(cs35l56->base.regmap); ret = cs35l56_wait_for_firmware_boot(&cs35l56->base);
In cs35l56_hda_unbind() cs_dsp_power_down() must be called to cleanup before calling cs_dsp_remove cs_dsp_remove().
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/pci/hda/cs35l56_hda.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index 2870f82bfa45..e8c41a4a0c40 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -649,6 +649,9 @@ static void cs35l56_hda_unbind(struct device *dev, struct device *master, void * debugfs_remove_recursive(cs35l56->debugfs_root); #endif
+ if (cs35l56->base.fw_patched) + cs_dsp_power_down(&cs35l56->cs_dsp); + cs_dsp_remove(&cs35l56->cs_dsp);
if (comps[cs35l56->index].dev == dev)
If cs35l56_hda_fw_load() successfully called cs_dsp_power_up() the error path must balance that with a call to cs_dsp_power_down().
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/pci/hda/cs35l56_hda.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index e8c41a4a0c40..803fa2da9ea4 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -567,20 +567,20 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) if (cs35l56->base.secured) { ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_REINIT); if (ret) - goto err; + goto err_powered_up; } else if (wmfw_firmware || coeff_firmware) { /* If we downloaded firmware, reset the device and wait for it to boot */ cs35l56_system_reset(&cs35l56->base, false); regcache_mark_dirty(cs35l56->base.regmap); ret = cs35l56_wait_for_firmware_boot(&cs35l56->base); if (ret) - goto err; + goto err_powered_up; }
/* Disable auto-hibernate so that runtime_pm has control */ ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_PREVENT_AUTO_HIBERNATE); if (ret) - goto err; + goto err_powered_up;
regcache_sync(cs35l56->base.regmap);
@@ -592,6 +592,9 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) if (ret) dev_dbg(cs35l56->base.dev, "%s: cs_dsp_run ret %d\n", __func__, ret);
+err_powered_up: + if (!cs35l56->base.fw_patched) + cs_dsp_power_down(&cs35l56->cs_dsp); err: pm_runtime_put(cs35l56->base.dev); mutex_unlock(&cs35l56->base.irq_lock);
A RAM firmware can only be downloaded if the CS35L56 is currently running from ROM firmware. The driver must not try to overwrite the RAM if the CS35L56 is already running from that RAM.
Firmware can be downloaded in these two cases:
- The BIOS has already patched the firmware (secured mode). In this case the firmware files will only contain tunings that are safe to overwrite.
- The CS35L56 is running the built-in ROM firmware.
After a RAM firmware has been downloaded it can only be cleared by hard resetting CS35L56. Some systems only hard-reset during power-on and do not give the driver control of hard reset.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/pci/hda/cs35l56_hda.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index 803fa2da9ea4..8f1665d38c92 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -525,6 +525,7 @@ 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; int ret = 0;
/* Prepare for a new DSP power-up */ @@ -533,11 +534,28 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
cs35l56->base.fw_patched = false;
- cs35l56_hda_request_firmware_files(cs35l56, &wmfw_firmware, &wmfw_filename, - &coeff_firmware, &coeff_filename); + 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. + */ + if (cs35l56->base.secured || firmware_missing) { + cs35l56_hda_request_firmware_files(cs35l56, &wmfw_firmware, &wmfw_filename, + &coeff_firmware, &coeff_filename); + }
mutex_lock(&cs35l56->base.irq_lock); - pm_runtime_get_sync(cs35l56->base.dev);
/* * When the device is running in secure mode the firmware files can @@ -596,11 +614,12 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) if (!cs35l56->base.fw_patched) cs_dsp_power_down(&cs35l56->cs_dsp); err: - pm_runtime_put(cs35l56->base.dev); mutex_unlock(&cs35l56->base.irq_lock);
cs35l56_hda_release_firmware_files(wmfw_firmware, wmfw_filename, coeff_firmware, coeff_filename); +err_pm_put: + pm_runtime_put(cs35l56->base.dev);
return ret; }
A tuning patch is always needed to enable the ASP audio port. If the BIOS did not patch the firmware, then it is mandatory to have a .bin file.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/pci/hda/cs35l56_hda.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index 8f1665d38c92..004740509dbd 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -555,6 +555,16 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) &coeff_firmware, &coeff_filename); }
+ /* + * If the BIOS didn't patch the firmware a bin file is mandatory to + * enable the ASP· + */ + if (!coeff_firmware && firmware_missing) { + dev_err(cs35l56->base.dev, ".bin file required but not found\n"); + ret = -ENOENT; + goto err_fw_release; + } + mutex_lock(&cs35l56->base.irq_lock);
/* @@ -615,7 +625,7 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) cs_dsp_power_down(&cs35l56->cs_dsp); err: mutex_unlock(&cs35l56->base.irq_lock); - +err_fw_release: cs35l56_hda_release_firmware_files(wmfw_firmware, wmfw_filename, coeff_firmware, coeff_filename); err_pm_put:
The ACPI nodes for CS35L56 can contain an extra I2CSerialBusV2 that is not a real device, it is an alias address.
This alias address will not be in the cirrus,dev-index array, so reject any instantions with a device address not found in the array.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/pci/hda/cs35l56_hda.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index 004740509dbd..76b9c685560b 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -852,8 +852,12 @@ static int cs35l56_hda_read_acpi(struct cs35l56_hda *cs35l56, int id) break; } } + /* + * It's not an error for the ID to be missing: for I2C there can be + * an alias address that is not a real device. So reject silently. + */ if (cs35l56->index == -1) { - dev_err(cs35l56->base.dev, "No index found in %s\n", property); + dev_dbg(cs35l56->base.dev, "No index found in %s\n", property); ret = -ENODEV; goto err; } @@ -891,7 +895,8 @@ static int cs35l56_hda_read_acpi(struct cs35l56_hda *cs35l56, int id) return 0;
err: - dev_err(cs35l56->base.dev, "Failed property %s: %d\n", property, ret); + if (ret != -ENODEV) + dev_err(cs35l56->base.dev, "Failed property %s: %d\n", property, ret);
return ret; } @@ -904,10 +909,8 @@ int cs35l56_hda_common_probe(struct cs35l56_hda *cs35l56, int id) dev_set_drvdata(cs35l56->base.dev, cs35l56);
ret = cs35l56_hda_read_acpi(cs35l56, id); - if (ret) { - dev_err_probe(cs35l56->base.dev, ret, "Platform not supported\n"); + if (ret) goto err; - }
cs35l56->amp_name = devm_kasprintf(cs35l56->base.dev, GFP_KERNEL, "AMP%d", cs35l56->index + 1);
On Mon, 31 Jul 2023 18:57:17 +0200, Richard Fitzgerald wrote:
A collection of various bugfixes to the cs35l56 driver.
Richard Fitzgerald (9): ALSA: hda/cs35l56: Complete firmware reboot before calling cs_dsp_run() ALSA: hda/cs35l56: Do not mark cache dirty after REINIT ALSA: hda/cs35l56: Call cs_dsp_power_down() before reloading firmware ALSA: hda/cs35l56: Always power-up and start cs_dsp ALSA: hda/cs35l56: Call cs_dsp_power_down() before calling cs_dsp_remove() ALSA: hda/cs35l56: cs_dsp_power_down() on cs35l56_hda_fw_load() error path ALSA: hda/cs35l56: Do not download firmware over existing RAM firmware ALSA: hda/cs35l56: Fail if .bin not found and firmware not patched ALSA: hda/cs35l56: Reject I2C alias addresses
Applied all patches now. Thanks.
Takashi
participants (2)
-
Richard Fitzgerald -
Takashi Iwai