[U-Boot] [PATCH v2 0/5] Tegra 2 USB ULPI series

[U-Boot] [PATCH v2 0/5] Tegra 2 USB ULPI series

[Lucas Stach]

With this series we are able to initialize USB controllers
using an external ULPI phy AKA USB2 on Tegra 2 devices.

This was tested to work on a Toradex Colibri T20 board,
where USB2 is used to access the ASIX ethernet chipset.
Testing was done with "tegra20: usb: rework set_host_mode" [1]
applied. I did not spot any regressions on the UTMI ports.

v2 incorporates all the review feedback I've got so far,
including now trying harder to enable VBus in all configurations.

Patch 3 is already in u-boot-usb and only provided as the ULPI
init code now depends on it.
Igor could you please take a look at Patch 4?

Patchset is based on top of u-boot-tegra/next

[1] http://lists.denx.de/pipermail/u-boot/2012-August/130177.html

Lucas Stach (5):
  tegra20: complete periph_id enum
  tegra20: add clock_set_pllout function
  usb: fix ulpi_set_vbus prototype
  usb: ulpi: add indicator configuration function
  tegra20: add USB ULPI init code

 arch/arm/cpu/armv7/tegra20/usb.c            | 154 +++++++++++++++++++++++-----
 arch/arm/cpu/tegra20-common/clock.c         |  39 +++++++
 arch/arm/cpu/tegra20-common/warmboot_avp.c  |   2 +-
 arch/arm/include/asm/arch-tegra20/clk_rst.h |  11 +-
 arch/arm/include/asm/arch-tegra20/clock.h   |  25 +++++
 arch/arm/include/asm/arch-tegra20/usb.h     |  29 +++++-
 drivers/usb/ulpi/ulpi.c                     |  26 ++++-
 include/usb/ulpi.h                          |  13 ++-
 8 Dateien ge?ndert, 262 Zeilen hinzugef?gt(+), 37 Zeilen entfernt(-)

-- 
1.7.11.4

[U-Boot] [PATCH v2 1/5] tegra20: complete periph_id enum

[Lucas Stach]

Most Tegra boards output the ULPI reference clock on pad DEV2.

Complete the periph_id enum so that we are able to enable this
clock output circuit.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
Acked-by: Stephen Warren <swarren@wwwdotorg.org>
Acked-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/tegra20-common/clock.c       | 1 +
 arch/arm/include/asm/arch-tegra20/clock.h | 6 ++++++
 2 Dateien ge?ndert, 7 Zeilen hinzugef?gt(+)

diff --git a/arch/arm/cpu/tegra20-common/clock.c b/arch/arm/cpu/tegra20-common/clock.c
index 2403874..d9bb851 100644
--- a/arch/arm/cpu/tegra20-common/clock.c
+++ b/arch/arm/cpu/tegra20-common/clock.c
@@ -502,6 +502,7 @@ static int clock_periph_id_isvalid(enum periph_id id)
 		case PERIPH_ID_RESERVED81:
 		case PERIPH_ID_RESERVED82:
 		case PERIPH_ID_RESERVED83:
+		case PERIPH_ID_RESERVED91:
 			printf("Peripheral id %d is reserved\n", id);
 			break;
 		default:
diff --git a/arch/arm/include/asm/arch-tegra20/clock.h b/arch/arm/include/asm/arch-tegra20/clock.h
index ff83bbf..20db9e6 100644
--- a/arch/arm/include/asm/arch-tegra20/clock.h
+++ b/arch/arm/include/asm/arch-tegra20/clock.h
@@ -175,6 +175,12 @@ enum periph_id {
 
 	/* 88 */
 	PERIPH_ID_CRAM2,
+	PERIPH_ID_SYNC_CLK_DOUBLER,
+	PERIPH_ID_CLK_M_DOUBLER,
+	PERIPH_ID_RESERVED91,
+	PERIPH_ID_SUS_OUT,
+	PERIPH_ID_DEV2_OUT,
+	PERIPH_ID_DEV1_OUT,
 
 	PERIPH_ID_COUNT,
 	PERIPH_ID_NONE = -1,
-- 
1.7.11.4

[U-Boot] [PATCH v2 2/5] tegra20: add clock_set_pllout function

[Lucas Stach]

Common practice on Tegra 2 boards is to use the pllp_out4 FO
to generate the ULPI reference clock. For this to work we have
to override the default hardware generated output divider.

This function adds a clean way to do so.

v2:
- check if pllout is valid

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 arch/arm/cpu/tegra20-common/clock.c         | 38 +++++++++++++++++++++++++++++
 arch/arm/cpu/tegra20-common/warmboot_avp.c  |  2 +-
 arch/arm/include/asm/arch-tegra20/clk_rst.h | 11 +++++++--
 arch/arm/include/asm/arch-tegra20/clock.h   | 19 +++++++++++++++
 4 Dateien ge?ndert, 67 Zeilen hinzugef?gt(+), 3 Zeilen entfernt(-)

diff --git a/arch/arm/cpu/tegra20-common/clock.c b/arch/arm/cpu/tegra20-common/clock.c
index d9bb851..6ebddf8 100644
--- a/arch/arm/cpu/tegra20-common/clock.c
+++ b/arch/arm/cpu/tegra20-common/clock.c
@@ -396,6 +396,16 @@ static s8 periph_id_to_internal_id[PERIPH_ID_COUNT] = {
 	NONE(CRAM2),
 };
 
+/* number of clock outputs of a PLL */
+static const u8 pll_num_clkouts[] = {
+	1,	/* PLLC */
+	1,	/* PLLM */
+	4,	/* PLLP */
+	1,	/* PLLA */
+	0,	/* PLLU */
+	0,	/* PLLD */
+};
+
 /*
  * Get the oscillator frequency, from the corresponding hardware configuration
  * field.
@@ -604,6 +614,34 @@ unsigned long clock_get_periph_rate(enum periph_id periph_id,
 		(readl(reg) & OUT_CLK_DIVISOR_MASK) >> OUT_CLK_DIVISOR_SHIFT);
 }
 
+int clock_set_pllout(enum clock_id clkid, enum pll_out_id pllout, unsigned rate)
+{
+	struct clk_pll *pll = get_pll(clkid);
+	int data = 0, div = 0, offset = 0;
+
+	if (!clock_id_is_pll(clkid))
+		return -1;
+
+	if (pllout + 1 > pll_num_clkouts[clkid])
+		return -1;
+
+	div = clk_get_divider(8, pll_rate[clkid], rate);
+
+	if (div < 0)
+		return -1;
+
+	/* out2 and out4 are in the high part of the register */
+	if (pllout == PLL_OUT2 || pllout == PLL_OUT4)
+		offset = 16;
+
+	data = (div << PLL_OUT_RATIO_SHIFT) |
+	       PLL_OUT_OVRRIDE | PLL_OUT_CLKEN | PLL_OUT_RSTN;
+	clrsetbits_le32(&pll->pll_out[pllout >> 1],
+	                PLL_OUT_RATIO_MASK << offset, data << offset);
+
+	return 0;
+}
+
 /**
  * Find the best available 7.1 format divisor given a parent clock rate and
  * required child clock rate. This function assumes that a second-stage
diff --git a/arch/arm/cpu/tegra20-common/warmboot_avp.c b/arch/arm/cpu/tegra20-common/warmboot_avp.c
index cd01908..f6c71cb 100644
--- a/arch/arm/cpu/tegra20-common/warmboot_avp.c
+++ b/arch/arm/cpu/tegra20-common/warmboot_avp.c
@@ -214,7 +214,7 @@ void wb_start(void)
 
 	reg = PLLM_OUT1_RSTN_RESET_DISABLE | PLLM_OUT1_CLKEN_ENABLE |
 	      PLLM_OUT1_RATIO_VAL_8;
-	writel(reg, &clkrst->crc_pll[CLOCK_ID_MEMORY].pll_out);
+	writel(reg, &clkrst->crc_pll[CLOCK_ID_MEMORY].pll_out[0]);
 
 	reg = SCLK_SWAKE_FIQ_SRC_PLLM_OUT1 | SCLK_SWAKE_IRQ_SRC_PLLM_OUT1 |
 	      SCLK_SWAKE_RUN_SRC_PLLM_OUT1 | SCLK_SWAKE_IDLE_SRC_PLLM_OUT1 |
diff --git a/arch/arm/include/asm/arch-tegra20/clk_rst.h b/arch/arm/include/asm/arch-tegra20/clk_rst.h
index 8c3be91..7b548c2 100644
--- a/arch/arm/include/asm/arch-tegra20/clk_rst.h
+++ b/arch/arm/include/asm/arch-tegra20/clk_rst.h
@@ -27,8 +27,7 @@
 /* PLL registers - there are several PLLs in the clock controller */
 struct clk_pll {
 	uint pll_base;		/* the control register */
-	uint pll_out;		/* output control */
-	uint reserved;
+	uint pll_out[2];	/* output control */
 	uint pll_misc;		/* other misc things */
 };
 
@@ -112,6 +111,14 @@ struct clk_rst_ctlr {
 #define PLL_DIVM_SHIFT		0
 #define PLL_DIVM_MASK		(0x1f << PLL_DIVM_SHIFT)
 
+/* CLK_RST_CONTROLLER_PLLx_OUTx_0 */
+#define PLL_OUT_RSTN		(1 << 0)
+#define PLL_OUT_CLKEN		(1 << 1)
+#define PLL_OUT_OVRRIDE		(1 << 2)
+
+#define PLL_OUT_RATIO_SHIFT	8
+#define PLL_OUT_RATIO_MASK	(0xffU << PLL_OUT_RATIO_SHIFT)
+
 /* CLK_RST_CONTROLLER_PLLx_MISC_0 */
 #define PLL_CPCON_SHIFT		8
 #define PLL_CPCON_MASK		(15U << PLL_CPCON_SHIFT)
diff --git a/arch/arm/include/asm/arch-tegra20/clock.h b/arch/arm/include/asm/arch-tegra20/clock.h
index 20db9e6..dfef51e 100644
--- a/arch/arm/include/asm/arch-tegra20/clock.h
+++ b/arch/arm/include/asm/arch-tegra20/clock.h
@@ -186,6 +186,13 @@ enum periph_id {
 	PERIPH_ID_NONE = -1,
 };
 
+enum pll_out_id {
+	PLL_OUT1,
+	PLL_OUT2,
+	PLL_OUT3,
+	PLL_OUT4
+};
+
 /* Converts a clock number to a clock register: 0=L, 1=H, 2=U */
 #define PERIPH_REG(id) ((id) >> 5)
 
@@ -218,6 +225,18 @@ unsigned long clock_start_pll(enum clock_id id, u32 divm, u32 divn,
 		u32 divp, u32 cpcon, u32 lfcon);
 
 /**
+ * Set PLL output frequency
+ *
+ * @param clkid		clock id
+ * @param pllout	pll output id (
+ * @param rate		desired output rate
+ *
+ * @return 0 if ok, -1 on error (invalid clock id or no suitable divider)
+ */
+int clock_set_pllout(enum clock_id clkid, enum pll_out_id pllout,
+                     unsigned rate);
+
+/**
  * Read low-level parameters of a PLL.
  *
  * @param id	clock id to read (note: USB is not supported)
-- 
1.7.11.4

[U-Boot] [PATCH v2 3/5] usb: fix ulpi_set_vbus prototype

[Lucas Stach]

Match the name of the header prototype with the actual
implementation.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 include/usb/ulpi.h | 2 +-
 1 Datei ge?ndert, 1 Zeile hinzugef?gt(+), 1 Zeile entfernt(-)

diff --git a/include/usb/ulpi.h b/include/usb/ulpi.h
index 4a23fd2..9a75c24 100644
--- a/include/usb/ulpi.h
+++ b/include/usb/ulpi.h
@@ -61,7 +61,7 @@ int ulpi_select_transceiver(struct ulpi_viewport *ulpi_vp, unsigned speed);
  *
  * returns 0 on success, ULPI_ERROR on failure.
  */
-int ulpi_enable_vbus(struct ulpi_viewport *ulpi_vp,
+int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp,
 			int on, int ext_power, int ext_ind);
 
 /*
-- 
1.7.11.4

[U-Boot] [PATCH v2 4/5] usb: ulpi: add indicator configuration function

[Lucas Stach]

Allows for easy configuration of the VBUS indicator related ULPI
config bits.

Also move the external indicator setup from ulpi_set_vbus() to
the new function.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 drivers/usb/ulpi/ulpi.c | 26 ++++++++++++++++++++++----
 include/usb/ulpi.h      | 13 +++++++++++--
 2 Dateien ge?ndert, 33 Zeilen hinzugef?gt(+), 6 Zeilen entfernt(-)

diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c
index dde2585..f358bde 100644
--- a/drivers/usb/ulpi/ulpi.c
+++ b/drivers/usb/ulpi/ulpi.c
@@ -106,20 +106,38 @@ int ulpi_select_transceiver(struct ulpi_viewport *ulpi_vp, unsigned speed)
 	return ulpi_write(ulpi_vp, &ulpi->function_ctrl, val);
 }
 
-int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power,
-			int ext_ind)
+int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power)
 {
 	u32 flags = ULPI_OTG_DRVVBUS;
 	u8 *reg = on ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
 
 	if (ext_power)
 		flags |= ULPI_OTG_DRVVBUS_EXT;
-	if (ext_ind)
-		flags |= ULPI_OTG_EXTVBUSIND;
 
 	return ulpi_write(ulpi_vp, reg, flags);
 }
 
+int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int external,
+			int passthu, int complement)
+{
+	u8 *reg;
+	int ret;
+
+	reg = external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
+	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND)))
+		return ret;
+
+	reg = passthu ? &ulpi->iface_ctrl_set : &ulpi->iface_ctrl_clear;
+	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_PASSTHRU)))
+		return ret;
+
+	reg = complement ? &ulpi->iface_ctrl_set : &ulpi->iface_ctrl_clear;
+	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_EXTVBUS_COMPLEMENT)))
+		return ret;
+
+	return 0;
+}
+
 int ulpi_set_pd(struct ulpi_viewport *ulpi_vp, int enable)
 {
 	u32 val = ULPI_OTG_DP_PULLDOWN | ULPI_OTG_DM_PULLDOWN;
diff --git a/include/usb/ulpi.h b/include/usb/ulpi.h
index 9a75c24..99166c4 100644
--- a/include/usb/ulpi.h
+++ b/include/usb/ulpi.h
@@ -61,8 +61,17 @@ int ulpi_select_transceiver(struct ulpi_viewport *ulpi_vp, unsigned speed);
  *
  * returns 0 on success, ULPI_ERROR on failure.
  */
-int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp,
-			int on, int ext_power, int ext_ind);
+int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power);
+
+/*
+ * Configure VBUS indicator
+ * @external		- external VBUS over-current indicator is used
+ * @passthru		- disables ANDing of internal VBUS comparator
+ *                    with external VBUS input
+ * @complement		- inverts the external VBUS input
+ */
+int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int external,
+			int passthru, int complement);
 
 /*
  * Enable/disable pull-down resistors on D+ and D- USB lines.
-- 
1.7.11.4

[U-Boot] [PATCH v2 5/5] tegra20: add USB ULPI init code

[Lucas Stach]

This adds the required code to set up a ULPI USB port. It is
mostly a port of the Linux ULPI setup code with some tweaks
added for more correctness, discovered along the way of
debugging this.

To use this both CONFIG_USB_ULPI and CONFIG_USB_ULPI_VIEWPORT
have to be set in the board configuration file.

v2:
- move all controller init stuff in the respective functions to
  make them self contained
- let board define ULPI_REF_CLK to account for the possibility
  that some ULPI phys need a other ref clk than 24MHz
- don't touch ULPI regs directly, use ULPI framework functions
- don't hide error messages under debug()

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

diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
index 77966e5..351300b 100644
--- a/arch/arm/cpu/armv7/tegra20/usb.c
+++ b/arch/arm/cpu/armv7/tegra20/usb.c
@@ -32,9 +32,17 @@
 #include <asm/arch/sys_proto.h>
 #include <asm/arch/uart.h>
 #include <asm/arch/usb.h>
+#include <usb/ulpi.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"
+	#endif
+#endif
+
 enum {
 	USB_PORTS_MAX	= 4,			/* Maximum ports we allow */
 };
@@ -68,11 +76,13 @@ enum dr_mode {
 struct fdt_usb {
 	struct usb_ctlr *reg;	/* address of registers in physical memory */
 	unsigned utmi:1;	/* 1 if port has external tranceiver, else 0 */
+	unsigned ulpi:1;	/* 1 if port has external ULPI transceiver */
 	unsigned enabled:1;	/* 1 to enable, 0 to disable */
 	unsigned has_legacy_mode:1; /* 1 if this port has legacy mode */
 	enum dr_mode dr_mode;	/* dual role mode */
 	enum periph_id periph_id;/* peripheral id */
 	struct fdt_gpio_state vbus_gpio;	/* GPIO for vbus enable */
+	struct fdt_gpio_state phy_reset_gpio; /* GPIO to reset ULPI phy */
 };
 
 static struct fdt_usb port[USB_PORTS_MAX];	/* List of valid USB ports */
@@ -187,8 +197,8 @@ static void usbf_reset_controller(struct fdt_usb *config,
 	 */
 }
 
-/* set up the USB controller with the parameters provided */
-static int init_usb_controller(struct fdt_usb *config,
+/* 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[])
 {
 	u32 val;
@@ -297,16 +307,109 @@ static int init_usb_controller(struct fdt_usb *config,
 	if (!loop_count)
 		return -1;
 
-	return 0;
-}
+	/* Disable ICUSB FS/LS transceiver */
+	clrbits_le32(&usbctlr->icusb_ctrl, IC_ENB1);
+
+	/* Select UTMI parallel interface */
+	clrsetbits_le32(&usbctlr->port_sc1, PTS_MASK,
+			PTS_UTMI << PTS_SHIFT);
+	clrbits_le32(&usbctlr->port_sc1, STS);
 
-static void power_up_port(struct usb_ctlr *usbctlr)
-{
 	/* Deassert power down state */
 	clrbits_le32(&usbctlr->utmip_xcvr_cfg0, UTMIP_FORCE_PD_POWERDOWN |
 		UTMIP_FORCE_PD2_POWERDOWN | UTMIP_FORCE_PDZI_POWERDOWN);
 	clrbits_le32(&usbctlr->utmip_xcvr_cfg1, UTMIP_FORCE_PDDISC_POWERDOWN |
 		UTMIP_FORCE_PDCHRP_POWERDOWN | UTMIP_FORCE_PDDR_POWERDOWN);
+
+	return 0;
+}
+
+/* set up the ULPI USB controller with the parameters provided */
+static int init_ulpi_usb_controller(struct fdt_usb *config,
+				struct usb_ctlr *usbctlr)
+{
+#ifdef CONFIG_USB_ULPI
+/* if board file does not set a ULPI reference frequency we default to 24MHz */
+#ifndef ULPI_REF_CLK
+#define ULPI_REF_CLK 24000000
+#endif
+	u32 val;
+	int loop_count;
+	struct ulpi_viewport ulpi_vp;
+
+	/* set up ULPI reference clock on pllp_out4 */
+	clock_enable(PERIPH_ID_DEV2_OUT);
+	clock_set_pllout(CLOCK_ID_PERIPH, PLL_OUT4, ULPI_REF_CLK);
+
+	/* reset ULPI phy */
+	if (fdt_gpio_isvalid(&config->phy_reset_gpio)) {
+		fdtdec_setup_gpio(&config->phy_reset_gpio);
+		gpio_direction_output(config->phy_reset_gpio.gpio, 0);
+		mdelay(5);
+		gpio_set_value(config->phy_reset_gpio.gpio, 1);
+	}
+
+	/* Reset the usb controller */
+	clock_enable(config->periph_id);
+	usbf_reset_controller(config, usbctlr);
+
+	/* enable pinmux bypass */
+	setbits_le32(&usbctlr->ulpi_timing_ctrl_0,
+				ULPI_CLKOUT_PINMUX_BYP | ULPI_OUTPUT_PINMUX_BYP);
+
+	/* Select ULPI parallel interface */
+	clrsetbits_le32(&usbctlr->port_sc1, PTS_MASK, PTS_ULPI << PTS_SHIFT);
+
+	/* enable ULPI transceiver */
+	setbits_le32(&usbctlr->susp_ctrl, ULPI_PHY_ENB);
+
+	/* configure ULPI transceiver timings */
+	val = 0;
+	writel(val, &usbctlr->ulpi_timing_ctrl_1);
+
+	val |= ULPI_DATA_TRIMMER_SEL(4);
+	val |= ULPI_STPDIRNXT_TRIMMER_SEL(4);
+	val |= ULPI_DIR_TRIMMER_SEL(4);
+	writel(val, &usbctlr->ulpi_timing_ctrl_1);
+	udelay(10);
+
+	val |= ULPI_DATA_TRIMMER_LOAD;
+	val |= ULPI_STPDIRNXT_TRIMMER_LOAD;
+	val |= ULPI_DIR_TRIMMER_LOAD;
+	writel(val, &usbctlr->ulpi_timing_ctrl_1);
+
+	/* set up phy for host operation with external vbus supply */
+	ulpi_vp.port_num = 0;
+	ulpi_vp.viewport_addr = (u32)&usbctlr->ulpi_viewport;
+
+	if (ulpi_init(&ulpi_vp)) {
+		printf("Tegra ULPI viewport init failed\n");
+		return -1;
+	}
+
+	ulpi_set_vbus(&ulpi_vp, 1, 1);
+	ulpi_set_vbus_indicator(&ulpi_vp, 1, 1, 0);
+
+	/* enable wakeup events */
+	setbits_le32(&usbctlr->port_sc1, WKCN | WKDS | WKOC);
+
+	setbits_le32(&usbctlr->susp_ctrl, USB_SUSP_CLR);
+	/* Wait for the phy clock to become valid in 100 ms */
+	for (loop_count = 100000; loop_count != 0; loop_count--) {
+		if (readl(&usbctlr->susp_ctrl) & USB_PHY_CLK_VALID)
+			break;
+		udelay(1);
+	}
+	if (!loop_count)
+		return -1;
+	clrbits_le32(&usbctlr->susp_ctrl, USB_SUSP_CLR);
+
+	return 0;
+#else
+	printf("No code to set up ULPI controller, please enable"
+			"CONFIG_USB_ULPI and CONFIG_USB_ULPI_VIEWPORT");
+	return -1;
+#endif
 }
 
 static void config_clock(const u32 timing[])
@@ -327,24 +430,25 @@ static int add_port(struct fdt_usb *config, const u32 timing[])
 	struct usb_ctlr *usbctlr = config->reg;
 
 	if (port_count == USB_PORTS_MAX) {
-		debug("tegrausb: Cannot register more than %d ports\n",
+		printf("tegrausb: Cannot register more than %d ports\n",
 		      USB_PORTS_MAX);
 		return -1;
 	}
-	if (init_usb_controller(config, usbctlr, timing)) {
-		debug("tegrausb: Cannot init port\n");
-		return -1;
-	}
+
 	if (config->utmi) {
-		/* Disable ICUSB FS/LS transceiver */
-		clrbits_le32(&usbctlr->icusb_ctrl, IC_ENB1);
-
-		/* Select UTMI parallel interface */
-		clrsetbits_le32(&usbctlr->port_sc1, PTS_MASK,
-				PTS_UTMI << PTS_SHIFT);
-		clrbits_le32(&usbctlr->port_sc1, STS);
-		power_up_port(usbctlr);
+		if (init_utmi_usb_controller(config, usbctlr, timing)) {
+			printf("tegrausb: Cannot init port\n");
+			return -1;
+		}
+	}
+
+	if (config->ulpi) {
+		if (init_ulpi_usb_controller(config, usbctlr)) {
+			printf("tegrausb: Cannot init port\n");
+			return -1;
+		}
 	}
+
 	port[port_count++] = *config;
 
 	return 0;
@@ -411,6 +515,7 @@ static int fdt_decode_usb(const void *blob, int node,
 
 	phy = fdt_getprop(blob, node, "phy_type", NULL);
 	config->utmi = phy && 0 == strcmp("utmi", phy);
+	config->ulpi = phy && 0 == strcmp("ulpi", phy);
 	config->enabled = fdtdec_get_is_enabled(blob, node);
 	config->has_legacy_mode = fdtdec_get_bool(blob, node,
 						  "nvidia,has-legacy-mode");
@@ -420,10 +525,13 @@ static int fdt_decode_usb(const void *blob, int node,
 		return -FDT_ERR_NOTFOUND;
 	}
 	fdtdec_decode_gpio(blob, node, "nvidia,vbus-gpio", &config->vbus_gpio);
-	debug("enabled=%d, legacy_mode=%d, utmi=%d, periph_id=%d, vbus=%d, "
-	      "dr_mode=%d\n", config->enabled, config->has_legacy_mode,
-	      config->utmi, config->periph_id, config->vbus_gpio.gpio,
-	      config->dr_mode);
+	fdtdec_decode_gpio(blob, node, "nvidia,phy-reset-gpio",
+	                   &config->phy_reset_gpio);
+	debug("enabled=%d, legacy_mode=%d, utmi=%d, ulpi=%d, periph_id=%d, "
+	      "vbus=%d, phy_reset=%d, dr_mode=%d\n",
+	      config->enabled, config->has_legacy_mode, config->utmi, config->ulpi,
+	      config->periph_id, config->vbus_gpio.gpio,
+	      config->phy_reset_gpio.gpio, config->dr_mode);
 
 	return 0;
 }
diff --git a/arch/arm/include/asm/arch-tegra20/usb.h b/arch/arm/include/asm/arch-tegra20/usb.h
index 638033b..bd89d66 100644
--- a/arch/arm/include/asm/arch-tegra20/usb.h
+++ b/arch/arm/include/asm/arch-tegra20/usb.h
@@ -100,10 +100,12 @@ struct usb_ctlr {
 
 	/* 0x410 */
 	uint usb1_legacy_ctrl;
-	uint reserved12[3];
+	uint reserved12[4];
 
-	/* 0x420 */
-	uint reserved13[56];
+	/* 0x424 */
+	uint ulpi_timing_ctrl_0;
+	uint ulpi_timing_ctrl_1;
+	uint reserved13[53];
 
 	/* 0x500 */
 	uint reserved14[64 * 3];
@@ -144,10 +146,24 @@ struct usb_ctlr {
 #define VBUS_SENSE_CTL_AB_SESS_VLD		2
 #define VBUS_SENSE_CTL_A_SESS_VLD		3
 
+/* USB2_IF_ULPI_TIMING_CTRL_0 */
+#define ULPI_OUTPUT_PINMUX_BYP			(1 << 10)
+#define ULPI_CLKOUT_PINMUX_BYP			(1 << 11)
+
+/* USB2_IF_ULPI_TIMING_CTRL_1 */
+#define ULPI_DATA_TRIMMER_LOAD			(1 << 0)
+#define ULPI_DATA_TRIMMER_SEL(x)		(((x) & 0x7) << 1)
+#define ULPI_STPDIRNXT_TRIMMER_LOAD		(1 << 16)
+#define ULPI_STPDIRNXT_TRIMMER_SEL(x)	(((x) & 0x7) << 17)
+#define ULPI_DIR_TRIMMER_LOAD			(1 << 24)
+#define ULPI_DIR_TRIMMER_SEL(x)			(((x) & 0x7) << 25)
+
 /* USBx_IF_USB_SUSP_CTRL_0 */
+#define ULPI_PHY_ENB				(1 << 13)
 #define UTMIP_PHY_ENB			        (1 << 12)
 #define UTMIP_RESET			        (1 << 11)
 #define USB_PHY_CLK_VALID			(1 << 7)
+#define USB_SUSP_CLR				(1 << 5)
 
 /* USBx_UTMIP_MISC_CFG1 */
 #define UTMIP_PLLU_STABLE_COUNT_SHIFT		6
@@ -203,12 +219,15 @@ struct usb_ctlr {
 /* SB2_CONTROLLER_2_USB2D_PORTSC1_0 */
 #define PTS_SHIFT				30
 #define PTS_MASK				(3U << PTS_SHIFT)
-#define PTS_UTMI	0
+#define PTS_UTMI		0
 #define PTS_RESERVED	1
-#define PTS_ULP		2
+#define PTS_ULPI		2
 #define PTS_ICUSB_SER	3
 
 #define STS					(1 << 29)
+#define WKOC				(1 << 22)
+#define WKDS				(1 << 21)
+#define WKCN				(1 << 20)
 
 /* USBx_UTMIP_XCVR_CFG0_0 */
 #define UTMIP_FORCE_PD_POWERDOWN		(1 << 14)
-- 
1.7.11.4

[U-Boot] [PATCH v2 0/5] Tegra 2 USB ULPI series

[Stephen Warren]

On 08/21/2012 02:18 PM, Lucas Stach wrote:
> With this series we are able to initialize USB controllers
> using an external ULPI phy AKA USB2 on Tegra 2 devices.
> 
> This was tested to work on a Toradex Colibri T20 board,
> where USB2 is used to access the ASIX ethernet chipset.
> Testing was done with "tegra20: usb: rework set_host_mode" [1]
> applied. I did not spot any regressions on the UTMI ports.
> 
> v2 incorporates all the review feedback I've got so far,
> including now trying harder to enable VBus in all configurations.
> 
> Patch 3 is already in u-boot-usb and only provided as the ULPI
> init code now depends on it.
> Igor could you please take a look at Patch 4?
> 
> Patchset is based on top of u-boot-tegra/next
> 
> [1] http://lists.denx.de/pipermail/u-boot/2012-August/130177.html

OK, with the addition of an appropriate pin_mux_usb() function to
harmony.c, this works now. So, please consider the series,

Tested-by: Stephen Warren <swarren@wwwdotorg.org>

Note that I did see the following:

Tegra20 (Harmony) # usb start
(Re)start USB...
USB:   Register 10011 NbrPorts 1
USB EHCI 1.00
scanning bus for devices... Register 10011 NbrPorts 1
USB EHCI 1.00
scanning bus for devices... 5 USB Device(s) found
       scanning bus for storage devices... Device NOT ready
   Request Sense returned 02 3A 00
2 Storage Device(s) found
       scanning bus for ethernet devices... 1 Ethernet Device(s) found

However, both "usb tree" and then "ext2ls" on the USB device succeeded
without issue, so I think that's tested well enough. I even booted a
kernel from the USB device, and the kernel was able to use the USB
device for the root filesystem, so U-Boot hadn't broken it in any way:-)

[U-Boot] [PATCH v2 4/5] usb: ulpi: add indicator configuration function

[Igor Grinberg]

Hi Lucas, Tom,

I'm sorry for the late reply.
I understand, that Tom has already applied this to tegra/next,
but as the changes/follow up patches are required,
may be we can do this in another fashion...

1) Thanks for the patch and working on extending the generic framework!
2) This patch has no dependencies on tegra specific patches, so
   I think, it should go through Marex usb tree, but doing this will
   require the right merge order, so bisectability will not suffer.
   So, Marek, Tom, you should decide which way is fine with you both.


Tom,
Yesterday, I was wondering if the patch was already applied, and
I had no clue what's its status. Also, the patchwork says "New".
So, if it is not hard for you in the future, I'd like a short reply
to the list, saying something like: "Applied, thanks.", like most
custodians do. Thanks!

On 08/21/12 23:18, Lucas Stach wrote:
> Allows for easy configuration of the VBUS indicator related ULPI
> config bits.
> 
> Also move the external indicator setup from ulpi_set_vbus() to
> the new function.
> 
> Signed-off-by: Lucas Stach <dev@lynxeye.de>

After the below comments are fixed:
Acked-by: Igor Grinberg <grinberg@compulab.co.il>

> ---
>  drivers/usb/ulpi/ulpi.c | 26 ++++++++++++++++++++++----
>  include/usb/ulpi.h      | 13 +++++++++++--
>  2 Dateien ge?ndert, 33 Zeilen hinzugef?gt(+), 6 Zeilen entfernt(-)
> 
> diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c
> index dde2585..f358bde 100644
> --- a/drivers/usb/ulpi/ulpi.c
> +++ b/drivers/usb/ulpi/ulpi.c
> @@ -106,20 +106,38 @@ int ulpi_select_transceiver(struct ulpi_viewport *ulpi_vp, unsigned speed)
>  	return ulpi_write(ulpi_vp, &ulpi->function_ctrl, val);
>  }
>  
> -int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power,
> -			int ext_ind)
> +int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power)
>  {
>  	u32 flags = ULPI_OTG_DRVVBUS;
>  	u8 *reg = on ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
>  
>  	if (ext_power)
>  		flags |= ULPI_OTG_DRVVBUS_EXT;
> -	if (ext_ind)
> -		flags |= ULPI_OTG_EXTVBUSIND;
>  
>  	return ulpi_write(ulpi_vp, reg, flags);
>  }
>  
> +int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int external,
> +			int passthu, int complement)
> +{
> +	u8 *reg;
> +	int ret;
> +
> +	reg = external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
> +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND)))
> +		return ret;
> +
> +	reg = passthu ? &ulpi->iface_ctrl_set : &ulpi->iface_ctrl_clear;
> +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_PASSTHRU)))
> +		return ret;
> +
> +	reg = complement ? &ulpi->iface_ctrl_set : &ulpi->iface_ctrl_clear;
> +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_EXTVBUS_COMPLEMENT)))
> +		return ret;

These are fine, two requests though:
1) As Tom already pointed in the private email:
ERROR: do not use assignment in if condition
#361: FILE: drivers/usb/ulpi/ulpi.c:127:
+	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND)))

ERROR: do not use assignment in if condition
#365: FILE: drivers/usb/ulpi/ulpi.c:131:
+	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_PASSTHRU)))

ERROR: do not use assignment in if condition
#369: FILE: drivers/usb/ulpi/ulpi.c:135:
+	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_EXTVBUS_COMPLEMENT)))

those must be fixed.

2) Can you make only one access for each register?
   Use flags/val variable (like in other places) and
   do only one access per register. Can you?

> +
> +	return 0;
> +}
> +
>  int ulpi_set_pd(struct ulpi_viewport *ulpi_vp, int enable)
>  {
>  	u32 val = ULPI_OTG_DP_PULLDOWN | ULPI_OTG_DM_PULLDOWN;
> diff --git a/include/usb/ulpi.h b/include/usb/ulpi.h
> index 9a75c24..99166c4 100644
> --- a/include/usb/ulpi.h
> +++ b/include/usb/ulpi.h
> @@ -61,8 +61,17 @@ int ulpi_select_transceiver(struct ulpi_viewport *ulpi_vp, unsigned speed);
>   *
>   * returns 0 on success, ULPI_ERROR on failure.
>   */
> -int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp,
> -			int on, int ext_power, int ext_ind);
> +int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power);
> +
> +/*
> + * Configure VBUS indicator
> + * @external		- external VBUS over-current indicator is used
> + * @passthru		- disables ANDing of internal VBUS comparator
> + *                    with external VBUS input
> + * @complement		- inverts the external VBUS input
> + */
> +int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int external,
> +			int passthru, int complement);
>  
>  /*
>   * Enable/disable pull-down resistors on D+ and D- USB lines.

-- 
Regards,
Igor.

[U-Boot] [PATCH v2 5/5] tegra20: add USB ULPI init code

[Igor Grinberg]

On 08/21/12 23:18, Lucas Stach wrote:
> This adds the required code to set up a ULPI USB port. It is
> mostly a port of the Linux ULPI setup code with some tweaks
> added for more correctness, discovered along the way of
> debugging this.
> 
> To use this both CONFIG_USB_ULPI and CONFIG_USB_ULPI_VIEWPORT
> have to be set in the board configuration file.
> 
> v2:
> - move all controller init stuff in the respective functions to
>   make them self contained
> - let board define ULPI_REF_CLK to account for the possibility
>   that some ULPI phys need a other ref clk than 24MHz
> - don't touch ULPI regs directly, use ULPI framework functions
> - don't hide error messages under debug()
> 
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
>  arch/arm/cpu/armv7/tegra20/usb.c        | 154 +++++++++++++++++++++++++++-----
>  arch/arm/include/asm/arch-tegra20/usb.h |  29 ++++--
>  2 Dateien ge?ndert, 155 Zeilen hinzugef?gt(+), 28 Zeilen entfernt(-)
> 
> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
> index 77966e5..351300b 100644
> --- a/arch/arm/cpu/armv7/tegra20/usb.c
> +++ b/arch/arm/cpu/armv7/tegra20/usb.c

[...]

> +/* set up the ULPI USB controller with the parameters provided */
> +static int init_ulpi_usb_controller(struct fdt_usb *config,
> +				struct usb_ctlr *usbctlr)
> +{
> +#ifdef CONFIG_USB_ULPI
> +/* if board file does not set a ULPI reference frequency we default to 24MHz */
> +#ifndef ULPI_REF_CLK
> +#define ULPI_REF_CLK 24000000
> +#endif

I would really like the above ifdefs out of any function.
So, how about something like:
#ifdef CONFIG_USB_ULPI
#ifndef ULPI_REF_CLK
#define ULPI_REF_CLK 24000000
#endif
static int init_ulpi_usb_controller(struct fdt_usb *config,
				struct usb_ctlr *usbctlr)
{
...
}
#else
static int init_ulpi_usb_controller(struct fdt_usb *config,
				struct usb_ctlr *usbctlr)
{
	printf("No code to set up ULPI controller, please enable"
			"CONFIG_USB_ULPI and CONFIG_USB_ULPI_VIEWPORT");
	return -1;
}
#endif

This way all the ifdef are out of the functions and it makes code
really cleaner and pleasant to read.

Also, if this is to work from config files, then we should
make it CONFIG_ULPI_REF_CLK and add some documentation to the README.


> +	u32 val;
> +	int loop_count;
> +	struct ulpi_viewport ulpi_vp;
> +
> +	/* set up ULPI reference clock on pllp_out4 */
> +	clock_enable(PERIPH_ID_DEV2_OUT);
> +	clock_set_pllout(CLOCK_ID_PERIPH, PLL_OUT4, ULPI_REF_CLK);
> +
> +	/* reset ULPI phy */
> +	if (fdt_gpio_isvalid(&config->phy_reset_gpio)) {
> +		fdtdec_setup_gpio(&config->phy_reset_gpio);
> +		gpio_direction_output(config->phy_reset_gpio.gpio, 0);
> +		mdelay(5);
> +		gpio_set_value(config->phy_reset_gpio.gpio, 1);
> +	}
> +
> +	/* Reset the usb controller */
> +	clock_enable(config->periph_id);
> +	usbf_reset_controller(config, usbctlr);
> +
> +	/* enable pinmux bypass */
> +	setbits_le32(&usbctlr->ulpi_timing_ctrl_0,
> +				ULPI_CLKOUT_PINMUX_BYP | ULPI_OUTPUT_PINMUX_BYP);
> +
> +	/* Select ULPI parallel interface */
> +	clrsetbits_le32(&usbctlr->port_sc1, PTS_MASK, PTS_ULPI << PTS_SHIFT);
> +
> +	/* enable ULPI transceiver */
> +	setbits_le32(&usbctlr->susp_ctrl, ULPI_PHY_ENB);
> +
> +	/* configure ULPI transceiver timings */
> +	val = 0;
> +	writel(val, &usbctlr->ulpi_timing_ctrl_1);
> +
> +	val |= ULPI_DATA_TRIMMER_SEL(4);
> +	val |= ULPI_STPDIRNXT_TRIMMER_SEL(4);
> +	val |= ULPI_DIR_TRIMMER_SEL(4);
> +	writel(val, &usbctlr->ulpi_timing_ctrl_1);
> +	udelay(10);
> +
> +	val |= ULPI_DATA_TRIMMER_LOAD;
> +	val |= ULPI_STPDIRNXT_TRIMMER_LOAD;
> +	val |= ULPI_DIR_TRIMMER_LOAD;
> +	writel(val, &usbctlr->ulpi_timing_ctrl_1);
> +
> +	/* set up phy for host operation with external vbus supply */
> +	ulpi_vp.port_num = 0;
> +	ulpi_vp.viewport_addr = (u32)&usbctlr->ulpi_viewport;
> +
> +	if (ulpi_init(&ulpi_vp)) {
> +		printf("Tegra ULPI viewport init failed\n");
> +		return -1;
> +	}
> +
> +	ulpi_set_vbus(&ulpi_vp, 1, 1);
> +	ulpi_set_vbus_indicator(&ulpi_vp, 1, 1, 0);
> +
> +	/* enable wakeup events */
> +	setbits_le32(&usbctlr->port_sc1, WKCN | WKDS | WKOC);
> +
> +	setbits_le32(&usbctlr->susp_ctrl, USB_SUSP_CLR);
> +	/* Wait for the phy clock to become valid in 100 ms */
> +	for (loop_count = 100000; loop_count != 0; loop_count--) {
> +		if (readl(&usbctlr->susp_ctrl) & USB_PHY_CLK_VALID)
> +			break;
> +		udelay(1);
> +	}

an empty line here would be nice.

> +	if (!loop_count)
> +		return -1;

and here.

> +	clrbits_le32(&usbctlr->susp_ctrl, USB_SUSP_CLR);
> +
> +	return 0;
> +#else
> +	printf("No code to set up ULPI controller, please enable"
> +			"CONFIG_USB_ULPI and CONFIG_USB_ULPI_VIEWPORT");
> +	return -1;
> +#endif
>  }
>  
>  static void config_clock(const u32 timing[])
> @@ -327,24 +430,25 @@ static int add_port(struct fdt_usb *config, const u32 timing[])
>  	struct usb_ctlr *usbctlr = config->reg;
>  
>  	if (port_count == USB_PORTS_MAX) {
> -		debug("tegrausb: Cannot register more than %d ports\n",
> +		printf("tegrausb: Cannot register more than %d ports\n",
>  		      USB_PORTS_MAX);
>  		return -1;
>  	}
> -	if (init_usb_controller(config, usbctlr, timing)) {
> -		debug("tegrausb: Cannot init port\n");
> -		return -1;
> -	}
> +
>  	if (config->utmi) {
> -		/* Disable ICUSB FS/LS transceiver */
> -		clrbits_le32(&usbctlr->icusb_ctrl, IC_ENB1);
> -
> -		/* Select UTMI parallel interface */
> -		clrsetbits_le32(&usbctlr->port_sc1, PTS_MASK,
> -				PTS_UTMI << PTS_SHIFT);
> -		clrbits_le32(&usbctlr->port_sc1, STS);
> -		power_up_port(usbctlr);
> +		if (init_utmi_usb_controller(config, usbctlr, timing)) {
> +			printf("tegrausb: Cannot init port\n");
> +			return -1;
> +		}
> +	}
> +
> +	if (config->ulpi) {
> +		if (init_ulpi_usb_controller(config, usbctlr)) {
> +			printf("tegrausb: Cannot init port\n");
> +			return -1;
> +		}
>  	}

here you can use:

	if (config->ulpi && init_ulpi_usb_controller(config, usbctlr)) {
		printf("tegrausb: Cannot init port\n");
		return -1;
	}

and spare one indentation level, but I don't really insist...

> +
>  	port[port_count++] = *config;
>  
>  	return 0;

[...]


-- 
Regards,
Igor.

[U-Boot] [PATCH v2 4/5] usb: ulpi: add indicator configuration function

[Marek Vasut]

Dear Igor Grinberg,

> Hi Lucas, Tom,
> 
> I'm sorry for the late reply.
> I understand, that Tom has already applied this to tegra/next,
> but as the changes/follow up patches are required,
> may be we can do this in another fashion...
> 
> 1) Thanks for the patch and working on extending the generic framework!
> 2) This patch has no dependencies on tegra specific patches, so
>    I think, it should go through Marex usb tree, but doing this will
>    require the right merge order, so bisectability will not suffer.
>    So, Marek, Tom, you should decide which way is fine with you both.

_ALWAYS_ CC the right custodians. That is, me. Seeing this patch bypassed me 
completely, I'm really unhappy.

> 
> Tom,
> Yesterday, I was wondering if the patch was already applied, and
> I had no clue what's its status. Also, the patchwork says "New".
> So, if it is not hard for you in the future, I'd like a short reply
> to the list, saying something like: "Applied, thanks.", like most
> custodians do. Thanks!

+1

> On 08/21/12 23:18, Lucas Stach wrote:
> > Allows for easy configuration of the VBUS indicator related ULPI
> > config bits.
> > 
> > Also move the external indicator setup from ulpi_set_vbus() to
> > the new function.
> > 
> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> 
> After the below comments are fixed:
> Acked-by: Igor Grinberg <grinberg@compulab.co.il>
> 
> > ---
> > 
> >  drivers/usb/ulpi/ulpi.c | 26 ++++++++++++++++++++++----
> >  include/usb/ulpi.h      | 13 +++++++++++--
> >  2 Dateien ge?ndert, 33 Zeilen hinzugef?gt(+), 6 Zeilen entfernt(-)
> > 
> > diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c
> > index dde2585..f358bde 100644
> > --- a/drivers/usb/ulpi/ulpi.c
> > +++ b/drivers/usb/ulpi/ulpi.c
> > @@ -106,20 +106,38 @@ int ulpi_select_transceiver(struct ulpi_viewport
> > *ulpi_vp, unsigned speed)
> > 
> >  	return ulpi_write(ulpi_vp, &ulpi->function_ctrl, val);
> >  
> >  }
> > 
> > -int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power,
> > -			int ext_ind)
> > +int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power)
> > 
> >  {
> >  
> >  	u32 flags = ULPI_OTG_DRVVBUS;
> >  	u8 *reg = on ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
> >  	
> >  	if (ext_power)
> >  	
> >  		flags |= ULPI_OTG_DRVVBUS_EXT;
> > 
> > -	if (ext_ind)
> > -		flags |= ULPI_OTG_EXTVBUSIND;
> > 
> >  	return ulpi_write(ulpi_vp, reg, flags);
> >  
> >  }
> > 
> > +int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int external,
> > +			int passthu, int complement)
> > +{
> > +	u8 *reg;
> > +	int ret;
> > +
> > +	reg = external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
> > +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND)))
> > +		return ret;
> > +
> > +	reg = passthu ? &ulpi->iface_ctrl_set : &ulpi->iface_ctrl_clear;
> > +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_PASSTHRU)))
> > +		return ret;
> > +
> > +	reg = complement ? &ulpi->iface_ctrl_set : &ulpi->iface_ctrl_clear;
> > +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_EXTVBUS_COMPLEMENT)))
> > +		return ret;
> 
> These are fine, two requests though:
> 1) As Tom already pointed in the private email:
> ERROR: do not use assignment in if condition
> #361: FILE: drivers/usb/ulpi/ulpi.c:127:
> +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND)))
> 
> ERROR: do not use assignment in if condition
> #365: FILE: drivers/usb/ulpi/ulpi.c:131:
> +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_PASSTHRU)))
> 
> ERROR: do not use assignment in if condition
> #369: FILE: drivers/usb/ulpi/ulpi.c:135:
> +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_EXTVBUS_COMPLEMENT)))
> 
> those must be fixed.

Agreed, _ALWAYS_ run checkpatch.pl before submitting. It's even better idea to 
add a git precommit hook for that.

> 2) Can you make only one access for each register?
>    Use flags/val variable (like in other places) and
>    do only one access per register. Can you?
> 
> > +
> > +	return 0;
> > +}
> > +
> > 
> >  int ulpi_set_pd(struct ulpi_viewport *ulpi_vp, int enable)
> >  {
> >  
> >  	u32 val = ULPI_OTG_DP_PULLDOWN | ULPI_OTG_DM_PULLDOWN;
> > 
> > diff --git a/include/usb/ulpi.h b/include/usb/ulpi.h
> > index 9a75c24..99166c4 100644
> > --- a/include/usb/ulpi.h
> > +++ b/include/usb/ulpi.h
> > @@ -61,8 +61,17 @@ int ulpi_select_transceiver(struct ulpi_viewport
> > *ulpi_vp, unsigned speed);
> > 
> >   *
> >   * returns 0 on success, ULPI_ERROR on failure.
> >   */
> > 
> > -int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp,
> > -			int on, int ext_power, int ext_ind);
> > +int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power);
> > +
> > +/*
> > + * Configure VBUS indicator
> > + * @external		- external VBUS over-current indicator is used
> > + * @passthru		- disables ANDing of internal VBUS comparator
> > + *                    with external VBUS input
> > + * @complement		- inverts the external VBUS input
> > + */
> > +int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int external,
> > +			int passthru, int complement);
> > 
> >  /*
> >  
> >   * Enable/disable pull-down resistors on D+ and D- USB lines.

[U-Boot] [PATCH v2 4/5] usb: ulpi: add indicator configuration function

[Tom Warren]

Igor/Marek,

> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Wednesday, September 05, 2012 1:52 AM
> To: Igor Grinberg
> Cc: Lucas Stach; u-boot at lists.denx.de; Stephen Warren; Tom Warren
> Subject: Re: [PATCH v2 4/5] usb: ulpi: add indicator configuration function
> 
> Dear Igor Grinberg,
> 
> > Hi Lucas, Tom,
> >
> > I'm sorry for the late reply.
> > I understand, that Tom has already applied this to tegra/next, but as
> > the changes/follow up patches are required, may be we can do this in
> > another fashion...
> >
> > 1) Thanks for the patch and working on extending the generic framework!
> > 2) This patch has no dependencies on tegra specific patches, so
> >    I think, it should go through Marex usb tree, but doing this will
> >    require the right merge order, so bisectability will not suffer.
> >    So, Marek, Tom, you should decide which way is fine with you both.

I'm not sure how the USB and Tegra repos can coordinate on patches like this, since I don't pull from/rebase against USB, and AFAIK Marek doesn't reference Tegra when he updates his repo. I'm a sub-repo of ARM, which is a sub-repo of TOT (u-boot/master). What I usually do (and have always done) is to take the entire patchset that includes a Tegra component (USB, mmc, SPI, etc.) and hope (pray?) that anyone merging my changes upstream of me will be able to resolve the conflicts/pre-existing patches. So far, I haven't heard from anyone (Albert or Wolfgang) that's had a problem with that, perhaps because it's pretty rare. AFAICT, there's no other procedure outlined in the U-Boot wiki custodian's page.  If there's a better procedure I should be following, let's get it documented and I'll be glad to hew to the line. I'm still on the learning curve for git merging, rebasing, etc.

> 
> _ALWAYS_ CC the right custodians. That is, me. Seeing this patch bypassed me
> completely, I'm really unhappy.
> 
> >
> > Tom,
> > Yesterday, I was wondering if the patch was already applied, and I had
> > no clue what's its status. Also, the patchwork says "New".
> > So, if it is not hard for you in the future, I'd like a short reply to
> > the list, saying something like: "Applied, thanks.", like most
> > custodians do. Thanks!
> 
> +1
> 

I _do_ owe the list, and the patch's author(s) an 'applied' message, as Igor points out. I've been viewing u-boot-tegra/next as just a staging area for patches that'll eventually go into master, and I've been copying patch authors and Tegra devs on pull requests, but I'll also send out a notice in the future when I apply a patch to /next.

Thanks,

Tom
> > On 08/21/12 23:18, Lucas Stach wrote:
> > > Allows for easy configuration of the VBUS indicator related ULPI
> > > config bits.
> > >
> > > Also move the external indicator setup from ulpi_set_vbus() to the
> > > new function.
> > >
> > > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> >
> > After the below comments are fixed:
> > Acked-by: Igor Grinberg <grinberg@compulab.co.il>
> >
> > > ---
> > >
> > >  drivers/usb/ulpi/ulpi.c | 26 ++++++++++++++++++++++----
> > >  include/usb/ulpi.h      | 13 +++++++++++--
> > >  2 Dateien ge?ndert, 33 Zeilen hinzugef?gt(+), 6 Zeilen entfernt(-)
> > >
> > > diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c index
> > > dde2585..f358bde 100644
> > > --- a/drivers/usb/ulpi/ulpi.c
> > > +++ b/drivers/usb/ulpi/ulpi.c
> > > @@ -106,20 +106,38 @@ int ulpi_select_transceiver(struct
> > > ulpi_viewport *ulpi_vp, unsigned speed)
> > >
> > >  	return ulpi_write(ulpi_vp, &ulpi->function_ctrl, val);
> > >
> > >  }
> > >
> > > -int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power,
> > > -			int ext_ind)
> > > +int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int
> > > +ext_power)
> > >
> > >  {
> > >
> > >  	u32 flags = ULPI_OTG_DRVVBUS;
> > >  	u8 *reg = on ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
> > >
> > >  	if (ext_power)
> > >
> > >  		flags |= ULPI_OTG_DRVVBUS_EXT;
> > >
> > > -	if (ext_ind)
> > > -		flags |= ULPI_OTG_EXTVBUSIND;
> > >
> > >  	return ulpi_write(ulpi_vp, reg, flags);
> > >
> > >  }
> > >
> > > +int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int
> external,
> > > +			int passthu, int complement)
> > > +{
> > > +	u8 *reg;
> > > +	int ret;
> > > +
> > > +	reg = external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
> > > +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND)))
> > > +		return ret;
> > > +
> > > +	reg = passthu ? &ulpi->iface_ctrl_set : &ulpi->iface_ctrl_clear;
> > > +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_PASSTHRU)))
> > > +		return ret;
> > > +
> > > +	reg = complement ? &ulpi->iface_ctrl_set : &ulpi->iface_ctrl_clear;
> > > +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_EXTVBUS_COMPLEMENT)))
> > > +		return ret;
> >
> > These are fine, two requests though:
> > 1) As Tom already pointed in the private email:
> > ERROR: do not use assignment in if condition
> > #361: FILE: drivers/usb/ulpi/ulpi.c:127:
> > +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND)))
> >
> > ERROR: do not use assignment in if condition
> > #365: FILE: drivers/usb/ulpi/ulpi.c:131:
> > +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_PASSTHRU)))
> >
> > ERROR: do not use assignment in if condition
> > #369: FILE: drivers/usb/ulpi/ulpi.c:135:
> > +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_EXTVBUS_COMPLEMENT)))
> >
> > those must be fixed.
> 
> Agreed, _ALWAYS_ run checkpatch.pl before submitting. It's even better idea
> to add a git precommit hook for that.
> 
> > 2) Can you make only one access for each register?
> >    Use flags/val variable (like in other places) and
> >    do only one access per register. Can you?
> >
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >
> > >  int ulpi_set_pd(struct ulpi_viewport *ulpi_vp, int enable)  {
> > >
> > >  	u32 val = ULPI_OTG_DP_PULLDOWN | ULPI_OTG_DM_PULLDOWN;
> > >
> > > diff --git a/include/usb/ulpi.h b/include/usb/ulpi.h index
> > > 9a75c24..99166c4 100644
> > > --- a/include/usb/ulpi.h
> > > +++ b/include/usb/ulpi.h
> > > @@ -61,8 +61,17 @@ int ulpi_select_transceiver(struct ulpi_viewport
> > > *ulpi_vp, unsigned speed);
> > >
> > >   *
> > >   * returns 0 on success, ULPI_ERROR on failure.
> > >   */
> > >
> > > -int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp,
> > > -			int on, int ext_power, int ext_ind);
> > > +int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int
> > > +ext_power);
> > > +
> > > +/*
> > > + * Configure VBUS indicator
> > > + * @external		- external VBUS over-current indicator is used
> > > + * @passthru		- disables ANDing of internal VBUS comparator
> > > + *                    with external VBUS input
> > > + * @complement		- inverts the external VBUS input
> > > + */
> > > +int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int
> external,
> > > +			int passthru, int complement);
> > >
> > >  /*
> > >
> > >   * Enable/disable pull-down resistors on D+ and D- USB lines.
-- 
nvpublic

[U-Boot] [PATCH v2 4/5] usb: ulpi: add indicator configuration function

[Lucas Stach]

Hi Tom,

Am Mittwoch, den 05.09.2012, 09:25 -0700 schrieb Tom Warren:
> Igor/Marek,
> 
> > -----Original Message-----
> > From: Marek Vasut [mailto:marex at denx.de]
> > Sent: Wednesday, September 05, 2012 1:52 AM
> > To: Igor Grinberg
> > Cc: Lucas Stach; u-boot at lists.denx.de; Stephen Warren; Tom Warren
> > Subject: Re: [PATCH v2 4/5] usb: ulpi: add indicator configuration function
> > 
> > Dear Igor Grinberg,
> > 
> > > Hi Lucas, Tom,
> > >
> > > I'm sorry for the late reply.
> > > I understand, that Tom has already applied this to tegra/next, but as
> > > the changes/follow up patches are required, may be we can do this in
> > > another fashion...
> > >
> > > 1) Thanks for the patch and working on extending the generic framework!
> > > 2) This patch has no dependencies on tegra specific patches, so
> > >    I think, it should go through Marex usb tree, but doing this will
> > >    require the right merge order, so bisectability will not suffer.
> > >    So, Marek, Tom, you should decide which way is fine with you both.
> 
> I'm not sure how the USB and Tegra repos can coordinate on patches like this, since I don't pull from/rebase against USB, and AFAIK Marek doesn't reference Tegra when he updates his repo. I'm a sub-repo of ARM, which is a sub-repo of TOT (u-boot/master). What I usually do (and have always done) is to take the entire patchset that includes a Tegra component (USB, mmc, SPI, etc.) and hope (pray?) that anyone merging my changes upstream of me will be able to resolve the conflicts/pre-existing patches. So far, I haven't heard from anyone (Albert or Wolfgang) that's had a problem with that, perhaps because it's pretty rare. AFAICT, there's no other procedure outlined in the U-Boot wiki custodian's page.  If there's a better procedure I should be following, let's get it documented and I'll be glad to hew to the line. I'm still on the learning curve for git merging, rebasing, etc.
> 
I thought about how we could merge all this without loosing our sanity.
I've already wrote this a bit hidden in a reply to the multi controller
thread: I think it's best to handle all USB related changes through the
u-boot-usb tree, as all this stuff should really be under drivers/usb.

This means: I'll split out the clock output related changes, so they can
go in the Tegra tree. Everything touching USB goes into the u-boot-usb
tree and I'll rebase my changes accordingly. This also means commit "dm:
Tegra: Staticize local functions" should be removed from the Tegra tree
and move over to the USB tree.

This way we won't get any build breakages and there should be no merge
conflicts. It also opens the possibility to move the Tegra USB
implementation to the right location in the source tree a bit later in
this cycle, without messing up the merge.

Thanks,
Lucas

[U-Boot] [PATCH v2 4/5] usb: ulpi: add indicator configuration function

[Marek Vasut]

Dear Lucas Stach,

> Hi Tom,
> 
> Am Mittwoch, den 05.09.2012, 09:25 -0700 schrieb Tom Warren:
> > Igor/Marek,
> > 
> > > -----Original Message-----
> > > From: Marek Vasut [mailto:marex at denx.de]
> > > Sent: Wednesday, September 05, 2012 1:52 AM
> > > To: Igor Grinberg
> > > Cc: Lucas Stach; u-boot at lists.denx.de; Stephen Warren; Tom Warren
> > > Subject: Re: [PATCH v2 4/5] usb: ulpi: add indicator configuration
> > > function
> > > 
> > > Dear Igor Grinberg,
> > > 
> > > > Hi Lucas, Tom,
> > > > 
> > > > I'm sorry for the late reply.
> > > > I understand, that Tom has already applied this to tegra/next, but as
> > > > the changes/follow up patches are required, may be we can do this in
> > > > another fashion...
> > > > 
> > > > 1) Thanks for the patch and working on extending the generic
> > > > framework! 2) This patch has no dependencies on tegra specific
> > > > patches, so
> > > > 
> > > >    I think, it should go through Marex usb tree, but doing this will
> > > >    require the right merge order, so bisectability will not suffer.
> > > >    So, Marek, Tom, you should decide which way is fine with you both.
> > 
> > I'm not sure how the USB and Tegra repos can coordinate on patches like
> > this, since I don't pull from/rebase against USB, and AFAIK Marek
> > doesn't reference Tegra when he updates his repo. I'm a sub-repo of ARM,
> > which is a sub-repo of TOT (u-boot/master). What I usually do (and have
> > always done) is to take the entire patchset that includes a Tegra
> > component (USB, mmc, SPI, etc.) and hope (pray?) that anyone merging my
> > changes upstream of me will be able to resolve the
> > conflicts/pre-existing patches. So far, I haven't heard from anyone
> > (Albert or Wolfgang) that's had a problem with that, perhaps because
> > it's pretty rare. AFAICT, there's no other procedure outlined in the
> > U-Boot wiki custodian's page.  If there's a better procedure I should be
> > following, let's get it documented and I'll be glad to hew to the line.
> > I'm still on the learning curve for git merging, rebasing, etc.
> 
> I thought about how we could merge all this without loosing our sanity.
> I've already wrote this a bit hidden in a reply to the multi controller
> thread: I think it's best to handle all USB related changes through the
> u-boot-usb tree, as all this stuff should really be under drivers/usb.

Let's extend this a bit. Since I'm really under heavy load now, why don't you 
prepare a patchset (that includes all the tegra goo, stuff that Tom will drop 
etc) that I can pick, submit it (and Cc me) and I'll just pick it all ? That way 
we'll have a proper ordering and nothing will be lost and all will be tested.

> This means: I'll split out the clock output related changes, so they can
> go in the Tegra tree. Everything touching USB goes into the u-boot-usb
> tree and I'll rebase my changes accordingly. This also means commit "dm:
> Tegra: Staticize local functions" should be removed from the Tegra tree
> and move over to the USB tree.

Drop the dm: from it along the way.

> This way we won't get any build breakages and there should be no merge
> conflicts. It also opens the possibility to move the Tegra USB
> implementation to the right location in the source tree a bit later in
> this cycle, without messing up the merge.
> 
> Thanks,
> Lucas

Best regards,
Marek Vasut