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 4ns (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 majord...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 4ns (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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 4ns (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 majord...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 majord...@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 4ns (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 majord...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 majord...@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 4ns (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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 majord...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rob Herring]

On Tue, Nov 21, 2017 at 1:49 PM, Adam Wallis  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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 
>> ---
>>  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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 
> ---
>  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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html