[U-Boot] [PATCH] usb: kbd: Fix key repeat not always using

[U-Boot] [PATCH] usb: kbd: Fix key repeat not always using

[Hans de Goede]

The usb-kbd key repeat code assumes that reports get repeated every 40 ms,
this is never true when using CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP, and
does not always works for CONFIG_SYS_USB_EVENT_POLL and
CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE since not all usb keyboards honor
the usb_set_idle() command.

For CONFIG_SYS_USB_EVENT_POLL we must use usb_set_idle() since we do a
blocking wait for the hid report, so if we do not tell the keyboard to send
a hid report every 40ms even if nothing changes then we will block u-boot
for 1s (the default u-boot usb interrupt packet timeout). Note that in this
case on keyboards which do not support usb_set_idle() we loose and we actually
get 1s latencies on other u-boot activities.

For the other poll-methods this commit stops using usb_set_idle() and instead
repeats the last received hid-report every 40 ms as long as no new hid-report
is received. This fixes key-repeat not working at all with
CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP and fixes it not working with
keyboards which do not implement usb_set_idle() when using
CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 common/usb_kbd.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 24a1a56..d4733b1 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -31,7 +31,7 @@ int overwrite_console(void)
 #endif
 
 /* Keyboard sampling rate */
-#define REPEAT_RATE	(40 / 4)	/* 40msec -> 25cps */
+#define REPEAT_RATE	40		/* 40msec -> 25cps */
 #define REPEAT_DELAY	10		/* 10 x REPEAT_RATE = 400msec */
 
 #define NUM_LOCK	0x53
@@ -103,6 +103,7 @@ struct usb_kbd_pdata {
 	unsigned long	intpipe;
 	int		intpktsize;
 	int		intinterval;
+	int		last_report;
 	struct int_queue *intq;
 
 	uint32_t	repeat_delay;
@@ -318,15 +319,16 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
 			   data->intinterval);
 
 	usb_kbd_irq_worker(dev);
-#elif	defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
+#else
+# if	defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
 	struct usb_interface *iface;
 	struct usb_kbd_pdata *data = dev->privptr;
 	iface = &dev->config.if_desc[0];
 	usb_get_report(dev, iface->desc.bInterfaceNumber,
 		       1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE);
-	if (memcmp(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE))
+	if (memcmp(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE)) {
 		usb_kbd_irq_worker(dev);
-#elif	defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
+# elif	defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
 	struct usb_kbd_pdata *data = dev->privptr;
 	if (poll_int_queue(dev, data->intq)) {
 		usb_kbd_irq_worker(dev);
@@ -335,6 +337,13 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
 		data->intq = create_int_queue(dev, data->intpipe, 1,
 				      USB_KBD_BOOT_REPORT_SIZE, data->new,
 				      data->intinterval);
+# endif
+		data->last_report = get_timer(0);
+	/* Repeat last usb hid report every REPEAT_RATE ms for keyrepeat */
+	} else if (data->last_report != -1 &&
+		   get_timer(data->last_report) > REPEAT_RATE) {
+		usb_kbd_irq_worker(dev);
+		data->last_report = get_timer(0);
 	}
 #endif
 }
@@ -445,12 +454,15 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
 	data->intpktsize = min(usb_maxpacket(dev, data->intpipe),
 			       USB_KBD_BOOT_REPORT_SIZE);
 	data->intinterval = ep->bInterval;
+	data->last_report = -1;
 
 	/* We found a USB Keyboard, install it. */
 	usb_set_protocol(dev, iface->desc.bInterfaceNumber, 0);
 
+#ifdef CONFIG_SYS_USB_EVENT_POLL
 	debug("USB KBD: found set idle...\n");
-	usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
+	usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE / 4, 0);
+#endif
 
 	debug("USB KBD: enable interrupt pipe...\n");
 #ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE
-- 
2.3.6

[U-Boot] [PATCH] usb: kbd: Fix key repeat not always using

[Michael Trimarchi]

Hi

On Tue, May 12, 2015 at 6:58 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> The usb-kbd key repeat code assumes that reports get repeated every 40 ms,
> this is never true when using CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP, and
> does not always works for CONFIG_SYS_USB_EVENT_POLL and
> CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE since not all usb keyboards honor
> the usb_set_idle() command.
>
> For CONFIG_SYS_USB_EVENT_POLL we must use usb_set_idle() since we do a
> blocking wait for the hid report, so if we do not tell the keyboard to send
> a hid report every 40ms even if nothing changes then we will block u-boot
> for 1s (the default u-boot usb interrupt packet timeout). Note that in this
> case on keyboards which do not support usb_set_idle() we loose and we actually
> get 1s latencies on other u-boot activities.
>
> For the other poll-methods this commit stops using usb_set_idle() and instead
> repeats the last received hid-report every 40 ms as long as no new hid-report
> is received. This fixes key-repeat not working at all with
> CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP and fixes it not working with
> keyboards which do not implement usb_set_idle() when using
> CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  common/usb_kbd.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 24a1a56..d4733b1 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -31,7 +31,7 @@ int overwrite_console(void)
>  #endif
>
>  /* Keyboard sampling rate */
> -#define REPEAT_RATE    (40 / 4)        /* 40msec -> 25cps */
> +#define REPEAT_RATE    40              /* 40msec -> 25cps */
>  #define REPEAT_DELAY   10              /* 10 x REPEAT_RATE = 400msec */
>
>  #define NUM_LOCK       0x53
> @@ -103,6 +103,7 @@ struct usb_kbd_pdata {
>         unsigned long   intpipe;
>         int             intpktsize;
>         int             intinterval;
> +       int             last_report;
>         struct int_queue *intq;
>
>         uint32_t        repeat_delay;
> @@ -318,15 +319,16 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
>                            data->intinterval);
>
>         usb_kbd_irq_worker(dev);
> -#elif  defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
> +#else
> +# if   defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
>         struct usb_interface *iface;
>         struct usb_kbd_pdata *data = dev->privptr;
>         iface = &dev->config.if_desc[0];
>         usb_get_report(dev, iface->desc.bInterfaceNumber,
>                        1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE);
> -       if (memcmp(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE))
> +       if (memcmp(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE)) {
>                 usb_kbd_irq_worker(dev);
> -#elif  defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
> +# elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
>         struct usb_kbd_pdata *data = dev->privptr;
>         if (poll_int_queue(dev, data->intq)) {
>                 usb_kbd_irq_worker(dev);
> @@ -335,6 +337,13 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
>                 data->intq = create_int_queue(dev, data->intpipe, 1,
>                                       USB_KBD_BOOT_REPORT_SIZE, data->new,
>                                       data->intinterval);
> +# endif

With or without space?

> +               data->last_report = get_timer(0);
> +       /* Repeat last usb hid report every REPEAT_RATE ms for keyrepeat */
> +       } else if (data->last_report != -1 &&
> +                  get_timer(data->last_report) > REPEAT_RATE) {
> +               usb_kbd_irq_worker(dev);
> +               data->last_report = get_timer(0);

is int get_timer type?

>         }
>  #endif
>  }
> @@ -445,12 +454,15 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
>         data->intpktsize = min(usb_maxpacket(dev, data->intpipe),
>                                USB_KBD_BOOT_REPORT_SIZE);
>         data->intinterval = ep->bInterval;
> +       data->last_report = -1;
>
>         /* We found a USB Keyboard, install it. */
>         usb_set_protocol(dev, iface->desc.bInterfaceNumber, 0);
>
> +#ifdef CONFIG_SYS_USB_EVENT_POLL
>         debug("USB KBD: found set idle...\n");
> -       usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
> +       usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE / 4, 0);
> +#endif
>
>         debug("USB KBD: enable interrupt pipe...\n");
>  #ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE
> --
> 2.3.6
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] usb: kbd: Fix key repeat not always using

[Marek Vasut]

On Tuesday, May 12, 2015 at 06:58:23 PM, Hans de Goede wrote:
> The usb-kbd key repeat code assumes that reports get repeated every 40 ms,
> this is never true when using CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP, and
> does not always works for CONFIG_SYS_USB_EVENT_POLL and
> CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE since not all usb keyboards honor
> the usb_set_idle() command.
> 
> For CONFIG_SYS_USB_EVENT_POLL we must use usb_set_idle() since we do a
> blocking wait for the hid report, so if we do not tell the keyboard to send
> a hid report every 40ms even if nothing changes then we will block u-boot
> for 1s (the default u-boot usb interrupt packet timeout). Note that in this
> case on keyboards which do not support usb_set_idle() we loose and we
> actually get 1s latencies on other u-boot activities.
> 
> For the other poll-methods this commit stops using usb_set_idle() and
> instead repeats the last received hid-report every 40 ms as long as no new
> hid-report is received. This fixes key-repeat not working at all with
> CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP and fixes it not working with
> keyboards which do not implement usb_set_idle() when using
> CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Looks reasonable, I don't really care about the ifdef indent, but the
data type for get_timer() should be fixed. Otherwise,

Reviewed-by: Marek Vasut <marex@denx.de>

Would this finally be a patch to pick through the USB tree ? :b

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb: kbd: Fix key repeat not always using

[Hans de Goede]

Hi,

On 12-05-15 21:30, Marek Vasut wrote:
> On Tuesday, May 12, 2015 at 06:58:23 PM, Hans de Goede wrote:
>> The usb-kbd key repeat code assumes that reports get repeated every 40 ms,
>> this is never true when using CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP, and
>> does not always works for CONFIG_SYS_USB_EVENT_POLL and
>> CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE since not all usb keyboards honor
>> the usb_set_idle() command.
>>
>> For CONFIG_SYS_USB_EVENT_POLL we must use usb_set_idle() since we do a
>> blocking wait for the hid report, so if we do not tell the keyboard to send
>> a hid report every 40ms even if nothing changes then we will block u-boot
>> for 1s (the default u-boot usb interrupt packet timeout). Note that in this
>> case on keyboards which do not support usb_set_idle() we loose and we
>> actually get 1s latencies on other u-boot activities.
>>
>> For the other poll-methods this commit stops using usb_set_idle() and
>> instead repeats the last received hid-report every 40 ms as long as no new
>> hid-report is received. This fixes key-repeat not working at all with
>> CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP and fixes it not working with
>> keyboards which do not implement usb_set_idle() when using
>> CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Looks reasonable, I don't really care about the ifdef indent, but the
> data type for get_timer() should be fixed. Otherwise,

OK v2 with last_report changed to unsigned_long is coming up.

> Reviewed-by: Marek Vasut <marex@denx.de>
>
> Would this finally be a patch to pick through the USB tree ? :b

Yes!  :)

Regards,

Hans

[U-Boot] [PATCH] usb: kbd: Fix key repeat not always using

[Marek Vasut]

On Wednesday, May 13, 2015 at 02:46:34 PM, Hans de Goede wrote:
> Hi,
> 
> On 12-05-15 21:30, Marek Vasut wrote:
> > On Tuesday, May 12, 2015 at 06:58:23 PM, Hans de Goede wrote:
> >> The usb-kbd key repeat code assumes that reports get repeated every 40
> >> ms, this is never true when using
> >> CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP, and does not always works for
> >> CONFIG_SYS_USB_EVENT_POLL and
> >> CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE since not all usb keyboards
> >> honor the usb_set_idle() command.
> >> 
> >> For CONFIG_SYS_USB_EVENT_POLL we must use usb_set_idle() since we do a
> >> blocking wait for the hid report, so if we do not tell the keyboard to
> >> send a hid report every 40ms even if nothing changes then we will block
> >> u-boot for 1s (the default u-boot usb interrupt packet timeout). Note
> >> that in this case on keyboards which do not support usb_set_idle() we
> >> loose and we actually get 1s latencies on other u-boot activities.
> >> 
> >> For the other poll-methods this commit stops using usb_set_idle() and
> >> instead repeats the last received hid-report every 40 ms as long as no
> >> new hid-report is received. This fixes key-repeat not working at all
> >> with CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP and fixes it not working
> >> with keyboards which do not implement usb_set_idle() when using
> >> CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE.
> >> 
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > Looks reasonable, I don't really care about the ifdef indent, but the
> > data type for get_timer() should be fixed. Otherwise,
> 
> OK v2 with last_report changed to unsigned_long is coming up.
> 
> > Reviewed-by: Marek Vasut <marex@denx.de>
> > 
> > Would this finally be a patch to pick through the USB tree ? :b
> 
> Yes!  :)

It's been getting quite boring in u-boot-usb, thanks ;-)

Best regards,
Marek Vasut