Hi Folks, We have been working with L137 and there is an issue where the serial driver handles the UART in a very bad way. This may be the same issue as described in the other two mails on this thread. The issue is that the serial driver 8250.c expects the uart to generate a fifo empty interrupt if the FIFO is empty and interrupts are enabled. The UART in the L137 only generates a FIFO empty interrupt the first time. The serial driver handles this situation by starting a timer which times out after 200ms when interrupts are enabled and only then begins to transmit. I have rewritten the interrupt handling so the driver maintains a flag which tells whether the UART is currently transmitting or not. If not then we begin filling the TX fifo immediately (we do not wait for an interrupt which never comes). This fixes the transmit latency issue for us.
I have attached the patch. It is against the linux-davinci git at kernel.org as of friday last week. I have sent the patch to the linux-serial list with no response. Could you who have problems with serial performance test the patch? I can try to send it again to linux-serial if I get good feedback from you. /Brian
From 73b3fdf65baef4d532787677dc709b6390b24d8a Mon Sep 17 00:00:00 2001 From: Brian Murphy <[email protected]> Date: Fri, 5 Feb 2010 15:59:02 +0100 Subject: [PATCH] performance fix --- drivers/serial/8250.c | 204 +++++++++---------------------------------------- drivers/serial/8250.h | 4 +- 2 files changed, 38 insertions(+), 170 deletions(-) diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c index c3e37c8..646e8ce 100644 --- a/drivers/serial/8250.c +++ b/drivers/serial/8250.c @@ -150,6 +150,7 @@ struct uart_8250_port { unsigned char lsr_saved_flags; #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA unsigned char msr_saved_flags; + unsigned char tx_active; /* * We provide a per-port pm hook. @@ -524,25 +525,6 @@ static void set_io_from_upio(struct uart_port *p) up->cur_iotype = p->iotype; } -static void -serial_out_sync(struct uart_8250_port *up, int offset, int value) -{ - 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: - p->serial_out(p, offset, value); - p->serial_in(p, UART_LCR); /* safe, no side-effects */ - break; - default: - p->serial_out(p, offset, value); - } -} - #define serial_in(up, offset) \ (up->port.serial_in(&(up)->port, (offset))) #define serial_out(up, offset, value) \ @@ -1311,14 +1293,20 @@ static inline void __stop_tx(struct uart_8250_port *p) p->ier &= ~UART_IER_THRI; serial_out(p, UART_IER, p->ier); } + p->tx_active = 0; } static void serial8250_stop_tx(struct uart_port *port) { struct uart_8250_port *up = (struct uart_8250_port *)port; + unsigned long flags; + + spin_lock_irqsave(&up->port.lock, flags); __stop_tx(up); + spin_unlock_irqrestore(&up->port.lock, flags); + /* * We really want to stop the transmitter from sending. */ @@ -1333,22 +1321,23 @@ static void transmit_chars(struct uart_8250_port *up); static void serial8250_start_tx(struct uart_port *port) { struct uart_8250_port *up = (struct uart_8250_port *)port; + unsigned long flags; + + spin_lock_irqsave(&up->port.lock, flags); if (!(up->ier & UART_IER_THRI)) { up->ier |= UART_IER_THRI; serial_out(up, UART_IER, up->ier); + } - if (up->bugs & UART_BUG_TXEN) { - unsigned char lsr; - lsr = serial_in(up, UART_LSR); - up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS; - if ((up->port.type == PORT_RM9000) ? - (lsr & UART_LSR_THRE) : - (lsr & UART_LSR_TEMT)) - transmit_chars(up); - } + if (!up->tx_active) + { + up->tx_active = 1; + transmit_chars(up); } + spin_unlock_irqrestore(&up->port.lock, flags); + /* * Re-enable the transmitter if we disabled it. */ @@ -1470,7 +1459,8 @@ static void transmit_chars(struct uart_8250_port *up) serial8250_stop_tx(&up->port); return; } - if (uart_circ_empty(xmit)) { + if (uart_circ_empty(xmit)) + { __stop_tx(up); return; } @@ -1488,9 +1478,6 @@ static void transmit_chars(struct uart_8250_port *up) uart_write_wakeup(&up->port); DEBUG_INTR("THRE..."); - - if (uart_circ_empty(xmit)) - __stop_tx(up); } static unsigned int check_modem_status(struct uart_8250_port *up) @@ -1519,12 +1506,9 @@ static unsigned int check_modem_status(struct uart_8250_port *up) /* * This handles the interrupt from one port. */ -static void serial8250_handle_port(struct uart_8250_port *up) +static void __serial8250_handle_port(struct uart_8250_port *up) { unsigned int status; - unsigned long flags; - - spin_lock_irqsave(&up->port.lock, flags); status = serial_inp(up, UART_LSR); @@ -1534,8 +1518,19 @@ static void serial8250_handle_port(struct uart_8250_port *up) receive_chars(up, &status); check_modem_status(up); if (status & UART_LSR_THRE) + { + up->tx_active = 1; transmit_chars(up); + } + +} +static void serial8250_handle_port(struct uart_8250_port *up) +{ + unsigned long flags; + + spin_lock_irqsave(&up->port.lock, flags); + __serial8250_handle_port(up); spin_unlock_irqrestore(&up->port.lock, flags); } @@ -1572,7 +1567,7 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id) iir = serial_in(up, UART_IIR); if (!(iir & UART_IIR_NO_INT)) { - serial8250_handle_port(up); + __serial8250_handle_port(up); handled = 1; @@ -1644,6 +1639,8 @@ static int serial_link_irq_chain(struct uart_8250_port *up) struct irq_info *i; int ret, irq_flags = up->port.flags & UPF_SHARE_IRQ ? IRQF_SHARED : 0; + printk(KERN_INFO "serial_link_irq_chain\n"); + mutex_lock(&hash_mutex); h = &irq_lists[up->port.irq % NR_IRQ_HASH]; @@ -1678,6 +1675,7 @@ static int serial_link_irq_chain(struct uart_8250_port *up) i->head = &up->list; spin_unlock_irq(&i->lock); irq_flags |= up->port.irqflags; + printk(KERN_INFO "ttyS%d: request irq %d\n", serial_index(&up->port), up->port.irq); ret = request_irq(up->port.irq, serial8250_interrupt, irq_flags, "serial", i); if (ret < 0) @@ -1736,51 +1734,6 @@ static void serial8250_timeout(unsigned long data) mod_timer(&up->timer, jiffies + poll_timeout(up->port.timeout)); } -static void serial8250_backup_timeout(unsigned long data) -{ - struct uart_8250_port *up = (struct uart_8250_port *)data; - unsigned int iir, ier = 0, lsr; - unsigned long flags; - - /* - * Must disable interrupts or else we risk racing with the interrupt - * based handler. - */ - if (is_real_interrupt(up->port.irq)) { - ier = serial_in(up, UART_IER); - serial_out(up, UART_IER, 0); - } - - iir = serial_in(up, UART_IIR); - - /* - * This should be a safe test for anyone who doesn't trust the - * IIR bits on their UART, but it's specifically designed for - * the "Diva" UART used on the management processor on many HP - * ia64 and parisc boxes. - */ - spin_lock_irqsave(&up->port.lock, flags); - lsr = serial_in(up, UART_LSR); - up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS; - spin_unlock_irqrestore(&up->port.lock, flags); - if ((iir & UART_IIR_NO_INT) && (up->ier & UART_IER_THRI) && - (!uart_circ_empty(&up->port.state->xmit) || up->port.x_char) && - (lsr & UART_LSR_THRE)) { - iir &= ~(UART_IIR_ID | UART_IIR_NO_INT); - iir |= UART_IIR_THRI; - } - - if (!(iir & UART_IIR_NO_INT)) - serial8250_handle_port(up); - - if (is_real_interrupt(up->port.irq)) - serial_out(up, UART_IER, ier); - - /* Standard timer interval plus 0.2s to keep the port running */ - mod_timer(&up->timer, - jiffies + poll_timeout(up->port.timeout) + HZ / 5); -} - static unsigned int serial8250_tx_empty(struct uart_port *port) { struct uart_8250_port *up = (struct uart_8250_port *)port; @@ -1942,9 +1895,9 @@ static int serial8250_startup(struct uart_port *port) { struct uart_8250_port *up = (struct uart_8250_port *)port; unsigned long flags; - unsigned char lsr, iir; int retval; + up->tx_active = 0; up->capabilities = uart_config[up->port.type].flags; up->mcr = 0; @@ -2015,62 +1968,13 @@ static int serial8250_startup(struct uart_port *port) serial_outp(up, UART_LCR, 0); } - if (is_real_interrupt(up->port.irq)) { - unsigned char iir1; - /* - * Test for UARTs that do not reassert THRE when the - * transmitter is idle and the interrupt has already - * been cleared. Real 16550s should always reassert - * this interrupt whenever the transmitter is idle and - * the interrupt is enabled. Delays are necessary to - * allow register changes to become visible. - */ - spin_lock_irqsave(&up->port.lock, flags); - if (up->port.irqflags & IRQF_SHARED) - disable_irq_nosync(up->port.irq); - - wait_for_xmitr(up, UART_LSR_THRE); - serial_out_sync(up, UART_IER, UART_IER_THRI); - udelay(1); /* allow THRE to set */ - iir1 = serial_in(up, UART_IIR); - serial_out(up, UART_IER, 0); - serial_out_sync(up, UART_IER, UART_IER_THRI); - udelay(1); /* allow a working UART time to re-assert THRE */ - iir = serial_in(up, UART_IIR); - serial_out(up, UART_IER, 0); - - if (up->port.irqflags & IRQF_SHARED) - enable_irq(up->port.irq); - spin_unlock_irqrestore(&up->port.lock, flags); - - /* - * If the interrupt is not reasserted, setup a timer to - * kick the UART on a regular basis. - */ - if (!(iir1 & UART_IIR_NO_INT) && (iir & UART_IIR_NO_INT)) { - up->bugs |= UART_BUG_THRE; - pr_debug("ttyS%d - using backup timer\n", - serial_index(port)); - } - } - - /* - * The above check will only give an accurate result the first time - * the port is opened so this value needs to be preserved. - */ - if (up->bugs & UART_BUG_THRE) { - up->timer.function = serial8250_backup_timeout; - up->timer.data = (unsigned long)up; - mod_timer(&up->timer, jiffies + - poll_timeout(up->port.timeout) + HZ / 5); - } - /* * If the "interrupt" for this port doesn't correspond with any * hardware interrupt, we use a timer-based system. The original * driver used to do this with IRQ0. */ if (!is_real_interrupt(up->port.irq)) { + printk("not real interrupt %d\n", up->port.irq); up->timer.data = (unsigned long)up; mod_timer(&up->timer, jiffies + poll_timeout(up->port.timeout)); } else { @@ -2097,40 +2001,6 @@ static int serial8250_startup(struct uart_port *port) serial8250_set_mctrl(&up->port, up->port.mctrl); - /* Serial over Lan (SoL) hack: - Intel 8257x Gigabit ethernet chips have a - 16550 emulation, to be used for Serial Over Lan. - Those chips take a longer time than a normal - serial device to signalize that a transmission - data was queued. Due to that, the above test generally - fails. One solution would be to delay the reading of - iir. However, this is not reliable, since the timeout - is variable. So, let's just don't test if we receive - TX irq. This way, we'll never enable UART_BUG_TXEN. - */ - if (skip_txen_test || up->port.flags & UPF_NO_TXEN_TEST) - goto dont_test_tx_en; - - /* - * Do a quick test to see if we receive an - * interrupt when we enable the TX irq. - */ - serial_outp(up, UART_IER, UART_IER_THRI); - lsr = serial_in(up, UART_LSR); - iir = serial_in(up, UART_IIR); - serial_outp(up, UART_IER, 0); - - if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT) { - if (!(up->bugs & UART_BUG_TXEN)) { - up->bugs |= UART_BUG_TXEN; - pr_debug("ttyS%d - enabling bad tx status workarounds\n", - serial_index(port)); - } - } else { - up->bugs &= ~UART_BUG_TXEN; - } - -dont_test_tx_en: spin_unlock_irqrestore(&up->port.lock, flags); /* diff --git a/drivers/serial/8250.h b/drivers/serial/8250.h index 6e19ea3..770a0e2 100644 --- a/drivers/serial/8250.h +++ b/drivers/serial/8250.h @@ -46,9 +46,7 @@ struct serial8250_config { #define UART_CAP_UUE (1 << 12) /* UART needs IER bit 6 set (Xscale) */ #define UART_BUG_QUOT (1 << 0) /* UART has buggy quot LSB */ -#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_NOMSR (1 << 1) /* UART has buggy MSR status bits (Au1x00) */ #define PROBE_RSA (1 << 0) #define PROBE_ANY (~0) -- 1.6.5.3
_______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
