Hi Stephen, On Thu, Feb 7, 2013 at 10:17 AM, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 02/07/2013 09:14 AM, Tom Warren wrote: >> Stephen, >> >> On Wed, Feb 6, 2013 at 5:00 PM, Stephen Warren <swar...@wwwdotorg.org> wrote: >>> On 02/06/2013 04:26 PM, Tom Warren wrote: >>>> Note that T114 does not have a separate/different DVC (power I2C) >>>> controller like T20 - all 5 I2C controllers are identical, but >>>> I2C5 is used to designate the controller intended for power >>>> control (PWR_I2C in the schematics). >>> >>>> diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi >>> >>>> + tegra_car: clock@60006000 { >>>> + compatible = "nvidia,tegra114-car, nvidia,tegra30-car"; >>> >>> Only the Tegra114 value should be listed here; the presence of the >>> Tegra30 value in the upstream kernel is a mistake that will be fixed as >>> part of the Tegra114 clock driver patches. >> >> OK. But this is why I think hewing to the Linux DT files, while >> laudable, is a time sink. Though since T114 is so new & still settling >> in, I guess some churn is expected. > > The issue here is not about making U-Boot fall in line with the kernel. > The issue is making the device tree in U-Boot be correct. Right now, the > kernel happens to have the most correct device tree, so it's a good > reference for U-Boot. > >>>> + i2c@7000c000 { >>>> + compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-i2c"; >>> >>> Same here; only include the Tegra114 value since the HW isn't 100% >>> compatible with the Tegra20 HW. >> >> What does the 'compatible' designater mean, exactly? Does it require >> 100% identical HW? Compatible, in a SW/HW sense, to me means that >> newer SW will work with older/similar (but not exactly the same) HW, >> etc. > > The idea here is that the first entry in compatible always defines the > most specific implementation identification possible. So, compatible > will at least contain: > > Tegra20: nvidia,tegra20-i2c > Tegra30: nvidia,tegra30-i2c > Tegra114: nvidia,tegra114-i2c > > Now, if a piece of newer hardware can be operated 100% correctly > (ignoring issues due to new features not being exposed, or operating at > degraded performance) by a driver that only knows about the older > hardware, then the compatible property can additionally contain other > values indicating what HW it is compatible with. So, we actually end up > with: > > Tegra20: nvidia,tegra20-i2c > Tegra30: nvidia,tegra30-i2c nvidia,tegra20-i2c > Tegra114: nvidia,tegra114-i2c > > Tegra114 I2C HW isn't marked as compatible with either Tegra20 or > Tegra30, because the clock divider programming must be different. > >> Tegra U-Boot uses the "tegra20-i2c" name to find and load the I2C >> driver. I could add a new entry in the compat-names table for >> tegra114-i2c, > > Yes, that is the way to go. The driver should include a list of all the > different compatible values that it supports. > >> but I still don't think there's enough difference >> between T20/T30 and T114 I2C to justify a whole new I2C driver - so >> far, it's just one register with a new clk divider that has to be >> taken in consideration when programming the clock source divider. > > But that's required to operate the hardware correctly. It doesn't matter > how trivial the difference is; it could just be a single bit that needs > to be set/cleared on new HW - there would still be a difference that > would otherwise make the HW operate incorrectly. > >> I could do as the driver does for T20, where there is a separate DVC >> controller, plus 3 normal I2C controllers. I could 'find_aliases' for >> the tegrat114-i2c controller first, process its nodes, and return. If >> no tegrat114-i2c exists, the driver would continue on to look for >> tegra20-i2c/tegra20-dvc controller info as it does now. It'd still be >> one tegra_i2c.c driver, with a flag in the i2c_bus struct telling me >> if I found T114 I2C HW (i2c_bus->single_clk, etc.) and I could then do >> the new clock divider dance based on that flag. How does that sound? > > The U-Boot function that returns the list of devices supported by the > driver should be enhanced so that it can search for nodes compatible > with any 1 (or more) of n compatible values at a time, rather than just > a single compatible value. Then, all you'd have to do with this change > is add a new entry into the table, not add extra code that calls that > function separately for each compatible value.
Yes, that needs to be done, and while we are at it I think the function should return a list containing struct {int offset; enum fdt_compat_id; }; I could probably do this by end of week if no one else can do it faster. Please let me know. Tests need to updated also. > >>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts >>>> b/board/nvidia/dts/tegra114-dalmore.dts >>> >>>> + i2c@7000d000 { >>>> + status = "okay"; >>>> + clock-frequency = <100000>; >>>> + }; >>> >>> According to our downstream Linux kernel, that I2C controller can run up >>> to 400KHz on this board. >> >> But it also runs @ 100KHz just fine, too. Do we need to run at the >> fastest clock? That's the DVC (PWR_I2C) controller, which U-Boot does >> little to nothing with right now. >> >> I can set it to 400KHz, but what are the advantages/justifications? Is >> anything wrong with leaving it at 100KHz? > > The device tree is about describing the hardware. The hardware can run > at 400KHz, so the device tree should accurately describe this. It's > simply a matter of correctness, rather than randomly filling in > something that happens to work. > > One practical advantage here is increased boot speed due to I2C accesses > taking less time. The advantage might be small here since we probably > don't configure too many regulators in U-Boot, but I bet Simon Glass is > counting every uS. Well we count uS, but only refactor code for mS :-) > > And again, if/when we can ever use the same DT for U-Boot and the > kernel, this needs to be corrected so the kernel isn't forced to run at > a reduced speed for some reason. The kernel sends many many more > commands to the PMIC, and many are time-sensitive (e.g. CPU voltage bus > adjustments for DVFS). Yes we should use the kernel FDT where possible. > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot