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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
>> Fixes: 47d3946ea220 ("usb: dwc3: refactor gadget endpoint count calculation")
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
>> ---
>>  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






[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux