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

login
register
mail settings
Submitter Tom Warren
Date Feb. 6, 2013, 11:26 p.m.
Message ID <1360193208-16055-3-git-send-email-twarren@nvidia.com>
Download mbox | patch
Permalink /patch/218801/
State Superseded
Delegated to: Tom Warren
Headers show

Comments

Tom Warren - Feb. 6, 2013, 11:26 p.m.
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).

Signed-off-by: Tom Warren <twarren@nvidia.com>
---
v2: Match dts files with kernel files, remove unused apdma node

 arch/arm/dts/tegra114.dtsi            |   56 +++++++++++++++++++++++++++++++++
 board/nvidia/dts/tegra114-dalmore.dts |   33 +++++++++++++++++++
 2 files changed, 89 insertions(+), 0 deletions(-)
Stephen Warren - Feb. 7, 2013, midnight
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.

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

> 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.
Laxman Dewangan - Feb. 7, 2013, 2:57 p.m.
On Thursday 07 February 2013 04:56 AM, 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).
>
> Signed-off-by: Tom Warren <twarren@nvidia.com>
> ---


> diff --git a/board/nvidia/dts/tegra114-dalmore.dts b/board/nvidia/dts/tegra114-dalmore.dts
> index 7315577..13b07f3 100644
> --- a/board/nvidia/dts/tegra114-dalmore.dts
> +++ b/board/nvidia/dts/tegra114-dalmore.dts
> @@ -6,8 +6,41 @@
>   	model =NVIDIA Dalmore";
>   	compatible =nvidia,dalmore", "nvidia,tegra114";
>   
> +	aliases {
> +		i2c0 =/i2c@7000d000";
> +		i2c1 =/i2c@7000c000";
> +		i2c2 =/i2c@7000c400";
> +		i2c3 =/i2c@7000c500";
> +		i2c4 =/i2c@7000c700";
> +	};

Can we move this to tegar114.dtsi file.

otherwise it looks good.
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
Tom Warren - Feb. 7, 2013, 4:14 p.m.
Stephen,

On Wed, Feb 6, 2013 at 5:00 PM, Stephen Warren <swarren@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.

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

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

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

Tom
Tom Warren - Feb. 7, 2013, 4:17 p.m.
Laxman,

On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> On Thursday 07 February 2013 04:56 AM, 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).
>>
>> Signed-off-by: Tom Warren <twarren@nvidia.com>
>> ---
>
>
>
>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts
>> b/board/nvidia/dts/tegra114-dalmore.dts
>> index 7315577..13b07f3 100644
>> --- a/board/nvidia/dts/tegra114-dalmore.dts
>> +++ b/board/nvidia/dts/tegra114-dalmore.dts
>> @@ -6,8 +6,41 @@
>>         model =NVIDIA Dalmore";
>>         compatible =nvidia,dalmore", "nvidia,tegra114";
>>
>>   +     aliases {
>> +               i2c0 =/i2c@7000d000";
>> +               i2c1 =/i2c@7000c000";
>> +               i2c2 =/i2c@7000c400";
>> +               i2c3 =/i2c@7000c500";
>> +               i2c4 =/i2c@7000c700";
>> +       };
>
>
> Can we move this to tegar114.dtsi file.

I could, but why? Most, if not all, of the U-Boot boards that use DT
are putting their aliases in the .dts file in the board directory.

>
> otherwise it looks good.
> Acked-by: Laxman Dewangan <ldewangan@nvidia.com>

Thanks, Laxman. Appreciate your taking a look.

Tom
>
>
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may
> contain
> confidential information.  Any unauthorized review, use, disclosure or
> distribution
> is prohibited.  If you are not the intended recipient, please contact the
> sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
Stephen Warren - Feb. 7, 2013, 6:07 p.m.
On 02/07/2013 09:17 AM, Tom Warren wrote:
> Laxman,
> 
> On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>> On Thursday 07 February 2013 04:56 AM, 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).
>>>
>>> Signed-off-by: Tom Warren <twarren@nvidia.com>
>>> ---
>>
>>
>>
>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts
>>> b/board/nvidia/dts/tegra114-dalmore.dts
>>> index 7315577..13b07f3 100644
>>> --- a/board/nvidia/dts/tegra114-dalmore.dts
>>> +++ b/board/nvidia/dts/tegra114-dalmore.dts
>>> @@ -6,8 +6,41 @@
>>>         model =NVIDIA Dalmore";
>>>         compatible =nvidia,dalmore", "nvidia,tegra114";
>>>
>>>   +     aliases {
>>> +               i2c0 =/i2c@7000d000";
>>> +               i2c1 =/i2c@7000c000";
>>> +               i2c2 =/i2c@7000c400";
>>> +               i2c3 =/i2c@7000c500";
>>> +               i2c4 =/i2c@7000c700";
>>> +       };
>>
>>
>> Can we move this to tegar114.dtsi file.
> 
> I could, but why? Most, if not all, of the U-Boot boards that use DT
> are putting their aliases in the .dts file in the board directory.

Laxman, the issue here is that right now in U-Boot, I believe, if a
particular board only uses I2C adapters 0, 2, and 4, then only U-Boot
device IDs 0, 1, and 2 are defined, rather than IDs 0, 2, and 4. That's
why the aliases are in the per-board file for now, because the actual
set of I2C adapters enabled is board-specific.

Tom, as background for Laxman's request, for other devices (e.g. serial
ports), customer engineers have pushed back on that naming scheme, and
always want a specific HW device to end up with a static name,
irrespective of which other devices of the same type are used, if any.
For that reason, in the kernel, we have aliases for the serial ports in
tegra*.dtsi rather than in per-board files, and the names are static.

So I wonder if in U-Boot we really have to have IDs 0..n rather than
e.g. IDs 0, 2, 4 for the I2C ports (when some aren't used). Then, we
could just put the aliases in tegra*.dtsi, which makes life simpler when
creating board .dts files...
Tom Warren - Feb. 7, 2013, 6:14 p.m.
Stephen,

On Thu, Feb 7, 2013 at 11:07 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/07/2013 09:17 AM, Tom Warren wrote:
>> Laxman,
>>
>> On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>> On Thursday 07 February 2013 04:56 AM, 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).
>>>>
>>>> Signed-off-by: Tom Warren <twarren@nvidia.com>
>>>> ---
>>>
>>>
>>>
>>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts
>>>> b/board/nvidia/dts/tegra114-dalmore.dts
>>>> index 7315577..13b07f3 100644
>>>> --- a/board/nvidia/dts/tegra114-dalmore.dts
>>>> +++ b/board/nvidia/dts/tegra114-dalmore.dts
>>>> @@ -6,8 +6,41 @@
>>>>         model =NVIDIA Dalmore";
>>>>         compatible =nvidia,dalmore", "nvidia,tegra114";
>>>>
>>>>   +     aliases {
>>>> +               i2c0 =/i2c@7000d000";
>>>> +               i2c1 =/i2c@7000c000";
>>>> +               i2c2 =/i2c@7000c400";
>>>> +               i2c3 =/i2c@7000c500";
>>>> +               i2c4 =/i2c@7000c700";
>>>> +       };
>>>
>>>
>>> Can we move this to tegar114.dtsi file.
>>
>> I could, but why? Most, if not all, of the U-Boot boards that use DT
>> are putting their aliases in the .dts file in the board directory.
>
> Laxman, the issue here is that right now in U-Boot, I believe, if a
> particular board only uses I2C adapters 0, 2, and 4, then only U-Boot
> device IDs 0, 1, and 2 are defined, rather than IDs 0, 2, and 4. That's
> why the aliases are in the per-board file for now, because the actual
> set of I2C adapters enabled is board-specific.
>
> Tom, as background for Laxman's request, for other devices (e.g. serial
> ports), customer engineers have pushed back on that naming scheme, and
> always want a specific HW device to end up with a static name,
> irrespective of which other devices of the same type are used, if any.
> For that reason, in the kernel, we have aliases for the serial ports in
> tegra*.dtsi rather than in per-board files, and the names are static.
>
> So I wonder if in U-Boot we really have to have IDs 0..n rather than
> e.g. IDs 0, 2, 4 for the I2C ports (when some aren't used). Then, we
> could just put the aliases in tegra*.dtsi, which makes life simpler when
> creating board .dts files...

Thanks for the background info. I'm focusing on getting T114 I2C in
right now, so I can move on to MMC and USB, and what I have seems
reasonable/conforms to what's already done in U-Boot.

I don't want to get into reworking everyone's dts/dtsi files right now
to move aliases around, or find out which boards really have unused
I2C ports (Dalmore uses 'em all, BTW, according to our I2C
spreadsheet). If you want to send a set of cleanup/re-org patches for
everybody's DTS files, or even just the Tegra boards, feel free. I'll
be glad to review it/apply it to u-boot-tegra later.

Thanks,

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

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

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).
Stephen Warren - Feb. 7, 2013, 6:20 p.m.
On 02/07/2013 11:14 AM, Tom Warren wrote:
> Stephen,
> 
> On Thu, Feb 7, 2013 at 11:07 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 02/07/2013 09:17 AM, Tom Warren wrote:
>>> Laxman,
>>>
>>> On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>>> On Thursday 07 February 2013 04:56 AM, 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).
>>>>>
>>>>> Signed-off-by: Tom Warren <twarren@nvidia.com>
>>>>> ---
>>>>
>>>>
>>>>
>>>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts
>>>>> b/board/nvidia/dts/tegra114-dalmore.dts
>>>>> index 7315577..13b07f3 100644
>>>>> --- a/board/nvidia/dts/tegra114-dalmore.dts
>>>>> +++ b/board/nvidia/dts/tegra114-dalmore.dts
>>>>> @@ -6,8 +6,41 @@
>>>>>         model =NVIDIA Dalmore";
>>>>>         compatible =nvidia,dalmore", "nvidia,tegra114";
>>>>>
>>>>>   +     aliases {
>>>>> +               i2c0 =/i2c@7000d000";
>>>>> +               i2c1 =/i2c@7000c000";
>>>>> +               i2c2 =/i2c@7000c400";
>>>>> +               i2c3 =/i2c@7000c500";
>>>>> +               i2c4 =/i2c@7000c700";
>>>>> +       };
>>>>
>>>>
>>>> Can we move this to tegar114.dtsi file.
>>>
>>> I could, but why? Most, if not all, of the U-Boot boards that use DT
>>> are putting their aliases in the .dts file in the board directory.
>>
>> Laxman, the issue here is that right now in U-Boot, I believe, if a
>> particular board only uses I2C adapters 0, 2, and 4, then only U-Boot
>> device IDs 0, 1, and 2 are defined, rather than IDs 0, 2, and 4. That's
>> why the aliases are in the per-board file for now, because the actual
>> set of I2C adapters enabled is board-specific.
>>
>> Tom, as background for Laxman's request, for other devices (e.g. serial
>> ports), customer engineers have pushed back on that naming scheme, and
>> always want a specific HW device to end up with a static name,
>> irrespective of which other devices of the same type are used, if any.
>> For that reason, in the kernel, we have aliases for the serial ports in
>> tegra*.dtsi rather than in per-board files, and the names are static.
>>
>> So I wonder if in U-Boot we really have to have IDs 0..n rather than
>> e.g. IDs 0, 2, 4 for the I2C ports (when some aren't used). Then, we
>> could just put the aliases in tegra*.dtsi, which makes life simpler when
>> creating board .dts files...
> 
> Thanks for the background info. I'm focusing on getting T114 I2C in
> right now, so I can move on to MMC and USB, and what I have seems
> reasonable/conforms to what's already done in U-Boot.

That doesn't necessarily make it correct.

> I don't want to get into reworking everyone's dts/dtsi files right now
> to move aliases around, or find out which boards really have unused
> I2C ports (Dalmore uses 'em all, BTW, according to our I2C
> spreadsheet). If you want to send a set of cleanup/re-org patches for
> everybody's DTS files, or even just the Tegra boards, feel free. I'll
> be glad to review it/apply it to u-boot-tegra later.

Sorry, but that's simply part of writing the .dts file for a board;
there is no way around that.
Tom Warren - Feb. 7, 2013, 7:05 p.m.
Stephen,

On Thu, Feb 7, 2013 at 11:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/07/2013 11:14 AM, Tom Warren wrote:
>> Stephen,
>>
>> On Thu, Feb 7, 2013 at 11:07 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 02/07/2013 09:17 AM, Tom Warren wrote:
>>>> Laxman,
>>>>
>>>> On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>>>> On Thursday 07 February 2013 04:56 AM, 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).
>>>>>>
>>>>>> Signed-off-by: Tom Warren <twarren@nvidia.com>
>>>>>> ---
>>>>>
>>>>>
>>>>>
>>>>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts
>>>>>> b/board/nvidia/dts/tegra114-dalmore.dts
>>>>>> index 7315577..13b07f3 100644
>>>>>> --- a/board/nvidia/dts/tegra114-dalmore.dts
>>>>>> +++ b/board/nvidia/dts/tegra114-dalmore.dts
>>>>>> @@ -6,8 +6,41 @@
>>>>>>         model =NVIDIA Dalmore";
>>>>>>         compatible =nvidia,dalmore", "nvidia,tegra114";
>>>>>>
>>>>>>   +     aliases {
>>>>>> +               i2c0 =/i2c@7000d000";
>>>>>> +               i2c1 =/i2c@7000c000";
>>>>>> +               i2c2 =/i2c@7000c400";
>>>>>> +               i2c3 =/i2c@7000c500";
>>>>>> +               i2c4 =/i2c@7000c700";
>>>>>> +       };
>>>>>
>>>>>
>>>>> Can we move this to tegar114.dtsi file.
>>>>
>>>> I could, but why? Most, if not all, of the U-Boot boards that use DT
>>>> are putting their aliases in the .dts file in the board directory.
>>>
>>> Laxman, the issue here is that right now in U-Boot, I believe, if a
>>> particular board only uses I2C adapters 0, 2, and 4, then only U-Boot
>>> device IDs 0, 1, and 2 are defined, rather than IDs 0, 2, and 4. That's
>>> why the aliases are in the per-board file for now, because the actual
>>> set of I2C adapters enabled is board-specific.
>>>
>>> Tom, as background for Laxman's request, for other devices (e.g. serial
>>> ports), customer engineers have pushed back on that naming scheme, and
>>> always want a specific HW device to end up with a static name,
>>> irrespective of which other devices of the same type are used, if any.
>>> For that reason, in the kernel, we have aliases for the serial ports in
>>> tegra*.dtsi rather than in per-board files, and the names are static.
>>>
>>> So I wonder if in U-Boot we really have to have IDs 0..n rather than
>>> e.g. IDs 0, 2, 4 for the I2C ports (when some aren't used). Then, we
>>> could just put the aliases in tegra*.dtsi, which makes life simpler when
>>> creating board .dts files...
>>
>> Thanks for the background info. I'm focusing on getting T114 I2C in
>> right now, so I can move on to MMC and USB, and what I have seems
>> reasonable/conforms to what's already done in U-Boot.
>
> That doesn't necessarily make it correct.
>
>> I don't want to get into reworking everyone's dts/dtsi files right now
>> to move aliases around, or find out which boards really have unused
>> I2C ports (Dalmore uses 'em all, BTW, according to our I2C
>> spreadsheet). If you want to send a set of cleanup/re-org patches for
>> everybody's DTS files, or even just the Tegra boards, feel free. I'll
>> be glad to review it/apply it to u-boot-tegra later.
>
> Sorry, but that's simply part of writing the .dts file for a board;
> there is no way around that.

OK, how about this. For T114, I'll move the aliases to tegra114.dtsi,
and use 400KHz for all I2C nodes except I2C1 (saw some internal code
saying that it may not run well at that speed on the E1611 board).
Since all 5 controllers have devices behind them, I think it's OK to
leave all 5 in the aliases.

What do you think of that approach?

Adding Yen Lin to CC

Tom
Stephen Warren - Feb. 7, 2013, 7:08 p.m.
On 02/07/2013 12:05 PM, Tom Warren wrote:
> Stephen,
> 
> On Thu, Feb 7, 2013 at 11:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 02/07/2013 11:14 AM, Tom Warren wrote:
>>> Stephen,
>>>
>>> On Thu, Feb 7, 2013 at 11:07 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> On 02/07/2013 09:17 AM, Tom Warren wrote:
>>>>> Laxman,
>>>>>
>>>>> On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>>>>> On Thursday 07 February 2013 04:56 AM, 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).
>>>>>>>
>>>>>>> Signed-off-by: Tom Warren <twarren@nvidia.com>
>>>>>>> ---
>>>>>>
>>>>>>
>>>>>>
>>>>>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts
>>>>>>> b/board/nvidia/dts/tegra114-dalmore.dts
>>>>>>> index 7315577..13b07f3 100644
>>>>>>> --- a/board/nvidia/dts/tegra114-dalmore.dts
>>>>>>> +++ b/board/nvidia/dts/tegra114-dalmore.dts
>>>>>>> @@ -6,8 +6,41 @@
>>>>>>>         model =NVIDIA Dalmore";
>>>>>>>         compatible =nvidia,dalmore", "nvidia,tegra114";
>>>>>>>
>>>>>>>   +     aliases {
>>>>>>> +               i2c0 =/i2c@7000d000";
>>>>>>> +               i2c1 =/i2c@7000c000";
>>>>>>> +               i2c2 =/i2c@7000c400";
>>>>>>> +               i2c3 =/i2c@7000c500";
>>>>>>> +               i2c4 =/i2c@7000c700";
>>>>>>> +       };
>>>>>>
>>>>>>
>>>>>> Can we move this to tegar114.dtsi file.
>>>>>
>>>>> I could, but why? Most, if not all, of the U-Boot boards that use DT
>>>>> are putting their aliases in the .dts file in the board directory.
>>>>
>>>> Laxman, the issue here is that right now in U-Boot, I believe, if a
>>>> particular board only uses I2C adapters 0, 2, and 4, then only U-Boot
>>>> device IDs 0, 1, and 2 are defined, rather than IDs 0, 2, and 4. That's
>>>> why the aliases are in the per-board file for now, because the actual
>>>> set of I2C adapters enabled is board-specific.
>>>>
>>>> Tom, as background for Laxman's request, for other devices (e.g. serial
>>>> ports), customer engineers have pushed back on that naming scheme, and
>>>> always want a specific HW device to end up with a static name,
>>>> irrespective of which other devices of the same type are used, if any.
>>>> For that reason, in the kernel, we have aliases for the serial ports in
>>>> tegra*.dtsi rather than in per-board files, and the names are static.
>>>>
>>>> So I wonder if in U-Boot we really have to have IDs 0..n rather than
>>>> e.g. IDs 0, 2, 4 for the I2C ports (when some aren't used). Then, we
>>>> could just put the aliases in tegra*.dtsi, which makes life simpler when
>>>> creating board .dts files...
>>>
>>> Thanks for the background info. I'm focusing on getting T114 I2C in
>>> right now, so I can move on to MMC and USB, and what I have seems
>>> reasonable/conforms to what's already done in U-Boot.
>>
>> That doesn't necessarily make it correct.
>>
>>> I don't want to get into reworking everyone's dts/dtsi files right now
>>> to move aliases around, or find out which boards really have unused
>>> I2C ports (Dalmore uses 'em all, BTW, according to our I2C
>>> spreadsheet). If you want to send a set of cleanup/re-org patches for
>>> everybody's DTS files, or even just the Tegra boards, feel free. I'll
>>> be glad to review it/apply it to u-boot-tegra later.
>>
>> Sorry, but that's simply part of writing the .dts file for a board;
>> there is no way around that.
> 
> OK, how about this. For T114, I'll move the aliases to tegra114.dtsi,

I believe that makes sense.

But please do check that if you do disable one of the controllers, that
the other 4 I2C ports do all get registered and still work OK, so we've
tested that code-path. I'm not sure if we have tested the case where the
I2C channels don't have contiguous IDs yet, so I have no idea if it works.

> and use 400KHz for all I2C nodes except I2C1 (saw some internal code
> saying that it may not run well at that speed on the E1611 board).
> Since all 5 controllers have devices behind them, I think it's OK to
> leave all 5 in the aliases.

The downstream kernel only has I2C5 (IIRC) set to 400KHz - the other 4
are limited to 100KHz.
Tom Warren - Feb. 7, 2013, 7:59 p.m.
Stephen,

On Thu, Feb 7, 2013 at 12:08 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/07/2013 12:05 PM, Tom Warren wrote:
>> Stephen,
>>
>> On Thu, Feb 7, 2013 at 11:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 02/07/2013 11:14 AM, Tom Warren wrote:
>>>> Stephen,
>>>>
>>>> On Thu, Feb 7, 2013 at 11:07 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>> On 02/07/2013 09:17 AM, Tom Warren wrote:
>>>>>> Laxman,
>>>>>>
>>>>>> On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>>>>>> On Thursday 07 February 2013 04:56 AM, 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).
>>>>>>>>
>>>>>>>> Signed-off-by: Tom Warren <twarren@nvidia.com>
>>>>>>>> ---
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts
>>>>>>>> b/board/nvidia/dts/tegra114-dalmore.dts
>>>>>>>> index 7315577..13b07f3 100644
>>>>>>>> --- a/board/nvidia/dts/tegra114-dalmore.dts
>>>>>>>> +++ b/board/nvidia/dts/tegra114-dalmore.dts
>>>>>>>> @@ -6,8 +6,41 @@
>>>>>>>>         model =NVIDIA Dalmore";
>>>>>>>>         compatible =nvidia,dalmore", "nvidia,tegra114";
>>>>>>>>
>>>>>>>>   +     aliases {
>>>>>>>> +               i2c0 =/i2c@7000d000";
>>>>>>>> +               i2c1 =/i2c@7000c000";
>>>>>>>> +               i2c2 =/i2c@7000c400";
>>>>>>>> +               i2c3 =/i2c@7000c500";
>>>>>>>> +               i2c4 =/i2c@7000c700";
>>>>>>>> +       };
>>>>>>>
>>>>>>>
>>>>>>> Can we move this to tegar114.dtsi file.
>>>>>>
>>>>>> I could, but why? Most, if not all, of the U-Boot boards that use DT
>>>>>> are putting their aliases in the .dts file in the board directory.
>>>>>
>>>>> Laxman, the issue here is that right now in U-Boot, I believe, if a
>>>>> particular board only uses I2C adapters 0, 2, and 4, then only U-Boot
>>>>> device IDs 0, 1, and 2 are defined, rather than IDs 0, 2, and 4. That's
>>>>> why the aliases are in the per-board file for now, because the actual
>>>>> set of I2C adapters enabled is board-specific.
>>>>>
>>>>> Tom, as background for Laxman's request, for other devices (e.g. serial
>>>>> ports), customer engineers have pushed back on that naming scheme, and
>>>>> always want a specific HW device to end up with a static name,
>>>>> irrespective of which other devices of the same type are used, if any.
>>>>> For that reason, in the kernel, we have aliases for the serial ports in
>>>>> tegra*.dtsi rather than in per-board files, and the names are static.
>>>>>
>>>>> So I wonder if in U-Boot we really have to have IDs 0..n rather than
>>>>> e.g. IDs 0, 2, 4 for the I2C ports (when some aren't used). Then, we
>>>>> could just put the aliases in tegra*.dtsi, which makes life simpler when
>>>>> creating board .dts files...
>>>>
>>>> Thanks for the background info. I'm focusing on getting T114 I2C in
>>>> right now, so I can move on to MMC and USB, and what I have seems
>>>> reasonable/conforms to what's already done in U-Boot.
>>>
>>> That doesn't necessarily make it correct.
>>>
>>>> I don't want to get into reworking everyone's dts/dtsi files right now
>>>> to move aliases around, or find out which boards really have unused
>>>> I2C ports (Dalmore uses 'em all, BTW, according to our I2C
>>>> spreadsheet). If you want to send a set of cleanup/re-org patches for
>>>> everybody's DTS files, or even just the Tegra boards, feel free. I'll
>>>> be glad to review it/apply it to u-boot-tegra later.
>>>
>>> Sorry, but that's simply part of writing the .dts file for a board;
>>> there is no way around that.
>>
>> OK, how about this. For T114, I'll move the aliases to tegra114.dtsi,
>
> I believe that makes sense.
>
> But please do check that if you do disable one of the controllers, that
> the other 4 I2C ports do all get registered and still work OK, so we've
> tested that code-path. I'm not sure if we have tested the case where the
> I2C channels don't have contiguous IDs yet, so I have no idea if it works.

Yep, if I remove the 'status = okay' from one of the ports (meaning
that the .dtsi 'status=disabled' takes over), it can't be selected in
the 'i2c' command @ the cmd prompt. Even if I take out dev 2, for
instance, I can still select/probe devs 0, 1, 3, and 4.
>
>> and use 400KHz for all I2C nodes except I2C1 (saw some internal code
>> saying that it may not run well at that speed on the E1611 board).
>> Since all 5 controllers have devices behind them, I think it's OK to
>> leave all 5 in the aliases.
>
> The downstream kernel only has I2C5 (IIRC) set to 400KHz - the other 4
> are limited to 100KHz.
OK, I'll update just I2C5/dev 0/PWR_I2C to 400KHz.

Thanks
Simon Glass - Feb. 12, 2013, 6:13 p.m.
Hi Stephen,

On Thu, Feb 7, 2013 at 10:17 AM, Stephen Warren <swarren@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@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.

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

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

Regards,
Simon
Tom Warren - Feb. 12, 2013, 7:13 p.m.
Simon,

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

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

Tom
>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
> Regards,
> Simon
Simon Glass - Feb. 12, 2013, 7:17 p.m.
Hi Tom,

On Tue, Feb 12, 2013 at 11:13 AM, Tom Warren <twarren.nvidia@gmail.com> wrote:
> Simon,
>
> On Tue, Feb 12, 2013 at 11:13 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Stephen,
>>
>> On Thu, Feb 7, 2013 at 10:17 AM, Stephen Warren <swarren@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@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
Stephen Warren - Feb. 12, 2013, 7:26 p.m.
On 02/12/2013 12:17 PM, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, Feb 12, 2013 at 11:13 AM, Tom Warren <twarren.nvidia@gmail.com> wrote:
...
>> 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.

I must say I find this quite puzzling; the kernel repos aren't exactly
hidden.

Tegra's kernel for-next is likely the best place right now as any DT
changes typically go through that tree. However, on the off-chance any
other maintainer picks up any changes, linux-next.git would have the
latest bindings. That's all been true for a year or more.

That said, there is a move to either move the binding definitions and
.dts files out of the kernel tree and/or stop taking changes to them
through individual SoC trees, but perhaps through the device tree repo.
If that does happen, I'll let you know.
Simon Glass - Feb. 12, 2013, 7:32 p.m.
Hi Stephen,

On Tue, Feb 12, 2013 at 11:26 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/12/2013 12:17 PM, Simon Glass wrote:
>> Hi Tom,
>>
>> On Tue, Feb 12, 2013 at 11:13 AM, Tom Warren <twarren.nvidia@gmail.com> wrote:
> ...
>>> 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.
>
> I must say I find this quite puzzling; the kernel repos aren't exactly
> hidden.

Well if a change has made it to a repo then things are easier,
particularly if it is next/. It was chasing down patches on mailing
lists that I found hard.

>
> Tegra's kernel for-next is likely the best place right now as any DT
> changes typically go through that tree. However, on the off-chance any
> other maintainer picks up any changes, linux-next.git would have the
> latest bindings. That's all been true for a year or more.
>
> That said, there is a move to either move the binding definitions and
> .dts files out of the kernel tree and/or stop taking changes to them
> through individual SoC trees, but perhaps through the device tree repo.
> If that does happen, I'll let you know.

Sounds good.

Regards,
Simon
Tom Warren - Feb. 12, 2013, 7:33 p.m.
On Tue, Feb 12, 2013 at 12:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/12/2013 12:17 PM, Simon Glass wrote:
>> Hi Tom,
>>
>> On Tue, Feb 12, 2013 at 11:13 AM, Tom Warren <twarren.nvidia@gmail.com> wrote:
> ...
>>> 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.
>
> I must say I find this quite puzzling; the kernel repos aren't exactly
> hidden.
Not hidden, but numerous. Perhaps I should have said 'can find _the
correct_ kernel ...'. I've been pointed to 2 or 3 different repos over
the course of these DT patches. I'm not a kernel guru, and I don't
have the bandwidth to monitor kernel reflectors, etc. When I need
something kernel-related, I resort to Google or asking you.

>
> Tegra's kernel for-next is likely the best place right now as any DT
> changes typically go through that tree. However, on the off-chance any
> other maintainer picks up any changes, linux-next.git would have the
> latest bindings. That's all been true for a year or more.
>
> That said, there is a move to either move the binding definitions and
> .dts files out of the kernel tree and/or stop taking changes to them
> through individual SoC trees, but perhaps through the device tree repo.
> If that does happen, I'll let you know.

Patch

diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi
index d06cd12..7b0c835 100644
--- a/arch/arm/dts/tegra114.dtsi
+++ b/arch/arm/dts/tegra114.dtsi
@@ -2,4 +2,60 @@ 
 
 / {
 	compatible = "nvidia,tegra114";
+
+	tegra_car: clock@60006000 {
+		compatible = "nvidia,tegra114-car, nvidia,tegra30-car";
+		reg = <0x60006000 0x1000>;
+		#clock-cells = <1>;
+	};
+
+	i2c@7000c000 {
+		compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-i2c";
+		reg = <0x7000c000 0x100>;
+		interrupts = <0 38 0x04>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&tegra_car 12>;
+		status = "disabled";
+	};
+
+	i2c@7000c400 {
+		compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-i2c";
+		reg = <0x7000c400 0x100>;
+		interrupts = <0 84 0x04>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&tegra_car 54>;
+		status = "disabled";
+	};
+
+	i2c@7000c500 {
+		compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-i2c";
+		reg = <0x7000c500 0x100>;
+		interrupts = <0 92 0x04>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&tegra_car 67>;
+		status = "disabled";
+	};
+
+	i2c@7000c700 {
+		compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-i2c";
+		reg = <0x7000c700 0x100>;
+		interrupts = <0 120 0x04>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&tegra_car 103>;
+		status = "disabled";
+	};
+
+	i2c@7000d000 {
+		compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-i2c";
+		reg = <0x7000d000 0x100>;
+		interrupts = <0 53 0x04>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&tegra_car 47>;
+		status = "disabled";
+	};
 };
diff --git a/board/nvidia/dts/tegra114-dalmore.dts b/board/nvidia/dts/tegra114-dalmore.dts
index 7315577..13b07f3 100644
--- a/board/nvidia/dts/tegra114-dalmore.dts
+++ b/board/nvidia/dts/tegra114-dalmore.dts
@@ -6,8 +6,41 @@ 
 	model = "NVIDIA Dalmore";
 	compatible = "nvidia,dalmore", "nvidia,tegra114";
 
+	aliases {
+		i2c0 = "/i2c@7000d000";
+		i2c1 = "/i2c@7000c000";
+		i2c2 = "/i2c@7000c400";
+		i2c3 = "/i2c@7000c500";
+		i2c4 = "/i2c@7000c700";
+	};
+
 	memory {
 		device_type = "memory";
 		reg = <0x80000000 0x80000000>;
 	};
+
+	i2c@7000c000 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
+
+	i2c@7000c400 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
+
+	i2c@7000c500 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
+
+	i2c@7000c700 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
+
+	i2c@7000d000 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
 };