NS16550: buffer reads

PATCH Superseded Simon Glass 5 participants 20 messages Oct 15, 2011 – Oct 17, 2011

lore

Share & tools

lore

Simon Glass

author

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

PATCH v2 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

 README                   |    8 ++++
 drivers/serial/ns16550.c |   96 +++++++++++++++++++++++++++++++++++++++++++--
 drivers/serial/serial.c  |    5 +-
 include/ns16550.h        |    4 +-
 4 files changed, 104 insertions(+), 9 deletions(-)

README +8 -0

index 7e032a9..28d2e8a 100644
@@ -2862,6 +2862,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:
 ---------------------------------------------------

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

Albert ARIBAUD

2011-10-15 10:43 UTC #1 lore raw

PATCH v2 1/1 NS16550: buffer reads

Hi Simon,

Le 15/10/2011 02:03, Simon Glass a ?crit :

Quoted 14 lines

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.

Quoted 7 lines

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

A general question: is the issue with input as such, or with echoing this input?

Quoted 64 lines

  README                   |    8 ++++
  drivers/serial/ns16550.c |   96 +++++++++++++++++++++++++++++++++++++++++++--
  drivers/serial/serial.c  |    5 +-
  include/ns16550.h        |    4 +-
  4 files changed, 104 insertions(+), 9 deletions(-)

README +8 -0

index 7e032a9..28d2e8a 100644
@@ -2862,6 +2862,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:
  ---------------------------------------------------

drivers/serial/ns16550.c +8 -3

index 0174744..a012e5d 100644
@@ -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(&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);
+}

drivers/serial/ns16550.c void NS16550_putc (NS16550_t com_port, char c) · line 119

Quoted 10 lines

drivers/serial/ns16550.c +10 -0

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

I guess this define conditions how well U-Boot will cope with steady high-rate input, correct? If so, I'd like it to be a configuration option -- actually, one could combine enabling the option *and* setting the expected buffer size.

Quoted 114 lines

drivers/serial/ns16550.c +73 -2

+#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 */

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

index 51f1c17..60b0abc 100644

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

Quoted 8 lines

include/ns16550.h +2 -2

@@ -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);

Amicalement,

Signature · 1 line

Albert.

Simon Glass

author

2011-10-15 14:47 UTC #2 lore raw

PATCH v2 1/1 NS16550: buffer reads

Hi Albert,

Quoted 21 lines · Albert ARIBAUD

Hi Simon,
Le 15/10/2011 02:03, Simon Glass a ?crit :
Quoted 18 lines
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.

Quoted 7 lines

Quoted 4 lines
Signed-off-by Scott Wood<[email protected]>
---
Changes in v2:
- Fix checkpatch warnings, the other one was already there
A general question: is the issue with input as such, or with echoing this input?

The problem is in echoing the input, since this requires the uart to wait once its transmit FIFO is full. While it is waiting (and not checking the receive side), it misses characters coming from the other end. Basically we need to constantly check the receiving side while we are transmitting.

Quoted 76 lines

Quoted 76 lines
?README ? ? ? ? ? ? ? ? ? | ? ?8 ++++ ?drivers/serial/ns16550.c | ? 96
+++++++++++++++++++++++++++++++++++++++++++--
?drivers/serial/serial.c ?| ? ?5 +- ?include/ns16550.h ? ? ? ?| ? ?4 +-
?4 files changed, 104 insertions(+), 9 deletions(-)
README +0 -0
index 7e032a9..28d2e8a 100644
@@ -2862,6 +2862,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:
?---------------------------------------------------
drivers/serial/ns16550.c +0 -0
index 0174744..a012e5d 100644
@@ -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>
drivers/serial/ns16550.c +2 -0
+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 */
drivers/serial/ns16550.c +0 -0
@@ -92,21 +95,104 @@ void NS16550_putc (NS16550_t com_port, char c)
 ?}
 
?#ifndef CONFIG_NS16550_MIN_FUNCTIONS
drivers/serial/ns16550.c +2 -1
-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);
drivers/serial/ns16550.c +8 -0
+}
+
+static int NS16550_raw_tstc(NS16550_t regs)
+{
+ ? ? ? return ((serial_in(&regs->lsr)& ?UART_LSR_DR) != 0);
+}
+
+

Quoted 8 lines

drivers/serial/ns16550.c void NS16550_putc (NS16550_t com_port, char c) · line 119
Quoted 3 lines
drivers/serial/ns16550.c +3 -0
+#ifdef CONFIG_NS16550_BUFFER_READS
+
+#define BUF_SIZE 256
I guess this define conditions how well U-Boot will cope with steady high-rate input, correct? If so, I'd like it to be a configuration option -- actually, one could combine enabling the option *and* setting the expected buffer size.

Yes so you could have something like:

#define CONFIG_NS16550_BUFFER_READS 256

Quoted 116 lines

drivers/serial/ns16550.c +70 -0

+#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);

drivers/serial/ns16550.c +1 -1

-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 */

drivers/serial/serial.c +0 -0

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

drivers/serial/serial.c +0 -0

@@ -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 +0 -0

index 51f1c17..60b0abc 100644
@@ -164,6 +164,6 @@ typedef volatile struct NS16550 *NS16550_t;

Quoted 13 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);

Amicalement,
--
Albert.

Regards,
Simon

Wolfgang Denk

2011-10-15 16:02 UTC #3 lore raw

PATCH v2 1/1 NS16550: buffer reads

Dear Simon Glass,

Quoted 9 lines · In message <CAPnjgZ34FFotBBiqb=Y5xsuyRiavOWud5G2sy9GModsG4LB6=w@mail.gmail.com>…

Quoted 3 lines
A general question: is the issue with input as such, or with echoing this input?
The problem is in echoing the input, since this requires the uart to wait once its transmit FIFO is full. While it is waiting (and not checking the receive side), it misses characters coming from the other end. Basically we need to constantly check the receiving side while we are transmitting.

No, we do not.

U-Boot is strictly single tasking. This is one of the fundamental design principles. We do not receive network packets while sending ourself, or when doing other things. We do not receive characters on the serial line while sending characters, or when doing other things.

This is known behaviour, and you should just adapt to it. I don't know what exactly is your situation when you run into such problems - the recommended approach for example when remote-controlling U-Boot is to wait for the next input prompt before sending the next line of characters - and stop after the newline, until you receive the next prompt.

Best regards,

Wolfgang Denk

Signature · 7 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
I believe you find life such a problem because you  think  there  are
the  good  people  and the bad people. You're wrong, of course. There
are, always and only, the bad people, but some of them are  on  oppo-
site sides.                      - Terry Pratchett, _Guards! Guards!_

Simon Glass

author

2011-10-15 16:12 UTC #4 lore raw

PATCH v2 1/1 NS16550: buffer reads

Hi Wolfgang,

Quoted 20 lines · Wolfgang Denk

Dear Simon Glass,
Quoted 9 lines · In message <CAPnjgZ34FFotBBiqb=Y5xsuyRiavOWud5G2sy9GModsG4LB6=w@mail.gmail.com>…
Quoted 3 lines
A general question: is the issue with input as such, or with echoing this input?
The problem is in echoing the input, since this requires the uart to wait once its transmit FIFO is full. While it is waiting (and not checking the receive side), it misses characters coming from the other end. Basically we need to constantly check the receiving side while we are transmitting.
No, we do not.
U-Boot is strictly single tasking. This is one of the fundamental design principles. We do not receive network packets while sending ourself, or when doing other things. We do not receive characters on the serial line while sending characters, or when doing other things.

Quoted 6 lines

This is known behaviour, and you should just adapt to it. I don't know what exactly is your situation when you run into such problems - the recommended approach for example when remote-controlling U-Boot is to wait for the next input prompt before sending the next line of characters - and stop after the newline, until you receive the next prompt.

I certainly don't want to break a fundamental design principle. .
The situation this occurs is when you paste characters into a serial terminal connected to U-Boot. This is a pretty common requirement. Yes you can manually select each line and paste it but that is a pain. With this patch it is possible to paste a series of 'setenv' commands, for example.

This caused an awful lot of confusion here because characters would be silently dropped within the command line (normally about 16 chars from the end). Then the system would not boot. It is really not a friendly experience.

Regards,
Simon

Quoted 14 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 I believe you find life such a problem because you ?think ?there ?are the ?good ?people ?and the bad people. You're wrong, of course. There are, always and only, the bad people, but some of them are ?on ?oppo- site sides. ? ? ? ? ? ? ? ? ? ? ?- Terry Pratchett, _Guards! Guards!_

Albert ARIBAUD

2011-10-15 16:21 UTC #5 lore raw

PATCH v2 1/1 NS16550: buffer reads

Le 15/10/2011 18:12, Simon Glass a ?crit :

Quoted 38 lines

Hi Wolfgang,
Quoted 20 lines · Wolfgang Denk
Dear Simon Glass,
Quoted 9 lines · In message<CAPnjgZ34FFotBBiqb=Y5xsuyRiavOWud5G2sy9GModsG4LB6=w@mail.gmail.com>…
Quoted 3 lines
A general question: is the issue with input as such, or with echoing this input?
The problem is in echoing the input, since this requires the uart to wait once its transmit FIFO is full. While it is waiting (and not checking the receive side), it misses characters coming from the other end. Basically we need to constantly check the receiving side while we are transmitting.
No, we do not.
U-Boot is strictly single tasking. This is one of the fundamental design principles. We do not receive network packets while sending ourself, or when doing other things. We do not receive characters on the serial line while sending characters, or when doing other things.
Quoted 6 lines
This is known behaviour, and you should just adapt to it. I don't know what exactly is your situation when you run into such problems - the recommended approach for example when remote-controlling U-Boot is to wait for the next input prompt before sending the next line of characters - and stop after the newline, until you receive the next prompt.
I certainly don't want to break a fundamental design principle. .
The situation this occurs is when you paste characters into a serial terminal connected to U-Boot. This is a pretty common requirement. Yes you can manually select each line and paste it but that is a pain. With this patch it is possible to paste a series of 'setenv' commands, for example.

Quoted 7 lines

This caused an awful lot of confusion here because characters would be silently dropped within the command line (normally about 16 chars from the end). Then the system would not boot. It is really not a friendly experience.
Regards,
Simon

How about using the time-honored flow-control mechanism? If the serial port supports hardware CTS and RTS signals, you can use them to sync with the sender. If not, you can send XOFF and XON to respectively ask the sender to pause and resume sending. Of course, the sender must support hardware or software flow control.

Amicalement,

Signature · 1 line

Albert.

Simon Glass

author

2011-10-15 16:50 UTC #6 lore raw

PATCH v2 1/1 NS16550: buffer reads

Hi Albert,

Quoted 49 lines · Albert ARIBAUD

Le 15/10/2011 18:12, Simon Glass a ?crit :
Quoted 48 lines
Hi Wolfgang,
Quoted 24 lines · On Sat, Oct 15, 2011 at 9:02 AM, Wolfgang Denk<[email protected]> ?wrote:
Dear Simon Glass,
In message<CAPnjgZ34FFotBBiqb=Y5xsuyRiavOWud5G2sy9GModsG4LB6=w@mail.gmail.com>
Quoted 10 lines · ?you
Quoted 4 lines
A general question: is the issue with input as such, or with echoing this
input?
The problem is in echoing the input, since this requires the uart to wait once its transmit FIFO is full. While it is waiting (and not checking the receive side), it misses characters coming from the other end. Basically we need to constantly check the receiving side while we are transmitting.
No, we do not.
U-Boot is strictly single tasking. This is one of the fundamental design principles. We do not receive network packets while sending ourself, or when doing other things. We do not receive characters on the serial line while sending characters, or when doing other things.
Quoted 6 lines
This is known behaviour, and you should just adapt to it. I don't know what exactly is your situation when you run into such problems - the recommended approach for example when remote-controlling U-Boot is to wait for the next input prompt before sending the next line of characters - and stop after the newline, until you receive the next prompt.
I certainly don't want to break a fundamental design principle. .
The situation this occurs is when you paste characters into a serial terminal connected to U-Boot. This is a pretty common requirement. Yes you can manually select each line and paste it but that is a pain. With this patch it is possible to paste a series of 'setenv' commands, for example.
This caused an awful lot of confusion here because characters would be silently dropped within the command line (normally about 16 chars from the end). Then the system would not boot. It is really not a friendly experience.

Quoted 8 lines

Quoted 2 lines
Regards,
Simon
How about using the time-honored flow-control mechanism? If the serial port supports hardware CTS and RTS signals, you can use them to sync with the sender. If not, you can send XOFF and XON to respectively ask the sender to pause and resume sending. Of course, the sender must support hardware or software flow control.

It is common to have just a 2-wire UART for the console. Perhaps this is unfortunate.

Are you suggesting that U-Boot should send XOFF sometimes, or does it support this already? (I haven't seen it). When would U-Boot send XOFF? Perhaps it could send XOFF at the end of each line that it receives. Then XON when it starts reading the next line or maybe when it runs out of characters and wants more.

Most terminal problems will support software flow control. But are you sure this is better?

Regards,
Simon

Quoted 5 lines

Amicalement,
--
Albert.

Simon Glass

author

2011-10-15 17:45 UTC #7 lore raw

PATCH v2 1/1 NS16550: buffer reads

Quoted 71 lines · Simon Glass

Hi Albert,
Quoted 49 lines · Albert ARIBAUD
Le 15/10/2011 18:12, Simon Glass a ?crit :
Quoted 48 lines
Hi Wolfgang,
Quoted 24 lines · On Sat, Oct 15, 2011 at 9:02 AM, Wolfgang Denk<[email protected]> ?wrote:
Dear Simon Glass,
In message<CAPnjgZ34FFotBBiqb=Y5xsuyRiavOWud5G2sy9GModsG4LB6=w@mail.gmail.com>
Quoted 10 lines · ?you
Quoted 4 lines
A general question: is the issue with input as such, or with echoing this
input?
The problem is in echoing the input, since this requires the uart to wait once its transmit FIFO is full. While it is waiting (and not checking the receive side), it misses characters coming from the other end. Basically we need to constantly check the receiving side while we are transmitting.
No, we do not.
U-Boot is strictly single tasking. This is one of the fundamental design principles. We do not receive network packets while sending ourself, or when doing other things. We do not receive characters on the serial line while sending characters, or when doing other things.
Quoted 6 lines
This is known behaviour, and you should just adapt to it. I don't know what exactly is your situation when you run into such problems - the recommended approach for example when remote-controlling U-Boot is to wait for the next input prompt before sending the next line of characters - and stop after the newline, until you receive the next prompt.
I certainly don't want to break a fundamental design principle. .
The situation this occurs is when you paste characters into a serial terminal connected to U-Boot. This is a pretty common requirement. Yes you can manually select each line and paste it but that is a pain. With this patch it is possible to paste a series of 'setenv' commands, for example.
This caused an awful lot of confusion here because characters would be silently dropped within the command line (normally about 16 chars from the end). Then the system would not boot. It is really not a friendly experience.
Quoted 8 lines
Quoted 2 lines
Regards,
Simon
How about using the time-honored flow-control mechanism? If the serial port supports hardware CTS and RTS signals, you can use them to sync with the sender. If not, you can send XOFF and XON to respectively ask the sender to pause and resume sending. Of course, the sender must support hardware or software flow control.
It is common to have just a 2-wire UART for the console. Perhaps this is unfortunate.
Are you suggesting that U-Boot should send XOFF sometimes, or does it support this already? (I haven't seen it). When would U-Boot send XOFF? Perhaps it could send XOFF at the end of each line that it receives. Then XON when it starts reading the next line or maybe when it runs out of characters and wants more.

Quoted 5 lines

Most terminal problems will support software flow control. But are you sure this is better?
Regards,
Simon

Another thought...the problem is that every character we receive we echo, so we can of course never get ahead of the workload. But we can get behind, since while we are doing something other than receiving serial characters (e.g. processing a command, outputing a prompt, updating the LCD) we are not transmitting. Then when we next go to receive, we find (say) 10 characters built up, and start echoing them all. We are now 10 characters behind the receive and will stay that way until there is a let-up. Even worse if one of the commands outputs something, since then we might be 20 or 30 characters behind, and start dropping things. Even if no commands output anything, delays and the U-Boot prompt will create a problem.

(As an aside, I note the call to the USB code to deal with transmission/reception so clearly there is some interrupt handling going on...)

So another approach would be to buffer all output at the console layer and send it out in the fullness of time using a function similar to tstc() that tests whether or not there is space in the output fifo.

This seems a more invasive but more general fix. I'm not that keen on it since it changes something that people might rely on - that printf() waits if the serial fifo is full. It might mean that prior to running bootm U-Boot would need to wait for the buffer to flush. Ick.

But I am happy to look at this if you think it isn't even worse.

Regards,
Simon

Quoted 7 lines

Quoted 6 lines
Amicalement,
--
Albert.

Wolfgang Denk

2011-10-15 19:14 UTC #8 lore raw

PATCH v2 1/1 NS16550: buffer reads

Dear Simon Glass,

Quoted 3 lines · In message <CAPnjgZ0s0-P4ubDb=AcB=28otKw+Fi08yLNi2GePoFqBRMn3Gw@mail.gmail.com>…

Another thought...the problem is that every character we receive we echo, so we can of course never get ahead of the workload. But we can

Even worse, if your input contains any commands that prodce output, mayeb even lots of output (say "md $addr 10000") then even your increased buffer size is not going to help you.

Quoted 4 lines

way until there is a let-up. Even worse if one of the commands outputs something, since then we might be 20 or 30 characters behind, and start dropping things. Even if no commands output anything, delays and the U-Boot prompt will create a problem.

Indeed. And this is why the simple rule applies: always wait for the next command prompt before sending the next line of input.

Best regards,

Wolfgang Denk

Signature · 5 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
To understand a program you must become  both  the  machine  and  the
program.

Simon Glass

author

2011-10-16 04:46 UTC #9 lore raw

PATCH v2 1/1 NS16550: buffer reads

Hi Wolfgang,

Quoted 10 lines · Wolfgang Denk

Dear Simon Glass,
Quoted 3 lines · In message <CAPnjgZ0s0-P4ubDb=AcB=28otKw+Fi08yLNi2GePoFqBRMn3Gw@mail.gmail.com>…
Another thought...the problem is that every character we receive we echo, so we can of course never get ahead of the workload. ?But we can
Even worse, if your input contains any commands that prodce output, mayeb even lots of output (say "md $addr 10000") then even your increased buffer size is not going to help you.

It helps a little - provided there are no more than 250 characters more 'serial input' after the md command, all is well. But I think this example is a little contrived.

Quoted 8 lines

Quoted 5 lines
way until there is a let-up. Even worse if one of the commands outputs something, since then we might be 20 or 30 characters behind, and start dropping things. Even if no commands output anything, delays and the U-Boot prompt will create a problem.
Indeed. ?And this is why the simple rule applies: always wait for the next command prompt before sending the next line of input.

I feel this is a painful restriction. I see no indication of this limitation anywhere on the command line, no error or warning, etc. It is common to want to paste in new bootargs, new network parameters, new environment variables. U-Boot has a command line which purports to support this, and with a fairly straightforward patch and in the majority of cases, it can.

Yes this is a minor point, but it does bite people, and usability is important.

Regards,
Simon

Quoted 12 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 To understand a program you must become ?both ?the ?machine ?and ?the program.

Wolfgang Denk

2011-10-16 19:52 UTC #10 lore raw

PATCH v2 1/1 NS16550: buffer reads

Dear Simon Glass,

Quoted 8 lines · In message <CAPnjgZ2Qf-Egdp9WCnoWVPJmjic5BPF0Db+z1oD=fdb3aoLNKw@mail.gmail.com>…

Quoted 4 lines
Even worse, if your input contains any commands that prodce output, mayeb even lots of output (say "md $addr 10000") then even your increased buffer size is not going to help you.
It helps a little - provided there are no more than 250 characters more 'serial input' after the md command, all is well. But I think this example is a little contrived.

250 characters? That's 4, maybe 5 lines of text. Not exactly much...

Quoted 6 lines

I feel this is a painful restriction. I see no indication of this limitation anywhere on the command line, no error or warning, etc. It is common to want to paste in new bootargs, new network parameters, new environment variables. U-Boot has a command line which purports to support this, and with a fairly straightforward patch and in the majority of cases, it can.

Pasting commands is OK for occasional use. If you use this more frequently you're doing something wrong.

Quoted 1 line

Yes this is a minor point, but it does bite people, and usability is important.

Agreed. But consistent behaviour has it's charme as well: it's the principle of least surprise (POLS) - see
http://en.wikipedia.org/wiki/Principle_of_least_surprise

As mentioned before, I'd rather have a clear restriction that is consistent on all systems, instead of some "works for me" implementation on a few systems that will break under just slightly modified usage modes.

Best regards,

Wolfgang Denk

Signature · 5 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
Uncertain fortune is thoroughly mastered by the equity of the  calcu-
lation.                                               - Blaise Pascal

Simon Glass

author

2011-10-16 21:02 UTC #11 lore raw

PATCH v2 1/1 NS16550: buffer reads

Hi Wolfgang,

Quoted 8 lines · Wolfgang Denk

Dear Simon Glass,
Quoted 5 lines · In message <CAPnjgZ2Qf-Egdp9WCnoWVPJmjic5BPF0Db+z1oD=fdb3aoLNKw@mail.gmail.com>…
Quoted 4 lines
Even worse, if your input contains any commands that prodce output, mayeb even lots of output (say "md $addr 10000") then even your increased buffer size is not going to help you.

Quoted 5 lines

Quoted 3 lines
It helps a little - provided there are no more than 250 characters more 'serial input' after the md command, all is well. But I think this example is a little contrived.
250 characters? That's 4, maybe 5 lines of text. ?Not exactly much...

It is 250 characters *behind* the serial input. If the only output on each line is the U-Boot prompt, then this is perhaps 50 lines of input, which is plenty for most people.

Quoted 10 lines

Quoted 7 lines
I feel this is a painful restriction. I see no indication of this limitation anywhere on the command line, no error or warning, etc. It is common to want to paste in new bootargs, new network parameters, new environment variables. U-Boot has a command line which purports to support this, and with a fairly straightforward patch and in the majority of cases, it can.
Pasting commands is OK for occasional use. ?If you use this more frequently you're doing something wrong.

I don't understand this comment - it fails every time.

Quoted 6 lines

Quoted 2 lines
Yes this is a minor point, but it does bite people, and usability is important.
Agreed. ?But consistent behaviour has it's charme as well: it's the principle of least surprise (POLS) - see
http://en.wikipedia.org/wiki/Principle_of_least_surprise

That seems like an argument for this patch. When pasting lines of text into U-Boot now, the bahaviour is not consistent. Sometimes it drops a character or two and the system won't boot. Sometimes it fails to complete and it is clear that something is wrong.

Quoted 5 lines

As mentioned before, I'd rather have a clear restriction that is consistent on all systems, instead of some "works for me" implementation on a few systems that will break under just slightly modified usage modes.

Yes, so in summary, we have:

1. The XON/XOFF option, which I will look at. This requires everyone affected to switch over to XON/XOFF in whatever terminal/ser2net program they use.

2. Putting a buffer features at the serial port level (one level up from this patch) to augment the 16-byte FIFO. This would be a pain to do until someone tidies up serial. We might find a way of emitting a warning when input overflow occurs. I could look at this but not in the immediate future. Of course even this option cannot provide infinite buffer space, that would need to be clearly understood.

3. This patch, which is a point solution for NS16550 only.

It is up to you what you want to select. The patch is up if you want it, and I should be able to look at XON/XOFF this week.

My preference: option 3 now, option 1 later for those who can use XON/XOFF, option 2 much much later.

Regards,
Simon

Quoted 12 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 Uncertain fortune is thoroughly mastered by the equity of the ?calcu- lation. ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? - Blaise Pascal

Wolfgang Denk

2011-10-15 19:05 UTC #12 lore raw

PATCH v2 1/1 NS16550: buffer reads

In reply to Simon Glass

Dear Simon Glass,

Quoted 9 lines · In message <CAPnjgZ37O2tr1kY5f-uv1zymGBdMLbyEhm6y-=rfENJ62jnREg@mail.gmail.com>…

Quoted 6 lines
How about using the time-honored flow-control mechanism? If the serial port supports hardware CTS and RTS signals, you can use them to sync with the sender. If not, you can send XOFF and XON to respectively ask the sender to pause and resume sending. Of course, the sender must support hardware or software flow control.
It is common to have just a 2-wire UART for the console. Perhaps this is unfortunate.

Indeed that's a pretty common configuration - but soft-flow would still work fine.

Quoted 5 lines

Are you suggesting that U-Boot should send XOFF sometimes, or does it support this already? (I haven't seen it). When would U-Boot send XOFF? Perhaps it could send XOFF at the end of each line that it receives. Then XON when it starts reading the next line or maybe when it runs out of characters and wants more.

It should be sufficient to send XOFF when receiving a '\n' character (and storing that state) and XON when expecting inout (if state says XOFF had been sent before).

Quoted 2 lines

Most terminal problems will support software flow control. But are you sure this is better?

Definitely. for example, because it is hardware independent and will work on all architectures, all SoCs, all UARTs.

Best regards,

Wolfgang Denk

Signature · 4 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
"We don't care.  We don't have to.  We're the Phone Company."

Wolfgang Denk

2011-10-15 19:00 UTC #13 lore raw

PATCH v2 1/1 NS16550: buffer reads

In reply to Simon Glass

Dear Simon Glass,

Quoted 4 lines · In message <CAPnjgZ3wZo4nkP2GjKbS_r8AWmRaCkagFDqrXrJg5MniykxCNw@mail.gmail.com>…

The situation this occurs is when you paste characters into a serial terminal connected to U-Boot. This is a pretty common requirement. Yes you can manually select each line and paste it but that is a pain.

But that's exactly how it's supposed to work. Depending on hardware that may or may not be FIFOs available to buffer such asynchronous input, and I rather see limited, but consistent behaviour, instead of fancy stuff that works oin one board but not on another.

Quoted 2 lines

With this patch it is possible to paste a series of 'setenv' commands, for example.

If you want to run multi-line command sequences, then load and run a script.

In the special case of setting environment variables, consider using featurs like "env import" instead.

Quoted 4 lines

This caused an awful lot of confusion here because characters would be silently dropped within the command line (normally about 16 chars from the end). Then the system would not boot. It is really not a friendly experience.

There should never be character loss when sending data on a line by line base. Even the slowest processors we support are fast enough to deal with that.

Multi-line input has never been supported.

Best regards,

Wolfgang Denk

Signature · 7 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
Alan Turing thought about criteria to settle the question of  whether
machines  can think, a question of which we now know that it is about
as relevant as the question of whether submarines can swim.
                                                   -- Edsger Dijkstra

Simon Glass

author

2011-10-16 04:39 UTC #14 lore raw

PATCH v2 1/1 NS16550: buffer reads

Hi Wolfgang,

Quoted 4 lines · Wolfgang Denk

Dear Simon Glass,
Quoted 1 line · In message <CAPnjgZ3wZo4nkP2GjKbS_r8AWmRaCkagFDqrXrJg5MniykxCNw@mail.gmail.com>…

Quoted 8 lines

Quoted 3 lines
The situation this occurs is when you paste characters into a serial terminal connected to U-Boot. This is a pretty common requirement. Yes you can manually select each line and paste it but that is a pain.
But that's exactly how it's supposed to work. ?Depending on hardware that may or may not be FIFOs available to buffer such asynchronous input, and I rather see limited, but consistent behaviour, instead of fancy stuff that works oin one board but not on another.

Well this is particular serial driver is in pretty widespread use. We could of course introduce a serial buffer scheme that extends across all drivers but I don't feel this is the right time, given that there are other priorities in serial.

Quoted 6 lines

Quoted 3 lines
With this patch it is possible to paste a series of 'setenv' commands, for example.
If you want to run multi-line command sequences, then load and run a script.

If the setenv commands are configuration commands (such as network config, or setting up a new boot path) then they may not already exist in the machine. It is much easier to paste commands from a text editor / notepad than create a script, put it onto the tftp server, tftp the script and run it.

Quoted 3 lines

In the special case of setting environment variables, consider using featurs like "env import" instead.

s/and run it/and "env import" it/

Quoted 9 lines

Quoted 5 lines
This caused an awful lot of confusion here because characters would be silently dropped within the command line (normally about 16 chars from the end). Then the system would not boot. It is really not a friendly experience.
There should never be character loss when sending data on a line by line base. ?Even the slowest processors we support are fast enough to deal with that.

That's right.

Quoted 2 lines

Multi-line input has never been supported.

Hence this patch, I feel. Pasting a number of lines into the terminal is convenient, natural, useful, and when it fails it is unexpected and feels like a bug to many. I would certainly like to fix it.

Regards,
Simon

Quoted 14 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 Alan Turing thought about criteria to settle the question of ?whether machines ?can think, a question of which we now know that it is about as relevant as the question of whether submarines can swim. ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? -- Edsger Dijkstra

Wolfgang Denk

2011-10-16 19:47 UTC #15 lore raw

PATCH v2 1/1 NS16550: buffer reads

Dear Simon Glass,

Quoted 7 lines · In message <CAPnjgZ0RjEhY7-n4=vFACgzpvSgMZJOjaCbngbdB1J2HaxFK6Q@mail.gmail.com>…

Quoted 5 lines
But that's exactly how it's supposed to work. =A0Depending on hardware that may or may not be FIFOs available to buffer such asynchronous input, and I rather see limited, but consistent behaviour, instead of fancy stuff that works oin one board but not on another.
Well this is particular serial driver is in pretty widespread use. We

Widespread, yes. But there is still a huge number of boards where it is not being used.

Quoted 8 lines

Quoted 2 lines
If you want to run multi-line command sequences, then load and run a script.
If the setenv commands are configuration commands (such as network config, or setting up a new boot path) then they may not already exist in the machine. It is much easier to paste commands from a text editor / notepad than create a script, put it onto the tftp server, tftp the script and run it.

Well, there is two cases:

- You are doing this just once or twice: then it does not make much sense to create scripts or use tools; but then it is not too much of an issue to copy & paste line by line either.

- You are doing this more frequently. Then copy & paste is a cumbersome and error prone procedure. You have to be careful about quoting etc., and errors are not easy to detect. In such a situation it always makes sense to think about more efficient ways:

1) Use tools. For example, use something like "expect" or similar to send your inout line by line, always waiting for the next command prompt.

2) Use a U-Boot script image which you can download and run.

3) Use a plain text file which you can "env import".

4) ...

Quoted 4 lines

Quoted 2 lines
In the special case of setting environment variables, consider using featurs like "env import" instead.
s/and run it/and "env import" it/

No. Running a script image is one solution; using "env import" is another, independent one.

Quoted 5 lines

Quoted 1 line
Multi-line input has never been supported.
Hence this patch, I feel. Pasting a number of lines into the terminal is convenient, natural, useful, and when it fails it is unexpected and feels like a bug to many. I would certainly like to fix it.

But your patch does not _solve_ the problem. It just extends the limits a bit - to som degree that seems to be acceptable to you, but the next person might paste a few lines more, and it's failing again.

I think it's much better to state clearly that multi-line input is not supported than to claim it is, without being able to give a precise limit where it will fail, or without being able to detect error situations clearly.

Best regards,

Wolfgang Denk

Signature · 5 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
Little known fact about Middle Earth:   The Hobbits had a very sophi-
sticated computer network!   It was a Tolkien Ring...

Simon Glass

author

2011-10-16 20:43 UTC #16 lore raw

PATCH v2 1/1 NS16550: buffer reads

Hi Wolfgang,

Quoted 9 lines · Wolfgang Denk

Dear Simon Glass,
Quoted 6 lines · In message <CAPnjgZ0RjEhY7-n4=vFACgzpvSgMZJOjaCbngbdB1J2HaxFK6Q@mail.gmail.com>…
Quoted 5 lines
But that's exactly how it's supposed to work. =A0Depending on hardware that may or may not be FIFOs available to buffer such asynchronous input, and I rather see limited, but consistent behaviour, instead of fancy stuff that works oin one board but not on another.

Quoted 4 lines

Quoted 1 line
Well this is particular serial driver is in pretty widespread use. We
Widespread, yes. ?But there is still a huge number of boards where it is not being used.

Yes.

Quoted 25 lines

Quoted 9 lines
Quoted 3 lines
If you want to run multi-line command sequences, then load and run a script.
If the setenv commands are configuration commands (such as network config, or setting up a new boot path) then they may not already exist in the machine. It is much easier to paste commands from a text editor / notepad than create a script, put it onto the tftp server, tftp the script and run it.
Well, there is two cases:
- You are doing this just once or twice: then it does not make much ?sense to create scripts or use tools; but then it is not too much of ?an issue to copy & paste line by line either.
- You are doing this more frequently. ?Then copy & paste is a ?cumbersome and error prone procedure. You have to be careful about ?quoting etc., and errors are not easy to detect. ?In such a ?situation it always makes sense to think about more efficient ways:
?1) Use tools. ?For example, use something like "expect" or similar ? ? to send your inout line by line, always waiting for the next ? ? command prompt.

Quoted 5 lines

?2) Use a U-Boot script image which you can download and run.
?3) Use a plain text file which you can "env import".
?4) ...

I think this is what said before. In the case of expect, I must put the board into the right state, disconnect my terminal from the board, run the expect script and re-connect.

In the case of 2 and 3, they are essentially the same. Both require a file transfer to the board (perhaps tftp) which involves editing and putting the file somewhere.

This is not friendly behaviour.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
In the special case of setting environment variables, consider using featurs like "env import" instead.
s/and run it/and "env import" it/
No. ?Running a script image is one solution; ?using "env import" is another, independent one.

To repeat the bit of my message that you omitted:

Quoted 5 lines

If the setenv commands are configuration commands (such as network config, or setting up a new boot path) then they may not already exist in the machine. It is much easier to paste commands from a text editor / notepad than create a script, put it onto the tftp server, tftp the script and run it.

My point was that they both require putting the file somewhere and using the network to get the file.

Quoted 10 lines

Quoted 6 lines
Quoted 2 lines
Multi-line input has never been supported.
Hence this patch, I feel. Pasting a number of lines into the terminal is convenient, natural, useful, and when it fails it is unexpected and feels like a bug to many. I would certainly like to fix it.
But your patch does not _solve_ the problem. ?It just extends the limits a bit - to som degree that seems to be acceptable to you, but the next person might paste a few lines more, and it's failing again.

Well I would like to have a message that a serial overflow happened - we don't have the means to check this at present but perhaps one day...

With this patch, using commands like setenv that don't emit output, I can paste at least 50 lines of text.

The problem over buffer overflow is not soluble in the limit. I could paste the works of Shakespeare into U-Boot but it would run out of buffer memory. There is always a buffer limit - at present it is about 16 characters which means within a few lines you run of space. This patch increases that to 256 characters.

I have already granted that the patch should perhaps move up a level, to encompass serial, but the serial layer is not ready for that yet.

Quoted 5 lines

I think it's much better to state clearly that multi-line input is not supported than to claim it is, without being able to give a precise limit where it will fail, or without being able to detect error situations clearly.

I would agree with this if there were an error message or warning that serial input overflow occurred. At the moment it is just confusing or buggy.

Regards,
SImon

Quoted 12 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 Little known fact about Middle Earth: ? The Hobbits had a very sophi- sticated computer network! ? It was a Tolkien Ring...

Wolfgang Denk

2011-10-16 21:00 UTC #17 lore raw

PATCH v2 1/1 NS16550: buffer reads

Dear Simon Glass,

Quoted 4 lines · In message <CAPnjgZ0yVMqzyGceUEXiBt6dtsVuqikSPJ_Vw8VU_nb-ST6zJA@mail.gmail.com>…

I think this is what said before. In the case of expect, I must put the board into the right state, disconnect my terminal from the board, run the expect script and re-connect.

You can run it under expect all the time when you are planning to perform such actions.

Quoted 5 lines

In the case of 2 and 3, they are essentially the same. Both require a file transfer to the board (perhaps tftp) which involves editing and putting the file somewhere.
This is not friendly behaviour.

Come on. It's even more friendly than what you are doing now.

With "env import" you can just use the output from "printenv" on a working, tested board, copy and paste this into a file, and then download nd import that (through network or serial download - serial download is not slower than pasting it over the serial line - just way more reliable).

For copy and paste, you have to manuallay insert "setenv " in front of all lines, and add appropriate quoting.

And the BIG benfit is: it just works.

Quoted 8 lines

Quoted 5 lines
If the setenv commands are configuration commands (such as network config, or setting up a new boot path) then they may not already exist in the machine. It is much easier to paste commands from a text editor / notepad than create a script, put it onto the tftp server, tftp the script and run it.
My point was that they both require putting the file somewhere and using the network to get the file.

Put the file somehwere, yes. Some file anywhere on your system. Not a big problem, or is it?

Using the network: this is one option; it's the most convenient one if
network is already configured. Otherwise just use serial download.

Quoted 3 lines

I would agree with this if there were an error message or warning that serial input overflow occurred. At the moment it is just confusing or buggy.

But your patch is NOT fixing that. It just pushes the limits a bit.

Best regards,

Wolfgang Denk

Signature · 5 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
It is clear that the individual who persecutes a  man,  his  brother,
because he is not of the same opinion, is a monster.       - Voltaire

Sandeep Kumar

2011-10-17 04:56 UTC #18 lore raw

[U-Boot] How to unsubscribe.

In reply to Simon Glass

Hi,
How can I unsubscribe from this mailing list.

Regards,
Sandeep

-----Original Message-----
From: [email protected] [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Simon Glass
Sent: 15 October 2011 05:33
To: U-Boot Mailing List
Cc: Scott Wood; Kumar Gala
Subject: [U-Boot] [PATCH v2] 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

 README                   |    8 ++++
 drivers/serial/ns16550.c |   96
+++++++++++++++++++++++++++++++++++++++++++--
 drivers/serial/serial.c  |    5 +-
 include/ns16550.h        |    4 +-
 4 files changed, 104 insertions(+), 9 deletions(-)

README +0 -0

index 7e032a9..28d2e8a 100644
@@ -2862,6 +2862,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:
---------------------------------------------------

drivers/serial/ns16550.c +3 -0

index 0174744..a012e5d 100644
@@ -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(&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 */

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 · 6 lines

1.7.3.1

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Mike Frysinger

2011-10-17 15:40 UTC #19 lore raw

[U-Boot] How to unsubscribe.

Quoted 1 line · On Monday 17 October 2011 00:56:37 Sandeep Kumar wrote:

How can I unsubscribe from this mailing list.

read the link at the end of every e-mail
-mike