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

[Simon Glass]

Hi Wolfgang,

On Sat, Oct 15, 2011 at 12:08 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon Glass,
>
> In message <1318694632-21872-1-git-send-email-sjg at chromium.org> you wrote:
>> From: Scott Wood <scottwood at freescale.com>
>>
>> From: Scott Wood <scottwood at 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 at chromium.org> - ARM code size goes from
>> 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes.
>>
>> Signed-off-by: Scott Wood <scottwood at 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.

Yes much better.

>
>> +#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).

Yes, this makes more sense with the new name, too.

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