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
