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

Tue Feb 12 20:13:11 CET 2013

Simon,

On Tue, Feb 12, 2013 at 11:13 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Stephen,
>
> On Thu, Feb 7, 2013 at 10:17 AM, Stephen Warren <swarren at 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 <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.
>
> 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.

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

Tom
>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
> Regards,
> Simon