[RESEND][PATCH 0/3] Raspberry pi improvements usb core
These patches are now in use by other developers, and I would like to get them into the upstream u-boot. This 3 patches were part of a 9 patch series posted in July prior to the merge window closure.
-- Travis CI checks https://github.com/u-boot/u-boot/pull/35 --
Pull Request #35 Rpi master
The commit 57805f2270c4 ("net: bcmgenet: Don't set ID_MODE_DIS when not using RGMII") needed to be extended for the case of using the rgmii-rxid. The latest version of the Rasbperry Pi4 dtb files for the 5.4 now specify the rgmii-rxid.
Signed-off-by: Jason Wessel jason.wessel@windriver.com
Commit 9a21770 #35: Rpi master Branch master
-- End Travis CI checks --
Jason Wessel (3): usb_kbd: succeed even if no interrupt is pending common/usb.c: Work around keyboard reporting "USB device not accepting new address" usb.c: Add a retry in the usb_prepare_device()
common/usb.c | 16 +++++++++++++--- common/usb_kbd.c | 4 +++- 2 files changed, 16 insertions(+), 4 deletions(-)
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.
Signed-off-by: Jason Wessel jason.wessel@windriver.com --- common/usb_kbd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index b316807844..3c0056e1b9 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -519,7 +519,9 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum) 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) { + data->intinterval, true) < 0) { + /* Read first packet if the device provides it, else pick it up later */ + return 1; #endif printf("Failed to get keyboard state from device %04x:%04x\n", dev->descriptor.idVendor, dev->descriptor.idProduct);
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.
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@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@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;
On 8/18/20 6:54 PM, Jason Wessel wrote:
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.
That's pretty awful and confusing code then. The original code wasn't stellar either, but can this be somehow improved now ?
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.
I think it's better to make it more readable if possible.
Would the following be acceptable, which I can put in the next version.
commit 319c75ee3d1fce8a3389d1de857751504b5110cb (HEAD -> master) Author: Jason Wessel jason.wessel@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
So I have to wonder, if the keyboard never returns a data packet until you press a key (that makes sense), how does Linux deal with this ?
On 8/18/20 12:20 PM, Marek Vasut wrote:
On 8/18/20 6:54 PM, Jason Wessel wrote:
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.
That's pretty awful and confusing code then. The original code wasn't stellar either, but can this be somehow improved now ?
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.
I think it's better to make it more readable if possible.
Would the following be acceptable, which I can put in the next version.
commit 319c75ee3d1fce8a3389d1de857751504b5110cb (HEAD -> master) Author: Jason Wessel jason.wessel@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:1009So I have to wonder, if the keyboard never returns a data packet until you press a key (that makes sense), how does Linux deal with this ?
As far as I can tell the usb_kbd_probe() probe function in the Linux kernel sets up the configuration and exits right away. The keyboard drivers state was zeroed out in the probe function and the kernel later processes callbacks from the interrupt handler.
Does that imply the other 2 code paths should also just return 1 and we get rid of the printf() entirely?
I don't have any boards that wanted either of these two paths through code, so I wasn't inclined to change it.
Jason.
On 8/18/20 8:47 PM, Jason Wessel wrote:
On 8/18/20 12:20 PM, Marek Vasut wrote:
On 8/18/20 6:54 PM, Jason Wessel wrote:
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.
That's pretty awful and confusing code then. The original code wasn't stellar either, but can this be somehow improved now ?
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.
I think it's better to make it more readable if possible.
Would the following be acceptable, which I can put in the next version.
commit 319c75ee3d1fce8a3389d1de857751504b5110cb (HEAD -> master) Author: Jason Wessel jason.wessel@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:1009So I have to wonder, if the keyboard never returns a data packet until you press a key (that makes sense), how does Linux deal with this ?
As far as I can tell the usb_kbd_probe() probe function in the Linux kernel sets up the configuration and exits right away. The keyboard drivers state was zeroed out in the probe function and the kernel later processes callbacks from the interrupt handler.
Why can't we return right away and why do we poll/wait then ?
Does that imply the other 2 code paths should also just return 1 and we get rid of the printf() entirely?
I don't have any boards that wanted either of these two paths through code, so I wasn't inclined to change it.
I'd say, change it, it'd go in ~October and the next release is in 3 months from October anyway.
When resetting the rpi3 board sometimes it will display: USB device not accepting new address (error=0)
After the message appears, the usb keyboard will not work. It seems that the configuration actually did succeed however. Checking the device status for a return code of zero and continuing allows the usb keyboard and other usb devices to work function.
Signed-off-by: Jason Wessel jason.wessel@windriver.com --- common/usb.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/common/usb.c b/common/usb.c index aad13fd9c5..0eb5d40a2d 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1054,11 +1054,12 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read, dev->devnum = addr;
err = usb_set_address(dev); /* set address */ - - if (err < 0) { + if (err < 0) + debug("\n usb_set_address return < 0\n"); + if (err < 0 && dev->status != 0) { printf("\n USB device not accepting new address " \ "(error=%lX)\n", dev->status); - return err; + return err; }
mdelay(10); /* Let the SET_ADDRESS settle */
On 8/18/20 4:34 PM, Jason Wessel wrote:
When resetting the rpi3 board sometimes it will display: USB device not accepting new address (error=0)
After the message appears, the usb keyboard will not work. It seems that the configuration actually did succeed however. Checking the device status for a return code of zero and continuing allows the usb keyboard and other usb devices to work function.
Signed-off-by: Jason Wessel jason.wessel@windriver.com
common/usb.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/common/usb.c b/common/usb.c index aad13fd9c5..0eb5d40a2d 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1054,11 +1054,12 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read, dev->devnum = addr;
err = usb_set_address(dev); /* set address */
- if (err < 0) {
- if (err < 0)
debug("\n usb_set_address return < 0\n");
Please print the return code here.
Is dev->status always set by usb_set_address() when err < 0 ?
- if (err < 0 && dev->status != 0) { printf("\n USB device not accepting new address " \ "(error=%lX)\n", dev->status);
return err;
return err;}
mdelay(10); /* Let the SET_ADDRESS settle */
On 8/18/20 10:08 AM, Marek Vasut wrote:
On 8/18/20 4:34 PM, Jason Wessel wrote:
When resetting the rpi3 board sometimes it will display: USB device not accepting new address (error=0)
After the message appears, the usb keyboard will not work. It seems that the configuration actually did succeed however. Checking the device status for a return code of zero and continuing allows the usb keyboard and other usb devices to work function.
Signed-off-by: Jason Wessel jason.wessel@windriver.com
common/usb.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/common/usb.c b/common/usb.c index aad13fd9c5..0eb5d40a2d 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1054,11 +1054,12 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read, dev->devnum = addr;
err = usb_set_address(dev); /* set address */
- if (err < 0) {
- if (err < 0)
debug("\n usb_set_address return < 0\n");Please print the return code here.
Good idea.
Is dev->status always set by usb_set_address() when err < 0 ?
Yes. The status is filled in as far as I can tell. I had received other values when exceeding timing thresholds with too many printfs() to the serial port while debugging.
The usb hardware hardware devices seem to like their initialization to be completed in a timely manner.
Jason.
- if (err < 0 && dev->status != 0) { printf("\n USB device not accepting new address " \ "(error=%lX)\n", dev->status);
return err;
return err;}
mdelay(10); /* Let the SET_ADDRESS settle */
On 8/18/20 8:16 PM, Jason Wessel wrote:
On 8/18/20 10:08 AM, Marek Vasut wrote:
On 8/18/20 4:34 PM, Jason Wessel wrote:
When resetting the rpi3 board sometimes it will display: USB device not accepting new address (error=0)
After the message appears, the usb keyboard will not work. It seems that the configuration actually did succeed however. Checking the device status for a return code of zero and continuing allows the usb keyboard and other usb devices to work function.
Signed-off-by: Jason Wessel jason.wessel@windriver.com
common/usb.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/common/usb.c b/common/usb.c index aad13fd9c5..0eb5d40a2d 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1054,11 +1054,12 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read, dev->devnum = addr;
err = usb_set_address(dev); /* set address */
- if (err < 0) {
- if (err < 0)
debug("\n usb_set_address return < 0\n");Please print the return code here.
Good idea.
Is dev->status always set by usb_set_address() when err < 0 ?
Yes. The status is filled in as far as I can tell. I had received other values when exceeding timing thresholds with too many printfs() to the serial port while debugging.
The usb hardware hardware devices seem to like their initialization to be completed in a timely manner.
I'd say, init the variable before usb_set_address with 0. All USB_ST_* are non-zero, so no matter what usb_set_address() does, usb_control_msg() will set dev->status to != 0, so the check is always true then.
I have found through testing some USB 2 composite mouse/keyboard devices do not response to the usb_set_address call immediately following the port reset. It can take anywhere from 2ms to 20ms.
This patch adds a retry and delay for usb_prepare_device() and allows all the USB keyboards I tried to function properly.
Signed-off-by: Jason Wessel jason.wessel@windriver.com --- common/usb.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/common/usb.c b/common/usb.c index 0eb5d40a2d..39bae86a11 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1032,6 +1032,7 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read, struct usb_device *parent) { int err; + int retry_msec = 0;
/* * Allocate usb 3.0 device context. @@ -1054,6 +1055,14 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read, dev->devnum = addr;
err = usb_set_address(dev); /* set address */ + /* Retry for old composite keyboard/mouse usb2 hardware */ + while (err < 0 && retry_msec <= 40) { + retry_msec += 20; + mdelay(20); + err = usb_set_address(dev); /* set address */ + } + if (retry_msec > 0) + debug("usb_set_address delay: %i\n", retry_msec); if (err < 0) debug("\n usb_set_address return < 0\n"); if (err < 0 && dev->status != 0) {
On 8/18/20 4:34 PM, Jason Wessel wrote:
I have found through testing some USB 2 composite mouse/keyboard devices do not response to the usb_set_address call immediately following the port reset. It can take anywhere from 2ms to 20ms.
Does it work if you do
=> setenv usb_pgood_delay 2000
before your test ?
From the Raspberry Pi3 - With the patch removed:
U-Boot> setenv usb_pgood_delay 2000 U-Boot> usb reset resetting USB... Bus usb@7e980000: USB DWC2 scanning bus usb@7e980000 for devices... unable to get device descriptor (error=-22) 6 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found
The only way I found to make it work reliably was to have it retry in the usb_prepare_device().
Jason.
On 8/18/20 10:08 AM, Marek Vasut wrote:
On 8/18/20 4:34 PM, Jason Wessel wrote:
I have found through testing some USB 2 composite mouse/keyboard devices do not response to the usb_set_address call immediately following the port reset. It can take anywhere from 2ms to 20ms.
Does it work if you do
=> setenv usb_pgood_delay 2000
before your test ?
On 8/18/20 8:01 PM, Jason Wessel wrote:
From the Raspberry Pi3 - With the patch removed:
U-Boot> setenv usb_pgood_delay 2000 U-Boot> usb reset resetting USB... Bus usb@7e980000: USB DWC2 scanning bus usb@7e980000 for devices... unable to get device descriptor (error=-22) 6 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found
The only way I found to make it work reliably was to have it retry in the usb_prepare_device().
Jason.
On 8/18/20 10:08 AM, Marek Vasut wrote:
On 8/18/20 4:34 PM, Jason Wessel wrote:
I have found through testing some USB 2 composite mouse/keyboard devices do not response to the usb_set_address call immediately following the port reset. It can take anywhere from 2ms to 20ms.
Does it work if you do
=> setenv usb_pgood_delay 2000
before your test ?
Please stop top-posting.
Can you check whether the pgood_delay isn't EHCI-only ? It does sure look similar to the problems which pgood_delay was supposed to help with.
On 8/18/20 4:34 PM, Jason Wessel wrote:
These patches are now in use by other developers, and I would like to get them into the upstream u-boot. This 3 patches were part of a 9 patch series posted in July prior to the merge window closure.
-- Travis CI checks https://github.com/u-boot/u-boot/pull/35 --
Pull Request #35 Rpi master
The commit 57805f2270c4 ("net: bcmgenet: Don't set ID_MODE_DIS when not using RGMII") needed to be extended for the case of using the rgmii-rxid. The latest version of the Rasbperry Pi4 dtb files for the 5.4 now specify the rgmii-rxid.
OK, so this is some networking patch series ?
participants (2)
-
Jason Wessel -
Marek Vasut