Re: [PATCH v3 1/2] dma: at_xdmac: creation of the atmel eXtended DMA Controller driver

On Mon, Jul 28, 2014 at 05:54:31PM +0200, Ludovic Desroches wrote:

quoted

quoted

+{
+	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?

quoted

quoted

+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?

quoted

quoted

+static enum dma_status
+at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
+		struct dma_tx_state *txstate)
+{
+	struct at_xdmac_chan 	*atchan = to_at_xdmac_chan(chan);
+	struct at_xdmac		*atxdmac = to_at_xdmac(atchan->chan.device);
+	struct at_xdmac_desc	*desc, *_desc;
+	unsigned long		flags;
+	enum dma_status		ret;
+	int			residue;
+	u32			cur_nda;
+
+	ret = dma_cookie_status(chan, cookie, txstate);
+	if (ret == DMA_COMPLETE)
+		return ret;
+
+	spin_lock_irqsave(&atchan->lock, flags);
+
+	desc = list_first_entry(&atchan->xfers_list, struct at_xdmac_desc, xfer_node);
+
+	if (!desc->active_xfer)
+		dev_err(chan2dev(chan),
+			"something goes wrong, there is no active transfer\n");

We can query resiude even when tx_submit hasnt been invoked. You need to
return the full length in that case.

Ok, I was not aware about this case.

quoted

quoted

+
+	residue = desc->xfer_size;

Also do check if txsate is NULL, in case just bail out here.

Return value will be the one returned by dma_cookie_status?

Yes

quoted

quoted

+	 */
+	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.

quoted

quoted

+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.

quoted

quoted

+	if (!atxdmac) {
+		dev_err(&pdev->dev, "can't allocate at_xdmac structure\n");
+		return -ENOMEM;
+	}
+
+	atxdmac->regs = base;
+	atxdmac->irq = irq;
+
+	ret = devm_request_irq(&pdev->dev, atxdmac->irq, at_xdmac_interrupt, 0,
+			       "at_xdmac", atxdmac);

and this can invoke your irq handler and you may not be ready. Second this
cause races with tasklet, so usualy recommednation is to use regular
request_irq()

You mean it is specific to devm variant, isn't it?

Yes.

quoted

quoted

+	dma_cap_set(DMA_CYCLIC, atxdmac->dma.cap_mask);
+	dma_cap_set(DMA_MEMCPY, atxdmac->dma.cap_mask);
+	dma_cap_set(DMA_SLAVE, atxdmac->dma.cap_mask);
+	atxdmac->dma.dev				= &pdev->dev;
+	atxdmac->dma.device_alloc_chan_resources	= at_xdmac_alloc_chan_resources;
+	atxdmac->dma.device_free_chan_resources		= at_xdmac_free_chan_resources;
+	atxdmac->dma.device_tx_status			= at_xdmac_tx_status;
+	atxdmac->dma.device_issue_pending		= at_xdmac_issue_pending;
+	atxdmac->dma.device_prep_dma_cyclic		= at_xdmac_prep_dma_cyclic;
+	atxdmac->dma.device_prep_dma_memcpy		= at_xdmac_prep_dma_memcpy;
+	atxdmac->dma.device_prep_slave_sg		= at_xdmac_prep_slave_sg;
+	atxdmac->dma.device_control			= at_xdmac_control;
+	atxdmac->dma.chancnt				= nr_channels;

no caps, pls do implement that as you have cyclic dma too

Sorry I don't understand what you mean here.

pls implement .device_slave_caps

quoted

quoted

+static int at_xdmac_remove(struct platform_device *pdev)
+{
+	struct at_xdmac	*atxdmac = (struct at_xdmac*)platform_get_drvdata(pdev);
+	int		i;
+
+	at_xdmac_off(atxdmac);
+	of_dma_controller_free(pdev->dev.of_node);
+	dma_async_device_unregister(&atxdmac->dma);
+	clk_disable_unprepare(atxdmac->clk);
+
+	synchronize_irq(atxdmac->irq);
+
+	for (i = 0; i < atxdmac->dma.chancnt; i++) {
+		struct at_xdmac_chan *atchan = &atxdmac->chan[i];
+
+		tasklet_kill(&atchan->tasklet);
+		at_xdmac_free_chan_resources(&atchan->chan);
+	}

and you can still get irq!

Why? I thought deactivating irq on device side and calling synchronize
irq will do the stuff.

what if we have buggy hardware or spurious interrupt!

quoted

quoted

+
+	return 0;
+}
+
+static const struct dev_pm_ops atmel_xdmac_dev_pm_ops = {
+	.prepare	= atmel_xdmac_prepare,
+	.suspend_noirq	= atmel_xdmac_suspend_noirq,
+	.resume_noirq	= atmel_xdmac_resume_noirq,
+};

no ifdef CONFIG_PM?

I'll add it.

quoted

You might want to make them late_suspend and early_resume to allow
clients suspending first

Ok, as I told before I was wondering which variant to use. For my
knowledge, it will be the case also with noirq, isn't it? It is called
after suspend_late.

Yes, thats why it makes sense for dmaengine drivers to use only late_suspend
and ealy_resume

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html