[PATCH] ohci misc fixes

This patch applies on top of the other two (for init problems):

- Uses time to balance interrupt load, not number of transfers.
   One 8-byte lowspeed transfer costs as much as ten same-size
   at full speed ... previous code could overcommit branches.
- Shrinks the code a smidgeon, mostly in the submit path.
- Updates comments, remove some magic numbers, etc.
- Adds some debug dump routines for EDs and TDs, which can
   be rather helpful when debugging!
- Lays ground work for a "shadow" <linux/list.h> TD queue
   (but doesn't enlarge the TD or ED on 32bit cpus)

I'm not sure anyone would have run into that time/balance
issue, though some folk have talked about hooking up lots
of lowspeed devices and that would have made trouble.

drivers/usb/host/ohci-dbg.c

@@ -74,9 +74,9 @@ static void urb_print (struct urb * urb, char * str, int small)
 static inline struct ed *
 dma_to_ed (struct ohci_hcd *hc, dma_addr_t ed_dma);
 
-#ifdef OHCI_VERBOSE_DEBUG
 /* print non-empty branches of the periodic ed tree */
-void ohci_dump_periodic (struct ohci_hcd *ohci, char *label)
+static void __attribute__ ((unused))
+ohci_dump_periodic (struct ohci_hcd *ohci, char *label)
 {
 	int i, j;
 	u32 *ed_p;
@@ -101,7 +101,6 @@ void ohci_dump_periodic (struct ohci_hcd *ohci, char *label)
 		printk (KERN_DEBUG "%s, ohci %s, empty periodic schedule\n",
 				label, ohci->hcd.self.bus_name);
 }
-#endif
 
 static void ohci_dump_intr_mask (char *label, __u32 mask)
 {
@@ -241,6 +240,97 @@ static void ohci_dump (struct ohci_hcd *controller, int verbose)
 	ohci_dump_roothub (controller, 1);
 }
 
+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",
+		label, td,
+		td->urb, td->index,
+		le32_to_cpup (&td->hwNextTD));
+	if ((tmp & TD_ISO) == 0) {
+		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_TOGGLE: toggle = "(CARRY)"; break;
+		default: toggle = "(?)"; break;
+		}
+		switch (tmp & TD_DP) {
+		case TD_DP_SETUP: pid = "SETUP"; break;
+		case TD_DP_IN: pid = "IN"; break;
+		case TD_DP_OUT: pid = "OUT"; break;
+		default: pid = "(bad pid)"; break;
+		}
+		dbg ("     info %08x CC=%x %s DI=%d %s %s", tmp,
+			TD_CC_GET(tmp), /* EC, */ toggle,
+			(tmp & TD_DI) >> 21, pid,
+			(tmp & TD_R) ? "R" : "");
+		cbp = le32_to_cpup (&td->hwCBP);
+		be = le32_to_cpup (&td->hwBE);
+		dbg ("     cbp %08x be %08x (len %d)", cbp, be,
+			cbp ? (be + 1 - cbp) : 0);
+	} else {
+		unsigned	i;
+		dbg ("     info %08x CC=%x DI=%d START=%04x", tmp,
+			TD_CC_GET(tmp), /* FC, */
+			(tmp & TD_DI) >> 21,
+			tmp & 0x0000ffff);
+		dbg ("     bp0 %08x be %08x",
+			le32_to_cpup (&td->hwCBP) & ~0x0fff,
+			le32_to_cpup (&td->hwBE));
+		for (i = 0; i < MAXPSW; i++) {
+			dbg ("       psw [%d] = %2x", i,
+				le16_to_cpu (td->hwPSW [i]));
+		}
+	}
+}
+
+/* caller MUST own hcd spinlock if verbose is set! */
+static void __attribute__((unused))
+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",
+		ohci->hcd.self.bus_name, label,
+		ed, ed->state, ed->type,
+		le32_to_cpup (&ed->hwNextED));
+	switch (tmp & (ED_IN|ED_OUT)) {
+	case ED_OUT: type = "-OUT"; break;
+	case ED_IN: type = "-IN"; break;
+	/* else from TDs ... control */
+	}
+	dbg ("  info %08x MAX=%d%s%s%s EP=%d%s DEV=%d", le32_to_cpu (tmp),
+		0x0fff & (le32_to_cpu (tmp) >> 16),
+		(tmp & ED_ISO) ? " ISO" : "",
+		(tmp & ED_SKIP) ? " SKIP" : "",
+		(tmp & ED_LOWSPEED) ? " LOW" : "",
+		0x000f & (le32_to_cpu (tmp) >> 7),
+		type,
+		0x007f & le32_to_cpu (tmp));
+	dbg ("  tds: head %08x%s%s tail %08x%s",
+		tmp = le32_to_cpup (&ed->hwHeadP),
+		(ed->hwHeadP & ED_H) ? " HALT" : "",
+		(ed->hwHeadP & ED_C) ? " CARRY" : "",
+		le32_to_cpup (&ed->hwTailP),
+		verbose ? "" : " (not listing)");
+	if (verbose) {
+		struct list_head	*tmp;
+
+		/* use ed->td_list because HC concurrently modifies
+		 * hwNextTD as it accumulates ed_donelist.
+		 */
+		list_for_each (tmp, &ed->td_list) {
+			struct td		*td;
+			td = list_entry (tmp, struct td, td_list);
+			ohci_dump_td ("  ->", td);
+		}
+	}
+}
 
 #endif
 

drivers/usb/host/ohci-hcd.c

@@ -100,7 +100,7 @@
  *	- lots more testing!!
  */
 
-#define DRIVER_VERSION "2002-Jun-10"
+#define DRIVER_VERSION "2002-Jun-15"
 #define DRIVER_AUTHOR "Roman Weissgaerber <weissg@vienna.at>, David Brownell"
 #define DRIVER_DESC "USB 1.1 'Open' Host Controller (OHCI) Driver"
 
@@ -145,8 +145,8 @@ static int ohci_urb_enqueue (
 	urb_print (urb, "SUB", usb_pipein (pipe));
 #endif
 	
-	/* every endpoint has a ed, locate and fill it */
-	if (! (ed = ep_add_ed (urb->dev, pipe, urb->interval, 1, mem_flags)))
+	/* every endpoint has a ed, locate and maybe (re)initialize it */
+	if (! (ed = ed_get (ohci, urb->dev, pipe, urb->interval)))
 		return -ENOMEM;
 
 	/* for the private part of the URB we need the number of TDs (size) */
@@ -498,6 +498,7 @@ static void ohci_irq (struct usb_hcd *hcd)
 	struct ohci_regs	*regs = ohci->regs;
  	int			ints; 
 
+	/* we can eliminate a (slow) readl() if _only_ WDH caused this irq */
 	if ((ohci->hcca->done_head != 0)
 			&& ! (le32_to_cpup (&ohci->hcca->done_head) & 0x01)) {
 		ints =  OHCI_INTR_WDH;

drivers/usb/host/ohci-mem.c

@@ -221,6 +221,7 @@ ed_alloc (struct ohci_hcd *hc, int mem_flags)
 	ed = pci_pool_alloc (hc->ed_cache, mem_flags, &dma);
 	if (ed) {
 		memset (ed, 0, sizeof (*ed));
+		INIT_LIST_HEAD (&ed->td_list);
 		ed->dma = dma;
 		/* hash it for later reverse mapping */
 		if (!hash_add_ed (hc, ed, mem_flags)) {

drivers/usb/host/ohci-q.c

@@ -131,8 +131,9 @@ static void intr_resub (struct ohci_hcd *hc, struct urb *urb)
 
 /* search for the right branch to insert an interrupt ed into the int tree 
  * do some load balancing;
- * returns the branch and 
- * sets the interval to interval = 2^integer (ld (interval))
+ * returns the branch
+ * FIXME allow for failure, when there's no bandwidth left;
+ * and consider iso loads too
  */
 static int ep_int_balance (struct ohci_hcd *ohci, int interval, int load)
 {
@@ -152,19 +153,6 @@ static int ep_int_balance (struct ohci_hcd *ohci, int interval, int load)
 
 /*-------------------------------------------------------------------------*/
 
-/*  2^int ( ld (inter)) */
-
-static int ep_2_n_interval (int inter)
-{	
-	int	i;
-
-	for (i = 0; ((inter >> i) > 1 ) && (i < 5); i++)
-		continue;
-	return 1 << i;
-}
-
-/*-------------------------------------------------------------------------*/
-
 /* the int tree is a binary tree 
  * in order to process it sequentially the indexes of the branches have
  * to be mapped the mapping reverses the bits of a word of num_bits length
@@ -230,8 +218,7 @@ static int ep_link (struct ohci_hcd *ohci, struct ed *edi)
 
 	case PIPE_INTERRUPT:
 		load = ed->intriso.intr_info.int_load;
-		interval = ep_2_n_interval (ed->intriso.intr_info.int_period);
-		ed->interval = interval;
+		interval = ed->interval;
 		int_branch = ep_int_balance (ohci, interval, load);
 		ed->intriso.intr_info.int_branch = int_branch;
 
@@ -301,6 +288,7 @@ static void periodic_unlink (
  * just the link to the ed is unlinked.
  * the link from the ed still points to another operational ed or 0
  * so the HC can eventually finish the processing of the unlinked ed
+ * caller guarantees the ED has no active TDs.
  */
 static int start_ed_unlink (struct ohci_hcd *ohci, struct ed *ed) 
 {
@@ -387,84 +375,99 @@ static int start_ed_unlink (struct ohci_hcd *ohci, struct ed *ed)
 
 /*-------------------------------------------------------------------------*/
 
-/* (re)init an endpoint; this _should_ be done once at the
- * usb_set_configuration command, but the USB stack is a bit stateless
- * so we do it at every transaction.
- * if the state of the ed is ED_NEW then a dummy td is added and the
- * state is changed to ED_UNLINK
- * in all other cases the state is left unchanged
- * the ed info fields are set even though most of them should
- * not change
+/* get and maybe (re)init an endpoint. init _should_ be done only as part
+ * of usb_set_configuration() or usb_set_interface() ... but the USB stack
+ * isn't very stateful, so we re-init whenever the HC isn't looking.
  */
-static struct ed *ep_add_ed (
+static struct ed *ed_get (
+	struct ohci_hcd		*ohci,
 	struct usb_device	*udev,
 	unsigned int		pipe,
-	int			interval,
-	int			load,
-	int			mem_flags
+	int			interval
 ) {
-   	struct ohci_hcd		*ohci = hcd_to_ohci (udev->bus->hcpriv);
+	int			is_out = !usb_pipein (pipe);
+	int			type = usb_pipetype (pipe);
+	int			bus_msecs = 0;
 	struct hcd_dev		*dev = (struct hcd_dev *) udev->hcpriv;
-	struct td		*td;
 	struct ed		*ed; 
 	unsigned		ep;
 	unsigned long		flags;
 
-	spin_lock_irqsave (&ohci->lock, flags);
-
 	ep = usb_pipeendpoint (pipe) << 1;
-	if (!usb_pipecontrol (pipe) && usb_pipeout (pipe))
+	if (type != PIPE_CONTROL && is_out)
 		ep |= 1;
+	if (type == PIPE_INTERRUPT)
+		bus_msecs = usb_calc_bus_time (udev->speed, !is_out, 0,
+			usb_maxpacket (udev, pipe, is_out)) / 1000;
+
+	spin_lock_irqsave (&ohci->lock, flags);
+
 	if (!(ed = dev->ep [ep])) {
 		ed = ed_alloc (ohci, SLAB_ATOMIC);
 		if (!ed) {
 			/* out of memory */
-			spin_unlock_irqrestore (&ohci->lock, flags);
-			return NULL;
+			goto done;
 		}
 		dev->ep [ep] = ed;
 	}
 
 	if (ed->state & ED_URB_DEL) {
 		/* pending unlink request */
-		spin_unlock_irqrestore (&ohci->lock, flags);
-		return NULL;
+		ed = 0;
+		goto done;
 	}
 
 	if (ed->state == ED_NEW) {
+		struct td		*td;
+
 		ed->hwINFO = ED_SKIP;
   		/* dummy td; end of td list for ed */
 		td = td_alloc (ohci, SLAB_ATOMIC);
  		if (!td) {
 			/* out of memory */
-			spin_unlock_irqrestore (&ohci->lock, flags);
-			return NULL;
+			ed = 0;
+			goto done;
 		}
 		ed->dummy = td;
 		ed->hwTailP = cpu_to_le32 (td->td_dma);
 		ed->hwHeadP = ed->hwTailP;	/* ED_C, ED_H zeroed */
 		ed->state = ED_UNLINK;
-		ed->type = usb_pipetype (pipe);
+		ed->type = type;
 	}
 
-// FIXME:  don't do this if it's linked to the HC, or without knowing it's
-// safe to clobber state/mode info tied to (previous) config/altsetting.
-// (but dev0/ep0, used by set_address, must get clobbered)
-
-	ed->hwINFO = cpu_to_le32 (usb_pipedevice (pipe)
-			| usb_pipeendpoint (pipe) << 7
-			| (usb_pipeisoc (pipe)? 0x8000: 0)
-			| (usb_pipecontrol (pipe)
-				? 0: (usb_pipeout (pipe)? 0x800: 0x1000)) 
-			| (udev->speed == USB_SPEED_LOW) << 13
-			| usb_maxpacket (udev, pipe, usb_pipeout (pipe))
-				<< 16);
-
-  	if (ed->type == PIPE_INTERRUPT && ed->state == ED_UNLINK) {
-  		ed->intriso.intr_info.int_period = interval;
-  		ed->intriso.intr_info.int_load = load;
-  	}
+	/* FIXME:  Don't do this without knowing it's safe to clobber this
+	 * state/mode info.  Currently the upper layers don't support such
+	 * guarantees; we're lucky changing config/altsetting is rare.
+	 */
+  	if (ed->state == ED_UNLINK) {
+		u32	info;
+
+		info = usb_pipedevice (pipe);
+		info |= (ep >> 1) << 7;
+		info |= usb_maxpacket (udev, pipe, is_out) << 16;
+		info = cpu_to_le32 (info);
+		if (udev->speed == USB_SPEED_LOW)
+			info |= ED_LOWSPEED;
+		/* control transfers store pids in tds */
+		if (type != PIPE_CONTROL) {
+			info |= is_out ? ED_OUT : ED_IN;
+			if (type == PIPE_ISOCHRONOUS)
+				info |= ED_ISO;
+			if (type == PIPE_INTERRUPT) {
+				ed->intriso.intr_info.int_load = bus_msecs;
+				if (interval > 32)
+					interval = 32;
+			}
+		}
+		ed->hwINFO = info;
 
+		/* value ignored except on periodic EDs, where
+		 * we know it's already a power of 2
+		 */
+		ed->interval = interval;
+	}
+
+done:
 	spin_unlock_irqrestore (&ohci->lock, flags);
 	return ed; 
 }
@@ -736,8 +739,8 @@ static void td_done (struct urb *urb, struct td *td)
 		urb->iso_frame_desc [td->index].status = cc_to_error [cc];
 
 		if (cc != 0)
-			dbg ("  urb %p iso TD %d len %d CC %d",
-				urb, td->index, dlen, cc);
+			dbg ("  urb %p iso TD %p (%d) len %d CC %d",
+				urb, td, 1 + td->index, dlen, cc);
 
 	/* BULK, INT, CONTROL ... drivers see aggregate length/status,
 	 * except that "setup" bytes aren't counted and "short" transfers
@@ -776,9 +779,13 @@ static void td_done (struct urb *urb, struct td *td)
 					- td->data_dma;
 		}
 
+#ifdef VERBOSE_DEBUG
 		if (cc != 0)
-			dbg ("  urb %p TD %d CC %d, len=%d",
-				urb, td->index, cc, urb->actual_length);
+			dbg ("  urb %p TD %p (%d) CC %d, len=%d/%d",
+				urb, td, 1 + td->index, cc,
+				urb->actual_length,
+				urb->transfer_buffer_length);
+#endif
   	}
 }
 
@@ -812,8 +819,8 @@ static struct td *dl_reverse_done_list (struct ohci_hcd *ohci)
 				if (urb_priv && ((td_list->index + 1)
 						< urb_priv->length)) {
 #ifdef OHCI_VERBOSE_DEBUG
-					dbg ("urb %p TD %d of %d, patch ED",
-						td_list->urb,
+					dbg ("urb %p TD %p (%d/%d), patch ED",
+						td_list->urb, td_list,
 						1 + td_list->index,
 						urb_priv->length);
 #endif

drivers/usb/host/ohci.h

@@ -19,7 +19,7 @@ struct ed {
 #define ED_SKIP		__constant_cpu_to_le32(1 << 14)
 #define ED_LOWSPEED	__constant_cpu_to_le32(1 << 13)
 #define ED_OUT		__constant_cpu_to_le32(0x01 << 11)
-#define ED_IN		__constant_cpu_to_le32(0x10 << 11)
+#define ED_IN		__constant_cpu_to_le32(0x02 << 11)
 	__u32			hwTailP;	/* tail of TD list */
 	__u32			hwHeadP;	/* head of TD list */
 #define ED_C		__constant_cpu_to_le32(0x02)	/* toggle carry */
@@ -30,24 +30,24 @@ struct ed {
 	dma_addr_t		dma;		/* addr of ED */
 	struct ed		*ed_prev;	/* for non-interrupt EDs */
 	struct td		*dummy;
+	struct list_head	td_list;	/* "shadow list" of our TDs */
+
+	u8			state;		/* ED_{NEW,UNLINK,OPER} */
+#define ED_NEW 		0x00		/* unused, no dummy td */
+#define ED_UNLINK 	0x01		/* dummy td, maybe linked to hc */
+#define ED_OPER		0x02		/* dummy td, _is_ linked to hc */
+#define ED_URB_DEL  	0x08		/* for unlinking; masked in */
 
 	u8			type; 		/* PIPE_{BULK,...} */
-	u8			interval;	/* interrupt, isochronous */
+	u16			interval;	/* interrupt, isochronous */
 	union {
 		struct intr_info {		/* interrupt */
-			u8	int_period;
 			u8	int_branch;
 			u8	int_load; 
 		} intr_info;
 		u16		last_iso;	/* isochronous */
 	} intriso;
 
-	u8			state;		/* ED_{NEW,UNLINK,OPER} */
-#define ED_NEW 		0x00		/* unused, no dummy td */
-#define ED_UNLINK 	0x01		/* dummy td, maybe linked to hc */
-#define ED_OPER		0x02		/* dummy td, _is_ linked to hc */
-#define ED_URB_DEL  	0x08		/* for unlinking; masked in */
-
 	/* HC may see EDs on rm_list until next frame (frame_no == tick) */
 	u16			tick;
 	struct ed		*ed_rm_list;
@@ -108,6 +108,8 @@ struct td {
 
 	dma_addr_t	td_dma;		/* addr of this TD */
 	dma_addr_t	data_dma;	/* addr of data it points to */
+
+	struct list_head td_list;	/* "shadow list", TDs on same ED */
 } __attribute__ ((aligned(32)));	/* c/b/i need 16; only iso needs 32 */
 
 #define TD_MASK	((u32)~0x1f)		/* strip hw status in low addr bits */