usb: xhci: allow imod-interval to be configurable

PATCH New Adam Wallis 3 participants 6 messages Nov 28, 2017 – Dec 1, 2017

Subsystems (4)

Device tree bindings USB Arm MediaTek USB xHCI driver

lore

Share & tools

lore

Adam Wallis

author

2017-11-28 17:11 UTC #0 lore raw

PATCH 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]>

---
 Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
 drivers/usb/host/xhci-plat.c                       | 15 +++++++++++++++
 drivers/usb/host/xhci.c                            |  7 ++-----
 drivers/usb/host/xhci.h                            |  2 ++
 4 files changed, 20 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 +15 -0

index 09f164f..f7730c8 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,20 @@ 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 in nanoseconds */
+	if (device_property_read_u32(sysdev,
+		"imod-interval", &xhci->imod_interval))
+		xhci->imod_interval = 40000;
+	else if (xhci->quirks & XHCI_MTK_HOST)
+		/*
+		 * The increment interval is 8 times as much as that defined in
+		 * the xHCI spec on MTK's controller. This is added to provide
+		 * backwards compatibility, however, this should be pushed into
+		 * the device tree files at some point in the future and
+		 * removed from xhci_plat_probe
+		 */
+		xhci->imod_interval = 5000;
+
 	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..6a09311 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 |= (u16) (xhci->imod_interval / 250);
+
 	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-28 19:35 UTC #1 lore raw

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

Quoted 57 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]>
---
 Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
 drivers/usb/host/xhci-plat.c                       | 15 +++++++++++++++
 drivers/usb/host/xhci.c                            |  7 ++-----
 drivers/usb/host/xhci.h                            |  2 ++
 4 files changed, 20 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..f7730c8 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;
 

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

Quoted 7 lines

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

@@ -269,6 +270,20 @@ 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 in nanoseconds */
+	if (device_property_read_u32(sysdev,
+		"imod-interval", &xhci->imod_interval))
+		xhci->imod_interval = 40000;

So no matter what value you read, you set it to 40000? Or am I reading this code incorrectly?

There's a good reason putting function calls inside if() is considered a bad coding style :)

thanks,

greg k-h

Adam Wallis

author

2017-11-28 20:32 UTC #2 lore raw

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

Quoted 4 lines · Greg Kroah-Hartman

Quoted 3 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

[..]

Quoted 14 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"
 
 static struct hc_driver __read_mostly xhci_plat_hc_driver;

@@ -269,6 +270,20 @@ 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 7 lines

drivers/usb/host/xhci-plat.c static int xhci_plat_probe(struct platform_device *pdev) · line 276
Quoted 4 lines
drivers/usb/host/xhci-plat.c +4 -0
+	/* imod interval in nanoseconds */
+	if (device_property_read_u32(sysdev,
+		"imod-interval", &xhci->imod_interval))
+		xhci->imod_interval = 40000;
So no matter what value you read, you set it to 40000? Or am I reading this code incorrectly?

I think you may be reading the code incorrectly. device_property_read_u32() returns 0 when the property is found and valid...and stored into xhci->imod_interval. When 0 is returned in this case, the default value of 40,000 is skipped over.

If the property is not found, a number of different errors could be returned, but any of them will cause the default value to be used.

Quoted 2 lines

There's a good reason putting function calls inside if() is considered a bad coding style :)

I do not disagree with you, however, I was trying to maintain style consistency with the device property reads with the xhci_plat_probe function.

If I break that consistency, a couple of ways I might write this cleaner

1) set xhci->imod_interval to 40,000 before the call to device_property_read_u32. If the property exists in a firmware node, it will update the imod_interval value...if it does not exist, it will not update this value and the default will be used. In this case, I would not even check the return value. This method is used quite a bit in the kernel.

2) use a if (device_property_present()) and check to see if that property is even present. If so, call device_property_read_u32() without check return value. This has the downside of still using a function call within the if() which you have already indicated is not your preference.

3) add a retval and test off of this instead of using the function directly in the if()

Quoted 3 lines

thanks,
greg k-h

Thanks for taking time to review my patch. I really have no strong preference on how call device_property_read_u32() is tested and I am open to any suggestions. 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.

Greg Kroah-Hartman

maintainer

2017-11-29 08:09 UTC #3 lore raw

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

Quoted 28 lines · Adam Wallis

Quoted 4 lines · Greg Kroah-Hartman
Quoted 3 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
[..]
Quoted 14 lines
Quoted 14 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"
 
 static struct hc_driver __read_mostly xhci_plat_hc_driver;
 
@@ -269,6 +270,20 @@ 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 7 lines
drivers/usb/host/xhci-plat.c static int xhci_plat_probe(struct platform_device *pdev) · line 276
Quoted 4 lines
drivers/usb/host/xhci-plat.c +4 -0
+	/* imod interval in nanoseconds */
+	if (device_property_read_u32(sysdev,
+		"imod-interval", &xhci->imod_interval))
+		xhci->imod_interval = 40000;
So no matter what value you read, you set it to 40000? Or am I reading this code incorrectly?

Quoted 4 lines

I think you may be reading the code incorrectly. device_property_read_u32() returns 0 when the property is found and valid...and stored into xhci->imod_interval. When 0 is returned in this case, the default value of 40,000 is skipped over.

Yes, it is very hard to read :(

Quoted 5 lines

Quoted 2 lines
There's a good reason putting function calls inside if() is considered a bad coding style :)
I do not disagree with you, however, I was trying to maintain style consistency with the device property reads with the xhci_plat_probe function.

Ok, maybe it should all be fixed :)

Quoted 7 lines

If I break that consistency, a couple of ways I might write this cleaner
1) set xhci->imod_interval to 40,000 before the call to device_property_read_u32. If the property exists in a firmware node, it will update the imod_interval value...if it does not exist, it will not update this value and the default will be used. In this case, I would not even check the return value. This method is used quite a bit in the kernel.

Sounds like a reasonable way to do it.

thanks,

greg k-h

Rob Herring

maintainer

2017-12-01 02:01 UTC #4 lore raw

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

Quoted 46 lines · Greg Kroah-Hartman

Quoted 28 lines · Adam Wallis
Quoted 4 lines · Greg Kroah-Hartman
Quoted 3 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
[..]
Quoted 14 lines
Quoted 14 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"
 
 static struct hc_driver __read_mostly xhci_plat_hc_driver;
 
@@ -269,6 +270,20 @@ 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 7 lines
drivers/usb/host/xhci-plat.c static int xhci_plat_probe(struct platform_device *pdev) · line 276
Quoted 4 lines
drivers/usb/host/xhci-plat.c +4 -0
+	/* imod interval in nanoseconds */
+	if (device_property_read_u32(sysdev,
+		"imod-interval", &xhci->imod_interval))
+		xhci->imod_interval = 40000;
So no matter what value you read, you set it to 40000? Or am I reading this code incorrectly?
Quoted 4 lines
I think you may be reading the code incorrectly. device_property_read_u32() returns 0 when the property is found and valid...and stored into xhci->imod_interval. When 0 is returned in this case, the default value of 40,000 is skipped over.
Yes, it is very hard to read :(
Quoted 5 lines
Quoted 2 lines
There's a good reason putting function calls inside if() is considered a bad coding style :)
I do not disagree with you, however, I was trying to maintain style consistency with the device property reads with the xhci_plat_probe function.
Ok, maybe it should all be fixed :)
Quoted 2 lines
If I break that consistency, a couple of ways I might write this cleaner

Quoted 7 lines

Quoted 5 lines
1) set xhci->imod_interval to 40,000 before the call to device_property_read_u32. If the property exists in a firmware node, it will update the imod_interval value...if it does not exist, it will not update this value and the default will be used. In this case, I would not even check the return value. This method is used quite a bit in the kernel.
Sounds like a reasonable way to do it.

And is exactly how the (of|device)_property_read_* functions were intended to be used.

Rob

Rob Herring

maintainer

2017-12-01 02:02 UTC #5 lore raw

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

In reply to Adam Wallis

Quoted 37 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]>
---
 Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
 drivers/usb/host/xhci-plat.c                       | 15 +++++++++++++++
 drivers/usb/host/xhci.c                            |  7 ++-----
 drivers/usb/host/xhci.h                            |  2 ++
 4 files changed, 20 insertions(+), 5 deletions(-)
Documentation/devicetree/bindings/usb/usb-xhci.txt +0 -0
index ae6e484..3998459 100644

Documentation/devicetree/bindings/usb/usb-xhci.txt Optional properties: · line 32

Quoted 4 lines

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

@@ -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

Needs a unit suffix as defined in property-units.txt.

Rob