[PATCH 0/9] ALSA: hda/cs35l56: Various bugfixes

[PATCH 0/9] ALSA: hda/cs35l56: Various bugfixes

[Richard Fitzgerald]

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(-)

-- 
2.30.2

[PATCH 1/9] ALSA: hda/cs35l56: Complete firmware reboot before calling cs_dsp_run()

[Richard Fitzgerald]

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);
-- 
2.30.2

[PATCH 2/9] ALSA: hda/cs35l56: Do not mark cache dirty after REINIT

[Richard Fitzgerald]

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,
-- 
2.30.2

[PATCH 3/9] ALSA: hda/cs35l56: Call cs_dsp_power_down() before reloading firmware

[Richard Fitzgerald]

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);
 
-- 
2.30.2

[PATCH 4/9] ALSA: hda/cs35l56: Always power-up and start cs_dsp

[Richard Fitzgerald]

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);
-- 
2.30.2

[PATCH 5/9] ALSA: hda/cs35l56: Call cs_dsp_power_down() before calling cs_dsp_remove()

[Richard Fitzgerald]

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)
-- 
2.30.2

[PATCH 6/9] ALSA: hda/cs35l56: cs_dsp_power_down() on cs35l56_hda_fw_load() error path

[Richard Fitzgerald]

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);
-- 
2.30.2

[PATCH 7/9] ALSA: hda/cs35l56: Do not download firmware over existing RAM firmware

[Richard Fitzgerald]

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;
 }
-- 
2.30.2

[PATCH 8/9] ALSA: hda/cs35l56: Fail if .bin not found and firmware not patched

[Richard Fitzgerald]

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:
-- 
2.30.2

[PATCH 9/9] ALSA: hda/cs35l56: Reject I2C alias addresses

[Richard Fitzgerald]

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);
-- 
2.30.2

Re: [PATCH 0/9] ALSA: hda/cs35l56: Various bugfixes

[Takashi Iwai]

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