diff mbox series

serial: imx: fix TX stop function not setting state to OFF in RS232-mode

Message ID ac5aa181-eca3-040a-302a-4992022d39f1@digi.com
State New
Headers show
Series serial: imx: fix TX stop function not setting state to OFF in RS232-mode | expand

Commit Message

Robert Hodaszi April 2, 2021, 11:05 a.m. UTC 
From: Robert Hodaszi <robert.hodaszi@digi.com>

If the mode had been changed to RS-485 after at least one character had
been sent in RS-232 mode, the RS-485 transmission was not working.

Commit cb1a609236096c278ecbfb7be678a693a70283f1 ("serial: imx: implement
rts delaying for rs485") added a TX state variable to keep track, when
it needs to enable / disable the RS-485 transmitter.

In RS-232 mode, the start TX function just sets the state to SEND, and
the stop function supposed to set it to OFF.

In RS-485 mode, the start TX function expects the state to be either
OFF, WAIT_AFTER_SEND, or WAIT_AFTER RTS. It cannot do anything if the
state is set to SEND, expects a stop first.

But stop TX function in RS-232 mode usually didn't set the state to OFF,
as it first checked if the shifter is empty, and if not, it just
returned, waiting for a TransmitComplete interrupt, which is only
enabled in RS-485 mode. So the stop function was never called again.

That check, and the subsequent code part is not needed for RS-232, it
just have to set the TX state to OFF.

Signed-off-by: Robert Hodaszi <robert.hodaszi@digi.com>
---
  drivers/tty/serial/imx.c | 46 +++++++++++++++++++++-------------------
  1 file changed, 24 insertions(+), 22 deletions(-)

Comments

Greg Kroah-Hartman April 10, 2021, 8:33 a.m. UTC | #1 
On Fri, Apr 02, 2021 at 01:05:48PM +0200, Robert Hodaszi wrote:
> From: Robert Hodaszi <robert.hodaszi@digi.com>

> 

> If the mode had been changed to RS-485 after at least one character had

> been sent in RS-232 mode, the RS-485 transmission was not working.

> 

> Commit cb1a609236096c278ecbfb7be678a693a70283f1 ("serial: imx: implement

> rts delaying for rs485") added a TX state variable to keep track, when

> it needs to enable / disable the RS-485 transmitter.

> 

> In RS-232 mode, the start TX function just sets the state to SEND, and

> the stop function supposed to set it to OFF.

> 

> In RS-485 mode, the start TX function expects the state to be either

> OFF, WAIT_AFTER_SEND, or WAIT_AFTER RTS. It cannot do anything if the

> state is set to SEND, expects a stop first.

> 

> But stop TX function in RS-232 mode usually didn't set the state to OFF,

> as it first checked if the shifter is empty, and if not, it just

> returned, waiting for a TransmitComplete interrupt, which is only

> enabled in RS-485 mode. So the stop function was never called again.

> 

> That check, and the subsequent code part is not needed for RS-232, it

> just have to set the TX state to OFF.

> 

> Signed-off-by: Robert Hodaszi <robert.hodaszi@digi.com>

> ---

>  drivers/tty/serial/imx.c | 46 +++++++++++++++++++++-------------------

>  1 file changed, 24 insertions(+), 22 deletions(-)

> 

> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c

> index 8257597d034d..511badce3edd 100644

> --- a/drivers/tty/serial/imx.c

> +++ b/drivers/tty/serial/imx.c

> @@ -443,6 +443,12 @@ static void imx_uart_stop_tx(struct uart_port *port)

>      ucr1 = imx_uart_readl(sport, UCR1);

>      imx_uart_writel(sport, ucr1 & ~UCR1_TRDYEN, UCR1);

> 

> +    if (!(port->rs485.flags & SER_RS485_ENABLED)) {

> +        /* Set the TX state in non-RS485 mode, nothing else to do */

> +        sport->tx_state = OFF;

> +        return;

> +    }

> +

>      usr2 = imx_uart_readl(sport, USR2);

>      if (!(usr2 & USR2_TXDC)) {

>          /* The shifter is still busy, so retry once TC triggers */

> @@ -453,33 +459,29 @@ static void imx_uart_stop_tx(struct uart_port *port)

>      ucr4 &= ~UCR4_TCEN;

>      imx_uart_writel(sport, ucr4, UCR4);

> 

> -    /* in rs485 mode disable transmitter */

> -    if (port->rs485.flags & SER_RS485_ENABLED) {

> -        if (sport->tx_state == SEND) {

> -            sport->tx_state = WAIT_AFTER_SEND;

> -            start_hrtimer_ms(&sport->trigger_stop_tx,

> -                     port->rs485.delay_rts_after_send);

> -            return;

> -        }

> +    if (sport->tx_state == SEND) {

> +        sport->tx_state = WAIT_AFTER_SEND;

> +        start_hrtimer_ms(&sport->trigger_stop_tx,

> +                    port->rs485.delay_rts_after_send);

> +        return;

> +    }

> 

> -        if (sport->tx_state == WAIT_AFTER_RTS ||

> -            sport->tx_state == WAIT_AFTER_SEND) {

> -            u32 ucr2;

> +    if (sport->tx_state == WAIT_AFTER_RTS ||

> +        sport->tx_state == WAIT_AFTER_SEND) {

> +        /* in rs485 mode disable transmitter */

> +        u32 ucr2;

> 

> - hrtimer_try_to_cancel(&sport->trigger_start_tx);

> +        hrtimer_try_to_cancel(&sport->trigger_start_tx);

> 

> -            ucr2 = imx_uart_readl(sport, UCR2);

> -            if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)

> -                imx_uart_rts_active(sport, &ucr2);

> -            else

> -                imx_uart_rts_inactive(sport, &ucr2);

> -            imx_uart_writel(sport, ucr2, UCR2);

> +        ucr2 = imx_uart_readl(sport, UCR2);

> +        if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)

> +            imx_uart_rts_active(sport, &ucr2);

> +        else

> +            imx_uart_rts_inactive(sport, &ucr2);

> +        imx_uart_writel(sport, ucr2, UCR2);

> 

> -            imx_uart_start_rx(port);

> +        imx_uart_start_rx(port);

> 

> -            sport->tx_state = OFF;

> -        }

> -    } else {

>          sport->tx_state = OFF;

>      }

>  }

> -- 

> 2.27.0


Your patch is corrupted, all the tabs are turned to spaces and it can
not be applied :(

Please fix up and resend.

thanks,

greg k-h
Robert Hodaszi April 20, 2021, 7:07 p.m. UTC | #2 
On 2021. 04. 10. 10:33, Greg Kroah-Hartman wrote:
> [EXTERNAL EMAIL]

>

> On Fri, Apr 02, 2021 at 01:05:48PM +0200, Robert Hodaszi wrote:

>> From: Robert Hodaszi <robert.hodaszi@digi.com>

>>

>> If the mode had been changed to RS-485 after at least one character had

>> been sent in RS-232 mode, the RS-485 transmission was not working.

>>

>> Commit cb1a609236096c278ecbfb7be678a693a70283f1 ("serial: imx: implement

>> rts delaying for rs485") added a TX state variable to keep track, when

>> it needs to enable / disable the RS-485 transmitter.

>>

>> In RS-232 mode, the start TX function just sets the state to SEND, and

>> the stop function supposed to set it to OFF.

>>

>> In RS-485 mode, the start TX function expects the state to be either

>> OFF, WAIT_AFTER_SEND, or WAIT_AFTER RTS. It cannot do anything if the

>> state is set to SEND, expects a stop first.

>>

>> But stop TX function in RS-232 mode usually didn't set the state to OFF,

>> as it first checked if the shifter is empty, and if not, it just

>> returned, waiting for a TransmitComplete interrupt, which is only

>> enabled in RS-485 mode. So the stop function was never called again.

>>

>> That check, and the subsequent code part is not needed for RS-232, it

>> just have to set the TX state to OFF.

>>

>> Signed-off-by: Robert Hodaszi <robert.hodaszi@digi.com>

>> ---

>>  drivers/tty/serial/imx.c | 46 +++++++++++++++++++++-------------------

>>  1 file changed, 24 insertions(+), 22 deletions(-)

>>

>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c

>> index 8257597d034d..511badce3edd 100644

>> --- a/drivers/tty/serial/imx.c

>> +++ b/drivers/tty/serial/imx.c

>> @@ -443,6 +443,12 @@ static void imx_uart_stop_tx(struct uart_port *port)

>>      ucr1 = imx_uart_readl(sport, UCR1);

>>      imx_uart_writel(sport, ucr1 & ~UCR1_TRDYEN, UCR1);

>>

>> +    if (!(port->rs485.flags & SER_RS485_ENABLED)) {

>> +        /* Set the TX state in non-RS485 mode, nothing else to do */

>> +        sport->tx_state = OFF;

>> +        return;

>> +    }

>> +

>>      usr2 = imx_uart_readl(sport, USR2);

>>      if (!(usr2 & USR2_TXDC)) {

>>          /* The shifter is still busy, so retry once TC triggers */

>> @@ -453,33 +459,29 @@ static void imx_uart_stop_tx(struct uart_port *port)

>>      ucr4 &= ~UCR4_TCEN;

>>      imx_uart_writel(sport, ucr4, UCR4);

>>

>> -    /* in rs485 mode disable transmitter */

>> -    if (port->rs485.flags & SER_RS485_ENABLED) {

>> -        if (sport->tx_state == SEND) {

>> -            sport->tx_state = WAIT_AFTER_SEND;

>> -            start_hrtimer_ms(&sport->trigger_stop_tx,

>> -                     port->rs485.delay_rts_after_send);

>> -            return;

>> -        }

>> +    if (sport->tx_state == SEND) {

>> +        sport->tx_state = WAIT_AFTER_SEND;

>> +        start_hrtimer_ms(&sport->trigger_stop_tx,

>> +                    port->rs485.delay_rts_after_send);

>> +        return;

>> +    }

>>

>> -        if (sport->tx_state == WAIT_AFTER_RTS ||

>> -            sport->tx_state == WAIT_AFTER_SEND) {

>> -            u32 ucr2;

>> +    if (sport->tx_state == WAIT_AFTER_RTS ||

>> +        sport->tx_state == WAIT_AFTER_SEND) {

>> +        /* in rs485 mode disable transmitter */

>> +        u32 ucr2;

>>

>> - hrtimer_try_to_cancel(&sport->trigger_start_tx);

>> +        hrtimer_try_to_cancel(&sport->trigger_start_tx);

>>

>> -            ucr2 = imx_uart_readl(sport, UCR2);

>> -            if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)

>> -                imx_uart_rts_active(sport, &ucr2);

>> -            else

>> -                imx_uart_rts_inactive(sport, &ucr2);

>> -            imx_uart_writel(sport, ucr2, UCR2);

>> +        ucr2 = imx_uart_readl(sport, UCR2);

>> +        if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)

>> +            imx_uart_rts_active(sport, &ucr2);

>> +        else

>> +            imx_uart_rts_inactive(sport, &ucr2);

>> +        imx_uart_writel(sport, ucr2, UCR2);

>>

>> -            imx_uart_start_rx(port);

>> +        imx_uart_start_rx(port);

>>

>> -            sport->tx_state = OFF;

>> -        }

>> -    } else {

>>          sport->tx_state = OFF;

>>      }

>>  }

>> --

>> 2.27.0

> Your patch is corrupted, all the tabs are turned to spaces and it can

> not be applied :(

>

> Please fix up and resend.

>

> thanks,

>

> greg k-h


Sorry, for that (and for the slow reply)! My latest Thunderbird setup
seems not playing very. Went back to my older setup, and sent out a v2.
Hopefully, that will be OK.

Thanks,
Robert Hodaszi
diff mbox series

Patch

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8257597d034d..511badce3edd 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -443,6 +443,12 @@  static void imx_uart_stop_tx(struct uart_port *port)
      ucr1 = imx_uart_readl(sport, UCR1);
      imx_uart_writel(sport, ucr1 & ~UCR1_TRDYEN, UCR1);

+    if (!(port->rs485.flags & SER_RS485_ENABLED)) {
+        /* Set the TX state in non-RS485 mode, nothing else to do */
+        sport->tx_state = OFF;
+        return;
+    }
+
      usr2 = imx_uart_readl(sport, USR2);
      if (!(usr2 & USR2_TXDC)) {
          /* The shifter is still busy, so retry once TC triggers */
@@ -453,33 +459,29 @@  static void imx_uart_stop_tx(struct uart_port *port)
      ucr4 &= ~UCR4_TCEN;
      imx_uart_writel(sport, ucr4, UCR4);

-    /* in rs485 mode disable transmitter */
-    if (port->rs485.flags & SER_RS485_ENABLED) {
-        if (sport->tx_state == SEND) {
-            sport->tx_state = WAIT_AFTER_SEND;
-            start_hrtimer_ms(&sport->trigger_stop_tx,
-                     port->rs485.delay_rts_after_send);
-            return;
-        }
+    if (sport->tx_state == SEND) {
+        sport->tx_state = WAIT_AFTER_SEND;
+        start_hrtimer_ms(&sport->trigger_stop_tx,
+                    port->rs485.delay_rts_after_send);
+        return;
+    }

-        if (sport->tx_state == WAIT_AFTER_RTS ||
-            sport->tx_state == WAIT_AFTER_SEND) {
-            u32 ucr2;
+    if (sport->tx_state == WAIT_AFTER_RTS ||
+        sport->tx_state == WAIT_AFTER_SEND) {
+        /* in rs485 mode disable transmitter */
+        u32 ucr2;

- hrtimer_try_to_cancel(&sport->trigger_start_tx);
+        hrtimer_try_to_cancel(&sport->trigger_start_tx);

-            ucr2 = imx_uart_readl(sport, UCR2);
-            if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
-                imx_uart_rts_active(sport, &ucr2);
-            else
-                imx_uart_rts_inactive(sport, &ucr2);
-            imx_uart_writel(sport, ucr2, UCR2);
+        ucr2 = imx_uart_readl(sport, UCR2);
+        if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+            imx_uart_rts_active(sport, &ucr2);
+        else
+            imx_uart_rts_inactive(sport, &ucr2);
+        imx_uart_writel(sport, ucr2, UCR2);

-            imx_uart_start_rx(port);
+        imx_uart_start_rx(port);

-            sport->tx_state = OFF;
-        }
-    } else {
          sport->tx_state = OFF;
      }
  }