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

[Baolin Wang]

On 13 April 2018 at 11:43, Vinod Koul <vinod.koul@xxxxxxxxx> wrote:
> On Thu, Apr 12, 2018 at 07:36:34PM +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.
>>
>> After more thinking, I think this will be not useful for users, since
>> users need configure one DMA channel through different 3 places,
>> specify the request mode in dts, specify the interrupt type through
>> prep call flags, and other configuration need to be configured through
>> 'struct sprd_dma_config'. That is not one good design for users. What
>> do you think? Thanks.
>
> Agreed, users only care about grabbing a channel, setting a descriptor and
> submitting that.
>
> I think you need to go back and think about this a bit, please do go thru
> dmaengine documentation and see other driver examples.
>
> We don't typically expose these to users, they give us a transfer and we set
> that up in hardware for efficient. Its DMA so people expect us to use fastest
> mechanism available.

But there are some configuration are really special for Spreadtrum
DMA, and must need user to specify how to configure, especially some
scenarios of audio. So I wander if we can add one pointer for
'dma_slave_config' to expand some special DMA configuration
requirements, like:

struct dma_slave_config {
    ......
    unsigned int slave_id;
    void *platform_data;
};

So if some DMA has some special configuration (such as Spreadtrum
DMA), they can user this platform_data pointer. Like xilinx DMA, they
also have some special configuration.

>
> I would say setup as Link list for sg transfers and use one of them modes
> (again think efficiency) for single transfers.
>
> Also use all the parameters in dma_slave_config and also setup capabilities if
> not done.

-- 
Baolin.wang
Best Regards