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

[Simon Glass]

Hi Stephen,

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:

Thank you.

I will tidy up the code changes later, but just wanted to pick up on a
few general points.

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

Not yet supported. I don't have a board that brings it out.

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

Do you mean device-tree list? I did send the patch there. Does the
kernel have another way of knowing that this port is special?

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

It is actually the bit position of the peripheral in the clock
registers, so arguably a hardware description. U-Boot uses this to
efficiently manage peripheral clocks, reset, pinmux, etc.

How does the kernel figure out the clock register (etc.) to use with a
particular peripheral? Also bear in mind that the intent with U-Boot
is to be a lot more lightweight with these things.

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

My actual intent was that the device tree would provide options for
each oscillator frequency and the board would simply select which it
is using.  This does not fit well with out the device tree works
though - we end up having everything in there and available at
run-time, even useless data. Anyway, the oscillator frequency is
detected at run-time, so I decided to put these into the SOC
description.

Since you say these values are fixed for all boards we might as well
put them back into the code.

>
>
>> +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 aliases are (I thought) the official way that device trees specify
device ordering. No we do not enumerate things in U-Boot - there is no
device model as such. We can do this on Tegra, but still need to know
the order to use (i.e. which is port 0).

Regards,
Simon

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