[PATCH] tty: serial: imx: fix rs485 rx after tx

[PATCH] tty: serial: imx: fix rs485 rx after tx

[Martin Fuzzey]

Since commit 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal
active high") RS485 reception no longer works after a transmission.

The following scenario shows the problem:
	1) Open a port in RS485 mode
	2) Receive data from remote (OK)
	3) Transmit data to remote (OK)
	4) Receive data from remote (Nothing received)

In RS485 mode, imx_uart_start_tx() calls imx_uart_stop_rx() and, when the
transmission is complete, imx_uart_stop_tx() calls imx_uart_start_rx().

Since the above commit imx_uart_stop_rx() now sets the loopback bit but
imx_uart_start_rx() does not clear it causing the hardware to remain in
loopback mode and not receive external data.

Fix this by moving the existing loopback disable code to a helper function
and calling it from imx_uart_start_rx() too.

Fixes: 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal active high")
Cc: stable@vger.kernel.org
Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
---
 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 c5e17569c3ad..3fe8ff241522 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -369,6 +369,16 @@ static void imx_uart_soft_reset(struct imx_port *sport)
 	sport->idle_counter = 0;
 }
 
+static void imx_uart_disable_loopback_rs485(struct imx_port *sport)
+{
+	unsigned int uts;
+
+	/* See SER_RS485_ENABLED/UTS_LOOP comment in imx_uart_probe() */
+	uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
+	uts &= ~UTS_LOOP;
+	imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
+}
+
 /* called with port.lock taken and irqs off */
 static void imx_uart_start_rx(struct uart_port *port)
 {
@@ -390,6 +400,7 @@ static void imx_uart_start_rx(struct uart_port *port)
 	/* Write UCR2 first as it includes RXEN */
 	imx_uart_writel(sport, ucr2, UCR2);
 	imx_uart_writel(sport, ucr1, UCR1);
+	imx_uart_disable_loopback_rs485(sport);
 }
 
 /* called with port.lock taken and irqs off */
@@ -1422,7 +1433,7 @@ static int imx_uart_startup(struct uart_port *port)
 	int retval;
 	unsigned long flags;
 	int dma_is_inited = 0;
-	u32 ucr1, ucr2, ucr3, ucr4, uts;
+	u32 ucr1, ucr2, ucr3, ucr4;
 
 	retval = clk_prepare_enable(sport->clk_per);
 	if (retval)
@@ -1521,10 +1532,7 @@ static int imx_uart_startup(struct uart_port *port)
 		imx_uart_writel(sport, ucr2, UCR2);
 	}
 
-	/* See SER_RS485_ENABLED/UTS_LOOP comment in imx_uart_probe() */
-	uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
-	uts &= ~UTS_LOOP;
-	imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
+	imx_uart_disable_loopback_rs485(sport);
 
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
-- 
2.25.1

Re: [PATCH] tty: serial: imx: fix rs485 rx after tx

[Marek Vasut]

On 6/16/23 12:47, Martin Fuzzey wrote:
> Since commit 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal
> active high") RS485 reception no longer works after a transmission.

This RS485 is just a gift that keeps on giving, sigh.

I'll dig into this in a few days as time permits.

Re: [PATCH] tty: serial: imx: fix rs485 rx after tx

[Marek Vasut]

On 6/16/23 13:16, Marek Vasut wrote:
> On 6/16/23 12:47, Martin Fuzzey wrote:
>> Since commit 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal
>> active high") RS485 reception no longer works after a transmission.
> 
> This RS485 is just a gift that keeps on giving, sigh.
> 
> I'll dig into this in a few days as time permits.

Alas, time did not permit until now, but yes, this makes sense.

Thanks

And sorry for the delayed reply.

Re: [PATCH] tty: serial: imx: fix rs485 rx after tx

[Ilpo Järvinen]

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

On Fri, 16 Jun 2023, Martin Fuzzey wrote:

> Since commit 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal
> active high") RS485 reception no longer works after a transmission.
> 
> The following scenario shows the problem:
> 	1) Open a port in RS485 mode
> 	2) Receive data from remote (OK)
> 	3) Transmit data to remote (OK)
> 	4) Receive data from remote (Nothing received)
> 
> In RS485 mode, imx_uart_start_tx() calls imx_uart_stop_rx() and, when the
> transmission is complete, imx_uart_stop_tx() calls imx_uart_start_rx().
> 
> Since the above commit imx_uart_stop_rx() now sets the loopback bit but
> imx_uart_start_rx() does not clear it causing the hardware to remain in
> loopback mode and not receive external data.
> 
> Fix this by moving the existing loopback disable code to a helper function
> and calling it from imx_uart_start_rx() too.
> 
> Fixes: 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal active high")
> Cc: stable@vger.kernel.org
> Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
> ---
>  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 c5e17569c3ad..3fe8ff241522 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -369,6 +369,16 @@ static void imx_uart_soft_reset(struct imx_port *sport)
>  	sport->idle_counter = 0;
>  }
>  
> +static void imx_uart_disable_loopback_rs485(struct imx_port *sport)
> +{
> +	unsigned int uts;
> +
> +	/* See SER_RS485_ENABLED/UTS_LOOP comment in imx_uart_probe() */
> +	uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
> +	uts &= ~UTS_LOOP;
> +	imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
> +}
> +
>  /* called with port.lock taken and irqs off */
>  static void imx_uart_start_rx(struct uart_port *port)
>  {
> @@ -390,6 +400,7 @@ static void imx_uart_start_rx(struct uart_port *port)
>  	/* Write UCR2 first as it includes RXEN */
>  	imx_uart_writel(sport, ucr2, UCR2);
>  	imx_uart_writel(sport, ucr1, UCR1);
> +	imx_uart_disable_loopback_rs485(sport);
>  }
>  
>  /* called with port.lock taken and irqs off */
> @@ -1422,7 +1433,7 @@ static int imx_uart_startup(struct uart_port *port)
>  	int retval;
>  	unsigned long flags;
>  	int dma_is_inited = 0;
> -	u32 ucr1, ucr2, ucr3, ucr4, uts;
> +	u32 ucr1, ucr2, ucr3, ucr4;
>  
>  	retval = clk_prepare_enable(sport->clk_per);
>  	if (retval)
> @@ -1521,10 +1532,7 @@ static int imx_uart_startup(struct uart_port *port)
>  		imx_uart_writel(sport, ucr2, UCR2);
>  	}
>  
> -	/* See SER_RS485_ENABLED/UTS_LOOP comment in imx_uart_probe() */
> -	uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
> -	uts &= ~UTS_LOOP;
> -	imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
> +	imx_uart_disable_loopback_rs485(sport);
>  
>  	spin_unlock_irqrestore(&sport->port.lock, flags);

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH] tty: serial: imx: fix rs485 rx after tx

[Sebastien Laveze]

> Since commit 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal
> active high") RS485 reception no longer works after a transmission.

I can confirm on a Modbus/RS485 setup.

> Fix this by moving the existing loopback disable code to a helper
> function
> and calling it from imx_uart_start_rx() too.
> 
> Fixes: 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal active
> high")

Unfortunately this doesn't fix the regression on my setup and I had to
fully revert 79d0224f6bf2. 

Since there's a Modbus layer on top, it's always TX to remote then RX.

Note that RS485 communication has never been perfect on my setup. After
TX the DE line is often held active for too long leading to corrupted
RX if too close from last TX. This leads to occasional frame loss in
Modbus but it's not a blocker. Hope to get some time to investigate.

https://bugzilla.kernel.org/show_bug.cgi?id=207751