Hi Stephen,
On Tue, Dec 6, 2011 at 12:42 PM, Stephen Warren swarren@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@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