[PATCH 0/3] Generic ULPI driver extention

[PATCH 0/3] Generic ULPI driver extention

[Igor Grinberg]

This patch series add support for SMSC USB3319 ulpi phy,
introduces the ulpi specific flags for control over the
ulpi phy and extends the generic ulpi driver with
support for Function and Interface control of upli phy.

Depends on:
From: Ajay Kumar Gupta <ajay.gupta@ti.com>
Date: Thu,  8 Jul 2010 14:03:01 +0530
Subject: USB: ulpi: fix compilation warning

From: Eric B?nard <eric@eukrea.com>
Date: Thu, 15 Jul 2010 09:20:19 +0200
Subject: [PATCH v2] otg/ulpi.c : fix register write

Igor Grinberg (3):
  usb/otg/ulpi: remove unused macro
  usb/otg/ulpi: add support for SMSC USB3319 ulpi phy
  usb/otg/ulpi: extend the generic ulpi driver.

 arch/arm/mach-mx2/mach-pca100.c          |    4 +-
 arch/arm/mach-mx3/mach-armadillo5x0.c    |    4 +-
 arch/arm/mach-mx3/mach-mx31lilly.c       |    4 +-
 arch/arm/mach-mx3/mach-mx31lite.c        |    2 +-
 arch/arm/mach-mx3/mach-mx31moboard.c     |    2 +-
 arch/arm/mach-mx3/mach-pcm037.c          |    4 +-
 arch/arm/mach-mx3/mach-pcm043.c          |    2 +-
 arch/arm/mach-mx3/mx31moboard-smartbot.c |    2 +-
 drivers/usb/otg/Kconfig                  |    2 -
 drivers/usb/otg/ulpi.c                   |  130 +++++++++++++++++++++++++++---
 include/linux/usb/otg.h                  |    7 --
 include/linux/usb/ulpi.h                 |   39 +++++++++
 12 files changed, 169 insertions(+), 33 deletions(-)

[PATCH 1/3] usb/otg/ulpi: remove unused macro

[Igor Grinberg]

Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
Signed-off-by: Mike Rapoport <mike@compulab.co.il>
---
 drivers/usb/otg/ulpi.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
index 10a1df6..448d643 100644
--- a/drivers/usb/otg/ulpi.c
+++ b/drivers/usb/otg/ulpi.c
@@ -31,8 +31,6 @@
 
 #define ULPI_ID(vendor, product) (((vendor) << 16) | (product))
 
-#define TR_FLAG(flags, a, b)	(((flags) & a) ? b : 0)
-
 /* ULPI hardcoded IDs, used for probing */
 static unsigned int ulpi_ids[] = {
 	ULPI_ID(0x04cc, 0x1504),	/* NXP ISP1504 */
-- 
1.7.1

[PATCH 2/3] usb/otg/ulpi: add support for SMSC USB3319 ulpi phy

[Igor Grinberg]

Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
Signed-off-by: Mike Rapoport <mike@compulab.co.il>
---
 drivers/usb/otg/Kconfig |    2 --
 drivers/usb/otg/ulpi.c  |    1 +
 2 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/otg/Kconfig b/drivers/usb/otg/Kconfig
index 3d2d3e5..3b12895 100644
--- a/drivers/usb/otg/Kconfig
+++ b/drivers/usb/otg/Kconfig
@@ -49,8 +49,6 @@ config USB_ULPI
 	  Enable this to support ULPI connected USB OTG transceivers which
 	  are likely found on embedded boards.
 
-	  The only chip currently supported is NXP's ISP1504
-
 config TWL4030_USB
 	tristate "TWL4030 USB Transceiver Driver"
 	depends on TWL4030_CORE && REGULATOR_TWL4030
diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
index 448d643..ef7dbe4 100644
--- a/drivers/usb/otg/ulpi.c
+++ b/drivers/usb/otg/ulpi.c
@@ -34,6 +34,7 @@
 /* ULPI hardcoded IDs, used for probing */
 static unsigned int ulpi_ids[] = {
 	ULPI_ID(0x04cc, 0x1504),	/* NXP ISP1504 */
+	ULPI_ID(0x0424, 0x0006),        /* SMSC USB3319 */
 };
 
 static int ulpi_set_flags(struct otg_transceiver *otg)
-- 
1.7.1

[PATCH 3/3] usb/otg/ulpi: extend the generic ulpi driver.

[Igor Grinberg]

1) Introduce ulpi specific flags for control of the ulpi phy
2) Extend the generic ulpi driver with support for Function and
Interface control of upli phy
3) Update the platforms using the generic ulpi driver with new ulpi
flags
4) Remove the otg control flags not in use

Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
Signed-off-by: Mike Rapoport <mike@compulab.co.il>
---
 arch/arm/mach-mx2/mach-pca100.c          |    4 +-
 arch/arm/mach-mx3/mach-armadillo5x0.c    |    4 +-
 arch/arm/mach-mx3/mach-mx31lilly.c       |    4 +-
 arch/arm/mach-mx3/mach-mx31lite.c        |    2 +-
 arch/arm/mach-mx3/mach-mx31moboard.c     |    2 +-
 arch/arm/mach-mx3/mach-pcm037.c          |    4 +-
 arch/arm/mach-mx3/mach-pcm043.c          |    2 +-
 arch/arm/mach-mx3/mx31moboard-smartbot.c |    2 +-
 drivers/usb/otg/ulpi.c                   |  127 +++++++++++++++++++++++++++---
 include/linux/usb/otg.h                  |    7 --
 include/linux/usb/ulpi.h                 |   39 +++++++++
 11 files changed, 168 insertions(+), 29 deletions(-)

diff --git a/arch/arm/mach-mx2/mach-pca100.c b/arch/arm/mach-mx2/mach-pca100.c
index a87422e..09f0d7c 100644
--- a/arch/arm/mach-mx2/mach-pca100.c
+++ b/arch/arm/mach-mx2/mach-pca100.c
@@ -357,13 +357,13 @@ static void __init pca100_init(void)
 #if defined(CONFIG_USB_ULPI)
 	if (otg_mode_host) {
 		otg_pdata.otg = otg_ulpi_create(&mxc_ulpi_access_ops,
-				USB_OTG_DRV_VBUS | USB_OTG_DRV_VBUS_EXT);
+				ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
 
 		mxc_register_device(&mxc_otg_host, &otg_pdata);
 	}
 
 	usbh2_pdata.otg = otg_ulpi_create(&mxc_ulpi_access_ops,
-				USB_OTG_DRV_VBUS | USB_OTG_DRV_VBUS_EXT);
+				ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
 
 	mxc_register_device(&mxc_usbh2, &usbh2_pdata);
 #endif
diff --git a/arch/arm/mach-mx3/mach-armadillo5x0.c b/arch/arm/mach-mx3/mach-armadillo5x0.c
index 5f72ec9..b400686 100644
--- a/arch/arm/mach-mx3/mach-armadillo5x0.c
+++ b/arch/arm/mach-mx3/mach-armadillo5x0.c
@@ -552,9 +552,9 @@ static void __init armadillo5x0_init(void)
 	/* USB */
 #if defined(CONFIG_USB_ULPI)
 	usbotg_pdata.otg = otg_ulpi_create(&mxc_ulpi_access_ops,
-			USB_OTG_DRV_VBUS | USB_OTG_DRV_VBUS_EXT);
+			ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
 	usbh2_pdata.otg = otg_ulpi_create(&mxc_ulpi_access_ops,
-			USB_OTG_DRV_VBUS | USB_OTG_DRV_VBUS_EXT);
+			ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
 
 	mxc_register_device(&mxc_otg_host, &usbotg_pdata);
 	mxc_register_device(&mxc_usbh2, &usbh2_pdata);
diff --git a/arch/arm/mach-mx3/mach-mx31lilly.c b/arch/arm/mach-mx3/mach-mx31lilly.c
index b2c7f51..6187988 100644
--- a/arch/arm/mach-mx3/mach-mx31lilly.c
+++ b/arch/arm/mach-mx3/mach-mx31lilly.c
@@ -249,9 +249,9 @@ static struct mxc_usbh_platform_data usbh2_pdata = {
 static void lilly1131_usb_init(void)
 {
 	usbotg_pdata.otg = otg_ulpi_create(&mxc_ulpi_access_ops,
-				USB_OTG_DRV_VBUS | USB_OTG_DRV_VBUS_EXT);
+				ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
 	usbh2_pdata.otg = otg_ulpi_create(&mxc_ulpi_access_ops,
-				USB_OTG_DRV_VBUS | USB_OTG_DRV_VBUS_EXT);
+				ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
 
 	mxc_register_device(&mxc_usbh1, &usbh1_pdata);
 	mxc_register_device(&mxc_usbh2, &usbh2_pdata);
diff --git a/arch/arm/mach-mx3/mach-mx31lite.c b/arch/arm/mach-mx3/mach-mx31lite.c
index 2b6d114..4bee3c5 100644
--- a/arch/arm/mach-mx3/mach-mx31lite.c
+++ b/arch/arm/mach-mx3/mach-mx31lite.c
@@ -261,7 +261,7 @@ static void __init mxc_board_init(void)
 #if defined(CONFIG_USB_ULPI)
 	/* USB */
 	usbh2_pdata.otg = otg_ulpi_create(&mxc_ulpi_access_ops,
-				USB_OTG_DRV_VBUS | USB_OTG_DRV_VBUS_EXT);
+				ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
 
 	mxc_register_device(&mxc_usbh2, &usbh2_pdata);
 #endif
diff --git a/arch/arm/mach-mx3/mach-mx31moboard.c b/arch/arm/mach-mx3/mach-mx31moboard.c
index 62b5e40..8821672 100644
--- a/arch/arm/mach-mx3/mach-mx31moboard.c
+++ b/arch/arm/mach-mx3/mach-mx31moboard.c
@@ -405,7 +405,7 @@ static struct mxc_usbh_platform_data usbh2_pdata = {
 static int __init moboard_usbh2_init(void)
 {
 	usbh2_pdata.otg = otg_ulpi_create(&mxc_ulpi_access_ops,
-			USB_OTG_DRV_VBUS | USB_OTG_DRV_VBUS_EXT);
+			ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
 
 	return mxc_register_device(&mxc_usbh2, &usbh2_pdata);
 }
diff --git a/arch/arm/mach-mx3/mach-pcm037.c b/arch/arm/mach-mx3/mach-pcm037.c
index cce4106..9718688 100644
--- a/arch/arm/mach-mx3/mach-pcm037.c
+++ b/arch/arm/mach-mx3/mach-pcm037.c
@@ -658,13 +658,13 @@ static void __init mxc_board_init(void)
 #if defined(CONFIG_USB_ULPI)
 	if (otg_mode_host) {
 		otg_pdata.otg = otg_ulpi_create(&mxc_ulpi_access_ops,
-				USB_OTG_DRV_VBUS | USB_OTG_DRV_VBUS_EXT);
+				ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
 
 		mxc_register_device(&mxc_otg_host, &otg_pdata);
 	}
 
 	usbh2_pdata.otg = otg_ulpi_create(&mxc_ulpi_access_ops,
-				USB_OTG_DRV_VBUS | USB_OTG_DRV_VBUS_EXT);
+				ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
 
 	mxc_register_device(&mxc_usbh2, &usbh2_pdata);
 #endif
diff --git a/arch/arm/mach-mx3/mach-pcm043.c b/arch/arm/mach-mx3/mach-pcm043.c
index 78d9185..5c80242 100644
--- a/arch/arm/mach-mx3/mach-pcm043.c
+++ b/arch/arm/mach-mx3/mach-pcm043.c
@@ -380,7 +380,7 @@ static void __init mxc_board_init(void)
 #if defined(CONFIG_USB_ULPI)
 	if (otg_mode_host) {
 		otg_pdata.otg = otg_ulpi_create(&mxc_ulpi_access_ops,
-				USB_OTG_DRV_VBUS | USB_OTG_DRV_VBUS_EXT);
+				ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
 
 		mxc_register_device(&mxc_otg_host, &otg_pdata);
 	}
diff --git a/arch/arm/mach-mx3/mx31moboard-smartbot.c b/arch/arm/mach-mx3/mx31moboard-smartbot.c
index 293eea6..53c9ad5 100644
--- a/arch/arm/mach-mx3/mx31moboard-smartbot.c
+++ b/arch/arm/mach-mx3/mx31moboard-smartbot.c
@@ -138,7 +138,7 @@ static struct mxc_usbh_platform_data otg_host_pdata = {
 static int __init smartbot_otg_host_init(void)
 {
 	otg_host_pdata.otg = otg_ulpi_create(&mxc_ulpi_access_ops,
-			USB_OTG_DRV_VBUS | USB_OTG_DRV_VBUS_EXT);
+			ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
 
 	return mxc_register_device(&mxc_otg_host, &otg_host_pdata);
 }
diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
index ef7dbe4..ccc8195 100644
--- a/drivers/usb/otg/ulpi.c
+++ b/drivers/usb/otg/ulpi.c
@@ -37,25 +37,106 @@ static unsigned int ulpi_ids[] = {
 	ULPI_ID(0x0424, 0x0006),        /* SMSC USB3319 */
 };
 
-static int ulpi_set_flags(struct otg_transceiver *otg)
+static int ulpi_set_otg_flags(struct otg_transceiver *otg)
 {
-	unsigned int flags = 0;
+	unsigned int flags = ULPI_OTG_CTRL_DP_PULLDOWN |
+			     ULPI_OTG_CTRL_DM_PULLDOWN;
 
-	if (otg->flags & USB_OTG_PULLUP_ID)
+	if (otg->flags & ULPI_OTG_ID_PULLUP)
 		flags |= ULPI_OTG_CTRL_ID_PULLUP;
 
-	if (otg->flags & USB_OTG_PULLDOWN_DM)
-		flags |= ULPI_OTG_CTRL_DM_PULLDOWN;
+	/*
+	 * ULPI Specification rev.1.1 default
+	 * for Dp/DmPulldown is enabled.
+	 */
+	if (otg->flags & ULPI_OTG_DP_PULLDOWN_DIS)
+		flags &= ~ULPI_OTG_CTRL_DP_PULLDOWN;
 
-	if (otg->flags & USB_OTG_PULLDOWN_DP)
-		flags |= ULPI_OTG_CTRL_DP_PULLDOWN;
+	if (otg->flags & ULPI_OTG_DM_PULLDOWN_DIS)
+		flags &= ~ULPI_OTG_CTRL_DM_PULLDOWN;
 
-	if (otg->flags & USB_OTG_EXT_VBUS_INDICATOR)
+	if (otg->flags & ULPI_OTG_EXTVBUSIND)
 		flags |= ULPI_OTG_CTRL_EXTVBUSIND;
 
 	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
+static int ulpi_set_fc_flags(struct otg_transceiver *otg)
+{
+	unsigned int flags = 0;
+
+	/*
+	 * ULPI Specification rev.1.1 default
+	 * for XcvrSelect is Full Speed.
+	 */
+	if (otg->flags & ULPI_FC_HS)
+		flags |= ULPI_FUNC_CTRL_HIGH_SPEED;
+	else if (otg->flags & ULPI_FC_LS)
+		flags |= ULPI_FUNC_CTRL_LOW_SPEED;
+	else if (otg->flags & ULPI_FC_FS4LS)
+		flags |= ULPI_FUNC_CTRL_FS4LS;
+	else
+		flags |= ULPI_FUNC_CTRL_FULL_SPEED;
+
+	if (otg->flags & ULPI_FC_TERMSEL)
+		flags |= ULPI_FUNC_CTRL_TERMSELECT;
+
+	/*
+	 * ULPI Specification rev.1.1 default
+	 * for OpMode is Normal Operation.
+	 */
+	if (otg->flags & ULPI_FC_OP_NODRV)
+		flags |= ULPI_FUNC_CTRL_OPMODE_NONDRIVING;
+	else if (otg->flags & ULPI_FC_OP_DIS_NRZI)
+		flags |= ULPI_FUNC_CTRL_OPMODE_DISABLE_NRZI;
+	else if (otg->flags & ULPI_FC_OP_NSYNC_NEOP)
+		flags |= ULPI_FUNC_CTRL_OPMODE_NOSYNC_NOEOP;
+	else
+		flags |= ULPI_FUNC_CTRL_OPMODE_NORMAL;
+
+	/*
+	 * ULPI Specification rev.1.1 default
+	 * for SuspendM is Powered.
+	 */
+	flags |= ULPI_FUNC_CTRL_SUSPENDM;
+
+	return otg_io_write(otg, flags, ULPI_FUNC_CTRL);
+}
+
+static int ulpi_set_ic_flags(struct otg_transceiver *otg)
+{
+	unsigned int flags = 0;
+
+	if (otg->flags & ULPI_IC_AUTORESUME)
+		flags |= ULPI_IFC_CTRL_AUTORESUME;
+
+	if (otg->flags & ULPI_IC_EXTVBUS_INDINV)
+		flags |= ULPI_IFC_CTRL_EXTERNAL_VBUS;
+
+	if (otg->flags & ULPI_IC_IND_PASSTHRU)
+		flags |= ULPI_IFC_CTRL_PASSTHRU;
+
+	if (otg->flags & ULPI_IC_PROTECT_DIS)
+		flags |= ULPI_IFC_CTRL_PROTECT_IFC_DISABLE;
+
+	return otg_io_write(otg, flags, ULPI_IFC_CTRL);
+}
+
+static int ulpi_set_flags(struct otg_transceiver *otg)
+{
+	int ret;
+
+	ret = ulpi_set_otg_flags(otg);
+	if (ret)
+		return ret;
+
+	ret = ulpi_set_ic_flags(otg);
+	if (ret)
+		return ret;
+
+	return ulpi_set_fc_flags(otg);
+}
+
 static int ulpi_init(struct otg_transceiver *otg)
 {
 	int i, vid, pid, ret;
@@ -80,6 +161,31 @@ static int ulpi_init(struct otg_transceiver *otg)
 	return -ENODEV;
 }
 
+static int ulpi_set_host(struct otg_transceiver *otg, struct usb_bus *host)
+{
+	unsigned int flags = otg_io_read(otg, ULPI_IFC_CTRL);
+
+	if (!host) {
+		otg->host = NULL;
+		return 0;
+	}
+
+	otg->host = host;
+
+	flags &= ~(ULPI_IFC_CTRL_6_PIN_SERIAL_MODE |
+		   ULPI_IFC_CTRL_3_PIN_SERIAL_MODE |
+		   ULPI_IFC_CTRL_CARKITMODE);
+
+	if (otg->flags & ULPI_IC_6PIN_SERIAL)
+		flags |= ULPI_IFC_CTRL_6_PIN_SERIAL_MODE;
+	else if (otg->flags & ULPI_IC_3PIN_SERIAL)
+		flags |= ULPI_IFC_CTRL_3_PIN_SERIAL_MODE;
+	else if (otg->flags & ULPI_IC_CARKIT)
+		flags |= ULPI_IFC_CTRL_CARKITMODE;
+
+	return otg_io_write(otg, flags, ULPI_IFC_CTRL);
+}
+
 static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
 {
 	unsigned int flags = otg_io_read(otg, ULPI_OTG_CTRL);
@@ -87,10 +193,10 @@ static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
 	flags &= ~(ULPI_OTG_CTRL_DRVVBUS | ULPI_OTG_CTRL_DRVVBUS_EXT);
 
 	if (on) {
-		if (otg->flags & USB_OTG_DRV_VBUS)
+		if (otg->flags & ULPI_OTG_DRVVBUS)
 			flags |= ULPI_OTG_CTRL_DRVVBUS;
 
-		if (otg->flags & USB_OTG_DRV_VBUS_EXT)
+		if (otg->flags & ULPI_OTG_DRVVBUS_EXT)
 			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
 	}
 
@@ -111,6 +217,7 @@ otg_ulpi_create(struct otg_io_access_ops *ops,
 	otg->flags	= flags;
 	otg->io_ops	= ops;
 	otg->init	= ulpi_init;
+	otg->set_host	= ulpi_set_host;
 	otg->set_vbus	= ulpi_set_vbus;
 
 	return otg;
diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
index f8302d0..3dfc3b7 100644
--- a/include/linux/usb/otg.h
+++ b/include/linux/usb/otg.h
@@ -43,13 +43,6 @@ enum usb_xceiv_events {
 	USB_EVENT_ENUMERATED,   /* gadget driver enumerated */
 };
 
-#define USB_OTG_PULLUP_ID		(1 << 0)
-#define USB_OTG_PULLDOWN_DP		(1 << 1)
-#define USB_OTG_PULLDOWN_DM		(1 << 2)
-#define USB_OTG_EXT_VBUS_INDICATOR	(1 << 3)
-#define USB_OTG_DRV_VBUS		(1 << 4)
-#define USB_OTG_DRV_VBUS_EXT		(1 << 5)
-
 struct otg_transceiver;
 
 /* for transceivers connected thru an ULPI interface, the user must
diff --git a/include/linux/usb/ulpi.h b/include/linux/usb/ulpi.h
index 900d97b..82b1507 100644
--- a/include/linux/usb/ulpi.h
+++ b/include/linux/usb/ulpi.h
@@ -15,6 +15,41 @@
 /*-------------------------------------------------------------------------*/
 
 /*
+ * ULPI Flags
+ */
+#define ULPI_OTG_ID_PULLUP		(1 << 0)
+#define ULPI_OTG_DP_PULLDOWN_DIS	(1 << 1)
+#define ULPI_OTG_DM_PULLDOWN_DIS	(1 << 2)
+#define ULPI_OTG_DISCHRGVBUS		(1 << 3)
+#define ULPI_OTG_CHRGVBUS		(1 << 4)
+#define ULPI_OTG_DRVVBUS		(1 << 5)
+#define ULPI_OTG_DRVVBUS_EXT		(1 << 6)
+#define ULPI_OTG_EXTVBUSIND		(1 << 7)
+
+#define ULPI_IC_6PIN_SERIAL		(1 << 8)
+#define ULPI_IC_3PIN_SERIAL		(1 << 9)
+#define ULPI_IC_CARKIT			(1 << 10)
+#define ULPI_IC_CLKSUSPM		(1 << 11)
+#define ULPI_IC_AUTORESUME		(1 << 12)
+#define ULPI_IC_EXTVBUS_INDINV		(1 << 13)
+#define ULPI_IC_IND_PASSTHRU		(1 << 14)
+#define ULPI_IC_PROTECT_DIS		(1 << 15)
+
+#define ULPI_FC_HS			(1 << 16)
+#define ULPI_FC_FS			(1 << 17)
+#define ULPI_FC_LS			(1 << 18)
+#define ULPI_FC_FS4LS			(1 << 19)
+#define ULPI_FC_TERMSEL			(1 << 20)
+#define ULPI_FC_OP_NORM			(1 << 21)
+#define ULPI_FC_OP_NODRV		(1 << 22)
+#define ULPI_FC_OP_DIS_NRZI		(1 << 23)
+#define ULPI_FC_OP_NSYNC_NEOP		(1 << 24)
+#define ULPI_FC_RST			(1 << 25)
+#define ULPI_FC_SUSPM			(1 << 26)
+
+/*-------------------------------------------------------------------------*/
+
+/*
  * Macros for Set and Clear
  * See ULPI 1.1 specification to find the registers with Set and Clear offsets
  */
@@ -59,6 +94,10 @@
 
 /*-------------------------------------------------------------------------*/
 
+/*
+ * Register Bits
+ */
+
 /* Function Control */
 #define ULPI_FUNC_CTRL_XCVRSEL			(1 << 0)
 #define  ULPI_FUNC_CTRL_XCVRSEL_MASK		(3 << 0)
-- 
1.7.1

[PATCH 0/3] Generic ULPI driver extention

[Igor Grinberg]

Greg, Daniel, Sascha,

Any comments?

> This patch series add support for SMSC USB3319 ulpi phy,
> introduces the ulpi specific flags for control over the
> ulpi phy and extends the generic ulpi driver with
> support for Function and Interface control of upli phy.
>
> Depends on:
> From: Ajay Kumar Gupta <ajay.gupta@ti.com>
> Date: Thu,  8 Jul 2010 14:03:01 +0530
> Subject: USB: ulpi: fix compilation warning
>
> From: Eric B?nard <eric@eukrea.com>
> Date: Thu, 15 Jul 2010 09:20:19 +0200
> Subject: [PATCH v2] otg/ulpi.c : fix register write
>
> Igor Grinberg (3):
>   usb/otg/ulpi: remove unused macro
>   usb/otg/ulpi: add support for SMSC USB3319 ulpi phy
>   usb/otg/ulpi: extend the generic ulpi driver.
>
>  arch/arm/mach-mx2/mach-pca100.c          |    4 +-
>  arch/arm/mach-mx3/mach-armadillo5x0.c    |    4 +-
>  arch/arm/mach-mx3/mach-mx31lilly.c       |    4 +-
>  arch/arm/mach-mx3/mach-mx31lite.c        |    2 +-
>  arch/arm/mach-mx3/mach-mx31moboard.c     |    2 +-
>  arch/arm/mach-mx3/mach-pcm037.c          |    4 +-
>  arch/arm/mach-mx3/mach-pcm043.c          |    2 +-
>  arch/arm/mach-mx3/mx31moboard-smartbot.c |    2 +-
>  drivers/usb/otg/Kconfig                  |    2 -
>  drivers/usb/otg/ulpi.c                   |  130 +++++++++++++++++++++++++++---
>  include/linux/usb/otg.h                  |    7 --
>  include/linux/usb/ulpi.h                 |   39 +++++++++
>  12 files changed, 169 insertions(+), 33 deletions(-)
>
>
>   

-- 
Regards,
Igor.

[PATCH 3/3] usb/otg/ulpi: extend the generic ulpi driver.

[Heikki Krogerus]

Hi,

This patch has already been applied and I have totally missed it,
sorry about that, but I have to ask..

On Thu, Jul 15, 2010 at 04:00:16PM +0300, Igor Grinberg wrote:
> 1) Introduce ulpi specific flags for control of the ulpi phy
> 2) Extend the generic ulpi driver with support for Function and
> Interface control of upli phy
> 3) Update the platforms using the generic ulpi driver with new ulpi
> flags
> 4) Remove the otg control flags not in use
> 
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> Signed-off-by: Mike Rapoport <mike@compulab.co.il>

<snip>

> diff --git a/include/linux/usb/ulpi.h b/include/linux/usb/ulpi.h
> index 900d97b..82b1507 100644
> --- a/include/linux/usb/ulpi.h
> +++ b/include/linux/usb/ulpi.h
> @@ -15,6 +15,41 @@
>  /*-------------------------------------------------------------------------*/
>  
>  /*
> + * ULPI Flags
> + */
> +#define ULPI_OTG_ID_PULLUP		(1 << 0)
> +#define ULPI_OTG_DP_PULLDOWN_DIS	(1 << 1)
> +#define ULPI_OTG_DM_PULLDOWN_DIS	(1 << 2)
> +#define ULPI_OTG_DISCHRGVBUS		(1 << 3)
> +#define ULPI_OTG_CHRGVBUS		(1 << 4)
> +#define ULPI_OTG_DRVVBUS		(1 << 5)
> +#define ULPI_OTG_DRVVBUS_EXT		(1 << 6)
> +#define ULPI_OTG_EXTVBUSIND		(1 << 7)

Why are you redefining these? If you look through the file you'll find
the same bits are already there for OTG Control?

> +#define ULPI_IC_6PIN_SERIAL		(1 << 8)
> +#define ULPI_IC_3PIN_SERIAL		(1 << 9)
> +#define ULPI_IC_CARKIT			(1 << 10)
> +#define ULPI_IC_CLKSUSPM		(1 << 11)
> +#define ULPI_IC_AUTORESUME		(1 << 12)
> +#define ULPI_IC_EXTVBUS_INDINV		(1 << 13)
> +#define ULPI_IC_IND_PASSTHRU		(1 << 14)
> +#define ULPI_IC_PROTECT_DIS		(1 << 15)

Why bit offsets starting from (1 << 8)? I took a look@ulpi.c and
you are still using ULPI_IFC_CTRL register, so these offsets can't
possibly work. There are also existing definitions for ULPI Interface
Control bits in any case.

> +#define ULPI_FC_HS			(1 << 16)
> +#define ULPI_FC_FS			(1 << 17)
> +#define ULPI_FC_LS			(1 << 18)
> +#define ULPI_FC_FS4LS			(1 << 19)
> +#define ULPI_FC_TERMSEL			(1 << 20)
> +#define ULPI_FC_OP_NORM			(1 << 21)
> +#define ULPI_FC_OP_NODRV		(1 << 22)
> +#define ULPI_FC_OP_DIS_NRZI		(1 << 23)
> +#define ULPI_FC_OP_NSYNC_NEOP		(1 << 24)
> +#define ULPI_FC_RST			(1 << 25)
> +#define ULPI_FC_SUSPM			(1 << 26)

Same here?

I'm asking in case I'm missing something and there is a reason the
above, before sending any patches.

br,

-- 
heikki

[PATCH 3/3] usb/otg/ulpi: extend the generic ulpi driver.

[Igor Grinberg]

On 08/13/10 16:27, Heikki Krogerus wrote:
> Hi,
>
> This patch has already been applied and I have totally missed it,
> sorry about that, but I have to ask..
>   

Hi,

It was on the list more then a week, I think, and then in linux-next.
I really wanted some eyes on it involved, so may be we could design
something better then what I've done, but it is never too late ;)

> On Thu, Jul 15, 2010 at 04:00:16PM +0300, Igor Grinberg wrote:
>   
>> 1) Introduce ulpi specific flags for control of the ulpi phy
>> 2) Extend the generic ulpi driver with support for Function and
>> Interface control of upli phy
>> 3) Update the platforms using the generic ulpi driver with new ulpi
>> flags
>> 4) Remove the otg control flags not in use
>>
>> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
>> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>>     
> <snip>
>
>   
>> diff --git a/include/linux/usb/ulpi.h b/include/linux/usb/ulpi.h
>> index 900d97b..82b1507 100644
>> --- a/include/linux/usb/ulpi.h
>> +++ b/include/linux/usb/ulpi.h
>> @@ -15,6 +15,41 @@
>>  /*-------------------------------------------------------------------------*/
>>  
>>  /*
>> + * ULPI Flags
>> + */
>> +#define ULPI_OTG_ID_PULLUP		(1 << 0)
>> +#define ULPI_OTG_DP_PULLDOWN_DIS	(1 << 1)
>> +#define ULPI_OTG_DM_PULLDOWN_DIS	(1 << 2)
>> +#define ULPI_OTG_DISCHRGVBUS		(1 << 3)
>> +#define ULPI_OTG_CHRGVBUS		(1 << 4)
>> +#define ULPI_OTG_DRVVBUS		(1 << 5)
>> +#define ULPI_OTG_DRVVBUS_EXT		(1 << 6)
>> +#define ULPI_OTG_EXTVBUSIND		(1 << 7)
>>     
> Why are you redefining these? If you look through the file you'll find
> the same bits are already there for OTG Control?
>   

Well, trust me I went through the file and more then one time.
If you overlook the changes in the ulpi.h, you can see that I've added
the comment before register bits, so it will be clearer.

The idea of the flags is to provide a control of the ulpi phy by the ulpi
driver only, so whoever wants to use it (any platform, not just imx),
will only specify the flags different from the default of the ulpi spec.
and the ulpi driver will take care of the rest (perfect case).

This makes any platform independent from the ulpi registers,
thus separating the interface from the implementation.
Makes possible for some changes (if ever) in ulpi spec. be
transparent to the ulpi driver users.
Lowers the legitimation for using the ulpi registers directly in
the platform code (like it is currently done in imx ehci glue).
Lets ulpi_create() method stay only with one parameter (flags)
instead of adding two more parameters for IC and FC.

Moreover, the best thing to do is to place the register bits only
inside ulpi.c and provide functional methods (like the
ulpi_set_host/peripheral(), etc..) and the flags above to control
their behavior.
I wanted to do this in first place, but currently, the ulpi driver
we have is not ready to deal with all the control of the ulpi phy
(it will never be, if nobody will contribute to it), I cannot afford
to myself this amount of work right now and I don't have
the imx hardware to test the changes in imx-ehci glue.

>   
>> +#define ULPI_IC_6PIN_SERIAL		(1 << 8)
>> +#define ULPI_IC_3PIN_SERIAL		(1 << 9)
>> +#define ULPI_IC_CARKIT			(1 << 10)
>> +#define ULPI_IC_CLKSUSPM		(1 << 11)
>> +#define ULPI_IC_AUTORESUME		(1 << 12)
>> +#define ULPI_IC_EXTVBUS_INDINV		(1 << 13)
>> +#define ULPI_IC_IND_PASSTHRU		(1 << 14)
>> +#define ULPI_IC_PROTECT_DIS		(1 << 15)
>>     
> Why bit offsets starting from (1 << 8)?

This is because they are not bits, but flags, so they can start
wherever you want. This provides the separation between the
interface and the implementation (I've already explained it above).

>  I took a look at ulpi.c and
> you are still using ULPI_IFC_CTRL register, so these offsets can't
> possibly work.

Of course I use the registers, it is the ulpi spec. is what defines
them, but the right thing is to move the registers and bits into
the ulpi driver (as I've explained above).

>  There are also existing definitions for ULPI Interface
> Control bits in any case.
>   

I wanted to make the flags somewhat consistent with the
registers bits and also fit one u32 parameter variable of
ulpi_create().

>   
>> +#define ULPI_FC_HS			(1 << 16)
>> +#define ULPI_FC_FS			(1 << 17)
>> +#define ULPI_FC_LS			(1 << 18)
>> +#define ULPI_FC_FS4LS			(1 << 19)
>> +#define ULPI_FC_TERMSEL			(1 << 20)
>> +#define ULPI_FC_OP_NORM			(1 << 21)
>> +#define ULPI_FC_OP_NODRV		(1 << 22)
>> +#define ULPI_FC_OP_DIS_NRZI		(1 << 23)
>> +#define ULPI_FC_OP_NSYNC_NEOP		(1 << 24)
>> +#define ULPI_FC_RST			(1 << 25)
>> +#define ULPI_FC_SUSPM			(1 << 26)
>>     
> Same here?
>
> I'm asking in case I'm missing something and there is a reason the
> above, before sending any patches.
>   

I hope the reason is clear now :)
Also, I'm trying to get more attention of the imx guys.
There is a ulpi driver, please use/develop it where
possible/appropriate instead of locking the code in
platforms (e.g. imx-ehci glue).
This way more platforms can benefit from your contribution.

> br,
>
>   

-- 
Regards,
Igor.

[PATCH 3/3] usb/otg/ulpi: extend the generic ulpi driver.

[Heikki Krogerus]

Hi,

On Sun, Aug 15, 2010 at 09:29:54AM +0200, ext Igor Grinberg wrote:
>On 08/13/10 16:27, Heikki Krogerus wrote:
>> This patch has already been applied and I have totally missed it,
>> sorry about that, but I have to ask..
>
> It was on the list more then a week, I think, and then in linux-next.
> I really wanted some eyes on it involved, so may be we could design
> something better then what I've done, but it is never too late ;)

I was on vacation, and only now saw your patches. Sorry about the late
comments.
 
>> On Thu, Jul 15, 2010 at 04:00:16PM +0300, Igor Grinberg wrote:
>>>  /*
>>> + * ULPI Flags
>>> + */
>>> +#define ULPI_OTG_ID_PULLUP		(1 << 0)
>>> +#define ULPI_OTG_DP_PULLDOWN_DIS	(1 << 1)
>>> +#define ULPI_OTG_DM_PULLDOWN_DIS	(1 << 2)
>>> +#define ULPI_OTG_DISCHRGVBUS		(1 << 3)
>>> +#define ULPI_OTG_CHRGVBUS		(1 << 4)
>>> +#define ULPI_OTG_DRVVBUS		(1 << 5)
>>> +#define ULPI_OTG_DRVVBUS_EXT		(1 << 6)
>>> +#define ULPI_OTG_EXTVBUSIND		(1 << 7)
>>>     
>> Why are you redefining these? If you look through the file you'll find
>> the same bits are already there for OTG Control?
>>   
>
> Well, trust me I went through the file and more then one time.
> If you overlook the changes in the ulpi.h, you can see that I've added
> the comment before register bits, so it will be clearer.
> 
> The idea of the flags is to provide a control of the ulpi phy by the ulpi
> driver only, so whoever wants to use it (any platform, not just imx),
> will only specify the flags different from the default of the ulpi spec.
> and the ulpi driver will take care of the rest (perfect case).
> 
> This makes any platform independent from the ulpi registers,
> thus separating the interface from the implementation.
> Makes possible for some changes (if ever) in ulpi spec. be
> transparent to the ulpi driver users.
> Lowers the legitimation for using the ulpi registers directly in
> the platform code (like it is currently done in imx ehci glue).
> Lets ulpi_create() method stay only with one parameter (flags)
> instead of adding two more parameters for IC and FC.
> 
> Moreover, the best thing to do is to place the register bits only
> inside ulpi.c and provide functional methods (like the
> ulpi_set_host/peripheral(), etc..) and the flags above to control
> their behavior.
> I wanted to do this in first place, but currently, the ulpi driver
> we have is not ready to deal with all the control of the ulpi phy
> (it will never be, if nobody will contribute to it), I cannot afford
> to myself this amount of work right now and I don't have
> the imx hardware to test the changes in imx-ehci glue.

OK, I get your idea now with the flags now. Should have read the code
more carefully. I'm not sure if it's the right way though, but if you
can show that they are beneficial in more then one case, then I'm sure
there is no problem with them. Please consider the following.

It seems that many ulpi transceivers are autonomous as they say.
There is little if any need to access ulpi registers expect the
vendor specific ones. The vendor specific stuff will require register
access in any case and in some cases they need the transceivers to be
programmed in a specific way. Right now to me using the flags for the
common parts and then separately programming the vendor specific stuff
does not seem very efficient. 

I need to re-send the charger driver for isp170x. These transceivers
are fairly commonly used with musb, and most platform probable only
need nop-usb-xceiv.c with them. However, the charger detection needs
access to the common ulpi registers as well as the vendor specific
ones. If you see that you can make this kind of functions work with
the flags, then I'm OK with them. 

The twl4030-usb has a few places where it needs to program some common
ulpi registers. Those would need to be removed, or the file would need
separate definitions for the ulpi registers if you want to only define
the register only in ulpi.c. Maybe you could give it a quick look
and see how would it be handled with the flags.

br,
-- 
heikki

[PATCH 3/3] usb/otg/ulpi: extend the generic ulpi driver.

[Igor Grinberg]

On 08/16/10 12:37, Heikki Krogerus wrote:
> Hi,
>
> On Sun, Aug 15, 2010 at 09:29:54AM +0200, ext Igor Grinberg wrote:
>> On 08/13/10 16:27, Heikki Krogerus wrote:
>>> This patch has already been applied and I have totally missed it,
>>> sorry about that, but I have to ask..
>> It was on the list more then a week, I think, and then in linux-next.
>> I really wanted some eyes on it involved, so may be we could design
>> something better then what I've done, but it is never too late ;)
> I was on vacation, and only now saw your patches. Sorry about the late
> comments.
>  
>>> On Thu, Jul 15, 2010 at 04:00:16PM +0300, Igor Grinberg wrote:
>>>>  /*
>>>> + * ULPI Flags
>>>> + */
>>>> +#define ULPI_OTG_ID_PULLUP		(1 << 0)
>>>> +#define ULPI_OTG_DP_PULLDOWN_DIS	(1 << 1)
>>>> +#define ULPI_OTG_DM_PULLDOWN_DIS	(1 << 2)
>>>> +#define ULPI_OTG_DISCHRGVBUS		(1 << 3)
>>>> +#define ULPI_OTG_CHRGVBUS		(1 << 4)
>>>> +#define ULPI_OTG_DRVVBUS		(1 << 5)
>>>> +#define ULPI_OTG_DRVVBUS_EXT		(1 << 6)
>>>> +#define ULPI_OTG_EXTVBUSIND		(1 << 7)
>>>>     
>>> Why are you redefining these? If you look through the file you'll find
>>> the same bits are already there for OTG Control?
>>>   
>> Well, trust me I went through the file and more then one time.
>> If you overlook the changes in the ulpi.h, you can see that I've added
>> the comment before register bits, so it will be clearer.
>>
>> The idea of the flags is to provide a control of the ulpi phy by the ulpi
>> driver only, so whoever wants to use it (any platform, not just imx),
>> will only specify the flags different from the default of the ulpi spec.
>> and the ulpi driver will take care of the rest (perfect case).
>>
>> This makes any platform independent from the ulpi registers,
>> thus separating the interface from the implementation.
>> Makes possible for some changes (if ever) in ulpi spec. be
>> transparent to the ulpi driver users.
>> Lowers the legitimation for using the ulpi registers directly in
>> the platform code (like it is currently done in imx ehci glue).
>> Lets ulpi_create() method stay only with one parameter (flags)
>> instead of adding two more parameters for IC and FC.
>>
>> Moreover, the best thing to do is to place the register bits only
>> inside ulpi.c and provide functional methods (like the
>> ulpi_set_host/peripheral(), etc..) and the flags above to control
>> their behavior.
>> I wanted to do this in first place, but currently, the ulpi driver
>> we have is not ready to deal with all the control of the ulpi phy
>> (it will never be, if nobody will contribute to it), I cannot afford
>> to myself this amount of work right now and I don't have
>> the imx hardware to test the changes in imx-ehci glue.
> OK, I get your idea now with the flags now. Should have read the code
> more carefully. I'm not sure if it's the right way though, but if you
> can show that they are beneficial in more then one case, then I'm sure
> there is no problem with them.

Indeed, this can be not the best way, but at least it is better then letting
everyone around access the ulpi defined registers in a way they like.
There is a ulpi standard and kernel should provide a framework/driver
that follows the standard (at least for the ulpi defined registers).

>  Please consider the following.
>
> It seems that many ulpi transceivers are autonomous as they say.
> There is little if any need to access ulpi registers expect the
> vendor specific ones. The vendor specific stuff will require register
> access in any case and in some cases they need the transceivers to be
> programmed in a specific way. Right now to me using the flags for the
> common parts and then separately programming the vendor specific stuff
> does not seem very efficient. 

Well, vendor specific registers are very important issue.
This is how I see it:

platform core   <-------wired-------->   ulpi    phy
      /                                 /           \
     /                     ulpi generic regs  |  vendor specific regs
    /                                 /               \
ulpi access ops <-----> ulpi generic driver <---> ulpi phy specific driver(s)
                          /           \
                  otg framework        \
                          \            /
                 usb subsystems e.g. musb, ehci...


If the ulpi transceiver is autonomous and platform does not need to
do any setup of the transceiver, then I think you don't need to
access the transceiver in any way, just setup the platform core,
which can be done in platform specific code.

If there is a need to access the ulpi transceiver, then it should be done
by using the ulpi driver. ulpi transceiver usually is a chip, so if it needs
to be setup, then it deserves a driver, I think.

Anyway, we need a good generic ulpi driver, which will provide some
kind of interface for the ulpi specific drivers that are aware of the
vendor specific registers and setup. This interface should make the
task of developing ulpi specific driver easy.

The driver does not have to end up with flags. The intension of those
flags is to give a start for something bigger... otherwise people will
continue to setup the hardware specific stuff in the generic drivers
(like the recent ulpi phy reset code in omap-ehci glue) instead of
contributing to the ulpi driver, so other platforms could reuse it.

> I need to re-send the charger driver for isp170x. These transceivers
> are fairly commonly used with musb, and most platform probable only
> need nop-usb-xceiv.c with them. However, the charger detection needs
> access to the common ulpi registers as well as the vendor specific
> ones. If you see that you can make this kind of functions work with
> the flags, then I'm OK with them. 

Is this series (part of it) you need to resend:
http://www.spinics.net/lists/linux-usb/msg29643.html

isp1704_test_ulpi() - can be done fully by the generic ulpi driver,
in fact it is already reads the id. May be we should think of a way to export
it, so drivers like this one can do the matching.

isp1704_charger_detect() - isp170x specific ulpi registers accesses.

isp1704_charger_verify() - I don't fully understand the purpose of this,
I see the reset (can be done by the generic driver) and then some
ulpi generic and specific accesses to the transceiver.
I think it can be handled by the ulpi generic and specific driver
and with help of the flags.

> The twl4030-usb has a few places where it needs to program some common
> ulpi registers. Those would need to be removed, or the file would need
> separate definitions for the ulpi registers if you want to only define
> the register only in ulpi.c. Maybe you could give it a quick look
> and see how would it be handled with the flags.

This one is a bit tricky... :)
Are those calls in twl4030_usb_set_mode() have to be in that specific order?
Otherwise, I think it can use the generic and specific driver (after someone
prepares it).

> br,

-- 
Regards,
Igor.

[PATCH 3/3] usb/otg/ulpi: extend the generic ulpi driver.

[Heikki Krogerus]

On Tue, Aug 17, 2010 at 06:17:09PM +0200, ext Igor Grinberg wrote:
>On 08/16/10 12:37, Heikki Krogerus wrote:
>> OK, I get your idea now with the flags now. Should have read the code
>> more carefully. I'm not sure if it's the right way though, but if you
>> can show that they are beneficial in more then one case, then I'm sure
>> there is no problem with them.
> 
> Indeed, this can be not the best way, but at least it is better then letting
> everyone around access the ulpi defined registers in a way they like.
> There is a ulpi standard and kernel should provide a framework/driver
> that follows the standard (at least for the ulpi defined registers).
> 
>>  Please consider the following.
>>
>> It seems that many ulpi transceivers are autonomous as they say.
>> There is little if any need to access ulpi registers expect the
>> vendor specific ones. The vendor specific stuff will require register
>> access in any case and in some cases they need the transceivers to be
>> programmed in a specific way. Right now to me using the flags for the
>> common parts and then separately programming the vendor specific stuff
>> does not seem very efficient. 
> 
> Well, vendor specific registers are very important issue.
> This is how I see it:
> 
> platform core   <-------wired-------->   ulpi    phy
>       /                                 /           \
>      /                     ulpi generic regs  |  vendor specific regs
>     /                                 /               \
> ulpi access ops <-----> ulpi generic driver <---> ulpi phy specific driver(s)
>                           /           \
>                   otg framework        \
>                           \            /
>                  usb subsystems e.g. musb, ehci...
> 
> 
> If the ulpi transceiver is autonomous and platform does not need to
> do any setup of the transceiver, then I think you don't need to
> access the transceiver in any way, just setup the platform core,
> which can be done in platform specific code.
> 
> If there is a need to access the ulpi transceiver, then it should be done
> by using the ulpi driver. ulpi transceiver usually is a chip, so if it needs
> to be setup, then it deserves a driver, I think.
> 
> Anyway, we need a good generic ulpi driver, which will provide some
> kind of interface for the ulpi specific drivers that are aware of the
> vendor specific registers and setup. This interface should make the
> task of developing ulpi specific driver easy.

I'm totally _not_ against a generic ulpi driver, and see it can be
something very useful.

> The driver does not have to end up with flags. The intension of those
> flags is to give a start for something bigger... otherwise people will
> continue to setup the hardware specific stuff in the generic drivers
> (like the recent ulpi phy reset code in omap-ehci glue) instead of
> contributing to the ulpi driver, so other platforms could reuse it.

Unfortunately this is not generating enough interest in people. I
guess this is one of those cases where people prefer to wait for
someone else to make the ulpi driver more mature instead of
contributing themselves, and in the meantime, hack their existing or
new driver (like me) :(. Someone simply needs to put effort in
developing it, but as I understood, you are too busy at the moment,
and so am I.

>> I need to re-send the charger driver for isp170x. These transceivers
>> are fairly commonly used with musb, and most platform probable only
>> need nop-usb-xceiv.c with them. However, the charger detection needs
>> access to the common ulpi registers as well as the vendor specific
>> ones. If you see that you can make this kind of functions work with
>> the flags, then I'm OK with them. 
> 
> Is this series (part of it) you need to resend:
> http://www.spinics.net/lists/linux-usb/msg29643.html
> 
> isp1704_test_ulpi() - can be done fully by the generic ulpi driver,
> in fact it is already reads the id. May be we should think of a way to export
> it, so drivers like this one can do the matching.

Yes it would make sense for now.

> isp1704_charger_detect() - isp170x specific ulpi registers accesses.
> 
> isp1704_charger_verify() - I don't fully understand the purpose of this,
> I see the reset (can be done by the generic driver) and then some
> ulpi generic and specific accesses to the transceiver.
> I think it can be handled by the ulpi generic and specific driver
> and with help of the flags.

This transceiver is a bit difficult as RX51 platform uses it as it's
second transceiver. So the primary transceiver provides the struct
otg_transceiver, and from software's point of view only charger
detection is done with isp170x, so I was unable to take advantage of
the ulpi driver.

This kind of oddities make things even more complicated. The otg
framework is not suited for multiple transceivers (maybe it newer
will), but the platform needs to be considered in any case :(.

Unfortunately I have to send the driver as it is, and can't spend any
time with it right now. I need to put more though into this at one
point.

br,
-- 
heikki

[PATCH 3/3] usb/otg/ulpi: extend the generic ulpi driver.

[Igor Grinberg]

 On 08/18/10 12:49, Heikki Krogerus wrote:
> On Tue, Aug 17, 2010 at 06:17:09PM +0200, ext Igor Grinberg wrote:
>> On 08/16/10 12:37, Heikki Krogerus wrote:
>>> OK, I get your idea now with the flags now. Should have read the code
>>> more carefully. I'm not sure if it's the right way though, but if you
>>> can show that they are beneficial in more then one case, then I'm sure
>>> there is no problem with them.
>> Indeed, this can be not the best way, but at least it is better then letting
>> everyone around access the ulpi defined registers in a way they like.
>> There is a ulpi standard and kernel should provide a framework/driver
>> that follows the standard (at least for the ulpi defined registers).
>>
>>>  Please consider the following.
>>>
>>> It seems that many ulpi transceivers are autonomous as they say.
>>> There is little if any need to access ulpi registers expect the
>>> vendor specific ones. The vendor specific stuff will require register
>>> access in any case and in some cases they need the transceivers to be
>>> programmed in a specific way. Right now to me using the flags for the
>>> common parts and then separately programming the vendor specific stuff
>>> does not seem very efficient. 
>> Well, vendor specific registers are very important issue.
>> This is how I see it:
>>
>> platform core   <-------wired-------->   ulpi    phy
>>       /                                 /           \
>>      /                     ulpi generic regs  |  vendor specific regs
>>     /                                 /               \
>> ulpi access ops <-----> ulpi generic driver <---> ulpi phy specific driver(s)
>>                           /           \
>>                   otg framework        \
>>                           \            /
>>                  usb subsystems e.g. musb, ehci...
>>
>>
>> If the ulpi transceiver is autonomous and platform does not need to
>> do any setup of the transceiver, then I think you don't need to
>> access the transceiver in any way, just setup the platform core,
>> which can be done in platform specific code.
>>
>> If there is a need to access the ulpi transceiver, then it should be done
>> by using the ulpi driver. ulpi transceiver usually is a chip, so if it needs
>> to be setup, then it deserves a driver, I think.
>>
>> Anyway, we need a good generic ulpi driver, which will provide some
>> kind of interface for the ulpi specific drivers that are aware of the
>> vendor specific registers and setup. This interface should make the
>> task of developing ulpi specific driver easy.
> I'm totally _not_ against a generic ulpi driver, and see it can be
> something very useful.
>
>> The driver does not have to end up with flags. The intension of those
>> flags is to give a start for something bigger... otherwise people will
>> continue to setup the hardware specific stuff in the generic drivers
>> (like the recent ulpi phy reset code in omap-ehci glue) instead of
>> contributing to the ulpi driver, so other platforms could reuse it.
> Unfortunately this is not generating enough interest in people. I
> guess this is one of those cases where people prefer to wait for
> someone else to make the ulpi driver more mature instead of
> contributing themselves, and in the meantime, hack their existing or
> new driver (like me) :(. Someone simply needs to put effort in
> developing it, but as I understood, you are too busy at the moment,
> and so am I.

Well, it's a pity, but that is the reality...
I think even small contributions could make a difference.
Lets say, a reset or the pm, are not so time consuming and are very
reusable.

>>> I need to re-send the charger driver for isp170x. These transceivers
>>> are fairly commonly used with musb, and most platform probable only
>>> need nop-usb-xceiv.c with them. However, the charger detection needs
>>> access to the common ulpi registers as well as the vendor specific
>>> ones. If you see that you can make this kind of functions work with
>>> the flags, then I'm OK with them. 
>> Is this series (part of it) you need to resend:
>> http://www.spinics.net/lists/linux-usb/msg29643.html
>>
>> isp1704_test_ulpi() - can be done fully by the generic ulpi driver,
>> in fact it is already reads the id. May be we should think of a way to export
>> it, so drivers like this one can do the matching.
> Yes it would make sense for now.
>
>> isp1704_charger_detect() - isp170x specific ulpi registers accesses.
>>
>> isp1704_charger_verify() - I don't fully understand the purpose of this,
>> I see the reset (can be done by the generic driver) and then some
>> ulpi generic and specific accesses to the transceiver.
>> I think it can be handled by the ulpi generic and specific driver
>> and with help of the flags.
> This transceiver is a bit difficult as RX51 platform uses it as it's
> second transceiver. So the primary transceiver provides the struct
> otg_transceiver, and from software's point of view only charger
> detection is done with isp170x, so I was unable to take advantage of
> the ulpi driver.
>
> This kind of oddities make things even more complicated. The otg
> framework is not suited for multiple transceivers (maybe it newer
> will), but the platform needs to be considered in any case :(.

As far as I know, the OTG standard does not permit more then one
OTG port (and thus transceiver) on the system, but we can have
multiple ulpi transceivers, which are not necessarily used for OTG.

> Unfortunately I have to send the driver as it is, and can't spend any
> time with it right now. I need to put more though into this at one
> point.

Yeah, I see that. Hope that point in time will come...

> br,

-- 
Regards,
Igor.