[U-Boot] [PATCH 2/3] ARM: Tegra: USB: Add driver support for Tegra30/Tegra114

[Stephen Warren <swarren@wwwdotorg.org>]

On 04/29/2013 03:21 AM, Jim Lin wrote:
> Tegra30 and Tegra114 are compatible except
> 1. T30 takes 55 ms to finish Port Reset. T114 takes
>    50 ms.

Is that 55-vs-50 some aspect of the HW specification itself, or the
overall result of multiple separate delays? I ask because the statement
that one piece of HW differs from another only in a delay, especially
where that delay is enormous in HW terms, seem very unusual.

> diff --git a/arch/arm/include/asm/arch-tegra114/tegra.h b/arch/arm/include/asm/arch-tegra114/tegra.h

> +#define TEGRA_USB1_BASE		0x7D000000

That shouldn't be needed; if some of the module base addresses come from
DT, then they all should, or there's no point using DT.

> diff --git a/arch/arm/include/asm/arch-tegra114/usb.h b/arch/arm/include/asm/arch-tegra114/usb.h

> +#ifndef _TEGRA_USB_H_
> +#define _TEGRA_USB_H_
> +
> +
> +/* USB Controller (USBx_CONTROLLER_) regs */

There's an extra blank line there. The same problem exists elsewhere,
and in the other copies of this file.

> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c

>  void ehci_powerup_fixup(uint32_t *status_reg, uint32_t *reg)
>  {
> -	mdelay(50);
> -	if (((u32) status_reg & TEGRA_USB_ADDR_MASK) != TEGRA_USB1_BASE)
> +	udelay(49990);
> +	if (is_T30_compatible) {

The code shouldn't test against a specific chip version, but rather
against some property of the HW (like the length of the delay). That
way, it will be scalable when we support 10 different Tegra versions and
the mapping from version number to feature isn't easy to represent.

> +		/*
> +		 * Tegra 114 takes 50 ms to assert Port Enable bit.
> +		 * We have to exit earlier. Otherwise ehci-hcd.c will clear
> +		 * our Port Enable bit.
> +		 */
> +		if (is_T114_compatible)
> +			return;
> +		/* Tegra 3 takes about 55 ms to assert Port Enable bit. */
> +		udelay(5010);
> +		return;
> +	}

This looks like an enormous hack. What is it doing? Why? Can't all 3
chips simply loop polling until some enable status bit is set?

> +	udelay(10);
> +	if (((u32)status_reg & TEGRA_USB_ADDR_MASK) != TEGRA_USB1_BASE)
>  		return;

This also should test against a feature, not the identity of the port.
The DT properties nvidia,has-legacy-mode or nvidia,needs-double-reset
might be relevant here?

> @@ -171,7 +177,7 @@ static void set_host_mode(struct fdt_usb *config)
>  	 * bail out in this case.
>  	 */
>  	if (config->dr_mode == DR_MODE_OTG &&
> -		(readl(&config->reg->phy_vbus_sensors) & VBUS_VLD_STS))
> +	    (readl(&config->reg->phy_vbus_sensors) & VBUS_VLD_STS))

That's an unrelated whitespace change. Whitespace cleanup should be in a
separate patch first, so it doesn't clutter functional changes.

> @@ -232,45 +240,112 @@ static int init_utmi_usb_controller(struct fdt_usb *config)
>  	 * To Use the A Session Valid for cable detection logic, VBUS_WAKEUP
>  	 * mux must be switched to actually use a_sess_vld threshold.
>  	 */
> -	if (fdt_gpio_isvalid(&config->vbus_gpio)) {
> +	if (config->dr_mode == DR_MODE_OTG &&
> +	    fdt_gpio_isvalid(&config->vbus_gpio))
>  		clrsetbits_le32(&usbctlr->usb1_legacy_ctrl,
> -			VBUS_SENSE_CTL_MASK,
> -			VBUS_SENSE_CTL_A_SESS_VLD << VBUS_SENSE_CTL_SHIFT);
> -	}
> +				VBUS_SENSE_CTL_MASK,
> +				VBUS_SENSE_CTL_A_SESS_VLD <<
> +				VBUS_SENSE_CTL_SHIFT);

Partially a white-space change. There are many other ws-changes later
that I didn't bother pointing out.

> +	timing = (u32 *)config->params;
> +	if (is_T30_compatible)
> +		goto pll_T30_init;
...
> +	goto pll_init_done;
> +
> +pll_T30_init:
...
> +pll_init_done:

Uggh. Use functions.

> @@ -325,17 +409,37 @@ static int init_utmi_usb_controller(struct fdt_usb *config)
>  	/* Disable ICUSB FS/LS transceiver */
>  	clrbits_le32(&usbctlr->icusb_ctrl, IC_ENB1);
>  
> -	/* Select UTMI parallel interface */
> -	clrsetbits_le32(&usbctlr->port_sc1, PTS_MASK,
> -			PTS_UTMI << PTS_SHIFT);
> -	clrbits_le32(&usbctlr->port_sc1, STS);
> +	if (!is_T30_compatible) {
> +		/* Select UTMI parallel interface */
> +		if (config->periph_id == PERIPH_ID_USBD) {
> +			clrsetbits_le32(&usbctlr->port_sc1, PTS1_MASK,
> +					PTS_UTMI << PTS1_SHIFT);
> +			clrbits_le32(&usbctlr->port_sc1, STS1);
> +		} else {
> +			clrsetbits_le32(&usbctlr->port_sc1, PTS_MASK,
> +					PTS_UTMI << PTS_SHIFT);
> +			clrbits_le32(&usbctlr->port_sc1, STS);
> +		}
> +	}

This changes behaviour on Tegra20. While it may be correct, it has
nothing to do with adding Tegra30/114 support, and hence should be a
separate patch earlier in the series.

> +int board_usb_init(const void *blob)

> +	if (!count)
> +		goto skip_T20_process;
> +	err = process_nodes(blob, node_list, count);
> +	if (err) {
> +		printf("%s: Error processing T20 compatible USB node(s)!\n",
> +		       __func__);
> +		return err;
> +	}
> +skip_T20_process:

Uggh. Instead of:

    if (!count)
        goto skip;
    code;
skip:

do:

if (count) {
    code;
}

> +
> +	count = fdtdec_find_aliases_for_id(blob, "usb",
> +			COMPAT_NVIDIA_TEGRA114_USB, node_list, USB_PORTS_MAX);
> +	if (count)
> +		is_T114_compatible = 1;

Don't use globals for that. Or, if you do, just return as soon as you've
found a match, so it's impossible to end up with some Tegra20 and some
Tegra30 controllers both enabled.