[U-Boot] [PATCH v3 1/2] console: usbkbd: Improve TFTP booting performance

[U-Boot] [PATCH v3 1/2] console: usbkbd: Improve TFTP booting performance

[Jim Lin]

TFTP booting is observed a little bit slow, especially when a USB
keyboard is installed.
The fix is to move polling to every second if we sense that other task
like TFTP boot 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.

 common/usb_kbd.c |   20 ++++++++++++++++++++
 doc/README.usb   |   15 ++++++++++++++-
 2 files changed, 34 insertions(+), 1 deletions(-)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 3174b5e..25bf677 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -121,6 +121,11 @@ struct usb_kbd_pdata {
 	uint8_t		flags;
 };
 
+#ifdef CONFIG_USBKB_TESTC_PERIOD
+/* The period of time between two calls of usb_kbd_testc(). */
+static unsigned long kbd_testc_tms;
+#endif
+
 /* Generic keyboard event polling. */
 void usb_kbd_generic_poll(void)
 {
@@ -366,6 +371,21 @@ static int usb_kbd_testc(void)
 	struct usb_device *usb_kbd_dev;
 	struct usb_kbd_pdata *data;
 
+#ifdef CONFIG_USBKB_TESTC_PERIOD
+	/*
+	 * T is the time between two calls of usb_kbd_testc().
+	 * If CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms,
+	 * it implies other task like TFTP boot is running,
+	 * then we reduce polling to every second
+	 * to improve TFTP booting performance.
+	 */
+	if ((get_timer(kbd_testc_tms) >=
+	    (CONFIG_USBKB_TESTC_PERIOD * CONFIG_SYS_HZ / 1000)) &&
+	    (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
+		return 0;
+	else
+		kbd_testc_tms = get_timer(0);
+#endif
 	dev = stdio_get_by_name(DEVNAME);
 	usb_kbd_dev = (struct usb_device *)dev->priv;
 	data = usb_kbd_dev->privptr;
diff --git a/doc/README.usb b/doc/README.usb
index b4c3ef5..ff4ee6f 100644
--- a/doc/README.usb
+++ b/doc/README.usb
@@ -80,7 +80,20 @@ CONFIG_USB_UHCI	    defines the lowlevel part.A lowlevel part must be defined
 CONFIG_USB_KEYBOARD enables the USB Keyboard
 CONFIG_USB_STORAGE  enables the USB storage devices
 CONFIG_USB_HOST_ETHER	enables USB ethernet adapter support
-
+CONFIG_USBKB_TESTC_PERIOD defines the average time (in ms) between two calls
+			of usb_kbd_testc() when TFTP boot is not running
+			In u-boot console, usb_kbd_testc() will be called
+			periodically.
+			When this configuration is defined and other task like
+			TFTP boot is running, USB keyboard will be polled less
+			frequently and will be polled every second.
+			This is to improve TFTP booting performance when USB
+			keyboard is installed.
+			In u-boot console, no impact on keyboard input if TFTP
+			boot is not running.
+			Example:
+			Define the following in configuration header file
+			#define CONFIG_USBKB_TESTC_PERIOD 100
 
 USB Host Networking
 ===================
-- 
1.7.7

[U-Boot] [PATCH v3 2/2] config: Tegra: Improve TFTP booting performance

[Jim Lin]

Add CONFIG_USBKB_TESTC_PERIOD configuration if CONFIG_USB_KEYBOARD
is defined in platform configuration file.

Signed-off-by: Jim Lin <jilin@nvidia.com>
---
Changes in v2:
 - None
Changes in v3:
 - Add CONFIG_USBKB_TESTC_PERIOD in include/configs/tegra-common-post.h

 include/configs/tegra-common-post.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h
index 6ed2fde..ee341ef 100644
--- a/include/configs/tegra-common-post.h
+++ b/include/configs/tegra-common-post.h
@@ -128,6 +128,7 @@
 #ifdef CONFIG_USB_KEYBOARD
 #define STDIN_KBD_USB ",usbkbd"
 #define CONFIG_SYS_USB_EVENT_POLL
+#define CONFIG_USBKB_TESTC_PERIOD 100
 #define CONFIG_PREBOOT			"usb start"
 #else
 #define STDIN_KBD_USB ""
-- 
1.7.7

[U-Boot] [PATCH v3 1/2] console: usbkbd: Improve TFTP booting performance

[Stephen Warren]

On 06/27/2013 03:45 AM, Jim Lin wrote:
> TFTP booting is observed a little bit slow, especially when a USB
> keyboard is installed.
> The fix is to move polling to every second if we sense that other task
> like TFTP boot is running.
> 

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

> +#ifdef CONFIG_USBKB_TESTC_PERIOD
> +	/*
> +	 * T is the time between two calls of usb_kbd_testc().
> +	 * If CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms,
> +	 * it implies other task like TFTP boot is running,
> +	 * then we reduce polling to every second
> +	 * to improve TFTP booting performance.
> +	 */
> +	if ((get_timer(kbd_testc_tms) >=
> +	    (CONFIG_USBKB_TESTC_PERIOD * CONFIG_SYS_HZ / 1000)) &&
> +	    (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
> +		return 0;
> +	else
> +		kbd_testc_tms = get_timer(0);
> +#endif

I have a hard time understanding why the fact that "some other task is
running" implies anything at all re: how often usb_kbd_testc() would be
called.

It's quite possible that "some other task" is extremely fine-grained,
and calls usb_kbd_testc() every 0.1ms, and would be severely negatively
affected by usb_kbd_testc() taking a long time to execute.

Conversly, it's quite possible that "some other task" is quite granular,
and calls usb_kbd_testc() a wide intervals, say every 200ms.

So, I think this change keys of entirely the wrong thing.

Shouldn't the TFTP process (or use of USB networking?) or other
long-running tasks that do check for keyboard IO simply set some flag to
indicate to usb_kbd_testc() that it should run at a reduced rate, or
even just have those long-running processses call usb_kbd_testc() at a
reduced rate themselves?

[U-Boot] [PATCH v3 1/2] console: usbkbd: Improve TFTP booting performance

[Jim Lin]

On Fri, 2013-06-28 at 02:20 +0800, Stephen Warren wrote:
> On 06/27/2013 03:45 AM, Jim Lin wrote:
> > TFTP booting is observed a little bit slow, especially when a USB
> > keyboard is installed.
> > The fix is to move polling to every second if we sense that other task
> > like TFTP boot is running.
> > 
> 
> > diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> 
> > +#ifdef CONFIG_USBKB_TESTC_PERIOD
> > +	/*
> > +	 * T is the time between two calls of usb_kbd_testc().
> > +	 * If CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms,
> > +	 * it implies other task like TFTP boot is running,
> > +	 * then we reduce polling to every second
> > +	 * to improve TFTP booting performance.
> > +	 */
> > +	if ((get_timer(kbd_testc_tms) >=
> > +	    (CONFIG_USBKB_TESTC_PERIOD * CONFIG_SYS_HZ / 1000)) &&
> > +	    (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
> > +		return 0;
> > +	else
> > +		kbd_testc_tms = get_timer(0);
> > +#endif
> 
> I have a hard time understanding why the fact that "some other task is
> running" implies anything at all re: how often usb_kbd_testc() would be
> called.
In my case it takes about 95 ms on Tegra20 and Tegra114 for
usb_kbd_testc() to be called periodically.
So I set CONFIG_USBKB_TESTC_PERIOD to 100.
Like I said, if CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms
we reduce polling (send command to USB keyboard to check is there
any key pressed) to every second.

> 
> It's quite possible that "some other task" is extremely fine-grained,
> and calls usb_kbd_testc() every 0.1ms, and would be severely negatively
> affected by usb_kbd_testc() taking a long time to execute.
> 
> Conversly, it's quite possible that "some other task" is quite granular,
> and calls usb_kbd_testc() a wide intervals, say every 200ms.
> 
> So, I think this change keys of entirely the wrong thing.
> 
> Shouldn't the TFTP process (or use of USB networking?) or other
> long-running tasks that do check for keyboard IO simply set some flag to
> indicate to usb_kbd_testc() that it should run at a reduced rate, or
> even just have those long-running processses call usb_kbd_testc() at a
> reduced rate themselves?
To fix in usb_kbd_testc() is easier because this issue happens only when
USB keyboard is installed and CONFIG_USB_KEYBOARD is defined.

--
nvpublic

[U-Boot] [PATCH v3 1/2] console: usbkbd: Improve TFTP booting performance

[Stephen Warren]

On 06/27/2013 09:59 PM, Jim Lin wrote:
> On Fri, 2013-06-28 at 02:20 +0800, Stephen Warren wrote:
>> On 06/27/2013 03:45 AM, Jim Lin wrote:
>>> TFTP booting is observed a little bit slow, especially when a USB
>>> keyboard is installed.
>>> The fix is to move polling to every second if we sense that other task
>>> like TFTP boot is running.
>>>
>>
>>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>>
>>> +#ifdef CONFIG_USBKB_TESTC_PERIOD
>>> +	/*
>>> +	 * T is the time between two calls of usb_kbd_testc().
>>> +	 * If CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms,
>>> +	 * it implies other task like TFTP boot is running,
>>> +	 * then we reduce polling to every second
>>> +	 * to improve TFTP booting performance.
>>> +	 */
>>> +	if ((get_timer(kbd_testc_tms) >=
>>> +	    (CONFIG_USBKB_TESTC_PERIOD * CONFIG_SYS_HZ / 1000)) &&
>>> +	    (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
>>> +		return 0;
>>> +	else
>>> +		kbd_testc_tms = get_timer(0);
>>> +#endif
>>
>> I have a hard time understanding why the fact that "some other task is
>> running" implies anything at all re: how often usb_kbd_testc() would be
>> called.
> In my case it takes about 95 ms on Tegra20 and Tegra114 for
> usb_kbd_testc() to be called periodically.
> So I set CONFIG_USBKB_TESTC_PERIOD to 100.
> Like I said, if CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms
> we reduce polling (send command to USB keyboard to check is there
> any key pressed) to every second.

OK, so I think how this works is: If nothing is happening, then
usb_kbd_testc() is repeatedly called back-to-back with no delay between.
So, if the time between two calls to usb_kbd_testc() is much longer than
the time it takes to execute it once, then something else is going on,
and hence the code should skip some calls to usb_kbd_testc().

If that's how this works, then why require CONFIG_USBKBD_TESTC_PERIOD to
be set? Why not simply measure the time between when usb_kbd_testc()
returns, and when it is re-entered? If it's very short, nothing else is
happening. If it's very long, something else is happening. That is a far
more direct measurement, and is immune to e.g. CPU frequency differences
in a way that a static value for CONFIG_USBKBD_TESTC_PERIOD is not.

Also, any kind of time measurement doesn't solve the issue I mentioned
re: how granular the other task is.

Finally, if you're sitting at the command-prompt, is usb_kbd_testc()
used at all? How does regular typing using a USB keyboard interact with
this code; will typing react fast, but CTRL-C react slowly?

[U-Boot] [PATCH v3 1/2] console: usbkbd: Improve TFTP booting performance

[Jim Lin]

On Fri, 2013-06-28 at 13:09 +0800, Stephen Warren wrote:
> On 06/27/2013 09:59 PM, Jim Lin wrote:
> > On Fri, 2013-06-28 at 02:20 +0800, Stephen Warren wrote:
> >> On 06/27/2013 03:45 AM, Jim Lin wrote:
> >>> TFTP booting is observed a little bit slow, especially when a USB
> >>> keyboard is installed.
> >>> The fix is to move polling to every second if we sense that other task
> >>> like TFTP boot is running.
> >>>
> >>
> >>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> >>
> >>> +#ifdef CONFIG_USBKB_TESTC_PERIOD
> >>> +	/*
> >>> +	 * T is the time between two calls of usb_kbd_testc().
> >>> +	 * If CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms,
> >>> +	 * it implies other task like TFTP boot is running,
> >>> +	 * then we reduce polling to every second
> >>> +	 * to improve TFTP booting performance.
> >>> +	 */
> >>> +	if ((get_timer(kbd_testc_tms) >=
> >>> +	    (CONFIG_USBKB_TESTC_PERIOD * CONFIG_SYS_HZ / 1000)) &&
> >>> +	    (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
> >>> +		return 0;
> >>> +	else
> >>> +		kbd_testc_tms = get_timer(0);
> >>> +#endif
> >>
> >> I have a hard time understanding why the fact that "some other task is
> >> running" implies anything at all re: how often usb_kbd_testc() would be
> >> called.
> > In my case it takes about 95 ms on Tegra20 and Tegra114 for
> > usb_kbd_testc() to be called periodically.
> > So I set CONFIG_USBKB_TESTC_PERIOD to 100.
> > Like I said, if CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms
> > we reduce polling (send command to USB keyboard to check is there
> > any key pressed) to every second.
> 
> OK, so I think how this works is: If nothing is happening, then
> usb_kbd_testc() is repeatedly called back-to-back with no delay between.
> So, if the time between two calls to usb_kbd_testc() is much longer than
> the time it takes to execute it once, then something else is going on,
> and hence the code should skip some calls to usb_kbd_testc().
> 
> If that's how this works, then why require CONFIG_USBKBD_TESTC_PERIOD to
> be set? Why not simply measure the time between when usb_kbd_testc()
> returns, and when it is re-entered? If it's very short, nothing else is
> happening. If it's very long, something else is happening. That is a far
> more direct measurement, and is immune to e.g. CPU frequency differences
Good idea and will be introduced in next revision.

> in a way that a static value for CONFIG_USBKBD_TESTC_PERIOD is not.
> 
> Also, any kind of time measurement doesn't solve the issue I mentioned
> re: how granular the other task is.
> 
> Finally, if you're sitting at the command-prompt, is usb_kbd_testc()
> used at all? How does regular typing using a USB keyboard interact with
> this code; will typing react fast, but CTRL-C react slowly?
They should react at same rate.

--
nvpublic