[3/3] usb: dwc3: Support option to disable USB2 LPM

[3/3] usb: dwc3: Support option to disable USB2 LPM

[Felipe Balbi]

Hi,

Oliver Neukum <oneukum@suse.com> writes:
> On Mi, 2018-11-07 at 18:10 -0800, Thinh Nguyen wrote:
>> 
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
>>   * @setup_packet_pending: true when there's a Setup Packet in FIFO. Workaround
>>   * @three_stage_setup: set if we perform a three phase setup
>>   * @usb3_lpm_capable: set if hadrware supports Link Power Management
>> + * @usb2_lpm_disable: set to disable usb2 lpm
>>   * @disable_scramble_quirk: set if we enable the disable scramble quirk
>>   * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
>>   * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
>> @@ -1146,6 +1147,7 @@ struct dwc3 {
>>         unsigned                setup_packet_pending:1;
>>         unsigned                three_stage_setup:1;
>>         unsigned                usb3_lpm_capable:1;
>> +       unsigned                usb2_lpm_disable:1;
>
> Hi,
>
> that may be a bit late, but why would this be a property of dwc3?
> Now, you may want to do this for a specific controller,
> but there is no reason to limit the flag to dwc3. We want this
> flag in the generic HCD attributes, so that other HCDs can share
> it. Maybe even expose it to sysfs.

this is used for the peripheral side of dwc3 too.

[3/3] usb: dwc3: Support option to disable USB2 LPM

[Roger Quadros]

On 12/11/18 13:03, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>>> Also can we have some consistency in usage of '-' vs '_'?
>>> Right.. I agree. I've been using '-' as it is the preferred syntax as
>>> most of the properties, but some old properties use '_'. Do you have any
>>> suggestion?
>>
>> I'd keep it consistent to "snps,usb3_lpm_capable" so we avoid mistakes
>> when writing the DT.
>> Felipe?
> 
> _ are not really accepted in DT, except for legacy properties. What we
> can do is rename usb3_lpm_capable to usb3-lpm-capable and change code to
> try with _ if - fails. Then we can introduce the new property using -
> 

I agree. This is a better approach.

cheers,
-roger

[3/3] usb: dwc3: Support option to disable USB2 LPM

[Felipe Balbi]

Hi,

Roger Quadros <rogerq@ti.com> writes:
>>> Also can we have some consistency in usage of '-' vs '_'?
>> Right.. I agree. I've been using '-' as it is the preferred syntax as
>> most of the properties, but some old properties use '_'. Do you have any
>> suggestion?
>
> I'd keep it consistent to "snps,usb3_lpm_capable" so we avoid mistakes
> when writing the DT.
> Felipe?

_ are not really accepted in DT, except for legacy properties. What we
can do is rename usb3_lpm_capable to usb3-lpm-capable and change code to
try with _ if - fails. Then we can introduce the new property using -

[3/3] usb: dwc3: Support option to disable USB2 LPM

[Roger Quadros]

Thinh,

On 12/11/18 07:29, Thinh Nguyen wrote:
> Hi Roger,
> 
> On 11/9/2018 3:58 AM, Roger Quadros wrote:
>> Hi,
>>
>> On 08/11/18 04:10, Thinh Nguyen wrote:
>>> Support the option to disable USB2 LPM. Set xhci "usb2-lpm-disable"
>>> property via "snps,usb2-lpm-disable" property.
>>>
>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>> ---
>>>  drivers/usb/dwc3/core.c | 2 ++
>>>  drivers/usb/dwc3/core.h | 2 ++
>>>  drivers/usb/dwc3/host.c | 5 ++++-
>>>  3 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index a4068a7b95dd..f6b80a545a78 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1248,6 +1248,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>  				&hird_threshold);
>>>  	dwc->usb3_lpm_capable = device_property_read_bool(dev,
>>>  				"snps,usb3_lpm_capable");
>>> +	dwc->usb2_lpm_disable = device_property_read_bool(dev,
>>> +				"snps,usb2-lpm-disable");
>> Can we use the same logic as usb3_lpm instead?
>> i.e. enable USB2 LPM only if "snps,usb2_lpm_capable" is present in DT.
>> This is because older platforms that are not tested for usb2 lpm
>> might break if you enable it by default.
> 
> I follow the same logic as usb-xhci property. The usb2-lpm-disable
> property from xHCI has been around for awhile. Do you suggest to change
> the property for xHCI then?

No.
I see it now that you are just setting the XHCI property and not
doing any change in the dwc3 functionality itself.
I think your patch is correct.

>> Also can we have some consistency in usage of '-' vs '_'?
> Right.. I agree. I've been using '-' as it is the preferred syntax as
> most of the properties, but some old properties use '_'. Do you have any
> suggestion?

I'd keep it consistent to "snps,usb3_lpm_capable" so we avoid mistakes
when writing the DT.
Felipe?

cheers,
-roger

[3/3] usb: dwc3: Support option to disable USB2 LPM

[Thinh Nguyen]

Hi Roger,

On 11/9/2018 3:58 AM, Roger Quadros wrote:
> Hi,
>
> On 08/11/18 04:10, Thinh Nguyen wrote:
>> Support the option to disable USB2 LPM. Set xhci "usb2-lpm-disable"
>> property via "snps,usb2-lpm-disable" property.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>>  drivers/usb/dwc3/core.c | 2 ++
>>  drivers/usb/dwc3/core.h | 2 ++
>>  drivers/usb/dwc3/host.c | 5 ++++-
>>  3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index a4068a7b95dd..f6b80a545a78 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1248,6 +1248,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>  				&hird_threshold);
>>  	dwc->usb3_lpm_capable = device_property_read_bool(dev,
>>  				"snps,usb3_lpm_capable");
>> +	dwc->usb2_lpm_disable = device_property_read_bool(dev,
>> +				"snps,usb2-lpm-disable");
> Can we use the same logic as usb3_lpm instead?
> i.e. enable USB2 LPM only if "snps,usb2_lpm_capable" is present in DT.
> This is because older platforms that are not tested for usb2 lpm
> might break if you enable it by default.

I follow the same logic as usb-xhci property. The usb2-lpm-disable
property from xHCI has been around for awhile. Do you suggest to change
the property for xHCI then?
> Also can we have some consistency in usage of '-' vs '_'?
Right.. I agree. I've been using '-' as it is the preferred syntax as
most of the properties, but some old properties use '_'. Do you have any
suggestion?

Thanks,
Thinh

[3/3] usb: dwc3: Support option to disable USB2 LPM

[Roger Quadros]

Hi,

On 08/11/18 04:10, Thinh Nguyen wrote:
> Support the option to disable USB2 LPM. Set xhci "usb2-lpm-disable"
> property via "snps,usb2-lpm-disable" property.
> 
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
>  drivers/usb/dwc3/core.c | 2 ++
>  drivers/usb/dwc3/core.h | 2 ++
>  drivers/usb/dwc3/host.c | 5 ++++-
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index a4068a7b95dd..f6b80a545a78 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1248,6 +1248,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  				&hird_threshold);
>  	dwc->usb3_lpm_capable = device_property_read_bool(dev,
>  				"snps,usb3_lpm_capable");
> +	dwc->usb2_lpm_disable = device_property_read_bool(dev,
> +				"snps,usb2-lpm-disable");

Can we use the same logic as usb3_lpm instead?
i.e. enable USB2 LPM only if "snps,usb2_lpm_capable" is present in DT.
This is because older platforms that are not tested for usb2 lpm
might break if you enable it by default.

Also can we have some consistency in usage of '-' vs '_'?

>  	device_property_read_u8(dev, "snps,rx-thr-num-pkt-prd",
>  				&rx_thr_num_pkt_prd);
>  	device_property_read_u8(dev, "snps,rx-max-burst-prd",
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5bfb62533e0f..ca7b5f46e1f0 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
>   * @setup_packet_pending: true when there's a Setup Packet in FIFO. Workaround
>   * @three_stage_setup: set if we perform a three phase setup
>   * @usb3_lpm_capable: set if hadrware supports Link Power Management
> + * @usb2_lpm_disable: set to disable usb2 lpm
>   * @disable_scramble_quirk: set if we enable the disable scramble quirk
>   * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
>   * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
> @@ -1146,6 +1147,7 @@ struct dwc3 {
>  	unsigned		setup_packet_pending:1;
>  	unsigned		three_stage_setup:1;
>  	unsigned		usb3_lpm_capable:1;
> +	unsigned		usb2_lpm_disable:1;
>  
>  	unsigned		disable_scramble_quirk:1;
>  	unsigned		u2exit_lfps_quirk:1;
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index 1a3878a3be78..f55947294f7c 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -46,7 +46,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
>  
>  int dwc3_host_init(struct dwc3 *dwc)
>  {
> -	struct property_entry	props[3];
> +	struct property_entry	props[4];
>  	struct platform_device	*xhci;
>  	int			ret, irq;
>  	struct resource		*res;
> @@ -93,6 +93,9 @@ int dwc3_host_init(struct dwc3 *dwc)
>  	if (dwc->usb3_lpm_capable)
>  		props[prop_idx++].name = "usb3-lpm-capable";
>  
> +	if (dwc->usb2_lpm_disable)
> +		props[prop_idx++].name = "usb2-lpm-disable";
> +
>  	/**
>  	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
>  	 * where Port Disable command doesn't work.
> 

cheers,
-roger

[3/3] usb: dwc3: Support option to disable USB2 LPM

[Felipe Balbi]

Hi,

Oliver Neukum <oneukum@suse.com> writes:
>> > > --- a/drivers/usb/dwc3/core.h
>> > > +++ b/drivers/usb/dwc3/core.h
>> > > @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
>> > >   * @setup_packet_pending: true when there's a Setup Packet in FIFO. Workaround
>> > >   * @three_stage_setup: set if we perform a three phase setup
>> > >   * @usb3_lpm_capable: set if hadrware supports Link Power Management
>> > > + * @usb2_lpm_disable: set to disable usb2 lpm
>> > >   * @disable_scramble_quirk: set if we enable the disable scramble quirk
>> > >   * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
>> > >   * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
>> > > @@ -1146,6 +1147,7 @@ struct dwc3 {
>> > >         unsigned                setup_packet_pending:1;
>> > >         unsigned                three_stage_setup:1;
>> > >         unsigned                usb3_lpm_capable:1;
>> > > +       unsigned                usb2_lpm_disable:1;
>> > 
>> > Hi,
>> > 
>> > that may be a bit late, but why would this be a property of dwc3?
>> > Now, you may want to do this for a specific controller,
>> > but there is no reason to limit the flag to dwc3. We want this
>> > flag in the generic HCD attributes, so that other HCDs can share
>> > it. Maybe even expose it to sysfs.
>> 
>> this is used for the peripheral side of dwc3 too.
>
> same argument. Whether a gadget supports LPM is a question
> in no way specific to dwc3. And whether this exposes internal
> registers does not really matter. It is a capability of the HC
> for a generic issue.

for the gadget api we don't have enough UDCs supporting LPM to design a
proper generic api. Until then, it's best to keep this private to the
driver otherwise we may make the wrong decisions in the generic layer.

[3/3] usb: dwc3: Support option to disable USB2 LPM

[Oliver Neukum]

On Do, 2018-11-08 at 12:47 +0200, Felipe Balbi wrote:

Hi,

> Oliver Neukum <oneukum@suse.com> writes:
> > On Mi, 2018-11-07 at 18:10 -0800, Thinh Nguyen wrote:
> > > 
> > > --- a/drivers/usb/dwc3/core.h
> > > +++ b/drivers/usb/dwc3/core.h
> > > @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
> > >   * @setup_packet_pending: true when there's a Setup Packet in FIFO. Workaround
> > >   * @three_stage_setup: set if we perform a three phase setup
> > >   * @usb3_lpm_capable: set if hadrware supports Link Power Management
> > > + * @usb2_lpm_disable: set to disable usb2 lpm
> > >   * @disable_scramble_quirk: set if we enable the disable scramble quirk
> > >   * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
> > >   * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
> > > @@ -1146,6 +1147,7 @@ struct dwc3 {
> > >         unsigned                setup_packet_pending:1;
> > >         unsigned                three_stage_setup:1;
> > >         unsigned                usb3_lpm_capable:1;
> > > +       unsigned                usb2_lpm_disable:1;
> > 
> > Hi,
> > 
> > that may be a bit late, but why would this be a property of dwc3?
> > Now, you may want to do this for a specific controller,
> > but there is no reason to limit the flag to dwc3. We want this
> > flag in the generic HCD attributes, so that other HCDs can share
> > it. Maybe even expose it to sysfs.
> 
> this is used for the peripheral side of dwc3 too.

same argument. Whether a gadget supports LPM is a question
in no way specific to dwc3. And whether this exposes internal
registers does not really matter. It is a capability of the HC
for a generic issue.

	Regards
		Oliver

[3/3] usb: dwc3: Support option to disable USB2 LPM

[Felipe Balbi]

Hi,

Felipe Balbi <balbi@kernel.org> writes:
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
>>>   * @setup_packet_pending: true when there's a Setup Packet in FIFO. Workaround
>>>   * @three_stage_setup: set if we perform a three phase setup
>>>   * @usb3_lpm_capable: set if hadrware supports Link Power Management
>>> + * @usb2_lpm_disable: set to disable usb2 lpm
>>>   * @disable_scramble_quirk: set if we enable the disable scramble quirk
>>>   * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
>>>   * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
>>> @@ -1146,6 +1147,7 @@ struct dwc3 {
>>>         unsigned                setup_packet_pending:1;
>>>         unsigned                three_stage_setup:1;
>>>         unsigned                usb3_lpm_capable:1;
>>> +       unsigned                usb2_lpm_disable:1;
>>
>> Hi,
>>
>> that may be a bit late, but why would this be a property of dwc3?
>> Now, you may want to do this for a specific controller,
>> but there is no reason to limit the flag to dwc3. We want this
>> flag in the generic HCD attributes, so that other HCDs can share
>> it. Maybe even expose it to sysfs.
>
> this is used for the peripheral side of dwc3 too.

oh, and this is exposed through dwc3's private registers, not the
generic XHCI registers.

[3/3] usb: dwc3: Support option to disable USB2 LPM

[Oliver Neukum]

On Mi, 2018-11-07 at 18:10 -0800, Thinh Nguyen wrote:
> 
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
>   * @setup_packet_pending: true when there's a Setup Packet in FIFO. Workaround
>   * @three_stage_setup: set if we perform a three phase setup
>   * @usb3_lpm_capable: set if hadrware supports Link Power Management
> + * @usb2_lpm_disable: set to disable usb2 lpm
>   * @disable_scramble_quirk: set if we enable the disable scramble quirk
>   * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
>   * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
> @@ -1146,6 +1147,7 @@ struct dwc3 {
>         unsigned                setup_packet_pending:1;
>         unsigned                three_stage_setup:1;
>         unsigned                usb3_lpm_capable:1;
> +       unsigned                usb2_lpm_disable:1;

Hi,

that may be a bit late, but why would this be a property of dwc3?
Now, you may want to do this for a specific controller,
but there is no reason to limit the flag to dwc3. We want this
flag in the generic HCD attributes, so that other HCDs can share
it. Maybe even expose it to sysfs.

	Regards
		Oliver

[3/3] usb: dwc3: Support option to disable USB2 LPM

[Thinh Nguyen]

Support the option to disable USB2 LPM. Set xhci "usb2-lpm-disable"
property via "snps,usb2-lpm-disable" property.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/core.c | 2 ++
 drivers/usb/dwc3/core.h | 2 ++
 drivers/usb/dwc3/host.c | 5 ++++-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a4068a7b95dd..f6b80a545a78 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1248,6 +1248,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 				&hird_threshold);
 	dwc->usb3_lpm_capable = device_property_read_bool(dev,
 				"snps,usb3_lpm_capable");
+	dwc->usb2_lpm_disable = device_property_read_bool(dev,
+				"snps,usb2-lpm-disable");
 	device_property_read_u8(dev, "snps,rx-thr-num-pkt-prd",
 				&rx_thr_num_pkt_prd);
 	device_property_read_u8(dev, "snps,rx-max-burst-prd",
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5bfb62533e0f..ca7b5f46e1f0 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
  * @setup_packet_pending: true when there's a Setup Packet in FIFO. Workaround
  * @three_stage_setup: set if we perform a three phase setup
  * @usb3_lpm_capable: set if hadrware supports Link Power Management
+ * @usb2_lpm_disable: set to disable usb2 lpm
  * @disable_scramble_quirk: set if we enable the disable scramble quirk
  * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
  * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
@@ -1146,6 +1147,7 @@ struct dwc3 {
 	unsigned		setup_packet_pending:1;
 	unsigned		three_stage_setup:1;
 	unsigned		usb3_lpm_capable:1;
+	unsigned		usb2_lpm_disable:1;
 
 	unsigned		disable_scramble_quirk:1;
 	unsigned		u2exit_lfps_quirk:1;
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 1a3878a3be78..f55947294f7c 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -46,7 +46,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)
 
 int dwc3_host_init(struct dwc3 *dwc)
 {
-	struct property_entry	props[3];
+	struct property_entry	props[4];
 	struct platform_device	*xhci;
 	int			ret, irq;
 	struct resource		*res;
@@ -93,6 +93,9 @@ int dwc3_host_init(struct dwc3 *dwc)
 	if (dwc->usb3_lpm_capable)
 		props[prop_idx++].name = "usb3-lpm-capable";
 
+	if (dwc->usb2_lpm_disable)
+		props[prop_idx++].name = "usb2-lpm-disable";
+
 	/**
 	 * WORKAROUND: dwc3 revisions <=3.00a have a limitation
 	 * where Port Disable command doesn't work.