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

[Stephen Warren]

On 12/05/2011 04:35 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Dec 5, 2011 at 2:15 PM, Stephen Warren <swarren at nvidia.com> wrote:
>> On 12/05/2011 02:46 PM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> On Mon, Dec 5, 2011 at 1:33 PM, Stephen Warren <swarren at nvidia.com> wrote:
>>>> On 12/02/2011 05:59 PM, Simon Glass wrote:
>>>>> 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.
>>>>
>>>>>>> +       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.
>>>>
>>>> That will cause backwards-compatibility problems; older FDTs won't work
>>>> with newer U-Boots. We should avoid incompatible changes like this if at
>>>> all possible.
>>>
>>> At the moment the fdts are in the U-Boot tree, so we should be able to
>>> change them at the same time. But of course when we update the fdt  we
>>> need to update the U-Boot code. Is there any alternative other than
>>> doing nothing until the kernel decides everything?
>>
>> You can choose not to represent these parameters in the device tree at
>> all, but rather hard-code the values in the driver. This is what the
>> kernel currently does; as luck would have it, the required values are
>> identical for all the boards the kernel currently supports. Once that's
>> all in place and working, we can work on defining a binding for those
>> parameters and review/implement it in both U-Boot and the kernel.
> 
> OK, but how about we have this conversation when things are actually
> in the kernel and working? The scheme used in this series (peripheral
> IDs) is very handy for U-Boot and avoids this hard-coding that you
> refer to.

I'd really like to avoid adding stuff to the .dts file when we know we
going to rip it out again ASAP. I'd prefer to incrementally enhance the
.dts bindings always moving forward, and never removing/breaking stuff
if possible.

Now you did point out that the .dts files are currently in U-Boot, so
this is slightly moot since the binding documentation, .dts file and
driver can all be rev'd in sync. However, I hope this will change soon.
Otherwise, every addition to them means changing U-Boot, the Linux
kernel, U-Boot v2, fastboot, quick boot, .....

> Also the only way I can see it being hard-coded is by the kernel
> looking at the peripheral address and converting this to an ID. That
> seems really ugly to me. Or am I missing something?

Well, the Tegra SD/MMC driver works exactly that way (well, mapping an
instance ID to both base address and periph ID), so there's certainly
precedent for this. And that driver is not unique.

>>>>>>> +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.
>>>>
>>>> Why does U-Boot care? Everything should be enumerating which peripherals
>>>> happen to appear on the various USB busses, and not care which host
>>>> controller they're attached to.
>>>
>>> At present we do:
>>>
>>> 'usb start 0'
>>> 'usb start 1'
>>>
>>> to select between the ports. There is a patch in the works to change
>>> that but it hasn't gone upstream yet, or at least wasn't accepted.

Judging by the USB driver, there's actually only "usb start" with no
parameter, and ehc-tegra.c:ehci_hcd_init() hard-codes this as starting
port 0. That's a little more severe. Anyway, that implies to me that
tegra-seaboard.dts should set status = "disabled" on all but one port,
because the others will be useless anyway.

>> Can you point out the patch that changes this, and what exactly it
>> changes. Hopefully it removes the parameter from the "usb start" command.
> 
> Yes, they are in the Chromium tree:
> 
> https://gerrit.chromium.org/gerrit/#change,4951
> https://gerrit.chromium.org/gerrit/#change,4981

OK, those look interesting, at least from the commit descriptions. I'd
assert they really should be upstreamed before or as part of the Tegra
USB driver addition, since it makes the whole /aliases thing completely
irrelevant for USB.

>> I'm still not convinced why U-Boot cares about this (as opposed to the
>> user using U-Boot).
> 
> Well if U-Boot presents the ports in the wrong order, the user's
> scripts will fail.
> 
> Also, what about the console UART problem? Surely the kernel provides
> a way to select the ordering of those? How do I know what UART I am
> getting on /dev/ttyS0?

That's a question my subconscious has been asking me for a while to. The
answer is that it's all very accidental...

The kernel serial driver needs a clock-frequency property. If it isn't
present, the driver won't initialize. The .dts files in the kernel only
have this property for serial ports that are hooked up on the given
board. In the case (Paz00 a/k/a Toshiba AC100) where multiple serial
ports have this property, the useful one just happens to be the one the
kernel processes first. So, it all just happens to work out. I have
since at least posted patches to add explicit status = "disabled" for
the ports that aren't hooked up, but we should start thinking about how
to use /aliases to force a particular enumeration order rather than
relying on whatever is making it work accidentally.

-- 
nvpublic