[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(®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
>
More information about the U-Boot
mailing list