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

Tue Dec 6 02:14:02 CET 2011

Hi Stephen,

On Mon, Dec 5, 2011 at 4:17 PM, Stephen Warren <swarren at nvidia.com> wrote:
> 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, .....

OK well I can remove the USB params and put in the C file with an
#ifdef for T30. Ick.

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

I don't know if I can NAK a comment but I would like to NAK this one.

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

No, that needs to be decoupled from this in my view. That is a large
and invasive change which is AFAIK only useful for Tegra. It's not at
all clear it will be accepted.

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

Ickity ick. I think I'll keep my aliases if it's ok with you?

Regards,
Simon

>
> --
> nvpublic