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

Thanks for the comments.

On 07/10/17 07:42, Jonathan Neuschäfer wrote:
> Hi,
> 
> 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
> 
> s/based of/based on/
> s/coerently/coherently/
Yep.. will fix this in next version.
> 
>> 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>
>> ---
> [...]
>> +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);
>> +	txn->ec = ((sl | (1 << 3)) | ((msg->start_offset & 0xFFF) << 4));
> 
> Shouldn't this be (cur | (1 << 3)?
cur seems to be redundant TBH, the only difference between cur and sl is 
that the slim_slicesize() can give slice size to program for any lengths 
between 1-16 bytes. However the slim_slicecodefromsize() can only 
handle 1,2,3,4, 6,8,12,16 byte sizes.

So we can delete slim_slicecodefromsize() call and function together.
looks like it was a leftover from downstream.

> (Also, what does cur mean? Cursor? Current?)
No Idea!! :-) it is supposed to return slice size as per number of bytes.

> 
>> +
>> +	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);
> [...]
>> +/*
>> + * slim_request_val_element: change and request a given value element

name should be fixed here..
>> + * @sb: client handle requesting elemental message reads, writes.
>> + * @msg: Input structure for start-offset, number of bytes to write.
>> + * 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.
> 
> Does rbuf contain the old value after this function finishes?
> 
Yep, device should send a reply value with the old value with matching tid.

>> + */
>> +int slim_request_change_val_element(struct slim_device *sb,
>> +					struct slim_val_inf *msg)
>> +{
>> +	struct slim_controller *ctrl = sb->ctrl;
>> +
>> +	if (!ctrl)
>> +		return -EINVAL;
>> +
>> +	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_CHANGE_VALUE);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_request_change_val_element);
> [...]
>> +/**
>> + * struct slim_pending: context of pending transfers
>> + * @cb: callback for this transfer
>> + * @ctx: contex for the callback function
> 
> s/contex/context/
> 
Will fix all these instances.
>> + * @need_tid: True if this transfer need Transaction ID
>> + */
>> +struct slim_pending {
>> +	void (*cb)(void *ctx, int err);
>> +	void *ctx;
>> +	bool need_tid;
>> +};
> 
> 
> Thanks,
> Jonathan Neuschäfer
> 

Powered by blists - more mailing lists