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

[Igor Grinberg]

On 08/20/12 15:41, Lucas Stach wrote:
> Hello Igor,
> 
> thanks for your review. Comments inline.
> 
> Am Montag, den 20.08.2012, 15:07 +0300 schrieb Igor Grinberg:
>> Hi Lucas,
>>
>> On 08/19/12 19:08, 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.
>>
>> Can you share which tweaks for correctness are there?
>>
> Linux code does not actually check if the phy clock comes up correctly,
> it just waits 100ms and hopes it is there. Also we explicitly select the
> ULPI interface, although it also works without this, as ULPI seems to be
> the default state for USB2.

Ok. Thanks.

> 
>>>
>>> To use this both CONFIG_USB_ULPI and CONFIG_USB_ULPI_VIEWPORT
>>> have to be set in the board configuration file.
>>>
>>> Signed-off-by: Lucas Stach <dev at lynxeye.de>
>>> ---
>>>  arch/arm/cpu/armv7/tegra20/usb.c        | 131 +++++++++++++++++++++++++++++---
>>>  arch/arm/include/asm/arch-tegra20/usb.h |  29 +++++--
>>>  2 Dateien geändert, 145 Zeilen hinzugefügt(+), 15 Zeilen entfernt(-)
>>>
>>> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
>>> index 77966e5..2ae1244 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"
>>
>> there is a mix of tabs and spaces in the above line, please make it only tabs
> 
> This was actually intentional to align the two lines exactly.

I see. Well, in U-Boot we have had huge threads discussing
how the alignment should be done and the conclusion was that only
tabs should be used (even if it means that the starting letters
will not be one below the other).
Now in the above particular case, you can place a tab between the
"#error" and the message - this will assure the wanted alignment
of both lines.

> 
>>> +	#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;
>>> @@ -300,6 +310,83 @@ static int init_usb_controller(struct fdt_usb *config,
>>>  	return 0;
>>>  }
>>>  
>>> +#ifdef CONFIG_USB_ULPI
>>> +/* set up the ULPI USB controller with the parameters provided */
>>> +static int init_ulpi_usb_controller(struct fdt_usb *config,
>>> +                                    struct usb_ctlr *usbctlr)
>>> +{
>>> +	u32 val;
>>> +	int loop_count;
>>> +	struct ulpi_regs *ulpi_reg = (struct ulpi_regs *)0;
>>> +	struct ulpi_viewport ulpi_vp;
>>> +
>>> +	/* 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)) {
>>> +		debug("Tegra ULPI viewport init failed\n");
>>
>> This looks like an error, right?
>> So why hide it under debug?
>>
> Yes, you're right. I'll fix that.
> 
>>> +		return -1;
>>> +	}
>>> +
>>> +	ulpi_write(&ulpi_vp, &ulpi_reg->iface_ctrl_set, ULPI_IFACE_PASSTHRU);
>>
>> The implementation for the indicator setup is currently missing
>> from the generic framework.
>> Have you thought about adding it?
>>
> Yep, only looked into the framework provided functions after writing
> this patch. I'll do a patch for this in the next series iteration.

Thanks!

> 
>>> +	ulpi_write(&ulpi_vp, &ulpi_reg->otg_ctrl_set, ULPI_OTG_EXTVBUSIND);
>>
>> Can ulpi_set_vbus(&ulpi_vp, 1, 1, 1) be used here?
>>
> Yes, I already changed this locally.
> 
>>> +
>>> +	/* 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;
>>> +}
>>> +#endif
>>> +
>>>  static void power_up_port(struct usb_ctlr *usbctlr)
>>>  {
>>>  	/* Deassert power down state */
>>> @@ -331,11 +418,13 @@ static int add_port(struct fdt_usb *config, const u32 timing[])
>>>  		      USB_PORTS_MAX);
>>>  		return -1;
>>>  	}
>>> -	if (init_usb_controller(config, usbctlr, timing)) {
>>> -		debug("tegrausb: Cannot init port\n");
>>> -		return -1;
>>> -	}
>>> +
>>>  	if (config->utmi) {
>>> +		if (init_utmi_usb_controller(config, usbctlr, timing)) {
>>> +			debug("tegrausb: Cannot init port\n");
>>
>> This also looks like an error...
>> So why debug()?
>>
>>> +			return -1;
>>> +		}
>>> +
>>>  		/* Disable ICUSB FS/LS transceiver */
>>>  		clrbits_le32(&usbctlr->icusb_ctrl, IC_ENB1);
>>>  
>>> @@ -345,6 +434,24 @@ static int add_port(struct fdt_usb *config, const u32 timing[])
>>>  		clrbits_le32(&usbctlr->port_sc1, STS);
>>>  		power_up_port(usbctlr);
>>>  	}
>>> +
>>> +	if (config->ulpi) {
>>> +#ifdef CONFIG_USB_ULPI
>>> +		/* set up 24MHz ULPI reference clock on pllp_out4 */
>>> +		clock_enable(PERIPH_ID_DEV2_OUT);
>>> +		clock_set_pllout(CLOCK_ID_PERIPH, PLL_OUT4, 24000000);
>>
>> Wouldn't it be clearer if:
>> 1) you put the above inside the init_ulpi_usb_controller() function
>> 2) Provide a !CONFIG_USB_ULPI implementation of the same function
>>    technically having only the code under #else below inside.
>>
> Actually I'm not really sure what to do about this. Although I've not
> seen any Tegra boards with a other ULPI reference freq used, maybe we
> should just move the clock setup into board code or add a device tree
> entry to tell the ref frequency.
> 
> Stephen, Tom, any ideas?
> 
>> So then here instead of the whole #ifdef, you will have something like:
>> if (config->ulpi) {
>> 	if (init_ulpi_usb_controller(config, usbctlr)) {
>> 		printf("tegrausb: Cannot init port\n");
>> 		return -1;
>> 	}
>> }
> 
>>> +
>>> +		if (init_ulpi_usb_controller(config, usbctlr)) {
>>> +			debug("tegrausb: Cannot init port\n");
>>> +			return -1;
>>> +		}
>>> +#else
>>> +		debug("No code to set up ULPI controller, please enable"
>>> +		      "CONFIG_USB_ULPI and CONFIG_USB_ULPI_VIEWPORT");
>>> +		return -1;
>>> +#endif
>>> +	}
>>> +
>>>  	port[port_count++] = *config;
>>>  
>>>  	return 0;
>>
>> [...]
>>
>>
>>
> 
> 
> 

-- 
Regards,
Igor.