On Wed, Sep 10, 2014 at 03:23:53PM +0200, Ludovic Desroches wrote:
> Hi,
>
> Sorry for the delay. I am about to send a new version but I have still
> have some questions.
>
> On Mon, Jul 28, 2014 at 10:17:50PM +0530, Vinod Koul wrote:
> > On Mon, Jul 28, 2014 at 05:54:31PM +0200, Ludovic Desroches wrote:
> > > > > +{
> > > > > + struct at_xdmac_desc *desc = txd_to_at_desc(tx);
> > > > > + struct at_xdmac_chan *atchan = to_at_xdmac_chan(tx->chan);
> > > > > + dma_cookie_t cookie;
> > > > > + unsigned long flags;
> > > > > +
> > > > > + spin_lock_irqsave(&atchan->lock, flags);
> > > > > + cookie = dma_cookie_assign(tx);
> > > > > +
> > > > > + dev_vdbg(chan2dev(tx->chan), "%s: atchan 0x%p, add desc 0x%p to
> > > > > xfers_list\n",
> > > > > + __func__, atchan, desc);
> > > > > + list_add_tail(&desc->xfer_node, &atchan->xfers_list);
> > > > > + if (list_is_singular(&atchan->xfers_list))
> > > > > + at_xdmac_start_xfer(atchan, desc);
> > > > so you dont start if list has more than 1 descriptors, why?
> > > Because I will let at_xdmac_advance_work managing the queue if not
> > > empty. So the only moment when I have to start the transfer in tx_submit
> > > is when I have only one transfer in the queue.
> > So xfers_list is current active descriptors right and if it is always going
> > to be singular why bother with a list?
>
> xfers_list won't be always singular. I put all transfers into this list.
> If it is not singular, I don't have to start this transfer now.
Okay i think i got it now, you want to start only when its sigular and if
list is larger then at_xdmac_advance_work() would do so
> > > > > +static struct dma_async_tx_descriptor *
> > > > > +at_xdmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest,
> > > > > dma_addr_t src,
> > > > > + size_t len, unsigned long flags)
> > > > > +{
> > > > > + struct at_xdmac_chan *atchan = to_at_xdmac_chan(chan);
> > > > > + struct at_xdmac_desc *first = NULL, *prev = NULL;
> > > > > + size_t remaining_size = len, xfer_size = 0,
> > > > > ublen;
> > > > > + dma_addr_t src_addr = src, dst_addr = dest;
> > > > > + u32 dwidth;
> > > > > + /*
> > > > > + * WARNING: The channel configuration is set here since there
> > > > > is no
> > > > > + * dmaengine_slave_config call in this case. Moreover we don't
> > > > > know the
> > > > > + * direction, it involves we can't dynamically set the source
> > > > > and dest
> > > > > + * interface so we have to use the same one. Only interface 0
> > > > > allows EBI
> > > > > + * access. Hopefully we can access DDR through both ports (at
> > > > > least on
> > > > > + * SAMA5D4x), so we can use the same interface for source and
> > > > > dest,
> > > > > + * that solves the fact we don't know the direction.
> > > > and why do you need slave config for memcpy?
> > > Maybe the comments is not clear. With slave config we have the direction
> > > per to mem or mem to per. Of course when doing memcpy we don't need this
> > > information. But I use this information not only to set the transfer
> > > direction but also to set the interfaces to use for the source and dest
> > > (these interfaces depend on the connection on the matrix).
> > > So slave config was useful due to direction field to tell which interface
> > > to use as source and which one as dest.
> > > I am lucky because NAND and DDR are on the same interfaces so it's not a
> > > problem.
> > >
> > > This comment is more a warning for myself to keep in mind this possible
> > > issue in order to not have a device with NAND and DDR on different
> > > interfaces because it will be difficult to tell which interface is the
> > > source and which one is the dest.
> > Well in that case shouldnt NAND be treated like periphral and not memory?
>
> Yes maybe it would be better but it is not needed at the moment.
>
> [...]
>
> > > > > + */
> > > > > + list_del(&desc->xfer_node);
> > > > > + list_splice_init(&desc->descs_list, &atchan->free_descs_list);
> > > > shouldnt you move all the desc in active list in one shot ?
> > > >
> > >
> > > Sorry I don't get it. What are you suggesting by one shot?
> > Rather than invoking this per descriptor moving descriptors to
> > free_desc_list one by one, we can move all descriptors togther.
>
> Per descriptor? list_splice_init move all the desc in one shot. Are you
> talking about the terminate all case?
In this case you invoke the function for a descriptor which you delete from
its list and then join it to free_descs_list.
> > > > > +static void at_xdmac_tasklet(unsigned long data)
> > > > > +{
> > > > > + struct at_xdmac_chan *atchan = (struct at_xdmac_chan *)data;
> > > > > + struct at_xdmac_desc *desc;
> > > > > + u32 error_mask;
> > > > > +
> > > > > + dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08lx\n",
> > > > > + __func__, atchan->status);
> > > > > +
> > > > > + error_mask = AT_XDMAC_CIS_RBEIS
> > > > > + | AT_XDMAC_CIS_WBEIS
> > > > > + | AT_XDMAC_CIS_ROIS;
> > > > > +
> > > > > + if (at_xdmac_chan_is_cyclic(atchan)) {
> > > > > + at_xdmac_handle_cyclic(atchan);
> > > > > + } else if ((atchan->status & AT_XDMAC_CIS_LIS)
> > > > > + || (atchan->status & error_mask)) {
> > > > > + struct dma_async_tx_descriptor *txd;
> > > > > +
> > > > > + if (atchan->status & AT_XDMAC_CIS_RBEIS)
> > > > > + dev_err(chan2dev(&atchan->chan), "read bus
> > > > > error!!!");
> > > > > + else if (atchan->status & AT_XDMAC_CIS_WBEIS)
> > > > > + dev_err(chan2dev(&atchan->chan), "write bus
> > > > > error!!!");
> > > > > + else if (atchan->status & AT_XDMAC_CIS_ROIS)
> > > > > + dev_err(chan2dev(&atchan->chan), "request
> > > > > overflow error!!!");
> > > > > +
> > > > > + desc = list_first_entry(&atchan->xfers_list,
> > > > > + struct at_xdmac_desc,
> > > > > + xfer_node);
> > > > > + dev_vdbg(chan2dev(&atchan->chan), "%s: desc 0x%p\n",
> > > > > __func__, desc);
> > > > > + BUG_ON(!desc->active_xfer);
> > > > > +
> > > > > + txd = &desc->tx_dma_desc;
> > > > > +
> > > > > + at_xdmac_terminate_xfer(atchan, desc);
> > > > why terminate here? This looks wrong
> > >
> > > Why? What do you have in mind? It is not obvious for me that it is the
> > > wrong place.
> > The descriptor has completed. The terminate has special meaning. The user
> > can cancel all transactions by invoking terminate_all(). That implies to
> > clean up the channel and abort the transfers. The transfer is done so what
> > does it mean to terminate.
>
> So maybe I have to rename this function because I need to remove the
> transfer from the transfer list and put the transfer descriptors in the
> free descriptor list once transfer is done.
ok
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
