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

[PATCH v10 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_ALWAYS_ON flag to keep usb30_prim gdsc active to retain
controller status during suspend/resume.

Changes in v10:
PATCH 1/6: Change device_set_wakeup_capable to device_set_wakeup_enable
PATCH 2/6: Remove redundant else part in dwc3_resume_common
PATCH 4/6: Change the irg flags
PATCH 5/6: Set flag GENPD_FLAG_ALWAYS_ON
PATCH 6/6: Remove remove disable interrupts function and enable
interrupts in probe.


Changes in v9:
Checking with device_may_makeup property instead of phy_power_off flag.
Changed the IRQ flags and removed hs_phy_mode variable.

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: 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: Change the IRQ flag for DP/DM hs phy irq
  usb: dwc3: qcom: Keep power domain on to retain controller status
  usb: dwc3: qcom: Enable the interrupts during probe

 drivers/usb/dwc3/core.c      |  4 +--
 drivers/usb/dwc3/dwc3-qcom.c | 64 +++++++++++++++-----------------------------
 drivers/usb/host/xhci-plat.c | 12 +++++++++
 3 files changed, 36 insertions(+), 44 deletions(-)

-- 
2.7.4

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

[Sandeep Maheswaram]

During suspend check if any wakeup capable devices are connected to the
controller (directly or through hubs), and set the wakeup enable property
for xhci plat device.

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Change device_set_wakeup_capable to device_set_wakeup_enable as 
wakeup capable false was deleting the sysfs property.

 drivers/usb/host/xhci-plat.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index c1edcc9..1c8fadb 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -431,6 +431,14 @@ static int xhci_plat_remove(struct platform_device *dev)
 	return 0;
 }
 
+static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd, struct device *dev)
+{
+	if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
+		device_set_wakeup_enable(dev, true);
+	else
+		device_set_wakeup_enable(dev, false);
+}
+
 static int __maybe_unused xhci_plat_suspend(struct device *dev)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
@@ -440,6 +448,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, dev);
+
 	/*
 	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
 	 * to do wakeup during suspend.
-- 
2.7.4

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

[Greg Kroah-Hartman]

On Mon, Jan 17, 2022 at 11:14:03AM +0530, Sandeep Maheswaram wrote:
> During suspend check if any wakeup capable devices are connected to the
> controller (directly or through hubs), and set the wakeup enable property
> for xhci plat device.
> 
> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> Change device_set_wakeup_capable to device_set_wakeup_enable as 
> wakeup capable false was deleting the sysfs property.
> 
>  drivers/usb/host/xhci-plat.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index c1edcc9..1c8fadb 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -431,6 +431,14 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	return 0;
>  }
>  
> +static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd, struct device *dev)
> +{
> +	if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
> +		device_set_wakeup_enable(dev, true);
> +	else
> +		device_set_wakeup_enable(dev, false);
> +}
> +
>  static int __maybe_unused xhci_plat_suspend(struct device *dev)
>  {
>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
> @@ -440,6 +448,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, dev);

I still think that checking for this type of thing needs to be in the
platform specific driver, not in the generic one.

thanks,

greg k-h

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

[Sandeep Maheswaram]

Avoiding phy powerdown when wakeup capable devices are connected
by checking wakeup property of xhci plat device.
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 <quic_c_sanm@quicinc.com>
Tested-by: Brian Norris <briannorris@chromium.org>
---
Remove redundant else part in dwc3_resume_common. This will not be
required if GDSC is always on during suspend/resume.


 drivers/usb/dwc3/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index f4c0995..e7a5e3f 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1789,7 +1789,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;
 		}
@@ -1850,7 +1850,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;
-- 
2.7.4

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

[Jun Li]

Sandeep Maheswaram <quic_c_sanm@quicinc.com> 于2022年1月17日周一 22:03写道：
>
> Avoiding phy powerdown when wakeup capable devices are connected
> by checking wakeup property of xhci plat device.
> 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 <quic_c_sanm@quicinc.com>
> Tested-by: Brian Norris <briannorris@chromium.org>
> ---
> Remove redundant else part in dwc3_resume_common. This will not be
> required if GDSC is always on during suspend/resume.
>
>
>  drivers/usb/dwc3/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index f4c0995..e7a5e3f 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1789,7 +1789,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;
>                 }
> @@ -1850,7 +1850,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)) {

If XHCI_SKIP_PHY_INIT is not set, I see the usb core will help to
handle phy power on/off and init/exit via drivers/usb/core/phy.c, so
if the wakeup is enabled for controller, then finally the phy will not
be power off/exit. I am wondering if this change is actually required if
that is the case.

Sorry for the late comment.

Li Jun



>                         ret = dwc3_core_init_for_resume(dwc);
>                         if (ret)
>                                 return ret;
> --
> 2.7.4
>

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

[Sandeep Maheswaram]

On 1/18/2022 2:27 PM, Jun Li wrote:
> Sandeep Maheswaram <quic_c_sanm@quicinc.com> 于2022年1月17日周一 22:03写道：
>> Avoiding phy powerdown when wakeup capable devices are connected
>> by checking wakeup property of xhci plat device.
>> 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 <quic_c_sanm@quicinc.com>
>> Tested-by: Brian Norris <briannorris@chromium.org>
>> ---
>> Remove redundant else part in dwc3_resume_common. This will not be
>> required if GDSC is always on during suspend/resume.
>>
>>
>>   drivers/usb/dwc3/core.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index f4c0995..e7a5e3f 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1789,7 +1789,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;
>>                  }
>> @@ -1850,7 +1850,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)) {
> If XHCI_SKIP_PHY_INIT is not set, I see the usb core will help to
> handle phy power on/off and init/exit via drivers/usb/core/phy.c, so
> if the wakeup is enabled for controller, then finally the phy will not
> be power off/exit. I am wondering if this change is actually required if
> that is the case.
>
> Sorry for the late comment.
>
> Li Jun
>
The patch is to avoid phy power off in case only if some wakeup capable 
devices are connected.

Regarding  XHCI_SKIP_PHY_INIT we are setting  in this patch

https://patchwork.kernel.org/project/linux-arm-msm/patch/1640153383-21036-1-git-send-email-quic_c_sanm@quicinc.com/

>
>>                          ret = dwc3_core_init_for_resume(dwc);
>>                          if (ret)
>>                                  return ret;
>> --
>> 2.7.4
>>

[PATCH v10 3/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 <quic_c_sanm@quicinc.com>
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 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

[PATCH v10 4/6] usb: dwc3: qcom: Change the IRQ flag for DP/DM hs phy irq

[Sandeep Maheswaram]

Change the IRQ flags for DP/DM hs phy irq to avoid interrupt
triggering during system suspend.

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
---
Change the irg flags

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

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 7352124..b13e542 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -473,7 +473,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
 		irq_set_status_flags(irq, IRQ_NOAUTOEN);
 		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
 					qcom_dwc3_resume_irq,
-					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+					IRQF_ONESHOT,
 					"qcom_dwc3 DP_HS", qcom);
 		if (ret) {
 			dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret);
@@ -488,7 +488,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
 		irq_set_status_flags(irq, IRQ_NOAUTOEN);
 		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
 					qcom_dwc3_resume_irq,
-					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+					IRQF_ONESHOT,
 					"qcom_dwc3 DM_HS", qcom);
 		if (ret) {
 			dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret);
-- 
2.7.4

[PATCH v10 5/6] usb: dwc3: qcom: Keep power domain on to retain controller status

[Sandeep Maheswaram]

Keep the power domain on in order to retail controller status and
to support wakeup from devices.

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
---
 drivers/usb/dwc3/dwc3-qcom.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index b13e542..54dc3d3 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>
@@ -710,6 +711,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	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)
@@ -718,6 +720,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) {
@@ -825,6 +829,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto interconnect_exit;
 
+	genpd->flags |= GENPD_FLAG_ALWAYS_ON;
+
 	device_init_wakeup(&pdev->dev, 1);
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
-- 
2.7.4

[PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Sandeep Maheswaram]

Enable the interrupts during probe and remove the disable interrupts
function.

Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
---
 drivers/usb/dwc3/dwc3-qcom.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 54dc3d3..7c5e636 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -306,25 +306,7 @@ static void dwc3_qcom_enable_wakeup_irq(int 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)
-{
-	dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
-
-	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)
 {
@@ -356,9 +338,6 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
 	if (ret)
 		dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
 
-	if (device_may_wakeup(qcom->dev))
-		dwc3_qcom_enable_interrupts(qcom);
-
 	qcom->is_suspended = true;
 
 	return 0;
@@ -372,9 +351,6 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
 	if (!qcom->is_suspended)
 		return 0;
 
-	if (device_may_wakeup(qcom->dev))
-		dwc3_qcom_disable_interrupts(qcom);
-
 	for (i = 0; i < qcom->num_clocks; i++) {
 		ret = clk_prepare_enable(qcom->clks[i]);
 		if (ret < 0) {
@@ -832,6 +808,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	genpd->flags |= GENPD_FLAG_ALWAYS_ON;
 
 	device_init_wakeup(&pdev->dev, 1);
+
+	if (device_may_wakeup(qcom->dev))
+		dwc3_qcom_enable_interrupts(qcom);
+
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
-- 
2.7.4

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Steev Klimaszewski]

On 1/16/22 11:44 PM, Sandeep Maheswaram wrote:
> Enable the interrupts during probe and remove the disable interrupts
> function.
>
> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> ---
>   drivers/usb/dwc3/dwc3-qcom.c | 28 ++++------------------------
>   1 file changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 54dc3d3..7c5e636 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -306,25 +306,7 @@ static void dwc3_qcom_enable_wakeup_irq(int 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)
> -{
> -	dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
> -
> -	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)
>   {
> @@ -356,9 +338,6 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
>   	if (ret)
>   		dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
>   
> -	if (device_may_wakeup(qcom->dev))
> -		dwc3_qcom_enable_interrupts(qcom);
> -
>   	qcom->is_suspended = true;
>   
>   	return 0;
> @@ -372,9 +351,6 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
>   	if (!qcom->is_suspended)
>   		return 0;
>   
> -	if (device_may_wakeup(qcom->dev))
> -		dwc3_qcom_disable_interrupts(qcom);
> -
>   	for (i = 0; i < qcom->num_clocks; i++) {
>   		ret = clk_prepare_enable(qcom->clks[i]);
>   		if (ret < 0) {
> @@ -832,6 +808,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>   	genpd->flags |= GENPD_FLAG_ALWAYS_ON;
>   
>   	device_init_wakeup(&pdev->dev, 1);
> +
> +	if (device_may_wakeup(qcom->dev))
> +		dwc3_qcom_enable_interrupts(qcom);
> +
>   	qcom->is_suspended = false;
>   	pm_runtime_set_active(dev);
>   	pm_runtime_enable(dev);

Hi Sandeep,

I was testing this series on my Lenovo Yoga C630, and with this patch in 
particular applied, my system will no longer boot. Unfortunately I don't 
get any sort of good output at all, I just get hung tasks when trying to 
probe things it would seem.


With the other 5 patches in the series applied, the system still boots 
and works correctly.


-- Steev

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Sandeep Maheswaram]

Hi Steev,

On 1/18/2022 11:42 AM, Steev Klimaszewski wrote:
>
> On 1/16/22 11:44 PM, Sandeep Maheswaram wrote:
>> Enable the interrupts during probe and remove the disable interrupts
>> function.
>>
>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
>> ---
>>   drivers/usb/dwc3/dwc3-qcom.c | 28 ++++------------------------
>>   1 file changed, 4 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index 54dc3d3..7c5e636 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> @@ -306,25 +306,7 @@ static void dwc3_qcom_enable_wakeup_irq(int 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)
>> -{
>> -    dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
>> -
>> -    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)
>>   {
>> @@ -356,9 +338,6 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
>>       if (ret)
>>           dev_warn(qcom->dev, "failed to disable interconnect: %d\n", 
>> ret);
>>   -    if (device_may_wakeup(qcom->dev))
>> -        dwc3_qcom_enable_interrupts(qcom);
>> -
>>       qcom->is_suspended = true;
>>         return 0;
>> @@ -372,9 +351,6 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
>>       if (!qcom->is_suspended)
>>           return 0;
>>   -    if (device_may_wakeup(qcom->dev))
>> -        dwc3_qcom_disable_interrupts(qcom);
>> -
>>       for (i = 0; i < qcom->num_clocks; i++) {
>>           ret = clk_prepare_enable(qcom->clks[i]);
>>           if (ret < 0) {
>> @@ -832,6 +808,10 @@ static int dwc3_qcom_probe(struct 
>> platform_device *pdev)
>>       genpd->flags |= GENPD_FLAG_ALWAYS_ON;
>>         device_init_wakeup(&pdev->dev, 1);
>> +
>> +    if (device_may_wakeup(qcom->dev))
>> +        dwc3_qcom_enable_interrupts(qcom);
>> +
>>       qcom->is_suspended = false;
>>       pm_runtime_set_active(dev);
>>       pm_runtime_enable(dev);
>
> Hi Sandeep,
>
> I was testing this series on my Lenovo Yoga C630, and with this patch 
> in particular applied, my system will no longer boot. Unfortunately I 
> don't get any sort of good output at all, I just get hung tasks when 
> trying to probe things it would seem.
>
>
> With the other 5 patches in the series applied, the system still boots 
> and works correctly.
>
>
> -- Steev
>
Will check this. Is your controller in host mode or device mode?

Regards

Sandeep

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Steev Klimaszewski]

On 1/18/22 12:30 AM, Sandeep Maheswaram wrote:
> Hi Steev,
<snip>
>> Hi Sandeep,
>>
>> I was testing this series on my Lenovo Yoga C630, and with this patch 
>> in particular applied, my system will no longer boot. Unfortunately I 
>> don't get any sort of good output at all, I just get hung tasks when 
>> trying to probe things it would seem.
>>
>>
>> With the other 5 patches in the series applied, the system still 
>> boots and works correctly.
>>
>>
>> -- Steev
>>
> Will check this. Is your controller in host mode or device mode?
>
> Regards
>
> Sandeep
>
Both usb_1_dwc3 and usb_2_dwc3 are in host mode

-- Steev

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Pavan Kondeti]

On Tue, Jan 18, 2022 at 12:12:30AM -0600, Steev Klimaszewski wrote:
> 
> On 1/16/22 11:44 PM, Sandeep Maheswaram wrote:
> >Enable the interrupts during probe and remove the disable interrupts
> >function.
> >
> >Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> >---
> >  drivers/usb/dwc3/dwc3-qcom.c | 28 ++++------------------------
> >  1 file changed, 4 insertions(+), 24 deletions(-)
> >
> >diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> >index 54dc3d3..7c5e636 100644
> >--- a/drivers/usb/dwc3/dwc3-qcom.c
> >+++ b/drivers/usb/dwc3/dwc3-qcom.c
> >@@ -306,25 +306,7 @@ static void dwc3_qcom_enable_wakeup_irq(int 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)
> >-{
> >-	dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
> >-
> >-	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)
> >  {
> >@@ -356,9 +338,6 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> >  	if (ret)
> >  		dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
> >-	if (device_may_wakeup(qcom->dev))
> >-		dwc3_qcom_enable_interrupts(qcom);
> >-
> >  	qcom->is_suspended = true;
> >  	return 0;
> >@@ -372,9 +351,6 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
> >  	if (!qcom->is_suspended)
> >  		return 0;
> >-	if (device_may_wakeup(qcom->dev))
> >-		dwc3_qcom_disable_interrupts(qcom);
> >-
> >  	for (i = 0; i < qcom->num_clocks; i++) {
> >  		ret = clk_prepare_enable(qcom->clks[i]);
> >  		if (ret < 0) {
> >@@ -832,6 +808,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> >  	genpd->flags |= GENPD_FLAG_ALWAYS_ON;
> >  	device_init_wakeup(&pdev->dev, 1);
> >+
> >+	if (device_may_wakeup(qcom->dev))
> >+		dwc3_qcom_enable_interrupts(qcom);
> >+
> >  	qcom->is_suspended = false;
> >  	pm_runtime_set_active(dev);
> >  	pm_runtime_enable(dev);
> 
> Hi Sandeep,
> 
> I was testing this series on my Lenovo Yoga C630, and with this patch in
> particular applied, my system will no longer boot. Unfortunately I don't get
> any sort of good output at all, I just get hung tasks when trying to probe
> things it would seem.
> 
> 
> With the other 5 patches in the series applied, the system still boots and
> works correctly.
> 
> 

Sandeep,

Enable DP/DM interrupts all the time might be creating a storm of interrupts.
calling enable_irq_wake() during probe is okay, but not the enable_irq().

Did you verify your change with a Highspeed/Fullspeed device connected?

Thanks,
Pavan

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Sandeep Maheswaram]

Hi Pavan,

On 1/18/2022 3:22 PM, Pavan Kondeti wrote:
> On Tue, Jan 18, 2022 at 12:12:30AM -0600, Steev Klimaszewski wrote:
>> On 1/16/22 11:44 PM, Sandeep Maheswaram wrote:
>>> Enable the interrupts during probe and remove the disable interrupts
>>> function.
>>>
>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
>>> ---
>>>   drivers/usb/dwc3/dwc3-qcom.c | 28 ++++------------------------
>>>   1 file changed, 4 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>>> index 54dc3d3..7c5e636 100644
>>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>>> @@ -306,25 +306,7 @@ static void dwc3_qcom_enable_wakeup_irq(int 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)
>>> -{
>>> -	dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
>>> -
>>> -	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)
>>>   {
>>> @@ -356,9 +338,6 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
>>>   	if (ret)
>>>   		dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
>>> -	if (device_may_wakeup(qcom->dev))
>>> -		dwc3_qcom_enable_interrupts(qcom);
>>> -
>>>   	qcom->is_suspended = true;
>>>   	return 0;
>>> @@ -372,9 +351,6 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
>>>   	if (!qcom->is_suspended)
>>>   		return 0;
>>> -	if (device_may_wakeup(qcom->dev))
>>> -		dwc3_qcom_disable_interrupts(qcom);
>>> -
>>>   	for (i = 0; i < qcom->num_clocks; i++) {
>>>   		ret = clk_prepare_enable(qcom->clks[i]);
>>>   		if (ret < 0) {
>>> @@ -832,6 +808,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>>   	genpd->flags |= GENPD_FLAG_ALWAYS_ON;
>>>   	device_init_wakeup(&pdev->dev, 1);
>>> +
>>> +	if (device_may_wakeup(qcom->dev))
>>> +		dwc3_qcom_enable_interrupts(qcom);
>>> +
>>>   	qcom->is_suspended = false;
>>>   	pm_runtime_set_active(dev);
>>>   	pm_runtime_enable(dev);
>> Hi Sandeep,
>>
>> I was testing this series on my Lenovo Yoga C630, and with this patch in
>> particular applied, my system will no longer boot. Unfortunately I don't get
>> any sort of good output at all, I just get hung tasks when trying to probe
>> things it would seem.
>>
>>
>> With the other 5 patches in the series applied, the system still boots and
>> works correctly.
>>
>>
> Sandeep,
>
> Enable DP/DM interrupts all the time might be creating a storm of interrupts.
> calling enable_irq_wake() during probe is okay, but not the enable_irq().
>
> Did you verify your change with a Highspeed/Fullspeed device connected?
>
> Thanks,
> Pavan

I didn't face any such issue with devices connected.

I think this is because I used IRQ_TYPE_EDGE_BOTH in device tree and 
Steev has IRQ_TYPE_LEVEL_HIGH in his device tree.

When i changed to IRQ_TYPE_LEVEL_HIGH I also observed a storm of 
interrupts in my device though it booted .

Regards

Sandeep

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Sandeep Maheswaram]

Hi Steev,

On 1/20/2022 10:52 AM, Sandeep Maheswaram wrote:
> Hi Pavan,
>
> On 1/18/2022 3:22 PM, Pavan Kondeti wrote:
>> On Tue, Jan 18, 2022 at 12:12:30AM -0600, Steev Klimaszewski wrote:
>>> On 1/16/22 11:44 PM, Sandeep Maheswaram wrote:
>>>> Enable the interrupts during probe and remove the disable interrupts
>>>> function.
>>>>
>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
>>>> ---
>>>>   drivers/usb/dwc3/dwc3-qcom.c | 28 ++++------------------------
>>>>   1 file changed, 4 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c 
>>>> b/drivers/usb/dwc3/dwc3-qcom.c
>>>> index 54dc3d3..7c5e636 100644
>>>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>>>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>>>> @@ -306,25 +306,7 @@ static void dwc3_qcom_enable_wakeup_irq(int 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)
>>>> -{
>>>> -    dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
>>>> -
>>>> -    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)
>>>>   {
>>>> @@ -356,9 +338,6 @@ static int dwc3_qcom_suspend(struct dwc3_qcom 
>>>> *qcom)
>>>>       if (ret)
>>>>           dev_warn(qcom->dev, "failed to disable interconnect: 
>>>> %d\n", ret);
>>>> -    if (device_may_wakeup(qcom->dev))
>>>> -        dwc3_qcom_enable_interrupts(qcom);
>>>> -
>>>>       qcom->is_suspended = true;
>>>>       return 0;
>>>> @@ -372,9 +351,6 @@ static int dwc3_qcom_resume(struct dwc3_qcom 
>>>> *qcom)
>>>>       if (!qcom->is_suspended)
>>>>           return 0;
>>>> -    if (device_may_wakeup(qcom->dev))
>>>> -        dwc3_qcom_disable_interrupts(qcom);
>>>> -
>>>>       for (i = 0; i < qcom->num_clocks; i++) {
>>>>           ret = clk_prepare_enable(qcom->clks[i]);
>>>>           if (ret < 0) {
>>>> @@ -832,6 +808,10 @@ static int dwc3_qcom_probe(struct 
>>>> platform_device *pdev)
>>>>       genpd->flags |= GENPD_FLAG_ALWAYS_ON;
>>>>       device_init_wakeup(&pdev->dev, 1);
>>>> +
>>>> +    if (device_may_wakeup(qcom->dev))
>>>> +        dwc3_qcom_enable_interrupts(qcom);
>>>> +
>>>>       qcom->is_suspended = false;
>>>>       pm_runtime_set_active(dev);
>>>>       pm_runtime_enable(dev);
>>> Hi Sandeep,
>>>
>>> I was testing this series on my Lenovo Yoga C630, and with this 
>>> patch in
>>> particular applied, my system will no longer boot. Unfortunately I 
>>> don't get
>>> any sort of good output at all, I just get hung tasks when trying to 
>>> probe
>>> things it would seem.
>>>
>>>
>>> With the other 5 patches in the series applied, the system still 
>>> boots and
>>> works correctly.
>>>
>>>
>> Sandeep,
>>
>> Enable DP/DM interrupts all the time might be creating a storm of 
>> interrupts.
>> calling enable_irq_wake() during probe is okay, but not the 
>> enable_irq().
>>
>> Did you verify your change with a Highspeed/Fullspeed device connected?
>>
>> Thanks,
>> Pavan
>
> I didn't face any such issue with devices connected.
>
> I think this is because I used IRQ_TYPE_EDGE_BOTH in device tree and 
> Steev has IRQ_TYPE_LEVEL_HIGH in his device tree.
>
> When i changed to IRQ_TYPE_LEVEL_HIGH I also observed a storm of 
> interrupts in my device though it booted .
>
> Regards
>
> Sandeep
>
Can you try with IRQ_TYPE_EDGE_BOTH in your device tree and see if you 
are getting the issue.

Regards

Sandeep

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Steev Klimaszewski]

Hi Sandeep,

On 1/25/22 3:17 AM, Sandeep Maheswaram wrote:
> Hi Steev,
>
>> I didn't face any such issue with devices connected.
>>
>> I think this is because I used IRQ_TYPE_EDGE_BOTH in device tree and 
>> Steev has IRQ_TYPE_LEVEL_HIGH in his device tree.
>>
>> When i changed to IRQ_TYPE_LEVEL_HIGH I also observed a storm of 
>> interrupts in my device though it booted .
>>
>> Regards
>>
>> Sandeep
>>
> Can you try with IRQ_TYPE_EDGE_BOTH in your device tree and see if you 
> are getting the issue.
>
> Regards
>
> Sandeep
>
I just tested here, changing both of the IRQ_TYPE_LEVEL_HIGH in the 
yoga's dts to EDGE_BOTH and I still do not get a booting system.

-- Steev

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Pavan Kondeti]

On Fri, Jan 28, 2022 at 02:36:38AM -0600, Steev Klimaszewski wrote:
> Hi Sandeep,
> 
> On 1/25/22 3:17 AM, Sandeep Maheswaram wrote:
> >Hi Steev,
> >
> >>I didn't face any such issue with devices connected.
> >>
> >>I think this is because I used IRQ_TYPE_EDGE_BOTH in device tree and
> >>Steev has IRQ_TYPE_LEVEL_HIGH in his device tree.
> >>
> >>When i changed to IRQ_TYPE_LEVEL_HIGH I also observed a storm of
> >>interrupts in my device though it booted .
> >>
> >>Regards
> >>
> >>Sandeep
> >>
> >Can you try with IRQ_TYPE_EDGE_BOTH in your device tree and see if you are
> >getting the issue.
> >
> >Regards
> >
> >Sandeep
> >
> I just tested here, changing both of the IRQ_TYPE_LEVEL_HIGH in the yoga's
> dts to EDGE_BOTH and I still do not get a booting system.
> 

Sandeep,

For whatever reason, if the interrupt comes immediately after enabling it in
the probe, are we ready to call pm_runtime_resume(&dwc->xhci->dev); ? 

I am not sure if Steev is facing an interrupt storm issue or some kind of
incorrect access and device is crashing. In any case, can you simulate this
and see if we can call the above runtime PM API in dwc->xhci->dev immediately
after enabling the IRQs.

Thanks,
Pavan

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Sandeep Maheswaram]

Hi Steev,

On 1/28/2022 2:06 PM, Steev Klimaszewski wrote:
> Hi Sandeep,
>
> On 1/25/22 3:17 AM, Sandeep Maheswaram wrote:
>> Hi Steev,
>>
>>> I didn't face any such issue with devices connected.
>>>
>>> I think this is because I used IRQ_TYPE_EDGE_BOTH in device tree and 
>>> Steev has IRQ_TYPE_LEVEL_HIGH in his device tree.
>>>
>>> When i changed to IRQ_TYPE_LEVEL_HIGH I also observed a storm of 
>>> interrupts in my device though it booted .
>>>
>>> Regards
>>>
>>> Sandeep
>>>
>> Can you try with IRQ_TYPE_EDGE_BOTH in your device tree and see if 
>> you are getting the issue.
>>
>> Regards
>>
>> Sandeep
>>
> I just tested here, changing both of the IRQ_TYPE_LEVEL_HIGH in the 
> yoga's dts to EDGE_BOTH and I still do not get a booting system.
>
> -- Steev
>
Please let us know what devices are connected to your setup and share 
the device tree file you are using.

Please share the failure logs also,

Regards

Sandeep

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Steev Klimaszewski]

Hi Sandeep,

On 2/15/22 3:40 AM, Sandeep Maheswaram wrote:
> Hi Steev,
>
>>>>
>>> Can you try with IRQ_TYPE_EDGE_BOTH in your device tree and see if 
>>> you are getting the issue.
>>>
>>> Regards
>>>
>>> Sandeep
>>>
>> I just tested here, changing both of the IRQ_TYPE_LEVEL_HIGH in the 
>> yoga's dts to EDGE_BOTH and I still do not get a booting system.
>>
>> -- Steev
>>
> Please let us know what devices are connected to your setup and share 
> the device tree file you are using.
>
> Please share the failure logs also,
>
> Regards
>
> Sandeep
>
The setup is a Lenovo Yoga C630 (Windows on ARM laptop).  I do not have 
any sort of serial console access to the device, unfortunately.  Even 
when taking it apart, it seems to have some sort of 26pin debug adapter 
port that I've never seen before which you can see on the far right in 
this picture of the motherboard at 
https://i.ebayimg.com/images/g/a2EAAOSwwzZiCxPM/s-l1600.jpg

I do not have anything plugged in to the USB ports (sometimes the power 
adapter, but I have tried both on mains as well as off.)

I am using this diff


diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
index eab3f00c603235..c54042b9e21df2 100644
--- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
+++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
@@ -370,7 +370,7 @@
  		reg = <0x15>;
  		hid-descr-addr = <0x1>;
  
-		interrupts-extended = <&tlmm 37 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts-extended = <&tlmm 37 IRQ_TYPE_EDGE_BOTH>;
  	};
  
  	tsc2: hid@2c {
@@ -378,7 +378,7 @@
  		reg = <0x2c>;
  		hid-descr-addr = <0x20>;
  
-		interrupts-extended = <&tlmm 37 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts-extended = <&tlmm 37 IRQ_TYPE_EDGE_BOTH>;
  	};
  };

Which I added as a commit to my kernel tree, and pushed so you can see 
the full dts here: 
https://github.com/steev/linux/blob/c8234e664491e35e3edcd211f3b78c04436402b0/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts

I am booting with the command line arguments of

clk_ignore_unused verbose module_blacklist=msm video=efifb 
earlyconsole=efifb

I can't provide a boot log, because I'm not actually getting anything.  
Booting a different kernel, and it doesn't appear that anything is 
logged at all.


-- steev

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Sandeep Maheswaram]

Hi Steev

On 2/16/2022 8:52 AM, Steev Klimaszewski wrote:
> Hi Sandeep,
>
> On 2/15/22 3:40 AM, Sandeep Maheswaram wrote:
>> Hi Steev,
>>
>>>>>
>>>> Can you try with IRQ_TYPE_EDGE_BOTH in your device tree and see if 
>>>> you are getting the issue.
>>>>
>>>> Regards
>>>>
>>>> Sandeep
>>>>
>>> I just tested here, changing both of the IRQ_TYPE_LEVEL_HIGH in the 
>>> yoga's dts to EDGE_BOTH and I still do not get a booting system.
>>>
>>> -- Steev
>>>
>> Please let us know what devices are connected to your setup and share 
>> the device tree file you are using.
>>
>> Please share the failure logs also,
>>
>> Regards
>>
>> Sandeep
>>
> The setup is a Lenovo Yoga C630 (Windows on ARM laptop).  I do not 
> have any sort of serial console access to the device, unfortunately.  
> Even when taking it apart, it seems to have some sort of 26pin debug 
> adapter port that I've never seen before which you can see on the far 
> right in this picture of the motherboard at 
> https://i.ebayimg.com/images/g/a2EAAOSwwzZiCxPM/s-l1600.jpg
>
> I do not have anything plugged in to the USB ports (sometimes the 
> power adapter, but I have tried both on mains as well as off.)
>
> I am using this diff
>
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts 
> b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> index eab3f00c603235..c54042b9e21df2 100644
> --- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> @@ -370,7 +370,7 @@
>          reg = <0x15>;
>          hid-descr-addr = <0x1>;
>
> -        interrupts-extended = <&tlmm 37 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupts-extended = <&tlmm 37 IRQ_TYPE_EDGE_BOTH>;
>      };
>
>      tsc2: hid@2c {
> @@ -378,7 +378,7 @@
>          reg = <0x2c>;
>          hid-descr-addr = <0x20>;
>
> -        interrupts-extended = <&tlmm 37 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupts-extended = <&tlmm 37 IRQ_TYPE_EDGE_BOTH>;
>      };
>  };
>
> Which I added as a commit to my kernel tree, and pushed so you can see 
> the full dts here: 
> https://github.com/steev/linux/blob/c8234e664491e35e3edcd211f3b78c04436402b0/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
>
> I am booting with the command line arguments of
>
> clk_ignore_unused verbose module_blacklist=msm video=efifb 
> earlyconsole=efifb
>
> I can't provide a boot log, because I'm not actually getting 
> anything.  Booting a different kernel, and it doesn't appear that 
> anything is logged at all.
>
>
> -- steev
>
Can you try with below change

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi 
b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 0d6286d..0a9c0f7 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -3796,8 +3796,8 @@

                         interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
                                      <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>,
-                                    <GIC_SPI 488 IRQ_TYPE_LEVEL_HIGH>,
-                                    <GIC_SPI 489 IRQ_TYPE_LEVEL_HIGH>;
+                                    <GIC_SPI 488 IRQ_TYPE_EDGE_BOTH>,
+                                    <GIC_SPI 489 IRQ_TYPE_EDGE_BOTH>;
                         interrupt-names = "hs_phy_irq", "ss_phy_irq",
                                           "dm_hs_phy_irq", "dp_hs_phy_irq";

@@ -3844,8 +3844,8 @@

                         interrupts = <GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>,
                                      <GIC_SPI 487 IRQ_TYPE_LEVEL_HIGH>,
-                                    <GIC_SPI 490 IRQ_TYPE_LEVEL_HIGH>,
-                                    <GIC_SPI 491 IRQ_TYPE_LEVEL_HIGH>;
+                                    <GIC_SPI 490 IRQ_TYPE_EDGE_BOTH>,
+                                    <GIC_SPI 491 IRQ_TYPE_EDGE_BOTH>;
                         interrupt-names = "hs_phy_irq", "ss_phy_irq",
                                           "dm_hs_phy_irq", "dp_hs_phy_irq";

Regards

Sandeep

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Steev Klimaszewski]

Hi Sandeep,

On 2/16/22 12:27 AM, Sandeep Maheswaram wrote:
> Hi Steev
>
> On 2/16/2022 8:52 AM, Steev Klimaszewski wrote:
>> Hi Sandeep,
>>
>> On 2/15/22 3:40 AM, Sandeep Maheswaram wrote:
>>> Hi Steev,
>>>
>>>>>>
>>>>> Can you try with IRQ_TYPE_EDGE_BOTH in your device tree and see if 
>>>>> you are getting the issue.
>>>>>
>>>>> Regards
>>>>>
>>>>> Sandeep
>>>>>
>>>> I just tested here, changing both of the IRQ_TYPE_LEVEL_HIGH in the 
>>>> yoga's dts to EDGE_BOTH and I still do not get a booting system.
>>>>
>>>> -- Steev
>>>>
>>> Please let us know what devices are connected to your setup and 
>>> share the device tree file you are using.
>>>
>>> Please share the failure logs also,
>>>
>>> Regards
>>>
>>> Sandeep
>>>
>> The setup is a Lenovo Yoga C630 (Windows on ARM laptop).  I do not 
>> have any sort of serial console access to the device, unfortunately.  
>> Even when taking it apart, it seems to have some sort of 26pin debug 
>> adapter port that I've never seen before which you can see on the far 
>> right in this picture of the motherboard at 
>> https://i.ebayimg.com/images/g/a2EAAOSwwzZiCxPM/s-l1600.jpg
>>
>> I do not have anything plugged in to the USB ports (sometimes the 
>> power adapter, but I have tried both on mains as well as off.)
>>
>> Which I added as a commit to my kernel tree, and pushed so you can 
>> see the full dts here: 
>> https://github.com/steev/linux/blob/c8234e664491e35e3edcd211f3b78c04436402b0/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
>>
>> I am booting with the command line arguments of
>>
>> clk_ignore_unused verbose module_blacklist=msm video=efifb 
>> earlyconsole=efifb
>>
>> I can't provide a boot log, because I'm not actually getting 
>> anything.  Booting a different kernel, and it doesn't appear that 
>> anything is logged at all.
>>
>>
>> -- steev
>>
> Can you try with below change
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi 
> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 0d6286d..0a9c0f7 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -3796,8 +3796,8 @@
>
>                         interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
>                                      <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>,
> -                                    <GIC_SPI 488 IRQ_TYPE_LEVEL_HIGH>,
> -                                    <GIC_SPI 489 IRQ_TYPE_LEVEL_HIGH>;
> +                                    <GIC_SPI 488 IRQ_TYPE_EDGE_BOTH>,
> +                                    <GIC_SPI 489 IRQ_TYPE_EDGE_BOTH>;
>                         interrupt-names = "hs_phy_irq", "ss_phy_irq",
>                                           "dm_hs_phy_irq", 
> "dp_hs_phy_irq";
>
> @@ -3844,8 +3844,8 @@
>
>                         interrupts = <GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>,
>                                      <GIC_SPI 487 IRQ_TYPE_LEVEL_HIGH>,
> -                                    <GIC_SPI 490 IRQ_TYPE_LEVEL_HIGH>,
> -                                    <GIC_SPI 491 IRQ_TYPE_LEVEL_HIGH>;
> +                                    <GIC_SPI 490 IRQ_TYPE_EDGE_BOTH>,
> +                                    <GIC_SPI 491 IRQ_TYPE_EDGE_BOTH>;
>                         interrupt-names = "hs_phy_irq", "ss_phy_irq",
>                                           "dm_hs_phy_irq", 
> "dp_hs_phy_irq";
>
> Regards
>
> Sandeep

That does allow it to boot, however.... it breaks USB.

[    2.013325] genirq: Setting trigger mode 3 for irq 35 failed 
(gic_set_type+0x0/0x1b0)
[    2.014063] dwc3-qcom a6f8800.usb: dp_hs_phy_irq failed: -22
[    2.014134] dwc3-qcom a6f8800.usb: failed to setup IRQs, err=-22
[    2.014351] dwc3-qcom: probe of a6f8800.usb failed with error -22
[    2.018496] genirq: Setting trigger mode 3 for irq 39 failed 
(gic_set_type+0x0/0x1b0)
[    2.019124] dwc3-qcom a8f8800.usb: dp_hs_phy_irq failed: -22
[    2.019193] dwc3-qcom a8f8800.usb: failed to setup IRQs, err=-22
[    2.019372] dwc3-qcom: probe of a8f8800.usb failed with error -22

steev@limitless:~$ lsusb
steev@limitless:~$


-- steev

Re: [PATCH v10 6/6] usb: dwc3: qcom: Enable the interrupts during probe

[Steev Klimaszewski]

Hi Sandeep,

On 2/17/22 12:05 AM, Sandeep Maheswaram wrote:
>
> Hi Steev,
>
> On 2/16/2022 3:21 PM, Steev Klimaszewski wrote:
>> That does allow it to boot, however.... it breaks USB.
>>
>> [    2.013325] genirq: Setting trigger mode 3 for irq 35 failed 
>> (gic_set_type+0x0/0x1b0)
>> [    2.014063] dwc3-qcom a6f8800.usb: dp_hs_phy_irq failed: -22
>> [    2.014134] dwc3-qcom a6f8800.usb: failed to setup IRQs, err=-22
>> [    2.014351] dwc3-qcom: probe of a6f8800.usb failed with error -22
>> [    2.018496] genirq: Setting trigger mode 3 for irq 39 failed 
>> (gic_set_type+0x0/0x1b0)
>> [    2.019124] dwc3-qcom a8f8800.usb: dp_hs_phy_irq failed: -22
>> [    2.019193] dwc3-qcom a8f8800.usb: failed to setup IRQs, err=-22
>> [    2.019372] dwc3-qcom: probe of a8f8800.usb failed with error -22
>>
>> steev@limitless:~$ lsusb
>> steev@limitless:~$
>>
>>
>> -- steev
>>
> Can you try with only IRQ_TYPE_EDGE_RISING as you are using GIC 
> interrupts  where IRQ_TYPE_EDGE_FALLING may not be supported
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi 
> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 0d6286d..ee3b031 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -3796,8 +3796,8 @@
>
>                         interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
>                                      <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>,
> -                                    <GIC_SPI 488 IRQ_TYPE_LEVEL_HIGH>,
> -                                    <GIC_SPI 489 IRQ_TYPE_LEVEL_HIGH>;
> +                                    <GIC_SPI 488 IRQ_TYPE_EDGE_RISING>,
> +                                    <GIC_SPI 489 IRQ_TYPE_EDGE_RISING>;
>                         interrupt-names = "hs_phy_irq", "ss_phy_irq",
>                                           "dm_hs_phy_irq", 
> "dp_hs_phy_irq";
>
> @@ -3844,8 +3844,8 @@
>
>                         interrupts = <GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>,
>                                      <GIC_SPI 487 IRQ_TYPE_LEVEL_HIGH>,
> -                                    <GIC_SPI 490 IRQ_TYPE_LEVEL_HIGH>,
> -                                    <GIC_SPI 491 IRQ_TYPE_LEVEL_HIGH>;
> +                                    <GIC_SPI 490 IRQ_TYPE_EDGE_RISING>,
> +                                    <GIC_SPI 491 IRQ_TYPE_EDGE_RISING>;
>                         interrupt-names = "hs_phy_irq", "ss_phy_irq",
>                                           "dm_hs_phy_irq", 
> "dp_hs_phy_irq";
> Regards
>
> Sandeep
>
With this change, and with either EDGE_RISING or EDGE_BOTH in the lenovo 
yoga c630 dts, it does indeed boot.  Leaving LEVEL_HIGH in the c630, it 
does also boot, but there is a delay of about 30 seconds (I'm assuming 
interrupt storm?) during the boot.

-- steev