tty: serial: imx: Fix broken RS485

PATCH Superseded Rickard Andersson 3 participants 6 messages Feb 16, 2024 – Feb 20, 2024

Subsystems (2)

Arm i.MX and Layerscape TTY layer and serial drivers

lore

Share & tools

lore

Rickard Andersson

author

2024-02-16 12:46 UTC #0 lore raw

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

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

Marek Vasut

2024-02-16 15:17 UTC #1 lore raw

Quoted 11 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.
Signed-off-by Rickard x Andersson <[email protected]>

Fixes tag is missing.

Quoted 12 lines

---
  drivers/tty/serial/imx.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

drivers/tty/serial/imx.c +0 -0

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

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

Quoted 10 lines

drivers/tty/serial/imx.c +8 -1

-			    !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);
+			}

Maybe it would be better to update the stop_rx and add parameter, whether it should/shouldn't enable the loopback ?

Rickard X Andersson

2024-02-20 06:07 UTC #2 lore raw

Quoted 9 lines · Marek Vasut

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 6 lines

Quoted 4 lines
This patch fixes the above problem by explicitly disabling loopback in the case described above.
Signed-off-by Rickard x Andersson <[email protected]>
Fixes tag is missing.

Ok, I will add.

Quoted 12 lines

---
  drivers/tty/serial/imx.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

drivers/tty/serial/imx.c +0 -0

index 4aa72d5aeafb..899e331bdfc8 100644
@@ -683,8 +683,15 @@ static void imx_uart_start_tx(struct uart_port *port)
                      imx_uart_writel(sport, ucr2, UCR2);

Quoted 14 lines

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

Quoted 11 lines

drivers/tty/serial/imx.c +8 -1

                      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);
+                     }

Maybe it would be better to update the stop_rx and add parameter, whether it should/shouldn't enable the loopback ?

Since *stop_rx is part of struct uart_ops I can not add an input argument to the function.

Best regards,
Rickard

________________________________________
From: Marek Vasut <[email protected]>
Sent: Friday, February 16, 2024 4:17 PM
To: Rickard X Andersson; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Christoph Niedermaier
Cc: kernel
Subject: Re: [PATCH] tty: serial: imx: Fix broken RS485

Quoted 11 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.
Signed-off-by Rickard x Andersson <[email protected]>

Fixes tag is missing.

Quoted 12 lines

---
  drivers/tty/serial/imx.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

drivers/tty/serial/imx.c +0 -0

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

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

Quoted 10 lines

drivers/tty/serial/imx.c +8 -1

-                         !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);
+                     }

Maybe it would be better to update the stop_rx and add parameter, whether it should/shouldn't enable the loopback ?

Marek Vasut

2024-02-20 07:03 UTC #3 lore raw

Quoted 45 lines · Rickard X Andersson

Quoted 9 lines · Marek Vasut
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 6 lines
Quoted 4 lines
This patch fixes the above problem by explicitly disabling loopback in the case described above.
Signed-off-by Rickard x Andersson <[email protected]>
Fixes tag is missing.
Ok, I will add.
Quoted 12 lines
Quoted 12 lines
---
   drivers/tty/serial/imx.c | 9 ++++++++-
   1 file changed, 8 insertions(+), 1 deletion(-)
drivers/tty/serial/imx.c +0 -0
index 4aa72d5aeafb..899e331bdfc8 100644
@@ -683,8 +683,15 @@ static void imx_uart_start_tx(struct uart_port *port)
                       imx_uart_writel(sport, ucr2, UCR2);
 
Quoted 14 lines
drivers/tty/serial/imx.c static void imx_uart_start_tx(struct uart_port *port) · line 694
Quoted 11 lines
drivers/tty/serial/imx.c +8 -1
                       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);
+                     }
Maybe it would be better to update the stop_rx and add parameter, whether it should/shouldn't enable the loopback ?

Quoted 1 line

Since *stop_rx is part of struct uart_ops I can not add an input argument to the function.

You could add a wrapper function and make stop_tx call that one.

Rickard X Andersson

2024-02-20 08:39 UTC #4 lore raw

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Maybe it would be better to update the stop_rx and add parameter, whether it should/shouldn't enable the loopback ?
Since *stop_rx is part of struct uart_ops I can not add an input argument to the function.
You could add a wrapper function and make stop_tx call that one.

Ok, yes that is possible. Do you want me to do that change?

Best regards,
Rickard

________________________________________
From: Marek Vasut <[email protected]>
Sent: Tuesday, February 20, 2024 7:52 AM
To: Rickard X Andersson; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Christoph Niedermaier
Cc: kernel
Subject: Re: [PATCH] tty: serial: imx: Fix broken RS485

Quoted 45 lines · Rickard X Andersson

Quoted 9 lines · Marek Vasut
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 6 lines
Quoted 4 lines
This patch fixes the above problem by explicitly disabling loopback in the case described above.
Signed-off-by Rickard x Andersson <[email protected]>
Fixes tag is missing.
Ok, I will add.
Quoted 12 lines
Quoted 12 lines
---
   drivers/tty/serial/imx.c | 9 ++++++++-
   1 file changed, 8 insertions(+), 1 deletion(-)
drivers/tty/serial/imx.c +0 -0
index 4aa72d5aeafb..899e331bdfc8 100644
@@ -683,8 +683,15 @@ static void imx_uart_start_tx(struct uart_port *port)
                       imx_uart_writel(sport, ucr2, UCR2);
 
Quoted 14 lines
drivers/tty/serial/imx.c static void imx_uart_start_tx(struct uart_port *port) · line 694
Quoted 11 lines
drivers/tty/serial/imx.c +8 -1
                       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);
+                     }
Maybe it would be better to update the stop_rx and add parameter, whether it should/shouldn't enable the loopback ?

Quoted 1 line

Since *stop_rx is part of struct uart_ops I can not add an input argument to the function.

You could add a wrapper function and make stop_tx call that one.

Marek Vasut

2024-02-20 09:35 UTC #5 lore raw

Quoted 9 lines · Rickard X Andersson

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Maybe it would be better to update the stop_rx and add parameter, whether it should/shouldn't enable the loopback ?
Since *stop_rx is part of struct uart_ops I can not add an input argument to the function.
You could add a wrapper function and make stop_tx call that one.
Ok, yes that is possible. Do you want me to do that change?

Better wait a few days before sending V3, so that others can chime in. And yes, I think it would be better.