[RFC] ehci-omap: simple suspend implementation

Andreas,

On 16/02/18 20:35, Andreas Kemnade wrote:

On Fri, 16 Feb 2018 13:13:11 -0500 (EST)
Alan Stern [off-list ref] wrote:

quoted

On Fri, 16 Feb 2018, Andreas Kemnade wrote:

quoted

This powers down the phy and on a gta04 it reduces
suspend current by 13 mA.
For unknown reasons usb does not power on properly.
Also calling usb_phy_shutdown() here feels wrong
apparently the reset line has to be activated.
usb_phy_set_suspend is not enough here. The power
consumption still stays approximately the same as
without any patch.

With a device connected the device does not enumerate
after resume. A rmmod ehci-omap ; modprobe ehci-omap
does not make it reenumerade.
So there is still something wrong here.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/usb/host/ehci-omap.c | 59 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 8d8bafc70c1f..0be2ccf8182a 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c

@@ -266,6 +266,58 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev)
 	return 0;
 }
 
+
+static int __maybe_unused ehci_omap_suspend(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+	int ret;
+	int i;
+
+	ret = ehci_suspend(hcd, false);
+	if (ret) {
+		dev_err(dev, "ehci suspend failed: %d\n", ret);
+		return ret;
+	}
+	for (i = 0; i < omap->nports; i++) {
+		if (omap->phy[i])
+			usb_phy_shutdown(omap->phy[i]);
+	}
+	pm_runtime_put_sync(dev);  

Why do you include a runtime PM call here, given that the driver
doesn't support runtime suspend or resume?

Well, the parent (drivers/mfd/omap-usb-host.c) has, and there are runtime PM
calls here in the _probe/_remove functions, so it seems to be sane to do it
here, too.

quoted

quoted

+
+	return 0;
+}
+
+static int __maybe_unused ehci_omap_resume(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+	int i;
+
+	pm_runtime_get_sync(dev);
+	/*
+	 * An undocumented "feature" in the OMAP3 EHCI controller,
+	 * causes suspended ports to be taken out of suspend when
+	 * the USBCMD.Run/Stop bit is cleared (for example when
+	 * we do ehci_bus_suspend).
+	 * This breaks suspend-resume if the root-hub is allowed
+	 * to suspend. Writing 1 to this undocumented register bit
+	 * disables this feature and restores normal behavior.
+	 */
+	ehci_write(hcd->regs, EHCI_INSNREG04,
+		   EHCI_INSNREG04_DISABLE_UNSUSPEND);  

Doesn't this code belong in ehci_hcd_omap_probe()?  I assume you only
need to set this undocumented bit once, not every time the controller
is suspended.  And in any case, according to the comment, you would
need to take care of this before the root hub is suspended -- by the
time the controller is suspended, it is already too late.

 
I am not really sure about this one. It is set in ehci_hcd_omap_probe()
so it is set before suspend. I set it here to ensure it is re-set when
the controller is powered down too much to keep register contents. I
do not know if that is the case. But I thought it would not harm.

If the Hardware SAR (Save and restore) functionality is enabled then
everything will be restored by hardware after a sleep to wake transition.

But you will need this patch to enable SAR for the USB power domain.
https://lkml.org/lkml/2013/7/10/356

Missing this might be the reason why things break for you after a system
suspend/resume.