[U-Boot] [PATCH 1/8] tegra: usb: convert USB_PORTS_MAX to be a define

[U-Boot] [PATCH 1/8] tegra: usb: convert USB_PORTS_MAX to be a define

[Lucas Stach]

No point in having this as an enum. Also while at it set it to the real hardware
maximum for both Tegra 2 and Tegra 3. If new Tegra hardware includes more
USB controllers we can always bump the limit then.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 arch/arm/cpu/armv7/tegra20/usb.c | 4 +---
 1 Datei ge?ndert, 1 Zeile hinzugef?gt(+), 3 Zeilen entfernt(-)

diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
index 1bccf2b..9fd1edc 100644
--- a/arch/arm/cpu/armv7/tegra20/usb.c
+++ b/arch/arm/cpu/armv7/tegra20/usb.c
@@ -43,9 +43,7 @@
 	#endif
 #endif
 
-enum {
-	USB_PORTS_MAX	= 4,			/* Maximum ports we allow */
-};
+#define USB_PORTS_MAX	3		/* Maximum ports we allow */
 
 /* Parameters we need for USB */
 enum {
-- 
1.7.11.7

[U-Boot] [PATCH 2/8] tegra: usb: make controller init functions more self contained

[Lucas Stach]

There is no need to pass around all those parameters. The init functions
are able to easily extract all the needed setup info on their own.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 arch/arm/cpu/armv7/tegra20/usb.c | 24 ++++++++++++------------
 1 Datei ge?ndert, 12 Zeilen hinzugef?gt(+), 12 Zeilen entfernt(-)

diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
index 9fd1edc..1725cd1 100644
--- a/arch/arm/cpu/armv7/tegra20/usb.c
+++ b/arch/arm/cpu/armv7/tegra20/usb.c
@@ -196,11 +196,12 @@ void usbf_reset_controller(struct fdt_usb *config, struct usb_ctlr *usbctlr)
 }
 
 /* set up the UTMI USB controller with the parameters provided */
-static int init_utmi_usb_controller(struct fdt_usb *config,
-				struct usb_ctlr *usbctlr, const u32 timing[])
+static int init_utmi_usb_controller(struct fdt_usb *config)
 {
 	u32 val;
 	int loop_count;
+	u32 *timing;
+	struct usb_ctlr *usbctlr = config->reg;
 
 	clock_enable(config->periph_id);
 
@@ -227,6 +228,8 @@ static int init_utmi_usb_controller(struct fdt_usb *config,
 	 * PLL Delay CONFIGURATION settings. The following parameters control
 	 * the bring up of the plls.
 	 */
+	timing = usb_pll[clock_get_osc_freq()];
+
 	val = readl(&usbctlr->utmip_misc_cfg1);
 	clrsetbits_le32(&val, UTMIP_PLLU_STABLE_COUNT_MASK,
 		timing[PARAM_STABLE_COUNT] << UTMIP_PLLU_STABLE_COUNT_SHIFT);
@@ -329,12 +332,12 @@ static int init_utmi_usb_controller(struct fdt_usb *config,
 #endif
 
 /* set up the ULPI USB controller with the parameters provided */
-static int init_ulpi_usb_controller(struct fdt_usb *config,
-				struct usb_ctlr *usbctlr)
+static int init_ulpi_usb_controller(struct fdt_usb *config)
 {
 	u32 val;
 	int loop_count;
 	struct ulpi_viewport ulpi_vp;
+	struct usb_ctlr *usbctlr = config->reg;
 
 	/* set up ULPI reference clock on pllp_out4 */
 	clock_enable(PERIPH_ID_DEV2_OUT);
@@ -406,8 +409,7 @@ static int init_ulpi_usb_controller(struct fdt_usb *config,
 	return 0;
 }
 #else
-static int init_ulpi_usb_controller(struct fdt_usb *config,
-				struct usb_ctlr *usbctlr)
+static int init_ulpi_usb_controller(struct fdt_usb *config)
 {
 	printf("No code to set up ULPI controller, please enable"
 			"CONFIG_USB_ULPI and CONFIG_USB_ULPI_VIEWPORT");
@@ -428,22 +430,20 @@ static void config_clock(const u32 timing[])
  * @param config	USB port configuration
  * @return 0 if ok, -1 if error (too many ports)
  */
-static int add_port(struct fdt_usb *config, const u32 timing[])
+static int add_port(struct fdt_usb *config)
 {
-	struct usb_ctlr *usbctlr = config->reg;
-
 	if (port_count == USB_PORTS_MAX) {
 		printf("tegrausb: Cannot register more than %d ports\n",
 		      USB_PORTS_MAX);
 		return -1;
 	}
 
-	if (config->utmi && init_utmi_usb_controller(config, usbctlr, timing)) {
+	if (config->utmi && init_utmi_usb_controller(config)) {
 		printf("tegrausb: Cannot init port\n");
 		return -1;
 	}
 
-	if (config->ulpi && init_ulpi_usb_controller(config, usbctlr)) {
+	if (config->ulpi && init_ulpi_usb_controller(config)) {
 		printf("tegrausb: Cannot init port\n");
 		return -1;
 	}
@@ -556,7 +556,7 @@ int board_usb_init(const void *blob)
 			return -1;
 		}
 
-		if (add_port(&config, usb_pll[freq]))
+		if (add_port(&config))
 			return -1;
 		set_host_mode(&config);
 	}
-- 
1.7.11.7

[U-Boot] [PATCH 3/8] tegra: usb: fold initial pll setup into board_usb_init

[Lucas Stach]

The setup is trivial, no need to split this out into a separate function.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 arch/arm/cpu/armv7/tegra20/usb.c | 15 +++++----------
 1 Datei ge?ndert, 5 Zeilen hinzugef?gt(+), 10 Zeilen entfernt(-)

diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
index 1725cd1..e61bd69 100644
--- a/arch/arm/cpu/armv7/tegra20/usb.c
+++ b/arch/arm/cpu/armv7/tegra20/usb.c
@@ -417,13 +417,6 @@ static int init_ulpi_usb_controller(struct fdt_usb *config)
 }
 #endif
 
-static void config_clock(const u32 timing[])
-{
-	clock_start_pll(CLOCK_ID_USB,
-		timing[PARAM_DIVM], timing[PARAM_DIVN], timing[PARAM_DIVP],
-		timing[PARAM_CPCON], timing[PARAM_LFCON]);
-}
-
 /**
  * Add a new USB port to the list of available ports.
  *
@@ -534,13 +527,15 @@ int board_usb_init(const void *blob)
 {
 	struct fdt_usb config;
 	unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
-	enum clock_osc_freq freq;
 	int node_list[USB_PORTS_MAX];
 	int node, count, i;
+	u32 *timing;
 
 	/* Set up the USB clocks correctly based on our oscillator frequency */
-	freq = clock_get_osc_freq();
-	config_clock(usb_pll[freq]);
+	timing = usb_pll[clock_get_osc_freq()];
+	clock_start_pll(CLOCK_ID_USB,
+		timing[PARAM_DIVM], timing[PARAM_DIVN], timing[PARAM_DIVP],
+		timing[PARAM_CPCON], timing[PARAM_LFCON]);
 
 	/* count may return <0 on error */
 	count = fdtdec_find_aliases_for_id(blob, "usb",
-- 
1.7.11.7

[U-Boot] [PATCH 4/8] tegra: usb: remove unneeded function parameter

[Lucas Stach]

Just a dead parameter, never actually used.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 arch/arm/cpu/armv7/tegra20/usb.c | 6 ++----
 1 Datei ge?ndert, 2 Zeilen hinzugef?gt(+), 4 Zeilen entfernt(-)

diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
index e61bd69..cf800b1 100644
--- a/arch/arm/cpu/armv7/tegra20/usb.c
+++ b/arch/arm/cpu/armv7/tegra20/usb.c
@@ -477,8 +477,7 @@ int tegrausb_stop_port(int portnum)
 	return 0;
 }
 
-int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
-		   struct fdt_usb *config)
+int fdt_decode_usb(const void *blob, int node, struct fdt_usb *config)
 {
 	const char *phy, *mode;
 
@@ -526,7 +525,6 @@ int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
 int board_usb_init(const void *blob)
 {
 	struct fdt_usb config;
-	unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
 	int node_list[USB_PORTS_MAX];
 	int node, count, i;
 	u32 *timing;
@@ -545,7 +543,7 @@ int board_usb_init(const void *blob)
 		node = node_list[i];
 		if (!node)
 			continue;
-		if (fdt_decode_usb(blob, node, osc_freq, &config)) {
+		if (fdt_decode_usb(blob, node, &config)) {
 			debug("Cannot decode USB node %s\n",
 			      fdt_get_name(blob, node, NULL));
 			return -1;
-- 
1.7.11.7

[U-Boot] [PATCH 5/8] tegra: usb: move controller init into start_port

[Lucas Stach]

There is no need to init a USB controller before the upper layers indicate
that they are actually going to use it.

board_usb_init now only parses the device tree and sets up the common pll.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 arch/arm/cpu/armv7/tegra20/usb.c | 47 +++++++++++++++-------------------------
 1 Datei ge?ndert, 18 Zeilen hinzugef?gt(+), 29 Zeilen entfernt(-)

diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
index cf800b1..e372b8b 100644
--- a/arch/arm/cpu/armv7/tegra20/usb.c
+++ b/arch/arm/cpu/armv7/tegra20/usb.c
@@ -417,44 +417,29 @@ static int init_ulpi_usb_controller(struct fdt_usb *config)
 }
 #endif
 
-/**
- * Add a new USB port to the list of available ports.
- *
- * @param config	USB port configuration
- * @return 0 if ok, -1 if error (too many ports)
- */
-static int add_port(struct fdt_usb *config)
+int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor)
 {
-	if (port_count == USB_PORTS_MAX) {
-		printf("tegrausb: Cannot register more than %d ports\n",
-		      USB_PORTS_MAX);
+	struct fdt_usb *config;
+	struct usb_ctlr *usbctlr;
+
+	if (portnum >= port_count)
 		return -1;
-	}
+
+	config = &port[portnum];
 
 	if (config->utmi && init_utmi_usb_controller(config)) {
-		printf("tegrausb: Cannot init port\n");
+		printf("tegrausb: Cannot init port %d\n", portnum);
 		return -1;
 	}
 
 	if (config->ulpi && init_ulpi_usb_controller(config)) {
-		printf("tegrausb: Cannot init port\n");
+		printf("tegrausb: Cannot init port %d\n", portnum);
 		return -1;
 	}
 
-	port[port_count++] = *config;
-
-	return 0;
-}
+	set_host_mode(config);
 
-int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor)
-{
-	struct usb_ctlr *usbctlr;
-
-	if (portnum >= port_count)
-		return -1;
-	set_host_mode(&port[portnum]);
-
-	usbctlr = port[portnum].reg;
+	usbctlr = config->reg;
 	*hccr = (u32)&usbctlr->cap_length;
 	*hcor = (u32)&usbctlr->usb_cmd;
 	return 0;
@@ -539,6 +524,12 @@ int board_usb_init(const void *blob)
 	count = fdtdec_find_aliases_for_id(blob, "usb",
 			COMPAT_NVIDIA_TEGRA20_USB, node_list, USB_PORTS_MAX);
 	for (i = 0; i < count; i++) {
+		if (port_count == USB_PORTS_MAX) {
+			printf("tegrausb: Cannot register more than %d ports\n",
+				USB_PORTS_MAX);
+			return -1;
+		}
+
 		debug("USB %d: ", i);
 		node = node_list[i];
 		if (!node)
@@ -549,9 +540,7 @@ int board_usb_init(const void *blob)
 			return -1;
 		}
 
-		if (add_port(&config))
-			return -1;
-		set_host_mode(&config);
+		port[port_count++] = config;
 	}
 
 	return 0;
-- 
1.7.11.7

[U-Boot] [PATCH 6/8] tegra: usb: various small cleanups

[Lucas Stach]

Remove unneeded headers, function prototype and stale comment.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 arch/arm/cpu/armv7/tegra20/usb.c        | 13 +------------
 arch/arm/include/asm/arch-tegra20/usb.h |  3 ---
 2 Dateien ge?ndert, 1 Zeile hinzugef?gt(+), 15 Zeilen entfernt(-)

diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
index e372b8b..2cc95d2 100644
--- a/arch/arm/cpu/armv7/tegra20/usb.c
+++ b/arch/arm/cpu/armv7/tegra20/usb.c
@@ -25,21 +25,15 @@
 #include <asm/io.h>
 #include <asm-generic/gpio.h>
 #include <asm/arch/clock.h>
-#include <asm/arch/gpio.h>
-#include <asm/arch/pinmux.h>
-#include <asm/arch/tegra.h>
 #include <asm/arch/usb.h>
 #include <usb/ulpi.h>
-#include <asm/arch-tegra/clk_rst.h>
-#include <asm/arch-tegra/sys_proto.h>
-#include <asm/arch-tegra/uart.h>
 #include <libfdt.h>
 #include <fdtdec.h>
 
 #ifdef CONFIG_USB_ULPI
 	#ifndef CONFIG_USB_ULPI_VIEWPORT
 	#error	"To use CONFIG_USB_ULPI on Tegra Boards you have to also \
-			define CONFIG_USB_ULPI_VIEWPORT"
+		define CONFIG_USB_ULPI_VIEWPORT"
 	#endif
 #endif
 
@@ -188,11 +182,6 @@ void usbf_reset_controller(struct fdt_usb *config, struct usb_ctlr *usbctlr)
 	/* Enable the UTMIP PHY */
 	if (config->utmi)
 		setbits_le32(&usbctlr->susp_ctrl, UTMIP_PHY_ENB);
-
-	/*
-	 * TODO: where do we take the USB1 out of reset? The old code would
-	 * take USB3 out of reset, but not USB1. This code doesn't do either.
-	 */
 }
 
 /* set up the UTMI USB controller with the parameters provided */
diff --git a/arch/arm/include/asm/arch-tegra20/usb.h b/arch/arm/include/asm/arch-tegra20/usb.h
index fdbd127..b18c850 100644
--- a/arch/arm/include/asm/arch-tegra20/usb.h
+++ b/arch/arm/include/asm/arch-tegra20/usb.h
@@ -243,9 +243,6 @@ struct usb_ctlr {
 #define VBUS_VLD_STS			(1 << 26)
 
 
-/* Change the USB host port into host mode */
-void usb_set_host_mode(void);
-
 /* Setup USB on the board */
 int board_usb_init(const void *blob);
 
-- 
1.7.11.7

[U-Boot] [PATCH 7/8] tegra: usb: move implementation into right directory

[Lucas Stach]

This moves the Tegra USB implementation into the drivers/usb/host
directory.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 arch/arm/cpu/armv7/tegra20/Makefile                |  2 -
 .../tegra20/usb.c => drivers/usb/host/ehci-tegra.c | 60 ++++++++++++++++++++--
 2 Dateien ge?ndert, 55 Zeilen hinzugef?gt(+), 7 Zeilen entfernt(-)
 rename arch/arm/cpu/armv7/tegra20/usb.c => drivers/usb/host/ehci-tegra.c (92%)

diff --git a/arch/arm/cpu/armv7/tegra20/Makefile b/arch/arm/cpu/armv7/tegra20/Makefile
index 09a0314..2c4d5c9 100644
--- a/arch/arm/cpu/armv7/tegra20/Makefile
+++ b/arch/arm/cpu/armv7/tegra20/Makefile
@@ -27,8 +27,6 @@ include $(TOPDIR)/config.mk
 
 LIB	=  $(obj)lib$(SOC).o
 
-COBJS-$(CONFIG_USB_EHCI_TEGRA) += usb.o
-
 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS))
diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/drivers/usb/host/ehci-tegra.c
similarity index 92%
rename from arch/arm/cpu/armv7/tegra20/usb.c
rename to drivers/usb/host/ehci-tegra.c
index 2cc95d2..3df43a9 100644
--- a/arch/arm/cpu/armv7/tegra20/usb.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -1,6 +1,7 @@
 /*
+ * Copyright (c) 2009-2012 NVIDIA Corporation
  * Copyright (c) 2011 The Chromium OS Authors.
- * (C) Copyright 2010,2011 NVIDIA Corporation <www.nvidia.com>
+ * Copyright (c) 2012 Lucas Stach
  *
  * See file CREDITS for list of people who contributed to this
  * project.
@@ -21,14 +22,16 @@
  * MA 02111-1307 USA
  */
 
-#include <common.h>
 #include <asm/io.h>
-#include <asm-generic/gpio.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/usb.h>
-#include <usb/ulpi.h>
-#include <libfdt.h>
+#include <asm-generic/gpio.h>
+#include <common.h>
 #include <fdtdec.h>
+#include <libfdt.h>
+#include <usb.h>
+#include <usb/ulpi.h>
+#include "ehci.h"
 
 #ifdef CONFIG_USB_ULPI
 	#ifndef CONFIG_USB_ULPI_VIEWPORT
@@ -138,6 +141,23 @@ static const u8 utmip_elastic_limit = 16;
 /* UTMIP High Speed Sync Start Delay */
 static const u8 utmip_hs_sync_start_delay = 9;
 
+/*
+ * A known hardware issue where Connect Status Change bit of PORTSC register
+ * of USB1 controller will be set after Port Reset.
+ * We have to clear it in order for later device enumeration to proceed.
+ * This ehci_powerup_fixup overrides the weak function ehci_powerup_fixup
+ * in "ehci-hcd.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)
+		return;
+	/* For EHCI_PS_CSC to be cleared in ehci_hcd.c */
+	if (ehci_readl(status_reg) & EHCI_PS_CSC)
+		*reg |= EHCI_PS_CSC;
+}
+
 /* Put the port into host mode */
 static void set_host_mode(struct fdt_usb *config)
 {
@@ -534,3 +554,33 @@ int board_usb_init(const void *blob)
 
 	return 0;
 }
+
+/*
+ * Create the appropriate control structures to manage
+ * a new EHCI host controller.
+ */
+int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor)
+{
+	u32 our_hccr, our_hcor;
+
+	/*
+	 * Select the first port, as we don't have a way of selecting others
+	 * yet
+	 */
+	if (tegrausb_start_port(index, &our_hccr, &our_hcor))
+		return -1;
+
+	*hccr = (struct ehci_hccr *)our_hccr;
+	*hcor = (struct ehci_hcor *)our_hcor;
+
+	return 0;
+}
+
+/*
+ * Destroy the appropriate control structures corresponding
+ * the the EHCI host controller.
+ */
+int ehci_hcd_stop(int index)
+{
+	return tegrausb_stop_port(index);
+}
-- 
1.7.11.7

[U-Boot] [PATCH 8/8] tegra: usb: move [start|stop]_port into ehci_hcd_[init|stop]

[Lucas Stach]

The ehci_hcd entry points were just calling into the Tegra USB functions. Now
that they are in the same file we can just move over the implementation.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 arch/arm/include/asm/arch-tegra20/usb.h | 19 -------
 drivers/usb/host/ehci-tegra.c           | 93 +++++++++++++--------------------
 2 Dateien ge?ndert, 35 Zeilen hinzugef?gt(+), 77 Zeilen entfernt(-)

diff --git a/arch/arm/include/asm/arch-tegra20/usb.h b/arch/arm/include/asm/arch-tegra20/usb.h
index b18c850..ef6c089 100644
--- a/arch/arm/include/asm/arch-tegra20/usb.h
+++ b/arch/arm/include/asm/arch-tegra20/usb.h
@@ -246,23 +246,4 @@ struct usb_ctlr {
 /* Setup USB on the board */
 int board_usb_init(const void *blob);
 
-/**
- * Start up the given port number (ports are numbered from 0 on each board).
- * This returns values for the appropriate hccr and hcor addresses to use for
- * USB EHCI operations.
- *
- * @param portnum	port number to start
- * @param hccr		returns start address of EHCI HCCR registers
- * @param hcor		returns start address of EHCI HCOR registers
- * @return 0 if ok, -1 on error (generally invalid port number)
- */
-int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor);
-
-/**
- * Stop the current port
- *
- * @return 0 if ok, -1 if no port was active
- */
-int tegrausb_stop_port(int portnum);
-
 #endif	/* _TEGRA_USB_H_ */
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 3df43a9..5966e2d 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -426,51 +426,6 @@ static int init_ulpi_usb_controller(struct fdt_usb *config)
 }
 #endif
 
-int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor)
-{
-	struct fdt_usb *config;
-	struct usb_ctlr *usbctlr;
-
-	if (portnum >= port_count)
-		return -1;
-
-	config = &port[portnum];
-
-	if (config->utmi && init_utmi_usb_controller(config)) {
-		printf("tegrausb: Cannot init port %d\n", portnum);
-		return -1;
-	}
-
-	if (config->ulpi && init_ulpi_usb_controller(config)) {
-		printf("tegrausb: Cannot init port %d\n", portnum);
-		return -1;
-	}
-
-	set_host_mode(config);
-
-	usbctlr = config->reg;
-	*hccr = (u32)&usbctlr->cap_length;
-	*hcor = (u32)&usbctlr->usb_cmd;
-	return 0;
-}
-
-int tegrausb_stop_port(int portnum)
-{
-	struct usb_ctlr *usbctlr;
-
-	usbctlr = port[portnum].reg;
-
-	/* Stop controller */
-	writel(0, &usbctlr->usb_cmd);
-	udelay(1000);
-
-	/* Initiate controller reset */
-	writel(2, &usbctlr->usb_cmd);
-	udelay(1000);
-
-	return 0;
-}
-
 int fdt_decode_usb(const void *blob, int node, struct fdt_usb *config)
 {
 	const char *phy, *mode;
@@ -556,31 +511,53 @@ int board_usb_init(const void *blob)
 }
 
 /*
- * Create the appropriate control structures to manage
- * a new EHCI host controller.
+ * Initialize the USB controller and return the control structures.
  */
 int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor)
 {
-	u32 our_hccr, our_hcor;
+	struct fdt_usb *config;
+	struct usb_ctlr *usbctlr;
 
-	/*
-	 * Select the first port, as we don't have a way of selecting others
-	 * yet
-	 */
-	if (tegrausb_start_port(index, &our_hccr, &our_hcor))
+	if (index >= port_count)
+		return -1;
+
+	config = &port[index];
+
+	if (config->utmi && init_utmi_usb_controller(config)) {
+		printf("tegrausb: Cannot init port %d\n", index);
+		return -1;
+	}
+
+	if (config->ulpi && init_ulpi_usb_controller(config)) {
+		printf("tegrausb: Cannot init port %d\n", index);
 		return -1;
+	}
+
+	set_host_mode(config);
 
-	*hccr = (struct ehci_hccr *)our_hccr;
-	*hcor = (struct ehci_hcor *)our_hcor;
+	usbctlr = config->reg;
+	*hccr = (struct ehci_hccr *)&usbctlr->cap_length;
+	*hcor = (struct ehci_hcor *)&usbctlr->usb_cmd;
 
 	return 0;
 }
 
 /*
- * Destroy the appropriate control structures corresponding
- * the the EHCI host controller.
+ * Bring down the USB controller.
  */
 int ehci_hcd_stop(int index)
 {
-	return tegrausb_stop_port(index);
+	struct usb_ctlr *usbctlr;
+
+	usbctlr = port[index].reg;
+
+	/* Stop controller */
+	writel(0, &usbctlr->usb_cmd);
+	udelay(1000);
+
+	/* Initiate controller reset */
+	writel(2, &usbctlr->usb_cmd);
+	udelay(1000);
+
+	return 0;
 }
-- 
1.7.11.7

[U-Boot] [PATCH 5/8] tegra: usb: move controller init into start_port

[Marek Vasut]

Dear Lucas Stach,

> There is no need to init a USB controller before the upper layers indicate
> that they are actually going to use it.
> 
> board_usb_init now only parses the device tree and sets up the common pll.
> 
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
>  arch/arm/cpu/armv7/tegra20/usb.c | 47
> +++++++++++++++------------------------- 1 Datei ge?ndert, 18 Zeilen
> hinzugef?gt(+), 29 Zeilen entfernt(-)
> 
> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c
> b/arch/arm/cpu/armv7/tegra20/usb.c index cf800b1..e372b8b 100644
> --- a/arch/arm/cpu/armv7/tegra20/usb.c
> +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> @@ -417,44 +417,29 @@ static int init_ulpi_usb_controller(struct fdt_usb
> *config) }
>  #endif
> 
> -/**
> - * Add a new USB port to the list of available ports.
> - *
> - * @param config	USB port configuration
> - * @return 0 if ok, -1 if error (too many ports)
> - */
> -static int add_port(struct fdt_usb *config)

Fix the comment instead of removing it?

[...]

Best regards,
Marek Vasut

[U-Boot] [PATCH 5/8] tegra: usb: move controller init into start_port

[Lucas Stach]

Hello Marek,

Am Dienstag, den 30.10.2012, 11:59 +0100 schrieb Marek Vasut:
> Dear Lucas Stach,
> 
> > There is no need to init a USB controller before the upper layers indicate
> > that they are actually going to use it.
> > 
> > board_usb_init now only parses the device tree and sets up the common pll.
> > 
> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> > ---
> >  arch/arm/cpu/armv7/tegra20/usb.c | 47
> > +++++++++++++++------------------------- 1 Datei ge?ndert, 18 Zeilen
> > hinzugef?gt(+), 29 Zeilen entfernt(-)
> > 
> > diff --git a/arch/arm/cpu/armv7/tegra20/usb.c
> > b/arch/arm/cpu/armv7/tegra20/usb.c index cf800b1..e372b8b 100644
> > --- a/arch/arm/cpu/armv7/tegra20/usb.c
> > +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> > @@ -417,44 +417,29 @@ static int init_ulpi_usb_controller(struct fdt_usb
> > *config) }
> >  #endif
> > 
> > -/**
> > - * Add a new USB port to the list of available ports.
> > - *
> > - * @param config	USB port configuration
> > - * @return 0 if ok, -1 if error (too many ports)
> > - */
> > -static int add_port(struct fdt_usb *config)
> 
> Fix the comment instead of removing it?
> 
I don't think that this comment adds any real value. The whole function
which this comment refers to is removed and it's content split between
board_usb_init and ehci_hcd_init, which are self explanatory.

Regards,
Lucas

[U-Boot] [PATCH 5/8] tegra: usb: move controller init into start_port

[Marek Vasut]

Dear Lucas Stach,

[...]

> > > -static int add_port(struct fdt_usb *config)
> > 
> > Fix the comment instead of removing it?
> 
> I don't think that this comment adds any real value. The whole function
> which this comment refers to is removed and it's content split between
> board_usb_init and ehci_hcd_init, which are self explanatory.

Then add a proper comment please. Call me a docu-nazi, but I'd really love u-
boot nicely and properly documented, please.

[1] http://www.denx.de/wiki/U-Boot/CodingStyle

Best regards,
Marek Vasut

[U-Boot] [PATCH 5/8] tegra: usb: move controller init into start_port

[Lucas Stach]

Hi Marek,

Am Dienstag, den 30.10.2012, 13:33 +0100 schrieb Marek Vasut:
> Dear Lucas Stach,
> 
> [...]
> 
> > > > -static int add_port(struct fdt_usb *config)
> > > 
> > > Fix the comment instead of removing it?
> > 
> > I don't think that this comment adds any real value. The whole function
> > which this comment refers to is removed and it's content split between
> > board_usb_init and ehci_hcd_init, which are self explanatory.
> 
> Then add a proper comment please. Call me a docu-nazi, but I'd really love u-
> boot nicely and properly documented, please.
> 
I'm all in favour of adding proper documentation, but I'm opposed to add
it in the middle of this cleanup/movement series.

I'll send a patch on top of this series to add doc, so it doesn't
interfere with the review of this series.

Regards,
Lucas

[U-Boot] [PATCH 2/8] tegra: usb: make controller init functions more self contained

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> There is no need to pass around all those parameters. The init functions
> are able to easily extract all the needed setup info on their own.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
>  arch/arm/cpu/armv7/tegra20/usb.c | 24 ++++++++++++------------
>  1 Datei ge?ndert, 12 Zeilen hinzugef?gt(+), 12 Zeilen entfernt(-)

I'm not sure I agree with the premise of this patch.

At the top level it calls clock_get_osc_freq() to get the frequency.
That is then passed to the two places that need it.

It doesn't seem right to me to call clock_get_osc_freq() again in the
lower level function just to avoid a parameter. On ARM at least a few
parameters are a cheap way of passing data around.

It also allows the lower-level functions to deal with what they need
to, rather than all functions having to reference the global state
independently, each one digging down to what it actually needs.

Regards,
Simon

[U-Boot] [PATCH 1/8] tegra: usb: convert USB_PORTS_MAX to be a define

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> No point in having this as an enum. Also while at it set it to the real hardware
> maximum for both Tegra 2 and Tegra 3. If new Tegra hardware includes more
> USB controllers we can always bump the limit then.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
>  arch/arm/cpu/armv7/tegra20/usb.c | 4 +---
>  1 Datei ge?ndert, 1 Zeile hinzugef?gt(+), 3 Zeilen entfernt(-)
>
> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
> index 1bccf2b..9fd1edc 100644
> --- a/arch/arm/cpu/armv7/tegra20/usb.c
> +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> @@ -43,9 +43,7 @@
>         #endif
>  #endif
>
> -enum {
> -       USB_PORTS_MAX   = 4,                    /* Maximum ports we allow */
> -};
> +#define USB_PORTS_MAX  3               /* Maximum ports we allow */

That's fine with me if you wan to change it.

I tend to use enums most of the time. It shows up as a symbol in the
debugger, avoids bracketed expressions, side-effects and the like, and
works well when numbering multiple things (automatic increment). It's
also a welcome language feature IMO - use it or lose it :-) But in
this case the benefit is small.

>
>  /* Parameters we need for USB */
>  enum {
> --
> 1.7.11.7
>

Regards,
Simon

[U-Boot] [PATCH 1/8] tegra: usb: convert USB_PORTS_MAX to be a define

[Marek Vasut]

Dear Simon Glass,

> Hi Lucas,
> 
> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> > No point in having this as an enum. Also while at it set it to the real
> > hardware maximum for both Tegra 2 and Tegra 3. If new Tegra hardware
> > includes more USB controllers we can always bump the limit then.
> > 
> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> > ---
> > 
> >  arch/arm/cpu/armv7/tegra20/usb.c | 4 +---
> >  1 Datei ge?ndert, 1 Zeile hinzugef?gt(+), 3 Zeilen entfernt(-)
> > 
> > diff --git a/arch/arm/cpu/armv7/tegra20/usb.c
> > b/arch/arm/cpu/armv7/tegra20/usb.c index 1bccf2b..9fd1edc 100644
> > --- a/arch/arm/cpu/armv7/tegra20/usb.c
> > +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> > @@ -43,9 +43,7 @@
> > 
> >         #endif
> >  
> >  #endif
> > 
> > -enum {
> > -       USB_PORTS_MAX   = 4,                    /* Maximum ports we allow
> > */ -};
> > +#define USB_PORTS_MAX  3               /* Maximum ports we allow */
> 
> That's fine with me if you wan to change it.
> 
> I tend to use enums most of the time. It shows up as a symbol in the
> debugger, avoids bracketed expressions, side-effects and the like, and
> works well when numbering multiple things (automatic increment). It's
> also a welcome language feature IMO - use it or lose it :-) But in
> this case the benefit is small.

What about using static const int ?

> >  /* Parameters we need for USB */
> >  enum {
> > 
> > --
> > 1.7.11.7
> 
> Regards,
> Simon

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/8] tegra: usb: make controller init functions more self contained

[Lucas Stach]

Hello Simon,

Am Dienstag, den 30.10.2012, 06:03 -0700 schrieb Simon Glass:
> Hi Lucas,
> 
> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> > There is no need to pass around all those parameters. The init functions
> > are able to easily extract all the needed setup info on their own.
> >
> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> > ---
> >  arch/arm/cpu/armv7/tegra20/usb.c | 24 ++++++++++++------------
> >  1 Datei ge?ndert, 12 Zeilen hinzugef?gt(+), 12 Zeilen entfernt(-)
> 
> I'm not sure I agree with the premise of this patch.
> 
> At the top level it calls clock_get_osc_freq() to get the frequency.
> That is then passed to the two places that need it.
> 
> It doesn't seem right to me to call clock_get_osc_freq() again in the
> lower level function just to avoid a parameter. On ARM at least a few
> parameters are a cheap way of passing data around.
> 
The intent of this patch is not really to save up on parameters passed,
but to make it possible to later move out the controller initialization
into the ehci_hcd_init function without having to save away this global
state for later use.

We have to init at most 2 controllers where timing matters, so I think
it's the right thing to get the SoC global clock state at those two
occasions to avoid inflating the file global state.

> It also allows the lower-level functions to deal with what they need
> to, rather than all functions having to reference the global state
> independently, each one digging down to what it actually needs.
> 
The controller init functions get passed the state only of the one port
they have to initialize. There is no point in extracting things at an
upper level and passing it into the functions, if it's exactly the same
thing that is stored in the port state.

Regards,
Lucas

[U-Boot] [PATCH 4/8] tegra: usb: remove unneeded function parameter

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> Just a dead parameter, never actually used.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>

Acked-by: Simon Glass <sjg@chromium.org>

> ---
>  arch/arm/cpu/armv7/tegra20/usb.c | 6 ++----
>  1 Datei ge?ndert, 2 Zeilen hinzugef?gt(+), 4 Zeilen entfernt(-)
>
> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
> index e61bd69..cf800b1 100644
> --- a/arch/arm/cpu/armv7/tegra20/usb.c
> +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> @@ -477,8 +477,7 @@ int tegrausb_stop_port(int portnum)
>         return 0;
>  }
>
> -int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
> -                  struct fdt_usb *config)
> +int fdt_decode_usb(const void *blob, int node, struct fdt_usb *config)
>  {
>         const char *phy, *mode;
>
> @@ -526,7 +525,6 @@ int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
>  int board_usb_init(const void *blob)
>  {
>         struct fdt_usb config;
> -       unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>         int node_list[USB_PORTS_MAX];
>         int node, count, i;
>         u32 *timing;
> @@ -545,7 +543,7 @@ int board_usb_init(const void *blob)
>                 node = node_list[i];
>                 if (!node)
>                         continue;
> -               if (fdt_decode_usb(blob, node, osc_freq, &config)) {
> +               if (fdt_decode_usb(blob, node, &config)) {

I believe there was originally some intent to put the timing data in
the fdt, so that this could be different for each board. Then the fdt
(most likely some common include file) might want to specify different
options for the different oscillator frequencies. However we have
never had to use such a construct. Even if we did then I suspect that
just adding a single timing set to each port would be good enough.

>                         debug("Cannot decode USB node %s\n",
>                               fdt_get_name(blob, node, NULL));
>                         return -1;
> --
> 1.7.11.7
>

Regards,
Simon

[U-Boot] [PATCH 3/8] tegra: usb: fold initial pll setup into board_usb_init

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> The setup is trivial, no need to split this out into a separate function.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
>  arch/arm/cpu/armv7/tegra20/usb.c | 15 +++++----------
>  1 Datei ge?ndert, 5 Zeilen hinzugef?gt(+), 10 Zeilen entfernt(-)
>
> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
> index 1725cd1..e61bd69 100644
> --- a/arch/arm/cpu/armv7/tegra20/usb.c
> +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> @@ -417,13 +417,6 @@ static int init_ulpi_usb_controller(struct fdt_usb *config)
>  }
>  #endif
>
> -static void config_clock(const u32 timing[])
> -{
> -       clock_start_pll(CLOCK_ID_USB,
> -               timing[PARAM_DIVM], timing[PARAM_DIVN], timing[PARAM_DIVP],
> -               timing[PARAM_CPCON], timing[PARAM_LFCON]);
> -}
> -
>  /**
>   * Add a new USB port to the list of available ports.
>   *
> @@ -534,13 +527,15 @@ int board_usb_init(const void *blob)
>  {
>         struct fdt_usb config;
>         unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
> -       enum clock_osc_freq freq;
>         int node_list[USB_PORTS_MAX];
>         int node, count, i;
> +       u32 *timing;
>
>         /* Set up the USB clocks correctly based on our oscillator frequency */
> -       freq = clock_get_osc_freq();
> -       config_clock(usb_pll[freq]);
> +       timing = usb_pll[clock_get_osc_freq()];
> +       clock_start_pll(CLOCK_ID_USB,
> +               timing[PARAM_DIVM], timing[PARAM_DIVN], timing[PARAM_DIVP],
> +               timing[PARAM_CPCON], timing[PARAM_LFCON]);

Sorry I don't see the benefit of this change. The function is there to
handle a clearly-defined task, hiding the detail of clock config
elsewhere. It has no effect on code generated.

>
>         /* count may return <0 on error */
>         count = fdtdec_find_aliases_for_id(blob, "usb",
> --
> 1.7.11.7
>

Regards,
Simon

[U-Boot] [PATCH 5/8] tegra: usb: move controller init into start_port

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> There is no need to init a USB controller before the upper layers indicate
> that they are actually going to use it.
>
> board_usb_init now only parses the device tree and sets up the common pll.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
>  arch/arm/cpu/armv7/tegra20/usb.c | 47 +++++++++++++++-------------------------
>  1 Datei ge?ndert, 18 Zeilen hinzugef?gt(+), 29 Zeilen entfernt(-)
>
> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
> index cf800b1..e372b8b 100644
> --- a/arch/arm/cpu/armv7/tegra20/usb.c
> +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> @@ -417,44 +417,29 @@ static int init_ulpi_usb_controller(struct fdt_usb *config)
>  }
>  #endif
>
> -/**
> - * Add a new USB port to the list of available ports.
> - *
> - * @param config       USB port configuration
> - * @return 0 if ok, -1 if error (too many ports)
> - */
> -static int add_port(struct fdt_usb *config)
> +int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor)
>  {
> -       if (port_count == USB_PORTS_MAX) {
> -               printf("tegrausb: Cannot register more than %d ports\n",
> -                     USB_PORTS_MAX);
> +       struct fdt_usb *config;
> +       struct usb_ctlr *usbctlr;
> +
> +       if (portnum >= port_count)
>                 return -1;
> -       }
> +
> +       config = &port[portnum];
>
>         if (config->utmi && init_utmi_usb_controller(config)) {
> -               printf("tegrausb: Cannot init port\n");
> +               printf("tegrausb: Cannot init port %d\n", portnum);
>                 return -1;
>         }
>
>         if (config->ulpi && init_ulpi_usb_controller(config)) {
> -               printf("tegrausb: Cannot init port\n");
> +               printf("tegrausb: Cannot init port %d\n", portnum);
>                 return -1;
>         }
>
> -       port[port_count++] = *config;
> -
> -       return 0;
> -}
> +       set_host_mode(config);

This is good, but now I think you will re-init the USB peripheral at
every 'usb start'. Perhaps you should remember whether it has been
inited and only do it the first time?

Regards,
Simon

>
> -int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor)
> -{
> -       struct usb_ctlr *usbctlr;
> -
> -       if (portnum >= port_count)
> -               return -1;
> -       set_host_mode(&port[portnum]);
> -
> -       usbctlr = port[portnum].reg;
> +       usbctlr = config->reg;
>         *hccr = (u32)&usbctlr->cap_length;
>         *hcor = (u32)&usbctlr->usb_cmd;
>         return 0;
> @@ -539,6 +524,12 @@ int board_usb_init(const void *blob)
>         count = fdtdec_find_aliases_for_id(blob, "usb",
>                         COMPAT_NVIDIA_TEGRA20_USB, node_list, USB_PORTS_MAX);
>         for (i = 0; i < count; i++) {
> +               if (port_count == USB_PORTS_MAX) {
> +                       printf("tegrausb: Cannot register more than %d ports\n",
> +                               USB_PORTS_MAX);
> +                       return -1;
> +               }
> +
>                 debug("USB %d: ", i);
>                 node = node_list[i];
>                 if (!node)
> @@ -549,9 +540,7 @@ int board_usb_init(const void *blob)
>                         return -1;
>                 }
>
> -               if (add_port(&config))
> -                       return -1;
> -               set_host_mode(&config);
> +               port[port_count++] = config;
>         }
>
>         return 0;
> --
> 1.7.11.7
>

[U-Boot] [PATCH 6/8] tegra: usb: various small cleanups

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> Remove unneeded headers, function prototype and stale comment.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
>  arch/arm/cpu/armv7/tegra20/usb.c        | 13 +------------
>  arch/arm/include/asm/arch-tegra20/usb.h |  3 ---
>  2 Dateien ge?ndert, 1 Zeile hinzugef?gt(+), 15 Zeilen entfernt(-)
>
> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
> index e372b8b..2cc95d2 100644
> --- a/arch/arm/cpu/armv7/tegra20/usb.c
> +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> @@ -25,21 +25,15 @@
>  #include <asm/io.h>
>  #include <asm-generic/gpio.h>
>  #include <asm/arch/clock.h>
> -#include <asm/arch/gpio.h>
> -#include <asm/arch/pinmux.h>
> -#include <asm/arch/tegra.h>
>  #include <asm/arch/usb.h>
>  #include <usb/ulpi.h>
> -#include <asm/arch-tegra/clk_rst.h>
> -#include <asm/arch-tegra/sys_proto.h>
> -#include <asm/arch-tegra/uart.h>
>  #include <libfdt.h>
>  #include <fdtdec.h>
>
>  #ifdef CONFIG_USB_ULPI
>         #ifndef CONFIG_USB_ULPI_VIEWPORT
>         #error  "To use CONFIG_USB_ULPI on Tegra Boards you have to also \
> -                       define CONFIG_USB_ULPI_VIEWPORT"
> +               define CONFIG_USB_ULPI_VIEWPORT"
>         #endif
>  #endif
>
> @@ -188,11 +182,6 @@ void usbf_reset_controller(struct fdt_usb *config, struct usb_ctlr *usbctlr)
>         /* Enable the UTMIP PHY */
>         if (config->utmi)
>                 setbits_le32(&usbctlr->susp_ctrl, UTMIP_PHY_ENB);
> -
> -       /*
> -        * TODO: where do we take the USB1 out of reset? The old code would
> -        * take USB3 out of reset, but not USB1. This code doesn't do either.
> -        */

How did this get resolved?

>  }
>
>  /* set up the UTMI USB controller with the parameters provided */
> diff --git a/arch/arm/include/asm/arch-tegra20/usb.h b/arch/arm/include/asm/arch-tegra20/usb.h
> index fdbd127..b18c850 100644
> --- a/arch/arm/include/asm/arch-tegra20/usb.h
> +++ b/arch/arm/include/asm/arch-tegra20/usb.h
> @@ -243,9 +243,6 @@ struct usb_ctlr {
>  #define VBUS_VLD_STS                   (1 << 26)
>
>
> -/* Change the USB host port into host mode */
> -void usb_set_host_mode(void);
> -

Everything else looks good.

>  /* Setup USB on the board */
>  int board_usb_init(const void *blob);
>
> --
> 1.7.11.7
>

Regards,
Simon

[U-Boot] [PATCH 3/8] tegra: usb: fold initial pll setup into board_usb_init

[Lucas Stach]

Am Dienstag, den 30.10.2012, 06:23 -0700 schrieb Simon Glass:
> Hi Lucas,
> 
> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> > The setup is trivial, no need to split this out into a separate function.
> >
> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> > ---
> >  arch/arm/cpu/armv7/tegra20/usb.c | 15 +++++----------
> >  1 Datei ge?ndert, 5 Zeilen hinzugef?gt(+), 10 Zeilen entfernt(-)
> >
> > diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
> > index 1725cd1..e61bd69 100644
> > --- a/arch/arm/cpu/armv7/tegra20/usb.c
> > +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> > @@ -417,13 +417,6 @@ static int init_ulpi_usb_controller(struct fdt_usb *config)
> >  }
> >  #endif
> >
> > -static void config_clock(const u32 timing[])
> > -{
> > -       clock_start_pll(CLOCK_ID_USB,
> > -               timing[PARAM_DIVM], timing[PARAM_DIVN], timing[PARAM_DIVP],
> > -               timing[PARAM_CPCON], timing[PARAM_LFCON]);
> > -}
> > -
> >  /**
> >   * Add a new USB port to the list of available ports.
> >   *
> > @@ -534,13 +527,15 @@ int board_usb_init(const void *blob)
> >  {
> >         struct fdt_usb config;
> >         unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
> > -       enum clock_osc_freq freq;
> >         int node_list[USB_PORTS_MAX];
> >         int node, count, i;
> > +       u32 *timing;
> >
> >         /* Set up the USB clocks correctly based on our oscillator frequency */
> > -       freq = clock_get_osc_freq();
> > -       config_clock(usb_pll[freq]);
> > +       timing = usb_pll[clock_get_osc_freq()];
> > +       clock_start_pll(CLOCK_ID_USB,
> > +               timing[PARAM_DIVM], timing[PARAM_DIVN], timing[PARAM_DIVP],
> > +               timing[PARAM_CPCON], timing[PARAM_LFCON]);
> 
> Sorry I don't see the benefit of this change. The function is there to
> handle a clearly-defined task, hiding the detail of clock config
> elsewhere. It has no effect on code generated.
> 
It's more of a personal thing, that every time there is a function call
it breaks the flow when reading the code. And IMHO it's not worth the
break if the called function does nothing other than just calling
another function.

If other people also dislike the change I may just drop it, but I would
like to hear some more opinions about this first.

Regards,
Lucas

[U-Boot] [PATCH 7/8] tegra: usb: move implementation into right directory

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> This moves the Tegra USB implementation into the drivers/usb/host
> directory.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
>  arch/arm/cpu/armv7/tegra20/Makefile                |  2 -
>  .../tegra20/usb.c => drivers/usb/host/ehci-tegra.c | 60 ++++++++++++++++++++--
>  2 Dateien ge?ndert, 55 Zeilen hinzugef?gt(+), 7 Zeilen entfernt(-)
>  rename arch/arm/cpu/armv7/tegra20/usb.c => drivers/usb/host/ehci-tegra.c (92%)

For me this patch did not apply:

Applying: tegra: usb: move implementation into right directory
error: drivers/usb/host/ehci-tegra.c: already exists in index
Patch failed at 0007 tegra: usb: move implementation into right directory
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

I tried master and tegra/master.

Regards,
Simon

[U-Boot] [PATCH 5/8] tegra: usb: move controller init into start_port

[Lucas Stach]

Am Dienstag, den 30.10.2012, 06:27 -0700 schrieb Simon Glass:
> Hi Lucas,
> 
> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> > There is no need to init a USB controller before the upper layers indicate
> > that they are actually going to use it.
> >
> > board_usb_init now only parses the device tree and sets up the common pll.
> >
> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> > ---
> >  arch/arm/cpu/armv7/tegra20/usb.c | 47 +++++++++++++++-------------------------
> >  1 Datei ge?ndert, 18 Zeilen hinzugef?gt(+), 29 Zeilen entfernt(-)
> >
> > diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
> > index cf800b1..e372b8b 100644
> > --- a/arch/arm/cpu/armv7/tegra20/usb.c
> > +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> > @@ -417,44 +417,29 @@ static int init_ulpi_usb_controller(struct fdt_usb *config)
> >  }
> >  #endif
> >
> > -/**
> > - * Add a new USB port to the list of available ports.
> > - *
> > - * @param config       USB port configuration
> > - * @return 0 if ok, -1 if error (too many ports)
> > - */
> > -static int add_port(struct fdt_usb *config)
> > +int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor)
> >  {
> > -       if (port_count == USB_PORTS_MAX) {
> > -               printf("tegrausb: Cannot register more than %d ports\n",
> > -                     USB_PORTS_MAX);
> > +       struct fdt_usb *config;
> > +       struct usb_ctlr *usbctlr;
> > +
> > +       if (portnum >= port_count)
> >                 return -1;
> > -       }
> > +
> > +       config = &port[portnum];
> >
> >         if (config->utmi && init_utmi_usb_controller(config)) {
> > -               printf("tegrausb: Cannot init port\n");
> > +               printf("tegrausb: Cannot init port %d\n", portnum);
> >                 return -1;
> >         }
> >
> >         if (config->ulpi && init_ulpi_usb_controller(config)) {
> > -               printf("tegrausb: Cannot init port\n");
> > +               printf("tegrausb: Cannot init port %d\n", portnum);
> >                 return -1;
> >         }
> >
> > -       port[port_count++] = *config;
> > -
> > -       return 0;
> > -}
> > +       set_host_mode(config);
> 
> This is good, but now I think you will re-init the USB peripheral at
> every 'usb start'. Perhaps you should remember whether it has been
> inited and only do it the first time?

I have to look this up, but the upper USB layers should not call those
lowlevel init functions repeatedly unless explicitly asked for it
through a "usb reset" or the like. If it actually does so it's a bug in
the upper layer and should not be fixed up in the lowlevel functions.

Regards,
Lucas

[U-Boot] [PATCH 7/8] tegra: usb: move implementation into right directory

[Lucas Stach]

Am Dienstag, den 30.10.2012, 06:33 -0700 schrieb Simon Glass:
> Hi Lucas,
> 
> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> > This moves the Tegra USB implementation into the drivers/usb/host
> > directory.
> >
> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> > ---
> >  arch/arm/cpu/armv7/tegra20/Makefile                |  2 -
> >  .../tegra20/usb.c => drivers/usb/host/ehci-tegra.c | 60 ++++++++++++++++++++--
> >  2 Dateien ge?ndert, 55 Zeilen hinzugef?gt(+), 7 Zeilen entfernt(-)
> >  rename arch/arm/cpu/armv7/tegra20/usb.c => drivers/usb/host/ehci-tegra.c (92%)
> 
> For me this patch did not apply:
> 
> Applying: tegra: usb: move implementation into right directory
> error: drivers/usb/host/ehci-tegra.c: already exists in index
> Patch failed at 0007 tegra: usb: move implementation into right directory
> When you have resolved this problem run "git am --resolved".
> If you would prefer to skip this patch, instead run "git am --skip".
> To restore the original branch and stop patching run "git am --abort".
> 
> I tried master and tegra/master.

The series based on u-boot-usb/master, as it's supposed to go in through
this tree.

Regards,
Lucas

[U-Boot] [PATCH 8/8] tegra: usb: move [start|stop]_port into ehci_hcd_[init|stop]

[Simon Glass]

Hi,

On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> The ehci_hcd entry points were just calling into the Tegra USB functions. Now
> that they are in the same file we can just move over the implementation.
>

Seems reasonable - the original approach was to put SOC-specific code
into arch/arm/cpu/armv7/tegra.., but I don't see any particular
benefit to that, and it could in fact get quite unwieldy. Since Tegra2
and Tegra3 both use the same USB it doesn't really matter anwyay.

> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
>  arch/arm/include/asm/arch-tegra20/usb.h | 19 -------
>  drivers/usb/host/ehci-tegra.c           | 93 +++++++++++++--------------------
>  2 Dateien ge?ndert, 35 Zeilen hinzugef?gt(+), 77 Zeilen entfernt(-)
>
> diff --git a/arch/arm/include/asm/arch-tegra20/usb.h b/arch/arm/include/asm/arch-tegra20/usb.h
> index b18c850..ef6c089 100644
> --- a/arch/arm/include/asm/arch-tegra20/usb.h
> +++ b/arch/arm/include/asm/arch-tegra20/usb.h
> @@ -246,23 +246,4 @@ struct usb_ctlr {
>  /* Setup USB on the board */
>  int board_usb_init(const void *blob);
>
> -/**
> - * Start up the given port number (ports are numbered from 0 on each board).
> - * This returns values for the appropriate hccr and hcor addresses to use for
> - * USB EHCI operations.
> - *
> - * @param portnum      port number to start
> - * @param hccr         returns start address of EHCI HCCR registers
> - * @param hcor         returns start address of EHCI HCOR registers
> - * @return 0 if ok, -1 on error (generally invalid port number)
> - */

But please can we keep this nice comment? I really don't like
uncommented functions.

[snip]

Regards,
Simon

[U-Boot] [PATCH 1/8] tegra: usb: convert USB_PORTS_MAX to be a define

[Simon Glass]

Hi Marek,

On Tue, Oct 30, 2012 at 6:11 AM, Marek Vasut <marex@denx.de> wrote:
> Dear Simon Glass,
>
>> Hi Lucas,
>>
>> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
>> > No point in having this as an enum. Also while at it set it to the real
>> > hardware maximum for both Tegra 2 and Tegra 3. If new Tegra hardware
>> > includes more USB controllers we can always bump the limit then.
>> >
>> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
>> > ---
>> >
>> >  arch/arm/cpu/armv7/tegra20/usb.c | 4 +---
>> >  1 Datei ge?ndert, 1 Zeile hinzugef?gt(+), 3 Zeilen entfernt(-)
>> >
>> > diff --git a/arch/arm/cpu/armv7/tegra20/usb.c
>> > b/arch/arm/cpu/armv7/tegra20/usb.c index 1bccf2b..9fd1edc 100644
>> > --- a/arch/arm/cpu/armv7/tegra20/usb.c
>> > +++ b/arch/arm/cpu/armv7/tegra20/usb.c
>> > @@ -43,9 +43,7 @@
>> >
>> >         #endif
>> >
>> >  #endif
>> >
>> > -enum {
>> > -       USB_PORTS_MAX   = 4,                    /* Maximum ports we allow
>> > */ -};
>> > +#define USB_PORTS_MAX  3               /* Maximum ports we allow */
>>
>> That's fine with me if you wan to change it.
>>
>> I tend to use enums most of the time. It shows up as a symbol in the
>> debugger, avoids bracketed expressions, side-effects and the like, and
>> works well when numbering multiple things (automatic increment). It's
>> also a welcome language feature IMO - use it or lose it :-) But in
>> this case the benefit is small.
>
> What about using static const int ?

That's fine too.

[snip]

>
> Best regards,
> Marek Vasut

Regards,
Simon

[U-Boot] [PATCH 5/8] tegra: usb: move controller init into start_port

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 6:37 AM, Lucas Stach <dev@lynxeye.de> wrote:
> Am Dienstag, den 30.10.2012, 06:27 -0700 schrieb Simon Glass:
>> Hi Lucas,
>>
>> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
>> > There is no need to init a USB controller before the upper layers indicate
>> > that they are actually going to use it.
>> >
>> > board_usb_init now only parses the device tree and sets up the common pll.
>> >
>> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
>> > ---
>> >  arch/arm/cpu/armv7/tegra20/usb.c | 47 +++++++++++++++-------------------------
>> >  1 Datei ge?ndert, 18 Zeilen hinzugef?gt(+), 29 Zeilen entfernt(-)
>> >
>> > diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
>> > index cf800b1..e372b8b 100644
>> > --- a/arch/arm/cpu/armv7/tegra20/usb.c
>> > +++ b/arch/arm/cpu/armv7/tegra20/usb.c
>> > @@ -417,44 +417,29 @@ static int init_ulpi_usb_controller(struct fdt_usb *config)
>> >  }
>> >  #endif
>> >
>> > -/**
>> > - * Add a new USB port to the list of available ports.
>> > - *
>> > - * @param config       USB port configuration
>> > - * @return 0 if ok, -1 if error (too many ports)
>> > - */
>> > -static int add_port(struct fdt_usb *config)
>> > +int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor)
>> >  {
>> > -       if (port_count == USB_PORTS_MAX) {
>> > -               printf("tegrausb: Cannot register more than %d ports\n",
>> > -                     USB_PORTS_MAX);
>> > +       struct fdt_usb *config;
>> > +       struct usb_ctlr *usbctlr;
>> > +
>> > +       if (portnum >= port_count)
>> >                 return -1;
>> > -       }
>> > +
>> > +       config = &port[portnum];
>> >
>> >         if (config->utmi && init_utmi_usb_controller(config)) {
>> > -               printf("tegrausb: Cannot init port\n");
>> > +               printf("tegrausb: Cannot init port %d\n", portnum);
>> >                 return -1;
>> >         }
>> >
>> >         if (config->ulpi && init_ulpi_usb_controller(config)) {
>> > -               printf("tegrausb: Cannot init port\n");
>> > +               printf("tegrausb: Cannot init port %d\n", portnum);
>> >                 return -1;
>> >         }
>> >
>> > -       port[port_count++] = *config;
>> > -
>> > -       return 0;
>> > -}
>> > +       set_host_mode(config);
>>
>> This is good, but now I think you will re-init the USB peripheral at
>> every 'usb start'. Perhaps you should remember whether it has been
>> inited and only do it the first time?
>
> I have to look this up, but the upper USB layers should not call those
> lowlevel init functions repeatedly unless explicitly asked for it
> through a "usb reset" or the like. If it actually does so it's a bug in
> the upper layer and should not be fixed up in the lowlevel functions.

Perhaps, but you have to write your code in the environment that
exists. At present usb_lowlevel_init() is called on every 'usb start'
(and ehci_hcd_init() from that).

Regards,
Simon

>
> Regards,
> Lucas
>
>

[U-Boot] [PATCH 7/8] tegra: usb: move implementation into right directory

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 6:38 AM, Lucas Stach <dev@lynxeye.de> wrote:
> Am Dienstag, den 30.10.2012, 06:33 -0700 schrieb Simon Glass:
>> Hi Lucas,
>>
>> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
>> > This moves the Tegra USB implementation into the drivers/usb/host
>> > directory.
>> >
>> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
>> > ---
>> >  arch/arm/cpu/armv7/tegra20/Makefile                |  2 -
>> >  .../tegra20/usb.c => drivers/usb/host/ehci-tegra.c | 60 ++++++++++++++++++++--
>> >  2 Dateien ge?ndert, 55 Zeilen hinzugef?gt(+), 7 Zeilen entfernt(-)
>> >  rename arch/arm/cpu/armv7/tegra20/usb.c => drivers/usb/host/ehci-tegra.c (92%)
>>
>> For me this patch did not apply:
>>
>> Applying: tegra: usb: move implementation into right directory
>> error: drivers/usb/host/ehci-tegra.c: already exists in index
>> Patch failed at 0007 tegra: usb: move implementation into right directory
>> When you have resolved this problem run "git am --resolved".
>> If you would prefer to skip this patch, instead run "git am --skip".
>> To restore the original branch and stop patching run "git am --abort".
>>
>> I tried master and tegra/master.
>
> The series based on u-boot-usb/master, as it's supposed to go in through
> this tree.

OK thanks, I assumed that because tegra: was the first tag it would go
through tegra.

But it doesn't seem to help:

git remote -v |grep upstream-usb
upstream-usb	http://git.denx.de/u-boot-usb.git (fetch)
upstream-usb	http://git.denx.de/u-boot-usb.git (push)
git fetch upstream-usb

[U-Boot] [PATCH 5/8] tegra: usb: move controller init into start_port

[Lucas Stach]

Am Dienstag, den 30.10.2012, 06:48 -0700 schrieb Simon Glass:
> Hi Lucas,
> 
> On Tue, Oct 30, 2012 at 6:37 AM, Lucas Stach <dev@lynxeye.de> wrote:
> > Am Dienstag, den 30.10.2012, 06:27 -0700 schrieb Simon Glass:
> >> Hi Lucas,
> >>
> >> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> >> > There is no need to init a USB controller before the upper layers indicate
> >> > that they are actually going to use it.
> >> >
> >> > board_usb_init now only parses the device tree and sets up the common pll.
> >> >
> >> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> >> > ---
> >> >  arch/arm/cpu/armv7/tegra20/usb.c | 47 +++++++++++++++-------------------------
> >> >  1 Datei ge?ndert, 18 Zeilen hinzugef?gt(+), 29 Zeilen entfernt(-)
> >> >
> >> > diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
> >> > index cf800b1..e372b8b 100644
> >> > --- a/arch/arm/cpu/armv7/tegra20/usb.c
> >> > +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> >> > @@ -417,44 +417,29 @@ static int init_ulpi_usb_controller(struct fdt_usb *config)
> >> >  }
> >> >  #endif
> >> >
> >> > -/**
> >> > - * Add a new USB port to the list of available ports.
> >> > - *
> >> > - * @param config       USB port configuration
> >> > - * @return 0 if ok, -1 if error (too many ports)
> >> > - */
> >> > -static int add_port(struct fdt_usb *config)
> >> > +int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor)
> >> >  {
> >> > -       if (port_count == USB_PORTS_MAX) {
> >> > -               printf("tegrausb: Cannot register more than %d ports\n",
> >> > -                     USB_PORTS_MAX);
> >> > +       struct fdt_usb *config;
> >> > +       struct usb_ctlr *usbctlr;
> >> > +
> >> > +       if (portnum >= port_count)
> >> >                 return -1;
> >> > -       }
> >> > +
> >> > +       config = &port[portnum];
> >> >
> >> >         if (config->utmi && init_utmi_usb_controller(config)) {
> >> > -               printf("tegrausb: Cannot init port\n");
> >> > +               printf("tegrausb: Cannot init port %d\n", portnum);
> >> >                 return -1;
> >> >         }
> >> >
> >> >         if (config->ulpi && init_ulpi_usb_controller(config)) {
> >> > -               printf("tegrausb: Cannot init port\n");
> >> > +               printf("tegrausb: Cannot init port %d\n", portnum);
> >> >                 return -1;
> >> >         }
> >> >
> >> > -       port[port_count++] = *config;
> >> > -
> >> > -       return 0;
> >> > -}
> >> > +       set_host_mode(config);
> >>
> >> This is good, but now I think you will re-init the USB peripheral at
> >> every 'usb start'. Perhaps you should remember whether it has been
> >> inited and only do it the first time?
> >
> > I have to look this up, but the upper USB layers should not call those
> > lowlevel init functions repeatedly unless explicitly asked for it
> > through a "usb reset" or the like. If it actually does so it's a bug in
> > the upper layer and should not be fixed up in the lowlevel functions.
> 
> Perhaps, but you have to write your code in the environment that
> exists. At present usb_lowlevel_init() is called on every 'usb start'
> (and ehci_hcd_init() from that).
> 
After all this is open source and I would rather spin a patch to fix
this at the right spot if we do the wrong thing, than having to cope
with the bug at a lower level. Even with bug present we are not failing
in any severe way, we are just wasting time bringing up a controller
which is already up.

Regards,
Lucas

[U-Boot] [PATCH 5/8] tegra: usb: move controller init into start_port

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 6:54 AM, Lucas Stach <dev@lynxeye.de> wrote:
> Am Dienstag, den 30.10.2012, 06:48 -0700 schrieb Simon Glass:
>> Hi Lucas,
>>
>> On Tue, Oct 30, 2012 at 6:37 AM, Lucas Stach <dev@lynxeye.de> wrote:
>> > Am Dienstag, den 30.10.2012, 06:27 -0700 schrieb Simon Glass:
>> >> Hi Lucas,
>> >>
>> >> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
>> >> > There is no need to init a USB controller before the upper layers indicate
>> >> > that they are actually going to use it.
>> >> >
>> >> > board_usb_init now only parses the device tree and sets up the common pll.
>> >> >
>> >> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
>> >> > ---
>> >> >  arch/arm/cpu/armv7/tegra20/usb.c | 47 +++++++++++++++-------------------------
>> >> >  1 Datei ge?ndert, 18 Zeilen hinzugef?gt(+), 29 Zeilen entfernt(-)
>> >> >
>> >> > diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
>> >> > index cf800b1..e372b8b 100644
>> >> > --- a/arch/arm/cpu/armv7/tegra20/usb.c
>> >> > +++ b/arch/arm/cpu/armv7/tegra20/usb.c
>> >> > @@ -417,44 +417,29 @@ static int init_ulpi_usb_controller(struct fdt_usb *config)
>> >> >  }
>> >> >  #endif
>> >> >
>> >> > -/**
>> >> > - * Add a new USB port to the list of available ports.
>> >> > - *
>> >> > - * @param config       USB port configuration
>> >> > - * @return 0 if ok, -1 if error (too many ports)
>> >> > - */
>> >> > -static int add_port(struct fdt_usb *config)
>> >> > +int tegrausb_start_port(int portnum, u32 *hccr, u32 *hcor)
>> >> >  {
>> >> > -       if (port_count == USB_PORTS_MAX) {
>> >> > -               printf("tegrausb: Cannot register more than %d ports\n",
>> >> > -                     USB_PORTS_MAX);
>> >> > +       struct fdt_usb *config;
>> >> > +       struct usb_ctlr *usbctlr;
>> >> > +
>> >> > +       if (portnum >= port_count)
>> >> >                 return -1;
>> >> > -       }
>> >> > +
>> >> > +       config = &port[portnum];
>> >> >
>> >> >         if (config->utmi && init_utmi_usb_controller(config)) {
>> >> > -               printf("tegrausb: Cannot init port\n");
>> >> > +               printf("tegrausb: Cannot init port %d\n", portnum);
>> >> >                 return -1;
>> >> >         }
>> >> >
>> >> >         if (config->ulpi && init_ulpi_usb_controller(config)) {
>> >> > -               printf("tegrausb: Cannot init port\n");
>> >> > +               printf("tegrausb: Cannot init port %d\n", portnum);
>> >> >                 return -1;
>> >> >         }
>> >> >
>> >> > -       port[port_count++] = *config;
>> >> > -
>> >> > -       return 0;
>> >> > -}
>> >> > +       set_host_mode(config);
>> >>
>> >> This is good, but now I think you will re-init the USB peripheral at
>> >> every 'usb start'. Perhaps you should remember whether it has been
>> >> inited and only do it the first time?
>> >
>> > I have to look this up, but the upper USB layers should not call those
>> > lowlevel init functions repeatedly unless explicitly asked for it
>> > through a "usb reset" or the like. If it actually does so it's a bug in
>> > the upper layer and should not be fixed up in the lowlevel functions.
>>
>> Perhaps, but you have to write your code in the environment that
>> exists. At present usb_lowlevel_init() is called on every 'usb start'
>> (and ehci_hcd_init() from that).
>>
> After all this is open source and I would rather spin a patch to fix
> this at the right spot if we do the wrong thing, than having to cope
> with the bug at a lower level. Even with bug present we are not failing
> in any severe way, we are just wasting time bringing up a controller
> which is already up.
>

OK, but perhaps my broader point is that this may in fact be the
intended behaviour of U-Boot. Until you bring this up and submit a
patch to change it, you may not know. I would suggest you change the
patch order here - first send a patch making usb_lowlevel_init() work
the way you want, then a patch to change Tegra to init each time
usb_lowlevel_init() is called.

I'm not sure about the time penalty - I suspect it is small - but why
have any such penalty? Boot time is a key concern (at least for me!).
Plus re-init of already-inited hardware can be problematic.

Or you could fix this for now by remembering what is inited and not
doing it again - just another flag in struct fdt_usb. It is good that
you don't init USB until needed, and that would solve the problem in
the interim, until you get usb_lowlevel_init() changed. Ultimately
with the device model we may be able to go further and not even do the
fdt decode until needed.

Regards,
Simon

> Regards,
> Lucas
>

[U-Boot] [PATCH 7/8] tegra: usb: move implementation into right directory

[Lucas Stach]

Am Dienstag, den 30.10.2012, 06:53 -0700 schrieb Simon Glass:
> Hi Lucas,
> 
> On Tue, Oct 30, 2012 at 6:38 AM, Lucas Stach <dev@lynxeye.de> wrote:
> > Am Dienstag, den 30.10.2012, 06:33 -0700 schrieb Simon Glass:
> >> Hi Lucas,
> >>
> >> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> >> > This moves the Tegra USB implementation into the drivers/usb/host
> >> > directory.
> >> >
> >> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> >> > ---
> >> >  arch/arm/cpu/armv7/tegra20/Makefile                |  2 -
> >> >  .../tegra20/usb.c => drivers/usb/host/ehci-tegra.c | 60 ++++++++++++++++++++--
> >> >  2 Dateien ge?ndert, 55 Zeilen hinzugef?gt(+), 7 Zeilen entfernt(-)
> >> >  rename arch/arm/cpu/armv7/tegra20/usb.c => drivers/usb/host/ehci-tegra.c (92%)
> >>
> >> For me this patch did not apply:
> >>
> >> Applying: tegra: usb: move implementation into right directory
> >> error: drivers/usb/host/ehci-tegra.c: already exists in index
> >> Patch failed at 0007 tegra: usb: move implementation into right directory
> >> When you have resolved this problem run "git am --resolved".
> >> If you would prefer to skip this patch, instead run "git am --skip".
> >> To restore the original branch and stop patching run "git am --abort".
> >>
> >> I tried master and tegra/master.
> >
> > The series based on u-boot-usb/master, as it's supposed to go in through
> > this tree.
> 
> OK thanks, I assumed that because tegra: was the first tag it would go
> through tegra.
> 
> But it doesn't seem to help:
> 
> git remote -v |grep upstream-usb
> upstream-usb	http://git.denx.de/u-boot-usb.git (fetch)
> upstream-usb	http://git.denx.de/u-boot-usb.git (push)
> git fetch upstream-usb
> >From http://git.denx.de/u-boot-usb
>  * [new branch]      at91sam9x35-ek -> upstream-usb/at91sam9x35-ek
>  + 5b2e031...0b92a45 cdc-at91   -> upstream-usb/cdc-at91  (forced update)
>  + 6722fd5...76454b2 master     -> upstream-usb/master  (forced update)
>  * [new branch]      merge_pending -> upstream-usb/merge_pending
>  + 2c8b43b...01afc4f next       -> upstream-usb/next  (forced update)
>  * [new branch]      uboot      -> upstream-usb/uboot
> (try-usb=5cf309: include/ lq out/ tools/ x/) ~/u> co -b try-usb2
> upstream-usb/master
> Branch try-usb2 set up to track remote branch master from upstream-usb.
> Switched to a new branch 'try-usb2'
> (try-usb2=76454b: include/ lq out/ tools/ x/) ~/u> git am
> ~/Downloads/bundle-3480.mbox
> Applying: tegra: usb: convert USB_PORTS_MAX to be a define
> Applying: tegra: usb: make controller init functions more self contained
> Applying: tegra: usb: fold initial pll setup into board_usb_init
> Applying: tegra: usb: remove unneeded function parameter
> Applying: tegra: usb: move controller init into start_port
> Applying: tegra: usb: various small cleanups
> Applying: tegra: usb: move implementation into right directory
> error: drivers/usb/host/ehci-tegra.c: already exists in index
> Patch failed at 0007 tegra: usb: move implementation into right directory
> When you have resolved this problem run "git am --resolved".
> If you would prefer to skip this patch, instead run "git am --skip".
> To restore the original branch and stop patching run "git am --abort".
> 
> Where was the patch that removed drivers/usb/host/ehci-tegra.c?
> 
Hm I'm no expert here, but I didn't actually remove the file. I just
copied over the contents of the old implementation file and both git
commit and git format-patch recognized this as a rename. Also the
cherry-pick from my devel to the usb branch worked flawlessly.
If git am can't cope with the rename to an already existing file I may
post the patch with the rename forcibly removed, but this will yield a
much bigger patch. I'll investigate this.

Thanks for the heads up.

Regards,
Lucas

[U-Boot] [PATCH 2/8] tegra: usb: make controller init functions more self contained

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 6:16 AM, Lucas Stach <dev@lynxeye.de> wrote:
> Hello Simon,
>
> Am Dienstag, den 30.10.2012, 06:03 -0700 schrieb Simon Glass:
>> Hi Lucas,
>>
>> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
>> > There is no need to pass around all those parameters. The init functions
>> > are able to easily extract all the needed setup info on their own.
>> >
>> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
>> > ---
>> >  arch/arm/cpu/armv7/tegra20/usb.c | 24 ++++++++++++------------
>> >  1 Datei ge?ndert, 12 Zeilen hinzugef?gt(+), 12 Zeilen entfernt(-)
>>
>> I'm not sure I agree with the premise of this patch.
>>
>> At the top level it calls clock_get_osc_freq() to get the frequency.
>> That is then passed to the two places that need it.
>>
>> It doesn't seem right to me to call clock_get_osc_freq() again in the
>> lower level function just to avoid a parameter. On ARM at least a few
>> parameters are a cheap way of passing data around.
>>
> The intent of this patch is not really to save up on parameters passed,
> but to make it possible to later move out the controller initialization
> into the ehci_hcd_init function without having to save away this global
> state for later use.
>
> We have to init at most 2 controllers where timing matters, so I think
> it's the right thing to get the SoC global clock state at those two
> occasions to avoid inflating the file global state.

OK fair enough, I see that you want to do these two types of init at
different times.

>
>> It also allows the lower-level functions to deal with what they need
>> to, rather than all functions having to reference the global state
>> independently, each one digging down to what it actually needs.
>>
> The controller init functions get passed the state only of the one port
> they have to initialize. There is no point in extracting things at an
> upper level and passing it into the functions, if it's exactly the same
> thing that is stored in the port state.

Well if the upper level is extracting it anyway, it often saves code
space to pass the extracted value. I would like to avoid this sort of
thing:

void do_something(struct level1 *level1)
{
   struct level2 *level2 = level1->level2;
   struct level3 *level3 = level2->level3;

   level3->...
   level3->...
}

void do_something_else(struct level1 *level1)
{
   struct level2 *level2 = level1->level2;
   struct level3 *level3 = level2->level3;

   level3->...
   level3->...
}

main()
{
   do_something(level1)
   do_something_else(level1)
}


I generally prefer:

void do_something(struct level3 *level3)
{
   level3->...
   level3->...
}

void do_something_else(struct level3 *level3)
{
   level3->...
   level3->...
}

main()
{
   struct level2 *level2 = level1->level2;
   struct level3 *level3 = level2->level3;

   do_something(level3)
   do_something_else(level3)
}


I hope that example makes sense.

>
> Regards,
> Lucas
>
>

Regards,
Simon

[U-Boot] [PATCH 7/8] tegra: usb: move implementation into right directory

[Tom Warren]

Simon, et al,

> -----Original Message-----
> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
> Sent: Tuesday, October 30, 2012 6:34 AM
> To: Lucas Stach
> Cc: Marek Vasut; u-boot at lists.denx.de; Stephen Warren; Tom Warren
> Subject: Re: [PATCH 7/8] tegra: usb: move implementation into right
> directory
> 
> Hi Lucas,
> 
> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> > This moves the Tegra USB implementation into the drivers/usb/host
> > directory.
> >
> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> > ---
> >  arch/arm/cpu/armv7/tegra20/Makefile                |  2 -
> >  .../tegra20/usb.c => drivers/usb/host/ehci-tegra.c | 60
> > ++++++++++++++++++++--
> >  2 Dateien ge?ndert, 55 Zeilen hinzugef?gt(+), 7 Zeilen entfernt(-)
> > rename arch/arm/cpu/armv7/tegra20/usb.c =>
> > drivers/usb/host/ehci-tegra.c (92%)
> 
> For me this patch did not apply:
> 
> Applying: tegra: usb: move implementation into right directory
> error: drivers/usb/host/ehci-tegra.c: already exists in index Patch failed
> at 0007 tegra: usb: move implementation into right directory When you have
> resolved this problem run "git am --resolved".
> If you would prefer to skip this patch, instead run "git am --skip".
> To restore the original branch and stop patching run "git am --abort".
> 
> I tried master and tegra/master.
Please apply/develop all Tegra patches on tegra/next. That's where I'll be applying new patches as they come in. /master is usually behind /next until a pull request occurs (which just happens to be what the state is now, so master == next, but that's not the norm).

Thanks,

Tom
> 
> Regards,
> Simon
-- 
nvpublic

[U-Boot] [PATCH 7/8] tegra: usb: move implementation into right directory

[Simon Glass]

Hi Tom,

On Tue, Oct 30, 2012 at 9:11 AM, Tom Warren <TWarren@nvidia.com> wrote:
> Simon, et al,
[snip]
> Please apply/develop all Tegra patches on tegra/next. That's where I'll be applying new patches as they come in. /master is usually behind /next until a pull request occurs (which just happens to be what the state is now, so master == next, but that's not the norm).

OK thanks for the clarification. I was tending towards that, but had
sometimes found next was old, if that makes any sense. Will do from
now on.

The LCD series applied on top of tegra/next for me without trouble.

Regards,
Simon

>
> Thanks,
>
> Tom
>>
>> Regards,
>> Simon
> --
> nvpublic
>

[U-Boot] [PATCH 5/8] tegra: usb: move controller init into start_port

[Stephen Warren]

On 10/30/2012 06:44 AM, Lucas Stach wrote:
> Hi Marek,
> 
> Am Dienstag, den 30.10.2012, 13:33 +0100 schrieb Marek Vasut:
>> Dear Lucas Stach,
>>
>> [...]
>>
>>>>> -static int add_port(struct fdt_usb *config)
>>>>
>>>> Fix the comment instead of removing it?
>>>
>>> I don't think that this comment adds any real value. The whole function
>>> which this comment refers to is removed and it's content split between
>>> board_usb_init and ehci_hcd_init, which are self explanatory.
>>
>> Then add a proper comment please. Call me a docu-nazi, but I'd really love u-
>> boot nicely and properly documented, please.
>>
> I'm all in favour of adding proper documentation, but I'm opposed to add
> it in the middle of this cleanup/movement series.
> 
> I'll send a patch on top of this series to add doc, so it doesn't
> interfere with the review of this series.

If the docs already exist and the function is simply changing its exact
semantics, the docs should remain and simply be updated.

[U-Boot] [PATCH 7/8] tegra: usb: move implementation into right directory

[Stephen Warren]

On 10/30/2012 03:22 AM, Lucas Stach wrote:
> This moves the Tegra USB implementation into the drivers/usb/host
> directory.

> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/drivers/usb/host/ehci-tegra.c

> + * Copyright (c) 2009-2012 NVIDIA Corporation
>   * Copyright (c) 2011 The Chromium OS Authors.
> - * (C) Copyright 2010,2011 NVIDIA Corporation <www.nvidia.com>

Why does NVIDIA's (c) notice change?

[U-Boot] [PATCH 7/8] tegra: usb: move implementation into right directory

[Lucas Stach]

Am Dienstag, den 30.10.2012, 12:38 -0600 schrieb Stephen Warren:
> On 10/30/2012 03:22 AM, Lucas Stach wrote:
> > This moves the Tegra USB implementation into the drivers/usb/host
> > directory.
> 
> > diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/drivers/usb/host/ehci-tegra.c
> 
> > + * Copyright (c) 2009-2012 NVIDIA Corporation
> >   * Copyright (c) 2011 The Chromium OS Authors.
> > - * (C) Copyright 2010,2011 NVIDIA Corporation <www.nvidia.com>
> 
> Why does NVIDIA's (c) notice change?
> 
Just because I took most of the licence header from the ehci_tegra.c
file, but as the patch shows the the diff modulo the copied part it
looks like a change. I choose this one as it actually spans the longer
copyright timeframe of the both licence headers. Is this a problem?
Should I also include the web address?

Regards,
Lucas

[U-Boot] [PATCH 7/8] tegra: usb: move implementation into right directory

[Stephen Warren]

On 10/30/2012 12:45 PM, Lucas Stach wrote:
> Am Dienstag, den 30.10.2012, 12:38 -0600 schrieb Stephen Warren:
>> On 10/30/2012 03:22 AM, Lucas Stach wrote:
>>> This moves the Tegra USB implementation into the drivers/usb/host
>>> directory.
>>
>>> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/drivers/usb/host/ehci-tegra.c
>>
>>> + * Copyright (c) 2009-2012 NVIDIA Corporation
>>>   * Copyright (c) 2011 The Chromium OS Authors.
>>> - * (C) Copyright 2010,2011 NVIDIA Corporation <www.nvidia.com>
>>
>> Why does NVIDIA's (c) notice change?
>>
> Just because I took most of the licence header from the ehci_tegra.c
> file, but as the patch shows the the diff modulo the copied part it
> looks like a change. I choose this one as it actually spans the longer
> copyright timeframe of the both licence headers. Is this a problem?
> Should I also include the web address?

Hmm. So this patch merges two files together into one? The diff looks
like it's creating a new file. If the new content in this patch came
from some other file, shouldn't the patch also remove it from the old
file? That would make the (c) header change more obvious.

[U-Boot] [PATCH 7/8] tegra: usb: move implementation into right directory

[Lucas Stach]

Am Dienstag, den 30.10.2012, 12:51 -0600 schrieb Stephen Warren:
> On 10/30/2012 12:45 PM, Lucas Stach wrote:
> > Am Dienstag, den 30.10.2012, 12:38 -0600 schrieb Stephen Warren:
> >> On 10/30/2012 03:22 AM, Lucas Stach wrote:
> >>> This moves the Tegra USB implementation into the drivers/usb/host
> >>> directory.
> >>
> >>> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/drivers/usb/host/ehci-tegra.c
> >>
> >>> + * Copyright (c) 2009-2012 NVIDIA Corporation
> >>>   * Copyright (c) 2011 The Chromium OS Authors.
> >>> - * (C) Copyright 2010,2011 NVIDIA Corporation <www.nvidia.com>
> >>
> >> Why does NVIDIA's (c) notice change?
> >>
> > Just because I took most of the licence header from the ehci_tegra.c
> > file, but as the patch shows the the diff modulo the copied part it
> > looks like a change. I choose this one as it actually spans the longer
> > copyright timeframe of the both licence headers. Is this a problem?
> > Should I also include the web address?
> 
> Hmm. So this patch merges two files together into one? The diff looks
> like it's creating a new file. If the new content in this patch came
> from some other file, shouldn't the patch also remove it from the old
> file? That would make the (c) header change more obvious.
> 
Hm, the rename presentation of this patch seems to cause major
confusion. Yes, this patch merges the Tegra usb implementation into the
pre-existing ehci_tegra.c file, which before this change did not much
more than to call into the Tegra SoC usb implementation. As the Tegra
usb implementation is not really SoC specific, but rather controller
specific this should really move over.

I will send this patch in the usual remove-add presentation for a V2 of
the series, as it even seems to confuse git right now.

Regards,
Lucas

[U-Boot] [PATCH 1/8] tegra: usb: convert USB_PORTS_MAX to be a define

[Marek Vasut]

Dear Lucas Stach,

[...]

Seeing Simon's comments, I expect new version of this series, right?

Best regards,
Marek Vasut