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

Simon Glass sjg at chromium.org
Thu May 5 01:30:00 CEST 2011


On Wed, Apr 6, 2011 at 2:28 PM, Scott Wood <scottwood at freescale.com> wrote:

> On Wed, 6 Apr 2011 22:42:12 +0200
> Wolfgang Denk <wd at denx.de> wrote:
>
> > Dear Scott Wood,
> >
> > In message <20110406203012.GA30167 at 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 <scottwood at freescale.com>
Tested-by: Simon Glass <sjg at chromium.org>
---
 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 <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..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
>


More information about the U-Boot mailing list