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

[U-Boot] [PATCH v2] 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

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

diff --git a/README b/README
index 7e032a9..28d2e8a 100644
--- a/README
+++ b/README
@@ -2862,6 +2862,14 @@ use the "saveenv" command to store a valid environment.
 		space for already greatly restricted images, including but not
 		limited to NAND_SPL configurations.
 
+- CONFIG_NS16550_BUFFER_READS:
+		Instead of reading directly from the receive register
+		every time U-Boot is ready for another byte, keep a
+		buffer and fill it from the hardware fifo every time
+		U-Boot reads a character.  This helps U-Boot keep up with
+		a larger amount of rapid input, such as happens when
+		pasting text into the terminal.
+
 Low Level (hardware related) configuration options:
 ---------------------------------------------------
 
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 0174744..a012e5d 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -4,12 +4,15 @@
  * modified to use CONFIG_SYS_ISA_MEM and new defines
  */
 
+#include <common.h>
 #include <config.h>
 #include <ns16550.h>
 #include <watchdog.h>
 #include <linux/types.h>
 #include <asm/io.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 #define UART_LCRVAL UART_LCR_8N1		/* 8 data, 1 stop, no parity */
 #define UART_MCRVAL (UART_MCR_DTR | \
 		     UART_MCR_RTS)		/* RTS/DTR */
@@ -92,21 +95,104 @@ void NS16550_putc (NS16550_t com_port, char c)
 }
 
 #ifndef CONFIG_NS16550_MIN_FUNCTIONS
-char NS16550_getc (NS16550_t com_port)
+
+static char NS16550_raw_getc(NS16550_t regs)
 {
-	while ((serial_in(&com_port->lsr) & UART_LSR_DR) == 0) {
+	while ((serial_in(&regs->lsr) & UART_LSR_DR) == 0) {
 #ifdef CONFIG_USB_TTY
 		extern void usbtty_poll(void);
 		usbtty_poll();
 #endif
 		WATCHDOG_RESET();
 	}
-	return serial_in(&com_port->rbr);
+	return serial_in(&regs->rbr);
+}
+
+static int NS16550_raw_tstc(NS16550_t regs)
+{
+	return ((serial_in(&regs->lsr) & UART_LSR_DR) != 0);
+}
+
+
+#ifdef CONFIG_NS16550_BUFFER_READS
+
+#define BUF_SIZE 256
+#define NUM_PORTS 4
+
+struct ns16550_priv {
+	char buf[BUF_SIZE];
+	unsigned int head, tail;
+};
+
+static struct ns16550_priv rxstate[NUM_PORTS];
+
+static void enqueue(unsigned int port, char ch)
+{
+	/* If queue is full, drop the character. */
+	if ((rxstate[port].head - rxstate[port].tail - 1) % BUF_SIZE == 0)
+		return;
+
+	rxstate[port].buf[rxstate[port].tail] = ch;
+	rxstate[port].tail = (rxstate[port].tail + 1) % BUF_SIZE;
+}
+
+static int dequeue(unsigned int port, char *ch)
+{
+	/* Empty queue? */
+	if (rxstate[port].head == rxstate[port].tail)
+		return 0;
+
+	*ch = rxstate[port].buf[rxstate[port].head];
+	rxstate[port].head = (rxstate[port].head + 1) % BUF_SIZE;
+	return 1;
+}
+
+static void fill_rx_buf(NS16550_t regs, unsigned int port)
+{
+	while ((serial_in(&regs->lsr) & UART_LSR_DR) != 0)
+		enqueue(port, serial_in(&regs->rbr));
+}
+
+char NS16550_getc(NS16550_t regs, unsigned int port)
+{
+	char ch;
+
+	if (port >= NUM_PORTS || !(gd->flags & GD_FLG_RELOC))
+		return NS16550_raw_getc(regs);
+
+	do  {
+#ifdef CONFIG_USB_TTY
+		extern void usbtty_poll(void);
+		usbtty_poll();
+#endif
+		fill_rx_buf(regs, port);
+		WATCHDOG_RESET();
+	} while (!dequeue(port, &ch));
+
+	return ch;
+}
+
+int NS16550_tstc(NS16550_t regs, unsigned int port)
+{
+	if (port >= NUM_PORTS || !(gd->flags & GD_FLG_RELOC))
+		return NS16550_raw_tstc(regs);
+
+	fill_rx_buf(regs, port);
+
+	return rxstate[port].head != rxstate[port].tail;
+}
+
+#else /* CONFIG_NS16550_BUFFER_READS */
+
+char NS16550_getc(NS16550_t regs, unsigned int port)
+{
+	return NS16550_raw_getc(regs);
 }
 
-int NS16550_tstc (NS16550_t com_port)
+int NS16550_tstc(NS16550_t regs, unsigned int port)
 {
-	return ((serial_in(&com_port->lsr) & UART_LSR_DR) != 0);
+	return NS16550_raw_tstc(regs);
 }
 
+#endif /* CONFIG_NS16550_BUFFER_READS */
 #endif /* CONFIG_NS16550_MIN_FUNCTIONS */
diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
index 0d56e78..cf9a1dd 100644
--- a/drivers/serial/serial.c
+++ b/drivers/serial/serial.c
@@ -93,6 +93,7 @@ static NS16550_t serial_ports[4] = {
 };
 
 #define PORT	serial_ports[port-1]
+#define PORTNR	(port-1)
 #if defined(CONFIG_CONS_INDEX)
 #define CONSOLE	(serial_ports[CONFIG_CONS_INDEX-1])
 #endif
@@ -219,13 +220,13 @@ _serial_puts (const char *s,const int port)
 int
 _serial_getc(const int port)
 {
-	return NS16550_getc(PORT);
+	return NS16550_getc(PORT, PORTNR);
 }
 
 int
 _serial_tstc(const int port)
 {
-	return NS16550_tstc(PORT);
+	return NS16550_tstc(PORT, PORTNR);
 }
 
 void
diff --git a/include/ns16550.h b/include/ns16550.h
index 51f1c17..60b0abc 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -164,6 +164,6 @@ typedef volatile struct NS16550 *NS16550_t;
 
 void	NS16550_init   (NS16550_t com_port, int baud_divisor);
 void	NS16550_putc   (NS16550_t com_port, char c);
-char	NS16550_getc   (NS16550_t com_port);
-int	NS16550_tstc   (NS16550_t com_port);
+char NS16550_getc(NS16550_t regs, unsigned int port);
+int NS16550_tstc(NS16550_t regs, unsigned int port);
 void	NS16550_reinit (NS16550_t com_port, int baud_divisor);
-- 
1.7.3.1

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

[Albert ARIBAUD]

Hi Simon,

Le 15/10/2011 02:03, 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

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

>   README                   |    8 ++++
>   drivers/serial/ns16550.c |   96 +++++++++++++++++++++++++++++++++++++++++++--
>   drivers/serial/serial.c  |    5 +-
>   include/ns16550.h        |    4 +-
>   4 files changed, 104 insertions(+), 9 deletions(-)
>
> diff --git a/README b/README
> index 7e032a9..28d2e8a 100644
> --- a/README
> +++ b/README
> @@ -2862,6 +2862,14 @@ use the "saveenv" command to store a valid environment.
>   		space for already greatly restricted images, including but not
>   		limited to NAND_SPL configurations.
>
> +- CONFIG_NS16550_BUFFER_READS:
> +		Instead of reading directly from the receive register
> +		every time U-Boot is ready for another byte, keep a
> +		buffer and fill it from the hardware fifo every time
> +		U-Boot reads a character.  This helps U-Boot keep up with
> +		a larger amount of rapid input, such as happens when
> +		pasting text into the terminal.
> +
>   Low Level (hardware related) configuration options:
>   ---------------------------------------------------
>
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 0174744..a012e5d 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -4,12 +4,15 @@
>    * modified to use CONFIG_SYS_ISA_MEM and new defines
>    */
>
> +#include<common.h>
>   #include<config.h>
>   #include<ns16550.h>
>   #include<watchdog.h>
>   #include<linux/types.h>
>   #include<asm/io.h>
>
> +DECLARE_GLOBAL_DATA_PTR;
> +
>   #define UART_LCRVAL UART_LCR_8N1		/* 8 data, 1 stop, no parity */
>   #define UART_MCRVAL (UART_MCR_DTR | \
>   		     UART_MCR_RTS)		/* RTS/DTR */
> @@ -92,21 +95,104 @@ void NS16550_putc (NS16550_t com_port, char c)
>   }
>
>   #ifndef CONFIG_NS16550_MIN_FUNCTIONS
> -char NS16550_getc (NS16550_t com_port)
> +
> +static char NS16550_raw_getc(NS16550_t regs)
>   {
> -	while ((serial_in(&com_port->lsr)&  UART_LSR_DR) == 0) {
> +	while ((serial_in(&regs->lsr)&  UART_LSR_DR) == 0) {
>   #ifdef CONFIG_USB_TTY
>   		extern void usbtty_poll(void);
>   		usbtty_poll();
>   #endif
>   		WATCHDOG_RESET();
>   	}
> -	return serial_in(&com_port->rbr);
> +	return serial_in(&regs->rbr);
> +}
> +
> +static int NS16550_raw_tstc(NS16550_t regs)
> +{
> +	return ((serial_in(&regs->lsr)&  UART_LSR_DR) != 0);
> +}
> +
> +
> +#ifdef CONFIG_NS16550_BUFFER_READS
> +
> +#define BUF_SIZE 256

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

> +#define NUM_PORTS 4
> +
> +struct ns16550_priv {
> +	char buf[BUF_SIZE];
> +	unsigned int head, tail;
> +};
> +
> +static struct ns16550_priv rxstate[NUM_PORTS];
> +
> +static void enqueue(unsigned int port, char ch)
> +{
> +	/* If queue is full, drop the character. */
> +	if ((rxstate[port].head - rxstate[port].tail - 1) % BUF_SIZE == 0)
> +		return;
> +
> +	rxstate[port].buf[rxstate[port].tail] = ch;
> +	rxstate[port].tail = (rxstate[port].tail + 1) % BUF_SIZE;
> +}
> +
> +static int dequeue(unsigned int port, char *ch)
> +{
> +	/* Empty queue? */
> +	if (rxstate[port].head == rxstate[port].tail)
> +		return 0;
> +
> +	*ch = rxstate[port].buf[rxstate[port].head];
> +	rxstate[port].head = (rxstate[port].head + 1) % BUF_SIZE;
> +	return 1;
> +}
> +
> +static void fill_rx_buf(NS16550_t regs, unsigned int port)
> +{
> +	while ((serial_in(&regs->lsr)&  UART_LSR_DR) != 0)
> +		enqueue(port, serial_in(&regs->rbr));
> +}
> +
> +char NS16550_getc(NS16550_t regs, unsigned int port)
> +{
> +	char ch;
> +
> +	if (port>= NUM_PORTS || !(gd->flags&  GD_FLG_RELOC))
> +		return NS16550_raw_getc(regs);
> +
> +	do  {
> +#ifdef CONFIG_USB_TTY
> +		extern void usbtty_poll(void);
> +		usbtty_poll();
> +#endif
> +		fill_rx_buf(regs, port);
> +		WATCHDOG_RESET();
> +	} while (!dequeue(port,&ch));
> +
> +	return ch;
> +}
> +
> +int NS16550_tstc(NS16550_t regs, unsigned int port)
> +{
> +	if (port>= NUM_PORTS || !(gd->flags&  GD_FLG_RELOC))
> +		return NS16550_raw_tstc(regs);
> +
> +	fill_rx_buf(regs, port);
> +
> +	return rxstate[port].head != rxstate[port].tail;
> +}
> +
> +#else /* CONFIG_NS16550_BUFFER_READS */
> +
> +char NS16550_getc(NS16550_t regs, unsigned int port)
> +{
> +	return NS16550_raw_getc(regs);
>   }
>
> -int NS16550_tstc (NS16550_t com_port)
> +int NS16550_tstc(NS16550_t regs, unsigned int port)
>   {
> -	return ((serial_in(&com_port->lsr)&  UART_LSR_DR) != 0);
> +	return NS16550_raw_tstc(regs);
>   }
>
> +#endif /* CONFIG_NS16550_BUFFER_READS */
>   #endif /* CONFIG_NS16550_MIN_FUNCTIONS */
> diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
> index 0d56e78..cf9a1dd 100644
> --- a/drivers/serial/serial.c
> +++ b/drivers/serial/serial.c
> @@ -93,6 +93,7 @@ static NS16550_t serial_ports[4] = {
>   };
>
>   #define PORT	serial_ports[port-1]
> +#define PORTNR	(port-1)
>   #if defined(CONFIG_CONS_INDEX)
>   #define CONSOLE	(serial_ports[CONFIG_CONS_INDEX-1])
>   #endif
> @@ -219,13 +220,13 @@ _serial_puts (const char *s,const int port)
>   int
>   _serial_getc(const int port)
>   {
> -	return NS16550_getc(PORT);
> +	return NS16550_getc(PORT, PORTNR);
>   }
>
>   int
>   _serial_tstc(const int port)
>   {
> -	return NS16550_tstc(PORT);
> +	return NS16550_tstc(PORT, PORTNR);
>   }
>
>   void
> diff --git a/include/ns16550.h b/include/ns16550.h
> index 51f1c17..60b0abc 100644
> --- a/include/ns16550.h
> +++ b/include/ns16550.h
> @@ -164,6 +164,6 @@ typedef volatile struct NS16550 *NS16550_t;
>
>   void	NS16550_init   (NS16550_t com_port, int baud_divisor);
>   void	NS16550_putc   (NS16550_t com_port, char c);
> -char	NS16550_getc   (NS16550_t com_port);
> -int	NS16550_tstc   (NS16550_t com_port);
> +char NS16550_getc(NS16550_t regs, unsigned int port);
> +int NS16550_tstc(NS16550_t regs, unsigned int port);
>   void	NS16550_reinit (NS16550_t com_port, int baud_divisor);


Amicalement,
-- 
Albert.

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

[Simon Glass]

Hi Albert,

On Sat, Oct 15, 2011 at 3:43 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Simon,
>
> Le 15/10/2011 02:03, 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
>
> A general question: is the issue with input as such, or with echoing this
> input?

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

>
>> ?README ? ? ? ? ? ? ? ? ? | ? ?8 ++++
>> ?drivers/serial/ns16550.c | ? 96
>> +++++++++++++++++++++++++++++++++++++++++++--
>> ?drivers/serial/serial.c ?| ? ?5 +-
>> ?include/ns16550.h ? ? ? ?| ? ?4 +-
>> ?4 files changed, 104 insertions(+), 9 deletions(-)
>>
>> diff --git a/README b/README
>> index 7e032a9..28d2e8a 100644
>> --- a/README
>> +++ b/README
>> @@ -2862,6 +2862,14 @@ use the "saveenv" command to store a valid
>> environment.
>> ? ? ? ? ? ? ? ?space for already greatly restricted images, including but
>> not
>> ? ? ? ? ? ? ? ?limited to NAND_SPL configurations.
>>
>> +- CONFIG_NS16550_BUFFER_READS:
>> + ? ? ? ? ? ? ? Instead of reading directly from the receive register
>> + ? ? ? ? ? ? ? every time U-Boot is ready for another byte, keep a
>> + ? ? ? ? ? ? ? buffer and fill it from the hardware fifo every time
>> + ? ? ? ? ? ? ? U-Boot reads a character. ?This helps U-Boot keep up with
>> + ? ? ? ? ? ? ? a larger amount of rapid input, such as happens when
>> + ? ? ? ? ? ? ? pasting text into the terminal.
>> +
>> ?Low Level (hardware related) configuration options:
>> ?---------------------------------------------------
>>
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index 0174744..a012e5d 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -4,12 +4,15 @@
>> ? * modified to use CONFIG_SYS_ISA_MEM and new defines
>> ? */
>>
>> +#include<common.h>
>> ?#include<config.h>
>> ?#include<ns16550.h>
>> ?#include<watchdog.h>
>> ?#include<linux/types.h>
>> ?#include<asm/io.h>
>>
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> ?#define UART_LCRVAL UART_LCR_8N1 ? ? ? ? ? ? ?/* 8 data, 1 stop, no
>> parity */
>> ?#define UART_MCRVAL (UART_MCR_DTR | \
>> ? ? ? ? ? ? ? ? ? ? UART_MCR_RTS) ? ? ? ? ? ? ?/* RTS/DTR */
>> @@ -92,21 +95,104 @@ void NS16550_putc (NS16550_t com_port, char c)
>> ?}
>>
>> ?#ifndef CONFIG_NS16550_MIN_FUNCTIONS
>> -char NS16550_getc (NS16550_t com_port)
>> +
>> +static char NS16550_raw_getc(NS16550_t regs)
>> ?{
>> - ? ? ? while ((serial_in(&com_port->lsr)& ?UART_LSR_DR) == 0) {
>> + ? ? ? while ((serial_in(&regs->lsr)& ?UART_LSR_DR) == 0) {
>> ?#ifdef CONFIG_USB_TTY
>> ? ? ? ? ? ? ? ?extern void usbtty_poll(void);
>> ? ? ? ? ? ? ? ?usbtty_poll();
>> ?#endif
>> ? ? ? ? ? ? ? ?WATCHDOG_RESET();
>> ? ? ? ?}
>> - ? ? ? return serial_in(&com_port->rbr);
>> + ? ? ? return serial_in(&regs->rbr);
>> +}
>> +
>> +static int NS16550_raw_tstc(NS16550_t regs)
>> +{
>> + ? ? ? return ((serial_in(&regs->lsr)& ?UART_LSR_DR) != 0);
>> +}
>> +
>> +
>> +#ifdef CONFIG_NS16550_BUFFER_READS
>> +
>> +#define BUF_SIZE 256
>
> I guess this define conditions how well U-Boot will cope with steady
> high-rate input, correct? If so, I'd like it to be a configuration option --
> actually, one could combine enabling the option *and* setting the expected
> buffer size.

Yes so you could have something like:

#define CONFIG_NS16550_BUFFER_READS 256

>
>> +#define NUM_PORTS 4
>> +
>> +struct ns16550_priv {
>> + ? ? ? char buf[BUF_SIZE];
>> + ? ? ? unsigned int head, tail;
>> +};
>> +
>> +static struct ns16550_priv rxstate[NUM_PORTS];
>> +
>> +static void enqueue(unsigned int port, char ch)
>> +{
>> + ? ? ? /* If queue is full, drop the character. */
>> + ? ? ? if ((rxstate[port].head - rxstate[port].tail - 1) % BUF_SIZE == 0)
>> + ? ? ? ? ? ? ? return;
>> +
>> + ? ? ? rxstate[port].buf[rxstate[port].tail] = ch;
>> + ? ? ? rxstate[port].tail = (rxstate[port].tail + 1) % BUF_SIZE;
>> +}
>> +
>> +static int dequeue(unsigned int port, char *ch)
>> +{
>> + ? ? ? /* Empty queue? */
>> + ? ? ? if (rxstate[port].head == rxstate[port].tail)
>> + ? ? ? ? ? ? ? return 0;
>> +
>> + ? ? ? *ch = rxstate[port].buf[rxstate[port].head];
>> + ? ? ? rxstate[port].head = (rxstate[port].head + 1) % BUF_SIZE;
>> + ? ? ? return 1;
>> +}
>> +
>> +static void fill_rx_buf(NS16550_t regs, unsigned int port)
>> +{
>> + ? ? ? while ((serial_in(&regs->lsr)& ?UART_LSR_DR) != 0)
>> + ? ? ? ? ? ? ? enqueue(port, serial_in(&regs->rbr));
>> +}
>> +
>> +char NS16550_getc(NS16550_t regs, unsigned int port)
>> +{
>> + ? ? ? char ch;
>> +
>> + ? ? ? if (port>= NUM_PORTS || !(gd->flags& ?GD_FLG_RELOC))
>> + ? ? ? ? ? ? ? return NS16550_raw_getc(regs);
>> +
>> + ? ? ? do ?{
>> +#ifdef CONFIG_USB_TTY
>> + ? ? ? ? ? ? ? extern void usbtty_poll(void);
>> + ? ? ? ? ? ? ? usbtty_poll();
>> +#endif
>> + ? ? ? ? ? ? ? fill_rx_buf(regs, port);
>> + ? ? ? ? ? ? ? WATCHDOG_RESET();
>> + ? ? ? } while (!dequeue(port,&ch));
>> +
>> + ? ? ? return ch;
>> +}
>> +
>> +int NS16550_tstc(NS16550_t regs, unsigned int port)
>> +{
>> + ? ? ? if (port>= NUM_PORTS || !(gd->flags& ?GD_FLG_RELOC))
>> + ? ? ? ? ? ? ? return NS16550_raw_tstc(regs);
>> +
>> + ? ? ? fill_rx_buf(regs, port);
>> +
>> + ? ? ? return rxstate[port].head != rxstate[port].tail;
>> +}
>> +
>> +#else /* CONFIG_NS16550_BUFFER_READS */
>> +
>> +char NS16550_getc(NS16550_t regs, unsigned int port)
>> +{
>> + ? ? ? return NS16550_raw_getc(regs);
>> ?}
>>
>> -int NS16550_tstc (NS16550_t com_port)
>> +int NS16550_tstc(NS16550_t regs, unsigned int port)
>> ?{
>> - ? ? ? return ((serial_in(&com_port->lsr)& ?UART_LSR_DR) != 0);
>> + ? ? ? return NS16550_raw_tstc(regs);
>> ?}
>>
>> +#endif /* CONFIG_NS16550_BUFFER_READS */
>> ?#endif /* CONFIG_NS16550_MIN_FUNCTIONS */
>> diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
>> index 0d56e78..cf9a1dd 100644
>> --- a/drivers/serial/serial.c
>> +++ b/drivers/serial/serial.c
>> @@ -93,6 +93,7 @@ static NS16550_t serial_ports[4] = {
>> ?};
>>
>> ?#define PORT ?serial_ports[port-1]
>> +#define PORTNR (port-1)
>> ?#if defined(CONFIG_CONS_INDEX)
>> ?#define CONSOLE ? ? ? (serial_ports[CONFIG_CONS_INDEX-1])
>> ?#endif
>> @@ -219,13 +220,13 @@ _serial_puts (const char *s,const int port)
>> ?int
>> ?_serial_getc(const int port)
>> ?{
>> - ? ? ? return NS16550_getc(PORT);
>> + ? ? ? return NS16550_getc(PORT, PORTNR);
>> ?}
>>
>> ?int
>> ?_serial_tstc(const int port)
>> ?{
>> - ? ? ? return NS16550_tstc(PORT);
>> + ? ? ? return NS16550_tstc(PORT, PORTNR);
>> ?}
>>
>> ?void
>> diff --git a/include/ns16550.h b/include/ns16550.h
>> index 51f1c17..60b0abc 100644
>> --- a/include/ns16550.h
>> +++ b/include/ns16550.h
>> @@ -164,6 +164,6 @@ typedef volatile struct NS16550 *NS16550_t;
>>
>> ?void ?NS16550_init ? (NS16550_t com_port, int baud_divisor);
>> ?void ?NS16550_putc ? (NS16550_t com_port, char c);
>> -char ? NS16550_getc ? (NS16550_t com_port);
>> -int ? ?NS16550_tstc ? (NS16550_t com_port);
>> +char NS16550_getc(NS16550_t regs, unsigned int port);
>> +int NS16550_tstc(NS16550_t regs, unsigned int port);
>> ?void ?NS16550_reinit (NS16550_t com_port, int baud_divisor);
>
>
> Amicalement,
> --
> Albert.
>

Regards,
Simon

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ34FFotBBiqb=Y5xsuyRiavOWud5G2sy9GModsG4LB6=w@mail.gmail.com> you wrote:
> 
> > A general question: is the issue with input as such, or with echoing this
> > input?
> 
> The problem is in echoing the input, since this requires the uart to
> wait once its transmit FIFO is full. While it is waiting (and not
> checking the receive side), it misses characters coming from the other
> end. Basically we need to constantly check the receiving side while we
> are transmitting.

No, we do not.

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

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I believe you find life such a problem because you  think  there  are
the  good  people  and the bad people. You're wrong, of course. There
are, always and only, the bad people, but some of them are  on  oppo-
site sides.                      - Terry Pratchett, _Guards! Guards!_

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

[Simon Glass]

Hi Wolfgang,

On Sat, Oct 15, 2011 at 9:02 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ34FFotBBiqb=Y5xsuyRiavOWud5G2sy9GModsG4LB6=w@mail.gmail.com> you wrote:
>>
>> > A general question: is the issue with input as such, or with echoing this
>> > input?
>>
>> The problem is in echoing the input, since this requires the uart to
>> wait once its transmit FIFO is full. While it is waiting (and not
>> checking the receive side), it misses characters coming from the other
>> end. Basically we need to constantly check the receiving side while we
>> are transmitting.
>
> No, we do not.
>
> U-Boot is strictly single tasking. This is one of the fundamental
> design principles. We do not receive network packets while sending
> ourself, or when doing other things. We do not receive characters on
> the serial line while sending characters, or when doing other things.
>
> This is known behaviour, and you should just adapt to it. I don't know
> what exactly is your situation when you run into such problems - the
> recommended approach for example when remote-controlling U-Boot is to
> wait for the next input prompt before sending the next line of
> characters - and stop after the newline, until you receive the next
> prompt.

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

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

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> I believe you find life such a problem because you ?think ?there ?are
> the ?good ?people ?and the bad people. You're wrong, of course. There
> are, always and only, the bad people, but some of them are ?on ?oppo-
> site sides. ? ? ? ? ? ? ? ? ? ? ?- Terry Pratchett, _Guards! Guards!_
>

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

[Albert ARIBAUD]

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

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

Amicalement,
-- 
Albert.

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

[Simon Glass]

Hi Albert,

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

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

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

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

Regards,
Simon

>
> Amicalement,
> --
> Albert.
>

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

[Simon Glass]

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

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

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

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

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

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

Regards,
Simon

>
>>
>> Amicalement,
>> --
>> Albert.
>>
>

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ0s0-P4ubDb=AcB=28otKw+Fi08yLNi2GePoFqBRMn3Gw@mail.gmail.com> you wrote:
>
> Another thought...the problem is that every character we receive we
> echo, so we can of course never get ahead of the workload.  But we can

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

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

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
To understand a program you must become  both  the  machine  and  the
program.

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

[Simon Glass]

Hi Wolfgang,

On Sat, Oct 15, 2011 at 12:14 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ0s0-P4ubDb=AcB=28otKw+Fi08yLNi2GePoFqBRMn3Gw@mail.gmail.com> you wrote:
>>
>> Another thought...the problem is that every character we receive we
>> echo, so we can of course never get ahead of the workload. ?But we can
>
> Even worse, if your input contains any commands that prodce output,
> mayeb even lots of output (say "md $addr 10000") then even your
> increased buffer size is not going to help you.

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

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

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

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

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> To understand a program you must become ?both ?the ?machine ?and ?the
> program.
>

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ2Qf-Egdp9WCnoWVPJmjic5BPF0Db+z1oD=fdb3aoLNKw@mail.gmail.com> you wrote:
> 
> > Even worse, if your input contains any commands that prodce output,
> > mayeb even lots of output (say "md $addr 10000") then even your
> > increased buffer size is not going to help you.
> 
> It helps a little - provided there are no more than 250 characters
> more 'serial input' after the md command, all is well. But I think
> this example is a little contrived.

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

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

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

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

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

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Uncertain fortune is thoroughly mastered by the equity of the  calcu-
lation.                                               - Blaise Pascal

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

[Simon Glass]

Hi Wolfgang,

On Sun, Oct 16, 2011 at 12:52 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ2Qf-Egdp9WCnoWVPJmjic5BPF0Db+z1oD=fdb3aoLNKw@mail.gmail.com> you wrote:
>>
>> > Even worse, if your input contains any commands that prodce output,
>> > mayeb even lots of output (say "md $addr 10000") then even your
>> > increased buffer size is not going to help you.
>>
>> It helps a little - provided there are no more than 250 characters
>> more 'serial input' after the md command, all is well. But I think
>> this example is a little contrived.
>
> 250 characters? That's 4, maybe 5 lines of text. ?Not exactly much...

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

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

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

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

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

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

Yes, so in summary, we have:

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

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

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

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

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

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Uncertain fortune is thoroughly mastered by the equity of the ?calcu-
> lation. ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? - Blaise Pascal
>

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ37O2tr1kY5f-uv1zymGBdMLbyEhm6y-=rfENJ62jnREg@mail.gmail.com> you wrote:
> 
> > How about using the time-honored flow-control mechanism? If the serial port
> > supports hardware CTS and RTS signals, you can use them to sync with the
> > sender. If not, you can send XOFF and XON to respectively ask the sender to
> > pause and resume sending. Of course, the sender must support hardware or
> > software flow control.
>
> It is common to have just a 2-wire UART for the console. Perhaps this
> is unfortunate.

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

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

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

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

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"We don't care.  We don't have to.  We're the Phone Company."

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ3wZo4nkP2GjKbS_r8AWmRaCkagFDqrXrJg5MniykxCNw@mail.gmail.com> you wrote:
> 
> The situation this occurs is when you paste characters into a serial
> terminal connected to U-Boot. This is a pretty common requirement. Yes
> you can manually select each line and paste it but that is a pain.

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

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

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

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

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

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

Multi-line input has never been supported.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Alan Turing thought about criteria to settle the question of  whether
machines  can think, a question of which we now know that it is about
as relevant as the question of whether submarines can swim.
                                                   -- Edsger Dijkstra

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

[Simon Glass]

Hi Wolfgang,

On Sat, Oct 15, 2011 at 12:00 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ3wZo4nkP2GjKbS_r8AWmRaCkagFDqrXrJg5MniykxCNw@mail.gmail.com> you wrote:
>>
>> The situation this occurs is when you paste characters into a serial
>> terminal connected to U-Boot. This is a pretty common requirement. Yes
>> you can manually select each line and paste it but that is a pain.
>
> But that's exactly how it's supposed to work. ?Depending on hardware
> that may or may not be FIFOs available to buffer such asynchronous
> input, and I rather see limited, but consistent behaviour, instead of
> fancy stuff that works oin one board but not on another.

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

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

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

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

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

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

That's right.

>
> Multi-line input has never been supported.

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

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Alan Turing thought about criteria to settle the question of ?whether
> machines ?can think, a question of which we now know that it is about
> as relevant as the question of whether submarines can swim.
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? -- Edsger Dijkstra
>

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ0RjEhY7-n4=vFACgzpvSgMZJOjaCbngbdB1J2HaxFK6Q@mail.gmail.com> you wrote:
>
> > But that's exactly how it's supposed to work. =A0Depending on hardware
> > that may or may not be FIFOs available to buffer such asynchronous
> > input, and I rather see limited, but consistent behaviour, instead of
> > fancy stuff that works oin one board but not on another.
> 
> Well this is particular serial driver is in pretty widespread use. We

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

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

Well, there is two cases:

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

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

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

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

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

  4) ...

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

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

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

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

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Little known fact about Middle Earth:   The Hobbits had a very sophi-
sticated computer network!   It was a Tolkien Ring...

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

[Simon Glass]

Hi Wolfgang,

On Sun, Oct 16, 2011 at 12:47 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ0RjEhY7-n4=vFACgzpvSgMZJOjaCbngbdB1J2HaxFK6Q@mail.gmail.com> you wrote:
>>
>> > But that's exactly how it's supposed to work. =A0Depending on hardware
>> > that may or may not be FIFOs available to buffer such asynchronous
>> > input, and I rather see limited, but consistent behaviour, instead of
>> > fancy stuff that works oin one board but not on another.
>>
>> Well this is particular serial driver is in pretty widespread use. We
>
> Widespread, yes. ?But there is still a huge number of boards where it
> is not being used.

Yes.

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

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

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

This is not friendly behaviour.

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

To repeat the bit of my message that you omitted:

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

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

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

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

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

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

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

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

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

Regards,
SImon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Little known fact about Middle Earth: ? The Hobbits had a very sophi-
> sticated computer network! ? It was a Tolkien Ring...
>

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ0yVMqzyGceUEXiBt6dtsVuqikSPJ_Vw8VU_nb-ST6zJA@mail.gmail.com> you wrote:
> 
> I think this is what said before. In the case of expect, I must put
> the board into the right state, disconnect my terminal from the board,
> run the expect script and re-connect.

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

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

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

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

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

And the BIG benfit is: it just works.

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

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

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

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

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It is clear that the individual who persecutes a  man,  his  brother,
because he is not of the same opinion, is a monster.       - Voltaire

[U-Boot] How to unsubscribe.

[Sandeep Kumar]

Hi,
How can I unsubscribe from this mailing list.

Regards,
Sandeep


-----Original Message-----
From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces at lists.denx.de]
On Behalf Of Simon Glass
Sent: 15 October 2011 05:33
To: U-Boot Mailing List
Cc: Scott Wood; Kumar Gala
Subject: [U-Boot] [PATCH v2] NS16550: buffer reads

From: Scott Wood <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

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

diff --git a/README b/README
index 7e032a9..28d2e8a 100644
--- a/README
+++ b/README
@@ -2862,6 +2862,14 @@ use the "saveenv" command to store a valid
environment.
 		space for already greatly restricted images, including
but not
 		limited to NAND_SPL configurations.
 
+- CONFIG_NS16550_BUFFER_READS:
+		Instead of reading directly from the receive register
+		every time U-Boot is ready for another byte, keep a
+		buffer and fill it from the hardware fifo every time
+		U-Boot reads a character.  This helps U-Boot keep up
with
+		a larger amount of rapid input, such as happens when
+		pasting text into the terminal.
+
 Low Level (hardware related) configuration options:
 ---------------------------------------------------
 
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 0174744..a012e5d 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -4,12 +4,15 @@
  * modified to use CONFIG_SYS_ISA_MEM and new defines
  */
 
+#include <common.h>
 #include <config.h>
 #include <ns16550.h>
 #include <watchdog.h>
 #include <linux/types.h>
 #include <asm/io.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 #define UART_LCRVAL UART_LCR_8N1		/* 8 data, 1 stop, no
parity */
 #define UART_MCRVAL (UART_MCR_DTR | \
 		     UART_MCR_RTS)		/* RTS/DTR */
@@ -92,21 +95,104 @@ void NS16550_putc (NS16550_t com_port, char c)
 }
 
 #ifndef CONFIG_NS16550_MIN_FUNCTIONS
-char NS16550_getc (NS16550_t com_port)
+
+static char NS16550_raw_getc(NS16550_t regs)
 {
-	while ((serial_in(&com_port->lsr) & UART_LSR_DR) == 0) {
+	while ((serial_in(&regs->lsr) & UART_LSR_DR) == 0) {
 #ifdef CONFIG_USB_TTY
 		extern void usbtty_poll(void);
 		usbtty_poll();
 #endif
 		WATCHDOG_RESET();
 	}
-	return serial_in(&com_port->rbr);
+	return serial_in(&regs->rbr);
+}
+
+static int NS16550_raw_tstc(NS16550_t regs)
+{
+	return ((serial_in(&regs->lsr) & UART_LSR_DR) != 0);
+}
+
+
+#ifdef CONFIG_NS16550_BUFFER_READS
+
+#define BUF_SIZE 256
+#define NUM_PORTS 4
+
+struct ns16550_priv {
+	char buf[BUF_SIZE];
+	unsigned int head, tail;
+};
+
+static struct ns16550_priv rxstate[NUM_PORTS];
+
+static void enqueue(unsigned int port, char ch)
+{
+	/* If queue is full, drop the character. */
+	if ((rxstate[port].head - rxstate[port].tail - 1) % BUF_SIZE ==
0)
+		return;
+
+	rxstate[port].buf[rxstate[port].tail] = ch;
+	rxstate[port].tail = (rxstate[port].tail + 1) % BUF_SIZE;
+}
+
+static int dequeue(unsigned int port, char *ch)
+{
+	/* Empty queue? */
+	if (rxstate[port].head == rxstate[port].tail)
+		return 0;
+
+	*ch = rxstate[port].buf[rxstate[port].head];
+	rxstate[port].head = (rxstate[port].head + 1) % BUF_SIZE;
+	return 1;
+}
+
+static void fill_rx_buf(NS16550_t regs, unsigned int port)
+{
+	while ((serial_in(&regs->lsr) & UART_LSR_DR) != 0)
+		enqueue(port, serial_in(&regs->rbr));
+}
+
+char NS16550_getc(NS16550_t regs, unsigned int port)
+{
+	char ch;
+
+	if (port >= NUM_PORTS || !(gd->flags & GD_FLG_RELOC))
+		return NS16550_raw_getc(regs);
+
+	do  {
+#ifdef CONFIG_USB_TTY
+		extern void usbtty_poll(void);
+		usbtty_poll();
+#endif
+		fill_rx_buf(regs, port);
+		WATCHDOG_RESET();
+	} while (!dequeue(port, &ch));
+
+	return ch;
+}
+
+int NS16550_tstc(NS16550_t regs, unsigned int port)
+{
+	if (port >= NUM_PORTS || !(gd->flags & GD_FLG_RELOC))
+		return NS16550_raw_tstc(regs);
+
+	fill_rx_buf(regs, port);
+
+	return rxstate[port].head != rxstate[port].tail;
+}
+
+#else /* CONFIG_NS16550_BUFFER_READS */
+
+char NS16550_getc(NS16550_t regs, unsigned int port)
+{
+	return NS16550_raw_getc(regs);
 }
 
-int NS16550_tstc (NS16550_t com_port)
+int NS16550_tstc(NS16550_t regs, unsigned int port)
 {
-	return ((serial_in(&com_port->lsr) & UART_LSR_DR) != 0);
+	return NS16550_raw_tstc(regs);
 }
 
+#endif /* CONFIG_NS16550_BUFFER_READS */
 #endif /* CONFIG_NS16550_MIN_FUNCTIONS */
diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
index 0d56e78..cf9a1dd 100644
--- a/drivers/serial/serial.c
+++ b/drivers/serial/serial.c
@@ -93,6 +93,7 @@ static NS16550_t serial_ports[4] = {
 };
 
 #define PORT	serial_ports[port-1]
+#define PORTNR	(port-1)
 #if defined(CONFIG_CONS_INDEX)
 #define CONSOLE	(serial_ports[CONFIG_CONS_INDEX-1])
 #endif
@@ -219,13 +220,13 @@ _serial_puts (const char *s,const int port)
 int
 _serial_getc(const int port)
 {
-	return NS16550_getc(PORT);
+	return NS16550_getc(PORT, PORTNR);
 }
 
 int
 _serial_tstc(const int port)
 {
-	return NS16550_tstc(PORT);
+	return NS16550_tstc(PORT, PORTNR);
 }
 
 void
diff --git a/include/ns16550.h b/include/ns16550.h
index 51f1c17..60b0abc 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -164,6 +164,6 @@ typedef volatile struct NS16550 *NS16550_t;
 
 void	NS16550_init   (NS16550_t com_port, int baud_divisor);
 void	NS16550_putc   (NS16550_t com_port, char c);
-char	NS16550_getc   (NS16550_t com_port);
-int	NS16550_tstc   (NS16550_t com_port);
+char NS16550_getc(NS16550_t regs, unsigned int port);
+int NS16550_tstc(NS16550_t regs, unsigned int port);
 void	NS16550_reinit (NS16550_t com_port, int baud_divisor);
-- 
1.7.3.1

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

[U-Boot] How to unsubscribe.

[Mike Frysinger]

On Monday 17 October 2011 00:56:37 Sandeep Kumar wrote:
> How can I unsubscribe from this mailing list.

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

[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

[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"