soundwire: bus: add CLOCK_STOP_MODE1 support back by bardliao · Pull Request #5624 · thesofproject/linux

bardliao · 2025-12-12T12:50:54Z

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.

plbossart · 2025-12-23T10:47:02Z

@bardliao how would you test this? In practice the intel host does a bus reset after restarting the clock which forces the device to lose context anyways.
IOW it doesn't matter if the device supports clock stop0 or 1, context always needs to be restored.

bardliao · 2025-12-24T05:31:31Z

@plbossart Indeed, we will do bus reset after restarting the clock. Using mode 1 is just for power consumption during clock stop. On the other hands, the peripheral will need to restore the context anyway, why don't we use mode 1 If the peripheral supports it?

plbossart · 2025-12-27T16:06:55Z

I don't disagree @bardliao, if the host does a bus reset we would benefit from the MODE1 on the device side.
There are still several opens
a) how was MODE0 tested, if we do a bus reset then by construction we cannot test the instant restart
b) if a device supports MODE1, do we always select MODE1? it's a trade-off between power and latency, not sure the property definition is that clear.
c) if the host does a bus reset, should it also override all the properties to set the MODE1?

bardliao · 2025-12-29T06:09:21Z

I don't disagree @bardliao, if the host does a bus reset we would benefit from the MODE1 on the device side. There are still several opens
a) how was MODE0 tested, if we do a bus reset then by construction we cannot test the instant restart

The existing code is using MODE0. I would expect that the behavior will not change on MODE0.

b) if a device supports MODE1, do we always select MODE1? it's a trade-off between power and latency, not sure the property definition is that clear.

No, the codec driver can implement the get_clk_stop_mode ops to set the condition of using MODE1

c) if the host does a bus reset, should it also override all the properties to set the MODE1?

sdw_get_clk_stop_mode() will set the clock mode before clock stops. I guess it should be enough?

bardliao · 2025-12-29T06:13:40Z

@vijendarmukunda Do you mind testing this PR on AMD platforms?

vijendarmukunda · 2025-12-29T09:20:04Z

@vijendarmukunda Do you mind testing this PR on AMD platforms?

@bardliao : Most of my team members are OOO. Is it okay if we can update the valiadation results some time early next week?

bardliao · 2025-12-29T09:35:50Z

@vijendarmukunda Do you mind testing this PR on AMD platforms?

@bardliao : Most of my team members are OOO. Is it okay if we can update the valiadation results some time early next week?

Yeah, same as us. It is totally fine to get the test result next week.

vijendarmukunda · 2026-01-06T11:41:34Z

@bardliao : Validated ClockStop Mode1 on AMD platform using RTK codec. It's working fine.

rfvirgil · 2026-02-09T16:34:34Z

Problem with this is that it only issues clock-stop in runtime_suspend.
It doesn't issue it in system_suspend.
The clock-stop1 tells the amp to go into lowest-power mode. But the clock-stop1 doesn't happen in system_suspend so the amps are still running at higher-power while system is suspended.

rfvirgil · 2026-02-09T17:08:54Z

In intel_suspend() maybe change it like this:

-	ret = sdw_intel_stop_bus(sdw, false);
+	ret = sdw_intel_stop_bus(sdw, true);

bardliao · 2026-02-10T09:19:46Z

In intel_suspend() maybe change it like this:
-	ret = sdw_intel_stop_bus(sdw, false);
+	ret = sdw_intel_stop_bus(sdw, true);

Yes, you are right. @rfvirgil We didn't stop SDW clock in system suspend. I wonder if it is Intentional or not? @plbossart any idea?

rfvirgil · 2026-02-10T17:16:35Z

Tested with MTL and PTL
On devices that support clock-stop 1 and devices that don't.

ujfalusi · 2026-03-24T06:25:49Z

drivers/soundwire/bus.c

+			mode = SDW_CLK_STOP_MODE0;
+	}
+
+	return mode;


Could be as simple as:

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;

ujfalusi · 2026-03-24T06:36:24Z

drivers/soundwire/bus.c

-		ret = sdw_slave_clk_stop_callback(slave,
-						  SDW_CLK_STOP_MODE0,
+		ret = sdw_slave_clk_stop_callback(slave, mode,
 						  SDW_CLK_POST_PREPARE);


Since you have introduced a member to store the mode slave->curr_clk_stop_mode, why not modify the sdw_slave_clk_stop_callback() and sdw_slave_clk_stop_prepare() to use this new member and drop the mode parameter?

ujfalusi · 2026-03-24T06:37:45Z

include/linux/soundwire/sdw.h

 	struct list_head node;
 	struct completion port_ready[SDW_MAX_PORTS];
 	unsigned int m_port_map[SDW_MAX_PORTS];
+	enum sdw_clk_stop_mode curr_clk_stop_mode;


Update the struct doc as well.
But, is there a prev_clk_stop_mode or next_clk_stop_mode? Should this be just a simple clk_stop_mode ?

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>

There is no need to keep the SoundWire clock active in system suspend. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Copilot

Pull request overview

This PR reintroduces support for SoundWire CLOCK_STOP_MODE1, enabling peripherals to request a deeper clock-stop mode (useful when device state is not retained during clock stop and power consumption sensitivity is higher).

Changes:

Adds a Slave driver hook to query the desired clock-stop mode and stores the mode in the sdw_slave structure.
Updates clock-stop prepare/callback flows to use the stored per-Slave clock-stop mode.
Switches Intel SoundWire system suspend to stop the bus using the clock-stop flow.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
include/linux/soundwire/sdw.h	Extends `sdw_slave_ops` and `sdw_slave` to support querying/storing clock-stop mode.
drivers/soundwire/intel_auxdevice.c	Changes system suspend behavior to stop the bus with clock stop enabled.
drivers/soundwire/bus.c	Introduces mode query helper and propagates per-Slave clock-stop mode through prepare/callback logic.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-24T14:53:41Z

drivers/soundwire/bus.c

+	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);
+	else
+		return slave->prop.clk_stop_mode1 ? SDW_CLK_STOP_MODE1 : SDW_CLK_STOP_MODE0;


sdw_get_clk_stop_mode() unconditionally calls drv_to_sdw_driver(dev->driver). If the Slave is enumerated but has no bound driver (dev->driver == NULL / slave->probed == false), this will dereference a NULL pointer via container_of and crash. Consider first checking slave->probed (and/or dev->driver) before accessing drv->ops, and falling back to the property-based default when no driver is bound. Also consider validating the returned value to ensure it is a supported sdw_clk_stop_mode (e.g., MODE0/MODE1) and falling back safely on unexpected/negative values.

Suggested change

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

else

return slave->prop.clk_stop_mode1 ? SDW_CLK_STOP_MODE1 : SDW_CLK_STOP_MODE0;

struct sdw_driver *drv;

enum sdw_clk_stop_mode mode;

/*

* Default to the property-based value. This is used when no

* driver is bound or when the driver returns an unsupported

* clock stop mode.

*/

mode = slave->prop.clk_stop_mode1 ? SDW_CLK_STOP_MODE1 : SDW_CLK_STOP_MODE0;

/*

* If the Slave is not yet probed or has no bound driver, fall back

* to the property-based default without dereferencing dev->driver.

*/

if (!slave->probed || !dev->driver)

return mode;

drv = drv_to_sdw_driver(dev->driver);

/*

* Query for clock stop mode if the Slave driver implements

* ops->get_clk_stop_mode. Validate the returned value and

* fall back to the property-based default on unexpected values.

*/

if (drv->ops && drv->ops->get_clk_stop_mode) {

enum sdw_clk_stop_mode drv_mode;

drv_mode = drv->ops->get_clk_stop_mode(slave);

if (drv_mode == SDW_CLK_STOP_MODE0 ||

drv_mode == SDW_CLK_STOP_MODE1)

return drv_mode;

}

return mode;

Copilot · 2026-03-24T14:53:42Z

include/linux/soundwire/sdw.h

 	int (*port_prep)(struct sdw_slave *slave,
 			 struct sdw_prepare_ch *prepare_ch,
 			 enum sdw_port_prep_ops pre_ops);
+	int (*get_clk_stop_mode)(struct sdw_slave *slave);


The new get_clk_stop_mode callback is declared as returning int, but the core treats it as an sdw_clk_stop_mode and uses the returned value to program hardware state. Using int makes it ambiguous whether negative error codes are allowed; if a driver returns -errno it could be propagated into mode handling unexpectedly. Consider changing the callback return type to enum sdw_clk_stop_mode (or documenting/handling errors explicitly in the core, e.g., treat negative as fallback to properties/default).

Suggested change

int (*get_clk_stop_mode)(struct sdw_slave *slave);

enum sdw_clk_stop_mode (*get_clk_stop_mode)(struct sdw_slave *slave);

Copilot · 2026-03-24T14:53:42Z

drivers/soundwire/intel_auxdevice.c

 		return 0;
 	}

-	ret = sdw_intel_stop_bus(sdw, false);
+	ret = sdw_intel_stop_bus(sdw, true);
 	if (ret < 0) {
 		dev_err(dev, "%s: cannot stop bus: %d\n", __func__, ret);
 		return ret;


intel_suspend() now always calls sdw_intel_stop_bus(sdw, true) when the device is runtime-active. This bypasses the clock_stop_quirks handling used in intel_suspend_runtime() (e.g., SDW_INTEL_CLK_STOP_TEARDOWN), and may attempt a clock-stop flow on platforms that explicitly require teardown/no clock-stop. Consider selecting the stop mode based on clock_stop_quirks here as well (matching the runtime suspend logic), or clearly justifying why system suspend must always use clock_stop=true.

ujfalusi · 2026-03-24T15:13:41Z

drivers/soundwire/bus.c

+	 */
+	if (drv->ops && drv->ops->get_clk_stop_mode)
+		return drv->ops->get_clk_stop_mode(slave);
+	else


no need for the else

bardliao requested review from charleskeepax, rfvirgil, shumingfan, stefdb and ujfalusi December 12, 2025 12:50

bardliao force-pushed the sdw-clk-stop-mode1 branch from 6de6e7b to 5789c69 Compare February 10, 2026 09:16

rfvirgil previously approved these changes Feb 10, 2026

View reviewed changes

bardliao marked this pull request as ready for review March 9, 2026 10:11

ujfalusi reviewed Mar 24, 2026

View reviewed changes

bardliao added 2 commits March 24, 2026 22:45

soundwire: Intel: stop sdw clock in system suspend

3f923b6

There is no need to keep the SoundWire clock active in system suspend. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Copilot AI review requested due to automatic review settings March 24, 2026 14:46

bardliao dismissed rfvirgil’s stale review via 3f923b6 March 24, 2026 14:46

bardliao force-pushed the sdw-clk-stop-mode1 branch from 5789c69 to 3f923b6 Compare March 24, 2026 14:46

Copilot started reviewing on behalf of bardliao March 24, 2026 14:47 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

ujfalusi reviewed Mar 24, 2026

View reviewed changes

-	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);
-	else
-		return slave->prop.clk_stop_mode1 ? SDW_CLK_STOP_MODE1 : SDW_CLK_STOP_MODE0;
+	struct sdw_driver *drv;
+	enum sdw_clk_stop_mode mode;
+	/*
+	 * Default to the property-based value. This is used when no
+	 * driver is bound or when the driver returns an unsupported
+	 * clock stop mode.
+	 */
+	mode = slave->prop.clk_stop_mode1 ? SDW_CLK_STOP_MODE1 : SDW_CLK_STOP_MODE0;
+	/*
+	 * If the Slave is not yet probed or has no bound driver, fall back
+	 * to the property-based default without dereferencing dev->driver.
+	 */
+	if (!slave->probed || !dev->driver)
+		return mode;
+	drv = drv_to_sdw_driver(dev->driver);
+	/*
+	 * Query for clock stop mode if the Slave driver implements
+	 * ops->get_clk_stop_mode. Validate the returned value and
+	 * fall back to the property-based default on unexpected values.
+	 */
+	if (drv->ops && drv->ops->get_clk_stop_mode) {
+		enum sdw_clk_stop_mode drv_mode;
+		drv_mode = drv->ops->get_clk_stop_mode(slave);
+		if (drv_mode == SDW_CLK_STOP_MODE0 ||
+		    drv_mode == SDW_CLK_STOP_MODE1)
+			return drv_mode;
+	}
+	return mode;

	int (get_clk_stop_mode)(struct sdw_slave slave);
	enum sdw_clk_stop_mode (get_clk_stop_mode)(struct sdw_slave slave);

Conversation

bardliao commented Dec 12, 2025

Uh oh!

plbossart commented Dec 23, 2025

Uh oh!

bardliao commented Dec 24, 2025

Uh oh!

plbossart commented Dec 27, 2025

Uh oh!

bardliao commented Dec 29, 2025

Uh oh!

bardliao commented Dec 29, 2025

Uh oh!

vijendarmukunda commented Dec 29, 2025

Uh oh!

bardliao commented Dec 29, 2025

Uh oh!

vijendarmukunda commented Jan 6, 2026

Uh oh!

rfvirgil commented Feb 9, 2026

Uh oh!

rfvirgil commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bardliao commented Feb 10, 2026

Uh oh!

rfvirgil commented Feb 10, 2026

Uh oh!

ujfalusi Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

ujfalusi Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

ujfalusi Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

ujfalusi Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rfvirgil commented Feb 9, 2026 •

edited

Loading

ujfalusi Mar 24, 2026 •

edited

Loading