Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC
@ 2020-05-27 10:40 Tejas Joglekar
  2020-05-27 10:40 ` [PATCH v3 1/4] dt-bindings: usb: Add documentation for SG trb cache size quirk Tejas Joglekar
                   ` (5 more replies)
  0 siblings, 6 replies; 22+ messages in thread
From: Tejas Joglekar @ 2020-05-27 10:40 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman, Tejas Joglekar, linux-usb,
	devicetree, Rob Herring, Mathias Nyman
  Cc: John Youn

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

-- 
2.11.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 1/4] dt-bindings: usb: Add documentation for SG trb cache size quirk
  2020-05-27 10:40 [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC Tejas Joglekar
@ 2020-05-27 10:40 ` Tejas Joglekar
  2020-05-29 18:05   ` Rob Herring
  2020-05-27 10:41 ` [PATCH v3 2/4] usb: xhci: Set quirk for XHCI_SG_TRB_CACHE_SIZE_QUIRK Tejas Joglekar
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 22+ messages in thread
From: Tejas Joglekar @ 2020-05-27 10:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Tejas Joglekar, linux-usb, devicetree, Rob Herring
  Cc: John Youn

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 <joglekar@synopsys.com>
---
 Documentation/devicetree/bindings/usb/dwc3.txt     | 4 ++++
 Documentation/devicetree/bindings/usb/usb-xhci.txt | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index d03edf9d3935..0fcbaa51f66e 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -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
diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
index dc025f126d71..c53eb19ae67e 100644
--- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
@@ -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.
-- 
2.11.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 2/4] usb: xhci: Set quirk for XHCI_SG_TRB_CACHE_SIZE_QUIRK
  2020-05-27 10:40 [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC Tejas Joglekar
  2020-05-27 10:40 ` [PATCH v3 1/4] dt-bindings: usb: Add documentation for SG trb cache size quirk Tejas Joglekar
@ 2020-05-27 10:41 ` Tejas Joglekar
  2020-05-27 10:41 ` [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk Tejas Joglekar
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Tejas Joglekar @ 2020-05-27 10:41 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Tejas Joglekar, linux-usb, Mathias Nyman; +Cc: John Youn

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 <joglekar@synopsys.com>
---
 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(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 766b74723e64..05b743a819ac 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -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");
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 1d4f6f85f0fe..a8526fe50811 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -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);
 	}
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 86cfefdd6632..69e3587e805c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -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;
-- 
2.11.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk
  2020-05-27 10:40 [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC Tejas Joglekar
  2020-05-27 10:40 ` [PATCH v3 1/4] dt-bindings: usb: Add documentation for SG trb cache size quirk Tejas Joglekar
  2020-05-27 10:41 ` [PATCH v3 2/4] usb: xhci: Set quirk for XHCI_SG_TRB_CACHE_SIZE_QUIRK Tejas Joglekar
@ 2020-05-27 10:41 ` Tejas Joglekar
  2020-07-06  5:07   ` Tejas Joglekar
  2020-05-27 10:42 ` [PATCH v3 4/4] usb: xhci: Use temporary buffer to consolidate SG Tejas Joglekar
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 22+ messages in thread
From: Tejas Joglekar @ 2020-05-27 10:41 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman, Tejas Joglekar, linux-usb; +Cc: John Youn

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 <joglekar@synopsys.com>
---
 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(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 25c686a752b0..bc295477e1bc 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -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",
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 013f42a2b5dc..0dca0dbf4309 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -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;
diff --git a/drivers/usb/dwc3/dwc3-haps.c b/drivers/usb/dwc3/dwc3-haps.c
index 3cecbf169452..9311cbe5f264 100644
--- a/drivers/usb/dwc3/dwc3-haps.c
+++ b/drivers/usb/dwc3/dwc3-haps.c
@@ -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"),
 	{ },
 };
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index bef1c1ac2067..e0089c82728e 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -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.
-- 
2.11.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 4/4] usb: xhci: Use temporary buffer to consolidate SG
  2020-05-27 10:40 [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC Tejas Joglekar
                   ` (2 preceding siblings ...)
  2020-05-27 10:41 ` [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk Tejas Joglekar
@ 2020-05-27 10:42 ` Tejas Joglekar
  2020-06-08  4:32 ` [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC Tejas Joglekar
  2020-07-23 10:35 ` Jun Li
  5 siblings, 0 replies; 22+ messages in thread
From: Tejas Joglekar @ 2020-05-27 10:42 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Tejas Joglekar, linux-usb, Mathias Nyman; +Cc: John Youn

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 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 <joglekar@synopsys.com>
---
 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(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 0fda0c0f4d31..104c9f683375 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -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);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index bee5deccc83d..646a6a542ec7 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1256,6 +1256,116 @@ EXPORT_SYMBOL_GPL(xhci_resume);
 
 /*-------------------------------------------------------------------------*/
 
+static int xhci_map_temp_buffer(struct usb_hcd *hcd, struct urb *urb)
+{
+	void *temp;
+	int ret = 0;
+	unsigned int len;
+	unsigned int buf_len;
+	enum dma_data_direction dir;
+	struct xhci_hcd *xhci;
+
+	xhci = hcd_to_xhci(hcd);
+	dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+	buf_len = urb->transfer_buffer_length;
+
+	temp = kzalloc_node(buf_len, GFP_ATOMIC,
+			    dev_to_node(hcd->self.sysdev));
+
+	if (usb_urb_dir_out(urb))
+		len = sg_pcopy_to_buffer(urb->sg, urb->num_sgs,
+					 temp, buf_len, 0);
+
+	urb->transfer_buffer = temp;
+	urb->transfer_dma = dma_map_single(hcd->self.sysdev,
+					   urb->transfer_buffer,
+					   urb->transfer_buffer_length,
+					   dir);
+
+	if (dma_mapping_error(hcd->self.sysdev,
+			      urb->transfer_dma)) {
+		ret = -EAGAIN;
+		kfree(temp);
+	} else {
+		urb->transfer_flags |= URB_DMA_MAP_SINGLE;
+	}
+
+	return ret;
+}
+
+static bool xhci_urb_temp_buffer_required(struct usb_hcd *hcd,
+					  struct urb *urb)
+{
+	bool ret = false;
+	unsigned int i;
+	unsigned int len = 0;
+	unsigned int buf_len;
+	unsigned int trb_size;
+	unsigned int max_pkt;
+	struct scatterlist *sg;
+	struct scatterlist *tail_sg;
+
+	sg = urb->sg;
+	tail_sg = urb->sg;
+	buf_len = urb->transfer_buffer_length;
+	max_pkt = usb_endpoint_maxp(&urb->ep->desc);
+
+	if (!urb->num_sgs)
+		return ret;
+
+	if (urb->dev->speed >= USB_SPEED_SUPER)
+		trb_size = TRB_CACHE_SIZE_SS;
+	else
+		trb_size = TRB_CACHE_SIZE_HS;
+
+	if (urb->transfer_buffer_length != 0 &&
+	    !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
+		for_each_sg(urb->sg, sg, urb->num_sgs, i) {
+			len = len + sg->length;
+			if (i > trb_size - 2) {
+				len = len - tail_sg->length;
+				if (len < max_pkt) {
+					ret = true;
+					break;
+				}
+
+				tail_sg = sg_next(tail_sg);
+			}
+		}
+	}
+	return ret;
+}
+
+static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb)
+{
+	struct scatterlist *sg;
+	unsigned int len;
+	unsigned int buf_len;
+	enum dma_data_direction dir;
+
+	dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+
+	sg = urb->sg;
+	buf_len = urb->transfer_buffer_length;
+
+	if (IS_ENABLED(CONFIG_HAS_DMA) &&
+	    (urb->transfer_flags & URB_DMA_MAP_SINGLE))
+		dma_unmap_single(hcd->self.sysdev,
+				 urb->transfer_dma,
+				 urb->transfer_buffer_length,
+				 dir);
+
+	if (usb_urb_dir_in(urb))
+		len = sg_pcopy_from_buffer(urb->sg, urb->num_sgs,
+					   urb->transfer_buffer,
+					   buf_len,
+					   0);
+
+	urb->transfer_flags &= ~URB_DMA_MAP_SINGLE;
+	kfree(urb->transfer_buffer);
+	urb->transfer_buffer = NULL;
+}
+
 /*
  * Bypass the DMA mapping if URB is suitable for Immediate Transfer (IDT),
  * we'll copy the actual data into the TRB address register. This is limited to
@@ -1265,12 +1375,36 @@ EXPORT_SYMBOL_GPL(xhci_resume);
 static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 				gfp_t mem_flags)
 {
+	struct xhci_hcd *xhci;
+
+	xhci = hcd_to_xhci(hcd);
+
 	if (xhci_urb_suitable_for_idt(urb))
 		return 0;
 
+	if (xhci->quirks & XHCI_SG_TRB_CACHE_SIZE_QUIRK) {
+		if (xhci_urb_temp_buffer_required(hcd, urb))
+			return xhci_map_temp_buffer(hcd, urb);
+	}
 	return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
 }
 
+static void xhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+{
+	struct xhci_hcd *xhci;
+	bool unmap_temp_buf = false;
+
+	xhci = hcd_to_xhci(hcd);
+
+	if (urb->num_sgs && (urb->transfer_flags & URB_DMA_MAP_SINGLE))
+		unmap_temp_buf = true;
+
+	if ((xhci->quirks & XHCI_SG_TRB_CACHE_SIZE_QUIRK) && unmap_temp_buf)
+		xhci_unmap_temp_buf(hcd, urb);
+	else
+		usb_hcd_unmap_urb_for_dma(hcd, urb);
+}
+
 /**
  * xhci_get_endpoint_index - Used for passing endpoint bitmasks between the core and
  * HCDs.  Find the index for an endpoint given its descriptor.  Use the return
@@ -5315,6 +5449,7 @@ static const struct hc_driver xhci_hc_driver = {
 	 * managing i/o requests and associated device resources
 	 */
 	.map_urb_for_dma =      xhci_map_urb_for_dma,
+	.unmap_urb_for_dma =    xhci_unmap_urb_for_dma,
 	.urb_enqueue =		xhci_urb_enqueue,
 	.urb_dequeue =		xhci_urb_dequeue,
 	.alloc_dev =		xhci_alloc_dev,
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 69e3587e805c..2d3ae699e40c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -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];
 };
-- 
2.11.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 1/4] dt-bindings: usb: Add documentation for SG trb cache size quirk
  2020-05-27 10:40 ` [PATCH v3 1/4] dt-bindings: usb: Add documentation for SG trb cache size quirk Tejas Joglekar
@ 2020-05-29 18:05   ` Rob Herring
  0 siblings, 0 replies; 22+ messages in thread
From: Rob Herring @ 2020-05-29 18:05 UTC (permalink / raw)
  To: Tejas Joglekar; +Cc: Greg Kroah-Hartman, linux-usb, devicetree, John Youn

On Wed, May 27, 2020 at 04:10:55PM +0530, Tejas Joglekar wrote:
> 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 <joglekar@synopsys.com>
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt     | 4 ++++
>  Documentation/devicetree/bindings/usb/usb-xhci.txt | 3 +++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index d03edf9d3935..0fcbaa51f66e 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -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
> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> index dc025f126d71..c53eb19ae67e 100644
> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> @@ -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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC
  2020-05-27 10:40 [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC Tejas Joglekar
                   ` (3 preceding siblings ...)
  2020-05-27 10:42 ` [PATCH v3 4/4] usb: xhci: Use temporary buffer to consolidate SG Tejas Joglekar
@ 2020-06-08  4:32 ` Tejas Joglekar
  2020-06-09  8:57   ` Mathias Nyman
  2020-07-23 10:35 ` Jun Li
  5 siblings, 1 reply; 22+ messages in thread
From: Tejas Joglekar @ 2020-06-08  4:32 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman, linux-usb, devicetree,
	Rob Herring, Mathias Nyman
  Cc: John Youn

Hi Mathias,
   Will this be added to your next branch ?
On 5/27/2020 4:10 PM, Tejas Joglekar wrote:
> 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(-)
> 

Thanks and Regards,
 Tejas Joglekar

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC
  2020-06-08  4:32 ` [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC Tejas Joglekar
@ 2020-06-09  8:57   ` Mathias Nyman
  2020-06-11 18:07     ` Tejas Joglekar
  0 siblings, 1 reply; 22+ messages in thread
From: Mathias Nyman @ 2020-06-09  8:57 UTC (permalink / raw)
  To: Tejas Joglekar, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	devicetree, Rob Herring, Mathias Nyman
  Cc: John Youn

On 8.6.2020 7.32, Tejas Joglekar wrote:
> 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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC
  2020-06-09  8:57   ` Mathias Nyman
@ 2020-06-11 18:07     ` Tejas Joglekar
  2020-06-30  6:28       ` Tejas Joglekar
  0 siblings, 1 reply; 22+ messages in thread
From: Tejas Joglekar @ 2020-06-11 18:07 UTC (permalink / raw)
  To: Mathias Nyman, Tejas Joglekar, Felipe Balbi, Greg Kroah-Hartman,
	linux-usb, devicetree, Rob Herring, Mathias Nyman
  Cc: John Youn

Hi,
On 6/9/2020 2:27 PM, Mathias Nyman wrote:
> On 8.6.2020 7.32, Tejas Joglekar wrote:
>> 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.  
> 
@Felipe: Can you please have a look and see if the current implementation is ok or anything to be changed?

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC
  2020-06-11 18:07     ` Tejas Joglekar
@ 2020-06-30  6:28       ` Tejas Joglekar
  0 siblings, 0 replies; 22+ messages in thread
From: Tejas Joglekar @ 2020-06-30  6:28 UTC (permalink / raw)
  To: Mathias Nyman, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	devicetree, Rob Herring, Mathias Nyman
  Cc: John Youn

Hi Felipe,
On 6/11/2020 11:37 PM, Tejas Joglekar wrote:
> Hi,
> On 6/9/2020 2:27 PM, Mathias Nyman wrote:
>> On 8.6.2020 7.32, Tejas Joglekar wrote:
>>> 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.  
>>
> @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?

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

Thanks & Regards,
  Tejas Joglekar

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk
  2020-05-27 10:41 ` [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk Tejas Joglekar
@ 2020-07-06  5:07   ` Tejas Joglekar
  2020-07-06  6:43     ` Felipe Balbi
  0 siblings, 1 reply; 22+ messages in thread
From: Tejas Joglekar @ 2020-07-06  5:07 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: linux-usb, John Youn

Hi Felipe,
On 5/27/2020 4:11 PM, Tejas Joglekar wrote:
> 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 <joglekar@synopsys.com>
> ---
>  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(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 25c686a752b0..bc295477e1bc 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -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",
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 013f42a2b5dc..0dca0dbf4309 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -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;
> diff --git a/drivers/usb/dwc3/dwc3-haps.c b/drivers/usb/dwc3/dwc3-haps.c
> index 3cecbf169452..9311cbe5f264 100644
> --- a/drivers/usb/dwc3/dwc3-haps.c
> +++ b/drivers/usb/dwc3/dwc3-haps.c
> @@ -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"),
>  	{ },
>  };
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index bef1c1ac2067..e0089c82728e 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -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.
> 
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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk
  2020-07-06  5:07   ` Tejas Joglekar
@ 2020-07-06  6:43     ` Felipe Balbi
  2020-07-07 17:21       ` Tejas Joglekar
                         ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Felipe Balbi @ 2020-07-06  6:43 UTC (permalink / raw)
  To: Tejas Joglekar; +Cc: linux-usb, John Youn


[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]


Hi,

Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
>> @@ -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?

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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk
  2020-07-06  6:43     ` Felipe Balbi
@ 2020-07-07 17:21       ` Tejas Joglekar
  2020-07-12 13:45       ` Tejas Joglekar
  2020-07-15  5:51       ` Tejas Joglekar
  2 siblings, 0 replies; 22+ messages in thread
From: Tejas Joglekar @ 2020-07-07 17:21 UTC (permalink / raw)
  To: Tejas Joglekar, Rob Herring; +Cc: linux-usb, John Youn, balbi

Hello Rob,
On 7/6/2020 12:13 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
>>> @@ -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?
> 
> 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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk
  2020-07-06  6:43     ` Felipe Balbi
  2020-07-07 17:21       ` Tejas Joglekar
@ 2020-07-12 13:45       ` Tejas Joglekar
  2020-07-15  5:51       ` Tejas Joglekar
  2 siblings, 0 replies; 22+ messages in thread
From: Tejas Joglekar @ 2020-07-12 13:45 UTC (permalink / raw)
  To: Tejas Joglekar, Rob Herring, Rob Herring
  Cc: Felipe Balbi, linux-usb, John Youn

Hello Rob,
On 7/6/2020 12:13 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
>>> @@ -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?
> 
> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk
  2020-07-06  6:43     ` Felipe Balbi
  2020-07-07 17:21       ` Tejas Joglekar
  2020-07-12 13:45       ` Tejas Joglekar
@ 2020-07-15  5:51       ` Tejas Joglekar
  2020-07-21  9:47         ` Felipe Balbi
  2 siblings, 1 reply; 22+ messages in thread
From: Tejas Joglekar @ 2020-07-15  5:51 UTC (permalink / raw)
  To: Tejas Joglekar, Rob Herring, Rob Herring
  Cc: Felipe Balbi, linux-usb, John Youn, gregkh, devicetree

Hi Rob,

On 7/6/2020 12:13 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
>>> @@ -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?
> 
> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk
  2020-07-15  5:51       ` Tejas Joglekar
@ 2020-07-21  9:47         ` Felipe Balbi
  2020-07-21 16:57           ` Tejas Joglekar
  0 siblings, 1 reply; 22+ messages in thread
From: Felipe Balbi @ 2020-07-21  9:47 UTC (permalink / raw)
  To: Tejas Joglekar, Tejas Joglekar, Rob Herring, Rob Herring
  Cc: linux-usb, John Youn, gregkh, devicetree


[-- Attachment #1: Type: text/plain, Size: 1934 bytes --]

Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:

> Hi Rob,
>
> On 7/6/2020 12:13 PM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
>>>> @@ -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?
>> 
>> 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?

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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk
  2020-07-21  9:47         ` Felipe Balbi
@ 2020-07-21 16:57           ` Tejas Joglekar
  2020-07-31 10:13             ` Tejas Joglekar
  0 siblings, 1 reply; 22+ messages in thread
From: Tejas Joglekar @ 2020-07-21 16:57 UTC (permalink / raw)
  To: Felipe Balbi, Tejas Joglekar, Rob Herring, Rob Herring
  Cc: linux-usb, John Youn, gregkh, devicetree

Hello,

On 7/21/2020 3:17 PM, Felipe Balbi wrote:
> Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
> 
>> Hi Rob,
>>
>> On 7/6/2020 12:13 PM, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
>>>>> @@ -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?
>>>
>>> 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?
> 
> 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.

> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC
  2020-05-27 10:40 [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC Tejas Joglekar
                   ` (4 preceding siblings ...)
  2020-06-08  4:32 ` [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC Tejas Joglekar
@ 2020-07-23 10:35 ` Jun Li
  2020-07-24  0:15   ` Tejas Joglekar
  5 siblings, 1 reply; 22+ messages in thread
From: Jun Li @ 2020-07-23 10:35 UTC (permalink / raw)
  To: Tejas Joglekar
  Cc: Felipe Balbi, Greg Kroah-Hartman, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Mathias Nyman, John Youn, Li Jun

Tejas Joglekar <Tejas.Joglekar@synopsys.com> 于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.
>
> 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
>
> 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
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC
  2020-07-23 10:35 ` Jun Li
@ 2020-07-24  0:15   ` Tejas Joglekar
  0 siblings, 0 replies; 22+ messages in thread
From: Tejas Joglekar @ 2020-07-24  0:15 UTC (permalink / raw)
  To: Jun Li, Tejas Joglekar
  Cc: Felipe Balbi, Greg Kroah-Hartman, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Mathias Nyman, John Youn, Li Jun, Thinh Nguyen

Hello,
On 7/23/2020 4:05 PM, Jun Li wrote:
> Tejas Joglekar <Tejas.Joglekar@synopsys.com> 于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.
>>
>> 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?
> 
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.

> thanks
> Li Jun

Thanks & Regards,
 Tejas Joglekar


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk
  2020-07-21 16:57           ` Tejas Joglekar
@ 2020-07-31 10:13             ` Tejas Joglekar
  2020-08-04  0:44               ` Thinh Nguyen
  0 siblings, 1 reply; 22+ messages in thread
From: Tejas Joglekar @ 2020-07-31 10:13 UTC (permalink / raw)
  To: Rob Herring, Rob Herring
  Cc: Felipe Balbi, linux-usb, John Youn, gregkh, devicetree

Hello Rob,
On 7/21/2020 10:27 PM, Tejas Joglekar wrote:
> Hello,
> 
> On 7/21/2020 3:17 PM, Felipe Balbi wrote:
>> Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
>>
>>> Hi Rob,
>>>
>>> On 7/6/2020 12:13 PM, Felipe Balbi wrote:
>>>>
>>>> Hi,
>>>>
>>>> Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
>>>>>> @@ -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?
>>>>
>>>> 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?
>>
>> 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.
> 
>> 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
 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk
  2020-07-31 10:13             ` Tejas Joglekar
@ 2020-08-04  0:44               ` Thinh Nguyen
  2020-08-04  1:59                 ` Jun Li
  0 siblings, 1 reply; 22+ messages in thread
From: Thinh Nguyen @ 2020-08-04  0:44 UTC (permalink / raw)
  To: Tejas Joglekar, Rob Herring, Rob Herring
  Cc: Felipe Balbi, linux-usb, John Youn, gregkh, devicetree

Hi Rob,

Tejas Joglekar wrote:
> Hello Rob,
> On 7/21/2020 10:27 PM, Tejas Joglekar wrote:
>> Hello,
>>
>> On 7/21/2020 3:17 PM, Felipe Balbi wrote:
>>> Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
>>>
>>>> Hi Rob,
>>>>
>>>> On 7/6/2020 12:13 PM, Felipe Balbi wrote:
>>>>> Hi,
>>>>>
>>>>> Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
>>>>>>> @@ -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?
>>>>> 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?
>>> 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.
>>
>>> 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
>  
>

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

Thanks,
Thinh

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk
  2020-08-04  0:44               ` Thinh Nguyen
@ 2020-08-04  1:59                 ` Jun Li
  0 siblings, 0 replies; 22+ messages in thread
From: Jun Li @ 2020-08-04  1:59 UTC (permalink / raw)
  To: Thinh Nguyen
  Cc: Tejas Joglekar, Rob Herring, Rob Herring, Felipe Balbi,
	linux-usb, John Youn, gregkh, devicetree

Thinh Nguyen <Thinh.Nguyen@synopsys.com> 于2020年8月4日周二 上午8:46写道:
>
> Hi Rob,
>
> Tejas Joglekar wrote:
> > Hello Rob,
> > On 7/21/2020 10:27 PM, Tejas Joglekar wrote:
> >> Hello,
> >>
> >> On 7/21/2020 3:17 PM, Felipe Balbi wrote:
> >>> Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
> >>>
> >>>> Hi Rob,
> >>>>
> >>>> On 7/6/2020 12:13 PM, Felipe Balbi wrote:
> >>>>> Hi,
> >>>>>
> >>>>> Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes:
> >>>>>>> @@ -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?
> >>>>> 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?
> >>> 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.
> >>
> >>> 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
> >
> >
>
> 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
>
> Thanks,
> Thinh

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, back to index

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 10:40 [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC Tejas Joglekar
2020-05-27 10:40 ` [PATCH v3 1/4] dt-bindings: usb: Add documentation for SG trb cache size quirk Tejas Joglekar
2020-05-29 18:05   ` Rob Herring
2020-05-27 10:41 ` [PATCH v3 2/4] usb: xhci: Set quirk for XHCI_SG_TRB_CACHE_SIZE_QUIRK Tejas Joglekar
2020-05-27 10:41 ` [PATCH v3 3/4] usb: dwc3: Add device property sgl-trb-cache-size-quirk Tejas Joglekar
2020-07-06  5:07   ` Tejas Joglekar
2020-07-06  6:43     ` Felipe Balbi
2020-07-07 17:21       ` Tejas Joglekar
2020-07-12 13:45       ` Tejas Joglekar
2020-07-15  5:51       ` Tejas Joglekar
2020-07-21  9:47         ` Felipe Balbi
2020-07-21 16:57           ` Tejas Joglekar
2020-07-31 10:13             ` Tejas Joglekar
2020-08-04  0:44               ` Thinh Nguyen
2020-08-04  1:59                 ` Jun Li
2020-05-27 10:42 ` [PATCH v3 4/4] usb: xhci: Use temporary buffer to consolidate SG Tejas Joglekar
2020-06-08  4:32 ` [PATCH v3 0/4] Add logic to consolidate TRBs for Synopsys xHC Tejas Joglekar
2020-06-09  8:57   ` Mathias Nyman
2020-06-11 18:07     ` Tejas Joglekar
2020-06-30  6:28       ` Tejas Joglekar
2020-07-23 10:35 ` Jun Li
2020-07-24  0:15   ` Tejas Joglekar

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git