[PATCH V2] tty: serial: fsl_lpuart: avoid idle preamble pending if CTS is enabled

[PATCH V2] tty: serial: fsl_lpuart: avoid idle preamble pending if CTS is enabled

[Sherry Sun]

If the remote uart device is not connected or not enabled after booting
up, the CTS line is high by default. At this time, if we enable the flow
control when opening the device(for example, using “stty -F /dev/ttyLP4
crtscts” command), there will be a pending idle preamble(first writing 0
and then writing 1 to UARTCTRL_TE will queue an idle preamble) that
cannot be sent out, resulting in the uart port fail to close(waiting for
TX empty), so the user space stty will have to wait for a long time or
forever.

This is an LPUART IP bug(idle preamble has higher priority than CTS),
here add a workaround patch to enable TX CTS after enabling UARTCTRL_TE,
so that the idle preamble does not get stuck due to CTS is deasserted.

Fixes: 380c966c093e ("tty: serial: fsl_lpuart: add 32-bit register interface support")
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
Changes in V2:
1. Move the "restore control register" comment message to the appropriate place.
2. Add Fixes tag.
---
 drivers/tty/serial/fsl_lpuart.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 5ddf110aedbe..bbcbc91482af 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2345,9 +2345,12 @@ lpuart32_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	lpuart32_write(&sport->port, bd, UARTBAUD);
 	lpuart32_serial_setbrg(sport, baud);
-	lpuart32_write(&sport->port, modem, UARTMODIR);
-	lpuart32_write(&sport->port, ctrl, UARTCTRL);
+	/* disable CTS before enabling UARTCTRL_TE to avoid pending idle preamble */
+	lpuart32_write(&sport->port, modem & ~UARTMODIR_TXCTSE, UARTMODIR);
 	/* restore control register */
+	lpuart32_write(&sport->port, ctrl, UARTCTRL);
+	/* re-enable the CTS if needed */
+	lpuart32_write(&sport->port, modem, UARTMODIR);
 
 	if ((ctrl & (UARTCTRL_PE | UARTCTRL_M)) == UARTCTRL_PE)
 		sport->is_cs7 = true;
-- 
2.37.1

Re: [PATCH V2] tty: serial: fsl_lpuart: avoid idle preamble pending if CTS is enabled

[Sverdlin, Alexander]

Hi Sherry,

thanks for re-spin!

On Tue, 2024-03-05 at 09:57 +0800, Sherry Sun wrote:
> If the remote uart device is not connected or not enabled after booting
> up, the CTS line is high by default. At this time, if we enable the flow
> control when opening the device(for example, using “stty -F /dev/ttyLP4
> crtscts” command), there will be a pending idle preamble(first writing 0
> and then writing 1 to UARTCTRL_TE will queue an idle preamble) that
> cannot be sent out, resulting in the uart port fail to close(waiting for
> TX empty), so the user space stty will have to wait for a long time or
> forever.
> 
> This is an LPUART IP bug(idle preamble has higher priority than CTS),
> here add a workaround patch to enable TX CTS after enabling UARTCTRL_TE,
> so that the idle preamble does not get stuck due to CTS is deasserted.
> 
> Fixes: 380c966c093e ("tty: serial: fsl_lpuart: add 32-bit register interface support")
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

> ---
> Changes in V2:
> 1. Move the "restore control register" comment message to the appropriate place.
> 2. Add Fixes tag.
> ---
>  drivers/tty/serial/fsl_lpuart.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index 5ddf110aedbe..bbcbc91482af 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -2345,9 +2345,12 @@ lpuart32_set_termios(struct uart_port *port, struct ktermios *termios,
>  
>         lpuart32_write(&sport->port, bd, UARTBAUD);
>         lpuart32_serial_setbrg(sport, baud);
> -       lpuart32_write(&sport->port, modem, UARTMODIR);
> -       lpuart32_write(&sport->port, ctrl, UARTCTRL);
> +       /* disable CTS before enabling UARTCTRL_TE to avoid pending idle preamble */
> +       lpuart32_write(&sport->port, modem & ~UARTMODIR_TXCTSE, UARTMODIR);
>         /* restore control register */
> +       lpuart32_write(&sport->port, ctrl, UARTCTRL);
> +       /* re-enable the CTS if needed */
> +       lpuart32_write(&sport->port, modem, UARTMODIR);
>  
>         if ((ctrl & (UARTCTRL_PE | UARTCTRL_M)) == UARTCTRL_PE)
>                 sport->is_cs7 = true;

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH V2] tty: serial: fsl_lpuart: avoid idle preamble pending if CTS is enabled

[Greg KH]

On Tue, Mar 05, 2024 at 09:57:06AM +0800, Sherry Sun wrote:
> If the remote uart device is not connected or not enabled after booting
> up, the CTS line is high by default. At this time, if we enable the flow
> control when opening the device(for example, using “stty -F /dev/ttyLP4
> crtscts” command), there will be a pending idle preamble(first writing 0
> and then writing 1 to UARTCTRL_TE will queue an idle preamble) that
> cannot be sent out, resulting in the uart port fail to close(waiting for
> TX empty), so the user space stty will have to wait for a long time or
> forever.
> 
> This is an LPUART IP bug(idle preamble has higher priority than CTS),
> here add a workaround patch to enable TX CTS after enabling UARTCTRL_TE,
> so that the idle preamble does not get stuck due to CTS is deasserted.
> 
> Fixes: 380c966c093e ("tty: serial: fsl_lpuart: add 32-bit register interface support")
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> ---
> Changes in V2:
> 1. Move the "restore control register" comment message to the appropriate place.
> 2. Add Fixes tag.
> ---
>  drivers/tty/serial/fsl_lpuart.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index 5ddf110aedbe..bbcbc91482af 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -2345,9 +2345,12 @@ lpuart32_set_termios(struct uart_port *port, struct ktermios *termios,
>  
>  	lpuart32_write(&sport->port, bd, UARTBAUD);
>  	lpuart32_serial_setbrg(sport, baud);
> -	lpuart32_write(&sport->port, modem, UARTMODIR);
> -	lpuart32_write(&sport->port, ctrl, UARTCTRL);
> +	/* disable CTS before enabling UARTCTRL_TE to avoid pending idle preamble */
> +	lpuart32_write(&sport->port, modem & ~UARTMODIR_TXCTSE, UARTMODIR);
>  	/* restore control register */
> +	lpuart32_write(&sport->port, ctrl, UARTCTRL);
> +	/* re-enable the CTS if needed */
> +	lpuart32_write(&sport->port, modem, UARTMODIR);
>  
>  	if ((ctrl & (UARTCTRL_PE | UARTCTRL_M)) == UARTCTRL_PE)
>  		sport->is_cs7 = true;
> -- 
> 2.37.1
> 
> 

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