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

[Thinh Nguyen]

Hi,

Anurag Kumar Vulisha wrote:
> Hi Claus,
>
>> -----Original Message-----
>> From: Claus H. Stovgaard [mailto:cst@xxxxxxxxxxxx]
>> Sent: Tuesday, May 07, 2019 2:28 AM
>> To: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>; Anurag Kumar Vulisha
>> <anuragku@xxxxxxxxxx>; Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Rob
>> Herring <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; Felipe Balbi
>> <balbi@xxxxxxxxxx>
>> Cc: linux-usb@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx; v.anuragkumar@xxxxxxxxx
>> Subject: Re: [PATCH 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2
>> entries
>>
>> Hi Thinh and Anurag
>>
>> On man, 2019-05-06 at 19:21 +0000, Thinh Nguyen wrote:
>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
>>>> a1b126f..4f0912c 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -1285,6 +1285,10 @@ static void dwc3_get_properties(struct dwc3
>>>> *dwc)
>>>>  				"snps,dis_u2_susphy_quirk");
>>>>  	dwc->dis_enblslpm_quirk = device_property_read_bool(dev,
>>>>  				"snps,dis_enblslpm_quirk");
>>>> +	dwc->dis_u1_entry_quirk = device_property_read_bool(dev,
>>>> +				"snps,dis_u1_entry_quirk");
>>>> +	dwc->dis_u2_entry_quirk = device_property_read_bool(dev,
>>>> +				"snps,dis_u2_entry_quirk");
>>> Please use "-" rather than "_" in the property names.
>> I have thought about this feature over the weekend, and think the naming should be
>> changed to something like "snps,bos-u1-exit-lat-in-us"
>> and named the same in the code. And then be the value used by the
>> get_config_params. E.g. the device-tree is used to set the values directly used for
>> bUxdevExitLat instead of named something not related to exit latency.
>>
>> With this the name and function is a 1 to 1 match, and you can among others set it to
>> 0 for optaining what Anurag wants.
>>
> Your suggestion looks good but the problem is the U1 and U2 exit latencies are
> fixed values in dwc3 controller(can be found in HCSPARAMS3). Adding different
> exit latencies may modify the U1SEL/U2SEL values sent from the host but the real
> dwc3 controller exit latencies are not getting changed. Because of this reason I
> had opted "snps,dis_u1_entry_quirk", so that the U1/U2 exit latency values
> reported in BOS descriptor can be either be zero (when U1/U2 entries needs to be
> disabled) or non-zero value (reported in HCSPARAMS3) when U1/U2 states allowed.
> Based on this I think it is better if we can continue with "snps,dis-u1-entry-quirk"
> instead of the "snps,bos-u1-exit-lat-in-us". Please  provide your opinion on this.

>  
>> Regarding the disabling of U1 / U2. I send this to Anurag
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dusb-26m-3D155683299311954-26w-3D2&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w&m=MBQpZmX-jgrlpT68k5VR-4xv_DYb5UGUiD5objMqwpA&s=Ca-zBV5t26-ZFPbNAkD8K3F3lbc3CwUXNpAgnkVasg4&e=
>> Here i created a configfs interface with the names "lpm_U1_disable" and
>> "lpm_U2_disable" for controlling the DTCL of dwc3, and reject
>> SET_FEATURE(U1/U2)
>>
>> Will send this in seperate patch tomorrow, in the hope that Anurags feature can
>> become a way for controlling exit latency, and my patch become a way for disabling
>> U1/U2
>>
> I agree with your suggestion. When U1 and U2 entries are not allowed  it is always
> better to report zero value for U1/U2 exit latencies in BOS descriptor (no point in
> reporting non-zero exit latency values when U1/U2 states are not allowed).  Along
> with that changes for preventing the dwc3 controller from initiating or accepting
> U1/U2 requests are also required (since there are some host platforms where sending
> 0 exit latency doesn't work). Based on these observations I believe both your patch
> changes and my patch changes needs to be added.
>
> @Thinh Nguyen
> Please provide your opinion on this
>

The 0 exit latency in the BOS descriptor doesn't mean that device
doesn't support U1/U2 (however unrealistic it may seem).

The exit latency values reported in the BOS decriptor are just
recommended latency. The host controls over what they should be (host
has its own U1/U2 exit latency). I don't think we should have a device
property to set the device exit latency.

If the gadget driver needs to know what the recommended latency to set
in the BOS descriptor, we can have those values exported to some new
fields in the usb_gadget structure.

BR,
Thinh