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

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

[Wolfgang Denk]

Dear Simon,

I wrote:

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

Actually I believe that the restriction is even worse - I think that
any commands that consume any significant amount of time will casue
problems, too.  Can you please test what happens when you have, for
example, a "sleep 10" or a "erase all" in the first few lines of the
pasted input ?

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
What can it profit a man to gain the whole world and to come  to  his
property with a gastric ulcer, a blown prostate, and bifocals?
                                     -- John Steinbeck, _Cannery Row_

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

[Scott Wood]

On 10/17/2011 07:53 AM, Wolfgang Denk wrote:
> Dear Simon,
> 
> I wrote:
> 
>> - 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).
> 
> Actually I believe that the restriction is even worse - I think that
> any commands that consume any significant amount of time will casue
> problems, too.  Can you please test what happens when you have, for
> example, a "sleep 10" or a "erase all" in the first few lines of the
> pasted input ?

The reference to "70 lines" was meant as an anecdote about how it made
pasting things significantly more usable for me.  I never advertised it
as a complete fix for serial loss (indeed, it even says that I still
experienced loss after 70 lines).  It's just a fairly simple change that
lessens the problem for me (and apparently others) to the point where
it's no longer a significant nuisance.

-Scott

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

[Wolfgang Denk]

Dear Graeme Russ,

In message <CALButCK0oVLTbYxbTF=y-vXL+3+=09VEJpzUnX1+-0SYJQEQJA@mail.gmail.com> you wrote:
>
> > > So how does kermit/ymodem send the XON after the user has entered the
> > > receive command and we have sent the XOFF after the newline?
> >
> > Upon the first getc() that follows?
> 
> And as there will be no corresponding newline, when do we send XOFF?

Never?  Note that kermit / ymodem / S-record download etc. all don't
have any issues with sendign characters back-to-back at line speed.

Problems happen only with multi-line input, so it is perfectly fine
to handle just that - at the root cause, i. e. when input turns into
multi-line input.

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
Include the success of others in your dreams for your own success.

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

[Graeme Russ]

Hi Wolfgang,

On Oct 24, 2011 4:15 AM, "Wolfgang Denk" <wd@denx.de> wrote:
>
> Dear Graeme Russ,
>
> In message <
CALButCLEg6c30En3N4LjPv1woJjFxwkEkHvQYMoJy8+MgSzqJw@mail.gmail.com> you
wrote:
> >

[snip]

> > > This should not be necessary. Actually the implementation should not
> > > need to know about such special cases.
> >
> > So how does kermit/ymodem send the XON after the user has entered the
> > receive command and we have sent the XOFF after the newline?
>
> Upon the first getc() that follows?

And as there will be no corresponding newline, when do we send XOFF?

Regards,

Graeme

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

[Wolfgang Denk]

Dear Simon Glass,

In message <1318742050-2201-2-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>

I've made up my mind.  The reasons why you want to add such code are
well understood, but the unsolved and unsolvable issues with this
approach are so severe that I don't want to raise false hopes in
innocent users.

Sorry, but I reject this patch.

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
"Nature is very un-American.  Nature never hurries."
- William George Jordan

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

[Wolfgang Denk]

Dear Graeme Russ,

In message <CALButCLEg6c30En3N4LjPv1woJjFxwkEkHvQYMoJy8+MgSzqJw@mail.gmail.com> you wrote:
>
> > It should be sufficient to send XOFF after receiving a newline
> > character.
> 
> And, ergo, we send an XON when entering the readline function

This is probably not sufficient, as some commands take direct input.
I think both getc() and tstc() should check the XON/XOFF state and
send a XON if XOFF was sent before.

> Hmm, should we move readline() into console.c

Makes sense.

> > This should not be necessary. Actually the implementation should not
> > need to know about such special cases.
> 
> So how does kermit/ymodem send the XON after the user has entered the
> receive command and we have sent the XOFF after the newline?

Upon the first getc() that follows?

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
Marriage is the sole cause of divorce.

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

[Graeme Russ]

Hi Wolgang,

On Oct 23, 2011 7:20 PM, "Wolfgang Denk" <wd@denx.de> wrote:
>
> Dear Graeme Russ,
>
> In message <4EA34086.4030101@gmail.com> you wrote:
> >
> > One problem I see with XON/XOFF is that if we don't send XOFF at the
right
> > time, we run the risk of entering a busy loop (any reasonable timeout
delay
> > for example) and loosing input. So in theory, we would need to send XOFF
> > after every getc() ...
>
> That's not true.  I am not aware of any significant delays that take
> place while receiving characters that belong to a single line.  If we
> had any of these, we would lose characters all the time - but we
> don't.
>
> It should be sufficient to send XOFF after receiving a newline
> character.

And, ergo, we send an XON when entering the readline function

Hmm, should we move readline() into console.c

> > Maybe we need disable/enable flow control functions for when we know we
> > will be entering a busy loop the consumes serial input (ymodem and
kermit
> > transfers and readline for example)
>
> This should not be necessary. Actually the implementation should not
> need to know about such special cases.

So how does kermit/ymodem send the XON after the user has entered the
receive command and we have sent the XOFF after the newline?

Regards,

Graeme

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

[Wolfgang Denk]

Dear Graeme Russ,

In message <4EA34086.4030101@gmail.com> you wrote:
> 
> One problem I see with XON/XOFF is that if we don't send XOFF at the right
> time, we run the risk of entering a busy loop (any reasonable timeout delay
> for example) and loosing input. So in theory, we would need to send XOFF
> after every getc() ...

That's not true.  I am not aware of any significant delays that take
place while receiving characters that belong to a single line.  If we
had any of these, we would lose characters all the time - but we
don't.

It should be sufficient to send XOFF after receiving a newline
character.

> Maybe we need disable/enable flow control functions for when we know we
> will be entering a busy loop the consumes serial input (ymodem and kermit
> transfers and readline for example)

This should not be necessary. Actually the implementation should not
need to know about such special cases.

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
Compassion -- that's the one things no machine ever had.  Maybe it's
the one thing that keeps men ahead of them.
	-- McCoy, "The Ultimate Computer", stardate 4731.3

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

[Graeme Russ]

Hi Simon, Albert,

On 22/10/11 19:44, Albert ARIBAUD wrote:
> Le 17/10/2011 18:40, Simon Glass a ?crit :

[snip]

>>> 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 fully agree with this - We should strive to eliminate bugs, not cover
them with more dirt :)

>> 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.
> 
> There *is* a much better solution within the fundamental single tasking 
> design constraint, and it is the one that was invented precisely for 
> this: flow control. Granted, hardware flow control may be impractical, 
> and that's why software flow control was conceived as early as 1963 and 
> is still widely used today and supported by any serial software or 
> driver worth its salt.

Agree - flow control is the way to go

> Well, FWIW, were I the serial code maintainer, I'd NAK this and ask for 
> a XON/XOFF implementation.
> 
>> 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)?
> 
> Software flow control, whatever way it is implemented, is pure SW and 
> hardware independent, so it should be a generic thing. If XON/XOFF input 
> flow control gets implemented (and I believe it should), then it should 
> be done above hardware serial drivers.

One problem I see with XON/XOFF is that if we don't send XOFF at the right
time, we run the risk of entering a busy loop (any reasonable timeout delay
for example) and loosing input. So in theory, we would need to send XOFF
after every getc() and XON before each getc() and then need to delay a bit
to wait for the character (unless tstc returns true in which case we can
just grab the char in the buffer) which seems a bit excessive.

Maybe we need disable/enable flow control functions for when we know we
will be entering a busy loop the consumes serial input (ymodem and kermit
transfers and readline for example)

Regards,

Graeme

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

[Albert ARIBAUD]

Le 17/10/2011 18:40, Simon Glass a ?crit :

>>   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 think you're not seeing the point that while your patch is good enough 
for 'making multiline input less impractical in some cases', it is not 
good enough for 'making multiline input reliable': it adds marginal 
benefits and we have clear evidence that it will never guarantee a 
correct result however you tweak it, because the only way it could would 
be by multi-tasking in some way; otherwise, there can always be some 
U-Boot activity in between input readings that will be long enough to 
cause character loss.

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

I don't think the warning would make the functionality better.

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

There *is* a much better solution within the fundamental single tasking 
design constraint, and it is the one that was invented precisely for 
this: flow control. Granted, hardware flow control may be impractical, 
and that's why software flow control was conceived as early as 1963 and 
is still widely used today and supported by any serial software or 
driver worth its salt.

>> - 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 :-)

Well, FWIW, were I the serial code maintainer, I'd NAK this and ask for 
a XON/XOFF implementation.

> 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)?

Software flow control, whatever way it is implemented, is pure SW and 
hardware independent, so it should be a generic thing. If XON/XOFF input 
flow control gets implemented (and I believe it should), then it should 
be done above hardware serial drivers.

> Regards,
> Simon

Amicalement,
-- 
Albert.

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

[Albert ARIBAUD]

Le 17/10/2011 22:58, Simon Glass a ?crit :
> Hi Wolfgang,
>
> On Mon, Oct 17, 2011 at 1:33 PM, Wolfgang Denk<wd@denx.de>  wrote:
>> Dear Simon Glass,
>>
>> In message<CAPnjgZ1412ezJh3f4Ww16k4Z55kJdgGWj7+vMyK_ot-MzVd70g@mail.gmail.com>  you wrote:
>>>
>>> Can you please tell me which ELDK version this is using? I see that
>>> the 405 board seems to need ppc_4xx which suggests 4.2 rather than 5,
>>> since in 5 the compiler is called powerpc-
>>
>> Right, I was testing this with ELDK 4.2.
>
> I am struggling to repeat this and don't even get the same numbers...

Could be due to two things:

1) both of you do not work from the same exact tree commit, or

2) both of you work in a different base directory

> For your AR405 board, you saw:
>> - 246058   12972   14636  273666   42d02 /work/wd/tmp-ppc/u-boot
>> + 246062   12972   14636  273670   42d06 /work/wd/tmp-ppc/u-boot

> For me:

>   245942	  12964	  14632	 273538	  42c82	ppc/u-boot

I suggest Wolfgang and you compare your trees to eliminate risk 1, and 
make sure you compare .bin files to reduce risk 2.

Amicalement,
-- 
Albert.

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

[Simon Glass]

Hi Wolfgang,

On Mon, Oct 17, 2011 at 1:33 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ1412ezJh3f4Ww16k4Z55kJdgGWj7+vMyK_ot-MzVd70g@mail.gmail.com> you wrote:
>>
>> Can you please tell me which ELDK version this is using? I see that
>> the 405 board seems to need ppc_4xx which suggests 4.2 rather than 5,
>> since in 5 the compiler is called powerpc-
>
> Right, I was testing this with ELDK 4.2.

I am struggling to repeat this and don't even get the same numbers...

For your AR405 board, you saw:
> - 246058   12972   14636  273666   42d02 /work/wd/tmp-ppc/u-boot
> + 246062   12972   14636  273670   42d06 /work/wd/tmp-ppc/u-boot

For me:

ppc_4xx-gcc -v
Reading specs from
/opt/eldk-4.2.4xx/usr/bin/../lib/gcc/powerpc-linux/4.2.2/specs
Target: powerpc-linux
Configured with:
/opt/eldk/build/ppc-2008-04-01/work/usr/src/denx/BUILD/crosstool-0.43/build/gcc-4.2.2-glibc-20070515T2025-eldk/powerpc-linux/gcc-4.2.2/configure
--target=powerpc-linux --host=i686-host_pc-linux-gnu
--prefix=/var/tmp/eldk.UZpAG7/usr/crosstool/gcc-4.2.2-glibc-20070515T2025-eldk/powerpc-linux
--disable-hosted-libstdcxx
--with-headers=/var/tmp/eldk.UZpAG7/usr/crosstool/gcc-4.2.2-glibc-20070515T2025-eldk/powerpc-linux/powerpc-linux/include
--with-local-prefix=/var/tmp/eldk.UZpAG7/usr/crosstool/gcc-4.2.2-glibc-20070515T2025-eldk/powerpc-linux/powerpc-linux
--disable-nls --enable-threads=posix --enable-symvers=gnu
--enable-__cxa_atexit --enable-languages=c,c++,java --enable-shared
--enable-c99 --enable-long-long --without-x
Thread model: posix
gcc version 4.2.2

ARCH=powerpc make O=ppc AR405_config
ARCH=powerpc make O=ppc

without feature (checkout of upstream/master d8fffa05):
$ ppc_4xx-objdump -h ppc/drivers/serial/ns16550.o
...
  7 .text.NS16550_tstc 00000020  00000000  00000000  00000bf4  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  8 .text.NS16550_putc 0000002c  00000000  00000000  00000c14  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  9 .text.NS16550_getc 00000038  00000000  00000000  00000c40  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
 10 .text.NS16550_init 0000007c  00000000  00000000  00000c78  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
 11 .text.NS16550_reinit 00000080  00000000  00000000  00000cf4  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE

   text	   data	    bss	    dec	    hex	filename
 245942	  12964	  14632	 273538	  42c82	ppc/u-boot


with feature but not enabled (with Scott's two patches):
  7 .text.NS16550_putc 0000002c  00000000  00000000  00000fec  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  8 .text.NS16550_getc 00000038  00000000  00000000  00001018  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  9 .text.NS16550_tstc 00000020  00000000  00000000  00001050  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
 10 .text.NS16550_init 0000007c  00000000  00000000  00001070  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
 11 .text.NS16550_reinit 00000080  00000000  00000000  000010ec  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE

size ppc/u-boot
   text	   data	    bss	    dec	    hex	filename
 245942	  12964	  14632	 273538	  42c82	ppc/u-boot

Do you have any suggestions please?

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
> You can observe a lot just by watchin'. ? ? ? ? ? ? ? ? ?- Yogi Berra
>

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ1412ezJh3f4Ww16k4Z55kJdgGWj7+vMyK_ot-MzVd70g@mail.gmail.com> you wrote:
>
> Can you please tell me which ELDK version this is using? I see that
> the 405 board seems to need ppc_4xx which suggests 4.2 rather than 5,
> since in 5 the compiler is called powerpc-

Right, I was testing this with ELDK 4.2.

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
You can observe a lot just by watchin'.                  - Yogi Berra

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

[Simon Glass]

On Mon, Oct 17, 2011 at 4:08 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1318742050-2201-2-git-send-email-sjg@chromium.org> you wrote:
> ...
>> 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.
>
> One problem I see is that the code size on some boards grows even if
> the new option is not enabled. ?Here a compare before and after
> applying your patches (without any changes to the respective board
> configurations):
>
> ?Configuring for AR405 board...
> ? ?text ? ?data ? ? bss ? ? dec ? ? hex filename
> - 246058 ? 12972 ? 14636 ?273666 ? 42d02 /work/wd/tmp-ppc/u-boot
> + 246062 ? 12972 ? 14636 ?273670 ? 42d06 /work/wd/tmp-ppc/u-boot
> ?Configuring for CANBT board...
> ? ?text ? ?data ? ? bss ? ? dec ? ? hex filename
> - 120710 ? ?8604 ? ?3876 ?133190 ? 20846 /work/wd/tmp-ppc/u-boot
> + 120714 ? ?8604 ? ?3876 ?133194 ? 2084a /work/wd/tmp-ppc/u-boot
> ?Configuring for pcs440ep board...
> ? ?text ? ?data ? ? bss ? ? dec ? ? hex filename
> - 296191 ? 19636 ?345088 ?660915 ? a15b3 /work/wd/tmp-ppc/u-boot
> + 296195 ? 19636 ?345088 ?660919 ? a15b7 /work/wd/tmp-ppc/u-boot
> ...

Hi Wolfgang,

Can you please tell me which ELDK version this is using? I see that
the 405 board seems to need ppc_4xx which suggests 4.2 rather than 5,
since in 5 the compiler is called powerpc-

But I might be confused.

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
> "Out of register space (ugh)"
> - vi
>

[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@denx.de> wrote:
> Dear Simon,
>
> In message <1318742050-2201-2-git-send-email-sjg@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
>

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

[Simon Glass]

Hi Wolfgang,

On Mon, Oct 17, 2011 at 4:08 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1318742050-2201-2-git-send-email-sjg@chromium.org> you wrote:
> ...
>> 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.
>
> One problem I see is that the code size on some boards grows even if
> the new option is not enabled. ?Here a compare before and after
> applying your patches (without any changes to the respective board
> configurations):
>
> ?Configuring for AR405 board...
> ? ?text ? ?data ? ? bss ? ? dec ? ? hex filename
> - 246058 ? 12972 ? 14636 ?273666 ? 42d02 /work/wd/tmp-ppc/u-boot
> + 246062 ? 12972 ? 14636 ?273670 ? 42d06 /work/wd/tmp-ppc/u-boot
> ?Configuring for CANBT board...
> ? ?text ? ?data ? ? bss ? ? dec ? ? hex filename
> - 120710 ? ?8604 ? ?3876 ?133190 ? 20846 /work/wd/tmp-ppc/u-boot
> + 120714 ? ?8604 ? ?3876 ?133194 ? 2084a /work/wd/tmp-ppc/u-boot
> ?Configuring for pcs440ep board...
> ? ?text ? ?data ? ? bss ? ? dec ? ? hex filename
> - 296191 ? 19636 ?345088 ?660915 ? a15b3 /work/wd/tmp-ppc/u-boot
> + 296195 ? 19636 ?345088 ?660919 ? a15b7 /work/wd/tmp-ppc/u-boot
> ...
>

This doesn't happen for me on ARM - the compiler just inlines the new
static raw functions into the public functions. I will see if I can
figure out what is happening on PPC. It is 4 bytes, or one
instruction, right?

The fix might be to duplicate more code, or perhaps add an inline
keyword to the two functions.

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
> "Out of register space (ugh)"
> - vi
>

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

[Wolfgang Denk]

Dear Simon,

In message <1318742050-2201-2-git-send-email-sjg@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.

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


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.

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


I have to admit that I'm at a loss with a decision 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
"We don't have to protect the environment -- the Second Coming is  at
hand."                                                   - James Watt

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

[Wolfgang Denk]

Dear Simon Glass,

In message <1318742050-2201-2-git-send-email-sjg@chromium.org> you wrote:
...
> 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.

One problem I see is that the code size on some boards grows even if
the new option is not enabled.  Here a compare before and after
applying your patches (without any changes to the respective board
configurations):

 Configuring for AR405 board...
    text    data     bss     dec     hex filename
- 246058   12972   14636  273666   42d02 /work/wd/tmp-ppc/u-boot
+ 246062   12972   14636  273670   42d06 /work/wd/tmp-ppc/u-boot
 Configuring for CANBT board...
    text    data     bss     dec     hex filename
- 120710    8604    3876  133190   20846 /work/wd/tmp-ppc/u-boot
+ 120714    8604    3876  133194   2084a /work/wd/tmp-ppc/u-boot
 Configuring for pcs440ep board...
    text    data     bss     dec     hex filename
- 296191   19636  345088  660915   a15b3 /work/wd/tmp-ppc/u-boot
+ 296195   19636  345088  660919   a15b7 /work/wd/tmp-ppc/u-boot
...

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
"Out of register space (ugh)"
- vi

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ1VUF-HFbDX9byuV32sJfEE3SKRiT124gfgQOm0f+QSEw@mail.gmail.com> you wrote:
> 
> In a similar way, the Linux kernel has a fatal flaw. Serial data
> coming into the flip buffers under extreme interrupt load can be lost,
> or the secondary buffers can become exhausted, or user space cannot
> keep up with input under heavy load, etc., etc. This is the reality of
> the world. As each problem presents itself we direct our attention to
> it.

This is not a fatal flaw.  There is nothing in the definition of RS232
or the serialinterface that guarantees you an error free data stream -
not on that protocol level.  You are facing loss of Ethernet packages
on the network as well.

Problems start only when you make wrong assumptions.


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
A girl with a future avoids the man with a past.
                                   -- Evan Esar, "The Humor of Humor"

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

[Simon Glass]

Hi Wolfgang,

On Sun, Oct 16, 2011 at 1:13 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ2Q3XtGK=5HhF0KhQo4XQDA6CAH3QFKfmgCwzNV_y0mUA@mail.gmail.com> you wrote:
>>
>> > Hmm... What about the conversation around V2 of the patch re: using XOFF/XON
>> > to control input flow? IIUC, even this V4 patch would not help much if there
>> > is a command within the pasted code that sends a lot of output, right?
>>
>> Well it will help so long as there isn't much more input. Perhaps the
>
> Don't you see that this "as long as there isn't much more input" is
> the fatal flaw of your patch? ?You are not fixing the problem, you are
> just extending the point wher eit hits to some extend - it may work
> for a few more situations, but it's still guaranteed to fail for
> others.

In a similar way, the Linux kernel has a fatal flaw. Serial data
coming into the flip buffers under extreme interrupt load can be lost,
or the secondary buffers can become exhausted, or user space cannot
keep up with input under heavy load, etc., etc. This is the reality of
the world. As each problem presents itself we direct our attention to
it.

>
>> To address your question, I think we did discuss it. Wolfgang felt
>> that it should be as simple as sending XOFF when receiving the end of
>> a line and XON when waiting for characters on the new line. It
>> certainly sounds plausible.
>
> Then please let's take this approach instead, if you really need to
> support such a mode of operation.

As I said I will look at this.

>
>> I have other reasons for favouring this patch. On the main hardware
>> platform I currently use, SPI and the console UART are multiplexed, so
>> before using SPI (e.g. saveenv, reading the environment) we must read
>> in any pending characters to avoid corruption. After re-enabling the
>> UART later we must clear out any junk that has arrived. This patch
>> provides a way around that which XON/XOFF does not (since when you
>> send XOFF you may already have input characters outstanding - junk
>> will later be added and you can't tell the difference).
>
> That's an unrelated hardware issue that needs to be addressed
> independently.

Yes it is. I can't help wondering what the solution might be though,
if we are not allowed to buffer things.

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
> Most legends have their basis in facts.
> ? ? ? ?-- Kirk, "And The Children Shall Lead", stardate 5029.5
>

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ2Q3XtGK=5HhF0KhQo4XQDA6CAH3QFKfmgCwzNV_y0mUA@mail.gmail.com> you wrote:
> 
> > Hmm... What about the conversation around V2 of the patch re: using XOFF/XON
> > to control input flow? IIUC, even this V4 patch would not help much if there
> > is a command within the pasted code that sends a lot of output, right?
>
> Well it will help so long as there isn't much more input. Perhaps the

Don't you see that this "as long as there isn't much more input" is
the fatal flaw of your patch?  You are not fixing the problem, you are
just extending the point wher eit hits to some extend - it may work
for a few more situations, but it's still guaranteed to fail for
others.

> To address your question, I think we did discuss it. Wolfgang felt
> that it should be as simple as sending XOFF when receiving the end of
> a line and XON when waiting for characters on the new line. It
> certainly sounds plausible.

Then please let's take this approach instead, if you really need to
support such a mode of operation.

> I have other reasons for favouring this patch. On the main hardware
> platform I currently use, SPI and the console UART are multiplexed, so
> before using SPI (e.g. saveenv, reading the environment) we must read
> in any pending characters to avoid corruption. After re-enabling the
> UART later we must clear out any junk that has arrived. This patch
> provides a way around that which XON/XOFF does not (since when you
> send XOFF you may already have input characters outstanding - junk
> will later be added and you can't tell the difference).

That's an unrelated hardware issue that needs to be addressed
independently.

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
Most legends have their basis in facts.
	-- Kirk, "And The Children Shall Lead", stardate 5029.5

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

[Simon Glass]

Hi Albert,

On Sun, Oct 16, 2011 at 5:57 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Simon,
>
> Le 16/10/2011 07:14, Simon Glass a ?crit :
>>
>> 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
>>
>> Changes in v4:
>> - Change config option to CONFIG_NS16550_RBUF_SIZE
>> - Add additional checkpatch-cleanup patch
>
> Hmm... What about the conversation around V2 of the patch re: using XOFF/XON
> to control input flow? IIUC, even this V4 patch would not help much if there
> is a command within the pasted code that sends a lot of output, right?

Well it will help so long as there isn't much more input. Perhaps the
way to think of it is that every character you output that isn't just
an echo, puts you one more character behind the input. This adds one
character to the buffer in this patch. When you have fallen (say) 256
characters behind the input, then you will start losing characters
even with this patch. However it is rare to paste in commands which
send lots of output - and 3 lines of full output is pretty uncommon in
my experience.

I feel that relying on the first level UART FIFO as the only buffering
is brave, and cannot be justified by lack of interrupts alone. There
seems to me to be no fundamental reason why U-Boot should not buffer
its input for when it is latest ready to process it. Granted this
should perhaps be higher up the software stack, but until someone
tidies up serial a little I suspect that would be a pain to implement.
With a more generic serial layer (a device pointer and a ->priv
pointer across all drivers for example) we could attach this buffering
as a separate file to be enabled / disabled by a Makefile.

To address your question, I think we did discuss it. Wolfgang felt
that it should be as simple as sending XOFF when receiving the end of
a line and XON when waiting for characters on the new line. It
certainly sounds plausible.

I have other reasons for favouring this patch. On the main hardware
platform I currently use, SPI and the console UART are multiplexed, so
before using SPI (e.g. saveenv, reading the environment) we must read
in any pending characters to avoid corruption. After re-enabling the
UART later we must clear out any junk that has arrived. This patch
provides a way around that which XON/XOFF does not (since when you
send XOFF you may already have input characters outstanding - junk
will later be added and you can't tell the difference).

I am unsure whether to bring this up or not - clearly this platform is
not ideal, but it is in very common use and we need to support it with
U-Boot.

Regards,
Simon

>
> Amicalement,
> --
> Albert.
>

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

[Albert ARIBAUD]

Hi Simon,

Le 16/10/2011 07:14, Simon Glass a ?crit :
> 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
>
> Changes in v4:
> - Change config option to CONFIG_NS16550_RBUF_SIZE
> - Add additional checkpatch-cleanup patch

Hmm... What about the conversation around V2 of the patch re: using 
XOFF/XON to control input flow? IIUC, even this V4 patch would not help 
much if there is a command within the pasted code that sends a lot of 
output, right?

Amicalement,
-- 
Albert.

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

[Simon Glass]

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

Changes in v4:
- Change config option to CONFIG_NS16550_RBUF_SIZE
- Add additional checkpatch-cleanup patch

 README                   |   12 ++++++
 drivers/serial/ns16550.c |   97 +++++++++++++++++++++++++++++++++++++++++++--
 drivers/serial/serial.c  |    5 +-
 include/ns16550.h        |    4 +-
 4 files changed, 109 insertions(+), 9 deletions(-)

diff --git a/README b/README
index 7e032a9..d8b4c4d 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_RBUF_SIZE:
+		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_RBUF_SIZE to
+		the size of the buffer you want (256 is a reasonable value).
+
+
 Low Level (hardware related) configuration options:
 ---------------------------------------------------
 
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 056c25d..12de014 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -4,12 +4,15 @@
  * modified to use CONFIG_SYS_ISA_MEM and new defines
  */
 
+#include <common.h>
 #include <config.h>
 #include <ns16550.h>
 #include <watchdog.h>
 #include <linux/types.h>
 #include <asm/io.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 #define UART_LCRVAL UART_LCR_8N1		/* 8 data, 1 stop, no parity */
 #define UART_MCRVAL (UART_MCR_DTR | \
 		     UART_MCR_RTS)		/* RTS/DTR */
@@ -95,21 +98,105 @@ void NS16550_putc(NS16550_t com_port, char c)
 }
 
 #ifndef CONFIG_NS16550_MIN_FUNCTIONS
-char NS16550_getc(NS16550_t com_port)
+static char NS16550_raw_getc(NS16550_t regs)
 {
-	while ((serial_in(&com_port->lsr) & UART_LSR_DR) == 0) {
+	while ((serial_in(&regs->lsr) & UART_LSR_DR) == 0) {
 #ifdef CONFIG_USB_TTY
 		extern void usbtty_poll(void);
 		usbtty_poll();
 #endif
 		WATCHDOG_RESET();
 	}
-	return serial_in(&com_port->rbr);
+	return serial_in(&regs->rbr);
+}
+
+static int NS16550_raw_tstc(NS16550_t regs)
+{
+	return ((serial_in(&regs->lsr) & UART_LSR_DR) != 0);
+}
+
+
+#ifdef CONFIG_NS16550_RBUF_SIZE
+
+#define NUM_PORTS 4
+
+struct ns16550_priv {
+	char buf[CONFIG_NS16550_RBUF_SIZE];
+	unsigned int head, tail;
+};
+
+static struct ns16550_priv rxstate[NUM_PORTS];
+
+static void enqueue(unsigned int port, char ch)
+{
+	/* If queue is full, drop the character. */
+	if ((rxstate[port].head - rxstate[port].tail - 1) %
+			CONFIG_NS16550_RBUF_SIZE == 0)
+		return;
+
+	rxstate[port].buf[rxstate[port].tail] = ch;
+	rxstate[port].tail = (rxstate[port].tail + 1) %
+		CONFIG_NS16550_RBUF_SIZE;
+}
+
+static int dequeue(unsigned int port, char *ch)
+{
+	/* Empty queue? */
+	if (rxstate[port].head == rxstate[port].tail)
+		return 0;
+
+	*ch = rxstate[port].buf[rxstate[port].head];
+	rxstate[port].head = (rxstate[port].head + 1) %
+		CONFIG_NS16550_RBUF_SIZE;
+	return 1;
+}
+
+static void fill_rx_buf(NS16550_t regs, unsigned int port)
+{
+	while ((serial_in(&regs->lsr) & UART_LSR_DR) != 0)
+		enqueue(port, serial_in(&regs->rbr));
+}
+
+char NS16550_getc(NS16550_t regs, unsigned int port)
+{
+	char ch;
+
+	if (port >= NUM_PORTS || !(gd->flags & GD_FLG_RELOC))
+		return NS16550_raw_getc(regs);
+
+	do  {
+#ifdef CONFIG_USB_TTY
+		extern void usbtty_poll(void);
+		usbtty_poll();
+#endif
+		fill_rx_buf(regs, port);
+		WATCHDOG_RESET();
+	} while (!dequeue(port, &ch));
+
+	return ch;
+}
+
+int NS16550_tstc(NS16550_t regs, unsigned int port)
+{
+	if (port >= NUM_PORTS || !(gd->flags & GD_FLG_RELOC))
+		return NS16550_raw_tstc(regs);
+
+	fill_rx_buf(regs, port);
+
+	return rxstate[port].head != rxstate[port].tail;
+}
+
+#else /* CONFIG_NS16550_RBUF_SIZE */
+
+char NS16550_getc(NS16550_t regs, unsigned int port)
+{
+	return NS16550_raw_getc(regs);
 }
 
-int NS16550_tstc(NS16550_t com_port)
+int NS16550_tstc(NS16550_t regs, unsigned int port)
 {
-	return (serial_in(&com_port->lsr) & UART_LSR_DR) != 0;
+	return NS16550_raw_tstc(regs);
 }
 
+#endif /* CONFIG_NS16550_RBUF_SIZE */
 #endif /* CONFIG_NS16550_MIN_FUNCTIONS */
diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
index 0d56e78..cf9a1dd 100644
--- a/drivers/serial/serial.c
+++ b/drivers/serial/serial.c
@@ -93,6 +93,7 @@ static NS16550_t serial_ports[4] = {
 };
 
 #define PORT	serial_ports[port-1]
+#define PORTNR	(port-1)
 #if defined(CONFIG_CONS_INDEX)
 #define CONSOLE	(serial_ports[CONFIG_CONS_INDEX-1])
 #endif
@@ -219,13 +220,13 @@ _serial_puts (const char *s,const int port)
 int
 _serial_getc(const int port)
 {
-	return NS16550_getc(PORT);
+	return NS16550_getc(PORT, PORTNR);
 }
 
 int
 _serial_tstc(const int port)
 {
-	return NS16550_tstc(PORT);
+	return NS16550_tstc(PORT, PORTNR);
 }
 
 void
diff --git a/include/ns16550.h b/include/ns16550.h
index e9d2eda..ed77722 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -164,6 +164,6 @@ typedef struct NS16550 *NS16550_t;
 
 void NS16550_init(NS16550_t com_port, int baud_divisor);
 void NS16550_putc(NS16550_t com_port, char c);
-char NS16550_getc(NS16550_t com_port);
-int NS16550_tstc(NS16550_t com_port);
+char NS16550_getc(NS16550_t regs, unsigned int port);
+int NS16550_tstc(NS16550_t regs, unsigned int port);
 void NS16550_reinit(NS16550_t com_port, int baud_divisor);
-- 
1.7.3.1