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

[Sascha Hauer <s.hauer@pengutronix.de>]

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