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

[Stephen Warren <swarren@nvidia.com>]

On 12/05/2011 06:14 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Dec 5, 2011 at 4:17 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/05/2011 04:35 PM, Simon Glass wrote:
...
>> 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.

There's no T30 support in mainline U-Boot, so I think you can avoid any
ifdefs.

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

I don't know what that means; that you believe my statement is
incorrect, or you don't like the argument I'm basing on it or ...?

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

Surely it's useful for any SoC that has multiple USB controllers, and
where the user may plug peripherals into more than 1 of them. I assume
that's common. Or am I misunderstanding what those patches do?

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

Having aliases is fine.

Using aliases to name the devices which get instantiated through
standardized enumeration mechanisms is fine.

To the best of my knowledge, restricting device enumeration based on the
aliases is non-standard, hence wrong.

As I mentioned elsewhere, the patches only allow one to actually use usb
controller 0; ehci-tegra.c's ehci_hcd_init() hard-codes port 0 when
calling tegrausb_start_port(). Rather than relying on non-standard
aliases usage to restrict the set of USB devices that get instantiated,
why not just add status = "disabled" to all but one USB host's node in
each board's .dts file; that's the standard way to disable devices.

-- 
nvpublic