serial: Patches for OCTEON CPU support.

PATCH In review David Daney 2 participants 9 messages Nov 26, 2008 – Dec 2, 2008

Subsystems (3)

MIPS TTY layer and serial drivers 8250/16?50 (and clone uarts) serial driver

lore

Share & tools

lore

David Daney

author

2008-11-26 00:15 UTC #0 lore raw

cover serial: Patches for OCTEON CPU support.

Previously I have posted these five patches as part of the larger CPU support patch set. I am separating them out now as I have incorporated changes as suggested by Alan Cox, and these are ready for review now.

Changes from previous version:

* I/O functions now called serial_{in,out}.

* UART_BUG_OCTEON_IIR workaround moved to OCTEON serial_in function and out of this patch set.

Since these are the only non-mips specific patches required for OCTEON cpu support, it is necessary to get these Signed-off-by before the full OCTEON patch set can proceed. Also since serial is lacking active maintainers, I am sending directly to akpm and Alan in hopes that one or more of them will review the patches.

Five patches to follow.

David Daney (5):
8250: Don't clobber spinlocks.
8250: Serial driver changes to support future Cavium OCTEON serial
patches.
Serial: Allow port type to be specified when calling

    serial8250_register_port.

8250: Allow port type to specify bugs that are not probed for.
Serial: UART driver changes for Cavium OCTEON.

 drivers/serial/8250.c        |  230 
+++++++++++++++++++++++++++++++-----------
 drivers/serial/8250.h        |    2 +
 drivers/serial/serial_core.c |    7 +-
 include/linux/serial_8250.h  |    3 +
 include/linux/serial_core.h  |    7 +-
 5 files changed, 185 insertions(+), 64 deletions(-)

David Daney

author

2008-11-26 00:18 UTC #1 lore raw

PATCH 1/5 8250: Don't clobber spinlocks.

In serial8250_isa_init_ports(), the port's lock is initialized. We should not overwrite it. In early_serial_setup(), only copy in the fields we need. Since the early console code only uses a subset of the fields, these are sufficient.

Signed-off-by David Daney <[email protected]>

Signed-off-by Tomaso Paoletti <[email protected]>

---
 drivers/serial/8250.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

drivers/serial/8250.c +13 -2

index 303272a..8e28750 100644
@@ -2752,12 +2752,23 @@ static struct uart_driver serial8250_reg = {
  */
 int __init early_serial_setup(struct uart_port *port)
 {
+	struct uart_port *p;
+
 	if (port->line >= ARRAY_SIZE(serial8250_ports))
 		return -ENODEV;
 
 	serial8250_isa_init_ports();
-	serial8250_ports[port->line].port	= *port;
-	serial8250_ports[port->line].port.ops	= &serial8250_pops;
+	p = &serial8250_ports[port->line].port;
+	p->iobase       = port->iobase;
+	p->membase      = port->membase;
+	p->irq          = port->irq;
+	p->uartclk      = port->uartclk;
+	p->fifosize     = port->fifosize;
+	p->regshift     = port->regshift;
+	p->iotype       = port->iotype;
+	p->flags        = port->flags;
+	p->mapbase      = port->mapbase;
+	p->private_data = port->private_data;
 	return 0;
 }

Signature · 1 line

1.5.6.5

David Daney

author

2008-11-26 00:18 UTC #2 lore raw

PATCH 2/5 8250: Serial driver changes to support future Cavium OCTEON serial patches.

In reply to David Daney

In order to use Cavium OCTEON specific serial i/o drivers, we first patch the 8250 driver to use replaceable I/O functions. Compatible I/O functions are added for existing iotypeS.

An added benefit of this change is that it makes it easy to factor some of the existing special cases out to board/SOC specific support code.

The alternative is to load up 8250.c with a bunch of OCTEON specific iotype code and bug work-arounds.

Signed-off-by David Daney <[email protected]>

Signed-off-by Tomaso Paoletti <[email protected]>

---
 drivers/serial/8250.c       |  194 ++++++++++++++++++++++++++++++-------------
 include/linux/serial_8250.h |    2 +
 include/linux/serial_core.h |    2 +
 3 files changed, 140 insertions(+), 58 deletions(-)

drivers/serial/8250.c +136 ~~-58~~

index 8e28750..849af9d 100644
@@ -303,16 +303,16 @@ static const u8 au_io_out_map[] = {
 };
 
 /* sane hardware needs no mapping */
-static inline int map_8250_in_reg(struct uart_8250_port *up, int offset)
+static inline int map_8250_in_reg(struct uart_port *p, int offset)
 {
-	if (up->port.iotype != UPIO_AU)
+	if (p->iotype != UPIO_AU)
 		return offset;
 	return au_io_in_map[offset];
 }
 
-static inline int map_8250_out_reg(struct uart_8250_port *up, int offset)
+static inline int map_8250_out_reg(struct uart_port *p, int offset)
 {
-	if (up->port.iotype != UPIO_AU)
+	if (p->iotype != UPIO_AU)
 		return offset;
 	return au_io_out_map[offset];
 }

@@ -341,16 +341,16 @@ static const u8
 		[UART_SCR]	= 0x2c
 	};
 
-static inline int map_8250_in_reg(struct uart_8250_port *up, int offset)
+static inline int map_8250_in_reg(struct uart_port *p, int offset)
 {
-	if (up->port.iotype != UPIO_RM9000)
+	if (p->iotype != UPIO_RM9000)
 		return offset;
 	return regmap_in[offset];
 }
 
-static inline int map_8250_out_reg(struct uart_8250_port *up, int offset)
+static inline int map_8250_out_reg(struct uart_port *p, int offset)
 {
-	if (up->port.iotype != UPIO_RM9000)
+	if (p->iotype != UPIO_RM9000)
 		return offset;
 	return regmap_out[offset];
 }

@@ -363,108 +363,170 @@ static inline int map_8250_out_reg(struct uart_8250_port *up, int offset)
 
 #endif
 
-static unsigned int serial_in(struct uart_8250_port *up, int offset)
+static unsigned int hub6_serial_in(struct uart_port *p, int offset)
 {
-	unsigned int tmp;
-	offset = map_8250_in_reg(up, offset) << up->port.regshift;
+	offset = map_8250_in_reg(p, offset) << p->regshift;
+	outb(p->hub6 - 1 + offset, p->iobase);
+	return inb(p->iobase + 1);
+}
 
-	switch (up->port.iotype) {
-	case UPIO_HUB6:
-		outb(up->port.hub6 - 1 + offset, up->port.iobase);
-		return inb(up->port.iobase + 1);
+static void hub6_serial_out(struct uart_port *p, int offset, int value)
+{
+	offset = map_8250_out_reg(p, offset) << p->regshift;
+	outb(p->hub6 - 1 + offset, p->iobase);
+	outb(value, p->iobase + 1);
+}
 
-	case UPIO_MEM:
-	case UPIO_DWAPB:
-		return readb(up->port.membase + offset);
+static unsigned int mem_serial_in(struct uart_port *p, int offset)
+{
+	offset = map_8250_in_reg(p, offset) << p->regshift;
+	return readb(p->membase + offset);
+}
 
-	case UPIO_RM9000:
-	case UPIO_MEM32:
-		return readl(up->port.membase + offset);
+static void mem_serial_out(struct uart_port *p, int offset, int value)
+{
+	offset = map_8250_out_reg(p, offset) << p->regshift;
+	writeb(value, p->membase + offset);
+}
+
+static void mem32_serial_out(struct uart_port *p, int offset, int value)
+{
+	offset = map_8250_out_reg(p, offset) << p->regshift;
+	writel(value, p->membase + offset);
+}
+
+static unsigned int mem32_serial_in(struct uart_port *p, int offset)
+{
+	offset = map_8250_in_reg(p, offset) << p->regshift;
+	return readl(p->membase + offset);
+}
 
 #ifdef CONFIG_SERIAL_8250_AU1X00
-	case UPIO_AU:
-		return __raw_readl(up->port.membase + offset);
+static unsigned int au_serial_in(struct uart_port *p, int offset)
+{
+	offset = map_8250_in_reg(p, offset) << p->regshift;
+	return __raw_readl(p->membase + offset);
+}
+
+static void au_serial_out(struct uart_port *p, int offset, int value)
+{
+	offset = map_8250_out_reg(p, offset) << p->regshift;
+	__raw_writel(value, p->membase + offset);
+}
 #endif
 
-	case UPIO_TSI:
-		if (offset == UART_IIR) {
-			tmp = readl(up->port.membase + (UART_IIR & ~3));
-			return (tmp >> 16) & 0xff; /* UART_IIR % 4 == 2 */
-		} else
-			return readb(up->port.membase + offset);
+static unsigned int tsi_serial_in(struct uart_port *p, int offset)
+{
+	unsigned int tmp;
+	offset = map_8250_in_reg(p, offset) << p->regshift;
+	if (offset == UART_IIR) {
+		tmp = readl(p->membase + (UART_IIR & ~3));
+		return (tmp >> 16) & 0xff; /* UART_IIR % 4 == 2 */
+	} else
+		return readb(p->membase + offset);
+}
 
-	default:
-		return inb(up->port.iobase + offset);
-	}
+static void tsi_serial_out(struct uart_port *p, int offset, int value)
+{
+	offset = map_8250_out_reg(p, offset) << p->regshift;
+	if (!((offset == UART_IER) && (value & UART_IER_UUE)))
+		writeb(value, p->membase + offset);
 }
 
-static void
-serial_out(struct uart_8250_port *up, int offset, int value)
+static void dwapb_serial_out(struct uart_port *p, int offset, int value)
 {
-	/* Save the offset before it's remapped */
 	int save_offset = offset;
-	offset = map_8250_out_reg(up, offset) << up->port.regshift;
+	offset = map_8250_out_reg(p, offset) << p->regshift;
+	/* Save the LCR value so it can be re-written when a
+	 * Busy Detect interrupt occurs. */
+	if (save_offset == UART_LCR) {
+		struct uart_8250_port *up = (struct uart_8250_port *)p;
+		up->lcr = value;
+	}
+	writeb(value, p->membase + offset);
+	/* Read the IER to ensure any interrupt is cleared before
+	 * returning from ISR. */
+	if (save_offset == UART_TX || save_offset == UART_IER)
+		value = p->serial_in(p, UART_IER);
+}
 
-	switch (up->port.iotype) {
+static unsigned int io_serial_in(struct uart_port *p, int offset)
+{
+	offset = map_8250_in_reg(p, offset) << p->regshift;
+	return inb(p->iobase + offset);
+}
+
+static void io_serial_out(struct uart_port *p, int offset, int value)
+{
+	offset = map_8250_out_reg(p, offset) << p->regshift;
+	outb(value, p->iobase + offset);
+}
+
+static void set_io_from_upio(struct uart_port *p)
+{
+	switch (p->iotype) {
 	case UPIO_HUB6:
-		outb(up->port.hub6 - 1 + offset, up->port.iobase);
-		outb(value, up->port.iobase + 1);
+		p->serial_in = hub6_serial_in;
+		p->serial_out = hub6_serial_out;
 		break;
 
 	case UPIO_MEM:
-		writeb(value, up->port.membase + offset);
+		p->serial_in = mem_serial_in;
+		p->serial_out = mem_serial_out;
 		break;
 
 	case UPIO_RM9000:
 	case UPIO_MEM32:
-		writel(value, up->port.membase + offset);
+		p->serial_in = mem32_serial_in;
+		p->serial_out = mem32_serial_out;
 		break;
 
 #ifdef CONFIG_SERIAL_8250_AU1X00
 	case UPIO_AU:
-		__raw_writel(value, up->port.membase + offset);
+		p->serial_in = au_serial_in;
+		p->serial_out = au_serial_out;
 		break;
 #endif
 	case UPIO_TSI:
-		if (!((offset == UART_IER) && (value & UART_IER_UUE)))
-			writeb(value, up->port.membase + offset);
+		p->serial_in = tsi_serial_in;
+		p->serial_out = tsi_serial_out;
 		break;
 
 	case UPIO_DWAPB:
-		/* Save the LCR value so it can be re-written when a
-		 * Busy Detect interrupt occurs. */
-		if (save_offset == UART_LCR)
-			up->lcr = value;
-		writeb(value, up->port.membase + offset);
-		/* Read the IER to ensure any interrupt is cleared before
-		 * returning from ISR. */
-		if (save_offset == UART_TX || save_offset == UART_IER)
-			value = serial_in(up, UART_IER);
+		p->serial_in = mem_serial_in;
+		p->serial_out = dwapb_serial_out;
 		break;
 
 	default:
-		outb(value, up->port.iobase + offset);
+		p->serial_in = io_serial_in;
+		p->serial_out = io_serial_out;
+		break;
 	}
 }
 
 static void
 serial_out_sync(struct uart_8250_port *up, int offset, int value)
 {
-	switch (up->port.iotype) {
+	struct uart_port *p = &up->port;
+	switch (p->iotype) {
 	case UPIO_MEM:
 	case UPIO_MEM32:
 #ifdef CONFIG_SERIAL_8250_AU1X00
 	case UPIO_AU:
 #endif
 	case UPIO_DWAPB:
-		serial_out(up, offset, value);
-		serial_in(up, UART_LCR);	/* safe, no side-effects */
+		p->serial_out(p, offset, value);
+		p->serial_in(p, UART_LCR);	/* safe, no side-effects */
 		break;
 	default:
-		serial_out(up, offset, value);
+		p->serial_out(p, offset, value);
 	}
 }
 
+#define serial_in(up, offset)		\
+	(up->port.serial_in(&(up)->port, (offset)))
+#define serial_out(up, offset, value)	\
+	(up->port.serial_out(&(up)->port, (offset), (value)))
 /*
  * We used to support using pause I/O for certain machines.  We
  * haven't supported this for a while, but just in case it's badly

@@ -2576,6 +2638,7 @@ static void __init serial8250_isa_init_ports(void)
 		up->port.membase  = old_serial_port[i].iomem_base;
 		up->port.iotype   = old_serial_port[i].io_type;
 		up->port.regshift = old_serial_port[i].iomem_reg_shift;
+		set_io_from_upio(&up->port);
 		if (share_irqs)
 			up->port.flags |= UPF_SHARE_IRQ;
 	}

@@ -2769,6 +2832,13 @@ int __init early_serial_setup(struct uart_port *port)
 	p->flags        = port->flags;
 	p->mapbase      = port->mapbase;
 	p->private_data = port->private_data;
+
+	set_io_from_upio(p);
+	if (port->serial_in)
+		p->serial_in = port->serial_in;
+	if (port->serial_out)
+		p->serial_out = port->serial_out;
+
 	return 0;
 }

@@ -2833,6 +2903,8 @@ static int __devinit serial8250_probe(struct platform_device *dev)
 		port.mapbase		= p->mapbase;
 		port.hub6		= p->hub6;
 		port.private_data	= p->private_data;
+		port.serial_in		= p->serial_in;
+		port.serial_out		= p->serial_out;
 		port.dev		= &dev->dev;
 		if (share_irqs)
 			port.flags |= UPF_SHARE_IRQ;

@@ -2986,6 +3058,12 @@ int serial8250_register_port(struct uart_port *port)
 		uart->port.private_data = port->private_data;
 		if (port->dev)
 			uart->port.dev = port->dev;
+		set_io_from_upio(&uart->port);
+		/* Possibly override default I/O functions.  */
+		if (port->serial_in)
+			uart->port.serial_in = port->serial_in;
+		if (port->serial_out)
+			uart->port.serial_out = port->serial_out;
 
 		ret = uart_add_one_port(&serial8250_reg, &uart->port);
 		if (ret == 0)

include/linux/serial_8250.h +2 -0

index 3d37c94..77d83d9 100644
@@ -28,6 +28,8 @@ struct plat_serial8250_port {
 	unsigned char	iotype;		/* UPIO_* */
 	unsigned char	hub6;
 	upf_t		flags;		/* UPF_* flags */
+	unsigned int	(*serial_in)(struct uart_port *, int);
+	void		(*serial_out)(struct uart_port *, int, int);
 };
 
 /*

include/linux/serial_core.h +2 -0

index 4e4f127..2d22c67 100644
@@ -246,6 +246,8 @@ struct uart_port {
 	spinlock_t		lock;			/* port lock */
 	unsigned long		iobase;			/* in/out[bwl] */
 	unsigned char __iomem	*membase;		/* read/write[bwl] */
+	unsigned int		(*serial_in)(struct uart_port *, int);
+	void			(*serial_out)(struct uart_port *, int, int);
 	unsigned int		irq;			/* irq number */
 	unsigned int		uartclk;		/* base uart clock */
 	unsigned int		fifosize;		/* tx fifo size */

Signature · 1 line

1.5.6.5

David Daney

author

2008-11-26 00:19 UTC #3 lore raw

PATCH 3/5 Serial: Allow port type to be specified when calling serial8250_register_port.

In reply to David Daney

Add flag value UPF_FIXED_TYPE which specifies that the UART type is known and should not be probed. For this case the UARTs properties are just copied out of the uart_config entry.

This allows us to keep SOC specific 8250 probe code out of 8250.c. In this case we know the serial hardware will not be changing as it is on the same silicon as the CPU, and we can specify it with certainty in the board/cpu setup code.

The alternative is to load up 8250.c with a bunch of OCTEON specific special cases in the probing code.

Signed-off-by David Daney <[email protected]>

---
 drivers/serial/8250.c        |    9 +++++++++
 drivers/serial/serial_core.c |    7 +++++--
 include/linux/serial_8250.h  |    1 +
 include/linux/serial_core.h  |    2 ++
 4 files changed, 17 insertions(+), 2 deletions(-)

drivers/serial/8250.c +9 -0

index 849af9d..3ae4974 100644
@@ -2903,6 +2903,7 @@ static int __devinit serial8250_probe(struct platform_device *dev)
 		port.mapbase		= p->mapbase;
 		port.hub6		= p->hub6;
 		port.private_data	= p->private_data;
+		port.type		= p->type;
 		port.serial_in		= p->serial_in;
 		port.serial_out		= p->serial_out;
 		port.dev		= &dev->dev;

@@ -3058,6 +3059,14 @@ int serial8250_register_port(struct uart_port *port)
 		uart->port.private_data = port->private_data;
 		if (port->dev)
 			uart->port.dev = port->dev;
+
+		if (port->flags & UPF_FIXED_TYPE) {
+			uart->port.type = port->type;
+			uart->port.fifosize = uart_config[port->type].fifo_size;
+			uart->capabilities = uart_config[port->type].flags;
+			uart->tx_loadsz = uart_config[port->type].tx_loadsz;
+		}
+
 		set_io_from_upio(&uart->port);
 		/* Possibly override default I/O functions.  */
 		if (port->serial_in)

drivers/serial/serial_core.c +5 -2

index 874786a..7fef45e 100644
@@ -2198,11 +2198,14 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 	 * Now do the auto configuration stuff.  Note that config_port
 	 * is expected to claim the resources and map the port for us.
 	 */
-	flags = UART_CONFIG_TYPE;
+	flags = 0;
 	if (port->flags & UPF_AUTO_IRQ)
 		flags |= UART_CONFIG_IRQ;
 	if (port->flags & UPF_BOOT_AUTOCONF) {
-		port->type = PORT_UNKNOWN;
+		if (!(port->flags & UPF_FIXED_TYPE)) {
+			port->type = PORT_UNKNOWN;
+			flags |= UART_CONFIG_TYPE;
+		}
 		port->ops->config_port(port, flags);
 	}

include/linux/serial_8250.h +1 -0

index 77d83d9..d4d2a78 100644
@@ -28,6 +28,7 @@ struct plat_serial8250_port {
 	unsigned char	iotype;		/* UPIO_* */
 	unsigned char	hub6;
 	upf_t		flags;		/* UPF_* flags */
+	unsigned int	type;		/* If UPF_FIXED_TYPE */
 	unsigned int	(*serial_in)(struct uart_port *, int);
 	void		(*serial_out)(struct uart_port *, int, int);
 };

include/linux/serial_core.h +2 -0

index 2d22c67..b9e7756 100644
@@ -293,6 +293,8 @@ struct uart_port {
 #define UPF_MAGIC_MULTIPLIER	((__force upf_t) (1 << 16))
 #define UPF_CONS_FLOW		((__force upf_t) (1 << 23))
 #define UPF_SHARE_IRQ		((__force upf_t) (1 << 24))
+/* The exact UART type is known and should not be probed.  */
+#define UPF_FIXED_TYPE		((__force upf_t) (1 << 27))
 #define UPF_BOOT_AUTOCONF	((__force upf_t) (1 << 28))
 #define UPF_FIXED_PORT		((__force upf_t) (1 << 29))
 #define UPF_DEAD		((__force upf_t) (1 << 30))

Signature · 1 line

1.5.6.5

David Daney

author

2008-11-26 00:18 UTC #4 lore raw

PATCH 4/5 8250: Allow port type to specify bugs that are not probed for.

In reply to David Daney

Add a bugs field to the serial8250_config and propagate it to the port's bugs field when the port is registered and configured.

This is a follow on to the previous patch. Now that we can specify the port type and don't have to probe for it, we can avoid probing for bugs if they are known with certainty to exist.

The alternative is to load up 8250.c with a bunch of OCTEON specific special cases in the probing code.

Signed-off-by David Daney <[email protected]>

---
 drivers/serial/8250.c |    2 ++
 drivers/serial/8250.h |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

drivers/serial/8250.c +2 -0

index 3ae4974..3bb8e30 100644
@@ -1212,6 +1212,7 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
 	up->port.fifosize = uart_config[up->port.type].fifo_size;
 	up->capabilities = uart_config[up->port.type].flags;
 	up->tx_loadsz = uart_config[up->port.type].tx_loadsz;
+	up->bugs |= uart_config[up->port.type].bugs;
 
 	if (up->port.type == PORT_UNKNOWN)
 		goto out;

@@ -3065,6 +3066,7 @@ int serial8250_register_port(struct uart_port *port)
 			uart->port.fifosize = uart_config[port->type].fifo_size;
 			uart->capabilities = uart_config[port->type].flags;
 			uart->tx_loadsz = uart_config[port->type].tx_loadsz;
+			uart->bugs = uart_config[port->type].bugs;
 		}
 
 		set_io_from_upio(&uart->port);

drivers/serial/8250.h +1 -0

index 5202603..c9b3002 100644
@@ -34,6 +34,7 @@ struct serial8250_config {
 	const char	*name;
 	unsigned short	fifo_size;
 	unsigned short	tx_loadsz;
+	unsigned short	bugs;
 	unsigned char	fcr;
 	unsigned int	flags;
 };

Signature · 1 line

1.5.6.5

David Daney

author

2008-11-26 00:18 UTC #5 lore raw

PATCH 5/5 Serial: UART driver changes for Cavium OCTEON.

In reply to David Daney

Cavium UART implementation won't work with the standard 8250 driver as-is. Define a new uart_config (PORT_OCTEON) and use it to enable special handling required by the OCTEON's serial port. Two new bug types are defined.

Signed-off-by Tomaso Paoletti <[email protected]>

Signed-off-by David Daney <[email protected]>

---
 drivers/serial/8250.c       |   10 +++++++++-
 drivers/serial/8250.h       |    1 +
 include/linux/serial_core.h |    3 ++-
 3 files changed, 12 insertions(+), 2 deletions(-)

drivers/serial/8250.c +9 -1

index 3bb8e30..63dbbb6 100644
@@ -279,6 +279,14 @@ static const struct serial8250_config uart_config[] = {
 		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
 		.flags		= UART_CAP_FIFO,
 	},
+	[PORT_OCTEON] = {
+		.name		= "OCTEON",
+		.fifo_size	= 64,
+		.tx_loadsz	= 64,
+		.bugs		= UART_BUG_TIMEOUT,
+		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
+		.flags		= UART_CAP_FIFO,
+	},
 };
 
 #if defined (CONFIG_SERIAL_8250_AU1X00)

@@ -1714,7 +1722,7 @@ static void serial8250_timeout(unsigned long data)
 	unsigned int iir;
 
 	iir = serial_in(up, UART_IIR);
-	if (!(iir & UART_IIR_NO_INT))
+	if (!(iir & UART_IIR_NO_INT) || (up->bugs & UART_BUG_TIMEOUT))
 		serial8250_handle_port(up);
 	mod_timer(&up->timer, jiffies + poll_timeout(up->port.timeout));
 }

drivers/serial/8250.h +1 -0

index c9b3002..17dc459 100644
@@ -49,6 +49,7 @@ struct serial8250_config {
 #define UART_BUG_TXEN	(1 << 1)	/* UART has buggy TX IIR status */
 #define UART_BUG_NOMSR	(1 << 2)	/* UART has buggy MSR status bits (Au1x00) */
 #define UART_BUG_THRE	(1 << 3)	/* UART has buggy THRE reassertion */
+#define UART_BUG_TIMEOUT (1 << 4)	/* UART should always handle timeout */
 
 #define PROBE_RSA	(1 << 0)
 #define PROBE_ANY	(~0)

include/linux/serial_core.h +2 -1

index b9e7756..fd772b5 100644
@@ -40,7 +40,8 @@
 #define PORT_NS16550A	14
 #define PORT_XSCALE	15
 #define PORT_RM9000	16	/* PMC-Sierra RM9xxx internal UART */
-#define PORT_MAX_8250	16	/* max port ID */
+#define PORT_OCTEON	17	/* Cavium OCTEON internal UART */
+#define PORT_MAX_8250	17	/* max port ID */
 
 /*
  * ARM specific type numbers.  These are not currently guaranteed

Signature · 1 line

1.5.6.5

Shinya Kuribayashi

2008-11-28 03:27 UTC #6 lore raw

Re: [PATCH 5/5] Serial: UART driver changes for Cavium OCTEON.

Hi David,

Quoted 4 lines · David Daney

Cavium UART implementation won't work with the standard 8250 driver as-is. Define a new uart_config (PORT_OCTEON) and use it to enable special handling required by the OCTEON's serial port. Two new bug types are defined.

You just added one new bug type, not two. ;-)

Quoted 9 lines

drivers/serial/8250.c +1 -0

index 3bb8e30..63dbbb6 100644
@@ -279,6 +279,14 @@ static const struct serial8250_config uart_config[] = {
 		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
 		.flags		= UART_CAP_FIFO,
 	},
+	[PORT_OCTEON] = {

drivers/serial/8250.c static const struct serial8250_config uart_config[] = { · line 292

Quoted 10 lines

drivers/serial/8250.c +7 -0

+		.name		= "OCTEON",
+		.fifo_size	= 64,
+		.tx_loadsz	= 64,
+		.bugs		= UART_BUG_TIMEOUT,
+		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
+		.flags		= UART_CAP_FIFO,
+	},
 };
 
 #if defined (CONFIG_SERIAL_8250_AU1X00)

I see your point. Then let's look at this commit:

|commit 40b36daad0ac704e6d5c1b75789f371ef5b053c1
|Author: Alex Williamson <[email protected]>
|Date:   Wed Feb 14 00:33:04 2007 -0800
|
|    [PATCH] 8250 UART backup timer
|
|    The patch below works around a minor bug found in the UART of the remote
|    management card used in many HP ia64 and parisc servers (aka the Diva
|    UARTs).  The problem is that the UART does not reassert the THRE interrupt
|    if it has been previously cleared and the IIR THRI bit is re-enabled.  This
|    can produce a very annoying failure mode when used as a serial console,
|    allowing a boot/reboot to hang indefinitely until an RX interrupt kicks it
|    into working again (ie.  an unattended reboot could stall).
|
|    To solve this problem, a backup timer is introduced that runs alongside the
|    standard interrupt driven mechanism.  This timer wakes up periodically,
|    checks for a hang condition and gets characters moving again.  This backup
|    mechanism is only enabled if the UART is detected as having this problem,
|    so systems without these UARTs will have no additional overhead.
|
|    This version of the patch incorporates previous comments from Pavel and
|    removes races in the bug detection code.  The test is now done before the
|    irq linking to prevent races with interrupt handler clearing the THRE
|    interrupt.  Short delays and syncs are also added to ensure the device is
|    able to update register state before the result is tested.
|
|    Aristeu says:
|
|      this was tested on the following HP machines and solved the problem:
|      rx2600, rx2620, rx1600 and rx1620s.
|
|    hpa says:
|
|      I have seen this same bug in soft UART IP from "a major vendor."
|
|    Signed-off-by: Alex Williamson <[email protected]>
|    Cc: "H. Peter Anvin" <[email protected]>
|    Cc: Russell King <[email protected]>
|    Acked-by: Aristeu Sergio Rozanski Filho <[email protected]>
|    Signed-off-by: Andrew Morton <[email protected]>
|    Signed-off-by: Linus Torvalds <[email protected]>

AFAICS this patch tries to handle the same problem, and added reasonable framework with serial8250_backup_timeout, etc. And now we have UART_BUG _THRE for this issue. Then it might be better to use existing codes as much as possible.

Just random thoughts,

- could we dynamically probe this bug in serial8250_startup as well?

- is there any possibility this hardware works with an existing serial8250_backup_timeout, not doing this below:

drivers/serial/8250.c static void serial8250_timeout(unsigned long data) · line 1728

Quoted 9 lines

drivers/serial/8250.c +1 -1

@@ -1714,7 +1722,7 @@ static void serial8250_timeout(unsigned long data)
 	unsigned int iir;
 
 	iir = serial_in(up, UART_IIR);
-	if (!(iir & UART_IIR_NO_INT))
+	if (!(iir & UART_IIR_NO_INT) || (up->bugs & UART_BUG_TIMEOUT))
 		serial8250_handle_port(up);
 	mod_timer(&up->timer, jiffies + poll_timeout(up->port.timeout));
 }

I'm not an UART expert, any feedback is highly appreciated.

Thanks,

Signature · 2 lines

Shinya Kuribayashi
NEC Electronics

David Daney

author

2008-12-01 18:43 UTC #7 lore raw

Re: [PATCH 5/5] Serial: UART driver changes for Cavium OCTEON.

Quoted 10 lines · Shinya Kuribayashi

Hi David,
Quoted 4 lines · David Daney
Cavium UART implementation won't work with the standard 8250 driver as-is. Define a new uart_config (PORT_OCTEON) and use it to enable special handling required by the OCTEON's serial port. Two new bug types are defined.
You just added one new bug type, not two. ;-)

Unfortunately I overlooked the patch comment when I moved the other bug
workaround to platform code.

However after your suggestion below...
[...]

Quoted 59 lines

I see your point. Then let's look at this commit:

|commit 40b36daad0ac704e6d5c1b75789f371ef5b053c1
|Author: Alex Williamson <[email protected]>
|Date:   Wed Feb 14 00:33:04 2007 -0800
|
|    [PATCH] 8250 UART backup timer
|
|    The patch below works around a minor bug found in the UART of the remote
|    management card used in many HP ia64 and parisc servers (aka the Diva
|    UARTs).  The problem is that the UART does not reassert the THRE interrupt
|    if it has been previously cleared and the IIR THRI bit is re-enabled.  This
|    can produce a very annoying failure mode when used as a serial console,
|    allowing a boot/reboot to hang indefinitely until an RX interrupt kicks it
|    into working again (ie.  an unattended reboot could stall).
|
|    To solve this problem, a backup timer is introduced that runs alongside the
|    standard interrupt driven mechanism.  This timer wakes up periodically,
|    checks for a hang condition and gets characters moving again.  This backup
|    mechanism is only enabled if the UART is detected as having this problem,
|    so systems without these UARTs will have no additional overhead.
|
|    This version of the patch incorporates previous comments from Pavel and
|    removes races in the bug detection code.  The test is now done before the
|    irq linking to prevent races with interrupt handler clearing the THRE
|    interrupt.  Short delays and syncs are also added to ensure the device is
|    able to update register state before the result is tested.
|
|    Aristeu says:
|
|      this was tested on the following HP machines and solved the problem:
|      rx2600, rx2620, rx1600 and rx1620s.
|
|    hpa says:
|
|      I have seen this same bug in soft UART IP from "a major vendor."
|
|    Signed-off-by: Alex Williamson <[email protected]>
|    Cc: "H. Peter Anvin" <[email protected]>
|    Cc: Russell King <[email protected]>
|    Acked-by: Aristeu Sergio Rozanski Filho <[email protected]>
|    Signed-off-by: Andrew Morton <[email protected]>
|    Signed-off-by: Linus Torvalds <[email protected]>

Just random thoughts,

- could we dynamically probe this bug in serial8250_startup as well?

- is there any possibility this hardware works with an existing serial8250_backup_timeout, not doing this below:

Quoted 3 lines

drivers/serial/8250.c +0 -0

@@ -1714,7 +1722,7 @@ static void serial8250_timeout(unsigned long data)
 	unsigned int iir;

Quoted 9 lines

drivers/serial/8250.c static void serial8250_timeout(unsigned long data) · line 1728
Quoted 6 lines
drivers/serial/8250.c +1 -1
 	iir = serial_in(up, UART_IIR);
-	if (!(iir & UART_IIR_NO_INT))
+	if (!(iir & UART_IIR_NO_INT) || (up->bugs & UART_BUG_TIMEOUT))
 		serial8250_handle_port(up);
 	mod_timer(&up->timer, jiffies + poll_timeout(up->port.timeout));
 }
I'm not an UART expert, any feedback is highly appreciated.

... All our boards are now working with no extra bug flags needed. So I will simplify this patch set by getting rid of UART_BUG_TIMEOUT.

Thanks,

David Daney

Shinya Kuribayashi

2008-12-02 01:31 UTC #8 lore raw

Re: [PATCH 5/5] Serial: UART driver changes for Cavium OCTEON.

Quoted 1 line · David Daney

... All our boards are now working with no extra bug flags needed. So I will simplify this patch set by getting rid of UART_BUG_TIMEOUT.

Great! and makes sense. Thank you for taking care of my comments. I've also checked the serial patches of version 2, and they looks in good shape.

Signature · 2 lines

Shinya Kuribayashi
NEC Electronics