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 <[email protected]>
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);
}
Atsushi Nemoto <[email protected]> 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?
On Sat, Jan 21, 2006 at 11:36:49PM -0800, Andrew Morton wrote:
> Atsushi Nemoto <[email protected]> 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
Russell King <[email protected]> wrote:
>
> On Sat, Jan 21, 2006 at 11:36:49PM -0800, Andrew Morton wrote:
> > Atsushi Nemoto <[email protected]> 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.
On Sun, Jan 22, 2006 at 12:33:07AM -0800, Andrew Morton wrote:
> Russell King <[email protected]> wrote:
> >
> > On Sat, Jan 21, 2006 at 11:36:49PM -0800, Andrew Morton wrote:
> > > Atsushi Nemoto <[email protected]> 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
>>>>> On Sun, 22 Jan 2006 23:02:09 +0000, Russell King <[email protected]> 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 <[email protected]>
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);
}
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
>>>>> On Mon, 23 Jan 2006 09:57:00 +0000, Russell King <[email protected]> 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
>>>>> On Mon, 23 Jan 2006 22:39:43 +0900 (JST), Atsushi Nemoto <[email protected]> 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
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
>>>>> On Mon, 23 Jan 2006 19:49:30 +0000, Russell King <[email protected]> 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