Re: [PATCH] 16C950 UART enable Hardware Flow Control

[Johan Hovold <johan@kernel.org>]

[ Adding Pavel who disabled EFR at one point to CC. ]

On Mon, May 25, 2020 at 07:49:54PM +0200, Denis Ahrens wrote:
> 
> 
> > On 25. May 2020, at 10:27, Johan Hovold <johan@kernel.org> wrote:
> > 
> > On Sun, May 24, 2020 at 06:31:44PM +0200, Denis Ahrens wrote:
> >> From: Denis Ahrens <denis@h3q.com>
> >> 
> >> Enable Automatic RTS/CTS flow control for the 16C950 UART in Enhanced Mode
> >> like described in the Data Sheet Revision 1.2 page 28 and 29.
> >> 
> >> Without this change normal console output works, but everything putting
> >> a little more pressure on the UART simply overruns the FIFO.
> >> 
> >> Signed-off-by: Denis Ahrens <denis@h3q.com>
> >> ---
> >> 
> >> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> >> index f77bf820b7a3..024235946f4d 100644
> >> --- a/drivers/tty/serial/8250/8250_port.c
> >> +++ b/drivers/tty/serial/8250/8250_port.c
> >> @@ -2168,7 +2168,9 @@ int serial8250_do_startup(struct uart_port *port)
> >>                serial_port_out(port, UART_LCR, 0);
> >>                serial_icr_write(up, UART_CSR, 0); /* Reset the UART */
> >>                serial_port_out(port, UART_LCR, UART_LCR_CONF_MODE_B);
> >> -               serial_port_out(port, UART_EFR, UART_EFR_ECB);
> >> +               serial_port_out(port, UART_EFR, UART_EFR_ECB |
> >> +                                               UART_EFR_RTS |
> >> +                                               UART_EFR_CTS);
> >>                serial_port_out(port, UART_LCR, 0);
> >>        }
> > 
> > This doesn't look right as you're now enabling automatic flow control
> > for everyone.
> > 
> > Try adding this to set_termios() instead when enabling flow control.
> 
> The part in set_termios() is never reached because the UART_CAP_EFR
> capability was removed ca. 10 years ago. The code fails to preserve
> the UART_EFR_ECB bit which is in the same register as UART_EFR_CTS.
> Also for some reason UART_EFR_RTS is not set.

The EFR capability was added by commit 7a56aa45982b ("serial: add
UART_CAP_EFR and UART_CAP_SLEEP flags to 16C950 UARTs definition") and
then removed half a year later by commit 0694e2aeb81 ("serial: unbreak
billionton CF card") -- you should mention that in the commit message
too.

I guess you need to determine how to enable this feature without
breaking something else.

> So lets fix the code instead of disabling a feature.
> 
> I could write a patch which adds UART_CAP_EFR back to the 16C950 and
> fixes the code in set_termios only for the 16C950. I would also add
> another line which adds RTS hardware flow control only for the 16C950.

Nah, auto-RTS should probably have been enabled from the start.

And just make sure not clear any other bits in that register when
updating the flow-control settings for example by reading it back first.

> The change would look like this:
> (I will write another mail with a real patch if I get the OK)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index f77bf820b7a3..ac7efc43b392 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -122,8 +122,7 @@ static const struct serial8250_config uart_config[] = {
>                 .fifo_size      = 128,
>                 .tx_loadsz      = 128,
>                 .fcr            = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
> -               /* UART_CAP_EFR breaks billionon CF bluetooth card. */
> -               .flags          = UART_CAP_FIFO | UART_CAP_SLEEP,
> +               .flags          = UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP,
>         },
>         [PORT_16654] = {
>                 .name           = "ST16654",
> @@ -2723,13 +2722,18 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
>  
>         if (up->capabilities & UART_CAP_EFR) {
>                 unsigned char efr = 0;
> +               if (port->type == PORT_16C950)
> +                       efr |= UART_EFR_ECB;
>                 /*
>                  * TI16C752/Startech hardware flow control.  FIXME:
>                  * - TI16C752 requires control thresholds to be set.
>                  * - UART_MCR_RTS is ineffective if auto-RTS mode is enabled.
>                  */
> -               if (termios->c_cflag & CRTSCTS)
> +               if (termios->c_cflag & CRTSCTS) {
>                         efr |= UART_EFR_CTS;
> +                       if (port->type == PORT_16C950)
> +                               efr |= UART_EFR_RTS;
> +               }
>  
>                 serial_port_out(port, UART_LCR, UART_LCR_CONF_MODE_B);
>                 if (port->flags & UPF_EXAR_EFR)
> 

Johan