[PATCH] serial: imx: disable DMA for RS-485 on i.MX6 SMP

[PATCH] serial: imx: disable DMA for RS-485 on i.MX6 SMP

[Clemens Gruber]

DMA support for half-duplex RS-485 never worked correctly on i.MX6Q/D
due to an undiscovered SMP-related bug, instead of the real data being
sent out, the rest of the transmit buffer is sent (xmit->tail jumps over
xmit->head in imx_transmit_buffer and UART_XMIT_SIZE bytes are sent out)
More details: https://lkml.org/lkml/2017/1/4/579

Disable it for that configuration until the bug is found and fixed.

We need to know at probe time if we can enable DMA. (RS-485 could be
enabled after that). Let's therefore only enable DMA if it is not an
i.MX6Q/D UART with uart-has-rtscts in the DT and CONFIG_SMP enabled.

Fixes: 17b8f2a3fdca ("serial: imx: add support for half duplex rs485")
Cc: <stable@vger.kernel.org>
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 drivers/tty/serial/imx.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index bbefddd92bfe..000949f686a4 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1277,8 +1277,14 @@ static int imx_startup(struct uart_port *port)
 
 	writel(temp & ~UCR4_DREN, sport->port.membase + UCR4);
 
-	/* Can we enable the DMA support? */
-	if (!uart_console(port) && !sport->dma_is_inited)
+	/*
+	 * Can we enable the DMA support?
+	 * RS-485 DMA is broken on i.MX6D/Q SMP systems. Do not use DMA on
+	 * UARTs with RTS/CTS to prevent misbehavior until the bug is fixed.
+	 */
+	if (!uart_console(port) && !sport->dma_is_inited &&
+	    !(sport->have_rtscts && is_imx6q_uart(sport) &&
+	      IS_ENABLED(CONFIG_SMP)))
 		imx_uart_dma_init(sport);
 
 	spin_lock_irqsave(&sport->port.lock, flags);
-- 
2.13.1

Re: [PATCH] serial: imx: disable DMA for RS-485 on i.MX6 SMP

[Fabio Estevam]

Hi Clemens,

On Tue, Jun 20, 2017 at 12:37 PM, Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
> DMA support for half-duplex RS-485 never worked correctly on i.MX6Q/D
> due to an undiscovered SMP-related bug, instead of the real data being
> sent out, the rest of the transmit buffer is sent (xmit->tail jumps over
> xmit->head in imx_transmit_buffer and UART_XMIT_SIZE bytes are sent out)
> More details: https://lkml.org/lkml/2017/1/4/579
>
> Disable it for that configuration until the bug is found and fixed.
>
> We need to know at probe time if we can enable DMA. (RS-485 could be
> enabled after that). Let's therefore only enable DMA if it is not an
> i.MX6Q/D UART with uart-has-rtscts in the DT and CONFIG_SMP enabled.
>
> Fixes: 17b8f2a3fdca ("serial: imx: add support for half duplex rs485")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
>  drivers/tty/serial/imx.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index bbefddd92bfe..000949f686a4 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1277,8 +1277,14 @@ static int imx_startup(struct uart_port *port)
>
>         writel(temp & ~UCR4_DREN, sport->port.membase + UCR4);
>
> -       /* Can we enable the DMA support? */
> -       if (!uart_console(port) && !sport->dma_is_inited)
> +       /*
> +        * Can we enable the DMA support?
> +        * RS-485 DMA is broken on i.MX6D/Q SMP systems. Do not use DMA on
> +        * UARTs with RTS/CTS to prevent misbehavior until the bug is fixed.
> +        */
> +       if (!uart_console(port) && !sport->dma_is_inited &&
> +           !(sport->have_rtscts && is_imx6q_uart(sport) &&
> +             IS_ENABLED(CONFIG_SMP)))
>                 imx_uart_dma_init(sport);

The subject gives the impression that the DMA will only be disabled
for RS485, but the impact of this change is wider.

For example: if I have a mx6q system with a Bluetooth serial
connection I can no longer use DMA with your change applied.

Ideally we should fix the RS485 DMA bug. If that is not possible, then
at least we need to restrict this change to the RS485 case.

Maybe we need to pass "linux,rs485-enabled-at-boot-time" in device
tree and then use this property to deceide if DMA will be enabled or
not:

if (!uart_console(port) && !sport->dma_is_inited && !sport->rs485_enabled)

Re: [PATCH] serial: imx: disable DMA for RS-485 on i.MX6 SMP

[Fabio Estevam]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

Hi Clemens,

On Tue, Jun 20, 2017 at 1:13 PM, Fabio Estevam <festevam@gmail.com> wrote:

> The subject gives the impression that the DMA will only be disabled
> for RS485, but the impact of this change is wider.
>
> For example: if I have a mx6q system with a Bluetooth serial
> connection I can no longer use DMA with your change applied.
>
> Ideally we should fix the RS485 DMA bug. If that is not possible, then
> at least we need to restrict this change to the RS485 case.
>
> Maybe we need to pass "linux,rs485-enabled-at-boot-time" in device
> tree and then use this property to deceide if DMA will be enabled or
> not:
>
> if (!uart_console(port) && !sport->dma_is_inited && !sport->rs485_enabled)

Could you please test the two attached patches and see if it solves the issue?

Unfortunately I no longer have access to the RS485 half-duplex board.

Just make sure to pass 'linux,rs485-enabled-at-boot-time' in your
device tree, thanks.

[-- Attachment #2: 0001-serial-imx-Allow-passing-linux-rs485-enabled-at-boot.patch --]
[-- Type: text/x-patch, Size: 1172 bytes --]

From 0a36d212228a8d093a03bbab94e9e6ffe99c87c3 Mon Sep 17 00:00:00 2001
From: Fabio Estevam <fabio.estevam@nxp.com>
Date: Tue, 20 Jun 2017 20:29:13 -0300
Subject: [PATCH 1/2] serial: imx: Support 'linux,rs485-enabled-at-boot-time'

According to Documentation/devicetree/bindings/serial/rs485.txt, the
"linux,rs485-enabled-at-boot-time" property can be used to indicate
that RS485 is enabled during boot time.

Add support for it in the driver.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 drivers/tty/serial/imx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 92606b1..90f8a5d 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2123,6 +2123,9 @@ static int serial_imx_probe(struct platform_device *pdev)
 	sport->port.rs485_config = imx_rs485_config;
 	sport->port.rs485.flags =
 		SER_RS485_RTS_ON_SEND | SER_RS485_RX_DURING_TX;
+	if (of_get_property(np, "linux,rs485-enabled-at-boot-time", NULL))
+		sport->port.rs485.flags |= SER_RS485_ENABLED;
+
 	sport->port.flags = UPF_BOOT_AUTOCONF;
 	init_timer(&sport->timer);
 	sport->timer.function = imx_timeout;
-- 
2.7.4


[-- Attachment #3: 0002-serial-imx-Disable-DMA-for-RS485.patch --]
[-- Type: text/x-patch, Size: 2240 bytes --]

From bf63a07c546ceed3d2c3d203d75e87001144423f Mon Sep 17 00:00:00 2001
From: Fabio Estevam <fabio.estevam@nxp.com>
Date: Tue, 20 Jun 2017 20:36:32 -0300
Subject: [PATCH 2/2] serial: imx: Disable DMA for RS485

Currently DMA support for half-duplex RS-485 does not work on i.MX6
as instead the real data being sent out, the rest of the transmit buffer
is sent (xmit->tail jumps over xmit->head in imx_transmit_buffer and
UART_XMIT_SIZE bytes are sent out).

Disable DMA for the RS485 case until a proper fix is found.

Reported-by: Clemens Gruber <clemens.gruber@pqgruber.com>
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 drivers/tty/serial/imx.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 90f8a5d..4812e11 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -213,6 +213,7 @@ struct imx_port {
 	struct clk		*clk_ipg;
 	struct clk		*clk_per;
 	const struct imx_uart_data *devdata;
+	bool			rs485_enabled_at_boot;
 
 	struct mctrl_gpios *gpios;
 
@@ -1278,7 +1279,8 @@ static int imx_startup(struct uart_port *port)
 	writel(temp & ~UCR4_DREN, sport->port.membase + UCR4);
 
 	/* Can we enable the DMA support? */
-	if (!uart_console(port) && !sport->dma_is_inited)
+	if (!uart_console(port) && !sport->dma_is_inited &&
+	    !sport->rs485_enabled_at_boot)
 		imx_uart_dma_init(sport);
 
 	spin_lock_irqsave(&sport->port.lock, flags);
@@ -2087,6 +2089,7 @@ static void serial_imx_probe_pdata(struct imx_port *sport,
 
 static int serial_imx_probe(struct platform_device *pdev)
 {
+	struct device_node *np = pdev->dev.of_node;
 	struct imx_port *sport;
 	void __iomem *base;
 	int ret = 0, reg;
@@ -2123,8 +2126,10 @@ static int serial_imx_probe(struct platform_device *pdev)
 	sport->port.rs485_config = imx_rs485_config;
 	sport->port.rs485.flags =
 		SER_RS485_RTS_ON_SEND | SER_RS485_RX_DURING_TX;
-	if (of_get_property(np, "linux,rs485-enabled-at-boot-time", NULL))
+	if (of_get_property(np, "linux,rs485-enabled-at-boot-time", NULL)) {
 		sport->port.rs485.flags |= SER_RS485_ENABLED;
+		sport->rs485_enabled_at_boot = true;
+	}
 
 	sport->port.flags = UPF_BOOT_AUTOCONF;
 	init_timer(&sport->timer);
-- 
2.7.4

Re: [PATCH] serial: imx: disable DMA for RS-485 on i.MX6 SMP

[Clemens Gruber]

Hi Fabio,

On Tue, Jun 20, 2017 at 01:13:18PM -0300, Fabio Estevam wrote:
> The subject gives the impression that the DMA will only be disabled
> for RS485, but the impact of this change is wider.
> 
> For example: if I have a mx6q system with a Bluetooth serial
> connection I can no longer use DMA with your change applied.
> 
> Ideally we should fix the RS485 DMA bug. If that is not possible, then
> at least we need to restrict this change to the RS485 case.
> 
> Maybe we need to pass "linux,rs485-enabled-at-boot-time" in device
> tree and then use this property to deceide if DMA will be enabled or
> not:
> 
> if (!uart_console(port) && !sport->dma_is_inited && !sport->rs485_enabled)

I'd also prefer fixing the underlying problem.
If you take a look at the DMA parts in drivers/tty/serial/imx.c, can you
spot anything SMP-unsafe?
By the way, can you get your hands on an i.MX6Q board with RS-485 to
reproduce it?

We could try for a few more weeks to find the bug and if we don't find
it, adding this rs485-enabled-at-boot-time property sounds good!

Thanks,
Clemens

Re: [PATCH] serial: imx: disable DMA for RS-485 on i.MX6 SMP

[Fabio Estevam]

Hi Clemens,

On Wed, Jun 21, 2017 at 11:05 AM, Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> I'd also prefer fixing the underlying problem.

Yes, that would be much better.

> If you take a look at the DMA parts in drivers/tty/serial/imx.c, can you
> spot anything SMP-unsafe?

Not really.

> By the way, can you get your hands on an i.MX6Q board with RS-485 to
> reproduce it?

Unfortunately I don't have such hardware. Even the mx6solo board that
I used to have access in the past I do not have it anymore.

> We could try for a few more weeks to find the bug and if we don't find
> it, adding this rs485-enabled-at-boot-time property sounds good!

Sounds like a good plan, thanks.

AW: [Customers.Eckelmann] [PATCH] serial: imx: disable DMA for RS-485 on i.MX6 SMP

[Schenk, Gavin]

Hi,

> 
> DMA support for half-duplex RS-485 never worked correctly on i.MX6Q/D
> due to an undiscovered SMP-related bug, instead of the real data being
> sent out, the rest of the transmit buffer is sent (xmit->tail jumps over
> xmit->head in imx_transmit_buffer and UART_XMIT_SIZE bytes are sent out)
> More details: https://lkml.org/lkml/2017/1/4/579
> 

We see exactly this issue on our platform, based on i.MX6dl. We use the RS485 mode to implement a Modbus interface. We have another platform based on i.MX25 running the same Linux 4.9.30 code that is not affected!

> Disable it for that configuration until the bug is found and fixed.
> 
> We need to know at probe time if we can enable DMA. (RS-485 could be
> enabled after that). Let's therefore only enable DMA if it is not an
> i.MX6Q/D UART with uart-has-rtscts in the DT and CONFIG_SMP enabled.
> 

Uwe Kleine-König had the idea to disable DMA on the i.MX6dl based platform via dts and this is our current workaround.

Regards
Gavin Schenk

Eckelmann AG
Vorstand: Dipl.-Ing. Peter Frankenbach (Sprecher) Dipl.-Wi.-Ing. Philipp Eckelmann
Dr.-Ing. Marco Münchhof Dr.-Ing. Frank Uhlemann
Vorsitzender des Aufsichtsrats: Hubertus G. Krossa
Stv. Vorsitzender des Aufsichtsrats: Dr.-Ing. Gerd Eckelmann
Sitz der Gesellschaft: Berliner Str. 161, 65205 Wiesbaden, Amtsgericht Wiesbaden HRB 12636
http://www.eckelmann.de 

Re: [PATCH] serial: imx: disable DMA for RS-485 on i.MX6 SMP

[Fabio Estevam]

Hi Clemens,

On Wed, Jun 21, 2017 at 11:12 AM, Fabio Estevam <festevam@gmail.com> wrote:

>> I'd also prefer fixing the underlying problem.
>
> Yes, that would be much better.

Just saw Romain's patch series that addresses several imx uart DMA issues:
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-June/516845.html

If you have a chance, please give it a try to see if it helps on the
RS485 DMA case.

Regards,

Fabio Estevam

Re: [PATCH] serial: imx: disable DMA for RS-485 on i.MX6 SMP

[Clemens Gruber]

Hi Fabio, Hi Romain,

On Fri, Jun 30, 2017 at 09:15:31AM -0300, Fabio Estevam wrote:
> Hi Clemens,
> 
> On Wed, Jun 21, 2017 at 11:12 AM, Fabio Estevam <festevam@gmail.com> wrote:
> 
> >> I'd also prefer fixing the underlying problem.
> >
> > Yes, that would be much better.
> 
> Just saw Romain's patch series that addresses several imx uart DMA issues:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-June/516845.html
> 
> If you have a chance, please give it a try to see if it helps on the
> RS485 DMA case.

Thanks, I just finished my first tests with Romain's patch series:

It looks like these patches fixed some of the bugs, causing this
behavior, but not all: The behavior changed, the rest of the circular
buffer is no longer sent out as seen in the previous bug report
(https://pqgruber.com/rs485_results.png)

But now, with the patch series applied, if I transmit "Test", the logic
analyzer records the following:
https://pqgruber.com/rs485txtest.png

Most of the time it looked like this (T e T e s s t t LF LF), but in a
few cases I observed another pattern (T e T s e s t t LF LF) when
transmitting "Test" by calling echo Test > /dev/ttymxc4.

If I do a echo A > /dev/ttymxc4 as in my first bug report, now I always
see the pattern A LF A LF on the TX line, but no longer A LF A LF 0 0 ..

Interestingly, the bug does not appear the first time I try echo A after
a reboot.

Romain: What board did you use to test your patch series?
The RS485 bug, I reported, only appears on i.MX6D and i.MX6Q, but not on
single-core / non-SMP systems. Would be great if you could reproduce it!

Regards,
Clemens

[PATCH] serial: imx: disable DMA for RS-485 on i.MX6 SMP

[Clemens Gruber]

Hi Fabio, Hi Romain,

On Fri, Jun 30, 2017 at 09:15:31AM -0300, Fabio Estevam wrote:
> Hi Clemens,
> 
> On Wed, Jun 21, 2017 at 11:12 AM, Fabio Estevam <festevam@gmail.com> wrote:
> 
> >> I'd also prefer fixing the underlying problem.
> >
> > Yes, that would be much better.
> 
> Just saw Romain's patch series that addresses several imx uart DMA issues:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-June/516845.html
> 
> If you have a chance, please give it a try to see if it helps on the
> RS485 DMA case.

Thanks, I just finished my first tests with Romain's patch series:

It looks like these patches fixed some of the bugs, causing this
behavior, but not all: The behavior changed, the rest of the circular
buffer is no longer sent out as seen in the previous bug report
(https://pqgruber.com/rs485_results.png)

But now, with the patch series applied, if I transmit "Test", the logic
analyzer records the following:
https://pqgruber.com/rs485txtest.png

Most of the time it looked like this (T e T e s s t t LF LF), but in a
few cases I observed another pattern (T e T s e s t t LF LF) when
transmitting "Test" by calling echo Test > /dev/ttymxc4.

If I do a echo A > /dev/ttymxc4 as in my first bug report, now I always
see the pattern A LF A LF on the TX line, but no longer A LF A LF 0 0 ..

Interestingly, the bug does not appear the first time I try echo A after
a reboot.

Romain: What board did you use to test your patch series?
The RS485 bug, I reported, only appears on i.MX6D and i.MX6Q, but not on
single-core / non-SMP systems. Would be great if you could reproduce it!

Regards,
Clemens

Re: serial: imx: disable DMA for RS-485 on i.MX6 SMP

[Clemens Gruber]

Hi Fabio,

On Tue, Jun 20, 2017 at 08:49:28PM -0300, Fabio Estevam wrote:
> Hi Clemens,
> 
> On Tue, Jun 20, 2017 at 1:13 PM, Fabio Estevam <festevam@gmail.com> wrote:
> 
> > The subject gives the impression that the DMA will only be disabled
> > for RS485, but the impact of this change is wider.
> >
> > For example: if I have a mx6q system with a Bluetooth serial
> > connection I can no longer use DMA with your change applied.
> >
> > Ideally we should fix the RS485 DMA bug. If that is not possible, then
> > at least we need to restrict this change to the RS485 case.
> >
> > Maybe we need to pass "linux,rs485-enabled-at-boot-time" in device
> > tree and then use this property to deceide if DMA will be enabled or
> > not:
> >
> > if (!uart_console(port) && !sport->dma_is_inited && !sport->rs485_enabled)
> 
> Could you please test the two attached patches and see if it solves the issue?
> 
> Unfortunately I no longer have access to the RS485 half-duplex board.
> 
> Just make sure to pass 'linux,rs485-enabled-at-boot-time' in your
> device tree, thanks.

The two patches work for me!

You can add my Tested-by tag if/when you submit them.

Tested-by: Clemens Gruber <clemens.gruber@pqgruber.com>

Regards,
Clemens

Re: serial: imx: disable DMA for RS-485 on i.MX6 SMP

[Fabio Estevam]

Hi Clemens,

On Sun, Jul 2, 2017 at 5:47 PM, Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:

> The two patches work for me!
>
> You can add my Tested-by tag if/when you submit them.
>
> Tested-by: Clemens Gruber <clemens.gruber@pqgruber.com>

Thanks for testing.

I will wait a bit longer to see if we can fix DMA usage with RS485.

Regards,

Fabio Estevam

Re: [Customers.Eckelmann] [PATCH] serial: imx: disable DMA for RS-485 on i.MX6 SMP

[Fabio Estevam]

Hi Gavin,

On Fri, Jun 23, 2017 at 6:26 AM, Schenk, Gavin <G.Schenk@eckelmann.de> wrote:

> Uwe Kleine-König had the idea to disable DMA on the i.MX6dl based platform via dts and this is our current workaround.

Please test Ian's patch:
http://marc.info/?l=linux-serial&m=150005865213531&w=2

It should fix this DMA problem with RS485.