Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
On 12/11/18 13:03, Felipe Balbi wrote: > > Hi, > > Roger Quadros 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 -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
Hi, Roger Quadros 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 - -- balbi signature.asc Description: PGP signature
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
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 >>> --- >>> 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) >>> _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 -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
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 >> --- >> 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) >> _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
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
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
> ---
> 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)
> _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",
> _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 {
> unsignedsetup_packet_pending:1;
> unsignedthree_stage_setup:1;
> unsignedusb3_lpm_capable:1;
> + unsignedusb2_lpm_disable:1;
>
> unsigneddisable_scramble_quirk:1;
> unsignedu2exit_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
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
Hi,
Oliver Neukum 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 {
>> > > unsignedsetup_packet_pending:1;
>> > > unsignedthree_stage_setup:1;
>> > > unsignedusb3_lpm_capable:1;
>> > > + unsignedusb2_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.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
On Do, 2018-11-08 at 12:47 +0200, Felipe Balbi wrote:
Hi,
> Oliver Neukum 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 {
> > > unsignedsetup_packet_pending:1;
> > > unsignedthree_stage_setup:1;
> > > unsignedusb3_lpm_capable:1;
> > > + unsignedusb2_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
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
Hi,
Felipe Balbi 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 {
>>> unsignedsetup_packet_pending:1;
>>> unsignedthree_stage_setup:1;
>>> unsignedusb3_lpm_capable:1;
>>> + unsignedusb2_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.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
Hi,
Oliver Neukum 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 {
>> unsignedsetup_packet_pending:1;
>> unsignedthree_stage_setup:1;
>> unsignedusb3_lpm_capable:1;
>> + unsignedusb2_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.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
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 {
> unsignedsetup_packet_pending:1;
> unsignedthree_stage_setup:1;
> unsignedusb3_lpm_capable:1;
> + unsignedusb2_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
[PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
Support the option to disable USB2 LPM. Set xhci "usb2-lpm-disable"
property via "snps,usb2-lpm-disable" property.
Signed-off-by: Thinh Nguyen
---
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)
_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",
_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 {
unsignedsetup_packet_pending:1;
unsignedthree_stage_setup:1;
unsignedusb3_lpm_capable:1;
+ unsignedusb2_lpm_disable:1;
unsigneddisable_scramble_quirk:1;
unsignedu2exit_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.
--
2.11.0
