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). 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 Thanks, Thinh
