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

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

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.