Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron
@ 2019-04-18  0:13 Douglas Anderson
  2019-04-18  0:13 ` [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE Doug Anderson
                   ` (5 more replies)
  0 siblings, 6 replies; 50+ messages in thread
From: Douglas Anderson @ 2019-04-18  0:13 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, linux-arm-kernel, Kai-Heng Feng, Mathias Nyman,
	devicetree, linux-kernel, Nicolas Boichat, Jon Flatley,
	Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	Nicolas Saenz Julienne

This re-hashes two older series I posted a long time ago, re-basing
them to mainline.  ...well, technically, atop another dwc2 series I
recently posted:

* usb: dwc2: Another attempt handling rk3288's remote wake quirk
  https://lkml.kernel.org/r/20190416215351.242246-1-dianders@chromium.org
  https://lore.kernel.org/patchwork/cover/1062972/

In general I've tried to add links to each patch pointing to relevant
older discussion.  Here are overall links to the cover letters though.
Note that for the previous "allow wakeup" series the discussion was
scattered a bit between the original post and the repost.

* usb: dwc2: bus suspend/resume that's not hibernate
  https://lkml.kernel.org/r/1446237173-15263-1-git-send-email-dianders@chromium.org
  https://lore.kernel.org/patchwork/patch/613761/

* dwc2 patches to allow wakeup on Rockchip rk3288
  https://lkml.kernel.org/r/1435017144-2971-1-git-send-email-dianders@chromium.org
  https://lore.kernel.org/patchwork/cover/572944/

* dwc2 patches to allow wakeup on Rockchip rk3288 (REPOST)
  https://lkml.kernel.org/r/1436207224-21849-1-git-send-email-dianders@chromium.org
  https://lore.kernel.org/patchwork/cover/576120/

I'm hoping there's a better chance of these things landing this time
around, but I guess we'll see.  ;-)

In case it's helpful I've put what I tested (which is based on Heiko's
for-next branch and includes patches to enable deep suspend plus two
other s2r fixes) at:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+log/refs/sandbox/dianders/190417-testing-dwc2-wakeup

Changes in v2:
- Rebased to mainline atop rk3288 remote wake quirk series.
- rk3288-veyron dts patch new for v2.

Douglas Anderson (5):
  usb: dwc2: bus suspend/resume for hosts with
    DWC2_POWER_DOWN_PARAM_NONE
  USB: Export usb_wakeup_enabled_descendants()
  Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
  USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
  ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports

 .../devicetree/bindings/usb/dwc2.txt          |  3 +
 arch/arm/boot/dts/rk3288-veyron.dtsi          |  2 +
 drivers/usb/core/hub.c                        |  7 +-
 drivers/usb/dwc2/core.h                       |  5 ++
 drivers/usb/dwc2/hcd.c                        | 84 ++++++++++++-------
 drivers/usb/dwc2/platform.c                   | 43 +++++++++-
 include/linux/usb/hcd.h                       |  5 ++
 7 files changed, 113 insertions(+), 36 deletions(-)

-- 
2.21.0.593.g511ec345e18-goog


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
@ 2019-04-18  0:13 ` Doug Anderson
  2019-04-18  0:13   ` [PATCH v2 1/5] " Douglas Anderson
                     ` (2 more replies)
  0 siblings, 3 replies; 50+ messages in thread
From: Doug Anderson @ 2019-04-18  0:13 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, Greg Kroah-Hartman, linux-kernel

This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
suspend/resume for dwc2") on ToT.  That commit was reverted in commit
b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
because apparently it broke the Altera SOCFPGA.

With all the changes that have happened to dwc2 in the meantime, it's
possible that the Altera SOCFPGA will just magically work with this
change now.  ...and it would be good to get bus suspend/resume
implemented.

This change is a forward port of one that's been living in the Chrome
OS 3.14 kernel tree.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This patch was last posted at:

https://lkml.kernel.org/r/1446237173-15263-1-git-send-email-dianders@chromium.org

...and appears to have died the death of silence.  Maybe it could get
some bake time in linuxnext if we can't find any proactive testing?

I will also freely admit that I don't know tons about the theory
behind this patch.  I'm mostly just re-hashing the original commit
from Kever that was reverted since:
* Turning on partial power down on rk3288 doesn't "just work".  I
  don't get hotplug events.  This is despite dwc2 auto-detecting that
  we are power optimized.
* If we don't do something like this commit we don't get into as low
  of a power mode.

Changes in v2: None

 drivers/usb/dwc2/hcd.c | 84 ++++++++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index e272d020012e..978232a9e4a8 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4482,6 +4482,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 	unsigned long flags;
 	int ret = 0;
 	u32 hprt0;
+	u32 pcgctl;
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 
@@ -4497,7 +4498,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 	if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
 		goto unlock;
 
-	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
+	if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL)
 		goto skip_power_saving;
 
 	/*
@@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 	 */
 	if (!hsotg->bus_suspended) {
 		hprt0 = dwc2_read_hprt0(hsotg);
-		hprt0 |= HPRT0_SUSP;
-		hprt0 &= ~HPRT0_PWR;
-		dwc2_writel(hsotg, hprt0, HPRT0);
-		spin_unlock_irqrestore(&hsotg->lock, flags);
-		dwc2_vbus_supply_exit(hsotg);
-		spin_lock_irqsave(&hsotg->lock, flags);
+		if (hprt0 & HPRT0_CONNSTS) {
+			hprt0 |= HPRT0_SUSP;
+			if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL)
+				hprt0 &= ~HPRT0_PWR;
+			dwc2_writel(hsotg, hprt0, HPRT0);
+		}
+		if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
+			spin_unlock_irqrestore(&hsotg->lock, flags);
+			dwc2_vbus_supply_exit(hsotg);
+			spin_lock_irqsave(&hsotg->lock, flags);
+		} else {
+			pcgctl = readl(hsotg->regs + PCGCTL);
+			pcgctl |= PCGCTL_STOPPCLK;
+			writel(pcgctl, hsotg->regs + PCGCTL);
+		}
 	}
 
-	/* Enter partial_power_down */
-	ret = dwc2_enter_partial_power_down(hsotg);
-	if (ret) {
-		if (ret != -ENOTSUPP)
-			dev_err(hsotg->dev,
-				"enter partial_power_down failed\n");
-		goto skip_power_saving;
+	if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
+		/* Enter partial_power_down */
+		ret = dwc2_enter_partial_power_down(hsotg);
+		if (ret) {
+			if (ret != -ENOTSUPP)
+				dev_err(hsotg->dev,
+					"enter partial_power_down failed\n");
+			goto skip_power_saving;
+		}
+
+		/* After entering partial_power_down, hardware is no more accessible */
+		clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 	}
 
 	/* Ask phy to be suspended */
@@ -4530,9 +4545,6 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 		spin_lock_irqsave(&hsotg->lock, flags);
 	}
 
-	/* After entering partial_power_down, hardware is no more accessible */
-	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
-
 skip_power_saving:
 	hsotg->lx_state = DWC2_L2;
 unlock:
@@ -4545,6 +4557,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 {
 	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
 	unsigned long flags;
+	u32 pcgctl;
 	int ret = 0;
 
 	spin_lock_irqsave(&hsotg->lock, flags);
@@ -4555,17 +4568,11 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 	if (hsotg->lx_state != DWC2_L2)
 		goto unlock;
 
-	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) {
+	if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL) {
 		hsotg->lx_state = DWC2_L0;
 		goto unlock;
 	}
 
-	/*
-	 * Set HW accessible bit before powering on the controller
-	 * since an interrupt may rise.
-	 */
-	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
-
 	/*
 	 * Enable power if not already done.
 	 * This must not be spinlocked since duration
@@ -4577,10 +4584,23 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 		spin_lock_irqsave(&hsotg->lock, flags);
 	}
 
-	/* Exit partial_power_down */
-	ret = dwc2_exit_partial_power_down(hsotg, true);
-	if (ret && (ret != -ENOTSUPP))
-		dev_err(hsotg->dev, "exit partial_power_down failed\n");
+	if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
+		/*
+		 * Set HW accessible bit before powering on the controller
+		 * since an interrupt may rise.
+		 */
+		set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+
+
+		/* Exit partial_power_down */
+		ret = dwc2_exit_partial_power_down(hsotg, true);
+		if (ret && (ret != -ENOTSUPP))
+			dev_err(hsotg->dev, "exit partial_power_down failed\n");
+	} else {
+		pcgctl = readl(hsotg->regs + PCGCTL);
+		pcgctl &= ~PCGCTL_STOPPCLK;
+		writel(pcgctl, hsotg->regs + PCGCTL);
+	}
 
 	hsotg->lx_state = DWC2_L0;
 
@@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 		spin_unlock_irqrestore(&hsotg->lock, flags);
 		dwc2_port_resume(hsotg);
 	} else {
-		dwc2_vbus_supply_init(hsotg);
+		if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
+			dwc2_vbus_supply_init(hsotg);
 
-		/* Wait for controller to correctly update D+/D- level */
-		usleep_range(3000, 5000);
+			/* Wait for controller to correctly update D+/D- level */
+			usleep_range(3000, 5000);
+		}
 
 		/*
 		 * Clear Port Enable and Port Status changes.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
  2019-04-18  0:13 ` [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE Doug Anderson
@ 2019-04-18  0:13   ` " Douglas Anderson
  2019-04-29  8:43   ` [v2,1/5] " Artur Petrosyan
  2019-05-01 23:58   ` [v2,1/5] " Doug Anderson
  2 siblings, 0 replies; 50+ messages in thread
From: Douglas Anderson @ 2019-04-18  0:13 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, Greg Kroah-Hartman, linux-kernel

This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
suspend/resume for dwc2") on ToT.  That commit was reverted in commit
b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
because apparently it broke the Altera SOCFPGA.

With all the changes that have happened to dwc2 in the meantime, it's
possible that the Altera SOCFPGA will just magically work with this
change now.  ...and it would be good to get bus suspend/resume
implemented.

This change is a forward port of one that's been living in the Chrome
OS 3.14 kernel tree.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This patch was last posted at:

https://lkml.kernel.org/r/1446237173-15263-1-git-send-email-dianders@chromium.org

...and appears to have died the death of silence.  Maybe it could get
some bake time in linuxnext if we can't find any proactive testing?

I will also freely admit that I don't know tons about the theory
behind this patch.  I'm mostly just re-hashing the original commit
from Kever that was reverted since:
* Turning on partial power down on rk3288 doesn't "just work".  I
  don't get hotplug events.  This is despite dwc2 auto-detecting that
  we are power optimized.
* If we don't do something like this commit we don't get into as low
  of a power mode.

Changes in v2: None

 drivers/usb/dwc2/hcd.c | 84 ++++++++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index e272d020012e..978232a9e4a8 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4482,6 +4482,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 	unsigned long flags;
 	int ret = 0;
 	u32 hprt0;
+	u32 pcgctl;
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 
@@ -4497,7 +4498,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 	if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
 		goto unlock;
 
-	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
+	if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL)
 		goto skip_power_saving;
 
 	/*
@@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 	 */
 	if (!hsotg->bus_suspended) {
 		hprt0 = dwc2_read_hprt0(hsotg);
-		hprt0 |= HPRT0_SUSP;
-		hprt0 &= ~HPRT0_PWR;
-		dwc2_writel(hsotg, hprt0, HPRT0);
-		spin_unlock_irqrestore(&hsotg->lock, flags);
-		dwc2_vbus_supply_exit(hsotg);
-		spin_lock_irqsave(&hsotg->lock, flags);
+		if (hprt0 & HPRT0_CONNSTS) {
+			hprt0 |= HPRT0_SUSP;
+			if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL)
+				hprt0 &= ~HPRT0_PWR;
+			dwc2_writel(hsotg, hprt0, HPRT0);
+		}
+		if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
+			spin_unlock_irqrestore(&hsotg->lock, flags);
+			dwc2_vbus_supply_exit(hsotg);
+			spin_lock_irqsave(&hsotg->lock, flags);
+		} else {
+			pcgctl = readl(hsotg->regs + PCGCTL);
+			pcgctl |= PCGCTL_STOPPCLK;
+			writel(pcgctl, hsotg->regs + PCGCTL);
+		}
 	}
 
-	/* Enter partial_power_down */
-	ret = dwc2_enter_partial_power_down(hsotg);
-	if (ret) {
-		if (ret != -ENOTSUPP)
-			dev_err(hsotg->dev,
-				"enter partial_power_down failed\n");
-		goto skip_power_saving;
+	if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
+		/* Enter partial_power_down */
+		ret = dwc2_enter_partial_power_down(hsotg);
+		if (ret) {
+			if (ret != -ENOTSUPP)
+				dev_err(hsotg->dev,
+					"enter partial_power_down failed\n");
+			goto skip_power_saving;
+		}
+
+		/* After entering partial_power_down, hardware is no more accessible */
+		clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 	}
 
 	/* Ask phy to be suspended */
@@ -4530,9 +4545,6 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 		spin_lock_irqsave(&hsotg->lock, flags);
 	}
 
-	/* After entering partial_power_down, hardware is no more accessible */
-	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
-
 skip_power_saving:
 	hsotg->lx_state = DWC2_L2;
 unlock:
@@ -4545,6 +4557,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 {
 	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
 	unsigned long flags;
+	u32 pcgctl;
 	int ret = 0;
 
 	spin_lock_irqsave(&hsotg->lock, flags);
@@ -4555,17 +4568,11 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 	if (hsotg->lx_state != DWC2_L2)
 		goto unlock;
 
-	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) {
+	if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL) {
 		hsotg->lx_state = DWC2_L0;
 		goto unlock;
 	}
 
-	/*
-	 * Set HW accessible bit before powering on the controller
-	 * since an interrupt may rise.
-	 */
-	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
-
 	/*
 	 * Enable power if not already done.
 	 * This must not be spinlocked since duration
@@ -4577,10 +4584,23 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 		spin_lock_irqsave(&hsotg->lock, flags);
 	}
 
-	/* Exit partial_power_down */
-	ret = dwc2_exit_partial_power_down(hsotg, true);
-	if (ret && (ret != -ENOTSUPP))
-		dev_err(hsotg->dev, "exit partial_power_down failed\n");
+	if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
+		/*
+		 * Set HW accessible bit before powering on the controller
+		 * since an interrupt may rise.
+		 */
+		set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+
+
+		/* Exit partial_power_down */
+		ret = dwc2_exit_partial_power_down(hsotg, true);
+		if (ret && (ret != -ENOTSUPP))
+			dev_err(hsotg->dev, "exit partial_power_down failed\n");
+	} else {
+		pcgctl = readl(hsotg->regs + PCGCTL);
+		pcgctl &= ~PCGCTL_STOPPCLK;
+		writel(pcgctl, hsotg->regs + PCGCTL);
+	}
 
 	hsotg->lx_state = DWC2_L0;
 
@@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 		spin_unlock_irqrestore(&hsotg->lock, flags);
 		dwc2_port_resume(hsotg);
 	} else {
-		dwc2_vbus_supply_init(hsotg);
+		if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
+			dwc2_vbus_supply_init(hsotg);
 
-		/* Wait for controller to correctly update D+/D- level */
-		usleep_range(3000, 5000);
+			/* Wait for controller to correctly update D+/D- level */
+			usleep_range(3000, 5000);
+		}
 
 		/*
 		 * Clear Port Enable and Port Status changes.
-- 
2.21.0.593.g511ec345e18-goog


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,2/5] USB: Export usb_wakeup_enabled_descendants()
@ 2019-04-18  0:13 ` Doug Anderson
  2019-04-18  0:13   ` [PATCH v2 2/5] " Douglas Anderson
  0 siblings, 1 reply; 50+ messages in thread
From: Doug Anderson @ 2019-04-18  0:13 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, Kai-Heng Feng, Nicolas Saenz Julienne,
	linux-kernel, Nicolas Boichat, Jon Flatley, Greg Kroah-Hartman,
	Mathias Nyman

In (e583d9d USB: global suspend and remote wakeup don't mix) we
introduced wakeup_enabled_descendants() as a static function.  We'd
like to use this function in USB controller drivers to know if we
should keep the controller on during suspend time, since doing so has
a power impact.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
For relevant prior discussion of this idea, see:

https://lkml.kernel.org/r/1436207224-21849-4-git-send-email-dianders@chromium.org

If I'm reading all the responses correctly folks were of the opinion
that this patch is still the right way to go.

Changes in v2: None

 drivers/usb/core/hub.c  | 7 ++++---
 include/linux/usb/hcd.h | 5 +++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 8d4631c81b9f..5e8f3fa7ae5a 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3174,13 +3174,14 @@ static int usb_disable_remote_wakeup(struct usb_device *udev)
 }
 
 /* Count of wakeup-enabled devices at or below udev */
-static unsigned wakeup_enabled_descendants(struct usb_device *udev)
+unsigned usb_wakeup_enabled_descendants(struct usb_device *udev)
 {
 	struct usb_hub *hub = usb_hub_to_struct_hub(udev);
 
 	return udev->do_remote_wakeup +
 			(hub ? hub->wakeup_enabled_descendants : 0);
 }
+EXPORT_SYMBOL_GPL(usb_wakeup_enabled_descendants);
 
 /*
  * usb_port_suspend - suspend a usb device's upstream port
@@ -3282,7 +3283,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 	 * Therefore we will turn on the suspend feature if udev or any of its
 	 * descendants is enabled for remote wakeup.
 	 */
-	else if (PMSG_IS_AUTO(msg) || wakeup_enabled_descendants(udev) > 0)
+	else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
 		status = set_port_feature(hub->hdev, port1,
 				USB_PORT_FEAT_SUSPEND);
 	else {
@@ -3687,7 +3688,7 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg)
 		}
 		if (udev)
 			hub->wakeup_enabled_descendants +=
-					wakeup_enabled_descendants(udev);
+					usb_wakeup_enabled_descendants(udev);
 	}
 
 	if (hdev->do_remote_wakeup && hub->quirk_check_port_auto_suspend) {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 695931b03684..ed4fbbd1b35f 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -652,11 +652,16 @@ extern wait_queue_head_t usb_kill_urb_queue;
 #define usb_endpoint_out(ep_dir)	(!((ep_dir) & USB_DIR_IN))
 
 #ifdef CONFIG_PM
+extern unsigned usb_wakeup_enabled_descendants(struct usb_device *udev);
 extern void usb_root_hub_lost_power(struct usb_device *rhdev);
 extern int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg);
 extern int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg);
 extern void usb_hcd_resume_root_hub(struct usb_hcd *hcd);
 #else
+static inline unsigned usb_wakeup_enabled_descendants(struct usb_device *udev)
+{
+	return 0;
+}
 static inline void usb_hcd_resume_root_hub(struct usb_hcd *hcd)
 {
 	return;

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH v2 2/5] USB: Export usb_wakeup_enabled_descendants()
  2019-04-18  0:13 ` [v2,2/5] USB: Export usb_wakeup_enabled_descendants() Doug Anderson
@ 2019-04-18  0:13   ` " Douglas Anderson
  0 siblings, 0 replies; 50+ messages in thread
From: Douglas Anderson @ 2019-04-18  0:13 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, Kai-Heng Feng, Nicolas Saenz Julienne,
	linux-kernel, Nicolas Boichat, Jon Flatley, Greg Kroah-Hartman,
	Mathias Nyman

In (e583d9d USB: global suspend and remote wakeup don't mix) we
introduced wakeup_enabled_descendants() as a static function.  We'd
like to use this function in USB controller drivers to know if we
should keep the controller on during suspend time, since doing so has
a power impact.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
For relevant prior discussion of this idea, see:

https://lkml.kernel.org/r/1436207224-21849-4-git-send-email-dianders@chromium.org

If I'm reading all the responses correctly folks were of the opinion
that this patch is still the right way to go.

Changes in v2: None

 drivers/usb/core/hub.c  | 7 ++++---
 include/linux/usb/hcd.h | 5 +++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 8d4631c81b9f..5e8f3fa7ae5a 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3174,13 +3174,14 @@ static int usb_disable_remote_wakeup(struct usb_device *udev)
 }
 
 /* Count of wakeup-enabled devices at or below udev */
-static unsigned wakeup_enabled_descendants(struct usb_device *udev)
+unsigned usb_wakeup_enabled_descendants(struct usb_device *udev)
 {
 	struct usb_hub *hub = usb_hub_to_struct_hub(udev);
 
 	return udev->do_remote_wakeup +
 			(hub ? hub->wakeup_enabled_descendants : 0);
 }
+EXPORT_SYMBOL_GPL(usb_wakeup_enabled_descendants);
 
 /*
  * usb_port_suspend - suspend a usb device's upstream port
@@ -3282,7 +3283,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 	 * Therefore we will turn on the suspend feature if udev or any of its
 	 * descendants is enabled for remote wakeup.
 	 */
-	else if (PMSG_IS_AUTO(msg) || wakeup_enabled_descendants(udev) > 0)
+	else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
 		status = set_port_feature(hub->hdev, port1,
 				USB_PORT_FEAT_SUSPEND);
 	else {
@@ -3687,7 +3688,7 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg)
 		}
 		if (udev)
 			hub->wakeup_enabled_descendants +=
-					wakeup_enabled_descendants(udev);
+					usb_wakeup_enabled_descendants(udev);
 	}
 
 	if (hdev->do_remote_wakeup && hub->quirk_check_port_auto_suspend) {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 695931b03684..ed4fbbd1b35f 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -652,11 +652,16 @@ extern wait_queue_head_t usb_kill_urb_queue;
 #define usb_endpoint_out(ep_dir)	(!((ep_dir) & USB_DIR_IN))
 
 #ifdef CONFIG_PM
+extern unsigned usb_wakeup_enabled_descendants(struct usb_device *udev);
 extern void usb_root_hub_lost_power(struct usb_device *rhdev);
 extern int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg);
 extern int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg);
 extern void usb_hcd_resume_root_hub(struct usb_hcd *hcd);
 #else
+static inline unsigned usb_wakeup_enabled_descendants(struct usb_device *udev)
+{
+	return 0;
+}
 static inline void usb_hcd_resume_root_hub(struct usb_hcd *hcd)
 {
 	return;
-- 
2.21.0.593.g511ec345e18-goog


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
@ 2019-04-18  0:13 ` Doug Anderson
  2019-04-18  0:13   ` [PATCH v2 3/5] " Douglas Anderson
                     ` (2 more replies)
  0 siblings, 3 replies; 50+ messages in thread
From: Doug Anderson @ 2019-04-18  0:13 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, devicetree, linux-kernel, Rob Herring,
	Greg Kroah-Hartman, Mark Rutland

Some SoCs with a dwc2 USB controller may need to keep the PHY on to
support remote wakeup.  Allow specifying this as a device tree
property.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
For relevant prior discussion on this patch, see:

https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org

I didn't make any changes from the prior version since I never found
out what Rob thought of my previous arguments.  If folks want a
change, perhaps they could choose from these options:

1. Assume that all dwc2 hosts would like to keep their PHY on for
   suspend if there's a USB wakeup enabled, thus we totally drop this
   binding.  This doesn't seem super great to me since I'd bet that
   many devices that use dwc2 weren't designed for USB wakeup (they
   may not keep enough clocks or rails on) so we might be wasting
   power for nothing.
2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
   it more obvious that this property is intended both to document
   that wakeup from suspend is possible and that we need the PHY for
   said wakeup.
3. Rename this property to "snps,can-wakeup-from-suspend" and assume
   it's implicit that if we can wakeup from suspend that we need to
   keep the PHY on.  If/when someone shows that a device exists using
   dwc2 where we can wakeup from suspend without the PHY they can add
   a new property.

Changes in v2: None

 Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
index f70f3aee4bfc..1c5e29d23c51 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -37,6 +37,8 @@ Refer to phy/phy-bindings.txt for generic phy consumer properties
 - g-rx-fifo-size: size of rx fifo size in gadget mode.
 - g-np-tx-fifo-size: size of non-periodic tx fifo size in gadget mode.
 - g-tx-fifo-size: size of periodic tx fifo per endpoint (except ep0) in gadget mode.
+- snps,need-phy-for-wake: If present indicates that the phy needs to be left
+                          on for remote wakeup during suspend.
 - snps,reset-phy-on-wake: If present indicates that we need to reset the PHY when
                           we detect a wakeup.  This is due to a hardware errata.
 
@@ -53,4 +55,5 @@ Example:
 		clock-names = "otg";
 		phys = <&usbphy>;
 		phy-names = "usb2-phy";
+		snps,need-phy-for-wake;
         };

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
  2019-04-18  0:13 ` [v2,3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB Doug Anderson
@ 2019-04-18  0:13   ` " Douglas Anderson
  2019-04-25 12:40   ` [v2,3/5] " Felipe Balbi
  2019-04-30  1:23   ` [v2,3/5] " Rob Herring
  2 siblings, 0 replies; 50+ messages in thread
From: Douglas Anderson @ 2019-04-18  0:13 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, devicetree, linux-kernel, Rob Herring,
	Greg Kroah-Hartman, Mark Rutland

Some SoCs with a dwc2 USB controller may need to keep the PHY on to
support remote wakeup.  Allow specifying this as a device tree
property.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
For relevant prior discussion on this patch, see:

https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org

I didn't make any changes from the prior version since I never found
out what Rob thought of my previous arguments.  If folks want a
change, perhaps they could choose from these options:

1. Assume that all dwc2 hosts would like to keep their PHY on for
   suspend if there's a USB wakeup enabled, thus we totally drop this
   binding.  This doesn't seem super great to me since I'd bet that
   many devices that use dwc2 weren't designed for USB wakeup (they
   may not keep enough clocks or rails on) so we might be wasting
   power for nothing.
2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
   it more obvious that this property is intended both to document
   that wakeup from suspend is possible and that we need the PHY for
   said wakeup.
3. Rename this property to "snps,can-wakeup-from-suspend" and assume
   it's implicit that if we can wakeup from suspend that we need to
   keep the PHY on.  If/when someone shows that a device exists using
   dwc2 where we can wakeup from suspend without the PHY they can add
   a new property.

Changes in v2: None

 Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
index f70f3aee4bfc..1c5e29d23c51 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -37,6 +37,8 @@ Refer to phy/phy-bindings.txt for generic phy consumer properties
 - g-rx-fifo-size: size of rx fifo size in gadget mode.
 - g-np-tx-fifo-size: size of non-periodic tx fifo size in gadget mode.
 - g-tx-fifo-size: size of periodic tx fifo per endpoint (except ep0) in gadget mode.
+- snps,need-phy-for-wake: If present indicates that the phy needs to be left
+                          on for remote wakeup during suspend.
 - snps,reset-phy-on-wake: If present indicates that we need to reset the PHY when
                           we detect a wakeup.  This is due to a hardware errata.
 
@@ -53,4 +55,5 @@ Example:
 		clock-names = "otg";
 		phys = <&usbphy>;
 		phy-names = "usb2-phy";
+		snps,need-phy-for-wake;
         };
-- 
2.21.0.593.g511ec345e18-goog


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,4/5] USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
@ 2019-04-18  0:13 ` Doug Anderson
  2019-04-18  0:13   ` [PATCH v2 4/5] " Douglas Anderson
  2019-04-25 12:41   ` [v2,4/5] " Felipe Balbi
  0 siblings, 2 replies; 50+ messages in thread
From: Doug Anderson @ 2019-04-18  0:13 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, Greg Kroah-Hartman, linux-kernel

If the 'snps,need-phy-for-wake' is set in the device tree then:

- We know that we can wakeup, so call device_set_wakeup_capable().
  The USB core will use this knowledge to enable wakeup by default.
- We know that we should keep the PHY on during suspend if something
  on our root hub needs remote wakeup.  This requires the patch (USB:
  Export usb_wakeup_enabled_descendants()).  Note that we don't keep
  the PHY on at suspend time if it's not needed because it would be a
  power draw.

If we later find some users of dwc2 that can support wakeup without
keeping the PHY on we may want to add a way to call
device_set_wakeup_capable() without keeping the PHY on at suspend
time.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Chris Zhong <zyw@rock-chips.com>
---
For relevant prior discussion of this idea, see:

https://lkml.kernel.org/r/1436207224-21849-4-git-send-email-dianders@chromium.org

If I'm reading all the responses correctly folks were of the opinion
that this patch is still the right way to go.

Changes in v2:
- Rebased to mainline atop rk3288 remote wake quirk series.

 drivers/usb/dwc2/core.h     |  5 +++++
 drivers/usb/dwc2/platform.c | 43 +++++++++++++++++++++++++++++++++++--
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index f30748f4fe7e..a99fe7851352 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -861,6 +861,9 @@ struct dwc2_hregs_backup {
  * @hibernated:		True if core is hibernated
  * @reset_phy_on_wake:	Quirk saying that we should assert PHY reset on a
  *			remote wakeup.
+ * @phy_off_for_suspend: Status of whether we turned the PHY off at suspend.
+ * @need_phy_for_wake:	Quirk saying that we should keep the PHY on at
+ *			suspend if we need USB to wake us up.
  * @frame_number:       Frame number read from the core. For both device
  *			and host modes. The value ranges are from 0
  *			to HFNUM_MAX_FRNUM.
@@ -1049,6 +1052,8 @@ struct dwc2_hsotg {
 	unsigned int ll_hw_enabled:1;
 	unsigned int hibernated:1;
 	unsigned int reset_phy_on_wake:1;
+	unsigned int need_phy_for_wake:1;
+	unsigned int phy_off_for_suspend:1;
 	u16 frame_number;
 
 	struct phy *phy;
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 9b65b766e0b9..6f0271c5d163 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -47,7 +47,9 @@
 #include <linux/phy/phy.h>
 #include <linux/platform_data/s3c-hsotg.h>
 #include <linux/reset.h>
+#include <linux/usb.h>
 
+#include <linux/usb/hcd.h>
 #include <linux/usb/of.h>
 
 #include "core.h"
@@ -450,6 +452,10 @@ static int dwc2_driver_probe(struct platform_device *dev)
 	if (retval)
 		goto error;
 
+	hsotg->need_phy_for_wake =
+		of_property_read_bool(dev->dev.of_node,
+				      "snps,need-phy-for-wake");
+
 	/*
 	 * Reset before dwc2_get_hwparams() then it could get power-on real
 	 * reset value form registers.
@@ -481,6 +487,14 @@ static int dwc2_driver_probe(struct platform_device *dev)
 		hsotg->gadget_enabled = 1;
 	}
 
+	/*
+	 * If we need PHY for wakeup we must be wakeup capable.
+	 * When we have a device that can wake without the PHY we
+	 * can adjust this condition.
+	 */
+	if (hsotg->need_phy_for_wake)
+		device_set_wakeup_capable(&dev->dev, true);
+
 	hsotg->reset_phy_on_wake =
 		of_property_read_bool(dev->dev.of_node,
 				      "snps,reset-phy-on-wake");
@@ -516,6 +530,28 @@ static int dwc2_driver_probe(struct platform_device *dev)
 	return retval;
 }
 
+static bool __maybe_unused dwc2_can_poweroff_phy(struct dwc2_hsotg *dwc2)
+{
+	struct usb_device *root_hub = dwc2_hsotg_to_hcd(dwc2)->self.root_hub;
+
+	if (!dwc2->ll_hw_enabled)
+		return false;
+
+	/* If the controller isn't allowed to wakeup then we can power off. */
+	if (!device_may_wakeup(dwc2->dev))
+		return true;
+
+	/*
+	 * We don't want to power off the PHY if something under the
+	 * root hub has wakeup enabled.
+	 */
+	if (usb_wakeup_enabled_descendants(root_hub))
+		return false;
+
+	/* No reason to keep the PHY powered, so allow poweroff */
+	return true;
+}
+
 static int __maybe_unused dwc2_suspend(struct device *dev)
 {
 	struct dwc2_hsotg *dwc2 = dev_get_drvdata(dev);
@@ -524,8 +560,10 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
 	if (dwc2_is_device_mode(dwc2))
 		dwc2_hsotg_suspend(dwc2);
 
-	if (dwc2->ll_hw_enabled)
+	if (dwc2_can_poweroff_phy(dwc2)) {
 		ret = __dwc2_lowlevel_hw_disable(dwc2);
+		dwc2->phy_off_for_suspend = true;
+	}
 
 	return ret;
 }
@@ -535,11 +573,12 @@ static int __maybe_unused dwc2_resume(struct device *dev)
 	struct dwc2_hsotg *dwc2 = dev_get_drvdata(dev);
 	int ret = 0;
 
-	if (dwc2->ll_hw_enabled) {
+	if (dwc2->phy_off_for_suspend && dwc2->ll_hw_enabled) {
 		ret = __dwc2_lowlevel_hw_enable(dwc2);
 		if (ret)
 			return ret;
 	}
+	dwc2->phy_off_for_suspend = false;
 
 	if (dwc2_is_device_mode(dwc2))
 		ret = dwc2_hsotg_resume(dwc2);

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH v2 4/5] USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
  2019-04-18  0:13 ` [v2,4/5] USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled Doug Anderson
@ 2019-04-18  0:13   ` " Douglas Anderson
  2019-04-25 12:41   ` [v2,4/5] " Felipe Balbi
  1 sibling, 0 replies; 50+ messages in thread
From: Douglas Anderson @ 2019-04-18  0:13 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, Greg Kroah-Hartman, linux-kernel

If the 'snps,need-phy-for-wake' is set in the device tree then:

- We know that we can wakeup, so call device_set_wakeup_capable().
  The USB core will use this knowledge to enable wakeup by default.
- We know that we should keep the PHY on during suspend if something
  on our root hub needs remote wakeup.  This requires the patch (USB:
  Export usb_wakeup_enabled_descendants()).  Note that we don't keep
  the PHY on at suspend time if it's not needed because it would be a
  power draw.

If we later find some users of dwc2 that can support wakeup without
keeping the PHY on we may want to add a way to call
device_set_wakeup_capable() without keeping the PHY on at suspend
time.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Chris Zhong <zyw@rock-chips.com>
---
For relevant prior discussion of this idea, see:

https://lkml.kernel.org/r/1436207224-21849-4-git-send-email-dianders@chromium.org

If I'm reading all the responses correctly folks were of the opinion
that this patch is still the right way to go.

Changes in v2:
- Rebased to mainline atop rk3288 remote wake quirk series.

 drivers/usb/dwc2/core.h     |  5 +++++
 drivers/usb/dwc2/platform.c | 43 +++++++++++++++++++++++++++++++++++--
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index f30748f4fe7e..a99fe7851352 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -861,6 +861,9 @@ struct dwc2_hregs_backup {
  * @hibernated:		True if core is hibernated
  * @reset_phy_on_wake:	Quirk saying that we should assert PHY reset on a
  *			remote wakeup.
+ * @phy_off_for_suspend: Status of whether we turned the PHY off at suspend.
+ * @need_phy_for_wake:	Quirk saying that we should keep the PHY on at
+ *			suspend if we need USB to wake us up.
  * @frame_number:       Frame number read from the core. For both device
  *			and host modes. The value ranges are from 0
  *			to HFNUM_MAX_FRNUM.
@@ -1049,6 +1052,8 @@ struct dwc2_hsotg {
 	unsigned int ll_hw_enabled:1;
 	unsigned int hibernated:1;
 	unsigned int reset_phy_on_wake:1;
+	unsigned int need_phy_for_wake:1;
+	unsigned int phy_off_for_suspend:1;
 	u16 frame_number;
 
 	struct phy *phy;
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 9b65b766e0b9..6f0271c5d163 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -47,7 +47,9 @@
 #include <linux/phy/phy.h>
 #include <linux/platform_data/s3c-hsotg.h>
 #include <linux/reset.h>
+#include <linux/usb.h>
 
+#include <linux/usb/hcd.h>
 #include <linux/usb/of.h>
 
 #include "core.h"
@@ -450,6 +452,10 @@ static int dwc2_driver_probe(struct platform_device *dev)
 	if (retval)
 		goto error;
 
+	hsotg->need_phy_for_wake =
+		of_property_read_bool(dev->dev.of_node,
+				      "snps,need-phy-for-wake");
+
 	/*
 	 * Reset before dwc2_get_hwparams() then it could get power-on real
 	 * reset value form registers.
@@ -481,6 +487,14 @@ static int dwc2_driver_probe(struct platform_device *dev)
 		hsotg->gadget_enabled = 1;
 	}
 
+	/*
+	 * If we need PHY for wakeup we must be wakeup capable.
+	 * When we have a device that can wake without the PHY we
+	 * can adjust this condition.
+	 */
+	if (hsotg->need_phy_for_wake)
+		device_set_wakeup_capable(&dev->dev, true);
+
 	hsotg->reset_phy_on_wake =
 		of_property_read_bool(dev->dev.of_node,
 				      "snps,reset-phy-on-wake");
@@ -516,6 +530,28 @@ static int dwc2_driver_probe(struct platform_device *dev)
 	return retval;
 }
 
+static bool __maybe_unused dwc2_can_poweroff_phy(struct dwc2_hsotg *dwc2)
+{
+	struct usb_device *root_hub = dwc2_hsotg_to_hcd(dwc2)->self.root_hub;
+
+	if (!dwc2->ll_hw_enabled)
+		return false;
+
+	/* If the controller isn't allowed to wakeup then we can power off. */
+	if (!device_may_wakeup(dwc2->dev))
+		return true;
+
+	/*
+	 * We don't want to power off the PHY if something under the
+	 * root hub has wakeup enabled.
+	 */
+	if (usb_wakeup_enabled_descendants(root_hub))
+		return false;
+
+	/* No reason to keep the PHY powered, so allow poweroff */
+	return true;
+}
+
 static int __maybe_unused dwc2_suspend(struct device *dev)
 {
 	struct dwc2_hsotg *dwc2 = dev_get_drvdata(dev);
@@ -524,8 +560,10 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
 	if (dwc2_is_device_mode(dwc2))
 		dwc2_hsotg_suspend(dwc2);
 
-	if (dwc2->ll_hw_enabled)
+	if (dwc2_can_poweroff_phy(dwc2)) {
 		ret = __dwc2_lowlevel_hw_disable(dwc2);
+		dwc2->phy_off_for_suspend = true;
+	}
 
 	return ret;
 }
@@ -535,11 +573,12 @@ static int __maybe_unused dwc2_resume(struct device *dev)
 	struct dwc2_hsotg *dwc2 = dev_get_drvdata(dev);
 	int ret = 0;
 
-	if (dwc2->ll_hw_enabled) {
+	if (dwc2->phy_off_for_suspend && dwc2->ll_hw_enabled) {
 		ret = __dwc2_lowlevel_hw_enable(dwc2);
 		if (ret)
 			return ret;
 	}
+	dwc2->phy_off_for_suspend = false;
 
 	if (dwc2_is_device_mode(dwc2))
 		ret = dwc2_hsotg_resume(dwc2);
-- 
2.21.0.593.g511ec345e18-goog


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,5/5] ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports
@ 2019-04-18  0:13 ` Doug Anderson
  2019-04-18  0:13   ` [PATCH v2 5/5] " Douglas Anderson
  0 siblings, 1 reply; 50+ messages in thread
From: Doug Anderson @ 2019-04-18  0:13 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, devicetree, linux-kernel, Rob Herring,
	Mark Rutland, linux-arm-kernel

We want to be able to wake from USB if a device is plugged in that
wants remote wakeup.  Enable it on both dwc2 controllers.

NOTE: this is added specifically to veyron and not to rk3288 in
general since it's not known whether all rk3288 boards are designed to
support USB wakeup.  It is plausible that some boards could shut down
important rails in S3.

Also note that currently wakeup doesn't seem to happen unless you use
the "deep" suspend mode (where SDRAM is turned off).  Presumably the
shallow suspend mode is gating some sort of clock that's important but
I couldn't easily figure out how to get it working.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- rk3288-veyron dts patch new for v2.

 arch/arm/boot/dts/rk3288-veyron.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-veyron.dtsi b/arch/arm/boot/dts/rk3288-veyron.dtsi
index 1252522392c7..1d8bfed7830c 100644
--- a/arch/arm/boot/dts/rk3288-veyron.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron.dtsi
@@ -424,6 +424,7 @@
 
 &usb_host1 {
 	status = "okay";
+	snps,need-phy-for-wake;
 };
 
 &usb_otg {
@@ -432,6 +433,7 @@
 	assigned-clocks = <&cru SCLK_USBPHY480M_SRC>;
 	assigned-clock-parents = <&usbphy0>;
 	dr_mode = "host";
+	snps,need-phy-for-wake;
 };
 
 &vopb {

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH v2 5/5] ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports
  2019-04-18  0:13 ` [v2,5/5] ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports Doug Anderson
@ 2019-04-18  0:13   ` " Douglas Anderson
  0 siblings, 0 replies; 50+ messages in thread
From: Douglas Anderson @ 2019-04-18  0:13 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, devicetree, linux-kernel, Rob Herring,
	Mark Rutland, linux-arm-kernel

We want to be able to wake from USB if a device is plugged in that
wants remote wakeup.  Enable it on both dwc2 controllers.

NOTE: this is added specifically to veyron and not to rk3288 in
general since it's not known whether all rk3288 boards are designed to
support USB wakeup.  It is plausible that some boards could shut down
important rails in S3.

Also note that currently wakeup doesn't seem to happen unless you use
the "deep" suspend mode (where SDRAM is turned off).  Presumably the
shallow suspend mode is gating some sort of clock that's important but
I couldn't easily figure out how to get it working.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- rk3288-veyron dts patch new for v2.

 arch/arm/boot/dts/rk3288-veyron.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-veyron.dtsi b/arch/arm/boot/dts/rk3288-veyron.dtsi
index 1252522392c7..1d8bfed7830c 100644
--- a/arch/arm/boot/dts/rk3288-veyron.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron.dtsi
@@ -424,6 +424,7 @@
 
 &usb_host1 {
 	status = "okay";
+	snps,need-phy-for-wake;
 };
 
 &usb_otg {
@@ -432,6 +433,7 @@
 	assigned-clocks = <&cru SCLK_USBPHY480M_SRC>;
 	assigned-clock-parents = <&usbphy0>;
 	dr_mode = "host";
+	snps,need-phy-for-wake;
 };
 
 &vopb {
-- 
2.21.0.593.g511ec345e18-goog


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron
  2019-04-18  0:13 [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron Douglas Anderson
                   ` (4 preceding siblings ...)
  2019-04-18  0:13 ` [v2,5/5] ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports Doug Anderson
@ 2019-04-18 12:40 ` Minas Harutyunyan
  2019-04-18 15:54   ` Doug Anderson
  5 siblings, 1 reply; 50+ messages in thread
From: Minas Harutyunyan @ 2019-04-18 12:40 UTC (permalink / raw)
  To: Douglas Anderson, Felipe Balbi, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	linux-arm-kernel, Kai-Heng Feng, Mathias Nyman, devicetree,
	linux-kernel, Nicolas Boichat, Jon Flatley, Greg Kroah-Hartman,
	Rob Herring, Mark Rutland, Nicolas Saenz Julienne

Hi Douglas,

On 4/18/2019 4:15 AM, Douglas Anderson wrote:
> This re-hashes two older series I posted a long time ago, re-basing
> them to mainline.  ...well, technically, atop another dwc2 series I
> recently posted:
> 
> * usb: dwc2: Another attempt handling rk3288's remote wake quirk
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_20190416215351.242246-2D1-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=b-IYjc3cgFyZYC_B9zSA_xbSLE2ODwAZX0Png-G4SwA&e=
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_patchwork_cover_1062972_&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=VCNBuDGf2VeFFWUUyE3wUdn-sDnUu0nFipeNpyWPXts&e=
> 
> In general I've tried to add links to each patch pointing to relevant
> older discussion.  Here are overall links to the cover letters though.
> Note that for the previous "allow wakeup" series the discussion was
> scattered a bit between the original post and the repost.
> 
> * usb: dwc2: bus suspend/resume that's not hibernate
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=FSVMrPFuSM--uXrAZN9GzCEXP60Li7miMsC4ydv6oDQ&e=
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_patchwork_patch_613761_&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=omRYPw4XVgY8Rq2UgJhApk2poeKXWCBc5QsYMlQkqk4&e=
> 
> * dwc2 patches to allow wakeup on Rockchip rk3288
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1435017144-2D2971-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=ep7GoHZcPlQbiOkXlwy9xXZEKdbd4o2erhLSblDL5Rg&e=
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_patchwork_cover_572944_&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=hq4errAA9YvVpHJaVoWGTIHnlwiq1iuadWW1WJavtCI&e=
> 
> * dwc2 patches to allow wakeup on Rockchip rk3288 (REPOST)
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1436207224-2D21849-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=n-nD8EdqtgTyPdPPJ06t2pxyjC1M65g5aXLT1OiuouY&e=
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_patchwork_cover_576120_&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=YkisA-u9q0yewqaRoOWWpps9E2QFh1asgnwkbM1B7mc&e=
> 
> I'm hoping there's a better chance of these things landing this time
> around, but I guess we'll see.  ;-)
> 
> In case it's helpful I've put what I tested (which is based on Heiko's
> for-next branch and includes patches to enable deep suspend plus two
> other s2r fixes) at:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium.googlesource.com_chromiumos_third-5Fparty_kernel_-2Blog_refs_sandbox_dianders_190417-2Dtesting-2Ddwc2-2Dwakeup&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=ws6kQVDDGxYJnhR693pQP9fYL-PH_TUGzSVmD4u-S9Q&e=
> 
> Changes in v2:
> - Rebased to mainline atop rk3288 remote wake quirk series.
> - rk3288-veyron dts patch new for v2.
> 
> Douglas Anderson (5):
>    usb: dwc2: bus suspend/resume for hosts with
>      DWC2_POWER_DOWN_PARAM_NONE
>    USB: Export usb_wakeup_enabled_descendants()
>    Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
>    USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
>    ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports
> 
>   .../devicetree/bindings/usb/dwc2.txt          |  3 +
>   arch/arm/boot/dts/rk3288-veyron.dtsi          |  2 +
>   drivers/usb/core/hub.c                        |  7 +-
>   drivers/usb/dwc2/core.h                       |  5 ++
>   drivers/usb/dwc2/hcd.c                        | 84 ++++++++++++-------
>   drivers/usb/dwc2/platform.c                   | 43 +++++++++-
>   include/linux/usb/hcd.h                       |  5 ++
>   7 files changed, 113 insertions(+), 36 deletions(-)
> 

Did you consider/reviewed patch series from Artur Petrosyan "[PATCH 
00/14] usb: dwc2: Fix and improve power saving modes" (submitted on 
April 12) which fixing partial power down and hibernation flows for both 
modes: host and device?
I suspect that this both patch series can be in conflict.

Thanks,
Minas


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron
  2019-04-18 12:40 ` [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron Minas Harutyunyan
@ 2019-04-18 15:54   ` Doug Anderson
  2019-04-19 11:43     ` Artur Petrosyan
  0 siblings, 1 reply; 50+ messages in thread
From: Doug Anderson @ 2019-04-18 15:54 UTC (permalink / raw)
  To: Minas Harutyunyan, Artur Petrosyan
  Cc: Felipe Balbi, heiko, Alan Stern, amstan, linux-rockchip,
	William Wu, linux-usb, Stefan Wahren, Randy Li, zyw, mka,
	ryandcase, Amelie Delaunay, jwerner, Elaine Zhang,
	linux-arm-kernel, Kai-Heng Feng, Mathias Nyman, devicetree,
	linux-kernel, Nicolas Boichat, Jon Flatley, Greg Kroah-Hartman,
	Rob Herring, Mark Rutland, Nicolas Saenz Julienne

Hi,

On Thu, Apr 18, 2019 at 5:41 AM Minas Harutyunyan
<Minas.Harutyunyan@synopsys.com> wrote:
> Did you consider/reviewed patch series from Artur Petrosyan "[PATCH
> 00/14] usb: dwc2: Fix and improve power saving modes" (submitted on
> April 12) which fixing partial power down and hibernation flows for both
> modes: host and device?
> I suspect that this both patch series can be in conflict.

No, I wasn't aware of them.  I'd like to try them out, but it looks
like it's currently impossible because they're not archived anywhere
that I can find.

1. LKML wasn't copied, so I can't find them on lore.kernel.org.  It is
suggested to CC LKML on all patches.

2. The Linux USB patchwork only has the cover letter plus the first 6
patches.  See <https://patchwork.kernel.org/cover/10898333/>

3. Searching my own archives I only see the cover letter plus the
first 6 patches.

Maybe you have some other pointer to how I can retrieve them?  I guess
I could try the first 6 patches without the later 8 but I'd rather get
the whole set...


-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron
  2019-04-18 15:54   ` Doug Anderson
@ 2019-04-19 11:43     ` Artur Petrosyan
  2019-04-19 16:44       ` Artur Petrosyan
  0 siblings, 1 reply; 50+ messages in thread
From: Artur Petrosyan @ 2019-04-19 11:43 UTC (permalink / raw)
  To: Doug Anderson, Minas Harutyunyan
  Cc: Felipe Balbi, heiko, Alan Stern, amstan, linux-rockchip,
	William Wu, linux-usb, Stefan Wahren, Randy Li, zyw, mka,
	ryandcase, Amelie Delaunay, jwerner, Elaine Zhang,
	linux-arm-kernel, Kai-Heng Feng, Mathias Nyman, devicetree,
	linux-kernel, Nicolas Boichat, Jon Flatley, Greg Kroah-Hartman,
	Rob Herring, Mark Rutland, Nicolas Saenz Julienne

Hi,

On 4/18/2019 19:55, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 18, 2019 at 5:41 AM Minas Harutyunyan
> <Minas.Harutyunyan@synopsys.com> wrote:
>> Did you consider/reviewed patch series from Artur Petrosyan "[PATCH
>> 00/14] usb: dwc2: Fix and improve power saving modes" (submitted on
>> April 12) which fixing partial power down and hibernation flows for both
>> modes: host and device?
>> I suspect that this both patch series can be in conflict.
> 
> No, I wasn't aware of them.  I'd like to try them out, but it looks
> like it's currently impossible because they're not archived anywhere
> that I can find.
> 
> 1. LKML wasn't copied, so I can't find them on lore.kernel.org.  It is
> suggested to CC LKML on all patches.
> 
> 2. The Linux USB patchwork only has the cover letter plus the first 6
> patches.  See <https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_cover_10898333_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=rJbofHXqjP3sUBgEikaZ89fRWtsquvhFUkhSPguCiWw&s=_CrI0uolquXIYC5SuoZ1vBs7BM19VfCceI96qZm9kAY&e=>
> 
> 3. Searching my own archives I only see the cover letter plus the
> first 6 patches.
> 
> Maybe you have some other pointer to how I can retrieve them?  I guess
> I could try the first 6 patches without the later 8 but I'd rather get
> the whole set...
> 
> 
> -Doug
>


I have resend the patch set. You can find it with cover letter
"[PATCH v1 00/14] usb: dwc2: Fix and improve power saving modes."

Let me know if you need any help.

-- 
Regards,
Artur

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron
  2019-04-19 11:43     ` Artur Petrosyan
@ 2019-04-19 16:44       ` Artur Petrosyan
  2019-04-22 15:50         ` Artur Petrosyan
  0 siblings, 1 reply; 50+ messages in thread
From: Artur Petrosyan @ 2019-04-19 16:44 UTC (permalink / raw)
  To: Doug Anderson, Minas Harutyunyan
  Cc: Felipe Balbi, heiko, Alan Stern, amstan, linux-rockchip,
	William Wu, linux-usb, Stefan Wahren, Randy Li, zyw, mka,
	ryandcase, Amelie Delaunay, jwerner, Elaine Zhang,
	linux-arm-kernel, Kai-Heng Feng, Mathias Nyman, devicetree,
	linux-kernel, Nicolas Boichat, Jon Flatley, Greg Kroah-Hartman,
	Rob Herring, Mark Rutland, Nicolas Saenz Julienne

Hi Doug,

On 4/19/2019 15:43, Artur Petrosyan wrote:
> Hi,
> 
> On 4/18/2019 19:55, Doug Anderson wrote:
>> Hi,
>>
>> On Thu, Apr 18, 2019 at 5:41 AM Minas Harutyunyan
>> <Minas.Harutyunyan@synopsys.com> wrote:
>>> Did you consider/reviewed patch series from Artur Petrosyan "[PATCH
>>> 00/14] usb: dwc2: Fix and improve power saving modes" (submitted on
>>> April 12) which fixing partial power down and hibernation flows for both
>>> modes: host and device?
>>> I suspect that this both patch series can be in conflict.
>>
>> No, I wasn't aware of them.  I'd like to try them out, but it looks
>> like it's currently impossible because they're not archived anywhere
>> that I can find.
>>
>> 1. LKML wasn't copied, so I can't find them on lore.kernel.org.  It is
>> suggested to CC LKML on all patches.
>>
>> 2. The Linux USB patchwork only has the cover letter plus the first 6
>> patches.  See <https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_cover_10898333_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=rJbofHXqjP3sUBgEikaZ89fRWtsquvhFUkhSPguCiWw&s=_CrI0uolquXIYC5SuoZ1vBs7BM19VfCceI96qZm9kAY&e=>
>>
>> 3. Searching my own archives I only see the cover letter plus the
>> first 6 patches.
>>
>> Maybe you have some other pointer to how I can retrieve them?  I guess
>> I could try the first 6 patches without the later 8 but I'd rather get
>> the whole set...
>>
>>
>> -Doug
>>
> 
> 
> I have resend the patch set. You can find it with cover letter
> "[PATCH v1 00/14] usb: dwc2: Fix and improve power saving modes."
> 
> Let me know if you need any help.
> 

I am so sorry to inform you that the patches have not reached to LKML.
Looks like there is a problem with the kernel mailing list servers or 
our local servers.
Will try to fix this as soon as possible and let you know about it.

-- 
Regards,
Artur

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron
  2019-04-19 16:44       ` Artur Petrosyan
@ 2019-04-22 15:50         ` Artur Petrosyan
  0 siblings, 0 replies; 50+ messages in thread
From: Artur Petrosyan @ 2019-04-22 15:50 UTC (permalink / raw)
  To: Doug Anderson, Minas Harutyunyan
  Cc: Felipe Balbi, heiko, Alan Stern, amstan, linux-rockchip,
	William Wu, linux-usb, Stefan Wahren, Randy Li, zyw, mka,
	ryandcase, Amelie Delaunay, jwerner, Elaine Zhang,
	linux-arm-kernel, Kai-Heng Feng, Mathias Nyman, devicetree,
	linux-kernel, Nicolas Boichat, Jon Flatley, Greg Kroah-Hartman,
	Rob Herring, Mark Rutland, Nicolas Saenz Julienne

Hi Doug,

On 4/19/2019 20:44, Artur Petrosyan wrote:
> Hi Doug,
> 
> On 4/19/2019 15:43, Artur Petrosyan wrote:
>> Hi,
>>
>> On 4/18/2019 19:55, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Thu, Apr 18, 2019 at 5:41 AM Minas Harutyunyan
>>> <Minas.Harutyunyan@synopsys.com> wrote:
>>>> Did you consider/reviewed patch series from Artur Petrosyan "[PATCH
>>>> 00/14] usb: dwc2: Fix and improve power saving modes" (submitted on
>>>> April 12) which fixing partial power down and hibernation flows for both
>>>> modes: host and device?
>>>> I suspect that this both patch series can be in conflict.
>>>
>>> No, I wasn't aware of them.  I'd like to try them out, but it looks
>>> like it's currently impossible because they're not archived anywhere
>>> that I can find.
>>>
>>> 1. LKML wasn't copied, so I can't find them on lore.kernel.org.  It is
>>> suggested to CC LKML on all patches.
>>>
>>> 2. The Linux USB patchwork only has the cover letter plus the first 6
>>> patches.  See <https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_cover_10898333_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=rJbofHXqjP3sUBgEikaZ89fRWtsquvhFUkhSPguCiWw&s=_CrI0uolquXIYC5SuoZ1vBs7BM19VfCceI96qZm9kAY&e=>
>>>
>>> 3. Searching my own archives I only see the cover letter plus the
>>> first 6 patches.
>>>
>>> Maybe you have some other pointer to how I can retrieve them?  I guess
>>> I could try the first 6 patches without the later 8 but I'd rather get
>>> the whole set...
>>>
>>>
>>> -Doug
>>>
>>
>>
>> I have resend the patch set. You can find it with cover letter
>> "[PATCH v1 00/14] usb: dwc2: Fix and improve power saving modes."
>>
>> Let me know if you need any help.
>>
> 
> I am so sorry to inform you that the patches have not reached to LKML.
> Looks like there is a problem with the kernel mailing list servers or
> our local servers.
> Will try to fix this as soon as possible and let you know about it.
> 

My patches has reached to LKML. You can find them with the patch set 
title "[PATCH v1 00/14] usb: dwc2: Fix and improve power saving modes."

This is the link to the patch set in marc.info
"https://marc.info/?l=linux-usb&m=155570003924638&w=2"
and a link in patchwork.kernel.org
"https://patchwork.kernel.org/project/linux-usb/list/?submitter=180003"

Sorry for the delay.

-- 
Regards,
Artur

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
@ 2019-04-25 12:40   ` " Felipe Balbi
  2019-04-25 12:40     ` [PATCH v2 3/5] " Felipe Balbi
                       ` (2 more replies)
  0 siblings, 3 replies; 50+ messages in thread
From: Felipe Balbi @ 2019-04-25 12:40 UTC (permalink / raw)
  To: Douglas Anderson, Minas Harutyunyan, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang, devicetree,
	linux-kernel, Rob Herring, Greg Kroah-Hartman, Mark Rutland

Douglas Anderson <dianders@chromium.org> writes:

> Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> support remote wakeup.  Allow specifying this as a device tree
> property.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> For relevant prior discussion on this patch, see:
>
> https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
>
> I didn't make any changes from the prior version since I never found
> out what Rob thought of my previous arguments.  If folks want a
> change, perhaps they could choose from these options:
>
> 1. Assume that all dwc2 hosts would like to keep their PHY on for
>    suspend if there's a USB wakeup enabled, thus we totally drop this
>    binding.  This doesn't seem super great to me since I'd bet that
>    many devices that use dwc2 weren't designed for USB wakeup (they
>    may not keep enough clocks or rails on) so we might be wasting
>    power for nothing.
> 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
>    it more obvious that this property is intended both to document
>    that wakeup from suspend is possible and that we need the PHY for
>    said wakeup.
> 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
>    it's implicit that if we can wakeup from suspend that we need to
>    keep the PHY on.  If/when someone shows that a device exists using
>    dwc2 where we can wakeup from suspend without the PHY they can add
>    a new property.
>
> Changes in v2: None
>
>  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
>  1 file changed, 3 insertions(+)

checking file Documentation/devicetree/bindings/usb/dwc2.txt
Hunk #1 FAILED at 37.
Hunk #2 succeeded at 52 (offset -1 lines).
1 out of 2 hunks FAILED

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
  2019-04-25 12:40   ` [v2,3/5] " Felipe Balbi
@ 2019-04-25 12:40     ` " Felipe Balbi
  2019-04-25 18:09     ` [v2,3/5] " Doug Anderson
  2019-05-02 18:36     ` [v2,3/5] " Doug Anderson
  2 siblings, 0 replies; 50+ messages in thread
From: Felipe Balbi @ 2019-04-25 12:40 UTC (permalink / raw)
  To: Douglas Anderson, Minas Harutyunyan, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, devicetree, linux-kernel, Rob Herring,
	Greg Kroah-Hartman, Mark Rutland

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

Douglas Anderson <dianders@chromium.org> writes:

> Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> support remote wakeup.  Allow specifying this as a device tree
> property.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> For relevant prior discussion on this patch, see:
>
> https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
>
> I didn't make any changes from the prior version since I never found
> out what Rob thought of my previous arguments.  If folks want a
> change, perhaps they could choose from these options:
>
> 1. Assume that all dwc2 hosts would like to keep their PHY on for
>    suspend if there's a USB wakeup enabled, thus we totally drop this
>    binding.  This doesn't seem super great to me since I'd bet that
>    many devices that use dwc2 weren't designed for USB wakeup (they
>    may not keep enough clocks or rails on) so we might be wasting
>    power for nothing.
> 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
>    it more obvious that this property is intended both to document
>    that wakeup from suspend is possible and that we need the PHY for
>    said wakeup.
> 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
>    it's implicit that if we can wakeup from suspend that we need to
>    keep the PHY on.  If/when someone shows that a device exists using
>    dwc2 where we can wakeup from suspend without the PHY they can add
>    a new property.
>
> Changes in v2: None
>
>  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
>  1 file changed, 3 insertions(+)

checking file Documentation/devicetree/bindings/usb/dwc2.txt
Hunk #1 FAILED at 37.
Hunk #2 succeeded at 52 (offset -1 lines).
1 out of 2 hunks FAILED
-- 
balbi

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,4/5] USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
@ 2019-04-25 12:41   ` " Felipe Balbi
  2019-04-25 12:41     ` [PATCH v2 4/5] " Felipe Balbi
  0 siblings, 1 reply; 50+ messages in thread
From: Felipe Balbi @ 2019-04-25 12:41 UTC (permalink / raw)
  To: Douglas Anderson, Minas Harutyunyan, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Greg Kroah-Hartman, linux-kernel

Douglas Anderson <dianders@chromium.org> writes:

> If the 'snps,need-phy-for-wake' is set in the device tree then:
>
> - We know that we can wakeup, so call device_set_wakeup_capable().
>   The USB core will use this knowledge to enable wakeup by default.
> - We know that we should keep the PHY on during suspend if something
>   on our root hub needs remote wakeup.  This requires the patch (USB:
>   Export usb_wakeup_enabled_descendants()).  Note that we don't keep
>   the PHY on at suspend time if it's not needed because it would be a
>   power draw.
>
> If we later find some users of dwc2 that can support wakeup without
> keeping the PHY on we may want to add a way to call
> device_set_wakeup_capable() without keeping the PHY on at suspend
> time.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>

checking file drivers/usb/dwc2/core.h
Hunk #1 FAILED at 861.
Hunk #2 FAILED at 1049.
2 out of 2 hunks FAILED
checking file drivers/usb/dwc2/platform.c
Hunk #3 FAILED at 487.
Hunk #4 succeeded at 513 (offset -9 lines).
Hunk #5 succeeded at 543 (offset -9 lines).
Hunk #6 succeeded at 556 (offset -9 lines).
1 out of 6 hunks FAILED

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 4/5] USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
  2019-04-25 12:41   ` [v2,4/5] " Felipe Balbi
@ 2019-04-25 12:41     ` " Felipe Balbi
  0 siblings, 0 replies; 50+ messages in thread
From: Felipe Balbi @ 2019-04-25 12:41 UTC (permalink / raw)
  To: Douglas Anderson, Minas Harutyunyan, heiko
  Cc: Alan Stern, Artur Petrosyan, amstan, linux-rockchip, William Wu,
	linux-usb, Stefan Wahren, Randy Li, zyw, mka, ryandcase,
	Amelie Delaunay, jwerner, dinguyen, Elaine Zhang,
	Douglas Anderson, Greg Kroah-Hartman, linux-kernel

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

Douglas Anderson <dianders@chromium.org> writes:

> If the 'snps,need-phy-for-wake' is set in the device tree then:
>
> - We know that we can wakeup, so call device_set_wakeup_capable().
>   The USB core will use this knowledge to enable wakeup by default.
> - We know that we should keep the PHY on during suspend if something
>   on our root hub needs remote wakeup.  This requires the patch (USB:
>   Export usb_wakeup_enabled_descendants()).  Note that we don't keep
>   the PHY on at suspend time if it's not needed because it would be a
>   power draw.
>
> If we later find some users of dwc2 that can support wakeup without
> keeping the PHY on we may want to add a way to call
> device_set_wakeup_capable() without keeping the PHY on at suspend
> time.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>

checking file drivers/usb/dwc2/core.h
Hunk #1 FAILED at 861.
Hunk #2 FAILED at 1049.
2 out of 2 hunks FAILED
checking file drivers/usb/dwc2/platform.c
Hunk #3 FAILED at 487.
Hunk #4 succeeded at 513 (offset -9 lines).
Hunk #5 succeeded at 543 (offset -9 lines).
Hunk #6 succeeded at 556 (offset -9 lines).
1 out of 6 hunks FAILED


-- 
balbi

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

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
@ 2019-04-25 18:09     ` " Doug Anderson
  2019-04-25 18:09       ` [PATCH v2 3/5] " Doug Anderson
  2019-04-25 19:58       ` [v2,3/5] " Doug Anderson
  0 siblings, 2 replies; 50+ messages in thread
From: Doug Anderson @ 2019-04-25 18:09 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Minas Harutyunyan, Heiko Stübner, Alan Stern,
	Artur Petrosyan, Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Dinh Nguyen, Elaine Zhang, devicetree, LKML, Rob Herring,
	Greg Kroah-Hartman, Mark Rutland

Hi,

On Thu, Apr 25, 2019 at 5:40 AM Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Douglas Anderson <dianders@chromium.org> writes:
>
> > Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> > support remote wakeup.  Allow specifying this as a device tree
> > property.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > For relevant prior discussion on this patch, see:
> >
> > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> >
> > I didn't make any changes from the prior version since I never found
> > out what Rob thought of my previous arguments.  If folks want a
> > change, perhaps they could choose from these options:
> >
> > 1. Assume that all dwc2 hosts would like to keep their PHY on for
> >    suspend if there's a USB wakeup enabled, thus we totally drop this
> >    binding.  This doesn't seem super great to me since I'd bet that
> >    many devices that use dwc2 weren't designed for USB wakeup (they
> >    may not keep enough clocks or rails on) so we might be wasting
> >    power for nothing.
> > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
> >    it more obvious that this property is intended both to document
> >    that wakeup from suspend is possible and that we need the PHY for
> >    said wakeup.
> > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
> >    it's implicit that if we can wakeup from suspend that we need to
> >    keep the PHY on.  If/when someone shows that a device exists using
> >    dwc2 where we can wakeup from suspend without the PHY they can add
> >    a new property.
> >
> > Changes in v2: None
> >
> >  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
> >  1 file changed, 3 insertions(+)
>
> checking file Documentation/devicetree/bindings/usb/dwc2.txt
> Hunk #1 FAILED at 37.
> Hunk #2 succeeded at 52 (offset -1 lines).
> 1 out of 2 hunks FAILED

Yeah, as Minas pointed out in the cover letter [1] my series conflicts
with Artur's.  I have it on my list to try out his series and see if,
perhaps, it allows me to enable the partial power down and also just
generally rebase.  It's fairly high on my list to do that--hopefully
in the next week.

[1] https://lkml.kernel.org/r/e4b3cd69-1c91-dfbe-bea7-bbca89ca1348@synopsys.com

-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
  2019-04-25 18:09     ` [v2,3/5] " Doug Anderson
@ 2019-04-25 18:09       ` " Doug Anderson
  2019-04-25 19:58       ` [v2,3/5] " Doug Anderson
  1 sibling, 0 replies; 50+ messages in thread
From: Doug Anderson @ 2019-04-25 18:09 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Minas Harutyunyan, Heiko Stübner, Alan Stern,
	Artur Petrosyan, Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Dinh Nguyen, Elaine Zhang, devicetree, LKML, Rob Herring,
	Greg Kroah-Hartman, Mark Rutland

Hi,

On Thu, Apr 25, 2019 at 5:40 AM Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Douglas Anderson <dianders@chromium.org> writes:
>
> > Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> > support remote wakeup.  Allow specifying this as a device tree
> > property.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > For relevant prior discussion on this patch, see:
> >
> > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> >
> > I didn't make any changes from the prior version since I never found
> > out what Rob thought of my previous arguments.  If folks want a
> > change, perhaps they could choose from these options:
> >
> > 1. Assume that all dwc2 hosts would like to keep their PHY on for
> >    suspend if there's a USB wakeup enabled, thus we totally drop this
> >    binding.  This doesn't seem super great to me since I'd bet that
> >    many devices that use dwc2 weren't designed for USB wakeup (they
> >    may not keep enough clocks or rails on) so we might be wasting
> >    power for nothing.
> > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
> >    it more obvious that this property is intended both to document
> >    that wakeup from suspend is possible and that we need the PHY for
> >    said wakeup.
> > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
> >    it's implicit that if we can wakeup from suspend that we need to
> >    keep the PHY on.  If/when someone shows that a device exists using
> >    dwc2 where we can wakeup from suspend without the PHY they can add
> >    a new property.
> >
> > Changes in v2: None
> >
> >  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
> >  1 file changed, 3 insertions(+)
>
> checking file Documentation/devicetree/bindings/usb/dwc2.txt
> Hunk #1 FAILED at 37.
> Hunk #2 succeeded at 52 (offset -1 lines).
> 1 out of 2 hunks FAILED

Yeah, as Minas pointed out in the cover letter [1] my series conflicts
with Artur's.  I have it on my list to try out his series and see if,
perhaps, it allows me to enable the partial power down and also just
generally rebase.  It's fairly high on my list to do that--hopefully
in the next week.

[1] https://lkml.kernel.org/r/e4b3cd69-1c91-dfbe-bea7-bbca89ca1348@synopsys.com

-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
@ 2019-04-25 19:58       ` " Doug Anderson
  2019-04-25 19:58         ` [PATCH v2 3/5] " Doug Anderson
  0 siblings, 1 reply; 50+ messages in thread
From: Doug Anderson @ 2019-04-25 19:58 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Minas Harutyunyan, Heiko Stübner, Alan Stern,
	Artur Petrosyan, Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Dinh Nguyen, Elaine Zhang, devicetree, LKML, Rob Herring,
	Greg Kroah-Hartman, Mark Rutland

Hi,

On Thu, Apr 25, 2019 at 11:09 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Apr 25, 2019 at 5:40 AM Felipe Balbi
> <felipe.balbi@linux.intel.com> wrote:
> >
> > Douglas Anderson <dianders@chromium.org> writes:
> >
> > > Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> > > support remote wakeup.  Allow specifying this as a device tree
> > > property.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > > For relevant prior discussion on this patch, see:
> > >
> > > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> > >
> > > I didn't make any changes from the prior version since I never found
> > > out what Rob thought of my previous arguments.  If folks want a
> > > change, perhaps they could choose from these options:
> > >
> > > 1. Assume that all dwc2 hosts would like to keep their PHY on for
> > >    suspend if there's a USB wakeup enabled, thus we totally drop this
> > >    binding.  This doesn't seem super great to me since I'd bet that
> > >    many devices that use dwc2 weren't designed for USB wakeup (they
> > >    may not keep enough clocks or rails on) so we might be wasting
> > >    power for nothing.
> > > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
> > >    it more obvious that this property is intended both to document
> > >    that wakeup from suspend is possible and that we need the PHY for
> > >    said wakeup.
> > > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
> > >    it's implicit that if we can wakeup from suspend that we need to
> > >    keep the PHY on.  If/when someone shows that a device exists using
> > >    dwc2 where we can wakeup from suspend without the PHY they can add
> > >    a new property.
> > >
> > > Changes in v2: None
> > >
> > >  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
> > >  1 file changed, 3 insertions(+)
> >
> > checking file Documentation/devicetree/bindings/usb/dwc2.txt
> > Hunk #1 FAILED at 37.
> > Hunk #2 succeeded at 52 (offset -1 lines).
> > 1 out of 2 hunks FAILED
>
> Yeah, as Minas pointed out in the cover letter [1] my series conflicts
> with Artur's.  I have it on my list to try out his series and see if,
> perhaps, it allows me to enable the partial power down and also just
> generally rebase.  It's fairly high on my list to do that--hopefully
> in the next week.
>
> [1] https://lkml.kernel.org/r/e4b3cd69-1c91-dfbe-bea7-bbca89ca1348@synopsys.com

Oh, it looks like you didn't apply Artur's patches, though.
Presumably you applied mine first and they won the race and thus I
guess it's up to Artur to rebase his patches atop mine.  This probably
explains why you told him the patches didn't apply.  I'll reply to
that thread.

...so it turns out that when I try now my patches apply fine [2].  I'm
guessing that you perhaps tried to apply these patches before my
"rk3288's remote wake quirk" which would indeed cause conflicts.  I
mentioned the dependency in my cover letter [1] but totally understand
that it's easy to miss stuff like that.  :-)

I'm going to assume you can just go-ahead and try applying patches 3,
4, and 5 in this series again.  If you want me to repost them then
please let me know.

Thanks, and sorry for the hassle.

[1] https://lkml.kernel.org/r/20190418001356.124334-1-dianders@chromium.org

[2] Showing that patches currently apply:

dianders@tictac2:v4.19 ((2e3cfcbbb140...))$ git checkout
linux_usb_balbi/testing/next
HEAD is now at 2e3cfcbbb140 dwc2: gadget: Fix completed transfer size
calculation in DDMA

dianders@tictac2:v4.19 ((2e3cfcbbb140...))$ curl -L
https://lore.kernel.org/patchwork/patch/1063477/mbox | git am
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  7273  100  7273    0     0  35827      0 --:--:-- --:--:-- --:--:-- 35827
Applying: Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB

dianders@tictac2:v4.19 ((84c34d7b9647...))$ curl -L
https://lore.kernel.org/patchwork/patch/1063478/mbox | git am
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  9587  100  9587    0     0  46538      0 --:--:-- --:--:-- --:--:-- 46538
Applying: USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled

dianders@tictac2:v4.19 ((ff5ab1cb16ab...))$ curl -L
https://lore.kernel.org/patchwork/patch/1063479/mbox | git am
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  6045  100  6045    0     0  29778      0 --:--:-- --:--:-- --:--:-- 29778
Applying: ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports

-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
  2019-04-25 19:58       ` [v2,3/5] " Doug Anderson
@ 2019-04-25 19:58         ` " Doug Anderson
  0 siblings, 0 replies; 50+ messages in thread
From: Doug Anderson @ 2019-04-25 19:58 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Minas Harutyunyan, Heiko Stübner, Alan Stern,
	Artur Petrosyan, Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Dinh Nguyen, Elaine Zhang, devicetree, LKML, Rob Herring,
	Greg Kroah-Hartman, Mark Rutland

Hi,

On Thu, Apr 25, 2019 at 11:09 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Apr 25, 2019 at 5:40 AM Felipe Balbi
> <felipe.balbi@linux.intel.com> wrote:
> >
> > Douglas Anderson <dianders@chromium.org> writes:
> >
> > > Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> > > support remote wakeup.  Allow specifying this as a device tree
> > > property.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > > For relevant prior discussion on this patch, see:
> > >
> > > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> > >
> > > I didn't make any changes from the prior version since I never found
> > > out what Rob thought of my previous arguments.  If folks want a
> > > change, perhaps they could choose from these options:
> > >
> > > 1. Assume that all dwc2 hosts would like to keep their PHY on for
> > >    suspend if there's a USB wakeup enabled, thus we totally drop this
> > >    binding.  This doesn't seem super great to me since I'd bet that
> > >    many devices that use dwc2 weren't designed for USB wakeup (they
> > >    may not keep enough clocks or rails on) so we might be wasting
> > >    power for nothing.
> > > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
> > >    it more obvious that this property is intended both to document
> > >    that wakeup from suspend is possible and that we need the PHY for
> > >    said wakeup.
> > > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
> > >    it's implicit that if we can wakeup from suspend that we need to
> > >    keep the PHY on.  If/when someone shows that a device exists using
> > >    dwc2 where we can wakeup from suspend without the PHY they can add
> > >    a new property.
> > >
> > > Changes in v2: None
> > >
> > >  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
> > >  1 file changed, 3 insertions(+)
> >
> > checking file Documentation/devicetree/bindings/usb/dwc2.txt
> > Hunk #1 FAILED at 37.
> > Hunk #2 succeeded at 52 (offset -1 lines).
> > 1 out of 2 hunks FAILED
>
> Yeah, as Minas pointed out in the cover letter [1] my series conflicts
> with Artur's.  I have it on my list to try out his series and see if,
> perhaps, it allows me to enable the partial power down and also just
> generally rebase.  It's fairly high on my list to do that--hopefully
> in the next week.
>
> [1] https://lkml.kernel.org/r/e4b3cd69-1c91-dfbe-bea7-bbca89ca1348@synopsys.com

Oh, it looks like you didn't apply Artur's patches, though.
Presumably you applied mine first and they won the race and thus I
guess it's up to Artur to rebase his patches atop mine.  This probably
explains why you told him the patches didn't apply.  I'll reply to
that thread.

...so it turns out that when I try now my patches apply fine [2].  I'm
guessing that you perhaps tried to apply these patches before my
"rk3288's remote wake quirk" which would indeed cause conflicts.  I
mentioned the dependency in my cover letter [1] but totally understand
that it's easy to miss stuff like that.  :-)

I'm going to assume you can just go-ahead and try applying patches 3,
4, and 5 in this series again.  If you want me to repost them then
please let me know.

Thanks, and sorry for the hassle.

[1] https://lkml.kernel.org/r/20190418001356.124334-1-dianders@chromium.org

[2] Showing that patches currently apply:

dianders@tictac2:v4.19 ((2e3cfcbbb140...))$ git checkout
linux_usb_balbi/testing/next
HEAD is now at 2e3cfcbbb140 dwc2: gadget: Fix completed transfer size
calculation in DDMA

dianders@tictac2:v4.19 ((2e3cfcbbb140...))$ curl -L
https://lore.kernel.org/patchwork/patch/1063477/mbox | git am
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  7273  100  7273    0     0  35827      0 --:--:-- --:--:-- --:--:-- 35827
Applying: Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB

dianders@tictac2:v4.19 ((84c34d7b9647...))$ curl -L
https://lore.kernel.org/patchwork/patch/1063478/mbox | git am
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  9587  100  9587    0     0  46538      0 --:--:-- --:--:-- --:--:-- 46538
Applying: USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled

dianders@tictac2:v4.19 ((ff5ab1cb16ab...))$ curl -L
https://lore.kernel.org/patchwork/patch/1063479/mbox | git am
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  6045  100  6045    0     0  29778      0 --:--:-- --:--:-- --:--:-- 29778
Applying: ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports

-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
@ 2019-04-29  8:43   ` " Artur Petrosyan
  2019-04-29  8:43     ` [PATCH v2 1/5] " Artur Petrosyan
  2019-04-29 17:33     ` [v2,1/5] " Doug Anderson
  0 siblings, 2 replies; 50+ messages in thread
From: Artur Petrosyan @ 2019-04-29  8:43 UTC (permalink / raw)
  To: Douglas Anderson, Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, amstan, linux-rockchip, William Wu, linux-usb,
	Stefan Wahren, Randy Li, zyw, mka, ryandcase, Amelie Delaunay,
	jwerner, dinguyen, Elaine Zhang, Greg Kroah-Hartman,
	linux-kernel

Hi,

On 4/18/2019 04:15, Douglas Anderson wrote:
> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> because apparently it broke the Altera SOCFPGA.
> 
> With all the changes that have happened to dwc2 in the meantime, it's
> possible that the Altera SOCFPGA will just magically work with this
> change now.  ...and it would be good to get bus suspend/resume
> implemented.
> 
> This change is a forward port of one that's been living in the Chrome
> OS 3.14 kernel tree.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This patch was last posted at:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
> 
> ...and appears to have died the death of silence.  Maybe it could get
> some bake time in linuxnext if we can't find any proactive testing?
> 
> I will also freely admit that I don't know tons about the theory
> behind this patch.  I'm mostly just re-hashing the original commit
> from Kever that was reverted since:
> * Turning on partial power down on rk3288 doesn't "just work".  I
>    don't get hotplug events.  This is despite dwc2 auto-detecting that
>    we are power optimized.
What do you mean by doesn't "just work" ? It seem to me that even after 
adding this patch you don't get issues fixed.
You mention that you don't get the hotplug events. Please provide dwc2 
debug logs and register dumps on this issue.

> * If we don't do something like this commit we don't get into as low
>    of a power mode.
> 
> Changes in v2: None
> 
>   drivers/usb/dwc2/hcd.c | 84 ++++++++++++++++++++++++++----------------
>   1 file changed, 53 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index e272d020012e..978232a9e4a8 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4482,6 +4482,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   	unsigned long flags;
>   	int ret = 0;
>   	u32 hprt0;
> +	u32 pcgctl;
>   
>   	spin_lock_irqsave(&hsotg->lock, flags);
>   
> @@ -4497,7 +4498,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   	if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
>   		goto unlock;
>   
> -	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
> +	if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL)
 >   		goto skip_power_saving;
 >

"hsotg->params.power_down" is assigned to "DWC2_POWER_DOWN_PARAM_NONE = 
0" if there is no hibernation or partial power down supported by the 
core or power saving features are disabled by 
"hsotg->params.power_saving = false" , "DWC2_POWER_DOWN_PARAM_PARTIAL" 
if core supports partial power down, "DWC2_POWER_DOWN_PARAM_HIBERNATION 
" if the core supports hibernation

When you check "if (hsotg->params.power_down > 
DWC2_POWER_DOWN_PARAM_PARTIAL)" you are saying that "skip_power_saving" 
only in that case when core supports Hibernation. But what if core 
doesn't support both hibernation and partial power down and the 
"hsotg->params.power_down" value us equal to 
"DWC2_POWER_DOWN_PARAM_NONE" which is 0.

With this implementation driver will program entering to suspend when 
core doesn't support both hibernation and partial power down.

>   	/*
> @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   	 */
>   	if (!hsotg->bus_suspended) {
>   		hprt0 = dwc2_read_hprt0(hsotg);
> -		hprt0 |= HPRT0_SUSP;
> -		hprt0 &= ~HPRT0_PWR;
> -		dwc2_writel(hsotg, hprt0, HPRT0);
> -		spin_unlock_irqrestore(&hsotg->lock, flags);
> -		dwc2_vbus_supply_exit(hsotg);
> -		spin_lock_irqsave(&hsotg->lock, flags);
> +		if (hprt0 & HPRT0_CONNSTS) { > +			hprt0 |= HPRT0_SUSP;
Here you set "HPRT0_SUSP" bit but what if core doesn't support both 
hibernation and Partial Power down assuming that 
hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE" 
which is 0.
> +			if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL)
You make one checking of hsotg->params.power_down mode here.
> +				hprt0 &= ~HPRT0_PWR;
> +			dwc2_writel(hsotg, hprt0, HPRT0);
> +		}
> +		if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
another checking of power_down mode here.
> +			spin_unlock_irqrestore(&hsotg->lock, flags);
> +			dwc2_vbus_supply_exit(hsotg);
> +			spin_lock_irqsave(&hsotg->lock, flags);
> +		} else {
> +			pcgctl = readl(hsotg->regs + PCGCTL);
> +			pcgctl |= PCGCTL_STOPPCLK;
> +			writel(pcgctl, hsotg->regs + PCGCTL);
"PCGCTL_STOPPCLK" bit is set only when core enters to partial power 
down. So here if hsotg->params.power_down is not equal to 
DWC2_POWER_DOWN_PARAM_PARTIAL and is DWC2_POWER_DOWN_PARAM_NONE the the 
bit will be set.
> +		}
>   	}
>   
> -	/* Enter partial_power_down */
> -	ret = dwc2_enter_partial_power_down(hsotg);
> -	if (ret) {
> -		if (ret != -ENOTSUPP)
> -			dev_err(hsotg->dev,
> -				"enter partial_power_down failed\n");
> -		goto skip_power_saving;
> +	if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
one more power_down mode checking here.
I understand that those checking are to make sure that we got partial 
power down mode enabled but before this patch it was done with one checking.
> +		/* Enter partial_power_down */
> +		ret = dwc2_enter_partial_power_down(hsotg);
> +		if (ret) {
> +			if (ret != -ENOTSUPP)
> +				dev_err(hsotg->dev,
> +					"enter partial_power_down failed\n");
> +			goto skip_power_saving;
> +		}
> +
> +		/* After entering partial_power_down, hardware is no more accessible */
> +		clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>   	}
>   
>   	/* Ask phy to be suspended */
> @@ -4530,9 +4545,6 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   		spin_lock_irqsave(&hsotg->lock, flags);
>   	}
>   
> -	/* After entering partial_power_down, hardware is no more accessible */
> -	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> -
>   skip_power_saving:
>   	hsotg->lx_state = DWC2_L2;
>   unlock:
> @@ -4545,6 +4557,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   {
>   	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>   	unsigned long flags;
> +	u32 pcgctl;
>   	int ret = 0;
>   
>   	spin_lock_irqsave(&hsotg->lock, flags);
> @@ -4555,17 +4568,11 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   	if (hsotg->lx_state != DWC2_L2)
>   		goto unlock;
>   
> -	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +	if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL) {
>   		hsotg->lx_state = DWC2_L0;
>   		goto unlock;
>   	}
>   
> -	/*
> -	 * Set HW accessible bit before powering on the controller
> -	 * since an interrupt may rise.
> -	 */
> -	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> -
>   	/*
>   	 * Enable power if not already done.
>   	 * This must not be spinlocked since duration
> @@ -4577,10 +4584,23 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   		spin_lock_irqsave(&hsotg->lock, flags);
>   	}
>   
> -	/* Exit partial_power_down */
> -	ret = dwc2_exit_partial_power_down(hsotg, true);
> -	if (ret && (ret != -ENOTSUPP))
> -		dev_err(hsotg->dev, "exit partial_power_down failed\n");
> +	if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +		/*
> +		 * Set HW accessible bit before powering on the controller
> +		 * since an interrupt may rise.
> +		 */
> +		set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +
> +
you leave an odd blank line here.  Please delete it.
> +		/* Exit partial_power_down */
> +		ret = dwc2_exit_partial_power_down(hsotg, true);
> +		if (ret && (ret != -ENOTSUPP))
> +			dev_err(hsotg->dev, "exit partial_power_down failed\n");
> +	} else {
> +		pcgctl = readl(hsotg->regs + PCGCTL);
> +		pcgctl &= ~PCGCTL_STOPPCLK;
> +		writel(pcgctl, hsotg->regs + PCGCTL);

Here if core doesn't support both hibernation and partial power down 
and "hsotg->params.power_down" is equal to "DWC2_POWER_DOWN_PARAM_NONE" 
which is 0 then "PCGCTL_STOPPCLK" bit is unset.

> +	}
>   
>   	hsotg->lx_state = DWC2_L0;
>   
> @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   		spin_unlock_irqrestore(&hsotg->lock, flags);
>   		dwc2_port_resume(hsotg);
>   	} else {
> -		dwc2_vbus_supply_init(hsotg);
> +		if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +			dwc2_vbus_supply_init(hsotg);
>   
> -		/* Wait for controller to correctly update D+/D- level */
> -		usleep_range(3000, 5000);
> +			/* Wait for controller to correctly update D+/D- level */
> +			usleep_range(3000, 5000);
> +		}
>   
>   		/*
>   		 * Clear Port Enable and Port Status changes.
> 

I have tested the patch on HAPS-DX. With this patch or without it when I 
have a device connected core  enters to partial power down and doesn't 
exit from it. So I cannot use the device.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
  2019-04-29  8:43   ` [v2,1/5] " Artur Petrosyan
@ 2019-04-29  8:43     ` " Artur Petrosyan
  2019-04-29 17:33     ` [v2,1/5] " Doug Anderson
  1 sibling, 0 replies; 50+ messages in thread
From: Artur Petrosyan @ 2019-04-29  8:43 UTC (permalink / raw)
  To: Douglas Anderson, Minas Harutyunyan, Felipe Balbi, heiko
  Cc: Alan Stern, amstan, linux-rockchip, William Wu, linux-usb,
	Stefan Wahren, Randy Li, zyw, mka, ryandcase, Amelie Delaunay,
	jwerner, dinguyen, Elaine Zhang, Greg Kroah-Hartman,
	linux-kernel

Hi,

On 4/18/2019 04:15, Douglas Anderson wrote:
> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> because apparently it broke the Altera SOCFPGA.
> 
> With all the changes that have happened to dwc2 in the meantime, it's
> possible that the Altera SOCFPGA will just magically work with this
> change now.  ...and it would be good to get bus suspend/resume
> implemented.
> 
> This change is a forward port of one that's been living in the Chrome
> OS 3.14 kernel tree.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This patch was last posted at:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
> 
> ...and appears to have died the death of silence.  Maybe it could get
> some bake time in linuxnext if we can't find any proactive testing?
> 
> I will also freely admit that I don't know tons about the theory
> behind this patch.  I'm mostly just re-hashing the original commit
> from Kever that was reverted since:
> * Turning on partial power down on rk3288 doesn't "just work".  I
>    don't get hotplug events.  This is despite dwc2 auto-detecting that
>    we are power optimized.
What do you mean by doesn't "just work" ? It seem to me that even after 
adding this patch you don't get issues fixed.
You mention that you don't get the hotplug events. Please provide dwc2 
debug logs and register dumps on this issue.

> * If we don't do something like this commit we don't get into as low
>    of a power mode.
> 
> Changes in v2: None
> 
>   drivers/usb/dwc2/hcd.c | 84 ++++++++++++++++++++++++++----------------
>   1 file changed, 53 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index e272d020012e..978232a9e4a8 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4482,6 +4482,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   	unsigned long flags;
>   	int ret = 0;
>   	u32 hprt0;
> +	u32 pcgctl;
>   
>   	spin_lock_irqsave(&hsotg->lock, flags);
>   
> @@ -4497,7 +4498,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   	if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
>   		goto unlock;
>   
> -	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
> +	if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL)
 >   		goto skip_power_saving;
 >

"hsotg->params.power_down" is assigned to "DWC2_POWER_DOWN_PARAM_NONE = 
0" if there is no hibernation or partial power down supported by the 
core or power saving features are disabled by 
"hsotg->params.power_saving = false" , "DWC2_POWER_DOWN_PARAM_PARTIAL" 
if core supports partial power down, "DWC2_POWER_DOWN_PARAM_HIBERNATION 
" if the core supports hibernation

When you check "if (hsotg->params.power_down > 
DWC2_POWER_DOWN_PARAM_PARTIAL)" you are saying that "skip_power_saving" 
only in that case when core supports Hibernation. But what if core 
doesn't support both hibernation and partial power down and the 
"hsotg->params.power_down" value us equal to 
"DWC2_POWER_DOWN_PARAM_NONE" which is 0.

With this implementation driver will program entering to suspend when 
core doesn't support both hibernation and partial power down.

>   	/*
> @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   	 */
>   	if (!hsotg->bus_suspended) {
>   		hprt0 = dwc2_read_hprt0(hsotg);
> -		hprt0 |= HPRT0_SUSP;
> -		hprt0 &= ~HPRT0_PWR;
> -		dwc2_writel(hsotg, hprt0, HPRT0);
> -		spin_unlock_irqrestore(&hsotg->lock, flags);
> -		dwc2_vbus_supply_exit(hsotg);
> -		spin_lock_irqsave(&hsotg->lock, flags);
> +		if (hprt0 & HPRT0_CONNSTS) { > +			hprt0 |= HPRT0_SUSP;
Here you set "HPRT0_SUSP" bit but what if core doesn't support both 
hibernation and Partial Power down assuming that 
hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE" 
which is 0.
> +			if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL)
You make one checking of hsotg->params.power_down mode here.
> +				hprt0 &= ~HPRT0_PWR;
> +			dwc2_writel(hsotg, hprt0, HPRT0);
> +		}
> +		if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
another checking of power_down mode here.
> +			spin_unlock_irqrestore(&hsotg->lock, flags);
> +			dwc2_vbus_supply_exit(hsotg);
> +			spin_lock_irqsave(&hsotg->lock, flags);
> +		} else {
> +			pcgctl = readl(hsotg->regs + PCGCTL);
> +			pcgctl |= PCGCTL_STOPPCLK;
> +			writel(pcgctl, hsotg->regs + PCGCTL);
"PCGCTL_STOPPCLK" bit is set only when core enters to partial power 
down. So here if hsotg->params.power_down is not equal to 
DWC2_POWER_DOWN_PARAM_PARTIAL and is DWC2_POWER_DOWN_PARAM_NONE the the 
bit will be set.
> +		}
>   	}
>   
> -	/* Enter partial_power_down */
> -	ret = dwc2_enter_partial_power_down(hsotg);
> -	if (ret) {
> -		if (ret != -ENOTSUPP)
> -			dev_err(hsotg->dev,
> -				"enter partial_power_down failed\n");
> -		goto skip_power_saving;
> +	if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
one more power_down mode checking here.
I understand that those checking are to make sure that we got partial 
power down mode enabled but before this patch it was done with one checking.
> +		/* Enter partial_power_down */
> +		ret = dwc2_enter_partial_power_down(hsotg);
> +		if (ret) {
> +			if (ret != -ENOTSUPP)
> +				dev_err(hsotg->dev,
> +					"enter partial_power_down failed\n");
> +			goto skip_power_saving;
> +		}
> +
> +		/* After entering partial_power_down, hardware is no more accessible */
> +		clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>   	}
>   
>   	/* Ask phy to be suspended */
> @@ -4530,9 +4545,6 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   		spin_lock_irqsave(&hsotg->lock, flags);
>   	}
>   
> -	/* After entering partial_power_down, hardware is no more accessible */
> -	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> -
>   skip_power_saving:
>   	hsotg->lx_state = DWC2_L2;
>   unlock:
> @@ -4545,6 +4557,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   {
>   	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>   	unsigned long flags;
> +	u32 pcgctl;
>   	int ret = 0;
>   
>   	spin_lock_irqsave(&hsotg->lock, flags);
> @@ -4555,17 +4568,11 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   	if (hsotg->lx_state != DWC2_L2)
>   		goto unlock;
>   
> -	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +	if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL) {
>   		hsotg->lx_state = DWC2_L0;
>   		goto unlock;
>   	}
>   
> -	/*
> -	 * Set HW accessible bit before powering on the controller
> -	 * since an interrupt may rise.
> -	 */
> -	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> -
>   	/*
>   	 * Enable power if not already done.
>   	 * This must not be spinlocked since duration
> @@ -4577,10 +4584,23 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   		spin_lock_irqsave(&hsotg->lock, flags);
>   	}
>   
> -	/* Exit partial_power_down */
> -	ret = dwc2_exit_partial_power_down(hsotg, true);
> -	if (ret && (ret != -ENOTSUPP))
> -		dev_err(hsotg->dev, "exit partial_power_down failed\n");
> +	if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +		/*
> +		 * Set HW accessible bit before powering on the controller
> +		 * since an interrupt may rise.
> +		 */
> +		set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +
> +
you leave an odd blank line here.  Please delete it.
> +		/* Exit partial_power_down */
> +		ret = dwc2_exit_partial_power_down(hsotg, true);
> +		if (ret && (ret != -ENOTSUPP))
> +			dev_err(hsotg->dev, "exit partial_power_down failed\n");
> +	} else {
> +		pcgctl = readl(hsotg->regs + PCGCTL);
> +		pcgctl &= ~PCGCTL_STOPPCLK;
> +		writel(pcgctl, hsotg->regs + PCGCTL);

Here if core doesn't support both hibernation and partial power down 
and "hsotg->params.power_down" is equal to "DWC2_POWER_DOWN_PARAM_NONE" 
which is 0 then "PCGCTL_STOPPCLK" bit is unset.

> +	}
>   
>   	hsotg->lx_state = DWC2_L0;
>   
> @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   		spin_unlock_irqrestore(&hsotg->lock, flags);
>   		dwc2_port_resume(hsotg);
>   	} else {
> -		dwc2_vbus_supply_init(hsotg);
> +		if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +			dwc2_vbus_supply_init(hsotg);
>   
> -		/* Wait for controller to correctly update D+/D- level */
> -		usleep_range(3000, 5000);
> +			/* Wait for controller to correctly update D+/D- level */
> +			usleep_range(3000, 5000);
> +		}
>   
>   		/*
>   		 * Clear Port Enable and Port Status changes.
> 

I have tested the patch on HAPS-DX. With this patch or without it when I 
have a device connected core  enters to partial power down and doesn't 
exit from it. So I cannot use the device.

-- 
Regards,
Artur

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
@ 2019-04-29 17:33     ` " Doug Anderson
  2019-04-29 17:33       ` [PATCH v2 1/5] " Doug Anderson
  2019-04-30  6:05       ` [v2,1/5] " Artur Petrosyan
  0 siblings, 2 replies; 50+ messages in thread
From: Doug Anderson @ 2019-04-29 17:33 UTC (permalink / raw)
  To: Artur Petrosyan
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern, amstan,
	linux-rockchip, William Wu, linux-usb, Stefan Wahren, Randy Li,
	zyw, mka, ryandcase, Amelie Delaunay, jwerner, dinguyen,
	Elaine Zhang, Greg Kroah-Hartman, linux-kernel

Hi,

On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> Hi,
>
> On 4/18/2019 04:15, Douglas Anderson wrote:
> > This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> > suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> > b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> > because apparently it broke the Altera SOCFPGA.
> >
> > With all the changes that have happened to dwc2 in the meantime, it's
> > possible that the Altera SOCFPGA will just magically work with this
> > change now.  ...and it would be good to get bus suspend/resume
> > implemented.
> >
> > This change is a forward port of one that's been living in the Chrome
> > OS 3.14 kernel tree.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > This patch was last posted at:
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
> >
> > ...and appears to have died the death of silence.  Maybe it could get
> > some bake time in linuxnext if we can't find any proactive testing?
> >
> > I will also freely admit that I don't know tons about the theory
> > behind this patch.  I'm mostly just re-hashing the original commit
> > from Kever that was reverted since:
> > * Turning on partial power down on rk3288 doesn't "just work".  I
> >    don't get hotplug events.  This is despite dwc2 auto-detecting that
> >    we are power optimized.
> What do you mean by doesn't "just work" ? It seem to me that even after
> adding this patch you don't get issues fixed.
> You mention that you don't get the hotplug events. Please provide dwc2
> debug logs and register dumps on this issue.

I mean that partial power down in the currently upstream driver
doesn't work.  AKA: if I turn on partial power down in the upstream
driver then hotplug events break.  I can try to provide some logs.  On
what exact version of the code do you want logs?  Just your series?
Just my series?  Mainline?  Some attempt at combining both series?  As
I said things seem to sorta work with the combined series.  I can try
to clarify if that's the series you want me to test with.  ...or I can
wait for your next version?


> > @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> >        */
> >       if (!hsotg->bus_suspended) {
> >               hprt0 = dwc2_read_hprt0(hsotg);
> > -             hprt0 |= HPRT0_SUSP;
> > -             hprt0 &= ~HPRT0_PWR;
> > -             dwc2_writel(hsotg, hprt0, HPRT0);
> > -             spin_unlock_irqrestore(&hsotg->lock, flags);
> > -             dwc2_vbus_supply_exit(hsotg);
> > -             spin_lock_irqsave(&hsotg->lock, flags);
> > +             if (hprt0 & HPRT0_CONNSTS) { > +                        hprt0 |= HPRT0_SUSP;
> Here you set "HPRT0_SUSP" bit but what if core doesn't support both
> hibernation and Partial Power down assuming that
> hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE"
> which is 0.

I am by no means an expert on dwc2, but an assumption made in my patch
is that even cores that can't support partial power down can still
save some amount of power when hcd_suspend is called.

Some evidence that this should be possible: looking at mainline Linux
and at dwc2_port_suspend(), I see:

* It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE

* It currently sets HPRT0_SUSP

* It currently sets PCGCTL_STOPPCLK specifically in the case where
power down is DWC2_POWER_DOWN_PARAM_NONE.

...I believe that the net effect of my patch ends up doing both those
same two things in hcd_suspend.  That is: when power_down is
DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the
same thing that dwc2_port_suspend() would do in the same case.  Is
that not OK?



> > +                     if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL)
> You make one checking of hsotg->params.power_down mode here.
> > +                             hprt0 &= ~HPRT0_PWR;
> > +                     dwc2_writel(hsotg, hprt0, HPRT0);
> > +             }
> > +             if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> another checking of power_down mode here.

Yeah, we can debate about how to best share/split code.  I'm not in
love with the current structure either.  When I rebased your patches
atop mine I changed this to more fully split them and I agree that was
better.


> > @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
> >               spin_unlock_irqrestore(&hsotg->lock, flags);
> >               dwc2_port_resume(hsotg);
> >       } else {
> > -             dwc2_vbus_supply_init(hsotg);
> > +             if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> > +                     dwc2_vbus_supply_init(hsotg);
> >
> > -             /* Wait for controller to correctly update D+/D- level */
> > -             usleep_range(3000, 5000);
> > +                     /* Wait for controller to correctly update D+/D- level */
> > +                     usleep_range(3000, 5000);
> > +             }
> >
> >               /*
> >                * Clear Port Enable and Port Status changes.
> >
>
> I have tested the patch on HAPS-DX. With this patch or without it when I
> have a device connected core  enters to partial power down and doesn't
> exit from it. So I cannot use the device.

Can you explain what HAPS-DX is?


-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
  2019-04-29 17:33     ` [v2,1/5] " Doug Anderson
@ 2019-04-29 17:33       ` " Doug Anderson
  2019-04-30  6:05       ` [v2,1/5] " Artur Petrosyan
  1 sibling, 0 replies; 50+ messages in thread
From: Doug Anderson @ 2019-04-29 17:33 UTC (permalink / raw)
  To: Artur Petrosyan
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern, amstan,
	linux-rockchip, William Wu, linux-usb, Stefan Wahren, Randy Li,
	zyw, mka, ryandcase, Amelie Delaunay, jwerner, dinguyen,
	Elaine Zhang, Greg Kroah-Hartman, linux-kernel

Hi,

On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> Hi,
>
> On 4/18/2019 04:15, Douglas Anderson wrote:
> > This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> > suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> > b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> > because apparently it broke the Altera SOCFPGA.
> >
> > With all the changes that have happened to dwc2 in the meantime, it's
> > possible that the Altera SOCFPGA will just magically work with this
> > change now.  ...and it would be good to get bus suspend/resume
> > implemented.
> >
> > This change is a forward port of one that's been living in the Chrome
> > OS 3.14 kernel tree.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > This patch was last posted at:
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
> >
> > ...and appears to have died the death of silence.  Maybe it could get
> > some bake time in linuxnext if we can't find any proactive testing?
> >
> > I will also freely admit that I don't know tons about the theory
> > behind this patch.  I'm mostly just re-hashing the original commit
> > from Kever that was reverted since:
> > * Turning on partial power down on rk3288 doesn't "just work".  I
> >    don't get hotplug events.  This is despite dwc2 auto-detecting that
> >    we are power optimized.
> What do you mean by doesn't "just work" ? It seem to me that even after
> adding this patch you don't get issues fixed.
> You mention that you don't get the hotplug events. Please provide dwc2
> debug logs and register dumps on this issue.

I mean that partial power down in the currently upstream driver
doesn't work.  AKA: if I turn on partial power down in the upstream
driver then hotplug events break.  I can try to provide some logs.  On
what exact version of the code do you want logs?  Just your series?
Just my series?  Mainline?  Some attempt at combining both series?  As
I said things seem to sorta work with the combined series.  I can try
to clarify if that's the series you want me to test with.  ...or I can
wait for your next version?


> > @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> >        */
> >       if (!hsotg->bus_suspended) {
> >               hprt0 = dwc2_read_hprt0(hsotg);
> > -             hprt0 |= HPRT0_SUSP;
> > -             hprt0 &= ~HPRT0_PWR;
> > -             dwc2_writel(hsotg, hprt0, HPRT0);
> > -             spin_unlock_irqrestore(&hsotg->lock, flags);
> > -             dwc2_vbus_supply_exit(hsotg);
> > -             spin_lock_irqsave(&hsotg->lock, flags);
> > +             if (hprt0 & HPRT0_CONNSTS) { > +                        hprt0 |= HPRT0_SUSP;
> Here you set "HPRT0_SUSP" bit but what if core doesn't support both
> hibernation and Partial Power down assuming that
> hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE"
> which is 0.

I am by no means an expert on dwc2, but an assumption made in my patch
is that even cores that can't support partial power down can still
save some amount of power when hcd_suspend is called.

Some evidence that this should be possible: looking at mainline Linux
and at dwc2_port_suspend(), I see:

* It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE

* It currently sets HPRT0_SUSP

* It currently sets PCGCTL_STOPPCLK specifically in the case where
power down is DWC2_POWER_DOWN_PARAM_NONE.

...I believe that the net effect of my patch ends up doing both those
same two things in hcd_suspend.  That is: when power_down is
DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the
same thing that dwc2_port_suspend() would do in the same case.  Is
that not OK?



> > +                     if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL)
> You make one checking of hsotg->params.power_down mode here.
> > +                             hprt0 &= ~HPRT0_PWR;
> > +                     dwc2_writel(hsotg, hprt0, HPRT0);
> > +             }
> > +             if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> another checking of power_down mode here.

Yeah, we can debate about how to best share/split code.  I'm not in
love with the current structure either.  When I rebased your patches
atop mine I changed this to more fully split them and I agree that was
better.


> > @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
> >               spin_unlock_irqrestore(&hsotg->lock, flags);
> >               dwc2_port_resume(hsotg);
> >       } else {
> > -             dwc2_vbus_supply_init(hsotg);
> > +             if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> > +                     dwc2_vbus_supply_init(hsotg);
> >
> > -             /* Wait for controller to correctly update D+/D- level */
> > -             usleep_range(3000, 5000);
> > +                     /* Wait for controller to correctly update D+/D- level */
> > +                     usleep_range(3000, 5000);
> > +             }
> >
> >               /*
> >                * Clear Port Enable and Port Status changes.
> >
>
> I have tested the patch on HAPS-DX. With this patch or without it when I
> have a device connected core  enters to partial power down and doesn't
> exit from it. So I cannot use the device.

Can you explain what HAPS-DX is?


-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
@ 2019-04-30  1:23   ` " Rob Herring
  2019-04-30  1:23     ` [PATCH v2 3/5] " Rob Herring
  2019-04-30  5:25     ` [v2,3/5] " Doug Anderson
  0 siblings, 2 replies; 50+ messages in thread
From: Rob Herring @ 2019-04-30  1:23 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern,
	Artur Petrosyan, amstan, linux-rockchip, William Wu, linux-usb,
	Stefan Wahren, Randy Li, zyw, mka, ryandcase, Amelie Delaunay,
	jwerner, dinguyen, Elaine Zhang, devicetree, linux-kernel,
	Greg Kroah-Hartman, Mark Rutland

On Wed, Apr 17, 2019 at 05:13:54PM -0700, Douglas Anderson wrote:
> Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> support remote wakeup.  Allow specifying this as a device tree
> property.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> For relevant prior discussion on this patch, see:
> 
> https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> 
> I didn't make any changes from the prior version since I never found
> out what Rob thought of my previous arguments.  If folks want a
> change, perhaps they could choose from these options:
> 
> 1. Assume that all dwc2 hosts would like to keep their PHY on for
>    suspend if there's a USB wakeup enabled, thus we totally drop this
>    binding.  This doesn't seem super great to me since I'd bet that
>    many devices that use dwc2 weren't designed for USB wakeup (they
>    may not keep enough clocks or rails on) so we might be wasting
>    power for nothing.

1b. Use SoC specific compatible strings to enable/disable remote 
wake-up. We can debate what the default is I guess.

> 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
>    it more obvious that this property is intended both to document
>    that wakeup from suspend is possible and that we need the PHY for
>    said wakeup.
> 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
>    it's implicit that if we can wakeup from suspend that we need to
>    keep the PHY on.  If/when someone shows that a device exists using
>    dwc2 where we can wakeup from suspend without the PHY they can add
>    a new property.
> 
> Changes in v2: None
> 
>  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
>  1 file changed, 3 insertions(+)
>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
  2019-04-30  1:23   ` [v2,3/5] " Rob Herring
@ 2019-04-30  1:23     ` " Rob Herring
  2019-04-30  5:25     ` [v2,3/5] " Doug Anderson
  1 sibling, 0 replies; 50+ messages in thread
From: Rob Herring @ 2019-04-30  1:23 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern,
	Artur Petrosyan, amstan, linux-rockchip, William Wu, linux-usb,
	Stefan Wahren, Randy Li, zyw, mka, ryandcase, Amelie Delaunay,
	jwerner, dinguyen, Elaine Zhang, devicetree, linux-kernel,
	Greg Kroah-Hartman, Mark Rutland

On Wed, Apr 17, 2019 at 05:13:54PM -0700, Douglas Anderson wrote:
> Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> support remote wakeup.  Allow specifying this as a device tree
> property.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> For relevant prior discussion on this patch, see:
> 
> https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> 
> I didn't make any changes from the prior version since I never found
> out what Rob thought of my previous arguments.  If folks want a
> change, perhaps they could choose from these options:
> 
> 1. Assume that all dwc2 hosts would like to keep their PHY on for
>    suspend if there's a USB wakeup enabled, thus we totally drop this
>    binding.  This doesn't seem super great to me since I'd bet that
>    many devices that use dwc2 weren't designed for USB wakeup (they
>    may not keep enough clocks or rails on) so we might be wasting
>    power for nothing.

1b. Use SoC specific compatible strings to enable/disable remote 
wake-up. We can debate what the default is I guess.

> 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
>    it more obvious that this property is intended both to document
>    that wakeup from suspend is possible and that we need the PHY for
>    said wakeup.
> 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
>    it's implicit that if we can wakeup from suspend that we need to
>    keep the PHY on.  If/when someone shows that a device exists using
>    dwc2 where we can wakeup from suspend without the PHY they can add
>    a new property.
> 
> Changes in v2: None
> 
>  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
@ 2019-04-30  5:25     ` " Doug Anderson
  2019-04-30  5:25       ` [PATCH v2 3/5] " Doug Anderson
  0 siblings, 1 reply; 50+ messages in thread
From: Doug Anderson @ 2019-04-30  5:25 UTC (permalink / raw)
  To: Rob Herring
  Cc: Minas Harutyunyan, Felipe Balbi, Heiko Stübner, Alan Stern,
	Artur Petrosyan, Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Dinh Nguyen, Elaine Zhang, devicetree, LKML, Greg Kroah-Hartman,
	Mark Rutland

Hi,

On Mon, Apr 29, 2019 at 6:23 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Apr 17, 2019 at 05:13:54PM -0700, Douglas Anderson wrote:
> > Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> > support remote wakeup.  Allow specifying this as a device tree
> > property.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > For relevant prior discussion on this patch, see:
> >
> > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> >
> > I didn't make any changes from the prior version since I never found
> > out what Rob thought of my previous arguments.  If folks want a
> > change, perhaps they could choose from these options:
> >
> > 1. Assume that all dwc2 hosts would like to keep their PHY on for
> >    suspend if there's a USB wakeup enabled, thus we totally drop this
> >    binding.  This doesn't seem super great to me since I'd bet that
> >    many devices that use dwc2 weren't designed for USB wakeup (they
> >    may not keep enough clocks or rails on) so we might be wasting
> >    power for nothing.
>
> 1b. Use SoC specific compatible strings to enable/disable remote
> wake-up. We can debate what the default is I guess.

Unfortunately it's more than just SoC.  While you need the SoC to be
able to support this type of wakeup, you also need the board design,
firmware design, regulator design, etc.  ...so I don't think we can
just use the SoC specific compatible string.

In fact, while testing this I found that USB wakeup was totally broken
unless I enabled "deep suspend" mode on my system.  Something about
the clocks / wakeup sources in the shallow suspend totally blocked it
and I couldn't figure out what.

...so I believe it really needs to be something where someone has
said: I tested it out on this board and everything is setup properly
to support USB wakeup.


> > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
> >    it more obvious that this property is intended both to document
> >    that wakeup from suspend is possible and that we need the PHY for
> >    said wakeup.
> > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
> >    it's implicit that if we can wakeup from suspend that we need to
> >    keep the PHY on.  If/when someone shows that a device exists using
> >    dwc2 where we can wakeup from suspend without the PHY they can add
> >    a new property.
> >
> > Changes in v2: None
> >
> >  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
> >  1 file changed, 3 insertions(+)

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
  2019-04-30  5:25     ` [v2,3/5] " Doug Anderson
@ 2019-04-30  5:25       ` " Doug Anderson
  0 siblings, 0 replies; 50+ messages in thread
From: Doug Anderson @ 2019-04-30  5:25 UTC (permalink / raw)
  To: Rob Herring
  Cc: Minas Harutyunyan, Felipe Balbi, Heiko Stübner, Alan Stern,
	Artur Petrosyan, Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Dinh Nguyen, Elaine Zhang, devicetree, LKML, Greg Kroah-Hartman,
	Mark Rutland

Hi,

On Mon, Apr 29, 2019 at 6:23 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Apr 17, 2019 at 05:13:54PM -0700, Douglas Anderson wrote:
> > Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> > support remote wakeup.  Allow specifying this as a device tree
> > property.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > For relevant prior discussion on this patch, see:
> >
> > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> >
> > I didn't make any changes from the prior version since I never found
> > out what Rob thought of my previous arguments.  If folks want a
> > change, perhaps they could choose from these options:
> >
> > 1. Assume that all dwc2 hosts would like to keep their PHY on for
> >    suspend if there's a USB wakeup enabled, thus we totally drop this
> >    binding.  This doesn't seem super great to me since I'd bet that
> >    many devices that use dwc2 weren't designed for USB wakeup (they
> >    may not keep enough clocks or rails on) so we might be wasting
> >    power for nothing.
>
> 1b. Use SoC specific compatible strings to enable/disable remote
> wake-up. We can debate what the default is I guess.

Unfortunately it's more than just SoC.  While you need the SoC to be
able to support this type of wakeup, you also need the board design,
firmware design, regulator design, etc.  ...so I don't think we can
just use the SoC specific compatible string.

In fact, while testing this I found that USB wakeup was totally broken
unless I enabled "deep suspend" mode on my system.  Something about
the clocks / wakeup sources in the shallow suspend totally blocked it
and I couldn't figure out what.

...so I believe it really needs to be something where someone has
said: I tested it out on this board and everything is setup properly
to support USB wakeup.


> > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
> >    it more obvious that this property is intended both to document
> >    that wakeup from suspend is possible and that we need the PHY for
> >    said wakeup.
> > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
> >    it's implicit that if we can wakeup from suspend that we need to
> >    keep the PHY on.  If/when someone shows that a device exists using
> >    dwc2 where we can wakeup from suspend without the PHY they can add
> >    a new property.
> >
> > Changes in v2: None
> >
> >  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
> >  1 file changed, 3 insertions(+)

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
@ 2019-04-30  6:05       ` " Artur Petrosyan
  2019-04-30  6:05         ` [PATCH v2 1/5] " Artur Petrosyan
  2019-05-01  1:51         ` [v2,1/5] " Doug Anderson
  0 siblings, 2 replies; 50+ messages in thread
From: Artur Petrosyan @ 2019-04-30  6:05 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern, amstan,
	linux-rockchip, William Wu, linux-usb, Stefan Wahren, Randy Li,
	zyw, mka, ryandcase, Amelie Delaunay, jwerner, dinguyen,
	Elaine Zhang, Greg Kroah-Hartman, linux-kernel

Hi,

On 4/29/2019 21:34, Doug Anderson wrote:
> Hi,
> 
> On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.com> wrote:
>>
>> Hi,
>>
>> On 4/18/2019 04:15, Douglas Anderson wrote:
>>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
>>> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
>>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
>>> because apparently it broke the Altera SOCFPGA.
>>>
>>> With all the changes that have happened to dwc2 in the meantime, it's
>>> possible that the Altera SOCFPGA will just magically work with this
>>> change now.  ...and it would be good to get bus suspend/resume
>>> implemented.
>>>
>>> This change is a forward port of one that's been living in the Chrome
>>> OS 3.14 kernel tree.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>> This patch was last posted at:
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
>>>
>>> ...and appears to have died the death of silence.  Maybe it could get
>>> some bake time in linuxnext if we can't find any proactive testing?
>>>
>>> I will also freely admit that I don't know tons about the theory
>>> behind this patch.  I'm mostly just re-hashing the original commit
>>> from Kever that was reverted since:
>>> * Turning on partial power down on rk3288 doesn't "just work".  I
>>>     don't get hotplug events.  This is despite dwc2 auto-detecting that
>>>     we are power optimized.
>> What do you mean by doesn't "just work" ? It seem to me that even after
>> adding this patch you don't get issues fixed.
>> You mention that you don't get the hotplug events. Please provide dwc2
>> debug logs and register dumps on this issue.
> 
> I mean that partial power down in the currently upstream driver
> doesn't work.  AKA: if I turn on partial power down in the upstream
> driver then hotplug events break.  I can try to provide some logs.  On
> what exact version of the code do you want logs?  Just your series?
> Just my series?  Mainline?  Some attempt at combining both series?  As
> I said things seem to sorta work with the combined series.  I can try
> to clarify if that's the series you want me to test with.  ...or I can
> wait for your next version?
As I said this patch doesn't fix the issue with hotplug. With this patch 
or without the hotplug behaves as it was. I have tested it on our setup.

Have you debugged your patch? Does it make any difference on your setup 
? Does it fix the issue with hotplug?

Try to debug with the following steps.
1. Debug code with just your patch. Capture the logs and provide. So 
that I can see what is difference with your patch.
2. Debug only with my series and see if those issues with hotplug are 
still there.


> 
> 
>>> @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>>>         */
>>>        if (!hsotg->bus_suspended) {
>>>                hprt0 = dwc2_read_hprt0(hsotg);
>>> -             hprt0 |= HPRT0_SUSP;
>>> -             hprt0 &= ~HPRT0_PWR;
>>> -             dwc2_writel(hsotg, hprt0, HPRT0);
>>> -             spin_unlock_irqrestore(&hsotg->lock, flags);
>>> -             dwc2_vbus_supply_exit(hsotg);
>>> -             spin_lock_irqsave(&hsotg->lock, flags);
>>> +             if (hprt0 & HPRT0_CONNSTS) { > +                        hprt0 |= HPRT0_SUSP;
>> Here you set "HPRT0_SUSP" bit but what if core doesn't support both
>> hibernation and Partial Power down assuming that
>> hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE"
>> which is 0.
> 
> I am by no means an expert on dwc2, but an assumption made in my patch
> is that even cores that can't support partial power down can still
> save some amount of power when hcd_suspend is called.
Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ?
> 
> Some evidence that this should be possible: looking at mainline Linux
> and at dwc2_port_suspend(), I see:
> 
> * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE
Currently (without your and my patches) (looking at mainline Linux) the 
function dwc2_port_suspend() is called anyway because its call is issued 
by the system. But it performs entering to suspend only in case of 
DWC2_POWER_DOWN_PARAM_PARTIAL.

This is not an assumption. What I am pointing out is based on debugging 
and before making assumptions without debugging for me seems not ok.

Currently without your patch and without my patches. In the 
dwc2_port_suspend() it will enter to suspend only in case that 
power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the 
code more carefully you will see

  	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
		goto skip_power_saving;

This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip 
power saving.

So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it 
tries to suspend.


> 
> * It currently sets HPRT0_SUSP
> 
> * It currently sets PCGCTL_STOPPCLK specifically in the case where
> power down is DWC2_POWER_DOWN_PARAM_NONE.
It currently (without my and your patches) doesn't set PCGCTL_STOPPCLK 
when power down is DWC2_POWER_DOWN_PARAM_NONE. Because again and again 
when power down is DWC2_POWER_DOWN_PARAM_NONE power saving is skipped.


> 
> ...I believe that the net effect of my patch ends up doing both those
> same two things in hcd_suspend.  That is: when power_down is
> DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the
> same thing that dwc2_port_suspend() would do in the same case.  Is
> that not OK?
No if your patch is doing the same thing as it was doing before what is 
the purpose of the patch ?

My testes show that your patch doesn't fix the issue related partial 
power down.

> 
> 
> 
>>> +                     if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL)
>> You make one checking of hsotg->params.power_down mode here.
>>> +                             hprt0 &= ~HPRT0_PWR;
>>> +                     dwc2_writel(hsotg, hprt0, HPRT0);
>>> +             }
>>> +             if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
>> another checking of power_down mode here.
> 
> Yeah, we can debate about how to best share/split code.  I'm not in
> love with the current structure either.  When I rebased your patches
> atop mine I changed this to more fully split them and I agree that was
> better.
> 
> 
>>> @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>>>                spin_unlock_irqrestore(&hsotg->lock, flags);
>>>                dwc2_port_resume(hsotg);
>>>        } else {
>>> -             dwc2_vbus_supply_init(hsotg);
>>> +             if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
>>> +                     dwc2_vbus_supply_init(hsotg);
>>>
>>> -             /* Wait for controller to correctly update D+/D- level */
>>> -             usleep_range(3000, 5000);
>>> +                     /* Wait for controller to correctly update D+/D- level */
>>> +                     usleep_range(3000, 5000);
>>> +             }
>>>
>>>                /*
>>>                 * Clear Port Enable and Port Status changes.
>>>
>>
>> I have tested the patch on HAPS-DX. With this patch or without it when I
>> have a device connected core  enters to partial power down and doesn't
>> exit from it. So I cannot use the device.
> 
> Can you explain what HAPS-DX is?
It is the general setup to perform our use case testes.
For more information about the details you can google about it.

> 
> 
> -Doug
>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
  2019-04-30  6:05       ` [v2,1/5] " Artur Petrosyan
@ 2019-04-30  6:05         ` " Artur Petrosyan
  2019-05-01  1:51         ` [v2,1/5] " Doug Anderson
  1 sibling, 0 replies; 50+ messages in thread
From: Artur Petrosyan @ 2019-04-30  6:05 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern, amstan,
	linux-rockchip, William Wu, linux-usb, Stefan Wahren, Randy Li,
	zyw, mka, ryandcase, Amelie Delaunay, jwerner, dinguyen,
	Elaine Zhang, Greg Kroah-Hartman, linux-kernel

Hi,

On 4/29/2019 21:34, Doug Anderson wrote:
> Hi,
> 
> On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.com> wrote:
>>
>> Hi,
>>
>> On 4/18/2019 04:15, Douglas Anderson wrote:
>>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
>>> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
>>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
>>> because apparently it broke the Altera SOCFPGA.
>>>
>>> With all the changes that have happened to dwc2 in the meantime, it's
>>> possible that the Altera SOCFPGA will just magically work with this
>>> change now.  ...and it would be good to get bus suspend/resume
>>> implemented.
>>>
>>> This change is a forward port of one that's been living in the Chrome
>>> OS 3.14 kernel tree.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>> This patch was last posted at:
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
>>>
>>> ...and appears to have died the death of silence.  Maybe it could get
>>> some bake time in linuxnext if we can't find any proactive testing?
>>>
>>> I will also freely admit that I don't know tons about the theory
>>> behind this patch.  I'm mostly just re-hashing the original commit
>>> from Kever that was reverted since:
>>> * Turning on partial power down on rk3288 doesn't "just work".  I
>>>     don't get hotplug events.  This is despite dwc2 auto-detecting that
>>>     we are power optimized.
>> What do you mean by doesn't "just work" ? It seem to me that even after
>> adding this patch you don't get issues fixed.
>> You mention that you don't get the hotplug events. Please provide dwc2
>> debug logs and register dumps on this issue.
> 
> I mean that partial power down in the currently upstream driver
> doesn't work.  AKA: if I turn on partial power down in the upstream
> driver then hotplug events break.  I can try to provide some logs.  On
> what exact version of the code do you want logs?  Just your series?
> Just my series?  Mainline?  Some attempt at combining both series?  As
> I said things seem to sorta work with the combined series.  I can try
> to clarify if that's the series you want me to test with.  ...or I can
> wait for your next version?
As I said this patch doesn't fix the issue with hotplug. With this patch 
or without the hotplug behaves as it was. I have tested it on our setup.

Have you debugged your patch? Does it make any difference on your setup 
? Does it fix the issue with hotplug?

Try to debug with the following steps.
1. Debug code with just your patch. Capture the logs and provide. So 
that I can see what is difference with your patch.
2. Debug only with my series and see if those issues with hotplug are 
still there.


> 
> 
>>> @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>>>         */
>>>        if (!hsotg->bus_suspended) {
>>>                hprt0 = dwc2_read_hprt0(hsotg);
>>> -             hprt0 |= HPRT0_SUSP;
>>> -             hprt0 &= ~HPRT0_PWR;
>>> -             dwc2_writel(hsotg, hprt0, HPRT0);
>>> -             spin_unlock_irqrestore(&hsotg->lock, flags);
>>> -             dwc2_vbus_supply_exit(hsotg);
>>> -             spin_lock_irqsave(&hsotg->lock, flags);
>>> +             if (hprt0 & HPRT0_CONNSTS) { > +                        hprt0 |= HPRT0_SUSP;
>> Here you set "HPRT0_SUSP" bit but what if core doesn't support both
>> hibernation and Partial Power down assuming that
>> hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE"
>> which is 0.
> 
> I am by no means an expert on dwc2, but an assumption made in my patch
> is that even cores that can't support partial power down can still
> save some amount of power when hcd_suspend is called.
Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ?
> 
> Some evidence that this should be possible: looking at mainline Linux
> and at dwc2_port_suspend(), I see:
> 
> * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE
Currently (without your and my patches) (looking at mainline Linux) the 
function dwc2_port_suspend() is called anyway because its call is issued 
by the system. But it performs entering to suspend only in case of 
DWC2_POWER_DOWN_PARAM_PARTIAL.

This is not an assumption. What I am pointing out is based on debugging 
and before making assumptions without debugging for me seems not ok.

Currently without your patch and without my patches. In the 
dwc2_port_suspend() it will enter to suspend only in case that 
power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the 
code more carefully you will see

  	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
		goto skip_power_saving;

This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip 
power saving.

So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it 
tries to suspend.


> 
> * It currently sets HPRT0_SUSP
> 
> * It currently sets PCGCTL_STOPPCLK specifically in the case where
> power down is DWC2_POWER_DOWN_PARAM_NONE.
It currently (without my and your patches) doesn't set PCGCTL_STOPPCLK 
when power down is DWC2_POWER_DOWN_PARAM_NONE. Because again and again 
when power down is DWC2_POWER_DOWN_PARAM_NONE power saving is skipped.


> 
> ...I believe that the net effect of my patch ends up doing both those
> same two things in hcd_suspend.  That is: when power_down is
> DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the
> same thing that dwc2_port_suspend() would do in the same case.  Is
> that not OK?
No if your patch is doing the same thing as it was doing before what is 
the purpose of the patch ?

My testes show that your patch doesn't fix the issue related partial 
power down.

> 
> 
> 
>>> +                     if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL)
>> You make one checking of hsotg->params.power_down mode here.
>>> +                             hprt0 &= ~HPRT0_PWR;
>>> +                     dwc2_writel(hsotg, hprt0, HPRT0);
>>> +             }
>>> +             if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
>> another checking of power_down mode here.
> 
> Yeah, we can debate about how to best share/split code.  I'm not in
> love with the current structure either.  When I rebased your patches
> atop mine I changed this to more fully split them and I agree that was
> better.
> 
> 
>>> @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>>>                spin_unlock_irqrestore(&hsotg->lock, flags);
>>>                dwc2_port_resume(hsotg);
>>>        } else {
>>> -             dwc2_vbus_supply_init(hsotg);
>>> +             if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
>>> +                     dwc2_vbus_supply_init(hsotg);
>>>
>>> -             /* Wait for controller to correctly update D+/D- level */
>>> -             usleep_range(3000, 5000);
>>> +                     /* Wait for controller to correctly update D+/D- level */
>>> +                     usleep_range(3000, 5000);
>>> +             }
>>>
>>>                /*
>>>                 * Clear Port Enable and Port Status changes.
>>>
>>
>> I have tested the patch on HAPS-DX. With this patch or without it when I
>> have a device connected core  enters to partial power down and doesn't
>> exit from it. So I cannot use the device.
> 
> Can you explain what HAPS-DX is?
It is the general setup to perform our use case testes.
For more information about the details you can google about it.

> 
> 
> -Doug
> 


-- 
Regards,
Artur

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
@ 2019-05-01  1:51         ` " Doug Anderson
  2019-05-01  1:51           ` [PATCH v2 1/5] " Doug Anderson
  2019-05-03  8:20           ` [v2,1/5] " Artur Petrosyan
  0 siblings, 2 replies; 50+ messages in thread
From: Doug Anderson @ 2019-05-01  1:51 UTC (permalink / raw)
  To: Artur Petrosyan
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern, amstan,
	linux-rockchip, William Wu, linux-usb, Stefan Wahren, Randy Li,
	zyw, mka, ryandcase, Amelie Delaunay, jwerner, dinguyen,
	Elaine Zhang, Greg Kroah-Hartman, linux-kernel

Hi,

On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> Hi,
>
> On 4/29/2019 21:34, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan
> > <Arthur.Petrosyan@synopsys.com> wrote:
> >>
> >> Hi,
> >>
> >> On 4/18/2019 04:15, Douglas Anderson wrote:
> >>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> >>> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> >>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> >>> because apparently it broke the Altera SOCFPGA.
> >>>
> >>> With all the changes that have happened to dwc2 in the meantime, it's
> >>> possible that the Altera SOCFPGA will just magically work with this
> >>> change now.  ...and it would be good to get bus suspend/resume
> >>> implemented.
> >>>
> >>> This change is a forward port of one that's been living in the Chrome
> >>> OS 3.14 kernel tree.
> >>>
> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>> ---
> >>> This patch was last posted at:
> >>>
> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
> >>>
> >>> ...and appears to have died the death of silence.  Maybe it could get
> >>> some bake time in linuxnext if we can't find any proactive testing?
> >>>
> >>> I will also freely admit that I don't know tons about the theory
> >>> behind this patch.  I'm mostly just re-hashing the original commit
> >>> from Kever that was reverted since:
> >>> * Turning on partial power down on rk3288 doesn't "just work".  I
> >>>     don't get hotplug events.  This is despite dwc2 auto-detecting that
> >>>     we are power optimized.
> >> What do you mean by doesn't "just work" ? It seem to me that even after
> >> adding this patch you don't get issues fixed.
> >> You mention that you don't get the hotplug events. Please provide dwc2
> >> debug logs and register dumps on this issue.
> >
> > I mean that partial power down in the currently upstream driver
> > doesn't work.  AKA: if I turn on partial power down in the upstream
> > driver then hotplug events break.  I can try to provide some logs.  On
> > what exact version of the code do you want logs?  Just your series?
> > Just my series?  Mainline?  Some attempt at combining both series?  As
> > I said things seem to sorta work with the combined series.  I can try
> > to clarify if that's the series you want me to test with.  ...or I can
> > wait for your next version?
> As I said this patch doesn't fix the issue with hotplug. With this patch
> or without the hotplug behaves as it was. I have tested it on our setup.
>
> Have you debugged your patch? Does it make any difference on your setup
> ? Does it fix the issue with hotplug?

I think we're still not taking on the same page.

My patch makes no attempt to make partial power down mode work.  My
patch attempts to make things work a little better when using
DWC2_POWER_DOWN_PARAM_NONE.  There is no use testing my patch with
partial power down as it shouldn't have any impact there.


> > I am by no means an expert on dwc2, but an assumption made in my patch
> > is that even cores that can't support partial power down can still
> > save some amount of power when hcd_suspend is called.
> Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ?
> >
> > Some evidence that this should be possible: looking at mainline Linux
> > and at dwc2_port_suspend(), I see:
> >
> > * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE
> Currently (without your and my patches) (looking at mainline Linux) the
> function dwc2_port_suspend() is called anyway because its call is issued
> by the system. But it performs entering to suspend only in case of
> DWC2_POWER_DOWN_PARAM_PARTIAL.
>
> This is not an assumption. What I am pointing out is based on debugging
> and before making assumptions without debugging for me seems not ok.
>
> Currently without your patch and without my patches. In the
> dwc2_port_suspend() it will enter to suspend only in case that
> power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the
> code more carefully you will see
>
>         if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
>                 goto skip_power_saving;
>
> This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip
> power saving.
>
> So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it
> tries to suspend.

We must be looking at different code.  I'm looking at Linux's tree, AKA:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc2/hcd.c#n3488

I took a mainline kernel ("v5.1-rc7-5-g83a50840e72a") and added
printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP and
PCGCTL_STOPPCLK in dwc2_port_suspend().

[  454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP
[  454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK

...and just to confirm:

# grep '^power' /sys/kernel/debug/*.usb/params
/sys/kernel/debug/ff540000.usb/params:power_down                    : 0
/sys/kernel/debug/ff580000.usb/params:power_down                    : 0

So I'm really quite convinced that on mainline Linux with
DWC2_POWER_DOWN_PARAM_NONE that dwc2_port_suspend() sets HPRT0_SUSP
and PCGCTL_STOPPCLK.


> > ...I believe that the net effect of my patch ends up doing both those
> > same two things in hcd_suspend.  That is: when power_down is
> > DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the
> > same thing that dwc2_port_suspend() would do in the same case.  Is
> > that not OK?
> No if your patch is doing the same thing as it was doing before what is
> the purpose of the patch ?

The purpose is to make _dwc2_hcd_suspend() work more correctly in the
case where power_down is DWC2_POWER_DOWN_PARAM_NONE


> My testes show that your patch doesn't fix the issue related partial
> power down.

Right.  I have been trying to say that my patch doesn't do anything at
all for partial power down.  I am simply trying to make
DWC2_POWER_DOWN_PARAM_NONE work more correctly.

I haven't run all the power consumption tests in quite a long time and
I'll try to get it hooked up tomorrow to confirm that my patch really
truly is still needed to help with power consumption.  I did confirm
that at least there are cases where _dwc2_hcd_suspend() is called and
my patch is what sets the important bits.

-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
  2019-05-01  1:51         ` [v2,1/5] " Doug Anderson
@ 2019-05-01  1:51           ` " Doug Anderson
  2019-05-03  8:20           ` [v2,1/5] " Artur Petrosyan
  1 sibling, 0 replies; 50+ messages in thread
From: Doug Anderson @ 2019-05-01  1:51 UTC (permalink / raw)
  To: Artur Petrosyan
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern, amstan,
	linux-rockchip, William Wu, linux-usb, Stefan Wahren, Randy Li,
	zyw, mka, ryandcase, Amelie Delaunay, jwerner, dinguyen,
	Elaine Zhang, Greg Kroah-Hartman, linux-kernel

Hi,

On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> Hi,
>
> On 4/29/2019 21:34, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan
> > <Arthur.Petrosyan@synopsys.com> wrote:
> >>
> >> Hi,
> >>
> >> On 4/18/2019 04:15, Douglas Anderson wrote:
> >>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> >>> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> >>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> >>> because apparently it broke the Altera SOCFPGA.
> >>>
> >>> With all the changes that have happened to dwc2 in the meantime, it's
> >>> possible that the Altera SOCFPGA will just magically work with this
> >>> change now.  ...and it would be good to get bus suspend/resume
> >>> implemented.
> >>>
> >>> This change is a forward port of one that's been living in the Chrome
> >>> OS 3.14 kernel tree.
> >>>
> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>> ---
> >>> This patch was last posted at:
> >>>
> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
> >>>
> >>> ...and appears to have died the death of silence.  Maybe it could get
> >>> some bake time in linuxnext if we can't find any proactive testing?
> >>>
> >>> I will also freely admit that I don't know tons about the theory
> >>> behind this patch.  I'm mostly just re-hashing the original commit
> >>> from Kever that was reverted since:
> >>> * Turning on partial power down on rk3288 doesn't "just work".  I
> >>>     don't get hotplug events.  This is despite dwc2 auto-detecting that
> >>>     we are power optimized.
> >> What do you mean by doesn't "just work" ? It seem to me that even after
> >> adding this patch you don't get issues fixed.
> >> You mention that you don't get the hotplug events. Please provide dwc2
> >> debug logs and register dumps on this issue.
> >
> > I mean that partial power down in the currently upstream driver
> > doesn't work.  AKA: if I turn on partial power down in the upstream
> > driver then hotplug events break.  I can try to provide some logs.  On
> > what exact version of the code do you want logs?  Just your series?
> > Just my series?  Mainline?  Some attempt at combining both series?  As
> > I said things seem to sorta work with the combined series.  I can try
> > to clarify if that's the series you want me to test with.  ...or I can
> > wait for your next version?
> As I said this patch doesn't fix the issue with hotplug. With this patch
> or without the hotplug behaves as it was. I have tested it on our setup.
>
> Have you debugged your patch? Does it make any difference on your setup
> ? Does it fix the issue with hotplug?

I think we're still not taking on the same page.

My patch makes no attempt to make partial power down mode work.  My
patch attempts to make things work a little better when using
DWC2_POWER_DOWN_PARAM_NONE.  There is no use testing my patch with
partial power down as it shouldn't have any impact there.


> > I am by no means an expert on dwc2, but an assumption made in my patch
> > is that even cores that can't support partial power down can still
> > save some amount of power when hcd_suspend is called.
> Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ?
> >
> > Some evidence that this should be possible: looking at mainline Linux
> > and at dwc2_port_suspend(), I see:
> >
> > * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE
> Currently (without your and my patches) (looking at mainline Linux) the
> function dwc2_port_suspend() is called anyway because its call is issued
> by the system. But it performs entering to suspend only in case of
> DWC2_POWER_DOWN_PARAM_PARTIAL.
>
> This is not an assumption. What I am pointing out is based on debugging
> and before making assumptions without debugging for me seems not ok.
>
> Currently without your patch and without my patches. In the
> dwc2_port_suspend() it will enter to suspend only in case that
> power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the
> code more carefully you will see
>
>         if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
>                 goto skip_power_saving;
>
> This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip
> power saving.
>
> So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it
> tries to suspend.

We must be looking at different code.  I'm looking at Linux's tree, AKA:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc2/hcd.c#n3488

I took a mainline kernel ("v5.1-rc7-5-g83a50840e72a") and added
printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP and
PCGCTL_STOPPCLK in dwc2_port_suspend().

[  454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP
[  454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK

...and just to confirm:

# grep '^power' /sys/kernel/debug/*.usb/params
/sys/kernel/debug/ff540000.usb/params:power_down                    : 0
/sys/kernel/debug/ff580000.usb/params:power_down                    : 0

So I'm really quite convinced that on mainline Linux with
DWC2_POWER_DOWN_PARAM_NONE that dwc2_port_suspend() sets HPRT0_SUSP
and PCGCTL_STOPPCLK.


> > ...I believe that the net effect of my patch ends up doing both those
> > same two things in hcd_suspend.  That is: when power_down is
> > DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the
> > same thing that dwc2_port_suspend() would do in the same case.  Is
> > that not OK?
> No if your patch is doing the same thing as it was doing before what is
> the purpose of the patch ?

The purpose is to make _dwc2_hcd_suspend() work more correctly in the
case where power_down is DWC2_POWER_DOWN_PARAM_NONE


> My testes show that your patch doesn't fix the issue related partial
> power down.

Right.  I have been trying to say that my patch doesn't do anything at
all for partial power down.  I am simply trying to make
DWC2_POWER_DOWN_PARAM_NONE work more correctly.

I haven't run all the power consumption tests in quite a long time and
I'll try to get it hooked up tomorrow to confirm that my patch really
truly is still needed to help with power consumption.  I did confirm
that at least there are cases where _dwc2_hcd_suspend() is called and
my patch is what sets the important bits.

-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
@ 2019-05-01 23:58   ` " Doug Anderson
  2019-05-01 23:58     ` [PATCH v2 1/5] " Doug Anderson
  2019-05-03  8:25     ` [v2,1/5] " Artur Petrosyan
  0 siblings, 2 replies; 50+ messages in thread
From: Doug Anderson @ 2019-05-01 23:58 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, Heiko Stübner, Artur Petrosyan
  Cc: Alan Stern, Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Dinh Nguyen, Elaine Zhang, Greg Kroah-Hartman, LKML

Hi,


On Wed, Apr 17, 2019 at 5:15 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> because apparently it broke the Altera SOCFPGA.
>
> With all the changes that have happened to dwc2 in the meantime, it's
> possible that the Altera SOCFPGA will just magically work with this
> change now.  ...and it would be good to get bus suspend/resume
> implemented.
>
> This change is a forward port of one that's been living in the Chrome
> OS 3.14 kernel tree.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This patch was last posted at:
>
> https://lkml.kernel.org/r/1446237173-15263-1-git-send-email-dianders@chromium.org
>
> ...and appears to have died the death of silence.  Maybe it could get
> some bake time in linuxnext if we can't find any proactive testing?
>
> I will also freely admit that I don't know tons about the theory
> behind this patch.  I'm mostly just re-hashing the original commit
> from Kever that was reverted since:
> * Turning on partial power down on rk3288 doesn't "just work".  I
>   don't get hotplug events.  This is despite dwc2 auto-detecting that
>   we are power optimized.
> * If we don't do something like this commit we don't get into as low
>   of a power mode.

OK, I spent the day digging more into this patch to confirm that it's
really the right thing to do.  ...and it still seems to be.

First off: I'm pretty sure the above sentence "If we don't do
something like this commit we don't get into as low of a power mode."
is totally wrong.  Luckily it's "after the cut" and not part of the
commit message.  Specifically I did a bunch of power testing and I
couldn't find any instance saving power after this patch.

...but, then I looked more carefully at all the history of this
commit.  I ended up at:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/306265/

...where I said that this fixes a resume speed regression.  More
details could be found at the linked bug, AKA:

https://bugs.chromium.org/p/chromium/issues/detail?id=548336

...but, sadly, I wasn't as verbose as I usually am and didn't describe
my exact testing setup.  So I tried to reproduce.  ...and I was able
to.  I tested on an rk3288-veyron-jerry with an empty USB hub plugged
into the left port (the host port) and my "servo 2" debug board hooked
up to the right port.  The "power_Resume" test in Chrome OS certainly
showed a regression in 3.14 when doing a suspend/resume cycle.


Digging into the logs in 3.14, before this patch I saw this in the logs:

usb 3-1: reset high-speed USB device number 2 using dwc2
usb 3-1.7: reset high-speed USB device number 3 using dwc2

...after this patch:

usb 3-1: USB disconnect, device number 2
usb 3-1.7: USB disconnect, device number 3
usb 3-1: new high-speed USB device number 4 using dwc2
usb 3-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00
usb 3-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0
usb 3-1: Product: USB 2.0 Hub [MTT]
usb 3-1.7: new high-speed USB device number 5 using dwc2
usb 3-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11
usb 3-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0
usb 3-1.7: Product: USB 2.0 Hub

...so basically my belief is that without this patch we're just sorta
leaving the device hanging and it get confused on resume.  After this
patch we behave slightly better.

I tested on 4.19 and found much the same.  There:

usb 2-1: reset high-speed USB device number 2 using dwc2
usb 2-1.7: reset high-speed USB device number 3 using dwc2

vs.

usb 2-1.7: USB disconnect, device number 3
usb 2-1: USB disconnect, device number 2
usb 2-1: new high-speed USB device number 4 using dwc2
usb 2-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00
usb 2-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0
usb 2-1: Product: USB 2.0 Hub [MTT]
usb 2-1.7: new high-speed USB device number 5 using dwc2
usb 2-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11
usb 2-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0
usb 2-1.7: Product: USB 2.0 Hub


On 4.19 I didn't actually notice a the same resume time regression,
presumably because things are happening more asynchronously there (I
didn't confirm this).  ...but in any case it seems like the right
thing to do to actually do the suspend.


I'll also re-iterate once more that I'm not claiming that my patch
helps with "partial power down".  It merely makes the "power savings
disabled" case work more properly.


I'll also note that my patch is already in Felipe's "testing/next"
branch which I continue to believe is correct and good.

-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
  2019-05-01 23:58   ` [v2,1/5] " Doug Anderson
@ 2019-05-01 23:58     ` " Doug Anderson
  2019-05-03  8:25     ` [v2,1/5] " Artur Petrosyan
  1 sibling, 0 replies; 50+ messages in thread
From: Doug Anderson @ 2019-05-01 23:58 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi, Heiko Stübner, Artur Petrosyan
  Cc: Alan Stern, Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Dinh Nguyen, Elaine Zhang, Greg Kroah-Hartman, LKML

Hi,


On Wed, Apr 17, 2019 at 5:15 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> because apparently it broke the Altera SOCFPGA.
>
> With all the changes that have happened to dwc2 in the meantime, it's
> possible that the Altera SOCFPGA will just magically work with this
> change now.  ...and it would be good to get bus suspend/resume
> implemented.
>
> This change is a forward port of one that's been living in the Chrome
> OS 3.14 kernel tree.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This patch was last posted at:
>
> https://lkml.kernel.org/r/1446237173-15263-1-git-send-email-dianders@chromium.org
>
> ...and appears to have died the death of silence.  Maybe it could get
> some bake time in linuxnext if we can't find any proactive testing?
>
> I will also freely admit that I don't know tons about the theory
> behind this patch.  I'm mostly just re-hashing the original commit
> from Kever that was reverted since:
> * Turning on partial power down on rk3288 doesn't "just work".  I
>   don't get hotplug events.  This is despite dwc2 auto-detecting that
>   we are power optimized.
> * If we don't do something like this commit we don't get into as low
>   of a power mode.

OK, I spent the day digging more into this patch to confirm that it's
really the right thing to do.  ...and it still seems to be.

First off: I'm pretty sure the above sentence "If we don't do
something like this commit we don't get into as low of a power mode."
is totally wrong.  Luckily it's "after the cut" and not part of the
commit message.  Specifically I did a bunch of power testing and I
couldn't find any instance saving power after this patch.

...but, then I looked more carefully at all the history of this
commit.  I ended up at:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/306265/

...where I said that this fixes a resume speed regression.  More
details could be found at the linked bug, AKA:

https://bugs.chromium.org/p/chromium/issues/detail?id=548336

...but, sadly, I wasn't as verbose as I usually am and didn't describe
my exact testing setup.  So I tried to reproduce.  ...and I was able
to.  I tested on an rk3288-veyron-jerry with an empty USB hub plugged
into the left port (the host port) and my "servo 2" debug board hooked
up to the right port.  The "power_Resume" test in Chrome OS certainly
showed a regression in 3.14 when doing a suspend/resume cycle.


Digging into the logs in 3.14, before this patch I saw this in the logs:

usb 3-1: reset high-speed USB device number 2 using dwc2
usb 3-1.7: reset high-speed USB device number 3 using dwc2

...after this patch:

usb 3-1: USB disconnect, device number 2
usb 3-1.7: USB disconnect, device number 3
usb 3-1: new high-speed USB device number 4 using dwc2
usb 3-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00
usb 3-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0
usb 3-1: Product: USB 2.0 Hub [MTT]
usb 3-1.7: new high-speed USB device number 5 using dwc2
usb 3-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11
usb 3-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0
usb 3-1.7: Product: USB 2.0 Hub

...so basically my belief is that without this patch we're just sorta
leaving the device hanging and it get confused on resume.  After this
patch we behave slightly better.

I tested on 4.19 and found much the same.  There:

usb 2-1: reset high-speed USB device number 2 using dwc2
usb 2-1.7: reset high-speed USB device number 3 using dwc2

vs.

usb 2-1.7: USB disconnect, device number 3
usb 2-1: USB disconnect, device number 2
usb 2-1: new high-speed USB device number 4 using dwc2
usb 2-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00
usb 2-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0
usb 2-1: Product: USB 2.0 Hub [MTT]
usb 2-1.7: new high-speed USB device number 5 using dwc2
usb 2-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11
usb 2-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0
usb 2-1.7: Product: USB 2.0 Hub


On 4.19 I didn't actually notice a the same resume time regression,
presumably because things are happening more asynchronously there (I
didn't confirm this).  ...but in any case it seems like the right
thing to do to actually do the suspend.


I'll also re-iterate once more that I'm not claiming that my patch
helps with "partial power down".  It merely makes the "power savings
disabled" case work more properly.


I'll also note that my patch is already in Felipe's "testing/next"
branch which I continue to believe is correct and good.

-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
@ 2019-05-02 18:36     ` " Doug Anderson
  2019-05-02 18:36       ` [PATCH v2 3/5] " Doug Anderson
  0 siblings, 1 reply; 50+ messages in thread
From: Doug Anderson @ 2019-05-02 18:36 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Minas Harutyunyan, Heiko Stübner, Alan Stern,
	Artur Petrosyan, Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Dinh Nguyen, Elaine Zhang, devicetree, LKML, Rob Herring,
	Greg Kroah-Hartman, Mark Rutland

Hi,

On Thu, Apr 25, 2019 at 5:40 AM Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Douglas Anderson <dianders@chromium.org> writes:
>
> > Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> > support remote wakeup.  Allow specifying this as a device tree
> > property.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > For relevant prior discussion on this patch, see:
> >
> > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> >
> > I didn't make any changes from the prior version since I never found
> > out what Rob thought of my previous arguments.  If folks want a
> > change, perhaps they could choose from these options:
> >
> > 1. Assume that all dwc2 hosts would like to keep their PHY on for
> >    suspend if there's a USB wakeup enabled, thus we totally drop this
> >    binding.  This doesn't seem super great to me since I'd bet that
> >    many devices that use dwc2 weren't designed for USB wakeup (they
> >    may not keep enough clocks or rails on) so we might be wasting
> >    power for nothing.
> > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
> >    it more obvious that this property is intended both to document
> >    that wakeup from suspend is possible and that we need the PHY for
> >    said wakeup.
> > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
> >    it's implicit that if we can wakeup from suspend that we need to
> >    keep the PHY on.  If/when someone shows that a device exists using
> >    dwc2 where we can wakeup from suspend without the PHY they can add
> >    a new property.
> >
> > Changes in v2: None
> >
> >  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
> >  1 file changed, 3 insertions(+)
>
> checking file Documentation/devicetree/bindings/usb/dwc2.txt
> Hunk #1 FAILED at 37.
> Hunk #2 succeeded at 52 (offset -1 lines).
> 1 out of 2 hunks FAILED

Can you try applying this and the next two patches again?  ...or let
me know that you'd like me to repost?

Thanks!

-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
  2019-05-02 18:36     ` [v2,3/5] " Doug Anderson
@ 2019-05-02 18:36       ` " Doug Anderson
  0 siblings, 0 replies; 50+ messages in thread
From: Doug Anderson @ 2019-05-02 18:36 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Minas Harutyunyan, Heiko Stübner, Alan Stern,
	Artur Petrosyan, Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Dinh Nguyen, Elaine Zhang, devicetree, LKML, Rob Herring,
	Greg Kroah-Hartman, Mark Rutland

Hi,

On Thu, Apr 25, 2019 at 5:40 AM Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Douglas Anderson <dianders@chromium.org> writes:
>
> > Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> > support remote wakeup.  Allow specifying this as a device tree
> > property.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > For relevant prior discussion on this patch, see:
> >
> > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> >
> > I didn't make any changes from the prior version since I never found
> > out what Rob thought of my previous arguments.  If folks want a
> > change, perhaps they could choose from these options:
> >
> > 1. Assume that all dwc2 hosts would like to keep their PHY on for
> >    suspend if there's a USB wakeup enabled, thus we totally drop this
> >    binding.  This doesn't seem super great to me since I'd bet that
> >    many devices that use dwc2 weren't designed for USB wakeup (they
> >    may not keep enough clocks or rails on) so we might be wasting
> >    power for nothing.
> > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
> >    it more obvious that this property is intended both to document
> >    that wakeup from suspend is possible and that we need the PHY for
> >    said wakeup.
> > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
> >    it's implicit that if we can wakeup from suspend that we need to
> >    keep the PHY on.  If/when someone shows that a device exists using
> >    dwc2 where we can wakeup from suspend without the PHY they can add
> >    a new property.
> >
> > Changes in v2: None
> >
> >  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
> >  1 file changed, 3 insertions(+)
>
> checking file Documentation/devicetree/bindings/usb/dwc2.txt
> Hunk #1 FAILED at 37.
> Hunk #2 succeeded at 52 (offset -1 lines).
> 1 out of 2 hunks FAILED

Can you try applying this and the next two patches again?  ...or let
me know that you'd like me to repost?

Thanks!

-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
@ 2019-05-03  8:20           ` " Artur Petrosyan
  2019-05-03  8:20             ` [PATCH v2 1/5] " Artur Petrosyan
  2019-05-03 15:03             ` [v2,1/5] " Doug Anderson
  0 siblings, 2 replies; 50+ messages in thread
From: Artur Petrosyan @ 2019-05-03  8:20 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern, amstan,
	linux-rockchip, William Wu, linux-usb, Stefan Wahren, Randy Li,
	zyw, mka, ryandcase, Amelie Delaunay, jwerner, dinguyen,
	Elaine Zhang, Greg Kroah-Hartman, linux-kernel

On 5/1/2019 05:57, Doug Anderson wrote:
> Hi,
> 
> On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.com> wrote:
>>
>> Hi,
>>
>> On 4/29/2019 21:34, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan
>>> <Arthur.Petrosyan@synopsys.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 4/18/2019 04:15, Douglas Anderson wrote:
>>>>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
>>>>> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
>>>>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
>>>>> because apparently it broke the Altera SOCFPGA.
>>>>>
>>>>> With all the changes that have happened to dwc2 in the meantime, it's
>>>>> possible that the Altera SOCFPGA will just magically work with this
>>>>> change now.  ...and it would be good to get bus suspend/resume
>>>>> implemented.
>>>>>
>>>>> This change is a forward port of one that's been living in the Chrome
>>>>> OS 3.14 kernel tree.
>>>>>
>>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>>> ---
>>>>> This patch was last posted at:
>>>>>
>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
>>>>>
>>>>> ...and appears to have died the death of silence.  Maybe it could get
>>>>> some bake time in linuxnext if we can't find any proactive testing?
>>>>>
>>>>> I will also freely admit that I don't know tons about the theory
>>>>> behind this patch.  I'm mostly just re-hashing the original commit
>>>>> from Kever that was reverted since:
>>>>> * Turning on partial power down on rk3288 doesn't "just work".  I
>>>>>      don't get hotplug events.  This is despite dwc2 auto-detecting that
>>>>>      we are power optimized.
>>>> What do you mean by doesn't "just work" ? It seem to me that even after
>>>> adding this patch you don't get issues fixed.
>>>> You mention that you don't get the hotplug events. Please provide dwc2
>>>> debug logs and register dumps on this issue.
>>>
>>> I mean that partial power down in the currently upstream driver
>>> doesn't work.  AKA: if I turn on partial power down in the upstream
>>> driver then hotplug events break.  I can try to provide some logs.  On
>>> what exact version of the code do you want logs?  Just your series?
>>> Just my series?  Mainline?  Some attempt at combining both series?  As
>>> I said things seem to sorta work with the combined series.  I can try
>>> to clarify if that's the series you want me to test with.  ...or I can
>>> wait for your next version?
>> As I said this patch doesn't fix the issue with hotplug. With this patch
>> or without the hotplug behaves as it was. I have tested it on our setup.
>>
>> Have you debugged your patch? Does it make any difference on your setup
>> ? Does it fix the issue with hotplug?
> 
> I think we're still not taking on the same page.
> 
> My patch makes no attempt to make partial power down mode work.  My
> patch attempts to make things work a little better when using
> DWC2_POWER_DOWN_PARAM_NONE.  There is no use testing my patch with
> partial power down as it shouldn't have any impact there.
> 
> 
>>> I am by no means an expert on dwc2, but an assumption made in my patch
>>> is that even cores that can't support partial power down can still
>>> save some amount of power when hcd_suspend is called.
>> Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ?
>>>
>>> Some evidence that this should be possible: looking at mainline Linux
>>> and at dwc2_port_suspend(), I see:
>>>
>>> * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE
>> Currently (without your and my patches) (looking at mainline Linux) the
>> function dwc2_port_suspend() is called anyway because its call is issued
>> by the system. But it performs entering to suspend only in case of
>> DWC2_POWER_DOWN_PARAM_PARTIAL.
>>
>> This is not an assumption. What I am pointing out is based on debugging
>> and before making assumptions without debugging for me seems not ok.
>>
>> Currently without your patch and without my patches. In the
>> dwc2_port_suspend() it will enter to suspend only in case that
>> power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the
>> code more carefully you will see
>>
>>          if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
>>                  goto skip_power_saving;
>>
>> This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip
>> power saving.
>>
>> So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it
>> tries to suspend.
> 
> We must be looking at different code.  I'm looking at Linux's tree, AKA:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_dwc2_hcd.c-23n3488&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=IWkDOOGTr0q-H1piDv2KOZe_Hnrz18g6rXFx-DsTuv4&s=AHu2iOKkybliRGtIfN7cF5p070UdvUKTYJsyAKYojis&e=
Here you are looking at the old code. After that there are several of 
changes related to suspend/resume functions.

This is the link to the code with changes. Latest version of those 
functions.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc2/hcd.c#n4489

Your changes are sitting on that latest version of code. Not the old 
version of it.

> 
> I took a mainline kernel ("v5.1-rc7-5-g83a50840e72a") and added
> printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP and
> PCGCTL_STOPPCLK in dwc2_port_suspend().
I think you did this tests on the old version of the code

I have tested the flow myself with the mainline Kernel on 
"torvalds/master" and not HPRT0_SUSP nor PCGCTL_STOPPCLK are not being set.

So here you need to review those things again.

> 
> [  454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP
> [  454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK
> 
> ...and just to confirm:
> 
> # grep '^power' /sys/kernel/debug/*.usb/params
> /sys/kernel/debug/ff540000.usb/params:power_down                    : 0
> /sys/kernel/debug/ff580000.usb/params:power_down                    : 0
> 
> So I'm really quite convinced that on mainline Linux with
> DWC2_POWER_DOWN_PARAM_NONE that dwc2_port_suspend() sets HPRT0_SUSP
> and PCGCTL_STOPPCLK.
> 
> 
>>> ...I believe that the net effect of my patch ends up doing both those
>>> same two things in hcd_suspend.  That is: when power_down is
>>> DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the
>>> same thing that dwc2_port_suspend() would do in the same case.  Is
>>> that not OK?
>> No if your patch is doing the same thing as it was doing before what is
>> the purpose of the patch ?
> 
> The purpose is to make _dwc2_hcd_suspend() work more correctly in the
> case where power_down is DWC2_POWER_DOWN_PARAM_NONE >
> 
>> My testes show that your patch doesn't fix the issue related partial
>> power down.
> 
> Right.  I have been trying to say that my patch doesn't do anything at
> all for partial power down.  I am simply trying to make
> DWC2_POWER_DOWN_PARAM_NONE work more correctly.
> 
> I haven't run all the power consumption tests in quite a long time and
> I'll try to get it hooked up tomorrow to confirm that my patch really
> truly is still needed to help with power consumption.  I did confirm
> that at least there are cases where _dwc2_hcd_suspend() is called and
> my patch is what sets the important bits.
> 
> -Doug
>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
  2019-05-03  8:20           ` [v2,1/5] " Artur Petrosyan
@ 2019-05-03  8:20             ` " Artur Petrosyan
  2019-05-03 15:03             ` [v2,1/5] " Doug Anderson
  1 sibling, 0 replies; 50+ messages in thread
From: Artur Petrosyan @ 2019-05-03  8:20 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern, amstan,
	linux-rockchip, William Wu, linux-usb, Stefan Wahren, Randy Li,
	zyw, mka, ryandcase, Amelie Delaunay, jwerner, dinguyen,
	Elaine Zhang, Greg Kroah-Hartman, linux-kernel

On 5/1/2019 05:57, Doug Anderson wrote:
> Hi,
> 
> On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.com> wrote:
>>
>> Hi,
>>
>> On 4/29/2019 21:34, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan
>>> <Arthur.Petrosyan@synopsys.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 4/18/2019 04:15, Douglas Anderson wrote:
>>>>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
>>>>> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
>>>>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
>>>>> because apparently it broke the Altera SOCFPGA.
>>>>>
>>>>> With all the changes that have happened to dwc2 in the meantime, it's
>>>>> possible that the Altera SOCFPGA will just magically work with this
>>>>> change now.  ...and it would be good to get bus suspend/resume
>>>>> implemented.
>>>>>
>>>>> This change is a forward port of one that's been living in the Chrome
>>>>> OS 3.14 kernel tree.
>>>>>
>>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>>> ---
>>>>> This patch was last posted at:
>>>>>
>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
>>>>>
>>>>> ...and appears to have died the death of silence.  Maybe it could get
>>>>> some bake time in linuxnext if we can't find any proactive testing?
>>>>>
>>>>> I will also freely admit that I don't know tons about the theory
>>>>> behind this patch.  I'm mostly just re-hashing the original commit
>>>>> from Kever that was reverted since:
>>>>> * Turning on partial power down on rk3288 doesn't "just work".  I
>>>>>      don't get hotplug events.  This is despite dwc2 auto-detecting that
>>>>>      we are power optimized.
>>>> What do you mean by doesn't "just work" ? It seem to me that even after
>>>> adding this patch you don't get issues fixed.
>>>> You mention that you don't get the hotplug events. Please provide dwc2
>>>> debug logs and register dumps on this issue.
>>>
>>> I mean that partial power down in the currently upstream driver
>>> doesn't work.  AKA: if I turn on partial power down in the upstream
>>> driver then hotplug events break.  I can try to provide some logs.  On
>>> what exact version of the code do you want logs?  Just your series?
>>> Just my series?  Mainline?  Some attempt at combining both series?  As
>>> I said things seem to sorta work with the combined series.  I can try
>>> to clarify if that's the series you want me to test with.  ...or I can
>>> wait for your next version?
>> As I said this patch doesn't fix the issue with hotplug. With this patch
>> or without the hotplug behaves as it was. I have tested it on our setup.
>>
>> Have you debugged your patch? Does it make any difference on your setup
>> ? Does it fix the issue with hotplug?
> 
> I think we're still not taking on the same page.
> 
> My patch makes no attempt to make partial power down mode work.  My
> patch attempts to make things work a little better when using
> DWC2_POWER_DOWN_PARAM_NONE.  There is no use testing my patch with
> partial power down as it shouldn't have any impact there.
> 
> 
>>> I am by no means an expert on dwc2, but an assumption made in my patch
>>> is that even cores that can't support partial power down can still
>>> save some amount of power when hcd_suspend is called.
>> Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ?
>>>
>>> Some evidence that this should be possible: looking at mainline Linux
>>> and at dwc2_port_suspend(), I see:
>>>
>>> * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE
>> Currently (without your and my patches) (looking at mainline Linux) the
>> function dwc2_port_suspend() is called anyway because its call is issued
>> by the system. But it performs entering to suspend only in case of
>> DWC2_POWER_DOWN_PARAM_PARTIAL.
>>
>> This is not an assumption. What I am pointing out is based on debugging
>> and before making assumptions without debugging for me seems not ok.
>>
>> Currently without your patch and without my patches. In the
>> dwc2_port_suspend() it will enter to suspend only in case that
>> power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the
>> code more carefully you will see
>>
>>          if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
>>                  goto skip_power_saving;
>>
>> This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip
>> power saving.
>>
>> So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it
>> tries to suspend.
> 
> We must be looking at different code.  I'm looking at Linux's tree, AKA:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_dwc2_hcd.c-23n3488&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=IWkDOOGTr0q-H1piDv2KOZe_Hnrz18g6rXFx-DsTuv4&s=AHu2iOKkybliRGtIfN7cF5p070UdvUKTYJsyAKYojis&e=
Here you are looking at the old code. After that there are several of 
changes related to suspend/resume functions.

This is the link to the code with changes. Latest version of those 
functions.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc2/hcd.c#n4489

Your changes are sitting on that latest version of code. Not the old 
version of it.

> 
> I took a mainline kernel ("v5.1-rc7-5-g83a50840e72a") and added
> printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP and
> PCGCTL_STOPPCLK in dwc2_port_suspend().
I think you did this tests on the old version of the code

I have tested the flow myself with the mainline Kernel on 
"torvalds/master" and not HPRT0_SUSP nor PCGCTL_STOPPCLK are not being set.

So here you need to review those things again.

> 
> [  454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP
> [  454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK
> 
> ...and just to confirm:
> 
> # grep '^power' /sys/kernel/debug/*.usb/params
> /sys/kernel/debug/ff540000.usb/params:power_down                    : 0
> /sys/kernel/debug/ff580000.usb/params:power_down                    : 0
> 
> So I'm really quite convinced that on mainline Linux with
> DWC2_POWER_DOWN_PARAM_NONE that dwc2_port_suspend() sets HPRT0_SUSP
> and PCGCTL_STOPPCLK.
> 
> 
>>> ...I believe that the net effect of my patch ends up doing both those
>>> same two things in hcd_suspend.  That is: when power_down is
>>> DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the
>>> same thing that dwc2_port_suspend() would do in the same case.  Is
>>> that not OK?
>> No if your patch is doing the same thing as it was doing before what is
>> the purpose of the patch ?
> 
> The purpose is to make _dwc2_hcd_suspend() work more correctly in the
> case where power_down is DWC2_POWER_DOWN_PARAM_NONE >
> 
>> My testes show that your patch doesn't fix the issue related partial
>> power down.
> 
> Right.  I have been trying to say that my patch doesn't do anything at
> all for partial power down.  I am simply trying to make
> DWC2_POWER_DOWN_PARAM_NONE work more correctly.
> 
> I haven't run all the power consumption tests in quite a long time and
> I'll try to get it hooked up tomorrow to confirm that my patch really
> truly is still needed to help with power consumption.  I did confirm
> that at least there are cases where _dwc2_hcd_suspend() is called and
> my patch is what sets the important bits.
> 
> -Doug
> 


-- 
Regards,
Artur

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
@ 2019-05-03  8:25     ` " Artur Petrosyan
  2019-05-03  8:25       ` [PATCH v2 1/5] " Artur Petrosyan
  2019-05-03 15:07       ` [v2,1/5] " Doug Anderson
  0 siblings, 2 replies; 50+ messages in thread
From: Artur Petrosyan @ 2019-05-03  8:25 UTC (permalink / raw)
  To: Doug Anderson, Minas Harutyunyan, Felipe Balbi, Heiko Stübner
  Cc: Alan Stern, Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Dinh Nguyen, Elaine Zhang, Greg Kroah-Hartman, LKML

On 5/2/2019 03:58, Doug Anderson wrote:
> Hi,
> 
> 
> On Wed, Apr 17, 2019 at 5:15 PM Douglas Anderson <dianders@chromium.org> wrote:
>>
>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
>> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
>> because apparently it broke the Altera SOCFPGA.
>>
>> With all the changes that have happened to dwc2 in the meantime, it's
>> possible that the Altera SOCFPGA will just magically work with this
>> change now.  ...and it would be good to get bus suspend/resume
>> implemented.
>>
>> This change is a forward port of one that's been living in the Chrome
>> OS 3.14 kernel tree.
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>> This patch was last posted at:
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=lTaNUA2XIYPat417fkd1A4Zpvb5eyYtTc1H_NIfW8Vw&e=
>>
>> ...and appears to have died the death of silence.  Maybe it could get
>> some bake time in linuxnext if we can't find any proactive testing?
>>
>> I will also freely admit that I don't know tons about the theory
>> behind this patch.  I'm mostly just re-hashing the original commit
>> from Kever that was reverted since:
>> * Turning on partial power down on rk3288 doesn't "just work".  I
>>    don't get hotplug events.  This is despite dwc2 auto-detecting that
>>    we are power optimized.
>> * If we don't do something like this commit we don't get into as low
>>    of a power mode.
> 
> OK, I spent the day digging more into this patch to confirm that it's
> really the right thing to do.  ...and it still seems to be.
> 
> First off: I'm pretty sure the above sentence "If we don't do
> something like this commit we don't get into as low of a power mode."
> is totally wrong.  Luckily it's "after the cut" and not part of the
> commit message.  Specifically I did a bunch of power testing and I
> couldn't find any instance saving power after this patch.
> 
> ...but, then I looked more carefully at all the history of this
> commit.  I ended up at:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium-2Dreview.googlesource.com_c_chromiumos_third-5Fparty_kernel_-2B_306265_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=LiyyIyaCPmr88nJeI7TCGtoJBFLRWir_reikYtAHHDw&e=
Looking at this code review I see that this patch fixes whatever issues 
you have on Chrome OS 3.14. But your patch has landed on the top of 
latest Kernel version. With the latest version I think you would not 
have the regression issue.
So you are fixing Chrome OS 3.14.

> 
> ...where I said that this fixes a resume speed regression.  More
> details could be found at the linked bug, AKA:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.chromium.org_p_chromium_issues_detail-3Fid-3D548336&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=7gK8ZGX2zZPqC98CDMhqxEY3Acm_TbYa3fpQjWtvexM&e=
> 
> ...but, sadly, I wasn't as verbose as I usually am and didn't describe
> my exact testing setup.  So I tried to reproduce.  ...and I was able
> to.  I tested on an rk3288-veyron-jerry with an empty USB hub plugged
> into the left port (the host port) and my "servo 2" debug board hooked
> up to the right port.  The "power_Resume" test in Chrome OS certainly
> showed a regression in 3.14 when doing a suspend/resume cycle.
> 
> 
> Digging into the logs in 3.14, before this patch I saw this in the logs:
> 
> usb 3-1: reset high-speed USB device number 2 using dwc2
> usb 3-1.7: reset high-speed USB device number 3 using dwc2
> 
> ...after this patch:
> 
> usb 3-1: USB disconnect, device number 2
> usb 3-1.7: USB disconnect, device number 3
> usb 3-1: new high-speed USB device number 4 using dwc2
> usb 3-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00
> usb 3-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0
> usb 3-1: Product: USB 2.0 Hub [MTT]
> usb 3-1.7: new high-speed USB device number 5 using dwc2
> usb 3-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11
> usb 3-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0
> usb 3-1.7: Product: USB 2.0 Hub
> 
> ...so basically my belief is that without this patch we're just sorta
> leaving the device hanging and it get confused on resume.  After this
> patch we behave slightly better.
> 
> I tested on 4.19 and found much the same.  There:
> 
> usb 2-1: reset high-speed USB device number 2 using dwc2
> usb 2-1.7: reset high-speed USB device number 3 using dwc2
> 
> vs.
> 
> usb 2-1.7: USB disconnect, device number 3
> usb 2-1: USB disconnect, device number 2
> usb 2-1: new high-speed USB device number 4 using dwc2
> usb 2-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00
> usb 2-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0
> usb 2-1: Product: USB 2.0 Hub [MTT]
> usb 2-1.7: new high-speed USB device number 5 using dwc2
> usb 2-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11
> usb 2-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0
> usb 2-1.7: Product: USB 2.0 Hub
> 
> 
> On 4.19 I didn't actually notice a the same resume time regression,
> presumably because things are happening more asynchronously there (I
> didn't confirm this).  ...but in any case it seems like the right
> thing to do to actually do the suspend.
> 
> 
> I'll also re-iterate once more that I'm not claiming that my patch
> helps with "partial power down".  It merely makes the "power savings
> disabled" case work more properly.
> 
> 
> I'll also note that my patch is already in Felipe's "testing/next"
> branch which I continue to believe is correct and good.
> 
> -Doug
>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
  2019-05-03  8:25     ` [v2,1/5] " Artur Petrosyan
@ 2019-05-03  8:25       ` " Artur Petrosyan
  2019-05-03 15:07       ` [v2,1/5] " Doug Anderson
  1 sibling, 0 replies; 50+ messages in thread
From: Artur Petrosyan @ 2019-05-03  8:25 UTC (permalink / raw)
  To: Doug Anderson, Minas Harutyunyan, Felipe Balbi, Heiko Stübner
  Cc: Alan Stern, Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Dinh Nguyen, Elaine Zhang, Greg Kroah-Hartman, LKML

On 5/2/2019 03:58, Doug Anderson wrote:
> Hi,
> 
> 
> On Wed, Apr 17, 2019 at 5:15 PM Douglas Anderson <dianders@chromium.org> wrote:
>>
>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
>> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
>> because apparently it broke the Altera SOCFPGA.
>>
>> With all the changes that have happened to dwc2 in the meantime, it's
>> possible that the Altera SOCFPGA will just magically work with this
>> change now.  ...and it would be good to get bus suspend/resume
>> implemented.
>>
>> This change is a forward port of one that's been living in the Chrome
>> OS 3.14 kernel tree.
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>> This patch was last posted at:
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=lTaNUA2XIYPat417fkd1A4Zpvb5eyYtTc1H_NIfW8Vw&e=
>>
>> ...and appears to have died the death of silence.  Maybe it could get
>> some bake time in linuxnext if we can't find any proactive testing?
>>
>> I will also freely admit that I don't know tons about the theory
>> behind this patch.  I'm mostly just re-hashing the original commit
>> from Kever that was reverted since:
>> * Turning on partial power down on rk3288 doesn't "just work".  I
>>    don't get hotplug events.  This is despite dwc2 auto-detecting that
>>    we are power optimized.
>> * If we don't do something like this commit we don't get into as low
>>    of a power mode.
> 
> OK, I spent the day digging more into this patch to confirm that it's
> really the right thing to do.  ...and it still seems to be.
> 
> First off: I'm pretty sure the above sentence "If we don't do
> something like this commit we don't get into as low of a power mode."
> is totally wrong.  Luckily it's "after the cut" and not part of the
> commit message.  Specifically I did a bunch of power testing and I
> couldn't find any instance saving power after this patch.
> 
> ...but, then I looked more carefully at all the history of this
> commit.  I ended up at:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium-2Dreview.googlesource.com_c_chromiumos_third-5Fparty_kernel_-2B_306265_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=LiyyIyaCPmr88nJeI7TCGtoJBFLRWir_reikYtAHHDw&e=
Looking at this code review I see that this patch fixes whatever issues 
you have on Chrome OS 3.14. But your patch has landed on the top of 
latest Kernel version. With the latest version I think you would not 
have the regression issue.
So you are fixing Chrome OS 3.14.

> 
> ...where I said that this fixes a resume speed regression.  More
> details could be found at the linked bug, AKA:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.chromium.org_p_chromium_issues_detail-3Fid-3D548336&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=7gK8ZGX2zZPqC98CDMhqxEY3Acm_TbYa3fpQjWtvexM&e=
> 
> ...but, sadly, I wasn't as verbose as I usually am and didn't describe
> my exact testing setup.  So I tried to reproduce.  ...and I was able
> to.  I tested on an rk3288-veyron-jerry with an empty USB hub plugged
> into the left port (the host port) and my "servo 2" debug board hooked
> up to the right port.  The "power_Resume" test in Chrome OS certainly
> showed a regression in 3.14 when doing a suspend/resume cycle.
> 
> 
> Digging into the logs in 3.14, before this patch I saw this in the logs:
> 
> usb 3-1: reset high-speed USB device number 2 using dwc2
> usb 3-1.7: reset high-speed USB device number 3 using dwc2
> 
> ...after this patch:
> 
> usb 3-1: USB disconnect, device number 2
> usb 3-1.7: USB disconnect, device number 3
> usb 3-1: new high-speed USB device number 4 using dwc2
> usb 3-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00
> usb 3-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0
> usb 3-1: Product: USB 2.0 Hub [MTT]
> usb 3-1.7: new high-speed USB device number 5 using dwc2
> usb 3-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11
> usb 3-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0
> usb 3-1.7: Product: USB 2.0 Hub
> 
> ...so basically my belief is that without this patch we're just sorta
> leaving the device hanging and it get confused on resume.  After this
> patch we behave slightly better.
> 
> I tested on 4.19 and found much the same.  There:
> 
> usb 2-1: reset high-speed USB device number 2 using dwc2
> usb 2-1.7: reset high-speed USB device number 3 using dwc2
> 
> vs.
> 
> usb 2-1.7: USB disconnect, device number 3
> usb 2-1: USB disconnect, device number 2
> usb 2-1: new high-speed USB device number 4 using dwc2
> usb 2-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00
> usb 2-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0
> usb 2-1: Product: USB 2.0 Hub [MTT]
> usb 2-1.7: new high-speed USB device number 5 using dwc2
> usb 2-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11
> usb 2-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0
> usb 2-1.7: Product: USB 2.0 Hub
> 
> 
> On 4.19 I didn't actually notice a the same resume time regression,
> presumably because things are happening more asynchronously there (I
> didn't confirm this).  ...but in any case it seems like the right
> thing to do to actually do the suspend.
> 
> 
> I'll also re-iterate once more that I'm not claiming that my patch
> helps with "partial power down".  It merely makes the "power savings
> disabled" case work more properly.
> 
> 
> I'll also note that my patch is already in Felipe's "testing/next"
> branch which I continue to believe is correct and good.
> 
> -Doug
> 


-- 
Regards,
Artur

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
@ 2019-05-03 15:03             ` " Doug Anderson
  2019-05-03 15:03               ` [PATCH v2 1/5] " Doug Anderson
  2019-05-07  7:26               ` Artur Petrosyan
  0 siblings, 2 replies; 50+ messages in thread
From: Doug Anderson @ 2019-05-03 15:03 UTC (permalink / raw)
  To: Artur Petrosyan
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern, amstan,
	linux-rockchip, William Wu, linux-usb, Stefan Wahren, Randy Li,
	zyw, mka, ryandcase, Amelie Delaunay, jwerner, dinguyen,
	Elaine Zhang, Greg Kroah-Hartman, linux-kernel

Hi,

On Fri, May 3, 2019 at 1:20 AM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> On 5/1/2019 05:57, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan
> > <Arthur.Petrosyan@synopsys.com> wrote:
> >>
> >> Hi,
> >>
> >> On 4/29/2019 21:34, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan
> >>> <Arthur.Petrosyan@synopsys.com> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On 4/18/2019 04:15, Douglas Anderson wrote:
> >>>>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> >>>>> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> >>>>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> >>>>> because apparently it broke the Altera SOCFPGA.
> >>>>>
> >>>>> With all the changes that have happened to dwc2 in the meantime, it's
> >>>>> possible that the Altera SOCFPGA will just magically work with this
> >>>>> change now.  ...and it would be good to get bus suspend/resume
> >>>>> implemented.
> >>>>>
> >>>>> This change is a forward port of one that's been living in the Chrome
> >>>>> OS 3.14 kernel tree.
> >>>>>
> >>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>>>> ---
> >>>>> This patch was last posted at:
> >>>>>
> >>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
> >>>>>
> >>>>> ...and appears to have died the death of silence.  Maybe it could get
> >>>>> some bake time in linuxnext if we can't find any proactive testing?
> >>>>>
> >>>>> I will also freely admit that I don't know tons about the theory
> >>>>> behind this patch.  I'm mostly just re-hashing the original commit
> >>>>> from Kever that was reverted since:
> >>>>> * Turning on partial power down on rk3288 doesn't "just work".  I
> >>>>>      don't get hotplug events.  This is despite dwc2 auto-detecting that
> >>>>>      we are power optimized.
> >>>> What do you mean by doesn't "just work" ? It seem to me that even after
> >>>> adding this patch you don't get issues fixed.
> >>>> You mention that you don't get the hotplug events. Please provide dwc2
> >>>> debug logs and register dumps on this issue.
> >>>
> >>> I mean that partial power down in the currently upstream driver
> >>> doesn't work.  AKA: if I turn on partial power down in the upstream
> >>> driver then hotplug events break.  I can try to provide some logs.  On
> >>> what exact version of the code do you want logs?  Just your series?
> >>> Just my series?  Mainline?  Some attempt at combining both series?  As
> >>> I said things seem to sorta work with the combined series.  I can try
> >>> to clarify if that's the series you want me to test with.  ...or I can
> >>> wait for your next version?
> >> As I said this patch doesn't fix the issue with hotplug. With this patch
> >> or without the hotplug behaves as it was. I have tested it on our setup.
> >>
> >> Have you debugged your patch? Does it make any difference on your setup
> >> ? Does it fix the issue with hotplug?
> >
> > I think we're still not taking on the same page.
> >
> > My patch makes no attempt to make partial power down mode work.  My
> > patch attempts to make things work a little better when using
> > DWC2_POWER_DOWN_PARAM_NONE.  There is no use testing my patch with
> > partial power down as it shouldn't have any impact there.
> >
> >
> >>> I am by no means an expert on dwc2, but an assumption made in my patch
> >>> is that even cores that can't support partial power down can still
> >>> save some amount of power when hcd_suspend is called.
> >> Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ?
> >>>
> >>> Some evidence that this should be possible: looking at mainline Linux
> >>> and at dwc2_port_suspend(), I see:
> >>>
> >>> * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE
> >> Currently (without your and my patches) (looking at mainline Linux) the
> >> function dwc2_port_suspend() is called anyway because its call is issued
> >> by the system. But it performs entering to suspend only in case of
> >> DWC2_POWER_DOWN_PARAM_PARTIAL.
> >>
> >> This is not an assumption. What I am pointing out is based on debugging
> >> and before making assumptions without debugging for me seems not ok.
> >>
> >> Currently without your patch and without my patches. In the
> >> dwc2_port_suspend() it will enter to suspend only in case that
> >> power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the
> >> code more carefully you will see
> >>
> >>          if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
> >>                  goto skip_power_saving;
> >>
> >> This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip
> >> power saving.
> >>
> >> So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it
> >> tries to suspend.
> >
> > We must be looking at different code.  I'm looking at Linux's tree, AKA:
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_dwc2_hcd.c-23n3488&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=IWkDOOGTr0q-H1piDv2KOZe_Hnrz18g6rXFx-DsTuv4&s=AHu2iOKkybliRGtIfN7cF5p070UdvUKTYJsyAKYojis&e=
> Here you are looking at the old code. After that there are several of
> changes related to suspend/resume functions.

In my email, see that I said I actually checked out mainline kernel
(and I gave you the exact version: "v5.1-rc7-5-g83a50840e72a") and
added printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP
and PCGCTL_STOPPCLK in dwc2_port_suspend().

[  454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP
[  454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK

The version "v5.1-rc7-5-g83a50840e72a" is not old code.


> This is the link to the code with changes. Latest version of those
> functions.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc2/hcd.c#n4489
>
> Your changes are sitting on that latest version of code. Not the old
> version of it.

You are pointing me at _dwc2_hcd_suspend() whereas I pointed at
dwc2_port_suspend().  Why?

I am saying that dwc2_port_suspend() _does_ set "HPRT0_SUSP" and
"PCGCTL_STOPPCLK" even with DWC2_POWER_DOWN_PARAM_NONE.  Do you
disagree?

I completely agree that on mainline _dwc2_hcd_suspend() _does not_ set
these bits with DWC2_POWER_DOWN_PARAM_NONE.  That is what my patch
fixes.


-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
  2019-05-03 15:03             ` [v2,1/5] " Doug Anderson
@ 2019-05-03 15:03               ` " Doug Anderson
  2019-05-07  7:26               ` Artur Petrosyan
  1 sibling, 0 replies; 50+ messages in thread
From: Doug Anderson @ 2019-05-03 15:03 UTC (permalink / raw)
  To: Artur Petrosyan
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern, amstan,
	linux-rockchip, William Wu, linux-usb, Stefan Wahren, Randy Li,
	zyw, mka, ryandcase, Amelie Delaunay, jwerner, dinguyen,
	Elaine Zhang, Greg Kroah-Hartman, linux-kernel

Hi,

On Fri, May 3, 2019 at 1:20 AM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> On 5/1/2019 05:57, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan
> > <Arthur.Petrosyan@synopsys.com> wrote:
> >>
> >> Hi,
> >>
> >> On 4/29/2019 21:34, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan
> >>> <Arthur.Petrosyan@synopsys.com> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On 4/18/2019 04:15, Douglas Anderson wrote:
> >>>>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> >>>>> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> >>>>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> >>>>> because apparently it broke the Altera SOCFPGA.
> >>>>>
> >>>>> With all the changes that have happened to dwc2 in the meantime, it's
> >>>>> possible that the Altera SOCFPGA will just magically work with this
> >>>>> change now.  ...and it would be good to get bus suspend/resume
> >>>>> implemented.
> >>>>>
> >>>>> This change is a forward port of one that's been living in the Chrome
> >>>>> OS 3.14 kernel tree.
> >>>>>
> >>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>>>> ---
> >>>>> This patch was last posted at:
> >>>>>
> >>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
> >>>>>
> >>>>> ...and appears to have died the death of silence.  Maybe it could get
> >>>>> some bake time in linuxnext if we can't find any proactive testing?
> >>>>>
> >>>>> I will also freely admit that I don't know tons about the theory
> >>>>> behind this patch.  I'm mostly just re-hashing the original commit
> >>>>> from Kever that was reverted since:
> >>>>> * Turning on partial power down on rk3288 doesn't "just work".  I
> >>>>>      don't get hotplug events.  This is despite dwc2 auto-detecting that
> >>>>>      we are power optimized.
> >>>> What do you mean by doesn't "just work" ? It seem to me that even after
> >>>> adding this patch you don't get issues fixed.
> >>>> You mention that you don't get the hotplug events. Please provide dwc2
> >>>> debug logs and register dumps on this issue.
> >>>
> >>> I mean that partial power down in the currently upstream driver
> >>> doesn't work.  AKA: if I turn on partial power down in the upstream
> >>> driver then hotplug events break.  I can try to provide some logs.  On
> >>> what exact version of the code do you want logs?  Just your series?
> >>> Just my series?  Mainline?  Some attempt at combining both series?  As
> >>> I said things seem to sorta work with the combined series.  I can try
> >>> to clarify if that's the series you want me to test with.  ...or I can
> >>> wait for your next version?
> >> As I said this patch doesn't fix the issue with hotplug. With this patch
> >> or without the hotplug behaves as it was. I have tested it on our setup.
> >>
> >> Have you debugged your patch? Does it make any difference on your setup
> >> ? Does it fix the issue with hotplug?
> >
> > I think we're still not taking on the same page.
> >
> > My patch makes no attempt to make partial power down mode work.  My
> > patch attempts to make things work a little better when using
> > DWC2_POWER_DOWN_PARAM_NONE.  There is no use testing my patch with
> > partial power down as it shouldn't have any impact there.
> >
> >
> >>> I am by no means an expert on dwc2, but an assumption made in my patch
> >>> is that even cores that can't support partial power down can still
> >>> save some amount of power when hcd_suspend is called.
> >> Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ?
> >>>
> >>> Some evidence that this should be possible: looking at mainline Linux
> >>> and at dwc2_port_suspend(), I see:
> >>>
> >>> * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE
> >> Currently (without your and my patches) (looking at mainline Linux) the
> >> function dwc2_port_suspend() is called anyway because its call is issued
> >> by the system. But it performs entering to suspend only in case of
> >> DWC2_POWER_DOWN_PARAM_PARTIAL.
> >>
> >> This is not an assumption. What I am pointing out is based on debugging
> >> and before making assumptions without debugging for me seems not ok.
> >>
> >> Currently without your patch and without my patches. In the
> >> dwc2_port_suspend() it will enter to suspend only in case that
> >> power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the
> >> code more carefully you will see
> >>
> >>          if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
> >>                  goto skip_power_saving;
> >>
> >> This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip
> >> power saving.
> >>
> >> So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it
> >> tries to suspend.
> >
> > We must be looking at different code.  I'm looking at Linux's tree, AKA:
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_dwc2_hcd.c-23n3488&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=IWkDOOGTr0q-H1piDv2KOZe_Hnrz18g6rXFx-DsTuv4&s=AHu2iOKkybliRGtIfN7cF5p070UdvUKTYJsyAKYojis&e=
> Here you are looking at the old code. After that there are several of
> changes related to suspend/resume functions.

In my email, see that I said I actually checked out mainline kernel
(and I gave you the exact version: "v5.1-rc7-5-g83a50840e72a") and
added printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP
and PCGCTL_STOPPCLK in dwc2_port_suspend().

[  454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP
[  454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK

The version "v5.1-rc7-5-g83a50840e72a" is not old code.


> This is the link to the code with changes. Latest version of those
> functions.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc2/hcd.c#n4489
>
> Your changes are sitting on that latest version of code. Not the old
> version of it.

You are pointing me at _dwc2_hcd_suspend() whereas I pointed at
dwc2_port_suspend().  Why?

I am saying that dwc2_port_suspend() _does_ set "HPRT0_SUSP" and
"PCGCTL_STOPPCLK" even with DWC2_POWER_DOWN_PARAM_NONE.  Do you
disagree?

I completely agree that on mainline _dwc2_hcd_suspend() _does not_ set
these bits with DWC2_POWER_DOWN_PARAM_NONE.  That is what my patch
fixes.


-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
@ 2019-05-03 15:07       ` " Doug Anderson
  2019-05-03 15:07         ` [PATCH v2 1/5] " Doug Anderson
  2019-05-07  7:05         ` Artur Petrosyan
  0 siblings, 2 replies; 50+ messages in thread
From: Doug Anderson @ 2019-05-03 15:07 UTC (permalink / raw)
  To: Artur Petrosyan
  Cc: Minas Harutyunyan, Felipe Balbi, Heiko Stübner, Alan Stern,
	Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Elaine Zhang, Greg Kroah-Hartman, LKML

Hi,

On Fri, May 3, 2019 at 1:25 AM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> On 5/2/2019 03:58, Doug Anderson wrote:
> > Hi,
> >
> >
> > On Wed, Apr 17, 2019 at 5:15 PM Douglas Anderson <dianders@chromium.org> wrote:
> >>
> >> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> >> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> >> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> >> because apparently it broke the Altera SOCFPGA.
> >>
> >> With all the changes that have happened to dwc2 in the meantime, it's
> >> possible that the Altera SOCFPGA will just magically work with this
> >> change now.  ...and it would be good to get bus suspend/resume
> >> implemented.
> >>
> >> This change is a forward port of one that's been living in the Chrome
> >> OS 3.14 kernel tree.
> >>
> >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >> ---
> >> This patch was last posted at:
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=lTaNUA2XIYPat417fkd1A4Zpvb5eyYtTc1H_NIfW8Vw&e=
> >>
> >> ...and appears to have died the death of silence.  Maybe it could get
> >> some bake time in linuxnext if we can't find any proactive testing?
> >>
> >> I will also freely admit that I don't know tons about the theory
> >> behind this patch.  I'm mostly just re-hashing the original commit
> >> from Kever that was reverted since:
> >> * Turning on partial power down on rk3288 doesn't "just work".  I
> >>    don't get hotplug events.  This is despite dwc2 auto-detecting that
> >>    we are power optimized.
> >> * If we don't do something like this commit we don't get into as low
> >>    of a power mode.
> >
> > OK, I spent the day digging more into this patch to confirm that it's
> > really the right thing to do.  ...and it still seems to be.
> >
> > First off: I'm pretty sure the above sentence "If we don't do
> > something like this commit we don't get into as low of a power mode."
> > is totally wrong.  Luckily it's "after the cut" and not part of the
> > commit message.  Specifically I did a bunch of power testing and I
> > couldn't find any instance saving power after this patch.
> >
> > ...but, then I looked more carefully at all the history of this
> > commit.  I ended up at:
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium-2Dreview.googlesource.com_c_chromiumos_third-5Fparty_kernel_-2B_306265_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=LiyyIyaCPmr88nJeI7TCGtoJBFLRWir_reikYtAHHDw&e=
> Looking at this code review I see that this patch fixes whatever issues
> you have on Chrome OS 3.14. But your patch has landed on the top of
> latest Kernel version. With the latest version I think you would not
> have the regression issue.
> So you are fixing Chrome OS 3.14.

I'm confused why you ignored the rest of my email where I said I also
ported it to 4.19 (which, from a dwc2 host point of view, is pretty
much mainline) and saw that the patch was still needed.

-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
  2019-05-03 15:07       ` [v2,1/5] " Doug Anderson
@ 2019-05-03 15:07         ` " Doug Anderson
  2019-05-07  7:05         ` Artur Petrosyan
  1 sibling, 0 replies; 50+ messages in thread
From: Doug Anderson @ 2019-05-03 15:07 UTC (permalink / raw)
  To: Artur Petrosyan
  Cc: Minas Harutyunyan, Felipe Balbi, Heiko Stübner, Alan Stern,
	Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Elaine Zhang, Greg Kroah-Hartman, LKML

Hi,

On Fri, May 3, 2019 at 1:25 AM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> On 5/2/2019 03:58, Doug Anderson wrote:
> > Hi,
> >
> >
> > On Wed, Apr 17, 2019 at 5:15 PM Douglas Anderson <dianders@chromium.org> wrote:
> >>
> >> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> >> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> >> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> >> because apparently it broke the Altera SOCFPGA.
> >>
> >> With all the changes that have happened to dwc2 in the meantime, it's
> >> possible that the Altera SOCFPGA will just magically work with this
> >> change now.  ...and it would be good to get bus suspend/resume
> >> implemented.
> >>
> >> This change is a forward port of one that's been living in the Chrome
> >> OS 3.14 kernel tree.
> >>
> >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >> ---
> >> This patch was last posted at:
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=lTaNUA2XIYPat417fkd1A4Zpvb5eyYtTc1H_NIfW8Vw&e=
> >>
> >> ...and appears to have died the death of silence.  Maybe it could get
> >> some bake time in linuxnext if we can't find any proactive testing?
> >>
> >> I will also freely admit that I don't know tons about the theory
> >> behind this patch.  I'm mostly just re-hashing the original commit
> >> from Kever that was reverted since:
> >> * Turning on partial power down on rk3288 doesn't "just work".  I
> >>    don't get hotplug events.  This is despite dwc2 auto-detecting that
> >>    we are power optimized.
> >> * If we don't do something like this commit we don't get into as low
> >>    of a power mode.
> >
> > OK, I spent the day digging more into this patch to confirm that it's
> > really the right thing to do.  ...and it still seems to be.
> >
> > First off: I'm pretty sure the above sentence "If we don't do
> > something like this commit we don't get into as low of a power mode."
> > is totally wrong.  Luckily it's "after the cut" and not part of the
> > commit message.  Specifically I did a bunch of power testing and I
> > couldn't find any instance saving power after this patch.
> >
> > ...but, then I looked more carefully at all the history of this
> > commit.  I ended up at:
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium-2Dreview.googlesource.com_c_chromiumos_third-5Fparty_kernel_-2B_306265_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=LiyyIyaCPmr88nJeI7TCGtoJBFLRWir_reikYtAHHDw&e=
> Looking at this code review I see that this patch fixes whatever issues
> you have on Chrome OS 3.14. But your patch has landed on the top of
> latest Kernel version. With the latest version I think you would not
> have the regression issue.
> So you are fixing Chrome OS 3.14.

I'm confused why you ignored the rest of my email where I said I also
ported it to 4.19 (which, from a dwc2 host point of view, is pretty
much mainline) and saw that the patch was still needed.

-Doug

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
  2019-05-03 15:07       ` [v2,1/5] " Doug Anderson
  2019-05-03 15:07         ` [PATCH v2 1/5] " Doug Anderson
@ 2019-05-07  7:05         ` Artur Petrosyan
  1 sibling, 0 replies; 50+ messages in thread
From: Artur Petrosyan @ 2019-05-07  7:05 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Minas Harutyunyan, Felipe Balbi, Heiko Stübner, Alan Stern,
	Alexandru M Stan, open list:ARM/Rockchip SoC...,
	William Wu, linux-usb, Stefan Wahren, Randy Li, Chris,
	Matthias Kaehlcke, Ryan Case, Amelie Delaunay, Julius Werner,
	Elaine Zhang, Greg Kroah-Hartman, LKML

Hi,

On 5/3/2019 19:08, Doug Anderson wrote:
> Hi,
> 
> On Fri, May 3, 2019 at 1:25 AM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.com> wrote:
>>
>> On 5/2/2019 03:58, Doug Anderson wrote:
>>> Hi,
>>>
>>>
>>> On Wed, Apr 17, 2019 at 5:15 PM Douglas Anderson <dianders@chromium.org> wrote:
>>>>
>>>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
>>>> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
>>>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
>>>> because apparently it broke the Altera SOCFPGA.
>>>>
>>>> With all the changes that have happened to dwc2 in the meantime, it's
>>>> possible that the Altera SOCFPGA will just magically work with this
>>>> change now.  ...and it would be good to get bus suspend/resume
>>>> implemented.
>>>>
>>>> This change is a forward port of one that's been living in the Chrome
>>>> OS 3.14 kernel tree.
>>>>
>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>> ---
>>>> This patch was last posted at:
>>>>
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=lTaNUA2XIYPat417fkd1A4Zpvb5eyYtTc1H_NIfW8Vw&e=
>>>>
>>>> ...and appears to have died the death of silence.  Maybe it could get
>>>> some bake time in linuxnext if we can't find any proactive testing?
>>>>
>>>> I will also freely admit that I don't know tons about the theory
>>>> behind this patch.  I'm mostly just re-hashing the original commit
>>>> from Kever that was reverted since:
>>>> * Turning on partial power down on rk3288 doesn't "just work".  I
>>>>     don't get hotplug events.  This is despite dwc2 auto-detecting that
>>>>     we are power optimized.
>>>> * If we don't do something like this commit we don't get into as low
>>>>     of a power mode.
>>>
>>> OK, I spent the day digging more into this patch to confirm that it's
>>> really the right thing to do.  ...and it still seems to be.
>>>
>>> First off: I'm pretty sure the above sentence "If we don't do
>>> something like this commit we don't get into as low of a power mode."
>>> is totally wrong.  Luckily it's "after the cut" and not part of the
>>> commit message.  Specifically I did a bunch of power testing and I
>>> couldn't find any instance saving power after this patch.
>>>
>>> ...but, then I looked more carefully at all the history of this
>>> commit.  I ended up at:
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium-2Dreview.googlesource.com_c_chromiumos_third-5Fparty_kernel_-2B_306265_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=LiyyIyaCPmr88nJeI7TCGtoJBFLRWir_reikYtAHHDw&e=
>> Looking at this code review I see that this patch fixes whatever issues
>> you have on Chrome OS 3.14. But your patch has landed on the top of
>> latest Kernel version. With the latest version I think you would not
>> have the regression issue.
>> So you are fixing Chrome OS 3.14.
> 
> I'm confused why you ignored the rest of my email where I said I also
> ported it to 4.19 (which, from a dwc2 host point of view, is pretty
> much mainline) and saw that the patch was still needed.
I didn't ignore it just I had to perform testes and reply to it with 
another email.
> 
> -Doug
> 
I spent yesterday debugging and performing testes with Linux Mainline. 
So when we don't have any of power saving modes supported and the 
power_down is DWC2_POWER_DOWN_PARAM_NONE. We can set "PCGCTL_STOPPCLK" 
bit whenever there is suspend ( Checked the programming guide and data 
book). I have not seen any case that this affects the flow. I have not 
been able to see if after setting "PCGCTL_STOPPCLK" bit there is any 
power saved or driver behaved differently. Maybe it is platform depended 
. However, there is a possibility that this might save power.

So as this is not breaking anything. I am ok with this patch.

-- 
Regards,
Artur

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
  2019-05-03 15:03             ` [v2,1/5] " Doug Anderson
  2019-05-03 15:03               ` [PATCH v2 1/5] " Doug Anderson
@ 2019-05-07  7:26               ` Artur Petrosyan
  1 sibling, 0 replies; 50+ messages in thread
From: Artur Petrosyan @ 2019-05-07  7:26 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Minas Harutyunyan, Felipe Balbi, heiko, Alan Stern, amstan,
	linux-rockchip, William Wu, linux-usb, Stefan Wahren, Randy Li,
	zyw, mka, ryandcase, Amelie Delaunay, jwerner, dinguyen,
	Elaine Zhang, Greg Kroah-Hartman, linux-kernel

On 5/3/2019 19:04, Doug Anderson wrote:
> Hi,
> 
> On Fri, May 3, 2019 at 1:20 AM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.com> wrote:
>>
>> On 5/1/2019 05:57, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan
>>> <Arthur.Petrosyan@synopsys.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 4/29/2019 21:34, Doug Anderson wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan
>>>>> <Arthur.Petrosyan@synopsys.com> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 4/18/2019 04:15, Douglas Anderson wrote:
>>>>>>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
>>>>>>> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
>>>>>>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
>>>>>>> because apparently it broke the Altera SOCFPGA.
>>>>>>>
>>>>>>> With all the changes that have happened to dwc2 in the meantime, it's
>>>>>>> possible that the Altera SOCFPGA will just magically work with this
>>>>>>> change now.  ...and it would be good to get bus suspend/resume
>>>>>>> implemented.
>>>>>>>
>>>>>>> This change is a forward port of one that's been living in the Chrome
>>>>>>> OS 3.14 kernel tree.
>>>>>>>
>>>>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>>>>> ---
>>>>>>> This patch was last posted at:
>>>>>>>
>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
>>>>>>>
>>>>>>> ...and appears to have died the death of silence.  Maybe it could get
>>>>>>> some bake time in linuxnext if we can't find any proactive testing?
>>>>>>>
>>>>>>> I will also freely admit that I don't know tons about the theory
>>>>>>> behind this patch.  I'm mostly just re-hashing the original commit
>>>>>>> from Kever that was reverted since:
>>>>>>> * Turning on partial power down on rk3288 doesn't "just work".  I
>>>>>>>       don't get hotplug events.  This is despite dwc2 auto-detecting that
>>>>>>>       we are power optimized.
>>>>>> What do you mean by doesn't "just work" ? It seem to me that even after
>>>>>> adding this patch you don't get issues fixed.
>>>>>> You mention that you don't get the hotplug events. Please provide dwc2
>>>>>> debug logs and register dumps on this issue.
>>>>>
>>>>> I mean that partial power down in the currently upstream driver
>>>>> doesn't work.  AKA: if I turn on partial power down in the upstream
>>>>> driver then hotplug events break.  I can try to provide some logs.  On
>>>>> what exact version of the code do you want logs?  Just your series?
>>>>> Just my series?  Mainline?  Some attempt at combining both series?  As
>>>>> I said things seem to sorta work with the combined series.  I can try
>>>>> to clarify if that's the series you want me to test with.  ...or I can
>>>>> wait for your next version?
>>>> As I said this patch doesn't fix the issue with hotplug. With this patch
>>>> or without the hotplug behaves as it was. I have tested it on our setup.
>>>>
>>>> Have you debugged your patch? Does it make any difference on your setup
>>>> ? Does it fix the issue with hotplug?
>>>
>>> I think we're still not taking on the same page.
>>>
>>> My patch makes no attempt to make partial power down mode work.  My
>>> patch attempts to make things work a little better when using
>>> DWC2_POWER_DOWN_PARAM_NONE.  There is no use testing my patch with
>>> partial power down as it shouldn't have any impact there.
>>>
>>>
>>>>> I am by no means an expert on dwc2, but an assumption made in my patch
>>>>> is that even cores that can't support partial power down can still
>>>>> save some amount of power when hcd_suspend is called.
>>>> Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ?
>>>>>
>>>>> Some evidence that this should be possible: looking at mainline Linux
>>>>> and at dwc2_port_suspend(), I see:
>>>>>
>>>>> * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE
>>>> Currently (without your and my patches) (looking at mainline Linux) the
>>>> function dwc2_port_suspend() is called anyway because its call is issued
>>>> by the system. But it performs entering to suspend only in case of
>>>> DWC2_POWER_DOWN_PARAM_PARTIAL.
>>>>
>>>> This is not an assumption. What I am pointing out is based on debugging
>>>> and before making assumptions without debugging for me seems not ok.
>>>>
>>>> Currently without your patch and without my patches. In the
>>>> dwc2_port_suspend() it will enter to suspend only in case that
>>>> power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the
>>>> code more carefully you will see
>>>>
>>>>           if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
>>>>                   goto skip_power_saving;
>>>>
>>>> This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip
>>>> power saving.
>>>>
>>>> So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it
>>>> tries to suspend.
>>>
>>> We must be looking at different code.  I'm looking at Linux's tree, AKA:
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_dwc2_hcd.c-23n3488&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=IWkDOOGTr0q-H1piDv2KOZe_Hnrz18g6rXFx-DsTuv4&s=AHu2iOKkybliRGtIfN7cF5p070UdvUKTYJsyAKYojis&e=
>> Here you are looking at the old code. After that there are several of
>> changes related to suspend/resume functions.
> 
> In my email, see that I said I actually checked out mainline kernel
> (and I gave you the exact version: "v5.1-rc7-5-g83a50840e72a") and
> added printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP
> and PCGCTL_STOPPCLK in dwc2_port_suspend().
I was talking about your patch which edits _dwc2_hcd_suspend() function.
What dwc2_port_suspend() implements for the hibernation or partial power 
down I have not tested. It is a different implementation.
> 
> [  454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP
> [  454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK
> 
> The version "v5.1-rc7-5-g83a50840e72a" is not old code.
> 
> 
>> This is the link to the code with changes. Latest version of those
>> functions.
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_dwc2_hcd.c-23n4489&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=4lskGlSdN5lp0MXVy1SP0zxzFtbPMSUoQKQzLLmfoOg&s=A-ikUNQklzouItTOQB078WrOiMcfCqgw8sBgLFwtyTU&e=
>>
>> Your changes are sitting on that latest version of code. Not the old
>> version of it.
> 
> You are pointing me at _dwc2_hcd_suspend() whereas I pointed at
> dwc2_port_suspend().  Why?
Because your patch is editing _dwc2_hcd_suspend().
> 
> I am saying that dwc2_port_suspend() _does_ set "HPRT0_SUSP" and
> "PCGCTL_STOPPCLK" even with DWC2_POWER_DOWN_PARAM_NONE.  Do you
> disagree?
Yes dwc2_port_suspend() does but that is a different implementation
> 
> I completely agree that on mainline _dwc2_hcd_suspend() _does not_ set
> these bits with DWC2_POWER_DOWN_PARAM_NONE.  That is what my patch
> fixes.
Yes so I was pointing to that.
> 
> 
> -Doug
> 

I have performed testes and made sure that the patch is ok. It is just 
that I am not sure if it does really save power. I have not been able to 
see if any power is saved.

So I am ok with this patch.

-- 
Regards,
Artur

^ permalink raw reply	[flat|nested] 50+ messages in thread

end of thread, back to index

Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-18  0:13 [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron Douglas Anderson
2019-04-18  0:13 ` [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE Doug Anderson
2019-04-18  0:13   ` [PATCH v2 1/5] " Douglas Anderson
2019-04-29  8:43   ` [v2,1/5] " Artur Petrosyan
2019-04-29  8:43     ` [PATCH v2 1/5] " Artur Petrosyan
2019-04-29 17:33     ` [v2,1/5] " Doug Anderson
2019-04-29 17:33       ` [PATCH v2 1/5] " Doug Anderson
2019-04-30  6:05       ` [v2,1/5] " Artur Petrosyan
2019-04-30  6:05         ` [PATCH v2 1/5] " Artur Petrosyan
2019-05-01  1:51         ` [v2,1/5] " Doug Anderson
2019-05-01  1:51           ` [PATCH v2 1/5] " Doug Anderson
2019-05-03  8:20           ` [v2,1/5] " Artur Petrosyan
2019-05-03  8:20             ` [PATCH v2 1/5] " Artur Petrosyan
2019-05-03 15:03             ` [v2,1/5] " Doug Anderson
2019-05-03 15:03               ` [PATCH v2 1/5] " Doug Anderson
2019-05-07  7:26               ` Artur Petrosyan
2019-05-01 23:58   ` [v2,1/5] " Doug Anderson
2019-05-01 23:58     ` [PATCH v2 1/5] " Doug Anderson
2019-05-03  8:25     ` [v2,1/5] " Artur Petrosyan
2019-05-03  8:25       ` [PATCH v2 1/5] " Artur Petrosyan
2019-05-03 15:07       ` [v2,1/5] " Doug Anderson
2019-05-03 15:07         ` [PATCH v2 1/5] " Doug Anderson
2019-05-07  7:05         ` Artur Petrosyan
2019-04-18  0:13 ` [v2,2/5] USB: Export usb_wakeup_enabled_descendants() Doug Anderson
2019-04-18  0:13   ` [PATCH v2 2/5] " Douglas Anderson
2019-04-18  0:13 ` [v2,3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB Doug Anderson
2019-04-18  0:13   ` [PATCH v2 3/5] " Douglas Anderson
2019-04-25 12:40   ` [v2,3/5] " Felipe Balbi
2019-04-25 12:40     ` [PATCH v2 3/5] " Felipe Balbi
2019-04-25 18:09     ` [v2,3/5] " Doug Anderson
2019-04-25 18:09       ` [PATCH v2 3/5] " Doug Anderson
2019-04-25 19:58       ` [v2,3/5] " Doug Anderson
2019-04-25 19:58         ` [PATCH v2 3/5] " Doug Anderson
2019-05-02 18:36     ` [v2,3/5] " Doug Anderson
2019-05-02 18:36       ` [PATCH v2 3/5] " Doug Anderson
2019-04-30  1:23   ` [v2,3/5] " Rob Herring
2019-04-30  1:23     ` [PATCH v2 3/5] " Rob Herring
2019-04-30  5:25     ` [v2,3/5] " Doug Anderson
2019-04-30  5:25       ` [PATCH v2 3/5] " Doug Anderson
2019-04-18  0:13 ` [v2,4/5] USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled Doug Anderson
2019-04-18  0:13   ` [PATCH v2 4/5] " Douglas Anderson
2019-04-25 12:41   ` [v2,4/5] " Felipe Balbi
2019-04-25 12:41     ` [PATCH v2 4/5] " Felipe Balbi
2019-04-18  0:13 ` [v2,5/5] ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports Doug Anderson
2019-04-18  0:13   ` [PATCH v2 5/5] " Douglas Anderson
2019-04-18 12:40 ` [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron Minas Harutyunyan
2019-04-18 15:54   ` Doug Anderson
2019-04-19 11:43     ` Artur Petrosyan
2019-04-19 16:44       ` Artur Petrosyan
2019-04-22 15:50         ` Artur Petrosyan

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git