From: Johan Hovold <johan@kernel.org>
To: Denis Ahrens <denis@h3q.com>
Cc: linux-serial@vger.kernel.org, Johan Hovold <johan@kernel.org>,
Pavel Machek <pavel@ucw.cz>
Subject: Re: [PATCH] 16C950 UART enable Hardware Flow Control
Date: Wed, 27 May 2020 09:55:54 +0200 [thread overview]
Message-ID: <20200527075554.GG5276@localhost> (raw)
In-Reply-To: <B7715399-667F-4DB7-A19D-4CB037ECE64A@h3q.com>
[ 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
next prev parent reply other threads:[~2020-05-27 7:56 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2020-05-28 12:06 ` Pavel Machek
2020-05-27 20:49 Denis Ahrens
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200527075554.GG5276@localhost \
--to=johan@kernel.org \
--cc=denis@h3q.com \
--cc=linux-serial@vger.kernel.org \
--cc=pavel@ucw.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).