All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bard Liao <yung-chuan.liao@linux.intel.com>
To: alsa-devel@alsa-project.org, vkoul@kernel.org
Cc: vinod.koul@linaro.org, linux-kernel@vger.kernel.org,
	gregkh@linuxfoundation.org, srinivas.kandagatla@linaro.org,
	rander.wang@linux.intel.com,
	pierre-louis.bossart@linux.intel.com, sanyog.r.kale@intel.com,
	bard.liao@intel.com
Subject: [PATCH v2 2/3] soundwire: intel: skip suspend/resume/wake when link was not started
Date: Wed, 18 Aug 2021 10:49:53 +0800	[thread overview]
Message-ID: <20210818024954.16873-3-yung-chuan.liao@linux.intel.com> (raw)
In-Reply-To: <20210818024954.16873-1-yung-chuan.liao@linux.intel.com>

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

The SoundWire Linux devices are created purely based on information
provided by platform firmware (e.g. ACPI DSDT table). When the kernel
finds a matching driver for the device address (_ADR), the probe will
initialize required data structures and initialize pm ops.

When the SoundWire link is started at a later point, the physical
devices will synchronize on the SoundWire frames and report their
attachment status, thereby triggering the enumeration and
initialization of device registers.

This two-step solution was a conscious design decision to allow e.g. a
driver to use sideband mechanisms to turn power rails on. This can
also allow OEMs to describe multiple platforms with the same DSDT
table, the devices that are not physically present in hardware.

The drawback of this approach is a bit of confusion, with more devices
than are actually present in hardware. This results in 'ghost'
devices, for which the driver successfully probes, but that will not
generate any traffic on the bus. suspend-resume transitions are
handled by drivers, and skipped when the devices are not physically
present.

This patch provides a work-around for a second-level of confusion in
platform firmware: some platforms only use HDaudio links, but
nevertheless expose SoundWire 'ghost' devices. This results in error
messages in the Intel driver while trying to suspend/resume these
links. The simplest solution is to add a boolean status flag to skip
all suspend/resume/wake sequences if the link was never started.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@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 808dda1380c2..8b42053b171f 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1525,6 +1525,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:
@@ -1564,8 +1565,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;
 	}
 
@@ -1602,8 +1604,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;
 	}
@@ -1656,8 +1658,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;
 	}
@@ -1721,8 +1723,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;
 	}
@@ -1819,8 +1821,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


WARNING: multiple messages have this Message-ID (diff)
From: Bard Liao <yung-chuan.liao@linux.intel.com>
To: alsa-devel@alsa-project.org, vkoul@kernel.org
Cc: vinod.koul@linaro.org, gregkh@linuxfoundation.org,
	pierre-louis.bossart@linux.intel.com,
	linux-kernel@vger.kernel.org, sanyog.r.kale@intel.com,
	rander.wang@linux.intel.com, bard.liao@intel.com
Subject: [PATCH v2 2/3] soundwire: intel: skip suspend/resume/wake when link was not started
Date: Wed, 18 Aug 2021 10:49:53 +0800	[thread overview]
Message-ID: <20210818024954.16873-3-yung-chuan.liao@linux.intel.com> (raw)
In-Reply-To: <20210818024954.16873-1-yung-chuan.liao@linux.intel.com>

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

The SoundWire Linux devices are created purely based on information
provided by platform firmware (e.g. ACPI DSDT table). When the kernel
finds a matching driver for the device address (_ADR), the probe will
initialize required data structures and initialize pm ops.

When the SoundWire link is started at a later point, the physical
devices will synchronize on the SoundWire frames and report their
attachment status, thereby triggering the enumeration and
initialization of device registers.

This two-step solution was a conscious design decision to allow e.g. a
driver to use sideband mechanisms to turn power rails on. This can
also allow OEMs to describe multiple platforms with the same DSDT
table, the devices that are not physically present in hardware.

The drawback of this approach is a bit of confusion, with more devices
than are actually present in hardware. This results in 'ghost'
devices, for which the driver successfully probes, but that will not
generate any traffic on the bus. suspend-resume transitions are
handled by drivers, and skipped when the devices are not physically
present.

This patch provides a work-around for a second-level of confusion in
platform firmware: some platforms only use HDaudio links, but
nevertheless expose SoundWire 'ghost' devices. This results in error
messages in the Intel driver while trying to suspend/resume these
links. The simplest solution is to add a boolean status flag to skip
all suspend/resume/wake sequences if the link was never started.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@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 808dda1380c2..8b42053b171f 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1525,6 +1525,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:
@@ -1564,8 +1565,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;
 	}
 
@@ -1602,8 +1604,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;
 	}
@@ -1656,8 +1658,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;
 	}
@@ -1721,8 +1723,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;
 	}
@@ -1819,8 +1821,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


  parent reply	other threads:[~2021-08-18  2:50 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-18  2:49 [PATCH v2 0/3] soundwire: intel: exit clock-stop mode before system suspend Bard Liao
2021-08-18  2:49 ` Bard Liao
2021-08-18  2:49 ` [PATCH v2 1/3] soundwire: intel: fix potential race condition during power down Bard Liao
2021-08-18  2:49   ` Bard Liao
2021-08-18  2:49 ` Bard Liao [this message]
2021-08-18  2:49   ` [PATCH v2 2/3] soundwire: intel: skip suspend/resume/wake when link was not started Bard Liao
2021-08-18  2:49 ` [PATCH v2 3/3] soundwire: intel: conditionally exit clock stop mode on system suspend Bard Liao
2021-08-18  2:49   ` Bard Liao
2021-08-23 12:12 ` [PATCH v2 0/3] soundwire: intel: exit clock-stop mode before " Vinod Koul
2021-08-23 12:12   ` Vinod Koul

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210818024954.16873-3-yung-chuan.liao@linux.intel.com \
    --to=yung-chuan.liao@linux.intel.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=bard.liao@intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=rander.wang@linux.intel.com \
    --cc=sanyog.r.kale@intel.com \
    --cc=srinivas.kandagatla@linaro.org \
    --cc=vinod.koul@linaro.org \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.