Hi Stephen, On Tue, Dec 6, 2011 at 12:42 PM, Stephen Warren <swar...@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 <swar...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot