From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Mon, 20 Aug 2012 12:27:04 -0600
Subject: [U-Boot] [PATCH 3/3] tegra20: add USB ULPI init code
In-Reply-To: <1345466486.12175.32.camel@selen>
References: <1345392496-28739-1-git-send-email-dev@lynxeye.de>
	<1345392496-28739-4-git-send-email-dev@lynxeye.de>
	<50322888.4080404@compulab.co.il> <1345466486.12175.32.camel@selen>
Message-ID: <50328178.9010608@wwwdotorg.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 08/20/2012 06:41 AM, 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.

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

Moving all the initialization into init_utmi_usb_controller() and
init_ulpi_usb_controller() sounds reasonable to me.

I imagine that the reference frequency is somewhat driven by the
requirements of USB itself and/or the ULPI interface. I think it's fine
to just hard-code that in the USB driver for now; we can easily enhance
the driver to make it configurable from either DT or U-Boot config file
in the future if we need.