[PATCH] usb: dwc3: gadget: Init only available HW eps

[PATCH] usb: dwc3: gadget: Init only available HW eps

[Thinh Nguyen]

Typically FPGA devices are configured with CoreConsultant parameter
DWC_USB3x_EN_LOG_PHYS_EP_SUPT=0 to reduce gate count and improve timing.
This means that the number of INs equals to OUTs endpoints. But
typically non-FPGA devices enable this CoreConsultant parameter to
support flexible endpoint mapping and potentially may have unequal
number of INs to OUTs physical endpoints.

The driver must check how many physical endpoints are available for each
direction and initialize them properly.

Cc: stable@vger.kernel.org
Fixes: 47d3946ea220 ("usb: dwc3: refactor gadget endpoint count calculation")
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/core.c   |  1 +
 drivers/usb/dwc3/core.h   |  2 ++
 drivers/usb/dwc3/gadget.c | 19 ++++++++++++-------
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 841daec70b6e..1084aa8623c2 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -529,6 +529,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 1b241f937d8f..1295dac019f9 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -990,6 +990,7 @@ struct dwc3_scratchpad_array {
  * @u1sel: parameter from Set SEL request.
  * @u1pel: parameter from Set SEL request.
  * @num_eps: number of endpoints
+ * @num_in_eps: number of IN endpoints
  * @ep0_next_event: hold the next expected event
  * @ep0state: state of endpoint zero
  * @link_state: link state
@@ -1193,6 +1194,7 @@ struct dwc3 {
 	u8			speed;
 
 	u8			num_eps;
+	u8			num_in_eps;
 
 	struct dwc3_hwparams	hwparams;
 	struct dentry		*root;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 25f654b79e48..8a38ee10c00b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2025,7 +2025,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];
@@ -2628,16 +2628,21 @@ 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				epnum;
+	u8				i;
+	int				ret;
 
 	INIT_LIST_HEAD(&dwc->gadget->ep_list);
 
-	for (epnum = 0; epnum < total; epnum++) {
-		int			ret;
+	for (i = 0; i < dwc->num_in_eps; i++) {
+		ret = dwc3_gadget_init_endpoint(dwc, i * 2 + 1);
+		if (ret)
+			return ret;
+	}
 
-		ret = dwc3_gadget_init_endpoint(dwc, epnum);
+	for (i = 0; i < dwc->num_eps - dwc->num_in_eps; i++) {
+		ret = dwc3_gadget_init_endpoint(dwc, i * 2);
 		if (ret)
 			return ret;
 	}
@@ -3863,7 +3868,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 	 * sure we're starting from a well known location.
 	 */
 
-	ret = dwc3_gadget_init_endpoints(dwc, dwc->num_eps);
+	ret = dwc3_gadget_init_endpoints(dwc);
 	if (ret)
 		goto err4;
 

base-commit: 96ebc9c871d8a28fb22aa758dd9188a4732df482
-- 
2.28.0

Re: [PATCH] usb: dwc3: gadget: Init only available HW eps

[Felipe Balbi]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> Typically FPGA devices are configured with CoreConsultant parameter
> DWC_USB3x_EN_LOG_PHYS_EP_SUPT=0 to reduce gate count and improve timing.
> This means that the number of INs equals to OUTs endpoints. But
> typically non-FPGA devices enable this CoreConsultant parameter to
> support flexible endpoint mapping and potentially may have unequal
> number of INs to OUTs physical endpoints.
>
> The driver must check how many physical endpoints are available for each
> direction and initialize them properly.
>
> Cc: stable@vger.kernel.org
> Fixes: 47d3946ea220 ("usb: dwc3: refactor gadget endpoint count calculation")
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/usb/dwc3/core.c   |  1 +
>  drivers/usb/dwc3/core.h   |  2 ++
>  drivers/usb/dwc3/gadget.c | 19 ++++++++++++-------
>  3 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 841daec70b6e..1084aa8623c2 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -529,6 +529,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 1b241f937d8f..1295dac019f9 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -990,6 +990,7 @@ struct dwc3_scratchpad_array {
>   * @u1sel: parameter from Set SEL request.
>   * @u1pel: parameter from Set SEL request.
>   * @num_eps: number of endpoints
> + * @num_in_eps: number of IN endpoints
>   * @ep0_next_event: hold the next expected event
>   * @ep0state: state of endpoint zero
>   * @link_state: link state
> @@ -1193,6 +1194,7 @@ struct dwc3 {
>  	u8			speed;
>  
>  	u8			num_eps;
> +	u8			num_in_eps;
>  
>  	struct dwc3_hwparams	hwparams;
>  	struct dentry		*root;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 25f654b79e48..8a38ee10c00b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2025,7 +2025,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];
> @@ -2628,16 +2628,21 @@ 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				epnum;
> +	u8				i;
> +	int				ret;
>  
>  	INIT_LIST_HEAD(&dwc->gadget->ep_list);
>  
> -	for (epnum = 0; epnum < total; epnum++) {
> -		int			ret;
> +	for (i = 0; i < dwc->num_in_eps; i++) {
> +		ret = dwc3_gadget_init_endpoint(dwc, i * 2 + 1);
> +		if (ret)
> +			return ret;
> +	}
>  
> -		ret = dwc3_gadget_init_endpoint(dwc, epnum);
> +	for (i = 0; i < dwc->num_eps - dwc->num_in_eps; i++) {
> +		ret = dwc3_gadget_init_endpoint(dwc, i * 2);
>  		if (ret)
>  			return ret;
>  	}
> @@ -3863,7 +3868,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>  	 * sure we're starting from a well known location.
>  	 */
>  
> -	ret = dwc3_gadget_init_endpoints(dwc, dwc->num_eps);
> +	ret = dwc3_gadget_init_endpoints(dwc);
>  	if (ret)
>  		goto err4;

heh, looking at original commit, we used to have num_in_eps and
num_out_eps. In fact, this commit will reintroduce another problem that
Bryan tried to solve. num_eps - num_in_eps is not necessarily
num_out_eps.

How have you verified this patch? Did you read Bryan's commit log? This
is likely to reintroduce the problem raised by Bryan.

-- 
balbi

Re: [PATCH] usb: dwc3: gadget: Init only available HW eps

[Thinh Nguyen]

Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Typically FPGA devices are configured with CoreConsultant parameter
>> DWC_USB3x_EN_LOG_PHYS_EP_SUPT=0 to reduce gate count and improve timing.
>> This means that the number of INs equals to OUTs endpoints. But
>> typically non-FPGA devices enable this CoreConsultant parameter to
>> support flexible endpoint mapping and potentially may have unequal
>> number of INs to OUTs physical endpoints.
>>
>> The driver must check how many physical endpoints are available for each
>> direction and initialize them properly.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 47d3946ea220 ("usb: dwc3: refactor gadget endpoint count calculation")
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>>  drivers/usb/dwc3/core.c   |  1 +
>>  drivers/usb/dwc3/core.h   |  2 ++
>>  drivers/usb/dwc3/gadget.c | 19 ++++++++++++-------
>>  3 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 841daec70b6e..1084aa8623c2 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -529,6 +529,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 1b241f937d8f..1295dac019f9 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -990,6 +990,7 @@ struct dwc3_scratchpad_array {
>>   * @u1sel: parameter from Set SEL request.
>>   * @u1pel: parameter from Set SEL request.
>>   * @num_eps: number of endpoints
>> + * @num_in_eps: number of IN endpoints
>>   * @ep0_next_event: hold the next expected event
>>   * @ep0state: state of endpoint zero
>>   * @link_state: link state
>> @@ -1193,6 +1194,7 @@ struct dwc3 {
>>  	u8			speed;
>>  
>>  	u8			num_eps;
>> +	u8			num_in_eps;
>>  
>>  	struct dwc3_hwparams	hwparams;
>>  	struct dentry		*root;
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 25f654b79e48..8a38ee10c00b 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2025,7 +2025,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];
>> @@ -2628,16 +2628,21 @@ 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				epnum;
>> +	u8				i;
>> +	int				ret;
>>  
>>  	INIT_LIST_HEAD(&dwc->gadget->ep_list);
>>  
>> -	for (epnum = 0; epnum < total; epnum++) {
>> -		int			ret;
>> +	for (i = 0; i < dwc->num_in_eps; i++) {
>> +		ret = dwc3_gadget_init_endpoint(dwc, i * 2 + 1);
>> +		if (ret)
>> +			return ret;
>> +	}
>>  
>> -		ret = dwc3_gadget_init_endpoint(dwc, epnum);
>> +	for (i = 0; i < dwc->num_eps - dwc->num_in_eps; i++) {
>> +		ret = dwc3_gadget_init_endpoint(dwc, i * 2);
>>  		if (ret)
>>  			return ret;
>>  	}
>> @@ -3863,7 +3868,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>  	 * sure we're starting from a well known location.
>>  	 */
>>  
>> -	ret = dwc3_gadget_init_endpoints(dwc, dwc->num_eps);
>> +	ret = dwc3_gadget_init_endpoints(dwc);
>>  	if (ret)
>>  		goto err4;
> heh, looking at original commit, we used to have num_in_eps and
> num_out_eps. In fact, this commit will reintroduce another problem that
> Bryan tried to solve. num_eps - num_in_eps is not necessarily
> num_out_eps.
>

From my understanding, that's not what Bryan's saying. Here's the
snippet of the commit message:

"
    It's possible to configure RTL such that DWC_USB3_NUM_EPS is equal to
    DWC_USB3_NUM_IN_EPS.

    dwc3-core calculates the number of OUT endpoints as DWC_USB3_NUM minus
    DWC_USB3_NUM_IN_EPS. If RTL has been configured with DWC_USB3_NUM_IN_EPS
    equal to DWC_USB3_NUM then dwc3-core will calculate the number of OUT
    endpoints as zero.

    For example a from dwc3_core_num_eps() shows:
    [    1.565000]  /usb0@f01d0000: found 8 IN and 0 OUT endpoints
"

He just stated that you can configure to have num_eps equals to
num_in_eps. Basically it means there's no OUT physical endpoint. Not
sure why you would ever want to do that because that will prevent any
device from working. The reason we have DWC_USB3x_NUM_IN_EPS and
DWC_USB3x_NUM_EPS exposed in the global HW param so that the driver know
how many endpoints are available for each direction. If for some reason
this mechanism fails, there's something fundamentally wrong in the HW
configuration. We have not seen this problem, and I don't think Bryan
did from his commit statement either.

> How have you verified this patch? Did you read Bryan's commit log? This
> is likely to reintroduce the problem raised by Bryan.
>

We verified with our FPGA HAPS with various number of endpoints. No
issue is seen.

BR,
Thinh

Re: [PATCH] usb: dwc3: gadget: Init only available HW eps

[Felipe Balbi]

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

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> @@ -3863,7 +3868,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>>  	 * sure we're starting from a well known location.
>>>  	 */
>>>  
>>> -	ret = dwc3_gadget_init_endpoints(dwc, dwc->num_eps);
>>> +	ret = dwc3_gadget_init_endpoints(dwc);
>>>  	if (ret)
>>>  		goto err4;
>> heh, looking at original commit, we used to have num_in_eps and
>> num_out_eps. In fact, this commit will reintroduce another problem that
>> Bryan tried to solve. num_eps - num_in_eps is not necessarily
>> num_out_eps.
>>
>
> From my understanding, that's not what Bryan's saying. Here's the
> snippet of the commit message:
>
> "
>     It's possible to configure RTL such that DWC_USB3_NUM_EPS is equal to
>     DWC_USB3_NUM_IN_EPS.
>
>     dwc3-core calculates the number of OUT endpoints as DWC_USB3_NUM minus
>     DWC_USB3_NUM_IN_EPS. If RTL has been configured with DWC_USB3_NUM_IN_EPS
>     equal to DWC_USB3_NUM then dwc3-core will calculate the number of OUT
>     endpoints as zero.
>
>     For example a from dwc3_core_num_eps() shows:
>     [    1.565000]  /usb0@f01d0000: found 8 IN and 0 OUT endpoints
> "
>
> He just stated that you can configure to have num_eps equals to
> num_in_eps. Basically it means there's no OUT physical endpoint. Not

no, that's not what it means. I don't have access to DWC3 documentation
anymore, but from what I remember every physical endpoint _can_ be
configured as bidirectional. In other words, DWC3_USB3_NUM_EPS ==
DWC3_USB3_NUM_IN_EPS could mean that every endpoint in the system is
bidirectional.

> sure why you would ever want to do that because that will prevent any
> device from working. The reason we have DWC_USB3x_NUM_IN_EPS and
> DWC_USB3x_NUM_EPS exposed in the global HW param so that the driver know
> how many endpoints are available for each direction. If for some reason
> this mechanism fails, there's something fundamentally wrong in the HW
> configuration. We have not seen this problem, and I don't think Bryan
> did from his commit statement either.

Please confirm this internally. That was my original assumption too,
until Bryan pointed me to a particular section of the
specification. Unfortunately it's far too long ago and I can't even
verify documentation :-)

>> How have you verified this patch? Did you read Bryan's commit log? This
>> is likely to reintroduce the problem raised by Bryan.
>>
>
> We verified with our FPGA HAPS with various number of endpoints. No
> issue is seen.

That's cool. Could you please make sure our understanding of this is
sound and won't interfere with any designs? If we modify this part of
the code again, I'd like to see a clear reference to a specific section
of the databook detailing the expected behavior :-)

cheers

-- 
balbi

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

Re: [PATCH] usb: dwc3: gadget: Init only available HW eps

[Thinh Nguyen]

Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> @@ -3863,7 +3868,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>>>  	 * sure we're starting from a well known location.
>>>>  	 */
>>>>  
>>>> -	ret = dwc3_gadget_init_endpoints(dwc, dwc->num_eps);
>>>> +	ret = dwc3_gadget_init_endpoints(dwc);
>>>>  	if (ret)
>>>>  		goto err4;
>>> heh, looking at original commit, we used to have num_in_eps and
>>> num_out_eps. In fact, this commit will reintroduce another problem that
>>> Bryan tried to solve. num_eps - num_in_eps is not necessarily
>>> num_out_eps.
>>>
>> From my understanding, that's not what Bryan's saying. Here's the
>> snippet of the commit message:
>>
>> "
>>     It's possible to configure RTL such that DWC_USB3_NUM_EPS is equal to
>>     DWC_USB3_NUM_IN_EPS.
>>
>>     dwc3-core calculates the number of OUT endpoints as DWC_USB3_NUM minus
>>     DWC_USB3_NUM_IN_EPS. If RTL has been configured with DWC_USB3_NUM_IN_EPS
>>     equal to DWC_USB3_NUM then dwc3-core will calculate the number of OUT
>>     endpoints as zero.
>>
>>     For example a from dwc3_core_num_eps() shows:
>>     [    1.565000]  /usb0@f01d0000: found 8 IN and 0 OUT endpoints
>> "
>>
>> He just stated that you can configure to have num_eps equals to
>> num_in_eps. Basically it means there's no OUT physical endpoint. Not
> no, that's not what it means. I don't have access to DWC3 documentation
> anymore, but from what I remember every physical endpoint _can_ be
> configured as bidirectional. In other words, DWC3_USB3_NUM_EPS ==
> DWC3_USB3_NUM_IN_EPS could mean that every endpoint in the system is
> bidirectional.
>
>> sure why you would ever want to do that because that will prevent any
>> device from working. The reason we have DWC_USB3x_NUM_IN_EPS and
>> DWC_USB3x_NUM_EPS exposed in the global HW param so that the driver know
>> how many endpoints are available for each direction. If for some reason
>> this mechanism fails, there's something fundamentally wrong in the HW
>> configuration. We have not seen this problem, and I don't think Bryan
>> did from his commit statement either.
> Please confirm this internally. That was my original assumption too,
> until Bryan pointed me to a particular section of the
> specification. Unfortunately it's far too long ago and I can't even
> verify documentation :-)
>
>>> How have you verified this patch? Did you read Bryan's commit log? This
>>> is likely to reintroduce the problem raised by Bryan.
>>>
>> We verified with our FPGA HAPS with various number of endpoints. No
>> issue is seen.
> That's cool. Could you please make sure our understanding of this is
> sound and won't interfere with any designs? If we modify this part of
> the code again, I'd like to see a clear reference to a specific section
> of the databook detailing the expected behavior :-)
>
> cheers
>

Hm... I didn't consider bidirection endpoint other than control endpoint.

DWC3_USB3x_NUM_EPS specifies the number of device mode for single
directional endpoints. A bidirectional endpoint needs 2 single
directional endpoints, 1 IN and 1 OUT. So, if your setup uses 3
bidirection endpoints and only those, DWC3_USB3x_NUM_EPS should be 6.
DWC3_USB3x_NUM_IN_EPS specifies the maximum number of IN endpoint active
at any time.

However, I will have to double check and confirm internally regarding
how to determine many endpoint would be available if bidirection
endpoints come into play.

Thanks for pointing this out. Will get back on this.

Thinh

Re: [PATCH] usb: dwc3: gadget: Init only available HW eps

[Thinh Nguyen]

Thinh Nguyen wrote:
> Hi,
>
> Felipe Balbi wrote:
>> Hi,
>>
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>> @@ -3863,7 +3868,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>>>>  	 * sure we're starting from a well known location.
>>>>>  	 */
>>>>>  
>>>>> -	ret = dwc3_gadget_init_endpoints(dwc, dwc->num_eps);
>>>>> +	ret = dwc3_gadget_init_endpoints(dwc);
>>>>>  	if (ret)
>>>>>  		goto err4;
>>>> heh, looking at original commit, we used to have num_in_eps and
>>>> num_out_eps. In fact, this commit will reintroduce another problem that
>>>> Bryan tried to solve. num_eps - num_in_eps is not necessarily
>>>> num_out_eps.
>>>>
>>> From my understanding, that's not what Bryan's saying. Here's the
>>> snippet of the commit message:
>>>
>>> "
>>>     It's possible to configure RTL such that DWC_USB3_NUM_EPS is equal to
>>>     DWC_USB3_NUM_IN_EPS.
>>>
>>>     dwc3-core calculates the number of OUT endpoints as DWC_USB3_NUM minus
>>>     DWC_USB3_NUM_IN_EPS. If RTL has been configured with DWC_USB3_NUM_IN_EPS
>>>     equal to DWC_USB3_NUM then dwc3-core will calculate the number of OUT
>>>     endpoints as zero.
>>>
>>>     For example a from dwc3_core_num_eps() shows:
>>>     [    1.565000]  /usb0@f01d0000: found 8 IN and 0 OUT endpoints
>>> "
>>>
>>> He just stated that you can configure to have num_eps equals to
>>> num_in_eps. Basically it means there's no OUT physical endpoint. Not
>> no, that's not what it means. I don't have access to DWC3 documentation
>> anymore, but from what I remember every physical endpoint _can_ be
>> configured as bidirectional. In other words, DWC3_USB3_NUM_EPS ==
>> DWC3_USB3_NUM_IN_EPS could mean that every endpoint in the system is
>> bidirectional.
>>
>>> sure why you would ever want to do that because that will prevent any
>>> device from working. The reason we have DWC_USB3x_NUM_IN_EPS and
>>> DWC_USB3x_NUM_EPS exposed in the global HW param so that the driver know
>>> how many endpoints are available for each direction. If for some reason
>>> this mechanism fails, there's something fundamentally wrong in the HW
>>> configuration. We have not seen this problem, and I don't think Bryan
>>> did from his commit statement either.
>> Please confirm this internally. That was my original assumption too,
>> until Bryan pointed me to a particular section of the
>> specification. Unfortunately it's far too long ago and I can't even
>> verify documentation :-)
>>
>>>> How have you verified this patch? Did you read Bryan's commit log? This
>>>> is likely to reintroduce the problem raised by Bryan.
>>>>
>>> We verified with our FPGA HAPS with various number of endpoints. No
>>> issue is seen.
>> That's cool. Could you please make sure our understanding of this is
>> sound and won't interfere with any designs? If we modify this part of
>> the code again, I'd like to see a clear reference to a specific section
>> of the databook detailing the expected behavior :-)
>>
>> cheers
>>
> Hm... I didn't consider bidirection endpoint other than control endpoint.
>
> DWC3_USB3x_NUM_EPS specifies the number of device mode for single
> directional endpoints. A bidirectional endpoint needs 2 single
> directional endpoints, 1 IN and 1 OUT. So, if your setup uses 3
> bidirection endpoints and only those, DWC3_USB3x_NUM_EPS should be 6.
> DWC3_USB3x_NUM_IN_EPS specifies the maximum number of IN endpoint active
> at any time.
>
> However, I will have to double check and confirm internally regarding
> how to determine many endpoint would be available if bidirection
> endpoints come into play.
>
> Thanks for pointing this out. Will get back on this.
>
> Thinh
>

Ok. Just had some discussion internally. So, like you said, any endpoint
can be configured in either direction. However, we are limited to
configuring up to DWC_USB3x_NUM_IN_EPS because each IN endpoint has its
own TxFIFO while for OUT, they share the same RxFIFO. So we could have
up to DWC_USB3x_NUM_EPS number of OUT endpoints. So, the issue Bryan
attempted to address is still there.

However, the current code still has some assumption on the number of IN
and OUT endpoints, I need to think of a better solution.

Thanks,
Thinh

Re: [PATCH] usb: dwc3: gadget: Init only available HW eps

[Felipe Balbi]

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>> How have you verified this patch? Did you read Bryan's commit log? This
>>>>> is likely to reintroduce the problem raised by Bryan.
>>>>>
>>>> We verified with our FPGA HAPS with various number of endpoints. No
>>>> issue is seen.
>>> That's cool. Could you please make sure our understanding of this is
>>> sound and won't interfere with any designs? If we modify this part of
>>> the code again, I'd like to see a clear reference to a specific section
>>> of the databook detailing the expected behavior :-)
>>>
>>> cheers
>>>
>> Hm... I didn't consider bidirection endpoint other than control endpoint.
>>
>> DWC3_USB3x_NUM_EPS specifies the number of device mode for single
>> directional endpoints. A bidirectional endpoint needs 2 single
>> directional endpoints, 1 IN and 1 OUT. So, if your setup uses 3
>> bidirection endpoints and only those, DWC3_USB3x_NUM_EPS should be 6.
>> DWC3_USB3x_NUM_IN_EPS specifies the maximum number of IN endpoint active
>> at any time.
>>
>> However, I will have to double check and confirm internally regarding
>> how to determine many endpoint would be available if bidirection
>> endpoints come into play.
>>
>> Thanks for pointing this out. Will get back on this.
>>
>> Thinh
>>
>
> Ok. Just had some discussion internally. So, like you said, any endpoint
> can be configured in either direction. However, we are limited to
> configuring up to DWC_USB3x_NUM_IN_EPS because each IN endpoint has its
> own TxFIFO while for OUT, they share the same RxFIFO. So we could have
> up to DWC_USB3x_NUM_EPS number of OUT endpoints. So, the issue Bryan
> attempted to address is still there.
>
> However, the current code still has some assumption on the number of IN
> and OUT endpoints, I need to think of a better solution.

Yes, the assumption still exists because at the time there was no better
solution :-)

-- 
balbi

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

Re: [PATCH] usb: dwc3: gadget: Init only available HW eps

[Bryan O'Donoghue]

On 08/01/2021 12:23, Felipe Balbi wrote:
> 
> Hi,
> 
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>>> How have you verified this patch? Did you read Bryan's commit log? This
>>>>>> is likely to reintroduce the problem raised by Bryan.
>>>>>>
>>>>> We verified with our FPGA HAPS with various number of endpoints. No
>>>>> issue is seen.
>>>> That's cool. Could you please make sure our understanding of this is
>>>> sound and won't interfere with any designs? If we modify this part of
>>>> the code again, I'd like to see a clear reference to a specific section
>>>> of the databook detailing the expected behavior :-)
>>>>
>>>> cheers
>>>>
>>> Hm... I didn't consider bidirection endpoint other than control endpoint.
>>>
>>> DWC3_USB3x_NUM_EPS specifies the number of device mode for single
>>> directional endpoints. A bidirectional endpoint needs 2 single
>>> directional endpoints, 1 IN and 1 OUT. So, if your setup uses 3
>>> bidirection endpoints and only those, DWC3_USB3x_NUM_EPS should be 6.
>>> DWC3_USB3x_NUM_IN_EPS specifies the maximum number of IN endpoint active
>>> at any time.
>>>
>>> However, I will have to double check and confirm internally regarding
>>> how to determine many endpoint would be available if bidirection
>>> endpoints come into play.
>>>
>>> Thanks for pointing this out. Will get back on this.
>>>
>>> Thinh
>>>
>>
>> Ok. Just had some discussion internally. So, like you said, any endpoint
>> can be configured in either direction. However, we are limited to
>> configuring up to DWC_USB3x_NUM_IN_EPS because each IN endpoint has its
>> own TxFIFO while for OUT, they share the same RxFIFO. So we could have
>> up to DWC_USB3x_NUM_EPS number of OUT endpoints. So, the issue Bryan
>> attempted to address is still there.
>>
>> However, the current code still has some assumption on the number of IN
>> and OUT endpoints, I need to think of a better solution.
> 
> Yes, the assumption still exists because at the time there was no better
> solution :-)
> 

Just found this now.

The commit log is too wordy. You can configure the RTL so that every 
endpoint can be either direction.

So DWC_USB3_NUM == DWC_USB3_NUM_IN_EPS

The old code was predicated on the notion the RTL was configured with 
EP0 IN/OUT basically fixed.

In practice this _should_ be the case but the RTL does not enforce it 
and yes this appears in a real SoC from Imagination.

---
bod