USB: host: ehci: allow tine of highspeed nak-count

From: Ben Dooks <[email protected]>

At least some systems benefit with less scheduling if the NAK count value is set higher than the default 4. For instance a Tegra3 with an SMSC9512 showed less interrupt load when this was changed to 14.

To allow the changing of this value, add a sysfs node to each of the controllers to allow run-time changing.

Signed-off-by Ben Dooks <[email protected]>

---
 drivers/usb/host/ehci-hcd.c   |  1 +
 drivers/usb/host/ehci-q.c     |  4 +--
 drivers/usb/host/ehci-sysfs.c | 52 +++++++++++++++++++++++++++++++++--
 drivers/usb/host/ehci.h       |  1 +
 4 files changed, 54 insertions(+), 4 deletions(-)

drivers/usb/host/ehci-hcd.c +1 -0

index 8608ac513fb7..799262951f41 100644
@@ -526,6 +526,7 @@ static int ehci_init(struct usb_hcd *hcd)
 	hw->hw_qtd_next = EHCI_LIST_END(ehci);
 	ehci->async->qh_state = QH_STATE_LINKED;
 	hw->hw_alt_next = QTD_NEXT(ehci, ehci->async->dummy->qtd_dma);
+	ehci->nak_tune_hs = EHCI_TUNE_RL_HS;
 
 	/* clear interrupt enables, set irq latency */
 	if (log2_irq_thresh < 0 || log2_irq_thresh > 6)

drivers/usb/host/ehci-q.c +2 -2

index 327630405695..ccb754893b5a 100644
@@ -898,12 +898,12 @@ qh_make (
 	case USB_SPEED_HIGH:		/* no TT involved */
 		info1 |= QH_HIGH_SPEED;
 		if (type == PIPE_CONTROL) {
-			info1 |= (EHCI_TUNE_RL_HS << 28);
+			info1 |= ehci->nak_tune_hs << 28;
 			info1 |= 64 << 16;	/* usb2 fixed maxpacket */
 			info1 |= QH_TOGGLE_CTL;	/* toggle from qtd */
 			info2 |= (EHCI_TUNE_MULT_HS << 30);
 		} else if (type == PIPE_BULK) {
-			info1 |= (EHCI_TUNE_RL_HS << 28);
+			info1 |= ehci->nak_tune_hs << 28;
 			/* The USB spec says that high speed bulk endpoints
 			 * always use 512 byte maxpacket.  But some device
 			 * vendors decided to ignore that, and MSFT is happy

drivers/usb/host/ehci-sysfs.c +50 -2

index 8f75cb7b197c..d710d35282a6 100644
@@ -145,19 +145,66 @@ static ssize_t uframe_periodic_max_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(uframe_periodic_max);
 
+static ssize_t nak_tune_hs_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct ehci_hcd		*ehci;
+
+	ehci = hcd_to_ehci(dev_get_drvdata(dev));
+	return scnprintf(buf, PAGE_SIZE, "%d\n", ehci->nak_tune_hs);
+}
+
+static ssize_t nak_tune_hs_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	struct ehci_hcd		*ehci;
+	unsigned		val;
+	unsigned long		flags;
+
+	ehci = hcd_to_ehci(dev_get_drvdata(dev));
+
+	if (kstrtouint(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (val >= 15) {
+		ehci_info(ehci, "invalid value for nak_tune_hs (%d)\n", val);
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave (&ehci->lock, flags);
+	ehci->nak_tune_hs = val;
+	spin_unlock_irqrestore (&ehci->lock, flags);
+	return count;
+}
+
+static DEVICE_ATTR_RW(nak_tune_hs);
 
 static inline int create_sysfs_files(struct ehci_hcd *ehci)
 {
 	struct device	*controller = ehci_to_hcd(ehci)->self.controller;
 	int	i = 0;
 
+	i = device_create_file(controller, &dev_attr_nak_tune_hs);
+	if (i)
+		goto out;
+
+	i = device_create_file(controller, &dev_attr_uframe_periodic_max);
+	if (i)
+		goto out_nak;
+
 	/* with integrated TT there is no companion! */
 	if (!ehci_is_TDI(ehci))
 		i = device_create_file(controller, &dev_attr_companion);
 	if (i)
-		goto out;
+		goto out_all;
 
-	i = device_create_file(controller, &dev_attr_uframe_periodic_max);
+	return 0;
+out_all:
+	device_remove_file(controller, &dev_attr_uframe_periodic_max);
+out_nak:
+	device_remove_file(controller, &dev_attr_nak_tune_hs);
 out:
 	return i;
 }

@@ -170,5 +217,6 @@ static inline void remove_sysfs_files(struct ehci_hcd *ehci)
 	if (!ehci_is_TDI(ehci))
 		device_remove_file(controller, &dev_attr_companion);
 
+	device_remove_file(controller, &dev_attr_nak_tune_hs);
 	device_remove_file(controller, &dev_attr_uframe_periodic_max);
 }

drivers/usb/host/ehci.h +1 -0

index c8e9a48e1d51..1fb6f1ad8128 100644
@@ -154,6 +154,7 @@ struct ehci_hcd {			/* one per controller */
 	dma_addr_t		periodic_dma;
 	struct list_head	intr_qh_list;
 	unsigned		i_thresh;	/* uframes HC might cache */
+	u32			nak_tune_hs;
 
 	union ehci_shadow	*pshadow;	/* mirror hw periodic table */
 	struct list_head	intr_unlink_wait;

Quoted 5 lines · Ben Dooks

From: Ben Dooks <[email protected]>

At least some systems benefit with less scheduling if the NAK count value is set higher than the default 4. For instance a Tegra3 with an SMSC9512 showed less interrupt load when this was changed to 14.

Interesting. Why do you think this is? In theory, increasing the NAK count RL value should cause a higher memory bus load and perhaps a slight rise in the interrupt load (transfers will complete a little more quickly because the controller tries harder to poll the endpoints and see if they are ready).

Quoted 4 lines

To allow the changing of this value, add a sysfs node to each of the controllers to allow run-time changing.

Signed-off-by Ben Dooks <[email protected]>

The patch looks okay to me.

Quoted 8 lines · Alan Stern

Quoted 5 lines · Ben Dooks

From: Ben Dooks <[email protected]>

At least some systems benefit with less scheduling if the NAK count value is set higher than the default 4. For instance a Tegra3 with an SMSC9512 showed less interrupt load when this was changed to 14.

Quoted 5 lines

Interesting. Why do you think this is? In theory, increasing the NAK count RL value should cause a higher memory bus load and perhaps a slight rise in the interrupt load (transfers will complete a little more quickly because the controller tries harder to poll the endpoints and see if they are ready).

I thought the NAK counter was decremented until the transfer is given up on.

I'm going to have to go back and get some actual figures from a running system as this was originally done over a year ago with the SMSC9512 (IIRC) network driver.

Quoted 3 lines

Quoted 3 lines

To allow the changing of this value, add a sysfs node to each of the controllers to allow run-time changing.

Quoted 3 lines

Quoted 1 line

Signed-off-by Ben Dooks <[email protected]>

The patch looks okay to me.

I'll look at getting some tracing from the SMSC driver to see what is going on.

Quoted 9 lines · Ben Dooks

Quoted 8 lines · Alan Stern

Quoted 5 lines · Ben Dooks

From: Ben Dooks <[email protected]>

At least some systems benefit with less scheduling if the NAK count value is set higher than the default 4. For instance a Tegra3 with an SMSC9512 showed less interrupt load when this was changed to 14.

Quoted 8 lines

Quoted 5 lines

Interesting. Why do you think this is? In theory, increasing the NAK count RL value should cause a higher memory bus load and perhaps a slight rise in the interrupt load (transfers will complete a little more quickly because the controller tries harder to poll the endpoints and see if they are ready).

I thought the NAK counter was decremented until the transfer is given up on.

That's right. So if the RL value is higher, there will be more polling attempts in quick succession before the NAK counter drops to 0 and the controller gives up. More polling attempts in quick succession means heavier memory bus usage.

Quoted 7 lines

I'm going to have to go back and get some actual figures from a running system as this was originally done over a year ago with the SMSC9512 (IIRC) network driver.

Quoted 3 lines

Quoted 3 lines

To allow the changing of this value, add a sysfs node to each of the controllers to allow run-time changing.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Signed-off-by Ben Dooks <[email protected]>

The patch looks okay to me.

I'll look at getting some tracing from the SMSC driver to see what is going on.

Okay. Should we consider the patch to be held in suspense until then?

From: Ben Dooks <[email protected]>

At least some systems benefit with less scheduling if the NAK count value is set higher than the default 4. For instance a Tegra3 with an SMSC9512 showed less interrupt load when this was changed to 14.

To allow the changing of this value, add a sysfs node to each of the controllers to allow run-time changing.