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