2020-08-29 06:00:33

by Wesley Cheng

[permalink] [raw]
Subject: [RFC v5 0/6] Re-introduce TX FIFO resize for larger EP bursting

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: dwc3: Resize TX FIFOs to meet EP bursting requirements
arm64: boot: dts: qcom: sm8150: Enable dynamic TX FIFO resize logic
dt-bindings: usb: dwc3: Add entry for tx-fifo-resize
usb: gadget: configfs: Check USB configuration before adding
usb: gadget: udc: core: Introduce check_config to verify USB
configuration
usb: dwc3: gadget: Ensure enough TXFIFO space for USB configuration

.../devicetree/bindings/usb/dwc3.txt | 2 +-
arch/arm64/boot/dts/qcom/sm8150.dtsi | 1 +
drivers/usb/dwc3/core.c | 2 +
drivers/usb/dwc3/core.h | 7 +
drivers/usb/dwc3/ep0.c | 2 +
drivers/usb/dwc3/gadget.c | 194 ++++++++++++++++++
drivers/usb/gadget/configfs.c | 22 ++
drivers/usb/gadget/udc/core.c | 9 +
include/linux/usb/gadget.h | 2 +
9 files changed, 240 insertions(+), 1 deletion(-)

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


2020-08-29 06:03:10

by Wesley Cheng

[permalink] [raw]
Subject: [RFC v5 5/6] usb: gadget: udc: core: Introduce check_config to verify USB configuration

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 | 9 +++++++++
include/linux/usb/gadget.h | 2 ++
2 files changed, 11 insertions(+)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index c33ad8a333ad..e006d69dff9b 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1001,6 +1001,15 @@ int usb_gadget_ep_match_desc(struct usb_gadget *gadget,
}
EXPORT_SYMBOL_GPL(usb_gadget_ep_match_desc);

+int usb_gadget_check_config(struct usb_gadget *gadget, unsigned long ep_map)
+{
+ if (!gadget->ops->check_config)
+ return 0;
+
+ return gadget->ops->check_config(gadget, ep_map);
+}
+EXPORT_SYMBOL_GPL(usb_gadget_check_config);
+
/* ------------------------------------------------------------------------- */

static void usb_gadget_state_work(struct work_struct *work)
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 52ce1f6b8f83..791ae5b352a1 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -326,6 +326,7 @@ struct usb_gadget_ops {
struct usb_ep *(*match_ep)(struct usb_gadget *,
struct usb_endpoint_descriptor *,
struct usb_ss_ep_comp_descriptor *);
+ int (*check_config)(struct usb_gadget *gadget, unsigned long ep_map);
};

/**
@@ -575,6 +576,7 @@ int usb_gadget_connect(struct usb_gadget *gadget);
int usb_gadget_disconnect(struct usb_gadget *gadget);
int usb_gadget_deactivate(struct usb_gadget *gadget);
int usb_gadget_activate(struct usb_gadget *gadget);
+int usb_gadget_check_config(struct usb_gadget *gadget, unsigned long ep_map);
#else
static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
{ return 0; }
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2020-08-29 06:03:10

by Wesley Cheng

[permalink] [raw]
Subject: [RFC v5 6/6] usb: dwc3: gadget: Ensure enough TXFIFO space for USB configuration

If TXFIFO resizing is enabled, then based on if endpoint bursting is
required or not, a larger amount of FIFO space is benefical. Sometimes
a particular interface can take all the available FIFO space, leading
to other interfaces not functioning properly. This callback ensures that
the minimum fifo requirements, a single fifo per endpoint, can be met,
otherwise the configuration binding will fail. This will be based on the
maximum number of eps existing in all configurations.

Signed-off-by: Wesley Cheng <[email protected]>
---
drivers/usb/dwc3/core.h | 1 +
drivers/usb/dwc3/gadget.c | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 36 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index e85c1ec70cc3..0559b0a82c4d 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1249,6 +1249,7 @@ struct dwc3 {
u16 imod_interval;
int last_fifo_depth;
int num_ep_resized;
+ int max_cfg_eps;
};

#define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 53e5220f9893..e8f7ea560920 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2411,6 +2411,7 @@ static int dwc3_gadget_stop(struct usb_gadget *g)

out:
dwc->gadget_driver = NULL;
+ dwc->max_cfg_eps = 0;
spin_unlock_irqrestore(&dwc->lock, flags);

free_irq(dwc->irq_gadget, dwc->ev_buf);
@@ -2518,6 +2519,39 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
spin_unlock_irqrestore(&dwc->lock, flags);
}

+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->needs_fifo_resize)
+ return 0;
+
+ /* Only interested in the IN endpoints */
+ in_ep_map = ep_map >> 16;
+ ep_num = hweight_long(in_ep_map);
+
+ if (ep_num <= dwc->max_cfg_eps)
+ return 0;
+
+ /* Update the max number of eps in the composition */
+ dwc->max_cfg_eps = ep_num;
+
+ fifo_size = dwc3_gadget_calc_tx_fifo_size(dwc, dwc->max_cfg_eps);
+ /* Based on the equation, increment by one for every ep */
+ fifo_size += dwc->max_cfg_eps;
+
+ /* Check if we can fit a single fifo per endpoint */
+ ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7);
+ if (fifo_size > ram1_depth)
+ return -ENOMEM;
+
+ return 0;
+}
+
static const struct usb_gadget_ops dwc3_gadget_ops = {
.get_frame = dwc3_gadget_get_frame,
.wakeup = dwc3_gadget_wakeup,
@@ -2527,6 +2561,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
.udc_stop = dwc3_gadget_stop,
.udc_set_speed = dwc3_gadget_set_speed,
.get_config_params = dwc3_gadget_config_params,
+ .check_config = dwc3_gadget_check_config,
};

/* -------------------------------------------------------------------------- */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2020-08-29 06:03:38

by Wesley Cheng

[permalink] [raw]
Subject: [RFC v5 2/6] arm64: boot: dts: qcom: sm8150: Enable dynamic TX FIFO resize logic

Enable the flexible TX FIFO resize logic on SM8150. Using a larger TX FIFO
SZ can help account for situations when system latency is greater than the
USB bus transmission latency.

Signed-off-by: Wesley Cheng <[email protected]>
---
arch/arm64/boot/dts/qcom/sm8150.dtsi | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
index 7a0c5b419ff0..169ac4c8e298 100644
--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
@@ -717,6 +717,7 @@ usb_1_dwc3: dwc3@a600000 {
interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
snps,dis_u2_susphy_quirk;
snps,dis_enblslpm_quirk;
+ tx-fifo-resize;
phys = <&usb_1_hsphy>, <&usb_1_ssphy>;
phy-names = "usb2-phy", "usb3-phy";
};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project