Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

[Baolin Wang <baolin.wang@linaro.org>]

Hi Vinod,

On 11 April 2018 at 17:36, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Apr 10, 2018 at 03:46:06PM +0800, Baolin Wang wrote:
>
>> +/*
>> + * struct sprd_dma_config - DMA configuration structure
>> + * @config: dma slave channel config
>> + * @fragment_len: specify one fragment transfer length
>> + * @block_len: specify one block transfer length
>> + * @transcation_len: specify one transcation transfer length
>> + * @wrap_ptr: wrap pointer address, once the transfer address reaches the
>> + * 'wrap_ptr', the next transfer address will jump to the 'wrap_to' address.
>> + * @wrap_to: wrap jump to address
>> + * @req_mode: specify the DMA request mode
>> + * @int_mode: specify the DMA interrupt type
>> + */
>> +struct sprd_dma_config {
>> +     struct dma_slave_config config;
>> +     u32 fragment_len;
>
> why not use _maxburst?

Yes, I can use maxburst.

>
>> +     u32 block_len;
>> +     u32 transcation_len;
>
> what does block and transaction len refer to here

 Our DMA has 3 transfer mode: transaction transfer, block transfer and
fragment transfer. One transaction transfer can contain several blocks
transfer, and each block can be set proper block step. One block can
contain several fragments transfer with proper fragment step. It can
generate interrupts when one transaction transfer or block transfer or
fragment transfer is completed if user set the interrupt type. So here
we should set the length for transaction transfer, block transfer and
fragment transfer.

>
>> +     phys_addr_t wrap_ptr;
>> +     phys_addr_t wrap_to;
>
> this sound sg_list to me, why are we not using that here

It is similar to sg list, but it is not one software action, we have
hardware registers to help to jump one specified address.

>
>> +     enum sprd_dma_req_mode req_mode;
>
> Looking at definition of request mode we have frag, block, transaction list
> etc.. That should depend upon dma request. If you have been asked to
> transfer a list, you shall configure list mode. if it is a single
> transaction then it should be transaction mode!

If I understand your points correctly, you mean we can specify the
request mode when requesting one slave channel by
'dma_request_slave_channel()'. But we need change the request mode
dynamically following different transfer task for this channel, so I
am afraid we can not specify the request mode of this channel at
requesting time.

>
>> +     enum sprd_dma_int_type int_mode;
>
> Here again I think driver needs to take a call based on dma_ctrl_flags.

The 'dma_ctrl_flags' defines DMA_PREP_INTERRUPT flag to indicate if a
interrupt is needed after transfer, but it can not distinguish
Spreadtrum special interrupt type of DMA. So can I pass the interrupt
type as one parameter for 'device_prep_slave_sg' interface?

Very appreciated for your useful comments.

-- 
Baolin.wang
Best Regards