| Message ID | 730f249434fe20f8c662943c1407cdddc364d391.1541642865.git.thinhn@synopsys.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 022a0208c0ff038f8970a71cb298f85722b4a0ef |
| Headers | show |
| Series | usb: dwc3: Misc USB2 LPM settings | expand |
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
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.
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.
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
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.
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
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
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
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 -
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
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.
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(-)