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

[Igor Grinberg <grinberg@compulab.co.il>]

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.