usb:ci_udc: Remove ULPI setting for i.MX OTG controller

PATCH New Peng Fan 3 participants 5 messages Dec 31, 2015 – Jan 5, 2016 USB

lore

Share & tools

lore

Peng Fan

author

2015-12-31 07:24 UTC #0 lore raw

PATCH v3 1/1 usb:ci_udc: Remove ULPI setting for i.MX OTG controller

From: "Ye.Li" <[email protected]>

All the i.MX6, i.MX23 and i.MX28 OTG controllers only support UTMI interface. Set to ULPI is not correct, even the controller will reject this setting in PORTSC register.

Signed-off-by Ye.Li <[email protected]>

Signed-off-by Peng Fan <[email protected]>

---

Hi Marek,
In V2: https://patchwork.ozlabs.org/patch/541663/
The reason still use DBG, but not change to debug, is that the driver use DBG for debugging and error handling usage.
So In this patch, I still keep DBG. If you think, it better to switch to use function debug, I can write another patch to convert all the DBGs to debug for ci_udc driver.

Regards,
Peng.

Changes since v2:
- Correct code indent.
Changes since v1:
- Add error handling for udc probe

 drivers/usb/gadget/ci_udc.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

drivers/usb/gadget/ci_udc.c +3 ~~-11~~

index 1ba5054..d36bcf6 100644
@@ -1018,18 +1018,10 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
 		return ret;
 
 	ret = ci_udc_probe();
-#if defined(CONFIG_USB_EHCI_MX6) || defined(CONFIG_USB_EHCI_MXS)
-	/*
-	 * FIXME: usb_lowlevel_init()->ehci_hcd_init() should be doing all
-	 * HW-specific initialization, e.g. ULPI-vs-UTMI PHY selection
-	 */
-	if (!ret) {
-		struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
-
-		/* select ULPI phy */
-		writel(PTS(PTS_ENABLE) | PFSC, &udc->portsc);
+	if (ret) {
+		DBG("udc probe failed, returned %d\n", ret);
+		return ret;
 	}
-#endif
 
 	ret = driver->bind(&controller.gadget);
 	if (ret) {

Signature · 1 line

2.6.2

Marek Vasut

2015-12-31 09:07 UTC #1 lore raw

PATCH v3 1/1 usb:ci_udc: Remove ULPI setting for i.MX OTG controller

Quoted 37 lines · Peng Fan

From: "Ye.Li" <[email protected]>
All the i.MX6, i.MX23 and i.MX28 OTG controllers only support UTMI interface. Set to ULPI is not correct, even the controller will reject this setting in PORTSC register.
Signed-off-by Ye.Li <[email protected]>
Signed-off-by Peng Fan <[email protected]>
---
Hi Marek,
In V2: https://patchwork.ozlabs.org/patch/541663/
The reason still use DBG, but not change to debug, is that the driver use DBG for debugging and error handling usage.
So In this patch, I still keep DBG. If you think, it better to switch to use function debug, I can write another patch to convert all the DBGs to debug for ci_udc driver.
Regards,
Peng.
Changes since v2:
- Correct code indent.
Changes since v1:
- Add error handling for udc probe
 drivers/usb/gadget/ci_udc.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)
drivers/usb/gadget/ci_udc.c +0 -0
index 1ba5054..d36bcf6 100644
@@ -1018,18 +1018,10 @@ int usb_gadget_register_driver(struct
 usb_gadget_driver *driver) return ret;
 
 	ret = ci_udc_probe();

drivers/usb/gadget/ci_udc.c int usb_gadget_register_driver(struct · line 1030

Quoted 10 lines

drivers/usb/gadget/ci_udc.c +0 ~~-10~~

-#if defined(CONFIG_USB_EHCI_MX6) || defined(CONFIG_USB_EHCI_MXS)
-	/*
-	 * FIXME: usb_lowlevel_init()->ehci_hcd_init() should be doing all
-	 * HW-specific initialization, e.g. ULPI-vs-UTMI PHY selection
-	 */
-	if (!ret) {
-		struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
-
-		/* select ULPI phy */
-		writel(PTS(PTS_ENABLE) | PFSC, &udc->portsc);

So if ci_udc_probe() doesn't fail, this code will select PTS_ENABLE and PFSC in the portsc register, right ? If you remove this code, who will do this configuration ? I suspect this breaks all i.MXes, no ?

drivers/usb/gadget/ci_udc.c int usb_gadget_register_driver(struct usb_gadget_driver *driver) · line 1027

Quoted 8 lines

drivers/usb/gadget/ci_udc.c +3 -1

+	if (ret) {
+		DBG("udc probe failed, returned %d\n", ret);
+		return ret;
 	}
-#endif
 
 	ret = driver->bind(&controller.gadget);
 	if (ret) {

Best regards,
Marek Vasut

Li Ye-B37916

2015-12-31 09:34 UTC #2 lore raw

PATCH v3 1/1 usb:ci_udc: Remove ULPI setting for i.MX OTG controller

Quoted 37 lines · Marek Vasut

Quoted 36 lines · Peng Fan
From: "Ye.Li" <[email protected]>
All the i.MX6, i.MX23 and i.MX28 OTG controllers only support UTMI interface. Set to ULPI is not correct, even the controller will reject this setting in PORTSC register.
Signed-off-by Ye.Li <[email protected]>
Signed-off-by Peng Fan <[email protected]>
---
Hi Marek,
In V2: https://patchwork.ozlabs.org/patch/541663/
The reason still use DBG, but not change to debug, is that the driver use DBG for debugging and error handling usage.
So In this patch, I still keep DBG. If you think, it better to switch to use function debug, I can write another patch to convert all the DBGs to debug for ci_udc driver.
Regards,
Peng.
Changes since v2:
- Correct code indent.
Changes since v1:
- Add error handling for udc probe
 drivers/usb/gadget/ci_udc.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)
drivers/usb/gadget/ci_udc.c +0 -0
index 1ba5054..d36bcf6 100644
@@ -1018,18 +1018,10 @@ int usb_gadget_register_driver(struct
 usb_gadget_driver *driver) return ret;
 

Quoted 14 lines

drivers/usb/gadget/ci_udc.c int usb_gadget_register_driver(struct usb_gadget_driver *driver) · line 1030
Quoted 11 lines
drivers/usb/gadget/ci_udc.c +0 ~~-10~~
 	ret = ci_udc_probe();
-#if defined(CONFIG_USB_EHCI_MX6) || defined(CONFIG_USB_EHCI_MXS)
-	/*
-	 * FIXME: usb_lowlevel_init()->ehci_hcd_init() should be doing all
-	 * HW-specific initialization, e.g. ULPI-vs-UTMI PHY selection
-	 */
-	if (!ret) {
-		struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
-
-		/* select ULPI phy */
-		writel(PTS(PTS_ENABLE) | PFSC, &udc->portsc);
So if ci_udc_probe() doesn't fail, this code will select PTS_ENABLE and PFSC in the portsc register, right ? If you remove this code, who will do this configuration ? I suspect this breaks all i.MXes, no ?

Selecting PTS_ENABLE and PFSC are NOT correct for i.MXes. This code wants to select ULPI PHY, but only UTMI PHY is supported. So we have to remove this code and leave the default UTMI setting.

Actually when setting to ULPI, other platforms except the i.MX6UL refuse the settings and keep default value. So there is no problem for them. But on 6UL TO1.0, there is a IC bug that the PTW bit of PORTSC1 register which is documented as RO can change. This cause the interface setting problem with USB PHY. And finally we traced to this incorrect setting.

Quoted 10 lines

drivers/usb/gadget/ci_udc.c int usb_gadget_register_driver(struct usb_gadget_driver *driver) · line 1027
Quoted 8 lines
drivers/usb/gadget/ci_udc.c +3 -1
+	if (ret) {
+		DBG("udc probe failed, returned %d\n", ret);
+		return ret;
 	}
-#endif
 
 	ret = driver->bind(&controller.gadget);
 	if (ret) {
Best regards,
Marek Vasut

Best regards,
Ye Li

Marek Vasut

2016-01-03 20:51 UTC #3 lore raw

PATCH v3 1/1 usb:ci_udc: Remove ULPI setting for i.MX OTG controller

Quoted 57 lines · On Thursday, December 31, 2015 at 10:34:06 AM, Li Ye-B37916 wrote:

Quoted 41 lines · Marek Vasut
Quoted 40 lines · Peng Fan
From: "Ye.Li" <[email protected]>
All the i.MX6, i.MX23 and i.MX28 OTG controllers only support UTMI interface. Set to ULPI is not correct, even the controller will reject this setting in PORTSC register.
Signed-off-by Ye.Li <[email protected]>
Signed-off-by Peng Fan <[email protected]>
---
Hi Marek,
In V2: https://patchwork.ozlabs.org/patch/541663/
The reason still use DBG, but not change to debug, is that the driver use DBG for debugging and error handling usage.
So In this patch, I still keep DBG. If you think, it better to switch to use function debug, I can write another patch to convert all the DBGs to debug for ci_udc driver.
Regards,
Peng.
Changes since v2:
- Correct code indent.
Changes since v1:
- Add error handling for udc probe
 drivers/usb/gadget/ci_udc.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)
drivers/usb/gadget/ci_udc.c +0 -0
index 1ba5054..d36bcf6 100644
@@ -1018,18 +1018,10 @@ int usb_gadget_register_driver(struct
 usb_gadget_driver *driver) return ret;
 
 	ret = ci_udc_probe();
 
Quoted 14 lines
drivers/usb/gadget/ci_udc.c int usb_gadget_register_driver(struct usb_gadget_driver *driver) · line 1030
Quoted 10 lines
drivers/usb/gadget/ci_udc.c +0 ~~-10~~
-#if defined(CONFIG_USB_EHCI_MX6) || defined(CONFIG_USB_EHCI_MXS)
-	/*
-	 * FIXME: usb_lowlevel_init()->ehci_hcd_init() should be doing all
-	 * HW-specific initialization, e.g. ULPI-vs-UTMI PHY selection
-	 */
-	if (!ret) {
-		struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
-
-		/* select ULPI phy */
-		writel(PTS(PTS_ENABLE) | PFSC, &udc->portsc);
So if ci_udc_probe() doesn't fail, this code will select PTS_ENABLE and PFSC in the portsc register, right ? If you remove this code, who will do this configuration ? I suspect this breaks all i.MXes, no ?

Quoted 3 lines

Selecting PTS_ENABLE and PFSC are NOT correct for i.MXes. This code wants to select ULPI PHY, but only UTMI PHY is supported. So we have to remove this code and leave the default UTMI setting.

Yeah, the code does look nonsensical. I am wondering how this ever worked. CCing Eric, Fabio, Tim.

Quoted 5 lines

Actually when setting to ULPI, other platforms except the i.MX6UL refuse the settings and keep default value. So there is no problem for them. But on 6UL TO1.0, there is a IC bug that the PTW bit of PORTSC1 register which is documented as RO can change. This cause the interface setting problem with USB PHY. And finally we traced to this incorrect setting.

I see.

I'd like to pick this, but I'd also like to get some Tested-bys on this first.

Quoted 16 lines

Quoted 6 lines
Quoted 6 lines
drivers/usb/gadget/ci_udc.c +3 -0
+	if (ret) {
+		DBG("udc probe failed, returned %d\n", ret);
+		return ret;
 
 	}
 
Quoted 7 lines
drivers/usb/gadget/ci_udc.c int usb_gadget_register_driver(struct usb_gadget_driver *driver) · line 1027
Quoted 4 lines
drivers/usb/gadget/ci_udc.c +0 -1
-#endif
drivers/usb/gadget/ci_udc.c +0 -0
 	ret = driver->bind(&controller.gadget);
 	if (ret) {
Best regards,
Marek Vasut
Best regards,
Ye Li

Marek Vasut

2016-01-05 11:40 UTC #4 lore raw

PATCH v3 1/1 usb:ci_udc: Remove ULPI setting for i.MX OTG controller

In reply to Li Ye-B37916

Quoted 61 lines · On Thursday, December 31, 2015 at 10:34:06 AM, Li Ye-B37916 wrote:

Quoted 41 lines · Marek Vasut
Quoted 40 lines · Peng Fan
From: "Ye.Li" <[email protected]>
All the i.MX6, i.MX23 and i.MX28 OTG controllers only support UTMI interface. Set to ULPI is not correct, even the controller will reject this setting in PORTSC register.
Signed-off-by Ye.Li <[email protected]>
Signed-off-by Peng Fan <[email protected]>
---
Hi Marek,
In V2: https://patchwork.ozlabs.org/patch/541663/
The reason still use DBG, but not change to debug, is that the driver use DBG for debugging and error handling usage.
So In this patch, I still keep DBG. If you think, it better to switch to use function debug, I can write another patch to convert all the DBGs to debug for ci_udc driver.
Regards,
Peng.
Changes since v2:
- Correct code indent.
Changes since v1:
- Add error handling for udc probe
 drivers/usb/gadget/ci_udc.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)
drivers/usb/gadget/ci_udc.c +0 -0
index 1ba5054..d36bcf6 100644
@@ -1018,18 +1018,10 @@ int usb_gadget_register_driver(struct
 usb_gadget_driver *driver) return ret;
 
 	ret = ci_udc_probe();
 
Quoted 14 lines
drivers/usb/gadget/ci_udc.c int usb_gadget_register_driver(struct usb_gadget_driver *driver) · line 1030
Quoted 10 lines
drivers/usb/gadget/ci_udc.c +0 ~~-10~~
-#if defined(CONFIG_USB_EHCI_MX6) || defined(CONFIG_USB_EHCI_MXS)
-	/*
-	 * FIXME: usb_lowlevel_init()->ehci_hcd_init() should be doing all
-	 * HW-specific initialization, e.g. ULPI-vs-UTMI PHY selection
-	 */
-	if (!ret) {
-		struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
-
-		/* select ULPI phy */
-		writel(PTS(PTS_ENABLE) | PFSC, &udc->portsc);
So if ci_udc_probe() doesn't fail, this code will select PTS_ENABLE and PFSC in the portsc register, right ? If you remove this code, who will do this configuration ? I suspect this breaks all i.MXes, no ?
Selecting PTS_ENABLE and PFSC are NOT correct for i.MXes. This code wants to select ULPI PHY, but only UTMI PHY is supported. So we have to remove this code and leave the default UTMI setting.

Quoted 5 lines

Actually when setting to ULPI, other platforms except the i.MX6UL refuse the settings and keep default value. So there is no problem for them. But on 6UL TO1.0, there is a IC bug that the PTW bit of PORTSC1 register which is documented as RO can change. This cause the interface setting problem with USB PHY. And finally we traced to this incorrect setting.

Tested on i.MX28 M28EVK board and applied, thanks.