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

[Simon Glass]

Hi Wolfgang,

On Mon, Oct 17, 2011 at 5:18 AM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon,
>
> In message <1318742050-2201-2-git-send-email-sjg at chromium.org> you wrote:
>>
>> 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.
>
> Let me try to summarize my thinking:
>
> - It is a fundamental design decision that U-Boot is single tasking.
>  This implies that while a command is running, no other things get
>  done in parallel. This includes communication over the serial line,
>  USB, Ethernet, ...
>
> - That means that by design U-Boot does not support multi-line input:
>  as soon as you hit "enter" U-Boot will start executing your command,
>  and will only become ready for new input when it has completed the
>  execution of the command(s), i. e. after printing the next prompt.
>
> - For this suported mode of operation your patch is not needed. It
>  just adds code size and complexity.
>
> - Your description "rapid input" is actually wrong.  The speed of
>  input over the serial line is limited by the baud rate settings,
>  and even if you transfer at maximum speed all supported systems
>  are fast enough to receive the data without loss.
>
> - The use case you are trying to support is actually multi-line
>  input, so you should describe it as such.  I agree that this would
>  be an interesting feature for some use cases, but if we go on and
>  implement it, we should (1) document it properly and (2) implement
>  it in a way that works reliably.

I can certainly improve the documentation and commit message.

>
> - However, your implementation does not result in reliable multi-line
>  input.  It works only in a few specific use cases, and will fail
>  silently for others.  I don't see a way ho you would announce this
>  new feature.  The numbers you mention in the commit message ("it
>  handles around 70 lines before lossage") apply for this specific
>  board only, and for your use case only (pasting of short lines that
>  produce no output).
>
>  So essentially you are saying: hey, we now have multi-line input,
>  but it works only a bit.  It will fail sooner or later, without
>  error messages, and I cannot even tell you what the limits for your
>  system are.  And it depends on which input you send.
>
>  This does not exactly sound promising.

That all sounds reasonable.

I think it is good enough, but not perfect. Due to the fundamental
design decision you mention at the top, we cannot squirrel away serial
input in the background. The best we can do is squirrel it away in the
foreground, as we output new serial data (I suppose we could squirrel
it away in net loops and other places but I don't want to go there!).
This turns out to be mostly good enough, because it is rare for people
to want to paste 'sleep 10' and the like into their terminal (your
other message).

I would like to spit out a warning when serial input is lost - as I
mentioned that could be combined with a serial overhaul at some point.

>
>
> On the other hand, we also have the rule that things that are useful
> to some and that don't hurt others should be allowed to go in.
>
> What makes me hesitate are two things:
>
> - The patch promises a feature (multi-line input), but fails to
>  provide a reliably working implementation.

I *think* it does the best possible within the fundamental design
decision constraint. If there is more it can do, please let me have
your ideas. I don't believe buffering conflicts with the constraint -
they are separate things. But yes in systems with interrupts normally
the input buffer is filled in the background and drained in the
foreground.

>
> - As it turns out, the patch increases code size even for boards that
>  do not activate this feature.

Yes, I will take a look at this problem.

>
>
> I have to admit that I'm at a loss with a decision here.

Well it's not easy being a maintainer :-) In any case this patch is
not the end of the story as serial needs some work - another objection
you didn't mention above is that this function lives in only one
driver. Is that a good thing (hide it away) or a bad thing (all
drivers should support it and the implementation should move up a
level)?

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
> "We don't have to protect the environment -- the Second Coming is  at
> hand."                                                   - James Watt
>