lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20180413034332.GI6014@localhost> Date: Fri, 13 Apr 2018 09:13:33 +0530 From: Vinod Koul <vinod.koul@...el.com> To: Baolin Wang <baolin.wang@...aro.org> Cc: Dan Williams <dan.j.williams@...el.com>, Eric Long <eric.long@...eadtrum.com>, Mark Brown <broonie@...nel.org>, dmaengine@...r.kernel.org, LKML <linux-kernel@...r.kernel.org> Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration 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. -- ~Vinod
Powered by blists - more mailing lists