This series of patches, together with
<https://patchwork.kernel.org/patch/6652341/> from Chris Zhong and a
dts change allow us to wake up from a USB device on rk3288 boards.
The patches were tested on rk3288-jerry in the chromeos-3.14 kernel.
The chromeos-3.14 kernel tested included a full set of dwc2 backports
from upstream, so this is expected to function upstream once we get
everything setup there.
Douglas Anderson (3):
USB: Export usb_wakeup_enabled_descendants()
Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
Documentation/devicetree/bindings/usb/dwc2.txt | 4 +++
drivers/usb/core/hub.c | 7 ++--
drivers/usb/dwc2/core.h | 2 ++
drivers/usb/dwc2/platform.c | 45 ++++++++++++++++++++++++--
include/linux/usb/hcd.h | 5 +++
5 files changed, 58 insertions(+), 5 deletions(-)
--
2.4.3.573.g4eafbef
In (e583d9d USB: global suspend and remote wakeup don't mix) we
introduced wakeup_enabled_descendants() as a static function. We'd
like to use this function in USB controller drivers to know if we
should keep the controller on during suspend time, since doing so has
a power impact.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/usb/core/hub.c | 7 ++++---
include/linux/usb/hcd.h | 5 +++++
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 43cb2f2..fdc59db 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3034,13 +3034,14 @@ static int usb_disable_remote_wakeup(struct usb_device *udev)
}
/* Count of wakeup-enabled devices at or below udev */
-static unsigned wakeup_enabled_descendants(struct usb_device *udev)
+unsigned usb_wakeup_enabled_descendants(struct usb_device *udev)
{
struct usb_hub *hub = usb_hub_to_struct_hub(udev);
return udev->do_remote_wakeup +
(hub ? hub->wakeup_enabled_descendants : 0);
}
+EXPORT_SYMBOL_GPL(usb_wakeup_enabled_descendants);
/*
* usb_port_suspend - suspend a usb device's upstream port
@@ -3149,7 +3150,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
* Therefore we will turn on the suspend feature if udev or any of its
* descendants is enabled for remote wakeup.
*/
- else if (PMSG_IS_AUTO(msg) || wakeup_enabled_descendants(udev) > 0)
+ else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
status = set_port_feature(hub->hdev, port1,
USB_PORT_FEAT_SUSPEND);
else {
@@ -3548,7 +3549,7 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg)
}
if (udev)
hub->wakeup_enabled_descendants +=
- wakeup_enabled_descendants(udev);
+ usb_wakeup_enabled_descendants(udev);
}
if (hdev->do_remote_wakeup && hub->quirk_check_port_auto_suspend) {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index c9aa779..30d74c9 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -626,11 +626,16 @@ extern wait_queue_head_t usb_kill_urb_queue;
#define usb_endpoint_out(ep_dir) (!((ep_dir) & USB_DIR_IN))
#ifdef CONFIG_PM
+extern unsigned usb_wakeup_enabled_descendants(struct usb_device *udev);
extern void usb_root_hub_lost_power(struct usb_device *rhdev);
extern int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg);
extern int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg);
extern void usb_hcd_resume_root_hub(struct usb_hcd *hcd);
#else
+static inline unsigned usb_wakeup_enabled_descendants(struct usb_device *udev)
+{
+ return 0;
+}
static inline void usb_hcd_resume_root_hub(struct usb_hcd *hcd)
{
return;
--
2.4.3.573.g4eafbef
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 <[email protected]>
---
Documentation/devicetree/bindings/usb/dwc2.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
index fd132cb..84d258d 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -17,6 +17,9 @@ Refer to clk/clock-bindings.txt for generic clock consumer properties
Optional properties:
- phys: phy provider specifier
- phy-names: shall be "usb2-phy"
+- snps,need-phy-for-wake: if present indicates that the phy needs to be left
+ on for remote wakeup during suspend.
+
Refer to phy/phy-bindings.txt for generic phy consumer properties
- dr_mode: shall be one of "host", "peripheral" and "otg"
Refer to usb/generic.txt
@@ -35,4 +38,5 @@ Example:
clock-names = "otg";
phys = <&usbphy>;
phy-names = "usb2-phy";
+ snps,need-phy-for-wake;
};
--
2.4.3.573.g4eafbef
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.
If we later find some users of dwc2 that can support wakeup without
keeping the PHY on we may want to add a way to call
device_set_wakeup_capable() without keeping the PHY on at suspend
time.
Signed-off-by: Chris Zhong <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/usb/dwc2/core.h | 2 ++
drivers/usb/dwc2/platform.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 45 insertions(+), 2 deletions(-)
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;
}
--
2.4.3.573.g4eafbef
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 ?
--
balbi
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
--
balbi
Felipe,
On Mon, Jul 6, 2015 at 11:34 AM, Felipe Balbi <[email protected]> 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 <[email protected]> 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.
--
balbi
Hi,
On Mon, Jul 6, 2015 at 12:02 PM, Felipe Balbi <[email protected]> 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