| Message ID | 1528975923-15141-6-git-send-email-shreyas.nc@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c > index 0e8a2eb5..771c963 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); > + Re-reading this v3, I wonder if there is a small logic flaw here or an intended behavior. the bus->multi_link field is a hardware capability, it provides information on whether the hardware can support synchronized bank switches across multiple Master interfaces. This is independent of the streams being handled, it's a bus property - not a stream one. However the default/typical case is a stream handled by a single master, so when the hardware is capable of handling multiple masters this does not mean you should use the deferred transfer, the regular case would work just fine for a regular stream. If your SSP rate is faster than the common sync signal it might even be beneficial not to use the deferred transfer. in other words, should the code be written with something like if (bus->multi_link && master_rt_count > 1) with master_rt_count being determined based on the stream properties. Or was the intent that you always rely on the deferred mechanism, even if the stream is not handled by multiple masters? In that case, do we even need the regular bank switch? > 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,36 +687,88 @@ 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; > } > } > > - /* Bank switch */ > + /* > + * Perform Bank switch operation. > + * For multi link cases, the actual bank switch is > + * synchronized across all Masters and happens later as a > + * part of post_bank_switch ops. > + */ > ret = sdw_bank_switch(bus); > if (ret < 0) { > dev_err(bus->dev, "Bank switch failed: %d", ret); > - return ret; > + goto error; > + > } > } > > + /* > + * For multi link cases, it is expected that the bank switch is > + * triggered by the post_bank_switch for the first Master in the list > + * and for the other Masters the post_bank_switch() should return doing > + * nothing. > + */ > list_for_each_entry(m_rt, &stream->master_list, stream_node) { > bus = m_rt->bus; > ops = bus->ops; > @@ -719,8 +779,44 @@ 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; > + > + /* Check if bank switch was successful */ > + ret = sdw_ml_sync_bank_switch(bus); > + if (ret < 0) { > + dev_err(bus->dev, > + "multi link bank switch 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; >
On Thu, Jun 14, 2018 at 01:34:13PM -0500, Pierre-Louis Bossart wrote: > > >diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c > >index 0e8a2eb5..771c963 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); > >+ > > Re-reading this v3, I wonder if there is a small logic flaw here or an > intended behavior. > > the bus->multi_link field is a hardware capability, it provides information > on whether the hardware can support synchronized bank switches across > multiple Master interfaces. This is independent of the streams being > handled, it's a bus property - not a stream one. > > However the default/typical case is a stream handled by a single master, so > when the hardware is capable of handling multiple masters this does not mean > you should use the deferred transfer, the regular case would work just fine > for a regular stream. If your SSP rate is faster than the common sync signal > it might even be beneficial not to use the deferred transfer. > > in other words, should the code be written with something like > if (bus->multi_link && master_rt_count > 1) > with master_rt_count being determined based on the stream properties. > > Or was the intent that you always rely on the deferred mechanism, even if > the stream is not handled by multiple masters? In that case, do we even need > the regular bank switch? After discussing with folks here, I understand that intent was to have an unified flow for both Multi and Single instance cases. Yes, we can always use the sdw_transfer_defer() and remove the references to bus->multi_link possibly. So, I will fix this up and post v4. Thanks for the catch :) --Shreyas
On 18-06-18, 15:20, Shreyas NC wrote: > On Thu, Jun 14, 2018 at 01:34:13PM -0500, Pierre-Louis Bossart wrote: > > > > >diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c > > >index 0e8a2eb5..771c963 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); > > >+ > > > > Re-reading this v3, I wonder if there is a small logic flaw here or an > > intended behavior. > > > > the bus->multi_link field is a hardware capability, it provides information > > on whether the hardware can support synchronized bank switches across > > multiple Master interfaces. This is independent of the streams being > > handled, it's a bus property - not a stream one. Pierre, You are right that multi-link is a hw capability BUT it is also a property of a stream. I can envision machines where bus may have the capability but the system configuration may have links being not clubbed in one board and made a multi-link stream in another board :) > > However the default/typical case is a stream handled by a single master, so > > when the hardware is capable of handling multiple masters this does not mean > > you should use the deferred transfer, the regular case would work just fine > > for a regular stream. If your SSP rate is faster than the common sync signal > > it might even be beneficial not to use the deferred transfer. > > > > in other words, should the code be written with something like > > if (bus->multi_link && master_rt_count > 1) > > with master_rt_count being determined based on the stream properties. Yes that makes sense to me > > Or was the intent that you always rely on the deferred mechanism, even if > > the stream is not handled by multiple masters? In that case, do we even need > > the regular bank switch? > > After discussing with folks here, I understand that intent was to have an > unified flow for both Multi and Single instance cases. Yes, we can always > use the sdw_transfer_defer() and remove the references to bus->multi_link > possibly. So, I will fix this up and post v4. Shreyas, That sounds like a good plan to me
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 0e8a2eb5..771c963 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,36 +687,88 @@ 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; } } - /* Bank switch */ + /* + * Perform Bank switch operation. + * For multi link cases, the actual bank switch is + * synchronized across all Masters and happens later as a + * part of post_bank_switch ops. + */ ret = sdw_bank_switch(bus); if (ret < 0) { dev_err(bus->dev, "Bank switch failed: %d", ret); - return ret; + goto error; + } } + /* + * For multi link cases, it is expected that the bank switch is + * triggered by the post_bank_switch for the first Master in the list + * and for the other Masters the post_bank_switch() should return doing + * nothing. + */ list_for_each_entry(m_rt, &stream->master_list, stream_node) { bus = m_rt->bus; ops = bus->ops; @@ -719,8 +779,44 @@ 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; + + /* Check if bank switch was successful */ + ret = sdw_ml_sync_bank_switch(bus); + if (ret < 0) { + dev_err(bus->dev, + "multi link bank switch 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;