On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote: > From: Sagar Dharia <sdha...@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).
I think we can implement this "optimization" with less complex code, regardless I don't think we need to mention this in the commit message... [..] > diff --git a/drivers/slimbus/slim-messaging.c > b/drivers/slimbus/slim-messaging.c [..] > +/** > + * slim_msg_response: Deliver Message response received from a device to the > + * framework. > + * @ctrl: Controller handle > + * @reply: Reply received from the device > + * @len: Length of the reply > + * @tid: Transaction ID received with which framework can associate reply. > + * Called by controller to inform framework about the response received. > + * This helps in making the API asynchronous, and controller-driver doesn't > need > + * to manage 1 more table other than the one managed by framework mapping TID > + * with buffers > + */ > +void slim_msg_response(struct slim_controller *ctrl, u8 *reply, u8 tid, u8 > len) Even if tid and len comes from the spec I recommend you making them int and size_t. > +{ > + struct slim_val_inf *msg; > + unsigned long flags; > + > + spin_lock_irqsave(&ctrl->txn_lock, flags); > + msg = ctrl->tid_tbl[tid]; > + if (msg == NULL || msg->rbuf == NULL) { if (!msg || !msg->rbuf) When is it valid to add a transaction to tid_tbl with msg->rbuf = NULL? Should we reject it earlier? > + spin_unlock_irqrestore(&ctrl->txn_lock, flags); > + dev_err(&ctrl->dev, "Got response to invalid TID:%d, len:%d\n", > + tid, len); > + return; > + } > + ctrl->tid_tbl[tid] = NULL; > + spin_unlock_irqrestore(&ctrl->txn_lock, flags); > + > + memcpy(msg->rbuf, reply, len); > + if (msg->comp_cb) > + msg->comp_cb(msg->ctx, 0); > +} > +EXPORT_SYMBOL_GPL(slim_msg_response); [..] > +int slim_processtxn(struct slim_controller *ctrl, > + struct slim_msg_txn *txn) > +{ > + 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 (ret) { if (need_tid) drop(); dev_err(); } Would probably make this a little bit cleaner... > + if (!async) { > + txn->msg->comp_cb = NULL; > + txn->msg->ctx = NULL; I believe txn->msg is always required, so you don't need to do this contidionally. > + } > + return ret; > +} > +EXPORT_SYMBOL_GPL(slim_processtxn); [..] > +int slim_request_val_element(struct slim_device *sb, > + struct slim_val_inf *msg) > +{ > + struct slim_controller *ctrl = sb->ctrl; > + > + if (!ctrl) > + return -EINVAL; >From patch 1 I believe it's invalid for sb->ctrl to be NULL, so there shouldn't be a need to check this. > + > + return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_VALUE); > +} > +EXPORT_SYMBOL_GPL(slim_request_val_element); [..] > +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); > + Please provide kerneldoc for exported symbols. > +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]; Why is this doing struct copy? > + 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); [..] > /** > + * struct slim_val_inf: Slimbus value or information element > + * @start_offset: Specifies starting offset in information/value element map > + * @num_bytes: upto 16. This ensures that the message will fit the slicesize > + * per slimbus spec > + * @comp_cb: Callback if this read/write is asynchronous > + * @ctx: Argument for comp_cb > + */ > +struct slim_val_inf { > + u16 start_offset; > + u8 num_bytes; > + u8 *rbuf; This is not mentioned in the kerneldoc. Use void * for data buffers. > + const u8 *wbuf; Can a message ever be read and write? Otherwise it should be sufficient to only have one data pointer. > + void (*comp_cb)(void *ctx, int err); > + void *ctx; > +}; > + [..] > +/** > + * struct slim_ctrl_buf: circular buffer used by contoller for TX, RX > + * @base: virtual base address for this buffer > + * @phy: physical address for this buffer (this is useful if controller can > + * DMA the buffers for TX and RX to/from controller hardware > + * @lock: lock protecting head and tail > + * @head: index where buffer is returned back > + * @tail: index from where buffer is consumed > + * @sl_sz: byte-size of each slot in this buffer > + * @n: number of elements in this circular ring, note that this > needs to be > + * 1 more than actual buffers to allow for one open slot > + */ Is this ringbuffer mechanism defined in the slimbus specification? Looks like something specific to the Qualcomm controller, rather than something that should be enforced in the framework. > +struct slim_ctrl_buf { > + void *base; > + phys_addr_t phy; > + spinlock_t lock; > + int head; > + int tail; > + int sl_sz; > + int n; > +}; [..] > +/** > * struct slim_controller: Controls every instance of SLIMbus > * (similar to 'master' on SPI) > * 'Manager device' is responsible for device management, bandwidth > @@ -139,6 +246,16 @@ struct slim_addrt { > * @addrt: Logical address table > * @num_dev: Number of active slimbus slaves on this bus > * @wq: Workqueue per controller used to notify devices when they report > present > + * @tid_tbl: Table of transactions having transaction ID > + * @txn_lock: Lock to protect table of transactions > + * @rx: RX buffers used by controller to receive messages. Ctrl may receive > more > + * than 1 message (e.g. multiple report-present messages or messages from > + * multiple slaves). > + * @tx: TX buffers used by controller to transmit messages. Ctrl may have > + * ability to send/queue multiple messages to HW at once. > + * @pending_wr: Pending write transactions to be acknowledged by controller This is out list of pending write requests, yet it's implemented as an array used in a complex ring buffer fashion. Wouldn't it be easier to just have this as a linked list of slim_pending struct? > + * @tx_sem: Semaphore for available TX buffers for this controller > + * @last_tid: Last used entry for TID transactions > * @xfer_msg: Transfer a message on this controller (this can be a broadcast > * control/status message like data channel setup, or a unicast message > * like value element read/write. > @@ -161,6 +278,15 @@ struct slim_controller { > struct slim_addrt *addrt; > u8 num_dev; > struct workqueue_struct *wq; > + struct slim_val_inf *tid_tbl[SLIM_MAX_TIDS]; > + u8 last_tid; I suggest that you replace these two with an idr, rather than having a fixed size array and then last_tid as an optimization to limit how far you linear search for an empty space. > + spinlock_t txn_lock; > + struct slim_ctrl_buf tx; > + struct slim_ctrl_buf rx; > + struct slim_pending *pending_wr; > + struct semaphore tx_sem; Please don't use semaphores. If you keep pending_wr as a list you can use list_empty() instead... > + int (*xfer_msg)(struct slim_controller *ctrl, > + struct slim_msg_txn *tx, void *buf); I believe buf has fixed size, so please document this. > int (*set_laddr)(struct slim_controller *ctrl, > struct slim_eaddr *ea, u8 laddr); > int (*get_laddr)(struct slim_controller *ctrl, > @@ -295,5 +421,40 @@ static inline void slim_set_devicedata(struct > slim_device *dev, void *data) > { > dev_set_drvdata(&dev->dev, data); > } Regards, Bjorn