[PATCH V2 0/6] USB: make ehci-tegra its own module

[PATCH V2 0/6] USB: make ehci-tegra its own module

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

ehci-tegra is currently built into the main ehci-hcd driver, rather than
being a separate module. This causes issues with multi-platform ARM
kernels. This series separates ehci-tegra into its own module to avoid
those problems.

Manjunath Goudar originally wrote most of this series. I've since cleaned
it up, rebased it on Venu's recent changes to the Tegra USB driver, and
tested it.

Note that I expect this series will need to go through the USB PHY tree,
since it builds upon Venu's recent changes to the Tegra USB driver, which
were applied there.

Manjunath Goudar (3):
  usb: phy: export ulpi_viewport_access_ops
  USB: EHCI: export ehci_handshake for ehci-hcd sub-drivers
  USB: EHCI: make ehci-tegra a separate driver

Stephen Warren (3):
  usb: phy: add MODULE_LICENSE to phy-tegra-usb.c
  USB: EHCI: tegra: remove all power management
  USB: EHCI: tegra: fix circular module dependencies

 drivers/usb/host/Kconfig            |   2 +-
 drivers/usb/host/Makefile           |   1 +
 drivers/usb/host/ehci-hcd.c         |  22 +-
 drivers/usb/host/ehci-hub.c         |   4 +-
 drivers/usb/host/ehci-tegra.c       | 397 +++++++-----------------------------
 drivers/usb/host/ehci.h             |   2 +
 drivers/usb/phy/phy-tegra-usb.c     |  43 +++-
 drivers/usb/phy/phy-ulpi-viewport.c |   2 +
 include/linux/usb/tegra_usb_phy.h   |   4 -
 9 files changed, 131 insertions(+), 346 deletions(-)

-- 
1.8.1.5

[PATCH V2 1/6] usb: phy: export ulpi_viewport_access_ops

[Stephen Warren]

From: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

In order to build a ULPI PHY driver as a module, this symbol needs to
be exported.

Signed-off-by: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
[swarren, reworked Manjunath's patches to split them more logically]
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2: Include export.h not module.h
---
 drivers/usb/phy/phy-ulpi-viewport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/phy/phy-ulpi-viewport.c b/drivers/usb/phy/phy-ulpi-viewport.c
index c5ba7e5..7c22a539 100644
--- a/drivers/usb/phy/phy-ulpi-viewport.c
+++ b/drivers/usb/phy/phy-ulpi-viewport.c
@@ -12,6 +12,7 @@
  *
  */
 
+#include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/usb.h>
 #include <linux/io.h>
@@ -78,3 +79,4 @@ struct usb_phy_io_ops ulpi_viewport_access_ops = {
 	.read	= ulpi_viewport_read,
 	.write	= ulpi_viewport_write,
 };
+EXPORT_SYMBOL_GPL(ulpi_viewport_access_ops);
-- 
1.8.1.5

[PATCH V2 2/6] usb: phy: add MODULE_LICENSE to phy-tegra-usb.c

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

When this file is built as a module, it needs a MODULE_LICENSE in order
to access many exported symbols.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2: No change.
---
 drivers/usb/phy/phy-tegra-usb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index 5d9af11..f0727f2 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/export.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
@@ -869,3 +870,6 @@ struct usb_phy *tegra_usb_get_phy(struct device_node *dn)
 	return &tegra_phy->u_phy;
 }
 EXPORT_SYMBOL_GPL(tegra_usb_get_phy);
+
+MODULE_DESCRIPTION("Tegra USB PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.1.5

--
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 V2 3/6] USB: EHCI: export ehci_handshake for ehci-hcd sub-drivers

[Stephen Warren]

From: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

In order to split ehci-hcd.c into separate modules, handshake() must be
exported. Rename the symbol to add an ehci_ prefix, to avoid any naming
clashes.

Signed-off-by: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
[swarren, split Manjunath's patches more logically, limit this change
to export just handshake()]
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2: Only export handshake(), not reset/tdi_reset/halt.
---
 drivers/usb/host/ehci-hcd.c   | 17 ++++++++++-------
 drivers/usb/host/ehci-hub.c   |  4 ++--
 drivers/usb/host/ehci-tegra.c | 12 ++++++------
 drivers/usb/host/ehci.h       |  2 ++
 4 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 246e124..e8a6f3d 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -139,7 +139,7 @@ static inline unsigned ehci_read_frame_index(struct ehci_hcd *ehci)
 /*-------------------------------------------------------------------------*/
 
 /*
- * handshake - spin reading hc until handshake completes or fails
+ * ehci_handshake - spin reading hc until handshake completes or fails
  * @ptr: address of hc register to be read
  * @mask: bits to look at in result of read
  * @done: value of those bits when handshake succeeds
@@ -155,8 +155,8 @@ static inline unsigned ehci_read_frame_index(struct ehci_hcd *ehci)
  * before driver shutdown. But it also seems to be caused by bugs in cardbus
  * bridge shutdown:  shutting down the bridge before the devices using it.
  */
-static int handshake (struct ehci_hcd *ehci, void __iomem *ptr,
-		      u32 mask, u32 done, int usec)
+int ehci_handshake(struct ehci_hcd *ehci, void __iomem *ptr,
+		   u32 mask, u32 done, int usec)
 {
 	u32	result;
 
@@ -172,6 +172,7 @@ static int handshake (struct ehci_hcd *ehci, void __iomem *ptr,
 	} while (usec > 0);
 	return -ETIMEDOUT;
 }
+EXPORT_SYMBOL_GPL(ehci_handshake);
 
 /* check TDI/ARC silicon is in host mode */
 static int tdi_in_host_mode (struct ehci_hcd *ehci)
@@ -212,7 +213,7 @@ static int ehci_halt (struct ehci_hcd *ehci)
 	spin_unlock_irq(&ehci->lock);
 	synchronize_irq(ehci_to_hcd(ehci)->irq);
 
-	return handshake(ehci, &ehci->regs->status,
+	return ehci_handshake(ehci, &ehci->regs->status,
 			  STS_HALT, STS_HALT, 16 * 125);
 }
 
@@ -251,7 +252,7 @@ static int ehci_reset (struct ehci_hcd *ehci)
 	ehci_writel(ehci, command, &ehci->regs->command);
 	ehci->rh_state = EHCI_RH_HALTED;
 	ehci->next_statechange = jiffies;
-	retval = handshake (ehci, &ehci->regs->command,
+	retval = ehci_handshake(ehci, &ehci->regs->command,
 			    CMD_RESET, 0, 250 * 1000);
 
 	if (ehci->has_hostpc) {
@@ -286,7 +287,8 @@ static void ehci_quiesce (struct ehci_hcd *ehci)
 
 	/* wait for any schedule enables/disables to take effect */
 	temp = (ehci->command << 10) & (STS_ASS | STS_PSS);
-	handshake(ehci, &ehci->regs->status, STS_ASS | STS_PSS, temp, 16 * 125);
+	ehci_handshake(ehci, &ehci->regs->status, STS_ASS | STS_PSS, temp,
+			16 * 125);
 
 	/* then disable anything that's still active */
 	spin_lock_irq(&ehci->lock);
@@ -295,7 +297,8 @@ static void ehci_quiesce (struct ehci_hcd *ehci)
 	spin_unlock_irq(&ehci->lock);
 
 	/* hardware can take 16 microframes to turn off ... */
-	handshake(ehci, &ehci->regs->status, STS_ASS | STS_PSS, 0, 16 * 125);
+	ehci_handshake(ehci, &ehci->regs->status, STS_ASS | STS_PSS, 0,
+			16 * 125);
 }
 
 /*-------------------------------------------------------------------------*/
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index b2f6450..2b70277 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -892,7 +892,7 @@ static int ehci_hub_control (
 						PORT_SUSPEND | PORT_RESUME);
 				ehci_writel(ehci, temp, status_reg);
 				clear_bit(wIndex, &ehci->resuming_ports);
-				retval = handshake(ehci, status_reg,
+				retval = ehci_handshake(ehci, status_reg,
 					   PORT_RESUME, 0, 2000 /* 2msec */);
 				if (retval != 0) {
 					ehci_err(ehci,
@@ -918,7 +918,7 @@ static int ehci_hub_control (
 			/* REVISIT:  some hardware needs 550+ usec to clear
 			 * this bit; seems too long to spin routinely...
 			 */
-			retval = handshake(ehci, status_reg,
+			retval = ehci_handshake(ehci, status_reg,
 					PORT_RESET, 0, 1000);
 			if (retval != 0) {
 				ehci_err (ehci, "port %d reset error %d\n",
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 8390c87..289b9b8 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -179,7 +179,7 @@ static int tegra_ehci_hub_control(
 		 * If a transaction is in progress, there may be a delay in
 		 * suspending the port. Poll until the port is suspended.
 		 */
-		if (handshake(ehci, status_reg, PORT_SUSPEND,
+		if (ehci_handshake(ehci, status_reg, PORT_SUSPEND,
 						PORT_SUSPEND, 5000))
 			pr_err("%s: timeout waiting for SUSPEND\n", __func__);
 
@@ -227,9 +227,9 @@ static int tegra_ehci_hub_control(
 		spin_lock_irqsave(&ehci->lock, flags);
 
 		/* Poll until the controller clears RESUME and SUSPEND */
-		if (handshake(ehci, status_reg, PORT_RESUME, 0, 2000))
+		if (ehci_handshake(ehci, status_reg, PORT_RESUME, 0, 2000))
 			pr_err("%s: timeout waiting for RESUME\n", __func__);
-		if (handshake(ehci, status_reg, PORT_SUSPEND, 0, 2000))
+		if (ehci_handshake(ehci, status_reg, PORT_SUSPEND, 0, 2000))
 			pr_err("%s: timeout waiting for SUSPEND\n", __func__);
 
 		ehci->reset_done[wIndex-1] = 0;
@@ -511,14 +511,14 @@ static int controller_resume(struct device *dev)
 	}
 
 	/* Poll until CCS is enabled */
-	if (handshake(ehci, &hw->port_status[0], PORT_CONNECT,
+	if (ehci_handshake(ehci, &hw->port_status[0], PORT_CONNECT,
 						 PORT_CONNECT, 2000)) {
 		pr_err("%s: timeout waiting for PORT_CONNECT\n", __func__);
 		goto restart;
 	}
 
 	/* Poll until PE is enabled */
-	if (handshake(ehci, &hw->port_status[0], PORT_PE,
+	if (ehci_handshake(ehci, &hw->port_status[0], PORT_PE,
 						 PORT_PE, 2000)) {
 		pr_err("%s: timeout waiting for USB_PORTSC1_PE\n", __func__);
 		goto restart;
@@ -536,7 +536,7 @@ static int controller_resume(struct device *dev)
 		writel(val, &hw->port_status[0]);
 
 		/* Wait until port suspend completes */
-		if (handshake(ehci, &hw->port_status[0], PORT_SUSPEND,
+		if (ehci_handshake(ehci, &hw->port_status[0], PORT_SUSPEND,
 							 PORT_SUSPEND, 1000)) {
 			pr_err("%s: timeout waiting for PORT_SUSPEND\n",
 								__func__);
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 7c978b2..64f9a08 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -800,6 +800,8 @@ struct ehci_driver_overrides {
 extern void	ehci_init_driver(struct hc_driver *drv,
 				const struct ehci_driver_overrides *over);
 extern int	ehci_setup(struct usb_hcd *hcd);
+extern int	ehci_handshake(struct ehci_hcd *ehci, void __iomem *ptr,
+				u32 mask, u32 done, int usec);
 
 #ifdef CONFIG_PM
 extern int	ehci_suspend(struct usb_hcd *hcd, bool do_wakeup);
-- 
1.8.1.5

--
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 V2 4/6] USB: EHCI: tegra: remove all power management

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The PM routines in ehci-tegra.c use APIs such as ehci_reset(),
ehci_halt(), and ehci_tdi_reset() that would need to be exported to
convert ehci-tegra.c into a separate module from ehci-hcd.c. However,
we'd prefer not to export them.

Instead, simply remove all power management functionality. Runtime PM
was disabled since it didn't work correctly, and system suspend isn't
yet supported in a meaningful way. So, this change doesn't lose any
functionality.

Hopefully the power management logic can be reimplemented in a cleaner
way in the future.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
NOTE: This could do with a little more testing on a few more boards. If
Alan is OK with this series, I'll go through and do that, and let you
know when I've tested it enough to be applied. I saw no issues on the one
board I tested with though.

I'd appreciate any hints if this patch is actually removing more
functionality than I think, and is going to break something horribly.

v2: New patch.
---
 drivers/usb/host/ehci-tegra.c | 225 ------------------------------------------
 1 file changed, 225 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 289b9b8..8f42b3a 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -61,15 +61,6 @@ static void tegra_ehci_power_up(struct usb_hcd *hcd)
 	tegra->host_resumed = 1;
 }
 
-static void tegra_ehci_power_down(struct usb_hcd *hcd)
-{
-	struct tegra_ehci_hcd *tegra = dev_get_drvdata(hcd->self.controller);
-
-	tegra->host_resumed = 0;
-	usb_phy_set_suspend(hcd->phy, 1);
-	clk_disable_unprepare(tegra->clk);
-}
-
 static int tegra_ehci_internal_port_reset(
 	struct ehci_hcd	*ehci,
 	u32 __iomem	*portsc_reg
@@ -248,27 +239,6 @@ done:
 	return retval;
 }
 
-static void tegra_ehci_restart(struct usb_hcd *hcd)
-{
-	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
-
-	ehci_reset(ehci);
-
-	/* setup the frame list and Async q heads */
-	ehci_writel(ehci, ehci->periodic_dma, &ehci->regs->frame_list);
-	ehci_writel(ehci, (u32)ehci->async->qh_dma, &ehci->regs->async_next);
-	/* setup the command register and set the controller in RUN mode */
-	ehci->command &= ~(CMD_LRESET|CMD_IAAD|CMD_PSE|CMD_ASE|CMD_RESET);
-	ehci->command |= CMD_RUN;
-	ehci_writel(ehci, ehci->command, &ehci->regs->command);
-
-	down_write(&ehci_cf_port_reset_rwsem);
-	ehci_writel(ehci, FLAG_CF, &ehci->regs->configured_flag);
-	/* flush posted writes */
-	ehci_readl(ehci, &ehci->regs->command);
-	up_write(&ehci_cf_port_reset_rwsem);
-}
-
 static void tegra_ehci_shutdown(struct usb_hcd *hcd)
 {
 	struct tegra_ehci_hcd *tegra = dev_get_drvdata(hcd->self.controller);
@@ -399,10 +369,6 @@ static const struct hc_driver tegra_ehci_hc_driver = {
 	.map_urb_for_dma	= tegra_ehci_map_urb_for_dma,
 	.unmap_urb_for_dma	= tegra_ehci_unmap_urb_for_dma,
 	.hub_control		= tegra_ehci_hub_control,
-#ifdef CONFIG_PM
-	.bus_suspend		= ehci_bus_suspend,
-	.bus_resume		= ehci_bus_resume,
-#endif
 };
 
 static int setup_vbus_gpio(struct platform_device *pdev,
@@ -432,182 +398,6 @@ static int setup_vbus_gpio(struct platform_device *pdev,
 	return err;
 }
 
-#ifdef CONFIG_PM
-
-static int controller_suspend(struct device *dev)
-{
-	struct tegra_ehci_hcd *tegra =
-			platform_get_drvdata(to_platform_device(dev));
-	struct ehci_hcd	*ehci = tegra->ehci;
-	struct usb_hcd *hcd = ehci_to_hcd(ehci);
-	struct ehci_regs __iomem *hw = ehci->regs;
-	unsigned long flags;
-
-	if (time_before(jiffies, ehci->next_statechange))
-		msleep(10);
-
-	ehci_halt(ehci);
-
-	spin_lock_irqsave(&ehci->lock, flags);
-	tegra->port_speed = (readl(&hw->port_status[0]) >> 26) & 0x3;
-	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
-	spin_unlock_irqrestore(&ehci->lock, flags);
-
-	tegra_ehci_power_down(hcd);
-	return 0;
-}
-
-static int controller_resume(struct device *dev)
-{
-	struct tegra_ehci_hcd *tegra =
-			platform_get_drvdata(to_platform_device(dev));
-	struct ehci_hcd	*ehci = tegra->ehci;
-	struct usb_hcd *hcd = ehci_to_hcd(ehci);
-	struct ehci_regs __iomem *hw = ehci->regs;
-	unsigned long val;
-
-	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
-	tegra_ehci_power_up(hcd);
-
-	if (tegra->port_speed > TEGRA_USB_PHY_PORT_SPEED_HIGH) {
-		/* Wait for the phy to detect new devices
-		 * before we restart the controller */
-		msleep(10);
-		goto restart;
-	}
-
-	/* Force the phy to keep data lines in suspend state */
-	tegra_ehci_phy_restore_start(hcd->phy, tegra->port_speed);
-
-	/* Enable host mode */
-	tdi_reset(ehci);
-
-	/* Enable Port Power */
-	val = readl(&hw->port_status[0]);
-	val |= PORT_POWER;
-	writel(val, &hw->port_status[0]);
-	udelay(10);
-
-	/* Check if the phy resume from LP0. When the phy resume from LP0
-	 * USB register will be reset. */
-	if (!readl(&hw->async_next)) {
-		/* Program the field PTC based on the saved speed mode */
-		val = readl(&hw->port_status[0]);
-		val &= ~PORT_TEST(~0);
-		if (tegra->port_speed == TEGRA_USB_PHY_PORT_SPEED_HIGH)
-			val |= PORT_TEST_FORCE;
-		else if (tegra->port_speed == TEGRA_USB_PHY_PORT_SPEED_FULL)
-			val |= PORT_TEST(6);
-		else if (tegra->port_speed == TEGRA_USB_PHY_PORT_SPEED_LOW)
-			val |= PORT_TEST(7);
-		writel(val, &hw->port_status[0]);
-		udelay(10);
-
-		/* Disable test mode by setting PTC field to NORMAL_OP */
-		val = readl(&hw->port_status[0]);
-		val &= ~PORT_TEST(~0);
-		writel(val, &hw->port_status[0]);
-		udelay(10);
-	}
-
-	/* Poll until CCS is enabled */
-	if (ehci_handshake(ehci, &hw->port_status[0], PORT_CONNECT,
-						 PORT_CONNECT, 2000)) {
-		pr_err("%s: timeout waiting for PORT_CONNECT\n", __func__);
-		goto restart;
-	}
-
-	/* Poll until PE is enabled */
-	if (ehci_handshake(ehci, &hw->port_status[0], PORT_PE,
-						 PORT_PE, 2000)) {
-		pr_err("%s: timeout waiting for USB_PORTSC1_PE\n", __func__);
-		goto restart;
-	}
-
-	/* Clear the PCI status, to avoid an interrupt taken upon resume */
-	val = readl(&hw->status);
-	val |= STS_PCD;
-	writel(val, &hw->status);
-
-	/* Put controller in suspend mode by writing 1 to SUSP bit of PORTSC */
-	val = readl(&hw->port_status[0]);
-	if ((val & PORT_POWER) && (val & PORT_PE)) {
-		val |= PORT_SUSPEND;
-		writel(val, &hw->port_status[0]);
-
-		/* Wait until port suspend completes */
-		if (ehci_handshake(ehci, &hw->port_status[0], PORT_SUSPEND,
-							 PORT_SUSPEND, 1000)) {
-			pr_err("%s: timeout waiting for PORT_SUSPEND\n",
-								__func__);
-			goto restart;
-		}
-	}
-
-	tegra_ehci_phy_restore_end(hcd->phy);
-	goto done;
-
- restart:
-	if (tegra->port_speed <= TEGRA_USB_PHY_PORT_SPEED_HIGH)
-		tegra_ehci_phy_restore_end(hcd->phy);
-
-	tegra_ehci_restart(hcd);
-
- done:
-	tegra_usb_phy_preresume(hcd->phy);
-	tegra->port_resuming = 1;
-	return 0;
-}
-
-static int tegra_ehci_suspend(struct device *dev)
-{
-	struct tegra_ehci_hcd *tegra =
-			platform_get_drvdata(to_platform_device(dev));
-	struct usb_hcd *hcd = ehci_to_hcd(tegra->ehci);
-	int rc = 0;
-
-	/*
-	 * When system sleep is supported and USB controller wakeup is
-	 * implemented: If the controller is runtime-suspended and the
-	 * wakeup setting needs to be changed, call pm_runtime_resume().
-	 */
-	if (HCD_HW_ACCESSIBLE(hcd))
-		rc = controller_suspend(dev);
-	return rc;
-}
-
-static int tegra_ehci_resume(struct device *dev)
-{
-	int rc;
-
-	rc = controller_resume(dev);
-	if (rc == 0) {
-		pm_runtime_disable(dev);
-		pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
-	}
-	return rc;
-}
-
-static int tegra_ehci_runtime_suspend(struct device *dev)
-{
-	return controller_suspend(dev);
-}
-
-static int tegra_ehci_runtime_resume(struct device *dev)
-{
-	return controller_resume(dev);
-}
-
-static const struct dev_pm_ops tegra_ehci_pm_ops = {
-	.suspend	= tegra_ehci_suspend,
-	.resume		= tegra_ehci_resume,
-	.runtime_suspend = tegra_ehci_runtime_suspend,
-	.runtime_resume	= tegra_ehci_runtime_resume,
-};
-
-#endif
-
 /* Bits of PORTSC1, which will get cleared by writing 1 into them */
 #define TEGRA_PORTSC1_RWC_BITS (PORT_CSC | PORT_PEC | PORT_OCC)
 
@@ -773,14 +563,6 @@ static int tegra_ehci_probe(struct platform_device *pdev)
 		goto cleanup_phy;
 	}
 
-	pm_runtime_set_active(&pdev->dev);
-	pm_runtime_get_noresume(&pdev->dev);
-
-	/* Don't skip the pm_runtime_forbid call if wakeup isn't working */
-	/* if (!pdata->power_down_on_bus_suspend) */
-		pm_runtime_forbid(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
-	pm_runtime_put_sync(&pdev->dev);
 	return err;
 
 cleanup_phy:
@@ -800,10 +582,6 @@ static int tegra_ehci_remove(struct platform_device *pdev)
 	struct tegra_ehci_hcd *tegra = platform_get_drvdata(pdev);
 	struct usb_hcd *hcd = ehci_to_hcd(tegra->ehci);
 
-	pm_runtime_get_sync(&pdev->dev);
-	pm_runtime_disable(&pdev->dev);
-	pm_runtime_put_noidle(&pdev->dev);
-
 	if (!IS_ERR(tegra->transceiver))
 		otg_set_host(tegra->transceiver->otg, NULL);
 
@@ -837,8 +615,5 @@ static struct platform_driver tegra_ehci_driver = {
 	.driver		= {
 		.name	= "tegra-ehci",
 		.of_match_table = tegra_ehci_of_match,
-#ifdef CONFIG_PM
-		.pm	= &tegra_ehci_pm_ops,
-#endif
 	}
 };
-- 
1.8.1.5

[PATCH V2 5/6] USB: EHCI: tegra: fix circular module dependencies

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The Tegra EHCI driver directly calls various functions in the Tegra USB
PHY driver. The reverse is also true; the PHY driver calls into the EHCI
driver. This is problematic when the two are built as modules.

The calls from the PHY to EHCI driver were originally added in commit
bbdabdb "usb: add APIs to access host registers from Tegra PHY", for the
following reasons:

1) The register being touched is an EHCI register, so logically only the
   EHCI driver should touch it.
2) (1) implies that some locking may be needed to correctly implement the
   r/m/w access to this shared register.
3) We were expecting to pass only the PHY register space to the Tegra PHY
   driver, and hence it would not have access to touch the shared
   registers.

To solve this, that commit added functions in the EHCI driver to touch the
shared register on behalf of the PHY driver.

In practice, we ended up not having any locking in the implementaiton of
those functions, and I've been led to believe this is safe. Equally, (3)
did not happen either. Hence, it is possible for the PHY driver to touch
the shared register directly.

Given that, this patch moves the code to touch the shared register back
into the PHY driver, to eliminate the module problems. If we actually
need locking or co-ordination in the future, I propose we put the lock
support into some pre-existing core module, or into a third separate
module, in order to avoid the circular dependencies.

I apologize for my contribution to code churn here.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2: No change; just rebased on new versions of earlier patches.
---
 drivers/usb/host/ehci-tegra.c     | 36 ------------------------------------
 drivers/usb/phy/phy-tegra-usb.c   | 39 ++++++++++++++++++++++++++++++++++++---
 include/linux/usb/tegra_usb_phy.h |  4 ----
 3 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 8f42b3a..c8dc687 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -34,11 +34,6 @@
 #define TEGRA_USB2_BASE			0xC5004000
 #define TEGRA_USB3_BASE			0xC5008000
 
-/* PORTSC registers */
-#define TEGRA_USB_PORTSC1			0x184
-#define TEGRA_USB_PORTSC1_PTS(x)	(((x) & 0x3) << 30)
-#define TEGRA_USB_PORTSC1_PHCD	(1 << 23)
-
 #define TEGRA_USB_DMA_ALIGN 32
 
 struct tegra_ehci_hcd {
@@ -398,37 +393,6 @@ static int setup_vbus_gpio(struct platform_device *pdev,
 	return err;
 }
 
-/* Bits of PORTSC1, which will get cleared by writing 1 into them */
-#define TEGRA_PORTSC1_RWC_BITS (PORT_CSC | PORT_PEC | PORT_OCC)
-
-void tegra_ehci_set_pts(struct usb_phy *x, u8 pts_val)
-{
-	unsigned long val;
-	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
-	void __iomem *base = hcd->regs;
-
-	val = readl(base + TEGRA_USB_PORTSC1) & ~TEGRA_PORTSC1_RWC_BITS;
-	val &= ~TEGRA_USB_PORTSC1_PTS(3);
-	val |= TEGRA_USB_PORTSC1_PTS(pts_val & 3);
-	writel(val, base + TEGRA_USB_PORTSC1);
-}
-EXPORT_SYMBOL_GPL(tegra_ehci_set_pts);
-
-void tegra_ehci_set_phcd(struct usb_phy *x, bool enable)
-{
-	unsigned long val;
-	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
-	void __iomem *base = hcd->regs;
-
-	val = readl(base + TEGRA_USB_PORTSC1) & ~TEGRA_PORTSC1_RWC_BITS;
-	if (enable)
-		val |= TEGRA_USB_PORTSC1_PHCD;
-	else
-		val &= ~TEGRA_USB_PORTSC1_PHCD;
-	writel(val, base + TEGRA_USB_PORTSC1);
-}
-EXPORT_SYMBOL_GPL(tegra_ehci_set_phcd);
-
 static int tegra_ehci_probe(struct platform_device *pdev)
 {
 	struct resource *res;
diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index f0727f2..3446245 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -32,11 +32,20 @@
 #include <linux/usb/otg.h>
 #include <linux/usb/ulpi.h>
 #include <asm/mach-types.h>
+#include <linux/usb/ehci_def.h>
 #include <linux/usb/tegra_usb_phy.h>
 #include <linux/module.h>
 
 #define ULPI_VIEWPORT		0x170
 
+/* PORTSC registers */
+#define TEGRA_USB_PORTSC1				0x184
+#define TEGRA_USB_PORTSC1_PTS(x)			(((x) & 0x3) << 30)
+#define TEGRA_USB_PORTSC1_PHCD				(1 << 23)
+
+/* Bits of PORTSC1, which will get cleared by writing 1 into them */
+#define TEGRA_PORTSC1_RWC_BITS	(PORT_CSC | PORT_PEC | PORT_OCC)
+
 #define USB_SUSP_CTRL		0x400
 #define   USB_WAKE_ON_CNNT_EN_DEV	(1 << 3)
 #define   USB_WAKE_ON_DISCON_EN_DEV	(1 << 4)
@@ -197,6 +206,30 @@ static struct tegra_utmip_config utmip_default[] = {
 	},
 };
 
+static void set_pts(struct tegra_usb_phy *phy, u8 pts_val)
+{
+	void __iomem *base = phy->regs;
+	unsigned long val;
+
+	val = readl(base + TEGRA_USB_PORTSC1) & ~TEGRA_PORTSC1_RWC_BITS;
+	val &= ~TEGRA_USB_PORTSC1_PTS(3);
+	val |= TEGRA_USB_PORTSC1_PTS(pts_val & 3);
+	writel(val, base + TEGRA_USB_PORTSC1);
+}
+
+static void set_phcd(struct tegra_usb_phy *phy, bool enable)
+{
+	void __iomem *base = phy->regs;
+	unsigned long val;
+
+	val = readl(base + TEGRA_USB_PORTSC1) & ~TEGRA_PORTSC1_RWC_BITS;
+	if (enable)
+		val |= TEGRA_USB_PORTSC1_PHCD;
+	else
+		val &= ~TEGRA_USB_PORTSC1_PHCD;
+	writel(val, base + TEGRA_USB_PORTSC1);
+}
+
 static int utmip_pad_open(struct tegra_usb_phy *phy)
 {
 	phy->pad_clk = devm_clk_get(phy->dev, "utmi-pads");
@@ -283,7 +316,7 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 		val &= ~USB_SUSP_SET;
 		writel(val, base + USB_SUSP_CTRL);
 	} else
-		tegra_ehci_set_phcd(&phy->u_phy, true);
+		set_phcd(phy, true);
 
 	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID, 0) < 0)
 		pr_err("%s: timeout waiting for phy to stabilize\n", __func__);
@@ -305,7 +338,7 @@ static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
 		val &= ~USB_SUSP_CLR;
 		writel(val, base + USB_SUSP_CTRL);
 	} else
-		tegra_ehci_set_phcd(&phy->u_phy, false);
+		set_phcd(phy, false);
 
 	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID,
 						     USB_PHY_CLK_VALID))
@@ -428,7 +461,7 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 	utmi_phy_clk_enable(phy);
 
 	if (!phy->is_legacy_phy)
-		tegra_ehci_set_pts(&phy->u_phy, 0);
+		set_pts(phy, 0);
 
 	return 0;
 }
diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
index 0cd15d2..d2ca919 100644
--- a/include/linux/usb/tegra_usb_phy.h
+++ b/include/linux/usb/tegra_usb_phy.h
@@ -76,8 +76,4 @@ void tegra_ehci_phy_restore_start(struct usb_phy *phy,
 
 void tegra_ehci_phy_restore_end(struct usb_phy *phy);
 
-void tegra_ehci_set_pts(struct usb_phy *x, u8 pts_val);
-
-void tegra_ehci_set_phcd(struct usb_phy *x, bool enable);

[PATCH V2 6/6] USB: EHCI: make ehci-tegra a separate driver

[Stephen Warren]

From: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Separate the Tegra on-chip host controller driver from
ehci-hcd host code so that it can be built as a separate driver module.
This work is part of enabling multi-platform kernels on ARM.

Signed-off-by: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
[swarren, reworked Manjunath's patches to split them more logically,
minor re-order of added lines to better match layout of other split-up
HCD drivers and existing code, add MODULE_DEVICE_TABLE, fix
MODULE_LICENSE.]
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2:
* Set non-standard fields in tegra_ehci_hc_driver manually, rather than
  relying on an expanded struct ehci_driver_overrides.
* Save orig_hub_control rather than relying on ehci_hub_control being
  exported.
* Rebased on new versions of earlier patches.
---
 drivers/usb/host/Kconfig      |   2 +-
 drivers/usb/host/Makefile     |   1 +
 drivers/usb/host/ehci-hcd.c   |   5 --
 drivers/usb/host/ehci-tegra.c | 132 +++++++++++++++++++++++-------------------
 4 files changed, 73 insertions(+), 67 deletions(-)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index fcb20fd..6c9347c 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -200,7 +200,7 @@ config USB_EHCI_MSM
 	  has an external PHY.
 
 config USB_EHCI_TEGRA
-       boolean "NVIDIA Tegra HCD support"
+       tristate "NVIDIA Tegra HCD support"
        depends on ARCH_TEGRA
        select USB_EHCI_ROOT_HUB_TT
        select USB_PHY
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index dbc785d..c170383 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_USB_EHCI_HCD_SPEAR)	+= ehci-spear.o
 obj-$(CONFIG_USB_EHCI_S5P)	+= ehci-s5p.o
 obj-$(CONFIG_USB_EHCI_HCD_AT91) += ehci-atmel.o
 obj-$(CONFIG_USB_EHCI_MSM)	+= ehci-msm.o
+obj-$(CONFIG_USB_EHCI_TEGRA)	+= ehci-tegra.o
 
 obj-$(CONFIG_USB_OXU210HP_HCD)	+= oxu210hp-hcd.o
 obj-$(CONFIG_USB_ISP116X_HCD)	+= isp116x-hcd.o
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index e8a6f3d..7abf1ce 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1269,11 +1269,6 @@ MODULE_LICENSE ("GPL");
 #define	PLATFORM_DRIVER		ehci_hcd_msp_driver
 #endif
 
-#ifdef CONFIG_USB_EHCI_TEGRA
-#include "ehci-tegra.c"
-#define PLATFORM_DRIVER		tegra_ehci_driver
-#endif
-
 #ifdef CONFIG_SPARC_LEON
 #include "ehci-grlib.c"
 #define PLATFORM_DRIVER		ehci_grlib_driver
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index c8dc687..b164757 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -17,25 +17,44 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clk/tegra.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
-#include <linux/platform_device.h>
-#include <linux/platform_data/tegra_usb.h>
-#include <linux/irq.h>
-#include <linux/usb/otg.h>
 #include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/tegra_usb.h>
 #include <linux/pm_runtime.h>
+#include <linux/slab.h>
 #include <linux/usb/ehci_def.h>
 #include <linux/usb/tegra_usb_phy.h>
-#include <linux/clk/tegra.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+#include <linux/usb/otg.h>
+
+#include "ehci.h"
 
 #define TEGRA_USB_BASE			0xC5000000
 #define TEGRA_USB2_BASE			0xC5004000
 #define TEGRA_USB3_BASE			0xC5008000
 
+#define PORT_WAKE_BITS (PORT_WKOC_E|PORT_WKDISC_E|PORT_WKCONN_E)
+
 #define TEGRA_USB_DMA_ALIGN 32
 
+#define DRIVER_DESC "Tegra EHCI driver"
+#define DRV_NAME "tegra-ehci"
+
+static struct hc_driver __read_mostly tegra_ehci_hc_driver;
+
+static int (*orig_hub_control)(struct usb_hcd *hcd,
+				u16 typeReq, u16 wValue, u16 wIndex,
+				char *buf, u16 wLength);
+
 struct tegra_ehci_hcd {
 	struct ehci_hcd *ehci;
 	struct tegra_usb_phy *phy;
@@ -228,37 +247,13 @@ static int tegra_ehci_hub_control(
 	spin_unlock_irqrestore(&ehci->lock, flags);
 
 	/* Handle the hub control events here */
-	return ehci_hub_control(hcd, typeReq, wValue, wIndex, buf, wLength);
+	return orig_hub_control(hcd, typeReq, wValue, wIndex, buf, wLength);
+
 done:
 	spin_unlock_irqrestore(&ehci->lock, flags);
 	return retval;
 }
 
-static void tegra_ehci_shutdown(struct usb_hcd *hcd)
-{
-	struct tegra_ehci_hcd *tegra = dev_get_drvdata(hcd->self.controller);
-
-	/* ehci_shutdown touches the USB controller registers, make sure
-	 * controller has clocks to it */
-	if (!tegra->host_resumed)
-		tegra_ehci_power_up(hcd);
-
-	ehci_shutdown(hcd);
-}
-
-static int tegra_ehci_setup(struct usb_hcd *hcd)
-{
-	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
-
-	/* EHCI registers start at offset 0x100 */
-	ehci->caps = hcd->regs + 0x100;
-
-	/* switch to host mode */
-	hcd->has_tt = 1;
-
-	return ehci_setup(hcd);
-}
-
 struct dma_aligned_buffer {
 	void *kmalloc_ptr;
 	void *old_xfer_buffer;
@@ -338,34 +333,6 @@ static void tegra_ehci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 	free_dma_aligned_buffer(urb);
 }
 
-static const struct hc_driver tegra_ehci_hc_driver = {
-	.description		= hcd_name,
-	.product_desc		= "Tegra EHCI Host Controller",
-	.hcd_priv_size		= sizeof(struct ehci_hcd),
-	.flags			= HCD_USB2 | HCD_MEMORY,
-
-	/* standard ehci functions */
-	.irq			= ehci_irq,
-	.start			= ehci_run,
-	.stop			= ehci_stop,
-	.urb_enqueue		= ehci_urb_enqueue,
-	.urb_dequeue		= ehci_urb_dequeue,
-	.endpoint_disable	= ehci_endpoint_disable,
-	.endpoint_reset		= ehci_endpoint_reset,
-	.get_frame_number	= ehci_get_frame,
-	.hub_status_data	= ehci_hub_status_data,
-	.clear_tt_buffer_complete = ehci_clear_tt_buffer_complete,
-	.relinquish_port	= ehci_relinquish_port,
-	.port_handed_over	= ehci_port_handed_over,
-
-	/* modified ehci functions for tegra */
-	.reset			= tegra_ehci_setup,
-	.shutdown		= tegra_ehci_shutdown,
-	.map_urb_for_dma	= tegra_ehci_map_urb_for_dma,
-	.unmap_urb_for_dma	= tegra_ehci_unmap_urb_for_dma,
-	.hub_control		= tegra_ehci_hub_control,
-};
-
 static int setup_vbus_gpio(struct platform_device *pdev,
 			   struct tegra_ehci_platform_data *pdata)
 {
@@ -462,6 +429,9 @@ static int tegra_ehci_probe(struct platform_device *pdev)
 		err = -ENOMEM;
 		goto cleanup_clk;
 	}
+	tegra->ehci = hcd_to_ehci(hcd);
+
+	hcd->has_tt = 1;
 	hcd->phy = u_phy;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -478,6 +448,7 @@ static int tegra_ehci_probe(struct platform_device *pdev)
 		err = -ENOMEM;
 		goto cleanup_hcd_create;
 	}
+	tegra->ehci->caps = hcd->regs + 0x100;
 
 	err = usb_phy_init(hcd->phy);
 	if (err) {
@@ -501,7 +472,6 @@ static int tegra_ehci_probe(struct platform_device *pdev)
 	}
 
 	tegra->host_resumed = 1;
-	tegra->ehci = hcd_to_ehci(hcd);
 
 	irq = platform_get_irq(pdev, 0);
 	if (!irq) {
@@ -563,6 +533,12 @@ static void tegra_ehci_hcd_shutdown(struct platform_device *pdev)
 	struct tegra_ehci_hcd *tegra = platform_get_drvdata(pdev);
 	struct usb_hcd *hcd = ehci_to_hcd(tegra->ehci);
 
+	/*
+	 * ehci_shutdown touches the USB controller registers, make sure
+	 * controller has clocks to it
+	 */
+	if (!tegra->host_resumed)
+		tegra_ehci_power_up(hcd);
 	if (hcd->driver->shutdown)
 		hcd->driver->shutdown(hcd);
 }
@@ -577,7 +553,41 @@ static struct platform_driver tegra_ehci_driver = {
 	.remove		= tegra_ehci_remove,
 	.shutdown	= tegra_ehci_hcd_shutdown,
 	.driver		= {
-		.name	= "tegra-ehci",
+		.name	= DRV_NAME,
 		.of_match_table = tegra_ehci_of_match,
 	}
 };
+
+static struct ehci_driver_overrides tegra_overrides __initdata = {
+	.extra_priv_size	= sizeof(struct tegra_ehci_hcd),
+};
+
+static int __init ehci_tegra_init(void)
+{
+	if (usb_disabled())
+		return -ENODEV;
+
+	pr_info(DRV_NAME ": " DRIVER_DESC "\n");
+
+	ehci_init_driver(&tegra_ehci_hc_driver, &tegra_overrides);
+
+	orig_hub_control = tegra_ehci_hc_driver.hub_control;
+
+	tegra_ehci_hc_driver.map_urb_for_dma = tegra_ehci_map_urb_for_dma;
+	tegra_ehci_hc_driver.unmap_urb_for_dma = tegra_ehci_unmap_urb_for_dma;
+	tegra_ehci_hc_driver.hub_control = tegra_ehci_hub_control;
+
+	return platform_driver_register(&tegra_ehci_driver);
+}
+module_init(ehci_tegra_init);
+
+static void __exit ehci_tegra_cleanup(void)
+{
+	platform_driver_unregister(&tegra_ehci_driver);
+}
+module_exit(ehci_tegra_cleanup);
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
+MODULE_DEVICE_TABLE(of, tegra_ehci_of_match);
-- 
1.8.1.5

Re: [PATCH V2 0/6] USB: make ehci-tegra its own module

[Arnd Bergmann]

On Wednesday 05 June 2013, Stephen Warren wrote:
> 
> Today 01:53:28
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> ehci-tegra is currently built into the main ehci-hcd driver, rather than
> being a separate module. This causes issues with multi-platform ARM
> kernels. This series separates ehci-tegra into its own module to avoid
> those problems.
> 
> Manjunath Goudar originally wrote most of this series. I've since cleaned
> it up, rebased it on Venu's recent changes to the Tegra USB driver, and
> tested it.
> 
> Note that I expect this series will need to go through the USB PHY tree,
> since it builds upon Venu's recent changes to the Tegra USB driver, which
> were applied there.

Thanks for following up on this!

Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

Re: [PATCH V2 3/6] USB: EHCI: export ehci_handshake for ehci-hcd sub-drivers

[Alan Stern]

On Tue, 4 Jun 2013, Stephen Warren wrote:

> From: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> In order to split ehci-hcd.c into separate modules, handshake() must be
> exported. Rename the symbol to add an ehci_ prefix, to avoid any naming
> clashes.
> 
> Signed-off-by: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> [swarren, split Manjunath's patches more logically, limit this change
> to export just handshake()]
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> v2: Only export handshake(), not reset/tdi_reset/halt.

Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>

Re: [PATCH V2 4/6] USB: EHCI: tegra: remove all power management

[Alan Stern]

On Tue, 4 Jun 2013, Stephen Warren wrote:

> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The PM routines in ehci-tegra.c use APIs such as ehci_reset(),
> ehci_halt(), and ehci_tdi_reset() that would need to be exported to
> convert ehci-tegra.c into a separate module from ehci-hcd.c. However,
> we'd prefer not to export them.
> 
> Instead, simply remove all power management functionality. Runtime PM
> was disabled since it didn't work correctly, and system suspend isn't
> yet supported in a meaningful way. So, this change doesn't lose any
> functionality.
> 
> Hopefully the power management logic can be reimplemented in a cleaner
> way in the future.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> NOTE: This could do with a little more testing on a few more boards. If
> Alan is OK with this series, I'll go through and do that, and let you
> know when I've tested it enough to be applied. I saw no issues on the one
> board I tested with though.
> 
> I'd appreciate any hints if this patch is actually removing more
> functionality than I think, and is going to break something horribly.

This is a good way to break the logjam.  It needs just a couple of
improvements:

> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index 289b9b8..8f42b3a 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -61,15 +61,6 @@ static void tegra_ehci_power_up(struct usb_hcd *hcd)
>  	tegra->host_resumed = 1;
>  }
>  
> -static void tegra_ehci_power_down(struct usb_hcd *hcd)
> -{
> -	struct tegra_ehci_hcd *tegra = dev_get_drvdata(hcd->self.controller);
> -
> -	tegra->host_resumed = 0;
> -	usb_phy_set_suspend(hcd->phy, 1);
> -	clk_disable_unprepare(tegra->clk);
> -}
> -

tegra_ehci_power_up() shouldn't be retained either.  And with these
functions removed, there's no need to keep the tegra->host_resumed
field.

Related to this, the tegra_ehci_shutdown() routine should be removed,
and the .shutdown entry in tegra_ehci_hc_driver should point to
ehci_shutdown.

> @@ -399,10 +369,6 @@ static const struct hc_driver tegra_ehci_hc_driver = {
>  	.map_urb_for_dma	= tegra_ehci_map_urb_for_dma,
>  	.unmap_urb_for_dma	= tegra_ehci_unmap_urb_for_dma,
>  	.hub_control		= tegra_ehci_hub_control,
> -#ifdef CONFIG_PM
> -	.bus_suspend		= ehci_bus_suspend,
> -	.bus_resume		= ehci_bus_resume,
> -#endif
>  };

These two entries should be retained.  They refer to USB bus suspend
rather than controller suspend, and they don't use any Tegra-specific
code.

Alan Stern

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

Re: [PATCH V2 5/6] USB: EHCI: tegra: fix circular module dependencies

[Alan Stern]

On Tue, 4 Jun 2013, Stephen Warren wrote:

> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The Tegra EHCI driver directly calls various functions in the Tegra USB
> PHY driver. The reverse is also true; the PHY driver calls into the EHCI
> driver. This is problematic when the two are built as modules.
> 
> The calls from the PHY to EHCI driver were originally added in commit
> bbdabdb "usb: add APIs to access host registers from Tegra PHY", for the
> following reasons:
> 
> 1) The register being touched is an EHCI register, so logically only the
>    EHCI driver should touch it.
> 2) (1) implies that some locking may be needed to correctly implement the
>    r/m/w access to this shared register.
> 3) We were expecting to pass only the PHY register space to the Tegra PHY
>    driver, and hence it would not have access to touch the shared
>    registers.
> 
> To solve this, that commit added functions in the EHCI driver to touch the
> shared register on behalf of the PHY driver.
> 
> In practice, we ended up not having any locking in the implementaiton of
> those functions, and I've been led to believe this is safe. Equally, (3)
> did not happen either. Hence, it is possible for the PHY driver to touch
> the shared register directly.
> 
> Given that, this patch moves the code to touch the shared register back
> into the PHY driver, to eliminate the module problems. If we actually
> need locking or co-ordination in the future, I propose we put the lock
> support into some pre-existing core module, or into a third separate
> module, in order to avoid the circular dependencies.
> 
> I apologize for my contribution to code churn here.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> v2: No change; just rebased on new versions of earlier patches.

I didn't bother to look in detail for possible future conflicts.  
Presumably we will be able to handle them as they arise.  Assuming the 
PHY needs adjustment only during probe, removal, suspend, and resume, 
we should be okay without locking.

Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>

Re: [PATCH V2 6/6] USB: EHCI: make ehci-tegra a separate driver

[Alan Stern]

On Tue, 4 Jun 2013, Stephen Warren wrote:

> From: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> Separate the Tegra on-chip host controller driver from
> ehci-hcd host code so that it can be built as a separate driver module.
> This work is part of enabling multi-platform kernels on ARM.
> 
> Signed-off-by: Manjunath Goudar <manjunath.goudar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> [swarren, reworked Manjunath's patches to split them more logically,
> minor re-order of added lines to better match layout of other split-up
> HCD drivers and existing code, add MODULE_DEVICE_TABLE, fix
> MODULE_LICENSE.]
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> v2:
> * Set non-standard fields in tegra_ehci_hc_driver manually, rather than
>   relying on an expanded struct ehci_driver_overrides.
> * Save orig_hub_control rather than relying on ehci_hub_control being
>   exported.
> * Rebased on new versions of earlier patches.

Considering the changes recommended for the 4/6 patch, this needs a few 
updates too.

> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index c8dc687..b164757 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c

>  struct tegra_ehci_hcd {
>  	struct ehci_hcd *ehci;
>  	struct tegra_usb_phy *phy;

Since you're doing the conversion, it makes sense also to convert this
structure from being separately allocated to using the private area at
the end of ehci_hcd.  (As part of that, the platform_set/get_drvdata
calls would store/retrieve the address of the ehci_hcd structure
instead of the tegra_ehci_hcd.)  Manjunath has already done this for
other drivers.

I suppose this could be added on in a separate patch.

> -static void tegra_ehci_shutdown(struct usb_hcd *hcd)
> -{
> -	struct tegra_ehci_hcd *tegra = dev_get_drvdata(hcd->self.controller);
> -
> -	/* ehci_shutdown touches the USB controller registers, make sure
> -	 * controller has clocks to it */
> -	if (!tegra->host_resumed)
> -		tegra_ehci_power_up(hcd);
> -
> -	ehci_shutdown(hcd);
> -}

Of course, this routine should already be gone.

> @@ -462,6 +429,9 @@ static int tegra_ehci_probe(struct platform_device *pdev)
>  		err = -ENOMEM;
>  		goto cleanup_clk;
>  	}
> +	tegra->ehci = hcd_to_ehci(hcd);

When the private data structure is moved to the private area, there 
will be no need for this back-pointer.  The "ehci" field can be 
eliminated.

> @@ -563,6 +533,12 @@ static void tegra_ehci_hcd_shutdown(struct platform_device *pdev)
>  	struct tegra_ehci_hcd *tegra = platform_get_drvdata(pdev);
>  	struct usb_hcd *hcd = ehci_to_hcd(tegra->ehci);
>  
> +	/*
> +	 * ehci_shutdown touches the USB controller registers, make sure
> +	 * controller has clocks to it
> +	 */
> +	if (!tegra->host_resumed)
> +		tegra_ehci_power_up(hcd);

This doesn't need to go here.  Since all the power management has been 
removed, the controller will never be suspended or powered down.  Hence 
there's no need to check or to power it back up.

> +static struct ehci_driver_overrides tegra_overrides __initdata = {
> +	.extra_priv_size	= sizeof(struct tegra_ehci_hcd),
> +};

The annotation should be __initconst rather than __initdata.

> +
> +static int __init ehci_tegra_init(void)
> +{
> +	if (usb_disabled())
> +		return -ENODEV;
> +
> +	pr_info(DRV_NAME ": " DRIVER_DESC "\n");
> +
> +	ehci_init_driver(&tegra_ehci_hc_driver, &tegra_overrides);
> +
> +	orig_hub_control = tegra_ehci_hc_driver.hub_control;
> +
> +	tegra_ehci_hc_driver.map_urb_for_dma = tegra_ehci_map_urb_for_dma;
> +	tegra_ehci_hc_driver.unmap_urb_for_dma = tegra_ehci_unmap_urb_for_dma;
> +	tegra_ehci_hc_driver.hub_control = tegra_ehci_hub_control;

You might want to add a comment explaining the reason for these manual 
overrides.  It's up to you.

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