usb: xhci: allow imod-interval to be configurable

PATCH Superseded Adam Wallis 4 participants 6 messages Nov 29, 2017 – Dec 1, 2017

Subsystems (5)

Device tree bindings USB Arm MediaTek USB xHCI driver +1 more subsystems

lore

Share & tools

lore

Adam Wallis

author

2017-11-29 15:52 UTC #0 lore raw

PATCH v2 1/1 usb: xhci: allow imod-interval to be configurable

The xHCI driver currently has the IMOD set to 160, which translates to an IMOD interval of 40,000ns (160 * 250)ns

Commit 0cbd4b34cda9 ("xhci: mediatek: support MTK xHCI host controller") introduced a QUIRK for the MTK platform to adjust this interval to 20, which translates to an IMOD interval of 5,000ns (20 * 250)ns. This is due to the fact that the MTK controller IMOD interval is 8 times as much as defined in xHCI spec.

Instead of adding more quirk bits for additional platforms, this patch introduces the ability for vendors to set the IMOD_INTERVAL as is optimal for their platform. By using device_property_read_u32() on "imod-interval", the IMOD INTERVAL can be specified in nano seconds. If no interval is specified, the default of 40,000ns (IMOD=160) will be used.

No bounds checking has been implemented due to the fact that a vendor may have violated the spec and would need to specify a value outside of the max 8,000 IRQs/second limit specified in the xHCI spec.

Backwards compatibility is maintained for MTK_HOSTS through the quirk bit, however, imod_interval should be pushed into device tree at a future point and this quirk should be removed from xhci_plat_probe

Signed-off-by Adam Wallis <[email protected]>

---
changes from v1:
* Removed device_property_read_u32() per suggestion from greg k-h
* Used ER_IRQ_INTERVAL_MASK in place of (u16) cast

 Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
 drivers/usb/host/xhci-plat.c                       | 13 +++++++++++++
 drivers/usb/host/xhci.c                            |  7 ++-----
 drivers/usb/host/xhci.h                            |  2 ++
 4 files changed, 18 insertions(+), 5 deletions(-)

Documentation/devicetree/bindings/usb/usb-xhci.txt +1 -0

index ae6e484..3998459 100644
@@ -29,6 +29,7 @@ Optional properties:
   - usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
   - usb3-lpm-capable: determines if platform is USB3 LPM capable
   - quirk-broken-port-ped: set if the controller has broken port disable mechanism
+  - imod-interval: IMOD_INTERVAL in nano-seconds. Default is 40000
 
 Example:
 	usb at f0931000 {

drivers/usb/host/xhci-plat.c +13 -0

index 09f164f..3c63de1 100644
@@ -23,6 +23,7 @@
 #include "xhci-plat.h"
 #include "xhci-mvebu.h"
 #include "xhci-rcar.h"
+#include "xhci-mtk.h"
 
 static struct hc_driver __read_mostly xhci_plat_hc_driver;

@@ -269,6 +270,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
 		xhci->quirks |= XHCI_BROKEN_PORT_PED;
 
+	/*
+	 * imod_interval is the interrupt modulation value in nanoseconds.
+	 * The increment interval is 8 times as much as that defined in
+	 * the xHCI spec on MTK's controller. This quirk check exists to provide
+	 * backwards compatibility, however, this should be pushed into
+	 * the device tree files at some point in the future and
+	 * checking the quirk should be removed from xhci_plat_probe.
+	 */
+	xhci->imod_interval = xhci->quirks & XHCI_MTK_HOST ? 5000 : 40000;
+
+	device_property_read_u32(sysdev, "imod-interval", &xhci->imod_interval);
+
 	hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0);
 	if (IS_ERR(hcd->usb_phy)) {
 		ret = PTR_ERR(hcd->usb_phy);

drivers/usb/host/xhci.c +2 -5

index 2424d30..0b7755b 100644
@@ -586,11 +586,8 @@ int xhci_run(struct usb_hcd *hcd)
 			"// Set the interrupt modulation register");
 	temp = readl(&xhci->ir_set->irq_control);
 	temp &= ~ER_IRQ_INTERVAL_MASK;
-	/*
-	 * the increment interval is 8 times as much as that defined
-	 * in xHCI spec on MTK's controller
-	 */
-	temp |= (u32) ((xhci->quirks & XHCI_MTK_HOST) ? 20 : 160);
+	temp |= (xhci->imod_interval / 250) & ER_IRQ_INTERVAL_MASK;
+
 	writel(temp, &xhci->ir_set->irq_control);
 
 	/* Set the HCD state before we enable the irqs */

drivers/usb/host/xhci.h +2 -0

index 99a014a..2a4177b 100644
@@ -1717,6 +1717,8 @@ struct xhci_hcd {
 	u8		max_interrupters;
 	u8		max_ports;
 	u8		isoc_threshold;
+	/* imod_interval in ns (I * 250ns) */
+	u32		imod_interval;
 	int		event_ring_max;
 	/* 4KB min, 128MB max */
 	int		page_size;

Signature · 3 lines

Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Greg Kroah-Hartman

maintainer

2017-11-29 17:46 UTC #1 lore raw

PATCH v2 1/1 usb: xhci: allow imod-interval to be configurable

Quoted 63 lines · Adam Wallis

The xHCI driver currently has the IMOD set to 160, which translates to an IMOD interval of 40,000ns (160 * 250)ns
Commit 0cbd4b34cda9 ("xhci: mediatek: support MTK xHCI host controller") introduced a QUIRK for the MTK platform to adjust this interval to 20, which translates to an IMOD interval of 5,000ns (20 * 250)ns. This is due to the fact that the MTK controller IMOD interval is 8 times as much as defined in xHCI spec.
Instead of adding more quirk bits for additional platforms, this patch introduces the ability for vendors to set the IMOD_INTERVAL as is optimal for their platform. By using device_property_read_u32() on "imod-interval", the IMOD INTERVAL can be specified in nano seconds. If no interval is specified, the default of 40,000ns (IMOD=160) will be used.
No bounds checking has been implemented due to the fact that a vendor may have violated the spec and would need to specify a value outside of the max 8,000 IRQs/second limit specified in the xHCI spec.
Backwards compatibility is maintained for MTK_HOSTS through the quirk bit, however, imod_interval should be pushed into device tree at a future point and this quirk should be removed from xhci_plat_probe
Signed-off-by Adam Wallis <[email protected]>
---
changes from v1:
* Removed device_property_read_u32() per suggestion from greg k-h
* Used ER_IRQ_INTERVAL_MASK in place of (u16) cast
 Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
 drivers/usb/host/xhci-plat.c                       | 13 +++++++++++++
 drivers/usb/host/xhci.c                            |  7 ++-----
 drivers/usb/host/xhci.h                            |  2 ++
 4 files changed, 18 insertions(+), 5 deletions(-)
Documentation/devicetree/bindings/usb/usb-xhci.txt +1 -0
index ae6e484..3998459 100644
@@ -29,6 +29,7 @@ Optional properties:
   - usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
   - usb3-lpm-capable: determines if platform is USB3 LPM capable
   - quirk-broken-port-ped: set if the controller has broken port disable mechanism
+  - imod-interval: IMOD_INTERVAL in nano-seconds. Default is 40000
 
 Example:
 	usb at f0931000 {
drivers/usb/host/xhci-plat.c +1 -0
index 09f164f..3c63de1 100644
@@ -23,6 +23,7 @@
 #include "xhci-plat.h"
 #include "xhci-mvebu.h"
 #include "xhci-rcar.h"
+#include "xhci-mtk.h"
 
 static struct hc_driver __read_mostly xhci_plat_hc_driver;
 
@@ -269,6 +270,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
 		xhci->quirks |= XHCI_BROKEN_PORT_PED;

drivers/usb/host/xhci-plat.c static int xhci_plat_probe(struct platform_device *pdev) · line 281

Quoted 10 lines

drivers/usb/host/xhci-plat.c +9 -0

 
+	/*
+	 * imod_interval is the interrupt modulation value in nanoseconds.
+	 * The increment interval is 8 times as much as that defined in
+	 * the xHCI spec on MTK's controller. This quirk check exists to provide
+	 * backwards compatibility, however, this should be pushed into
+	 * the device tree files at some point in the future and
+	 * checking the quirk should be removed from xhci_plat_probe.
+	 */
+	xhci->imod_interval = xhci->quirks & XHCI_MTK_HOST ? 5000 : 40000;

Personally I hate ? : logic, just write the if statement out. It doesn't save anything except make it harder to read.

Yeah, the original code had it too. Oh well, I'll leave it up to the xhci maintainer, he is the one that has to maintain this, not me :)

thanks,

greg k-h

Mathias Nyman

2017-11-30 08:41 UTC #2 lore raw

PATCH v2 1/1 usb: xhci: allow imod-interval to be configurable

Quoted 78 lines · Greg Kroah-Hartman

Quoted 63 lines · Adam Wallis
The xHCI driver currently has the IMOD set to 160, which translates to an IMOD interval of 40,000ns (160 * 250)ns
Commit 0cbd4b34cda9 ("xhci: mediatek: support MTK xHCI host controller") introduced a QUIRK for the MTK platform to adjust this interval to 20, which translates to an IMOD interval of 5,000ns (20 * 250)ns. This is due to the fact that the MTK controller IMOD interval is 8 times as much as defined in xHCI spec.
Instead of adding more quirk bits for additional platforms, this patch introduces the ability for vendors to set the IMOD_INTERVAL as is optimal for their platform. By using device_property_read_u32() on "imod-interval", the IMOD INTERVAL can be specified in nano seconds. If no interval is specified, the default of 40,000ns (IMOD=160) will be used.
No bounds checking has been implemented due to the fact that a vendor may have violated the spec and would need to specify a value outside of the max 8,000 IRQs/second limit specified in the xHCI spec.
Backwards compatibility is maintained for MTK_HOSTS through the quirk bit, however, imod_interval should be pushed into device tree at a future point and this quirk should be removed from xhci_plat_probe
Signed-off-by Adam Wallis <[email protected]>
---
changes from v1:
* Removed device_property_read_u32() per suggestion from greg k-h
* Used ER_IRQ_INTERVAL_MASK in place of (u16) cast
  Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
  drivers/usb/host/xhci-plat.c                       | 13 +++++++++++++
  drivers/usb/host/xhci.c                            |  7 ++-----
  drivers/usb/host/xhci.h                            |  2 ++
  4 files changed, 18 insertions(+), 5 deletions(-)
Documentation/devicetree/bindings/usb/usb-xhci.txt +1 -0
index ae6e484..3998459 100644
@@ -29,6 +29,7 @@ Optional properties:
    - usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
    - usb3-lpm-capable: determines if platform is USB3 LPM capable
    - quirk-broken-port-ped: set if the controller has broken port disable mechanism
+  - imod-interval: IMOD_INTERVAL in nano-seconds. Default is 40000
 
  Example:
  	usb at f0931000 {
drivers/usb/host/xhci-plat.c +1 -0
index 09f164f..3c63de1 100644
@@ -23,6 +23,7 @@
  #include "xhci-plat.h"
  #include "xhci-mvebu.h"
  #include "xhci-rcar.h"
+#include "xhci-mtk.h"
 
  static struct hc_driver __read_mostly xhci_plat_hc_driver;
 
@@ -269,6 +270,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
  	if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
  		xhci->quirks |= XHCI_BROKEN_PORT_PED;
drivers/usb/host/xhci-plat.c static int xhci_plat_probe(struct platform_device *pdev) · line 281
Quoted 10 lines
drivers/usb/host/xhci-plat.c +9 -0
 
+	/*
+	 * imod_interval is the interrupt modulation value in nanoseconds.
+	 * The increment interval is 8 times as much as that defined in
+	 * the xHCI spec on MTK's controller. This quirk check exists to provide
+	 * backwards compatibility, however, this should be pushed into
+	 * the device tree files at some point in the future and
+	 * checking the quirk should be removed from xhci_plat_probe.
+	 */
+	xhci->imod_interval = xhci->quirks & XHCI_MTK_HOST ? 5000 : 40000;
Personally I hate ? : logic, just write the if statement out. It doesn't save anything except make it harder to read.

Quoted 3 lines

Yeah, the original code had it too. Oh well, I'll leave it up to the xhci maintainer, he is the one that has to maintain this, not me :)

I don't mind that line so much, this still looks better than the original.

I do however mind that I don't get any default imod interval value for my pci based xhci host after this patch.
That needs to be fixed

Thanks
-Mathias

Adam Wallis

author

2017-11-30 17:52 UTC #3 lore raw

PATCH v2 1/1 usb: xhci: allow imod-interval to be configurable

On 11/30/2017 3:41 AM, Mathias Nyman wrote:
[..]

Quoted 4 lines

I do however mind that I don't get any default imod interval value for my pci based xhci host after this patch.
That needs to be fixed

Thanks Mathias, good catch. Where do you think I should initialize imod default in PCI subsytem? I was thinking about in xhci_pci_setup but would like to hear your thoughts since I deal much less often with USB PCI.

Quoted 7 lines

Thanks
-Mathias
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Adam

Signature · 4 lines

Adam Wallis
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Chunfeng Yun

maintainer

2017-12-01 03:32 UTC #4 lore raw

PATCH v2 1/1 usb: xhci: allow imod-interval to be configurable

In reply to Adam Wallis

Hi,

Quoted 53 lines · Adam Wallis

The xHCI driver currently has the IMOD set to 160, which translates to an IMOD interval of 40,000ns (160 * 250)ns
Commit 0cbd4b34cda9 ("xhci: mediatek: support MTK xHCI host controller") introduced a QUIRK for the MTK platform to adjust this interval to 20, which translates to an IMOD interval of 5,000ns (20 * 250)ns. This is due to the fact that the MTK controller IMOD interval is 8 times as much as defined in xHCI spec.
Instead of adding more quirk bits for additional platforms, this patch introduces the ability for vendors to set the IMOD_INTERVAL as is optimal for their platform. By using device_property_read_u32() on "imod-interval", the IMOD INTERVAL can be specified in nano seconds. If no interval is specified, the default of 40,000ns (IMOD=160) will be used.
No bounds checking has been implemented due to the fact that a vendor may have violated the spec and would need to specify a value outside of the max 8,000 IRQs/second limit specified in the xHCI spec.
Backwards compatibility is maintained for MTK_HOSTS through the quirk bit, however, imod_interval should be pushed into device tree at a future point and this quirk should be removed from xhci_plat_probe
Signed-off-by Adam Wallis <[email protected]>
---
changes from v1:
* Removed device_property_read_u32() per suggestion from greg k-h
* Used ER_IRQ_INTERVAL_MASK in place of (u16) cast
 Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
 drivers/usb/host/xhci-plat.c                       | 13 +++++++++++++
 drivers/usb/host/xhci.c                            |  7 ++-----
 drivers/usb/host/xhci.h                            |  2 ++
 4 files changed, 18 insertions(+), 5 deletions(-)
Documentation/devicetree/bindings/usb/usb-xhci.txt +1 -0
index ae6e484..3998459 100644
@@ -29,6 +29,7 @@ Optional properties:
   - usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
   - usb3-lpm-capable: determines if platform is USB3 LPM capable
   - quirk-broken-port-ped: set if the controller has broken port disable mechanism
+  - imod-interval: IMOD_INTERVAL in nano-seconds. Default is 40000
 
 Example:
 	usb at f0931000 {
drivers/usb/host/xhci-plat.c +0 -0
index 09f164f..3c63de1 100644

drivers/usb/host/xhci-plat.c line 26

Quoted 4 lines

drivers/usb/host/xhci-plat.c +1 -0

@@ -23,6 +23,7 @@
 #include "xhci-plat.h"
 #include "xhci-mvebu.h"
 #include "xhci-rcar.h"
+#include "xhci-mtk.h"

Needn't it, MTK makes use of xhci-mtk.c but not xhci-plat.c

Quoted 6 lines

drivers/usb/host/xhci-plat.c +0 -0

 static struct hc_driver __read_mostly xhci_plat_hc_driver;

@@ -269,6 +270,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
 		xhci->quirks |= XHCI_BROKEN_PORT_PED;

drivers/usb/host/xhci-plat.c static int xhci_plat_probe(struct platform_device *pdev) · line 281

Quoted 10 lines

drivers/usb/host/xhci-plat.c +9 -0

 
+	/*
+	 * imod_interval is the interrupt modulation value in nanoseconds.
+	 * The increment interval is 8 times as much as that defined in
+	 * the xHCI spec on MTK's controller. This quirk check exists to provide
+	 * backwards compatibility, however, this should be pushed into
+	 * the device tree files at some point in the future and
+	 * checking the quirk should be removed from xhci_plat_probe.
+	 */
+	xhci->imod_interval = xhci->quirks & XHCI_MTK_HOST ? 5000 : 40000;

Can be moved into xhci-mtk.c for MTK quirk, like as following:

drivers/usb/host/xhci-mtk.c +7 -0

index 2d473e0..a03451b 100644
@@ -699,6 +699,13 @@ static int xhci_mtk_probe(struct platform_device *pdev)
                goto power_off_phys;
        }
 
+       /*
+        * the increment interval is 8 times as much as that defined
+        * in xHCI spec on MTK's controller
+        */
+ 	xhci->imod_interval = 5000;
+       device_property_read_u32(dev, "imod-interval", &xhci->imod_interval);
+
        ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
        if (ret)
                goto put_usb3_hcd;

Thanks

Quoted 37 lines

drivers/usb/host/xhci-plat.c +3 -0

+
+	device_property_read_u32(sysdev, "imod-interval", &xhci->imod_interval);
+
 	hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0);
 	if (IS_ERR(hcd->usb_phy)) {
 		ret = PTR_ERR(hcd->usb_phy);

drivers/usb/host/xhci.c +2 -5

index 2424d30..0b7755b 100644
@@ -586,11 +586,8 @@ int xhci_run(struct usb_hcd *hcd)
 			"// Set the interrupt modulation register");
 	temp = readl(&xhci->ir_set->irq_control);
 	temp &= ~ER_IRQ_INTERVAL_MASK;
-	/*
-	 * the increment interval is 8 times as much as that defined
-	 * in xHCI spec on MTK's controller
-	 */
-	temp |= (u32) ((xhci->quirks & XHCI_MTK_HOST) ? 20 : 160);
+	temp |= (xhci->imod_interval / 250) & ER_IRQ_INTERVAL_MASK;
+
 	writel(temp, &xhci->ir_set->irq_control);
 
 	/* Set the HCD state before we enable the irqs */

drivers/usb/host/xhci.h +2 -0

index 99a014a..2a4177b 100644
@@ -1717,6 +1717,8 @@ struct xhci_hcd {
 	u8		max_interrupters;
 	u8		max_ports;
 	u8		isoc_threshold;
+	/* imod_interval in ns (I * 250ns) */
+	u32		imod_interval;
 	int		event_ring_max;
 	/* 4KB min, 128MB max */
 	int		page_size;

Adam Wallis

author

2017-12-01 15:50 UTC #5 lore raw

PATCH v2 1/1 usb: xhci: allow imod-interval to be configurable

Hi Chunfeng,

Quoted 7 lines · Chunfeng Yun

Hi,
Quoted 5 lines · Adam Wallis
The xHCI driver currently has the IMOD set to 160, which translates to an IMOD interval of 40,000ns (160 * 250)ns
Commit 0cbd4b34cda9 ("xhci: mediatek: support MTK xHCI host controller") introduced a QUIRK for the MTK platform to adjust this interval to 20,

[..]

Quoted 5 lines

Quoted 3 lines
#include "xhci-mvebu.h"
#include "xhci-rcar.h"
+#include "xhci-mtk.h"
Needn't it, MTK makes use of xhci-mtk.c but not xhci-plat.c

Thanks - I made this change in V3.

[..]

Quoted 7 lines

static struct hc_driver __read_mostly xhci_plat_hc_driver;

(quoted diff) +0 -0

@@ -269,6 +270,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
 		xhci->quirks |= XHCI_BROKEN_PORT_PED;

Quoted 11 lines

Review static int xhci_plat_probe(struct platform_device *pdev) · line 281

Quoted 9 lines

(quoted diff) +9 -0

+	/*
+	 * imod_interval is the interrupt modulation value in nanoseconds.
+	 * The increment interval is 8 times as much as that defined in
+	 * the xHCI spec on MTK's controller. This quirk check exists to provide
+	 * backwards compatibility, however, this should be pushed into
+	 * the device tree files at some point in the future and
+	 * checking the quirk should be removed from xhci_plat_probe.
+	 */
+	xhci->imod_interval = xhci->quirks & XHCI_MTK_HOST ? 5000 : 40000;

Can be moved into xhci-mtk.c for MTK quirk, like as following:

I moved this into V3 as suggested - could you test on an MTK platform (I don't have one available) to see if it performs as expected?

Signature · 4 lines

Adam Wallis
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.