[PATCH 0/6] ASoC: SOF: Add support ctx_save with IPC4

[PATCH 0/6] ASoC: SOF: Add support ctx_save with IPC4

[Peter Ujfalusi]

Hi,

The context save functionality with IPC4 is triggered by sending a message to
the firmware about the pending power down of the primary core by the host.

In order to have this functionality implemented in a clean way we need to
introduce a new IPC level PM ops for core state management and use that instead
of open coding IPC messages here and there.

The first patch updates the ctx store/ctx_restore documentation to clarify that
they are optional.

Regards,
Peter
---
Peter Ujfalusi (6):
  ASoC: SOF: make ctx_store and ctx_restore as optional
  ASoC: SOF: sof_ipc_pm_ops: Add support for DSP core power management
  ASoC: SOF: ipc3: Add set_core_state pm_ops implementation
  ASoC: SOF: ipc4: Add set_core_state pm_ops implementation
  ASoC: SOF: Intel: Switch to use the generic pm_ops.set_core_state
  ASoC: SOF: ipc4: implement pm ctx_save callback

 include/sound/sof/ipc4/header.h |  8 +++++++
 sound/soc/sof/intel/hda-dsp.c   | 15 +++++-------
 sound/soc/sof/intel/tgl.c       | 30 ++++++++----------------
 sound/soc/sof/ipc3.c            | 18 +++++++++++++++
 sound/soc/sof/ipc4.c            | 41 +++++++++++++++++++++++++++++++++
 sound/soc/sof/sof-priv.h        |  6 +++--
 6 files changed, 87 insertions(+), 31 deletions(-)

-- 
2.36.1

[PATCH 1/6] ASoC: SOF: make ctx_store and ctx_restore as optional

[Peter Ujfalusi]

Commit 657774acd00f ("ASoC: SOF: Make sof_suspend/resume IPC agnostic")
did not marked ctx_store and ctx_restore as Optional.

Fixes: 657774acd00f ("ASoC: SOF: Make sof_suspend/resume IPC agnostic")
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/sof-priv.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 9d7f53ff9c70..58bcb8d6f72b 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -376,8 +376,8 @@ struct sof_ipc_fw_tracing_ops {
 
 /**
  * struct sof_ipc_pm_ops - IPC-specific PM ops
- * @ctx_save:		Function pointer for context save
- * @ctx_restore:	Function pointer for context restore
+ * @ctx_save:		Optional function pointer for context save
+ * @ctx_restore:	Optional function pointer for context restore
  */
 struct sof_ipc_pm_ops {
 	int (*ctx_save)(struct snd_sof_dev *sdev);
-- 
2.36.1

[PATCH 2/6] ASoC: SOF: sof_ipc_pm_ops: Add support for DSP core power management

[Peter Ujfalusi]

Add a new ops for handling DSP core power state which can be used to tell
the DSP to turn on/off a core (or to inform it that a core is going to be
turned on/off if the core is host managed).

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/sof-priv.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 58bcb8d6f72b..0544eb6a2322 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -378,10 +378,12 @@ struct sof_ipc_fw_tracing_ops {
  * struct sof_ipc_pm_ops - IPC-specific PM ops
  * @ctx_save:		Optional function pointer for context save
  * @ctx_restore:	Optional function pointer for context restore
+ * @set_core_state:	Optional function pointer for turning on/off a DSP core
  */
 struct sof_ipc_pm_ops {
 	int (*ctx_save)(struct snd_sof_dev *sdev);
 	int (*ctx_restore)(struct snd_sof_dev *sdev);
+	int (*set_core_state)(struct snd_sof_dev *sdev, int core_idx, bool on);
 };
 
 /**
-- 
2.36.1

[PATCH 3/6] ASoC: SOF: ipc3: Add set_core_state pm_ops implementation

[Peter Ujfalusi]

IPC3 uses sof_ipc_pm_core_config message
(SOF_IPC_GLB_PM_MSG | SOF_IPC_PM_CORE_ENABLE) to enable/disable cores
managed by the DSP.
The core state is set via a single bitfield, if the bit is 1 the core
should be on, if it is 0 then it is off.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/ipc3.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/sound/soc/sof/ipc3.c b/sound/soc/sof/ipc3.c
index dff5feaad370..ba81a6c490e9 100644
--- a/sound/soc/sof/ipc3.c
+++ b/sound/soc/sof/ipc3.c
@@ -1037,6 +1037,23 @@ static void sof_ipc3_rx_msg(struct snd_sof_dev *sdev)
 	ipc3_log_header(sdev->dev, "ipc rx done", hdr.cmd);
 }
 
+static int sof_ipc3_set_core_state(struct snd_sof_dev *sdev, int core_idx, bool on)
+{
+	struct sof_ipc_pm_core_config core_cfg = {
+		.hdr.size = sizeof(core_cfg),
+		.hdr.cmd = SOF_IPC_GLB_PM_MSG | SOF_IPC_PM_CORE_ENABLE,
+	};
+	struct sof_ipc_reply reply;
+
+	if (on)
+		core_cfg.enable_mask = sdev->enabled_cores_mask | BIT(core_idx);
+	else
+		core_cfg.enable_mask = sdev->enabled_cores_mask & ~BIT(core_idx);
+
+	return sof_ipc3_tx_msg(sdev, &core_cfg, sizeof(core_cfg),
+			       &reply, sizeof(reply), false);
+}
+
 static int sof_ipc3_ctx_ipc(struct snd_sof_dev *sdev, int cmd)
 {
 	struct sof_ipc_pm_ctx pm_ctx = {
@@ -1063,6 +1080,7 @@ static int sof_ipc3_ctx_restore(struct snd_sof_dev *sdev)
 static const struct sof_ipc_pm_ops ipc3_pm_ops = {
 	.ctx_save = sof_ipc3_ctx_save,
 	.ctx_restore = sof_ipc3_ctx_restore,
+	.set_core_state = sof_ipc3_set_core_state,
 };
 
 const struct sof_ipc_ops ipc3_ops = {
-- 
2.36.1

[PATCH 4/6] ASoC: SOF: ipc4: Add set_core_state pm_ops implementation

[Peter Ujfalusi]

IPC4 uses the SET_DX message to enable/disable cores managed by the DSP.
The dx_state.core_mask indicates which core is going to change state,
the dx_state.dx_mask is to power on (1) or off (0) the core.
In the dx_mask only those bits (cores) checked which bit is set in the
core_mask, other bits (cores) ignored.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/sof/ipc4/header.h |  8 ++++++++
 sound/soc/sof/ipc4.c            | 26 ++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/sound/sof/ipc4/header.h b/include/sound/sof/ipc4/header.h
index b8b8e5b5e3e1..a795deacc2ea 100644
--- a/include/sound/sof/ipc4/header.h
+++ b/include/sound/sof/ipc4/header.h
@@ -385,6 +385,14 @@ struct sof_ipc4_fw_version {
 	uint16_t build;
 } __packed;
 
+/* Payload data for SOF_IPC4_MOD_SET_DX */
+struct sof_ipc4_dx_state_info {
+	/* core(s) to apply the change */
+	uint32_t core_mask;
+	/* core state: 0: put core_id to D3; 1: put core_id to D0 */
+	uint32_t dx_mask;
+} __packed __aligned(4);
+
 /* Reply messages */
 
 /*
diff --git a/sound/soc/sof/ipc4.c b/sound/soc/sof/ipc4.c
index 658802c86685..b2cb92745ec6 100644
--- a/sound/soc/sof/ipc4.c
+++ b/sound/soc/sof/ipc4.c
@@ -597,10 +597,36 @@ static void sof_ipc4_rx_msg(struct snd_sof_dev *sdev)
 	}
 }
 
+static int sof_ipc4_set_core_state(struct snd_sof_dev *sdev, int core_idx, bool on)
+{
+	struct sof_ipc4_dx_state_info dx_state;
+	struct sof_ipc4_msg msg;
+
+	dx_state.core_mask = BIT(core_idx);
+	if (on)
+		dx_state.dx_mask = BIT(core_idx);
+	else
+		dx_state.dx_mask = 0;
+
+	msg.primary = SOF_IPC4_MSG_TYPE_SET(SOF_IPC4_MOD_SET_DX);
+	msg.primary |= SOF_IPC4_MSG_DIR(SOF_IPC4_MSG_REQUEST);
+	msg.primary |= SOF_IPC4_MSG_TARGET(SOF_IPC4_MODULE_MSG);
+	msg.extension = 0;
+	msg.data_ptr = &dx_state;
+	msg.data_size = sizeof(dx_state);
+
+	return sof_ipc4_tx_msg(sdev, &msg, msg.data_size, NULL, 0, false);
+}
+
+static const struct sof_ipc_pm_ops ipc4_pm_ops = {
+	.set_core_state = sof_ipc4_set_core_state,
+};
+
 const struct sof_ipc_ops ipc4_ops = {
 	.tx_msg = sof_ipc4_tx_msg,
 	.rx_msg = sof_ipc4_rx_msg,
 	.set_get_data = sof_ipc4_set_get_data,
 	.get_reply = sof_ipc4_get_reply,
+	.pm = &ipc4_pm_ops,
 	.fw_loader = &ipc4_loader_ops,
 };
-- 
2.36.1

[PATCH 5/6] ASoC: SOF: Intel: Switch to use the generic pm_ops.set_core_state

[Peter Ujfalusi]

Instead of craft and send an IPC(3) message in hda_dsp_core_get(),
tgl_dsp_core_get() and tgl_dsp_core_put(), use the generic ops for handling
the IPC dependent implementation of core power on/off.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/intel/hda-dsp.c | 15 ++++++---------
 sound/soc/sof/intel/tgl.c     | 30 ++++++++++--------------------
 2 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
index e24eea725acb..263ad455e283 100644
--- a/sound/soc/sof/intel/hda-dsp.c
+++ b/sound/soc/sof/intel/hda-dsp.c
@@ -940,13 +940,7 @@ void hda_dsp_d0i3_work(struct work_struct *work)
 
 int hda_dsp_core_get(struct snd_sof_dev *sdev, int core)
 {
-	struct sof_ipc_pm_core_config pm_core_config = {
-		.hdr = {
-			.cmd = SOF_IPC_GLB_PM_MSG | SOF_IPC_PM_CORE_ENABLE,
-			.size = sizeof(pm_core_config),
-		},
-		.enable_mask = sdev->enabled_cores_mask | BIT(core),
-	};
+	const struct sof_ipc_pm_ops *pm_ops = sdev->ipc->ops->pm;
 	int ret, ret1;
 
 	/* power up core */
@@ -961,9 +955,12 @@ int hda_dsp_core_get(struct snd_sof_dev *sdev, int core)
 	if (sdev->fw_state != SOF_FW_BOOT_COMPLETE || core == SOF_DSP_PRIMARY_CORE)
 		return 0;
 
+	/* No need to continue the set_core_state ops is not available */
+	if (!pm_ops->set_core_state)
+		return 0;
+
 	/* Now notify DSP for secondary cores */
-	ret = sof_ipc_tx_message(sdev->ipc, &pm_core_config, sizeof(pm_core_config),
-				 &pm_core_config, sizeof(pm_core_config));
+	ret = pm_ops->set_core_state(sdev, core, true);
 	if (ret < 0) {
 		dev_err(sdev->dev, "failed to enable secondary core '%d' failed with %d\n",
 			core, ret);
diff --git a/sound/soc/sof/intel/tgl.c b/sound/soc/sof/intel/tgl.c
index 1ddc492f1b13..dcad7c382de6 100644
--- a/sound/soc/sof/intel/tgl.c
+++ b/sound/soc/sof/intel/tgl.c
@@ -24,40 +24,30 @@ static const struct snd_sof_debugfs_map tgl_dsp_debugfs[] = {
 
 static int tgl_dsp_core_get(struct snd_sof_dev *sdev, int core)
 {
-	struct sof_ipc_pm_core_config pm_core_config = {
-		.hdr = {
-			.cmd = SOF_IPC_GLB_PM_MSG | SOF_IPC_PM_CORE_ENABLE,
-			.size = sizeof(pm_core_config),
-		},
-		.enable_mask = sdev->enabled_cores_mask | BIT(core),
-	};
+	const struct sof_ipc_pm_ops *pm_ops = sdev->ipc->ops->pm;
 
 	/* power up primary core if not already powered up and return */
 	if (core == SOF_DSP_PRIMARY_CORE)
 		return hda_dsp_enable_core(sdev, BIT(core));
 
-	/* notify DSP for secondary cores */
-	return sof_ipc_tx_message(sdev->ipc, &pm_core_config, sizeof(pm_core_config),
-				 &pm_core_config, sizeof(pm_core_config));
+	if (pm_ops->set_core_state)
+		return pm_ops->set_core_state(sdev, core, true);
+
+	return 0;
 }
 
 static int tgl_dsp_core_put(struct snd_sof_dev *sdev, int core)
 {
-	struct sof_ipc_pm_core_config pm_core_config = {
-		.hdr = {
-			.cmd = SOF_IPC_GLB_PM_MSG | SOF_IPC_PM_CORE_ENABLE,
-			.size = sizeof(pm_core_config),
-		},
-		.enable_mask = sdev->enabled_cores_mask & ~BIT(core),
-	};
+	const struct sof_ipc_pm_ops *pm_ops = sdev->ipc->ops->pm;
 
 	/* power down primary core and return */
 	if (core == SOF_DSP_PRIMARY_CORE)
 		return hda_dsp_core_reset_power_down(sdev, BIT(core));
 
-	/* notify DSP for secondary cores */
-	return sof_ipc_tx_message(sdev->ipc, &pm_core_config, sizeof(pm_core_config),
-				 &pm_core_config, sizeof(pm_core_config));
+	if (pm_ops->set_core_state)
+		return pm_ops->set_core_state(sdev, core, false);
+
+	return 0;
 }
 
 /* Tigerlake ops */
-- 
2.36.1

[PATCH 6/6] ASoC: SOF: ipc4: implement pm ctx_save callback

[Peter Ujfalusi]

Use the context save callback to power down the primary core which is used
by the firmware as an indication that the DSP is going to be turned off.

The IMR boot setup is done in response to the primary core power down.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/ipc4.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/sound/soc/sof/ipc4.c b/sound/soc/sof/ipc4.c
index b2cb92745ec6..5dd22f6a0605 100644
--- a/sound/soc/sof/ipc4.c
+++ b/sound/soc/sof/ipc4.c
@@ -618,7 +618,22 @@ static int sof_ipc4_set_core_state(struct snd_sof_dev *sdev, int core_idx, bool
 	return sof_ipc4_tx_msg(sdev, &msg, msg.data_size, NULL, 0, false);
 }
 
+/*
+ * The context save callback is used to send a message to the firmware notifying
+ * it that the primary core is going to be turned off, which is used as an
+ * indication to prepare for a full power down, thus preparing for IMR boot
+ * (when supported)
+ *
+ * Note: in IPC4 there is no message used to restore context, thus no context
+ * restore callback is implemented
+ */
+static int sof_ipc4_ctx_save(struct snd_sof_dev *sdev)
+{
+	return sof_ipc4_set_core_state(sdev, SOF_DSP_PRIMARY_CORE, false);
+}
+
 static const struct sof_ipc_pm_ops ipc4_pm_ops = {
+	.ctx_save = sof_ipc4_ctx_save,
 	.set_core_state = sof_ipc4_set_core_state,
 };
 
-- 
2.36.1

Re: [PATCH 0/6] ASoC: SOF: Add support ctx_save with IPC4

[Mark Brown]

On Fri, 10 Jun 2022 11:35:43 +0300, Peter Ujfalusi wrote:
> The context save functionality with IPC4 is triggered by sending a message to
> the firmware about the pending power down of the primary core by the host.
> 
> In order to have this functionality implemented in a clean way we need to
> introduce a new IPC level PM ops for core state management and use that instead
> of open coding IPC messages here and there.
> 
> [...]

Applied to

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

Thanks!

[1/6] ASoC: SOF: make ctx_store and ctx_restore as optional
      commit: 03f69725749f453b9a4d454a92805f8eb5f095c2
[2/6] ASoC: SOF: sof_ipc_pm_ops: Add support for DSP core power management
      commit: b41252d8820c7009078c3d401a807a9da899075f
[3/6] ASoC: SOF: ipc3: Add set_core_state pm_ops implementation
      commit: 0a047dafefafbccc931fab2d187ce75c302088d5
[4/6] ASoC: SOF: ipc4: Add set_core_state pm_ops implementation
      commit: bd3df9ff25b32b66630c283bb2e065e8bb822e72
[5/6] ASoC: SOF: Intel: Switch to use the generic pm_ops.set_core_state
      commit: 7a5677407300e8ba6af95e66f4e8cfe23059f4a7
[6/6] ASoC: SOF: ipc4: implement pm ctx_save callback
      commit: 63b9069653a710b08d5fd174ac05d43711356541

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