[U-Boot] [PATCH v3] NS16550: buffer reads

From: Scott Wood scottwood@freescale.com
From: Scott Wood scottwood@freescale.com
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 lines can be pasted. With this patch, it handles around 70 lines before lossage, long enough for most things you'd paste.
With it enabled, on ppc it's an extra 396 bytes of image size, and 1056 bytes of BSS.
ARM note from Simon Glass sjg@chromium.org - ARM code size goes from 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.
Signed-off-by: Scott Wood scottwood@freescale.com --- Changes in v2: - Fix checkpatch warnings, the other one was already there
Changes in v3: - Use CONFIG_NS16550_BUFFER_READS to set the buffer size also
README | 12 ++++++ drivers/serial/ns16550.c | 96 +++++++++++++++++++++++++++++++++++++++++++-- drivers/serial/serial.c | 5 +- include/ns16550.h | 4 +- 4 files changed, 108 insertions(+), 9 deletions(-)
diff --git a/README b/README index 7e032a9..1150d6c 100644 --- a/README +++ b/README @@ -2862,6 +2862,18 @@ 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. + + To use this option, define CONFIG_NS16550_BUFFER_READS to + the size of the buffer you want (256 is a reasonable value). + + Low Level (hardware related) configuration options: ---------------------------------------------------
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index 0174744..6429e4d 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 */ @@ -92,21 +95,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 CONFIG_NS16550_BUFFER_READS +#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 0d56e78..cf9a1dd 100644 --- a/drivers/serial/serial.c +++ b/drivers/serial/serial.c @@ -93,6 +93,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 51f1c17..60b0abc 100644 --- a/include/ns16550.h +++ b/include/ns16550.h @@ -164,6 +164,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);

On Saturday, October 15, 2011 06:03:52 PM Simon Glass wrote:
From: Scott Wood scottwood@freescale.com
From: Scott Wood scottwood@freescale.com
This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands.
Hi Simon,
[...]
diff --git a/include/ns16550.h b/include/ns16550.h index 51f1c17..60b0abc 100644 --- a/include/ns16550.h +++ b/include/ns16550.h @@ -164,6 +164,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);
Maybe you should fix the indent altogether ?
Cheers

Hi Marek,
On Sat, Oct 15, 2011 at 10:55 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 06:03:52 PM Simon Glass wrote:
From: Scott Wood scottwood@freescale.com
From: Scott Wood scottwood@freescale.com
This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands.
Hi Simon,
[...]
diff --git a/include/ns16550.h b/include/ns16550.h index 51f1c17..60b0abc 100644 --- a/include/ns16550.h +++ b/include/ns16550.h @@ -164,6 +164,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);
Maybe you should fix the indent altogether ?
I should probably do a separate patch for that.
Regards, Simon
Cheers

On Sunday, October 16, 2011 05:56:56 AM Simon Glass wrote:
Hi Marek,
On Sat, Oct 15, 2011 at 10:55 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 06:03:52 PM Simon Glass wrote:
From: Scott Wood scottwood@freescale.com
From: Scott Wood scottwood@freescale.com
This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands.
Hi Simon,
[...]
diff --git a/include/ns16550.h b/include/ns16550.h index 51f1c17..60b0abc 100644 --- a/include/ns16550.h +++ b/include/ns16550.h @@ -164,6 +164,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);
Maybe you should fix the indent altogether ?
I should probably do a separate patch for that.
Maybe queue it before this one ? :) Thanks!
Regards, Simon
Cheers

Hi Marek,
On Sat, Oct 15, 2011 at 9:18 PM, Marek Vasut marek.vasut@gmail.com wrote:
On Sunday, October 16, 2011 05:56:56 AM Simon Glass wrote:
Hi Marek,
On Sat, Oct 15, 2011 at 10:55 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Saturday, October 15, 2011 06:03:52 PM Simon Glass wrote:
From: Scott Wood scottwood@freescale.com
From: Scott Wood scottwood@freescale.com
This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands.
Hi Simon,
[...]
diff --git a/include/ns16550.h b/include/ns16550.h index 51f1c17..60b0abc 100644 --- a/include/ns16550.h +++ b/include/ns16550.h @@ -164,6 +164,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);
Maybe you should fix the indent altogether ?
I should probably do a separate patch for that.
Maybe queue it before this one ? :) Thanks!
OK I will add a new patch before the main one which cleans up the warnings in the file.
Regards, Simon
Regards, Simon
Cheers

Dear Simon Glass,
In message 1318694632-21872-1-git-send-email-sjg@chromium.org you wrote:
From: Scott Wood scottwood@freescale.com
From: Scott Wood scottwood@freescale.com
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 lines can be pasted. With this patch, it handles around 70 lines before lossage, long enough for most things you'd paste.
With it enabled, on ppc it's an extra 396 bytes of image size, and 1056 bytes of BSS.
ARM note from Simon Glass sjg@chromium.org - ARM code size goes from 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.
Signed-off-by: Scott Wood scottwood@freescale.com
Changes in v2:
- Fix checkpatch warnings, the other one was already there
Changes in v3:
- Use CONFIG_NS16550_BUFFER_READS to set the buffer size also
README | 12 ++++++ drivers/serial/ns16550.c | 96 +++++++++++++++++++++++++++++++++++++++++++-- drivers/serial/serial.c | 5 +- include/ns16550.h | 4 +- 4 files changed, 108 insertions(+), 9 deletions(-)
diff --git a/README b/README index 7e032a9..1150d6c 100644 --- a/README +++ b/README @@ -2862,6 +2862,18 @@ 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.
To use this option, define CONFIG_NS16550_BUFFER_READS to
the size of the buffer you want (256 is a reasonable value).
If we should accept that patch, then please chose a better name here. For example, CONFIG_NS16550_RBUF_SIZE (or similar) seems to be a somewhat better choice.
+#ifdef CONFIG_NS16550_BUFFER_READS
+#define BUF_SIZE CONFIG_NS16550_BUFFER_READS +#define NUM_PORTS 4
+struct ns16550_priv {
- char buf[BUF_SIZE];
- unsigned int head, tail;
+};
There is little sense in adding another variable here. Just write:
char buf[CONFIG_NS16550_BUFFER_READS];
(or whatever variable name would be used here).
Best regards,
Wolfgang Denk

Hi Wolfgang,
On Sat, Oct 15, 2011 at 12:08 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message 1318694632-21872-1-git-send-email-sjg@chromium.org you wrote:
From: Scott Wood scottwood@freescale.com
From: Scott Wood scottwood@freescale.com
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 lines can be pasted. With this patch, it handles around 70 lines before lossage, long enough for most things you'd paste.
With it enabled, on ppc it's an extra 396 bytes of image size, and 1056 bytes of BSS.
ARM note from Simon Glass sjg@chromium.org - ARM code size goes from 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.
Signed-off-by: Scott Wood scottwood@freescale.com
Changes in v2:
- Fix checkpatch warnings, the other one was already there
Changes in v3:
- Use CONFIG_NS16550_BUFFER_READS to set the buffer size also
README | 12 ++++++ drivers/serial/ns16550.c | 96 +++++++++++++++++++++++++++++++++++++++++++-- drivers/serial/serial.c | 5 +- include/ns16550.h | 4 +- 4 files changed, 108 insertions(+), 9 deletions(-)
diff --git a/README b/README index 7e032a9..1150d6c 100644 --- a/README +++ b/README @@ -2862,6 +2862,18 @@ 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.
- To use this option, define CONFIG_NS16550_BUFFER_READS to
- the size of the buffer you want (256 is a reasonable value).
If we should accept that patch, then please chose a better name here. For example, CONFIG_NS16550_RBUF_SIZE (or similar) seems to be a somewhat better choice.
Yes much better.
+#ifdef CONFIG_NS16550_BUFFER_READS
+#define BUF_SIZE CONFIG_NS16550_BUFFER_READS +#define NUM_PORTS 4
+struct ns16550_priv {
- char buf[BUF_SIZE];
- unsigned int head, tail;
+};
There is little sense in adding another variable here. Just write:
char buf[CONFIG_NS16550_BUFFER_READS];
(or whatever variable name would be used here).
Yes, this makes more sense with the new name, too.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "The release of emotion is what keeps us health. Emotionally healthy." "That may be, Doctor. However, I have noted that the healthy release of emotion is frequently unhealthy for those closest to you." -- McCoy and Spock, "Plato's Stepchildren", stardate 5784.3
participants (3)
-
Marek Vasut
-
Simon Glass
-
Wolfgang Denk