RE: [PATCH] serial: imx: also enable Transmit Complete interrupt in rs232 mode

[Sherry Sun]

> -----Original Message-----
> From: Rasmus Villemoes <rasmus.villemoes@xxxxxxxxx>
> Sent: 2023年11月20日 21:23
> To: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Jiri Slaby
> <jirislaby@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Sascha Hauer
> <s.hauer@xxxxxxxxxxxxxx>; Pengutronix Kernel Team
> <kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; dl-linux-
> imx <linux-imx@xxxxxxx>
> Cc: Rasmus Villemoes <rasmus.villemoes@xxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; linux-serial@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Subject: [PATCH] serial: imx: also enable Transmit Complete interrupt in
> rs232 mode
> 
> Currently, if one switches to rs232 mode, writes something to the device, and
> then switches to rs485 mode, the imx_port's ->tx_state is left as SEND. This
> then prevents a subsequent write in rs485 mode from properly asserting the
> rts pin (i.e. enabling the transceiver), because imx_uart_start_rx() does not
> enter the "if (sport->tx_state == OFF)" branch. Hence nothing is actually
> transmitted.
> 
> The problem is that in rs232 mode, ->tx_state never gets set to OFF, due to
> 
> 	usr2 = imx_uart_readl(sport, USR2);
> 	if (!(usr2 & USR2_TXDC)) {
> 		/* The shifter is still busy, so retry once TC triggers */
> 		return;
> 	}
> 
> in imx_uart_stop_tx(), and TC never triggers because the Transmit Complete
> interrupt is not enabled for rs232.

Hi Rasmus,
I am afraid this is not right, USR2_TXDC is just a flag, it is not affected by whether UCR4_TCEN is set or not, UCR4_TCEN only determines if USR2_TXDC flag can generate a interrupt or not.
I tried on imx8mp-evk board, for rs232, sport->tx_state can get set to OFF even we don’t set UCR4_TCEN.

Best Regards
Sherry

> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@xxxxxxxxx>
> ---
> I'm not sure this is the best fix.
> 
> At first I considered doing something much more targeted, but definitely also
> more hacky: In imx_uart_rs485_config(), if switching on rs485 mode, simply
> add "sport->tx_state = OFF;".
> 
> If someone has a better suggestion, I'm all ears.
> 
>  drivers/tty/serial/imx.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> 7341d060f85c..ffee157e13cd 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -708,25 +708,19 @@ static void imx_uart_start_tx(struct uart_port
> *port)
>  		    || sport->tx_state == WAIT_AFTER_RTS) {
> 
>  			hrtimer_try_to_cancel(&sport->trigger_stop_tx);
> -
> -			/*
> -			 * Enable transmitter and shifter empty irq only if
> DMA
> -			 * is off.  In the DMA case this is done in the
> -			 * tx-callback.
> -			 */
> -			if (!sport->dma_is_enabled) {
> -				u32 ucr4 = imx_uart_readl(sport, UCR4);
> -				ucr4 |= UCR4_TCEN;
> -				imx_uart_writel(sport, ucr4, UCR4);
> -			}
> -
> -			sport->tx_state = SEND;
>  		}
> -	} else {
> -		sport->tx_state = SEND;
>  	}
> 
> +	sport->tx_state = SEND;
> +
> +	/*
> +	 * Enable transmitter and shifter empty irq only if DMA is
> +	 * off.  In the DMA case this is done in the tx-callback.
> +	 */
>  	if (!sport->dma_is_enabled) {
> +		u32 ucr4 = imx_uart_readl(sport, UCR4);
> +		ucr4 |= UCR4_TCEN;
> +		imx_uart_writel(sport, ucr4, UCR4);
>  		ucr1 = imx_uart_readl(sport, UCR1);
>  		imx_uart_writel(sport, ucr1 | UCR1_TRDYEN, UCR1);
>  	}
> --
> 2.40.1.1.g1c60b9335d