Re: [PATCH] usb: dwc3: fix endpoint direction when inputs are more than outputs

From: Thinh Nguyen
Date: Thu Sep 09 2021 - 19:43:39 EST

Shantur Rathore wrote:
> In RK3399 as per documentation (
> https://urldefense.com/v3/__https://usermanual.wiki/Document/RockchipDeveloperGuidelinux44USB.31610806__;!!A4F2R9G_pg!JqYr6U87SL3rYylirF6W2vwNzC0Ft8YiZwTlMTwWl7bpaHGZuh-JMfOvUaG781GKZIBd$
> ), there are 7 Input Endpoints and 6 Output endpoints, in total 13
> endpoints.
>
> Currently dwc3/gadget.c driver uses the number of endpoints
> available and starts setting them up with even endpoints as output
> endpoints and odd numbered as even endpoints. This leads to 7 Output
> endpoints and 6 input endpoints for RK3399.
>
> If you try to create a composite gadget which uses all the input
> endpoints, one can see the issue. You just need to create functions to
> use up the last input ep and it would fail to create. No need to
> connect it to the host.
> This was confirmed when running a rockchip-linux bsp image.
>
> [root@rockpro rock]# ls /sys/kernel/debug/usb/fe800000.usb/
> ep0in ep0out ep1in ep1out ep2in ep2out ep3in ep3out ep4in
> ep4out ep5in ep5out ep6in link_state lsp_dump mode regdump
> testmode
>
> Currently in linux mainline it is
>
> [root@rockpro rock]# ls /sys/kernel/debug/usb/fe800000.usb/
> ep0in ep0out ep1in ep1out ep2in ep2out ep3in ep3out ep4in
> ep4out ep5in ep5out ep6out link_state lsp_dump mode regdump
> testmode
>
> ep6 being out instead of in as per the hardware spec.
>
> Upon investigation of rockchip bsp kernel,
> https://urldefense.com/v3/__https://github.com/rockchip-linux/kernel/__;!!A4F2R9G_pg!JqYr6U87SL3rYylirF6W2vwNzC0Ft8YiZwTlMTwWl7bpaHGZuh-JMfOvUaG788DHJSUE$
>
> The issue was clear, currently, dwc3/gadget driver doesn't take
> DWC3_NUM_IN_EPS into consideration while enumerating them.
>
> The patch below fixes the issue and ep6 is correctly enumerated as input.

No Signed-of-by?

> ---
> drivers/usb/dwc3/core.c | 1 +
> drivers/usb/dwc3/core.h | 1 +
> drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++---------------
> 3 files changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 01866dcb953b..279c9a97cb8c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -555,6 +555,7 @@ static void dwc3_core_num_eps(struct dwc3 *dwc)
> struct dwc3_hwparams *parms = &dwc->hwparams;
>
> dwc->num_eps = DWC3_NUM_EPS(parms);
> + dwc->num_in_eps = DWC3_NUM_IN_EPS(parms);
> }
>
> static void dwc3_cache_hwparams(struct dwc3 *dwc)
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5612bfdf37da..89a0998c618c 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1227,6 +1227,7 @@ struct dwc3 {
> u8 speed;
>
> u8 num_eps;
> + u8 num_in_eps;
>
> struct dwc3_hwparams hwparams;
> struct debugfs_regset32 *regset;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 804b50548163..d9d19dc0a29f 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -693,9 +693,11 @@ void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
>
> dwc->last_fifo_depth = fifo_depth;
> /* Clear existing TXFIFO for all IN eps except ep0 */
> - for (num = 3; num < min_t(int, dwc->num_eps, DWC3_ENDPOINTS_NUM);
> - num += 2) {
> + for (num = 3; num < DWC3_ENDPOINTS_NUM; num += 2) {
> dep = dwc->eps[num];
> +
> + if(!dep)
> + continue;
> /* Don't change TXFRAMNUM on usb31 version */
> size = DWC3_IP_IS(DWC3) ? 0 :
> dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1)) &
> @@ -2257,7 +2259,7 @@ static void dwc3_stop_active_transfers(struct dwc3 *dwc)
> {
> u32 epnum;
>
> - for (epnum = 2; epnum < dwc->num_eps; epnum++) {
> + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
> struct dwc3_ep *dep;
>
> dep = dwc->eps[epnum];
> @@ -2960,10 +2962,9 @@ static int dwc3_gadget_init_out_endpoint(struct dwc3_ep *dep)
> return dwc3_alloc_trb_pool(dep);
> }
>
> -static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum)
> +static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum, bool direction)
> {
> struct dwc3_ep *dep;
> - bool direction = epnum & 1;
> int ret;
> u8 num = epnum >> 1;
>
> @@ -3011,21 +3012,30 @@ static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum)
> return 0;
> }
>
> -static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 total)
> +static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 total, u8 num_in_eps)
> {
> - u8 epnum;
> + u8 num;
> + int ret;
>
> INIT_LIST_HEAD(&dwc->gadget->ep_list);
>
> - for (epnum = 0; epnum < total; epnum++) {
> - int ret;
> + /* init input endpoints as reported by hw */
> + for (num = 0; num < num_in_eps; num++) {
>
> - ret = dwc3_gadget_init_endpoint(dwc, epnum);
> - if (ret)
> - return ret;
> - }
> + ret = dwc3_gadget_init_endpoint(dwc, (num << 1) + 1, 1);
> + if (ret)
> + return ret;
> + }
>
> - return 0;
> + /* init rest endpoints as output endpoints */
> + for (num = 0; num < total - num_in_eps; num++) {
> +
> + ret = dwc3_gadget_init_endpoint(dwc, num << 1, 0);
> + if (ret)
> + return ret;
> + }
> +
> + return ret;
> }
>

* DWC3_NUM_EPS(parms) is the total number of endpoints configured in HW
* DWC3_NUM_IN_EPS(parms) is the max number of IN endpoints that SW may
configure

The number of OUT endpoints does not mean DWC3_NUM_EPS(parms) -
DWC3_NUM_IN_EPS(parms).

As long as physical endpoint 0 and 1 are dedicated for control endpoint,
other endpoints can be assigned as IN or OUT direction. So, you can have
as many as DWC3_NUM_EPS(parms) - 1 number of OUT endpoints.

Currently, dwc3 driver assumes that DWC3_NUM_IN_EPS(params) is at least
half of DWC3_NUM_EPS(parms). If that's not the case, we may see
problems. To cover most application setup, the driver tries to setup
number of OUT = IN.

For your case, is there an application that needs all
DWC3_NUM_IN_EPS(params)? If you're going to make a change, please keep
in mind of the info above to prevent regression.

Thanks,
Thinh