USB: host: ehci: allow tine of highspeed nak-count
Subsystems (2)
PATCH 1/1 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.
--- 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
@@ -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
@@ -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
74 lines not on this page · raw message
drivers/usb/host/ehci.h +1 -0
@@ -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.
Alan Stern
Quoted 131 lines
--- 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@@ -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@@ -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@@ -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@@ -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 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 6 lines
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.
I'll look at getting some tracing from the SMSC driver to see what is going on.
Alan Stern
maintainerQuoted 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?
Alan Stern
Quoted 32 lines · Alan Stern
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 8 lines
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?
Yes, I'm not going to have access to any of the test hardware until the end of the week, and will re-verify my initial notes from last year.
Greg KH
maintainerQuoted 8 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.
To allow the changing of this value, add a sysfs node to each of the controllers to allow run-time changing.
That's going to be a pain, why can you not just figure this out at runtime and adjust it that way?
Also, you can't add a sysfs file and not also add a Documentation/API/ update :(
Can you fix this up to work without needing manual adjustments?
thanks,
greg k-h