[U-Boot] [PATCH v4 1/1] NET: Improve TFTP booting performance when CONFIG_USB_KEYBOARD
TFTP booting is slow when a USB keyboard is installed and CONFIG_USB_KEYBOARD is defined. The fix is to change Ctrl-C polling 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
net/net.c | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-)
diff --git a/net/net.c b/net/net.c index df94789..06b41e0 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 kbd_ctrlc_tms = 0; + unsigned long ctrlc_t; + int ctrlc_result; +#endif
NetRestarted = 0; NetDevExists = 0; @@ -472,7 +477,22 @@ restart: /* * Abort if ctrl-c was pressed. */ +#ifdef CONFIG_USB_KEYBOARD + /* + * Reduce ctrl-c checking to 1 second once + * to improve TFTP boot performance. + */ + ctrlc_t = get_timer(kbd_ctrlc_tms); + if (ctrlc_t > CONFIG_SYS_HZ) { + ctrlc_result = ctrlc(); + kbd_ctrlc_tms = get_timer(0); + } else { + ctrlc_result = 0; + } + if (ctrlc_result) { +#else if (ctrlc()) { +#endif /* cancel any ARP that may not have completed */ NetArpWaitPacketIP = 0;
Dear Jim Lin,
In message 1372847667-31928-1-git-send-email-jilin@nvidia.com you wrote:
TFTP booting is slow when a USB keyboard is installed and CONFIG_USB_KEYBOARD is defined. The fix is to change Ctrl-C polling to every second when NET transfer is running.
I'm not sure if we can accept this implementation.
+#ifdef CONFIG_USB_KEYBOARD
/** Reduce ctrl-c checking to 1 second once* to improve TFTP boot performance.*/ctrlc_t = get_timer(kbd_ctrlc_tms);if (ctrlc_t > CONFIG_SYS_HZ) {ctrlc_result = ctrlc();kbd_ctrlc_tms = get_timer(0);} else {ctrlc_result = 0;}if (ctrlc_result) {+#else
get_timer() is used by a number of network related services. For information, just grep for it in the net/ and drivers/net/ directories. The "get_timer(0)" used in your code resets a global resource, and has thus the potential of messing up a number of timeouts running elsewhere in the network code. I wonder to which extend this has actually been considered (and tested) ?
Best regards,
Wolfgang Denk
On 07/03/2013 05:20 AM, Wolfgang Denk wrote:
Dear Jim Lin,
In message 1372847667-31928-1-git-send-email-jilin@nvidia.com you wrote:
TFTP booting is slow when a USB keyboard is installed and CONFIG_USB_KEYBOARD is defined. The fix is to change Ctrl-C polling to every second when NET transfer is running.
I'm not sure if we can accept this implementation.
+#ifdef CONFIG_USB_KEYBOARD
/** Reduce ctrl-c checking to 1 second once* to improve TFTP boot performance.*/ctrlc_t = get_timer(kbd_ctrlc_tms);if (ctrlc_t > CONFIG_SYS_HZ) {ctrlc_result = ctrlc();kbd_ctrlc_tms = get_timer(0);} else {ctrlc_result = 0;}if (ctrlc_result) {+#else
get_timer() is used by a number of network related services. For information, just grep for it in the net/ and drivers/net/ directories. The "get_timer(0)" used in your code resets a global resource, and has thus the potential of messing up a number of timeouts running elsewhere in the network code. I wonder to which extend this has actually been considered (and tested) ?
I recall you mentioning this before, but can you expand on this a bit please?
For the two platforms I'm familiar with (Tegra and BCM2835), the implementation of get_timer() is simply:
unlong get_timer(ulong base) { ulong time = read_hw_register() time -= base; return time; }
There's no global state involved. Is this implementation of get_timer() wrong somehow? I'm having a hard time envisaging what kind of global state it's supposed to maintain.
I always thought that every user of get_timer() was supposed to do something like:
ulong base = get_timer(0); ... work to be timed ulong time_diff = get_timer(base);
in other words, every user maintains their own base variable, and hence get_timer(0) doesn't affect any other users.
Dear Stephen,
In message 51D45F4D.2010908@wwwdotorg.org you wrote:
get_timer() is used by a number of network related services. For information, just grep for it in the net/ and drivers/net/ directories. The "get_timer(0)" used in your code resets a global resource, and has thus the potential of messing up a number of timeouts running elsewhere in the network code. I wonder to which extend this has actually been considered (and tested) ?
I recall you mentioning this before, but can you expand on this a bit please?
For the two platforms I'm familiar with (Tegra and BCM2835), the implementation of get_timer() is simply:
unlong get_timer(ulong base) { ulong time = read_hw_register() time -= base; return time; }
There's no global state involved. Is this implementation of get_timer() wrong somehow? I'm having a hard time envisaging what kind of global state it's supposed to maintain.
It appears you are (basically) right. Eventually I remember an old implementation that has been fixed since.
I checked all implementations I could find (all 102 of them) and they all behave as you showed in your example, i. e. harmless.
An exception is "arch/arm/cpu/sa1100/timer.c" which does not respect the "base" argument at all, i. e. which is broken.
I always thought that every user of get_timer() was supposed to do something like:
ulong base = get_timer(0); ... work to be timed ulong time_diff = get_timer(base);
in other words, every user maintains their own base variable, and hence get_timer(0) doesn't affect any other users.
Yes, you are right. Sorry for causing confusion here.
Best regards,
Wolfgang Denk
participants (3)
-
Jim Lin -
Stephen Warren -
Wolfgang Denk