Hi Stephen, Here are my comments on the rest of your email.
On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swar...@nvidia.com> wrote: > On 11/23/2011 08:54 PM, Simon Glass wrote: >> This adds basic support for the Tegra2 USB controller. Board files should >> call board_usb_init() to set things up. > > Just a very brief review: > >> +/* Put the port into host mode (this only works for USB1) */ >> +static void set_host_mode(struct usb_ctlr *usbctlr) >> +{ >> + /* Check whether remote host from USB1 is driving VBus */ >> + if (readl(&usbctlr->phy_vbus_sensors) & VBUS_VLD_STS) >> + return; >> + >> + /* >> + * If not driving, we set GPIO USB1_VBus_En. Seaboard platform uses >> + * PAD SLXK (GPIO D.00) as USB1_VBus_En Config as GPIO >> + */ >> + gpio_direction_output(GPIO_PD0, 1); > > The GPIO to use here needs to be parameterized; there's no reason to > believe it'll be the same on all boards (or even that boards will have > any such GPIO). I have done this. It is tricky since so far we have no way of setting up the pinmux for a given GPIO. Rather than bite off that camel I have just set up the pinmux in the board file. > >> +/* Put our ports into host mode */ >> +void usb_set_host_mode(void) >> +{ >> + if (host_dev_ctlr) >> + set_host_mode(host_dev_ctlr); >> +} > > Why not just call set_host_mode() directly from board_usb_init()? This function is exported, and the caller doesn't have a pointer to the host/device-switchable port. > >> +#ifndef CONFIG_OF_CONTROL >> +static int probe_port(struct usb_ctlr *usbctlr, const int params[]) >> +{ >> + enum periph_id id; >> + int utmi = 0; >> + >> + /* >> + * Get the periph id. Port 1 has an internal transceiver, port 3 is >> + * external >> + */ >> + switch ((u32)usbctlr) { >> + case TEGRA_USB1_BASE: >> + id = PERIPH_ID_USBD; >> + break; >> + >> + case TEGRA_USB3_BASE: >> + id = PERIPH_ID_USB3; >> + utmi = 1; >> + break; >> + >> + default: >> + printf("tegrausb: probe_port: no such port %p\n", usbctlr); >> + return -1; >> + } > > What about the other port (USB2)? I have removed this function. > >> +#ifdef CONFIG_OF_CONTROL >> +int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz, >> + struct fdt_usb *config) >> +{ >> + int clk_node = 0, rate; >> + >> + /* Find the parameters for our oscillator frequency */ >> + do { >> + clk_node = fdt_node_offset_by_compatible(blob, clk_node, >> + "nvidia,tegra20-usbparams"); >> + if (clk_node < 0) { >> + debug("Cannot find USB params for clock %u", >> + osc_frequency_mhz); >> + return -FDT_ERR_NOTFOUND; >> + } >> + rate = fdtdec_get_int(blob, clk_node, "osc-frequency", 0); >> + } while (rate != osc_frequency_mhz); >> + >> + config->reg = (struct usb_ctlr *)fdtdec_get_addr(blob, node, "reg"); >> + config->host_mode = fdtdec_get_bool(blob, node, "support-host-mode"); > > Property "support-host-mode" isn't something that's supported by the > kernel binding, and needs discussion/ack there. OK will await comments. The kernel bindings seem very basic at this stage. > >> + config->utmi = fdtdec_lookup_phandle(blob, node, "utmi") >= 0; > > In the kernel DT binding, this is property "phy_type" with legal values > "utmi" or "ulpi." OK I have changed this. > >> + config->enabled = fdtdec_get_is_enabled(blob, node); >> + config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1); > > periph-id is a U-Boot specific concept, not HW description. The DT > shouldn't contain that value. See my previous comments as to why this is desirable. We can change over to a clock-based approach once the kernel implements it. > >> + if (config->periph_id == -1) >> + return -FDT_ERR_NOTFOUND; >> + >> + return fdtdec_get_int_array(blob, clk_node, "params", config->params, >> + PARAM_COUNT); > > Property "params" (which should probably be named something better > anyway) isn't something that's supported by the kernel binding, and > needs discussion/ack there. Instead, I suggest following the kernel's > approach for now - don't specify these PHY parameters in the binding, > but rather just apply the defaults, which are apparently suitable for > all the boards supported by the mainline kernel at least. As discussed I have changed the name but otherwise left this as is. since you mentioned that some boards may need to change the parameters (it seems Tegra3 is different also). > > >> +int board_usb_init(const void *blob) >> +{ >> +#ifdef CONFIG_OF_CONTROL >> + struct fdt_usb config; >> + int clk_done = 0; >> + int node, upto = 0; >> + unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC); >> + >> + do { >> + node = fdtdec_next_alias(blob, "usb", >> + COMPAT_NVIDIA_TEGRA20_USB, &upto); > > Why only initialize USB controllers with aliases? Surely this should > enumerate all nodes with a specific compatible flag? See my other comments - we want to know that port 0 is USB3 on Seaboard. > >> + /* The first port we find gets to set the clocks */ > > Ick. Well they all uses the same params anyway. It could be refactored to read the timing separately but it is convenient to have it in one structure. > >> + /* Set up our two ports */ >> +#ifdef CONFIG_TEGRA_USB1_HOST >> + host_dev_ctlr = (struct usb_ctlr *)TEGRA_USB1_BASE; >> +#endif > > That port is always the host/device switchable controller. Why not > always make that assignment? The issue here is probably that > set_host_mode() isn't suitable for all boards. The solution seems to be > to fix that. I have removed this code. Regards, Simon > > -- > nvpublic > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot