diff mbox

[3/3] serial: pl011: allow very high baudrates

Message ID CACRpkdakLAPmD0TQwe2VNhdePDuFv2KXD2U1xoP1C0axkx2p7A@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Linus Walleij Sept. 21, 2012, 5:52 p.m. UTC
On Fri, Sep 21, 2012 at 5:25 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> Untested but I suspect the following may help

Nope it doesn't, it's not this part that goes wrong, it's the call to
tty_termios_encode_baud_rate() that is the problem, not how it
gets called. It's that function that fuzzes and "snaps" the baudrate
to some rough-fit speed and screws things up for me.

But I looked a bit at the patch as such.

It doesn't compile, but when I fix it like this:

> -               if (baud >= min && baud <= max)
> +               if (baud >= min && baud <= max) {
> +                       tty_termios_encode_baud_rate(tty, baud, baud);

s/tty/termios/

Then it compiles, but regresses.

What's going wrong is that the termios encoding of zero does
not happen anymore, for baudrate 0, i.e whereas we used to
encode 0 into termios and then return 9600 this encodes
9600 and returns 9600.

So if I handle baudrate 9600 specially instead like this it works:

 		 * The spd_hi, spd_vhi, spd_shi, spd_warp kludge...
@@ -362,26 +363,27 @@ uart_get_baud_rate(struct uart_port *port,
struct ktermios *termios,
 			baud = altbaud;

 		/*
-		 * Special case: B0 rate.
+		 * Special case: B0 rate. Note: this breaks if the
+		 * device cannot support 9600 baud
 		 */
 		if (baud == 0) {
 			hung_up = 1;
-			baud = 9600;
+			/* Encode zeroes to preserve semantics */
+			tty_termios_encode_baud_rate(termios, 0, 0);
+			return 9600;
 		}

-		if (baud >= min && baud <= max)
+		if (baud >= min && baud <= max) {
+			tty_termios_encode_baud_rate(termios, baud, baud);
 			return baud;
+		}

 		/*
 		 * Oops, the quotient was zero.  Try again with
 		 * the old baud rate if possible.
 		 */
-		termios->c_cflag &= ~CBAUD;
 		if (old) {
 			baud = tty_termios_baud_rate(old);
-			if (!hung_up)
-				tty_termios_encode_baud_rate(termios,
-								baud, baud);
 			old = NULL;
 			continue;
 		}
@@ -392,11 +394,9 @@ uart_get_baud_rate(struct uart_port *port, struct
ktermios *termios,
 		 */
 		if (!hung_up) {
 			if (baud <= min)
-				tty_termios_encode_baud_rate(termios,
-							min + 1, min + 1);
+				baud = min + 1;
 			else
-				tty_termios_encode_baud_rate(termios,
-							max - 1, max - 1);
+				baud = max - 1;
 		}
 	}
 	/* Should never happen */

(FWIW Signed-off-by: Linus Walleij <linus.walleij@linaro.org> for the twoliner)

But as mentioned I get the same errors...

Yours,
Linus Walleij

Comments

Alan Cox Sept. 21, 2012, 7:56 p.m. UTC | #1
On Fri, 21 Sep 2012 19:52:04 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Fri, Sep 21, 2012 at 5:25 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> 
> > Untested but I suspect the following may help
> 
> Nope it doesn't, it's not this part that goes wrong, it's the call to
> tty_termios_encode_baud_rate() that is the problem, not how it
> gets called. It's that function that fuzzes and "snaps" the baudrate
> to some rough-fit speed and screws things up for me.

It should only snap the baud rate if your caller didn't ask for a specific
speed. With the older uart_get_baud_rate code it mangled the encoding
because BOTHER got mangled by uart_get_baud_rate.

The intended behaviour at tty layer is

Caller passes BOTHER and actual bit rate - we return BOTHER and a bit
rate

Caller does not pass BOTHER (may not be TCGETS2 aware) we snap to the
nearest Bfoo rate if within 5% otherwise we return BOTHER based rates.

If you are calling TCSETS2 passing BOTHER and an actual specific speed
you should always be getting handed back the speed requested as it'll see
the BOTHER flag is present and assume the caller is smart.

So how are you setting the speed and can you see at which point it is
getting mushed ?

Alan
Russell King - ARM Linux Sept. 21, 2012, 8:34 p.m. UTC | #2
On Fri, Sep 21, 2012 at 08:56:03PM +0100, Alan Cox wrote:
> If you are calling TCSETS2 passing BOTHER and an actual specific speed
> you should always be getting handed back the speed requested as it'll see
> the BOTHER flag is present and assume the caller is smart.

Is this something that should be handled by glibc?  If so, ARM for
whatever reason still seems to use the standard TCGETS and TCSETS
calls... at least stracing stty in ubuntu precise suggests that's
the case.
Alan Cox Sept. 21, 2012, 8:55 p.m. UTC | #3
On Fri, 21 Sep 2012 21:34:40 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Fri, Sep 21, 2012 at 08:56:03PM +0100, Alan Cox wrote:
> > If you are calling TCSETS2 passing BOTHER and an actual specific speed
> > you should always be getting handed back the speed requested as it'll see
> > the BOTHER flag is present and assume the caller is smart.
> 
> Is this something that should be handled by glibc?  If so, ARM for
> whatever reason still seems to use the standard TCGETS and TCSETS
> calls... at least stracing stty in ubuntu precise suggests that's
> the case.

The design was agreed with the glibc people years ago. The glibc folks
then repeatedly ignored it and refused to add it. So I gave up on them.

If you are doing low level tty speed stuff, use ioctl directly.

Now that glibc has had a management change and a clue implant maybe it
can be fixed. Feel free to go kicking deal whales down beaches if you
care.

Alan
Linus Walleij Sept. 25, 2012, 6:48 p.m. UTC | #4
On Fri, Sep 21, 2012 at 9:56 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Fri, 21 Sep 2012 19:52:04 +0200
> Linus Walleij <linus.walleij@linaro.org> wrote:

>> it's the call to
>> tty_termios_encode_baud_rate() that is the problem, not how it
>> gets called. It's that function that fuzzes and "snaps" the baudrate
>> to some rough-fit speed and screws things up for me.
>
> (...)
> The intended behaviour at tty layer is
>
> Caller passes BOTHER and actual bit rate - we return BOTHER and a bit
> rate
>
> Caller does not pass BOTHER (may not be TCGETS2 aware) we snap to the
> nearest Bfoo rate if within 5% otherwise we return BOTHER based rates.

OK sorry for getting this backwards, so I was under the impression that
BOTHER was an internal detail of the TTY layer, not to be or:ed on
and passed in from the outside.

Then everything makes perfect sense, I'll try to patch the caller
instead for this "bug" and see what happens. Probably it JustWorks...

Thanks a lot Alan!
Linus Walleij
Alan Cox Sept. 25, 2012, 7 p.m. UTC | #5
> OK sorry for getting this backwards, so I was under the impression that
> BOTHER was an internal detail of the TTY layer, not to be or:ed on
> and passed in from the outside.

BOTHER is a speed setting like the other Bxxx values - it's an external
API.

> 
> Then everything makes perfect sense, I'll try to patch the caller
> instead for this "bug" and see what happens. Probably it JustWorks...

Cool
Linus Walleij Sept. 26, 2012, 8:06 a.m. UTC | #6
On Tue, Sep 25, 2012 at 8:48 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Sep 21, 2012 at 9:56 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>>
>> Caller passes BOTHER and actual bit rate - we return BOTHER and a bit
>> rate
>>
>> Caller does not pass BOTHER (may not be TCGETS2 aware) we snap to the
>> nearest Bfoo rate if within 5% otherwise we return BOTHER based rates.
>
> OK sorry for getting this backwards, so I was under the impression that
> BOTHER was an internal detail of the TTY layer, not to be or:ed on
> and passed in from the outside.

OK not I got it working thusly:

/*
 * Make sure the core will not snap baudrate to something
 * "close to" requested rate by setting the BOTHER
 * (baud rate other) flag.
 */
tty->termios->c_cflag &= ~CBAUD;
tty->termios->c_cflag |= BOTHER | (BOTHER >> IBSHIFT);
tty_encode_baud_rate(tty, baud, baud);

There are no in-kernel consumers doing this wicked thing so
mailing it here for reference.

Hope I got it right now...

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 7d9fbb8..a2442fb 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -351,8 +351,9 @@  uart_get_baud_rate(struct uart_port *port, struct
ktermios *termios,
 	else if (flags == UPF_SPD_WARP)
 		altbaud = 460800;

+	baud = tty_termios_baud_rate(termios);
+
 	for (try = 0; try < 2; try++) {
-		baud = tty_termios_baud_rate(termios);

 		/*