[PATCH v2 0/2] Support EHCI reset after resume quirk

[PATCH v2 0/2] Support EHCI reset after resume quirk

[Wu Liang feng]

[PATCH v2 1/2]: Add an optional property "needs-reset-on-resume"
		in usb-ehci dt-bingdings.
[PATCH v2 2/2]: modify ehci-platform driver to support ehci reset
		after resume quirk if necessary.

Wu Liang feng (2):
  dt-bindings: usb-ehci: Add an optional property
	       "needs-reset-on-resume"
  USB: ehci-platform: Support ehci reset after resume quirk

tested on rk3288-pinky

 Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
 drivers/usb/host/ehci-hcd.c                        | 6 +++---
 drivers/usb/host/ehci-platform.c                   | 6 +++++-
 drivers/usb/host/ehci.h                            | 2 +-
 include/linux/usb/ehci_pdriver.h                   | 3 +++
 5 files changed, 13 insertions(+), 5 deletions(-)

-- 
1.8.3.2

[PATCH 1/2] dt-bindings: usb-ehci: Add an optional property "needs-reset-on-resume"

[Wu Liang feng]

Add device-tree bindings for EHCI so we can use "needs-reset-on-resume"
property to force EHCI reset after resume if necessary. This is necessary
on platforms like rk3288 that need a reset after resume to detect if a
device has been disconnected during suspend time.

Signed-off-by: Wu Liang feng <wulf@rock-chips.com>
Reviewed-by: Julius Werner <jwerner@google.com>
Reviewed-by: Doug Anderson <dianders@google.com>
Reviewed-by: Tomasz Figa <tfiga@google.com>
Reviewed-by: Pawel Osciak <posciak@google.com>
Reviewed-by: Sonny Rao <sonnyrao@google.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Tested-by: Doug Anderson <dianders@google.com>
---
 Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
index 43c1a4e..0b04fdf 100644
--- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
@@ -12,6 +12,7 @@ Optional properties:
  - big-endian-regs : boolean, set this for hcds with big-endian registers
  - big-endian-desc : boolean, set this for hcds with big-endian descriptors
  - big-endian : boolean, for hcds with big-endian-regs + big-endian-desc
+ - needs-reset-on-resume : boolean, set this to force EHCI reset after resume
  - clocks : a list of phandle + clock specifier pairs
  - phys : phandle + phy specifier pair
  - phy-names : "usb"
-- 
1.8.3.2

[PATCH 2/2] USB: ehci-platform: Support ehci reset after resume quirk

[Wu Liang feng]

The Rockchip rk3288 EHCI controller doesn't properly detect
the case when a device is removed during suspend. Specifically,
when usb resume from suspend, the EHCI controller maintaining
the USB state (FLAG_CF is 1, Current Connect Status is 1),
but a USB device (like a USB camera on rk3288) may have been
disconnected actually.

Let's add a quirk to force ehci to go into the
usb_root_hub_lost_power() path and reset after resume.
This should generally reset the whole controller and all
ports and initialize everything cleanly again, and bring
the devices back up.

As part of this, rename the "hibernation" paramter of
ehci_resume() to force_reset since hibernation is simply
another case where we can't trust the autodetected status
and need to force a reset of devices.

Signed-off-by: Wu Liang feng <wulf@rock-chips.com>
Reviewed-by: Julius Werner <jwerner@google.com>
Reviewed-by: Doug Anderson <dianders@google.com>
Reviewed-by: Tomasz Figa <tfiga@google.com>
Reviewed-by: Pawel Osciak <posciak@google.com>
Reviewed-by: Sonny Rao <sonnyrao@google.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Tested-by: Doug Anderson <dianders@google.com>
---
 drivers/usb/host/ehci-hcd.c      | 6 +++---
 drivers/usb/host/ehci-platform.c | 6 +++++-
 drivers/usb/host/ehci.h          | 2 +-
 include/linux/usb/ehci_pdriver.h | 3 +++
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 38bfeed..85e56d1 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1110,7 +1110,7 @@ int ehci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 EXPORT_SYMBOL_GPL(ehci_suspend);
 
 /* Returns 0 if power was preserved, 1 if power was lost */
-int ehci_resume(struct usb_hcd *hcd, bool hibernated)
+int ehci_resume(struct usb_hcd *hcd, bool force_reset)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci(hcd);
 
@@ -1124,12 +1124,12 @@ int ehci_resume(struct usb_hcd *hcd, bool hibernated)
 		return 0;		/* Controller is dead */
 
 	/*
-	 * If CF is still set and we aren't resuming from hibernation
+	 * If CF is still set and reset isn't forced
 	 * then we maintained suspend power.
 	 * Just undo the effect of ehci_suspend().
 	 */
 	if (ehci_readl(ehci, &ehci->regs->configured_flag) == FLAG_CF &&
-			!hibernated) {
+			!force_reset) {
 		int	mask = INTR_MASK;
 
 		ehci_prepare_ports_for_controller_resume(ehci);
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index 8557803..db5c29e 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -185,6 +185,10 @@ static int ehci_platform_probe(struct platform_device *dev)
 		if (of_property_read_bool(dev->dev.of_node, "big-endian"))
 			ehci->big_endian_mmio = ehci->big_endian_desc = 1;
 
+		if (of_property_read_bool(dev->dev.of_node,
+					  "needs-reset-on-resume"))
+			pdata->reset_on_resume = 1;
+
 		priv->phy = devm_phy_get(&dev->dev, "usb");
 		if (IS_ERR(priv->phy)) {
 			err = PTR_ERR(priv->phy);
@@ -340,7 +344,7 @@ static int ehci_platform_resume(struct device *dev)
 			return err;
 	}
 
-	ehci_resume(hcd, false);
+	ehci_resume(hcd, pdata->reset_on_resume);
 	return 0;
 }
 #endif /* CONFIG_PM_SLEEP */
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 6f0577b..52ef084 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -871,7 +871,7 @@ extern int	ehci_handshake(struct ehci_hcd *ehci, void __iomem *ptr,
 
 #ifdef CONFIG_PM
 extern int	ehci_suspend(struct usb_hcd *hcd, bool do_wakeup);
-extern int	ehci_resume(struct usb_hcd *hcd, bool hibernated);
+extern int	ehci_resume(struct usb_hcd *hcd, bool force_reset);
 #endif	/* CONFIG_PM */
 
 extern int	ehci_hub_control(struct usb_hcd	*hcd, u16 typeReq, u16 wValue,
diff --git a/include/linux/usb/ehci_pdriver.h b/include/linux/usb/ehci_pdriver.h
index 7eb4dcd..6287b39 100644
--- a/include/linux/usb/ehci_pdriver.h
+++ b/include/linux/usb/ehci_pdriver.h
@@ -34,6 +34,8 @@ struct usb_hcd;
  *			after initialization.
  * @no_io_watchdog:	set to 1 if the controller does not need the I/O
  *			watchdog to run.
+ * @reset_on_resume:	set to 1 if the controller needs to be reset after
+ * 			a suspend / resume cycle (but can't detect that itself).
  *
  * These are general configuration options for the EHCI controller. All of
  * these options are activating more or less workarounds for some hardware.
@@ -45,6 +47,7 @@ struct usb_ehci_pdata {
 	unsigned	big_endian_desc:1;
 	unsigned	big_endian_mmio:1;
 	unsigned	no_io_watchdog:1;
+	unsigned	reset_on_resume:1;
 
 	/* Turn on all power and clocks */
 	int (*power_on)(struct platform_device *pdev);
-- 
1.8.3.2

[PATCH 0/2] Support EHCI reset after resume quirk

[Wu Liang feng]

This series adds support for EHCI controller to do reset
after resume if necessory. 

Wu Liang feng (2):
  dt-bindings: usb-ehci: Add an optional property
    "needs-reset-on-resume"
  USB: ehci-platform: Support ehci reset after resume quirk

tested on rk3288 chromebook board

 Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
 drivers/usb/host/ehci-hcd.c                        | 6 +++---
 drivers/usb/host/ehci-platform.c                   | 6 +++++-
 drivers/usb/host/ehci.h                            | 2 +-
 include/linux/usb/ehci_pdriver.h                   | 3 +++
 5 files changed, 13 insertions(+), 5 deletions(-)

-- 
1.8.3.2

[PATCH 2/2] USB: ehci-platform: Support ehci reset after resume quirk

[Wu Liang feng]

The EHCI controller doesn't properly detect the case when
a device is removed during suspend. Specifically,when we
resume from suspend the EHCI controller maintaining the
USB state (FLAG_CF is 1 Current Connect Status is 1) but
a USB device (like a USB camera on rk3288) may have been
disconnected actually.

Let's add a quirk to force ehci to go into the
usb_root_hub_lost_power() path and reset after resume.
This should generally reset the whole controller and all
ports and initialize everything cleanly again, and bring
the devices back up.

As part of this, rename the "hibernation" paramter of
ehci_resume() to force_reset since hibernation is simply
another case where we can't trust the autodetected status
and need to force a reset of devices.

Signed-off-by: Wu Liang feng <wulf@rock-chips.com>
---
 drivers/usb/host/ehci-hcd.c      | 6 +++---
 drivers/usb/host/ehci-platform.c | 6 +++++-
 drivers/usb/host/ehci.h          | 2 +-
 include/linux/usb/ehci_pdriver.h | 3 +++
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 38bfeed..85e56d1 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1110,7 +1110,7 @@ int ehci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 EXPORT_SYMBOL_GPL(ehci_suspend);
 
 /* Returns 0 if power was preserved, 1 if power was lost */
-int ehci_resume(struct usb_hcd *hcd, bool hibernated)
+int ehci_resume(struct usb_hcd *hcd, bool force_reset)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci(hcd);
 
@@ -1124,12 +1124,12 @@ int ehci_resume(struct usb_hcd *hcd, bool hibernated)
 		return 0;		/* Controller is dead */
 
 	/*
-	 * If CF is still set and we aren't resuming from hibernation
+	 * If CF is still set and reset isn't forced
 	 * then we maintained suspend power.
 	 * Just undo the effect of ehci_suspend().
 	 */
 	if (ehci_readl(ehci, &ehci->regs->configured_flag) == FLAG_CF &&
-			!hibernated) {
+			!force_reset) {
 		int	mask = INTR_MASK;
 
 		ehci_prepare_ports_for_controller_resume(ehci);
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index 8557803..db5c29e 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -185,6 +185,10 @@ static int ehci_platform_probe(struct platform_device *dev)
 		if (of_property_read_bool(dev->dev.of_node, "big-endian"))
 			ehci->big_endian_mmio = ehci->big_endian_desc = 1;
 
+		if (of_property_read_bool(dev->dev.of_node,
+					  "needs-reset-on-resume"))
+			pdata->reset_on_resume = 1;
+
 		priv->phy = devm_phy_get(&dev->dev, "usb");
 		if (IS_ERR(priv->phy)) {
 			err = PTR_ERR(priv->phy);
@@ -340,7 +344,7 @@ static int ehci_platform_resume(struct device *dev)
 			return err;
 	}
 
-	ehci_resume(hcd, false);
+	ehci_resume(hcd, pdata->reset_on_resume);
 	return 0;
 }
 #endif /* CONFIG_PM_SLEEP */
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 6f0577b..52ef084 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -871,7 +871,7 @@ extern int	ehci_handshake(struct ehci_hcd *ehci, void __iomem *ptr,
 
 #ifdef CONFIG_PM
 extern int	ehci_suspend(struct usb_hcd *hcd, bool do_wakeup);
-extern int	ehci_resume(struct usb_hcd *hcd, bool hibernated);
+extern int	ehci_resume(struct usb_hcd *hcd, bool force_reset);
 #endif	/* CONFIG_PM */
 
 extern int	ehci_hub_control(struct usb_hcd	*hcd, u16 typeReq, u16 wValue,
diff --git a/include/linux/usb/ehci_pdriver.h b/include/linux/usb/ehci_pdriver.h
index 7eb4dcd..6287b39 100644
--- a/include/linux/usb/ehci_pdriver.h
+++ b/include/linux/usb/ehci_pdriver.h
@@ -34,6 +34,8 @@ struct usb_hcd;
  *			after initialization.
  * @no_io_watchdog:	set to 1 if the controller does not need the I/O
  *			watchdog to run.
+ * @reset_on_resume:	set to 1 if the controller needs to be reset after
+ * 			a suspend / resume cycle (but can't detect that itself).
  *
  * These are general configuration options for the EHCI controller. All of
  * these options are activating more or less workarounds for some hardware.
@@ -45,6 +47,7 @@ struct usb_ehci_pdata {
 	unsigned	big_endian_desc:1;
 	unsigned	big_endian_mmio:1;
 	unsigned	no_io_watchdog:1;
+	unsigned	reset_on_resume:1;
 
 	/* Turn on all power and clocks */
 	int (*power_on)(struct platform_device *pdev);
-- 
1.8.3.2

Re: [PATCH 2/2] USB: ehci-platform: Support ehci reset after resume quirk

[Alan Stern]

On Mon, 22 Dec 2014, Wu Liang feng wrote:

> The EHCI controller doesn't properly detect the case when

"The" EHCI controller?  I don't know what EHCI controller you're 
talking about, but my controllers don't have any trouble detecting 
device removal during suspend.

> a device is removed during suspend. Specifically,when we
> resume from suspend the EHCI controller maintaining the
> USB state (FLAG_CF is 1 Current Connect Status is 1) but
> a USB device (like a USB camera on rk3288) may have been
> disconnected actually.
> 
> Let's add a quirk to force ehci to go into the
> usb_root_hub_lost_power() path and reset after resume.
> This should generally reset the whole controller and all
> ports and initialize everything cleanly again, and bring
> the devices back up.

Isn't this solution too extreme?  What if the device was a flash 
storage drive and it wasn't unplugged during suspend?  This patch would 
force it to be removed, messing up any mounted filesystems, when there 
was no need.

Can you find a better way to work around the problem?

Alan Stern

Re: [PATCH 2/2] USB: ehci-platform: Support ehci reset after resume quirk

[Doug Anderson]

Alan,

On Mon, Dec 22, 2014 at 8:06 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 22 Dec 2014, Wu Liang feng wrote:
>
>> The EHCI controller doesn't properly detect the case when
>
> "The" EHCI controller?  I don't know what EHCI controller you're
> talking about, but my controllers don't have any trouble detecting
> device removal during suspend.

This is specifically the EHCI controller on rk3288.  Not sure why Wu
Liang feng removed that part of the description.


>> a device is removed during suspend. Specifically,when we
>> resume from suspend the EHCI controller maintaining the
>> USB state (FLAG_CF is 1 Current Connect Status is 1) but
>> a USB device (like a USB camera on rk3288) may have been
>> disconnected actually.
>>
>> Let's add a quirk to force ehci to go into the
>> usb_root_hub_lost_power() path and reset after resume.
>> This should generally reset the whole controller and all
>> ports and initialize everything cleanly again, and bring
>> the devices back up.
>
> Isn't this solution too extreme?  What if the device was a flash
> storage drive and it wasn't unplugged during suspend?  This patch would
> force it to be removed, messing up any mounted filesystems, when there
> was no need.

I'm told by Julius (CCed, who knows the USB stack infinitely better
than I do) that you can work around this using "persist".  I would
imagine that anyone on a machine using hibernation would run into the
same problem, right?


> Can you find a better way to work around the problem?

We asked a lot about this and you can find a whole set of detailed
discussion at:

https://chromium-review.googlesource.com/#/c/232077/1/drivers/usb/host/ehci-platform.c


Since I don't know the USB subsystem particularly well, my review
isn't terribly meaningful, but just in case:

Reviewed-by: Doug Anderson <dianders@chromium.org>


On rk3288-pinky I have tested that without this patch the EHCI
controller flips out when a USB webcam is plugged into the port and is
power cycled across suspend/resume.  With this patch the controller
properly unplugs / replugs the webcam.

Tested-by: Doug Anderson <dianders@chromium.org>


-Doug

Re: [PATCH 2/2] USB: ehci-platform: Support ehci reset after resume quirk

[Alan Stern]

On Mon, 22 Dec 2014, Doug Anderson wrote:

> Alan,
> 
> On Mon, Dec 22, 2014 at 8:06 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Mon, 22 Dec 2014, Wu Liang feng wrote:
> >
> >> The EHCI controller doesn't properly detect the case when
> >
> > "The" EHCI controller?  I don't know what EHCI controller you're
> > talking about, but my controllers don't have any trouble detecting
> > device removal during suspend.
> 
> This is specifically the EHCI controller on rk3288.  Not sure why Wu
> Liang feng removed that part of the description.

Maybe it could be put back in.

> > Isn't this solution too extreme?  What if the device was a flash
> > storage drive and it wasn't unplugged during suspend?  This patch would
> > force it to be removed, messing up any mounted filesystems, when there
> > was no need.
> 
> I'm told by Julius (CCed, who knows the USB stack infinitely better
> than I do) that you can work around this using "persist".  I would
> imagine that anyone on a machine using hibernation would run into the
> same problem, right?

> We asked a lot about this and you can find a whole set of detailed
> discussion at:
> 
> https://chromium-review.googlesource.com/#/c/232077/1/drivers/usb/host/ehci-platform.c
> 
> 
> Since I don't know the USB subsystem particularly well, my review
> isn't terribly meaningful, but just in case:
> 
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> 
> 
> On rk3288-pinky I have tested that without this patch the EHCI
> controller flips out when a USB webcam is plugged into the port and is
> power cycled across suspend/resume.  With this patch the controller
> properly unplugs / replugs the webcam.
> 
> Tested-by: Doug Anderson <dianders@chromium.org>

I assume you also tested the case where the webcam is _not_ 
power-cycled during the suspend.

On Mon, 22 Dec 2014, Julius Werner wrote:

> As Doug said, I think persist is the solution. We have essentially the
> same case: all we know is that there is now a device connected to the 
> same port that a device had been connected during suspend... but with 
> no guarantees whether it is the same device or in the same state. By  
> forcing people to use persist, we acknowledge that this has the same  
> risks (e.g. data corruption if a mounted mass storage device was
> swapped out for another one), and we benefit from the same safety
> checks like comparing the serial number.

Okay, I see.  This makes sense.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

Re: [PATCH 2/2] USB: ehci-platform: Support ehci reset after resume quirk

[Doug Anderson]

Hi,

On Mon, Dec 22, 2014 at 1:04 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 22 Dec 2014, Doug Anderson wrote:
>
>> Alan,
>>
>> On Mon, Dec 22, 2014 at 8:06 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Mon, 22 Dec 2014, Wu Liang feng wrote:
>> >
>> >> The EHCI controller doesn't properly detect the case when
>> >
>> > "The" EHCI controller?  I don't know what EHCI controller you're
>> > talking about, but my controllers don't have any trouble detecting
>> > device removal during suspend.
>>
>> This is specifically the EHCI controller on rk3288.  Not sure why Wu
>> Liang feng removed that part of the description.
>
> Maybe it could be put back in.

Sounds good.  Wu Liang feng, could you spin and make sure you say that
this is the EHCI controller in rk3288?  Make sure to keep tags that
were added (Acked, Reviewed, Tested).


>> On rk3288-pinky I have tested that without this patch the EHCI
>> controller flips out when a USB webcam is plugged into the port and is
>> power cycled across suspend/resume.  With this patch the controller
>> properly unplugs / replugs the webcam.
>>
>> Tested-by: Doug Anderson <dianders@chromium.org>
>
> I assume you also tested the case where the webcam is _not_
> power-cycled during the suspend.

Good point.  I was about 99% sure I tested that case, but I just
retested now and now I'm 100% sure.  ;)

-Doug

Re: [PATCH 2/2] USB: ehci-platform: Support ehci reset after resume quirk

[Julius Werner]

>> The EHCI controller doesn't properly detect the case when
>
> "The" EHCI controller?  I don't know what EHCI controller you're
> talking about, but my controllers don't have any trouble detecting
> device removal during suspend.

This is similar to other SoC-based controllers that loose state in
suspend, like on Samsung's Exynos. Usually we catch that with the
FLAG_CF check in ehci_resume(). In the case of RK3288 it unfortunately
leaves the CF flag (and other things, like PORTSC[PED] bits) set,
although it doesn't react to any events correctly. If the device
looses its power session the controller won't notice and then on
resume get stuck trying to send resume signals to something that had
been reset (or newly plugged in). I think since we can't trust the
controller to do anything right, the safest thing is to fall back to
the solution of resetting everything (at least we know that works),
and since the FLAG_CF check doesn't work we need another solution to
mark which controllers are affected.

> Isn't this solution too extreme?  What if the device was a flash
> storage drive and it wasn't unplugged during suspend?  This patch would
> force it to be removed, messing up any mounted filesystems, when there
> was no need.
>
> Can you find a better way to work around the problem?

As Doug said, I think persist is the solution. We have essentially the
same case: all we know is that there is now a device connected to the
same port that a device had been connected during suspend... but with
no guarantees whether it is the same device or in the same state. By
forcing people to use persist, we acknowledge that this has the same
risks (e.g. data corruption if a mounted mass storage device was
swapped out for another one), and we benefit from the same safety
checks like comparing the serial number.