[U-Boot] [PATCH 1/2] NS16550: buffer reads
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(-)
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);
Signed-off-by: Scott Wood scottwood@freescale.com --- include/configs/corenet_ds.h | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/include/configs/corenet_ds.h b/include/configs/corenet_ds.h index 7bafa05..47edabd 100644 --- a/include/configs/corenet_ds.h +++ b/include/configs/corenet_ds.h @@ -236,6 +236,8 @@ #define CONFIG_SYS_NS16550_REG_SIZE 1 #define CONFIG_SYS_NS16550_CLK (get_bus_freq(0)/2)
+#define CONFIG_NS16550_BUFFER_READS + #define CONFIG_SYS_BAUDRATE_TABLE \ {300, 600, 1200, 2400, 4800, 9600, 19200, 38400, 57600, 115200}
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
On Wed, Apr 6, 2011 at 2:28 PM, Scott Wood scottwood@freescale.com wrote:
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
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@freescale.com Tested-by: Simon Glass sjg@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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On Wed, 4 May 2011 16:30:00 -0700 Simon Glass sjg@chromium.org wrote:
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.
Thanks for spotting!
Please see below, and sorry if the patch doesn't come through cleanly.
The whitespace got mangled.
There's another thing that needs to be fixed -- the signatures of the 16550 functions should not change when the feature is disabled (see the discussion on http://patchwork.ozlabs.org/patch/90066/), or it'll break boards like linkstation.
I'll include both fixes in my respin.
-Scott
Hi Scott,
On Fri, May 6, 2011 at 1:28 PM, Scott Wood scottwood@freescale.com wrote:
On Wed, 4 May 2011 16:30:00 -0700 Simon Glass sjg@chromium.org wrote:
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.
Thanks for spotting!
Please see below, and sorry if the patch doesn't come through cleanly.
The whitespace got mangled.
There's another thing that needs to be fixed -- the signatures of the 16550 functions should not change when the feature is disabled (see the discussion on http://patchwork.ozlabs.org/patch/90066/), or it'll break boards like linkstation.
I'll include both fixes in my respin.
Are you planning a respin of this? If not I will take this up as it is pretty important for Tegra.
Regards, Simon
-Scott
On Oct 12, 2011, at 4:23 PM, Simon Glass wrote:
Hi Scott,
On Fri, May 6, 2011 at 1:28 PM, Scott Wood scottwood@freescale.com wrote:
On Wed, 4 May 2011 16:30:00 -0700 Simon Glass sjg@chromium.org wrote:
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.
Thanks for spotting!
Please see below, and sorry if the patch doesn't come through cleanly.
The whitespace got mangled.
There's another thing that needs to be fixed -- the signatures of the 16550 functions should not change when the feature is disabled (see the discussion on http://patchwork.ozlabs.org/patch/90066/), or it'll break boards like linkstation.
I'll include both fixes in my respin.
Are you planning a respin of this? If not I will take this up as it is pretty important for Tegra.
Scott's on vacation for next 2 weeks so guessing he'd be fine w/you respining.
- k
Hi Kumar,
On Wed, Oct 12, 2011 at 6:08 PM, Kumar Gala galak@kernel.crashing.org wrote:
On Oct 12, 2011, at 4:23 PM, Simon Glass wrote:
Hi Scott,
On Fri, May 6, 2011 at 1:28 PM, Scott Wood scottwood@freescale.com wrote:
On Wed, 4 May 2011 16:30:00 -0700 Simon Glass sjg@chromium.org wrote:
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.
Thanks for spotting!
Please see below, and sorry if the patch doesn't come through cleanly.
The whitespace got mangled.
There's another thing that needs to be fixed -- the signatures of the 16550 functions should not change when the feature is disabled (see the discussion on http://patchwork.ozlabs.org/patch/90066/), or it'll break boards like linkstation.
I'll include both fixes in my respin.
Are you planning a respin of this? If not I will take this up as it is pretty important for Tegra.
Scott's on vacation for next 2 weeks so guessing he'd be fine w/you respining.
OK I have sent it to the list, just fixing two checkpatch warnings.
It is pretty-much essential on Tegra since otherwise pasting stuff into the terminal hardly works at all.
Regards, Simon
- k
participants (4)
-
Kumar Gala -
Scott Wood -
Simon Glass -
Wolfgang Denk