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.
> > > > +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?
> > > > +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.
Thanks
Ludovic
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
