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

[Sanyog Kale <sanyog.r.kale@intel.com>]

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

--