From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Wed, 4 May 2011 16:30:00 -0700 Subject: [U-Boot] [PATCH 1/2] NS16550: buffer reads In-Reply-To: <20110406162829.3507733b@schlenkerla.am.freescale.net> References: <20110406203012.GA30167@schlenkerla.am.freescale.net> <20110406204212.76780151F83@gemini.denx.de> <20110406162829.3507733b@schlenkerla.am.freescale.net> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Wed, Apr 6, 2011 at 2:28 PM, Scott Wood wrote: > On Wed, 6 Apr 2011 22:42:12 +0200 > Wolfgang Denk 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 > Hi Scott, This is a very useful patch and it works well. I have taken the liberty of modifying it slightly, because I think you should subtract 1 from the port number that you pass to NS16550. For some reason the 'COM' ports are numbered from 1 instead of 0. Please see below, and sorry if the patch doesn't come through cleanly. Regards, Simon 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 Tested-by: Simon Glass --- README | 8 ++++ drivers/serial/ns16550.c | 96 +++++++++++++++++++++++++++++++++++++++++++-- drivers/serial/serial.c | 5 +- include/ns16550.h | 4 +- 4 files changed, 104 insertions(+), 9 deletions(-) diff --git a/README b/README index ba01c52..5f6044f 100644 --- a/README +++ b/README @@ -2665,6 +2665,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 #include #include #include #include #include +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..49b2591 100644 --- a/drivers/serial/serial.c +++ b/drivers/serial/serial.c @@ -92,6 +92,7 @@ static NS16550_t serial_ports[4] = { }; #define PORT serial_ports[port-1] +#define PORTNR (port-1) #if defined(CONFIG_CONS_INDEX) #define CONSOLE (serial_ports[CONFIG_CONS_INDEX-1]) #endif @@ -219,13 +220,13 @@ _serial_puts (const char *s,const int port) int _serial_getc(const int port) { - return NS16550_getc(PORT); + return NS16550_getc(PORT, PORTNR); } int _serial_tstc(const int port) { - return NS16550_tstc(PORT); + return NS16550_tstc(PORT, PORTNR); } 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); -- 1.7.3.1 > > _______________________________________________ > U-Boot mailing list > U-Boot at lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >