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

[Anurag Kumar Vulisha]

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://marc.info/?l=linux-usb&m=155683299311954&w=2
>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

Thanks,
Anurag Kumar Vulisha

>BR
>Claus