[U-Boot] [PATCH v2 0/3] Add I2C driver for T114 Dalmore
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(-)
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);
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
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 Dewanganldewangan@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. -----------------------------------------------------------------------------------
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. -----------------------------------------------------------------------------------
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.
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@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>; + }; };
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.
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
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).
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
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
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
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.
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
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.
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. -----------------------------------------------------------------------------------
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.
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...
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
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.
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
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.
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
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_ */
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
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 Dewanganldewangan@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. -----------------------------------------------------------------------------------
participants (4)
-
Laxman Dewangan -
Simon Glass -
Stephen Warren -
Tom Warren