[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Stephen Warren <swarren@nvidia.com>]

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

> +/* 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()?

> +#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)?

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

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

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

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


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

> +               /* The first port we find gets to set the clocks */

Ick.

> +       /* 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.

-- 
nvpublic