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

[Simon Glass]

Hi Stephen,

On Tue, Dec 6, 2011 at 12:42 PM, Stephen Warren <swarren at nvidia.com> wrote:
> On 12/05/2011 06:14 PM, Simon Glass wrote:
>> 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:
> ...
>>> 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 ...?

What happens is that the SD/MMC driver (dating from pre-FDT days) has
a hard-coded base address and peripheral ID, based on an instance ID
(0, 1, 2).

I would expect that we want the FDT to control all of this - it
already has the base address, can have the peripheral ID, and the
instance ID (ordering) should be set by the FDT not hard-coded IMO.
That's the reason for my NAK comment. It just seems completely wrong
to duplicate this information in the driver and require the instance
ordering to be hard-coded in the driver. It means that boards that
want to change this ordering must fiddle around in the driver to make
this work.

Also this is U-Boot, not the kernel. Commands like 'ext2load' require
that you pass the instance ID to indicate which device to use.

>
>>>>>> 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 don't believe it has been an issue with other SOCs, but I don't know
for sure. Anyway, it is sideways to this issue. I agree it should be
upstreamed but a separate series IMO.

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

OK, so is your objection that we ignore a peripheral that has no
alias? If I change the code to locate these and add them at the end
would that be better?

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

I can do that, but how do I solve the ordering problem?

Regards,
Simon

>
> --
> nvpublic