[PATCH] serial: serial_txx9 driver update

[PATCH] serial: serial_txx9 driver update

[Atsushi Nemoto]

This patch updates serial_txx9 driver.  (diff from 2.6.16-rc1)

 * More strict check in verify_port.  Cleanup.
 * Do not insert a char caused previous overrun.
 * Fix some spin_locks.
 * Do not call uart_add_one_port for absent ports.

Also, this patch removes a BROKEN tag from Kconfig.  This driver has
been marked as BROKEN by removal of uart_register_port, but it has
been solved already on Sep 2005.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 5e7199f..761c97c 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -858,7 +858,7 @@ config SERIAL_M32R_PLDSIO
 
 config SERIAL_TXX9
 	bool "TMPTX39XX/49XX SIO support"
-	depends HAS_TXX9_SERIAL && BROKEN
+	depends HAS_TXX9_SERIAL
 	select SERIAL_CORE
 	default y
 
diff --git a/drivers/serial/serial_txx9.c b/drivers/serial/serial_txx9.c
index ee98a86..b131383 100644
--- a/drivers/serial/serial_txx9.c
+++ b/drivers/serial/serial_txx9.c
@@ -33,6 +33,10 @@
  *	1.02	Cleanup. (import 8250.c changes)
  *	1.03	Fix low-latency mode. (import 8250.c changes)
  *	1.04	Remove usage of deprecated functions, cleanup.
+ *	1.05	More strict check in verify_port.  Cleanup.
+ *	1.06	Do not insert a char caused previous overrun.
+ *		Fix some spin_locks.
+ *		Do not call uart_add_one_port for absent ports.
  */
 #include <linux/config.h>
 
@@ -57,7 +61,7 @@
 #include <asm/io.h>
 #include <asm/irq.h>
 
-static char *serial_version = "1.04";
+static char *serial_version = "1.06";
 static char *serial_name = "TX39/49 Serial driver";
 
 #define PASS_LIMIT	256
@@ -210,7 +214,7 @@ static inline unsigned int sio_in(struct
 {
 	switch (up->port.iotype) {
 	default:
-		return *(volatile u32 *)(up->port.membase + offset);
+		return __raw_readl(up->port.membase + offset);
 	case UPIO_PORT:
 		return inl(up->port.iobase + offset);
 	}
@@ -221,7 +225,7 @@ sio_out(struct uart_txx9_port *up, int o
 {
 	switch (up->port.iotype) {
 	default:
-		*(volatile u32 *)(up->port.membase + offset) = value;
+		__raw_writel(value, up->port.membase + offset);
 		break;
 	case UPIO_PORT:
 		outl(value, up->port.iobase + offset);
@@ -259,34 +263,19 @@ sio_quot_set(struct uart_txx9_port *up, 
 static void serial_txx9_stop_tx(struct uart_port *port)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
-
-	spin_lock_irqsave(&up->port.lock, flags);
 	sio_mask(up, TXX9_SIDICR, TXX9_SIDICR_TIE);
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_start_tx(struct uart_port *port)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
-
-	spin_lock_irqsave(&up->port.lock, flags);
 	sio_set(up, TXX9_SIDICR, TXX9_SIDICR_TIE);
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_stop_rx(struct uart_port *port)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
-
-	spin_lock_irqsave(&up->port.lock, flags);
 	up->port.read_status_mask &= ~TXX9_SIDISR_RDIS;
-#if 0
-	sio_mask(up, TXX9_SIDICR, TXX9_SIDICR_RIE);
-#endif
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_enable_ms(struct uart_port *port)
@@ -302,12 +291,16 @@ receive_chars(struct uart_txx9_port *up,
 	unsigned int disr = *status;
 	int max_count = 256;
 	char flag;
+	unsigned int next_ignore_status_mask;
 
 	do {
 		ch = sio_in(up, TXX9_SIRFIFO);
 		flag = TTY_NORMAL;
 		up->port.icount.rx++;
 
+		/* mask out RFDN_MASK bit added by previous overrun */
+		next_ignore_status_mask =
+			up->port.ignore_status_mask & ~TXX9_SIDISR_RFDN_MASK;
 		if (unlikely(disr & (TXX9_SIDISR_UBRK | TXX9_SIDISR_UPER |
 				     TXX9_SIDISR_UFER | TXX9_SIDISR_UOER))) {
 			/*
@@ -328,8 +321,17 @@ receive_chars(struct uart_txx9_port *up,
 				up->port.icount.parity++;
 			else if (disr & TXX9_SIDISR_UFER)
 				up->port.icount.frame++;
-			if (disr & TXX9_SIDISR_UOER)
+			if (disr & TXX9_SIDISR_UOER) {
 				up->port.icount.overrun++;
+				/*
+				 * The receiver read buffer still hold
+				 * a char which caused overrun.
+				 * Ignore next char by adding RFDN_MASK
+				 * to ignore_status_mask temporarily.
+				 */
+				next_ignore_status_mask |=
+					TXX9_SIDISR_RFDN_MASK;
+			}
 
 			/*
 			 * Mask off conditions which should be ingored.
@@ -349,6 +351,7 @@ receive_chars(struct uart_txx9_port *up,
 		uart_insert_char(&up->port, disr, TXX9_SIDISR_UOER, ch, flag);
 
 	ignore_char:
+		up->port.ignore_status_mask = next_ignore_status_mask;
 		disr = sio_in(up, TXX9_SIDISR);
 	} while (!(disr & TXX9_SIDISR_UVALID) && (max_count-- > 0));
 	spin_unlock(&up->port.lock);
@@ -450,14 +453,11 @@ static unsigned int serial_txx9_get_mctr
 static void serial_txx9_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
 
-	spin_lock_irqsave(&up->port.lock, flags);
 	if (mctrl & TIOCM_RTS)
 		sio_mask(up, TXX9_SIFLCR, TXX9_SIFLCR_RTSSC);
 	else
 		sio_set(up, TXX9_SIFLCR, TXX9_SIFLCR_RTSSC);
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_break_ctl(struct uart_port *port, int break_state)
@@ -784,8 +784,13 @@ static void serial_txx9_config_port(stru
 static int
 serial_txx9_verify_port(struct uart_port *port, struct serial_struct *ser)
 {
-	if (ser->irq < 0 ||
-	    ser->baud_base < 9600 || ser->type != PORT_TXX9)
+	unsigned long new_port = (unsigned long)ser->port +
+		((unsigned long)ser->port_high << ((sizeof(long) - sizeof(int)) * 8));
+	if (ser->type != port->type ||
+	    ser->irq != port->irq ||
+	    ser->io_type != port->iotype ||
+	    new_port != port->iobase ||
+	    (unsigned long)ser->iomem_base != port->mapbase)
 		return -EINVAL;
 	return 0;
 }
@@ -827,7 +832,8 @@ static void __init serial_txx9_register_
 
 		up->port.line = i;
 		up->port.ops = &serial_txx9_pops;
-		uart_add_one_port(drv, &up->port);
+		if (up->port.iobase || up->port.mapbase)
+			uart_add_one_port(drv, &up->port);
 	}
 }
 
@@ -927,11 +933,6 @@ static int serial_txx9_console_setup(str
 		return -ENODEV;
 
 	/*
-	 * Temporary fix.
-	 */
-	spin_lock_init(&port->lock);
-
-	/*
 	 *	Disable UART interrupts, set DTR and RTS high
 	 *	and set speed.
 	 */
@@ -1041,11 +1042,10 @@ static int __devinit serial_txx9_registe
 	mutex_lock(&serial_txx9_mutex);
 	for (i = 0; i < UART_NR; i++) {
 		uart = &serial_txx9_ports[i];
-		if (uart->port.type == PORT_UNKNOWN)
+		if (!(uart->port.iobase || uart->port.mapbase))
 			break;
 	}
 	if (i < UART_NR) {
-		uart_remove_one_port(&serial_txx9_reg, &uart->port);
 		uart->port.iobase = port->iobase;
 		uart->port.membase = port->membase;
 		uart->port.irq      = port->irq;
@@ -1080,9 +1080,8 @@ static void __devexit serial_txx9_unregi
 	uart->port.type = PORT_UNKNOWN;
 	uart->port.iobase = 0;
 	uart->port.mapbase = 0;
-	uart->port.membase = 0;
+	uart->port.membase = NULL;
 	uart->port.dev = NULL;
-	uart_add_one_port(&serial_txx9_reg, &uart->port);
 	mutex_unlock(&serial_txx9_mutex);
 }
 
@@ -1198,8 +1197,11 @@ static void __exit serial_txx9_exit(void
 #ifdef ENABLE_SERIAL_TXX9_PCI
 	pci_unregister_driver(&serial_txx9_pci_driver);
 #endif
-	for (i = 0; i < UART_NR; i++)
-		uart_remove_one_port(&serial_txx9_reg, &serial_txx9_ports[i].port);
+	for (i = 0; i < UART_NR; i++) {
+		struct uart_txx9_port *up = &serial_txx9_ports[i];
+		if (up->port.iobase || up->port.mapbase)
+			uart_remove_one_port(&serial_txx9_reg, &up->port);
+	}
 
 	uart_unregister_driver(&serial_txx9_reg);
 }

Re: [PATCH] serial: serial_txx9 driver update

[Andrew Morton]

Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
>
>  serial_txx9_verify_port(struct uart_port *port, struct serial_struct *ser)
>   {
>  -	if (ser->irq < 0 ||
>  -	    ser->baud_base < 9600 || ser->type != PORT_TXX9)
>  +	unsigned long new_port = (unsigned long)ser->port +
>  +		((unsigned long)ser->port_high << ((sizeof(long) - sizeof(int)) * 8));

Are you sure about this part?  Shifting something left by sizeof(something)
seems very strange.  It'll give different results on 64-bit machines for
the same hardware.  Are you sure it wasn't supposed to be an addition?

If this was indeed intended then can you please explain why?

If it was supposed to be an addition, wouldn't this be more clearly
expressed by defining a suitable structure and using sizeof(that structure)
to work out the address range?

Re: [PATCH] serial: serial_txx9 driver update

[Russell King]

On Sat, Jan 21, 2006 at 11:36:49PM -0800, Andrew Morton wrote:
> Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> >
> >  serial_txx9_verify_port(struct uart_port *port, struct serial_struct *ser)
> >   {
> >  -	if (ser->irq < 0 ||
> >  -	    ser->baud_base < 9600 || ser->type != PORT_TXX9)
> >  +	unsigned long new_port = (unsigned long)ser->port +
> >  +		((unsigned long)ser->port_high << ((sizeof(long) - sizeof(int)) * 8));
> 
> Are you sure about this part?  Shifting something left by sizeof(something)
> seems very strange.  It'll give different results on 64-bit machines for
> the same hardware.  Are you sure it wasn't supposed to be an addition?

There is a definition for that constant - it's called HIGH_BITS_OFFSET.
No need to try to buggily recreate it.

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

Re: [PATCH] serial: serial_txx9 driver update

[Andrew Morton]

Russell King <rmk+lkml@arm.linux.org.uk> wrote:
>
> On Sat, Jan 21, 2006 at 11:36:49PM -0800, Andrew Morton wrote:
> > Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> > >
> > >  serial_txx9_verify_port(struct uart_port *port, struct serial_struct *ser)
> > >   {
> > >  -	if (ser->irq < 0 ||
> > >  -	    ser->baud_base < 9600 || ser->type != PORT_TXX9)
> > >  +	unsigned long new_port = (unsigned long)ser->port +
> > >  +		((unsigned long)ser->port_high << ((sizeof(long) - sizeof(int)) * 8));
> > 
> > Are you sure about this part?  Shifting something left by sizeof(something)
> > seems very strange.  It'll give different results on 64-bit machines for
> > the same hardware.  Are you sure it wasn't supposed to be an addition?
> 
> There is a definition for that constant - it's called HIGH_BITS_OFFSET.

There are two definitions, actually.  drivers/serial/serial_core.c and
drivers/serial/8250.h.

> No need to try to buggily recreate it.

Where's the bug in the proposed code?

Can you tell us what HIGH_BITS_OFFSET actually does?  Stuffing the port
address into the upper 32-bits of a ulong on 64-bit machines.  Am consumed
by curiosity.

Re: [PATCH] serial: serial_txx9 driver update

[Russell King]

On Sun, Jan 22, 2006 at 12:33:07AM -0800, Andrew Morton wrote:
> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> >
> > On Sat, Jan 21, 2006 at 11:36:49PM -0800, Andrew Morton wrote:
> > > Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> > > >
> > > >  serial_txx9_verify_port(struct uart_port *port, struct serial_struct *ser)
> > > >   {
> > > >  -	if (ser->irq < 0 ||
> > > >  -	    ser->baud_base < 9600 || ser->type != PORT_TXX9)
> > > >  +	unsigned long new_port = (unsigned long)ser->port +
> > > >  +		((unsigned long)ser->port_high << ((sizeof(long) - sizeof(int)) * 8));
> > > 
> > > Are you sure about this part?  Shifting something left by sizeof(something)
> > > seems very strange.  It'll give different results on 64-bit machines for
> > > the same hardware.  Are you sure it wasn't supposed to be an addition?
> > 
> > There is a definition for that constant - it's called HIGH_BITS_OFFSET.
> 
> There are two definitions, actually.  drivers/serial/serial_core.c and
> drivers/serial/8250.h.
> 
> > No need to try to buggily recreate it.
> 
> Where's the bug in the proposed code?

There isn't, but because it's wider than 80 columns, it got mis-read.
As demonstrated, the 80 column rule _still_ matters for readability.

> Can you tell us what HIGH_BITS_OFFSET actually does?  Stuffing the port
> address into the upper 32-bits of a ulong on 64-bit machines.  Am consumed
> by curiosity.

It's history.  port in serial_struct has been declared as:

        unsigned int    port;

since the year dot, and has been exposed to userland since it's
inception. I guess that 64-bit machines came along, and this was
found to be inadequate.  Rather than deprecate the current interface
and provide a sane API, whoever did this came up with this yucky
solution where

        unsigned int    port_high;

contains the address bits outside the range of "port".  Presumably
they assumed that sizeof(unsigned long) == 2 * sizeof(unsigned int)
here. If not, this silly interface again breaks on us.

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

Re: [PATCH] serial: serial_txx9 driver update

[Atsushi Nemoto]

>>>>> On Sun, 22 Jan 2006 23:02:09 +0000, Russell King <rmk+lkml@arm.linux.org.uk> said:
>> > > Are you sure about this part?  Shifting something left by sizeof(something)
>> > > seems very strange.  It'll give different results on 64-bit machines for
>> > > the same hardware.  Are you sure it wasn't supposed to be an addition?
>> > 
>> > There is a definition for that constant - it's called HIGH_BITS_OFFSET.
>> 
>> There are two definitions, actually.  drivers/serial/serial_core.c and
>> drivers/serial/8250.h.
>> 
>> > No need to try to buggily recreate it.
>> 
>> Where's the bug in the proposed code?

rmk> There isn't, but because it's wider than 80 columns, it got mis-read.
rmk> As demonstrated, the 80 column rule _still_ matters for readability.

Thank you for all comments.  Then I updated my patch using the
HIGH_BITS_OFFSET.

---
This patch updates serial_txx9 driver.  (diff from 2.6.16-rc1)

 * More strict check in verify_port.  Cleanup.
 * Do not insert a char caused previous overrun.
 * Fix some spin_locks.
 * Do not call uart_add_one_port for absent ports.

Also, this patch removes a BROKEN tag from Kconfig.  This driver has
been marked as BROKEN by removal of uart_register_port, but it has
been solved already on Sep 2005.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 9fd1925..27c4676 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -858,7 +858,7 @@ config SERIAL_M32R_PLDSIO
 
 config SERIAL_TXX9
 	bool "TMPTX39XX/49XX SIO support"
-	depends HAS_TXX9_SERIAL && BROKEN
+	depends HAS_TXX9_SERIAL
 	select SERIAL_CORE
 	default y
 
diff --git a/drivers/serial/serial_txx9.c b/drivers/serial/serial_txx9.c
index ee98a86..141173e 100644
--- a/drivers/serial/serial_txx9.c
+++ b/drivers/serial/serial_txx9.c
@@ -33,6 +33,10 @@
  *	1.02	Cleanup. (import 8250.c changes)
  *	1.03	Fix low-latency mode. (import 8250.c changes)
  *	1.04	Remove usage of deprecated functions, cleanup.
+ *	1.05	More strict check in verify_port.  Cleanup.
+ *	1.06	Do not insert a char caused previous overrun.
+ *		Fix some spin_locks.
+ *		Do not call uart_add_one_port for absent ports.
  */
 #include <linux/config.h>
 
@@ -57,7 +61,7 @@
 #include <asm/io.h>
 #include <asm/irq.h>
 
-static char *serial_version = "1.04";
+static char *serial_version = "1.06";
 static char *serial_name = "TX39/49 Serial driver";
 
 #define PASS_LIMIT	256
@@ -94,6 +98,8 @@ static char *serial_name = "TX39/49 Seri
 #define UART_NR  4
 #endif
 
+#define HIGH_BITS_OFFSET	((sizeof(long)-sizeof(int))*8)
+
 struct uart_txx9_port {
 	struct uart_port	port;
 
@@ -210,7 +216,7 @@ static inline unsigned int sio_in(struct
 {
 	switch (up->port.iotype) {
 	default:
-		return *(volatile u32 *)(up->port.membase + offset);
+		return __raw_readl(up->port.membase + offset);
 	case UPIO_PORT:
 		return inl(up->port.iobase + offset);
 	}
@@ -221,7 +227,7 @@ sio_out(struct uart_txx9_port *up, int o
 {
 	switch (up->port.iotype) {
 	default:
-		*(volatile u32 *)(up->port.membase + offset) = value;
+		__raw_writel(value, up->port.membase + offset);
 		break;
 	case UPIO_PORT:
 		outl(value, up->port.iobase + offset);
@@ -259,34 +265,19 @@ sio_quot_set(struct uart_txx9_port *up, 
 static void serial_txx9_stop_tx(struct uart_port *port)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
-
-	spin_lock_irqsave(&up->port.lock, flags);
 	sio_mask(up, TXX9_SIDICR, TXX9_SIDICR_TIE);
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_start_tx(struct uart_port *port)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
-
-	spin_lock_irqsave(&up->port.lock, flags);
 	sio_set(up, TXX9_SIDICR, TXX9_SIDICR_TIE);
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_stop_rx(struct uart_port *port)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
-
-	spin_lock_irqsave(&up->port.lock, flags);
 	up->port.read_status_mask &= ~TXX9_SIDISR_RDIS;
-#if 0
-	sio_mask(up, TXX9_SIDICR, TXX9_SIDICR_RIE);
-#endif
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_enable_ms(struct uart_port *port)
@@ -302,12 +293,16 @@ receive_chars(struct uart_txx9_port *up,
 	unsigned int disr = *status;
 	int max_count = 256;
 	char flag;
+	unsigned int next_ignore_status_mask;
 
 	do {
 		ch = sio_in(up, TXX9_SIRFIFO);
 		flag = TTY_NORMAL;
 		up->port.icount.rx++;
 
+		/* mask out RFDN_MASK bit added by previous overrun */
+		next_ignore_status_mask =
+			up->port.ignore_status_mask & ~TXX9_SIDISR_RFDN_MASK;
 		if (unlikely(disr & (TXX9_SIDISR_UBRK | TXX9_SIDISR_UPER |
 				     TXX9_SIDISR_UFER | TXX9_SIDISR_UOER))) {
 			/*
@@ -328,8 +323,17 @@ receive_chars(struct uart_txx9_port *up,
 				up->port.icount.parity++;
 			else if (disr & TXX9_SIDISR_UFER)
 				up->port.icount.frame++;
-			if (disr & TXX9_SIDISR_UOER)
+			if (disr & TXX9_SIDISR_UOER) {
 				up->port.icount.overrun++;
+				/*
+				 * The receiver read buffer still hold
+				 * a char which caused overrun.
+				 * Ignore next char by adding RFDN_MASK
+				 * to ignore_status_mask temporarily.
+				 */
+				next_ignore_status_mask |=
+					TXX9_SIDISR_RFDN_MASK;
+			}
 
 			/*
 			 * Mask off conditions which should be ingored.
@@ -349,6 +353,7 @@ receive_chars(struct uart_txx9_port *up,
 		uart_insert_char(&up->port, disr, TXX9_SIDISR_UOER, ch, flag);
 
 	ignore_char:
+		up->port.ignore_status_mask = next_ignore_status_mask;
 		disr = sio_in(up, TXX9_SIDISR);
 	} while (!(disr & TXX9_SIDISR_UVALID) && (max_count-- > 0));
 	spin_unlock(&up->port.lock);
@@ -450,14 +455,11 @@ static unsigned int serial_txx9_get_mctr
 static void serial_txx9_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 	struct uart_txx9_port *up = (struct uart_txx9_port *)port;
-	unsigned long flags;
 
-	spin_lock_irqsave(&up->port.lock, flags);
 	if (mctrl & TIOCM_RTS)
 		sio_mask(up, TXX9_SIFLCR, TXX9_SIFLCR_RTSSC);
 	else
 		sio_set(up, TXX9_SIFLCR, TXX9_SIFLCR_RTSSC);
-	spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static void serial_txx9_break_ctl(struct uart_port *port, int break_state)
@@ -784,8 +786,14 @@ static void serial_txx9_config_port(stru
 static int
 serial_txx9_verify_port(struct uart_port *port, struct serial_struct *ser)
 {
-	if (ser->irq < 0 ||
-	    ser->baud_base < 9600 || ser->type != PORT_TXX9)
+	unsigned long new_port = ser->port;
+	if (HIGH_BITS_OFFSET)
+		new_port += (unsigned long)ser->port_high << HIGH_BITS_OFFSET;
+	if (ser->type != port->type ||
+	    ser->irq != port->irq ||
+	    ser->io_type != port->iotype ||
+	    new_port != port->iobase ||
+	    (unsigned long)ser->iomem_base != port->mapbase)
 		return -EINVAL;
 	return 0;
 }
@@ -827,7 +835,8 @@ static void __init serial_txx9_register_
 
 		up->port.line = i;
 		up->port.ops = &serial_txx9_pops;
-		uart_add_one_port(drv, &up->port);
+		if (up->port.iobase || up->port.mapbase)
+			uart_add_one_port(drv, &up->port);
 	}
 }
 
@@ -927,11 +936,6 @@ static int serial_txx9_console_setup(str
 		return -ENODEV;
 
 	/*
-	 * Temporary fix.
-	 */
-	spin_lock_init(&port->lock);
-
-	/*
 	 *	Disable UART interrupts, set DTR and RTS high
 	 *	and set speed.
 	 */
@@ -1041,11 +1045,10 @@ static int __devinit serial_txx9_registe
 	mutex_lock(&serial_txx9_mutex);
 	for (i = 0; i < UART_NR; i++) {
 		uart = &serial_txx9_ports[i];
-		if (uart->port.type == PORT_UNKNOWN)
+		if (!(uart->port.iobase || uart->port.mapbase))
 			break;
 	}
 	if (i < UART_NR) {
-		uart_remove_one_port(&serial_txx9_reg, &uart->port);
 		uart->port.iobase = port->iobase;
 		uart->port.membase = port->membase;
 		uart->port.irq      = port->irq;
@@ -1080,9 +1083,8 @@ static void __devexit serial_txx9_unregi
 	uart->port.type = PORT_UNKNOWN;
 	uart->port.iobase = 0;
 	uart->port.mapbase = 0;
-	uart->port.membase = 0;
+	uart->port.membase = NULL;
 	uart->port.dev = NULL;
-	uart_add_one_port(&serial_txx9_reg, &uart->port);
 	mutex_unlock(&serial_txx9_mutex);
 }
 
@@ -1198,8 +1200,11 @@ static void __exit serial_txx9_exit(void
 #ifdef ENABLE_SERIAL_TXX9_PCI
 	pci_unregister_driver(&serial_txx9_pci_driver);
 #endif
-	for (i = 0; i < UART_NR; i++)
-		uart_remove_one_port(&serial_txx9_reg, &serial_txx9_ports[i].port);
+	for (i = 0; i < UART_NR; i++) {
+		struct uart_txx9_port *up = &serial_txx9_ports[i];
+		if (up->port.iobase || up->port.mapbase)
+			uart_remove_one_port(&serial_txx9_reg, &up->port);
+	}
 
 	uart_unregister_driver(&serial_txx9_reg);
 }

Re: [PATCH] serial: serial_txx9 driver update

[Russell King]

On Mon, Jan 23, 2006 at 03:05:02PM +0900, Atsushi Nemoto wrote:
> -			if (disr & TXX9_SIDISR_UOER)
> +			if (disr & TXX9_SIDISR_UOER) {
>  				up->port.icount.overrun++;
> +				/*
> +				 * The receiver read buffer still hold
> +				 * a char which caused overrun.
> +				 * Ignore next char by adding RFDN_MASK
> +				 * to ignore_status_mask temporarily.
> +				 */
> +				next_ignore_status_mask |=
> +					TXX9_SIDISR_RFDN_MASK;
> +			}

I'm not sure what you mean here.

If we successfully received the string ABCDEFGH, and the next character
to be received (I) causes an overrun condition, what happens in the
case that overruns are not ignored?

Will you read ABCDEFG without any errors from the UART, and then H with
an overrun error?  If so, you should pass to the TTY layer ABCDEFGH and
then a NUL character with TTY_OVERRUN set.  Note that uart_insert_char()
does this for you.

Alternatively, the UART may give you: ABCDEFGI where I is marked as an
overrun error.  In this case, you want to pass to the TTY layer ABCDEFG,
then a NUL character with TTY_OVERRUN set, then I.

If overruns are ignored, you merely omit to insert the NUL character with
TTY_OVERRUN set.

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

Re: [PATCH] serial: serial_txx9 driver update

[Atsushi Nemoto]

>>>>> On Mon, 23 Jan 2006 09:57:00 +0000, Russell King <rmk+lkml@arm.linux.org.uk> said:
>> -			if (disr & TXX9_SIDISR_UOER)
>> +			if (disr & TXX9_SIDISR_UOER) {
>>  				up-> port.icount.overrun++;
>> +				/*
>> +				 * The receiver read buffer still hold
>> +				 * a char which caused overrun.
>> +				 * Ignore next char by adding RFDN_MASK
>> +				 * to ignore_status_mask temporarily.
>> +				 */
>> +				next_ignore_status_mask |=
>> +					TXX9_SIDISR_RFDN_MASK;
>> +			}

rmk> I'm not sure what you mean here.

rmk> If we successfully received the string ABCDEFGH, and the next character
rmk> to be received (I) causes an overrun condition, what happens in the
rmk> case that overruns are not ignored?

In this case, I will read ABCDEFG without errors, and then I with an overrun 

rmk> Will you read ABCDEFG without any errors from the UART, and then H with
rmk> an overrun error?  If so, you should pass to the TTY layer ABCDEFGH and
rmk> then a NUL character with TTY_OVERRUN set.  Note that uart_insert_char()
rmk> does this for you.

Yes, in this case I will read ABCDEFG without error, and then H with
an overrun error.  But the UART still hold "I" in its "read buffer".
The "read buffer" is exist outside the receiver FIFO.  So if 'J' comes
in later, I will read "IJ".  There is no way to clear the "read
buffer" except resetting the UART.

Resetting the whole UART is too intrusive, so I chose a way in the
patch (Ignore just next one char after an overrun error.)

---
Atsushi Nemoto

Re: [PATCH] serial: serial_txx9 driver update

[Atsushi Nemoto]

>>>>> On Mon, 23 Jan 2006 22:39:43 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> said:

anemo> In this case, I will read ABCDEFG without errors, and then I with an overrun 

Excuse me, please ignore this broken line.
---
Atsushi Nemoto

Re: [PATCH] serial: serial_txx9 driver update

[Russell King]

On Mon, Jan 23, 2006 at 10:39:43PM +0900, Atsushi Nemoto wrote:
> rmk> If we successfully received the string ABCDEFGH, and the next character
> rmk> to be received (I) causes an overrun condition, what happens in the
> rmk> case that overruns are not ignored?
> 
> In this case, I will read ABCDEFG without errors, and then I with an overrun 

Duely ignored.

> rmk> Will you read ABCDEFG without any errors from the UART, and then H with
> rmk> an overrun error?  If so, you should pass to the TTY layer ABCDEFGH and
> rmk> then a NUL character with TTY_OVERRUN set.  Note that uart_insert_char()
> rmk> does this for you.
> 
> Yes, in this case I will read ABCDEFG without error, and then H with
> an overrun error.  But the UART still hold "I" in its "read buffer".
> The "read buffer" is exist outside the receiver FIFO.  So if 'J' comes
> in later, I will read "IJ".  There is no way to clear the "read
> buffer" except resetting the UART.

Ok, so if someone sent you ABCDEFGHIJ, all before you could read anything
from the UART, where I causes an overrun, you'll read ABCDEFGHJ, but the
status associated with H will indicate an overrun condition?

However, either way the behaviour after the overrun condition has no
bearing on what follows.

Your overrun behaviour is near enough to typical 8250 behaviour that you
can use the helper provided - uart_insert_char().  This eliminates the
special flag handling you seem to have created.

IOW, you want to do:

	ch = read_uart_fifo_data_register();
	status = read_uart_status_register();

	/*
	... error processing ... to set flag but omitting overrun.
	... don't do ignore processing here - uart_insert_char does
	... that for you ...
	*/

	uart_insert_char(port, status, STATUS_OVERRUN_BIT, ch, flag);

For an example, see receive_chars() in 8250.c.

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

Re: [PATCH] serial: serial_txx9 driver update

[Atsushi Nemoto]

>>>>> On Mon, 23 Jan 2006 19:49:30 +0000, Russell King <rmk+lkml@arm.linux.org.uk> said:
>> Yes, in this case I will read ABCDEFG without error, and then H
>> with an overrun error.  But the UART still hold "I" in its "read
>> buffer".  The "read buffer" is exist outside the receiver FIFO.  So
>> if 'J' comes in later, I will read "IJ".  There is no way to clear
>> the "read buffer" except resetting the UART.

rmk> Ok, so if someone sent you ABCDEFGHIJ, all before you could read
rmk> anything from the UART, where I causes an overrun, you'll read
rmk> ABCDEFGHJ, but the status associated with H will indicate an
rmk> overrun condition?

Partially incorrect.  With or without the patch, "H" will indicate an
overrun.

If someone sent me "ABCDEFGHI(...long delay...)J", all before I could
read "ABCDEFGH", NUL with TTY_OVERRUN, then "IJ".  With the patch, I
will read "ABCDEFGH", NUL with TTY_OVERRUN, then "J".

I do not want to read "I" after long delay.  Dropping the "I" is not a
problem while I can know that an overrun happened there.

rmk> Your overrun behaviour is near enough to typical 8250 behaviour
rmk> that you can use the helper provided - uart_insert_char().  This
rmk> eliminates the special flag handling you seem to have created.

I suppose typical 8250 will drop "I".  The TXX9 UART does not (it is
described in the datasheet.  Not a bug).

I have been using uart_insert_char() and it helps me to pass NUL (with
TTY_OVERRUN) after "H" char.  But the it is not enough to handle this
queer behavior.

---
Atsushi Nemoto

[PATCH] serial: serial_txx9 driver update

[Atsushi Nemoto]

Update the serial_txx9 driver.

 * Configurable manumum port number. (SERIAL_TXX9_NR_UARTS)
 * Remove some code which is unneeded if CONFIG_PM=n.
 * Use PCI_DEVICE() for pci device id table and make it const.
 * Do not include <asm/irq.h>

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
 Kconfig       |    5 +++++
 serial_txx9.c |   23 +++++++++++------------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 2978c09..5cc6b91 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -916,6 +916,11 @@ config SERIAL_TXX9
 config HAS_TXX9_SERIAL
 	bool
 
+config SERIAL_TXX9_NR_UARTS
+	int "Maximum number of TMPTX39XX/49XX SIO ports"
+	depends on SERIAL_TXX9
+	default "6"
+
 config SERIAL_TXX9_CONSOLE
 	bool "TMPTX39XX/49XX SIO Console support"
 	depends on SERIAL_TXX9=y
diff --git a/drivers/serial/serial_txx9.c b/drivers/serial/serial_txx9.c
index 7186a82..f4440d3 100644
--- a/drivers/serial/serial_txx9.c
+++ b/drivers/serial/serial_txx9.c
@@ -37,6 +37,7 @@
  *	1.06	Do not insert a char caused previous overrun.
  *		Fix some spin_locks.
  *		Do not call uart_add_one_port for absent ports.
+ *	1.07	Use CONFIG_SERIAL_TXX9_NR_UARTS.  Cleanup.
  */
 
 #if defined(CONFIG_SERIAL_TXX9_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
@@ -58,9 +59,8 @@ #include <linux/serial.h>
 #include <linux/mutex.h>
 
 #include <asm/io.h>
-#include <asm/irq.h>
 
-static char *serial_version = "1.06";
+static char *serial_version = "1.07";
 static char *serial_name = "TX39/49 Serial driver";
 
 #define PASS_LIMIT	256
@@ -88,12 +88,7 @@ #endif
 /*
  * Number of serial ports
  */
-#ifdef ENABLE_SERIAL_TXX9_PCI
-#define NR_PCI_BOARDS	4
-#define UART_NR  (4 + NR_PCI_BOARDS)
-#else
-#define UART_NR  4
-#endif
+#define UART_NR  CONFIG_SERIAL_TXX9_NR_UARTS
 
 #define HIGH_BITS_OFFSET	((sizeof(long)-sizeof(int))*8)
 
@@ -987,6 +982,7 @@ int __init early_serial_txx9_setup(struc
 }
 
 #ifdef ENABLE_SERIAL_TXX9_PCI
+#ifdef CONFIG_PM
 /**
  *	serial_txx9_suspend_port - suspend one serial port
  *	@line:  serial line number
@@ -1008,6 +1004,7 @@ static void serial_txx9_resume_port(int 
 {
 	uart_resume_port(&serial_txx9_reg, &serial_txx9_ports[line].port);
 }
+#endif
 
 static DEFINE_MUTEX(serial_txx9_mutex);
 
@@ -1118,6 +1115,7 @@ static void __devexit pciserial_txx9_rem
 	}
 }
 
+#ifdef CONFIG_PM
 static int pciserial_txx9_suspend_one(struct pci_dev *dev, pm_message_t state)
 {
 	int line = (int)(long)pci_get_drvdata(dev);
@@ -1142,11 +1140,10 @@ static int pciserial_txx9_resume_one(str
 	}
 	return 0;
 }
+#endif
 
-static struct pci_device_id serial_txx9_pci_tbl[] = {
-	{	PCI_VENDOR_ID_TOSHIBA_2, PCI_DEVICE_ID_TOSHIBA_TC86C001_MISC,
-		PCI_ANY_ID, PCI_ANY_ID,
-		0, 0, 0 },
+static const struct pci_device_id serial_txx9_pci_tbl[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA_2, PCI_DEVICE_ID_TOSHIBA_TC86C001_MISC) },
 	{ 0, }
 };
 
@@ -1154,8 +1151,10 @@ static struct pci_driver serial_txx9_pci
 	.name		= "serial_txx9",
 	.probe		= pciserial_txx9_init_one,
 	.remove		= __devexit_p(pciserial_txx9_remove_one),
+#ifdef CONFIG_PM
 	.suspend	= pciserial_txx9_suspend_one,
 	.resume		= pciserial_txx9_resume_one,
+#endif
 	.id_table	= serial_txx9_pci_tbl,
 };