Hi Gerd,
Have you had a chance to review the XHCI branch I've been testing? https://github.com/KevinOConnor/seabios/commits/xhci-testing
Beyond a general review, I have a few questions on the XHCI code:
1 - It looks like usb-xhci.c is missing a copyright. Are you okay if I add one that looks like what you have on ahci.c?
2 - What is the mdelay(100) in configure_xhci() for? I don't understand the comment there. Also, any delay should probably be an msleep(), so that seabios can continue to bring up other devices. (An mdelay() doesn't let other threads run.)
3 - In the original code xhci_hub_disconnect() free'd the xhci slot for a device, but this doesn't look right to me as there can be more than one slot for a given port (in the case of a hub). The testing branch above never free's slots now - do you have any thoughts on how slot allocation/free'ing should be done?
4 - In the testing branch above, I added a patch to make hubs work (see "xhci: Hack to get hubs working."). This further overloads xhci_alloc_pipe() though. Do you think that is okay or do you think there is a better way to catch hub registration so that the xhci controller knows about it?
-Kevin
On Mi, 2014-01-22 at 17:52 -0500, Kevin O'Connor wrote:
Hi Gerd,
Have you had a chance to review the XHCI branch I've been testing? https://github.com/KevinOConnor/seabios/commits/xhci-testing
I'll finally have a look later today.
Beyond a general review, I have a few questions on the XHCI code:
1 - It looks like usb-xhci.c is missing a copyright. Are you okay if I add one that looks like what you have on ahci.c?
Yes.
2 - What is the mdelay(100) in configure_xhci() for? I don't understand the comment there. Also, any delay should probably be an msleep(), so that seabios can continue to bring up other devices. (An mdelay() doesn't let other threads run.)
It gives the xhci controller some time to detect the usb devices.
A real os would just enable port status notification events, then do a scan once (without delay), and depend on the irq firing for ports not ready yet on the initial scan.
The seabios driver has no irq handler so we can't do that, and I've settled for the somewhat hackish delay. And, yes, it should be a msleep.
3 - In the original code xhci_hub_disconnect() free'd the xhci slot for a device, but this doesn't look right to me as there can be more than one slot for a given port (in the case of a hub). The testing branch above never free's slots now - do you have any thoughts on how slot allocation/free'ing should be done?
I think the original code is correct. Slots are for devices, not root hub ports. Of course the slot for the usb hub must stay active in case seabios handles devices connected to the hub, but I'm sure this is the case as seabios doesn't disconnect these hubs.
4 - In the testing branch above, I added a patch to make hubs work (see "xhci: Hack to get hubs working."). This further overloads xhci_alloc_pipe() though. Do you think that is okay or do you think there is a better way to catch hub registration so that the xhci controller knows about it?
Need to look at the patch.
cheers, Gerd
On Do, 2014-01-23 at 12:19 +0100, Gerd Hoffmann wrote:
On Mi, 2014-01-22 at 17:52 -0500, Kevin O'Connor wrote:
Hi Gerd,
Have you had a chance to review the XHCI branch I've been testing? https://github.com/KevinOConnor/seabios/commits/xhci-testing
I'll finally have a look later today.
Looks all sane, and survived a quick test with qemu.
3 - In the original code xhci_hub_disconnect() free'd the xhci slot for a device, but this doesn't look right to me as there can be more than one slot for a given port (in the case of a hub). The testing branch above never free's slots now - do you have any thoughts on how slot allocation/free'ing should be done?
I think the original code is correct. Slots are for devices, not root hub ports. Of course the slot for the usb hub must stay active in case seabios handles devices connected to the hub, but I'm sure this is the case as seabios doesn't disconnect these hubs.
It doesn't hurt to not free slots though ...
And it helps simplifying the data structures (kill xhci_device) and handle 64bit xhci data structures.
4 - In the testing branch above, I added a patch to make hubs work (see "xhci: Hack to get hubs working."). This further overloads xhci_alloc_pipe() though. Do you think that is okay or do you think there is a better way to catch hub registration so that the xhci controller knows about it?
Need to look at the patch.
Looks good to me. Maybe factor out the device init (hub hook, set address, ...) into a separate function to streamline the code a bit.
cheers, Gerd
On Thu, Jan 23, 2014 at 02:07:36PM +0100, Gerd Hoffmann wrote:
On Do, 2014-01-23 at 12:19 +0100, Gerd Hoffmann wrote:
On Mi, 2014-01-22 at 17:52 -0500, Kevin O'Connor wrote:
Hi Gerd,
Have you had a chance to review the XHCI branch I've been testing? https://github.com/KevinOConnor/seabios/commits/xhci-testing
I'll finally have a look later today.
Looks all sane, and survived a quick test with qemu.
3 - In the original code xhci_hub_disconnect() free'd the xhci slot for a device, but this doesn't look right to me as there can be more than one slot for a given port (in the case of a hub). The testing branch above never free's slots now - do you have any thoughts on how slot allocation/free'ing should be done?
I think the original code is correct. Slots are for devices, not root hub ports. Of course the slot for the usb hub must stay active in case seabios handles devices connected to the hub, but I'm sure this is the case as seabios doesn't disconnect these hubs.
It doesn't hurt to not free slots though ...
And it helps simplifying the data structures (kill xhci_device) and handle 64bit xhci data structures.
4 - In the testing branch above, I added a patch to make hubs work (see "xhci: Hack to get hubs working."). This further overloads xhci_alloc_pipe() though. Do you think that is okay or do you think there is a better way to catch hub registration so that the xhci controller knows about it?
Need to look at the patch.
Looks good to me. Maybe factor out the device init (hub hook, set address, ...) into a separate function to streamline the code a bit.
Thanks for reviewing.
I got the Haswell based Acer c720 to enable the xhci controller. All my low speed and high speed devices seem to work okay (including via a hub). However, I'm seeing a failure on a superspeed flash drive when in the superspeed port:
XHCI port #10: 0x000a1203, powered, enabled, pls 0, speed 4 [Super] xhci_update_pipe: reconf ctl endpoint pkt size: 8 -> 9 WARNING - Timeout at xhci_event_wait:394! xhci_control: control xfer failed (cc -1)
I'm going to see if I can track that down, and I'll look to respin the patch series after that.
-Kevin
-
Gerd Hoffmann -
Kevin O'Connor