diff mbox series

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

Message ID 20210727055608.30247-3-yung-chuan.liao@linux.intel.com (mailing list archive)
State Superseded
Headers show
Series soundwire: intel: exit clock-stop mode before system suspend | expand

Commit Message

Bard Liao July 27, 2021, 5:56 a.m. UTC 
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(-)

Comments

Vinod Koul Aug. 2, 2021, 4:02 a.m. UTC | #1 
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
Pierre-Louis Bossart Aug. 2, 2021, 1:59 p.m. UTC | #2 
>> 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?
Vinod Koul Aug. 6, 2021, 1:24 p.m. UTC | #3 
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.
Pierre-Louis Bossart Aug. 6, 2021, 3:57 p.m. UTC | #4 
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.
diff mbox series

Patch

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