diff mbox

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

Message ID 0c2b76aba6a49e583f920ae582d6815fa9cc4361.1523346135.git.baolin.wang@linaro.org (mailing list archive)
State Changes Requested
Headers show

Commit Message

(Exiting) Baolin Wang April 10, 2018, 7:46 a.m. UTC
There are some Spreadtrum special configuration for DMA controller,
thus this patch adds one 'struct sprd_dma_config' structure for users
to configure.

Moreover this patch did some optimization for sprd_dma_config() and
sprd_dma_prep_dma_memcpy() to prepare to configure DMA from users.

Signed-off-by: Eric Long <eric.long@spreadtrum.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/dma/sprd-dma.c       |  262 ++++++++++++++++++++++++++++++++++--------
 include/linux/dma/sprd-dma.h |   25 ++++
 2 files changed, 238 insertions(+), 49 deletions(-)

Comments

Vinod Koul April 11, 2018, 9:36 a.m. UTC | #1
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?

> +	u32 block_len;
> +	u32 transcation_len;

what does block and transaction len refer to here

> +	phys_addr_t wrap_ptr;
> +	phys_addr_t wrap_to;

this sound sg_list to me, why are we not using that here

> +	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!

> +	enum sprd_dma_int_type int_mode;

Here again I think driver needs to take a call based on dma_ctrl_flags.

Okay I dont think we are proceeding in right direction on this one. This
seems to be fairly generic dma controller and in line with other IP blocks
and you should take reference from those one. I dont think we need this
configuration and can do with generic api and configuration provided.

Thanks
(Exiting) Baolin Wang April 11, 2018, 12:13 p.m. UTC | #2
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.
Vinod Koul April 12, 2018, 9:37 a.m. UTC | #3
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@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.

what are the max size these types support?

> 
> >
> >> +     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.

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

Btw other dma controllers have similar capabilities and driver manages these
:)
(Exiting) Baolin Wang April 12, 2018, 11:30 a.m. UTC | #4
Hi Vinod,

On 12 April 2018 at 17:37, Vinod Koul <vinod.koul@intel.com> 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@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.
>
> 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.
(Exiting) Baolin Wang April 12, 2018, 11:36 a.m. UTC | #5
On 12 April 2018 at 19:30, Baolin Wang <baolin.wang@linaro.org> wrote:
> Hi Vinod,
>
> On 12 April 2018 at 17:37, Vinod Koul <vinod.koul@intel.com> 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@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.
>>
>> 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.
Vinod Koul April 13, 2018, 3:39 a.m. UTC | #6
On Thu, Apr 12, 2018 at 07:30:01PM +0800, Baolin Wang wrote:

> >> > 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)

They are register defines. How many items or bytes do each type of txn
support?
Vinod Koul April 13, 2018, 3:43 a.m. UTC | #7
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.

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.
(Exiting) Baolin Wang April 13, 2018, 5:44 a.m. UTC | #8
On 13 April 2018 at 11:39, Vinod Koul <vinod.koul@intel.com> wrote:
> On Thu, Apr 12, 2018 at 07:30:01PM +0800, Baolin Wang wrote:
>
>> >> > 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)
>
> They are register defines. How many items or bytes do each type of txn
> support?

These macros are the max size definitions, for example one fragment
length can support to 0x1ffff bytes, one transaction transfer can
support to 0xfffffff.
(Exiting) Baolin Wang April 13, 2018, 6:17 a.m. UTC | #9
On 13 April 2018 at 11:43, Vinod Koul <vinod.koul@intel.com> 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.
Vinod Koul April 13, 2018, 6:36 a.m. UTC | #10
On Fri, Apr 13, 2018 at 02:17:34PM +0800, Baolin Wang wrote:

> > 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.

Well we all think our HW is special and needs some additional stuff, most of
the cases turns out not to be the case.

Can you explain how audio in this case additional configuration...
(Exiting) Baolin Wang April 13, 2018, 6:41 a.m. UTC | #11
On 13 April 2018 at 14:36, Vinod Koul <vinod.koul@intel.com> wrote:
> On Fri, Apr 13, 2018 at 02:17:34PM +0800, Baolin Wang wrote:
>
>> > 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.
>
> Well we all think our HW is special and needs some additional stuff, most of
> the cases turns out not to be the case.
>
> Can you explain how audio in this case additional configuration...
>

Beside the general configuration, our audio driver will configure the
fragment length, block length, maybe transaction length, and they must
specify the request type and interrupt type, these are what we want to
export for users.
Vinod Koul April 13, 2018, 10:11 a.m. UTC | #12
On Fri, Apr 13, 2018 at 02:41:48PM +0800, Baolin Wang wrote:
> On 13 April 2018 at 14:36, Vinod Koul <vinod.koul@intel.com> wrote:
> > On Fri, Apr 13, 2018 at 02:17:34PM +0800, Baolin Wang wrote:
> >
> >> > 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.
> >
> > Well we all think our HW is special and needs some additional stuff, most of
> > the cases turns out not to be the case.
> >
> > Can you explain how audio in this case additional configuration...
> >
> 
> Beside the general configuration, our audio driver will configure the
> fragment length, block length, maybe transaction length, and they must
> specify the request type and interrupt type, these are what we want to
> export for users.

First doesn't it use sound dmaengine library, it should :)

Second, I think you should calculate the lengths based on given input. Audio
is circular buffer so you shall create a circular linked list and submit.
See how other driver implement circular prepare callback
(Exiting) Baolin Wang April 13, 2018, 10:48 a.m. UTC | #13
On 13 April 2018 at 18:11, Vinod Koul <vinod.koul@intel.com> wrote:
> On Fri, Apr 13, 2018 at 02:41:48PM +0800, Baolin Wang wrote:
>> On 13 April 2018 at 14:36, Vinod Koul <vinod.koul@intel.com> wrote:
>> > On Fri, Apr 13, 2018 at 02:17:34PM +0800, Baolin Wang wrote:
>> >
>> >> > 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.
>> >
>> > Well we all think our HW is special and needs some additional stuff, most of
>> > the cases turns out not to be the case.
>> >
>> > Can you explain how audio in this case additional configuration...
>> >
>>
>> Beside the general configuration, our audio driver will configure the
>> fragment length, block length, maybe transaction length, and they must
>> specify the request type and interrupt type, these are what we want to
>> export for users.
>
> First doesn't it use sound dmaengine library, it should :)

I do not think so. That's the DMA configuration need to set before
audio use it. Not only audio, but also other drivers using DMA need to
configure these configuration what we exported in sprd_dma_config.

>
> Second, I think you should calculate the lengths based on given input. Audio
> is circular buffer so you shall create a circular linked list and submit.
> See how other driver implement circular prepare callback

Yes,  we have not implemented the link list mode for audio now, but in
our plan. So firstly we want to export these necessary configuration
for users to configure.
Vinod Koul April 16, 2018, 3:58 a.m. UTC | #14
On Fri, Apr 13, 2018 at 02:41:48PM +0800, Baolin Wang wrote:
> On 13 April 2018 at 14:36, Vinod Koul <vinod.koul@intel.com> wrote:
> > On Fri, Apr 13, 2018 at 02:17:34PM +0800, Baolin Wang wrote:
> >
> >> > 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.
> >
> > Well we all think our HW is special and needs some additional stuff, most of
> > the cases turns out not to be the case.
> >
> > Can you explain how audio in this case additional configuration...
> 
> Beside the general configuration, our audio driver will configure the
> fragment length, block length, maybe transaction length, and they must
> specify the request type and interrupt type, these are what we want to
> export for users.

As I said before, you need to derive fragment, block or transaction from
given prep_cyclic values. Interrupt type needs to be derived with the flags
passed. Please do see how other drivers make use of it.
(Exiting) Baolin Wang April 16, 2018, 6:32 a.m. UTC | #15
On 16 April 2018 at 11:58, Vinod Koul <vinod.koul@intel.com> wrote:
> On Fri, Apr 13, 2018 at 02:41:48PM +0800, Baolin Wang wrote:
>> On 13 April 2018 at 14:36, Vinod Koul <vinod.koul@intel.com> wrote:
>> > On Fri, Apr 13, 2018 at 02:17:34PM +0800, Baolin Wang wrote:
>> >
>> >> > 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.
>> >
>> > Well we all think our HW is special and needs some additional stuff, most of
>> > the cases turns out not to be the case.
>> >
>> > Can you explain how audio in this case additional configuration...
>>
>> Beside the general configuration, our audio driver will configure the
>> fragment length, block length, maybe transaction length, and they must
>> specify the request type and interrupt type, these are what we want to
>> export for users.
>
> As I said before, you need to derive fragment, block or transaction from

I am sorry I did not make things clear here. What I mean is not only
link list mode(prep_cyclic), but also other modes (prep_slave,
prep_interleaved ...), users still need to configure the fragment
length, block length or transaction length according to their
requirements.

> given prep_cyclic values. Interrupt type needs to be derived with the flags
> passed. Please do see how other drivers make use of it.

Fine. We configure the Interrupt type through the flags passed.
Vinod Koul April 16, 2018, 3:35 p.m. UTC | #16
On Mon, Apr 16, 2018 at 02:32:05PM +0800, Baolin Wang wrote:

> >> Beside the general configuration, our audio driver will configure the
> >> fragment length, block length, maybe transaction length, and they must
> >> specify the request type and interrupt type, these are what we want to
> >> export for users.
> >
> > As I said before, you need to derive fragment, block or transaction from
> 
> I am sorry I did not make things clear here. What I mean is not only
> link list mode(prep_cyclic), but also other modes (prep_slave,
> prep_interleaved ...), users still need to configure the fragment
> length, block length or transaction length according to their
> requirements.

Other modes are similar, they also use the parameters programmed from
dma_slave_config. Pls see other driver examples. IIRC dw dma has
similar capabilities but not all are exposed to user and driver configures.
(Exiting) Baolin Wang April 17, 2018, 6:06 a.m. UTC | #17
On 16 April 2018 at 23:35, Vinod Koul <vinod.koul@intel.com> wrote:
> On Mon, Apr 16, 2018 at 02:32:05PM +0800, Baolin Wang wrote:
>
>> >> Beside the general configuration, our audio driver will configure the
>> >> fragment length, block length, maybe transaction length, and they must
>> >> specify the request type and interrupt type, these are what we want to
>> >> export for users.
>> >
>> > As I said before, you need to derive fragment, block or transaction from
>>
>> I am sorry I did not make things clear here. What I mean is not only
>> link list mode(prep_cyclic), but also other modes (prep_slave,
>> prep_interleaved ...), users still need to configure the fragment
>> length, block length or transaction length according to their
>> requirements.
>
> Other modes are similar, they also use the parameters programmed from
> dma_slave_config. Pls see other driver examples. IIRC dw dma has
> similar capabilities but not all are exposed to user and driver configures.

I'll check the dw dma. Really thanks for your help :)
diff mbox

Patch

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 5c26fde..f8038de 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -100,6 +100,8 @@ 
 #define SPRD_DMA_DES_DATAWIDTH_OFFSET	28
 #define SPRD_DMA_SWT_MODE_OFFSET	26
 #define SPRD_DMA_REQ_MODE_OFFSET	24
+#define SPRD_DMA_WRAP_SEL_OFFSET	23
+#define SPRD_DMA_WRAP_EN_OFFSET		22
 #define SPRD_DMA_REQ_MODE_MASK		GENMASK(1, 0)
 #define SPRD_DMA_FIX_SEL_OFFSET		21
 #define SPRD_DMA_FIX_EN_OFFSET		20
@@ -173,6 +175,7 @@  struct sprd_dma_desc {
 struct sprd_dma_chn {
 	struct virt_dma_chan	vc;
 	void __iomem		*chn_base;
+	struct sprd_dma_config	slave_cfg;
 	u32			chn_num;
 	u32			dev_id;
 	struct sprd_dma_desc	*cur_desc;
@@ -561,52 +564,162 @@  static void sprd_dma_issue_pending(struct dma_chan *chan)
 	spin_unlock_irqrestore(&schan->vc.lock, flags);
 }
 
+static enum sprd_dma_datawidth
+sprd_dma_get_datawidth(enum dma_slave_buswidth buswidth)
+{
+	switch (buswidth) {
+	case DMA_SLAVE_BUSWIDTH_1_BYTE:
+		return SPRD_DMA_DATAWIDTH_1_BYTE;
+
+	case DMA_SLAVE_BUSWIDTH_2_BYTES:
+		return SPRD_DMA_DATAWIDTH_2_BYTES;
+
+	case DMA_SLAVE_BUSWIDTH_4_BYTES:
+		return SPRD_DMA_DATAWIDTH_4_BYTES;
+
+	case DMA_SLAVE_BUSWIDTH_8_BYTES:
+		return SPRD_DMA_DATAWIDTH_8_BYTES;
+
+	default:
+		return SPRD_DMA_DATAWIDTH_4_BYTES;
+	}
+}
+
+static int sprd_dma_get_step(enum dma_slave_buswidth buswidth,
+			     enum dma_transfer_direction dir,
+			     enum sprd_dma_step *src_step,
+			     enum sprd_dma_step *dst_step)
+{
+	switch (dir) {
+	case DMA_MEM_TO_MEM:
+		switch (buswidth) {
+		case DMA_SLAVE_BUSWIDTH_1_BYTE:
+			*src_step = SPRD_DMA_BYTE_STEP;
+			*dst_step = SPRD_DMA_BYTE_STEP;
+			break;
+
+		case DMA_SLAVE_BUSWIDTH_2_BYTES:
+			*src_step = SPRD_DMA_SHORT_STEP;
+			*dst_step = SPRD_DMA_SHORT_STEP;
+			break;
+
+		case DMA_SLAVE_BUSWIDTH_4_BYTES:
+			*src_step = SPRD_DMA_WORD_STEP;
+			*dst_step = SPRD_DMA_WORD_STEP;
+			break;
+
+		case DMA_SLAVE_BUSWIDTH_8_BYTES:
+			*src_step = SPRD_DMA_DWORD_STEP;
+			*dst_step = SPRD_DMA_DWORD_STEP;
+			break;
+
+		default:
+			*src_step = SPRD_DMA_WORD_STEP;
+			*dst_step = SPRD_DMA_WORD_STEP;
+			break;
+		}
+		break;
+
+	case DMA_MEM_TO_DEV:
+		switch (buswidth) {
+		case DMA_SLAVE_BUSWIDTH_1_BYTE:
+			*src_step = SPRD_DMA_BYTE_STEP;
+			break;
+
+		case DMA_SLAVE_BUSWIDTH_2_BYTES:
+			*src_step = SPRD_DMA_SHORT_STEP;
+			break;
+
+		case DMA_SLAVE_BUSWIDTH_4_BYTES:
+			*src_step = SPRD_DMA_WORD_STEP;
+			break;
+
+		case DMA_SLAVE_BUSWIDTH_8_BYTES:
+			*src_step = SPRD_DMA_DWORD_STEP;
+			break;
+
+		default:
+			*src_step = SPRD_DMA_WORD_STEP;
+			break;
+		}
+
+		*dst_step = SPRD_DMA_NONE_STEP;
+		break;
+
+	case DMA_DEV_TO_MEM:
+		switch (buswidth) {
+		case DMA_SLAVE_BUSWIDTH_1_BYTE:
+			*dst_step = SPRD_DMA_BYTE_STEP;
+			break;
+
+		case DMA_SLAVE_BUSWIDTH_2_BYTES:
+			*dst_step = SPRD_DMA_SHORT_STEP;
+			break;
+
+		case DMA_SLAVE_BUSWIDTH_4_BYTES:
+			*dst_step = SPRD_DMA_WORD_STEP;
+			break;
+
+		case DMA_SLAVE_BUSWIDTH_8_BYTES:
+			*dst_step = SPRD_DMA_DWORD_STEP;
+			break;
+
+		default:
+			*dst_step = SPRD_DMA_WORD_STEP;
+			break;
+		}
+
+		*src_step = SPRD_DMA_NONE_STEP;
+		break;
+
+	case DMA_DEV_TO_DEV:
+		*src_step = SPRD_DMA_NONE_STEP;
+		*dst_step = SPRD_DMA_NONE_STEP;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
-			   dma_addr_t dest, dma_addr_t src, size_t len)
+			   struct sprd_dma_config *slave_cfg)
 {
 	struct sprd_dma_dev *sdev = to_sprd_dma_dev(chan);
+	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
 	struct sprd_dma_chn_hw *hw = &sdesc->chn_hw;
-	u32 datawidth, src_step, des_step, fragment_len;
-	u32 block_len, req_mode, irq_mode, transcation_len;
-	u32 fix_mode = 0, fix_en = 0;
+	u32 fix_mode = 0, fix_en = 0, wrap_en = 0, wrap_mode = 0;
+	enum sprd_dma_step src_step, dst_step;
+	enum sprd_dma_datawidth src_datawidth, dst_datawidth;
+	int ret;
 
-	if (IS_ALIGNED(len, 4)) {
-		datawidth = SPRD_DMA_DATAWIDTH_4_BYTES;
-		src_step = SPRD_DMA_WORD_STEP;
-		des_step = SPRD_DMA_WORD_STEP;
-	} else if (IS_ALIGNED(len, 2)) {
-		datawidth = SPRD_DMA_DATAWIDTH_2_BYTES;
-		src_step = SPRD_DMA_SHORT_STEP;
-		des_step = SPRD_DMA_SHORT_STEP;
-	} else {
-		datawidth = SPRD_DMA_DATAWIDTH_1_BYTE;
-		src_step = SPRD_DMA_BYTE_STEP;
-		des_step = SPRD_DMA_BYTE_STEP;
+	ret = sprd_dma_get_step(slave_cfg->config.src_addr_width,
+				slave_cfg->config.direction,
+				&src_step, &dst_step);
+	if (ret) {
+		dev_err(sdev->dma_dev.dev, "invalid step values\n");
+		return ret;
 	}
 
-	fragment_len = SPRD_DMA_MEMCPY_MIN_SIZE;
-	if (len <= SPRD_DMA_BLK_LEN_MASK) {
-		block_len = len;
-		transcation_len = 0;
-		req_mode = SPRD_DMA_BLK_REQ;
-		irq_mode = SPRD_DMA_BLK_INT;
-	} else {
-		block_len = SPRD_DMA_MEMCPY_MIN_SIZE;
-		transcation_len = len;
-		req_mode = SPRD_DMA_TRANS_REQ;
-		irq_mode = SPRD_DMA_TRANS_INT;
-	}
+	if (slave_cfg->config.slave_id)
+		schan->dev_id = slave_cfg->config.slave_id;
 
 	hw->cfg = SPRD_DMA_DONOT_WAIT_BDONE << SPRD_DMA_WAIT_BDONE_OFFSET;
-	hw->wrap_ptr = (u32)((src >> SPRD_DMA_HIGH_ADDR_OFFSET) &
-			     SPRD_DMA_HIGH_ADDR_MASK);
-	hw->wrap_to = (u32)((dest >> SPRD_DMA_HIGH_ADDR_OFFSET) &
-			    SPRD_DMA_HIGH_ADDR_MASK);
-
-	hw->src_addr = (u32)(src & SPRD_DMA_LOW_ADDR_MASK);
-	hw->des_addr = (u32)(dest & SPRD_DMA_LOW_ADDR_MASK);
-
-	if ((src_step != 0 && des_step != 0) || (src_step | des_step) == 0) {
+	hw->wrap_ptr = (u32)((slave_cfg->wrap_ptr & SPRD_DMA_LOW_ADDR_MASK) |
+		((slave_cfg->config.src_addr >> SPRD_DMA_HIGH_ADDR_OFFSET) &
+		 SPRD_DMA_HIGH_ADDR_MASK));
+	hw->wrap_to = (u32)((slave_cfg->wrap_to & SPRD_DMA_LOW_ADDR_MASK) |
+		((slave_cfg->config.dst_addr >> SPRD_DMA_HIGH_ADDR_OFFSET) &
+		 SPRD_DMA_HIGH_ADDR_MASK));
+
+	hw->src_addr =
+		(u32)(slave_cfg->config.src_addr & SPRD_DMA_LOW_ADDR_MASK);
+	hw->des_addr =
+		(u32)(slave_cfg->config.dst_addr & SPRD_DMA_LOW_ADDR_MASK);
+
+	if ((src_step != 0 && dst_step != 0) || (src_step | dst_step) == 0) {
 		fix_en = 0;
 	} else {
 		fix_en = 1;
@@ -616,17 +729,37 @@  static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
 			fix_mode = 0;
 	}
 
-	hw->frg_len = datawidth << SPRD_DMA_SRC_DATAWIDTH_OFFSET |
-		datawidth << SPRD_DMA_DES_DATAWIDTH_OFFSET |
-		req_mode << SPRD_DMA_REQ_MODE_OFFSET |
+	if (slave_cfg->wrap_ptr && slave_cfg->wrap_to) {
+		wrap_en = 1;
+		if (slave_cfg->wrap_to == slave_cfg->config.src_addr) {
+			wrap_mode = 0;
+		} else if (slave_cfg->wrap_to == slave_cfg->config.dst_addr) {
+			wrap_mode = 1;
+		} else {
+			dev_err(sdev->dma_dev.dev, "invalid wrap mode\n");
+			return -EINVAL;
+		}
+	}
+
+	src_datawidth =
+		sprd_dma_get_datawidth(slave_cfg->config.src_addr_width);
+	dst_datawidth =
+		sprd_dma_get_datawidth(slave_cfg->config.dst_addr_width);
+
+	hw->frg_len = src_datawidth << SPRD_DMA_SRC_DATAWIDTH_OFFSET |
+		dst_datawidth << SPRD_DMA_DES_DATAWIDTH_OFFSET |
+		slave_cfg->req_mode << SPRD_DMA_REQ_MODE_OFFSET |
+		wrap_mode << SPRD_DMA_WRAP_SEL_OFFSET |
+		wrap_en << SPRD_DMA_WRAP_EN_OFFSET |
 		fix_mode << SPRD_DMA_FIX_SEL_OFFSET |
 		fix_en << SPRD_DMA_FIX_EN_OFFSET |
-		(fragment_len & SPRD_DMA_FRG_LEN_MASK);
-	hw->blk_len = block_len & SPRD_DMA_BLK_LEN_MASK;
+		(slave_cfg->fragment_len & SPRD_DMA_FRG_LEN_MASK);
+
+	hw->blk_len = slave_cfg->block_len & SPRD_DMA_BLK_LEN_MASK;
 
 	hw->intc = SPRD_DMA_CFG_ERR_INT_EN;
 
-	switch (irq_mode) {
+	switch (slave_cfg->int_mode) {
 	case SPRD_DMA_NO_INT:
 		break;
 
@@ -667,12 +800,13 @@  static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
 		return -EINVAL;
 	}
 
-	if (transcation_len == 0)
-		hw->trsc_len = block_len & SPRD_DMA_TRSC_LEN_MASK;
+	if (slave_cfg->transcation_len == 0)
+		hw->trsc_len = slave_cfg->block_len & SPRD_DMA_TRSC_LEN_MASK;
 	else
-		hw->trsc_len = transcation_len & SPRD_DMA_TRSC_LEN_MASK;
+		hw->trsc_len =
+			slave_cfg->transcation_len & SPRD_DMA_TRSC_LEN_MASK;
 
-	hw->trsf_step = (des_step & SPRD_DMA_TRSF_STEP_MASK) <<
+	hw->trsf_step = (dst_step & SPRD_DMA_TRSF_STEP_MASK) <<
 			SPRD_DMA_DEST_TRSF_STEP_OFFSET |
 			(src_step & SPRD_DMA_TRSF_STEP_MASK) <<
 			SPRD_DMA_SRC_TRSF_STEP_OFFSET;
@@ -680,7 +814,6 @@  static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
 	hw->frg_step = 0;
 	hw->src_blk_step = 0;
 	hw->des_blk_step = 0;
-	hw->src_blk_step = 0;
 	return 0;
 }
 
@@ -689,6 +822,7 @@  static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
 			 size_t len, unsigned long flags)
 {
 	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
+	struct sprd_dma_config *slave_cfg = &schan->slave_cfg;
 	struct sprd_dma_desc *sdesc;
 	int ret;
 
@@ -696,7 +830,37 @@  static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
 	if (!sdesc)
 		return NULL;
 
-	ret = sprd_dma_config(chan, sdesc, dest, src, len);
+	memset(slave_cfg, 0, sizeof(*slave_cfg));
+
+	slave_cfg->config.src_addr = src;
+	slave_cfg->config.dst_addr = dest;
+	slave_cfg->config.direction = DMA_MEM_TO_MEM;
+
+	if (IS_ALIGNED(len, 4)) {
+		slave_cfg->config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		slave_cfg->config.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+	} else if (IS_ALIGNED(len, 2)) {
+		slave_cfg->config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+		slave_cfg->config.dst_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+	} else {
+		slave_cfg->config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+		slave_cfg->config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	}
+
+	slave_cfg->fragment_len = SPRD_DMA_MEMCPY_MIN_SIZE;
+	if (len <= SPRD_DMA_BLK_LEN_MASK) {
+		slave_cfg->block_len = len;
+		slave_cfg->transcation_len = 0;
+		slave_cfg->req_mode = SPRD_DMA_BLK_REQ;
+		slave_cfg->int_mode = SPRD_DMA_BLK_INT;
+	} else {
+		slave_cfg->block_len = SPRD_DMA_MEMCPY_MIN_SIZE;
+		slave_cfg->transcation_len = len;
+		slave_cfg->req_mode = SPRD_DMA_TRANS_REQ;
+		slave_cfg->int_mode = SPRD_DMA_TRANS_INT;
+	}
+
+	ret = sprd_dma_config(chan, sdesc, slave_cfg);
 	if (ret) {
 		kfree(sdesc);
 		return NULL;
diff --git a/include/linux/dma/sprd-dma.h b/include/linux/dma/sprd-dma.h
index c545162..8bda7d7 100644
--- a/include/linux/dma/sprd-dma.h
+++ b/include/linux/dma/sprd-dma.h
@@ -3,6 +3,8 @@ 
 #ifndef _SPRD_DMA_H_
 #define _SPRD_DMA_H_
 
+#include <linux/dmaengine.h>
+
 /*
  * enum sprd_dma_req_mode: define the DMA request mode
  * @SPRD_DMA_FRAG_REQ: fragment request mode
@@ -54,4 +56,27 @@  enum sprd_dma_int_type {
 	SPRD_DMA_CFGERR_INT,
 };
 
+/*
+ * 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;
+	u32 block_len;
+	u32 transcation_len;
+	phys_addr_t wrap_ptr;
+	phys_addr_t wrap_to;
+	enum sprd_dma_req_mode req_mode;
+	enum sprd_dma_int_type int_mode;
+};
+
 #endif