Re-introduce TX FIFO resize for larger EP bursting

Some UDCs may have constraints on how many high bandwidth endpoints it can support in a certain configuration. This API allows for the composite driver to pass down the total number of endpoints to the UDC so it can verify it has the required resources to support the configuration.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/gadget/udc/core.c | 25 +++++++++++++++++++++++++
 include/linux/usb/gadget.h    |  5 +++++
 2 files changed, 30 insertions(+)

drivers/usb/gadget/udc/core.c +0 -0

index b7f0b1e..e33ae2d 100644

Quoted 19 lines · Wesley Cheng

Some UDCs may have constraints on how many high bandwidth endpoints it can support in a certain configuration. This API allows for the composite driver to pass down the total number of endpoints to the UDC so it can verify it has the required resources to support the configuration.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/gadget/udc/core.c | 25 +++++++++++++++++++++++++
 include/linux/usb/gadget.h    |  5 +++++
 2 files changed, 30 insertions(+)

drivers/usb/gadget/udc/core.c +0 -0

index b7f0b1e..e33ae2d 100644
@@ -1003,6 +1003,31 @@ int usb_gadget_ep_match_desc(struct usb_gadget *gadget,
 }
 EXPORT_SYMBOL_GPL(usb_gadget_ep_match_desc);
 

drivers/usb/gadget/udc/core.c int usb_gadget_ep_match_desc(struct usb_gadget *gadget, · line 1009

Quoted 4 lines

drivers/usb/gadget/udc/core.c +4 -0

+/**
+ * usb_gadget_check_config - checks if the UDC can support the number of eps
+ * @gadget: controller to check the USB configuration
+ * @ep_map: bitmap of endpoints being requested by a USB configuration

Will a u64 really hold all of the possible endpoints?

Why make it odd like this, why not just provide a list like we do in the USB core with the structure that USB drivers use? What can a driver do with a bitmap only?

thanks,

greg k-h

Ensure that the USB gadget is able to support the configuration being added based on the number of endpoints required from all interfaces. This is for accounting for any bandwidth or space limitations.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/gadget/configfs.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/usb/gadget/configfs.c +1 -0

index 15a607c..76b9983 100644
@@ -1374,6 +1374,7 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 		struct usb_function *f;
 		struct usb_function *tmp;
 		struct gadget_config_name *cn;
+		unsigned long ep_map = 0;
 
 		if (gadget_is_otg(gadget))
 			c->descriptors = otg_desc;

Quoted 19 lines · Wesley Cheng

Ensure that the USB gadget is able to support the configuration being added based on the number of endpoints required from all interfaces. This is for accounting for any bandwidth or space limitations.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/gadget/configfs.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/usb/gadget/configfs.c +1 -0

index 15a607c..76b9983 100644
@@ -1374,6 +1374,7 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 		struct usb_function *f;
 		struct usb_function *tmp;
 		struct gadget_config_name *cn;
+		unsigned long ep_map = 0;
 

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1411

Quoted 11 lines

drivers/usb/gadget/configfs.c +5 -0

 		if (gadget_is_otg(gadget))
 			c->descriptors = otg_desc;

@@ -1403,7 +1404,28 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 				list_add(&f->list, &cfg->func_list);
 				goto err_purge_funcs;
 			}
+			if (f->fs_descriptors) {
+				struct usb_descriptor_header **d;
+
+				d = f->fs_descriptors;
+				for (; *d; ++d) {

With this check, there really is not a need to check for f->fs_descriptors above in the if statement, right?

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1420

Quoted 9 lines

drivers/usb/gadget/configfs.c +9 -0

+					struct usb_endpoint_descriptor *ep;
+					int addr;
+
+					if ((*d)->bDescriptorType != USB_DT_ENDPOINT)
+						continue;
+
+					ep = (struct usb_endpoint_descriptor *)*d;
+					addr = ((ep->bEndpointAddress & 0x80) >> 3) |
+						(ep->bEndpointAddress & 0x0f);

Don't we have direction macros for this type of check?

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1422

Quoted 2 lines

drivers/usb/gadget/configfs.c +2 -0

+					set_bit(addr, &ep_map);
+				}

What is this loop trying to do? Please document it as I can not figure it out at all.

thanks,

greg k-h

Quoted 20 lines · Greg KH

Quoted 19 lines · Wesley Cheng

Ensure that the USB gadget is able to support the configuration being added based on the number of endpoints required from all interfaces. This is for accounting for any bandwidth or space limitations.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/gadget/configfs.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/usb/gadget/configfs.c +1 -0

index 15a607c..76b9983 100644
@@ -1374,6 +1374,7 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 		struct usb_function *f;
 		struct usb_function *tmp;
 		struct gadget_config_name *cn;
+		unsigned long ep_map = 0;
 

Quoted 11 lines

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1411

Quoted 11 lines

drivers/usb/gadget/configfs.c +5 -0

 		if (gadget_is_otg(gadget))
 			c->descriptors = otg_desc;

@@ -1403,7 +1404,28 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 				list_add(&f->list, &cfg->func_list);
 				goto err_purge_funcs;
 			}
+			if (f->fs_descriptors) {
+				struct usb_descriptor_header **d;
+
+				d = f->fs_descriptors;
+				for (; *d; ++d) {

Hi Greg,

Thanks for the review and feedback.

Quoted 4 lines

With this check, there really is not a need to check for f->fs_descriptors above in the if statement, right?

f->fs_descriptor will carry the table of descriptors that a particular function driver has assigned to it. The for loop here, will dereference the individual descriptors within that descriptor array, so we need to first ensure the descriptor array is present before traversing through the individual entries/elements.

Quoted 12 lines

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1420

Quoted 9 lines

drivers/usb/gadget/configfs.c +9 -0

+					struct usb_endpoint_descriptor *ep;
+					int addr;
+
+					if ((*d)->bDescriptorType != USB_DT_ENDPOINT)
+						continue;
+
+					ep = (struct usb_endpoint_descriptor *)*d;
+					addr = ((ep->bEndpointAddress & 0x80) >> 3) |
+						(ep->bEndpointAddress & 0x0f);

Don't we have direction macros for this type of check?

I don't believe we have a macro which would be able to convert the bEndpointAddress field into the bit which needs to be set, assuming that the 32bit ep_map has the lower 16bits carrying OUT EPs, and the upper 16bits carrying the IN EPs.

Quoted 44 lines · Wesley Cheng

Quoted 20 lines · Greg KH

Quoted 19 lines · Wesley Cheng

Ensure that the USB gadget is able to support the configuration being added based on the number of endpoints required from all interfaces. This is for accounting for any bandwidth or space limitations.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/gadget/configfs.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/usb/gadget/configfs.c +1 -0

index 15a607c..76b9983 100644
@@ -1374,6 +1374,7 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 		struct usb_function *f;
 		struct usb_function *tmp;
 		struct gadget_config_name *cn;
+		unsigned long ep_map = 0;
 

Quoted 11 lines

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1411

Quoted 11 lines

drivers/usb/gadget/configfs.c +5 -0

 		if (gadget_is_otg(gadget))
 			c->descriptors = otg_desc;

@@ -1403,7 +1404,28 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 				list_add(&f->list, &cfg->func_list);
 				goto err_purge_funcs;
 			}
+			if (f->fs_descriptors) {
+				struct usb_descriptor_header **d;
+
+				d = f->fs_descriptors;
+				for (; *d; ++d) {

Hi Greg,

Thanks for the review and feedback.

Quoted 4 lines

With this check, there really is not a need to check for f->fs_descriptors above in the if statement, right?

Quoted 5 lines

f->fs_descriptor will carry the table of descriptors that a particular function driver has assigned to it. The for loop here, will dereference the individual descriptors within that descriptor array, so we need to first ensure the descriptor array is present before traversing through the individual entries/elements.

Ah, it's a dereference of an array element. Subtle. Tricky. Messy :(

Quoted 17 lines

Quoted 12 lines

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1420

Quoted 9 lines

drivers/usb/gadget/configfs.c +9 -0

+					struct usb_endpoint_descriptor *ep;
+					int addr;
+
+					if ((*d)->bDescriptorType != USB_DT_ENDPOINT)
+						continue;
+
+					ep = (struct usb_endpoint_descriptor *)*d;
+					addr = ((ep->bEndpointAddress & 0x80) >> 3) |
+						(ep->bEndpointAddress & 0x0f);

Don't we have direction macros for this type of check?

I don't believe we have a macro which would be able to convert the bEndpointAddress field into the bit which needs to be set, assuming that the 32bit ep_map has the lower 16bits carrying OUT EPs, and the upper 16bits carrying the IN EPs.

We have macros to tell if an endpoint is IN or OUT, please use those.

And this "cram the whole thing into 64 bits" is not obvious at all. Just pass around the original pointer to the descriptors if someone wants to use it or not, don't make up yet-another-data-structure here for no good reason. We aren't so memory constrained we need to pack stuff into bits here.

thanks,

greg k-h

Quoted 45 lines · Greg KH

Quoted 44 lines · Wesley Cheng

Quoted 20 lines · Greg KH

Quoted 19 lines · Wesley Cheng

Ensure that the USB gadget is able to support the configuration being added based on the number of endpoints required from all interfaces. This is for accounting for any bandwidth or space limitations.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/gadget/configfs.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/usb/gadget/configfs.c +1 -0

index 15a607c..76b9983 100644
@@ -1374,6 +1374,7 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 		struct usb_function *f;
 		struct usb_function *tmp;
 		struct gadget_config_name *cn;
+		unsigned long ep_map = 0;
 

Quoted 11 lines

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1411

Quoted 11 lines

drivers/usb/gadget/configfs.c +5 -0

 		if (gadget_is_otg(gadget))
 			c->descriptors = otg_desc;

@@ -1403,7 +1404,28 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 				list_add(&f->list, &cfg->func_list);
 				goto err_purge_funcs;
 			}
+			if (f->fs_descriptors) {
+				struct usb_descriptor_header **d;
+
+				d = f->fs_descriptors;
+				for (; *d; ++d) {

Hi Greg,

Thanks for the review and feedback.

Quoted 4 lines

With this check, there really is not a need to check for f->fs_descriptors above in the if statement, right?

Quoted 25 lines

Quoted 5 lines

f->fs_descriptor will carry the table of descriptors that a particular function driver has assigned to it. The for loop here, will dereference the individual descriptors within that descriptor array, so we need to first ensure the descriptor array is present before traversing through the individual entries/elements.

Ah, it's a dereference of an array element. Subtle. Tricky. Messy :(

Quoted 17 lines

Quoted 12 lines

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1420

Quoted 9 lines

drivers/usb/gadget/configfs.c +9 -0

+					struct usb_endpoint_descriptor *ep;
+					int addr;
+
+					if ((*d)->bDescriptorType != USB_DT_ENDPOINT)
+						continue;
+
+					ep = (struct usb_endpoint_descriptor *)*d;
+					addr = ((ep->bEndpointAddress & 0x80) >> 3) |
+						(ep->bEndpointAddress & 0x0f);

Don't we have direction macros for this type of check?

I don't believe we have a macro which would be able to convert the bEndpointAddress field into the bit which needs to be set, assuming that the 32bit ep_map has the lower 16bits carrying OUT EPs, and the upper 16bits carrying the IN EPs.

Hi Greg,

Quoted 9 lines

We have macros to tell if an endpoint is IN or OUT, please use those.

And this "cram the whole thing into 64 bits" is not obvious at all. Just pass around the original pointer to the descriptors if someone wants to use it or not, don't make up yet-another-data-structure here for no good reason. We aren't so memory constrained we need to pack stuff into bits here.

Hmm ok, what I can do is to move this logic into the check_config() callback itself, which is implemented by the UDC driver. So now, the DWC3 will have to do something similar to what is done here, ie loop the EP descriptors for each function to determine the number of IN endpoints being used.

Quoted 86 lines · Wesley Cheng

Quoted 45 lines · Greg KH

Quoted 44 lines · Wesley Cheng

Quoted 20 lines · Greg KH

Quoted 19 lines · Wesley Cheng

Ensure that the USB gadget is able to support the configuration being added based on the number of endpoints required from all interfaces. This is for accounting for any bandwidth or space limitations.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/gadget/configfs.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/usb/gadget/configfs.c +1 -0

index 15a607c..76b9983 100644
@@ -1374,6 +1374,7 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 		struct usb_function *f;
 		struct usb_function *tmp;
 		struct gadget_config_name *cn;
+		unsigned long ep_map = 0;
 

Quoted 11 lines

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1411

Quoted 11 lines

drivers/usb/gadget/configfs.c +5 -0

 		if (gadget_is_otg(gadget))
 			c->descriptors = otg_desc;

@@ -1403,7 +1404,28 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 				list_add(&f->list, &cfg->func_list);
 				goto err_purge_funcs;
 			}
+			if (f->fs_descriptors) {
+				struct usb_descriptor_header **d;
+
+				d = f->fs_descriptors;
+				for (; *d; ++d) {

Hi Greg,

Thanks for the review and feedback.

Quoted 4 lines

With this check, there really is not a need to check for f->fs_descriptors above in the if statement, right?

Quoted 25 lines

Quoted 5 lines

f->fs_descriptor will carry the table of descriptors that a particular function driver has assigned to it. The for loop here, will dereference the individual descriptors within that descriptor array, so we need to first ensure the descriptor array is present before traversing through the individual entries/elements.

Ah, it's a dereference of an array element. Subtle. Tricky. Messy :(

Quoted 17 lines

Quoted 12 lines

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1420

Quoted 9 lines

drivers/usb/gadget/configfs.c +9 -0

+					struct usb_endpoint_descriptor *ep;
+					int addr;
+
+					if ((*d)->bDescriptorType != USB_DT_ENDPOINT)
+						continue;
+
+					ep = (struct usb_endpoint_descriptor *)*d;
+					addr = ((ep->bEndpointAddress & 0x80) >> 3) |
+						(ep->bEndpointAddress & 0x0f);

Don't we have direction macros for this type of check?

I don't believe we have a macro which would be able to convert the bEndpointAddress field into the bit which needs to be set, assuming that the 32bit ep_map has the lower 16bits carrying OUT EPs, and the upper 16bits carrying the IN EPs.

Hi Greg,

Quoted 9 lines

We have macros to tell if an endpoint is IN or OUT, please use those.

And this "cram the whole thing into 64 bits" is not obvious at all. Just pass around the original pointer to the descriptors if someone wants to use it or not, don't make up yet-another-data-structure here for no good reason. We aren't so memory constrained we need to pack stuff into bits here.

Quoted 5 lines

Hmm ok, what I can do is to move this logic into the check_config() callback itself, which is implemented by the UDC driver. So now, the DWC3 will have to do something similar to what is done here, ie loop the EP descriptors for each function to determine the number of IN endpoints being used.

We have common USB core functions for this, why can't you just use them?

Please do not take data that we already have in one format, and convert it to another one just for a single driver to consume. That's pointless.

thanks,

greg k-h

Quoted 87 lines · Greg KH

Quoted 86 lines · Wesley Cheng

Quoted 45 lines · Greg KH

Quoted 44 lines · Wesley Cheng

Quoted 20 lines · Greg KH

Quoted 19 lines · Wesley Cheng

Ensure that the USB gadget is able to support the configuration being added based on the number of endpoints required from all interfaces. This is for accounting for any bandwidth or space limitations.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/gadget/configfs.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/usb/gadget/configfs.c +1 -0

index 15a607c..76b9983 100644
@@ -1374,6 +1374,7 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 		struct usb_function *f;
 		struct usb_function *tmp;
 		struct gadget_config_name *cn;
+		unsigned long ep_map = 0;
 

Quoted 11 lines

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1411

Quoted 11 lines

drivers/usb/gadget/configfs.c +5 -0

 		if (gadget_is_otg(gadget))
 			c->descriptors = otg_desc;

@@ -1403,7 +1404,28 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 				list_add(&f->list, &cfg->func_list);
 				goto err_purge_funcs;
 			}
+			if (f->fs_descriptors) {
+				struct usb_descriptor_header **d;
+
+				d = f->fs_descriptors;
+				for (; *d; ++d) {

Hi Greg,

Thanks for the review and feedback.

Quoted 4 lines

With this check, there really is not a need to check for f->fs_descriptors above in the if statement, right?

Quoted 25 lines

Quoted 5 lines

f->fs_descriptor will carry the table of descriptors that a particular function driver has assigned to it. The for loop here, will dereference the individual descriptors within that descriptor array, so we need to first ensure the descriptor array is present before traversing through the individual entries/elements.

Ah, it's a dereference of an array element. Subtle. Tricky. Messy :(

Quoted 17 lines

Quoted 12 lines

drivers/usb/gadget/configfs.c static int configfs_composite_bind(struct usb_gadget *gadget, · line 1420

Quoted 9 lines

drivers/usb/gadget/configfs.c +9 -0

+					struct usb_endpoint_descriptor *ep;
+					int addr;
+
+					if ((*d)->bDescriptorType != USB_DT_ENDPOINT)
+						continue;
+
+					ep = (struct usb_endpoint_descriptor *)*d;
+					addr = ((ep->bEndpointAddress & 0x80) >> 3) |
+						(ep->bEndpointAddress & 0x0f);

Don't we have direction macros for this type of check?

I don't believe we have a macro which would be able to convert the bEndpointAddress field into the bit which needs to be set, assuming that the 32bit ep_map has the lower 16bits carrying OUT EPs, and the upper 16bits carrying the IN EPs.

Hi Greg,

Quoted 9 lines

We have macros to tell if an endpoint is IN or OUT, please use those.

And this "cram the whole thing into 64 bits" is not obvious at all. Just pass around the original pointer to the descriptors if someone wants to use it or not, don't make up yet-another-data-structure here for no good reason. We aren't so memory constrained we need to pack stuff into bits here.

Quoted 5 lines

Quoted 5 lines

Hmm ok, what I can do is to move this logic into the check_config() callback itself, which is implemented by the UDC driver. So now, the DWC3 will have to do something similar to what is done here, ie loop the EP descriptors for each function to determine the number of IN endpoints being used.

Hi Greg,

Quoted 1 line

Quoted 2 lines

We have common USB core functions for this, why can't you just use them?

So, I've tried to use pre-existing mechanisms there in the USB core, but they are not populated correctly at the time of function binding. I will highlight some of the things I've tried, and why they do not work.
If possible, if you could point which core functions can achieve what
we are trying to do here, that would help as well.

- f->endpoints - This is a bitmap which carries the endpoints used by
a particular function driver. This does not work, as this is set during receiving the SET_CONFIG packet. (we need this during the function driver binding stage)

- gadget->in_epnum/gadget->out_epnum - This carries the count of
endpoints used per configuration. This would be perfect, but this count is only incremented when we are not matching EPs using the EP name. So in designs where the EP name is used to match, it can not be used.

- gadget->ep_list - I can use this now in the check_config() to iterate
through the list of eps to see which ones have been claimed for a particular configuration.

So just to re-iterate, the TXFIFO resize logic kicks in when the host sends the SET_CONFIG packet, which is the "end" of USB enumeration. We had discussed a concern previously where, what if we run the resize logic, and there is not enough internal memory. We'd end up with an enumerated device w/ certain functions broken.

This is where the check_config() comes into the picture. It uses the number of endpoints collected during the bind() stage, and checks to make sure the resize logic can at least allocate 1 TXFIFO per endpoint.
If it can not, then it will fail the bind sequence.

drivers/usb/dwc3/gadget.c static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA) · line 2778

Quoted 13 lines · Wesley Cheng

drivers/usb/dwc3/gadget.c +13 -0

+static int dwc3_gadget_check_config(struct usb_gadget *g, unsigned long ep_map)
+{
+	struct dwc3 *dwc = gadget_to_dwc(g);
+	unsigned long in_ep_map;
+	int fifo_size = 0;
+	int ram1_depth;
+	int ep_num;
+
+	if (!dwc->do_fifo_resize)
+		return 0;
+
+	/* Only interested in the IN endpoints */
+	in_ep_map = ep_map >> 16;

Wait, this "map" is split up into 16/16 somehow? So it's only 32bits big?

Where did you document this map structure? Why is it needed at all, you have the gadget, don't you have access to the full list of endpoints here as well?

confused,

greg k-h

Quoted 14 lines · Greg KH

drivers/usb/dwc3/gadget.c static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA) · line 2778

Quoted 13 lines · Wesley Cheng

drivers/usb/dwc3/gadget.c +13 -0

+static int dwc3_gadget_check_config(struct usb_gadget *g, unsigned long ep_map)
+{
+	struct dwc3 *dwc = gadget_to_dwc(g);
+	unsigned long in_ep_map;
+	int fifo_size = 0;
+	int ram1_depth;
+	int ep_num;
+
+	if (!dwc->do_fifo_resize)
+		return 0;
+
+	/* Only interested in the IN endpoints */
+	in_ep_map = ep_map >> 16;

Hi Greg,

Quoted 4 lines

Wait, this "map" is split up into 16/16 somehow? So it's only 32bits big?

Yes, correct. Upper 16 carries IN eps, lower 16 carries OUT eps. Will fix that based off your other comment.

Quoted 6 lines

Where did you document this map structure? Why is it needed at all, you have the gadget, don't you have access to the full list of endpoints here as well?

confused,

Unfortunately, we do not have the entire list of endpoints for each function until the composite driver receives the SET_CONFIG packet from the host. By this time, if we incorrectly allowed the configuration to start enumeration w/ the host, and there were some EPs which have no FIFO memory allocated, that would lead to those interfaces/functions being broken.

In order to take advantage of the TX fifo resizing logic, manually add these properties to the DWC3 child node by default. This will allow the DWC3 gadget to resize the TX fifos for the IN endpoints, which help with performance.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/dwc3/dwc3-qcom.c | 9 +++++++++
 1 file changed, 9 insertions(+)

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

index 49e6ca9..44e0eaa1 100644
@@ -645,6 +645,7 @@ static int dwc3_qcom_of_register_core(struct platform_device *pdev)
 	struct dwc3_qcom	*qcom = platform_get_drvdata(pdev);
 	struct device_node	*np = pdev->dev.of_node, *dwc3_np;
 	struct device		*dev = &pdev->dev;
+	struct property		*prop;
 	int			ret;
 
 	dwc3_np = of_get_compatible_child(np, "snps,dwc3");

Hi Wesley,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on robh/for-next v5.13-rc6 next-20210617] [cannot apply to balbi-usb/testing/next]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Wesley-Cheng/Re-introduce-TX-FIFO-resize-for-larger-EP-bursting/20210617-180037
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: csky-randconfig-s031-20210617 (attached as .config)
compiler: csky-linux-gcc (GCC) 9.3.0

reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.3-341-g8af24329-dirty
# https://github.com/0day-ci/linux/commit/54b6eb3d44464c519a4546e9a2c3041eeced1009
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Wesley-Cheng/Re-introduce-TX-FIFO-resize-for-larger-EP-bursting/20210617-180037 git checkout 54b6eb3d44464c519a4546e9a2c3041eeced1009
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=csky

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>, old ones prefixed by <<):

Quoted 1 line

Quoted 1 line

ERROR: modpost: "of_add_property" [drivers/usb/dwc3/dwc3-qcom.ko] undefined!

Changes in V10:
- Fixed compilation errors in config where OF is not used (error due to unknown symbol for of_add_property()). Add of_add_property() stub.
- Fixed compilation warning for incorrect argument being passed to dwc3_mdwidth

Changes in V9:
- Fixed incorrect patch in series. Removed changes in DTSI, as dwc3-qcom will add the property by default from the kernel.

Changes in V8:
- Rebased to usb-testing
- Using devm_kzalloc for adding txfifo property in dwc3-qcom
- Removed DWC3 QCOM ACPI property for enabling the txfifo resize

Changes in V7:
- Added a new property tx-fifo-max-num for limiting how much fifo space the resizing logic can allocate for endpoints with large burst values. This can differ across platforms, and tie in closely with overall system latency.
- Added recommended checks for DWC32.
- Added changes to set the tx-fifo-resize property from dwc3-qcom by default instead of modifying the current DTSI files.
- Added comments on all APIs/variables introduced.
- Updated the DWC3 YAML to include a better description of the tx-fifo-resize property and added an entry for tx-fifo-max-num.

Changes in V6:
- Rebased patches to usb-testing.
- Renamed to PATCH series instead of RFC.
- Checking for fs_descriptors instead of ss_descriptors for determining the endpoint count for a particular configuration.
- Re-ordered patch series to fix patch dependencies.

Changes in V5:
- Added check_config() logic, which is used to communicate the number of EPs used in a particular configuration. Based on this, the DWC3 gadget driver has the ability to know the maximum number of eps utilized in all configs. This helps reduce unnecessary allocation to unused eps, and will catch fifo allocation issues at bind() time.
- Fixed variable declaration to single line per variable, and reverse xmas.
- Created a helper for fifo clearing, which is used by ep0.c

Changes in V4:
- Removed struct dwc3* as an argument for dwc3_gadget_resize_tx_fifos()
- Removed WARN_ON(1) in case we run out of fifo space

Changes in V3:
- Removed "Reviewed-by" tags
- Renamed series back to RFC
- Modified logic to ensure that fifo_size is reset if we pass the minimum threshold. Tested with binding multiple FDs requesting 6 FIFOs.

Changes in V2:
- Modified TXFIFO resizing logic to ensure that each EP is reserved a FIFO.
- Removed dev_dbg() prints and fixed typos from patches
- Added some more description on the dt-bindings commit message

Currently, there is no functionality to allow for resizing the TXFIFOs, and relying on the HW default setting for the TXFIFO depth. In most cases, the HW default is probably sufficient, but for USB compositions that contain multiple functions that require EP bursting, the default settings might not be enough. Also to note, the current SW will assign an EP to a function driver w/o checking to see if the TXFIFO size for that particular EP is large enough. (this is a problem if there are multiple HW defined values for the TXFIFO size)

It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3 is required for an EP that supports bursting. Otherwise, there may be frequent occurences of bursts ending. For high bandwidth functions, such as data tethering (protocols that support data aggregation), mass storage, and media transfer protocol (over FFS), the bMaxBurst value can be large, and a bigger TXFIFO depth may prove to be beneficial in terms of USB throughput. (which can be associated to system access latency, etc...) It allows for a more consistent burst of traffic, w/o any interruptions, as data is readily available in the FIFO.

With testing done using the mass storage function driver, the results show that with a larger TXFIFO depth, the bandwidth increased significantly.

Test Parameters:
- Platform: Qualcomm SM8150
- bMaxBurst = 6
- USB req size = 256kB
- Num of USB reqs = 16
- USB Speed = Super-Speed
- Function Driver: Mass Storage (w/ ramdisk)
- Test Application: CrystalDiskMark

Results:

TXFIFO Depth = 3 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------

Sequential|1 GB x     |
Read      |9 loops    | 193.60
	  |           | 195.86
           |           | 184.77
           |           | 193.60

-------------------------------------------

TXFIFO Depth = 6 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------

Sequential|1 GB x     |
Read      |9 loops    | 287.35
	  |           | 304.94
           |           | 289.64
           |           | 293.61

-------------------------------------------

Wesley Cheng (6):
usb: gadget: udc: core: Introduce check_config to verify USB
configuration
usb: gadget: configfs: Check USB configuration before adding
usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
of: Add stub for of_add_property()
usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default
dt-bindings: usb: dwc3: Update dwc3 TX fifo properties

  .../devicetree/bindings/usb/snps,dwc3.yaml         |  15 +-
  drivers/usb/dwc3/core.c                            |   9 +
  drivers/usb/dwc3/core.h                            |  15 ++
  drivers/usb/dwc3/dwc3-qcom.c                       |   9 +
  drivers/usb/dwc3/ep0.c                             |   2 +
  drivers/usb/dwc3/gadget.c                          | 212 +++++++++++++++++++++
  drivers/usb/gadget/configfs.c                      |  22 +++
  drivers/usb/gadget/udc/core.c                      |  25 +++
  include/linux/of.h                                 |   5 +
  include/linux/usb/gadget.h                         |   5 +
  10 files changed, 317 insertions(+), 2 deletions(-)

Hi

Op 17-06-2021 om 11:58 schreef Wesley Cheng:

Quoted 5 lines

Changes in V10:
  - Fixed compilation errors in config where OF is not used (error due to unknown symbol for of_add_property()).  Add of_add_property() stub.
  - Fixed compilation warning for incorrect argument being passed to dwc3_mdwidth

This fixes the OOPS I had in V9. I do not see any change in performance on Merrifield though.

Quoted 148 lines

Changes in V9:
  - Fixed incorrect patch in series.  Removed changes in DTSI, as dwc3-qcom will
    add the property by default from the kernel.

Changes in V8:
  - Rebased to usb-testing
  - Using devm_kzalloc for adding txfifo property in dwc3-qcom
  - Removed DWC3 QCOM ACPI property for enabling the txfifo resize

Changes in V7:
  - Added a new property tx-fifo-max-num for limiting how much fifo space the
    resizing logic can allocate for endpoints with large burst values.  This
    can differ across platforms, and tie in closely with overall system latency.
  - Added recommended checks for DWC32.
  - Added changes to set the tx-fifo-resize property from dwc3-qcom by default
    instead of modifying the current DTSI files.
  - Added comments on all APIs/variables introduced.
  - Updated the DWC3 YAML to include a better description of the tx-fifo-resize
    property and added an entry for tx-fifo-max-num.

Changes in V6:
  - Rebased patches to usb-testing.
  - Renamed to PATCH series instead of RFC.
  - Checking for fs_descriptors instead of ss_descriptors for determining the
    endpoint count for a particular configuration.
  - Re-ordered patch series to fix patch dependencies.

Changes in V5:
  - Added check_config() logic, which is used to communicate the number of EPs
    used in a particular configuration.  Based on this, the DWC3 gadget driver
    has the ability to know the maximum number of eps utilized in all configs.
    This helps reduce unnecessary allocation to unused eps, and will catch fifo
    allocation issues at bind() time.
  - Fixed variable declaration to single line per variable, and reverse xmas.
  - Created a helper for fifo clearing, which is used by ep0.c

Changes in V4:
  - Removed struct dwc3* as an argument for dwc3_gadget_resize_tx_fifos()
  - Removed WARN_ON(1) in case we run out of fifo space Changes in V3:
  - Removed "Reviewed-by" tags
  - Renamed series back to RFC
  - Modified logic to ensure that fifo_size is reset if we pass the minimum
    threshold.  Tested with binding multiple FDs requesting 6 FIFOs.

Changes in V2:
  - Modified TXFIFO resizing logic to ensure that each EP is reserved a FIFO.
  - Removed dev_dbg() prints and fixed typos from patches
  - Added some more description on the dt-bindings commit message

Currently, there is no functionality to allow for resizing the TXFIFOs, and
relying on the HW default setting for the TXFIFO depth.  In most cases, the
HW default is probably sufficient, but for USB compositions that contain multiple functions that require EP bursting, the default settings might not be enough.  Also to note, the current SW will assign an EP to a function driver w/o checking to see if the TXFIFO size for that particular
EP is large enough. (this is a problem if there are multiple HW defined values for the TXFIFO size)

It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3 is required for an EP that supports bursting.  Otherwise, there may be frequent occurences of bursts ending.  For high bandwidth functions, such as data tethering (protocols that support data aggregation), mass storage, and media transfer protocol (over FFS), the bMaxBurst value can be
large, and a bigger TXFIFO depth may prove to be beneficial in terms of USB
throughput. (which can be associated to system access latency, etc...)  It
allows for a more consistent burst of traffic, w/o any interruptions, as data is readily available in the FIFO.

With testing done using the mass storage function driver, the results show
that with a larger TXFIFO depth, the bandwidth increased significantly.

Test Parameters:
  - Platform: Qualcomm SM8150
  - bMaxBurst = 6
  - USB req size = 256kB
  - Num of USB reqs = 16
  - USB Speed = Super-Speed
  - Function Driver: Mass Storage (w/ ramdisk)
  - Test Application: CrystalDiskMark

Results:

TXFIFO Depth = 3 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 193.60

      |           | 195.86
           |           | 184.77
           |           | 193.60

-------------------------------------------

TXFIFO Depth = 6 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 287.35

      |           | 304.94
           |           | 289.64
           |           | 293.61

-------------------------------------------

Wesley Cheng (6):
   usb: gadget: udc: core: Introduce check_config to verify USB configuration
   usb: gadget: configfs: Check USB configuration before adding
   usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
   of: Add stub for of_add_property()
   usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default
   dt-bindings: usb: dwc3: Update dwc3 TX fifo properties

  .../devicetree/bindings/usb/snps,dwc3.yaml         |  15 +- drivers/usb/dwc3/core.c                            |   9 + drivers/usb/dwc3/core.h                            |  15 ++ drivers/usb/dwc3/dwc3-qcom.c                       |   9 + drivers/usb/dwc3/ep0.c                             |   2 + drivers/usb/dwc3/gadget.c                          | 212
+++++++++++++++++++++
  drivers/usb/gadget/configfs.c                      |  22 +++ drivers/usb/gadget/udc/core.c                      |  25 +++ include/linux/of.h                                 |   5 + include/linux/usb/gadget.h                         |   5 +

  10 files changed, 317 insertions(+), 2 deletions(-)

Hi

Op 17-06-2021 om 23:48 schreef Wesley Cheng:

Quoted 11 lines

Hi,

Quoted 8 lines · Ferry Toth

Hi

Op 17-06-2021 om 11:58 schreef Wesley Cheng:

Quoted 5 lines

Changes in V10:
  - Fixed compilation errors in config where OF is not used (error due to unknown symbol for of_add_property()).  Add of_add_property() stub.
  - Fixed compilation warning for incorrect argument being passed to dwc3_mdwidth

Quoted 4 lines

Quoted 2 lines

This fixes the OOPS I had in V9. I do not see any change in performance on Merrifield though.

I see...thanks Ferry! With your testing, are you writing to the device's internal storage (ie UFS, eMMC, etc...), or did you use a ramdisk as well?

In this case I just tested the EEM path using iperf3.

Quoted 6 lines

If not with a ramdisk, we might want to give that a try to avoid the storage path being the bottleneck. You can use "dd" to create an empty file, and then just use that as the LUN's backing file.

echo ramdisk.img > /sys/kernel/config/usb_gadget/g1/functions/mass_storage.0/lun.0/file

Ah, why didn't I think of that. I have currently mass storage setup with eMMC but it seems that is indeed the bottleneck.

I'll try with a ramdisk and let you know.

Quoted 151 lines

Thanks
Wesley Cheng

Quoted 148 lines

Quoted 148 lines

Changes in V9:
  - Fixed incorrect patch in series.  Removed changes in DTSI, as dwc3-qcom will
    add the property by default from the kernel.

Changes in V8:
  - Rebased to usb-testing
  - Using devm_kzalloc for adding txfifo property in dwc3-qcom
  - Removed DWC3 QCOM ACPI property for enabling the txfifo resize

Changes in V7:
  - Added a new property tx-fifo-max-num for limiting how much fifo space the
    resizing logic can allocate for endpoints with large burst values.  This
    can differ across platforms, and tie in closely with overall system latency.
  - Added recommended checks for DWC32.
  - Added changes to set the tx-fifo-resize property from dwc3-qcom by default
    instead of modifying the current DTSI files.
  - Added comments on all APIs/variables introduced.
  - Updated the DWC3 YAML to include a better description of the tx-fifo-resize
    property and added an entry for tx-fifo-max-num.

Changes in V6:
  - Rebased patches to usb-testing.
  - Renamed to PATCH series instead of RFC.
  - Checking for fs_descriptors instead of ss_descriptors for determining the
    endpoint count for a particular configuration.
  - Re-ordered patch series to fix patch dependencies.

Changes in V5:
  - Added check_config() logic, which is used to communicate the number of EPs
    used in a particular configuration.  Based on this, the DWC3 gadget driver
    has the ability to know the maximum number of eps utilized in all configs.
    This helps reduce unnecessary allocation to unused eps, and will catch fifo
    allocation issues at bind() time.
  - Fixed variable declaration to single line per variable, and reverse xmas.
  - Created a helper for fifo clearing, which is used by ep0.c

Changes in V4:
  - Removed struct dwc3* as an argument for dwc3_gadget_resize_tx_fifos()
  - Removed WARN_ON(1) in case we run out of fifo space Changes in V3:
  - Removed "Reviewed-by" tags
  - Renamed series back to RFC
  - Modified logic to ensure that fifo_size is reset if we pass the minimum
    threshold.  Tested with binding multiple FDs requesting 6 FIFOs.

Changes in V2:
  - Modified TXFIFO resizing logic to ensure that each EP is reserved a FIFO.
  - Removed dev_dbg() prints and fixed typos from patches
  - Added some more description on the dt-bindings commit message

Currently, there is no functionality to allow for resizing the TXFIFOs, and
relying on the HW default setting for the TXFIFO depth.  In most cases, the
HW default is probably sufficient, but for USB compositions that contain multiple functions that require EP bursting, the default settings might not be enough.  Also to note, the current SW will assign an EP to a function driver w/o checking to see if the TXFIFO size for that particular
EP is large enough. (this is a problem if there are multiple HW defined values for the TXFIFO size)

It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3 is required for an EP that supports bursting.  Otherwise, there may be frequent occurences of bursts ending.  For high bandwidth functions, such as data tethering (protocols that support data aggregation), mass storage, and media transfer protocol (over FFS), the bMaxBurst value can be
large, and a bigger TXFIFO depth may prove to be beneficial in terms of USB
throughput. (which can be associated to system access latency, etc...)  It
allows for a more consistent burst of traffic, w/o any interruptions, as data is readily available in the FIFO.

With testing done using the mass storage function driver, the results show
that with a larger TXFIFO depth, the bandwidth increased significantly.

Test Parameters:
  - Platform: Qualcomm SM8150
  - bMaxBurst = 6
  - USB req size = 256kB
  - Num of USB reqs = 16
  - USB Speed = Super-Speed
  - Function Driver: Mass Storage (w/ ramdisk)
  - Test Application: CrystalDiskMark

Results:

TXFIFO Depth = 3 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 193.60

      |           | 195.86
           |           | 184.77
           |           | 193.60

-------------------------------------------

TXFIFO Depth = 6 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 287.35

      |           | 304.94
           |           | 289.64
           |           | 293.61

-------------------------------------------

Wesley Cheng (6):
   usb: gadget: udc: core: Introduce check_config to verify USB configuration
   usb: gadget: configfs: Check USB configuration before adding
   usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
   of: Add stub for of_add_property()
   usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default
   dt-bindings: usb: dwc3: Update dwc3 TX fifo properties

  .../devicetree/bindings/usb/snps,dwc3.yaml         |  15 +- drivers/usb/dwc3/core.c                            |   9 + drivers/usb/dwc3/core.h                            |  15 ++ drivers/usb/dwc3/dwc3-qcom.c                       |   9 + drivers/usb/dwc3/ep0.c                             |   2 + drivers/usb/dwc3/gadget.c                          | 212
+++++++++++++++++++++
  drivers/usb/gadget/configfs.c                      |  22 +++ drivers/usb/gadget/udc/core.c                      |  25 +++ include/linux/of.h                                 |   5 + include/linux/usb/gadget.h                         |   5 +

  10 files changed, 317 insertions(+), 2 deletions(-)

Hi,

Quoted 14 lines · Ferry Toth

Hi

Op 17-06-2021 om 23:48 schreef Wesley Cheng:

Quoted 11 lines

Hi,

Quoted 8 lines · Ferry Toth

Hi

Op 17-06-2021 om 11:58 schreef Wesley Cheng:

Quoted 5 lines

Changes in V10:
  - Fixed compilation errors in config where OF is not used (error due to unknown symbol for of_add_property()).  Add of_add_property() stub.
  - Fixed compilation warning for incorrect argument being passed to
dwc3_mdwidth

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

This fixes the OOPS I had in V9. I do not see any change in performance on Merrifield though.

I see...thanks Ferry! With your testing, are you writing to the device's internal storage (ie UFS, eMMC, etc...), or did you use a ramdisk as well?

In this case I just tested the EEM path using iperf3.

Got it. I don't believe f_eem will use a high enough (if at all) bMaxBurst value to change the TXFIFO size.

Quoted 9 lines

Quoted 6 lines

If not with a ramdisk, we might want to give that a try to avoid the storage path being the bottleneck. You can use "dd" to create an empty file, and then just use that as the LUN's backing file.

echo ramdisk.img > /sys/kernel/config/usb_gadget/g1/functions/mass_storage.0/lun.0/file

Ah, why didn't I think of that. I have currently mass storage setup with eMMC but it seems that is indeed the bottleneck.

:), not a problem...I've been working on getting the ideal set up for the performance profiling for awhile, so anything I can do to make sure we get some good results.

Quoted 2 lines

I'll try with a ramdisk and let you know.

Thanks again for the testing, Ferry.

Thanks
Wesley Cheng

Quoted 151 lines

Quoted 151 lines

Thanks
Wesley Cheng

Quoted 148 lines

Quoted 148 lines

Changes in V9:
  - Fixed incorrect patch in series.  Removed changes in DTSI, as
dwc3-qcom will
    add the property by default from the kernel.

Changes in V8:
  - Rebased to usb-testing
  - Using devm_kzalloc for adding txfifo property in dwc3-qcom
  - Removed DWC3 QCOM ACPI property for enabling the txfifo resize

Changes in V7:
  - Added a new property tx-fifo-max-num for limiting how much fifo
space the
    resizing logic can allocate for endpoints with large burst
values.  This
    can differ across platforms, and tie in closely with overall
system latency.
  - Added recommended checks for DWC32.
  - Added changes to set the tx-fifo-resize property from dwc3-qcom by
default
    instead of modifying the current DTSI files.
  - Added comments on all APIs/variables introduced.
  - Updated the DWC3 YAML to include a better description of the
tx-fifo-resize
    property and added an entry for tx-fifo-max-num.

Changes in V6:
  - Rebased patches to usb-testing.
  - Renamed to PATCH series instead of RFC.
  - Checking for fs_descriptors instead of ss_descriptors for
determining the
    endpoint count for a particular configuration.
  - Re-ordered patch series to fix patch dependencies.

Changes in V5:
  - Added check_config() logic, which is used to communicate the
number of EPs
    used in a particular configuration.  Based on this, the DWC3
gadget driver
    has the ability to know the maximum number of eps utilized in all
configs.
    This helps reduce unnecessary allocation to unused eps, and will
catch fifo
    allocation issues at bind() time.
  - Fixed variable declaration to single line per variable, and
reverse xmas.
  - Created a helper for fifo clearing, which is used by ep0.c

Changes in V4:
  - Removed struct dwc3* as an argument for dwc3_gadget_resize_tx_fifos()
  - Removed WARN_ON(1) in case we run out of fifo space Changes in V3:
  - Removed "Reviewed-by" tags
  - Renamed series back to RFC
  - Modified logic to ensure that fifo_size is reset if we pass the
minimum
    threshold.  Tested with binding multiple FDs requesting 6 FIFOs.

Changes in V2:
  - Modified TXFIFO resizing logic to ensure that each EP is reserved a FIFO.
  - Removed dev_dbg() prints and fixed typos from patches
  - Added some more description on the dt-bindings commit message

Currently, there is no functionality to allow for resizing the TXFIFOs, and
relying on the HW default setting for the TXFIFO depth.  In most cases, the
HW default is probably sufficient, but for USB compositions that contain multiple functions that require EP bursting, the default settings might not be enough.  Also to note, the current SW will assign an EP to a function driver w/o checking to see if the TXFIFO size for that particular
EP is large enough. (this is a problem if there are multiple HW defined values for the TXFIFO size)

It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3 is required for an EP that supports bursting.  Otherwise, there may be frequent occurences of bursts ending.  For high bandwidth functions, such as data tethering (protocols that support data aggregation), mass storage, and media transfer protocol (over FFS), the bMaxBurst value can be
large, and a bigger TXFIFO depth may prove to be beneficial in terms of USB
throughput. (which can be associated to system access latency, etc...)  It
allows for a more consistent burst of traffic, w/o any interruptions, as data is readily available in the FIFO.

With testing done using the mass storage function driver, the results show
that with a larger TXFIFO depth, the bandwidth increased significantly.

Test Parameters:
  - Platform: Qualcomm SM8150
  - bMaxBurst = 6
  - USB req size = 256kB
  - Num of USB reqs = 16
  - USB Speed = Super-Speed
  - Function Driver: Mass Storage (w/ ramdisk)
  - Test Application: CrystalDiskMark

Results:

TXFIFO Depth = 3 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 193.60

       |           | 195.86
            |           | 184.77
            |           | 193.60

-------------------------------------------

TXFIFO Depth = 6 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 287.35

       |           | 304.94
            |           | 289.64
            |           | 293.61

-------------------------------------------

Wesley Cheng (6):
   usb: gadget: udc: core: Introduce check_config to verify USB configuration
   usb: gadget: configfs: Check USB configuration before adding
   usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
   of: Add stub for of_add_property()
   usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default
   dt-bindings: usb: dwc3: Update dwc3 TX fifo properties

   .../devicetree/bindings/usb/snps,dwc3.yaml         |  15 +-
   drivers/usb/dwc3/core.c                            |   9 +
   drivers/usb/dwc3/core.h                            |  15 ++
   drivers/usb/dwc3/dwc3-qcom.c                       |   9 +
   drivers/usb/dwc3/ep0.c                             |   2 +
   drivers/usb/dwc3/gadget.c                          | 212
+++++++++++++++++++++
   drivers/usb/gadget/configfs.c                      |  22 +++
   drivers/usb/gadget/udc/core.c                      |  25 +++
   include/linux/of.h                                 |   5 +
   include/linux/usb/gadget.h                         |   5 +
   10 files changed, 317 insertions(+), 2 deletions(-)

Hi

Op 18-06-2021 om 00:25 schreef Wesley Cheng:

Quoted 22 lines

Hi,

Quoted 19 lines · Ferry Toth

Hi

Op 17-06-2021 om 23:48 schreef Wesley Cheng:

Quoted 16 lines

Hi,

Quoted 13 lines · Ferry Toth

Hi

Op 17-06-2021 om 11:58 schreef Wesley Cheng:

Quoted 7 lines

Changes in V10:
   - Fixed compilation errors in config where OF is not used (error due to
     unknown symbol for of_add_property()).  Add of_add_property() stub.
   - Fixed compilation warning for incorrect argument being passed to dwc3_mdwidth

This fixes the OOPS I had in V9. I do not see any change in performance
on Merrifield though.

Quoted 18 lines

Quoted 6 lines

Quoted 4 lines

I see...thanks Ferry! With your testing, are you writing to the device's
internal storage (ie UFS, eMMC, etc...), or did you use a ramdisk as well?

In this case I just tested the EEM path using iperf3.

Got it.  I don't believe f_eem will use a high enough (if at all) bMaxBurst value to change the TXFIFO size.

Quoted 9 lines

Quoted 6 lines

If not with a ramdisk, we might want to give that a try to avoid the storage path being the bottleneck.  You can use "dd" to create an empty file, and then just use that as the LUN's backing file.

echo ramdisk.img > /sys/kernel/config/usb_gadget/g1/functions/mass_storage.0/lun.0/file

Ah, why didn't I think of that. I have currently mass storage setup with eMMC but it seems that is indeed the bottleneck.

I created a 64MB disk following the instructions here
http://www.linux-usb.org/gadget/file_storage.html (that seems a little outdated, at least I can not start the first partition at sector 8, but minimum 2048), and added a test file on it.

I then copy the file to /dev/shm prior to setting configfs (composite device gser/eem/mass_storage/uac2).

journal shows:

kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)

I don't know what that means, because I see the test file on the ramdisk.

Then I again used gnome disks to benchmark (read/write 10MB):

With V10 on top v5.13.0-rc5:

R/W speed = 35.6/35.8MB/s, access time 0.35ms

With no patches on top v5.12.0:

R/W speed = 35.7/36.1MB/s, access time 0.35ms

I see no speed difference (and it's about the same as with the eMMC backed disk). But the patches are causing a new call trace

kernel: using random self ethernet address
kernel: using random host ethernet address
kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)
kernel: usb0: HOST MAC aa:bb:cc:dd:ee:f2
kernel: usb0: MAC aa:bb:cc:dd:ee:f1
kernel: IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: ------------[ cut here ]------------
kernel: WARNING: CPU: 0 PID: 594 at drivers/usb/gadget/udc/core.c:278 usb_ep_queue+0x75/0x80
kernel: Modules linked in: usb_f_uac2 u_audio usb_f_mass_storage usb_f_eem u_ether usb_f_serial u_serial libcomposite rfcomm iptable_nat bnep snd_sof_nocodec spi_pxa2>
kernel: CPU: 0 PID: 594 Comm: irq/14-dwc3 Not tainted 5.13.0-rc5-edison-acpi-standard #1
kernel: Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
kernel: RIP: 0010:usb_ep_queue+0x75/0x80
kernel: Code: 01 73 e4 48 8b 05 fb 63 06 01 48 85 c0 74 12 48 8b 78 08 44 89 e9 4c 89 e2 48 89 ee e8 74 05 00 00 44 89 e8 5d 41 5c 41 5d c3 <0f> 0b 41 bd 94 ff ff ff >
kernel: RSP: 0000:ffff91eec083fc98 EFLAGS: 00010082
kernel: RAX: ffff8af20357d960 RBX: 0000000000000000 RCX: ffff8af202f06400
kernel: RDX: 0000000000000a20 RSI: ffff8af208785780 RDI: ffff8af202e9ae00
kernel: RBP: ffff8af202e9ae00 R08: 00000000000000c0 R09: ffff8af208785780
kernel: R10: 00000000ffffe000 R11: 3fffffffffffffff R12: ffff8af208785780
kernel: R13: 0000000000000000 R14: ffff8af202e9ae00 R15: ffff8af203e26cc0
kernel: FS:  0000000000000000(0000) GS:ffff8af23e200000(0000) knlGS:0000000000000000
kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 000055e2c21f2100 CR3: 0000000003b38000 CR4: 00000000001006f0
kernel: Call Trace:
kernel:  u_audio_start_playback+0x107/0x1a0 [u_audio]
kernel:  composite_setup+0x224/0x1ba0 [libcomposite]
kernel:  ? dwc3_gadget_ep_queue+0xf6/0x1a0
kernel:  ? usb_ep_queue+0x2a/0x80
kernel:  ? configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  dwc3_ep0_interrupt+0x469/0xa80
kernel:  dwc3_thread_interrupt+0x8ee/0xf40
kernel:  ? __wake_up_common_lock+0x85/0xb0
kernel:  ? disable_irq_nosync+0x10/0x10
kernel:  irq_thread_fn+0x1b/0x60
kernel:  irq_thread+0xd6/0x170
kernel:  ? irq_thread_check_affinity+0x70/0x70
kernel:  ? irq_forced_thread_fn+0x70/0x70
kernel:  kthread+0x116/0x130
kernel:  ? kthread_create_worker_on_cpu+0x60/0x60
kernel:  ret_from_fork+0x22/0x30
kernel: ---[ end trace e5b9e28058c53584 ]---
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000036ac129 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in

Removing uac2 from the config makes the call trace go away, but the R/W speed does not change.

Quoted 174 lines

:), not a problem...I've been working on getting the ideal set up for the performance profiling for awhile, so anything I can do to make sure we get some good results.

Quoted 2 lines

I'll try with a ramdisk and let you know.

Thanks again for the testing, Ferry.

Thanks
Wesley Cheng

Quoted 163 lines

Quoted 163 lines

Thanks
Wesley Cheng

Quoted 160 lines

Quoted 160 lines

Changes in V9:
   - Fixed incorrect patch in series.  Removed changes in DTSI, as dwc3-qcom will
     add the property by default from the kernel.

Changes in V8:
   - Rebased to usb-testing
   - Using devm_kzalloc for adding txfifo property in dwc3-qcom
   - Removed DWC3 QCOM ACPI property for enabling the txfifo resize

Changes in V7:
   - Added a new property tx-fifo-max-num for limiting how much fifo space the
     resizing logic can allocate for endpoints with large burst values.  This
     can differ across platforms, and tie in closely with overall system latency.
   - Added recommended checks for DWC32.
   - Added changes to set the tx-fifo-resize property from dwc3-qcom by
default
     instead of modifying the current DTSI files.
   - Added comments on all APIs/variables introduced.
   - Updated the DWC3 YAML to include a better description of the tx-fifo-resize
     property and added an entry for tx-fifo-max-num.

Changes in V6:
   - Rebased patches to usb-testing.
   - Renamed to PATCH series instead of RFC.
   - Checking for fs_descriptors instead of ss_descriptors for determining the
     endpoint count for a particular configuration.
   - Re-ordered patch series to fix patch dependencies.

Changes in V5:
   - Added check_config() logic, which is used to communicate the number of EPs
     used in a particular configuration.  Based on this, the DWC3 gadget driver
     has the ability to know the maximum number of eps utilized in all
configs.
     This helps reduce unnecessary allocation to unused eps, and will catch fifo
     allocation issues at bind() time.
   - Fixed variable declaration to single line per variable, and reverse xmas.
   - Created a helper for fifo clearing, which is used by ep0.c

Changes in V4:
   - Removed struct dwc3* as an argument for dwc3_gadget_resize_tx_fifos()
   - Removed WARN_ON(1) in case we run out of fifo space Changes in V3:
   - Removed "Reviewed-by" tags
   - Renamed series back to RFC
   - Modified logic to ensure that fifo_size is reset if we pass the minimum
     threshold.  Tested with binding multiple FDs requesting 6 FIFOs.

Changes in V2:
   - Modified TXFIFO resizing logic to ensure that each EP is reserved a
     FIFO.
   - Removed dev_dbg() prints and fixed typos from patches
   - Added some more description on the dt-bindings commit message

Currently, there is no functionality to allow for resizing the TXFIFOs, and
relying on the HW default setting for the TXFIFO depth.  In most cases, the
HW default is probably sufficient, but for USB compositions that contain
multiple functions that require EP bursting, the default settings might not be enough.  Also to note, the current SW will assign an EP to a
function driver w/o checking to see if the TXFIFO size for that particular
EP is large enough. (this is a problem if there are multiple HW defined
values for the TXFIFO size)

It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3
is required for an EP that supports bursting.  Otherwise, there may be
frequent occurences of bursts ending.  For high bandwidth functions, such as data tethering (protocols that support data aggregation), mass
storage, and media transfer protocol (over FFS), the bMaxBurst value can be
large, and a bigger TXFIFO depth may prove to be beneficial in terms of USB
throughput. (which can be associated to system access latency, etc...)  It
allows for a more consistent burst of traffic, w/o any interruptions, as
data is readily available in the FIFO.

With testing done using the mass storage function driver, the results show
that with a larger TXFIFO depth, the bandwidth increased significantly.

Test Parameters:
   - Platform: Qualcomm SM8150
   - bMaxBurst = 6
   - USB req size = 256kB
   - Num of USB reqs = 16
   - USB Speed = Super-Speed
   - Function Driver: Mass Storage (w/ ramdisk)
   - Test Application: CrystalDiskMark

Results:

TXFIFO Depth = 3 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 193.60

       |           | 195.86
            |           | 184.77
            |           | 193.60

-------------------------------------------

TXFIFO Depth = 6 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 287.35

       |           | 304.94
            |           | 289.64
            |           | 293.61

-------------------------------------------

Wesley Cheng (6):
    usb: gadget: udc: core: Introduce check_config to verify USB configuration
    usb: gadget: configfs: Check USB configuration before adding
    usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
    of: Add stub for of_add_property()
    usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default
    dt-bindings: usb: dwc3: Update dwc3 TX fifo properties

   .../devicetree/bindings/usb/snps,dwc3.yaml         |  15 +- drivers/usb/dwc3/core.c                            |   9 + drivers/usb/dwc3/core.h                            |  15 ++ drivers/usb/dwc3/dwc3-qcom.c                       |   9 + drivers/usb/dwc3/ep0.c                             |   2 + drivers/usb/dwc3/gadget.c                          | 212
+++++++++++++++++++++
   drivers/usb/gadget/configfs.c                      |  22 +++ drivers/usb/gadget/udc/core.c                      |  25 +++ include/linux/of.h                                 |   5 + include/linux/usb/gadget.h                         |   5 +

   10 files changed, 317 insertions(+), 2 deletions(-)

Quoted 60 lines

Hi

Op 18-06-2021 om 00:25 schreef Wesley Cheng:

Quoted 22 lines

Hi,

Quoted 19 lines · Ferry Toth

Hi

Op 17-06-2021 om 23:48 schreef Wesley Cheng:

Quoted 16 lines

Hi,

Quoted 13 lines · Ferry Toth

Hi

Op 17-06-2021 om 11:58 schreef Wesley Cheng:

Quoted 7 lines

Changes in V10:
   - Fixed compilation errors in config where OF is not used
(error due to
     unknown symbol for of_add_property()).  Add of_add_property()
stub.
   - Fixed compilation warning for incorrect argument being passed to
dwc3_mdwidth

This fixes the OOPS I had in V9. I do not see any change in performance
on Merrifield though.

Quoted 18 lines

Quoted 6 lines

Quoted 4 lines

I see...thanks Ferry! With your testing, are you writing to the device's
internal storage (ie UFS, eMMC, etc...), or did you use a ramdisk as well?

In this case I just tested the EEM path using iperf3.

Got it.  I don't believe f_eem will use a high enough (if at all) bMaxBurst value to change the TXFIFO size.

Quoted 9 lines

Quoted 6 lines

If not with a ramdisk, we might want to give that a try to avoid the storage path being the bottleneck.  You can use "dd" to create an empty file, and then just use that as the LUN's backing file.

echo ramdisk.img > /sys/kernel/config/usb_gadget/g1/functions/mass_storage.0/lun.0/file

Ah, why didn't I think of that. I have currently mass storage setup with eMMC but it seems that is indeed the bottleneck.

I created a 64MB disk following the instructions here
http://www.linux-usb.org/gadget/file_storage.html (that seems a little outdated, at least I can not start the first partition at sector 8, but minimum 2048), and added a test file on it.

I then copy the file to /dev/shm prior to setting configfs (composite device gser/eem/mass_storage/uac2).

journal shows:

kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)

I don't know what that means, because I see the test file on the ramdisk.

Then I again used gnome disks to benchmark (read/write 10MB):

Quoted 7 lines

With V10 on top v5.13.0-rc5:

R/W speed = 35.6/35.8MB/s, access time 0.35ms

With no patches on top v5.12.0:

R/W speed = 35.7/36.1MB/s, access time 0.35ms

Hi Ferry,

Quoted 3 lines

I see no speed difference (and it's about the same as with the eMMC backed disk). But the patches are causing a new call trace

Would you happen to know what DWC3 controller revision the device is using? The callstack print occurs, because it looks like it ran out of internal memory, although there should be logic present for making sure that at least there is enough room for 1 FIFO per endpoint. (possibly the logic/math depends on the controller revision)

Also, is there a way to use just a mass storage only composition? Based on the above observation, that probably means that the mass storage interface wasn't resized at all, because the configuration took up a lot of the internal FIFO space.

Thanks
Wesley Cheng

Quoted 258 lines

kernel: using random self ethernet address
kernel: using random host ethernet address
kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)
kernel: usb0: HOST MAC aa:bb:cc:dd:ee:f2
kernel: usb0: MAC aa:bb:cc:dd:ee:f1
kernel: IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: ------------[ cut here ]------------
kernel: WARNING: CPU: 0 PID: 594 at drivers/usb/gadget/udc/core.c:278 usb_ep_queue+0x75/0x80
kernel: Modules linked in: usb_f_uac2 u_audio usb_f_mass_storage usb_f_eem u_ether usb_f_serial u_serial libcomposite rfcomm iptable_nat bnep snd_sof_nocodec spi_pxa2>
kernel: CPU: 0 PID: 594 Comm: irq/14-dwc3 Not tainted 5.13.0-rc5-edison-acpi-standard #1
kernel: Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
kernel: RIP: 0010:usb_ep_queue+0x75/0x80
kernel: Code: 01 73 e4 48 8b 05 fb 63 06 01 48 85 c0 74 12 48 8b 78 08 44 89 e9 4c 89 e2 48 89 ee e8 74 05 00 00 44 89 e8 5d 41 5c 41 5d c3 <0f> 0b 41 bd 94 ff ff ff >
kernel: RSP: 0000:ffff91eec083fc98 EFLAGS: 00010082
kernel: RAX: ffff8af20357d960 RBX: 0000000000000000 RCX: ffff8af202f06400
kernel: RDX: 0000000000000a20 RSI: ffff8af208785780 RDI: ffff8af202e9ae00
kernel: RBP: ffff8af202e9ae00 R08: 00000000000000c0 R09: ffff8af208785780
kernel: R10: 00000000ffffe000 R11: 3fffffffffffffff R12: ffff8af208785780
kernel: R13: 0000000000000000 R14: ffff8af202e9ae00 R15: ffff8af203e26cc0
kernel: FS:  0000000000000000(0000) GS:ffff8af23e200000(0000) knlGS:0000000000000000
kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 000055e2c21f2100 CR3: 0000000003b38000 CR4: 00000000001006f0
kernel: Call Trace:
kernel:  u_audio_start_playback+0x107/0x1a0 [u_audio]
kernel:  composite_setup+0x224/0x1ba0 [libcomposite]
kernel:  ? dwc3_gadget_ep_queue+0xf6/0x1a0
kernel:  ? usb_ep_queue+0x2a/0x80
kernel:  ? configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  dwc3_ep0_interrupt+0x469/0xa80
kernel:  dwc3_thread_interrupt+0x8ee/0xf40
kernel:  ? __wake_up_common_lock+0x85/0xb0
kernel:  ? disable_irq_nosync+0x10/0x10
kernel:  irq_thread_fn+0x1b/0x60
kernel:  irq_thread+0xd6/0x170
kernel:  ? irq_thread_check_affinity+0x70/0x70
kernel:  ? irq_forced_thread_fn+0x70/0x70
kernel:  kthread+0x116/0x130
kernel:  ? kthread_create_worker_on_cpu+0x60/0x60
kernel:  ret_from_fork+0x22/0x30
kernel: ---[ end trace e5b9e28058c53584 ]---
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000036ac129 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in

Removing uac2 from the config makes the call trace go away, but the R/W speed does not change.

Quoted 174 lines

:), not a problem...I've been working on getting the ideal set up for the performance profiling for awhile, so anything I can do to make sure we get some good results.

Quoted 2 lines

I'll try with a ramdisk and let you know.

Thanks again for the testing, Ferry.

Thanks
Wesley Cheng

Quoted 163 lines

Quoted 163 lines

Thanks
Wesley Cheng

Quoted 160 lines

Quoted 160 lines

Changes in V9:
   - Fixed incorrect patch in series.  Removed changes in DTSI, as
dwc3-qcom will
     add the property by default from the kernel.

Changes in V8:
   - Rebased to usb-testing
   - Using devm_kzalloc for adding txfifo property in dwc3-qcom
   - Removed DWC3 QCOM ACPI property for enabling the txfifo resize

Changes in V7:
   - Added a new property tx-fifo-max-num for limiting how much fifo
space the
     resizing logic can allocate for endpoints with large burst
values.  This
     can differ across platforms, and tie in closely with overall
system latency.
   - Added recommended checks for DWC32.
   - Added changes to set the tx-fifo-resize property from
dwc3-qcom by
default
     instead of modifying the current DTSI files.
   - Added comments on all APIs/variables introduced.
   - Updated the DWC3 YAML to include a better description of the
tx-fifo-resize
     property and added an entry for tx-fifo-max-num.

Changes in V6:
   - Rebased patches to usb-testing.
   - Renamed to PATCH series instead of RFC.
   - Checking for fs_descriptors instead of ss_descriptors for
determining the
     endpoint count for a particular configuration.
   - Re-ordered patch series to fix patch dependencies.

Changes in V5:
   - Added check_config() logic, which is used to communicate the
number of EPs
     used in a particular configuration.  Based on this, the DWC3
gadget driver
     has the ability to know the maximum number of eps utilized in
all
configs.
     This helps reduce unnecessary allocation to unused eps, and will
catch fifo
     allocation issues at bind() time.
   - Fixed variable declaration to single line per variable, and
reverse xmas.
   - Created a helper for fifo clearing, which is used by ep0.c

Changes in V4:
   - Removed struct dwc3* as an argument for
dwc3_gadget_resize_tx_fifos()
   - Removed WARN_ON(1) in case we run out of fifo space Changes in V3:
   - Removed "Reviewed-by" tags
   - Renamed series back to RFC
   - Modified logic to ensure that fifo_size is reset if we pass the
minimum
     threshold.  Tested with binding multiple FDs requesting 6 FIFOs.

Changes in V2:
   - Modified TXFIFO resizing logic to ensure that each EP is
reserved a
     FIFO.
   - Removed dev_dbg() prints and fixed typos from patches
   - Added some more description on the dt-bindings commit message

Currently, there is no functionality to allow for resizing the TXFIFOs, and
relying on the HW default setting for the TXFIFO depth.  In most cases, the
HW default is probably sufficient, but for USB compositions that contain
multiple functions that require EP bursting, the default settings might not be enough.  Also to note, the current SW will assign an EP to a
function driver w/o checking to see if the TXFIFO size for that particular
EP is large enough. (this is a problem if there are multiple HW defined
values for the TXFIFO size)

It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3
is required for an EP that supports bursting.  Otherwise, there may be
frequent occurences of bursts ending.  For high bandwidth functions, such as data tethering (protocols that support data aggregation), mass
storage, and media transfer protocol (over FFS), the bMaxBurst value can be
large, and a bigger TXFIFO depth may prove to be beneficial in terms of USB
throughput. (which can be associated to system access latency, etc...)  It
allows for a more consistent burst of traffic, w/o any interruptions, as
data is readily available in the FIFO.

With testing done using the mass storage function driver, the results show
that with a larger TXFIFO depth, the bandwidth increased significantly.

Test Parameters:
   - Platform: Qualcomm SM8150
   - bMaxBurst = 6
   - USB req size = 256kB
   - Num of USB reqs = 16
   - USB Speed = Super-Speed
   - Function Driver: Mass Storage (w/ ramdisk)
   - Test Application: CrystalDiskMark

Results:

TXFIFO Depth = 3 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 193.60

        |           | 195.86
             |           | 184.77
             |           | 193.60

-------------------------------------------

TXFIFO Depth = 6 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 287.35

        |           | 304.94
             |           | 289.64
             |           | 293.61

-------------------------------------------

Wesley Cheng (6):
    usb: gadget: udc: core: Introduce check_config to verify USB configuration
    usb: gadget: configfs: Check USB configuration before adding
    usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
    of: Add stub for of_add_property()
    usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default
    dt-bindings: usb: dwc3: Update dwc3 TX fifo properties

    .../devicetree/bindings/usb/snps,dwc3.yaml         |  15 +-
    drivers/usb/dwc3/core.c                            |   9 +
    drivers/usb/dwc3/core.h                            |  15 ++
    drivers/usb/dwc3/dwc3-qcom.c                       |   9 +
    drivers/usb/dwc3/ep0.c                             |   2 +
    drivers/usb/dwc3/gadget.c                          | 212
+++++++++++++++++++++
    drivers/usb/gadget/configfs.c                      |  22 +++
    drivers/usb/gadget/udc/core.c                      |  25 +++
    include/linux/of.h                                 |   5 +
    include/linux/usb/gadget.h                         |   5 +
    10 files changed, 317 insertions(+), 2 deletions(-)

Hi

Op 22-06-2021 om 20:38 schreef Wesley Cheng:

Quoted 83 lines · Ferry Toth

Quoted 64 lines

Hi

Op 18-06-2021 om 00:25 schreef Wesley Cheng:

Quoted 23 lines

Hi,

Quoted 20 lines · Ferry Toth

Hi

Op 17-06-2021 om 23:48 schreef Wesley Cheng:

Quoted 17 lines

Hi,

Quoted 14 lines · Ferry Toth

Hi

Op 17-06-2021 om 11:58 schreef Wesley Cheng:

Quoted 8 lines

Changes in V10:
    - Fixed compilation errors in config where OF is not used (error due to
      unknown symbol for of_add_property()).  Add of_add_property() stub.
    - Fixed compilation warning for incorrect argument being passed to
dwc3_mdwidth

This fixes the OOPS I had in V9. I do not see any change in performance
on Merrifield though.

Quoted 20 lines

Quoted 6 lines

Quoted 4 lines

I see...thanks Ferry! With your testing, are you writing to the device's
internal storage (ie UFS, eMMC, etc...), or did you use a ramdisk as well?

In this case I just tested the EEM path using iperf3.

Got it.  I don't believe f_eem will use a high enough (if at all) bMaxBurst value to change the TXFIFO size.

Quoted 11 lines

Quoted 7 lines

If not with a ramdisk, we might want to give that a try to avoid the storage path being the bottleneck.  You can use "dd" to create an empty
file, and then just use that as the LUN's backing file.

echo ramdisk.img > /sys/kernel/config/usb_gadget/g1/functions/mass_storage.0/lun.0/file

Ah, why didn't I think of that. I have currently mass storage setup with
eMMC but it seems that is indeed the bottleneck.

I created a 64MB disk following the instructions here
http://www.linux-usb.org/gadget/file_storage.html (that seems a little outdated, at least I can not start the first partition at sector 8, but minimum 2048), and added a test file on it.

I then copy the file to /dev/shm prior to setting configfs (composite device gser/eem/mass_storage/uac2).

journal shows:

kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)

I don't know what that means, because I see the test file on the ramdisk.

Then I again used gnome disks to benchmark (read/write 10MB):

Quoted 7 lines

With V10 on top v5.13.0-rc5:

R/W speed = 35.6/35.8MB/s, access time 0.35ms

With no patches on top v5.12.0:

R/W speed = 35.7/36.1MB/s, access time 0.35ms

Hi Ferry,

Quoted 3 lines

I see no speed difference (and it's about the same as with the eMMC backed disk). But the patches are causing a new call trace

Would you happen to know what DWC3 controller revision the device is using?  The callstack print occurs, because it looks like it ran out of internal memory, although there should be logic present for making sure that at least there is enough room for 1 FIFO per endpoint.  (possibly the logic/math depends on the controller revision)

Do you know where I could find that in a file on the device?

Otherwise, I'm hoping Andy will know?

Quoted 5 lines

Also, is there a way to use just a mass storage only composition?  Based on the above observation, that probably means that the mass storage interface wasn't resized at all, because the configuration took up a lot of the internal FIFO space.

Sure, it's configured through configfs. With only mass_storage I have:

With V10 on top v5.13.0-rc5:

R/W speed = 41,6/39,3MB/s, access time 0.33ms

With no patches on top v5.12.0:

R/W speed = 41,1/38,7MB/s, access time 0.38ms

Quoted 287 lines

Thanks
Wesley Cheng

Quoted 284 lines

kernel: using random self ethernet address
kernel: using random host ethernet address
kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)
kernel: usb0: HOST MAC aa:bb:cc:dd:ee:f2
kernel: usb0: MAC aa:bb:cc:dd:ee:f1
kernel: IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: ------------[ cut here ]------------
kernel: WARNING: CPU: 0 PID: 594 at drivers/usb/gadget/udc/core.c:278 usb_ep_queue+0x75/0x80
kernel: Modules linked in: usb_f_uac2 u_audio usb_f_mass_storage usb_f_eem u_ether usb_f_serial u_serial libcomposite rfcomm iptable_nat bnep snd_sof_nocodec spi_pxa2>
kernel: CPU: 0 PID: 594 Comm: irq/14-dwc3 Not tainted 5.13.0-rc5-edison-acpi-standard #1
kernel: Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
kernel: RIP: 0010:usb_ep_queue+0x75/0x80
kernel: Code: 01 73 e4 48 8b 05 fb 63 06 01 48 85 c0 74 12 48 8b 78 08 44 89 e9 4c 89 e2 48 89 ee e8 74 05 00 00 44 89 e8 5d 41 5c 41 5d c3 <0f> 0b 41 bd 94 ff ff ff >
kernel: RSP: 0000:ffff91eec083fc98 EFLAGS: 00010082
kernel: RAX: ffff8af20357d960 RBX: 0000000000000000 RCX:
ffff8af202f06400
kernel: RDX: 0000000000000a20 RSI: ffff8af208785780 RDI:
ffff8af202e9ae00
kernel: RBP: ffff8af202e9ae00 R08: 00000000000000c0 R09:
ffff8af208785780
kernel: R10: 00000000ffffe000 R11: 3fffffffffffffff R12:
ffff8af208785780
kernel: R13: 0000000000000000 R14: ffff8af202e9ae00 R15:
ffff8af203e26cc0
kernel: FS:  0000000000000000(0000) GS:ffff8af23e200000(0000) knlGS:0000000000000000
kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 000055e2c21f2100 CR3: 0000000003b38000 CR4:
00000000001006f0
kernel: Call Trace:
kernel:  u_audio_start_playback+0x107/0x1a0 [u_audio] kernel:  composite_setup+0x224/0x1ba0 [libcomposite]
kernel:  ? dwc3_gadget_ep_queue+0xf6/0x1a0
kernel:  ? usb_ep_queue+0x2a/0x80
kernel:  ? configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  dwc3_ep0_interrupt+0x469/0xa80
kernel:  dwc3_thread_interrupt+0x8ee/0xf40
kernel:  ? __wake_up_common_lock+0x85/0xb0
kernel:  ? disable_irq_nosync+0x10/0x10
kernel:  irq_thread_fn+0x1b/0x60
kernel:  irq_thread+0xd6/0x170
kernel:  ? irq_thread_check_affinity+0x70/0x70
kernel:  ? irq_forced_thread_fn+0x70/0x70
kernel:  kthread+0x116/0x130
kernel:  ? kthread_create_worker_on_cpu+0x60/0x60
kernel:  ret_from_fork+0x22/0x30
kernel: ---[ end trace e5b9e28058c53584 ]---
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000036ac129 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in

Removing uac2 from the config makes the call trace go away, but the R/W speed does not change.

Quoted 184 lines

:), not a problem...I've been working on getting the ideal set up for the performance profiling for awhile, so anything I can do to make sure we get some good results.

Quoted 2 lines

I'll try with a ramdisk and let you know.

Thanks again for the testing, Ferry.

Thanks
Wesley Cheng

Quoted 173 lines

Quoted 173 lines

Thanks
Wesley Cheng

Quoted 170 lines

Quoted 170 lines

Changes in V9:
    - Fixed incorrect patch in series.  Removed changes in DTSI, as dwc3-qcom will
      add the property by default from the kernel.

Changes in V8:
    - Rebased to usb-testing
    - Using devm_kzalloc for adding txfifo property in dwc3-qcom
    - Removed DWC3 QCOM ACPI property for enabling the txfifo resize

Changes in V7:
    - Added a new property tx-fifo-max-num for limiting how much fifo
space the
      resizing logic can allocate for endpoints with large burst values.  This
      can differ across platforms, and tie in closely with overall system latency.
    - Added recommended checks for DWC32.
    - Added changes to set the tx-fifo-resize property from dwc3-qcom by
default
      instead of modifying the current DTSI files.
    - Added comments on all APIs/variables introduced.
    - Updated the DWC3 YAML to include a better description of the tx-fifo-resize
      property and added an entry for tx-fifo-max-num.

Changes in V6:
    - Rebased patches to usb-testing.
    - Renamed to PATCH series instead of RFC.
    - Checking for fs_descriptors instead of ss_descriptors for determining the
      endpoint count for a particular configuration.
    - Re-ordered patch series to fix patch dependencies.

Changes in V5:
    - Added check_config() logic, which is used to communicate the number of EPs
      used in a particular configuration.  Based on this, the DWC3 gadget driver
      has the ability to know the maximum number of eps utilized in all
configs.
      This helps reduce unnecessary allocation to unused eps, and will
catch fifo
      allocation issues at bind() time.
    - Fixed variable declaration to single line per variable, and reverse xmas.
    - Created a helper for fifo clearing, which is used by ep0.c

Changes in V4:
    - Removed struct dwc3* as an argument for dwc3_gadget_resize_tx_fifos()
    - Removed WARN_ON(1) in case we run out of fifo space Changes in V3:
    - Removed "Reviewed-by" tags
    - Renamed series back to RFC
    - Modified logic to ensure that fifo_size is reset if we pass the
minimum
      threshold.  Tested with binding multiple FDs requesting 6 FIFOs.

Changes in V2:
    - Modified TXFIFO resizing logic to ensure that each EP is reserved a
      FIFO.
    - Removed dev_dbg() prints and fixed typos from patches
    - Added some more description on the dt-bindings commit message

Currently, there is no functionality to allow for resizing the TXFIFOs, and
relying on the HW default setting for the TXFIFO depth.  In most cases, the
HW default is probably sufficient, but for USB compositions that contain
multiple functions that require EP bursting, the default settings might not be enough.  Also to note, the current SW will assign an EP to a
function driver w/o checking to see if the TXFIFO size for that particular
EP is large enough. (this is a problem if there are multiple HW defined
values for the TXFIFO size)

It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3
is required for an EP that supports bursting.  Otherwise, there may be
frequent occurences of bursts ending.  For high bandwidth functions,
such as data tethering (protocols that support data aggregation), mass
storage, and media transfer protocol (over FFS), the bMaxBurst value
can be
large, and a bigger TXFIFO depth may prove to be beneficial in terms
of USB
throughput. (which can be associated to system access latency, etc...)  It
allows for a more consistent burst of traffic, w/o any interruptions, as
data is readily available in the FIFO.

With testing done using the mass storage function driver, the results
show
that with a larger TXFIFO depth, the bandwidth increased significantly.

Test Parameters:
    - Platform: Qualcomm SM8150
    - bMaxBurst = 6
    - USB req size = 256kB
    - Num of USB reqs = 16
    - USB Speed = Super-Speed
    - Function Driver: Mass Storage (w/ ramdisk)
    - Test Application: CrystalDiskMark

Results:

TXFIFO Depth = 3 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 193.60

        |           | 195.86
             |           | 184.77
             |           | 193.60

-------------------------------------------

TXFIFO Depth = 6 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 287.35

        |           | 304.94
             |           | 289.64
             |           | 293.61

-------------------------------------------

Wesley Cheng (6):
     usb: gadget: udc: core: Introduce check_config to verify USB configuration
     usb: gadget: configfs: Check USB configuration before adding
     usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
     of: Add stub for of_add_property()
     usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default
     dt-bindings: usb: dwc3: Update dwc3 TX fifo properties

    .../devicetree/bindings/usb/snps,dwc3.yaml         |  15 +- drivers/usb/dwc3/core.c                            |   9 + drivers/usb/dwc3/core.h                            |  15 ++ drivers/usb/dwc3/dwc3-qcom.c                       |   9 + drivers/usb/dwc3/ep0.c                             |   2 + drivers/usb/dwc3/gadget.c                          | 212
+++++++++++++++++++++
    drivers/usb/gadget/configfs.c                      |  22 +++ drivers/usb/gadget/udc/core.c                      |  25 +++ include/linux/of.h                                 |   5 + include/linux/usb/gadget.h                         |   5 +

    10 files changed, 317 insertions(+), 2 deletions(-)

Quoted 85 lines

Hi

Op 22-06-2021 om 20:38 schreef Wesley Cheng:

Quoted 82 lines · Ferry Toth

Quoted 64 lines

Hi

Op 18-06-2021 om 00:25 schreef Wesley Cheng:

Quoted 23 lines

Hi,

Quoted 20 lines · Ferry Toth

Hi

Op 17-06-2021 om 23:48 schreef Wesley Cheng:

Quoted 17 lines

Hi,

Quoted 14 lines · Ferry Toth

Hi

Op 17-06-2021 om 11:58 schreef Wesley Cheng:

Quoted 8 lines

Changes in V10:
    - Fixed compilation errors in config where OF is not used
(error due to
      unknown symbol for of_add_property()).  Add of_add_property()
stub.
    - Fixed compilation warning for incorrect argument being
passed to
dwc3_mdwidth

This fixes the OOPS I had in V9. I do not see any change in performance
on Merrifield though.

Quoted 20 lines

Quoted 6 lines

Quoted 4 lines

I see...thanks Ferry! With your testing, are you writing to the device's
internal storage (ie UFS, eMMC, etc...), or did you use a ramdisk as well?

In this case I just tested the EEM path using iperf3.

Got it.  I don't believe f_eem will use a high enough (if at all) bMaxBurst value to change the TXFIFO size.

Quoted 11 lines

Quoted 7 lines

If not with a ramdisk, we might want to give that a try to avoid the storage path being the bottleneck.  You can use "dd" to create an empty
file, and then just use that as the LUN's backing file.

echo ramdisk.img > /sys/kernel/config/usb_gadget/g1/functions/mass_storage.0/lun.0/file

Ah, why didn't I think of that. I have currently mass storage setup with
eMMC but it seems that is indeed the bottleneck.

I created a 64MB disk following the instructions here
http://www.linux-usb.org/gadget/file_storage.html (that seems a little outdated, at least I can not start the first partition at sector 8, but minimum 2048), and added a test file on it.

I then copy the file to /dev/shm prior to setting configfs (composite device gser/eem/mass_storage/uac2).

journal shows:

kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)

I don't know what that means, because I see the test file on the ramdisk.

Then I again used gnome disks to benchmark (read/write 10MB):

Quoted 7 lines

With V10 on top v5.13.0-rc5:

R/W speed = 35.6/35.8MB/s, access time 0.35ms

With no patches on top v5.12.0:

R/W speed = 35.7/36.1MB/s, access time 0.35ms

Hi Ferry,

Quoted 3 lines

I see no speed difference (and it's about the same as with the eMMC backed disk). But the patches are causing a new call trace

Would you happen to know what DWC3 controller revision the device is using?  The callstack print occurs, because it looks like it ran out of internal memory, although there should be logic present for making sure that at least there is enough room for 1 FIFO per endpoint.  (possibly the logic/math depends on the controller revision)

Hi Ferry,

Quoted 2 lines

Do you know where I could find that in a file on the device?

Maybe you can just dump the DWC3 registers? cat /sys/kernel/debug/usb/<controller name>/regdump

Was going to ask for the same to confirm the TXFIFO sizes from your results below :).

Quoted 8 lines

Otherwise, I'm hoping Andy will know?

Quoted 4 lines

Also, is there a way to use just a mass storage only composition?  Based on the above observation, that probably means that the mass storage interface wasn't resized at all, because the configuration took up a lot of the internal FIFO space.

Sure, it's configured through configfs. With only mass_storage I have:

Quoted 8 lines

With V10 on top v5.13.0-rc5:

R/W speed = 41,6/39,3MB/s, access time 0.33ms

With no patches on top v5.12.0:

R/W speed = 41,1/38,7MB/s, access time 0.38ms

Thanks Ferry! Could you collect the regdump, so I can confirm the two things mentioned?

Thanks
Wesley Cheng

Quoted 287 lines

Quoted 287 lines

Thanks
Wesley Cheng

Quoted 284 lines

kernel: using random self ethernet address
kernel: using random host ethernet address
kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)
kernel: usb0: HOST MAC aa:bb:cc:dd:ee:f2
kernel: usb0: MAC aa:bb:cc:dd:ee:f1
kernel: IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: ------------[ cut here ]------------
kernel: WARNING: CPU: 0 PID: 594 at drivers/usb/gadget/udc/core.c:278 usb_ep_queue+0x75/0x80
kernel: Modules linked in: usb_f_uac2 u_audio usb_f_mass_storage usb_f_eem u_ether usb_f_serial u_serial libcomposite rfcomm iptable_nat bnep snd_sof_nocodec spi_pxa2>
kernel: CPU: 0 PID: 594 Comm: irq/14-dwc3 Not tainted 5.13.0-rc5-edison-acpi-standard #1
kernel: Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
kernel: RIP: 0010:usb_ep_queue+0x75/0x80
kernel: Code: 01 73 e4 48 8b 05 fb 63 06 01 48 85 c0 74 12 48 8b 78 08 44 89 e9 4c 89 e2 48 89 ee e8 74 05 00 00 44 89 e8 5d 41 5c 41 5d c3 <0f> 0b 41 bd 94 ff ff ff >
kernel: RSP: 0000:ffff91eec083fc98 EFLAGS: 00010082
kernel: RAX: ffff8af20357d960 RBX: 0000000000000000 RCX:
ffff8af202f06400
kernel: RDX: 0000000000000a20 RSI: ffff8af208785780 RDI:
ffff8af202e9ae00
kernel: RBP: ffff8af202e9ae00 R08: 00000000000000c0 R09:
ffff8af208785780
kernel: R10: 00000000ffffe000 R11: 3fffffffffffffff R12:
ffff8af208785780
kernel: R13: 0000000000000000 R14: ffff8af202e9ae00 R15:
ffff8af203e26cc0
kernel: FS:  0000000000000000(0000) GS:ffff8af23e200000(0000) knlGS:0000000000000000
kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 000055e2c21f2100 CR3: 0000000003b38000 CR4:
00000000001006f0
kernel: Call Trace:
kernel:  u_audio_start_playback+0x107/0x1a0 [u_audio] kernel:  composite_setup+0x224/0x1ba0 [libcomposite]
kernel:  ? dwc3_gadget_ep_queue+0xf6/0x1a0
kernel:  ? usb_ep_queue+0x2a/0x80
kernel:  ? configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  dwc3_ep0_interrupt+0x469/0xa80
kernel:  dwc3_thread_interrupt+0x8ee/0xf40
kernel:  ? __wake_up_common_lock+0x85/0xb0
kernel:  ? disable_irq_nosync+0x10/0x10
kernel:  irq_thread_fn+0x1b/0x60
kernel:  irq_thread+0xd6/0x170
kernel:  ? irq_thread_check_affinity+0x70/0x70
kernel:  ? irq_forced_thread_fn+0x70/0x70
kernel:  kthread+0x116/0x130
kernel:  ? kthread_create_worker_on_cpu+0x60/0x60
kernel:  ret_from_fork+0x22/0x30
kernel: ---[ end trace e5b9e28058c53584 ]---
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000036ac129 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in

Removing uac2 from the config makes the call trace go away, but the R/W speed does not change.

Quoted 184 lines

:), not a problem...I've been working on getting the ideal set up for the performance profiling for awhile, so anything I can do to make sure we get some good results.

Quoted 2 lines

I'll try with a ramdisk and let you know.

Thanks again for the testing, Ferry.

Thanks
Wesley Cheng

Quoted 173 lines

Quoted 173 lines

Thanks
Wesley Cheng

Quoted 170 lines

Quoted 170 lines

Changes in V9:
    - Fixed incorrect patch in series.  Removed changes in DTSI, as
dwc3-qcom will
      add the property by default from the kernel.

Changes in V8:
    - Rebased to usb-testing
    - Using devm_kzalloc for adding txfifo property in dwc3-qcom
    - Removed DWC3 QCOM ACPI property for enabling the txfifo
resize

Changes in V7:
    - Added a new property tx-fifo-max-num for limiting how much
fifo
space the
      resizing logic can allocate for endpoints with large burst
values.  This
      can differ across platforms, and tie in closely with overall
system latency.
    - Added recommended checks for DWC32.
    - Added changes to set the tx-fifo-resize property from
dwc3-qcom by
default
      instead of modifying the current DTSI files.
    - Added comments on all APIs/variables introduced.
    - Updated the DWC3 YAML to include a better description of the
tx-fifo-resize
      property and added an entry for tx-fifo-max-num.

Changes in V6:
    - Rebased patches to usb-testing.
    - Renamed to PATCH series instead of RFC.
    - Checking for fs_descriptors instead of ss_descriptors for
determining the
      endpoint count for a particular configuration.
    - Re-ordered patch series to fix patch dependencies.

Changes in V5:
    - Added check_config() logic, which is used to communicate the
number of EPs
      used in a particular configuration.  Based on this, the DWC3
gadget driver
      has the ability to know the maximum number of eps utilized in
all
configs.
      This helps reduce unnecessary allocation to unused eps,
and will
catch fifo
      allocation issues at bind() time.
    - Fixed variable declaration to single line per variable, and
reverse xmas.
    - Created a helper for fifo clearing, which is used by ep0.c

Changes in V4:
    - Removed struct dwc3* as an argument for
dwc3_gadget_resize_tx_fifos()
    - Removed WARN_ON(1) in case we run out of fifo space Changes in V3:
    - Removed "Reviewed-by" tags
    - Renamed series back to RFC
    - Modified logic to ensure that fifo_size is reset if we
pass the
minimum
      threshold.  Tested with binding multiple FDs requesting 6
FIFOs.

Changes in V2:
    - Modified TXFIFO resizing logic to ensure that each EP is
reserved a
      FIFO.
    - Removed dev_dbg() prints and fixed typos from patches
    - Added some more description on the dt-bindings commit message

Currently, there is no functionality to allow for resizing the TXFIFOs, and
relying on the HW default setting for the TXFIFO depth.  In most cases, the
HW default is probably sufficient, but for USB compositions that contain
multiple functions that require EP bursting, the default settings might not be enough.  Also to note, the current SW will assign an EP to a
function driver w/o checking to see if the TXFIFO size for that particular
EP is large enough. (this is a problem if there are multiple HW defined
values for the TXFIFO size)

It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3
is required for an EP that supports bursting.  Otherwise, there may be
frequent occurences of bursts ending.  For high bandwidth functions,
such as data tethering (protocols that support data aggregation), mass
storage, and media transfer protocol (over FFS), the bMaxBurst value
can be
large, and a bigger TXFIFO depth may prove to be beneficial in terms
of USB
throughput. (which can be associated to system access latency, etc...)  It
allows for a more consistent burst of traffic, w/o any interruptions, as
data is readily available in the FIFO.

With testing done using the mass storage function driver, the results
show
that with a larger TXFIFO depth, the bandwidth increased significantly.

Test Parameters:
    - Platform: Qualcomm SM8150
    - bMaxBurst = 6
    - USB req size = 256kB
    - Num of USB reqs = 16
    - USB Speed = Super-Speed
    - Function Driver: Mass Storage (w/ ramdisk)
    - Test Application: CrystalDiskMark

Results:

TXFIFO Depth = 3 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 193.60

         |           | 195.86
              |           | 184.77
              |           | 193.60

-------------------------------------------

TXFIFO Depth = 6 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 287.35

         |           | 304.94
              |           | 289.64
              |           | 293.61

-------------------------------------------

Wesley Cheng (6):
     usb: gadget: udc: core: Introduce check_config to verify USB configuration
     usb: gadget: configfs: Check USB configuration before adding
     usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
     of: Add stub for of_add_property()
     usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by
default
     dt-bindings: usb: dwc3: Update dwc3 TX fifo properties

     .../devicetree/bindings/usb/snps,dwc3.yaml         |  15 +-
     drivers/usb/dwc3/core.c                            |   9 +
     drivers/usb/dwc3/core.h                            |  15 ++
     drivers/usb/dwc3/dwc3-qcom.c                       |   9 +
     drivers/usb/dwc3/ep0.c                             |   2 +
     drivers/usb/dwc3/gadget.c                          | 212
+++++++++++++++++++++
     drivers/usb/gadget/configfs.c                      |  22 +++
     drivers/usb/gadget/udc/core.c                      |  25 +++
     include/linux/of.h                                 |   5 +
     include/linux/usb/gadget.h                         |   5 +
     10 files changed, 317 insertions(+), 2 deletions(-)

Hi

Op 23-06-2021 om 00:00 schreef Wesley Cheng:

Quoted 92 lines · Ferry Toth

Quoted 90 lines

Hi

Op 22-06-2021 om 20:38 schreef Wesley Cheng:

Quoted 87 lines · Ferry Toth

Quoted 69 lines

Hi

Op 18-06-2021 om 00:25 schreef Wesley Cheng:

Quoted 29 lines

Hi,

Quoted 26 lines · Ferry Toth

Hi

Op 17-06-2021 om 23:48 schreef Wesley Cheng:

Quoted 23 lines

Hi,

Quoted 12 lines · Ferry Toth

Hi

Op 17-06-2021 om 11:58 schreef Wesley Cheng:

Quoted 9 lines

Changes in V10:
     - Fixed compilation errors in config where OF is not used (error due to
       unknown symbol for of_add_property()).  Add of_add_property()
stub.
     - Fixed compilation warning for incorrect argument being passed to
dwc3_mdwidth

Quoted 3 lines

This fixes the OOPS I had in V9. I do not see any change in performance
on Merrifield though.

I see...thanks Ferry! With your testing, are you writing to the device's
internal storage (ie UFS, eMMC, etc...), or did you use a ramdisk as
well?

Quoted 18 lines

Quoted 2 lines

In this case I just tested the EEM path using iperf3.

Got it.  I don't believe f_eem will use a high enough (if at all) bMaxBurst value to change the TXFIFO size.

Quoted 13 lines

Quoted 9 lines

If not with a ramdisk, we might want to give that a try to avoid the
storage path being the bottleneck.  You can use "dd" to create an empty
file, and then just use that as the LUN's backing file.

echo ramdisk.img > /sys/kernel/config/usb_gadget/g1/functions/mass_storage.0/lun.0/file

Ah, why didn't I think of that. I have currently mass storage setup with
eMMC but it seems that is indeed the bottleneck.

I created a 64MB disk following the instructions here
http://www.linux-usb.org/gadget/file_storage.html (that seems a little outdated, at least I can not start the first partition at sector 8, but
minimum 2048), and added a test file on it.

I then copy the file to /dev/shm prior to setting configfs (composite device gser/eem/mass_storage/uac2).

journal shows:

kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)

I don't know what that means, because I see the test file on the ramdisk.

Then I again used gnome disks to benchmark (read/write 10MB):

Quoted 7 lines

With V10 on top v5.13.0-rc5:

R/W speed = 35.6/35.8MB/s, access time 0.35ms

With no patches on top v5.12.0:

R/W speed = 35.7/36.1MB/s, access time 0.35ms

Hi Ferry,

Quoted 3 lines

I see no speed difference (and it's about the same as with the eMMC backed disk). But the patches are causing a new call trace

Would you happen to know what DWC3 controller revision the device is using?  The callstack print occurs, because it looks like it ran out of internal memory, although there should be logic present for making sure that at least there is enough room for 1 FIFO per endpoint.  (possibly the logic/math depends on the controller revision)

Quoted 6 lines

Hi Ferry,

Quoted 2 lines

Do you know where I could find that in a file on the device?

Maybe you can just dump the DWC3 registers? cat /sys/kernel/debug/usb/<controller name>/regdump

This isin host mode:

~# cat /sys/kernel/debug/usb/dwc3.0.auto/regdump
GSBUSCFG0 = 0x00000006
GSBUSCFG1 = 0x00000f00
GTXTHRCFG = 0x230a0000
GRXTHRCFG = 0x22800000
GCTL = 0x45801002
GEVTEN = 0x00000000
GSTS = 0x3e800001
GUCTL1 = 0x00000000
GSNPSID = 0x5533250a
GGPIO = 0x00000000
GUID = 0x00050d00
GUCTL = 0x0200ce00
GBUSERRADDR0 = 0x00000000
GBUSERRADDR1 = 0x00000000

Quoted 328 lines

Was going to ask for the same to confirm the TXFIFO sizes from your results below :).

Quoted 10 lines

Otherwise, I'm hoping Andy will know?

Quoted 6 lines

Also, is there a way to use just a mass storage only composition? Based
on the above observation, that probably means that the mass storage interface wasn't resized at all, because the configuration took up a lot
of the internal FIFO space.

Sure, it's configured through configfs. With only mass_storage I have:

Quoted 8 lines

With V10 on top v5.13.0-rc5:

R/W speed = 41,6/39,3MB/s, access time 0.33ms

With no patches on top v5.12.0:

R/W speed = 41,1/38,7MB/s, access time 0.38ms

Thanks Ferry!  Could you collect the regdump, so I can confirm the two things mentioned?

Thanks
Wesley Cheng

Quoted 301 lines

Quoted 301 lines

Thanks
Wesley Cheng

Quoted 298 lines

kernel: using random self ethernet address
kernel: using random host ethernet address
kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)
kernel: usb0: HOST MAC aa:bb:cc:dd:ee:f2
kernel: usb0: MAC aa:bb:cc:dd:ee:f1
kernel: IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: ------------[ cut here ]------------
kernel: WARNING: CPU: 0 PID: 594 at drivers/usb/gadget/udc/core.c:278 usb_ep_queue+0x75/0x80
kernel: Modules linked in: usb_f_uac2 u_audio usb_f_mass_storage usb_f_eem u_ether usb_f_serial u_serial libcomposite rfcomm iptable_nat
bnep snd_sof_nocodec spi_pxa2>
kernel: CPU: 0 PID: 594 Comm: irq/14-dwc3 Not tainted 5.13.0-rc5-edison-acpi-standard #1
kernel: Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542
2015.01.21:18.19.48
kernel: RIP: 0010:usb_ep_queue+0x75/0x80
kernel: Code: 01 73 e4 48 8b 05 fb 63 06 01 48 85 c0 74 12 48 8b 78 08 44 89 e9 4c 89 e2 48 89 ee e8 74 05 00 00 44 89 e8 5d 41 5c 41 5d c3 <0f> 0b 41 bd 94 ff ff ff >
kernel: RSP: 0000:ffff91eec083fc98 EFLAGS: 00010082
kernel: RAX: ffff8af20357d960 RBX: 0000000000000000 RCX:
ffff8af202f06400
kernel: RDX: 0000000000000a20 RSI: ffff8af208785780 RDI:
ffff8af202e9ae00
kernel: RBP: ffff8af202e9ae00 R08: 00000000000000c0 R09:
ffff8af208785780
kernel: R10: 00000000ffffe000 R11: 3fffffffffffffff R12:
ffff8af208785780
kernel: R13: 0000000000000000 R14: ffff8af202e9ae00 R15:
ffff8af203e26cc0
kernel: FS:  0000000000000000(0000) GS:ffff8af23e200000(0000) knlGS:0000000000000000
kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 000055e2c21f2100 CR3: 0000000003b38000 CR4:
00000000001006f0
kernel: Call Trace:
kernel:  u_audio_start_playback+0x107/0x1a0 [u_audio] kernel:  composite_setup+0x224/0x1ba0 [libcomposite] kernel:  ? dwc3_gadget_ep_queue+0xf6/0x1a0
kernel:  ? usb_ep_queue+0x2a/0x80
kernel:  ? configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  dwc3_ep0_interrupt+0x469/0xa80
kernel:  dwc3_thread_interrupt+0x8ee/0xf40
kernel:  ? __wake_up_common_lock+0x85/0xb0
kernel:  ? disable_irq_nosync+0x10/0x10
kernel:  irq_thread_fn+0x1b/0x60
kernel:  irq_thread+0xd6/0x170
kernel:  ? irq_thread_check_affinity+0x70/0x70
kernel:  ? irq_forced_thread_fn+0x70/0x70
kernel:  kthread+0x116/0x130
kernel:  ? kthread_create_worker_on_cpu+0x60/0x60
kernel:  ret_from_fork+0x22/0x30
kernel: ---[ end trace e5b9e28058c53584 ]---
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000036ac129 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in

Removing uac2 from the config makes the call trace go away, but the R/W
speed does not change.

Quoted 195 lines

:), not a problem...I've been working on getting the ideal set up for the performance profiling for awhile, so anything I can do to make sure
we get some good results.

Quoted 2 lines

I'll try with a ramdisk and let you know.

Thanks again for the testing, Ferry.

Thanks
Wesley Cheng

Quoted 183 lines

Quoted 183 lines

Thanks
Wesley Cheng

Quoted 180 lines

Quoted 180 lines

Changes in V9:
     - Fixed incorrect patch in series.  Removed changes in DTSI, as
dwc3-qcom will
       add the property by default from the kernel.

Changes in V8:
     - Rebased to usb-testing
     - Using devm_kzalloc for adding txfifo property in dwc3-qcom
     - Removed DWC3 QCOM ACPI property for enabling the txfifo resize

Changes in V7:
     - Added a new property tx-fifo-max-num for limiting how much fifo
space the
       resizing logic can allocate for endpoints with large burst values.  This
       can differ across platforms, and tie in closely with overall
system latency.
     - Added recommended checks for DWC32.
     - Added changes to set the tx-fifo-resize property from dwc3-qcom by
default
       instead of modifying the current DTSI files.
     - Added comments on all APIs/variables introduced.
     - Updated the DWC3 YAML to include a better description of the
tx-fifo-resize
       property and added an entry for tx-fifo-max-num.

Changes in V6:
     - Rebased patches to usb-testing.
     - Renamed to PATCH series instead of RFC.
     - Checking for fs_descriptors instead of ss_descriptors for determining the
       endpoint count for a particular configuration.
     - Re-ordered patch series to fix patch dependencies.

Changes in V5:
     - Added check_config() logic, which is used to communicate the
number of EPs
       used in a particular configuration.  Based on this, the DWC3
gadget driver
       has the ability to know the maximum number of eps utilized in
all
configs.
       This helps reduce unnecessary allocation to unused eps, and will
catch fifo
       allocation issues at bind() time.
     - Fixed variable declaration to single line per variable, and
reverse xmas.
     - Created a helper for fifo clearing, which is used by ep0.c

Changes in V4:
     - Removed struct dwc3* as an argument for dwc3_gadget_resize_tx_fifos()
     - Removed WARN_ON(1) in case we run out of fifo space Changes in V3:
     - Removed "Reviewed-by" tags
     - Renamed series back to RFC
     - Modified logic to ensure that fifo_size is reset if we pass the
minimum
       threshold.  Tested with binding multiple FDs requesting 6 FIFOs.

Changes in V2:
     - Modified TXFIFO resizing logic to ensure that each EP is reserved a
       FIFO.
     - Removed dev_dbg() prints and fixed typos from patches
     - Added some more description on the dt-bindings commit message

Currently, there is no functionality to allow for resizing the TXFIFOs, and
relying on the HW default setting for the TXFIFO depth.  In most cases, the
HW default is probably sufficient, but for USB compositions that contain
multiple functions that require EP bursting, the default settings might not be enough.  Also to note, the current SW will assign an EP to a
function driver w/o checking to see if the TXFIFO size for that particular
EP is large enough. (this is a problem if there are multiple HW defined
values for the TXFIFO size)

It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3
is required for an EP that supports bursting.  Otherwise, there may be
frequent occurences of bursts ending.  For high bandwidth functions,
such as data tethering (protocols that support data aggregation), mass
storage, and media transfer protocol (over FFS), the bMaxBurst value
can be
large, and a bigger TXFIFO depth may prove to be beneficial in terms
of USB
throughput. (which can be associated to system access latency, etc...)  It
allows for a more consistent burst of traffic, w/o any interruptions, as
data is readily available in the FIFO.

With testing done using the mass storage function driver, the results
show
that with a larger TXFIFO depth, the bandwidth increased significantly.

Test Parameters:
     - Platform: Qualcomm SM8150
     - bMaxBurst = 6
     - USB req size = 256kB
     - Num of USB reqs = 16
     - USB Speed = Super-Speed
     - Function Driver: Mass Storage (w/ ramdisk)
     - Test Application: CrystalDiskMark

Results:

TXFIFO Depth = 3 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 193.60

         |           | 195.86
              |           | 184.77
              |           | 193.60

-------------------------------------------

TXFIFO Depth = 6 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 287.35

         |           | 304.94
              |           | 289.64
              |           | 293.61

-------------------------------------------

Wesley Cheng (6):
      usb: gadget: udc: core: Introduce check_config to verify USB
        configuration
      usb: gadget: configfs: Check USB configuration before adding
      usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
      of: Add stub for of_add_property()
      usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default
      dt-bindings: usb: dwc3: Update dwc3 TX fifo properties

     .../devicetree/bindings/usb/snps,dwc3.yaml         |  15 +- drivers/usb/dwc3/core.c                            |   9 + drivers/usb/dwc3/core.h                            |  15 ++ drivers/usb/dwc3/dwc3-qcom.c                       |   9 + drivers/usb/dwc3/ep0.c                             |   2 + drivers/usb/dwc3/gadget.c                          | 212
+++++++++++++++++++++
     drivers/usb/gadget/configfs.c                      |  22 +++ drivers/usb/gadget/udc/core.c                      |  25 +++ include/linux/of.h                                 |   5 + include/linux/usb/gadget.h                         |   5 +

     10 files changed, 317 insertions(+), 2 deletions(-)

Quoted 102 lines

Hi

Op 23-06-2021 om 00:00 schreef Wesley Cheng:

Quoted 91 lines · Ferry Toth

Quoted 90 lines

Hi

Op 22-06-2021 om 20:38 schreef Wesley Cheng:

Quoted 87 lines · Ferry Toth

Quoted 69 lines

Hi

Op 18-06-2021 om 00:25 schreef Wesley Cheng:

Quoted 29 lines

Hi,

Quoted 26 lines · Ferry Toth

Hi

Op 17-06-2021 om 23:48 schreef Wesley Cheng:

Quoted 23 lines

Hi,

Quoted 12 lines · Ferry Toth

Hi

Op 17-06-2021 om 11:58 schreef Wesley Cheng:

Quoted 9 lines

Changes in V10:
     - Fixed compilation errors in config where OF is not used
(error due to
       unknown symbol for of_add_property()).  Add
of_add_property()
stub.
     - Fixed compilation warning for incorrect argument being
passed to
dwc3_mdwidth

Quoted 3 lines

This fixes the OOPS I had in V9. I do not see any change in performance
on Merrifield though.

I see...thanks Ferry! With your testing, are you writing to the device's
internal storage (ie UFS, eMMC, etc...), or did you use a ramdisk as
well?

Quoted 18 lines

Quoted 2 lines

In this case I just tested the EEM path using iperf3.

Got it.  I don't believe f_eem will use a high enough (if at all) bMaxBurst value to change the TXFIFO size.

Quoted 13 lines

Quoted 9 lines

If not with a ramdisk, we might want to give that a try to avoid the
storage path being the bottleneck.  You can use "dd" to create an empty
file, and then just use that as the LUN's backing file.

echo ramdisk.img > /sys/kernel/config/usb_gadget/g1/functions/mass_storage.0/lun.0/file

Ah, why didn't I think of that. I have currently mass storage setup with
eMMC but it seems that is indeed the bottleneck.

I created a 64MB disk following the instructions here
http://www.linux-usb.org/gadget/file_storage.html (that seems a little outdated, at least I can not start the first partition at sector 8, but
minimum 2048), and added a test file on it.

I then copy the file to /dev/shm prior to setting configfs (composite device gser/eem/mass_storage/uac2).

journal shows:

kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)

I don't know what that means, because I see the test file on the ramdisk.

Then I again used gnome disks to benchmark (read/write 10MB):

Quoted 7 lines

With V10 on top v5.13.0-rc5:

R/W speed = 35.6/35.8MB/s, access time 0.35ms

With no patches on top v5.12.0:

R/W speed = 35.7/36.1MB/s, access time 0.35ms

Hi Ferry,

Quoted 3 lines

I see no speed difference (and it's about the same as with the eMMC backed disk). But the patches are causing a new call trace

Would you happen to know what DWC3 controller revision the device is using?  The callstack print occurs, because it looks like it ran out of internal memory, although there should be logic present for making sure that at least there is enough room for 1 FIFO per endpoint.  (possibly the logic/math depends on the controller revision)

Quoted 6 lines

Hi Ferry,

Quoted 2 lines

Do you know where I could find that in a file on the device?

Maybe you can just dump the DWC3 registers? cat /sys/kernel/debug/usb/<controller name>/regdump

This isin host mode:

Quoted 15 lines

~# cat /sys/kernel/debug/usb/dwc3.0.auto/regdump
GSBUSCFG0 = 0x00000006
GSBUSCFG1 = 0x00000f00
GTXTHRCFG = 0x230a0000
GRXTHRCFG = 0x22800000
GCTL = 0x45801002
GEVTEN = 0x00000000
GSTS = 0x3e800001
GUCTL1 = 0x00000000
GSNPSID = 0x5533250a
GGPIO = 0x00000000
GUID = 0x00050d00
GUCTL = 0x0200ce00
GBUSERRADDR0 = 0x00000000
GBUSERRADDR1 = 0x00000000

...
Hi Ferry,

Great! Thanks, that got me the information regarding the DWC3 revision you're using. (its version 2.50a) I checked the DWC3 databook and looks like certain versions have some different math to calculate the TXFIFO size. I have taken this into account and factored that in in the next revision.

Is there a way you could collect the regdump in device mode w/ mass storage only composition after enumerating with the PC? That would help confirm how the TXFIFO resizing logic is working on your platform.

Thanks
Wesley Cheng

Quoted 328 lines

Quoted 328 lines

Was going to ask for the same to confirm the TXFIFO sizes from your results below :).

Quoted 10 lines

Otherwise, I'm hoping Andy will know?

Quoted 6 lines

Also, is there a way to use just a mass storage only composition? Based
on the above observation, that probably means that the mass storage interface wasn't resized at all, because the configuration took up a lot
of the internal FIFO space.

Sure, it's configured through configfs. With only mass_storage I have:

Quoted 8 lines

With V10 on top v5.13.0-rc5:

R/W speed = 41,6/39,3MB/s, access time 0.33ms

With no patches on top v5.12.0:

R/W speed = 41,1/38,7MB/s, access time 0.38ms

Thanks Ferry!  Could you collect the regdump, so I can confirm the two things mentioned?

Thanks
Wesley Cheng

Quoted 301 lines

Quoted 301 lines

Thanks
Wesley Cheng

Quoted 298 lines

kernel: using random self ethernet address
kernel: using random host ethernet address
kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)
kernel: usb0: HOST MAC aa:bb:cc:dd:ee:f2
kernel: usb0: MAC aa:bb:cc:dd:ee:f1
kernel: IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: ------------[ cut here ]------------
kernel: WARNING: CPU: 0 PID: 594 at drivers/usb/gadget/udc/core.c:278 usb_ep_queue+0x75/0x80
kernel: Modules linked in: usb_f_uac2 u_audio usb_f_mass_storage usb_f_eem u_ether usb_f_serial u_serial libcomposite rfcomm iptable_nat
bnep snd_sof_nocodec spi_pxa2>
kernel: CPU: 0 PID: 594 Comm: irq/14-dwc3 Not tainted 5.13.0-rc5-edison-acpi-standard #1
kernel: Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542
2015.01.21:18.19.48
kernel: RIP: 0010:usb_ep_queue+0x75/0x80
kernel: Code: 01 73 e4 48 8b 05 fb 63 06 01 48 85 c0 74 12 48 8b 78 08 44 89 e9 4c 89 e2 48 89 ee e8 74 05 00 00 44 89 e8 5d 41 5c 41 5d c3 <0f> 0b 41 bd 94 ff ff ff >
kernel: RSP: 0000:ffff91eec083fc98 EFLAGS: 00010082
kernel: RAX: ffff8af20357d960 RBX: 0000000000000000 RCX:
ffff8af202f06400
kernel: RDX: 0000000000000a20 RSI: ffff8af208785780 RDI:
ffff8af202e9ae00
kernel: RBP: ffff8af202e9ae00 R08: 00000000000000c0 R09:
ffff8af208785780
kernel: R10: 00000000ffffe000 R11: 3fffffffffffffff R12:
ffff8af208785780
kernel: R13: 0000000000000000 R14: ffff8af202e9ae00 R15:
ffff8af203e26cc0
kernel: FS:  0000000000000000(0000) GS:ffff8af23e200000(0000) knlGS:0000000000000000
kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 000055e2c21f2100 CR3: 0000000003b38000 CR4:
00000000001006f0
kernel: Call Trace:
kernel:  u_audio_start_playback+0x107/0x1a0 [u_audio] kernel:  composite_setup+0x224/0x1ba0 [libcomposite] kernel:  ? dwc3_gadget_ep_queue+0xf6/0x1a0
kernel:  ? usb_ep_queue+0x2a/0x80
kernel:  ? configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  dwc3_ep0_interrupt+0x469/0xa80
kernel:  dwc3_thread_interrupt+0x8ee/0xf40
kernel:  ? __wake_up_common_lock+0x85/0xb0
kernel:  ? disable_irq_nosync+0x10/0x10
kernel:  irq_thread_fn+0x1b/0x60
kernel:  irq_thread+0xd6/0x170
kernel:  ? irq_thread_check_affinity+0x70/0x70
kernel:  ? irq_forced_thread_fn+0x70/0x70
kernel:  kthread+0x116/0x130
kernel:  ? kthread_create_worker_on_cpu+0x60/0x60
kernel:  ret_from_fork+0x22/0x30
kernel: ---[ end trace e5b9e28058c53584 ]---
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000036ac129 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in

Removing uac2 from the config makes the call trace go away, but the R/W
speed does not change.

Quoted 195 lines

:), not a problem...I've been working on getting the ideal set up for the performance profiling for awhile, so anything I can do to make sure
we get some good results.

Quoted 2 lines

I'll try with a ramdisk and let you know.

Thanks again for the testing, Ferry.

Thanks
Wesley Cheng

Quoted 183 lines

Quoted 183 lines

Thanks
Wesley Cheng

Quoted 180 lines

Quoted 180 lines

Changes in V9:
     - Fixed incorrect patch in series.  Removed changes in
DTSI, as
dwc3-qcom will
       add the property by default from the kernel.

Changes in V8:
     - Rebased to usb-testing
     - Using devm_kzalloc for adding txfifo property in dwc3-qcom
     - Removed DWC3 QCOM ACPI property for enabling the txfifo
resize

Changes in V7:
     - Added a new property tx-fifo-max-num for limiting how much
fifo
space the
       resizing logic can allocate for endpoints with large burst
values.  This
       can differ across platforms, and tie in closely with
overall
system latency.
     - Added recommended checks for DWC32.
     - Added changes to set the tx-fifo-resize property from
dwc3-qcom by
default
       instead of modifying the current DTSI files.
     - Added comments on all APIs/variables introduced.
     - Updated the DWC3 YAML to include a better description
of the
tx-fifo-resize
       property and added an entry for tx-fifo-max-num.

Changes in V6:
     - Rebased patches to usb-testing.
     - Renamed to PATCH series instead of RFC.
     - Checking for fs_descriptors instead of ss_descriptors for
determining the
       endpoint count for a particular configuration.
     - Re-ordered patch series to fix patch dependencies.

Changes in V5:
     - Added check_config() logic, which is used to
communicate the
number of EPs
       used in a particular configuration.  Based on this, the
DWC3
gadget driver
       has the ability to know the maximum number of eps
utilized in
all
configs.
       This helps reduce unnecessary allocation to unused eps,
and will
catch fifo
       allocation issues at bind() time.
     - Fixed variable declaration to single line per variable,
and
reverse xmas.
     - Created a helper for fifo clearing, which is used by ep0.c

Changes in V4:
     - Removed struct dwc3* as an argument for
dwc3_gadget_resize_tx_fifos()
     - Removed WARN_ON(1) in case we run out of fifo space Changes in V3:
     - Removed "Reviewed-by" tags
     - Renamed series back to RFC
     - Modified logic to ensure that fifo_size is reset if we
pass the
minimum
       threshold.  Tested with binding multiple FDs requesting 6
FIFOs.

Changes in V2:
     - Modified TXFIFO resizing logic to ensure that each EP is
reserved a
       FIFO.
     - Removed dev_dbg() prints and fixed typos from patches
     - Added some more description on the dt-bindings commit
message

Currently, there is no functionality to allow for resizing the TXFIFOs, and
relying on the HW default setting for the TXFIFO depth.  In most cases, the
HW default is probably sufficient, but for USB compositions that contain
multiple functions that require EP bursting, the default settings might not be enough.  Also to note, the current SW will assign an EP to a
function driver w/o checking to see if the TXFIFO size for that particular
EP is large enough. (this is a problem if there are multiple HW defined
values for the TXFIFO size)

It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3
is required for an EP that supports bursting.  Otherwise, there may be
frequent occurences of bursts ending.  For high bandwidth functions,
such as data tethering (protocols that support data aggregation), mass
storage, and media transfer protocol (over FFS), the bMaxBurst value
can be
large, and a bigger TXFIFO depth may prove to be beneficial in terms
of USB
throughput. (which can be associated to system access latency, etc...)  It
allows for a more consistent burst of traffic, w/o any interruptions, as
data is readily available in the FIFO.

With testing done using the mass storage function driver, the results
show
that with a larger TXFIFO depth, the bandwidth increased significantly.

Test Parameters:
     - Platform: Qualcomm SM8150
     - bMaxBurst = 6
     - USB req size = 256kB
     - Num of USB reqs = 16
     - USB Speed = Super-Speed
     - Function Driver: Mass Storage (w/ ramdisk)
     - Test Application: CrystalDiskMark

Results:

TXFIFO Depth = 3 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 193.60

          |           | 195.86
               |           | 184.77
               |           | 193.60

-------------------------------------------

TXFIFO Depth = 6 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 287.35

          |           | 304.94
               |           | 289.64
               |           | 293.61

-------------------------------------------

Wesley Cheng (6):
      usb: gadget: udc: core: Introduce check_config to verify
USB
        configuration
      usb: gadget: configfs: Check USB configuration before
adding
      usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
      of: Add stub for of_add_property()
      usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by
default
      dt-bindings: usb: dwc3: Update dwc3 TX fifo properties

      .../devicetree/bindings/usb/snps,dwc3.yaml         |  15 +-
      drivers/usb/dwc3/core.c                            |   9 +
      drivers/usb/dwc3/core.h                            |  15 ++
      drivers/usb/dwc3/dwc3-qcom.c                       |   9 +
      drivers/usb/dwc3/ep0.c                             |   2 +
      drivers/usb/dwc3/gadget.c                          | 212
+++++++++++++++++++++
      drivers/usb/gadget/configfs.c                      |  22 +++
      drivers/usb/gadget/udc/core.c                      |  25 +++
      include/linux/of.h                                 |   5 +
      include/linux/usb/gadget.h                         |   5 +
      10 files changed, 317 insertions(+), 2 deletions(-)

Hi

Op 23-06-2021 om 09:50 schreef Wesley Cheng:

Quoted 135 lines · Ferry Toth

Quoted 109 lines

Hi

Op 23-06-2021 om 00:00 schreef Wesley Cheng:

Quoted 98 lines · Ferry Toth

Quoted 97 lines

Hi

Op 22-06-2021 om 20:38 schreef Wesley Cheng:

Quoted 94 lines · Ferry Toth

Quoted 73 lines

Hi

Op 18-06-2021 om 00:25 schreef Wesley Cheng:

Quoted 29 lines

Hi,

Quoted 26 lines · Ferry Toth

Hi

Op 17-06-2021 om 23:48 schreef Wesley Cheng:

Quoted 23 lines

Hi,

Quoted 12 lines · Ferry Toth

Hi

Op 17-06-2021 om 11:58 schreef Wesley Cheng:

Quoted 9 lines

Changes in V10:
      - Fixed compilation errors in config where OF is not used (error due to
        unknown symbol for of_add_property()).  Add of_add_property()
stub.
      - Fixed compilation warning for incorrect argument being passed to
dwc3_mdwidth

Quoted 3 lines

This fixes the OOPS I had in V9. I do not see any change in performance
on Merrifield though.

I see...thanks Ferry! With your testing, are you writing to the device's
internal storage (ie UFS, eMMC, etc...), or did you use a ramdisk as
well?

Quoted 20 lines

Quoted 2 lines

In this case I just tested the EEM path using iperf3.

Got it.  I don't believe f_eem will use a high enough (if at all) bMaxBurst value to change the TXFIFO size.

Quoted 15 lines

Quoted 10 lines

If not with a ramdisk, we might want to give that a try to avoid the
storage path being the bottleneck.  You can use "dd" to create an empty
file, and then just use that as the LUN's backing file.

echo ramdisk.img > /sys/kernel/config/usb_gadget/g1/functions/mass_storage.0/lun.0/file

Ah, why didn't I think of that. I have currently mass storage setup
with
eMMC but it seems that is indeed the bottleneck.

I created a 64MB disk following the instructions here
http://www.linux-usb.org/gadget/file_storage.html (that seems a little
outdated, at least I can not start the first partition at sector 8, but
minimum 2048), and added a test file on it.

I then copy the file to /dev/shm prior to setting configfs (composite
device gser/eem/mass_storage/uac2).

journal shows:

kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)

I don't know what that means, because I see the test file on the ramdisk.

Then I again used gnome disks to benchmark (read/write 10MB):

Quoted 7 lines

With V10 on top v5.13.0-rc5:

R/W speed = 35.6/35.8MB/s, access time 0.35ms

With no patches on top v5.12.0:

R/W speed = 35.7/36.1MB/s, access time 0.35ms

Hi Ferry,

Quoted 3 lines

I see no speed difference (and it's about the same as with the eMMC backed disk). But the patches are causing a new call trace

Would you happen to know what DWC3 controller revision the device is using?  The callstack print occurs, because it looks like it ran out of
internal memory, although there should be logic present for making sure
that at least there is enough room for 1 FIFO per endpoint. (possibly
the logic/math depends on the controller revision)

Quoted 6 lines

Hi Ferry,

Quoted 2 lines

Do you know where I could find that in a file on the device?

Maybe you can just dump the DWC3 registers? cat /sys/kernel/debug/usb/<controller name>/regdump

This isin host mode:

Quoted 15 lines

~# cat /sys/kernel/debug/usb/dwc3.0.auto/regdump
GSBUSCFG0 = 0x00000006
GSBUSCFG1 = 0x00000f00
GTXTHRCFG = 0x230a0000
GRXTHRCFG = 0x22800000
GCTL = 0x45801002
GEVTEN = 0x00000000
GSTS = 0x3e800001
GUCTL1 = 0x00000000
GSNPSID = 0x5533250a
GGPIO = 0x00000000
GUID = 0x00050d00
GUCTL = 0x0200ce00
GBUSERRADDR0 = 0x00000000
GBUSERRADDR1 = 0x00000000

...
Hi Ferry,

Great!  Thanks, that got me the information regarding the DWC3 revision you're using. (its version 2.50a)  I checked the DWC3 databook and looks like certain versions have some different math to calculate the TXFIFO size.  I have taken this into account and factored that in in the next revision.

Quoted 3 lines

Is there a way you could collect the regdump in device mode w/ mass storage only composition after enumerating with the PC?  That would help confirm how the TXFIFO resizing logic is working on your platform.

Sorry for the wait. Yes I have serial console. So here is in device mode, mass_storage only, enumerated but not mounted:

GSBUSCFG0 = 0x00000006
GSBUSCFG1 = 0x00000f00
GTXTHRCFG = 0x230a0000
GRXTHRCFG = 0x02800000
GCTL = 0x45802002
GEVTEN = 0x00000000

Quoted 356 lines

Thanks
Wesley Cheng

Quoted 353 lines

Quoted 353 lines

Was going to ask for the same to confirm the TXFIFO sizes from your results below :).

Quoted 10 lines

Otherwise, I'm hoping Andy will know?

Quoted 6 lines

Also, is there a way to use just a mass storage only composition? Based
on the above observation, that probably means that the mass storage interface wasn't resized at all, because the configuration took up a lot
of the internal FIFO space.

Sure, it's configured through configfs. With only mass_storage I have:

Quoted 8 lines

With V10 on top v5.13.0-rc5:

R/W speed = 41,6/39,3MB/s, access time 0.33ms

With no patches on top v5.12.0:

R/W speed = 41,1/38,7MB/s, access time 0.38ms

Thanks Ferry!  Could you collect the regdump, so I can confirm the two things mentioned?

Thanks
Wesley Cheng

Quoted 326 lines

Quoted 326 lines

Thanks
Wesley Cheng

Quoted 323 lines

kernel: using random self ethernet address
kernel: using random host ethernet address
kernel: Mass Storage Function, version: 2009/09/11
kernel: LUN: removable file: (no medium)
kernel: usb0: HOST MAC aa:bb:cc:dd:ee:f2
kernel: usb0: MAC aa:bb:cc:dd:ee:f1
kernel: IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: ------------[ cut here ]------------
kernel: WARNING: CPU: 0 PID: 594 at drivers/usb/gadget/udc/core.c:278
usb_ep_queue+0x75/0x80
kernel: Modules linked in: usb_f_uac2 u_audio usb_f_mass_storage usb_f_eem u_ether usb_f_serial u_serial libcomposite rfcomm iptable_nat
bnep snd_sof_nocodec spi_pxa2>
kernel: CPU: 0 PID: 594 Comm: irq/14-dwc3 Not tainted 5.13.0-rc5-edison-acpi-standard #1
kernel: Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542
2015.01.21:18.19.48
kernel: RIP: 0010:usb_ep_queue+0x75/0x80
kernel: Code: 01 73 e4 48 8b 05 fb 63 06 01 48 85 c0 74 12 48 8b 78 08
44 89 e9 4c 89 e2 48 89 ee e8 74 05 00 00 44 89 e8 5d 41 5c 41 5d c3 <0f> 0b 41 bd 94 ff ff ff >
kernel: RSP: 0000:ffff91eec083fc98 EFLAGS: 00010082
kernel: RAX: ffff8af20357d960 RBX: 0000000000000000 RCX:
ffff8af202f06400
kernel: RDX: 0000000000000a20 RSI: ffff8af208785780 RDI:
ffff8af202e9ae00
kernel: RBP: ffff8af202e9ae00 R08: 00000000000000c0 R09:
ffff8af208785780
kernel: R10: 00000000ffffe000 R11: 3fffffffffffffff R12:
ffff8af208785780
kernel: R13: 0000000000000000 R14: ffff8af202e9ae00 R15:
ffff8af203e26cc0
kernel: FS:  0000000000000000(0000) GS:ffff8af23e200000(0000) knlGS:0000000000000000
kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 000055e2c21f2100 CR3: 0000000003b38000 CR4:
00000000001006f0
kernel: Call Trace:
kernel:  u_audio_start_playback+0x107/0x1a0 [u_audio] kernel:  composite_setup+0x224/0x1ba0 [libcomposite] kernel:  ? dwc3_gadget_ep_queue+0xf6/0x1a0
kernel:  ? usb_ep_queue+0x2a/0x80
kernel:  ? configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  configfs_composite_setup+0x6b/0x90 [libcomposite] kernel:  dwc3_ep0_interrupt+0x469/0xa80
kernel:  dwc3_thread_interrupt+0x8ee/0xf40
kernel:  ? __wake_up_common_lock+0x85/0xb0
kernel:  ? disable_irq_nosync+0x10/0x10
kernel:  irq_thread_fn+0x1b/0x60
kernel:  irq_thread+0xd6/0x170
kernel:  ? irq_thread_check_affinity+0x70/0x70
kernel:  ? irq_forced_thread_fn+0x70/0x70
kernel:  kthread+0x116/0x130
kernel:  ? kthread_create_worker_on_cpu+0x60/0x60
kernel:  ret_from_fork+0x22/0x30
kernel: ---[ end trace e5b9e28058c53584 ]---
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000036ac129 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 00000000fbc71244 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000ad1b8c18 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: Fifosize(2154) > RAM size(2022) ep5in depth:115540359
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: configfs-gadget gadget: u_audio_start_playback:451 Error!
kernel: dwc3 dwc3.0.auto: request 000000003c32dcc5 was not queued to ep5in
kernel: dwc3 dwc3.0.auto: request 00000000b2512aa9 was not queued to ep5in

Removing uac2 from the config makes the call trace go away, but the R/W
speed does not change.

Quoted 218 lines

:), not a problem...I've been working on getting the ideal set up for
the performance profiling for awhile, so anything I can do to make sure
we get some good results.

Quoted 2 lines

I'll try with a ramdisk and let you know.

Thanks again for the testing, Ferry.

Thanks
Wesley Cheng

Quoted 205 lines

Quoted 205 lines

Thanks
Wesley Cheng

Quoted 202 lines

Quoted 202 lines

Changes in V9:
      - Fixed incorrect patch in series.  Removed changes in DTSI, as
dwc3-qcom will
        add the property by default from the kernel.

Changes in V8:
      - Rebased to usb-testing
      - Using devm_kzalloc for adding txfifo property in dwc3-qcom
      - Removed DWC3 QCOM ACPI property for enabling the txfifo resize

Changes in V7:
      - Added a new property tx-fifo-max-num for limiting how much
fifo
space the
        resizing logic can allocate for endpoints with large burst
values.  This
        can differ across platforms, and tie in closely with overall
system latency.
      - Added recommended checks for DWC32.
      - Added changes to set the tx-fifo-resize property from dwc3-qcom by
default
        instead of modifying the current DTSI files.
      - Added comments on all APIs/variables introduced.
      - Updated the DWC3 YAML to include a better description of the
tx-fifo-resize
        property and added an entry for tx-fifo-max-num.

Changes in V6:
      - Rebased patches to usb-testing.
      - Renamed to PATCH series instead of RFC.
      - Checking for fs_descriptors instead of ss_descriptors for
determining the
        endpoint count for a particular configuration.
      - Re-ordered patch series to fix patch dependencies.

Changes in V5:
      - Added check_config() logic, which is used to communicate the
number of EPs
        used in a particular configuration.  Based on this, the DWC3
gadget driver
        has the ability to know the maximum number of eps utilized in
all
configs.
        This helps reduce unnecessary allocation to unused eps, and will
catch fifo
        allocation issues at bind() time.
      - Fixed variable declaration to single line per variable, and
reverse xmas.
      - Created a helper for fifo clearing, which is used by ep0.c

Changes in V4:
      - Removed struct dwc3* as an argument for dwc3_gadget_resize_tx_fifos()
      - Removed WARN_ON(1) in case we run out of fifo space Changes in V3:
      - Removed "Reviewed-by" tags
      - Renamed series back to RFC
      - Modified logic to ensure that fifo_size is reset if we pass the
minimum
        threshold.  Tested with binding multiple FDs requesting 6
FIFOs.

Changes in V2:
      - Modified TXFIFO resizing logic to ensure that each EP is
reserved a
        FIFO.
      - Removed dev_dbg() prints and fixed typos from patches
      - Added some more description on the dt-bindings commit message

Currently, there is no functionality to allow for resizing the TXFIFOs, and
relying on the HW default setting for the TXFIFO depth.  In most
cases, the
HW default is probably sufficient, but for USB compositions that
contain
multiple functions that require EP bursting, the default settings
might not be enough.  Also to note, the current SW will assign an
EP to a
function driver w/o checking to see if the TXFIFO size for that particular
EP is large enough. (this is a problem if there are multiple HW defined
values for the TXFIFO size)

It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3
is required for an EP that supports bursting.  Otherwise, there may be
frequent occurences of bursts ending.  For high bandwidth functions,
such as data tethering (protocols that support data aggregation),
mass
storage, and media transfer protocol (over FFS), the bMaxBurst value
can be
large, and a bigger TXFIFO depth may prove to be beneficial in terms
of USB
throughput. (which can be associated to system access latency, etc...)  It
allows for a more consistent burst of traffic, w/o any interruptions, as
data is readily available in the FIFO.

With testing done using the mass storage function driver, the results
show
that with a larger TXFIFO depth, the bandwidth increased significantly.

Test Parameters:
      - Platform: Qualcomm SM8150
      - bMaxBurst = 6
      - USB req size = 256kB
      - Num of USB reqs = 16
      - USB Speed = Super-Speed
      - Function Driver: Mass Storage (w/ ramdisk)
      - Test Application: CrystalDiskMark

Results:

TXFIFO Depth = 3 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 193.60

          |           | 195.86
               |           | 184.77
               |           | 193.60

-------------------------------------------

TXFIFO Depth = 6 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     |
Read      |9 loops    | 287.35

          |           | 304.94
               |           | 289.64
               |           | 293.61

-------------------------------------------

Wesley Cheng (6):
       usb: gadget: udc: core: Introduce check_config to verify USB
         configuration
       usb: gadget: configfs: Check USB configuration before adding
       usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
       of: Add stub for of_add_property()
       usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default
       dt-bindings: usb: dwc3: Update dwc3 TX fifo properties

      .../devicetree/bindings/usb/snps,dwc3.yaml         | 15 +-
      drivers/usb/dwc3/core.c                            | 9 +
      drivers/usb/dwc3/core.h                            | 15 ++
      drivers/usb/dwc3/dwc3-qcom.c                       | 9 +
      drivers/usb/dwc3/ep0.c                             | 2 +
      drivers/usb/dwc3/gadget.c                          | 212
+++++++++++++++++++++
      drivers/usb/gadget/configfs.c                      | 22 +++
      drivers/usb/gadget/udc/core.c                      | 25 +++
      include/linux/of.h                                 | 5 +
      include/linux/usb/gadget.h                         | 5 +

      10 files changed, 317 insertions(+), 2 deletions(-)