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

[Vinod Koul <vkoul@kernel.org>]

On 06-05-19, 16:42, Gustavo Pimentel wrote:

> > > +	if (unlikely(!chunk))
> > > +		return NULL;
> > > +
> > > +	INIT_LIST_HEAD(&chunk->list);
> > > +	chunk->chan = chan;
> > > +	chunk->cb = !(desc->chunks_alloc % 2);
> > ? why %2?
> 
> I think it's explained on the patch description. CB also is known as 
> Change Bit that must be toggled in order to the HW assume a new linked 
> list is available to be consumed.
> Since desc->chunks_alloc variable is an incremental counter the remainder 
> after division by 2 will be zero (if chunks_alloc is even) or one (if 
> chunks_alloc is odd).

Okay it would be great to add a comment here to explain as well

> > > +static enum dma_status
> > > +dw_edma_device_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
> > > +			 struct dma_tx_state *txstate)
> > > +{
> > > +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> > > +	struct dw_edma_desc *desc;
> > > +	struct virt_dma_desc *vd;
> > > +	unsigned long flags;
> > > +	enum dma_status ret;
> > > +	u32 residue = 0;
> > > +
> > > +	ret = dma_cookie_status(dchan, cookie, txstate);
> > > +	if (ret == DMA_COMPLETE)
> > > +		return ret;
> > > +
> > > +	if (ret == DMA_IN_PROGRESS && chan->status == EDMA_ST_PAUSE)
> > > +		ret = DMA_PAUSED;
> > 
> > Don't you want to set residue on paused channel, how else will user know
> > the position of pause?
> 
> I didn't catch you on this. I'm only setting the dma status here. After 
> this function, the residue is calculated and set, isn't it?

Hmm I thought you returned for paused case, if not then it is okay

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

> If there is some way to set and get the address for the source (in this 
> case) into each scatter-gather element, that would be much nicer, is that 
> possible?

> > > +		case EDMA_REQ_STOP:
> > > +			list_del(&vd->node);
> > > +			vchan_cookie_complete(vd);
> > > +			chan->request = EDMA_REQ_NONE;
> > > +			chan->status = EDMA_ST_IDLE;
> > 
> > Why do we need to track request as well as status?
> 
> Since I don't actually have the PAUSE state feature available on HW, I'm 
> emulating it through software. As far as HW is concerned, it thinks that 
> it has transferred everything (no more bursts valid available), but in 
> terms of software, we still have a lot of chunks (each one containing 
> several bursts) to process.

Why do you need to emulate, if HW doesnt support so be it?
The applications should handle a device which doesnt support pause and
not a low level driver

> > > +struct dw_edma_transfer {
> > > +	struct dma_chan			*dchan;
> > > +	union Xfer {
> > 
> > no camel case please
> 
> Ok.
> 
> > 
> > It would help to run checkpatch with --strict option to find any style
> > issues and fix them as well
> 
> I usually run with that option, but for now, that option is giving some 
> warnings about macro variable names that are pure noise.

yeah that is a *guide* and to be used as guidance. If code looks worse
off then it shouldn't be used. But many of the test are helpful. Some
macros checks actually make sense, but again use your judgement :)

-- 
~Vinod