Patchwork [U-Boot,1/2] NS16550: buffer reads

login
register
mail settings
Submitter Scott Wood
Date April 6, 2011, 8:30 p.m.
Message ID <20110406203012.GA30167@schlenkerla.am.freescale.net>
Download mbox | patch
Permalink /patch/90066/
State Rejected
Headers show

Comments

Scott Wood - April 6, 2011, 8:30 p.m.
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(-)
Wolfgang Denk - April 6, 2011, 8:42 p.m.
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
Scott Wood - April 6, 2011, 9:28 p.m.
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

Patch

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(&regs->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(&regs->rbr);
+}
+
+static int NS16550_raw_tstc(NS16550_t regs)
+{
+	return ((serial_in(&regs->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(&regs->lsr) & UART_LSR_DR) != 0)
+		enqueue(port, serial_in(&regs->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);