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

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

Hi Vinod,

On 17/12/2018 06:51, Vinod Koul wrote:
> On 12-12-18, 12:13, Gustavo Pimentel wrote:
>> Add Synopsys eDMA IP core driver to kernel.
>>
>> This core driver, initializes and configures the eDMA IP using vma-helpers
>> functions and dma-engine subsystem.
>>
>> Also creates an abstration layer through callbacks allowing different
>> registers mappings in the future, organized in to versions.
>>
>> This driver can be compile as built-in or external module in kernel.
>>
>> To enable this driver just select DW_EDMA option in kernel configuration,
>> however it requires and selects automatically DMA_ENGINE and
>> DMA_VIRTUAL_CHANNELS option too.
> 
> The subsystem name is dmaengine: so please use that tag. If you are not
> aware then git log for that subsystem helps you with the patterns
> expected

Ok, I'll change all patch description tags. For some reason I got the idea that
this would be the correct tag.

> 
> I did a quick look at the patch, I have highlighted few concerns and
> they repeat in similar code patterns in this patch

Ok, great! Thanks Vinod.

> 
>> +#include "dw-edma-core.h"
>> +#include "../dmaengine.h"
>> +#include "../virt-dma.h"
>> +
>> +#define DRV_CORE_NAME				"dw-edma-core"
> 
> Why is this required?

It's use on devm_request_irq(), requires a name probably for debugging proposes
or to show some output to the user.

> 
>> +
>> +#define SET(reg, name, val)			\
>> +	reg.name = val
>> +
>> +#define SET_BOTH_CH(name, value)		\
>> +	do {					\
>> +		SET(dw->wr_edma, name, value);	\
>> +		SET(dw->rd_edma, name, value);	\
>> +	} while (0)
> 
> I am not sure how this helps, makes things not explicit..

Since this driver has 2 channels (write and read) I'd like to simplify all the
configurations that I've to make on both channels (avoiding any omission), that
why I created this macro.

Should I add some comment on top of this macro or do you think that is better to
replicate the code for each channel?

> 
>> +static struct dw_edma_burst *dw_edma_alloc_burst(struct dw_edma_chunk *chunk)
>> +{
>> +	struct dw_edma_chan *chan = chunk->chan;
>> +	struct dw_edma_burst *burst;
>> +
>> +	burst = kzalloc(sizeof(struct dw_edma_burst), GFP_NOWAIT);
>> +	if (unlikely(!burst)) {
>> +		dev_err(chan2dev(chan), ": fail to alloc new burst\n");
> 
> no need to log mem alloc failures

Ok.

> 
>> +		return NULL;
>> +	}
>> +
>> +	INIT_LIST_HEAD(&burst->list);
>> +	burst->sar = 0;
>> +	burst->dar = 0;
>> +	burst->sz = 0;
> 
> you did kzalloc right?

Nice catch! I don't need those zero initialization, since I use kzalloc. I'll
remove them.

> 
>> +
>> +	if (chunk->burst) {
>> +		atomic_inc(&chunk->bursts_alloc);
> 
> why does this need atomic variables?

Since the variable bursts_alloc can be manipulated through the following
functions: dw_edma_free_desc(), dw_edma_done_interrupt(),
dw_edma_device_resume(), dw_edma_device_issue_pending() and
dw_edma_device_prep_slave_sg() (which can be called in different contexts), I
thought it would be safer to define this variable as atomic.

However looking now, I notice that I don't need it since
dw_edma_done_interrupt(), dw_edma_device_resume() and
dw_edma_device_issue_pending() are protected by a spin lock, however I also
noticed that dw_edma_free_desc() and dw_edma_device_prep_slave_sg() aren't
protected at all. I'll add the spin lock to those functions and replace the
atomic variable bu a u32 type.

> 
>> +static void dw_edma_free_burst(struct dw_edma_chunk *chunk)
>> +{
>> +	struct dw_edma_burst *child, *_next;
>> +
>> +	if (!chunk->burst)
>> +		return;
>> +
>> +	// Remove all the list elements
> 
> We dont use C99 style comments, please use 
>         /* single line */
> and
>         /*
>          * multi
>          * line
>          */

Understood, Bjorn also warned me about that. I'll fix that.

> 
>> +static void start_transfer(struct dw_edma_chan *chan)
>> +{
>> +	struct virt_dma_desc *vd;
>> +	struct dw_edma_desc *desc;
>> +	struct dw_edma_chunk *child, *_next;
>> +	const struct dw_edma_core_ops *ops = chan2ops(chan);
>> +
>> +	vd = vchan_next_desc(&chan->vc);
>> +	if (!vd)
>> +		return;
>> +
>> +	desc = vd2dw_edma_desc(vd);
>> +	if (!desc)
>> +		return;
>> +
>> +	list_for_each_entry_safe(child, _next, &desc->chunk->list, list) {
>> +		ops->start(child, !desc->xfer_sz);
>> +		desc->xfer_sz += child->sz;
>> +		dev_dbg(chan2dev(chan),
>> +			": transfer of %u bytes started\n", child->sz);
>> +
>> +		dw_edma_free_burst(child);
>> +		if (atomic_read(&child->bursts_alloc))
>> +			dev_dbg(chan2dev(chan),
>> +				": %d bursts still allocated\n",
>> +				atomic_read(&child->bursts_alloc));
>> +		list_del(&child->list);
>> +		kfree(child);
>> +		atomic_dec(&desc->chunks_alloc);
>> +
>> +		return;
>> +	}
>> +}
>> +
>> +static int dw_edma_device_config(struct dma_chan *dchan,
>> +				 struct dma_slave_config *config)
> 
> please align to preceding brace. Also running checkpatch with --strict
> option helps, warning checkpatch is a guidebook and not a rule book!

I didn't know that option on the checkpatch script. Cool!
I've fixed some warnings and checks with that option, but there is some false
positives there that I didn't fixed most of them related with macros (reuse and
parenthesis).

> 
> 
>> +{
>> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>> +	const struct dw_edma_core_ops *ops = chan2ops(chan);
>> +	enum dma_transfer_direction dir;
>> +	unsigned long flags;
>> +	int err = 0;
>> +
>> +	spin_lock_irqsave(&chan->vc.lock, flags);
>> +
>> +	if (!config) {
>> +		err = -EINVAL;
>> +		goto err_config;
>> +	}
>> +
>> +	if (chan->configured) {
>> +		dev_err(chan2dev(chan), ": channel already configured\n");
>> +		err = -EPERM;
>> +		goto err_config;
>> +	}
>> +
>> +	dir = config->direction;
> 
> Direction is depreciated, I have already removed the usages, so please
> do not add new ones.
> 
> You need to take direction for respective prep_ calls

Ok, I already do that. IMHO I found it strange to have the same information
repeated on two places. But now that you say that this is deprecated, it makes
sense now.

> 
>> +	if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE) {
>> +		dev_info(chan2dev(chan),
>> +			": direction DMA_DEV_TO_MEM (EDMA_DIR_WRITE)\n");
>> +		chan->p_addr = config->src_addr;
>> +	} else if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ) {
>> +		dev_info(chan2dev(chan),
>> +			": direction DMA_MEM_TO_DEV (EDMA_DIR_READ)\n");
>> +		chan->p_addr = config->dst_addr;
>> +	} else {
>> +		dev_err(chan2dev(chan), ": invalid direction\n");
>> +		err = -EINVAL;
>> +		goto err_config;
>> +	}
> 
> This should be removed

Yeah, it was just for validation purposes. Now that direction is deprecated on
the API, makes no sense to validate it.

> 
>> +
>> +	dev_info(chan2dev(chan),
>> +		": src_addr(physical) = 0x%.16x\n", config->src_addr);
>> +	dev_info(chan2dev(chan),
>> +		": dst_addr(physical) = 0x%.16x\n", config->dst_addr);
> 
> You have too many logs, it is good for bringup and initial work but not
> suited for production.

You're right. Andy Shevchenko also warned me about that. I've move pretty all
from info to dbg.

> 
>> +
>> +	err = ops->device_config(dchan);
> 
> okay what does this callback do. You are already under and dmaengine fwk
> so what is the need to add one more abstraction layer, can you explain
> that in details please

This callback just configures the eDMA HW block interrupt address (abort and
done) and data for each channel. This callback could easily moved to the
dw_edma_probe() where each channel is created at first.
Should I do it in your opinion?

> 
>> +static int dw_edma_device_pause(struct dma_chan *dchan)
>> +{
>> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>> +	unsigned long flags;
>> +	int err = 0;
>> +
>> +	spin_lock_irqsave(&chan->vc.lock, flags);
>> +
>> +	if (!chan->configured) {
>> +		dev_err(dchan2dev(dchan), ": channel not configured\n");
>> +		err = -EPERM;
>> +		goto err_pause;
>> +	}
>> +
>> +	switch (chan->status) {
>> +	case EDMA_ST_IDLE:
>> +		dev_err(dchan2dev(dchan), ": channel is idle\n");
>> +		err = -EPERM;
>> +		goto err_pause;
>> +	case EDMA_ST_PAUSE:
>> +		dev_err(dchan2dev(dchan), ": channel is already paused\n");
>> +		err = -EPERM;
>> +		goto err_pause;
>> +	case EDMA_ST_BUSY:
>> +		// Only acceptable state
>> +		break;
> 
> Doesn't it look as overkill to use switch for single acceptable case.
> Why not do
> 
>         if (chan->status != EDMA_ST_BUSY) {
>                 err = -EPERM;
>                 ...
>         }

The idea behind of this was to have more information about the current state,
but like you said before it's nice for bring up and initial work but not it not
suitable for production. I'll rework it.
> 
>> +	default:
>> +		dev_err(dchan2dev(dchan), ": invalid status state\n");
>> +		err = -EINVAL;
>> +		goto err_pause;
>> +	}
>> +
>> +	switch (chan->request) {
> 
> what is the need to track channel status and channel requests?
> 

Just to avoid mistakes from other driver that would use this driver to transfer
data to/from the device. Maybe I'm being paranoid.


Thanks Vinod for the review!