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

[Simon Glass]

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.

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

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

Regards,
Simon