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

Tuning the SSI fifo watermark & maxburst settings needs to be
optimized differently depending on the demands on the system.  The
current default of 2 is too low for high data-rate systems.  This
patch maintains exactly the same behavior by default (i.e defaults to
2), but adds device tree options to set maxburst & fifo depth from the
device tree.  This is necessary because a setting of 2 simply doesn't
work at higher data rates.

Signed-off-by: Caleb Crome <redacted>
---
 .../devicetree/bindings/sound/fsl,ssi.txt          | 10 +++
 sound/soc/fsl/fsl_ssi.c                            | 87 ++++++++++++++--------
 2 files changed, 68 insertions(+), 29 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/fsl,ssi.txt b/Documentation/devicetree/bindings/sound/fsl,ssi.txt
index 5b76be4..7af62b9 100644
--- a/Documentation/devicetree/bindings/sound/fsl,ssi.txt
+++ b/Documentation/devicetree/bindings/sound/fsl,ssi.txt

@@ -61,6 +61,16 @@ Optional properties:
 - fsl,mode:         The operating mode for the AC97 interface only.
                     "ac97-slave" - AC97 mode, SSI is clock slave
                     "ac97-master" - AC97 mode, SSI is clock master
+- fsl,fifo-watermark: Sets the fifo watermark.  The default is
+                    fifo_depth-2 words, meaning 'initiate dma transfer
+                    when 2 words are left in the fifo'.  At higher
+                    data rates (48kHz, 16-channels for example), this
+                    causes silent but deadly DMA xruns and channel
+                    slips.  For 15 word FIFOs (like on MX5, MX6) 8 is
+                    a good value when running at high data rates
+- fsl,dma-maxburst: sets the max number of words to transfer in DMA.
+                    This defaults to the same value as
+                    fsl,fifo-watermark.
 
 Child 'codec' node required properties:
 - compatible:       Compatible list, contains the name of the codec

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 5cfc540..94d8b5d7 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c

@@ -221,6 +221,12 @@ struct fsl_ssi_soc_data {
  * @dbg_stats: Debugging statistics
  *
  * @soc: SoC specific data
+ *
+ * @fifo_watermark: the FIFO watermark setting.  Notifies DMA when
+ *             there are @fifo_watermark or fewer words in TX fifo or
+ *             @fifo_watermark or more empty words in RX fifo.
+ * @dma_maxburst: max number of words to transfer in one go.  So far,
+ *             this is always the same as fifo_watermark.
  */
 struct fsl_ssi_private {
 	struct regmap *regs;

@@ -259,6 +265,9 @@ struct fsl_ssi_private {
 
 	const struct fsl_ssi_soc_data *soc;
 	struct device *dev;
+
+	u32 fifo_watermark;
+	u32 dma_maxburst;
 };
 
 /*

@@ -1037,21 +1046,7 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
 	regmap_write(regs, CCSR_SSI_SRCR, srcr);
 	regmap_write(regs, CCSR_SSI_SCR, scr);
 
-	/*
-	 * Set the watermark for transmit FIFI 0 and receive FIFO 0. We don't
-	 * use FIFO 1. We program the transmit water to signal a DMA transfer
-	 * if there are only two (or fewer) elements left in the FIFO. Two
-	 * elements equals one frame (left channel, right channel). This value,
-	 * however, depends on the depth of the transmit buffer.
-	 *
-	 * We set the watermark on the same level as the DMA burstsize.  For
-	 * fiq it is probably better to use the biggest possible watermark
-	 * size.
-	 */
-	if (ssi_private->use_dma)
-		wm = ssi_private->fifo_depth - 2;
-	else
-		wm = ssi_private->fifo_depth;
+	wm = ssi_private->fifo_watermark;
 
 	regmap_write(regs, CCSR_SSI_SFCSR,
 			CCSR_SSI_SFCSR_TFWM0(wm) | CCSR_SSI_SFCSR_RFWM0(wm) |

@@ -1359,12 +1354,8 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev,
 		dev_dbg(&pdev->dev, "could not get baud clock: %ld\n",
 			 PTR_ERR(ssi_private->baudclk));
 
-	/*
-	 * We have burstsize be "fifo_depth - 2" to match the SSI
-	 * watermark setting in fsl_ssi_startup().
-	 */
-	ssi_private->dma_params_tx.maxburst = ssi_private->fifo_depth - 2;
-	ssi_private->dma_params_rx.maxburst = ssi_private->fifo_depth - 2;
+	ssi_private->dma_params_tx.maxburst = ssi_private->dma_maxburst;
+	ssi_private->dma_params_rx.maxburst = ssi_private->dma_maxburst;
 	ssi_private->dma_params_tx.addr = ssi_private->ssi_phys + CCSR_SSI_STX0;
 	ssi_private->dma_params_rx.addr = ssi_private->ssi_phys + CCSR_SSI_SRX0;
 

@@ -1421,6 +1412,51 @@ static void fsl_ssi_imx_clean(struct platform_device *pdev,
 		clk_disable_unprepare(ssi_private->clk);
 }
 
+static void fsl_ssi_set_fifo_settings(
+	struct device_node *np,
+	struct fsl_ssi_private *ssi_private)
+{
+	const uint32_t *iprop;
+	/*
+	 * Determine the FIFO depth. & watermark
+	 *
+	 * Set the watermark for transmit FIFO 0/1 and receive FIFO
+	 * 0/1. We program the transmit water to signal a DMA transfer
+	 * when this many words are left to be transmitted (or
+	 * received).  Previous setting was 2 elements, which was
+	 * assumed to be one frame.  Such a small value causes silent
+	 * DMA xruns at high data rates.  8 seems to be a good
+	 * tradeoff between xruns & number of DMA transfers.
+	 *
+	 * We set the watermark on the same level as the DMA burstsize.  For
+	 * fiq it is probably better to use the biggest possible watermark
+	 * size.
+	 */
+	iprop = of_get_property(np, "fsl,fifo-depth", NULL);
+	if (iprop)
+		ssi_private->fifo_depth = be32_to_cpup(iprop);
+	else
+		/* Older 8610 DTs didn't have the fifo-depth property */
+		ssi_private->fifo_depth = 8;
+
+	iprop = of_get_property(np, "fsl,fifo-watermark", NULL);
+	if (iprop)
+		ssi_private->fifo_watermark = be32_to_cpup(iprop);
+	else
+		/* Default to old value of 2, which is too
+		 * small at high data rates, but it's the
+		 * default that's been there for years.
+		 */
+		ssi_private->fifo_watermark = ssi_private->fifo_depth - 2;
+
+	iprop = of_get_property(np, "fsl,dma-maxburst", NULL);
+
+	if (iprop)
+		ssi_private->dma_maxburst = be32_to_cpup(iprop);
+	else
+		ssi_private->dma_maxburst = ssi_private->fifo_watermark;
+}
+
 static int fsl_ssi_probe(struct platform_device *pdev)
 {
 	struct fsl_ssi_private *ssi_private;

@@ -1428,7 +1464,6 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	const struct of_device_id *of_id;
 	const char *p, *sprop;
-	const uint32_t *iprop;
 	struct resource *res;
 	void __iomem *iomem;
 	char name[64];

@@ -1510,13 +1545,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 		ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
 	}
 
-	/* Determine the FIFO depth. */
-	iprop = of_get_property(np, "fsl,fifo-depth", NULL);
-	if (iprop)
-		ssi_private->fifo_depth = be32_to_cpup(iprop);
-	else
-                /* Older 8610 DTs didn't have the fifo-depth property */
-		ssi_private->fifo_depth = 8;
+	fsl_ssi_set_fifo_settings(np, ssi_private);
 
 	dev_set_drvdata(&pdev->dev, ssi_private);
 

-- 
2.1.4

On Thu, Jan 14, 2016 at 08:29:42AM -0800, Caleb Crome wrote:

Tuning the SSI fifo watermark & maxburst settings needs to be
optimized differently depending on the demands on the system.  The
current default of 2 is too low for high data-rate systems.  This
patch maintains exactly the same behavior by default (i.e defaults to
2), but adds device tree options to set maxburst & fifo depth from the
device tree.  This is necessary because a setting of 2 simply doesn't
work at higher data rates.

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 forgot which values you've figured out for these two properties,
but I think those two values should work for normal cases as well:
as SSI only has limited FIFO depth, it won't hurt (increasing too
much latency) even if using a higher watermark configuration imo.
So it could be a good idea to use optimized settings for all use
cases and let other users test it.

Nicolin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Jan 14, 2016 at 12:18 PM, Nicolin Chen [off-list ref] wrote:

On Thu, Jan 14, 2016 at 08:29:42AM -0800, Caleb Crome wrote:

quoted

Tuning the SSI fifo watermark & maxburst settings needs to be
optimized differently depending on the demands on the system.  The
current default of 2 is too low for high data-rate systems.  This
patch maintains exactly the same behavior by default (i.e defaults to
2), but adds device tree options to set maxburst & fifo depth from the
device tree.  This is necessary because a setting of 2 simply doesn't
work at higher data rates.

quoted

@@ -61,6 +61,16 @@ Optional properties:
 - fsl,mode:         The operating mode for the AC97 interface only.
                     "ac97-slave" - AC97 mode, SSI is clock slave
                     "ac97-master" - AC97 mode, SSI is clock master
+- fsl,fifo-watermark: Sets the fifo watermark.  The default is
+                    fifo_depth-2 words, meaning 'initiate dma transfer
+                    when 2 words are left in the fifo'.  At higher
+                    data rates (48kHz, 16-channels for example), this
+                    causes silent but deadly DMA xruns and channel
+                    slips.  For 15 word FIFOs (like on MX5, MX6) 8 is
+                    a good value when running at high data rates
+- fsl,dma-maxburst: sets the max number of words to transfer in DMA.
+                    This defaults to the same value as
+                    fsl,fifo-watermark.

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.)

Yeah, I thought I'd just ask alsa first, rather than send to DT
maintainers.  Is it preferable to just send to everybody that
get_maintainers spits out even for an RFC?

I forgot which values you've figured out for these two properties,
but I think those two values should work for normal cases as well:
as SSI only has limited FIFO depth, it won't hurt (increasing too
much latency) even if using a higher watermark configuration imo.
So it could be a good idea to use optimized settings for all use
cases and let other users test it.

Nicolin

As for optimal settings, I finally came to a setting of 4 for depth &
maxburst, which will result in more DMA requests, but it's the only
way that works at 48kHz for me.  The default settings is 13 (15 - 2)
for the ones of the 15 item fifo, which is a pretty dramatic
difference.  I just don't know if other chips will behave badly in
that case.

I'd be happy to just submit a patch that sets it to 4 if we think
that's the right way to go.

-Caleb
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

On Thu, Jan 14, 2016 at 07:31:19PM -0600, Timur Tabi wrote:

Nicolin Chen wrote:

quoted

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.

Wish we could settle down a common solution for each case. If that
doesn't work for PPC, we may confine the modifications to i.MX only
by overriding those settings in the fsl_ssi_imx_probe() for safety.

On Thu, Jan 14, 2016 at 01:26:24PM -0800, Caleb Crome wrote:

As for optimal settings, I finally came to a setting of 4 for depth &
maxburst, which will result in more DMA requests, but it's the only
way that works at 48kHz for me.  The default settings is 13 (15 - 2)
for the ones of the 15 item fifo, which is a pretty dramatic
difference.  I just don't know if other chips will behave badly in
that case.

What's your final configuration for TFWM0 bits, 4?

On Thu, Jan 14, 2016 at 07:31:19PM -0600, Timur Tabi wrote:

Nicolin Chen wrote:

quoted

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.

We've allowed similar properties for other things like SPI controllers.

It really depends on the frequency you expect to change it. The more 
often it changes, the higher up the s/w stack it should be controlled 
(firmware/DT, kernel, or userspace). A function of the SOC or codec 
rate, then yes DT is fine. Every user needs to tune it on the same 
platform, then no, don't put it in DT. My recollection from i.MX is this 
would be the former case.

Rob

On Thu, Jan 14, 2016 at 6:45 PM, Nicolin Chen [off-list ref] wrote:

On Thu, Jan 14, 2016 at 01:26:24PM -0800, Caleb Crome wrote:

quoted

As for optimal settings, I finally came to a setting of 4 for depth &
maxburst, which will result in more DMA requests, but it's the only
way that works at 48kHz for me.  The default settings is 13 (15 - 2)
for the ones of the 15 item fifo, which is a pretty dramatic
difference.  I just don't know if other chips will behave badly in
that case.

What's your final configuration for TFWM0 bits, 4?

Yes, a value of 4 for my use case:  i.MX6 @ 768000 words/second (48khz
* 16 channels).

Also, works at 8kHz, 16kHz 32 kHz.

A setting of 8 does not work reliably at 48kHz but does work at 8, 16 and 32.

-caleb
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Jan 14, 2016 at 07:31:19PM -0600, Timur Tabi wrote:

Nicolin Chen wrote:

quoted

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.

Mark Brown wrote:

quoted

quoted

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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Jan 15, 2016 at 5:46 AM, Timur Tabi [off-list ref] wrote:

Mark Brown wrote:

quoted

quoted

quoted

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.

quoted

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

On Thu, Jan 14, 2016 at 08:56:31PM -0800, Caleb Crome wrote:

On Thu, Jan 14, 2016 at 6:45 PM, Nicolin Chen [off-list ref] wrote:

quoted

On Thu, Jan 14, 2016 at 01:26:24PM -0800, Caleb Crome wrote:

quoted

As for optimal settings, I finally came to a setting of 4 for depth &
maxburst, which will result in more DMA requests, but it's the only
way that works at 48kHz for me.  The default settings is 13 (15 - 2)
for the ones of the 15 item fifo, which is a pretty dramatic
difference.  I just don't know if other chips will behave badly in
that case.

What's your final configuration for TFWM0 bits, 4?

Yes, a value of 4 for my use case:  i.MX6 @ 768000 words/second (48khz
* 16 channels).

4 means there are >= 4 empty slots in the FIFO, so there are no more
than 11 remaining data. This makes sense.

IIRC, the Freescale official BSP release for i.MX is used to set 6 to
TFWM0/1 in the old day, not sure about recent ones though. So I think
setting 4 to TFWM0/1 should work for most of cases. We may also let
others test it before merging it.

Actually a setting of 13 is much more risky in my opinion. It means
only two empty slots in the FIFO, so it might be easily to get under/
overflow if a DMA transaction gets delay somehow. The only benefit is
that DMA requests and interrupt (FIQ) can be reduced.

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.

On Fri, Jan 15, 2016 at 10:38 AM, Nicolin Chen [off-list ref] wrote:

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

quoted

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.

quoted

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?

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

quoted

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.

On Fri, Jan 15, 2016 at 10:57 AM, Nicolin Chen [off-list ref] wrote:

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

quoted

quoted

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

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.

On Fri, Jan 15, 2016 at 1:10 PM, Caleb Crome [off-list ref] 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.

On Fri, Jan 15, 2016 at 12:38 PM, Nicolin Chen [off-list ref] wrote:

quoted

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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 [off-list ref] wrote:

Caleb Crome wrote:

quoted

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.