| Message ID | 1436207224-21849-4-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Mon, Jul 06, 2015 at 11:27:04AM -0700, Douglas Anderson wrote: > @@ -282,6 +296,28 @@ static int dwc2_driver_probe(struct platform_device *dev) > return retval; > } > > +static bool __maybe_unused dwc2_can_poweroff_phy(struct dwc2_hsotg *dwc2) > +{ > + struct usb_device *root_hub = dwc2_hsotg_to_hcd(dwc2)->self.root_hub; what happens on peripheral only builds ?
On Mon, 6 Jul 2015, Douglas Anderson wrote: > If the 'snps,need-phy-for-wake' is set in the device tree then: > > - We know that we can wakeup, so call device_set_wakeup_capable(). > The USB core will use this knowledge to enable wakeup by default. > - We know that we should keep the PHY on during suspend if something > on our root hub needs remote wakeup. This requires the patch (USB: > Export usb_wakeup_enabled_descendants()). Note that we don't keep > the PHY on at suspend time if it's not needed because it would be a > power draw. You know, this is the first time I've run across this optimization. In principle it applies to any USB host controller, not just to PHYs. There's no reason to enable wakeup for a controller if none of the attached devices can issue a wakeup request. I don't know if implementing this in other HCDs would save any power. Any ideas? Alan Stern
On Mon, Jul 06, 2015 at 02:58:16PM -0400, Alan Stern wrote: > On Mon, 6 Jul 2015, Douglas Anderson wrote: > > > If the 'snps,need-phy-for-wake' is set in the device tree then: > > > > - We know that we can wakeup, so call device_set_wakeup_capable(). > > The USB core will use this knowledge to enable wakeup by default. > > - We know that we should keep the PHY on during suspend if something > > on our root hub needs remote wakeup. This requires the patch (USB: > > Export usb_wakeup_enabled_descendants()). Note that we don't keep > > the PHY on at suspend time if it's not needed because it would be a > > power draw. > > You know, this is the first time I've run across this optimization. > > In principle it applies to any USB host controller, not just to PHYs. > There's no reason to enable wakeup for a controller if none of the > attached devices can issue a wakeup request. > > I don't know if implementing this in other HCDs would save any power. > Any ideas? most likely it would. Enabling wakeup usually boils down to keeping a tiny part of the controller (or PHY) powered up. Sometimes that lies in an always-on power domain, so there would be no difference. cheers
Felipe, On Mon, Jul 6, 2015 at 11:34 AM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Mon, Jul 06, 2015 at 11:27:04AM -0700, Douglas Anderson wrote: >> @@ -282,6 +296,28 @@ static int dwc2_driver_probe(struct platform_device *dev) >> return retval; >> } >> >> +static bool __maybe_unused dwc2_can_poweroff_phy(struct dwc2_hsotg *dwc2) >> +{ >> + struct usb_device *root_hub = dwc2_hsotg_to_hcd(dwc2)->self.root_hub; > > what happens on peripheral only builds ? The function is only called on if "dwc2_is_device_mode(dwc2)" returns false. I think that means we're safe, right? -Doug
On Mon, Jul 06, 2015 at 12:32:56PM -0700, Doug Anderson wrote: > Felipe, > > On Mon, Jul 6, 2015 at 11:34 AM, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > > > On Mon, Jul 06, 2015 at 11:27:04AM -0700, Douglas Anderson wrote: > >> @@ -282,6 +296,28 @@ static int dwc2_driver_probe(struct platform_device *dev) > >> return retval; > >> } > >> > >> +static bool __maybe_unused dwc2_can_poweroff_phy(struct dwc2_hsotg *dwc2) > >> +{ > >> + struct usb_device *root_hub = dwc2_hsotg_to_hcd(dwc2)->self.root_hub; > > > > what happens on peripheral only builds ? > > The function is only called on if "dwc2_is_device_mode(dwc2)" returns > false. I think that means we're safe, right? heh, missed that. Should be safe, yeah.
Hi, On Mon, Jul 6, 2015 at 12:02 PM, Felipe Balbi <balbi@ti.com> wrote: > On Mon, Jul 06, 2015 at 02:58:16PM -0400, Alan Stern wrote: >> On Mon, 6 Jul 2015, Douglas Anderson wrote: >> >> > If the 'snps,need-phy-for-wake' is set in the device tree then: >> > >> > - We know that we can wakeup, so call device_set_wakeup_capable(). >> > The USB core will use this knowledge to enable wakeup by default. >> > - We know that we should keep the PHY on during suspend if something >> > on our root hub needs remote wakeup. This requires the patch (USB: >> > Export usb_wakeup_enabled_descendants()). Note that we don't keep >> > the PHY on at suspend time if it's not needed because it would be a >> > power draw. >> >> You know, this is the first time I've run across this optimization. >> >> In principle it applies to any USB host controller, not just to PHYs. >> There's no reason to enable wakeup for a controller if none of the >> attached devices can issue a wakeup request. >> >> I don't know if implementing this in other HCDs would save any power. >> Any ideas? > > most likely it would. Enabling wakeup usually boils down to keeping a > tiny part of the controller (or PHY) powered up. Sometimes that lies in > an always-on power domain, so there would be no difference. As per Andrew Bresticker (CCed on this email), the optimization made sense in Tegra. If you're willing to look into the chromeos-3.10 tree, you can see that Andrew added a usb_port_may_wakeup() call in <https://chromium-review.googlesource.com/196593>. He then used it in the tegra XHCI driver in <https://chromium-review.googlesource.com/196594>. Recently I talked to Andrew and he indicated that rather than add usb_port_may_wakeup() like he did it probably made sense to just export usb_wakeup_enabled_descendants(). -Doug
On Mon, 6 Jul 2015, Felipe Balbi wrote: > > You know, this is the first time I've run across this optimization. > > > > In principle it applies to any USB host controller, not just to PHYs. > > There's no reason to enable wakeup for a controller if none of the > > attached devices can issue a wakeup request. > > > > I don't know if implementing this in other HCDs would save any power. > > Any ideas? > > most likely it would. Enabling wakeup usually boils down to keeping a > tiny part of the controller (or PHY) powered up. Sometimes that lies in > an always-on power domain, so there would be no difference. Doug, how would you feel about reworking the patch that exports usb_wakeup_enabled_descendants()? Instead of doing it that way, create and export a new subroutine in hcd.c called usb_hcd_wakeup_not_needed(), or something similar. The idea is that a host controller driver can do something like this: do_wakeup = device_may_wakeup(...); if (usb_hcd_wakeup_not_needed(hcd)) do_wakeup = false; It encapsulates what you want in a form that can easily be used by every HCD. We can then add this call into the HCDs, over time. (Merging a change like this would be a challenge. I guess Felipe would have to put it in a separate branch which Greg could pull, or vice versa, so that the new routine would be available to patches submitted to either maintainer.) Alan Stern
> Doug, how would you feel about reworking the patch that exports > usb_wakeup_enabled_descendants()? Instead of doing it that way, create > and export a new subroutine in hcd.c called > usb_hcd_wakeup_not_needed(), or something similar. We have a use case with another host controller (Tegra, which I think is still in the process of being upstreamed) where we are able to power down PHYs (and thus reduce power consumption) per port. I think we should prefer the more flexible 'number of wake devices in subtree' interface to be able to support cases like that. (And for the simple case, 'if (usb_hcd_wakeup_not_needed(hcd))' and 'if (!usb_wakeup_enabled_descendants(hcd->self.root_hub))' look pretty similar anyway.)
On Tue, 7 Jul 2015, Julius Werner wrote: > > Doug, how would you feel about reworking the patch that exports > > usb_wakeup_enabled_descendants()? Instead of doing it that way, create > > and export a new subroutine in hcd.c called > > usb_hcd_wakeup_not_needed(), or something similar. > > We have a use case with another host controller (Tegra, which I think > is still in the process of being upstreamed) where we are able to > power down PHYs (and thus reduce power consumption) per port. I think > we should prefer the more flexible 'number of wake devices in subtree' > interface to be able to support cases like that. (And for the simple > case, 'if (usb_hcd_wakeup_not_needed(hcd))' and 'if > (!usb_wakeup_enabled_descendants(hcd->self.root_hub))' look pretty > similar anyway.) Okay, that's a good point. But I don't see how you will make it work when the root hub itself is not enabled for wakeup and a non-hub device plugged into one of the root hub's ports is enabled. It seems like you would need a usb_hcd_wakeup_not_needed(hcd, port) subroutine. Alan Stern
> But I don't see how you will make it work when the root hub itself is > not enabled for wakeup and a non-hub device plugged into one of the > root hub's ports is enabled. > > It seems like you would need a usb_hcd_wakeup_not_needed(hcd, port) > subroutine. We'd just put that in the Tegra platform driver, I guess. Iterate over all ports, call usb_wakeup_enabled_descendants() on the connected device (if any) and disable that port's PHY if it returns 0 or wasn't connected. Since usb_wakeup_enabled_descendants() also counts do_remote_wakeup from the device itself and is safe to call even on non-hubs, that should work for all cases.
On Wed, 8 Jul 2015, Julius Werner wrote: > > But I don't see how you will make it work when the root hub itself is > > not enabled for wakeup and a non-hub device plugged into one of the > > root hub's ports is enabled. > > > > It seems like you would need a usb_hcd_wakeup_not_needed(hcd, port) > > subroutine. > > We'd just put that in the Tegra platform driver, I guess. Iterate over > all ports, call usb_wakeup_enabled_descendants() on the connected > device (if any) and disable that port's PHY if it returns 0 or wasn't > connected. Since usb_wakeup_enabled_descendants() also counts > do_remote_wakeup from the device itself and is safe to call even on > non-hubs, that should work for all cases. All right, that seems reasonable. But remember not to do this if wakeup is enabled on the root hub -- in that case all the PHYs must remain powered because unplugging and plugging are both wakeup events. Alan Stern
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 53b8de0..b60a1e8 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -687,6 +687,8 @@ struct dwc2_hsotg { enum usb_dr_mode dr_mode; unsigned int hcd_enabled:1; unsigned int gadget_enabled:1; + unsigned int need_phy_for_wake:1; + unsigned int phy_off_for_suspend:1; struct phy *phy; struct usb_phy *uphy; diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index 9093530..38fce75 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -42,7 +42,9 @@ #include <linux/of_device.h> #include <linux/mutex.h> #include <linux/platform_device.h> +#include <linux/usb.h> +#include <linux/usb/hcd.h> #include <linux/usb/of.h> #include "core.h" @@ -222,6 +224,10 @@ static int dwc2_driver_probe(struct platform_device *dev) hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node); + hsotg->need_phy_for_wake = + of_property_read_bool(dev->dev.of_node, + "snps,need-phy-for-wake"); + /* * Attempt to find a generic PHY, then look for an old style * USB PHY @@ -265,6 +271,14 @@ static int dwc2_driver_probe(struct platform_device *dev) hsotg->gadget_enabled = 1; } + /* + * If we need PHY for wakeup we must be wakeup capable. + * When we have a device that can wake without the PHY we + * can adjust this condition. + */ + if (hsotg->need_phy_for_wake) + device_set_wakeup_capable(&dev->dev, true); + if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) { retval = dwc2_hcd_init(hsotg, irq); if (retval) { @@ -282,6 +296,28 @@ static int dwc2_driver_probe(struct platform_device *dev) return retval; } +static bool __maybe_unused dwc2_can_poweroff_phy(struct dwc2_hsotg *dwc2) +{ + struct usb_device *root_hub = dwc2_hsotg_to_hcd(dwc2)->self.root_hub; + + if (dwc2->lx_state == DWC2_L0) + return false; + + /* If the controller isn't allowed to wakeup then we can power off. */ + if (!device_may_wakeup(dwc2->dev)) + return true; + + /* + * We don't want to power off the PHY if something under the + * root hub has wakeup enabled. + */ + if (usb_wakeup_enabled_descendants(root_hub)) + return false; + + /* No reason to keep the PHY powered, so allow poweroff */ + return true; +} + static int __maybe_unused dwc2_suspend(struct device *dev) { struct dwc2_hsotg *dwc2 = dev_get_drvdata(dev); @@ -290,8 +326,10 @@ static int __maybe_unused dwc2_suspend(struct device *dev) if (dwc2_is_device_mode(dwc2)) { ret = s3c_hsotg_suspend(dwc2); } else { - if (dwc2->lx_state == DWC2_L0) + if (!dwc2_can_poweroff_phy(dwc2)) return 0; + + dwc2->phy_off_for_suspend = true; phy_exit(dwc2->phy); phy_power_off(dwc2->phy); @@ -307,9 +345,12 @@ static int __maybe_unused dwc2_resume(struct device *dev) if (dwc2_is_device_mode(dwc2)) { ret = s3c_hsotg_resume(dwc2); } else { + if (!dwc2->phy_off_for_suspend) + return ret; + phy_power_on(dwc2->phy); phy_init(dwc2->phy); - + dwc2->phy_off_for_suspend = false; } return ret; }