Hello, Vinod.
Thanks for your feedback.

2014-05-02 21:03 GMT+04:00 Vinod Koul [off-list ref]:

On Wed, Apr 23, 2014 at 05:53:25PM +0400, Alexander Popov wrote:

quoted

+static struct dma_async_tx_descriptor *
+mpc_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
+             unsigned int sg_len, enum dma_transfer_direction direction,
+             unsigned long flags, void *context)
+{
+     struct mpc_dma *mdma = dma_chan_to_mpc_dma(chan);
+     struct mpc_dma_chan *mchan = dma_chan_to_mpc_dma_chan(chan);
+     struct mpc_dma_desc *mdesc = NULL;
+     dma_addr_t per_paddr;
+     u32 tcd_nunits;
+     struct mpc_dma_tcd *tcd;
+     unsigned long iflags;
+     struct scatterlist *sg;
+     size_t len;
+     int iter, i;
+
+     /* Currently there is no proper support for scatter/gather */

Why? Is this HW or SW limitation?

This is the software limitation.

As Gerhard noticed, unfortunately the Linux DMA API combines
peripheral access with scatter/gather function.

But the original MPC512x DMA driver already uses scatter/gather feature
of the hardware for chaining together individual mpc_dma_desc's
in mpc_dma_execute() while mpc_dma_desc itself cannot use
scatter/gather feature, because each mpc_dma_desc is associated
with exactly one mpc_dma_tcd.

quoted

+             if (direction == DMA_DEV_TO_MEM) {
+                     tcd->saddr = per_paddr;
+                     tcd->daddr = sg_dma_address(sg);
+                     tcd->soff = 0;
+                     tcd->doff = 4;

what are these?

SOFF is source address signed offset. It is applied to the current
source address to form the next-state value as each source read is completed.
DOFF is destination address signed offset.

quoted

+     case DMA_TERMINATE_ALL:
+             /* Disable channel requests */
+             mdma = dma_chan_to_mpc_dma(chan);
+
+             spin_lock_irqsave(&mchan->lock, flags);
+
+             out_8(&mdma->regs->dmacerq, chan->chan_id);
+             list_splice_tail_init(&mchan->prepared, &mchan->free);
+             list_splice_tail_init(&mchan->queued, &mchan->free);
+             list_splice_tail_init(&mchan->active, &mchan->free);
+
+             spin_unlock_irqrestore(&mchan->lock, flags);
+
+             return 0;

empty line after this pls

ok

quoted

+     case DMA_SLAVE_CONFIG:
+             /*
+              * Constraints:
+              *  - only transfers between a peripheral device and
+              *     memory are supported;
+              *  - minimal transfer chunk is 4 bytes and consequently
+              *     source and destination addresses must be 4-byte aligned
+              *     and transfer size must be aligned on (4 * maxburst)
+              *     boundary;
+              *  - during the transfer RAM address is being incremented by
+              *     the size of minimal transfer chunk;
+              *  - peripheral port's address is constant during the transfer.
+              */
+
+             cfg = (void *)arg;
+
+             if (cfg->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES ||
+                 cfg->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES ||

and why this limtation, doesnt seem covered above?

I created this limitation because FIFO registers of LPC and SDHC
support _only_ 4-byte access.

I tried to cover this limitation in the statement "minimal transfer chunk
is 4 bytes". Should I make it more explicit?

quoted

+                 !IS_ALIGNED(cfg->src_addr, 4) ||
+                 !IS_ALIGNED(cfg->dst_addr, 4)) {
+                     return -EINVAL;
+             }
+
+             spin_lock_irqsave(&mchan->lock, flags);
+
+             mchan->src_per_paddr = cfg->src_addr;
+             mchan->src_tcd_nunits = cfg->src_maxburst;
+             mchan->dst_per_paddr = cfg->dst_addr;
+             mchan->dst_tcd_nunits = cfg->dst_maxburst;
+
+             /* Apply defaults */
+             if (mchan->src_tcd_nunits == 0)
+                     mchan->src_tcd_nunits = 1;
+             if (mchan->dst_tcd_nunits == 0)
+                     mchan->dst_tcd_nunits = 1;
+
+             spin_unlock_irqrestore(&mchan->lock, flags);
+
+             return 0;

empty line here too

ok

Best regards,
Alexander