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

[Igor Grinberg]

On 08/20/12 21:27, Stephen Warren wrote:
> 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.

Agreed completely.

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

Well, it can also be both the controller and the ULPI PHY,
but I agree, you can hard code it for now or make something like:

#define DEFAULT_ULPI_REF_CLK 24000000
#ifndef ULPI_REF_CLK
#define ULPI_REF_CLK DEFAULT_ULPI_REF_CLK
#endif

and use the ULPI_REF_CLK where appropriate.

-- 
Regards,
Igor.