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

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

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

The ST Microelectronics variant of the PL011 is capable of supporting
very high non-standard baud rates, even above 4 Mbps. However the
uart_get_baud_rate() will not allow us to set these, so override that
calculation on very high speeds.

Cc: Par-Gunnar Hjalmdahl <par-gunnar.hjalmdahl@stericsson.com>
Signed-off-by: Guillaume Jaunet <guillaume.jaunet@stericsson.com>
Signed-off-by: Christophe Arnal <christophe.arnal@stericsson.com>
Signed-off-by: Matthias Locher <matthias.locher@stericsson.com>
Signed-off-by: Rajanikanth H.V <rajanikanth.hv@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/tty/serial/amba-pl011.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 32240a7..8ac1a42 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1507,10 +1507,19 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
 	}
 
 	/*
-	 * Ask the core to calculate the divisor for us.
+	 * Override baud calculation for very high non-standard baudrates
+	 * on the ST Micro oversampling variant. This must override the
+	 * uart_get_baud_rate() call, because this function changes
+	 * baudrate to the default on these baudrates.
 	 */
-	baud = uart_get_baud_rate(port, termios, old, 0,
-				  max_baud);
+	if (uap->vendor->oversampling &&
+	    ((termios->c_ispeed > 4000000) ||
+	     (termios->c_ospeed > 4000000)))
+		baud = termios->c_ispeed;
+	else
+		/* Ask the core to calculate the divisor for us. */
+		baud = uart_get_baud_rate(port, termios, old, 0,
+					  max_baud);
 
 	if (baud > port->uartclk/16)
 		quot = DIV_ROUND_CLOSEST(port->uartclk * 8, baud);
-- 
1.7.11.3

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

[Russell King - ARM Linux]

On Thu, Sep 20, 2012 at 11:46:08AM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> The ST Microelectronics variant of the PL011 is capable of supporting
> very high non-standard baud rates, even above 4 Mbps. However the
> uart_get_baud_rate() will not allow us to set these, so override that
> calculation on very high speeds.

You don't explain why it doesn't.  It should in theory allow you to,
because there's no limits within that function other than those which
you pass in as the minimum and maximum.

If your userspace hasn't been updated to use the integer baud rate
setting mechanisms, then that could be where the problem lies.
Alternatively, if some Bxxxx setting is not being respected by
tty_termios_baud_rate(), that also would need fixing.

But the fix you propose in this patch just looks wrong.

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

[Linus Walleij]

On Thu, Sep 20, 2012 at 9:00 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Sep 20, 2012 at 11:46:08AM +0200, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> The ST Microelectronics variant of the PL011 is capable of supporting
>> very high non-standard baud rates, even above 4 Mbps. However the
>> uart_get_baud_rate() will not allow us to set these, so override that
>> calculation on very high speeds.
>
> You don't explain why it doesn't.  It should in theory allow you to,
> because there's no limits within that function other than those which
> you pass in as the minimum and maximum.

So I looked closer: uart_get_baud_rate() in turn calls out to
tty_termios_baud_rate() which does some fuzzy matching to the
fix baudrate table and simply "snaps" the baudrate to the closest
match if it is "close enough".

I.e. 4050000 baud is close enough to 4000000 so it will round
off and "snap" to 4000000, whereas 3250000 baud will not
"close enough" to 3000000 so it will not "snap" to 3000000.

So we don't like that 4050000 snaps to 4000000.

I wonder how to fix this ... I'm planning to do what the comment
in tty_ioctl says:

/**
 *	tty_termios_baud_rate
 *	@termios: termios structure
 *
 *	Convert termios baud rate data into a speed. This should be called
 *	with the termios lock held if this termios is a terminal termios
 *	structure. May change the termios data. Device drivers can call this
 *	function but should use ->c_[io]speed directly as they are updated.
 *
 *	Locking: none
 */

Device drivers should use c_[io]speed directly!

Alan Cox wrote this, so Alan: should I just ditch the use of
uart_get_baud_rate() and program the divider directly from
c_[io]speed?

Yours,
Linus Walleij

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

[Alan Cox]

> Device drivers should use c_[io]speed directly!
> 
> Alan Cox wrote this, so Alan: should I just ditch the use of
> uart_get_baud_rate() and program the divider directly from
> c_[io]speed?

Yes.

The functions are designed to act as helpers for old devices. In fact
we can actually probably abolish tty_termios_baud_rate at this point as
I don't think there is much if anything left which blows up fed a non
Bxxx table entry.

I will have a look at that in fact see what it involves at this point.

Alan

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

[Russell King - ARM Linux]

On Fri, Sep 21, 2012 at 03:37:10PM +0100, Alan Cox wrote:
> > Device drivers should use c_[io]speed directly!
> > 
> > Alan Cox wrote this, so Alan: should I just ditch the use of
> > uart_get_baud_rate() and program the divider directly from
> > c_[io]speed?
> 
> Yes.
> 
> The functions are designed to act as helpers for old devices. In fact
> we can actually probably abolish tty_termios_baud_rate at this point as
> I don't think there is much if anything left which blows up fed a non
> Bxxx table entry.
> 
> I will have a look at that in fact see what it involves at this point.

Alan - the only issue that remains is handling the invalid baud rate
situation - if left to individual drivers to do this, we will see them
doing stuff (as was the case with this very patch - and was the case
prior to serial_core) such as using dev_err() to print an error and
merely returning from their set_termios function, or clamping to some
speed and not feeding back to userspace what they're actually doing.

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

[Alan Cox]

> Alan - the only issue that remains is handling the invalid baud rate
> situation - if left to individual drivers to do this, we will see them
> doing stuff (as was the case with this very patch - and was the case
> prior to serial_core) such as using dev_err() to print an error and
> merely returning from their set_termios function, or clamping to some
> speed and not feeding back to userspace what they're actually doing.

They've had years to adapt. It's time they got with the program or just
broke horribly.

Alan

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

[Russell King - ARM Linux]

On Fri, Sep 21, 2012 at 04:05:03PM +0100, Alan Cox wrote:
> > Alan - the only issue that remains is handling the invalid baud rate
> > situation - if left to individual drivers to do this, we will see them
> > doing stuff (as was the case with this very patch - and was the case
> > prior to serial_core) such as using dev_err() to print an error and
> > merely returning from their set_termios function, or clamping to some
> > speed and not feeding back to userspace what they're actually doing.
> 
> They've had years to adapt. It's time they got with the program or just
> broke horribly.

Sorry Alan, I don't think you really understand what I'm saying.

In the old days, lots of serial drivers just ignored termios settings
that they didn't support, or baud rates that they didn't know about -
so when userspace requests, eg 4Mbps on a UART port, the ioctl didn't
fail, and getting the termios back reported 4Mbps.

That is in violation of the POSIX general terminal interface standard,
and is something that I fixed with the uart baud rate functions - which
most serial_core using drivers are using.

Now you're pushing that logic back down into drivers, where driver authors
have to understand these details again, and that's precisely the reverse
of what should be happening.  We should be helping driver authors get
things right by providing helpers to do that, which is exactly what has
been done.

We need helper functions so we don't end up with 10s of buggy
implementations of the same calculation of baud rate quotient, which
each do their own range checking, and then go on to set some random
unreported-back-to-userspace baud rate.

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

[Alan Cox]

> So I looked closer: uart_get_baud_rate() in turn calls out to
> tty_termios_baud_rate() which does some fuzzy matching to the
> fix baudrate table and simply "snaps" the baudrate to the closest
> match if it is "close enough".

The fuzzing is getting done when re-encoding and because the code
encodes/re-decodes stuff.

> I wonder how to fix this ... I'm planning to do what the comment
> in tty_ioctl says:

If you support the ancient alt speed hacks then its not quite so neat.

Alan

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

[Alan Cox]

Untested but I suspect the following may help

commit 44590a8cc4ee6e40737b843d1d9593345ef3bf04
Author: Alan Cox <alan@linux.intel.com>
Date:   Fri Sep 21 16:36:36 2012 +0100

    serial: avoid iteratively re-encoding the termios
    
    If we do that then we will corrupt the termios data in some
    situations and thus not nicely encode the baud rates as we should.
    
    Signed-off-by: Alan Cox <alan@linux.intel.com>

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 78036c5..73a169e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -348,8 +348,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);
 
 		/*
 		 * The spd_hi, spd_vhi, spd_shi, spd_warp kludge...
@@ -359,26 +360,25 @@ 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;
 		}
 
-		if (baud >= min && baud <= max)
+		if (baud >= min && baud <= max) {
+			tty_termios_encode_baud_rate(tty, 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;
 		}
@@ -389,11 +389,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 */

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

[Alan Cox]

> Sorry Alan, I don't think you really understand what I'm saying.

I'm taking about getting rid of the tty_termios_baud_rate() helper not
the uart_get_baud_rate() helper. The tty one has no real purpose any more
because we always set c_ospeed.

The uart layer one just needs some bug fixing I think.

Alan

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

[Linus Walleij]

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:

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

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

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

[Alan Cox]

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

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

[Russell King - ARM Linux]

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.

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

[Alan Cox]

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

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

[Linus Walleij]

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

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

[Alan Cox]

> 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

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

[Linus Walleij]

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

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

[Alan Cox]

On Wed, 26 Sep 2012 10:06:04 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

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

Yes.