diff mbox

[U-Boot,V3] usb:ci_udc: Remove ULPI setting for i.MX OTG controller

Message ID 1451546685-6275-1-git-send-email-peng.fan@nxp.com
State Accepted
Commit 8991fdef6d94b3c32722f413f399c3fce0b5061d
Delegated to: Stefano Babic
Headers show

Commit Message

Peng Fan Dec. 31, 2015, 7:24 a.m. UTC 
From: "Ye.Li" <B37916@freescale.com>

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 <B37916@freescale.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

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(-)

Comments

Marek Vasut Dec. 31, 2015, 9:07 a.m. UTC | #1 
On Thursday, December 31, 2015 at 08:24:45 AM, Peng Fan wrote:
> From: "Ye.Li" <B37916@freescale.com>
> 
> 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 <B37916@freescale.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> 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(-)
> 
> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> index 1ba5054..d36bcf6 100644
> --- a/drivers/usb/gadget/ci_udc.c
> +++ b/drivers/usb/gadget/ci_udc.c
> @@ -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);

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 ?

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

Best regards,
Marek Vasut
Ye.Li Dec. 31, 2015, 9:34 a.m. UTC | #2 
On 12/31/2015 5:07 PM, Marek Vasut wrote:
> On Thursday, December 31, 2015 at 08:24:45 AM, Peng Fan wrote:
>> From: "Ye.Li" <B37916@freescale.com>
>>
>> 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 <B37916@freescale.com>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>
>> 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(-)
>>
>> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
>> index 1ba5054..d36bcf6 100644
>> --- a/drivers/usb/gadget/ci_udc.c
>> +++ b/drivers/usb/gadget/ci_udc.c
>> @@ -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);
> 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.

>> +	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 Jan. 3, 2016, 8:51 p.m. UTC | #3 
On Thursday, December 31, 2015 at 10:34:06 AM, Li Ye-B37916 wrote:
> On 12/31/2015 5:07 PM, Marek Vasut wrote:
> > On Thursday, December 31, 2015 at 08:24:45 AM, Peng Fan wrote:
> >> From: "Ye.Li" <B37916@freescale.com>
> >> 
> >> 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 <B37916@freescale.com>
> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >> ---
> >> 
> >> 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(-)
> >> 
> >> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> >> index 1ba5054..d36bcf6 100644
> >> --- a/drivers/usb/gadget/ci_udc.c
> >> +++ b/drivers/usb/gadget/ci_udc.c
> >> @@ -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);
> > 
> > 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.

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

> 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.

> >> +	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 Jan. 5, 2016, 11:40 a.m. UTC | #4 
On Thursday, December 31, 2015 at 10:34:06 AM, Li Ye-B37916 wrote:
> On 12/31/2015 5:07 PM, Marek Vasut wrote:
> > On Thursday, December 31, 2015 at 08:24:45 AM, Peng Fan wrote:
> >> From: "Ye.Li" <B37916@freescale.com>
> >> 
> >> 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 <B37916@freescale.com>
> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >> ---
> >> 
> >> 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(-)
> >> 
> >> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> >> index 1ba5054..d36bcf6 100644
> >> --- a/drivers/usb/gadget/ci_udc.c
> >> +++ b/drivers/usb/gadget/ci_udc.c
> >> @@ -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);
> > 
> > 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.

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

Patch

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index 1ba5054..d36bcf6 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -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) {