[PATCH 0/2] soundwire: bus: re-enable CLOCK_STOP_MODE1 support

[PATCH 0/2] soundwire: bus: re-enable CLOCK_STOP_MODE1 support

[Bard Liao]

Add CLOCK_STOP_MODE1 back to allow the peripheral to utilize it when
the clock is stopped.
This mode is designed for power-sensitive devices as it enables a deeper
power-saving state, although it does not retain the peripheral's state
during the clock stop.

Bard Liao (2):
  soundwire: bus: add CLOCK_STOP_MODE1 support back
  soundwire: Intel: stop sdw clock in system suspend

 drivers/soundwire/bus.c             | 45 ++++++++++++++++-------------
 drivers/soundwire/intel_auxdevice.c |  3 +-
 include/linux/soundwire/sdw.h       |  4 +++
 3 files changed, 31 insertions(+), 21 deletions(-)

-- 
2.43.0

[PATCH 1/2] soundwire: bus: add CLOCK_STOP_MODE1 support back

[Bard Liao]

CLOCK_STOP_MODE1 is used when the Peripheral might have entered a deeper
power-saving mode that does not retain state while the Clock is stopped.
It is useful when the device is more power consumption sensitive. Add it
back to allow the Peripheral use CLOCK_STOP_MODE1.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 drivers/soundwire/bus.c       | 45 +++++++++++++++++++----------------
 include/linux/soundwire/sdw.h |  4 ++++
 2 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 0490777fa406..21d8d6d7147a 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -956,8 +956,22 @@ static void sdw_modify_slave_status(struct sdw_slave *slave,
 	mutex_unlock(&bus->bus_lock);
 }
 
+static enum sdw_clk_stop_mode sdw_get_clk_stop_mode(struct sdw_slave *slave)
+{
+	struct device *dev = &slave->dev;
+	struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
+
+	/*
+	 * Query for clock stop mode if Slave implements
+	 * ops->get_clk_stop_mode, else read from property.
+	 */
+	if (drv->ops && drv->ops->get_clk_stop_mode)
+		return drv->ops->get_clk_stop_mode(slave);
+
+	return slave->prop.clk_stop_mode1 ? SDW_CLK_STOP_MODE1 : SDW_CLK_STOP_MODE0;
+}
+
 static int sdw_slave_clk_stop_callback(struct sdw_slave *slave,
-				       enum sdw_clk_stop_mode mode,
 				       enum sdw_clk_stop_type type)
 {
 	int ret = 0;
@@ -969,7 +983,7 @@ static int sdw_slave_clk_stop_callback(struct sdw_slave *slave,
 		struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
 
 		if (drv->ops && drv->ops->clk_stop)
-			ret = drv->ops->clk_stop(slave, mode, type);
+			ret = drv->ops->clk_stop(slave, slave->clk_stop_mode, type);
 	}
 
 	mutex_unlock(&slave->sdw_dev_lock);
@@ -978,7 +992,6 @@ static int sdw_slave_clk_stop_callback(struct sdw_slave *slave,
 }
 
 static int sdw_slave_clk_stop_prepare(struct sdw_slave *slave,
-				      enum sdw_clk_stop_mode mode,
 				      bool prepare)
 {
 	bool wake_en;
@@ -990,7 +1003,7 @@ static int sdw_slave_clk_stop_prepare(struct sdw_slave *slave,
 	if (prepare) {
 		val = SDW_SCP_SYSTEMCTRL_CLK_STP_PREP;
 
-		if (mode == SDW_CLK_STOP_MODE1)
+		if (slave->clk_stop_mode == SDW_CLK_STOP_MODE1)
 			val |= SDW_SCP_SYSTEMCTRL_CLK_STP_MODE1;
 
 		if (wake_en)
@@ -1078,9 +1091,9 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus)
 		/* Identify if Slave(s) are available on Bus */
 		is_slave = true;
 
-		ret = sdw_slave_clk_stop_callback(slave,
-						  SDW_CLK_STOP_MODE0,
-						  SDW_CLK_PRE_PREPARE);
+		slave->clk_stop_mode = sdw_get_clk_stop_mode(slave);
+
+		ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_PRE_PREPARE);
 		if (ret < 0 && ret != -ENODATA) {
 			dev_err(&slave->dev, "clock stop pre-prepare cb failed:%d\n", ret);
 			return ret;
@@ -1090,9 +1103,7 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus)
 		if (!slave->prop.simple_clk_stop_capable) {
 			simple_clk_stop = false;
 
-			ret = sdw_slave_clk_stop_prepare(slave,
-							 SDW_CLK_STOP_MODE0,
-							 true);
+			ret = sdw_slave_clk_stop_prepare(slave, true);
 			if (ret < 0 && ret != -ENODATA) {
 				dev_err(&slave->dev, "clock stop prepare failed:%d\n", ret);
 				return ret;
@@ -1130,9 +1141,7 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus)
 		    slave->status != SDW_SLAVE_ALERT)
 			continue;
 
-		ret = sdw_slave_clk_stop_callback(slave,
-						  SDW_CLK_STOP_MODE0,
-						  SDW_CLK_POST_PREPARE);
+		ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_POST_PREPARE);
 
 		if (ret < 0 && ret != -ENODATA) {
 			dev_err(&slave->dev, "clock stop post-prepare cb failed:%d\n", ret);
@@ -1204,8 +1213,7 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus)
 		/* Identify if Slave(s) are available on Bus */
 		is_slave = true;
 
-		ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_STOP_MODE0,
-						  SDW_CLK_PRE_DEPREPARE);
+		ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_PRE_DEPREPARE);
 		if (ret < 0)
 			dev_warn(&slave->dev, "clock stop pre-deprepare cb failed:%d\n", ret);
 
@@ -1213,9 +1221,7 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus)
 		if (!slave->prop.simple_clk_stop_capable) {
 			simple_clk_stop = false;
 
-			ret = sdw_slave_clk_stop_prepare(slave, SDW_CLK_STOP_MODE0,
-							 false);
-
+			ret = sdw_slave_clk_stop_prepare(slave, false);
 			if (ret < 0)
 				dev_warn(&slave->dev, "clock stop deprepare failed:%d\n", ret);
 		}
@@ -1243,8 +1249,7 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus)
 		    slave->status != SDW_SLAVE_ALERT)
 			continue;
 
-		ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_STOP_MODE0,
-						  SDW_CLK_POST_DEPREPARE);
+		ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_POST_DEPREPARE);
 		if (ret < 0)
 			dev_warn(&slave->dev, "clock stop post-deprepare cb failed:%d\n", ret);
 	}
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index b484784e2690..c1d2fc69bed5 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -612,6 +612,7 @@ struct sdw_bus_params {
  * @update_status: Update Slave status
  * @bus_config: Update the bus config for Slave
  * @port_prep: Prepare the port with parameters
+ * @get_clk_stop_mode: Get the clock stop mode of the Slave
  * @clk_stop: handle imp-def sequences before and after prepare and de-prepare
  */
 struct sdw_slave_ops {
@@ -625,6 +626,7 @@ struct sdw_slave_ops {
 	int (*port_prep)(struct sdw_slave *slave,
 			 struct sdw_prepare_ch *prepare_ch,
 			 enum sdw_port_prep_ops pre_ops);
+	enum sdw_clk_stop_mode (*get_clk_stop_mode)(struct sdw_slave *slave);
 	int (*clk_stop)(struct sdw_slave *slave,
 			enum sdw_clk_stop_mode mode,
 			enum sdw_clk_stop_type type);
@@ -643,6 +645,7 @@ struct sdw_slave_ops {
  * @node: node for bus list
  * @port_ready: Port ready completion flag for each Slave port
  * @m_port_map: static Master port map for each Slave port
+ * @clk_stop_mode: The clock stop mode of the Slave
  * @dev_num: Current Device Number, values can be 0 or dev_num_sticky
  * @dev_num_sticky: one-time static Device Number assigned by Bus
  * @probed: boolean tracking driver state
@@ -677,6 +680,7 @@ struct sdw_slave {
 	struct list_head node;
 	struct completion port_ready[SDW_MAX_PORTS];
 	unsigned int m_port_map[SDW_MAX_PORTS];
+	enum sdw_clk_stop_mode clk_stop_mode;
 	u16 dev_num;
 	u16 dev_num_sticky;
 	bool probed;
-- 
2.43.0

[PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Bard Liao]

There is no need to keep the SoundWire clock active in system suspend.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 drivers/soundwire/intel_auxdevice.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
index 0b8107bec9ab..10fd27f4fe39 100644
--- a/drivers/soundwire/intel_auxdevice.c
+++ b/drivers/soundwire/intel_auxdevice.c
@@ -669,7 +669,8 @@ static int __maybe_unused intel_suspend(struct device *dev)
 		return 0;
 	}
 
-	ret = sdw_intel_stop_bus(sdw, false);
+	/* No need to keep the SoundWire clock active in system suspend */
+	ret = sdw_intel_stop_bus(sdw, true);
 	if (ret < 0) {
 		dev_err(dev, "%s: cannot stop bus: %d\n", __func__, ret);
 		return ret;
-- 
2.43.0

Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Pierre-Louis Bossart]

On 6/29/26 16:44, Bard Liao wrote:
> There is no need to keep the SoundWire clock active in system suspend.
> 
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
> ---
>  drivers/soundwire/intel_auxdevice.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
> index 0b8107bec9ab..10fd27f4fe39 100644
> --- a/drivers/soundwire/intel_auxdevice.c
> +++ b/drivers/soundwire/intel_auxdevice.c
> @@ -669,7 +669,8 @@ static int __maybe_unused intel_suspend(struct device *dev)
>  		return 0;
>  	}
>  
> -	ret = sdw_intel_stop_bus(sdw, false);
> +	/* No need to keep the SoundWire clock active in system suspend */
> +	ret = sdw_intel_stop_bus(sdw, true);

erm, are you sure about this change?

What this does is stop the SoundWire clock before entering system suspend. 
Is this needed? The whole point of system suspend is that the host will stop operating completely.
There is in theory no need to stop the clock because the clock restart capability will not be used.
Think for example of jack detection, it's supported in pm_runtime suspend with the SoundWire interrupt capability, but not in system suspend.
By changing the clock-stop argument to true, you are requesting the clock stop mechanism to be enabled even though you don't need it.

I do recall that in some generations it was actually not supported to turn off power to the IP while the wake detector was enabled.

>  	if (ret < 0) {
>  		dev_err(dev, "%s: cannot stop bus: %d\n", __func__, ret);
>  		return ret;

Re: [PATCH 1/2] soundwire: bus: add CLOCK_STOP_MODE1 support back

[Pierre-Louis Bossart]

On 6/29/26 16:44, Bard Liao wrote:
> CLOCK_STOP_MODE1 is used when the Peripheral might have entered a deeper
> power-saving mode that does not retain state while the Clock is stopped.
> It is useful when the device is more power consumption sensitive. Add it
> back to allow the Peripheral use CLOCK_STOP_MODE1.
> 
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>

This patch looks fine with the understanding that the DisCo property is defined on a system-by-system basis and isn't a codec property extracted from a datasheet.

If it was a codec property, this could be problematic in some cases. For example in a latency-constrained environment, the host could require peripherals to use the clock-stop mode0 to restart faster, even if this means writing-off power saving that the peripheral supports.

In other words, if the OEMs use a copy-pasted DisCo definition this could be problematic, and at some point we may have to discard the codec property. For now it's fine, so here's my

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>

> ---
>  drivers/soundwire/bus.c       | 45 +++++++++++++++++++----------------
>  include/linux/soundwire/sdw.h |  4 ++++
>  2 files changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 0490777fa406..21d8d6d7147a 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -956,8 +956,22 @@ static void sdw_modify_slave_status(struct sdw_slave *slave,
>  	mutex_unlock(&bus->bus_lock);
>  }
>  
> +static enum sdw_clk_stop_mode sdw_get_clk_stop_mode(struct sdw_slave *slave)
> +{
> +	struct device *dev = &slave->dev;
> +	struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
> +
> +	/*
> +	 * Query for clock stop mode if Slave implements
> +	 * ops->get_clk_stop_mode, else read from property.
> +	 */
> +	if (drv->ops && drv->ops->get_clk_stop_mode)
> +		return drv->ops->get_clk_stop_mode(slave);
> +
> +	return slave->prop.clk_stop_mode1 ? SDW_CLK_STOP_MODE1 : SDW_CLK_STOP_MODE0;
> +}
> +
>  static int sdw_slave_clk_stop_callback(struct sdw_slave *slave,
> -				       enum sdw_clk_stop_mode mode,
>  				       enum sdw_clk_stop_type type)
>  {
>  	int ret = 0;
> @@ -969,7 +983,7 @@ static int sdw_slave_clk_stop_callback(struct sdw_slave *slave,
>  		struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
>  
>  		if (drv->ops && drv->ops->clk_stop)
> -			ret = drv->ops->clk_stop(slave, mode, type);
> +			ret = drv->ops->clk_stop(slave, slave->clk_stop_mode, type);
>  	}
>  
>  	mutex_unlock(&slave->sdw_dev_lock);
> @@ -978,7 +992,6 @@ static int sdw_slave_clk_stop_callback(struct sdw_slave *slave,
>  }
>  
>  static int sdw_slave_clk_stop_prepare(struct sdw_slave *slave,
> -				      enum sdw_clk_stop_mode mode,
>  				      bool prepare)
>  {
>  	bool wake_en;
> @@ -990,7 +1003,7 @@ static int sdw_slave_clk_stop_prepare(struct sdw_slave *slave,
>  	if (prepare) {
>  		val = SDW_SCP_SYSTEMCTRL_CLK_STP_PREP;
>  
> -		if (mode == SDW_CLK_STOP_MODE1)
> +		if (slave->clk_stop_mode == SDW_CLK_STOP_MODE1)
>  			val |= SDW_SCP_SYSTEMCTRL_CLK_STP_MODE1;
>  
>  		if (wake_en)
> @@ -1078,9 +1091,9 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus)
>  		/* Identify if Slave(s) are available on Bus */
>  		is_slave = true;
>  
> -		ret = sdw_slave_clk_stop_callback(slave,
> -						  SDW_CLK_STOP_MODE0,
> -						  SDW_CLK_PRE_PREPARE);
> +		slave->clk_stop_mode = sdw_get_clk_stop_mode(slave);
> +
> +		ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_PRE_PREPARE);
>  		if (ret < 0 && ret != -ENODATA) {
>  			dev_err(&slave->dev, "clock stop pre-prepare cb failed:%d\n", ret);
>  			return ret;
> @@ -1090,9 +1103,7 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus)
>  		if (!slave->prop.simple_clk_stop_capable) {
>  			simple_clk_stop = false;
>  
> -			ret = sdw_slave_clk_stop_prepare(slave,
> -							 SDW_CLK_STOP_MODE0,
> -							 true);
> +			ret = sdw_slave_clk_stop_prepare(slave, true);
>  			if (ret < 0 && ret != -ENODATA) {
>  				dev_err(&slave->dev, "clock stop prepare failed:%d\n", ret);
>  				return ret;
> @@ -1130,9 +1141,7 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus)
>  		    slave->status != SDW_SLAVE_ALERT)
>  			continue;
>  
> -		ret = sdw_slave_clk_stop_callback(slave,
> -						  SDW_CLK_STOP_MODE0,
> -						  SDW_CLK_POST_PREPARE);
> +		ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_POST_PREPARE);
>  
>  		if (ret < 0 && ret != -ENODATA) {
>  			dev_err(&slave->dev, "clock stop post-prepare cb failed:%d\n", ret);
> @@ -1204,8 +1213,7 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus)
>  		/* Identify if Slave(s) are available on Bus */
>  		is_slave = true;
>  
> -		ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_STOP_MODE0,
> -						  SDW_CLK_PRE_DEPREPARE);
> +		ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_PRE_DEPREPARE);
>  		if (ret < 0)
>  			dev_warn(&slave->dev, "clock stop pre-deprepare cb failed:%d\n", ret);
>  
> @@ -1213,9 +1221,7 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus)
>  		if (!slave->prop.simple_clk_stop_capable) {
>  			simple_clk_stop = false;
>  
> -			ret = sdw_slave_clk_stop_prepare(slave, SDW_CLK_STOP_MODE0,
> -							 false);
> -
> +			ret = sdw_slave_clk_stop_prepare(slave, false);
>  			if (ret < 0)
>  				dev_warn(&slave->dev, "clock stop deprepare failed:%d\n", ret);
>  		}
> @@ -1243,8 +1249,7 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus)
>  		    slave->status != SDW_SLAVE_ALERT)
>  			continue;
>  
> -		ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_STOP_MODE0,
> -						  SDW_CLK_POST_DEPREPARE);
> +		ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_POST_DEPREPARE);
>  		if (ret < 0)
>  			dev_warn(&slave->dev, "clock stop post-deprepare cb failed:%d\n", ret);
>  	}
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index b484784e2690..c1d2fc69bed5 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -612,6 +612,7 @@ struct sdw_bus_params {
>   * @update_status: Update Slave status
>   * @bus_config: Update the bus config for Slave
>   * @port_prep: Prepare the port with parameters
> + * @get_clk_stop_mode: Get the clock stop mode of the Slave
>   * @clk_stop: handle imp-def sequences before and after prepare and de-prepare
>   */
>  struct sdw_slave_ops {
> @@ -625,6 +626,7 @@ struct sdw_slave_ops {
>  	int (*port_prep)(struct sdw_slave *slave,
>  			 struct sdw_prepare_ch *prepare_ch,
>  			 enum sdw_port_prep_ops pre_ops);
> +	enum sdw_clk_stop_mode (*get_clk_stop_mode)(struct sdw_slave *slave);
>  	int (*clk_stop)(struct sdw_slave *slave,
>  			enum sdw_clk_stop_mode mode,
>  			enum sdw_clk_stop_type type);
> @@ -643,6 +645,7 @@ struct sdw_slave_ops {
>   * @node: node for bus list
>   * @port_ready: Port ready completion flag for each Slave port
>   * @m_port_map: static Master port map for each Slave port
> + * @clk_stop_mode: The clock stop mode of the Slave
>   * @dev_num: Current Device Number, values can be 0 or dev_num_sticky
>   * @dev_num_sticky: one-time static Device Number assigned by Bus
>   * @probed: boolean tracking driver state
> @@ -677,6 +680,7 @@ struct sdw_slave {
>  	struct list_head node;
>  	struct completion port_ready[SDW_MAX_PORTS];
>  	unsigned int m_port_map[SDW_MAX_PORTS];
> +	enum sdw_clk_stop_mode clk_stop_mode;
>  	u16 dev_num;
>  	u16 dev_num_sticky;
>  	bool probed;

RE: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Liao, Bard]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>
> Sent: Tuesday, June 30, 2026 5:33 PM
> To: Bard Liao <yung-chuan.liao@linux.intel.com>; linux-
> sound@vger.kernel.org; vkoul@kernel.org
> Cc: vinod.koul@linaro.org; linux-kernel@vger.kernel.org;
> peter.ujfalusi@linux.intel.com; Liao, Bard <bard.liao@intel.com>
> Subject: Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend
> 
> On 6/29/26 16:44, Bard Liao wrote:
> > There is no need to keep the SoundWire clock active in system suspend.
> >
> > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
> > ---
> >  drivers/soundwire/intel_auxdevice.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soundwire/intel_auxdevice.c
> b/drivers/soundwire/intel_auxdevice.c
> > index 0b8107bec9ab..10fd27f4fe39 100644
> > --- a/drivers/soundwire/intel_auxdevice.c
> > +++ b/drivers/soundwire/intel_auxdevice.c
> > @@ -669,7 +669,8 @@ static int __maybe_unused intel_suspend(struct
> device *dev)
> >  		return 0;
> >  	}
> >
> > -	ret = sdw_intel_stop_bus(sdw, false);
> > +	/* No need to keep the SoundWire clock active in system suspend */
> > +	ret = sdw_intel_stop_bus(sdw, true);
> 
> erm, are you sure about this change?
> 
> What this does is stop the SoundWire clock before entering system suspend.
> Is this needed? The whole point of system suspend is that the host will stop
> operating completely.

Yes, that's what we need. The purpose is to inform the Peripheral to go
into the deserved clock stop mode before system suspend.

> There is in theory no need to stop the clock because the clock restart capability
> will not be used.
> Think for example of jack detection, it's supported in pm_runtime suspend
> with the SoundWire interrupt capability, but not in system suspend.

An example that we need clock stop1 in system suspend is to keep the amp's
firmware in system suspend. In other words, we don't want to reload the
firmware after system resume

> By changing the clock-stop argument to true, you are requesting the clock
> stop mechanism to be enabled even though you don't need it.
> 
> I do recall that in some generations it was actually not supported to turn off
> power to the IP while the wake detector was enabled.
> 
> >  	if (ret < 0) {
> >  		dev_err(dev, "%s: cannot stop bus: %d\n", __func__, ret);
> >  		return ret;

Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Richard Fitzgerald]

On 30/06/2026 11:35 am, Liao, Bard wrote:
> 
> 
>> -----Original Message-----
>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>
>> Sent: Tuesday, June 30, 2026 5:33 PM
>> To: Bard Liao <yung-chuan.liao@linux.intel.com>; linux-
>> sound@vger.kernel.org; vkoul@kernel.org
>> Cc: vinod.koul@linaro.org; linux-kernel@vger.kernel.org;
>> peter.ujfalusi@linux.intel.com; Liao, Bard <bard.liao@intel.com>
>> Subject: Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend
>>
>> On 6/29/26 16:44, Bard Liao wrote:
>>> There is no need to keep the SoundWire clock active in system suspend.
>>>
>>> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
>>> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
>>> ---
>>>   drivers/soundwire/intel_auxdevice.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soundwire/intel_auxdevice.c
>> b/drivers/soundwire/intel_auxdevice.c
>>> index 0b8107bec9ab..10fd27f4fe39 100644
>>> --- a/drivers/soundwire/intel_auxdevice.c
>>> +++ b/drivers/soundwire/intel_auxdevice.c
>>> @@ -669,7 +669,8 @@ static int __maybe_unused intel_suspend(struct
>> device *dev)
>>>   		return 0;
>>>   	}
>>>
>>> -	ret = sdw_intel_stop_bus(sdw, false);
>>> +	/* No need to keep the SoundWire clock active in system suspend */
>>> +	ret = sdw_intel_stop_bus(sdw, true);
>>
>> erm, are you sure about this change?
>>
>> What this does is stop the SoundWire clock before entering system suspend.
>> Is this needed? The whole point of system suspend is that the host will stop
>> operating completely.

Not necessarily. There are multiple types of system suspend, like
suspend-to-RAM. The system doesn't always shut down.
Also depends on the OS: Android seems to use system suspend as a deep
sleep state.

> 
> Yes, that's what we need. The purpose is to inform the Peripheral to go
> into the deserved clock stop mode before system suspend.
> 
>> There is in theory no need to stop the clock because the clock restart capability
>> will not be used.

As Bard says, the peripherals should be informed that the clock will be
stopped. They at least should have the opportunity to act on that. And
they could still be powered across one of the lighter forms of system
suspend.

>> Think for example of jack detection, it's supported in pm_runtime suspend
>> with the SoundWire interrupt capability, but not in system suspend.
> 
> An example that we need clock stop1 in system suspend is to keep the amp's
> firmware in system suspend. In other words, we don't want to reload the
> firmware after system resume
> 
>> By changing the clock-stop argument to true, you are requesting the clock
>> stop mechanism to be enabled even though you don't need it.
>>
>> I do recall that in some generations it was actually not supported to turn off
>> power to the IP while the wake detector was enabled.
>>
>>>   	if (ret < 0) {
>>>   		dev_err(dev, "%s: cannot stop bus: %d\n", __func__, ret);
>>>   		return ret;
> 
> 

Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Pierre-Louis Bossart]

>>>> --- a/drivers/soundwire/intel_auxdevice.c
>>>> +++ b/drivers/soundwire/intel_auxdevice.c
>>>> @@ -669,7 +669,8 @@ static int __maybe_unused intel_suspend(struct
>>> device *dev)
>>>>           return 0;
>>>>       }
>>>>
>>>> -    ret = sdw_intel_stop_bus(sdw, false);
>>>> +    /* No need to keep the SoundWire clock active in system suspend */
>>>> +    ret = sdw_intel_stop_bus(sdw, true);
>>>
>>> erm, are you sure about this change?
>>>
>>> What this does is stop the SoundWire clock before entering system suspend.
>>> Is this needed? The whole point of system suspend is that the host will stop
>>> operating completely.
> 
> Not necessarily. There are multiple types of system suspend, like
> suspend-to-RAM. The system doesn't always shut down.

Let's be clear: all types of system suspend do not use the clock stop mode, at least on Intel platforms.

The clock stop mode is ONLY used in the pm_runtime suspend.

The main reason for this is that the host cannot keep a detector alive to restart the bus at the request of a peripheral.
As a result, the jack detection only works in pm_runtime suspend and NOT in system suspend. It's been the default since we introduced support of clock stop...

Note that in case where the peripheral is not capable of generating a wake, the clock stop is not used either. No need to keep that detector alive on the host if it's never going to see a wake.

> Also depends on the OS: Android seems to use system suspend as a deep
> sleep state.
> 
>>
>> Yes, that's what we need. The purpose is to inform the Peripheral to go
>> into the deserved clock stop mode before system suspend.

Sorry but this isn't what this patch does. 

I think you are confusing power savings in clock stop mode with the ability for the codec to enter a lower power in system suspend.

If you do need the ability to put the peripheral in a lower power mode, it's perfectly acceptable, but that can be done in the system suspend routine of the peripheral driver. You do not need or want to piggy-back on the clock stop mechanism which isn't used in system suspend.

>>> There is in theory no need to stop the clock because the clock restart capability
>>> will not be used.
> 
> As Bard says, the peripherals should be informed that the clock will be
> stopped. They at least should have the opportunity to act on that. And
> they could still be powered across one of the lighter forms of system
> suspend.

Again it'd be fine to reach a lower power for the peripheral or keep some power rails active, but that's orthogonal to the clock stop.

>>> Think for example of jack detection, it's supported in pm_runtime suspend
>>> with the SoundWire interrupt capability, but not in system suspend.
>>
>> An example that we need clock stop1 in system suspend is to keep the amp's
>> firmware in system suspend. In other words, we don't want to reload the
>> firmware after system resume

If you want to use lower power modes in system suspend, that's fine. The suspend/resume routines can be modified to introduce lower power modes on the peripheral side.
I am not disagreeing with the concept of lowering power consumption in system suspend, just on the mechanisms that you trying to use.

But keeping the context on the peripheral is one bridge too far I am afraid. Intel platforms perform a bus reset in all cases, including in clock-stop modes, that was the hardware guidance. That means that no matter what you do prior to pm_runtime and system suspend the peripheral WILL lose context.

To rehash the different points:
- The only significant difference between pm_runtime and system suspend is the wake detection capability which is only supported in the former case. 
- On Intel platforms, context is lost on the peripheral in all resume routines because of the bus reset, the latency on resume is pretty much the same in both cases
- The peripheral driver can introduce a lower-power mode in system suspend by changing the system suspend routine.

Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Richard Fitzgerald]

On 30/06/2026 1:36 pm, Pierre-Louis Bossart wrote:
> 
>>>>> --- a/drivers/soundwire/intel_auxdevice.c
>>>>> +++ b/drivers/soundwire/intel_auxdevice.c
>>>>> @@ -669,7 +669,8 @@ static int __maybe_unused intel_suspend(struct
>>>> device *dev)
>>>>>            return 0;
>>>>>        }
>>>>>
>>>>> -    ret = sdw_intel_stop_bus(sdw, false);
>>>>> +    /* No need to keep the SoundWire clock active in system suspend */
>>>>> +    ret = sdw_intel_stop_bus(sdw, true);
>>>>
>>>> erm, are you sure about this change?
>>>>
>>>> What this does is stop the SoundWire clock before entering system suspend.
>>>> Is this needed? The whole point of system suspend is that the host will stop
>>>> operating completely.
>>
>> Not necessarily. There are multiple types of system suspend, like
>> suspend-to-RAM. The system doesn't always shut down.
> 
> Let's be clear: all types of system suspend do not use the clock stop mode, at least on Intel platforms.
> 
> The clock stop mode is ONLY used in the pm_runtime suspend.
> 
> The main reason for this is that the host cannot keep a detector alive to restart the bus at the request of a peripheral.
> As a result, the jack detection only works in pm_runtime suspend and NOT in system suspend. It's been the default since we introduced support of clock stop...
> 
> Note that in case where the peripheral is not capable of generating a wake, the clock stop is not used either. No need to keep that detector alive on the host if it's never going to see a wake.
> 
>> Also depends on the OS: Android seems to use system suspend as a deep
>> sleep state.
>>
>>>
>>> Yes, that's what we need. The purpose is to inform the Peripheral to go
>>> into the deserved clock stop mode before system suspend.
> 
> Sorry but this isn't what this patch does.
> 
> I think you are confusing power savings in clock stop mode with the ability for the codec to enter a lower power in system suspend.
> 
> If you do need the ability to put the peripheral in a lower power mode, it's perfectly acceptable, but that can be done in the system suspend routine of the peripheral driver. You do not need or want to piggy-back on the clock stop mechanism which isn't used in system suspend.
> 
>>>> There is in theory no need to stop the clock because the clock restart capability
>>>> will not be used.
>>
>> As Bard says, the peripherals should be informed that the clock will be
>> stopped. They at least should have the opportunity to act on that. And
>> they could still be powered across one of the lighter forms of system
>> suspend.
> 
> Again it'd be fine to reach a lower power for the peripheral or keep some power rails active, but that's orthogonal to the clock stop.
> 
>>>> Think for example of jack detection, it's supported in pm_runtime suspend
>>>> with the SoundWire interrupt capability, but not in system suspend.
>>>
>>> An example that we need clock stop1 in system suspend is to keep the amp's
>>> firmware in system suspend. In other words, we don't want to reload the
>>> firmware after system resume
> 
> If you want to use lower power modes in system suspend, that's fine. The suspend/resume routines can be modified to introduce lower power modes on the peripheral side.
> I am not disagreeing with the concept of lowering power consumption in system suspend, just on the mechanisms that you trying to use.
> 
> But keeping the context on the peripheral is one bridge too far I am afraid. Intel platforms perform a bus reset in all cases, including in clock-stop modes, that was the hardware guidance. That means that no matter what you do prior to pm_runtime and system suspend the peripheral WILL lose context.
> 
> To rehash the different points:
> - The only significant difference between pm_runtime and system suspend is the wake detection capability which is only supported in the former case.
> - On Intel platforms, context is lost on the peripheral in all resume routines because of the bus reset, the latency on resume is pretty much the same in both cases
> - The peripheral driver can introduce a lower-power mode in system suspend by changing the system suspend routine.
> 

If the Manager doesn't send a clock-stop, the peripherals don't get a
notification that they can enter a lower-power mode. The clock suddenly
disappears without warning and without the peripherals being notified
why, so they don't have any information to know what is happening.

The Manager should send a clock-stop notification before stopping the
clock.

Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Pierre-Louis Bossart]

> If the Manager doesn't send a clock-stop, the peripherals don't get a
> notification that they can enter a lower-power mode. The clock suddenly
> disappears without warning and without the peripherals being notified
> why, so they don't have any information to know what is happening.

that's not quite right, see below.

> The Manager should send a clock-stop notification before stopping the
> clock.

That's exactly the existing logic in drivers/soundwire/bus.c, the core does notify all peripheral drivers of a clock stop transition.

sdw_slave_clk_stop_callback(slave, SDW_CLK_PRE_PREPARE);
sdw_slave_clk_stop_callback(slave, SDW_CLK_POST_PREPARE);

These pre- and post callback that can be used by the peripheral for imp-def actions.

I am not sure if the POST_PREPARE makes sense, since the clock is already stopped the peripheral cannot be programmed. IIRC it was added for symmetry.
The PRE_PREPARE is most likely what you are looking for in the implementation of the drv->ops->clk_stop callback.

Hope this helps!

Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Pierre-Louis Bossart]

On 7/1/26 13:55, Pierre-Louis Bossart wrote:
> 
>> If the Manager doesn't send a clock-stop, the peripherals don't get a
>> notification that they can enter a lower-power mode. The clock suddenly
>> disappears without warning and without the peripherals being notified
>> why, so they don't have any information to know what is happening.
> 
> that's not quite right, see below.
> 
>> The Manager should send a clock-stop notification before stopping the
>> clock.
> 
> That's exactly the existing logic in drivers/soundwire/bus.c, the core does notify all peripheral drivers of a clock stop transition.
> 
> sdw_slave_clk_stop_callback(slave, SDW_CLK_PRE_PREPARE);
> sdw_slave_clk_stop_callback(slave, SDW_CLK_POST_PREPARE);
> 
> These pre- and post callback that can be used by the peripheral for imp-def actions.
> 
> I am not sure if the POST_PREPARE makes sense, since the clock is already stopped the peripheral cannot be programmed. IIRC it was added for symmetry.
> The PRE_PREPARE is most likely what you are looking for in the implementation of the drv->ops->clk_stop callback.
> 
> Hope this helps!

Oh and in the case of a system suspend, then you also get the notification in the driver suspend callback. In that case no need to be notified of the implicit bus teardown, you can program the peripheral to enter lower power. You do not need any clock stop mechanisms to deal with system suspend optimizations.

Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Richard Fitzgerald]

On 01/07/2026 12:55 pm, Pierre-Louis Bossart wrote:
> 
>> If the Manager doesn't send a clock-stop, the peripherals don't get a
>> notification that they can enter a lower-power mode. The clock suddenly
>> disappears without warning and without the peripherals being notified
>> why, so they don't have any information to know what is happening.
> 
> that's not quite right, see below.
> 
>> The Manager should send a clock-stop notification before stopping the
>> clock.
> 
> That's exactly the existing logic in drivers/soundwire/bus.c, the core does notify all peripheral drivers of a clock stop transition.
>

It's been observed with logic analyzer captures on real hardware
that the clock stop is only being sent in runtime suspends, not
system suspends.

Because of this, if the bus was in a runtime-resumed state when
system suspend started the clock would stop without warning.

Bard - that's the observed behavior this patch is fixing, right?

> sdw_slave_clk_stop_callback(slave, SDW_CLK_PRE_PREPARE);
> sdw_slave_clk_stop_callback(slave, SDW_CLK_POST_PREPARE);
> 
> These pre- and post callback that can be used by the peripheral for imp-def actions.
> 
> I am not sure if the POST_PREPARE makes sense, since the clock is already stopped the peripheral cannot be programmed. IIRC it was added for symmetry.
> The PRE_PREPARE is most likely what you are looking for in the implementation of the drv->ops->clk_stop callback.
> 
> Hope this helps!
> 
> 
> 
> 
> 
> 
> 
> 
> 

RE: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Liao, Bard]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>
> Sent: Wednesday, July 1, 2026 8:01 PM
> To: Richard Fitzgerald <rf@opensource.cirrus.com>; Liao, Bard
> <bard.liao@intel.com>; Bard Liao <yung-chuan.liao@linux.intel.com>; linux-
> sound@vger.kernel.org; vkoul@kernel.org
> Cc: vinod.koul@linaro.org; linux-kernel@vger.kernel.org;
> peter.ujfalusi@linux.intel.com; Richard Fitzgerald
> <Richard.Fitzgerald@cirrus.com>
> Subject: Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend
> 
> On 7/1/26 13:55, Pierre-Louis Bossart wrote:
> >
> >> If the Manager doesn't send a clock-stop, the peripherals don't get a
> >> notification that they can enter a lower-power mode. The clock suddenly
> >> disappears without warning and without the peripherals being notified
> >> why, so they don't have any information to know what is happening.
> >
> > that's not quite right, see below.
> >
> >> The Manager should send a clock-stop notification before stopping the
> >> clock.
> >
> > That's exactly the existing logic in drivers/soundwire/bus.c, the core does
> notify all peripheral drivers of a clock stop transition.
> >
> > sdw_slave_clk_stop_callback(slave, SDW_CLK_PRE_PREPARE);
> > sdw_slave_clk_stop_callback(slave, SDW_CLK_POST_PREPARE);
> >
> > These pre- and post callback that can be used by the peripheral for imp-def
> actions.
> >
> > I am not sure if the POST_PREPARE makes sense, since the clock is already
> stopped the peripheral cannot be programmed. IIRC it was added for
> symmetry.
> > The PRE_PREPARE is most likely what you are looking for in the
> implementation of the drv->ops->clk_stop callback.
> >
> > Hope this helps!
> 
> Oh and in the case of a system suspend, then you also get the notification in
> the driver suspend callback. In that case no need to be notified of the implicit
> bus teardown, you can program the peripheral to enter lower power. You do
> not need any clock stop mechanisms to deal with system suspend
> optimizations.

The point is that the SDW bus clock will stop anyway in system suspend
even though sdw_cdns_clock_stop() is not called.
And the peripheral will not get the clock-stop notification in this case.
Not sure will codec driver's system suspend callback launch before the
sdw bus clock getting stopped. And how to ensure it?

Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Richard Fitzgerald]

On 01/07/2026 1:00 pm, Pierre-Louis Bossart wrote:
> On 7/1/26 13:55, Pierre-Louis Bossart wrote:
>>
>>> If the Manager doesn't send a clock-stop, the peripherals don't get a
>>> notification that they can enter a lower-power mode. The clock suddenly
>>> disappears without warning and without the peripherals being notified
>>> why, so they don't have any information to know what is happening.
>>
>> that's not quite right, see below.
>>
>>> The Manager should send a clock-stop notification before stopping the
>>> clock.
>>
>> That's exactly the existing logic in drivers/soundwire/bus.c, the core does notify all peripheral drivers of a clock stop transition.
>>
>> sdw_slave_clk_stop_callback(slave, SDW_CLK_PRE_PREPARE);
>> sdw_slave_clk_stop_callback(slave, SDW_CLK_POST_PREPARE);
>>
>> These pre- and post callback that can be used by the peripheral for imp-def actions.
>>
>> I am not sure if the POST_PREPARE makes sense, since the clock is already stopped the peripheral cannot be programmed. IIRC it was added for symmetry.
>> The PRE_PREPARE is most likely what you are looking for in the implementation of the drv->ops->clk_stop callback.
>>
>> Hope this helps!
> 
> Oh and in the case of a system suspend, then you also get the notification in the driver suspend callback. In that case no need to be notified of the implicit bus teardown, you can program the peripheral to enter lower power. You do not need any clock stop mechanisms to deal with system suspend optimizations.
> 

the codec driver should not have to be working around the Manager driver
failing to write the clock-stop notification.

Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Pierre-Louis Bossart]

On 7/1/26 14:15, Richard Fitzgerald wrote:
> On 01/07/2026 12:55 pm, Pierre-Louis Bossart wrote:
>>
>>> If the Manager doesn't send a clock-stop, the peripherals don't get a
>>> notification that they can enter a lower-power mode. The clock suddenly
>>> disappears without warning and without the peripherals being notified
>>> why, so they don't have any information to know what is happening.
>>
>> that's not quite right, see below.
>>
>>> The Manager should send a clock-stop notification before stopping the
>>> clock.
>>
>> That's exactly the existing logic in drivers/soundwire/bus.c, the core does notify all peripheral drivers of a clock stop transition.
>>
> 
> It's been observed with logic analyzer captures on real hardware
> that the clock stop is only being sent in runtime suspends, not
> system suspends.

That's the expected behavior.

> Because of this, if the bus was in a runtime-resumed state when
> system suspend started the clock would stop without warning.
> 
> Bard - that's the observed behavior this patch is fixing, right?

I think you are confusing 'clock stop' and 'clock pause', this is a well-known confusion in SoundWire. See Section 8.1.2.2 Clock Pause in the 1.2.1 spec.

The clock stop mechanism refers to the ability to remove all transitions on the clock line, but restart the transitions when the data line is driven high for some time.
In other words, the "clock stop" used in pm_runtime suspend will also arm a detector on the data line. That's the problematic part on the host, this detector cannot be kept alive in system suspend since it has standby power implications.

In addition, on Intel platforms, there's a well-known issue where you cannot remove power to the SoundWire IP while this wake detector is active. That's the reason why we introduced a pm_runtime resume before the system suspend, the transition from pm_runtime suspended to system suspended is not supported. see intel_pm_prepare().

the 'clock pause' is what you see with a logic analyzer, it's not the same as the full blown 'clock stop' mechanism because there is no handshake to prepare the wake, and no support for the detector. There is indeed no signaling or known timing on when this pause occurs in the system suspend phase.

But again such signaling is not required: if you want to enter a lower power mode prior to system suspend, you can do so in your peripheral driver. You do not need to know if/when the clock will be paused. Just add the relevant programming sequence before the device transitions to system suspend.

Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Richard Fitzgerald]

On 01/07/2026 3:09 pm, Pierre-Louis Bossart wrote:
> On 7/1/26 14:15, Richard Fitzgerald wrote:
>> On 01/07/2026 12:55 pm, Pierre-Louis Bossart wrote:
>>>
>>>> If the Manager doesn't send a clock-stop, the peripherals don't get a
>>>> notification that they can enter a lower-power mode. The clock suddenly
>>>> disappears without warning and without the peripherals being notified
>>>> why, so they don't have any information to know what is happening.
>>>
>>> that's not quite right, see below.
>>>
>>>> The Manager should send a clock-stop notification before stopping the
>>>> clock.
>>>
>>> That's exactly the existing logic in drivers/soundwire/bus.c, the core does notify all peripheral drivers of a clock stop transition.
>>>
>>
>> It's been observed with logic analyzer captures on real hardware
>> that the clock stop is only being sent in runtime suspends, not
>> system suspends.
> 
> That's the expected behavior.
> 
>> Because of this, if the bus was in a runtime-resumed state when
>> system suspend started the clock would stop without warning.
>>
>> Bard - that's the observed behavior this patch is fixing, right?
> 
> I think you are confusing 'clock stop' and 'clock pause', this is a well-known confusion in SoundWire. See Section 8.1.2.2 Clock Pause in the 1.2.1 spec.
> 
> The clock stop mechanism refers to the ability to remove all transitions on the clock line, but restart the transitions when the data line is driven high for some time.
> In other words, the "clock stop" used in pm_runtime suspend will also arm a detector on the data line. That's the problematic part on the host, this detector cannot be kept alive in system suspend since it has standby power implications.
> 
> In addition, on Intel platforms, there's a well-known issue where you cannot remove power to the SoundWire IP while this wake detector is active. That's the reason why we introduced a pm_runtime resume before the system suspend, the transition from pm_runtime suspended to system suspended is not supported. see intel_pm_prepare().
> 
> the 'clock pause' is what you see with a logic analyzer, it's not the same as the full blown 'clock stop' mechanism because there is no handshake to prepare the wake, and no support for the detector. There is indeed no signaling or known timing on when this pause occurs in the system suspend phase.
> 
> But again such signaling is not required: if you want to enter a lower power mode prior to system suspend, you can do so in your peripheral driver. You do not need to know if/when the clock will be paused. Just add the relevant programming sequence before the device transitions to system suspend.
> 

So you're saying that if we're running on an Intel platform the codec
driver will have to write the clock-stop-1 notification to the device?

Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Pierre-Louis Bossart]

On 7/1/26 16:59, Richard Fitzgerald wrote:
> On 01/07/2026 3:09 pm, Pierre-Louis Bossart wrote:
>> On 7/1/26 14:15, Richard Fitzgerald wrote:
>>> On 01/07/2026 12:55 pm, Pierre-Louis Bossart wrote:
>>>>
>>>>> If the Manager doesn't send a clock-stop, the peripherals don't get a
>>>>> notification that they can enter a lower-power mode. The clock suddenly
>>>>> disappears without warning and without the peripherals being notified
>>>>> why, so they don't have any information to know what is happening.
>>>>
>>>> that's not quite right, see below.
>>>>
>>>>> The Manager should send a clock-stop notification before stopping the
>>>>> clock.
>>>>
>>>> That's exactly the existing logic in drivers/soundwire/bus.c, the core does notify all peripheral drivers of a clock stop transition.
>>>>
>>>
>>> It's been observed with logic analyzer captures on real hardware
>>> that the clock stop is only being sent in runtime suspends, not
>>> system suspends.
>>
>> That's the expected behavior.
>>
>>> Because of this, if the bus was in a runtime-resumed state when
>>> system suspend started the clock would stop without warning.
>>>
>>> Bard - that's the observed behavior this patch is fixing, right?
>>
>> I think you are confusing 'clock stop' and 'clock pause', this is a well-known confusion in SoundWire. See Section 8.1.2.2 Clock Pause in the 1.2.1 spec.
>>
>> The clock stop mechanism refers to the ability to remove all transitions on the clock line, but restart the transitions when the data line is driven high for some time.
>> In other words, the "clock stop" used in pm_runtime suspend will also arm a detector on the data line. That's the problematic part on the host, this detector cannot be kept alive in system suspend since it has standby power implications.
>>
>> In addition, on Intel platforms, there's a well-known issue where you cannot remove power to the SoundWire IP while this wake detector is active. That's the reason why we introduced a pm_runtime resume before the system suspend, the transition from pm_runtime suspended to system suspended is not supported. see intel_pm_prepare().
>>
>> the 'clock pause' is what you see with a logic analyzer, it's not the same as the full blown 'clock stop' mechanism because there is no handshake to prepare the wake, and no support for the detector. There is indeed no signaling or known timing on when this pause occurs in the system suspend phase.
>>
>> But again such signaling is not required: if you want to enter a lower power mode prior to system suspend, you can do so in your peripheral driver. You do not need to know if/when the clock will be paused. Just add the relevant programming sequence before the device transitions to system suspend.
>>
> 
> So you're saying that if we're running on an Intel platform the codec
> driver will have to write the clock-stop-1 notification to the device?

Not sure I am following the question. My entire argument was that the clock stop mechanism shouldn't be used unless the wake capabilities are needed.

Maybe I missed something in the peripheral capabilities. Is the lower power mode entered by
- programming registers, e.g. move to PS3 or something equivalent?
- tracking the level of the clock and data lines?
- tracking the ClockStopNow bit in the PCP_Ctrl Register?
- a combination of multiple actions?

I don't think letting the peripheral driver send a command that sets this bit is a valid option. The Manager is supposed to own all the bits in the frame and bring the signals to a Low level, if the command is sent by a peripheral driver that sequence couldn't possibly be enforced on the Manager side.

If you insist that the clock stop mechanism be used in all suspend modes, both system and pm_runtime, then thinking out loud one would need to create a bespoke mode where the clock-stop signaling is used to unleash power savings on the peripheral side but the wake detector is not enabled on the host to avoid known/documented issues. 

This piece of code is where I think the problem is, see intel_stop_bus(struct sdw_intel *sdw, bool clock_stop)

if (clock_stop) {
	ret = sdw_cdns_clock_stop(cdns, true);
	if (ret < 0)
		dev_err(dev, "%s: cannot stop clock: %d\n", __func__, ret);
	else
		wake_enable = true; <<< HERE, that can't be enabled in system suspend.
}

I guess you'd need to change the clock_stop argument to have more than just boolean values, so that the wake_enable remains false in the system suspend case.

That would be a significant change requiring quite a bit of testing, probably only for new platforms and not a generic change that applies to all generations since 2018. Again just trying to throw some ideas to make progress.

Just enabling clock-stop in system suspend across all generations will fail, that's pretty much guaranteed based on known bug reports related to the wake detector.