* [PATCH] 16C950 UART enable Hardware Flow Control
@ 2020-05-24 16:31 Denis Ahrens
2020-05-25 8:27 ` Johan Hovold
0 siblings, 1 reply; 6+ messages in thread
From: Denis Ahrens @ 2020-05-24 16:31 UTC (permalink / raw)
To: linux-serial; +Cc: gregkh
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);
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] 16C950 UART enable Hardware Flow Control
2020-05-24 16:31 [PATCH] 16C950 UART enable Hardware Flow Control Denis Ahrens
@ 2020-05-25 8:27 ` Johan Hovold
2020-05-25 17:49 ` Denis Ahrens
0 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2020-05-25 8:27 UTC (permalink / raw)
To: Denis Ahrens; +Cc: linux-serial, gregkh
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.
Also your patch has had all tabs turned into spaces. You may need to fix
your mail setup. Try sending the patch to yourself first and make sure
you can apply it (also take a look at git-send-email).
Johan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 16C950 UART enable Hardware Flow Control
2020-05-25 8:27 ` Johan Hovold
@ 2020-05-25 17:49 ` Denis Ahrens
2020-05-27 7:55 ` Johan Hovold
0 siblings, 1 reply; 6+ messages in thread
From: Denis Ahrens @ 2020-05-25 17:49 UTC (permalink / raw)
To: linux-serial; +Cc: Johan Hovold
> 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.
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)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] 16C950 UART enable Hardware Flow Control
2020-05-25 17:49 ` Denis Ahrens
@ 2020-05-27 7:55 ` Johan Hovold
2020-05-28 12:06 ` Pavel Machek
0 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2020-05-27 7:55 UTC (permalink / raw)
To: Denis Ahrens; +Cc: linux-serial, Johan Hovold, Pavel Machek
[ 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 16C950 UART enable Hardware Flow Control
2020-05-27 7:55 ` Johan Hovold
@ 2020-05-28 12:06 ` Pavel Machek
0 siblings, 0 replies; 6+ messages in thread
From: Pavel Machek @ 2020-05-28 12:06 UTC (permalink / raw)
To: Johan Hovold; +Cc: Denis Ahrens, linux-serial
[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]
Hi!
> [ 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.
Hmm, that's compact flash card, and long time ago. I probably still
have it somewhere, but I have not used it in a long long time...
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 16C950 UART enable Hardware Flow Control
@ 2020-05-27 20:49 Denis Ahrens
0 siblings, 0 replies; 6+ messages in thread
From: Denis Ahrens @ 2020-05-27 20:49 UTC (permalink / raw)
To: linux-serial
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-28 12:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-24 16:31 [PATCH] 16C950 UART enable Hardware Flow Control Denis Ahrens
2020-05-25 8:27 ` Johan Hovold
2020-05-25 17:49 ` Denis Ahrens
2020-05-27 7:55 ` Johan Hovold
2020-05-28 12:06 ` Pavel Machek
2020-05-27 20:49 Denis Ahrens
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.