| Message ID | 20110406203012.GA30167@schlenkerla.am.freescale.net |
|---|---|
| State | Rejected |
| Headers | show |
Dear Scott Wood, In message <20110406203012.GA30167@schlenkerla.am.freescale.net> you wrote: > This improves the performance of U-Boot when accepting rapid input, > such as pasting a sequence of commands. ... > +static struct ns16550_priv rxstate[NUM_PORTS]; Do we really need to statically allocate these buffers for all configured serial ports? Actual I/O will always be done to a single port only, so eventually only one such buffer will ever be used? > +static void enqueue(unsigned int port, char ch) > +{ > + /* If queue is full, drop the character. */ > + if ((rxstate[port].head - rxstate[port].tail - 1) % BUF_SIZE == 0) > + return; Is it really wise to silentrly drop characters here? Maybe we should stop reading from the device, and/or issue some error message? What is the increase of the memory footprint (flash and RAM) with this patch, with CONFIG_NS16550_BUFFER_READS enabled and not enabled? Best regards, Wolfgang Denk
On Wed, 6 Apr 2011 22:42:12 +0200 Wolfgang Denk <wd@denx.de> wrote: > Dear Scott Wood, > > In message <20110406203012.GA30167@schlenkerla.am.freescale.net> you wrote: > > This improves the performance of U-Boot when accepting rapid input, > > such as pasting a sequence of commands. > ... > > +static struct ns16550_priv rxstate[NUM_PORTS]; > > Do we really need to statically allocate these buffers for all > configured serial ports? Actual I/O will always be done to a single > port only, so eventually only one such buffer will ever be used? If we use just one buffer, then switching ports could have some latency, in terms of some queued data after the switch command being used from the old port -- unless we add code to flush the queue when switching. It probably won't matter much in practice, though, unless you do something like: "setenv stdout=...; something else" all at once. And it wouldn't matter at all for boards without CONFIG_SERIAL_MULTI -- except for linkstation, which does some access to a secondary serial port (see board/linkstation/avr.c). > > +static void enqueue(unsigned int port, char ch) > > +{ > > + /* If queue is full, drop the character. */ > > + if ((rxstate[port].head - rxstate[port].tail - 1) % BUF_SIZE == 0) > > + return; > > Is it really wise to silentrly drop characters here? It's what currently happens, except they're silently dropped by the hardware instead (and that's still possible with this patch, it just requires a longer time between getc/tstc calls). It's also what happens with usbtty (in lib/circbuf.c), though it drops old characters rather than new (both options seem about equally damaging). In case you're wondering, I didn't use lib/circbuf.c here as it would have been larger and more complicated (requiring dynamic port initialization). > Maybe we should stop reading from the device, That would hang the U-Boot console, for what is usually a temporary problem. > and/or issue some error message? Would probably exacerbate the problem by slowing things down further, would need to be ratelimited to avoid scrolling all useful stuff off the screen, and it would behave inconsistently (only happenning if the dropping doesn't happen in hardware). > What is the increase of the memory footprint (flash and RAM) with > this patch, with CONFIG_NS16550_BUFFER_READS enabled and not enabled? Currently, looks like up to 16 bytes with it not enabled -- due to not ifdeffing the change to pass an extra argument to the public NS16550 functions. This would go away if we just use a single queue. If we don't do that, I think we should ifdef the function signature change as well -- not so much for the size savings, as that it appears that some boards use or define these functions themselves (alternatively, I could try to fix up those boards). With it enabled, on ppc it's an extra 396 bytes of image size, and 1056 bytes of BSS. -Scott
diff --git a/README b/README index bd03523..d21ea9c 100644 --- a/README +++ b/README @@ -2682,6 +2682,14 @@ use the "saveenv" command to store a valid environment. space for already greatly restricted images, including but not limited to NAND_SPL configurations. +- CONFIG_NS16550_BUFFER_READS: + Instead of reading directly from the receive register + every time U-Boot is ready for another byte, keep a + buffer and fill it from the hardware fifo every time + U-Boot reads a character. This helps U-Boot keep up with + a larger amount of rapid input, such as happens when + pasting text into the terminal. + Low Level (hardware related) configuration options: --------------------------------------------------- diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index 8eeb48f..ed3428d 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -4,12 +4,15 @@ * modified to use CONFIG_SYS_ISA_MEM and new defines */ +#include <common.h> #include <config.h> #include <ns16550.h> #include <watchdog.h> #include <linux/types.h> #include <asm/io.h> +DECLARE_GLOBAL_DATA_PTR; + #define UART_LCRVAL UART_LCR_8N1 /* 8 data, 1 stop, no parity */ #define UART_MCRVAL (UART_MCR_DTR | \ UART_MCR_RTS) /* RTS/DTR */ @@ -86,21 +89,104 @@ void NS16550_putc (NS16550_t com_port, char c) } #ifndef CONFIG_NS16550_MIN_FUNCTIONS -char NS16550_getc (NS16550_t com_port) + +static char NS16550_raw_getc(NS16550_t regs) { - while ((serial_in(&com_port->lsr) & UART_LSR_DR) == 0) { + while ((serial_in(®s->lsr) & UART_LSR_DR) == 0) { #ifdef CONFIG_USB_TTY extern void usbtty_poll(void); usbtty_poll(); #endif WATCHDOG_RESET(); } - return serial_in(&com_port->rbr); + return serial_in(®s->rbr); +} + +static int NS16550_raw_tstc(NS16550_t regs) +{ + return ((serial_in(®s->lsr) & UART_LSR_DR) != 0); +} + + +#ifdef CONFIG_NS16550_BUFFER_READS + +#define BUF_SIZE 256 +#define NUM_PORTS 4 + +struct ns16550_priv { + char buf[BUF_SIZE]; + unsigned int head, tail; +}; + +static struct ns16550_priv rxstate[NUM_PORTS]; + +static void enqueue(unsigned int port, char ch) +{ + /* If queue is full, drop the character. */ + if ((rxstate[port].head - rxstate[port].tail - 1) % BUF_SIZE == 0) + return; + + rxstate[port].buf[rxstate[port].tail] = ch; + rxstate[port].tail = (rxstate[port].tail + 1) % BUF_SIZE; +} + +static int dequeue(unsigned int port, char *ch) +{ + /* Empty queue? */ + if (rxstate[port].head == rxstate[port].tail) + return 0; + + *ch = rxstate[port].buf[rxstate[port].head]; + rxstate[port].head = (rxstate[port].head + 1) % BUF_SIZE; + return 1; +} + +static void fill_rx_buf(NS16550_t regs, unsigned int port) +{ + while ((serial_in(®s->lsr) & UART_LSR_DR) != 0) + enqueue(port, serial_in(®s->rbr)); +} + +char NS16550_getc(NS16550_t regs, unsigned int port) +{ + char ch; + + if (port >= NUM_PORTS || !(gd->flags & GD_FLG_RELOC)) + return NS16550_raw_getc(regs); + + do { +#ifdef CONFIG_USB_TTY + extern void usbtty_poll(void); + usbtty_poll(); +#endif + fill_rx_buf(regs, port); + WATCHDOG_RESET(); + } while (!dequeue(port, &ch)); + + return ch; +} + +int NS16550_tstc(NS16550_t regs, unsigned int port) +{ + if (port >= NUM_PORTS || !(gd->flags & GD_FLG_RELOC)) + return NS16550_raw_tstc(regs); + + fill_rx_buf(regs, port); + + return rxstate[port].head != rxstate[port].tail; +} + +#else /* CONFIG_NS16550_BUFFER_READS */ + +char NS16550_getc(NS16550_t regs, unsigned int port) +{ + return NS16550_raw_getc(regs); } -int NS16550_tstc (NS16550_t com_port) +int NS16550_tstc(NS16550_t regs, unsigned int port) { - return ((serial_in(&com_port->lsr) & UART_LSR_DR) != 0); + return NS16550_raw_tstc(regs); } +#endif /* CONFIG_NS16550_BUFFER_READS */ #endif /* CONFIG_NS16550_MIN_FUNCTIONS */ diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c index 4032dfd..3fc80b1 100644 --- a/drivers/serial/serial.c +++ b/drivers/serial/serial.c @@ -219,13 +219,13 @@ _serial_puts (const char *s,const int port) int _serial_getc(const int port) { - return NS16550_getc(PORT); + return NS16550_getc(PORT, port); } int _serial_tstc(const int port) { - return NS16550_tstc(PORT); + return NS16550_tstc(PORT, port); } void diff --git a/include/ns16550.h b/include/ns16550.h index 9ea81e9..fa3e62e 100644 --- a/include/ns16550.h +++ b/include/ns16550.h @@ -160,6 +160,6 @@ typedef volatile struct NS16550 *NS16550_t; void NS16550_init (NS16550_t com_port, int baud_divisor); void NS16550_putc (NS16550_t com_port, char c); -char NS16550_getc (NS16550_t com_port); -int NS16550_tstc (NS16550_t com_port); +char NS16550_getc (NS16550_t regs, unsigned int port); +int NS16550_tstc (NS16550_t regs, unsigned int port); void NS16550_reinit (NS16550_t com_port, int baud_divisor);
This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands. Without this patch, on P4080DS I see a maximum of around 5 mw.l lines can be pasted. With this patch, it handles around 70 lines before lossage, long enough for most things you'd paste. Signed-off-by: Scott Wood <scottwood@freescale.com> --- README | 8 ++++ drivers/serial/ns16550.c | 96 +++++++++++++++++++++++++++++++++++++++++++-- drivers/serial/serial.c | 4 +- include/ns16550.h | 4 +- 4 files changed, 103 insertions(+), 9 deletions(-)