| Message ID | 1522946904-2089-3-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 4/5/18 11:48 AM, Vinod Koul wrote: > From: Sanyog Kale <sanyog.r.kale@intel.com> > > This patch adds APIs and relevant stream data structures > for initialization and release of stream. > > Signed-off-by: Hardik T Shah <hardik.t.shah@intel.com> > Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com> > Signed-off-by: Shreyas NC <shreyas.nc@intel.com> > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > --- > drivers/soundwire/Makefile | 2 +- > drivers/soundwire/bus.c | 1 + > drivers/soundwire/bus.h | 36 +++++ > drivers/soundwire/stream.c | 354 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/soundwire/sdw.h | 109 +++++++++++++ > 5 files changed, 501 insertions(+), 1 deletion(-) > create mode 100644 drivers/soundwire/stream.c > > diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile > index e1a74c5692aa..5817beaca0e1 100644 > --- a/drivers/soundwire/Makefile > +++ b/drivers/soundwire/Makefile > @@ -3,7 +3,7 @@ > # > > #Bus Objs > -soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o > +soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o > obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o > > #Cadence Objs > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c > index d6dc8e7a8614..abf046f6b188 100644 > --- a/drivers/soundwire/bus.c > +++ b/drivers/soundwire/bus.c > @@ -32,6 +32,7 @@ int sdw_add_bus_master(struct sdw_bus *bus) > mutex_init(&bus->msg_lock); > mutex_init(&bus->bus_lock); > INIT_LIST_HEAD(&bus->slaves); > + INIT_LIST_HEAD(&bus->m_rt_list); > > if (bus->ops->read_prop) { > ret = bus->ops->read_prop(bus); > diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h > index 345c34d697e9..3c66b6aecc14 100644 > --- a/drivers/soundwire/bus.h > +++ b/drivers/soundwire/bus.h > @@ -45,6 +45,42 @@ struct sdw_msg { > bool page; > }; > > +/** > + * sdw_slave_runtime: Runtime Stream parameters for Slave > + * > + * @slave: Slave handle > + * @direction: Data direction for Slave > + * @ch_count: Number of channels handled by the Slave for > + * this stream > + * @m_rt_node: sdw_master_runtime list node > + */ > +struct sdw_slave_runtime { > + struct sdw_slave *slave; > + enum sdw_data_direction direction; > + unsigned int ch_count; > + struct list_head m_rt_node; > +}; > + > +/** > + * sdw_master_runtime: Runtime stream parameters for Master > + * > + * @bus: Bus handle > + * @stream: Stream runtime handle > + * @direction: Data direction for Master > + * @ch_count: Number of channels handled by the Master for > + * this stream Didn't we say that this count could be zero for device-to-device communication? 'handled by the Master' is not really correct. > + * @slave_rt_list: Slave runtime list > + * @bus_node: sdw_bus m_rt_list node I think the intention was to progress in steps, but by omitting references to the multi-master case in this series or comments explaining the next steps the structure and code are not easy to follow. There should be a clear explanation that - a stream can be connected a least one Slave Device with one or more ports. - a stream may be connected to zero or more Master Devices. In the former case one of the Slave devices has provide TX ports. - the slave runtime keeps track of all the masters the stream is connected to - the master runtime keeps track of 1. all the Slaves it is physically connected to which support a stream. 2. all bus instances (and related masters) that support this stream. > + */ > +struct sdw_master_runtime { > + struct sdw_bus *bus; > + struct sdw_stream_runtime *stream; > + enum sdw_data_direction direction; > + unsigned int ch_count; > + struct list_head slave_rt_list; > + struct list_head bus_node; > +}; > + > int sdw_transfer(struct sdw_bus *bus, struct sdw_msg *msg); > int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg, > struct sdw_defer *defer); > diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c > new file mode 100644 > index 000000000000..5c34177e4954 > --- /dev/null > +++ b/drivers/soundwire/stream.c > @@ -0,0 +1,354 @@ > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) > +// Copyright(c) 2015-18 Intel Corporation. > + > +/* > + * stream.c - SoundWire Bus stream operations. > + */ > + > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/slab.h> > +#include <linux/soundwire/sdw.h> > +#include "bus.h" > + > +/** > + * sdw_release_stream: Free the assigned stream runtime > + * > + * @stream: SoundWire stream runtime > + * > + * sdw_release_stream should be called only once per stream > + */ > +void sdw_release_stream(struct sdw_stream_runtime *stream) > +{ > + kfree(stream); > +} > +EXPORT_SYMBOL(sdw_release_stream); > + > +/** > + * sdw_alloc_stream: Allocate and return stream runtime > + * > + * @stream_name: SoundWire stream name > + * > + * Allocates a SoundWire stream runtime instance. > + * sdw_alloc_stream should be called only once per stream You should explain who the caller might be, and probably check if a stream with the same name was not already allocated. > + */ > +struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name) > +{ > + struct sdw_stream_runtime *stream; > + > + stream = kzalloc(sizeof(*stream), GFP_KERNEL); > + if (!stream) > + return NULL; > + > + stream->name = stream_name; > + stream->state = SDW_STREAM_ALLOCATED; > + > + return stream; > +} > +EXPORT_SYMBOL(sdw_alloc_stream); > + > +/** > + * sdw_alloc_master_rt: Allocates and initialize Master runtime handle > + * > + * @bus: SDW bus instance > + * @stream_config: Stream configuration > + * @stream: Stream runtime handle. > + * > + * This function is to be called with bus_lock held. > + */ > +static struct sdw_master_runtime > +*sdw_alloc_master_rt(struct sdw_bus *bus, > + struct sdw_stream_config *stream_config, > + struct sdw_stream_runtime *stream) > +{ > + struct sdw_master_runtime *m_rt; > + > + m_rt = stream->m_rt; > + if (m_rt) > + goto stream_config; I know there is a reason for this code pattern but I just can't remember it and there is no comment indicated why this is valid. > + > + m_rt = kzalloc(sizeof(*m_rt), GFP_KERNEL); > + if (!m_rt) > + return NULL; > + > + /* Initialization of Master runtime handle */ > + INIT_LIST_HEAD(&m_rt->slave_rt_list); > + stream->m_rt = m_rt; > + > + list_add_tail(&m_rt->bus_node, &bus->m_rt_list); > + > +stream_config: > + m_rt->ch_count = stream_config->ch_count; > + m_rt->bus = bus; > + m_rt->stream = stream; > + m_rt->direction = stream_config->direction; > + > + return m_rt; > +} > + > +/** > + * sdw_alloc_slave_rt: Allocate and initialize Slave runtime handle. > + * > + * @slave: Slave handle > + * @stream_config: Stream configuration > + * @stream: Stream runtime handle > + * > + * This function is to be called with bus_lock held. > + */ > +static struct sdw_slave_runtime > +*sdw_alloc_slave_rt(struct sdw_slave *slave, > + struct sdw_stream_config *stream_config, > + struct sdw_stream_runtime *stream) > +{ > + struct sdw_slave_runtime *s_rt = NULL; > + > + s_rt = kzalloc(sizeof(*s_rt), GFP_KERNEL); > + if (!s_rt) > + return NULL; > + > + > + s_rt->ch_count = stream_config->ch_count; > + s_rt->direction = stream_config->direction; > + s_rt->slave = slave; > + > + return s_rt; > +} > + > +/** > + * sdw_release_slave_stream: Free Slave(s) runtime handle > + * > + * @slave: Slave handle. > + * @stream: Stream runtime handle. > + * > + * This function is to be called with bus_lock held. > + */ > +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) { > + > + if (s_rt->slave == slave) { > + list_del(&s_rt->m_rt_node); > + kfree(s_rt); > + return; > + } > + } > +} > + > +static void sdw_release_master_stream(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_release_slave_stream(s_rt->slave, stream); > + > + list_del(&m_rt->bus_node); > +} > + > +/** > + * sdw_stream_remove_master: Remove master from sdw_stream > + * > + * @bus: SDW Bus instance > + * @stream: Soundwire stream > + * > + * This removes and frees master_rt from a stream > + */ > +int sdw_stream_remove_master(struct sdw_bus *bus, > + struct sdw_stream_runtime *stream) > +{ > + mutex_lock(&bus->bus_lock); > + > + sdw_release_master_stream(stream); > + stream->state = SDW_STREAM_RELEASED; > + kfree(stream->m_rt); > + stream->m_rt = NULL; > + > + mutex_unlock(&bus->bus_lock); > + > + return 0; > +} > +EXPORT_SYMBOL(sdw_stream_remove_master); > + > +/** > + * sdw_stream_remove_slave: Remove slave from sdw_stream > + * > + * @slave: SDW Slave instance > + * @stream: Soundwire stream > + * > + * This removes and frees slave_rt from a stream > + */ > +int sdw_stream_remove_slave(struct sdw_slave *slave, > + struct sdw_stream_runtime *stream) > +{ > + mutex_lock(&slave->bus->bus_lock); > + > + sdw_release_slave_stream(slave, stream); > + > + mutex_unlock(&slave->bus->bus_lock); > + > + return 0; > +} > +EXPORT_SYMBOL(sdw_stream_remove_slave); > + > +/** > + * sdw_config_stream: Configure the allocated stream > + * > + * @dev: SDW device > + * @stream: Soundwire stream > + * @stream_config: Stream configuration for audio stream > + * @is_slave: is API called from Slave or Master > + * > + * This function is to be called with bus_lock held. > + */ > +static int sdw_config_stream(struct device *dev, > + struct sdw_stream_runtime *stream, > + struct sdw_stream_config *stream_config, bool is_slave) > +{ > + > + /* > + * Update the stream rate, channel and bps based on data > + * source. For more than one data source (multilink), > + * match the rate, bps, stream type and increment number of channels. > + */ > + if ((stream->params.rate) && > + (stream->params.rate != stream_config->frame_rate)) { > + dev_err(dev, "rate not matching, stream:%s", stream->name); > + return -EINVAL; > + } > + > + if ((stream->params.bps) && > + (stream->params.bps != stream_config->bps)) { > + dev_err(dev, "bps not matching, stream:%s", stream->name); > + return -EINVAL; > + } > + > + stream->type = stream_config->type; > + stream->params.rate = stream_config->frame_rate; > + stream->params.bps = stream_config->bps; > + if (is_slave) > + stream->params.ch_count += stream_config->ch_count; > + > + return 0; > +} > + > +/** > + * sdw_stream_add_master: Allocate and add master runtime to a stream > + * > + * @bus: SDW Bus instance > + * @stream_config: Stream configuration for audio stream > + * @stream: Soundwire stream > + */ > +int sdw_stream_add_master(struct sdw_bus *bus, > + struct sdw_stream_config *stream_config, > + struct sdw_stream_runtime *stream) > +{ > + struct sdw_master_runtime *m_rt = NULL; > + int ret; > + > + if (stream->state != SDW_STREAM_ALLOCATED && > + stream->state != SDW_STREAM_CONFIGURED) { > + dev_err(bus->dev, > + "Invalid stream state %d", stream->state); > + return -EINVAL; > + } Humm, this check looks like new code but I don't get it. Why would one add a master in either of those two states? Why not ALLOCATED only? The documentation states that: +SDW_STREAM_CONFIGURED +~~~~~~~~~~~~~~~~~~~~~ + +Configuration state of stream. Operations performed before entering in +this state: + + (1) The resources allocated for stream information in SDW_STREAM_ALLOCATED + state are updated here. This includes stream parameters, Master(s) + and Slave(s) runtime information associated with current stream. + + (2) All the Master(s) and Slave(s) associated with current stream provide + the port information to Bus which includes port numbers allocated by + Master(s) and Slave(s) for current stream and their channel mask. so how can Master ports be configured if it is added to a stream *after* the CONFIGURED state? > + > + mutex_lock(&bus->bus_lock); > + > + m_rt = sdw_alloc_master_rt(bus, stream_config, stream); > + if (!m_rt) { > + dev_err(bus->dev, > + "Master runtime config failed for stream:%s", > + stream->name); > + ret = -ENOMEM; > + goto error; > + } > + > + ret = sdw_config_stream(bus->dev, stream, stream_config, false); > + if (ret) > + goto stream_error; > + > + if (!list_empty(&m_rt->slave_rt_list) && > + stream->state == SDW_STREAM_ALLOCATED) > + stream->state = SDW_STREAM_CONFIGURED; this looks also like new code, what is the idea about this configuration business? To me this is inconsistent with the documentation which says sdw_stream_add_master is called in ALLOCATED state. Under what circumstances would this routing be called from two different states? This may be ok but the intent is clear as mud. > + > +stream_error: > + sdw_release_master_stream(stream); > +error: > + mutex_unlock(&bus->bus_lock); > + return ret; > + > +} > +EXPORT_SYMBOL(sdw_stream_add_master); > + > +/** > + * sdw_stream_add_slave: Allocate and add master/slave runtime to a stream > + * > + * @slave: SDW Slave instance > + * @stream_config: Stream configuration for audio stream > + * @stream: Soundwire stream > + */ > +int sdw_stream_add_slave(struct sdw_slave *slave, > + struct sdw_stream_config *stream_config, > + struct sdw_stream_runtime *stream) > +{ > + struct sdw_slave_runtime *s_rt; > + struct sdw_master_runtime *m_rt; > + int ret; > + > + if (stream->state != SDW_STREAM_ALLOCATED && > + stream->state != SDW_STREAM_CONFIGURED) { > + dev_err(&slave->dev, > + "Invalid stream state %d", stream->state); > + return -EINVAL; > + } same as for the master, why would one call this routine from two separate states? > + > + mutex_lock(&slave->bus->bus_lock); > + > + /* > + * If this API is invoked by slave first then m_rt is not valid. > + * So, allocate that and add the slave to it. > + */ > + m_rt = sdw_alloc_master_rt(slave->bus, stream_config, stream); > + if (!m_rt) { > + dev_err(&slave->dev, > + "alloc master runtime failed for stream:%s", > + stream->name); > + ret = -ENOMEM; > + goto error; > + } > + > + s_rt = sdw_alloc_slave_rt(slave, stream_config, stream); > + if (!s_rt) { > + dev_err(&slave->dev, > + "Slave runtime config failed for stream:%s", > + stream->name); > + ret = -ENOMEM; > + goto stream_error; > + } > + > + ret = sdw_config_stream(&slave->dev, stream, stream_config, true); > + if (ret) > + goto stream_error; > + > + list_add_tail(&s_rt->m_rt_node, &m_rt->slave_rt_list); so you have a potential to add the same node twice from ALLOCATED and CONFIGURED states? I am completely lost. > + > + stream->state = SDW_STREAM_CONFIGURED; > + goto error; > + > +stream_error: > + sdw_release_master_stream(stream); maybe add a comment that this will also release all previously allocated slave_rt nodes for that stream? > +error: > + mutex_unlock(&slave->bus->bus_lock); > + return ret; > +} > +EXPORT_SYMBOL(sdw_stream_add_slave); > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > index e91fdcf41049..893e1b6b4914 100644 > --- a/include/linux/soundwire/sdw.h > +++ b/include/linux/soundwire/sdw.h > @@ -61,6 +61,30 @@ enum sdw_command_response { > SDW_CMD_FAIL_OTHER = 4, > }; > > +/** > + * enum sdw_stream_type: data stream type > + * > + * @SDW_STREAM_PCM: PCM data stream > + * @SDW_STREAM_PDM: PDM data stream > + * > + * spec doesn't define this, but is used in implementation > + */ > +enum sdw_stream_type { > + SDW_STREAM_PCM = 0, > + SDW_STREAM_PDM = 1, > +}; > + > +/** > + * enum sdw_data_direction: Data direction > + * > + * @SDW_DATA_DIR_RX: Data into Port > + * @SDW_DATA_DIR_TX: Data out of Port > + */ > +enum sdw_data_direction { > + SDW_DATA_DIR_RX = 0, > + SDW_DATA_DIR_TX = 1, > +}; > + > /* > * SDW properties, defined in MIPI DisCo spec v1.0 > */ > @@ -450,6 +474,9 @@ struct sdw_master_ops { > * @msg_lock: message lock > * @ops: Master callback ops > * @prop: Master properties > + * @m_rt_list: List of Master instance of all stream(s) running on Bus. This > + * is used to compute and program bus bandwidth, clock, frame shape, > + * transport and port parameters > * @defer_msg: Defer message > * @clk_stop_timeout: Clock stop timeout computed > */ > @@ -462,6 +489,7 @@ struct sdw_bus { > struct mutex msg_lock; > const struct sdw_master_ops *ops; > struct sdw_master_prop prop; > + struct list_head m_rt_list; > struct sdw_defer defer_msg; > unsigned int clk_stop_timeout; > }; > @@ -469,6 +497,87 @@ struct sdw_bus { > int sdw_add_bus_master(struct sdw_bus *bus); > void sdw_delete_bus_master(struct sdw_bus *bus); > > +/** > + * sdw_stream_config: Master or Slave stream configuration > + * > + * @frame_rate: Audio frame rate of the stream, in Hz > + * @ch_count: Channel count of the stream > + * @bps: Number of bits per audio sample > + * @direction: Data direction > + * @type: Stream type PCM or PDM > + */ > +struct sdw_stream_config { > + unsigned int frame_rate; > + unsigned int ch_count; > + unsigned int bps; > + enum sdw_data_direction direction; > + enum sdw_stream_type type; > +}; > + > +/** > + * sdw_stream_state: Stream states > + * > + * @SDW_STREAM_RELEASED: Stream released > + * @SDW_STREAM_ALLOCATED: New stream allocated. > + * @SDW_STREAM_CONFIGURED: Stream configured > + * @SDW_STREAM_PREPARED: Stream prepared > + * @SDW_STREAM_ENABLED: Stream enabled > + * @SDW_STREAM_DISABLED: Stream disabled > + * @SDW_STREAM_DEPREPARED: Stream de-prepared > + */ > +enum sdw_stream_state { > + SDW_STREAM_RELEASED = 0, > + SDW_STREAM_ALLOCATED = 1, > + SDW_STREAM_CONFIGURED = 2, > + SDW_STREAM_PREPARED = 3, > + SDW_STREAM_ENABLED = 4, > + SDW_STREAM_DISABLED = 5, > + SDW_STREAM_DEPREPARED = 6, > +}; > + > +/** > + * sdw_stream_params: Stream parameters > + * > + * @rate: Sampling frequency, in Hz > + * @ch_count: Number of channels > + * @bps: bits per channel sample > + */ > +struct sdw_stream_params { > + unsigned int rate; > + unsigned int ch_count; > + unsigned int bps; > +}; > + > +/** > + * sdw_stream_runtime: Runtime stream parameters > + * > + * @name: SoundWire stream name > + * @params: Stream parameters > + * @state: Current state of the stream > + * @type: Stream type PCM or PDM > + * @m_rt: Master runtime > + */ > +struct sdw_stream_runtime { > + char *name; > + struct sdw_stream_params params; > + enum sdw_stream_state state; > + enum sdw_stream_type type; > + struct sdw_master_runtime *m_rt; > +}; > + > +struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name); > +void sdw_release_stream(struct sdw_stream_runtime *stream); > +int sdw_stream_add_master(struct sdw_bus *bus, > + struct sdw_stream_config *stream_config, > + struct sdw_stream_runtime *stream); > +int sdw_stream_add_slave(struct sdw_slave *slave, > + struct sdw_stream_config *stream_config, > + struct sdw_stream_runtime *stream); > +int sdw_stream_remove_master(struct sdw_bus *bus, > + struct sdw_stream_runtime *stream); > +int sdw_stream_remove_slave(struct sdw_slave *slave, > + struct sdw_stream_runtime *stream); > + > /* messaging and data APIs */ > > int sdw_read(struct sdw_slave *slave, u32 addr); >
On Thu, Apr 05, 2018 at 05:34:18PM -0500, Pierre-Louis Bossart wrote: > On 4/5/18 11:48 AM, Vinod Koul wrote: > >+/** > >+ * sdw_master_runtime: Runtime stream parameters for Master > >+ * > >+ * @bus: Bus handle > >+ * @stream: Stream runtime handle > >+ * @direction: Data direction for Master > >+ * @ch_count: Number of channels handled by the Master for > >+ * this stream > > Didn't we say that this count could be zero for device-to-device > communication? 'handled by the Master' is not really correct. Yes for device-to-device this would be zero. And master will handle no channels as indicated by zero value. Won't that be right? > >+ * @slave_rt_list: Slave runtime list > >+ * @bus_node: sdw_bus m_rt_list node > > I think the intention was to progress in steps, but by omitting references > to the multi-master case in this series or comments explaining the next > steps the structure and code are not easy to follow. We cannot have any ref to multi-master as it is not in this series. That is how patches would be done, you add a step and then increment with another. Ofcourse if you have any question feel free to ask and we can clarify, but I do not think it would be correct to add comments/reference here for multi-master. These will be updated as required when we post multi-master. > There should be a clear explanation that > - a stream can be connected a least one Slave Device with one or more ports. explained in document patch > - a stream may be connected to zero or more Master Devices. In the former > case one of the Slave devices has provide TX ports. That's multi-link/device-device not under review here > - the slave runtime keeps track of all the masters the stream is connected > to not that is not a correct assumption > - the master runtime keeps track of > 1. all the Slaves it is physically connected to which support a stream. Yes that is added here > 2. all bus instances (and related masters) that support this stream. That's again multi-link! Again we are not writing a spec but code and comments to explain the code. The above is not related to "this" series so we should not add it here. When we add device-device support, multi-master support these can be added We are adding blocks in a giant building, but you are only looking at the giant picture but not the block in discussion! > >+/** > >+ * sdw_alloc_stream: Allocate and return stream runtime > >+ * > >+ * @stream_name: SoundWire stream name > >+ * > >+ * Allocates a SoundWire stream runtime instance. > >+ * sdw_alloc_stream should be called only once per stream > > You should explain who the caller might be, and probably check if a stream > with the same name was not already allocated. Checking name might be bit iffy as we would need to check all streams. The name is used for prints so if caller screws up name then it would be at his own peril to get confused between same stream names in logs :) > >+static struct sdw_master_runtime > >+*sdw_alloc_master_rt(struct sdw_bus *bus, > >+ struct sdw_stream_config *stream_config, > >+ struct sdw_stream_runtime *stream) > >+{ > >+ struct sdw_master_runtime *m_rt; > >+ > >+ m_rt = stream->m_rt; > >+ if (m_rt) > >+ goto stream_config; > > I know there is a reason for this code pattern but I just can't remember it > and there is no comment indicated why this is valid. If m_rt is already allocated we skip the initialization and configure it. As we have told you earlier, the slave runtime can be added which triggers master runtime to be allocated as well, so we need to check. We do have such a comment in sdw_stream_add_slave() as you advised. Should we duplicate it here too? > >+int sdw_stream_add_master(struct sdw_bus *bus, > >+ struct sdw_stream_config *stream_config, > >+ struct sdw_stream_runtime *stream) > >+{ > >+ struct sdw_master_runtime *m_rt = NULL; > >+ int ret; > >+ > >+ if (stream->state != SDW_STREAM_ALLOCATED && > >+ stream->state != SDW_STREAM_CONFIGURED) { > >+ dev_err(bus->dev, > >+ "Invalid stream state %d", stream->state); > >+ return -EINVAL; > >+ } > > Humm, this check looks like new code but I don't get it. Why would one add a > master in either of those two states? Why not ALLOCATED only? So slave or master can be added to a stream in any order. Only if slave is called first we need master_rt to be allocated too. In ideal world we would do allocation and then configuration, but due to user calling in any order we may do allocation and configuration of slave first followed by configuring the master. > > The documentation states that: > > +SDW_STREAM_CONFIGURED > +~~~~~~~~~~~~~~~~~~~~~ > + > +Configuration state of stream. Operations performed before entering in > +this state: > + > + (1) The resources allocated for stream information in > SDW_STREAM_ALLOCATED > + state are updated here. This includes stream parameters, Master(s) > + and Slave(s) runtime information associated with current stream. > + > + (2) All the Master(s) and Slave(s) associated with current stream provide > + the port information to Bus which includes port numbers allocated by > + Master(s) and Slave(s) for current stream and their channel mask. > > so how can Master ports be configured if it is added to a stream *after* the > CONFIGURED state? Yes you have a valid point wrt documentation, will update it. > > >+ > >+ mutex_lock(&bus->bus_lock); > >+ > >+ m_rt = sdw_alloc_master_rt(bus, stream_config, stream); > >+ if (!m_rt) { > >+ dev_err(bus->dev, > >+ "Master runtime config failed for stream:%s", > >+ stream->name); > >+ ret = -ENOMEM; > >+ goto error; > >+ } > >+ > >+ ret = sdw_config_stream(bus->dev, stream, stream_config, false); > >+ if (ret) > >+ goto stream_error; > >+ > >+ if (!list_empty(&m_rt->slave_rt_list) && > >+ stream->state == SDW_STREAM_ALLOCATED) > >+ stream->state = SDW_STREAM_CONFIGURED; > > this looks also like new code, what is the idea about this configuration > business? > To me this is inconsistent with the documentation which says > sdw_stream_add_master is called in ALLOCATED state. Under what circumstances > would this routing be called from two different states? Yes it "may" be due to sequencing in callers control. > This may be ok but the intent is clear as mud. Yes that is a bit unfortunate :( > >+int sdw_stream_add_slave(struct sdw_slave *slave, > >+ struct sdw_stream_config *stream_config, > >+ struct sdw_stream_runtime *stream) > >+{ > >+ struct sdw_slave_runtime *s_rt; > >+ struct sdw_master_runtime *m_rt; > >+ int ret; > >+ > >+ if (stream->state != SDW_STREAM_ALLOCATED && > >+ stream->state != SDW_STREAM_CONFIGURED) { > >+ dev_err(&slave->dev, > >+ "Invalid stream state %d", stream->state); > >+ return -EINVAL; > >+ } > > same as for the master, why would one call this routine from two separate > states? as discussed above > > >+ > >+ mutex_lock(&slave->bus->bus_lock); > >+ > >+ /* > >+ * If this API is invoked by slave first then m_rt is not valid. > >+ * So, allocate that and add the slave to it. > >+ */ > >+ m_rt = sdw_alloc_master_rt(slave->bus, stream_config, stream); > >+ if (!m_rt) { > >+ dev_err(&slave->dev, > >+ "alloc master runtime failed for stream:%s", > >+ stream->name); > >+ ret = -ENOMEM; > >+ goto error; > >+ } > >+ > >+ s_rt = sdw_alloc_slave_rt(slave, stream_config, stream); > >+ if (!s_rt) { > >+ dev_err(&slave->dev, > >+ "Slave runtime config failed for stream:%s", > >+ stream->name); > >+ ret = -ENOMEM; > >+ goto stream_error; > >+ } > >+ > >+ ret = sdw_config_stream(&slave->dev, stream, stream_config, true); > >+ if (ret) > >+ goto stream_error; > >+ > >+ list_add_tail(&s_rt->m_rt_node, &m_rt->slave_rt_list); > > so you have a potential to add the same node twice from ALLOCATED and > CONFIGURED states? I am completely lost. That would be the case if Slave invokes this routine erroneously twice. We expect this would be called only once by caller.
On 4/5/18 11:53 PM, Vinod Koul wrote: > On Thu, Apr 05, 2018 at 05:34:18PM -0500, Pierre-Louis Bossart wrote: >> On 4/5/18 11:48 AM, Vinod Koul wrote: > >>> +/** >>> + * sdw_master_runtime: Runtime stream parameters for Master >>> + * >>> + * @bus: Bus handle >>> + * @stream: Stream runtime handle >>> + * @direction: Data direction for Master >>> + * @ch_count: Number of channels handled by the Master for >>> + * this stream >> >> Didn't we say that this count could be zero for device-to-device >> communication? 'handled by the Master' is not really correct. > > Yes for device-to-device this would be zero. And master will handle no > channels as indicated by zero value. Won't that be right? It doesn't hurt to say that zero is an acceptable value by design. > >>> + * @slave_rt_list: Slave runtime list >>> + * @bus_node: sdw_bus m_rt_list node >> >> I think the intention was to progress in steps, but by omitting references >> to the multi-master case in this series or comments explaining the next >> steps the structure and code are not easy to follow. > > We cannot have any ref to multi-master as it is not in this series. That is how > patches would be done, you add a step and then increment with another. The point is that the bus_node is only relevant for multi-master, so you have structure fields are present, not currently used but will be. It's perfectly ok to progress in steps, but you have to be consistent. If you keep a structure that will only be used later, you need to tell this to reviewers. > Ofcourse if you have any question feel free to ask and we can clarify, but I > do not think it would be correct to add comments/reference here for > multi-master. These will be updated as required when we post multi-master. > >> There should be a clear explanation that >> - a stream can be connected a least one Slave Device with one or more ports. > > explained in document patch > >> - a stream may be connected to zero or more Master Devices. In the former >> case one of the Slave devices has provide TX ports. > > That's multi-link/device-device not under review here zero or more describes the single master case. >> - the slave runtime keeps track of all the masters the stream is connected >> to > > not that is not a correct assumption yes this is wrong, I meant "the master runtime keeps tracks off all the Slaves the stream is connected to" > >> - the master runtime keeps track of >> 1. all the Slaves it is physically connected to which support a stream. > > Yes that is added here > >> 2. all bus instances (and related masters) that support this stream. > > That's again multi-link! the statement is still correct with a single instance :-) > Again we are not writing a spec but code and comments to explain the code. > The above is not related to "this" series so we should not add it here. When > we add device-device support, multi-master support these can be added > > We are adding blocks in a giant building, but you are only looking at the > giant picture but not the block in discussion! no, you have keep mentions to multi-link that are questionable. Either you remove them or you state it's reserved for future use. >>> +/** >>> + * sdw_alloc_stream: Allocate and return stream runtime >>> + * >>> + * @stream_name: SoundWire stream name >>> + * >>> + * Allocates a SoundWire stream runtime instance. >>> + * sdw_alloc_stream should be called only once per stream >> >> You should explain who the caller might be, and probably check if a stream >> with the same name was not already allocated. > > Checking name might be bit iffy as we would need to check all streams. The > name is used for prints so if caller screws up name then it would be at his > own peril to get confused between same stream names in logs :) You didn't answer to the question: who is supposed to call sdw_alloc_steam()? the machine driver? The platform driver? the codec driver? How do you garantee that it's called once >>> +static struct sdw_master_runtime >>> +*sdw_alloc_master_rt(struct sdw_bus *bus, >>> + struct sdw_stream_config *stream_config, >>> + struct sdw_stream_runtime *stream) >>> +{ >>> + struct sdw_master_runtime *m_rt; >>> + >>> + m_rt = stream->m_rt; >>> + if (m_rt) >>> + goto stream_config; >> >> I know there is a reason for this code pattern but I just can't remember it >> and there is no comment indicated why this is valid. > > If m_rt is already allocated we skip the initialization and configure it. > As we have told you earlier, the slave runtime can be added which triggers > master runtime to be allocated as well, so we need to check. > > We do have such a comment in sdw_stream_add_slave() as you advised. Should > we duplicate it here too? doesn't hurt to add a comment pointing to sdw_stream_add_slave() here as well, the reviewers read linearly and if the comment is in 200 lines it doesn't help. > >>> +int sdw_stream_add_master(struct sdw_bus *bus, >>> + struct sdw_stream_config *stream_config, >>> + struct sdw_stream_runtime *stream) >>> +{ >>> + struct sdw_master_runtime *m_rt = NULL; >>> + int ret; >>> + >>> + if (stream->state != SDW_STREAM_ALLOCATED && >>> + stream->state != SDW_STREAM_CONFIGURED) { >>> + dev_err(bus->dev, >>> + "Invalid stream state %d", stream->state); >>> + return -EINVAL; >>> + } >> >> Humm, this check looks like new code but I don't get it. Why would one add a >> master in either of those two states? Why not ALLOCATED only? > > So slave or master can be added to a stream in any order. Only if slave is > called first we need master_rt to be allocated too. In ideal world we would > do allocation and then configuration, but due to user calling in any order > we may do allocation and configuration of slave first followed by > configuring the master. that makes no sense. You have a state diagram that defines a state transition which precludes this order between allocation and configuration. > >> >> The documentation states that: >> >> +SDW_STREAM_CONFIGURED >> +~~~~~~~~~~~~~~~~~~~~~ >> + >> +Configuration state of stream. Operations performed before entering in >> +this state: >> + >> + (1) The resources allocated for stream information in >> SDW_STREAM_ALLOCATED >> + state are updated here. This includes stream parameters, Master(s) >> + and Slave(s) runtime information associated with current stream. >> + >> + (2) All the Master(s) and Slave(s) associated with current stream provide >> + the port information to Bus which includes port numbers allocated by >> + Master(s) and Slave(s) for current stream and their channel mask. >> >> so how can Master ports be configured if it is added to a stream *after* the >> CONFIGURED state? > > Yes you have a valid point wrt documentation, will update it. It's not just the documentation, I wonder if it actually works. You would want to make sure all masters and slaves are allocated before configuring them, otherwise the error handling flow is just awful to deal with. You could end-up in a situation where all Slaves are configured but the master allocation fails. > >> >>> + >>> + mutex_lock(&bus->bus_lock); >>> + >>> + m_rt = sdw_alloc_master_rt(bus, stream_config, stream); >>> + if (!m_rt) { >>> + dev_err(bus->dev, >>> + "Master runtime config failed for stream:%s", >>> + stream->name); >>> + ret = -ENOMEM; >>> + goto error; >>> + } >>> + >>> + ret = sdw_config_stream(bus->dev, stream, stream_config, false); >>> + if (ret) >>> + goto stream_error; >>> + >>> + if (!list_empty(&m_rt->slave_rt_list) && >>> + stream->state == SDW_STREAM_ALLOCATED) >>> + stream->state = SDW_STREAM_CONFIGURED; >> >> this looks also like new code, what is the idea about this configuration >> business? >> To me this is inconsistent with the documentation which says >> sdw_stream_add_master is called in ALLOCATED state. Under what circumstances >> would this routing be called from two different states? > > Yes it "may" be due to sequencing in callers control. > >> This may be ok but the intent is clear as mud. > > Yes that is a bit unfortunate :( You'll need to really rework this, i don't get why this major change comes in a v2 without the associated collateral explaining the rationale for this change. > >>> +int sdw_stream_add_slave(struct sdw_slave *slave, >>> + struct sdw_stream_config *stream_config, >>> + struct sdw_stream_runtime *stream) >>> +{ >>> + struct sdw_slave_runtime *s_rt; >>> + struct sdw_master_runtime *m_rt; >>> + int ret; >>> + >>> + if (stream->state != SDW_STREAM_ALLOCATED && >>> + stream->state != SDW_STREAM_CONFIGURED) { >>> + dev_err(&slave->dev, >>> + "Invalid stream state %d", stream->state); >>> + return -EINVAL; >>> + } >> >> same as for the master, why would one call this routine from two separate >> states? > > as discussed above > >> >>> + >>> + mutex_lock(&slave->bus->bus_lock); >>> + >>> + /* >>> + * If this API is invoked by slave first then m_rt is not valid. >>> + * So, allocate that and add the slave to it. >>> + */ >>> + m_rt = sdw_alloc_master_rt(slave->bus, stream_config, stream); >>> + if (!m_rt) { >>> + dev_err(&slave->dev, >>> + "alloc master runtime failed for stream:%s", >>> + stream->name); >>> + ret = -ENOMEM; >>> + goto error; >>> + } >>> + >>> + s_rt = sdw_alloc_slave_rt(slave, stream_config, stream); >>> + if (!s_rt) { >>> + dev_err(&slave->dev, >>> + "Slave runtime config failed for stream:%s", >>> + stream->name); >>> + ret = -ENOMEM; >>> + goto stream_error; >>> + } >>> + >>> + ret = sdw_config_stream(&slave->dev, stream, stream_config, true); >>> + if (ret) >>> + goto stream_error; >>> + >>> + list_add_tail(&s_rt->m_rt_node, &m_rt->slave_rt_list); >> >> so you have a potential to add the same node twice from ALLOCATED and >> CONFIGURED states? I am completely lost. > > That would be the case if Slave invokes this routine erroneously twice. We expect this > would be called only once by caller. >
On Fri, Apr 06, 2018 at 10:21:05AM -0500, Pierre-Louis Bossart wrote: > On 4/5/18 11:53 PM, Vinod Koul wrote: > >On Thu, Apr 05, 2018 at 05:34:18PM -0500, Pierre-Louis Bossart wrote: > >>On 4/5/18 11:48 AM, Vinod Koul wrote: > > > >>>+/** > >>>+ * sdw_master_runtime: Runtime stream parameters for Master > >>>+ * > >>>+ * @bus: Bus handle > >>>+ * @stream: Stream runtime handle > >>>+ * @direction: Data direction for Master > >>>+ * @ch_count: Number of channels handled by the Master for > >>>+ * this stream > >> > >>Didn't we say that this count could be zero for device-to-device > >>communication? 'handled by the Master' is not really correct. > > > >Yes for device-to-device this would be zero. And master will handle no > >channels as indicated by zero value. Won't that be right? > > It doesn't hurt to say that zero is an acceptable value by design. ok > >>>+ * @slave_rt_list: Slave runtime list > >>>+ * @bus_node: sdw_bus m_rt_list node > >> > >>I think the intention was to progress in steps, but by omitting references > >>to the multi-master case in this series or comments explaining the next > >>steps the structure and code are not easy to follow. > > > >We cannot have any ref to multi-master as it is not in this series. That is how > >patches would be done, you add a step and then increment with another. > > The point is that the bus_node is only relevant for multi-master, so you > have structure fields are present, not currently used but will be. > It's perfectly ok to progress in steps, but you have to be consistent. If > you keep a structure that will only be used later, you need to tell this to > reviewers. Sorry no it is not the case. bus_node keeps track of master_rt on a bus. This is used in non multi-link case Ex multiple streams over a bus. We need to redo bandwidth calculation etc on a bus when a new stream arrives or existing stream frees up, so a bus will always track the master_rt > >Ofcourse if you have any question feel free to ask and we can clarify, but I > >do not think it would be correct to add comments/reference here for > >multi-master. These will be updated as required when we post multi-master. > > > >>There should be a clear explanation that > >>- a stream can be connected a least one Slave Device with one or more ports. > > > >explained in document patch > > > >>- a stream may be connected to zero or more Master Devices. In the former > >>case one of the Slave devices has provide TX ports. > > > >That's multi-link/device-device not under review here > > zero or more describes the single master case. > > >>- the slave runtime keeps track of all the masters the stream is connected > >>to > > > >not that is not a correct assumption > > yes this is wrong, I meant "the master runtime keeps tracks off all the > Slaves the stream is connected to" > > > >>- the master runtime keeps track of > >>1. all the Slaves it is physically connected to which support a stream. > > > >Yes that is added here > > > >>2. all bus instances (and related masters) that support this stream. > > > >That's again multi-link! > > the statement is still correct with a single instance :-) > > >Again we are not writing a spec but code and comments to explain the code. > >The above is not related to "this" series so we should not add it here. When > >we add device-device support, multi-master support these can be added > > > >We are adding blocks in a giant building, but you are only looking at the > >giant picture but not the block in discussion! > > no, you have keep mentions to multi-link that are questionable. Either you > remove them or you state it's reserved for future use. I have clarified on bus_node above, can you check and tell me if there is anything else? > > >>>+/** > >>>+ * sdw_alloc_stream: Allocate and return stream runtime > >>>+ * > >>>+ * @stream_name: SoundWire stream name > >>>+ * > >>>+ * Allocates a SoundWire stream runtime instance. > >>>+ * sdw_alloc_stream should be called only once per stream > >> > >>You should explain who the caller might be, and probably check if a stream > >>with the same name was not already allocated. > > > >Checking name might be bit iffy as we would need to check all streams. The > >name is used for prints so if caller screws up name then it would be at his > >own peril to get confused between same stream names in logs :) > > You didn't answer to the question: who is supposed to call > sdw_alloc_steam()? the machine driver? The platform driver? the codec > driver? How do you garantee that it's called once Sorry about that, it would by machine or platform but only once. We can't guarantee that as it is caller prerogative but code will not work if someone calls multiple times and they will debug. Btw this assumption is explicitly added in documentation. > >>>+static struct sdw_master_runtime > >>>+*sdw_alloc_master_rt(struct sdw_bus *bus, > >>>+ struct sdw_stream_config *stream_config, > >>>+ struct sdw_stream_runtime *stream) > >>>+{ > >>>+ struct sdw_master_runtime *m_rt; > >>>+ > >>>+ m_rt = stream->m_rt; > >>>+ if (m_rt) > >>>+ goto stream_config; > >> > >>I know there is a reason for this code pattern but I just can't remember it > >>and there is no comment indicated why this is valid. > > > >If m_rt is already allocated we skip the initialization and configure it. > >As we have told you earlier, the slave runtime can be added which triggers > >master runtime to be allocated as well, so we need to check. > > > >We do have such a comment in sdw_stream_add_slave() as you advised. Should > >we duplicate it here too? > > doesn't hurt to add a comment pointing to sdw_stream_add_slave() here as > well, the reviewers read linearly and if the comment is in 200 lines it > doesn't help. ok > >>>+int sdw_stream_add_master(struct sdw_bus *bus, > >>>+ struct sdw_stream_config *stream_config, > >>>+ struct sdw_stream_runtime *stream) > >>>+{ > >>>+ struct sdw_master_runtime *m_rt = NULL; > >>>+ int ret; > >>>+ > >>>+ if (stream->state != SDW_STREAM_ALLOCATED && > >>>+ stream->state != SDW_STREAM_CONFIGURED) { > >>>+ dev_err(bus->dev, > >>>+ "Invalid stream state %d", stream->state); > >>>+ return -EINVAL; > >>>+ } > >> > >>Humm, this check looks like new code but I don't get it. Why would one add a > >>master in either of those two states? Why not ALLOCATED only? > > > >So slave or master can be added to a stream in any order. Only if slave is > >called first we need master_rt to be allocated too. In ideal world we would > >do allocation and then configuration, but due to user calling in any order > >we may do allocation and configuration of slave first followed by > >configuring the master. > > that makes no sense. You have a state diagram that defines a state > transition which precludes this order between allocation and configuration. Okay so we are going to track if master and slaves runtimes are configured by adding a flag in each of them. Once all the components are configured the stream stated will be updated. Does that sound ok? > >>+Configuration state of stream. Operations performed before entering in > >>+this state: > >>+ > >>+ (1) The resources allocated for stream information in > >>SDW_STREAM_ALLOCATED > >>+ state are updated here. This includes stream parameters, Master(s) > >>+ and Slave(s) runtime information associated with current stream. > >>+ > >>+ (2) All the Master(s) and Slave(s) associated with current stream provide > >>+ the port information to Bus which includes port numbers allocated by > >>+ Master(s) and Slave(s) for current stream and their channel mask. > >> > >>so how can Master ports be configured if it is added to a stream *after* the > >>CONFIGURED state? > > > >Yes you have a valid point wrt documentation, will update it. > > It's not just the documentation, I wonder if it actually works. You would > want to make sure all masters and slaves are allocated before configuring > them, otherwise the error handling flow is just awful to deal with. You > could end-up in a situation where all Slaves are configured but the master > allocation fails. Ok > >>>+ > >>>+ m_rt = sdw_alloc_master_rt(bus, stream_config, stream); > >>>+ if (!m_rt) { > >>>+ dev_err(bus->dev, > >>>+ "Master runtime config failed for stream:%s", > >>>+ stream->name); > >>>+ ret = -ENOMEM; > >>>+ goto error; > >>>+ } > >>>+ > >>>+ ret = sdw_config_stream(bus->dev, stream, stream_config, false); > >>>+ if (ret) > >>>+ goto stream_error; > >>>+ > >>>+ if (!list_empty(&m_rt->slave_rt_list) && > >>>+ stream->state == SDW_STREAM_ALLOCATED) > >>>+ stream->state = SDW_STREAM_CONFIGURED; > >> > >>this looks also like new code, what is the idea about this configuration > >>business? > >>To me this is inconsistent with the documentation which says > >>sdw_stream_add_master is called in ALLOCATED state. Under what circumstances > >>would this routing be called from two different states? > > > >Yes it "may" be due to sequencing in callers control. > > > >>This may be ok but the intent is clear as mud. > > > >Yes that is a bit unfortunate :( > > You'll need to really rework this, i don't get why this major change comes > in a v2 without the associated collateral explaining the rationale for this > change. Btw this change was not introduced in v2, it was there in v1 too
>>>>> + * @slave_rt_list: Slave runtime list >>>>> + * @bus_node: sdw_bus m_rt_list node >>>> >>>> I think the intention was to progress in steps, but by omitting references >>>> to the multi-master case in this series or comments explaining the next >>>> steps the structure and code are not easy to follow. >>> >>> We cannot have any ref to multi-master as it is not in this series. That is how >>> patches would be done, you add a step and then increment with another. >> >> The point is that the bus_node is only relevant for multi-master, so you >> have structure fields are present, not currently used but will be. >> It's perfectly ok to progress in steps, but you have to be consistent. If >> you keep a structure that will only be used later, you need to tell this to >> reviewers. > > Sorry no it is not the case. bus_node keeps track of master_rt on a bus. > This is used in non multi-link case Ex multiple streams over a bus. > We need to redo bandwidth calculation etc on a bus when a new stream > arrives or existing stream frees up, so a bus will always track > the master_rt I stand corrected, thanks. > >>> Ofcourse if you have any question feel free to ask and we can clarify, but I >>> do not think it would be correct to add comments/reference here for >>> multi-master. These will be updated as required when we post multi-master. >>> >>>> There should be a clear explanation that >>>> - a stream can be connected a least one Slave Device with one or more ports. >>> >>> explained in document patch >>> >>>> - a stream may be connected to zero or more Master Devices. In the former >>>> case one of the Slave devices has provide TX ports. >>> >>> That's multi-link/device-device not under review here >> >> zero or more describes the single master case. >> >>>> - the slave runtime keeps track of all the masters the stream is connected >>>> to >>> >>> not that is not a correct assumption >> >> yes this is wrong, I meant "the master runtime keeps tracks off all the >> Slaves the stream is connected to" >>> >>>> - the master runtime keeps track of >>>> 1. all the Slaves it is physically connected to which support a stream. >>> >>> Yes that is added here >>> >>>> 2. all bus instances (and related masters) that support this stream. >>> >>> That's again multi-link! >> >> the statement is still correct with a single instance :-) >> >>> Again we are not writing a spec but code and comments to explain the code. >>> The above is not related to "this" series so we should not add it here. When >>> we add device-device support, multi-master support these can be added >>> >>> We are adding blocks in a giant building, but you are only looking at the >>> giant picture but not the block in discussion! >> >> no, you have keep mentions to multi-link that are questionable. Either you >> remove them or you state it's reserved for future use. > > I have clarified on bus_node above, can you check and tell me if there is > anything else? The code pattern for the bank switch was inspired by multi-link. >> >>>>> +/** >>>>> + * sdw_alloc_stream: Allocate and return stream runtime >>>>> + * >>>>> + * @stream_name: SoundWire stream name >>>>> + * >>>>> + * Allocates a SoundWire stream runtime instance. >>>>> + * sdw_alloc_stream should be called only once per stream >>>> >>>> You should explain who the caller might be, and probably check if a stream >>>> with the same name was not already allocated. >>> >>> Checking name might be bit iffy as we would need to check all streams. The >>> name is used for prints so if caller screws up name then it would be at his >>> own peril to get confused between same stream names in logs :) >> >> You didn't answer to the question: who is supposed to call >> sdw_alloc_steam()? the machine driver? The platform driver? the codec >> driver? How do you garantee that it's called once > > Sorry about that, it would by machine or platform but only once. > We can't guarantee that as it is caller prerogative but code will not work if > someone calls multiple times and they will debug. > > Btw this assumption is explicitly added in documentation. I don't get how this would work. To me the platform drivers and codec drivers expose DAIs, and the machine drivers connect them. The stream is an additional level that the machine driver should know. I just don't see how a platform driver would handle this? >>>>> +int sdw_stream_add_master(struct sdw_bus *bus, >>>>> + struct sdw_stream_config *stream_config, >>>>> + struct sdw_stream_runtime *stream) >>>>> +{ >>>>> + struct sdw_master_runtime *m_rt = NULL; >>>>> + int ret; >>>>> + >>>>> + if (stream->state != SDW_STREAM_ALLOCATED && >>>>> + stream->state != SDW_STREAM_CONFIGURED) { >>>>> + dev_err(bus->dev, >>>>> + "Invalid stream state %d", stream->state); >>>>> + return -EINVAL; >>>>> + } >>>> >>>> Humm, this check looks like new code but I don't get it. Why would one add a >>>> master in either of those two states? Why not ALLOCATED only? >>> >>> So slave or master can be added to a stream in any order. Only if slave is >>> called first we need master_rt to be allocated too. In ideal world we would >>> do allocation and then configuration, but due to user calling in any order >>> we may do allocation and configuration of slave first followed by >>> configuring the master. >> >> that makes no sense. You have a state diagram that defines a state >> transition which precludes this order between allocation and configuration. > > Okay so we are going to track if master and slaves runtimes are configured > by adding a flag in each of them. Once all the components are configured the > stream stated will be updated. Does that sound ok? How do you detect that all components are configured? We have a distributed allocation/configuration with multiple actors, I don't what triggers the transition to a new state. see more below. > >>>> +Configuration state of stream. Operations performed before entering in >>>> +this state: >>>> + >>>> + (1) The resources allocated for stream information in >>>> SDW_STREAM_ALLOCATED >>>> + state are updated here. This includes stream parameters, Master(s) >>>> + and Slave(s) runtime information associated with current stream. >>>> + >>>> + (2) All the Master(s) and Slave(s) associated with current stream provide >>>> + the port information to Bus which includes port numbers allocated by >>>> + Master(s) and Slave(s) for current stream and their channel mask. >>>> >>>> so how can Master ports be configured if it is added to a stream *after* the >>>> CONFIGURED state? >>> >>> Yes you have a valid point wrt documentation, will update it. >> >> It's not just the documentation, I wonder if it actually works. You would >> want to make sure all masters and slaves are allocated before configuring >> them, otherwise the error handling flow is just awful to deal with. You >> could end-up in a situation where all Slaves are configured but the master >> allocation fails. > > Ok If the ALLOCATED and CONFIGURED states are clearly separated out, then what if the flag you mentioned used for? >>>>> + >>>>> + m_rt = sdw_alloc_master_rt(bus, stream_config, stream); >>>>> + if (!m_rt) { >>>>> + dev_err(bus->dev, >>>>> + "Master runtime config failed for stream:%s", >>>>> + stream->name); >>>>> + ret = -ENOMEM; >>>>> + goto error; >>>>> + } >>>>> + >>>>> + ret = sdw_config_stream(bus->dev, stream, stream_config, false); >>>>> + if (ret) >>>>> + goto stream_error; >>>>> + >>>>> + if (!list_empty(&m_rt->slave_rt_list) && >>>>> + stream->state == SDW_STREAM_ALLOCATED) >>>>> + stream->state = SDW_STREAM_CONFIGURED; >>>> >>>> this looks also like new code, what is the idea about this configuration >>>> business? >>>> To me this is inconsistent with the documentation which says >>>> sdw_stream_add_master is called in ALLOCATED state. Under what circumstances >>>> would this routing be called from two different states? >>> >>> Yes it "may" be due to sequencing in callers control. >>> >>>> This may be ok but the intent is clear as mud. >>> >>> Yes that is a bit unfortunate :( >> >> You'll need to really rework this, i don't get why this major change comes >> in a v2 without the associated collateral explaining the rationale for this >> change. > > Btw this change was not introduced in v2, it was there in v1 too It wasn't there in the initial patches shared with me in February. There were too many missing parts in v1 that I stopped the review.
On Tue, Apr 10, 2018 at 10:47:24AM -0500, Pierre-Louis Bossart wrote: > >>>Again we are not writing a spec but code and comments to explain the code. > >>>The above is not related to "this" series so we should not add it here. When > >>>we add device-device support, multi-master support these can be added > >>> > >>>We are adding blocks in a giant building, but you are only looking at the > >>>giant picture but not the block in discussion! > >> > >>no, you have keep mentions to multi-link that are questionable. Either you > >>remove them or you state it's reserved for future use. > > > >I have clarified on bus_node above, can you check and tell me if there is > >anything else? > > The code pattern for the bank switch was inspired by multi-link. As you pointed out, we are fixing that too > >Okay so we are going to track if master and slaves runtimes are configured > >by adding a flag in each of them. Once all the components are configured the > >stream stated will be updated. Does that sound ok? > > How do you detect that all components are configured? We have a distributed > allocation/configuration with multiple actors, I don't what triggers the > transition to a new state. > see more below. > > > > >>>>+Configuration state of stream. Operations performed before entering in > >>>>+this state: > >>>>+ > >>>>+ (1) The resources allocated for stream information in > >>>>SDW_STREAM_ALLOCATED > >>>>+ state are updated here. This includes stream parameters, Master(s) > >>>>+ and Slave(s) runtime information associated with current stream. > >>>>+ > >>>>+ (2) All the Master(s) and Slave(s) associated with current stream provide > >>>>+ the port information to Bus which includes port numbers allocated by > >>>>+ Master(s) and Slave(s) for current stream and their channel mask. > >>>> > >>>>so how can Master ports be configured if it is added to a stream *after* the > >>>>CONFIGURED state? > >>> > >>>Yes you have a valid point wrt documentation, will update it. > >> > >>It's not just the documentation, I wonder if it actually works. You would > >>want to make sure all masters and slaves are allocated before configuring > >>them, otherwise the error handling flow is just awful to deal with. You > >>could end-up in a situation where all Slaves are configured but the master > >>allocation fails. > > > >Ok > > If the ALLOCATED and CONFIGURED states are clearly separated out, then what > if the flag you mentioned used for? As you rightly pointed out that we have distributed allocation/configuration with multiple actors. So to ensure that all actors are properly configured before we move stream state to CONFIGURED, we need to check and to do so the flag will be very useful :-)
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile index e1a74c5692aa..5817beaca0e1 100644 --- a/drivers/soundwire/Makefile +++ b/drivers/soundwire/Makefile @@ -3,7 +3,7 @@ # #Bus Objs -soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o +soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o #Cadence Objs diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index d6dc8e7a8614..abf046f6b188 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -32,6 +32,7 @@ int sdw_add_bus_master(struct sdw_bus *bus) mutex_init(&bus->msg_lock); mutex_init(&bus->bus_lock); INIT_LIST_HEAD(&bus->slaves); + INIT_LIST_HEAD(&bus->m_rt_list); if (bus->ops->read_prop) { ret = bus->ops->read_prop(bus); diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h index 345c34d697e9..3c66b6aecc14 100644 --- a/drivers/soundwire/bus.h +++ b/drivers/soundwire/bus.h @@ -45,6 +45,42 @@ struct sdw_msg { bool page; }; +/** + * sdw_slave_runtime: Runtime Stream parameters for Slave + * + * @slave: Slave handle + * @direction: Data direction for Slave + * @ch_count: Number of channels handled by the Slave for + * this stream + * @m_rt_node: sdw_master_runtime list node + */ +struct sdw_slave_runtime { + struct sdw_slave *slave; + enum sdw_data_direction direction; + unsigned int ch_count; + struct list_head m_rt_node; +}; + +/** + * sdw_master_runtime: Runtime stream parameters for Master + * + * @bus: Bus handle + * @stream: Stream runtime handle + * @direction: Data direction for Master + * @ch_count: Number of channels handled by the Master for + * this stream + * @slave_rt_list: Slave runtime list + * @bus_node: sdw_bus m_rt_list node + */ +struct sdw_master_runtime { + struct sdw_bus *bus; + struct sdw_stream_runtime *stream; + enum sdw_data_direction direction; + unsigned int ch_count; + struct list_head slave_rt_list; + struct list_head bus_node; +}; + int sdw_transfer(struct sdw_bus *bus, struct sdw_msg *msg); int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg, struct sdw_defer *defer); diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c new file mode 100644 index 000000000000..5c34177e4954 --- /dev/null +++ b/drivers/soundwire/stream.c @@ -0,0 +1,354 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +// Copyright(c) 2015-18 Intel Corporation. + +/* + * stream.c - SoundWire Bus stream operations. + */ + +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/slab.h> +#include <linux/soundwire/sdw.h> +#include "bus.h" + +/** + * sdw_release_stream: Free the assigned stream runtime + * + * @stream: SoundWire stream runtime + * + * sdw_release_stream should be called only once per stream + */ +void sdw_release_stream(struct sdw_stream_runtime *stream) +{ + kfree(stream); +} +EXPORT_SYMBOL(sdw_release_stream); + +/** + * sdw_alloc_stream: Allocate and return stream runtime + * + * @stream_name: SoundWire stream name + * + * Allocates a SoundWire stream runtime instance. + * sdw_alloc_stream should be called only once per stream + */ +struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name) +{ + struct sdw_stream_runtime *stream; + + stream = kzalloc(sizeof(*stream), GFP_KERNEL); + if (!stream) + return NULL; + + stream->name = stream_name; + stream->state = SDW_STREAM_ALLOCATED; + + return stream; +} +EXPORT_SYMBOL(sdw_alloc_stream); + +/** + * sdw_alloc_master_rt: Allocates and initialize Master runtime handle + * + * @bus: SDW bus instance + * @stream_config: Stream configuration + * @stream: Stream runtime handle. + * + * This function is to be called with bus_lock held. + */ +static struct sdw_master_runtime +*sdw_alloc_master_rt(struct sdw_bus *bus, + struct sdw_stream_config *stream_config, + struct sdw_stream_runtime *stream) +{ + struct sdw_master_runtime *m_rt; + + m_rt = stream->m_rt; + if (m_rt) + goto stream_config; + + m_rt = kzalloc(sizeof(*m_rt), GFP_KERNEL); + if (!m_rt) + return NULL; + + /* Initialization of Master runtime handle */ + INIT_LIST_HEAD(&m_rt->slave_rt_list); + stream->m_rt = m_rt; + + list_add_tail(&m_rt->bus_node, &bus->m_rt_list); + +stream_config: + m_rt->ch_count = stream_config->ch_count; + m_rt->bus = bus; + m_rt->stream = stream; + m_rt->direction = stream_config->direction; + + return m_rt; +} + +/** + * sdw_alloc_slave_rt: Allocate and initialize Slave runtime handle. + * + * @slave: Slave handle + * @stream_config: Stream configuration + * @stream: Stream runtime handle + * + * This function is to be called with bus_lock held. + */ +static struct sdw_slave_runtime +*sdw_alloc_slave_rt(struct sdw_slave *slave, + struct sdw_stream_config *stream_config, + struct sdw_stream_runtime *stream) +{ + struct sdw_slave_runtime *s_rt = NULL; + + s_rt = kzalloc(sizeof(*s_rt), GFP_KERNEL); + if (!s_rt) + return NULL; + + + s_rt->ch_count = stream_config->ch_count; + s_rt->direction = stream_config->direction; + s_rt->slave = slave; + + return s_rt; +} + +/** + * sdw_release_slave_stream: Free Slave(s) runtime handle + * + * @slave: Slave handle. + * @stream: Stream runtime handle. + * + * This function is to be called with bus_lock held. + */ +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) { + + if (s_rt->slave == slave) { + list_del(&s_rt->m_rt_node); + kfree(s_rt); + return; + } + } +} + +static void sdw_release_master_stream(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_release_slave_stream(s_rt->slave, stream); + + list_del(&m_rt->bus_node); +} + +/** + * sdw_stream_remove_master: Remove master from sdw_stream + * + * @bus: SDW Bus instance + * @stream: Soundwire stream + * + * This removes and frees master_rt from a stream + */ +int sdw_stream_remove_master(struct sdw_bus *bus, + struct sdw_stream_runtime *stream) +{ + mutex_lock(&bus->bus_lock); + + sdw_release_master_stream(stream); + stream->state = SDW_STREAM_RELEASED; + kfree(stream->m_rt); + stream->m_rt = NULL; + + mutex_unlock(&bus->bus_lock); + + return 0; +} +EXPORT_SYMBOL(sdw_stream_remove_master); + +/** + * sdw_stream_remove_slave: Remove slave from sdw_stream + * + * @slave: SDW Slave instance + * @stream: Soundwire stream + * + * This removes and frees slave_rt from a stream + */ +int sdw_stream_remove_slave(struct sdw_slave *slave, + struct sdw_stream_runtime *stream) +{ + mutex_lock(&slave->bus->bus_lock); + + sdw_release_slave_stream(slave, stream); + + mutex_unlock(&slave->bus->bus_lock); + + return 0; +} +EXPORT_SYMBOL(sdw_stream_remove_slave); + +/** + * sdw_config_stream: Configure the allocated stream + * + * @dev: SDW device + * @stream: Soundwire stream + * @stream_config: Stream configuration for audio stream + * @is_slave: is API called from Slave or Master + * + * This function is to be called with bus_lock held. + */ +static int sdw_config_stream(struct device *dev, + struct sdw_stream_runtime *stream, + struct sdw_stream_config *stream_config, bool is_slave) +{ + + /* + * Update the stream rate, channel and bps based on data + * source. For more than one data source (multilink), + * match the rate, bps, stream type and increment number of channels. + */ + if ((stream->params.rate) && + (stream->params.rate != stream_config->frame_rate)) { + dev_err(dev, "rate not matching, stream:%s", stream->name); + return -EINVAL; + } + + if ((stream->params.bps) && + (stream->params.bps != stream_config->bps)) { + dev_err(dev, "bps not matching, stream:%s", stream->name); + return -EINVAL; + } + + stream->type = stream_config->type; + stream->params.rate = stream_config->frame_rate; + stream->params.bps = stream_config->bps; + if (is_slave) + stream->params.ch_count += stream_config->ch_count; + + return 0; +} + +/** + * sdw_stream_add_master: Allocate and add master runtime to a stream + * + * @bus: SDW Bus instance + * @stream_config: Stream configuration for audio stream + * @stream: Soundwire stream + */ +int sdw_stream_add_master(struct sdw_bus *bus, + struct sdw_stream_config *stream_config, + struct sdw_stream_runtime *stream) +{ + struct sdw_master_runtime *m_rt = NULL; + int ret; + + if (stream->state != SDW_STREAM_ALLOCATED && + stream->state != SDW_STREAM_CONFIGURED) { + dev_err(bus->dev, + "Invalid stream state %d", stream->state); + return -EINVAL; + } + + mutex_lock(&bus->bus_lock); + + m_rt = sdw_alloc_master_rt(bus, stream_config, stream); + if (!m_rt) { + dev_err(bus->dev, + "Master runtime config failed for stream:%s", + stream->name); + ret = -ENOMEM; + goto error; + } + + ret = sdw_config_stream(bus->dev, stream, stream_config, false); + if (ret) + goto stream_error; + + if (!list_empty(&m_rt->slave_rt_list) && + stream->state == SDW_STREAM_ALLOCATED) + stream->state = SDW_STREAM_CONFIGURED; + +stream_error: + sdw_release_master_stream(stream); +error: + mutex_unlock(&bus->bus_lock); + return ret; + +} +EXPORT_SYMBOL(sdw_stream_add_master); + +/** + * sdw_stream_add_slave: Allocate and add master/slave runtime to a stream + * + * @slave: SDW Slave instance + * @stream_config: Stream configuration for audio stream + * @stream: Soundwire stream + */ +int sdw_stream_add_slave(struct sdw_slave *slave, + struct sdw_stream_config *stream_config, + struct sdw_stream_runtime *stream) +{ + struct sdw_slave_runtime *s_rt; + struct sdw_master_runtime *m_rt; + int ret; + + if (stream->state != SDW_STREAM_ALLOCATED && + stream->state != SDW_STREAM_CONFIGURED) { + dev_err(&slave->dev, + "Invalid stream state %d", stream->state); + return -EINVAL; + } + + mutex_lock(&slave->bus->bus_lock); + + /* + * If this API is invoked by slave first then m_rt is not valid. + * So, allocate that and add the slave to it. + */ + m_rt = sdw_alloc_master_rt(slave->bus, stream_config, stream); + if (!m_rt) { + dev_err(&slave->dev, + "alloc master runtime failed for stream:%s", + stream->name); + ret = -ENOMEM; + goto error; + } + + s_rt = sdw_alloc_slave_rt(slave, stream_config, stream); + if (!s_rt) { + dev_err(&slave->dev, + "Slave runtime config failed for stream:%s", + stream->name); + ret = -ENOMEM; + goto stream_error; + } + + ret = sdw_config_stream(&slave->dev, stream, stream_config, true); + if (ret) + goto stream_error; + + list_add_tail(&s_rt->m_rt_node, &m_rt->slave_rt_list); + + stream->state = SDW_STREAM_CONFIGURED; + goto error; + +stream_error: + sdw_release_master_stream(stream); +error: + mutex_unlock(&slave->bus->bus_lock); + return ret; +} +EXPORT_SYMBOL(sdw_stream_add_slave); diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index e91fdcf41049..893e1b6b4914 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -61,6 +61,30 @@ enum sdw_command_response { SDW_CMD_FAIL_OTHER = 4, }; +/** + * enum sdw_stream_type: data stream type + * + * @SDW_STREAM_PCM: PCM data stream + * @SDW_STREAM_PDM: PDM data stream + * + * spec doesn't define this, but is used in implementation + */ +enum sdw_stream_type { + SDW_STREAM_PCM = 0, + SDW_STREAM_PDM = 1, +}; + +/** + * enum sdw_data_direction: Data direction + * + * @SDW_DATA_DIR_RX: Data into Port + * @SDW_DATA_DIR_TX: Data out of Port + */ +enum sdw_data_direction { + SDW_DATA_DIR_RX = 0, + SDW_DATA_DIR_TX = 1, +}; + /* * SDW properties, defined in MIPI DisCo spec v1.0 */ @@ -450,6 +474,9 @@ struct sdw_master_ops { * @msg_lock: message lock * @ops: Master callback ops * @prop: Master properties + * @m_rt_list: List of Master instance of all stream(s) running on Bus. This + * is used to compute and program bus bandwidth, clock, frame shape, + * transport and port parameters * @defer_msg: Defer message * @clk_stop_timeout: Clock stop timeout computed */ @@ -462,6 +489,7 @@ struct sdw_bus { struct mutex msg_lock; const struct sdw_master_ops *ops; struct sdw_master_prop prop; + struct list_head m_rt_list; struct sdw_defer defer_msg; unsigned int clk_stop_timeout; }; @@ -469,6 +497,87 @@ struct sdw_bus { int sdw_add_bus_master(struct sdw_bus *bus); void sdw_delete_bus_master(struct sdw_bus *bus); +/** + * sdw_stream_config: Master or Slave stream configuration + * + * @frame_rate: Audio frame rate of the stream, in Hz + * @ch_count: Channel count of the stream + * @bps: Number of bits per audio sample + * @direction: Data direction + * @type: Stream type PCM or PDM + */ +struct sdw_stream_config { + unsigned int frame_rate; + unsigned int ch_count; + unsigned int bps; + enum sdw_data_direction direction; + enum sdw_stream_type type; +}; + +/** + * sdw_stream_state: Stream states + * + * @SDW_STREAM_RELEASED: Stream released + * @SDW_STREAM_ALLOCATED: New stream allocated. + * @SDW_STREAM_CONFIGURED: Stream configured + * @SDW_STREAM_PREPARED: Stream prepared + * @SDW_STREAM_ENABLED: Stream enabled + * @SDW_STREAM_DISABLED: Stream disabled + * @SDW_STREAM_DEPREPARED: Stream de-prepared + */ +enum sdw_stream_state { + SDW_STREAM_RELEASED = 0, + SDW_STREAM_ALLOCATED = 1, + SDW_STREAM_CONFIGURED = 2, + SDW_STREAM_PREPARED = 3, + SDW_STREAM_ENABLED = 4, + SDW_STREAM_DISABLED = 5, + SDW_STREAM_DEPREPARED = 6, +}; + +/** + * sdw_stream_params: Stream parameters + * + * @rate: Sampling frequency, in Hz + * @ch_count: Number of channels + * @bps: bits per channel sample + */ +struct sdw_stream_params { + unsigned int rate; + unsigned int ch_count; + unsigned int bps; +}; + +/** + * sdw_stream_runtime: Runtime stream parameters + * + * @name: SoundWire stream name + * @params: Stream parameters + * @state: Current state of the stream + * @type: Stream type PCM or PDM + * @m_rt: Master runtime + */ +struct sdw_stream_runtime { + char *name; + struct sdw_stream_params params; + enum sdw_stream_state state; + enum sdw_stream_type type; + struct sdw_master_runtime *m_rt; +}; + +struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name); +void sdw_release_stream(struct sdw_stream_runtime *stream); +int sdw_stream_add_master(struct sdw_bus *bus, + struct sdw_stream_config *stream_config, + struct sdw_stream_runtime *stream); +int sdw_stream_add_slave(struct sdw_slave *slave, + struct sdw_stream_config *stream_config, + struct sdw_stream_runtime *stream); +int sdw_stream_remove_master(struct sdw_bus *bus, + struct sdw_stream_runtime *stream); +int sdw_stream_remove_slave(struct sdw_slave *slave, + struct sdw_stream_runtime *stream); + /* messaging and data APIs */ int sdw_read(struct sdw_slave *slave, u32 addr);