[PATCH v2 0/6] soundwire: Add multi link support

[PATCH v2 0/6] soundwire: Add multi link support

[Shreyas NC]

Currently, in the SoundWire subsystem, the concept of stream is
limited to a Master and one or more Slaves(Codecs). This series
aims to add support for multiple Master(s) sharing the same
reference clock and synchronized in the hardware.

This patch series adds:
 - Helpers to lock bus instances part of the stream
 - Boiler plate conversion of code to support a list of
   Master runtime
 - Support multi link bank switch to support synchronization
   between multiple masters
 - Add Intel platform ops for pre/post bank switch

Changes in v2:
 - Add the example in Documentation as pointed out by Vinod
 - As Pierre suggested, added more description to the
   sdw_acquire_bus_lock() and sdw_release_bus_lock() API and
   sdw_stream_runtime data structure.

Sanyog Kale (2):
  Documentation: soundwire: Add documentation for Multi link
  soundwire: Add support to lock across bus instances

Shreyas NC (3):
  soundwire: Initialize completion for defer messages
  soundwire: Add support for multilink bank switch
  soundwire: intel: Add pre/post bank switch ops

Vinod Koul (1):
  soundwire: Handle multiple master instances in a stream

 Documentation/driver-api/soundwire/stream.rst |  29 +-
 drivers/soundwire/bus.c                       |   1 +
 drivers/soundwire/bus.h                       |   4 +
 drivers/soundwire/intel.c                     |  54 ++++
 drivers/soundwire/stream.c                    | 441 ++++++++++++++++++--------
 include/linux/soundwire/sdw.h                 |   8 +-
 6 files changed, 407 insertions(+), 130 deletions(-)

-- 
2.7.4

[PATCH v2 1/6] Documentation: soundwire: Add documentation for Multi link

[Shreyas NC]

From: Sanyog Kale <sanyog.r.kale@intel.com>

Add example and documentation to describe Multi Link streams

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 Documentation/driver-api/soundwire/stream.rst | 29 ++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/Documentation/driver-api/soundwire/stream.rst b/Documentation/driver-api/soundwire/stream.rst
index 29121aa..11623aab 100644
--- a/Documentation/driver-api/soundwire/stream.rst
+++ b/Documentation/driver-api/soundwire/stream.rst
@@ -75,7 +75,6 @@ Slaves are using single port. ::
 	                                                   |     (Data)    |
 	                                                   +---------------+
 
-
 Example 4: Stereo Stream with L and R channel is rendered by two different
 Ports of the Master and is received by only single Port of the Slave
 interface. ::
@@ -101,6 +100,34 @@ interface. ::
 	+--------------------+                             |                |
 							   +----------------+
 
+Example 5: Stereo Stream with L and R channel is rendered by 2 Masters, each
+rendering one channel, and is received by two different Slaves, each
+receiving one channel. Both Masters and both Slaves are using single port. ::
+
+	+---------------+                    Clock Signal  +---------------+
+	|    Master     +----------------------------------+     Slave     |
+	|   Interface   |                                  |   Interface   |
+	|       1       |                                  |       1       |
+	|               |                     Data Signal  |               |
+	|       L       +----------------------------------+       L       |
+	|     (Data)    |     Data Direction               |     (Data)    |
+	+---------------+  +----------------------->       +---------------+
+
+	+---------------+                    Clock Signal  +---------------+
+	|    Master     +----------------------------------+     Slave     |
+	|   Interface   |                                  |   Interface   |
+	|       2       |                                  |       2       |
+	|               |                     Data Signal  |               |
+	|       R       +----------------------------------+       R       |
+	|     (Data)    |     Data Direction               |     (Data)    |
+	+---------------+  +----------------------->       +---------------+
+
+Note: In multi-link cases like above, to lock, one would acquire a global
+lock and then go on locking bus instances. But, in this case the caller
+framework(ASoC DPCM) guarantees that stream operations on a card are
+always serialized. So, there is no race condition and hence no need for
+global lock.
+
 SoundWire Stream Management flow
 ================================
 
-- 
2.7.4

[PATCH v2 2/6] soundwire: Initialize completion for defer messages

[Shreyas NC]

Deferred messages are async messages used to synchronize
transitions mostly while doing a bank switch on multi links.
On successful transitions these messages are marked complete
and thereby confirming that all the buses performed bank switch
successfully.

So, initialize the completion structure for the same.

Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 drivers/soundwire/bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index dcc0ff9..dbabd5e 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -175,6 +175,7 @@ static inline int do_transfer_defer(struct sdw_bus *bus,
 
 	defer->msg = msg;
 	defer->length = msg->len;
+	init_completion(&defer->complete);
 
 	for (i = 0; i <= retry; i++) {
 		resp = bus->ops->xfer_msg_defer(bus, msg, defer);
-- 
2.7.4

[PATCH v2 3/6] soundwire: Add support to lock across bus instances

[Shreyas NC]

From: Sanyog Kale <sanyog.r.kale@intel.com>

Currently, the stream concept is limited to a Master and one or
more Codecs.

This patch extends the concept to support multiple Master(s)
sharing the same reference clock and synchronized in the hardware.
Modify sdw_stream_runtime to support a list of sdw_master_runtime
for the same. The existing reference to a single m_rt is removed
in the next patch.

Typically to lock, one would acquire a global lock and then lock
bus instances. In this case, the caller framework(ASoC DPCM)
guarantees that stream operations on a card are always serialized.
So, there is no race condition and hence no need for global lock.

Bus lock(s) are acquired to reconfigure the bus while the stream
is set-up.
So, we add sdw_acquire_bus_lock()/sdw_release_bus_lock() which
are used only to reconfigure the bus.

Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 drivers/soundwire/bus.h       |  2 ++
 drivers/soundwire/stream.c    | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/soundwire/sdw.h |  4 ++++
 3 files changed, 47 insertions(+)

diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 3b15c4e..b6cfbdf 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -99,6 +99,7 @@ struct sdw_slave_runtime {
  * this stream, can be zero.
  * @slave_rt_list: Slave runtime list
  * @port_list: List of Master Ports configured for this stream, can be zero.
+ * @stream_node: sdw_stream_runtime master_list node
  * @bus_node: sdw_bus m_rt_list node
  */
 struct sdw_master_runtime {
@@ -108,6 +109,7 @@ struct sdw_master_runtime {
 	unsigned int ch_count;
 	struct list_head slave_rt_list;
 	struct list_head port_list;
+	struct list_head stream_node;
 	struct list_head bus_node;
 };
 
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 8974a0f..eb942c6 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -747,6 +747,7 @@ struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name)
 		return NULL;
 
 	stream->name = stream_name;
+	INIT_LIST_HEAD(&stream->master_list);
 	stream->state = SDW_STREAM_ALLOCATED;
 
 	return stream;
@@ -1234,6 +1235,46 @@ struct sdw_dpn_prop *sdw_get_slave_dpn_prop(struct sdw_slave *slave,
 	return NULL;
 }
 
+/**
+ * sdw_acquire_bus_lock: Acquire bus lock for all Master runtime(s)
+ *
+ * @stream: SoundWire stream
+ *
+ * Acquire bus_lock for each of the master runtime(m_rt) part of this
+ * stream to reconfigure the bus.
+ */
+static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream)
+{
+	struct sdw_master_runtime *m_rt = NULL;
+	struct sdw_bus *bus = NULL;
+
+	/* Iterate for all Master(s) in Master list */
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+
+		mutex_lock(&bus->bus_lock);
+	}
+}
+
+/**
+ * sdw_release_bus_lock: Release bus lock for all Master runtime(s)
+ *
+ * @stream: SoundWire stream
+ *
+ * Release the previously held bus_lock after reconfiguring the bus.
+ */
+static void sdw_release_bus_lock(struct sdw_stream_runtime *stream)
+{
+	struct sdw_master_runtime *m_rt = NULL;
+	struct sdw_bus *bus = NULL;
+
+	/* Iterate for all Master(s) in Master list */
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		mutex_unlock(&bus->bus_lock);
+	}
+}
+
 static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
 {
 	struct sdw_master_runtime *m_rt = stream->m_rt;
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 962971e..ccd8dcdf 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -769,6 +769,9 @@ struct sdw_stream_params {
  * @state: Current state of the stream
  * @type: Stream type PCM or PDM
  * @m_rt: Master runtime
+ * @master_list: List of Master runtime(s) in this stream.
+ * master_list can contain only one m_rt per Master instance
+ * for a stream
  */
 struct sdw_stream_runtime {
 	char *name;
@@ -776,6 +779,7 @@ struct sdw_stream_runtime {
 	enum sdw_stream_state state;
 	enum sdw_stream_type type;
 	struct sdw_master_runtime *m_rt;
+	struct list_head master_list;
 };
 
 struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name);
-- 
2.7.4

[PATCH v2 4/6] soundwire: Handle multiple master instances in a stream

[Shreyas NC]

From: Vinod Koul <vkoul@kernel.org>

For each SoundWire stream operation, we need to parse master
list and operate upon all master runtime.

This patch does the boilerplate conversion of stream handling
from single master runtime to handle a list of master runtime.

Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 drivers/soundwire/stream.c    | 308 +++++++++++++++++++++++++-----------------
 include/linux/soundwire/sdw.h |   2 -
 2 files changed, 185 insertions(+), 125 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index eb942c6..36506a7 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -681,35 +681,44 @@ static int sdw_bank_switch(struct sdw_bus *bus)
 
 static int do_bank_switch(struct sdw_stream_runtime *stream)
 {
-	struct sdw_master_runtime *m_rt = stream->m_rt;
+	struct sdw_master_runtime *m_rt = NULL;
 	const struct sdw_master_ops *ops;
-	struct sdw_bus *bus = m_rt->bus;
+	struct sdw_bus *bus = NULL;
 	int ret = 0;
 
-	ops = bus->ops;
 
-	/* Pre-bank switch */
-	if (ops->pre_bank_switch) {
-		ret = ops->pre_bank_switch(bus);
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		ops = bus->ops;
+
+		/* Pre-bank switch */
+		if (ops->pre_bank_switch) {
+			ret = ops->pre_bank_switch(bus);
+			if (ret < 0) {
+				dev_err(bus->dev,
+					"Pre bank switch op failed: %d", ret);
+				return ret;
+			}
+		}
+
+		/* Bank switch */
+		ret = sdw_bank_switch(bus);
 		if (ret < 0) {
-			dev_err(bus->dev, "Pre bank switch op failed: %d", ret);
+			dev_err(bus->dev, "Bank switch failed: %d", ret);
 			return ret;
 		}
 	}
 
-	/* Bank switch */
-	ret = sdw_bank_switch(bus);
-	if (ret < 0) {
-		dev_err(bus->dev, "Bank switch failed: %d", ret);
-		return ret;
-	}
-
 	/* Post-bank switch */
-	if (ops->post_bank_switch) {
-		ret = ops->post_bank_switch(bus);
-		if (ret < 0) {
-			dev_err(bus->dev,
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		ops = bus->ops;
+		if (ops->post_bank_switch) {
+			ret = ops->post_bank_switch(bus);
+			if (ret < 0) {
+				dev_err(bus->dev,
 					"Post bank switch op failed: %d", ret);
+			}
 		}
 	}
 
@@ -754,6 +763,21 @@ struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name)
 }
 EXPORT_SYMBOL(sdw_alloc_stream);
 
+static struct sdw_master_runtime
+*sdw_find_master_rt(struct sdw_bus *bus,
+			struct sdw_stream_runtime *stream)
+{
+	struct sdw_master_runtime *m_rt = NULL;
+
+	/* Retrieve Bus handle if already available */
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		if (m_rt->bus == bus)
+			return m_rt;
+	}
+
+	return NULL;
+}
+
 /**
  * sdw_alloc_master_rt() - Allocates and initialize Master runtime handle
  *
@@ -770,12 +794,11 @@ static struct sdw_master_runtime
 {
 	struct sdw_master_runtime *m_rt;
 
-	m_rt = stream->m_rt;
-
 	/*
 	 * check if Master is already allocated (as a result of Slave adding
 	 * it first), if so skip allocation and go to configure
 	 */
+	m_rt = sdw_find_master_rt(bus, stream);
 	if (m_rt)
 		goto stream_config;
 
@@ -786,7 +809,7 @@ static struct sdw_master_runtime
 	/* Initialization of Master runtime handle */
 	INIT_LIST_HEAD(&m_rt->port_list);
 	INIT_LIST_HEAD(&m_rt->slave_rt_list);
-	stream->m_rt = m_rt;
+	list_add_tail(&m_rt->stream_node, &stream->master_list);
 
 	list_add_tail(&m_rt->bus_node, &bus->m_rt_list);
 
@@ -844,17 +867,21 @@ static void sdw_slave_port_release(struct sdw_bus *bus,
 			struct sdw_stream_runtime *stream)
 {
 	struct sdw_port_runtime *p_rt, *_p_rt;
-	struct sdw_master_runtime *m_rt = stream->m_rt;
+	struct sdw_master_runtime *m_rt;
 	struct sdw_slave_runtime *s_rt;
 
-	list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
-		if (s_rt->slave != slave)
-			continue;
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
 
-		list_for_each_entry_safe(p_rt, _p_rt,
-				&s_rt->port_list, port_node) {
-			list_del(&p_rt->port_node);
-			kfree(p_rt);
+			if (s_rt->slave != slave)
+				continue;
+
+			list_for_each_entry_safe(p_rt, _p_rt,
+					&s_rt->port_list, port_node) {
+
+				list_del(&p_rt->port_node);
+				kfree(p_rt);
+			}
 		}
 	}
 }
@@ -871,16 +898,18 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
 			struct sdw_stream_runtime *stream)
 {
 	struct sdw_slave_runtime *s_rt, *_s_rt;
-	struct sdw_master_runtime *m_rt = stream->m_rt;
-
-	/* Retrieve Slave runtime handle */
-	list_for_each_entry_safe(s_rt, _s_rt,
-			&m_rt->slave_rt_list, m_rt_node) {
+	struct sdw_master_runtime *m_rt;
 
-		if (s_rt->slave == slave) {
-			list_del(&s_rt->m_rt_node);
-			kfree(s_rt);
-			return;
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		/* Retrieve Slave runtime handle */
+		list_for_each_entry_safe(s_rt, _s_rt,
+					&m_rt->slave_rt_list, m_rt_node) {
+
+			if (s_rt->slave == slave) {
+				list_del(&s_rt->m_rt_node);
+				kfree(s_rt);
+				return;
+			}
 		}
 	}
 }
@@ -888,6 +917,7 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
 /**
  * sdw_release_master_stream() - Free Master runtime handle
  *
+ * @m_rt: Master runtime node
  * @stream: Stream runtime handle.
  *
  * This function is to be called with bus_lock held
@@ -895,16 +925,18 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
  * handle. If this is called first then sdw_release_slave_stream() will have
  * no effect as Slave(s) runtime handle would already be freed up.
  */
-static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
+static void sdw_release_master_stream(struct sdw_master_runtime *m_rt,
+			struct sdw_stream_runtime *stream)
 {
-	struct sdw_master_runtime *m_rt = stream->m_rt;
 	struct sdw_slave_runtime *s_rt, *_s_rt;
 
 	list_for_each_entry_safe(s_rt, _s_rt,
 			&m_rt->slave_rt_list, m_rt_node)
 		sdw_stream_remove_slave(s_rt->slave, stream);
 
+	list_del(&m_rt->stream_node);
 	list_del(&m_rt->bus_node);
+	kfree(m_rt);
 }
 
 /**
@@ -918,13 +950,22 @@ static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
 int sdw_stream_remove_master(struct sdw_bus *bus,
 		struct sdw_stream_runtime *stream)
 {
+	struct sdw_master_runtime *m_rt, *_m_rt;
+
 	mutex_lock(&bus->bus_lock);
 
-	sdw_release_master_stream(stream);
-	sdw_master_port_release(bus, stream->m_rt);
-	stream->state = SDW_STREAM_RELEASED;
-	kfree(stream->m_rt);
-	stream->m_rt = NULL;
+	list_for_each_entry_safe(m_rt, _m_rt,
+			&stream->master_list, stream_node) {
+
+		if (m_rt->bus != bus)
+			continue;
+
+		sdw_master_port_release(bus, m_rt);
+		sdw_release_master_stream(m_rt, stream);
+	}
+
+	if (list_empty(&stream->master_list))
+		stream->state = SDW_STREAM_RELEASED;
 
 	mutex_unlock(&bus->bus_lock);
 
@@ -1127,7 +1168,7 @@ int sdw_stream_add_master(struct sdw_bus *bus,
 	stream->state = SDW_STREAM_CONFIGURED;
 
 stream_error:
-	sdw_release_master_stream(stream);
+	sdw_release_master_stream(m_rt, stream);
 error:
 	mutex_unlock(&bus->bus_lock);
 	return ret;
@@ -1195,7 +1236,7 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
 	 * we hit error so cleanup the stream, release all Slave(s) and
 	 * Master runtime
 	 */
-	sdw_release_master_stream(stream);
+	sdw_release_master_stream(m_rt, stream);
 error:
 	mutex_unlock(&slave->bus->bus_lock);
 	return ret;
@@ -1277,31 +1318,36 @@ static void sdw_release_bus_lock(struct sdw_stream_runtime *stream)
 
 static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
 {
-	struct sdw_master_runtime *m_rt = stream->m_rt;
-	struct sdw_bus *bus = m_rt->bus;
+	struct sdw_master_runtime *m_rt = NULL;
+	struct sdw_bus *bus = NULL;
 	struct sdw_master_prop *prop = NULL;
 	struct sdw_bus_params params;
 	int ret;
 
-	prop = &bus->prop;
-	memcpy(&params, &bus->params, sizeof(params));
+	/* Prepare  Master(s) and Slave(s) port(s) associated with stream */
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		prop = &bus->prop;
+		memcpy(&params, &bus->params, sizeof(params));
 
-	/* TODO: Support Asynchronous mode */
-	if ((prop->max_freq % stream->params.rate) != 0) {
-		dev_err(bus->dev, "Async mode not supported");
-		return -EINVAL;
-	}
+		/* TODO: Support Asynchronous mode */
+		if ((prop->max_freq % stream->params.rate) != 0) {
+			dev_err(bus->dev, "Async mode not supported");
+			return -EINVAL;
+		}
 
-	/* Increment cumulative bus bandwidth */
-	/* TODO: Update this during Device-Device support */
-	bus->params.bandwidth += m_rt->stream->params.rate *
-		m_rt->ch_count * m_rt->stream->params.bps;
+		/* Increment cumulative bus bandwidth */
+		/* TODO: Update this during Device-Device support */
+		bus->params.bandwidth += m_rt->stream->params.rate *
+			m_rt->ch_count * m_rt->stream->params.bps;
+
+		/* Program params */
+		ret = sdw_program_params(bus);
+		if (ret < 0) {
+			dev_err(bus->dev, "Program params failed: %d", ret);
+			goto restore_params;
+		}
 
-	/* Program params */
-	ret = sdw_program_params(bus);
-	if (ret < 0) {
-		dev_err(bus->dev, "Program params failed: %d", ret);
-		goto restore_params;
 	}
 
 	ret = do_bank_switch(stream);
@@ -1310,12 +1356,16 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
 		goto restore_params;
 	}
 
-	/* Prepare port(s) on the new clock configuration */
-	ret = sdw_prep_deprep_ports(m_rt, true);
-	if (ret < 0) {
-		dev_err(bus->dev, "Prepare port(s) failed ret = %d",
-				ret);
-		return ret;
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+
+		/* Prepare port(s) on the new clock configuration */
+		ret = sdw_prep_deprep_ports(m_rt, true);
+		if (ret < 0) {
+			dev_err(bus->dev, "Prepare port(s) failed ret = %d",
+					ret);
+			return ret;
+		}
 	}
 
 	stream->state = SDW_STREAM_PREPARED;
@@ -1343,35 +1393,40 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
 		return -EINVAL;
 	}
 
-	mutex_lock(&stream->m_rt->bus->bus_lock);
+	sdw_acquire_bus_lock(stream);
 
 	ret = _sdw_prepare_stream(stream);
 	if (ret < 0)
 		pr_err("Prepare for stream:%s failed: %d", stream->name, ret);
 
-	mutex_unlock(&stream->m_rt->bus->bus_lock);
+	sdw_release_bus_lock(stream);
 	return ret;
 }
 EXPORT_SYMBOL(sdw_prepare_stream);
 
 static int _sdw_enable_stream(struct sdw_stream_runtime *stream)
 {
-	struct sdw_master_runtime *m_rt = stream->m_rt;
-	struct sdw_bus *bus = m_rt->bus;
+	struct sdw_master_runtime *m_rt = NULL;
+	struct sdw_bus *bus = NULL;
 	int ret;
 
-	/* Program params */
-	ret = sdw_program_params(bus);
-	if (ret < 0) {
-		dev_err(bus->dev, "Program params failed: %d", ret);
-		return ret;
-	}
+	/* Enable Master(s) and Slave(s) port(s) associated with stream */
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
 
-	/* Enable port(s) */
-	ret = sdw_enable_disable_ports(m_rt, true);
-	if (ret < 0) {
-		dev_err(bus->dev, "Enable port(s) failed ret: %d", ret);
-		return ret;
+		/* Program params */
+		ret = sdw_program_params(bus);
+		if (ret < 0) {
+			dev_err(bus->dev, "Program params failed: %d", ret);
+			return ret;
+		}
+
+		/* Enable port(s) */
+		ret = sdw_enable_disable_ports(m_rt, true);
+		if (ret < 0) {
+			dev_err(bus->dev, "Enable port(s) failed ret: %d", ret);
+			return ret;
+		}
 	}
 
 	ret = do_bank_switch(stream);
@@ -1400,37 +1455,42 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
 		return -EINVAL;
 	}
 
-	mutex_lock(&stream->m_rt->bus->bus_lock);
+	sdw_acquire_bus_lock(stream);
 
 	ret = _sdw_enable_stream(stream);
 	if (ret < 0)
 		pr_err("Enable for stream:%s failed: %d", stream->name, ret);
 
-	mutex_unlock(&stream->m_rt->bus->bus_lock);
+	sdw_release_bus_lock(stream);
 	return ret;
 }
 EXPORT_SYMBOL(sdw_enable_stream);
 
 static int _sdw_disable_stream(struct sdw_stream_runtime *stream)
 {
-	struct sdw_master_runtime *m_rt = stream->m_rt;
-	struct sdw_bus *bus = m_rt->bus;
+	struct sdw_master_runtime *m_rt = NULL;
+	struct sdw_bus *bus = NULL;
 	int ret;
 
-	/* Disable port(s) */
-	ret = sdw_enable_disable_ports(m_rt, false);
-	if (ret < 0) {
-		dev_err(bus->dev, "Disable port(s) failed: %d", ret);
-		return ret;
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		/* Disable port(s) */
+		ret = sdw_enable_disable_ports(m_rt, false);
+		if (ret < 0) {
+			dev_err(bus->dev, "Disable port(s) failed: %d", ret);
+			return ret;
+		}
 	}
-
 	stream->state = SDW_STREAM_DISABLED;
 
-	/* Program params */
-	ret = sdw_program_params(bus);
-	if (ret < 0) {
-		dev_err(bus->dev, "Program params failed: %d", ret);
-		return ret;
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		/* Program params */
+		ret = sdw_program_params(bus);
+		if (ret < 0) {
+			dev_err(bus->dev, "Program params failed: %d", ret);
+			return ret;
+		}
 	}
 
 	return do_bank_switch(stream);
@@ -1452,43 +1512,46 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
 		return -EINVAL;
 	}
 
-	mutex_lock(&stream->m_rt->bus->bus_lock);
+	sdw_acquire_bus_lock(stream);
 
 	ret = _sdw_disable_stream(stream);
 	if (ret < 0)
 		pr_err("Disable for stream:%s failed: %d", stream->name, ret);
 
-	mutex_unlock(&stream->m_rt->bus->bus_lock);
+	sdw_release_bus_lock(stream);
 	return ret;
 }
 EXPORT_SYMBOL(sdw_disable_stream);
 
 static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream)
 {
-	struct sdw_master_runtime *m_rt = stream->m_rt;
-	struct sdw_bus *bus = m_rt->bus;
+	struct sdw_master_runtime *m_rt = NULL;
+	struct sdw_bus *bus = NULL;
 	int ret = 0;
 
-	/* De-prepare port(s) */
-	ret = sdw_prep_deprep_ports(m_rt, false);
-	if (ret < 0) {
-		dev_err(bus->dev, "De-prepare port(s) failed: %d", ret);
-		return ret;
-	}
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		/* De-prepare port(s) */
+		ret = sdw_prep_deprep_ports(m_rt, false);
+		if (ret < 0) {
+			dev_err(bus->dev, "De-prepare port(s) failed: %d", ret);
+			return ret;
+		}
 
-	stream->state = SDW_STREAM_DEPREPARED;
+		/* TODO: Update this during Device-Device support */
+		bus->params.bandwidth -= m_rt->stream->params.rate *
+			m_rt->ch_count * m_rt->stream->params.bps;
 
-	/* TODO: Update this during Device-Device support */
-	bus->params.bandwidth -= m_rt->stream->params.rate *
-		m_rt->ch_count * m_rt->stream->params.bps;
+		/* Program params */
+		ret = sdw_program_params(bus);
+		if (ret < 0) {
+			dev_err(bus->dev, "Program params failed: %d", ret);
+			return ret;
+		}
 
-	/* Program params */
-	ret = sdw_program_params(bus);
-	if (ret < 0) {
-		dev_err(bus->dev, "Program params failed: %d", ret);
-		return ret;
 	}
 
+	stream->state = SDW_STREAM_DEPREPARED;
 	return do_bank_switch(stream);
 }
 
@@ -1508,13 +1571,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
 		return -EINVAL;
 	}
 
-	mutex_lock(&stream->m_rt->bus->bus_lock);
-
+	sdw_acquire_bus_lock(stream);
 	ret = _sdw_deprepare_stream(stream);
 	if (ret < 0)
 		pr_err("De-prepare for stream:%d failed: %d", ret, ret);
 
-	mutex_unlock(&stream->m_rt->bus->bus_lock);
+	sdw_release_bus_lock(stream);
 	return ret;
 }
 EXPORT_SYMBOL(sdw_deprepare_stream);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index ccd8dcdf..03df709 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -768,7 +768,6 @@ struct sdw_stream_params {
  * @params: Stream parameters
  * @state: Current state of the stream
  * @type: Stream type PCM or PDM
- * @m_rt: Master runtime
  * @master_list: List of Master runtime(s) in this stream.
  * master_list can contain only one m_rt per Master instance
  * for a stream
@@ -778,7 +777,6 @@ struct sdw_stream_runtime {
 	struct sdw_stream_params params;
 	enum sdw_stream_state state;
 	enum sdw_stream_type type;
-	struct sdw_master_runtime *m_rt;
 	struct list_head master_list;
 };
 
-- 
2.7.4

[PATCH v2 5/6] soundwire: Add support for multilink bank switch

[Shreyas NC]

In cases of multiple master in a stream, synchronization
between multiple master(s) is achieved by performing bank switch
together and using master methods.

Add sdw_ml_bank_switch() to wait for completion of bank switch.

Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 drivers/soundwire/bus.h       |   2 +
 drivers/soundwire/stream.c    | 102 ++++++++++++++++++++++++++++++++++++++----
 include/linux/soundwire/sdw.h |   2 +
 3 files changed, 97 insertions(+), 9 deletions(-)

diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index b6cfbdf..c77de05 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -4,6 +4,8 @@
 #ifndef __SDW_BUS_H
 #define __SDW_BUS_H
 
+#define DEFAULT_BANK_SWITCH_TIMEOUT 3000
+
 #if IS_ENABLED(CONFIG_ACPI)
 int sdw_acpi_find_slaves(struct sdw_bus *bus);
 #else
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 36506a7..103840c 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -638,6 +638,8 @@ static int sdw_bank_switch(struct sdw_bus *bus)
 	if (!wr_msg)
 		return -ENOMEM;
 
+	bus->defer_msg.msg = wr_msg;
+
 	wbuf = kzalloc(sizeof(*wbuf), GFP_KERNEL);
 	if (!wbuf) {
 		ret = -ENOMEM;
@@ -658,17 +660,23 @@ static int sdw_bank_switch(struct sdw_bus *bus)
 					SDW_MSG_FLAG_WRITE, wbuf);
 	wr_msg->ssp_sync = true;
 
-	ret = sdw_transfer(bus, wr_msg);
+	if (bus->multi_link)
+		ret = sdw_transfer_defer(bus, wr_msg, &bus->defer_msg);
+	else
+		ret = sdw_transfer(bus, wr_msg);
+
 	if (ret < 0) {
 		dev_err(bus->dev, "Slave frame_ctrl reg write failed");
 		goto error;
 	}
 
-	kfree(wr_msg);
-	kfree(wbuf);
-	bus->defer_msg.msg = NULL;
-	bus->params.curr_bank = !bus->params.curr_bank;
-	bus->params.next_bank = !bus->params.next_bank;
+	if (!bus->multi_link) {
+		kfree(wr_msg);
+		kfree(wbuf);
+		bus->defer_msg.msg = NULL;
+		bus->params.curr_bank = !bus->params.curr_bank;
+		bus->params.next_bank = !bus->params.next_bank;
+	}
 
 	return 0;
 
@@ -679,25 +687,65 @@ static int sdw_bank_switch(struct sdw_bus *bus)
 	return ret;
 }
 
+/**
+ * sdw_ml_sync_bank_switch: Multilink register bank switch
+ *
+ * @bus: SDW bus instance
+ *
+ * Caller function should free the buffers on error
+ */
+static int sdw_ml_sync_bank_switch(struct sdw_bus *bus)
+{
+	unsigned long time_left;
+	int ret = 0;
+
+	if (!bus->multi_link)
+		return ret;
+
+	/* Wait for completion of transfer */
+	time_left = wait_for_completion_timeout(&bus->defer_msg.complete,
+						bus->bank_switch_timeout);
+
+	if (!time_left) {
+		dev_err(bus->dev, "Controller Timed out on bank switch");
+		return -ETIMEDOUT;
+	}
+
+	bus->params.curr_bank = !bus->params.curr_bank;
+	bus->params.next_bank = !bus->params.next_bank;
+
+	if (bus->defer_msg.msg) {
+		kfree(bus->defer_msg.msg->buf);
+		kfree(bus->defer_msg.msg);
+	}
+
+	return ret;
+}
+
 static int do_bank_switch(struct sdw_stream_runtime *stream)
 {
 	struct sdw_master_runtime *m_rt = NULL;
 	const struct sdw_master_ops *ops;
 	struct sdw_bus *bus = NULL;
+	bool multi_link = false;
 	int ret = 0;
 
-
 	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
 		bus = m_rt->bus;
 		ops = bus->ops;
 
+		if (bus->multi_link) {
+			multi_link = true;
+			mutex_lock(&bus->msg_lock);
+		}
+
 		/* Pre-bank switch */
 		if (ops->pre_bank_switch) {
 			ret = ops->pre_bank_switch(bus);
 			if (ret < 0) {
 				dev_err(bus->dev,
 					"Pre bank switch op failed: %d", ret);
-				return ret;
+				goto msg_unlock;
 			}
 		}
 
@@ -705,7 +753,8 @@ static int do_bank_switch(struct sdw_stream_runtime *stream)
 		ret = sdw_bank_switch(bus);
 		if (ret < 0) {
 			dev_err(bus->dev, "Bank switch failed: %d", ret);
-			return ret;
+			goto error;
+
 		}
 	}
 
@@ -718,8 +767,43 @@ static int do_bank_switch(struct sdw_stream_runtime *stream)
 			if (ret < 0) {
 				dev_err(bus->dev,
 					"Post bank switch op failed: %d", ret);
+				goto error;
 			}
 		}
+
+		/* Set the bank switch timeout to default, if not set */
+		if (!bus->bank_switch_timeout)
+			bus->bank_switch_timeout = DEFAULT_BANK_SWITCH_TIMEOUT;
+
+		ret = sdw_ml_sync_bank_switch(bus);
+		if (ret < 0) {
+			dev_err(bus->dev,
+					"Post bank switch op failed: %d", ret);
+			goto error;
+		}
+
+		mutex_unlock(&bus->msg_lock);
+	}
+
+	return ret;
+
+error:
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+
+		bus = m_rt->bus;
+
+		kfree(bus->defer_msg.msg->buf);
+		kfree(bus->defer_msg.msg);
+	}
+
+msg_unlock:
+
+	if (multi_link) {
+		list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+			bus = m_rt->bus;
+			if (mutex_is_locked(&bus->msg_lock))
+				mutex_unlock(&bus->msg_lock);
+		}
 	}
 
 	return ret;
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 03df709..f006029 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -673,6 +673,7 @@ struct sdw_master_ops {
  * @params: Current bus parameters
  * @prop: Master properties
  * @m_rt_list: List of Master instance of all stream(s) running on Bus. This
+ * @multi_link: if multi links are supported
  * is used to compute and program bus bandwidth, clock, frame shape,
  * transport and port parameters
  * @defer_msg: Defer message
@@ -691,6 +692,7 @@ struct sdw_bus {
 	struct sdw_bus_params params;
 	struct sdw_master_prop prop;
 	struct list_head m_rt_list;
+	bool multi_link;
 	struct sdw_defer defer_msg;
 	unsigned int clk_stop_timeout;
 	u32 bank_switch_timeout;
-- 
2.7.4

[PATCH v2 6/6] soundwire: intel: Add pre/post bank switch ops

[Shreyas NC]

To support multi link on Intel platforms, we need to update
SDW SHIM registers.

So, add pre/post bank switch ops for the same in Intel driver.

Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 drivers/soundwire/intel.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 0a8990e..524110f 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -398,6 +398,58 @@ static int intel_config_stream(struct sdw_intel *sdw,
 }
 
 /*
+ * bank switch routines
+ */
+
+static int intel_pre_bank_switch(struct sdw_bus *bus)
+{
+	struct sdw_cdns *cdns = bus_to_cdns(bus);
+	struct sdw_intel *sdw = cdns_to_intel(cdns);
+	void __iomem *shim = sdw->res->shim;
+	int sync_reg;
+
+	/* Write to register only for multi-link */
+	if (!bus->multi_link)
+		return 0;
+
+	/* Read SYNC register */
+	sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
+	sync_reg |= SDW_SHIM_SYNC_CMDSYNC << sdw->instance;
+	intel_writel(shim, SDW_SHIM_SYNC, sync_reg);
+
+	return 0;
+}
+
+static int intel_post_bank_switch(struct sdw_bus *bus)
+{
+	struct sdw_cdns *cdns = bus_to_cdns(bus);
+	struct sdw_intel *sdw = cdns_to_intel(cdns);
+	void __iomem *shim = sdw->res->shim;
+	int sync_reg, ret;
+
+	/* Write to register only for multi-link */
+	if (!bus->multi_link)
+		return 0;
+
+	/* Read SYNC register */
+	sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
+
+	/* Check CMDSYNC bit set for any Master */
+	if (!(sync_reg & SDW_SHIM_SYNC_CMDSYNC_MASK))
+		return 0;
+
+	/* Set SyncGO bit */
+	sync_reg |= SDW_SHIM_SYNC_SYNCGO;
+
+	ret = intel_clear_bit(shim, SDW_SHIM_SYNC, sync_reg,
+					SDW_SHIM_SYNC_SYNCGO);
+	if (ret < 0)
+		dev_err(sdw->cdns.dev, "Post bank switch failed: %d", ret);
+
+	return ret;
+}
+
+/*
  * DAI routines
  */
 
@@ -750,6 +802,8 @@ static struct sdw_master_ops sdw_intel_ops = {
 	.xfer_msg_defer = cdns_xfer_msg_defer,
 	.reset_page_addr = cdns_reset_page_addr,
 	.set_bus_conf = cdns_bus_conf,
+	.pre_bank_switch = intel_pre_bank_switch,
+	.post_bank_switch = intel_post_bank_switch,
 };
 
 /*
-- 
2.7.4

Re: [PATCH v2 4/6] soundwire: Handle multiple master instances in a stream

[Pierre-Louis Bossart]

On 06/12/2018 05:53 AM, Shreyas NC wrote:
> From: Vinod Koul <vkoul@kernel.org>
>
> For each SoundWire stream operation, we need to parse master
> list and operate upon all master runtime.
>
> This patch does the boilerplate conversion of stream handling
> from single master runtime to handle a list of master runtime.
>
> Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
> ---
>   drivers/soundwire/stream.c    | 308 +++++++++++++++++++++++++-----------------
>   include/linux/soundwire/sdw.h |   2 -
>   2 files changed, 185 insertions(+), 125 deletions(-)
>
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index eb942c6..36506a7 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -681,35 +681,44 @@ static int sdw_bank_switch(struct sdw_bus *bus)
>   
>   static int do_bank_switch(struct sdw_stream_runtime *stream)
>   {
> -	struct sdw_master_runtime *m_rt = stream->m_rt;
> +	struct sdw_master_runtime *m_rt = NULL;
>   	const struct sdw_master_ops *ops;
> -	struct sdw_bus *bus = m_rt->bus;
> +	struct sdw_bus *bus = NULL;
>   	int ret = 0;
>   
> -	ops = bus->ops;
>   
> -	/* Pre-bank switch */
> -	if (ops->pre_bank_switch) {
> -		ret = ops->pre_bank_switch(bus);
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		bus = m_rt->bus;
> +		ops = bus->ops;
> +
> +		/* Pre-bank switch */
> +		if (ops->pre_bank_switch) {
> +			ret = ops->pre_bank_switch(bus);
> +			if (ret < 0) {
> +				dev_err(bus->dev,
> +					"Pre bank switch op failed: %d", ret);
> +				return ret;
> +			}
> +		}
> +
> +		/* Bank switch */
> +		ret = sdw_bank_switch(bus);
>   		if (ret < 0) {
> -			dev_err(bus->dev, "Pre bank switch op failed: %d", ret);
> +			dev_err(bus->dev, "Bank switch failed: %d", ret);
>   			return ret;
>   		}
You probably want to add a comment that in multi-link operation the 
actual bank_switch happens later and is done in a synchronized manner. 
This is lost if you just move code around and move the bank_switch into 
a loop.

I am actually no longer clear just looking at this code on when the 
bank_switch happens (i know we've discussed this before but I am trying 
to find out just based on this v2 instead of projecting how I think it 
should work):
In Patch 6/6 it's pretty obvious that the bank switch happens when the 
SyncGO bit is set, but there is no comment or explanation on how we 
reach the intel_post_bank_switch() routine once for all masters handling 
a stream when everything is based on loops. Clearly the 
intel_pre_bank_switch is called multiple times (once per master), I 
guess I am missing what the trigger is for the intel_post_bank_switch() 
routine to be invoked?

>   	}
>   
> -	/* Bank switch */
> -	ret = sdw_bank_switch(bus);
> -	if (ret < 0) {
> -		dev_err(bus->dev, "Bank switch failed: %d", ret);
> -		return ret;
> -	}
> -
>   	/* Post-bank switch */
> -	if (ops->post_bank_switch) {
> -		ret = ops->post_bank_switch(bus);
> -		if (ret < 0) {
> -			dev_err(bus->dev,
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		bus = m_rt->bus;
> +		ops = bus->ops;
> +		if (ops->post_bank_switch) {
> +			ret = ops->post_bank_switch(bus);
> +			if (ret < 0) {
> +				dev_err(bus->dev,
>   					"Post bank switch op failed: %d", ret);
> +			}
>   		}
>   	}
>   
> @@ -754,6 +763,21 @@ struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name)
>   }
>   EXPORT_SYMBOL(sdw_alloc_stream);
>   
> +static struct sdw_master_runtime
> +*sdw_find_master_rt(struct sdw_bus *bus,
> +			struct sdw_stream_runtime *stream)
> +{
> +	struct sdw_master_runtime *m_rt = NULL;
> +
> +	/* Retrieve Bus handle if already available */
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		if (m_rt->bus == bus)
> +			return m_rt;
> +	}
> +
> +	return NULL;
> +}
> +
>   /**
>    * sdw_alloc_master_rt() - Allocates and initialize Master runtime handle
>    *
> @@ -770,12 +794,11 @@ static struct sdw_master_runtime
>   {
>   	struct sdw_master_runtime *m_rt;
>   
> -	m_rt = stream->m_rt;
> -
>   	/*
>   	 * check if Master is already allocated (as a result of Slave adding
>   	 * it first), if so skip allocation and go to configure
>   	 */
> +	m_rt = sdw_find_master_rt(bus, stream);
>   	if (m_rt)
>   		goto stream_config;
>   
> @@ -786,7 +809,7 @@ static struct sdw_master_runtime
>   	/* Initialization of Master runtime handle */
>   	INIT_LIST_HEAD(&m_rt->port_list);
>   	INIT_LIST_HEAD(&m_rt->slave_rt_list);
> -	stream->m_rt = m_rt;
> +	list_add_tail(&m_rt->stream_node, &stream->master_list);
>   
>   	list_add_tail(&m_rt->bus_node, &bus->m_rt_list);
>   
> @@ -844,17 +867,21 @@ static void sdw_slave_port_release(struct sdw_bus *bus,
>   			struct sdw_stream_runtime *stream)
>   {
>   	struct sdw_port_runtime *p_rt, *_p_rt;
> -	struct sdw_master_runtime *m_rt = stream->m_rt;
> +	struct sdw_master_runtime *m_rt;
>   	struct sdw_slave_runtime *s_rt;
>   
> -	list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> -		if (s_rt->slave != slave)
> -			continue;
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
>   
> -		list_for_each_entry_safe(p_rt, _p_rt,
> -				&s_rt->port_list, port_node) {
> -			list_del(&p_rt->port_node);
> -			kfree(p_rt);
> +			if (s_rt->slave != slave)
> +				continue;
> +
> +			list_for_each_entry_safe(p_rt, _p_rt,
> +					&s_rt->port_list, port_node) {
> +
> +				list_del(&p_rt->port_node);
> +				kfree(p_rt);
> +			}
>   		}
>   	}
>   }
> @@ -871,16 +898,18 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
>   			struct sdw_stream_runtime *stream)
>   {
>   	struct sdw_slave_runtime *s_rt, *_s_rt;
> -	struct sdw_master_runtime *m_rt = stream->m_rt;
> -
> -	/* Retrieve Slave runtime handle */
> -	list_for_each_entry_safe(s_rt, _s_rt,
> -			&m_rt->slave_rt_list, m_rt_node) {
> +	struct sdw_master_runtime *m_rt;
>   
> -		if (s_rt->slave == slave) {
> -			list_del(&s_rt->m_rt_node);
> -			kfree(s_rt);
> -			return;
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		/* Retrieve Slave runtime handle */
> +		list_for_each_entry_safe(s_rt, _s_rt,
> +					&m_rt->slave_rt_list, m_rt_node) {
> +
> +			if (s_rt->slave == slave) {
> +				list_del(&s_rt->m_rt_node);
> +				kfree(s_rt);
> +				return;
> +			}
>   		}
>   	}
>   }
> @@ -888,6 +917,7 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
>   /**
>    * sdw_release_master_stream() - Free Master runtime handle
>    *
> + * @m_rt: Master runtime node
>    * @stream: Stream runtime handle.
>    *
>    * This function is to be called with bus_lock held
> @@ -895,16 +925,18 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
>    * handle. If this is called first then sdw_release_slave_stream() will have
>    * no effect as Slave(s) runtime handle would already be freed up.
>    */
> -static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
> +static void sdw_release_master_stream(struct sdw_master_runtime *m_rt,
> +			struct sdw_stream_runtime *stream)
>   {
> -	struct sdw_master_runtime *m_rt = stream->m_rt;
>   	struct sdw_slave_runtime *s_rt, *_s_rt;
>   
>   	list_for_each_entry_safe(s_rt, _s_rt,
>   			&m_rt->slave_rt_list, m_rt_node)
>   		sdw_stream_remove_slave(s_rt->slave, stream);
>   
> +	list_del(&m_rt->stream_node);
>   	list_del(&m_rt->bus_node);
> +	kfree(m_rt);
>   }
>   
>   /**
> @@ -918,13 +950,22 @@ static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
>   int sdw_stream_remove_master(struct sdw_bus *bus,
>   		struct sdw_stream_runtime *stream)
>   {
> +	struct sdw_master_runtime *m_rt, *_m_rt;
> +
>   	mutex_lock(&bus->bus_lock);
>   
> -	sdw_release_master_stream(stream);
> -	sdw_master_port_release(bus, stream->m_rt);
> -	stream->state = SDW_STREAM_RELEASED;
> -	kfree(stream->m_rt);
> -	stream->m_rt = NULL;
> +	list_for_each_entry_safe(m_rt, _m_rt,
> +			&stream->master_list, stream_node) {
> +
> +		if (m_rt->bus != bus)
> +			continue;
> +
> +		sdw_master_port_release(bus, m_rt);
> +		sdw_release_master_stream(m_rt, stream);
> +	}
> +
> +	if (list_empty(&stream->master_list))
> +		stream->state = SDW_STREAM_RELEASED;
>   
>   	mutex_unlock(&bus->bus_lock);
>   
> @@ -1127,7 +1168,7 @@ int sdw_stream_add_master(struct sdw_bus *bus,
>   	stream->state = SDW_STREAM_CONFIGURED;
>   
>   stream_error:
> -	sdw_release_master_stream(stream);
> +	sdw_release_master_stream(m_rt, stream);
>   error:
>   	mutex_unlock(&bus->bus_lock);
>   	return ret;
> @@ -1195,7 +1236,7 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
>   	 * we hit error so cleanup the stream, release all Slave(s) and
>   	 * Master runtime
>   	 */
> -	sdw_release_master_stream(stream);
> +	sdw_release_master_stream(m_rt, stream);
>   error:
>   	mutex_unlock(&slave->bus->bus_lock);
>   	return ret;
> @@ -1277,31 +1318,36 @@ static void sdw_release_bus_lock(struct sdw_stream_runtime *stream)
>   
>   static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
>   {
> -	struct sdw_master_runtime *m_rt = stream->m_rt;
> -	struct sdw_bus *bus = m_rt->bus;
> +	struct sdw_master_runtime *m_rt = NULL;
> +	struct sdw_bus *bus = NULL;
>   	struct sdw_master_prop *prop = NULL;
>   	struct sdw_bus_params params;
>   	int ret;
>   
> -	prop = &bus->prop;
> -	memcpy(&params, &bus->params, sizeof(params));
> +	/* Prepare  Master(s) and Slave(s) port(s) associated with stream */
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		bus = m_rt->bus;
> +		prop = &bus->prop;
> +		memcpy(&params, &bus->params, sizeof(params));
>   
> -	/* TODO: Support Asynchronous mode */
> -	if ((prop->max_freq % stream->params.rate) != 0) {
> -		dev_err(bus->dev, "Async mode not supported");
> -		return -EINVAL;
> -	}
> +		/* TODO: Support Asynchronous mode */
> +		if ((prop->max_freq % stream->params.rate) != 0) {
> +			dev_err(bus->dev, "Async mode not supported");
> +			return -EINVAL;
> +		}
>   
> -	/* Increment cumulative bus bandwidth */
> -	/* TODO: Update this during Device-Device support */
> -	bus->params.bandwidth += m_rt->stream->params.rate *
> -		m_rt->ch_count * m_rt->stream->params.bps;
> +		/* Increment cumulative bus bandwidth */
> +		/* TODO: Update this during Device-Device support */
> +		bus->params.bandwidth += m_rt->stream->params.rate *
> +			m_rt->ch_count * m_rt->stream->params.bps;
> +
> +		/* Program params */
> +		ret = sdw_program_params(bus);
> +		if (ret < 0) {
> +			dev_err(bus->dev, "Program params failed: %d", ret);
> +			goto restore_params;
> +		}
>   
> -	/* Program params */
> -	ret = sdw_program_params(bus);
> -	if (ret < 0) {
> -		dev_err(bus->dev, "Program params failed: %d", ret);
> -		goto restore_params;
>   	}
>   
>   	ret = do_bank_switch(stream);
> @@ -1310,12 +1356,16 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
>   		goto restore_params;
>   	}
>   
> -	/* Prepare port(s) on the new clock configuration */
> -	ret = sdw_prep_deprep_ports(m_rt, true);
> -	if (ret < 0) {
> -		dev_err(bus->dev, "Prepare port(s) failed ret = %d",
> -				ret);
> -		return ret;
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		bus = m_rt->bus;
> +
> +		/* Prepare port(s) on the new clock configuration */
> +		ret = sdw_prep_deprep_ports(m_rt, true);
> +		if (ret < 0) {
> +			dev_err(bus->dev, "Prepare port(s) failed ret = %d",
> +					ret);
> +			return ret;
> +		}
>   	}
>   
>   	stream->state = SDW_STREAM_PREPARED;
> @@ -1343,35 +1393,40 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
>   		return -EINVAL;
>   	}
>   
> -	mutex_lock(&stream->m_rt->bus->bus_lock);
> +	sdw_acquire_bus_lock(stream);
>   
>   	ret = _sdw_prepare_stream(stream);
>   	if (ret < 0)
>   		pr_err("Prepare for stream:%s failed: %d", stream->name, ret);
>   
> -	mutex_unlock(&stream->m_rt->bus->bus_lock);
> +	sdw_release_bus_lock(stream);
>   	return ret;
>   }
>   EXPORT_SYMBOL(sdw_prepare_stream);
>   
>   static int _sdw_enable_stream(struct sdw_stream_runtime *stream)
>   {
> -	struct sdw_master_runtime *m_rt = stream->m_rt;
> -	struct sdw_bus *bus = m_rt->bus;
> +	struct sdw_master_runtime *m_rt = NULL;
> +	struct sdw_bus *bus = NULL;
>   	int ret;
>   
> -	/* Program params */
> -	ret = sdw_program_params(bus);
> -	if (ret < 0) {
> -		dev_err(bus->dev, "Program params failed: %d", ret);
> -		return ret;
> -	}
> +	/* Enable Master(s) and Slave(s) port(s) associated with stream */
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		bus = m_rt->bus;
>   
> -	/* Enable port(s) */
> -	ret = sdw_enable_disable_ports(m_rt, true);
> -	if (ret < 0) {
> -		dev_err(bus->dev, "Enable port(s) failed ret: %d", ret);
> -		return ret;
> +		/* Program params */
> +		ret = sdw_program_params(bus);
> +		if (ret < 0) {
> +			dev_err(bus->dev, "Program params failed: %d", ret);
> +			return ret;
> +		}
> +
> +		/* Enable port(s) */
> +		ret = sdw_enable_disable_ports(m_rt, true);
> +		if (ret < 0) {
> +			dev_err(bus->dev, "Enable port(s) failed ret: %d", ret);
> +			return ret;
> +		}
>   	}
>   
>   	ret = do_bank_switch(stream);
> @@ -1400,37 +1455,42 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
>   		return -EINVAL;
>   	}
>   
> -	mutex_lock(&stream->m_rt->bus->bus_lock);
> +	sdw_acquire_bus_lock(stream);
>   
>   	ret = _sdw_enable_stream(stream);
>   	if (ret < 0)
>   		pr_err("Enable for stream:%s failed: %d", stream->name, ret);
>   
> -	mutex_unlock(&stream->m_rt->bus->bus_lock);
> +	sdw_release_bus_lock(stream);
>   	return ret;
>   }
>   EXPORT_SYMBOL(sdw_enable_stream);
>   
>   static int _sdw_disable_stream(struct sdw_stream_runtime *stream)
>   {
> -	struct sdw_master_runtime *m_rt = stream->m_rt;
> -	struct sdw_bus *bus = m_rt->bus;
> +	struct sdw_master_runtime *m_rt = NULL;
> +	struct sdw_bus *bus = NULL;
>   	int ret;
>   
> -	/* Disable port(s) */
> -	ret = sdw_enable_disable_ports(m_rt, false);
> -	if (ret < 0) {
> -		dev_err(bus->dev, "Disable port(s) failed: %d", ret);
> -		return ret;
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		bus = m_rt->bus;
> +		/* Disable port(s) */
> +		ret = sdw_enable_disable_ports(m_rt, false);
> +		if (ret < 0) {
> +			dev_err(bus->dev, "Disable port(s) failed: %d", ret);
> +			return ret;
> +		}
>   	}
> -
>   	stream->state = SDW_STREAM_DISABLED;
>   
> -	/* Program params */
> -	ret = sdw_program_params(bus);
> -	if (ret < 0) {
> -		dev_err(bus->dev, "Program params failed: %d", ret);
> -		return ret;
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		bus = m_rt->bus;
> +		/* Program params */
> +		ret = sdw_program_params(bus);
> +		if (ret < 0) {
> +			dev_err(bus->dev, "Program params failed: %d", ret);
> +			return ret;
> +		}
>   	}
>   
>   	return do_bank_switch(stream);
> @@ -1452,43 +1512,46 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
>   		return -EINVAL;
>   	}
>   
> -	mutex_lock(&stream->m_rt->bus->bus_lock);
> +	sdw_acquire_bus_lock(stream);
>   
>   	ret = _sdw_disable_stream(stream);
>   	if (ret < 0)
>   		pr_err("Disable for stream:%s failed: %d", stream->name, ret);
>   
> -	mutex_unlock(&stream->m_rt->bus->bus_lock);
> +	sdw_release_bus_lock(stream);
>   	return ret;
>   }
>   EXPORT_SYMBOL(sdw_disable_stream);
>   
>   static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream)
>   {
> -	struct sdw_master_runtime *m_rt = stream->m_rt;
> -	struct sdw_bus *bus = m_rt->bus;
> +	struct sdw_master_runtime *m_rt = NULL;
> +	struct sdw_bus *bus = NULL;
>   	int ret = 0;
>   
> -	/* De-prepare port(s) */
> -	ret = sdw_prep_deprep_ports(m_rt, false);
> -	if (ret < 0) {
> -		dev_err(bus->dev, "De-prepare port(s) failed: %d", ret);
> -		return ret;
> -	}
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		bus = m_rt->bus;
> +		/* De-prepare port(s) */
> +		ret = sdw_prep_deprep_ports(m_rt, false);
> +		if (ret < 0) {
> +			dev_err(bus->dev, "De-prepare port(s) failed: %d", ret);
> +			return ret;
> +		}
>   
> -	stream->state = SDW_STREAM_DEPREPARED;
> +		/* TODO: Update this during Device-Device support */
> +		bus->params.bandwidth -= m_rt->stream->params.rate *
> +			m_rt->ch_count * m_rt->stream->params.bps;
>   
> -	/* TODO: Update this during Device-Device support */
> -	bus->params.bandwidth -= m_rt->stream->params.rate *
> -		m_rt->ch_count * m_rt->stream->params.bps;
> +		/* Program params */
> +		ret = sdw_program_params(bus);
> +		if (ret < 0) {
> +			dev_err(bus->dev, "Program params failed: %d", ret);
> +			return ret;
> +		}
>   
> -	/* Program params */
> -	ret = sdw_program_params(bus);
> -	if (ret < 0) {
> -		dev_err(bus->dev, "Program params failed: %d", ret);
> -		return ret;
>   	}
>   
> +	stream->state = SDW_STREAM_DEPREPARED;
>   	return do_bank_switch(stream);
>   }
>   
> @@ -1508,13 +1571,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
>   		return -EINVAL;
>   	}
>   
> -	mutex_lock(&stream->m_rt->bus->bus_lock);
> -
> +	sdw_acquire_bus_lock(stream);
>   	ret = _sdw_deprepare_stream(stream);
>   	if (ret < 0)
>   		pr_err("De-prepare for stream:%d failed: %d", ret, ret);
>   
> -	mutex_unlock(&stream->m_rt->bus->bus_lock);
> +	sdw_release_bus_lock(stream);
>   	return ret;
>   }
>   EXPORT_SYMBOL(sdw_deprepare_stream);
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index ccd8dcdf..03df709 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -768,7 +768,6 @@ struct sdw_stream_params {
>    * @params: Stream parameters
>    * @state: Current state of the stream
>    * @type: Stream type PCM or PDM
> - * @m_rt: Master runtime
>    * @master_list: List of Master runtime(s) in this stream.
>    * master_list can contain only one m_rt per Master instance
>    * for a stream
> @@ -778,7 +777,6 @@ struct sdw_stream_runtime {
>   	struct sdw_stream_params params;
>   	enum sdw_stream_state state;
>   	enum sdw_stream_type type;
> -	struct sdw_master_runtime *m_rt;
>   	struct list_head master_list;
>   };
>   

Re: [PATCH v2 4/6] soundwire: Handle multiple master instances in a stream

[Sanyog Kale]

On Tue, Jun 12, 2018 at 02:12:46PM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 06/12/2018 05:53 AM, Shreyas NC wrote:
> >From: Vinod Koul <vkoul@kernel.org>
> >
> >For each SoundWire stream operation, we need to parse master
> >list and operate upon all master runtime.
> >
> >This patch does the boilerplate conversion of stream handling
> >from single master runtime to handle a list of master runtime.
> >
> >Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
> >Signed-off-by: Vinod Koul <vkoul@kernel.org>
> >Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
> >---
> >  drivers/soundwire/stream.c    | 308 +++++++++++++++++++++++++-----------------
> >  include/linux/soundwire/sdw.h |   2 -
> >  2 files changed, 185 insertions(+), 125 deletions(-)
> >
> >diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> >index eb942c6..36506a7 100644
> >--- a/drivers/soundwire/stream.c
> >+++ b/drivers/soundwire/stream.c
> >@@ -681,35 +681,44 @@ static int sdw_bank_switch(struct sdw_bus *bus)
> >  static int do_bank_switch(struct sdw_stream_runtime *stream)
> >  {
> >-	struct sdw_master_runtime *m_rt = stream->m_rt;
> >+	struct sdw_master_runtime *m_rt = NULL;
> >  	const struct sdw_master_ops *ops;
> >-	struct sdw_bus *bus = m_rt->bus;
> >+	struct sdw_bus *bus = NULL;
> >  	int ret = 0;
> >-	ops = bus->ops;
> >-	/* Pre-bank switch */
> >-	if (ops->pre_bank_switch) {
> >-		ret = ops->pre_bank_switch(bus);
> >+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> >+		bus = m_rt->bus;
> >+		ops = bus->ops;
> >+
> >+		/* Pre-bank switch */
> >+		if (ops->pre_bank_switch) {
> >+			ret = ops->pre_bank_switch(bus);
> >+			if (ret < 0) {
> >+				dev_err(bus->dev,
> >+					"Pre bank switch op failed: %d", ret);
> >+				return ret;
> >+			}
> >+		}
> >+
> >+		/* Bank switch */
> >+		ret = sdw_bank_switch(bus);
> >  		if (ret < 0) {
> >-			dev_err(bus->dev, "Pre bank switch op failed: %d", ret);
> >+			dev_err(bus->dev, "Bank switch failed: %d", ret);
> >  			return ret;
> >  		}
> You probably want to add a comment that in multi-link operation the actual
> bank_switch happens later and is done in a synchronized manner. This is lost
> if you just move code around and move the bank_switch into a loop.
> 
> I am actually no longer clear just looking at this code on when the
> bank_switch happens (i know we've discussed this before but I am trying to
> find out just based on this v2 instead of projecting how I think it should
> work):
> In Patch 6/6 it's pretty obvious that the bank switch happens when the
> SyncGO bit is set, but there is no comment or explanation on how we reach
> the intel_post_bank_switch() routine once for all masters handling a stream
> when everything is based on loops. Clearly the intel_pre_bank_switch is
> called multiple times (once per master), I guess I am missing what the
> trigger is for the intel_post_bank_switch() routine to be invoked?
>

Hi Pierre,

To answer your last question, do_bank_switch is where we perform all the
bank switch operations.

In first loop for Master(s) involved in stream, ops->pre_bank_switch and
bank_switch is performed. In 2nd loop for Master(s) involved in stream,
ops->post_bank_switch and wait for bank switch is performed.

Assuming a stream with Master 1 and Master 2, the go_sync bit will be
set in Master 1 intel_post_bank_switch call which will trigger bank
switch for both the Master's. The Master 2 intel_post_bank_switch call
will just return as it will won't see CMDSYNC bit set for any Master(s).

> >  	}
> >-	/* Bank switch */
> >-	ret = sdw_bank_switch(bus);
> >-	if (ret < 0) {
> >-		dev_err(bus->dev, "Bank switch failed: %d", ret);
> >-		return ret;
> >-	}
> >-
> >  	/* Post-bank switch */
> >-	if (ops->post_bank_switch) {
> >-		ret = ops->post_bank_switch(bus);
> >-		if (ret < 0) {
> >-			dev_err(bus->dev,
> >+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> >+		bus = m_rt->bus;
> >+		ops = bus->ops;
> >+		if (ops->post_bank_switch) {
> >+			ret = ops->post_bank_switch(bus);
> >+			if (ret < 0) {
> >+				dev_err(bus->dev,
> >  					"Post bank switch op failed: %d", ret);
> >+			}
> >  		}
> >  	}
> >  };
> 

-- 

Re: [PATCH v2 4/6] soundwire: Handle multiple master instances in a stream

[Shreyas NC]

> >  static int do_bank_switch(struct sdw_stream_runtime *stream)
> >  {
> >-	struct sdw_master_runtime *m_rt = stream->m_rt;
> >+	struct sdw_master_runtime *m_rt = NULL;
> >  	const struct sdw_master_ops *ops;
> >-	struct sdw_bus *bus = m_rt->bus;
> >+	struct sdw_bus *bus = NULL;
> >  	int ret = 0;
> >-	ops = bus->ops;
> >-	/* Pre-bank switch */
> >-	if (ops->pre_bank_switch) {
> >-		ret = ops->pre_bank_switch(bus);
> >+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> >+		bus = m_rt->bus;
> >+		ops = bus->ops;
> >+
> >+		/* Pre-bank switch */
> >+		if (ops->pre_bank_switch) {
> >+			ret = ops->pre_bank_switch(bus);
> >+			if (ret < 0) {
> >+				dev_err(bus->dev,
> >+					"Pre bank switch op failed: %d", ret);
> >+				return ret;
> >+			}
> >+		}
> >+
> >+		/* Bank switch */
> >+		ret = sdw_bank_switch(bus);
> >  		if (ret < 0) {
> >-			dev_err(bus->dev, "Pre bank switch op failed: %d", ret);
> >+			dev_err(bus->dev, "Bank switch failed: %d", ret);
> >  			return ret;
> >  		}
> You probably want to add a comment that in multi-link operation the actual
> bank_switch happens later and is done in a synchronized manner. This is lost
> if you just move code around and move the bank_switch into a loop.
> 

As mentioned in the commit log, this is just a boilerplate conversion to
loop over multiple m_rt and it is in the next patch that the
multilink_bank_switch() is added.
So, I can mention in the commit log of this patch that this is just a
preparatory patch for the next patch. And, also I will add the necessary
details in the next patch.

--Shreyas

-- 

Re: [PATCH v2 4/6] soundwire: Handle multiple master instances in a stream

[Pierre-Louis Bossart]

On 6/13/18 12:54 AM, Sanyog Kale wrote:
> On Tue, Jun 12, 2018 at 02:12:46PM -0500, Pierre-Louis Bossart wrote:
>>
>>
>> On 06/12/2018 05:53 AM, Shreyas NC wrote:
>>> From: Vinod Koul <vkoul@kernel.org>
>>>
>>> For each SoundWire stream operation, we need to parse master
>>> list and operate upon all master runtime.
>>>
>>> This patch does the boilerplate conversion of stream handling
>> >from single master runtime to handle a list of master runtime.
>>>
>>> Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
>>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>>> Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
>>> ---
>>>   drivers/soundwire/stream.c    | 308 +++++++++++++++++++++++++-----------------
>>>   include/linux/soundwire/sdw.h |   2 -
>>>   2 files changed, 185 insertions(+), 125 deletions(-)
>>>
>>> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
>>> index eb942c6..36506a7 100644
>>> --- a/drivers/soundwire/stream.c
>>> +++ b/drivers/soundwire/stream.c
>>> @@ -681,35 +681,44 @@ static int sdw_bank_switch(struct sdw_bus *bus)
>>>   static int do_bank_switch(struct sdw_stream_runtime *stream)
>>>   {
>>> -	struct sdw_master_runtime *m_rt = stream->m_rt;
>>> +	struct sdw_master_runtime *m_rt = NULL;
>>>   	const struct sdw_master_ops *ops;
>>> -	struct sdw_bus *bus = m_rt->bus;
>>> +	struct sdw_bus *bus = NULL;
>>>   	int ret = 0;
>>> -	ops = bus->ops;
>>> -	/* Pre-bank switch */
>>> -	if (ops->pre_bank_switch) {
>>> -		ret = ops->pre_bank_switch(bus);
>>> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
>>> +		bus = m_rt->bus;
>>> +		ops = bus->ops;
>>> +
>>> +		/* Pre-bank switch */
>>> +		if (ops->pre_bank_switch) {
>>> +			ret = ops->pre_bank_switch(bus);
>>> +			if (ret < 0) {
>>> +				dev_err(bus->dev,
>>> +					"Pre bank switch op failed: %d", ret);
>>> +				return ret;
>>> +			}
>>> +		}
>>> +
>>> +		/* Bank switch */
>>> +		ret = sdw_bank_switch(bus);
>>>   		if (ret < 0) {
>>> -			dev_err(bus->dev, "Pre bank switch op failed: %d", ret);
>>> +			dev_err(bus->dev, "Bank switch failed: %d", ret);
>>>   			return ret;
>>>   		}
>> You probably want to add a comment that in multi-link operation the actual
>> bank_switch happens later and is done in a synchronized manner. This is lost
>> if you just move code around and move the bank_switch into a loop.
>>
>> I am actually no longer clear just looking at this code on when the
>> bank_switch happens (i know we've discussed this before but I am trying to
>> find out just based on this v2 instead of projecting how I think it should
>> work):
>> In Patch 6/6 it's pretty obvious that the bank switch happens when the
>> SyncGO bit is set, but there is no comment or explanation on how we reach
>> the intel_post_bank_switch() routine once for all masters handling a stream
>> when everything is based on loops. Clearly the intel_pre_bank_switch is
>> called multiple times (once per master), I guess I am missing what the
>> trigger is for the intel_post_bank_switch() routine to be invoked?
>>
> 
> Hi Pierre,
> 
> To answer your last question, do_bank_switch is where we perform all the
> bank switch operations.
> 
> In first loop for Master(s) involved in stream, ops->pre_bank_switch and
> bank_switch is performed. In 2nd loop for Master(s) involved in stream,
> ops->post_bank_switch and wait for bank switch is performed.
> 
> Assuming a stream with Master 1 and Master 2, the go_sync bit will be
> set in Master 1 intel_post_bank_switch call which will trigger bank
> switch for both the Master's. The Master 2 intel_post_bank_switch call
> will just return as it will won't see CMDSYNC bit set for any Master(s).

Makes sense, thanks for taking the time to provide the details I didn't 
remember.
Sreyas, if you could add a bit of information on this it'd be a good 
thing - specifically the expectation is that the bank switch is 
triggered by the first master in the master_list loop while others just 
return without doing anything.

> 
>>>   	}
>>> -	/* Bank switch */
>>> -	ret = sdw_bank_switch(bus);
>>> -	if (ret < 0) {
>>> -		dev_err(bus->dev, "Bank switch failed: %d", ret);
>>> -		return ret;
>>> -	}
>>> -
>>>   	/* Post-bank switch */
>>> -	if (ops->post_bank_switch) {
>>> -		ret = ops->post_bank_switch(bus);
>>> -		if (ret < 0) {
>>> -			dev_err(bus->dev,
>>> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
>>> +		bus = m_rt->bus;
>>> +		ops = bus->ops;
>>> +		if (ops->post_bank_switch) {
>>> +			ret = ops->post_bank_switch(bus);
>>> +			if (ret < 0) {
>>> +				dev_err(bus->dev,
>>>   					"Post bank switch op failed: %d", ret);
>>> +			}
>>>   		}
>>>   	}
>>>   };
>>
> 

Re: [PATCH v2 4/6] soundwire: Handle multiple master instances in a stream

[Shreyas NC]

> >>>+
> >>>+		/* Bank switch */
> >>>+		ret = sdw_bank_switch(bus);
> >>>  		if (ret < 0) {
> >>>-			dev_err(bus->dev, "Pre bank switch op failed: %d", ret);
> >>>+			dev_err(bus->dev, "Bank switch failed: %d", ret);
> >>>  			return ret;
> >>>  		}
> >>You probably want to add a comment that in multi-link operation the actual
> >>bank_switch happens later and is done in a synchronized manner. This is lost
> >>if you just move code around and move the bank_switch into a loop.
> >>
> >>I am actually no longer clear just looking at this code on when the
> >>bank_switch happens (i know we've discussed this before but I am trying to
> >>find out just based on this v2 instead of projecting how I think it should
> >>work):
> >>In Patch 6/6 it's pretty obvious that the bank switch happens when the
> >>SyncGO bit is set, but there is no comment or explanation on how we reach
> >>the intel_post_bank_switch() routine once for all masters handling a stream
> >>when everything is based on loops. Clearly the intel_pre_bank_switch is
> >>called multiple times (once per master), I guess I am missing what the
> >>trigger is for the intel_post_bank_switch() routine to be invoked?
> >>
> >
> >Hi Pierre,
> >
> >To answer your last question, do_bank_switch is where we perform all the
> >bank switch operations.
> >
> >In first loop for Master(s) involved in stream, ops->pre_bank_switch and
> >bank_switch is performed. In 2nd loop for Master(s) involved in stream,
> >ops->post_bank_switch and wait for bank switch is performed.
> >
> >Assuming a stream with Master 1 and Master 2, the go_sync bit will be
> >set in Master 1 intel_post_bank_switch call which will trigger bank
> >switch for both the Master's. The Master 2 intel_post_bank_switch call
> >will just return as it will won't see CMDSYNC bit set for any Master(s).
> 
> Makes sense, thanks for taking the time to provide the details I didn't
> remember.
> Sreyas, if you could add a bit of information on this it'd be a good thing -
> specifically the expectation is that the bank switch is triggered by the
> first master in the master_list loop while others just return without doing
> anything.
> 

Sure, makes sense. I will add these details in the relevant patch.

--Shreyas

--