Re: [Patch v6 2/7] slimbus: Add messaging APIs to slimbus framework

[Bjorn Andersson]

On Wed 18 Oct 09:39 PDT 2017, Srinivas Kandagatla wrote:

> Thanks for Review Comments,
>
>
> On 18/10/17 07:15, Bjorn Andersson wrote:
> > On Fri 06 Oct 08:51 PDT 2017, srinivas.kandagatla@xxxxxxxxxx wrote:
[..]
> >
> > > + 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.
>
> I don't get this, why do you want to set comp_cb to NULL unconditionally?
>

I'm just not happy about the complexity of this function, but perhaps
it's confusing to always set them, regardless of them being used. Feel
free to keep it.

[..]
> > > +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?
> >
> Not sure, do you see any issue with this?
>

It's a rarely used feature and I don't see a reason for using it here.

It's probably better to make a copy of cur.cb and cur.ctx to make their
use after the spin-unlock more obvious (but should be fine as the
spinlock is for the pending_wr array.

> > > + 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_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.
> >
>
> Yes, this is not part of the slimbus specs, but Qcom SOCs have concept of
> Message Queues.
>
> Are you suggesting that this buffer handling has to be moved out of core
> into controller driver?
>

The fact that this seems to describe a physical ring buffer, with some
set of properties that are related to how a ring buffer works in the
Qualcomm hardware and it carries a notion of physical mapping, all
indicates to me that this describes some Qualcomm hardware interface.

I believe this is a hardware implementation detail that should reside in
the hardware part of the implementation (i.e. the Qualcomm driver).

>
> > > +struct slim_ctrl_buf {
> > > + void *base;
> > > + phys_addr_t phy;
> > > + spinlock_t lock;
> > > + int head;
> > > + int tail;
> > > + int sl_sz;
> > > + int n;
> > > +};

Regards,
Bjorn