New patch to review: b551855 Do not use C bit-fields to manipulate EHCI host bridge portsc_t register.

[Steven A. Falco]

Steven A. Falco ([e-mail]) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/101

-gerrit

commit b551855e895382b7570881bebcf28809a5619d63
Author: Steven A. Falco 
Date:   Thu Jul 14 19:56:50 2011 -0400

Do not use C bit-fields to manipulate EHCI host bridge portsc_t register.

I have observed two separate EHCI host bridges that do not tolerate
    using C bit-fields to manipulate the portsc_t register.  The reason
    for this is that the EHCI spec says that PORT_PE must go to 0 at the
    time that PORT_RESET goes to 1.  Naturally this cannot be done using
    separate bit-fields for each bit.  Instead, we use a temporary register
    and do explicit read-modify-write cycles.

Signed-off-by: Steven A. Falco 
    Change-Id: If138faee43e0293efa203b86f7893fdf1e811269
---
 payloads/libpayload/drivers/usb/ehci_private.h |   29 +++++++++++++-
 payloads/libpayload/drivers/usb/ehci_rh.c      |   49 +++++++++++++++++------
 2 files changed, 64 insertions(+), 14 deletions(-)

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;