[ 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@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.
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
