[PATCH] 8250: Now honours baud rate lower bounds

[PATCH] 8250: Now honours baud rate lower bounds

[Anton Vorontsov]

A platform clock drives 8250 ports in most SOC systems, the clock
might run at high frequencies, and so it's not always possible to
downscale uart clock to a desired value.

Currently the 8250 uart driver accepts not supported baud rates, and
what is worse, it is doing this silently, and then passes not accepted
values to a new termios, so userspace has no chance to catch this kind
of errors (userspace verifies that settings were accepted by reading
back and comparing the settings).

This patch fixes the issue by passing minimum baud rate to the
uart_get_baud_rate() call, the call should take care of all bounds,
so userspace should now report:

  # stty -F /dev/ttyS0 speed 300
  115200
  stty: /dev/ttyS0: unable to perform all requested operations

p.s. uart_get_baud_rate() falls back to 9600, which still might be too
     low for some 10 GHz platforms, but that's a separate issue, and
     we can wait with fixing this till we find such a platform.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/serial/8250.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index fb867a9..22d8d4f 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -567,6 +567,11 @@ static inline void _serial_dl_write(struct uart_8250_port *up, int value)
 	serial_outp(up, UART_DLM, value >> 8 & 0xff);
 }
 
+static unsigned int _serial_dl_max(struct uart_8250_port *up)
+{
+	return 0xffff;
+}
+
 #if defined(CONFIG_SERIAL_8250_AU1X00)
 /* Au1x00 haven't got a standard divisor latch */
 static int serial_dl_read(struct uart_8250_port *up)
@@ -584,6 +589,14 @@ static void serial_dl_write(struct uart_8250_port *up, int value)
 	else
 		_serial_dl_write(up, value);
 }
+
+static unsigned int serial_dl_max(struct uart_8250_port *up)
+{
+	if (up->port.iotype == UPIO_AU)
+		return 0xffffffff;
+	else
+		return _serial_dl_max(up);
+}
 #elif defined(CONFIG_SERIAL_8250_RM9K)
 static int serial_dl_read(struct uart_8250_port *up)
 {
@@ -602,9 +615,15 @@ static void serial_dl_write(struct uart_8250_port *up, int value)
 		_serial_dl_write(up, value);
 	}
 }
+
+static unsigned int serial_dl_max(struct uart_8250_port *up)
+{
+	return _serial_dl_max(up);
+}
 #else
 #define serial_dl_read(up) _serial_dl_read(up)
 #define serial_dl_write(up, value) _serial_dl_write(up, value)
+#define serial_dl_max(up) _serial_dl_max(up)
 #endif
 
 /*
@@ -2272,7 +2291,9 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
 	/*
 	 * Ask the core to calculate the divisor for us.
 	 */
-	baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/16);
+	baud = uart_get_baud_rate(port, termios, old,
+				  port->uartclk / 16 / serial_dl_max(up),
+				  port->uartclk / 16);
 	quot = serial8250_get_divisor(port, baud);
 
 	/*
-- 
1.6.3.3

Re: [PATCH] 8250: Now honours baud rate lower bounds

[Alan Cox]

On Fri, 3 Jul 2009 01:36:24 +0400
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> A platform clock drives 8250 ports in most SOC systems, the clock
> might run at high frequencies, and so it's not always possible to
> downscale uart clock to a desired value.

> This patch fixes the issue by passing minimum baud rate to the
> uart_get_baud_rate() call, the call should take care of all bounds,
> so userspace should now report:

We ought to be able to deduce the minimum baud rate from the divisor
being bigger than the chip supports - or do some of these devices not
support the full hw divisor range of the real chip which is what I assume
from your patch ?

> +static unsigned int _serial_dl_max(struct uart_8250_port *up)
> +{
> +	return 0xffff;
> +}
> +

We have far too many magic ifdef routines for this sort of stuff already.
Is there any reason we can't put info on the true divisor or at least the
baud rate range into the uart_8250_port structure so that we get rid of
all of 

> +static unsigned int serial_dl_max(struct uart_8250_port *up)
> +{
> +	if (up->port.iotype == UPIO_AU)
> +		return 0xffffffff;
> +	else
> +		return _serial_dl_max(up);
> +}
>  #elif defined(CONFIG_SERIAL_8250_RM9K)
>  static int serial_dl_read(struct uart_8250_port *up)
>  {
> @@ -602,9 +615,15 @@ static void serial_dl_write(struct uart_8250_port *up, int value)
>  		_serial_dl_write(up, value);
>  	}
>  }
> +
> +static unsigned int serial_dl_max(struct uart_8250_port *up)
> +{
> +	return _serial_dl_max(up);
> +}
>  #else
>  #define serial_dl_read(up) _serial_dl_read(up)
>  #define serial_dl_write(up, value) _serial_dl_write(up, value)
> +#define serial_dl_max(up) _serial_dl_max(up)
>  #endif
>  
>  /*
> @@ -2272,7 +2291,9 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
>  	/*
>  	 * Ask the core to calculate the divisor for us.
>  	 */
> -	baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/16);
> +	baud = uart_get_baud_rate(port, termios, old,
> +				  port->uartclk / 16 / serial_dl_max(up),
> +				  port->uartclk / 16);
>  	quot = serial8250_get_divisor(port, baud);

in favour of

		uart_get_baud_rate(port, termios, old, port->uartclk / 16,
				port->uartclk / 16 / port->uartdivider)

Alan

[PATCH v2] 8250: Now honours baud rate lower bounds

[Anton Vorontsov]

A platform clock drives 8250 ports in most SOC systems, the clock
might run at high frequencies, and so it's not always possible to
downscale uart clock to a desired value.

Currently the 8250 uart driver accepts not supported baud rates, and
what is worse, it is doing this silently, and then passes not accepted
values to a new termios, so userspace has no chance to catch this kind
of errors (userspace verifies that settings were accepted by reading
back and comparing the settings).

This patch fixes the issue by passing minimum baud rate to the
uart_get_baud_rate() call, the call should take care of all bounds,
so userspace should now report:

  # stty -F /dev/ttyS0 speed 300
  115200
  stty: /dev/ttyS0: unable to perform all requested operations

p.s. uart_get_baud_rate() falls back to 9600, which still might be too
     low for some 10 GHz platforms, but that's a separate issue, and
     we can wait with fixing this till we find such a platform.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

On Thu, Jul 02, 2009 at 10:53:12PM +0100, Alan Cox wrote:
> On Fri, 3 Jul 2009 01:36:24 +0400
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> > A platform clock drives 8250 ports in most SOC systems, the clock
> > might run at high frequencies, and so it's not always possible to
> > downscale uart clock to a desired value.
> 
> > This patch fixes the issue by passing minimum baud rate to the
> > uart_get_baud_rate() call, the call should take care of all bounds,
> > so userspace should now report:
> 
> We ought to be able to deduce the minimum baud rate from the divisor
> being bigger than the chip supports - or do some of these devices not
> support the full hw divisor range of the real chip which is what I assume
> from your patch ?

No, HW supports full hw divisor range, but uart clk is 533 MHz,
and 533 MHz / 16 / 0xffff = 508 bauds. But currently the driver
calculates divisor without min value, and 300 bauds ends up with
0x1b2071 div value which doesn't fit into 16 bits, and so we're
getting bogus baud rate and garbage on the console, and no single
message that something went wrong.

> > +static unsigned int _serial_dl_max(struct uart_8250_port *up)
> > +{
> > +	return 0xffff;
> > +}
> > +
> 
> We have far too many magic ifdef routines for this sort of stuff already.
> Is there any reason we can't put info on the true divisor or at least the
> baud rate range into the uart_8250_port structure so that we get rid of
> all of 

Heh. I just found specs for Au1100, and it appears they use 32
bit registers, but the divisor itself is still 0xffff (other bits
are reserved). So I don't think we need any special cases, at least
for now.

How about this patch down below?

Thanks.

 drivers/serial/8250.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index fb867a9..7720816 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2272,7 +2272,9 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
 	/*
 	 * Ask the core to calculate the divisor for us.
 	 */
-	baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/16);
+	baud = uart_get_baud_rate(port, termios, old,
+				  port->uartclk / 16 / 0xffff,
+				  port->uartclk / 16);
 	quot = serial8250_get_divisor(port, baud);
 
 	/*
-- 
1.6.3.3

Re: [PATCH v2] 8250: Now honours baud rate lower bounds

[Alan Cox]

> How about this patch down below?

Thats much better.

Alan

Re: [PATCH v2] 8250: Now honours baud rate lower bounds

[Anton Vorontsov]

On Fri, Jul 03, 2009 at 10:39:24AM +0100, Alan Cox wrote:
> > How about this patch down below?
> 
> Thats much better.

Alan, is that patch in your queue? I believe you didn't give up on
8250 maintenance so I don't have to resend it to Greg, correct?

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2