Hi Anurag, Anurag Kumar Vulisha wrote: > Hi Thinh, > >> -----Original Message----- >> From: Thinh Nguyen [mailto:[email protected]] >> Sent: Saturday, May 11, 2019 7:18 AM >> To: Anurag Kumar Vulisha <[email protected]>; Thinh Nguyen >> <[email protected]>; Greg Kroah-Hartman >> <[email protected]>; Rob Herring <[email protected]>; Mark Rutland >> <[email protected]>; Felipe Balbi <[email protected]>; Claus H. Stovgaard >> <[email protected]> >> Cc: [email protected]; [email protected]; linux- >> [email protected]; [email protected] >> Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 >> and U2 >> entries >> >> Hi, >> >> Anurag Kumar Vulisha wrote: >>> Hi Thinh, >>> >>>> -----Original Message----- >>>> From: Thinh Nguyen [mailto:[email protected]] >>>> Sent: Friday, May 10, 2019 5:30 AM >>>> To: Anurag Kumar Vulisha <[email protected]>; Thinh Nguyen >>>> <[email protected]>; Greg Kroah-Hartman >>>> <[email protected]>; Rob Herring <[email protected]>; Mark >>>> Rutland >>>> <[email protected]>; Felipe Balbi <[email protected]>; Claus H. Stovgaard >>>> <[email protected]> >>>> Cc: [email protected]; [email protected]; linux- >>>> [email protected]; [email protected] >>>> Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling >>>> U1 and >> U2 >>>> entries >>>> >>>> Hi Anurag, >>>> >>>> Anurag Kumar Vulisha wrote: >>>>>>> + return -EINVAL; >>>>>>> >>>>>>> reg = dwc3_readl(dwc->regs, DWC3_DCTL); >>>>>>> if (set) >>>>>>> @@ -626,7 +630,10 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, >>>> struct >>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>>>> index e293400..f2d3112 100644 >>>>>>> --- a/drivers/usb/dwc3/gadget.c >>>>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>>>> @@ -2073,6 +2073,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g) >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> +static void dwc3_gadget_config_params(struct usb_gadget *g, >>>>>>> + struct usb_dcd_config_params >>>>>>> *params) >>>>>>> +{ >>>>>>> + struct dwc3 *dwc = gadget_to_dwc(g); >>>>>>> + >>>>>>> + /* U1 Device exit Latency */ >>>>>>> + if (dwc->dis_u1_entry_quirk) >>>>>>> + params->bU1devExitLat = 0; >>>>>> It doesn't make sense to have exit latency of 0. Rejecting >>>>>> SET_FEATURE(enable U1/U2) should already let the host know that the >>>>>> device doesn't support U1/U2. >>>>>> >>>>> I am okay to remove this, but I feel that it is better to report zero >>>>> value instead >>>>> of a non-zero value in exit latency of BOS when U1 or U2 entries are not >> supported. >>>>> Advantage of reporting 0 is that some hosts doesn't even send >>>> SET_FEATURE(U1/U2) >>>>> requests on seeing zero value in BOS descriptor. Also there can be cases >>>>> where >> U1 is >>>>> disabled and U2 entry is allowed or vice versa, for these kind of cases >>>>> the driver >> can >>>>> set zero exit latency value for U1 and non-zero exit latency value for U2 >>>>> . Based >> on >>>> this >>>>> I think it would be better to report 0 when U1/U2 states are not enabled. >>>>> Please >>>> provide >>>>> your opinion on this. >>>> Hm... I assume you're testing against linux usb stack and xhci host. If >>>> that's the case, it looks like host will still request the device to >>>> enter U1/U2 despite the device rejecting SET_FEATURE(enable U1/U2). This >>>> needs to be fixed. I think what you have is fine to workaround this issue. >>> Thanks . Will send the next series with the other fixes that you have >>> suggested >>> >>> Best Regards, >>> Anurag Kumar Vulisha >>> >> I want to try something. Can you see if this helps with your performance >> test without setting the U1/U2 exit latency to 0? >> (No need to change what you have in your patch. This is just for testing). >> > With your patch , the link doesn't enter into U1/U2 and I am also getting > better performance > > Thanks, > Anurag Kumar Vulisha
Thanks for testing. BR, Thinh
