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

[Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>]

Hi Vinod,

On Mon, May 6, 2019 at 12:20:1, Vinod Koul <vkoul@kernel.org> wrote:

> On 23-04-19, 20:30, Gustavo Pimentel wrote:
> > Add Synopsys PCIe Endpoint eDMA IP core driver to kernel.
> 
> Still an RFC ?

Yes, it was RFC til I get a formal validation from the HW team. Now that 
I got it, I can formally submit the very first patch version.

> 
> > +static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc)
> > +{
> > +	struct dw_edma_chan *chan = desc->chan;
> > +	struct dw_edma *dw = chan->chip->dw;
> > +	struct dw_edma_chunk *chunk;
> > +
> > +	chunk = kzalloc(sizeof(*chunk), GFP_KERNEL);
> 
> Looking at the code this should be called from one of the
> device_prep_xxx calls so this should not sleep, so GFP_NOWAIT please
> 
> (pls audit rest of the mem allocations in the code)

Ok. Fixed on dw_edma_alloc_burst(), dw_edma_alloc_chunk() and 
dw_edma_alloc_desc().
The other memory allocations are on probe() and dw_edma_channel_setup() 
that is called by probe(), that doesn't require to be GFP_NOWAIT.

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

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

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

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

> 
> > +static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
> > +{
> > +	struct dw_edma_desc *desc;
> > +	struct virt_dma_desc *vd;
> > +	unsigned long flags;
> > +
> > +	dw_edma_v0_core_clear_done_int(chan);
> > +
> > +	spin_lock_irqsave(&chan->vc.lock, flags);
> > +	vd = vchan_next_desc(&chan->vc);
> > +	if (vd) {
> > +		switch (chan->request) {
> > +		case EDMA_REQ_NONE:
> > +			desc = vd2dw_edma_desc(vd);
> > +			if (desc->chunks_alloc) {
> > +				chan->status = EDMA_ST_BUSY;
> > +				dw_edma_start_transfer(chan);
> > +			} else {
> > +				list_del(&vd->node);
> > +				vchan_cookie_complete(vd);
> > +				chan->status = EDMA_ST_IDLE;
> > +			}
> > +			break;
> 
> Empty line after each break please

Ok. Done.

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

> 
> > +static int dw_edma_alloc_chan_resources(struct dma_chan *dchan)
> > +{
> > +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> > +
> > +	if (chan->status != EDMA_ST_IDLE)
> > +		return -EBUSY;
> > +
> > +	dma_cookie_init(dchan);
> 
> not using vchan_init() you need to do this and init the lists..?

That's right, vchan_init() already does that.

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

> -- 
> ~Vinod

Regards,
Gustavo