[RESEND][PATCH 1/3] usb_kbd: succeed even if no interrupt is pending

[Jason Wessel]

On 8/18/20 10:05 AM, Marek Vasut wrote:
> On 8/18/20 4:34 PM, Jason Wessel wrote:
>> After the initial configuration some USB keyboard+mouse devices never
>> return any kind of event on the interrupt line.  In particular, the
>> device identified by "Cypress Cypress USB Keyboard / PS2 Mouse as
>> 3f980000.usb/usb1/1-1/1-1.3/1-1.3:1.0/0003:04B4:0101.0001/input/input0"
>> never returns a data packet until the first external input event.
>>
>> I found this was also true with some newer model Dell keyboards.
>>
>> When the device is plugged into a xhci controller there is also no
>> point in waiting 5 seconds for a device that is never going to present
>> data, so the call to the interrupt service was changed to a
>> nonblocking operation for the controllers that support this.
>>
>> With the patch applied, the rpi3 and rpi4 work well with the more
>> complex keyboard devices.
> 
> Please extend the comment in the code, it is not clear from it or from
> the commit message what the problem really is that this patch tries to fix.
> 
> Also, the printf() below the return 1 is never reached.
> 


The printf() is only reached in the case of the #ifdef above it being true. 

The compiler in theory should optimize it away, but for clarity it can be moved 
with in the #ifdef but that also requires fixing it in two places because 
there are multiple levels to the #ifdef.

Would the following be acceptable, which I can put in the next version.


commit 319c75ee3d1fce8a3389d1de857751504b5110cb (HEAD -> master)
Author: Jason Wessel <jason.wessel at windriver.com>
Date:   Thu Jun 25 05:31:25 2020 -0700

    usb_kbd: succeed even if no interrupt is pending
    
    After the initial configuration some USB keyboard+mouse devices never
    return any kind of event on the interrupt line.  In particular, the
    device identified by "Cypress Cypress USB Keyboard / PS2 Mouse as
    3f980000.usb/usb1/1-1/1-1.3/1-1.3:1.0/0003:04B4:0101.0001/input/input0"
    never returns a data packet until the first external input event.
    
    I found this was also true with some newer model Dell keyboards.
    
    Without the patch u-boot prints the following on one of keyboards and
    leave it unable to accept input events.
    
    =====
    scanning bus xhci_pci for devices...
      Failed to get keyboard state from device 14dd:1009
    =====
    
    With the patch, all families of the Raspberry Pi boards can use this
    keyboard device.
    
    Signed-off-by: Jason Wessel <jason.wessel at windriver.com>

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index b316807844..9ff008d5dc 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -514,18 +514,28 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum)
                                      USB_KBD_BOOT_REPORT_SIZE, data->new,
                                      data->intinterval);
        if (!data->intq) {
+               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. */
+               return 0;
+       }
 #elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
        if (usb_get_report(dev, iface->desc.bInterfaceNumber,
                           1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE) < 0) {
-#else
-       if (usb_int_msg(dev, data->intpipe, data->new, data->intpktsize,
-                       data->intinterval, false) < 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. */
                return 0;
        }
+#else
+       /*
+        * Set poll to true for initial interrupt transfer
+        * because some keyboard devices do not return an
+        * event until the first keypress.
+        */
+       usb_int_msg(dev, data->intpipe, data->new, data->intpktsize,
+                   data->intinterval, true);
+#endif
 
        /* Success. */
        return 1;