| Message ID | 20190418001356.124334-4-dianders@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron | expand |
Douglas Anderson <dianders@chromium.org> writes: > Some SoCs with a dwc2 USB controller may need to keep the PHY on to > support remote wakeup. Allow specifying this as a device tree > property. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > For relevant prior discussion on this patch, see: > > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org > > I didn't make any changes from the prior version since I never found > out what Rob thought of my previous arguments. If folks want a > change, perhaps they could choose from these options: > > 1. Assume that all dwc2 hosts would like to keep their PHY on for > suspend if there's a USB wakeup enabled, thus we totally drop this > binding. This doesn't seem super great to me since I'd bet that > many devices that use dwc2 weren't designed for USB wakeup (they > may not keep enough clocks or rails on) so we might be wasting > power for nothing. > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make > it more obvious that this property is intended both to document > that wakeup from suspend is possible and that we need the PHY for > said wakeup. > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume > it's implicit that if we can wakeup from suspend that we need to > keep the PHY on. If/when someone shows that a device exists using > dwc2 where we can wakeup from suspend without the PHY they can add > a new property. > > Changes in v2: None > > Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++ > 1 file changed, 3 insertions(+) checking file Documentation/devicetree/bindings/usb/dwc2.txt Hunk #1 FAILED at 37. Hunk #2 succeeded at 52 (offset -1 lines). 1 out of 2 hunks FAILED
Hi, On Thu, Apr 25, 2019 at 5:40 AM Felipe Balbi <felipe.balbi@linux.intel.com> wrote: > > Douglas Anderson <dianders@chromium.org> writes: > > > Some SoCs with a dwc2 USB controller may need to keep the PHY on to > > support remote wakeup. Allow specifying this as a device tree > > property. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > For relevant prior discussion on this patch, see: > > > > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org > > > > I didn't make any changes from the prior version since I never found > > out what Rob thought of my previous arguments. If folks want a > > change, perhaps they could choose from these options: > > > > 1. Assume that all dwc2 hosts would like to keep their PHY on for > > suspend if there's a USB wakeup enabled, thus we totally drop this > > binding. This doesn't seem super great to me since I'd bet that > > many devices that use dwc2 weren't designed for USB wakeup (they > > may not keep enough clocks or rails on) so we might be wasting > > power for nothing. > > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make > > it more obvious that this property is intended both to document > > that wakeup from suspend is possible and that we need the PHY for > > said wakeup. > > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume > > it's implicit that if we can wakeup from suspend that we need to > > keep the PHY on. If/when someone shows that a device exists using > > dwc2 where we can wakeup from suspend without the PHY they can add > > a new property. > > > > Changes in v2: None > > > > Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++ > > 1 file changed, 3 insertions(+) > > checking file Documentation/devicetree/bindings/usb/dwc2.txt > Hunk #1 FAILED at 37. > Hunk #2 succeeded at 52 (offset -1 lines). > 1 out of 2 hunks FAILED Yeah, as Minas pointed out in the cover letter [1] my series conflicts with Artur's. I have it on my list to try out his series and see if, perhaps, it allows me to enable the partial power down and also just generally rebase. It's fairly high on my list to do that--hopefully in the next week. [1] https://lkml.kernel.org/r/e4b3cd69-1c91-dfbe-bea7-bbca89ca1348@synopsys.com -Doug
Hi, On Thu, Apr 25, 2019 at 11:09 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Apr 25, 2019 at 5:40 AM Felipe Balbi > <felipe.balbi@linux.intel.com> wrote: > > > > Douglas Anderson <dianders@chromium.org> writes: > > > > > Some SoCs with a dwc2 USB controller may need to keep the PHY on to > > > support remote wakeup. Allow specifying this as a device tree > > > property. > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > --- > > > For relevant prior discussion on this patch, see: > > > > > > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org > > > > > > I didn't make any changes from the prior version since I never found > > > out what Rob thought of my previous arguments. If folks want a > > > change, perhaps they could choose from these options: > > > > > > 1. Assume that all dwc2 hosts would like to keep their PHY on for > > > suspend if there's a USB wakeup enabled, thus we totally drop this > > > binding. This doesn't seem super great to me since I'd bet that > > > many devices that use dwc2 weren't designed for USB wakeup (they > > > may not keep enough clocks or rails on) so we might be wasting > > > power for nothing. > > > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make > > > it more obvious that this property is intended both to document > > > that wakeup from suspend is possible and that we need the PHY for > > > said wakeup. > > > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume > > > it's implicit that if we can wakeup from suspend that we need to > > > keep the PHY on. If/when someone shows that a device exists using > > > dwc2 where we can wakeup from suspend without the PHY they can add > > > a new property. > > > > > > Changes in v2: None > > > > > > Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > checking file Documentation/devicetree/bindings/usb/dwc2.txt > > Hunk #1 FAILED at 37. > > Hunk #2 succeeded at 52 (offset -1 lines). > > 1 out of 2 hunks FAILED > > Yeah, as Minas pointed out in the cover letter [1] my series conflicts > with Artur's. I have it on my list to try out his series and see if, > perhaps, it allows me to enable the partial power down and also just > generally rebase. It's fairly high on my list to do that--hopefully > in the next week. > > [1] https://lkml.kernel.org/r/e4b3cd69-1c91-dfbe-bea7-bbca89ca1348@synopsys.com Oh, it looks like you didn't apply Artur's patches, though. Presumably you applied mine first and they won the race and thus I guess it's up to Artur to rebase his patches atop mine. This probably explains why you told him the patches didn't apply. I'll reply to that thread. ...so it turns out that when I try now my patches apply fine [2]. I'm guessing that you perhaps tried to apply these patches before my "rk3288's remote wake quirk" which would indeed cause conflicts. I mentioned the dependency in my cover letter [1] but totally understand that it's easy to miss stuff like that. :-) I'm going to assume you can just go-ahead and try applying patches 3, 4, and 5 in this series again. If you want me to repost them then please let me know. Thanks, and sorry for the hassle. [1] https://lkml.kernel.org/r/20190418001356.124334-1-dianders@chromium.org [2] Showing that patches currently apply: dianders@tictac2:v4.19 ((2e3cfcbbb140...))$ git checkout linux_usb_balbi/testing/next HEAD is now at 2e3cfcbbb140 dwc2: gadget: Fix completed transfer size calculation in DDMA dianders@tictac2:v4.19 ((2e3cfcbbb140...))$ curl -L https://lore.kernel.org/patchwork/patch/1063477/mbox | git am % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 100 7273 100 7273 0 0 35827 0 --:--:-- --:--:-- --:--:-- 35827 Applying: Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB dianders@tictac2:v4.19 ((84c34d7b9647...))$ curl -L https://lore.kernel.org/patchwork/patch/1063478/mbox | git am % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 100 9587 100 9587 0 0 46538 0 --:--:-- --:--:-- --:--:-- 46538 Applying: USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled dianders@tictac2:v4.19 ((ff5ab1cb16ab...))$ curl -L https://lore.kernel.org/patchwork/patch/1063479/mbox | git am % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 100 6045 100 6045 0 0 29778 0 --:--:-- --:--:-- --:--:-- 29778 Applying: ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports -Doug
On Wed, Apr 17, 2019 at 05:13:54PM -0700, Douglas Anderson wrote: > Some SoCs with a dwc2 USB controller may need to keep the PHY on to > support remote wakeup. Allow specifying this as a device tree > property. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > For relevant prior discussion on this patch, see: > > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org > > I didn't make any changes from the prior version since I never found > out what Rob thought of my previous arguments. If folks want a > change, perhaps they could choose from these options: > > 1. Assume that all dwc2 hosts would like to keep their PHY on for > suspend if there's a USB wakeup enabled, thus we totally drop this > binding. This doesn't seem super great to me since I'd bet that > many devices that use dwc2 weren't designed for USB wakeup (they > may not keep enough clocks or rails on) so we might be wasting > power for nothing. 1b. Use SoC specific compatible strings to enable/disable remote wake-up. We can debate what the default is I guess. > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make > it more obvious that this property is intended both to document > that wakeup from suspend is possible and that we need the PHY for > said wakeup. > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume > it's implicit that if we can wakeup from suspend that we need to > keep the PHY on. If/when someone shows that a device exists using > dwc2 where we can wakeup from suspend without the PHY they can add > a new property. > > Changes in v2: None > > Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++ > 1 file changed, 3 insertions(+) >
Hi, On Mon, Apr 29, 2019 at 6:23 PM Rob Herring <robh@kernel.org> wrote: > > On Wed, Apr 17, 2019 at 05:13:54PM -0700, Douglas Anderson wrote: > > Some SoCs with a dwc2 USB controller may need to keep the PHY on to > > support remote wakeup. Allow specifying this as a device tree > > property. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > For relevant prior discussion on this patch, see: > > > > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org > > > > I didn't make any changes from the prior version since I never found > > out what Rob thought of my previous arguments. If folks want a > > change, perhaps they could choose from these options: > > > > 1. Assume that all dwc2 hosts would like to keep their PHY on for > > suspend if there's a USB wakeup enabled, thus we totally drop this > > binding. This doesn't seem super great to me since I'd bet that > > many devices that use dwc2 weren't designed for USB wakeup (they > > may not keep enough clocks or rails on) so we might be wasting > > power for nothing. > > 1b. Use SoC specific compatible strings to enable/disable remote > wake-up. We can debate what the default is I guess. Unfortunately it's more than just SoC. While you need the SoC to be able to support this type of wakeup, you also need the board design, firmware design, regulator design, etc. ...so I don't think we can just use the SoC specific compatible string. In fact, while testing this I found that USB wakeup was totally broken unless I enabled "deep suspend" mode on my system. Something about the clocks / wakeup sources in the shallow suspend totally blocked it and I couldn't figure out what. ...so I believe it really needs to be something where someone has said: I tested it out on this board and everything is setup properly to support USB wakeup. > > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make > > it more obvious that this property is intended both to document > > that wakeup from suspend is possible and that we need the PHY for > > said wakeup. > > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume > > it's implicit that if we can wakeup from suspend that we need to > > keep the PHY on. If/when someone shows that a device exists using > > dwc2 where we can wakeup from suspend without the PHY they can add > > a new property. > > > > Changes in v2: None > > > > Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++ > > 1 file changed, 3 insertions(+)
Hi, On Thu, Apr 25, 2019 at 5:40 AM Felipe Balbi <felipe.balbi@linux.intel.com> wrote: > > Douglas Anderson <dianders@chromium.org> writes: > > > Some SoCs with a dwc2 USB controller may need to keep the PHY on to > > support remote wakeup. Allow specifying this as a device tree > > property. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > For relevant prior discussion on this patch, see: > > > > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org > > > > I didn't make any changes from the prior version since I never found > > out what Rob thought of my previous arguments. If folks want a > > change, perhaps they could choose from these options: > > > > 1. Assume that all dwc2 hosts would like to keep their PHY on for > > suspend if there's a USB wakeup enabled, thus we totally drop this > > binding. This doesn't seem super great to me since I'd bet that > > many devices that use dwc2 weren't designed for USB wakeup (they > > may not keep enough clocks or rails on) so we might be wasting > > power for nothing. > > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make > > it more obvious that this property is intended both to document > > that wakeup from suspend is possible and that we need the PHY for > > said wakeup. > > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume > > it's implicit that if we can wakeup from suspend that we need to > > keep the PHY on. If/when someone shows that a device exists using > > dwc2 where we can wakeup from suspend without the PHY they can add > > a new property. > > > > Changes in v2: None > > > > Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++ > > 1 file changed, 3 insertions(+) > > checking file Documentation/devicetree/bindings/usb/dwc2.txt > Hunk #1 FAILED at 37. > Hunk #2 succeeded at 52 (offset -1 lines). > 1 out of 2 hunks FAILED Can you try applying this and the next two patches again? ...or let me know that you'd like me to repost? Thanks! -Doug
diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt index f70f3aee4bfc..1c5e29d23c51 100644 --- a/Documentation/devicetree/bindings/usb/dwc2.txt +++ b/Documentation/devicetree/bindings/usb/dwc2.txt @@ -37,6 +37,8 @@ Refer to phy/phy-bindings.txt for generic phy consumer properties - g-rx-fifo-size: size of rx fifo size in gadget mode. - g-np-tx-fifo-size: size of non-periodic tx fifo size in gadget mode. - g-tx-fifo-size: size of periodic tx fifo per endpoint (except ep0) in gadget mode. +- snps,need-phy-for-wake: If present indicates that the phy needs to be left + on for remote wakeup during suspend. - snps,reset-phy-on-wake: If present indicates that we need to reset the PHY when we detect a wakeup. This is due to a hardware errata. @@ -53,4 +55,5 @@ Example: clock-names = "otg"; phys = <&usbphy>; phy-names = "usb2-phy"; + snps,need-phy-for-wake; };
Some SoCs with a dwc2 USB controller may need to keep the PHY on to support remote wakeup. Allow specifying this as a device tree property. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- For relevant prior discussion on this patch, see: https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org I didn't make any changes from the prior version since I never found out what Rob thought of my previous arguments. If folks want a change, perhaps they could choose from these options: 1. Assume that all dwc2 hosts would like to keep their PHY on for suspend if there's a USB wakeup enabled, thus we totally drop this binding. This doesn't seem super great to me since I'd bet that many devices that use dwc2 weren't designed for USB wakeup (they may not keep enough clocks or rails on) so we might be wasting power for nothing. 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make it more obvious that this property is intended both to document that wakeup from suspend is possible and that we need the PHY for said wakeup. 3. Rename this property to "snps,can-wakeup-from-suspend" and assume it's implicit that if we can wakeup from suspend that we need to keep the PHY on. If/when someone shows that a device exists using dwc2 where we can wakeup from suspend without the PHY they can add a new property. Changes in v2: None Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++ 1 file changed, 3 insertions(+)