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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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