[PATCH] ohci misc

This patch  includes the innocuous bits from a larger one that
I'm still working on (mostly unlink fixes):

- updates comments
- flags TDs that were seen in the donelist
- removes some bogus whitespace (at EOL etc) and tabs
- checks for an enumeration issue that might cause trouble
- delays IRQs a bit more aggressively
- shortens TD submit paths a smidgeon (smaller ".o")
- updates some of the debug output
- sanitizes usb_make_path() output on the SA-1111

drivers/usb/host/ohci-dbg.c

@@ -12,13 +12,14 @@
  
 #ifdef DEBUG
 
-#define pipestring(pipe) ({ char *temp; \
-	switch (usb_pipetype (pipe)) { \
+#define edstring(ed_type) ({ char *temp; \
+	switch (ed_type) { \
 	case PIPE_CONTROL:	temp = "CTRL"; break; \
 	case PIPE_BULK:		temp = "BULK"; break; \
 	case PIPE_INTERRUPT:	temp = "INTR"; break; \
 	default: 		temp = "ISOC"; break; \
 	}; temp;})
+#define pipestring(pipe) edstring(usb_pipetype(pipe))
 
 /* debug| print the main components of an URB     
  * small: 0) header + data packets 1) just header
@@ -35,9 +36,9 @@ static void urb_print (struct urb * urb, char * str, int small)
 #ifndef	OHCI_VERBOSE_DEBUG
 	if (urb->status != 0)
 #endif
-	dbg("%s:[%4x] dev:%d,ep=%d-%c,%s,flags:%4x,len:%d/%d,stat:%d", 
+	dbg("%s %p dev:%d,ep=%d-%c,%s,flags:%x,len:%d/%d,stat:%d", 
 		    str,
-		    usb_get_current_frame_number (urb->dev), 
+		    urb,
 		    usb_pipedevice (pipe),
 		    usb_pipeendpoint (pipe), 
 		    usb_pipeout (pipe)? 'O': 'I',
@@ -242,21 +243,25 @@ static void ohci_dump (struct ohci_hcd *controller, int verbose)
 	ohci_dump_roothub (controller, 1);
 }
 
+static const char data0 [] = "DATA0";
+static const char data1 [] = "DATA1";
+
 static void ohci_dump_td (char *label, struct td *td)
 {
 	u32	tmp = le32_to_cpup (&td->hwINFO);
 
-	dbg ("%s td %p; urb %p index %d; hw next td %08x",
+	dbg ("%s td %p%s; urb %p index %d; hw next td %08x",
 		label, td,
+		(tmp & TD_DONE) ? " (DONE)" : "",
 		td->urb, td->index,
 		le32_to_cpup (&td->hwNextTD));
 	if ((tmp & TD_ISO) == 0) {
-		char	*toggle, *pid;
+		const char	*toggle, *pid;
 		u32	cbp, be;
 
 		switch (tmp & TD_T) {
-		case TD_T_DATA0: toggle = "DATA0"; break;
-		case TD_T_DATA1: toggle = "DATA1"; break;
+		case TD_T_DATA0: toggle = data0; break;
+		case TD_T_DATA1: toggle = data1; break;
 		case TD_T_TOGGLE: toggle = "(CARRY)"; break;
 		default: toggle = "(?)"; break;
 		}
@@ -297,9 +302,9 @@ ohci_dump_ed (struct ohci_hcd *ohci, char *label, struct ed *ed, int verbose)
 	u32	tmp = ed->hwINFO;
 	char	*type = "";
 
-	dbg ("%s: %s, ed %p state 0x%x type %d; next ed %08x",
+	dbg ("%s: %s, ed %p state 0x%x type %s; next ed %08x",
 		ohci->hcd.self.bus_name, label,
-		ed, ed->state, ed->type,
+		ed, ed->state, edstring (ed->type),
 		le32_to_cpup (&ed->hwNextED));
 	switch (tmp & (ED_IN|ED_OUT)) {
 	case ED_OUT: type = "-OUT"; break;
@@ -314,10 +319,10 @@ ohci_dump_ed (struct ohci_hcd *ohci, char *label, struct ed *ed, int verbose)
 		0x000f & (le32_to_cpu (tmp) >> 7),
 		type,
 		0x007f & le32_to_cpu (tmp));
-	dbg ("  tds: head %08x%s%s tail %08x%s",
+	dbg ("  tds: head %08x %s%s tail %08x%s",
 		tmp = le32_to_cpup (&ed->hwHeadP),
+		(ed->hwHeadP & ED_C) ? data1 : data0,
 		(ed->hwHeadP & ED_H) ? " HALT" : "",
-		(ed->hwHeadP & ED_C) ? " CARRY" : "",
 		le32_to_cpup (&ed->hwTailP),
 		verbose ? "" : " (not listing)");
 	if (verbose) {

drivers/usb/host/ohci-hcd.c

@@ -197,7 +197,7 @@ static int ohci_urb_enqueue (
 
 	/* allocate the TDs (updating hash chains) */
 	spin_lock_irqsave (&ohci->lock, flags);
-	for (i = 0; i < size; i++) { 
+	for (i = 0; i < size; i++) {
 		urb_priv->td [i] = td_alloc (ohci, SLAB_ATOMIC);
 		if (!urb_priv->td [i]) {
 			urb_priv->length = i;
@@ -208,6 +208,8 @@ static int ohci_urb_enqueue (
 	}	
 
 // FIXME:  much of this switch should be generic, move to hcd code ...
+// ... and what's not generic can't really be handled this way.
+// need to consider periodicity for both types!
 
 	/* allocate and claim bandwidth if needed; ISO
 	 * needs start frame index if it was't provided.
@@ -247,14 +249,14 @@ static int ohci_urb_enqueue (
 
 	spin_unlock_irqrestore (&ohci->lock, flags);
 
-	return 0;	
+	return 0;
 }
 
 /*
  * decouple the URB from the HC queues (TDs, urb_priv); it's
- * already marked for deletion.  reporting is always done
+ * already marked using urb->status.  reporting is always done
  * asynchronously, and we might be dealing with an urb that's
- * almost completed anyway...
+ * partially transferred, or an ED with other urbs being unlinked.
  */
 static int ohci_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
 {

drivers/usb/host/ohci-q.c

@@ -465,6 +465,25 @@ static struct ed *ed_get (
 		 * we know it's already a power of 2
 		 */
 		ed->interval = interval;
+#ifdef DEBUG
+	/*
+	 * There are two other cases we ought to change hwINFO, both during
+	 * enumeration.  There, the control request completes, unlinks, and
+	 * the next request gets queued before the unlink completes, so it
+	 * uses old/wrong hwINFO.  How much of a problem is this?  khubd is
+	 * already retrying after such failures...
+	 */
+	} else if (type == PIPE_CONTROL) {
+		u32	info = le32_to_cpup (&ed->hwINFO);
+
+		if (!(info & 0x7f))
+			dbg ("RETRY ctrl: address != 0");
+		info >>= 16;
+		if (info != udev->epmaxpacketin [0])
+			dbg ("RETRY ctrl: maxpacket %d != 8",
+				udev->epmaxpacketin [0]);
+
+#endif /* DEBUG */
 	}
 
 done:
@@ -539,12 +558,15 @@ td_fill (struct ohci_hcd *ohci, unsigned int info,
 
 	/* aim for only one interrupt per urb.  mostly applies to control
 	 * and iso; other urbs rarely need more than one TD per urb.
+	 * this way, only final tds (or ones with an error) cause IRQs.
 	 *
 	 * NOTE: could delay interrupts even for the last TD, and get fewer
 	 * interrupts ... increasing per-urb latency by sharing interrupts.
+	 * Drivers that queue bulk urbs may request that behavior.
 	 */
-	if (index != (urb_priv->length - 1))
-		info |= is_iso ? TD_DI_SET (7) : TD_DI_SET (1);
+	if (index != (urb_priv->length - 1)
+			|| (urb->transfer_flags & URB_NO_INTERRUPT))
+		info |= TD_DI_SET (7);
 
 	/* use this td as the next dummy */
 	td_pt = urb_priv->td [index];
@@ -565,6 +587,7 @@ td_fill (struct ohci_hcd *ohci, unsigned int info,
 	td->hwINFO = cpu_to_le32 (info);
 	if (is_iso) {
 		td->hwCBP = cpu_to_le32 (data & 0xFFFFF000);
+		td->hwPSW [0] = cpu_to_le16 ((data & 0x0FFF) | 0xE000);
 		td->ed->intriso.last_iso = info & 0xffff;
 	} else {
 		td->hwCBP = cpu_to_le32 (data); 
@@ -574,7 +597,6 @@ td_fill (struct ohci_hcd *ohci, unsigned int info,
 	else
 		td->hwBE = 0;
 	td->hwNextTD = cpu_to_le32 (td_pt->td_dma);
-	td->hwPSW [0] = cpu_to_le16 ((data & 0x0FFF) | 0xE000);
 
 	/* HC might read the TD right after we link it ... */
 	wmb ();
@@ -596,17 +618,17 @@ static void td_submit_urb (struct urb *urb)
 	int		cnt = 0; 
 	__u32		info = 0;
   	unsigned int	toggle = 0;
+	int		is_out = usb_pipeout (urb->pipe);
 
 	/* OHCI handles the DATA-toggles itself, we just use the
 	 * USB-toggle bits for resetting
 	 */
-  	if (usb_gettoggle (urb->dev, usb_pipeendpoint (urb->pipe),
-			usb_pipeout (urb->pipe))) {
+  	if (usb_gettoggle (urb->dev, usb_pipeendpoint (urb->pipe), is_out)) {
   		toggle = TD_T_TOGGLE;
 	} else {
   		toggle = TD_T_DATA0;
 		usb_settoggle (urb->dev, usb_pipeendpoint (urb->pipe),
-			usb_pipeout (urb->pipe), 1);
+			is_out, 1);
 	}
 
 	urb_priv->td_cnt = 0;
@@ -614,9 +636,9 @@ static void td_submit_urb (struct urb *urb)
 	if (data_len) {
 		data = pci_map_single (ohci->hcd.pdev,
 				       urb->transfer_buffer, data_len,
-				       usb_pipeout (urb->pipe)
-				       ? PCI_DMA_TODEVICE
-				       : PCI_DMA_FROMDEVICE);
+				       is_out
+					       ? PCI_DMA_TODEVICE
+					       : PCI_DMA_FROMDEVICE);
 	} else
 		data = 0;
 
@@ -625,18 +647,20 @@ static void td_submit_urb (struct urb *urb)
 	 */
 	switch (usb_pipetype (urb->pipe)) {
 		case PIPE_BULK:
-			info = usb_pipeout (urb->pipe)
+			info = is_out
 				? TD_CC | TD_DP_OUT
 				: TD_CC | TD_DP_IN ;
+			/* TDs _could_ transfer up to 8K each */
 			while (data_len > 4096) {		
 				td_fill (ohci,
 					info | (cnt? TD_T_TOGGLE:toggle),
 					data, 4096, urb, cnt);
 				data += 4096; data_len -= 4096; cnt++;
 			}
-			info = usb_pipeout (urb->pipe)?
-				TD_CC | TD_DP_OUT : TD_CC | TD_R | TD_DP_IN ;
-			td_fill (ohci, info | (cnt? TD_T_TOGGLE:toggle),
+			/* maybe avoid ED halt on final TD short read */
+			if (!(urb->transfer_flags & USB_DISABLE_SPD))
+				info |= TD_R;
+			td_fill (ohci, info | (cnt ? TD_T_TOGGLE : toggle),
 				data, data_len, urb, cnt);
 			cnt++;
 			if ((urb->transfer_flags & USB_ZERO_PACKET)
@@ -653,8 +677,11 @@ static void td_submit_urb (struct urb *urb)
 			break;
 
 		case PIPE_INTERRUPT:
+			/* current policy:  only one TD per request.
+			 * otherwise identical to bulk, except for BLF
+			 */
 			info = TD_CC | toggle;
-			info |= usb_pipeout (urb->pipe) 
+			info |= is_out
 				?  TD_DP_OUT
 				:  TD_R | TD_DP_IN;
 			td_fill (ohci, info, data, data_len, urb, cnt++);
@@ -670,14 +697,12 @@ static void td_submit_urb (struct urb *urb)
 				 8, urb, cnt++); 
 			if (data_len > 0) {  
 				info = TD_CC | TD_R | TD_T_DATA1;
-				info |= usb_pipeout (urb->pipe)
-				    ? TD_DP_OUT
-				    : TD_DP_IN;
+				info |= is_out ? TD_DP_OUT : TD_DP_IN;
 				/* NOTE:  mishandles transfers >8K, some >4K */
 				td_fill (ohci, info, data, data_len,
 						urb, cnt++);  
 			} 
-			info = usb_pipeout (urb->pipe)
+			info = is_out
 				? TD_CC | TD_DP_IN | TD_T_DATA1
 				: TD_CC | TD_DP_OUT | TD_T_DATA1;
 			td_fill (ohci, info, data, 0, urb, cnt++);
@@ -806,10 +831,13 @@ static struct td *dl_reverse_done_list (struct ohci_hcd *ohci)
 	while (td_list_hc) {		
 		td_list = dma_to_td (ohci, td_list_hc);
 
+		td_list->hwINFO |= cpu_to_le32 (TD_DONE);
+
 		if (TD_CC_GET (le32_to_cpup (&td_list->hwINFO))) {
 			urb_priv = (urb_priv_t *) td_list->urb->hcpriv;
-			/* typically the endpoint halts on error; un-halt,
-			 * and maybe dequeue other TDs from this urb
+			/* Non-iso endpoints can halt on error; un-halt,
+			 * and dequeue any other TDs from this urb.
+			 * No other TD could have caused the halt.
 			 */
 			if (td_list->ed->hwHeadP & ED_H) {
 				if (urb_priv && ((td_list->index + 1)

drivers/usb/host/ohci-sa1111.c

@@ -175,7 +175,7 @@ int usb_hcd_sa1111_probe (const struct hc_driver *driver, struct usb_hcd **hcd_o
 	usb_bus_init (&hcd->self);
 	hcd->self.op = &usb_hcd_operations;
 	hcd->self.hcpriv = (void *) hcd;
-	hcd->self.bus_name = "SA-1111";
+	hcd->self.bus_name = "sa1111";
 	hcd->product_desc = "SA-1111 OHCI";
 
 	INIT_LIST_HEAD (&hcd->dev_list);

drivers/usb/host/ohci.h

@@ -11,6 +11,9 @@
 /*
  * OHCI Endpoint Descriptor (ED) ... holds TD queue
  * See OHCI spec, section 4.2
+ *
+ * This is a "Queue Head" for those transfers, which is why
+ * both EHCI and UHCI call similar structures a "QH".
  */
 struct ed {
 	/* first fields are hardware-specified, le32 */
@@ -74,8 +77,8 @@ struct td {
 	/* these two bits are available for definition/use by HCDs in both
 	 * general and iso tds ... others are available for only one type
 	 */
-//#define TD____	    0x00020000
-#define TD_ISO	    0x00010000			/* copy of ED_ISO */
+#define TD_DONE     0x00020000			/* retired to donelist */
+#define TD_ISO      0x00010000			/* copy of ED_ISO */
 
 	/* hwINFO bits for general tds: */
 #define TD_EC       0x0C000000			/* error count */
@@ -349,12 +352,14 @@ struct ohci_hcd {
         struct device		*parent_dev;
 
 	/*
-	 * I/O memory used to communicate with the HC (uncached);
+	 * I/O memory used to communicate with the HC (dma-consistent)
 	 */
 	struct ohci_regs	*regs;
 
 	/*
-	 * main memory used to communicate with the HC (uncached)
+	 * main memory used to communicate with the HC (dma-consistent).
+	 * hcd adds to schedule for a live hc any time, but removals finish
+	 * only at the start of the next frame.
 	 */
 	struct ohci_hcca	*hcca;
 	dma_addr_t		hcca_dma;
@@ -365,6 +370,9 @@ struct ohci_hcd {
 	struct ed		*ed_controltail;	/* last in ctrl list */
  	struct ed		*ed_isotail;		/* last in iso list */
 
+	/*
+	 * memory management for queue data structures
+	 */
 	struct pci_pool		*td_cache;
 	struct pci_pool		*ed_cache;
 	struct hash_list_t	td_hash [TD_HASH_SIZE];
@@ -380,6 +388,7 @@ struct ohci_hcd {
 
 	unsigned long		flags;		/* for HC bugs */
 #define	OHCI_QUIRK_AMD756	0x01			/* erratum #4 */
+	// there are also chip quirks/bugs in init logic
 
 	/*
 	 * framework state