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

[Denis Ahrens <denis@xxxxxxx>]

> On 25. May 2020, at 10:27, Johan Hovold <johan@xxxxxxxxxx> wrote:
> 
> On Sun, May 24, 2020 at 06:31:44PM +0200, Denis Ahrens wrote:
>> From: Denis Ahrens <denis@xxxxxxx>
>> 
>> 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@xxxxxxx>
>> ---
>> 
>> 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.

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.

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)