[alsa-devel] [PATCH v2 02/13] soundwire: Add support for SoundWire stream management

Pierre-Louis Bossart pierre-louis.bossart at linux.intel.com
Fri Apr 6 00:34:18 CEST 2018


On 4/5/18 11:48 AM, Vinod Koul wrote:
> From: Sanyog Kale <sanyog.r.kale at 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 at intel.com>
> Signed-off-by: Sanyog Kale <sanyog.r.kale at intel.com>
> Signed-off-by: Shreyas NC <shreyas.nc at intel.com>
> Signed-off-by: Vinod Koul <vinod.koul at 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);
> 



More information about the Alsa-devel mailing list