[PATCH 0/3] ASoC: cs35l56: Bugfixes and efficiency improvement

[PATCH 0/3] ASoC: cs35l56: Bugfixes and efficiency improvement

[Richard Fitzgerald]

First two patches are bugfixes.
Third patch skips the overhead of rebooting the amp after applying
firmware files when we know that it isn't necessary.

Simon Trimmer (3):
  ASoC: cs35l56: Move DSP part string generation so that it is done only
    once
  ASoC: cs35l56: sdw_write_no_pm() should be performed under a
    pm_runtime request
  ASoC: cs35l56: In secure mode skip SHUTDOWN and RESET around fw
    download

 include/sound/cs35l56.h    |  1 +
 sound/soc/codecs/cs35l56.c | 65 +++++++++++++++++++++++++++-----------
 2 files changed, 47 insertions(+), 19 deletions(-)

-- 
2.30.2

[PATCH 1/3] ASoC: cs35l56: Move DSP part string generation so that it is done only once

[Richard Fitzgerald]

From: Simon Trimmer <simont@opensource.cirrus.com>

Each time we go through dsp_work() it does a devm_kasprintf() to
allocate memory to hold the part name string. It's not strictly a memory
leak because devm will free it all if the driver is removed. But we keep
allocating more and more memory to hold the same string.

Move the allocation so that it is performed after the version and
secured state information is gathered and handle allocation errors.

Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com>
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l56.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index d1677d76d018..906aa416879b 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -837,12 +837,6 @@ static void cs35l56_dsp_work(struct work_struct *work)
 	if (!cs35l56->init_done)
 		return;
 
-	cs35l56->dsp.part = devm_kasprintf(cs35l56->dev, GFP_KERNEL, "cs35l56%s-%02x",
-					   cs35l56->secured ? "s" : "", cs35l56->rev);
-
-	if (!cs35l56->dsp.part)
-		return;
-
 	pm_runtime_get_sync(cs35l56->dev);
 
 	/*
@@ -1508,6 +1502,12 @@ int cs35l56_init(struct cs35l56_private *cs35l56)
 	dev_info(cs35l56->dev, "Cirrus Logic CS35L56%s Rev %02X OTP%d\n",
 		 cs35l56->secured ? "s" : "", cs35l56->rev, otpid);
 
+	/* Populate the DSP information with the revision and security state */
+	cs35l56->dsp.part = devm_kasprintf(cs35l56->dev, GFP_KERNEL, "cs35l56%s-%02x",
+					   cs35l56->secured ? "s" : "", cs35l56->rev);
+	if (!cs35l56->dsp.part)
+		return -ENOMEM;
+
 	/* Wake source and *_BLOCKED interrupts default to unmasked, so mask them */
 	regmap_write(cs35l56->regmap, CS35L56_IRQ1_MASK_20, 0xffffffff);
 	regmap_update_bits(cs35l56->regmap, CS35L56_IRQ1_MASK_1,
-- 
2.30.2

[PATCH 2/3] ASoC: cs35l56: sdw_write_no_pm() should be performed under a pm_runtime request

[Richard Fitzgerald]

From: Simon Trimmer <simont@opensource.cirrus.com>

SoundWire bus accesses must be performed under the guard of a pm_runtime
request, in this case the write was being performed just after the
request had been put() and so the bus could not be guaranteed to be
available.

Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com>
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l56.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 906aa416879b..255c442308f2 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -904,15 +904,15 @@ static void cs35l56_dsp_work(struct work_struct *work)
 err_unlock:
 	mutex_unlock(&cs35l56->irq_lock);
 err:
-	pm_runtime_mark_last_busy(cs35l56->dev);
-	pm_runtime_put_autosuspend(cs35l56->dev);
-
 	/* Re-enable SoundWire interrupts */
 	if (cs35l56->sdw_peripheral) {
 		cs35l56->sdw_irq_no_unmask = false;
 		sdw_write_no_pm(cs35l56->sdw_peripheral, CS35L56_SDW_GEN_INT_MASK_1,
 				CS35L56_SDW_INT_MASK_CODEC_IRQ);
 	}
+
+	pm_runtime_mark_last_busy(cs35l56->dev);
+	pm_runtime_put_autosuspend(cs35l56->dev);
 }
 
 static int cs35l56_component_probe(struct snd_soc_component *component)
-- 
2.30.2

[PATCH 3/3] ASoC: cs35l56: In secure mode skip SHUTDOWN and RESET around fw download

[Richard Fitzgerald]

From: Simon Trimmer <simont@opensource.cirrus.com>

If the device is in secure mode it's unnecessary to send a SHUTDOWN and
SYSTEM_RESET around the firmware download. It could only be patching
insecure tunings. A tuning patch doesn't need a SHUTDOWN and only needs
a REINIT afterwards. This will reduce the overhead of exiting system
suspend in secure mode.

Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com>
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 include/sound/cs35l56.h    |  1 +
 sound/soc/codecs/cs35l56.c | 47 ++++++++++++++++++++++++++++++--------
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/include/sound/cs35l56.h b/include/sound/cs35l56.h
index 002042b1c73c..1f9713d7ca76 100644
--- a/include/sound/cs35l56.h
+++ b/include/sound/cs35l56.h
@@ -223,6 +223,7 @@
 
 #define CS35L56_MBOX_CMD_AUDIO_PLAY			0x0B000001
 #define CS35L56_MBOX_CMD_AUDIO_PAUSE			0x0B000002
+#define CS35L56_MBOX_CMD_AUDIO_REINIT			0x0B000003
 #define CS35L56_MBOX_CMD_HIBERNATE_NOW			0x02000001
 #define CS35L56_MBOX_CMD_WAKEUP				0x02000002
 #define CS35L56_MBOX_CMD_PREVENT_AUTO_HIBERNATE		0x02000003
diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 255c442308f2..3c07bd1e959e 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -825,19 +825,23 @@ static void cs35l56_system_reset(struct cs35l56_private *cs35l56)
 	regcache_cache_only(cs35l56->regmap, false);
 }
 
-static void cs35l56_dsp_work(struct work_struct *work)
+static void cs35l56_secure_patch(struct cs35l56_private *cs35l56)
+{
+	int ret;
+
+	/* Use wm_adsp to load and apply the firmware patch and coefficient files */
+	ret = wm_adsp_power_up(&cs35l56->dsp);
+	if (ret)
+		dev_dbg(cs35l56->dev, "%s: wm_adsp_power_up ret %d\n", __func__, ret);
+	else
+		cs35l56_mbox_send(cs35l56, CS35L56_MBOX_CMD_AUDIO_REINIT);
+}
+
+static void cs35l56_patch(struct cs35l56_private *cs35l56)
 {
-	struct cs35l56_private *cs35l56 = container_of(work,
-						       struct cs35l56_private,
-						       dsp_work);
 	unsigned int reg;
 	unsigned int val;
-	int ret = 0;
-
-	if (!cs35l56->init_done)
-		return;
-
-	pm_runtime_get_sync(cs35l56->dev);
+	int ret;
 
 	/*
 	 * Disable SoundWire interrupts to prevent race with IRQ work.
@@ -910,6 +914,29 @@ static void cs35l56_dsp_work(struct work_struct *work)
 		sdw_write_no_pm(cs35l56->sdw_peripheral, CS35L56_SDW_GEN_INT_MASK_1,
 				CS35L56_SDW_INT_MASK_CODEC_IRQ);
 	}
+}
+
+static void cs35l56_dsp_work(struct work_struct *work)
+{
+	struct cs35l56_private *cs35l56 = container_of(work,
+						       struct cs35l56_private,
+						       dsp_work);
+
+	if (!cs35l56->init_done)
+		return;
+
+	pm_runtime_get_sync(cs35l56->dev);
+
+	/*
+	 * 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 (cs35l56->secured)
+		cs35l56_secure_patch(cs35l56);
+	else
+		cs35l56_patch(cs35l56);
 
 	pm_runtime_mark_last_busy(cs35l56->dev);
 	pm_runtime_put_autosuspend(cs35l56->dev);
-- 
2.30.2

Re: [PATCH 0/3] ASoC: cs35l56: Bugfixes and efficiency improvement

[Mark Brown]

On Thu, 18 May 2023 16:02:47 +0100, Richard Fitzgerald wrote:
> First two patches are bugfixes.
> Third patch skips the overhead of rebooting the amp after applying
> firmware files when we know that it isn't necessary.
> 
> Simon Trimmer (3):
>   ASoC: cs35l56: Move DSP part string generation so that it is done only
>     once
>   ASoC: cs35l56: sdw_write_no_pm() should be performed under a
>     pm_runtime request
>   ASoC: cs35l56: In secure mode skip SHUTDOWN and RESET around fw
>     download
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/3] ASoC: cs35l56: Move DSP part string generation so that it is done only once
      commit: 608f1b0dbddec6b2fd766c10bcce2b651995e936
[2/3] ASoC: cs35l56: sdw_write_no_pm() should be performed under a pm_runtime request
      commit: c9001a2754528fa5da20e8674b3afbd8c134cc91
[3/3] ASoC: cs35l56: In secure mode skip SHUTDOWN and RESET around fw download
      commit: 1a8edfcffa2803afc0ef3a6a48819230cdbda2c9

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