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

[Stephen Warren]

On 12/01/2011 06:51 PM, Simon Glass wrote:
> 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.

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

linux-tegra and perhaps linux-usb too.

> Does the kernel have another way of knowing that this port is special?

When booted without DT, the platform data defines mode: device, host, OTG.

The current DT bindings for Tegra USB are the minimum required to get
host mode working, and don't include any representation of this, and so
host mode is assumed.

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

The DT binding has to be identical though; U-Boot implementation details
aren't supposed to affect the content of the DT.

Clock bindings are an area of active development. I haven't been
following the progress, but I imagine that the clock controller will
define a node per clock, and the devices that consume the clock will
refer to that node using a phandle. It's then up to the clock controller
driver to extract whatever information it needs from the clock node and
map that to an internal periph-id. It's plausible that a legitimate part
of the clock binding itself is such a periph-id field, but that should
be defined by the clock controller binding, not the peripheral binding.

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

The values aren't necessarily fixed for /all/ boards, they just /happen/
to be identical for all boards currently supported in the mainline
kernel. I imagine that as more boards are supported, we'll see different
sets of values in use.

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

I don't believe the kernel uses the alias for anything at all right now.
Instead, it enumerates all nodes that match a certain compatible flag,
and instantiates a device for each one it has a driver for. I believe
this mode of operation is pretty implicit in DT itself; it's something
U-Boot should do too.

If U-boot were to operate solely based on the aliases node, it wouldn't
work at all for the .dts files currently in the kernel since there are
no aliases, and if there were aliases, might end up instantiating a
subset of devices if some devices weren't mentioned in aliases.

I believe what aliases is meant for is that once you've enumerated all
the devices, then check the aliases node in order to override the
default naming (or set up alternative names). Still, you'd have to check
with a DT expert here, since I've never done anything with the aliases node.

-- 
nvpublic