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

2013-06-17 Thread Jim Lin
Tegra30 and Tegra114 are compatible except PLL parameters.

Tested on Tegra30 Cardhu, and Tegra114 Dalmore
platforms. All works well.

Signed-off-by: Jim Lin ji...@nvidia.com
---
Changes in v2:
 - Move common definitions into arch-tegra/usb.h and
   chip specific definitions into arch-tegraXX(X)/usb.h
 - In ehci-tegra.c, add PLL parameters for Tegra30 and Tegra114.
 - In ehci-tegra.c, use the port pointed by nvidia,has-legacy-mode
   to know whether we do special handling on Port Reset.
 - Remove some irrelevant whitespace changes.
 - Use if-else, instead of goto in ehci-tegra.c
   init_utmi_usb_controller().
 - Use original coding for PTS_MASK in ehci-tegra.c
   init_utmi_usb_controller().
   Reason is that these bits are read-only on Tegra20.
   Don't need special handling between USB1 and USB3 ports.
 - Use if-else, instead of goto in ehci-tegra.c board_usb_init().
Changes in v3:
 None

 arch/arm/include/asm/arch-tegra/clk_rst.h |   10 +
 arch/arm/include/asm/arch-tegra/usb.h |  182 --
 arch/arm/include/asm/arch-tegra114/usb.h  |  156 +++
 arch/arm/include/asm/arch-tegra20/usb.h   |  155 +++
 arch/arm/include/asm/arch-tegra30/usb.h   |  168 
 board/nvidia/common/board.c   |2 +-
 drivers/usb/host/ehci-tegra.c |  297 +
 7 files changed, 796 insertions(+), 174 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-tegra114/usb.h
 create mode 100644 arch/arm/include/asm/arch-tegra20/usb.h
 create mode 100644 arch/arm/include/asm/arch-tegra30/usb.h

diff --git a/arch/arm/include/asm/arch-tegra/clk_rst.h 
b/arch/arm/include/asm/arch-tegra/clk_rst.h
index c754ec7..9b8de9c 100644
--- a/arch/arm/include/asm/arch-tegra/clk_rst.h
+++ b/arch/arm/include/asm/arch-tegra/clk_rst.h
@@ -225,6 +225,16 @@ enum {
IN_408_OUT_9_6_DIVISOR = 83,
 };
 
+/* CLK_RST_CONTROLLER_UTMIP_PLL_CFG1_0 */
+#define PLLU_POWERDOWN (1  16)
+#define PLL_ENABLE_POWERDOWN   (1  14)
+#define PLL_ACTIVE_POWERDOWN   (1  12)
+
+/* CLK_RST_CONTROLLER_UTMIP_PLL_CFG2_0 */
+#define UTMIP_FORCE_PD_SAMP_C_POWERDOWN(1  4)
+#define UTMIP_FORCE_PD_SAMP_B_POWERDOWN(1  2)
+#define UTMIP_FORCE_PD_SAMP_A_POWERDOWN(1  0)
+
 /* CLK_RST_CONTROLLER_OSC_CTRL_0 */
 #define OSC_XOBP_SHIFT 1
 #define OSC_XOBP_MASK  (1U  OSC_XOBP_SHIFT)
diff --git a/arch/arm/include/asm/arch-tegra/usb.h 
b/arch/arm/include/asm/arch-tegra/usb.h
index ef6c089..cefe0d2 100644
--- a/arch/arm/include/asm/arch-tegra/usb.h
+++ b/arch/arm/include/asm/arch-tegra/usb.h
@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2011 The Chromium OS Authors.
+ * Copyright (c) 2013 NVIDIA Corporation
  * See file CREDITS for list of people who contributed to this
  * project.
  *
@@ -22,120 +23,6 @@
 #ifndef _TEGRA_USB_H_
 #define _TEGRA_USB_H_
 
-
-/* USB Controller (USBx_CONTROLLER_) regs */
-struct usb_ctlr {
-   /* 0x000 */
-   uint id;
-   uint reserved0;
-   uint host;
-   uint device;
-
-   /* 0x010 */
-   uint txbuf;
-   uint rxbuf;
-   uint reserved1[2];
-
-   /* 0x020 */
-   uint reserved2[56];
-
-   /* 0x100 */
-   u16 cap_length;
-   u16 hci_version;
-   uint hcs_params;
-   uint hcc_params;
-   uint reserved3[5];
-
-   /* 0x120 */
-   uint dci_version;
-   uint dcc_params;
-   uint reserved4[6];
-
-   /* 0x140 */
-   uint usb_cmd;
-   uint usb_sts;
-   uint usb_intr;
-   uint frindex;
-
-   /* 0x150 */
-   uint reserved5;
-   uint periodic_list_base;
-   uint async_list_addr;
-   uint async_tt_sts;
-
-   /* 0x160 */
-   uint burst_size;
-   uint tx_fill_tuning;
-   uint reserved6;   /* is this port_sc1 on some controllers? */
-   uint icusb_ctrl;
-
-   /* 0x170 */
-   uint ulpi_viewport;
-   uint reserved7;
-   uint endpt_nak;
-   uint endpt_nak_enable;
-
-   /* 0x180 */
-   uint reserved;
-   uint port_sc1;
-   uint reserved8[6];
-
-   /* 0x1a0 */
-   uint reserved9;
-   uint otgsc;
-   uint usb_mode;
-   uint endpt_setup_stat;
-
-   /* 0x1b0 */
-   uint reserved10[20];
-
-   /* 0x200 */
-   uint reserved11[0x80];
-
-   /* 0x400 */
-   uint susp_ctrl;
-   uint phy_vbus_sensors;
-   uint phy_vbus_wakeup_id;
-   uint phy_alt_vbus_sys;
-
-   /* 0x410 */
-   uint usb1_legacy_ctrl;
-   uint reserved12[4];
-
-   /* 0x424 */
-   uint ulpi_timing_ctrl_0;
-   uint ulpi_timing_ctrl_1;
-   uint reserved13[53];
-
-   /* 0x500 */
-   uint reserved14[64 * 3];
-
-   /* 0x800 */
-   uint utmip_pll_cfg0;
-   uint utmip_pll_cfg1;
-   uint utmip_xcvr_cfg0;
-   uint utmip_bias_cfg0;
-
-   /* 0x810 */
-   uint utmip_hsrx_cfg0;
-   uint utmip_hsrx_cfg1;
-   uint utmip_fslsrx_cfg0;
-   uint utmip_fslsrx_cfg1;
-
-   /* 

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

2013-06-17 Thread Thierry Reding
On Mon, Jun 17, 2013 at 05:09:56PM +0800, Jim Lin wrote:
[...]
 diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
 index 8d7a227..f0f81c9 100644
 --- a/board/nvidia/common/board.c
 +++ b/board/nvidia/common/board.c
 @@ -46,7 +46,7 @@
  #include asm/arch/emc.h
  #endif
  #ifdef CONFIG_USB_EHCI_TEGRA
 -#include asm/arch-tegra/usb.h
 +#include asm/arch/usb.h
  #endif
  #ifdef CONFIG_TEGRA_MMC
  #include asm/arch-tegra/tegra_mmc.h

With this hunk applied I get the following new build warning:

../../nvidia/common/board.c: In function 'board_init':
../../nvidia/common/board.c:171:2: warning: implicit declaration of 
function 'board_usb_init' [-Wimplicit-function-declaration]
  board_usb_init(gd-fdt_blob);
  ^

Reverting that one hunk makes the warning go away again and everything
still builds fine, so I think it can just be removed from the patch.

Besides the one issue I'm still seeing with the very old flash drive,
which might turn out not to be specific to Tegra, this series:

Tested-by: Thierry Reding thierry.red...@gmail.com


pgpEVEDZe7r2R.pgp
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2013-06-17 Thread 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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot