[U-Boot] [PATCH v4 1/2] NS16550: trivial code clean for checkpatch

[U-Boot] [PATCH v4 1/2] NS16550: trivial code clean for checkpatch

[Simon Glass]

This removes most checkpatch warnings from the ns16550 driver and its
header.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 drivers/serial/ns16550.c |   37 ++++++++++++++++++++-----------------
 include/ns16550.h        |   16 ++++++++--------
 2 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 0174744..056c25d 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -17,24 +17,24 @@
 		     UART_FCR_RXSR |	\
 		     UART_FCR_TXSR)		/* Clear & enable FIFOs */
 #ifdef CONFIG_SYS_NS16550_PORT_MAPPED
-#define serial_out(x,y)	outb(x,(ulong)y)
-#define serial_in(y)	inb((ulong)y)
+#define serial_out(x, y)	outb(x, (ulong)y)
+#define serial_in(y)		inb((ulong)y)
 #elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE > 0)
-#define serial_out(x,y) out_be32(y,x)
-#define serial_in(y) 	in_be32(y)
+#define serial_out(x, y)	out_be32(y, x)
+#define serial_in(y)		in_be32(y)
 #elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE < 0)
-#define serial_out(x,y) out_le32(y,x)
-#define serial_in(y) 	in_le32(y)
+#define serial_out(x, y)	out_le32(y, x)
+#define serial_in(y)		in_le32(y)
 #else
-#define serial_out(x,y) writeb(x,y)
-#define serial_in(y) 	readb(y)
+#define serial_out(x, y)	writeb(x, y)
+#define serial_in(y)		readb(y)
 #endif
 
 #ifndef CONFIG_SYS_NS16550_IER
 #define CONFIG_SYS_NS16550_IER  0x00
 #endif /* CONFIG_SYS_NS16550_IER */
 
-void NS16550_init (NS16550_t com_port, int baud_divisor)
+void NS16550_init(NS16550_t com_port, int baud_divisor)
 {
 	serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
 #if defined(CONFIG_OMAP) && !defined(CONFIG_OMAP3_ZOOM2)
@@ -52,15 +52,17 @@ void NS16550_init (NS16550_t com_port, int baud_divisor)
 	serial_out(UART_LCRVAL, &com_port->lcr);
 #if defined(CONFIG_OMAP) && !defined(CONFIG_OMAP3_ZOOM2)
 #if defined(CONFIG_APTIX)
-	serial_out(3, &com_port->mdr1);	/* /13 mode so Aptix 6MHz can hit 115200 */
+	/* /13 mode so Aptix 6MHz can hit 115200 */
+	serial_out(3, &com_port->mdr1);
 #else
-	serial_out(0, &com_port->mdr1);	/* /16 is proper to hit 115200 with 48MHz */
+	/* /16 is proper to hit 115200 with 48MHz */
+	serial_out(0, &com_port->mdr1);
 #endif
 #endif /* CONFIG_OMAP */
 }
 
 #ifndef CONFIG_NS16550_MIN_FUNCTIONS
-void NS16550_reinit (NS16550_t com_port, int baud_divisor)
+void NS16550_reinit(NS16550_t com_port, int baud_divisor)
 {
 	serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
 	serial_out(UART_LCR_BKSE | UART_LCRVAL, &com_port->lcr);
@@ -76,9 +78,10 @@ void NS16550_reinit (NS16550_t com_port, int baud_divisor)
 }
 #endif /* CONFIG_NS16550_MIN_FUNCTIONS */
 
-void NS16550_putc (NS16550_t com_port, char c)
+void NS16550_putc(NS16550_t com_port, char c)
 {
-	while ((serial_in(&com_port->lsr) & UART_LSR_THRE) == 0);
+	while ((serial_in(&com_port->lsr) & UART_LSR_THRE) == 0)
+		;
 	serial_out(c, &com_port->thr);
 
 	/*
@@ -92,7 +95,7 @@ void NS16550_putc (NS16550_t com_port, char c)
 }
 
 #ifndef CONFIG_NS16550_MIN_FUNCTIONS
-char NS16550_getc (NS16550_t com_port)
+char NS16550_getc(NS16550_t com_port)
 {
 	while ((serial_in(&com_port->lsr) & UART_LSR_DR) == 0) {
 #ifdef CONFIG_USB_TTY
@@ -104,9 +107,9 @@ char NS16550_getc (NS16550_t com_port)
 	return serial_in(&com_port->rbr);
 }
 
-int NS16550_tstc (NS16550_t com_port)
+int NS16550_tstc(NS16550_t com_port)
 {
-	return ((serial_in(&com_port->lsr) & UART_LSR_DR) != 0);
+	return (serial_in(&com_port->lsr) & UART_LSR_DR) != 0;
 }
 
 #endif /* CONFIG_NS16550_MIN_FUNCTIONS */
diff --git a/include/ns16550.h b/include/ns16550.h
index 51f1c17..e9d2eda 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -65,12 +65,12 @@ struct NS16550 {
 #define dll rbr
 #define dlm ier
 
-typedef volatile struct NS16550 *NS16550_t;
+typedef struct NS16550 *NS16550_t;
 
 /*
  * These are the definitions for the FIFO Control Register
  */
-#define UART_FCR_FIFO_EN 	0x01 /* Fifo enable */
+#define UART_FCR_FIFO_EN	0x01 /* Fifo enable */
 #define UART_FCR_CLEAR_RCVR	0x02 /* Clear the RCVR FIFO */
 #define UART_FCR_CLEAR_XMIT	0x04 /* Clear the XMIT FIFO */
 #define UART_FCR_DMA_SELECT	0x08 /* For DMA applications */
@@ -106,7 +106,7 @@ typedef volatile struct NS16550 *NS16550_t;
 #define UART_LCR_WLS_6	0x01		/* 6 bit character length */
 #define UART_LCR_WLS_7	0x02		/* 7 bit character length */
 #define UART_LCR_WLS_8	0x03		/* 8 bit character length */
-#define UART_LCR_STB	0x04		/* Number of stop Bits, off = 1, on = 1.5 or 2) */
+#define UART_LCR_STB	0x04		/* # stop Bits, off=1, on=1.5 or 2) */
 #define UART_LCR_PEN	0x08		/* Parity eneble */
 #define UART_LCR_EPS	0x10		/* Even Parity Select */
 #define UART_LCR_STKP	0x20		/* Stick Parity */
@@ -162,8 +162,8 @@ typedef volatile struct NS16550 *NS16550_t;
 /* useful defaults for LCR */
 #define UART_LCR_8N1	0x03
 
-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);
-void	NS16550_reinit (NS16550_t com_port, int baud_divisor);
+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);
+void NS16550_reinit(NS16550_t com_port, int baud_divisor);
-- 
1.7.3.1

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Simon Glass]

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

Changes in v4:
- Change config option to CONFIG_NS16550_RBUF_SIZE
- Add additional checkpatch-cleanup patch

 README                   |   12 ++++++
 drivers/serial/ns16550.c |   97 +++++++++++++++++++++++++++++++++++++++++++--
 drivers/serial/serial.c  |    5 +-
 include/ns16550.h        |    4 +-
 4 files changed, 109 insertions(+), 9 deletions(-)

diff --git a/README b/README
index 7e032a9..d8b4c4d 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_RBUF_SIZE:
+		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_RBUF_SIZE 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 056c25d..12de014 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 */
@@ -95,21 +98,105 @@ 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_RBUF_SIZE
+
+#define NUM_PORTS 4
+
+struct ns16550_priv {
+	char buf[CONFIG_NS16550_RBUF_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) %
+			CONFIG_NS16550_RBUF_SIZE == 0)
+		return;
+
+	rxstate[port].buf[rxstate[port].tail] = ch;
+	rxstate[port].tail = (rxstate[port].tail + 1) %
+		CONFIG_NS16550_RBUF_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) %
+		CONFIG_NS16550_RBUF_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_RBUF_SIZE */
+
+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_RBUF_SIZE */
 #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 e9d2eda..ed77722 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -164,6 +164,6 @@ typedef struct NS16550 *NS16550_t;
 
 void NS16550_init(NS16550_t com_port, int baud_divisor);
 void NS16550_putc(NS16550_t com_port, char c);
-char NS16550_getc(NS16550_t com_port);
-int NS16550_tstc(NS16550_t com_port);
+char NS16550_getc(NS16550_t regs, unsigned int port);
+int NS16550_tstc(NS16550_t regs, unsigned int port);
 void NS16550_reinit(NS16550_t com_port, int baud_divisor);
-- 
1.7.3.1

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Albert ARIBAUD]

Hi Simon,

Le 16/10/2011 07:14, Simon Glass a ?crit :
> 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
>
> Changes in v4:
> - Change config option to CONFIG_NS16550_RBUF_SIZE
> - Add additional checkpatch-cleanup patch

Hmm... What about the conversation around V2 of the patch re: using 
XOFF/XON to control input flow? IIUC, even this V4 patch would not help 
much if there is a command within the pasted code that sends a lot of 
output, right?

Amicalement,
-- 
Albert.

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Simon Glass]

Hi Albert,

On Sun, Oct 16, 2011 at 5:57 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Simon,
>
> Le 16/10/2011 07:14, Simon Glass a ?crit :
>>
>> 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
>>
>> Changes in v4:
>> - Change config option to CONFIG_NS16550_RBUF_SIZE
>> - Add additional checkpatch-cleanup patch
>
> Hmm... What about the conversation around V2 of the patch re: using XOFF/XON
> to control input flow? IIUC, even this V4 patch would not help much if there
> is a command within the pasted code that sends a lot of output, right?

Well it will help so long as there isn't much more input. Perhaps the
way to think of it is that every character you output that isn't just
an echo, puts you one more character behind the input. This adds one
character to the buffer in this patch. When you have fallen (say) 256
characters behind the input, then you will start losing characters
even with this patch. However it is rare to paste in commands which
send lots of output - and 3 lines of full output is pretty uncommon in
my experience.

I feel that relying on the first level UART FIFO as the only buffering
is brave, and cannot be justified by lack of interrupts alone. There
seems to me to be no fundamental reason why U-Boot should not buffer
its input for when it is latest ready to process it. Granted this
should perhaps be higher up the software stack, but until someone
tidies up serial a little I suspect that would be a pain to implement.
With a more generic serial layer (a device pointer and a ->priv
pointer across all drivers for example) we could attach this buffering
as a separate file to be enabled / disabled by a Makefile.

To address your question, I think we did discuss it. Wolfgang felt
that it should be as simple as sending XOFF when receiving the end of
a line and XON when waiting for characters on the new line. It
certainly sounds plausible.

I have other reasons for favouring this patch. On the main hardware
platform I currently use, SPI and the console UART are multiplexed, so
before using SPI (e.g. saveenv, reading the environment) we must read
in any pending characters to avoid corruption. After re-enabling the
UART later we must clear out any junk that has arrived. This patch
provides a way around that which XON/XOFF does not (since when you
send XOFF you may already have input characters outstanding - junk
will later be added and you can't tell the difference).

I am unsure whether to bring this up or not - clearly this platform is
not ideal, but it is in very common use and we need to support it with
U-Boot.

Regards,
Simon

>
> Amicalement,
> --
> Albert.
>

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ2Q3XtGK=5HhF0KhQo4XQDA6CAH3QFKfmgCwzNV_y0mUA@mail.gmail.com> you wrote:
> 
> > Hmm... What about the conversation around V2 of the patch re: using XOFF/XON
> > to control input flow? IIUC, even this V4 patch would not help much if there
> > is a command within the pasted code that sends a lot of output, right?
>
> Well it will help so long as there isn't much more input. Perhaps the

Don't you see that this "as long as there isn't much more input" is
the fatal flaw of your patch?  You are not fixing the problem, you are
just extending the point wher eit hits to some extend - it may work
for a few more situations, but it's still guaranteed to fail for
others.

> To address your question, I think we did discuss it. Wolfgang felt
> that it should be as simple as sending XOFF when receiving the end of
> a line and XON when waiting for characters on the new line. It
> certainly sounds plausible.

Then please let's take this approach instead, if you really need to
support such a mode of operation.

> I have other reasons for favouring this patch. On the main hardware
> platform I currently use, SPI and the console UART are multiplexed, so
> before using SPI (e.g. saveenv, reading the environment) we must read
> in any pending characters to avoid corruption. After re-enabling the
> UART later we must clear out any junk that has arrived. This patch
> provides a way around that which XON/XOFF does not (since when you
> send XOFF you may already have input characters outstanding - junk
> will later be added and you can't tell the difference).

That's an unrelated hardware issue that needs to be addressed
independently.

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
Most legends have their basis in facts.
	-- Kirk, "And The Children Shall Lead", stardate 5029.5

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Simon Glass]

Hi Wolfgang,

On Sun, Oct 16, 2011 at 1:13 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ2Q3XtGK=5HhF0KhQo4XQDA6CAH3QFKfmgCwzNV_y0mUA@mail.gmail.com> you wrote:
>>
>> > Hmm... What about the conversation around V2 of the patch re: using XOFF/XON
>> > to control input flow? IIUC, even this V4 patch would not help much if there
>> > is a command within the pasted code that sends a lot of output, right?
>>
>> Well it will help so long as there isn't much more input. Perhaps the
>
> Don't you see that this "as long as there isn't much more input" is
> the fatal flaw of your patch? ?You are not fixing the problem, you are
> just extending the point wher eit hits to some extend - it may work
> for a few more situations, but it's still guaranteed to fail for
> others.

In a similar way, the Linux kernel has a fatal flaw. Serial data
coming into the flip buffers under extreme interrupt load can be lost,
or the secondary buffers can become exhausted, or user space cannot
keep up with input under heavy load, etc., etc. This is the reality of
the world. As each problem presents itself we direct our attention to
it.

>
>> To address your question, I think we did discuss it. Wolfgang felt
>> that it should be as simple as sending XOFF when receiving the end of
>> a line and XON when waiting for characters on the new line. It
>> certainly sounds plausible.
>
> Then please let's take this approach instead, if you really need to
> support such a mode of operation.

As I said I will look at this.

>
>> I have other reasons for favouring this patch. On the main hardware
>> platform I currently use, SPI and the console UART are multiplexed, so
>> before using SPI (e.g. saveenv, reading the environment) we must read
>> in any pending characters to avoid corruption. After re-enabling the
>> UART later we must clear out any junk that has arrived. This patch
>> provides a way around that which XON/XOFF does not (since when you
>> send XOFF you may already have input characters outstanding - junk
>> will later be added and you can't tell the difference).
>
> That's an unrelated hardware issue that needs to be addressed
> independently.

Yes it is. I can't help wondering what the solution might be though,
if we are not allowed to buffer things.

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 at denx.de
> Most legends have their basis in facts.
> ? ? ? ?-- Kirk, "And The Children Shall Lead", stardate 5029.5
>

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ1VUF-HFbDX9byuV32sJfEE3SKRiT124gfgQOm0f+QSEw@mail.gmail.com> you wrote:
> 
> In a similar way, the Linux kernel has a fatal flaw. Serial data
> coming into the flip buffers under extreme interrupt load can be lost,
> or the secondary buffers can become exhausted, or user space cannot
> keep up with input under heavy load, etc., etc. This is the reality of
> the world. As each problem presents itself we direct our attention to
> it.

This is not a fatal flaw.  There is nothing in the definition of RS232
or the serialinterface that guarantees you an error free data stream -
not on that protocol level.  You are facing loss of Ethernet packages
on the network as well.

Problems start only when you make wrong assumptions.


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
A girl with a future avoids the man with a past.
                                   -- Evan Esar, "The Humor of Humor"

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Wolfgang Denk]

Dear Simon Glass,

In message <1318742050-2201-2-git-send-email-sjg@chromium.org> you wrote:
...
> 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.

One problem I see is that the code size on some boards grows even if
the new option is not enabled.  Here a compare before and after
applying your patches (without any changes to the respective board
configurations):

 Configuring for AR405 board...
    text    data     bss     dec     hex filename
- 246058   12972   14636  273666   42d02 /work/wd/tmp-ppc/u-boot
+ 246062   12972   14636  273670   42d06 /work/wd/tmp-ppc/u-boot
 Configuring for CANBT board...
    text    data     bss     dec     hex filename
- 120710    8604    3876  133190   20846 /work/wd/tmp-ppc/u-boot
+ 120714    8604    3876  133194   2084a /work/wd/tmp-ppc/u-boot
 Configuring for pcs440ep board...
    text    data     bss     dec     hex filename
- 296191   19636  345088  660915   a15b3 /work/wd/tmp-ppc/u-boot
+ 296195   19636  345088  660919   a15b7 /work/wd/tmp-ppc/u-boot
...

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
"Out of register space (ugh)"
- vi

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Simon Glass]

Hi Wolfgang,

On Mon, Oct 17, 2011 at 4:08 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1318742050-2201-2-git-send-email-sjg@chromium.org> you wrote:
> ...
>> 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.
>
> One problem I see is that the code size on some boards grows even if
> the new option is not enabled. ?Here a compare before and after
> applying your patches (without any changes to the respective board
> configurations):
>
> ?Configuring for AR405 board...
> ? ?text ? ?data ? ? bss ? ? dec ? ? hex filename
> - 246058 ? 12972 ? 14636 ?273666 ? 42d02 /work/wd/tmp-ppc/u-boot
> + 246062 ? 12972 ? 14636 ?273670 ? 42d06 /work/wd/tmp-ppc/u-boot
> ?Configuring for CANBT board...
> ? ?text ? ?data ? ? bss ? ? dec ? ? hex filename
> - 120710 ? ?8604 ? ?3876 ?133190 ? 20846 /work/wd/tmp-ppc/u-boot
> + 120714 ? ?8604 ? ?3876 ?133194 ? 2084a /work/wd/tmp-ppc/u-boot
> ?Configuring for pcs440ep board...
> ? ?text ? ?data ? ? bss ? ? dec ? ? hex filename
> - 296191 ? 19636 ?345088 ?660915 ? a15b3 /work/wd/tmp-ppc/u-boot
> + 296195 ? 19636 ?345088 ?660919 ? a15b7 /work/wd/tmp-ppc/u-boot
> ...
>

This doesn't happen for me on ARM - the compiler just inlines the new
static raw functions into the public functions. I will see if I can
figure out what is happening on PPC. It is 4 bytes, or one
instruction, right?

The fix might be to duplicate more code, or perhaps add an inline
keyword to the two functions.

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 at denx.de
> "Out of register space (ugh)"
> - vi
>

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Simon Glass]

On Mon, Oct 17, 2011 at 4:08 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1318742050-2201-2-git-send-email-sjg@chromium.org> you wrote:
> ...
>> 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.
>
> One problem I see is that the code size on some boards grows even if
> the new option is not enabled. ?Here a compare before and after
> applying your patches (without any changes to the respective board
> configurations):
>
> ?Configuring for AR405 board...
> ? ?text ? ?data ? ? bss ? ? dec ? ? hex filename
> - 246058 ? 12972 ? 14636 ?273666 ? 42d02 /work/wd/tmp-ppc/u-boot
> + 246062 ? 12972 ? 14636 ?273670 ? 42d06 /work/wd/tmp-ppc/u-boot
> ?Configuring for CANBT board...
> ? ?text ? ?data ? ? bss ? ? dec ? ? hex filename
> - 120710 ? ?8604 ? ?3876 ?133190 ? 20846 /work/wd/tmp-ppc/u-boot
> + 120714 ? ?8604 ? ?3876 ?133194 ? 2084a /work/wd/tmp-ppc/u-boot
> ?Configuring for pcs440ep board...
> ? ?text ? ?data ? ? bss ? ? dec ? ? hex filename
> - 296191 ? 19636 ?345088 ?660915 ? a15b3 /work/wd/tmp-ppc/u-boot
> + 296195 ? 19636 ?345088 ?660919 ? a15b7 /work/wd/tmp-ppc/u-boot
> ...

Hi Wolfgang,

Can you please tell me which ELDK version this is using? I see that
the 405 board seems to need ppc_4xx which suggests 4.2 rather than 5,
since in 5 the compiler is called powerpc-

But I might be confused.

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 at denx.de
> "Out of register space (ugh)"
> - vi
>

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ1412ezJh3f4Ww16k4Z55kJdgGWj7+vMyK_ot-MzVd70g@mail.gmail.com> you wrote:
>
> Can you please tell me which ELDK version this is using? I see that
> the 405 board seems to need ppc_4xx which suggests 4.2 rather than 5,
> since in 5 the compiler is called powerpc-

Right, I was testing this with ELDK 4.2.

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
You can observe a lot just by watchin'.                  - Yogi Berra

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Simon Glass]

Hi Wolfgang,

On Mon, Oct 17, 2011 at 1:33 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ1412ezJh3f4Ww16k4Z55kJdgGWj7+vMyK_ot-MzVd70g@mail.gmail.com> you wrote:
>>
>> Can you please tell me which ELDK version this is using? I see that
>> the 405 board seems to need ppc_4xx which suggests 4.2 rather than 5,
>> since in 5 the compiler is called powerpc-
>
> Right, I was testing this with ELDK 4.2.

I am struggling to repeat this and don't even get the same numbers...

For your AR405 board, you saw:
> - 246058   12972   14636  273666   42d02 /work/wd/tmp-ppc/u-boot
> + 246062   12972   14636  273670   42d06 /work/wd/tmp-ppc/u-boot

For me:

ppc_4xx-gcc -v
Reading specs from
/opt/eldk-4.2.4xx/usr/bin/../lib/gcc/powerpc-linux/4.2.2/specs
Target: powerpc-linux
Configured with:
/opt/eldk/build/ppc-2008-04-01/work/usr/src/denx/BUILD/crosstool-0.43/build/gcc-4.2.2-glibc-20070515T2025-eldk/powerpc-linux/gcc-4.2.2/configure
--target=powerpc-linux --host=i686-host_pc-linux-gnu
--prefix=/var/tmp/eldk.UZpAG7/usr/crosstool/gcc-4.2.2-glibc-20070515T2025-eldk/powerpc-linux
--disable-hosted-libstdcxx
--with-headers=/var/tmp/eldk.UZpAG7/usr/crosstool/gcc-4.2.2-glibc-20070515T2025-eldk/powerpc-linux/powerpc-linux/include
--with-local-prefix=/var/tmp/eldk.UZpAG7/usr/crosstool/gcc-4.2.2-glibc-20070515T2025-eldk/powerpc-linux/powerpc-linux
--disable-nls --enable-threads=posix --enable-symvers=gnu
--enable-__cxa_atexit --enable-languages=c,c++,java --enable-shared
--enable-c99 --enable-long-long --without-x
Thread model: posix
gcc version 4.2.2

ARCH=powerpc make O=ppc AR405_config
ARCH=powerpc make O=ppc

without feature (checkout of upstream/master d8fffa05):
$ ppc_4xx-objdump -h ppc/drivers/serial/ns16550.o
...
  7 .text.NS16550_tstc 00000020  00000000  00000000  00000bf4  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  8 .text.NS16550_putc 0000002c  00000000  00000000  00000c14  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  9 .text.NS16550_getc 00000038  00000000  00000000  00000c40  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
 10 .text.NS16550_init 0000007c  00000000  00000000  00000c78  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
 11 .text.NS16550_reinit 00000080  00000000  00000000  00000cf4  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE

   text	   data	    bss	    dec	    hex	filename
 245942	  12964	  14632	 273538	  42c82	ppc/u-boot


with feature but not enabled (with Scott's two patches):
  7 .text.NS16550_putc 0000002c  00000000  00000000  00000fec  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  8 .text.NS16550_getc 00000038  00000000  00000000  00001018  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  9 .text.NS16550_tstc 00000020  00000000  00000000  00001050  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
 10 .text.NS16550_init 0000007c  00000000  00000000  00001070  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
 11 .text.NS16550_reinit 00000080  00000000  00000000  000010ec  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE

size ppc/u-boot
   text	   data	    bss	    dec	    hex	filename
 245942	  12964	  14632	 273538	  42c82	ppc/u-boot

Do you have any suggestions please?

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 at denx.de
> You can observe a lot just by watchin'. ? ? ? ? ? ? ? ? ?- Yogi Berra
>

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Albert ARIBAUD]

Le 17/10/2011 22:58, Simon Glass a ?crit :
> Hi Wolfgang,
>
> On Mon, Oct 17, 2011 at 1:33 PM, Wolfgang Denk<wd@denx.de>  wrote:
>> Dear Simon Glass,
>>
>> In message<CAPnjgZ1412ezJh3f4Ww16k4Z55kJdgGWj7+vMyK_ot-MzVd70g@mail.gmail.com>  you wrote:
>>>
>>> Can you please tell me which ELDK version this is using? I see that
>>> the 405 board seems to need ppc_4xx which suggests 4.2 rather than 5,
>>> since in 5 the compiler is called powerpc-
>>
>> Right, I was testing this with ELDK 4.2.
>
> I am struggling to repeat this and don't even get the same numbers...

Could be due to two things:

1) both of you do not work from the same exact tree commit, or

2) both of you work in a different base directory

> For your AR405 board, you saw:
>> - 246058   12972   14636  273666   42d02 /work/wd/tmp-ppc/u-boot
>> + 246062   12972   14636  273670   42d06 /work/wd/tmp-ppc/u-boot

> For me:

>   245942	  12964	  14632	 273538	  42c82	ppc/u-boot

I suggest Wolfgang and you compare your trees to eliminate risk 1, and 
make sure you compare .bin files to reduce risk 2.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Wolfgang Denk]

Dear Simon,

In message <1318742050-2201-2-git-send-email-sjg@chromium.org> you wrote:
> 
> 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.

Let me try to summarize my thinking:

- It is a fundamental design decision that U-Boot is single tasking.
  This implies that while a command is running, no other things get
  done in parallel. This includes communication over the serial line,
  USB, Ethernet, ...

- That means that by design U-Boot does not support multi-line input:
  as soon as you hit "enter" U-Boot will start executing your command,
  and will only become ready for new input when it has completed the
  execution of the command(s), i. e. after printing the next prompt.

- For this suported mode of operation your patch is not needed. It
  just adds code size and complexity.

- Your description "rapid input" is actually wrong.  The speed of
  input over the serial line is limited by the baud rate settings,
  and even if you transfer at maximum speed all supported systems
  are fast enough to receive the data without loss.

- The use case you are trying to support is actually multi-line
  input, so you should describe it as such.  I agree that this would
  be an interesting feature for some use cases, but if we go on and
  implement it, we should (1) document it properly and (2) implement
  it in a way that works reliably.

- However, your implementation does not result in reliable multi-line
  input.  It works only in a few specific use cases, and will fail
  silently for others.  I don't see a way ho you would announce this
  new feature.  The numbers you mention in the commit message ("it
  handles around 70 lines before lossage") apply for this specific
  board only, and for your use case only (pasting of short lines that
  produce no output).

  So essentially you are saying: hey, we now have multi-line input,
  but it works only a bit.  It will fail sooner or later, without
  error messages, and I cannot even tell you what the limits for your
  system are.  And it depends on which input you send.

  This does not exactly sound promising.


On the other hand, we also have the rule that things that are useful
to some and that don't hurt others should be allowed to go in.

What makes me hesitate are two things:

- The patch promises a feature (multi-line input), but fails to
  provide a reliably working implementation.

- As it turns out, the patch increases code size even for boards that
  do not activate this feature.


I have to admit that I'm at a loss with a decision here.

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
"We don't have to protect the environment -- the Second Coming is  at
hand."                                                   - James Watt

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Simon Glass]

Hi Wolfgang,

On Mon, Oct 17, 2011 at 5:18 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon,
>
> In message <1318742050-2201-2-git-send-email-sjg@chromium.org> you wrote:
>>
>> 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.
>
> Let me try to summarize my thinking:
>
> - It is a fundamental design decision that U-Boot is single tasking.
> ?This implies that while a command is running, no other things get
> ?done in parallel. This includes communication over the serial line,
> ?USB, Ethernet, ...
>
> - That means that by design U-Boot does not support multi-line input:
> ?as soon as you hit "enter" U-Boot will start executing your command,
> ?and will only become ready for new input when it has completed the
> ?execution of the command(s), i. e. after printing the next prompt.
>
> - For this suported mode of operation your patch is not needed. It
> ?just adds code size and complexity.
>
> - Your description "rapid input" is actually wrong. ?The speed of
> ?input over the serial line is limited by the baud rate settings,
> ?and even if you transfer at maximum speed all supported systems
> ?are fast enough to receive the data without loss.
>
> - The use case you are trying to support is actually multi-line
> ?input, so you should describe it as such. ?I agree that this would
> ?be an interesting feature for some use cases, but if we go on and
> ?implement it, we should (1) document it properly and (2) implement
> ?it in a way that works reliably.

I can certainly improve the documentation and commit message.

>
> - However, your implementation does not result in reliable multi-line
> ?input. ?It works only in a few specific use cases, and will fail
> ?silently for others. ?I don't see a way ho you would announce this
> ?new feature. ?The numbers you mention in the commit message ("it
> ?handles around 70 lines before lossage") apply for this specific
> ?board only, and for your use case only (pasting of short lines that
> ?produce no output).
>
> ?So essentially you are saying: hey, we now have multi-line input,
> ?but it works only a bit. ?It will fail sooner or later, without
> ?error messages, and I cannot even tell you what the limits for your
> ?system are. ?And it depends on which input you send.
>
> ?This does not exactly sound promising.

That all sounds reasonable.

I think it is good enough, but not perfect. Due to the fundamental
design decision you mention at the top, we cannot squirrel away serial
input in the background. The best we can do is squirrel it away in the
foreground, as we output new serial data (I suppose we could squirrel
it away in net loops and other places but I don't want to go there!).
This turns out to be mostly good enough, because it is rare for people
to want to paste 'sleep 10' and the like into their terminal (your
other message).

I would like to spit out a warning when serial input is lost - as I
mentioned that could be combined with a serial overhaul at some point.

>
>
> On the other hand, we also have the rule that things that are useful
> to some and that don't hurt others should be allowed to go in.
>
> What makes me hesitate are two things:
>
> - The patch promises a feature (multi-line input), but fails to
> ?provide a reliably working implementation.

I *think* it does the best possible within the fundamental design
decision constraint. If there is more it can do, please let me have
your ideas. I don't believe buffering conflicts with the constraint -
they are separate things. But yes in systems with interrupts normally
the input buffer is filled in the background and drained in the
foreground.

>
> - As it turns out, the patch increases code size even for boards that
> ?do not activate this feature.

Yes, I will take a look at this problem.

>
>
> I have to admit that I'm at a loss with a decision here.

Well it's not easy being a maintainer :-) In any case this patch is
not the end of the story as serial needs some work - another objection
you didn't mention above is that this function lives in only one
driver. Is that a good thing (hide it away) or a bad thing (all
drivers should support it and the implementation should move up a
level)?

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 at denx.de
> "We don't have to protect the environment -- the Second Coming is ?at
> hand." ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? - James Watt
>

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Albert ARIBAUD]

Le 17/10/2011 18:40, Simon Glass a ?crit :

>>   So essentially you are saying: hey, we now have multi-line input,
>>   but it works only a bit.  It will fail sooner or later, without
>>   error messages, and I cannot even tell you what the limits for your
>>   system are.  And it depends on which input you send.
>>
>>   This does not exactly sound promising.
>
> That all sounds reasonable.
>
> I think it is good enough, but not perfect. Due to the fundamental
> design decision you mention at the top, we cannot squirrel away serial
> input in the background. The best we can do is squirrel it away in the
> foreground, as we output new serial data (I suppose we could squirrel
> it away in net loops and other places but I don't want to go there!).
> This turns out to be mostly good enough, because it is rare for people
> to want to paste 'sleep 10' and the like into their terminal (your
> other message).

I think you're not seeing the point that while your patch is good enough 
for 'making multiline input less impractical in some cases', it is not 
good enough for 'making multiline input reliable': it adds marginal 
benefits and we have clear evidence that it will never guarantee a 
correct result however you tweak it, because the only way it could would 
be by multi-tasking in some way; otherwise, there can always be some 
U-Boot activity in between input readings that will be long enough to 
cause character loss.

> I would like to spit out a warning when serial input is lost - as I
> mentioned that could be combined with a serial overhaul at some point.

I don't think the warning would make the functionality better.

>> On the other hand, we also have the rule that things that are useful
>> to some and that don't hurt others should be allowed to go in.
>>
>> What makes me hesitate are two things:
>>
>> - The patch promises a feature (multi-line input), but fails to
>>   provide a reliably working implementation.
>
> I *think* it does the best possible within the fundamental design
> decision constraint. If there is more it can do, please let me have
> your ideas. I don't believe buffering conflicts with the constraint -
> they are separate things. But yes in systems with interrupts normally
> the input buffer is filled in the background and drained in the
> foreground.

There *is* a much better solution within the fundamental single tasking 
design constraint, and it is the one that was invented precisely for 
this: flow control. Granted, hardware flow control may be impractical, 
and that's why software flow control was conceived as early as 1963 and 
is still widely used today and supported by any serial software or 
driver worth its salt.

>> - As it turns out, the patch increases code size even for boards that
>>   do not activate this feature.
>
> Yes, I will take a look at this problem.
>
>>
>>
>> I have to admit that I'm at a loss with a decision here.
>
> Well it's not easy being a maintainer :-)

Well, FWIW, were I the serial code maintainer, I'd NAK this and ask for 
a XON/XOFF implementation.

> In any case this patch is
> not the end of the story as serial needs some work - another objection
> you didn't mention above is that this function lives in only one
> driver. Is that a good thing (hide it away) or a bad thing (all
> drivers should support it and the implementation should move up a
> level)?

Software flow control, whatever way it is implemented, is pure SW and 
hardware independent, so it should be a generic thing. If XON/XOFF input 
flow control gets implemented (and I believe it should), then it should 
be done above hardware serial drivers.

> Regards,
> Simon

Amicalement,
-- 
Albert.

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Graeme Russ]

Hi Simon, Albert,

On 22/10/11 19:44, Albert ARIBAUD wrote:
> Le 17/10/2011 18:40, Simon Glass a ?crit :

[snip]

>>> On the other hand, we also have the rule that things that are useful
>>> to some and that don't hurt others should be allowed to go in.
>>>
>>> What makes me hesitate are two things:
>>>
>>> - The patch promises a feature (multi-line input), but fails to
>>>   provide a reliably working implementation.

I fully agree with this - We should strive to eliminate bugs, not cover
them with more dirt :)

>> I *think* it does the best possible within the fundamental design
>> decision constraint. If there is more it can do, please let me have
>> your ideas. I don't believe buffering conflicts with the constraint -
>> they are separate things. But yes in systems with interrupts normally
>> the input buffer is filled in the background and drained in the
>> foreground.
> 
> There *is* a much better solution within the fundamental single tasking 
> design constraint, and it is the one that was invented precisely for 
> this: flow control. Granted, hardware flow control may be impractical, 
> and that's why software flow control was conceived as early as 1963 and 
> is still widely used today and supported by any serial software or 
> driver worth its salt.

Agree - flow control is the way to go

> Well, FWIW, were I the serial code maintainer, I'd NAK this and ask for 
> a XON/XOFF implementation.
> 
>> In any case this patch is
>> not the end of the story as serial needs some work - another objection
>> you didn't mention above is that this function lives in only one
>> driver. Is that a good thing (hide it away) or a bad thing (all
>> drivers should support it and the implementation should move up a
>> level)?
> 
> Software flow control, whatever way it is implemented, is pure SW and 
> hardware independent, so it should be a generic thing. If XON/XOFF input 
> flow control gets implemented (and I believe it should), then it should 
> be done above hardware serial drivers.

One problem I see with XON/XOFF is that if we don't send XOFF at the right
time, we run the risk of entering a busy loop (any reasonable timeout delay
for example) and loosing input. So in theory, we would need to send XOFF
after every getc() and XON before each getc() and then need to delay a bit
to wait for the character (unless tstc returns true in which case we can
just grab the char in the buffer) which seems a bit excessive.

Maybe we need disable/enable flow control functions for when we know we
will be entering a busy loop the consumes serial input (ymodem and kermit
transfers and readline for example)

Regards,

Graeme

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Wolfgang Denk]

Dear Graeme Russ,

In message <4EA34086.4030101@gmail.com> you wrote:
> 
> One problem I see with XON/XOFF is that if we don't send XOFF at the right
> time, we run the risk of entering a busy loop (any reasonable timeout delay
> for example) and loosing input. So in theory, we would need to send XOFF
> after every getc() ...

That's not true.  I am not aware of any significant delays that take
place while receiving characters that belong to a single line.  If we
had any of these, we would lose characters all the time - but we
don't.

It should be sufficient to send XOFF after receiving a newline
character.

> Maybe we need disable/enable flow control functions for when we know we
> will be entering a busy loop the consumes serial input (ymodem and kermit
> transfers and readline for example)

This should not be necessary. Actually the implementation should not
need to know about such special cases.

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
Compassion -- that's the one things no machine ever had.  Maybe it's
the one thing that keeps men ahead of them.
	-- McCoy, "The Ultimate Computer", stardate 4731.3

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Graeme Russ]

Hi Wolgang,

On Oct 23, 2011 7:20 PM, "Wolfgang Denk" <wd@denx.de> wrote:
>
> Dear Graeme Russ,
>
> In message <4EA34086.4030101@gmail.com> you wrote:
> >
> > One problem I see with XON/XOFF is that if we don't send XOFF at the
right
> > time, we run the risk of entering a busy loop (any reasonable timeout
delay
> > for example) and loosing input. So in theory, we would need to send XOFF
> > after every getc() ...
>
> That's not true.  I am not aware of any significant delays that take
> place while receiving characters that belong to a single line.  If we
> had any of these, we would lose characters all the time - but we
> don't.
>
> It should be sufficient to send XOFF after receiving a newline
> character.

And, ergo, we send an XON when entering the readline function

Hmm, should we move readline() into console.c

> > Maybe we need disable/enable flow control functions for when we know we
> > will be entering a busy loop the consumes serial input (ymodem and
kermit
> > transfers and readline for example)
>
> This should not be necessary. Actually the implementation should not
> need to know about such special cases.

So how does kermit/ymodem send the XON after the user has entered the
receive command and we have sent the XOFF after the newline?

Regards,

Graeme

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Wolfgang Denk]

Dear Graeme Russ,

In message <CALButCLEg6c30En3N4LjPv1woJjFxwkEkHvQYMoJy8+MgSzqJw@mail.gmail.com> you wrote:
>
> > It should be sufficient to send XOFF after receiving a newline
> > character.
> 
> And, ergo, we send an XON when entering the readline function

This is probably not sufficient, as some commands take direct input.
I think both getc() and tstc() should check the XON/XOFF state and
send a XON if XOFF was sent before.

> Hmm, should we move readline() into console.c

Makes sense.

> > This should not be necessary. Actually the implementation should not
> > need to know about such special cases.
> 
> So how does kermit/ymodem send the XON after the user has entered the
> receive command and we have sent the XOFF after the newline?

Upon the first getc() that follows?

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
Marriage is the sole cause of divorce.

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Graeme Russ]

Hi Wolfgang,

On Oct 24, 2011 4:15 AM, "Wolfgang Denk" <wd@denx.de> wrote:
>
> Dear Graeme Russ,
>
> In message <
CALButCLEg6c30En3N4LjPv1woJjFxwkEkHvQYMoJy8+MgSzqJw@mail.gmail.com> you
wrote:
> >

[snip]

> > > This should not be necessary. Actually the implementation should not
> > > need to know about such special cases.
> >
> > So how does kermit/ymodem send the XON after the user has entered the
> > receive command and we have sent the XOFF after the newline?
>
> Upon the first getc() that follows?

And as there will be no corresponding newline, when do we send XOFF?

Regards,

Graeme

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Wolfgang Denk]

Dear Graeme Russ,

In message <CALButCK0oVLTbYxbTF=y-vXL+3+=09VEJpzUnX1+-0SYJQEQJA@mail.gmail.com> you wrote:
>
> > > So how does kermit/ymodem send the XON after the user has entered the
> > > receive command and we have sent the XOFF after the newline?
> >
> > Upon the first getc() that follows?
> 
> And as there will be no corresponding newline, when do we send XOFF?

Never?  Note that kermit / ymodem / S-record download etc. all don't
have any issues with sendign characters back-to-back at line speed.

Problems happen only with multi-line input, so it is perfectly fine
to handle just that - at the root cause, i. e. when input turns into
multi-line input.

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
Include the success of others in your dreams for your own success.

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

[Graeme Russ]

Hi Wolfgang,

On Monday, October 24, 2011, Wolfgang Denk <wd@denx.de> wrote:
> Dear Graeme Russ,
>
> In message <CALButCK0oVLTbYxbTF=y-vXL+3+=
09VEJpzUnX1+-0SYJQEQJA@mail.gmail.com> you wrote:
>>
>> > > So how does kermit/ymodem send the XON after the user has entered the
>> > > receive command and we have sent the XOFF after the newline?
>> >
>> > Upon the first getc() that follows?
>>
>> And as there will be no corresponding newline, when do we send XOFF?
>
> Never?  Note that kermit / ymodem / S-record download etc. all don't
> have any issues with sendign characters back-to-back at line speed.
>
> Problems happen only with multi-line input, so it is perfectly fine
> to handle just that - at the root cause, i. e. when input turns into
> multi-line input.

Can the U-Boot command line handle multiple commands per line (delimited by
; for example)

If so, could it not be possible that a Kermit/ymodem command followed by a
time consuming command on the same line cause lost input?

Regards,

Graeme

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

[Wolfgang Denk]

Dear Graeme Russ,

In message <CALButCJH8BVzfVh14D83WR2JOV89O9jvJO9VzzB7r_ZgKzZqeg@mail.gmail.com> you wrote:
>
> > Problems happen only with multi-line input, so it is perfectly fine
> > to handle just that - at the root cause, i. e. when input turns into
> > multi-line input.
> 
> Can the U-Boot command line handle multiple commands per line (delimited by
> ; for example)

Yes, it can.

> If so, could it not be possible that a Kermit/ymodem command followed by a
> time consuming command on the same line cause lost input?

I don't think so.  All serial transfers use a protocol - and when the
transfer is complete, it does not matter any more, because no more
data are flowing.

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
History tends to exaggerate.
	-- Col. Green, "The Savage Curtain", stardate 5906.4

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

[Graeme Russ]

Hi Wolfgang,

On Monday, October 24, 2011, Wolfgang Denk <wd@denx.de> wrote:
> Dear Graeme Russ,
>
> In message <
CALButCJH8BVzfVh14D83WR2JOV89O9jvJO9VzzB7r_ZgKzZqeg@mail.gmail.com> you
wrote:
>>
>> > Problems happen only with multi-line input, so it is perfectly fine
>> > to handle just that - at the root cause, i. e. when input turns into
>> > multi-line input.
>>
>> Can the U-Boot command line handle multiple commands per line (delimited
by
>> ; for example)
>
> Yes, it can.
>
>> If so, could it not be possible that a Kermit/ymodem command followed by
a
>> time consuming command on the same line cause lost input?
>
> I don't think so.  All serial transfers use a protocol - and when the
> transfer is complete, it does not matter any more, because no more
> data are flowing.

My point is that the transfer turns off flow control - When the transfer
completes, flow control will be off when the next command begins to run.
If the next command is one which takes a long time to execute and it is on
the same line as the transfer command (i.e. no \r to send XOFF) and the
user types something then that input can be lost.

I think the solution is fairly trivial though - During the processing of
commands entered via readline(), cause an XOFF to be sent each time (i.e.
immediately before) the command string is dispatched a to the command
processor just in case the previous command called getc() (and thus caused
an XON to be sent)

Regards,

Graeme

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

[Simon Glass]

Hi,

On Sun, Oct 23, 2011 at 4:30 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Wolfgang,
>
> On Monday, October 24, 2011, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Graeme Russ,
>>
>> In message <
> CALButCJH8BVzfVh14D83WR2JOV89O9jvJO9VzzB7r_ZgKzZqeg at mail.gmail.com> you
> wrote:
>>>
>>> > Problems happen only with multi-line input, so it is perfectly fine
>>> > to handle just that - at the root cause, i. e. when input turns into
>>> > multi-line input.
>>>
>>> Can the U-Boot command line handle multiple commands per line (delimited
> by
>>> ; for example)
>>
>> Yes, it can.
>>
>>> If so, could it not be possible that a Kermit/ymodem command followed by
> a
>>> time consuming command on the same line cause lost input?
>>
>> I don't think so. ?All serial transfers use a protocol - and when the
>> transfer is complete, it does not matter any more, because no more
>> data are flowing.
>
> My point is that the transfer turns off flow control - When the transfer
> completes, flow control will be off when the next command begins to run.
> If the next command is one which takes a long time to execute and it is on
> the same line as the transfer command (i.e. no \r to send XOFF) and the
> user types something then that input can be lost.
>
> I think the solution is fairly trivial though - During the processing of
> commands entered via readline(), cause an XOFF to be sent each time (i.e.
> immediately before) the command string is dispatched a to the command
> processor just in case the previous command called getc() (and thus caused
> an XON to be sent)

I had a go at a patch for this, will send out tomorrow. It will need
fixing up for xmodem etc. So far it works ok with minicom but not
ser2net. It is pretty simple.

I think Albert mentioned that XON/XOFF was invented in 1962. Maybe it
will come back into fashion? Not at all sure it will...

Regards,
Simon

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

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

[Wolfgang Denk]

Dear Graeme Russ,

In message <CALButCKD2ucJ0ZUQJpLCP2ABYcCzO-mACa=FPWczCTVeHeoTKg@mail.gmail.com> you wrote:
>
> >> If so, could it not be possible that a Kermit/ymodem command followed by a
> >> time consuming command on the same line cause lost input?
> >
> > I don't think so.  All serial transfers use a protocol - and when the
> > transfer is complete, it does not matter any more, because no more
> > data are flowing.
> 
> My point is that the transfer turns off flow control - When the transfer
> completes, flow control will be off when the next command begins to run.

Why would any of the transfer commands actually turn off flow control?
There is no need to do that so far.  And even if they do - that's no
fundamental difference to now, where we are not reading the input
then, either.

> If the next command is one which takes a long time to execute and it is on
> the same line as the transfer command (i.e. no \r to send XOFF) and the
> user types something then that input can be lost.

I don't understand what you mean.  We're talking about a single line
of input here, right?  Re-enabling XON is not needed before we're
ready to read the next line.  And during that, no characters would be
lost because none are sent due to flow control being shut off.

> I think the solution is fairly trivial though - During the processing of
> commands entered via readline(), cause an XOFF to be sent each time (i.e.
> immediately before) the command string is dispatched a to the command
> processor just in case the previous command called getc() (and thus caused
> an XON to be sent)

This sounds like unneeded overhead to me.

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
Teenagers are people who express a burning desire to be different by
dressing exactly alike.
There are some strings. They're just not attached.

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

[Graeme Russ]

Hi Wolfgang,

On 25/10/11 05:46, Wolfgang Denk wrote:
> Dear Graeme Russ,
> 
> In message <CALButCKD2ucJ0ZUQJpLCP2ABYcCzO-mACa=FPWczCTVeHeoTKg@mail.gmail.com> you wrote:
>>
>>>> If so, could it not be possible that a Kermit/ymodem command followed by a
>>>> time consuming command on the same line cause lost input?
>>>
>>> I don't think so.  All serial transfers use a protocol - and when the
>>> transfer is complete, it does not matter any more, because no more
>>> data are flowing.
>>
>> My point is that the transfer turns off flow control - When the transfer
>> completes, flow control will be off when the next command begins to run.
> 
> Why would any of the transfer commands actually turn off flow control?

getc() sends an XOFF

> There is no need to do that so far.  And even if they do - that's no
> fundamental difference to now, where we are not reading the input
> then, either.
> 
>> If the next command is one which takes a long time to execute and it is on
>> the same line as the transfer command (i.e. no \r to send XOFF) and the
>> user types something then that input can be lost.
> 
> I don't understand what you mean.  We're talking about a single line
> of input here, right?  Re-enabling XON is not needed before we're
> ready to read the next line.  And during that, no characters would be
> lost because none are sent due to flow control being shut off.
> 
>> I think the solution is fairly trivial though - During the processing of
>> commands entered via readline(), cause an XOFF to be sent each time (i.e.
>> immediately before) the command string is dispatched a to the command
>> processor just in case the previous command called getc() (and thus caused
>> an XON to be sent)
> 
> This sounds like unneeded overhead to me.

consider the follow (admittedly canned) example:

loadb ; sleep 20

An XOFF will be sent when the user hits 'enter' but loadb will send an XON
when it calls getc(). Now after the transfer is complete, there will have
been no XOFF before the sleep command is run so if the user enters anything
during the sleep command, those characters can be lost

Regards,

Graeme

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

[Wolfgang Denk]

Dear Graeme Russ,

In message <4EA5BBEF.1050706@gmail.com> you wrote:
> 
> > Why would any of the transfer commands actually turn off flow control?
> 
> getc() sends an XOFF

Oops?  You got something wrong, I think.

Receiving a '\n' would cause sending XOFF, and getc() would, if state
is XOFF, send an XON.

> consider the follow (admittedly canned) example:
> 
> loadb ; sleep 20
> 
> An XOFF will be sent when the user hits 'enter' but loadb will send an XON
> when it calls getc(). Now after the transfer is complete, there will have
> been no XOFF before the sleep command is run so if the user enters anything
> during the sleep command, those characters can be lost

Agreed.  You can relax situation if functions that use the serial line
for their own purposes (like running some serial transfer protocol)
would have some initialization part where they remember the line state
(and, if it was XOFF, send XON), plus a termination part where they
restore the old line state (i. e. if it was XOFF initially, send XOFF
again).


But note that such software flow control is again just a little
improvement, it is definitely not a guaranteed way to prevent any
character losses.  Assume the situation where the user copy & pastes
some multi-line command sequence. We will receive a character sequence
like this:

	a b c d e '\n' f g h i j k ...

We will send XOFF after receiving the '\n' - guess when XOFF will be
loaded into our transmitter? When the last bit and stop bits have been
sent? When the receiver on the other size will actually read this from
his FIFO? And How long it then will take until his Tx fifo empties,
and he stops sending more characters?

Assume we have a simple device with a small Rx FIFO - say, 8 bytes
only. Guess what the chances are that we will overrun this FIFO (and
then lose characters) before the other side stops sending?

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
As far as the laws of mathematics refer  to  reality,  they  are  not
certain;  and  as  far  as  they  are  certain,  they do not refer to
reality.                                           -- Albert Einstein

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

[Graeme Russ]

Hi Wolfgang,

On 25/10/11 07:00, Wolfgang Denk wrote:
> Dear Graeme Russ,
> 
> In message <4EA5BBEF.1050706@gmail.com> you wrote:
>>
>>> Why would any of the transfer commands actually turn off flow control?
>>
>> getc() sends an XOFF
> 
> Oops?  You got something wrong, I think.
> 
> Receiving a '\n' would cause sending XOFF, and getc() would, if state
> is XOFF, send an XON.

Oops, yes you are right

>> consider the follow (admittedly canned) example:
>>
>> loadb ; sleep 20
>>
>> An XOFF will be sent when the user hits 'enter' but loadb will send an XON
>> when it calls getc(). Now after the transfer is complete, there will have
>> been no XOFF before the sleep command is run so if the user enters anything
>> during the sleep command, those characters can be lost
> 
> Agreed.  You can relax situation if functions that use the serial line
> for their own purposes (like running some serial transfer protocol)
> would have some initialization part where they remember the line state
> (and, if it was XOFF, send XON), plus a termination part where they
> restore the old line state (i. e. if it was XOFF initially, send XOFF
> again).

I think it is sufficient to send an XOFF just before starting to process a
command and XON after command processing is complete. I would think it safe
to assume any command that consumes characters either does so fast enough,
or does flow control internally. And while not processing a command, we are
in a busy loop processing terminal input waiting for a command.

> But note that such software flow control is again just a little
> improvement, it is definitely not a guaranteed way to prevent any
> character losses.  Assume the situation where the user copy & pastes

Actually, I think we can guarantee no loss provided both ends of the serial
link are configured correctly.

> some multi-line command sequence. We will receive a character sequence
> like this:
> 
> 	a b c d e '\n' f g h i j k ...
> 
> We will send XOFF after receiving the '\n' - guess when XOFF will be
> loaded into our transmitter? When the last bit and stop bits have been
> sent? When the receiver on the other size will actually read this from
> his FIFO? And How long it then will take until his Tx fifo empties,
> and he stops sending more characters?

Provided the transmitter and receiver code are running in busy loops, at
most a few characters. This is why 16550 et. al. have hardware Rx buffers,
to account for the delay in receiving the XOFF. The hardware Tx should
never be used unless you have hardware flow control which is controlled
directly by the UART (and I think the original implementation of the 16550
only had Rx buffers for this very reason).

> Assume we have a simple device with a small Rx FIFO - say, 8 bytes
> only. Guess what the chances are that we will overrun this FIFO (and
> then lose characters) before the other side stops sending?

Should be zero (if you disable the hardware Tx buffer) unless you have an
incredibly slow Rx routine (in which case, your baud rate is too high). And
even with a Tx buffer in play the solution is fairly simple - After sending
an XOFF, flush the Rx buffer (and remote Tx buffer) into an internal buffer
until tstc() returns false - use this buffer for getc() until it is empty.
Of course, this fails if the remote end does not honour XOFF, but that's
the users own fault for turning software flow control on at one end only
and/or using a broken serial emulator.

Regards,

Graeme

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

[Wolfgang Denk]

Dear Graeme Russ,

In message <4EA5CD39.2070203@gmail.com> you wrote:
> 
> > Assume we have a simple device with a small Rx FIFO - say, 8 bytes
> > only. Guess what the chances are that we will overrun this FIFO (and
> > then lose characters) before the other side stops sending?
> 
> Should be zero (if you disable the hardware Tx buffer) unless you have an
> incredibly slow Rx routine (in which case, your baud rate is too high). And
> even with a Tx buffer in play the solution is fairly simple - After sending
> an XOFF, flush the Rx buffer (and remote Tx buffer) into an internal buffer
> until tstc() returns false - use this buffer for getc() until it is empty.

Things are becoming more and more complicated, it seems.

If you ask me: forget about all this, and stick with single line
input.  Do the processing on the sender's side (always wait for a
prompt before sending the next line).  This is MUCH easier.


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
"Never face facts; if you do, you'll never get up in the morning."
- Marlo Thomas

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

[Graeme Russ]

Hi Wolfgang,

On Tue, Oct 25, 2011 at 8:59 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Graeme Russ,
>
> In message <4EA5CD39.2070203@gmail.com> you wrote:
>>
>> > Assume we have a simple device with a small Rx FIFO - say, 8 bytes
>> > only. Guess what the chances are that we will overrun this FIFO (and
>> > then lose characters) before the other side stops sending?
>>
>> Should be zero (if you disable the hardware Tx buffer) unless you have an
>> incredibly slow Rx routine (in which case, your baud rate is too high). And
>> even with a Tx buffer in play the solution is fairly simple - After sending
>> an XOFF, flush the Rx buffer (and remote Tx buffer) into an internal buffer
>> until tstc() returns false - use this buffer for getc() until it is empty.
>
> Things are becoming more and more complicated, it seems.
>
> If you ask me: forget about all this, and stick with single line
> input.  Do the processing on the sender's side (always wait for a
> prompt before sending the next line).  This is MUCH easier.

Oh, I wholehearedly agree that if we impose such a limitation, life will
be much easier for us all ;)

But realistically, the solution is actually very trivial and comprises
just two parts:

1a) When the command line interpreter (which is just the really simple
    function detecting new-line or command delimiter) dispatches a
    command string to the command interpreter (which looks up the
    command table and if it finds a command runs it) send an XOFF
1b) When the command interpreter returns control back to the command
    line interpreter, send an XON
1c) If an XOFF has been recently sent (i.e. no XON since) send an XON
    in getc()

2) After sending XOFF, flush the transmitter's buffer into a small
   (16 bytes chould sufffice) buffer. Grab characters from this buffer
   before grabing from the UART when getc() is called

The second part should not even be necessary if you have a half-sane
serial port with a half-decent hardware buffer.

It really would not be that hard (maybe a few dozen lines of code at most),
would work for all UARTS and should (hopefully) resolve all 'dropped
character' issues.

Now 1c) makes the bold assumption that any command which calls getc()
'knows what it is doing' and will consume all input characters at a fast
enough rate AND will not invoke a delay between receiving the last
character and returning back to the command line interpreter. If the
command knows it received characters and knows it will invoke a delay (some
kind of post-processing) then in order to not lose characters, the command
MUST issue it's own XOFF - nothing else aside from an interrupt driven
serial driver will solve the problem in this case.

Regards,

Graeme

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

[J. William Campbell]

On 10/24/2011 3:22 PM, Graeme Russ wrote:
> Hi Wolfgang,
>
> On Tue, Oct 25, 2011 at 8:59 AM, Wolfgang Denk<wd@denx.de>  wrote:
>> Dear Graeme Russ,
>>
>> In message<4EA5CD39.2070203@gmail.com>  you wrote:
>>>> Assume we have a simple device with a small Rx FIFO - say, 8 bytes
>>>> only. Guess what the chances are that we will overrun this FIFO (and
>>>> then lose characters) before the other side stops sending?
>>> Should be zero (if you disable the hardware Tx buffer) unless you have an
>>> incredibly slow Rx routine (in which case, your baud rate is too high). And
>>> even with a Tx buffer in play the solution is fairly simple - After sending
>>> an XOFF, flush the Rx buffer (and remote Tx buffer) into an internal buffer
>>> until tstc() returns false - use this buffer for getc() until it is empty.
>> Things are becoming more and more complicated, it seems.
>>
>> If you ask me: forget about all this, and stick with single line
>> input.  Do the processing on the sender's side (always wait for a
>> prompt before sending the next line).  This is MUCH easier.
> Oh, I wholehearedly agree that if we impose such a limitation, life will
> be much easier for us all ;)
>
> But realistically, the solution is actually very trivial and comprises
> just two parts:
>
> 1a) When the command line interpreter (which is just the really simple
>      function detecting new-line or command delimiter) dispatches a
>      command string to the command interpreter (which looks up the
>      command table and if it finds a command runs it) send an XOFF
> 1b) When the command interpreter returns control back to the command
>      line interpreter, send an XON
> 1c) If an XOFF has been recently sent (i.e. no XON since) send an XON
>      in getc()
>
> 2) After sending XOFF, flush the transmitter's buffer into a small
>     (16 bytes chould sufffice) buffer. Grab characters from this buffer
>     before grabing from the UART when getc() is called
Hi All,
       The problem with all this is that the transmitting CPU may send 
"several" characters after the XOFF is transmitted, where several will 
be at least two if the transmitting CPU has no output fifo, up to 
possibly the output fifo length + 2. Furthermore, this "overage" can 
theoretically increase with each x-on x-off sequence, as each command 
may be, in theory,  shorter than the output fifo length. So the input 
buffering is needed in all cases, i.e. the receive fifo must be "run 
dry" before executing a command. If the receive fifo is shorter than the 
transmit fifo +2, you are doomed. If the receive fifo is larger than 
this, you are ok, the transmitting CPU will stop before overrunning the 
input fifo and data being lost. However, a 16 byte buffer will not be 
enough if there are lots of "short" commands in the input stream.

Best Regards,
Bill Campbell
>
> The second part should not even be necessary if you have a half-sane
> serial port with a half-decent hardware buffer.
>
> It really would not be that hard (maybe a few dozen lines of code at most),
> would work for all UARTS and should (hopefully) resolve all 'dropped
> character' issues.
>
> Now 1c) makes the bold assumption that any command which calls getc()
> 'knows what it is doing' and will consume all input characters at a fast
> enough rate AND will not invoke a delay between receiving the last
> character and returning back to the command line interpreter. If the
> command knows it received characters and knows it will invoke a delay (some
> kind of post-processing) then in order to not lose characters, the command
> MUST issue it's own XOFF - nothing else aside from an interrupt driven
> serial driver will solve the problem in this case.
>
> Regards,
>
> Graeme
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
>

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

[Wolfgang Denk]

Dear Graeme Russ,

In message <CALButCLNEjjEB=1zvvikrVo5-X9vyYOECU1-ZrpXf2myavM+Pg@mail.gmail.com> you wrote:
> 
> Now 1c) makes the bold assumption that any command which calls getc()
> 'knows what it is doing' and will consume all input characters at a fast
> enough rate AND will not invoke a delay between receiving the last
> character and returning back to the command line interpreter. If the
> command knows it received characters and knows it will invoke a delay (some
> kind of post-processing) then in order to not lose characters, the command
> MUST issue it's own XOFF - nothing else aside from an interrupt driven
> serial driver will solve the problem in this case.

This is a fundamentally broken design.  No command should ever need to
have any such knowledge, not to mention that we must not littering
potentially all code with serial flow control calls.

You will have really hard times to get anything like that into mainline.

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 optimum committee has no members.
                                                   - Norman Augustine

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

[Graeme Russ]

Hi Wolfgang,

On 25/10/11 18:31, Wolfgang Denk wrote:
> Dear Graeme Russ,
> 
> In message <CALButCLNEjjEB=1zvvikrVo5-X9vyYOECU1-ZrpXf2myavM+Pg@mail.gmail.com> you wrote:
>>
>> Now 1c) makes the bold assumption that any command which calls getc()
>> 'knows what it is doing' and will consume all input characters at a fast
>> enough rate AND will not invoke a delay between receiving the last
>> character and returning back to the command line interpreter. If the
>> command knows it received characters and knows it will invoke a delay (some
>> kind of post-processing) then in order to not lose characters, the command
>> MUST issue it's own XOFF - nothing else aside from an interrupt driven
>> serial driver will solve the problem in this case.
> 
> This is a fundamentally broken design.  No command should ever need to
> have any such knowledge, not to mention that we must not littering
> potentially all code with serial flow control calls.

Well, if a command reads from the console (a transfer command for example)
and then has a long delay (busy processing loop) before returning back to
the command line processor then that command must be fundamentally broken -
there are five ways I see to fix it:

 a) Fix the command so it isn't broken
 b) Have the command tell the console it has finished with the console
    before it starts the busy loop
 c) Use UART managed hardware flow control
 d) Implement interrupt based serial drivers
 e) Prohibit multi-line input

> You will have really hard times to get anything like that into mainline.

So let's assume for the meantime that there are no 'broken' commands we can
simply issue an XOFF before running each command - That should eliminate
most dropped character issues - This is Simon's latest 2-patch series.

Adding rx_flush() and a small local buffer will fix dropped characters due
to insufficiently small local UART Rx buffers (and brain-dead remote Tx
buffers) - That's a separate small patch

So any remaining dropped character issues are reduced to 'broken' commands
that use console I/O and then enter a busy loop which does not process
console I/O - Now we're into hack territory

Regards,

Graeme

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

[Wolfgang Denk]

Dear Graeme Russ,

In message <4EA67491.5090802@gmail.com> you wrote:
> 
> Well, if a command reads from the console (a transfer command for example)
> and then has a long delay (busy processing loop) before returning back to
> the command line processor then that command must be fundamentally broken -

???? Could you please explain what makes you think so?

What the command does, and when it performs I/O or when it spens time
for other tasks is only up to the command.

If anything is broken then it is a design that puts restrictions on
such behaviour.

>  a) Fix the command so it isn't broken
>  b) Have the command tell the console it has finished with the console
>     before it starts the busy loop
>  c) Use UART managed hardware flow control
>  d) Implement interrupt based serial drivers
>  e) Prohibit multi-line input

Prohibit is a bit of a strong word here.  "Not support" is more like
it.

> So let's assume for the meantime that there are no 'broken' commands we can

This is not an assumption.  No implementation must put any
restrictions on the timing behaviour of any commands.

> simply issue an XOFF before running each command - That should eliminate

That's brainded overhead. In 99.999% of all cases it's not needed.
And it happens at completely the wrong place.

Serial flow control is something we should deal with in the serial
driver code.  Nowhere else.

I will not accept such a design nor such an implementation.


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
"Have you lived in this village all your life?"        "No, not yet."

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

[Graeme Russ]

Hi Wolfgang,

On Wed, Oct 26, 2011 at 5:41 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Graeme Russ,
>
> In message <4EA67491.5090802@gmail.com> you wrote:
>>
>> Well, if a command reads from the console (a transfer command for example)
>> and then has a long delay (busy processing loop) before returning back to
>> the command line processor then that command must be fundamentally broken -
>
> ???? Could you please explain what makes you think so?
>
> What the command does, and when it performs I/O or when it spens time
> for other tasks is only up to the command.
>
> If anything is broken then it is a design that puts restrictions on
> such behaviour.
>
>>  a) Fix the command so it isn't broken
>>  b) Have the command tell the console it has finished with the console
>>     before it starts the busy loop
>>  c) Use UART managed hardware flow control
>>  d) Implement interrupt based serial drivers
>>  e) Prohibit multi-line input
>
> Prohibit is a bit of a strong word here.  "Not support" is more like
> it.
>
>> So let's assume for the meantime that there are no 'broken' commands we can
>
> This is not an assumption.  No implementation must put any
> restrictions on the timing behaviour of any commands.
>
>> simply issue an XOFF before running each command - That should eliminate
>
> That's brainded overhead. In 99.999% of all cases it's not needed.
> And it happens at completely the wrong place.
>
> Serial flow control is something we should deal with in the serial
> driver code.  Nowhere else.

This problem comes down to managing two asynchronous tasks - Command
Processing and Serial Input Processing. Any solution to the problem
is going to involve task switching. The way this is done in U-Boot
currently is:

 - When in the main() loop, U-Boot is effectively running a Command
   Processing Taks (monitor inbound characters, determine when a
   command has been entered) which continually switches over to Serial
   Input Processing Task. When a valid command is detected, U-Boot
   switches to the corresponding Command Processing Task
 - Some 'commands' (file transfers in particular) have their own Serial
   Input Processing Sub-Tasks - The Command Processing Task may run for a
   'significant' amount of time after these Sub-Tasks are finished with
   and no longer processing serial input

When you are dealing with asynchronous tasks, you can task switch by
either:

 1) Using a hardware interrupt (either a tick-timer or interrupt line from
    the serial UART)
 2) Littering the main taks with co-operative interrupt calls
 3) Task switch at clearly defined locations in the code

Option 1 is not supported in U-Boot
Option 2 is used by U-Boot for triggering the watchdog - It's not pretty,
but in the absence of #1, we have no other option
Option 3 is all we are left with...

And U-Boot does actually do #3, but the 'clearly defined locations' are
not obvious, and the switch is done _without_ telling the remote serial
transmitter that U-Boot is busy and will not be able to process any more
inbound characters for the time being.

Note that Serial Input Processing Task does not need to run after we have
sent an XOFF and flushed the remote Tx buffer and local Rx buffer as the
remote end _should_ have stopped sending characters. So in order to
absolutely prevent dropped characters, it is a simple matter of sending an
XOFF and flushing the buffers when we switch out of the Serial Input
Processing Task and into the Command Processing Task. Sounds easy enough,
BUT, the Serial Input Processing Task is a dumb task (it simply reads
single characters from the UART) and has no idea what triggers the switch
to the Command Processing Task so there is no way for it to know when to
sent the XOFF - The Command Processing Task needs to tell the Serial
Input Processing Task

I suggested a solution whereby any task which _knows_ it switches to the
Serial Input Processing Taks (i.e. calls getc()) can:
 - Tell the Serial Input Processing Task 'I will need serial input from
   the remote end, can you please arrange it so that I recieve those
   characters'
 - Process the incoming stream (it is the responsibility of the task to
   make sure the stream is processed without dropping characters, or
   use a protocol which allows for resending of dropped charaters)
 - Tell the Serial Input Processing Task 'OK, I've recieved all the data
   I need to and I will not be processing any more - You have been warned,
   so if you want don't want to loose input, you'll need to do something
   about it'

I'm at a loss to think of any other solution - Can you?

> I will not accept such a design nor such an implementation.

Then we live with dropped characters

Regards,

Graeme

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

[Simon Glass]

Hi,

On Tue, Oct 25, 2011 at 3:37 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Wolfgang,
>
> On Wed, Oct 26, 2011 at 5:41 AM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Graeme Russ,
>>
>> In message <4EA67491.5090802@gmail.com> you wrote:
>>>
>>> Well, if a command reads from the console (a transfer command for example)
>>> and then has a long delay (busy processing loop) before returning back to
>>> the command line processor then that command must be fundamentally broken -
>>
>> ???? Could you please explain what makes you think so?
>>
>> What the command does, and when it performs I/O or when it spens time
>> for other tasks is only up to the command.
>>
>> If anything is broken then it is a design that puts restrictions on
>> such behaviour.
>>
>>> ?a) Fix the command so it isn't broken
>>> ?b) Have the command tell the console it has finished with the console
>>> ? ? before it starts the busy loop
>>> ?c) Use UART managed hardware flow control
>>> ?d) Implement interrupt based serial drivers
>>> ?e) Prohibit multi-line input
>>
>> Prohibit is a bit of a strong word here. ?"Not support" is more like
>> it.
>>
>>> So let's assume for the meantime that there are no 'broken' commands we can
>>
>> This is not an assumption. ?No implementation must put any
>> restrictions on the timing behaviour of any commands.
>>
>>> simply issue an XOFF before running each command - That should eliminate
>>
>> That's brainded overhead. In 99.999% of all cases it's not needed.
>> And it happens at completely the wrong place.
>>
>> Serial flow control is something we should deal with in the serial
>> driver code. ?Nowhere else.
>
> This problem comes down to managing two asynchronous tasks - Command
> Processing and Serial Input Processing. Any solution to the problem
> is going to involve task switching. The way this is done in U-Boot
> currently is:
>
> ?- When in the main() loop, U-Boot is effectively running a Command
> ? Processing Taks (monitor inbound characters, determine when a
> ? command has been entered) which continually switches over to Serial
> ? Input Processing Task. When a valid command is detected, U-Boot
> ? switches to the corresponding Command Processing Task
> ?- Some 'commands' (file transfers in particular) have their own Serial
> ? Input Processing Sub-Tasks - The Command Processing Task may run for a
> ? 'significant' amount of time after these Sub-Tasks are finished with
> ? and no longer processing serial input
>
> When you are dealing with asynchronous tasks, you can task switch by
> either:
>
> ?1) Using a hardware interrupt (either a tick-timer or interrupt line from
> ? ?the serial UART)
> ?2) Littering the main taks with co-operative interrupt calls
> ?3) Task switch at clearly defined locations in the code
>
> Option 1 is not supported in U-Boot
> Option 2 is used by U-Boot for triggering the watchdog - It's not pretty,
> but in the absence of #1, we have no other option
> Option 3 is all we are left with...
>
> And U-Boot does actually do #3, but the 'clearly defined locations' are
> not obvious, and the switch is done _without_ telling the remote serial
> transmitter that U-Boot is busy and will not be able to process any more
> inbound characters for the time being.
>
> Note that Serial Input Processing Task does not need to run after we have
> sent an XOFF and flushed the remote Tx buffer and local Rx buffer as the
> remote end _should_ have stopped sending characters. So in order to
> absolutely prevent dropped characters, it is a simple matter of sending an
> XOFF and flushing the buffers when we switch out of the Serial Input
> Processing Task and into the Command Processing Task. Sounds easy enough,
> BUT, the Serial Input Processing Task is a dumb task (it simply reads
> single characters from the UART) and has no idea what triggers the switch
> to the Command Processing Task so there is no way for it to know when to
> sent the XOFF - The Command Processing Task needs to tell the Serial
> Input Processing Task
>
> I suggested a solution whereby any task which _knows_ it switches to the
> Serial Input Processing Taks (i.e. calls getc()) can:
> ?- Tell the Serial Input Processing Task 'I will need serial input from
> ? the remote end, can you please arrange it so that I recieve those
> ? characters'
> ?- Process the incoming stream (it is the responsibility of the task to
> ? make sure the stream is processed without dropping characters, or
> ? use a protocol which allows for resending of dropped charaters)
> ?- Tell the Serial Input Processing Task 'OK, I've recieved all the data
> ? I need to and I will not be processing any more - You have been warned,
> ? so if you want don't want to loose input, you'll need to do something
> ? about it'
>
> I'm at a loss to think of any other solution - Can you?
>
>> I will not accept such a design nor such an implementation.
>
> Then we live with dropped characters
>
> Regards,
>
> Graeme
>

Did I mention a can of worms? After 65 messages on this topic Scott's
patch seems pretty appealing right now! We can even move it up a level
in the s/w stack if that helps.

But to continue this a little, and donning my asbestos suit, should
U-Boot have a CONFIG to enable an event loop called in delay
functions, network functions, read/write operations, etc.? It would
permit us to solve this problem properly I think, if we think it is
worth solving. Not saying it is a good idea...

Regards,
Simon

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

[Graeme Russ]

Hi Simon,

On Wed, Oct 26, 2011 at 10:17 AM, Simon Glass <sjg@chromium.org> wrote:

[big snip]

> Did I mention a can of worms? After 65 messages on this topic Scott's
> patch seems pretty appealing right now! We can even move it up a level
> in the s/w stack if that helps.

But I agree with Wolfgang that Scott's proposal only hides the real
problem even deeper and will make a real solution more difficult
next time around

> But to continue this a little, and donning my asbestos suit, should
> U-Boot have a CONFIG to enable an event loop called in delay
> functions, network functions, read/write operations, etc.? It would
> permit us to solve this problem properly I think, if we think it is
> worth solving. Not saying it is a good idea...

That is what the watchdog code does - It is a horrible hack, and every now
and again, another patch comes through which adds another call to the
watchdog trigger in some obscure loop in some obscure code location. Or
someone submits new code for a board that does not have a watchdog that
somebody else then uses on a board that does have one and the watchdog
suddenly triggers resetting the board. It's messy and ugly.

My biggest point about the 'dropped character' issue is that the fix is
simple provided the 'tasks' that use the serial console behave and inform
the serial console driver that they have finished receiving characters -
not a big ask in my opinion. Way smaller that spattering 'Check the
serial port' all over the code

Regards,

Graeme

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

[Simon Glass]

Hi Graeme,

On Tue, Oct 25, 2011 at 4:37 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Simon,
>
> On Wed, Oct 26, 2011 at 10:17 AM, Simon Glass <sjg@chromium.org> wrote:
>
> [big snip]
>
>> Did I mention a can of worms? After 65 messages on this topic Scott's
>> patch seems pretty appealing right now! We can even move it up a level
>> in the s/w stack if that helps.
>
> But I agree with Wolfgang that Scott's proposal only hides the real
> problem even deeper and will make a real solution more difficult
> next time around

I feel that the 'real' solution is flow control or buffering and probably both.

>
>> But to continue this a little, and donning my asbestos suit, should
>> U-Boot have a CONFIG to enable an event loop called in delay
>> functions, network functions, read/write operations, etc.? It would
>> permit us to solve this problem properly I think, if we think it is
>> worth solving. Not saying it is a good idea...
>
> That is what the watchdog code does - It is a horrible hack, and every now
> and again, another patch comes through which adds another call to the
> watchdog trigger in some obscure loop in some obscure code location. Or
> someone submits new code for a board that does not have a watchdog that
> somebody else then uses on a board that does have one and the watchdog
> suddenly triggers resetting the board. It's messy and ugly.
>
> My biggest point about the 'dropped character' issue is that the fix is
> simple provided the 'tasks' that use the serial console behave and inform
> the serial console driver that they have finished receiving characters -
> not a big ask in my opinion. Way smaller that spattering 'Check the
> serial port' all over the code

I kind-of agree so long as it is more of an event loop and less of
something specific to serial (of course serial might be the only
customer but then we could display a spinning thing on the LCD...).
Funnily enough that problem (one bit of code wanting to tell serial
something) exists on Seaboard, where the SPI driver has to tell the
UART to flush or invalidate since they cannot co-exist. And that is
why I was so interested in this patch.

Regards,
Simon

>
> Regards,
>
> Graeme
>

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

[Graeme Russ]

Hi Simon,

On Wed, Oct 26, 2011 at 10:48 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Graeme,
>
> On Tue, Oct 25, 2011 at 4:37 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>> Hi Simon,
>>
>> On Wed, Oct 26, 2011 at 10:17 AM, Simon Glass <sjg@chromium.org> wrote:
>>
>> [big snip]
>>
>>> Did I mention a can of worms? After 65 messages on this topic Scott's
>>> patch seems pretty appealing right now! We can even move it up a level
>>> in the s/w stack if that helps.
>>
>> But I agree with Wolfgang that Scott's proposal only hides the real
>> problem even deeper and will make a real solution more difficult
>> next time around
>
> I feel that the 'real' solution is flow control or buffering and probably both.
>
>>
>>> But to continue this a little, and donning my asbestos suit, should
>>> U-Boot have a CONFIG to enable an event loop called in delay
>>> functions, network functions, read/write operations, etc.? It would
>>> permit us to solve this problem properly I think, if we think it is
>>> worth solving. Not saying it is a good idea...
>>
>> That is what the watchdog code does - It is a horrible hack, and every now
>> and again, another patch comes through which adds another call to the
>> watchdog trigger in some obscure loop in some obscure code location. Or
>> someone submits new code for a board that does not have a watchdog that
>> somebody else then uses on a board that does have one and the watchdog
>> suddenly triggers resetting the board. It's messy and ugly.
>>
>> My biggest point about the 'dropped character' issue is that the fix is
>> simple provided the 'tasks' that use the serial console behave and inform
>> the serial console driver that they have finished receiving characters -
>> not a big ask in my opinion. Way smaller that spattering 'Check the
>> serial port' all over the code
>
> I kind-of agree so long as it is more of an event loop and less of
> something specific to serial (of course serial might be the only
> customer but then we could display a spinning thing on the LCD...)

I don't think we need to get too fancy - U-Boot is not an OS :)

> Funnily enough that problem (one bit of code wanting to tell serial
> something) exists on Seaboard, where the SPI driver has to tell the
> UART to flush or invalidate since they cannot co-exist. And that is
> why I was so interested in this patch.

Well, I have the feeling than an console API might be in order. The way
U-Boot is structured at the moment, serial I/O is kind of taken for
granted to 'just exist' and nobody needs to really be self-aware that
they use it - The same cannot be said of users of USB and Network -
Net is a good example - all 'users' of net call NetLoop() which does
an eth_init()...do stuff...eth_halt()

So maybe a serial API which 'users' must 'wake up' and 'sleep'...

 - console_configure() - Set parameters (port, baud rate etc)
 - console_wake() - Prepare the console (could be serial, USB, netconsole)
 - console_flush() - Throw away any buffered characters
 - console_getc(), console_putc(), console_tstc() - Self explainatory
 - console_sleep() - Shut the console down (send XOFF, turn of USB etc)

This would resolve the issue were devices are multiplexed with the serial
UART like your SPI example. The command line interpreter calls
console_sleep() to release the serial UART resource before the command
which uses the multiplexed device wakes that device, does it's thing and
shuts the device down before returning to the command line interpreter
which wakes up the serial UART...

Regards,

Graeme



Regards,

Graeme

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

[Wolfgang Denk]

Dear Graeme,

In message <CALButCKQ_bgpOZquozzQnU01v41Y+uWo8ELLNqSE15D30p52ug@mail.gmail.com> you wrote:
> 
> Well, I have the feeling than an console API might be in order. The way
> U-Boot is structured at the moment, serial I/O is kind of taken for
> granted to 'just exist' and nobody needs to really be self-aware that
> they use it...

Indeed. All we need is a working set of srdin/stdout/stderr.  Keep in
mind that anything couldbe attached - not only a serial line.

>           ... The same cannot be said of users of USB and Network -
> Net is a good example - all 'users' of net call NetLoop() which does
> an eth_init()...do stuff...eth_halt()

Driver API...

> So maybe a serial API which 'users' must 'wake up' and 'sleep'...

Arghhh...

>  - console_configure() - Set parameters (port, baud rate etc)
>  - console_wake() - Prepare the console (could be serial, USB, netconsole)
>  - console_flush() - Throw away any buffered characters
>  - console_getc(), console_putc(), console_tstc() - Self explainatory
>  - console_sleep() - Shut the console down (send XOFF, turn of USB etc)

Forget it.  We will not have a separate and completely non-standard "serial API".
If anything gets changed here, then in the context of a general driver
API cleanup, or at least based on a design for such a generic driver
API.

> UART like your SPI example. The command line interpreter calls
> console_sleep() to release the serial UART resource before the command
> which uses the multiplexed device wakes that device, does it's thing and
> shuts the device down before returning to the command line interpreter
> which wakes up the serial UART...

Sorry, but that's crappy by design.

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
Marriage is the sole cause of divorce.

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

[Graeme Russ]

Hi Wolfgang,

On 26/10/11 18:00, Wolfgang Denk wrote:
> Dear Graeme,
> 
> In message <CALButCKQ_bgpOZquozzQnU01v41Y+uWo8ELLNqSE15D30p52ug@mail.gmail.com> you wrote:
>>
>> Well, I have the feeling than an console API might be in order. The way
>> U-Boot is structured at the moment, serial I/O is kind of taken for
>> granted to 'just exist' and nobody needs to really be self-aware that
>> they use it...
> 
> Indeed. All we need is a working set of srdin/stdout/stderr.  Keep in
> mind that anything couldbe attached - not only a serial line.
> 
>>           ... The same cannot be said of users of USB and Network -
>> Net is a good example - all 'users' of net call NetLoop() which does
>> an eth_init()...do stuff...eth_halt()
> 
> Driver API...
> 
>> So maybe a serial API which 'users' must 'wake up' and 'sleep'...
> 
> Arghhh...
> 
>>  - console_configure() - Set parameters (port, baud rate etc)
>>  - console_wake() - Prepare the console (could be serial, USB, netconsole)
>>  - console_flush() - Throw away any buffered characters
>>  - console_getc(), console_putc(), console_tstc() - Self explainatory
>>  - console_sleep() - Shut the console down (send XOFF, turn of USB etc)
> 
> Forget it.  We will not have a separate and completely non-standard "serial API".
> If anything gets changed here, then in the context of a general driver
> API cleanup, or at least based on a design for such a generic driver
> API.

Funny, I wrote that without the code in front of me. But look at this...

struct serial_device {
	char name[NAMESIZE];

	int  (*init) (void);
	int  (*uninit) (void);
	void (*setbrg) (void);
	int (*getc) (void);
	int (*tstc) (void);
	void (*putc) (const char c);
	void (*puts) (const char *s);
#if CONFIG_POST & CONFIG_SYS_POST_UART
	void (*loop) (int);
#endif

	struct serial_device *next;
};

and

struct stdio_dev {
	int	flags;		/* Device flags: input/output/system	*/
	int	ext;		/* Supported extensions			*/
	char	name[16]	/* Device name				*/

/* GENERAL functions */

	int (*start) (void);	/* To start the device		*/
	int (*stop) (void);	/* To stop the device		*/

/* OUTPUT functions */

	void (*putc) (const char c);	/* To put a char	*/
	void (*puts) (const char *s);	/* To put a string	*/

/* INPUT functions */

	int (*tstc) (void);		/* To test if a char is ready...*/
	int (*getc) (void);		/* To get that char		*/

/* Other functions */

	void *priv;			/* Private extensions		*/
	struct list_head list;
};

and

void serial_stdio_init (void)
{
	struct stdio_dev dev;
	struct serial_device *s = serial_devices;

	while (s) {
		memset (&dev, 0, sizeof (dev));

		strcpy (dev.name, s->name);
		dev.flags = DEV_FLAGS_OUTPUT | DEV_FLAGS_INPUT;

		dev.start = s->init;
		dev.stop = s->uninit;
		dev.putc = s->putc;
		dev.puts = s->puts;
		dev.getc = s->getc;
		dev.tstc = s->tstc;

		stdio_register (&dev);

		s = s->next;
	}
}

and mpc512x has this gem

int close_port(int num)
{
	struct stdio_dev *port;
	int ret;
	char name[7];

	if (num < 0 || num > 11)
		return -1;

	sprintf(name, "psc%d", num);
	port = stdio_get_by_name(name);
	if (!port)
		return -1;

	ret = port->stop();
	clear_bit(num, &initialized);

	return ret;
}

stdio_dev->start() is called in console_setfile() which is called by
console_init_r(). stdio_dev->stop() is never called apart from in the above
mpc512x example.

As I said, stdio is, unlike other devices, taken for granted - It gets
setup during console_init_r() and is assumed to be in-volatile from then on.

So the hooks are already there - All that would be needed is a hook to
allow console users to ultimately call stdio_dev->start when they want to
start using stdio and stdio_dev->stop when they are finished. Of course the
physical device itself is hidden and could be serial, USB, Ethernet, VGA,
Braille display - whatever.

Hmmm, stdio_open() and stdio_close()

>> UART like your SPI example. The command line interpreter calls
>> console_sleep() to release the serial UART resource before the command
>> which uses the multiplexed device wakes that device, does it's thing and
>> shuts the device down before returning to the command line interpreter
>> which wakes up the serial UART...
> 
> Sorry, but that's crappy by design.

When resources are tight - The eNET board has 8-bit CF ports instead of 16
bit because of board space and FPGA limitation which necessitated a slight
mod to the Linux CF driver. Crappy yes, but hey, when given lemons

Regards,

Graeme

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

[Wolfgang Denk]

Dear Graeme Russ,

In message <4EA7D071.9010104@gmail.com> you wrote:
> 
> So the hooks are already there - All that would be needed is a hook to
> allow console users to ultimately call stdio_dev->start when they want to
> start using stdio and stdio_dev->stop when they are finished. Of course the
> physical device itself is hidden and could be serial, USB, Ethernet, VGA,
> Braille display - whatever.

And I say No.  No application whatever should have to care about
sarting or stopping any drivers that provide stdio functionality.

> Hmmm, stdio_open() and stdio_close()

NAK.

Sorry, we are wasting time here.  Can we please put this to an end
now?

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
Above all else -- sky.

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

[Scott Wood]

On 10/25/2011 06:37 PM, Graeme Russ wrote:
> Hi Simon,
> 
> On Wed, Oct 26, 2011 at 10:17 AM, Simon Glass <sjg@chromium.org> wrote:
> 
> [big snip]
> 
>> Did I mention a can of worms? After 65 messages on this topic Scott's
>> patch seems pretty appealing right now! We can even move it up a level
>> in the s/w stack if that helps.
> 
> But I agree with Wolfgang that Scott's proposal only hides the real
> problem even deeper and will make a real solution more difficult
> next time around

Even in Linux you can get loss if you overflow the kernel's buffer.

Flow control is just not that commonly used in this sort of application.
 If we insist on software flow control for U-Boot to get any usable
multi-line pasting to work, the user has to reconfigure their terminal
software for this -- it would take some effort for me to figure out how
to do that on our board farm serial server.  And maybe that's not the
right setting for what the user wants to do with the port later on.

Someone also raised the issue of TX buffering -- and you replied that it
should be turned off if hardware flow control is not used.  This is not
what happens in Linux, at least, and we should not be depending in
U-Boot on the other end not using TX buffers.

The point here is to make an incremental improvement that solves a
problem for some people (even if not "solving" the general problem),
without significant impact elsewhere.  The minor size increase when the
option is not enabled can be fixed.  See previous discussion:

http://lists.denx.de/pipermail/u-boot/2011-April/089878.html

If someone wants to further improve things by adding optional interrupt
support or flow control, I don't see how this patch makes that more
difficult.

-Scott

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

[Wolfgang Denk]

Dear Scott Wood,

In message <4EA83B88.8040109@freescale.com> you wrote:
>
> The point here is to make an incremental improvement that solves a
> problem for some people (even if not "solving" the general problem),

Sorry, but this is not an improment in any way, even if it looks so at
first glance.  Actually it's papering over the bugs that result from
mis-use of the given user interface, without actually providing a fix.

Instead of making the users aware of correct use, and providing help
to do so, this makes people believe that serial input was a reliable
transfer medium and will accept any amount of data dumped to it.

I can only repeat (and I think I'll do this for the last time in this
thread):

- Patches are welcome that allow to overcome this existing limitation
  are welcome - butt hese must provide a reliablke solution that works
  in (at least nearly) all test cases.

- Failure to process multi-line input is a restriction, and if you
  like you may even consider it a bug.  But papering over that bug and
  suggesting to the end users that they are allowed to use multi-line
  input while we all know very well that it actually works only under
  certain (somewhar fragile) assumptions is a fraudulent misrepresen-
  tation.

The approaches discussed so far all have issues. So far I don't see
an approach that appears to be generally usable, clean in design and
reliable in operation.


Regarding this specific patch you submitted: I made my mind up, and I
reject it for the resons given above and in previous messages.

Regarding approaches to lift this restriction: I insist on a clean
design.  It would be helpful if we could base this on a clean driver
model, but we are not there yet - nevertheless, we should keep that in
mind.  Iam also pretty much convinced that the implementation must be
contained within the serial input driver layer, without need to change
"application" code or to put restrictions on the behaviour of any code
that reads or writes from resp. to the console in genral, and the
serial port in particular.

Any code to test one approach or another is welcome, even if it's
half-baked.  But I will not accept such prelimiary code for mainline
that "solves a problem for some people" without actually fixing it.

Thanks.

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
At the source of every error which is blamed on the computer you will
find at least two human errors, including the error of blaming it  on
the computer.

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

[Scott Wood]

On 10/26/2011 01:17 PM, Wolfgang Denk wrote:
> - Failure to process multi-line input is a restriction, and if you
>   like you may even consider it a bug.

Is this restriction documented anywhere?

>   But papering over that bug and
>   suggesting to the end users that they are allowed to use multi-line
>   input while we all know very well that it actually works only under
>   certain (somewhar fragile) assumptions is a fraudulent misrepresen-
>   tation.

Well then I guess you'd better forcibly dump any input received after a
newline, before the next prompt is displayed -- because the current
U-Boot implementation "fraudulently" made me think such a thing was
possible but resource-constrained when it worked for up to 5 lines for
the particular thing I was trying to paste. :-)

> The approaches discussed so far all have issues. So far I don't see
> an approach that appears to be generally usable, clean in design and
> reliable in operation.

That seems like it would be interrupts, but there's a conflict here
between "solve it perfectly (at least up to the limit of the internal
buffer)" and "this is just a bootloader, keep it simple".  And it seems
there's no room for a pragmatic middle ground.

> Regarding this specific patch you submitted: I made my mind up, and I
> reject it for the resons given above and in previous messages.

Oh well, I guess we'll just maintain it locally, or find some
alternative to pasting things.

-Scott

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

[Wolfgang Denk]

Dear Scott Wood,

In message <4EA8566A.1050102@freescale.com> you wrote:
>
> > - Failure to process multi-line input is a restriction, and if you
> >   like you may even consider it a bug.
> 
> Is this restriction documented anywhere?

Only indirectly - it's a consequence of the "strictly single-tasking"
statement.

This is not very explicit, agreed.  Please feel free to improve the
documentation - you are already registered as wiki user, so you just
have to edit the text.

> > The approaches discussed so far all have issues. So far I don't see
> > an approach that appears to be generally usable, clean in design and
> > reliable in operation.
> 
> That seems like it would be interrupts, but there's a conflict here
> between "solve it perfectly (at least up to the limit of the internal
> buffer)" and "this is just a bootloader, keep it simple".  And it seems
> there's no room for a pragmatic middle ground.

I also think an interrupt driven driver would be the best approach.
If the interrupt part can be configured out so users who don't
need/want this feature are unaffected I see no reason why such a
driver would not be accepted. 

> Oh well, I guess we'll just maintain it locally, or find some
> alternative to pasting things.

Some terminal emulations allow for special configurations, like
inter-character delays, end-of-line delays; maybe there are even ones
that can wait for a prompt?  If not - expect is your friend ...

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
On our campus the UNIX system has proved to be not only an  effective
software tool, but an agent of technical and social change within the
University.                          - John Lions (U. of Toronto (?))

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

[Wolfgang Denk]

Dear Graeme,

In message <CALButCKRs7kNa086V-jUC0h-yjA7jfNo+2C9ifKpoREsdmWZQA@mail.gmail.com> you wrote:
> 
> This problem comes down to managing two asynchronous tasks - Command
> Processing and Serial Input Processing. Any solution to the problem
> is going to involve task switching. The way this is done in U-Boot
> currently is:

I wonder how this idea of "tasks" sneaked into your brain.

I agree that one thing we suffer from is a clean device model.  And I
will not hesitate to agree that things would be way easier if the
serial driver was interrupt based.

But tasks in the classical sense have nothing to do with it (I don't
consider an ISR a "task").

>  - When in the main() loop, U-Boot is effectively running a Command
>    Processing Taks (monitor inbound characters, determine when a
>    command has been entered) which continually switches over to Serial
>    Input Processing Task. When a valid command is detected, U-Boot
>    switches to the corresponding Command Processing Task
>  - Some 'commands' (file transfers in particular) have their own Serial
>    Input Processing Sub-Tasks - The Command Processing Task may run for a
>    'significant' amount of time after these Sub-Tasks are finished with
>    and no longer processing serial input

I disagree completely.  There is no such thing as "Command Processing
Tasks" or "Serial Input Processing Tasks" or Subtasks or anything like
that.  There are no tasks at all.

> When you are dealing with asynchronous tasks, you can task switch by
> either:
> 
>  1) Using a hardware interrupt (either a tick-timer or interrupt line from
>     the serial UART)

Yes, an interrupt driven driver would be one valid approach to
adddress this issue.  It brings additional complexity in other areas,
though - things like code size, need for dual mode (polled before
relocation, interrupt driven later?) etc. come to mind.

>  2) Littering the main taks with co-operative interrupt calls
>  3) Task switch at clearly defined locations in the code

> Option 1 is not supported in U-Boot

??? What makes you think so?  It's frowned upon, because the
disadvantages are way bigger than the advantages, but that's all.

> Option 2 is used by U-Boot for triggering the watchdog - It's not pretty,
> but in the absence of #1, we have no other option

This is another misconception.  We do have interrupts on some
architectures, but if you trigger a watchdog from a timer interrupt or
similar you can as well turn it off.  That's not how a watchdog is
supposed to work.

> Option 3 is all we are left with...

I disagree.

> I suggested a solution whereby any task which _knows_ it switches to the
> Serial Input Processing Taks (i.e. calls getc()) can:

Please stop talking about a "Serial Input Processing Task". What you
mean is a serial driver.  This driver is supposed to provide nothing
else to the "application code" than the functionality we can attach to
the stdin and stdout/stderr I/O channels.

All things like flow control (be it hard or soft), buffering, etc.
etc. are internal to the serial driver only.

Actually please stop talking about tasks at all.  U-Boot is
single-tasked.  Period.

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
Cogito cogito ergo cogito sum - "I think that I  think,  therefore  I
think that I am."          - Ambrose Bierce, "The Devil's Dictionary"

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Wolfgang Denk]

Dear Simon Glass,

In message <1318742050-2201-2-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>

I've made up my mind.  The reasons why you want to add such code are
well understood, but the unsolved and unsolvable issues with this
approach are so severe that I don't want to raise false hopes in
innocent users.

Sorry, but I reject this patch.

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
"Nature is very un-American.  Nature never hurries."
- William George Jordan

[U-Boot] [PATCH v4 1/2] NS16550: trivial code clean for checkpatch

[Wolfgang Denk]

Dear Simon Glass,

In message <1318742050-2201-1-git-send-email-sjg@chromium.org> you wrote:
> This removes most checkpatch warnings from the ns16550 driver and its
> header.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  drivers/serial/ns16550.c |   37 ++++++++++++++++++++-----------------
>  include/ns16550.h        |   16 ++++++++--------
>  2 files changed, 28 insertions(+), 25 deletions(-)

Applied, thanks.

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
Don't put off for tomorrow what you can  do  today,  because  if  you
enjoy it today you can do it again tomorrow.

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Wolfgang Denk]

Dear Simon,

I wrote:

> - However, your implementation does not result in reliable multi-line
>   input.  It works only in a few specific use cases, and will fail
>   silently for others.  I don't see a way ho you would announce this
>   new feature.  The numbers you mention in the commit message ("it
>   handles around 70 lines before lossage") apply for this specific
>   board only, and for your use case only (pasting of short lines that
>   produce no output).

Actually I believe that the restriction is even worse - I think that
any commands that consume any significant amount of time will casue
problems, too.  Can you please test what happens when you have, for
example, a "sleep 10" or a "erase all" in the first few lines of the
pasted input ?

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
What can it profit a man to gain the whole world and to come  to  his
property with a gastric ulcer, a blown prostate, and bifocals?
                                     -- John Steinbeck, _Cannery Row_

[U-Boot] [PATCH v4 2/2] NS16550: buffer reads

[Scott Wood]

On 10/17/2011 07:53 AM, Wolfgang Denk wrote:
> Dear Simon,
> 
> I wrote:
> 
>> - However, your implementation does not result in reliable multi-line
>>   input.  It works only in a few specific use cases, and will fail
>>   silently for others.  I don't see a way ho you would announce this
>>   new feature.  The numbers you mention in the commit message ("it
>>   handles around 70 lines before lossage") apply for this specific
>>   board only, and for your use case only (pasting of short lines that
>>   produce no output).
> 
> Actually I believe that the restriction is even worse - I think that
> any commands that consume any significant amount of time will casue
> problems, too.  Can you please test what happens when you have, for
> example, a "sleep 10" or a "erase all" in the first few lines of the
> pasted input ?

The reference to "70 lines" was meant as an anecdote about how it made
pasting things significantly more usable for me.  I never advertised it
as a complete fix for serial loss (indeed, it even says that I still
experienced loss after 70 lines).  It's just a fairly simple change that
lessens the problem for me (and apparently others) to the point where
it's no longer a significant nuisance.

-Scott