lkml.org 
[lkml]   [2017]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [Patch v6 2/7] slimbus: Add messaging APIs to slimbus framework
From
Date
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@linaro.org wrote:
>> From: Sagar Dharia <sdharia@codeaurora.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@codeaurora.org>
>> Tested-by: Naveen Kaje <nkaje@codeaurora.org>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.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
>

\
 
 \ /
  Last update: 2017-10-07 12:25    [W:0.144 / U:14.148 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site