[PATCH 0/4] soundwire: intel: exit clock-stop mode before system suspend

[PATCH 0/4] soundwire: intel: exit clock-stop mode before system suspend

[Bard Liao]

Intel validation reported an issue where the HW_RST self-clearing bit
is not cleared in hardware, which as a ripple effect creates issues
with the clock stop mode.

This happens is a specific sequence where the Intel manager is
pm_runtime suspended with the clock-stop mode enabled. During the
system suspend, we currently do nothing, which can lead to potential
issues on system resume and the following pm_runtime suspend,
depending on the hardware state.

This patch suggests a full resume if the clock-stop mode is used. This
may require extra time but will make the suspend/resume flows
completely symmetric. This also removes a race condition where we
could not access SHIM registers if the parent was suspended as
well. Resuming the link also resumes the parent by construction.

BugLink: https://github.com/thesofproject/linux/issues/2606

Pierre-Louis Bossart (4):
  soundwire: intel: fix potential race condition during power down
  soundwire: intel: skip suspend/resume/wake when link was not started
  soundwire: intel: exit clock stop mode on system suspend
  soundwire: intel: simplify pm_runtime handling in suspend/resume

 drivers/soundwire/intel.c | 126 ++++++++++++++++++++++++++++----------
 drivers/soundwire/intel.h |   1 +
 2 files changed, 94 insertions(+), 33 deletions(-)

-- 
2.17.1

[PATCH 1/4] soundwire: intel: fix potential race condition during power down

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The power down sequence sets the link_up flag as false outside of the
mutex_lock. This is potentially unsafe.

In additional the flow in that sequence can be improved by first
testing if the link was powered, setting the link_up flag as false and
proceeding with the power down. In case the CPA bits cannot be
cleared, we only flag an error since we cannot deal with interrupts
any longer.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/intel.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index a0178779a5ba..3af922e20e64 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -464,12 +464,14 @@ static int intel_link_power_down(struct sdw_intel *sdw)
 
 	mutex_lock(sdw->link_res->shim_lock);
 
-	intel_shim_master_ip_to_glue(sdw);
-
 	if (!(*shim_mask & BIT(link_id)))
 		dev_err(sdw->cdns.dev,
 			"%s: Unbalanced power-up/down calls\n", __func__);
 
+	sdw->cdns.link_up = false;
+
+	intel_shim_master_ip_to_glue(sdw);
+
 	*shim_mask &= ~BIT(link_id);
 
 	if (!*shim_mask) {
@@ -486,18 +488,19 @@ static int intel_link_power_down(struct sdw_intel *sdw)
 		link_control &=  spa_mask;
 
 		ret = intel_clear_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
+		if (ret < 0) {
+			dev_err(sdw->cdns.dev, "%s: could not power down link\n", __func__);
+
+			/*
+			 * we leave the sdw->cdns.link_up flag as false since we've disabled
+			 * the link at this point and cannot handle interrupts any longer.
+			 */
+		}
 	}
 
 	mutex_unlock(sdw->link_res->shim_lock);
 
-	if (ret < 0) {
-		dev_err(sdw->cdns.dev, "%s: could not power down link\n", __func__);
-
-		return ret;
-	}
-
-	sdw->cdns.link_up = false;
-	return 0;
+	return ret;
 }
 
 static void intel_shim_sync_arm(struct sdw_intel *sdw)
-- 
2.17.1

[PATCH 2/4] soundwire: intel: skip suspend/resume/wake when link was not started

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

On some HDaudio platforms, SoundWire devices are described in the
DSDT but never used. This patch adds a boolean status flag to skip all
suspend/resume/wake sequences for this configuration.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/intel.c | 22 ++++++++++++----------
 drivers/soundwire/intel.h |  1 +
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 3af922e20e64..46d1645cb7fe 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1456,6 +1456,7 @@ int intel_link_startup(struct auxiliary_device *auxdev)
 	if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME_IDLE))
 		pm_runtime_idle(dev);
 
+	sdw->startup_done = true;
 	return 0;
 
 err_interrupt:
@@ -1495,8 +1496,9 @@ int intel_link_process_wakeen_event(struct auxiliary_device *auxdev)
 	sdw = dev_get_drvdata(dev);
 	bus = &sdw->cdns.bus;
 
-	if (bus->prop.hw_disabled) {
-		dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n", bus->link_id);
+	if (bus->prop.hw_disabled || !sdw->startup_done) {
+		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
+			bus->link_id);
 		return 0;
 	}
 
@@ -1533,8 +1535,8 @@ static int __maybe_unused intel_suspend(struct device *dev)
 	u32 clock_stop_quirks;
 	int ret;
 
-	if (bus->prop.hw_disabled) {
-		dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n",
+	if (bus->prop.hw_disabled || !sdw->startup_done) {
+		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
 			bus->link_id);
 		return 0;
 	}
@@ -1587,8 +1589,8 @@ static int __maybe_unused intel_suspend_runtime(struct device *dev)
 	u32 clock_stop_quirks;
 	int ret;
 
-	if (bus->prop.hw_disabled) {
-		dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n",
+	if (bus->prop.hw_disabled || !sdw->startup_done) {
+		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
 			bus->link_id);
 		return 0;
 	}
@@ -1652,8 +1654,8 @@ static int __maybe_unused intel_resume(struct device *dev)
 	bool multi_link;
 	int ret;
 
-	if (bus->prop.hw_disabled) {
-		dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n",
+	if (bus->prop.hw_disabled || !sdw->startup_done) {
+		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
 			bus->link_id);
 		return 0;
 	}
@@ -1750,8 +1752,8 @@ static int __maybe_unused intel_resume_runtime(struct device *dev)
 	int status;
 	int ret;
 
-	if (bus->prop.hw_disabled) {
-		dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n",
+	if (bus->prop.hw_disabled || !sdw->startup_done) {
+		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
 			bus->link_id);
 		return 0;
 	}
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index 0b47b148da3f..cd93a44dba9a 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -41,6 +41,7 @@ struct sdw_intel {
 	struct sdw_cdns cdns;
 	int instance;
 	struct sdw_intel_link_res *link_res;
+	bool startup_done;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs;
 #endif
-- 
2.17.1

[PATCH 3/4] soundwire: intel: exit clock stop mode on system suspend

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Intel validation reported an issue where the HW_RST self-clearing bit
is not cleared in hardware, which as a ripple effect creates issues
with the clock stop mode.

This happens is a specific sequence where the Intel manager is
pm_runtime suspended with the clock-stop mode enabled. During the
system suspend, we currently do nothing, which can lead to potential
issues on system resume and the following pm_runtime suspend,
depending on the hardware state.

This patch suggests a full resume (parent+child devices) if the
clock-stop mode is used. This may require extra time but will make the
suspend/resume flows completely symmetric. This also removes a race
condition where we could not access SHIM registers if the parent was
suspended as well. Resuming the link also resumes the parent by
construction.

BugLink: https://github.com/thesofproject/linux/issues/2606
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/intel.c | 65 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 46d1645cb7fe..9d05e158fe0e 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1527,6 +1527,70 @@ int intel_link_process_wakeen_event(struct auxiliary_device *auxdev)
  * PM calls
  */
 
+static int intel_resume_child_device(struct device *dev, void *data)
+{
+	int ret;
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+
+	if (!slave->probed) {
+		dev_dbg(dev, "%s: skipping device, no probed driver\n", __func__);
+		return 0;
+	}
+	if (!slave->dev_num_sticky) {
+		dev_dbg(dev, "%s: skipping device, never detected on bus\n", __func__);
+		return 0;
+	}
+
+	ret = pm_request_resume(dev);
+	if (ret < 0)
+		dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret);
+
+	return ret;
+}
+
+static int __maybe_unused intel_pm_prepare(struct device *dev)
+{
+	struct sdw_cdns *cdns = dev_get_drvdata(dev);
+	struct sdw_intel *sdw = cdns_to_intel(cdns);
+	struct sdw_bus *bus = &cdns->bus;
+	u32 clock_stop_quirks;
+	int ret = 0;
+
+	if (bus->prop.hw_disabled || !sdw->startup_done) {
+		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
+			bus->link_id);
+		return 0;
+	}
+
+	clock_stop_quirks = sdw->link_res->clock_stop_quirks;
+
+	if ((clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) ||
+	    !clock_stop_quirks) {
+		/*
+		 * Try to resume the entire bus (parent + child devices) to exit
+		 * the clock stop mode. If this fails, we keep going since we don't want
+		 * to prevent system suspend from happening and errors should be recoverable
+		 * on resume.
+		 */
+		ret = device_for_each_child(bus->dev, NULL, intel_resume_child_device);
+
+		if (ret < 0)
+			dev_err(dev, "%s: intel_resume_child_device failed: %d\n", __func__, ret);
+
+		/*
+		 * in the case where a link was started but does not have anything connected,
+		 * we still need to resume to keep link power up/down sequences balanced.
+		 * This is a no-op if a child device was present, since resuming the child
+		 * device would also resume the parent
+		 */
+		ret = pm_request_resume(dev);
+		if (ret < 0)
+			dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret);
+	}
+
+	return 0;
+}
+
 static int __maybe_unused intel_suspend(struct device *dev)
 {
 	struct sdw_cdns *cdns = dev_get_drvdata(dev);
@@ -1923,6 +1987,7 @@ static int __maybe_unused intel_resume_runtime(struct device *dev)
 }
 
 static const struct dev_pm_ops intel_pm = {
+	.prepare = intel_pm_prepare,
 	SET_SYSTEM_SLEEP_PM_OPS(intel_suspend, intel_resume)
 	SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL)
 };
-- 
2.17.1

[PATCH 4/4] soundwire: intel: simplify pm_runtime handling in suspend/resume

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Since we've introduced a .prepare callback that brings all devices to
full power on S state transitions if the clock-stop mode was used, we
no longer need to special-case when the device was pm_runtime
suspended.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/intel.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 9d05e158fe0e..ec8d6fcbfc9e 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1610,19 +1610,9 @@ static int __maybe_unused intel_suspend(struct device *dev)
 
 		clock_stop_quirks = sdw->link_res->clock_stop_quirks;
 
-		if ((clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET ||
-		     !clock_stop_quirks) &&
-		    !pm_runtime_suspended(dev->parent)) {
-
-			/*
-			 * if we've enabled clock stop, and the parent
-			 * is still active, disable shim wake. The
-			 * SHIM registers are not accessible if the
-			 * parent is already pm_runtime suspended so
-			 * it's too late to change that configuration
-			 */
-
-			intel_shim_wake(sdw, false);
+		if (clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET || !clock_stop_quirks) {
+			/* this should not happen but throw a log in case of a broken sequence */
+			dev_err(dev, "%s: pm_runtime status is suspended with clock-stop mode\n", __func__);
 		}
 
 		return 0;
-- 
2.17.1

Re: [PATCH 2/4] soundwire: intel: skip suspend/resume/wake when link was not started

[Vinod Koul]

On 27-07-21, 13:56, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> On some HDaudio platforms, SoundWire devices are described in the
> DSDT but never used. This patch adds a boolean status flag to skip all
> suspend/resume/wake sequences for this configuration.

Why are the sdw devices created in this case then? I would assume you
are detecting this configuration and should skip device creation?

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/intel.c | 22 ++++++++++++----------
>  drivers/soundwire/intel.h |  1 +
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 3af922e20e64..46d1645cb7fe 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1456,6 +1456,7 @@ int intel_link_startup(struct auxiliary_device *auxdev)
>  	if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME_IDLE))
>  		pm_runtime_idle(dev);
>  
> +	sdw->startup_done = true;
>  	return 0;
>  
>  err_interrupt:
> @@ -1495,8 +1496,9 @@ int intel_link_process_wakeen_event(struct auxiliary_device *auxdev)
>  	sdw = dev_get_drvdata(dev);
>  	bus = &sdw->cdns.bus;
>  
> -	if (bus->prop.hw_disabled) {
> -		dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n", bus->link_id);
> +	if (bus->prop.hw_disabled || !sdw->startup_done) {
> +		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
> +			bus->link_id);
>  		return 0;
>  	}
>  
> @@ -1533,8 +1535,8 @@ static int __maybe_unused intel_suspend(struct device *dev)
>  	u32 clock_stop_quirks;
>  	int ret;
>  
> -	if (bus->prop.hw_disabled) {
> -		dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n",
> +	if (bus->prop.hw_disabled || !sdw->startup_done) {
> +		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
>  			bus->link_id);
>  		return 0;
>  	}
> @@ -1587,8 +1589,8 @@ static int __maybe_unused intel_suspend_runtime(struct device *dev)
>  	u32 clock_stop_quirks;
>  	int ret;
>  
> -	if (bus->prop.hw_disabled) {
> -		dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n",
> +	if (bus->prop.hw_disabled || !sdw->startup_done) {
> +		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
>  			bus->link_id);
>  		return 0;
>  	}
> @@ -1652,8 +1654,8 @@ static int __maybe_unused intel_resume(struct device *dev)
>  	bool multi_link;
>  	int ret;
>  
> -	if (bus->prop.hw_disabled) {
> -		dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n",
> +	if (bus->prop.hw_disabled || !sdw->startup_done) {
> +		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
>  			bus->link_id);
>  		return 0;
>  	}
> @@ -1750,8 +1752,8 @@ static int __maybe_unused intel_resume_runtime(struct device *dev)
>  	int status;
>  	int ret;
>  
> -	if (bus->prop.hw_disabled) {
> -		dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n",
> +	if (bus->prop.hw_disabled || !sdw->startup_done) {
> +		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
>  			bus->link_id);
>  		return 0;
>  	}
> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
> index 0b47b148da3f..cd93a44dba9a 100644
> --- a/drivers/soundwire/intel.h
> +++ b/drivers/soundwire/intel.h
> @@ -41,6 +41,7 @@ struct sdw_intel {
>  	struct sdw_cdns cdns;
>  	int instance;
>  	struct sdw_intel_link_res *link_res;
> +	bool startup_done;
>  #ifdef CONFIG_DEBUG_FS
>  	struct dentry *debugfs;
>  #endif
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 3/4] soundwire: intel: exit clock stop mode on system suspend

[Vinod Koul]

On 27-07-21, 13:56, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> Intel validation reported an issue where the HW_RST self-clearing bit
> is not cleared in hardware, which as a ripple effect creates issues
> with the clock stop mode.
> 
> This happens is a specific sequence where the Intel manager is
> pm_runtime suspended with the clock-stop mode enabled. During the
> system suspend, we currently do nothing, which can lead to potential
> issues on system resume and the following pm_runtime suspend,
> depending on the hardware state.
> 
> This patch suggests a full resume (parent+child devices) if the
> clock-stop mode is used. This may require extra time but will make the
> suspend/resume flows completely symmetric. This also removes a race
> condition where we could not access SHIM registers if the parent was
> suspended as well. Resuming the link also resumes the parent by
> construction.
> 
> BugLink: https://github.com/thesofproject/linux/issues/2606
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/intel.c | 65 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 46d1645cb7fe..9d05e158fe0e 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1527,6 +1527,70 @@ int intel_link_process_wakeen_event(struct auxiliary_device *auxdev)
>   * PM calls
>   */
>  
> +static int intel_resume_child_device(struct device *dev, void *data)
> +{
> +	int ret;
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +
> +	if (!slave->probed) {
> +		dev_dbg(dev, "%s: skipping device, no probed driver\n", __func__);
> +		return 0;
> +	}
> +	if (!slave->dev_num_sticky) {
> +		dev_dbg(dev, "%s: skipping device, never detected on bus\n", __func__);
> +		return 0;
> +	}
> +
> +	ret = pm_request_resume(dev);
> +	if (ret < 0)
> +		dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static int __maybe_unused intel_pm_prepare(struct device *dev)
> +{
> +	struct sdw_cdns *cdns = dev_get_drvdata(dev);
> +	struct sdw_intel *sdw = cdns_to_intel(cdns);
> +	struct sdw_bus *bus = &cdns->bus;
> +	u32 clock_stop_quirks;
> +	int ret = 0;
> +
> +	if (bus->prop.hw_disabled || !sdw->startup_done) {
> +		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
> +			bus->link_id);
> +		return 0;
> +	}
> +
> +	clock_stop_quirks = sdw->link_res->clock_stop_quirks;
> +
> +	if ((clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) ||
> +	    !clock_stop_quirks) {
> +		/*
> +		 * Try to resume the entire bus (parent + child devices) to exit
> +		 * the clock stop mode. If this fails, we keep going since we don't want
> +		 * to prevent system suspend from happening and errors should be recoverable
> +		 * on resume.
> +		 */
> +		ret = device_for_each_child(bus->dev, NULL, intel_resume_child_device);
> +
> +		if (ret < 0)
> +			dev_err(dev, "%s: intel_resume_child_device failed: %d\n", __func__, ret);
> +
> +		/*
> +		 * in the case where a link was started but does not have anything connected,
> +		 * we still need to resume to keep link power up/down sequences balanced.
> +		 * This is a no-op if a child device was present, since resuming the child
> +		 * device would also resume the parent
> +		 */
> +		ret = pm_request_resume(dev);

I am not sure of this patch yet, maybe I am comprehending it..

1. In above you are calling resume of child devices first and then intel
device, which sounds reverse, should you not resume intel device first
and then child (codec devices) ?

2. What about when resume is invoked by the core for the child devices.
That would be called in the PM resume flow, so why do it here?

> +		if (ret < 0)
> +			dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret);
> +	}
> +
> +	return 0;
> +}
> +
>  static int __maybe_unused intel_suspend(struct device *dev)
>  {
>  	struct sdw_cdns *cdns = dev_get_drvdata(dev);
> @@ -1923,6 +1987,7 @@ static int __maybe_unused intel_resume_runtime(struct device *dev)
>  }
>  
>  static const struct dev_pm_ops intel_pm = {
> +	.prepare = intel_pm_prepare,
>  	SET_SYSTEM_SLEEP_PM_OPS(intel_suspend, intel_resume)
>  	SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL)
>  };
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 2/4] soundwire: intel: skip suspend/resume/wake when link was not started

[Pierre-Louis Bossart]

>> On some HDaudio platforms, SoundWire devices are described in the
>> DSDT but never used. This patch adds a boolean status flag to skip all
>> suspend/resume/wake sequences for this configuration.
> 
> Why are the sdw devices created in this case then? I would assume you
> are detecting this configuration and should skip device creation?

The SDW Linux devices are created during the probe step, based on
information extracted from platform firmware. Since we see a matching
driver, there will be a probe and that driver also contains pm ops.

We only know if the physical peripherals described in ACPI are real or
not during the startup() phase. After the bus reset, SoundWire
peripherals will report as ATTACHED as Device0 and the enumeration starts.

So in these HDaudio cases, we create the Linux devices based on
incorrect ACPI information, but since we detect an HDaudio configuration
we never start the links and the suspend-resume fails.

For a bit of historical context, the decision to handle devices in this
way was not the original proposal from Intel. In the initial patches,
the Linux devices were created when their matching physical peripheral
was showing signs of activity and attached after synchronizing. We
modified this behavior so that a device driver could use out-of-band
signaling to power-up a peripheral so that it could attach. That wasn't
a bad idea, but that also exposes a number of 'ghost devices' that are
not real. And unfortunately the Intel BIOS reference keeps using those
invalid _ADR values...

Does this answer to the question?

Re: [PATCH 3/4] soundwire: intel: exit clock stop mode on system suspend

[Pierre-Louis Bossart]

>> +static int __maybe_unused intel_pm_prepare(struct device *dev)
>> +{
>> +	struct sdw_cdns *cdns = dev_get_drvdata(dev);
>> +	struct sdw_intel *sdw = cdns_to_intel(cdns);
>> +	struct sdw_bus *bus = &cdns->bus;
>> +	u32 clock_stop_quirks;
>> +	int ret = 0;
>> +
>> +	if (bus->prop.hw_disabled || !sdw->startup_done) {
>> +		dev_dbg(dev, "SoundWire master %d is disabled or not-started, ignoring\n",
>> +			bus->link_id);
>> +		return 0;
>> +	}
>> +
>> +	clock_stop_quirks = sdw->link_res->clock_stop_quirks;
>> +
>> +	if ((clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) ||
>> +	    !clock_stop_quirks) {
>> +		/*
>> +		 * Try to resume the entire bus (parent + child devices) to exit
>> +		 * the clock stop mode. If this fails, we keep going since we don't want
>> +		 * to prevent system suspend from happening and errors should be recoverable
>> +		 * on resume.
>> +		 */
>> +		ret = device_for_each_child(bus->dev, NULL, intel_resume_child_device);
>> +
>> +		if (ret < 0)
>> +			dev_err(dev, "%s: intel_resume_child_device failed: %d\n", __func__, ret);
>> +
>> +		/*
>> +		 * in the case where a link was started but does not have anything connected,
>> +		 * we still need to resume to keep link power up/down sequences balanced.
>> +		 * This is a no-op if a child device was present, since resuming the child
>> +		 * device would also resume the parent
>> +		 */
>> +		ret = pm_request_resume(dev);
> 
> I am not sure of this patch yet, maybe I am comprehending it..
> 
> 1. In above you are calling resume of child devices first and then intel
> device, which sounds reverse, should you not resume intel device first
> and then child (codec devices) ?
> 
> 2. What about when resume is invoked by the core for the child devices.
> That would be called in the PM resume flow, so why do it here?

I realize it's a complicated sequence, it took us multiple phases to get
it right. There are multiple layers between power domain, bus and driver.

The .prepare phase happens before the system suspend phase. Unlike
suspend, which progresses from children to parents, the .prepare is
handled parent first.

When we do a request_resume of the child device, by construction that
also resumes the parent. In other words, if we have multiple codecs on a
link, the first iteration of device_for_each_child() will already resume
the parent and the first device, and the second iteration will only
resume the second device.

What this step does is make sure than when the codec .suspend routine is
invoked, the entire bus is already back to full power. I did check
privately with Rafael (CC:ed) if this sequence was legit.

We did consider modifying the system suspend callback in codec drivers,
so that we would do a pm_runtime resume(). This is functionally
equivalent to what we are suggesting here, but we decided not to do so
for two main reasons

a) lots of code changes across all codecs for an Intel-specific issue

b) we would need to add a flag so that codec drivers would know in which
Intel-specific clock-stop mode the bus was configured. That's not so
good either.

It seemed simpler to use to add this .prepare step and test on the Intel
clock stop mode before doing a pm_runtime_resume for all codecs.

> 
>> +		if (ret < 0)
>> +			dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int __maybe_unused intel_suspend(struct device *dev)
>>  {
>>  	struct sdw_cdns *cdns = dev_get_drvdata(dev);
>> @@ -1923,6 +1987,7 @@ static int __maybe_unused intel_resume_runtime(struct device *dev)
>>  }
>>  
>>  static const struct dev_pm_ops intel_pm = {
>> +	.prepare = intel_pm_prepare,
>>  	SET_SYSTEM_SLEEP_PM_OPS(intel_suspend, intel_resume)
>>  	SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL)
>>  };
>> -- 
>> 2.17.1
> 

Re: [PATCH 3/4] soundwire: intel: exit clock stop mode on system suspend

[Pierre-Louis Bossart]

>> 1. In above you are calling resume of child devices first and then intel
>> device, which sounds reverse, should you not resume intel device first
>> and then child (codec devices) ?
>>
>> 2. What about when resume is invoked by the core for the child devices.
>> That would be called in the PM resume flow, so why do it here?
> 
> I realize it's a complicated sequence, it took us multiple phases to get
> it right. There are multiple layers between power domain, bus and driver.
> 
> The .prepare phase happens before the system suspend phase. Unlike
> suspend, which progresses from children to parents, the .prepare is
> handled parent first.
> 
> When we do a request_resume of the child device, by construction that
> also resumes the parent. In other words, if we have multiple codecs on a
> link, the first iteration of device_for_each_child() will already resume
> the parent and the first device, and the second iteration will only
> resume the second device.
> 
> What this step does is make sure than when the codec .suspend routine is
> invoked, the entire bus is already back to full power. I did check
> privately with Rafael (CC:ed) if this sequence was legit.
> 
> We did consider modifying the system suspend callback in codec drivers,
> so that we would do a pm_runtime resume(). This is functionally
> equivalent to what we are suggesting here, but we decided not to do so
> for two main reasons
> 
> a) lots of code changes across all codecs for an Intel-specific issue
> 
> b) we would need to add a flag so that codec drivers would know in which
> Intel-specific clock-stop mode the bus was configured. That's not so
> good either.
> 
> It seemed simpler to use to add this .prepare step and test on the Intel
> clock stop mode before doing a pm_runtime_resume for all codecs.

Note that we could invert the two parts and do a parent resume first,
and a loop for all children second. It's completely equivalent, but
might be less convoluted to understand without any implicit behavior
assumed.

	if ((clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) ||
	    !clock_stop_quirks) {

		/* resume parent first */
		ret = pm_request_resume(dev);
		if (ret < 0)
			dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret);

		/*
		 * resume all children next.
		 * if there are no children on this link,
		 * this is a no-op
		 */
		ret = device_for_each_child(bus->dev, NULL, intel_resume_child_device);

		if (ret < 0)
			dev_err(dev, "%s: intel_resume_child_device failed: %d\n", __func__,
ret);
	}
	

Re: [PATCH 2/4] soundwire: intel: skip suspend/resume/wake when link was not started

[Vinod Koul]

On 02-08-21, 08:59, Pierre-Louis Bossart wrote:
> 
> 
> 
> >> On some HDaudio platforms, SoundWire devices are described in the
> >> DSDT but never used. This patch adds a boolean status flag to skip all
> >> suspend/resume/wake sequences for this configuration.
> > 
> > Why are the sdw devices created in this case then? I would assume you
> > are detecting this configuration and should skip device creation?
> 
> The SDW Linux devices are created during the probe step, based on
> information extracted from platform firmware. Since we see a matching
> driver, there will be a probe and that driver also contains pm ops.
> 
> We only know if the physical peripherals described in ACPI are real or
> not during the startup() phase. After the bus reset, SoundWire
> peripherals will report as ATTACHED as Device0 and the enumeration starts.
> 
> So in these HDaudio cases, we create the Linux devices based on
> incorrect ACPI information, but since we detect an HDaudio configuration
> we never start the links and the suspend-resume fails.
> 
> For a bit of historical context, the decision to handle devices in this
> way was not the original proposal from Intel. In the initial patches,
> the Linux devices were created when their matching physical peripheral
> was showing signs of activity and attached after synchronizing. We
> modified this behavior so that a device driver could use out-of-band
> signaling to power-up a peripheral so that it could attach. That wasn't
> a bad idea, but that also exposes a number of 'ghost devices' that are
> not real. And unfortunately the Intel BIOS reference keeps using those
> invalid _ADR values...
> 
> Does this answer to the question?

Yes it does thanks, very helpful.

Can we add this to the changelog, am sure down the line people might
forget why it was added.

-- 
~Vinod

Re: [PATCH 3/4] soundwire: intel: exit clock stop mode on system suspend

[Vinod Koul]

On 02-08-21, 11:28, Pierre-Louis Bossart wrote:
> 
> 
> 
> >> 1. In above you are calling resume of child devices first and then intel
> >> device, which sounds reverse, should you not resume intel device first
> >> and then child (codec devices) ?
> >>
> >> 2. What about when resume is invoked by the core for the child devices.
> >> That would be called in the PM resume flow, so why do it here?
> > 
> > I realize it's a complicated sequence, it took us multiple phases to get
> > it right. There are multiple layers between power domain, bus and driver.
> > 
> > The .prepare phase happens before the system suspend phase. Unlike
> > suspend, which progresses from children to parents, the .prepare is
> > handled parent first.
> > 
> > When we do a request_resume of the child device, by construction that
> > also resumes the parent. In other words, if we have multiple codecs on a
> > link, the first iteration of device_for_each_child() will already resume
> > the parent and the first device, and the second iteration will only
> > resume the second device.
> > 
> > What this step does is make sure than when the codec .suspend routine is
> > invoked, the entire bus is already back to full power. I did check
> > privately with Rafael (CC:ed) if this sequence was legit.
> > 
> > We did consider modifying the system suspend callback in codec drivers,
> > so that we would do a pm_runtime resume(). This is functionally
> > equivalent to what we are suggesting here, but we decided not to do so
> > for two main reasons
> > 
> > a) lots of code changes across all codecs for an Intel-specific issue
> > 
> > b) we would need to add a flag so that codec drivers would know in which
> > Intel-specific clock-stop mode the bus was configured. That's not so
> > good either.
> > 
> > It seemed simpler to use to add this .prepare step and test on the Intel
> > clock stop mode before doing a pm_runtime_resume for all codecs.

Ack, the code looks neat. But glancing at it, reader might get confused
about the sequencing done here.. It is not very obvious, so consider
adding this to changelog or driver comments. It will be helpful

> 
> Note that we could invert the two parts and do a parent resume first,
> and a loop for all children second. It's completely equivalent, but
> might be less convoluted to understand without any implicit behavior
> assumed.

Agree, it would be redundant as PM core would take care of it. maybe
add a comment so that it is explicit

-- 
~Vinod

Re: [PATCH 2/4] soundwire: intel: skip suspend/resume/wake when link was not started

[Pierre-Louis Bossart]

On 8/6/21 8:24 AM, Vinod Koul wrote:
> On 02-08-21, 08:59, Pierre-Louis Bossart wrote:
>>
>>
>>
>>>> On some HDaudio platforms, SoundWire devices are described in the
>>>> DSDT but never used. This patch adds a boolean status flag to skip all
>>>> suspend/resume/wake sequences for this configuration.
>>>
>>> Why are the sdw devices created in this case then? I would assume you
>>> are detecting this configuration and should skip device creation?
>>
>> The SDW Linux devices are created during the probe step, based on
>> information extracted from platform firmware. Since we see a matching
>> driver, there will be a probe and that driver also contains pm ops.
>>
>> We only know if the physical peripherals described in ACPI are real or
>> not during the startup() phase. After the bus reset, SoundWire
>> peripherals will report as ATTACHED as Device0 and the enumeration starts.
>>
>> So in these HDaudio cases, we create the Linux devices based on
>> incorrect ACPI information, but since we detect an HDaudio configuration
>> we never start the links and the suspend-resume fails.
>>
>> For a bit of historical context, the decision to handle devices in this
>> way was not the original proposal from Intel. In the initial patches,
>> the Linux devices were created when their matching physical peripheral
>> was showing signs of activity and attached after synchronizing. We
>> modified this behavior so that a device driver could use out-of-band
>> signaling to power-up a peripheral so that it could attach. That wasn't
>> a bad idea, but that also exposes a number of 'ghost devices' that are
>> not real. And unfortunately the Intel BIOS reference keeps using those
>> invalid _ADR values...
>>
>> Does this answer to the question?
> 
> Yes it does thanks, very helpful.
> 
> Can we add this to the changelog, am sure down the line people might
> forget why it was added.

yes, will do.

Re: [PATCH 3/4] soundwire: intel: exit clock stop mode on system suspend

[Pierre-Louis Bossart]

On 8/6/21 8:31 AM, Vinod Koul wrote:
> On 02-08-21, 11:28, Pierre-Louis Bossart wrote:
>>
>>
>>
>>>> 1. In above you are calling resume of child devices first and then intel
>>>> device, which sounds reverse, should you not resume intel device first
>>>> and then child (codec devices) ?
>>>>
>>>> 2. What about when resume is invoked by the core for the child devices.
>>>> That would be called in the PM resume flow, so why do it here?
>>>
>>> I realize it's a complicated sequence, it took us multiple phases to get
>>> it right. There are multiple layers between power domain, bus and driver.
>>>
>>> The .prepare phase happens before the system suspend phase. Unlike
>>> suspend, which progresses from children to parents, the .prepare is
>>> handled parent first.
>>>
>>> When we do a request_resume of the child device, by construction that
>>> also resumes the parent. In other words, if we have multiple codecs on a
>>> link, the first iteration of device_for_each_child() will already resume
>>> the parent and the first device, and the second iteration will only
>>> resume the second device.
>>>
>>> What this step does is make sure than when the codec .suspend routine is
>>> invoked, the entire bus is already back to full power. I did check
>>> privately with Rafael (CC:ed) if this sequence was legit.
>>>
>>> We did consider modifying the system suspend callback in codec drivers,
>>> so that we would do a pm_runtime resume(). This is functionally
>>> equivalent to what we are suggesting here, but we decided not to do so
>>> for two main reasons
>>>
>>> a) lots of code changes across all codecs for an Intel-specific issue
>>>
>>> b) we would need to add a flag so that codec drivers would know in which
>>> Intel-specific clock-stop mode the bus was configured. That's not so
>>> good either.
>>>
>>> It seemed simpler to use to add this .prepare step and test on the Intel
>>> clock stop mode before doing a pm_runtime_resume for all codecs.
> 
> Ack, the code looks neat. But glancing at it, reader might get confused
> about the sequencing done here.. It is not very obvious, so consider
> adding this to changelog or driver comments. It will be helpful

Yep, even in internal reviews this was far from straightforward to
explain. I added comments but I can certainly try to explain more.

>>
>> Note that we could invert the two parts and do a parent resume first,
>> and a loop for all children second. It's completely equivalent, but
>> might be less convoluted to understand without any implicit behavior
>> assumed.
> 
> Agree, it would be redundant as PM core would take care of it. maybe
> add a comment so that it is explicit

Will add comments as well.

Note that I have another lead to further improve suspend-resume, running
stress tests on thousands of cycles atm. I'll wait until we have more
results to resubmit this series.

Thanks for the reviews!