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:
- you put the above inside the init_ulpi_usb_controller() function
- 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.