Hi Thinh, >-----Original Message----- >From: Thinh Nguyen [mailto:Thinh.Nguyen@xxxxxxxxxxxx] >Sent: Saturday, May 11, 2019 7:18 AM >To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; Thinh Nguyen ><Thinh.Nguyen@xxxxxxxxxxxx>; Greg Kroah-Hartman ><gregkh@xxxxxxxxxxxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Mark Rutland ><mark.rutland@xxxxxxx>; Felipe Balbi <balbi@xxxxxxxxxx>; Claus H. Stovgaard ><cst@xxxxxxxxxxxx> >Cc: linux-usb@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- >kernel@xxxxxxxxxxxxxxx; v.anuragkumar@xxxxxxxxx >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:Thinh.Nguyen@xxxxxxxxxxxx] >>> Sent: Friday, May 10, 2019 5:30 AM >>> To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; Thinh Nguyen >>> <Thinh.Nguyen@xxxxxxxxxxxx>; Greg Kroah-Hartman >>> <gregkh@xxxxxxxxxxxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Mark Rutland >>> <mark.rutland@xxxxxxx>; Felipe Balbi <balbi@xxxxxxxxxx>; Claus H. Stovgaard >>> <cst@xxxxxxxxxxxx> >>> Cc: linux-usb@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- >>> kernel@xxxxxxxxxxxxxxx; v.anuragkumar@xxxxxxxxx >>> 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 >diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c >index 2f94568ba385..619351c581cf 100644 >--- a/drivers/usb/core/hub.c >+++ b/drivers/usb/core/hub.c >@@ -4057,8 +4057,18 @@ static void usb_enable_link_state(struct usb_hcd >*hcd, struct usb_device *udev, > /* Only a configured device will accept the Set Feature > * U1/U2_ENABLE > */ >- if (udev->actconfig) >- usb_set_device_initiated_lpm(udev, state, true); >+ if (udev->actconfig) { >+ if (usb_set_device_initiated_lpm(udev, state, >true)) { >+ /* >+ * Don't request U1/U2 entry if the device >+ * cannot enable U1/U2. >+ */ >+ usb_set_lpm_timeout(udev, state, 0); >+ >hcd->driver->disable_usb3_lpm_timeout(hcd, udev, >+ >state); >+ return; >+ } >+ } > > /* As soon as usb_set_lpm_timeout(timeout) returns 0, the > * hub-initiated LPM is enabled. Thus, LPM is enabled no > >
