Date:   Tue, 10 Oct 2017 14:01:10 +0100
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Charles Keepax <ckeepax@...nsource.cirrus.com>
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

Thanks for the review comments,

On 10/10/17 13:19, Charles Keepax wrote:
> 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.
Thats true, Will fix it in next version.
> 
>> +{
>> +	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.
Its clearing the temporary callback and context field when user wants to 
read/write on simbus synchronously.

If async is false user should not put anything in comp_cb or ctx.
comp_cb and ctx are only used when drivers are doing asynchronous 
read/writes on slimbus. Completion of those are indicated by comp_cb() 
with context.



> 
>> +	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.
Yep.

> 
>> +	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.

I will give that a try in next version.

> 
>> +
>> +	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.

If that makes it more readable I can rename the functions as suggested, 
and I will also add kernel doc in next version.

> 
>> +
>> +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.

I will try to do add some details in the document in next version.

> 
>> 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/
> 
Will fix this in next version.
>> +
>> +/* 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?
> 
Yep, if the destination type is broadcast we should not set la or ea in 
the header. may be set 0 here.

>> +
>> +#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.
Will do that!!

thanks
srini
> 
> Thanks,
> Charles
> 

Powered by blists - more mailing lists