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

[Scott Wood <scottwood@freescale.com>]

On Wed, 6 Apr 2011 22:42:12 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear Scott Wood,
> 
> In message <20110406203012.GA30167@schlenkerla.am.freescale.net> you wrote:
> > This improves the performance of U-Boot when accepting rapid input,
> > such as pasting a sequence of commands.
> ...
> > +static struct ns16550_priv rxstate[NUM_PORTS];
> 
> Do we really need to statically allocate these buffers for all
> configured serial ports?  Actual I/O will always be done to a single
> port only, so eventually only one such buffer will ever be used?

If we use just one buffer, then switching ports could have some latency, in
terms of some queued data after the switch command being used from the old
port -- unless we add code to flush the queue when switching.  It probably
won't matter much in practice, though, unless you do something like:

"setenv stdout=...; something else" all at once.

And it wouldn't matter at all for boards without CONFIG_SERIAL_MULTI --
except for linkstation, which does some access to a secondary serial port
(see board/linkstation/avr.c).

> > +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;
> 
> Is it really wise to silentrly drop characters here?

It's what currently happens, except they're silently dropped by the
hardware instead (and that's still possible with this patch, it just
requires a longer time between getc/tstc calls).

It's also what happens with usbtty (in lib/circbuf.c), though it drops old
characters rather than new (both options seem about equally damaging).

In case you're wondering, I didn't use lib/circbuf.c here as it would have
been larger and more complicated (requiring dynamic port initialization).

> Maybe we should stop reading from the device, 

That would hang the U-Boot console, for what is usually a temporary
problem.

> and/or issue some error message?

Would probably exacerbate the problem by slowing things down further,
would need to be ratelimited to avoid scrolling all useful stuff off the
screen, and it would behave inconsistently (only happenning if the dropping
doesn't happen in hardware).

> What is the increase of the memory footprint (flash and RAM) with
> this patch, with CONFIG_NS16550_BUFFER_READS enabled and not enabled?

Currently, looks like up to 16 bytes with it not enabled -- due to not
ifdeffing the change to pass an extra argument to the public NS16550
functions.  This would go away if we just use a single queue.  If we don't
do that, I think we should ifdef the function signature change as well --
not so much for the size savings, as that it appears that some boards use
or define these functions themselves (alternatively, I could try to fix up
those boards).

With it enabled, on ppc it's an extra 396 bytes of image size, and 1056
bytes of BSS.

-Scott