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

[Denis Ahrens <denis@h3q.com>]

> On 27. May 2020, at 09:55, Johan Hovold <johan@kernel.org> wrote:
> 
> [ 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.
>>> 
>>> 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.

UARTS with UART_CAP_EFR activates auto-RTS with UART_EFR_RTS and that is
not used anywhere. Setting this bit fixes my problem.

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

I can read the EFR register before using it (it was simply cleared before).
But then I change things for the XR17V35x too. But I don’t want to touch
that one because this UART has UART_CAP_AFE set and tries to set
auto-RTSCTS in two places. But someone with access to that hardware should
take a look, it seems it has the same problems. No auto-RTS and enhanced
mode is disabled in set_termios().

I think the code below is the safest way. It fixes things I can test
and does not change anything else.

summary for reviewers:

1. this patch keeps the enhanced mode activated for the 16C950 instead
of deactivating it in set_termios(). 
2. it activates auto-RTS for the 16C950
3. reenables UART_CAP_EFR for the 16C950 because of fix 1 and 2

Denis

> 
>> 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