Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1310096 > unrolled thread

Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst settings device tree options

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst  settings device tree options Mark Brown <broonie@kernel.org> - 2016-01-15 14:20 +0100
    Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst  settings device tree options Timur Tabi <timur@tabi.org> - 2016-01-15 14:50 +0100
      Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst  settings device tree options Caleb Crome <caleb@crome.org> - 2016-01-15 18:10 +0100
        Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst  settings device tree options Nicolin Chen <nicoleotsuka@gmail.com> - 2016-01-15 19:40 +0100
          Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst  settings device tree options Caleb Crome <caleb@crome.org> - 2016-01-15 19:50 +0100
            Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst  settings device tree options Nicolin Chen <nicoleotsuka@gmail.com> - 2016-01-15 20:00 +0100
              Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst  settings device tree options Caleb Crome <caleb@crome.org> - 2016-01-15 20:20 +0100
                Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst  settings device tree options Nicolin Chen <nicoleotsuka@gmail.com> - 2016-01-15 20:30 +0100
                Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst  settings device tree options Timur Tabi <timur@tabi.org> - 2016-01-15 20:50 +0100
                Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst  settings device tree options Timur Tabi <timur@tabi.org> - 2016-01-16 15:20 +0100
                  Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst  settings device tree options Caleb Crome <caleb@crome.org> - 2016-01-18 00:40 +0100
          Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst  settings device tree options Timur Tabi <timur@tabi.org> - 2016-01-15 21:00 +0100

#1310096 — Re: [PATCH RFC 1/1] ASoC: fsl_ssi: Make fifo watermark and maxburst settings device tree options

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jan 14, 2016 at 07:31:19PM -0600, Timur Tabi wrote:
> Nicolin Chen wrote:
> >I think DT maintainers may not give a consent towards these two
> >properties as they are not to describe the hardware but to hack
> >software configurations. (And it seems you haven't CCed them.)

> I admit it's a grey area, but the hardware doesn't work if you use the wrong
> value, and it is a fixed value per device.  A p1022ds would use a different
> value than in in i.MX6, and once you pick a value, it's the same no matter
> which sample rate, buffer size, etc you choose.

Caleb's original message suggested this was rate dependant.

[toc] | [next] | [standalone]

#1310118

Mark Brown wrote:
>> >I admit it's a grey area, but the hardware doesn't work if you use the wrong
>> >value, and it is a fixed value per device.  A p1022ds would use a different
>> >value than in in i.MX6, and once you pick a value, it's the same no matter
>> >which sample rate, buffer size, etc you choose.

> Caleb's original message suggested this was rate dependant.

Yeah, I just noticed that.  In that case, I agree that a device tree 
property is inappropriate, unless it's an array that contains tuples of 
sample rates and watermark/maxburst settings.  That would get unwieldy 
very easily, though.

[toc] | [prev] | [next] | [standalone]

#1310284

On Fri, Jan 15, 2016 at 5:46 AM, Timur Tabi <timur@tabi.org> wrote:
> Mark Brown wrote:
>>>
>>> >I admit it's a grey area, but the hardware doesn't work if you use the
>>> > wrong
>>> >value, and it is a fixed value per device.  A p1022ds would use a
>>> > different
>>> >value than in in i.MX6, and once you pick a value, it's the same no
>>> > matter
>>> >which sample rate, buffer size, etc you choose.
>
>
>> Caleb's original message suggested this was rate dependant.
>


>
> Yeah, I just noticed that.  In that case, I agree that a device tree
> property is inappropriate, unless it's an array that contains tuples of
> sample rates and watermark/maxburst settings.  That would get unwieldy very
> easily, though.


The rate dependance is only a *potential* issue.  I suspect that a
value of 4 should be functional for all rates and chips.  The only
trade off is more DMA requests/bursts.

In a typical 15 word fifo, 48kHz, stereo, single fifo DMA system,  the
old value was 15-2 = 13, which would mean 7385 13-word DMA
bursts/second.    A new value of 4 would mean 24,000 4-word DMA
bursts/second.

Is that consequential for anybody?  It's about the same total
bandwidth on the system, but just broken up into smaller chunks (I
don't know what the overhead is for a DMA burst)

In a high channel count system (16 channels @ 48kHz), the old value
doesn't work, and the new value would mean 192,000 4-word DMA
bursts/second, which works on my MX6.  So given that 192000 works
fine, I'm not sure that the difference in a typical system would
matter at all.

If nobody objects, we can just set the value to 4 and be done with it.

Another question:  is the watermark ever going to be different than
maxburst?  Is there any reason to have them different?

-Caleb

[toc] | [prev] | [next] | [standalone]

#1310364

On Fri, Jan 15, 2016 at 09:03:28AM -0800, Caleb Crome wrote:

> If nobody objects, we can just set the value to 4 and be done with it.

I agree. And we may apply it only to i.MX platforms with DMA if
other platform owners feel comfortable with the previous settings.
 
> Another question:  is the watermark ever going to be different than
> maxburst?  Is there any reason to have them different?

The watermark is merely a threshold to trigger a DMA request. The
only relationship with the burst size is that each burst transfer
should not carry more data than the number of empty slots; FIFO
under/overflow occurs otherwise. So it's just more efficient and
safer to set an identical value to both of them. I don't think
it will cause functional problems to set TFWM to 4 and burst size
to 1 -- It just lets DMA operate in a single data transfer mode.

[toc] | [prev] | [next] | [standalone]

#1310367

On Fri, Jan 15, 2016 at 10:38 AM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> On Fri, Jan 15, 2016 at 09:03:28AM -0800, Caleb Crome wrote:
>
>> If nobody objects, we can just set the value to 4 and be done with it.
>
> I agree. And we may apply it only to i.MX platforms with DMA if
> other platform owners feel comfortable with the previous settings.
>
>> Another question:  is the watermark ever going to be different than
>> maxburst?  Is there any reason to have them different?
>
> The watermark is merely a threshold to trigger a DMA request. The
> only relationship with the burst size is that each burst transfer
> should not carry more data than the number of empty slots; FIFO
> under/overflow occurs otherwise. So it's just more efficient and
> safer to set an identical value to both of them. I don't think
> it will cause functional problems to set TFWM to 4 and burst size
> to 1 -- It just lets DMA operate in a single data transfer mode.

If there is no penalty for setting maxburst to 1 (or 2 in the case of
dual fifo I think), then should we just set both the watermark and
maxburst to 1?

I guess the real difference would be when you're in FIQ mode.  In FIQ
mode, the penalty of an interrupt per word would be pretty bad, but in
DMA mode, if we just set both to 1, we should be fine, right?

[toc] | [prev] | [next] | [standalone]

#1310374

On Fri, Jan 15, 2016 at 10:49:04AM -0800, Caleb Crome wrote:

> > The watermark is merely a threshold to trigger a DMA request. The
> > only relationship with the burst size is that each burst transfer
> > should not carry more data than the number of empty slots; FIFO
> > under/overflow occurs otherwise. So it's just more efficient and
> > safer to set an identical value to both of them. I don't think
> > it will cause functional problems to set TFWM to 4 and burst size
> > to 1 -- It just lets DMA operate in a single data transfer mode.
> 
> If there is no penalty for setting maxburst to 1 (or 2 in the case of
> dual fifo I think), then should we just set both the watermark and
> maxburst to 1?
> 
> I guess the real difference would be when you're in FIQ mode.  In FIQ
> mode, the penalty of an interrupt per word would be pretty bad, but in
> DMA mode, if we just set both to 1, we should be fine, right?

There will be much more overhead drawn by frequent DMA transfers.
I believe you understand the idea -- less burst size then more DMA
request. Each DMA transfer contains a pair of handshaking overhead
according to the bus protocol. Apparently the bus will be wasted
with lots of handshaking section instead of keep dedicated to data
transfer truly. It might work if SSI is the only user of the bus,
which we shouldn't assume.

[toc] | [prev] | [next] | [standalone]

#1310389

On Fri, Jan 15, 2016 at 10:57 AM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> On Fri, Jan 15, 2016 at 10:49:04AM -0800, Caleb Crome wrote:
>
>> > The watermark is merely a threshold to trigger a DMA request. The
>> > only relationship with the burst size is that each burst transfer
>> > should not carry more data than the number of empty slots; FIFO
>> > under/overflow occurs otherwise. So it's just more efficient and
>> > safer to set an identical value to both of them. I don't think
>> > it will cause functional problems to set TFWM to 4 and burst size
>> > to 1 -- It just lets DMA operate in a single data transfer mode.
>>
>> If there is no penalty for setting maxburst to 1 (or 2 in the case of
>> dual fifo I think), then should we just set both the watermark and
>> maxburst to 1?
>>
>> I guess the real difference would be when you're in FIQ mode.  In FIQ
>> mode, the penalty of an interrupt per word would be pretty bad, but in
>> DMA mode, if we just set both to 1, we should be fine, right?
>
> There will be much more overhead drawn by frequent DMA transfers.
> I believe you understand the idea -- less burst size then more DMA
> request. Each DMA transfer contains a pair of handshaking overhead
> according to the bus protocol. Apparently the bus will be wasted
> with lots of handshaking section instead of keep dedicated to data
> transfer truly. It might work if SSI is the only user of the bus,
> which we shouldn't assume.

Right, I knew there must be a trade off.  So, how about I submit a
patch with a fixed value of 4 for whichever platforms you think is
correct.

I see the 4 compatibles are:  fsl,mpc8610-ssi, fsl,imx51-ssi,
fsl,imx35-ssi, and fsl,imx21-ssi.

Shall I submit a patch that sets the value to 4 for for any/all of the
above?  Should it be different based on whether fiq is enabled.  Maybe
set it to 4 for whenever DMA is used, and to 13 whenever FIQ is used?

-Caleb

[toc] | [prev] | [next] | [standalone]

#1310395

On Fri, Jan 15, 2016 at 11:10:29AM -0800, Caleb Crome wrote:
 
> Shall I submit a patch that sets the value to 4 for for any/all of the
> above?  Should it be different based on whether fiq is enabled.  Maybe
> set it to 4 for whenever DMA is used, and to 13 whenever FIQ is used?

A possible solution:
Use the original settings (depth - 2) as a common configuration
in the main probe() and overwrite it in the fsl_ssi_imx_probe()
if detecting a use_dma flag.

[toc] | [prev] | [next] | [standalone]

#1310422

On Fri, Jan 15, 2016 at 1:10 PM, Caleb Crome <caleb@crome.org> wrote:
>
> I see the 4 compatibles are:  fsl,mpc8610-ssi, fsl,imx51-ssi,
> fsl,imx35-ssi, and fsl,imx21-ssi.
>
> Shall I submit a patch that sets the value to 4 for for any/all of the
> above?  Should it be different based on whether fiq is enabled.  Maybe
> set it to 4 for whenever DMA is used, and to 13 whenever FIQ is used?


I don't like the idea that we'll need a new device tree to make a new
kernel work.  The driver should continue to work with old device trees
as-is.

[toc] | [prev] | [next] | [standalone]

#1310959

Caleb Crome wrote:
> I see the 4 compatibles are:  fsl,mpc8610-ssi, fsl,imx51-ssi,
> fsl,imx35-ssi, and fsl,imx21-ssi.
>
> Shall I submit a patch that sets the value to 4 for for any/all of the
> above?  Should it be different based on whether fiq is enabled.  Maybe
> set it to 4 for whenever DMA is used, and to 13 whenever FIQ is used?

Do NOT change the watermark value for fsl,mpc8610-ssi from whatever it 
is today.  You would need to retest your code on an mpc8610 and p1022ds, 
and I don't think you have either of those boards.

[toc] | [prev] | [next] | [standalone]

#1311200

Okay, I'll submit a patch that does the following:

fsl,mpc8610:  using DMA=fifo_depth-2 (unchanged from current), using
FIQ=fifo_depth  (unchanged from current).

all others that are marked with 'imx':  watermark&maxburst = 4 for DMA
(new setting).  watermark = fifo_depth for FIQ (unchanged from
previous).

-Caleb



On Sat, Jan 16, 2016 at 6:15 AM, Timur Tabi <timur@tabi.org> wrote:
> Caleb Crome wrote:
>>
>> I see the 4 compatibles are:  fsl,mpc8610-ssi, fsl,imx51-ssi,
>> fsl,imx35-ssi, and fsl,imx21-ssi.
>>
>> Shall I submit a patch that sets the value to 4 for for any/all of the
>> above?  Should it be different based on whether fiq is enabled.  Maybe
>> set it to 4 for whenever DMA is used, and to 13 whenever FIQ is used?
>
>
> Do NOT change the watermark value for fsl,mpc8610-ssi from whatever it is
> today.  You would need to retest your code on an mpc8610 and p1022ds, and I
> don't think you have either of those boards.

[toc] | [prev] | [next] | [standalone]

#1310431

On Fri, Jan 15, 2016 at 12:38 PM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
>> If nobody objects, we can just set the value to 4 and be done with it.
>
> I agree. And we may apply it only to i.MX platforms with DMA if
> other platform owners feel comfortable with the previous settings.

It's imperative that the PowerPC chips continue to use whatever
setting they're using now.  It took a long time for me to find values
that actually work on those chips, and I don't want to have to re-test
the driver on hardware that I don't have any more.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web