RE: [PATCH 0/8][v2]Add OTG support for FSL socs

RE: [PATCH 0/8][v2]Add OTG support for FSL socs

[Ramneek Mehresh]

Please ignore v2 ... sent by mistake...please consider v3 patch-set

> -----Original Message-----
> From: Ramneek Mehresh [mailto:ramneek.mehresh@freescale.com]
> Sent: Thursday, August 13, 2015 11:55 PM
> To: linux-kernel@vger.kernel.org
> Cc: balbi@ti.com; stern@rowland.harvard.edu; gregkh@linuxfoundation.org;
> linux-usb@vger.kernel.org; Mehresh Ramneek-B31383
> <ramneek.mehresh@freescale.com>
> Subject: [PATCH 0/8][v2]Add OTG support for FSL socs
> 
> Add support for otg for all freescale socs having internal usb phy.
> 
> 
> Ramneek Mehresh (8):
>   usb:fsl:otg: Make fsl otg driver as tristate
>   usb:fsl:otg: Add controller version based ULPI and UTMI phy
>   usb:fsl:otg: Add support to add/remove usb host driver
>   usb:fsl:otg: Signal host drv when host is otg
>   usb:fsl:otg: Modify otg_event to start host drv
>   usb:fsl:otg: Combine host/gadget start/resume for ID change
>   usb:fsl:otg: Remove host drv upon otg bring-up
>   usb:fsl:otg: Add host-gadget drv sync delay
> 
>  drivers/usb/host/ehci-fsl.c   | 85
> +++++++++++++++++++++++++++++++++----------
>  drivers/usb/host/ehci-fsl.h   | 20 ++++++++++
>  drivers/usb/phy/Kconfig       |  2 +-
>  drivers/usb/phy/phy-fsl-usb.c | 58 ++++++++++++++++++++++-------
> drivers/usb/phy/phy-fsl-usb.h |  7 ++++
>  include/linux/usb.h           |  1 +
>  6 files changed, 139 insertions(+), 34 deletions(-)
> 
> --
> Changes for v2:
> 	- moved struct ehci_fsl out of ehci-fsl.c
> 	  to ehci-fsl.h
> 	- made struct ehci_fsl private of struct ehci_hcd
> 1.8.3.1

[PATCH 0/8][v3] Add OTG support for FSL socs

[Ramneek Mehresh]

Add support for otg for all freescale socs having internal
usb phy.

Ramneek Mehresh (8):
  usb:fsl:otg: Make fsl otg driver as tristate
  usb:fsl:otg: Add controller version based ULPI and UTMI phy
  usb:fsl:otg: Add support to add/remove usb host driver
  usb:fsl:otg: Signal host drv when host is otg
  usb:fsl:otg: Modify otg_event to start host drv
  usb:fsl:otg: Combine host/gadget start/resume for ID change
  usb:fsl:otg: Remove host drv upon otg bring-up
  usb:fsl:otg: Add host-gadget drv sync delay

 drivers/usb/host/ehci-fsl.c   | 73 +++++++++++++++++++++++++++++++------------
 drivers/usb/host/ehci-fsl.h   | 18 +++++++++++
 drivers/usb/phy/Kconfig       |  2 +-
 drivers/usb/phy/phy-fsl-usb.c | 58 +++++++++++++++++++++++++---------
 drivers/usb/phy/phy-fsl-usb.h |  7 +++++
 include/linux/usb.h           |  1 +
 6 files changed, 124 insertions(+), 35 deletions(-)

--
Changes for v3:
	- removed CONFIG_FSL_USB2_OTG and CONFIG_FSL_USB2_OTG_MODULE
	  macros
	- removed call to usb_hcd_resume_root_hub(hcd) from 
	  ehci_fsl_drv_resume()
1.8.3.1

[PATCH 1/8][v3]usb:fsl:otg: Make fsl otg driver as tristate

[Ramneek Mehresh]

Provide option to load fsl otg driver as loadable module.

Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
---
 drivers/usb/phy/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index 869c0cfcad..8a09267 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -19,7 +19,7 @@ config AB8500_USB
 	  in host mode, low speed.
 
 config FSL_USB2_OTG
-	bool "Freescale USB OTG Transceiver Driver"
+	tristate "Freescale USB OTG Transceiver Driver"
 	depends on USB_EHCI_FSL && USB_FSL_USB2 && USB_OTG_FSM && PM
 	select USB_OTG
 	select USB_PHY
-- 
1.8.3.1

[PATCH 2/8][v3]usb:fsl:otg: Add controller version based ULPI and UTMI phy

[Ramneek Mehresh]

Add controller version based ULPI and UTMI phy
initialization for otg driver.

Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com>
Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
Reviewed-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
Tested-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
---
 drivers/usb/phy/phy-fsl-usb.c | 20 ++++++++++++++++++++
 drivers/usb/phy/phy-fsl-usb.h |  7 +++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index 94eb292..4e58aee 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -923,12 +923,32 @@ int usb_otg_start(struct platform_device *pdev)
 	temp &= ~(PORTSC_PHY_TYPE_SEL | PORTSC_PTW);
 	switch (pdata->phy_mode) {
 	case FSL_USB2_PHY_ULPI:
+		if (pdata->controller_ver) {
+			/* controller version 1.6 or above */
+			setbits32(&p_otg->dr_mem_map->control,
+				USB_CTRL_ULPI_PHY_CLK_SEL);
+			/*
+			 * Due to controller issue of PHY_CLK_VALID in ULPI
+			 * mode, set USB_CTRL_USB_EN before checking
+			 * PHY_CLK_VALID, otherwise PHY_CLK_VALID doesn't work
+			 */
+			clrsetbits_be32(&p_otg->dr_mem_map->control,
+				USB_CTRL_UTMI_PHY_EN, USB_CTRL_IOENB);
+		}
 		temp |= PORTSC_PTS_ULPI;
 		break;
 	case FSL_USB2_PHY_UTMI_WIDE:
 		temp |= PORTSC_PTW_16BIT;
 		/* fall through */
 	case FSL_USB2_PHY_UTMI:
+		if (pdata->controller_ver) {
+			/* controller version 1.6 or above */
+			setbits32(&p_otg->dr_mem_map->control,
+				USB_CTRL_UTMI_PHY_EN);
+			/* Delay for UTMI PHY CLK to become stable - 10ms */
+			mdelay(FSL_UTMI_PHY_DLY);
+		}
+		setbits32(&p_otg->dr_mem_map->control, USB_CTRL_UTMI_PHY_EN);
 		temp |= PORTSC_PTS_UTMI;
 		/* fall through */
 	default:
diff --git a/drivers/usb/phy/phy-fsl-usb.h b/drivers/usb/phy/phy-fsl-usb.h
index 2314995..4a78fb3 100644
--- a/drivers/usb/phy/phy-fsl-usb.h
+++ b/drivers/usb/phy/phy-fsl-usb.h
@@ -199,6 +199,13 @@
 /* control Register Bit Masks */
 #define  USB_CTRL_IOENB			(0x1<<2)
 #define  USB_CTRL_ULPI_INT0EN		(0x1<<0)
+#define  USB_CTRL_WU_INT_EN		(0x1<<1)
+#define  USB_CTRL_LINE_STATE_FILTER__EN	(0x1<<3)
+#define  USB_CTRL_KEEP_OTG_ON		(0x1<<4)
+#define  USB_CTRL_OTG_PORT		(0x1<<5)
+#define  USB_CTRL_PLL_RESET		(0x1<<8)
+#define  USB_CTRL_UTMI_PHY_EN		(0x1<<9)
+#define  USB_CTRL_ULPI_PHY_CLK_SEL	(0x1<<10)
 
 /* BCSR5 */
 #define BCSR5_INT_USB			(0x02)
-- 
1.8.3.1

[PATCH 3/8][v3]usb:fsl:otg: Add support to add/remove usb host driver

[Ramneek Mehresh]

Add workqueue to add/remove host driver (outside
interrupt context) upon each id change.

Signed-off-by: Li Yang <leoli@freescale.com>
Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
---
Changes for v3:
        - removed CONFIG_FSL_USB2_OTG and CONFIG_FSL_USB2_OTG_MODULE
          macros
        - removed call to usb_hcd_resume_root_hub(hcd) from
          ehci_fsl_drv_resume()

 drivers/usb/host/ehci-fsl.c | 71 ++++++++++++++++++++++++++++++++-------------
 drivers/usb/host/ehci-fsl.h | 18 ++++++++++++
 2 files changed, 69 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 202dafb..ef04c5a 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -44,6 +44,31 @@
 
 static struct hc_driver __read_mostly fsl_ehci_hc_driver;
 
+static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd)
+{
+	return (struct ehci_fsl *)hcd_to_ehci(hcd)->priv;
+}
+
+static void do_change_hcd(struct work_struct *work)
+{
+	struct ehci_fsl *ehci_fsl = container_of(work, struct ehci_fsl,
+					change_hcd_work);
+	struct usb_hcd *hcd = ehci_fsl->hcd;
+	void __iomem *non_ehci = hcd->regs;
+	int retval;
+
+	if (ehci_fsl->hcd_add && !ehci_fsl->have_hcd) {
+		writel(USBMODE_CM_HOST, non_ehci + FSL_SOC_USB_USBMODE);
+		/* host, gadget and otg share same int line */
+		retval = usb_add_hcd(hcd, hcd->irq, IRQF_SHARED);
+		if (retval == 0)
+			ehci_fsl->have_hcd = 1;
+	} else if (!ehci_fsl->hcd_add && ehci_fsl->have_hcd) {
+		usb_remove_hcd(hcd);
+		ehci_fsl->have_hcd = 0;
+	}
+}
+
 /* configure so an HC device and id are always provided */
 /* always called with process context; sleeping is OK */
 
@@ -147,11 +172,15 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
 		goto err2;
 	device_wakeup_enable(hcd->self.controller);
 
-#ifdef CONFIG_USB_OTG
 	if (pdata->operating_mode == FSL_USB2_DR_OTG) {
 		struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+		struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 
+		ehci_fsl->hcd = hcd;
 		hcd->usb_phy = usb_get_phy(USB_PHY_TYPE_USB2);
+
+		INIT_WORK(&ehci_fsl->change_hcd_work, do_change_hcd);
+
 		dev_dbg(&pdev->dev, "hcd=0x%p  ehci=0x%p, phy=0x%p\n",
 			hcd, ehci, hcd->usb_phy);
 
@@ -168,7 +197,7 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
 			goto err2;
 		}
 	}
-#endif
+
 	return retval;
 
       err2:
@@ -371,15 +400,6 @@ static int ehci_fsl_setup(struct usb_hcd *hcd)
 	return retval;
 }
 
-struct ehci_fsl {
-	struct ehci_hcd	ehci;
-
-#ifdef CONFIG_PM
-	/* Saved USB PHY settings, need to restore after deep sleep. */
-	u32 usb_ctrl;
-#endif
-};
-
 #ifdef CONFIG_PM
 
 #ifdef CONFIG_PPC_MPC512x
@@ -527,24 +547,26 @@ static inline int ehci_fsl_mpc512x_drv_resume(struct device *dev)
 }
 #endif /* CONFIG_PPC_MPC512x */
 
-static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd)
-{
-	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
-
-	return container_of(ehci, struct ehci_fsl, ehci);
-}
-
 static int ehci_fsl_drv_suspend(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
-	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 	void __iomem *non_ehci = hcd->regs;
+	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
+	struct usb_bus host = hcd->self;
 
 	if (of_device_is_compatible(dev->parent->of_node,
 				    "fsl,mpc5121-usb2-dr")) {
 		return ehci_fsl_mpc512x_drv_suspend(dev);
 	}
 
+	if (host.is_otg) {
+		/* remove hcd */
+		ehci_fsl->hcd_add = 0;
+		schedule_work(&ehci_fsl->change_hcd_work);
+		host.is_otg = 0;
+		return 0;
+	}
+
 	ehci_prepare_ports_for_controller_suspend(hcd_to_ehci(hcd),
 			device_may_wakeup(dev));
 	if (!fsl_deep_sleep())
@@ -557,15 +579,24 @@ static int ehci_fsl_drv_suspend(struct device *dev)
 static int ehci_fsl_drv_resume(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
-	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 	void __iomem *non_ehci = hcd->regs;
+	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
+	struct usb_bus host = hcd->self;
 
 	if (of_device_is_compatible(dev->parent->of_node,
 				    "fsl,mpc5121-usb2-dr")) {
 		return ehci_fsl_mpc512x_drv_resume(dev);
 	}
 
+	if (host.is_otg) {
+		/* add hcd */
+		ehci_fsl->hcd_add = 1;
+		schedule_work(&ehci_fsl->change_hcd_work);
+		host.is_otg = 0;
+		return 0;
+	}
+
 	ehci_prepare_ports_for_controller_resume(ehci);
 	if (!fsl_deep_sleep())
 		return 0;
diff --git a/drivers/usb/host/ehci-fsl.h b/drivers/usb/host/ehci-fsl.h
index 1a8a60a..7c46725 100644
--- a/drivers/usb/host/ehci-fsl.h
+++ b/drivers/usb/host/ehci-fsl.h
@@ -63,4 +63,22 @@
 #define UTMI_PHY_EN             (1<<9)
 #define ULPI_PHY_CLK_SEL        (1<<10)
 #define PHY_CLK_VALID		(1<<17)
+
+struct ehci_fsl {
+#ifdef CONFIG_PM
+	/* Saved USB PHY settings, need to restore after deep sleep. */
+	u32 usb_ctrl;
+#endif
+	struct usb_hcd *hcd;
+	struct work_struct change_hcd_work;
+	/*
+	 * store current hcd state for otg;
+	 * have_hcd is true when host drv al already part of otg framework,
+	 * otherwise false;
+	 * hcd_add is true when otg framework wants to add host
+	 * drv as part of otg;flase when it wants to remove it
+	 */
+	unsigned have_hcd:1;
+	unsigned hcd_add:1;
+};
 #endif				/* _EHCI_FSL_H */
-- 
1.8.3.1

[PATCH 4/8][v3]usb:fsl:otg: Signal host drv when host is otg

[Ramneek Mehresh]

Set is_otg boolean flag to signal host driver when host
is running in context of otg host suspend/resume.

Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
---
 drivers/usb/phy/phy-fsl-usb.c | 7 ++++++-
 include/linux/usb.h           | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index 4e58aee..815c22c 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -463,6 +463,7 @@ void otg_reset_controller(void)
 int fsl_otg_start_host(struct otg_fsm *fsm, int on)
 {
 	struct usb_otg *otg = fsm->otg;
+	struct usb_bus *host = otg->host;
 	struct device *dev;
 	struct fsl_otg *otg_dev =
 		container_of(otg->usb_phy, struct fsl_otg, phy);
@@ -485,6 +486,7 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
 		else {
 			otg_reset_controller();
 			VDBG("host on......\n");
+			host->is_otg = 1;
 			if (dev->driver->pm && dev->driver->pm->resume) {
 				retval = dev->driver->pm->resume(dev);
 				if (fsm->id) {
@@ -510,8 +512,11 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
 		else {
 			VDBG("host off......\n");
 			if (dev && dev->driver) {
-				if (dev->driver->pm && dev->driver->pm->suspend)
+				if (dev->driver->pm &&
+					dev->driver->pm->suspend) {
+					host->is_otg = 1;
 					retval = dev->driver->pm->suspend(dev);
+				}
 				if (fsm->id)
 					/* default-b */
 					fsl_otg_drv_vbus(fsm, 0);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 447fe29..2208822 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -355,6 +355,7 @@ struct usb_bus {
 					 * for control transfers?
 					 */
 	u8 otg_port;			/* 0, or number of OTG/HNP port */
+	unsigned is_otg:1;		/* true when host is also otg */
 	unsigned is_b_host:1;		/* true during some HNP roleswitches */
 	unsigned b_hnp_enable:1;	/* OTG: did A-Host enable HNP? */
 	unsigned no_stop_on_short:1;    /*
-- 
1.8.3.1

[PATCH 5/8][v3]usb:fsl:otg: Modify otg_event to start host drv

[Ramneek Mehresh]

Add mechanism to start host driver from inside fsl_otg_even upon
each id change interrupt.

Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
Reviewed-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
Tested-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
---
 drivers/usb/phy/phy-fsl-usb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index 815c22c..3b8a9e5 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -677,6 +677,10 @@ static void fsl_otg_event(struct work_struct *work)
 		fsl_otg_start_host(fsm, 0);
 		otg_drv_vbus(fsm, 0);
 		fsl_otg_start_gadget(fsm, 1);
+	} else {
+		fsl_otg_start_gadget(fsm, 0);
+		otg_drv_vbus(fsm, 1);
+		fsl_otg_start_host(fsm, 1);
 	}
 }
 
-- 
1.8.3.1

[PATCH 6/8][v3]usb:fsl:otg: Combine host/gadget start/resume for ID change

[Ramneek Mehresh]

Make call to fsl_otg_event for each id change even.

Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
Reviewed-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
Tested-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
---
 drivers/usb/phy/phy-fsl-usb.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index 3b8a9e5..689516d 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -733,6 +733,7 @@ irqreturn_t fsl_otg_isr(int irq, void *dev_id)
 {
 	struct otg_fsm *fsm = &((struct fsl_otg *)dev_id)->fsm;
 	struct usb_otg *otg = ((struct fsl_otg *)dev_id)->phy.otg;
+	struct fsl_otg *otg_dev = dev_id;
 	u32 otg_int_src, otg_sc;
 
 	otg_sc = fsl_readl(&usb_dr_regs->otgsc);
@@ -762,18 +763,8 @@ irqreturn_t fsl_otg_isr(int irq, void *dev_id)
 				otg->gadget->is_a_peripheral = !fsm->id;
 			VDBG("ID int (ID is %d)\n", fsm->id);
 
-			if (fsm->id) {	/* switch to gadget */
-				schedule_delayed_work(
-					&((struct fsl_otg *)dev_id)->otg_event,
-					100);
-			} else {	/* switch to host */
-				cancel_delayed_work(&
-						    ((struct fsl_otg *)dev_id)->
-						    otg_event);
-				fsl_otg_start_gadget(fsm, 0);
-				otg_drv_vbus(fsm, 1);
-				fsl_otg_start_host(fsm, 1);
-			}
+			schedule_delayed_work(&otg_dev->otg_event, 100);
+
 			return IRQ_HANDLED;
 		}
 	}
-- 
1.8.3.1

[PATCH 7/8][v3]usb:fsl:otg: Remove host drv upon otg bring-up

[Ramneek Mehresh]

Change have_hcd variable to remove/suspend host driver on
completion of otg initialization for otg auto detect.

Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
Reviewed-by: Li Yang-R58472 <LeoLi@freescale.com>
Reviewed-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
Tested-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
---
 drivers/usb/host/ehci-fsl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index ef04c5a..5dd3243 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -196,6 +196,8 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
 			retval = -ENODEV;
 			goto err2;
 		}
+
+		ehci_fsl->have_hcd = 1;
 	}
 
 	return retval;
-- 
1.8.3.1

[PATCH 8/8][v3]usb:fsl:otg: Add host-gadget drv sync delay

[Ramneek Mehresh]

Resolve synchronization issue between host
and gadget drivers upon role-reversal.

Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
Reviewed-by: Li Yang-R58472 <LeoLi@freescale.com>
Reviewed-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
Tested-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
---
 drivers/usb/phy/phy-fsl-usb.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index 689516d..5d678ea 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -544,8 +544,18 @@ int fsl_otg_start_gadget(struct otg_fsm *fsm, int on)
 	dev = otg->gadget->dev.parent;
 
 	if (on) {
-		if (dev->driver->resume)
+		/*
+		 * Delay gadget resume to synchronize between host and gadget
+		 * drivers. Upon role-reversal host drv is shutdown by kernel
+		 * worker thread. By the time host drv shuts down, controller
+		 * gets programmed for gadget role. Shutting host drv after
+		 * this results in controller getting reset, and it stops
+		 * responding to otg events
+		 */
+		if (dev->driver->resume) {
+			msleep(1000);
 			dev->driver->resume(dev);
+		}
 	} else {
 		if (dev->driver->suspend)
 			dev->driver->suspend(dev, otg_suspend_state);
-- 
1.8.3.1

[PATCH 0/8][v2]Add OTG support for FSL socs

[Ramneek Mehresh]

Add support for otg for all freescale socs having internal
usb phy. 


Ramneek Mehresh (8):
  usb:fsl:otg: Make fsl otg driver as tristate
  usb:fsl:otg: Add controller version based ULPI and UTMI phy
  usb:fsl:otg: Add support to add/remove usb host driver
  usb:fsl:otg: Signal host drv when host is otg
  usb:fsl:otg: Modify otg_event to start host drv
  usb:fsl:otg: Combine host/gadget start/resume for ID change
  usb:fsl:otg: Remove host drv upon otg bring-up
  usb:fsl:otg: Add host-gadget drv sync delay

 drivers/usb/host/ehci-fsl.c   | 85 +++++++++++++++++++++++++++++++++----------
 drivers/usb/host/ehci-fsl.h   | 20 ++++++++++
 drivers/usb/phy/Kconfig       |  2 +-
 drivers/usb/phy/phy-fsl-usb.c | 58 ++++++++++++++++++++++-------
 drivers/usb/phy/phy-fsl-usb.h |  7 ++++
 include/linux/usb.h           |  1 +
 6 files changed, 139 insertions(+), 34 deletions(-)

--
Changes for v2:
	- moved struct ehci_fsl out of ehci-fsl.c
	  to ehci-fsl.h
	- made struct ehci_fsl private of struct ehci_hcd
1.8.3.1

[PATCH 1/8][v2]usb:fsl:otg: Make fsl otg driver as tristate

[Ramneek Mehresh]

Provide option to load fsl otg driver as loadable module.

Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
---
 drivers/usb/phy/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index 869c0cfcad..8a09267 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -19,7 +19,7 @@ config AB8500_USB
 	  in host mode, low speed.
 
 config FSL_USB2_OTG
-	bool "Freescale USB OTG Transceiver Driver"
+	tristate "Freescale USB OTG Transceiver Driver"
 	depends on USB_EHCI_FSL && USB_FSL_USB2 && USB_OTG_FSM && PM
 	select USB_OTG
 	select USB_PHY
-- 
1.8.3.1

[PATCH 2/8][v2]usb:fsl:otg: Add controller version based ULPI and UTMI phy

[Ramneek Mehresh]

Add controller version based ULPI and UTMI phy
initialization for otg driver.

Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com>
Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
Reviewed-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
Tested-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
---
 drivers/usb/phy/phy-fsl-usb.c | 20 ++++++++++++++++++++
 drivers/usb/phy/phy-fsl-usb.h |  7 +++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index 94eb292..4e58aee 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -923,12 +923,32 @@ int usb_otg_start(struct platform_device *pdev)
 	temp &= ~(PORTSC_PHY_TYPE_SEL | PORTSC_PTW);
 	switch (pdata->phy_mode) {
 	case FSL_USB2_PHY_ULPI:
+		if (pdata->controller_ver) {
+			/* controller version 1.6 or above */
+			setbits32(&p_otg->dr_mem_map->control,
+				USB_CTRL_ULPI_PHY_CLK_SEL);
+			/*
+			 * Due to controller issue of PHY_CLK_VALID in ULPI
+			 * mode, set USB_CTRL_USB_EN before checking
+			 * PHY_CLK_VALID, otherwise PHY_CLK_VALID doesn't work
+			 */
+			clrsetbits_be32(&p_otg->dr_mem_map->control,
+				USB_CTRL_UTMI_PHY_EN, USB_CTRL_IOENB);
+		}
 		temp |= PORTSC_PTS_ULPI;
 		break;
 	case FSL_USB2_PHY_UTMI_WIDE:
 		temp |= PORTSC_PTW_16BIT;
 		/* fall through */
 	case FSL_USB2_PHY_UTMI:
+		if (pdata->controller_ver) {
+			/* controller version 1.6 or above */
+			setbits32(&p_otg->dr_mem_map->control,
+				USB_CTRL_UTMI_PHY_EN);
+			/* Delay for UTMI PHY CLK to become stable - 10ms */
+			mdelay(FSL_UTMI_PHY_DLY);
+		}
+		setbits32(&p_otg->dr_mem_map->control, USB_CTRL_UTMI_PHY_EN);
 		temp |= PORTSC_PTS_UTMI;
 		/* fall through */
 	default:
diff --git a/drivers/usb/phy/phy-fsl-usb.h b/drivers/usb/phy/phy-fsl-usb.h
index 2314995..4a78fb3 100644
--- a/drivers/usb/phy/phy-fsl-usb.h
+++ b/drivers/usb/phy/phy-fsl-usb.h
@@ -199,6 +199,13 @@
 /* control Register Bit Masks */
 #define  USB_CTRL_IOENB			(0x1<<2)
 #define  USB_CTRL_ULPI_INT0EN		(0x1<<0)
+#define  USB_CTRL_WU_INT_EN		(0x1<<1)
+#define  USB_CTRL_LINE_STATE_FILTER__EN	(0x1<<3)
+#define  USB_CTRL_KEEP_OTG_ON		(0x1<<4)
+#define  USB_CTRL_OTG_PORT		(0x1<<5)
+#define  USB_CTRL_PLL_RESET		(0x1<<8)
+#define  USB_CTRL_UTMI_PHY_EN		(0x1<<9)
+#define  USB_CTRL_ULPI_PHY_CLK_SEL	(0x1<<10)
 
 /* BCSR5 */
 #define BCSR5_INT_USB			(0x02)
-- 
1.8.3.1

[PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver

[Ramneek Mehresh]

Add workqueue to add/remove host driver (outside
interrupt context) upon each id change.

Signed-off-by: Li Yang <leoli@freescale.com>
Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
---
 drivers/usb/host/ehci-fsl.c | 83 ++++++++++++++++++++++++++++++++++-----------
 drivers/usb/host/ehci-fsl.h | 20 +++++++++++
 2 files changed, 84 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 5352e74..81e4bf5 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -44,6 +44,34 @@
 
 static struct hc_driver __read_mostly fsl_ehci_hc_driver;
 
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
+static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd)
+{
+	return (struct ehci_fsl *)hcd_to_ehci(hcd)->priv;
+}
+
+static void do_change_hcd(struct work_struct *work)
+{
+	struct ehci_fsl *ehci_fsl = container_of(work, struct ehci_fsl,
+					change_hcd_work);
+	struct usb_hcd *hcd = ehci_fsl->hcd;
+	void __iomem *non_ehci = hcd->regs;
+	int retval;
+
+	if (ehci_fsl->hcd_add && !ehci_fsl->have_hcd) {
+		writel(USBMODE_CM_HOST, non_ehci + FSL_SOC_USB_USBMODE);
+		/* host, gadget and otg share same int line */
+		retval = usb_add_hcd(hcd, hcd->irq, IRQF_SHARED);
+		if (retval == 0)
+			ehci_fsl->have_hcd = 1;
+	} else if (!ehci_fsl->hcd_add && ehci_fsl->have_hcd) {
+		usb_remove_hcd(hcd);
+		ehci_fsl->have_hcd = 0;
+	}
+}
+#endif
+
+
 /* configure so an HC device and id are always provided */
 /* always called with process context; sleeping is OK */
 
@@ -136,11 +164,16 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
 		goto err2;
 	device_wakeup_enable(hcd->self.controller);
 
-#ifdef CONFIG_USB_OTG
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
 	if (pdata->operating_mode == FSL_USB2_DR_OTG) {
 		struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+		struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 
+		ehci_fsl->hcd = hcd;
 		hcd->usb_phy = usb_get_phy(USB_PHY_TYPE_USB2);
+
+		INIT_WORK(&ehci_fsl->change_hcd_work, do_change_hcd);
+
 		dev_dbg(&pdev->dev, "hcd=0x%p  ehci=0x%p, phy=0x%p\n",
 			hcd, ehci, hcd->usb_phy);
 
@@ -354,15 +387,6 @@ static int ehci_fsl_setup(struct usb_hcd *hcd)
 	return retval;
 }
 
-struct ehci_fsl {
-	struct ehci_hcd	ehci;
-
-#ifdef CONFIG_PM
-	/* Saved USB PHY settings, need to restore after deep sleep. */
-	u32 usb_ctrl;
-#endif
-};
-
 #ifdef CONFIG_PM
 
 #ifdef CONFIG_PPC_MPC512x
@@ -510,24 +534,31 @@ static inline int ehci_fsl_mpc512x_drv_resume(struct device *dev)
 }
 #endif /* CONFIG_PPC_MPC512x */
 
-static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd)
-{
-	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
-
-	return container_of(ehci, struct ehci_fsl, ehci);
-}
-
 static int ehci_fsl_drv_suspend(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
-	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 	void __iomem *non_ehci = hcd->regs;
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
+	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
+	struct usb_bus host = hcd->self;
+#endif
+
 
 	if (of_device_is_compatible(dev->parent->of_node,
 				    "fsl,mpc5121-usb2-dr")) {
 		return ehci_fsl_mpc512x_drv_suspend(dev);
 	}
 
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
+	if (host.is_otg) {
+		/* remove hcd */
+		ehci_fsl->hcd_add = 0;
+		schedule_work(&ehci_fsl->change_hcd_work);
+		host.is_otg = 0;
+		return 0;
+	}
+#endif
+
 	ehci_prepare_ports_for_controller_suspend(hcd_to_ehci(hcd),
 			device_may_wakeup(dev));
 	if (!fsl_deep_sleep())
@@ -540,15 +571,29 @@ static int ehci_fsl_drv_suspend(struct device *dev)
 static int ehci_fsl_drv_resume(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
-	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 	void __iomem *non_ehci = hcd->regs;
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
+	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
+	struct usb_bus host = hcd->self;
+#endif
 
 	if (of_device_is_compatible(dev->parent->of_node,
 				    "fsl,mpc5121-usb2-dr")) {
 		return ehci_fsl_mpc512x_drv_resume(dev);
 	}
 
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
+	if (host.is_otg) {
+		/* add hcd */
+		ehci_fsl->hcd_add = 1;
+		schedule_work(&ehci_fsl->change_hcd_work);
+		usb_hcd_resume_root_hub(hcd);
+		host.is_otg = 0;
+		return 0;
+	}
+#endif
+
 	ehci_prepare_ports_for_controller_resume(ehci);
 	if (!fsl_deep_sleep())
 		return 0;
diff --git a/drivers/usb/host/ehci-fsl.h b/drivers/usb/host/ehci-fsl.h
index dbd292e..3fd1fd0 100644
--- a/drivers/usb/host/ehci-fsl.h
+++ b/drivers/usb/host/ehci-fsl.h
@@ -62,4 +62,24 @@
 #define UTMI_PHY_EN             (1<<9)
 #define ULPI_PHY_CLK_SEL        (1<<10)
 #define PHY_CLK_VALID		(1<<17)
+
+struct ehci_fsl {
+#ifdef CONFIG_PM
+	/* Saved USB PHY settings, need to restore after deep sleep. */
+	u32 usb_ctrl;
+#endif
+	struct usb_hcd *hcd;
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
+	struct work_struct change_hcd_work;
+#endif
+	/*
+	 * store current hcd state for otg;
+	 * have_hcd is true when host drv al already part of otg framework,
+	 * otherwise false;
+	 * hcd_add is true when otg framework wants to add host
+	 * drv as part of otg;flase when it wants to remove it
+	 */
+	unsigned have_hcd:1;
+	unsigned hcd_add:1;
+};
 #endif				/* _EHCI_FSL_H */
-- 
1.8.3.1

[PATCH 4/8][v2]usb:fsl:otg: Signal host drv when host is otg

[Ramneek Mehresh]

Set is_otg boolean flag to signal host driver when host
is running in context of otg host suspend/resume.

Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
---
 drivers/usb/phy/phy-fsl-usb.c | 7 ++++++-
 include/linux/usb.h           | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index 4e58aee..815c22c 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -463,6 +463,7 @@ void otg_reset_controller(void)
 int fsl_otg_start_host(struct otg_fsm *fsm, int on)
 {
 	struct usb_otg *otg = fsm->otg;
+	struct usb_bus *host = otg->host;
 	struct device *dev;
 	struct fsl_otg *otg_dev =
 		container_of(otg->usb_phy, struct fsl_otg, phy);
@@ -485,6 +486,7 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
 		else {
 			otg_reset_controller();
 			VDBG("host on......\n");
+			host->is_otg = 1;
 			if (dev->driver->pm && dev->driver->pm->resume) {
 				retval = dev->driver->pm->resume(dev);
 				if (fsm->id) {
@@ -510,8 +512,11 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
 		else {
 			VDBG("host off......\n");
 			if (dev && dev->driver) {
-				if (dev->driver->pm && dev->driver->pm->suspend)
+				if (dev->driver->pm &&
+					dev->driver->pm->suspend) {
+					host->is_otg = 1;
 					retval = dev->driver->pm->suspend(dev);
+				}
 				if (fsm->id)
 					/* default-b */
 					fsl_otg_drv_vbus(fsm, 0);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 447fe29..2208822 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -355,6 +355,7 @@ struct usb_bus {
 					 * for control transfers?
 					 */
 	u8 otg_port;			/* 0, or number of OTG/HNP port */
+	unsigned is_otg:1;		/* true when host is also otg */
 	unsigned is_b_host:1;		/* true during some HNP roleswitches */
 	unsigned b_hnp_enable:1;	/* OTG: did A-Host enable HNP? */
 	unsigned no_stop_on_short:1;    /*
-- 
1.8.3.1

[PATCH 5/8][v2]usb:fsl:otg: Modify otg_event to start host drv

[Ramneek Mehresh]

Add mechanism to start host driver from inside fsl_otg_even upon
each id change interrupt.

Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
Reviewed-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
Tested-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
---
 drivers/usb/phy/phy-fsl-usb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index 815c22c..3b8a9e5 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -677,6 +677,10 @@ static void fsl_otg_event(struct work_struct *work)
 		fsl_otg_start_host(fsm, 0);
 		otg_drv_vbus(fsm, 0);
 		fsl_otg_start_gadget(fsm, 1);
+	} else {
+		fsl_otg_start_gadget(fsm, 0);
+		otg_drv_vbus(fsm, 1);
+		fsl_otg_start_host(fsm, 1);
 	}
 }
 
-- 
1.8.3.1

[PATCH 6/8][v2]usb:fsl:otg: Combine host/gadget start/resume for ID change

[Ramneek Mehresh]

Make call to fsl_otg_event for each id change even.

Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
Reviewed-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
Tested-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
---
 drivers/usb/phy/phy-fsl-usb.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index 3b8a9e5..689516d 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -733,6 +733,7 @@ irqreturn_t fsl_otg_isr(int irq, void *dev_id)
 {
 	struct otg_fsm *fsm = &((struct fsl_otg *)dev_id)->fsm;
 	struct usb_otg *otg = ((struct fsl_otg *)dev_id)->phy.otg;
+	struct fsl_otg *otg_dev = dev_id;
 	u32 otg_int_src, otg_sc;
 
 	otg_sc = fsl_readl(&usb_dr_regs->otgsc);
@@ -762,18 +763,8 @@ irqreturn_t fsl_otg_isr(int irq, void *dev_id)
 				otg->gadget->is_a_peripheral = !fsm->id;
 			VDBG("ID int (ID is %d)\n", fsm->id);
 
-			if (fsm->id) {	/* switch to gadget */
-				schedule_delayed_work(
-					&((struct fsl_otg *)dev_id)->otg_event,
-					100);
-			} else {	/* switch to host */
-				cancel_delayed_work(&
-						    ((struct fsl_otg *)dev_id)->
-						    otg_event);
-				fsl_otg_start_gadget(fsm, 0);
-				otg_drv_vbus(fsm, 1);
-				fsl_otg_start_host(fsm, 1);
-			}
+			schedule_delayed_work(&otg_dev->otg_event, 100);
+
 			return IRQ_HANDLED;
 		}
 	}
-- 
1.8.3.1

[PATCH 7/8][v2]usb:fsl:otg: Remove host drv upon otg bring-up

[Ramneek Mehresh]

Change have_hcd variable to remove/suspend host driver on
completion of otg initialization for otg auto detect.

Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
Reviewed-by: Li Yang-R58472 <LeoLi@freescale.com>
Reviewed-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
Tested-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
---
 drivers/usb/host/ehci-fsl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 81e4bf5..daca957 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -189,6 +189,8 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
 			retval = -ENODEV;
 			goto err2;
 		}
+
+		ehci_fsl->have_hcd = 1;
 	}
 #endif
 	return retval;
-- 
1.8.3.1

[PATCH 8/8][v2]usb:fsl:otg: Add host-gadget drv sync delay

[Ramneek Mehresh]

Resolve synchronization issue between host
and gadget drivers upon role-reversal.

Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
Reviewed-by: Li Yang-R58472 <LeoLi@freescale.com>
Reviewed-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
Tested-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
---
 drivers/usb/phy/phy-fsl-usb.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index 689516d..5d678ea 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -544,8 +544,18 @@ int fsl_otg_start_gadget(struct otg_fsm *fsm, int on)
 	dev = otg->gadget->dev.parent;
 
 	if (on) {
-		if (dev->driver->resume)
+		/*
+		 * Delay gadget resume to synchronize between host and gadget
+		 * drivers. Upon role-reversal host drv is shutdown by kernel
+		 * worker thread. By the time host drv shuts down, controller
+		 * gets programmed for gadget role. Shutting host drv after
+		 * this results in controller getting reset, and it stops
+		 * responding to otg events
+		 */
+		if (dev->driver->resume) {
+			msleep(1000);
 			dev->driver->resume(dev);
+		}
 	} else {
 		if (dev->driver->suspend)
 			dev->driver->suspend(dev, otg_suspend_state);
-- 
1.8.3.1

Re: [PATCH 7/8][v3]usb:fsl:otg: Remove host drv upon otg bring-up

[Alan Stern]

On Thu, 13 Aug 2015, Ramneek Mehresh wrote:

> Change have_hcd variable to remove/suspend host driver on
> completion of otg initialization for otg auto detect.
> 
> Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
> Reviewed-by: Li Yang-R58472 <LeoLi@freescale.com>
> Reviewed-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
> Tested-by: Fleming Andrew-AFLEMING <AFLEMING@freescale.com>
> ---
>  drivers/usb/host/ehci-fsl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index ef04c5a..5dd3243 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -196,6 +196,8 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
>  			retval = -ENODEV;
>  			goto err2;
>  		}
> +
> +		ehci_fsl->have_hcd = 1;
>  	}
>  
>  	return retval;

Shouldn't this be merged into patch 3/8?  It looks like it just fixes 
an oversight in the earlier patch.

Alan Stern

Re: [PATCH 3/8][v3]usb:fsl:otg: Add support to add/remove usb host driver

[Alan Stern]

On Thu, 13 Aug 2015, Ramneek Mehresh wrote:

> Add workqueue to add/remove host driver (outside
> interrupt context) upon each id change.
> 
> Signed-off-by: Li Yang <leoli@freescale.com>
> Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
> ---
> Changes for v3:
>         - removed CONFIG_FSL_USB2_OTG and CONFIG_FSL_USB2_OTG_MODULE
>           macros
>         - removed call to usb_hcd_resume_root_hub(hcd) from
>           ehci_fsl_drv_resume()
> 
>  drivers/usb/host/ehci-fsl.c | 71 ++++++++++++++++++++++++++++++++-------------
>  drivers/usb/host/ehci-fsl.h | 18 ++++++++++++
>  2 files changed, 69 insertions(+), 20 deletions(-)


>  static int ehci_fsl_drv_suspend(struct device *dev)
>  {
>  	struct usb_hcd *hcd = dev_get_drvdata(dev);
> -	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
>  	void __iomem *non_ehci = hcd->regs;
> +	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> +	struct usb_bus host = hcd->self;

I just noticed this line.  It does not look right at all; it does a 
structure copy instead of copying a pointer.

>  
>  	if (of_device_is_compatible(dev->parent->of_node,
>  				    "fsl,mpc5121-usb2-dr")) {
>  		return ehci_fsl_mpc512x_drv_suspend(dev);
>  	}
>  
> +	if (host.is_otg) {
> +		/* remove hcd */
> +		ehci_fsl->hcd_add = 0;
> +		schedule_work(&ehci_fsl->change_hcd_work);
> +		host.is_otg = 0;

And here you turn off the flag in the local copy of the structure, 
which accomplishes nothing.

> +		return 0;
> +	}
> +
>  	ehci_prepare_ports_for_controller_suspend(hcd_to_ehci(hcd),
>  			device_may_wakeup(dev));
>  	if (!fsl_deep_sleep())
> @@ -557,15 +579,24 @@ static int ehci_fsl_drv_suspend(struct device *dev)
>  static int ehci_fsl_drv_resume(struct device *dev)
>  {
>  	struct usb_hcd *hcd = dev_get_drvdata(dev);
> -	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
>  	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>  	void __iomem *non_ehci = hcd->regs;
> +	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> +	struct usb_bus host = hcd->self;

Same here.

> --- a/drivers/usb/host/ehci-fsl.h
> +++ b/drivers/usb/host/ehci-fsl.h
> @@ -63,4 +63,22 @@
>  #define UTMI_PHY_EN             (1<<9)
>  #define ULPI_PHY_CLK_SEL        (1<<10)
>  #define PHY_CLK_VALID		(1<<17)
> +
> +struct ehci_fsl {
> +#ifdef CONFIG_PM
> +	/* Saved USB PHY settings, need to restore after deep sleep. */
> +	u32 usb_ctrl;
> +#endif

Do you need this #ifdef?

Alan Stern

RE: [PATCH 3/8][v3]usb:fsl:otg: Add support to add/remove usb host driver

[Ramneek Mehresh]

> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Friday, August 14, 2015 12:17 AM
> To: Mehresh Ramneek-B31383 <ramneek.mehresh@freescale.com>
> Cc: linux-kernel@vger.kernel.org; balbi@ti.com;
> gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Li Yang-Leo-R58472
> <LeoLi@freescale.com>
> Subject: Re: [PATCH 3/8][v3]usb:fsl:otg: Add support to add/remove usb
> host driver
> 
> On Thu, 13 Aug 2015, Ramneek Mehresh wrote:
> 
> > Add workqueue to add/remove host driver (outside
> > interrupt context) upon each id change.
> >
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
> > ---
> > Changes for v3:
> >         - removed CONFIG_FSL_USB2_OTG and
> CONFIG_FSL_USB2_OTG_MODULE
> >           macros
> >         - removed call to usb_hcd_resume_root_hub(hcd) from
> >           ehci_fsl_drv_resume()
> >
> >  drivers/usb/host/ehci-fsl.c | 71 ++++++++++++++++++++++++++++++++-
> ------------
> >  drivers/usb/host/ehci-fsl.h | 18 ++++++++++++
> >  2 files changed, 69 insertions(+), 20 deletions(-)
> 
> 
> >  static int ehci_fsl_drv_suspend(struct device *dev)
> >  {
> >  	struct usb_hcd *hcd = dev_get_drvdata(dev);
> > -	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> >  	void __iomem *non_ehci = hcd->regs;
> > +	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> > +	struct usb_bus host = hcd->self;
> 
> I just noticed this line.  It does not look right at all; it does a
> structure copy instead of copying a pointer.
my bad...will change to struct usb_bus *host = &hcd->self;
> 
> >
> >  	if (of_device_is_compatible(dev->parent->of_node,
> >  				    "fsl,mpc5121-usb2-dr")) {
> >  		return ehci_fsl_mpc512x_drv_suspend(dev);
> >  	}
> >
> > +	if (host.is_otg) {
> > +		/* remove hcd */
> > +		ehci_fsl->hcd_add = 0;
> > +		schedule_work(&ehci_fsl->change_hcd_work);
> > +		host.is_otg = 0;
> 
> And here you turn off the flag in the local copy of the structure,
> which accomplishes nothing.
> 
agree...will change to host->is_otg = 0;
> > +		return 0;
> > +	}
> > +
> >  	ehci_prepare_ports_for_controller_suspend(hcd_to_ehci(hcd),
> >  			device_may_wakeup(dev));
> >  	if (!fsl_deep_sleep())
> > @@ -557,15 +579,24 @@ static int ehci_fsl_drv_suspend(struct device
> *dev)
> >  static int ehci_fsl_drv_resume(struct device *dev)
> >  {
> >  	struct usb_hcd *hcd = dev_get_drvdata(dev);
> > -	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> >  	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> >  	void __iomem *non_ehci = hcd->regs;
> > +	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> > +	struct usb_bus host = hcd->self;
> 
> Same here.
agree
> 
> > --- a/drivers/usb/host/ehci-fsl.h
> > +++ b/drivers/usb/host/ehci-fsl.h
> > @@ -63,4 +63,22 @@
> >  #define UTMI_PHY_EN             (1<<9)
> >  #define ULPI_PHY_CLK_SEL        (1<<10)
> >  #define PHY_CLK_VALID		(1<<17)
> > +
> > +struct ehci_fsl {
> > +#ifdef CONFIG_PM
> > +	/* Saved USB PHY settings, need to restore after deep sleep. */
> > +	u32 usb_ctrl;
> > +#endif
> 
> Do you need this #ifdef?
> 
Yes, this is required for deep-sleep support...we need to save/restore controller
registers during deep-sleep when usb controller power is shut-off. Don't need this
during normal usb operation...saving/restoring usb controller registers in non deep-sleep
scenario will add unnecessary delays
> Alan Stern

RE: [PATCH 3/8][v3]usb:fsl:otg: Add support to add/remove usb host driver

[Alan Stern]

On Thu, 20 Aug 2015, Ramneek Mehresh wrote:

> > > --- a/drivers/usb/host/ehci-fsl.h
> > > +++ b/drivers/usb/host/ehci-fsl.h
> > > @@ -63,4 +63,22 @@
> > >  #define UTMI_PHY_EN             (1<<9)
> > >  #define ULPI_PHY_CLK_SEL        (1<<10)
> > >  #define PHY_CLK_VALID		(1<<17)
> > > +
> > > +struct ehci_fsl {
> > > +#ifdef CONFIG_PM
> > > +	/* Saved USB PHY settings, need to restore after deep sleep. */
> > > +	u32 usb_ctrl;
> > > +#endif
> > 
> > Do you need this #ifdef?
> > 
> Yes, this is required for deep-sleep support...we need to save/restore controller
> registers during deep-sleep when usb controller power is shut-off. Don't need this
> during normal usb operation...saving/restoring usb controller registers in non deep-sleep
> scenario will add unnecessary delays

What I meant was, can you keep the "u32 usb_ctrl;" line but get rid of
the "#ifdef CONFIG_PM" and "#endif" lines?

Alan Stern

RE: [PATCH 3/8][v3]usb:fsl:otg: Add support to add/remove usb host driver

[Ramneek Mehresh]

> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Thursday, August 20, 2015 7:40 PM
> To: Mehresh Ramneek-B31383 <ramneek.mehresh@freescale.com>
> Cc: linux-kernel@vger.kernel.org; balbi@ti.com;
> gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Li Yang-Leo-R58472
> <LeoLi@freescale.com>
> Subject: RE: [PATCH 3/8][v3]usb:fsl:otg: Add support to add/remove usb
> host driver
> 
> On Thu, 20 Aug 2015, Ramneek Mehresh wrote:
> 
> > > > --- a/drivers/usb/host/ehci-fsl.h
> > > > +++ b/drivers/usb/host/ehci-fsl.h
> > > > @@ -63,4 +63,22 @@
> > > >  #define UTMI_PHY_EN             (1<<9)
> > > >  #define ULPI_PHY_CLK_SEL        (1<<10)
> > > >  #define PHY_CLK_VALID		(1<<17)
> > > > +
> > > > +struct ehci_fsl {
> > > > +#ifdef CONFIG_PM
> > > > +	/* Saved USB PHY settings, need to restore after deep sleep. */
> > > > +	u32 usb_ctrl;
> > > > +#endif
> > >
> > > Do you need this #ifdef?
> > >
> > Yes, this is required for deep-sleep support...we need to save/restore
> controller
> > registers during deep-sleep when usb controller power is shut-off. Don't
> need this
> > during normal usb operation...saving/restoring usb controller registers in
> non deep-sleep
> > scenario will add unnecessary delays
> 
> What I meant was, can you keep the "u32 usb_ctrl;" line but get rid of
> the "#ifdef CONFIG_PM" and "#endif" lines?
> 
> Alan Stern
I do understand that. However, USB suspend/resume functionality work in context of
PM. Only in this context, we need to save/restore usb controller register
for deep-sleep functionality. If you see usage of this in ehci-fsl.c file, it's used in 
ehci_fsl_drv_suspend() under CONFIG_PM to save USB CNTL register.
If I remove CONFIG_PM from struct ehci_fsl{}, I'll need to change the entire driver
also to make suspend and resume functionalities compile by default.
   

RE: [PATCH 3/8][v3]usb:fsl:otg: Add support to add/remove usb host driver

[Alan Stern]

On Mon, 24 Aug 2015, Ramneek Mehresh wrote:

> > On Thu, 20 Aug 2015, Ramneek Mehresh wrote:
> > 
> > > > > --- a/drivers/usb/host/ehci-fsl.h
> > > > > +++ b/drivers/usb/host/ehci-fsl.h
> > > > > @@ -63,4 +63,22 @@
> > > > >  #define UTMI_PHY_EN             (1<<9)
> > > > >  #define ULPI_PHY_CLK_SEL        (1<<10)
> > > > >  #define PHY_CLK_VALID		(1<<17)
> > > > > +
> > > > > +struct ehci_fsl {
> > > > > +#ifdef CONFIG_PM
> > > > > +	/* Saved USB PHY settings, need to restore after deep sleep. */
> > > > > +	u32 usb_ctrl;
> > > > > +#endif
> > > >
> > > > Do you need this #ifdef?
> > > >
> > > Yes, this is required for deep-sleep support...we need to save/restore
> > controller
> > > registers during deep-sleep when usb controller power is shut-off. Don't
> > need this
> > > during normal usb operation...saving/restoring usb controller registers in
> > non deep-sleep
> > > scenario will add unnecessary delays
> > 
> > What I meant was, can you keep the "u32 usb_ctrl;" line but get rid of
> > the "#ifdef CONFIG_PM" and "#endif" lines?
> > 
> > Alan Stern
> I do understand that.

It doesn't sound like you do.

>  However, USB suspend/resume functionality work in context of
> PM. Only in this context, we need to save/restore usb controller register
> for deep-sleep functionality. If you see usage of this in ehci-fsl.c file, it's used in 
> ehci_fsl_drv_suspend() under CONFIG_PM to save USB CNTL register.

Yes, I know.

> If I remove CONFIG_PM from struct ehci_fsl{}, I'll need to change the entire driver
> also to make suspend and resume functionalities compile by default.

Why?  Suppose you remove the "#ifdef CONFIG_PM" and "#endif" lines but 
leave everything else the same.  Won't the driver still work?

If CONFIG_PM is enabled then everything will be exactly the same as it 
is now.  If CONFIG_PM isn't enabled then you will have an extra 
usb_ctrl field in the ehci_fsl structure.  It will never get used for 
anything but the driver should still work.  Right?

Alan Stern

Re: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver

[Fabio Estevam]

On Mon, Aug 10, 2015 at 12:35 PM, Ramneek Mehresh
<ramneek.mehresh@freescale.com> wrote:

> Understood...let me check this once again. Thanks.

Why don't use the chipidea driver instead?

RE: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver

[Ramneek Mehresh]

> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Monday, August 10, 2015 8:14 PM
> To: Mehresh Ramneek-B31383
> Cc: linux-kernel@vger.kernel.org; balbi@ti.com;
> gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Li Yang-Leo-R58472
> Subject: RE: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb
> host driver
> 
> On Mon, 10 Aug 2015, Ramneek Mehresh wrote:
> 
> > > Also, what is the reason for calling usb_hcd_resume_root_hub()?  It
> won't
> > > do anything, because it will run before the scheduled work, so there
> won't
> > > be a root hub for it to resume.
> > >
> > Well, you're right...it's not a surety that usb_hcd_resume_root_hub() will
> run after
> > scheduled work...i'll move it inside do_change_hcd() after usb_add_hcd().
> Thanks.
> 
> You're missing the point.  There's no reason ever to call
> usb_hcd_resume_root_hub() immediately after usb_add_hcd().  The hub
> driver does everything necessary when the root hub is registered.
> Resuming it (if it is suspended at that point) won't accomplish
> anything because all the important work has just been done.
> 
> Now, if you _weren't_ calling usb_add_hcd() (for example if host.is_otg
> were 0) then it might make sense to call usb_hcd_resume_root_hub() --
> although I don't see why you would need to.
> 
> Alan Stern
Understood...let me check this once again. Thanks.

RE: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver

[Alan Stern]

On Mon, 10 Aug 2015, Ramneek Mehresh wrote:

> > Also, what is the reason for calling usb_hcd_resume_root_hub()?  It won't
> > do anything, because it will run before the scheduled work, so there won't
> > be a root hub for it to resume.
> > 
> Well, you're right...it's not a surety that usb_hcd_resume_root_hub() will run after
> scheduled work...i'll move it inside do_change_hcd() after usb_add_hcd(). Thanks.

You're missing the point.  There's no reason ever to call 
usb_hcd_resume_root_hub() immediately after usb_add_hcd().  The hub 
driver does everything necessary when the root hub is registered.  
Resuming it (if it is suspended at that point) won't accomplish 
anything because all the important work has just been done.

Now, if you _weren't_ calling usb_add_hcd() (for example if host.is_otg
were 0) then it might make sense to call usb_hcd_resume_root_hub() --
although I don't see why you would need to.

Alan Stern

Re: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

On Fri, Aug 07, 2015 at 04:33:33PM -0400, Alan Stern wrote:
> On Wed, 15 Jul 2015, Ramneek Mehresh wrote:
> 
> > Add workqueue to add/remove host driver (outside
> > interrupt context) upon each id change.
> > 
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
> > ---
> >  drivers/usb/host/ehci-fsl.c | 83 ++++++++++++++++++++++++++++++++++-----------
> >  drivers/usb/host/ehci-fsl.h | 20 +++++++++++
> >  2 files changed, 84 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> > index 5352e74..81e4bf5 100644
> > --- a/drivers/usb/host/ehci-fsl.c
> > +++ b/drivers/usb/host/ehci-fsl.c
> > @@ -44,6 +44,34 @@
> >  
> >  static struct hc_driver __read_mostly fsl_ehci_hc_driver;
> >  
> > +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> 
> You've got these #if lines all over the place.  They look ugly and make
> the code hard to read.  Consider removing them.  Or even if you can't
> remove them entirely, removing most of them would help.
> 
> Also, instead of testing both CONFIG_FSL_USB2_OTG and
> CONFIG_FSL_USB2_OTG_MODULE, how about testing a single symbol?  For 
> example:
> 
> #if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> #define CHANGE_HCD 1
> #else
> #define CHANGE_HCD 0
> #endif
> 
> Then all you need later on is "#if CHANGE_HCD".  Or if it's inside a 
> code block, just "if (CHANGE_HCD)".

what about IS_ENABLED() ?

if (IS_ENABLED(CONFIG_FSL_USB2_OTG)) {
	foo();
} else {
	bar();
}

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver

[Ramneek Mehresh]

> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Saturday, August 08, 2015 2:04 AM
> To: Mehresh Ramneek-B31383
> Cc: linux-kernel@vger.kernel.org; balbi@ti.com;
> gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Li Yang-Leo-R58472
> Subject: Re: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb
> host driver
> 
> On Wed, 15 Jul 2015, Ramneek Mehresh wrote:
> 
> > Add workqueue to add/remove host driver (outside interrupt context)
> > upon each id change.
> >
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
> > ---
> >  drivers/usb/host/ehci-fsl.c | 83
> > ++++++++++++++++++++++++++++++++++-----------
> >  drivers/usb/host/ehci-fsl.h | 20 +++++++++++
> >  2 files changed, 84 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> > index 5352e74..81e4bf5 100644
> > --- a/drivers/usb/host/ehci-fsl.c
> > +++ b/drivers/usb/host/ehci-fsl.c
> > @@ -44,6 +44,34 @@
> >
> >  static struct hc_driver __read_mostly fsl_ehci_hc_driver;
> >
> > +#if defined(CONFIG_FSL_USB2_OTG) ||
> > +defined(CONFIG_FSL_USB2_OTG_MODULE)
> 
> You've got these #if lines all over the place.  They look ugly and make the
> code hard to read.  Consider removing them.  Or even if you can't remove
> them entirely, removing most of them would help.
> 
> Also, instead of testing both CONFIG_FSL_USB2_OTG and
> CONFIG_FSL_USB2_OTG_MODULE, how about testing a single symbol?  For
> example:
> 
> #if defined(CONFIG_FSL_USB2_OTG) ||
> defined(CONFIG_FSL_USB2_OTG_MODULE)
> #define CHANGE_HCD 1
> #else
> #define CHANGE_HCD 0
> #endif
> 
> Then all you need later on is "#if CHANGE_HCD".  Or if it's inside a code block,
> just "if (CHANGE_HCD)".
> 
Ok, let me see what can be done to minimize these macros. Thanks

> > +static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd) {
> > +	return (struct ehci_fsl *)hcd_to_ehci(hcd)->priv; }
> > +
> > +static void do_change_hcd(struct work_struct *work) {
> > +	struct ehci_fsl *ehci_fsl = container_of(work, struct ehci_fsl,
> > +					change_hcd_work);
> > +	struct usb_hcd *hcd = ehci_fsl->hcd;
> > +	void __iomem *non_ehci = hcd->regs;
> > +	int retval;
> > +
> > +	if (ehci_fsl->hcd_add && !ehci_fsl->have_hcd) {
> > +		writel(USBMODE_CM_HOST, non_ehci +
> FSL_SOC_USB_USBMODE);
> > +		/* host, gadget and otg share same int line */
> > +		retval = usb_add_hcd(hcd, hcd->irq, IRQF_SHARED);
> > +		if (retval == 0)
> > +			ehci_fsl->have_hcd = 1;
> > +	} else if (!ehci_fsl->hcd_add && ehci_fsl->have_hcd) {
> > +		usb_remove_hcd(hcd);
> > +		ehci_fsl->have_hcd = 0;
> 
> Don't you have to turn off the USBMODE_CM_HOST bit here?  It looks
> strange to turn it on above but not turn it off again.
> 
USBMODE bit defines mode for the controller. It gets auto cleared when controller is 
reset in host/gadget driver. After resetting, it's mandatory to write this bit once to define
controller's mode. 
> > +	}
> > +}
> > +#endif
> 
> 
> >  static int ehci_fsl_drv_suspend(struct device *dev)  {
> >  	struct usb_hcd *hcd = dev_get_drvdata(dev);
> > -	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> >  	void __iomem *non_ehci = hcd->regs;
> > +#if defined(CONFIG_FSL_USB2_OTG) ||
> defined(CONFIG_FSL_USB2_OTG_MODULE)
> > +	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> > +	struct usb_bus host = hcd->self;
> > +#endif
> > +
> >
> >  	if (of_device_is_compatible(dev->parent->of_node,
> >  				    "fsl,mpc5121-usb2-dr")) {
> >  		return ehci_fsl_mpc512x_drv_suspend(dev);
> >  	}
> >
> > +#if defined(CONFIG_FSL_USB2_OTG) ||
> defined(CONFIG_FSL_USB2_OTG_MODULE)
> > +	if (host.is_otg) {
> > +		/* remove hcd */
> > +		ehci_fsl->hcd_add = 0;
> > +		schedule_work(&ehci_fsl->change_hcd_work);
> > +		host.is_otg = 0;
> 
> Why do you set host.is_otg to 0 here?  Why not do it in the work routine?
Well, it can be done there also. I just wanted to keep host-bus house-keeping out
of otg role-changing specific workqueue (where host driver is brought-up/shut-down). 
> 
> > +		return 0;
> > +	}
> > +#endif
> > +
> >  	ehci_prepare_ports_for_controller_suspend(hcd_to_ehci(hcd),
> >  			device_may_wakeup(dev));
> >  	if (!fsl_deep_sleep())
> > @@ -540,15 +571,29 @@ static int ehci_fsl_drv_suspend(struct device
> > *dev)  static int ehci_fsl_drv_resume(struct device *dev)  {
> >  	struct usb_hcd *hcd = dev_get_drvdata(dev);
> > -	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> >  	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> >  	void __iomem *non_ehci = hcd->regs;
> > +#if defined(CONFIG_FSL_USB2_OTG) ||
> defined(CONFIG_FSL_USB2_OTG_MODULE)
> > +	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> > +	struct usb_bus host = hcd->self;
> > +#endif
> >
> >  	if (of_device_is_compatible(dev->parent->of_node,
> >  				    "fsl,mpc5121-usb2-dr")) {
> >  		return ehci_fsl_mpc512x_drv_resume(dev);
> >  	}
> >
> > +#if defined(CONFIG_FSL_USB2_OTG) ||
> defined(CONFIG_FSL_USB2_OTG_MODULE)
> > +	if (host.is_otg) {
> > +		/* add hcd */
> > +		ehci_fsl->hcd_add = 1;
> > +		schedule_work(&ehci_fsl->change_hcd_work);
> > +		usb_hcd_resume_root_hub(hcd);
> > +		host.is_otg = 0;
> 
> Again, why change host.is_otg here?  And for that matter, where does
> host.is_otg ever get set to 1?
host.is_otg sets to 1 in phy driver before calling ehci drv suspend/resume.
This is donr to make it clear in host drv suspend/resume that these functions
Are called in context of otg drv 
> 
> Also, what is the reason for calling usb_hcd_resume_root_hub()?  It won't
> do anything, because it will run before the scheduled work, so there won't
> be a root hub for it to resume.
> 
Well, you're right...it's not a surety that usb_hcd_resume_root_hub() will run after
scheduled work...i'll move it inside do_change_hcd() after usb_add_hcd(). Thanks.
> > +		return 0;
> > +	}
> > +#endif
> > +
> >  	ehci_prepare_ports_for_controller_resume(ehci);
> >  	if (!fsl_deep_sleep())
> >  		return 0;
> > diff --git a/drivers/usb/host/ehci-fsl.h b/drivers/usb/host/ehci-fsl.h
> > index dbd292e..3fd1fd0 100644
> > --- a/drivers/usb/host/ehci-fsl.h
> > +++ b/drivers/usb/host/ehci-fsl.h
> > @@ -62,4 +62,24 @@
> >  #define UTMI_PHY_EN             (1<<9)
> >  #define ULPI_PHY_CLK_SEL        (1<<10)
> >  #define PHY_CLK_VALID		(1<<17)
> > +
> > +struct ehci_fsl {
> > +#ifdef CONFIG_PM
> > +	/* Saved USB PHY settings, need to restore after deep sleep. */
> > +	u32 usb_ctrl;
> > +#endif
> > +	struct usb_hcd *hcd;
> > +#if defined(CONFIG_FSL_USB2_OTG) ||
> defined(CONFIG_FSL_USB2_OTG_MODULE)
> > +	struct work_struct change_hcd_work;
> > +#endif
> 
> Again, try to eliminate these #if's.  There really isn't anything wrong with
> allocating the space for these things even in a non-OTG build.
> 
> > +	/*
> > +	 * store current hcd state for otg;
> > +	 * have_hcd is true when host drv al already part of otg framework,
> > +	 * otherwise false;
> > +	 * hcd_add is true when otg framework wants to add host
> > +	 * drv as part of otg;flase when it wants to remove it
> > +	 */
> > +	unsigned have_hcd:1;
> > +	unsigned hcd_add:1;
> > +};
> >  #endif				/* _EHCI_FSL_H */
> 
> Alan Stern

Re: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver

[Alan Stern]

On Wed, 15 Jul 2015, Ramneek Mehresh wrote:

> Add workqueue to add/remove host driver (outside
> interrupt context) upon each id change.
> 
> Signed-off-by: Li Yang <leoli@freescale.com>
> Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
> ---
>  drivers/usb/host/ehci-fsl.c | 83 ++++++++++++++++++++++++++++++++++-----------
>  drivers/usb/host/ehci-fsl.h | 20 +++++++++++
>  2 files changed, 84 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index 5352e74..81e4bf5 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -44,6 +44,34 @@
>  
>  static struct hc_driver __read_mostly fsl_ehci_hc_driver;
>  
> +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)

You've got these #if lines all over the place.  They look ugly and make
the code hard to read.  Consider removing them.  Or even if you can't
remove them entirely, removing most of them would help.

Also, instead of testing both CONFIG_FSL_USB2_OTG and
CONFIG_FSL_USB2_OTG_MODULE, how about testing a single symbol?  For 
example:

#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
#define CHANGE_HCD 1
#else
#define CHANGE_HCD 0
#endif

Then all you need later on is "#if CHANGE_HCD".  Or if it's inside a 
code block, just "if (CHANGE_HCD)".

> +static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd)
> +{
> +	return (struct ehci_fsl *)hcd_to_ehci(hcd)->priv;
> +}
> +
> +static void do_change_hcd(struct work_struct *work)
> +{
> +	struct ehci_fsl *ehci_fsl = container_of(work, struct ehci_fsl,
> +					change_hcd_work);
> +	struct usb_hcd *hcd = ehci_fsl->hcd;
> +	void __iomem *non_ehci = hcd->regs;
> +	int retval;
> +
> +	if (ehci_fsl->hcd_add && !ehci_fsl->have_hcd) {
> +		writel(USBMODE_CM_HOST, non_ehci + FSL_SOC_USB_USBMODE);
> +		/* host, gadget and otg share same int line */
> +		retval = usb_add_hcd(hcd, hcd->irq, IRQF_SHARED);
> +		if (retval == 0)
> +			ehci_fsl->have_hcd = 1;
> +	} else if (!ehci_fsl->hcd_add && ehci_fsl->have_hcd) {
> +		usb_remove_hcd(hcd);
> +		ehci_fsl->have_hcd = 0;

Don't you have to turn off the USBMODE_CM_HOST bit here?  It looks 
strange to turn it on above but not turn it off again.

> +	}
> +}
> +#endif


>  static int ehci_fsl_drv_suspend(struct device *dev)
>  {
>  	struct usb_hcd *hcd = dev_get_drvdata(dev);
> -	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
>  	void __iomem *non_ehci = hcd->regs;
> +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> +	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> +	struct usb_bus host = hcd->self;
> +#endif
> +
>  
>  	if (of_device_is_compatible(dev->parent->of_node,
>  				    "fsl,mpc5121-usb2-dr")) {
>  		return ehci_fsl_mpc512x_drv_suspend(dev);
>  	}
>  
> +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> +	if (host.is_otg) {
> +		/* remove hcd */
> +		ehci_fsl->hcd_add = 0;
> +		schedule_work(&ehci_fsl->change_hcd_work);
> +		host.is_otg = 0;

Why do you set host.is_otg to 0 here?  Why not do it in the work 
routine?

> +		return 0;
> +	}
> +#endif
> +
>  	ehci_prepare_ports_for_controller_suspend(hcd_to_ehci(hcd),
>  			device_may_wakeup(dev));
>  	if (!fsl_deep_sleep())
> @@ -540,15 +571,29 @@ static int ehci_fsl_drv_suspend(struct device *dev)
>  static int ehci_fsl_drv_resume(struct device *dev)
>  {
>  	struct usb_hcd *hcd = dev_get_drvdata(dev);
> -	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
>  	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>  	void __iomem *non_ehci = hcd->regs;
> +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> +	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> +	struct usb_bus host = hcd->self;
> +#endif
>  
>  	if (of_device_is_compatible(dev->parent->of_node,
>  				    "fsl,mpc5121-usb2-dr")) {
>  		return ehci_fsl_mpc512x_drv_resume(dev);
>  	}
>  
> +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> +	if (host.is_otg) {
> +		/* add hcd */
> +		ehci_fsl->hcd_add = 1;
> +		schedule_work(&ehci_fsl->change_hcd_work);
> +		usb_hcd_resume_root_hub(hcd);
> +		host.is_otg = 0;

Again, why change host.is_otg here?  And for that matter, where does 
host.is_otg ever get set to 1?

Also, what is the reason for calling usb_hcd_resume_root_hub()?  It 
won't do anything, because it will run before the scheduled work, so 
there won't be a root hub for it to resume.

> +		return 0;
> +	}
> +#endif
> +
>  	ehci_prepare_ports_for_controller_resume(ehci);
>  	if (!fsl_deep_sleep())
>  		return 0;
> diff --git a/drivers/usb/host/ehci-fsl.h b/drivers/usb/host/ehci-fsl.h
> index dbd292e..3fd1fd0 100644
> --- a/drivers/usb/host/ehci-fsl.h
> +++ b/drivers/usb/host/ehci-fsl.h
> @@ -62,4 +62,24 @@
>  #define UTMI_PHY_EN             (1<<9)
>  #define ULPI_PHY_CLK_SEL        (1<<10)
>  #define PHY_CLK_VALID		(1<<17)
> +
> +struct ehci_fsl {
> +#ifdef CONFIG_PM
> +	/* Saved USB PHY settings, need to restore after deep sleep. */
> +	u32 usb_ctrl;
> +#endif
> +	struct usb_hcd *hcd;
> +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> +	struct work_struct change_hcd_work;
> +#endif

Again, try to eliminate these #if's.  There really isn't anything wrong 
with allocating the space for these things even in a non-OTG build.

> +	/*
> +	 * store current hcd state for otg;
> +	 * have_hcd is true when host drv al already part of otg framework,
> +	 * otherwise false;
> +	 * hcd_add is true when otg framework wants to add host
> +	 * drv as part of otg;flase when it wants to remove it
> +	 */
> +	unsigned have_hcd:1;
> +	unsigned hcd_add:1;
> +};
>  #endif				/* _EHCI_FSL_H */

Alan Stern

[PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver

[Ramneek Mehresh]

Add workqueue to add/remove host driver (outside
interrupt context) upon each id change.

Signed-off-by: Li Yang <leoli@freescale.com>
Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
---
 drivers/usb/host/ehci-fsl.c | 83 ++++++++++++++++++++++++++++++++++-----------
 drivers/usb/host/ehci-fsl.h | 20 +++++++++++
 2 files changed, 84 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 5352e74..81e4bf5 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -44,6 +44,34 @@
 
 static struct hc_driver __read_mostly fsl_ehci_hc_driver;
 
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
+static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd)
+{
+	return (struct ehci_fsl *)hcd_to_ehci(hcd)->priv;
+}
+
+static void do_change_hcd(struct work_struct *work)
+{
+	struct ehci_fsl *ehci_fsl = container_of(work, struct ehci_fsl,
+					change_hcd_work);
+	struct usb_hcd *hcd = ehci_fsl->hcd;
+	void __iomem *non_ehci = hcd->regs;
+	int retval;
+
+	if (ehci_fsl->hcd_add && !ehci_fsl->have_hcd) {
+		writel(USBMODE_CM_HOST, non_ehci + FSL_SOC_USB_USBMODE);
+		/* host, gadget and otg share same int line */
+		retval = usb_add_hcd(hcd, hcd->irq, IRQF_SHARED);
+		if (retval == 0)
+			ehci_fsl->have_hcd = 1;
+	} else if (!ehci_fsl->hcd_add && ehci_fsl->have_hcd) {
+		usb_remove_hcd(hcd);
+		ehci_fsl->have_hcd = 0;
+	}
+}
+#endif
+
+
 /* configure so an HC device and id are always provided */
 /* always called with process context; sleeping is OK */
 
@@ -136,11 +164,16 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
 		goto err2;
 	device_wakeup_enable(hcd->self.controller);
 
-#ifdef CONFIG_USB_OTG
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
 	if (pdata->operating_mode == FSL_USB2_DR_OTG) {
 		struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+		struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 
+		ehci_fsl->hcd = hcd;
 		hcd->usb_phy = usb_get_phy(USB_PHY_TYPE_USB2);
+
+		INIT_WORK(&ehci_fsl->change_hcd_work, do_change_hcd);
+
 		dev_dbg(&pdev->dev, "hcd=0x%p  ehci=0x%p, phy=0x%p\n",
 			hcd, ehci, hcd->usb_phy);
 
@@ -354,15 +387,6 @@ static int ehci_fsl_setup(struct usb_hcd *hcd)
 	return retval;
 }
 
-struct ehci_fsl {
-	struct ehci_hcd	ehci;
-
-#ifdef CONFIG_PM
-	/* Saved USB PHY settings, need to restore after deep sleep. */
-	u32 usb_ctrl;
-#endif
-};
-
 #ifdef CONFIG_PM
 
 #ifdef CONFIG_PPC_MPC512x
@@ -510,24 +534,31 @@ static inline int ehci_fsl_mpc512x_drv_resume(struct device *dev)
 }
 #endif /* CONFIG_PPC_MPC512x */
 
-static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd)
-{
-	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
-
-	return container_of(ehci, struct ehci_fsl, ehci);
-}
-
 static int ehci_fsl_drv_suspend(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
-	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 	void __iomem *non_ehci = hcd->regs;
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
+	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
+	struct usb_bus host = hcd->self;
+#endif
+
 
 	if (of_device_is_compatible(dev->parent->of_node,
 				    "fsl,mpc5121-usb2-dr")) {
 		return ehci_fsl_mpc512x_drv_suspend(dev);
 	}
 
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
+	if (host.is_otg) {
+		/* remove hcd */
+		ehci_fsl->hcd_add = 0;
+		schedule_work(&ehci_fsl->change_hcd_work);
+		host.is_otg = 0;
+		return 0;
+	}
+#endif
+
 	ehci_prepare_ports_for_controller_suspend(hcd_to_ehci(hcd),
 			device_may_wakeup(dev));
 	if (!fsl_deep_sleep())
@@ -540,15 +571,29 @@ static int ehci_fsl_drv_suspend(struct device *dev)
 static int ehci_fsl_drv_resume(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
-	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 	void __iomem *non_ehci = hcd->regs;
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
+	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
+	struct usb_bus host = hcd->self;
+#endif
 
 	if (of_device_is_compatible(dev->parent->of_node,
 				    "fsl,mpc5121-usb2-dr")) {
 		return ehci_fsl_mpc512x_drv_resume(dev);
 	}
 
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
+	if (host.is_otg) {
+		/* add hcd */
+		ehci_fsl->hcd_add = 1;
+		schedule_work(&ehci_fsl->change_hcd_work);
+		usb_hcd_resume_root_hub(hcd);
+		host.is_otg = 0;
+		return 0;
+	}
+#endif
+
 	ehci_prepare_ports_for_controller_resume(ehci);
 	if (!fsl_deep_sleep())
 		return 0;
diff --git a/drivers/usb/host/ehci-fsl.h b/drivers/usb/host/ehci-fsl.h
index dbd292e..3fd1fd0 100644
--- a/drivers/usb/host/ehci-fsl.h
+++ b/drivers/usb/host/ehci-fsl.h
@@ -62,4 +62,24 @@
 #define UTMI_PHY_EN             (1<<9)
 #define ULPI_PHY_CLK_SEL        (1<<10)
 #define PHY_CLK_VALID		(1<<17)
+
+struct ehci_fsl {
+#ifdef CONFIG_PM
+	/* Saved USB PHY settings, need to restore after deep sleep. */
+	u32 usb_ctrl;
+#endif
+	struct usb_hcd *hcd;
+#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
+	struct work_struct change_hcd_work;
+#endif
+	/*
+	 * store current hcd state for otg;
+	 * have_hcd is true when host drv al already part of otg framework,
+	 * otherwise false;
+	 * hcd_add is true when otg framework wants to add host
+	 * drv as part of otg;flase when it wants to remove it
+	 */
+	unsigned have_hcd:1;
+	unsigned hcd_add:1;
+};
 #endif				/* _EHCI_FSL_H */
-- 
1.8.3.1