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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
