Re: [PATCH v2] serial: imx: fix TX stop function not setting state to OFF

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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

On Tue, Apr 20, 2021 at 07:02:33PM +0000, Hodaszi, Robert wrote:
> 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>
> ---
> 
> Changes in v2:
>  - Fixed incorrect tabs
> 
>  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 */

What this hunk essentially does is to skip clearing UCR4_TCEN. Doesn't
this result in an irq storm until there is new data to send or the
device is closed? At least if DMA isn't used for the port this is the
only place that disables the TC irq (apart from .shutdown).

I wonder if a better fix would be to only switch between RS485 and RS232
if the transmitter is empty.

> @@ -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;
>  	}
>  }

This hunk is only removing the if check and reindenting the body, right?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]