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

[Wolfgang Denk <wd@denx.de>]

Dear Simon Glass,

In message <1318694632-21872-1-git-send-email-sjg@chromium.org> you wrote:
> From: Scott Wood <scottwood@freescale.com>
> 
> From: Scott Wood <scottwood@freescale.com>
> 
> This improves the performance of U-Boot when accepting rapid input,
> such as pasting a sequence of commands.
> 
> Without this patch, on P4080DS I see a maximum of around 5 lines can
> be pasted.  With this patch, it handles around 70 lines before lossage,
> long enough for most things you'd paste.
> 
> With it enabled, on ppc it's an extra 396 bytes of image size, and 1056
> bytes of BSS.
> 
> ARM note from Simon Glass <sjg@chromium.org> - ARM code size goes from
> 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> Changes in v2:
> - Fix checkpatch warnings, the other one was already there
> 
> Changes in v3:
> - Use CONFIG_NS16550_BUFFER_READS to set the buffer size also
> 
>  README                   |   12 ++++++
>  drivers/serial/ns16550.c |   96 +++++++++++++++++++++++++++++++++++++++++++--
>  drivers/serial/serial.c  |    5 +-
>  include/ns16550.h        |    4 +-
>  4 files changed, 108 insertions(+), 9 deletions(-)
> 
> diff --git a/README b/README
> index 7e032a9..1150d6c 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_BUFFER_READS:
> +		Instead of reading directly from the receive register
> +		every time U-Boot is ready for another byte, keep a
> +		buffer and fill it from the hardware fifo every time
> +		U-Boot reads a character.  This helps U-Boot keep up with
> +		a larger amount of rapid input, such as happens when
> +		pasting text into the terminal.
> +
> +		To use this option, define CONFIG_NS16550_BUFFER_READS to
> +		the size of the buffer you want (256 is a reasonable value).

If we should accept that patch, then please chose a better name here.
For example, CONFIG_NS16550_RBUF_SIZE (or similar) seems to be a
somewhat better choice.

> +#ifdef CONFIG_NS16550_BUFFER_READS
> +
> +#define BUF_SIZE CONFIG_NS16550_BUFFER_READS
> +#define NUM_PORTS 4
> +
> +struct ns16550_priv {
> +	char buf[BUF_SIZE];
> +	unsigned int head, tail;
> +};

There is little sense in adding another variable here.  Just write:

	char buf[CONFIG_NS16550_BUFFER_READS];

(or whatever variable name would be used here).


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The  release  of  emotion  is  what  keeps  us  health.  Emotionally
healthy."
"That may be, Doctor. However, I have noted that the healthy  release
of emotion is frequently unhealthy for those closest to you."
	-- McCoy and Spock, "Plato's Stepchildren", stardate 5784.3