[PATCH 0/2] usb: xhci: addition of timing quirk

[PATCH 0/2] usb: xhci: addition of timing quirk

[Adam Wallis]

On systems that allow CPUs to run at an extremely reduced frequency,
it is possible to create an "interrupt" storm by USB settings that are not
ideal at these speeds. This patch series introduces a quirk bit that currently
only touches the Interrupt Control register but might be used in the future
with other similar registers. This change allows for firmware (or scripts)
to set these registers as required in place of using hard-coded values
which might not be ideal for all platforms.

Adam Wallis (2):
  usb: xhci: add relaxed timing quirk bit
  usb: host: xhci-plat: check relaxed timing quirk bit

 Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
 drivers/usb/host/xhci-plat.c                       |  3 +++
 drivers/usb/host/xhci.c                            | 25 +++++++++++++++-------
 drivers/usb/host/xhci.h                            |  1 +
 4 files changed, 22 insertions(+), 8 deletions(-)

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

[PATCH 0/2] usb: xhci: addition of timing quirk

[Adam Wallis]

On systems that allow CPUs to run at an extremely reduced frequency,
it is possible to create an "interrupt" storm by USB settings that are not
ideal at these speeds. This patch series introduces a quirk bit that currently
only touches the Interrupt Control register but might be used in the future
with other similar registers. This change allows for firmware (or scripts)
to set these registers as required in place of using hard-coded values
which might not be ideal for all platforms.

Adam Wallis (2):
  usb: xhci: add relaxed timing quirk bit
  usb: host: xhci-plat: check relaxed timing quirk bit

 Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
 drivers/usb/host/xhci-plat.c                       |  3 +++
 drivers/usb/host/xhci.c                            | 25 +++++++++++++++-------
 drivers/usb/host/xhci.h                            |  1 +
 4 files changed, 22 insertions(+), 8 deletions(-)

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

[PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Adam Wallis]

Certain systems may run with CPUs at a very slow frequency. This
patch adds a quirk bit that can be used to relax certain timings, etc.

This quirk might be needed for other fields in the future, but
initially, it will be used only on the IRQ control register to allow
firmare to control the value of the register. This can prevent an
"interrupt storm" effect on certain systems.

Signed-off-by: Adam Wallis <awallis-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
 drivers/usb/host/xhci.c                            | 25 +++++++++++++++-------
 drivers/usb/host/xhci.h                            |  1 +
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
index ae6e484..af2faa24 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
+  - quirk-relaxed-timing: allows firmware to relax timing on certain registers
 
 Example:
 	usb@f0931000 {
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 327ba8b..e14a204 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -582,16 +582,25 @@ int xhci_run(struct usb_hcd *hcd)
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
 			"ERST deq = 64'h%0lx", (long unsigned int) temp_64);
 
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-			"// 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
+	 * Systems with slow CPUs may not be able to tolerate
+	 * agressive interrupt timing that silicon can tolerate. The
+	 * XHCI_RELAXED_TIMING will allow firmware to set the IRQ
+	 * Control field.
 	 */
-	temp |= (u32) ((xhci->quirks & XHCI_MTK_HOST) ? 20 : 160);
-	writel(temp, &xhci->ir_set->irq_control);
+	if (!(xhci->quirks & XHCI_RELAXED_TIMING_QUIRK)) {
+		xhci_dbg_trace(xhci, trace_xhci_dbg_init,
+				"// 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);
+		writel(temp, &xhci->ir_set->irq_control);
+	}
 
 	/* Set the HCD state before we enable the irqs */
 	temp = readl(&xhci->op_regs->command);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 99a014a..6d451be 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1823,6 +1823,7 @@ struct xhci_hcd {
 /* Reserved. It was XHCI_U2_DISABLE_WAKE */
 #define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	(1 << 28)
 #define XHCI_HW_LPM_DISABLE	(1 << 29)
+#define XHCI_RELAXED_TIMING_QUIRK	(1 << 30)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
-- 
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

[PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Adam Wallis]

Certain systems may run with CPUs at a very slow frequency. This
patch adds a quirk bit that can be used to relax certain timings, etc.

This quirk might be needed for other fields in the future, but
initially, it will be used only on the IRQ control register to allow
firmare to control the value of the register. This can prevent an
"interrupt storm" effect on certain systems.

Signed-off-by: Adam Wallis <awallis@codeaurora.org>
---
 Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
 drivers/usb/host/xhci.c                            | 25 +++++++++++++++-------
 drivers/usb/host/xhci.h                            |  1 +
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
index ae6e484..af2faa24 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
+  - quirk-relaxed-timing: allows firmware to relax timing on certain registers
 
 Example:
 	usb at f0931000 {
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 327ba8b..e14a204 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -582,16 +582,25 @@ int xhci_run(struct usb_hcd *hcd)
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
 			"ERST deq = 64'h%0lx", (long unsigned int) temp_64);
 
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-			"// 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
+	 * Systems with slow CPUs may not be able to tolerate
+	 * agressive interrupt timing that silicon can tolerate. The
+	 * XHCI_RELAXED_TIMING will allow firmware to set the IRQ
+	 * Control field.
 	 */
-	temp |= (u32) ((xhci->quirks & XHCI_MTK_HOST) ? 20 : 160);
-	writel(temp, &xhci->ir_set->irq_control);
+	if (!(xhci->quirks & XHCI_RELAXED_TIMING_QUIRK)) {
+		xhci_dbg_trace(xhci, trace_xhci_dbg_init,
+				"// 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);
+		writel(temp, &xhci->ir_set->irq_control);
+	}
 
 	/* Set the HCD state before we enable the irqs */
 	temp = readl(&xhci->op_regs->command);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 99a014a..6d451be 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1823,6 +1823,7 @@ struct xhci_hcd {
 /* Reserved. It was XHCI_U2_DISABLE_WAKE */
 #define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	(1 << 28)
 #define XHCI_HW_LPM_DISABLE	(1 << 29)
+#define XHCI_RELAXED_TIMING_QUIRK	(1 << 30)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
-- 
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.

[PATCH 2/2] usb: host: xhci-plat: check relaxed timing quirk bit

[Adam Wallis]

Check sysdev to see if the relaxed timing quirk ("quirk-relaxed-timing")
is set.

Signed-off-by: Adam Wallis <awallis-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/usb/host/xhci-plat.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 09f164f..ea221f3 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -266,6 +266,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
 		xhci->quirks |= XHCI_LPM_SUPPORT;
 
+	if (device_property_read_bool(sysdev, "quirk-relaxed-timing"))
+		xhci->quirks |= XHCI_RELAXED_TIMING_QUIRK;
+
 	if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
 		xhci->quirks |= XHCI_BROKEN_PORT_PED;
 
-- 
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

[PATCH 2/2] usb: host: xhci-plat: check relaxed timing quirk bit

[Adam Wallis]

Check sysdev to see if the relaxed timing quirk ("quirk-relaxed-timing")
is set.

Signed-off-by: Adam Wallis <awallis@codeaurora.org>
---
 drivers/usb/host/xhci-plat.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 09f164f..ea221f3 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -266,6 +266,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
 		xhci->quirks |= XHCI_LPM_SUPPORT;
 
+	if (device_property_read_bool(sysdev, "quirk-relaxed-timing"))
+		xhci->quirks |= XHCI_RELAXED_TIMING_QUIRK;
+
 	if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
 		xhci->quirks |= XHCI_BROKEN_PORT_PED;
 
-- 
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.

Re: [PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Rob Herring]

On Tue, Nov 21, 2017 at 12:18:09PM -0500, Adam Wallis wrote:
> Certain systems may run with CPUs at a very slow frequency. This
> patch adds a quirk bit that can be used to relax certain timings, etc.
> 
> This quirk might be needed for other fields in the future, but
> initially, it will be used only on the IRQ control register to allow
> firmare to control the value of the register. This can prevent an

s/firmare/firmware/

By firmware control, you mean the register is initialized on boot and 
then not touched by the kernel? What if the XHCI block is reset? Not 
sure if that's possible.

> "interrupt storm" effect on certain systems.

So now we have 2 ways to deal with this? The existing MediaTek quirk and 
now this one. 

I think you should change the existing quirk to a value and set the 
value based on compatible strings.

> Signed-off-by: Adam Wallis <awallis-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
>  drivers/usb/host/xhci.c                            | 25 +++++++++++++++-------
>  drivers/usb/host/xhci.h                            |  1 +
>  3 files changed, 19 insertions(+), 8 deletions(-)
--
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

[PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Rob Herring]

On Tue, Nov 21, 2017 at 12:18:09PM -0500, Adam Wallis wrote:
> Certain systems may run with CPUs at a very slow frequency. This
> patch adds a quirk bit that can be used to relax certain timings, etc.
> 
> This quirk might be needed for other fields in the future, but
> initially, it will be used only on the IRQ control register to allow
> firmare to control the value of the register. This can prevent an

s/firmare/firmware/

By firmware control, you mean the register is initialized on boot and 
then not touched by the kernel? What if the XHCI block is reset? Not 
sure if that's possible.

> "interrupt storm" effect on certain systems.

So now we have 2 ways to deal with this? The existing MediaTek quirk and 
now this one. 

I think you should change the existing quirk to a value and set the 
value based on compatible strings.

> Signed-off-by: Adam Wallis <awallis@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
>  drivers/usb/host/xhci.c                            | 25 +++++++++++++++-------
>  drivers/usb/host/xhci.h                            |  1 +
>  3 files changed, 19 insertions(+), 8 deletions(-)

Re: [PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Adam Wallis]

On 11/21/2017 2:11 PM, Rob Herring wrote:
> On Tue, Nov 21, 2017 at 12:18:09PM -0500, Adam Wallis wrote:
>> Certain systems may run with CPUs at a very slow frequency. This
>> patch adds a quirk bit that can be used to relax certain timings, etc.
>>
>> This quirk might be needed for other fields in the future, but
>> initially, it will be used only on the IRQ control register to allow
>> firmare to control the value of the register. This can prevent an
> 
> s/firmare/firmware/
> 

Thanks, good catch.

> By firmware control, you mean the register is initialized on boot and 
> then not touched by the kernel? What if the XHCI block is reset? Not 
> sure if that's possible.
> 
>> "interrupt storm" effect on certain systems.
> 
> So now we have 2 ways to deal with this? The existing MediaTek quirk and 
> now this one. 

I do agree that 2 different ways to deal with it is not ideal. I also think that
having one hard-coded value (and one alternate for MTK) is also not ideal.

> 
> I think you should change the existing quirk to a value and set the 
> value based on compatible strings.

I like where you are going with this. Are you saying that I could read for a
device property read from firmware (for DTB or ACPI) like DWC3 does for
"snps,hird-threshold"? If you mean this, where do you recommend I store the
desired IRQ_CONTROL value - in struct xhci_hcd ?

Or by "compatible" strings, did you mean storing hard-coded values in the
of_device_id usb_xhci_of_match[] array? This would still be hard-coding (which I
would like to avoid) and also would not work for the ACPI case.

> 
>> Signed-off-by: Adam Wallis <awallis-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
>>  drivers/usb/host/xhci.c                            | 25 +++++++++++++++-------
>>  drivers/usb/host/xhci.h                            |  1 +
>>  3 files changed, 19 insertions(+), 8 deletions(-)

Thanks for the feedback Rob.

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

[PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Adam Wallis]

On 11/21/2017 2:11 PM, Rob Herring wrote:
> On Tue, Nov 21, 2017 at 12:18:09PM -0500, Adam Wallis wrote:
>> Certain systems may run with CPUs at a very slow frequency. This
>> patch adds a quirk bit that can be used to relax certain timings, etc.
>>
>> This quirk might be needed for other fields in the future, but
>> initially, it will be used only on the IRQ control register to allow
>> firmare to control the value of the register. This can prevent an
> 
> s/firmare/firmware/
> 

Thanks, good catch.

> By firmware control, you mean the register is initialized on boot and 
> then not touched by the kernel? What if the XHCI block is reset? Not 
> sure if that's possible.
> 
>> "interrupt storm" effect on certain systems.
> 
> So now we have 2 ways to deal with this? The existing MediaTek quirk and 
> now this one. 

I do agree that 2 different ways to deal with it is not ideal. I also think that
having one hard-coded value (and one alternate for MTK) is also not ideal.

> 
> I think you should change the existing quirk to a value and set the 
> value based on compatible strings.

I like where you are going with this. Are you saying that I could read for a
device property read from firmware (for DTB or ACPI) like DWC3 does for
"snps,hird-threshold"? If you mean this, where do you recommend I store the
desired IRQ_CONTROL value - in struct xhci_hcd ?

Or by "compatible" strings, did you mean storing hard-coded values in the
of_device_id usb_xhci_of_match[] array? This would still be hard-coding (which I
would like to avoid) and also would not work for the ACPI case.

> 
>> Signed-off-by: Adam Wallis <awallis@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
>>  drivers/usb/host/xhci.c                            | 25 +++++++++++++++-------
>>  drivers/usb/host/xhci.h                            |  1 +
>>  3 files changed, 19 insertions(+), 8 deletions(-)

Thanks for the feedback Rob.

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.

Re: [PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Rob Herring]

On Tue, Nov 21, 2017 at 1:49 PM, Adam Wallis <awallis-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> On 11/21/2017 2:11 PM, Rob Herring wrote:
>> On Tue, Nov 21, 2017 at 12:18:09PM -0500, Adam Wallis wrote:
>>> Certain systems may run with CPUs at a very slow frequency. This
>>> patch adds a quirk bit that can be used to relax certain timings, etc.
>>>
>>> This quirk might be needed for other fields in the future, but
>>> initially, it will be used only on the IRQ control register to allow
>>> firmare to control the value of the register. This can prevent an
>>
>> s/firmare/firmware/
>>
>
> Thanks, good catch.
>
>> By firmware control, you mean the register is initialized on boot and
>> then not touched by the kernel? What if the XHCI block is reset? Not
>> sure if that's possible.
>>
>>> "interrupt storm" effect on certain systems.
>>
>> So now we have 2 ways to deal with this? The existing MediaTek quirk and
>> now this one.
>
> I do agree that 2 different ways to deal with it is not ideal. I also think that
> having one hard-coded value (and one alternate for MTK) is also not ideal.

Agreed.

>>
>> I think you should change the existing quirk to a value and set the
>> value based on compatible strings.
>
> I like where you are going with this. Are you saying that I could read for a
> device property read from firmware (for DTB or ACPI) like DWC3 does for
> "snps,hird-threshold"?

Is that for the same thing? If so, drop the vendor prefix and use
that. Otherwise, a separate property should really be something that
is per board rather than per SoC.

> If you mean this, where do you recommend I store the
> desired IRQ_CONTROL value - in struct xhci_hcd ?

No idea.

> Or by "compatible" strings, did you mean storing hard-coded values in the
> of_device_id usb_xhci_of_match[] array? This would still be hard-coding (which I
> would like to avoid) and also would not work for the ACPI case.

ACPI has match tables too?

It would only be hardcoded per compatible which should be per SoC. Do
you need per board/device tuning? If so, use a property.

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

[PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Rob Herring]

On Tue, Nov 21, 2017 at 1:49 PM, Adam Wallis <awallis@codeaurora.org> wrote:
> On 11/21/2017 2:11 PM, Rob Herring wrote:
>> On Tue, Nov 21, 2017 at 12:18:09PM -0500, Adam Wallis wrote:
>>> Certain systems may run with CPUs at a very slow frequency. This
>>> patch adds a quirk bit that can be used to relax certain timings, etc.
>>>
>>> This quirk might be needed for other fields in the future, but
>>> initially, it will be used only on the IRQ control register to allow
>>> firmare to control the value of the register. This can prevent an
>>
>> s/firmare/firmware/
>>
>
> Thanks, good catch.
>
>> By firmware control, you mean the register is initialized on boot and
>> then not touched by the kernel? What if the XHCI block is reset? Not
>> sure if that's possible.
>>
>>> "interrupt storm" effect on certain systems.
>>
>> So now we have 2 ways to deal with this? The existing MediaTek quirk and
>> now this one.
>
> I do agree that 2 different ways to deal with it is not ideal. I also think that
> having one hard-coded value (and one alternate for MTK) is also not ideal.

Agreed.

>>
>> I think you should change the existing quirk to a value and set the
>> value based on compatible strings.
>
> I like where you are going with this. Are you saying that I could read for a
> device property read from firmware (for DTB or ACPI) like DWC3 does for
> "snps,hird-threshold"?

Is that for the same thing? If so, drop the vendor prefix and use
that. Otherwise, a separate property should really be something that
is per board rather than per SoC.

> If you mean this, where do you recommend I store the
> desired IRQ_CONTROL value - in struct xhci_hcd ?

No idea.

> Or by "compatible" strings, did you mean storing hard-coded values in the
> of_device_id usb_xhci_of_match[] array? This would still be hard-coding (which I
> would like to avoid) and also would not work for the ACPI case.

ACPI has match tables too?

It would only be hardcoded per compatible which should be per SoC. Do
you need per board/device tuning? If so, use a property.

Rob

Re: [PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Adam Wallis]

On 11/21/2017 3:06 PM, Rob Herring wrote:

[..]

>> I like where you are going with this. Are you saying that I could read for a
>> device property read from firmware (for DTB or ACPI) like DWC3 does for
>> "snps,hird-threshold"?
> 
> Is that for the same thing? If so, drop the vendor prefix and use
> that. Otherwise, a separate property should really be something that
> is per board rather than per SoC.

I don't think that's exactly the same property, but it's the same idea I would
prefer to go with. That way, an integer can be passed in via the firmware tables.

> 
>> If you mean this, where do you recommend I store the
>> desired IRQ_CONTROL value - in struct xhci_hcd ?
> 
> No idea.
> 
>> Or by "compatible" strings, did you mean storing hard-coded values in the
>> of_device_id usb_xhci_of_match[] array? This would still be hard-coding (which I
>> would like to avoid) and also would not work for the ACPI case.
> 
> ACPI has match tables too?
> 

Yes, you can use DSD in a way that is similar to OF properties

> It would only be hardcoded per compatible which should be per SoC. Do
> you need per board/device tuning? If so, use a property.
> 

The reason why I think it should dynamic via firmware tables is that

* It's much less invasive for vendors to update their DT tables if they need to
adjust on a per device/controller/family/etc basis then to adjust a properties
table in xhci-plat
* This would lead to less polluting in xhci-plat code

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

I will provide an updated proposed patch sometime this week. I also hope to get
some feedback from Mathias to see what he prefers.

Thanks again for the feedback Rob.


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

[PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Adam Wallis]

On 11/21/2017 3:06 PM, Rob Herring wrote:

[..]

>> I like where you are going with this. Are you saying that I could read for a
>> device property read from firmware (for DTB or ACPI) like DWC3 does for
>> "snps,hird-threshold"?
> 
> Is that for the same thing? If so, drop the vendor prefix and use
> that. Otherwise, a separate property should really be something that
> is per board rather than per SoC.

I don't think that's exactly the same property, but it's the same idea I would
prefer to go with. That way, an integer can be passed in via the firmware tables.

> 
>> If you mean this, where do you recommend I store the
>> desired IRQ_CONTROL value - in struct xhci_hcd ?
> 
> No idea.
> 
>> Or by "compatible" strings, did you mean storing hard-coded values in the
>> of_device_id usb_xhci_of_match[] array? This would still be hard-coding (which I
>> would like to avoid) and also would not work for the ACPI case.
> 
> ACPI has match tables too?
> 

Yes, you can use DSD in a way that is similar to OF properties

> It would only be hardcoded per compatible which should be per SoC. Do
> you need per board/device tuning? If so, use a property.
> 

The reason why I think it should dynamic via firmware tables is that

* It's much less invasive for vendors to update their DT tables if they need to
adjust on a per device/controller/family/etc basis then to adjust a properties
table in xhci-plat
* This would lead to less polluting in xhci-plat code

> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

I will provide an updated proposed patch sometime this week. I also hope to get
some feedback from Mathias to see what he prefers.

Thanks again for the feedback Rob.


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

Re: [PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Mathias Nyman]

On 22.11.2017 02:07, Adam Wallis wrote:
> On 11/21/2017 3:06 PM, Rob Herring wrote:
> 
> [..]
> 
>>> I like where you are going with this. Are you saying that I could read for a
>>> device property read from firmware (for DTB or ACPI) like DWC3 does for
>>> "snps,hird-threshold"?
>>
>> Is that for the same thing? If so, drop the vendor prefix and use
>> that. Otherwise, a separate property should really be something that
>> is per board rather than per SoC.
> 
> I don't think that's exactly the same property, but it's the same idea I would
> prefer to go with. That way, an integer can be passed in via the firmware tables.
> 
>>
>>> If you mean this, where do you recommend I store the
>>> desired IRQ_CONTROL value - in struct xhci_hcd ?
>>
>> No idea.
>>
>>> Or by "compatible" strings, did you mean storing hard-coded values in the
>>> of_device_id usb_xhci_of_match[] array? This would still be hard-coding (which I
>>> would like to avoid) and also would not work for the ACPI case.
>>
>> ACPI has match tables too?
>>
> 
> Yes, you can use DSD in a way that is similar to OF properties
> 
>> It would only be hardcoded per compatible which should be per SoC. Do
>> you need per board/device tuning? If so, use a property.
>>
> 
> The reason why I think it should dynamic via firmware tables is that
> 
> * It's much less invasive for vendors to update their DT tables if they need to
> adjust on a per device/controller/family/etc basis then to adjust a properties
> table in xhci-plat
> * This would lead to less polluting in xhci-plat code
> 
>> Rob
>> --
>> 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
>>
> 
> I will provide an updated proposed patch sometime this week. I also hope to get
> some feedback from Mathias to see what he prefers.

We know have at least two hosts/platforms that need custom interrupt moderation values

How about adding a u32 device property for xhci with the interrupt moderation interval in
nanoseconds?  And also add a u32 imod_interval variable to struct xhci_hcd?
  
imod_interval can be set to the current default 40000ns (160*250ns) and overwritten if
device_property_read_u32() returns something else.

XHCI_MTK_HOST could then use whatever preferred device propery interval value,
and we can get rid of using XHCI_MTK_HOST quirk flag when setting up the IMODI
  
-Mathias


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

[PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Mathias Nyman]

On 22.11.2017 02:07, Adam Wallis wrote:
> On 11/21/2017 3:06 PM, Rob Herring wrote:
> 
> [..]
> 
>>> I like where you are going with this. Are you saying that I could read for a
>>> device property read from firmware (for DTB or ACPI) like DWC3 does for
>>> "snps,hird-threshold"?
>>
>> Is that for the same thing? If so, drop the vendor prefix and use
>> that. Otherwise, a separate property should really be something that
>> is per board rather than per SoC.
> 
> I don't think that's exactly the same property, but it's the same idea I would
> prefer to go with. That way, an integer can be passed in via the firmware tables.
> 
>>
>>> If you mean this, where do you recommend I store the
>>> desired IRQ_CONTROL value - in struct xhci_hcd ?
>>
>> No idea.
>>
>>> Or by "compatible" strings, did you mean storing hard-coded values in the
>>> of_device_id usb_xhci_of_match[] array? This would still be hard-coding (which I
>>> would like to avoid) and also would not work for the ACPI case.
>>
>> ACPI has match tables too?
>>
> 
> Yes, you can use DSD in a way that is similar to OF properties
> 
>> It would only be hardcoded per compatible which should be per SoC. Do
>> you need per board/device tuning? If so, use a property.
>>
> 
> The reason why I think it should dynamic via firmware tables is that
> 
> * It's much less invasive for vendors to update their DT tables if they need to
> adjust on a per device/controller/family/etc basis then to adjust a properties
> table in xhci-plat
> * This would lead to less polluting in xhci-plat code
> 
>> Rob
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> I will provide an updated proposed patch sometime this week. I also hope to get
> some feedback from Mathias to see what he prefers.

We know have at least two hosts/platforms that need custom interrupt moderation values

How about adding a u32 device property for xhci with the interrupt moderation interval in
nanoseconds?  And also add a u32 imod_interval variable to struct xhci_hcd?
  
imod_interval can be set to the current default 40000ns (160*250ns) and overwritten if
device_property_read_u32() returns something else.

XHCI_MTK_HOST could then use whatever preferred device propery interval value,
and we can get rid of using XHCI_MTK_HOST quirk flag when setting up the IMODI
  
-Mathias

Re: [PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Adam Wallis]

On 11/22/2017 10:24 AM, Mathias Nyman wrote:
> On 22.11.2017 02:07, Adam Wallis wrote:
>> On 11/21/2017 3:06 PM, Rob Herring wrote:
>>
>> [..]
>>
>>>> I like where you are going with this. Are you saying that I could read for a
>>>> device property read from firmware (for DTB or ACPI) like DWC3 does for
>>>> "snps,hird-threshold"?
>>>
>>> Is that for the same thing? If so, drop the vendor prefix and use
>>> that. Otherwise, a separate property should really be something that
>>> is per board rather than per SoC.
>>
>> I don't think that's exactly the same property, but it's the same idea I would
>> prefer to go with. That way, an integer can be passed in via the firmware tables.
>>
>>>
>>>> If you mean this, where do you recommend I store the
>>>> desired IRQ_CONTROL value - in struct xhci_hcd ?
>>>
>>> No idea.
>>>
>>>> Or by "compatible" strings, did you mean storing hard-coded values in the
>>>> of_device_id usb_xhci_of_match[] array? This would still be hard-coding
>>>> (which I
>>>> would like to avoid) and also would not work for the ACPI case.
>>>
>>> ACPI has match tables too?
>>>
>>
>> Yes, you can use DSD in a way that is similar to OF properties
>>
>>> It would only be hardcoded per compatible which should be per SoC. Do
>>> you need per board/device tuning? If so, use a property.
>>>
>>
>> The reason why I think it should dynamic via firmware tables is that
>>
>> * It's much less invasive for vendors to update their DT tables if they need to
>> adjust on a per device/controller/family/etc basis then to adjust a properties
>> table in xhci-plat
>> * This would lead to less polluting in xhci-plat code
>>
>>> Rob
>>> -- 
>>> 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
>>>
>>
>> I will provide an updated proposed patch sometime this week. I also hope to get
>> some feedback from Mathias to see what he prefers.
> 
> We know have at least two hosts/platforms that need custom interrupt moderation
> values
> 
> How about adding a u32 device property for xhci with the interrupt moderation
> interval in
> nanoseconds?  And also add a u32 imod_interval variable to struct xhci_hcd?
>  
> imod_interval can be set to the current default 40000ns (160*250ns) and
> overwritten if
> device_property_read_u32() returns something else.
> 
> XHCI_MTK_HOST could then use whatever preferred device propery interval value,
> and we can get rid of using XHCI_MTK_HOST quirk flag when setting up the IMODI
>  
> -Mathias
> 

This sounds excellent Mathias. Let me put together a patch along these lines.
Thanks for the feedback!

Adam

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


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

[PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Adam Wallis]

On 11/22/2017 10:24 AM, Mathias Nyman wrote:
> On 22.11.2017 02:07, Adam Wallis wrote:
>> On 11/21/2017 3:06 PM, Rob Herring wrote:
>>
>> [..]
>>
>>>> I like where you are going with this. Are you saying that I could read for a
>>>> device property read from firmware (for DTB or ACPI) like DWC3 does for
>>>> "snps,hird-threshold"?
>>>
>>> Is that for the same thing? If so, drop the vendor prefix and use
>>> that. Otherwise, a separate property should really be something that
>>> is per board rather than per SoC.
>>
>> I don't think that's exactly the same property, but it's the same idea I would
>> prefer to go with. That way, an integer can be passed in via the firmware tables.
>>
>>>
>>>> If you mean this, where do you recommend I store the
>>>> desired IRQ_CONTROL value - in struct xhci_hcd ?
>>>
>>> No idea.
>>>
>>>> Or by "compatible" strings, did you mean storing hard-coded values in the
>>>> of_device_id usb_xhci_of_match[] array? This would still be hard-coding
>>>> (which I
>>>> would like to avoid) and also would not work for the ACPI case.
>>>
>>> ACPI has match tables too?
>>>
>>
>> Yes, you can use DSD in a way that is similar to OF properties
>>
>>> It would only be hardcoded per compatible which should be per SoC. Do
>>> you need per board/device tuning? If so, use a property.
>>>
>>
>> The reason why I think it should dynamic via firmware tables is that
>>
>> * It's much less invasive for vendors to update their DT tables if they need to
>> adjust on a per device/controller/family/etc basis then to adjust a properties
>> table in xhci-plat
>> * This would lead to less polluting in xhci-plat code
>>
>>> Rob
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>>> the body of a message to majordomo at vger.kernel.org
>>> More majordomo info at? http://vger.kernel.org/majordomo-info.html
>>>
>>
>> I will provide an updated proposed patch sometime this week. I also hope to get
>> some feedback from Mathias to see what he prefers.
> 
> We know have at least two hosts/platforms that need custom interrupt moderation
> values
> 
> How about adding a u32 device property for xhci with the interrupt moderation
> interval in
> nanoseconds?? And also add a u32 imod_interval variable to struct xhci_hcd?
> ?
> imod_interval can be set to the current default 40000ns (160*250ns) and
> overwritten if
> device_property_read_u32() returns something else.
> 
> XHCI_MTK_HOST could then use whatever preferred device propery interval value,
> and we can get rid of using XHCI_MTK_HOST quirk flag when setting up the IMODI
> ?
> -Mathias
> 

This sounds excellent Mathias. Let me put together a patch along these lines.
Thanks for the feedback!

Adam

> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at? http://vger.kernel.org/majordomo-info.html


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

Re: [PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Adam Wallis]

On 11/22/2017 10:24 AM, Mathias Nyman wrote:
[..]
> 
> We know have at least two hosts/platforms that need custom interrupt moderation
> values
> 
> How about adding a u32 device property for xhci with the interrupt moderation
> interval in
> nanoseconds?  And also add a u32 imod_interval variable to struct xhci_hcd?
>  
> imod_interval can be set to the current default 40000ns (160*250ns) and
> overwritten if
> device_property_read_u32() returns something else.
> 

Isn't the 160 value quite aggressive anyway? Section 5.5.2.2 of the xHCI spec
says that maximum observable interrupt rate should never exceed 8000
interrupts/second. I believe the IMOD value in the most aggressive case would
then be 500 by this statement [ 1 / (250e-9 * 500) = 8000 irqs/second ]

Perhaps I am misreading the spec or just doing the math wrong? With the default
value of 160, we are interrupting 25,000 irq/second...which is over 3 times the
maximum stated value (again, assuming I did the math right)

Anyway, my preference would be to set the IMOD default val to 4000 (~1ms) per
the recommended value in Table 49 of the spec and allow platforms to adjust as
necessary from that point.

Thoughts on this?

> XHCI_MTK_HOST could then use whatever preferred device propery interval value,
> and we can get rid of using XHCI_MTK_HOST quirk flag when setting up the IMODI
>  
> -Mathias
> 
> 
> -- 
> 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

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

[PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Adam Wallis]

On 11/22/2017 10:24 AM, Mathias Nyman wrote:
[..]
> 
> We know have at least two hosts/platforms that need custom interrupt moderation
> values
> 
> How about adding a u32 device property for xhci with the interrupt moderation
> interval in
> nanoseconds?? And also add a u32 imod_interval variable to struct xhci_hcd?
> ?
> imod_interval can be set to the current default 40000ns (160*250ns) and
> overwritten if
> device_property_read_u32() returns something else.
> 

Isn't the 160 value quite aggressive anyway? Section 5.5.2.2 of the xHCI spec
says that maximum observable interrupt rate should never exceed 8000
interrupts/second. I believe the IMOD value in the most aggressive case would
then be 500 by this statement [ 1 / (250e-9 * 500) = 8000 irqs/second ]

Perhaps I am misreading the spec or just doing the math wrong? With the default
value of 160, we are interrupting 25,000 irq/second...which is over 3 times the
maximum stated value (again, assuming I did the math right)

Anyway, my preference would be to set the IMOD default val to 4000 (~1ms) per
the recommended value in Table 49 of the spec and allow platforms to adjust as
necessary from that point.

Thoughts on this?

> XHCI_MTK_HOST could then use whatever preferred device propery interval value,
> and we can get rid of using XHCI_MTK_HOST quirk flag when setting up the IMODI
> ?
> -Mathias
> 
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at? http://vger.kernel.org/majordomo-info.html

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.

Re: [PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Mathias Nyman]

On 23.11.2017 01:32, Adam Wallis wrote:
> On 11/22/2017 10:24 AM, Mathias Nyman wrote:
> [..]
>>
>> We know have at least two hosts/platforms that need custom interrupt moderation
>> values
>>
>> How about adding a u32 device property for xhci with the interrupt moderation
>> interval in
>> nanoseconds?  And also add a u32 imod_interval variable to struct xhci_hcd?
>>   
>> imod_interval can be set to the current default 40000ns (160*250ns) and
>> overwritten if
>> device_property_read_u32() returns something else.
>>
> 
> Isn't the 160 value quite aggressive anyway? Section 5.5.2.2 of the xHCI spec
> says that maximum observable interrupt rate should never exceed 8000
> interrupts/second. I believe the IMOD value in the most aggressive case would
> then be 500 by this statement [ 1 / (250e-9 * 500) = 8000 irqs/second ]
> 
> Perhaps I am misreading the spec or just doing the math wrong? With the default
> value of 160, we are interrupting 25,000 irq/second...which is over 3 times the
> maximum stated value (again, assuming I did the math right)
> 
> Anyway, my preference would be to set the IMOD default val to 4000 (~1ms) per
> the recommended value in Table 49 of the spec and allow platforms to adjust as
> necessary from that point.
> 
> Thoughts on this?
> 

I think current 40us is still reasonable, it's about ~3 times per
usb microframe (125us) .8000 interrupts per second just covers the microframe rate,
which is the shortest interval a interrupt transfer can require service.

1ms interrupt interval is too long. In the worst case ~8 microframes could pass
before the driver is aware of a error it needs to take care of.
USB 3.1 devices can transfer 6 burst of 16 max sized packets (6 x 16 x 2014) bytes
per microframe.

closer to 125us could probably work as well, but unless we are fixing some issue or
getting some other bigger benefit out of it I don't think it's worth changing it
just to see if it breaks stuff.

Thanks
-Mathias
--
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

[PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Mathias Nyman]

On 23.11.2017 01:32, Adam Wallis wrote:
> On 11/22/2017 10:24 AM, Mathias Nyman wrote:
> [..]
>>
>> We know have at least two hosts/platforms that need custom interrupt moderation
>> values
>>
>> How about adding a u32 device property for xhci with the interrupt moderation
>> interval in
>> nanoseconds?? And also add a u32 imod_interval variable to struct xhci_hcd?
>>   
>> imod_interval can be set to the current default 40000ns (160*250ns) and
>> overwritten if
>> device_property_read_u32() returns something else.
>>
> 
> Isn't the 160 value quite aggressive anyway? Section 5.5.2.2 of the xHCI spec
> says that maximum observable interrupt rate should never exceed 8000
> interrupts/second. I believe the IMOD value in the most aggressive case would
> then be 500 by this statement [ 1 / (250e-9 * 500) = 8000 irqs/second ]
> 
> Perhaps I am misreading the spec or just doing the math wrong? With the default
> value of 160, we are interrupting 25,000 irq/second...which is over 3 times the
> maximum stated value (again, assuming I did the math right)
> 
> Anyway, my preference would be to set the IMOD default val to 4000 (~1ms) per
> the recommended value in Table 49 of the spec and allow platforms to adjust as
> necessary from that point.
> 
> Thoughts on this?
> 

I think current 40us is still reasonable, it's about ~3 times per
usb microframe (125us) .8000 interrupts per second just covers the microframe rate,
which is the shortest interval a interrupt transfer can require service.

1ms interrupt interval is too long. In the worst case ~8 microframes could pass
before the driver is aware of a error it needs to take care of.
USB 3.1 devices can transfer 6 burst of 16 max sized packets (6 x 16 x 2014) bytes
per microframe.

closer to 125us could probably work as well, but unless we are fixing some issue or
getting some other bigger benefit out of it I don't think it's worth changing it
just to see if it breaks stuff.

Thanks
-Mathias

Re: [PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Adam Wallis]

On 11/23/2017 5:59 AM, Mathias Nyman wrote:
> On 23.11.2017 01:32, Adam Wallis wrote:
>> On 11/22/2017 10:24 AM, Mathias Nyman wrote:
>> [..]
>>>
>>> We know have at least two hosts/platforms that need custom interrupt moderation
>>> values
>>>
>>> How about adding a u32 device property for xhci with the interrupt moderation
>>> interval in
>>> nanoseconds?  And also add a u32 imod_interval variable to struct xhci_hcd?
>>>   imod_interval can be set to the current default 40000ns (160*250ns) and
>>> overwritten if
>>> device_property_read_u32() returns something else.
>>>
>>
>> Isn't the 160 value quite aggressive anyway? Section 5.5.2.2 of the xHCI spec
>> says that maximum observable interrupt rate should never exceed 8000
>> interrupts/second. I believe the IMOD value in the most aggressive case would
>> then be 500 by this statement [ 1 / (250e-9 * 500) = 8000 irqs/second ]
>>
>> Perhaps I am misreading the spec or just doing the math wrong? With the default
>> value of 160, we are interrupting 25,000 irq/second...which is over 3 times the
>> maximum stated value (again, assuming I did the math right)
>>
>> Anyway, my preference would be to set the IMOD default val to 4000 (~1ms) per
>> the recommended value in Table 49 of the spec and allow platforms to adjust as
>> necessary from that point.
>>
>> Thoughts on this?
>>
> 
> I think current 40us is still reasonable, it's about ~3 times per
> usb microframe (125us) .8000 interrupts per second just covers the microframe rate,
> which is the shortest interval a interrupt transfer can require service.
> 
> 1ms interrupt interval is too long. In the worst case ~8 microframes could pass
> before the driver is aware of a error it needs to take care of.
> USB 3.1 devices can transfer 6 burst of 16 max sized packets (6 x 16 x 2014) bytes
> per microframe.
> 
> closer to 125us could probably work as well, but unless we are fixing some issue or
> getting some other bigger benefit out of it I don't think it's worth changing it
> just to see if it breaks stuff.

I agree - my patch will just stick with the current default as the fallback
value if no device property is submitted.

> 
> Thanks
> -Mathias
> -- 
> 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


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

[PATCH 1/2] usb: xhci: add relaxed timing quirk bit

[Adam Wallis]

On 11/23/2017 5:59 AM, Mathias Nyman wrote:
> On 23.11.2017 01:32, Adam Wallis wrote:
>> On 11/22/2017 10:24 AM, Mathias Nyman wrote:
>> [..]
>>>
>>> We know have at least two hosts/platforms that need custom interrupt moderation
>>> values
>>>
>>> How about adding a u32 device property for xhci with the interrupt moderation
>>> interval in
>>> nanoseconds?? And also add a u32 imod_interval variable to struct xhci_hcd?
>>> ? imod_interval can be set to the current default 40000ns (160*250ns) and
>>> overwritten if
>>> device_property_read_u32() returns something else.
>>>
>>
>> Isn't the 160 value quite aggressive anyway? Section 5.5.2.2 of the xHCI spec
>> says that maximum observable interrupt rate should never exceed 8000
>> interrupts/second. I believe the IMOD value in the most aggressive case would
>> then be 500 by this statement [ 1 / (250e-9 * 500) = 8000 irqs/second ]
>>
>> Perhaps I am misreading the spec or just doing the math wrong? With the default
>> value of 160, we are interrupting 25,000 irq/second...which is over 3 times the
>> maximum stated value (again, assuming I did the math right)
>>
>> Anyway, my preference would be to set the IMOD default val to 4000 (~1ms) per
>> the recommended value in Table 49 of the spec and allow platforms to adjust as
>> necessary from that point.
>>
>> Thoughts on this?
>>
> 
> I think current 40us is still reasonable, it's about ~3 times per
> usb microframe (125us) .8000 interrupts per second just covers the microframe rate,
> which is the shortest interval a interrupt transfer can require service.
> 
> 1ms interrupt interval is too long. In the worst case ~8 microframes could pass
> before the driver is aware of a error it needs to take care of.
> USB 3.1 devices can transfer 6 burst of 16 max sized packets (6 x 16 x 2014) bytes
> per microframe.
> 
> closer to 125us could probably work as well, but unless we are fixing some issue or
> getting some other bigger benefit out of it I don't think it's worth changing it
> just to see if it breaks stuff.

I agree - my patch will just stick with the current default as the fallback
value if no device property is submitted.

> 
> Thanks
> -Mathias
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at? http://vger.kernel.org/majordomo-info.html


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