Re: [RFC v6 1/6] dmaengine: Add Synopsys eDMA IP core driver

[Vinod Koul <vkoul@kernel.org>]

On 07-05-19, 09:08, Gustavo Pimentel wrote:
> On Tue, May 7, 2019 at 6:3:10, Vinod Koul <vkoul@kernel.org> wrote:
> > On 06-05-19, 16:42, Gustavo Pimentel wrote:

> > > > > +static struct dma_async_tx_descriptor *
> > > > > +dw_edma_device_transfer(struct dw_edma_transfer *xfer)
> > > > > +{
> > > > > +	struct dw_edma_chan *chan = dchan2dw_edma_chan(xfer->dchan);
> > > > > +	enum dma_transfer_direction direction = xfer->direction;
> > > > > +	phys_addr_t src_addr, dst_addr;
> > > > > +	struct scatterlist *sg = NULL;
> > > > > +	struct dw_edma_chunk *chunk;
> > > > > +	struct dw_edma_burst *burst;
> > > > > +	struct dw_edma_desc *desc;
> > > > > +	u32 cnt;
> > > > > +	int i;
> > > > > +
> > > > > +	if ((direction == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_WRITE) ||
> > > > > +	    (direction == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_READ))
> > > > > +		return NULL;
> > > > > +
> > > > > +	if (xfer->cyclic) {
> > > > > +		if (!xfer->xfer.cyclic.len || !xfer->xfer.cyclic.cnt)
> > > > > +			return NULL;
> > > > > +	} else {
> > > > > +		if (xfer->xfer.sg.len < 1)
> > > > > +			return NULL;
> > > > > +	}
> > > > > +
> > > > > +	if (!chan->configured)
> > > > > +		return NULL;
> > > > > +
> > > > > +	desc = dw_edma_alloc_desc(chan);
> > > > > +	if (unlikely(!desc))
> > > > > +		goto err_alloc;
> > > > > +
> > > > > +	chunk = dw_edma_alloc_chunk(desc);
> > > > > +	if (unlikely(!chunk))
> > > > > +		goto err_alloc;
> > > > > +
> > > > > +	src_addr = chan->config.src_addr;
> > > > > +	dst_addr = chan->config.dst_addr;
> > > > > +
> > > > > +	if (xfer->cyclic) {
> > > > > +		cnt = xfer->xfer.cyclic.cnt;
> > > > > +	} else {
> > > > > +		cnt = xfer->xfer.sg.len;
> > > > > +		sg = xfer->xfer.sg.sgl;
> > > > > +	}
> > > > > +
> > > > > +	for (i = 0; i < cnt; i++) {
> > > > > +		if (!xfer->cyclic && !sg)
> > > > > +			break;
> > > > > +
> > > > > +		if (chunk->bursts_alloc == chan->ll_max) {
> > > > > +			chunk = dw_edma_alloc_chunk(desc);
> > > > > +			if (unlikely(!chunk))
> > > > > +				goto err_alloc;
> > > > > +		}
> > > > > +
> > > > > +		burst = dw_edma_alloc_burst(chunk);
> > > > > +		if (unlikely(!burst))
> > > > > +			goto err_alloc;
> > > > > +
> > > > > +		if (xfer->cyclic)
> > > > > +			burst->sz = xfer->xfer.cyclic.len;
> > > > > +		else
> > > > > +			burst->sz = sg_dma_len(sg);
> > > > > +
> > > > > +		chunk->ll_region.sz += burst->sz;
> > > > > +		desc->alloc_sz += burst->sz;
> > > > > +
> > > > > +		if (direction == DMA_DEV_TO_MEM) {
> > > > > +			burst->sar = src_addr;
> > > > 
> > > > We are device to mem, so src is peripheral.. okay
> > > > 
> > > > > +			if (xfer->cyclic) {
> > > > > +				burst->dar = xfer->xfer.cyclic.paddr;
> > > > > +			} else {
> > > > > +				burst->dar = sg_dma_address(sg);
> > > > > +				src_addr += sg_dma_len(sg);
> > > > 
> > > > and we increment the src, doesn't make sense to me!
> > > > 
> > > > > +			}
> > > > > +		} else {
> > > > > +			burst->dar = dst_addr;
> > > > > +			if (xfer->cyclic) {
> > > > > +				burst->sar = xfer->xfer.cyclic.paddr;
> > > > > +			} else {
> > > > > +				burst->sar = sg_dma_address(sg);
> > > > > +				dst_addr += sg_dma_len(sg);
> > > > 
> > > > same here as well
> > > 
> > > This is hard to explain in words...
> > > Well, in my perspective I want to transfer a piece of memory from the 
> > > peripheral into local RAM
> > 
> > Right and most of the case RAM address (sg) needs to increment whereas
> > peripheral is a constant one
> > 
> > > Through the DMA client API I'll break this piece of memory in several 
> > > small parts and add all into a list (scatter-gather), right?
> > > Each element of the scatter-gather has the sg_dma_address (in the 
> > > DMA_DEV_TO_MEM case will be the destination address) and the 
> > > corresponding size.
> > 
> > Correct
> > 
> > > However, I still need the other address (in the DMA_DEV_TO_MEM case will 
> > > be the source address) for that small part of memory.
> > > Since I get that address from the config, I still need to increment the 
> > > source address in the same proportion of the destination address, in 
> > > other words, the increment will be the part size.
> > 
> > I don't think so. Typically the device address is a FIFO, which does not
> > increment and you keep pushing data at same address. It is not a memory
> 
> In my use case, it's a memory, perhaps that is what is causing this 
> confusing.
> I'm copying "plain and flat" data from point A to B, with the 
> particularity that the peripheral memory is always continuous and the CPU 
> memory can be constituted by scatter-gather chunks of contiguous memory

Then why should it be slave transfer, it should be treated as memcpy
with src and dst sg lists..
-- 
~Vinod