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

[Simon Glass]

Hi Stephen,

Here are my comments on the rest of your email.

On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren at 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
>