Skip to contentSkip to conversation

tty: serial: imx: Fix broken RS485

PATCH Superseded Rickard Andersson 5 participants 9 messages Feb 20, 2024 – Mar 7, 2024

Subsystems (2)
Arm i.MX and Layerscape TTY layer and serial drivers

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Rickard Andersson

author
#0 lore raw

PATCH v2 1/1 tty: serial: imx: Fix broken RS485

From: Rickard x Andersson <[email protected]>

When about to transmit the function imx_uart_start_tx is called and in some RS485 configurations this function will call imx_uart_stop_rx. The problem is that imx_uart_stop_rx will enable loopback and when loopback is enabled transmitted data will just be looped to RX.

This patch fixes the above problem by explicitly disabling loopback in the case described above.

Fixes 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal active high")
Signed-off-by Rickard x Andersson <[email protected]>
---
 drivers/tty/serial/imx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/tty/serial/imx.c +8 -1
index 4aa72d5aeafb..899e331bdfc8 100644
@@ -683,8 +683,15 @@ static void imx_uart_start_tx(struct uart_port *port)
 			imx_uart_writel(sport, ucr2, UCR2);
 
 			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX) &&
-			    !port->rs485_rx_during_tx_gpio)
+			    !port->rs485_rx_during_tx_gpio) {
 				imx_uart_stop_rx(port);
+				/*
+				 * The function imx_uart_stop_rx right above
+				 * will enable loopback, but since we are just
+				 * about to transmit then disable loopback.
+				 */
+				imx_uart_disable_loopback_rs485(sport);
+			}
 
 			sport->tx_state = WAIT_AFTER_RTS;
Signature · 1 line
2.30.2
maintainer
#1 lore raw
Quoted 7 lines · Rickard Andersson

From: Rickard x Andersson <[email protected]>

When about to transmit the function imx_uart_start_tx is called and in some RS485 configurations this function will call imx_uart_stop_rx. The problem is that imx_uart_stop_rx will enable loopback and when loopback is enabled transmitted data will just be looped to RX.

Quoted 8 lines

This patch fixes the above problem by explicitly disabling loopback in the case described above.

Fixes 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal active high")
Signed-off-by Rickard x Andersson <[email protected]>
---
 drivers/tty/serial/imx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree.

You are receiving this message because of the following common error(s) as indicated below:

- This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what needs to be done here to properly describe this.

- You sent a patch that has been sent multiple times in the past few days, and is identical to ones that has been recently rejected. Please always look at the mailing list traffic to determine if you are duplicating other people's work.

- You have marked a patch with a "Fixes:" tag for a commit that is in an older released kernel, yet you do not have a cc: stable line in the signed-off-by area at all, which means that the patch will not be applied to any older kernel releases. To properly fix this, please follow the documented rules in the Documentation/process/stable-kernel-rules.rst file for how to resolve this.

If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers.

thanks,

greg k-h's patch email bot

maintainer
#2 lore raw
In reply to Rickard Andersson
Quoted 41 lines · Rickard Andersson

From: Rickard x Andersson <[email protected]>

When about to transmit the function imx_uart_start_tx is called and in some RS485 configurations this function will call imx_uart_stop_rx. The problem is that imx_uart_stop_rx will enable loopback and when loopback is enabled transmitted data will just be looped to RX.

This patch fixes the above problem by explicitly disabling loopback in the case described above.

Fixes 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal active high")
Signed-off-by Rickard x Andersson <[email protected]>
---
 drivers/tty/serial/imx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/tty/serial/imx.c +8 -1
index 4aa72d5aeafb..899e331bdfc8 100644
@@ -683,8 +683,15 @@ static void imx_uart_start_tx(struct uart_port *port)
 			imx_uart_writel(sport, ucr2, UCR2);
 
 			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX) &&
-			    !port->rs485_rx_during_tx_gpio)
+			    !port->rs485_rx_during_tx_gpio) {
 				imx_uart_stop_rx(port);
+				/*
+				 * The function imx_uart_stop_rx right above
+				 * will enable loopback, but since we are just
+				 * about to transmit then disable loopback.
+				 */
+				imx_uart_disable_loopback_rs485(sport);
+			}
 
 			sport->tx_state = WAIT_AFTER_RTS;
Signature · 1 line
2.30.2

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree.

You are receiving this message because of the following common error(s) as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an older released kernel, yet you do not have a cc: stable line in the signed-off-by area at all, which means that the patch will not be applied to any older kernel releases. To properly fix this, please follow the documented rules in the Documentation/process/stable-kernel-rules.rst file for how to resolve this.

If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers.

thanks,

greg k-h's patch email bot

#3 lore raw
In reply to Rickard Andersson
Quoted 27 lines · Rickard Andersson

From: Rickard x Andersson <[email protected]>

When about to transmit the function imx_uart_start_tx is called and in some RS485 configurations this function will call imx_uart_stop_rx. The problem is that imx_uart_stop_rx will enable loopback and when loopback is enabled transmitted data will just be looped to RX.

This patch fixes the above problem by explicitly disabling loopback in the case described above.

Fixes 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal active high")
Signed-off-by Rickard x Andersson <[email protected]>
---
  drivers/tty/serial/imx.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)
drivers/tty/serial/imx.c +1 -1
index 4aa72d5aeafb..899e331bdfc8 100644
@@ -683,8 +683,15 @@ static void imx_uart_start_tx(struct uart_port *port)
  			imx_uart_writel(sport, ucr2, UCR2);
 
  			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX) &&
-			    !port->rs485_rx_during_tx_gpio)
+			    !port->rs485_rx_during_tx_gpio) {
  				imx_uart_stop_rx(port);

drivers/tty/serial/imx.c static void imx_uart_start_tx(struct uart_port *port) · line 697

Quoted 10 lines
drivers/tty/serial/imx.c +7 -0
+				/*
+				 * The function imx_uart_stop_rx right above
+				 * will enable loopback, but since we are just
+				 * about to transmit then disable loopback.
+				 */
+				imx_uart_disable_loopback_rs485(sport);
+			}
 
  			sport->tx_state = WAIT_AFTER_RTS;

+CC Christoph, I suspect he will comment on this soon-ish, we had an off-list discussion about this yesterday.

#4 lore raw

RFC PATCH 1/1 serial: imx: Fix RS485 behaviour on disabled RX_DURING_TX

In reply to Rickard Andersson

I have made a patch that does not enable the loopback when the RX_DURING_TX flag is disabled. Would you be so kind to test my patch to see if it also solves your problem.

Regards
Christoph

Commit 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal active high") activated the loopback mode for RS485 controlled by UART CTS_B when the function imx_uart_stop_rx() is called. But through that changes the RS485 flag RX_DURING_TX isn’t able to turn off the receiver anymore. If the flag RX_DURING_TX is disabled everything that was sent will be received due to the active loopback. To turn off the receiver in this case the function imx_uart_stop_rx() is extended by a wrapper function imx_uart_stop_rx_with_loopback_rs485_ctrl() for the use of RS485 with disabled flag RX_DURING_TX, where the receiver is turn off. For all other cases there aren’t a functional change.

When the receiver is turned off the UART CTS_B signal is always high. To ensure that the RS485 bus won’t blocked by a disabled receiver caused by a high UART CTS_B signal the function imx_uart_shutdown() is extended. So in RS485 loopback mode the receiver is always on after shutdown the device.

Fixes 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal active high")
Signed-off-by Christoph Niedermaier <[email protected]>

---
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jiri Slaby <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Pengutronix Kernel Team <[email protected]>
Cc: NXP Linux Team <[email protected]>
Cc: Sergey Organov <[email protected]>
Cc: "Uwe Kleine-König" <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: "Ilpo Järvinen" <[email protected]>
Cc: Tom Rix <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Lukas Wunner <[email protected]>
To: [email protected]

---
 drivers/tty/serial/imx.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
drivers/tty/serial/imx.c +13 -3
index 4aa72d5aeafb..222e2c929bd7 100644
@@ -463,7 +463,8 @@ static void imx_uart_stop_tx(struct uart_port *port)
 }
 
 /* called with port.lock taken and irqs off */
-static void imx_uart_stop_rx(struct uart_port *port)
+static void imx_uart_stop_rx_with_loopback_rs485_ctrl(struct uart_port *port,
+						  bool loopback_rs485_enable)
 {
 	struct imx_port *sport = (struct imx_port *)port;
 	u32 ucr1, ucr2, ucr4, uts;

@@ -483,7 +484,7 @@ static void imx_uart_stop_rx(struct uart_port *port)
 	imx_uart_writel(sport, ucr4, UCR4);
 
 	/* See SER_RS485_ENABLED/UTS_LOOP comment in imx_uart_probe() */
-	if (port->rs485.flags & SER_RS485_ENABLED &&
+	if (port->rs485.flags & SER_RS485_ENABLED && loopback_rs485_enable &&
 	    port->rs485.flags & SER_RS485_RTS_ON_SEND &&
 	    sport->have_rtscts && !sport->have_rtsgpio) {
 		uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));

@@ -498,6 +499,12 @@ static void imx_uart_stop_rx(struct uart_port *port)
 }
 
 /* called with port.lock taken and irqs off */
+static void imx_uart_stop_rx(struct uart_port *port)
+{
+	imx_uart_stop_rx_with_loopback_rs485_ctrl(port, 1);
+}
+
+/* called with port.lock taken and irqs off */
 static void imx_uart_enable_ms(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;

@@ -684,7 +691,7 @@ static void imx_uart_start_tx(struct uart_port *port)
 
 			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX) &&
 			    !port->rs485_rx_during_tx_gpio)
-				imx_uart_stop_rx(port);
+				imx_uart_stop_rx_with_loopback_rs485_ctrl(port, 0);
 
 			sport->tx_state = WAIT_AFTER_RTS;
 

@@ -1596,6 +1603,9 @@ static void imx_uart_shutdown(struct uart_port *port)
 		uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
 		uts |= UTS_LOOP;
 		imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
+		ucr2 = imx_uart_readl(sport, UCR2);
+		ucr2 |= UCR2_RXEN;
+		imx_uart_writel(sport, ucr2, UCR2);
 		ucr1 |= UCR1_UARTEN;
 	} else {
 		ucr1 &= ~UCR1_UARTEN;
Signature · 1 line
2.11.0
#5 lore raw

RE: [RFC][PATCH] serial: imx: Fix RS485 behaviour on disabled RX_DURING_TX

Quoted 13 lines

From: Christoph Niedermaier <[email protected]>
Sent: Tuesday, February 20, 2024 10:56 AM
To: [email protected]
Cc: Christoph Niedermaier <[email protected]>; Greg Kroah-Hartman <[email protected]>; Jiri Slaby <[email protected]>; Shawn Guo <[email protected]>; Marek Vasut <[email protected]>; Fabio Estevam <[email protected]>; Sascha Hauer <[email protected]>; Pengutronix Kernel Team <[email protected]>; NXP Linux Team <[email protected]>; Sergey Organov <[email protected]>; Uwe Kleine-König <[email protected]>; Rob Herring <[email protected]>; Ilpo Järvinen <[email protected]>; Tom Rix <[email protected]>; Thomas Gleixner <[email protected]>; Lukas Wunner <[email protected]>
Subject: [RFC][PATCH] serial: imx: Fix RS485 behaviour on disabled RX_DURING_TX

Quoted 6 lines

I have made a patch that does not enable the loopback when the RX_DURING_TX flag is disabled. Would you be so kind to test my patch to see if it also solves your problem.

Regards
Christoph

+CC Rickard X Andersson, because missed to add him.

Quoted 98 lines

Commit 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal active high") activated the loopback mode for RS485 controlled by UART CTS_B when the function imx_uart_stop_rx() is called. But through that changes the RS485 flag RX_DURING_TX isn’t able to turn off the receiver anymore. If the flag RX_DURING_TX is disabled everything that was sent will be received due to the active loopback. To turn off the receiver in this case the function imx_uart_stop_rx() is extended by a wrapper function imx_uart_stop_rx_with_loopback_rs485_ctrl() for the use of RS485 with disabled flag RX_DURING_TX, where the receiver is turn off. For all other cases there aren’t a functional change.

When the receiver is turned off the UART CTS_B signal is always high. To ensure that the RS485 bus won’t blocked by a disabled receiver caused by a high UART CTS_B signal the function imx_uart_shutdown() is extended. So in RS485 loopback mode the receiver is always on after shutdown the device.

Fixes 79d0224f6bf2 ("tty: serial: imx: Handle RS485 DE signal active high")
Signed-off-by Christoph Niedermaier <[email protected]>

---
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jiri Slaby <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Pengutronix Kernel Team <[email protected]>
Cc: NXP Linux Team <[email protected]>
Cc: Sergey Organov <[email protected]>
Cc: "Uwe Kleine-König" <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: "Ilpo Järvinen" <[email protected]>
Cc: Tom Rix <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Lukas Wunner <[email protected]>
To: [email protected]

---
 drivers/tty/serial/imx.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
drivers/tty/serial/imx.c +13 -4
index 4aa72d5aeafb..222e2c929bd7 100644
@@ -463,7 +463,8 @@ static void imx_uart_stop_tx(struct uart_port *port)
 }
 
 /* called with port.lock taken and irqs off */
-static void imx_uart_stop_rx(struct uart_port *port)
+static void imx_uart_stop_rx_with_loopback_rs485_ctrl(struct uart_port *port,
+						  bool loopback_rs485_enable)
 {
 	struct imx_port *sport = (struct imx_port *)port;
 	u32 ucr1, ucr2, ucr4, uts;

@@ -483,7 +484,7 @@ static void imx_uart_stop_rx(struct uart_port *port)
 	imx_uart_writel(sport, ucr4, UCR4);
 
 	/* See SER_RS485_ENABLED/UTS_LOOP comment in imx_uart_probe() */
-	if (port->rs485.flags & SER_RS485_ENABLED &&
+	if (port->rs485.flags & SER_RS485_ENABLED && loopback_rs485_enable &&
 	    port->rs485.flags & SER_RS485_RTS_ON_SEND &&
 	    sport->have_rtscts && !sport->have_rtsgpio) {
 		uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));

@@ -498,6 +499,12 @@ static void imx_uart_stop_rx(struct uart_port *port)
 }
 
 /* called with port.lock taken and irqs off */
+static void imx_uart_stop_rx(struct uart_port *port)
+{
+	imx_uart_stop_rx_with_loopback_rs485_ctrl(port, 1);
+}
+
+/* called with port.lock taken and irqs off */
 static void imx_uart_enable_ms(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;

@@ -684,7 +691,7 @@ static void imx_uart_start_tx(struct uart_port *port)
 
 			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX) &&
 			    !port->rs485_rx_during_tx_gpio)
-				imx_uart_stop_rx(port);
+				imx_uart_stop_rx_with_loopback_rs485_ctrl(port, 0);
 
 			sport->tx_state = WAIT_AFTER_RTS;
 

@@ -1596,6 +1603,9 @@ static void imx_uart_shutdown(struct uart_port *port)
 		uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
 		uts |= UTS_LOOP;
 		imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
+		ucr2 = imx_uart_readl(sport, UCR2);
+		ucr2 |= UCR2_RXEN;
+		imx_uart_writel(sport, ucr2, UCR2);
 		ucr1 |= UCR1_UARTEN;
 	} else {
 		ucr1 &= ~UCR1_UARTEN;
--

2.11.0

#6 lore raw

RE: [RFC][PATCH] serial: imx: Fix RS485 behaviour on disabled RX_DURING_TX

Hello,

I also noticed last year that RS485 was problematic on i.MX hardware. We use a i.MX7 SoC located on a SoM by manufacturer Toradex. The SoM itself is plugged into a proprietary board that features a Renesas ISL8xxx RS485 transceiver. We talk ModBus RTU over the RS485 link. The SoC is the ModBus master, the slave is a insulation monitoring device.

Since we source the i.MX through Toradex, I opened a thread on their community forum last year:
https://community.toradex.com/t/fixing-native-rs485-de-toggling-on-uart6/20573

I also realized very quickly that the newly introduced loopback logic was the culprit. My solution currently consists of a new DT property that effectively disables the toggling of the loopback mode:
https://gitlab.freedesktop.org/tobiasjakobi.compleo/linux-toradex/-/commit/98397c9436f48a149e2da0b2969a2d1d2d88c611

Anyway, I have tested Christoph's patch and can confirm that the patch (also) fixes the problems we are seeing. Test was done by applying the patch to 6.1.80. I haven't tested Rickard's patch so far.

Would be nice if this could later be backported to the 6.1.x kernel (this is the kernel we currently use on our hardware).

With best wishes,
Tobias

#7 lore raw

RE: [RFC][PATCH] serial: imx: Fix RS485 behaviour on disabled RX_DURING_TX

From: Tobias Jakobi (Compleo) <[email protected]>
Sent: Thursday, March 7, 2024 12:07 PM

Quoted 19 lines

Hello,

I also noticed last year that RS485 was problematic on i.MX hardware. We use a i.MX7 SoC located on a SoM by manufacturer Toradex. The SoM itself is plugged into a proprietary board that features a Renesas ISL8xxx RS485 transceiver. We talk ModBus RTU over the RS485 link. The SoC is the ModBus master, the slave is a insulation monitoring device.

Since we source the i.MX through Toradex, I opened a thread on their community forum last year:
https://community.toradex.com/t/fixing-native-rs485-de-toggling-on-uart6/20573

I also realized very quickly that the newly introduced loopback logic was the culprit. My solution currently consists of a new DT property that effectively disables the toggling of the loopback mode:
https://gitlab.freedesktop.org/tobiasjakobi.compleo/linux-toradex/-
/commit/98397c9436f48a149e2da0b2969a2d1d2d88c611

Quoted 6 lines

Anyway, I have tested Christoph's patch and can confirm that the patch (also) fixes the problems we are seeing. Test was done by applying the patch to 6.1.80. I haven't tested Rickard's patch so far.

Would be nice if this could later be backported to the 6.1.x kernel (this is the kernel we currently use on our hardware).

Rickard's patch in version 3 solved the same problem, so it should also fix the problem on your side. It has already been accepted and is already available in the next Kernel. It has a Fixes tag on it, so it should normally be backported to the 6.1.x Kernel soon.

Regards
Christo1ph

#8 lore raw

Re: [RFC][PATCH] serial: imx: Fix RS485 behaviour on disabled RX_DURING_TX

Quoted 4 lines · Christoph Niedermaier

Rickard's patch in version 3 solved the same problem, so it should also fix the problem on your side. It has already been accepted and is already available in the next Kernel. It has a Fixes tag on it, so it should normally be backported to the 6.1.x Kernel soon.

Sounds good! Going to keep an eye open when it lands in the stable tree.

With best wishes,
Tobias

Quoted 3 lines

Regards
Christo1ph