Date:   Tue, 10 Oct 2017 13:19:38 +0100
From:   Charles Keepax <ckeepax@...nsource.cirrus.com>
To:     <srinivas.kandagatla@...aro.org>
CC:     <gregkh@...uxfoundation.org>, <broonie@...nel.org>,
        <alsa-devel@...a-project.org>, <mark.rutland@....com>,
        <michael.opdenacker@...e-electrons.com>, <poeschel@...onage.de>,
        <andreas.noever@...il.com>, <gong.chen@...ux.intel.com>,
        <arnd@...db.de>, <kheitke@...ience.com>, <bp@...e.de>,
        <devicetree@...r.kernel.org>, <james.hogan@...tec.com>,
        <pawel.moll@....com>, <linux-arm-msm@...r.kernel.org>,
        <sharon.dvir1@...l.huji.ac.il>, <robh+dt@...nel.org>,
        <sdharia@...eaurora.org>, <alan@...ux.intel.com>,
        <treding@...dia.com>, <mathieu.poirier@...aro.org>,
        <jkosina@...e.cz>, <linux-kernel@...r.kernel.org>,
        <daniel@...ll.ch>, <joe@...ches.com>, <davem@...emloft.net>
Subject: Re: [alsa-devel] [Patch v6 2/7] slimbus: Add messaging APIs to
 slimbus framework

On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@...aro.org wrote:
> From: Sagar Dharia <sdharia@...eaurora.org>
> 
> Slimbus devices use value-element, and information elements to
> control device parameters (e.g. value element is used to represent
> gain for codec, information element is used to represent interrupt
> status for codec when codec interrupt fires).
> Messaging APIs are used to set/get these value and information
> elements. Slimbus specification uses 8-bit "transaction IDs" for
> messages where a read-value is anticipated. Framework uses a table
> of pointers to store those TIDs and responds back to the caller in
> O(1).
> Caller can opt to do synchronous, or asynchronous reads/writes. For
> asynchronous operations, the callback will be called from atomic
> context.
> TX and RX circular rings are used to allow queuing of multiple
> transfers per controller. Controller can choose size of these rings
> based of controller HW implementation. The buffers are coerently
> mapped so that controller can utilize DMA operations for the
> transactions without remapping every transaction buffer.
> Statically allocated rings help to improve performance by avoiding
> overhead of dynamically allocating transactions on need basis.
> 
> Signed-off-by: Sagar Dharia <sdharia@...eaurora.org>
> Tested-by: Naveen Kaje <nkaje@...eaurora.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> ---
>  drivers/slimbus/Makefile         |   2 +-
>  drivers/slimbus/slim-core.c      |  15 ++
>  drivers/slimbus/slim-messaging.c | 471 +++++++++++++++++++++++++++++++++++++++
>  include/linux/slimbus.h          | 161 +++++++++++++
>  4 files changed, 648 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/slimbus/slim-messaging.c
> 
> +/**
> + * slim_processtxn: Process a slimbus-messaging transaction
> + * @ctrl: Controller handle
> + * @txn: Transaction to be sent over SLIMbus
> + * Called by controller to transmit messaging transactions not dealing with
> + * Interface/Value elements. (e.g. transmittting a message to assign logical
> + * address to a slave device
> + * Returns:
> + * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
> + *	not being clocked or driven by controller)
> + * -ENOTCONN: If the transmitted message was not ACKed by destination device.
> + */
> +int slim_processtxn(struct slim_controller *ctrl,
> +				struct slim_msg_txn *txn)

Can all go on one line.

> +{
> +	int ret, i = 0;
> +	unsigned long flags;
> +	u8 *buf;
> +	bool async = false;
> +	struct slim_cb_data cbd;
> +	DECLARE_COMPLETION_ONSTACK(done);
> +	bool need_tid = slim_tid_txn(txn->mt, txn->mc);
> +
> +	if (!txn->msg->comp_cb) {
> +		txn->msg->comp_cb = slim_sync_default_cb;
> +		cbd.comp = &done;
> +		txn->msg->ctx = &cbd;
> +	} else {
> +		async = true;
> +	}
> +
> +	buf = slim_get_tx(ctrl, txn, need_tid);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (need_tid) {
> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
> +		for (i = 0; i < ctrl->last_tid; i++) {
> +			if (ctrl->tid_tbl[i] == NULL)
> +				break;
> +		}
> +		if (i >= ctrl->last_tid) {
> +			if (ctrl->last_tid == (SLIM_MAX_TIDS - 1)) {
> +				spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +				slim_return_tx(ctrl, -ENOMEM);
> +				return -ENOMEM;
> +			}
> +			ctrl->last_tid++;
> +		}
> +		ctrl->tid_tbl[i] = txn->msg;
> +		txn->tid = i;
> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +	}
> +
> +	ret = ctrl->xfer_msg(ctrl, txn, buf);
> +
> +	if (!ret && !async) { /* sync transaction */
> +		/* Fine-tune calculation after bandwidth management */
> +		unsigned long ms = txn->rl + 100;
> +
> +		ret = wait_for_completion_timeout(&done,
> +						  msecs_to_jiffies(ms));
> +		if (!ret)
> +			slim_return_tx(ctrl, -ETIMEDOUT);
> +
> +		ret = cbd.ret;
> +	}
> +
> +	if (ret && need_tid) {
> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
> +		/* Invalidate the transaction */
> +		ctrl->tid_tbl[txn->tid] = NULL;
> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +	}
> +	if (ret)
> +		dev_err(&ctrl->dev, "Tx:MT:0x%x, MC:0x%x, LA:0x%x failed:%d\n",
> +			txn->mt, txn->mc, txn->la, ret);
> +	if (!async) {
> +		txn->msg->comp_cb = NULL;
> +		txn->msg->ctx = NULL;
> +	}

What is the intent of this if statement here? We set async
locally so this code only runs if we executed the else on the if
statement at the top. If its just clearing anything the user
might have put in these fields why not do it up there.

> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(slim_processtxn);
> +
> +int slim_xfer_msg(struct slim_controller *ctrl,
> +			struct slim_device *sbdev, struct slim_val_inf *msg,
> +			u8 mc)
> +{
> +	DEFINE_SLIM_LDEST_TXN(txn_stack, mc, 6, sbdev->laddr, msg);
> +	struct slim_msg_txn *txn = &txn_stack;
> +	int ret;
> +	u16 sl, cur;
> +
> +	ret = slim_val_inf_sanity(ctrl, msg, mc);
> +	if (ret)
> +		return ret;
> +
> +	sl = slim_slicesize(msg->num_bytes);
> +
> +	dev_dbg(&ctrl->dev, "SB xfer msg:os:%x, len:%d, MC:%x, sl:%x\n",
> +		msg->start_offset, msg->num_bytes, mc, sl);
> +
> +	cur = slim_slicecodefromsize(sl);

cur should probably be removed until it is needed.

> +	txn->ec = ((sl | (1 << 3)) | ((msg->start_offset & 0xFFF) << 4));
> +
> +	switch (mc) {
> +	case SLIM_MSG_MC_REQUEST_CHANGE_VALUE:
> +	case SLIM_MSG_MC_CHANGE_VALUE:
> +	case SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION:
> +	case SLIM_MSG_MC_CLEAR_INFORMATION:
> +		txn->rl += msg->num_bytes;
> +	default:
> +		break;
> +	}
> +
> +	if (slim_tid_txn(txn->mt, txn->mc))
> +		txn->rl++;
> +
> +	return slim_processtxn(ctrl, txn);
> +}
> +EXPORT_SYMBOL_GPL(slim_xfer_msg);
> +
> +/* Message APIs Unicast message APIs used by slimbus slave drivers */
> +
> +/*
> + * slim_request_val_element: request value element
> + * @sb: client handle requesting elemental message reads, writes.
> + * @msg: Input structure for start-offset, number of bytes to read.
> + * context: can sleep
> + * Returns:
> + * -EINVAL: Invalid parameters
> + * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
> + *	not being clocked or driven by controller)
> + * -ENOTCONN: If the transmitted message was not ACKed by destination device.
> + */
> +int slim_request_val_element(struct slim_device *sb,
> +				struct slim_val_inf *msg)
> +{
> +	struct slim_controller *ctrl = sb->ctrl;
> +
> +	if (!ctrl)
> +		return -EINVAL;

You could put this check into slim_xfer_msg and save duplicating
it. Would also then cover cases that call that function directly,
also would let you make these functions either inlines or macros.

> +
> +	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_VALUE);
> +}
> +EXPORT_SYMBOL_GPL(slim_request_val_element);
> +
> +/* Functions to get/return TX, RX buffers for messaging. */
> +
> +/**
> + * slim_get_rx: To get RX buffers for messaging.
> + * @ctrl: Controller handle
> + * These functions are called by controller to process the RX buffers.
> + * RX buffer is requested by controller when data is received from HW, but is
> + * not processed (e.g. 'report-present message was sent by HW in ISR and SW
> + * needs more time to process the buffer to assign Logical Address)
> + * RX buffer is returned back to the pool when associated RX action
> + * is taken (e.g. Received message is decoded and client's
> + * response buffer is filled in.)
> + */
> +void *slim_get_rx(struct slim_controller *ctrl)
> +{
> +	unsigned long flags;
> +	int idx;
> +
> +	spin_lock_irqsave(&ctrl->rx.lock, flags);
> +	if ((ctrl->rx.tail + 1) % ctrl->rx.n == ctrl->rx.head) {
> +		spin_unlock_irqrestore(&ctrl->rx.lock, flags);
> +		dev_err(&ctrl->dev, "RX QUEUE full!");
> +		return NULL;
> +	}
> +	idx = ctrl->rx.tail;
> +	ctrl->rx.tail = (ctrl->rx.tail + 1) % ctrl->rx.n;
> +	spin_unlock_irqrestore(&ctrl->rx.lock, flags);
> +
> +	return ctrl->rx.base + (idx * ctrl->rx.sl_sz);
> +}
> +EXPORT_SYMBOL_GPL(slim_get_rx);
> +
> +int slim_return_rx(struct slim_controller *ctrl, void *buf)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ctrl->rx.lock, flags);
> +	if (ctrl->rx.tail == ctrl->rx.head) {
> +		spin_unlock_irqrestore(&ctrl->rx.lock, flags);
> +		return -ENODATA;
> +	}
> +	memcpy(buf, ctrl->rx.base + (ctrl->rx.head * ctrl->rx.sl_sz),
> +				ctrl->rx.sl_sz);
> +	ctrl->rx.head = (ctrl->rx.head + 1) % ctrl->rx.n;
> +	spin_unlock_irqrestore(&ctrl->rx.lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(slim_return_rx);

I find the combination of get/return a bit odd, would get/put
maybe more idiomatic? Also the return could use some kernel doc.

> +
> +void slim_return_tx(struct slim_controller *ctrl, int err)
> +{
> +	unsigned long flags;
> +	int idx;
> +	struct slim_pending cur;
> +
> +	spin_lock_irqsave(&ctrl->tx.lock, flags);
> +	idx = ctrl->tx.head;
> +	ctrl->tx.head = (ctrl->tx.head + 1) % ctrl->tx.n;
> +	cur = ctrl->pending_wr[idx];
> +	spin_unlock_irqrestore(&ctrl->tx.lock, flags);
> +
> +	if (!cur.cb)
> +		dev_err(&ctrl->dev, "NULL Transaction or completion");
> +	else
> +		cur.cb(cur.ctx, err);
> +
> +	up(&ctrl->tx_sem);
> +}
> +EXPORT_SYMBOL_GPL(slim_return_tx);
> +
> +/**
> + * slim_get_tx: To get TX buffers for messaging.
> + * @ctrl: Controller handle
> + * These functions are called by controller to process the TX buffers.
> + * TX buffer is requested by controller when it's filled-in and sent to the
> + * HW. When HW has finished processing this buffer, controller should return it
> + * back to the pool.
> + */
> +void *slim_get_tx(struct slim_controller *ctrl, struct slim_msg_txn *txn,
> +		bool need_tid)
> +{
> +	unsigned long flags;
> +	int ret, idx;
> +
> +	ret = down_interruptible(&ctrl->tx_sem);
> +	if (ret < 0) {
> +		dev_err(&ctrl->dev, "TX semaphore down returned:%d", ret);
> +		return NULL;
> +	}
> +	spin_lock_irqsave(&ctrl->tx.lock, flags);
> +
> +	if (((ctrl->tx.head + 1) % ctrl->tx.n) == ctrl->tx.tail) {
> +		spin_unlock_irqrestore(&ctrl->tx.lock, flags);
> +		dev_err(&ctrl->dev, "controller TX buf unavailable");
> +		up(&ctrl->tx_sem);
> +		return NULL;
> +	}
> +	idx = ctrl->tx.tail;
> +	ctrl->tx.tail = (ctrl->tx.tail + 1) % ctrl->tx.n;
> +	ctrl->pending_wr[idx].cb = txn->msg->comp_cb;
> +	ctrl->pending_wr[idx].ctx = txn->msg->ctx;
> +	ctrl->pending_wr[idx].need_tid = need_tid;
> +	spin_unlock_irqrestore(&ctrl->tx.lock, flags);
> +
> +	return ctrl->tx.base + (idx * ctrl->tx.sl_sz);
> +}
> +EXPORT_SYMBOL_GPL(slim_get_tx);

The rx calls seem ok that is basically the controller's job to
call those, but with these two calls it seems sometimes the
framework calls them sometimes the controller driver has to. Is
there anyway we can simplify that a bit? Or at least include some
documentation as to when the controller should call them.

> diff --git a/include/linux/slimbus.h b/include/linux/slimbus.h
> index b5064b6..080d86a 100644
> --- a/include/linux/slimbus.h
> +++ b/include/linux/slimbus.h
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/device.h>
>  #include <linux/mutex.h>
> +#include <linux/semaphore.h>
>  #include <linux/mod_devicetable.h>
>  
>  /**
> @@ -34,6 +35,9 @@ extern struct bus_type slimbus_type;
>  #define SLIM_FRM_SLOTS_PER_SUPERFRAME	16
>  #define SLIM_GDE_SLOTS_PER_SUPERFRAME	2
>  
> +/* MAX in-flight transactions neededing transaction ID (8-bit, per spec) */

s/neededing/needing/

> +
> +/* Frequently used message transaction structures */
> +#define DEFINE_SLIM_LDEST_TXN(name, mc, rl, la, msg) \
> +	struct slim_msg_txn name = { rl, 0, mc, SLIM_MSG_DEST_LOGICALADDR, 0,\
> +					0, la, msg, }
> +
> +#define DEFINE_SLIM_BCAST_TXN(name, mc, rl, la, msg) \
> +	struct slim_msg_txn name = { rl, 0, mc, SLIM_MSG_DEST_BROADCAST, 0,\
> +					0, la, msg, }

If the LA isn't used in broadcast messages wouldn't it be simpler
to set it to a fixed value in this macro?

> +
> +#define DEFINE_SLIM_EDEST_TXN(name, mc, rl, la, msg) \
> +	struct slim_msg_txn name = { rl, 0, mc, SLIM_MSG_DEST_ENUMADDR, 0,\
> +					0, la, msg, }
> +

Also one final overall comment this commit contains a lot of two
and three letter abreviations that are not always clear. I would
probably suggest expanding a few of the less standard ones out to
make the code a little easier to follow.

Thanks,
Charles

Powered by blists - more mailing lists