[U-Boot] [PATCH 0/7] Add a better USB keyboard polling method

[U-Boot] [PATCH 0/7] Add a better USB keyboard polling method

[Hans de Goede]

Hi Marek,

This time a patch-set for next :)

Currently one can choose between 2 poll methods for usb keyboards, both of
which are suboptimal. One option is to use control messages to get reports,
which some devices (e.g. my kvm) do not like. The other option is to use
interrupt urbs, but usb_submit_int_msg waits for the interrupt packet to
show up, meaning that each poll takes 40 ms, slowing anything else down
tremendously.

This patch-sets adds a third method (only usable with ehci for now), which
makes use of the int_queue concept in the ehci code. This allows us to submit
an interrupt message, and then poll for the actual completion of this message
giving us much lower latency then even the control message method (effectively
this gives us 0 latency), while using standard interrupt messages which seems
to keep keyboards much happier.

Regards,

Hans

[U-Boot] [PATCH 1/7] usb: ehci: Do not disable an already disabled periodic schedule

[Hans de Goede]

When periodic_schedules == 0, the schedule is disabled and there is no reason
to disable it again.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/ehci-hcd.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 6323c50..20830d7 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1258,9 +1258,11 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize,
 			   ALIGN_END_ADDR(struct qTD, result->tds,
 					  queuesize));
 
-	if (disable_periodic(ctrl) < 0) {
-		debug("FATAL: periodic should never fail, but did");
-		goto fail3;
+	if (ctrl->periodic_schedules > 0) {
+		if (disable_periodic(ctrl) < 0) {
+			debug("FATAL: periodic should never fail, but did");
+			goto fail3;
+		}
 	}
 
 	/* hook up to periodic list */
-- 
2.1.0

[U-Boot] [PATCH 2/7] usb: ehci: Move interrupt packet length check to create_int_queue

[Hans de Goede]

Preperation patch to use create_int_queue outside of ehci-hcd.c .

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/ehci-hcd.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 20830d7..cf3e3c0 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1163,6 +1163,23 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize,
 	struct int_queue *result = NULL;
 	int i;
 
+	/*
+	 * Interrupt transfers requiring several transactions are not supported
+	 * because bInterval is ignored.
+	 *
+	 * Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2
+	 * <= PKT_ALIGN if several qTDs are required, while the USB
+	 * specification does not constrain this for interrupt transfers. That
+	 * means that ehci_submit_async() would support interrupt transfers
+	 * requiring several transactions only as long as the transfer size does
+	 * not require more than a single qTD.
+	 */
+	if (elementsize > usb_maxpacket(dev, pipe)) {
+		printf("%s: xfers requiring several transactions are not supported.\n",
+		       __func__);
+		return NULL;
+	}
+
 	debug("Enter create_int_queue\n");
 	if (usb_pipetype(pipe) != PIPE_INTERRUPT) {
 		debug("non-interrupt pipe (type=%lu)", usb_pipetype(pipe));
@@ -1384,24 +1401,9 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 	debug("dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d",
 	      dev, pipe, buffer, length, interval);
 
-	/*
-	 * Interrupt transfers requiring several transactions are not supported
-	 * because bInterval is ignored.
-	 *
-	 * Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2
-	 * <= PKT_ALIGN if several qTDs are required, while the USB
-	 * specification does not constrain this for interrupt transfers. That
-	 * means that ehci_submit_async() would support interrupt transfers
-	 * requiring several transactions only as long as the transfer size does
-	 * not require more than a single qTD.
-	 */
-	if (length > usb_maxpacket(dev, pipe)) {
-		printf("%s: Interrupt transfers requiring several "
-			"transactions are not supported.\n", __func__);
-		return -1;
-	}
-
 	queue = create_int_queue(dev, pipe, 1, length, buffer);
+	if (!queue)
+		return -1;
 
 	timeout = get_timer(0) + USB_TIMEOUT_MS(pipe);
 	while ((backbuffer = poll_int_queue(dev, queue)) == NULL)
-- 
2.1.0

[U-Boot] [PATCH 2/7] usb: ehci: Move interrupt packet length check to create_int_queue

[Michael Trimarchi]

Hi

On Sat, Sep 20, 2014 at 5:01 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Preperation patch to use create_int_queue outside of ehci-hcd.c .
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/host/ehci-hcd.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 20830d7..cf3e3c0 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -1163,6 +1163,23 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize,
>         struct int_queue *result = NULL;
>         int i;
>
> +       /*
> +        * Interrupt transfers requiring several transactions are not supported
> +        * because bInterval is ignored.
> +        *
> +        * Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2
> +        * <= PKT_ALIGN if several qTDs are required, while the USB
> +        * specification does not constrain this for interrupt transfers. That
> +        * means that ehci_submit_async() would support interrupt transfers
> +        * requiring several transactions only as long as the transfer size does
> +        * not require more than a single qTD.
> +        */
> +       if (elementsize > usb_maxpacket(dev, pipe)) {
> +               printf("%s: xfers requiring several transactions are not supported.\n",
> +                      __func__);
> +               return NULL;
> +       }
> +
>         debug("Enter create_int_queue\n");
>         if (usb_pipetype(pipe) != PIPE_INTERRUPT) {
>                 debug("non-interrupt pipe (type=%lu)", usb_pipetype(pipe));
> @@ -1384,24 +1401,9 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
>         debug("dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d",
>               dev, pipe, buffer, length, interval);
>
> -       /*
> -        * Interrupt transfers requiring several transactions are not supported
> -        * because bInterval is ignored.
> -        *
> -        * Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2
> -        * <= PKT_ALIGN if several qTDs are required, while the USB
> -        * specification does not constrain this for interrupt transfers. That
> -        * means that ehci_submit_async() would support interrupt transfers
> -        * requiring several transactions only as long as the transfer size does
> -        * not require more than a single qTD.
> -        */
> -       if (length > usb_maxpacket(dev, pipe)) {
> -               printf("%s: Interrupt transfers requiring several "
> -                       "transactions are not supported.\n", __func__);
> -               return -1;
> -       }
> -
>         queue = create_int_queue(dev, pipe, 1, length, buffer);
> +       if (!queue)
> +               return -1;

Can you return a more consistent error code?

Michael

>
>         timeout = get_timer(0) + USB_TIMEOUT_MS(pipe);
>         while ((backbuffer = poll_int_queue(dev, queue)) == NULL)
> --
> 2.1.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 2/7] usb: ehci: Move interrupt packet length check to create_int_queue

[Hans de Goede]

Hi,

On 09/20/2014 07:42 PM, Michael Trimarchi wrote:
> Hi
> 
> On Sat, Sep 20, 2014 at 5:01 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Preperation patch to use create_int_queue outside of ehci-hcd.c .
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/usb/host/ehci-hcd.c | 36 +++++++++++++++++++-----------------
>>  1 file changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>> index 20830d7..cf3e3c0 100644
>> --- a/drivers/usb/host/ehci-hcd.c
>> +++ b/drivers/usb/host/ehci-hcd.c
>> @@ -1163,6 +1163,23 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize,
>>         struct int_queue *result = NULL;
>>         int i;
>>
>> +       /*
>> +        * Interrupt transfers requiring several transactions are not supported
>> +        * because bInterval is ignored.
>> +        *
>> +        * Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2
>> +        * <= PKT_ALIGN if several qTDs are required, while the USB
>> +        * specification does not constrain this for interrupt transfers. That
>> +        * means that ehci_submit_async() would support interrupt transfers
>> +        * requiring several transactions only as long as the transfer size does
>> +        * not require more than a single qTD.
>> +        */
>> +       if (elementsize > usb_maxpacket(dev, pipe)) {
>> +               printf("%s: xfers requiring several transactions are not supported.\n",
>> +                      __func__);
>> +               return NULL;
>> +       }
>> +
>>         debug("Enter create_int_queue\n");
>>         if (usb_pipetype(pipe) != PIPE_INTERRUPT) {
>>                 debug("non-interrupt pipe (type=%lu)", usb_pipetype(pipe));
>> @@ -1384,24 +1401,9 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
>>         debug("dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d",
>>               dev, pipe, buffer, length, interval);
>>
>> -       /*
>> -        * Interrupt transfers requiring several transactions are not supported
>> -        * because bInterval is ignored.
>> -        *
>> -        * Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2
>> -        * <= PKT_ALIGN if several qTDs are required, while the USB
>> -        * specification does not constrain this for interrupt transfers. That
>> -        * means that ehci_submit_async() would support interrupt transfers
>> -        * requiring several transactions only as long as the transfer size does
>> -        * not require more than a single qTD.
>> -        */
>> -       if (length > usb_maxpacket(dev, pipe)) {
>> -               printf("%s: Interrupt transfers requiring several "
>> -                       "transactions are not supported.\n", __func__);
>> -               return -1;
>> -       }
>> -
>>         queue = create_int_queue(dev, pipe, 1, length, buffer);
>> +       if (!queue)
>> +               return -1;
> 
> Can you return a more consistent error code?

I'm just moving code around, and returning the same error code as before. Surely
changing the error code belongs in another patch ?

Regards,

Hans

[U-Boot] [PATCH 2/7] usb: ehci: Move interrupt packet length check to create_int_queue

[Marek Vasut]

On Sunday, September 21, 2014 at 07:53:35 PM, Hans de Goede wrote:
> Hi,
[...]
> >> -       if (length > usb_maxpacket(dev, pipe)) {
> >> -               printf("%s: Interrupt transfers requiring several "
> >> -                       "transactions are not supported.\n", __func__);
> >> -               return -1;
> >> -       }
> >> -
> >> 
> >>         queue = create_int_queue(dev, pipe, 1, length, buffer);
> >> 
> >> +       if (!queue)
> >> +               return -1;
> > 
> > Can you return a more consistent error code?
> 
> I'm just moving code around, and returning the same error code as before.
> Surely changing the error code belongs in another patch ?

Yes, full ACK. This is exactly a prime examply where squashing two fixes into 
one patch would break bisectability absolutely perfectly.

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/7] usb: ehci: Move interrupt packet length check to create_int_queue

[Michael Trimarchi]

Hi Marek

On Sun, Sep 21, 2014 at 9:36 PM, Marek Vasut <marex@denx.de> wrote:
> On Sunday, September 21, 2014 at 07:53:35 PM, Hans de Goede wrote:
>> Hi,
> [...]
>> >> -       if (length > usb_maxpacket(dev, pipe)) {
>> >> -               printf("%s: Interrupt transfers requiring several "
>> >> -                       "transactions are not supported.\n", __func__);
>> >> -               return -1;
>> >> -       }
>> >> -
>> >>
>> >>         queue = create_int_queue(dev, pipe, 1, length, buffer);
>> >>
>> >> +       if (!queue)
>> >> +               return -1;
>> >
>> > Can you return a more consistent error code?
>>
>> I'm just moving code around, and returning the same error code as before.
>> Surely changing the error code belongs in another patch ?
>
> Yes, full ACK. This is exactly a prime examply where squashing two fixes into
> one patch would break bisectability absolutely perfectly.
>

Agree on separated patch, I have just ask if Hans can do in the
patches queue. Marek, thanks for the lesson. Anyway seems that in USB
part we have already several -1 return.

Michael


> Best regards,
> Marek Vasut

[U-Boot] [PATCH 2/7] usb: ehci: Move interrupt packet length check to create_int_queue

[Marek Vasut]

On Sunday, September 21, 2014 at 10:00:24 PM, Michael Trimarchi wrote:
> Hi Marek
> 
> On Sun, Sep 21, 2014 at 9:36 PM, Marek Vasut <marex@denx.de> wrote:
> > On Sunday, September 21, 2014 at 07:53:35 PM, Hans de Goede wrote:
> >> Hi,
> > 
> > [...]
> > 
> >> >> -       if (length > usb_maxpacket(dev, pipe)) {
> >> >> -               printf("%s: Interrupt transfers requiring several "
> >> >> -                       "transactions are not supported.\n",
> >> >> __func__); -               return -1;
> >> >> -       }
> >> >> -
> >> >> 
> >> >>         queue = create_int_queue(dev, pipe, 1, length, buffer);
> >> >> 
> >> >> +       if (!queue)
> >> >> +               return -1;
> >> > 
> >> > Can you return a more consistent error code?
> >> 
> >> I'm just moving code around, and returning the same error code as
> >> before. Surely changing the error code belongs in another patch ?
> > 
> > Yes, full ACK. This is exactly a prime examply where squashing two fixes
> > into one patch would break bisectability absolutely perfectly.
> 
> Agree on separated patch, I have just ask if Hans can do in the
> patches queue. Marek, thanks for the lesson. Anyway seems that in USB
> part we have already several -1 return.

You know how it goes, patches are welcome ;-)

Best regards,
Marek Vasut

[U-Boot] [PATCH 3/7] usb: ehci: Move cache invalidation to poll_int_queue

[Hans de Goede]

Preperation patch to use poll_int_queue outside of ehci-hcd.c .

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/ehci-hcd.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index cf3e3c0..e527a7a 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1106,6 +1106,7 @@ submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 }
 
 struct int_queue {
+	int elementsize;
 	struct QH *first;
 	struct QH *current;
 	struct QH *last;
@@ -1200,6 +1201,7 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize,
 		debug("ehci intr queue: out of memory\n");
 		goto fail1;
 	}
+	result->elementsize = elementsize;
 	result->first = memalign(USB_DMA_MINALIGN,
 				 sizeof(struct QH) * queuesize);
 	if (!result->first) {
@@ -1336,6 +1338,11 @@ void *poll_int_queue(struct usb_device *dev, struct int_queue *queue)
 		queue->current++;
 	else
 		queue->current = NULL;
+
+	invalidate_dcache_range((uint32_t)cur->buffer,
+				ALIGN_END_ADDR(char, cur->buffer,
+					       queue->elementsize));
+
 	debug("Exit poll_int_queue with completed intr transfer. token is %x at %p (first at %p)\n",
 	      hc32_to_cpu(cur_td->qt_token), cur, queue->first);
 	return cur->buffer;
@@ -1419,9 +1426,6 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 		return -EINVAL;
 	}
 
-	invalidate_dcache_range((uint32_t)buffer,
-				ALIGN_END_ADDR(char, buffer, length));
-
 	ret = destroy_int_queue(dev, queue);
 	if (ret < 0)
 		return ret;
-- 
2.1.0

[U-Boot] [PATCH 4/7] usb: Make pollable int support available outside of ehci-hcd.c

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/usb.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/usb.h b/include/usb.h
index c355fbe..6b29aed 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -129,6 +129,8 @@ struct usb_device {
 	unsigned int slot_id;
 };
 
+struct int_queue;
+
 /*
  * You can initialize platform's USB host or device
  * ports by passing this enum as an argument to
@@ -162,6 +164,13 @@ int submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 			int transfer_len, int interval);
 
+#ifdef CONFIG_USB_EHCI /* Only the ehci code has pollable int support */
+struct int_queue *create_int_queue(struct usb_device *dev, unsigned long pipe,
+	int queuesize, int elementsize, void *buffer);
+int destroy_int_queue(struct usb_device *dev, struct int_queue *queue);
+void *poll_int_queue(struct usb_device *dev, struct int_queue *queue);
+#endif
+
 /* Defines */
 #define USB_UHCI_VEND_ID	0x8086
 #define USB_UHCI_DEV_ID		0x7112
-- 
2.1.0

[U-Boot] [PATCH 5/7] usb: kbd: Remove unused usb_kbd_generic_poll function

[Hans de Goede]

This is not used anywhere, so lets remove it.

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

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index fdc083c..cb869ac 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -116,32 +116,6 @@ extern int __maybe_unused net_busy_flag;
 /* The period of time between two calls of usb_kbd_testc(). */
 static unsigned long __maybe_unused kbd_testc_tms;
 
-/* Generic keyboard event polling. */
-void usb_kbd_generic_poll(void)
-{
-	struct stdio_dev *dev;
-	struct usb_device *usb_kbd_dev;
-	struct usb_kbd_pdata *data;
-	struct usb_interface *iface;
-	struct usb_endpoint_descriptor *ep;
-	int pipe;
-	int maxp;
-
-	/* Get the pointer to USB Keyboard device pointer */
-	dev = stdio_get_by_name(DEVNAME);
-	usb_kbd_dev = (struct usb_device *)dev->priv;
-	data = usb_kbd_dev->privptr;
-	iface = &usb_kbd_dev->config.if_desc[0];
-	ep = &iface->ep_desc[0];
-	pipe = usb_rcvintpipe(usb_kbd_dev, ep->bEndpointAddress);
-
-	/* Submit a interrupt transfer request */
-	maxp = usb_maxpacket(usb_kbd_dev, pipe);
-	usb_submit_int_msg(usb_kbd_dev, pipe, data->new,
-		min(maxp, USB_KBD_BOOT_REPORT_SIZE),
-		ep->bInterval);
-}
-
 /* Puts character in the queue and sets up the in and out pointer. */
 static void usb_kbd_put_queue(struct usb_kbd_pdata *data, char c)
 {
-- 
2.1.0

[U-Boot] [PATCH 6/7] usb: kbd: Cache pipe, interval and packetsize

[Hans de Goede]

Instead of looking them up every time we need them.

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

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index cb869ac..85ee1c8 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -99,6 +99,10 @@ static const unsigned char usb_kbd_arrow[] = {
 #define USB_KBD_BOOT_REPORT_SIZE 8
 
 struct usb_kbd_pdata {
+	unsigned long	intpipe;
+	int		intpktsize;
+	int		intinterval;
+
 	uint32_t	repeat_delay;
 
 	uint32_t	usb_in_pointer;
@@ -305,23 +309,11 @@ static int usb_kbd_irq(struct usb_device *dev)
 static inline void usb_kbd_poll_for_event(struct usb_device *dev)
 {
 #if	defined(CONFIG_SYS_USB_EVENT_POLL)
-	struct usb_interface *iface;
-	struct usb_endpoint_descriptor *ep;
-	struct usb_kbd_pdata *data;
-	int pipe;
-	int maxp;
-
-	/* Get the pointer to USB Keyboard device pointer */
-	data = dev->privptr;
-	iface = &dev->config.if_desc[0];
-	ep = &iface->ep_desc[0];
-	pipe = usb_rcvintpipe(dev, ep->bEndpointAddress);
+	struct usb_kbd_pdata *data = dev->privptr;
 
 	/* Submit a interrupt transfer request */
-	maxp = usb_maxpacket(dev, pipe);
-	usb_submit_int_msg(dev, pipe, &data->new[0],
-		min(maxp, USB_KBD_BOOT_REPORT_SIZE),
-		ep->bInterval);
+	usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,
+			   data->intinterval);
 
 	usb_kbd_irq_worker(dev);
 #elif	defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
@@ -389,7 +381,6 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
 	struct usb_interface *iface;
 	struct usb_endpoint_descriptor *ep;
 	struct usb_kbd_pdata *data;
-	int pipe, maxp;
 
 	if (dev->descriptor.bNumConfigurations != 1)
 		return 0;
@@ -438,8 +429,10 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
 	/* Set IRQ handler */
 	dev->irq_handle = usb_kbd_irq;
 
-	pipe = usb_rcvintpipe(dev, ep->bEndpointAddress);
-	maxp = usb_maxpacket(dev, pipe);
+	data->intpipe = usb_rcvintpipe(dev, ep->bEndpointAddress);
+	data->intpktsize = min(usb_maxpacket(dev, data->intpipe),
+			       USB_KBD_BOOT_REPORT_SIZE);
+	data->intinterval = ep->bInterval;
 
 	/* We found a USB Keyboard, install it. */
 	usb_set_protocol(dev, iface->desc.bInterfaceNumber, 0);
@@ -448,9 +441,8 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
 	usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
 
 	debug("USB KBD: enable interrupt pipe...\n");
-	if (usb_submit_int_msg(dev, pipe, data->new,
-			       min(maxp, USB_KBD_BOOT_REPORT_SIZE),
-			       ep->bInterval) < 0) {
+	if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,
+			       data->intinterval) < 0) {
 		printf("Failed to get keyboard state from device %04x:%04x\n",
 		       dev->descriptor.idVendor, dev->descriptor.idProduct);
 		/* Abort, we don't want to use that non-functional keyboard. */
-- 
2.1.0

[U-Boot] [PATCH 6/7] usb: kbd: Cache pipe, interval and packetsize

[Michael Trimarchi]

Hi

On Sat, Sep 20, 2014 at 5:01 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Instead of looking them up every time we need them.
>

split subject and patch description

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  common/usb_kbd.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index cb869ac..85ee1c8 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -99,6 +99,10 @@ static const unsigned char usb_kbd_arrow[] = {
>  #define USB_KBD_BOOT_REPORT_SIZE 8
>
>  struct usb_kbd_pdata {
> +       unsigned long   intpipe;

unsigned int intpipe ??

Michael

> +       int             intpktsize;
> +       int             intinterval;
> +
>         uint32_t        repeat_delay;
>
>         uint32_t        usb_in_pointer;
> @@ -305,23 +309,11 @@ static int usb_kbd_irq(struct usb_device *dev)
>  static inline void usb_kbd_poll_for_event(struct usb_device *dev)
>  {
>  #if    defined(CONFIG_SYS_USB_EVENT_POLL)
> -       struct usb_interface *iface;
> -       struct usb_endpoint_descriptor *ep;
> -       struct usb_kbd_pdata *data;
> -       int pipe;
> -       int maxp;
> -
> -       /* Get the pointer to USB Keyboard device pointer */
> -       data = dev->privptr;
> -       iface = &dev->config.if_desc[0];
> -       ep = &iface->ep_desc[0];
> -       pipe = usb_rcvintpipe(dev, ep->bEndpointAddress);
> +       struct usb_kbd_pdata *data = dev->privptr;
>
>         /* Submit a interrupt transfer request */
> -       maxp = usb_maxpacket(dev, pipe);
> -       usb_submit_int_msg(dev, pipe, &data->new[0],
> -               min(maxp, USB_KBD_BOOT_REPORT_SIZE),
> -               ep->bInterval);
> +       usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,
> +                          data->intinterval);
>
>         usb_kbd_irq_worker(dev);
>  #elif  defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
> @@ -389,7 +381,6 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
>         struct usb_interface *iface;
>         struct usb_endpoint_descriptor *ep;
>         struct usb_kbd_pdata *data;
> -       int pipe, maxp;
>
>         if (dev->descriptor.bNumConfigurations != 1)
>                 return 0;
> @@ -438,8 +429,10 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
>         /* Set IRQ handler */
>         dev->irq_handle = usb_kbd_irq;
>
> -       pipe = usb_rcvintpipe(dev, ep->bEndpointAddress);
> -       maxp = usb_maxpacket(dev, pipe);
> +       data->intpipe = usb_rcvintpipe(dev, ep->bEndpointAddress);
> +       data->intpktsize = min(usb_maxpacket(dev, data->intpipe),
> +                              USB_KBD_BOOT_REPORT_SIZE);
> +       data->intinterval = ep->bInterval;
>
>         /* We found a USB Keyboard, install it. */
>         usb_set_protocol(dev, iface->desc.bInterfaceNumber, 0);
> @@ -448,9 +441,8 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
>         usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
>
>         debug("USB KBD: enable interrupt pipe...\n");
> -       if (usb_submit_int_msg(dev, pipe, data->new,
> -                              min(maxp, USB_KBD_BOOT_REPORT_SIZE),
> -                              ep->bInterval) < 0) {
> +       if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,
> +                              data->intinterval) < 0) {
>                 printf("Failed to get keyboard state from device %04x:%04x\n",
>                        dev->descriptor.idVendor, dev->descriptor.idProduct);
>                 /* Abort, we don't want to use that non-functional keyboard. */
> --
> 2.1.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [PATCH 6/7] usb: kbd: Cache pipe, interval and packetsize

[Hans de Goede]

Hi,

On 09/20/2014 07:53 PM, Michael Trimarchi wrote:
> Hi
> 
> On Sat, Sep 20, 2014 at 5:01 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Instead of looking them up every time we need them.
>>
> 
> split subject and patch description
> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  common/usb_kbd.c | 34 +++++++++++++---------------------
>>  1 file changed, 13 insertions(+), 21 deletions(-)
>>
>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>> index cb869ac..85ee1c8 100644
>> --- a/common/usb_kbd.c
>> +++ b/common/usb_kbd.c
>> @@ -99,6 +99,10 @@ static const unsigned char usb_kbd_arrow[] = {
>>  #define USB_KBD_BOOT_REPORT_SIZE 8
>>
>>  struct usb_kbd_pdata {
>> +       unsigned long   intpipe;
> 
> unsigned int intpipe ??

usb "pipe"-s are unsigned long everywhere in u-boot.

Regards,

Hans

> 
> Michael
> 
>> +       int             intpktsize;
>> +       int             intinterval;
>> +
>>         uint32_t        repeat_delay;
>>
>>         uint32_t        usb_in_pointer;
>> @@ -305,23 +309,11 @@ static int usb_kbd_irq(struct usb_device *dev)
>>  static inline void usb_kbd_poll_for_event(struct usb_device *dev)
>>  {
>>  #if    defined(CONFIG_SYS_USB_EVENT_POLL)
>> -       struct usb_interface *iface;
>> -       struct usb_endpoint_descriptor *ep;
>> -       struct usb_kbd_pdata *data;
>> -       int pipe;
>> -       int maxp;
>> -
>> -       /* Get the pointer to USB Keyboard device pointer */
>> -       data = dev->privptr;
>> -       iface = &dev->config.if_desc[0];
>> -       ep = &iface->ep_desc[0];
>> -       pipe = usb_rcvintpipe(dev, ep->bEndpointAddress);
>> +       struct usb_kbd_pdata *data = dev->privptr;
>>
>>         /* Submit a interrupt transfer request */
>> -       maxp = usb_maxpacket(dev, pipe);
>> -       usb_submit_int_msg(dev, pipe, &data->new[0],
>> -               min(maxp, USB_KBD_BOOT_REPORT_SIZE),
>> -               ep->bInterval);
>> +       usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,
>> +                          data->intinterval);
>>
>>         usb_kbd_irq_worker(dev);
>>  #elif  defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
>> @@ -389,7 +381,6 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
>>         struct usb_interface *iface;
>>         struct usb_endpoint_descriptor *ep;
>>         struct usb_kbd_pdata *data;
>> -       int pipe, maxp;
>>
>>         if (dev->descriptor.bNumConfigurations != 1)
>>                 return 0;
>> @@ -438,8 +429,10 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
>>         /* Set IRQ handler */
>>         dev->irq_handle = usb_kbd_irq;
>>
>> -       pipe = usb_rcvintpipe(dev, ep->bEndpointAddress);
>> -       maxp = usb_maxpacket(dev, pipe);
>> +       data->intpipe = usb_rcvintpipe(dev, ep->bEndpointAddress);
>> +       data->intpktsize = min(usb_maxpacket(dev, data->intpipe),
>> +                              USB_KBD_BOOT_REPORT_SIZE);
>> +       data->intinterval = ep->bInterval;
>>
>>         /* We found a USB Keyboard, install it. */
>>         usb_set_protocol(dev, iface->desc.bInterfaceNumber, 0);
>> @@ -448,9 +441,8 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
>>         usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
>>
>>         debug("USB KBD: enable interrupt pipe...\n");
>> -       if (usb_submit_int_msg(dev, pipe, data->new,
>> -                              min(maxp, USB_KBD_BOOT_REPORT_SIZE),
>> -                              ep->bInterval) < 0) {
>> +       if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,
>> +                              data->intinterval) < 0) {
>>                 printf("Failed to get keyboard state from device %04x:%04x\n",
>>                        dev->descriptor.idVendor, dev->descriptor.idProduct);
>>                 /* Abort, we don't want to use that non-functional keyboard. */
>> --
>> 2.1.0
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
> 
> 
> 

[U-Boot] [PATCH 7/7] usb: kbd: Add (optional) support for using an interrupt queue for polling

[Hans de Goede]

Waiting an interrupt packet to complete in usb_kbd_poll_for_event, causes
a 40 ms latency for each call to usb_kbd_testc, which is undesirable.

Using control messages leads to lower (but still not 0) latency, but some
devices do not work well with control messages (e.g. my kvm behaves funny
with them).

This commit adds support for using the int_queue mechanism which at least
the ehci-hcd driver supports. This allows polling with 0 latency, while
using interrupt packets.

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

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 85ee1c8..278937c 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -102,6 +102,7 @@ struct usb_kbd_pdata {
 	unsigned long	intpipe;
 	int		intpktsize;
 	int		intinterval;
+	struct int_queue *intq;
 
 	uint32_t	repeat_delay;
 
@@ -324,6 +325,15 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
 		       1, 0, 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)
+	struct usb_kbd_pdata *data = dev->privptr;
+	if (poll_int_queue(dev, data->intq)) {
+		usb_kbd_irq_worker(dev);
+		/* We've consumed all queued int packets, create new */
+		destroy_int_queue(dev, data->intq);
+		data->intq = create_int_queue(dev, data->intpipe, 1,
+				      USB_KBD_BOOT_REPORT_SIZE, data->new);
+	}
 #endif
 }
 
@@ -441,8 +451,14 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
 	usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
 
 	debug("USB KBD: enable interrupt pipe...\n");
+#ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE
+	data->intq = create_int_queue(dev, data->intpipe, 1,
+				      USB_KBD_BOOT_REPORT_SIZE, data->new);
+	if (!data->intq) {
+#else
 	if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,
 			       data->intinterval) < 0) {
+#endif
 		printf("Failed to get keyboard state from device %04x:%04x\n",
 		       dev->descriptor.idVendor, dev->descriptor.idProduct);
 		/* Abort, we don't want to use that non-functional keyboard. */
@@ -515,13 +531,28 @@ int drv_usb_kbd_init(void)
 /* Deregister the keyboard. */
 int usb_kbd_deregister(int force)
 {
-#ifdef CONFIG_SYS_STDIO_DEREGISTER
+#if !defined(CONFIG_SYS_STDIO_DEREGISTER)
+	return 1;
+#elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
+	struct stdio_dev *dev;
+	struct usb_device *usb_kbd_dev;
+	struct usb_kbd_pdata *data;
+
+	dev = stdio_get_by_name(DEVNAME);
+	if (dev) {
+		if (stdio_deregister_dev(dev, force) != 0)
+			return 1;
+		usb_kbd_dev = (struct usb_device *)dev->priv;
+		data = usb_kbd_dev->privptr;
+		destroy_int_queue(usb_kbd_dev, data->intq);
+	}
+
+	return 0;
+#else
 	int ret = stdio_deregister(DEVNAME, force);
 	if (ret && ret != -ENODEV)
 		return ret;
 
 	return 0;
-#else
-	return 1;
 #endif
 }
-- 
2.1.0

[U-Boot] [PATCH 7/7] usb: kbd: Add (optional) support for using an interrupt queue for polling

[Hans de Goede]

Hi,

On 09/20/2014 05:01 PM, Hans de Goede wrote:
> Waiting an interrupt packet to complete in usb_kbd_poll_for_event, causes
> a 40 ms latency for each call to usb_kbd_testc, which is undesirable.
> 
> Using control messages leads to lower (but still not 0) latency, but some
> devices do not work well with control messages (e.g. my kvm behaves funny
> with them).
> 
> This commit adds support for using the int_queue mechanism which at least
> the ehci-hcd driver supports. This allows polling with 0 latency, while
> using interrupt packets.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  common/usb_kbd.c | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 85ee1c8..278937c 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -102,6 +102,7 @@ struct usb_kbd_pdata {
>  	unsigned long	intpipe;
>  	int		intpktsize;
>  	int		intinterval;
> +	struct int_queue *intq;
>  
>  	uint32_t	repeat_delay;
>  
> @@ -324,6 +325,15 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
>  		       1, 0, 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)
> +	struct usb_kbd_pdata *data = dev->privptr;
> +	if (poll_int_queue(dev, data->intq)) {
> +		usb_kbd_irq_worker(dev);
> +		/* We've consumed all queued int packets, create new */
> +		destroy_int_queue(dev, data->intq);
> +		data->intq = create_int_queue(dev, data->intpipe, 1,
> +				      USB_KBD_BOOT_REPORT_SIZE, data->new);
> +	}
>  #endif
>  }
>  
> @@ -441,8 +451,14 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
>  	usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
>  
>  	debug("USB KBD: enable interrupt pipe...\n");
> +#ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE
> +	data->intq = create_int_queue(dev, data->intpipe, 1,
> +				      USB_KBD_BOOT_REPORT_SIZE, data->new);
> +	if (!data->intq) {
> +#else
>  	if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,
>  			       data->intinterval) < 0) {
> +#endif
>  		printf("Failed to get keyboard state from device %04x:%04x\n",
>  		       dev->descriptor.idVendor, dev->descriptor.idProduct);
>  		/* Abort, we don't want to use that non-functional keyboard. */
> @@ -515,13 +531,28 @@ int drv_usb_kbd_init(void)
>  /* Deregister the keyboard. */
>  int usb_kbd_deregister(int force)
>  {
> -#ifdef CONFIG_SYS_STDIO_DEREGISTER
> +#if !defined(CONFIG_SYS_STDIO_DEREGISTER)
> +	return 1;
> +#elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
> +	struct stdio_dev *dev;
> +	struct usb_device *usb_kbd_dev;
> +	struct usb_kbd_pdata *data;
> +
> +	dev = stdio_get_by_name(DEVNAME);
> +	if (dev) {
> +		if (stdio_deregister_dev(dev, force) != 0)
> +			return 1;
> +		usb_kbd_dev = (struct usb_device *)dev->priv;

Hmm I've just realized that this is a use after free for dev.
I've fixed this in my local tree. I'll wait for review of the
other patches in this set before sending a v2.

Note this is not really a use after free, since stdio_deregister_dev
does not free dev, but it reallu should free dev, since stdio_register_dev
does a clone of the passed in dev, so stdio_deregister_dev should free it,
so as to not leak memory. Once that memory leak gets fixed, this will be
a use after free.

Likewise usb_kbd.c should free usb_kbd_pdata and the data->new buffer on
deregister. I'll add a fix plugging the memleak in usb_kbd.c to v2 of this
set.

Regards,

Hans

[U-Boot] [PATCH 0/7] Add a better USB keyboard polling method

[Marek Vasut]

On Saturday, September 20, 2014 at 05:01:05 PM, Hans de Goede wrote:
> Hi Marek,
> 
> This time a patch-set for next :)
> 
> Currently one can choose between 2 poll methods for usb keyboards, both of
> which are suboptimal. One option is to use control messages to get reports,
> which some devices (e.g. my kvm) do not like. The other option is to use
> interrupt urbs, but usb_submit_int_msg waits for the interrupt packet to
> show up, meaning that each poll takes 40 ms, slowing anything else down
> tremendously.
> 
> This patch-sets adds a third method (only usable with ehci for now), which
> makes use of the int_queue concept in the ehci code. This allows us to
> submit an interrupt message, and then poll for the actual completion of
> this message giving us much lower latency then even the control message
> method (effectively this gives us 0 latency), while using standard
> interrupt messages which seems to keep keyboards much happier.

I'd be happy to just pick V2 as it would be. The patches look really nice.

Best regards,
Marek Vasut