tegra: Add I2C driver and associated parts

Some devices can deal with multiple compatible properties. The devices need to know which nodes to bind to which features. For example an I2C driver which supports two different controller types will want to know which type it is dealing with in each case.

The new fdtdec_add_aliases_for_id() function deals with this by allowing the driver to search for additional compatible nodes for a different ID. It can then detect the new ones and perform appropriate processing.

Another option considered was to return a tuple (node offset, compat id) and have the function be passed a list of compatible IDs. This is more overhead for the common case though. We may add such a function later if more drivers in U-Boot require it.

lib/fdtdec.c +2 -0

+int fdtdec_add_aliases_for_id(const void *blob, const char *name,
+			enum fdt_compat_id id, int *node_list, int maxcount)

lib/fdtdec.c +1 -0

@@ -232,11 +238,13 @@ int fdtdec_find_aliases_for_id(const void *blob, const char *name,
 		 * it as done.
 		 */
 		if (fdtdec_get_is_enabled(blob, node)) {
+			if (node_list[number])

Quoted 16 lines · Simon Glass

Some devices can deal with multiple compatible properties. The devices need to know which nodes to bind to which features. For example an I2C driver which supports two different controller types will want to know which type it is dealing with in each case.

The new fdtdec_add_aliases_for_id() function deals with this by allowing the driver to search for additional compatible nodes for a different ID. It can then detect the new ones and perform appropriate processing.

Another option considered was to return a tuple (node offset, compat id) and have the function be passed a list of compatible IDs. This is more overhead for the common case though. We may add such a function later if more drivers in U-Boot require it.

+int fdtdec_add_aliases_for_id(const void *blob, const char *name,
+                     enum fdt_compat_id id, int *node_list, int maxcount)

...

Quoted 14 lines

lib/fdtdec.c +3 -1

@@ -232,11 +238,13 @@ int fdtdec_find_aliases_for_id(const void *blob, const char *name,
                * it as done.
                */
               if (fdtdec_get_is_enabled(blob, node)) {
+                     if (node_list[number])
+                             continue;
                       node_list[number] = node;
                       if (number >= num_found)
                               num_found = number + 1;
               }
-             nodes[j] = 0;
+             nodes[found] = 0;
       }
 

lib/fdtdec.c int fdtdec_find_aliases_for_id(const void *blob, const char *name, · line 250

Quoted 1 line

lib/fdtdec.c +0 -0

       /* Add any nodes not mentioned by an alias */

Aren't those two changes really bug-fixes to the underlying patch rather than part of this enhancement?

I'm not convinced this patch is correct in all cases. It'll probably work fine for simply device-trees though. Consider the following case:

4 device nodes
A: compat=i2c alias for usb0
B: compat=i2c no alias
C: compat=i2c alias for usb2
D: compat=i2cx alias for usb1

First, we search for all compat=i2c, the list comes back:

0=A
1=B
2=C

Then we search for all compat=i2cx, the list comes back:

-1
-1
-1
D

So D's alias of 1 isn't honored even though if both IDs were searched for together, it would have been.

Also, what about the scenario where a driver searches for both nvidia,tegra20-i2c then later nvidia,tegra30-i2c, given that all the DT nodes are probably compatible with both?

--
nvpublic

Hi Stephen,

Quoted 14 lines · Stephen Warren

Quoted 13 lines · Simon Glass

Some devices can deal with multiple compatible properties. The devices need to know which nodes to bind to which features. For example an I2C driver which supports two different controller types will want to know which type it is dealing with in each case.

The new fdtdec_add_aliases_for_id() function deals with this by allowing the driver to search for additional compatible nodes for a different ID. It can then detect the new ones and perform appropriate processing.

Another option considered was to return a tuple (node offset, compat id) and have the function be passed a list of compatible IDs. This is more overhead for the common case though. We may add such a function later if more drivers in U-Boot require it.

Quoted 17 lines

I'm not convinced this patch is correct in all cases. It'll probably work fine for simply device-trees though. Consider the following case:

4 device nodes
A: compat=i2c alias for usb0
B: compat=i2c no alias
C: compat=i2c alias for usb2
D: compat=i2cx alias for usb1

First, we search for all compat=i2c, the list comes back:

0=A
1=B
2=C

Then we search for all compat=i2cx, the list comes back:

Quoted 7 lines

-1
-1
-1
D

So D's alias of 1 isn't honored even though if both IDs were searched for together, it would have been.

Do we really want that behaviour? Overall I think it is a bit odd to have devices with no aliases if you actually care about the ordering. At least in U-Boot ordering is important. The simple fix above is to provide an alias for everything, which is what we do. If we do want the behaviour you suggest then I will need the change to doing it in one pass.

Quoted 3 lines

Also, what about the scenario where a driver searches for both nvidia,tegra20-i2c then later nvidia,tegra30-i2c, given that all the DT nodes are probably compatible with both?

If all DT notes are compatible with both, why are you searching for one and then the other? Why not just search for one?

Quoted 17 lines · Simon Glass

Hi Stephen,

Quoted 14 lines · Stephen Warren

Quoted 13 lines · Simon Glass

Some devices can deal with multiple compatible properties. The devices need to know which nodes to bind to which features. For example an I2C driver which supports two different controller types will want to know which type it is dealing with in each case.

The new fdtdec_add_aliases_for_id() function deals with this by allowing the driver to search for additional compatible nodes for a different ID. It can then detect the new ones and perform appropriate processing.

Another option considered was to return a tuple (node offset, compat id) and have the function be passed a list of compatible IDs. This is more overhead for the common case though. We may add such a function later if more drivers in U-Boot require it.

...

Quoted 25 lines

Quoted 17 lines

I'm not convinced this patch is correct in all cases. It'll probably work fine for simply device-trees though. Consider the following case:

4 device nodes
A: compat=i2c alias for usb0
B: compat=i2c no alias
C: compat=i2c alias for usb2
D: compat=i2cx alias for usb1

First, we search for all compat=i2c, the list comes back:

0=A
1=B
2=C

Then we search for all compat=i2cx, the list comes back:

Quoted 7 lines

-1
-1
-1
D

So D's alias of 1 isn't honored even though if both IDs were searched for together, it would have been.

Quoted 6 lines

Do we really want that behaviour? Overall I think it is a bit odd to have devices with no aliases if you actually care about the ordering. At least in U-Boot ordering is important. The simple fix above is to provide an alias for everything, which is what we do. If we do want the behaviour you suggest then I will need the change to doing it in one pass.

I think that behaviour is the intended given that DT content. Despite the fact that content is admittedly a little twisted and perhaps not commonly useful, since this is user-supplied content in general, I don't think we can simply not accept it or not implement the fairly obvious intent.

Quoted 6 lines

Quoted 3 lines

Also, what about the scenario where a driver searches for both nvidia,tegra20-i2c then later nvidia,tegra30-i2c, given that all the DT nodes are probably compatible with both?

If all DT notes are compatible with both, why are you searching for one and then the other? Why not just search for one?

I suppose one /shouldn't/ need to.

I was thinking of supporting the case where somebody only wrote compatible = "nvidia,tegra30-i2c" and left out the Tegra20 variant. Still, is arguably a bug, and that won't work in the kernel either since we're not going around adding all the new compatible values into the drivers when the devices are compatible with the existing HW anyway.

I guess if we do get some legitimate case that trips this issue, we can just fix it then, since it's entirely isolated to the code and doesn't impact anything externally visible etc.

--
nvpublic

Add U-Boot's peripheral clock information to the Tegra20 device tree file.

Signed-off-by Simon Glass <[email protected]>

---
Changes in v2:
- Adjust definitions to fit new peripheral clock bindings
- Change 'speed' to 'clock-frequency'
- Remove u-boot,pinmux binding (sadly)

 arch/arm/dts/tegra20.dtsi |   10 +++++++++-

Hello Simon,

Quoted 9 lines · Simon Glass

Add U-Boot's peripheral clock information to the Tegra20 device tree file.

Signed-off-by Simon Glass <[email protected]>

---
Changes in v2:
- Adjust definitions to fit new peripheral clock bindings
- Change 'speed' to 'clock-frequency'
- Remove u-boot,pinmux binding (sadly)

Quoted 1 line

 arch/arm/dts/tegra20.dtsi |   10 +++++++++-

Hmm.. we have no such a directory in the u-boot source tree ... ? I think, this patch is for the linux tree?

[...]

bye,
Heiko
--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

arch/arm/dts/tegra20.dtsi line 48

Quoted 11 lines · Simon Glass

Add U-Boot's peripheral clock information to the Tegra20 device tree file.

arch/arm/dts/tegra20.dtsi +1 -0

index ca7b523..963cf27 100644
@@ -45,6 +45,8 @@
               compatible = "nvidia,tegra20-i2c";
               reg = <0x7000C000 0x100>;
               interrupts = < 70 >;
+             clock-frequency = <100000>;

That's board-specific information. I'd rather not include it in the SoC .dtsi file even as a default; that way, it forces people to think about the correct value and put it in their board file, whereas including a default means people probably won't think about it.

arch/arm/dts/tegra20.dtsi line 49

Quoted 2 lines

arch/arm/dts/tegra20.dtsi +1 -0

+             clocks = <&periph_clk 12>;      // PERIPH_ID_I2C1

Of course, that'll need updating based on the resolution of the clock binding thread I started.

--
nvpublic

Hi Stephen,

Quoted 17 lines · Stephen Warren

arch/arm/dts/tegra20.dtsi line 48

Quoted 11 lines · Simon Glass

Add U-Boot's peripheral clock information to the Tegra20 device tree file.

arch/arm/dts/tegra20.dtsi +1 -0

index ca7b523..963cf27 100644
@@ -45,6 +45,8 @@
              compatible = "nvidia,tegra20-i2c";
              reg = <0x7000C000 0x100>;
              interrupts = < 70 >;
+             clock-frequency = <100000>;

That's board-specific information. I'd rather not include it in the SoC .dtsi file even as a default; that way, it forces people to think about the correct value and put it in their board file, whereas including a default means people probably won't think about it.

OK I moved this into the board file - sadly this means everyone will need to specify it here. Or should we have a default?

Quoted 21 lines · Simon Glass

Hi Stephen,

Quoted 17 lines · Stephen Warren

arch/arm/dts/tegra20.dtsi line 48

Quoted 11 lines · Simon Glass

Add U-Boot's peripheral clock information to the Tegra20 device tree file.

arch/arm/dts/tegra20.dtsi +1 -0

index ca7b523..963cf27 100644
@@ -45,6 +45,8 @@
               compatible = "nvidia,tegra20-i2c";
               reg = <0x7000C000 0x100>;
               interrupts = < 70 >;
+             clock-frequency = <100000>;

That's board-specific information. I'd rather not include it in the SoC .dtsi file even as a default; that way, it forces people to think about the correct value and put it in their board file, whereas including a default means people probably won't think about it.

Quoted 2 lines

OK I moved this into the board file - sadly this means everyone will need to specify it here. Or should we have a default?

I think I'd rather not have the default. That way, people need to think about the correct value to use in their board file, rather than having it magically work without having validated the value.

If people disagree, we'd need to update the kernel's tegra[23]0.dtsi, since they assume ${board}.dts will set this property.

--
nvpublic

From: Yen Lin <[email protected]>

Add basic i2c driver for Tegra2 with 8- and 16-bit address support. The driver requires CONFIG_OF_CONTROL to obtain its configuration from the device tree.

(Simon Glass: sjg at chromium.org modified for upstream)

Signed-off-by Simon Glass <[email protected]>

---
Changes in v2:
- Use DIV_ROUND_UP() instead of a home-grown macro
- Tidy comment style
- Change i2c array to static
- Adjust definitions to fit new peripheral clock bindings
- Remove i2c configuring using CONFIG (use fdt instead)

- Make i2c/dvc decision come from fdt
- Use new fdtdec alias decode function
- Simplify code in i2c_addr_ok()
- Return an error if an unavailable i2c bus is selected

 arch/arm/include/asm/arch-tegra2/tegra2_i2c.h |  160 +++++++
 drivers/i2c/Makefile                          |    1 +
 drivers/i2c/tegra2_i2c.c                      |  551 +++++++++++++++++++++++++
 include/fdtdec.h                              |    2 +
 lib/fdtdec.c                                  |    2 +
 5 files changed, 716 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-tegra2/tegra2_i2c.h
 create mode 100644 drivers/i2c/tegra2_i2c.c

drivers/i2c/tegra2_i2c.c new file +0 -0

new file mode 100644
index 0000000..a7db714
@@ -0,0 +1,551 @@

drivers/i2c/tegra2_i2c.c new file +68 -0

+/*
+ * Process a list of nodes, adding them to our list of I2C ports.
+ *
+ * @param blob		fdt blob
+ * @param node_list	list of nodes to process (any <=0 are ignored)
+ * @param count		number of nodes to process
+ * @param is_dvc	1 if these are DVC ports, 0 if standard I2C
+ * @return 0 if ok, -1 on error
+ */
+static int process_nodes(const void *blob, int node_list[], int count,
+			 int is_dvc)
+{
+	struct i2c_bus *i2c_bus;
+	int i;
+
+	/* build the i2c_controllers[] for each controller */
+	for (i = 0; i < count; i++) {
+		int node = node_list[i];
+
+		if (node <= 0)
+			continue;
+
+		i2c_bus = &i2c_controllers[i];
+		i2c_bus->id = i;
+
+		if (i2c_get_config(blob, node, i2c_bus)) {
+			printf("i2c_init_board: failed to decode bus %d\n", i);
+			return -1;
+		}
+
+		i2c_bus->is_dvc = is_dvc;
+		if (is_dvc) {
+			i2c_bus->control =
+				&((struct dvc_ctlr *)i2c_bus->regs)->control;
+		} else {
+			i2c_bus->control = &i2c_bus->regs->control;
+		}
+		debug("%s: controller bus %d at %p, periph_id %d, speed %d: ",
+		      is_dvc ? "dvc" : "i2c", i, i2c_bus->regs,
+		      i2c_bus->periph_id, i2c_bus->speed);
+		i2c_init_controller(i2c_bus);
+		debug("ok\n");
+		i2c_bus->inited = 1;
+
+		/* Mark position as used */
+		node_list[i] = -1;
+	}
+
+	return 0;
+}
+
+int i2c_init_board(void)
+{
+	int node_list[CONFIG_SYS_MAX_I2C_BUS];
+	const void *blob = gd->fdt_blob;
+	int count;
+
+	/* First get the normal i2c ports */
+	count = fdtdec_find_aliases_for_id(blob, "i2c",
+			COMPAT_NVIDIA_TEGRA20_I2C, node_list,
+			CONFIG_SYS_MAX_I2C_BUS);
+	if (process_nodes(blob, node_list, count, 0))
+		return -1;
+
+	/* Now look for dvc ports */
+	count = fdtdec_add_aliases_for_id(blob, "i2c",
+			COMPAT_NVIDIA_TEGRA20_DVC, node_list,
+			CONFIG_SYS_MAX_I2C_BUS);

Hello Simon,

Quoted 8 lines · Simon Glass

From: Yen Lin <[email protected]>

Add basic i2c driver for Tegra2 with 8- and 16-bit address support. The driver requires CONFIG_OF_CONTROL to obtain its configuration from the device tree.

(Simon Glass: sjg at chromium.org modified for upstream)

Quoted 8 lines

Signed-off-by Simon Glass <[email protected]>

---
Changes in v2:
- Use DIV_ROUND_UP() instead of a home-grown macro
- Tidy comment style
- Change i2c array to static
- Adjust definitions to fit new peripheral clock bindings
- Remove i2c configuring using CONFIG (use fdt instead)

Why? Ah found it ... Hmm.. why we don't need the non OF case?

Quoted 14 lines

- Make i2c/dvc decision come from fdt
- Use new fdtdec alias decode function
- Simplify code in i2c_addr_ok()
- Return an error if an unavailable i2c bus is selected

?arch/arm/include/asm/arch-tegra2/tegra2_i2c.h | ?160 +++++++
?drivers/i2c/Makefile ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
?drivers/i2c/tegra2_i2c.c ? ? ? ? ? ? ? ? ? ? ?| ?551 +++++++++++++++++++++++++
?include/fdtdec.h ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?2 +
?lib/fdtdec.c ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?2 +

?5 files changed, 716 insertions(+), 0 deletions(-) ?create mode 100644 arch/arm/include/asm/arch-tegra2/tegra2_i2c.h ?create mode 100644 drivers/i2c/tegra2_i2c.c

[...]

Quoted 6 lines

drivers/i2c/tegra2_i2c.c new file +0 -0

new file mode 100644
index 0000000..a7db714
@@ -0,0 +1,551 @@

[...]

drivers/i2c/tegra2_i2c.c

Quoted 5 lines

drivers/i2c/tegra2_i2c.c new file +5 -0

+static void i2c_init_controller(struct i2c_bus *i2c_bus)
+{
+ ? ? /* TODO: Fix bug which makes us need to do this */
+ ? ? clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_OSC,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?i2c_bus->speed * (8 * 2 - 1));

? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? Can you use here some defines? What is (8 * 2 - 1) ?

drivers/i2c/tegra2_i2c.c line 297

Quoted 4 lines

drivers/i2c/tegra2_i2c.c new file +3 -0

+#ifndef CONFIG_OF_CONTROL
+#error "Please enable device tree support to use this driver"
+#endif

Hmm.. see above question. Ok, if somebody need want to use this driver without CONFIG_OF_CONTROL it must be added ...

Quoted 69 lines

drivers/i2c/tegra2_i2c.c new file +68 -0

+/*
+ * Process a list of nodes, adding them to our list of I2C ports.
+ *
+ * @param blob ? ? ? ? ? ? ? fdt blob
+ * @param node_list ?list of nodes to process (any <=0 are ignored)
+ * @param count ? ? ? ? ? ? ?number of nodes to process
+ * @param is_dvc ? ? 1 if these are DVC ports, 0 if standard I2C
+ * @return 0 if ok, -1 on error
+ */
+static int process_nodes(const void *blob, int node_list[], int count,
+ ? ? ? ? ? ? ? ? ? ? ?int is_dvc)
+{
+ ? ? struct i2c_bus *i2c_bus;
+ ? ? int i;
+
+ ? ? /* build the i2c_controllers[] for each controller */
+ ? ? for (i = 0; i < count; i++) {
+ ? ? ? ? ? ? int node = node_list[i];
+
+ ? ? ? ? ? ? if (node <= 0)
+ ? ? ? ? ? ? ? ? ? ? continue;
+
+ ? ? ? ? ? ? i2c_bus = &i2c_controllers[i];
+ ? ? ? ? ? ? i2c_bus->id = i;
+
+ ? ? ? ? ? ? if (i2c_get_config(blob, node, i2c_bus)) {
+ ? ? ? ? ? ? ? ? ? ? printf("i2c_init_board: failed to decode bus %d\n", i);
+ ? ? ? ? ? ? ? ? ? ? return -1;
+ ? ? ? ? ? ? }
+
+ ? ? ? ? ? ? i2c_bus->is_dvc = is_dvc;
+ ? ? ? ? ? ? if (is_dvc) {
+ ? ? ? ? ? ? ? ? ? ? i2c_bus->control =
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? &((struct dvc_ctlr *)i2c_bus->regs)->control;
+ ? ? ? ? ? ? } else {
+ ? ? ? ? ? ? ? ? ? ? i2c_bus->control = &i2c_bus->regs->control;
+ ? ? ? ? ? ? }
+ ? ? ? ? ? ? debug("%s: controller bus %d at %p, periph_id %d, speed %d: ",
+ ? ? ? ? ? ? ? ? ? is_dvc ? "dvc" : "i2c", i, i2c_bus->regs,
+ ? ? ? ? ? ? ? ? ? i2c_bus->periph_id, i2c_bus->speed);
+ ? ? ? ? ? ? i2c_init_controller(i2c_bus);
+ ? ? ? ? ? ? debug("ok\n");
+ ? ? ? ? ? ? i2c_bus->inited = 1;
+
+ ? ? ? ? ? ? /* Mark position as used */
+ ? ? ? ? ? ? node_list[i] = -1;
+ ? ? }
+
+ ? ? return 0;
+}
+
+int i2c_init_board(void)
+{
+ ? ? int node_list[CONFIG_SYS_MAX_I2C_BUS];
+ ? ? const void *blob = gd->fdt_blob;
+ ? ? int count;
+
+ ? ? /* First get the normal i2c ports */
+ ? ? count = fdtdec_find_aliases_for_id(blob, "i2c",
+ ? ? ? ? ? ? ? ? ? ? COMPAT_NVIDIA_TEGRA20_I2C, node_list,
+ ? ? ? ? ? ? ? ? ? ? CONFIG_SYS_MAX_I2C_BUS);
+ ? ? if (process_nodes(blob, node_list, count, 0))
+ ? ? ? ? ? ? return -1;
+
+ ? ? /* Now look for dvc ports */
+ ? ? count = fdtdec_add_aliases_for_id(blob, "i2c",
+ ? ? ? ? ? ? ? ? ? ? COMPAT_NVIDIA_TEGRA20_DVC, node_list,
+ ? ? ? ? ? ? ? ? ? ? CONFIG_SYS_MAX_I2C_BUS);

drivers/i2c/tegra2_i2c.c

Quoted 10 lines

drivers/i2c/tegra2_i2c.c new file +10 -0

+ ? ? if (process_nodes(blob, node_list, count, 1))
+ ? ? ? ? ? ? return -1;
+
+ ? ? return 0;
+}
+
+void i2c_init(int speed, int slaveaddr)
+{
+ ? ? debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
+}

Hmm... i2c_init is called to init the i2c subsystem ... you do nothing here ... and use i2c_init_board for init the i2c bus, right?

But i2c_init_board is not called from the driver ... ah, you do this in board code ... Ok ...

I think, you do this, because i2c_init is called very early, and so processing fdt is slow?

[...]

bye,
Heiko
--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Hi Heiko,

Quoted 19 lines · Heiko Schocher

Hello Simon,

Quoted 16 lines · Simon Glass

From: Yen Lin <[email protected]>

Add basic i2c driver for Tegra2 with 8- and 16-bit address support. The driver requires CONFIG_OF_CONTROL to obtain its configuration from the device tree.

(Simon Glass: sjg at chromium.org modified for upstream)

Signed-off-by Simon Glass <[email protected]>

---
Changes in v2:
- Use DIV_ROUND_UP() instead of a home-grown macro
- Tidy comment style
- Change i2c array to static
- Adjust definitions to fit new peripheral clock bindings
- Remove i2c configuring using CONFIG (use fdt instead)

Quoted 2 lines

Why? Ah found it ... Hmm.. why we don't need the non OF case?

The intent is that Tegra boards will run with fdt turned on. It means that we should be able to build U-Boot for a Tegra platform using the Linux fdt file and not lots of manual config. Of course this works better for some peripherals than others, but I2C works well enough.

Quoted 21 lines

Quoted 13 lines

- Make i2c/dvc decision come from fdt
- Use new fdtdec alias decode function
- Simplify code in i2c_addr_ok()
- Return an error if an unavailable i2c bus is selected

 arch/arm/include/asm/arch-tegra2/tegra2_i2c.h |  160 +++++++
 drivers/i2c/Makefile                          |    1 +
 drivers/i2c/tegra2_i2c.c                      |  551 +++++++++++++++++++++++++
 include/fdtdec.h                              |    2 +
 lib/fdtdec.c                                  |    2 +
 5 files changed, 716 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-tegra2/tegra2_i2c.h
 create mode 100644 drivers/i2c/tegra2_i2c.c

[...]

Quoted 6 lines

drivers/i2c/tegra2_i2c.c new file +0 -0

new file mode 100644
index 0000000..a7db714
@@ -0,0 +1,551 @@

[...]

Quoted 7 lines

drivers/i2c/tegra2_i2c.c line 86

Quoted 5 lines

drivers/i2c/tegra2_i2c.c new file +5 -0

+static void i2c_init_controller(struct i2c_bus *i2c_bus)
+{
+     /* TODO: Fix bug which makes us need to do this */
+     clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_OSC,
+                            i2c_bus->speed * (8 * 2 - 1));

Can you use here some defines?
What is (8 * 2 - 1) ?

I will look up the bug and find out.

Quoted 5 lines

drivers/i2c/tegra2_i2c.c line 297

Quoted 3 lines

drivers/i2c/tegra2_i2c.c new file +3 -0

+#ifndef CONFIG_OF_CONTROL
+#error "Please enable device tree support to use this driver"
+#endif

Hmm.. see above question. Ok, if somebody need want to use this driver without CONFIG_OF_CONTROL it must be added ...

Yes and they need a way of getting the configuration in there. The fdt is nicer...

Quoted 10 lines

drivers/i2c/tegra2_i2c.c line 410

Quoted 4 lines

drivers/i2c/tegra2_i2c.c new file +4 -0

+void i2c_init(int speed, int slaveaddr)
+{
+     debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
+}

Hmm... i2c_init is called to init the i2c subsystem ... you do nothing here ... and use i2c_init_board for init the i2c bus, right?

But i2c_init_board is not called from the driver ... ah, you do this in board code ... Ok ...

Quoted 2 lines

I think, you do this, because i2c_init is called very early, and so processing fdt is slow?

That's not the main reason but it is true. We have no need of early I2C on the platform. Also we don't want to set the speed as this is defined individually per port in the fdt.

From: Yen Lin <[email protected]>

Add basic i2c driver for Tegra2 with 8- and 16-bit address support. The driver requires CONFIG_OF_CONTROL to obtain its configuration from the device tree.

(Simon Glass: sjg at chromium.org modified for upstream)

Signed-off-by Simon Glass <[email protected]>

Quoted 6 lines · Simon Glass

From: Yen Lin <[email protected]>

Add basic i2c driver for Tegra2 with 8- and 16-bit address support. The driver requires CONFIG_OF_CONTROL to obtain its configuration from the device tree.

Quoted 3 lines

(Simon Glass: sjg at chromium.org modified for upstream)

Signed-off-by Simon Glass <[email protected]>

Why isn't Yen's S-o-b line here, since he's listed as the patch author in git?

drivers/i2c/tegra2_i2c.c

Quoted 4 lines

drivers/i2c/tegra2_i2c.c +1 -0

 
+static struct i2c_bus i2c_controllers[CONFIG_SYS_MAX_I2C_BUS];

I still don't like using the wrong define here, but since I see you pushing the multi-bus stuff upstream, so I assume you'll come back and fix this when it makes a difference, so I won't object too much about this.

drivers/i2c/tegra2_i2c.c line 410

Quoted 5 lines

drivers/i2c/tegra2_i2c.c +4 -0

+void i2c_init(int speed, int slaveaddr)
+{
+ ? ? ? debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
+}

What was the resolution on making this do something correct instead of ignoring the request?

--
nvpublic

Quoted 3 lines

Why isn't Yen's S-o-b line here, since he's listed as the patch author in git?

It will work if he replies to this thread or the new patch that I sent out. I cannot add it for him.

Hi Simon,

Quoted 1 line · Simon Glass

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

Why isn't Yen's S-o-b line here, since he's listed as the patch author in git?

It will work if he replies to this thread or the new patch that I sent out. I cannot add it for him.

Don't know how to add a S-o-b line to the patch. Please add "Signed-off-by: Yen Lin <[email protected]>"

Thanks,
Yen

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

Select the port ordering for I2C on Seaboard.

Signed-off-by Simon Glass <[email protected]>

Acked-by Stephen Warren <[email protected]>

Quoted 4 lines · Simon Glass

Select the port ordering for I2C on Seaboard.

Signed-off-by Simon Glass <[email protected]>

Acked-by Stephen Warren <[email protected]>

This isn't the patch that I ack'd.

board/nvidia/dts/tegra2-seaboard.dts

Quoted 2 lines

board/nvidia/dts/tegra2-seaboard.dts +0 -0

Unrelated to this patch, but shouldn't that be tegra-seaboard.dts not tegra2-seaboard.dts to match the naming in the kernel?

board/nvidia/dts/tegra2-seaboard.dts line 57

Quoted 10 lines

board/nvidia/dts/tegra2-seaboard.dts +5 -0

@@ -44,4 +49,9 @@
 ? ? ? usb at c5004000 {
 ? ? ? ? ? ? ? status = "disabled";
 ? ? ? };
+
+ ? ? i2c at 7000c400 {
+ ? ? ? ? ? ? status = "disabled";
+ ? ? };
+
 ?};

That chunk wasn't in the original patch, and doesn't match the kernel's .dts file (and I believe that I2C controller really is in use, so shouldn't be disabled).

--
nvpublic

Hi Stephen,

Quoted 7 lines · Stephen Warren

Quoted 4 lines · Simon Glass

Select the port ordering for I2C on Seaboard.

Signed-off-by Simon Glass <[email protected]>

Acked-by Stephen Warren <[email protected]>

This isn't the patch that I ack'd.

Sorry, I added the disable.

Quoted 6 lines

board/nvidia/dts/tegra2-seaboard.dts

Quoted 2 lines

board/nvidia/dts/tegra2-seaboard.dts +0 -0

Unrelated to this patch, but shouldn't that be tegra-seaboard.dts not tegra2-seaboard.dts to match the naming in the kernel?

Quoted 14 lines

board/nvidia/dts/tegra2-seaboard.dts line 57

Quoted 10 lines

board/nvidia/dts/tegra2-seaboard.dts +5 -0

@@ -44,4 +49,9 @@
      usb at c5004000 {
              status = "disabled";
      };
+
+     i2c at 7000c400 {
+             status = "disabled";
+     };
+
 };

That chunk wasn't in the original patch, and doesn't match the kernel's .dts file (and I believe that I2C controller really is in use, so shouldn't be disabled).

It cannot be used - remember the discussion about pinmux? We elected to disable I2C1 at present since you didn't like my nvidia,pinmux binding for selecting which value to pass to funcmux. The fix is to pass 1 instead of 0 for that port, but we have no clean way to specify this.

Rather disable it than leave it enabled and not working.

Quoted 40 lines · Simon Glass

Hi Stephen,

Quoted 7 lines · Stephen Warren

Quoted 4 lines · Simon Glass

Select the port ordering for I2C on Seaboard.

Signed-off-by Simon Glass <[email protected]>

Acked-by Stephen Warren <[email protected]>

This isn't the patch that I ack'd.

Sorry, I added the disable.

Quoted 6 lines

board/nvidia/dts/tegra2-seaboard.dts

Quoted 2 lines

board/nvidia/dts/tegra2-seaboard.dts +0 -0

Unrelated to this patch, but shouldn't that be tegra-seaboard.dts not tegra2-seaboard.dts to match the naming in the kernel?

Quoted 14 lines

board/nvidia/dts/tegra2-seaboard.dts line 57

Quoted 10 lines

board/nvidia/dts/tegra2-seaboard.dts +5 -0

@@ -44,4 +49,9 @@
 ? ? ? usb at c5004000 {
 ? ? ? ? ? ? ? status = "disabled";
 ? ? ? };
+
+ ? ? i2c at 7000c400 {
+ ? ? ? ? ? ? status = "disabled";
+ ? ? };
+
 ?};

That chunk wasn't in the original patch, and doesn't match the kernel's .dts file (and I believe that I2C controller really is in use, so shouldn't be disabled).

It cannot be used - remember the discussion about pinmux? We elected to disable I2C1 at present since you didn't like my nvidia,pinmux binding for selecting which value to pass to funcmux. The fix is to pass 1 instead of 0 for that port, but we have no clean way to specify this.

Quoted 1 line

Rather disable it than leave it enabled and not working.

Rather than having the .dts file not correctly describe the HW, wouldn't it be better to limit U-Boot's to only initializing the 1 I2C controller that it knows the valid pinmux setting for?

--
nvpublic

Hi Stephen,

Quoted 43 lines · Stephen Warren

Quoted 41 lines · Simon Glass

Hi Stephen,

Quoted 7 lines · Stephen Warren

Quoted 4 lines · Simon Glass

Select the port ordering for I2C on Seaboard.

Signed-off-by Simon Glass <[email protected]>

Acked-by Stephen Warren <[email protected]>

This isn't the patch that I ack'd.

Sorry, I added the disable.

Quoted 6 lines

board/nvidia/dts/tegra2-seaboard.dts

Quoted 2 lines

board/nvidia/dts/tegra2-seaboard.dts +0 -0

Unrelated to this patch, but shouldn't that be tegra-seaboard.dts not tegra2-seaboard.dts to match the naming in the kernel?

Quoted 14 lines

board/nvidia/dts/tegra2-seaboard.dts line 57

Quoted 10 lines

board/nvidia/dts/tegra2-seaboard.dts +5 -0

@@ -44,4 +49,9 @@
      usb at c5004000 {
              status = "disabled";
      };
+
+     i2c at 7000c400 {
+             status = "disabled";
+     };
+
 };

That chunk wasn't in the original patch, and doesn't match the kernel's .dts file (and I believe that I2C controller really is in use, so shouldn't be disabled).

It cannot be used - remember the discussion about pinmux? We elected to disable I2C1 at present since you didn't like my nvidia,pinmux binding for selecting which value to pass to funcmux. The fix is to pass 1 instead of 0 for that port, but we have no clean way to specify this.

Rather disable it than leave it enabled and not working.

Quoted 3 lines

Rather than having the .dts file not correctly describe the HW, wouldn't it be better to limit U-Boot's to only initializing the 1 I2C controller that it knows the valid pinmux setting for?

Well, on Seaboard I2C2 (sorry, not I2C1 as I said in my email) is not used or connected to anything, so it is a reasonable description of the hardware. The other 3 pinmux settings are valid.

Quoted 50 lines · Simon Glass

Hi Stephen,

Quoted 46 lines · Stephen Warren

Quoted 34 lines · Simon Glass

Hi Stephen,

Quoted 7 lines · Stephen Warren

Quoted 4 lines · Simon Glass

Select the port ordering for I2C on Seaboard.

Signed-off-by Simon Glass <[email protected]>

Acked-by Stephen Warren <[email protected]>

This isn't the patch that I ack'd.

Sorry, I added the disable.

Quoted 6 lines

board/nvidia/dts/tegra2-seaboard.dts

Quoted 2 lines

board/nvidia/dts/tegra2-seaboard.dts +0 -0

Unrelated to this patch, but shouldn't that be tegra-seaboard.dts not tegra2-seaboard.dts to match the naming in the kernel?

Quoted 14 lines

board/nvidia/dts/tegra2-seaboard.dts line 57

Quoted 10 lines

board/nvidia/dts/tegra2-seaboard.dts +5 -0

@@ -44,4 +49,9 @@
 ? ? ? usb at c5004000 {
 ? ? ? ? ? ? ? status = "disabled";
 ? ? ? };
+
+ ? ? i2c at 7000c400 {
+ ? ? ? ? ? ? status = "disabled";
+ ? ? };
+
 ?};

That chunk wasn't in the original patch, and doesn't match the kernel's .dts file (and I believe that I2C controller really is in use, so shouldn't be disabled).

Quoted 7 lines

It cannot be used - remember the discussion about pinmux? We elected to disable I2C1 at present since you didn't like my nvidia,pinmux binding for selecting which value to pass to funcmux. The fix is to pass 1 instead of 0 for that port, but we have no clean way to specify this.

Rather disable it than leave it enabled and not working.

Rather than having the .dts file not correctly describe the HW, wouldn't it be better to limit U-Boot's to only initializing the 1 I2C controller that it knows the valid pinmux setting for?

Quoted 3 lines

Well, on Seaboard I2C2 (sorry, not I2C1 as I said in my email) is not used or connected to anything, so it is a reasonable description of the hardware. The other 3 pinmux settings are valid.

As best I can tell from my schematics, all 4 I2C ports are used on both Seaboard and Springbank.

--
nvpublic

Hi Stephen,

Quoted 55 lines · Stephen Warren

Quoted 53 lines · Simon Glass

Hi Stephen,

Quoted 41 lines · Stephen Warren

Quoted 40 lines · Simon Glass

Hi Stephen,

Quoted 7 lines · Stephen Warren

Quoted 4 lines · Simon Glass

Select the port ordering for I2C on Seaboard.

Signed-off-by Simon Glass <[email protected]>

Acked-by Stephen Warren <[email protected]>

This isn't the patch that I ack'd.

Sorry, I added the disable.

Quoted 6 lines

board/nvidia/dts/tegra2-seaboard.dts

Quoted 2 lines

board/nvidia/dts/tegra2-seaboard.dts +0 -0

Unrelated to this patch, but shouldn't that be tegra-seaboard.dts not tegra2-seaboard.dts to match the naming in the kernel?

Quoted 14 lines

board/nvidia/dts/tegra2-seaboard.dts line 57

Quoted 10 lines

board/nvidia/dts/tegra2-seaboard.dts +5 -0

@@ -44,4 +49,9 @@
      usb at c5004000 {
              status = "disabled";
      };
+
+     i2c at 7000c400 {
+             status = "disabled";
+     };
+
 };

That chunk wasn't in the original patch, and doesn't match the kernel's .dts file (and I believe that I2C controller really is in use, so shouldn't be disabled).

It cannot be used - remember the discussion about pinmux? We elected to disable I2C1 at present since you didn't like my nvidia,pinmux binding for selecting which value to pass to funcmux. The fix is to pass 1 instead of 0 for that port, but we have no clean way to specify this.

Quoted 5 lines

Quoted 1 line

Rather disable it than leave it enabled and not working.

Rather than having the .dts file not correctly describe the HW, wouldn't it be better to limit U-Boot's to only initializing the 1 I2C controller that it knows the valid pinmux setting for?

Well, on Seaboard I2C2 (sorry, not I2C1 as I said in my email) is not used or connected to anything, so it is a reasonable description of the hardware. The other 3 pinmux settings are valid.

Quoted 2 lines

As best I can tell from my schematics, all 4 I2C ports are used on both Seaboard and Springbank.

I don't know about Springbank, but this patch is for Seaboard.

There is nothing on the bus and I can't see anything on the schematic. Can you please tell me what it is used for and which pins you are referring to?

Quoted 58 lines · Simon Glass

Hi Stephen,

Quoted 55 lines · Stephen Warren

Quoted 53 lines · Simon Glass

Hi Stephen,

Quoted 41 lines · Stephen Warren

Quoted 40 lines · Simon Glass

Hi Stephen,

Quoted 7 lines · Stephen Warren

Quoted 4 lines · Simon Glass

Select the port ordering for I2C on Seaboard.

Signed-off-by Simon Glass <[email protected]>

Acked-by Stephen Warren <[email protected]>

This isn't the patch that I ack'd.

Sorry, I added the disable.

Quoted 6 lines

board/nvidia/dts/tegra2-seaboard.dts

Quoted 2 lines

board/nvidia/dts/tegra2-seaboard.dts +0 -0

Unrelated to this patch, but shouldn't that be tegra-seaboard.dts not tegra2-seaboard.dts to match the naming in the kernel?

Quoted 14 lines

board/nvidia/dts/tegra2-seaboard.dts line 57

Quoted 10 lines

board/nvidia/dts/tegra2-seaboard.dts +5 -0

@@ -44,4 +49,9 @@
 ? ? ? usb at c5004000 {
 ? ? ? ? ? ? ? status = "disabled";
 ? ? ? };
+
+ ? ? i2c at 7000c400 {
+ ? ? ? ? ? ? status = "disabled";
+ ? ? };
+
 ?};

That chunk wasn't in the original patch, and doesn't match the kernel's .dts file (and I believe that I2C controller really is in use, so shouldn't be disabled).

It cannot be used - remember the discussion about pinmux? We elected to disable I2C1 at present since you didn't like my nvidia,pinmux binding for selecting which value to pass to funcmux. The fix is to pass 1 instead of 0 for that port, but we have no clean way to specify this.

Quoted 5 lines

Quoted 1 line

Rather disable it than leave it enabled and not working.

Rather than having the .dts file not correctly describe the HW, wouldn't it be better to limit U-Boot's to only initializing the 1 I2C controller that it knows the valid pinmux setting for?

Well, on Seaboard I2C2 (sorry, not I2C1 as I said in my email) is not used or connected to anything, so it is a reasonable description of the hardware. The other 3 pinmux settings are valid.

Quoted 8 lines

Quoted 2 lines

As best I can tell from my schematics, all 4 I2C ports are used on both Seaboard and Springbank.

I don't know about Springbank, but this patch is for Seaboard.

There is nothing on the bus and I can't see anything on the schematic. Can you please tell me what it is used for and which pins you are referring to?

GEN2_I2C_SCL/SDA are connected to:

J4 (battery or charger)
TPM
J6 (touchscreen connector)
J26 (mini PCIe)
J18 (satellite board)

I assume you have access to the schematics?

--
nvpublic

This series brings in an I2C driver for Tegra which can be configured by a flat device tree.

It supports 8- and 16-bit addresses and both the normal I2C ports and the DVC port (for controlling the power management unit (PMU)).

Recent Linux bindings are used, based on example .dts files found in branch for-3.3/dt at:

git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git

(I could not find the actual binding documentation to include here)

Changes in v2:

Hello Simon,

Quoted 11 lines · Simon Glass

This series brings in an I2C driver for Tegra which can be configured by a flat device tree.

It supports 8- and 16-bit addresses and both the normal I2C ports and the DVC port (for controlling the power management unit (PMU)).

Recent Linux bindings are used, based on example .dts files found in branch for-3.3/dt at:

git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git

Quoted 4 lines

(I could not find the actual binding documentation to include here)

Changes in v2:

Hups, seems I missed the v1 ?... please add the i2c custodian (me ;-) to cc in further EMails, thanks!

bye,
Heiko
--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany