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

Tue Feb 12 20:17:27 CET 2013

Hi Tom,

On Tue, Feb 12, 2013 at 11:13 AM, Tom Warren <twarren.nvidia at gmail.com> wrote:
> 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.

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

Yes that was always my problem - finding what the kernel actually
used, might use, etc.

Regards,
Simon

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