[PATCH 0/1] mx31: Fix usb otg host initialisation

[PATCH 0/1] mx31: Fix usb otg host initialisation

[Philippe Rétornaz]

Hello

This patch fix the usb otg host initialisation on imx31 platform.
The ULPI access had timeout problem. This try to fix them by reseting 
the host controller before accessing the PHY.
With a such reset the configuration registers need to be rewritten after
the ULPI access.

Tested on 3 different board with an imx31.

Regards,
Philippe


Philippe Retornaz (1):
  ehci-mxc: Fix mx31 OTG host initialisation

 drivers/usb/host/ehci-mxc.c |   68 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)

[PATCH 1/1] ehci-mxc: Fix mx31 OTG host initialisation

[Philippe Rétornaz]

On mx31 the OTG host initialisation fail if you need to have
an ULPI transfert to initialize the PHY.

In order to be able to communicate with the PHY a complete reset
of the usb host is needed. After the PHY initialization the host
usb configuration registers need to be rewritten to avoid a host
controller lockup.

Signed-off-by: Philippe R?tornaz <philippe.retornaz@epfl.ch>
---
 drivers/usb/host/ehci-mxc.c |   68 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c
index 544ccfd..39d28da 100644
--- a/drivers/usb/host/ehci-mxc.c
+++ b/drivers/usb/host/ehci-mxc.c
@@ -29,6 +29,11 @@
 #define PORTSC_OFFSET		0x184
 #define USBMODE_OFFSET		0x1a8
 #define USBMODE_CM_HOST		3
+#define USBCMD_OFFSET		0x140
+#define USBCMD_RS		(1 << 0)
+#define USBCMD_RST		(1 << 1)
+#define USBSTS_OFFSET		0x144
+#define USBSTS_HCH		(1 << 12)
 
 struct ehci_mxc_priv {
 	struct clk *usbclk, *ahbclk;
@@ -120,6 +125,7 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
 	int irq, ret, temp;
 	struct ehci_mxc_priv *priv;
 	struct device *dev = &pdev->dev;
+	int i;
 
 	dev_info(&pdev->dev, "initializing i.MX USB Controller\n");
 
@@ -204,6 +210,51 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_init;
 
+	/* i.Mx31 OTG host has a bug, if you don't do a reset, then ULPI
+	 * transfert timeout. */
+	if (cpu_is_mx31() && pdev->id == 0) {
+		/* Wait for the controller to go idle */
+		for (i = 0; i < 10000; i++) {
+			if (readl(hcd->regs + USBSTS_OFFSET) & USBSTS_HCH)
+				break;
+			udelay(1);
+		}
+		if (i == 10000) {
+			dev_err(dev, "Timeout while stopping USB controller\n");
+			goto err_init;
+		}
+
+		/* Stop the usb controller */
+		temp = readl(hcd->regs + USBCMD_OFFSET);
+		writel(temp & (~USBCMD_RS), hcd->regs + USBCMD_OFFSET);
+
+		for (i = 0; i < 10000; i++) {
+			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RS))
+				break;
+			udelay(1);
+		}
+
+		if (i == 10000) {
+			dev_err(dev, "Timeout while stopping USB controller\n");
+			goto err_init;
+		}
+
+		/* Reset the usb controller */
+		temp = readl(hcd->regs + USBCMD_OFFSET);
+		writel(temp | USBCMD_RST, hcd->regs + USBCMD_OFFSET);
+
+		for (i = 0; i < 10000; i++) {
+			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RST))
+				break;
+			udelay(1);
+		}
+
+		if (i == 10000) {
+			dev_err(dev, "Timeout while reseting USB controller\n");
+			goto err_init;
+		}
+	}
+
 	/* Initialize the transceiver */
 	if (pdata->otg) {
 		pdata->otg->io_priv = hcd->regs + ULPI_VIEWPORT_OFFSET;
@@ -213,6 +264,23 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
 			dev_err(dev, "unable to enable vbus on transceiver\n");
 	}
 
+	/* i.Mx31 OTG host has a bug, if you do an ULPI transfert then the host
+	 * controller stay busy. Rewriting the register is enough to make it
+	 * working */
+	if (cpu_is_mx31() && pdev->id == 0) {
+		/* set USBMODE to host mode */
+		temp = readl(hcd->regs + USBMODE_OFFSET);
+		writel(temp | USBMODE_CM_HOST, hcd->regs + USBMODE_OFFSET);
+
+		/* set up the PORTSCx register */
+		writel(pdata->portsc, hcd->regs + PORTSC_OFFSET);
+
+		/* setup USBCONTROL. */
+		ret = mxc_initialize_usb_hw(pdev->id, pdata->flags);
+		if (ret < 0)
+			goto err_init;
+	}
+
 	priv->hcd = hcd;
 	platform_set_drvdata(pdev, priv);
 
-- 
1.6.3.3

[PATCH 1/1] ehci-mxc: Fix mx31 OTG host initialisation

[Daniel Mack]

On Mon, May 10, 2010 at 08:13:40PM +0200, Philippe R?tornaz wrote:
> On mx31 the OTG host initialisation fail if you need to have
> an ULPI transfert to initialize the PHY.
> 
> In order to be able to communicate with the PHY a complete reset
> of the usb host is needed. After the PHY initialization the host
> usb configuration registers need to be rewritten to avoid a host
> controller lockup.

Given that things are wired up correctly on the board, yes. Many boards
don't, as they copy-pasted the schematics which got it wrong.

Anyway, if it helps you, it should go in. Some commenty below ...

> Signed-off-by: Philippe R?tornaz <philippe.retornaz@epfl.ch>
> ---
>  drivers/usb/host/ehci-mxc.c |   68 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c
> index 544ccfd..39d28da 100644
> --- a/drivers/usb/host/ehci-mxc.c
> +++ b/drivers/usb/host/ehci-mxc.c
> @@ -29,6 +29,11 @@
>  #define PORTSC_OFFSET		0x184
>  #define USBMODE_OFFSET		0x1a8
>  #define USBMODE_CM_HOST		3
> +#define USBCMD_OFFSET		0x140
> +#define USBCMD_RS		(1 << 0)
> +#define USBCMD_RST		(1 << 1)
> +#define USBSTS_OFFSET		0x144
> +#define USBSTS_HCH		(1 << 12)
>  
>  struct ehci_mxc_priv {
>  	struct clk *usbclk, *ahbclk;
> @@ -120,6 +125,7 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>  	int irq, ret, temp;
>  	struct ehci_mxc_priv *priv;
>  	struct device *dev = &pdev->dev;
> +	int i;
>  
>  	dev_info(&pdev->dev, "initializing i.MX USB Controller\n");
>  
> @@ -204,6 +210,51 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err_init;
>  
> +	/* i.Mx31 OTG host has a bug, if you don't do a reset, then ULPI
> +	 * transfert timeout. */
> +	if (cpu_is_mx31() && pdev->id == 0) {
> +		/* Wait for the controller to go idle */
> +		for (i = 0; i < 10000; i++) {
> +			if (readl(hcd->regs + USBSTS_OFFSET) & USBSTS_HCH)
> +				break;
> +			udelay(1);
> +		}

Please use a #defined value rather than the 10000 magic. Also, use
cpu_relax() instead of the udelay(1).

> +		if (i == 10000) {
> +			dev_err(dev, "Timeout while stopping USB controller\n");
> +			goto err_init;
> +		}
> +
> +		/* Stop the usb controller */
> +		temp = readl(hcd->regs + USBCMD_OFFSET);
> +		writel(temp & (~USBCMD_RS), hcd->regs + USBCMD_OFFSET);
> +
> +		for (i = 0; i < 10000; i++) {
> +			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RS))
> +				break;
> +			udelay(1);
> +		}
> +
> +		if (i == 10000) {
> +			dev_err(dev, "Timeout while stopping USB controller\n");
> +			goto err_init;
> +		}

This seems to be done all over the place. Wouldn't a static inline
function simplify the code a lot here?

Daniel


> +		/* Reset the usb controller */
> +		temp = readl(hcd->regs + USBCMD_OFFSET);
> +		writel(temp | USBCMD_RST, hcd->regs + USBCMD_OFFSET);
> +
> +		for (i = 0; i < 10000; i++) {
> +			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RST))
> +				break;
> +			udelay(1);
> +		}
> +
> +		if (i == 10000) {
> +			dev_err(dev, "Timeout while reseting USB controller\n");
> +			goto err_init;
> +		}
> +	}
> +
>  	/* Initialize the transceiver */
>  	if (pdata->otg) {
>  		pdata->otg->io_priv = hcd->regs + ULPI_VIEWPORT_OFFSET;
> @@ -213,6 +264,23 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>  			dev_err(dev, "unable to enable vbus on transceiver\n");
>  	}
>  
> +	/* i.Mx31 OTG host has a bug, if you do an ULPI transfert then the host
> +	 * controller stay busy. Rewriting the register is enough to make it
> +	 * working */
> +	if (cpu_is_mx31() && pdev->id == 0) {
> +		/* set USBMODE to host mode */
> +		temp = readl(hcd->regs + USBMODE_OFFSET);
> +		writel(temp | USBMODE_CM_HOST, hcd->regs + USBMODE_OFFSET);
> +
> +		/* set up the PORTSCx register */
> +		writel(pdata->portsc, hcd->regs + PORTSC_OFFSET);
> +
> +		/* setup USBCONTROL. */
> +		ret = mxc_initialize_usb_hw(pdev->id, pdata->flags);
> +		if (ret < 0)
> +			goto err_init;
> +	}
> +
>  	priv->hcd = hcd;
>  	platform_set_drvdata(pdev, priv);
>  
> -- 
> 1.6.3.3
> 

[PATCH 1/1] ehci-mxc: Fix mx31 OTG host initialisation

[Nguyen Dinh-R00091]

 -----Original Message-----
From: Philippe R?tornaz [mailto:philippe.retornaz at epfl.ch] 
Sent: Monday, May 10, 2010 1:14 PM
To: linux-arm-kernel at lists.infradead.org; linux-usb at vger.kernel.org
Cc: s.hauer at pengutronix.de; valentin.longchamp at epfl.ch; daniel at caiaq.de; Nguyen Dinh-R00091; gregkh at suse.de; Philippe R?tornaz
Subject: [PATCH 1/1] ehci-mxc: Fix mx31 OTG host initialisation

On mx31 the OTG host initialisation fail if you need to have an ULPI transfert to initialize the PHY.

In order to be able to communicate with the PHY a complete reset of the usb host is needed. After the PHY initialization the host usb configuration registers need to be rewritten to avoid a host controller lockup.

Signed-off-by: Philippe R?tornaz <philippe.retornaz@epfl.ch>
---
 drivers/usb/host/ehci-mxc.c |   68 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c index 544ccfd..39d28da 100644
--- a/drivers/usb/host/ehci-mxc.c
+++ b/drivers/usb/host/ehci-mxc.c
@@ -29,6 +29,11 @@
 #define PORTSC_OFFSET		0x184
 #define USBMODE_OFFSET		0x1a8
 #define USBMODE_CM_HOST		3
+#define USBCMD_OFFSET		0x140
+#define USBCMD_RS		(1 << 0)
+#define USBCMD_RST		(1 << 1)
+#define USBSTS_OFFSET		0x144
+#define USBSTS_HCH		(1 << 12)
 
 struct ehci_mxc_priv {
 	struct clk *usbclk, *ahbclk;
@@ -120,6 +125,7 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
 	int irq, ret, temp;
 	struct ehci_mxc_priv *priv;
 	struct device *dev = &pdev->dev;
+	int i;
 
 	dev_info(&pdev->dev, "initializing i.MX USB Controller\n");
 
@@ -204,6 +210,51 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_init;
 
+	/* i.Mx31 OTG host has a bug, if you don't do a reset, then ULPI
+	 * transfert timeout. */
+	if (cpu_is_mx31() && pdev->id == 0) {
+		/* Wait for the controller to go idle */
+		for (i = 0; i < 10000; i++) {
+			if (readl(hcd->regs + USBSTS_OFFSET) & USBSTS_HCH)
+				break;
+			udelay(1);
+		}
+		if (i == 10000) {
+			dev_err(dev, "Timeout while stopping USB controller\n");
+			goto err_init;
+		}
+
+		/* Stop the usb controller */
+		temp = readl(hcd->regs + USBCMD_OFFSET);
+		writel(temp & (~USBCMD_RS), hcd->regs + USBCMD_OFFSET);
+
+		for (i = 0; i < 10000; i++) {
+			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RS))
+				break;
+			udelay(1);
+		}
+
+		if (i == 10000) {
+			dev_err(dev, "Timeout while stopping USB controller\n");
+			goto err_init;
+		}
+
+		/* Reset the usb controller */
+		temp = readl(hcd->regs + USBCMD_OFFSET);
+		writel(temp | USBCMD_RST, hcd->regs + USBCMD_OFFSET);
+
+		for (i = 0; i < 10000; i++) {
+			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RST))
+				break;
+			udelay(1);
+		}
+
+		if (i == 10000) {
+			dev_err(dev, "Timeout while reseting USB controller\n");
+			goto err_init;
+		}
+	}
+
 	/* Initialize the transceiver */
 	if (pdata->otg) {
 		pdata->otg->io_priv = hcd->regs + ULPI_VIEWPORT_OFFSET; @@ -213,6 +264,23 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
 			dev_err(dev, "unable to enable vbus on transceiver\n");
 	}
 
+	/* i.Mx31 OTG host has a bug, if you do an ULPI transfert then the host
+	 * controller stay busy. Rewriting the register is enough to make it
+	 * working */
+	if (cpu_is_mx31() && pdev->id == 0) {
+		/* set USBMODE to host mode */
+		temp = readl(hcd->regs + USBMODE_OFFSET);
+		writel(temp | USBMODE_CM_HOST, hcd->regs + USBMODE_OFFSET);
+
+		/* set up the PORTSCx register */
+		writel(pdata->portsc, hcd->regs + PORTSC_OFFSET);
+
+		/* setup USBCONTROL. */
+		ret = mxc_initialize_usb_hw(pdev->id, pdata->flags);
+		if (ret < 0)
+			goto err_init;
+	}
+

[Dinh Nguyen] - Should this be done in mxc_initialize_usb_hw() in ehci.c? The probe function gets kinda of ugly with this.

 	priv->hcd = hcd;
 	platform_set_drvdata(pdev, priv);
 
--
1.6.3.3

[PATCH 1/1] ehci-mxc: Fix mx31 OTG host initialisation

[Sascha Hauer]

On Mon, May 10, 2010 at 08:13:40PM +0200, Philippe R?tornaz wrote:
> On mx31 the OTG host initialisation fail if you need to have
> an ULPI transfert to initialize the PHY.
> 
> In order to be able to communicate with the PHY a complete reset
> of the usb host is needed. After the PHY initialization the host
> usb configuration registers need to be rewritten to avoid a host
> controller lockup.
> 
> Signed-off-by: Philippe R?tornaz <philippe.retornaz@epfl.ch>
> ---
>  drivers/usb/host/ehci-mxc.c |   68 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c
> index 544ccfd..39d28da 100644
> --- a/drivers/usb/host/ehci-mxc.c
> +++ b/drivers/usb/host/ehci-mxc.c
> @@ -29,6 +29,11 @@
>  #define PORTSC_OFFSET		0x184
>  #define USBMODE_OFFSET		0x1a8
>  #define USBMODE_CM_HOST		3
> +#define USBCMD_OFFSET		0x140
> +#define USBCMD_RS		(1 << 0)
> +#define USBCMD_RST		(1 << 1)
> +#define USBSTS_OFFSET		0x144
> +#define USBSTS_HCH		(1 << 12)
>  
>  struct ehci_mxc_priv {
>  	struct clk *usbclk, *ahbclk;
> @@ -120,6 +125,7 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>  	int irq, ret, temp;
>  	struct ehci_mxc_priv *priv;
>  	struct device *dev = &pdev->dev;
> +	int i;
>  
>  	dev_info(&pdev->dev, "initializing i.MX USB Controller\n");
>  
> @@ -204,6 +210,51 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err_init;
>  
> +	/* i.Mx31 OTG host has a bug, if you don't do a reset, then ULPI
> +	 * transfert timeout. */

s/transfert/transfers/
Same below.

> +	if (cpu_is_mx31() && pdev->id == 0) {
> +		/* Wait for the controller to go idle */
> +		for (i = 0; i < 10000; i++) {
> +			if (readl(hcd->regs + USBSTS_OFFSET) & USBSTS_HCH)
> +				break;
> +			udelay(1);
> +		}
> +		if (i == 10000) {
> +			dev_err(dev, "Timeout while stopping USB controller\n");
> +			goto err_init;
> +		}
> +
> +		/* Stop the usb controller */
> +		temp = readl(hcd->regs + USBCMD_OFFSET);
> +		writel(temp & (~USBCMD_RS), hcd->regs + USBCMD_OFFSET);
> +
> +		for (i = 0; i < 10000; i++) {
> +			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RS))
> +				break;
> +			udelay(1);
> +		}
> +
> +		if (i == 10000) {
> +			dev_err(dev, "Timeout while stopping USB controller\n");
> +			goto err_init;
> +		}
> +
> +		/* Reset the usb controller */
> +		temp = readl(hcd->regs + USBCMD_OFFSET);
> +		writel(temp | USBCMD_RST, hcd->regs + USBCMD_OFFSET);
> +
> +		for (i = 0; i < 10000; i++) {
> +			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RST))
> +				break;
> +			udelay(1);
> +		}
> +
> +		if (i == 10000) {
> +			dev_err(dev, "Timeout while reseting USB controller\n");
> +			goto err_init;
> +		}
> +	}

You add the resetting of the controller after setting up USBMODE/PORTSC
setup. Wouldn't it be possible to change the order so that we do not
have to do it twice?
Also, I think the whole reset functionality should be a seperate
function. I can imagine we'll need it elsewhere or on different SoCs in
which case the if clause above gets complicated.

Sascha


> +
>  	/* Initialize the transceiver */
>  	if (pdata->otg) {
>  		pdata->otg->io_priv = hcd->regs + ULPI_VIEWPORT_OFFSET;
> @@ -213,6 +264,23 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>  			dev_err(dev, "unable to enable vbus on transceiver\n");
>  	}
>  
> +	/* i.Mx31 OTG host has a bug, if you do an ULPI transfert then the host
> +	 * controller stay busy. Rewriting the register is enough to make it
> +	 * working */
> +	if (cpu_is_mx31() && pdev->id == 0) {
> +		/* set USBMODE to host mode */
> +		temp = readl(hcd->regs + USBMODE_OFFSET);
> +		writel(temp | USBMODE_CM_HOST, hcd->regs + USBMODE_OFFSET);
> +
> +		/* set up the PORTSCx register */
> +		writel(pdata->portsc, hcd->regs + PORTSC_OFFSET);
> +
> +		/* setup USBCONTROL. */
> +		ret = mxc_initialize_usb_hw(pdev->id, pdata->flags);
> +		if (ret < 0)
> +			goto err_init;
> +	}
> +
>  	priv->hcd = hcd;
>  	platform_set_drvdata(pdev, priv);
>  
> -- 
> 1.6.3.3
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/1] ehci-mxc: Fix mx31 OTG host initialisation

[Valentin Longchamp]

See the comments below.

On 05/11/2010 08:39 AM, Sascha Hauer wrote:
> On Mon, May 10, 2010 at 08:13:40PM +0200, Philippe R?tornaz wrote:
>> On mx31 the OTG host initialisation fail if you need to have
>> an ULPI transfert to initialize the PHY.
>>
>> In order to be able to communicate with the PHY a complete reset
>> of the usb host is needed. After the PHY initialization the host
>> usb configuration registers need to be rewritten to avoid a host
>> controller lockup.
>>
>> Signed-off-by: Philippe R?tornaz<philippe.retornaz@epfl.ch>
>> ---
>>   drivers/usb/host/ehci-mxc.c |   68 +++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 68 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c
>> index 544ccfd..39d28da 100644
>> --- a/drivers/usb/host/ehci-mxc.c
>> +++ b/drivers/usb/host/ehci-mxc.c
>> @@ -29,6 +29,11 @@
>>   #define PORTSC_OFFSET		0x184
>>   #define USBMODE_OFFSET		0x1a8
>>   #define USBMODE_CM_HOST		3
>> +#define USBCMD_OFFSET		0x140
>> +#define USBCMD_RS		(1<<  0)
>> +#define USBCMD_RST		(1<<  1)
>> +#define USBSTS_OFFSET		0x144
>> +#define USBSTS_HCH		(1<<  12)
>>
>>   struct ehci_mxc_priv {
>>   	struct clk *usbclk, *ahbclk;
>> @@ -120,6 +125,7 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>>   	int irq, ret, temp;
>>   	struct ehci_mxc_priv *priv;
>>   	struct device *dev =&pdev->dev;
>> +	int i;
>>
>>   	dev_info(&pdev->dev, "initializing i.MX USB Controller\n");
>>
>> @@ -204,6 +210,51 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>>   	if (ret<  0)
>>   		goto err_init;
>>
>> +	/* i.Mx31 OTG host has a bug, if you don't do a reset, then ULPI
>> +	 * transfert timeout. */
>
> s/transfert/transfers/
> Same below.
>
>> +	if (cpu_is_mx31()&&  pdev->id == 0) {
>> +		/* Wait for the controller to go idle */
>> +		for (i = 0; i<  10000; i++) {
>> +			if (readl(hcd->regs + USBSTS_OFFSET)&  USBSTS_HCH)
>> +				break;
>> +			udelay(1);
>> +		}
>> +		if (i == 10000) {
>> +			dev_err(dev, "Timeout while stopping USB controller\n");
>> +			goto err_init;
>> +		}
>> +
>> +		/* Stop the usb controller */
>> +		temp = readl(hcd->regs + USBCMD_OFFSET);
>> +		writel(temp&  (~USBCMD_RS), hcd->regs + USBCMD_OFFSET);
>> +
>> +		for (i = 0; i<  10000; i++) {
>> +			if (!(readl(hcd->regs + USBCMD_OFFSET)&  USBCMD_RS))
>> +				break;
>> +			udelay(1);
>> +		}
>> +
>> +		if (i == 10000) {
>> +			dev_err(dev, "Timeout while stopping USB controller\n");
>> +			goto err_init;
>> +		}
>> +
>> +		/* Reset the usb controller */
>> +		temp = readl(hcd->regs + USBCMD_OFFSET);
>> +		writel(temp | USBCMD_RST, hcd->regs + USBCMD_OFFSET);
>> +
>> +		for (i = 0; i<  10000; i++) {
>> +			if (!(readl(hcd->regs + USBCMD_OFFSET)&  USBCMD_RST))
>> +				break;
>> +			udelay(1);
>> +		}
>> +
>> +		if (i == 10000) {
>> +			dev_err(dev, "Timeout while reseting USB controller\n");
>> +			goto err_init;
>> +		}
>> +	}
>
> You add the resetting of the controller after setting up USBMODE/PORTSC
> setup. Wouldn't it be possible to change the order so that we do not
> have to do it twice?

It has to be tested, but if it works the code would then be clearer.

> Also, I think the whole reset functionality should be a seperate
> function. I can imagine we'll need it elsewhere or on different SoCs in
> which case the if clause above gets complicated.

I also think most of the things could be implemented as functions for a 
better readability (reset, and as Mark suggested a static inline 
function for the delay).

Val

>
> Sascha
>
>
>> +
>>   	/* Initialize the transceiver */
>>   	if (pdata->otg) {
>>   		pdata->otg->io_priv = hcd->regs + ULPI_VIEWPORT_OFFSET;
>> @@ -213,6 +264,23 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>>   			dev_err(dev, "unable to enable vbus on transceiver\n");
>>   	}
>>
>> +	/* i.Mx31 OTG host has a bug, if you do an ULPI transfert then the host
>> +	 * controller stay busy. Rewriting the register is enough to make it
>> +	 * working */
>> +	if (cpu_is_mx31()&&  pdev->id == 0) {
>> +		/* set USBMODE to host mode */
>> +		temp = readl(hcd->regs + USBMODE_OFFSET);
>> +		writel(temp | USBMODE_CM_HOST, hcd->regs + USBMODE_OFFSET);
>> +
>> +		/* set up the PORTSCx register */
>> +		writel(pdata->portsc, hcd->regs + PORTSC_OFFSET);
>> +
>> +		/* setup USBCONTROL. */
>> +		ret = mxc_initialize_usb_hw(pdev->id, pdata->flags);
>> +		if (ret<  0)
>> +			goto err_init;
>> +	}
>> +
>>   	priv->hcd = hcd;
>>   	platform_set_drvdata(pdev, priv);
>>
>> --
>> 1.6.3.3
>>
>>
>


-- 
Valentin Longchamp, PhD Student, EPFL-STI-LSRO1
valentin.longchamp at epfl.ch, Phone: +41216937827
http://people.epfl.ch/valentin.longchamp
MEB3494, Station 9, CH-1015 Lausanne

[PATCH 1/1] ehci-mxc: Fix mx31 OTG host initialisation

[Valentin Longchamp]

On 05/10/2010 08:58 PM, Nguyen Dinh-R00091 wrote:
>   -----Original Message-----
> From: Philippe R?tornaz [mailto:philippe.retornaz at epfl.ch]
> Sent: Monday, May 10, 2010 1:14 PM
> To: linux-arm-kernel at lists.infradead.org; linux-usb at vger.kernel.org
> Cc: s.hauer at pengutronix.de; valentin.longchamp at epfl.ch; daniel at caiaq.de; Nguyen Dinh-R00091; gregkh at suse.de; Philippe R?tornaz
> Subject: [PATCH 1/1] ehci-mxc: Fix mx31 OTG host initialisation
>
> On mx31 the OTG host initialisation fail if you need to have an ULPI transfert to initialize the PHY.
>
> In order to be able to communicate with the PHY a complete reset of the usb host is needed. After the PHY initialization the host usb configuration registers need to be rewritten to avoid a host controller lockup.
>
> Signed-off-by: Philippe R?tornaz<philippe.retornaz@epfl.ch>
> ---
>   drivers/usb/host/ehci-mxc.c |   68 +++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 68 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c index 544ccfd..39d28da 100644
> --- a/drivers/usb/host/ehci-mxc.c
> +++ b/drivers/usb/host/ehci-mxc.c
> @@ -29,6 +29,11 @@
>   #define PORTSC_OFFSET		0x184
>   #define USBMODE_OFFSET		0x1a8
>   #define USBMODE_CM_HOST		3
> +#define USBCMD_OFFSET		0x140
> +#define USBCMD_RS		(1<<  0)
> +#define USBCMD_RST		(1<<  1)
> +#define USBSTS_OFFSET		0x144
> +#define USBSTS_HCH		(1<<  12)
>
>   struct ehci_mxc_priv {
>   	struct clk *usbclk, *ahbclk;
> @@ -120,6 +125,7 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>   	int irq, ret, temp;
>   	struct ehci_mxc_priv *priv;
>   	struct device *dev =&pdev->dev;
> +	int i;
>
>   	dev_info(&pdev->dev, "initializing i.MX USB Controller\n");
>
> @@ -204,6 +210,51 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>   	if (ret<  0)
>   		goto err_init;
>
> +	/* i.Mx31 OTG host has a bug, if you don't do a reset, then ULPI
> +	 * transfert timeout. */
> +	if (cpu_is_mx31()&&  pdev->id == 0) {
> +		/* Wait for the controller to go idle */
> +		for (i = 0; i<  10000; i++) {
> +			if (readl(hcd->regs + USBSTS_OFFSET)&  USBSTS_HCH)
> +				break;
> +			udelay(1);
> +		}
> +		if (i == 10000) {
> +			dev_err(dev, "Timeout while stopping USB controller\n");
> +			goto err_init;
> +		}
> +
> +		/* Stop the usb controller */
> +		temp = readl(hcd->regs + USBCMD_OFFSET);
> +		writel(temp&  (~USBCMD_RS), hcd->regs + USBCMD_OFFSET);
> +
> +		for (i = 0; i<  10000; i++) {
> +			if (!(readl(hcd->regs + USBCMD_OFFSET)&  USBCMD_RS))
> +				break;
> +			udelay(1);
> +		}
> +
> +		if (i == 10000) {
> +			dev_err(dev, "Timeout while stopping USB controller\n");
> +			goto err_init;
> +		}
> +
> +		/* Reset the usb controller */
> +		temp = readl(hcd->regs + USBCMD_OFFSET);
> +		writel(temp | USBCMD_RST, hcd->regs + USBCMD_OFFSET);
> +
> +		for (i = 0; i<  10000; i++) {
> +			if (!(readl(hcd->regs + USBCMD_OFFSET)&  USBCMD_RST))
> +				break;
> +			udelay(1);
> +		}
> +
> +		if (i == 10000) {
> +			dev_err(dev, "Timeout while reseting USB controller\n");
> +			goto err_init;
> +		}
> +	}
> +
>   	/* Initialize the transceiver */
>   	if (pdata->otg) {
>   		pdata->otg->io_priv = hcd->regs + ULPI_VIEWPORT_OFFSET; @@ -213,6 +264,23 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>   			dev_err(dev, "unable to enable vbus on transceiver\n");
>   	}
>
> +	/* i.Mx31 OTG host has a bug, if you do an ULPI transfert then the host
> +	 * controller stay busy. Rewriting the register is enough to make it
> +	 * working */
> +	if (cpu_is_mx31()&&  pdev->id == 0) {
> +		/* set USBMODE to host mode */
> +		temp = readl(hcd->regs + USBMODE_OFFSET);
> +		writel(temp | USBMODE_CM_HOST, hcd->regs + USBMODE_OFFSET);
> +
> +		/* set up the PORTSCx register */
> +		writel(pdata->portsc, hcd->regs + PORTSC_OFFSET);
> +
> +		/* setup USBCONTROL. */
> +		ret = mxc_initialize_usb_hw(pdev->id, pdata->flags);
> +		if (ret<  0)
> +			goto err_init;
> +	}
> +
>
> [Dinh Nguyen] - Should this be done in mxc_initialize_usb_hw() in ehci.c? The probe function gets kinda of ugly with this.
>
>   	priv->hcd = hcd;
>   	platform_set_drvdata(pdev, priv);

 From what I seen in arch/arm/plat-mxc/ehci.c (at least for mx31 for 
which we have the documentation), we do not write the hcd->regs. I think 
it is clearer to write all the hcd->regs at the same place, and 
ehci-mxc.c looks the correct place to do it.

Now the code could be cleaner with the help of a few additionnal 
functions maybe, as already suggested by Sascha and Mark.

Val

-- 
Valentin Longchamp, PhD Student, EPFL-STI-LSRO1
valentin.longchamp at epfl.ch, Phone: +41216937827
http://people.epfl.ch/valentin.longchamp
MEB3494, Station 9, CH-1015 Lausanne

[PATCH 1/1] ehci-mxc: Fix mx31 OTG host initialisation

[Philippe Rétornaz]

Le mardi, 11 mai 2010 08.39:07, Sascha Hauer a ?crit :
> On Mon, May 10, 2010 at 08:13:40PM +0200, Philippe R?tornaz wrote:
> 
> > +	if (cpu_is_mx31() && pdev->id == 0) {
> > +		/* Wait for the controller to go idle */
> > +		for (i = 0; i < 10000; i++) {
> > +			if (readl(hcd->regs + USBSTS_OFFSET) & USBSTS_HCH)
> > +				break;
> > +			udelay(1);
> > +		}
> > +		if (i == 10000) {
> > +			dev_err(dev, "Timeout while stopping USB controller\n");
> > +			goto err_init;
> > +		}
> > +
> > +		/* Stop the usb controller */
> > +		temp = readl(hcd->regs + USBCMD_OFFSET);
> > +		writel(temp & (~USBCMD_RS), hcd->regs + USBCMD_OFFSET);
> > +
> > +		for (i = 0; i < 10000; i++) {
> > +			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RS))
> > +				break;
> > +			udelay(1);
> > +		}
> > +
> > +		if (i == 10000) {
> > +			dev_err(dev, "Timeout while stopping USB controller\n");
> > +			goto err_init;
> > +		}
> > +
> > +		/* Reset the usb controller */
> > +		temp = readl(hcd->regs + USBCMD_OFFSET);
> > +		writel(temp | USBCMD_RST, hcd->regs + USBCMD_OFFSET);
> > +
> > +		for (i = 0; i < 10000; i++) {
> > +			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RST))
> > +				break;
> > +			udelay(1);
> > +		}
> > +
> > +		if (i == 10000) {
> > +			dev_err(dev, "Timeout while reseting USB controller\n");
> > +			goto err_init;
> > +		}
> > +	}
> 
> You add the resetting of the controller after setting up USBMODE/PORTSC
> setup. Wouldn't it be possible to change the order so that we do not
> have to do it twice?

I tried and it does work on host2, but not on OTG. 

I did it this way so I don't change the behavior on other SoC or even on 
host2. 

> Also, I think the whole reset functionality should be a seperate
> function. I can imagine we'll need it elsewhere or on different SoCs in
> which case the if clause above gets complicated.

Ok.

Philippe

[PATCH 1/1] ehci-mxc: Fix mx31 OTG host initialisation

[Philippe Rétornaz]

Le lundi, 10 mai 2010 20.35:09, Daniel Mack a ?crit :
> On Mon, May 10, 2010 at 08:13:40PM +0200, Philippe R?tornaz wrote:
> > On mx31 the OTG host initialisation fail if you need to have
> > an ULPI transfert to initialize the PHY.
> >
> > In order to be able to communicate with the PHY a complete reset
> > of the usb host is needed. After the PHY initialization the host
> > usb configuration registers need to be rewritten to avoid a host
> > controller lockup.
> 
> Given that things are wired up correctly on the board, yes. Many boards
> don't, as they copy-pasted the schematics which got it wrong.

Can you elaborate a bit more ? I don't see how you could do it another way 
with an isp1504 and an external power switch.

> 
> Anyway, if it helps you, it should go in. Some commenty below ...
> 
> > +	/* i.Mx31 OTG host has a bug, if you don't do a reset, then ULPI
> > +	 * transfert timeout. */
> > +	if (cpu_is_mx31() && pdev->id == 0) {
> > +		/* Wait for the controller to go idle */
> > +		for (i = 0; i < 10000; i++) {
> > +			if (readl(hcd->regs + USBSTS_OFFSET) & USBSTS_HCH)
> > +				break;
> > +			udelay(1);
> > +		}
> 
> Please use a #defined value rather than the 10000 magic.

Ok.

> Also, use cpu_relax() instead of the udelay(1).

I don't argee because if I use cpu_relax() it will make the timeout cpu 
frequency dependant. With the udelay it is clear that the timeout is about 
10ms.
But of course, I can do the change if this is really the way to do it.

> 
> > +		if (i == 10000) {
> > +			dev_err(dev, "Timeout while stopping USB controller\n");
> > +			goto err_init;
> > +		}
> > +
> > +		/* Stop the usb controller */
> > +		temp = readl(hcd->regs + USBCMD_OFFSET);
> > +		writel(temp & (~USBCMD_RS), hcd->regs + USBCMD_OFFSET);
> > +
> > +		for (i = 0; i < 10000; i++) {
> > +			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RS))
> > +				break;
> > +			udelay(1);
> > +		}
> > +
> > +		if (i == 10000) {
> > +			dev_err(dev, "Timeout while stopping USB controller\n");
> > +			goto err_init;
> > +		}
> 
> This seems to be done all over the place. Wouldn't a static inline
> function simplify the code a lot here?

Ok.

Philippe

[PATCH 1/1] ehci-mxc: Fix mx31 OTG host initialisation

[Philippe Rétornaz]

>  -----Original Message-----
> From: Philippe R?tornaz [mailto:philippe.retornaz at epfl.ch]
> Sent: Monday, May 10, 2010 1:14 PM
> To: linux-arm-kernel at lists.infradead.org; linux-usb at vger.kernel.org
> Cc: s.hauer at pengutronix.de; valentin.longchamp at epfl.ch; daniel at caiaq.de;
>  Nguyen Dinh-R00091; gregkh at suse.de; Philippe R?tornaz Subject: [PATCH 1/1]
>  ehci-mxc: Fix mx31 OTG host initialisation
> 
> On mx31 the OTG host initialisation fail if you need to have an ULPI
>  transfert to initialize the PHY.
> 
> In order to be able to communicate with the PHY a complete reset of the usb
>  host is needed. After the PHY initialization the host usb configuration
>  registers need to be rewritten to avoid a host controller lockup.
> 
> Signed-off-by: Philippe R?tornaz <philippe.retornaz@epfl.ch>
> ---
>  drivers/usb/host/ehci-mxc.c |   68
>  +++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 68
>  insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c
>  index 544ccfd..39d28da 100644 --- a/drivers/usb/host/ehci-mxc.c
> +++ b/drivers/usb/host/ehci-mxc.c
> @@ -29,6 +29,11 @@
>  #define PORTSC_OFFSET		0x184
>  #define USBMODE_OFFSET		0x1a8
>  #define USBMODE_CM_HOST		3
> +#define USBCMD_OFFSET		0x140
> +#define USBCMD_RS		(1 << 0)
> +#define USBCMD_RST		(1 << 1)
> +#define USBSTS_OFFSET		0x144
> +#define USBSTS_HCH		(1 << 12)
> 
>  struct ehci_mxc_priv {
>  	struct clk *usbclk, *ahbclk;
> @@ -120,6 +125,7 @@ static int ehci_mxc_drv_probe(struct platform_device
>  *pdev) int irq, ret, temp;
>  	struct ehci_mxc_priv *priv;
>  	struct device *dev = &pdev->dev;
> +	int i;
> 
>  	dev_info(&pdev->dev, "initializing i.MX USB Controller\n");
> 
> @@ -204,6 +210,51 @@ static int ehci_mxc_drv_probe(struct platform_device
>  *pdev) if (ret < 0)
>  		goto err_init;
> 
> +	/* i.Mx31 OTG host has a bug, if you don't do a reset, then ULPI
> +	 * transfert timeout. */
> +	if (cpu_is_mx31() && pdev->id == 0) {
> +		/* Wait for the controller to go idle */
> +		for (i = 0; i < 10000; i++) {
> +			if (readl(hcd->regs + USBSTS_OFFSET) & USBSTS_HCH)
> +				break;
> +			udelay(1);
> +		}
> +		if (i == 10000) {
> +			dev_err(dev, "Timeout while stopping USB controller\n");
> +			goto err_init;
> +		}
> +
> +		/* Stop the usb controller */
> +		temp = readl(hcd->regs + USBCMD_OFFSET);
> +		writel(temp & (~USBCMD_RS), hcd->regs + USBCMD_OFFSET);
> +
> +		for (i = 0; i < 10000; i++) {
> +			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RS))
> +				break;
> +			udelay(1);
> +		}
> +
> +		if (i == 10000) {
> +			dev_err(dev, "Timeout while stopping USB controller\n");
> +			goto err_init;
> +		}
> +
> +		/* Reset the usb controller */
> +		temp = readl(hcd->regs + USBCMD_OFFSET);
> +		writel(temp | USBCMD_RST, hcd->regs + USBCMD_OFFSET);
> +
> +		for (i = 0; i < 10000; i++) {
> +			if (!(readl(hcd->regs + USBCMD_OFFSET) & USBCMD_RST))
> +				break;
> +			udelay(1);
> +		}
> +
> +		if (i == 10000) {
> +			dev_err(dev, "Timeout while reseting USB controller\n");
> +			goto err_init;
> +		}
> +	}
> +
>  	/* Initialize the transceiver */
>  	if (pdata->otg) {
>  		pdata->otg->io_priv = hcd->regs + ULPI_VIEWPORT_OFFSET; @@ -213,6
>  +264,23 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
>  dev_err(dev, "unable to enable vbus on transceiver\n");
>  	}
> 
> +	/* i.Mx31 OTG host has a bug, if you do an ULPI transfert then the host
> +	 * controller stay busy. Rewriting the register is enough to make it
> +	 * working */
> +	if (cpu_is_mx31() && pdev->id == 0) {
> +		/* set USBMODE to host mode */
> +		temp = readl(hcd->regs + USBMODE_OFFSET);
> +		writel(temp | USBMODE_CM_HOST, hcd->regs + USBMODE_OFFSET);
> +
> +		/* set up the PORTSCx register */
> +		writel(pdata->portsc, hcd->regs + PORTSC_OFFSET);
> +
> +		/* setup USBCONTROL. */
> +		ret = mxc_initialize_usb_hw(pdev->id, pdata->flags);
> +		if (ret < 0)
> +			goto err_init;
> +	}
> +
> 
> [Dinh Nguyen] - Should this be done in mxc_initialize_usb_hw() in ehci.c?
>  The probe function gets kinda of ugly with this.

I cannot do this is mxc_initialize_usb_hw() because I really need to do the 
reset before initializing the PHY which is done inside 
otg_init()/otg_set_vbus().
And the registers needs to be rewritten after the PHY initialisation. The 
whole patch is a single fix for one problem.

This is only needed on the usb OTG port, and I really suspect a hardware bug.
Since you work at freescale, maybe you could get some information about why 
the OTG port has a such strange behavior.

I will repost a patch soon addressing all the feedback I got.


Philippe