[U-Boot] [PATCH v2 2/3] Tegra114: fdt: Update DT files with I2C info for T114/Dalmore

[Stephen Warren]

On 02/07/2013 09:14 AM, Tom Warren wrote:
> Stephen,
> 
> On Wed, Feb 6, 2013 at 5:00 PM, Stephen Warren <swarren at 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 at 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 at 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.

>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts b/board/nvidia/dts/tegra114-dalmore.dts
>>
>>> +     i2c at 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.

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