[PATCH v7 0/5] USB DWC3 host wake up support from system suspend

[PATCH v7 0/5] 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 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 (5):
  usb: dwc3: host: Set PHY mode during suspend
  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 | 85 ++++++++++++++++++++++++++++----------------
 drivers/usb/dwc3/host.c      | 59 ++++++++++++++++++++++++++++++
 4 files changed, 122 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 v7 1/5] usb: dwc3: host: Set PHY mode during suspend

[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/dwc3/core.h |  3 +++
 drivers/usb/dwc3/host.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b1e875c..cecd278 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;
 
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index f29a264..527f04c 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -11,6 +11,14 @@
 #include <linux/platform_device.h>
 
 #include "core.h"
+#include "../host/xhci.h"
+#include "../host/xhci-plat.h"
+
+static int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd);
+
+static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
+	.suspend_quirk = xhci_dwc3_suspend_quirk,
+};
 
 static int dwc3_host_get_irq(struct dwc3 *dwc)
 {
@@ -115,6 +123,13 @@ int dwc3_host_init(struct dwc3 *dwc)
 		}
 	}
 
+	ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci,
+			sizeof(struct xhci_plat_priv));
+	if (ret) {
+		dev_err(dwc->dev, "failed to add data to xHCI\n");
+		goto err;
+	}
+
 	ret = platform_device_add(xhci);
 	if (ret) {
 		dev_err(dwc->dev, "failed to register xHCI device\n");
@@ -127,6 +142,50 @@ int dwc3_host_init(struct dwc3 *dwc)
 	return ret;
 }
 
+static void dwc3_set_phy_mode(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);
+}
+
+int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd)
+{
+	struct dwc3 *dwc = dev_get_drvdata(hcd->self.controller->parent);
+
+	dwc3_set_phy_mode(hcd);
+
+	if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
+		dwc->phy_power_off = false;
+	else
+		dwc->phy_power_off = true;
+
+	return 0;
+}
+
 void dwc3_host_exit(struct dwc3 *dwc)
 {
 	platform_device_unregister(dwc->xhci);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v7 1/5] usb: dwc3: host: Set PHY mode during suspend

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2790 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/dwc3/core.h |  3 +++
>  drivers/usb/dwc3/host.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index b1e875c..cecd278 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;
>  
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index f29a264..527f04c 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -11,6 +11,14 @@
>  #include <linux/platform_device.h>
>  
>  #include "core.h"
> +#include "../host/xhci.h"
> +#include "../host/xhci-plat.h"
> +
> +static int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd);
> +
> +static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
> +	.suspend_quirk = xhci_dwc3_suspend_quirk,
> +};

we're passing data using device_properties, why do you want this here?

> @@ -115,6 +123,13 @@ int dwc3_host_init(struct dwc3 *dwc)
>  		}
>  	}
>  
> +	ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci,
> +			sizeof(struct xhci_plat_priv));
> +	if (ret) {
> +		dev_err(dwc->dev, "failed to add data to xHCI\n");
> +		goto err;
> +	}
> +
>  	ret = platform_device_add(xhci);
>  	if (ret) {
>  		dev_err(dwc->dev, "failed to register xHCI device\n");
> @@ -127,6 +142,50 @@ int dwc3_host_init(struct dwc3 *dwc)
>  	return ret;
>  }
>  
> +static void dwc3_set_phy_mode(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);

there's a big assumption here that xhci is still alive. Why isn't this
quirk implemented in xhci-plat itself?

> +int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd)

who calls this?

-- 
balbi

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

Re: [PATCH v7 1/5] usb: dwc3: host: Set PHY mode during suspend

[Felipe Balbi]

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


Hi,

Sandeep Maheswaram <sanm@codeaurora.org> writes:
>>> @@ -11,6 +11,14 @@
>>>   #include <linux/platform_device.h>
>>>   
>>>   #include "core.h"
>>> +#include "../host/xhci.h"
>>> +#include "../host/xhci-plat.h"
>>> +
>>> +static int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd);
>>> +
>>> +static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
>>> +	.suspend_quirk = xhci_dwc3_suspend_quirk,
>>> +};
>> we're passing data using device_properties, why do you want this here?
> Similar implemenation was done in 
> drivers/usb/cdns3/host.c<https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/usb/cdns3/host.c?h=next-20210503> 

then it seems we have two places to correct :-)

>>> @@ -127,6 +142,50 @@ int dwc3_host_init(struct dwc3 *dwc)
>>>   	return ret;
>>>   }
>>>   
>>> +static void dwc3_set_phy_mode(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);
>> there's a big assumption here that xhci is still alive. Why isn't this
>> quirk implemented in xhci-plat itself?
>>
>>> +int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd)
>> who calls this?
> This will be called from 
> xhci_plat_suspend->xhci_priv_suspend_quirk->xhci_dwc3_suspend_quirk

So xhci.ko calls a function from dwc3.ko? That sounds odd, doesn't it?
It really looks like we're just finding ways to bypass the driver
layering, rather than working with it.

-- 
balbi

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

Re: [PATCH v7 1/5] usb: dwc3: host: Set PHY mode during suspend

[Matthias Kaehlcke]

On Wed, Apr 28, 2021 at 12:55:21PM +0300, Felipe Balbi wrote:
> 
> 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/dwc3/core.h |  3 +++
> >  drivers/usb/dwc3/host.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 62 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index b1e875c..cecd278 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;
> >  
> > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> > index f29a264..527f04c 100644
> > --- a/drivers/usb/dwc3/host.c
> > +++ b/drivers/usb/dwc3/host.c
> > @@ -11,6 +11,14 @@
> >  #include <linux/platform_device.h>
> >  
> >  #include "core.h"
> > +#include "../host/xhci.h"
> > +#include "../host/xhci-plat.h"
> > +
> > +static int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd);
> > +
> > +static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
> > +	.suspend_quirk = xhci_dwc3_suspend_quirk,
> > +};
> 
> we're passing data using device_properties, why do you want this here?
> 
> > @@ -115,6 +123,13 @@ int dwc3_host_init(struct dwc3 *dwc)
> >  		}
> >  	}
> >  
> > +	ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci,
> > +			sizeof(struct xhci_plat_priv));
> > +	if (ret) {
> > +		dev_err(dwc->dev, "failed to add data to xHCI\n");
> > +		goto err;
> > +	}
> > +
> >  	ret = platform_device_add(xhci);
> >  	if (ret) {
> >  		dev_err(dwc->dev, "failed to register xHCI device\n");
> > @@ -127,6 +142,50 @@ int dwc3_host_init(struct dwc3 *dwc)
> >  	return ret;
> >  }
> >  
> > +static void dwc3_set_phy_mode(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);
> 
> there's a big assumption here that xhci is still alive. Why isn't this
> quirk implemented in xhci-plat itself?

That should work for determining which types of devices are connected to
the PHYs, however IIUC the xhci-plat doesn't know about the PHY topology.
Are you suggesting to move that info into the xhci-plat driver so that it
can set the corresponding PHY modes?

Re: [PATCH v7 1/5] usb: dwc3: host: Set PHY mode during suspend

[Felipe Balbi]

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


Hi,

Matthias Kaehlcke <mka@chromium.org> writes:
>> > @@ -127,6 +142,50 @@ int dwc3_host_init(struct dwc3 *dwc)
>> >  	return ret;
>> >  }
>> >  
>> > +static void dwc3_set_phy_mode(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);
>> 
>> there's a big assumption here that xhci is still alive. Why isn't this
>> quirk implemented in xhci-plat itself?
>
> That should work for determining which types of devices are connected to
> the PHYs, however IIUC the xhci-plat doesn't know about the PHY topology.
> Are you suggesting to move that info into the xhci-plat driver so that it
> can set the corresponding PHY modes?

Yes, if xHCI needs to know about PHYs in order to properly configure the
PHYs, so be it :-)

-- 
balbi

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

Re: [PATCH v7 1/5] usb: dwc3: host: Set PHY mode during suspend

[Matthias Kaehlcke]

On Thu, May 13, 2021 at 04:46:41PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Matthias Kaehlcke <mka@chromium.org> writes:
> >> > @@ -127,6 +142,50 @@ int dwc3_host_init(struct dwc3 *dwc)
> >> >  	return ret;
> >> >  }
> >> >  
> >> > +static void dwc3_set_phy_mode(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);
> >> 
> >> there's a big assumption here that xhci is still alive. Why isn't this
> >> quirk implemented in xhci-plat itself?
> >
> > That should work for determining which types of devices are connected to
> > the PHYs, however IIUC the xhci-plat doesn't know about the PHY topology.
> > Are you suggesting to move that info into the xhci-plat driver so that it
> > can set the corresponding PHY modes?
> 
> Yes, if xHCI needs to know about PHYs in order to properly configure the
> PHYs, so be it :-)

Thanks for the confirmation, looks like we have a path forward here then :)

[PATCH v7 2/5] 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 b6e53d8..bb414c3 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

Re: [PATCH v7 2/5] usb: dwc3: core: Host wake up support from system suspend

[Felipe Balbi]

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


Hi,

Sandeep Maheswaram <sanm@codeaurora.org> writes:
> 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 b6e53d8..bb414c3 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) {

should be able to detect this generically, no? Shouldn't
device_may_wakeup() be valid here and give you the answer you want?

-- 
balbi

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

Re: [PATCH v7 2/5] usb: dwc3: core: Host wake up support from system suspend

[Sandeep Maheswaram]

On 4/28/2021 3:29 PM, Felipe Balbi wrote:
> Hi,
>
> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>> 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 b6e53d8..bb414c3 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) {
> should be able to detect this generically, no? Shouldn't
> device_may_wakeup() be valid here and give you the answer you want?

I think  device_may_wakeup() gives whether the controller is wake up 
capable or not.

But we want to keep phy powered on only when some wakeup capable devices 
(eg:keyboard ,mouse ) are connected to controller.

Re: [PATCH v7 2/5] usb: dwc3: core: Host wake up support from system suspend

[Felipe Balbi]

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


Hi,

Sandeep Maheswaram <sanm@codeaurora.org> writes:
>> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>>> 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 b6e53d8..bb414c3 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) {
>> should be able to detect this generically, no? Shouldn't
>> device_may_wakeup() be valid here and give you the answer you want?
>
> I think  device_may_wakeup() gives whether the controller is wake up 
> capable or not.

Yes, but it's a bit more than that. Looking at devices.rst we read:

If :c:func:`device_may_wakeup(dev)` returns ``true``, the device should be
prepared for generating hardware wakeup signals to trigger a system wakeup event
when the system is in the sleep state.  For example, :c:func:`enable_irq_wake()`
might identify GPIO signals hooked up to a switch or other external hardware,
and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal.

So, if there is a condition where $this device has to, somehow, deal
with wakeup, it should be configured accordingly. This ->phy_power_off
flag is telling us the same thing.

> But we want to keep phy powered on only when some wakeup capable devices 
> (eg:keyboard ,mouse ) are connected to controller.

Understood, it could be that we're missing some method for propagating
that state (i.e. keyboard with PM support) up to the parent device, but
that's no excuse to bypass driver boundaries. Wouldn't you agree?

-- 
balbi

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

Re: [PATCH v7 2/5] usb: dwc3: core: Host wake up support from system suspend

[Matthias Kaehlcke]

On Mon, May 03, 2021 at 02:20:23PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Sandeep Maheswaram <sanm@codeaurora.org> writes:
> >> Sandeep Maheswaram <sanm@codeaurora.org> writes:
> >>> 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 b6e53d8..bb414c3 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) {
> >> should be able to detect this generically, no? Shouldn't
> >> device_may_wakeup() be valid here and give you the answer you want?
> >
> > I think  device_may_wakeup() gives whether the controller is wake up 
> > capable or not.
> 
> Yes, but it's a bit more than that. Looking at devices.rst we read:
> 
> If :c:func:`device_may_wakeup(dev)` returns ``true``, the device should be
> prepared for generating hardware wakeup signals to trigger a system wakeup event
> when the system is in the sleep state.  For example, :c:func:`enable_irq_wake()`
> might identify GPIO signals hooked up to a switch or other external hardware,
> and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal.
> 
> So, if there is a condition where $this device has to, somehow, deal
> with wakeup, it should be configured accordingly. This ->phy_power_off
> flag is telling us the same thing.
> 
> > But we want to keep phy powered on only when some wakeup capable devices 
> > (eg:keyboard ,mouse ) are connected to controller.
> 
> Understood, it could be that we're missing some method for propagating
> that state (i.e. keyboard with PM support) up to the parent device, but
> that's no excuse to bypass driver boundaries. Wouldn't you agree?

I'm not sure if device_may_wakeup() is really the right tool for the
job. This is the current implementation:

static inline bool device_may_wakeup(struct device *dev)
{
	return dev->power.can_wakeup && !!dev->power.wakeup;
}

IIUC power.can_wakeup specifies whether the device is wakeup
capable, primarily in physical terms and indicating that the
driver is ready to handle wakeups, and power.wakeup represents
the policy which can be changed by userspace.

Supposing the hub is generally wakeup capable that flag
shouldn't be changed. Neither should be the policy based on
what is connected to the bus.

Re: [PATCH v7 2/5] usb: dwc3: core: Host wake up support from system suspend

[Sandeep Maheswaram]

Hi Felipe,

On 5/13/2021 3:17 AM, Matthias Kaehlcke wrote:
> On Mon, May 03, 2021 at 02:20:23PM +0300, Felipe Balbi wrote:
>> Hi,
>>
>> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>>>> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>>>>> 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 b6e53d8..bb414c3 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) {
>>>> should be able to detect this generically, no? Shouldn't
>>>> device_may_wakeup() be valid here and give you the answer you want?
>>> I think  device_may_wakeup() gives whether the controller is wake up
>>> capable or not.
>> Yes, but it's a bit more than that. Looking at devices.rst we read:
>>
>> If :c:func:`device_may_wakeup(dev)` returns ``true``, the device should be
>> prepared for generating hardware wakeup signals to trigger a system wakeup event
>> when the system is in the sleep state.  For example, :c:func:`enable_irq_wake()`
>> might identify GPIO signals hooked up to a switch or other external hardware,
>> and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal.
>>
>> So, if there is a condition where $this device has to, somehow, deal
>> with wakeup, it should be configured accordingly. This ->phy_power_off
>> flag is telling us the same thing.
>>
>>> But we want to keep phy powered on only when some wakeup capable devices
>>> (eg:keyboard ,mouse ) are connected to controller.
>> Understood, it could be that we're missing some method for propagating
>> that state (i.e. keyboard with PM support) up to the parent device, but
>> that's no excuse to bypass driver boundaries. Wouldn't you agree?
> I'm not sure if device_may_wakeup() is really the right tool for the
> job. This is the current implementation:
>
> static inline bool device_may_wakeup(struct device *dev)
> {
> 	return dev->power.can_wakeup && !!dev->power.wakeup;
> }
>
> IIUC power.can_wakeup specifies whether the device is wakeup
> capable, primarily in physical terms and indicating that the
> driver is ready to handle wakeups, and power.wakeup represents
> the policy which can be changed by userspace.
>
> Supposing the hub is generally wakeup capable that flag
> shouldn't be changed. Neither should be the policy based on
> what is connected to the bus.
Please suggest us how to proceed further with this patch.

[PATCH v7 3/5] 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 v7 4/5] 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

Re: [PATCH v7 4/5] usb: dwc3: qcom: Configure wakeup interrupts during suspend

[Felipe Balbi]

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


Hi,

Sandeep Maheswaram <sanm@codeaurora.org> writes:
> 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);

and who's using this?

>  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);

or this?

-- 
balbi

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

[PATCH v7 5/5] 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>
---
 drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 82125bc..1e220af 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -17,9 +17,11 @@
 #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>
+#include <linux/usb/hcd.h>
 
 #include "core.h"
 
@@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
 {
 	u32 val;
 	int i, ret;
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+	struct usb_hcd  *hcd;
+	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
 
 	if (qcom->is_suspended)
 		return 0;
 
+	if (dwc->xhci) {
+		hcd = platform_get_drvdata(dwc->xhci);
+		if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
+			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 +393,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 v7 5/5] usb: dwc3: qcom: Keep power domain on to support wakeup

[Felipe Balbi]

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


Hi,

Sandeep Maheswaram <sanm@codeaurora.org> writes:
> 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>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 82125bc..1e220af 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -17,9 +17,11 @@
>  #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>
> +#include <linux/usb/hcd.h>
>  
>  #include "core.h"
>  
> @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
>  {
>  	u32 val;
>  	int i, ret;
> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> +	struct usb_hcd  *hcd;
> +	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
>  
>  	if (qcom->is_suspended)
>  		return 0;
>  
> +	if (dwc->xhci) {
> +		hcd = platform_get_drvdata(dwc->xhci);
> +		if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
> +			genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
> +	}

wow, you really need to find a way to do these things generically
instead of bypassing a bunch of layers and access stuff $this doesn't
directly own.

I'm gonna say 'no' to this, sorry. It looks like xhci should, directly,
learn about much of this instead of hiding it 3-layers deep into the
dwc3 glue layer for your specific SoC.

-- 
balbi

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

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

[Matthias Kaehlcke]

On Wed, Apr 28, 2021 at 01:04:43PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Sandeep Maheswaram <sanm@codeaurora.org> writes:
> > 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>
> > ---
> >  drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 82125bc..1e220af 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -17,9 +17,11 @@
> >  #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>
> > +#include <linux/usb/hcd.h>
> >  
> >  #include "core.h"
> >  
> > @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> >  {
> >  	u32 val;
> >  	int i, ret;
> > +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> > +	struct usb_hcd  *hcd;
> > +	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
> >  
> >  	if (qcom->is_suspended)
> >  		return 0;
> >  
> > +	if (dwc->xhci) {
> > +		hcd = platform_get_drvdata(dwc->xhci);
> > +		if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
> > +			genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
> > +	}
> 
> wow, you really need to find a way to do these things generically
> instead of bypassing a bunch of layers and access stuff $this doesn't
> directly own.
>
> I'm gonna say 'no' to this, sorry. It looks like xhci should, directly,
> learn about much of this instead of hiding it 3-layers deep into the
> dwc3 glue layer for your specific SoC.

Maybe this could be addressed with a pair of wakeup quirks, one for suspend,
another for resume. An optional genpd field could be added to struct
xhci_plat_priv. The wakeup quirks would set/clear GENPD_FLAG_ACTIVE_WAKEUP
of the genpd.

Does the above sound more palatable?

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

[Felipe Balbi]

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


Hi,

Matthias Kaehlcke <mka@chromium.org> writes:
>> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>> > 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>
>> > ---
>> >  drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++
>> >  1 file changed, 17 insertions(+)
>> >
>> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> > index 82125bc..1e220af 100644
>> > --- a/drivers/usb/dwc3/dwc3-qcom.c
>> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> > @@ -17,9 +17,11 @@
>> >  #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>
>> > +#include <linux/usb/hcd.h>
>> >  
>> >  #include "core.h"
>> >  
>> > @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
>> >  {
>> >  	u32 val;
>> >  	int i, ret;
>> > +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>> > +	struct usb_hcd  *hcd;
>> > +	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
>> >  
>> >  	if (qcom->is_suspended)
>> >  		return 0;
>> >  
>> > +	if (dwc->xhci) {
>> > +		hcd = platform_get_drvdata(dwc->xhci);
>> > +		if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
>> > +			genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
>> > +	}
>> 
>> wow, you really need to find a way to do these things generically
>> instead of bypassing a bunch of layers and access stuff $this doesn't
>> directly own.
>>
>> I'm gonna say 'no' to this, sorry. It looks like xhci should, directly,
>> learn about much of this instead of hiding it 3-layers deep into the
>> dwc3 glue layer for your specific SoC.
>
> Maybe this could be addressed with a pair of wakeup quirks, one for suspend,
> another for resume. An optional genpd field could be added to struct
> xhci_plat_priv. The wakeup quirks would set/clear GENPD_FLAG_ACTIVE_WAKEUP
> of the genpd.
>
> Does the above sound more palatable?

I don't get why you need quirk flags. All you're doing here is:

	if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
        	genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;

If you move this test to xhci_suspend(), you shouldn't need all the
magic, right?

-- 
balbi

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

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

[Matthias Kaehlcke]

On Thu, May 13, 2021 at 04:49:19PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Matthias Kaehlcke <mka@chromium.org> writes:
> >> Sandeep Maheswaram <sanm@codeaurora.org> writes:
> >> > 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>
> >> > ---
> >> >  drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++
> >> >  1 file changed, 17 insertions(+)
> >> >
> >> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> >> > index 82125bc..1e220af 100644
> >> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> >> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> >> > @@ -17,9 +17,11 @@
> >> >  #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>
> >> > +#include <linux/usb/hcd.h>
> >> >  
> >> >  #include "core.h"
> >> >  
> >> > @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> >> >  {
> >> >  	u32 val;
> >> >  	int i, ret;
> >> > +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> >> > +	struct usb_hcd  *hcd;
> >> > +	struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
> >> >  
> >> >  	if (qcom->is_suspended)
> >> >  		return 0;
> >> >  
> >> > +	if (dwc->xhci) {
> >> > +		hcd = platform_get_drvdata(dwc->xhci);
> >> > +		if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
> >> > +			genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
> >> > +	}
> >> 
> >> wow, you really need to find a way to do these things generically
> >> instead of bypassing a bunch of layers and access stuff $this doesn't
> >> directly own.
> >>
> >> I'm gonna say 'no' to this, sorry. It looks like xhci should, directly,
> >> learn about much of this instead of hiding it 3-layers deep into the
> >> dwc3 glue layer for your specific SoC.
> >
> > Maybe this could be addressed with a pair of wakeup quirks, one for suspend,
> > another for resume. An optional genpd field could be added to struct
> > xhci_plat_priv. The wakeup quirks would set/clear GENPD_FLAG_ACTIVE_WAKEUP
> > of the genpd.
> >
> > Does the above sound more palatable?
> 
> I don't get why you need quirk flags. All you're doing here is:
> 
> 	if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
>         	genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
> 
> If you move this test to xhci_suspend(), you shouldn't need all the
> magic, right?

Right, the quirks shouldn't be necessary if setting/clearing the genpd
flag is the right thing to do for any controller that sets the genpd
field in _priv, which probably is the case.