Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

Hi,

Anurag Kumar Vulisha wrote:
> Hi Thinh,
>
>> -----Original Message-----
>> From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com]
>> Sent: Friday, May 10, 2019 5:30 AM
>> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Thinh Nguyen
>> <Thinh.Nguyen@synopsys.com>; Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
>> <mark.rutland@arm.com>; Felipe Balbi <balbi@kernel.org>; Claus H. Stovgaard
>> <cst@phaseone.com>
>> Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org; v.anuragkumar@gmail.com
>> 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