[PATCH RFC v12 3/7] dma: mpc512x: add support for peripheral transfers

[Alexander Popov]

Hello, Vinod.
Thanks for your feedback.

2014-05-02 21:03 GMT+04:00 Vinod Koul <vinod.koul at intel.com>:
> On Wed, Apr 23, 2014 at 05:53:25PM +0400, Alexander Popov wrote:
>> +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.

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

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

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

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