[U-Boot] [PATCH v2 0/3] Add I2C driver for T114 Dalmore

[U-Boot] [PATCH v2 0/3] Add I2C driver for T114 Dalmore

[Tom Warren]

Update DT tables and enable I2C driver support for the
Tegra114 Dalmore board. This uses the standard Tegra I2C driver.
5 controllers are supported, although all may not have devices
behind them on every board.

Changes in V2:
- use new calculation for T114 I2C clocks
- incorporate review comments from StephenW for the dtsi file

Tom Warren (3):
  Tegra: I2C: Add T114 clock support to tegra_i2c driver
  Tegra114: fdt: Update DT files with I2C info for T114/Dalmore
  Tegra114: I2C: Enable I2C driver on Dalmore E1611 eval board

 arch/arm/dts/tegra114.dtsi                  |   56 +++++++++++++++++++++++++++
 arch/arm/include/asm/arch-tegra/tegra_i2c.h |    6 +++
 board/nvidia/dts/tegra114-dalmore.dts       |   33 ++++++++++++++++
 drivers/i2c/tegra_i2c.c                     |   22 ++++++++++-
 include/configs/dalmore.h                   |    9 ++++
 include/configs/tegra114-common.h           |    3 +
 6 files changed, 128 insertions(+), 1 deletions(-)

[U-Boot] [PATCH v2 1/3] Tegra: I2C: Add T114 clock support to tegra_i2c driver

[Tom Warren]

T114 has a slightly different I2C clock, with a new divisor for
standard/fast mode and HS mode. Tested on my Dalmore, and the I2C
clock is 100KHz +/- 3% on my Saleae Logic analyzer.

Signed-off-by: Tom Warren <twarren@nvidia.com>
---
v2: new

 arch/arm/include/asm/arch-tegra/tegra_i2c.h |    6 ++++++
 drivers/i2c/tegra_i2c.c                     |   22 +++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/arch-tegra/tegra_i2c.h b/arch/arm/include/asm/arch-tegra/tegra_i2c.h
index 2650744..853e59b 100644
--- a/arch/arm/include/asm/arch-tegra/tegra_i2c.h
+++ b/arch/arm/include/asm/arch-tegra/tegra_i2c.h
@@ -105,6 +105,7 @@ struct i2c_ctlr {
 	u32 sl_delay_count;		/* 3C: I2C_I2C_SL_DELAY_COUNT */
 	u32 reserved_2[4];		/* 40: */
 	struct i2c_control control;	/* 50 ~ 68 */
+	u32 clk_div;			/* 6C: I2C_I2C_CLOCK_DIVISOR */
 };
 
 /* bit fields definitions for IO Packet Header 1 format */
@@ -154,6 +155,11 @@ struct i2c_ctlr {
 #define I2C_INT_ARBITRATION_LOST_SHIFT	2
 #define I2C_INT_ARBITRATION_LOST_MASK	(1 << I2C_INT_ARBITRATION_LOST_SHIFT)
 
+/* I2C_CLK_DIVISOR_REGISTER */
+#define CLK_DIV_STD_FAST_MODE		0x19
+#define CLK_DIV_HS_MODE			1
+#define CLK_MULT_STD_FAST_MODE		8
+
 /**
  * Returns the bus number of the DVC controller
  *
diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c
index efc77fa..0558648 100644
--- a/drivers/i2c/tegra_i2c.c
+++ b/drivers/i2c/tegra_i2c.c
@@ -88,7 +88,27 @@ static void i2c_init_controller(struct i2c_bus *i2c_bus)
 	 * 16 to get the right frequency.
 	 */
 	clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_PERIPH,
-			       i2c_bus->speed * 2 * 8);
+		i2c_bus->speed * 2 * 8);
+#if defined(CONFIG_TEGRA114)
+	/*
+	 * T114 I2C went to a single clock source for standard/fast and
+	 * HS clock speeds. The new clock rate setting calculation is:
+	 *  SCL = CLK_SOURCE.I2C /
+	 *   (CLK_MULT_STD_FAST_MODE * (I2C_CLK_DIV_STD_FAST_MODE+1) *
+	 *   I2C FREQUENCY DIVISOR) as per the T114 TRM (sec 30.3.1).
+	 *
+	 * NOTE: We do this here, after the initial clock/pll start,
+	 * because if we read the clk_div reg before the controller
+	 * is running, we hang, and we need it for the new calc.
+	 */
+	int clk_div_std_fast_mode = readl(&i2c_bus->regs->clk_div) >> 16;
+	debug("%s: CLK_DIV_STD_FAST_MODE setting = %d\n", __func__,
+		clk_div_std_fast_mode);
+
+	clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_PERIPH,
+		CLK_MULT_STD_FAST_MODE * (clk_div_std_fast_mode+1) *
+		i2c_bus->speed * 2);
+#endif	/* T114 */
 
 	/* Reset I2C controller. */
 	i2c_reset_controller(i2c_bus);
-- 
1.7.0.4

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

[Tom Warren]

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

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 at 60006000 {
+		compatible = "nvidia,tegra114-car, nvidia,tegra30-car";
+		reg = <0x60006000 0x1000>;
+		#clock-cells = <1>;
+	};
+
+	i2c at 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 at 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 at 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 at 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 at 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 at 7000d000";
+		i2c1 = "/i2c at 7000c000";
+		i2c2 = "/i2c at 7000c400";
+		i2c3 = "/i2c at 7000c500";
+		i2c4 = "/i2c at 7000c700";
+	};
+
 	memory {
 		device_type = "memory";
 		reg = <0x80000000 0x80000000>;
 	};
+
+	i2c at 7000c000 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
+
+	i2c at 7000c400 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
+
+	i2c at 7000c500 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
+
+	i2c at 7000c700 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
+
+	i2c at 7000d000 {
+		status = "okay";
+		clock-frequency = <100000>;
+	};
 };
-- 
1.7.0.4

[U-Boot] [PATCH v2 3/3] Tegra114: I2C: Enable I2C driver on Dalmore E1611 eval board

[Tom Warren]

Tested all 5 'buses', i2c probe enumerates device addresses on bus
1 and 2.

Signed-off-by: Tom Warren <twarren@nvidia.com>
---
v2: No change

 include/configs/dalmore.h         |    9 +++++++++
 include/configs/tegra114-common.h |    3 +++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/include/configs/dalmore.h b/include/configs/dalmore.h
index ce32c80..b1a6e34 100644
--- a/include/configs/dalmore.h
+++ b/include/configs/dalmore.h
@@ -41,6 +41,15 @@
 #define CONFIG_MACH_TYPE		MACH_TYPE_DALMORE
 
 #define CONFIG_BOARD_EARLY_INIT_F
+
+/* I2C */
+#define CONFIG_TEGRA_I2C
+#define CONFIG_SYS_I2C_INIT_BOARD
+#define CONFIG_I2C_MULTI_BUS
+#define CONFIG_SYS_MAX_I2C_BUS		TEGRA_I2C_NUM_CONTROLLERS
+#define CONFIG_SYS_I2C_SPEED		100000
+#define CONFIG_CMD_I2C
+
 #define CONFIG_ENV_IS_NOWHERE
 
 #define MACH_TYPE_DALMORE	4304	/* not yet in mach-types.h */
diff --git a/include/configs/tegra114-common.h b/include/configs/tegra114-common.h
index 0033530..c2986d8 100644
--- a/include/configs/tegra114-common.h
+++ b/include/configs/tegra114-common.h
@@ -76,4 +76,7 @@
 
 #define CONFIG_SPL_LDSCRIPT		"$(CPUDIR)/tegra114/u-boot-spl.lds"
 
+/* Total I2C ports on Tegra114 */
+#define TEGRA_I2C_NUM_CONTROLLERS	5
+
 #endif /* _TEGRA114_COMMON_H_ */
-- 
1.7.0.4

[U-Boot] [PATCH v2 1/3] Tegra: I2C: Add T114 clock support to tegra_i2c driver

[Stephen Warren]

On 02/06/2013 04:26 PM, Tom Warren wrote:
> T114 has a slightly different I2C clock, with a new divisor for
> standard/fast mode and HS mode. Tested on my Dalmore, and the I2C
> clock is 100KHz +/- 3% on my Saleae Logic analyzer.

This looks plausible to me, but best to have Laxman review it since he's
the I2C expert. I'll forward the whole original message to him for this
purpose.

At least for me though,
Reviewed-by: Stephen Warren <swarren@nvidia.com>

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

[Stephen Warren]

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.

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

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

[U-Boot] [PATCH v2 3/3] Tegra114: I2C: Enable I2C driver on Dalmore E1611 eval board

[Stephen Warren]

On 02/06/2013 04:26 PM, Tom Warren wrote:
> Tested all 5 'buses', i2c probe enumerates device addresses on bus
> 1 and 2.

This patch,
Reviewed-by: Stephen Warren <swarren@nvidia.com>

[U-Boot] [PATCH v2 1/3] Tegra: I2C: Add T114 clock support to tegra_i2c driver

[Laxman Dewangan]

On Thursday 07 February 2013 04:56 AM, Tom Warren wrote:
> T114 has a slightly different I2C clock, with a new divisor for
> standard/fast mode and HS mode. Tested on my Dalmore, and the I2C
> clock is 100KHz +/- 3% on my Saleae Logic analyzer.
>
> Signed-off-by: Tom Warren <twarren@nvidia.com>
> ---
>

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

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

[Laxman Dewangan]

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 at 7000d000";
> +		i2c1 =/i2c at 7000c000";
> +		i2c2 =/i2c at 7000c400";
> +		i2c3 =/i2c at 7000c500";
> +		i2c4 =/i2c at 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.
-----------------------------------------------------------------------------------

[U-Boot] [PATCH v2 3/3] Tegra114: I2C: Enable I2C driver on Dalmore E1611 eval board

[Laxman Dewangan]

On Thursday 07 February 2013 04:56 AM, Tom Warren wrote:
> Tested all 5 'buses', i2c probe enumerates device addresses on bus
> 1 and 2.
>
> Signed-off-by: Tom Warren <twarren@nvidia.com>
> ---
> v2: No change
>
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.
-----------------------------------------------------------------------------------

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

[Tom Warren]

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

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

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

Tom

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

[Tom Warren]

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 at 7000d000";
>> +               i2c1 =/i2c at 7000c000";
>> +               i2c2 =/i2c at 7000c400";
>> +               i2c3 =/i2c at 7000c500";
>> +               i2c4 =/i2c at 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.
> -----------------------------------------------------------------------------------

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

[Stephen Warren]

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 at 7000d000";
>>> +               i2c1 =/i2c at 7000c000";
>>> +               i2c2 =/i2c at 7000c400";
>>> +               i2c3 =/i2c at 7000c500";
>>> +               i2c4 =/i2c at 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...

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

[Tom Warren]

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 at 7000d000";
>>>> +               i2c1 =/i2c at 7000c000";
>>>> +               i2c2 =/i2c at 7000c400";
>>>> +               i2c3 =/i2c at 7000c500";
>>>> +               i2c4 =/i2c at 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

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

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

[Stephen Warren]

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 at 7000d000";
>>>>> +               i2c1 =/i2c at 7000c000";
>>>>> +               i2c2 =/i2c at 7000c400";
>>>>> +               i2c3 =/i2c at 7000c500";
>>>>> +               i2c4 =/i2c at 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.

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

[Tom Warren]

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 at 7000d000";
>>>>>> +               i2c1 =/i2c at 7000c000";
>>>>>> +               i2c2 =/i2c at 7000c400";
>>>>>> +               i2c3 =/i2c at 7000c500";
>>>>>> +               i2c4 =/i2c at 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

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

[Stephen Warren]

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 at 7000d000";
>>>>>>> +               i2c1 =/i2c at 7000c000";
>>>>>>> +               i2c2 =/i2c at 7000c400";
>>>>>>> +               i2c3 =/i2c at 7000c500";
>>>>>>> +               i2c4 =/i2c at 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.

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

[Tom Warren]

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 at 7000d000";
>>>>>>>> +               i2c1 =/i2c at 7000c000";
>>>>>>>> +               i2c2 =/i2c at 7000c400";
>>>>>>>> +               i2c3 =/i2c at 7000c500";
>>>>>>>> +               i2c4 =/i2c at 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

[U-Boot] [PATCH v2 1/3] Tegra: I2C: Add T114 clock support to tegra_i2c driver

[Laxman Dewangan]

On Thursday 07 February 2013 04:56 AM, Tom Warren wrote:
> T114 has a slightly different I2C clock, with a new divisor for
> standard/fast mode and HS mode. Tested on my Dalmore, and the I2C
> clock is 100KHz +/- 3% on my Saleae Logic analyzer.
>
> Signed-off-by: Tom Warren <twarren@nvidia.com>
> ---
> v2: new

>   	 */
>   	clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_PERIPH,
> -			       i2c_bus->speed * 2 * 8);
> +		i2c_bus->speed * 2 * 8);

I think you do not need to multipled by 2 again here. *2 can be remove.
I2C clock divder is U16 type.


>

> +
> +	clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_PERIPH,
> +		CLK_MULT_STD_FAST_MODE * (clk_div_std_fast_mode+1) *
> +		i2c_bus->speed * 2);

Same as above, *2 is not required.


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

[U-Boot] [PATCH v2 1/3] Tegra: I2C: Add T114 clock support to tegra_i2c driver

[Tom Warren]

Laxman,

On Fri, Feb 8, 2013 at 2:15 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> On Thursday 07 February 2013 04:56 AM, Tom Warren wrote:
>>
>> T114 has a slightly different I2C clock, with a new divisor for
>> standard/fast mode and HS mode. Tested on my Dalmore, and the I2C
>> clock is 100KHz +/- 3% on my Saleae Logic analyzer.
>>
>> Signed-off-by: Tom Warren <twarren@nvidia.com>
>> ---
>> v2: new
>
>
>>          */
>>         clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_PERIPH,
>> -                              i2c_bus->speed * 2 * 8);
>> +               i2c_bus->speed * 2 * 8);
>
>
> I think you do not need to multipled by 2 again here. *2 can be remove.
> I2C clock divder is U16 type.
>
>
>
>>
>
>> +
>> +       clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_PERIPH,
>> +               CLK_MULT_STD_FAST_MODE * (clk_div_std_fast_mode+1) *
>> +               i2c_bus->speed * 2);
>
>
> Same as above, *2 is not required.
I measured the I2C clock on J51 (GEN1_I2C) on my Dalmore, and the
clock is indeed 100KHz (+/- 3 Hz).

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

[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@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 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

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

[Tom Warren]

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

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

[Simon Glass]

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

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

[Stephen Warren]

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.

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

[Simon Glass]

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

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

[Tom Warren]

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.