Re: [RFC PATCH 07/16] soundwire: bus: add API for BPT protocol

[Vinod Koul <vkoul@xxxxxxxxxx>]

On 07-12-23, 16:29, Pierre-Louis Bossart wrote:
> Add definitions and helpers for the BPT/BRA protocol. Peripheral
> drivers (aka ASoC codec drivers) can use this API to send bulk data
> such as firmware or tables.
> 
> The API is only available when no other audio streams have been
> allocated, and only one BTP/BRA stream is allowed per link.  To avoid
> the addition of yet another lock, the refcount tests are handled in
> the stream master_runtime alloc/free routines where the bus_lock is
> already held. Another benefit of this approach is that the same
> bus_lock is used to handle runtime and port linked lists, which
> reduces the potential for misaligned configurations.
> 
> In addition to exclusion with audio streams, BPT transfers have a lot
> of overhead, specifically registers writes are needed to enable
> transport in DP0. In addition, most DMAs don't handle too well very
> small data sets.
> 
> This patch suggests a minimum bound of 64 bytes, for smaller transfers
> codec drivers should rely on the regular read/write commands in
> Column0.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> ---
>  drivers/soundwire/bus.c       | 77 +++++++++++++++++++++++++++++++++++
>  drivers/soundwire/bus.h       | 18 ++++++++
>  drivers/soundwire/stream.c    | 30 ++++++++++++++
>  include/linux/soundwire/sdw.h | 76 ++++++++++++++++++++++++++++++++++
>  4 files changed, 201 insertions(+)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index f3fec15c3112..e5758d2ed88f 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -2015,3 +2015,80 @@ void sdw_clear_slave_status(struct sdw_bus *bus, u32 request)
>  	}
>  }
>  EXPORT_SYMBOL(sdw_clear_slave_status);
> +
> +int sdw_bpt_close_stream(struct sdw_bus *bus,
> +			 struct sdw_slave *slave,
> +			 enum sdw_bpt_type mode,
> +			 struct sdw_bpt_msg *msg)
> +{
> +	int ret;
> +
> +	ret = bus->ops->bpt_close_stream(bus, slave, mode, msg);
> +	if (ret < 0)
> +		dev_err(bus->dev, "BPT stream close, err %d\n", ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(sdw_bpt_close_stream);
> +
> +int sdw_bpt_open_stream(struct sdw_bus *bus,
> +			struct sdw_slave *slave,
> +			enum sdw_bpt_type mode,
> +			struct sdw_bpt_msg *msg)
> +{
> +	int ret;
> +
> +	/* only Bulk Register Access (BRA) is supported for now */
> +	if (mode != SDW_BRA)
> +		return -EINVAL;
> +
> +	if (msg->len < SDW_BPT_MSG_MIN_BYTES) {
> +		dev_err(bus->dev, "BPT message length %d, min supported %d\n",
> +			msg->len, SDW_BPT_MSG_MIN_BYTES);
> +		return -EINVAL;
> +	}
> +
> +	if (msg->len % SDW_BPT_MSG_BYTE_ALIGNMENT) {
> +		dev_err(bus->dev, "BPT message length %d is not a multiple of %d bytes\n",
> +			msg->len, SDW_BPT_MSG_BYTE_ALIGNMENT);
> +		return -EINVAL;
> +	}

Is this a protocol requirement?

> +
> +	/* check device is enumerated */
> +	if (slave->dev_num == SDW_ENUM_DEV_NUM ||
> +	    slave->dev_num > SDW_MAX_DEVICES)
> +		return -ENODEV;
> +
> +	/* make sure all callbacks are defined */
> +	if (!bus->ops->bpt_open_stream ||
> +	    !bus->ops->bpt_close_stream ||
> +	    !bus->ops->bpt_send_async ||
> +	    !bus->ops->bpt_wait)
> +		return -ENOTSUPP;

should this not be checked at probe time, if device declares the support

> +
> +	ret = bus->ops->bpt_open_stream(bus, slave, mode, msg);
> +	if (ret < 0)
> +		dev_err(bus->dev, "BPT stream open, err %d\n", ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(sdw_bpt_open_stream);

can we open multiple times (i dont see a check preventing that), how do
we close..?

Re-iterating my comment on documentation patch, can we do with a async api
and wait api, that makes symantics a lot simpler, right..?

> +
> +int sdw_bpt_send_async(struct sdw_bus *bus,
> +		       struct sdw_slave *slave,
> +		       struct sdw_bpt_msg *msg)
> +{
> +	if (msg->len > SDW_BPT_MSG_MAX_BYTES)
> +		return -EINVAL;
> +
> +	return bus->ops->bpt_send_async(bus, slave, msg);
> +}
> +EXPORT_SYMBOL(sdw_bpt_send_async);

Can we call this multiple times after open, it is unclear to me. Can you
please add kernel-doc comments about the APIs here as well

>  struct sdw_master_ops {
>  	int (*read_prop)(struct sdw_bus *bus);
> @@ -869,6 +913,20 @@ struct sdw_master_ops {
>  	void (*new_peripheral_assigned)(struct sdw_bus *bus,
>  					struct sdw_slave *slave,
>  					int dev_num);
> +	int (*bpt_open_stream)(struct sdw_bus *bus,
> +			       struct sdw_slave *slave,
> +			       enum sdw_bpt_type mode,
> +			       struct sdw_bpt_msg *msg);
> +	int (*bpt_close_stream)(struct sdw_bus *bus,
> +				struct sdw_slave *slave,
> +				enum sdw_bpt_type mode,
> +				struct sdw_bpt_msg *msg);
> +	int (*bpt_send_async)(struct sdw_bus *bus,
> +			      struct sdw_slave *slave,
> +			      struct sdw_bpt_msg *msg);
> +	int (*bpt_wait)(struct sdw_bus *bus,
> +			struct sdw_slave *slave,
> +			struct sdw_bpt_msg *msg);

do we need both bus and slave, that was a mistake in orignal design IMO.
We should fix that for bpt_ apis

-- 
~Vinod