Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
On 16 April 2018 at 23:35, Vinod Koul 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 :) -- Baolin.wang Best Regards
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
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. -- ~Vinod
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
On 16 April 2018 at 11:58, Vinod Koul wrote:
> On Fri, Apr 13, 2018 at 02:41:48PM +0800, Baolin Wang wrote:
>> On 13 April 2018 at 14:36, Vinod Koul 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.
--
Baolin.wang
Best Regards
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
On Fri, Apr 13, 2018 at 02:41:48PM +0800, Baolin Wang wrote:
> On 13 April 2018 at 14:36, Vinod Koul 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.
--
~Vinod
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
On 13 April 2018 at 18:11, Vinod Koul wrote:
> On Fri, Apr 13, 2018 at 02:41:48PM +0800, Baolin Wang wrote:
>> On 13 April 2018 at 14:36, Vinod Koul 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.
--
Baolin.wang
Best Regards
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
On Fri, Apr 13, 2018 at 02:41:48PM +0800, Baolin Wang wrote:
> On 13 April 2018 at 14:36, Vinod Koul 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
--
~Vinod
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
On 13 April 2018 at 14:36, Vinod Koul 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.
--
Baolin.wang
Best Regards
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
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...
--
~Vinod
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
On 13 April 2018 at 11:43, Vinod Koul 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
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
On 13 April 2018 at 11:39, Vinod Koul 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 0x1 bytes, one transaction transfer can support to 0xfff. -- Baolin.wang Best Regards
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
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
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
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
On 12 April 2018 at 19:30, Baolin Wang wrote:
> Hi Vinod,
>
> On 12 April 2018 at 17:37, Vinod Koul 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 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.
--
Baolin.wang
Best Regards
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
Hi Vinod,
On 12 April 2018 at 17:37, Vinod Koul 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 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.
--
Baolin.wang
Best Regards
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
On Wed, Apr 11, 2018 at 08:13:28PM +0800, Baolin Wang wrote:
> Hi Vinod,
>
> On 11 April 2018 at 17:36, Vinod Koul 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
:)
--
~Vinod
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
Hi Vinod,
On 11 April 2018 at 17:36, Vinod Koul 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.
--
Baolin.wang
Best Regards
Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
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
--
~Vinod
[PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration
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
Signed-off-by: Baolin Wang
---
drivers/dma/sprd-dma.c | 262 ++
include/linux/dma/sprd-dma.h | 25
2 files changed, 238 insertions(+), 49 deletions(-)
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_OFFSET22
#define SPRD_DMA_REQ_MODE_MASK GENMASK(1, 0)
#define SPRD_DMA_FIX_SEL_OFFSET21
#define SPRD_DMA_FIX_EN_OFFSET 20
@@ -173,6 +175,7 @@ struct sprd_dma_desc {
struct sprd_dma_chn {
struct virt_dma_chanvc;
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;
+ }
+
+
