* [U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance
@ 2013-07-18 14:15 Jim Lin
2013-07-18 14:15 ` [U-Boot] [PATCH v6 2/2] NET: Add net_busy_flag Jim Lin
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Jim Lin @ 2013-07-18 14:15 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH v6 2/2] NET: Add net_busy_flag
2013-07-18 14:15 [U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance Jim Lin
@ 2013-07-18 14:15 ` Jim Lin
2013-07-18 17:32 ` Stephen Warren
2013-07-18 14:24 ` [U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance Marek Vasut
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Jim Lin @ 2013-07-18 14:15 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance
2013-07-18 14:15 [U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance Jim Lin
2013-07-18 14:15 ` [U-Boot] [PATCH v6 2/2] NET: Add net_busy_flag Jim Lin
@ 2013-07-18 14:24 ` Marek Vasut
2013-07-18 17:29 ` Stephen Warren
2013-07-19 6:43 ` Wolfgang Denk
3 siblings, 0 replies; 7+ messages in thread
From: Marek Vasut @ 2013-07-18 14:24 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance
2013-07-18 14:15 [U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance Jim Lin
2013-07-18 14:15 ` [U-Boot] [PATCH v6 2/2] NET: Add net_busy_flag Jim Lin
2013-07-18 14:24 ` [U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance Marek Vasut
@ 2013-07-18 17:29 ` Stephen Warren
2013-07-19 6:43 ` Wolfgang Denk
3 siblings, 0 replies; 7+ messages in thread
From: Stephen Warren @ 2013-07-18 17:29 UTC (permalink / raw)
To: u-boot
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);
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH v6 2/2] NET: Add net_busy_flag
2013-07-18 14:15 ` [U-Boot] [PATCH v6 2/2] NET: Add net_busy_flag Jim Lin
@ 2013-07-18 17:32 ` Stephen Warren
2013-07-18 23:37 ` Marek Vasut
0 siblings, 1 reply; 7+ messages in thread
From: Stephen Warren @ 2013-07-18 17:32 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH v6 2/2] NET: Add net_busy_flag
2013-07-18 17:32 ` Stephen Warren
@ 2013-07-18 23:37 ` Marek Vasut
0 siblings, 0 replies; 7+ messages in thread
From: Marek Vasut @ 2013-07-18 23:37 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance
2013-07-18 14:15 [U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance Jim Lin
` (2 preceding siblings ...)
2013-07-18 17:29 ` Stephen Warren
@ 2013-07-19 6:43 ` Wolfgang Denk
3 siblings, 0 replies; 7+ messages in thread
From: Wolfgang Denk @ 2013-07-19 6:43 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-07-19 6:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-18 14:15 [U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance Jim Lin
2013-07-18 14:15 ` [U-Boot] [PATCH v6 2/2] NET: Add net_busy_flag Jim Lin
2013-07-18 17:32 ` Stephen Warren
2013-07-18 23:37 ` Marek Vasut
2013-07-18 14:24 ` [U-Boot] [PATCH v6 1/2] console: usb: kbd: To improve TFTP booting performance Marek Vasut
2013-07-18 17:29 ` Stephen Warren
2013-07-19 6:43 ` Wolfgang Denk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.