Add logic to consolidate TRBs for Synopsys xHC

PATCH Superseded Tejas Joglekar 6 participants 22 messages May 27, 2020 – Aug 4, 2020

Subsystems (4)

Device tree bindings USB USB xHCI driver DesignWare USB3 DRD IP driver

lore

Share & tools

lore

Tejas Joglekar

author

2020-05-27 10:42 UTC #0 lore raw

v3 cover Add logic to consolidate TRBs for Synopsys xHC

The Synopsys xHC has an internal TRB cache of size TRB_CACHE_SIZE for each endpoint. The default value for TRB_CACHE_SIZE is 16 for SS and 8 for HS. The controller loads and updates the TRB cache from the transfer ring in system memory whenever the driver issues a start transfer or update transfer command.

For chained TRBs, the Synopsys xHC requires that the total amount of bytes for all TRBs loaded in the TRB cache be greater than or equal to 1 MPS. Or the chain ends within the TRB cache (with a last TRB).

If this requirement is not met, the controller will not be able to send or receive a packet and it will hang causing a driver timeout and error.

This patch set adds logic to the XHCI driver to detect and prevent this from happening along with the quirk to enable this logic for Synopsys HAPS platform.

Based on Mathias's feedback on previous implementation where consolidation was done in TRB cache, with this patch series the implementation is done during mapping of the URB by consolidating the SG list into a temporary buffer if the SG list buffer sizes within TRB_CACHE_SIZE is less than MPS.

Changes since v2:
- Modified the xhci_unmap_temp_buffer function to unmap dma and clear the dma flag
- Removed RFC tag

Changes since v1:
- Comments from Greg are addressed on [PATCH 4/4] and [PATCH 1/4]
- Renamed the property and quirk as in other patches based on [PATCH 1/4]

Tejas Joglekar (4):
dt-bindings: usb: Add documentation for SG trb cache size quirk
usb: xhci: Set quirk for XHCI_SG_TRB_CACHE_SIZE_QUIRK
usb: dwc3: Add device property sgl-trb-cache-size-quirk
usb: xhci: Use temporary buffer to consolidate SG

 Documentation/devicetree/bindings/usb/dwc3.txt     |   4 +
 Documentation/devicetree/bindings/usb/usb-xhci.txt |   3 +
 drivers/usb/dwc3/core.c                            |   2 +
 drivers/usb/dwc3/core.h                            |   2 +
 drivers/usb/dwc3/dwc3-haps.c                       |   1 +
 drivers/usb/dwc3/host.c                            |   6 +-
 drivers/usb/host/xhci-pci.c                        |   3 +
 drivers/usb/host/xhci-plat.c                       |   4 +
 drivers/usb/host/xhci-ring.c                       |   2 +-
 drivers/usb/host/xhci.c                            | 135 +++++++++++++++++++++
 drivers/usb/host/xhci.h                            |   5 +
 11 files changed, 165 insertions(+), 2 deletions(-)

Signature · 1 line

2.11.0

Tejas Joglekar

author

2020-05-27 10:43 UTC #1 lore raw

PATCH v3 1/4 dt-bindings: usb: Add documentation for SG trb cache size quirk

This commit adds the documentation for sgl-trb-cache-size-quirk, and snps,sgl-trb-cache-size-quirk property. These when set enables the quirk for XHCI driver for consolidation of sg list into a temporary buffer when small buffer sizes are scattered over the sg list not making up to MPS or total transfer size within TRB cache size with Synopsys xHC.

Signed-off-by Tejas Joglekar <[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc3.txt     | 4 ++++
 Documentation/devicetree/bindings/usb/usb-xhci.txt | 3 +++
 2 files changed, 7 insertions(+)

Documentation/devicetree/bindings/usb/dwc3.txt +4 -0

index d03edf9d3935..0fcbaa51f66e 100644
@@ -102,6 +102,10 @@ Optional properties:
 			this and tx-thr-num-pkt-prd to a valid, non-zero value
 			1-16 (DWC_usb31 programming guide section 1.2.3) to
 			enable periodic ESS TX threshold.
+ - snps,sgl-trb-cache-size-quirk: enable sg list consolidation - host mode
+			only. Set to use SG buffers of at least MPS size
+			by consolidating smaller SG buffers list into a
+			single buffer.
 
  - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
  - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0

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

index dc025f126d71..c53eb19ae67e 100644
@@ -44,6 +44,9 @@ Optional properties:
   - quirk-broken-port-ped: set if the controller has broken port disable mechanism
   - imod-interval-ns: default interrupt moderation interval is 5000ns
   - phys : see usb-hcd.yaml in the current directory
+  - sgl-trb-cache-size-quirk: set if you need to consolidate sg list into a
+    temporary buffer when small SG buffer sizes does not make upto MPS
+    size or total transfer size across the TRB cache size.
 
 additionally the properties from usb-hcd.yaml (in the current directory) are
 supported.

Signature · 1 line

2.11.0

Rob Herring

maintainer

2020-05-29 18:05 UTC #2 lore raw

Re: [PATCH v3 1/4] dt-bindings: usb: Add documentation for SG trb cache size quirk

Quoted 33 lines · Tejas Joglekar

This commit adds the documentation for sgl-trb-cache-size-quirk, and snps,sgl-trb-cache-size-quirk property. These when set enables the quirk for XHCI driver for consolidation of sg list into a temporary buffer when small buffer sizes are scattered over the sg list not making up to MPS or total transfer size within TRB cache size with Synopsys xHC.
Signed-off-by Tejas Joglekar <[email protected]>
---
 Documentation/devicetree/bindings/usb/dwc3.txt     | 4 ++++
 Documentation/devicetree/bindings/usb/usb-xhci.txt | 3 +++
 2 files changed, 7 insertions(+)
Documentation/devicetree/bindings/usb/dwc3.txt +4 -0
index d03edf9d3935..0fcbaa51f66e 100644
@@ -102,6 +102,10 @@ Optional properties:
 			this and tx-thr-num-pkt-prd to a valid, non-zero value
 			1-16 (DWC_usb31 programming guide section 1.2.3) to
 			enable periodic ESS TX threshold.
+ - snps,sgl-trb-cache-size-quirk: enable sg list consolidation - host mode
+			only. Set to use SG buffers of at least MPS size
+			by consolidating smaller SG buffers list into a
+			single buffer.
 
  - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
  - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0
Documentation/devicetree/bindings/usb/usb-xhci.txt +0 -0
index dc025f126d71..c53eb19ae67e 100644

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

Quoted 6 lines

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

@@ -44,6 +44,9 @@ Optional properties:
   - quirk-broken-port-ped: set if the controller has broken port disable mechanism
   - imod-interval-ns: default interrupt moderation interval is 5000ns
   - phys : see usb-hcd.yaml in the current directory
+  - sgl-trb-cache-size-quirk: set if you need to consolidate sg list into a
+    temporary buffer when small SG buffer sizes does not make upto MPS
+    size or total transfer size across the TRB cache size.

Still don't understand why you have 2 properties? Is this a generic issue for multiple XHCI controllers? If yes, you don't need the first one. If no, then you don't need the second one.

Really, I'd prefer neither, and this should be implied by a specific compatible string. Having a separate property doesn't work if you find this issue later on after already adding XHCI support. IOW, don't make users update their DT to handle a quirk.

Rob

Tejas Joglekar

author

2020-05-27 10:43 UTC #3 lore raw

PATCH v3 2/4 usb: xhci: Set quirk for XHCI_SG_TRB_CACHE_SIZE_QUIRK

In reply to Tejas Joglekar

This commit enables the sgl-trb-cache-size-quirk property is set for the Synopsys xHC. This patch fixes the SNPS xHC hang issue when the data is scattered across small buffers which does not make atleast MPS size for given TRB cache size of SNPS xHC.

Signed-off-by Tejas Joglekar <[email protected]>

---
 drivers/usb/host/xhci-pci.c  | 3 +++
 drivers/usb/host/xhci-plat.c | 4 ++++
 drivers/usb/host/xhci.h      | 1 +
 3 files changed, 8 insertions(+)

drivers/usb/host/xhci-pci.c +3 -0

index 766b74723e64..05b743a819ac 100644
@@ -268,6 +268,9 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	     pdev->device == 0x9026)
 		xhci->quirks |= XHCI_RESET_PLL_ON_DISCONNECT;
 
+	if (pdev->vendor == PCI_VENDOR_ID_SYNOPSYS)
+		xhci->quirks |= XHCI_SG_TRB_CACHE_SIZE_QUIRK;
+
 	if (xhci->quirks & XHCI_RESET_ON_RESUME)
 		xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 				"QUIRK: Resetting on resume");

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

index 1d4f6f85f0fe..a8526fe50811 100644
@@ -288,6 +288,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		if (device_property_read_bool(tmpdev, "quirk-broken-port-ped"))
 			xhci->quirks |= XHCI_BROKEN_PORT_PED;
 
+		if (device_property_read_bool(tmpdev,
+					      "sgl-trb-cache-size-quirk"))
+			xhci->quirks |= XHCI_SG_TRB_CACHE_SIZE_QUIRK;
+
 		device_property_read_u32(tmpdev, "imod-interval-ns",
 					 &xhci->imod_interval);
 	}

drivers/usb/host/xhci.h +1 -0

index 86cfefdd6632..69e3587e805c 100644
@@ -1873,6 +1873,7 @@ struct xhci_hcd {
 #define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
 #define XHCI_RESET_PLL_ON_DISCONNECT	BIT_ULL(34)
 #define XHCI_SNPS_BROKEN_SUSPEND    BIT_ULL(35)
+#define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(36)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;

Signature · 1 line

2.11.0

Tejas Joglekar

author

2020-05-27 10:43 UTC #4 lore raw

PATCH v3 3/4 usb: dwc3: Add device property sgl-trb-cache-size-quirk

In reply to Tejas Joglekar

This commit adds the sgl-trb-cache-size-quirk property to enable quirk for the XHCI driver with Synopsys xHC. This property is enabled as initial property for the dwc3-haps driver.

Signed-off-by Tejas Joglekar <[email protected]>

---
 drivers/usb/dwc3/core.c      | 2 ++
 drivers/usb/dwc3/core.h      | 2 ++
 drivers/usb/dwc3/dwc3-haps.c | 1 +
 drivers/usb/dwc3/host.c      | 6 +++++-
 4 files changed, 10 insertions(+), 1 deletion(-)

drivers/usb/dwc3/core.c +2 -0

index 25c686a752b0..bc295477e1bc 100644
@@ -1299,6 +1299,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 				"snps,usb3_lpm_capable");
 	dwc->usb2_lpm_disable = device_property_read_bool(dev,
 				"snps,usb2-lpm-disable");
+	dwc->sgl_trb_cache_size_quirk = device_property_read_bool(dev,
+				"snps,sgl-trb-cache-size-quirk");
 	device_property_read_u8(dev, "snps,rx-thr-num-pkt-prd",
 				&rx_thr_num_pkt_prd);
 	device_property_read_u8(dev, "snps,rx-max-burst-prd",

drivers/usb/dwc3/core.h +2 -0

index 013f42a2b5dc..0dca0dbf4309 100644
@@ -1021,6 +1021,7 @@ struct dwc3_scratchpad_array {
  *			not needed for DWC_usb31 version 1.70a-ea06 and below
  * @usb3_lpm_capable: set if hadrware supports Link Power Management
  * @usb2_lpm_disable: set to disable usb2 lpm
+ * @sgl_trb_cache_size_quirk: set to enable the SG list consolidation
  * @disable_scramble_quirk: set if we enable the disable scramble quirk
  * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
  * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk

@@ -1220,6 +1221,7 @@ struct dwc3 {
 	unsigned		dis_start_transfer_quirk:1;
 	unsigned		usb3_lpm_capable:1;
 	unsigned		usb2_lpm_disable:1;
+	unsigned		sgl_trb_cache_size_quirk:1;
 
 	unsigned		disable_scramble_quirk:1;
 	unsigned		u2exit_lfps_quirk:1;

drivers/usb/dwc3/dwc3-haps.c +1 -0

index 3cecbf169452..9311cbe5f264 100644
@@ -29,6 +29,7 @@ static const struct property_entry initial_properties[] = {
 	PROPERTY_ENTRY_BOOL("snps,usb3_lpm_capable"),
 	PROPERTY_ENTRY_BOOL("snps,has-lpm-erratum"),
 	PROPERTY_ENTRY_BOOL("snps,dis_enblslpm_quirk"),
+	PROPERTY_ENTRY_BOOL("snps,sgl-trb-cache-size-quirk"),
 	PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
 	{ },
 };

drivers/usb/dwc3/host.c +5 -1

index bef1c1ac2067..e0089c82728e 100644
@@ -44,7 +44,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
 
 int dwc3_host_init(struct dwc3 *dwc)
 {
-	struct property_entry	props[4];
+	struct property_entry	props[5];
 	struct platform_device	*xhci;
 	int			ret, irq;
 	struct resource		*res;

@@ -95,6 +95,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (dwc->usb2_lpm_disable)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
+	if (dwc->sgl_trb_cache_size_quirk)
+		props[prop_idx++] =
+			PROPERTY_ENTRY_BOOL("sgl-trb-cache-size-quirk");
+
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
 	 * where Port Disable command doesn't work.

Signature · 1 line

2.11.0

Tejas Joglekar

author

2020-07-06 05:07 UTC #5 lore raw

Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk

Hi Felipe,

Quoted 72 lines · Tejas Joglekar

This commit adds the sgl-trb-cache-size-quirk property to enable quirk for the XHCI driver with Synopsys xHC. This property is enabled as initial property for the dwc3-haps driver.

Signed-off-by Tejas Joglekar <[email protected]>

---
 drivers/usb/dwc3/core.c      | 2 ++
 drivers/usb/dwc3/core.h      | 2 ++
 drivers/usb/dwc3/dwc3-haps.c | 1 +
 drivers/usb/dwc3/host.c      | 6 +++++-
 4 files changed, 10 insertions(+), 1 deletion(-)

drivers/usb/dwc3/core.c +2 -0

index 25c686a752b0..bc295477e1bc 100644
@@ -1299,6 +1299,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 				"snps,usb3_lpm_capable");
 	dwc->usb2_lpm_disable = device_property_read_bool(dev,
 				"snps,usb2-lpm-disable");
+	dwc->sgl_trb_cache_size_quirk = device_property_read_bool(dev,
+				"snps,sgl-trb-cache-size-quirk");
 	device_property_read_u8(dev, "snps,rx-thr-num-pkt-prd",
 				&rx_thr_num_pkt_prd);
 	device_property_read_u8(dev, "snps,rx-max-burst-prd",

drivers/usb/dwc3/core.h +2 -0

index 013f42a2b5dc..0dca0dbf4309 100644
@@ -1021,6 +1021,7 @@ struct dwc3_scratchpad_array {
  *			not needed for DWC_usb31 version 1.70a-ea06 and below
  * @usb3_lpm_capable: set if hadrware supports Link Power Management
  * @usb2_lpm_disable: set to disable usb2 lpm
+ * @sgl_trb_cache_size_quirk: set to enable the SG list consolidation
  * @disable_scramble_quirk: set if we enable the disable scramble quirk
  * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
  * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk

@@ -1220,6 +1221,7 @@ struct dwc3 {
 	unsigned		dis_start_transfer_quirk:1;
 	unsigned		usb3_lpm_capable:1;
 	unsigned		usb2_lpm_disable:1;
+	unsigned		sgl_trb_cache_size_quirk:1;
 
 	unsigned		disable_scramble_quirk:1;
 	unsigned		u2exit_lfps_quirk:1;

drivers/usb/dwc3/dwc3-haps.c +1 -0

index 3cecbf169452..9311cbe5f264 100644
@@ -29,6 +29,7 @@ static const struct property_entry initial_properties[] = {
 	PROPERTY_ENTRY_BOOL("snps,usb3_lpm_capable"),
 	PROPERTY_ENTRY_BOOL("snps,has-lpm-erratum"),
 	PROPERTY_ENTRY_BOOL("snps,dis_enblslpm_quirk"),
+	PROPERTY_ENTRY_BOOL("snps,sgl-trb-cache-size-quirk"),
 	PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
 	{ },
 };

drivers/usb/dwc3/host.c +1 -1

index bef1c1ac2067..e0089c82728e 100644
@@ -44,7 +44,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
 
 int dwc3_host_init(struct dwc3 *dwc)
 {
-	struct property_entry	props[4];
+	struct property_entry	props[5];
 	struct platform_device	*xhci;
 	int			ret, irq;
 	struct resource		*res;

drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 104

Quoted 10 lines

drivers/usb/dwc3/host.c +4 -0

@@ -95,6 +95,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (dwc->usb2_lpm_disable)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
+	if (dwc->sgl_trb_cache_size_quirk)
+		props[prop_idx++] =
+			PROPERTY_ENTRY_BOOL("sgl-trb-cache-size-quirk");
+
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
 	 * where Port Disable command doesn't work.

Does this implementation looks good to you? Rob has some concerned over the DT entries, you suggested using compatible string with this quirk addition.
Can you please brief about how you would like to have this quirk implemented? I can send the updated patch. My patch series is pending for merge just because of the DT and quirk issue. Can you please help?

Thanks & Regards,
Tejas Joglekar

Felipe Balbi

2020-07-06 06:43 UTC #6 lore raw

Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk

Hi,

Quoted 4 lines · Tejas Joglekar

Quoted 4 lines

drivers/usb/dwc3/host.c +0 -0

@@ -95,6 +95,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (dwc->usb2_lpm_disable)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");

Quoted 13 lines

drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 104
Quoted 8 lines
drivers/usb/dwc3/host.c +4 -0
+	if (dwc->sgl_trb_cache_size_quirk)
+		props[prop_idx++] =
+			PROPERTY_ENTRY_BOOL("sgl-trb-cache-size-quirk");
+
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
 	 * where Port Disable command doesn't work.
Does this implementation looks good to you? Rob has some concerned over the DT entries, you suggested using compatible string with this quirk addition.
Can you please brief about how you would like to have this quirk implemented? I can send the updated patch. My patch series is pending for merge just because of the DT and quirk issue. Can you please help?

Yeah, you need to get into an agreement with Rob :-) I don't mind having extra DT flags for things which can't be detected in runtime, Rob disagrees.

Signature · 1 line

balbi

Tejas Joglekar

author

2020-07-07 17:21 UTC #7 lore raw

Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk

Hello Rob,

Quoted 22 lines · Felipe Balbi

Hi,
Quoted 4 lines · Tejas Joglekar
Quoted 4 lines
drivers/usb/dwc3/host.c +0 -0
@@ -95,6 +95,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (dwc->usb2_lpm_disable)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
Quoted 13 lines
drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 104
Quoted 8 lines
drivers/usb/dwc3/host.c +4 -0
+	if (dwc->sgl_trb_cache_size_quirk)
+		props[prop_idx++] =
+			PROPERTY_ENTRY_BOOL("sgl-trb-cache-size-quirk");
+
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
 	 * where Port Disable command doesn't work.
Does this implementation looks good to you? Rob has some concerned over the DT entries, you suggested using compatible string with this quirk addition.
Can you please brief about how you would like to have this quirk implemented? I can send the updated patch. My patch series is pending for merge just because of the DT and quirk issue. Can you please help?

Quoted 4 lines

Yeah, you need to get into an agreement with Rob :-) I don't mind having extra DT flags for things which can't be detected in runtime, Rob disagrees.

The compatible string is not suitable option as it does not work with platform drivers with PCI based system. Also Synopsys controllers IP version register is not visible to xhci driver and hence we don't have separate compatible string for each Synopsys version on the xhci driver side.
Due to which I depend on DT flag addition for the quirk. Can we add these DT flags and quirk?

Thanks & Regards,
Tejas Joglekar

Tejas Joglekar

author

2020-07-12 13:45 UTC #8 lore raw

Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk

In reply to Felipe Balbi

Hello Rob,

Quoted 22 lines · Felipe Balbi

Hi,
Quoted 4 lines · Tejas Joglekar
Quoted 4 lines
drivers/usb/dwc3/host.c +0 -0
@@ -95,6 +95,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (dwc->usb2_lpm_disable)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
Quoted 13 lines
drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 104
Quoted 8 lines
drivers/usb/dwc3/host.c +4 -0
+	if (dwc->sgl_trb_cache_size_quirk)
+		props[prop_idx++] =
+			PROPERTY_ENTRY_BOOL("sgl-trb-cache-size-quirk");
+
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
 	 * where Port Disable command doesn't work.
Does this implementation looks good to you? Rob has some concerned over the DT entries, you suggested using compatible string with this quirk addition.
Can you please brief about how you would like to have this quirk implemented? I can send the updated patch. My patch series is pending for merge just because of the DT and quirk issue. Can you please help?

Quoted 4 lines

Yeah, you need to get into an agreement with Rob :-) I don't mind having extra DT flags for things which can't be detected in runtime, Rob disagrees.

Thanks & Regards,
Tejas Joglekar

Tejas Joglekar

author

2020-07-15 05:51 UTC #9 lore raw

Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk

In reply to Felipe Balbi

Hi Rob,

Quoted 22 lines · Felipe Balbi

Hi,
Quoted 4 lines · Tejas Joglekar
Quoted 4 lines
drivers/usb/dwc3/host.c +0 -0
@@ -95,6 +95,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (dwc->usb2_lpm_disable)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
Quoted 13 lines
drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 104
Quoted 8 lines
drivers/usb/dwc3/host.c +4 -0
+	if (dwc->sgl_trb_cache_size_quirk)
+		props[prop_idx++] =
+			PROPERTY_ENTRY_BOOL("sgl-trb-cache-size-quirk");
+
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
 	 * where Port Disable command doesn't work.
Does this implementation looks good to you? Rob has some concerned over the DT entries, you suggested using compatible string with this quirk addition.
Can you please brief about how you would like to have this quirk implemented? I can send the updated patch. My patch series is pending for merge just because of the DT and quirk issue. Can you please help?

Quoted 4 lines

Yeah, you need to get into an agreement with Rob :-) I don't mind having extra DT flags for things which can't be detected in runtime, Rob disagrees.

Thanks & Regards,
Tejas Joglekar

Felipe Balbi

2020-07-21 09:48 UTC #10 lore raw

Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk

Quoted 29 lines · Tejas Joglekar

Hi Rob,
Quoted 22 lines · Felipe Balbi
Hi,
Quoted 4 lines · Tejas Joglekar
Quoted 4 lines
drivers/usb/dwc3/host.c +0 -0
@@ -95,6 +95,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (dwc->usb2_lpm_disable)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
Quoted 13 lines
drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 104
Quoted 8 lines
drivers/usb/dwc3/host.c +4 -0
+	if (dwc->sgl_trb_cache_size_quirk)
+		props[prop_idx++] =
+			PROPERTY_ENTRY_BOOL("sgl-trb-cache-size-quirk");
+
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
 	 * where Port Disable command doesn't work.
Does this implementation looks good to you? Rob has some concerned over the DT entries, you suggested using compatible string with this quirk addition.
Can you please brief about how you would like to have this quirk implemented? I can send the updated patch. My patch series is pending for merge just because of the DT and quirk issue. Can you please help?
Quoted 4 lines
Yeah, you need to get into an agreement with Rob :-) I don't mind having extra DT flags for things which can't be detected in runtime, Rob disagrees.

Quoted 5 lines

The compatible string is not suitable option as it does not work with platform drivers with PCI based system. Also Synopsys controllers IP version register is not visible to xhci driver and hence we don't have separate compatible string for each Synopsys version on the xhci driver side.
Due to which I depend on DT flag addition for the quirk. Can we add these DT flags and quirk?

As I said, I'm well aware of the situation regarding usage of compatible strings and the fact that dwc3 must work on PCI and non-PCI systems (I wrote the thing as it is after all). The person blocking new quirk flags is Rob, not me. You need to convince Rob that this is the way to go.

Rob, ball's in your court. Sorry.

Signature · 1 line

balbi

Tejas Joglekar

author

2020-07-21 17:03 UTC #11 lore raw

Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk

Hello,

Quoted 37 lines · Felipe Balbi

Quoted 29 lines · Tejas Joglekar
Hi Rob,
Quoted 22 lines · Felipe Balbi
Hi,
Quoted 4 lines · Tejas Joglekar
Quoted 4 lines
drivers/usb/dwc3/host.c +0 -0
@@ -95,6 +95,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (dwc->usb2_lpm_disable)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
Quoted 13 lines
drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 104
Quoted 8 lines
drivers/usb/dwc3/host.c +4 -0
+	if (dwc->sgl_trb_cache_size_quirk)
+		props[prop_idx++] =
+			PROPERTY_ENTRY_BOOL("sgl-trb-cache-size-quirk");
+
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
 	 * where Port Disable command doesn't work.
Does this implementation looks good to you? Rob has some concerned over the DT entries, you suggested using compatible string with this quirk addition.
Can you please brief about how you would like to have this quirk implemented? I can send the updated patch. My patch series is pending for merge just because of the DT and quirk issue. Can you please help?
Quoted 4 lines
Yeah, you need to get into an agreement with Rob :-) I don't mind having extra DT flags for things which can't be detected in runtime, Rob disagrees.
Quoted 5 lines
The compatible string is not suitable option as it does not work with platform drivers with PCI based system. Also Synopsys controllers IP version register is not visible to xhci driver and hence we don't have separate compatible string for each Synopsys version on the xhci driver side.
Due to which I depend on DT flag addition for the quirk. Can we add these DT flags and quirk?

Quoted 5 lines

As I said, I'm well aware of the situation regarding usage of compatible strings and the fact that dwc3 must work on PCI and non-PCI systems (I wrote the thing as it is after all). The person blocking new quirk flags is Rob, not me. You need to convince Rob that this is the way to go.

@Felipe: Sorry for confusion if any, previous mail was intended for Rob asking about his approval.

Quoted 1 line

Rob, ball's in your court. Sorry.>

@Rob: As I and Felipe have mentioned before, it is very much necessary to have quirk flags for the current changes as compatible string would not be a solution for PCI and non-PCI systems. Can you please approve this change ? If you have any concern about naming or any other thing, please let us know.

Thanks & Regards,
Tejas Joglekar

Tejas Joglekar

author

2020-07-31 10:13 UTC #12 lore raw

Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk

Hello Rob,

Quoted 45 lines · Tejas Joglekar

Hello,
Quoted 37 lines · Felipe Balbi
Quoted 29 lines · Tejas Joglekar
Hi Rob,
Quoted 22 lines · Felipe Balbi
Hi,
Quoted 4 lines · Tejas Joglekar
Quoted 4 lines
drivers/usb/dwc3/host.c +0 -0
@@ -95,6 +95,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (dwc->usb2_lpm_disable)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
Quoted 13 lines
drivers/usb/dwc3/host.c int dwc3_host_init(struct dwc3 *dwc) · line 104
Quoted 8 lines
drivers/usb/dwc3/host.c +4 -0
+	if (dwc->sgl_trb_cache_size_quirk)
+		props[prop_idx++] =
+			PROPERTY_ENTRY_BOOL("sgl-trb-cache-size-quirk");
+
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
 	 * where Port Disable command doesn't work.
Does this implementation looks good to you? Rob has some concerned over the DT entries, you suggested using compatible string with this quirk addition.
Can you please brief about how you would like to have this quirk implemented? I can send the updated patch. My patch series is pending for merge just because of the DT and quirk issue. Can you please help?
Quoted 4 lines
Yeah, you need to get into an agreement with Rob :-) I don't mind having extra DT flags for things which can't be detected in runtime, Rob disagrees.
Quoted 5 lines
The compatible string is not suitable option as it does not work with platform drivers with PCI based system. Also Synopsys controllers IP version register is not visible to xhci driver and hence we don't have separate compatible string for each Synopsys version on the xhci driver side.
Due to which I depend on DT flag addition for the quirk. Can we add these DT flags and quirk?
Quoted 5 lines
As I said, I'm well aware of the situation regarding usage of compatible strings and the fact that dwc3 must work on PCI and non-PCI systems (I wrote the thing as it is after all). The person blocking new quirk flags is Rob, not me. You need to convince Rob that this is the way to go.

Quoted 8 lines

@Felipe: Sorry for confusion if any, previous mail was intended for Rob asking about his approval.
Quoted 1 line
Rob, ball's in your court. Sorry.>
@Rob: As I and Felipe have mentioned before, it is very much necessary to have quirk flags for the current changes as compatible string would not be a solution for PCI and non-PCI systems. Can you please approve this change ? If you have any concern about naming or any other thing, please let us know.

Can you please comment?

Thanks & Regards,
Tejas Joglekar

Thinh Nguyen

maintainer

2020-08-04 00:44 UTC #13 lore raw

Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk

Hi Rob,

Quoted 52 lines · Tejas Joglekar

Hello Rob,
Quoted 42 lines · Tejas Joglekar
Hello,
Quoted 34 lines · Felipe Balbi
Quoted 27 lines · Tejas Joglekar
Hi Rob,
Quoted 20 lines · Felipe Balbi
Hi,
Quoted 4 lines · Tejas Joglekar
Quoted 4 lines
(quoted diff) +0 -0
@@ -95,6 +95,10 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (dwc->usb2_lpm_disable)
 		props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
Quoted 13 lines
Review int dwc3_host_init(struct dwc3 *dwc) · line 104
Quoted 8 lines
(quoted diff) +4 -0
+	if (dwc->sgl_trb_cache_size_quirk)
+		props[prop_idx++] =
+			PROPERTY_ENTRY_BOOL("sgl-trb-cache-size-quirk");
+
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
 	 * where Port Disable command doesn't work.
Does this implementation looks good to you? Rob has some concerned over the DT entries, you suggested using compatible string with this quirk addition.
Can you please brief about how you would like to have this quirk implemented? I can send the updated patch. My patch series is pending for merge just because of the DT and quirk issue. Can you please help?
Quoted 4 lines
Yeah, you need to get into an agreement with Rob :-) I don't mind having extra DT flags for things which can't be detected in runtime, Rob disagrees.
Quoted 5 lines
The compatible string is not suitable option as it does not work with platform drivers with PCI based system. Also Synopsys controllers IP version register is not visible to xhci driver and hence we don't have separate compatible string for each Synopsys version on the xhci driver side.
Due to which I depend on DT flag addition for the quirk. Can we add these DT flags and quirk?
Quoted 5 lines
As I said, I'm well aware of the situation regarding usage of compatible strings and the fact that dwc3 must work on PCI and non-PCI systems (I wrote the thing as it is after all). The person blocking new quirk flags is Rob, not me. You need to convince Rob that this is the way to go.
Quoted 8 lines
@Felipe: Sorry for confusion if any, previous mail was intended for Rob asking about his approval.
Quoted 1 line
Rob, ball's in your court. Sorry.>
@Rob: As I and Felipe have mentioned before, it is very much necessary to have quirk flags for the current changes as compatible string would not be a solution for PCI and non-PCI systems. Can you please approve this change ? If you have any concern about naming or any other thing, please let us know.

Quoted 6 lines

Can you please comment?
Thanks & Regards,
Tejas Joglekar

Can you help provide pointers and changes that Tejas can make to help bring this issue to conclusion?

Thanks,
Thinh

Jun Li

2020-08-04 02:00 UTC #14 lore raw

Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk

Quoted 58 lines · Thinh Nguyen <[email protected]> 于2020年8月4日周二上午8:46写道：

Hi Rob,
Quoted 54 lines · Tejas Joglekar
Hello Rob,
Quoted 42 lines · Tejas Joglekar
Hello,
Quoted 34 lines · Felipe Balbi
Quoted 27 lines · Tejas Joglekar
Hi Rob,
Quoted 20 lines · Felipe Balbi
Hi,
Quoted 4 lines · Tejas Joglekar
Quoted 4 lines
(quoted diff) +0 -0
@@ -95,6 +95,10 @@ int dwc3_host_init(struct dwc3 *dwc)
        if (dwc->usb2_lpm_disable)
                props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb2-lpm-disable");
 
Quoted 13 lines
Review int dwc3_host_init(struct dwc3 *dwc) · line 104
Quoted 8 lines
(quoted diff) +4 -0
+       if (dwc->sgl_trb_cache_size_quirk)
+               props[prop_idx++] =
+                       PROPERTY_ENTRY_BOOL("sgl-trb-cache-size-quirk");
+
        /**
         * WORKAROUND: dwc3 revisions <=3.00a have a limitation
         * where Port Disable command doesn't work.
Does this implementation looks good to you? Rob has some concerned over the DT entries, you suggested using compatible string with this quirk addition.
Can you please brief about how you would like to have this quirk implemented? I can send the updated patch. My patch series is pending for merge just because of the DT and quirk issue. Can you please help?
Quoted 4 lines
Yeah, you need to get into an agreement with Rob :-) I don't mind having extra DT flags for things which can't be detected in runtime, Rob disagrees.
Quoted 5 lines
The compatible string is not suitable option as it does not work with platform drivers with PCI based system. Also Synopsys controllers IP version register is not visible to xhci driver and hence we don't have separate compatible string for each Synopsys version on the xhci driver side.
Due to which I depend on DT flag addition for the quirk. Can we add these DT flags and quirk?
Quoted 5 lines
As I said, I'm well aware of the situation regarding usage of compatible strings and the fact that dwc3 must work on PCI and non-PCI systems (I wrote the thing as it is after all). The person blocking new quirk flags is Rob, not me. You need to convince Rob that this is the way to go.
Quoted 8 lines
@Felipe: Sorry for confusion if any, previous mail was intended for Rob asking about his approval.
Quoted 1 line
Rob, ball's in your court. Sorry.>
@Rob: As I and Felipe have mentioned before, it is very much necessary to have quirk flags for the current changes as compatible string would not be a solution for PCI and non-PCI systems. Can you please approve this change ? If you have any concern about naming or any other thing, please let us know.
Can you please comment?

Quoted 7 lines

Quoted 4 lines
Thanks & Regards,
Tejas Joglekar
Can you help provide pointers and changes that Tejas can make to help bring this issue to conclusion?

We really need a direction to handle growing dwc3 quirks/flags as dwc3 is widly used,
I have a patchset also pending there after tried both property[1] and platform data[2].

[1] https://www.spinics.net/lists/linux-usb/msg196055.html
[2] https://www.spinics.net/lists/arm-kernel/msg824995.html

thanks
Li Jun

Quoted 3 lines

Thanks,
Thinh

Tejas Joglekar

author

2020-05-27 10:44 UTC #15 lore raw

PATCH v3 4/4 usb: xhci: Use temporary buffer to consolidate SG

In reply to Tejas Joglekar

If this requirement is not met, the controller will not be able to send or receive a packet and it will hang causing a driver timeout and error.

This can be a problem if a class driver queues SG requests with many small-buffer entries. The XHCI driver will create a chained TRB for each entry which may trigger this issue.

This patch adds logic to the XHCI driver to detect and prevent this from happening.

For every (TRB_CACHE_SIZE - 2), we check the total buffer size of the SG list and if the last window of (TRB_CACHE_SIZE - 2) SG list length and we don't make up at least 1 MPS, we create a temporary buffer to consolidate full SG list into the buffer.

We check at (TRB_CACHE_SIZE - 2) window because it is possible that there would be a link and/or event data TRB that take up to 2 of the cache entries.

We discovered this issue with devices on other platforms but have not yet come across any device that triggers this on Linux. But it could be a real problem now or in the future. All it takes is N number of small chained TRBs. And other instances of the Synopsys IP may have smaller values for the TRB_CACHE_SIZE which would exacerbate the problem.

Signed-off-by Tejas Joglekar <[email protected]>

---
 drivers/usb/host/xhci-ring.c |   2 +-
 drivers/usb/host/xhci.c      | 135 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h      |   4 ++
 3 files changed, 140 insertions(+), 1 deletion(-)

drivers/usb/host/xhci-ring.c +1 -1

index 0fda0c0f4d31..104c9f683375 100644
@@ -3325,7 +3325,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 
 	full_len = urb->transfer_buffer_length;
 	/* If we have scatter/gather list, we use it. */
-	if (urb->num_sgs) {
+	if (urb->num_sgs && !(urb->transfer_flags & URB_DMA_MAP_SINGLE)) {
 		num_sgs = urb->num_mapped_sgs;
 		sg = urb->sg;
 		addr = (u64) sg_dma_address(sg);

drivers/usb/host/xhci.c +135 -0

159 lines not on this page · raw message

drivers/usb/host/xhci.h +4 -0

index 69e3587e805c..2d3ae699e40c 100644
@@ -1330,6 +1330,10 @@ enum xhci_setup_dev {
 #define TRB_SIA			(1<<31)
 #define TRB_FRAME_ID(p)		(((p) & 0x7ff) << 20)
 
+/* TRB cache size for xHC with TRB cache */
+#define TRB_CACHE_SIZE_HS	8
+#define TRB_CACHE_SIZE_SS	16
+
 struct xhci_generic_trb {
 	__le32 field[4];
 };

Signature · 1 line

2.11.0

Tejas Joglekar

author

2020-06-08 04:32 UTC #16 lore raw

In reply to Tejas Joglekar

Hi Mathias,
Will this be added to your next branch ?

Quoted 38 lines · Tejas Joglekar

The Synopsys xHC has an internal TRB cache of size TRB_CACHE_SIZE for each endpoint. The default value for TRB_CACHE_SIZE is 16 for SS and 8 for HS. The controller loads and updates the TRB cache from the transfer ring in system memory whenever the driver issues a start transfer or update transfer command.
For chained TRBs, the Synopsys xHC requires that the total amount of bytes for all TRBs loaded in the TRB cache be greater than or equal to 1 MPS. Or the chain ends within the TRB cache (with a last TRB).
If this requirement is not met, the controller will not be able to send or receive a packet and it will hang causing a driver timeout and error.
This patch set adds logic to the XHCI driver to detect and prevent this from happening along with the quirk to enable this logic for Synopsys HAPS platform.
Based on Mathias's feedback on previous implementation where consolidation was done in TRB cache, with this patch series the implementation is done during mapping of the URB by consolidating the SG list into a temporary buffer if the SG list buffer sizes within TRB_CACHE_SIZE is less than MPS.
Changes since v2:
- Modified the xhci_unmap_temp_buffer function to unmap dma and clear the dma flag
- Removed RFC tag
Changes since v1:
- Comments from Greg are addressed on [PATCH 4/4] and [PATCH 1/4]
- Renamed the property and quirk as in other patches based on [PATCH 1/4]
Tejas Joglekar (4):
dt-bindings: usb: Add documentation for SG trb cache size quirk
usb: xhci: Set quirk for XHCI_SG_TRB_CACHE_SIZE_QUIRK
usb: dwc3: Add device property sgl-trb-cache-size-quirk
usb: xhci: Use temporary buffer to consolidate SG

Quoted 13 lines

 Documentation/devicetree/bindings/usb/dwc3.txt     |   4 +
 Documentation/devicetree/bindings/usb/usb-xhci.txt |   3 +
 drivers/usb/dwc3/core.c                            |   2 +
 drivers/usb/dwc3/core.h                            |   2 +
 drivers/usb/dwc3/dwc3-haps.c                       |   1 +
 drivers/usb/dwc3/host.c                            |   6 +-
 drivers/usb/host/xhci-pci.c                        |   3 +
 drivers/usb/host/xhci-plat.c                       |   4 +
 drivers/usb/host/xhci-ring.c                       |   2 +-
 drivers/usb/host/xhci.c                            | 135 +++++++++++++++++++++
 drivers/usb/host/xhci.h                            |   5 +
 11 files changed, 165 insertions(+), 2 deletions(-)

Thanks and Regards,
Tejas Joglekar

Mathias Nyman

2020-06-09 08:54 UTC #17 lore raw

Quoted 2 lines · Tejas Joglekar

Hi Mathias,
Will this be added to your next branch ?

There are still some opens Rob Herring pointed out regarding devicetree. Adding a compatible string for the synopsys xhci and setting quirk based on that sounds good to me.

Not sure how that works in cases where the xhci device is created by the DWC3 driver. Once we have a solution that Felipe and Rob agrees with I can take the whole series.

The xhci parts and PCI case looks good to me.

-Mathias

Tejas Joglekar

author

2020-06-11 18:07 UTC #18 lore raw

Hi,

Quoted 4 lines · Mathias Nyman

Quoted 2 lines · Tejas Joglekar
Hi Mathias,
Will this be added to your next branch ?

Quoted 8 lines

There are still some opens Rob Herring pointed out regarding devicetree. Adding a compatible string for the synopsys xhci and setting quirk based on that sounds good to me.
Not sure how that works in cases where the xhci device is created by the DWC3 driver. Once we have a solution that Felipe and Rob agrees with I can take the whole series.

@Felipe: Can you please have a look and see if the current implementation is ok or anything to be changed?

Quoted 5 lines

The xhci parts and PCI case looks good to me.
-Mathias

Thanks & Regards,
Tejas Joglekar

Tejas Joglekar

author

2020-06-30 06:29 UTC #19 lore raw

Hi Felipe,

Quoted 10 lines · Tejas Joglekar

Hi,
Quoted 8 lines · Mathias Nyman
Quoted 2 lines · Tejas Joglekar
Hi Mathias,
Will this be added to your next branch ?
There are still some opens Rob Herring pointed out regarding devicetree. Adding a compatible string for the synopsys xhci and setting quirk based on that sounds good to me.

Quoted 6 lines

Quoted 4 lines
Not sure how that works in cases where the xhci device is created by the DWC3 driver. Once we have a solution that Felipe and Rob agrees with I can take the whole series.
@Felipe: Can you please have a look and see if the current implementation is ok or anything to be changed?

Can you please have review my patch and suggest the changes required, so Mathias can accept the series?

Quoted 8 lines

Quoted 5 lines
The xhci parts and PCI case looks good to me.
-Mathias
Thanks & Regards,
Tejas Joglekar

Thanks & Regards,
Tejas Joglekar

Jun Li

2020-07-23 10:36 UTC #20 lore raw

In reply to Tejas Joglekar

Quoted 7 lines · Tejas Joglekar <[email protected]> 于2020年5月27日周三下午7:54写道：

The Synopsys xHC has an internal TRB cache of size TRB_CACHE_SIZE for each endpoint. The default value for TRB_CACHE_SIZE is 16 for SS and 8 for HS. The controller loads and updates the TRB cache from the transfer ring in system memory whenever the driver issues a start transfer or update transfer command.

Quoted 7 lines

For chained TRBs, the Synopsys xHC requires that the total amount of bytes for all TRBs loaded in the TRB cache be greater than or equal to 1 MPS. Or the chain ends within the TRB cache (with a last TRB).
If this requirement is not met, the controller will not be able to send or receive a packet and it will hang causing a driver timeout and error.

Hi Tejas Joglekar

I am debugging a similar issue on Synipsys XHC, it's not the same case but I am wondering if it also linked to this HW limitation.

My Synopsys XHC based host enable UAS, when enumerates a UAS HDD, one BULK-IN EP with stream enabled will not generate event for trb(with stream ID 1) after a 16/4096 bytes(with stream ID 2) finished in previous trb.

If I change the last OK urb/trb's buffer length from 4096 to 512, the issue will gone.

following is the sequence of the question EP-IN:

<idle>-0 [000] d.h1 154.961710: xhci_urb_giveback: ep3in-bulk:
urb ffff0001775f6f00 pipe 3221324672 slot 1 length 36/36 sgs 1/1 stream 1 flags 00040200
<idle>-0 [000] d.h1 154.962023: xhci_urb_giveback: ep3in-bulk:
urb ffff000177d00400 pipe 3221324672 slot 1 length 96/96 sgs 1/1 stream 1 flags 00040200
<idle>-0 [000] d.h1 154.970395: xhci_urb_giveback: ep3in-bulk:
urb ffff000177d00400 pipe 3221324672 slot 1 length 11/255 sgs 1/1 stream 1 flags 00040200
<idle>-0 [000] d.h1 154.970562: xhci_urb_giveback: ep3in-bulk:
urb ffff000177d00400 pipe 3221324672 slot 1 length 20/255 sgs 1/1 stream 1 flags 00040200
<idle>-0 [000] d.h1 154.970786: xhci_urb_giveback: ep3in-bulk:
urb ffff000177d00400 pipe 3221324672 slot 1 length 60/255 sgs 1/1 stream 1 flags 00040200
<idle>-0 [000] d.h1 155.851600: xhci_urb_giveback: ep3in-bulk:
urb ffff000177d00200 pipe 3221324672 slot 1 length 16/4096 sgs 1/1 stream 2 flags 00040200

/* then the next ep3-in trb will not generate event and stopped, so driver timeout in the end */
kworker/u8:2-349 [003] d..3 155.851987: xhci_urb_enqueue:
ep3in-bulk: urb ffff000170492400 pipe 3221324672 slot 1 length 0/32 sgs 1/1 stream 1 flags 00040200
kworker/u8:2-349 [003] d..4 155.851989: xhci_queue_trb: STREAM:
Buffer 00000000c19cf000 length 32 TD size 0 intr 0 type 'Normal' flags b:i:I:c:s:I:e:c
kworker/u8:2-349 [003] d..4 155.851991: xhci_inc_enq: STREAM
ffff000177f86f80: enq 0x00000000be0eb060(0x00000000be0eb000) deq 0x00000000be0eb050(0x00000000be0eb000) segs 2 stream 1 free_trbs 508 bounce 1024 cycle 1

Do you have any ideas?

thanks
Li Jun

Quoted 41 lines

This patch set adds logic to the XHCI driver to detect and prevent this from happening along with the quirk to enable this logic for Synopsys HAPS platform.
Based on Mathias's feedback on previous implementation where consolidation was done in TRB cache, with this patch series the implementation is done during mapping of the URB by consolidating the SG list into a temporary buffer if the SG list buffer sizes within TRB_CACHE_SIZE is less than MPS.
Changes since v2:
- Modified the xhci_unmap_temp_buffer function to unmap dma and clear the dma flag
- Removed RFC tag
Changes since v1:
- Comments from Greg are addressed on [PATCH 4/4] and [PATCH 1/4]
- Renamed the property and quirk as in other patches based on [PATCH 1/4]
Tejas Joglekar (4):
dt-bindings: usb: Add documentation for SG trb cache size quirk
usb: xhci: Set quirk for XHCI_SG_TRB_CACHE_SIZE_QUIRK
usb: dwc3: Add device property sgl-trb-cache-size-quirk
usb: xhci: Use temporary buffer to consolidate SG
 Documentation/devicetree/bindings/usb/dwc3.txt     |   4 +
 Documentation/devicetree/bindings/usb/usb-xhci.txt |   3 +
 drivers/usb/dwc3/core.c                            |   2 +
 drivers/usb/dwc3/core.h                            |   2 +
 drivers/usb/dwc3/dwc3-haps.c                       |   1 +
 drivers/usb/dwc3/host.c                            |   6 +-
 drivers/usb/host/xhci-pci.c                        |   3 +
 drivers/usb/host/xhci-plat.c                       |   4 +
 drivers/usb/host/xhci-ring.c                       |   2 +-
 drivers/usb/host/xhci.c                            | 135 +++++++++++++++++++++
 drivers/usb/host/xhci.h                            |   5 +
 11 files changed, 165 insertions(+), 2 deletions(-)
--
2.11.0

Tejas Joglekar

author

2020-07-24 00:15 UTC #21 lore raw

Hello,

Quoted 63 lines · Jun Li

Quoted 7 lines · Tejas Joglekar <[email protected]> 于2020年5月27日周三下午7:54写道：
The Synopsys xHC has an internal TRB cache of size TRB_CACHE_SIZE for each endpoint. The default value for TRB_CACHE_SIZE is 16 for SS and 8 for HS. The controller loads and updates the TRB cache from the transfer ring in system memory whenever the driver issues a start transfer or update transfer command.
Quoted 7 lines
For chained TRBs, the Synopsys xHC requires that the total amount of bytes for all TRBs loaded in the TRB cache be greater than or equal to 1 MPS. Or the chain ends within the TRB cache (with a last TRB).
If this requirement is not met, the controller will not be able to send or receive a packet and it will hang causing a driver timeout and error.
Hi Tejas Joglekar
I am debugging a similar issue on Synipsys XHC, it's not the same case but I am wondering if it also linked to this HW limitation.
My Synopsys XHC based host enable UAS, when enumerates a UAS HDD, one BULK-IN EP with stream enabled will not generate event for trb(with stream ID 1) after a 16/4096 bytes(with stream ID 2) finished in previous trb.
If I change the last OK urb/trb's buffer length from 4096 to 512, the issue will gone.
following is the sequence of the question EP-IN:
<idle>-0 [000] d.h1 154.961710: xhci_urb_giveback: ep3in-bulk:
urb ffff0001775f6f00 pipe 3221324672 slot 1 length 36/36 sgs 1/1 stream 1 flags 00040200
<idle>-0 [000] d.h1 154.962023: xhci_urb_giveback: ep3in-bulk:
urb ffff000177d00400 pipe 3221324672 slot 1 length 96/96 sgs 1/1 stream 1 flags 00040200
<idle>-0 [000] d.h1 154.970395: xhci_urb_giveback: ep3in-bulk:
urb ffff000177d00400 pipe 3221324672 slot 1 length 11/255 sgs 1/1 stream 1 flags 00040200
<idle>-0 [000] d.h1 154.970562: xhci_urb_giveback: ep3in-bulk:
urb ffff000177d00400 pipe 3221324672 slot 1 length 20/255 sgs 1/1 stream 1 flags 00040200
<idle>-0 [000] d.h1 154.970786: xhci_urb_giveback: ep3in-bulk:
urb ffff000177d00400 pipe 3221324672 slot 1 length 60/255 sgs 1/1 stream 1 flags 00040200
<idle>-0 [000] d.h1 155.851600: xhci_urb_giveback: ep3in-bulk:
urb ffff000177d00200 pipe 3221324672 slot 1 length 16/4096 sgs 1/1 stream 2 flags 00040200
/* then the next ep3-in trb will not generate event and stopped, so driver timeout in the end */
kworker/u8:2-349 [003] d..3 155.851987: xhci_urb_enqueue:
ep3in-bulk: urb ffff000170492400 pipe 3221324672 slot 1 length 0/32 sgs 1/1 stream 1 flags 00040200
kworker/u8:2-349 [003] d..4 155.851989: xhci_queue_trb: STREAM:
Buffer 00000000c19cf000 length 32 TD size 0 intr 0 type 'Normal' flags b:i:I:c:s:I:e:c
kworker/u8:2-349 [003] d..4 155.851991: xhci_inc_enq: STREAM
ffff000177f86f80: enq 0x00000000be0eb060(0x00000000be0eb000) deq 0x00000000be0eb050(0x00000000be0eb000) segs 2 stream 1 free_trbs 508 bounce 1024 cycle 1

Quoted 2 lines

Do you have any ideas?

From initial observation of your issue it seems to be unrelated with the changes in my patch. But could you please provide, USB analyzer trace logs for the working and non-working case? Also it would be helpful if you can provide device details which you are testing.

Quoted 2 lines

thanks
Li Jun

Thanks & Regards,
Tejas Joglekar