Just debugged the following:
ehci_setup() happens. Starts a thread to probe its ports... ohci_setup() happens. Starts a thread to probe its ports... OHCI probes port zero, decides there's nothing there. EHCI probes port zero, decides there's a USB1 device there and gives it away to OHCI... which is never going to see it.
What's wrong here? I've "fixed" it with a wait_threads() right before the ohci_setup() in usb.c and now I can see my keyboard, but that's definitely not the right answer.
Do we have any kind of hotplug support for USB devices? If not, should we at least have a kind of "manual" hotplug notification for when EHCI actively gives a port away to the USB1 controller?
(How) is this supposed to work?
On Do, 2013-12-05 at 14:03 +0000, David Woodhouse wrote:
Just debugged the following:
ehci_setup() happens. Starts a thread to probe its ports... ohci_setup() happens. Starts a thread to probe its ports... OHCI probes port zero, decides there's nothing there. EHCI probes port zero, decides there's a USB1 device there and gives it away to OHCI... which is never going to see it.
What's wrong here? I've "fixed" it with a wait_threads() right before the ohci_setup() in usb.c and now I can see my keyboard, but that's definitely not the right answer.
I think not running configure_ehci() as thread should work too and is a bit less agressive than wait_threads(). Waits only for the ehci port probe threads finish (see usb_enumerate() func), not all threads.
Do we have any kind of hotplug support for USB devices?
No.
cheers, Gerd
On Thu, 2013-12-05 at 16:11 +0100, Gerd Hoffmann wrote:
I think not running configure_ehci() as thread should work too and is a bit less agressive than wait_threads(). Waits only for the ehci port probe threads finish (see usb_enumerate() func), not all threads.
Yeah. Is EHCI always initialised before OHCI/UHCI though? I'm staring at the code in usb_setup() and it seems to be attempting that, but I'm not 100% sure if that's what it's actually doing.
And what about XHCI? Does that have similar interactions?
On Thu, Dec 05, 2013 at 02:03:23PM +0000, David Woodhouse wrote:
Just debugged the following:
ehci_setup() happens. Starts a thread to probe its ports... ohci_setup() happens. Starts a thread to probe its ports... OHCI probes port zero, decides there's nothing there. EHCI probes port zero, decides there's a USB1 device there and gives it away to OHCI... which is never going to see it.
What is supposed to happen, is that usb_setup() sees that there is an ehci device and only runs ehci_setup() on the whole device. ohci_setup is not supposed to run from usb_setup() for that device at all. Then, if the ehci code finds a legacy device, it runs ohci_setup() from ehci_note_port() after it has configured all the legacy devices into legacy mode.
This certainly used to work, I'll take a look.
-Kevin
On Thu, Dec 05, 2013 at 10:47:21AM -0500, Kevin O'Connor wrote:
On Thu, Dec 05, 2013 at 02:03:23PM +0000, David Woodhouse wrote:
Just debugged the following:
ehci_setup() happens. Starts a thread to probe its ports... ohci_setup() happens. Starts a thread to probe its ports... OHCI probes port zero, decides there's nothing there. EHCI probes port zero, decides there's a USB1 device there and gives it away to OHCI... which is never going to see it.
What is supposed to happen, is that usb_setup() sees that there is an ehci device and only runs ehci_setup() on the whole device. ohci_setup is not supposed to run from usb_setup() for that device at all. Then, if the ehci code finds a legacy device, it runs ohci_setup() from ehci_note_port() after it has configured all the legacy devices into legacy mode.
This certainly used to work, I'll take a look.
I reviewed the code and booted my e350m1 board with an OHCI keyboard and I can't reproduce your issue. Can you post the logs (with debug level 8) of the failure you are seeing?
-Kevin
On Thu, 2013-12-05 at 11:59 -0500, Kevin O'Connor wrote:
I reviewed the code and booted my e350m1 board with an OHCI keyboard and I can't reproduce your issue. Can you post the logs (with debug level 8) of the failure you are seeing?
With some debugging of my own added. Note that ohci_setup() is called from usb_setup(). Not from ehci_note_port(). And that this happens:
|00fe1000| ohci_hub_detect port 0: sts 0
Before this does:
|00fe3000| port 0 has a full speed device |00fe3000| ehci_hub_reset returns -1
I think this is because the EHCI device has a lower PCI function (0:14.3) than its corresponding OHCI device (0:14.4)? Trying to make sense of the loop in usb_setup(), I think it only really works if the EHCI device comes *after* the OHCI device?
[dwmw2@shinybook Quark_EDKII_0_8_0_RC3_1]$ grep -A103 'init usb' ~/git/clantonusb5.cap | sed 's/ *$//' init usb ehci_setup() _malloc zone=0x00fefe8f handle=ffffffff size=72 align=10 ret=0x00fe5e60 (detail=0x00fe5eb0) EHCI init on dev 00:14.3 (regs=0x9000d010) _malloc zone=0x00fefe8f handle=ffffffff size=4096 align=1000 ret=0x00fe4000 (detail=0x00fe5e30) /00fe4000\ Start thread |00fe4000| _malloc zone=0x00fefe9b handle=ffffffff size=4096 align=1000 ret=0x00fff000 (detail=0x00fe5e00) |00fe4000| _malloc zone=0x00fefe9b handle=ffffffff size=68 align=80 ret=0x00ffef80 (detail=0x00fe5dd0) |00fe4000| _malloc zone=0x00fefe9b handle=ffffffff size=68 align=80 ret=0x00ffef00 (detail=0x00fe5da0) |00fe4000| _malloc zone=0x00fefe8f handle=ffffffff size=28 align=10 ret=0x00fe5d50 (detail=0x00fe5d70) |00fe4000| _malloc zone=0x00fefe8f handle=ffffffff size=4096 align=1000 ret=0x00fe3000 (detail=0x00fe5d20) /00fe3000\ Start thread ohci_setup() called from usb_setup ohci_setup() _malloc zone=0x00fefe8f handle=ffffffff size=24 align=10 ret=0x00fe5cd0 (detail=0x00fe5cf0) OHCI init on dev 00:14.4 (regs=0x9000c000) _malloc zone=0x00fefe8f handle=ffffffff size=4096 align=1000 ret=0x00fe2000 (detail=0x00fe5ca0) /00fe2000\ Start thread |00fe2000| _malloc zone=0x00fefe9b handle=ffffffff size=256 align=100 ret=0x00ffee00 (detail=0x00fe5c70) |00fe2000| _malloc zone=0x00fefe9b handle=ffffffff size=16 align=10 ret=0x00ffeff0 (detail=0x00fe5c40) |00fe4000| _malloc zone=0x00fefe8f handle=ffffffff size=28 align=10 ret=0x00fe5bf0 (detail=0x00fe5c10) |00fe4000| _malloc zone=0x00fefe8f handle=ffffffff size=4096 align=1000 ret=0x00fe1000 (detail=0x00fe5bc0) /00fe1000\ Start thread init ps2port _malloc zone=0x00fefe8f handle=ffffffff size=4096 align=1000 ret=0x00fe0000 (detail=0x00fe5b90) /00fe0000\ Start thread |00fe0000| i8042_flush |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| i8042 flushed ff (status=ff) |00fe0000| WARNING - Timeout at i8042_flush:71! |00fe2000| _free 0x00fe0000 (detail=0x00fe5b90) \00fe0000/ End thread |00fe1000| ehci_hub_detect 1 not found one |00fe1000| _free 0x00fe5bf0 (detail=0x00fe5c10) |00fe3000| _free 0x00fe1000 (detail=0x00fe5bc0) \00fe1000/ End thread |00fe3000| ehci_hub_detect 0 found one init lpt Found 0 lpt ports init serial Found 0 serial ports init floppy drives init hard drives init ahci Found floppy of size 1474560 _malloc zone=0x00fefe97 handle=ffffffff size=36 align=10 ret=0x000f5720 (detail=0x00fe5c10) Mapping SPI floppy at addr 0xff800000 _malloc zone=0x00fefe8f handle=ffffffff size=24 align=10 ret=0x00fe5bc0 (detail=0x00fe5be0) Registering bootable: Ramdisk [SPI] (type:1 prio:0 data:f5720) init megasas handle_08 handle_hwpic1 irq=1 |00fe2000| _malloc zone=0x00fefe8f handle=ffffffff size=28 align=10 ret=0x00fe5b70 (detail=0x00fe5b90) |00fe2000| _malloc zone=0x00fefe8f handle=ffffffff size=4096 align=1000 ret=0x00fe1000 (detail=0x00fe5b40) /00fe1000\ Start thread |00fe1000| ohci_hub_detect port 0: sts 0 |00fe1000| _free 0x00fe5b70 (detail=0x00fe5b90) |00fe4000| _free 0x00fe1000 (detail=0x00fe5b40) \00fe1000/ End thread |00fe3000| port 0 has a full speed device |00fe3000| ehci_hub_reset returns -1 |00fe3000| port 0 speed -1 |00fe3000| _free 0x00fe5d50 (detail=0x00fe5d70) _free 0x00fe3000 (detail=0x00fe5d20) \00fe3000/ End thread handle_08 |00fe2000| _malloc zone=0x00fefe8f handle=ffffffff size=28 align=10 ret=0x00fe5d50 (detail=0x00fe5d70) |00fe2000| _malloc zone=0x00fefe8f handle=ffffffff size=4096 align=1000 ret=0x00fe3000 (detail=0x00fe5d20) /00fe3000\ Start thread |00fe3000| ohci_hub_detect port 1: sts 0 |00fe3000| _free 0x00fe5d50 (detail=0x00fe5d70) |00fe4000| _free 0x00fe3000 (detail=0x00fe5d20) \00fe3000/ End thread |00fe4000| check_ehci_ports found 0 |00fe4000| ehci_free_pipes 0x00fe5e60 |00fe2000| check_ohci_ports() found 0 devices |00fe2000| ohci_free_pipes 0x00fe5cd0 handle_08 |00fe2000| _free 0x00ffee00 (detail=0x00fe5c70) |00fe2000| _free 0x00ffeff0 (detail=0x00fe5c40) |00fe4000| _free 0x00fe2000 (detail=0x00fe5ca0) \00fe2000/ End thread |00fe4000| _free 0x00fff000 (detail=0x00fe5e00) |00fe4000| _free 0x00ffef80 (detail=0x00fe5dd0) |00fe4000| _free 0x00ffef00 (detail=0x00fe5da0) |00fe4000| _free 0x00fe5e60 (detail=0x00fe5eb0) _free 0x00fe4000 (detail=0x00fe5e30) \00fe4000/ End thread All threads complete. enter handle_16:
On Thu, 2013-12-05 at 21:26 +0000, David Woodhouse wrote:
I think this is because the EHCI device has a lower PCI function (0:14.3) than its corresponding OHCI device (0:14.4)?
It is indeed. And reading the EHCI spec, I see that the companion controllers *must* have a lower function number than the EHCI. This appears to be a hardware bug. Will chase internally, but may need a quirk for this.
On Thu, Dec 05, 2013 at 09:44:25PM +0000, David Woodhouse wrote:
On Thu, 2013-12-05 at 21:26 +0000, David Woodhouse wrote:
I think this is because the EHCI device has a lower PCI function (0:14.3) than its corresponding OHCI device (0:14.4)?
It is indeed. And reading the EHCI spec, I see that the companion controllers *must* have a lower function number than the EHCI. This appears to be a hardware bug. Will chase internally, but may need a quirk for this.
Right.
It occurred to me that we could actually simplify the code by initializing all the ehci controllers and then init all the ohci/uhci controllers. The current complex interactions between the PCI walking and EHCI setup is really hard to understand. I put together a sample patch - see attached and at:
https://github.com/KevinOConnor/seabios/tree/testing
I've only lightly tested it. I think something like this would have to wait until after v1.7.4.
-Kevin
On Thu, 2013-12-05 at 18:59 -0500, Kevin O'Connor wrote:
It occurred to me that we could actually simplify the code by initializing all the ehci controllers and then init all the ohci/uhci controllers. The current complex interactions between the PCI walking and EHCI setup is really hard to understand. I put together a sample patch - see attached and at:
That works here, and I think I prefer it to what I'd come up with:
diff --git a/src/hw/usb-ehci.c b/src/hw/usb-ehci.c index b495d6c..e800322 100644 --- a/src/hw/usb-ehci.c +++ b/src/hw/usb-ehci.c @@ -371,6 +371,9 @@ ehci_setup(struct pci_device *pci, int busid, struct pci_device *comppci) cntl->companion[count++] = comppci; else if (pci_classprog(comppci) == PCI_CLASS_SERIAL_USB_OHCI) cntl->companion[count++] = comppci; + // Intel Quark quirk. Precisely one OHCI comes *after* the EHCI. + if (comppci->bdf > pci->bdf) + break; comppci = container_of(comppci->node.next, struct pci_device, node); }
diff --git a/src/hw/usb.c b/src/hw/usb.c index 8fe741f..9c09e42 100644 --- a/src/hw/usb.c +++ b/src/hw/usb.c @@ -461,12 +461,28 @@ usb_setup(void) for (;;) { if (pci_classprog(ehcipci) == PCI_CLASS_SERIAL_USB_EHCI) { // Found an ehci controller. + + // Intel Quark got EHCI and OHCI the wrong way round :( + if (pci->vendor == PCI_VENDOR_ID_INTEL && + pci->device == 0x0939) { + struct pci_device *nextpci; + nextpci = container_of_or_null(ehcipci->node.next, + struct pci_device, node); + if (nextpci && nextpci->vendor == PCI_VENDOR_ID_INTEL && + nextpci->device == 0x093a && + pci_bdf_to_busdev(nextpci->bdf) == + pci_bdf_to_busdev(pci->bdf)) { + pci = nextpci; + found++; + } + } int ret = ehci_setup(ehcipci, count++, pci); if (ret) // Error break; count += found; - pci = ehcipci; + if (pci->bdf < ehcipci->bdf) + pci = ehcipci; break; } if (ehcipci->class == PCI_CLASS_SERIAL_USB) @@ -479,6 +495,9 @@ usb_setup(void) break; } } + if (pci->bdf > ehcipci->bdf) + // Quark OHCI. It's been handled. + continue;
if (pci_classprog(pci) == PCI_CLASS_SERIAL_USB_UHCI) uhci_setup(pci, count++);
On Do, 2013-12-05 at 15:20 +0000, David Woodhouse wrote:
On Thu, 2013-12-05 at 16:11 +0100, Gerd Hoffmann wrote:
I think not running configure_ehci() as thread should work too and is a bit less agressive than wait_threads(). Waits only for the ehci port probe threads finish (see usb_enumerate() func), not all threads.
Yeah. Is EHCI always initialised before OHCI/UHCI though? I'm staring at the code in usb_setup() and it seems to be attempting that, but I'm not 100% sure if that's what it's actually doing.
Yes, order is ehci first.
And what about XHCI? Does that have similar interactions?
No, xhci handles all usb speeds itself.
cheers, Gerd