Re: [PATCH] EHCI interrupt transactions failing during CPU frequency changes

<[email protected]> Wed, 7 Mar 2007 15:10:28 -0600

Newsgroups	gmane.linux.usb.devel,gmane.spam.detected
Message-ID	<DFEF91B22ED07447AB6AA4B237F913F9258999@ausx3mpc125.aus.amer.dell.com>

David Brownell wrote:
> On Tuesday 20 February 2007 12:27 pm, [email protected] wrote:
>> 
>> This patch turns off periodic list processing in the EHCI controller
>> for the duration of processor frequency changes.
> 
> Did you test this with any kind of isochronous device active, like a
> set of USB speakers playing?  ISO transfers work differently than the
> interrupt transfers you tested (with a HID device).  
> 
> Also ... what kind of hardware did you test this with?  I wonder how
> much of the (potential) 1.125 msec it was spinning while waiting for
> the periodic schedule to actually turn off.  That seems like it
> should be modifying the latency metrics used by cpufreq... is there
> even a feedback mechanism whereby the system can say that _right now_
> it would take this much more time?  Last time I remember looking at
> cpufreq, it had static metrics basically relating only to the CPUs;
> so costs related to components like EHCI were hidden.  (Much less
> costs associated with re-clocking other peripherals, which is a big
> issue with embedded SOC chips!)         
> 
> 
> The patch looks mostly OK, but the issue with isochronous transfers
> is a difference in how transfer completion is handled in the
> hardware.  
> 
> Rather than scanning a queue head node in the schedule tree, which
> will be re-scanned after the periodic list is re-activated.  Instead,
> isochronous transfers use an entirely different hardware mechanism.  
> Their ITDs go before that schedule tree, and won't get re-scanned
> after the relevant frame passes. 
> 
> I'd not be sure that the current iso scanning logic would behave
> correctly with this kind of on/off mechanism.  In fact I'd kind of
> expect it to break.  
> 
> That's in addition to the issue that it might not be a Good Thing to
> let cpufreq create audio (or video etc) dropouts ... 
> 
> Maybe the best solution to this issue would be to reject the cpufreq
> change if ISO transfers are active on EHCI. 
> 
> - Dave

OK, I've made an attempt to solve this issue without using such a large
hammer.  Instead of shutting off the entire periodic schedule during
cpufreq changes, this patch will just inactivate (using the "I" bit in
qh->hw_info1, currently unused in the EHCI driver) any queue heads that
are to full/low speed interrupt endpoints, during the cpufreq changes.

I have found that the ICH7 works without this patch, because it caches 7
uframes of the periodic schedule, so it never has to read main memory in
order to complete a split transaction in time.  The Broadcom/Serverworks
HT1000 only caches 1 uframe, so it fails without this patch.

I have tested this code on both the HT1000 and ICH7 (I tested the patch
on the ICH7 with the cpufreq notifier registered, too, even though the
patch won't even register it, just to make sure it would work
correctly).

This patch adds more code, but it avoids a couple of the issues that
were brought up with the previous patch:

1--This patch doesn't mess with isochronous transfers (or high speed
interrupt transfers) at all.
2--The notifier will take very little time unless the cpufreq change
happens while a split transaction is in progress (or is already cached
in the controller).

I can repost it with a more concise summarization of the problem/fix and
a signed-off-by if it looks ok.

Thanks!
Stuart

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

take7_i_bit.patch (application/octet-stream, 10.1 KB)

diff -purN --exclude='*cmd' --exclude=tags linux-2.6.20orig/drivers/usb/host/ehci.h linux-2.6.20/drivers/usb/host/ehci.h
--- linux-2.6.20orig/drivers/usb/host/ehci.h	2007-02-04 12:44:54.000000000 -0600
+++ linux-2.6.20/drivers/usb/host/ehci.h	2007-03-07 05:25:46.000000000 -0600
@@ -55,6 +55,12 @@ struct ehci_hcd {			/* one per controlle
 	__u32			hcs_params;	/* cached register copy */
 	spinlock_t		lock;
 
+#ifdef CONFIG_CPU_FREQ
+	struct notifier_block	cpufreq_transition;
+	int			cpufreq_changing;
+	struct list_head	split_intr_qhs;
+#endif
+
 	/* async schedule support */
 	struct ehci_qh		*async;
 	struct ehci_qh		*reclaim;
@@ -390,6 +396,7 @@ struct ehci_qh {
 	__le32			hw_next;	 /* see EHCI 3.6.1 */
 	__le32			hw_info1;        /* see EHCI 3.6.2 */
 #define	QH_HEAD		0x00008000
+#define	QH_INACTIVATE	0x00000080
 	__le32			hw_info2;        /* see EHCI 3.6.2 */
 #define	QH_SMASK	0x000000ff
 #define	QH_CMASK	0x0000ff00
@@ -432,6 +439,10 @@ struct ehci_qh {
 	unsigned short		start;		/* where polling starts */
 #define NO_FRAME ((unsigned short)~0)			/* pick new start */
 	struct usb_device	*dev;		/* access to TT */
+#ifdef CONFIG_CPU_FREQ
+	struct list_head	split_intr_qhs; /* list of split qhs */
+	__le32			was_active;	/* active bit before "i" set */
+#endif
 } __attribute__ ((aligned (32)));
 
 /*-------------------------------------------------------------------------*/
diff -purN --exclude='*cmd' --exclude=tags linux-2.6.20orig/drivers/usb/host/ehci-hcd.c linux-2.6.20/drivers/usb/host/ehci-hcd.c
--- linux-2.6.20orig/drivers/usb/host/ehci-hcd.c	2007-02-04 12:44:54.000000000 -0600
+++ linux-2.6.20/drivers/usb/host/ehci-hcd.c	2007-03-07 05:36:45.000000000 -0600
@@ -268,6 +268,51 @@ static void ehci_work(struct ehci_hcd *e
 
 /*-------------------------------------------------------------------------*/
 
+#ifdef CONFIG_CPU_FREQ
+
+#include <linux/cpufreq.h>
+
+static void ehci_cpufreq_pause (struct ehci_hcd *ehci)
+{
+	unsigned long	flags;
+
+	spin_lock_irqsave(&ehci->lock, flags);
+	if (!ehci->cpufreq_changing++)
+		qh_inactivate_split_intr_qhs(ehci);
+	spin_unlock_irqrestore(&ehci->lock, flags);
+}
+
+static void ehci_cpufreq_unpause (struct ehci_hcd *ehci)
+{
+	unsigned long	flags;
+
+	spin_lock_irqsave(&ehci->lock, flags);
+	if (!--ehci->cpufreq_changing)
+		qh_reactivate_split_intr_qhs(ehci);
+	spin_unlock_irqrestore(&ehci->lock, flags);
+}
+
+static int ehci_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
+				 void *data)
+{
+	struct ehci_hcd *ehci = container_of(nb, struct ehci_hcd,
+					     cpufreq_transition);
+
+	switch (val) {
+	case CPUFREQ_PRECHANGE:
+		ehci_cpufreq_pause(ehci);
+		break;
+	case CPUFREQ_POSTCHANGE:
+		ehci_cpufreq_unpause(ehci);
+		break;
+	}
+	return 0;
+}
+
+#endif
+
+/*-------------------------------------------------------------------------*/
+
 static void ehci_watchdog (unsigned long param)
 {
 	struct ehci_hcd		*ehci = (struct ehci_hcd *) param;
@@ -308,6 +353,12 @@ ehci_shutdown (struct usb_hcd *hcd)
 	ehci = hcd_to_ehci (hcd);
 	(void) ehci_halt (ehci);
 
+#ifdef CONFIG_CPU_FREQ
+	if (ehci->i_thresh < 8) {
+		cpufreq_unregister_notifier(&ehci->cpufreq_transition,
+					    CPUFREQ_TRANSITION_NOTIFIER);
+	}
+#endif
 	/* make BIOS/etc use companion controller during reboot */
 	writel (0, &ehci->regs->configured_flag);
 }
@@ -485,6 +536,22 @@ static int ehci_init(struct usb_hcd *hcd
 	}
 	ehci->command = temp;
 
+#ifdef CONFIG_CPU_FREQ
+	INIT_LIST_HEAD (&ehci->split_intr_qhs);
+	if (ehci->i_thresh < 8) {
+		/*
+		 * If the EHCI controller caches enough uframes, this
+		 * isn't needed.  If there's a controller with an i_thresh
+		 * of 6-7, the QH inactivate/reactivate code may need
+		 * modification to work with QHs for full/low speed
+		 * devices behind a hub that have a period of 1.
+		 */
+		ehci->cpufreq_transition.notifier_call = ehci_cpufreq_notifier;
+		cpufreq_register_notifier(&ehci->cpufreq_transition,
+					  CPUFREQ_TRANSITION_NOTIFIER);
+	}
+#endif
+
 	return 0;
 }
 
diff -purN --exclude='*cmd' --exclude=tags linux-2.6.20orig/drivers/usb/host/ehci-mem.c linux-2.6.20/drivers/usb/host/ehci-mem.c
--- linux-2.6.20orig/drivers/usb/host/ehci-mem.c	2007-02-04 12:44:54.000000000 -0600
+++ linux-2.6.20/drivers/usb/host/ehci-mem.c	2007-03-02 05:32:52.000000000 -0600
@@ -94,6 +94,9 @@ static struct ehci_qh *ehci_qh_alloc (st
 	qh->qh_dma = dma;
 	// INIT_LIST_HEAD (&qh->qh_list);
 	INIT_LIST_HEAD (&qh->qtd_list);
+#ifdef CONFIG_CPU_FREQ
+	INIT_LIST_HEAD (&qh->split_intr_qhs);
+#endif
 
 	/* dummy td enables safe urb queuing */
 	qh->dummy = ehci_qtd_alloc (ehci, flags);
diff -purN --exclude='*cmd' --exclude=tags linux-2.6.20orig/drivers/usb/host/ehci-q.c linux-2.6.20/drivers/usb/host/ehci-q.c
--- linux-2.6.20orig/drivers/usb/host/ehci-q.c	2007-02-04 12:44:54.000000000 -0600
+++ linux-2.6.20/drivers/usb/host/ehci-q.c	2007-03-07 05:59:33.000000000 -0600
@@ -311,6 +311,10 @@ qh_completions (struct ehci_hcd *ehci, s
 		struct urb	*urb;
 		u32		token = 0;
 
+		/* ignore QHs that are currently inactive */
+		if (qh->hw_info1 & __constant_cpu_to_le32 (QH_INACTIVATE))
+			break;
+
 		qtd = list_entry (entry, struct ehci_qtd, qtd_list);
 		urb = qtd->urb;
 
diff -purN --exclude='*cmd' --exclude=tags linux-2.6.20orig/drivers/usb/host/ehci-sched.c linux-2.6.20/drivers/usb/host/ehci-sched.c
--- linux-2.6.20orig/drivers/usb/host/ehci-sched.c	2007-02-04 12:44:54.000000000 -0600
+++ linux-2.6.20/drivers/usb/host/ehci-sched.c	2007-03-07 05:25:15.000000000 -0600
@@ -473,6 +473,103 @@ static int disable_periodic (struct ehci
 }
 
 /*-------------------------------------------------------------------------*/
+#ifdef CONFIG_CPU_FREQ
+
+/* ignore/inactivate bit in QH hw_info1 */
+#define INACTIVATE_BIT __constant_cpu_to_le32 (QH_INACTIVATE)
+
+#define HALT_BIT __constant_cpu_to_le32(QTD_STS_HALT)
+#define ACTIVE_BIT __constant_cpu_to_le32(QTD_STS_ACTIVE)
+#define STATUS_BIT __constant_cpu_to_le32(QTD_STS_STS)
+
+static int safe_to_inactivate (struct ehci_hcd *ehci, struct ehci_qh *qh)
+{
+	/*
+	 * Some HCs (Broadcom/ServerWorks HT1000) will stop in the
+	 * middle of a split transaction when you set the "I" bit.
+	 */
+	int now; /* current (frame * 8) + uframe */
+	int prev_start, next_start; /* uframes from/to split start */
+	int start_uframe = ffs (le32_to_cpup (&qh->hw_info2) & QH_SMASK);
+
+	now = readl (&ehci->regs->frame_index) % (ehci->periodic_size << 3);
+
+	next_start = ((1024 << 3) + (qh->start << 3) + start_uframe - now) %
+		     (qh->period << 3);
+	prev_start = (qh->period << 3) - next_start;
+
+	if ((qh->period << 3) <= (ehci->i_thresh + 2)) {
+		/* Can't handle qh->period = 1 with ehci->i_thresh >= 6,
+		 * because we'd never have a uframe during which we know
+		 * we could inactivate the QH without it completing after
+		 * we set the "I" bit.
+		 */
+		return -EINVAL;
+	}
+
+	/*
+	 * Make sure the split transaction hasn't already started.  We'll see
+	 * QTD_STS_STS set if the split transaction was started (but not
+	 * done) if we wait until the 2nd frame after the split should have
+	 * started.
+	 * Also, to avoid setting "I" after the start split happens, don't
+	 * set the "I" bit in a uframe that might already be cached in the
+	 * controller when we set it (using ehci->i_thresh).
+	 *
+	 * It's easier to set the "i" bit when the transaction isn't active
+	 * than trying to figure out if it completed after we set the "i" bit!
+	 */
+	if ((next_start > ehci->i_thresh) && (prev_start > 1))
+		/* safe to set "i" bit if split isn't in progress */
+		return (qh->hw_token & STATUS_BIT) ? 0 : 1;
+	else
+		return 0;
+}
+
+static void qh_inactivate_split_intr_qhs (struct ehci_hcd *ehci)
+{
+	struct ehci_qh	*qh;
+	int not_done, safe;
+
+	/* Set inactivate bit for all the split interrupt QHs. */
+	do {
+		not_done = 0;
+		list_for_each_entry (qh, &ehci->split_intr_qhs,
+				     split_intr_qhs) {
+			if (qh->hw_info1 & INACTIVATE_BIT) /* already off */
+				continue;
+
+			safe = safe_to_inactivate (ehci, qh);
+			if (safe < 0) {
+				continue;
+			} else if (safe > 0) {
+				qh->was_active = qh->hw_token & ACTIVE_BIT; 
+				qh->hw_info1 |= INACTIVATE_BIT;
+			} else
+				not_done = 1;
+		}
+	} while (not_done);
+	wmb();
+}
+
+static void qh_reactivate_split_intr_qhs (struct ehci_hcd *ehci)
+{
+	struct ehci_qh	*qh;
+	u32		token;
+
+	list_for_each_entry (qh, &ehci->split_intr_qhs, split_intr_qhs) {
+		if (qh->hw_info1 & INACTIVATE_BIT) {
+			/* See EHCI 1.0 section 4.15.2.4 for description. */
+			token = qh->hw_token;
+			qh->hw_token = (token | HALT_BIT) & ~ACTIVE_BIT;
+			wmb();
+			qh->hw_info1 &= ~INACTIVATE_BIT;
+			wmb();
+			qh->hw_token = (token & ~HALT_BIT) | qh->was_active;
+		}
+	}
+}
+#endif
 
 /* periodic schedule slots have iso tds (normal or split) first, then a
  * sparse tree for active interrupt transfers.
@@ -490,6 +587,16 @@ static int qh_link_periodic (struct ehci
 		period, le32_to_cpup (&qh->hw_info2) & (QH_CMASK | QH_SMASK),
 		qh, qh->start, qh->usecs, qh->c_usecs);
 
+#ifdef CONFIG_CPU_FREQ
+	/*
+	 * If low/full speed interrupt QHs are inactive (because of
+	 * cpufreq changing processor speeds), start QH with I flag set--
+	 * it will automatically be cleared when cpufreq is done.
+	 */
+	if (ehci->cpufreq_changing)
+		qh->hw_info1 |= __constant_cpu_to_le32 (QH_INACTIVATE);
+#endif
+
 	/* high bandwidth, or otherwise every microframe */
 	if (period == 0)
 		period = 1;
@@ -538,6 +645,12 @@ static int qh_link_periodic (struct ehci
 		? ((qh->usecs + qh->c_usecs) / qh->period)
 		: (qh->usecs * 8);
 
+#ifdef CONFIG_CPU_FREQ
+	/* add qh to list of low/full speed interrupt QHs, if applicable */
+	if (!(qh->hw_info1 & (cpu_to_le32(1 << 13)))) {
+		list_add (&qh->split_intr_qhs, &ehci->split_intr_qhs);
+	}
+#endif
 	/* maybe enable periodic schedule processing */
 	if (!ehci->periodic_sched++)
 		return enable_periodic (ehci);
@@ -557,6 +670,13 @@ static void qh_unlink_periodic (struct e
 	// THEN
 	//   qh->hw_info1 |= __constant_cpu_to_le32 (1 << 7 /* "ignore" */);
 
+#ifdef CONFIG_CPU_FREQ
+	/* remove qh from list of low/full speed interrupt QHs */
+	if (!(qh->hw_info1 & (cpu_to_le32(1 << 13)))) {
+		list_del_init (&qh->split_intr_qhs);
+	}
+#endif
+
 	/* high bandwidth, or otherwise part of every microframe */
 	if ((period = qh->period) == 0)
 		period = 1;