Hi Tom, On Tue, Feb 12, 2013 at 11:13 AM, Tom Warren <twarren.nvi...@gmail.com> wrote: > Simon, > > On Tue, Feb 12, 2013 at 11:13 AM, Simon Glass <s...@chromium.org> wrote: >> 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. > > I won't have time for this this week (on vacation Friday thru > Tuesday). If you want to tackle this, go ahead. The current T114 I2C > patchset is good-to-go AFAICT, with the exception of the clock divisor > fix Stephen just pointed out in another thread. So you can use that as > your basis for rewrite, if you wish.
OK, well it is independent of these patches, but seem find as they are anyway. It's something we should do but doesn' t need to hold up proceedings. > >> >>> >>>>>> 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. > That's the current POR for Tegra DT use in upstream U-Boot, assuming I > can find an up-to-date kernel with the latest DTS files (I'll use > Stephen's swarren/linux-tegra.git/for-next until told otherwise). Yes that was always my problem - finding what the kernel actually used, might use, etc. Regards, Simon > > Tom >> >>> _______________________________________________ >>> 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