[PATCH 0/6] USB: Implement runtime idling and remote wakeup for OMAP EHCI controller

[PATCH 0/6] USB: Implement runtime idling and remote wakeup for OMAP EHCI controller

[Roger Quadros]

Hi,

This series implements suspend/resume for the OMAP EHCI host controller during
runtime idle. This will cause its parent, the OMAP USB Host Module as well
as the USB TLL Module to be put in suspend and hence allow the USB power domain
to be put in a lower power state. Then we no longer prevent the rest of the OMAP
SoC from entering lower power states like RETention or OFF mode when
USB (or system) is suspended. This was one of the main reason why EHCI_OMAP
is still not enabled in OMAP2 defconfig.

In order for remote wakeup or hub events (connect/disconnect) to be detected
while in suspend, we need to rely on the IO daisy chaining mechanism on OMAP.
This is nothing but configuring a pin to be wakeup capable and triggering an
interrupt on any pin activity while the hardware module or the entire SoC is
in sleep state. For this to work, we rely on the wakeup feature added to the
omap-pinctrl-single driver in [1]. This takes care of routing IO pad wakeup
interrupt to the right driver's interrupt handler (i.e. ehci_irq in our case).

The pin state information for DEFAULT and IDLE is specified for the omap3beagle-xm
board in patch 2. So this is tested only on omap3beagle-xm board.

Patch 4 takes care of switching the pin states between DEFAULT and IDLE during
idle/active mode using the pinctrl framework.

Patch 5 takes care of handling the wakeup IRQ when the USB controller is suspended
and Hardware is not accessible.

Patch 6 implements runtime and system suspend/resume for the OMAP EHCI controller

Changelog:
v1:
- addressed review comments on RFC patchset
- Special case handling of controllers that can generate wakeup IRQ when
suspended is done in HCD core instead of ehci-hcd.
- Patches based on linus/master commit 496322bc91e35007ed754184dcd447a02b6dd685
with the following patches on top

[1] - OMAP pinctrl wakeup support
http://thread.gmane.org/gmane.linux.ports.arm.omap/99010/focus=99041
This does not apply cleanly though. Tony has agreed to post a revision anytime soon.

[2] - [PATCH v5 0/2] ARM: dts: Add USB host support for Beagle-xm
http://thread.gmane.org/gmane.linux.drivers.devicetree/39291

[3] - USB-EHCI-Fix-resume-signalling-on-remote-wakeup.patch
http://thread.gmane.org/gmane.linux.usb.general/89608

cheers,
-roger

---
Roger Quadros (6):
  ARM: OMAP3: Enable Hardware Save and Restore for USB Host
  ARM: dts: omap3beagle-xm: Add idle state pins for USB host
  mfd: omap-usb-host: move initialization to module_init()
  mfd: omap-usb-host: Put pins in IDLE state on suspend
  USB: Support wakeup IRQ for suspended controllers
  USB: ehci-omap: Implement suspend/resume

 arch/arm/boot/dts/omap3-beagle-xm.dts       |   29 ++++++++++---
 arch/arm/mach-omap2/powerdomains3xxx_data.c |    8 +---
 drivers/mfd/omap-usb-host.c                 |   45 ++++++++++--------
 drivers/mfd/omap-usb-tll.c                  |   20 +--------
 drivers/usb/core/hcd.c                      |   21 ++++++++-
 drivers/usb/host/ehci-omap.c                |   64 ++++++++++++++++++++++++++-
 include/linux/usb/hcd.h                     |    3 +
 7 files changed, 136 insertions(+), 54 deletions(-)

-- 
1.7.4.1

[PATCH 0/6] USB: Implement runtime idling and remote wakeup for OMAP EHCI controller

[Roger Quadros]

Hi,

This series implements suspend/resume for the OMAP EHCI host controller during
runtime idle. This will cause its parent, the OMAP USB Host Module as well
as the USB TLL Module to be put in suspend and hence allow the USB power domain
to be put in a lower power state. Then we no longer prevent the rest of the OMAP
SoC from entering lower power states like RETention or OFF mode when
USB (or system) is suspended. This was one of the main reason why EHCI_OMAP
is still not enabled in OMAP2 defconfig.

In order for remote wakeup or hub events (connect/disconnect) to be detected
while in suspend, we need to rely on the IO daisy chaining mechanism on OMAP.
This is nothing but configuring a pin to be wakeup capable and triggering an
interrupt on any pin activity while the hardware module or the entire SoC is
in sleep state. For this to work, we rely on the wakeup feature added to the
omap-pinctrl-single driver in [1]. This takes care of routing IO pad wakeup
interrupt to the right driver's interrupt handler (i.e. ehci_irq in our case).

The pin state information for DEFAULT and IDLE is specified for the omap3beagle-xm
board in patch 2. So this is tested only on omap3beagle-xm board.

Patch 4 takes care of switching the pin states between DEFAULT and IDLE during
idle/active mode using the pinctrl framework.

Patch 5 takes care of handling the wakeup IRQ when the USB controller is suspended
and Hardware is not accessible.

Patch 6 implements runtime and system suspend/resume for the OMAP EHCI controller

Changelog:
v1:
- addressed review comments on RFC patchset
- Special case handling of controllers that can generate wakeup IRQ when
suspended is done in HCD core instead of ehci-hcd.
- Patches based on linus/master commit 496322bc91e35007ed754184dcd447a02b6dd685
with the following patches on top

[1] - OMAP pinctrl wakeup support
http://thread.gmane.org/gmane.linux.ports.arm.omap/99010/focus=99041
This does not apply cleanly though. Tony has agreed to post a revision anytime soon.

[2] - [PATCH v5 0/2] ARM: dts: Add USB host support for Beagle-xm
http://thread.gmane.org/gmane.linux.drivers.devicetree/39291

[3] - USB-EHCI-Fix-resume-signalling-on-remote-wakeup.patch
http://thread.gmane.org/gmane.linux.usb.general/89608

cheers,
-roger

---
Roger Quadros (6):
  ARM: OMAP3: Enable Hardware Save and Restore for USB Host
  ARM: dts: omap3beagle-xm: Add idle state pins for USB host
  mfd: omap-usb-host: move initialization to module_init()
  mfd: omap-usb-host: Put pins in IDLE state on suspend
  USB: Support wakeup IRQ for suspended controllers
  USB: ehci-omap: Implement suspend/resume

 arch/arm/boot/dts/omap3-beagle-xm.dts       |   29 ++++++++++---
 arch/arm/mach-omap2/powerdomains3xxx_data.c |    8 +---
 drivers/mfd/omap-usb-host.c                 |   45 ++++++++++--------
 drivers/mfd/omap-usb-tll.c                  |   20 +--------
 drivers/usb/core/hcd.c                      |   21 ++++++++-
 drivers/usb/host/ehci-omap.c                |   64 ++++++++++++++++++++++++++-
 include/linux/usb/hcd.h                     |    3 +
 7 files changed, 136 insertions(+), 54 deletions(-)

-- 
1.7.4.1

[PATCH 0/6] USB: Implement runtime idling and remote wakeup for OMAP EHCI controller

[Roger Quadros]

Hi,

This series implements suspend/resume for the OMAP EHCI host controller during
runtime idle. This will cause its parent, the OMAP USB Host Module as well
as the USB TLL Module to be put in suspend and hence allow the USB power domain
to be put in a lower power state. Then we no longer prevent the rest of the OMAP
SoC from entering lower power states like RETention or OFF mode when
USB (or system) is suspended. This was one of the main reason why EHCI_OMAP
is still not enabled in OMAP2 defconfig.

In order for remote wakeup or hub events (connect/disconnect) to be detected
while in suspend, we need to rely on the IO daisy chaining mechanism on OMAP.
This is nothing but configuring a pin to be wakeup capable and triggering an
interrupt on any pin activity while the hardware module or the entire SoC is
in sleep state. For this to work, we rely on the wakeup feature added to the
omap-pinctrl-single driver in [1]. This takes care of routing IO pad wakeup
interrupt to the right driver's interrupt handler (i.e. ehci_irq in our case).

The pin state information for DEFAULT and IDLE is specified for the omap3beagle-xm
board in patch 2. So this is tested only on omap3beagle-xm board.

Patch 4 takes care of switching the pin states between DEFAULT and IDLE during
idle/active mode using the pinctrl framework.

Patch 5 takes care of handling the wakeup IRQ when the USB controller is suspended
and Hardware is not accessible.

Patch 6 implements runtime and system suspend/resume for the OMAP EHCI controller

Changelog:
v1:
- addressed review comments on RFC patchset
- Special case handling of controllers that can generate wakeup IRQ when
suspended is done in HCD core instead of ehci-hcd.
- Patches based on linus/master commit 496322bc91e35007ed754184dcd447a02b6dd685
with the following patches on top

[1] - OMAP pinctrl wakeup support
http://thread.gmane.org/gmane.linux.ports.arm.omap/99010/focus=99041
This does not apply cleanly though. Tony has agreed to post a revision anytime soon.

[2] - [PATCH v5 0/2] ARM: dts: Add USB host support for Beagle-xm
http://thread.gmane.org/gmane.linux.drivers.devicetree/39291

[3] - USB-EHCI-Fix-resume-signalling-on-remote-wakeup.patch
http://thread.gmane.org/gmane.linux.usb.general/89608

cheers,
-roger

---
Roger Quadros (6):
  ARM: OMAP3: Enable Hardware Save and Restore for USB Host
  ARM: dts: omap3beagle-xm: Add idle state pins for USB host
  mfd: omap-usb-host: move initialization to module_init()
  mfd: omap-usb-host: Put pins in IDLE state on suspend
  USB: Support wakeup IRQ for suspended controllers
  USB: ehci-omap: Implement suspend/resume

 arch/arm/boot/dts/omap3-beagle-xm.dts       |   29 ++++++++++---
 arch/arm/mach-omap2/powerdomains3xxx_data.c |    8 +---
 drivers/mfd/omap-usb-host.c                 |   45 ++++++++++--------
 drivers/mfd/omap-usb-tll.c                  |   20 +--------
 drivers/usb/core/hcd.c                      |   21 ++++++++-
 drivers/usb/host/ehci-omap.c                |   64 ++++++++++++++++++++++++++-
 include/linux/usb/hcd.h                     |    3 +
 7 files changed, 136 insertions(+), 54 deletions(-)

-- 
1.7.4.1

[PATCH 1/6] ARM: OMAP3: Enable Hardware Save and Restore for USB Host

[Roger Quadros]

To ensure hardware context is restored while resuming from
OFF mode we need to enable the Hardware SAR bit for the
USB Host power domain.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 arch/arm/mach-omap2/powerdomains3xxx_data.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomains3xxx_data.c b/arch/arm/mach-omap2/powerdomains3xxx_data.c
index e2d4bd8..7b44d1f 100644
--- a/arch/arm/mach-omap2/powerdomains3xxx_data.c
+++ b/arch/arm/mach-omap2/powerdomains3xxx_data.c
@@ -289,13 +289,7 @@ static struct powerdomain usbhost_pwrdm = {
 	.prcm_offs	  = OMAP3430ES2_USBHOST_MOD,
 	.pwrsts		  = PWRSTS_OFF_RET_ON,
 	.pwrsts_logic_ret = PWRSTS_RET,
-	/*
-	 * REVISIT: Enabling usb host save and restore mechanism seems to
-	 * leave the usb host domain permanently in ACTIVE mode after
-	 * changing the usb host power domain state from OFF to active once.
-	 * Disabling for now.
-	 */
-	/*.flags	  = PWRDM_HAS_HDWR_SAR,*/ /* for USBHOST ctrlr only */
+	.flags		  = PWRDM_HAS_HDWR_SAR, /* for USBHOST ctrlr only */
 	.banks		  = 1,
 	.pwrsts_mem_ret	  = {
 		[0] = PWRSTS_RET, /* MEMRETSTATE */
-- 
1.7.4.1

[PATCH 1/6] ARM: OMAP3: Enable Hardware Save and Restore for USB Host

[Roger Quadros]

To ensure hardware context is restored while resuming from
OFF mode we need to enable the Hardware SAR bit for the
USB Host power domain.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 arch/arm/mach-omap2/powerdomains3xxx_data.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomains3xxx_data.c b/arch/arm/mach-omap2/powerdomains3xxx_data.c
index e2d4bd8..7b44d1f 100644
--- a/arch/arm/mach-omap2/powerdomains3xxx_data.c
+++ b/arch/arm/mach-omap2/powerdomains3xxx_data.c
@@ -289,13 +289,7 @@ static struct powerdomain usbhost_pwrdm = {
 	.prcm_offs	  = OMAP3430ES2_USBHOST_MOD,
 	.pwrsts		  = PWRSTS_OFF_RET_ON,
 	.pwrsts_logic_ret = PWRSTS_RET,
-	/*
-	 * REVISIT: Enabling usb host save and restore mechanism seems to
-	 * leave the usb host domain permanently in ACTIVE mode after
-	 * changing the usb host power domain state from OFF to active once.
-	 * Disabling for now.
-	 */
-	/*.flags	  = PWRDM_HAS_HDWR_SAR,*/ /* for USBHOST ctrlr only */
+	.flags		  = PWRDM_HAS_HDWR_SAR, /* for USBHOST ctrlr only */
 	.banks		  = 1,
 	.pwrsts_mem_ret	  = {
 		[0] = PWRSTS_RET, /* MEMRETSTATE */
-- 
1.7.4.1

[PATCH 1/6] ARM: OMAP3: Enable Hardware Save and Restore for USB Host

[Roger Quadros]

To ensure hardware context is restored while resuming from
OFF mode we need to enable the Hardware SAR bit for the
USB Host power domain.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 arch/arm/mach-omap2/powerdomains3xxx_data.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomains3xxx_data.c b/arch/arm/mach-omap2/powerdomains3xxx_data.c
index e2d4bd8..7b44d1f 100644
--- a/arch/arm/mach-omap2/powerdomains3xxx_data.c
+++ b/arch/arm/mach-omap2/powerdomains3xxx_data.c
@@ -289,13 +289,7 @@ static struct powerdomain usbhost_pwrdm = {
 	.prcm_offs	  = OMAP3430ES2_USBHOST_MOD,
 	.pwrsts		  = PWRSTS_OFF_RET_ON,
 	.pwrsts_logic_ret = PWRSTS_RET,
-	/*
-	 * REVISIT: Enabling usb host save and restore mechanism seems to
-	 * leave the usb host domain permanently in ACTIVE mode after
-	 * changing the usb host power domain state from OFF to active once.
-	 * Disabling for now.
-	 */
-	/*.flags	  = PWRDM_HAS_HDWR_SAR,*/ /* for USBHOST ctrlr only */
+	.flags		  = PWRDM_HAS_HDWR_SAR, /* for USBHOST ctrlr only */
 	.banks		  = 1,
 	.pwrsts_mem_ret	  = {
 		[0] = PWRSTS_RET, /* MEMRETSTATE */
-- 
1.7.4.1

[PATCH 2/6] ARM: dts: omap3beagle-xm: Add idle state pins for USB host

[Roger Quadros]

Add the Idle state pins for USB host and enable WAKEUP on
DIR, DAT0-3, so that the PHY can wakeup the OMAP SoC from
sleep on any USB activity (e.g. remote wakeup or connect/disconnect).

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 arch/arm/boot/dts/omap3-beagle-xm.dts |   29 +++++++++++++++++++++++------
 1 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/omap3-beagle-xm.dts b/arch/arm/boot/dts/omap3-beagle-xm.dts
index 371b1e2..91d19fa 100644
--- a/arch/arm/boot/dts/omap3-beagle-xm.dts
+++ b/arch/arm/boot/dts/omap3-beagle-xm.dts
@@ -113,11 +113,6 @@
 };
 
 &omap3_pmx_core {
-	pinctrl-names = "default";
-	pinctrl-0 = <
-			&hsusbb2_pins
-	>;
-
 	uart3_pins: pinmux_uart3_pins {
 		pinctrl-single,pins = <
 			0x16e (PIN_INPUT | PIN_OFF_WAKEUPENABLE | MUX_MODE0) /* uart3_rx_irrx.uart3_rx_irrx */
@@ -125,7 +120,7 @@
 		>;
 	};
 
-	hsusbb2_pins: pinmux_hsusbb2_pins {
+	hsusb2_pins: pinmux_hsusb2_pins {
 		pinctrl-single,pins = <
 			0x5c0 (PIN_OUTPUT | MUX_MODE3)		/* etk_d10.hsusb2_clk */
 			0x5c2 (PIN_OUTPUT | MUX_MODE3)		/* etk_d11.hsusb2_stp */
@@ -141,6 +136,25 @@
 			0x1ae (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_cs1.hsusb2_data3 */
 		>;
 	};
+
+	/* WAKEUP enabled on DIR, DAT0-3 */
+	hsusb2_idle_pins: pinmux_hsusb2_idle_pins {
+		pinctrl-single,pins = <
+			0x5c0 (PIN_OUTPUT | MUX_MODE3)		/* etk_d10.hsusb2_clk */
+			0x5c2 (PIN_OUTPUT | MUX_MODE3)		/* etk_d11.hsusb2_stp */
+			0x5c4 (PIN_INPUT_PULLDOWN | WAKEUP_EN | MUX_MODE3)	/* etk_d12.hsusb2_dir */
+			0x5c6 (PIN_INPUT_PULLDOWN | MUX_MODE3)			/* etk_d13.hsusb2_nxt */
+			0x5c8 (PIN_INPUT_PULLDOWN | WAKEUP_EN | MUX_MODE3)	/* etk_d14.hsusb2_data0 */
+			0x5cA (PIN_INPUT_PULLDOWN | WAKEUP_EN | MUX_MODE3)	/* etk_d15.hsusb2_data1 */
+			0x1a4 (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi1_cs3.hsusb2_data2 */
+			0x1a6 (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_clk.hsusb2_data7 */
+			0x1a8 (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_simo.hsusb2_data4 */
+			0x1aa (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_somi.hsusb2_data5 */
+			0x1ac (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_cs0.hsusb2_data6 */
+			0x1ae (PIN_INPUT_PULLDOWN | WAKEUP_EN | MUX_MODE3)	/* mcspi2_cs1.hsusb2_data3 */
+		>;
+		interrupts = <77>; /* route padconf wakeup to EHCI IRQ */
+	};
 };
 
 &i2c1 {
@@ -223,6 +237,9 @@
 };
 
 &usbhshost {
+	pinctrl-names = "default", "idle";
+	pinctrl-0 = <&hsusb2_pins>;
+	pinctrl-1 = <&hsusb2_idle_pins>;
 	port2-mode = "ehci-phy";
 };
 
-- 
1.7.4.1

[PATCH 2/6] ARM: dts: omap3beagle-xm: Add idle state pins for USB host

[Roger Quadros]

Add the Idle state pins for USB host and enable WAKEUP on
DIR, DAT0-3, so that the PHY can wakeup the OMAP SoC from
sleep on any USB activity (e.g. remote wakeup or connect/disconnect).

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 arch/arm/boot/dts/omap3-beagle-xm.dts |   29 +++++++++++++++++++++++------
 1 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/omap3-beagle-xm.dts b/arch/arm/boot/dts/omap3-beagle-xm.dts
index 371b1e2..91d19fa 100644
--- a/arch/arm/boot/dts/omap3-beagle-xm.dts
+++ b/arch/arm/boot/dts/omap3-beagle-xm.dts
@@ -113,11 +113,6 @@
 };
 
 &omap3_pmx_core {
-	pinctrl-names = "default";
-	pinctrl-0 = <
-			&hsusbb2_pins
-	>;
-
 	uart3_pins: pinmux_uart3_pins {
 		pinctrl-single,pins = <
 			0x16e (PIN_INPUT | PIN_OFF_WAKEUPENABLE | MUX_MODE0) /* uart3_rx_irrx.uart3_rx_irrx */
@@ -125,7 +120,7 @@
 		>;
 	};
 
-	hsusbb2_pins: pinmux_hsusbb2_pins {
+	hsusb2_pins: pinmux_hsusb2_pins {
 		pinctrl-single,pins = <
 			0x5c0 (PIN_OUTPUT | MUX_MODE3)		/* etk_d10.hsusb2_clk */
 			0x5c2 (PIN_OUTPUT | MUX_MODE3)		/* etk_d11.hsusb2_stp */
@@ -141,6 +136,25 @@
 			0x1ae (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_cs1.hsusb2_data3 */
 		>;
 	};
+
+	/* WAKEUP enabled on DIR, DAT0-3 */
+	hsusb2_idle_pins: pinmux_hsusb2_idle_pins {
+		pinctrl-single,pins = <
+			0x5c0 (PIN_OUTPUT | MUX_MODE3)		/* etk_d10.hsusb2_clk */
+			0x5c2 (PIN_OUTPUT | MUX_MODE3)		/* etk_d11.hsusb2_stp */
+			0x5c4 (PIN_INPUT_PULLDOWN | WAKEUP_EN | MUX_MODE3)	/* etk_d12.hsusb2_dir */
+			0x5c6 (PIN_INPUT_PULLDOWN | MUX_MODE3)			/* etk_d13.hsusb2_nxt */
+			0x5c8 (PIN_INPUT_PULLDOWN | WAKEUP_EN | MUX_MODE3)	/* etk_d14.hsusb2_data0 */
+			0x5cA (PIN_INPUT_PULLDOWN | WAKEUP_EN | MUX_MODE3)	/* etk_d15.hsusb2_data1 */
+			0x1a4 (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi1_cs3.hsusb2_data2 */
+			0x1a6 (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_clk.hsusb2_data7 */
+			0x1a8 (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_simo.hsusb2_data4 */
+			0x1aa (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_somi.hsusb2_data5 */
+			0x1ac (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_cs0.hsusb2_data6 */
+			0x1ae (PIN_INPUT_PULLDOWN | WAKEUP_EN | MUX_MODE3)	/* mcspi2_cs1.hsusb2_data3 */
+		>;
+		interrupts = <77>; /* route padconf wakeup to EHCI IRQ */
+	};
 };
 
 &i2c1 {
@@ -223,6 +237,9 @@
 };
 
 &usbhshost {
+	pinctrl-names = "default", "idle";
+	pinctrl-0 = <&hsusb2_pins>;
+	pinctrl-1 = <&hsusb2_idle_pins>;
 	port2-mode = "ehci-phy";
 };
 
-- 
1.7.4.1

[PATCH 2/6] ARM: dts: omap3beagle-xm: Add idle state pins for USB host

[Roger Quadros]

Add the Idle state pins for USB host and enable WAKEUP on
DIR, DAT0-3, so that the PHY can wakeup the OMAP SoC from
sleep on any USB activity (e.g. remote wakeup or connect/disconnect).

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 arch/arm/boot/dts/omap3-beagle-xm.dts |   29 +++++++++++++++++++++++------
 1 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/omap3-beagle-xm.dts b/arch/arm/boot/dts/omap3-beagle-xm.dts
index 371b1e2..91d19fa 100644
--- a/arch/arm/boot/dts/omap3-beagle-xm.dts
+++ b/arch/arm/boot/dts/omap3-beagle-xm.dts
@@ -113,11 +113,6 @@
 };
 
 &omap3_pmx_core {
-	pinctrl-names = "default";
-	pinctrl-0 = <
-			&hsusbb2_pins
-	>;
-
 	uart3_pins: pinmux_uart3_pins {
 		pinctrl-single,pins = <
 			0x16e (PIN_INPUT | PIN_OFF_WAKEUPENABLE | MUX_MODE0) /* uart3_rx_irrx.uart3_rx_irrx */
@@ -125,7 +120,7 @@
 		>;
 	};
 
-	hsusbb2_pins: pinmux_hsusbb2_pins {
+	hsusb2_pins: pinmux_hsusb2_pins {
 		pinctrl-single,pins = <
 			0x5c0 (PIN_OUTPUT | MUX_MODE3)		/* etk_d10.hsusb2_clk */
 			0x5c2 (PIN_OUTPUT | MUX_MODE3)		/* etk_d11.hsusb2_stp */
@@ -141,6 +136,25 @@
 			0x1ae (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_cs1.hsusb2_data3 */
 		>;
 	};
+
+	/* WAKEUP enabled on DIR, DAT0-3 */
+	hsusb2_idle_pins: pinmux_hsusb2_idle_pins {
+		pinctrl-single,pins = <
+			0x5c0 (PIN_OUTPUT | MUX_MODE3)		/* etk_d10.hsusb2_clk */
+			0x5c2 (PIN_OUTPUT | MUX_MODE3)		/* etk_d11.hsusb2_stp */
+			0x5c4 (PIN_INPUT_PULLDOWN | WAKEUP_EN | MUX_MODE3)	/* etk_d12.hsusb2_dir */
+			0x5c6 (PIN_INPUT_PULLDOWN | MUX_MODE3)			/* etk_d13.hsusb2_nxt */
+			0x5c8 (PIN_INPUT_PULLDOWN | WAKEUP_EN | MUX_MODE3)	/* etk_d14.hsusb2_data0 */
+			0x5cA (PIN_INPUT_PULLDOWN | WAKEUP_EN | MUX_MODE3)	/* etk_d15.hsusb2_data1 */
+			0x1a4 (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi1_cs3.hsusb2_data2 */
+			0x1a6 (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_clk.hsusb2_data7 */
+			0x1a8 (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_simo.hsusb2_data4 */
+			0x1aa (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_somi.hsusb2_data5 */
+			0x1ac (PIN_INPUT_PULLDOWN | MUX_MODE3)	/* mcspi2_cs0.hsusb2_data6 */
+			0x1ae (PIN_INPUT_PULLDOWN | WAKEUP_EN | MUX_MODE3)	/* mcspi2_cs1.hsusb2_data3 */
+		>;
+		interrupts = <77>; /* route padconf wakeup to EHCI IRQ */
+	};
 };
 
 &i2c1 {
@@ -223,6 +237,9 @@
 };
 
 &usbhshost {
+	pinctrl-names = "default", "idle";
+	pinctrl-0 = <&hsusb2_pins>;
+	pinctrl-1 = <&hsusb2_idle_pins>;
 	port2-mode = "ehci-phy";
 };
 
-- 
1.7.4.1

[PATCH 3/6] mfd: omap-usb-host: move initialization to module_init()

[Roger Quadros]

We no longer need to be initialized in any particular order
so move driver initialization to the standard place i.e. module_init()

CC: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/mfd/omap-usb-host.c |   23 +++--------------------
 drivers/mfd/omap-usb-tll.c  |   20 ++------------------
 2 files changed, 5 insertions(+), 38 deletions(-)

diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
index 759fae3..fb2b3d8 100644
--- a/drivers/mfd/omap-usb-host.c
+++ b/drivers/mfd/omap-usb-host.c
@@ -895,31 +895,14 @@ static struct platform_driver usbhs_omap_driver = {
 		.pm		= &usbhsomap_dev_pm_ops,
 		.of_match_table = of_match_ptr(usbhs_omap_dt_ids),
 	},
+	.probe		= usbhs_omap_probe,
 	.remove		= usbhs_omap_remove,
 };
 
+module_platform_driver(usbhs_omap_driver);
+
 MODULE_AUTHOR("Keshava Munegowda <keshava_mgowda@ti.com>");
 MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>");
 MODULE_ALIAS("platform:" USBHS_DRIVER_NAME);
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("usb host common core driver for omap EHCI and OHCI");
-
-static int __init omap_usbhs_drvinit(void)
-{
-	return platform_driver_probe(&usbhs_omap_driver, usbhs_omap_probe);
-}
-
-/*
- * init before ehci and ohci drivers;
- * The usbhs core driver should be initialized much before
- * the omap ehci and ohci probe functions are called.
- * This usbhs core driver should be initialized after
- * usb tll driver
- */
-fs_initcall_sync(omap_usbhs_drvinit);
-
-static void __exit omap_usbhs_drvexit(void)
-{
-	platform_driver_unregister(&usbhs_omap_driver);
-}
-module_exit(omap_usbhs_drvexit);
diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c
index e59ac4c..1a3a26f 100644
--- a/drivers/mfd/omap-usb-tll.c
+++ b/drivers/mfd/omap-usb-tll.c
@@ -326,6 +326,8 @@ static struct platform_driver usbtll_omap_driver = {
 	.remove		= usbtll_omap_remove,
 };
 
+module_platform_driver(usbtll_omap_driver);
+
 int omap_tll_init(struct usbhs_omap_platform_data *pdata)
 {
 	int i;
@@ -477,21 +479,3 @@ MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>");
 MODULE_ALIAS("platform:" USBHS_DRIVER_NAME);
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("usb tll driver for TI OMAP EHCI and OHCI controllers");
-
-static int __init omap_usbtll_drvinit(void)
-{
-	return platform_driver_register(&usbtll_omap_driver);
-}
-
-/*
- * init before usbhs core driver;
- * The usbtll driver should be initialized before
- * the usbhs core driver probe function is called.
- */
-fs_initcall(omap_usbtll_drvinit);
-
-static void __exit omap_usbtll_drvexit(void)
-{
-	platform_driver_unregister(&usbtll_omap_driver);
-}
-module_exit(omap_usbtll_drvexit);
-- 
1.7.4.1

[PATCH 3/6] mfd: omap-usb-host: move initialization to module_init()

[Roger Quadros]

We no longer need to be initialized in any particular order
so move driver initialization to the standard place i.e. module_init()

CC: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/mfd/omap-usb-host.c |   23 +++--------------------
 drivers/mfd/omap-usb-tll.c  |   20 ++------------------
 2 files changed, 5 insertions(+), 38 deletions(-)

diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
index 759fae3..fb2b3d8 100644
--- a/drivers/mfd/omap-usb-host.c
+++ b/drivers/mfd/omap-usb-host.c
@@ -895,31 +895,14 @@ static struct platform_driver usbhs_omap_driver = {
 		.pm		= &usbhsomap_dev_pm_ops,
 		.of_match_table = of_match_ptr(usbhs_omap_dt_ids),
 	},
+	.probe		= usbhs_omap_probe,
 	.remove		= usbhs_omap_remove,
 };
 
+module_platform_driver(usbhs_omap_driver);
+
 MODULE_AUTHOR("Keshava Munegowda <keshava_mgowda@ti.com>");
 MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>");
 MODULE_ALIAS("platform:" USBHS_DRIVER_NAME);
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("usb host common core driver for omap EHCI and OHCI");
-
-static int __init omap_usbhs_drvinit(void)
-{
-	return platform_driver_probe(&usbhs_omap_driver, usbhs_omap_probe);
-}
-
-/*
- * init before ehci and ohci drivers;
- * The usbhs core driver should be initialized much before
- * the omap ehci and ohci probe functions are called.
- * This usbhs core driver should be initialized after
- * usb tll driver
- */
-fs_initcall_sync(omap_usbhs_drvinit);
-
-static void __exit omap_usbhs_drvexit(void)
-{
-	platform_driver_unregister(&usbhs_omap_driver);
-}
-module_exit(omap_usbhs_drvexit);
diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c
index e59ac4c..1a3a26f 100644
--- a/drivers/mfd/omap-usb-tll.c
+++ b/drivers/mfd/omap-usb-tll.c
@@ -326,6 +326,8 @@ static struct platform_driver usbtll_omap_driver = {
 	.remove		= usbtll_omap_remove,
 };
 
+module_platform_driver(usbtll_omap_driver);
+
 int omap_tll_init(struct usbhs_omap_platform_data *pdata)
 {
 	int i;
@@ -477,21 +479,3 @@ MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>");
 MODULE_ALIAS("platform:" USBHS_DRIVER_NAME);
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("usb tll driver for TI OMAP EHCI and OHCI controllers");
-
-static int __init omap_usbtll_drvinit(void)
-{
-	return platform_driver_register(&usbtll_omap_driver);
-}
-
-/*
- * init before usbhs core driver;
- * The usbtll driver should be initialized before
- * the usbhs core driver probe function is called.
- */
-fs_initcall(omap_usbtll_drvinit);
-
-static void __exit omap_usbtll_drvexit(void)
-{
-	platform_driver_unregister(&usbtll_omap_driver);
-}
-module_exit(omap_usbtll_drvexit);
-- 
1.7.4.1

[PATCH 3/6] mfd: omap-usb-host: move initialization to module_init()

[Roger Quadros]

We no longer need to be initialized in any particular order
so move driver initialization to the standard place i.e. module_init()

CC: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/mfd/omap-usb-host.c |   23 +++--------------------
 drivers/mfd/omap-usb-tll.c  |   20 ++------------------
 2 files changed, 5 insertions(+), 38 deletions(-)

diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
index 759fae3..fb2b3d8 100644
--- a/drivers/mfd/omap-usb-host.c
+++ b/drivers/mfd/omap-usb-host.c
@@ -895,31 +895,14 @@ static struct platform_driver usbhs_omap_driver = {
 		.pm		= &usbhsomap_dev_pm_ops,
 		.of_match_table = of_match_ptr(usbhs_omap_dt_ids),
 	},
+	.probe		= usbhs_omap_probe,
 	.remove		= usbhs_omap_remove,
 };
 
+module_platform_driver(usbhs_omap_driver);
+
 MODULE_AUTHOR("Keshava Munegowda <keshava_mgowda@ti.com>");
 MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>");
 MODULE_ALIAS("platform:" USBHS_DRIVER_NAME);
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("usb host common core driver for omap EHCI and OHCI");
-
-static int __init omap_usbhs_drvinit(void)
-{
-	return platform_driver_probe(&usbhs_omap_driver, usbhs_omap_probe);
-}
-
-/*
- * init before ehci and ohci drivers;
- * The usbhs core driver should be initialized much before
- * the omap ehci and ohci probe functions are called.
- * This usbhs core driver should be initialized after
- * usb tll driver
- */
-fs_initcall_sync(omap_usbhs_drvinit);
-
-static void __exit omap_usbhs_drvexit(void)
-{
-	platform_driver_unregister(&usbhs_omap_driver);
-}
-module_exit(omap_usbhs_drvexit);
diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c
index e59ac4c..1a3a26f 100644
--- a/drivers/mfd/omap-usb-tll.c
+++ b/drivers/mfd/omap-usb-tll.c
@@ -326,6 +326,8 @@ static struct platform_driver usbtll_omap_driver = {
 	.remove		= usbtll_omap_remove,
 };
 
+module_platform_driver(usbtll_omap_driver);
+
 int omap_tll_init(struct usbhs_omap_platform_data *pdata)
 {
 	int i;
@@ -477,21 +479,3 @@ MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>");
 MODULE_ALIAS("platform:" USBHS_DRIVER_NAME);
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("usb tll driver for TI OMAP EHCI and OHCI controllers");
-
-static int __init omap_usbtll_drvinit(void)
-{
-	return platform_driver_register(&usbtll_omap_driver);
-}
-
-/*
- * init before usbhs core driver;
- * The usbtll driver should be initialized before
- * the usbhs core driver probe function is called.
- */
-fs_initcall(omap_usbtll_drvinit);
-
-static void __exit omap_usbtll_drvexit(void)
-{
-	platform_driver_unregister(&usbtll_omap_driver);
-}
-module_exit(omap_usbtll_drvexit);
-- 
1.7.4.1

[PATCH 4/6] mfd: omap-usb-host: Put pins in IDLE state on suspend

[Roger Quadros]

In order to support wake up from suspend use the pinctrl
framework to put the USB host pins in IDLE state during suspend.

CC: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/mfd/omap-usb-host.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
index fb2b3d8..3734d16 100644
--- a/drivers/mfd/omap-usb-host.c
+++ b/drivers/mfd/omap-usb-host.c
@@ -31,6 +31,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/err.h>
+#include <linux/pinctrl/consumer.h>
 
 #include "omap-usb.h"
 
@@ -111,6 +112,7 @@ struct usbhs_hcd_omap {
 	struct usbhs_omap_platform_data	*pdata;
 
 	u32				usbhs_rev;
+	bool				no_idle;
 };
 /*-------------------------------------------------------------------------*/
 
@@ -325,6 +327,7 @@ static int usbhs_runtime_resume(struct device *dev)
 
 	dev_dbg(dev, "usbhs_runtime_resume\n");
 
+	pinctrl_pm_select_default_state(dev);
 	omap_tll_enable(pdata);
 
 	if (!IS_ERR(omap->ehci_logic_fck))
@@ -378,6 +381,12 @@ static int usbhs_runtime_suspend(struct device *dev)
 
 	dev_dbg(dev, "usbhs_runtime_suspend\n");
 
+	if (omap->no_idle) {
+		dev_dbg(dev, "%s: Not suspending as IDLE pins not available\n",
+					__func__);
+		return -EBUSY;
+	}
+
 	for (i = 0; i < omap->nports; i++) {
 		switch (pdata->port_mode[i]) {
 		case OMAP_EHCI_PORT_MODE_HSIC:
@@ -401,6 +410,7 @@ static int usbhs_runtime_suspend(struct device *dev)
 		clk_disable(omap->ehci_logic_fck);
 
 	omap_tll_disable(pdata);
+	pinctrl_pm_select_idle_state(dev);
 
 	return 0;
 }
@@ -608,6 +618,14 @@ static int usbhs_omap_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	if (!dev->pins || !dev->pins->idle_state) {
+		/* If IDLE pins are not available, we can't remote wakeup,
+		 * so prevent idling in that case.
+		 */
+		omap->no_idle = true;
+		dev_info(dev, "No IDLE pins so runtime idle disabled\n");
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	omap->uhh_base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(omap->uhh_base))
@@ -796,6 +814,8 @@ static int usbhs_omap_probe(struct platform_device *pdev)
 		}
 	}
 
+	pinctrl_pm_select_default_state(dev);
+
 	return 0;
 
 err_alloc:
@@ -872,6 +892,8 @@ static int usbhs_omap_remove(struct platform_device *pdev)
 
 	/* remove children */
 	device_for_each_child(&pdev->dev, NULL, usbhs_omap_remove_child);
+	pinctrl_pm_select_idle_state(&pdev->dev);
+
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH 4/6] mfd: omap-usb-host: Put pins in IDLE state on suspend

[Roger Quadros]

In order to support wake up from suspend use the pinctrl
framework to put the USB host pins in IDLE state during suspend.

CC: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/mfd/omap-usb-host.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
index fb2b3d8..3734d16 100644
--- a/drivers/mfd/omap-usb-host.c
+++ b/drivers/mfd/omap-usb-host.c
@@ -31,6 +31,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/err.h>
+#include <linux/pinctrl/consumer.h>
 
 #include "omap-usb.h"
 
@@ -111,6 +112,7 @@ struct usbhs_hcd_omap {
 	struct usbhs_omap_platform_data	*pdata;
 
 	u32				usbhs_rev;
+	bool				no_idle;
 };
 /*-------------------------------------------------------------------------*/
 
@@ -325,6 +327,7 @@ static int usbhs_runtime_resume(struct device *dev)
 
 	dev_dbg(dev, "usbhs_runtime_resume\n");
 
+	pinctrl_pm_select_default_state(dev);
 	omap_tll_enable(pdata);
 
 	if (!IS_ERR(omap->ehci_logic_fck))
@@ -378,6 +381,12 @@ static int usbhs_runtime_suspend(struct device *dev)
 
 	dev_dbg(dev, "usbhs_runtime_suspend\n");
 
+	if (omap->no_idle) {
+		dev_dbg(dev, "%s: Not suspending as IDLE pins not available\n",
+					__func__);
+		return -EBUSY;
+	}
+
 	for (i = 0; i < omap->nports; i++) {
 		switch (pdata->port_mode[i]) {
 		case OMAP_EHCI_PORT_MODE_HSIC:
@@ -401,6 +410,7 @@ static int usbhs_runtime_suspend(struct device *dev)
 		clk_disable(omap->ehci_logic_fck);
 
 	omap_tll_disable(pdata);
+	pinctrl_pm_select_idle_state(dev);
 
 	return 0;
 }
@@ -608,6 +618,14 @@ static int usbhs_omap_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	if (!dev->pins || !dev->pins->idle_state) {
+		/* If IDLE pins are not available, we can't remote wakeup,
+		 * so prevent idling in that case.
+		 */
+		omap->no_idle = true;
+		dev_info(dev, "No IDLE pins so runtime idle disabled\n");
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	omap->uhh_base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(omap->uhh_base))
@@ -796,6 +814,8 @@ static int usbhs_omap_probe(struct platform_device *pdev)
 		}
 	}
 
+	pinctrl_pm_select_default_state(dev);
+
 	return 0;
 
 err_alloc:
@@ -872,6 +892,8 @@ static int usbhs_omap_remove(struct platform_device *pdev)
 
 	/* remove children */
 	device_for_each_child(&pdev->dev, NULL, usbhs_omap_remove_child);
+	pinctrl_pm_select_idle_state(&pdev->dev);
+
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH 4/6] mfd: omap-usb-host: Put pins in IDLE state on suspend

[Roger Quadros]

In order to support wake up from suspend use the pinctrl
framework to put the USB host pins in IDLE state during suspend.

CC: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/mfd/omap-usb-host.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
index fb2b3d8..3734d16 100644
--- a/drivers/mfd/omap-usb-host.c
+++ b/drivers/mfd/omap-usb-host.c
@@ -31,6 +31,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/err.h>
+#include <linux/pinctrl/consumer.h>
 
 #include "omap-usb.h"
 
@@ -111,6 +112,7 @@ struct usbhs_hcd_omap {
 	struct usbhs_omap_platform_data	*pdata;
 
 	u32				usbhs_rev;
+	bool				no_idle;
 };
 /*-------------------------------------------------------------------------*/
 
@@ -325,6 +327,7 @@ static int usbhs_runtime_resume(struct device *dev)
 
 	dev_dbg(dev, "usbhs_runtime_resume\n");
 
+	pinctrl_pm_select_default_state(dev);
 	omap_tll_enable(pdata);
 
 	if (!IS_ERR(omap->ehci_logic_fck))
@@ -378,6 +381,12 @@ static int usbhs_runtime_suspend(struct device *dev)
 
 	dev_dbg(dev, "usbhs_runtime_suspend\n");
 
+	if (omap->no_idle) {
+		dev_dbg(dev, "%s: Not suspending as IDLE pins not available\n",
+					__func__);
+		return -EBUSY;
+	}
+
 	for (i = 0; i < omap->nports; i++) {
 		switch (pdata->port_mode[i]) {
 		case OMAP_EHCI_PORT_MODE_HSIC:
@@ -401,6 +410,7 @@ static int usbhs_runtime_suspend(struct device *dev)
 		clk_disable(omap->ehci_logic_fck);
 
 	omap_tll_disable(pdata);
+	pinctrl_pm_select_idle_state(dev);
 
 	return 0;
 }
@@ -608,6 +618,14 @@ static int usbhs_omap_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	if (!dev->pins || !dev->pins->idle_state) {
+		/* If IDLE pins are not available, we can't remote wakeup,
+		 * so prevent idling in that case.
+		 */
+		omap->no_idle = true;
+		dev_info(dev, "No IDLE pins so runtime idle disabled\n");
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	omap->uhh_base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(omap->uhh_base))
@@ -796,6 +814,8 @@ static int usbhs_omap_probe(struct platform_device *pdev)
 		}
 	}
 
+	pinctrl_pm_select_default_state(dev);
+
 	return 0;
 
 err_alloc:
@@ -872,6 +892,8 @@ static int usbhs_omap_remove(struct platform_device *pdev)
 
 	/* remove children */
 	device_for_each_child(&pdev->dev, NULL, usbhs_omap_remove_child);
+	pinctrl_pm_select_idle_state(&pdev->dev);
+
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Roger Quadros]

Some platforms e.g. ehci-omap can generate an interrupt
(i.e. remote wakeup) even when the controller is suspended i.e.
HW_ACCESSIBLE is cleared.

Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
such cases.

We tackle this case by disabling the IRQ, scheduling a
hub resume and enabling back the IRQ after the controller has
resumed. This ensures that the IRQ handler runs only after the
controller is accessible.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/core/hcd.c  |   21 ++++++++++++++++++++-
 include/linux/usb/hcd.h |    3 +++
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 014dc99..933d8f1 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2161,6 +2161,11 @@ static void hcd_resume_work(struct work_struct *work)
 	usb_lock_device(udev);
 	usb_remote_wakeup(udev);
 	usb_unlock_device(udev);
+	if (HCD_IRQ_DISABLED(hcd)) {
+		/* We can now process IRQs so enable IRQ */
+		clear_bit(HCD_FLAG_IRQ_DISABLED, &hcd->flags);
+		enable_irq(hcd->irq);
+	}
 }
 
 /**
@@ -2248,7 +2253,21 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
 	 */
 	local_irq_save(flags);
 
-	if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
+	if (unlikely(HCD_DEAD(hcd)))
+		rc = IRQ_NONE;
+	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd) && hcd->has_wakeup_irq)) {
+		/*
+		 * We got a wakeup interrupt while the controller was
+		 * suspending or suspended.  We can't handle it now, so
+		 * disable the IRQ and resume the root hub (and hence
+		 * the controller too).
+		 */
+		disable_irq_nosync(hcd->irq);
+		set_bit(HCD_FLAG_IRQ_DISABLED, &hcd->flags);
+		usb_hcd_resume_root_hub(hcd);
+
+		rc = IRQ_HANDLED;
+	} else if (unlikely(!HCD_HW_ACCESSIBLE(hcd)))
 		rc = IRQ_NONE;
 	else if (hcd->driver->irq(hcd) == IRQ_NONE)
 		rc = IRQ_NONE;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 1e88377..5eb9f85 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -110,6 +110,7 @@ struct usb_hcd {
 #define HCD_FLAG_WAKEUP_PENDING		4	/* root hub is resuming? */
 #define HCD_FLAG_RH_RUNNING		5	/* root hub is running? */
 #define HCD_FLAG_DEAD			6	/* controller has died? */
+#define HCD_FLAG_IRQ_DISABLED		7	/* Interrupt was disabled */
 
 	/* The flags can be tested using these macros; they are likely to
 	 * be slightly faster than test_bit().
@@ -120,6 +121,7 @@ struct usb_hcd {
 #define HCD_WAKEUP_PENDING(hcd)	((hcd)->flags & (1U << HCD_FLAG_WAKEUP_PENDING))
 #define HCD_RH_RUNNING(hcd)	((hcd)->flags & (1U << HCD_FLAG_RH_RUNNING))
 #define HCD_DEAD(hcd)		((hcd)->flags & (1U << HCD_FLAG_DEAD))
+#define HCD_IRQ_DISABLED(hcd)	((hcd)->flags & (1U << HCD_FLAG_IRQ_DISABLED))
 
 	/* Flags that get set only during HCD registration or removal. */
 	unsigned		rh_registered:1;/* is root hub registered? */
@@ -132,6 +134,7 @@ struct usb_hcd {
 	unsigned		wireless:1;	/* Wireless USB HCD */
 	unsigned		authorized_default:1;
 	unsigned		has_tt:1;	/* Integrated TT in root hub */
+	unsigned		has_wakeup_irq:1; /* Can IRQ when suspended */
 
 	unsigned int		irq;		/* irq allocated */
 	void __iomem		*regs;		/* device memory/io */
-- 
1.7.4.1

[PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Roger Quadros]

Some platforms e.g. ehci-omap can generate an interrupt
(i.e. remote wakeup) even when the controller is suspended i.e.
HW_ACCESSIBLE is cleared.

Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
such cases.

We tackle this case by disabling the IRQ, scheduling a
hub resume and enabling back the IRQ after the controller has
resumed. This ensures that the IRQ handler runs only after the
controller is accessible.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/core/hcd.c  |   21 ++++++++++++++++++++-
 include/linux/usb/hcd.h |    3 +++
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 014dc99..933d8f1 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2161,6 +2161,11 @@ static void hcd_resume_work(struct work_struct *work)
 	usb_lock_device(udev);
 	usb_remote_wakeup(udev);
 	usb_unlock_device(udev);
+	if (HCD_IRQ_DISABLED(hcd)) {
+		/* We can now process IRQs so enable IRQ */
+		clear_bit(HCD_FLAG_IRQ_DISABLED, &hcd->flags);
+		enable_irq(hcd->irq);
+	}
 }
 
 /**
@@ -2248,7 +2253,21 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
 	 */
 	local_irq_save(flags);
 
-	if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
+	if (unlikely(HCD_DEAD(hcd)))
+		rc = IRQ_NONE;
+	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd) && hcd->has_wakeup_irq)) {
+		/*
+		 * We got a wakeup interrupt while the controller was
+		 * suspending or suspended.  We can't handle it now, so
+		 * disable the IRQ and resume the root hub (and hence
+		 * the controller too).
+		 */
+		disable_irq_nosync(hcd->irq);
+		set_bit(HCD_FLAG_IRQ_DISABLED, &hcd->flags);
+		usb_hcd_resume_root_hub(hcd);
+
+		rc = IRQ_HANDLED;
+	} else if (unlikely(!HCD_HW_ACCESSIBLE(hcd)))
 		rc = IRQ_NONE;
 	else if (hcd->driver->irq(hcd) == IRQ_NONE)
 		rc = IRQ_NONE;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 1e88377..5eb9f85 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -110,6 +110,7 @@ struct usb_hcd {
 #define HCD_FLAG_WAKEUP_PENDING		4	/* root hub is resuming? */
 #define HCD_FLAG_RH_RUNNING		5	/* root hub is running? */
 #define HCD_FLAG_DEAD			6	/* controller has died? */
+#define HCD_FLAG_IRQ_DISABLED		7	/* Interrupt was disabled */
 
 	/* The flags can be tested using these macros; they are likely to
 	 * be slightly faster than test_bit().
@@ -120,6 +121,7 @@ struct usb_hcd {
 #define HCD_WAKEUP_PENDING(hcd)	((hcd)->flags & (1U << HCD_FLAG_WAKEUP_PENDING))
 #define HCD_RH_RUNNING(hcd)	((hcd)->flags & (1U << HCD_FLAG_RH_RUNNING))
 #define HCD_DEAD(hcd)		((hcd)->flags & (1U << HCD_FLAG_DEAD))
+#define HCD_IRQ_DISABLED(hcd)	((hcd)->flags & (1U << HCD_FLAG_IRQ_DISABLED))
 
 	/* Flags that get set only during HCD registration or removal. */
 	unsigned		rh_registered:1;/* is root hub registered? */
@@ -132,6 +134,7 @@ struct usb_hcd {
 	unsigned		wireless:1;	/* Wireless USB HCD */
 	unsigned		authorized_default:1;
 	unsigned		has_tt:1;	/* Integrated TT in root hub */
+	unsigned		has_wakeup_irq:1; /* Can IRQ when suspended */
 
 	unsigned int		irq;		/* irq allocated */
 	void __iomem		*regs;		/* device memory/io */
-- 
1.7.4.1

[PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Roger Quadros]

Some platforms e.g. ehci-omap can generate an interrupt
(i.e. remote wakeup) even when the controller is suspended i.e.
HW_ACCESSIBLE is cleared.

Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
such cases.

We tackle this case by disabling the IRQ, scheduling a
hub resume and enabling back the IRQ after the controller has
resumed. This ensures that the IRQ handler runs only after the
controller is accessible.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/core/hcd.c  |   21 ++++++++++++++++++++-
 include/linux/usb/hcd.h |    3 +++
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 014dc99..933d8f1 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2161,6 +2161,11 @@ static void hcd_resume_work(struct work_struct *work)
 	usb_lock_device(udev);
 	usb_remote_wakeup(udev);
 	usb_unlock_device(udev);
+	if (HCD_IRQ_DISABLED(hcd)) {
+		/* We can now process IRQs so enable IRQ */
+		clear_bit(HCD_FLAG_IRQ_DISABLED, &hcd->flags);
+		enable_irq(hcd->irq);
+	}
 }
 
 /**
@@ -2248,7 +2253,21 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
 	 */
 	local_irq_save(flags);
 
-	if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
+	if (unlikely(HCD_DEAD(hcd)))
+		rc = IRQ_NONE;
+	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd) && hcd->has_wakeup_irq)) {
+		/*
+		 * We got a wakeup interrupt while the controller was
+		 * suspending or suspended.  We can't handle it now, so
+		 * disable the IRQ and resume the root hub (and hence
+		 * the controller too).
+		 */
+		disable_irq_nosync(hcd->irq);
+		set_bit(HCD_FLAG_IRQ_DISABLED, &hcd->flags);
+		usb_hcd_resume_root_hub(hcd);
+
+		rc = IRQ_HANDLED;
+	} else if (unlikely(!HCD_HW_ACCESSIBLE(hcd)))
 		rc = IRQ_NONE;
 	else if (hcd->driver->irq(hcd) == IRQ_NONE)
 		rc = IRQ_NONE;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 1e88377..5eb9f85 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -110,6 +110,7 @@ struct usb_hcd {
 #define HCD_FLAG_WAKEUP_PENDING		4	/* root hub is resuming? */
 #define HCD_FLAG_RH_RUNNING		5	/* root hub is running? */
 #define HCD_FLAG_DEAD			6	/* controller has died? */
+#define HCD_FLAG_IRQ_DISABLED		7	/* Interrupt was disabled */
 
 	/* The flags can be tested using these macros; they are likely to
 	 * be slightly faster than test_bit().
@@ -120,6 +121,7 @@ struct usb_hcd {
 #define HCD_WAKEUP_PENDING(hcd)	((hcd)->flags & (1U << HCD_FLAG_WAKEUP_PENDING))
 #define HCD_RH_RUNNING(hcd)	((hcd)->flags & (1U << HCD_FLAG_RH_RUNNING))
 #define HCD_DEAD(hcd)		((hcd)->flags & (1U << HCD_FLAG_DEAD))
+#define HCD_IRQ_DISABLED(hcd)	((hcd)->flags & (1U << HCD_FLAG_IRQ_DISABLED))
 
 	/* Flags that get set only during HCD registration or removal. */
 	unsigned		rh_registered:1;/* is root hub registered? */
@@ -132,6 +134,7 @@ struct usb_hcd {
 	unsigned		wireless:1;	/* Wireless USB HCD */
 	unsigned		authorized_default:1;
 	unsigned		has_tt:1;	/* Integrated TT in root hub */
+	unsigned		has_wakeup_irq:1; /* Can IRQ when suspended */
 
 	unsigned int		irq;		/* irq allocated */
 	void __iomem		*regs;		/* device memory/io */
-- 
1.7.4.1

[PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros]

Call ehci_suspend/resume() during runtime suspend/resume
as well as system suspend/resume.

Use a flag "bound" to indicate that the HCD structures are valid.
This is only true between usb_add_hcd() and usb_remove_hcd() calls.

The flag can be used by omap_ehci_runtime_suspend/resume() handlers
to avoid calling ehci_suspend/resume() when we are no longer bound
to the HCD e.g. during probe() and remove();

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/host/ehci-omap.c |   64 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 9bd7dfe..6d1fde97 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -69,6 +69,7 @@ static const char hcd_name[] = "ehci-omap";
 struct omap_hcd {
 	struct usb_phy *phy[OMAP3_HS_USB_PORTS]; /* one PHY for each port */
 	int nports;
+	bool bound;	/* HCD structures are valid */
 };
 
 static inline void ehci_write(void __iomem *base, u32 reg, u32 val)
@@ -159,6 +160,7 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 	hcd->rsrc_start = res->start;
 	hcd->rsrc_len = resource_size(res);
 	hcd->regs = regs;
+	hcd->has_wakeup_irq = true;
 	hcd_to_ehci(hcd)->caps = regs;
 
 	omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
@@ -216,6 +218,8 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 		goto err_pm_runtime;
 	}
 
+	omap->bound = true;
+
 	/*
 	 * Bring PHYs out of reset for non PHY modes.
 	 * Even though HSIC mode is a PHY-less mode, the reset
@@ -232,6 +236,8 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 		usb_phy_set_suspend(omap->phy[i], 0);
 	}
 
+	pm_runtime_put_sync(dev);
+
 	return 0;
 
 err_pm_runtime:
@@ -264,7 +270,9 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev)
 	struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
 	int i;
 
+	pm_runtime_get_sync(dev);
 	usb_remove_hcd(hcd);
+	omap->bound = false;
 
 	for (i = 0; i < omap->nports; i++) {
 		if (omap->phy[i])
@@ -293,15 +301,67 @@ static const struct of_device_id omap_ehci_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids);
 
+static int omap_ehci_suspend(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	return ehci_suspend(hcd, true);
+}
+
+static int omap_ehci_resume(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	return ehci_resume(hcd, false);
+}
+
+static int omap_ehci_runtime_suspend(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+	bool do_wakeup = device_may_wakeup(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	if (omap->bound)
+		ehci_suspend(hcd, do_wakeup);
+
+	return 0;
+}
+
+static int omap_ehci_runtime_resume(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	if (omap->bound)
+		ehci_resume(hcd, false);
+
+	return 0;
+}
+
+
+static const struct dev_pm_ops omap_ehci_pm_ops = {
+	.suspend = omap_ehci_suspend,
+	.resume = omap_ehci_resume,
+	.runtime_suspend = omap_ehci_runtime_suspend,
+	.runtime_resume = omap_ehci_runtime_resume,
+};
+
 static struct platform_driver ehci_hcd_omap_driver = {
 	.probe			= ehci_hcd_omap_probe,
 	.remove			= ehci_hcd_omap_remove,
 	.shutdown		= ehci_hcd_omap_shutdown,
-	/*.suspend		= ehci_hcd_omap_suspend, */
-	/*.resume		= ehci_hcd_omap_resume, */
 	.driver = {
 		.name		= hcd_name,
 		.of_match_table = omap_ehci_dt_ids,
+		.pm		= &omap_ehci_pm_ops,
 	}
 };
 
-- 
1.7.4.1

[PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros]

Call ehci_suspend/resume() during runtime suspend/resume
as well as system suspend/resume.

Use a flag "bound" to indicate that the HCD structures are valid.
This is only true between usb_add_hcd() and usb_remove_hcd() calls.

The flag can be used by omap_ehci_runtime_suspend/resume() handlers
to avoid calling ehci_suspend/resume() when we are no longer bound
to the HCD e.g. during probe() and remove();

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/host/ehci-omap.c |   64 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 9bd7dfe..6d1fde97 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -69,6 +69,7 @@ static const char hcd_name[] = "ehci-omap";
 struct omap_hcd {
 	struct usb_phy *phy[OMAP3_HS_USB_PORTS]; /* one PHY for each port */
 	int nports;
+	bool bound;	/* HCD structures are valid */
 };
 
 static inline void ehci_write(void __iomem *base, u32 reg, u32 val)
@@ -159,6 +160,7 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 	hcd->rsrc_start = res->start;
 	hcd->rsrc_len = resource_size(res);
 	hcd->regs = regs;
+	hcd->has_wakeup_irq = true;
 	hcd_to_ehci(hcd)->caps = regs;
 
 	omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
@@ -216,6 +218,8 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 		goto err_pm_runtime;
 	}
 
+	omap->bound = true;
+
 	/*
 	 * Bring PHYs out of reset for non PHY modes.
 	 * Even though HSIC mode is a PHY-less mode, the reset
@@ -232,6 +236,8 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 		usb_phy_set_suspend(omap->phy[i], 0);
 	}
 
+	pm_runtime_put_sync(dev);
+
 	return 0;
 
 err_pm_runtime:
@@ -264,7 +270,9 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev)
 	struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
 	int i;
 
+	pm_runtime_get_sync(dev);
 	usb_remove_hcd(hcd);
+	omap->bound = false;
 
 	for (i = 0; i < omap->nports; i++) {
 		if (omap->phy[i])
@@ -293,15 +301,67 @@ static const struct of_device_id omap_ehci_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids);
 
+static int omap_ehci_suspend(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	return ehci_suspend(hcd, true);
+}
+
+static int omap_ehci_resume(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	return ehci_resume(hcd, false);
+}
+
+static int omap_ehci_runtime_suspend(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+	bool do_wakeup = device_may_wakeup(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	if (omap->bound)
+		ehci_suspend(hcd, do_wakeup);
+
+	return 0;
+}
+
+static int omap_ehci_runtime_resume(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	if (omap->bound)
+		ehci_resume(hcd, false);
+
+	return 0;
+}
+
+
+static const struct dev_pm_ops omap_ehci_pm_ops = {
+	.suspend = omap_ehci_suspend,
+	.resume = omap_ehci_resume,
+	.runtime_suspend = omap_ehci_runtime_suspend,
+	.runtime_resume = omap_ehci_runtime_resume,
+};
+
 static struct platform_driver ehci_hcd_omap_driver = {
 	.probe			= ehci_hcd_omap_probe,
 	.remove			= ehci_hcd_omap_remove,
 	.shutdown		= ehci_hcd_omap_shutdown,
-	/*.suspend		= ehci_hcd_omap_suspend, */
-	/*.resume		= ehci_hcd_omap_resume, */
 	.driver = {
 		.name		= hcd_name,
 		.of_match_table = omap_ehci_dt_ids,
+		.pm		= &omap_ehci_pm_ops,
 	}
 };
 
-- 
1.7.4.1

[PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros]

Call ehci_suspend/resume() during runtime suspend/resume
as well as system suspend/resume.

Use a flag "bound" to indicate that the HCD structures are valid.
This is only true between usb_add_hcd() and usb_remove_hcd() calls.

The flag can be used by omap_ehci_runtime_suspend/resume() handlers
to avoid calling ehci_suspend/resume() when we are no longer bound
to the HCD e.g. during probe() and remove();

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/host/ehci-omap.c |   64 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 9bd7dfe..6d1fde97 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -69,6 +69,7 @@ static const char hcd_name[] = "ehci-omap";
 struct omap_hcd {
 	struct usb_phy *phy[OMAP3_HS_USB_PORTS]; /* one PHY for each port */
 	int nports;
+	bool bound;	/* HCD structures are valid */
 };
 
 static inline void ehci_write(void __iomem *base, u32 reg, u32 val)
@@ -159,6 +160,7 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 	hcd->rsrc_start = res->start;
 	hcd->rsrc_len = resource_size(res);
 	hcd->regs = regs;
+	hcd->has_wakeup_irq = true;
 	hcd_to_ehci(hcd)->caps = regs;
 
 	omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
@@ -216,6 +218,8 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 		goto err_pm_runtime;
 	}
 
+	omap->bound = true;
+
 	/*
 	 * Bring PHYs out of reset for non PHY modes.
 	 * Even though HSIC mode is a PHY-less mode, the reset
@@ -232,6 +236,8 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 		usb_phy_set_suspend(omap->phy[i], 0);
 	}
 
+	pm_runtime_put_sync(dev);
+
 	return 0;
 
 err_pm_runtime:
@@ -264,7 +270,9 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev)
 	struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
 	int i;
 
+	pm_runtime_get_sync(dev);
 	usb_remove_hcd(hcd);
+	omap->bound = false;
 
 	for (i = 0; i < omap->nports; i++) {
 		if (omap->phy[i])
@@ -293,15 +301,67 @@ static const struct of_device_id omap_ehci_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids);
 
+static int omap_ehci_suspend(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	return ehci_suspend(hcd, true);
+}
+
+static int omap_ehci_resume(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	return ehci_resume(hcd, false);
+}
+
+static int omap_ehci_runtime_suspend(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+	bool do_wakeup = device_may_wakeup(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	if (omap->bound)
+		ehci_suspend(hcd, do_wakeup);
+
+	return 0;
+}
+
+static int omap_ehci_runtime_resume(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	if (omap->bound)
+		ehci_resume(hcd, false);
+
+	return 0;
+}
+
+
+static const struct dev_pm_ops omap_ehci_pm_ops = {
+	.suspend = omap_ehci_suspend,
+	.resume = omap_ehci_resume,
+	.runtime_suspend = omap_ehci_runtime_suspend,
+	.runtime_resume = omap_ehci_runtime_resume,
+};
+
 static struct platform_driver ehci_hcd_omap_driver = {
 	.probe			= ehci_hcd_omap_probe,
 	.remove			= ehci_hcd_omap_remove,
 	.shutdown		= ehci_hcd_omap_shutdown,
-	/*.suspend		= ehci_hcd_omap_suspend, */
-	/*.resume		= ehci_hcd_omap_resume, */
 	.driver = {
 		.name		= hcd_name,
 		.of_match_table = omap_ehci_dt_ids,
+		.pm		= &omap_ehci_pm_ops,
 	}
 };
 
-- 
1.7.4.1

Re: [PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Alan Stern]

On Wed, 10 Jul 2013, Roger Quadros wrote:

> Some platforms e.g. ehci-omap can generate an interrupt
> (i.e. remote wakeup) even when the controller is suspended i.e.
> HW_ACCESSIBLE is cleared.
> 
> Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
> such cases.
> 
> We tackle this case by disabling the IRQ, scheduling a
> hub resume and enabling back the IRQ after the controller has
> resumed. This ensures that the IRQ handler runs only after the
> controller is accessible.

> @@ -2248,7 +2253,21 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
>  	 */
>  	local_irq_save(flags);
>  
> -	if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
> +	if (unlikely(HCD_DEAD(hcd)))
> +		rc = IRQ_NONE;
> +	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd) && hcd->has_wakeup_irq)) {
> +		/*
> +		 * We got a wakeup interrupt while the controller was
> +		 * suspending or suspended.  We can't handle it now, so
> +		 * disable the IRQ and resume the root hub (and hence
> +		 * the controller too).
> +		 */
> +		disable_irq_nosync(hcd->irq);
> +		set_bit(HCD_FLAG_IRQ_DISABLED, &hcd->flags);
> +		usb_hcd_resume_root_hub(hcd);
> +
> +		rc = IRQ_HANDLED;
> +	} else if (unlikely(!HCD_HW_ACCESSIBLE(hcd)))
>  		rc = IRQ_NONE;

In the interest of minimizing the amount of work needed in the most 
common case, this should be written as:

	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd))) {
		if (hcd->has_wakeup_irq) {
			...
		} else
			rc = IRQ_NONE;

>  	else if (hcd->driver->irq(hcd) == IRQ_NONE)
>  		rc = IRQ_NONE;

Apart from that, the patch is okay.  When you rearrange the logic, you 
can add my Acked-by.

Alan Stern

Re: [PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Alan Stern]

On Wed, 10 Jul 2013, Roger Quadros wrote:

> Some platforms e.g. ehci-omap can generate an interrupt
> (i.e. remote wakeup) even when the controller is suspended i.e.
> HW_ACCESSIBLE is cleared.
> 
> Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
> such cases.
> 
> We tackle this case by disabling the IRQ, scheduling a
> hub resume and enabling back the IRQ after the controller has
> resumed. This ensures that the IRQ handler runs only after the
> controller is accessible.

> @@ -2248,7 +2253,21 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
>  	 */
>  	local_irq_save(flags);
>  
> -	if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
> +	if (unlikely(HCD_DEAD(hcd)))
> +		rc = IRQ_NONE;
> +	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd) && hcd->has_wakeup_irq)) {
> +		/*
> +		 * We got a wakeup interrupt while the controller was
> +		 * suspending or suspended.  We can't handle it now, so
> +		 * disable the IRQ and resume the root hub (and hence
> +		 * the controller too).
> +		 */
> +		disable_irq_nosync(hcd->irq);
> +		set_bit(HCD_FLAG_IRQ_DISABLED, &hcd->flags);
> +		usb_hcd_resume_root_hub(hcd);
> +
> +		rc = IRQ_HANDLED;
> +	} else if (unlikely(!HCD_HW_ACCESSIBLE(hcd)))
>  		rc = IRQ_NONE;

In the interest of minimizing the amount of work needed in the most 
common case, this should be written as:

	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd))) {
		if (hcd->has_wakeup_irq) {
			...
		} else
			rc = IRQ_NONE;

>  	else if (hcd->driver->irq(hcd) == IRQ_NONE)
>  		rc = IRQ_NONE;

Apart from that, the patch is okay.  When you rearrange the logic, you 
can add my Acked-by.

Alan Stern

[PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Alan Stern]

On Wed, 10 Jul 2013, Roger Quadros wrote:

> Some platforms e.g. ehci-omap can generate an interrupt
> (i.e. remote wakeup) even when the controller is suspended i.e.
> HW_ACCESSIBLE is cleared.
> 
> Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
> such cases.
> 
> We tackle this case by disabling the IRQ, scheduling a
> hub resume and enabling back the IRQ after the controller has
> resumed. This ensures that the IRQ handler runs only after the
> controller is accessible.

> @@ -2248,7 +2253,21 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
>  	 */
>  	local_irq_save(flags);
>  
> -	if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
> +	if (unlikely(HCD_DEAD(hcd)))
> +		rc = IRQ_NONE;
> +	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd) && hcd->has_wakeup_irq)) {
> +		/*
> +		 * We got a wakeup interrupt while the controller was
> +		 * suspending or suspended.  We can't handle it now, so
> +		 * disable the IRQ and resume the root hub (and hence
> +		 * the controller too).
> +		 */
> +		disable_irq_nosync(hcd->irq);
> +		set_bit(HCD_FLAG_IRQ_DISABLED, &hcd->flags);
> +		usb_hcd_resume_root_hub(hcd);
> +
> +		rc = IRQ_HANDLED;
> +	} else if (unlikely(!HCD_HW_ACCESSIBLE(hcd)))
>  		rc = IRQ_NONE;

In the interest of minimizing the amount of work needed in the most 
common case, this should be written as:

	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd))) {
		if (hcd->has_wakeup_irq) {
			...
		} else
			rc = IRQ_NONE;

>  	else if (hcd->driver->irq(hcd) == IRQ_NONE)
>  		rc = IRQ_NONE;

Apart from that, the patch is okay.  When you rearrange the logic, you 
can add my Acked-by.

Alan Stern

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Alan Stern]

On Wed, 10 Jul 2013, Roger Quadros wrote:

> Call ehci_suspend/resume() during runtime suspend/resume
> as well as system suspend/resume.
> 
> Use a flag "bound" to indicate that the HCD structures are valid.
> This is only true between usb_add_hcd() and usb_remove_hcd() calls.
> 
> The flag can be used by omap_ehci_runtime_suspend/resume() handlers
> to avoid calling ehci_suspend/resume() when we are no longer bound
> to the HCD e.g. during probe() and remove();

> @@ -293,15 +301,67 @@ static const struct of_device_id omap_ehci_dt_ids[] = {
>  
>  MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids);
>  
> +static int omap_ehci_suspend(struct device *dev)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +
> +	dev_dbg(dev, "%s\n", __func__);
> +
> +	return ehci_suspend(hcd, true);
> +}
> +
> +static int omap_ehci_resume(struct device *dev)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +
> +	dev_dbg(dev, "%s\n", __func__);
> +
> +	return ehci_resume(hcd, false);
> +}

There are three problems here.  The first is very simple: the wakeup 
settings are messed up.  Wakeup is supposed to be enabled always for
runtime suspend, whereas it is controlled by device_may_wakeup() for
system suspend.  You reversed them.

The other two problems are both related to the interaction between
system PM and runtime PM.  Suppose the controller is already runtime
suspended when the system goes to sleep.  Because it is runtime
suspended, it is enabled for wakeup.  But device_may_wakeup() could
return false; if this happens then you have to do a runtime-resume in
omap_ehci_suspend() before calling ehci_suspend(), so that the
controller can be suspended again with wakeups disabled.  (Or you could
choose an alternative method for accomplishing the same result, such as
disabling the wakeup signal from the pad without going through a whole
EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
true then you shouldn't do anything at all, because the controller is
already suspended with the correct wakeup setting.

For the third problem, suppose the controller was runtime suspended
when the system went to sleep.  When the system wakes up, the
controller will become active, so you have to inform the runtime PM
core about its change of state.  Basically, if omap_ehci_resume() sees
that ehci_resume() returned 0 then it must do:

	pm_runtime_disable(dev);
	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);

All of these issues are discussed (among lots of other material) in 
Documentation/power/runtime_pm.txt.

Alan Stern

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Alan Stern]

On Wed, 10 Jul 2013, Roger Quadros wrote:

> Call ehci_suspend/resume() during runtime suspend/resume
> as well as system suspend/resume.
> 
> Use a flag "bound" to indicate that the HCD structures are valid.
> This is only true between usb_add_hcd() and usb_remove_hcd() calls.
> 
> The flag can be used by omap_ehci_runtime_suspend/resume() handlers
> to avoid calling ehci_suspend/resume() when we are no longer bound
> to the HCD e.g. during probe() and remove();

> @@ -293,15 +301,67 @@ static const struct of_device_id omap_ehci_dt_ids[] = {
>  
>  MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids);
>  
> +static int omap_ehci_suspend(struct device *dev)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +
> +	dev_dbg(dev, "%s\n", __func__);
> +
> +	return ehci_suspend(hcd, true);
> +}
> +
> +static int omap_ehci_resume(struct device *dev)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +
> +	dev_dbg(dev, "%s\n", __func__);
> +
> +	return ehci_resume(hcd, false);
> +}

There are three problems here.  The first is very simple: the wakeup 
settings are messed up.  Wakeup is supposed to be enabled always for
runtime suspend, whereas it is controlled by device_may_wakeup() for
system suspend.  You reversed them.

The other two problems are both related to the interaction between
system PM and runtime PM.  Suppose the controller is already runtime
suspended when the system goes to sleep.  Because it is runtime
suspended, it is enabled for wakeup.  But device_may_wakeup() could
return false; if this happens then you have to do a runtime-resume in
omap_ehci_suspend() before calling ehci_suspend(), so that the
controller can be suspended again with wakeups disabled.  (Or you could
choose an alternative method for accomplishing the same result, such as
disabling the wakeup signal from the pad without going through a whole
EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
true then you shouldn't do anything at all, because the controller is
already suspended with the correct wakeup setting.

For the third problem, suppose the controller was runtime suspended
when the system went to sleep.  When the system wakes up, the
controller will become active, so you have to inform the runtime PM
core about its change of state.  Basically, if omap_ehci_resume() sees
that ehci_resume() returned 0 then it must do:

	pm_runtime_disable(dev);
	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);

All of these issues are discussed (among lots of other material) in 
Documentation/power/runtime_pm.txt.

Alan Stern

[PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Alan Stern]

On Wed, 10 Jul 2013, Roger Quadros wrote:

> Call ehci_suspend/resume() during runtime suspend/resume
> as well as system suspend/resume.
> 
> Use a flag "bound" to indicate that the HCD structures are valid.
> This is only true between usb_add_hcd() and usb_remove_hcd() calls.
> 
> The flag can be used by omap_ehci_runtime_suspend/resume() handlers
> to avoid calling ehci_suspend/resume() when we are no longer bound
> to the HCD e.g. during probe() and remove();

> @@ -293,15 +301,67 @@ static const struct of_device_id omap_ehci_dt_ids[] = {
>  
>  MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids);
>  
> +static int omap_ehci_suspend(struct device *dev)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +
> +	dev_dbg(dev, "%s\n", __func__);
> +
> +	return ehci_suspend(hcd, true);
> +}
> +
> +static int omap_ehci_resume(struct device *dev)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +
> +	dev_dbg(dev, "%s\n", __func__);
> +
> +	return ehci_resume(hcd, false);
> +}

There are three problems here.  The first is very simple: the wakeup 
settings are messed up.  Wakeup is supposed to be enabled always for
runtime suspend, whereas it is controlled by device_may_wakeup() for
system suspend.  You reversed them.

The other two problems are both related to the interaction between
system PM and runtime PM.  Suppose the controller is already runtime
suspended when the system goes to sleep.  Because it is runtime
suspended, it is enabled for wakeup.  But device_may_wakeup() could
return false; if this happens then you have to do a runtime-resume in
omap_ehci_suspend() before calling ehci_suspend(), so that the
controller can be suspended again with wakeups disabled.  (Or you could
choose an alternative method for accomplishing the same result, such as
disabling the wakeup signal from the pad without going through a whole
EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
true then you shouldn't do anything at all, because the controller is
already suspended with the correct wakeup setting.

For the third problem, suppose the controller was runtime suspended
when the system went to sleep.  When the system wakes up, the
controller will become active, so you have to inform the runtime PM
core about its change of state.  Basically, if omap_ehci_resume() sees
that ehci_resume() returned 0 then it must do:

	pm_runtime_disable(dev);
	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);

All of these issues are discussed (among lots of other material) in 
Documentation/power/runtime_pm.txt.

Alan Stern

Re: [PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Alan Stern]

On Wed, 10 Jul 2013, Roger Quadros wrote:

> Some platforms e.g. ehci-omap can generate an interrupt
> (i.e. remote wakeup) even when the controller is suspended i.e.
> HW_ACCESSIBLE is cleared.
> 
> Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
> such cases.
> 
> We tackle this case by disabling the IRQ, scheduling a
> hub resume and enabling back the IRQ after the controller has
> resumed. This ensures that the IRQ handler runs only after the
> controller is accessible.

Oh yes, one more thing...

> @@ -132,6 +134,7 @@ struct usb_hcd {
>  	unsigned		wireless:1;	/* Wireless USB HCD */
>  	unsigned		authorized_default:1;
>  	unsigned		has_tt:1;	/* Integrated TT in root hub */
> +	unsigned		has_wakeup_irq:1; /* Can IRQ when suspended */

Please add a highly visible comment here, warning that has_wakeup_irq
should never be set on systems with shared IRQs.  Having both would ...
well, it would indicate a really bad system design.

Alan Stern

Re: [PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Alan Stern]

On Wed, 10 Jul 2013, Roger Quadros wrote:

> Some platforms e.g. ehci-omap can generate an interrupt
> (i.e. remote wakeup) even when the controller is suspended i.e.
> HW_ACCESSIBLE is cleared.
> 
> Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
> such cases.
> 
> We tackle this case by disabling the IRQ, scheduling a
> hub resume and enabling back the IRQ after the controller has
> resumed. This ensures that the IRQ handler runs only after the
> controller is accessible.

Oh yes, one more thing...

> @@ -132,6 +134,7 @@ struct usb_hcd {
>  	unsigned		wireless:1;	/* Wireless USB HCD */
>  	unsigned		authorized_default:1;
>  	unsigned		has_tt:1;	/* Integrated TT in root hub */
> +	unsigned		has_wakeup_irq:1; /* Can IRQ when suspended */

Please add a highly visible comment here, warning that has_wakeup_irq
should never be set on systems with shared IRQs.  Having both would ...
well, it would indicate a really bad system design.

Alan Stern

[PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Alan Stern]

On Wed, 10 Jul 2013, Roger Quadros wrote:

> Some platforms e.g. ehci-omap can generate an interrupt
> (i.e. remote wakeup) even when the controller is suspended i.e.
> HW_ACCESSIBLE is cleared.
> 
> Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
> such cases.
> 
> We tackle this case by disabling the IRQ, scheduling a
> hub resume and enabling back the IRQ after the controller has
> resumed. This ensures that the IRQ handler runs only after the
> controller is accessible.

Oh yes, one more thing...

> @@ -132,6 +134,7 @@ struct usb_hcd {
>  	unsigned		wireless:1;	/* Wireless USB HCD */
>  	unsigned		authorized_default:1;
>  	unsigned		has_tt:1;	/* Integrated TT in root hub */
> +	unsigned		has_wakeup_irq:1; /* Can IRQ when suspended */

Please add a highly visible comment here, warning that has_wakeup_irq
should never be set on systems with shared IRQs.  Having both would ...
well, it would indicate a really bad system design.

Alan Stern

Re: [PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Roger Quadros]

On 07/10/2013 09:45 PM, Alan Stern wrote:
> On Wed, 10 Jul 2013, Roger Quadros wrote:
> 
>> Some platforms e.g. ehci-omap can generate an interrupt
>> (i.e. remote wakeup) even when the controller is suspended i.e.
>> HW_ACCESSIBLE is cleared.
>>
>> Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
>> such cases.
>>
>> We tackle this case by disabling the IRQ, scheduling a
>> hub resume and enabling back the IRQ after the controller has
>> resumed. This ensures that the IRQ handler runs only after the
>> controller is accessible.
> 
>> @@ -2248,7 +2253,21 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
>>  	 */
>>  	local_irq_save(flags);
>>  
>> -	if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
>> +	if (unlikely(HCD_DEAD(hcd)))
>> +		rc = IRQ_NONE;
>> +	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd) && hcd->has_wakeup_irq)) {
>> +		/*
>> +		 * We got a wakeup interrupt while the controller was
>> +		 * suspending or suspended.  We can't handle it now, so
>> +		 * disable the IRQ and resume the root hub (and hence
>> +		 * the controller too).
>> +		 */
>> +		disable_irq_nosync(hcd->irq);
>> +		set_bit(HCD_FLAG_IRQ_DISABLED, &hcd->flags);
>> +		usb_hcd_resume_root_hub(hcd);
>> +
>> +		rc = IRQ_HANDLED;
>> +	} else if (unlikely(!HCD_HW_ACCESSIBLE(hcd)))
>>  		rc = IRQ_NONE;
> 
> In the interest of minimizing the amount of work needed in the most 
> common case, this should be written as:
> 
> 	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd))) {
> 		if (hcd->has_wakeup_irq) {
> 			...
> 		} else
> 			rc = IRQ_NONE;
> 

OK.

>>  	else if (hcd->driver->irq(hcd) == IRQ_NONE)
>>  		rc = IRQ_NONE;
> 
> Apart from that, the patch is okay.  When you rearrange the logic, you 
> can add my Acked-by.
> 
Thanks.

cheers,
-roger

Re: [PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Roger Quadros]

On 07/10/2013 09:45 PM, Alan Stern wrote:
> On Wed, 10 Jul 2013, Roger Quadros wrote:
> 
>> Some platforms e.g. ehci-omap can generate an interrupt
>> (i.e. remote wakeup) even when the controller is suspended i.e.
>> HW_ACCESSIBLE is cleared.
>>
>> Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
>> such cases.
>>
>> We tackle this case by disabling the IRQ, scheduling a
>> hub resume and enabling back the IRQ after the controller has
>> resumed. This ensures that the IRQ handler runs only after the
>> controller is accessible.
> 
>> @@ -2248,7 +2253,21 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
>>  	 */
>>  	local_irq_save(flags);
>>  
>> -	if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
>> +	if (unlikely(HCD_DEAD(hcd)))
>> +		rc = IRQ_NONE;
>> +	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd) && hcd->has_wakeup_irq)) {
>> +		/*
>> +		 * We got a wakeup interrupt while the controller was
>> +		 * suspending or suspended.  We can't handle it now, so
>> +		 * disable the IRQ and resume the root hub (and hence
>> +		 * the controller too).
>> +		 */
>> +		disable_irq_nosync(hcd->irq);
>> +		set_bit(HCD_FLAG_IRQ_DISABLED, &hcd->flags);
>> +		usb_hcd_resume_root_hub(hcd);
>> +
>> +		rc = IRQ_HANDLED;
>> +	} else if (unlikely(!HCD_HW_ACCESSIBLE(hcd)))
>>  		rc = IRQ_NONE;
> 
> In the interest of minimizing the amount of work needed in the most 
> common case, this should be written as:
> 
> 	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd))) {
> 		if (hcd->has_wakeup_irq) {
> 			...
> 		} else
> 			rc = IRQ_NONE;
> 

OK.

>>  	else if (hcd->driver->irq(hcd) == IRQ_NONE)
>>  		rc = IRQ_NONE;
> 
> Apart from that, the patch is okay.  When you rearrange the logic, you 
> can add my Acked-by.
> 
Thanks.

cheers,
-roger

[PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Roger Quadros]

On 07/10/2013 09:45 PM, Alan Stern wrote:
> On Wed, 10 Jul 2013, Roger Quadros wrote:
> 
>> Some platforms e.g. ehci-omap can generate an interrupt
>> (i.e. remote wakeup) even when the controller is suspended i.e.
>> HW_ACCESSIBLE is cleared.
>>
>> Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
>> such cases.
>>
>> We tackle this case by disabling the IRQ, scheduling a
>> hub resume and enabling back the IRQ after the controller has
>> resumed. This ensures that the IRQ handler runs only after the
>> controller is accessible.
> 
>> @@ -2248,7 +2253,21 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
>>  	 */
>>  	local_irq_save(flags);
>>  
>> -	if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
>> +	if (unlikely(HCD_DEAD(hcd)))
>> +		rc = IRQ_NONE;
>> +	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd) && hcd->has_wakeup_irq)) {
>> +		/*
>> +		 * We got a wakeup interrupt while the controller was
>> +		 * suspending or suspended.  We can't handle it now, so
>> +		 * disable the IRQ and resume the root hub (and hence
>> +		 * the controller too).
>> +		 */
>> +		disable_irq_nosync(hcd->irq);
>> +		set_bit(HCD_FLAG_IRQ_DISABLED, &hcd->flags);
>> +		usb_hcd_resume_root_hub(hcd);
>> +
>> +		rc = IRQ_HANDLED;
>> +	} else if (unlikely(!HCD_HW_ACCESSIBLE(hcd)))
>>  		rc = IRQ_NONE;
> 
> In the interest of minimizing the amount of work needed in the most 
> common case, this should be written as:
> 
> 	else if (unlikely(!HCD_HW_ACCESSIBLE(hcd))) {
> 		if (hcd->has_wakeup_irq) {
> 			...
> 		} else
> 			rc = IRQ_NONE;
> 

OK.

>>  	else if (hcd->driver->irq(hcd) == IRQ_NONE)
>>  		rc = IRQ_NONE;
> 
> Apart from that, the patch is okay.  When you rearrange the logic, you 
> can add my Acked-by.
> 
Thanks.

cheers,
-roger

Re: [PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Roger Quadros]

On 07/10/2013 10:08 PM, Alan Stern wrote:
> On Wed, 10 Jul 2013, Roger Quadros wrote:
> 
>> Some platforms e.g. ehci-omap can generate an interrupt
>> (i.e. remote wakeup) even when the controller is suspended i.e.
>> HW_ACCESSIBLE is cleared.
>>
>> Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
>> such cases.
>>
>> We tackle this case by disabling the IRQ, scheduling a
>> hub resume and enabling back the IRQ after the controller has
>> resumed. This ensures that the IRQ handler runs only after the
>> controller is accessible.
> 
> Oh yes, one more thing...
> 
>> @@ -132,6 +134,7 @@ struct usb_hcd {
>>  	unsigned		wireless:1;	/* Wireless USB HCD */
>>  	unsigned		authorized_default:1;
>>  	unsigned		has_tt:1;	/* Integrated TT in root hub */
>> +	unsigned		has_wakeup_irq:1; /* Can IRQ when suspended */
> 
> Please add a highly visible comment here, warning that has_wakeup_irq
> should never be set on systems with shared IRQs.  Having both would ...
> well, it would indicate a really bad system design.
> 
OK, will do.

cheers,
-roger

Re: [PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Roger Quadros]

On 07/10/2013 10:08 PM, Alan Stern wrote:
> On Wed, 10 Jul 2013, Roger Quadros wrote:
> 
>> Some platforms e.g. ehci-omap can generate an interrupt
>> (i.e. remote wakeup) even when the controller is suspended i.e.
>> HW_ACCESSIBLE is cleared.
>>
>> Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
>> such cases.
>>
>> We tackle this case by disabling the IRQ, scheduling a
>> hub resume and enabling back the IRQ after the controller has
>> resumed. This ensures that the IRQ handler runs only after the
>> controller is accessible.
> 
> Oh yes, one more thing...
> 
>> @@ -132,6 +134,7 @@ struct usb_hcd {
>>  	unsigned		wireless:1;	/* Wireless USB HCD */
>>  	unsigned		authorized_default:1;
>>  	unsigned		has_tt:1;	/* Integrated TT in root hub */
>> +	unsigned		has_wakeup_irq:1; /* Can IRQ when suspended */
> 
> Please add a highly visible comment here, warning that has_wakeup_irq
> should never be set on systems with shared IRQs.  Having both would ...
> well, it would indicate a really bad system design.
> 
OK, will do.

cheers,
-roger

[PATCH 5/6] USB: Support wakeup IRQ for suspended controllers

[Roger Quadros]

On 07/10/2013 10:08 PM, Alan Stern wrote:
> On Wed, 10 Jul 2013, Roger Quadros wrote:
> 
>> Some platforms e.g. ehci-omap can generate an interrupt
>> (i.e. remote wakeup) even when the controller is suspended i.e.
>> HW_ACCESSIBLE is cleared.
>>
>> Introduce a flag "has_wakeup_irq" in struct usb_hcd to indicate
>> such cases.
>>
>> We tackle this case by disabling the IRQ, scheduling a
>> hub resume and enabling back the IRQ after the controller has
>> resumed. This ensures that the IRQ handler runs only after the
>> controller is accessible.
> 
> Oh yes, one more thing...
> 
>> @@ -132,6 +134,7 @@ struct usb_hcd {
>>  	unsigned		wireless:1;	/* Wireless USB HCD */
>>  	unsigned		authorized_default:1;
>>  	unsigned		has_tt:1;	/* Integrated TT in root hub */
>> +	unsigned		has_wakeup_irq:1; /* Can IRQ when suspended */
> 
> Please add a highly visible comment here, warning that has_wakeup_irq
> should never be set on systems with shared IRQs.  Having both would ...
> well, it would indicate a really bad system design.
> 
OK, will do.

cheers,
-roger

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros]

On 07/10/2013 10:04 PM, Alan Stern wrote:
> On Wed, 10 Jul 2013, Roger Quadros wrote:
> 
>> Call ehci_suspend/resume() during runtime suspend/resume
>> as well as system suspend/resume.
>>
>> Use a flag "bound" to indicate that the HCD structures are valid.
>> This is only true between usb_add_hcd() and usb_remove_hcd() calls.
>>
>> The flag can be used by omap_ehci_runtime_suspend/resume() handlers
>> to avoid calling ehci_suspend/resume() when we are no longer bound
>> to the HCD e.g. during probe() and remove();
> 
>> @@ -293,15 +301,67 @@ static const struct of_device_id omap_ehci_dt_ids[] = {
>>  
>>  MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids);
>>  
>> +static int omap_ehci_suspend(struct device *dev)
>> +{
>> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
>> +
>> +	dev_dbg(dev, "%s\n", __func__);
>> +
>> +	return ehci_suspend(hcd, true);
>> +}
>> +
>> +static int omap_ehci_resume(struct device *dev)
>> +{
>> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
>> +
>> +	dev_dbg(dev, "%s\n", __func__);
>> +
>> +	return ehci_resume(hcd, false);
>> +}
> 
> There are three problems here.  The first is very simple: the wakeup 
> settings are messed up.  Wakeup is supposed to be enabled always for
> runtime suspend, whereas it is controlled by device_may_wakeup() for
> system suspend.  You reversed them.

Indeed. I'll fix that up.

However, even if wakeup is disabled during system suspend, the OMAP will
still wakeup on any activity on the USB pins.

The only way to disable that is to disable IO pad wakeup. This can
only be done in mfd/omap-usb-host.c

I suppose I can use device_may_wakeup() there and configure the IO pad
wakeup accordingly.

> 
> The other two problems are both related to the interaction between
> system PM and runtime PM.  Suppose the controller is already runtime
> suspended when the system goes to sleep.  Because it is runtime
> suspended, it is enabled for wakeup.  But device_may_wakeup() could
> return false; if this happens then you have to do a runtime-resume in
> omap_ehci_suspend() before calling ehci_suspend(), so that the
> controller can be suspended again with wakeups disabled.  (Or you could
> choose an alternative method for accomplishing the same result, such as
> disabling the wakeup signal from the pad without going through a whole
> EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
> true then you shouldn't do anything at all, because the controller is
> already suspended with the correct wakeup setting.

I think this case is taken care of by the Runtime PM core at least for the OMAP
platform according to the documentation

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647

At the end of this mail is the log during system suspend/resume

You can notice the following sequence

-ehci runtime suspends
-system suspend triggered
-ehci runtime resumes
-ehci suspends (uses new wakeup settings)
-system wakeup triggered
-ehci resumes
-ehci runtime suspends

> 
> For the third problem, suppose the controller was runtime suspended
> when the system went to sleep.  When the system wakes up, the
> controller will become active, so you have to inform the runtime PM
> core about its change of state.  Basically, if omap_ehci_resume() sees
> that ehci_resume() returned 0 then it must do:
> 
> 	pm_runtime_disable(dev);
> 	pm_runtime_set_active(dev);
> 	pm_runtime_enable(dev);
> 
> All of these issues are discussed (among lots of other material) in 
> Documentation/power/runtime_pm.txt.

Is this still applicable? Documentation claims

   "During system resume it calls pm_runtime_enable() and pm_runtime_put_sync()
    for every device right after executing the subsystem-level .resume_early()
    callback and right after executing the subsystem-level .resume() callback
    for it, respectively."

cheers,
-roger

---
[   13.256286] smsc95xx 1-2.1:1.0 eth0: entering SUSPEND2 mode
[   13.264648] usb 1-2.1: usb auto-suspend, wakeup 0
[   13.281677] hub 1-2:1.0: hub_suspend
[   13.285858] usb 1-2: unlink qh256-0001/dd443880 start 1 [1/0 us]
/
/ # [   13.296386] usb 1-2: usb auto-suspend, wakeup 1
[   13.321411] hub 1-0:1.0: hub_suspend
[   13.325225] usb usb1: bus auto-suspend, wakeup 1
[   13.330139] ehci-omap 48064800.ehci: suspend root hub
[   13.336395] ehci-omap 48064800.ehci: omap_ehci_runtime_suspend

/ # 
/ # echo mem > /sys/power/state 
[   26.821838] PM: Syncing filesystems ... done.
[   26.828491] PM: Preparing system for mem sleep
[   26.841796] Freezing user space processes ... (elapsed 0.000 seconds) done.
[   26.849426] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[   26.860046] PM: Entering mem sleep
[   26.863769] Suspending console(s) (use no_console_suspend to debug)
[   26.878234] ehci-omap 48064800.ehci: omap_ehci_runtime_resume
[   26.878295] usb usb1: usb auto-resume
[   26.878326] ehci-omap 48064800.ehci: resume root hub
[   26.878417] hub 1-0:1.0: hub_resume
[   26.878570] hub 1-0:1.0: port 2: status 0507 change 0000
[   26.879089] hub 1-0:1.0: hub_suspend
[   26.879211] usb usb1: bus suspend, wakeup 0
[   26.879241] ehci-omap 48064800.ehci: suspend root hub
[   26.891845] ehci-omap 48064800.ehci: omap_ehci_suspend may_wakeup 0
[   26.923797] PM: suspend of devices complete after 47.912 msecs
[   26.929443] PM: late suspend of devices complete after 5.645 msecs
[   26.935882] PM: noirq suspend of devices complete after 6.378 msecs
[   26.935943] Disabling non-boot CPUs ...
[   26.936065] Successfully put all powerdomains to target state
[   26.939636] PM: noirq resume of devices complete after 3.448 msecs
[   26.945190] PM: early resume of devices complete after 4.180 msecs
[   26.954833] ehci-omap 48064800.ehci: omap_ehci_resume
[   26.955383] usb usb1: usb resume
[   26.955383] ehci-omap 48064800.ehci: resume root hub
[   26.955413] hub 1-0:1.0: hub_resume
[   26.955535] hub 1-0:1.0: port 2: status 0507 change 0000
[   26.955902] usb 1-2: usb resume
[   26.990661] ehci-omap 48064800.ehci: GetStatus port:2 status 001005 0  ACK POWER sig=se0 PE CONNECT
[   27.010528] usb 1-2: finish resume
[   27.010894] hub 1-2:1.0: hub_resume
[   27.011352] hub 1-2:1.0: port 1: status 0507 change 0000
[   27.012390] ehci-omap 48064800.ehci: reused qh dd443880 schedule
[   27.012390] usb 1-2: link qh256-0001/dd443880 start 1 [1/0 us]
[   27.013244] usb 1-2.1: usb resume
[   27.070800] usb 1-2.1: finish resume
[   27.072998] PM: resume of devices complete after 127.716 msecs
[   27.243438] PM: Finishing wakeup.
[   27.246948] Restarting tasks ... [   27.251312] hub 1-0:1.0: state 7 ports 3 chg 0000 evt 0000
[   27.257171] hub 1-2:1.0: state 7 ports 5 chg 0000 evt 0000
done.
/ # [   29.074493] smsc95xx 1-2.1:1.0 eth0: entering SUSPEND2 mode
[   29.082458] usb 1-2.1: usb auto-suspend, wakeup 0
[   29.100891] hub 1-2:1.0: hub_suspend
[   29.104736] usb 1-2: unlink qh256-0001/dd443880 start 1 [1/0 us]
[   29.114288] usb 1-2: usb auto-suspend, wakeup 1
[   29.130859] hub 1-0:1.0: hub_suspend
[   29.134735] usb usb1: bus auto-suspend, wakeup 1
[   29.139648] ehci-omap 48064800.ehci: suspend root hub
[   29.145446] ehci-omap 48064800.ehci: omap_ehci_runtime_suspend

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros]

On 07/10/2013 10:04 PM, Alan Stern wrote:
> On Wed, 10 Jul 2013, Roger Quadros wrote:
> 
>> Call ehci_suspend/resume() during runtime suspend/resume
>> as well as system suspend/resume.
>>
>> Use a flag "bound" to indicate that the HCD structures are valid.
>> This is only true between usb_add_hcd() and usb_remove_hcd() calls.
>>
>> The flag can be used by omap_ehci_runtime_suspend/resume() handlers
>> to avoid calling ehci_suspend/resume() when we are no longer bound
>> to the HCD e.g. during probe() and remove();
> 
>> @@ -293,15 +301,67 @@ static const struct of_device_id omap_ehci_dt_ids[] = {
>>  
>>  MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids);
>>  
>> +static int omap_ehci_suspend(struct device *dev)
>> +{
>> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
>> +
>> +	dev_dbg(dev, "%s\n", __func__);
>> +
>> +	return ehci_suspend(hcd, true);
>> +}
>> +
>> +static int omap_ehci_resume(struct device *dev)
>> +{
>> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
>> +
>> +	dev_dbg(dev, "%s\n", __func__);
>> +
>> +	return ehci_resume(hcd, false);
>> +}
> 
> There are three problems here.  The first is very simple: the wakeup 
> settings are messed up.  Wakeup is supposed to be enabled always for
> runtime suspend, whereas it is controlled by device_may_wakeup() for
> system suspend.  You reversed them.

Indeed. I'll fix that up.

However, even if wakeup is disabled during system suspend, the OMAP will
still wakeup on any activity on the USB pins.

The only way to disable that is to disable IO pad wakeup. This can
only be done in mfd/omap-usb-host.c

I suppose I can use device_may_wakeup() there and configure the IO pad
wakeup accordingly.

> 
> The other two problems are both related to the interaction between
> system PM and runtime PM.  Suppose the controller is already runtime
> suspended when the system goes to sleep.  Because it is runtime
> suspended, it is enabled for wakeup.  But device_may_wakeup() could
> return false; if this happens then you have to do a runtime-resume in
> omap_ehci_suspend() before calling ehci_suspend(), so that the
> controller can be suspended again with wakeups disabled.  (Or you could
> choose an alternative method for accomplishing the same result, such as
> disabling the wakeup signal from the pad without going through a whole
> EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
> true then you shouldn't do anything at all, because the controller is
> already suspended with the correct wakeup setting.

I think this case is taken care of by the Runtime PM core at least for the OMAP
platform according to the documentation

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647

At the end of this mail is the log during system suspend/resume

You can notice the following sequence

-ehci runtime suspends
-system suspend triggered
-ehci runtime resumes
-ehci suspends (uses new wakeup settings)
-system wakeup triggered
-ehci resumes
-ehci runtime suspends

> 
> For the third problem, suppose the controller was runtime suspended
> when the system went to sleep.  When the system wakes up, the
> controller will become active, so you have to inform the runtime PM
> core about its change of state.  Basically, if omap_ehci_resume() sees
> that ehci_resume() returned 0 then it must do:
> 
> 	pm_runtime_disable(dev);
> 	pm_runtime_set_active(dev);
> 	pm_runtime_enable(dev);
> 
> All of these issues are discussed (among lots of other material) in 
> Documentation/power/runtime_pm.txt.

Is this still applicable? Documentation claims

   "During system resume it calls pm_runtime_enable() and pm_runtime_put_sync()
    for every device right after executing the subsystem-level .resume_early()
    callback and right after executing the subsystem-level .resume() callback
    for it, respectively."

cheers,
-roger

---
[   13.256286] smsc95xx 1-2.1:1.0 eth0: entering SUSPEND2 mode
[   13.264648] usb 1-2.1: usb auto-suspend, wakeup 0
[   13.281677] hub 1-2:1.0: hub_suspend
[   13.285858] usb 1-2: unlink qh256-0001/dd443880 start 1 [1/0 us]
/
/ # [   13.296386] usb 1-2: usb auto-suspend, wakeup 1
[   13.321411] hub 1-0:1.0: hub_suspend
[   13.325225] usb usb1: bus auto-suspend, wakeup 1
[   13.330139] ehci-omap 48064800.ehci: suspend root hub
[   13.336395] ehci-omap 48064800.ehci: omap_ehci_runtime_suspend

/ # 
/ # echo mem > /sys/power/state 
[   26.821838] PM: Syncing filesystems ... done.
[   26.828491] PM: Preparing system for mem sleep
[   26.841796] Freezing user space processes ... (elapsed 0.000 seconds) done.
[   26.849426] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[   26.860046] PM: Entering mem sleep
[   26.863769] Suspending console(s) (use no_console_suspend to debug)
[   26.878234] ehci-omap 48064800.ehci: omap_ehci_runtime_resume
[   26.878295] usb usb1: usb auto-resume
[   26.878326] ehci-omap 48064800.ehci: resume root hub
[   26.878417] hub 1-0:1.0: hub_resume
[   26.878570] hub 1-0:1.0: port 2: status 0507 change 0000
[   26.879089] hub 1-0:1.0: hub_suspend
[   26.879211] usb usb1: bus suspend, wakeup 0
[   26.879241] ehci-omap 48064800.ehci: suspend root hub
[   26.891845] ehci-omap 48064800.ehci: omap_ehci_suspend may_wakeup 0
[   26.923797] PM: suspend of devices complete after 47.912 msecs
[   26.929443] PM: late suspend of devices complete after 5.645 msecs
[   26.935882] PM: noirq suspend of devices complete after 6.378 msecs
[   26.935943] Disabling non-boot CPUs ...
[   26.936065] Successfully put all powerdomains to target state
[   26.939636] PM: noirq resume of devices complete after 3.448 msecs
[   26.945190] PM: early resume of devices complete after 4.180 msecs
[   26.954833] ehci-omap 48064800.ehci: omap_ehci_resume
[   26.955383] usb usb1: usb resume
[   26.955383] ehci-omap 48064800.ehci: resume root hub
[   26.955413] hub 1-0:1.0: hub_resume
[   26.955535] hub 1-0:1.0: port 2: status 0507 change 0000
[   26.955902] usb 1-2: usb resume
[   26.990661] ehci-omap 48064800.ehci: GetStatus port:2 status 001005 0  ACK POWER sig=se0 PE CONNECT
[   27.010528] usb 1-2: finish resume
[   27.010894] hub 1-2:1.0: hub_resume
[   27.011352] hub 1-2:1.0: port 1: status 0507 change 0000
[   27.012390] ehci-omap 48064800.ehci: reused qh dd443880 schedule
[   27.012390] usb 1-2: link qh256-0001/dd443880 start 1 [1/0 us]
[   27.013244] usb 1-2.1: usb resume
[   27.070800] usb 1-2.1: finish resume
[   27.072998] PM: resume of devices complete after 127.716 msecs
[   27.243438] PM: Finishing wakeup.
[   27.246948] Restarting tasks ... [   27.251312] hub 1-0:1.0: state 7 ports 3 chg 0000 evt 0000
[   27.257171] hub 1-2:1.0: state 7 ports 5 chg 0000 evt 0000
done.
/ # [   29.074493] smsc95xx 1-2.1:1.0 eth0: entering SUSPEND2 mode
[   29.082458] usb 1-2.1: usb auto-suspend, wakeup 0
[   29.100891] hub 1-2:1.0: hub_suspend
[   29.104736] usb 1-2: unlink qh256-0001/dd443880 start 1 [1/0 us]
[   29.114288] usb 1-2: usb auto-suspend, wakeup 1
[   29.130859] hub 1-0:1.0: hub_suspend
[   29.134735] usb usb1: bus auto-suspend, wakeup 1
[   29.139648] ehci-omap 48064800.ehci: suspend root hub
[   29.145446] ehci-omap 48064800.ehci: omap_ehci_runtime_suspend

[PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros]

On 07/10/2013 10:04 PM, Alan Stern wrote:
> On Wed, 10 Jul 2013, Roger Quadros wrote:
> 
>> Call ehci_suspend/resume() during runtime suspend/resume
>> as well as system suspend/resume.
>>
>> Use a flag "bound" to indicate that the HCD structures are valid.
>> This is only true between usb_add_hcd() and usb_remove_hcd() calls.
>>
>> The flag can be used by omap_ehci_runtime_suspend/resume() handlers
>> to avoid calling ehci_suspend/resume() when we are no longer bound
>> to the HCD e.g. during probe() and remove();
> 
>> @@ -293,15 +301,67 @@ static const struct of_device_id omap_ehci_dt_ids[] = {
>>  
>>  MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids);
>>  
>> +static int omap_ehci_suspend(struct device *dev)
>> +{
>> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
>> +
>> +	dev_dbg(dev, "%s\n", __func__);
>> +
>> +	return ehci_suspend(hcd, true);
>> +}
>> +
>> +static int omap_ehci_resume(struct device *dev)
>> +{
>> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
>> +
>> +	dev_dbg(dev, "%s\n", __func__);
>> +
>> +	return ehci_resume(hcd, false);
>> +}
> 
> There are three problems here.  The first is very simple: the wakeup 
> settings are messed up.  Wakeup is supposed to be enabled always for
> runtime suspend, whereas it is controlled by device_may_wakeup() for
> system suspend.  You reversed them.

Indeed. I'll fix that up.

However, even if wakeup is disabled during system suspend, the OMAP will
still wakeup on any activity on the USB pins.

The only way to disable that is to disable IO pad wakeup. This can
only be done in mfd/omap-usb-host.c

I suppose I can use device_may_wakeup() there and configure the IO pad
wakeup accordingly.

> 
> The other two problems are both related to the interaction between
> system PM and runtime PM.  Suppose the controller is already runtime
> suspended when the system goes to sleep.  Because it is runtime
> suspended, it is enabled for wakeup.  But device_may_wakeup() could
> return false; if this happens then you have to do a runtime-resume in
> omap_ehci_suspend() before calling ehci_suspend(), so that the
> controller can be suspended again with wakeups disabled.  (Or you could
> choose an alternative method for accomplishing the same result, such as
> disabling the wakeup signal from the pad without going through a whole
> EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
> true then you shouldn't do anything at all, because the controller is
> already suspended with the correct wakeup setting.

I think this case is taken care of by the Runtime PM core at least for the OMAP
platform according to the documentation

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647

At the end of this mail is the log during system suspend/resume

You can notice the following sequence

-ehci runtime suspends
-system suspend triggered
-ehci runtime resumes
-ehci suspends (uses new wakeup settings)
-system wakeup triggered
-ehci resumes
-ehci runtime suspends

> 
> For the third problem, suppose the controller was runtime suspended
> when the system went to sleep.  When the system wakes up, the
> controller will become active, so you have to inform the runtime PM
> core about its change of state.  Basically, if omap_ehci_resume() sees
> that ehci_resume() returned 0 then it must do:
> 
> 	pm_runtime_disable(dev);
> 	pm_runtime_set_active(dev);
> 	pm_runtime_enable(dev);
> 
> All of these issues are discussed (among lots of other material) in 
> Documentation/power/runtime_pm.txt.

Is this still applicable? Documentation claims

   "During system resume it calls pm_runtime_enable() and pm_runtime_put_sync()
    for every device right after executing the subsystem-level .resume_early()
    callback and right after executing the subsystem-level .resume() callback
    for it, respectively."

cheers,
-roger

---
[   13.256286] smsc95xx 1-2.1:1.0 eth0: entering SUSPEND2 mode
[   13.264648] usb 1-2.1: usb auto-suspend, wakeup 0
[   13.281677] hub 1-2:1.0: hub_suspend
[   13.285858] usb 1-2: unlink qh256-0001/dd443880 start 1 [1/0 us]
/
/ # [   13.296386] usb 1-2: usb auto-suspend, wakeup 1
[   13.321411] hub 1-0:1.0: hub_suspend
[   13.325225] usb usb1: bus auto-suspend, wakeup 1
[   13.330139] ehci-omap 48064800.ehci: suspend root hub
[   13.336395] ehci-omap 48064800.ehci: omap_ehci_runtime_suspend

/ # 
/ # echo mem > /sys/power/state 
[   26.821838] PM: Syncing filesystems ... done.
[   26.828491] PM: Preparing system for mem sleep
[   26.841796] Freezing user space processes ... (elapsed 0.000 seconds) done.
[   26.849426] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[   26.860046] PM: Entering mem sleep
[   26.863769] Suspending console(s) (use no_console_suspend to debug)
[   26.878234] ehci-omap 48064800.ehci: omap_ehci_runtime_resume
[   26.878295] usb usb1: usb auto-resume
[   26.878326] ehci-omap 48064800.ehci: resume root hub
[   26.878417] hub 1-0:1.0: hub_resume
[   26.878570] hub 1-0:1.0: port 2: status 0507 change 0000
[   26.879089] hub 1-0:1.0: hub_suspend
[   26.879211] usb usb1: bus suspend, wakeup 0
[   26.879241] ehci-omap 48064800.ehci: suspend root hub
[   26.891845] ehci-omap 48064800.ehci: omap_ehci_suspend may_wakeup 0
[   26.923797] PM: suspend of devices complete after 47.912 msecs
[   26.929443] PM: late suspend of devices complete after 5.645 msecs
[   26.935882] PM: noirq suspend of devices complete after 6.378 msecs
[   26.935943] Disabling non-boot CPUs ...
[   26.936065] Successfully put all powerdomains to target state
[   26.939636] PM: noirq resume of devices complete after 3.448 msecs
[   26.945190] PM: early resume of devices complete after 4.180 msecs
[   26.954833] ehci-omap 48064800.ehci: omap_ehci_resume
[   26.955383] usb usb1: usb resume
[   26.955383] ehci-omap 48064800.ehci: resume root hub
[   26.955413] hub 1-0:1.0: hub_resume
[   26.955535] hub 1-0:1.0: port 2: status 0507 change 0000
[   26.955902] usb 1-2: usb resume
[   26.990661] ehci-omap 48064800.ehci: GetStatus port:2 status 001005 0  ACK POWER sig=se0 PE CONNECT
[   27.010528] usb 1-2: finish resume
[   27.010894] hub 1-2:1.0: hub_resume
[   27.011352] hub 1-2:1.0: port 1: status 0507 change 0000
[   27.012390] ehci-omap 48064800.ehci: reused qh dd443880 schedule
[   27.012390] usb 1-2: link qh256-0001/dd443880 start 1 [1/0 us]
[   27.013244] usb 1-2.1: usb resume
[   27.070800] usb 1-2.1: finish resume
[   27.072998] PM: resume of devices complete after 127.716 msecs
[   27.243438] PM: Finishing wakeup.
[   27.246948] Restarting tasks ... [   27.251312] hub 1-0:1.0: state 7 ports 3 chg 0000 evt 0000
[   27.257171] hub 1-2:1.0: state 7 ports 5 chg 0000 evt 0000
done.
/ # [   29.074493] smsc95xx 1-2.1:1.0 eth0: entering SUSPEND2 mode
[   29.082458] usb 1-2.1: usb auto-suspend, wakeup 0
[   29.100891] hub 1-2:1.0: hub_suspend
[   29.104736] usb 1-2: unlink qh256-0001/dd443880 start 1 [1/0 us]
[   29.114288] usb 1-2: usb auto-suspend, wakeup 1
[   29.130859] hub 1-0:1.0: hub_suspend
[   29.134735] usb usb1: bus auto-suspend, wakeup 1
[   29.139648] ehci-omap 48064800.ehci: suspend root hub
[   29.145446] ehci-omap 48064800.ehci: omap_ehci_runtime_suspend

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Alan Stern]

On Thu, 11 Jul 2013, Roger Quadros wrote:

> > The other two problems are both related to the interaction between
> > system PM and runtime PM.  Suppose the controller is already runtime
> > suspended when the system goes to sleep.  Because it is runtime
> > suspended, it is enabled for wakeup.  But device_may_wakeup() could
> > return false; if this happens then you have to do a runtime-resume in
> > omap_ehci_suspend() before calling ehci_suspend(), so that the
> > controller can be suspended again with wakeups disabled.  (Or you could
> > choose an alternative method for accomplishing the same result, such as
> > disabling the wakeup signal from the pad without going through a whole
> > EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
> > true then you shouldn't do anything at all, because the controller is
> > already suspended with the correct wakeup setting.
> 
> I think this case is taken care of by the Runtime PM core at least for the OMAP
> platform according to the documentation
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647

No; that section refers only to races, not to wakeup settings.

> At the end of this mail is the log during system suspend/resume
> 
> You can notice the following sequence
> 
> -ehci runtime suspends
> -system suspend triggered
> -ehci runtime resumes
> -ehci suspends (uses new wakeup settings)
> -system wakeup triggered
> -ehci resumes
> -ehci runtime suspends

This is because the root hub was runtime suspended with the wrong
wakeup setting.  The USB core, which is careful about these things,
resumed and re-suspended it with the proper wakeup setting.  In the 
process, the controller had to be runtime resumed as well.

Try doing the test over again, but this time with the root hub enabled 
for wakeup and the controller disabled.  (I know this is a bizarre 
combination, but try it anyway.)  Also, after the system wakes up, see 
whether the root hub and controller get runtime suspended.

> > For the third problem, suppose the controller was runtime suspended
> > when the system went to sleep.  When the system wakes up, the
> > controller will become active, so you have to inform the runtime PM
> > core about its change of state.  Basically, if omap_ehci_resume() sees
> > that ehci_resume() returned 0 then it must do:
> > 
> > 	pm_runtime_disable(dev);
> > 	pm_runtime_set_active(dev);
> > 	pm_runtime_enable(dev);
> > 
> > All of these issues are discussed (among lots of other material) in 
> > Documentation/power/runtime_pm.txt.
> 
> Is this still applicable? Documentation claims
> 
>    "During system resume it calls pm_runtime_enable() and pm_runtime_put_sync()
>     for every device right after executing the subsystem-level .resume_early()
>     callback and right after executing the subsystem-level .resume() callback
>     for it, respectively."

Yes, this is applicable, but it is irrelevant to the problem I 
described.  You still have to tell the runtime PM core that the device 
is now active.

Alan Stern

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Alan Stern]

On Thu, 11 Jul 2013, Roger Quadros wrote:

> > The other two problems are both related to the interaction between
> > system PM and runtime PM.  Suppose the controller is already runtime
> > suspended when the system goes to sleep.  Because it is runtime
> > suspended, it is enabled for wakeup.  But device_may_wakeup() could
> > return false; if this happens then you have to do a runtime-resume in
> > omap_ehci_suspend() before calling ehci_suspend(), so that the
> > controller can be suspended again with wakeups disabled.  (Or you could
> > choose an alternative method for accomplishing the same result, such as
> > disabling the wakeup signal from the pad without going through a whole
> > EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
> > true then you shouldn't do anything at all, because the controller is
> > already suspended with the correct wakeup setting.
> 
> I think this case is taken care of by the Runtime PM core at least for the OMAP
> platform according to the documentation
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647

No; that section refers only to races, not to wakeup settings.

> At the end of this mail is the log during system suspend/resume
> 
> You can notice the following sequence
> 
> -ehci runtime suspends
> -system suspend triggered
> -ehci runtime resumes
> -ehci suspends (uses new wakeup settings)
> -system wakeup triggered
> -ehci resumes
> -ehci runtime suspends

This is because the root hub was runtime suspended with the wrong
wakeup setting.  The USB core, which is careful about these things,
resumed and re-suspended it with the proper wakeup setting.  In the 
process, the controller had to be runtime resumed as well.

Try doing the test over again, but this time with the root hub enabled 
for wakeup and the controller disabled.  (I know this is a bizarre 
combination, but try it anyway.)  Also, after the system wakes up, see 
whether the root hub and controller get runtime suspended.

> > For the third problem, suppose the controller was runtime suspended
> > when the system went to sleep.  When the system wakes up, the
> > controller will become active, so you have to inform the runtime PM
> > core about its change of state.  Basically, if omap_ehci_resume() sees
> > that ehci_resume() returned 0 then it must do:
> > 
> > 	pm_runtime_disable(dev);
> > 	pm_runtime_set_active(dev);
> > 	pm_runtime_enable(dev);
> > 
> > All of these issues are discussed (among lots of other material) in 
> > Documentation/power/runtime_pm.txt.
> 
> Is this still applicable? Documentation claims
> 
>    "During system resume it calls pm_runtime_enable() and pm_runtime_put_sync()
>     for every device right after executing the subsystem-level .resume_early()
>     callback and right after executing the subsystem-level .resume() callback
>     for it, respectively."

Yes, this is applicable, but it is irrelevant to the problem I 
described.  You still have to tell the runtime PM core that the device 
is now active.

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

[PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Alan Stern]

On Thu, 11 Jul 2013, Roger Quadros wrote:

> > The other two problems are both related to the interaction between
> > system PM and runtime PM.  Suppose the controller is already runtime
> > suspended when the system goes to sleep.  Because it is runtime
> > suspended, it is enabled for wakeup.  But device_may_wakeup() could
> > return false; if this happens then you have to do a runtime-resume in
> > omap_ehci_suspend() before calling ehci_suspend(), so that the
> > controller can be suspended again with wakeups disabled.  (Or you could
> > choose an alternative method for accomplishing the same result, such as
> > disabling the wakeup signal from the pad without going through a whole
> > EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
> > true then you shouldn't do anything at all, because the controller is
> > already suspended with the correct wakeup setting.
> 
> I think this case is taken care of by the Runtime PM core at least for the OMAP
> platform according to the documentation
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647

No; that section refers only to races, not to wakeup settings.

> At the end of this mail is the log during system suspend/resume
> 
> You can notice the following sequence
> 
> -ehci runtime suspends
> -system suspend triggered
> -ehci runtime resumes
> -ehci suspends (uses new wakeup settings)
> -system wakeup triggered
> -ehci resumes
> -ehci runtime suspends

This is because the root hub was runtime suspended with the wrong
wakeup setting.  The USB core, which is careful about these things,
resumed and re-suspended it with the proper wakeup setting.  In the 
process, the controller had to be runtime resumed as well.

Try doing the test over again, but this time with the root hub enabled 
for wakeup and the controller disabled.  (I know this is a bizarre 
combination, but try it anyway.)  Also, after the system wakes up, see 
whether the root hub and controller get runtime suspended.

> > For the third problem, suppose the controller was runtime suspended
> > when the system went to sleep.  When the system wakes up, the
> > controller will become active, so you have to inform the runtime PM
> > core about its change of state.  Basically, if omap_ehci_resume() sees
> > that ehci_resume() returned 0 then it must do:
> > 
> > 	pm_runtime_disable(dev);
> > 	pm_runtime_set_active(dev);
> > 	pm_runtime_enable(dev);
> > 
> > All of these issues are discussed (among lots of other material) in 
> > Documentation/power/runtime_pm.txt.
> 
> Is this still applicable? Documentation claims
> 
>    "During system resume it calls pm_runtime_enable() and pm_runtime_put_sync()
>     for every device right after executing the subsystem-level .resume_early()
>     callback and right after executing the subsystem-level .resume() callback
>     for it, respectively."

Yes, this is applicable, but it is irrelevant to the problem I 
described.  You still have to tell the runtime PM core that the device 
is now active.

Alan Stern

Re: [PATCH 4/6] mfd: omap-usb-host: Put pins in IDLE state on suspend

[Kevin Hilman]

On 07/10/2013 05:23 PM, Roger Quadros wrote:
> In order to support wake up from suspend use the pinctrl
> framework to put the USB host pins in IDLE state during suspend.
> 
> CC: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>

[...]

> @@ -608,6 +618,14 @@ static int usbhs_omap_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> +	if (!dev->pins || !dev->pins->idle_state) {
> +		/* If IDLE pins are not available, we can't remote wakeup,
> +		 * so prevent idling in that case.
> +		 */

nit: multi-line comment style

Also, if there are no pins, aren't the pinctrl ops nops anyways?  IOW,
not sure the need
for this is clear, and it's not mentioned in the changelog.

Kevin

[PATCH 4/6] mfd: omap-usb-host: Put pins in IDLE state on suspend

[Kevin Hilman]

On 07/10/2013 05:23 PM, Roger Quadros wrote:
> In order to support wake up from suspend use the pinctrl
> framework to put the USB host pins in IDLE state during suspend.
> 
> CC: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>

[...]

> @@ -608,6 +618,14 @@ static int usbhs_omap_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> +	if (!dev->pins || !dev->pins->idle_state) {
> +		/* If IDLE pins are not available, we can't remote wakeup,
> +		 * so prevent idling in that case.
> +		 */

nit: multi-line comment style

Also, if there are no pins, aren't the pinctrl ops nops anyways?  IOW,
not sure the need
for this is clear, and it's not mentioned in the changelog.

Kevin

Re: [PATCH 4/6] mfd: omap-usb-host: Put pins in IDLE state on suspend

[Roger Quadros]

On 07/14/2013 04:22 PM, Kevin Hilman wrote:
> On 07/10/2013 05:23 PM, Roger Quadros wrote:
>> In order to support wake up from suspend use the pinctrl
>> framework to put the USB host pins in IDLE state during suspend.
>>
>> CC: Samuel Ortiz <sameo@linux.intel.com>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> [...]
> 
>> @@ -608,6 +618,14 @@ static int usbhs_omap_probe(struct platform_device *pdev)
>>  		return -ENOMEM;
>>  	}
>>  
>> +	if (!dev->pins || !dev->pins->idle_state) {
>> +		/* If IDLE pins are not available, we can't remote wakeup,
>> +		 * so prevent idling in that case.
>> +		 */
> 
> nit: multi-line comment style
> 
> Also, if there are no pins, aren't the pinctrl ops nops anyways?  IOW,
> not sure the need
> for this is clear, and it's not mentioned in the changelog.

The pinctrl ops are nops but the runtime suspend isn't. So in this case,
we'll never be able to wake up the USB controller.

I'll update the changelog to reflect this.

cheers,
-roger

Re: [PATCH 4/6] mfd: omap-usb-host: Put pins in IDLE state on suspend

[Roger Quadros]

On 07/14/2013 04:22 PM, Kevin Hilman wrote:
> On 07/10/2013 05:23 PM, Roger Quadros wrote:
>> In order to support wake up from suspend use the pinctrl
>> framework to put the USB host pins in IDLE state during suspend.
>>
>> CC: Samuel Ortiz <sameo@linux.intel.com>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> [...]
> 
>> @@ -608,6 +618,14 @@ static int usbhs_omap_probe(struct platform_device *pdev)
>>  		return -ENOMEM;
>>  	}
>>  
>> +	if (!dev->pins || !dev->pins->idle_state) {
>> +		/* If IDLE pins are not available, we can't remote wakeup,
>> +		 * so prevent idling in that case.
>> +		 */
> 
> nit: multi-line comment style
> 
> Also, if there are no pins, aren't the pinctrl ops nops anyways?  IOW,
> not sure the need
> for this is clear, and it's not mentioned in the changelog.

The pinctrl ops are nops but the runtime suspend isn't. So in this case,
we'll never be able to wake up the USB controller.

I'll update the changelog to reflect this.

cheers,
-roger

[PATCH 4/6] mfd: omap-usb-host: Put pins in IDLE state on suspend

[Roger Quadros]

On 07/14/2013 04:22 PM, Kevin Hilman wrote:
> On 07/10/2013 05:23 PM, Roger Quadros wrote:
>> In order to support wake up from suspend use the pinctrl
>> framework to put the USB host pins in IDLE state during suspend.
>>
>> CC: Samuel Ortiz <sameo@linux.intel.com>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> [...]
> 
>> @@ -608,6 +618,14 @@ static int usbhs_omap_probe(struct platform_device *pdev)
>>  		return -ENOMEM;
>>  	}
>>  
>> +	if (!dev->pins || !dev->pins->idle_state) {
>> +		/* If IDLE pins are not available, we can't remote wakeup,
>> +		 * so prevent idling in that case.
>> +		 */
> 
> nit: multi-line comment style
> 
> Also, if there are no pins, aren't the pinctrl ops nops anyways?  IOW,
> not sure the need
> for this is clear, and it's not mentioned in the changelog.

The pinctrl ops are nops but the runtime suspend isn't. So in this case,
we'll never be able to wake up the USB controller.

I'll update the changelog to reflect this.

cheers,
-roger

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros]

Hi Alan,

On 07/11/2013 06:14 PM, Alan Stern wrote:
> On Thu, 11 Jul 2013, Roger Quadros wrote:
> 
>>> The other two problems are both related to the interaction between
>>> system PM and runtime PM.  Suppose the controller is already runtime
>>> suspended when the system goes to sleep.  Because it is runtime
>>> suspended, it is enabled for wakeup.  But device_may_wakeup() could
>>> return false; if this happens then you have to do a runtime-resume in
>>> omap_ehci_suspend() before calling ehci_suspend(), so that the
>>> controller can be suspended again with wakeups disabled.  (Or you could
>>> choose an alternative method for accomplishing the same result, such as
>>> disabling the wakeup signal from the pad without going through a whole
>>> EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
>>> true then you shouldn't do anything at all, because the controller is
>>> already suspended with the correct wakeup setting.
>>
>> I think this case is taken care of by the Runtime PM core at least for the OMAP
>> platform according to the documentation
>>
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647
> 
> No; that section refers only to races, not to wakeup settings.
> 
>> At the end of this mail is the log during system suspend/resume
>>
>> You can notice the following sequence
>>
>> -ehci runtime suspends
>> -system suspend triggered
>> -ehci runtime resumes
>> -ehci suspends (uses new wakeup settings)
>> -system wakeup triggered
>> -ehci resumes
>> -ehci runtime suspends
> 
> This is because the root hub was runtime suspended with the wrong
> wakeup setting.  The USB core, which is careful about these things,
> resumed and re-suspended it with the proper wakeup setting.  In the 
> process, the controller had to be runtime resumed as well.
> 
> Try doing the test over again, but this time with the root hub enabled 
> for wakeup and the controller disabled.  (I know this is a bizarre 
> combination, but try it anyway.)  Also, after the system wakes up, see 
> whether the root hub and controller get runtime suspended.
> 

The first part of the test caught the problem were we were trying to access
EHCI registers when HW is not accessible. So this was a good test case.

>>> For the third problem, suppose the controller was runtime suspended
>>> when the system went to sleep.  When the system wakes up, the
>>> controller will become active, so you have to inform the runtime PM
>>> core about its change of state.  Basically, if omap_ehci_resume() sees
>>> that ehci_resume() returned 0 then it must do:
>>>
>>> 	pm_runtime_disable(dev);
>>> 	pm_runtime_set_active(dev);
>>> 	pm_runtime_enable(dev);
>>>
>>> All of these issues are discussed (among lots of other material) in 
>>> Documentation/power/runtime_pm.txt.
>>
>> Is this still applicable? Documentation claims
>>
>>    "During system resume it calls pm_runtime_enable() and pm_runtime_put_sync()
>>     for every device right after executing the subsystem-level .resume_early()
>>     callback and right after executing the subsystem-level .resume() callback
>>     for it, respectively."
> 
> Yes, this is applicable, but it is irrelevant to the problem I 
> described.  You still have to tell the runtime PM core that the device 
> is now active.

Right, I understand it now. How does the below code look?

+static int omap_ehci_suspend(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       bool do_wakeup = device_may_wakeup(dev);
+       int ret;
+
+       dev_dbg(dev, "%s may_wakeup %d\n", __func__, do_wakeup);
+
+       if (pm_runtime_status_suspended(dev)) {
+               pm_runtime_get_sync(dev);
+               ehci_resume(hcd, false);
+               ret = ehci_suspend(hcd, do_wakeup);
+               pm_runtime_put_sync(dev);
+
+       } else {
+               ret = ehci_suspend(hcd, do_wakeup);
+       }
+
+       return ret;
+}
+
+static int omap_ehci_resume(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       int ret;
+
+       dev_dbg(dev, "%s\n", __func__);
+
+       ret = ehci_resume(hcd, false);
+       if (!ret) {
+               /*
+                * Controller was powered ON so reflect state
+                */
+               pm_runtime_disable(dev);
+               pm_runtime_set_active(dev);
+               pm_runtime_enable(dev);
+       }
+
+       return ret;
+}
+
+static int omap_ehci_runtime_suspend(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+       dev_dbg(dev, "%s\n", __func__);
+
+       if (omap->bound)
+               ehci_suspend(hcd, true);
+
+       return 0;
+}
+
+static int omap_ehci_runtime_resume(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+       dev_dbg(dev, "%s\n", __func__);
+
+       if (omap->bound)
+               ehci_resume(hcd, false);
+
+       return 0;
+}


cheers,
-roger

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros]

Hi Alan,

On 07/11/2013 06:14 PM, Alan Stern wrote:
> On Thu, 11 Jul 2013, Roger Quadros wrote:
> 
>>> The other two problems are both related to the interaction between
>>> system PM and runtime PM.  Suppose the controller is already runtime
>>> suspended when the system goes to sleep.  Because it is runtime
>>> suspended, it is enabled for wakeup.  But device_may_wakeup() could
>>> return false; if this happens then you have to do a runtime-resume in
>>> omap_ehci_suspend() before calling ehci_suspend(), so that the
>>> controller can be suspended again with wakeups disabled.  (Or you could
>>> choose an alternative method for accomplishing the same result, such as
>>> disabling the wakeup signal from the pad without going through a whole
>>> EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
>>> true then you shouldn't do anything at all, because the controller is
>>> already suspended with the correct wakeup setting.
>>
>> I think this case is taken care of by the Runtime PM core at least for the OMAP
>> platform according to the documentation
>>
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647
> 
> No; that section refers only to races, not to wakeup settings.
> 
>> At the end of this mail is the log during system suspend/resume
>>
>> You can notice the following sequence
>>
>> -ehci runtime suspends
>> -system suspend triggered
>> -ehci runtime resumes
>> -ehci suspends (uses new wakeup settings)
>> -system wakeup triggered
>> -ehci resumes
>> -ehci runtime suspends
> 
> This is because the root hub was runtime suspended with the wrong
> wakeup setting.  The USB core, which is careful about these things,
> resumed and re-suspended it with the proper wakeup setting.  In the 
> process, the controller had to be runtime resumed as well.
> 
> Try doing the test over again, but this time with the root hub enabled 
> for wakeup and the controller disabled.  (I know this is a bizarre 
> combination, but try it anyway.)  Also, after the system wakes up, see 
> whether the root hub and controller get runtime suspended.
> 

The first part of the test caught the problem were we were trying to access
EHCI registers when HW is not accessible. So this was a good test case.

>>> For the third problem, suppose the controller was runtime suspended
>>> when the system went to sleep.  When the system wakes up, the
>>> controller will become active, so you have to inform the runtime PM
>>> core about its change of state.  Basically, if omap_ehci_resume() sees
>>> that ehci_resume() returned 0 then it must do:
>>>
>>> 	pm_runtime_disable(dev);
>>> 	pm_runtime_set_active(dev);
>>> 	pm_runtime_enable(dev);
>>>
>>> All of these issues are discussed (among lots of other material) in 
>>> Documentation/power/runtime_pm.txt.
>>
>> Is this still applicable? Documentation claims
>>
>>    "During system resume it calls pm_runtime_enable() and pm_runtime_put_sync()
>>     for every device right after executing the subsystem-level .resume_early()
>>     callback and right after executing the subsystem-level .resume() callback
>>     for it, respectively."
> 
> Yes, this is applicable, but it is irrelevant to the problem I 
> described.  You still have to tell the runtime PM core that the device 
> is now active.

Right, I understand it now. How does the below code look?

+static int omap_ehci_suspend(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       bool do_wakeup = device_may_wakeup(dev);
+       int ret;
+
+       dev_dbg(dev, "%s may_wakeup %d\n", __func__, do_wakeup);
+
+       if (pm_runtime_status_suspended(dev)) {
+               pm_runtime_get_sync(dev);
+               ehci_resume(hcd, false);
+               ret = ehci_suspend(hcd, do_wakeup);
+               pm_runtime_put_sync(dev);
+
+       } else {
+               ret = ehci_suspend(hcd, do_wakeup);
+       }
+
+       return ret;
+}
+
+static int omap_ehci_resume(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       int ret;
+
+       dev_dbg(dev, "%s\n", __func__);
+
+       ret = ehci_resume(hcd, false);
+       if (!ret) {
+               /*
+                * Controller was powered ON so reflect state
+                */
+               pm_runtime_disable(dev);
+               pm_runtime_set_active(dev);
+               pm_runtime_enable(dev);
+       }
+
+       return ret;
+}
+
+static int omap_ehci_runtime_suspend(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+       dev_dbg(dev, "%s\n", __func__);
+
+       if (omap->bound)
+               ehci_suspend(hcd, true);
+
+       return 0;
+}
+
+static int omap_ehci_runtime_resume(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+       dev_dbg(dev, "%s\n", __func__);
+
+       if (omap->bound)
+               ehci_resume(hcd, false);
+
+       return 0;
+}


cheers,
-roger

[PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros]

Hi Alan,

On 07/11/2013 06:14 PM, Alan Stern wrote:
> On Thu, 11 Jul 2013, Roger Quadros wrote:
> 
>>> The other two problems are both related to the interaction between
>>> system PM and runtime PM.  Suppose the controller is already runtime
>>> suspended when the system goes to sleep.  Because it is runtime
>>> suspended, it is enabled for wakeup.  But device_may_wakeup() could
>>> return false; if this happens then you have to do a runtime-resume in
>>> omap_ehci_suspend() before calling ehci_suspend(), so that the
>>> controller can be suspended again with wakeups disabled.  (Or you could
>>> choose an alternative method for accomplishing the same result, such as
>>> disabling the wakeup signal from the pad without going through a whole
>>> EHCI resume/suspend cycle.)  Conversely, if device_may_wakeup() returns
>>> true then you shouldn't do anything at all, because the controller is
>>> already suspended with the correct wakeup setting.
>>
>> I think this case is taken care of by the Runtime PM core at least for the OMAP
>> platform according to the documentation
>>
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647
> 
> No; that section refers only to races, not to wakeup settings.
> 
>> At the end of this mail is the log during system suspend/resume
>>
>> You can notice the following sequence
>>
>> -ehci runtime suspends
>> -system suspend triggered
>> -ehci runtime resumes
>> -ehci suspends (uses new wakeup settings)
>> -system wakeup triggered
>> -ehci resumes
>> -ehci runtime suspends
> 
> This is because the root hub was runtime suspended with the wrong
> wakeup setting.  The USB core, which is careful about these things,
> resumed and re-suspended it with the proper wakeup setting.  In the 
> process, the controller had to be runtime resumed as well.
> 
> Try doing the test over again, but this time with the root hub enabled 
> for wakeup and the controller disabled.  (I know this is a bizarre 
> combination, but try it anyway.)  Also, after the system wakes up, see 
> whether the root hub and controller get runtime suspended.
> 

The first part of the test caught the problem were we were trying to access
EHCI registers when HW is not accessible. So this was a good test case.

>>> For the third problem, suppose the controller was runtime suspended
>>> when the system went to sleep.  When the system wakes up, the
>>> controller will become active, so you have to inform the runtime PM
>>> core about its change of state.  Basically, if omap_ehci_resume() sees
>>> that ehci_resume() returned 0 then it must do:
>>>
>>> 	pm_runtime_disable(dev);
>>> 	pm_runtime_set_active(dev);
>>> 	pm_runtime_enable(dev);
>>>
>>> All of these issues are discussed (among lots of other material) in 
>>> Documentation/power/runtime_pm.txt.
>>
>> Is this still applicable? Documentation claims
>>
>>    "During system resume it calls pm_runtime_enable() and pm_runtime_put_sync()
>>     for every device right after executing the subsystem-level .resume_early()
>>     callback and right after executing the subsystem-level .resume() callback
>>     for it, respectively."
> 
> Yes, this is applicable, but it is irrelevant to the problem I 
> described.  You still have to tell the runtime PM core that the device 
> is now active.

Right, I understand it now. How does the below code look?

+static int omap_ehci_suspend(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       bool do_wakeup = device_may_wakeup(dev);
+       int ret;
+
+       dev_dbg(dev, "%s may_wakeup %d\n", __func__, do_wakeup);
+
+       if (pm_runtime_status_suspended(dev)) {
+               pm_runtime_get_sync(dev);
+               ehci_resume(hcd, false);
+               ret = ehci_suspend(hcd, do_wakeup);
+               pm_runtime_put_sync(dev);
+
+       } else {
+               ret = ehci_suspend(hcd, do_wakeup);
+       }
+
+       return ret;
+}
+
+static int omap_ehci_resume(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       int ret;
+
+       dev_dbg(dev, "%s\n", __func__);
+
+       ret = ehci_resume(hcd, false);
+       if (!ret) {
+               /*
+                * Controller was powered ON so reflect state
+                */
+               pm_runtime_disable(dev);
+               pm_runtime_set_active(dev);
+               pm_runtime_enable(dev);
+       }
+
+       return ret;
+}
+
+static int omap_ehci_runtime_suspend(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+       dev_dbg(dev, "%s\n", __func__);
+
+       if (omap->bound)
+               ehci_suspend(hcd, true);
+
+       return 0;
+}
+
+static int omap_ehci_runtime_resume(struct device *dev)
+{
+       struct usb_hcd *hcd = dev_get_drvdata(dev);
+       struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+       dev_dbg(dev, "%s\n", __func__);
+
+       if (omap->bound)
+               ehci_resume(hcd, false);
+
+       return 0;
+}


cheers,
-roger

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Alan Stern]

On Mon, 22 Jul 2013, Roger Quadros wrote:

> Right, I understand it now. How does the below code look?
> 
> +static int omap_ehci_suspend(struct device *dev)
> +{
> +       struct usb_hcd *hcd = dev_get_drvdata(dev);
> +       bool do_wakeup = device_may_wakeup(dev);
> +       int ret;
> +
> +       dev_dbg(dev, "%s may_wakeup %d\n", __func__, do_wakeup);
> +
> +       if (pm_runtime_status_suspended(dev)) {

You need to do this only when do_wakeup is false.

> +               pm_runtime_get_sync(dev);
> +               ehci_resume(hcd, false);
> +               ret = ehci_suspend(hcd, do_wakeup);
> +               pm_runtime_put_sync(dev);

It would be better to call pm_runtime_resume(dev) at the start instead
of pm_runtime_get_sync(), and eliminate the last two lines.  Then the
ehci_suspend() below could be moved outside the "if" statement.

Alternatively, if you are able to turn off the wakeup setting without
going through an entire resume/suspend cycle, that would be preferable.

> +
> +       } else {
> +               ret = ehci_suspend(hcd, do_wakeup);
> +       }
> +
> +       return ret;
> +}
> +
> +static int omap_ehci_resume(struct device *dev)
> +{
> +       struct usb_hcd *hcd = dev_get_drvdata(dev);
> +       int ret;
> +
> +       dev_dbg(dev, "%s\n", __func__);
> +
> +       ret = ehci_resume(hcd, false);
> +       if (!ret) {
> +               /*
> +                * Controller was powered ON so reflect state
> +                */
> +               pm_runtime_disable(dev);
> +               pm_runtime_set_active(dev);
> +               pm_runtime_enable(dev);
> +       }
> +
> +       return ret;
> +}

All good.

Alan Stern

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Alan Stern]

On Mon, 22 Jul 2013, Roger Quadros wrote:

> Right, I understand it now. How does the below code look?
> 
> +static int omap_ehci_suspend(struct device *dev)
> +{
> +       struct usb_hcd *hcd = dev_get_drvdata(dev);
> +       bool do_wakeup = device_may_wakeup(dev);
> +       int ret;
> +
> +       dev_dbg(dev, "%s may_wakeup %d\n", __func__, do_wakeup);
> +
> +       if (pm_runtime_status_suspended(dev)) {

You need to do this only when do_wakeup is false.

> +               pm_runtime_get_sync(dev);
> +               ehci_resume(hcd, false);
> +               ret = ehci_suspend(hcd, do_wakeup);
> +               pm_runtime_put_sync(dev);

It would be better to call pm_runtime_resume(dev) at the start instead
of pm_runtime_get_sync(), and eliminate the last two lines.  Then the
ehci_suspend() below could be moved outside the "if" statement.

Alternatively, if you are able to turn off the wakeup setting without
going through an entire resume/suspend cycle, that would be preferable.

> +
> +       } else {
> +               ret = ehci_suspend(hcd, do_wakeup);
> +       }
> +
> +       return ret;
> +}
> +
> +static int omap_ehci_resume(struct device *dev)
> +{
> +       struct usb_hcd *hcd = dev_get_drvdata(dev);
> +       int ret;
> +
> +       dev_dbg(dev, "%s\n", __func__);
> +
> +       ret = ehci_resume(hcd, false);
> +       if (!ret) {
> +               /*
> +                * Controller was powered ON so reflect state
> +                */
> +               pm_runtime_disable(dev);
> +               pm_runtime_set_active(dev);
> +               pm_runtime_enable(dev);
> +       }
> +
> +       return ret;
> +}

All good.

Alan Stern

[PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Alan Stern]

On Mon, 22 Jul 2013, Roger Quadros wrote:

> Right, I understand it now. How does the below code look?
> 
> +static int omap_ehci_suspend(struct device *dev)
> +{
> +       struct usb_hcd *hcd = dev_get_drvdata(dev);
> +       bool do_wakeup = device_may_wakeup(dev);
> +       int ret;
> +
> +       dev_dbg(dev, "%s may_wakeup %d\n", __func__, do_wakeup);
> +
> +       if (pm_runtime_status_suspended(dev)) {

You need to do this only when do_wakeup is false.

> +               pm_runtime_get_sync(dev);
> +               ehci_resume(hcd, false);
> +               ret = ehci_suspend(hcd, do_wakeup);
> +               pm_runtime_put_sync(dev);

It would be better to call pm_runtime_resume(dev) at the start instead
of pm_runtime_get_sync(), and eliminate the last two lines.  Then the
ehci_suspend() below could be moved outside the "if" statement.

Alternatively, if you are able to turn off the wakeup setting without
going through an entire resume/suspend cycle, that would be preferable.

> +
> +       } else {
> +               ret = ehci_suspend(hcd, do_wakeup);
> +       }
> +
> +       return ret;
> +}
> +
> +static int omap_ehci_resume(struct device *dev)
> +{
> +       struct usb_hcd *hcd = dev_get_drvdata(dev);
> +       int ret;
> +
> +       dev_dbg(dev, "%s\n", __func__);
> +
> +       ret = ehci_resume(hcd, false);
> +       if (!ret) {
> +               /*
> +                * Controller was powered ON so reflect state
> +                */
> +               pm_runtime_disable(dev);
> +               pm_runtime_set_active(dev);
> +               pm_runtime_enable(dev);
> +       }
> +
> +       return ret;
> +}

All good.

Alan Stern

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros]

On 07/22/2013 06:18 PM, Alan Stern wrote:
> On Mon, 22 Jul 2013, Roger Quadros wrote:
> 
>> Right, I understand it now. How does the below code look?
>>
>> +static int omap_ehci_suspend(struct device *dev)
>> +{
>> +       struct usb_hcd *hcd = dev_get_drvdata(dev);
>> +       bool do_wakeup = device_may_wakeup(dev);
>> +       int ret;
>> +
>> +       dev_dbg(dev, "%s may_wakeup %d\n", __func__, do_wakeup);
>> +
>> +       if (pm_runtime_status_suspended(dev)) {
> 
> You need to do this only when do_wakeup is false.

Right. Missed that.
> 
>> +               pm_runtime_get_sync(dev);
>> +               ehci_resume(hcd, false);
>> +               ret = ehci_suspend(hcd, do_wakeup);
>> +               pm_runtime_put_sync(dev);
> 
> It would be better to call pm_runtime_resume(dev) at the start instead
> of pm_runtime_get_sync(), and eliminate the last two lines.  Then the
> ehci_suspend() below could be moved outside the "if" statement.

Why do I find pm_runtime_* so confusing ;)

> 
> Alternatively, if you are able to turn off the wakeup setting without
> going through an entire resume/suspend cycle, that would be preferable.
> 

As we don't rely on the EHCI controller's interrupt any more after we
power it down, maybe ehci_resume/suspend cycle is not required at all on OMAP,
even if the wakeup setting is disabled during system suspend.

As the wakeup is controlled entirely by pad wakeup, all we need to do is make sure
the IO pad wakeup is configured correctly based on do_wakeup. How this is done
is still in transition but it might be turn out to be as simple as irq_set_irq_wake()

So IMHO, for ehci-omap this should suffice

static int omap_ehci_suspend(struct device *dev)
{
	struct usb_hcd *hcd = dev_get_drvdata(dev);
	bool do_wakeup = device_may_wakeup(dev);
	int ret = 0;

	if (!pm_runtime_status_suspended(dev))
		ret = ehci_suspend(hcd, do_wakeup);

	/* Not tested yet */
	irq_set_irq_wake(hcd->irq, do_wakeup);

	return ret;
}

What do you think?

As the OMAP IO pad wakeup management code [1] is still in transition, I'll wait till
that gets settled and then resend this series.

cheers,
-roger

[1] - http://thread.gmane.org/gmane.linux.ports.arm.omap/99010

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros]

On 07/22/2013 06:18 PM, Alan Stern wrote:
> On Mon, 22 Jul 2013, Roger Quadros wrote:
> 
>> Right, I understand it now. How does the below code look?
>>
>> +static int omap_ehci_suspend(struct device *dev)
>> +{
>> +       struct usb_hcd *hcd = dev_get_drvdata(dev);
>> +       bool do_wakeup = device_may_wakeup(dev);
>> +       int ret;
>> +
>> +       dev_dbg(dev, "%s may_wakeup %d\n", __func__, do_wakeup);
>> +
>> +       if (pm_runtime_status_suspended(dev)) {
> 
> You need to do this only when do_wakeup is false.

Right. Missed that.
> 
>> +               pm_runtime_get_sync(dev);
>> +               ehci_resume(hcd, false);
>> +               ret = ehci_suspend(hcd, do_wakeup);
>> +               pm_runtime_put_sync(dev);
> 
> It would be better to call pm_runtime_resume(dev) at the start instead
> of pm_runtime_get_sync(), and eliminate the last two lines.  Then the
> ehci_suspend() below could be moved outside the "if" statement.

Why do I find pm_runtime_* so confusing ;)

> 
> Alternatively, if you are able to turn off the wakeup setting without
> going through an entire resume/suspend cycle, that would be preferable.
> 

As we don't rely on the EHCI controller's interrupt any more after we
power it down, maybe ehci_resume/suspend cycle is not required at all on OMAP,
even if the wakeup setting is disabled during system suspend.

As the wakeup is controlled entirely by pad wakeup, all we need to do is make sure
the IO pad wakeup is configured correctly based on do_wakeup. How this is done
is still in transition but it might be turn out to be as simple as irq_set_irq_wake()

So IMHO, for ehci-omap this should suffice

static int omap_ehci_suspend(struct device *dev)
{
	struct usb_hcd *hcd = dev_get_drvdata(dev);
	bool do_wakeup = device_may_wakeup(dev);
	int ret = 0;

	if (!pm_runtime_status_suspended(dev))
		ret = ehci_suspend(hcd, do_wakeup);

	/* Not tested yet */
	irq_set_irq_wake(hcd->irq, do_wakeup);

	return ret;
}

What do you think?

As the OMAP IO pad wakeup management code [1] is still in transition, I'll wait till
that gets settled and then resend this series.

cheers,
-roger

[1] - http://thread.gmane.org/gmane.linux.ports.arm.omap/99010

[PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Roger Quadros]

On 07/22/2013 06:18 PM, Alan Stern wrote:
> On Mon, 22 Jul 2013, Roger Quadros wrote:
> 
>> Right, I understand it now. How does the below code look?
>>
>> +static int omap_ehci_suspend(struct device *dev)
>> +{
>> +       struct usb_hcd *hcd = dev_get_drvdata(dev);
>> +       bool do_wakeup = device_may_wakeup(dev);
>> +       int ret;
>> +
>> +       dev_dbg(dev, "%s may_wakeup %d\n", __func__, do_wakeup);
>> +
>> +       if (pm_runtime_status_suspended(dev)) {
> 
> You need to do this only when do_wakeup is false.

Right. Missed that.
> 
>> +               pm_runtime_get_sync(dev);
>> +               ehci_resume(hcd, false);
>> +               ret = ehci_suspend(hcd, do_wakeup);
>> +               pm_runtime_put_sync(dev);
> 
> It would be better to call pm_runtime_resume(dev) at the start instead
> of pm_runtime_get_sync(), and eliminate the last two lines.  Then the
> ehci_suspend() below could be moved outside the "if" statement.

Why do I find pm_runtime_* so confusing ;)

> 
> Alternatively, if you are able to turn off the wakeup setting without
> going through an entire resume/suspend cycle, that would be preferable.
> 

As we don't rely on the EHCI controller's interrupt any more after we
power it down, maybe ehci_resume/suspend cycle is not required at all on OMAP,
even if the wakeup setting is disabled during system suspend.

As the wakeup is controlled entirely by pad wakeup, all we need to do is make sure
the IO pad wakeup is configured correctly based on do_wakeup. How this is done
is still in transition but it might be turn out to be as simple as irq_set_irq_wake()

So IMHO, for ehci-omap this should suffice

static int omap_ehci_suspend(struct device *dev)
{
	struct usb_hcd *hcd = dev_get_drvdata(dev);
	bool do_wakeup = device_may_wakeup(dev);
	int ret = 0;

	if (!pm_runtime_status_suspended(dev))
		ret = ehci_suspend(hcd, do_wakeup);

	/* Not tested yet */
	irq_set_irq_wake(hcd->irq, do_wakeup);

	return ret;
}

What do you think?

As the OMAP IO pad wakeup management code [1] is still in transition, I'll wait till
that gets settled and then resend this series.

cheers,
-roger

[1] - http://thread.gmane.org/gmane.linux.ports.arm.omap/99010

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Alan Stern]

On Tue, 23 Jul 2013, Roger Quadros wrote:

> >> +               pm_runtime_get_sync(dev);
> >> +               ehci_resume(hcd, false);
> >> +               ret = ehci_suspend(hcd, do_wakeup);
> >> +               pm_runtime_put_sync(dev);
> > 
> > It would be better to call pm_runtime_resume(dev) at the start instead
> > of pm_runtime_get_sync(), and eliminate the last two lines.  Then the
> > ehci_suspend() below could be moved outside the "if" statement.
> 
> Why do I find pm_runtime_* so confusing ;)

It tries to provide every service anyone might want, and ends up being 
confusingly large.

In this case, though, the reasoning is simple.  We know that after the 
system resumes, the device will be back in the active state.  Hence 
there's no point in calling pm_runtime_put_sync here, other than to 
balance the _get_sync above.  Since there's no danger of another thread 
trying to do a runtime suspend at this point, you don't need to 
increment the usage counter.  Therefore pm_runtime_resume is good 
enough.

> > Alternatively, if you are able to turn off the wakeup setting without
> > going through an entire resume/suspend cycle, that would be preferable.
> > 
> 
> As we don't rely on the EHCI controller's interrupt any more after we
> power it down, maybe ehci_resume/suspend cycle is not required at all on OMAP,
> even if the wakeup setting is disabled during system suspend.
> 
> As the wakeup is controlled entirely by pad wakeup, all we need to do is make sure
> the IO pad wakeup is configured correctly based on do_wakeup. How this is done
> is still in transition but it might be turn out to be as simple as irq_set_irq_wake()
> 
> So IMHO, for ehci-omap this should suffice
> 
> static int omap_ehci_suspend(struct device *dev)
> {
> 	struct usb_hcd *hcd = dev_get_drvdata(dev);
> 	bool do_wakeup = device_may_wakeup(dev);
> 	int ret = 0;
> 
> 	if (!pm_runtime_status_suspended(dev))
> 		ret = ehci_suspend(hcd, do_wakeup);
> 
> 	/* Not tested yet */
> 	irq_set_irq_wake(hcd->irq, do_wakeup);
> 
> 	return ret;
> }
> 
> What do you think?

Nice and simple.  It looks good.

> As the OMAP IO pad wakeup management code [1] is still in transition, I'll wait till
> that gets settled and then resend this series.

Okay.

Alan Stern

Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Alan Stern]

On Tue, 23 Jul 2013, Roger Quadros wrote:

> >> +               pm_runtime_get_sync(dev);
> >> +               ehci_resume(hcd, false);
> >> +               ret = ehci_suspend(hcd, do_wakeup);
> >> +               pm_runtime_put_sync(dev);
> > 
> > It would be better to call pm_runtime_resume(dev) at the start instead
> > of pm_runtime_get_sync(), and eliminate the last two lines.  Then the
> > ehci_suspend() below could be moved outside the "if" statement.
> 
> Why do I find pm_runtime_* so confusing ;)

It tries to provide every service anyone might want, and ends up being 
confusingly large.

In this case, though, the reasoning is simple.  We know that after the 
system resumes, the device will be back in the active state.  Hence 
there's no point in calling pm_runtime_put_sync here, other than to 
balance the _get_sync above.  Since there's no danger of another thread 
trying to do a runtime suspend at this point, you don't need to 
increment the usage counter.  Therefore pm_runtime_resume is good 
enough.

> > Alternatively, if you are able to turn off the wakeup setting without
> > going through an entire resume/suspend cycle, that would be preferable.
> > 
> 
> As we don't rely on the EHCI controller's interrupt any more after we
> power it down, maybe ehci_resume/suspend cycle is not required at all on OMAP,
> even if the wakeup setting is disabled during system suspend.
> 
> As the wakeup is controlled entirely by pad wakeup, all we need to do is make sure
> the IO pad wakeup is configured correctly based on do_wakeup. How this is done
> is still in transition but it might be turn out to be as simple as irq_set_irq_wake()
> 
> So IMHO, for ehci-omap this should suffice
> 
> static int omap_ehci_suspend(struct device *dev)
> {
> 	struct usb_hcd *hcd = dev_get_drvdata(dev);
> 	bool do_wakeup = device_may_wakeup(dev);
> 	int ret = 0;
> 
> 	if (!pm_runtime_status_suspended(dev))
> 		ret = ehci_suspend(hcd, do_wakeup);
> 
> 	/* Not tested yet */
> 	irq_set_irq_wake(hcd->irq, do_wakeup);
> 
> 	return ret;
> }
> 
> What do you think?

Nice and simple.  It looks good.

> As the OMAP IO pad wakeup management code [1] is still in transition, I'll wait till
> that gets settled and then resend this series.

Okay.

Alan Stern

[PATCH 6/6] USB: ehci-omap: Implement suspend/resume

[Alan Stern]

On Tue, 23 Jul 2013, Roger Quadros wrote:

> >> +               pm_runtime_get_sync(dev);
> >> +               ehci_resume(hcd, false);
> >> +               ret = ehci_suspend(hcd, do_wakeup);
> >> +               pm_runtime_put_sync(dev);
> > 
> > It would be better to call pm_runtime_resume(dev) at the start instead
> > of pm_runtime_get_sync(), and eliminate the last two lines.  Then the
> > ehci_suspend() below could be moved outside the "if" statement.
> 
> Why do I find pm_runtime_* so confusing ;)

It tries to provide every service anyone might want, and ends up being 
confusingly large.

In this case, though, the reasoning is simple.  We know that after the 
system resumes, the device will be back in the active state.  Hence 
there's no point in calling pm_runtime_put_sync here, other than to 
balance the _get_sync above.  Since there's no danger of another thread 
trying to do a runtime suspend at this point, you don't need to 
increment the usage counter.  Therefore pm_runtime_resume is good 
enough.

> > Alternatively, if you are able to turn off the wakeup setting without
> > going through an entire resume/suspend cycle, that would be preferable.
> > 
> 
> As we don't rely on the EHCI controller's interrupt any more after we
> power it down, maybe ehci_resume/suspend cycle is not required at all on OMAP,
> even if the wakeup setting is disabled during system suspend.
> 
> As the wakeup is controlled entirely by pad wakeup, all we need to do is make sure
> the IO pad wakeup is configured correctly based on do_wakeup. How this is done
> is still in transition but it might be turn out to be as simple as irq_set_irq_wake()
> 
> So IMHO, for ehci-omap this should suffice
> 
> static int omap_ehci_suspend(struct device *dev)
> {
> 	struct usb_hcd *hcd = dev_get_drvdata(dev);
> 	bool do_wakeup = device_may_wakeup(dev);
> 	int ret = 0;
> 
> 	if (!pm_runtime_status_suspended(dev))
> 		ret = ehci_suspend(hcd, do_wakeup);
> 
> 	/* Not tested yet */
> 	irq_set_irq_wake(hcd->irq, do_wakeup);
> 
> 	return ret;
> }
> 
> What do you think?

Nice and simple.  It looks good.

> As the OMAP IO pad wakeup management code [1] is still in transition, I'll wait till
> that gets settled and then resend this series.

Okay.

Alan Stern