On 1/24/2017 11:39 AM, Felipe Balbi wrote: > Hi, > > (sorry if formatting sucks, using web interface) > > On Tue, Jan 24, 2017 at 9:21 PM John Youn <John.Youn@xxxxxxxxxxxx > <mailto:John.Youn@xxxxxxxxxxxx>> wrote: > > On 1/24/2017 12:53 AM, Felipe Balbi wrote: > > > > Hi, > > > > John Youn <John.Youn@xxxxxxxxxxxx <mailto:John.Youn@xxxxxxxxxxxx>> > writes: > >>>> Bryan O'Donoghue <pure.logic@xxxxxxxxxxxxxxxxx > <mailto:pure.logic@xxxxxxxxxxxxxxxxx>> writes: > >>>>> - DWC_USB3_NUM indicates the number of Device mode single > directional > >>>>> endpoints, including OUT and IN endpoint 0. > >>>>> > >>>>> - DWC_USB3_NUM_IN_EPS indicates the maximum number of Device > mode IN > >>>>> endpoints active at any time, including control endpoint 0. > >>>>> > >>>>> 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 > >>>>> > >>>>> This patch works around this case by detecting when > DWC_USB3_NUM_EPS is > >>>>> equal to DWC3_USB3_NUM_IN_EPS and over-rides the calculation > of the number > >>>>> of OUT and IN endpoints to make dwc->num_in_eps equal to half > >>>>> DWC_USB3_NUM_EPS. > >>>>> > >>>>> If DWC_USB3_NUM_EPS is equal to DWC3_USB3_NUM_IN_EPS and the > endpoint count > >>>>> is an odd number then dwc->num_out_eps will be assigned the > extra endpoint. > >>>> > >>>> sorry, now that I spent some more time with this. Isn't > something like > >>>> below solving all problems? > >>>> > >>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >>>> index 369bab16a824..68c9c84b7216 100644 > >>>> --- a/drivers/usb/dwc3/core.c > >>>> +++ b/drivers/usb/dwc3/core.c > >>>> @@ -397,8 +397,7 @@ static void dwc3_core_num_eps(struct dwc3 *dwc) > >>>> { > >>>> struct dwc3_hwparams *parms = &dwc->hwparams; > >>>> > >>>> - dwc->num_in_eps = DWC3_NUM_IN_EPS(parms); > >>>> - dwc->num_out_eps = DWC3_NUM_EPS(parms) - dwc->num_in_eps; > >>>> + dwc->num_eps = DWC3_NUM_EPS(parms); > >>>> } > >>>> > >>>> static void dwc3_cache_hwparams(struct dwc3 *dwc) > >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > >>>> index 0a664d8eba3f..8c187df0aa42 100644 > >>>> --- a/drivers/usb/dwc3/gadget.c > >>>> +++ b/drivers/usb/dwc3/gadget.c > >>>> @@ -2001,23 +2002,7 @@ static int > dwc3_gadget_init_hw_endpoints(struct dwc3 *dwc, > >>>> > >>>> static int dwc3_gadget_init_endpoints(struct dwc3 *dwc) > >>>> { > >>>> - int ret; > >>>> - > >>>> - INIT_LIST_HEAD(&dwc->gadget.ep_list); > >>>> - > >>>> - ret = dwc3_gadget_init_hw_endpoints(dwc, dwc->num_out_eps, 0); > >>>> - if (ret < 0) { > >>>> - dev_err(dwc->dev, "failed to initialize OUT > endpoints\n"); > >>>> - return ret; > >>>> - } > >>>> - > >>>> - ret = dwc3_gadget_init_hw_endpoints(dwc, dwc->num_in_eps, 1); > >>>> - if (ret < 0) { > >>>> - dev_err(dwc->dev, "failed to initialize IN > endpoints\n"); > >>>> - return ret; > >>>> - } > >>>> - > >>>> - return 0; > >>>> + return dwc3_gadget_init_hw_endpoints(dwc, dwc->num_eps); > >>> > >>> Well I hadn't considered that level of change myself but, it > should work. > >>> > >> > >> It should work for ASICS. > >> > >> But it won't work for our FPGA platform. It needs to work the > >> existing, more limited, way. You can check this with the > >> GHWPARAMS6.EN_FPGA. > > > > Can you clarify a little more? Why wouldn't above work for FPGA? > > > > Actually my mistake, this should still work fine as long as you are > predefining the number and direction the same as before. > > See DWC_USB3_EN_LOG_PHYS_EP_SUPT for the FPGA configuration. > > This configuration should be enabled (the default), but can be > disabled for FPGA configs to meet timing and save space. Looks like > DWC3 driver is currently programming as if this is always disabled, > which should be fine, it will just be more limiting than necessary. > > > We always map endpoints as if that option was disabled. We gain nothing > at all by mapping physical ep 30 to USB ep 1 OUT. I'd say this is rather > pointless :-) We might need some more flexible endpoint mapping, but > should we really care about that now? We haven't thus far and nobody > complained. Perhaps we should add a note to dwc3_gadget_init_hw_endpoints() > mentioning that we're mapping endpoints linearly? > The problem comes if we have a limited number of HW endpoints and we want to use more than half of them for either IN or OUT. But yeah, no one has complained yet. John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
