2002-07-06 23:57:36

by Russell King

[permalink] [raw]
Subject: Serial: updated serial drivers

Hi,

I've been maintaining a serial driver "off the side" of the ARM port
which cleans up the serial driver mess that we currently have, with
many duplications of serial.c, each with subtle bugs.

Since ARM seems to have a poliferation of various UARTs, each of them
different, if this were to carry on we'd end up with 6 or 7 more
copies of serial.c. This is obviously unacceptable.

So, with a view to cutting down on this duplication, I split serial.c
into a core driver, which can be used by various "low level" serial
hardware drivers, including the ARM ones, but also the 8250/16550
drivers.

Jeff and Arjan (iirc) expressed some concern about the PCI and PNP
code in serial.c, so that got separated out in what has now come to
be known as the serial rewrite.

The only non-ARM driver this patch affects is the 8250/16550 serial.c
driver. The others have not been ported to this infrastructure yet.

The patch does contain details on the interface between the low level
and core serial drivers.

This project was first announced to linux-kernel on 6 November 2001:

http://marc.theaimsgroup.com/?l=linux-kernel&m=100504194616062&w=2

Linus has expressed an interest in merging this work; hence this
message. Before I do send it to Linus, I would like to get some
feedback on the code. So, here is the latest patch against 2.5.24:

http://www.home.arm.linux.org.uk/cvs/serial-2.5.24.diff.bz2

It should cleanly apply to 2.5.24. The names of the serial options
have changed, so use your favourite configuration program to select
the relevant options.

This patch affects the following mainline drivers:
- 8250/16550 "generic" serial driver (serial.o)
- PCMCIA serial probe module (serial_cs.o)

Please send any bug reports in my direction. There is one specific
area that I'm unable to test here - 8250/16x50 "shared" interrupts
(where multiple 8250/16x50 devices share the same interrupt line).
I'm expecting my mailbox to overflow tomorrow, so...

Note that, as ever, 8250/16x50 sharing with non-serial devices is
NOT guaranteed to work on edge-based IRQ systems like PCs (and if
you want this to work you can either configure your kernel
appropriately, or pass "share_irqs=1" in modules.conf for
serial_8250.o)

The future:

- work with tytso to integrate any relevant bits of serial_core.c
with the tty layer.
- fix bugs
- there seems to be a fair number of people wanting support for
the higher speeds of UARTs (eg 16C950 and most motherboard
devices)
- add support for driverfs
- remove the silly situation where we have ports openable, but
no hardware behind them (and use some other method to create
serial devices on demand if still required)
- remove callout functionality, which has been marked as such for
a few years now. tytso mentions that the only program this
might break is minicom, which should be fixed.

I'm sure there's other stuff people want. 8)

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


2002-07-07 09:59:18

by Miquel van Smoorenburg

[permalink] [raw]
Subject: Re: Serial: updated serial drivers

In article <[email protected]>,
Russell King <[email protected]> wrote:
> - remove callout functionality, which has been marked as such for
> a few years now. tytso mentions that the only program this
> might break is minicom, which should be fixed.

Minicom doesn't care which port you use. It might be that some
people have saved configs of 6 years old that still contain
references to a cua device, but that's a problem any serial
program has.

>From the manpage of any recent minicom:

Serial port setup
*A - Serial device
/dev/tty1 or /dev/ttyS1 for most people.
/dev/cua<n> is still possible under linux, but not
recommended any more because these devices are
obsolete and many newly installed systems with
kernel 2.2.x don't have them. Use /dev/ttyS<n>
instead.

Mike.

2002-07-07 14:12:36

by Marko Kohtala

[permalink] [raw]
Subject: Re: Serial: updated serial drivers

Russell King wrote:
> Linus has expressed an interest in merging this work; hence this
> message. Before I do send it to Linus, I would like to get some
> feedback on the code. So, here is the latest patch against 2.5.24:
>
> http://www.home.arm.linux.org.uk/cvs/serial-2.5.24.diff.bz2
...
> I'm sure there's other stuff people want. 8)

It looks good.

I have one additional request that might best be done while doing this kind of rewrite.

I came across an IndustryPack (IP) carrier card. Each IP sits on a 16 bit bus. The carrier is a PCI device and has slots for four IP, each IP module I had carried eight 16550 serial ports.

With this I came up with two problems:

1) The PCI<->IP bridge had a bug with byte accesses. The iotype SERIAL_IO_MEM and iomem_reg_shift 1 addressed right addresses, but with byte access, so it did not work.

2) All the 32 serial ports were on same IRQ. You could find out which ports need service on an interrupt with one or two register reads, but the serial driver wanted to get and handle the IRQ on its own. The multiport support did not quite work for this.

One solution to allow the carrier driver address these would be to replace the struct uart_port iotype with a pointer to

struct serial_hw_ops {
unsigned int (*serial_in)(struct uart_port *, int offset);
void (*serial_out)(struct uart_port *, int offset, int value);
int (*enable_irq)(struct uart_port *, void (*irq_func)(struct uart_port *));
void (*disable_irq)(struct uart_port *);
};

Then implement new register_serial functions for the carrier card driver to use that take this as argument (serial8250_register?). The irq_func would be the function carrier driver calls to get the IRQ serviced (close to serial8250_handle_port).

The struct uart_port would need to also carry a pointer to carrier specific data. Only the enable_irq and disable_irq will need carrier data.

I think the current iobase and other members are good enough such that the serial_in and serial_out can be implemented rather efficiently without the extra indirection through the carrier data pointer.

I'm not sure if it would still incur a small overhead to I/O access. Perhaps not, as it would seem to me the serial_in does not get inlined and there would be one switch statement we could avoid by this. You are more competent to judge this.

This would also nicely clean up the SERIAL_IO_HUB6 case in serial_in/out. And it would help other multiport serial cards to utilize the registers they have to isolate the ports that interrupt.

The enable_irq in the carrier driver would see if the port is the first on the carrier, and request the IRQ to its own interrupt function. On the same interrupt there can be other things besides the serial ports.

struct serial_hw_ops instances and the functions for the standard single port or standard multiport cases should be exported. The carrier card has it's own module that needs access to these things for the serial8250_register call.

There are IP with other UART than 16550, so this should be used for all UART drivers.

Since this seems to affect the interface to port registration functions, it might be better to have it done now rather than later.


............................................................
Maksuton s?hk?posti aina k?yt?ss? http://luukku.com
Kuukausimaksuton MTV3 Internet-liittym? http://www.mtv3.fi/liittyma

2002-07-15 10:01:33

by William Lee Irwin III

[permalink] [raw]
Subject: Re: Serial: updated serial drivers

On Sun, Jul 07, 2002 at 01:00:09AM +0100, Russell King wrote:
> I've been maintaining a serial driver "off the side" of the ARM port
> which cleans up the serial driver mess that we currently have, with
> many duplications of serial.c, each with subtle bugs.

global_cli() overhead on my testbox is a significant problem.

Profile info from tbench 1024 with ttyS0 as stdout, taken on a 16 cpu
i386 box with 16GB of RAM and irqbalance disabled, (needed to boot):

90372662 total 713.6412
44801051 default_idle 861558.6731
36220921 mod_timer 113190.3781
2510075 timer_bh 2764.3998
2344795 __global_cli 8620.5699
1446315 __wake_up 7693.1649
1370742 do_gettimeofday 10078.9853
924996 schedule 831.8309
512368 do_softirq 2328.9455
103136 tasklet_hi_action 526.2041
40640 system_call 923.6364
19238 do_page_fault 14.2927
12835 add_wait_queue 103.5081
10667 remove_wait_queue 83.3359
7990 bh_action 38.4135
5303 pte_alloc_one 27.6198
4665 schedule_timeout 29.1562
4584 pgd_alloc 24.3830
3870 syscall_call 351.8182
3633 try_to_wake_up 6.3073
3100 exit_notify 3.5068
2566 del_timer 14.9186

The disabling of irqbalance should make these profiling results valid.


Cheers,
Bill

2002-07-15 10:59:51

by William Lee Irwin III

[permalink] [raw]
Subject: Re: Serial: updated serial drivers

On Sun, Jul 07, 2002 at 01:00:09AM +0100, Russell King wrote:
>> I've been maintaining a serial driver "off the side" of the ARM port
>> which cleans up the serial driver mess that we currently have, with
>> many duplications of serial.c, each with subtle bugs.

On Mon, Jul 15, 2002 at 03:03:10AM -0700, William Lee Irwin III wrote:
> global_cli() overhead on my testbox is a significant problem.
> Profile info from tbench 1024 with ttyS0 as stdout, taken on a 16 cpu
> i386 box with 16GB of RAM and irqbalance disabled, (needed to boot):

the profiling results were for a kernel without the new serial stuff.
The new serial stuff appears to need some arch compatibility auditing/
fixes for NUMA-Q.



Cheers,
Bill

2002-07-15 11:22:04

by Russell King

[permalink] [raw]
Subject: Re: Serial: updated serial drivers

On Mon, Jul 15, 2002 at 04:01:35AM -0700, William Lee Irwin III wrote:
> the profiling results were for a kernel without the new serial stuff.
> The new serial stuff appears to need some arch compatibility auditing/
> fixes for NUMA-Q.

I am not aware of any architecture specific code in the new serial
code, with the exception of a couple of writes to port 0x80 for i386
architectures (which the original serial.c driver did as well.)

Can you give an idea where it fails/kernel messages please?

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

2002-07-15 14:22:58

by Dave Hansen

[permalink] [raw]
Subject: Re: Serial: updated serial drivers

William Lee Irwin III wrote:
> On Sun, Jul 07, 2002 at 01:00:09AM +0100, Russell King wrote:
>
>>I've been maintaining a serial driver "off the side" of the ARM port
>>which cleans up the serial driver mess that we currently have, with
>>many duplications of serial.c, each with subtle bugs.
>
>
> global_cli() overhead on my testbox is a significant problem.
>
> Profile info from tbench 1024 with ttyS0 as stdout, taken on a 16 cpu
> i386 box with 16GB of RAM and irqbalance disabled, (needed to boot):
>
<snip profile>
>
> The disabling of irqbalance should make these profiling results valid.

So, is irqbalance the thing that is screwing up our profiles on 2.5?
We were getting some strage profiles that made us look at oprofile
again.

oprofile is really cool, but readprofile is dead simple.
--
Dave Hansen
[email protected]

2002-07-15 18:15:58

by William Lee Irwin III

[permalink] [raw]
Subject: Re: Serial: updated serial drivers

On Mon, Jul 15, 2002 at 04:01:35AM -0700, William Lee Irwin III wrote:
>> the profiling results were for a kernel without the new serial stuff.
>> The new serial stuff appears to need some arch compatibility auditing/
>> fixes for NUMA-Q.

On Mon, Jul 15, 2002 at 12:24:55PM +0100, Russell King wrote:
> I am not aware of any architecture specific code in the new serial
> code, with the exception of a couple of writes to port 0x80 for i386
> architectures (which the original serial.c driver did as well.)
> Can you give an idea where it fails/kernel messages please?

It's not obvious to me, either, I actually read the stuff and didn't
see where the problem could be.

It appeared to fail before console_init(). The system is largely
dedicated to other projects so there will be a bit of turnaround time
between test runs. I'll probably be able to pinpoint the point of
failure later tonight.


Cheers,
Bill