| Message ID | 1511284690-3878-2-git-send-email-awallis@codeaurora.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | usb: xhci: addition of timing quirk | expand |
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(-) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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.
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@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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Adam
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)