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

[Simon Glass]

Hi Stephen,

On Fri, Dec 2, 2011 at 8:10 AM, Stephen Warren <swarren at nvidia.com> wrote:
> 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.

OK

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

OK, I will take a closer look, but may keep this in anticipation of
the kernel catching up.

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

OK, well this is an example of where I would like to run with what we
have, and adjust it later when things are finalized in the kernel.

I'm not sure about your analysis here actually. The peripherals have a
selectable source clock and their own divider from that clock, plus
they have bits for enabling their internal clock and reset. The
registers for all of these can sort-of be indexed through the
peripheral ID. I think with this model you would need to have a
separate clock node for every peripheral, with the peripheral node
pointing back to that. Perhaps that is what you mean. It means that
every peripheral has its own node and then a clock node. It probably
won't be too slow to decode.

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

OK so perhaps I should keep my bindings here...

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

It does this at present with USB. But we want to enumerate the ports
and know which is port 0, which is port 1, etc. How does the kernel do
that?

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

I did go around this loop some time ago and my understanding was that
aliases were the correct way to enumerate ports (I originally just had
an ID in the port which is easier to decode).

Regards,
Simon

>
> --
> nvpublic
>