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