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

[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?

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

> +	#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?

> +		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?

> +	ulpi_write(&ulpi_vp, &ulpi_reg->otg_ctrl_set, ULPI_OTG_EXTVBUSIND);

Can ulpi_set_vbus(&ulpi_vp, 1, 1, 1) be used here?

> +
> +	/* 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.

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.