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

[Baolin Wang]

Hi Vinod,

On 12 April 2018 at 17:37, Vinod Koul <vinod.koul@xxxxxxxxx> wrote:
> On Wed, Apr 11, 2018 at 08:13:28PM +0800, Baolin Wang wrote:
>> Hi Vinod,
>>
>> On 11 April 2018 at 17:36, Vinod Koul <vinod.koul@xxxxxxxxx> 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.
>
> what are the max size these types support?

These types max size definition:

#define SPRD_DMA_FRG_LEN_MASK GENMASK(16, 0)

#define SPRD_DMA_BLK_LEN_MASK GENMASK(16, 0)

#define SPRD_DMA_TRSC_LEN_MASK GENMASK(27, 0)

>>
>> >
>> >> +     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.
>
> Nope a channel has nothing to do with request type. You request and grab a
> channel. Then you prepare a descriptor for a dma transaction. Based on
> transaction requested you should intelligently break it down and create a
> descriptor which uses transaction/block/fragment so that DMA throughput is
> efficient. If prepare has sg list then you should use link list mode.
> Further if you support max length, say 16KB and request is for 20KB you may
> break it down for link list with two segments.

OK. So I can add one more cell to specify the request mode for this channel.

dmas = <&apdma 11 SPRD_DMA_BLK_REQ>

>
> Each prep call has flags associated, that can help you configure interrupt
> behaviour.

Sounds reasonable. Thanks for your comments.

-- 
Baolin.wang
Best Regards