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

Lucas Stach

21 Aug 2012 21 Aug '12

10:18 p.m.

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

Show replies by date

Lucas Stach

21 Aug 21 Aug

10:18 p.m.

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

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

Lucas Stach

10:18 p.m.

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

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

Lucas Stach

10:18 p.m.

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

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

Lucas Stach

10:18 p.m.

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

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

Igor Grinberg

5 Sep 5 Sep

9:52 a.m.

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

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.

Marek Vasut

10:51 a.m.

New subject: [U-Boot] [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...

Thanks for the patch and working on extending the generic framework!

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!

...

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:

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.

...

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.

Tom Warren

6:25 p.m.

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

Igor/Marek,

...

-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Wednesday, September 05, 2012 1:52 AM To: Igor Grinberg Cc: Lucas Stach; u-boot@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...

Thanks for the patch and working on extending the generic framework!

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:

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.

...
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

Lucas Stach

6 Sep 6 Sep

2:06 a.m.

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

Hi Tom,

Am Mittwoch, den 05.09.2012, 09:25 -0700 schrieb Tom Warren:

...

Igor/Marek,

...
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Wednesday, September 05, 2012 1:52 AM To: Igor Grinberg Cc: Lucas Stach; u-boot@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...

Thanks for the patch and working on extending the generic framework!

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

Marek Vasut

7 Sep 7 Sep

2:11 a.m.

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

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@denx.de] Sent: Wednesday, September 05, 2012 1:52 AM To: Igor Grinberg Cc: Lucas Stach; u-boot@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...

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

Lucas Stach

21 Aug 21 Aug

10:18 p.m.

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

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

Igor Grinberg

5 Sep 5 Sep

10:22 a.m.

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

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.

Stephen Warren

23 Aug 23 Aug

8:42 p.m.

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:-)

4627

Age (days ago)

4644

Last active (days ago)

List overview

Download

12 comments

5 participants

tags (0)

participants (5)

Igor Grinberg
Lucas Stach
Marek Vasut
Stephen Warren
Tom Warren