LinuxLists.cc - [PATCH v14 0/7] USB DWC3 host wake up support from system suspend

[permalink] [raw]

Subject: [PATCH v14 1/7] dt-bindings: usb: dwc3: Add wakeup-source property support

Added support for wakeup-source property. This property can be
used to check and power down the phy during system suspend if
wake up is not supported.

Signed-off-by: Sandeep Maheswaram <[email protected]>
Reviewed-by: Matthias Kaehlcke <[email protected]>
Acked-by: Krzysztof Kozlowski <[email protected]>
---
Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index f4471f8..4d4de2f 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -341,6 +341,11 @@ properties:
This port is used with the 'usb-role-switch' property to connect the
dwc3 to type C connector.

+ wakeup-source:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description:
+ Enable USB remote wakeup.
+
unevaluatedProperties: false

required:
--
2.7.4

2022-04-20 18:45:16

[permalink] [raw]

Subject: [PATCH v14 6/7] usb: dwc3: qcom: Keep power domain on to retain controller status

Keep the power domain always on during runtime suspend or if the
controller supports wakeup in order to retain controller status
and to support wakeup from devices.

Signed-off-by: Sandeep Maheswaram <[email protected]>
---
v14:
Removed the unwanted spaces between the type
and the variable name in dwc3_qcom_probe.
Avoiding device_may_wakeup condition check multiple times.

v13:
Added GENPD_FLAG_RPM_ALWAYS_ON irrespective of wakeup capability.
Enable GENPD_FLAG_ALWAYS_ON only if device_may_wakeup is true.

drivers/usb/dwc3/dwc3-qcom.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 9804a19..1f9589a 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -17,6 +17,7 @@
#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/phy/phy.h>
+#include <linux/pm_domain.h>
#include <linux/usb/of.h>
#include <linux/reset.h>
#include <linux/iopoll.h>
@@ -718,12 +719,13 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)

static int dwc3_qcom_probe(struct platform_device *pdev)
{
- struct device_node *np = pdev->dev.of_node;
- struct device *dev = &pdev->dev;
- struct dwc3_qcom *qcom;
- struct resource *res, *parent_res = NULL;
- int ret, i;
- bool ignore_pipe_clk;
+ struct device_node *np = pdev->dev.of_node;
+ struct device *dev = &pdev->dev;
+ struct dwc3_qcom *qcom;
+ struct resource *res, *parent_res = NULL;
+ int ret, i;
+ bool ignore_pipe_clk;
+ struct generic_pm_domain *genpd;

qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
if (!qcom)
@@ -732,6 +734,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, qcom);
qcom->dev = &pdev->dev;

+ genpd = pd_to_genpd(qcom->dev->pm_domain);
+
if (has_acpi_companion(dev)) {
qcom->acpi_pdata = acpi_device_get_match_data(dev);
if (!qcom->acpi_pdata) {
@@ -839,7 +843,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
if (ret)
goto interconnect_exit;

- device_init_wakeup(&pdev->dev, 1);
+ genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
+
+ if (device_may_wakeup(&qcom->dwc3->dev)) {
+ genpd->flags |= GENPD_FLAG_ALWAYS_ON;
+ device_init_wakeup(&pdev->dev, true);
+ }
qcom->is_suspended = false;
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
--
2.7.4

2022-04-21 17:53:14

[permalink] [raw]

Subject: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

From: Matthias Kaehlcke <[email protected]>

Add device_children_wakeup_capable() which checks whether the device itself
or one if its descendants is wakeup capable.

Suggested-by: Felipe Balbi <[email protected]>
Signed-off-by: Sandeep Maheswaram <[email protected]>
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
drivers/base/power/wakeup.c | 18 ++++++++++++++++++
include/linux/pm_wakeup.h | 7 +++++++
2 files changed, 25 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index a57d469..1900637 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -541,6 +541,24 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
}
EXPORT_SYMBOL_GPL(device_set_wakeup_enable);

+static int __device_children_wakeup_capable(struct device *dev, void *dummy)
+{
+ return device_may_wakeup(dev) ||
+ device_for_each_child(dev, NULL, __device_children_wakeup_capable);
+}
+
+/**
+ * device_children_wakeup_capable - Check whether a device or one of its descendants is
+ * wakeup capable.
+ * @dev: Device to handle.
+ */
+bool device_children_wakeup_capable(struct device *dev)
+{
+ return __device_children_wakeup_capable(dev, NULL);
+}
+EXPORT_SYMBOL_GPL(device_children_wakeup_capable);
+
+
/**
* wakeup_source_not_registered - validate the given wakeup source.
* @ws: Wakeup source to be validated.
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index 196a157..9a3005b 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -109,6 +109,7 @@ extern struct wakeup_source *wakeup_sources_walk_next(struct wakeup_source *ws);
extern int device_wakeup_enable(struct device *dev);
extern int device_wakeup_disable(struct device *dev);
extern void device_set_wakeup_capable(struct device *dev, bool capable);
+extern bool device_children_wakeup_capable(struct device *dev);
extern int device_init_wakeup(struct device *dev, bool val);
extern int device_set_wakeup_enable(struct device *dev, bool enable);
extern void __pm_stay_awake(struct wakeup_source *ws);
@@ -186,6 +187,12 @@ static inline bool device_wakeup_path(struct device *dev)

static inline void device_set_wakeup_path(struct device *dev) {}

+static inline bool device_children_wakeup_capable(struct device *dev)
+{
+ return false;
+}
+
+
static inline void __pm_stay_awake(struct wakeup_source *ws) {}

static inline void pm_stay_awake(struct device *dev) {}
--
2.7.4

2022-04-21 20:35:04

[permalink] [raw]

Subject: [PATCH v14 7/7] arm64: dts: qcom: sc7280: Add wakeup-source property for USB node

Adding wakeup-source property for USB controller in SC7280.
This property is added to inform that the USB controller is
wake up capable and to conditionally power down the phy during
system suspend.

Signed-off-by: Sandeep Maheswaram <[email protected]>
Reviewed-by: Matthias Kaehlcke <[email protected]>
---
arch/arm64/boot/dts/qcom/sc7280.dtsi | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 9ece357..00bacc4 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3106,6 +3106,7 @@
phys = <&usb_1_hsphy>, <&usb_1_ssphy>;
phy-names = "usb2-phy", "usb3-phy";
maximum-speed = "super-speed";
+ wakeup-source;
};
};

--
2.7.4

2022-04-22 09:36:41

[permalink] [raw]

Subject: [PATCH v14 5/7] usb: dwc3: qcom: Configure wakeup interrupts during suspend

Configure DP/DM interrupts to detect line state changes based on
hs_phy_mode. Enable the triggers opposite of what the current
DP, DM levels. For HS/FS mode enable DM interrupt and for LS enable DP
interrupt.

Signed-off-by: Sandeep Maheswaram <[email protected]>
Reviewed-by: Matthias Kaehlcke <[email protected]>
---
drivers/usb/dwc3/dwc3-qcom.c | 26 ++++++++++++++++++++------
1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 7352124..9804a19 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -316,22 +316,36 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)

static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
{
- dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
+ struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);

- dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);

- dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
+ if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_LS) {
+ dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
+ } else if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_HS) {
+ dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
+ } else {
+ dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
+ }

dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq);
}

static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
{
- dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq);
+ struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);

- dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq);
+ dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq);

- dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq);
+ if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_LS) {
+ dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq);
+ } else if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_HS) {
+ dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq);
+ } else {
+ dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq);
+ dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq);
+ }

dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq);
}
--
2.7.4

2022-04-22 14:09:55

[permalink] [raw]

Subject: [PATCH v14 4/7] usb: dwc3: qcom: Add helper functions to enable,disable wake irqs

Adding helper functions to enable,disable wake irqs to make
the code simple and readable.

Signed-off-by: Sandeep Maheswaram <[email protected]>
Reviewed-by: Matthias Kaehlcke <[email protected]>
---
drivers/usb/dwc3/dwc3-qcom.c | 58 ++++++++++++++++++++------------------------
1 file changed, 26 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 6cba990..7352124 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -296,50 +296,44 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
icc_put(qcom->icc_path_apps);
}

+static void dwc3_qcom_enable_wakeup_irq(int irq)
+{
+ if (!irq)
+ return;
+
+ enable_irq(irq);
+ enable_irq_wake(irq);
+}
+
+static void dwc3_qcom_disable_wakeup_irq(int irq)
+{
+ if (!irq)
+ return;
+
+ disable_irq_wake(irq);
+ disable_irq_nosync(irq);
+}
+
static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
{
- if (qcom->hs_phy_irq) {
- disable_irq_wake(qcom->hs_phy_irq);
- disable_irq_nosync(qcom->hs_phy_irq);
- }
+ dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);

- if (qcom->dp_hs_phy_irq) {
- disable_irq_wake(qcom->dp_hs_phy_irq);
- disable_irq_nosync(qcom->dp_hs_phy_irq);
- }
+ dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);

- if (qcom->dm_hs_phy_irq) {
- disable_irq_wake(qcom->dm_hs_phy_irq);
- disable_irq_nosync(qcom->dm_hs_phy_irq);
- }
+ dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);

- if (qcom->ss_phy_irq) {
- disable_irq_wake(qcom->ss_phy_irq);
- disable_irq_nosync(qcom->ss_phy_irq);
- }
+ dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq);
}

static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
{
- if (qcom->hs_phy_irq) {
- enable_irq(qcom->hs_phy_irq);
- enable_irq_wake(qcom->hs_phy_irq);
- }
+ dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq);

- if (qcom->dp_hs_phy_irq) {
- enable_irq(qcom->dp_hs_phy_irq);
- enable_irq_wake(qcom->dp_hs_phy_irq);
- }
+ dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq);

- if (qcom->dm_hs_phy_irq) {
- enable_irq(qcom->dm_hs_phy_irq);
- enable_irq_wake(qcom->dm_hs_phy_irq);
- }
+ dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq);

- if (qcom->ss_phy_irq) {
- enable_irq(qcom->ss_phy_irq);
- enable_irq_wake(qcom->ss_phy_irq);
- }
+ dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq);
}

static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
--
2.7.4

2022-04-22 22:18:22

[permalink] [raw]

Subject: [PATCH v14 3/7] usb: dwc3: core: Host wake up support from system suspend

During suspend read the status of all port and set hs phy mode
based on current speed. Use this hs phy mode to configure wakeup
interrupts in qcom glue driver.

Check wakeup-source property for dwc3 core node to set the
wakeup capability. Drop the device_init_wakeup call from
runtime suspend and resume.

Also check during suspend if any wakeup capable devices are
connected to the controller (directly or through hubs), if there
are none set a flag to indicate that the PHY is powered
down during suspend.

Signed-off-by: Sandeep Maheswaram <[email protected]>
---
v14:
Used device_children_wakeup_capable instead of usb_wakeup_enabled_descendants.

v13:
Changed dwc3_set_phy_speed_mode to dwc3_check_phy_speed_mode.
Removed device_init_wakeup calls from dwc3_runtime_suspend and dwc3_runtime_resume
as we have a new dt property wakeup-source.

drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++-------------
drivers/usb/dwc3/core.h | 4 ++++
drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++
3 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 1170b80..898aa66 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -32,6 +32,7 @@
#include <linux/usb/gadget.h>
#include <linux/usb/of.h>
#include <linux/usb/otg.h>
+#include <linux/usb/hcd.h>

#include "core.h"
#include "gadget.h"
@@ -1723,6 +1724,7 @@ static int dwc3_probe(struct platform_device *pdev)

platform_set_drvdata(pdev, dwc);
dwc3_cache_hwparams(dwc);
+ device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));

spin_lock_init(&dwc->lock);
mutex_init(&dwc->mutex);
@@ -1865,6 +1867,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
{
unsigned long flags;
u32 reg;
+ struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);

switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
@@ -1877,10 +1880,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
dwc3_core_exit(dwc);
break;
case DWC3_GCTL_PRTCAP_HOST:
- if (!PMSG_IS_AUTO(msg)) {
- dwc3_core_exit(dwc);
- break;
- }
+ dwc3_check_phy_speed_mode(dwc);

/* Let controller to suspend HSPHY before PHY driver suspends */
if (dwc->dis_u2_susphy_quirk ||
@@ -1896,6 +1896,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)

phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
+
+ if (!PMSG_IS_AUTO(msg)) {
+ if (device_may_wakeup(dwc->dev) &&
+ device_children_wakeup_capable(&hcd->self.root_hub->dev)) {
+ dwc->phy_power_off = false;
+ } else {
+ dwc->phy_power_off = true;
+ dwc3_core_exit(dwc);
+ }
+ }
break;
case DWC3_GCTL_PRTCAP_OTG:
/* do nothing during runtime_suspend */
@@ -1939,11 +1949,12 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
break;
case DWC3_GCTL_PRTCAP_HOST:
if (!PMSG_IS_AUTO(msg)) {
- ret = dwc3_core_init_for_resume(dwc);
- if (ret)
- return ret;
- dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
- break;
+ if (dwc->phy_power_off) {
+ ret = dwc3_core_init_for_resume(dwc);
+ if (ret)
+ return ret;
+ dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
+ }
}
/* Restore GUSB2PHYCFG bits that were modified in suspend */
reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
@@ -2015,8 +2026,6 @@ static int dwc3_runtime_suspend(struct device *dev)
if (ret)
return ret;

- device_init_wakeup(dev, true);
-
return 0;
}

@@ -2025,8 +2034,6 @@ static int dwc3_runtime_resume(struct device *dev)
struct dwc3 *dwc = dev_get_drvdata(dev);
int ret;

- device_init_wakeup(dev, false);
-
ret = dwc3_resume_common(dwc, PMSG_AUTO_RESUME);
if (ret)
return ret;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5c9d467..6a5845f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1154,6 +1154,9 @@ struct dwc3 {

bool phys_ready;

+ unsigned int hs_phy_mode;
+ bool phy_power_off;
+
struct ulpi *ulpi;
bool ulpi_ready;

@@ -1537,6 +1540,7 @@ int dwc3_core_soft_reset(struct dwc3 *dwc);
#if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
int dwc3_host_init(struct dwc3 *dwc);
void dwc3_host_exit(struct dwc3 *dwc);
+void dwc3_check_phy_speed_mode(struct dwc3 *dwc);
#else
static inline int dwc3_host_init(struct dwc3 *dwc)
{ return 0; }
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index eda8719..3902b56 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -13,6 +13,7 @@
#include <linux/platform_device.h>

#include "core.h"
+#include "../host/xhci.h"

static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc,
int irq, char *name)
@@ -138,3 +139,26 @@ void dwc3_host_exit(struct dwc3 *dwc)
{
platform_device_unregister(dwc->xhci);
}
+
+void dwc3_check_phy_speed_mode(struct dwc3 *dwc)
+{
+ int i, num_ports;
+ u32 reg;
+ struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
+ struct xhci_hcd *xhci_hcd = hcd_to_xhci(hcd);
+
+ dwc->hs_phy_mode = 0;
+
+ reg = readl(&xhci_hcd->cap_regs->hcs_params1);
+
+ num_ports = HCS_MAX_PORTS(reg);
+ for (i = 0; i < num_ports; i++) {
+ reg = readl(&xhci_hcd->op_regs->port_status_base + i * NUM_PORT_REGS);
+ if (reg & PORT_PE) {
+ if (DEV_HIGHSPEED(reg) || DEV_FULLSPEED(reg))
+ dwc->hs_phy_mode |= PHY_MODE_USB_HOST_HS;
+ else if (DEV_LOWSPEED(reg))
+ dwc->hs_phy_mode |= PHY_MODE_USB_HOST_LS;
+ }
+ }
+}
--
2.7.4

2022-04-22 22:33:27

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
<[email protected]> wrote:
>
> From: Matthias Kaehlcke <[email protected]>
>
> Add device_children_wakeup_capable() which checks whether the device itself
> or one if its descendants is wakeup capable.

device_wakeup_path() exists for a very similar purpose.

Is it not usable for whatever you need the new function introduced here?

> Suggested-by: Felipe Balbi <[email protected]>
> Signed-off-by: Sandeep Maheswaram <[email protected]>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
> drivers/base/power/wakeup.c | 18 ++++++++++++++++++
> include/linux/pm_wakeup.h | 7 +++++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index a57d469..1900637 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -541,6 +541,24 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
> }
> EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
>
> +static int __device_children_wakeup_capable(struct device *dev, void *dummy)
> +{
> + return device_may_wakeup(dev) ||
> + device_for_each_child(dev, NULL, __device_children_wakeup_capable);
> +}
> +
> +/**
> + * device_children_wakeup_capable - Check whether a device or one of its descendants is
> + * wakeup capable.
> + * @dev: Device to handle.
> + */
> +bool device_children_wakeup_capable(struct device *dev)
> +{
> + return __device_children_wakeup_capable(dev, NULL);
> +}
> +EXPORT_SYMBOL_GPL(device_children_wakeup_capable);
> +
> +
> /**
> * wakeup_source_not_registered - validate the given wakeup source.
> * @ws: Wakeup source to be validated.
> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
> index 196a157..9a3005b 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -109,6 +109,7 @@ extern struct wakeup_source *wakeup_sources_walk_next(struct wakeup_source *ws);
> extern int device_wakeup_enable(struct device *dev);
> extern int device_wakeup_disable(struct device *dev);
> extern void device_set_wakeup_capable(struct device *dev, bool capable);
> +extern bool device_children_wakeup_capable(struct device *dev);
> extern int device_init_wakeup(struct device *dev, bool val);
> extern int device_set_wakeup_enable(struct device *dev, bool enable);
> extern void __pm_stay_awake(struct wakeup_source *ws);
> @@ -186,6 +187,12 @@ static inline bool device_wakeup_path(struct device *dev)
>
> static inline void device_set_wakeup_path(struct device *dev) {}
>
> +static inline bool device_children_wakeup_capable(struct device *dev)
> +{
> + return false;
> +}
> +
> +
> static inline void __pm_stay_awake(struct wakeup_source *ws) {}
>
> static inline void pm_stay_awake(struct device *dev) {}
> --
> 2.7.4
>

2022-04-22 22:48:11

[permalink] [raw]

Subject: Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> <[email protected]> wrote:
> >
> > From: Matthias Kaehlcke <[email protected]>
> >
> > Add device_children_wakeup_capable() which checks whether the device itself
> > or one if its descendants is wakeup capable.
>
> device_wakeup_path() exists for a very similar purpose.
>
> Is it not usable for whatever you need the new function introduced here?

I wasn't aware of it's function, there are no doc comments and the
name isn't really self explanatory.

In a quick test device_wakeup_path() returned inconsistent values for the
root hub, sometimes true, others false when a wakeup capable USB device was
connected.

2022-04-25 15:32:38

[permalink] [raw]

Subject: Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

Hi Matthias,

On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote:
> On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> > <[email protected]> wrote:
> > >
> > > From: Matthias Kaehlcke <[email protected]>
> > >
> > > Add device_children_wakeup_capable() which checks whether the device itself
> > > or one if its descendants is wakeup capable.
> >
> > device_wakeup_path() exists for a very similar purpose.
> >
> > Is it not usable for whatever you need the new function introduced here?
>
> I wasn't aware of it's function, there are no doc comments and the
> name isn't really self explanatory.
>
> In a quick test device_wakeup_path() returned inconsistent values for the
> root hub, sometimes true, others false when a wakeup capable USB device was
> connected.

We will also test the same to double confirm the behavior of
device_wakeup_path(). I am assuming that you checked device_wakeup_path()
only during system suspend path.

Here is what I understood by looking at __device_suspend(). Please share
your thoughts on this.

power.wakeup_path is set to true for the parent *after* a wakeup capable
device is suspended. This means when the root hub(s) is suspended, it is
propagated to xhci-plat and when xhci-plat is suspended, it is propagated
to dwc3. bottom up propgation during system suspend.

I believe we can directly check something like this in the dwc3 driver
instead of having another wrapper like device_children_wakeup_capable().

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 1170b80..a783257 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
break;
case DWC3_GCTL_PRTCAP_HOST:
if (!PMSG_IS_AUTO(msg)) {
+ /*
+ * Don't kill the host when dwc3 is wakeup capable and
+ * its children needs wakeup.
+ */
+ if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev))
+ handle_it();
+ } else {
dwc3_core_exit(dwc);
- break;
}

/* Let controller to suspend HSPHY before PHY driver suspends */

Thanks,
Pavan

2022-04-30 11:30:47

[permalink] [raw]

Subject: Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

Hi Matthias,

On Mon, Apr 25, 2022 at 06:33:03PM +0530, Pavan Kondeti wrote:
> Hi Matthias,
>
> On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote:
> > On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> > > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> > > <[email protected]> wrote:
> > > >
> > > > From: Matthias Kaehlcke <[email protected]>
> > > >
> > > > Add device_children_wakeup_capable() which checks whether the device itself
> > > > or one if its descendants is wakeup capable.
> > >
> > > device_wakeup_path() exists for a very similar purpose.
> > >
> > > Is it not usable for whatever you need the new function introduced here?
> >
> > I wasn't aware of it's function, there are no doc comments and the
> > name isn't really self explanatory.
> >
> > In a quick test device_wakeup_path() returned inconsistent values for the
> > root hub, sometimes true, others false when a wakeup capable USB device was
> > connected.
>
> We will also test the same to double confirm the behavior of
> device_wakeup_path(). I am assuming that you checked device_wakeup_path()
> only during system suspend path.
>
> Here is what I understood by looking at __device_suspend(). Please share
> your thoughts on this.
>
> power.wakeup_path is set to true for the parent *after* a wakeup capable
> device is suspended. This means when the root hub(s) is suspended, it is
> propagated to xhci-plat and when xhci-plat is suspended, it is propagated
> to dwc3. bottom up propgation during system suspend.
>
> I believe we can directly check something like this in the dwc3 driver
> instead of having another wrapper like device_children_wakeup_capable().
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 1170b80..a783257 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> break;
> case DWC3_GCTL_PRTCAP_HOST:
> if (!PMSG_IS_AUTO(msg)) {
> + /*
> + * Don't kill the host when dwc3 is wakeup capable and
> + * its children needs wakeup.
> + */
> + if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev))
> + handle_it();
> + } else {
> dwc3_core_exit(dwc);
> - break;
> }
>
> /* Let controller to suspend HSPHY before PHY driver suspends */
>

device_wakeup_path(dwc->dev) is returning true all the time irrespective of
the wakeup capability (and enabled status) of the connected USB devices. That
is because xhci-plat device is configured to wakeup all the time. Since the
child is wakeup capable, its parent i.e dwc3 has device_wakeup_path() set.
device_children_wakeup_capable() will also suffer the problem. However,

device_children_wakeup_capable(&hcd->self.root_hub->dev) is what Sandeep's
patch is using. That is not correct. we have two root hubs (HS and SS) associated
with a USB3 controller and calling it on one root hub is incorrect.
device_children_wakeup_capable() must be called on xhci-plat so that it covers
both HS and SS root hubs

I am thinking of dynamically enabling/disabling xhci-plat wakeup capability so
that the wakeup path is correctly propagated to dwc3. something like below.
Does it make sense to you?

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 649ffd8..be0c55b 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -412,6 +412,9 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
int ret;

+ if (!device_wakeup_path(dev))
+ device_wakeup_disable(dev);
+
if (pm_runtime_suspended(dev))
pm_runtime_resume(dev);

@@ -443,6 +446,8 @@ static int __maybe_unused xhci_plat_resume(struct device *dev)
pm_runtime_set_active(dev);
pm_runtime_enable(dev);

+ device_wakeup_enable(dev);
+
return 0;
}

Thanks,
Pavan

2022-05-01 17:15:42

[permalink] [raw]

Subject: Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

Hi Pavan,

On Fri, Apr 29, 2022 at 06:29:56PM +0530, Pavan Kondeti wrote:
> Hi Matthias,
>
> On Mon, Apr 25, 2022 at 06:33:03PM +0530, Pavan Kondeti wrote:
> > Hi Matthias,
> >
> > On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote:
> > > On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> > > > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> > > > <[email protected]> wrote:
> > > > >
> > > > > From: Matthias Kaehlcke <[email protected]>
> > > > >
> > > > > Add device_children_wakeup_capable() which checks whether the device itself
> > > > > or one if its descendants is wakeup capable.
> > > >
> > > > device_wakeup_path() exists for a very similar purpose.
> > > >
> > > > Is it not usable for whatever you need the new function introduced here?
> > >
> > > I wasn't aware of it's function, there are no doc comments and the
> > > name isn't really self explanatory.
> > >
> > > In a quick test device_wakeup_path() returned inconsistent values for the
> > > root hub, sometimes true, others false when a wakeup capable USB device was
> > > connected.
> >
> > We will also test the same to double confirm the behavior of
> > device_wakeup_path(). I am assuming that you checked device_wakeup_path()
> > only during system suspend path.
> >
> > Here is what I understood by looking at __device_suspend(). Please share
> > your thoughts on this.
> >
> > power.wakeup_path is set to true for the parent *after* a wakeup capable
> > device is suspended. This means when the root hub(s) is suspended, it is
> > propagated to xhci-plat and when xhci-plat is suspended, it is propagated
> > to dwc3. bottom up propgation during system suspend.
> >
> > I believe we can directly check something like this in the dwc3 driver
> > instead of having another wrapper like device_children_wakeup_capable().
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 1170b80..a783257 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > break;
> > case DWC3_GCTL_PRTCAP_HOST:
> > if (!PMSG_IS_AUTO(msg)) {
> > + /*
> > + * Don't kill the host when dwc3 is wakeup capable and
> > + * its children needs wakeup.
> > + */
> > + if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev))
> > + handle_it();
> > + } else {
> > dwc3_core_exit(dwc);
> > - break;
> > }
> >
> > /* Let controller to suspend HSPHY before PHY driver suspends */
> >
>
> device_wakeup_path(dwc->dev) is returning true all the time irrespective of
> the wakeup capability (and enabled status) of the connected USB devices. That
> is because xhci-plat device is configured to wakeup all the time. Since the
> child is wakeup capable, its parent i.e dwc3 has device_wakeup_path() set.
> device_children_wakeup_capable() will also suffer the problem. However,
>
> device_children_wakeup_capable(&hcd->self.root_hub->dev) is what Sandeep's
> patch is using. That is not correct. we have two root hubs (HS and SS) associated
> with a USB3 controller and calling it on one root hub is incorrect.
> device_children_wakeup_capable() must be called on xhci-plat so that it covers
> both HS and SS root hubs

Thanks for pointing that out!

> I am thinking of dynamically enabling/disabling xhci-plat wakeup capability so
> that the wakeup path is correctly propagated to dwc3. something like below.
> Does it make sense to you?
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 649ffd8..be0c55b 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -412,6 +412,9 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> int ret;
>
> + if (!device_wakeup_path(dev))
> + device_wakeup_disable(dev);
> +
> if (pm_runtime_suspended(dev))
> pm_runtime_resume(dev);
>
> @@ -443,6 +446,8 @@ static int __maybe_unused xhci_plat_resume(struct device *dev)
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
>
> + device_wakeup_enable(dev);

I think this also needs to be done conditionally, otherwise it would
create a new wake source on every resume when wakeup is already
enabled.

Other than that this seems to do the trick and keeps the USB layer out of
the dwc3 code.

2022-05-02 23:31:33

[permalink] [raw]

Subject: Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

Hi Matthias,

On Fri, Apr 29, 2022 at 12:19:22PM -0700, Matthias Kaehlcke wrote:
> Hi Pavan,
>
> On Fri, Apr 29, 2022 at 06:29:56PM +0530, Pavan Kondeti wrote:
> > Hi Matthias,
> >
> > On Mon, Apr 25, 2022 at 06:33:03PM +0530, Pavan Kondeti wrote:
> > > Hi Matthias,
> > >
> > > On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote:
> > > > On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> > > > > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > From: Matthias Kaehlcke <[email protected]>
> > > > > >
> > > > > > Add device_children_wakeup_capable() which checks whether the device itself
> > > > > > or one if its descendants is wakeup capable.
> > > > >
> > > > > device_wakeup_path() exists for a very similar purpose.
> > > > >
> > > > > Is it not usable for whatever you need the new function introduced here?
> > > >
> > > > I wasn't aware of it's function, there are no doc comments and the
> > > > name isn't really self explanatory.
> > > >
> > > > In a quick test device_wakeup_path() returned inconsistent values for the
> > > > root hub, sometimes true, others false when a wakeup capable USB device was
> > > > connected.
> > >
> > > We will also test the same to double confirm the behavior of
> > > device_wakeup_path(). I am assuming that you checked device_wakeup_path()
> > > only during system suspend path.
> > >
> > > Here is what I understood by looking at __device_suspend(). Please share
> > > your thoughts on this.
> > >
> > > power.wakeup_path is set to true for the parent *after* a wakeup capable
> > > device is suspended. This means when the root hub(s) is suspended, it is
> > > propagated to xhci-plat and when xhci-plat is suspended, it is propagated
> > > to dwc3. bottom up propgation during system suspend.
> > >
> > > I believe we can directly check something like this in the dwc3 driver
> > > instead of having another wrapper like device_children_wakeup_capable().
> > >
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 1170b80..a783257 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > > break;
> > > case DWC3_GCTL_PRTCAP_HOST:
> > > if (!PMSG_IS_AUTO(msg)) {
> > > + /*
> > > + * Don't kill the host when dwc3 is wakeup capable and
> > > + * its children needs wakeup.
> > > + */
> > > + if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev))
> > > + handle_it();
> > > + } else {
> > > dwc3_core_exit(dwc);
> > > - break;
> > > }
> > >
> > > /* Let controller to suspend HSPHY before PHY driver suspends */
> > >
> >
> > device_wakeup_path(dwc->dev) is returning true all the time irrespective of
> > the wakeup capability (and enabled status) of the connected USB devices. That
> > is because xhci-plat device is configured to wakeup all the time. Since the
> > child is wakeup capable, its parent i.e dwc3 has device_wakeup_path() set.
> > device_children_wakeup_capable() will also suffer the problem. However,
> >
> > device_children_wakeup_capable(&hcd->self.root_hub->dev) is what Sandeep's
> > patch is using. That is not correct. we have two root hubs (HS and SS) associated
> > with a USB3 controller and calling it on one root hub is incorrect.
> > device_children_wakeup_capable() must be called on xhci-plat so that it covers
> > both HS and SS root hubs
>
> Thanks for pointing that out!
>
> > I am thinking of dynamically enabling/disabling xhci-plat wakeup capability so
> > that the wakeup path is correctly propagated to dwc3. something like below.
> > Does it make sense to you?
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 649ffd8..be0c55b 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -412,6 +412,9 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
> > struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > int ret;
> >
> > + if (!device_wakeup_path(dev))
> > + device_wakeup_disable(dev);
> > +
> > if (pm_runtime_suspended(dev))
> > pm_runtime_resume(dev);
> >
> > @@ -443,6 +446,8 @@ static int __maybe_unused xhci_plat_resume(struct device *dev)
> > pm_runtime_set_active(dev);
> > pm_runtime_enable(dev);
> >
> > + device_wakeup_enable(dev);
>
> I think this also needs to be done conditionally, otherwise it would
> create a new wake source on every resume when wakeup is already
> enabled.
>
Right, this needs to be done conditionally. However, there is a silent
warning inside device_wakeup_enable() if it is called during system
transition. Not sure if we really need to worry about that or not.

Thanks,
Pavan

2022-05-03 01:34:49

[permalink] [raw]

Subject: Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

Hi Pavan,

On Sat, Apr 30, 2022 at 08:41:30AM +0530, Pavan Kondeti wrote:
> Hi Matthias,
>
> On Fri, Apr 29, 2022 at 12:19:22PM -0700, Matthias Kaehlcke wrote:
> > Hi Pavan,
> >
> > On Fri, Apr 29, 2022 at 06:29:56PM +0530, Pavan Kondeti wrote:
> > > Hi Matthias,
> > >
> > > On Mon, Apr 25, 2022 at 06:33:03PM +0530, Pavan Kondeti wrote:
> > > > Hi Matthias,
> > > >
> > > > On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote:
> > > > > On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > From: Matthias Kaehlcke <[email protected]>
> > > > > > >
> > > > > > > Add device_children_wakeup_capable() which checks whether the device itself
> > > > > > > or one if its descendants is wakeup capable.
> > > > > >
> > > > > > device_wakeup_path() exists for a very similar purpose.
> > > > > >
> > > > > > Is it not usable for whatever you need the new function introduced here?
> > > > >
> > > > > I wasn't aware of it's function, there are no doc comments and the
> > > > > name isn't really self explanatory.
> > > > >
> > > > > In a quick test device_wakeup_path() returned inconsistent values for the
> > > > > root hub, sometimes true, others false when a wakeup capable USB device was
> > > > > connected.
> > > >
> > > > We will also test the same to double confirm the behavior of
> > > > device_wakeup_path(). I am assuming that you checked device_wakeup_path()
> > > > only during system suspend path.
> > > >
> > > > Here is what I understood by looking at __device_suspend(). Please share
> > > > your thoughts on this.
> > > >
> > > > power.wakeup_path is set to true for the parent *after* a wakeup capable
> > > > device is suspended. This means when the root hub(s) is suspended, it is
> > > > propagated to xhci-plat and when xhci-plat is suspended, it is propagated
> > > > to dwc3. bottom up propgation during system suspend.
> > > >
> > > > I believe we can directly check something like this in the dwc3 driver
> > > > instead of having another wrapper like device_children_wakeup_capable().
> > > >
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 1170b80..a783257 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > > > break;
> > > > case DWC3_GCTL_PRTCAP_HOST:
> > > > if (!PMSG_IS_AUTO(msg)) {
> > > > + /*
> > > > + * Don't kill the host when dwc3 is wakeup capable and
> > > > + * its children needs wakeup.
> > > > + */
> > > > + if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev))
> > > > + handle_it();
> > > > + } else {
> > > > dwc3_core_exit(dwc);
> > > > - break;
> > > > }
> > > >
> > > > /* Let controller to suspend HSPHY before PHY driver suspends */
> > > >
> > >
> > > device_wakeup_path(dwc->dev) is returning true all the time irrespective of
> > > the wakeup capability (and enabled status) of the connected USB devices. That
> > > is because xhci-plat device is configured to wakeup all the time. Since the
> > > child is wakeup capable, its parent i.e dwc3 has device_wakeup_path() set.
> > > device_children_wakeup_capable() will also suffer the problem. However,
> > >
> > > device_children_wakeup_capable(&hcd->self.root_hub->dev) is what Sandeep's
> > > patch is using. That is not correct. we have two root hubs (HS and SS) associated
> > > with a USB3 controller and calling it on one root hub is incorrect.
> > > device_children_wakeup_capable() must be called on xhci-plat so that it covers
> > > both HS and SS root hubs
> >
> > Thanks for pointing that out!
> >
> > > I am thinking of dynamically enabling/disabling xhci-plat wakeup capability so
> > > that the wakeup path is correctly propagated to dwc3. something like below.
> > > Does it make sense to you?
> > >
> > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > index 649ffd8..be0c55b 100644
> > > --- a/drivers/usb/host/xhci-plat.c
> > > +++ b/drivers/usb/host/xhci-plat.c
> > > @@ -412,6 +412,9 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
> > > struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > > int ret;
> > >
> > > + if (!device_wakeup_path(dev))
> > > + device_wakeup_disable(dev);
> > > +
> > > if (pm_runtime_suspended(dev))
> > > pm_runtime_resume(dev);
> > >
> > > @@ -443,6 +446,8 @@ static int __maybe_unused xhci_plat_resume(struct device *dev)
> > > pm_runtime_set_active(dev);
> > > pm_runtime_enable(dev);
> > >
> > > + device_wakeup_enable(dev);
> >
> > I think this also needs to be done conditionally, otherwise it would
> > create a new wake source on every resume when wakeup is already
> > enabled.
> >
> Right, this needs to be done conditionally. However, there is a silent
> warning inside device_wakeup_enable() if it is called during system
> transition. Not sure if we really need to worry about that or not.

I guess it's up to the maintainers. Removing and adding the wakeup source on
suspend/resume is a bit of a hack, but it might be acceptable if it addresses
the issue and doesn't have negative side effects.

2022-05-05 00:16:26

[permalink] [raw]

Subject: Re: [PATCH v14 3/7] usb: dwc3: core: Host wake up support from system suspend

On Wed, Apr 20, 2022 at 12:41:06AM +0530, Sandeep Maheswaram wrote:
> During suspend read the status of all port and set hs phy mode
> based on current speed. Use this hs phy mode to configure wakeup
> interrupts in qcom glue driver.
>
> Check wakeup-source property for dwc3 core node to set the
> wakeup capability. Drop the device_init_wakeup call from
> runtime suspend and resume.
>
> Also check during suspend if any wakeup capable devices are
> connected to the controller (directly or through hubs), if there
> are none set a flag to indicate that the PHY is powered
> down during suspend.
>
> Signed-off-by: Sandeep Maheswaram <[email protected]>
> ---
> v14:
> Used device_children_wakeup_capable instead of usb_wakeup_enabled_descendants.
>
> v13:
> Changed dwc3_set_phy_speed_mode to dwc3_check_phy_speed_mode.
> Removed device_init_wakeup calls from dwc3_runtime_suspend and dwc3_runtime_resume
> as we have a new dt property wakeup-source.
>
>
> drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++-------------
> drivers/usb/dwc3/core.h | 4 ++++
> drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++
> 3 files changed, 48 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 1170b80..898aa66 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -32,6 +32,7 @@
> #include <linux/usb/gadget.h>
> #include <linux/usb/of.h>
> #include <linux/usb/otg.h>
> +#include <linux/usb/hcd.h>
>
> #include "core.h"
> #include "gadget.h"
> @@ -1723,6 +1724,7 @@ static int dwc3_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, dwc);
> dwc3_cache_hwparams(dwc);
> + device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
>
> spin_lock_init(&dwc->lock);
> mutex_init(&dwc->mutex);
> @@ -1865,6 +1867,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> {
> unsigned long flags;
> u32 reg;
> + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
>
> switch (dwc->current_dr_role) {
> case DWC3_GCTL_PRTCAP_DEVICE:
> @@ -1877,10 +1880,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> dwc3_core_exit(dwc);
> break;
> case DWC3_GCTL_PRTCAP_HOST:
> - if (!PMSG_IS_AUTO(msg)) {
> - dwc3_core_exit(dwc);
> - break;
> - }
> + dwc3_check_phy_speed_mode(dwc);
>
> /* Let controller to suspend HSPHY before PHY driver suspends */
> if (dwc->dis_u2_susphy_quirk ||
> @@ -1896,6 +1896,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>
> phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
> phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
> +
> + if (!PMSG_IS_AUTO(msg)) {
> + if (device_may_wakeup(dwc->dev) &&
> + device_children_wakeup_capable(&hcd->self.root_hub->dev)) {
> + dwc->phy_power_off = false;
> + } else {
> + dwc->phy_power_off = true;
> + dwc3_core_exit(dwc);

I found that shutting the PHYs down during suspend leads to high power
consumption of a downstream hub (about 80mW vs 15mW when the PHYs are
not shut down).

It would be interesting to know if this also impacts other non-hub
peripherals. Unfortunately I can't test that, the hub on my system is
soldered to the board.

I understand that shutting the PHYs down might be beneficial in terms
of power on some systems, however on those I'm looking at we'd strongly
prefer to save the 65mW of power consumed by the hub, rather than
whatever smaller amount of power that is saved by powering down the
PHYs.

Could we introduce a sysfs attribute (or some other sort of knob) to
allow the admin to configure whether the PHYs should remain on or off
during suspend? That is assuming that it is actually desirable to power
them off on some systems.

2022-05-05 23:59:24

[permalink] [raw]

Subject: Re: [PATCH v14 3/7] usb: dwc3: core: Host wake up support from system suspend

On Thu, May 05, 2022 at 08:56:18AM +0530, Pavan Kondeti wrote:
> On Wed, May 04, 2022 at 10:46:30AM -0700, Matthias Kaehlcke wrote:
> > On Wed, Apr 20, 2022 at 12:41:06AM +0530, Sandeep Maheswaram wrote:
> > > During suspend read the status of all port and set hs phy mode
> > > based on current speed. Use this hs phy mode to configure wakeup
> > > interrupts in qcom glue driver.
> > >
> > > Check wakeup-source property for dwc3 core node to set the
> > > wakeup capability. Drop the device_init_wakeup call from
> > > runtime suspend and resume.
> > >
> > > Also check during suspend if any wakeup capable devices are
> > > connected to the controller (directly or through hubs), if there
> > > are none set a flag to indicate that the PHY is powered
> > > down during suspend.
> > >
> > > Signed-off-by: Sandeep Maheswaram <[email protected]>
> > > ---
> > > v14:
> > > Used device_children_wakeup_capable instead of usb_wakeup_enabled_descendants.
> > >
> > > v13:
> > > Changed dwc3_set_phy_speed_mode to dwc3_check_phy_speed_mode.
> > > Removed device_init_wakeup calls from dwc3_runtime_suspend and dwc3_runtime_resume
> > > as we have a new dt property wakeup-source.
> > >
> > >
> > > drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++-------------
> > > drivers/usb/dwc3/core.h | 4 ++++
> > > drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++
> > > 3 files changed, 48 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 1170b80..898aa66 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -32,6 +32,7 @@
> > > #include <linux/usb/gadget.h>
> > > #include <linux/usb/of.h>
> > > #include <linux/usb/otg.h>
> > > +#include <linux/usb/hcd.h>
> > >
> > > #include "core.h"
> > > #include "gadget.h"
> > > @@ -1723,6 +1724,7 @@ static int dwc3_probe(struct platform_device *pdev)
> > >
> > > platform_set_drvdata(pdev, dwc);
> > > dwc3_cache_hwparams(dwc);
> > > + device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
> > >
> > > spin_lock_init(&dwc->lock);
> > > mutex_init(&dwc->mutex);
> > > @@ -1865,6 +1867,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > > {
> > > unsigned long flags;
> > > u32 reg;
> > > + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
> > >
> > > switch (dwc->current_dr_role) {
> > > case DWC3_GCTL_PRTCAP_DEVICE:
> > > @@ -1877,10 +1880,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > > dwc3_core_exit(dwc);
> > > break;
> > > case DWC3_GCTL_PRTCAP_HOST:
> > > - if (!PMSG_IS_AUTO(msg)) {
> > > - dwc3_core_exit(dwc);
> > > - break;
> > > - }
> > > + dwc3_check_phy_speed_mode(dwc);
> > >
> > > /* Let controller to suspend HSPHY before PHY driver suspends */
> > > if (dwc->dis_u2_susphy_quirk ||
> > > @@ -1896,6 +1896,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > >
> > > phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
> > > phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
> > > +
> > > + if (!PMSG_IS_AUTO(msg)) {
> > > + if (device_may_wakeup(dwc->dev) &&
> > > + device_children_wakeup_capable(&hcd->self.root_hub->dev)) {
> > > + dwc->phy_power_off = false;
> > > + } else {
> > > + dwc->phy_power_off = true;
> > > + dwc3_core_exit(dwc);
> >
> > I found that shutting the PHYs down during suspend leads to high power
> > consumption of a downstream hub (about 80mW vs 15mW when the PHYs are
> > not shut down).
> >
> > It would be interesting to know if this also impacts other non-hub
> > peripherals. Unfortunately I can't test that, the hub on my system is
> > soldered to the board.
> >
> > I understand that shutting the PHYs down might be beneficial in terms
> > of power on some systems, however on those I'm looking at we'd strongly
> > prefer to save the 65mW of power consumed by the hub, rather than
> > whatever smaller amount of power that is saved by powering down the
> > PHYs.
> >
> > Could we introduce a sysfs attribute (or some other sort of knob) to
> > allow the admin to configure whether the PHYs should remain on or off
> > during suspend? That is assuming that it is actually desirable to power
> > them off on some systems.
>
> The result may vary across SoCs also. The current proposal is to keep PHY
> powered during system suspend if any of the downstream USB devices are enabled
> for wakeup. This also includes USB2/USB3 root hub. If one wants to keep PHY
> always powered on even when no device is attached, they can do so by enabling
> wakeup (echo enabled > /sys/bus/usb/devices/usbX/power/wakeup). This is anyway
> needed if you want to detect a peripheral attach during system suspend.

My concern is that it is not evident for an admin what causes the high power
consumption of the USB client (if they detect/localize it in the first place),
and even less that wakeup needs to be enabled to mitigate it.

Why can't we just put the PHYs in suspend, rather than taking the controller
down completely during suspend?

2022-05-06 12:33:13

[permalink] [raw]

Subject: Re: [PATCH v14 3/7] usb: dwc3: core: Host wake up support from system suspend

On Thu, May 05, 2022 at 09:45:49AM -0700, Matthias Kaehlcke wrote:
> On Thu, May 05, 2022 at 08:56:18AM +0530, Pavan Kondeti wrote:
> > On Wed, May 04, 2022 at 10:46:30AM -0700, Matthias Kaehlcke wrote:
> > > On Wed, Apr 20, 2022 at 12:41:06AM +0530, Sandeep Maheswaram wrote:
> > > > During suspend read the status of all port and set hs phy mode
> > > > based on current speed. Use this hs phy mode to configure wakeup
> > > > interrupts in qcom glue driver.
> > > >
> > > > Check wakeup-source property for dwc3 core node to set the
> > > > wakeup capability. Drop the device_init_wakeup call from
> > > > runtime suspend and resume.
> > > >
> > > > Also check during suspend if any wakeup capable devices are
> > > > connected to the controller (directly or through hubs), if there
> > > > are none set a flag to indicate that the PHY is powered
> > > > down during suspend.
> > > >
> > > > Signed-off-by: Sandeep Maheswaram <[email protected]>
> > > > ---
> > > > v14:
> > > > Used device_children_wakeup_capable instead of usb_wakeup_enabled_descendants.
> > > >
> > > > v13:
> > > > Changed dwc3_set_phy_speed_mode to dwc3_check_phy_speed_mode.
> > > > Removed device_init_wakeup calls from dwc3_runtime_suspend and dwc3_runtime_resume
> > > > as we have a new dt property wakeup-source.
> > > >
> > > >
> > > > drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++-------------
> > > > drivers/usb/dwc3/core.h | 4 ++++
> > > > drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++
> > > > 3 files changed, 48 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 1170b80..898aa66 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -32,6 +32,7 @@
> > > > #include <linux/usb/gadget.h>
> > > > #include <linux/usb/of.h>
> > > > #include <linux/usb/otg.h>
> > > > +#include <linux/usb/hcd.h>
> > > >
> > > > #include "core.h"
> > > > #include "gadget.h"
> > > > @@ -1723,6 +1724,7 @@ static int dwc3_probe(struct platform_device *pdev)
> > > >
> > > > platform_set_drvdata(pdev, dwc);
> > > > dwc3_cache_hwparams(dwc);
> > > > + device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
> > > >
> > > > spin_lock_init(&dwc->lock);
> > > > mutex_init(&dwc->mutex);
> > > > @@ -1865,6 +1867,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > > > {
> > > > unsigned long flags;
> > > > u32 reg;
> > > > + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
> > > >
> > > > switch (dwc->current_dr_role) {
> > > > case DWC3_GCTL_PRTCAP_DEVICE:
> > > > @@ -1877,10 +1880,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > > > dwc3_core_exit(dwc);
> > > > break;
> > > > case DWC3_GCTL_PRTCAP_HOST:
> > > > - if (!PMSG_IS_AUTO(msg)) {
> > > > - dwc3_core_exit(dwc);
> > > > - break;
> > > > - }
> > > > + dwc3_check_phy_speed_mode(dwc);
> > > >
> > > > /* Let controller to suspend HSPHY before PHY driver suspends */
> > > > if (dwc->dis_u2_susphy_quirk ||
> > > > @@ -1896,6 +1896,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > > >
> > > > phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
> > > > phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
> > > > +
> > > > + if (!PMSG_IS_AUTO(msg)) {
> > > > + if (device_may_wakeup(dwc->dev) &&
> > > > + device_children_wakeup_capable(&hcd->self.root_hub->dev)) {
> > > > + dwc->phy_power_off = false;
> > > > + } else {
> > > > + dwc->phy_power_off = true;
> > > > + dwc3_core_exit(dwc);
> > >
> > > I found that shutting the PHYs down during suspend leads to high power
> > > consumption of a downstream hub (about 80mW vs 15mW when the PHYs are
> > > not shut down).
> > >
> > > It would be interesting to know if this also impacts other non-hub
> > > peripherals. Unfortunately I can't test that, the hub on my system is
> > > soldered to the board.
> > >
> > > I understand that shutting the PHYs down might be beneficial in terms
> > > of power on some systems, however on those I'm looking at we'd strongly
> > > prefer to save the 65mW of power consumed by the hub, rather than
> > > whatever smaller amount of power that is saved by powering down the
> > > PHYs.
> > >
> > > Could we introduce a sysfs attribute (or some other sort of knob) to
> > > allow the admin to configure whether the PHYs should remain on or off
> > > during suspend? That is assuming that it is actually desirable to power
> > > them off on some systems.
> >
> > The result may vary across SoCs also. The current proposal is to keep PHY
> > powered during system suspend if any of the downstream USB devices are enabled
> > for wakeup. This also includes USB2/USB3 root hub. If one wants to keep PHY
> > always powered on even when no device is attached, they can do so by enabling
> > wakeup (echo enabled > /sys/bus/usb/devices/usbX/power/wakeup). This is anyway
> > needed if you want to detect a peripheral attach during system suspend.
>
> My concern is that it is not evident for an admin what causes the high power
> consumption of the USB client (if they detect/localize it in the first place),
> and even less that wakeup needs to be enabled to mitigate it.
>
> Why can't we just put the PHYs in suspend, rather than taking the controller
> down completely during suspend?

Agreed and I also have the same question.

I don't know the background on why DWC3 chooses to power down the PHY(s)
during system suspend. Probably it is beneficial in some board designs.
Atleast this patch series provides a way to wakeup the USB from system
suspend, which also can be used not to power down the PHY(s). If all the users
of DWC3 agree that powering down the PHY is bad, then we can do something
about it.

Thanks,
Pavan

2022-05-09 02:32:17

[permalink] [raw]

Subject: Re: [PATCH v14 3/7] usb: dwc3: core: Host wake up support from system suspend

On Wed, May 04, 2022 at 10:46:30AM -0700, Matthias Kaehlcke wrote:
> On Wed, Apr 20, 2022 at 12:41:06AM +0530, Sandeep Maheswaram wrote:
> > During suspend read the status of all port and set hs phy mode
> > based on current speed. Use this hs phy mode to configure wakeup
> > interrupts in qcom glue driver.
> >
> > Check wakeup-source property for dwc3 core node to set the
> > wakeup capability. Drop the device_init_wakeup call from
> > runtime suspend and resume.
> >
> > Also check during suspend if any wakeup capable devices are
> > connected to the controller (directly or through hubs), if there
> > are none set a flag to indicate that the PHY is powered
> > down during suspend.
> >
> > Signed-off-by: Sandeep Maheswaram <[email protected]>
> > ---
> > v14:
> > Used device_children_wakeup_capable instead of usb_wakeup_enabled_descendants.
> >
> > v13:
> > Changed dwc3_set_phy_speed_mode to dwc3_check_phy_speed_mode.
> > Removed device_init_wakeup calls from dwc3_runtime_suspend and dwc3_runtime_resume
> > as we have a new dt property wakeup-source.
> >
> >
> > drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++-------------
> > drivers/usb/dwc3/core.h | 4 ++++
> > drivers/usb/dwc3/host.c | 24 ++++++++++++++++++++++++
> > 3 files changed, 48 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 1170b80..898aa66 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -32,6 +32,7 @@
> > #include <linux/usb/gadget.h>
> > #include <linux/usb/of.h>
> > #include <linux/usb/otg.h>
> > +#include <linux/usb/hcd.h>
> >
> > #include "core.h"
> > #include "gadget.h"
> > @@ -1723,6 +1724,7 @@ static int dwc3_probe(struct platform_device *pdev)
> >
> > platform_set_drvdata(pdev, dwc);
> > dwc3_cache_hwparams(dwc);
> > + device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
> >
> > spin_lock_init(&dwc->lock);
> > mutex_init(&dwc->mutex);
> > @@ -1865,6 +1867,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > {
> > unsigned long flags;
> > u32 reg;
> > + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
> >
> > switch (dwc->current_dr_role) {
> > case DWC3_GCTL_PRTCAP_DEVICE:
> > @@ -1877,10 +1880,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > dwc3_core_exit(dwc);
> > break;
> > case DWC3_GCTL_PRTCAP_HOST:
> > - if (!PMSG_IS_AUTO(msg)) {
> > - dwc3_core_exit(dwc);
> > - break;
> > - }
> > + dwc3_check_phy_speed_mode(dwc);
> >
> > /* Let controller to suspend HSPHY before PHY driver suspends */
> > if (dwc->dis_u2_susphy_quirk ||
> > @@ -1896,6 +1896,16 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> >
> > phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
> > phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
> > +
> > + if (!PMSG_IS_AUTO(msg)) {
> > + if (device_may_wakeup(dwc->dev) &&
> > + device_children_wakeup_capable(&hcd->self.root_hub->dev)) {
> > + dwc->phy_power_off = false;
> > + } else {
> > + dwc->phy_power_off = true;
> > + dwc3_core_exit(dwc);
>
> I found that shutting the PHYs down during suspend leads to high power
> consumption of a downstream hub (about 80mW vs 15mW when the PHYs are
> not shut down).
>
> It would be interesting to know if this also impacts other non-hub
> peripherals. Unfortunately I can't test that, the hub on my system is
> soldered to the board.
>
> I understand that shutting the PHYs down might be beneficial in terms
> of power on some systems, however on those I'm looking at we'd strongly
> prefer to save the 65mW of power consumed by the hub, rather than
> whatever smaller amount of power that is saved by powering down the
> PHYs.
>
> Could we introduce a sysfs attribute (or some other sort of knob) to
> allow the admin to configure whether the PHYs should remain on or off
> during suspend? That is assuming that it is actually desirable to power
> them off on some systems.

The result may vary across SoCs also. The current proposal is to keep PHY
powered during system suspend if any of the downstream USB devices are enabled
for wakeup. This also includes USB2/USB3 root hub. If one wants to keep PHY
always powered on even when no device is attached, they can do so by enabling
wakeup (echo enabled > /sys/bus/usb/devices/usbX/power/wakeup). This is anyway
needed if you want to detect a peripheral attach during system suspend.

Thanks,
Pavan

2022-05-09 06:55:08