[alsa-devel] [PATCH v5 0/9] soundwire: Add multi link support
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: - Fix to avoid duplicate stream state assignment - Avoid incorrect stream release after configuring Master - 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 v5: - Add patch to fix duplicate stream state assignment - fix incorrect exit which was releasing the stream after configuring master - In sdw_release_bus_lock(), release the mutex in reverse order as suggested by Takashi
changes in v4: - Added changes in sdw_stream_runtime structure to track Masters added to stream - Add changes to support ml_bankswitch only if both the hardware supports and the stream is handled by multiple masters
Sanyog Kale (2): Documentation: soundwire: Add documentation for multi link soundwire: Add support to lock across bus instances
Shreyas NC (6): soundwire: Fix duplicate stream state assignment soundwire: fix incorrect exit after configuring stream soundwire: Initialize completion for defer messages soundwire: keep track of Masters in a stream soundwire: Add support for multi link 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 | 28 ++ drivers/soundwire/bus.c | 6 + drivers/soundwire/bus.h | 4 + drivers/soundwire/intel.c | 65 ++++ drivers/soundwire/stream.c | 502 +++++++++++++++++++------- include/linux/soundwire/sdw.h | 12 +- 6 files changed, 483 insertions(+), 134 deletions(-)
For a SoundWire stream it is expected that a Slave is added to the stream before Master is added.
So, move the stream state to CONFIGURED after the first Slave is added and remove the stream state assignment for Master add. Along with these changes, add additional comments to explain the same.
Signed-off-by: Shreyas NC shreyas.nc@intel.com --- drivers/soundwire/stream.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 8974a0f..37603c3 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1123,8 +1123,6 @@ int sdw_stream_add_master(struct sdw_bus *bus, if (ret) goto stream_error;
- stream->state = SDW_STREAM_CONFIGURED; - stream_error: sdw_release_master_stream(stream); error: @@ -1141,6 +1139,10 @@ EXPORT_SYMBOL(sdw_stream_add_master); * @stream: SoundWire stream * @port_config: Port configuration for audio stream * @num_ports: Number of ports + * + * It is expected that Slave is added before adding Master + * to the Stream. + * */ int sdw_stream_add_slave(struct sdw_slave *slave, struct sdw_stream_config *stream_config, @@ -1186,6 +1188,11 @@ int sdw_stream_add_slave(struct sdw_slave *slave, if (ret) goto stream_error;
+ /* + * Change stream state to CONFIGURED on first Slave add. + * We don't know number of Slaves in a stream currently so cant + * wait for all Slaves to be added + */ stream->state = SDW_STREAM_CONFIGURED; goto error;
On 07/05/2018 06:46 AM, Shreyas NC wrote:
For a SoundWire stream it is expected that a Slave is added to the stream before Master is added.
So, move the stream state to CONFIGURED after the first Slave is added and remove the stream state assignment for Master add. Along with these changes, add additional comments to explain the same.
Signed-off-by: Shreyas NC shreyas.nc@intel.com
drivers/soundwire/stream.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 8974a0f..37603c3 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1123,8 +1123,6 @@ int sdw_stream_add_master(struct sdw_bus *bus, if (ret) goto stream_error;
- stream->state = SDW_STREAM_CONFIGURED;
- stream_error: sdw_release_master_stream(stream); error:
@@ -1141,6 +1139,10 @@ EXPORT_SYMBOL(sdw_stream_add_master);
- @stream: SoundWire stream
- @port_config: Port configuration for audio stream
- @num_ports: Number of ports
- It is expected that Slave is added before adding Master
- to the Stream.
*/ int sdw_stream_add_slave(struct sdw_slave *slave, struct sdw_stream_config *stream_config,
@@ -1186,6 +1188,11 @@ int sdw_stream_add_slave(struct sdw_slave *slave, if (ret) goto stream_error;
- /*
* Change stream state to CONFIGURED on first Slave add.* We don't know number of Slaves in a stream currently so cant
can't or cannot.
* wait for all Slaves to be added*/
On Mon, Jul 09, 2018 at 06:41:40PM -0500, Pierre-Louis Bossart wrote:
On 07/05/2018 06:46 AM, Shreyas NC wrote:
For a SoundWire stream it is expected that a Slave is added to the stream before Master is added.
So, move the stream state to CONFIGURED after the first Slave is added and remove the stream state assignment for Master add. Along with these changes, add additional comments to explain the same.
Signed-off-by: Shreyas NC shreyas.nc@intel.com
drivers/soundwire/stream.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 8974a0f..37603c3 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1123,8 +1123,6 @@ int sdw_stream_add_master(struct sdw_bus *bus, if (ret) goto stream_error;
- stream->state = SDW_STREAM_CONFIGURED;
stream_error: sdw_release_master_stream(stream); error: @@ -1141,6 +1139,10 @@ EXPORT_SYMBOL(sdw_stream_add_master);
- @stream: SoundWire stream
- @port_config: Port configuration for audio stream
- @num_ports: Number of ports
- It is expected that Slave is added before adding Master
- to the Stream.
*/
int sdw_stream_add_slave(struct sdw_slave *slave, struct sdw_stream_config *stream_config, @@ -1186,6 +1188,11 @@ int sdw_stream_add_slave(struct sdw_slave *slave, if (ret) goto stream_error;
- /*
* Change stream state to CONFIGURED on first Slave add.* We don't know number of Slaves in a stream currently so cantcan't or cannot.
Will fix this.
* wait for all Slaves to be added*/
--
In sdw_stream_add_master() after the Master ports are configured, the stream is released incorrectly.
So, fix it by avoiding stream release after configuring the Master for the stream. While at it, rename the label appropriately.
Signed-off-by: Shreyas NC shreyas.nc@intel.com --- drivers/soundwire/stream.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 37603c3..5871594 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1112,7 +1112,7 @@ int sdw_stream_add_master(struct sdw_bus *bus, "Master runtime config failed for stream:%s", stream->name); ret = -ENOMEM; - goto error; + goto unlock; }
ret = sdw_config_stream(bus->dev, stream, stream_config, false); @@ -1123,9 +1123,11 @@ int sdw_stream_add_master(struct sdw_bus *bus, if (ret) goto stream_error;
+ goto unlock; + stream_error: sdw_release_master_stream(stream); -error: +unlock: mutex_unlock(&bus->bus_lock); return ret; }
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 | 28 +++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/Documentation/driver-api/soundwire/stream.rst b/Documentation/driver-api/soundwire/stream.rst index 29121aa..19482ee 100644 --- a/Documentation/driver-api/soundwire/stream.rst +++ b/Documentation/driver-api/soundwire/stream.rst @@ -101,6 +101,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 ================================
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);
From: Sanyog Kale sanyog.r.kale@intel.com
Currently, the stream concept is limited to single 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() APIs 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 | 45 +++++++++++++++++++++++++++++++++++++++++++ include/linux/soundwire/sdw.h | 4 ++++ 3 files changed, 51 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 5871594..8ddaad16 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; @@ -1243,6 +1244,50 @@ 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. + * + * This function is called from SoundWire stream ops and is + * expected that a global lock (DPCM mutex) is held before acquiring + * bus_lock. + */ +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_reverse(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);
From: Vinod Koul vkoul@kernel.org
For each SoundWire stream operation, we need to parse master list and operate upon all master runtime.
This is a preparatory patch to do the boilerplate conversion of stream handling from single master runtime to handle a list of master runtime. The code to support bank switch for multiple master instances is added in the next patch.
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 | 309 +++++++++++++++++++++++++----------------- include/linux/soundwire/sdw.h | 2 - 2 files changed, 186 insertions(+), 125 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 8ddaad16..c1a2c3d 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -681,35 +681,45 @@ 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; - } + list_for_each_entry(m_rt, &stream->master_list, stream_node) { + bus = m_rt->bus; + ops = bus->ops;
- /* Post-bank switch */ - if (ops->post_bank_switch) { - ret = ops->post_bank_switch(bus); - if (ret < 0) { - dev_err(bus->dev, + /* Post-bank switch */ + 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 +764,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 +795,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 +810,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 +868,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 +899,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 +918,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 +926,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 +951,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 +1169,7 @@ int sdw_stream_add_master(struct sdw_bus *bus, goto unlock;
stream_error: - sdw_release_master_stream(stream); + sdw_release_master_stream(m_rt, stream); unlock: mutex_unlock(&bus->bus_lock); return ret; @@ -1204,7 +1246,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; @@ -1290,31 +1332,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(¶ms, &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(¶ms, &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); @@ -1323,12 +1370,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; @@ -1356,35 +1407,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); @@ -1413,37 +1469,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); @@ -1465,43 +1526,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); }
@@ -1521,13 +1585,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; };
Sorry, another issue that I found while reviewing the entire section.
} @@ -888,6 +918,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 +926,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 +951,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);
So the sequence is
mutex_lock sdw_master_port_release() sdw_release_master_stream() Â Â Â sdw_stream_remove_slave() Â Â Â Â Â Â mutex_lock
Is this intentional to take the same mutex twice (not sure if it even works). what you probably wanted is to replace sdw_stream_remove_slave() by the equivalent sequence
sdw_slave_port_release() sdw_release_slave_stream()
which are both supposed to be called with a bus_lock held.
On Mon, Jul 09, 2018 at 06:42:34PM -0500, Pierre-Louis Bossart wrote:
Sorry, another issue that I found while reviewing the entire section.
} @@ -888,6 +918,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 +926,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 +951,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;So the sequence is
mutex_lock sdw_master_port_release() sdw_release_master_stream() ?????? sdw_stream_remove_slave() ?????? ?????? mutex_lock
Is this intentional to take the same mutex twice (not sure if it even works).
sdw_stream_remove_slave is called from sdw_release_master_stream to make sure all Slave(s) resources are freed up before freeing Master. sdw_stream_remove_slave is also called by Slave driver to free up Slave resources. In either case, we wanted to make sure the bus_lock is held hence the bus lock is held in sdw_stream_remove_slave API as well.
It doesnt look correct to take same mutex twice. Will check on this.
what you probably wanted is to replace sdw_stream_remove_slave() by the equivalent sequence
sdw_slave_port_release() sdw_release_slave_stream()
which are both supposed to be called with a bus_lock held.
you mean to say perform sdw_slave_port_release and sdw_release_slave_stream in sdw_release_master_stream instead of calling sdw_stream_remove_slave??
--
On 7/10/18 12:02 PM, Sanyog Kale wrote:
On Mon, Jul 09, 2018 at 06:42:34PM -0500, Pierre-Louis Bossart wrote:
Sorry, another issue that I found while reviewing the entire section.
} @@ -888,6 +918,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 +926,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 +951,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;So the sequence is
mutex_lock sdw_master_port_release() sdw_release_master_stream() ?????? sdw_stream_remove_slave() ?????? ?????? mutex_lock
Is this intentional to take the same mutex twice (not sure if it even works).
sdw_stream_remove_slave is called from sdw_release_master_stream to make sure all Slave(s) resources are freed up before freeing Master. sdw_stream_remove_slave is also called by Slave driver to free up Slave resources. In either case, we wanted to make sure the bus_lock is held hence the bus lock is held in sdw_stream_remove_slave API as well.
Yes, it's fine to take the lock from sdw_stream_remove_slave(), the point was to avoid taking the lock twice when the master is removed first.
It doesnt look correct to take same mutex twice. Will check on this.
what you probably wanted is to replace sdw_stream_remove_slave() by the equivalent sequence
sdw_slave_port_release() sdw_release_slave_stream()
which are both supposed to be called with a bus_lock held.
you mean to say perform sdw_slave_port_release and sdw_release_slave_stream in sdw_release_master_stream instead of calling sdw_stream_remove_slave??
Yes, something like the change below:
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); + sdw_slave_port_release() + sdw_release_slave_stream() list_del(&m_rt->stream_node); list_del(&m_rt->bus_node); kfree(m_rt); }
On Tue, Jul 10, 2018 at 01:16:30PM -0500, Pierre-Louis Bossart wrote:
On 7/10/18 12:02 PM, Sanyog Kale wrote:
On Mon, Jul 09, 2018 at 06:42:34PM -0500, Pierre-Louis Bossart wrote:
Sorry, another issue that I found while reviewing the entire section.
} @@ -888,6 +918,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 +926,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 +951,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;So the sequence is
mutex_lock sdw_master_port_release() sdw_release_master_stream() ?????? sdw_stream_remove_slave() ?????? ?????? mutex_lock
Is this intentional to take the same mutex twice (not sure if it even works).
sdw_stream_remove_slave is called from sdw_release_master_stream to make sure all Slave(s) resources are freed up before freeing Master. sdw_stream_remove_slave is also called by Slave driver to free up Slave resources. In either case, we wanted to make sure the bus_lock is held hence the bus lock is held in sdw_stream_remove_slave API as well.
Yes, it's fine to take the lock from sdw_stream_remove_slave(), the point was to avoid taking the lock twice when the master is removed first.
It doesnt look correct to take same mutex twice. Will check on this.
what you probably wanted is to replace sdw_stream_remove_slave() by the equivalent sequence
sdw_slave_port_release() sdw_release_slave_stream()
which are both supposed to be called with a bus_lock held.
you mean to say perform sdw_slave_port_release and sdw_release_slave_stream in sdw_release_master_stream instead of calling sdw_stream_remove_slave??
Yes, something like the change below:
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);
sdw_slave_port_release()sdw_release_slave_stream()}
Ok. Will update in next version.
Thanks
--
A multi link bankswitch can be done if the hardware supports and the stream is handled by multiple Master(s).
This preparatory patch adds support to track m_rt in a stream.
Signed-off-by: Shreyas NC shreyas.nc@intel.com --- drivers/soundwire/stream.c | 4 ++++ include/linux/soundwire/sdw.h | 2 ++ 2 files changed, 6 insertions(+)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index c1a2c3d..71d7e0b 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -759,6 +759,7 @@ struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name) stream->name = stream_name; INIT_LIST_HEAD(&stream->master_list); stream->state = SDW_STREAM_ALLOCATED; + stream->m_rt_count = 0;
return stream; } @@ -968,6 +969,7 @@ int sdw_stream_remove_master(struct sdw_bus *bus, if (list_empty(&stream->master_list)) stream->state = SDW_STREAM_RELEASED;
+ stream->m_rt_count--; mutex_unlock(&bus->bus_lock);
return 0; @@ -1166,6 +1168,8 @@ int sdw_stream_add_master(struct sdw_bus *bus, if (ret) goto stream_error;
+ stream->m_rt_count++; + goto unlock;
stream_error: diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 03df709..214e146 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -771,6 +771,7 @@ struct sdw_stream_params { * @master_list: List of Master runtime(s) in this stream. * master_list can contain only one m_rt per Master instance * for a stream + * @m_rt_count: Count of Master runtime(s) in this stream */ struct sdw_stream_runtime { char *name; @@ -778,6 +779,7 @@ struct sdw_stream_runtime { enum sdw_stream_state state; enum sdw_stream_type type; struct list_head master_list; + int m_rt_count; };
struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name);
In cases of multiple Masters 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.c | 5 ++ drivers/soundwire/bus.h | 2 + drivers/soundwire/stream.c | 141 ++++++++++++++++++++++++++++++++++++++---- include/linux/soundwire/sdw.h | 4 ++ 4 files changed, 140 insertions(+), 12 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index dbabd5e..1cbfedf 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -35,6 +35,11 @@ int sdw_add_bus_master(struct sdw_bus *bus) INIT_LIST_HEAD(&bus->slaves); INIT_LIST_HEAD(&bus->m_rt_list);
+ /* + * Initialize multi_link flag + * TODO: populate this flag by reading property from FW node + */ + bus->multi_link = false; if (bus->ops->read_prop) { ret = bus->ops->read_prop(bus); if (ret < 0) { 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 71d7e0b..60b562a 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -626,9 +626,10 @@ static int sdw_program_params(struct sdw_bus *bus) return ret; }
-static int sdw_bank_switch(struct sdw_bus *bus) +static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count) { int col_index, row_index; + bool multi_link; struct sdw_msg *wr_msg; u8 *wbuf = NULL; int ret = 0; @@ -638,6 +639,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 +661,29 @@ 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); + /* + * Set the multi_link flag only when both the hardware supports + * and there is a stream handled by multiple masters + */ + multi_link = bus->multi_link && (m_rt_count > 1); + + if (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 +694,87 @@ 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; + + if (!bus->multi_link) + return 0; + + /* 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 0; +} + 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 */ - ret = sdw_bank_switch(bus); + /* + * 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, stream->m_rt_count); 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,7 +785,47 @@ 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; } + } else if (bus->multi_link && stream->m_rt_count > 1) { + dev_err(bus->dev, + "Post bank switch ops not implemented"); + 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); } }
@@ -1170,6 +1276,17 @@ int sdw_stream_add_master(struct sdw_bus *bus,
stream->m_rt_count++;
+ /* + * For multi link streams, add the second master only if + * the bus supports it. + * Check if bus->multi_link is set + */ + if (!bus->multi_link && stream->m_rt_count > 1) { + dev_err(bus->dev, + "Multilink not supported, link %d", bus->link_id); + goto stream_error; + } + goto unlock;
stream_error: diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 214e146..df31391 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -678,6 +678,9 @@ struct sdw_master_ops { * @defer_msg: Defer message * @clk_stop_timeout: Clock stop timeout computed * @bank_switch_timeout: Bank switch timeout computed + * @multi_link: Store bus property that indicates if multi links + * are supported. This flag is populated by drivers after reading + * appropriate firmware (ACPI/DT). */ struct sdw_bus { struct device *dev; @@ -694,6 +697,7 @@ struct sdw_bus { struct sdw_defer defer_msg; unsigned int clk_stop_timeout; u32 bank_switch_timeout; + bool multi_link; };
int sdw_add_bus_master(struct sdw_bus *bus);
@@ -1170,6 +1276,17 @@ int sdw_stream_add_master(struct sdw_bus *bus,
stream->m_rt_count++;
/*
* For multi link streams, add the second master only if* the bus supports it.* Check if bus->multi_link is set*/if (!bus->multi_link && stream->m_rt_count > 1) {
dev_err(bus->dev,"Multilink not supported, link %d", bus->link_id);goto stream_error;}
goto unlock;
stream_error:
This error handling is a bit weird. It's make more sense to check upfront if there is already one m_rt allocated (m_rt_count > 0) and bail to avoid allocating/releasing a stream. Also there should be a return value set (-EINVAL?)
On Mon, Jul 09, 2018 at 06:22:26PM -0500, Pierre-Louis Bossart wrote:
@@ -1170,6 +1276,17 @@ int sdw_stream_add_master(struct sdw_bus *bus, stream->m_rt_count++;
- /*
* For multi link streams, add the second master only if* the bus supports it.* Check if bus->multi_link is set*/- if (!bus->multi_link && stream->m_rt_count > 1) {
dev_err(bus->dev,"Multilink not supported, link %d", bus->link_id);goto stream_error;- }
- goto unlock;
stream_error:
This error handling is a bit weird. It's make more sense to check upfront if there is already one m_rt allocated (m_rt_count > 0) and bail to avoid allocating/releasing a stream. Also there should be a return value set (-EINVAL?)
Agreed, this error check should be done upfront. Will fix this and also the return value.
--
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 | 65 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 0a8990e..da7f91f 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -398,6 +398,69 @@ 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); + + /* + * post_bank_switch() ops is called from the bus in loop for + * all the Masters in the steam with the expectation that + * we trigger the bankswitch for the only first Master in the list + * and do nothing for the other Masters + * + * So, set the SYNCGO bit only if CMDSYNC bit is set for any Master. + */ + if (!(sync_reg & SDW_SHIM_SYNC_CMDSYNC_MASK)) + return 0; + + /* + * Set SyncGO bit to synchronously trigger a bank switch for + * all the masters. A write to SYNCGO bit clears CMDSYNC bit for all + * the Masters. + */ + 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 +813,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, };
/*
On 07/05/2018 06:46 AM, Shreyas NC wrote:
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:
- Fix to avoid duplicate stream state assignment
- Avoid incorrect stream release after configuring Master
- 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 v5:
- Add patch to fix duplicate stream state assignment
- fix incorrect exit which was releasing the stream after configuring master
- In sdw_release_bus_lock(), release the mutex in reverse order as suggested by Takashi
It's almost good, I just had a couple of comments on the error cases and the lock handling on release. Can we respin a quick v6?
changes in v4:
- Added changes in sdw_stream_runtime structure to track Masters added to stream
- Add changes to support ml_bankswitch only if both the hardware supports and the stream is handled by multiple masters
Sanyog Kale (2): Documentation: soundwire: Add documentation for multi link soundwire: Add support to lock across bus instances
Shreyas NC (6): soundwire: Fix duplicate stream state assignment soundwire: fix incorrect exit after configuring stream soundwire: Initialize completion for defer messages soundwire: keep track of Masters in a stream soundwire: Add support for multi link 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 | 28 ++ drivers/soundwire/bus.c | 6 + drivers/soundwire/bus.h | 4 + drivers/soundwire/intel.c | 65 ++++ drivers/soundwire/stream.c | 502 +++++++++++++++++++------- include/linux/soundwire/sdw.h | 12 +- 6 files changed, 483 insertions(+), 134 deletions(-)
participants (3)
-
Pierre-Louis Bossart -
Sanyog Kale -
Shreyas NC