Changes to power management in ehci-tegra

Changes to power management in ehci-tegra

[Alan Stern]

Benoit or anyone else:

Right now the ehci-tegra driver powers down the EHCI controller during 
bus suspend, if the appropriate parameter is set.  This is not the 
right thing to do, and it will break under some newly added changes to 
the USB core.

Instead, the driver should leave the controller powered up during bus 
suspend, and power it down during controller suspend.  Below is a patch 
that attempts to do this.

I have no way to test this patch; I haven't even compiled it.  Please 
see if it works okay, or if it needs to be fixed at all.

Thanks,

Alan Stern



 drivers/usb/host/ehci-tegra.c |  368 +++++++++++++++++++++---------------------
 1 file changed, 188 insertions(+), 180 deletions(-)

Index: usb-3.4/drivers/usb/host/ehci-tegra.c
===================================================================
--- usb-3.4.orig/drivers/usb/host/ehci-tegra.c
+++ usb-3.4/drivers/usb/host/ehci-tegra.c
@@ -24,6 +24,7 @@
 #include <linux/gpio.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include <linux/pm_runtime.h>
 
 #include <mach/usb_phy.h>
 #include <mach/iomap.h>
@@ -37,9 +38,7 @@ struct tegra_ehci_hcd {
 	struct clk *emc_clk;
 	struct usb_phy *transceiver;
 	int host_resumed;
-	int bus_suspended;
 	int port_resuming;
-	int power_down_on_bus_suspend;
 	enum tegra_usb_phy_port_speed port_speed;
 };
 
@@ -273,120 +272,6 @@ static void tegra_ehci_restart(struct us
 	up_write(&ehci_cf_port_reset_rwsem);
 }
 
-static int tegra_usb_suspend(struct usb_hcd *hcd)
-{
-	struct tegra_ehci_hcd *tegra = dev_get_drvdata(hcd->self.controller);
-	struct ehci_regs __iomem *hw = tegra->ehci->regs;
-	unsigned long flags;
-
-	spin_lock_irqsave(&tegra->ehci->lock, flags);
-
-	tegra->port_speed = (readl(&hw->port_status[0]) >> 26) & 0x3;
-	ehci_halt(tegra->ehci);
-	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
-
-	spin_unlock_irqrestore(&tegra->ehci->lock, flags);
-
-	tegra_ehci_power_down(hcd);
-	return 0;
-}
-
-static int tegra_usb_resume(struct usb_hcd *hcd)
-{
-	struct tegra_ehci_hcd *tegra = dev_get_drvdata(hcd->self.controller);
-	struct ehci_hcd	*ehci = hcd_to_ehci(hcd);
-	struct ehci_regs __iomem *hw = ehci->regs;
-	unsigned long val;
-
-	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
-	tegra_ehci_power_up(hcd);
-
-	if (tegra->port_speed > TEGRA_USB_PHY_PORT_SPEED_HIGH) {
-		/* Wait for the phy to detect new devices
-		 * before we restart the controller */
-		msleep(10);
-		goto restart;
-	}
-
-	/* Force the phy to keep data lines in suspend state */
-	tegra_ehci_phy_restore_start(tegra->phy, tegra->port_speed);
-
-	/* Enable host mode */
-	tdi_reset(ehci);
-
-	/* Enable Port Power */
-	val = readl(&hw->port_status[0]);
-	val |= PORT_POWER;
-	writel(val, &hw->port_status[0]);
-	udelay(10);
-
-	/* Check if the phy resume from LP0. When the phy resume from LP0
-	 * USB register will be reset. */
-	if (!readl(&hw->async_next)) {
-		/* Program the field PTC based on the saved speed mode */
-		val = readl(&hw->port_status[0]);
-		val &= ~PORT_TEST(~0);
-		if (tegra->port_speed == TEGRA_USB_PHY_PORT_SPEED_HIGH)
-			val |= PORT_TEST_FORCE;
-		else if (tegra->port_speed == TEGRA_USB_PHY_PORT_SPEED_FULL)
-			val |= PORT_TEST(6);
-		else if (tegra->port_speed == TEGRA_USB_PHY_PORT_SPEED_LOW)
-			val |= PORT_TEST(7);
-		writel(val, &hw->port_status[0]);
-		udelay(10);
-
-		/* Disable test mode by setting PTC field to NORMAL_OP */
-		val = readl(&hw->port_status[0]);
-		val &= ~PORT_TEST(~0);
-		writel(val, &hw->port_status[0]);
-		udelay(10);
-	}
-
-	/* Poll until CCS is enabled */
-	if (handshake(ehci, &hw->port_status[0], PORT_CONNECT,
-						 PORT_CONNECT, 2000)) {
-		pr_err("%s: timeout waiting for PORT_CONNECT\n", __func__);
-		goto restart;
-	}
-
-	/* Poll until PE is enabled */
-	if (handshake(ehci, &hw->port_status[0], PORT_PE,
-						 PORT_PE, 2000)) {
-		pr_err("%s: timeout waiting for USB_PORTSC1_PE\n", __func__);
-		goto restart;
-	}
-
-	/* Clear the PCI status, to avoid an interrupt taken upon resume */
-	val = readl(&hw->status);
-	val |= STS_PCD;
-	writel(val, &hw->status);
-
-	/* Put controller in suspend mode by writing 1 to SUSP bit of PORTSC */
-	val = readl(&hw->port_status[0]);
-	if ((val & PORT_POWER) && (val & PORT_PE)) {
-		val |= PORT_SUSPEND;
-		writel(val, &hw->port_status[0]);
-
-		/* Wait until port suspend completes */
-		if (handshake(ehci, &hw->port_status[0], PORT_SUSPEND,
-							 PORT_SUSPEND, 1000)) {
-			pr_err("%s: timeout waiting for PORT_SUSPEND\n",
-								__func__);
-			goto restart;
-		}
-	}
-
-	tegra_ehci_phy_restore_end(tegra->phy);
-	return 0;
-
-restart:
-	if (tegra->port_speed <= TEGRA_USB_PHY_PORT_SPEED_HIGH)
-		tegra_ehci_phy_restore_end(tegra->phy);
-
-	tegra_ehci_restart(hcd);
-	return 0;
-}
-
 static void tegra_ehci_shutdown(struct usb_hcd *hcd)
 {
 	struct tegra_ehci_hcd *tegra = dev_get_drvdata(hcd->self.controller);
@@ -434,36 +319,6 @@ static int tegra_ehci_setup(struct usb_h
 	return retval;
 }
 
-#ifdef CONFIG_PM
-static int tegra_ehci_bus_suspend(struct usb_hcd *hcd)
-{
-	struct tegra_ehci_hcd *tegra = dev_get_drvdata(hcd->self.controller);
-	int error_status = 0;
-
-	error_status = ehci_bus_suspend(hcd);
-	if (!error_status && tegra->power_down_on_bus_suspend) {
-		tegra_usb_suspend(hcd);
-		tegra->bus_suspended = 1;
-	}
-
-	return error_status;
-}
-
-static int tegra_ehci_bus_resume(struct usb_hcd *hcd)
-{
-	struct tegra_ehci_hcd *tegra = dev_get_drvdata(hcd->self.controller);
-
-	if (tegra->bus_suspended && tegra->power_down_on_bus_suspend) {
-		tegra_usb_resume(hcd);
-		tegra->bus_suspended = 0;
-	}
-
-	tegra_usb_phy_preresume(tegra->phy);
-	tegra->port_resuming = 1;
-	return ehci_bus_resume(hcd);
-}
-#endif
-
 struct temp_buffer {
 	void *kmalloc_ptr;
 	void *old_xfer_buffer;
@@ -574,8 +429,8 @@ static const struct hc_driver tegra_ehci
 	.hub_control		= tegra_ehci_hub_control,
 	.clear_tt_buffer_complete = ehci_clear_tt_buffer_complete,
 #ifdef CONFIG_PM
-	.bus_suspend		= tegra_ehci_bus_suspend,
-	.bus_resume		= tegra_ehci_bus_resume,
+	.bus_suspend		= ehci_bus_suspend,
+	.bus_resume		= ehci_bus_resume,
 #endif
 	.relinquish_port	= ehci_relinquish_port,
 	.port_handed_over	= ehci_port_handed_over,
@@ -608,6 +463,178 @@ static int setup_vbus_gpio(struct platfo
 	return err;
 }
 
+#ifdef CONFIG_PM
+
+static int controller_suspend(struct device *dev)
+{
+	struct tegra_ehci_hcd *tegra =
+			platform_get_drvdata(to_platform_device(dev));
+	struct ehci_hcd	*ehci = tegra->ehci;
+	struct usb_hcd *hcd = ehci_to_hcd(ehci);
+	struct ehci_regs __iomem *hw = ehci->regs;
+	unsigned long flags;
+
+	if (time_before(jiffies, ehci->next_statechange))
+		msleep(10);
+
+	spin_lock_irqsave(&ehci->lock, flags);
+
+	tegra->port_speed = (readl(&hw->port_status[0]) >> 26) & 0x3;
+	ehci_halt(ehci);
+	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+
+	spin_unlock_irqrestore(&ehci->lock, flags);
+
+	tegra_ehci_power_down(hcd);
+	return 0;
+}
+
+static int controller_resume(struct device *dev)
+{
+	struct tegra_ehci_hcd *tegra =
+			platform_get_drvdata(to_platform_device(dev));
+	struct ehci_hcd	*ehci = tegra->ehci;
+	struct usb_hcd *hcd = ehci_to_hcd(ehci);
+	struct ehci_regs __iomem *hw = ehci->regs;
+	unsigned long val;
+
+	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+	tegra_ehci_power_up(hcd);
+
+	if (tegra->port_speed > TEGRA_USB_PHY_PORT_SPEED_HIGH) {
+		/* Wait for the phy to detect new devices
+		 * before we restart the controller */
+		msleep(10);
+		goto restart;
+	}
+
+	/* Force the phy to keep data lines in suspend state */
+	tegra_ehci_phy_restore_start(tegra->phy, tegra->port_speed);
+
+	/* Enable host mode */
+	tdi_reset(ehci);
+
+	/* Enable Port Power */
+	val = readl(&hw->port_status[0]);
+	val |= PORT_POWER;
+	writel(val, &hw->port_status[0]);
+	udelay(10);
+
+	/* Check if the phy resume from LP0. When the phy resume from LP0
+	 * USB register will be reset. */
+	if (!readl(&hw->async_next)) {
+		/* Program the field PTC based on the saved speed mode */
+		val = readl(&hw->port_status[0]);
+		val &= ~PORT_TEST(~0);
+		if (tegra->port_speed == TEGRA_USB_PHY_PORT_SPEED_HIGH)
+			val |= PORT_TEST_FORCE;
+		else if (tegra->port_speed == TEGRA_USB_PHY_PORT_SPEED_FULL)
+			val |= PORT_TEST(6);
+		else if (tegra->port_speed == TEGRA_USB_PHY_PORT_SPEED_LOW)
+			val |= PORT_TEST(7);
+		writel(val, &hw->port_status[0]);
+		udelay(10);
+
+		/* Disable test mode by setting PTC field to NORMAL_OP */
+		val = readl(&hw->port_status[0]);
+		val &= ~PORT_TEST(~0);
+		writel(val, &hw->port_status[0]);
+		udelay(10);
+	}
+
+	/* Poll until CCS is enabled */
+	if (handshake(ehci, &hw->port_status[0], PORT_CONNECT,
+						 PORT_CONNECT, 2000)) {
+		pr_err("%s: timeout waiting for PORT_CONNECT\n", __func__);
+		goto restart;
+	}
+
+	/* Poll until PE is enabled */
+	if (handshake(ehci, &hw->port_status[0], PORT_PE,
+						 PORT_PE, 2000)) {
+		pr_err("%s: timeout waiting for USB_PORTSC1_PE\n", __func__);
+		goto restart;
+	}
+
+	/* Clear the PCI status, to avoid an interrupt taken upon resume */
+	val = readl(&hw->status);
+	val |= STS_PCD;
+	writel(val, &hw->status);
+
+	/* Put controller in suspend mode by writing 1 to SUSP bit of PORTSC */
+	val = readl(&hw->port_status[0]);
+	if ((val & PORT_POWER) && (val & PORT_PE)) {
+		val |= PORT_SUSPEND;
+		writel(val, &hw->port_status[0]);
+
+		/* Wait until port suspend completes */
+		if (handshake(ehci, &hw->port_status[0], PORT_SUSPEND,
+							 PORT_SUSPEND, 1000)) {
+			pr_err("%s: timeout waiting for PORT_SUSPEND\n",
+								__func__);
+			goto restart;
+		}
+	}
+
+	tegra_ehci_phy_restore_end(tegra->phy);
+	goto done;
+
+ restart:
+	if (tegra->port_speed <= TEGRA_USB_PHY_PORT_SPEED_HIGH)
+		tegra_ehci_phy_restore_end(tegra->phy);
+
+	tegra_ehci_restart(hcd);
+
+ done:
+	tegra_usb_phy_preresume(tegra->phy);
+	tegra->port_resuming = 1;
+	return 0;
+}
+
+static int tegra_ehci_suspend(struct device *dev)
+{
+	struct tegra_ehci_hcd *tegra =
+			platform_get_drvdata(to_platform_device(dev));
+	struct usb_hcd *hcd = ehci_to_hcd(tegra->ehci);
+	int rc = 0;
+
+	if (HCD_HW_ACCESSIBLE(hcd))
+		rc = controller_suspend(dev);
+	return rc;
+}
+
+static int tegra_ehci_resume(struct device *dev)
+{
+	int rc;
+
+	rc = controller_resume(dev);
+	if (rc == 0) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
+	return rc;
+}
+
+static int tegra_ehci_runtime_suspend(struct device *dev)
+{
+	return controller_suspend(dev);
+}
+
+static int tegra_ehci_resume(struct device *dev)
+{
+	return controller_resume(dev);
+}
+
+static const struct dev_pm_ops tegra_ehci_pm_ops = {
+	.suspend	= tegra_ehci_suspend,
+	.resume		= tegra_ehci_resume,
+	.runtime_suspend = tegra_ehci_runtime_suspend,
+	.runtime_resume	= tegra_ehci_runtime_resume,
+};
+
+#endif
+
 static u64 tegra_ehci_dma_mask = DMA_BIT_MASK(32);
 
 static int tegra_ehci_probe(struct platform_device *pdev)
@@ -722,7 +749,6 @@ static int tegra_ehci_probe(struct platf
 	}
 
 	tegra->host_resumed = 1;
-	tegra->power_down_on_bus_suspend = pdata->power_down_on_bus_suspend;
 	tegra->ehci = hcd_to_ehci(hcd);
 
 	irq = platform_get_irq(pdev, 0);
@@ -747,6 +773,12 @@ static int tegra_ehci_probe(struct platf
 		goto fail;
 	}
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_get_noresume(&pdev->dev);
+	if (!pdata->power_down_on_bus_suspend)
+		pm_runtime_forbid(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_put_sync(&pdev->dev);
 	return err;
 
 fail:
@@ -773,33 +805,6 @@ fail_hcd:
 	return err;
 }
 
-#ifdef CONFIG_PM
-static int tegra_ehci_resume(struct platform_device *pdev)
-{
-	struct tegra_ehci_hcd *tegra = platform_get_drvdata(pdev);
-	struct usb_hcd *hcd = ehci_to_hcd(tegra->ehci);
-
-	if (tegra->bus_suspended)
-		return 0;
-
-	return tegra_usb_resume(hcd);
-}
-
-static int tegra_ehci_suspend(struct platform_device *pdev, pm_message_t state)
-{
-	struct tegra_ehci_hcd *tegra = platform_get_drvdata(pdev);
-	struct usb_hcd *hcd = ehci_to_hcd(tegra->ehci);
-
-	if (tegra->bus_suspended)
-		return 0;
-
-	if (time_before(jiffies, tegra->ehci->next_statechange))
-		msleep(10);
-
-	return tegra_usb_suspend(hcd);
-}
-#endif
-
 static int tegra_ehci_remove(struct platform_device *pdev)
 {
 	struct tegra_ehci_hcd *tegra = platform_get_drvdata(pdev);
@@ -808,6 +813,10 @@ static int tegra_ehci_remove(struct plat
 	if (tegra == NULL || hcd == NULL)
 		return -EINVAL;
 
+	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+
 #ifdef CONFIG_USB_OTG_UTILS
 	if (tegra->transceiver) {
 		otg_set_host(tegra->transceiver->otg, NULL);
@@ -848,13 +857,12 @@ static struct of_device_id tegra_ehci_of
 static struct platform_driver tegra_ehci_driver = {
 	.probe		= tegra_ehci_probe,
 	.remove		= tegra_ehci_remove,
-#ifdef CONFIG_PM
-	.suspend	= tegra_ehci_suspend,
-	.resume		= tegra_ehci_resume,
-#endif
 	.shutdown	= tegra_ehci_hcd_shutdown,
 	.driver		= {
 		.name	= "tegra-ehci",
 		.of_match_table = tegra_ehci_of_match,
+#ifdef CONFIG_PM
+		.pm	= &tegra_ehci_pm_ops,
+#endif
 	}
 };

Re: Changes to power management in ehci-tegra

[Stephen Warren]

On 04/12/2012 12:07 PM, Alan Stern wrote:
> Benoit or anyone else:
> 
> Right now the ehci-tegra driver powers down the EHCI controller during 
> bus suspend, if the appropriate parameter is set.  This is not the 
> right thing to do, and it will break under some newly added changes to 
> the USB core.
> 
> Instead, the driver should leave the controller powered up during bus 
> suspend, and power it down during controller suspend.  Below is a patch 
> that attempts to do this.
> 
> I have no way to test this patch; I haven't even compiled it.  Please 
> see if it works okay, or if it needs to be fixed at all.

I can test this. How would I trigger the bus conditions this patch
affects? Note that we don't have system suspend on Tegra yet.

Re: Changes to power management in ehci-tegra

[Alan Stern]

On Thu, 12 Apr 2012, Stephen Warren wrote:

> On 04/12/2012 12:07 PM, Alan Stern wrote:
> > Benoit or anyone else:
> > 
> > Right now the ehci-tegra driver powers down the EHCI controller during 
> > bus suspend, if the appropriate parameter is set.  This is not the 
> > right thing to do, and it will break under some newly added changes to 
> > the USB core.
> > 
> > Instead, the driver should leave the controller powered up during bus 
> > suspend, and power it down during controller suspend.  Below is a patch 
> > that attempts to do this.
> > 
> > I have no way to test this patch; I haven't even compiled it.  Please 
> > see if it works okay, or if it needs to be fixed at all.
> 
> I can test this. How would I trigger the bus conditions this patch
> affects? Note that we don't have system suspend on Tegra yet.

If you do:

	echo auto >/sys/devices/.../power/control

(where the ... gets filled in with the path to the EHCI controller),
then the controller should automatically get powered down whenever the
bus does.  The bus will get powered down whenever no USB devices are
plugged in (or the only attached USB devices are things like hubs that
will themselves get suspended) after a 2-second delay.

This is intended to be equivalent to enabling the
power_down_on_bus_suspend flag in the tegra_ehci_platform_data 
structure.  That
is, tegra_ehci_probe checks that flag and sets the PM control to auto
if the flag is enabled.

Alan Stern

Re: Changes to power management in ehci-tegra

[Stephen Warren]

On 04/12/2012 12:45 PM, Alan Stern wrote:
> On Thu, 12 Apr 2012, Stephen Warren wrote:
> 
>> On 04/12/2012 12:07 PM, Alan Stern wrote:
>>> Benoit or anyone else:
>>>
>>> Right now the ehci-tegra driver powers down the EHCI controller during 
>>> bus suspend, if the appropriate parameter is set.  This is not the 
>>> right thing to do, and it will break under some newly added changes to 
>>> the USB core.
>>>
>>> Instead, the driver should leave the controller powered up during bus 
>>> suspend, and power it down during controller suspend.  Below is a patch 
>>> that attempts to do this.
>>>
>>> I have no way to test this patch; I haven't even compiled it.  Please 
>>> see if it works okay, or if it needs to be fixed at all.
>>
>> I can test this. How would I trigger the bus conditions this patch
>> affects? Note that we don't have system suspend on Tegra yet.
> 
> If you do:
> 
> 	echo auto >/sys/devices/.../power/control
> 
> (where the ... gets filled in with the path to the EHCI controller),
> then the controller should automatically get powered down whenever the
> bus does.  The bus will get powered down whenever no USB devices are
> plugged in (or the only attached USB devices are things like hubs that
> will themselves get suspended) after a 2-second delay.

One small issue with the patch: There are two copies of
tegra_ehci_resume(). I assume the second should be renamed
tegra_ehci_runtime_resume().

I've tested the patch, and can unplug a USB device, wait 10 seconds,
then plug it back in, and everything works as expected.

However, I don't notice /sys/devices/.../power/runtime_status ever
saying anything other than active, and I can't cat autosuspend_delay_ms,
so I assume I'm still not triggering the path you want me to test. I do
have PM_RUNTIME turned on. Any ideas? Do I need some patch to the USB
core to enable this? I tested on next-20120413 plus a few local patches.

Re: Changes to power management in ehci-tegra

[Alan Stern]

On Fri, 13 Apr 2012, Stephen Warren wrote:

> > If you do:
> > 
> > 	echo auto >/sys/devices/.../power/control
> > 
> > (where the ... gets filled in with the path to the EHCI controller),
> > then the controller should automatically get powered down whenever the
> > bus does.  The bus will get powered down whenever no USB devices are
> > plugged in (or the only attached USB devices are things like hubs that
> > will themselves get suspended) after a 2-second delay.
> 
> One small issue with the patch: There are two copies of
> tegra_ehci_resume(). I assume the second should be renamed
> tegra_ehci_runtime_resume().

Whoops!  You're right, of course.  I'll update my patch.

> I've tested the patch, and can unplug a USB device, wait 10 seconds,
> then plug it back in, and everything works as expected.
> 
> However, I don't notice /sys/devices/.../power/runtime_status ever
> saying anything other than active, and I can't cat autosuspend_delay_ms,
> so I assume I'm still not triggering the path you want me to test. I do
> have PM_RUNTIME turned on. Any ideas? Do I need some patch to the USB
> core to enable this? I tested on next-20120413 plus a few local patches.

autosuspend_delay_ms isn't relevant here; the controller should be 
suspended with no delay as soon as the root hub suspends.  No other 
patches should be needed; it sounds like you're doing everything right.

If you enable CONFIG_PM_ADVANCED_DEBUG along with CONFIG_PM_RUNTIME, 
what do all the other power/runtime_* files show?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Changes to power management in ehci-tegra

[Stephen Warren]

On 04/13/2012 01:46 PM, Alan Stern wrote:
> On Fri, 13 Apr 2012, Stephen Warren wrote:
>> I've tested the patch, and can unplug a USB device, wait 10 seconds,
>> then plug it back in, and everything works as expected.
>>
>> However, I don't notice /sys/devices/.../power/runtime_status ever
>> saying anything other than active, and I can't cat autosuspend_delay_ms,
>> so I assume I'm still not triggering the path you want me to test. I do
>> have PM_RUNTIME turned on. Any ideas? Do I need some patch to the USB
>> core to enable this? I tested on next-20120413 plus a few local patches.
> 
> autosuspend_delay_ms isn't relevant here; the controller should be 
> suspended with no delay as soon as the root hub suspends.  No other 
> patches should be needed; it sounds like you're doing everything right.
> 
> If you enable CONFIG_PM_ADVANCED_DEBUG along with CONFIG_PM_RUNTIME, 
> what do all the other power/runtime_* files show?

The EHCI controller's active_kids count stays at 1 all the time. I
assume that's the root USB hub device. Looking in its power/ directory,
I see that even when nothing is plugged in, runtime_status==enabled,
even with no active_kids or runtime_usage:

[  242.611209] usb 3-1: USB disconnect, device number 4
root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat async
enabled
root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat autosuspend_delay_ms
0
root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat control
auto
root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat runtime_active_kids
0
root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat runtime_active_time
262840
root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat runtime_enabled
enabled
root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat runtime_status
active
root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat
runtime_suspended_time
0
root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat runtime_usage
0

Are the wakeup_* files in this directory relevant? wakeup says
"disabled", and all the others are empty.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Changes to power management in ehci-tegra

[Alan Stern]

On Fri, 13 Apr 2012, Stephen Warren wrote:

> On 04/13/2012 01:46 PM, Alan Stern wrote:
> > On Fri, 13 Apr 2012, Stephen Warren wrote:
> >> I've tested the patch, and can unplug a USB device, wait 10 seconds,
> >> then plug it back in, and everything works as expected.
> >>
> >> However, I don't notice /sys/devices/.../power/runtime_status ever
> >> saying anything other than active, and I can't cat autosuspend_delay_ms,
> >> so I assume I'm still not triggering the path you want me to test. I do
> >> have PM_RUNTIME turned on. Any ideas? Do I need some patch to the USB
> >> core to enable this? I tested on next-20120413 plus a few local patches.
> > 
> > autosuspend_delay_ms isn't relevant here; the controller should be 
> > suspended with no delay as soon as the root hub suspends.  No other 
> > patches should be needed; it sounds like you're doing everything right.
> > 
> > If you enable CONFIG_PM_ADVANCED_DEBUG along with CONFIG_PM_RUNTIME, 
> > what do all the other power/runtime_* files show?
> 
> The EHCI controller's active_kids count stays at 1 all the time. I
> assume that's the root USB hub device. Looking in its power/ directory,
> I see that even when nothing is plugged in, runtime_status==enabled,
> even with no active_kids or runtime_usage:

That does indeed sound like a bug.

> [  242.611209] usb 3-1: USB disconnect, device number 4
> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat async
> enabled
> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat autosuspend_delay_ms
> 0

Odd...  This should contain 2000.  Maybe some userspace program has 
changed it?  Either value would be okay, however.

You could try writing 2000 to this file, then plugging in a device and 
unplugging it.  The root hub should then suspend 2 seconds after you 
unplug the device.

> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat control
> auto
> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat runtime_active_kids
> 0
> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat runtime_active_time
> 262840
> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat runtime_enabled
> enabled
> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat runtime_status
> active
> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat
> runtime_suspended_time
> 0
> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat runtime_usage
> 0
> 
> Are the wakeup_* files in this directory relevant? wakeup says
> "disabled", and all the others are empty.

No, the wakeup files aren't relevant -- they refer to system sleep, not
runtime suspend.  Is CONFIG_PM_SLEEP disabled?  It should be, since you
don't support system suspend.

I don't know what's going wrong.  Maybe if you enable CONFIG_USB_DEBUG 
while running your test, something will show up.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Changes to power management in ehci-tegra

[Stephen Warren]

On 04/13/2012 03:00 PM, Alan Stern wrote:
> On Fri, 13 Apr 2012, Stephen Warren wrote:
> 
>> On 04/13/2012 01:46 PM, Alan Stern wrote:
>>> On Fri, 13 Apr 2012, Stephen Warren wrote:
>>>> I've tested the patch, and can unplug a USB device, wait 10 seconds,
>>>> then plug it back in, and everything works as expected.
>>>>
>>>> However, I don't notice /sys/devices/.../power/runtime_status ever
>>>> saying anything other than active, and I can't cat autosuspend_delay_ms,
>>>> so I assume I'm still not triggering the path you want me to test. I do
>>>> have PM_RUNTIME turned on. Any ideas? Do I need some patch to the USB
>>>> core to enable this? I tested on next-20120413 plus a few local patches.
>>>
>>> autosuspend_delay_ms isn't relevant here; the controller should be 
>>> suspended with no delay as soon as the root hub suspends.  No other 
>>> patches should be needed; it sounds like you're doing everything right.
>>>
>>> If you enable CONFIG_PM_ADVANCED_DEBUG along with CONFIG_PM_RUNTIME, 
>>> what do all the other power/runtime_* files show?
>>
>> The EHCI controller's active_kids count stays at 1 all the time. I
>> assume that's the root USB hub device. Looking in its power/ directory,
>> I see that even when nothing is plugged in, runtime_status==enabled,
>> even with no active_kids or runtime_usage:
> 
> That does indeed sound like a bug.
> 
>> [  242.611209] usb 3-1: USB disconnect, device number 4
>> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat async
>> enabled
>> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat autosuspend_delay_ms
>> 0
> 
> Odd...  This should contain 2000.  Maybe some userspace program has 
> changed it?  Either value would be okay, however.
> 
> You could try writing 2000 to this file, then plugging in a device and 
> unplugging it.  The root hub should then suspend 2 seconds after you 
> unplug the device.

There's no difference if I write 2000 to this file.

I've tried turning on dynamic debug for the entire of USB. None of those
fire, which I believe implies there aren't any attempted suspends of the
Tegra EHCI controller that fail. I also added a few extra printks to the
Tegra EHCI controller's suspend/resume functions which don't fire.

Is this anything to do with it:

./ehci-hub.c:615:	/* FIXME autosuspend idle root hubs */

I'm at a loss how to do anything more useful here.

Re: Changes to power management in ehci-tegra

[Alan Stern]

On Mon, 16 Apr 2012, Stephen Warren wrote:

> >> The EHCI controller's active_kids count stays at 1 all the time. I
> >> assume that's the root USB hub device. Looking in its power/ directory,
> >> I see that even when nothing is plugged in, runtime_status==enabled,
> >> even with no active_kids or runtime_usage:
> > 
> > That does indeed sound like a bug.
> > 
> >> [  242.611209] usb 3-1: USB disconnect, device number 4
> >> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat async
> >> enabled
> >> root@ilyich:/sys/devices/tegra-ehci.2/usb3/power# cat autosuspend_delay_ms
> >> 0
> > 
> > Odd...  This should contain 2000.  Maybe some userspace program has 
> > changed it?  Either value would be okay, however.
> > 
> > You could try writing 2000 to this file, then plugging in a device and 
> > unplugging it.  The root hub should then suspend 2 seconds after you 
> > unplug the device.
> 
> There's no difference if I write 2000 to this file.
> 
> I've tried turning on dynamic debug for the entire of USB. None of those
> fire, which I believe implies there aren't any attempted suspends of the
> Tegra EHCI controller that fail. I also added a few extra printks to the
> Tegra EHCI controller's suspend/resume functions which don't fire.
> 
> Is this anything to do with it:
> 
> ./ehci-hub.c:615:	/* FIXME autosuspend idle root hubs */

No, that's a very old, fossil comment.  It should have been removed 
years ago.

> I'm at a loss how to do anything more useful here.

We need to find out why the root hub isn't suspending.  Can you do some 
debugging in drivers/usb/core/driver.c?  The routines to check are 
autosuspend_check(), usb_runtime_idle(), and usb_runtime_suspend().

In particular, if autosuspend_check() doesn't return 0, what is the 
reason?  And if it does, what does the usb_suspend_both() call return?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Changes to power management in ehci-tegra

[Stephen Warren]

On 04/17/2012 08:56 AM, Alan Stern wrote:
> On Mon, 16 Apr 2012, Stephen Warren wrote:
> 
>>>> The EHCI controller's active_kids count stays at 1 all the time. I
>>>> assume that's the root USB hub device. Looking in its power/ directory,
>>>> I see that even when nothing is plugged in, runtime_status==enabled,
>>>> even with no active_kids or runtime_usage:
>>>
>>> That does indeed sound like a bug.

Aha, there's a CONFIG_USB_SUSPEND which I didn't have enabled. Now I see
the controller suspending.

However, the controller suspends immediately the system boots even if
there's a device already connected, and never resumes if there wasn't
connected at boot, but one is plugged in later.

In this state, I must set tegra-ehci.2/usb3/power/control=on to get
anything to work. Forcing just the controller on
(tegra-ehci.2/power/control=on) doesn't fix this.

After than, I can set tegra-ehci.2/usb3/power/control=auto, and see the
root hub (tegra-ehci.2/usb3/power) suspending/resuming as devices are
unplugged/plugged.

I just can't have tegra-ehci.2/power/control=auto while nothing is
plugged in, or the cycle starts again.

I assume this a bug in the Tegra EHCI driver's suspend implementation?

Re: Changes to power management in ehci-tegra

[Alan Stern]

On Tue, 17 Apr 2012, Stephen Warren wrote:

> Aha, there's a CONFIG_USB_SUSPEND which I didn't have enabled. Now I see
> the controller suspending.

Good job.  I should have thought of that...

> However, the controller suspends immediately the system boots even if
> there's a device already connected, and never resumes if there wasn't
> connected at boot, but one is plugged in later.

The general idea is that when usb_add_hcd() registers the root hub, the
hub driver prevents the root hub from autosuspending until it has a
chance to scan the ports with plugged-in devices.  And naturally, the
controller won't suspend while the root hub is running.  So maybe the
root hub doesn't see the connected devices?

You can find out by collecting a usbmon trace for bus 0, starting
before you load the ehci-hcd module.

> In this state, I must set tegra-ehci.2/usb3/power/control=on to get
> anything to work. Forcing just the controller on
> (tegra-ehci.2/power/control=on) doesn't fix this.

Yeah, powering up the controller doesn't necessarily cause the root hub
to be powered up too.  Of course, if power_down_on_bus_suspend isn't
enabled then the controller's power/control is supposed to be set to
"on" by default.

> After than, I can set tegra-ehci.2/usb3/power/control=auto, and see the
> root hub (tegra-ehci.2/usb3/power) suspending/resuming as devices are
> unplugged/plugged.
> 
> I just can't have tegra-ehci.2/power/control=auto while nothing is
> plugged in, or the cycle starts again.

In other words, if both the root hub and the controller are powered 
down, then neither one wakes up when a device is plugged in?

> I assume this a bug in the Tegra EHCI driver's suspend implementation?

Actually it sounds like a bug in the controller's wakeup mechanism.  
Maybe a hardware bug, maybe a software bug.

While looking through the code, I didn't notice anything about enabling
wakeups.  But I wasn't looking very carefully, because I don't know how
the Tegra platform works.  A quick search through the patch now seems
to show that wakeups never get enabled at all!

How well does the existing driver work if you enable the 
power_down_on_bus_suspend flag in the platform data?  

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Changes to power management in ehci-tegra

[Stephen Warren]

On 04/18/2012 09:22 AM, Alan Stern wrote:
> On Tue, 17 Apr 2012, Stephen Warren wrote:
...
>> After than, I can set tegra-ehci.2/usb3/power/control=auto, and see the
>> root hub (tegra-ehci.2/usb3/power) suspending/resuming as devices are
>> unplugged/plugged.
>>
>> I just can't have tegra-ehci.2/power/control=auto while nothing is
>> plugged in, or the cycle starts again.
> 
> In other words, if both the root hub and the controller are powered 
> down, then neither one wakes up when a device is plugged in?

Yes.

>> I assume this a bug in the Tegra EHCI driver's suspend implementation?
> 
> Actually it sounds like a bug in the controller's wakeup mechanism.  
> Maybe a hardware bug, maybe a software bug.
> 
> While looking through the code, I didn't notice anything about enabling
> wakeups.  But I wasn't looking very carefully, because I don't know how
> the Tegra platform works.  A quick search through the patch now seems
> to show that wakeups never get enabled at all!
> 
> How well does the existing driver work if you enable the 
> power_down_on_bus_suspend flag in the platform data?  

power_down_on_bus_suspend is always enabled for the Tegra EHCI controllers.

The existing driver works fine (with or without USB_SUSPEND=y and
PM_RUNTIME=y), although doesn't ever runtime-suspended, so I guess this
is just hiding the problem:

/sys/devices/tegra-ehci.2# cat {,usb3/}power/{control,runtime_status}
auto
unsupported
auto
active

(I see the same with or without anything plugged in)

By the way, how broken is the current tegra_ehci_hub_control()? I see
that it mostly duplicates ehci_hub_control() for a few cases, but has
diverged a little since, e.g. the set_bit()/clear_bit() calls are using
"wIndex - 1" instead of "wIndex" etc. Also, I wonder if the logic in the
ClearPortFeature/USB_PORT_FEAT_ENABLE case might be worth moving into
ehci-hub.c, if it's generally applicable? I did try removing that
function and making the driver use ehci_hub_control() instead, but that
didn't solve the resume issue.

Re: Changes to power management in ehci-tegra

[Stephen Warren]

On 04/18/2012 09:22 AM, Alan Stern wrote:
> On Tue, 17 Apr 2012, Stephen Warren wrote:
...
> In other words, if both the root hub and the controller are powered 
> down, then neither one wakes up when a device is plugged in?
> 
>> I assume this a bug in the Tegra EHCI driver's suspend implementation?
> 
> Actually it sounds like a bug in the controller's wakeup mechanism.  
> Maybe a hardware bug, maybe a software bug.
> 
> While looking through the code, I didn't notice anything about enabling
> wakeups.  But I wasn't looking very carefully, because I don't know how
> the Tegra platform works.  A quick search through the patch now seems
> to show that wakeups never get enabled at all!

I looked at our downstream ehci-tegra.c, and there's a lot more code
there for suspend/resume, wakeup, and a bunch of other stuff. Given
that, I'm not too surprised that the upstream driver isn't resuming from
suspend as expected.

I think we can still apply your patch if you want, but we additionally
need to make CONFIG_USB_EHCI_TEGRA depend on !CONFIG_USB_SUSPEND, which
will ensure nobody gets surprised by this fails-to-resume issue. Does
that seem reasonable? I don't think this will have any user-impact,
since (a) the controllers weren't suspending before this patch, and (b)
neither CONFIG_USB_SUSPEND nor CONFIG_PM_RUNTIME were enabled in
tegra_defconfig (although in 3.5 we'll enable CONFIG_PM_RUNTIME).

Those responsible for ehci-tegra.c in our downstream kernels are
supposed to be working on a plan to upstream the missing parts. When
that comes to fruition, we should be able to remove that Kconfig
restriction. I'm afraid I don't have an ETA for when that will be though.

Re: Changes to power management in ehci-tegra

[Alan Stern]

On Wed, 18 Apr 2012, Stephen Warren wrote:

> > How well does the existing driver work if you enable the 
> > power_down_on_bus_suspend flag in the platform data?  
> 
> power_down_on_bus_suspend is always enabled for the Tegra EHCI controllers.
> 
> The existing driver works fine (with or without USB_SUSPEND=y and
> PM_RUNTIME=y), although doesn't ever runtime-suspended, so I guess this
> is just hiding the problem:
> 
> /sys/devices/tegra-ehci.2# cat {,usb3/}power/{control,runtime_status}
> auto
> unsupported
> auto
> active
> 
> (I see the same with or without anything plugged in)

That's not meaningful in this situation.  With the existing driver, the
controller will get powered down whenever the root hub suspends -- that
is, when tegra_ehci_bus_suspend() calls tegra_usb_suspend() which calls
tegra_ehci_power_down().  This should happen whenever no devices are
plugged in (assuming CONFIG_USB_SUSPEND is enabled).  This mechanism
for powering down the controller doesn't use runtime PM, but it will
operate anyway.

The questions are: If you unplug everything under the existing driver,
what does usb3/power/runtime_status show?  When you plug in a new
device, is it detected (presumably it is)?  And finally, how does the 
controller detect the new device while it is powered down?

> By the way, how broken is the current tegra_ehci_hub_control()? I see
> that it mostly duplicates ehci_hub_control() for a few cases, but has
> diverged a little since, e.g. the set_bit()/clear_bit() calls are using
> "wIndex - 1" instead of "wIndex" etc.

That's just a minor optimization.  ehci_hub_control() decrements wIndex
near the start of certain cases, to avoid all those "- 1"  things later
on.

I haven't tried to do a detailed check of all the code in
tegra_ehci_hub_control().

> Also, I wonder if the logic in the
> ClearPortFeature/USB_PORT_FEAT_ENABLE case might be worth moving into
> ehci-hub.c, if it's generally applicable? I did try removing that
> function and making the driver use ehci_hub_control() instead, but that
> didn't solve the resume issue.

Yes, that does look like a bug in ehci_hub_control().  Feel free to 
submit a patch moving the Tegra code over.

Alan Stern

Re: Changes to power management in ehci-tegra

[Alan Stern]

On Wed, 18 Apr 2012, Stephen Warren wrote:

> I looked at our downstream ehci-tegra.c, and there's a lot more code
> there for suspend/resume, wakeup, and a bunch of other stuff. Given
> that, I'm not too surprised that the upstream driver isn't resuming from
> suspend as expected.
> 
> I think we can still apply your patch if you want,

It's important.  Recent changes in usbcore will cause
ehci_hub_status_data() to be called after the root hub is suspended.  
(See commit 879d38e6bc36d73b0ac40ec9b0d839fda9fa8b1a in Greg's USB
tree.)  If the controller is already powered down, you'll run into
trouble.

Besides, ehci-tegra might as well use the runtime PM framework for 
managing the controller's power state.  That's what it's for.

>  but we additionally
> need to make CONFIG_USB_EHCI_TEGRA depend on !CONFIG_USB_SUSPEND, which
> will ensure nobody gets surprised by this fails-to-resume issue. Does
> that seem reasonable? I don't think this will have any user-impact,
> since (a) the controllers weren't suspending before this patch, and (b)
> neither CONFIG_USB_SUSPEND nor CONFIG_PM_RUNTIME were enabled in
> tegra_defconfig (although in 3.5 we'll enable CONFIG_PM_RUNTIME).

Are you sure the controllers aren't getting suspending under the 
current driver?  It seems like they ought to be -- otherwise there 
would be no point in defining power_down_on_bus_suspend.

> Those responsible for ehci-tegra.c in our downstream kernels are
> supposed to be working on a plan to upstream the missing parts. When
> that comes to fruition, we should be able to remove that Kconfig
> restriction. I'm afraid I don't have an ETA for when that will be though.

Is this patch compatible with the downstream driver?  It would be a 
shame to mess up a bunch of other work.

Alan Stern

Re: Changes to power management in ehci-tegra

[Stephen Warren]

On 04/18/2012 02:47 PM, Alan Stern wrote:
...
> Are you sure the controllers aren't getting suspending under the 
> current driver?  It seems like they ought to be -- otherwise there 
> would be no point in defining power_down_on_bus_suspend.

Ah. The ports are getting suspended (when nothing is plugged in), and
tegra_ehci_bus_suspend() is returning 0. However, as soon as any port is
suspended, it resumes immediately. I guess that's why plugging something
in works even after the controller is suspended; it keeps waking up and
is essentially polling for any plugged-in device.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Changes to power management in ehci-tegra

[Alex Courbot]

On 04/19/2012 05:47 AM, Alan Stern wrote:
>> Those responsible for ehci-tegra.c in our downstream kernels are
>> supposed to be working on a plan to upstream the missing parts. When
>> that comes to fruition, we should be able to remove that Kconfig
>> restriction. I'm afraid I don't have an ETA for when that will be though.
>
> Is this patch compatible with the downstream driver?  It would be a
> shame to mess up a bunch of other work.

Downstream driver is dramatically out-of-sync with respect to upstream, 
and we don't know when we will be able to catch up, so if your patch 
improves things there is no reason not to go ahead. AFAIK nothing 
equivalent to what you implemented has been done on our side (i.e. we 
are still powering down during bus suspend), so this is welcome.

Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Changes to power management in ehci-tegra

[Alan Stern]

On Wed, 18 Apr 2012, Stephen Warren wrote:

> On 04/18/2012 02:47 PM, Alan Stern wrote:
> ...
> > Are you sure the controllers aren't getting suspending under the 
> > current driver?  It seems like they ought to be -- otherwise there 
> > would be no point in defining power_down_on_bus_suspend.
> 
> Ah. The ports are getting suspended (when nothing is plugged in), and
> tegra_ehci_bus_suspend() is returning 0. However, as soon as any port is
> suspended, it resumes immediately. I guess that's why plugging something
> in works even after the controller is suspended; it keeps waking up and
> is essentially polling for any plugged-in device.

All right.  In that case there's no point in powering down the 
controller at all.

Here's what you can do.  In the patch, change the lines

+	if (!pdata->power_down_on_bus_suspend)
+		pm_runtime_forbid(&pdev->dev);

so that pm_runtime_forbid() gets called unconditionally.  For example:

+//	if (!pdata->power_down_on_bus_suspend)
+		pm_runtime_forbid(&pdev->dev);

If you make that change, does the driver continue to work as well as it 
did before?

Alan Stern

Re: Changes to power management in ehci-tegra

[Stephen Warren]

On 04/19/2012 07:52 AM, Alan Stern wrote:
> On Wed, 18 Apr 2012, Stephen Warren wrote:
> 
>> On 04/18/2012 02:47 PM, Alan Stern wrote:
>> ...
>>> Are you sure the controllers aren't getting suspending under the 
>>> current driver?  It seems like they ought to be -- otherwise there 
>>> would be no point in defining power_down_on_bus_suspend.
>>
>> Ah. The ports are getting suspended (when nothing is plugged in), and
>> tegra_ehci_bus_suspend() is returning 0. However, as soon as any port is
>> suspended, it resumes immediately. I guess that's why plugging something
>> in works even after the controller is suspended; it keeps waking up and
>> is essentially polling for any plugged-in device.
> 
> All right.  In that case there's no point in powering down the 
> controller at all.
> 
> Here's what you can do.  In the patch, change the lines
> 
> +	if (!pdata->power_down_on_bus_suspend)
> +		pm_runtime_forbid(&pdev->dev);
> 
> so that pm_runtime_forbid() gets called unconditionally.  For example:
> 
> +//	if (!pdata->power_down_on_bus_suspend)
> +		pm_runtime_forbid(&pdev->dev);
> 
> If you make that change, does the driver continue to work as well as it 
> did before?

Yes, I believe so.

Note: I found that the immediate resume is because hcd_bus_suspend()
aborts part-way through due to "suspend raced with wakeup event", which
is due to ehci_hub_status_data() finding that port_status has all of
PORT_CSC | PORT_PEC | PORT_OCC set. Is it worth tracking that down
further? I have no idea if it's an ehci-tegra.c issue or quirk in our
EHCI HW.

Re: Changes to power management in ehci-tegra

[Alan Stern]

On Thu, 19 Apr 2012, Stephen Warren wrote:

> > If you make that change, does the driver continue to work as well as it 
> > did before?
> 
> Yes, I believe so.
> 
> Note: I found that the immediate resume is because hcd_bus_suspend()
> aborts part-way through due to "suspend raced with wakeup event", which
> is due to ehci_hub_status_data() finding that port_status has all of
> PORT_CSC | PORT_PEC | PORT_OCC set. Is it worth tracking that down
> further? I have no idea if it's an ehci-tegra.c issue or quirk in our
> EHCI HW.

It's probably because of the recent change to hcd_bus_suspend().  
Before that change, it did not call ehci_hub_status_data() after doing
the suspend.  But now you see the problem: ehci_hub_status_data()  
returns bogus information because the controller has been powered down.  
That's the reason why I wanted to update ehci-tegra.c.

If you revert commit 879d38e6bc36d73b0ac40ec9b0d839fda9fa8b1a, how well
does the current driver behave?

Alan Stern

Re: Changes to power management in ehci-tegra

[Stephen Warren]

On 04/19/2012 01:01 PM, Alan Stern wrote:
> On Thu, 19 Apr 2012, Stephen Warren wrote:
> 
>>> If you make that change, does the driver continue to work as well as it 
>>> did before?
>>
>> Yes, I believe so.
>>
>> Note: I found that the immediate resume is because hcd_bus_suspend()
>> aborts part-way through due to "suspend raced with wakeup event", which
>> is due to ehci_hub_status_data() finding that port_status has all of
>> PORT_CSC | PORT_PEC | PORT_OCC set. Is it worth tracking that down
>> further? I have no idea if it's an ehci-tegra.c issue or quirk in our
>> EHCI HW.
> 
> It's probably because of the recent change to hcd_bus_suspend().  
> Before that change, it did not call ehci_hub_status_data() after doing
> the suspend.  But now you see the problem: ehci_hub_status_data()  
> returns bogus information because the controller has been powered down.  
> That's the reason why I wanted to update ehci-tegra.c.

Aha, I'm beginning to understand!

> If you revert commit 879d38e6bc36d73b0ac40ec9b0d839fda9fa8b1a, how well
> does the current driver behave?

It's basically the same as the patch you posted at the start of this
thread. In other words:

* When nothing is plugged in, the port(hub) suspends and stays
suspended; no oscillating.

* When I plug something in, the system doesn't see it.

* If I then manually resume the hub ("echo on > usb3/power/control"),
new devices are seen, and devices can be unplugged/replugged and seen.

* While a device is plugged in, I can "echo auto > usb3/power/control"
and it stays operational.

So, I guess the only question is: the patch you posted was to solve the
problem of seeing bogus status during suspend, which causes the
immediate resume; Why doesn't that work?

Re: Changes to power management in ehci-tegra

[Alan Stern]

On Thu, 19 Apr 2012, Stephen Warren wrote:

> >> Note: I found that the immediate resume is because hcd_bus_suspend()
> >> aborts part-way through due to "suspend raced with wakeup event", which
> >> is due to ehci_hub_status_data() finding that port_status has all of
> >> PORT_CSC | PORT_PEC | PORT_OCC set. Is it worth tracking that down
> >> further? I have no idea if it's an ehci-tegra.c issue or quirk in our
> >> EHCI HW.
> > 
> > It's probably because of the recent change to hcd_bus_suspend().  
> > Before that change, it did not call ehci_hub_status_data() after doing
> > the suspend.  But now you see the problem: ehci_hub_status_data()  
> > returns bogus information because the controller has been powered down.  
> > That's the reason why I wanted to update ehci-tegra.c.
> 
> Aha, I'm beginning to understand!
> 
> > If you revert commit 879d38e6bc36d73b0ac40ec9b0d839fda9fa8b1a, how well
> > does the current driver behave?
> 
> It's basically the same as the patch you posted at the start of this
> thread. In other words:
> 
> * When nothing is plugged in, the port(hub) suspends and stays
> suspended; no oscillating.
> 
> * When I plug something in, the system doesn't see it.
> 
> * If I then manually resume the hub ("echo on > usb3/power/control"),
> new devices are seen, and devices can be unplugged/replugged and seen.
> 
> * While a device is plugged in, I can "echo auto > usb3/power/control"
> and it stays operational.
> 
> So, I guess the only question is: the patch you posted was to solve the
> problem of seeing bogus status during suspend, which causes the
> immediate resume; Why doesn't that work?

According to what you said earlier, it _does_ work.  That is, you said
with my patch to ehci-tegra the controller does not resume immediately;  
it goes to low power and stays that way even when a new device is
plugged in.  Which means that wakeup is a problem, but then it's also a
problem with the existing driver.

Thus the combination of 879d38e6b plus my ehci-tegra patch leaves
the behavior basically unchanged.  Which is what I wanted to verify.

Unless you have an objection, I'll submit the patch (including the
change that calls pm_runtime_forbid() unconditionally).

Alan Stern

Re: Changes to power management in ehci-tegra

[Stephen Warren]

On 04/19/2012 01:45 PM, Alan Stern wrote:
> On Thu, 19 Apr 2012, Stephen Warren wrote:
> 
>>>> Note: I found that the immediate resume is because hcd_bus_suspend()
>>>> aborts part-way through due to "suspend raced with wakeup event", which
>>>> is due to ehci_hub_status_data() finding that port_status has all of
>>>> PORT_CSC | PORT_PEC | PORT_OCC set. Is it worth tracking that down
>>>> further? I have no idea if it's an ehci-tegra.c issue or quirk in our
>>>> EHCI HW.
>>>
>>> It's probably because of the recent change to hcd_bus_suspend().  
>>> Before that change, it did not call ehci_hub_status_data() after doing
>>> the suspend.  But now you see the problem: ehci_hub_status_data()  
>>> returns bogus information because the controller has been powered down.  
>>> That's the reason why I wanted to update ehci-tegra.c.
>>
>> Aha, I'm beginning to understand!
>>
>>> If you revert commit 879d38e6bc36d73b0ac40ec9b0d839fda9fa8b1a, how well
>>> does the current driver behave?
>>
>> It's basically the same as the patch you posted at the start of this
>> thread. In other words:
>>
>> * When nothing is plugged in, the port(hub) suspends and stays
>> suspended; no oscillating.
>>
>> * When I plug something in, the system doesn't see it.
>>
>> * If I then manually resume the hub ("echo on > usb3/power/control"),
>> new devices are seen, and devices can be unplugged/replugged and seen.
>>
>> * While a device is plugged in, I can "echo auto > usb3/power/control"
>> and it stays operational.
>>
>> So, I guess the only question is: the patch you posted was to solve the
>> problem of seeing bogus status during suspend, which causes the
>> immediate resume; Why doesn't that work?
> 
> According to what you said earlier, it _does_ work.  That is, you said
> with my patch to ehci-tegra the controller does not resume immediately;  
> it goes to low power and stays that way even when a new device is
> plugged in.  Which means that wakeup is a problem, but then it's also a
> problem with the existing driver.

OK, well I meant "work overall" i.e. suspend OK and also auto-resume
when something was plugged in, although I guess it looked like I was
talking about just suspending.

> Thus the combination of 879d38e6b plus my ehci-tegra patch leaves
> the behavior basically unchanged.  Which is what I wanted to verify.
> 
> Unless you have an objection, I'll submit the patch (including the
> change that calls pm_runtime_forbid() unconditionally).

I think that's fine. Thanks for working through this.