| Message ID | 0c2b76aba6a49e583f920ae582d6815fa9cc4361.1523346135.git.baolin.wang@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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.
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 :)
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.
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.
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?
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.
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.
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.
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...
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.
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
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.
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.
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.
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.
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 --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