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

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.