[PATCH v2 0/3] ASoC: SOF: suspend/resume debug tools

[PATCH v2 0/3] ASoC: SOF: suspend/resume debug tools

[Pierre-Louis Bossart]

These patches were submitted in an earlier series but not merged and
then conflicted with the IPC flood test.

Resending, no new functionality. These helpers are really helpful for
e.g. SoundWire or HDaudio bringup.

Pierre-Louis Bossart (3):
  ASoC: SOF: trace: move to opt-in with Kconfig and module parameter
  ASoC: SOF: acpi: add module param to disable pm_runtime
  ASoC: SOF: pci: add module param to disable pm_runtime

 sound/soc/sof/Kconfig        |  8 ++++++++
 sound/soc/sof/core.c         | 26 ++++++++++++++++++++------
 sound/soc/sof/sof-acpi-dev.c | 12 +++++++++++-
 sound/soc/sof/sof-pci-dev.c  | 12 +++++++++++-
 sound/soc/sof/sof-priv.h     |  1 +
 sound/soc/sof/trace.c        | 17 ++++++++++++++++-
 6 files changed, 67 insertions(+), 9 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] ASoC: SOF: trace: move to opt-in with Kconfig and module parameter

[Pierre-Louis Bossart]

In a number of debug cases, the DMA-based trace can add problems
(e.g. with HDaudio channel allocation). It also generates additional
traffic on the bus and if the DMA handling is unreliable will prevent
audio use-cases from working normally. Using the trace also requires
tools to be installed on the target.

The trace can be instead handled as dynamic debug. We can use a
Kconfig to force the trace to be enabled in all cases, or use a module
parameter to enable it on a need-basis, e.g. by setting "options
snd_sof sof_debug=0x1" in a /etc/modprobe.d file.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/Kconfig    |  8 ++++++++
 sound/soc/sof/core.c     | 26 ++++++++++++++++++++------
 sound/soc/sof/sof-priv.h |  1 +
 sound/soc/sof/trace.c    | 17 ++++++++++++++++-
 4 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
index 41f79cdcbf47..efb4cf3eec56 100644
--- a/sound/soc/sof/Kconfig
+++ b/sound/soc/sof/Kconfig
@@ -132,6 +132,14 @@ config SND_SOC_SOF_DEBUG_ENABLE_DEBUGFS_CACHE
 	  Say Y if you want to enable caching the memory windows.
 	  If unsure, select "N".
 
+config SND_SOC_SOF_DEBUG_ENABLE_FIRMWARE_TRACE
+	bool "SOF enable firmware trace"
+	help
+	  The firmware trace can be enabled either at build-time with
+	  this option, or dynamically by setting flags in the SOF core
+	  module parameter (similar to dynamic debug)
+	  If unsure, select "N".
+
 config SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST
 	bool "SOF enable IPC flood test"
 	help
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
index 5beda47cdf9f..70b471be07b2 100644
--- a/sound/soc/sof/core.c
+++ b/sound/soc/sof/core.c
@@ -16,6 +16,12 @@
 #include "sof-priv.h"
 #include "ops.h"
 
+static int sof_core_debug;
+module_param_named(sof_debug, sof_core_debug, int, 0444);
+MODULE_PARM_DESC(sof_debug, "SOF core debug options (0x0 all off)");
+
+#define SOF_CORE_ENABLE_TRACE BIT(0)
+
 /* SOF defaults if not provided by the platform in ms */
 #define TIMEOUT_DEFAULT_IPC_MS  5
 #define TIMEOUT_DEFAULT_BOOT_MS 100
@@ -350,12 +356,20 @@ static int sof_probe_continue(struct snd_sof_dev *sdev)
 		goto fw_run_err;
 	}
 
-	/* init DMA trace */
-	ret = snd_sof_init_trace(sdev);
-	if (ret < 0) {
-		/* non fatal */
-		dev_warn(sdev->dev,
-			 "warning: failed to initialize trace %d\n", ret);
+	if (IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_ENABLE_FIRMWARE_TRACE) ||
+	    (sof_core_debug & SOF_CORE_ENABLE_TRACE)) {
+		sdev->dtrace_is_supported = true;
+
+		/* init DMA trace */
+		ret = snd_sof_init_trace(sdev);
+		if (ret < 0) {
+			/* non fatal */
+			dev_warn(sdev->dev,
+				 "warning: failed to initialize trace %d\n",
+				 ret);
+		}
+	} else {
+		dev_dbg(sdev->dev, "SOF firmware trace disabled\n");
 	}
 
 	/* hereafter all FW boot flows are for PM reasons */
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 17f3d2a5a701..563623bcaad6 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -419,6 +419,7 @@ struct snd_sof_dev {
 	int dma_trace_pages;
 	wait_queue_head_t trace_sleep;
 	u32 host_offset;
+	u32 dtrace_is_supported; /* set with Kconfig or module parameter */
 	u32 dtrace_is_enabled;
 	u32 dtrace_error;
 	u32 dtrace_draining;
diff --git a/sound/soc/sof/trace.c b/sound/soc/sof/trace.c
index befed975161c..84d89206a0e0 100644
--- a/sound/soc/sof/trace.c
+++ b/sound/soc/sof/trace.c
@@ -167,6 +167,9 @@ int snd_sof_init_trace_ipc(struct snd_sof_dev *sdev)
 	struct sof_ipc_reply ipc_reply;
 	int ret;
 
+	if (!sdev->dtrace_is_supported)
+		return 0;
+
 	if (sdev->dtrace_is_enabled || !sdev->dma_trace_pages)
 		return -EINVAL;
 
@@ -227,6 +230,9 @@ int snd_sof_init_trace(struct snd_sof_dev *sdev)
 {
 	int ret;
 
+	if (!sdev->dtrace_is_supported)
+		return 0;
+
 	/* set false before start initialization */
 	sdev->dtrace_is_enabled = false;
 
@@ -282,6 +288,9 @@ EXPORT_SYMBOL(snd_sof_init_trace);
 int snd_sof_trace_update_pos(struct snd_sof_dev *sdev,
 			     struct sof_ipc_dma_trace_posn *posn)
 {
+	if (!sdev->dtrace_is_supported)
+		return 0;
+
 	if (sdev->dtrace_is_enabled && sdev->host_offset != posn->host_offset) {
 		sdev->host_offset = posn->host_offset;
 		wake_up(&sdev->trace_sleep);
@@ -298,6 +307,9 @@ int snd_sof_trace_update_pos(struct snd_sof_dev *sdev,
 /* an error has occurred within the DSP that prevents further trace */
 void snd_sof_trace_notify_for_error(struct snd_sof_dev *sdev)
 {
+	if (!sdev->dtrace_is_supported)
+		return;
+
 	if (sdev->dtrace_is_enabled) {
 		dev_err(sdev->dev, "error: waking up any trace sleepers\n");
 		sdev->dtrace_error = true;
@@ -310,7 +322,7 @@ void snd_sof_release_trace(struct snd_sof_dev *sdev)
 {
 	int ret;
 
-	if (!sdev->dtrace_is_enabled)
+	if (!sdev->dtrace_is_supported || !sdev->dtrace_is_enabled)
 		return;
 
 	ret = snd_sof_dma_trace_trigger(sdev, SNDRV_PCM_TRIGGER_STOP);
@@ -331,6 +343,9 @@ EXPORT_SYMBOL(snd_sof_release_trace);
 
 void snd_sof_free_trace(struct snd_sof_dev *sdev)
 {
+	if (!sdev->dtrace_is_supported)
+		return;
+
 	snd_sof_release_trace(sdev);
 
 	snd_dma_free_pages(&sdev->dmatb);
-- 
2.20.1

[PATCH v2 2/3] ASoC: SOF: acpi: add module param to disable pm_runtime

[Pierre-Louis Bossart]

Add debug option to disable pm_runtime. This is not intended for
production devices but is very useful for platform bringup.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/sof-acpi-dev.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/sound/soc/sof/sof-acpi-dev.c b/sound/soc/sof/sof-acpi-dev.c
index c8dafb1ac54e..93a8e15bbd2c 100644
--- a/sound/soc/sof/sof-acpi-dev.c
+++ b/sound/soc/sof/sof-acpi-dev.c
@@ -29,6 +29,12 @@ static char *tplg_path;
 module_param(tplg_path, charp, 0444);
 MODULE_PARM_DESC(tplg_path, "alternate path for SOF topology.");
 
+static int sof_acpi_debug;
+module_param_named(sof_debug, sof_acpi_debug, int, 0444);
+MODULE_PARM_DESC(sof_debug, "SOF ACPI debug options (0x0 all off)");
+
+#define SOF_ACPI_DISABLE_PM_RUNTIME BIT(0)
+
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HASWELL)
 static const struct sof_dev_desc sof_acpi_haswell_desc = {
 	.machines = snd_soc_acpi_intel_haswell_machines,
@@ -121,6 +127,9 @@ static const struct dev_pm_ops sof_acpi_pm = {
 
 static void sof_acpi_probe_complete(struct device *dev)
 {
+	if (sof_acpi_debug & SOF_ACPI_DISABLE_PM_RUNTIME)
+		return;
+
 	/* allow runtime_pm */
 	pm_runtime_set_autosuspend_delay(dev, SND_SOF_SUSPEND_DELAY_MS);
 	pm_runtime_use_autosuspend(dev);
@@ -221,7 +230,8 @@ static int sof_acpi_probe(struct platform_device *pdev)
 
 static int sof_acpi_remove(struct platform_device *pdev)
 {
-	pm_runtime_disable(&pdev->dev);
+	if (!(sof_acpi_debug & SOF_ACPI_DISABLE_PM_RUNTIME))
+		pm_runtime_disable(&pdev->dev);
 
 	/* call sof helper for DSP hardware remove */
 	snd_sof_device_remove(&pdev->dev);
-- 
2.20.1

[PATCH v2 3/3] ASoC: SOF: pci: add module param to disable pm_runtime

[Pierre-Louis Bossart]

Add debug option to disable pm_runtime. This is not intended for
production devices but is very useful for platform bringup.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/sof-pci-dev.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c
index ab58d4f9119f..ffe485f4ba71 100644
--- a/sound/soc/sof/sof-pci-dev.c
+++ b/sound/soc/sof/sof-pci-dev.c
@@ -29,6 +29,12 @@ static char *tplg_path;
 module_param(tplg_path, charp, 0444);
 MODULE_PARM_DESC(tplg_path, "alternate path for SOF topology.");
 
+static int sof_pci_debug;
+module_param_named(sof_debug, sof_pci_debug, int, 0444);
+MODULE_PARM_DESC(sof_debug, "SOF PCI debug options (0x0 all off)");
+
+#define SOF_PCI_DISABLE_PM_RUNTIME BIT(0)
+
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_APOLLOLAKE)
 static const struct sof_dev_desc bxt_desc = {
 	.machines		= snd_soc_acpi_intel_bxt_machines,
@@ -213,6 +219,9 @@ static void sof_pci_probe_complete(struct device *dev)
 {
 	dev_dbg(dev, "Completing SOF PCI probe");
 
+	if (sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME)
+		return;
+
 	/* allow runtime_pm */
 	pm_runtime_set_autosuspend_delay(dev, SND_SOF_SUSPEND_DELAY_MS);
 	pm_runtime_use_autosuspend(dev);
@@ -333,7 +342,8 @@ static void sof_pci_remove(struct pci_dev *pci)
 	snd_sof_device_remove(&pci->dev);
 
 	/* follow recommendation in pci-driver.c to increment usage counter */
-	pm_runtime_get_noresume(&pci->dev);
+	if (!(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME))
+		pm_runtime_get_noresume(&pci->dev);
 
 	/* release pci regions and disable device */
 	pci_release_regions(pci);
-- 
2.20.1

Re: [PATCH v2 1/3] ASoC: SOF: trace: move to opt-in with Kconfig and module parameter

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 400 bytes --]

On Wed, Jun 12, 2019 at 11:47:24AM -0500, Pierre-Louis Bossart wrote:

> +	    (sof_core_debug & SOF_CORE_ENABLE_TRACE)) {
> +		sdev->dtrace_is_supported = true;

Given that dtrace is a widely known product with a Linux implementation
can we not call this dtrace?  use_dma_trace or something?  A long name
doesn't seem too bad here but a collision with the more common usage of
the name isn't great.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 2/3] ASoC: SOF: acpi: add module param to disable pm_runtime

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 419 bytes --]

On Wed, Jun 12, 2019 at 11:47:25AM -0500, Pierre-Louis Bossart wrote:
> Add debug option to disable pm_runtime. This is not intended for
> production devices but is very useful for platform bringup.

I can't immediately find it right now but isn't there some generic way
of doing this in the runtime PM framework?  If not it seems like it'd be
a good thing to add, these can't be the only devices where it'd be
useful.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 2/3] ASoC: SOF: acpi: add module param to disable pm_runtime

[Takashi Iwai]

On Thu, 13 Jun 2019 20:48:01 +0200,
Mark Brown wrote:
> 
> On Wed, Jun 12, 2019 at 11:47:25AM -0500, Pierre-Louis Bossart wrote:
> > Add debug option to disable pm_runtime. This is not intended for
> > production devices but is very useful for platform bringup.
> 
> I can't immediately find it right now but isn't there some generic way
> of doing this in the runtime PM framework?  If not it seems like it'd be
> a good thing to add, these can't be the only devices where it'd be
> useful.

Well, runtime PM can be fully controlled via sysfs, but the problem is
that the driver declares itself being runtime-enabled.
So, either we leave it default and let user-space enabling it (via
udev or other way), or introduce some condition in the driver side.


Takashi

Re: [PATCH v2 2/3] ASoC: SOF: acpi: add module param to disable pm_runtime

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 752 bytes --]

On Thu, Jun 13, 2019 at 09:04:28PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > I can't immediately find it right now but isn't there some generic way
> > of doing this in the runtime PM framework?  If not it seems like it'd be
> > a good thing to add, these can't be the only devices where it'd be
> > useful.

> Well, runtime PM can be fully controlled via sysfs, but the problem is
> that the driver declares itself being runtime-enabled.
> So, either we leave it default and let user-space enabling it (via
> udev or other way), or introduce some condition in the driver side.

I thought someone had added a command line parameter to do it based on
dev_name(), perhaps they were just talking about it or it was in some
BSP somewhere though.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/3] ASoC: SOF: trace: move to opt-in with Kconfig and module parameter

[Pierre-Louis Bossart]

On 6/13/19 8:44 PM, Mark Brown wrote:
> On Wed, Jun 12, 2019 at 11:47:24AM -0500, Pierre-Louis Bossart wrote:
> 
>> +	    (sof_core_debug & SOF_CORE_ENABLE_TRACE)) {
>> +		sdev->dtrace_is_supported = true;
> 
> Given that dtrace is a widely known product with a Linux implementation
> can we not call this dtrace?  use_dma_trace or something?  A long name
> doesn't seem too bad here but a collision with the more common usage of
> the name isn't great.

Sure, we can rename these fields. I'll send a v3.

Re: [PATCH v2 2/3] ASoC: SOF: acpi: add module param to disable pm_runtime

[Pierre-Louis Bossart]

>>> I can't immediately find it right now but isn't there some generic way
>>> of doing this in the runtime PM framework?  If not it seems like it'd be
>>> a good thing to add, these can't be the only devices where it'd be
>>> useful.
> 
>> Well, runtime PM can be fully controlled via sysfs, but the problem is
>> that the driver declares itself being runtime-enabled.
>> So, either we leave it default and let user-space enabling it (via
>> udev or other way), or introduce some condition in the driver side.
> 
> I thought someone had added a command line parameter to do it based on
> dev_name(), perhaps they were just talking about it or it was in some
> BSP somewhere though.

If there is a better way I am all ears. It's indeed not very elegant to 
duplicate the same parameter for two different modules and it's not an 
SOF-specific need.

The only way I am aware of is to play with 
/sys/bus/pci/devices/xyz/power/ files but it's not very useful if you 
want to disable the initial runtime pm transition which is often the 
more problematic one. Completely removing runtime_pm support from all 
drivers at compile time is also not very good either.