[PATCH] serial: imx: Revert "initialized DMA w/o HW flow enabled"

[PATCH] serial: imx: Revert "initialized DMA w/o HW flow enabled"

[Sebastian Andrzej Siewior]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

This basically reverts commit 068500e08dc8 ("serial: imx: initialized
DMA w/o HW flow enabled") simply because it does not work as expected
without additional magic which I am not aware of. The DMA mode is only
used for non-console UARTs so the "standard" boot test won't notice this.

I tested it on various IMX6 (S and Q) boards and the default SDMA firmware
that is included in ROM does work properly. The SDMA interrupt counter does
not stop increment (which means the SDMA does not stop working or the ACK
is working) and I see a lot of 0x00 (sometimes other bytes) which are added
to the tty buffer which are not part of the data transfer.
Jiada claimed that there is a firmware which makes it work but failed to
provide me a link to the firmware. All he said was that it can not be
shipped as part of the kernel due to license issues. I've found a firmware
blob in the FSL-tree but this firmware causes only TTY-overflows an *no*
data is transmitted.

Currently I see no other way out.

Fixes: 068500e08dc8 ("serial: imx: initialized DMA w/o HW flow enabled")
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Jiada Wang <jiada_wang@mentor.com>
Cc: nicolae.rosia@gmail.com
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/imx.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8aff0b4d8ddf..e686c75fa4dc 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1113,11 +1113,6 @@ static int imx_startup(struct uart_port *port)
 
 	writel(temp & ~UCR4_DREN, sport->port.membase + UCR4);
 
-	/* Can we enable the DMA support? */
-	if (is_imx6q_uart(sport) && !uart_console(port) &&
-	    !sport->dma_is_inited)
-		imx_uart_dma_init(sport);
-
 	spin_lock_irqsave(&sport->port.lock, flags);
 	/* Reset fifo's and state machines */
 	i = 100;
@@ -1308,6 +1303,17 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
 			} else {
 				ucr2 |= UCR2_CTSC;
 			}
+
+			/*
+			 * Can we enable the DMA support? Be aware that
+			 * at least IMX6 needs to load external SDMA firmware in
+			 * order work properly. The in-ROM firmware is not
+			 * wokring properly.
+			 */
+			if ((ucr2 & UCR2_CTSC) && is_imx6q_uart(sport) &&
+			    !uart_console(port) && !sport->dma_is_inited)
+				imx_uart_dma_init(sport);
+
 		} else {
 			termios->c_cflag &= ~CRTSCTS;
 		}
@@ -1424,6 +1430,8 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
 	if (UART_ENABLE_MS(&sport->port, termios->c_cflag))
 		imx_enable_ms(&sport->port);
 
+	if (sport->dma_is_inited && !sport->dma_is_enabled)
+		imx_enable_dma(sport);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 }
 
-- 
2.1.4

Re: [PATCH] serial: imx: Revert "initialized DMA w/o HW flow enabled"

[Fabio Estevam]

On Thu, May 7, 2015 at 5:24 PM, Sebastian Andrzej Siewior
<sebastian@breakpoint.cc> wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> This basically reverts commit 068500e08dc8 ("serial: imx: initialized
> DMA w/o HW flow enabled") simply because it does not work as expected
> without additional magic which I am not aware of. The DMA mode is only
> used for non-console UARTs so the "standard" boot test won't notice this.
>
> I tested it on various IMX6 (S and Q) boards and the default SDMA firmware
> that is included in ROM does work properly. The SDMA interrupt counter does
> not stop increment (which means the SDMA does not stop working or the ACK
> is working) and I see a lot of 0x00 (sometimes other bytes) which are added
> to the tty buffer which are not part of the data transfer.
> Jiada claimed that there is a firmware which makes it work but failed to
> provide me a link to the firmware. All he said was that it can not be
> shipped as part of the kernel due to license issues. I've found a firmware
> blob in the FSL-tree but this firmware causes only TTY-overflows an *no*
> data is transmitted.
>
> Currently I see no other way out.
>
> Fixes: 068500e08dc8 ("serial: imx: initialized DMA w/o HW flow enabled")
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Jiada Wang <jiada_wang@mentor.com>
> Cc: nicolae.rosia@gmail.com
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Thanks, this fixes Bluetooth operation on a imx6sl-warp running
without SDMA firmware.

Two suggestions:

1. You should Cc stable as it affects 4.0 also

2. It doesn't apply cleanly against linux-next, so I think Greg will
have problems applying it.

Re: [PATCH] serial: imx: Revert "initialized DMA w/o HW flow enabled"

[Sebastian Andrzej Siewior]

On 2015-05-07 17:54:31 [-0300], Fabio Estevam wrote:
> Thanks, this fixes Bluetooth operation on a imx6sl-warp running
> without SDMA firmware.
> 
> Two suggestions:
> 
> 1. You should Cc stable as it affects 4.0 also

I have the Fixes tag, doesn't this count?

> 2. It doesn't apply cleanly against linux-next, so I think Greg will
> have problems applying it.

It is on top of Greg's tty-next tree. Shouldn't this work?

Sebastian

Re: [PATCH] serial: imx: Revert "initialized DMA w/o HW flow enabled"

[Fabio Estevam]

On Thu, May 7, 2015 at 6:16 PM, Sebastian Andrzej Siewior
<sebastian@breakpoint.cc> wrote:
> On 2015-05-07 17:54:31 [-0300], Fabio Estevam wrote:
>> Thanks, this fixes Bluetooth operation on a imx6sl-warp running
>> without SDMA firmware.
>>
>> Two suggestions:
>>
>> 1. You should Cc stable as it affects 4.0 also
>
> I have the Fixes tag, doesn't this count?

According to Documentation/stable_kernel_rules.txt you need to Cc
stable so that it can go to the stable tree.

>> 2. It doesn't apply cleanly against linux-next, so I think Greg will
>> have problems applying it.
>
> It is on top of Greg's tty-next tree. Shouldn't this work?

Yes, this should be fine.

Re: [PATCH] serial: imx: Revert "initialized DMA w/o HW flow enabled"

[Fabio Estevam]

On Thu, May 7, 2015 at 5:24 PM, Sebastian Andrzej Siewior
<sebastian@breakpoint.cc> wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> This basically reverts commit 068500e08dc8 ("serial: imx: initialized
> DMA w/o HW flow enabled") simply because it does not work as expected
> without additional magic which I am not aware of. The DMA mode is only
> used for non-console UARTs so the "standard" boot test won't notice this.
>
> I tested it on various IMX6 (S and Q) boards and the default SDMA firmware
> that is included in ROM does work properly. The SDMA interrupt counter does

Here you say that the ROM SDMA works properly.

> not stop increment (which means the SDMA does not stop working or the ACK
> is working) and I see a lot of 0x00 (sometimes other bytes) which are added
> to the tty buffer which are not part of the data transfer.
> Jiada claimed that there is a firmware which makes it work but failed to
> provide me a link to the firmware. All he said was that it can not be
> shipped as part of the kernel due to license issues. I've found a firmware
> blob in the FSL-tree but this firmware causes only TTY-overflows an *no*
> data is transmitted.
>
> Currently I see no other way out.
>
> Fixes: 068500e08dc8 ("serial: imx: initialized DMA w/o HW flow enabled")
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Jiada Wang <jiada_wang@mentor.com>
> Cc: nicolae.rosia@gmail.com
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/tty/serial/imx.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8aff0b4d8ddf..e686c75fa4dc 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1113,11 +1113,6 @@ static int imx_startup(struct uart_port *port)
>
>         writel(temp & ~UCR4_DREN, sport->port.membase + UCR4);
>
> -       /* Can we enable the DMA support? */
> -       if (is_imx6q_uart(sport) && !uart_console(port) &&
> -           !sport->dma_is_inited)
> -               imx_uart_dma_init(sport);
> -
>         spin_lock_irqsave(&sport->port.lock, flags);
>         /* Reset fifo's and state machines */
>         i = 100;
> @@ -1308,6 +1303,17 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
>                         } else {
>                                 ucr2 |= UCR2_CTSC;
>                         }
> +
> +                       /*
> +                        * Can we enable the DMA support? Be aware that
> +                        * at least IMX6 needs to load external SDMA firmware in
> +                        * order work properly. The in-ROM firmware is not
> +                        * wokring properly.

,but here you say the contrary.

With this fixed (and please also Cc stable):

Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>
Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

[PATCH v2] serial: imx: Revert "initialized DMA w/o HW flow enabled"

[Sebastian Andrzej Siewior]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

This basically reverts commit 068500e08dc8 ("serial: imx: initialized
DMA w/o HW flow enabled") simply because it does not work.
I tested it on various IMX6 boards and the default SDMA firmware, that is
included in ROM, does not work properly. The SDMA interrupt counter does
not stop increment (which means the SDMA does not stop working) and I see
a lot of 0x00 (sometimes other bytes) which are added to the tty buffer
which are not part of the data transfer.
Jiada claimed that there is a firmware which makes it work but failed to
provide me a link to the firmware. All he said was that it can not be
shipped as part of the kernel due to license issues. I've found one
firmware in the FSL-tree but this firmware causes only TTY-overflows and
*no* data is transmitted.

Currently I see no other way out.

Fixes: 068500e08dc8 ("serial: imx: initialized DMA w/o HW flow enabled")
Cc: stable@vger.kernel.org
Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>
Tested-by: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Jiada Wang <jiada_wang@mentor.com>
Cc: nicolae.rosia@gmail.com
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: wording

 drivers/tty/serial/imx.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8aff0b4d8ddf..e686c75fa4dc 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1113,11 +1113,6 @@ static int imx_startup(struct uart_port *port)
 
 	writel(temp & ~UCR4_DREN, sport->port.membase + UCR4);
 
-	/* Can we enable the DMA support? */
-	if (is_imx6q_uart(sport) && !uart_console(port) &&
-	    !sport->dma_is_inited)
-		imx_uart_dma_init(sport);
-
 	spin_lock_irqsave(&sport->port.lock, flags);
 	/* Reset fifo's and state machines */
 	i = 100;
@@ -1308,6 +1303,17 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
 			} else {
 				ucr2 |= UCR2_CTSC;
 			}
+
+			/*
+			 * Can we enable the DMA support? Be aware that
+			 * at least IMX6 needs to load external SDMA firmware in
+			 * order work properly. The in-ROM firmware is not
+			 * wokring properly.
+			 */
+			if ((ucr2 & UCR2_CTSC) && is_imx6q_uart(sport) &&
+			    !uart_console(port) && !sport->dma_is_inited)
+				imx_uart_dma_init(sport);
+
 		} else {
 			termios->c_cflag &= ~CRTSCTS;
 		}
@@ -1424,6 +1430,8 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
 	if (UART_ENABLE_MS(&sport->port, termios->c_cflag))
 		imx_enable_ms(&sport->port);
 
+	if (sport->dma_is_inited && !sport->dma_is_enabled)
+		imx_enable_dma(sport);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 }
 
-- 
2.1.4

Re: [PATCH v2] serial: imx: Revert "initialized DMA w/o HW flow enabled"

[Fabio Estevam]

On Fri, May 8, 2015 at 4:13 AM, Sebastian Andrzej Siewior
<sebastian@breakpoint.cc> wrote:

> +
> +                       /*
> +                        * Can we enable the DMA support? Be aware that
> +                        * at least IMX6 needs to load external SDMA firmware in
> +                        * order work properly. The in-ROM firmware is not
> +                        * wokring properly.
> +                        */

On my tests:

Prior to this patch: with the internal ROM SDMA firmware I got UART
issues when communicating through a Bluetooth module. With external
SDMA firmware (version 3.1) it worked fine.

With your patch applied: Bluetooth worked fine even with no external
SDMA firmware, which does not match the comments you added.

Can't you just remove these comments?

Thanks

Re: [PATCH v2] serial: imx: Revert "initialized DMA w/o HW flow enabled"

[Sebastian Andrzej Siewior]

On 05/08/2015 01:30 PM, Fabio Estevam wrote:
> On Fri, May 8, 2015 at 4:13 AM, Sebastian Andrzej Siewior
> <sebastian@breakpoint.cc> wrote:
> 
>> +
>> +                       /*
>> +                        * Can we enable the DMA support? Be aware that
>> +                        * at least IMX6 needs to load external SDMA firmware in
>> +                        * order work properly. The in-ROM firmware is not
>> +                        * wokring properly.
>> +                        */
> 
> On my tests:
> 
> Prior to this patch: with the internal ROM SDMA firmware I got UART
> issues when communicating through a Bluetooth module. With external
> SDMA firmware (version 3.1) it worked fine.
> 
> With your patch applied: Bluetooth worked fine even with no external
> SDMA firmware, which does not match the comments you added.

Why doesn't it match the comment? The patch disables DMA so the SDMA
firmware does not matter.

> Can't you just remove these comments?
> 
> Thanks

Sebastian

Re: [PATCH v2] serial: imx: Revert "initialized DMA w/o HW flow enabled"

[Fabio Estevam]

On Fri, May 8, 2015 at 8:50 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:

> Why doesn't it match the comment? The patch disables DMA so the SDMA
> firmware does not matter.

Got it. It is clear now, thanks.

Re: [PATCH v2] serial: imx: Revert "initialized DMA w/o HW flow enabled"

[Fabio Estevam]

Hi Sebastian,

On Fri, May 8, 2015 at 4:13 AM, Sebastian Andrzej Siewior
<sebastian@breakpoint.cc> wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> This basically reverts commit 068500e08dc8 ("serial: imx: initialized
> DMA w/o HW flow enabled") simply because it does not work.
> I tested it on various IMX6 boards and the default SDMA firmware, that is
> included in ROM, does not work properly. The SDMA interrupt counter does
> not stop increment (which means the SDMA does not stop working) and I see
> a lot of 0x00 (sometimes other bytes) which are added to the tty buffer
> which are not part of the data transfer.
> Jiada claimed that there is a firmware which makes it work but failed to
> provide me a link to the firmware. All he said was that it can not be
> shipped as part of the kernel due to license issues. I've found one
> firmware in the FSL-tree but this firmware causes only TTY-overflows and
> *no* data is transmitted.
>
> Currently I see no other way out.

Could you please test Philipp's patch?
http://marc.info/?l=linux-serial&m=143202566332128&q=raw

It worked on my tests and DMA still works with his patch, so I would
prefer his solution.

Thanks,

Fabio Estevam

Re: [PATCH v2] serial: imx: Revert "initialized DMA w/o HW flow enabled"

[Sebastian Andrzej Siewior]

On 05/19/2015 02:18 PM, Fabio Estevam wrote:
> Hi Sebastian,

Hi Fabio,

> Could you please test Philipp's patch?
> http://marc.info/?l=linux-serial&m=143202566332128&q=raw
> 
> It worked on my tests and DMA still works with his patch, so I would
> prefer his solution.

It seems to work here, too. Thanks.

> 
> Thanks,
> 
> Fabio Estevam

Sebastian