NS16550: buffer reads

PATCH Superseded Simon Glass 3 participants 7 messages Oct 15, 2011 – Oct 16, 2011

lore

Share & tools

lore

Simon Glass

author

2011-10-15 16:03 UTC #0 lore raw

PATCH v3 1/1 NS16550: buffer reads

From: Scott Wood <[email protected]>

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 <[email protected]> - ARM code size goes from 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.

Signed-off-by Scott Wood <[email protected]>

---
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(-)

README +12 -0

index 7e032a9..1150d6c 100644
@@ -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:
 ---------------------------------------------------

drivers/serial/ns16550.c +91 -5

124 lines not on this page · raw message

drivers/serial/serial.c +3 -2

index 0d56e78..cf9a1dd 100644
@@ -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

include/ns16550.h +2 -2

index 51f1c17..60b0abc 100644
@@ -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);

Signature · 1 line

1.7.3.1

Marek Vasut

2011-10-15 17:55 UTC #1 lore raw

PATCH v3 1/1 NS16550: buffer reads

Quoted 6 lines · Simon Glass

From: Scott Wood <[email protected]>
From: Scott Wood <[email protected]>
This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands.

Hi Simon,

[...]

include/ns16550.h typedef volatile struct NS16550 *NS16550_t; · line 169

Quoted 13 lines

include/ns16550.h +2 -2

index 51f1c17..60b0abc 100644
@@ -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

Simon Glass

author

2011-10-16 03:56 UTC #2 lore raw

PATCH v3 1/1 NS16550: buffer reads

Hi Marek,

Quoted 18 lines · Marek Vasut

Quoted 6 lines · Simon Glass
From: Scott Wood <[email protected]>
From: Scott Wood <[email protected]>
This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands.
Hi Simon,
[...]
Quoted 6 lines
include/ns16550.h +0 -0
index 51f1c17..60b0abc 100644
@@ -164,6 +164,6 @@ typedef volatile struct NS16550 *NS16550_t;
 

Quoted 10 lines

include/ns16550.h typedef volatile struct NS16550 *NS16550_t; · line 169

Quoted 7 lines

include/ns16550.h +2 -2

 ?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

Quoted 2 lines

Cheers

Marek Vasut

2011-10-16 04:18 UTC #3 lore raw

PATCH v3 1/1 NS16550: buffer reads

Quoted 31 lines · Simon Glass

Hi Marek,
Quoted 18 lines · Marek Vasut
Quoted 6 lines · Simon Glass
From: Scott Wood <[email protected]>
From: Scott Wood <[email protected]>
This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands.
Hi Simon,
[...]
Quoted 6 lines
include/ns16550.h +0 -0
index 51f1c17..60b0abc 100644
@@ -164,6 +164,6 @@ typedef volatile struct NS16550 *NS16550_t;
 
Quoted 9 lines
include/ns16550.h typedef volatile struct NS16550 *NS16550_t; · line 169
Quoted 7 lines
include/ns16550.h +2 -2
 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 ?

Quoted 1 line

I should probably do a separate patch for that.

Maybe queue it before this one ? :) Thanks!

Quoted 5 lines

Regards,
Simon
Quoted 1 line
Cheers

Simon Glass

author

2011-10-16 05:11 UTC #4 lore raw

PATCH v3 1/1 NS16550: buffer reads

Hi Marek,

Quoted 32 lines · Marek Vasut

Quoted 31 lines · Simon Glass
Hi Marek,
Quoted 27 lines · Marek Vasut
Quoted 6 lines · Simon Glass
From: Scott Wood <[email protected]>
From: Scott Wood <[email protected]>
This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands.
Hi Simon,
[...]
include/ns16550.h typedef volatile struct NS16550 *NS16550_t; · line 169
Quoted 13 lines
include/ns16550.h +2 -2
index 51f1c17..60b0abc 100644
@@ -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 ?

Quoted 3 lines

Quoted 1 line
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

Quoted 7 lines

Quoted 6 lines
Regards,
Simon
Quoted 1 line
Cheers

Wolfgang Denk

2011-10-15 19:08 UTC #5 lore raw

PATCH v3 1/1 NS16550: buffer reads

In reply to Simon Glass

Dear Simon Glass,

Quoted 39 lines · In message <[email protected]> you wrote:

From: Scott Wood <[email protected]>
From: Scott Wood <[email protected]>
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 <[email protected]> - ARM code size goes from 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.
Signed-off-by Scott Wood <[email protected]>
---
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(-)
README +0 -0
index 7e032a9..1150d6c 100644
@@ -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.
 

README use the "saveenv" command to store a valid environment. · line 2874

Quoted 10 lines

README +10 -0

+- 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.

README use the "saveenv" command to store a valid environment.

Quoted 9 lines

README +9 -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;
+};

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

Signature · 8 lines

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 at 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

Simon Glass

author

2011-10-16 05:11 UTC #6 lore raw

PATCH v3 1/1 NS16550: buffer reads

Hi Wolfgang,

Quoted 42 lines · Wolfgang Denk

Dear Simon Glass,
Quoted 39 lines · In message <[email protected]> you wrote:
From: Scott Wood <[email protected]>
From: Scott Wood <[email protected]>
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 <[email protected]> - ARM code size goes from 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.
Signed-off-by Scott Wood <[email protected]>
---
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(-)
README +0 -0
index 7e032a9..1150d6c 100644
@@ -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.

Quoted 14 lines

Quoted 10 lines
+- 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.

Quoted 13 lines

README
Quoted 10 lines
README +9 -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;
+};
There is little sense in adding another variable here. ?Just write:

Quoted 3 lines

? ? ? ?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

Quoted 16 lines

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 at 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