Re: [SeaBIOS] USB1 devices and CONFIG_THREADS

2013-12-06 Thread Gerd Hoffmann 
On Do, 2013-12-05 at 15:20 +, 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



___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] USB1 devices and CONFIG_THREADS

2013-12-05 Thread David Woodhouse 
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:
> 
> https://github.com/KevinOConnor/seabios/tree/testing

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++);


-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] USB1 devices and CONFIG_THREADS

2013-12-05 Thread Kevin O'Connor 
On Thu, Dec 05, 2013 at 09:44:25PM +, David Woodhouse wrote:
> On Thu, 2013-12-05 at 21:26 +, 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
>From 3aa68c32da6236728d3f20040ce7c9d33b89c991 Mon Sep 17 00:00:00 2001
Message-Id: 
<3aa68c32da6236728d3f20040ce7c9d33b89c991.1386287376.git.ke...@koconnor.net>
From: Kevin O'Connor 
Date: Thu, 5 Dec 2013 18:43:20 -0500
Subject: [PATCH] usb: Replace EHCI to UHCI/OHCI synchronization with new
 scheme.
To: seabios@seabios.org

The previous code attempts to correlate which UHCI and OHCI
controllers correlate with which EHCI controllers so that it can
ensure high speed devices are handled by the EHCI code while low/full
speed devices are handled by the UHCI/OHCI code.  Replace this logic
by initializing all EHCI controllers first, and then initializing all
UHCI and OHCI controllers.  This simplifies the code and improves
support for some hardware devices that don't follow the OHCI/UHCI to
EHCI correlation standard.

Also, remove the unused usb->busid field.

Signed-off-by: Kevin O'Connor 
---
 src/hw/usb-ehci.c | 83 ++-
 src/hw/usb-ehci.h |  2 +-
 src/hw/usb-ohci.c | 21 ++
 src/hw/usb-ohci.h |  2 +-
 src/hw/usb-uhci.c | 21 ++
 src/hw/usb-uhci.h |  2 +-
 src/hw/usb-xhci.c | 26 +++--
 src/hw/usb-xhci.h |  2 +-
 src/hw/usb.c  | 59 ---
 src/hw/usb.h  |  1 -
 10 files changed, 92 insertions(+), 127 deletions(-)

diff --git a/src/hw/usb-ehci.c b/src/hw/usb-ehci.c
index b495d6c..10c92fe 100644
--- a/src/hw/usb-ehci.c
+++ b/src/hw/usb-ehci.c
@@ -1,6 +1,6 @@
 // Code for handling EHCI USB controllers.
 //
-// Copyright (C) 2010  Kevin O'Connor 
+// Copyright (C) 2010-2013  Kevin O'Connor 
 //
 // This file may be distributed under the terms of the GNU LGPLv3 license.
 
@@ -14,8 +14,6 @@
 #include "string.h" // memset
 #include "usb.h" // struct usb_s
 #include "usb-ehci.h" // struct ehci_qh
-#include "usb-ohci.h" // ohci_setup
-#include "usb-uhci.h" // uhci_setup
 #include "util.h" // msleep
 #include "x86.h" // readl
 
@@ -24,9 +22,7 @@ struct usb_ehci_s {
 struct ehci_caps *caps;
 struct ehci_regs *regs;
 struct ehci_qh *async_qh;
-struct pci_device *companion[8];
 int checkports;
-int legacycount;
 };
 
 struct ehci_pipe {
@@ -36,6 +32,8 @@ struct ehci_pipe {
 struct usb_pipe pipe;
 };
 
+static int PendingEHCIPorts;
+
 
 /
  * Root hub
@@ -44,33 +42,6 @@ struct ehci_pipe {
 #define EHCI_TIME_POSTPOWER 20
 #define EHCI_TIME_POSTRESET 2
 
-// Check if need companion controllers for full/low speed devices
-static void
-ehci_note_port(struct usb_ehci_s *cntl)
-{
-if (--cntl->checkports)
-// Ports still being detected.
-return;
-if (! cntl->legacycount)
-// No full/low speed devices found.
-return;
-// Start companion controllers.
-int i;
-for (i=0; icompanion); i++) {
-struct pci_device *pci = cntl->companion[i];
-if (!pci)
-break;
-
-// ohci/uhci_setup call pci_config_X - don't run from irq handler.
-wait_preempt();
-
-if (pci_classprog(pci) == PCI_CLASS_SERIAL_USB_UHCI)
-uhci_setup(pci, cntl->usb.busid + i);
-else if (pci_classprog(pci) == PCI_CLASS_SERIAL_USB_OHCI)
-ohci_setup(pci, cntl->usb.busid + i);
-}
-}
-
 // Check if device attached to port
 static int
 ehci_hub_detect(struct usbhub_s *hub, u32 port)
@@ -97,7 +68,6 @@ ehci_hub_detect(struct usbhub_s *hub, u32 port)
 
 if ((portsc & PORT_LINESTATUS_MASK) == PORT_LINESTATUS_KSTATE) {
 // low speed device
-cntl->legacycount++;
 writel(portreg, portsc | PORT_OWNER);
 goto doneearly;
 }
@@ -111,7 +81,7 @@ ehci_hub_detect(struct usbhub_s *hub, u32 port)
 return 0;
 
 doneearly:
-ehci_note_port(cntl);
+PendingEHCIPorts--;
 return -1;
 }
 
@@ -135,14 +105,13 @@ ehci_hub_reset(struct usbhub_s *hub, u32 port)
 goto resetfail;
 if (!(portsc & PORT_PE)) {
 // full speed device
-cntl->legacyco

Re: [SeaBIOS] USB1 devices and CONFIG_THREADS

2013-12-05 Thread David Woodhouse 
On Thu, 2013-12-05 at 21:26 +, 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.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] USB1 devices and CONFIG_THREADS

2013-12-05 Thread David Woodhouse 
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= size=72 align=10 ret=0x00fe5e60 
(detail=0x00fe5eb0)
EHCI init on dev 00:14.3 (regs=0x9000d010)
_malloc zone=0x00fefe8f handle= size=4096 align=1000 ret=0x00fe4000 
(detail=0x00fe5e30)
/00fe4000\ Start thread
|00fe4000| _malloc zone=0x00fefe9b handle= size=4096 align=1000 
ret=0x00fff000 (detail=0x00fe5e00)
|00fe4000| _malloc zone=0x00fefe9b handle= size=68 align=80 
ret=0x00ffef80 (detail=0x00fe5dd0)
|00fe4000| _malloc zone=0x00fefe9b handle= size=68 align=80 
ret=0x00ffef00 (detail=0x00fe5da0)
|00fe4000| _malloc zone=0x00fefe8f handle= size=28 align=10 
ret=0x00fe5d50 (detail=0x00fe5d70)
|00fe4000| _malloc zone=0x00fefe8f handle= size=4096 align=1000 
ret=0x00fe3000 (detail=0x00fe5d20)
/00fe3000\ Start thread
ohci_setup() called from usb_setup
ohci_setup()
_malloc zone=0x00fefe8f handle= size=24 align=10 ret=0x00fe5cd0 
(detail=0x00fe5cf0)
OHCI init on dev 00:14.4 (regs=0x9000c000)
_malloc zone=0x00fefe8f handle= size=4096 align=1000 ret=0x00fe2000 
(detail=0x00fe5ca0)
/00fe2000\ Start thread
|00fe2000| _malloc zone=0x00fefe9b handle= size=256 align=100 
ret=0x00ffee00 (detail=0x00fe5c70)
|00fe2000| _malloc zone=0x00fefe9b handle= size=16 align=10 
ret=0x00ffeff0 (detail=0x00fe5c40)
|00fe4000| _malloc zone=0x00fefe8f handle= size=28 align=10 
ret=0x00fe5bf0 (detail=0x00fe5c10)
|00fe4000| _malloc zone=0x00fefe8f handle= size=4096 align=1000 
ret=0x00fe1000 (detail=0x00fe5bc0)
/00fe1000\ Start thread
init ps2port
_malloc zone=0x00fefe8f handle= size=4096 align=1000 ret=0x00fe 
(detail=0x00fe5b90)
/00fe\ Start thread
|00fe| i8042_flush
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| i8042 flushed ff (status=ff)
|00fe| WARNING - Timeout at i8042_flush:71!
|00fe2000| _free 0x00fe (detail=0x00fe5b90)
\00fe/ 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= size=36 align=10 ret=0x000f5720 
(detail=0x00fe5c10)
Mapping SPI floppy at addr 0xff80
_malloc zone=0x00fefe8f handle= 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= size=28 align=10 
ret=0x00fe5b70 (detail=0x00fe5b90)
|00fe2000| _malloc zone=0x00fefe8f handle= 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= size=28 align=10 
ret=0x00fe5d50 (detail=0x00fe5d70)
|00fe2000| _malloc zone=0x00fefe8f handle= 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

Re: [SeaBIOS] USB1 devices and CONFIG_THREADS

2013-12-05 Thread Kevin O'Connor 
On Thu, Dec 05, 2013 at 10:47:21AM -0500, Kevin O'Connor wrote:
> On Thu, Dec 05, 2013 at 02:03:23PM +, 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

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] USB1 devices and CONFIG_THREADS

2013-12-05 Thread Kevin O'Connor 
On Thu, Dec 05, 2013 at 02:03:23PM +, 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

___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] USB1 devices and CONFIG_THREADS

2013-12-05 Thread David Woodhouse 
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?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

Re: [SeaBIOS] USB1 devices and CONFIG_THREADS

2013-12-05 Thread Gerd Hoffmann 
On Do, 2013-12-05 at 14:03 +, 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



___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

[SeaBIOS] USB1 devices and CONFIG_THREADS

2013-12-05 Thread David Woodhouse 
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?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios