Subject: [patch] Incorrect mapping of serial ports to lines

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 28 Jun 2004 15:25:04 +0200 (CEST)

Hello,

 Onboard PC-compatible serial ports of the 8250 family are expected to be
assigned to lines 0 - 3.  Unfortunately for MIPS this is not guaranteed as
EXTRA_SERIAL_PORT_DEFNS and HUB6_SERIAL_PORT_DFNS precede
STD_SERIAL_PORT_DEFNS on the port list and their definitions change
depending on CONFIG_SERIAL_MANY_PORTS and CONFIG_HUB6 which are user
settable.  As a result, they may get different assignments depending on
configuration -- e.g. my last build for the Malta board resulted in its
onboard ports being assigned to lines 28 and 29.

 This can be fixed with a correct ordering of entries on the port list, 
like the following.  OK to apply?

  Maciej

patch-mips-2.4.26-20040531-mips-serial-0
diff -up --recursive --new-file linux-mips-2.4.26-20040531.macro/include/asm-mips/serial.h linux-mips-2.4.26-20040531/include/asm-mips/serial.h
--- linux-mips-2.4.26-20040531.macro/include/asm-mips/serial.h	2004-06-13 23:16:56.000000000 +0000
+++ linux-mips-2.4.26-20040531/include/asm-mips/serial.h	2004-06-27 12:49:23.000000000 +0000
@@ -5,6 +5,7 @@
  *
  * Copyright (C) 1999 by Ralf Baechle
  * Copyright (C) 1999, 2000 Silicon Graphics, Inc.
+ * Copyright (C) 2004  Maciej W. Rozycki
  */
 #ifndef _ASM_SERIAL_H
 #define _ASM_SERIAL_H
@@ -410,14 +411,25 @@
 #define DDB5477_SERIAL_PORT_DEFNS
 #endif
 
+/*
+ * The order matters here and should be as follows:
+ *
+ * 1. STD_SERIAL_PORT_DEFNS
+ * 2. board-specific ports (please keep sorted)
+ * 3. EXTRA_SERIAL_PORT_DEFNS
+ * 4. HUB6_SERIAL_PORT_DFNS
+ *
+ * otherwise serial line numbers may change across
+ * kernel builds if configuration changes. --macro
+ */
 #define SERIAL_PORT_DFNS			\
+	STD_SERIAL_PORT_DEFNS			\
+						\
 	ATLAS_SERIAL_PORT_DEFNS			\
 	AU1000_SERIAL_PORT_DEFNS		\
 	COBALT_SERIAL_PORT_DEFNS		\
 	DDB5477_SERIAL_PORT_DEFNS		\
 	EV96100_SERIAL_PORT_DEFNS		\
-	EXTRA_SERIAL_PORT_DEFNS			\
-	HUB6_SERIAL_PORT_DFNS			\
 	ITE_SERIAL_PORT_DEFNS           	\
 	IVR_SERIAL_PORT_DEFNS           	\
 	JAZZ_SERIAL_PORT_DEFNS			\
@@ -426,8 +438,10 @@
 	MOMENCO_OCELOT_C_SERIAL_PORT_DEFNS	\
 	MOMENCO_JAGUAR_ATX_SERIAL_PORT_DEFNS	\
 	SEAD_SERIAL_PORT_DEFNS			\
-	STD_SERIAL_PORT_DEFNS			\
 	TITAN_SERIAL_PORT_DEFNS			\
-	TXX927_SERIAL_PORT_DEFNS
+	TXX927_SERIAL_PORT_DEFNS		\
+						\
+	EXTRA_SERIAL_PORT_DEFNS			\
+	HUB6_SERIAL_PORT_DFNS
 
 #endif /* _ASM_SERIAL_H */
diff -up --recursive --new-file linux-mips-2.4.26-20040531.macro/include/asm-mips64/serial.h linux-mips-2.4.26-20040531/include/asm-mips64/serial.h
--- linux-mips-2.4.26-20040531.macro/include/asm-mips64/serial.h	2003-12-18 03:57:25.000000000 +0000
+++ linux-mips-2.4.26-20040531/include/asm-mips64/serial.h	2004-06-27 12:52:51.000000000 +0000
@@ -5,6 +5,7 @@
  *
  * Copyright (C) 1999 by Ralf Baechle
  * Copyright (C) 1999, 2000 Silicon Graphics, Inc.
+ * Copyright (C) 2004  Maciej W. Rozycki
  */
 #ifndef _ASM_SERIAL_H
 #define _ASM_SERIAL_H
@@ -146,12 +147,22 @@
 #define IP27_SERIAL_PORT_DEFNS
 #endif /* CONFIG_SGI_IP27 */
 
+/*
+ * The order matters here and should be as follows:
+ *
+ * 1. STD_SERIAL_PORT_DEFNS
+ * 2. board-specific ports (please keep sorted)
+ *
+ * otherwise serial line numbers may change across
+ * kernel builds if configuration changes. --macro
+ */
 #define SERIAL_PORT_DFNS				\
+	STD_SERIAL_PORT_DEFNS				\
+							\
 	IP27_SERIAL_PORT_DEFNS				\
 	MOMENCO_OCELOT_C_SERIAL_PORT_DEFNS		\
 	MOMENCO_JAGUAR_ATX_SERIAL_PORT_DEFNS		\
 	SEAD_SERIAL_PORT_DEFNS				\
-	STD_SERIAL_PORT_DEFNS				\
 	TITAN_SERIAL_PORT_DEFNS
 
 #define RS_TABLE_SIZE	64

---

From: Ralf Baechle <[email-masked]>
Date: Tue, 29 Jun 2004 01:59:08 +0200

> like the following.  OK to apply?

Yep, having STD_SERIAL_PORT_DEFNS after EXTRA_SERIAL_PORT_DEFNS was
unintentional.  The idea was to have to have all the system-specific at
the start of the list or we get fun on all system that may have on-board
serials which should receive the lowest numbers and any (E)ISA serial cards
at the end, so my suggestion for fixing this would look a little different:

#define SERIAL_PORT_DFNS                                \
        COBALT_SERIAL_PORT_DEFNS                        \
        DDB5477_SERIAL_PORT_DEFNS                       \
        EV96100_SERIAL_PORT_DEFNS                       \
        IP32_SERIAL_PORT_DEFNS                          \
        ITE_SERIAL_PORT_DEFNS                           \
        IVR_SERIAL_PORT_DEFNS                           \
        JAZZ_SERIAL_PORT_DEFNS                          \
        MOMENCO_OCELOT_G_SERIAL_PORT_DEFNS              \
        MOMENCO_OCELOT_C_SERIAL_PORT_DEFNS              \
        MOMENCO_OCELOT_SERIAL_PORT_DEFNS                \
        TXX927_SERIAL_PORT_DEFNS                        \
        AU1000_SERIAL_PORT_DEFNS			\
							\
        STD_SERIAL_PORT_DEFNS                           \
	EXTRA_SERIAL_PORT_DEFNS				\
        HUB6_SERIAL_PORT_DFNS                           \

Comments?

 Ralf

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Tue, 29 Jun 2004 13:57:34 +0200 (CEST)

>         AU1000_SERIAL_PORT_DEFNS			\

 Hmm, why is Au1000 at the end -- does the system have others from the
list above, too?

 Also you've removed a few system-specific ports -- why?

> 	EXTRA_SERIAL_PORT_DEFNS				\

 That's a bit troublesome for the Malta board which has both a pair of
PC-compatible serial ports which are expected to be lines 0 and 1 and an
Atlas serial port, which is expected to be line 2.  The Atlas port on the
Malta board isn't handled by Linux right now, but I plan to fix it.  Are
there systems that have both PC-compatible ports and system-specific ones
and expect them to be mapped in the reverse order?

 AFAIK, PC-compatible serial ports on PCI cards get mapped dynamically to
lines above this standard list.  I don't know about EISA boards, but it
would be consistent to handle them the same way, i.e. I'd propose to fix
the driver in this case.

  Maciej

---

From: Geert Uytterhoeven <[email-masked]>
Date: Tue, 29 Jun 2004 14:09:36 +0200 (MEST)

> and expect them to be mapped in the reverse order?

The NEC DDB Vrc-5074 (and probably the other DDB variants as well) has one
serial port in the Nile 4 host bridge, and 2 serial ports in the Super I/O.

To me it sounds the most logical if the one in the Nile 4 is ttyS0.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email-masked]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Tue, 29 Jun 2004 15:49:11 +0200 (CEST)

> To me it sounds the most logical if the one in the Nile 4 is ttyS0.

 Then we need to find a way to make the order configurable somehow.

---

From: Ralf Baechle <[email-masked]>
Date: Tue, 29 Jun 2004 17:03:16 +0200

On Tue, Jun 29, 2004 at 03:49:11PM +0200, Maciej W. Rozycki wrote:
> Date: Tue, 29 Jun 2004 15:49:11 +0200 (CEST)
> From: "Maciej W. Rozycki" <[email-masked]>
> To: Geert Uytterhoeven <[email-masked]>
> Cc: Ralf Baechle <[email-masked]>,
> 	Linux/MIPS Development <[email-masked]>
> Subject: Re: [patch] Incorrect mapping of serial ports to lines
> Content-Type: TEXT/PLAIN; charset=US-ASCII
> 

How about you leave CONFIG_HAVE_STD_PC_SERIAL_PORT disabled for Malta then
and supply your own MALTA_SERIAL_PORT_DEFNS instead then?  That would
require some small changes to no longer nest the CONFIG_SERIAL_MANY_PORTS
ifdef inside CONFIG_HAVE_STD_PC_SERIAL_PORT - about as below.

  Ralf

Index: include/asm-mips/serial.h
===================================================================
RCS file: /home/cvs/linux/include/asm-mips/serial.h,v
retrieving revision 1.23.2.18
diff -u -r1.23.2.18 serial.h
--- include/asm-mips/serial.h	18 Dec 2003 01:51:37 -0000	1.23.2.18
+++ include/asm-mips/serial.h	29 Jun 2004 14:56:06 -0000
@@ -184,6 +184,18 @@
 #define TXX927_SERIAL_PORT_DEFNS
 #endif
 
+#ifdef CONFIG_MIPS_MALTA
+#define MALTA_SERIAL_PORT_DEFNS			\
+	/* UART CLK   PORT IRQ     FLAGS        */			\
+	{ 0, BASE_BAUD, 0x3F8, 4, STD_COM_FLAGS },	/* ttyS0 */	\
+	{ 0, BASE_BAUD, 0x2F8, 3, STD_COM_FLAGS },	/* ttyS1 */	\
+	{ ...  extra Malta blurb goes here },		/* ttyS2 */	\
+	{ ...  extra Malta blurb goes here },		/* ttyS3 */	\
+
+#else /* CONFIG_MIPS_MALTA */
+#define MALTA_SERIAL_PORT_DEFNS
+#endif /* CONFIG_MIPS_MALTA */
+
 #ifdef CONFIG_HAVE_STD_PC_SERIAL_PORT
 #define STD_SERIAL_PORT_DEFNS			\
 	/* UART CLK   PORT IRQ     FLAGS        */			\
@@ -192,6 +204,10 @@
 	{ 0, BASE_BAUD, 0x3E8, 4, STD_COM_FLAGS },	/* ttyS2 */	\
 	{ 0, BASE_BAUD, 0x2E8, 3, STD_COM4_FLAGS },	/* ttyS3 */
 
+#else /* CONFIG_HAVE_STD_PC_SERIAL_PORTS */
+#define STD_SERIAL_PORT_DEFNS
+#endif /* CONFIG_HAVE_STD_PC_SERIAL_PORTS */
+
 #ifdef CONFIG_SERIAL_MANY_PORTS
 #define EXTRA_SERIAL_PORT_DEFNS			\
 	{ 0, BASE_BAUD, 0x1A0, 9, FOURPORT_FLAGS }, 	/* ttyS4 */	\
@@ -226,11 +242,6 @@
 #define EXTRA_SERIAL_PORT_DEFNS
 #endif /* CONFIG_SERIAL_MANY_PORTS */
 
-#else /* CONFIG_HAVE_STD_PC_SERIAL_PORTS */
-#define STD_SERIAL_PORT_DEFNS
-#define EXTRA_SERIAL_PORT_DEFNS
-#endif /* CONFIG_HAVE_STD_PC_SERIAL_PORTS */
-
 /* You can have up to four HUB6's in the system, but I've only
  * included two cards here for a total of twelve ports.
  */
@@ -416,18 +427,20 @@
 	COBALT_SERIAL_PORT_DEFNS		\
 	DDB5477_SERIAL_PORT_DEFNS		\
 	EV96100_SERIAL_PORT_DEFNS		\
-	EXTRA_SERIAL_PORT_DEFNS			\
-	HUB6_SERIAL_PORT_DFNS			\
 	ITE_SERIAL_PORT_DEFNS           	\
 	IVR_SERIAL_PORT_DEFNS           	\
 	JAZZ_SERIAL_PORT_DEFNS			\
+	MALTA_SERIAL_PORT_DEFNS			\
 	MOMENCO_OCELOT_SERIAL_PORT_DEFNS	\
 	MOMENCO_OCELOT_G_SERIAL_PORT_DEFNS	\
 	MOMENCO_OCELOT_C_SERIAL_PORT_DEFNS	\
 	MOMENCO_JAGUAR_ATX_SERIAL_PORT_DEFNS	\
 	SEAD_SERIAL_PORT_DEFNS			\
-	STD_SERIAL_PORT_DEFNS			\
 	TITAN_SERIAL_PORT_DEFNS			\
-	TXX927_SERIAL_PORT_DEFNS
+	TXX927_SERIAL_PORT_DEFNS		\
+						\
+	STD_SERIAL_PORT_DEFNS			\
+	EXTRA_SERIAL_PORT_DEFNS			\
+	HUB6_SERIAL_PORT_DFNS			\

---

From: Jun Sun <[email-masked]>
Date: Tue, 29 Jun 2004 15:13:13 -0700

>  Then we need to find a way to make the order configurable somehow.

This is why I favor run-time serial port configuration.  My view
(maybe a little dramatic) is to remove all static serial port definition
and push them into board setup routine.  asm/serial.h only needs
to define the number serial lines, which itself could be configurable.

Jun

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Wed, 30 Jun 2004 00:43:47 +0200 (CEST)

> This is why I favor run-time serial port configuration.  My view

 Well, that's certainly a reasonable long-time strategy.

> and push them into board setup routine.  asm/serial.h only needs

 I'm not sure that is the right way of doing it -- note that one problem
is serial drivers can be built as modules and inserted at the run time.

 I haven't looked into the serial I/O subsystem of 2.6, yet, so I don't
know if it offers any support for different wirings of the same U(S)ART.  
In theory I think the most reasonable approach it would be to provide
system-specific frontends to a generic backend for a given U(S)ART (like
an 8250-compatible).  The frontends would handle address mapping, DMA if
available, etc. and be a way to deal with system-specific quirks.

> to define the number serial lines, which itself could be configurable.

 I don't think there needs to be any arbitrary limit here.  With hot-plug
PCI and similar setups ports can appear and disappear from a system at any
time, so the associated resources should be allocated dynamically anyway.

  Maciej

---

From: Ralf Baechle <[email-masked]>
Date: Wed, 30 Jun 2004 00:49:32 +0200

> to define the number serial lines, which itself could be configurable.

<asm/serial.h> is on it's way out of the kernel - it's only a question of
time until either the current maintainer of the serial driver or somebody
with more time at hands will eleminate it.  And serial.h was always only
meant to handle the kind of serial interfaces of which you just have to
know that they're there because probing for it isn't possible.  Something
which these days is getting increasingly more rare thanks to PCI.

What I really wouldn't like to see is the runtime registration for all
the legacy serial stuff that possibly could be plugged into some board
be duplicated into half a dozen of systems ...

  Ralf

---

From: Jun Sun <[email-masked]>
Date: Tue, 29 Jun 2004 17:15:37 -0700

> be duplicated into half a dozen of systems ...

No fear really.  You can still provide STD_SERIAL_PORT in the asm/serial.h
where each individual board simply does a registration for each line
defined there.  You might even provide some inline function for 
doing so in asm/serial.h.

The big advantage of this scheme is that the board-level complexity is not 
exposed to MIPS arch layer.  So when it is time for a board to die, one
does not have to clean up a dozen or so common files like asm/serial.h file.

Of course it also offers complete control over the ordering of serial
ports to the board.

See arch/mips/vr4181/common/serial.c for a simple example of run-time
registeration.  I believe a couple of other boards are doing this too.

Jun

---

From: Geert Uytterhoeven <[email-masked]>
Date: Wed, 30 Jun 2004 10:07:11 +0200 (MEST)

> is serial drivers can be built as modules and inserted at the run time.

The same is true for whatever other type of device (SCSI, IDE, Ethernet, ...).
And depending on the order of module loading, the order of the devices will
change.

Gr{oetje,eeting}s,

						Geert

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Wed, 30 Jun 2004 14:10:43 +0200 (CEST)

> The same is true for whatever other type of device (SCSI, IDE, Ethernet, ...).

 Yes and sensible drivers handle it correctly.  A classical example is the 
8390.c Ethernet driver with its various frontends.

> And depending on the order of module loading, the order of the devices will

 And given the order is fully configurable, it's actually a good thing.

  Maciej