[PATCH] RM9000 serial driver

[PATCH] RM9000 serial driver

[Thomas Koeller]

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 <thomas.koeller@baslerweb.com>
---
 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(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 0ae9ced..b89de8b 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -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);
 		}
 	}
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 9799cce..6ae58ba 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -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.
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 86501a3..8a97caf 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -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
diff --git a/include/linux/serial_reg.h b/include/linux/serial_reg.h
index 3c8a6aa..c5c991a 100644
--- a/include/linux/serial_reg.h
+++ b/include/linux/serial_reg.h
@@ -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
 
 /*
  * LCR=0xBF (or DLAB=1 for 16C660)
  */
+#if defined(CONFIG_SERIAL_RM9000)
+#define UART_EFR	0xff	/* I/O: Extended Features Register */
+#else
 #define UART_EFR	2	/* I/O: Extended Features Register */
+#endif
 #define UART_EFR_CTS		0x80 /* CTS flow control */
 #define UART_EFR_RTS		0x40 /* RTS flow control */
 #define UART_EFR_SCD		0x20 /* Special character detect */
-- 
1.4.0


-- 
Thomas Koeller, Software Development

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel +49 (4102) 463-390
Fax +49 (4102) 463-46390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello.

Thomas Koeller wrote:

> 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.

    Nothing uncommon here, 8250 drivers supported 32-bit MMIO for years. The 
different register layout is a real problem.

> Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>
> ---
>  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(-)
> 
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 0ae9ced..b89de8b 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -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);
>  		}
>  	}
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 9799cce..6ae58ba 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -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.
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 86501a3..8a97caf 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -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
> diff --git a/include/linux/serial_reg.h b/include/linux/serial_reg.h
> index 3c8a6aa..c5c991a 100644
> --- a/include/linux/serial_reg.h
> +++ b/include/linux/serial_reg.h
> @@ -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
>  
>  /*
>   * LCR=0xBF (or DLAB=1 for 16C660)
>   */
> +#if defined(CONFIG_SERIAL_RM9000)
> +#define UART_EFR	0xff	/* I/O: Extended Features Register */
> +#else
>  #define UART_EFR	2	/* I/O: Extended Features Register */
> +#endif
>  #define UART_EFR_CTS		0x80 /* CTS flow control */
>  #define UART_EFR_RTS		0x40 /* RTS flow control */
>  #define UART_EFR_SCD		0x20 /* Special character detect */

    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

Re: [PATCH] RM9000 serial driver

[Thomas Koeller]

On Friday 11 August 2006 21:39, Sergei Shtylyov wrote:
>     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
-- 
Thomas Koeller, Software Development

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel +49 (4102) 463-390
Fax +49 (4102) 463-46390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello.

Thomas Koeller wrote:
> On Friday 11 August 2006 21:39, Sergei Shtylyov wrote:
> 
>>    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?

    8250.c and 8250-au1x00.c in drivers/serial/...

> Thanks,
> Thomas

WBR, Sergei

Re: [PATCH] RM9000 serial driver

[Thomas Koeller]

On Friday 11 August 2006 21:39, Sergei Shtylyov wrote:
>
>     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.
>

I rewrote the patch, so that it now uses the existing register mapping
support. After writing my own serial_dl_read()/serial_dl_write() I found
that autoconfig_read_divisor_id() did not use these, so I had to fix
that as well.

Here is the result:

-----------------------------------------------------------------------

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 <thomas.koeller@baslerweb.com>
---
 drivers/serial/8250.c       |   84 +++++++++++++++++++++++++++++++++----------
 include/linux/serial_core.h |    3 +-
 2 files changed, 67 insertions(+), 20 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 0ae9ced..c6c28ed 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -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[] = {
@@ -289,6 +296,34 @@ static inline int map_8250_out_reg(struc
 	return au_io_out_map[offset];
 }
 
+#elif defined (CONFIG_SERIAL_RM9000)
+
+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
+		((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);
+}
 #else
 #define serial_dl_read(up) _serial_dl_read(up)
 #define serial_dl_write(up, value) _serial_dl_write(up, value)
@@ -576,22 +624,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);
 
 	return id;
@@ -1138,8 +1181,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);
 		}
 	}
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 86501a3..8a97caf 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -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

-- 
Thomas Koeller, Software Development

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel +49 (4102) 463-390
Fax +49 (4102) 463-46390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Re: [PATCH] RM9000 serial driver

[Yoichi Yuasa]

Hi,

On Tue, 22 Aug 2006 00:57:51 +0200
Thomas Koeller <thomas.koeller@baslerweb.com> wrote:

<snip>

> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 0ae9ced..c6c28ed 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -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[] = {
> @@ -289,6 +296,34 @@ static inline int map_8250_out_reg(struc
>  	return au_io_out_map[offset];
>  }
>  
> +#elif defined (CONFIG_SERIAL_RM9000)

CONFIG_SERIAL_8250_RM9000.
Morover, you should update drivers/serial/Kconfig

> +
> +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
> +		((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);
> +}
>  #else

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

Yoichi

Re: [PATCH] RM9000 serial driver

[Thomas Koeller]

On Tuesday 22 August 2006 02:59, Yoichi Yuasa wrote:
>
> If you have an another standard 8250 port. this driver cannot support it
> You should do as well as AU1X00.
>
> Yoichi

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().

-- 
Thomas Koeller, Software Development

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel +49 (4102) 463-390
Fax +49 (4102) 463-46390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Re: [PATCH] RM9000 serial driver

[Thomas Koeller]

On Tuesday 22 August 2006 02:59, Yoichi Yuasa wrote:
>
> If you have an another standard 8250 port. this driver cannot support it
> You should do as well as AU1X00.
>
> Yoichi

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:



Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>
---
832ac1406f2530b7971cb0d23d3ede20a6057fa1
 drivers/serial/8250.c       |   86 ++++++++++++++++++++++++++++++++++---------
 drivers/serial/Kconfig      |    9 +++++
 include/linux/serial_core.h |    3 +-
 3 files changed, 78 insertions(+), 20 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 0ae9ced..afe0e1f 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -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[] = {
@@ -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,
+		[UART_LCR]	= 0x1c,
+		[UART_MCR]	= 0x20,
+		[UART_LSR]	= 0x24,
+		[UART_MSR]	= 0x28,
+		[UART_SCR]	= 0x2c
+	};
+
+#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))
+
+
 #else
 
 /* sane hardware needs no mapping */
@@ -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)
+{
+	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);
+}
 #else
 #define serial_dl_read(up) _serial_dl_read(up)
 #define serial_dl_write(up, value) _serial_dl_write(up, value)
@@ -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);
 
 	return id;
@@ -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) ?
+			   	(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);
 		}
 	}
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 9799cce..dfb51ff 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -244,6 +244,15 @@ config SERIAL_8250_AU1X00
 	  to this option.  The driver can handle 1 or 2 serial ports.
 	  If unsure, say N.
 
+config SERIAL_8250_RM9K
+	bool "Support for MIPS RM9xxx integrated serial port"
+	depends on SERIAL_8250 != n && SERIAL_RM9000
+	select SERIAL_8250_SHARE_IRQ
+	help
+	  Selecting this option will add support for the integrated serial
+	  port hardware found on MIPS RM9122 and similar processors.
+	  If unsure, say N.
+
 comment "Non-8250 serial port support"
 
 config SERIAL_AMBA_PL010
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 86501a3..8a97caf 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -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


-- 
Thomas Koeller, Software Development

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel +49 (4102) 463-390
Fax +49 (4102) 463-46390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Re: [PATCH] RM9000 serial driver

[Jonathan Day]

As it's not a driver I think I use, my opinions on the
matter should not carry any significant weight. On the
other hand, if there are no other opinions on the
matter, then no non-zero weight is insignificant.
(I'll leave it as an exercise to resolve the triple
negative.)

Anyway, it is my opinion that code believed to be
dubious should be fixed and that it is an error to add
more code that carries the same potential flaw until
either the flaw is resolved or proven insignificant.
That includes flaws in style or code cleanliness.

If the flaw is easily fixed, then it would seem vastly
better to fix it once now rather than twice (or more)
later - particularly if there is any risk that copied
flaws could be forgotten. Forgotten temporary code is
an excellent breeding-ground for bugs.

There's also a potential for friction as there is a
very understandable unease when it comes to knowingly
adding brokenness to the mainstream kernel if it's not
necessary, again even if that isn't brokenness in
terms of logic but merely in some aspect of how it's
represented.

On the flip-side, if we waited for code to be perfect,
we'd still be waiting for Alan Turing to finish the
world's first stored program.

--- Thomas Koeller <thomas.koeller@baslerweb.com>
wrote:

> 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:


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello.

Thomas Koeller wrote:

>>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,

    Not everybody have time to comment instantly. And the issue you've pointed
out is only theoretical at this point -- the "half-compatible" UARTs like
Alchemy's one are seen only in the SOCs so far...

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

    Now, to the patch itself...

> Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>
> ---
> 832ac1406f2530b7971cb0d23d3ede20a6057fa1
>  drivers/serial/8250.c       |   86 ++++++++++++++++++++++++++++++++++---------
>  drivers/serial/Kconfig      |    9 +++++
>  include/linux/serial_core.h |    3 +-
>  3 files changed, 78 insertions(+), 20 deletions(-)

> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 0ae9ced..afe0e1f 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -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,
> +	},
>  };

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

> -#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,
> +		[UART_LCR]	= 0x1c,
> +		[UART_MCR]	= 0x20,
> +		[UART_LSR]	= 0x24,
> +		[UART_MSR]	= 0x28,
> +		[UART_SCR]	= 0x2c
> +	};

    I guess you're using regshift == 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?

>  #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)
> +{
> +	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.

> @@ -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);
>  
>  	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.

> @@ -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) ?
> +			   	(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...

WBR, Sergei

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello.

Thomas Koeller wrote:

>>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

    The key word here is *could*. So far, this case is purely speculative. The 
"half-compatible" UARTs seem to only reside in some SOCs for which case the 
current scheme is still adequate.
    I think I understand why such "half- compatible" hardware has appeared at 
all -- the weird 8250 addressing scheme with several registers mapped to the 
single address may be hard to implement...

> 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.

    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.

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

    There actually may be some other (and more valid than your case :-) issues 
preventing autoconfure from use with SOC UARTs. I guess autoconfigure code 
wan't intended for the case of SOC chips -- their UARTs' charactarestics are 
usually known beforehand, and the correct PORT_* might be pre-set by the 
platform setup code.

WBR, Sergei

Re: [PATCH] RM9000 serial driver

[Russell King]

On Tue, Aug 29, 2006 at 05:32:35PM +0400, Sergei Shtylyov wrote:
> >@@ -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);
> > 
> > 	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.

>    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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello.

Russell King wrote:
>>>@@ -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);
>>>
>>>	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.

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

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

>>   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.

> 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?

> 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()?

WBR, Sergei

Re: [PATCH] RM9000 serial driver

[Russell King]

On Tue, Aug 29, 2006 at 11:37:18PM +0400, Sergei Shtylyov wrote:
> >>  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_*()?

Not sure.  Does this code actually even get reached?

> >>  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.
> 
> >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?

Not really - writing 0xEF is one such magic number, and it doesn't
change the current settings.  If you then clear DLAB (iow, 0x6F),
there's no guarantee that it won't change the settings on you, and,
eg start sending a break condition.  Why?  0xEF is defined to be a
magic number to access additional features, and 0x6f has no such
meaning.

> >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()?

In a similar way to the TSI ports?  Yes, that'd work.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] RM9000 serial driver

[Thomas Koeller]

On Tuesday 29 August 2006 15:32, Sergei Shtylyov wrote:
> > +	[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.

>
> > -#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,
> > +		[UART_LCR]	= 0x1c,
> > +		[UART_MCR]	= 0x20,
> > +		[UART_LSR]	= 0x24,
> > +		[UART_MSR]	= 0x28,
> > +		[UART_SCR]	= 0x2c
> > +	};
>
>     I guess you're using regshift == 0?

Yes.

>
> > +
> > +#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.

>
> >  #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)
> > +{
> > +	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.

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.

>
> > @@ -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);
> >
> >  	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?

>
>     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.
>
> > @@ -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) ?
> > +			   	(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
-- 
Thomas Koeller, Software Development

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel +49 (4102) 463-390
Fax +49 (4102) 463-46390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Re: [PATCH] RM9000 serial driver

[Thomas Koeller]

On Tuesday 29 August 2006 17:14, Sergei Shtylyov wrote:
> > 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 
-- 
Thomas Koeller, Software Development

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel +49 (4102) 463-390
Fax +49 (4102) 463-46390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello.

Thomas Koeller wrote:

>>>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?

    The iotype is passed to driver when registering the platform device or 
calling early_serial_setup().
There's absolutely no way for 8250.c to figure it out yourself. Please, review 
the driver's code more carefully. It was not at all that complex task to copy 
from the existing Alchemy code...

> Thomas 

WBR, Sergei

Re: [PATCH] RM9000 serial driver

[Russell King]

On Wed, Aug 30, 2006 at 01:00:32AM +0200, Thomas Koeller wrote:
> 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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] RM9000 serial driver

[Russell King]

On Sat, Aug 26, 2006 at 12:38:13AM +0200, Thomas Koeller wrote:
> 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.

Or maybe your assumption is wrong.  You were referring to the port type
not being setup at autodetect time, and assuming that is used to define
the register mapping.  It isn't, so your premise for it being broken is
incorrect.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello.

Thomas Koeller wrote:

>>>+	[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.

    Yes, I see one case where the distinct port type serves its purpose --
iotype could have been used instead anyway.

>>>@@ -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,
>>>+		[UART_LCR]	= 0x1c,
>>>+		[UART_MCR]	= 0x20,
>>>+		[UART_LSR]	= 0x24,
>>>+		[UART_MSR]	= 0x28,
>>>+		[UART_SCR]	= 0x2c
>>>+	};

>>    I guess you're using regshift == 0?

> 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.

>>>+
>>>+#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

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

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

    It is not.

> will return to this topic further down.

    So, read on... :-)

>>>@@ -374,21 +411,21 @@ #define serial_inp(up, offset)		serial_i
[...]
>>>-#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)
>>>+{
>>>+	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.

> 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.

    Yes. And it's after you have yourself pointed out the compatibility 
issues. :-)

>>>@@ -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);
>>>
>>> 	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?

    Now this is up to Russel. :-)

>>    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.

    BTW, I guess for RM9000 it should be also reserved?

>>>@@ -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) ?
>>>+			   	(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...

    Does RM9000 have UART_BUG_TXEN flag set?  Note that this code will only 
execute in such case.

> 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.

> 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.

> case, UPIO_AU might be the best choice,

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

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

    Why you've used readl() and writel() then, may I ask? :-)

> Would I still need to invent UPIO_RM9K,

    Yes.

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

    A good "just to".

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

    Why?

> Thomas

WBR, Sergei

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello, I wrote:

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

>>>> +
>>>> +#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

>    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.

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

>    It is not.

    Actually, it should fit well, indeed.

>> will return to this topic further down.

>    So, read on... :-)

>> 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.

>> 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.

>> case, UPIO_AU might be the best choice,

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

>> Would I still need to invent UPIO_RM9K,

>    Yes.

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

>    A good "just to".

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

>    Why?

    Indeed, I now see that this is weird.

>> Thomas

WBR, Sergei

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello, I wrote:

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

>>>>> +
>>>>> +#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

>>    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.

    That was also rash. :-)
    Introducing new UPIO_* values seem to be the necessary evil.
    Shows my knowledge of the serial core. :-<

>>> 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.

    But since we need to know the addressing scheme before poking at the 
registers in the autoconfig code (and that code seem to be *always* executed 
for the 8250 platform devices) and it's actually different from the other 
8250-compatible UARTs, we have no other solution then to introduce UPIO_AU.
    I guess you're using early_serial_setup() to register UARTs with the 8250 
driver? I think that the platform device method is preferrable now.

>>> 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.

    UPIO_TSI still seems somewhat of an abuse to me WRT to its UUE bit 
workaround. UPIO_AU however is a necessary evil (unless the driver code is 
changed to be able to pass the port type for platform devices and therefore 
not autoconfig them).

>>> Thomas

WBR, Sergei

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello.

Russell King wrote:

>>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.

    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

Re: [PATCH] RM9000 serial driver

[Thomas Koeller]

On Tuesday 29 August 2006 21:04, Russell King wrote:
> 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.

For the  RM9000, DLL and DLM are located at distinct addresses, so these 
registers could be accessed without prior setting of DLAB. However, the
h/w docs say that DLAB has to be used nonetheless. I doubt that, but
wanted to play safe. So, in order to make this work, I had two options:

1. to monitor all register writes for setting/clearing of DLAB, and 
   switch the register mapping tables accordingly, or

2. implement serial_dl_read()/serial_dl_write() to directly access
   the registers, thus bypassing the mapping.

I decided to implement option #2, because it seemed less of a kludge.
Would you still say that this is an abuse?

Thomas
-- 
Thomas Koeller, Software Development

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel +49 (4102) 463-390
Fax +49 (4102) 463-46390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Re: [PATCH] RM9000 serial driver

[Thomas Koeller]

On Wednesday 30 August 2006 14:12, Russell King wrote:

> 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

-- 
Thomas Koeller, Software Development

Basler Vision Technologies
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel +49 (4102) 463-390
Fax +49 (4102) 463-46390

mailto:thomas.koeller@baslerweb.com
http://www.baslerweb.com

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello.

Thomas Koeller wrote:

>>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.

    Different mapping requirements actually put the device beyond 8250 
compatibility, so it seems quite natural they're addressed by the different 
entity.
    RM9000 UART is, strictly speaking, *not* 8250 compatible and needs some 
trickery for the 8250 driver to support it (historically, Alchemy UARTs were 
driven by the distinct driver, au1x00_uart.c -- however, its code was for the 
most part copied over from 8250.c).

> 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?

    I think it's determined by the value of the programming interface register 
of the PCI device. The values 0 to 6 imply full backward compatibility to 8250 
WRT the addressing scheme, IIUC.

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

    For the PCI bus, it is.

> Thomas

WBR, Sergei

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello, I wrote:

>>>> @@ -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,
>>>> +        [UART_LCR]    = 0x1c,
>>>> +        [UART_MCR]    = 0x20,
>>>> +        [UART_LSR]    = 0x24,
>>>> +        [UART_MSR]    = 0x28,
>>>> +        [UART_SCR]    = 0x2c
>>>> +    };

>>>    I guess you're using regshift == 0?

>> 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.

    Not only that -- look at serial8250_request_std_resources(). Withouth the
proper regshift of 2 it won't be able to correctly calculate UART decoded
memory range size.  So, 0 simply doesn't fit.

>> 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.

    Further on, even if the regshift is correct it (being used as 8 << 
regshift) still won't give you the correct resource size since as I just said, 
the UART is not 8250-compatible and has more than 8 32-bit registers. So we 
end up with the need to modify serial8250_request_std_resources() and 
serial8250_release_std_resources().

>> Thomas

WBR, Sergei

Re: [PATCH] RM9000 serial driver

[Thomas Koeller]

On Mittwoch, 30. August 2006, Sergei Shtylyov wrote:
>
> Russell King wrote:
> >>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.
>
>     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

Hi,

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),
   > 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

-- 
Thomas Koeller
thomas@koeller.dyndns.org

Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov]

Hello.

Thomas Koeller wrote:

>>>>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.

>>    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

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

    I should also apologize for my discussion style back then -- I just was 
short of time (and now I am as well), and might have gotten too nervous for 
that reason...

> 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.

    It was good in theory but the practice has shown that this approach had a 
breache.

> 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?

    Of course.

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

    As they say, "patches welcome". :-)

> 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?

    Good question...
    It was probably easier to get all info from one place than to parse the 
resources. :-)

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

    More like necessary evil since Pantelis decided to merge support for the 
Alchemy UARTs... :-)

> 3. Russel says (qoute from an earlier mail),
>    > 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.

    Could you remind regarding which exactly code Russell has said this,
the one in autoconfig_read_divisor_id()?  Are you sure this code gets actually 
called for your UART or Alchemy one?  From looking at the code, I could 
conclude that it shouldn't be -- IIR (sharing address with EFR) shouldn't be 
reading as zero unless the modem status interrupt is pending -- that's what 
the EFR detection code counts on...
     But anyway, I don't see why this code is unsafe to convert to using the 
dl_serail_read/write accessors...

> 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

    Why, if we already have a working approach?

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

    Why would it be unacceptable, if the like code is already there, in the 
driver?

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

    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)...

> tk

MBR, Sergei

Re: [PATCH] RM9000 serial driver

[Thomas Koeller]

On Samstag, 10. Februar 2007, Sergei Shtylyov wrote:
>     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)...

Thank you for the hint - I did not notice that Russell retired in the
meantime. So I'll submit the driver to Andrew directly.

regards,
tk


-- 
Thomas Koeller
thomas@koeller.dyndns.org

[PATCH] RM9000 serial driver

[Thomas Koeller]

This patch adds support for the integrated serial ports of the MIPS RM9122
processor and its relatives. There has been some discussion about this
patch. The interesting part starts here:

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

Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>
---
 arch/mips/Kconfig           |    3 +
 drivers/serial/8250.c       |  102 +++++++++++++++++++++++++++++++-----------
 drivers/serial/Kconfig      |    9 ++++
 include/linux/serial_core.h |    3 +-
 4 files changed, 89 insertions(+), 28 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 34a52c8..5317ebb 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -1004,6 +1004,9 @@ config SOC_AU1X00
 	select SYS_HAS_CPU_MIPS32_R1
 	select SYS_SUPPORTS_32BIT_KERNEL
 
+config SERIAL_RM9000
+	bool
+
 config PNX8550
 	bool
 	select SOC_PNX8550
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 5261f0a..3f0fec4 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -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[] = {
@@ -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,
+		[UART_LCR]	= 0x1c,
+		[UART_MCR]	= 0x20,
+		[UART_LSR]	= 0x24,
+		[UART_MSR]	= 0x28,
+		[UART_SCR]	= 0x2c
+	};
+
+#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))
+
+
 #else
 
 /* sane hardware needs no mapping */
@@ -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)
+{
+	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);
+}
 #else
 #define serial_dl_read(up) _serial_dl_read(up)
 #define serial_dl_write(up, value) _serial_dl_write(up, value)
@@ -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_dl_write(p, 0);
+	id = serial_dl_read(p);
 
-	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_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);
 
 	return id;
@@ -604,7 +649,7 @@ static unsigned int autoconfig_read_divi
  * its clones.  (We treat the broken original StarTech 16650 V1 as a
  * 16550, and why not?  Startech doesn't seem to even acknowledge its
  * existence.)
- * 
+ *
  * What evil have men's minds wrought...
  */
 static void autoconfig_has_efr(struct uart_8250_port *up)
@@ -657,7 +702,7 @@ static void autoconfig_has_efr(struct ua
 			up->bugs |= UART_BUG_QUOT;
 		return;
 	}
-	
+
 	/*
 	 * We check for a XR16C850 by setting DLL and DLM to 0, and then
 	 * reading back DLL and DLM.  The chip type depends on the DLM
@@ -800,7 +845,7 @@ static void autoconfig_16550a(struct uar
 			status1 &= ~0xB0; /* Disable LOCK, mask out PRESL[01] */
 			status1 |= 0x10;  /* 1.625 divisor for baud_base --> 921600 */
 			serial_outp(up, 0x04, status1);
-			
+
 			serial_dl_write(up, quot);
 
 			serial_outp(up, UART_LCR, 0);
@@ -905,7 +950,7 @@ static void autoconfig(struct uart_8250_
 		/*
 		 * Do a simple existence test first; if we fail this,
 		 * there's no point trying anything else.
-		 * 
+		 *
 		 * 0x80 is used as a nonsense port to prevent against
 		 * false positives due to ISA bus float.  The
 		 * assumption is that 0x80 is a non-existent port;
@@ -940,7 +985,7 @@ #endif
 	save_mcr = serial_in(up, UART_MCR);
 	save_lcr = serial_in(up, UART_LCR);
 
-	/* 
+	/*
 	 * Check to see if a UART is really there.  Certain broken
 	 * internal modems based on the Rockwell chipset fail this
 	 * test, because they apparently don't implement the loopback
@@ -1047,7 +1092,7 @@ #endif
 	else
 		serial_outp(up, UART_IER, 0);
 
- out:	
+ out:
 	spin_unlock_irqrestore(&up->port.lock, flags);
 //	restore_flags(flags);
 	DEBUG_AUTOCONF("type=%s\n", uart_config[up->port.type].name);
@@ -1073,7 +1118,7 @@ static void autoconfig_irq(struct uart_8
 	save_mcr = serial_inp(up, UART_MCR);
 	save_ier = serial_inp(up, UART_IER);
 	serial_outp(up, UART_MCR, UART_MCR_OUT1 | UART_MCR_OUT2);
-	
+
 	irqs = probe_irq_on();
 	serial_outp(up, UART_MCR, 0);
 	udelay (10);
@@ -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) ?
+				(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);
 		}
 	}
@@ -1801,7 +1849,7 @@ #endif
 	/*
 	 * 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, 0, port->uartclk/16);
 	quot = serial8250_get_divisor(port, baud);
 
 	/*
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 9ddfb84..228ae12 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -254,6 +254,15 @@ config SERIAL_8250_AU1X00
 	  to this option.  The driver can handle 1 or 2 serial ports.
 	  If unsure, say N.
 
+config SERIAL_8250_RM9K
+	bool "Support for MIPS RM9xxx integrated serial port"
+	depends on SERIAL_8250 != n && SERIAL_RM9000
+	select SERIAL_8250_SHARE_IRQ
+	help
+	  Selecting this option will add support for the integrated serial
+	  port hardware found on MIPS RM9122 and similar processors.
+	  If unsure, say N.
+
 comment "Non-8250 serial port support"
 
 config SERIAL_AMBA_PL010
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index cf23813..275392f 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -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
-- 
1.4.3