[PATCH] usb: ehci: call platform init before ehci reset

[PATCH] usb: ehci: call platform init before ehci reset

[Uwe Kleine-König]

On i.MX25 platform init sets up things like the polarity of the
overcurrent pin. If the reset default value is still wrong at ehci_reset
time, this results in an overcurrent event being pending in the hardware
even if the pin is actually in it's inactive level. To prevent this call
platform init before ehci_reset().

Without this change barebox fails to correctly handle the imagined
overcurrent event resulting in the inability to access the contents of
an USB thumb drive. So there must be another problem somewhere, but I
didn't debug that. The change introduced in this patch works around this
problem but is correct on its own anyhow.

Note there is a chance that other platforms rely on the previous order,
I'm not aware of actual problems though.

The problem was debugged with Michael Grzeschik, thanks to him for his
valuable aid.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/usb/host/ehci-hcd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 9bbdda365c01..18ff6b589773 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -801,16 +801,16 @@ static int ehci_init(struct usb_host *host)
 
 	ehci_halt(ehci);
 
-	/* EHCI spec section 4.1 */
-	if (ehci_reset(ehci) != 0)
-		return -1;
-
 	if (ehci->init) {
 		ret = ehci->init(ehci->drvdata);
 		if (ret)
 			return ret;
 	}
 
+	/* EHCI spec section 4.1 */
+	if (ehci_reset(ehci) != 0)
+		return -1;
+
 	memset(ehci->qh_list, 0, sizeof(struct QH) * NUM_TD);
 
 	ehci->qh_list->qh_link = cpu_to_hc32((uint32_t)ehci->qh_list | QH_LINK_TYPE_QH);
-- 
2.19.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] usb: ehci: call platform init before ehci reset

[Sascha Hauer]

On Fri, Oct 19, 2018 at 02:07:50PM +0200, Uwe Kleine-König wrote:
> On i.MX25 platform init sets up things like the polarity of the
> overcurrent pin. If the reset default value is still wrong at ehci_reset
> time, this results in an overcurrent event being pending in the hardware
> even if the pin is actually in it's inactive level. To prevent this call
> platform init before ehci_reset().
> 
> Without this change barebox fails to correctly handle the imagined
> overcurrent event resulting in the inability to access the contents of
> an USB thumb drive. So there must be another problem somewhere, but I
> didn't debug that. The change introduced in this patch works around this
> problem but is correct on its own anyhow.
> 
> Note there is a chance that other platforms rely on the previous order,
> I'm not aware of actual problems though.
> 
> The problem was debugged with Michael Grzeschik, thanks to him for his
> valuable aid.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/usb/host/ehci-hcd.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 9bbdda365c01..18ff6b589773 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -801,16 +801,16 @@ static int ehci_init(struct usb_host *host)
>  
>  	ehci_halt(ehci);
>  
> -	/* EHCI spec section 4.1 */
> -	if (ehci_reset(ehci) != 0)
> -		return -1;
> -
>  	if (ehci->init) {
>  		ret = ehci->init(ehci->drvdata);
>  		if (ret)
>  			return ret;
>  	}
>  
> +	/* EHCI spec section 4.1 */
> +	if (ehci_reset(ehci) != 0)
> +		return -1;
> +

This will lead to problems, see imx_chipidea_port_init():

> static int imx_chipidea_port_init(void *drvdata)
> {
> 	struct imx_chipidea *ci = drvdata;
> 	uint32_t portsc;
> 	int ret;
> 
> 	if ((ci->flags & MXC_EHCI_PORTSC_MASK) == MXC_EHCI_MODE_ULPI) {
> 		dev_dbg(ci->dev, "using ULPI phy\n");
> 		if (IS_ENABLED(CONFIG_USB_ULPI)) {
> 			ret = ulpi_setup(ci->base + 0x170, 1);
> 			if (ret)
> 				dev_err(ci->dev, "ULPI setup failed with %s\n",
> 						strerror(-ret));
> 			mdelay(20);
> 		} else {
> 			dev_err(ci->dev, "no ULPI support available\n");
> 			ret = -ENODEV;
> 		}
> 
> 		if (ret)
> 			return ret;
> 	}

Not sure about ULPI support, but I wouldn't be surprised if this needs a
controller in a known, i.e. resetted state.

> 
> 	ret = imx_usbmisc_port_init(ci->usbmisc, ci->portno, ci->flags);
> 	if (ret)
> 		dev_err(ci->dev, "misc init failed: %s\n", strerror(-ret));
> 
> 	/* PFSC bit is reset by ehci_reset(), thus have to set it not in
> 	 * probe but here, after ehci_reset() is already called */
> 	if (ci->flags & MXC_EHCI_PFSC) {
> 		portsc = readl(ci->base + 0x184);
> 		portsc |= MXC_EHCI_PFSC;
> 		writel(portsc, ci->base + 0x184);
> 	}

The comment here explicitly states that the code should be executed
after ehci_reset().

Sascha

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

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox