[U-Boot] [PATCH v5 1/1] NET: Improve TFTP booting performance when CONFIG_USB_KEYBOARD

[Jim Lin]

On Tue, 2013-07-09 at 06:17 +0800, Joe Hershberger wrote:
> Hi Jim and Stephen,
> 
> On Wed, Jul 3, 2013 at 11:01 PM, Jim Lin <jilin at nvidia.com> wrote:
> > TFTP booting is slow when a USB keyboard is installed and
> > CONFIG_USB_KEYBOARD is defined.
> > This fix is to change Ctrl-C polling to every second when NET transfer
> > is running.
> >
> > Signed-off-by: Jim Lin <jilin at nvidia.com>
> > ---
> > Changes in v2:
> >  1. Change configuration name from CONFIG_CTRLC_POLL_MS to CONFIG_CTRLC_POLL_S.
> >  2. New code will be executed only when CONFIG_CTRLC_POLL_S is defined in
> >     configuration header file.
> >  3. Add description in README.console.
> > Changes in v3:
> >  1. Move changes to common/usb_kbd.c and doc/README.usb
> >  2. Rename config setting to CONFIG_USBKB_TESTC_PERIOD.
> >  3. Remove slow response on USB-keyboard input when TFTP boot is not running.
> > Changes in v4:
> >  1. Remove changes in doc/README.usb, common/usb_kbd.c and
> >     CONFIG_USBKB_TESTC_PERIOD
> >  2. Modify net/net.c
> > Changes in v5:
> >  1. Change variable name to ctrlc_t_start.
> >  2. Use two calls of get_timer(0) to get time gap.
> >
> >  net/net.c |   22 ++++++++++++++++++++++
> >  1 files changed, 22 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/net.c b/net/net.c
> > index df94789..ec88b02 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -322,6 +322,11 @@ int NetLoop(enum proto_t protocol)
> >  {
> >         bd_t *bd = gd->bd;
> >         int ret = -1;
> > +#ifdef CONFIG_USB_KEYBOARD
> > +       unsigned long ctrlc_t_start;
> > +       unsigned long ctrlc_t;
> > +       int ctrlc_result;
> > +#endif
> >
> >         NetRestarted = 0;
> >         NetDevExists = 0;
> > @@ -472,7 +477,24 @@ restart:
> >                 /*
> >                  *      Abort if ctrl-c was pressed.
> >                  */
> > +#ifdef CONFIG_USB_KEYBOARD
> 
> It seems this is the result of the USB Keyboard behavior.  Why is it a
> good idea to litter the TFTP code with this unrelated code?  It seems

So far this is the best place to resolve this issue.

> the very same check could be down inside of ctrlc() somewhere that is
> at least console I/O related.  Besides, having it in a common place
> will allow any operation that accesses the keyboard to benefit from
> not hanging up on slow USB stuff.
> 
> It also seems that it should depend on what the actual source of the
> stdin is, not just if you compiled in CONFIG_USB_KEYBOARD support.

This issue only goes with USB keyboard installed and CONFIG_USB_KEYBOARD
defined. Therefore compiled in CONFIG_USB_KEYBOARD support.
Non-usb-keyboard doesn't have such issue.

> Again, something that belongs in the console source.
> 
> > +               /*
> > +                * Reduce ctrl-c checking to 1 second once
> > +                * to improve TFTP boot performance.
> > +                */
> > +               if (ctrlc_t_start > get_timer(0))
> > +                       ctrlc_t_start = get_timer(0);
> > +               ctrlc_t = get_timer(0) - ctrlc_t_start;
> 
> Why is it preferable to do the subtraction yourself instead of letting
> get_timer() do it?  I.e. what compelled did you change this from v4?

As Wolfgang Denk said in another mail, 
"An exception is "arch/arm/cpu/sa1100/timer.c" which does not respect
the "base" argument at all, i. e. which is broken.
".

So this v5 patch uses get_timer(0), like other code did in this file.

> 
> > +               if (ctrlc_t > CONFIG_SYS_HZ) {
> 
> Why is hard-coding it to 1 second a good idea?
>  Is that really how unresponsive it has to be to not
>  significantly impact TFTP boot time?

Do you want me to add a CONFIG setting to have this time adjustable?
I was thinking "1 second" checking on Ctrl-C should be fine while TFT
boot is running.

--
nvpublic