Some EHCI controllers misbehave if bits in the portsc register do not change atomically. Thus we use a temporary variable to do a single read-modify-write when changing related bits in the register.
The symptom is an incorrect handoff from EHCI to OHCI.
Signed-off-by: Steven A. Falco sfalco@coincident.com
---
I've observed the problem with several EHCI controllers: an Intel Topcliff IOH and a Pericom PI7C9X440SL. With the changes below, both these controllers work properly.
Note that several other registers in ehci_private.h are described in terms of separate bits too. I have not changed them because I have not observed any problems with them.
diff --git a/payloads/libpayload/drivers/usb/ehci_private.h b/payloads/libpayload/drivers/usb/ehci_private.h index e63a81d..07c1a2f 100644 --- a/payloads/libpayload/drivers/usb/ehci_private.h +++ b/payloads/libpayload/drivers/usb/ehci_private.h @@ -37,8 +37,34 @@ #define FLADJ 0x61 #define FLADJ_framelength(x) (((x)-59488)/16)
+#define PORT_WKOC_E (1<<22) +#define PORT_WKDISC_E (1<<21) +#define PORT_WKCONN_E (1<<20) +#define PORT_TEST_PKT (0x4<<16) +#define PORT_LED_OFF (0<<14) +#define PORT_LED_AMBER (1<<14) +#define PORT_LED_GREEN (2<<14) +#define PORT_LED_MASK (3<<14) +#define PORT_OWNER (1<<13) +#define PORT_POWER (1<<12) +#define PORT_LINESTATUS_MASK (3<<10) +#define PORT_LINESTATUS_KSTATE (1<<10) +#define PORT_RESET (1<<8) +#define PORT_SUSPEND (1<<7) +#define PORT_RESUME (1<<6) +#define PORT_OCC (1<<5) +#define PORT_OC (1<<4) +#define PORT_PEC (1<<3) +#define PORT_PE (1<<2) +#define PORT_CSC (1<<1) +#define PORT_CONNECT (1<<0) + typedef union { - u32 val; + volatile u32 val; + // WARNING - some controllers require port_enable and port_reset to + // change atomically. Therefore, using these separate bits is not + // recommended. +#if 0 volatile struct { unsigned long current_conn_status:1; unsigned long conn_status_change:1; @@ -60,6 +86,7 @@ typedef union { unsigned long wake_on_overcurrent_en:1; unsigned long:9; } __attribute__ ((packed)); +#endif } __attribute__ ((packed)) portsc_t;
typedef struct { diff --git a/payloads/libpayload/drivers/usb/ehci_rh.c b/payloads/libpayload/drivers/usb/ehci_rh.c index 5eaeeb6..c588a3d 100644 --- a/payloads/libpayload/drivers/usb/ehci_rh.c +++ b/payloads/libpayload/drivers/usb/ehci_rh.c @@ -54,14 +54,19 @@ static void ehci_rh_hand_over_port (usbdev_t *dev, int port) { volatile portsc_t *p = &(RH_INST(dev)->ports[port]); + volatile u32 tmp;
printf("giving up port %x, it's USB1\n", port+1);
/* Lowspeed device. Hand over to companion */ - p->port_owner = 1; - do {} while (!p->conn_status_change); + tmp = p->val; + tmp |= PORT_OWNER; + p->val = tmp; + while (!(p->val & PORT_CSC)); /* RW/C register, so clear it by writing 1 */ - p->conn_status_change = 1; + tmp = p->val; + tmp |= PORT_CSC; + p->val = tmp; return; }
@@ -69,41 +74,54 @@ static void ehci_rh_scanport (usbdev_t *dev, int port) { volatile portsc_t *p = &(RH_INST(dev)->ports[port]); + volatile u32 tmp; if (RH_INST(dev)->devices[port]!=-1) { printf("Unregister device at port %x\n", port+1); usb_detach_device(dev->controller, RH_INST(dev)->devices[port]); RH_INST(dev)->devices[port]=-1; } /* device connected, handle */ - if (p->current_conn_status) { + if (p->val & PORT_CONNECT) { mdelay(100); - if (p->line_status == 0x1) { + if ((p->val & PORT_LINESTATUS_MASK) == PORT_LINESTATUS_KSTATE) { ehci_rh_hand_over_port(dev, port); return; } - p->port_enable = 0; - p->port_reset = 1; + + /* Remove port_enable, set port_reset. This must be atomic. */ + tmp = p->val; + tmp &= ~PORT_PE; + tmp |= PORT_RESET; + p->val = tmp; + mdelay(50); - p->port_reset = 0; + tmp &= ~PORT_RESET; + p->val = tmp; + /* Wait for flag change to finish. The controller might take a while */ - while (p->port_reset) ; - if (!p->port_enable) { + while(p->val & PORT_RESET) ; + if (!(p->val & PORT_PE)) { ehci_rh_hand_over_port(dev, port); return; } printf("port %x hosts a USB2 device\n", port+1); RH_INST(dev)->devices[port] = usb_attach_device(dev->controller, dev->address, port, 2); } + /* RW/C register, so clear it by writing 1 */ - p->conn_status_change = 1; + tmp = p->val; + tmp |= PORT_CSC; + p->val = tmp; }
static int ehci_rh_report_port_changes (usbdev_t *dev) { + volatile portsc_t *p; int i; for (i=0; i<RH_INST(dev)->n_ports; i++) { - if (RH_INST(dev)->ports[i].conn_status_change) + p = &(RH_INST(dev)->ports[i]); + if (p->val & PORT_CSC) return i; } return -1; @@ -121,6 +139,8 @@ ehci_rh_poll (usbdev_t *dev) void ehci_rh_init (usbdev_t *dev) { + volatile portsc_t *p; + volatile u32 tmp; int i;
dev->destroy = ehci_rh_destroy; @@ -135,8 +155,11 @@ ehci_rh_init (usbdev_t *dev)
RH_INST(dev)->devices = malloc(RH_INST(dev)->n_ports * sizeof(int)); for (i=0; i < RH_INST(dev)->n_ports; i++) { + p = &(RH_INST(dev)->ports[i]); + tmp = p->val; + tmp |= PORT_POWER; + p->val = tmp; RH_INST(dev)->devices[i] = -1; - RH_INST(dev)->ports[i].pp = 1; }
dev->address = 0;
-
Steven A. Falco