[U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance

[U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance

[Jim Lin]

TFTP booting is slow when a USB keyboard is installed and
stdin has usbkbd added.
This fix is to change Ctrl-C polling for USB keyboard to every second
when NET transfer is running.

Signed-off-by: Jim Lin <jilin@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.
Changes in v6:
 1. In common/usb_kbd.c, check net_busy_flag to determine whether we poll
    USB keyboard status.
 2. In include/usb.h, add external variable declaration net_busy_flag


 common/usb_kbd.c |   15 +++++++++++++++
 include/usb.h    |    2 +-
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 3174b5e..3288c69 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -121,6 +121,9 @@ struct usb_kbd_pdata {
 	uint8_t		flags;
 };
 
+/* The period of time between two calls of usb_kbd_testc(). */
+static unsigned long kbd_testc_tms;
+
 /* Generic keyboard event polling. */
 void usb_kbd_generic_poll(void)
 {
@@ -366,6 +369,18 @@ static int usb_kbd_testc(void)
 	struct usb_device *usb_kbd_dev;
 	struct usb_kbd_pdata *data;
 
+	/*
+	 * If net_busy_flag is 1, NET transfer is running,
+	 * then we check key pressed every second to improve
+	 * TFTP booting performance.
+	 */
+	if (net_busy_flag) {
+		if (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ)
+			return 0;
+		else
+			kbd_testc_tms = get_timer(0);
+	}
+
 	dev = stdio_get_by_name(DEVNAME);
 	usb_kbd_dev = (struct usb_device *)dev->priv;
 	data = usb_kbd_dev->privptr;
diff --git a/include/usb.h b/include/usb.h
index d7b082d..824b394 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -206,7 +206,7 @@ int usb_host_eth_scan(int mode);
 
 int drv_usb_kbd_init(void);
 int usb_kbd_deregister(void);
-
+extern int net_busy_flag;
 #endif
 /* routines */
 int usb_init(void); /* initialize the USB Controller */
-- 
1.7.7

[U-Boot] [PATCH v6 2/2] NET: Add net_busy_flag

[Jim Lin]

This flag is to have console aware that NET transfer is running or not.

Signed-off-by: Jim Lin <jilin@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.
Changes in v6:
 1. Add net_busy_flag to make console aware NET transfer is running or not.

 net/net.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net/net.c b/net/net.c
index df94789..76d8ea9 100644
--- a/net/net.c
+++ b/net/net.c
@@ -207,6 +207,8 @@ static int net_check_prereq(enum proto_t protocol);
 
 static int NetTryCount;
 
+int net_busy_flag;
+
 /**********************************************************************/
 
 static int on_bootfile(const char *name, const char *value, enum env_op op,
@@ -341,6 +343,7 @@ int NetLoop(enum proto_t protocol)
 		eth_init_state_only(bd);
 
 restart:
+	net_busy_flag = 0;
 	net_set_state(NETLOOP_CONTINUE);
 
 	/*
@@ -454,6 +457,8 @@ restart:
 #endif /* CONFIG_SYS_FAULT_ECHO_LINK_DOWN, ... */
 #endif /* CONFIG_MII, ... */
 
+	net_busy_flag = 1;
+
 	/*
 	 *	Main packet reception loop.  Loop receiving packets until
 	 *	someone sets `net_state' to a state that terminates.
@@ -558,6 +563,7 @@ restart:
 	}
 
 done:
+	net_busy_flag = 0;
 #ifdef CONFIG_CMD_TFTPPUT
 	/* Clear out the handlers */
 	net_set_udp_handler(NULL);
-- 
1.7.7

[U-Boot] [PATCH v6 2/2] NET: Add net_busy_flag

[Stephen Warren]

On 07/18/2013 08:15 AM, Jim Lin wrote:
> This flag is to have console aware that NET transfer is running or not.

I actually wonder if the shell interpreter or command-invocation code
(to cover automatic bootcmd?) shouldn't set this flag before invoking
every command, and clear it afterward? Or perhaps, a flag should be
added to commands that would benefit from this flag being set?

But this patch is probably a reasonable way to implement it in the first
place, so we don't add complex infra-structure if it turns out that
nothing else is going to require this feature.

[U-Boot] [PATCH v6 2/2] NET: Add net_busy_flag

[Marek Vasut]

Dear Stephen Warren,

> On 07/18/2013 08:15 AM, Jim Lin wrote:
> > This flag is to have console aware that NET transfer is running or not.
> 
> I actually wonder if the shell interpreter or command-invocation code
> (to cover automatic bootcmd?) shouldn't set this flag before invoking
> every command, and clear it afterward? Or perhaps, a flag should be
> added to commands that would benefit from this flag being set?

Not a bad idea, but then this might be also needed for USB if the transfer is 
long etc etc
 
> But this patch is probably a reasonable way to implement it in the first
> place, so we don't add complex infra-structure if it turns out that
> nothing else is going to require this feature.

Best regards,
Marek Vasut

[U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance

[Marek Vasut]

Dear Jim Lin,

> TFTP booting is slow when a USB keyboard is installed and
> stdin has usbkbd added.
> This fix is to change Ctrl-C polling for USB keyboard to every second
> when NET transfer is running.
> 
> Signed-off-by: Jim Lin <jilin@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.
> Changes in v6:
>  1. In common/usb_kbd.c, check net_busy_flag to determine whether we poll
>     USB keyboard status.
>  2. In include/usb.h, add external variable declaration net_busy_flag
> 
> 
>  common/usb_kbd.c |   15 +++++++++++++++
>  include/usb.h    |    2 +-
>  2 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 3174b5e..3288c69 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -121,6 +121,9 @@ struct usb_kbd_pdata {
>  	uint8_t		flags;
>  };
> 
> +/* The period of time between two calls of usb_kbd_testc(). */
> +static unsigned long kbd_testc_tms;
> +
>  /* Generic keyboard event polling. */
>  void usb_kbd_generic_poll(void)
>  {
> @@ -366,6 +369,18 @@ static int usb_kbd_testc(void)
>  	struct usb_device *usb_kbd_dev;
>  	struct usb_kbd_pdata *data;
> 
> +	/*
> +	 * If net_busy_flag is 1, NET transfer is running,
> +	 * then we check key pressed every second to improve
> +	 * TFTP booting performance.
> +	 */
> +	if (net_busy_flag) {

THis variable is not declared so this patch won't compile

> +		if (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ)
> +			return 0;
> +		else
> +			kbd_testc_tms = get_timer(0);
> +	}
> +
>  	dev = stdio_get_by_name(DEVNAME);
>  	usb_kbd_dev = (struct usb_device *)dev->priv;
>  	data = usb_kbd_dev->privptr;
> diff --git a/include/usb.h b/include/usb.h
> index d7b082d..824b394 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -206,7 +206,7 @@ int usb_host_eth_scan(int mode);
> 
>  int drv_usb_kbd_init(void);
>  int usb_kbd_deregister(void);
> -
> +extern int net_busy_flag;
>  #endif
>  /* routines */
>  int usb_init(void); /* initialize the USB Controller */

Best regards,
Marek Vasut

[U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance

[Stephen Warren]

On 07/18/2013 08:15 AM, Jim Lin wrote:
> TFTP booting is slow when a USB keyboard is installed and
> stdin has usbkbd added.
> This fix is to change Ctrl-C polling for USB keyboard to every second
> when NET transfer is running.

I think this general approach is a reasonable compromise to the problem.

Some issues need to be considered:

1) git bisect is broken as Marek pointed out.

2) Does the code still compile/link if USB keyboard and/or networking
support is not enabled? I suspect it won't in the case where USB
keyboard is enabled, yet networking support isn't. This probably needs
to be solved by putting "int net_busy_flag;" into some common
non-optional file so that all the users of that variable don't have to
be chronically ifdef'd for all the possible feature combinations.

3) Is "net_busy_flag" the best name for the variable? Perhaps the flag
could be used to disable other time-consuming polling beyond just USB
keyboard CTRL-C handling. Perhaps other operations besides networking
transfers could benefit from setting this flag. At the risk of
bike-shedding, how about renaming this one of:

reduce_non_critical_polling_frequency
non_interactive_operation_in_progress

?

A comment on the code below:

> diff --git a/common/usb_kbd.c b/common/usb_kbd.c

> @@ -366,6 +369,18 @@ static int usb_kbd_testc(void)
>  	struct usb_device *usb_kbd_dev;
>  	struct usb_kbd_pdata *data;
>  
> +	/*
> +	 * If net_busy_flag is 1, NET transfer is running,
> +	 * then we check key pressed every second to improve
> +	 * TFTP booting performance.
> +	 */
> +	if (net_busy_flag) {
> +		if (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ)
> +			return 0;
> +		else
> +			kbd_testc_tms = get_timer(0);
> +	}

I think you always want to assign to kbd_testc_tms so it always records
the most recent time of the most recent USB keyboard activity, not the
most recent USB keyboard activity while a network operation was in
progress. So,

if (net_busy_flag && get_timer(kbd_testc_tms) < CONFIG_SYS_HZ)
    return 0;
kbd_testc_tms = get_timer(0);

[U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance

[Wolfgang Denk]

Dear Jim Lin,

In message <1374156931-1718-1-git-send-email-jilin@nvidia.com> you wrote:
> TFTP booting is slow when a USB keyboard is installed and
> stdin has usbkbd added.
> This fix is to change Ctrl-C polling for USB keyboard to every second
> when NET transfer is running.
> 
> Signed-off-by: Jim Lin <jilin@nvidia.com>

The sequence of your patches is wrong; you must reverse it: as is,
with only patch 1/2 applied, you will get compile errors because
net_busy_flag is undefined.  This breaks bisectability.

Please switch the order of your patches.


> +	/*
> +	 * If net_busy_flag is 1, NET transfer is running,
> +	 * then we check key pressed every second to improve
> +	 * TFTP booting performance.
> +	 */
> +	if (net_busy_flag) {
> +		if (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ)
> +			return 0;
> +		else
> +			kbd_testc_tms = get_timer(0);
> +	}

When first entering this code, the variable kbd_testc_tms is
(implicitly) initialized as zero;  later, for example when running
multiple network commands, the last used value (i. e. a random number)
is used.  So strictly speaking the comment above is incorrect, as you
don't test for key presses "every second" - the first test may happen
much earlier (even immediately).  I think this should be explained in
the comment to prevent incorrect expectations on the behaviour.



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
Eureka!                                                 -- Archimedes