2003-01-03 16:12:16

by Russell King

[permalink] [raw]
Subject: [SERIAL] change_speed -> settermios change

Hi,

This is the final installment of the "sanitise change_speed" changes.
There are now three drivers which want to get the raw baud rate rather
than the divisor - nb85e_uart.c, sunsab.c, and sunzilog.c.

We have provided methods in previous csets to allow low level drivers
to convert a termios to a divisor or baud rate. Now, we provide the
drivers with the termios structure.

This has several advantages:

- the low level driver can obtain the divisor or baud rate directly,
without having to play math games.

- the low level driver can force various termios flags for settings
which it doesn't support.

I have one concerns surrounding the three drivers mentioned above.
Currently, they blindly use uart_get_baud_rate() without limiting the
maximum baud rate. Since the baud rate will be limited (by the
hardware), we must limit the resulting baud rate must be reflected
back into the termios c_cflag member.

I believe the low level drivers should be responsible for handling
this themselves - this is especially true when you start considering
the "high speed" modes of x86 motherboard serial ports (which dwmw2
would like to support.)

Lastly, uart_update_timeout() is imperfect. It takes a divisor, which
you may not have (since you're only working with baud rates.) There
is also the related mess of using the "magic" custom divisors, which
uart_update_timeout() would not be friendly towards. However, it is
friendly towards nonstandard baud rates which satisfy clock / divisor,
eg, 3600 baud, 7200 baud (both of which I have used in the past.)

I suspect that uart_update_timeout() will eventually take the cflag
setting and the period in picoseconds to send one byte. This approach,
along with support for the "magic" custom divisors, should mean that
we adequately cover this area without too much pain.

A patch against 2.5.54-bkcur is available from:

http://www.arm.linux.org.uk/misc/serial.diff

It should also be compatible with plain unpacked 2.5.54. If BK people
want to pull the changes, drop me a mail prior to pulling, thanks.

Documentation/serial/driver | 41 ++++++++++++++++++++------
drivers/serial/21285.c | 55 ++++++++++++++++++++++++++---------
drivers/serial/8250.c | 60 ++++++++++++++++++++++----------------
drivers/serial/amba.c | 43 +++++++++++++++++----------
drivers/serial/anakin.c | 33 +++++++++++++++++----
drivers/serial/clps711x.c | 45 ++++++++++++++++++----------
drivers/serial/core.c | 33 +++++++++------------
drivers/serial/mux.c | 13 +++-----
drivers/serial/nb85e_uart.c | 14 +++-----
drivers/serial/sa1100.c | 69 ++++++++++++++++++++++++++++----------------
drivers/serial/sunsab.c | 15 +++------
drivers/serial/sunsu.c | 36 +++++++++++++++++-----
drivers/serial/sunzilog.c | 15 +++++----
drivers/serial/uart00.c | 46 +++++++++++++++++++----------
include/linux/serial_core.h | 25 +++++++++++++--
15 files changed, 359 insertions, 184 deletions

through these ChangeSets:

<[email protected]> (03/01/03 1.944)
[SERIAL] Convert change_speed() to settermios()

Several serial drivers want to obtain the numeric baud rate when
configuring their serial ports. Currently, two methods are used
to "work around" this inadequacy in the change_speed API:

baud = tty_get_baud_rate(port->info->tty);

baud = BAUD_BASE / (16 * quot);

Passing the termios structure down means that we can use
uart_get_baud_rate() instead. We can also ensure that the various
termios flags for options we don't support are correctly set.

Lastly, this also provides 8250.c with a clean method for supporting
divisors that are greater than the baud_base.

<[email protected]> (03/01/03 1.943)
[SERIAL] Remove unused info->event

<[email protected]> (03/01/03 1.942)
[SERIAL] Add prototypes and rename UPF_FLAGS

UPF_FLAGS is confusing - rename it to UPF_CHANGE_MASK.
Add uart_update_timeout, uart_get_baud_rate and uart_get_divisor
prototypes.

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html


2003-01-06 07:02:03

by David Miller

[permalink] [raw]
Subject: Re: [SERIAL] change_speed -> settermios change

From: Russell King <[email protected]>
Date: Fri, 3 Jan 2003 16:19:16 +0000

I have one concerns surrounding the three drivers mentioned above.
Currently, they blindly use uart_get_baud_rate() without limiting the
maximum baud rate. Since the baud rate will be limited (by the
hardware), we must limit the resulting baud rate must be reflected
back into the termios c_cflag member.

Hmmm, maybe it's a better idea to store the min/max in the UART port
structure and have the upper layer do the limiting before we call
down into the driver?

2003-01-06 07:41:50

by Miles Bader

[permalink] [raw]
Subject: Re: [SERIAL] change_speed -> settermios change

Looks OK to me; as soon as you decide how the low-level driver should
limit the baud rate, I'll update nb85e_uart.c to do it (storing max/min in
the uart_port structure, as someone suggested, seems pretty convenient).

If unsupported termios flags are passed in, what should the low-level
function do? Tweak the contents of *termios to reflect reality?

BTW, why the name `settermios'? Something like `set_termios' seems easier
to read and more in keeping with the style used elsewhere in the
serial-port code. [settermios brings back horrible memories of BSD kernel
code...]

Thanks,

-miles
--
Run away! Run away!

2003-01-06 11:28:16

by Russell King

[permalink] [raw]
Subject: Re: [SERIAL] change_speed -> settermios change

On Sun, Jan 05, 2003 at 11:02:18PM -0800, David S. Miller wrote:
> Hmmm, maybe it's a better idea to store the min/max in the UART port
> structure and have the upper layer do the limiting before we call
> down into the driver?

I like that idea. However, I'd rather not put these in the uart port
structure because:

1. the max/min would be dependent on the uart clock rate for ports which
use a clock divisor.

2. the max/min rate may require driver specific knowledge (eg, when the
port supports *2 and *4 high speed modes)

3. the max/min might be constant for certain ports (eg, sunsab, nb85e)

I'm currently considering whether to pass the max / min into
uart_get_baud_rate(), along with the old termios:

a) move the loop out of uart_get_divisor() into uart_get_baud_rate()
b) uart_get_divisor() would be responsible for providing
uart_get_baud_rate() with the appropriate min/max for case (1).
c) for case (2), the driver itself would pass these parameters itself.
Whether or not the min/max depend on the uart clock rate is something
which only the driver itself can know.

There is a fourth case which I didn't list above:

4. ports that use the raw baud rate may not support all baud rates that
we are able to pass between a maximum and minimum.

This is outside our current scope at the moment (the usb serial drivers),
and from reading the usb code, it is unclear whether this is an issue for
any of these drivers.

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html