[PATCH v8 0/6] USB DWC3 host wake up support from system suspend

[PATCH v8 0/6] USB DWC3 host wake up support from system suspend

[Sandeep Maheswaram]

Avoiding phy powerdown in host mode when wakeup capable devices are 
connected, so that it can be wake up by devices.
Set GENPD_FLAG_ACTIVE_WAKEUP flag to keep usb30_prim gdsc active
when wakeup capable devices are connected to the host.

Changes in v8:
Moved the dwc3 suspend quirk code in dwc3/host.c to xhci-plat.c
Checking phy_power_off flag instead of usb_wakeup_enabled_descendants 
to keep gdsc active.

Changes in v7:
Change in commit text and message in PATCH 1/5 and PATCH 5/5
as per Matthias suggestion.
Added curly braces for if and else if sections in PATCH 4/5.

Changes in v6:
Addressed comments in host.c and core.c
Separated the patches in dwc3-qcom.c to make it simple.
Dropped wakeup-source change as it is not related to this series.

Changes in v5:
Added phy_power_off flag to check presence of wakeup capable devices.
Dropped patch[v4,4/5] as it is present linux-next.
Addressed comments in host.c and dwc3-qcom.c.

Changes in v4:
Addressed Matthias comments raised in v3.

Changes in v3:
Removed need_phy_for_wakeup flag and by default avoiding phy powerdown.
Addressed Matthias comments and added entry for DEV_SUPERSPEED.
Added suspend_quirk in dwc3 host and moved the dwc3_set_phy_speed_flags.
Added wakeup-source dt entry and reading in dwc-qcom.c glue driver.

Changes in v2:
Dropped the patch in clock to set GENPD_FLAG_ACTIVE_WAKEUP flag and 
setting in usb dwc3 driver.
Separated the core patch and glue driver patch.
Made need_phy_for_wakeup flag part of dwc structure and 
hs_phy_flags as unsgined int.
Adrressed the comment on device_init_wakeup call.
Corrected offset for reading portsc register.
Added pacth to support wakeup in xo shutdown case.

Sandeep Maheswaram (6):
  usb: dwc3: core: Add HS phy mode variable and phy poweroff flag
  usb: host: xhci: plat: Add suspend quirk for dwc3 controller
  usb: dwc3: core: Host wake up support from system suspend
  usb: dwc3: qcom: Add helper functions to enable,disable wake irqs
  usb: dwc3: qcom: Configure wakeup interrupts during suspend
  usb: dwc3: qcom: Keep power domain on to support wakeup

 drivers/usb/dwc3/core.c      |  7 ++--
 drivers/usb/dwc3/core.h      |  3 ++
 drivers/usb/dwc3/dwc3-qcom.c | 81 ++++++++++++++++++++++++++++----------------
 drivers/usb/host/xhci-plat.c | 38 +++++++++++++++++++++
 4 files changed, 97 insertions(+), 32 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v8 1/6] usb: dwc3: core: Add HS phy mode variable and phy poweroff flag

[Sandeep Maheswaram]

Add variables in dwc3 structure to check HS phy mode which is
used to configure interrupts and phy poweroff flag to check if
we can powerdown the phy during system suspend.

Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
---
 drivers/usb/dwc3/core.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index dccdf13..ce274d8 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1123,6 +1123,9 @@ struct dwc3 {
 
 	bool			phys_ready;
 
+	unsigned int            hs_phy_mode;
+	bool			phy_power_off;
+
 	struct ulpi		*ulpi;
 	bool			ulpi_ready;
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v8 2/6] usb: host: xhci: plat: Add suspend quirk for dwc3 controller

[Sandeep Maheswaram]

During suspend read the status of all port and make sure the PHYs
are in the correct mode based on current speed.
Phy interrupt masks are set based on this mode. Keep track of the mode
of the HS PHY to be able to configure wakeup properly.

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 should be powered
down during suspend.

Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
---
 drivers/usb/host/xhci-plat.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index c1edcc9..ee87923 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -24,6 +24,7 @@
 #include "xhci-plat.h"
 #include "xhci-mvebu.h"
 #include "xhci-rcar.h"
+#include "../dwc3/core.h"
 
 static struct hc_driver __read_mostly xhci_plat_hc_driver;
 
@@ -430,6 +431,39 @@ static int xhci_plat_remove(struct platform_device *dev)
 
 	return 0;
 }
+static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd)
+{
+	int i, num_ports;
+	u32 reg;
+	unsigned int ss_phy_mode = 0;
+	struct dwc3 *dwc = dev_get_drvdata(hcd->self.controller->parent);
+	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 * 0x04);
+		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;
+
+			if (DEV_SUPERSPEED(reg))
+				ss_phy_mode |= PHY_MODE_USB_HOST_SS;
+		}
+	}
+	phy_set_mode(dwc->usb2_generic_phy, dwc->hs_phy_mode);
+	phy_set_mode(dwc->usb3_generic_phy, ss_phy_mode);
+
+	if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
+		dwc->phy_power_off = false;
+	else
+		dwc->phy_power_off = true;
+}
 
 static int __maybe_unused xhci_plat_suspend(struct device *dev)
 {
@@ -440,6 +474,10 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
 	ret = xhci_priv_suspend_quirk(hcd);
 	if (ret)
 		return ret;
+
+	if (of_device_is_compatible(dev->parent->of_node, "snps,dwc3"))
+		xhci_dwc3_suspend_quirk(hcd);
+
 	/*
 	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
 	 * to do wakeup during suspend.
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v8 3/6] usb: dwc3: core: Host wake up support from system suspend

[Sandeep Maheswaram]

Avoiding phy powerdown when wakeup capable devices are connected
by checking phy_power_off flag.
Phy should be on to wake up the device from suspend using wakeup capable
devices such as keyboard and mouse.

Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/usb/dwc3/core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index ba74ad7..ef96a5f 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1738,7 +1738,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)) {
+		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {
 			dwc3_core_exit(dwc);
 			break;
 		}
@@ -1799,13 +1799,16 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg)) {
+		if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) {
 			ret = dwc3_core_init_for_resume(dwc);
 			if (ret)
 				return ret;
 			dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
 			break;
+		} else {
+			dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
 		}
+
 		/* Restore GUSB2PHYCFG bits that were modified in suspend */
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
 		if (dwc->dis_u2_susphy_quirk)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v8 4/6] usb: dwc3: qcom: Add helper functions to enable,disable wake irqs

[Sandeep Maheswaram]

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

Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 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 49e6ca9..66183c6 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)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v8 5/6] usb: dwc3: qcom: Configure wakeup interrupts during suspend

[Sandeep Maheswaram]

Configure interrupts based on hs_phy_mode to avoid triggering of
interrupts during system suspend and suspend the device successfully.

Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 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 66183c6..82125bc 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);
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v8 6/6] usb: dwc3: qcom: Keep power domain on to support wakeup

[Sandeep Maheswaram]

If wakeup capable devices are connected to the controller (directly
or through hubs) at suspend time keep the power domain on in order
to support wakeup from these devices.

Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
---
Checking phy_power_off flag instead of usb_wakeup_enabled_descendants 
to keep gdsc active.

 drivers/usb/dwc3/dwc3-qcom.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 82125bc..ba31aa3 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>
@@ -355,9 +356,15 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
 	u32 val;
 	int i, ret;
 
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
+
 	if (qcom->is_suspended)
 		return 0;
 
+	if (!dwc->phy_power_off && dwc->xhci)
+		genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
+
 	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
 	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
 		dev_err(qcom->dev, "HS-PHY not in L2\n");
@@ -382,9 +389,15 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
 	int ret;
 	int i;
 
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
+
 	if (!qcom->is_suspended)
 		return 0;
 
+	if (dwc->xhci)
+		genpd->flags &= ~GENPD_FLAG_ACTIVE_WAKEUP;
+
 	if (device_may_wakeup(qcom->dev))
 		dwc3_qcom_disable_interrupts(qcom);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v8 6/6] usb: dwc3: qcom: Keep power domain on to support wakeup

[Matthias Kaehlcke]

On Mon, Jun 28, 2021 at 05:38:17PM +0530, Sandeep Maheswaram wrote:
> If wakeup capable devices are connected to the controller (directly
> or through hubs) at suspend time keep the power domain on in order
> to support wakeup from these devices.
> 
> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> ---
> Checking phy_power_off flag instead of usb_wakeup_enabled_descendants 
> to keep gdsc active.
> 
>  drivers/usb/dwc3/dwc3-qcom.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 82125bc..ba31aa3 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>
> @@ -355,9 +356,15 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
>  	u32 val;
>  	int i, ret;
>  
> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> +	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
> +
>  	if (qcom->is_suspended)
>  		return 0;
>  
> +	if (!dwc->phy_power_off && dwc->xhci)
> +		genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
> +
>  	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
>  	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>  		dev_err(qcom->dev, "HS-PHY not in L2\n");
> @@ -382,9 +389,15 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
>  	int ret;
>  	int i;
>  
> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> +	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
> +
>  	if (!qcom->is_suspended)
>  		return 0;
>  
> +	if (dwc->xhci)
> +		genpd->flags &= ~GENPD_FLAG_ACTIVE_WAKEUP;
> +
>  	if (device_may_wakeup(qcom->dev))
>  		dwc3_qcom_disable_interrupts(qcom);
>  

This is essentially the same as v7, which Felipe NAKed
(https://patchwork.kernel.org/project/linux-arm-msm/patch/1619586716-8687-6-git-send-email-sanm@codeaurora.org/)

I think Felipe wants to see the handling of the power domain in the
xhci-plat driver. One problem here is that the power domain is owned
by the glue driver. For dwc3 the glue device is the parent of the xHCI
device, this is also the case for some other drivers like histb or
cdns3, but I'm not sure if it is universally true. If it isn't
xhci-plat could only make use of dev->parent->pm_domain for certain
compatible strings.

One could argue that it isn't very clean either if xhci-plat manipulates
a resource of it's parent. At the same time the glue driver isn't
supposed to check for the wakeup capable devices, so I guess some kind
of trade-off needs to be made.

Re: [PATCH v8 2/6] usb: host: xhci: plat: Add suspend quirk for dwc3 controller

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]


Hi,

Sandeep Maheswaram <sanm@codeaurora.org> writes:
> During suspend read the status of all port and make sure the PHYs
> are in the correct mode based on current speed.
> Phy interrupt masks are set based on this mode. Keep track of the mode
> of the HS PHY to be able to configure wakeup properly.
>
> 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 should be powered
> down during suspend.
>
> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> ---
>  drivers/usb/host/xhci-plat.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index c1edcc9..ee87923 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -24,6 +24,7 @@
>  #include "xhci-plat.h"
>  #include "xhci-mvebu.h"
>  #include "xhci-rcar.h"
> +#include "../dwc3/core.h"
>  
>  static struct hc_driver __read_mostly xhci_plat_hc_driver;
>  
> @@ -430,6 +431,39 @@ static int xhci_plat_remove(struct platform_device *dev)
>  
>  	return 0;
>  }
> +static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd)
> +{
> +	int i, num_ports;
> +	u32 reg;
> +	unsigned int ss_phy_mode = 0;
> +	struct dwc3 *dwc = dev_get_drvdata(hcd->self.controller->parent);
> +	struct xhci_hcd	*xhci_hcd = hcd_to_xhci(hcd);
> +
> +	dwc->hs_phy_mode = 0;

you're still bypassing the driver layering. First you had dwc access
xhci, now you want xhci to access dwc. Either way is wrong. You need to
rely on drivers core and device properties for this stuff. Don't access
data you don't own.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

Re: [PATCH v8 6/6] usb: dwc3: qcom: Keep power domain on to support wakeup

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3856 bytes --]


Hi,

Matthias Kaehlcke <mka@chromium.org> writes:
> On Mon, Jun 28, 2021 at 05:38:17PM +0530, Sandeep Maheswaram wrote:
>> If wakeup capable devices are connected to the controller (directly
>> or through hubs) at suspend time keep the power domain on in order
>> to support wakeup from these devices.
>> 
>> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
>> ---
>> Checking phy_power_off flag instead of usb_wakeup_enabled_descendants 
>> to keep gdsc active.
>> 
>>  drivers/usb/dwc3/dwc3-qcom.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>> 
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index 82125bc..ba31aa3 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>
>> @@ -355,9 +356,15 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
>>  	u32 val;
>>  	int i, ret;
>>  
>> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>> +	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
>> +
>>  	if (qcom->is_suspended)
>>  		return 0;
>>  
>> +	if (!dwc->phy_power_off && dwc->xhci)
>> +		genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
>> +
>>  	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
>>  	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>  		dev_err(qcom->dev, "HS-PHY not in L2\n");
>> @@ -382,9 +389,15 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
>>  	int ret;
>>  	int i;
>>  
>> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>> +	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
>> +
>>  	if (!qcom->is_suspended)
>>  		return 0;
>>  
>> +	if (dwc->xhci)
>> +		genpd->flags &= ~GENPD_FLAG_ACTIVE_WAKEUP;
>> +
>>  	if (device_may_wakeup(qcom->dev))
>>  		dwc3_qcom_disable_interrupts(qcom);
>>  
>
> This is essentially the same as v7, which Felipe NAKed
> (https://patchwork.kernel.org/project/linux-arm-msm/patch/1619586716-8687-6-git-send-email-sanm@codeaurora.org/)
>
> I think Felipe wants to see the handling of the power domain in the
> xhci-plat driver. One problem here is that the power domain is owned

this is not exactly what I meant to say, though. I want drivers to be
self-contained. I.e. dwc3 doesn't modify xhci data and vice-versa. There
are a few assummpmtions that we can make, though. The structure is
usually like this:

glue {
  dwc3 {
    xhci
  };
};

This means that in order for glue_suspend() to run, dwc3 has to suspend
first and xhci has to suspend before dwc3.

For example, in the suspend call above, qcom (the glue) is directly
accessing dwc3 core data, which is incorrect. It looks like we want to
know if the PHY is not powered off and if it isn't, then we want to
change the power domain ACTIVE_WAKEUP flag. Now, phy_power_off is false
whenever any of xHCI's children enable USB wakeup.

It seems like we need to way to generically propagate that knowledge up
the parent tree. I.e., a parent needs to know if its child is wakeup
capable, then dwc3 could, in its suspend routine:

static int dwc3_suspend(struct device *dev)
{
	/* ... */

	if (device_children_wakeup_capable(dev))
        	device_enable_wakeup(dev);

	/* ... */
}

and similarly for qcom glue:

static int dwc3_qcom_suspend(struct device *dev)
{
	/* ... */

	if (device_children_wakeup_capable(dev)) {
        	device_enable_wakeup(dev);
		genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
        }

	/* ... */
}

It also seems plausible that this could be done at driver core and
completely hidden away from drivers.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

Re: [PATCH v8 6/6] usb: dwc3: qcom: Keep power domain on to support wakeup

[Sandeep Maheswaram]

Hi Felipe,

On 7/12/2021 3:12 PM, Felipe Balbi wrote:
> Hi,
>
> Matthias Kaehlcke <mka@chromium.org> writes:
>> On Mon, Jun 28, 2021 at 05:38:17PM +0530, Sandeep Maheswaram wrote:
>>> If wakeup capable devices are connected to the controller (directly
>>> or through hubs) at suspend time keep the power domain on in order
>>> to support wakeup from these devices.
>>>
>>> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
>>> ---
>>> Checking phy_power_off flag instead of usb_wakeup_enabled_descendants
>>> to keep gdsc active.
>>>
>>>   drivers/usb/dwc3/dwc3-qcom.c | 13 +++++++++++++
>>>   1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>>> index 82125bc..ba31aa3 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>
>>> @@ -355,9 +356,15 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
>>>   	u32 val;
>>>   	int i, ret;
>>>   
>>> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>>> +	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
>>> +
>>>   	if (qcom->is_suspended)
>>>   		return 0;
>>>   
>>> +	if (!dwc->phy_power_off && dwc->xhci)
>>> +		genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
>>> +
>>>   	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
>>>   	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>>   		dev_err(qcom->dev, "HS-PHY not in L2\n");
>>> @@ -382,9 +389,15 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
>>>   	int ret;
>>>   	int i;
>>>   
>>> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>>> +	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
>>> +
>>>   	if (!qcom->is_suspended)
>>>   		return 0;
>>>   
>>> +	if (dwc->xhci)
>>> +		genpd->flags &= ~GENPD_FLAG_ACTIVE_WAKEUP;
>>> +
>>>   	if (device_may_wakeup(qcom->dev))
>>>   		dwc3_qcom_disable_interrupts(qcom);
>>>   
>> This is essentially the same as v7, which Felipe NAKed
>> (https://patchwork.kernel.org/project/linux-arm-msm/patch/1619586716-8687-6-git-send-email-sanm@codeaurora.org/)
>>
>> I think Felipe wants to see the handling of the power domain in the
>> xhci-plat driver. One problem here is that the power domain is owned
> this is not exactly what I meant to say, though. I want drivers to be
> self-contained. I.e. dwc3 doesn't modify xhci data and vice-versa. There
> are a few assummpmtions that we can make, though. The structure is
> usually like this:
>
> glue {
>    dwc3 {
>      xhci
>    };
> };
>
> This means that in order for glue_suspend() to run, dwc3 has to suspend
> first and xhci has to suspend before dwc3.
>
> For example, in the suspend call above, qcom (the glue) is directly
> accessing dwc3 core data, which is incorrect. It looks like we want to
> know if the PHY is not powered off and if it isn't, then we want to
> change the power domain ACTIVE_WAKEUP flag. Now, phy_power_off is false
> whenever any of xHCI's children enable USB wakeup.
>
> It seems like we need to way to generically propagate that knowledge up
> the parent tree. I.e., a parent needs to know if its child is wakeup
> capable, then dwc3 could, in its suspend routine:
>
> static int dwc3_suspend(struct device *dev)
> {
> 	/* ... */
>
> 	if (device_children_wakeup_capable(dev))
>          	device_enable_wakeup(dev);
>
> 	/* ... */
> }

Can we use like  this device_may_wakeup(&dwc->xhci->dev) to check if 
children is wakeup capable like below ?

static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
{
	
/* ... */
	if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
			dwc3_core_exit(dwc);
			break;
	}

/* ... */

  }

> and similarly for qcom glue:
>
> static int dwc3_qcom_suspend(struct device *dev)
> {
> 	/* ... */
>
>
> 	if (device_children_wakeup_capable(dev)) {
>          	device_enable_wakeup(dev);
> 		genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
>          }
>
> 	/* ... */
> }
>
> It also seems plausible that this could be done at driver core and
> completely hidden away from drivers.

And in qcom glue like this

static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
{

/* ... */

     struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);


     if (device_may_wakeup(&dwc->xhci->dev) && dwc->xhci)
         genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;

/* ... */

}

Re: [PATCH v8 6/6] usb: dwc3: qcom: Keep power domain on to support wakeup

[Felipe Balbi]

Hi,

Sandeep Maheswaram <sanm@codeaurora.org> writes:
>> This means that in order for glue_suspend() to run, dwc3 has to suspend
>> first and xhci has to suspend before dwc3.
>>
>> For example, in the suspend call above, qcom (the glue) is directly
>> accessing dwc3 core data, which is incorrect. It looks like we want to
>> know if the PHY is not powered off and if it isn't, then we want to
>> change the power domain ACTIVE_WAKEUP flag. Now, phy_power_off is false
>> whenever any of xHCI's children enable USB wakeup.
>>
>> It seems like we need to way to generically propagate that knowledge up
>> the parent tree. I.e., a parent needs to know if its child is wakeup
>> capable, then dwc3 could, in its suspend routine:
>>
>> static int dwc3_suspend(struct device *dev)
>> {
>> 	/* ... */
>>
>> 	if (device_children_wakeup_capable(dev))
>>          	device_enable_wakeup(dev);
>>
>> 	/* ... */
>> }
>
> Can we use like  this device_may_wakeup(&dwc->xhci->dev) to check if
> children is wakeup capable like below ?

that really doesn't sound like a good idea, IMHO. We're still passing
through layers of abstraction without anyone's knowledge :-)

It looks to me like we're missing some infrastructure in the wakeup code
so parents can make decisions based on the state of their children.

>> and similarly for qcom glue:
>>
>> static int dwc3_qcom_suspend(struct device *dev)
>> {
>> 	/* ... */
>>
>>
>> 	if (device_children_wakeup_capable(dev)) {
>>          	device_enable_wakeup(dev);
>> 		genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
>>          }
>>
>> 	/* ... */
>> }
>>
>> It also seems plausible that this could be done at driver core and
>> completely hidden away from drivers.
>
> And in qcom glue like this
>
> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> {
>
> /* ... */
>
>     struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);

you see, here there's an assumption that the platform data is still
valid and not some bogus dangling pointer. There's also an assumption
that the type is struct dwc3 (which is unlikely to change, but still).

-- 
balbi

Re: [PATCH v8 6/6] usb: dwc3: qcom: Keep power domain on to support wakeup

[Pavan Kondeti]

+linux-pm and Rafael

Hi Balbi,

On Mon, Jul 12, 2021 at 12:42:17PM +0300, Felipe Balbi wrote:
> 
> >> @@ -355,9 +356,15 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> >>  	u32 val;
> >>  	int i, ret;
> >>  
> >> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> >> +	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
> >> +
> >>  	if (qcom->is_suspended)
> >>  		return 0;
> >>  
> >> +	if (!dwc->phy_power_off && dwc->xhci)
> >> +		genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
> >> +
> >>  	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
> >>  	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> >>  		dev_err(qcom->dev, "HS-PHY not in L2\n");
> >> @@ -382,9 +389,15 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
> >>  	int ret;
> >>  	int i;
> >>  
> >> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> >> +	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
> >> +
> >>  	if (!qcom->is_suspended)
> >>  		return 0;
> >>  
> >> +	if (dwc->xhci)
> >> +		genpd->flags &= ~GENPD_FLAG_ACTIVE_WAKEUP;
> >> +
> >>  	if (device_may_wakeup(qcom->dev))
> >>  		dwc3_qcom_disable_interrupts(qcom);
> >>  
> >
> > This is essentially the same as v7, which Felipe NAKed
> > (https://patchwork.kernel.org/project/linux-arm-msm/patch/1619586716-8687-6-git-send-email-sanm@codeaurora.org/)
> >
> > I think Felipe wants to see the handling of the power domain in the
> > xhci-plat driver. One problem here is that the power domain is owned
> 
> this is not exactly what I meant to say, though. I want drivers to be
> self-contained. I.e. dwc3 doesn't modify xhci data and vice-versa. There
> are a few assummpmtions that we can make, though. The structure is
> usually like this:
> 
> glue {
>   dwc3 {
>     xhci
>   };
> };
> 
> This means that in order for glue_suspend() to run, dwc3 has to suspend
> first and xhci has to suspend before dwc3.
> 
> For example, in the suspend call above, qcom (the glue) is directly
> accessing dwc3 core data, which is incorrect. It looks like we want to
> know if the PHY is not powered off and if it isn't, then we want to
> change the power domain ACTIVE_WAKEUP flag. Now, phy_power_off is false
> whenever any of xHCI's children enable USB wakeup.
> 
> It seems like we need to way to generically propagate that knowledge up
> the parent tree. I.e., a parent needs to know if its child is wakeup
> capable, then dwc3 could, in its suspend routine:
> 
> static int dwc3_suspend(struct device *dev)
> {
> 	/* ... */
> 
> 	if (device_children_wakeup_capable(dev))
>         	device_enable_wakeup(dev);
> 
> 	/* ... */
> }
> 
> and similarly for qcom glue:
> 
> static int dwc3_qcom_suspend(struct device *dev)
> {
> 	/* ... */
> 
> 	if (device_children_wakeup_capable(dev)) {
>         	device_enable_wakeup(dev);
> 		genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
>         }
> 
> 	/* ... */
> }
> 
> It also seems plausible that this could be done at driver core and
> completely hidden away from drivers.
> 

device_children_wakeup_capable() idea sounds good. Added linux-pm and Rafael
for more inputs on this.

AFAICT, device wakeup settings are NOT propagated to the parent. Ideally one
expects the parent to be wake up capable and enabled when any of its children
is enabling the wakeup. If that had been propagated, we could simply check
device_may_wakeup() and take an action. I am sure there are good reason why
and how this propgation may not work as we expect it to work for all devices.

Can we make something like below to make sure that DWC3 core respects its
child wakeup setting? This is inline with your suggestion of propagating it
through layers. we should probably enable wakeup on dwc3 dev so that the
glue drivers can take the action appropriately.


diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 172dfd2..9c43d37 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1749,7 +1749,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)) {
+               if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
                        dwc3_core_exit(dwc);
                        break;
                }
@@ -1810,7 +1810,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
                spin_unlock_irqrestore(&dwc->lock, flags);
                break;
        case DWC3_GCTL_PRTCAP_HOST:
-               if (!PMSG_IS_AUTO(msg)) {
+               if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
                        ret = dwc3_core_init_for_resume(dwc);
                        if (ret)
                                return ret;


Thanks,
Pavan
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v8 2/6] usb: host: xhci: plat: Add suspend quirk for dwc3 controller

[Brian Norris]

Hi,

On Mon, Jun 28, 2021 at 05:38:13PM +0530, Sandeep Maheswaram wrote:
> During suspend read the status of all port and make sure the PHYs
> are in the correct mode based on current speed.
> Phy interrupt masks are set based on this mode. Keep track of the mode
> of the HS PHY to be able to configure wakeup properly.
> 
> 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 should be powered
> down during suspend.

...

> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c

> @@ -430,6 +431,39 @@ static int xhci_plat_remove(struct platform_device *dev)
>  
>  	return 0;
>  }
> +static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd)

nit: you need a blank line above this (in between functions).

> +{
> +	int i, num_ports;
> +	u32 reg;
> +	unsigned int ss_phy_mode = 0;
> +	struct dwc3 *dwc = dev_get_drvdata(hcd->self.controller->parent);
> +	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 * 0x04);
> +		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;
> +
> +			if (DEV_SUPERSPEED(reg))
> +				ss_phy_mode |= PHY_MODE_USB_HOST_SS;
> +		}
> +	}
> +	phy_set_mode(dwc->usb2_generic_phy, dwc->hs_phy_mode);
> +	phy_set_mode(dwc->usb3_generic_phy, ss_phy_mode);
> +
> +	if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
> +		dwc->phy_power_off = false;
> +	else
> +		dwc->phy_power_off = true;
> +}

This series breaks USB across S3 suspend/resume on Rockchip RK3399 Gru
platforms. Those platforms do not support USB wake (they power off many
of the relevant IP blocks in S3), and they *require* reconfiguring the
PHY on resume, but usb_wakeup_enabled_descendants() is returning
non-zero. If I revert patch 3, things work again.

Perhaps that's a Rockchip bug (should such platforms be disabling all PM
wakeup capabilities for their child hubs/devices?), but I'd much
appreciate resolving that before merging such a regression.

This also may be a sign that usb_wakeup_enabled_descendants() isn't
really the precise condition you should be using, if other platforms
aren't accurately reflecting feature support status in here.

Brian

P.S. In case it matters, I'm noticing this because earlier versions of
your patches (which have the same problem) are merged in our downstream
Chromium kernel tree.