Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Vinod Koul]

On Tue, Nov 01, 2016 at 10:55:13PM +0800, Chen-Yu Tsai wrote:

> >>  * @src_maxburst: the maximum number of words (note: words, as in
> >>  * units of the src_addr_width member, not bytes) that can be sent
> >>  * in one burst to the device. Typically something like half the
> >>  * FIFO depth on I/O peripherals so you don't overflow it. This
> >>  * may or may not be applicable on memory sources.
> >>  * @dst_maxburst: same as src_maxburst but for destination target
> >>  * mutatis mutandis.
> >>
> >> The DMA engine driver should be free to select whatever burst size
> >> that doesn't exceed this. So for max_burst = 4, the driver can select
> >> burst = 4 for controllers that do support it, or burst = 1 for those
> >> that don't, and do more bursts.
> >
> > Nope, the client configures these parameters and dmaengine driver
> > validates and programs
> 
> Shouldn't we just name it "burst_size" then if it's meant to be what
> the client specifically asks for?

Well if for some reason we program lesser than than max it would work
technically. But a larger burst wont work at all, so thats why maxburst is
significant.

> My understanding is that the client configures its own parameters,
> such as the trigger level for the DRQ, like raise DRQ when level < 1/4
> FIFO depth, request maxburst = 1/4 or 1/2 FIFO depth, so as not to
> overrun the FIFO. When the DRQ is raised, the DMA engine will do a
> burst, and after the burst the DRQ would be low again, so the DMA
> engine will wait. So the DMA engine driver should be free to
> program the actual burst size to something less than maxburst, shouldn't
> it?

Yup but not more that max..

> >> This also means we can increase max_burst for the audio codec, as
> >> the FIFO is 64 samples deep for stereo, or 128 samples for mono.
> >
> > Beware that higher bursts means chance of underrun of FIFO. This value
> > is selected with consideration of power and performance required. Lazy
> > allocation would be half of FIFO size..
> 
> You mean underrun if its the source right? So the client setting maxburst
> should take the DRQ trigger level into account for this.

Yes

-- 
~Vinod

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Vinod Koul]

On Tue, Nov 01, 2016 at 10:55:13PM +0800, Chen-Yu Tsai wrote:

> >>  * @src_maxburst: the maximum number of words (note: words, as in
> >>  * units of the src_addr_width member, not bytes) that can be sent
> >>  * in one burst to the device. Typically something like half the
> >>  * FIFO depth on I/O peripherals so you don't overflow it. This
> >>  * may or may not be applicable on memory sources.
> >>  * @dst_maxburst: same as src_maxburst but for destination target
> >>  * mutatis mutandis.
> >>
> >> The DMA engine driver should be free to select whatever burst size
> >> that doesn't exceed this. So for max_burst = 4, the driver can select
> >> burst = 4 for controllers that do support it, or burst = 1 for those
> >> that don't, and do more bursts.
> >
> > Nope, the client configures these parameters and dmaengine driver
> > validates and programs
> 
> Shouldn't we just name it "burst_size" then if it's meant to be what
> the client specifically asks for?

Well if for some reason we program lesser than than max it would work
technically. But a larger burst wont work at all, so thats why maxburst is
significant.

> My understanding is that the client configures its own parameters,
> such as the trigger level for the DRQ, like raise DRQ when level < 1/4
> FIFO depth, request maxburst = 1/4 or 1/2 FIFO depth, so as not to
> overrun the FIFO. When the DRQ is raised, the DMA engine will do a
> burst, and after the burst the DRQ would be low again, so the DMA
> engine will wait. So the DMA engine driver should be free to
> program the actual burst size to something less than maxburst, shouldn't
> it?

Yup but not more that max..

> >> This also means we can increase max_burst for the audio codec, as
> >> the FIFO is 64 samples deep for stereo, or 128 samples for mono.
> >
> > Beware that higher bursts means chance of underrun of FIFO. This value
> > is selected with consideration of power and performance required. Lazy
> > allocation would be half of FIFO size..
> 
> You mean underrun if its the source right? So the client setting maxburst
> should take the DRQ trigger level into account for this.

Yes

-- 
~Vinod

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Chen-Yu Tsai]

On Tue, Nov 1, 2016 at 9:46 PM, Koul, Vinod  wrote:
> On Sun, 2016-10-30 at 10:06 +0800, Chen-Yu Tsai wrote:
>> Looking at the dmaengine API, I believe we got it wrong.
>>
>> max_burst in dma_slave_config denotes the largest amount of data
>> a single transfer should be, as described in dmaengine.h:
>
> Not a single transfer but smallest transaction within a transfer of a
> block. So dmaengines transfer data in bursts from source to destination,
> this parameter decides the size of that bursts

Right.

>
>>
>>  * @src_maxburst: the maximum number of words (note: words, as in
>>  * units of the src_addr_width member, not bytes) that can be sent
>>  * in one burst to the device. Typically something like half the
>>  * FIFO depth on I/O peripherals so you don't overflow it. This
>>  * may or may not be applicable on memory sources.
>>  * @dst_maxburst: same as src_maxburst but for destination target
>>  * mutatis mutandis.
>>
>> The DMA engine driver should be free to select whatever burst size
>> that doesn't exceed this. So for max_burst = 4, the driver can select
>> burst = 4 for controllers that do support it, or burst = 1 for those
>> that don't, and do more bursts.
>
> Nope, the client configures these parameters and dmaengine driver
> validates and programs

Shouldn't we just name it "burst_size" then if it's meant to be what
the client specifically asks for?

My understanding is that the client configures its own parameters,
such as the trigger level for the DRQ, like raise DRQ when level < 1/4
FIFO depth, request maxburst = 1/4 or 1/2 FIFO depth, so as not to
overrun the FIFO. When the DRQ is raised, the DMA engine will do a
burst, and after the burst the DRQ would be low again, so the DMA
engine will wait. So the DMA engine driver should be free to
program the actual burst size to something less than maxburst, shouldn't
it?

>>
>> This also means we can increase max_burst for the audio codec, as
>> the FIFO is 64 samples deep for stereo, or 128 samples for mono.
>
> Beware that higher bursts means chance of underrun of FIFO. This value
> is selected with consideration of power and performance required. Lazy
> allocation would be half of FIFO size..

You mean underrun if its the source right? So the client setting maxburst
should take the DRQ trigger level into account for this.


Regards
ChenYu

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Chen-Yu Tsai]

On Tue, Nov 1, 2016 at 9:46 PM, Koul, Vinod  wrote:
> On Sun, 2016-10-30 at 10:06 +0800, Chen-Yu Tsai wrote:
>> Looking at the dmaengine API, I believe we got it wrong.
>>
>> max_burst in dma_slave_config denotes the largest amount of data
>> a single transfer should be, as described in dmaengine.h:
>
> Not a single transfer but smallest transaction within a transfer of a
> block. So dmaengines transfer data in bursts from source to destination,
> this parameter decides the size of that bursts

Right.

>
>>
>>  * @src_maxburst: the maximum number of words (note: words, as in
>>  * units of the src_addr_width member, not bytes) that can be sent
>>  * in one burst to the device. Typically something like half the
>>  * FIFO depth on I/O peripherals so you don't overflow it. This
>>  * may or may not be applicable on memory sources.
>>  * @dst_maxburst: same as src_maxburst but for destination target
>>  * mutatis mutandis.
>>
>> The DMA engine driver should be free to select whatever burst size
>> that doesn't exceed this. So for max_burst = 4, the driver can select
>> burst = 4 for controllers that do support it, or burst = 1 for those
>> that don't, and do more bursts.
>
> Nope, the client configures these parameters and dmaengine driver
> validates and programs

Shouldn't we just name it "burst_size" then if it's meant to be what
the client specifically asks for?

My understanding is that the client configures its own parameters,
such as the trigger level for the DRQ, like raise DRQ when level < 1/4
FIFO depth, request maxburst = 1/4 or 1/2 FIFO depth, so as not to
overrun the FIFO. When the DRQ is raised, the DMA engine will do a
burst, and after the burst the DRQ would be low again, so the DMA
engine will wait. So the DMA engine driver should be free to
program the actual burst size to something less than maxburst, shouldn't
it?

>>
>> This also means we can increase max_burst for the audio codec, as
>> the FIFO is 64 samples deep for stereo, or 128 samples for mono.
>
> Beware that higher bursts means chance of underrun of FIFO. This value
> is selected with consideration of power and performance required. Lazy
> allocation would be half of FIFO size..

You mean underrun if its the source right? So the client setting maxburst
should take the DRQ trigger level into account for this.


Regards
ChenYu

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Koul, Vinod]

On Sun, 2016-10-30 at 10:06 +0800, Chen-Yu Tsai wrote:
> Looking at the dmaengine API, I believe we got it wrong.
> 
> max_burst in dma_slave_config denotes the largest amount of data
> a single transfer should be, as described in dmaengine.h:

Not a single transfer but smallest transaction within a transfer of a
block. So dmaengines transfer data in bursts from source to destination,
this parameter decides the size of that bursts

> 
>  * @src_maxburst: the maximum number of words (note: words, as in
>  * units of the src_addr_width member, not bytes) that can be sent
>  * in one burst to the device. Typically something like half the
>  * FIFO depth on I/O peripherals so you don't overflow it. This
>  * may or may not be applicable on memory sources.
>  * @dst_maxburst: same as src_maxburst but for destination target
>  * mutatis mutandis.
> 
> The DMA engine driver should be free to select whatever burst size
> that doesn't exceed this. So for max_burst = 4, the driver can select
> burst = 4 for controllers that do support it, or burst = 1 for those
> that don't, and do more bursts.

Nope, the client configures these parameters and dmaengine driver
validates and programs

> 
> This also means we can increase max_burst for the audio codec, as
> the FIFO is 64 samples deep for stereo, or 128 samples for mono.

Beware that higher bursts means chance of underrun of FIFO. This value
is selected with consideration of power and performance required. Lazy
allocation would be half of FIFO size..

-- 
~Vinod

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Koul, Vinod]

On Sun, 2016-10-30 at 10:06 +0800, Chen-Yu Tsai wrote:
> Looking at the dmaengine API, I believe we got it wrong.
> 
> max_burst in dma_slave_config denotes the largest amount of data
> a single transfer should be, as described in dmaengine.h:

Not a single transfer but smallest transaction within a transfer of a
block. So dmaengines transfer data in bursts from source to destination,
this parameter decides the size of that bursts

> 
>  * @src_maxburst: the maximum number of words (note: words, as in
>  * units of the src_addr_width member, not bytes) that can be sent
>  * in one burst to the device. Typically something like half the
>  * FIFO depth on I/O peripherals so you don't overflow it. This
>  * may or may not be applicable on memory sources.
>  * @dst_maxburst: same as src_maxburst but for destination target
>  * mutatis mutandis.
> 
> The DMA engine driver should be free to select whatever burst size
> that doesn't exceed this. So for max_burst = 4, the driver can select
> burst = 4 for controllers that do support it, or burst = 1 for those
> that don't, and do more bursts.

Nope, the client configures these parameters and dmaengine driver
validates and programs

> 
> This also means we can increase max_burst for the audio codec, as
> the FIFO is 64 samples deep for stereo, or 128 samples for mono.

Beware that higher bursts means chance of underrun of FIFO. This value
is selected with consideration of power and performance required. Lazy
allocation would be half of FIFO size..

-- 
~Vinod

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Jean-Francois Moine]

On Sun, 30 Oct 2016 10:06:20 +0800
Chen-Yu Tsai  wrote:

> >> Yes, I know that the burst size is always a power of 2.
> >> The best way to check it would be to change the {src,dts}_maxburst to a
> >> bitmap of the possible bursts as 0x0d for 1,4 and 8 bytes. But this
> >> asks for a lot of changes...
> >
> > To be honest, I'm not really a big fan of those tree wide changes
> > without any way to maintain compatibility. It ends up in a single,
> > huge patch if we want to maintain bisectability that just isn't
> > reviewable.
> 
> Looking at the dmaengine API, I believe we got it wrong.
> 
> max_burst in dma_slave_config denotes the largest amount of data
> a single transfer should be, as described in dmaengine.h:
> 
>  * @src_maxburst: the maximum number of words (note: words, as in
>  * units of the src_addr_width member, not bytes) that can be sent
>  * in one burst to the device. Typically something like half the
>  * FIFO depth on I/O peripherals so you don't overflow it. This
>  * may or may not be applicable on memory sources.
>  * @dst_maxburst: same as src_maxburst but for destination target
>  * mutatis mutandis.
> 
> The DMA engine driver should be free to select whatever burst size
> that doesn't exceed this. So for max_burst = 4, the driver can select
> burst = 4 for controllers that do support it, or burst = 1 for those
> that don't, and do more bursts.
> 
> This also means we can increase max_burst for the audio codec, as
> the FIFO is 64 samples deep for stereo, or 128 samples for mono.
> 
> 
> If we agree on the above I can send some patches for this.

That's fine to me, but this does not solve the main problem which is
how/where are defined the possible bursts of a SoC.

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Jean-Francois Moine]

On Sun, 30 Oct 2016 10:06:20 +0800
Chen-Yu Tsai  wrote:

> >> Yes, I know that the burst size is always a power of 2.
> >> The best way to check it would be to change the {src,dts}_maxburst to a
> >> bitmap of the possible bursts as 0x0d for 1,4 and 8 bytes. But this
> >> asks for a lot of changes...
> >
> > To be honest, I'm not really a big fan of those tree wide changes
> > without any way to maintain compatibility. It ends up in a single,
> > huge patch if we want to maintain bisectability that just isn't
> > reviewable.
> 
> Looking at the dmaengine API, I believe we got it wrong.
> 
> max_burst in dma_slave_config denotes the largest amount of data
> a single transfer should be, as described in dmaengine.h:
> 
>  * @src_maxburst: the maximum number of words (note: words, as in
>  * units of the src_addr_width member, not bytes) that can be sent
>  * in one burst to the device. Typically something like half the
>  * FIFO depth on I/O peripherals so you don't overflow it. This
>  * may or may not be applicable on memory sources.
>  * @dst_maxburst: same as src_maxburst but for destination target
>  * mutatis mutandis.
> 
> The DMA engine driver should be free to select whatever burst size
> that doesn't exceed this. So for max_burst = 4, the driver can select
> burst = 4 for controllers that do support it, or burst = 1 for those
> that don't, and do more bursts.
> 
> This also means we can increase max_burst for the audio codec, as
> the FIFO is 64 samples deep for stereo, or 128 samples for mono.
> 
> 
> If we agree on the above I can send some patches for this.

That's fine to me, but this does not solve the main problem which is
how/where are defined the possible bursts of a SoC.

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Chen-Yu Tsai]

Hi,

On Fri, Oct 28, 2016 at 1:51 AM, Maxime Ripard
 wrote:
> On Sun, Oct 23, 2016 at 06:31:07PM +0200, Jean-Francois Moine wrote:
>> On Tue, 4 Oct 2016 18:55:54 +0200
>> Maxime Ripard  wrote:
>>
>> > On Tue, Oct 04, 2016 at 12:40:11PM +0200, Jean-Francois Moine wrote:
>> > > On Tue,  4 Oct 2016 11:46:14 +0200
>> > > Mylène Josserand  wrote:
>> > >
>> > > > Add the case of a burst of 4 which is handled by the SoC.
>> > > >
>> > > > Signed-off-by: Mylène Josserand 
>> > > > ---
>> > > >  drivers/dma/sun6i-dma.c | 2 ++
>> > > >  1 file changed, 2 insertions(+)
>> > > >
>> > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
>> > > > index 8346199..0485204 100644
>> > > > --- a/drivers/dma/sun6i-dma.c
>> > > > +++ b/drivers/dma/sun6i-dma.c
>> > > > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
>> > > > switch (maxburst) {
>> > > > case 1:
>> > > > return 0;
>> > > > +   case 4:
>> > > > +   return 1;
>> > > > case 8:
>> > > > return 2;
>> > > > default:
>> > > > --
>> > > > 2.9.3
>> > >
>> > > This patch has already been rejected by Maxime in the threads
>> > >   http://www.spinics.net/lists/dmaengine/msg08610.html
>> > > and
>> > >   http://www.spinics.net/lists/dmaengine/msg08719.html
>> > >
>> > > I hope you will find the way he wants for this maxburst to be added.
>> >
>> > I was talking about something along these lines (not tested):
>>
>> I wonder why you don't submit this yourself.
>
> I thought you were the one who cared. You asked for what I had in
> mind, here it is.
>
>> > ---8<-
>> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
>> > index 83461994e418..573ac4608293 100644
>> > --- a/drivers/dma/sun6i-dma.c
>> > +++ b/drivers/dma/sun6i-dma.c
>> > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
>> > switch (maxburst) {
>> > case 1:
>> > return 0;
>> > +   case 4:
>> > +   return 1;
>> > case 8:
>> > return 2;
>> > default:
>> > @@ -1110,11 +1112,19 @@ static int sun6i_dma_probe(struct platform_device 
>> > *pdev)
>> > sdc->slave.dst_addr_widths  = 
>> > BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>> >   
>> > BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>> >   
>> > BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
>> > +   sdc->slave.dst_bursts   = BIT(1) | BIT(8);
>> > +   sdc->slave.src_bursts   = BIT(1) | BIT(8);
>> > sdc->slave.directions   = BIT(DMA_DEV_TO_MEM) |
>> >   BIT(DMA_MEM_TO_DEV);
>> > sdc->slave.residue_granularity  = 
>> > DMA_RESIDUE_GRANULARITY_BURST;
>> > sdc->slave.dev = >dev;
>> >
>> > +   if (of_device_is_compatible(pdev->dev.of_node,
>> > +   "allwinner,sun8i-h3-dma")) {
>> > +   sdc->slave.dst_bursts |= BIT(4);
>> > +   sdc->slave.src_bursts |= BIT(4);
>> > +   }
>> > +
>> > sdc->pchans = devm_kcalloc(>dev, sdc->cfg->nr_max_channels,
>> >sizeof(struct sun6i_pchan), GFP_KERNEL);
>> > if (!sdc->pchans)
>> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> > index cc535a478bae..f7bbec24bb58 100644
>> > --- a/include/linux/dmaengine.h
>> > +++ b/include/linux/dmaengine.h
>> > @@ -673,6 +673,8 @@ struct dma_filter {
>> >   * each type of direction, the dma controller should fill (1 <<
>> >   * ) and same should be checked by controller as well
>> >   * @max_burst: max burst capability per-transfer
>> > + * @dst_bursts: bitfield of the available burst sizes for the destination
>> > + * @src_bursts: bitfield of the available burst sizes for the source
>>
>> You did not define dst_bursts nor src_bursts.
>>
>> >   * @residue_granularity: granularity of the transfer residue reported
>> >   * by tx_status
>> >   * @device_alloc_chan_resources: allocate resources and return the
>> > @@ -800,6 +802,14 @@ struct dma_device {
>> >  static inline int dmaengine_slave_config(struct dma_chan *chan,
>> >   struct dma_slave_config *config)
>> >  {
>> > +   if (config->src_maxburst && config->device->src_bursts &&
>> > +   !(BIT(config->src_maxburst) & config->device->src_bursts))
>>
>> The maxburst may be as big as 4Kibytes, then, I am not sure that this
>> code will work!
>>
>> > +   return -EINVAL;
>> > +
>> > +   if (config->dst_maxburst && config->device->dst_bursts &&
>> > +   !(BIT(config->dst_maxburst) & config->device->dst_bursts))
>> > +   return -EINVAL;
>> > +
>> > if (chan->device->device_config)
>> > return chan->device->device_config(chan, config);
>> > 

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Chen-Yu Tsai]

Hi,

On Fri, Oct 28, 2016 at 1:51 AM, Maxime Ripard
 wrote:
> On Sun, Oct 23, 2016 at 06:31:07PM +0200, Jean-Francois Moine wrote:
>> On Tue, 4 Oct 2016 18:55:54 +0200
>> Maxime Ripard  wrote:
>>
>> > On Tue, Oct 04, 2016 at 12:40:11PM +0200, Jean-Francois Moine wrote:
>> > > On Tue,  4 Oct 2016 11:46:14 +0200
>> > > Mylène Josserand  wrote:
>> > >
>> > > > Add the case of a burst of 4 which is handled by the SoC.
>> > > >
>> > > > Signed-off-by: Mylène Josserand 
>> > > > ---
>> > > >  drivers/dma/sun6i-dma.c | 2 ++
>> > > >  1 file changed, 2 insertions(+)
>> > > >
>> > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
>> > > > index 8346199..0485204 100644
>> > > > --- a/drivers/dma/sun6i-dma.c
>> > > > +++ b/drivers/dma/sun6i-dma.c
>> > > > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
>> > > > switch (maxburst) {
>> > > > case 1:
>> > > > return 0;
>> > > > +   case 4:
>> > > > +   return 1;
>> > > > case 8:
>> > > > return 2;
>> > > > default:
>> > > > --
>> > > > 2.9.3
>> > >
>> > > This patch has already been rejected by Maxime in the threads
>> > >   http://www.spinics.net/lists/dmaengine/msg08610.html
>> > > and
>> > >   http://www.spinics.net/lists/dmaengine/msg08719.html
>> > >
>> > > I hope you will find the way he wants for this maxburst to be added.
>> >
>> > I was talking about something along these lines (not tested):
>>
>> I wonder why you don't submit this yourself.
>
> I thought you were the one who cared. You asked for what I had in
> mind, here it is.
>
>> > ---8<-
>> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
>> > index 83461994e418..573ac4608293 100644
>> > --- a/drivers/dma/sun6i-dma.c
>> > +++ b/drivers/dma/sun6i-dma.c
>> > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
>> > switch (maxburst) {
>> > case 1:
>> > return 0;
>> > +   case 4:
>> > +   return 1;
>> > case 8:
>> > return 2;
>> > default:
>> > @@ -1110,11 +1112,19 @@ static int sun6i_dma_probe(struct platform_device 
>> > *pdev)
>> > sdc->slave.dst_addr_widths  = 
>> > BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>> >   
>> > BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>> >   
>> > BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
>> > +   sdc->slave.dst_bursts   = BIT(1) | BIT(8);
>> > +   sdc->slave.src_bursts   = BIT(1) | BIT(8);
>> > sdc->slave.directions   = BIT(DMA_DEV_TO_MEM) |
>> >   BIT(DMA_MEM_TO_DEV);
>> > sdc->slave.residue_granularity  = 
>> > DMA_RESIDUE_GRANULARITY_BURST;
>> > sdc->slave.dev = >dev;
>> >
>> > +   if (of_device_is_compatible(pdev->dev.of_node,
>> > +   "allwinner,sun8i-h3-dma")) {
>> > +   sdc->slave.dst_bursts |= BIT(4);
>> > +   sdc->slave.src_bursts |= BIT(4);
>> > +   }
>> > +
>> > sdc->pchans = devm_kcalloc(>dev, sdc->cfg->nr_max_channels,
>> >sizeof(struct sun6i_pchan), GFP_KERNEL);
>> > if (!sdc->pchans)
>> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> > index cc535a478bae..f7bbec24bb58 100644
>> > --- a/include/linux/dmaengine.h
>> > +++ b/include/linux/dmaengine.h
>> > @@ -673,6 +673,8 @@ struct dma_filter {
>> >   * each type of direction, the dma controller should fill (1 <<
>> >   * ) and same should be checked by controller as well
>> >   * @max_burst: max burst capability per-transfer
>> > + * @dst_bursts: bitfield of the available burst sizes for the destination
>> > + * @src_bursts: bitfield of the available burst sizes for the source
>>
>> You did not define dst_bursts nor src_bursts.
>>
>> >   * @residue_granularity: granularity of the transfer residue reported
>> >   * by tx_status
>> >   * @device_alloc_chan_resources: allocate resources and return the
>> > @@ -800,6 +802,14 @@ struct dma_device {
>> >  static inline int dmaengine_slave_config(struct dma_chan *chan,
>> >   struct dma_slave_config *config)
>> >  {
>> > +   if (config->src_maxburst && config->device->src_bursts &&
>> > +   !(BIT(config->src_maxburst) & config->device->src_bursts))
>>
>> The maxburst may be as big as 4Kibytes, then, I am not sure that this
>> code will work!
>>
>> > +   return -EINVAL;
>> > +
>> > +   if (config->dst_maxburst && config->device->dst_bursts &&
>> > +   !(BIT(config->dst_maxburst) & config->device->dst_bursts))
>> > +   return -EINVAL;
>> > +
>> > if (chan->device->device_config)
>> > return chan->device->device_config(chan, config);
>> > ---8<
>>
>> Yes, I know that the burst size is always a power of 2.
>> The best way to check it would be to change the {src,dts}_maxburst to 

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Maxime Ripard]

On Sun, Oct 23, 2016 at 06:31:07PM +0200, Jean-Francois Moine wrote:
> On Tue, 4 Oct 2016 18:55:54 +0200
> Maxime Ripard  wrote:
> 
> > On Tue, Oct 04, 2016 at 12:40:11PM +0200, Jean-Francois Moine wrote:
> > > On Tue,  4 Oct 2016 11:46:14 +0200
> > > Mylène Josserand  wrote:
> > > 
> > > > Add the case of a burst of 4 which is handled by the SoC.
> > > > 
> > > > Signed-off-by: Mylène Josserand 
> > > > ---
> > > >  drivers/dma/sun6i-dma.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > > > index 8346199..0485204 100644
> > > > --- a/drivers/dma/sun6i-dma.c
> > > > +++ b/drivers/dma/sun6i-dma.c
> > > > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > > > switch (maxburst) {
> > > > case 1:
> > > > return 0;
> > > > +   case 4:
> > > > +   return 1;
> > > > case 8:
> > > > return 2;
> > > > default:
> > > > -- 
> > > > 2.9.3
> > > 
> > > This patch has already been rejected by Maxime in the threads
> > >   http://www.spinics.net/lists/dmaengine/msg08610.html
> > > and
> > >   http://www.spinics.net/lists/dmaengine/msg08719.html
> > > 
> > > I hope you will find the way he wants for this maxburst to be added.
> > 
> > I was talking about something along these lines (not tested):
> 
> I wonder why you don't submit this yourself.

I thought you were the one who cared. You asked for what I had in
mind, here it is.

> > ---8<-
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index 83461994e418..573ac4608293 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > switch (maxburst) {
> > case 1:
> > return 0;
> > +   case 4:
> > +   return 1;
> > case 8:
> > return 2;
> > default:
> > @@ -1110,11 +1112,19 @@ static int sun6i_dma_probe(struct platform_device 
> > *pdev)
> > sdc->slave.dst_addr_widths  = 
> > BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> >   
> > BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> >   
> > BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> > +   sdc->slave.dst_bursts   = BIT(1) | BIT(8);
> > +   sdc->slave.src_bursts   = BIT(1) | BIT(8);
> > sdc->slave.directions   = BIT(DMA_DEV_TO_MEM) |
> >   BIT(DMA_MEM_TO_DEV);
> > sdc->slave.residue_granularity  = DMA_RESIDUE_GRANULARITY_BURST;
> > sdc->slave.dev = >dev;
> >  
> > +   if (of_device_is_compatible(pdev->dev.of_node,
> > +   "allwinner,sun8i-h3-dma")) {
> > +   sdc->slave.dst_bursts |= BIT(4);
> > +   sdc->slave.src_bursts |= BIT(4);
> > +   }
> > +
> > sdc->pchans = devm_kcalloc(>dev, sdc->cfg->nr_max_channels,
> >sizeof(struct sun6i_pchan), GFP_KERNEL);
> > if (!sdc->pchans)
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index cc535a478bae..f7bbec24bb58 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -673,6 +673,8 @@ struct dma_filter {
> >   * each type of direction, the dma controller should fill (1 <<
> >   * ) and same should be checked by controller as well
> >   * @max_burst: max burst capability per-transfer
> > + * @dst_bursts: bitfield of the available burst sizes for the destination
> > + * @src_bursts: bitfield of the available burst sizes for the source
> 
> You did not define dst_bursts nor src_bursts.
> 
> >   * @residue_granularity: granularity of the transfer residue reported
> >   * by tx_status
> >   * @device_alloc_chan_resources: allocate resources and return the
> > @@ -800,6 +802,14 @@ struct dma_device {
> >  static inline int dmaengine_slave_config(struct dma_chan *chan,
> >   struct dma_slave_config *config)
> >  {
> > +   if (config->src_maxburst && config->device->src_bursts &&
> > +   !(BIT(config->src_maxburst) & config->device->src_bursts))
> 
> The maxburst may be as big as 4Kibytes, then, I am not sure that this
> code will work!
> 
> > +   return -EINVAL;
> > +
> > +   if (config->dst_maxburst && config->device->dst_bursts &&
> > +   !(BIT(config->dst_maxburst) & config->device->dst_bursts))
> > +   return -EINVAL;
> > +
> > if (chan->device->device_config)
> > return chan->device->device_config(chan, config);
> > ---8< 
> 
> Yes, I know that the burst size is always a power of 2.
> The best way to check it would be to change the {src,dts}_maxburst to a
> bitmap of the possible bursts as 0x0d for 1,4 and 8 bytes. 

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Maxime Ripard]

On Sun, Oct 23, 2016 at 06:31:07PM +0200, Jean-Francois Moine wrote:
> On Tue, 4 Oct 2016 18:55:54 +0200
> Maxime Ripard  wrote:
> 
> > On Tue, Oct 04, 2016 at 12:40:11PM +0200, Jean-Francois Moine wrote:
> > > On Tue,  4 Oct 2016 11:46:14 +0200
> > > Mylène Josserand  wrote:
> > > 
> > > > Add the case of a burst of 4 which is handled by the SoC.
> > > > 
> > > > Signed-off-by: Mylène Josserand 
> > > > ---
> > > >  drivers/dma/sun6i-dma.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > > > index 8346199..0485204 100644
> > > > --- a/drivers/dma/sun6i-dma.c
> > > > +++ b/drivers/dma/sun6i-dma.c
> > > > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > > > switch (maxburst) {
> > > > case 1:
> > > > return 0;
> > > > +   case 4:
> > > > +   return 1;
> > > > case 8:
> > > > return 2;
> > > > default:
> > > > -- 
> > > > 2.9.3
> > > 
> > > This patch has already been rejected by Maxime in the threads
> > >   http://www.spinics.net/lists/dmaengine/msg08610.html
> > > and
> > >   http://www.spinics.net/lists/dmaengine/msg08719.html
> > > 
> > > I hope you will find the way he wants for this maxburst to be added.
> > 
> > I was talking about something along these lines (not tested):
> 
> I wonder why you don't submit this yourself.

I thought you were the one who cared. You asked for what I had in
mind, here it is.

> > ---8<-
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index 83461994e418..573ac4608293 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > switch (maxburst) {
> > case 1:
> > return 0;
> > +   case 4:
> > +   return 1;
> > case 8:
> > return 2;
> > default:
> > @@ -1110,11 +1112,19 @@ static int sun6i_dma_probe(struct platform_device 
> > *pdev)
> > sdc->slave.dst_addr_widths  = 
> > BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> >   
> > BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> >   
> > BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> > +   sdc->slave.dst_bursts   = BIT(1) | BIT(8);
> > +   sdc->slave.src_bursts   = BIT(1) | BIT(8);
> > sdc->slave.directions   = BIT(DMA_DEV_TO_MEM) |
> >   BIT(DMA_MEM_TO_DEV);
> > sdc->slave.residue_granularity  = DMA_RESIDUE_GRANULARITY_BURST;
> > sdc->slave.dev = >dev;
> >  
> > +   if (of_device_is_compatible(pdev->dev.of_node,
> > +   "allwinner,sun8i-h3-dma")) {
> > +   sdc->slave.dst_bursts |= BIT(4);
> > +   sdc->slave.src_bursts |= BIT(4);
> > +   }
> > +
> > sdc->pchans = devm_kcalloc(>dev, sdc->cfg->nr_max_channels,
> >sizeof(struct sun6i_pchan), GFP_KERNEL);
> > if (!sdc->pchans)
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index cc535a478bae..f7bbec24bb58 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -673,6 +673,8 @@ struct dma_filter {
> >   * each type of direction, the dma controller should fill (1 <<
> >   * ) and same should be checked by controller as well
> >   * @max_burst: max burst capability per-transfer
> > + * @dst_bursts: bitfield of the available burst sizes for the destination
> > + * @src_bursts: bitfield of the available burst sizes for the source
> 
> You did not define dst_bursts nor src_bursts.
> 
> >   * @residue_granularity: granularity of the transfer residue reported
> >   * by tx_status
> >   * @device_alloc_chan_resources: allocate resources and return the
> > @@ -800,6 +802,14 @@ struct dma_device {
> >  static inline int dmaengine_slave_config(struct dma_chan *chan,
> >   struct dma_slave_config *config)
> >  {
> > +   if (config->src_maxburst && config->device->src_bursts &&
> > +   !(BIT(config->src_maxburst) & config->device->src_bursts))
> 
> The maxburst may be as big as 4Kibytes, then, I am not sure that this
> code will work!
> 
> > +   return -EINVAL;
> > +
> > +   if (config->dst_maxburst && config->device->dst_bursts &&
> > +   !(BIT(config->dst_maxburst) & config->device->dst_bursts))
> > +   return -EINVAL;
> > +
> > if (chan->device->device_config)
> > return chan->device->device_config(chan, config);
> > ---8< 
> 
> Yes, I know that the burst size is always a power of 2.
> The best way to check it would be to change the {src,dts}_maxburst to a
> bitmap of the possible bursts as 0x0d for 1,4 and 8 bytes. But this
> asks for a lot of changes...

To be honest, I'm not really a big fan of those tree wide changes

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Jean-Francois Moine]

On Tue, 4 Oct 2016 18:55:54 +0200
Maxime Ripard  wrote:

> On Tue, Oct 04, 2016 at 12:40:11PM +0200, Jean-Francois Moine wrote:
> > On Tue,  4 Oct 2016 11:46:14 +0200
> > Mylène Josserand  wrote:
> > 
> > > Add the case of a burst of 4 which is handled by the SoC.
> > > 
> > > Signed-off-by: Mylène Josserand 
> > > ---
> > >  drivers/dma/sun6i-dma.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > > index 8346199..0485204 100644
> > > --- a/drivers/dma/sun6i-dma.c
> > > +++ b/drivers/dma/sun6i-dma.c
> > > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > >   switch (maxburst) {
> > >   case 1:
> > >   return 0;
> > > + case 4:
> > > + return 1;
> > >   case 8:
> > >   return 2;
> > >   default:
> > > -- 
> > > 2.9.3
> > 
> > This patch has already been rejected by Maxime in the threads
> > http://www.spinics.net/lists/dmaengine/msg08610.html
> > and
> > http://www.spinics.net/lists/dmaengine/msg08719.html
> > 
> > I hope you will find the way he wants for this maxburst to be added.
> 
> I was talking about something along these lines (not tested):

I wonder why you don't submit this yourself.

> ---8<-
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 83461994e418..573ac4608293 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
>   switch (maxburst) {
>   case 1:
>   return 0;
> + case 4:
> + return 1;
>   case 8:
>   return 2;
>   default:
> @@ -1110,11 +1112,19 @@ static int sun6i_dma_probe(struct platform_device 
> *pdev)
>   sdc->slave.dst_addr_widths  = 
> BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> 
> BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> 
> BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> + sdc->slave.dst_bursts   = BIT(1) | BIT(8);
> + sdc->slave.src_bursts   = BIT(1) | BIT(8);
>   sdc->slave.directions   = BIT(DMA_DEV_TO_MEM) |
> BIT(DMA_MEM_TO_DEV);
>   sdc->slave.residue_granularity  = DMA_RESIDUE_GRANULARITY_BURST;
>   sdc->slave.dev = >dev;
>  
> + if (of_device_is_compatible(pdev->dev.of_node,
> + "allwinner,sun8i-h3-dma")) {
> + sdc->slave.dst_bursts |= BIT(4);
> + sdc->slave.src_bursts |= BIT(4);
> + }
> +
>   sdc->pchans = devm_kcalloc(>dev, sdc->cfg->nr_max_channels,
>  sizeof(struct sun6i_pchan), GFP_KERNEL);
>   if (!sdc->pchans)
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index cc535a478bae..f7bbec24bb58 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -673,6 +673,8 @@ struct dma_filter {
>   *   each type of direction, the dma controller should fill (1 <<
>   *   ) and same should be checked by controller as well
>   * @max_burst: max burst capability per-transfer
> + * @dst_bursts: bitfield of the available burst sizes for the destination
> + * @src_bursts: bitfield of the available burst sizes for the source

You did not define dst_bursts nor src_bursts.

>   * @residue_granularity: granularity of the transfer residue reported
>   *   by tx_status
>   * @device_alloc_chan_resources: allocate resources and return the
> @@ -800,6 +802,14 @@ struct dma_device {
>  static inline int dmaengine_slave_config(struct dma_chan *chan,
> struct dma_slave_config *config)
>  {
> + if (config->src_maxburst && config->device->src_bursts &&
> + !(BIT(config->src_maxburst) & config->device->src_bursts))

The maxburst may be as big as 4Kibytes, then, I am not sure that this
code will work!

> + return -EINVAL;
> +
> + if (config->dst_maxburst && config->device->dst_bursts &&
> + !(BIT(config->dst_maxburst) & config->device->dst_bursts))
> + return -EINVAL;
> +
>   if (chan->device->device_config)
>   return chan->device->device_config(chan, config);
> ---8< 

Yes, I know that the burst size is always a power of 2.
The best way to check it would be to change the {src,dts}_maxburst to a
bitmap of the possible bursts as 0x0d for 1,4 and 8 bytes. But this
asks for a lot of changes...

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Jean-Francois Moine]

On Tue, 4 Oct 2016 18:55:54 +0200
Maxime Ripard  wrote:

> On Tue, Oct 04, 2016 at 12:40:11PM +0200, Jean-Francois Moine wrote:
> > On Tue,  4 Oct 2016 11:46:14 +0200
> > Mylène Josserand  wrote:
> > 
> > > Add the case of a burst of 4 which is handled by the SoC.
> > > 
> > > Signed-off-by: Mylène Josserand 
> > > ---
> > >  drivers/dma/sun6i-dma.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > > index 8346199..0485204 100644
> > > --- a/drivers/dma/sun6i-dma.c
> > > +++ b/drivers/dma/sun6i-dma.c
> > > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > >   switch (maxburst) {
> > >   case 1:
> > >   return 0;
> > > + case 4:
> > > + return 1;
> > >   case 8:
> > >   return 2;
> > >   default:
> > > -- 
> > > 2.9.3
> > 
> > This patch has already been rejected by Maxime in the threads
> > http://www.spinics.net/lists/dmaengine/msg08610.html
> > and
> > http://www.spinics.net/lists/dmaengine/msg08719.html
> > 
> > I hope you will find the way he wants for this maxburst to be added.
> 
> I was talking about something along these lines (not tested):

I wonder why you don't submit this yourself.

> ---8<-
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 83461994e418..573ac4608293 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
>   switch (maxburst) {
>   case 1:
>   return 0;
> + case 4:
> + return 1;
>   case 8:
>   return 2;
>   default:
> @@ -1110,11 +1112,19 @@ static int sun6i_dma_probe(struct platform_device 
> *pdev)
>   sdc->slave.dst_addr_widths  = 
> BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> 
> BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> 
> BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> + sdc->slave.dst_bursts   = BIT(1) | BIT(8);
> + sdc->slave.src_bursts   = BIT(1) | BIT(8);
>   sdc->slave.directions   = BIT(DMA_DEV_TO_MEM) |
> BIT(DMA_MEM_TO_DEV);
>   sdc->slave.residue_granularity  = DMA_RESIDUE_GRANULARITY_BURST;
>   sdc->slave.dev = >dev;
>  
> + if (of_device_is_compatible(pdev->dev.of_node,
> + "allwinner,sun8i-h3-dma")) {
> + sdc->slave.dst_bursts |= BIT(4);
> + sdc->slave.src_bursts |= BIT(4);
> + }
> +
>   sdc->pchans = devm_kcalloc(>dev, sdc->cfg->nr_max_channels,
>  sizeof(struct sun6i_pchan), GFP_KERNEL);
>   if (!sdc->pchans)
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index cc535a478bae..f7bbec24bb58 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -673,6 +673,8 @@ struct dma_filter {
>   *   each type of direction, the dma controller should fill (1 <<
>   *   ) and same should be checked by controller as well
>   * @max_burst: max burst capability per-transfer
> + * @dst_bursts: bitfield of the available burst sizes for the destination
> + * @src_bursts: bitfield of the available burst sizes for the source

You did not define dst_bursts nor src_bursts.

>   * @residue_granularity: granularity of the transfer residue reported
>   *   by tx_status
>   * @device_alloc_chan_resources: allocate resources and return the
> @@ -800,6 +802,14 @@ struct dma_device {
>  static inline int dmaengine_slave_config(struct dma_chan *chan,
> struct dma_slave_config *config)
>  {
> + if (config->src_maxburst && config->device->src_bursts &&
> + !(BIT(config->src_maxburst) & config->device->src_bursts))

The maxburst may be as big as 4Kibytes, then, I am not sure that this
code will work!

> + return -EINVAL;
> +
> + if (config->dst_maxburst && config->device->dst_bursts &&
> + !(BIT(config->dst_maxburst) & config->device->dst_bursts))
> + return -EINVAL;
> +
>   if (chan->device->device_config)
>   return chan->device->device_config(chan, config);
> ---8< 

Yes, I know that the burst size is always a power of 2.
The best way to check it would be to change the {src,dts}_maxburst to a
bitmap of the possible bursts as 0x0d for 1,4 and 8 bytes. But this
asks for a lot of changes...

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Maxime Ripard]

On Tue, Oct 04, 2016 at 12:40:11PM +0200, Jean-Francois Moine wrote:
> On Tue,  4 Oct 2016 11:46:14 +0200
> Mylène Josserand  wrote:
> 
> > Add the case of a burst of 4 which is handled by the SoC.
> > 
> > Signed-off-by: Mylène Josserand 
> > ---
> >  drivers/dma/sun6i-dma.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index 8346199..0485204 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > switch (maxburst) {
> > case 1:
> > return 0;
> > +   case 4:
> > +   return 1;
> > case 8:
> > return 2;
> > default:
> > -- 
> > 2.9.3
> 
> This patch has already been rejected by Maxime in the threads
>   http://www.spinics.net/lists/dmaengine/msg08610.html
> and
>   http://www.spinics.net/lists/dmaengine/msg08719.html
> 
> I hope you will find the way he wants for this maxburst to be added.

I was talking about something along these lines (not tested):

---8<-
diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 83461994e418..573ac4608293 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
switch (maxburst) {
case 1:
return 0;
+   case 4:
+   return 1;
case 8:
return 2;
default:
@@ -1110,11 +1112,19 @@ static int sun6i_dma_probe(struct platform_device *pdev)
sdc->slave.dst_addr_widths  = 
BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
  
BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
  
BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+   sdc->slave.dst_bursts   = BIT(1) | BIT(8);
+   sdc->slave.src_bursts   = BIT(1) | BIT(8);
sdc->slave.directions   = BIT(DMA_DEV_TO_MEM) |
  BIT(DMA_MEM_TO_DEV);
sdc->slave.residue_granularity  = DMA_RESIDUE_GRANULARITY_BURST;
sdc->slave.dev = >dev;
 
+   if (of_device_is_compatible(pdev->dev.of_node,
+   "allwinner,sun8i-h3-dma")) {
+   sdc->slave.dst_bursts |= BIT(4);
+   sdc->slave.src_bursts |= BIT(4);
+   }
+
sdc->pchans = devm_kcalloc(>dev, sdc->cfg->nr_max_channels,
   sizeof(struct sun6i_pchan), GFP_KERNEL);
if (!sdc->pchans)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index cc535a478bae..f7bbec24bb58 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -673,6 +673,8 @@ struct dma_filter {
  * each type of direction, the dma controller should fill (1 <<
  * ) and same should be checked by controller as well
  * @max_burst: max burst capability per-transfer
+ * @dst_bursts: bitfield of the available burst sizes for the destination
+ * @src_bursts: bitfield of the available burst sizes for the source
  * @residue_granularity: granularity of the transfer residue reported
  * by tx_status
  * @device_alloc_chan_resources: allocate resources and return the
@@ -800,6 +802,14 @@ struct dma_device {
 static inline int dmaengine_slave_config(struct dma_chan *chan,
  struct dma_slave_config *config)
 {
+   if (config->src_maxburst && config->device->src_bursts &&
+   !(BIT(config->src_maxburst) & config->device->src_bursts))
+   return -EINVAL;
+
+   if (config->dst_maxburst && config->device->dst_bursts &&
+   !(BIT(config->dst_maxburst) & config->device->dst_bursts))
+   return -EINVAL;
+
if (chan->device->device_config)
return chan->device->device_config(chan, config);
---8< 

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Maxime Ripard]

On Tue, Oct 04, 2016 at 12:40:11PM +0200, Jean-Francois Moine wrote:
> On Tue,  4 Oct 2016 11:46:14 +0200
> Mylène Josserand  wrote:
> 
> > Add the case of a burst of 4 which is handled by the SoC.
> > 
> > Signed-off-by: Mylène Josserand 
> > ---
> >  drivers/dma/sun6i-dma.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index 8346199..0485204 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > switch (maxburst) {
> > case 1:
> > return 0;
> > +   case 4:
> > +   return 1;
> > case 8:
> > return 2;
> > default:
> > -- 
> > 2.9.3
> 
> This patch has already been rejected by Maxime in the threads
>   http://www.spinics.net/lists/dmaengine/msg08610.html
> and
>   http://www.spinics.net/lists/dmaengine/msg08719.html
> 
> I hope you will find the way he wants for this maxburst to be added.

I was talking about something along these lines (not tested):

---8<-
diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 83461994e418..573ac4608293 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
switch (maxburst) {
case 1:
return 0;
+   case 4:
+   return 1;
case 8:
return 2;
default:
@@ -1110,11 +1112,19 @@ static int sun6i_dma_probe(struct platform_device *pdev)
sdc->slave.dst_addr_widths  = 
BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
  
BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
  
BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+   sdc->slave.dst_bursts   = BIT(1) | BIT(8);
+   sdc->slave.src_bursts   = BIT(1) | BIT(8);
sdc->slave.directions   = BIT(DMA_DEV_TO_MEM) |
  BIT(DMA_MEM_TO_DEV);
sdc->slave.residue_granularity  = DMA_RESIDUE_GRANULARITY_BURST;
sdc->slave.dev = >dev;
 
+   if (of_device_is_compatible(pdev->dev.of_node,
+   "allwinner,sun8i-h3-dma")) {
+   sdc->slave.dst_bursts |= BIT(4);
+   sdc->slave.src_bursts |= BIT(4);
+   }
+
sdc->pchans = devm_kcalloc(>dev, sdc->cfg->nr_max_channels,
   sizeof(struct sun6i_pchan), GFP_KERNEL);
if (!sdc->pchans)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index cc535a478bae..f7bbec24bb58 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -673,6 +673,8 @@ struct dma_filter {
  * each type of direction, the dma controller should fill (1 <<
  * ) and same should be checked by controller as well
  * @max_burst: max burst capability per-transfer
+ * @dst_bursts: bitfield of the available burst sizes for the destination
+ * @src_bursts: bitfield of the available burst sizes for the source
  * @residue_granularity: granularity of the transfer residue reported
  * by tx_status
  * @device_alloc_chan_resources: allocate resources and return the
@@ -800,6 +802,14 @@ struct dma_device {
 static inline int dmaengine_slave_config(struct dma_chan *chan,
  struct dma_slave_config *config)
 {
+   if (config->src_maxburst && config->device->src_bursts &&
+   !(BIT(config->src_maxburst) & config->device->src_bursts))
+   return -EINVAL;
+
+   if (config->dst_maxburst && config->device->dst_bursts &&
+   !(BIT(config->dst_maxburst) & config->device->dst_bursts))
+   return -EINVAL;
+
if (chan->device->device_config)
return chan->device->device_config(chan, config);
---8< 

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Vinod Koul]

On Tue, Oct 04, 2016 at 03:46:51PM +0200, Jean-Francois Moine wrote:
> On Tue, 4 Oct 2016 14:12:21 +0200
> Thomas Petazzoni  wrote:
> 
> > > > Add the case of a burst of 4 which is handled by the SoC.
> > > > 
> > > > Signed-off-by: Mylène Josserand 
> > > > ---
> > > >  drivers/dma/sun6i-dma.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > > > index 8346199..0485204 100644
> > > > --- a/drivers/dma/sun6i-dma.c
> > > > +++ b/drivers/dma/sun6i-dma.c
> > > > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > > > switch (maxburst) {
> > > > case 1:
> > > > return 0;
> > > > +   case 4:
> > > > +   return 1;
> > > > case 8:
> > > > return 2;
> > > > default:
> > > > -- 
> > > > 2.9.3  
> > > 
> > > This patch has already been rejected by Maxime in the threads
> > >   http://www.spinics.net/lists/dmaengine/msg08610.html
> > > and
> > >   http://www.spinics.net/lists/dmaengine/msg08719.html
> > > 
> > > I hope you will find the way he wants for this maxburst to be added.
> > 
> > I was about to reply to Mylene's e-mail, suggesting that she should add
> > a comment in the code (and maybe in the commit log) to explain why this
> > addition is needed, and also that even though the schematics say that
> > value "1" (max burst size of 4 bytes) is reserved, it is in fact
> > incorrect. The Allwinner BSP code is really using this value, and it's
> > the value that makes audio work, so we believe the datasheet is simply
> > incorrect.
> > 
> > We already discussed it with Maxime, so I believe he should agree this
> > time. But I would suggest to have such details explained in the commit
> > log and in a comment in the code.
> 
> Strange. Looking at the datasheets of the A23, A31, A33, A83T and H3
> (these are the SoCs using the DMA sun6i), only the H3 can have 4 as the
> burst size (the doc is unclear for the A31).
> 
> Well, I was submitting for the H3, Mylène is submitting for the A33.
> So, what about the A23, A31 and A83T?

Since these are device properties, I feel we should move this to DT. That
way any new controller can have any variation based on the mood of hw
designer that day and we can hopefully cope with it :-)

But yes we would need to set the bursts supported in driver and allow above
for supported bursts only.

-- 
~Vinod

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Vinod Koul]

On Tue, Oct 04, 2016 at 03:46:51PM +0200, Jean-Francois Moine wrote:
> On Tue, 4 Oct 2016 14:12:21 +0200
> Thomas Petazzoni  wrote:
> 
> > > > Add the case of a burst of 4 which is handled by the SoC.
> > > > 
> > > > Signed-off-by: Mylène Josserand 
> > > > ---
> > > >  drivers/dma/sun6i-dma.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > > > index 8346199..0485204 100644
> > > > --- a/drivers/dma/sun6i-dma.c
> > > > +++ b/drivers/dma/sun6i-dma.c
> > > > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > > > switch (maxburst) {
> > > > case 1:
> > > > return 0;
> > > > +   case 4:
> > > > +   return 1;
> > > > case 8:
> > > > return 2;
> > > > default:
> > > > -- 
> > > > 2.9.3  
> > > 
> > > This patch has already been rejected by Maxime in the threads
> > >   http://www.spinics.net/lists/dmaengine/msg08610.html
> > > and
> > >   http://www.spinics.net/lists/dmaengine/msg08719.html
> > > 
> > > I hope you will find the way he wants for this maxburst to be added.
> > 
> > I was about to reply to Mylene's e-mail, suggesting that she should add
> > a comment in the code (and maybe in the commit log) to explain why this
> > addition is needed, and also that even though the schematics say that
> > value "1" (max burst size of 4 bytes) is reserved, it is in fact
> > incorrect. The Allwinner BSP code is really using this value, and it's
> > the value that makes audio work, so we believe the datasheet is simply
> > incorrect.
> > 
> > We already discussed it with Maxime, so I believe he should agree this
> > time. But I would suggest to have such details explained in the commit
> > log and in a comment in the code.
> 
> Strange. Looking at the datasheets of the A23, A31, A33, A83T and H3
> (these are the SoCs using the DMA sun6i), only the H3 can have 4 as the
> burst size (the doc is unclear for the A31).
> 
> Well, I was submitting for the H3, Mylène is submitting for the A33.
> So, what about the A23, A31 and A83T?

Since these are device properties, I feel we should move this to DT. That
way any new controller can have any variation based on the mood of hw
designer that day and we can hopefully cope with it :-)

But yes we would need to set the bursts supported in driver and allow above
for supported bursts only.

-- 
~Vinod

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Jean-Francois Moine]

On Tue, 4 Oct 2016 14:12:21 +0200
Thomas Petazzoni  wrote:

> > > Add the case of a burst of 4 which is handled by the SoC.
> > > 
> > > Signed-off-by: Mylène Josserand 
> > > ---
> > >  drivers/dma/sun6i-dma.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > > index 8346199..0485204 100644
> > > --- a/drivers/dma/sun6i-dma.c
> > > +++ b/drivers/dma/sun6i-dma.c
> > > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > >   switch (maxburst) {
> > >   case 1:
> > >   return 0;
> > > + case 4:
> > > + return 1;
> > >   case 8:
> > >   return 2;
> > >   default:
> > > -- 
> > > 2.9.3  
> > 
> > This patch has already been rejected by Maxime in the threads
> > http://www.spinics.net/lists/dmaengine/msg08610.html
> > and
> > http://www.spinics.net/lists/dmaengine/msg08719.html
> > 
> > I hope you will find the way he wants for this maxburst to be added.
> 
> I was about to reply to Mylene's e-mail, suggesting that she should add
> a comment in the code (and maybe in the commit log) to explain why this
> addition is needed, and also that even though the schematics say that
> value "1" (max burst size of 4 bytes) is reserved, it is in fact
> incorrect. The Allwinner BSP code is really using this value, and it's
> the value that makes audio work, so we believe the datasheet is simply
> incorrect.
> 
> We already discussed it with Maxime, so I believe he should agree this
> time. But I would suggest to have such details explained in the commit
> log and in a comment in the code.

Strange. Looking at the datasheets of the A23, A31, A33, A83T and H3
(these are the SoCs using the DMA sun6i), only the H3 can have 4 as the
burst size (the doc is unclear for the A31).

Well, I was submitting for the H3, Mylène is submitting for the A33.
So, what about the A23, A31 and A83T?

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Jean-Francois Moine]

On Tue, 4 Oct 2016 14:12:21 +0200
Thomas Petazzoni  wrote:

> > > Add the case of a burst of 4 which is handled by the SoC.
> > > 
> > > Signed-off-by: Mylène Josserand 
> > > ---
> > >  drivers/dma/sun6i-dma.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > > index 8346199..0485204 100644
> > > --- a/drivers/dma/sun6i-dma.c
> > > +++ b/drivers/dma/sun6i-dma.c
> > > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > >   switch (maxburst) {
> > >   case 1:
> > >   return 0;
> > > + case 4:
> > > + return 1;
> > >   case 8:
> > >   return 2;
> > >   default:
> > > -- 
> > > 2.9.3  
> > 
> > This patch has already been rejected by Maxime in the threads
> > http://www.spinics.net/lists/dmaengine/msg08610.html
> > and
> > http://www.spinics.net/lists/dmaengine/msg08719.html
> > 
> > I hope you will find the way he wants for this maxburst to be added.
> 
> I was about to reply to Mylene's e-mail, suggesting that she should add
> a comment in the code (and maybe in the commit log) to explain why this
> addition is needed, and also that even though the schematics say that
> value "1" (max burst size of 4 bytes) is reserved, it is in fact
> incorrect. The Allwinner BSP code is really using this value, and it's
> the value that makes audio work, so we believe the datasheet is simply
> incorrect.
> 
> We already discussed it with Maxime, so I believe he should agree this
> time. But I would suggest to have such details explained in the commit
> log and in a comment in the code.

Strange. Looking at the datasheets of the A23, A31, A33, A83T and H3
(these are the SoCs using the DMA sun6i), only the H3 can have 4 as the
burst size (the doc is unclear for the A31).

Well, I was submitting for the H3, Mylène is submitting for the A33.
So, what about the A23, A31 and A83T?

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Thomas Petazzoni]

Hello,

On Tue, 4 Oct 2016 12:40:11 +0200, Jean-Francois Moine wrote:

> > Add the case of a burst of 4 which is handled by the SoC.
> > 
> > Signed-off-by: Mylène Josserand 
> > ---
> >  drivers/dma/sun6i-dma.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index 8346199..0485204 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > switch (maxburst) {
> > case 1:
> > return 0;
> > +   case 4:
> > +   return 1;
> > case 8:
> > return 2;
> > default:
> > -- 
> > 2.9.3  
> 
> This patch has already been rejected by Maxime in the threads
>   http://www.spinics.net/lists/dmaengine/msg08610.html
> and
>   http://www.spinics.net/lists/dmaengine/msg08719.html
> 
> I hope you will find the way he wants for this maxburst to be added.

I was about to reply to Mylene's e-mail, suggesting that she should add
a comment in the code (and maybe in the commit log) to explain why this
addition is needed, and also that even though the schematics say that
value "1" (max burst size of 4 bytes) is reserved, it is in fact
incorrect. The Allwinner BSP code is really using this value, and it's
the value that makes audio work, so we believe the datasheet is simply
incorrect.

We already discussed it with Maxime, so I believe he should agree this
time. But I would suggest to have such details explained in the commit
log and in a comment in the code.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Thomas Petazzoni]

Hello,

On Tue, 4 Oct 2016 12:40:11 +0200, Jean-Francois Moine wrote:

> > Add the case of a burst of 4 which is handled by the SoC.
> > 
> > Signed-off-by: Mylène Josserand 
> > ---
> >  drivers/dma/sun6i-dma.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index 8346199..0485204 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > switch (maxburst) {
> > case 1:
> > return 0;
> > +   case 4:
> > +   return 1;
> > case 8:
> > return 2;
> > default:
> > -- 
> > 2.9.3  
> 
> This patch has already been rejected by Maxime in the threads
>   http://www.spinics.net/lists/dmaengine/msg08610.html
> and
>   http://www.spinics.net/lists/dmaengine/msg08719.html
> 
> I hope you will find the way he wants for this maxburst to be added.

I was about to reply to Mylene's e-mail, suggesting that she should add
a comment in the code (and maybe in the commit log) to explain why this
addition is needed, and also that even though the schematics say that
value "1" (max burst size of 4 bytes) is reserved, it is in fact
incorrect. The Allwinner BSP code is really using this value, and it's
the value that makes audio work, so we believe the datasheet is simply
incorrect.

We already discussed it with Maxime, so I believe he should agree this
time. But I would suggest to have such details explained in the commit
log and in a comment in the code.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Jean-Francois Moine]

On Tue,  4 Oct 2016 11:46:14 +0200
Mylène Josserand  wrote:

> Add the case of a burst of 4 which is handled by the SoC.
> 
> Signed-off-by: Mylène Josserand 
> ---
>  drivers/dma/sun6i-dma.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 8346199..0485204 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
>   switch (maxburst) {
>   case 1:
>   return 0;
> + case 4:
> + return 1;
>   case 8:
>   return 2;
>   default:
> -- 
> 2.9.3

This patch has already been rejected by Maxime in the threads
http://www.spinics.net/lists/dmaengine/msg08610.html
and
http://www.spinics.net/lists/dmaengine/msg08719.html

I hope you will find the way he wants for this maxburst to be added.

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

[Jean-Francois Moine]

On Tue,  4 Oct 2016 11:46:14 +0200
Mylène Josserand  wrote:

> Add the case of a burst of 4 which is handled by the SoC.
> 
> Signed-off-by: Mylène Josserand 
> ---
>  drivers/dma/sun6i-dma.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 8346199..0485204 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
>   switch (maxburst) {
>   case 1:
>   return 0;
> + case 4:
> + return 1;
>   case 8:
>   return 2;
>   default:
> -- 
> 2.9.3

This patch has already been rejected by Maxime in the threads
http://www.spinics.net/lists/dmaengine/msg08610.html
and
http://www.spinics.net/lists/dmaengine/msg08719.html

I hope you will find the way he wants for this maxburst to be added.

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/