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

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

Hi,

On 18/12/2018 03:54, Vinod Koul wrote:
> On 17-12-18, 15:56, Gustavo Pimentel wrote:
> 
>>>> +
>>>> +#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.
> 
> So in order to configure a channel you need to write to both?

No. Let me show you in a different point of view. I think we're talking about
different things.

On dw_edma_probe() ~ line 755

I've this:

SET_BOTH_CH(src_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
SET_BOTH_CH(dst_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
SET_BOTH_CH(residue_granularity, DMA_RESIDUE_GRANULARITY_DESCRIPTOR);

SET_BOTH_CH(dev, chip->dev);

SET_BOTH_CH(device_alloc_chan_resources, dw_edma_alloc_chan_resources);
SET_BOTH_CH(device_free_chan_resources, dw_edma_free_chan_resources);

SET_BOTH_CH(device_config, dw_edma_device_config);
SET_BOTH_CH(device_pause, dw_edma_device_pause);
SET_BOTH_CH(device_resume, dw_edma_device_resume);
SET_BOTH_CH(device_terminate_all, dw_edma_device_terminate_all);
SET_BOTH_CH(device_issue_pending, dw_edma_device_issue_pending);
SET_BOTH_CH(device_tx_status, dw_edma_device_tx_status);
SET_BOTH_CH(device_prep_slave_sg, dw_edma_device_prep_slave_sg);

which is equivalent to do this:

SET(dw->wr_edma, src_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
SET(dw->rd_edma, src_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
SET(dw->wr_edma, dst_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
SET(dw->rd_edma, dst_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
SET(dw->wr_edma, residue_granularity, DMA_RESIDUE_GRANULARITY_DESCRIPTOR);
SET(dw->rd_edma, residue_granularity, DMA_RESIDUE_GRANULARITY_DESCRIPTOR);

SET(dw->wr_edma, dev, chip->dev);
SET(dw->rd_edma, dev, chip->dev);

SET(dw->wr_edma, device_alloc_chan_resources, dw_edma_alloc_chan_resources);
SET(dw->rd_edma, device_alloc_chan_resources, dw_edma_alloc_chan_resources);
SET(dw->wr_edma, device_free_chan_resources, dw_edma_free_chan_resources);
SET(dw->rd_edma, device_free_chan_resources, dw_edma_free_chan_resources);

SET(dw->wr_edma, device_config, dw_edma_device_config);
SET(dw->rd_edma, device_config, dw_edma_device_config);
SET(dw->wr_edma, device_pause, dw_edma_device_pause);
SET(dw->rd_edma, device_pause, dw_edma_device_pause);
SET(dw->wr_edma, device_resume, dw_edma_device_resume);
SET(dw->rd_edma, device_resume, dw_edma_device_resume);
SET(dw->wr_edma, device_terminate_all, dw_edma_device_terminate_all);
SET(dw->rd_edma, device_terminate_all, dw_edma_device_terminate_all);
SET(dw->wr_edma, device_issue_pending, dw_edma_device_issue_pending);
SET(dw->rd_edma, device_issue_pending, dw_edma_device_issue_pending);
SET(dw->wr_edma, device_tx_status, dw_edma_device_tx_status);
SET(dw->rd_edma, device_tx_status, dw_edma_device_tx_status);
SET(dw->wr_edma, device_prep_slave_sg, dw_edma_device_prep_slave_sg);
SET(dw->rd_edma, device_prep_slave_sg, dw_edma_device_prep_slave_sg);

What you type of coding style do prefer in your opinion?
I only created the macro to simplify the visual coding view of those operations.

> 
>> Should I add some comment on top of this macro or do you think that is better to
>> replicate the code for each channel?
> 
> That will help to explain..
> 
>>>> +
>>>> +	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?
> 
> My question is about callbacks in this driver in general. You are
> already under a dmaengine fwk, so this is adding one more layer on top
> with callbacks implementing low level stiff. Why do we need another layer
> is the question..

Feel free to ask anytime.

I created a second layer, because I've knowledge that will be necessary to
implement in the future several eDMA registers mapping versions, which may or
may not require a different handling, that's why I created that layer.

Gustavo

>