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

[Stephen Warren]

On 06/17/2013 03:09 AM, Jim Lin wrote:
> Tegra30 and Tegra114 are compatible except PLL parameters.
> 
> Tested on Tegra30 Cardhu, and Tegra114 Dalmore
> platforms. All works well.

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

> +static u32 port_clear_csc; /* Port that needs to clear CSC after Port Reset */

What "units" is that variable in? The variable name should be more like
"status_reg_addr_needing_clear_csc".

> +static unsigned is_T30_compatible;
> +static unsigned is_T114_compatible;

Given that there is code in this patch that does:

+	if (is_T30_compatible) {
+		if (is_T114_compatible)

I think those should be is_T30_or_later and is_T114_or_later.

But, testing against SoC version is the wrong way to go. Instead, the
code should set a bunch of feature flags based on the SoC it's running
on during initialization, and then test those feature flags throughout
the code. That way, if Tegra30 and (hypothetical future chips) Tegra200
and Tegra300 need a fix, but Tegra114 doesn't, you don't have to write
tortuous if statements throughout the code, but simply have a lookup
table that maps from SoC ID to the set of feature/fix flags that it needs.

See for example Linux kernel driver drivers/gpio/gpio-tegra.c's
tegra20_gpio_config, tegra30_gpio_config, and tegra_gpio_of_match[], or
drivers/dma/tegra20-apb-dma.c's tegra_dma_of_match[].

> +static const unsigned *get_pll_timing(void)
> +{
> +	const unsigned *timing;
> +
> +	if (is_T30_compatible) {
> +		if (is_T114_compatible)
> +			timing = T114_usb_pll[clock_get_osc_freq()];
> +		else
> +			timing = T30_usb_pll[clock_get_osc_freq()];
> +	} else {
> +		timing = T20_usb_pll[clock_get_osc_freq()];
> +	}
> +
> +	return timing;
> +}

Following on from the feature flag discussion above, it'd be better to
simply include a pointer in the per-SoC configuration table that pointed
at the PLL table. That way, this function could be removed, and replaced
with a simple read through a pointer.

> +int board_usb_init(const void *blob)
> +{
> +	int node_list[USB_PORTS_MAX];
> +	int count, err = 0;
> +
> +	is_T30_compatible = 0;
> +	is_T114_compatible = 0;
> +
> +	/* count may return <0 on error */
> +	count = fdtdec_find_aliases_for_id(blob, "usb",
> +			COMPAT_NVIDIA_TEGRA20_USB, node_list, USB_PORTS_MAX);
> +	if (count) {
> +		err = process_usb_nodes(blob, node_list, count);
> +		if (err)
> +			printf("%s: Error processing T20 USB node!\n",
> +			       __func__);
> +		return err;
> +	}
> +	count = fdtdec_find_aliases_for_id(blob, "usb",
> +			COMPAT_NVIDIA_TEGRA114_USB, node_list, USB_PORTS_MAX);
> +	if (count)
> +		is_T114_compatible = 1;
> +	else
> +		count = fdtdec_find_aliases_for_id(blob, "usb",
> +			COMPAT_NVIDIA_TEGRA30_USB, node_list, USB_PORTS_MAX);
> +	if (count) {
> +		/* T114 is also mostly compatible to T30 */
> +		is_T30_compatible = 1;
> +		err = process_usb_nodes(blob, node_list, count);
> +		if (err) {
> +			if (is_T114_compatible)
> +				printf("%s: Error processing T114 USB node!\n",
> +				       __func__);
> +			else
> +				printf("%s: Error processing T30 USB node!\n",
> +				       __func__);

(Following on from the comment below: Why not just say "USB node" rather
than "T30 USB node" or "T114 USB node" here. That way, you completely
avoid having to write an if statement.

> +		}
> +	}
> +
> +	return err;
> +}

This function is pretty convoluted. It'd be far better to enhance
fdtdec_find_aliases_for_id() to accept a list of compatible values, and
simply return all matching instances in one go.
fdtdec_find_aliases_for_id() should also return the "type" of each
match, so you know if each one is a Tegra20/30/114 port.