LinuxLists.cc - [PATCH 00/11] soundwire: intel: add multi-link support

[permalink] [raw]

Subject: [PATCH 01/11] soundwire: intel: disable shim wake on suspend

From: Pierre-Louis Bossart <[email protected]>

If we enabled the clock stop mode and suspend, we need to disable the
shim wake. We do so only if the parent is pm_runtime active due to
power rail dependencies.

GitHub issue: https://github.com/thesofproject/linux/issues/1678
Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index dbcbe2708563..fe9b92fd48db 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1532,6 +1532,7 @@ static int intel_suspend(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;

if (bus->prop.hw_disabled) {
@@ -1543,6 +1544,23 @@ static int intel_suspend(struct device *dev)
if (pm_runtime_suspended(dev)) {
dev_dbg(dev, "%s: pm_runtime status: suspended\n", __func__);

+ 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);
+ }
+
return 0;
}

--
2.17.1

2020-08-18 14:36:31

[permalink] [raw]

Subject: [PATCH 02/11] soundwire: intel: ignore software command retries

From: Pierre-Louis Bossart <[email protected]>

with multiple links synchronized in hardware, retrying commands in
software is not recommended.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index fe9b92fd48db..c9ba706e20c6 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1355,6 +1355,11 @@ static int intel_master_probe(struct platform_device *pdev)
dev_info(dev,
"SoundWire master %d is disabled, will be ignored\n",
bus->link_id);
+ /*
+ * Ignore BIOS err_threshold, it's a really bad idea when dealing
+ * with multiple hardware synchronized links
+ */
+ bus->prop.err_threshold = 0;

return 0;
}
--
2.17.1

2020-08-18 14:36:42

[permalink] [raw]

Subject: [PATCH 04/11] soundwire: intel: add missing support for all clock stop modes

From: Pierre-Louis Bossart <[email protected]>

Deal with the BUS_RESET case, which is the default. The only change is
to add support for the exit sequence using the syncArm/syncGo mode for
the exit reset sequence.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.c | 49 +++++++++++++++++++++++++++++++--------
1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index a3aa8ab49285..f4441684bd7e 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1868,11 +1868,6 @@ static int intel_resume_runtime(struct device *dev)

if (!clock_stop0) {

- /*
- * Re-initialize the IP since it was powered-off
- */
- sdw_cdns_init(&sdw->cdns);
-
/*
* make sure all Slaves are tagged as UNATTACHED and
* provide reason for reinitialization
@@ -1880,13 +1875,31 @@ static int intel_resume_runtime(struct device *dev)

status = SDW_UNATTACH_REQUEST_MASTER_RESET;
sdw_clear_slave_status(bus, status);
- }

+ ret = sdw_cdns_enable_interrupt(cdns, true);
+ if (ret < 0) {
+ dev_err(dev, "cannot enable interrupts during resume\n");
+ return ret;
+ }

- ret = sdw_cdns_enable_interrupt(cdns, true);
- if (ret < 0) {
- dev_err(dev, "cannot enable interrupts during resume\n");
- return ret;
+ /*
+ * follow recommended programming flows to avoid
+ * timeouts when gsync is enabled
+ */
+ if (multi_link)
+ intel_shim_sync_arm(sdw);
+
+ /*
+ * Re-initialize the IP since it was powered-off
+ */
+ sdw_cdns_init(&sdw->cdns);
+
+ } else {
+ ret = sdw_cdns_enable_interrupt(cdns, true);
+ if (ret < 0) {
+ dev_err(dev, "cannot enable interrupts during resume\n");
+ return ret;
+ }
}

ret = sdw_cdns_clock_restart(cdns, !clock_stop0);
@@ -1894,6 +1907,22 @@ static int intel_resume_runtime(struct device *dev)
dev_err(dev, "unable to restart clock during resume\n");
return ret;
}
+
+ if (!clock_stop0) {
+ ret = sdw_cdns_exit_reset(cdns);
+ if (ret < 0) {
+ dev_err(dev, "unable to exit bus reset sequence during resume\n");
+ return ret;
+ }
+
+ if (multi_link) {
+ ret = intel_shim_sync_go(sdw);
+ if (ret < 0) {
+ dev_err(sdw->cdns.dev, "sync go failed during resume\n");
+ return ret;
+ }
+ }
+ }
} else if (!clock_stop_quirks) {
ret = intel_init(sdw);
if (ret) {
--
2.17.1

2020-08-18 14:36:47

[permalink] [raw]

Subject: [PATCH 05/11] soundwire: bus: update multi-link definition with hw sync details

From: Pierre-Louis Bossart <[email protected]>

Hardware-based synchronization is typically required when the
bus->multi_link flag is set.

On Intel platforms, when the Cadence IP is configured in 'Multi Master
Mode', the hardware synchronization is required even when a stream
only uses a single segment. The existing code only deal with hardware
synchronization when a stream uses more than one segment so to remain
backwards compatible we add a configuration threshold. For Intel cases
this threshold will be set to one, other platforms may be able to use
the SSP-based sync in those cases.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
include/linux/soundwire/sdw.h | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 76052f12c9f7..9adbe4fd7980 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -827,6 +827,11 @@ struct sdw_master_ops {
* @multi_link: Store bus property that indicates if multi links
* are supported. This flag is populated by drivers after reading
* appropriate firmware (ACPI/DT).
+ * @hw_sync_min_links: Number of links used by a stream above which
+ * hardware-based synchronization is required. This value is only
+ * meaningful if multi_link is set. If set to 1, hardware-based
+ * synchronization will be used even if a stream only uses a single
+ * SoundWire segment.
*/
struct sdw_bus {
struct device *dev;
@@ -850,6 +855,7 @@ struct sdw_bus {
unsigned int clk_stop_timeout;
u32 bank_switch_timeout;
bool multi_link;
+ int hw_sync_min_links;
};

int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
--
2.17.1

2020-08-18 14:37:06

[permalink] [raw]

Subject: [PATCH 06/11] soundwire: intel: add multi-link hw_synchronization information

From: Pierre-Louis Bossart <[email protected]>

set the flags as required by hardware implementation

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index f4441684bd7e..89a8ad1f80e8 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1400,7 +1400,14 @@ int intel_master_startup(struct platform_device *pdev)
dev_dbg(dev, "Multi-link is disabled\n");
bus->multi_link = false;
} else {
+ /*
+ * hardware-based synchronization is required regardless
+ * of the number of segments used by a stream: SSP-based
+ * synchronization is gated by gsync when the multi-master
+ * mode is set.
+ */
bus->multi_link = true;
+ bus->hw_sync_min_links = 1;
}

/* Initialize shim, controller */
--
2.17.1

2020-08-18 14:37:06

[permalink] [raw]

Subject: [PATCH 03/11] soundwire: intel: add multi-link support

From: Pierre-Louis Bossart <[email protected]>

The multi-link support is enabled with a hardware gsync signal
connecting all links. All commands and operations which typically are
handled on an SSP boundary will be deferred further and enabled across
all links with the 'syncGo' sequence.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.c | 120 ++++++++++++++++++++++++++++++++++----
1 file changed, 110 insertions(+), 10 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index c9ba706e20c6..a3aa8ab49285 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -34,6 +34,7 @@
#define SDW_INTEL_MASTER_DISABLE_PM_RUNTIME BIT(0)
#define SDW_INTEL_MASTER_DISABLE_CLOCK_STOP BIT(1)
#define SDW_INTEL_MASTER_DISABLE_PM_RUNTIME_IDLE BIT(2)
+#define SDW_INTEL_MASTER_DISABLE_MULTI_LINK BIT(3)

static int md_flags;
module_param_named(sdw_md_flags, md_flags, int, 0444);
@@ -555,6 +556,19 @@ static int intel_shim_sync_go_unlocked(struct sdw_intel *sdw)
return ret;
}

+static int intel_shim_sync_go(struct sdw_intel *sdw)
+{
+ int ret;
+
+ mutex_lock(sdw->link_res->shim_lock);
+
+ ret = intel_shim_sync_go_unlocked(sdw);
+
+ mutex_unlock(sdw->link_res->shim_lock);
+
+ return ret;
+}
+
/*
* PDI routines
*/
@@ -1303,10 +1317,7 @@ static int intel_init(struct sdw_intel *sdw)

intel_shim_init(sdw, clock_stop);

- if (clock_stop)
- return 0;
-
- return sdw_cdns_init(&sdw->cdns);
+ return 0;
}

/*
@@ -1372,6 +1383,7 @@ int intel_master_startup(struct platform_device *pdev)
struct sdw_intel *sdw = cdns_to_intel(cdns);
struct sdw_bus *bus = &cdns->bus;
int link_flags;
+ bool multi_link;
u32 clock_stop_quirks;
int ret;

@@ -1382,7 +1394,16 @@ int intel_master_startup(struct platform_device *pdev)
return 0;
}

- /* Initialize shim, controller and Cadence IP */
+ link_flags = md_flags >> (bus->link_id * 8);
+ multi_link = !(link_flags & SDW_INTEL_MASTER_DISABLE_MULTI_LINK);
+ if (!multi_link) {
+ dev_dbg(dev, "Multi-link is disabled\n");
+ bus->multi_link = false;
+ } else {
+ bus->multi_link = true;
+ }
+
+ /* Initialize shim, controller */
ret = intel_init(sdw);
if (ret)
goto err_init;
@@ -1401,12 +1422,33 @@ int intel_master_startup(struct platform_device *pdev)
goto err_init;
}

+ /*
+ * follow recommended programming flows to avoid timeouts when
+ * gsync is enabled
+ */
+ if (multi_link)
+ intel_shim_sync_arm(sdw);
+
+ ret = sdw_cdns_init(cdns);
+ if (ret < 0) {
+ dev_err(dev, "unable to initialize Cadence IP\n");
+ goto err_interrupt;
+ }
+
ret = sdw_cdns_exit_reset(cdns);
if (ret < 0) {
dev_err(dev, "unable to exit bus reset sequence\n");
goto err_interrupt;
}

+ if (multi_link) {
+ ret = intel_shim_sync_go(sdw);
+ if (ret < 0) {
+ dev_err(dev, "sync go failed: %d\n", ret);
+ goto err_interrupt;
+ }
+ }
+
/* Register DAIs */
ret = intel_register_dai(sdw);
if (ret) {
@@ -1418,7 +1460,6 @@ int intel_master_startup(struct platform_device *pdev)
intel_debugfs_init(sdw);

/* Enable runtime PM */
- link_flags = md_flags >> (bus->link_id * 8);
if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME)) {
pm_runtime_set_autosuspend_delay(dev,
INTEL_MASTER_SUSPEND_DELAY_MS);
@@ -1654,6 +1695,7 @@ static int intel_resume(struct device *dev)
struct sdw_intel *sdw = cdns_to_intel(cdns);
struct sdw_bus *bus = &cdns->bus;
int link_flags;
+ bool multi_link;
int ret;

if (bus->prop.hw_disabled) {
@@ -1662,6 +1704,9 @@ static int intel_resume(struct device *dev)
return 0;
}

+ link_flags = md_flags >> (bus->link_id * 8);
+ multi_link = !(link_flags & SDW_INTEL_MASTER_DISABLE_MULTI_LINK);
+
if (pm_runtime_suspended(dev)) {
dev_dbg(dev, "%s: pm_runtime status was suspended, forcing active\n", __func__);

@@ -1672,6 +1717,7 @@ static int intel_resume(struct device *dev)
pm_runtime_enable(dev);

link_flags = md_flags >> (bus->link_id * 8);
+
if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME_IDLE))
pm_runtime_idle(dev);
}
@@ -1694,12 +1740,33 @@ static int intel_resume(struct device *dev)
return ret;
}

+ /*
+ * follow recommended programming flows to avoid timeouts when
+ * gsync is enabled
+ */
+ if (multi_link)
+ intel_shim_sync_arm(sdw);
+
+ ret = sdw_cdns_init(&sdw->cdns);
+ if (ret < 0) {
+ dev_err(dev, "unable to initialize Cadence IP during resume\n");
+ return ret;
+ }
+
ret = sdw_cdns_exit_reset(cdns);
if (ret < 0) {
dev_err(dev, "unable to exit bus reset sequence during resume\n");
return ret;
}

+ if (multi_link) {
+ ret = intel_shim_sync_go(sdw);
+ if (ret < 0) {
+ dev_err(dev, "sync go failed during resume\n");
+ return ret;
+ }
+ }
+
/*
* after system resume, the pm_runtime suspend() may kick in
* during the enumeration, before any children device force the
@@ -1722,6 +1789,8 @@ static int intel_resume_runtime(struct device *dev)
struct sdw_bus *bus = &cdns->bus;
u32 clock_stop_quirks;
bool clock_stop0;
+ int link_flags;
+ bool multi_link;
int status;
int ret;

@@ -1731,6 +1800,9 @@ static int intel_resume_runtime(struct device *dev)
return 0;
}

+ link_flags = md_flags >> (bus->link_id * 8);
+ multi_link = !(link_flags & SDW_INTEL_MASTER_DISABLE_MULTI_LINK);
+
clock_stop_quirks = sdw->link_res->clock_stop_quirks;

if (clock_stop_quirks & SDW_INTEL_CLK_STOP_TEARDOWN) {
@@ -1752,11 +1824,32 @@ static int intel_resume_runtime(struct device *dev)
return ret;
}

+ /*
+ * follow recommended programming flows to avoid
+ * timeouts when gsync is enabled
+ */
+ if (multi_link)
+ intel_shim_sync_arm(sdw);
+
+ ret = sdw_cdns_init(&sdw->cdns);
+ if (ret < 0) {
+ dev_err(dev, "unable to initialize Cadence IP during resume\n");
+ return ret;
+ }
+
ret = sdw_cdns_exit_reset(cdns);
if (ret < 0) {
dev_err(dev, "unable to exit bus reset sequence during resume\n");
return ret;
}
+
+ if (multi_link) {
+ ret = intel_shim_sync_go(sdw);
+ if (ret < 0) {
+ dev_err(dev, "sync go failed during resume\n");
+ return ret;
+ }
+ }
} else if (clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) {
ret = intel_init(sdw);
if (ret) {
@@ -1773,11 +1866,18 @@ static int intel_resume_runtime(struct device *dev)
*/
clock_stop0 = sdw_cdns_is_clock_stop(&sdw->cdns);

- /*
- * make sure all Slaves are tagged as UNATTACHED and
- * provide reason for reinitialization
- */
if (!clock_stop0) {
+
+ /*
+ * Re-initialize the IP since it was powered-off
+ */
+ sdw_cdns_init(&sdw->cdns);
+
+ /*
+ * make sure all Slaves are tagged as UNATTACHED and
+ * provide reason for reinitialization
+ */
+
status = SDW_UNATTACH_REQUEST_MASTER_RESET;
sdw_clear_slave_status(bus, status);
}
--
2.17.1

2020-08-18 14:37:25

[permalink] [raw]

Subject: [PATCH 08/11] soundwire: stream: enable hw_sync as needed by hardware

From: Pierre-Louis Bossart <[email protected]>

Use platform-specific information to decide when to use hw_sync, not
only a number of links > 1.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/stream.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 37290a799023..e4cf484f5905 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -689,9 +689,9 @@ static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count)

/*
* Set the multi_link flag only when both the hardware supports
- * and there is a stream handled by multiple masters
+ * and hardware-based sync is required
*/
- multi_link = bus->multi_link && (m_rt_count > 1);
+ multi_link = bus->multi_link && (m_rt_count >= bus->hw_sync_min_links);

if (multi_link)
ret = sdw_transfer_defer(bus, wr_msg, &bus->defer_msg);
@@ -760,13 +760,16 @@ static int do_bank_switch(struct sdw_stream_runtime *stream)
const struct sdw_master_ops *ops;
struct sdw_bus *bus;
bool multi_link = false;
+ int m_rt_count;
int ret = 0;

+ m_rt_count = stream->m_rt_count;
+
list_for_each_entry(m_rt, &stream->master_list, stream_node) {
bus = m_rt->bus;
ops = bus->ops;

- if (bus->multi_link) {
+ if (bus->multi_link && m_rt_count >= bus->hw_sync_min_links) {
multi_link = true;
mutex_lock(&bus->msg_lock);
}
@@ -787,7 +790,7 @@ static int do_bank_switch(struct sdw_stream_runtime *stream)
* synchronized across all Masters and happens later as a
* part of post_bank_switch ops.
*/
- ret = sdw_bank_switch(bus, stream->m_rt_count);
+ ret = sdw_bank_switch(bus, m_rt_count);
if (ret < 0) {
dev_err(bus->dev, "Bank switch failed: %d\n", ret);
goto error;
@@ -813,7 +816,7 @@ static int do_bank_switch(struct sdw_stream_runtime *stream)
ret);
goto error;
}
- } else if (bus->multi_link && stream->m_rt_count > 1) {
+ } else if (multi_link) {
dev_err(bus->dev,
"Post bank switch ops not implemented\n");
goto error;
@@ -831,7 +834,7 @@ static int do_bank_switch(struct sdw_stream_runtime *stream)
goto error;
}

- if (bus->multi_link)
+ if (multi_link)
mutex_unlock(&bus->msg_lock);
}

--
2.17.1

2020-08-18 14:37:45

[permalink] [raw]

Subject: [PATCH 09/11] soundwire: intel: add dynamic debug trace for clock-stop invalid configs

From: Pierre-Louis Bossart <[email protected]>

Detect cases where the clock is assumed to be stopped but the IP is
not in the relevant state, and add a dynamic debug trace.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 7c63581270fd..b82d02af3c4f 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1964,6 +1964,11 @@ static int intel_resume_runtime(struct device *dev)
}
}
} else if (!clock_stop_quirks) {
+
+ clock_stop0 = sdw_cdns_is_clock_stop(&sdw->cdns);
+ if (!clock_stop0)
+ dev_err(dev, "%s invalid configuration, clock was not stopped", __func__);
+
ret = intel_init(sdw);
if (ret) {
dev_err(dev, "%s failed: %d", __func__, ret);
--
2.17.1

2020-08-18 14:38:02

[permalink] [raw]

Subject: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for the first cpu_dai

We should call these APIs once per stream. So we can only call it
when the dai ops is invoked for the first cpu dai.

Signed-off-by: Bard Liao <[email protected]>
Reviewed-by: Pierre-Louis Bossart <[email protected]>
Reviewed-by: Ranjani Sridharan <[email protected]>
---
drivers/soundwire/intel.c | 45 +++++++++++++++++++++++++++++++++------
1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 89a8ad1f80e8..7c63581270fd 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -941,11 +941,13 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
static int intel_prepare(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0);
struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
struct sdw_intel *sdw = cdns_to_intel(cdns);
struct sdw_cdns_dma_data *dma;
int ch, dir;
- int ret;
+ int ret = 0;

dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma) {
@@ -985,7 +987,13 @@ static int intel_prepare(struct snd_pcm_substream *substream,
goto err;
}

- ret = sdw_prepare_stream(dma->stream);
+ /*
+ * All cpu dais belong to a stream. To ensure sdw_prepare_stream
+ * is called once per stream, we should call it only when
+ * dai = first_cpu_dai.
+ */
+ if (first_cpu_dai == dai)
+ ret = sdw_prepare_stream(dma->stream);

err:
return ret;
@@ -994,9 +1002,19 @@ static int intel_prepare(struct snd_pcm_substream *substream,
static int intel_trigger(struct snd_pcm_substream *substream, int cmd,
struct snd_soc_dai *dai)
{
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0);
struct sdw_cdns_dma_data *dma;
int ret;

+ /*
+ * All cpu dais belong to a stream. To ensure sdw_enable/disable_stream
+ * are called once per stream, we should call them only when
+ * dai = first_cpu_dai.
+ */
+ if (first_cpu_dai != dai)
+ return 0;
+
dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma) {
dev_err(dai->dev, "failed to get dma data in %s", __func__);
@@ -1031,6 +1049,8 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd,
static int
intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
{
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0);
struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
struct sdw_intel *sdw = cdns_to_intel(cdns);
struct sdw_cdns_dma_data *dma;
@@ -1040,12 +1060,25 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
if (!dma)
return -EIO;

- ret = sdw_deprepare_stream(dma->stream);
- if (ret) {
- dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret);
- return ret;
+ /*
+ * All cpu dais belong to a stream. To ensure sdw_deprepare_stream
+ * is called once per stream, we should call it only when
+ * dai = first_cpu_dai.
+ */
+ if (first_cpu_dai == dai) {
+ ret = sdw_deprepare_stream(dma->stream);
+ if (ret) {
+ dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret);
+ return ret;
+ }
}

+ /*
+ * The sdw stream state will transition to RELEASED when stream->
+ * master_list is empty. So the stream state will transition to
+ * DEPREPARED for the first cpu-dai and to RELEASED for the last
+ * cpu-dai.
+ */
ret = sdw_stream_remove_master(&cdns->bus, dma->stream);
if (ret < 0) {
dev_err(dai->dev, "remove master from stream %s failed: %d\n",
--
2.17.1

2020-08-18 14:38:16

[permalink] [raw]

Subject: [PATCH 10/11] soundwire: intel: pass link_mask information to each master

From: Pierre-Louis Bossart <[email protected]>

While the hardware exposes independent bits to power-up each master,
the recommended sequence is to power all links or none. Idle links can
still use the clock stop mode while the master is powered.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.h | 2 ++
drivers/soundwire/intel_init.c | 1 +
2 files changed, 3 insertions(+)

diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index 23daab9da329..76820d0b9deb 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -18,6 +18,7 @@
* @shim_lock: mutex to handle access to shared SHIM registers
* @shim_mask: global pointer to check SHIM register initialization
* @clock_stop_quirks: mask defining requested behavior on pm_suspend
+ * @link_mask: global mask needed for power-up/down sequences
* @cdns: Cadence master descriptor
* @list: used to walk-through all masters exposed by the same controller
*/
@@ -33,6 +34,7 @@ struct sdw_intel_link_res {
struct mutex *shim_lock; /* protect shared registers */
u32 *shim_mask;
u32 clock_stop_quirks;
+ u32 link_mask;
struct sdw_cdns *cdns;
struct list_head list;
};
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index add46d8fc85c..65d9b9bd2106 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -255,6 +255,7 @@ static struct sdw_intel_ctx
link->clock_stop_quirks = res->clock_stop_quirks;
link->shim_lock = &ctx->shim_lock;
link->shim_mask = &ctx->shim_mask;
+ link->link_mask = link_mask;

memset(&pdevinfo, 0, sizeof(pdevinfo));

--
2.17.1

2020-08-18 14:38:17

[permalink] [raw]

Subject: [PATCH 11/11] soundwire: intel: don't manage link power individually

From: Pierre-Louis Bossart <[email protected]>

Each link has separate power controls, but experimental results show
we need to use an all-or-none approach to the link power management.

This change has marginal power impacts, the DSP needs to be powered
anyways before SoundWire links can be powered, and even when powered a
link can be in clock-stopped mode.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.c | 70 +++++++++++++++++++++++++--------------
1 file changed, 46 insertions(+), 24 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index b82d02af3c4f..154aa7c0561a 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -63,7 +63,9 @@ MODULE_PARM_DESC(sdw_md_flags, "SoundWire Intel Master device flags (0x0 all off
#define SDW_SHIM_WAKESTS 0x192

#define SDW_SHIM_LCTL_SPA BIT(0)
+#define SDW_SHIM_LCTL_SPA_MASK GENMASK(3, 0)
#define SDW_SHIM_LCTL_CPA BIT(8)
+#define SDW_SHIM_LCTL_CPA_MASK GENMASK(11, 8)

#define SDW_SHIM_SYNC_SYNCPRD_VAL_24 (24000 / SDW_CADENCE_GSYNC_KHZ - 1)
#define SDW_SHIM_SYNC_SYNCPRD_VAL_38_4 (38400 / SDW_CADENCE_GSYNC_KHZ - 1)
@@ -295,8 +297,8 @@ static int intel_link_power_up(struct sdw_intel *sdw)
u32 *shim_mask = sdw->link_res->shim_mask;
struct sdw_bus *bus = &sdw->cdns.bus;
struct sdw_master_prop *prop = &bus->prop;
- int spa_mask, cpa_mask;
- int link_control;
+ u32 spa_mask, cpa_mask;
+ u32 link_control;
int ret = 0;
u32 syncprd;
u32 sync_reg;
@@ -319,6 +321,8 @@ static int intel_link_power_up(struct sdw_intel *sdw)
syncprd = SDW_SHIM_SYNC_SYNCPRD_VAL_24;

if (!*shim_mask) {
+ dev_dbg(sdw->cdns.dev, "%s: powering up all links\n", __func__);
+
/* we first need to program the SyncPRD/CPU registers */
dev_dbg(sdw->cdns.dev,
"%s: first link up, programming SYNCPRD\n", __func__);
@@ -331,21 +335,24 @@ static int intel_link_power_up(struct sdw_intel *sdw)
/* Set SyncCPU bit */
sync_reg |= SDW_SHIM_SYNC_SYNCCPU;
intel_writel(shim, SDW_SHIM_SYNC, sync_reg);
- }

- /* Link power up sequence */
- link_control = intel_readl(shim, SDW_SHIM_LCTL);
- spa_mask = (SDW_SHIM_LCTL_SPA << link_id);
- cpa_mask = (SDW_SHIM_LCTL_CPA << link_id);
- link_control |= spa_mask;
+ /* Link power up sequence */
+ link_control = intel_readl(shim, SDW_SHIM_LCTL);

- ret = intel_set_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
- if (ret < 0) {
- dev_err(sdw->cdns.dev, "Failed to power up link: %d\n", ret);
- goto out;
- }
+ /* only power-up enabled links */
+ spa_mask = sdw->link_res->link_mask <<
+ SDW_REG_SHIFT(SDW_SHIM_LCTL_SPA_MASK);
+ cpa_mask = sdw->link_res->link_mask <<
+ SDW_REG_SHIFT(SDW_SHIM_LCTL_CPA_MASK);
+
+ link_control |= spa_mask;
+
+ ret = intel_set_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
+ if (ret < 0) {
+ dev_err(sdw->cdns.dev, "Failed to power up link: %d\n", ret);
+ goto out;
+ }

- if (!*shim_mask) {
/* SyncCPU will change once link is active */
ret = intel_wait_bit(shim, SDW_SHIM_SYNC,
SDW_SHIM_SYNC_SYNCCPU, 0);
@@ -483,7 +490,7 @@ static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)

static int intel_link_power_down(struct sdw_intel *sdw)
{
- int link_control, spa_mask, cpa_mask;
+ u32 link_control, spa_mask, cpa_mask;
unsigned int link_id = sdw->instance;
void __iomem *shim = sdw->link_res->shim;
u32 *shim_mask = sdw->link_res->shim_mask;
@@ -493,24 +500,39 @@ static int intel_link_power_down(struct sdw_intel *sdw)

intel_shim_master_ip_to_glue(sdw);

- /* Link power down sequence */
- link_control = intel_readl(shim, SDW_SHIM_LCTL);
- spa_mask = ~(SDW_SHIM_LCTL_SPA << link_id);
- cpa_mask = (SDW_SHIM_LCTL_CPA << link_id);
- link_control &= spa_mask;
-
- ret = intel_clear_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
-
if (!(*shim_mask & BIT(link_id)))
dev_err(sdw->cdns.dev,
"%s: Unbalanced power-up/down calls\n", __func__);

*shim_mask &= ~BIT(link_id);

+ if (!*shim_mask) {
+
+ dev_dbg(sdw->cdns.dev, "%s: powering down all links\n", __func__);
+
+ /* Link power down sequence */
+ link_control = intel_readl(shim, SDW_SHIM_LCTL);
+
+ /* only power-down enabled links */
+ spa_mask = (~sdw->link_res->link_mask) <<
+ SDW_REG_SHIFT(SDW_SHIM_LCTL_SPA_MASK);
+ cpa_mask = sdw->link_res->link_mask <<
+ SDW_REG_SHIFT(SDW_SHIM_LCTL_CPA_MASK);
+
+ link_control &= spa_mask;
+
+ ret = intel_clear_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
+ }
+
+ link_control = intel_readl(shim, SDW_SHIM_LCTL);
+
mutex_unlock(sdw->link_res->shim_lock);

- if (ret < 0)
+ 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;
--
2.17.1

2020-08-26 09:52:00

[permalink] [raw]

Subject: Re: [PATCH 09/11] soundwire: intel: add dynamic debug trace for clock-stop invalid configs

On 18-08-20, 10:41, Bard Liao wrote:
> From: Pierre-Louis Bossart <[email protected]>
>
> Detect cases where the clock is assumed to be stopped but the IP is
> not in the relevant state, and add a dynamic debug trace.

you meant a debug print..and it looks like error print below (also in title).

>
> Signed-off-by: Pierre-Louis Bossart <[email protected]>
> Signed-off-by: Bard Liao <[email protected]>
> ---
> drivers/soundwire/intel.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 7c63581270fd..b82d02af3c4f 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1964,6 +1964,11 @@ static int intel_resume_runtime(struct device *dev)
> }
> }
> } else if (!clock_stop_quirks) {
> +
> + clock_stop0 = sdw_cdns_is_clock_stop(&sdw->cdns);
> + if (!clock_stop0)
> + dev_err(dev, "%s invalid configuration, clock was not stopped", __func__);
> +
> ret = intel_init(sdw);
> if (ret) {
> dev_err(dev, "%s failed: %d", __func__, ret);
> --
> 2.17.1

--
~Vinod

2020-08-26 14:18:48

[permalink] [raw]

Subject: Re: [PATCH 05/11] soundwire: bus: update multi-link definition with hw sync details

On 18-08-20, 10:41, Bard Liao wrote:
> From: Pierre-Louis Bossart <[email protected]>
>
> Hardware-based synchronization is typically required when the
> bus->multi_link flag is set.
>
> On Intel platforms, when the Cadence IP is configured in 'Multi Master
> Mode', the hardware synchronization is required even when a stream
> only uses a single segment. The existing code only deal with hardware
> synchronization when a stream uses more than one segment so to remain
> backwards compatible we add a configuration threshold. For Intel cases
> this threshold will be set to one, other platforms may be able to use
> the SSP-based sync in those cases.
>
> Signed-off-by: Pierre-Louis Bossart <[email protected]>
> Signed-off-by: Bard Liao <[email protected]>
> ---
> include/linux/soundwire/sdw.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 76052f12c9f7..9adbe4fd7980 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -827,6 +827,11 @@ struct sdw_master_ops {
> * @multi_link: Store bus property that indicates if multi links
> * are supported. This flag is populated by drivers after reading
> * appropriate firmware (ACPI/DT).
> + * @hw_sync_min_links: Number of links used by a stream above which
> + * hardware-based synchronization is required. This value is only
> + * meaningful if multi_link is set. If set to 1, hardware-based
> + * synchronization will be used even if a stream only uses a single
> + * SoundWire segment.

Soundwire spec does not say anything about multi-link so this is left to
implementer. Assuming that value of 1 would mean hw based sync will
be used even for single stream does not make sense in generic terms.
Maybe yes for Intel but may not be true for everyone?

We already use m_rt_count in code for this, so the question is why is
that not sufficient?

> */
> struct sdw_bus {
> struct device *dev;
> @@ -850,6 +855,7 @@ struct sdw_bus {
> unsigned int clk_stop_timeout;
> u32 bank_switch_timeout;
> bool multi_link;
> + int hw_sync_min_links;
> };
>
> int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
> --
> 2.17.1

--
~Vinod

2020-08-26 14:18:50

[permalink] [raw]

Subject: Re: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for the first cpu_dai

On 18-08-20, 10:41, Bard Liao wrote:
> We should call these APIs once per stream. So we can only call it
> when the dai ops is invoked for the first cpu dai.
>
> Signed-off-by: Bard Liao <[email protected]>
> Reviewed-by: Pierre-Louis Bossart <[email protected]>
> Reviewed-by: Ranjani Sridharan <[email protected]>
> ---
> drivers/soundwire/intel.c | 45 +++++++++++++++++++++++++++++++++------
> 1 file changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 89a8ad1f80e8..7c63581270fd 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -941,11 +941,13 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
> static int intel_prepare(struct snd_pcm_substream *substream,
> struct snd_soc_dai *dai)
> {
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
> struct sdw_intel *sdw = cdns_to_intel(cdns);
> struct sdw_cdns_dma_data *dma;
> int ch, dir;
> - int ret;
> + int ret = 0;
>
> dma = snd_soc_dai_get_dma_data(dai, substream);
> if (!dma) {
> @@ -985,7 +987,13 @@ static int intel_prepare(struct snd_pcm_substream *substream,
> goto err;
> }
>
> - ret = sdw_prepare_stream(dma->stream);
> + /*
> + * All cpu dais belong to a stream. To ensure sdw_prepare_stream
> + * is called once per stream, we should call it only when
> + * dai = first_cpu_dai.
> + */
> + if (first_cpu_dai == dai)
> + ret = sdw_prepare_stream(dma->stream);

Hmmm why not use the one place which is unique in the card to call this,
hint machine dais are only called once.

>
> err:
> return ret;
> @@ -994,9 +1002,19 @@ static int intel_prepare(struct snd_pcm_substream *substream,
> static int intel_trigger(struct snd_pcm_substream *substream, int cmd,
> struct snd_soc_dai *dai)
> {
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> struct sdw_cdns_dma_data *dma;
> int ret;
>
> + /*
> + * All cpu dais belong to a stream. To ensure sdw_enable/disable_stream
> + * are called once per stream, we should call them only when
> + * dai = first_cpu_dai.
> + */
> + if (first_cpu_dai != dai)
> + return 0;
> +
> dma = snd_soc_dai_get_dma_data(dai, substream);
> if (!dma) {
> dev_err(dai->dev, "failed to get dma data in %s", __func__);
> @@ -1031,6 +1049,8 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd,
> static int
> intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> {
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
> struct sdw_intel *sdw = cdns_to_intel(cdns);
> struct sdw_cdns_dma_data *dma;
> @@ -1040,12 +1060,25 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> if (!dma)
> return -EIO;
>
> - ret = sdw_deprepare_stream(dma->stream);
> - if (ret) {
> - dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret);
> - return ret;
> + /*
> + * All cpu dais belong to a stream. To ensure sdw_deprepare_stream
> + * is called once per stream, we should call it only when
> + * dai = first_cpu_dai.
> + */
> + if (first_cpu_dai == dai) {
> + ret = sdw_deprepare_stream(dma->stream);
> + if (ret) {
> + dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret);
> + return ret;
> + }
> }
>
> + /*
> + * The sdw stream state will transition to RELEASED when stream->
> + * master_list is empty. So the stream state will transition to
> + * DEPREPARED for the first cpu-dai and to RELEASED for the last
> + * cpu-dai.
> + */
> ret = sdw_stream_remove_master(&cdns->bus, dma->stream);
> if (ret < 0) {
> dev_err(dai->dev, "remove master from stream %s failed: %d\n",
> --
> 2.17.1

--
~Vinod

2020-08-26 14:41:25

[permalink] [raw]

Subject: Re: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for the first cpu_dai

>> - ret = sdw_prepare_stream(dma->stream);
>> + /*
>> + * All cpu dais belong to a stream. To ensure sdw_prepare_stream
>> + * is called once per stream, we should call it only when
>> + * dai = first_cpu_dai.
>> + */
>> + if (first_cpu_dai == dai)
>> + ret = sdw_prepare_stream(dma->stream);
>
> Hmmm why not use the one place which is unique in the card to call this,
> hint machine dais are only called once.

we are already calling directly sdw_startup_stream() and
sdw_shutdown_stream() from the machine driver.

We could call sdw_stream_enable() in the dailink .trigger as well, since
it only calls the stream API.

However for both .prepare() and .hw_free() there are a set of dai-level
configurations using static functions defined only in intel.c, and I
don't think we can move the code to the machine driver, or split the
prepare/hw_free in two (dailink and dai operations).

I am not against your idea, I am not sure if it can be done.

Would you be ok to merge this as a first step and let us work on an
optimization later (which would require ASoC/SoundWire synchronization)?

2020-08-26 14:41:25

[permalink] [raw]

Subject: Re: [PATCH 09/11] soundwire: intel: add dynamic debug trace for clock-stop invalid configs

On 8/26/20 4:48 AM, Vinod Koul wrote:
> On 18-08-20, 10:41, Bard Liao wrote:
>> From: Pierre-Louis Bossart <[email protected]>
>>
>> Detect cases where the clock is assumed to be stopped but the IP is
>> not in the relevant state, and add a dynamic debug trace.
>
> you meant a debug print..and it looks like error print below (also in title).

I don't understand the comment. Is the 'trace' confusing and are you
asking to e.g. change the commit message to 'add dynamic debug log'?

>
>>
>> Signed-off-by: Pierre-Louis Bossart <[email protected]>
>> Signed-off-by: Bard Liao <[email protected]>
>> ---
>> drivers/soundwire/intel.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
>> index 7c63581270fd..b82d02af3c4f 100644
>> --- a/drivers/soundwire/intel.c
>> +++ b/drivers/soundwire/intel.c
>> @@ -1964,6 +1964,11 @@ static int intel_resume_runtime(struct device *dev)
>> }
>> }
>> } else if (!clock_stop_quirks) {
>> +
>> + clock_stop0 = sdw_cdns_is_clock_stop(&sdw->cdns);
>> + if (!clock_stop0)
>> + dev_err(dev, "%s invalid configuration, clock was not stopped", __func__);
>> +
>> ret = intel_init(sdw);
>> if (ret) {
>> dev_err(dev, "%s failed: %d", __func__, ret);
>> --
>> 2.17.1
>

2020-08-26 14:43:23

[permalink] [raw]

Subject: Re: [PATCH 05/11] soundwire: bus: update multi-link definition with hw sync details

>> + * @hw_sync_min_links: Number of links used by a stream above which
>> + * hardware-based synchronization is required. This value is only
>> + * meaningful if multi_link is set. If set to 1, hardware-based
>> + * synchronization will be used even if a stream only uses a single
>> + * SoundWire segment.
>
> Soundwire spec does not say anything about multi-link so this is left to
> implementer. Assuming that value of 1 would mean hw based sync will
> be used even for single stream does not make sense in generic terms.
> Maybe yes for Intel but may not be true for everyone?

hw-based sync is required for Intel even for single stream. It's been
part of the recommended programming flows since the beginning but
ignored so far.

That said, this value is set by each master implementation, no one
forces non-Intel users to implement an Intel-specific requirement.

> We already use m_rt_count in code for this, so the question is why is
> that not sufficient?

Because as you rightly said above, Intel requires the hw_sync to be used
even for single stream, but we didn't want others to be forced to use
the hw-sync for single stream. the m_rt_count is not sufficient for Intel.

I think we are in agreement on not forcing everyone to follow what is
required by Intel, and that's precisely why we added this setting. If
you set it to two you would only use hw_sync when two masters are used.

2020-08-28 01:50:28

by Liao, Bard

[permalink] [raw]

Subject: RE: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for the first cpu_dai

> -----Original Message-----
> From: Vinod Koul <[email protected]>
> Sent: Wednesday, August 26, 2020 5:47 PM
> To: Bard Liao <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; pierre-
> [email protected]; Kale, Sanyog R <[email protected]>; Lin,
> Mengdong <[email protected]>; Liao, Bard <[email protected]>
> Subject: Re: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for
> the first cpu_dai
>
> On 18-08-20, 10:41, Bard Liao wrote:
> > We should call these APIs once per stream. So we can only call it when
> > the dai ops is invoked for the first cpu dai.
> >
> > Signed-off-by: Bard Liao <[email protected]>
> > Reviewed-by: Pierre-Louis Bossart
> > <[email protected]>
> > Reviewed-by: Ranjani Sridharan <[email protected]>
> > ---
> > drivers/soundwire/intel.c | 45
> > +++++++++++++++++++++++++++++++++------
> > 1 file changed, 39 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> > index 89a8ad1f80e8..7c63581270fd 100644
> > --- a/drivers/soundwire/intel.c
> > +++ b/drivers/soundwire/intel.c
> > @@ -941,11 +941,13 @@ static int intel_hw_params(struct
> > snd_pcm_substream *substream, static int intel_prepare(struct
> snd_pcm_substream *substream,
> > struct snd_soc_dai *dai)
> > {
> > + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> > struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
> > struct sdw_intel *sdw = cdns_to_intel(cdns);
> > struct sdw_cdns_dma_data *dma;
> > int ch, dir;
> > - int ret;
> > + int ret = 0;
> >
> > dma = snd_soc_dai_get_dma_data(dai, substream);
> > if (!dma) {
> > @@ -985,7 +987,13 @@ static int intel_prepare(struct
> snd_pcm_substream *substream,
> > goto err;
> > }
> >
> > - ret = sdw_prepare_stream(dma->stream);
> > + /*
> > + * All cpu dais belong to a stream. To ensure sdw_prepare_stream
> > + * is called once per stream, we should call it only when
> > + * dai = first_cpu_dai.
> > + */
> > + if (first_cpu_dai == dai)
> > + ret = sdw_prepare_stream(dma->stream);
>
> Hmmm why not use the one place which is unique in the card to call this,
> hint machine dais are only called once.

Yes, we can call it in machine driver. But, shouldn't it belong to platform
level? The point is that if we move the stuff to machine driver, it will
force people to implement these stuff on their own Intel machine driver.

>
> ~Vinod

2020-08-28 07:31:13

[permalink] [raw]

Subject: Re: [PATCH 05/11] soundwire: bus: update multi-link definition with hw sync details

On 26-08-20, 09:09, Pierre-Louis Bossart wrote:
>
>
> > > + * @hw_sync_min_links: Number of links used by a stream above which
> > > + * hardware-based synchronization is required. This value is only
> > > + * meaningful if multi_link is set. If set to 1, hardware-based
> > > + * synchronization will be used even if a stream only uses a single
> > > + * SoundWire segment.
> >
> > Soundwire spec does not say anything about multi-link so this is left to
> > implementer. Assuming that value of 1 would mean hw based sync will
> > be used even for single stream does not make sense in generic terms.
> > Maybe yes for Intel but may not be true for everyone?
>
> hw-based sync is required for Intel even for single stream. It's been part
> of the recommended programming flows since the beginning but ignored so far.
>
> That said, this value is set by each master implementation, no one forces
> non-Intel users to implement an Intel-specific requirement.
>
> > We already use m_rt_count in code for this, so the question is why is
> > that not sufficient?
>
> Because as you rightly said above, Intel requires the hw_sync to be used
> even for single stream, but we didn't want others to be forced to use the
> hw-sync for single stream. the m_rt_count is not sufficient for Intel.
>
> I think we are in agreement on not forcing everyone to follow what is
> required by Intel, and that's precisely why we added this setting. If you
> set it to two you would only use hw_sync when two masters are used.

Okay, it would be better if we move it to intel driver, but I see it may
not be trivial, so lets go with this approach.

--
~Vinod

2020-08-28 07:49:17

[permalink] [raw]

Subject: Re: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for the first cpu_dai

On 28-08-20, 01:47, Liao, Bard wrote:
> > snd_pcm_substream *substream,
> > > goto err;
> > > }
> > >
> > > - ret = sdw_prepare_stream(dma->stream);
> > > + /*
> > > + * All cpu dais belong to a stream. To ensure sdw_prepare_stream
> > > + * is called once per stream, we should call it only when
> > > + * dai = first_cpu_dai.
> > > + */
> > > + if (first_cpu_dai == dai)
> > > + ret = sdw_prepare_stream(dma->stream);
> >
> > Hmmm why not use the one place which is unique in the card to call this,
> > hint machine dais are only called once.
>
> Yes, we can call it in machine driver. But, shouldn't it belong to platform
> level? The point is that if we move the stuff to machine driver, it will
> force people to implement these stuff on their own Intel machine driver.

nothing stops anyone from doing that right! machine driver is another
component so it can be moved there as well

--
~Vinod

2020-08-28 07:50:22

[permalink] [raw]

Subject: Re: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for the first cpu_dai

On 26-08-20, 09:35, Pierre-Louis Bossart wrote:
>
> > > - ret = sdw_prepare_stream(dma->stream);
> > > + /*
> > > + * All cpu dais belong to a stream. To ensure sdw_prepare_stream
> > > + * is called once per stream, we should call it only when
> > > + * dai = first_cpu_dai.
> > > + */
> > > + if (first_cpu_dai == dai)
> > > + ret = sdw_prepare_stream(dma->stream);
> >
> > Hmmm why not use the one place which is unique in the card to call this,
> > hint machine dais are only called once.
>
> we are already calling directly sdw_startup_stream() and
> sdw_shutdown_stream() from the machine driver.
>
> We could call sdw_stream_enable() in the dailink .trigger as well, since it
> only calls the stream API.

Correct :)

> However for both .prepare() and .hw_free() there are a set of dai-level
> configurations using static functions defined only in intel.c, and I don't
> think we can move the code to the machine driver, or split the
> prepare/hw_free in two (dailink and dai operations).

Cant they be exported and continue to call those apis

> I am not against your idea, I am not sure if it can be done.
>
> Would you be ok to merge this as a first step and let us work on an
> optimization later (which would require ASoC/SoundWire synchronization)?

The problem is that we add one flag then another and it does become an
issue eventually, better to do the right thing now than later.

--
~Vinod

2020-08-28 07:51:24

[permalink] [raw]

Subject: Re: [PATCH 09/11] soundwire: intel: add dynamic debug trace for clock-stop invalid configs

On 26-08-20, 09:38, Pierre-Louis Bossart wrote:
>
>
> On 8/26/20 4:48 AM, Vinod Koul wrote:
> > On 18-08-20, 10:41, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <[email protected]>
> > >
> > > Detect cases where the clock is assumed to be stopped but the IP is
> > > not in the relevant state, and add a dynamic debug trace.
> >
> > you meant a debug print..and it looks like error print below (also in title).
>
> I don't understand the comment. Is the 'trace' confusing and are you asking
> to e.g. change the commit message to 'add dynamic debug log'?

Question is what is dynamic about this?

--
~Vinod

2020-08-28 15:11:05

[permalink] [raw]

Subject: Re: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for the first cpu_dai

On 8/28/20 2:45 AM, Vinod Koul wrote:
> On 28-08-20, 01:47, Liao, Bard wrote:
>>> snd_pcm_substream *substream,
>>>> goto err;
>>>> }
>>>>
>>>> - ret = sdw_prepare_stream(dma->stream);
>>>> + /*
>>>> + * All cpu dais belong to a stream. To ensure sdw_prepare_stream
>>>> + * is called once per stream, we should call it only when
>>>> + * dai = first_cpu_dai.
>>>> + */
>>>> + if (first_cpu_dai == dai)
>>>> + ret = sdw_prepare_stream(dma->stream);
>>>
>>> Hmmm why not use the one place which is unique in the card to call this,
>>> hint machine dais are only called once.
>>
>> Yes, we can call it in machine driver. But, shouldn't it belong to platform
>> level? The point is that if we move the stuff to machine driver, it will
>> force people to implement these stuff on their own Intel machine driver.
>
> nothing stops anyone from doing that right! machine driver is another
> component so it can be moved there as well

What Bard is saying is that there is nothing board-specific here. This
is platform-driver code that is independent of the actual machine
configuration.

Machine drivers can be board-specific, so we would have to add the code
for prepare/deprepare/trigger to every machine driver.

Today it's true that we worked to have a single machine driver for all
SoundWire-based devices, so the change is a 1:1 move, but we can't
guarantee that this would be the case moving forward. In fact, we *know*
we will need a different machine driver when we parse platform firmware
to create the card for SDCA support. So in the end there would be
duplication of code.

See the code we worked on at
https://github.com/thesofproject/linux/commit/7322e1d25ce2ec9bb78c6e861919f61e0be7cc0b.patch

it'd really a bit silly to have this generic code in the machine driver.

it would be fine to call a set of helpers called from the machine driver
dailink, but where would we put these helpers? on the ASoC or SoundWire
sides?

2020-08-28 15:19:43

[permalink] [raw]

Subject: Re: [PATCH 09/11] soundwire: intel: add dynamic debug trace for clock-stop invalid configs

>>>> Detect cases where the clock is assumed to be stopped but the IP is
>>>> not in the relevant state, and add a dynamic debug trace.
>>>
>>> you meant a debug print..and it looks like error print below (also in title).
>>
>> I don't understand the comment. Is the 'trace' confusing and are you asking
>> to e.g. change the commit message to 'add dynamic debug log'?
>
> Question is what is dynamic about this?
dev_dbg() is part of the kernel dynamic debug capability...

https://www.kernel.org/doc/html/latest/admin-guide/dynamic-debug-howto.html

Not sure what you are asking here?

2020-08-29 11:02:15

[permalink] [raw]

Subject: Re: [PATCH 09/11] soundwire: intel: add dynamic debug trace for clock-stop invalid configs

On 28-08-20, 09:54, Pierre-Louis Bossart wrote:
>
> > > > > Detect cases where the clock is assumed to be stopped but the IP is
> > > > > not in the relevant state, and add a dynamic debug trace.
> > > >
> > > > you meant a debug print..and it looks like error print below (also in title).
> > >
> > > I don't understand the comment. Is the 'trace' confusing and are you asking
> > > to e.g. change the commit message to 'add dynamic debug log'?
> >
> > Question is what is dynamic about this?
> dev_dbg() is part of the kernel dynamic debug capability...
>
> https://www.kernel.org/doc/html/latest/admin-guide/dynamic-debug-howto.html
>
> Not sure what you are asking here?

:-| where is dev_dbg() ?

See [1]

On 18-08-20, 10:41, Bard Liao wrote:
> From: Pierre-Louis Bossart <[email protected]>
>
> Detect cases where the clock is assumed to be stopped but the IP is
> not in the relevant state, and add a dynamic debug trace.
>
> Signed-off-by: Pierre-Louis Bossart <[email protected]>
> Signed-off-by: Bard Liao <[email protected]>
> ---
> drivers/soundwire/intel.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 7c63581270fd..b82d02af3c4f 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1964,6 +1964,11 @@ static int intel_resume_runtime(struct device *dev)
> }
> }
> } else if (!clock_stop_quirks) {
> +
> + clock_stop0 = sdw_cdns_is_clock_stop(&sdw->cdns);
> + if (!clock_stop0)

[1]

> + dev_err(dev, "%s invalid configuration, clock was not stopped", __func__);

^^^^^^^

> +
> ret = intel_init(sdw);
> if (ret) {
> dev_err(dev, "%s failed: %d", __func__, ret);
> --
> 2.17.1

--
~Vinod

2020-08-31 15:18:47

[permalink] [raw]

Subject: Re: [PATCH 09/11] soundwire: intel: add dynamic debug trace for clock-stop invalid configs

>>>>>> Detect cases where the clock is assumed to be stopped but the IP is
>>>>>> not in the relevant state, and add a dynamic debug trace.
>>>>>
>>>>> you meant a debug print..and it looks like error print below (also in title).
>>>>
>>>> I don't understand the comment. Is the 'trace' confusing and are you asking
>>>> to e.g. change the commit message to 'add dynamic debug log'?
>>>
>>> Question is what is dynamic about this?
>> dev_dbg() is part of the kernel dynamic debug capability...
>>
>> https://www.kernel.org/doc/html/latest/admin-guide/dynamic-debug-howto.html
>>
>> Not sure what you are asking here?
>
> :-| where is dev_dbg() ?
>
> See [1]

>
> [1]
>
>> + dev_err(dev, "%s invalid configuration, clock was not stopped", __func__);
>
> ^^^^^^^

it's still a log using the "dynamic debug" framework.

Again, what are you asking us to fix?

2020-09-01 11:42:13

[permalink] [raw]

Subject: Re: [PATCH 09/11] soundwire: intel: add dynamic debug trace for clock-stop invalid configs

On 31-08-20, 10:15, Pierre-Louis Bossart wrote:
>
> > > > > > > Detect cases where the clock is assumed to be stopped but the IP is
> > > > > > > not in the relevant state, and add a dynamic debug trace.
> > > > > >
> > > > > > you meant a debug print..and it looks like error print below (also in title).
> > > > >
> > > > > I don't understand the comment. Is the 'trace' confusing and are you asking
> > > > > to e.g. change the commit message to 'add dynamic debug log'?
> > > >
> > > > Question is what is dynamic about this?
> > > dev_dbg() is part of the kernel dynamic debug capability...
> > >
> > > https://www.kernel.org/doc/html/latest/admin-guide/dynamic-debug-howto.html
> > >
> > > Not sure what you are asking here?
> >
> > :-| where is dev_dbg() ?
> >
> > See [1]
>
> >
> > [1]
> >
> > > + dev_err(dev, "%s invalid configuration, clock was not stopped", __func__);
> >
> > ^^^^^^^
>
> it's still a log using the "dynamic debug" framework.
>
> Again, what are you asking us to fix?

Ah you are really testing my patience!

The title says "dynamic debug" and then you use a dev_err which is *not*
part of dynamic debug as it is printed always and cannot be dynamically
enabled and disabled!

See Documentation/admin-guide/dynamic-debug-howto.rst:

"Dynamic debug is designed to allow you to dynamically enable/disable
kernel code to obtain additional kernel information. Currently, if
``CONFIG_DYNAMIC_DEBUG`` is set, then all ``pr_debug()``/``dev_dbg()`` and
``print_hex_dump_debug()``/``print_hex_dump_bytes()`` calls can be dynamically
enabled per-callsite."

No dev_err here!

--
~Vinod

2020-09-01 13:49:08