RM9000 serial driver

This patch adds support for the integrated serial ports of the MIPS RM9122 processor and its relatives. While the hardware manual claims the serial port hardware to be 16550 compatible, the differences are in fact rather large, the biggest of them being that all register accesses must be 32-bit wide.

Signed-off-by Thomas Koeller <[email protected]>

---
 drivers/serial/8250.c       |   14 +++++++++++--
 drivers/serial/Kconfig      |    6 +++++
 include/linux/serial_core.h |    3 ++-
 include/linux/serial_reg.h  |   48 
+++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 68 insertions(+), 3 deletions(-)

drivers/serial/8250.c +12 -2

index 0ae9ced..b89de8b 100644
@@ -251,6 +251,13 @@ static const struct serial8250_config ua
 		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
 		.flags		= UART_CAP_FIFO | UART_CAP_UUE,
 	},
+	[PORT_RM9000] = {
+		.name		= "RM9000",
+		.fifo_size	= 16,
+		.tx_loadsz	= 16,
+		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
+		.flags		= UART_CAP_FIFO,
+	},
 };
 
 #ifdef CONFIG_SERIAL_8250_AU1X00

@@ -1138,8 +1145,11 @@ static void serial8250_start_tx(struct u
 		if (up->bugs & UART_BUG_TXEN) {
 			unsigned char lsr, iir;
 			lsr = serial_in(up, UART_LSR);
-			iir = serial_in(up, UART_IIR);
-			if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT)
+			iir = serial_in(up, UART_IIR) & 0x0f;
+			if ((up->port.type == PORT_RM9000) ?
+			   	(lsr & UART_LSR_THRE &&
+				(iir == UART_IIR_NO_INT || iir == UART_IIR_THRI)) :
+				(lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT))
 				transmit_chars(up);
 		}
 	}

drivers/serial/Kconfig +6 -0

index 9799cce..6ae58ba 100644
@@ -979,3 +979,9 @@ config SERIAL_NETX_CONSOLE
 	  CPU you can make it the console by answering Y to this option.
 
 endmenu
+
+config SERIAL_RM9000
+	bool "MIPS RM9000 integrated serial port support"
+	help
+	  Provide support for the integrated serial ports found on
+	  MIPS RM912x processors.

include/linux/serial_core.h +2 -1

index 86501a3..8a97caf 100644
@@ -39,7 +39,8 @@ #define PORT_16850	12
 #define PORT_RSA	13
 #define PORT_NS16550A	14
 #define PORT_XSCALE	15
-#define PORT_MAX_8250	15	/* max port ID */
+#define PORT_RM9000	16	/* PMC-Sierra RM9xxx internal UART */
+#define PORT_MAX_8250	16	/* max port ID */
 
 /*
  * ARM specific type numbers.  These are not currently guaranteed

include/linux/serial_reg.h +44 -0

index 3c8a6aa..c5c991a 100644
@@ -14,13 +14,24 @@
 #ifndef _LINUX_SERIAL_REG_H
 #define _LINUX_SERIAL_REG_H
 
+#include <linux/config.h>
+
 /*
  * DLAB=0
  */
+#if defined(CONFIG_SERIAL_RM9000)
+#define UART_RX		0x00	/* In:  Receive buffer*/
+#define UART_TX		0x04	/* Out: Transmit buffer*/
+#else
 #define UART_RX		0	/* In:  Receive buffer */
 #define UART_TX		0	/* Out: Transmit buffer */
+#endif
 
+#if defined(CONFIG_SERIAL_RM9000)
+#define UART_IER	0x0c	/* Out: Interrupt Enable Register */
+#else
 #define UART_IER	1	/* Out: Interrupt Enable Register */
+#endif
 #define UART_IER_MSI		0x08 /* Enable Modem status interrupt */
 #define UART_IER_RLSI		0x04 /* Enable receiver line status interrupt */
 #define UART_IER_THRI		0x02 /* Enable Transmitter holding register int. */

@@ -30,7 +41,11 @@ #define UART_IER_RDI		0x01 /* Enable rec
  */
 #define UART_IERX_SLEEP		0x10 /* Enable sleep mode */
 
+#if defined(CONFIG_SERIAL_RM9000)
+#define UART_IIR	0x14	/* In:  Interrupt ID Register */
+#else
 #define UART_IIR	2	/* In:  Interrupt ID Register */
+#endif
 #define UART_IIR_NO_INT		0x01 /* No interrupts pending */
 #define UART_IIR_ID		0x06 /* Mask for the interrupt ID */
 #define UART_IIR_MSI		0x00 /* Modem status interrupt */

@@ -38,7 +53,11 @@ #define UART_IIR_THRI		0x02 /* Transmitt
 #define UART_IIR_RDI		0x04 /* Receiver data interrupt */
 #define UART_IIR_RLSI		0x06 /* Receiver line status interrupt */
 
+#if defined(CONFIG_SERIAL_RM9000)
+#define UART_FCR	0x18	/* Out: FIFO Control Register */
+#else
 #define UART_FCR	2	/* Out: FIFO Control Register */
+#endif
 #define UART_FCR_ENABLE_FIFO	0x01 /* Enable the FIFO */
 #define UART_FCR_CLEAR_RCVR	0x02 /* Clear the RCVR FIFO */
 #define UART_FCR_CLEAR_XMIT	0x04 /* Clear the XMIT FIFO */

@@ -81,7 +100,11 @@ #define UART_FCR6_T_TRIGGER_24  0x20 /* 
 #define UART_FCR6_T_TRIGGER_30	0x30 /* Mask for transmit trigger set at 30 */
 #define UART_FCR7_64BYTE	0x20 /* Go into 64 byte mode (TI16C750) */
 
+#if defined(CONFIG_SERIAL_RM9000)
+#define UART_LCR	0x1c	/* Out: Line Control Register */
+#else
 #define UART_LCR	3	/* Out: Line Control Register */
+#endif
 /*
  * Note: if the word length is 5 bits (UART_LCR_WLEN5), then setting 
  * UART_LCR_STOP will select 1.5 stop bits, not 2 stop bits.

@@ -97,7 +120,11 @@ #define UART_LCR_WLEN6		0x01 /* Wordleng
 #define UART_LCR_WLEN7		0x02 /* Wordlength: 7 bits */
 #define UART_LCR_WLEN8		0x03 /* Wordlength: 8 bits */
 
+#if defined(CONFIG_SERIAL_RM9000)
+#define UART_MCR	0x20	/* Out: Modem Control Register */
+#else
 #define UART_MCR	4	/* Out: Modem Control Register */
+#endif
 #define UART_MCR_CLKSEL		0x80 /* Divide clock by 4 (TI16C752, EFR[4]=1) */
 #define UART_MCR_TCRTLR		0x40 /* Access TCR/TLR (TI16C752, EFR[4]=1) */
 #define UART_MCR_XONANY		0x20 /* Enable Xon Any (TI16C752, EFR[4]=1) */

@@ -108,7 +135,11 @@ #define UART_MCR_OUT1		0x04 /* Out1 comp
 #define UART_MCR_RTS		0x02 /* RTS complement */
 #define UART_MCR_DTR		0x01 /* DTR complement */
 
+#if defined(CONFIG_SERIAL_RM9000)
+#define UART_LSR	0x24	/* In:  Line Status Register */
+#else
 #define UART_LSR	5	/* In:  Line Status Register */
+#endif
 #define UART_LSR_TEMT		0x40 /* Transmitter empty */
 #define UART_LSR_THRE		0x20 /* Transmit-hold-register empty */
 #define UART_LSR_BI		0x10 /* Break interrupt indicator */

@@ -117,7 +148,11 @@ #define UART_LSR_PE		0x04 /* Parity erro
 #define UART_LSR_OE		0x02 /* Overrun error indicator */
 #define UART_LSR_DR		0x01 /* Receiver data ready */
 
+#if defined(CONFIG_SERIAL_RM9000)
+#define UART_MSR	0x28	/* In:  Modem Status Register */
+#else
 #define UART_MSR	6	/* In:  Modem Status Register */
+#endif
 #define UART_MSR_DCD		0x80 /* Data Carrier Detect */
 #define UART_MSR_RI		0x40 /* Ring Indicator */
 #define UART_MSR_DSR		0x20 /* Data Set Ready */

@@ -128,18 +163,31 @@ #define UART_MSR_DDSR		0x02 /* Delta DSR
 #define UART_MSR_DCTS		0x01 /* Delta CTS */
 #define UART_MSR_ANY_DELTA	0x0F /* Any of the delta bits! */
 
+#if defined(CONFIG_SERIAL_RM9000)
+#define UART_SCR	0x2c	/* I/O: Scratch Register */
+#else
 #define UART_SCR	7	/* I/O: Scratch Register */
+#endif
 
 /*
  * DLAB=1
  */
+#if defined(CONFIG_SERIAL_RM9000)
+#define UART_DLL	0x08	/* Out: Divisor Latch Low */
+#define UART_DLM	0x10	/* Out: Divisor Latch High */
+#else
 #define UART_DLL	0	/* Out: Divisor Latch Low */
 #define UART_DLM	1	/* Out: Divisor Latch High */
+#endif
 
 /*

I highly doubt this is how it should be done. Look at the Alchemy code
as an example how the "partly-compatible" UART support should be written. Alchemy also has 32-bit MMIO and some registers mapped differently.

WBR, Sergei

Quoted 5 lines · Sergei Shtylyov

I highly doubt this is how it should be done. Look at the Alchemy code
as an example how the "partly-compatible" UART support should be written. Alchemy also has 32-bit MMIO and some registers mapped differently.

WBR, Sergei

Seems I cannot find the code you are referring to - could you point me at the particular file(s) to look at?

Thanks,
Thomas

I highly doubt this is how it should be done. Look at the Alchemy code
as an example how the "partly-compatible" UART support should be written. Alchemy also has 32-bit MMIO and some registers mapped differently.

drivers/serial/8250.c +8 -1

index 0ae9ced..c6c28ed 100644
@@ -251,9 +251,16 @@ static const struct serial8250_config ua
 		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
 		.flags		= UART_CAP_FIFO | UART_CAP_UUE,
 	},
+	[PORT_RM9000] = {
+		.name		= "RM9000",
+		.fifo_size	= 16,
+		.tx_loadsz	= 16,
+		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
+		.flags		= UART_CAP_FIFO,
+	},
 };
 
-#ifdef CONFIG_SERIAL_8250_AU1X00
+#if defined (CONFIG_SERIAL_8250_AU1X00)
 
 /* Au1x00 UART hardware has a weird register layout */
 static const u8 au_io_in_map[] = {

drivers/serial/8250.c +36 -5

+
+static const u8
+	regmap_in[8] = {
+		[UART_RX]	= 0x00,
+		[UART_IER]	= 0x0c,
+		[UART_IIR]	= 0x14,
+		[UART_LCR]	= 0x1c,
+		[UART_MCR]	= 0x20,
+		[UART_LSR]	= 0x24,
+		[UART_MSR]	= 0x28,
+		[UART_SCR]	= 0x2c
+	},
+	regmap_out[8] = {
+		[UART_TX] 	= 0x04,
+		[UART_IER]	= 0x0c,
+		[UART_FCR]	= 0x18,
+		[UART_LCR]	= 0x1c,
+		[UART_MCR]	= 0x20,
+		[UART_LSR]	= 0x24,
+		[UART_MSR]	= 0x28,
+		[UART_SCR]	= 0x2c
+	};
+
+#define map_8250_in_reg(up, offset) (regmap_in[offset])
+#define map_8250_out_reg(up, offset) (regmap_out[offset])
+
+
 #else
 
 /* sane hardware needs no mapping */

@@ -374,21 +409,21 @@ #define serial_inp(up, offset)		serial_i
 #define serial_outp(up, offset, value)	serial_out(up, offset, value)
 
 /* Uart divisor latch read */
-static inline int _serial_dl_read(struct uart_8250_port *up)
+static inline unsigned int _serial_dl_read(struct uart_8250_port *up)
 {
 	return serial_inp(up, UART_DLL) | serial_inp(up, UART_DLM) << 8;
 }
 
 /* Uart divisor latch write */
-static inline void _serial_dl_write(struct uart_8250_port *up, int value)
+static inline void _serial_dl_write(struct uart_8250_port *up, unsigned int value)
 {
 	serial_outp(up, UART_DLL, value & 0xff);
 	serial_outp(up, UART_DLM, value >> 8 & 0xff);
 }
 
-#ifdef CONFIG_SERIAL_8250_AU1X00
+#if defined (CONFIG_SERIAL_8250_AU1X00)
 /* Au1x00 haven't got a standard divisor latch */
-static int serial_dl_read(struct uart_8250_port *up)
+static unsigned int serial_dl_read(struct uart_8250_port *up)
 {
 	if (up->port.iotype == UPIO_AU)
 		return __raw_readl(up->port.membase + 0x28);

@@ -396,13 +431,26 @@ static int serial_dl_read(struct uart_82
 		return _serial_dl_read(up);
 }
 
-static void serial_dl_write(struct uart_8250_port *up, int value)
+static void serial_dl_write(struct uart_8250_port *up, unsigned int value)
 {
 	if (up->port.iotype == UPIO_AU)
 		__raw_writel(value, up->port.membase + 0x28);
 	else
 		_serial_dl_write(up, value);
 }
+#elif defined (CONFIG_SERIAL_RM9000)
+static inline unsigned int serial_dl_read(struct uart_8250_port *up)
+{
+	return

If you have an another standard 8250 port. this driver cannot support it You should do as well as AU1X00.

Yoichi

Quoted 2 lines

If you have an another standard 8250 port. this driver cannot support it You should do as well as AU1X00.

The AU1X00 code obviously assumes that every port that is not an AU1X00 is a standard port requiring no register mapping. However, this is of course not necessarily true in the most general case. There could be platforms with multiple ports, all non-standard, but in different ways. Handling this

would require per-port mapping functions, which could be achieved by adding function pointers to struct uart_8250_port. However, this would add the overhead of a real, non-inlined function call to every register access.

Also, it seems to me that the whole register-mapping stuff conflicts with autodetection, because autoconfig() uses serial_inp() and serial_outp() before the port types, and hence the mapping requirements, are known.

This is not a problem for me, however, since the correct port type is set up by the platform using early_serial_setup().

Quoted 3 lines

Also, it seems to me that the whole register-mapping stuff conflicts with autodetection, because autoconfig() uses serial_inp() and serial_outp() before the port types, and hence the mapping requirements, are known.

Port types have nothing to do with this. Or at least they hadn't until
your recent patch. :-)
iotype was used to identify the addressing scheme, and it's alsready
known beforehand.

Quoted 8 lines

Quoted 3 lines

Also, it seems to me that the whole register-mapping stuff conflicts with autodetection, because autoconfig() uses serial_inp() and serial_outp() before the port types, and hence the mapping requirements, are known.

Port types have nothing to do with this. Or at least they hadn't until
your recent patch. :-)
iotype was used to identify the addressing scheme, and it's alsready
known beforehand.

How so? If I do not yet know which hardware I am dealing with, how can I know the iotype?

Thomas

If you have an another standard 8250 port. this driver cannot support it You should do as well as AU1X00.

Yoichi

On Tuesday 22 August 2006 02:59, Yoichi Yuasa wrote:
Hi Yoichi,

so far nobody commented on my recent mail, in which I explained why I
think that the AU1X00 code in 8250.c is not entirely correct, so I assume
nobody cares. I therefore modified my code to take the same approach,
although I still have my doubts about it. Here's the updated patch:

Quoted 2 lines

If you have an another standard 8250 port. this driver cannot support it You should do as well as AU1X00.

so far nobody commented on my recent mail, in which I explained why I think that the AU1X00 code in 8250.c is not entirely correct, so I assume nobody cares. I therefore modified my code to take the same approach,

although I still have my doubts about it. Here's the updated patch:

Signed-off-by Thomas Koeller <[email protected]>

---
832ac1406f2530b7971cb0d23d3ede20a6057fa1

 drivers/serial/8250.c       |   86 ++++++++++++++++++++++++++++++++++---------
 drivers/serial/Kconfig      |    9 +++++
 include/linux/serial_core.h |    3 +-
 3 files changed, 78 insertions(+), 20 deletions(-)

drivers/serial/8250.c +0 -0

index 0ae9ced..afe0e1f 100644
@@ -251,9 +251,16 @@ static const struct serial8250_config ua
 		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,

drivers/serial/8250.c +14 -1

-#ifdef CONFIG_SERIAL_8250_AU1X00
+#if defined (CONFIG_SERIAL_8250_AU1X00)
 
 /* Au1x00 UART hardware has a weird register layout */
 static const u8 au_io_in_map[] = {

@@ -289,6 +296,36 @@ static inline int map_8250_out_reg(struc
 	return au_io_out_map[offset];
 }
 
+#elif defined (CONFIG_SERIAL_8250_RM9K)
+
+static const u8
+	regmap_in[8] = {
+		[UART_RX]	= 0x00,
+		[UART_IER]	= 0x0c,
+		[UART_IIR]	= 0x14,
+		[UART_LCR]	= 0x1c,
+		[UART_MCR]	= 0x20,
+		[UART_LSR]	= 0x24,
+		[UART_MSR]	= 0x28,
+		[UART_SCR]	= 0x2c
+	},

drivers/serial/8250.c +8 -5

 #else

@@ -374,21 +411,21 @@ #define serial_inp(up, offset)		serial_i
 #define serial_outp(up, offset, value)	serial_out(up, offset, value)
 
 /* Uart divisor latch read */
-static inline int _serial_dl_read(struct uart_8250_port *up)
+static inline unsigned int _serial_dl_read(struct uart_8250_port *up)
 {
 	return serial_inp(up, UART_DLL) | serial_inp(up, UART_DLM) << 8;
 }
 
 /* Uart divisor latch write */
-static inline void _serial_dl_write(struct uart_8250_port *up, int value)
+static inline void _serial_dl_write(struct uart_8250_port *up, unsigned int value)
 {
 	serial_outp(up, UART_DLL, value & 0xff);
 	serial_outp(up, UART_DLM, value >> 8 & 0xff);
 }
 
-#ifdef CONFIG_SERIAL_8250_AU1X00
+#if defined (CONFIG_SERIAL_8250_AU1X00)
 /* Au1x00 haven't got a standard divisor latch */
-static int serial_dl_read(struct uart_8250_port *up)
+static unsigned int serial_dl_read(struct uart_8250_port *up)
 {
 	if (up->port.iotype == UPIO_AU)
 		return __raw_readl(up->port.membase + 0x28);

@@ -396,13 +433,26 @@ static int serial_dl_read(struct uart_82
 		return _serial_dl_read(up);
 }
 
-static void serial_dl_write(struct uart_8250_port *up, int value)
+static void serial_dl_write(struct uart_8250_port *up, unsigned int value)
 {
 	if (up->port.iotype == UPIO_AU)
 		__raw_writel(value, up->port.membase + 0x28);
 	else
 		_serial_dl_write(up, value);
 }
+#elif defined (CONFIG_SERIAL_8250_RM9K)
+static inline unsigned int serial_dl_read(struct uart_8250_port *up)
+{

drivers/serial/8250.c +3 -8

@@ -576,22 +626,17 @@ static int size_fifo(struct uart_8250_po
  */
 static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
 {
-	unsigned char old_dll, old_dlm, old_lcr;
-	unsigned int id;
+	unsigned char old_lcr;
+	unsigned int id, old_dl;
 
 	old_lcr = serial_inp(p, UART_LCR);
 	serial_outp(p, UART_LCR, UART_LCR_DLAB);
+	old_dl = _serial_dl_read(p);
 
-	old_dll = serial_inp(p, UART_DLL);
-	old_dlm = serial_inp(p, UART_DLM);
-
-	serial_outp(p, UART_DLL, 0);
-	serial_outp(p, UART_DLM, 0);
-

drivers/serial/8250.c +0 -0

@@ -1138,8 +1183,11 @@ static void serial8250_start_tx(struct u
 		if (up->bugs & UART_BUG_TXEN) {
 			unsigned char lsr, iir;
 			lsr = serial_in(up, UART_LSR);

Quoted 28 lines

drivers/serial/8250.c +6 -11

@@ -576,22 +626,17 @@ static int size_fifo(struct uart_8250_po
 */
 static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
 {
-	unsigned char old_dll, old_dlm, old_lcr;
-	unsigned int id;
+	unsigned char old_lcr;
+	unsigned int id, old_dl;
 
 	old_lcr = serial_inp(p, UART_LCR);
 	serial_outp(p, UART_LCR, UART_LCR_DLAB);
+	old_dl = _serial_dl_read(p);
 
-	old_dll = serial_inp(p, UART_DLL);
-	old_dlm = serial_inp(p, UART_DLM);
-
-	serial_outp(p, UART_DLL, 0);
-	serial_outp(p, UART_DLM, 0);
-
-	id = serial_inp(p, UART_DLL) | serial_inp(p, UART_DLM) << 8;
+	serial_dl_write(p, 0);
+	id = serial_dl_read(p);
 
-	serial_outp(p, UART_DLL, old_dll);
-	serial_outp(p, UART_DLM, old_dlm);
+	serial_dl_write(p, old_dl);
 	serial_outp(p, UART_LCR, old_lcr);
 

drivers/serial/8250.c static int size_fifo(struct uart_8250_po · line 642

Quoted 1 line

drivers/serial/8250.c +0 -0

 	return id;

Not sure the autoconfig code was intended for half-compatible UARTs. Note that it sets up->port.type as its result. However, your change seems
correct, it just have nothing to do with RM9000.

As a side note, I think that the code that sets DLAB before and resets it
after the divisor latch read/write should be part of serial_dl_read() and serial_dl_write() actually. In the Alchemy UARTs this bit is reserved.

Quoted 28 lines

Quoted 28 lines

drivers/serial/8250.c +6 -11

@@ -576,22 +626,17 @@ static int size_fifo(struct uart_8250_po
 */
 static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
 {
-	unsigned char old_dll, old_dlm, old_lcr;
-	unsigned int id;
+	unsigned char old_lcr;
+	unsigned int id, old_dl;
 
 	old_lcr = serial_inp(p, UART_LCR);
 	serial_outp(p, UART_LCR, UART_LCR_DLAB);
+	old_dl = _serial_dl_read(p);
 
-	old_dll = serial_inp(p, UART_DLL);
-	old_dlm = serial_inp(p, UART_DLM);
-
-	serial_outp(p, UART_DLL, 0);
-	serial_outp(p, UART_DLM, 0);
-
-	id = serial_inp(p, UART_DLL) | serial_inp(p, UART_DLM) << 8;
+	serial_dl_write(p, 0);
+	id = serial_dl_read(p);
 
-	serial_outp(p, UART_DLL, old_dll);
-	serial_outp(p, UART_DLM, old_dlm);
+	serial_dl_write(p, old_dl);
 	serial_outp(p, UART_LCR, old_lcr);
 

Quoted 5 lines

drivers/serial/8250.c static int size_fifo(struct uart_8250_po · line 642

Quoted 1 line

drivers/serial/8250.c +0 -0

 	return id;

Not sure the autoconfig code was intended for half-compatible UARTs. Note that it sets up->port.type as its result. However, your change seems
correct, it just have nothing to do with RM9000.

It's worse than that - this code is there to read the ID from the divisor registers implemented in some UARTs. If it isn't one of those UARTs, it's expected to return zero.

So we don't actually want to be prodding some other random registers on differing UARTs.

Quoted 3 lines

As a side note, I think that the code that sets DLAB before and resets it
after the divisor latch read/write should be part of serial_dl_read() and serial_dl_write() actually. In the Alchemy UARTs this bit is reserved.

Not really, for two reasons.

1. We end up with additional pointless writes to undo what serial_dl_* did.

2. setting DLAB might work for a subset of ports, but others require different magic numbers written to LCR to access the divisor.

3. other ports have additional properties when DLAB is set, to the extent that you must not write other registers when it's reset to avoid clearing some features you want to enable.

So, really, Moving that stuff into serial_dl_* ends up adding additional code and complexity where it isn't needed.

Quoted 7 lines

Quoted 3 lines

Not sure the autoconfig code was intended for half-compatible UARTs. Note that it sets up->port.type as its result. However, your change seems correct, it just have nothing to do with RM9000.

It's worse than that - this code is there to read the ID from the divisor registers implemented in some UARTs. If it isn't one of those UARTs, it's expected to return zero.

Well, I guess it should still return 0 (or revision) if we use serial_dl_*()?

Quoted 9 lines

Quoted 4 lines

As a side note, I think that the code that sets DLAB before and resets it
after the divisor latch read/write should be part of serial_dl_read() and serial_dl_write() actually. In the Alchemy UARTs this bit is reserved.

Not really, for two reasons.

1. We end up with additional pointless writes to undo what serial_dl_* did.

Yes, sometimes.

Quoted 2 lines

2. setting DLAB might work for a subset of ports, but others require different magic numbers written to LCR to access the divisor.

Indeed, I've spotted one such case. But we could possible RMW the line
control reg. so that serial_dl_*() "cleanup" after themselves?

Quoted 6 lines

3. other ports have additional properties when DLAB is set, to the extent that you must not write other registers when it's reset to avoid clearing some features you want to enable.

So, really, Moving that stuff into serial_dl_* ends up adding additional code and complexity where it isn't needed.

Well, alternatively, the checks might be added to the places where DLAB is written preventing the write for UARTs that don't have the bit. Or even such check and LCR masking or even write skipping might be added to serial_out()?

It's worse than that - this code is there to read the ID from the divisor registers implemented in some UARTs. If it isn't one of those UARTs, it's expected to return zero.

So we don't actually want to be prodding some other random registers on differing UARTs.

drivers/serial/8250.c static const struct serial8250_config ua · line 261

Quoted 8 lines

drivers/serial/8250.c +7 -0

+	[PORT_RM9000] = {
+		.name		= "RM9000",
+		.fifo_size	= 16,
+		.tx_loadsz	= 16,
+		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
+		.flags		= UART_CAP_FIFO,
+	},
 };

What was the point of introducing the separate port type if its
settings are the same as for PORT_16550A?

Quoted 27 lines

drivers/serial/8250.c +18 -1

-#ifdef CONFIG_SERIAL_8250_AU1X00
+#if defined (CONFIG_SERIAL_8250_AU1X00)
 
 /* Au1x00 UART hardware has a weird register layout */
 static const u8 au_io_in_map[] = {

@@ -289,6 +296,36 @@ static inline int map_8250_out_reg(struc
 	return au_io_out_map[offset];
 }
 
+#elif defined (CONFIG_SERIAL_8250_RM9K)
+
+static const u8
+	regmap_in[8] = {
+		[UART_RX]	= 0x00,
+		[UART_IER]	= 0x0c,
+		[UART_IIR]	= 0x14,
+		[UART_LCR]	= 0x1c,
+		[UART_MCR]	= 0x20,
+		[UART_LSR]	= 0x24,
+		[UART_MSR]	= 0x28,
+		[UART_SCR]	= 0x2c
+	},
+	regmap_out[8] = {
+		[UART_TX] 	= 0x04,
+		[UART_IER]	= 0x0c,
+		[UART_FCR]	= 0x18,

drivers/serial/8250.c static inline int map_8250_out_reg(struc · line 321

Quoted 6 lines

drivers/serial/8250.c +6 -0

+		[UART_LCR]	= 0x1c,
+		[UART_MCR]	= 0x20,
+		[UART_LSR]	= 0x24,
+		[UART_MSR]	= 0x28,
+		[UART_SCR]	= 0x2c
+	};

    I guess you're using regshift == 0?

drivers/serial/8250.c static inline int map_8250_out_reg(struc · line 328

Quoted 8 lines

drivers/serial/8250.c +7 -0

+
+#define map_8250_in_reg(up, offset) \
+	(((up)->port.type == PORT_RM9000) ? regmap_in[offset] : (offset))
+#define map_8250_out_reg(up, offset) \
+	(((up)->port.type == PORT_RM9000) ? regmap_out[offset] : (offset))
+
+

Why you're not using specific iotype for RM9000 UARTs?

Quoted 43 lines

drivers/serial/8250.c +7 -5

 #else

@@ -374,21 +411,21 @@ #define serial_inp(up, offset)		serial_i
 #define serial_outp(up, offset, value)	serial_out(up, offset, value)
 
 /* Uart divisor latch read */
-static inline int _serial_dl_read(struct uart_8250_port *up)
+static inline unsigned int _serial_dl_read(struct uart_8250_port *up)
 {
 	return serial_inp(up, UART_DLL) | serial_inp(up, UART_DLM) << 8;
 }
 
 /* Uart divisor latch write */
-static inline void _serial_dl_write(struct uart_8250_port *up, int
 value) +static inline void _serial_dl_write(struct uart_8250_port *up,
 unsigned int value) {
 	serial_outp(up, UART_DLL, value & 0xff);
 	serial_outp(up, UART_DLM, value >> 8 & 0xff);
 }
 
-#ifdef CONFIG_SERIAL_8250_AU1X00
+#if defined (CONFIG_SERIAL_8250_AU1X00)
 /* Au1x00 haven't got a standard divisor latch */
-static int serial_dl_read(struct uart_8250_port *up)
+static unsigned int serial_dl_read(struct uart_8250_port *up)
 {
 	if (up->port.iotype == UPIO_AU)
 		return __raw_readl(up->port.membase + 0x28);

@@ -396,13 +433,26 @@ static int serial_dl_read(struct uart_82
 		return _serial_dl_read(up);
 }
 
-static void serial_dl_write(struct uart_8250_port *up, int value)
+static void serial_dl_write(struct uart_8250_port *up, unsigned int
 value) {
 	if (up->port.iotype == UPIO_AU)
 		__raw_writel(value, up->port.membase + 0x28);
 	else
 		_serial_dl_write(up, value);
 }
+#elif defined (CONFIG_SERIAL_8250_RM9K)
+static inline unsigned int serial_dl_read(struct uart_8250_port *up)
+{

drivers/serial/8250.c static int serial_dl_read(struct uart_82 · line 455

Quoted 10 lines

drivers/serial/8250.c +9 -0

+	return
+		((readl(up->port.membase + 0x10) << 8) |
+		(readl(up->port.membase + 0x08) & 0xff)) & 0xffff;
+}
+
+static inline void serial_dl_write(struct uart_8250_port *up, unsigned
 int value) +{
+	writel(value, up->port.membase + 0x08);
+	writel(value >> 8, up->port.membase + 0x10);
+}

    And why this doesn't check for up->port.type == PORT_RM9000 first? This

way it won't work with any compatible UARTs anymore. This is wrong.

Quoted 29 lines

drivers/serial/8250.c +6 -11

@@ -576,22 +626,17 @@ static int size_fifo(struct uart_8250_po
  */
 static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
 {
-	unsigned char old_dll, old_dlm, old_lcr;
-	unsigned int id;
+	unsigned char old_lcr;
+	unsigned int id, old_dl;
 
 	old_lcr = serial_inp(p, UART_LCR);
 	serial_outp(p, UART_LCR, UART_LCR_DLAB);
+	old_dl = _serial_dl_read(p);
 
-	old_dll = serial_inp(p, UART_DLL);
-	old_dlm = serial_inp(p, UART_DLM);
-
-	serial_outp(p, UART_DLL, 0);
-	serial_outp(p, UART_DLM, 0);
-
-	id = serial_inp(p, UART_DLL) | serial_inp(p, UART_DLM) << 8;
+	serial_dl_write(p, 0);
+	id = serial_dl_read(p);
 
-	serial_outp(p, UART_DLL, old_dll);
-	serial_outp(p, UART_DLM, old_dlm);
+	serial_dl_write(p, old_dl);
 	serial_outp(p, UART_LCR, old_lcr);
 

drivers/serial/8250.c static int size_fifo(struct uart_8250_po · line 642

Quoted 1 line

drivers/serial/8250.c +0 -0

 	return id;

Not sure the autoconfig code was intended for half-compatible UARTs.
Note that it sets up->port.type as its result. However, your change seems correct, it just have nothing to do with RM9000.

As a side note, I think that the code that sets DLAB before and resets
it after the divisor latch read/write should be part of serial_dl_read() and serial_dl_write() actually. In the Alchemy UARTs this bit is reserved.

Quoted 4 lines

drivers/serial/8250.c +0 -0

@@ -1138,8 +1183,11 @@ static void serial8250_start_tx(struct u
 		if (up->bugs & UART_BUG_TXEN) {
 			unsigned char lsr, iir;
 			lsr = serial_in(up, UART_LSR);

drivers/serial/8250.c static void serial8250_start_tx(struct u · line 1193

Quoted 10 lines

drivers/serial/8250.c +5 -2

-			iir = serial_in(up, UART_IIR);
-			if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT)
+			iir = serial_in(up, UART_IIR) & 0x0f;
+			if ((up->port.type == PORT_RM9000) ?
+			   	(lsr & UART_LSR_THRE &&
+				(iir == UART_IIR_NO_INT || iir == UART_IIR_THRI)) :
+				(lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT))
 				transmit_chars(up);
 		}
 	}

It would be good to clarify why this is needed...

I would like to return to the port type vs. iotype stuff once again. From what you wrote I seem to understand that the iotype is not just a method of accessing device registers, but also the primary means of discrimination between different h/w implementations, and hence every code to support a nonstandard device must define an iotype of its own, even though one of the existing iotypes would work just fine?

Quoted 6 lines

I would like to return to the port type vs. iotype stuff once again. From what you wrote I seem to understand that the iotype is not just a method of accessing device registers, but also the primary means of discrimination between different h/w implementations, and hence every code to support a nonstandard device must define an iotype of its own, even though one of the existing iotypes would work just fine?

iotype is all about the access method used to access the registers of the device, be it by byte or word, and it also takes account of any variance in the addressing of the registers.

It does not refer to features or bugs in any particular implementation.

Quoted 9 lines · Russell King

Quoted 1 line

I would like to return to the port type vs. iotype stuff once again.

From what you wrote I seem to understand that the iotype is not just

Quoted 4 lines

a method of accessing device registers, but also the primary means of discrimination between different h/w implementations, and hence every code to support a nonstandard device must define an iotype of its own, even though one of the existing iotypes would work just fine?

Quoted 5 lines

iotype is all about the access method used to access the registers of the device, be it by byte or word, and it also takes account of any variance in the addressing of the registers.

It does not refer to features or bugs in any particular implementation.

Well, the introduction of the UPIO_TSI case seems to contradict this --
it's exactly about the bugs in the particular UART implementation (otherwise well described by UPIO_MEM). Its only function was to mask 2 hardware issues. And the UUE bit workaround seems like an abuse to me. The driver could just skip the UUE test altogether based on iotype == UPIO_TSI (or at least not to ignore writes with UUE set completely like it does but just mask off UUE bit). With no provision to pass the implicit UART type for platform devices (and skip the autoconfiguation), the introduction of UPIO_TSI seems again the necessary evil. Otherwise, we could have this handled with a distinct TSI UART type...

WBR, Sergei

It's worse than that - this code is there to read the ID from the divisor registers implemented in some UARTs. If it isn't one of those UARTs, it's expected to return zero.

Quoted 25 lines

Quoted 9 lines

Quoted 1 line

I would like to return to the port type vs. iotype stuff once again.

From what you wrote I seem to understand that the iotype is not just

Quoted 4 lines

a method of accessing device registers, but also the primary means of discrimination between different h/w implementations, and hence every code to support a nonstandard device must define an iotype of its own, even though one of the existing iotypes would work just fine?

Quoted 5 lines

iotype is all about the access method used to access the registers of the device, be it by byte or word, and it also takes account of any variance in the addressing of the registers.

It does not refer to features or bugs in any particular implementation.

Well, the introduction of the UPIO_TSI case seems to contradict this --
it's exactly about the bugs in the particular UART implementation (otherwise well described by UPIO_MEM). Its only function was to mask 2 hardware issues. And the UUE bit workaround seems like an abuse to me. The driver could just skip the UUE test altogether based on iotype == UPIO_TSI (or at least not to ignore writes with UUE set completely like it does but just mask off UUE bit). With no provision to pass the implicit UART type for platform devices (and skip the autoconfiguation), the introduction of UPIO_TSI seems again the necessary evil. Otherwise, we could have this handled with a distinct TSI UART type...

Quoted 1 line

WBR, Sergei

after a long delay (for which I apologize) I would like to return to

this topic. Since so much time has passed since the discussion ceased, I think I should summarize its state at that point:

1. In the RM9000 serial driver patch I submitted, I had introduced a new port type PORT_RM9000. I set the iotype to UPIO_MEM32. Wherever I needed to handle any peculiarities of the RM9000 h/w, I did so based upon the port type. AFAICT this is in agreement with Russel's view as quoted above.

2. Sergei says this is wrong, I need to introduce a new iotype instead, and make the code conditional upon that, like the AU1000 does (UPIO_AU). The reason he gives (see quote above) is that there is no way to pass the port type along with a platform device. Is this argument still valid?

What about including the port type in the information attached to platform_device.platform_data?

Btw., I had a look ath the existing code dealing with platform devices. Is there a particular reason for not using the standard platform_device.resource mechanism of passing mem/io/irq resources from the platform to the driver?

Another topic discussed was the use (or abuse) of dl_serial_read()/ dl_serial_write() to get/set the divisor registers.

3. Russel says (qoute from an earlier mail),

Quoted 3 lines

It's worse than that - this code is there to read the ID from the divisor registers implemented in some UARTs. If it isn't one of those UARTs, it's expected to return zero.

4. I followed the AU1X00 code in this case, using the functions to read/write the divisor value. My hardware has its UART_DLL and UART_DLM registers at nonstandard addresses, and so the only other option would have been to monitor every write to the LCR register for setting or clearing the DLAB bit, and to switch between different mapping tables accordingly. In the

light of this, would it still be unacceptable to use the dl_*() functions tho access the divisor registers?

Once these issues have been resolved, I will submit an updated version of my patch.

tk

I'm afraid you're on your own with resolving the issues now, as the
serial drivers are no longer maintaned by anybody (so, you probably will have to work with Andrew Morton)...

iotype is all about the access method used to access the registers of the device, be it by byte or word, and it also takes account of any variance in the addressing of the registers.

It does not refer to features or bugs in any particular implementation.

Quoted 5 lines

iotype is all about the access method used to access the registers of the device, be it by byte or word, and it also takes account of any variance in the addressing of the registers.

It does not refer to features or bugs in any particular implementation.

That's what I assumed, too - it seemed obvious. And it seemed equally obvious that it is the port type that encodes the the implementation's peculiarities. Among these are the register offset mapping requirements, so I assumed these should depend on the port type as well.

Now Sergei strongly insist that it's the iotype that should be checked whenever to get to the hardware type. I still do not quite understand how that is supposed to work. If I have a PCI device, for example, then the iotype will always be either UPIO_MEM or UPIO_PORT, so how could I learn something about the hardware implementation by looking at these values?

Or is the assumption that devices on a standard bus will always be of a standard type?

Thomas

Quoted 11 lines

Quoted 8 lines

drivers/serial/8250.c +7 -0

+	[PORT_RM9000] = {
+		.name		= "RM9000",
+		.fifo_size	= 16,
+		.tx_loadsz	= 16,
+		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
+		.flags		= UART_CAP_FIFO,
+	},

};

What was the point of introducing the separate port type if its
settings are the same as for PORT_16550A?

I was under the impression that for every serial port hardware that is not exactly identical to one of the standard types, a distinct port type is required to be able to write code that takes care of its peculiarities. But from what you have written so far I conclude that this was a misconception.

Quoted 21 lines

Quoted 21 lines

drivers/serial/8250.c +17 -0

@@ -289,6 +296,36 @@ static inline int map_8250_out_reg(struc
 	return au_io_out_map[offset];
 }
 
+#elif defined (CONFIG_SERIAL_8250_RM9K)
+
+static const u8
+	regmap_in[8] = {
+		[UART_RX]	= 0x00,
+		[UART_IER]	= 0x0c,
+		[UART_IIR]	= 0x14,
+		[UART_LCR]	= 0x1c,
+		[UART_MCR]	= 0x20,
+		[UART_LSR]	= 0x24,
+		[UART_MSR]	= 0x28,
+		[UART_SCR]	= 0x2c
+	},
+	regmap_out[8] = {
+		[UART_TX] 	= 0x04,
+		[UART_IER]	= 0x0c,
+		[UART_FCR]	= 0x18,

Quoted 8 lines

drivers/serial/8250.c static inline int map_8250_out_reg(struc · line 321

Quoted 6 lines

drivers/serial/8250.c +6 -0

+		[UART_LCR]	= 0x1c,
+		[UART_MCR]	= 0x20,
+		[UART_LSR]	= 0x24,
+		[UART_MSR]	= 0x28,
+		[UART_SCR]	= 0x2c
+	};

   I guess you're using regshift == 0?

Yes.

Quoted 9 lines

drivers/serial/8250.c static inline int map_8250_out_reg(struc · line 328

Quoted 7 lines

drivers/serial/8250.c +7 -0

+
+#define map_8250_in_reg(up, offset) \
+	(((up)->port.type == PORT_RM9000) ? regmap_in[offset] : (offset))
+#define map_8250_out_reg(up, offset) \
+	(((up)->port.type == PORT_RM9000) ? regmap_out[offset] : (offset))
+
+

Why you're not using specific iotype for RM9000 UARTs?

Because I did not realize that this was necessary. The device registers are

ioremapped, and so the standard UPIO_MEM32 seemed the right thing to use. I

will return to this topic further down.

Quoted 1 line

Quoted 1 line

(quoted diff) +0 -0

@@ -374,21 +411,21 @@ #define serial_inp(up, offset)		serial_i

Quoted 28 lines

Quoted 28 lines

(quoted diff) +1 -1

-#ifdef CONFIG_SERIAL_8250_AU1X00
+#if defined (CONFIG_SERIAL_8250_AU1X00)

/* Au1x00 haven't got a standard divisor latch */

drivers/serial/8250.c +1 -1

-static int serial_dl_read(struct uart_8250_port *up)
+static unsigned int serial_dl_read(struct uart_8250_port *up)

{
	if (up->port.iotype == UPIO_AU)
		return __raw_readl(up->port.membase + 0x28);

drivers/serial/8250.c +9 -1

@@ -396,13 +433,26 @@ static int serial_dl_read(struct uart_82
 		return _serial_dl_read(up);
 }
 
-static void serial_dl_write(struct uart_8250_port *up, int value)
+static void serial_dl_write(struct uart_8250_port *up, unsigned int
 value) {
 	if (up->port.iotype == UPIO_AU)
 		__raw_writel(value, up->port.membase + 0x28);
 	else
 		_serial_dl_write(up, value);
 }
+#elif defined (CONFIG_SERIAL_8250_RM9K)
+static inline unsigned int serial_dl_read(struct uart_8250_port *up)
+{
+	return
+		((readl(up->port.membase + 0x10) << 8) |
+		(readl(up->port.membase + 0x08) & 0xff)) & 0xffff;
+}
+

Quoted 8 lines

drivers/serial/8250.c static int serial_dl_read(struct uart_82 · line 455

Quoted 5 lines

drivers/serial/8250.c +4 -0

+static inline void serial_dl_write(struct uart_8250_port *up, unsigned
 int value) +{
+	writel(value, up->port.membase + 0x08);
+	writel(value >> 8, up->port.membase + 0x10);
+}

And why this doesn't check for up->port.type == PORT_RM9000 first? This
way it won't work with any compatible UARTs anymore. This is wrong.

Because it is inside a conditional block already. I now realize that even if the driver is configured for some special silicon it still has to support the standard types, something that escaped me when I started to write the code.

Quoted 28 lines

Quoted 28 lines

drivers/serial/8250.c +6 -11

@@ -576,22 +626,17 @@ static int size_fifo(struct uart_8250_po
 */
 static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
 {
-	unsigned char old_dll, old_dlm, old_lcr;
-	unsigned int id;
+	unsigned char old_lcr;
+	unsigned int id, old_dl;
 
 	old_lcr = serial_inp(p, UART_LCR);
 	serial_outp(p, UART_LCR, UART_LCR_DLAB);
+	old_dl = _serial_dl_read(p);
 
-	old_dll = serial_inp(p, UART_DLL);
-	old_dlm = serial_inp(p, UART_DLM);
-
-	serial_outp(p, UART_DLL, 0);
-	serial_outp(p, UART_DLM, 0);
-
-	id = serial_inp(p, UART_DLL) | serial_inp(p, UART_DLM) << 8;
+	serial_dl_write(p, 0);
+	id = serial_dl_read(p);
 
-	serial_outp(p, UART_DLL, old_dll);
-	serial_outp(p, UART_DLM, old_dlm);
+	serial_dl_write(p, old_dl);
 	serial_outp(p, UART_LCR, old_lcr);
 

Quoted 5 lines

drivers/serial/8250.c static int size_fifo(struct uart_8250_po · line 642

Quoted 1 line

drivers/serial/8250.c +0 -0

 	return id;

Not sure the autoconfig code was intended for half-compatible UARTs.
Note that it sets up->port.type as its result. However, your change seems correct, it just have nothing to do with RM9000.

Should I factor out this part and create a separate patch for it?

Quoted 3 lines

As a side note, I think that the code that sets DLAB before and resets
it after the divisor latch read/write should be part of serial_dl_read() and serial_dl_write() actually. In the Alchemy UARTs this bit is reserved.

Quoted 8 lines

Quoted 8 lines

drivers/serial/8250.c +2 -2

@@ -1138,8 +1183,11 @@ static void serial8250_start_tx(struct u
 		if (up->bugs & UART_BUG_TXEN) {
 			unsigned char lsr, iir;
 			lsr = serial_in(up, UART_LSR);
-			iir = serial_in(up, UART_IIR);
-			if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT)
+			iir = serial_in(up, UART_IIR) & 0x0f;
+			if ((up->port.type == PORT_RM9000) ?

Quoted 8 lines

drivers/serial/8250.c static void serial8250_start_tx(struct u · line 1193

Quoted 6 lines

drivers/serial/8250.c +3 -0

+			   	(lsr & UART_LSR_THRE &&
+				(iir == UART_IIR_NO_INT || iir == UART_IIR_THRI)) :
+				(lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT))
 				transmit_chars(up);
 		}
 	}

It would be good to clarify why this is needed...

The RM9000 serial h/w also needs to be kicked if a transmitter holding register empty interrupt is pending. Oh, and no need to tell me, I realize that I have to deal with the standard case here as well...

I would like to return to the port type vs. iotype stuff once again. From what you wrote I seem to understand that the iotype is not just a method of accessing device registers, but also the primary means of discrimination between different h/w

implementations, and hence every code to support a nonstandard device must define an iotype of its own, even though one of the existing iotypes would work just fine? In my

case, UPIO_AU might be the best choice,

as __raw_readl() and __raw_writel() are insensitive to CONFIG_SWAP_IO_SPACE, and that is what I want.

Would I still need to invent UPIO_RM9K,

just to have a distinct iotype, and be able to do 'if (up->port.iotype == UPIO_RM9K)'

where I now use 'if (up->port.type == PORT_RM9000)'? That seems a bit weird.

Thomas

Quoted 18 lines

Quoted 18 lines

Quoted 9 lines

drivers/serial/8250.c +7 -0

+    [PORT_RM9000] = {
+        .name        = "RM9000",
+        .fifo_size    = 16,
+        .tx_loadsz    = 16,
+        .fcr        = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
+        .flags        = UART_CAP_FIFO,
+    },

};

drivers/serial/8250.c static inline int map_8250_out_reg(struc · line 328

Quoted 7 lines

drivers/serial/8250.c +7 -0

+
+#define map_8250_in_reg(up, offset) \
+    (((up)->port.type == PORT_RM9000) ? regmap_in[offset] : (offset))
+#define map_8250_out_reg(up, offset) \
+    (((up)->port.type == PORT_RM9000) ? regmap_out[offset] : (offset))
+
+

Why you're not using specific iotype for RM9000 UARTs?

Quoted 1 line

Because I did not realize that this was necessary. The device registers are

This is strange as you had an opposite example before your eyes.

Quoted 1 line

ioremapped, and so the standard UPIO_MEM32 seemed the right thing to use. I

It is not.

Quoted 1 line

will return to this topic further down.

   So, read on... :-)

Quoted 3 lines

I would like to return to the port type vs. iotype stuff once again. From what you wrote I seem to understand that the iotype is not just a method of accessing device registers, but also the primary means of discrimination between different h/w

No, it's intended as just a method of accessing device registers.

Quoted 2 lines

implementations, and hence every code to support a nonstandard device must define an iotype of its own, even though one of the existing iotypes would work just fine? In my

UPIO_MEM32 doesn't actually cover your case as it corresponds to the
UART with the
fully 8250-compatible register set, just having 32-bit registers instead of the usual 8-bit ones. RM9000 is clearly not fully compatible to 8250 in regard to the register addresses since it has RX/TX regs, FCR and the divisor latch mapped to the separate addresses, just like Alchemy UART. And I stressed that it's the main issue with this UART's compatibility to 8250 in my first followup.

Quoted 1 line

case, UPIO_AU might be the best choice,

Alchemy UARTs have *different* address mapping, so UPIO_AU clearly
*cannot* be used for RM9000 UART.

Quoted 1 line

Would I still need to invent UPIO_RM9K,

Yes.

Quoted 1 line

just to have a distinct iotype, and be able to do 'if (up->port.iotype == UPIO_RM9K)'

A good "just to".

Quoted 1 line

where I now use 'if (up->port.type == PORT_RM9000)'? That seems a bit weird.

Why?

Quoted 1 line

Thomas

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 15 lines

drivers/serial/8250.c +7 -0

+    [PORT_RM9000] = {
+        .name        = "RM9000",
+        .fifo_size    = 16,
+        .tx_loadsz    = 16,
+        .fcr        = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
+        .flags        = UART_CAP_FIFO,
+    },

};

drivers/serial/8250.c +5 -0

+
+#define map_8250_in_reg(up, offset) \
+    (((up)->port.type == PORT_RM9000) ? regmap_in[offset] : (offset))
+#define map_8250_out_reg(up, offset) \
+    (((up)->port.type == PORT_RM9000) ? regmap_out[offset] : 

(offset))

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+

Why you're not using specific iotype for RM9000 UARTs?

Quoted 3 lines

Quoted 1 line

Because I did not realize that this was necessary. The device registers are

This is strange as you had an opposite example before your eyes.

Now, it doesn't seem so strange. I thing I'm gonna agree with your
point.

Quoted 5 lines

Quoted 3 lines

I would like to return to the port type vs. iotype stuff once again. From what you wrote I seem to understand that the iotype is not just a method of accessing device registers, but also the primary means of discrimination between different h/w

No, it's intended as just a method of accessing device registers.

Only relevant to 8250 driver. The method is indeed quite
describabable by UPIO_MEM32.

Quoted 10 lines

Quoted 2 lines

implementations, and hence every code to support a nonstandard device must define an iotype of its own, even though one of the existing iotypes would work just fine? In my

UPIO_MEM32 doesn't actually cover your case as it corresponds to
the UART with the
fully 8250-compatible register set, just having 32-bit registers instead of the usual 8-bit ones. RM9000 is clearly not fully compatible to 8250 in regard to the register addresses since it has RX/TX regs, FCR and the divisor latch mapped to the separate addresses, just like Alchemy UART. And I stressed that it's the main issue with this UART's compatibility to 8250 in my first followup.

What I didn't take into account is that iotype thing is not at all
specific to 8250 driver. In the light of this, the reasons for appearance of UPIO_AU and UPIO_TSI indeed seem questionable.

Quoted 1 line

Quoted 1 line

Thomas

Quoted 29 lines

Quoted 29 lines

Quoted 17 lines

drivers/serial/8250.c +13 -0

@@ -289,6 +296,36 @@ static inline int map_8250_out_reg(struc
    return au_io_out_map[offset];
 }
 
+#elif defined (CONFIG_SERIAL_8250_RM9K)
+
+static const u8
+    regmap_in[8] = {
+        [UART_RX]    = 0x00,
+        [UART_IER]    = 0x0c,
+        [UART_IIR]    = 0x14,
+        [UART_LCR]    = 0x1c,
+        [UART_MCR]    = 0x20,
+        [UART_LSR]    = 0x24,
+        [UART_MSR]    = 0x28,
+        [UART_SCR]    = 0x2c
+    },

drivers/serial/8250.c static inline int map_8250_out_reg(struc · line 321

Quoted 10 lines

drivers/serial/8250.c +10 -0

+    regmap_out[8] = {
+        [UART_TX]     = 0x04,
+        [UART_IER]    = 0x0c,
+        [UART_FCR]    = 0x18,
+        [UART_LCR]    = 0x1c,
+        [UART_MCR]    = 0x20,
+        [UART_LSR]    = 0x24,
+        [UART_MSR]    = 0x28,
+        [UART_SCR]    = 0x2c
+    };

   I guess you're using regshift == 0?

Quoted 1 line

Yes.

Well, regshift of 2 seems more fitting for the 32-bit registers. This
is not principal but using 0 regshift don't actually buy anything -- the shift will be perfomed anyway.

Quoted 2 lines

implementations, and hence every code to support a nonstandard device must define an iotype of its own, even though one of the existing iotypes would work just fine? In my

UPIO_MEM32 doesn't actually cover your case as it corresponds to the
UART with the
fully 8250-compatible register set, just having 32-bit registers instead of the usual 8-bit ones. RM9000 is clearly not fully compatible to 8250 in regard to the register addresses since it has RX/TX regs, FCR and the divisor latch mapped to the separate addresses, just like Alchemy UART. And I stressed that it's the main issue with this UART's compatibility to 8250 in my first followup.

Quoted 1 line

Thomas

so far nobody commented on my recent mail, in which I explained why I think that the AU1X00 code in 8250.c is not entirely correct, so I assume nobody cares.