* [PATCH v1] usb: xhci: allow imod-interval to be configurable
@ 2017-11-28 17:11 ` Adam Wallis
0 siblings, 0 replies; 12+ messages in thread
From: Adam Wallis @ 2017-11-28 17:11 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Greg Kroah-Hartman, Rob Herring, Mathias Nyman,
linux-usb-u79uwXL29TY76Z2rM5mHXA, Mark Rutland, Matthias Brugger,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
chunfeng.yun-NuS5LvNUpcJWk0Htik3J/w
Cc: timur-sgV2jX0FEOL9JmXXK+q4OQ
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 <awallis-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
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(-)
diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
index ae6e484..3998459 100644
--- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
@@ -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@f0931000 {
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 09f164f..f7730c8 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -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);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 2424d30..6a09311 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -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 */
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 99a014a..2a4177b 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -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;
--
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1] usb: xhci: allow imod-interval to be configurable
@ 2017-11-28 17:11 ` Adam Wallis
0 siblings, 0 replies; 12+ messages in thread
From: Adam Wallis @ 2017-11-28 17:11 UTC (permalink / raw)
To: linux-arm-kernel
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 <awallis@codeaurora.org>
---
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(-)
diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
index ae6e484..3998459 100644
--- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
@@ -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 {
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 09f164f..f7730c8 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -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);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 2424d30..6a09311 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -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 */
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 99a014a..2a4177b 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -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;
--
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.
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1] usb: xhci: allow imod-interval to be configurable
2017-11-28 17:11 ` Adam Wallis
@ 2017-11-28 19:35 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2017-11-28 19:35 UTC (permalink / raw)
To: Adam Wallis
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring,
Mathias Nyman, linux-usb-u79uwXL29TY76Z2rM5mHXA, Mark Rutland,
Matthias Brugger, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
chunfeng.yun-NuS5LvNUpcJWk0Htik3J/w,
timur-sgV2jX0FEOL9JmXXK+q4OQ
On Tue, Nov 28, 2017 at 12:11:46PM -0500, Adam Wallis wrote:
> 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 <awallis-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
> 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(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> index ae6e484..3998459 100644
> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> @@ -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@f0931000 {
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 09f164f..f7730c8 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -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;
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1] usb: xhci: allow imod-interval to be configurable
@ 2017-11-28 19:35 ` Greg Kroah-Hartman
0 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2017-11-28 19:35 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Nov 28, 2017 at 12:11:46PM -0500, Adam Wallis wrote:
> 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 <awallis@codeaurora.org>
> ---
> 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(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> index ae6e484..3998459 100644
> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> @@ -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 {
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 09f164f..f7730c8 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -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;
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1] usb: xhci: allow imod-interval to be configurable
2017-11-28 19:35 ` Greg Kroah-Hartman
@ 2017-11-28 20:32 ` Adam Wallis
-1 siblings, 0 replies; 12+ messages in thread
From: Adam Wallis @ 2017-11-28 20:32 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Mathias Nyman,
timur-sgV2jX0FEOL9JmXXK+q4OQ, linux-usb-u79uwXL29TY76Z2rM5mHXA,
chunfeng.yun-NuS5LvNUpcJWk0Htik3J/w, Rob Herring,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Matthias Brugger,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 11/28/2017 2:35 PM, Greg Kroah-Hartman wrote:
> On Tue, Nov 28, 2017 at 12:11:46PM -0500, Adam Wallis wrote:
>> The xHCI driver currently has the IMOD set to 160, which
>> translates to an IMOD interval of 40,000ns (160 * 250)ns
>>
[..]
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -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;
>
> 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.
> 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()
> 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
--
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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1] usb: xhci: allow imod-interval to be configurable
@ 2017-11-28 20:32 ` Adam Wallis
0 siblings, 0 replies; 12+ messages in thread
From: Adam Wallis @ 2017-11-28 20:32 UTC (permalink / raw)
To: linux-arm-kernel
On 11/28/2017 2:35 PM, Greg Kroah-Hartman wrote:
> On Tue, Nov 28, 2017 at 12:11:46PM -0500, Adam Wallis wrote:
>> The xHCI driver currently has the IMOD set to 160, which
>> translates to an IMOD interval of 40,000ns (160 * 250)ns
>>
[..]
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -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;
>
> 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.
> 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()
> 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
--
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1] usb: xhci: allow imod-interval to be configurable
2017-11-28 20:32 ` Adam Wallis
@ 2017-11-29 8:09 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2017-11-29 8:09 UTC (permalink / raw)
To: Adam Wallis
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Mathias Nyman,
timur-sgV2jX0FEOL9JmXXK+q4OQ, linux-usb-u79uwXL29TY76Z2rM5mHXA,
chunfeng.yun-NuS5LvNUpcJWk0Htik3J/w, Rob Herring,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Matthias Brugger,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Tue, Nov 28, 2017 at 03:32:29PM -0500, Adam Wallis wrote:
> On 11/28/2017 2:35 PM, Greg Kroah-Hartman wrote:
> > On Tue, Nov 28, 2017 at 12:11:46PM -0500, Adam Wallis wrote:
> >> The xHCI driver currently has the IMOD set to 160, which
> >> translates to an IMOD interval of 40,000ns (160 * 250)ns
> >>
> [..]
> >> --- a/drivers/usb/host/xhci-plat.c
> >> +++ b/drivers/usb/host/xhci-plat.c
> >> @@ -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;
> >
> > 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.
Yes, it is very hard to read :(
> > 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 :)
> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1] usb: xhci: allow imod-interval to be configurable
@ 2017-11-29 8:09 ` Greg Kroah-Hartman
0 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2017-11-29 8:09 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Nov 28, 2017 at 03:32:29PM -0500, Adam Wallis wrote:
> On 11/28/2017 2:35 PM, Greg Kroah-Hartman wrote:
> > On Tue, Nov 28, 2017 at 12:11:46PM -0500, Adam Wallis wrote:
> >> The xHCI driver currently has the IMOD set to 160, which
> >> translates to an IMOD interval of 40,000ns (160 * 250)ns
> >>
> [..]
> >> --- a/drivers/usb/host/xhci-plat.c
> >> +++ b/drivers/usb/host/xhci-plat.c
> >> @@ -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;
> >
> > 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.
Yes, it is very hard to read :(
> > 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 :)
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1] usb: xhci: allow imod-interval to be configurable
2017-11-29 8:09 ` Greg Kroah-Hartman
@ 2017-12-01 2:01 ` Rob Herring
-1 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2017-12-01 2:01 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Adam Wallis, Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA,
Mathias Nyman, timur-sgV2jX0FEOL9JmXXK+q4OQ,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
chunfeng.yun-NuS5LvNUpcJWk0Htik3J/w,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Matthias Brugger,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Wed, Nov 29, 2017 at 09:09:41AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 28, 2017 at 03:32:29PM -0500, Adam Wallis wrote:
> > On 11/28/2017 2:35 PM, Greg Kroah-Hartman wrote:
> > > On Tue, Nov 28, 2017 at 12:11:46PM -0500, Adam Wallis wrote:
> > >> The xHCI driver currently has the IMOD set to 160, which
> > >> translates to an IMOD interval of 40,000ns (160 * 250)ns
> > >>
> > [..]
> > >> --- a/drivers/usb/host/xhci-plat.c
> > >> +++ b/drivers/usb/host/xhci-plat.c
> > >> @@ -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;
> > >
> > > 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.
>
> Yes, it is very hard to read :(
>
> > > 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 :)
>
> > 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.
And is exactly how the (of|device)_property_read_* functions were
intended to be used.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1] usb: xhci: allow imod-interval to be configurable
@ 2017-12-01 2:01 ` Rob Herring
0 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2017-12-01 2:01 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 29, 2017 at 09:09:41AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 28, 2017 at 03:32:29PM -0500, Adam Wallis wrote:
> > On 11/28/2017 2:35 PM, Greg Kroah-Hartman wrote:
> > > On Tue, Nov 28, 2017 at 12:11:46PM -0500, Adam Wallis wrote:
> > >> The xHCI driver currently has the IMOD set to 160, which
> > >> translates to an IMOD interval of 40,000ns (160 * 250)ns
> > >>
> > [..]
> > >> --- a/drivers/usb/host/xhci-plat.c
> > >> +++ b/drivers/usb/host/xhci-plat.c
> > >> @@ -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;
> > >
> > > 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.
>
> Yes, it is very hard to read :(
>
> > > 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 :)
>
> > 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.
And is exactly how the (of|device)_property_read_* functions were
intended to be used.
Rob
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1] usb: xhci: allow imod-interval to be configurable
2017-11-28 17:11 ` Adam Wallis
@ 2017-12-01 2:02 ` Rob Herring
-1 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2017-12-01 2:02 UTC (permalink / raw)
To: Adam Wallis
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Greg Kroah-Hartman, Mathias Nyman,
linux-usb-u79uwXL29TY76Z2rM5mHXA, Mark Rutland, Matthias Brugger,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
chunfeng.yun-NuS5LvNUpcJWk0Htik3J/w,
timur-sgV2jX0FEOL9JmXXK+q4OQ
On Tue, Nov 28, 2017 at 12:11:46PM -0500, Adam Wallis wrote:
> 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 <awallis-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
> 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(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> index ae6e484..3998459 100644
> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1] usb: xhci: allow imod-interval to be configurable
@ 2017-12-01 2:02 ` Rob Herring
0 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2017-12-01 2:02 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Nov 28, 2017 at 12:11:46PM -0500, Adam Wallis wrote:
> 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 <awallis@codeaurora.org>
> ---
> 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(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> index ae6e484..3998459 100644
> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> @@ -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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-12-01 2:02 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-28 17:11 [PATCH v1] usb: xhci: allow imod-interval to be configurable Adam Wallis
2017-11-28 17:11 ` Adam Wallis
[not found] ` <1511889106-9239-1-git-send-email-awallis-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2017-11-28 19:35 ` Greg Kroah-Hartman
2017-11-28 19:35 ` Greg Kroah-Hartman
[not found] ` <20171128193500.GA30609-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2017-11-28 20:32 ` Adam Wallis
2017-11-28 20:32 ` Adam Wallis
[not found] ` <22b57b69-3728-d879-42c6-92e87e7c7955-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2017-11-29 8:09 ` Greg Kroah-Hartman
2017-11-29 8:09 ` Greg Kroah-Hartman
[not found] ` <20171129080941.GA2319-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2017-12-01 2:01 ` Rob Herring
2017-12-01 2:01 ` Rob Herring
2017-12-01 2:02 ` Rob Herring
2017-12-01 2:02 ` Rob Herring
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.