Patchwork [U-Boot,v7,2/2] console: usb: kbd: To improve TFTP booting performance

login
register
mail settings
Submitter Jim Lin
Date July 19, 2013, 9:36 a.m.
Message ID <1374226576-13401-2-git-send-email-jilin@nvidia.com>
Download mbox | patch
Permalink /patch/260229/
State Superseded
Headers show

Comments

Jim Lin - July 19, 2013, 9:36 a.m.
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
Changes in v7:
 1. In common/usb_kbd.c and include/usb.h, add #ifdef CONFIG_CMD_NET.
 2. In common/usb_kbd.c, modify code to get correct time gap.

 common/usb_kbd.c |   14 ++++++++++++++
 include/usb.h    |    4 +++-
 2 files changed, 17 insertions(+), 1 deletions(-)
Wolfgang Denk - July 19, 2013, 11:17 a.m.
Dear Jim Lin,

In message <1374226576-13401-2-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.
...
> +#ifdef CONFIG_CMD_NET
> +/* 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 +370,16 @@ static int usb_kbd_testc(void)
>  	struct usb_device *usb_kbd_dev;
>  	struct usb_kbd_pdata *data;
>  
> +#ifdef CONFIG_CMD_NET
> +	/*
> +	 * 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 && (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
> +		return 0;
> +	kbd_testc_tms = get_timer(0);
> +#endif

You did not comment on my remark about  kbd_testc_tms  being used
basically with a random start value for each invocation of a network
command.  The "every second" above is wrong.  The actual interval may
be much shorter (even nearly zero, at least once), or longer.


BTW: Please fix the list of mail addresses you're sending to, and
remove non-existing ones.

Best regards,

Wolfgang Denk
Jim Lin - July 19, 2013, 12:06 p.m.
On Fri, 2013-07-19 at 19:17 +0800, Wolfgang Denk wrote:
> Dear Jim Lin,
> 
> In message <1374226576-13401-2-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.
> ...
> > +#ifdef CONFIG_CMD_NET
> > +/* 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 +370,16 @@ static int usb_kbd_testc(void)
> >  	struct usb_device *usb_kbd_dev;
> >  	struct usb_kbd_pdata *data;
> >  
> > +#ifdef CONFIG_CMD_NET
> > +	/*
> > +	 * 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 && (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
> > +		return 0;
> > +	kbd_testc_tms = get_timer(0);
> > +#endif
> 
> You did not comment on my remark about  kbd_testc_tms  being used
> basically with a random start value for each invocation of a network
> command.  The "every second" above is wrong.  The actual interval may
> be much shorter (even nearly zero, at least once), or longer.
Okay, I will check again.




-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
Marek Vasut - July 19, 2013, 1:53 p.m.
Dear Jim Lin,

> On Fri, 2013-07-19 at 19:17 +0800, Wolfgang Denk wrote:
> > Dear Jim Lin,
> > 
> > In message <1374226576-13401-2-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.
> > 
> > ...
> > 
> > > +#ifdef CONFIG_CMD_NET
> > > +/* 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 +370,16 @@ static int usb_kbd_testc(void)
> > > 
> > >  	struct usb_device *usb_kbd_dev;
> > >  	struct usb_kbd_pdata *data;
> > > 
> > > +#ifdef CONFIG_CMD_NET
> > > +	/*
> > > +	 * 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 && (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
> > > +		return 0;
> > > +	kbd_testc_tms = get_timer(0);
> > > +#endif
> > 
> > You did not comment on my remark about  kbd_testc_tms  being used
> > basically with a random start value for each invocation of a network
> > command.  The "every second" above is wrong.  The actual interval may
> > be much shorter (even nearly zero, at least once), or longer.
> 
> Okay, I will check again.
> 
I'll wait for v3. In the meantime, I homestly dont like having such hacks on 
both sides, but I dont think much can be done about it. Maybe use __maybe_unused 
for the net_busy_flag to drop the ifdef ?

Best regards,
Marek Vasut
Jim Lin - July 31, 2013, 9:48 a.m.
On Fri, 2013-07-19 at 19:17 +0800, Wolfgang Denk wrote:
> Dear Jim Lin,
> 
> In message <1374226576-13401-2-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.
> ...
> > +#ifdef CONFIG_CMD_NET
> > +/* 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 +370,16 @@ static int usb_kbd_testc(void)
> >  	struct usb_device *usb_kbd_dev;
> >  	struct usb_kbd_pdata *data;
> >  
> > +#ifdef CONFIG_CMD_NET
> > +	/*
> > +	 * 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 && (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
> > +		return 0;
> > +	kbd_testc_tms = get_timer(0);
> > +#endif
> 
> You did not comment on my remark about  kbd_testc_tms  being used
> basically with a random start value for each invocation of a network
> command.  The "every second" above is wrong.  The actual interval may
> be much shorter (even nearly zero, at least once), or longer.
I will change the comment as the following.
/*
 * If net_busy_flag is 1, NET transfer is running,
 * then we check key pressed every second (first check
 * may be less than 1 second)
 * to improve TFTP booting performance.
 */

The behavior you mentioned is not that critical.

--nvpublic
Jim Lin - July 31, 2013, 10:21 a.m.
On Fri, 2013-07-19 at 21:53 +0800, Marek Vasut wrote:
> Dear Jim Lin,
> 
> > On Fri, 2013-07-19 at 19:17 +0800, Wolfgang Denk wrote:
> > > Dear Jim Lin,
> > > 
> > > In message <1374226576-13401-2-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.
> > > 
> > > ...
> > > 
> > > > +#ifdef CONFIG_CMD_NET
> > > > +/* 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 +370,16 @@ static int usb_kbd_testc(void)
> > > > 
> > > >  	struct usb_device *usb_kbd_dev;
> > > >  	struct usb_kbd_pdata *data;
> > > > 
> > > > +#ifdef CONFIG_CMD_NET
> > > > +	/*
> > > > +	 * 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 && (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
> > > > +		return 0;
> > > > +	kbd_testc_tms = get_timer(0);
> > > > +#endif
> > > 
> > > You did not comment on my remark about  kbd_testc_tms  being used
> > > basically with a random start value for each invocation of a network
> > > command.  The "every second" above is wrong.  The actual interval may
> > > be much shorter (even nearly zero, at least once), or longer.
> > 
> > Okay, I will check again.
> > 
> I'll wait for v3. In the meantime, I homestly dont like having such hacks on 
> both sides, but I dont think much can be done about it. Maybe use __maybe_unused 
> for the net_busy_flag to drop the ifdef ?
I don't quite understand what you wanted me to do.
Could you give me an example? What to add and what to remove.
Thanks.

--nvpublic
Jim Lin - July 31, 2013, 10:30 a.m.
On Fri, 2013-07-19 at 21:53 +0800, Marek Vasut wrote:
> Dear Jim Lin,
> 
> > On Fri, 2013-07-19 at 19:17 +0800, Wolfgang Denk wrote:
> > > Dear Jim Lin,
> > > 
> > > In message <1374226576-13401-2-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.
> > > 
> > > ...
> > > 
> > > > +#ifdef CONFIG_CMD_NET
> > > > +/* 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 +370,16 @@ static int usb_kbd_testc(void)
> > > > 
> > > >  	struct usb_device *usb_kbd_dev;
> > > >  	struct usb_kbd_pdata *data;
> > > > 
> > > > +#ifdef CONFIG_CMD_NET
> > > > +	/*
> > > > +	 * 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 && (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
> > > > +		return 0;
> > > > +	kbd_testc_tms = get_timer(0);
> > > > +#endif
> > > 
> > > You did not comment on my remark about  kbd_testc_tms  being used
> > > basically with a random start value for each invocation of a network
> > > command.  The "every second" above is wrong.  The actual interval may
> > > be much shorter (even nearly zero, at least once), or longer.
> > 
> > Okay, I will check again.
> > 
> I'll wait for v3. In the meantime, I homestly dont like having such hacks on 
> both sides, but I dont think much can be done about it. Maybe use __maybe_unused 
> for the net_busy_flag to drop the ifdef ?
I don't quite understand what you wanted me to do.
Could you give me an example? What to add and what to remove.
Thanks.

--nvpublic
Marek Vasut - July 31, 2013, 10:36 a.m.
Dear Jim Lin,

> On Fri, 2013-07-19 at 21:53 +0800, Marek Vasut wrote:
> > Dear Jim Lin,
> > 
> > > On Fri, 2013-07-19 at 19:17 +0800, Wolfgang Denk wrote:
> > > > Dear Jim Lin,
> > > > 
> > > > In message <1374226576-13401-2-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.
> > > > 
> > > > ...
> > > > 
> > > > > +#ifdef CONFIG_CMD_NET
> > > > > +/* The period of time between two calls of usb_kbd_testc(). */
> > > > > +static unsigned long kbd_testc_tms;
> > > > > +#endif

Just use __maybe_unused here to squash the ifdef ;-)

[...]

> > I'll wait for v3. In the meantime, I homestly dont like having such hacks
> > on both sides, but I dont think much can be done about it. Maybe use
> > __maybe_unused for the net_busy_flag to drop the ifdef ?
> 
> I don't quite understand what you wanted me to do.
> Could you give me an example? What to add and what to remove.
> Thanks.
> 
> --nvpublic

Best regards,
Marek Vasut

Patch

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 3174b5e..05e0c7e 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -120,6 +120,10 @@  struct usb_kbd_pdata {
 
 	uint8_t		flags;
 };
+#ifdef CONFIG_CMD_NET
+/* 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 +370,16 @@  static int usb_kbd_testc(void)
 	struct usb_device *usb_kbd_dev;
 	struct usb_kbd_pdata *data;
 
+#ifdef CONFIG_CMD_NET
+	/*
+	 * 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 && (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
+		return 0;
+	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/include/usb.h b/include/usb.h
index d7b082d..e1901d1 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -206,7 +206,9 @@  int usb_host_eth_scan(int mode);
 
 int drv_usb_kbd_init(void);
 int usb_kbd_deregister(void);
-
+#ifdef CONFIG_CMD_NET
+extern int net_busy_flag;
+#endif
 #endif
 /* routines */
 int usb_init(void); /* initialize the USB Controller */