From: Baolin Wang <baolin.wang@linaro.org> To: Vinod Koul <vinod.koul@intel.com> Cc: Dan Williams <dan.j.williams@intel.com>, Eric Long <eric.long@spreadtrum.com>, Mark Brown <broonie@kernel.org>, dmaengine@vger.kernel.org, LKML <linux-kernel@vger.kernel.org> Subject: [4/5] dmaengine: sprd: Add Spreadtrum DMA configuration Date: Fri, 13 Apr 2018 14:17:34 +0800 [thread overview] Message-ID: <CAMz4kuL_i7u5OCXrtJy92=OisOzgbWk_e-S9dSuBRPenX=ScPA@mail.gmail.com> (raw) 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.
WARNING: multiple messages have this Message-ID (diff)
From: Baolin Wang <baolin.wang@linaro.org> To: Vinod Koul <vinod.koul@intel.com> Cc: Dan Williams <dan.j.williams@intel.com>, Eric Long <eric.long@spreadtrum.com>, Mark Brown <broonie@kernel.org>, dmaengine@vger.kernel.org, LKML <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration Date: Fri, 13 Apr 2018 14:17:34 +0800 [thread overview] Message-ID: <CAMz4kuL_i7u5OCXrtJy92=OisOzgbWk_e-S9dSuBRPenX=ScPA@mail.gmail.com> (raw) In-Reply-To: <20180413034332.GI6014@localhost> 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. -- Baolin.wang Best Regards
next reply other threads:[~2018-04-13 6:17 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-13 6:17 Baolin Wang [this message] 2018-04-13 6:17 ` [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration Baolin Wang -- strict thread matches above, loose matches on Subject: below -- 2018-04-18 5:40 [5/5] dmaengine: sprd: Add 'device_config' and 'device_prep_slave_sg' interfaces Baolin Wang 2018-04-18 5:40 ` [PATCH 5/5] " Baolin Wang 2018-04-17 10:45 [5/5] " Lars-Peter Clausen 2018-04-17 10:45 ` [PATCH 5/5] " Lars-Peter Clausen 2018-04-17 6:06 [4/5] dmaengine: sprd: Add Spreadtrum DMA configuration Baolin Wang 2018-04-17 6:06 ` [PATCH 4/5] " Baolin Wang 2018-04-16 15:35 [4/5] " Vinod Koul 2018-04-16 15:35 ` [PATCH 4/5] " Vinod Koul 2018-04-16 6:32 [4/5] " Baolin Wang 2018-04-16 6:32 ` [PATCH 4/5] " Baolin Wang 2018-04-16 3:58 [4/5] " Vinod Koul 2018-04-16 3:58 ` [PATCH 4/5] " Vinod Koul 2018-04-13 10:48 [4/5] " Baolin Wang 2018-04-13 10:48 ` [PATCH 4/5] " Baolin Wang 2018-04-13 10:11 [4/5] " Vinod Koul 2018-04-13 10:11 ` [PATCH 4/5] " Vinod Koul 2018-04-13 6:41 [4/5] " Baolin Wang 2018-04-13 6:41 ` [PATCH 4/5] " Baolin Wang 2018-04-13 6:36 [4/5] " Vinod Koul 2018-04-13 6:36 ` [PATCH 4/5] " Vinod Koul 2018-04-13 5:44 [4/5] " Baolin Wang 2018-04-13 5:44 ` [PATCH 4/5] " Baolin Wang 2018-04-13 3:43 [4/5] " Vinod Koul 2018-04-13 3:43 ` [PATCH 4/5] " Vinod Koul 2018-04-13 3:39 [4/5] " Vinod Koul 2018-04-13 3:39 ` [PATCH 4/5] " Vinod Koul 2018-04-12 11:36 [4/5] " Baolin Wang 2018-04-12 11:36 ` [PATCH 4/5] " Baolin Wang 2018-04-12 11:30 [4/5] " Baolin Wang 2018-04-12 11:30 ` [PATCH 4/5] " Baolin Wang 2018-04-12 9:37 [4/5] " Vinod Koul 2018-04-12 9:37 ` [PATCH 4/5] " Vinod Koul 2018-04-11 12:13 [4/5] " Baolin Wang 2018-04-11 12:13 ` [PATCH 4/5] " Baolin Wang 2018-04-11 10:51 [5/5] dmaengine: sprd: Add 'device_config' and 'device_prep_slave_sg' interfaces Baolin Wang 2018-04-11 10:51 ` [PATCH 5/5] " Baolin Wang 2018-04-11 10:49 [1/5] dmaengine: sprd: Define the DMA transfer step type Baolin Wang 2018-04-11 10:49 ` [PATCH 1/5] " Baolin Wang 2018-04-11 9:40 [5/5] dmaengine: sprd: Add 'device_config' and 'device_prep_slave_sg' interfaces Vinod Koul 2018-04-11 9:40 ` [PATCH 5/5] " Vinod Koul 2018-04-11 9:36 [4/5] dmaengine: sprd: Add Spreadtrum DMA configuration Vinod Koul 2018-04-11 9:36 ` [PATCH 4/5] " Vinod Koul 2018-04-11 9:24 [1/5] dmaengine: sprd: Define the DMA transfer step type Vinod Koul 2018-04-11 9:24 ` [PATCH 1/5] " Vinod Koul 2018-04-10 7:46 [5/5] dmaengine: sprd: Add 'device_config' and 'device_prep_slave_sg' interfaces Baolin Wang 2018-04-10 7:46 ` [PATCH 5/5] " Baolin Wang 2018-04-10 7:46 [4/5] dmaengine: sprd: Add Spreadtrum DMA configuration Baolin Wang 2018-04-10 7:46 ` [PATCH 4/5] " Baolin Wang 2018-04-10 7:46 [3/5] dmaengine: sprd: Move DMA request mode and interrupt type into head file Baolin Wang 2018-04-10 7:46 ` [PATCH 3/5] " Baolin Wang 2018-04-10 7:46 [2/5] dmaengine: sprd: Define the DMA data width type Baolin Wang 2018-04-10 7:46 ` [PATCH 2/5] " Baolin Wang 2018-04-10 7:46 [1/5] dmaengine: sprd: Define the DMA transfer step type Baolin Wang 2018-04-10 7:46 ` [PATCH 1/5] " Baolin Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAMz4kuL_i7u5OCXrtJy92=OisOzgbWk_e-S9dSuBRPenX=ScPA@mail.gmail.com' \ --to=baolin.wang@linaro.org \ --cc=broonie@kernel.org \ --cc=dan.j.williams@intel.com \ --cc=dmaengine@vger.kernel.org \ --cc=eric.long@spreadtrum.com \ --cc=linux-kernel@vger.kernel.org \ --cc=vinod.koul@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.