[PATCH 0/6] ASoC: cs35l56: Add system suspend handling

[PATCH 0/6] ASoC: cs35l56: Add system suspend handling

[Richard Fitzgerald]

This set of patches adds handling for system suspend.
Patches 1..4 make some code changes that simplify the
suspend implementation, mainly to avoid race conditions.

There are two seperate aspects to suspend, and these have
been done as two patches:
- the main suspend-resume handling,
- re-loading the firmware if necessary after resume.

Richard Fitzgerald (6):
  ASoC: cs35l56: Remove quick-cancelling of dsp_work()
  ASoC: cs35l56: Use DAPM widget for firmware PLAY/PAUSE
  ASoC: cs35l56: Skip first init_completion wait in dsp_work if
    init_done
  ASoC: cs35l56: Always wait for firmware boot in runtime-resume
  ASoC: cs35l56: Add basic system suspend handling
  ASoC: cs35l56: Re-patch firmware after system suspend

 include/sound/cs35l56.h        |   4 +
 sound/soc/codecs/cs35l56-sdw.c |  36 ++++
 sound/soc/codecs/cs35l56.c     | 335 ++++++++++++++++++++++++++-------
 sound/soc/codecs/cs35l56.h     |   7 +-
 4 files changed, 308 insertions(+), 74 deletions(-)

-- 
2.30.2

[PATCH 1/6] ASoC: cs35l56: Remove quick-cancelling of dsp_work()

[Richard Fitzgerald]

Delete the 'removing' flag and don't kick init_completion to make a
quick cancel of dsp_work(). Just let it timeout on the wait for the
completion.

Simplify the code to standard cancelling or flushing of the work.
This avoids introducing corner cases from a layer of custom signalling.
It also avoids potential race conditions when system-suspend handling
is added.

Unless the hardware is broken, the dsp_work() will already have started
and passed the completion before the driver would want to cancel it.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l56.c | 8 +-------
 sound/soc/codecs/cs35l56.h | 1 -
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index d97b465f0d3c..675aad8e909f 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -867,7 +867,7 @@ static void cs35l56_dsp_work(struct work_struct *work)
 		goto complete;
 	}
 
-	if (!cs35l56->init_done || cs35l56->removing)
+	if (!cs35l56->init_done)
 		goto complete;
 
 	cs35l56->dsp.part = devm_kasprintf(cs35l56->dev, GFP_KERNEL, "cs35l56%s-%02x",
@@ -917,9 +917,6 @@ static void cs35l56_dsp_work(struct work_struct *work)
 		goto err;
 	}
 
-	if (cs35l56->removing)
-		goto err;
-
 	mutex_lock(&cs35l56->irq_lock);
 
 	init_completion(&cs35l56->init_completion);
@@ -967,7 +964,6 @@ static int cs35l56_component_probe(struct snd_soc_component *component)
 
 	BUILD_BUG_ON(ARRAY_SIZE(cs35l56_tx_input_texts) != ARRAY_SIZE(cs35l56_tx_input_values));
 
-	cs35l56->removing = false;
 	cs35l56->component = component;
 	wm_adsp2_component_probe(&cs35l56->dsp, component);
 
@@ -984,8 +980,6 @@ static void cs35l56_component_remove(struct snd_soc_component *component)
 {
 	struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(component);
 
-	cs35l56->removing = true;
-	complete(&cs35l56->init_completion);
 	cancel_work_sync(&cs35l56->dsp_work);
 }
 
diff --git a/sound/soc/codecs/cs35l56.h b/sound/soc/codecs/cs35l56.h
index efc4b99180f9..dc91cd7d877f 100644
--- a/sound/soc/codecs/cs35l56.h
+++ b/sound/soc/codecs/cs35l56.h
@@ -49,7 +49,6 @@ struct cs35l56_private {
 	bool soft_resetting;
 	bool init_done;
 	bool sdw_attached;
-	bool removing;
 	bool fw_patched;
 	bool can_hibernate;
 	struct completion init_completion;
-- 
2.30.2

[PATCH 2/6] ASoC: cs35l56: Use DAPM widget for firmware PLAY/PAUSE

[Richard Fitzgerald]

If we use a DAPM widget instead of mute_stream() to send the
PLAY command we can issue the plays to multiple amps in parallel.
With mute_stream each codec driver instance is called one at a
time so we get N * PS0 delay time.

DAPM does each stage on every widget in a card before moving to
the next stage. So all amps will do the PRE_PMU then all will do
the POST_PMU. The PLAY is sent in the PRE_PMU so that they all
power-up in parallel. After the PS0 wait in the first POST_PMU
all the other amps will also be ready so there won't be any extra
delay, or it will be negligible.

There's also no point waiting for the MBOX ack in the PRE_PMU.
We won't see a PS0 state in POST_PMU if it didn't ack the PLAY
command. So we can save a little extra time.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l56.c | 105 +++++++++++++++++++------------------
 1 file changed, 55 insertions(+), 50 deletions(-)

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 675aad8e909f..997a5c5acaab 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -32,6 +32,23 @@
 static int cs35l56_dsp_event(struct snd_soc_dapm_widget *w,
 			     struct snd_kcontrol *kcontrol, int event);
 
+static int cs35l56_mbox_send(struct cs35l56_private *cs35l56, unsigned int command)
+{
+	unsigned int val;
+	int ret;
+
+	regmap_write(cs35l56->regmap, CS35L56_DSP_VIRTUAL1_MBOX_1, command);
+	ret = regmap_read_poll_timeout(cs35l56->regmap, CS35L56_DSP_VIRTUAL1_MBOX_1,
+				       val, (val == 0),
+				       CS35L56_MBOX_POLL_US, CS35L56_MBOX_TIMEOUT_US);
+	if (ret) {
+		dev_warn(cs35l56->dev, "MBOX command %#x failed: %d\n", command, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int cs35l56_wait_dsp_ready(struct cs35l56_private *cs35l56)
 {
 	int ret;
@@ -182,10 +199,45 @@ static SOC_VALUE_ENUM_SINGLE_DECL(cs35l56_sdw1tx6_enum,
 static const struct snd_kcontrol_new sdw1_tx6_mux =
 	SOC_DAPM_ENUM("SDW1TX6 SRC", cs35l56_sdw1tx6_enum);
 
+static int cs35l56_play_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);
+	unsigned int val;
+	int ret;
+
+	dev_dbg(cs35l56->dev, "play: %d\n", event);
+
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		/* Don't wait for ACK, we check in POST_PMU that it completed */
+		return regmap_write(cs35l56->regmap, CS35L56_DSP_VIRTUAL1_MBOX_1,
+				    CS35L56_MBOX_CMD_AUDIO_PLAY);
+	case SND_SOC_DAPM_POST_PMU:
+		/* Wait for firmware to enter PS0 power state */
+		ret = regmap_read_poll_timeout(cs35l56->regmap,
+					       CS35L56_TRANSDUCER_ACTUAL_PS,
+					       val, (val == CS35L56_PS0),
+					       CS35L56_PS0_POLL_US,
+					       CS35L56_PS0_TIMEOUT_US);
+		if (ret)
+			dev_err(cs35l56->dev, "PS0 wait failed: %d\n", ret);
+		return ret;
+	case SND_SOC_DAPM_POST_PMD:
+		return cs35l56_mbox_send(cs35l56, CS35L56_MBOX_CMD_AUDIO_PAUSE);
+	default:
+		return 0;
+	}
+}
+
 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("PLAY", SND_SOC_NOPM, 0, 0, cs35l56_play_event,
+			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
+
 	SND_SOC_DAPM_OUT_DRV("AMP", SND_SOC_NOPM, 0, 0, NULL, 0),
 	SND_SOC_DAPM_OUTPUT("SPK"),
 
@@ -252,6 +304,9 @@ static const struct snd_soc_dapm_route cs35l56_audio_map[] = {
 	{ "AMP", NULL, "VDD_B" },
 	{ "AMP", NULL, "VDD_AMP" },
 
+	{ "ASP1 Playback", NULL, "PLAY" },
+	{ "SDW1 Playback", NULL, "PLAY" },
+
 	{ "ASP1RX1", NULL, "ASP1 Playback" },
 	{ "ASP1RX2", NULL, "ASP1 Playback" },
 	{ "DSP1", NULL, "ASP1RX1" },
@@ -288,23 +343,6 @@ static const struct snd_soc_dapm_route cs35l56_audio_map[] = {
 	{ "SDW1 Capture", NULL, "SDW1 TX6 Source" },
 };
 
-static int cs35l56_mbox_send(struct cs35l56_private *cs35l56, unsigned int command)
-{
-	unsigned int val;
-	int ret;
-
-	regmap_write(cs35l56->regmap, CS35L56_DSP_VIRTUAL1_MBOX_1, command);
-	ret = regmap_read_poll_timeout(cs35l56->regmap, CS35L56_DSP_VIRTUAL1_MBOX_1,
-				       val, (val == 0),
-				       CS35L56_MBOX_POLL_US, CS35L56_MBOX_TIMEOUT_US);
-	if (ret) {
-		dev_warn(cs35l56->dev, "MBOX command %#x failed: %d\n", command, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
 static int cs35l56_dsp_event(struct snd_soc_dapm_widget *w,
 			     struct snd_kcontrol *kcontrol, int event)
 {
@@ -611,43 +649,11 @@ static int cs35l56_asp_dai_set_sysclk(struct snd_soc_dai *dai,
 	return 0;
 }
 
-static int cs35l56_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
-{
-	struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(dai->component);
-	unsigned int val;
-	int ret;
-
-	dev_dbg(cs35l56->dev, "%s: %d %s\n", __func__, stream, mute ? "mute" : "unmute");
-
-	if (stream != SNDRV_PCM_STREAM_PLAYBACK)
-		return 0;
-
-	if (mute) {
-		ret = cs35l56_mbox_send(cs35l56, CS35L56_MBOX_CMD_AUDIO_PAUSE);
-	} else {
-		ret = cs35l56_mbox_send(cs35l56, CS35L56_MBOX_CMD_AUDIO_PLAY);
-		if (ret == 0) {
-			/* Wait for firmware to enter PS0 power state */
-			ret = regmap_read_poll_timeout(cs35l56->regmap,
-						       CS35L56_TRANSDUCER_ACTUAL_PS,
-						       val, (val == CS35L56_PS0),
-						       CS35L56_PS0_POLL_US,
-						       CS35L56_PS0_TIMEOUT_US);
-			if (ret)
-				dev_err(cs35l56->dev, "PS0 wait failed: %d\n", ret);
-			ret = 0;
-		}
-	}
-
-	return ret;
-}
-
 static const struct snd_soc_dai_ops cs35l56_ops = {
 	.set_fmt = cs35l56_asp_dai_set_fmt,
 	.set_tdm_slot = cs35l56_asp_dai_set_tdm_slot,
 	.hw_params = cs35l56_asp_dai_hw_params,
 	.set_sysclk = cs35l56_asp_dai_set_sysclk,
-	.mute_stream = cs35l56_mute_stream,
 };
 
 static void cs35l56_sdw_dai_shutdown(struct snd_pcm_substream *substream,
@@ -749,7 +755,6 @@ static const struct snd_soc_dai_ops cs35l56_sdw_dai_ops = {
 	.shutdown = cs35l56_sdw_dai_shutdown,
 	.hw_params = cs35l56_sdw_dai_hw_params,
 	.hw_free = cs35l56_sdw_dai_hw_free,
-	.mute_stream = cs35l56_mute_stream,
 	.set_stream = cs35l56_sdw_dai_set_stream,
 };
 
-- 
2.30.2

[PATCH 3/6] ASoC: cs35l56: Skip first init_completion wait in dsp_work if init_done

[Richard Fitzgerald]

At the start of dsp_work() only wait for init_completion if !init_done.
This allows system suspend to re-queue dsp_work() without having to
do a dummy complete() of init_completion.

A dummy completion in system suspend would have to be conditional on
init_done. But that would create a possible race condition between our
system resume and cs35l56_init() in the corner case that we suspend right
after the SoundWire core has enumerated and reported ATTACHED.

It is safer and simpler to have cs35l56_init() as the only place that
init_completion is completed, and dsp_work() as the only place that
it is consumed.

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

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 997a5c5acaab..62c44276c121 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -866,7 +866,8 @@ static void cs35l56_dsp_work(struct work_struct *work)
 	unsigned int val;
 	int ret = 0;
 
-	if (!wait_for_completion_timeout(&cs35l56->init_completion,
+	if (!cs35l56->init_done &&
+	    !wait_for_completion_timeout(&cs35l56->init_completion,
 					 msecs_to_jiffies(5000))) {
 		dev_err(cs35l56->dev, "%s: init_completion timed out\n", __func__);
 		goto complete;
-- 
2.30.2

[PATCH 4/6] ASoC: cs35l56: Always wait for firmware boot in runtime-resume

[Richard Fitzgerald]

When we are resuming from a system suspend the CS35L56 has probably
been hard reset (usually a power-on reset). So we must wait for the
firmware to boot. On SoundWire we also need it to re-initialize before
we can read the registers to check the CS35L56 state.

The simplest way to handle this is for runtime-resume to always wait
for firmware boot. If the firmware is already booted the overhead is
only one register read.

The system-resume will have to runtime-resume the driver anyway before
attempting any register access. So this will automatically include the
wait for initialization on SoundWire.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l56.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 62c44276c121..74abcf1c604a 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -1102,10 +1102,8 @@ int cs35l56_runtime_resume_common(struct cs35l56_private *cs35l56)
 	unsigned int val;
 	int ret;
 
-	if (!cs35l56->can_hibernate) {
-		regcache_cache_only(cs35l56->regmap, false);
+	if (!cs35l56->can_hibernate)
 		goto out_sync;
-	}
 
 	if (!cs35l56->sdw_peripheral) {
 		/*
@@ -1120,19 +1118,19 @@ int cs35l56_runtime_resume_common(struct cs35l56_private *cs35l56)
 			     CS35L56_CONTROL_PORT_READY_US + 400);
 	}
 
-	regcache_cache_only(cs35l56->regmap, false);
-
-	ret = cs35l56_wait_for_firmware_boot(cs35l56);
-	if (ret) {
-		dev_err(cs35l56->dev, "Hibernate wake failed: %d\n", ret);
-		goto err;
-	}
-
-	ret = cs35l56_mbox_send(cs35l56, CS35L56_MBOX_CMD_PREVENT_AUTO_HIBERNATE);
-	if (ret)
-		goto err;
-
 out_sync:
+	regcache_cache_only(cs35l56->regmap, false);
+
+	ret = cs35l56_wait_for_firmware_boot(cs35l56);
+	if (ret) {
+		dev_err(cs35l56->dev, "Hibernate wake failed: %d\n", ret);
+		goto err;
+	}
+
+	ret = cs35l56_mbox_send(cs35l56, CS35L56_MBOX_CMD_PREVENT_AUTO_HIBERNATE);
+	if (ret)
+		goto err;
+
 	/* BOOT_DONE will be 1 if the amp reset */
 	regmap_read(cs35l56->regmap, CS35L56_IRQ1_EINT_4, &val);
 	if (val & CS35L56_OTP_BOOT_DONE_MASK) {
-- 
2.30.2

[PATCH 5/6] ASoC: cs35l56: Add basic system suspend handling

[Richard Fitzgerald]

This adds the main handling for system suspend but does not handle
re-patching the firmware after system resume.

This is a multi-stage suspend and resume because if there is a
RESET line it is almost certain that it will be shared by all the
amps. So every amp must have done its suspend before we can
assert RESET. Likewise we must de-assert RESET before the amps
can resume.

It's preferable to assert RESET before we turning off regulators, and
while they power up.

The actual suspend and resume is done by using the pair
pm_runtime_force_suspend() and pm_runtime_force_resume() to
re-use our runtime suspend/resume sequences.

pm_runtime_force_suspend() will disable our pm_runtime. If we were
runtime-resumed it calls our runtime_suspend().

pm_runtime_force_resume() re-enables pm_runtime and if we were
originally runtime-resumed before the pm_runtime_force_suspend()
it calls our runtime_resume(). Otherwise it leaves us
runtime-suspended.

The general process is therefore:

 suspend() -> finish dsp_work and then run our runtime_suspend
 suspend_late() -> assert RESET and turn off supplies
 resume_early() -> enable supplies and de-assert RESET
 resume() -> pm_runtime_force_resume()

In addition, to prevent the IRQ handler running in the period
between pm_runtime_force_suspend() and pm_runtime_force_resume()
the parent IRQ is temporarily disabled:
 - from suspend until suspend_noirq
 - from resume_noirq until resume

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l56-sdw.c |  26 +++++++
 sound/soc/codecs/cs35l56.c     | 126 +++++++++++++++++++++++++++++++++
 sound/soc/codecs/cs35l56.h     |   6 ++
 3 files changed, 158 insertions(+)

diff --git a/sound/soc/codecs/cs35l56-sdw.c b/sound/soc/codecs/cs35l56-sdw.c
index 448ef3609f4c..947d4e5f4dc9 100644
--- a/sound/soc/codecs/cs35l56-sdw.c
+++ b/sound/soc/codecs/cs35l56-sdw.c
@@ -450,6 +450,29 @@ static int __maybe_unused cs35l56_sdw_runtime_resume(struct device *dev)
 	return 0;
 }
 
+static int __maybe_unused cs35l56_sdw_system_suspend(struct device *dev)
+{
+	struct cs35l56_private *cs35l56 = dev_get_drvdata(dev);
+
+	if (!cs35l56->init_done)
+		return 0;
+
+	/*
+	 * Disable SoundWire interrupts.
+	 * Flush - don't cancel because that could leave an unbalanced pm_runtime_get.
+	 */
+	cs35l56->sdw_irq_no_unmask = true;
+	flush_work(&cs35l56->sdw_irq_work);
+
+	/* Mask interrupts and flush in case sdw_irq_work was queued again */
+	sdw_write_no_pm(cs35l56->sdw_peripheral, CS35L56_SDW_GEN_INT_MASK_1, 0);
+	sdw_read_no_pm(cs35l56->sdw_peripheral, CS35L56_SDW_GEN_INT_STAT_1);
+	sdw_write_no_pm(cs35l56->sdw_peripheral, CS35L56_SDW_GEN_INT_STAT_1, 0xFF);
+	flush_work(&cs35l56->sdw_irq_work);
+
+	return cs35l56_system_suspend(dev);
+}
+
 static int cs35l56_sdw_probe(struct sdw_slave *peripheral, const struct sdw_device_id *id)
 {
 	struct device *dev = &peripheral->dev;
@@ -499,6 +522,9 @@ static int cs35l56_sdw_remove(struct sdw_slave *peripheral)
 
 static const struct dev_pm_ops cs35l56_sdw_pm = {
 	SET_RUNTIME_PM_OPS(cs35l56_sdw_runtime_suspend, cs35l56_sdw_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(cs35l56_sdw_system_suspend, cs35l56_system_resume)
+	LATE_SYSTEM_SLEEP_PM_OPS(cs35l56_system_suspend_late, cs35l56_system_resume_early)
+	/* NOIRQ stage not needed, SoundWire doesn't use a hard IRQ */
 };
 
 static const struct sdw_device_id cs35l56_sdw_id[] = {
diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 74abcf1c604a..eb85c27ab087 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -11,8 +11,10 @@
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
 #include <linux/math.h>
 #include <linux/module.h>
+#include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
@@ -1154,6 +1156,127 @@ int cs35l56_runtime_resume_common(struct cs35l56_private *cs35l56)
 }
 EXPORT_SYMBOL_NS_GPL(cs35l56_runtime_resume_common, SND_SOC_CS35L56_CORE);
 
+int cs35l56_system_suspend(struct device *dev)
+{
+	struct cs35l56_private *cs35l56 = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "system_suspend\n");
+
+	if (cs35l56->component)
+		flush_work(&cs35l56->dsp_work);
+
+	/*
+	 * The interrupt line is normally shared, but after we start suspending
+	 * we can't check if our device is the source of an interrupt, and can't
+	 * clear it. Prevent this race by temporarily disabling the parent irq
+	 * until we reach _no_irq.
+	 */
+	if (cs35l56->irq)
+		disable_irq(cs35l56->irq);
+
+	return pm_runtime_force_suspend(dev);
+}
+EXPORT_SYMBOL_GPL(cs35l56_system_suspend);
+
+int cs35l56_system_suspend_late(struct device *dev)
+{
+	struct cs35l56_private *cs35l56 = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "system_suspend_late\n");
+
+	/*
+	 * Assert RESET before removing supplies.
+	 * RESET is usually shared by all amps so it must not be asserted until
+	 * all driver instances have done their suspend() stage.
+	 */
+	if (cs35l56->reset_gpio) {
+		gpiod_set_value_cansleep(cs35l56->reset_gpio, 0);
+		usleep_range(CS35L56_RESET_PULSE_MIN_US, CS35L56_RESET_PULSE_MIN_US + 400);
+	}
+
+	regulator_bulk_disable(ARRAY_SIZE(cs35l56->supplies), cs35l56->supplies);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cs35l56_system_suspend_late);
+
+int cs35l56_system_suspend_no_irq(struct device *dev)
+{
+	struct cs35l56_private *cs35l56 = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "system_suspend_no_irq\n");
+
+	/* Handlers are now disabled so the parent IRQ can safely be re-enabled. */
+	if (cs35l56->irq)
+		enable_irq(cs35l56->irq);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cs35l56_system_suspend_no_irq);
+
+int cs35l56_system_resume_no_irq(struct device *dev)
+{
+	struct cs35l56_private *cs35l56 = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "system_resume_no_irq\n");
+
+	/*
+	 * WAKE interrupts unmask if the CS35L56 hibernates, which can cause
+	 * spurious interrupts, and the interrupt line is normally shared.
+	 * We can't check if our device is the source of an interrupt, and can't
+	 * clear it, until it has fully resumed. Prevent this race by temporarily
+	 * disabling the parent irq until we complete resume().
+	 */
+	if (cs35l56->irq)
+		disable_irq(cs35l56->irq);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cs35l56_system_resume_no_irq);
+
+int cs35l56_system_resume_early(struct device *dev)
+{
+	struct cs35l56_private *cs35l56 = dev_get_drvdata(dev);
+	int ret;
+
+	dev_dbg(dev, "system_resume_early\n");
+
+	/* Ensure a spec-compliant RESET pulse. */
+	if (cs35l56->reset_gpio) {
+		gpiod_set_value_cansleep(cs35l56->reset_gpio, 0);
+		usleep_range(CS35L56_RESET_PULSE_MIN_US, CS35L56_RESET_PULSE_MIN_US + 400);
+	}
+
+	/* Enable supplies before releasing RESET. */
+	ret = regulator_bulk_enable(ARRAY_SIZE(cs35l56->supplies), cs35l56->supplies);
+	if (ret) {
+		dev_err(dev, "system_resume_early failed to enable supplies: %d\n", ret);
+		return ret;
+	}
+
+	/* Release shared RESET before drivers start resume(). */
+	gpiod_set_value_cansleep(cs35l56->reset_gpio, 1);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cs35l56_system_resume_early);
+
+int cs35l56_system_resume(struct device *dev)
+{
+	struct cs35l56_private *cs35l56 = dev_get_drvdata(dev);
+	int ret;
+
+	dev_dbg(dev, "system_resume\n");
+
+	/* Undo pm_runtime_force_suspend() before re-enabling the irq */
+	ret = pm_runtime_force_resume(dev);
+	if (cs35l56->irq)
+		enable_irq(cs35l56->irq);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cs35l56_system_resume);
+
 static int cs35l56_dsp_init(struct cs35l56_private *cs35l56)
 {
 	struct wm_adsp *dsp;
@@ -1451,6 +1574,9 @@ EXPORT_SYMBOL_NS_GPL(cs35l56_remove, SND_SOC_CS35L56_CORE);
 
 const struct dev_pm_ops cs35l56_pm_ops_i2c_spi = {
 	SET_RUNTIME_PM_OPS(cs35l56_runtime_suspend, cs35l56_runtime_resume_i2c_spi, NULL)
+	SYSTEM_SLEEP_PM_OPS(cs35l56_system_suspend, cs35l56_system_resume)
+	LATE_SYSTEM_SLEEP_PM_OPS(cs35l56_system_suspend_late, cs35l56_system_resume_early)
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(cs35l56_system_suspend_no_irq, cs35l56_system_resume_no_irq)
 };
 EXPORT_SYMBOL_NS_GPL(cs35l56_pm_ops_i2c_spi, SND_SOC_CS35L56_CORE);
 
diff --git a/sound/soc/codecs/cs35l56.h b/sound/soc/codecs/cs35l56.h
index dc91cd7d877f..50278dafc9ca 100644
--- a/sound/soc/codecs/cs35l56.h
+++ b/sound/soc/codecs/cs35l56.h
@@ -67,6 +67,12 @@ extern const struct dev_pm_ops cs35l56_pm_ops_i2c_spi;
 
 int cs35l56_runtime_suspend(struct device *dev);
 int cs35l56_runtime_resume_common(struct cs35l56_private *cs35l56);
+int cs35l56_system_suspend(struct device *dev);
+int cs35l56_system_suspend_late(struct device *dev);
+int cs35l56_system_suspend_no_irq(struct device *dev);
+int cs35l56_system_resume_no_irq(struct device *dev);
+int cs35l56_system_resume_early(struct device *dev);
+int cs35l56_system_resume(struct device *dev);
 irqreturn_t cs35l56_irq(int irq, void *data);
 int cs35l56_irq_request(struct cs35l56_private *cs35l56);
 int cs35l56_common_probe(struct cs35l56_private *cs35l56);
-- 
2.30.2

[PATCH 6/6] ASoC: cs35l56: Re-patch firmware after system suspend

[Richard Fitzgerald]

Check during cs35l56_system_resume() whether the firmware patch must
be applied again.

The FIRMWARE_MISSING flag in the PROTECTION_STATUS register indicates
whether the firmware has been patched.

In non-secure mode the FIRMWARE_MISSING flag is cleared at the end of
dsp_work(). If it is set after system-resume we know that dsp_work()
must be run again.

In secure mode the pre-OS loader will have done the secure patching
and cleared the FIRMWARE_MISSING flag. So this flag does not tell us
whether firmware memory was lost. But the driver could only be
downloading non-secure tunings, which is always safe to do.

If the driver has control of RESET we will have asserted it during
suspend so the firmware patch will have been lost. The driver would only
have control of RESET in non-secure mode.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 include/sound/cs35l56.h        |  4 ++
 sound/soc/codecs/cs35l56-sdw.c | 12 +++++-
 sound/soc/codecs/cs35l56.c     | 67 +++++++++++++++++++++++++++++++++-
 3 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/include/sound/cs35l56.h b/include/sound/cs35l56.h
index 5f8ea2dfaa21..b3300bce74f4 100644
--- a/include/sound/cs35l56.h
+++ b/include/sound/cs35l56.h
@@ -95,6 +95,7 @@
 #define CS35L56_MAIN_RENDER_USER_MUTE			0x3400024
 #define CS35L56_MAIN_RENDER_USER_VOLUME			0x340002C
 #define CS35L56_MAIN_POSTURE_NUMBER			0x3400094
+#define CS35L56_PROTECTION_STATUS			0x34000D8
 #define CS35L56_TRANSDUCER_ACTUAL_PS			0x3400150
 #define CS35L56_DSP1_YMEM_UNPACKED24_6141		0x3405FF4
 #define CS35L56_DSP1_PMEM_0				0x3800000
@@ -216,6 +217,9 @@
 #define CS35L56_MAIN_POSTURE_MAX			255
 #define CS35L56_MAIN_POSTURE_MASK			CS35L56_MAIN_POSTURE_MAX
 
+/* CS35L56_PROTECTION_STATUS */
+#define CS35L56_FIRMWARE_MISSING			BIT(0)
+
 /* Software Values */
 #define CS35L56_HALO_STATE_SHUTDOWN			1
 #define CS35L56_HALO_STATE_BOOT_DONE			2
diff --git a/sound/soc/codecs/cs35l56-sdw.c b/sound/soc/codecs/cs35l56-sdw.c
index 947d4e5f4dc9..e759347423cf 100644
--- a/sound/soc/codecs/cs35l56-sdw.c
+++ b/sound/soc/codecs/cs35l56-sdw.c
@@ -473,6 +473,16 @@ static int __maybe_unused cs35l56_sdw_system_suspend(struct device *dev)
 	return cs35l56_system_suspend(dev);
 }
 
+static int __maybe_unused cs35l56_sdw_system_resume(struct device *dev)
+{
+	struct cs35l56_private *cs35l56 = dev_get_drvdata(dev);
+
+	cs35l56->sdw_irq_no_unmask = false;
+	/* runtime_resume re-enables the interrupt */
+
+	return cs35l56_system_resume(dev);
+}
+
 static int cs35l56_sdw_probe(struct sdw_slave *peripheral, const struct sdw_device_id *id)
 {
 	struct device *dev = &peripheral->dev;
@@ -522,7 +532,7 @@ static int cs35l56_sdw_remove(struct sdw_slave *peripheral)
 
 static const struct dev_pm_ops cs35l56_sdw_pm = {
 	SET_RUNTIME_PM_OPS(cs35l56_sdw_runtime_suspend, cs35l56_sdw_runtime_resume, NULL)
-	SYSTEM_SLEEP_PM_OPS(cs35l56_sdw_system_suspend, cs35l56_system_resume)
+	SYSTEM_SLEEP_PM_OPS(cs35l56_sdw_system_suspend, cs35l56_sdw_system_resume)
 	LATE_SYSTEM_SLEEP_PM_OPS(cs35l56_system_suspend_late, cs35l56_system_resume_early)
 	/* NOIRQ stage not needed, SoundWire doesn't use a hard IRQ */
 };
diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index eb85c27ab087..18e341744839 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -946,6 +946,7 @@ static void cs35l56_dsp_work(struct work_struct *work)
 		goto err_unlock;
 	}
 
+	regmap_clear_bits(cs35l56->regmap, CS35L56_PROTECTION_STATUS, CS35L56_FIRMWARE_MISSING);
 	cs35l56->fw_patched = true;
 
 err_unlock:
@@ -1026,6 +1027,8 @@ static const struct snd_soc_component_driver soc_component_dev_cs35l56 = {
 	.num_controls = ARRAY_SIZE(cs35l56_controls),
 
 	.set_bias_level = cs35l56_set_bias_level,
+
+	.suspend_bias_off = 1, /* see cs35l56_system_resume() */
 };
 
 static const struct reg_sequence cs35l56_hibernate_seq[] = {
@@ -1156,6 +1159,47 @@ int cs35l56_runtime_resume_common(struct cs35l56_private *cs35l56)
 }
 EXPORT_SYMBOL_NS_GPL(cs35l56_runtime_resume_common, SND_SOC_CS35L56_CORE);
 
+static int cs35l56_is_fw_reload_needed(struct cs35l56_private *cs35l56)
+{
+	unsigned int val;
+	int ret;
+
+	/* Nothing to re-patch if we haven't done any patching yet. */
+	if (!cs35l56->fw_patched)
+		return false;
+
+	/*
+	 * If we have control of RESET we will have asserted it so the firmware
+	 * will need re-patching.
+	 */
+	if (cs35l56->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.
+	 * Assume that tuning must be re-loaded.
+	 */
+	if (cs35l56->secured)
+		return true;
+
+	ret = pm_runtime_resume_and_get(cs35l56->dev);
+	if (ret) {
+		dev_err(cs35l56->dev, "Failed to runtime_get: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_read(cs35l56->regmap, CS35L56_PROTECTION_STATUS, &val);
+	if (ret)
+		dev_err(cs35l56->dev, "Failed to read PROTECTION_STATUS: %d\n", ret);
+	else
+		ret = !!(val & CS35L56_FIRMWARE_MISSING);
+
+	pm_runtime_put_autosuspend(cs35l56->dev);
+
+	return ret;
+}
+
 int cs35l56_system_suspend(struct device *dev)
 {
 	struct cs35l56_private *cs35l56 = dev_get_drvdata(dev);
@@ -1273,7 +1317,28 @@ int cs35l56_system_resume(struct device *dev)
 	if (cs35l56->irq)
 		enable_irq(cs35l56->irq);
 
-	return ret;
+	if (ret)
+		return ret;
+
+	/* Firmware won't have been loaded if the component hasn't probed */
+	if (!cs35l56->component)
+		return 0;
+
+	ret = cs35l56_is_fw_reload_needed(cs35l56);
+	dev_dbg(cs35l56->dev, "fw_reload_needed: %d\n", ret);
+	if (ret < 1)
+		return ret;
+
+	cs35l56->fw_patched = false;
+	init_completion(&cs35l56->dsp_ready_completion);
+	queue_work(cs35l56->dsp_wq, &cs35l56->dsp_work);
+
+	/*
+	 * suspend_bias_off ensures we are now in BIAS_OFF so there will be
+	 * a BIAS_OFF->BIAS_STANDBY transition to complete dsp patching.
+	 */
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(cs35l56_system_resume);
 
-- 
2.30.2

Re: [PATCH 0/6] ASoC: cs35l56: Add system suspend handling

[Mark Brown]

On Tue, 11 Apr 2023 16:25:22 +0100, Richard Fitzgerald wrote:
> This set of patches adds handling for system suspend.
> Patches 1..4 make some code changes that simplify the
> suspend implementation, mainly to avoid race conditions.
> 
> There are two seperate aspects to suspend, and these have
> been done as two patches:
> - the main suspend-resume handling,
> - re-loading the firmware if necessary after resume.
> 
> [...]

Applied to

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

Thanks!

[1/6] ASoC: cs35l56: Remove quick-cancelling of dsp_work()
      commit: 39a594dc0b4ac949edf221db33c7061c45e2c90b
[2/6] ASoC: cs35l56: Use DAPM widget for firmware PLAY/PAUSE
      commit: 7b98a1efbabfd729441f46823b24432f2c32deeb
[3/6] ASoC: cs35l56: Skip first init_completion wait in dsp_work if init_done
      commit: 7816e3407110d887726687740aa18c9ce8eeb0d2
[4/6] ASoC: cs35l56: Always wait for firmware boot in runtime-resume
      commit: f00abaddf0300bd9ca87918148a26bdb748129db
[5/6] ASoC: cs35l56: Add basic system suspend handling
      commit: f9dc6b875ec0a6a6d4091cd9603d193ec98c75a2
[6/6] ASoC: cs35l56: Re-patch firmware after system suspend
      commit: 59322d35179987e85b593e504fd334de8683c835

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