[U-Boot] [PATCH 0/3] Bring in new I2C framework

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Simon Glass]

This series provides Heiko's upgraded I2C framework from a few years ago.
I hope that we can bring this in and move boards over to it as time
permits, rather than switching everything in one fell swoop which never
happens.

To show it working I have enabled it for Tegra in a very rough way. It
seems fine with my limited testing.

In terms of changes, I have just fixed some checkpatch errors and fiddled
with a couple of function signatures.

I will start a thread on the list with a few thoughts on this series
at some point.


Heiko Schocher (2):
  i2c: add i2c_core and prepare for new multibus support
  i2c: common changes for multibus/multiadapter support

Simon Glass (1):
  WIP: tegra: i2c: Enable new I2C framework

 README                                    |   82 +++++++-
 arch/arm/include/asm/global_data.h        |    3 +
 arch/arm/lib/board.c                      |    3 +-
 arch/avr32/include/asm/global_data.h      |    3 +
 arch/blackfin/include/asm/global_data.h   |    4 +-
 arch/blackfin/lib/board.c                 |    7 +
 arch/m68k/include/asm/global_data.h       |    3 +
 arch/m68k/lib/board.c                     |   18 ++-
 arch/microblaze/include/asm/global_data.h |    3 +
 arch/mips/include/asm/global_data.h       |    3 +
 arch/mips/lib/board.c                     |    7 +
 arch/nios2/include/asm/global_data.h      |    3 +
 arch/powerpc/cpu/mpc8xx/video.c           |    4 +
 arch/powerpc/include/asm/global_data.h    |    3 +
 arch/powerpc/lib/board.c                  |   18 ++-
 arch/sh/include/asm/global_data.h         |    3 +
 arch/sparc/include/asm/global_data.h      |    3 +
 arch/x86/include/asm/global_data.h        |    3 +
 common/cmd_date.c                         |    9 +
 common/cmd_dtt.c                          |    9 +
 common/cmd_i2c.c                          |  127 +++++++----
 common/stdio.c                            |   13 +-
 drivers/i2c/Makefile                      |    1 +
 drivers/i2c/i2c_core.c                    |  360 +++++++++++++++++++++++++++++
 drivers/i2c/tegra2_i2c.c                  |   53 ++---
 include/configs/seaboard.h                |    2 +
 include/i2c.h                             |  190 +++++++++++++++-
 27 files changed, 842 insertions(+), 95 deletions(-)
 create mode 100644 drivers/i2c/i2c_core.c

-- 
1.7.7.3

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Simon Glass]

From: Heiko Schocher <hs@denx.de>

This Patch introduce the new i2c_core file, which holds
the I2C core functions, for the rework of the multibus/
multiadapter support.
Also adds changes in i2c.h for the new I2C multibus/multiadapter
support. This new support can be activated with the
CONFIG_SYS_I2C define.

Signed-off-by: Heiko Schocher <hs@denx.de>
Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/include/asm/global_data.h        |    3 +
 arch/avr32/include/asm/global_data.h      |    3 +
 arch/blackfin/include/asm/global_data.h   |    4 +-
 arch/m68k/include/asm/global_data.h       |    3 +
 arch/microblaze/include/asm/global_data.h |    3 +
 arch/mips/include/asm/global_data.h       |    3 +
 arch/nios2/include/asm/global_data.h      |    3 +
 arch/powerpc/include/asm/global_data.h    |    3 +
 arch/sh/include/asm/global_data.h         |    3 +
 arch/sparc/include/asm/global_data.h      |    3 +
 arch/x86/include/asm/global_data.h        |    3 +
 drivers/i2c/Makefile                      |    1 +
 drivers/i2c/i2c_core.c                    |  360 +++++++++++++++++++++++++++++
 include/i2c.h                             |  199 +++++++++++++++-
 14 files changed, 584 insertions(+), 10 deletions(-)
 create mode 100644 drivers/i2c/i2c_core.c

diff --git a/arch/arm/include/asm/global_data.h b/arch/arm/include/asm/global_data.h
index 23a6077..924cea2 100644
--- a/arch/arm/include/asm/global_data.h
+++ b/arch/arm/include/asm/global_data.h
@@ -87,6 +87,9 @@ typedef	struct	global_data {
 	unsigned long	post_log_res; /* success of POST test */
 	unsigned long	post_init_f_time; /* When post_init_f started */
 #endif
+#if defined(CONFIG_SYS_I2C)
+	void		*cur_adap;	/* current used i2c adapter */
+#endif
 } gd_t;
 
 /*
diff --git a/arch/avr32/include/asm/global_data.h b/arch/avr32/include/asm/global_data.h
index 5c654bd..605b1a7 100644
--- a/arch/avr32/include/asm/global_data.h
+++ b/arch/avr32/include/asm/global_data.h
@@ -50,6 +50,9 @@ typedef	struct	global_data {
 #endif
 	void		**jt;		/* jump table */
 	char		env_buf[32];	/* buffer for getenv() before reloc. */
+#if defined(CONFIG_SYS_I2C)
+	void		*cur_adap;	/* current used i2c adapter */
+#endif
 } gd_t;
 
 /*
diff --git a/arch/blackfin/include/asm/global_data.h b/arch/blackfin/include/asm/global_data.h
index 67aa30f..eacfd17 100644
--- a/arch/blackfin/include/asm/global_data.h
+++ b/arch/blackfin/include/asm/global_data.h
@@ -56,9 +56,11 @@ typedef struct global_data {
 	unsigned long post_log_res; 	/* success of POST test */
 	unsigned long post_init_f_time;	/* When post_init_f started */
 #endif
-
 	void	**jt;			/* jump table */
 	char	env_buf[32];		/* buffer for getenv() before reloc. */
+#if defined(CONFIG_SYS_I2C)
+	void		*cur_adap;	/* current used i2c adapter */
+#endif
 } gd_t;
 
 /*
diff --git a/arch/m68k/include/asm/global_data.h b/arch/m68k/include/asm/global_data.h
index 0ba2b43..fb171f8 100644
--- a/arch/m68k/include/asm/global_data.h
+++ b/arch/m68k/include/asm/global_data.h
@@ -68,6 +68,9 @@ typedef	struct	global_data {
 #endif
 	void		**jt;		/* Standalone app jump table */
 	char		env_buf[32];	/* buffer for getenv() before reloc. */
+#if defined(CONFIG_SYS_I2C)
+	void		*cur_adap;	/* current used i2c adapter */
+#endif
 } gd_t;
 
 /*
diff --git a/arch/microblaze/include/asm/global_data.h b/arch/microblaze/include/asm/global_data.h
index 6e8537c..a43ca0d 100644
--- a/arch/microblaze/include/asm/global_data.h
+++ b/arch/microblaze/include/asm/global_data.h
@@ -47,6 +47,9 @@ typedef	struct	global_data {
 	unsigned long	fb_base;	/* base address of frame buffer */
 	void		**jt;		/* jump table */
 	char		env_buf[32];	/* buffer for getenv() before reloc. */
+#if defined(CONFIG_SYS_I2C)
+	void		*cur_adap;	/* current used i2c adapter */
+#endif
 } gd_t;
 
 /*
diff --git a/arch/mips/include/asm/global_data.h b/arch/mips/include/asm/global_data.h
index f6cf9fe..436c0c7 100644
--- a/arch/mips/include/asm/global_data.h
+++ b/arch/mips/include/asm/global_data.h
@@ -61,6 +61,9 @@ typedef	struct	global_data {
 	unsigned long	env_valid;	/* Checksum of Environment valid? */
 	void		**jt;		/* jump table */
 	char		env_buf[32];	/* buffer for getenv() before reloc. */
+#if defined(CONFIG_SYS_I2C)
+	void		*cur_adap;	/* current used i2c adapter */
+#endif
 } gd_t;
 
 /*
diff --git a/arch/nios2/include/asm/global_data.h b/arch/nios2/include/asm/global_data.h
index 4b86fbd..714e2f8 100644
--- a/arch/nios2/include/asm/global_data.h
+++ b/arch/nios2/include/asm/global_data.h
@@ -42,6 +42,9 @@ typedef	struct	global_data {
 #endif
 	void		**jt;		/* Standalone app jump table */
 	char		env_buf[32];	/* buffer for getenv() before reloc. */
+#if defined(CONFIG_SYS_I2C)
+	void		*cur_adap;	/* current used i2c adapter */
+#endif
 } gd_t;
 
 /* flags */
diff --git a/arch/powerpc/include/asm/global_data.h b/arch/powerpc/include/asm/global_data.h
index 01f1d4a..96f2ad8 100644
--- a/arch/powerpc/include/asm/global_data.h
+++ b/arch/powerpc/include/asm/global_data.h
@@ -184,6 +184,9 @@ typedef	struct	global_data {
 #endif
 	void		**jt;		/* jump table */
 	char		env_buf[32];	/* buffer for getenv() before reloc. */
+#if defined(CONFIG_SYS_I2C)
+	void		*cur_adap;	/* current used i2c adapter */
+#endif
 } gd_t;
 
 /*
diff --git a/arch/sh/include/asm/global_data.h b/arch/sh/include/asm/global_data.h
index 1b782fc..7b25d8f 100644
--- a/arch/sh/include/asm/global_data.h
+++ b/arch/sh/include/asm/global_data.h
@@ -42,6 +42,9 @@ typedef	struct global_data
 	unsigned long	env_valid;	/* Checksum of Environment valid */
 	void		**jt;		/* Standalone app jump table */
 	char		env_buf[32];	/* buffer for getenv() before reloc. */
+#if defined(CONFIG_SYS_I2C)
+	void		*cur_adap;	/* current used i2c adapter */
+#endif
 } gd_t;
 
 #define	GD_FLG_RELOC		0x00001	/* Code was relocated to RAM		*/
diff --git a/arch/sparc/include/asm/global_data.h b/arch/sparc/include/asm/global_data.h
index 613e2d8..6a86b69 100644
--- a/arch/sparc/include/asm/global_data.h
+++ b/arch/sparc/include/asm/global_data.h
@@ -76,6 +76,9 @@ typedef struct global_data {
 #endif
 	void	**jt;			/* jump table */
 	char	env_buf[32];		/* buffer for getenv() before reloc. */
+#if defined(CONFIG_SYS_I2C)
+	void		*cur_adap;	/* current used i2c adapter */
+#endif
 } gd_t;
 
 /*
diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h
index 05a2139..4713b41 100644
--- a/arch/x86/include/asm/global_data.h
+++ b/arch/x86/include/asm/global_data.h
@@ -55,6 +55,9 @@ typedef	struct global_data {
 	unsigned long	reset_status;	/* reset status register at boot */
 	void		**jt;		/* jump table */
 	char		env_buf[32];	/* buffer for getenv() before reloc. */
+#if defined(CONFIG_SYS_I2C)
+	void		*cur_adap;	/* current used i2c adapter */
+#endif
 } gd_t;
 
 extern gd_t *gd;
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index c123c72..ebedef2 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -45,6 +45,7 @@ COBJS-$(CONFIG_TEGRA2_I2C) += tegra2_i2c.o
 COBJS-$(CONFIG_TSI108_I2C) += tsi108_i2c.o
 COBJS-$(CONFIG_U8500_I2C) += u8500_i2c.o
 COBJS-$(CONFIG_SH_I2C) += sh_i2c.o
+COBJS-$(CONFIG_SYS_I2C) += i2c_core.o
 
 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)
diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c
new file mode 100644
index 0000000..139f4c9
--- /dev/null
+++ b/drivers/i2c/i2c_core.c
@@ -0,0 +1,360 @@
+/*
+ * Copyright (C) 2009 Sergey Kubushyn <ksi@koi8.net>
+ *
+ * Multibus/multiadapter I2C core functions (wrappers)
+ */
+#include <common.h>
+#include <i2c.h>
+
+#ifdef CONFIG_TEGRA2_I2C
+extern struct i2c_adapter tegra_i2c_adap[];
+#endif
+
+struct i2c_adapter *i2c_adap[CONFIG_SYS_NUM_I2C_ADAPTERS] =
+			CONFIG_SYS_I2C_ADAPTERS;
+#ifndef CONFIG_SYS_I2C_DIRECT_BUS
+struct i2c_bus	i2c_bus[CONFIG_SYS_NUM_I2C_BUSSES] = CONFIG_SYS_I2C_BUSSES;
+#endif
+
+static unsigned int i2c_cur_bus __attribute__((section(".data"))) =
+				CONFIG_SYS_SPD_BUS_NUM;
+
+DECLARE_GLOBAL_DATA_PTR;
+
+void i2c_reloc_fixup(void)
+{
+#if defined(CONFIG_NEEDS_MANUAL_RELOC)
+	int		i;
+	unsigned long	addr;
+
+	for (i = 0; i < CONFIG_SYS_NUM_I2C_ADAPTERS; i++) {
+		/* Adapter itself */
+		addr = (unsigned long)i2c_adap[i];
+		if (addr != 0)
+			addr += gd->reloc_off;
+
+		i2c_adap[i] = (struct i2c_adapter *)addr;
+		/* i2c_init() */
+		addr = (unsigned long)i2c_adap[i]->init;
+		if (addr != 0)
+			addr += gd->reloc_off;
+
+		i2c_adap[i]->init = (void (*)(int, int))addr;
+		/* i2c_probe() */
+		addr = (unsigned long)i2c_adap[i]->probe;
+		if (addr != 0)
+			addr += gd->reloc_off;
+
+		i2c_adap[i]->probe = (int (*)(u_int8_t))addr;
+		/* i2c_read() */
+		addr = (unsigned long)i2c_adap[i]->read;
+		if (addr != 0)
+			addr += gd->reloc_off;
+
+		i2c_adap[i]->read = (int (*)(u_int8_t, uint, int, u_int8_t *,
+					int))addr;
+		/* i2c_write() */
+		addr = (unsigned long)i2c_adap[i]->write;
+		if (addr != 0)
+			addr += gd->reloc_off;
+
+		i2c_adap[i]->write = (int (*)(u_int8_t, uint, int, u_int8_t *,
+					int))addr;
+		/* i2c_set_bus_speed() */
+		addr = (unsigned long)i2c_adap[i]->set_bus_speed;
+		if (addr != 0)
+			addr += gd->reloc_off;
+
+		i2c_adap[i]->set_bus_speed = (uint (*)(uint))addr;
+		/* name */
+		addr = (unsigned long)i2c_adap[i]->name;
+		if (addr != 0)
+			addr += gd->reloc_off;
+
+		i2c_adap[i]->name = (char *)addr;
+	}
+	gd->cur_adap += gd->reloc_off;
+#else
+	/*
+	 * we need this here to set after relocation gd->cur_adap
+	 * to the new relocated value.
+	 */
+	gd->cur_adap = NULL;
+	i2c_set_bus_num(i2c_cur_bus);
+#endif
+}
+
+#ifndef CONFIG_SYS_I2C_DIRECT_BUS
+/*
+ * i2c_mux_set()
+ * -------------
+ *
+ * This turns on the given channel on I2C multiplexer chip connected to
+ * a given I2C adapter directly or via other multiplexers. In the latter
+ * case the entire multiplexer chain must be initialized first starting
+ * with the one connected directly to the adapter. When disabling a chain
+ * muxes must be programmed in reverse order, starting with the one
+ * farthest from the adapter.
+ *
+ * mux_id is the multiplexer chip type from defined in i2c.h. So far only
+ * NXP (Philips) PCA954x multiplexers are supported. Switches are NOT
+ * supported (anybody uses them?)
+ */
+
+static int i2c_mux_set(int adapter, int mux_id, int chip, int channel)
+{
+	u_int8_t	buf;
+
+	/* channel < 0 - turn off the mux */
+	if (channel < 0) {
+		buf = 0;
+		return i2c_adap[adapter]->write(chip, 0, 0, &buf, 1);
+	}
+
+	switch (mux_id) {
+	case I2C_MUX_PCA9540_ID:
+	case I2C_MUX_PCA9542_ID:
+		if (channel > 1)
+			return -1;
+		buf = (u_int8_t)((channel & 0x01) | (1 << 2));
+		break;
+	case I2C_MUX_PCA9544_ID:
+		if (channel > 3)
+			return -1;
+		buf = (u_int8_t)((channel & 0x03) | (1 << 2));
+		break;
+	case I2C_MUX_PCA9547_ID:
+		if (channel > 7)
+			return -1;
+		buf = (u_int8_t)((channel & 0x07) | (1 << 3));
+		break;
+	default:
+		return -1;
+	}
+
+	return i2c_adap[adapter]->write(chip, 0, 0, &buf, 1);
+}
+#endif
+
+/*
+ * i2c_init_bus():
+ * ---------------
+ *
+ * Initializes one bus. Will initialize the parent adapter. No current bus
+ * changes, no mux (if any) setup.
+ */
+static void i2c_init_bus(unsigned int bus_no, int speed, int slaveaddr)
+{
+	if (bus_no >= CONFIG_SYS_NUM_I2C_BUSSES)
+		return;
+
+	I2C_ADAP->init(speed, slaveaddr);
+
+	if (gd->flags & GD_FLG_RELOC) {
+		I2C_ADAP->init_done = 1;
+		I2C_ADAP->speed = speed;
+		I2C_ADAP->slaveaddr = slaveaddr;
+	}
+}
+
+/*
+ * i2c_init_all():
+ *
+ * not longer needed, will deleted. Actual init the SPD_BUS
+ * for compatibility.
+ * i2c_adap[] must be initialized beforehead with function pointers and
+ * data, including speed and slaveaddr.
+ */
+void i2c_init_all(void)
+{
+	i2c_set_bus_num(CONFIG_SYS_SPD_BUS_NUM);
+	return;
+}
+
+/*
+ * i2c_get_bus_num():
+ * ------------------
+ *
+ *  Returns index of currently active I2C bus.  Zero-based.
+ */
+unsigned int i2c_get_bus_num(void)
+{
+	return i2c_cur_bus;
+}
+
+/*
+ * i2c_set_bus_num():
+ * ------------------
+ *
+ *  Change the active I2C bus. Subsequent read/write calls will
+ *  go to this one. Sets all of the muxes in a proper condition
+ *  if that bus is behind muxes.
+ *  If previously selected bus is behind the muxes turns off all the
+ *  muxes along the path to that bus.
+ *
+ *	bus - bus index, zero based
+ *
+ *	Returns: 0 on success, not 0 on failure
+ */
+int i2c_set_bus_num(unsigned int bus)
+{
+#ifndef CONFIG_SYS_I2C_DIRECT_BUS
+	int		i;
+	u_int8_t	buf;
+#endif
+
+	if (bus >= CONFIG_SYS_NUM_I2C_BUSSES)
+		return -1;
+
+	if (gd->cur_adap == NULL)
+		gd->cur_adap = I2C_ADAP_NR(bus);
+	else if ((bus == i2c_cur_bus) &&
+		(I2C_ADAP->init_done > 0))
+		return 0;
+#ifndef CONFIG_SYS_I2C_DIRECT_BUS
+	else {
+		/* Disconnect current bus (turn off muxes if any) */
+		if ((i2c_bus[i2c_cur_bus].next_hop[0].chip != 0) &&
+		    (I2C_ADAP->init_done != 0)) {
+			i = CONFIG_SYS_I2C_MAX_HOPS;
+			do {
+				u_int8_t	chip;
+
+				chip = i2c_bus[i2c_cur_bus].next_hop[--i].chip;
+				if (chip == 0)
+					continue;
+
+				I2C_ADAP->write(chip, 0, 0, &buf, 1);
+			} while (i > 0);
+		}
+	}
+#endif
+
+	gd->cur_adap = I2C_ADAP_NR(bus);
+	if (I2C_ADAP->init_done == 0)
+		i2c_init_bus(bus, I2C_ADAP->speed, I2C_ADAP->slaveaddr);
+
+#ifndef CONFIG_SYS_I2C_DIRECT_BUS
+	/* Connect requested bus if behind muxes */
+	if (i2c_bus[bus].next_hop[0].chip != 0) {
+
+		/* Set all muxes along the path to that bus */
+		for (i = 0; i < CONFIG_SYS_I2C_MAX_HOPS; i++) {
+			int	ret;
+
+			if (i2c_bus[bus].next_hop[i].chip == 0)
+				break;
+
+			ret = i2c_mux_set(i2c_bus[bus].adapter,
+					i2c_bus[bus].next_hop[i].mux.id,
+					i2c_bus[bus].next_hop[i].chip,
+					i2c_bus[bus].next_hop[i].channel);
+			if (ret != 0) {
+				printf("%s: could not set mux: id: %d "\
+					"chip: %x channel: %d\n", __func__,
+					i2c_bus[bus].next_hop[i].mux.id,
+					i2c_bus[bus].next_hop[i].chip,
+					i2c_bus[bus].next_hop[i].channel);
+
+			}
+		}
+	}
+#endif
+
+	i2c_cur_bus = bus;
+	return 0;
+}
+
+/*
+ * Probe the given I2C chip address.  Returns 0 if a chip responded,
+ * not 0 on failure.
+ */
+int i2c_probe(u_int8_t chip)
+{
+	return I2C_ADAP->probe(chip);
+}
+
+/*
+ * Read/Write interface:
+ *   chip:    I2C chip address, range 0..127
+ *   addr:    Memory (register) address within the chip
+ *   alen:    Number of bytes to use for addr (typically 1, 2 for larger
+ *              memories, 0 for register type devices with only one
+ *              register)
+ *   buffer:  Where to read/write the data
+ *   len:     How many bytes to read/write
+ *
+ *   Returns: 0 on success, not 0 on failure
+ */
+int i2c_read(u_int8_t chip, unsigned int addr, int alen,
+				u_int8_t *buffer, int len)
+{
+	return I2C_ADAP->read(chip, addr, alen, buffer, len);
+}
+
+int i2c_write(u_int8_t chip, unsigned int addr, int alen,
+				u_int8_t *buffer, int len)
+{
+	return I2C_ADAP->write(chip, addr, alen, buffer, len);
+}
+
+unsigned int i2c_set_bus_speed(unsigned int speed)
+{
+	unsigned int ret;
+
+	if (I2C_ADAP->set_bus_speed == NULL)
+		return 0;
+	ret = I2C_ADAP->set_bus_speed(speed);
+	if (gd->flags & GD_FLG_RELOC)
+		I2C_ADAP->speed = ret;
+
+	return ret;
+}
+
+unsigned int i2c_get_bus_speed(void)
+{
+	struct i2c_adapter *cur = I2C_ADAP;
+	return cur->speed;
+}
+
+u_int8_t i2c_reg_read(u_int8_t addr, u_int8_t reg)
+{
+	u_int8_t buf;
+
+#ifdef CONFIG_8xx
+	/* MPC8xx needs this.  Maybe one day we can get rid of it. */
+	/* maybe it is now the time for it ... */
+	i2c_set_bus_num(i2c_cur_bus);
+#endif
+	i2c_read(addr, reg, 1, &buf, 1);
+
+#ifdef DEBUG
+	printf("%s: bus=%d addr=0x%02x, reg=0x%02x, val=0x%02x\n",
+		 __func__, i2c_cur_bus, addr, reg, buf);
+#endif
+
+	return buf;
+}
+
+void i2c_reg_write(u_int8_t addr, u_int8_t reg, u_int8_t val)
+{
+#ifdef CONFIG_8xx
+	/* MPC8xx needs this.  Maybe one day we can get rid of it. */
+	/* maybe it is now the time for it ... */
+	i2c_set_bus_num(i2c_cur_bus);
+#endif
+
+#ifdef DEBUG
+	printf("%s: bus=%d addr=0x%02x, reg=0x%02x, val=0x%02x\n",
+	       __func__, i2c_cur_bus, addr, reg, val);
+#endif
+
+	i2c_write(addr, reg, 1, &val, 1);
+}
+
+void __i2c_init(int speed, int slaveaddr)
+{
+	i2c_init_bus(i2c_cur_bus, speed, slaveaddr);
+}
+void i2c_init(int speed, int slaveaddr)
+	__attribute__((weak, alias("__i2c_init")));
diff --git a/include/i2c.h b/include/i2c.h
index ee31034..910552d 100644
--- a/include/i2c.h
+++ b/include/i2c.h
@@ -1,4 +1,8 @@
 /*
+ * Copyright (C) 2009 Sergey Kubushyn <ksi@koi8.net>
+ * Copyright (C) 2009 Heiko Schocher <hs@denx.de>
+ * Changes for multibus/multiadapter I2C support.
+ *
  * (C) Copyright 2001
  * Gerald Van Baren, Custom IDEAS, vanbaren at cideas.com.
  *
@@ -46,16 +50,20 @@
  */
 #define I2C_RXTX_LEN	128	/* maximum tx/rx buffer length */
 
-#ifdef	CONFIG_I2C_MULTI_BUS
-#define	MAX_I2C_BUS			2
-#define	I2C_MULTI_BUS			1
-#else
-#define	MAX_I2C_BUS			1
-#define	I2C_MULTI_BUS			0
+#ifndef CONFIG_SYS_NUM_I2C_ADAPTERS
+#define CONFIG_SYS_NUM_I2C_ADAPTERS	1
 #endif
 
-#if !defined(CONFIG_SYS_MAX_I2C_BUS)
-#define CONFIG_SYS_MAX_I2C_BUS		MAX_I2C_BUS
+#if !defined(CONFIG_SYS_I2C_MAX_HOPS) || (CONFIG_SYS_I2C_MAX_HOPS == 0)
+#define CONFIG_SYS_I2C_DIRECT_BUS	1
+#if !defined(CONFIG_SYS_NUM_I2C_BUSSES)
+#define CONFIG_SYS_NUM_I2C_BUSSES	CONFIG_SYS_NUM_I2C_ADAPTERS
+#endif
+#else
+#undef CONFIG_SYS_I2C_DIRECT_BUS
+#ifndef CONFIG_SYS_NUM_I2C_BUSSES
+#define CONFIG_SYS_NUM_I2C_BUSSES	1
+#endif
 #endif
 
 /* define the I2C bus number for RTC and DTT if not already done */
@@ -69,6 +77,65 @@
 #define CONFIG_SYS_SPD_BUS_NUM		0
 #endif
 
+struct i2c_adapter {
+	void		(*init)(int speed, int slaveaddr);
+	int		(*probe)(u_int8_t chip);
+	int		(*read)(u_int8_t chip, uint addr, int alen,
+				u_int8_t *buffer, int len);
+	int		(*write)(u_int8_t chip, uint addr, int alen,
+				u_int8_t *buffer, int len);
+	uint		(*set_bus_speed)(uint speed);
+
+	int		speed;
+	int		slaveaddr;
+	int		init_done;
+	int		hwadapnr;
+	char		*name;
+};
+
+#ifndef CONFIG_SYS_I2C_DIRECT_BUS
+#define	I2C_ADAP_NR(bus)	i2c_adap[i2c_bus[(bus)].adapter]
+#else
+#define	I2C_ADAP_NR(bus)	i2c_adap[bus]
+#endif
+#define	I2C_ADAP		((struct i2c_adapter *)gd->cur_adap)
+
+#define I2C_ADAP_HWNR		(I2C_ADAP->hwadapnr)
+
+#ifndef CONFIG_SYS_I2C_DIRECT_BUS
+#define I2C_MUX_PCA9540_ID	1
+#define I2C_MUX_PCA9540		{I2C_MUX_PCA9540_ID, "PCA9540B"}
+#define I2C_MUX_PCA9542_ID	2
+#define I2C_MUX_PCA9542		{I2C_MUX_PCA9542_ID, "PCA9542A"}
+#define I2C_MUX_PCA9544_ID	3
+#define I2C_MUX_PCA9544		{I2C_MUX_PCA9544_ID, "PCA9544A"}
+#define I2C_MUX_PCA9547_ID	4
+#define I2C_MUX_PCA9547		{I2C_MUX_PCA9547_ID, "PCA9547A"}
+
+struct i2c_mux {
+	int	id;
+	char	name[16];
+};
+
+struct i2c_next_hop {
+	i2c_mux_t		mux;
+	u_int8_t		chip;
+	u_int8_t		channel;
+};
+
+struct i2c_bus_hose {
+	int		adapter;
+	i2c_next_hop_t	next_hop[CONFIG_SYS_I2C_MAX_HOPS];
+};
+
+#define I2C_NULL_HOP	{{-1, ""}, 0, 0}
+
+extern i2c_bus_t	i2c_bus[];
+#endif
+
+extern struct i2c_adapter	*i2c_adap[];
+extern struct i2c_adapter	*cur_adap_nr;
+
 #ifndef I2C_SOFT_DECLARATIONS
 # if defined(CONFIG_MPC8260)
 #  define I2C_SOFT_DECLARATIONS volatile ioport_t *iop = ioport_addr((immap_t *)CONFIG_SYS_IMMR, I2C_PORT);
@@ -108,7 +175,7 @@
  * repeatedly to change the speed and slave addresses.
  */
 void i2c_init(int speed, int slaveaddr);
-void i2c_init_board(void);
+int i2c_init_board(void);
 #ifdef CONFIG_SYS_I2C_BOARD_LATE_INIT
 void i2c_board_late_init(void);
 #endif
@@ -134,6 +201,104 @@ int i2x_mux_select_mux(int bus);
 int i2c_mux_ident_muxstring_f (uchar *buf);
 #endif
 
+#ifdef CONFIG_SYS_I2C
+/*
+ * Initialization, must be called once on start up, may be called
+ * repeatedly to change the speed and slave addresses.
+ */
+void i2c_init(unsigned int speed, int slaveaddr);
+#ifdef CONFIG_SYS_I2C_INIT_BOARD
+void i2c_init_board(void);
+#endif
+
+/*
+ * i2c_get_bus_num:
+ *
+ *  Returns index of currently active I2C bus.  Zero-based.
+ */
+unsigned int i2c_get_bus_num(void);
+
+/*
+ * i2c_set_bus_num:
+ *
+ *  Change the active I2C bus.  Subsequent read/write calls will
+ *  go to this one.
+ *
+ *	bus - bus index, zero based
+ *
+ *	Returns: 0 on success, not 0 on failure
+ *
+ */
+int i2c_set_bus_num(unsigned int bus);
+
+/*
+ * i2c_init_all():
+ *
+ * Initializes all I2C adapters in the system. All i2c_adap structures must
+ * be initialized beforehead with function pointers and data, including
+ * speed and slaveaddr. Returns 0 on success, non-0 on failure.
+ */
+void i2c_init_all(void);
+
+/*
+ * Probe the given I2C chip address.  Returns 0 if a chip responded,
+ * not 0 on failure.
+ */
+int i2c_probe(u_int8_t chip);
+
+/*
+ * Read/Write interface:
+ *   chip:    I2C chip address, range 0..127
+ *   addr:    Memory (register) address within the chip
+ *   alen:    Number of bytes to use for addr (typically 1, 2 for larger
+ *              memories, 0 for register type devices with only one
+ *              register)
+ *   buffer:  Where to read/write the data
+ *   len:     How many bytes to read/write
+ *
+ *   Returns: 0 on success, not 0 on failure
+ */
+int i2c_read(u_int8_t chip, unsigned int addr, int alen,
+				u_int8_t *buffer, int len);
+
+int i2c_write(u_int8_t chip, unsigned int addr, int alen,
+				u_int8_t *buffer, int len);
+
+/*
+ * Utility routines to read/write registers.
+ */
+u_int8_t i2c_reg_read(u_int8_t addr, u_int8_t reg);
+
+void i2c_reg_write(u_int8_t addr, u_int8_t reg, u_int8_t val);
+
+/*
+ * i2c_set_bus_speed:
+ *
+ *  Change the speed of the active I2C bus
+ *
+ *	speed - bus speed in Hz
+ *
+ *	Returns: new bus speed
+ *
+ */
+unsigned int i2c_set_bus_speed(unsigned int speed);
+
+/*
+ * i2c_get_bus_speed:
+ *
+ *  Returns speed of currently active I2C bus in Hz
+ */
+
+unsigned int i2c_get_bus_speed(void);
+
+/*
+ * i2c_reloc_fixup:
+ *
+ * Adjusts I2C pointers after U-Boot is relocated to DRAM
+ */
+void i2c_reloc_fixup(void);
+#else
+
 /*
  * Probe the given I2C chip address.  Returns 0 if a chip responded,
  * not 0 on failure.
@@ -235,6 +400,22 @@ int i2c_set_bus_speed(unsigned int);
  */
 
 unsigned int i2c_get_bus_speed(void);
+#endif /* CONFIG_SYS_I2C */
+
+/*
+ * only for backwardcompatibility, should go away if we switched
+ * completely to new multibus support.
+ */
+#if (CONFIG_SYS_NUM_I2C_BUSSES > 1) || defined(CONFIG_I2C_MULTI_BUS)
+# if !defined(CONFIG_SYS_MAX_I2C_BUS)
+#  define CONFIG_SYS_MAX_I2C_BUS		2
+# endif
+# define I2C_MULTI_BUS				0
+#else
+# define CONFIG_SYS_MAX_I2C_BUS		1
+# define I2C_MULTI_BUS				0
+#endif
+
 
 /* NOTE: These two functions MUST be always_inline to avoid code growth! */
 static inline unsigned int I2C_GET_BUS(void) __attribute__((always_inline));
-- 
1.7.7.3

[U-Boot] [PATCH 2/3] i2c: common changes for multibus/multiadapter support

[Simon Glass]

From: Heiko Schocher <hs@denx.de>

Signed-off-by: Heiko Schocher <hs@denx.de>
Signed-off-by: Simon Glass <sjg@chromium.org>
---
 README                          |   82 ++++++++++++++++++++++++-
 arch/arm/lib/board.c            |    3 +-
 arch/blackfin/lib/board.c       |    7 ++
 arch/m68k/lib/board.c           |   18 +++++-
 arch/mips/lib/board.c           |    7 ++
 arch/powerpc/cpu/mpc8xx/video.c |    4 +
 arch/powerpc/lib/board.c        |   18 +++++-
 common/cmd_date.c               |    9 +++
 common/cmd_dtt.c                |    9 +++
 common/cmd_i2c.c                |  127 +++++++++++++++++++++++++-------------
 common/stdio.c                  |   13 ++++-
 include/i2c.h                   |    9 ---
 12 files changed, 242 insertions(+), 64 deletions(-)

diff --git a/README b/README
index 7adf7c7..fe1c0d9 100644
--- a/README
+++ b/README
@@ -1669,9 +1669,85 @@ The following options need to be configured:
 		on those systems that support this (optional)
 		feature, like the TQM8xxL modules.
 
-- I2C Support:	CONFIG_HARD_I2C | CONFIG_SOFT_I2C
-
-		These enable I2C serial bus commands. Defining either of
+- I2C Support:	CONFIG_SYS_I2C
+
+		This enable the NEW i2c subsystem, and will allow you to use
+		i2c commands at the u-boot command line (as long as you set
+		CONFIG_CMD_I2C in CONFIG_COMMANDS) and communicate with i2c
+		based realtime clock chips or other i2c devices. See
+		common/cmd_i2c.c for a description of the command line
+		interface.
+
+		additional defines:
+
+		CONFIG_SYS_NUM_I2C_ADAPTERS
+		define how many i2c adapters you want to use on your
+		hardware. If you need only 1 i2c adapter, you can ommit
+		this define.
+
+		CONFIG_SYS_I2C_ADAPTERS
+		hold a list of adapters you want to use, for example:
+		{&soft_i2c_adap[0], &mpc5xxx_i2c_adap[0]}
+		with this configuration you have one soft_i2c adapter,
+		and one mpc5xxx i2c adapter.
+
+		No I2C adapters current suppor this new interface.
+
+		You need to define for each adapter a speed and a
+		slave address.
+
+		CONFIG_SYS_NUM_I2C_BUSSES
+		Hold the number of i2c busses you want to use. If you
+		don't use/have i2c muxes on your i2c bus, this
+		is equal to CONFIG_SYS_NUM_I2C_ADAPTERS, and you can
+		omit this define.
+
+		CONFIG_SYS_I2C_DIRECT_BUS
+		define this, if you don't use i2c muxes on your hardware.
+		if CONFIG_SYS_I2C_MAX_HOPS is not defined or == 0 you can
+		omit this define.
+
+		CONFIG_SYS_I2C_MAX_HOPS
+		define how many muxes are maximal consecutively connected
+		on one i2c bus.
+
+		CONFIG_SYS_I2C_BUSSES
+		hold a list of busses you want to use, only used if
+		CONFIG_SYS_I2C_DIRECT_BUS is not defined, for example
+		a board with CONFIG_SYS_I2C_MAX_HOPS = 1 and
+		CONFIG_SYS_NUM_I2C_BUSSES = 9:
+
+		 CONFIG_SYS_I2C_BUSSES	{{0, {I2C_NULL_HOP}}, \
+					{0, {{I2C_MUX_PCA9547, 0x70, 1}}}, \
+					{0, {{I2C_MUX_PCA9547, 0x70, 2}}}, \
+					{0, {{I2C_MUX_PCA9547, 0x70, 3}}}, \
+					{0, {{I2C_MUX_PCA9547, 0x70, 4}}}, \
+					{0, {{I2C_MUX_PCA9547, 0x70, 5}}}, \
+					{1, {I2C_NULL_HOP}}, \
+					{1, {{I2C_MUX_PCA9544, 0x72, 1}}}, \
+					{1, {{I2C_MUX_PCA9544, 0x72, 2}}}, \
+					}
+
+		which defines
+			bus 0 on adapter 0 without a mux
+			bus 1 on adapter 0 without a PCA9547 on address 0x70 port 1
+			bus 2 on adapter 0 without a PCA9547 on address 0x70 port 2
+			bus 3 on adapter 0 without a PCA9547 on address 0x70 port 3
+			bus 4 on adapter 0 without a PCA9547 on address 0x70 port 4
+			bus 5 on adapter 0 without a PCA9547 on address 0x70 port 5
+			bus 6 on adapter 1 without a mux
+			bus 7 on adapter 1 without a PCA9544 on address 0x72 port 1
+			bus 8 on adapter 1 without a PCA9544 on address 0x72 port 2
+
+- Legacy I2C Support:	CONFIG_HARD_I2C | CONFIG_SOFT_I2C
+
+		NOTE: It is intended to move drivers to CONFIG_SYS_I2C which
+		provides the following compelling advantages:
+			1. Heiko to fill in
+
+		** Please consider updating your I2C driver now. **
+
+		These enable legacy I2C serial bus commands. Defining either of
 		(but not both of) CONFIG_HARD_I2C or CONFIG_SOFT_I2C will
 		include the appropriate I2C driver for the selected CPU.
 
diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
index 81293c3..8b86a7d 100644
--- a/arch/arm/lib/board.c
+++ b/arch/arm/lib/board.c
@@ -74,7 +74,8 @@ extern void dataflash_print_info(void);
 #endif
 
 #if defined(CONFIG_HARD_I2C) || \
-    defined(CONFIG_SOFT_I2C)
+	defined(CONFIG_SOFT_I2C) || \
+	defined(CONFIG_SYS_I2C)
 #include <i2c.h>
 #endif
 
diff --git a/arch/blackfin/lib/board.c b/arch/blackfin/lib/board.c
index e3ee4cd..660fa57 100644
--- a/arch/blackfin/lib/board.c
+++ b/arch/blackfin/lib/board.c
@@ -37,6 +37,10 @@
 int post_flag;
 #endif
 
+#if defined(CONFIG_SYS_I2C_ADAPTERS)
+#include <i2c.h>
+#endif
+
 DECLARE_GLOBAL_DATA_PTR;
 
 __attribute__((always_inline))
@@ -353,6 +357,9 @@ void board_init_r(gd_t * id, ulong dest_addr)
 
 	/* Initialize stdio devices */
 	stdio_init();
+#if defined(CONFIG_SYS_I2C_ADAPTERS)
+	i2c_reloc_fixup();
+#endif
 	jumptable_init();
 
 	/* Initialize the console (after the relocation and devices init) */
diff --git a/arch/m68k/lib/board.c b/arch/m68k/lib/board.c
index 259b71c..17876d5 100644
--- a/arch/m68k/lib/board.c
+++ b/arch/m68k/lib/board.c
@@ -55,7 +55,8 @@
 #include <version.h>
 
 #if defined(CONFIG_HARD_I2C) || \
-    defined(CONFIG_SOFT_I2C)
+	defined(CONFIG_SOFT_I2C) || \
+	defined(CONFIG_SYS_I2C_ADAPTERS)
 #include <i2c.h>
 #endif
 
@@ -140,11 +141,16 @@ static int init_func_ram (void)
 
 /***********************************************************************/
 
-#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
+#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C) \
+		|| defined(CONFIG_SYS_I2C_ADAPTERS)
 static int init_func_i2c (void)
 {
 	puts ("I2C:   ");
+#ifdef CONFIG_SYS_I2C
+	i2c_init_all();
+#else
 	i2c_init (CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
+#endif
 	puts ("ready\n");
 	return (0);
 }
@@ -176,7 +182,8 @@ init_fnc_t *init_sequence[] = {
 	display_options,
 	checkcpu,
 	checkboard,
-#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
+#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C) \
+		|| defined(CONFIG_SYS_I2C_ADAPTERS)
 	init_func_i2c,
 #endif
 #if defined(CONFIG_HARD_SPI)
@@ -507,6 +514,11 @@ void board_init_r (gd_t *id, ulong dest_addr)
 	/* relocate environment function pointers etc. */
 	env_relocate ();
 
+#if defined(CONFIG_SYS_I2C) && defined(CONFIG_SYS_I2C_ADAPTERS)
+	/* Adjust I2C subsystem pointers after relocation */
+	i2c_reloc_fixup();
+#endif
+
 	/*
 	 * Fill in missing fields of bd_info.
 	 * We do this here, where we have "normal" access to the
diff --git a/arch/mips/lib/board.c b/arch/mips/lib/board.c
index d998f0e..8c6d35e 100644
--- a/arch/mips/lib/board.c
+++ b/arch/mips/lib/board.c
@@ -35,6 +35,9 @@
 #ifdef CONFIG_BITBANGMII
 #include <miiphy.h>
 #endif
+#if defined(CONFIG_SYS_I2C_ADAPTERS)
+#include <i2c.h>
+#endif
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -316,6 +319,10 @@ void board_init_r(gd_t *id, ulong dest_addr)
 	/* relocate environment function pointers etc. */
 	env_relocate();
 
+#if defined(CONFIG_SYS_I2C_ADAPTERS)
+	i2c_reloc_fixup();
+#endif
+
 	/* IP Address */
 	bd->bi_ip_addr = getenv_IPaddr("ipaddr");
 
diff --git a/arch/powerpc/cpu/mpc8xx/video.c b/arch/powerpc/cpu/mpc8xx/video.c
index 1bbf4cc..10e3e88 100644
--- a/arch/powerpc/cpu/mpc8xx/video.c
+++ b/arch/powerpc/cpu/mpc8xx/video.c
@@ -809,7 +809,11 @@ static void video_encoder_init (void)
 
 	/* Initialize the I2C */
 	debug ("[VIDEO ENCODER] Initializing I2C bus...\n");
+#ifdef CONFIG_SYS_I2C
+	i2c_init_all();
+#else
 	i2c_init (CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
+#endif
 
 #ifdef CONFIG_FADS
 	/* Reset ADV7176 chip */
diff --git a/arch/powerpc/lib/board.c b/arch/powerpc/lib/board.c
index ff5888e..563da9a 100644
--- a/arch/powerpc/lib/board.c
+++ b/arch/powerpc/lib/board.c
@@ -99,7 +99,8 @@ extern void sc3_read_eeprom(void);
 void doc_init(void);
 #endif
 #if defined(CONFIG_HARD_I2C) || \
-    defined(CONFIG_SOFT_I2C)
+	defined(CONFIG_SOFT_I2C) || \
+	defined(CONFIG_SYS_I2C_ADAPTERS)
 #include <i2c.h>
 #endif
 #include <spi.h>
@@ -214,11 +215,16 @@ static int init_func_ram(void)
 
 /***********************************************************************/
 
-#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
+#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C) \
+		|| defined(CONFIG_SYS_I2C_ADAPTERS)
 static int init_func_i2c(void)
 {
 	puts("I2C:   ");
+#ifdef CONFIG_SYS_I2C
+	i2c_init_all();
+#else
 	i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
+#endif
 	puts("ready\n");
 	return 0;
 }
@@ -317,7 +323,8 @@ init_fnc_t *init_sequence[] = {
 	misc_init_f,
 #endif
 	INIT_FUNC_WATCHDOG_RESET
-#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
+#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C) \
+		|| defined(CONFIG_SYS_I2C_ADAPTERS)
 	init_func_i2c,
 #endif
 #if defined(CONFIG_HARD_SPI)
@@ -823,6 +830,11 @@ void board_init_r(gd_t *id, ulong dest_addr)
 	 * the environment is in EEPROM.
 	 */
 
+#if defined(CONFIG_SYS_I2C) && defined(CONFIG_SYS_I2C_ADAPTERS)
+	/* Adjust I2C subsystem pointers after relocation */
+	i2c_reloc_fixup();
+#endif
+
 #if defined(CONFIG_SYS_EXTBDINFO)
 #if defined(CONFIG_405GP) || defined(CONFIG_405EP)
 #if defined(CONFIG_I2CFAST)
diff --git a/common/cmd_date.c b/common/cmd_date.c
index f0fa02a..21e72b8 100644
--- a/common/cmd_date.c
+++ b/common/cmd_date.c
@@ -50,8 +50,13 @@ int do_date (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	int old_bus;
 
 	/* switch to correct I2C bus */
+#ifdef CONFIG_SYS_I2C
+	old_bus = i2c_get_bus_num();
+	i2c_set_bus_num(CONFIG_SYS_RTC_BUS_NUM);
+#else
 	old_bus = I2C_GET_BUS();
 	I2C_SET_BUS(CONFIG_SYS_RTC_BUS_NUM);
+#endif
 
 	switch (argc) {
 	case 2:			/* set date & time */
@@ -98,7 +103,11 @@ int do_date (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 
 	/* switch back to original I2C bus */
+#ifdef CONFIG_SYS_I2C
+	i2c_set_bus_num(old_bus);
+#else
 	I2C_SET_BUS(old_bus);
+#endif
 
 	return rcode;
 }
diff --git a/common/cmd_dtt.c b/common/cmd_dtt.c
index cd94423..cb73614 100644
--- a/common/cmd_dtt.c
+++ b/common/cmd_dtt.c
@@ -69,8 +69,13 @@ int do_dtt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 	/* Force a compilation error, if there are more then 32 sensors */
 	BUILD_BUG_ON(sizeof(sensors) > 32);
 	/* switch to correct I2C bus */
+#ifdef CONFIG_SYS_I2C
+	old_bus = i2c_get_bus_num();
+	i2c_set_bus_num(CONFIG_SYS_DTT_BUS_NUM);
+#else
 	old_bus = I2C_GET_BUS();
 	I2C_SET_BUS(CONFIG_SYS_DTT_BUS_NUM);
+#endif
 
 	_initialize_dtt();
 
@@ -82,7 +87,11 @@ int do_dtt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 		printf("DTT%d: %i C\n", i + 1, dtt_get_temp(sensors[i]));
 
 	/* switch back to original I2C bus */
+#ifdef CONFIG_SYS_I2C
+	i2c_set_bus_num(old_bus);
+#else
 	I2C_SET_BUS(old_bus);
+#endif
 
 	return 0;
 }	/* do_dtt() */
diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
index e795139..50d006f 100644
--- a/common/cmd_i2c.c
+++ b/common/cmd_i2c.c
@@ -1,4 +1,9 @@
 /*
+ * (C) Copyright 2009
+ * Sergey Kubushyn, himself, ksi at koi8.net
+ *
+ * Changes for unified multibus/multiadapter I2C support.
+ *
  * (C) Copyright 2001
  * Gerald Van Baren, Custom IDEAS, vanbaren at cideas.com.
  *
@@ -83,6 +88,8 @@
 #include <malloc.h>
 #include <asm/byteorder.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 /* Display values from last command.
  * Memory modify remembered values are different from display memory.
  */
@@ -101,7 +108,8 @@ static uint	i2c_mm_last_alen;
  * pairs.  The following macros take care of this */
 
 #if defined(CONFIG_SYS_I2C_NOPROBES)
-#if defined(CONFIG_I2C_MULTI_BUS)
+#if (CONFIG_SYS_NUM_I2C_BUSSES > 1) || defined(CONFIG_I2C_MUX) || \
+	defined(CONFIG_I2C_MULTI_BUS)
 static struct
 {
 	uchar	bus;
@@ -117,7 +125,7 @@ static uchar i2c_no_probes[] = CONFIG_SYS_I2C_NOPROBES;
 #define COMPARE_BUS(b,i)	((b) == 0)	/* Make compiler happy */
 #define COMPARE_ADDR(a,i)	(i2c_no_probes[(i)] == (a))
 #define NO_PROBE_ADDR(i)	i2c_no_probes[(i)]
-#endif	/* CONFIG_MULTI_BUS */
+#endif	/* CONFIG_SYS_NUM_I2C_BUSSES > 1 */
 
 #define NUM_ELEMENTS_NOPROBE (sizeof(i2c_no_probes)/sizeof(i2c_no_probes[0]))
 #endif
@@ -125,22 +133,23 @@ static uchar i2c_no_probes[] = CONFIG_SYS_I2C_NOPROBES;
 #if defined(CONFIG_I2C_MUX)
 static I2C_MUX_DEVICE	*i2c_mux_devices = NULL;
 static	int	i2c_mux_busid = CONFIG_SYS_MAX_I2C_BUS;
-
-DECLARE_GLOBAL_DATA_PTR;
-
 #endif
 
 #define DISP_LINE_LEN	16
 
 /* implement possible board specific board init */
-void __def_i2c_init_board(void)
+int __def_i2c_init_board(void)
 {
-	return;
+	return 0;
 }
-void i2c_init_board(void)
+int i2c_init_board(void)
 	__attribute__((weak, alias("__def_i2c_init_board")));
 
-/* TODO: Implement architecture-specific get/set functions */
+#if !defined(CONFIG_SYS_I2C)
+/*
+ * TODO: Implement architecture-specific get/set functions
+ * Should go away, if we switched completely to new multibus support
+ */
 unsigned int __def_i2c_get_bus_speed(void)
 {
 	return CONFIG_SYS_I2C_SPEED;
@@ -157,6 +166,7 @@ int __def_i2c_set_bus_speed(unsigned int speed)
 }
 int i2c_set_bus_speed(unsigned int)
 	__attribute__((weak, alias("__def_i2c_set_bus_speed")));
+#endif
 
 /*
  * get_alen: small parser helper function to get address length
@@ -564,7 +574,7 @@ static int do_i2c_probe (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv
 	int j;
 #if defined(CONFIG_SYS_I2C_NOPROBES)
 	int k, skip;
-	uchar bus = GET_BUS_NUM;
+	unsigned int bus = GET_BUS_NUM;
 #endif	/* NOPROBES */
 
 	puts ("Valid chip addresses:");
@@ -1186,53 +1196,78 @@ static int do_sdram (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 }
 #endif
 
-#if defined(CONFIG_I2C_MUX)
-static int do_i2c_add_bus(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
+#if (CONFIG_SYS_NUM_I2C_BUSSES > 1) && defined(CONFIG_SYS_I2C)
+int do_i2c_show_bus(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	int ret=0;
+	int	i;
+#ifndef CONFIG_SYS_I2C_DIRECT_BUS
+	int	j;
+#endif
 
-	if (argc == 1) {
+	if (argc == 0) {
 		/* show all busses */
-		I2C_MUX		*mux;
-		I2C_MUX_DEVICE	*device = i2c_mux_devices;
-
-		printf ("Busses reached over muxes:\n");
-		while (device != NULL) {
-			printf ("Bus ID: %x\n", device->busid);
-			printf ("  reached over Mux(es):\n");
-			mux = device->mux;
-			while (mux != NULL) {
-				printf ("    %s@%x ch: %x\n", mux->name, mux->chip, mux->channel);
-				mux = mux->next;
+		for (i = 0; i < CONFIG_SYS_NUM_I2C_BUSSES; i++) {
+			printf("Bus %d:\t%s", i, I2C_ADAP_NR(i)->name);
+#ifndef CONFIG_SYS_I2C_DIRECT_BUS
+			for (j = 0; j < CONFIG_SYS_I2C_MAX_HOPS; j++) {
+				if (i2c_bus[i].next_hop[j].chip == 0)
+					break;
+				printf("->%s at 0x%2x:%d",
+					i2c_bus[i].next_hop[j].mux.name,
+					i2c_bus[i].next_hop[j].chip,
+					i2c_bus[i].next_hop[j].channel);
 			}
-			device = device->next;
+#endif
+			printf("\n");
 		}
 	} else {
-		(void)i2c_mux_ident_muxstring ((uchar *)argv[1]);
-		ret = 0;
+		/* show specific bus */
+		i = simple_strtoul(argv[1], NULL, 10);
+		if (i >= CONFIG_SYS_NUM_I2C_BUSSES) {
+			printf("Invalid bus %d\n", i);
+			return -1;
+		}
+		printf("Bus %d:\t%s", i, I2C_ADAP_NR(i)->name);
+#ifndef CONFIG_SYS_I2C_DIRECT_BUS
+			for (j = 0; j < CONFIG_SYS_I2C_MAX_HOPS; j++) {
+				if (i2c_bus[i].next_hop[j].chip == 0)
+					break;
+				printf("->%s at 0x%2x:%d",
+					i2c_bus[i].next_hop[j].mux.name,
+					i2c_bus[i].next_hop[j].chip,
+					i2c_bus[i].next_hop[j].channel);
+			}
+#endif
+		printf("\n");
 	}
-	return ret;
+
+	return 0 ;
 }
-#endif  /* CONFIG_I2C_MUX */
+#endif
 
-#if defined(CONFIG_I2C_MULTI_BUS)
-static int do_i2c_bus_num(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
+#if CONFIG_SYS_NUM_I2C_BUSSES > 1
+int do_i2c_bus_num(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	int bus_idx, ret=0;
+	int		ret = 0;
+	unsigned int	bus_no;
 
-	if (argc == 1)
+	if (argc == 0)
 		/* querying current setting */
 		printf("Current bus is %d\n", i2c_get_bus_num());
 	else {
-		bus_idx = simple_strtoul(argv[1], NULL, 10);
-		printf("Setting bus to %d\n", bus_idx);
-		ret = i2c_set_bus_num(bus_idx);
+		bus_no = simple_strtoul(argv[1], NULL, 10);
+		if (bus_no >= CONFIG_SYS_NUM_I2C_BUSSES) {
+			printf("Invalid bus %d\n", bus_no);
+			return -1;
+		}
+		printf("Setting bus to %d\n", bus_no);
+		ret = i2c_set_bus_num(bus_no);
 		if (ret)
 			printf("Failure changing bus number (%d)\n", ret);
 	}
 	return ret;
 }
-#endif  /* CONFIG_I2C_MULTI_BUS */
+#endif  /* CONFIG_SYS_NUM_I2C_BUSSES > 1 */
 
 static int do_i2c_bus_speed(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 {
@@ -1263,16 +1298,20 @@ static int do_i2c_nm(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 
 static int do_i2c_reset(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 {
+#if defined(CONFIG_SYS_I2C)
+	i2c_init(I2C_ADAP->speed, I2C_ADAP->slaveaddr);
+#else
 	i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
+#endif
 	return 0;
 }
 
 static cmd_tbl_t cmd_i2c_sub[] = {
-#if defined(CONFIG_I2C_MUX)
-	U_BOOT_CMD_MKENT(bus, 1, 1, do_i2c_add_bus, "", ""),
+#if (CONFIG_SYS_NUM_I2C_BUSSES > 1) && defined(CONFIG_SYS_I2C)
+	U_BOOT_CMD_MKENT(bus, 1, 1, do_i2c_show_bus, "", ""),
 #endif  /* CONFIG_I2C_MUX */
 	U_BOOT_CMD_MKENT(crc32, 3, 1, do_i2c_crc, "", ""),
-#if defined(CONFIG_I2C_MULTI_BUS)
+#if (CONFIG_SYS_NUM_I2C_BUSSES > 1) && defined(CONFIG_SYS_I2C)
 	U_BOOT_CMD_MKENT(dev, 1, 1, do_i2c_bus_num, "", ""),
 #endif  /* CONFIG_I2C_MULTI_BUS */
 	U_BOOT_CMD_MKENT(loop, 3, 1, do_i2c_loop, "", ""),
@@ -1319,11 +1358,11 @@ static int do_i2c(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 U_BOOT_CMD(
 	i2c, 6, 1, do_i2c,
 	"I2C sub-system",
-#if defined(CONFIG_I2C_MUX)
-	"bus [muxtype:muxaddr:muxchannel] - add a new bus reached over muxes\ni2c "
+#if CONFIG_SYS_NUM_I2C_BUSSES > 1
+	"bus [muxtype:muxaddr:muxchannel] - show I2C bus info\n"
 #endif  /* CONFIG_I2C_MUX */
 	"crc32 chip address[.0, .1, .2] count - compute CRC32 checksum\n"
-#if defined(CONFIG_I2C_MULTI_BUS)
+#if CONFIG_SYS_NUM_I2C_BUSSES > 1
 	"i2c dev [dev] - show or set current I2C bus\n"
 #endif  /* CONFIG_I2C_MULTI_BUS */
 	"i2c loop chip address[.0, .1, .2] [# of objects] - looping read of device\n"
diff --git a/common/stdio.c b/common/stdio.c
index 1bf9ba0..08337f9 100644
--- a/common/stdio.c
+++ b/common/stdio.c
@@ -1,4 +1,8 @@
 /*
+ * Copyright (C) 2009 Sergey Kubushyn <ksi@koi8.net>
+ *
+ * Changes for multibus/multiadapter I2C support.
+ *
  * (C) Copyright 2000
  * Paolo Scaffardi, AIRVENT SAM s.p.a - RIMINI(ITALY), arsenio at tin.it
  *
@@ -30,7 +34,8 @@
 #ifdef CONFIG_LOGBUFFER
 #include <logbuff.h>
 #endif
-#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
+#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C) \
+		|| defined(CONFIG_SYS_I2C_ADAPTERS)
 #include <i2c.h>
 #endif
 
@@ -211,9 +216,15 @@ int stdio_init (void)
 #ifdef CONFIG_ARM_DCC_MULTI
 	drv_arm_dcc_init ();
 #endif
+#ifdef CONFIG_SYS_I2C
+#ifdef CONFIG_SYS_I2C_ADAPTERS
+	i2c_init_all();
+#endif
+#else
 #if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
 	i2c_init (CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
 #endif
+#endif
 #ifdef CONFIG_LCD
 	drv_lcd_init ();
 #endif
diff --git a/include/i2c.h b/include/i2c.h
index 910552d..e482827 100644
--- a/include/i2c.h
+++ b/include/i2c.h
@@ -203,15 +203,6 @@ int i2c_mux_ident_muxstring_f (uchar *buf);
 
 #ifdef CONFIG_SYS_I2C
 /*
- * Initialization, must be called once on start up, may be called
- * repeatedly to change the speed and slave addresses.
- */
-void i2c_init(unsigned int speed, int slaveaddr);
-#ifdef CONFIG_SYS_I2C_INIT_BOARD
-void i2c_init_board(void);
-#endif
-
-/*
  * i2c_get_bus_num:
  *
  *  Returns index of currently active I2C bus.  Zero-based.
-- 
1.7.7.3

[U-Boot] [PATCH 3/3] WIP: tegra: i2c: Enable new I2C framework

[Simon Glass]

This is just a rough patch to show how this might be done.

Not to be applied, please.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 drivers/i2c/tegra2_i2c.c   |   53 +++++++++++++++++++------------------------
 include/configs/seaboard.h |    2 +
 2 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/drivers/i2c/tegra2_i2c.c b/drivers/i2c/tegra2_i2c.c
index b42d9ac..93f3269 100644
--- a/drivers/i2c/tegra2_i2c.c
+++ b/drivers/i2c/tegra2_i2c.c
@@ -30,7 +30,9 @@
 #include <asm/arch/gpio.h>
 #include <asm/arch/pinmux.h>
 #include <asm/arch/tegra2_i2c.h>
+
 #include <fdtdec.h>
+#include <i2c.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -296,12 +298,7 @@ static int tegra2_i2c_read_data(u32 addr, u8 *data, u32 len)
 #error "Please enable device tree support to use this driver"
 #endif
 
-unsigned int i2c_get_bus_speed(void)
-{
-	return i2c_controllers[i2c_bus_num].speed;
-}
-
-int i2c_set_bus_speed(unsigned int speed)
+uint tegra_i2c_set_bus_speed(unsigned int speed)
 {
 	struct i2c_bus *i2c_bus;
 
@@ -309,7 +306,7 @@ int i2c_set_bus_speed(unsigned int speed)
 	i2c_bus->speed = speed;
 	i2c_init_controller(i2c_bus);
 
-	return 0;
+	return 0;	/* TODO: return actual speed */
 }
 
 static int i2c_get_config(const void *blob, int node, struct i2c_bus *i2c_bus)
@@ -404,7 +401,7 @@ int i2c_init_board(void)
 	return 0;
 }
 
-void i2c_init(int speed, int slaveaddr)
+void tegra_i2c_init(int speed, int slaveaddr)
 {
 	debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
 }
@@ -450,7 +447,7 @@ int i2c_read_data(uchar chip, uchar *buffer, int len)
 }
 
 /* Probe to see if a chip is present. */
-int i2c_probe(uchar chip)
+int tegra_i2c_probe(uchar chip)
 {
 	int rc;
 	uchar reg;
@@ -472,7 +469,7 @@ static int i2c_addr_ok(const uint addr, const int alen)
 }
 
 /* Read bytes */
-int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
+int tegra_i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 {
 	uint offset;
 	int i;
@@ -506,7 +503,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 }
 
 /* Write bytes */
-int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
+int tegra_i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 {
 	uint offset;
 	int i;
@@ -531,25 +528,6 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 	return 0;
 }
 
-#if defined(CONFIG_I2C_MULTI_BUS)
-/*
- * Functions for multiple I2C bus handling
- */
-unsigned int i2c_get_bus_num(void)
-{
-	return i2c_bus_num;
-}
-
-int i2c_set_bus_num(unsigned int bus)
-{
-	if (bus >= CONFIG_SYS_MAX_I2C_BUS || !i2c_controllers[bus].inited)
-		return -1;
-	i2c_bus_num = bus;
-
-	return 0;
-}
-#endif
-
 int tegra_i2c_get_dvc_bus_num(void)
 {
 	int i;
@@ -563,3 +541,18 @@ int tegra_i2c_get_dvc_bus_num(void)
 
 	return -1;
 }
+
+struct i2c_adapter tegra_i2c_adap[] = {
+	{
+		.init		= tegra_i2c_init,
+		.probe		= tegra_i2c_probe,
+		.read		= tegra_i2c_read,
+		.write		= tegra_i2c_write,
+		.set_bus_speed	= tegra_i2c_set_bus_speed,
+		.speed		= 100000,
+		.slaveaddr	= 0,
+		.name		= "tegra-i2c",
+		.init_done	= 0,
+		.hwadapnr	= 0,
+	}
+};
diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h
index d2d0115..64c804a 100644
--- a/include/configs/seaboard.h
+++ b/include/configs/seaboard.h
@@ -79,6 +79,8 @@
 #define CONFIG_SYS_MAX_I2C_BUS		4
 #define CONFIG_SYS_I2C_SPEED		100000
 #define CONFIG_CMD_I2C
+#define CONFIG_SYS_I2C
+#define CONFIG_SYS_I2C_ADAPTERS		{&tegra_i2c_adap}
 
 /* SD/MMC */
 #define CONFIG_MMC
-- 
1.7.7.3

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Heiko Schocher]

Hello Simon,

Simon Glass wrote:
> This series provides Heiko's upgraded I2C framework from a few years ago.
> I hope that we can bring this in and move boards over to it as time
> permits, rather than switching everything in one fell swoop which never
> happens.

Ok, lets try it!

> To show it working I have enabled it for Tegra in a very rough way. It
> seems fine with my limited testing.

Great! Thanks! Patches for other i2c drivers can be found here:

http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2_20111112

They just need a rebase and an update to your changes (and of course
some tests)

> In terms of changes, I have just fixed some checkpatch errors and fiddled
> with a couple of function signatures.
> 
> I will start a thread on the list with a few thoughts on this series
> at some point.

Ok, thanks. Here some thoughts comming in my mind:

- Why a "cur_adap" added in gd_t:
  - This points allways to the current used i2c adapter.
  - because gd_t is writeable when running in flash,
    complete multiadapter/multibus functionality is
    usable, when running in flash, which is needed for
    some SoCs.
  - using a pointer brings faster accesses to the i2c_adapter_t
    struct and saves some bytes here and there.

- init from a i2c controller:
  In current code all i2c controllers, as a precaution,
  getting initialized. In the new code, a i2c controller
  gets only initialized if it is used. This is done in
  i2c_set_bus_num().

  Also, with this approach, we can easy add in a second step,
  a i2c_deinit() function, called from i2c_set_bus_num(),
  so we can easy deactivate a no longer used controller.

- added "hw_adapnr" in i2c_adapter_t struct:
  when for example a CPU has more then one i2c controller
  we can use this variable to differentiate which
  controller the actual i2c adapter uses.

- Maybe we should add a base_addr field in struct i2c_adapter?
  This would help for SoCs, who have more then one identical
  controller, just differ in their base_addr...
  (Currently I made a function, or an array which returns
  the base_addr, dependend on "hw_adapnr"). We should drop
  this, and introduce a "base_addr" field.

[...]

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

[U-Boot] [PATCH 3/3] WIP: tegra: i2c: Enable new I2C framework

[Heiko Schocher]

Hello Simon,

Simon Glass wrote:
> This is just a rough patch to show how this might be done.
> 
> Not to be applied, please.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  drivers/i2c/tegra2_i2c.c   |   53 +++++++++++++++++++------------------------
>  include/configs/seaboard.h |    2 +
>  2 files changed, 25 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/i2c/tegra2_i2c.c b/drivers/i2c/tegra2_i2c.c
> index b42d9ac..93f3269 100644
> --- a/drivers/i2c/tegra2_i2c.c
> +++ b/drivers/i2c/tegra2_i2c.c
> @@ -30,7 +30,9 @@
>  #include <asm/arch/gpio.h>
>  #include <asm/arch/pinmux.h>
>  #include <asm/arch/tegra2_i2c.h>
> +
>  #include <fdtdec.h>
> +#include <i2c.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -296,12 +298,7 @@ static int tegra2_i2c_read_data(u32 addr, u8 *data, u32 len)
>  #error "Please enable device tree support to use this driver"
>  #endif
>  
> -unsigned int i2c_get_bus_speed(void)
> -{
> -	return i2c_controllers[i2c_bus_num].speed;
> -}
> -
> -int i2c_set_bus_speed(unsigned int speed)
> +uint tegra_i2c_set_bus_speed(unsigned int speed)

static

>  {
>  	struct i2c_bus *i2c_bus;
>  
> @@ -309,7 +306,7 @@ int i2c_set_bus_speed(unsigned int speed)
>  	i2c_bus->speed = speed;
>  	i2c_init_controller(i2c_bus);
>  
> -	return 0;
> +	return 0;	/* TODO: return actual speed */
>  }
>  
>  static int i2c_get_config(const void *blob, int node, struct i2c_bus *i2c_bus)
> @@ -404,7 +401,7 @@ int i2c_init_board(void)
>  	return 0;
>  }
>  
> -void i2c_init(int speed, int slaveaddr)
> +void tegra_i2c_init(int speed, int slaveaddr)

static, add this for all functions

[...]
> diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h
> index d2d0115..64c804a 100644
> --- a/include/configs/seaboard.h
> +++ b/include/configs/seaboard.h
> @@ -79,6 +79,8 @@
>  #define CONFIG_SYS_MAX_I2C_BUS		4

This define is no longer valid if using the new framework.
Do you have really 4 busses with only one i2c adapter?

This works only, if you have i2c muxes! In this case you have to
define

/*
 * how many muxes are maximal consecutively connected on
 * one i2c bus
 */
#define CONFIG_SYS_I2C_MAX_HOPS	1

and for example using a PCA9547 mux:

#define CONFIG_SYS_I2C_BUSSES   {       {0, {I2C_NULL_HOP}}, \
                                        {0, {{I2C_MUX_PCA9547, 0x70, 1}}}, \
                                        {0, {{I2C_MUX_PCA9547, 0x70, 2}}}, \
                                        {0, {{I2C_MUX_PCA9547, 0x70, 3}}},
				}

or you use other i2c adapters like soft_i2c, but I could not see
this in your patch ... and in this case you must define
CONFIG_SYS_NUM_I2C_ADAPTERS ...

>  #define CONFIG_SYS_I2C_SPEED		100000

should not longer be defined, instead you should make a option to define
a i2c speed for your i2c driver ... for example
#define CONFIG_SYS_I2C_TEGRA2_SPEED	100000

>  #define CONFIG_CMD_I2C
> +#define CONFIG_SYS_I2C
> +#define CONFIG_SYS_I2C_ADAPTERS		{&tegra_i2c_adap}

You must have a define in drivers/i2c/Makefile which "activates" your
drivers/i2c/tegra2_i2c.c ... this must defined in your board config
file too (maybe a CONFIG_SYS_I2C_TEGRA2 ?)!

Also you must have a change in drivers/i2c/i2c_core.c

#ifdef CONFIG_SYS_I2C_TEGRA2
extern struct i2c_adap       tegra_i2c_adap[];
#endif

or?

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

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Mike Frysinger]

On Tuesday 17 January 2012 02:12:23 Simon Glass wrote:
> +#if defined(CONFIG_SYS_I2C)
> +	void		*cur_adap;	/* current used i2c adapter */
> +#endif

let's have "i2c" in the variable name somewhere.  "curr_i2c" ?

> --- /dev/null
> +++ b/drivers/i2c/i2c_core.c
>
> +#ifdef CONFIG_TEGRA2_I2C
> +extern struct i2c_adapter tegra_i2c_adap[];
> +#endif

i feel like if your initial run includes i2c bus specific defines, the end 
result is a failure

> +struct i2c_adapter *i2c_adap[CONFIG_SYS_NUM_I2C_ADAPTERS] =
> +			CONFIG_SYS_I2C_ADAPTERS;
> +#ifndef CONFIG_SYS_I2C_DIRECT_BUS
> +struct i2c_bus	i2c_bus[CONFIG_SYS_NUM_I2C_BUSSES] = CONFIG_SYS_I2C_BUSSES;
> +#endif

the nand layer is moving away from this style ...

> + * mux_id is the multiplexer chip type from defined in i2c.h. So far only
> + * NXP (Philips) PCA954x multiplexers are supported. Switches are NOT
> + * supported (anybody uses them?)

does part-specific stuff belong in the core ?  seems like it should be in a 
part-specific driver ...

> +int i2c_probe(u_int8_t chip)

use uintx_t types.  the u_intx_t types are deprecated.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120117/35443437/attachment.pgp>

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Tabi Timur-B04825]

On Tue, Jan 17, 2012 at 1:12 AM, Simon Glass <sjg@chromium.org> wrote:
> From: Heiko Schocher <hs@denx.de>
>
> This Patch introduce the new i2c_core file, which holds
> the I2C core functions, for the rework of the multibus/
> multiadapter support.
> Also adds changes in i2c.h for the new I2C multibus/multiadapter
> support. This new support can be activated with the
> CONFIG_SYS_I2C define.
>
> Signed-off-by: Heiko Schocher <hs@denx.de>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> ?arch/arm/include/asm/global_data.h ? ? ? ?| ? ?3 +
> ?arch/avr32/include/asm/global_data.h ? ? ?| ? ?3 +
> ?arch/blackfin/include/asm/global_data.h ? | ? ?4 +-
> ?arch/m68k/include/asm/global_data.h ? ? ? | ? ?3 +
> ?arch/microblaze/include/asm/global_data.h | ? ?3 +
> ?arch/mips/include/asm/global_data.h ? ? ? | ? ?3 +
> ?arch/nios2/include/asm/global_data.h ? ? ?| ? ?3 +
> ?arch/powerpc/include/asm/global_data.h ? ?| ? ?3 +
> ?arch/sh/include/asm/global_data.h ? ? ? ? | ? ?3 +
> ?arch/sparc/include/asm/global_data.h ? ? ?| ? ?3 +
> ?arch/x86/include/asm/global_data.h ? ? ? ?| ? ?3 +
> ?drivers/i2c/Makefile ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
> ?drivers/i2c/i2c_core.c ? ? ? ? ? ? ? ? ? ?| ?360 +++++++++++++++++++++++++++++
> ?include/i2c.h ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ?199 +++++++++++++++-
> ?14 files changed, 584 insertions(+), 10 deletions(-)
> ?create mode 100644 drivers/i2c/i2c_core.c
>
> diff --git a/arch/arm/include/asm/global_data.h b/arch/arm/include/asm/global_data.h
> index 23a6077..924cea2 100644
> --- a/arch/arm/include/asm/global_data.h
> +++ b/arch/arm/include/asm/global_data.h
> @@ -87,6 +87,9 @@ typedef ? ? ? struct ?global_data {
> ? ? ? ?unsigned long ? post_log_res; /* success of POST test */
> ? ? ? ?unsigned long ? post_init_f_time; /* When post_init_f started */
> ?#endif
> +#if defined(CONFIG_SYS_I2C)
> + ? ? ? void ? ? ? ? ? ?*cur_adap; ? ? ?/* current used i2c adapter */
> +#endif

I was really hoping we could get rid of the concept of a "current" i2c
adapter, and just force all drivers to specify the I2C adapter they
want to use for a given I2C operation.  That's how Linux operates, and
it will prevent stuff like this:

void *old;
void *new;


old = get_current_i2c_adapter();
set_i2c_adapter(new);
// do I2C stuff
set_i2c_adapter(old);

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Mike Frysinger]

On Wednesday 18 January 2012 15:11:56 Tabi Timur-B04825 wrote:
> On Tue, Jan 17, 2012 at 1:12 AM, Simon Glass wrote:
> > --- a/arch/arm/include/asm/global_data.h
> > +++ b/arch/arm/include/asm/global_data.h
> > @@ -87,6 +87,9 @@ typedef       struct  global_data {
> >        unsigned long   post_log_res; /* success of POST test */
> >        unsigned long   post_init_f_time; /* When post_init_f started */
> >  #endif
> > +#if defined(CONFIG_SYS_I2C)
> > +       void            *cur_adap;      /* current used i2c adapter */
> > +#endif
> 
> I was really hoping we could get rid of the concept of a "current" i2c
> adapter, and just force all drivers to specify the I2C adapter they
> want to use for a given I2C operation.  That's how Linux operates, and
> it will prevent stuff like this:
> 
> void *old;
> void *new;
> 
> old = get_current_i2c_adapter();
> set_i2c_adapter(new);
> // do I2C stuff
> set_i2c_adapter(old);

that's only needed if you expect the pointer to stay valid across calls.  i 
don't think it does for most (all?) drivers.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120118/75ccaa78/attachment.pgp>

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Timur Tabi]

Mike Frysinger wrote:
> that's only needed if you expect the pointer to stay valid across calls.  i 
> don't think it does for most (all?) drivers.

True, but it's hard to know sometimes when it's needed.  I do it in my
code just to be sure.

Regardless, I still think the idea of a "current" i2c bus is flawed.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Simon Glass]

Hi Tabi,

On Wed, Jan 18, 2012 at 12:11 PM, Tabi Timur-B04825
<B04825@freescale.com> wrote:
> On Tue, Jan 17, 2012 at 1:12 AM, Simon Glass <sjg@chromium.org> wrote:
>> From: Heiko Schocher <hs@denx.de>
>>
>> This Patch introduce the new i2c_core file, which holds
>> the I2C core functions, for the rework of the multibus/
>> multiadapter support.
>> Also adds changes in i2c.h for the new I2C multibus/multiadapter
>> support. This new support can be activated with the
>> CONFIG_SYS_I2C define.
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>> ?arch/arm/include/asm/global_data.h ? ? ? ?| ? ?3 +
>> ?arch/avr32/include/asm/global_data.h ? ? ?| ? ?3 +
>> ?arch/blackfin/include/asm/global_data.h ? | ? ?4 +-
>> ?arch/m68k/include/asm/global_data.h ? ? ? | ? ?3 +
>> ?arch/microblaze/include/asm/global_data.h | ? ?3 +
>> ?arch/mips/include/asm/global_data.h ? ? ? | ? ?3 +
>> ?arch/nios2/include/asm/global_data.h ? ? ?| ? ?3 +
>> ?arch/powerpc/include/asm/global_data.h ? ?| ? ?3 +
>> ?arch/sh/include/asm/global_data.h ? ? ? ? | ? ?3 +
>> ?arch/sparc/include/asm/global_data.h ? ? ?| ? ?3 +
>> ?arch/x86/include/asm/global_data.h ? ? ? ?| ? ?3 +
>> ?drivers/i2c/Makefile ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
>> ?drivers/i2c/i2c_core.c ? ? ? ? ? ? ? ? ? ?| ?360 +++++++++++++++++++++++++++++
>> ?include/i2c.h ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ?199 +++++++++++++++-
>> ?14 files changed, 584 insertions(+), 10 deletions(-)
>> ?create mode 100644 drivers/i2c/i2c_core.c
>>
>> diff --git a/arch/arm/include/asm/global_data.h b/arch/arm/include/asm/global_data.h
>> index 23a6077..924cea2 100644
>> --- a/arch/arm/include/asm/global_data.h
>> +++ b/arch/arm/include/asm/global_data.h
>> @@ -87,6 +87,9 @@ typedef ? ? ? struct ?global_data {
>> ? ? ? ?unsigned long ? post_log_res; /* success of POST test */
>> ? ? ? ?unsigned long ? post_init_f_time; /* When post_init_f started */
>> ?#endif
>> +#if defined(CONFIG_SYS_I2C)
>> + ? ? ? void ? ? ? ? ? ?*cur_adap; ? ? ?/* current used i2c adapter */
>> +#endif
>
> I was really hoping we could get rid of the concept of a "current" i2c
> adapter, and just force all drivers to specify the I2C adapter they
> want to use for a given I2C operation. ?That's how Linux operates, and
> it will prevent stuff like this:

I agree completely, it was one of the things I was going to ask for.
We should add a new parameter to calls instead IMO.

>
> void *old;
> void *new;
>
>
> old = get_current_i2c_adapter();
> set_i2c_adapter(new);
> // do I2C stuff
> set_i2c_adapter(old);
>
> --
> Timur Tabi
> Linux kernel developer at Freescale

Regards,
Simon

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Timur Tabi]

Simon Glass wrote:
> I agree completely, it was one of the things I was going to ask for.
> We should add a new parameter to calls instead IMO.

I would be in favor of a patch that replaces all of the I2C calls.  It
would be a massive patch, but it solve a lot of problems in one shot.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Simon Glass]

Hi Timur,

On Wed, Jan 18, 2012 at 1:39 PM, Timur Tabi <timur@freescale.com> wrote:
> Simon Glass wrote:
>> I agree completely, it was one of the things I was going to ask for.
>> We should add a new parameter to calls instead IMO.
>
> I would be in favor of a patch that replaces all of the I2C calls. ?It
> would be a massive patch, but it solve a lot of problems in one shot.

I agree. Do you know of such a patch? :-)

Regards,
Simon

>
> --
> Timur Tabi
> Linux kernel developer at Freescale
>

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Timur Tabi]

Simon Glass wrote:
> I agree. Do you know of such a patch? :-)

No, but it wouldn't be heard to create -- mostly a global
search-and-replace.  I wouldn't even attempt it without getting
pre-approved by Wolfgang first, though.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ2kLPfMzzwgHkDJ4eL+wixqLv89+CvdVP7PCcy+XFaqNQ@mail.gmail.com> you wrote:
> 
> > I was really hoping we could get rid of the concept of a "current" i2c
> > adapter, and just force all drivers to specify the I2C adapter they
> > want to use for a given I2C operation. =A0That's how Linux operates, and
> > it will prevent stuff like this:
> 
> I agree completely, it was one of the things I was going to ask for.
> We should add a new parameter to calls instead IMO.

Let's do one step at a time.  Now we go for multi-bus support.
Implementing a new, better device interface is one of the next steps,
then.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Free markets select for winning solutions."        - Eric S. Raymond

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Heiko Schocher]

Hello Wolfgang, Timur, Simon,

Wolfgang Denk wrote:
> Dear Simon Glass,
> 
> In message <CAPnjgZ2kLPfMzzwgHkDJ4eL+wixqLv89+CvdVP7PCcy+XFaqNQ@mail.gmail.com> you wrote:
>>> I was really hoping we could get rid of the concept of a "current" i2c
>>> adapter, and just force all drivers to specify the I2C adapter they
>>> want to use for a given I2C operation. =A0That's how Linux operates, and
>>> it will prevent stuff like this:
>> I agree completely, it was one of the things I was going to ask for.
>> We should add a new parameter to calls instead IMO.
> 
> Let's do one step at a time.  Now we go for multi-bus support.

Ok.

> Implementing a new, better device interface is one of the next steps,
> then.

Some thoughts to the subject "get rid of concept of a current i2c"

- First, it would be great to get rid of that ;-)

- 2 reasons why we currently need the info, what is the current
  i2c adapter/i2c bus:

- U-Boot principle says, don't init a device, if you don't use it.
  So, if we switch to another i2c adapter or i2c bus (A i2c bus is a
  combination if one i2c adpater and n i2c muxes), we must deinit
  the current adapter/bus. Ok, current code didn't this too, but
  this should a goal to keep in mind ... or we define, that this
  is not needed.

- If we have i2c muxes, and you don't know your current i2c bus,
  you must on every i2c access also setup the i2c muxes on this
  i2c bus ... would we do this really?
  if we have the current i2c bus info, we just have to check, if
  we are on this bus, and do the i2c access ... if we are not on
  this i2c bus, we can deinit it complete (the new i2c_core disconnects
  for example all i2c muxes on the i2c bus) and init the new bus.
  All this work is done in a central function i2c_set_bus_numer()

- Looking in the new multibus i2c_core.c file, we should get rid of

  static unsigned int i2c_cur_bus __attribute__((section(".data"))) =
                                CONFIG_SYS_SPD_BUS_NUM;

  and "cur_adap" renamed to "cur_i2c_bus" and should be a
  "struct i2c_bus_hose" pointer.

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

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Simon Glass]

Hi Heiko,

On Wed, Jan 18, 2012 at 10:35 PM, Heiko Schocher <hs@denx.de> wrote:
> Hello Wolfgang, Timur, Simon,
>
> Wolfgang Denk wrote:
>> Dear Simon Glass,
>>
>> In message <CAPnjgZ2kLPfMzzwgHkDJ4eL+wixqLv89+CvdVP7PCcy+XFaqNQ@mail.gmail.com> you wrote:
>>>> I was really hoping we could get rid of the concept of a "current" i2c
>>>> adapter, and just force all drivers to specify the I2C adapter they
>>>> want to use for a given I2C operation. =A0That's how Linux operates, and
>>>> it will prevent stuff like this:
>>> I agree completely, it was one of the things I was going to ask for.
>>> We should add a new parameter to calls instead IMO.
>>
>> Let's do one step at a time. ?Now we go for multi-bus support.
>
> Ok.
>
>> Implementing a new, better device interface is one of the next steps,
>> then.
>
> Some thoughts to the subject "get rid of concept of a current i2c"
>
> - First, it would be great to get rid of that ;-)
>
> - 2 reasons why we currently need the info, what is the current
> ?i2c adapter/i2c bus:
>
> - U-Boot principle says, don't init a device, if you don't use it.
> ?So, if we switch to another i2c adapter or i2c bus (A i2c bus is a
> ?combination if one i2c adpater and n i2c muxes), we must deinit
> ?the current adapter/bus. Ok, current code didn't this too, but
> ?this should a goal to keep in mind ... or we define, that this
> ?is not needed.
>
> - If we have i2c muxes, and you don't know your current i2c bus,
> ?you must on every i2c access also setup the i2c muxes on this
> ?i2c bus ... would we do this really?

Ignoring muxes, we can have more than one i2c adaptor inited at a time
(i.e. de-initing is optional).

Perhaps reword this slightly. U-Boot can have knowledge of a current
adaptor, mux settings and so on, and use this internally within the
i2c layer to optimise performance and redundant i2c traffic. But the
pain is when the concept of a 'current adaptor' is exposed outside the
i2c layer.

> ?if we have the current i2c bus info, we just have to check, if
> ?we are on this bus, and do the i2c access ... if we are not on
> ?this i2c bus, we can deinit it complete (the new i2c_core disconnects
> ?for example all i2c muxes on the i2c bus) and init the new bus.
> ?All this work is done in a central function i2c_set_bus_numer()

Yes but this function could become internal perhaps.

>
> - Looking in the new multibus i2c_core.c file, we should get rid of
>
> ?static unsigned int i2c_cur_bus __attribute__((section(".data"))) =
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?CONFIG_SYS_SPD_BUS_NUM;
>
> ?and "cur_adap" renamed to "cur_i2c_bus" and should be a
> ?"struct i2c_bus_hose" pointer.

Sounds good. Heiko do you have time to take over your two patches I
sent and tidy them up, or should I continue?

Regards,
Simon

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

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Heiko Schocher]

Hello Simon,

Simon Glass wrote:
> Hi Heiko,
> 
> On Wed, Jan 18, 2012 at 10:35 PM, Heiko Schocher <hs@denx.de> wrote:
>> Hello Wolfgang, Timur, Simon,
>>
>> Wolfgang Denk wrote:
>>> Dear Simon Glass,
>>>
>>> In message <CAPnjgZ2kLPfMzzwgHkDJ4eL+wixqLv89+CvdVP7PCcy+XFaqNQ@mail.gmail.com> you wrote:
>>>>> I was really hoping we could get rid of the concept of a "current" i2c
>>>>> adapter, and just force all drivers to specify the I2C adapter they
>>>>> want to use for a given I2C operation. =A0That's how Linux operates, and
>>>>> it will prevent stuff like this:
>>>> I agree completely, it was one of the things I was going to ask for.
>>>> We should add a new parameter to calls instead IMO.
>>> Let's do one step at a time.  Now we go for multi-bus support.
>> Ok.
>>
>>> Implementing a new, better device interface is one of the next steps,
>>> then.
>> Some thoughts to the subject "get rid of concept of a current i2c"
>>
>> - First, it would be great to get rid of that ;-)
>>
>> - 2 reasons why we currently need the info, what is the current
>>  i2c adapter/i2c bus:
>>
>> - U-Boot principle says, don't init a device, if you don't use it.
>>  So, if we switch to another i2c adapter or i2c bus (A i2c bus is a
>>  combination if one i2c adpater and n i2c muxes), we must deinit
>>  the current adapter/bus. Ok, current code didn't this too, but
>>  this should a goal to keep in mind ... or we define, that this
>>  is not needed.
>>
>> - If we have i2c muxes, and you don't know your current i2c bus,
>>  you must on every i2c access also setup the i2c muxes on this
>>  i2c bus ... would we do this really?
> 
> Ignoring muxes, we can have more than one i2c adaptor inited at a time
> (i.e. de-initing is optional).

Ok for the point "more adapters inited at one time".

But we have to setup the muxes every time! We cannot ignore this.

> Perhaps reword this slightly. U-Boot can have knowledge of a current
> adaptor, mux settings and so on, and use this internally within the
> i2c layer to optimise performance and redundant i2c traffic. But the

Ok, thats what I mean. The "cur_i2c_bus" pointer should only be used
in the i2c subsystem! This pointer is in gd_t because it must be
writeable, also when running from flash ...

> pain is when the concept of a 'current adaptor' is exposed outside the
> i2c layer.

The "cur_i2c_bus" pointer is only used inside the i2c subsystem.

>>  if we have the current i2c bus info, we just have to check, if
>>  we are on this bus, and do the i2c access ... if we are not on
>>  this i2c bus, we can deinit it complete (the new i2c_core disconnects
>>  for example all i2c muxes on the i2c bus) and init the new bus.
>>  All this work is done in a central function i2c_set_bus_numer()
> 
> Yes but this function could become internal perhaps.

Yes, if we make the change, Timur suggested ... but that can be done
easy in a second step, as Wolfgang mentioned.

>> - Looking in the new multibus i2c_core.c file, we should get rid of
>>
>>  static unsigned int i2c_cur_bus __attribute__((section(".data"))) =
>>                                CONFIG_SYS_SPD_BUS_NUM;
>>
>>  and "cur_adap" renamed to "cur_i2c_bus" and should be a
>>  "struct i2c_bus_hose" pointer.
> 
> Sounds good. Heiko do you have time to take over your two patches I
> sent and tidy them up, or should I continue?

I did some work on this 2 patches, to get it working with soft_i2c.c
driver ... I try to rework this suggestions in, and send it to the
ML as a v2 ... if I do not find time, I speak up ;-)

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

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ1RoyguaAScOQiw8PoQkzVp5rim7KJ6zzHLNPwuBYK48Q@mail.gmail.com> you wrote:
> 
> Perhaps reword this slightly. U-Boot can have knowledge of a current
> adaptor, mux settings and so on, and use this internally within the
> i2c layer to optimise performance and redundant i2c traffic. But the
> pain is when the concept of a 'current adaptor' is exposed outside the
> i2c layer.

As mentioned before, this is what we currently have as "device model"
in U-Boot - not only I2C: we have the same "current device" concept
with IDE, USB, ...

It makes no sense trying to change it here; it would only cause even
more incompatibilities.  If you want to fix that, then help working on
the new driver model.  Marek is planning for this.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Good morning. This is the telephone company. Due  to  repairs,  we're
giving  you  advance notice that your service will be cut off indefi-
nitely at ten o'clock. That's two minutes from now.

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Simon Glass]

Hi Heiko,

On Wed, Jan 18, 2012 at 11:53 PM, Heiko Schocher <hs@denx.de> wrote:
> Hello Simon,
>
> Simon Glass wrote:
>> Hi Heiko,
>>
>> On Wed, Jan 18, 2012 at 10:35 PM, Heiko Schocher <hs@denx.de> wrote:
>>> Hello Wolfgang, Timur, Simon,
>>>
>>> Wolfgang Denk wrote:
>>>> Dear Simon Glass,
>>>>
>>>> In message <CAPnjgZ2kLPfMzzwgHkDJ4eL+wixqLv89+CvdVP7PCcy+XFaqNQ@mail.gmail.com> you wrote:
>>>>>> I was really hoping we could get rid of the concept of a "current" i2c
>>>>>> adapter, and just force all drivers to specify the I2C adapter they
>>>>>> want to use for a given I2C operation. =A0That's how Linux operates, and
>>>>>> it will prevent stuff like this:
>>>>> I agree completely, it was one of the things I was going to ask for.
>>>>> We should add a new parameter to calls instead IMO.
>>>> Let's do one step at a time. ?Now we go for multi-bus support.
>>> Ok.
>>>
>>>> Implementing a new, better device interface is one of the next steps,
>>>> then.
>>> Some thoughts to the subject "get rid of concept of a current i2c"
>>>
>>> - First, it would be great to get rid of that ;-)
>>>
>>> - 2 reasons why we currently need the info, what is the current
>>> ?i2c adapter/i2c bus:
>>>
>>> - U-Boot principle says, don't init a device, if you don't use it.
>>> ?So, if we switch to another i2c adapter or i2c bus (A i2c bus is a
>>> ?combination if one i2c adpater and n i2c muxes), we must deinit
>>> ?the current adapter/bus. Ok, current code didn't this too, but
>>> ?this should a goal to keep in mind ... or we define, that this
>>> ?is not needed.
>>>
>>> - If we have i2c muxes, and you don't know your current i2c bus,
>>> ?you must on every i2c access also setup the i2c muxes on this
>>> ?i2c bus ... would we do this really?
>>
>> Ignoring muxes, we can have more than one i2c adaptor inited at a time
>> (i.e. de-initing is optional).
>
> Ok for the point "more adapters inited at one time".
>
> But we have to setup the muxes every time! We cannot ignore this.
>
>> Perhaps reword this slightly. U-Boot can have knowledge of a current
>> adaptor, mux settings and so on, and use this internally within the
>> i2c layer to optimise performance and redundant i2c traffic. But the
>
> Ok, thats what I mean. The "cur_i2c_bus" pointer should only be used
> in the i2c subsystem! This pointer is in gd_t because it must be
> writeable, also when running from flash ...

Yes that's fine.

>
>> pain is when the concept of a 'current adaptor' is exposed outside the
>> i2c layer.
>
> The "cur_i2c_bus" pointer is only used inside the i2c subsystem.

OK good.

>
>>> ?if we have the current i2c bus info, we just have to check, if
>>> ?we are on this bus, and do the i2c access ... if we are not on
>>> ?this i2c bus, we can deinit it complete (the new i2c_core disconnects
>>> ?for example all i2c muxes on the i2c bus) and init the new bus.
>>> ?All this work is done in a central function i2c_set_bus_numer()
>>
>> Yes but this function could become internal perhaps.
>
> Yes, if we make the change, Timur suggested ... but that can be done
> easy in a second step, as Wolfgang mentioned.

Yes indeed, and quite a major one by the sound of it.

>
>>> - Looking in the new multibus i2c_core.c file, we should get rid of
>>>
>>> ?static unsigned int i2c_cur_bus __attribute__((section(".data"))) =
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?CONFIG_SYS_SPD_BUS_NUM;
>>>
>>> ?and "cur_adap" renamed to "cur_i2c_bus" and should be a
>>> ?"struct i2c_bus_hose" pointer.
>>
>> Sounds good. Heiko do you have time to take over your two patches I
>> sent and tidy them up, or should I continue?
>
> I did some work on this 2 patches, to get it working with soft_i2c.c
> driver ... I try to rework this suggestions in, and send it to the
> ML as a v2 ... if I do not find time, I speak up ;-)

Sounds good.

Regards,
Simon

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

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Simon Glass]

Hi Wolfgang,

On Thu, Jan 19, 2012 at 3:20 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ1RoyguaAScOQiw8PoQkzVp5rim7KJ6zzHLNPwuBYK48Q@mail.gmail.com> you wrote:
>>
>> Perhaps reword this slightly. U-Boot can have knowledge of a current
>> adaptor, mux settings and so on, and use this internally within the
>> i2c layer to optimise performance and redundant i2c traffic. But the
>> pain is when the concept of a 'current adaptor' is exposed outside the
>> i2c layer.
>
> As mentioned before, this is what we currently have as "device model"
> in U-Boot - not only I2C: we have the same "current device" concept
> with IDE, USB, ...
>
> It makes no sense trying to change it here; it would only cause even
> more incompatibilities. ?If you want to fix that, then help working on
> the new driver model. ?Marek is planning for this.

Yes of course, sorry for not replying earlier, but I completely agree.
Heiko's patch here fits with the "current device" model. I was talking
about the way I hope it will be in the future. I can help with new
driver model also but I would like to get the rationalisation of code
duplication (relocation, board init) to bed first.

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Good morning. This is the telephone company. Due ?to ?repairs, ?we're
> giving ?you ?advance notice that your service will be cut off indefi-
> nitely at ten o'clock. That's two minutes from now.

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Timur Tabi]

Wolfgang Denk wrote:
> As mentioned before, this is what we currently have as "device model"
> in U-Boot - not only I2C: we have the same "current device" concept
> with IDE, USB, ...

The difference is that I2C operations are typically done internally by
other code, whereas IDE, USB, etc are done by the user on the command
line.  It's not unusual for boot code to access multiple I2C devices on
different buses, so we're switching I2C buses a lot.  People generally
don't try to access two networks or two USB devices back-to-back, but
that's exactly what we do with I2C.

The other problem is that I2C operations are necessary prior to
relocation, but IDE, USB, etc generally are not.  That's why we have this:

static unsigned int i2c_bus_num __attribute__ ((section (".data"))) =
CONFIG_SYS_SPD_BUS_NUM;

We need to initialize i2c_bus_num to the I2C bus that SPD is on, because
i2c_bus_num is not writable until after relocation, and DDR initialization
requires I2C.

A board that has SPD on two different I2C buses could not be supported by
U-Boot today.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 1/3] i2c: add i2c_core and prepare for new multibus support

[Heiko Schocher]

Hello Timur,

Timur Tabi wrote:
> Wolfgang Denk wrote:
>> As mentioned before, this is what we currently have as "device model"
>> in U-Boot - not only I2C: we have the same "current device" concept
>> with IDE, USB, ...
> 
> The difference is that I2C operations are typically done internally by
> other code, whereas IDE, USB, etc are done by the user on the command
> line.  It's not unusual for boot code to access multiple I2C devices on
> different buses, so we're switching I2C buses a lot.  People generally
> don't try to access two networks or two USB devices back-to-back, but
> that's exactly what we do with I2C.
>
> The other problem is that I2C operations are necessary prior to
> relocation, but IDE, USB, etc generally are not.  That's why we have this:
> 
> static unsigned int i2c_bus_num __attribute__ ((section (".data"))) =
> CONFIG_SYS_SPD_BUS_NUM;

Yep, correct. For that case, I need the "trickery" with the "cur_i2c_adap"
pointer in gd_t in the "new" framework, as gd_t is writeable before
relocation ...

> We need to initialize i2c_bus_num to the I2C bus that SPD is on, because
> i2c_bus_num is not writable until after relocation, and DDR initialization
> requires I2C.
> 
> A board that has SPD on two different I2C buses could not be supported by
> U-Boot today.

... and so with the "new" framework, it should be possible to switch to
other i2c busses (incl. i2c muxes on that), before relocation.

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

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Heiko Schocher]

rebased/reworked the I2C multibus patches from Simon Glass found
here:

http://www.mail-archive.com/u-boot at lists.denx.de/msg75530.html

It seems the timing is coming, to bring this in mainline and
move boards over to the new i2c framework. As an example I
converted the soft-i2c driver (and all boards using it) to
the new framework, so this patchseries has to be tested
intensively, as I can check compile only ...

Cc: Simon Glass <sjg@chromium.org>
Cc: Piotr Wilczek <p.wilczek@samsung.com>

Heiko Schocher (3):
  i2c: add i2c_core and prepare for new multibus support
  i2c: common changes for multibus/multiadapter support
  i2c, soft-i2c: switch to new multibus/multiadapter support

 README                                    |   89 +++++++-
 arch/arm/include/asm/global_data.h        |    3 +
 arch/arm/lib/board.c                      |   15 +-
 arch/avr32/include/asm/global_data.h      |    3 +
 arch/blackfin/include/asm/global_data.h   |    4 +-
 arch/blackfin/lib/board.c                 |    7 +
 arch/m68k/include/asm/global_data.h       |    3 +
 arch/m68k/lib/board.c                     |   15 +-
 arch/microblaze/include/asm/global_data.h |    3 +
 arch/mips/include/asm/global_data.h       |    3 +
 arch/mips/lib/board.c                     |    7 +
 arch/nds32/include/asm/global_data.h      |    3 +
 arch/nds32/lib/board.c                    |   10 +-
 arch/nios2/include/asm/global_data.h      |    3 +
 arch/powerpc/cpu/mpc8xx/video.c           |    4 +
 arch/powerpc/include/asm/global_data.h    |    3 +
 arch/powerpc/lib/board.c                  |   16 +-
 arch/sh/include/asm/global_data.h         |    3 +
 arch/sparc/include/asm/global_data.h      |    3 +
 arch/x86/include/asm/global_data.h        |    3 +
 board/BuS/eb_cpux9k2/cpux9k2.c            |    2 +-
 board/BuS/vl_ma2sc/vl_ma2sc.c             |    2 +-
 board/atc/atc.c                           |    2 +-
 board/bluewater/snapper9260/snapper9260.c |    2 +-
 board/cm5200/cm5200.c                     |    4 +-
 board/cpu86/cpu86.c                       |    2 +-
 board/cpu87/cpu87.c                       |    2 +-
 board/emk/top9000/top9000.c               |    8 +-
 board/eukrea/cpuat91/cpuat91.c            |    2 +-
 board/freescale/m52277evb/README          |    2 +-
 board/freescale/m53017evb/README          |    2 +-
 board/freescale/m5373evb/README           |    2 +-
 board/freescale/m54455evb/README          |    2 +-
 board/freescale/m547xevb/README           |    2 +-
 board/ids8247/ids8247.c                   |    2 +-
 board/keymile/common/common.c             |    3 +-
 board/keymile/common/ivm.c                |   15 +-
 board/keymile/km82xx/km82xx.c             |    2 +-
 board/keymile/km_arm/km_arm.c             |    8 +-
 board/lwmon/lwmon.c                       |    2 +-
 board/lwmon/pcmcia.c                      |    4 +-
 board/pm826/pm826.c                       |    2 +-
 board/pm828/pm828.c                       |    2 +-
 board/sacsng/ioconfig.h                   |    2 +-
 board/sandburst/karef/karef.c             |    1 -
 board/siemens/SCM/scm.c                   |    2 +-
 board/tqc/tqm8260/tqm8260.c               |    2 +-
 board/tqc/tqm8272/tqm8272.c               |    2 +-
 board/tqc/tqm8272/tqm8272.h               |    2 +-
 common/cmd_date.c                         |    9 +
 common/cmd_dtt.c                          |    9 +
 common/cmd_eeprom.c                       |    3 +-
 common/cmd_i2c.c                          |  118 ++++++---
 common/env_eeprom.c                       |   14 +
 common/stdio.c                            |   14 +-
 drivers/i2c/Makefile                      |    3 +-
 drivers/i2c/i2c_core.c                    |  367 +++++++++++++++++++++++++++++
 drivers/i2c/soft_i2c.c                    |  122 ++++++----
 include/configs/A3000.h                   |    4 +-
 include/configs/BSC9131RDB.h              |    1 -
 include/configs/CANBT.h                   |    4 +
 include/configs/CPU86.h                   |    9 +-
 include/configs/CPU87.h                   |    9 +-
 include/configs/DU440.h                   |    1 -
 include/configs/GEN860T.h                 |   36 ++--
 include/configs/HIDDEN_DRAGON.h           |   10 +-
 include/configs/IAD210.h                  |   12 +-
 include/configs/ICU862.h                  |   14 +-
 include/configs/IDS8247.h                 |   10 +-
 include/configs/IP860.h                   |   10 +-
 include/configs/IPHASE4539.h              |   12 +-
 include/configs/JSE.h                     |    1 -
 include/configs/KAREF.h                   |    1 -
 include/configs/KUP4K.h                   |   12 +-
 include/configs/KUP4X.h                   |   14 +-
 include/configs/M5208EVBE.h               |    1 -
 include/configs/M52277EVB.h               |    1 -
 include/configs/M5235EVB.h                |    1 -
 include/configs/M5271EVB.h                |    1 -
 include/configs/M5275EVB.h                |    1 -
 include/configs/M53017EVB.h               |    1 -
 include/configs/M5329EVB.h                |    1 -
 include/configs/M5373EVB.h                |    1 -
 include/configs/M54451EVB.h               |    1 -
 include/configs/M54455EVB.h               |    1 -
 include/configs/M5475EVB.h                |    1 -
 include/configs/M5485EVB.h                |    1 -
 include/configs/METROBOX.h                |    1 -
 include/configs/MHPC.h                    |    9 +-
 include/configs/MPC8323ERDB.h             |    1 -
 include/configs/MPC832XEMDS.h             |    1 -
 include/configs/MPC8349EMDS.h             |    1 -
 include/configs/MPC8349ITX.h              |    2 -
 include/configs/MPC8360EMDS.h             |    1 -
 include/configs/MPC8360ERDK.h             |    1 -
 include/configs/MPC837XEMDS.h             |    1 -
 include/configs/MPC837XERDB.h             |    1 -
 include/configs/MPC8536DS.h               |    1 -
 include/configs/MPC8540ADS.h              |    1 -
 include/configs/MPC8541CDS.h              |    1 -
 include/configs/MPC8544DS.h               |    1 -
 include/configs/MPC8548CDS.h              |    3 +-
 include/configs/MPC8555CDS.h              |    1 -
 include/configs/MPC8560ADS.h              |    1 -
 include/configs/MPC8568MDS.h              |    1 -
 include/configs/MPC8569MDS.h              |    1 -
 include/configs/MPC8572DS.h               |    1 -
 include/configs/MPC8610HPCD.h             |    1 -
 include/configs/MPC8641HPCN.h             |    1 -
 include/configs/P1010RDB.h                |    1 -
 include/configs/P1023RDS.h                |    1 -
 include/configs/P1_P2_RDB.h               |    1 -
 include/configs/P2020COME.h               |    1 -
 include/configs/PM826.h                   |    9 +-
 include/configs/PM828.h                   |    9 +-
 include/configs/PMC440.h                  |    1 -
 include/configs/R360MPI.h                 |    7 +-
 include/configs/RPXClassic.h              |   16 +-
 include/configs/RPXlite.h                 |   26 ++
 include/configs/RRvision.h                |   13 +-
 include/configs/SBC8540.h                 |    1 -
 include/configs/SCM.h                     |   11 +-
 include/configs/SXNI855T.h                |    8 +-
 include/configs/Sandpoint8240.h           |   10 +-
 include/configs/Sandpoint8245.h           |   14 +-
 include/configs/TASREG.h                  |   27 +-
 include/configs/TK885D.h                  |   13 +-
 include/configs/TOP5200.h                 |   15 +-
 include/configs/TOP860.h                  |   11 +-
 include/configs/TQM8260.h                 |    9 +-
 include/configs/TQM8272.h                 |   13 +-
 include/configs/TQM834x.h                 |    1 -
 include/configs/TQM855M.h                 |   13 +-
 include/configs/TQM866M.h                 |   13 +-
 include/configs/TQM885D.h                 |   13 +-
 include/configs/alpr.h                    |    1 -
 include/configs/aria.h                    |    1 -
 include/configs/astro_mcf5373l.h          |    1 -
 include/configs/bf533-ezkit.h             |   10 +-
 include/configs/bf533-stamp.h             |   47 ++++-
 include/configs/bf561-ezkit.h             |    9 +-
 include/configs/bfin_adi_common.h         |    2 +-
 include/configs/blackstamp.h              |    1 -
 include/configs/cpuat91.h                 |    1 -
 include/configs/debris.h                  |   10 +-
 include/configs/eXalion.h                 |    2 +-
 include/configs/eb_cpux9k2.h              |   14 +-
 include/configs/ep8260.h                  |   14 +-
 include/configs/ethernut5.h               |    9 +-
 include/configs/ibf-dsp561.h              |    5 +-
 include/configs/iocon.h                   |   11 +-
 include/configs/km/keymile-common.h       |    3 -
 include/configs/km/km83xx-common.h        |    3 +
 include/configs/km/km_arm.h               |   26 ++-
 include/configs/km82xx.h                  |   18 +-
 include/configs/km_kirkwood.h             |   14 +-
 include/configs/korat.h                   |    1 -
 include/configs/lwmon.h                   |   13 +-
 include/configs/lwmon5.h                  |    1 -
 include/configs/mecp5123.h                |    1 -
 include/configs/mpc5121ads.h              |    1 -
 include/configs/nhk8815.h                 |    8 +-
 include/configs/otc570.h                  |   18 +-
 include/configs/p1_p2_rdb_pc.h            |    1 -
 include/configs/p3p440.h                  |    1 -
 include/configs/pcs440ep.h                |    1 -
 include/configs/pdnb3.h                   |   11 +-
 include/configs/quad100hd.h               |    1 -
 include/configs/s5p_goni.h                |    7 +-
 include/configs/s5pc210_universal.h       |    7 +-
 include/configs/sacsng.h                  |   12 +-
 include/configs/sbc405.h                  |    1 -
 include/configs/sbc8349.h                 |    1 -
 include/configs/sbc8548.h                 |    1 -
 include/configs/sbc8560.h                 |    1 -
 include/configs/sbc8641d.h                |    1 -
 include/configs/sc3.h                     |    1 -
 include/configs/snapper9260.h             |    9 +-
 include/configs/socrates.h                |    1 -
 include/configs/spc1920.h                 |   13 +-
 include/configs/stxgp3.h                  |    1 -
 include/configs/stxssa.h                  |    1 -
 include/configs/top9000.h                 |   10 +-
 include/configs/trats.h                   |    7 +-
 include/configs/u8500_href.h              |    1 -
 include/configs/uc100.h                   |   13 +-
 include/configs/utx8245.h                 |    3 +-
 include/configs/vct.h                     |   12 +-
 include/configs/vl_ma2sc.h                |    7 +-
 include/configs/vme8349.h                 |    1 -
 include/configs/zeus.h                    |    1 -
 include/i2c.h                             |  196 +++++++++++++++-
 192 files changed, 1443 insertions(+), 533 deletions(-)
 create mode 100644 drivers/i2c/i2c_core.c

-- 
1.7.7.6

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Simon Glass]

Hi Heiko,

On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher <hs@denx.de> wrote:
> rebased/reworked the I2C multibus patches from Simon Glass found
> here:
>
> http://www.mail-archive.com/u-boot at lists.denx.de/msg75530.html
>
> It seems the timing is coming, to bring this in mainline and
> move boards over to the new i2c framework. As an example I
> converted the soft-i2c driver (and all boards using it) to
> the new framework, so this patchseries has to be tested
> intensively, as I can check compile only ...

I am very happy to see this, thank you.

I have brought this in and tried to get it running for Tegra. A few points:

1. The methods in struct i2c_adapter should IMO be passed a struct
i2c_adapter *, so they can determine which adapter is being accessed.
Otherwise I can't see how they would know.

2. The init is a bit odd, in that we can call init() repeatedly.
Perhaps that function should be renamed to reset, and the init should
be used for calling just once at the start?

3. Rather than each device having to call i2c_get_bus_num() to find
out what bus is referred to, why not just pass this information in? In
fact the adapter pointer can serve for this.

4. So perhaps the i2c read/write functions should change from:

int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)

to:

int i2c_read((struct i2c_adapter *adapter, uint addr, int alen, uchar
*buffer, int len)

Later, I wonder whether the concept of a 'current' i2c bus should be
maintained by the command line interpreter, rather than the i2c
system. Because to be honest, most of the drivers I see have to save
the current bus number, set the current bus, do the operation, then
set the bus back how they found it (to preserve whatever the user
things is the current bus).

Granted there is overhead with i2c muxes, but the i2c core can
remember the state of these muxes and doesn't have to switch things
until there has been a change since the last transaction.

This last suggestion can be dealt with later, but I thought I would bring it up.

Regards,
Simon

>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Piotr Wilczek <p.wilczek@samsung.com>
>
> Heiko Schocher (3):
>   i2c: add i2c_core and prepare for new multibus support
>   i2c: common changes for multibus/multiadapter support
>   i2c, soft-i2c: switch to new multibus/multiadapter support
>
>  README                                    |   89 +++++++-
>  arch/arm/include/asm/global_data.h        |    3 +
>  arch/arm/lib/board.c                      |   15 +-
>  arch/avr32/include/asm/global_data.h      |    3 +
>  arch/blackfin/include/asm/global_data.h   |    4 +-
>  arch/blackfin/lib/board.c                 |    7 +
>  arch/m68k/include/asm/global_data.h       |    3 +
>  arch/m68k/lib/board.c                     |   15 +-
>  arch/microblaze/include/asm/global_data.h |    3 +
>  arch/mips/include/asm/global_data.h       |    3 +
>  arch/mips/lib/board.c                     |    7 +
>  arch/nds32/include/asm/global_data.h      |    3 +
>  arch/nds32/lib/board.c                    |   10 +-
>  arch/nios2/include/asm/global_data.h      |    3 +
>  arch/powerpc/cpu/mpc8xx/video.c           |    4 +
>  arch/powerpc/include/asm/global_data.h    |    3 +
>  arch/powerpc/lib/board.c                  |   16 +-
>  arch/sh/include/asm/global_data.h         |    3 +
>  arch/sparc/include/asm/global_data.h      |    3 +
>  arch/x86/include/asm/global_data.h        |    3 +
>  board/BuS/eb_cpux9k2/cpux9k2.c            |    2 +-
>  board/BuS/vl_ma2sc/vl_ma2sc.c             |    2 +-
>  board/atc/atc.c                           |    2 +-
>  board/bluewater/snapper9260/snapper9260.c |    2 +-
>  board/cm5200/cm5200.c                     |    4 +-
>  board/cpu86/cpu86.c                       |    2 +-
>  board/cpu87/cpu87.c                       |    2 +-
>  board/emk/top9000/top9000.c               |    8 +-
>  board/eukrea/cpuat91/cpuat91.c            |    2 +-
>  board/freescale/m52277evb/README          |    2 +-
>  board/freescale/m53017evb/README          |    2 +-
>  board/freescale/m5373evb/README           |    2 +-
>  board/freescale/m54455evb/README          |    2 +-
>  board/freescale/m547xevb/README           |    2 +-
>  board/ids8247/ids8247.c                   |    2 +-
>  board/keymile/common/common.c             |    3 +-
>  board/keymile/common/ivm.c                |   15 +-
>  board/keymile/km82xx/km82xx.c             |    2 +-
>  board/keymile/km_arm/km_arm.c             |    8 +-
>  board/lwmon/lwmon.c                       |    2 +-
>  board/lwmon/pcmcia.c                      |    4 +-
>  board/pm826/pm826.c                       |    2 +-
>  board/pm828/pm828.c                       |    2 +-
>  board/sacsng/ioconfig.h                   |    2 +-
>  board/sandburst/karef/karef.c             |    1 -
>  board/siemens/SCM/scm.c                   |    2 +-
>  board/tqc/tqm8260/tqm8260.c               |    2 +-
>  board/tqc/tqm8272/tqm8272.c               |    2 +-
>  board/tqc/tqm8272/tqm8272.h               |    2 +-
>  common/cmd_date.c                         |    9 +
>  common/cmd_dtt.c                          |    9 +
>  common/cmd_eeprom.c                       |    3 +-
>  common/cmd_i2c.c                          |  118 ++++++---
>  common/env_eeprom.c                       |   14 +
>  common/stdio.c                            |   14 +-
>  drivers/i2c/Makefile                      |    3 +-
>  drivers/i2c/i2c_core.c                    |  367 +++++++++++++++++++++++++++++
>  drivers/i2c/soft_i2c.c                    |  122 ++++++----
>  include/configs/A3000.h                   |    4 +-
>  include/configs/BSC9131RDB.h              |    1 -
>  include/configs/CANBT.h                   |    4 +
>  include/configs/CPU86.h                   |    9 +-
>  include/configs/CPU87.h                   |    9 +-
>  include/configs/DU440.h                   |    1 -
>  include/configs/GEN860T.h                 |   36 ++--
>  include/configs/HIDDEN_DRAGON.h           |   10 +-
>  include/configs/IAD210.h                  |   12 +-
>  include/configs/ICU862.h                  |   14 +-
>  include/configs/IDS8247.h                 |   10 +-
>  include/configs/IP860.h                   |   10 +-
>  include/configs/IPHASE4539.h              |   12 +-
>  include/configs/JSE.h                     |    1 -
>  include/configs/KAREF.h                   |    1 -
>  include/configs/KUP4K.h                   |   12 +-
>  include/configs/KUP4X.h                   |   14 +-
>  include/configs/M5208EVBE.h               |    1 -
>  include/configs/M52277EVB.h               |    1 -
>  include/configs/M5235EVB.h                |    1 -
>  include/configs/M5271EVB.h                |    1 -
>  include/configs/M5275EVB.h                |    1 -
>  include/configs/M53017EVB.h               |    1 -
>  include/configs/M5329EVB.h                |    1 -
>  include/configs/M5373EVB.h                |    1 -
>  include/configs/M54451EVB.h               |    1 -
>  include/configs/M54455EVB.h               |    1 -
>  include/configs/M5475EVB.h                |    1 -
>  include/configs/M5485EVB.h                |    1 -
>  include/configs/METROBOX.h                |    1 -
>  include/configs/MHPC.h                    |    9 +-
>  include/configs/MPC8323ERDB.h             |    1 -
>  include/configs/MPC832XEMDS.h             |    1 -
>  include/configs/MPC8349EMDS.h             |    1 -
>  include/configs/MPC8349ITX.h              |    2 -
>  include/configs/MPC8360EMDS.h             |    1 -
>  include/configs/MPC8360ERDK.h             |    1 -
>  include/configs/MPC837XEMDS.h             |    1 -
>  include/configs/MPC837XERDB.h             |    1 -
>  include/configs/MPC8536DS.h               |    1 -
>  include/configs/MPC8540ADS.h              |    1 -
>  include/configs/MPC8541CDS.h              |    1 -
>  include/configs/MPC8544DS.h               |    1 -
>  include/configs/MPC8548CDS.h              |    3 +-
>  include/configs/MPC8555CDS.h              |    1 -
>  include/configs/MPC8560ADS.h              |    1 -
>  include/configs/MPC8568MDS.h              |    1 -
>  include/configs/MPC8569MDS.h              |    1 -
>  include/configs/MPC8572DS.h               |    1 -
>  include/configs/MPC8610HPCD.h             |    1 -
>  include/configs/MPC8641HPCN.h             |    1 -
>  include/configs/P1010RDB.h                |    1 -
>  include/configs/P1023RDS.h                |    1 -
>  include/configs/P1_P2_RDB.h               |    1 -
>  include/configs/P2020COME.h               |    1 -
>  include/configs/PM826.h                   |    9 +-
>  include/configs/PM828.h                   |    9 +-
>  include/configs/PMC440.h                  |    1 -
>  include/configs/R360MPI.h                 |    7 +-
>  include/configs/RPXClassic.h              |   16 +-
>  include/configs/RPXlite.h                 |   26 ++
>  include/configs/RRvision.h                |   13 +-
>  include/configs/SBC8540.h                 |    1 -
>  include/configs/SCM.h                     |   11 +-
>  include/configs/SXNI855T.h                |    8 +-
>  include/configs/Sandpoint8240.h           |   10 +-
>  include/configs/Sandpoint8245.h           |   14 +-
>  include/configs/TASREG.h                  |   27 +-
>  include/configs/TK885D.h                  |   13 +-
>  include/configs/TOP5200.h                 |   15 +-
>  include/configs/TOP860.h                  |   11 +-
>  include/configs/TQM8260.h                 |    9 +-
>  include/configs/TQM8272.h                 |   13 +-
>  include/configs/TQM834x.h                 |    1 -
>  include/configs/TQM855M.h                 |   13 +-
>  include/configs/TQM866M.h                 |   13 +-
>  include/configs/TQM885D.h                 |   13 +-
>  include/configs/alpr.h                    |    1 -
>  include/configs/aria.h                    |    1 -
>  include/configs/astro_mcf5373l.h          |    1 -
>  include/configs/bf533-ezkit.h             |   10 +-
>  include/configs/bf533-stamp.h             |   47 ++++-
>  include/configs/bf561-ezkit.h             |    9 +-
>  include/configs/bfin_adi_common.h         |    2 +-
>  include/configs/blackstamp.h              |    1 -
>  include/configs/cpuat91.h                 |    1 -
>  include/configs/debris.h                  |   10 +-
>  include/configs/eXalion.h                 |    2 +-
>  include/configs/eb_cpux9k2.h              |   14 +-
>  include/configs/ep8260.h                  |   14 +-
>  include/configs/ethernut5.h               |    9 +-
>  include/configs/ibf-dsp561.h              |    5 +-
>  include/configs/iocon.h                   |   11 +-
>  include/configs/km/keymile-common.h       |    3 -
>  include/configs/km/km83xx-common.h        |    3 +
>  include/configs/km/km_arm.h               |   26 ++-
>  include/configs/km82xx.h                  |   18 +-
>  include/configs/km_kirkwood.h             |   14 +-
>  include/configs/korat.h                   |    1 -
>  include/configs/lwmon.h                   |   13 +-
>  include/configs/lwmon5.h                  |    1 -
>  include/configs/mecp5123.h                |    1 -
>  include/configs/mpc5121ads.h              |    1 -
>  include/configs/nhk8815.h                 |    8 +-
>  include/configs/otc570.h                  |   18 +-
>  include/configs/p1_p2_rdb_pc.h            |    1 -
>  include/configs/p3p440.h                  |    1 -
>  include/configs/pcs440ep.h                |    1 -
>  include/configs/pdnb3.h                   |   11 +-
>  include/configs/quad100hd.h               |    1 -
>  include/configs/s5p_goni.h                |    7 +-
>  include/configs/s5pc210_universal.h       |    7 +-
>  include/configs/sacsng.h                  |   12 +-
>  include/configs/sbc405.h                  |    1 -
>  include/configs/sbc8349.h                 |    1 -
>  include/configs/sbc8548.h                 |    1 -
>  include/configs/sbc8560.h                 |    1 -
>  include/configs/sbc8641d.h                |    1 -
>  include/configs/sc3.h                     |    1 -
>  include/configs/snapper9260.h             |    9 +-
>  include/configs/socrates.h                |    1 -
>  include/configs/spc1920.h                 |   13 +-
>  include/configs/stxgp3.h                  |    1 -
>  include/configs/stxssa.h                  |    1 -
>  include/configs/top9000.h                 |   10 +-
>  include/configs/trats.h                   |    7 +-
>  include/configs/u8500_href.h              |    1 -
>  include/configs/uc100.h                   |   13 +-
>  include/configs/utx8245.h                 |    3 +-
>  include/configs/vct.h                     |   12 +-
>  include/configs/vl_ma2sc.h                |    7 +-
>  include/configs/vme8349.h                 |    1 -
>  include/configs/zeus.h                    |    1 -
>  include/i2c.h                             |  196 +++++++++++++++-
>  192 files changed, 1443 insertions(+), 533 deletions(-)
>  create mode 100644 drivers/i2c/i2c_core.c
>
> --
> 1.7.7.6
>

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Heiko Schocher]

Hello Simon,

On 25.10.2012 23:37, Simon Glass wrote:
> On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher<hs@denx.de>  wrote:
>> rebased/reworked the I2C multibus patches from Simon Glass found
>> here:
>>
>> http://www.mail-archive.com/u-boot at lists.denx.de/msg75530.html
>>
>> It seems the timing is coming, to bring this in mainline and
>> move boards over to the new i2c framework. As an example I
>> converted the soft-i2c driver (and all boards using it) to
>> the new framework, so this patchseries has to be tested
>> intensively, as I can check compile only ...
>
> I am very happy to see this, thank you.

I am too ;-) and Sorry that I am only now ready ...

> I have brought this in and tried to get it running for Tegra. A few points:
>
> 1. The methods in struct i2c_adapter should IMO be passed a struct
> i2c_adapter *, so they can determine which adapter is being accessed.
> Otherwise I can't see how they would know.

They can get the current used adapter through the defines in
include/i2c.h:
[...]
#define I2C_ADAP_NR(bus)        i2c_adap[i2c_bus[bus].adapter]
#define I2C_BUS                 ((struct i2c_bus_hose *)gd->cur_i2c_bus)
#define I2C_ADAP                i2c_adap[I2C_BUS->adapter]
#define I2C_ADAP_HWNR           (I2C_ADAP->hwadapnr)

preparing just the fsl i2c driver and there I do for example:
drivers/i2c/fsl_i2c.c
[...]
static const struct fsl_i2c *i2c_dev[2] = {
         (struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C_OFFSET),
#ifdef CONFIG_SYS_FSL_I2C2_OFFSET
         (struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C2_OFFSET)
#endif
};
[...]
static int fsl_i2c_probe(uchar chip)
{
         struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
[...]

but of course, we still can change the "struct i2c_adapter" if
needed ... but we have one more parameter ... Ok, not really a bad
problem.

> 2. The init is a bit odd, in that we can call init() repeatedly.
> Perhaps that function should be renamed to reset, and the init should
> be used for calling just once at the start?

What do you mean here exactly? I couldn?t parse this ...

> 3. Rather than each device having to call i2c_get_bus_num() to find
> out what bus is referred to, why not just pass this information in? In
> fact the adapter pointer can serve for this.

Not the "struct i2c_adapter" must passed, but the "struct
i2c_bus"!

And each device should know, which i2c bus it uses, or? So at
the end we should have something like i2c_read(struct i2c_bus *bus, ...)
calls ... and the i2c core can detect, if this bus is the
current, if so go on, if not switch to this bus. So at the
end i2c_set_bus_num would be go static ...

> 4. So perhaps the i2c read/write functions should change from:
>
> int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
>
> to:
>
> int i2c_read((struct i2c_adapter *adapter, uint addr, int alen, uchar
> *buffer, int len)

Yep, exactly, see comments to point 3 ...

That would be the best (and I think in previous discussions I defined
this as one goal ...), but this would be (another) big change,
because this is an *API* change, with maybe a lot of config file
changes ...

Let us bring in the new i2c framework with all i2c drivers converted,
and then do the next step ... maybe one step more, if mareks device
model is ready, we can switch easy to DM ... and maybe get this API
change for free ...

> Later, I wonder whether the concept of a 'current' i2c bus should be
> maintained by the command line interpreter, rather than the i2c
> system. Because to be honest, most of the drivers I see have to save
> the current bus number, set the current bus, do the operation, then
> set the bus back how they found it (to preserve whatever the user
> things is the current bus).

Yes, suboptimal ... but this is independent from the new i2c framework
patches!

It is possible (with old/new i2c bus framework) to introduce a
"current commandline i2c bus", and then, before calling i2c_read/write
from the commandline, call a i2c_set_bus_num() ... then we can get rid
of this store/restore current i2c bus ... waiting for patches ;-)

> Granted there is overhead with i2c muxes, but the i2c core can
> remember the state of these muxes and doesn't have to switch things
> until there has been a change since the last transaction.

This exactly do the i2c_set_bus_num() now!

> This last suggestion can be dealt with later, but I thought I would bring it up.

I am happy about every comment! :-)

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

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Stephen Warren]

On 10/25/2012 11:48 PM, Heiko Schocher wrote:
> Hello Simon,
> 
> On 25.10.2012 23:37, Simon Glass wrote:
>> On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher<hs@denx.de>  wrote:
>>> rebased/reworked the I2C multibus patches from Simon Glass found
>>> here:
>>>
>>> http://www.mail-archive.com/u-boot at lists.denx.de/msg75530.html
>>>
>>> It seems the timing is coming, to bring this in mainline and
>>> move boards over to the new i2c framework. As an example I
>>> converted the soft-i2c driver (and all boards using it) to
>>> the new framework, so this patchseries has to be tested
>>> intensively, as I can check compile only ...
>>
>> I am very happy to see this, thank you.
> 
> I am too ;-) and Sorry that I am only now ready ...
> 
>> I have brought this in and tried to get it running for Tegra. A few
>> points:
>>
>> 1. The methods in struct i2c_adapter should IMO be passed a struct
>> i2c_adapter *, so they can determine which adapter is being accessed.
>> Otherwise I can't see how they would know.
> 
> They can get the current used adapter through the defines in
> include/i2c.h:
> [...]
> #define I2C_ADAP_NR(bus)        i2c_adap[i2c_bus[bus].adapter]
> #define I2C_BUS                 ((struct i2c_bus_hose *)gd->cur_i2c_bus)
> #define I2C_ADAP                i2c_adap[I2C_BUS->adapter]
> #define I2C_ADAP_HWNR           (I2C_ADAP->hwadapnr)
> 
> preparing just the fsl i2c driver and there I do for example:
> drivers/i2c/fsl_i2c.c
> [...]
> static const struct fsl_i2c *i2c_dev[2] = {
>         (struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C_OFFSET),
> #ifdef CONFIG_SYS_FSL_I2C2_OFFSET
>         (struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C2_OFFSET)
> #endif
> };
> [...]
> static int fsl_i2c_probe(uchar chip)
> {
>         struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
> [...]
> 
> but of course, we still can change the "struct i2c_adapter" if
> needed ... but we have one more parameter ... Ok, not really a bad
> problem.

That rather relies on their being a concept of a "current" I2C adapter.
It seems a little limiting to require that. What if the "current"
adapter is the user-selected adapter for commands to operate on, but
e.g. some power-management driver wants to use I2C to communicate with a
PMIC during the internals of some other command. Sure, you could save
and later restore the I2C core's idea of "current" adapter, but it'd
surely be cleaner to just pass around the I2C adapter ID or struct
pointer everywhere to avoid the need for save/restore.

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Simon Glass]

Hi Heiko,

On Thu, Oct 25, 2012 at 10:48 PM, Heiko Schocher <hs@denx.de> wrote:
> Hello Simon,
>
>
> On 25.10.2012 23:37, Simon Glass wrote:
>>
>> On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher<hs@denx.de>  wrote:
>>>
>>> rebased/reworked the I2C multibus patches from Simon Glass found
>>> here:
>>>
>>> http://www.mail-archive.com/u-boot at lists.denx.de/msg75530.html
>>>
>>> It seems the timing is coming, to bring this in mainline and
>>> move boards over to the new i2c framework. As an example I
>>> converted the soft-i2c driver (and all boards using it) to
>>> the new framework, so this patchseries has to be tested
>>> intensively, as I can check compile only ...
>>
>>
>> I am very happy to see this, thank you.
>
>
> I am too ;-) and Sorry that I am only now ready ...
>
>
>> I have brought this in and tried to get it running for Tegra. A few
>> points:
>>
>> 1. The methods in struct i2c_adapter should IMO be passed a struct
>> i2c_adapter *, so they can determine which adapter is being accessed.
>> Otherwise I can't see how they would know.
>
>
> They can get the current used adapter through the defines in
> include/i2c.h:
> [...]
> #define I2C_ADAP_NR(bus)        i2c_adap[i2c_bus[bus].adapter]
> #define I2C_BUS                 ((struct i2c_bus_hose *)gd->cur_i2c_bus)
> #define I2C_ADAP                i2c_adap[I2C_BUS->adapter]
> #define I2C_ADAP_HWNR           (I2C_ADAP->hwadapnr)
>
> preparing just the fsl i2c driver and there I do for example:
> drivers/i2c/fsl_i2c.c
> [...]
> static const struct fsl_i2c *i2c_dev[2] = {
>         (struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C_OFFSET),
> #ifdef CONFIG_SYS_FSL_I2C2_OFFSET
>         (struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C2_OFFSET)
> #endif
> };
> [...]
> static int fsl_i2c_probe(uchar chip)
> {
>         struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
> [...]
>
> but of course, we still can change the "struct i2c_adapter" if
> needed ... but we have one more parameter ... Ok, not really a bad
> problem.
>
>
>> 2. The init is a bit odd, in that we can call init() repeatedly.
>> Perhaps that function should be renamed to reset, and the init should
>> be used for calling just once at the start?
>
>
> What do you mean here exactly? I couldn?t parse this ...

Well there is start-of-day setup, which I think should be called init.
This is done once on boot for each i2c adapter.

And then there is the i2c_init() which seems to be called whenever we
feel like it - e.g. to change speed. I suggest that we use init() to
mean start-of-day init and reset(), or similar, to mean any post-init.
I am not suggest that for this series, just as a future effort.

>
>
>> 3. Rather than each device having to call i2c_get_bus_num() to find
>> out what bus is referred to, why not just pass this information in? In
>> fact the adapter pointer can serve for this.
>
>
> Not the "struct i2c_adapter" must passed, but the "struct
> i2c_bus"!
>
> And each device should know, which i2c bus it uses, or? So at
> the end we should have something like i2c_read(struct i2c_bus *bus, ...)
> calls ... and the i2c core can detect, if this bus is the
> current, if so go on, if not switch to this bus. So at the
> end i2c_set_bus_num would be go static ...
>
>
>> 4. So perhaps the i2c read/write functions should change from:
>>
>> int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
>>
>> to:
>>
>> int i2c_read((struct i2c_adapter *adapter, uint addr, int alen, uchar
>> *buffer, int len)
>
>
> Yep, exactly, see comments to point 3 ...
>
> That would be the best (and I think in previous discussions I defined
> this as one goal ...), but this would be (another) big change,
> because this is an *API* change, with maybe a lot of config file
> changes ...
>
> Let us bring in the new i2c framework with all i2c drivers converted,
> and then do the next step ... maybe one step more, if mareks device
> model is ready, we can switch easy to DM ... and maybe get this API
> change for free ...

Yes. I certainly understand the need to fit in with what is already
there, and avoid a massive API change, which can be performed as a
follow-on patch anyway. With your info above I will adjust the tegra
driver to work with this and test it.

>
>
>> Later, I wonder whether the concept of a 'current' i2c bus should be
>> maintained by the command line interpreter, rather than the i2c
>> system. Because to be honest, most of the drivers I see have to save
>> the current bus number, set the current bus, do the operation, then
>> set the bus back how they found it (to preserve whatever the user
>> things is the current bus).
>
>
> Yes, suboptimal ... but this is independent from the new i2c framework
> patches!
>
> It is possible (with old/new i2c bus framework) to introduce a
> "current commandline i2c bus", and then, before calling i2c_read/write
> from the commandline, call a i2c_set_bus_num() ... then we can get rid
> of this store/restore current i2c bus ... waiting for patches ;-)

OK.

>
>
>> Granted there is overhead with i2c muxes, but the i2c core can
>> remember the state of these muxes and doesn't have to switch things
>> until there has been a change since the last transaction.
>
>
> This exactly do the i2c_set_bus_num() now!

Great.

>
>
>> This last suggestion can be dealt with later, but I thought I would bring
>> it up.
>
>
> I am happy about every comment! :-)

Thanks,
Simon

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

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Tom Rini]

On Thu, Oct 25, 2012 at 02:37:08PM -0700, Simon Glass wrote:

[snip]
> Later, I wonder whether the concept of a 'current' i2c bus should be
> maintained by the command line interpreter, rather than the i2c
> system. Because to be honest, most of the drivers I see have to save
> the current bus number, set the current bus, do the operation, then
> set the bus back how they found it (to preserve whatever the user
> things is the current bus).

I agree.  Lets move the notion of "current" to cmd_i2c and make
everything internally pass around the bus to operate on.  Or try going
down this path and find a fatal problem :)

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121026/f80ee320/attachment.pgp>

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Heiko Schocher]

Hello Simon,

On 26.10.2012 18:08, Simon Glass wrote:
> On Thu, Oct 25, 2012 at 10:48 PM, Heiko Schocher<hs@denx.de>  wrote:
>> Hello Simon,
>>
>>
>> On 25.10.2012 23:37, Simon Glass wrote:
>>>
>>> On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher<hs@denx.de>   wrote:
[...]
>>> 2. The init is a bit odd, in that we can call init() repeatedly.
>>> Perhaps that function should be renamed to reset, and the init should
>>> be used for calling just once at the start?
>>
>>
>> What do you mean here exactly? I couldn?t parse this ...
>
> Well there is start-of-day setup, which I think should be called init.
> This is done once on boot for each i2c adapter.

Hmm... I am not sure if this is only done on boot, because we should
"close" or "deinit" an adapter if not used any more in U-Boot as the
U-Boot principle says:

http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast

So I want to add in future some "deinit" to every adapter, and
call it from i2c_set_bus() when switching to another i2c adapter ...

> And then there is the i2c_init() which seems to be called whenever we
> feel like it - e.g. to change speed. I suggest that we use init() to
> mean start-of-day init and reset(), or similar, to mean any post-init.
> I am not suggest that for this series, just as a future effort.

Yes, that should be changed. We do not need an init() in the i2c
API, as i2c_set_bus_num() do this for us (and later also the deinit())

We just need a set/get_speed() and a deblock()/reset() ?

Maybe a step in the API cleanup?

>>> 3. Rather than each device having to call i2c_get_bus_num() to find
>>> out what bus is referred to, why not just pass this information in? In
>>> fact the adapter pointer can serve for this.
>>
>>
>> Not the "struct i2c_adapter" must passed, but the "struct
>> i2c_bus"!
>>
>> And each device should know, which i2c bus it uses, or? So at
>> the end we should have something like i2c_read(struct i2c_bus *bus, ...)
>> calls ... and the i2c core can detect, if this bus is the
>> current, if so go on, if not switch to this bus. So at the
>> end i2c_set_bus_num would be go static ...
>>
>>
>>> 4. So perhaps the i2c read/write functions should change from:
>>>
>>> int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
>>>
>>> to:
>>>
>>> int i2c_read((struct i2c_adapter *adapter, uint addr, int alen, uchar
>>> *buffer, int len)
>>
>>
>> Yep, exactly, see comments to point 3 ...
>>
>> That would be the best (and I think in previous discussions I defined
>> this as one goal ...), but this would be (another) big change,
>> because this is an *API* change, with maybe a lot of config file
>> changes ...
>>
>> Let us bring in the new i2c framework with all i2c drivers converted,
>> and then do the next step ... maybe one step more, if mareks device
>> model is ready, we can switch easy to DM ... and maybe get this API
>> change for free ...
>
> Yes. I certainly understand the need to fit in with what is already
> there, and avoid a massive API change, which can be performed as a
> follow-on patch anyway. With your info above I will adjust the tegra
> driver to work with this and test it.

Ok, great! So I post v2 patches after you tested ...

And Yes, we should do this API change, but I tend to do this step after
the new i2c framework is stable and all i2c drivers are converted to it ...

>>> Later, I wonder whether the concept of a 'current' i2c bus should be
>>> maintained by the command line interpreter, rather than the i2c
>>> system. Because to be honest, most of the drivers I see have to save
>>> the current bus number, set the current bus, do the operation, then
>>> set the bus back how they found it (to preserve whatever the user
>>> things is the current bus).
>>
>>
>> Yes, suboptimal ... but this is independent from the new i2c framework
>> patches!
>>
>> It is possible (with old/new i2c bus framework) to introduce a
>> "current commandline i2c bus", and then, before calling i2c_read/write
>> from the commandline, call a i2c_set_bus_num() ... then we can get rid
>> of this store/restore current i2c bus ... waiting for patches ;-)
>
> OK.
>
>>
>>
>>> Granted there is overhead with i2c muxes, but the i2c core can
>>> remember the state of these muxes and doesn't have to switch things
>>> until there has been a change since the last transaction.
>>
>>
>> This exactly do the i2c_set_bus_num() now!
>
> Great.
>
>>
>>
>>> This last suggestion can be dealt with later, but I thought I would bring
>>> it up.
>>
>>
>> I am happy about every comment! :-)
>
> Thanks,
> Simon

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

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Heiko Schocher]

Hello Stephen,

On 26.10.2012 18:07, Stephen Warren wrote:
> On 10/25/2012 11:48 PM, Heiko Schocher wrote:
>> Hello Simon,
>>
>> On 25.10.2012 23:37, Simon Glass wrote:
>>> On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher<hs@denx.de>   wrote:
>>>> rebased/reworked the I2C multibus patches from Simon Glass found
>>>> here:
>>>>
>>>> http://www.mail-archive.com/u-boot at lists.denx.de/msg75530.html
>>>>
>>>> It seems the timing is coming, to bring this in mainline and
>>>> move boards over to the new i2c framework. As an example I
>>>> converted the soft-i2c driver (and all boards using it) to
>>>> the new framework, so this patchseries has to be tested
>>>> intensively, as I can check compile only ...
>>>
>>> I am very happy to see this, thank you.
>>
>> I am too ;-) and Sorry that I am only now ready ...
>>
>>> I have brought this in and tried to get it running for Tegra. A few
>>> points:
>>>
>>> 1. The methods in struct i2c_adapter should IMO be passed a struct
>>> i2c_adapter *, so they can determine which adapter is being accessed.
>>> Otherwise I can't see how they would know.
>>
>> They can get the current used adapter through the defines in
>> include/i2c.h:
>> [...]
>> #define I2C_ADAP_NR(bus)        i2c_adap[i2c_bus[bus].adapter]
>> #define I2C_BUS                 ((struct i2c_bus_hose *)gd->cur_i2c_bus)
>> #define I2C_ADAP                i2c_adap[I2C_BUS->adapter]
>> #define I2C_ADAP_HWNR           (I2C_ADAP->hwadapnr)
>>
>> preparing just the fsl i2c driver and there I do for example:
>> drivers/i2c/fsl_i2c.c
>> [...]
>> static const struct fsl_i2c *i2c_dev[2] = {
>>          (struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C_OFFSET),
>> #ifdef CONFIG_SYS_FSL_I2C2_OFFSET
>>          (struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C2_OFFSET)
>> #endif
>> };
>> [...]
>> static int fsl_i2c_probe(uchar chip)
>> {
>>          struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
>> [...]
>>
>> but of course, we still can change the "struct i2c_adapter" if
>> needed ... but we have one more parameter ... Ok, not really a bad
>> problem.
>
> That rather relies on their being a concept of a "current" I2C adapter.
> It seems a little limiting to require that. What if the "current"
> adapter is the user-selected adapter for commands to operate on, but
> e.g. some power-management driver wants to use I2C to communicate with a
> PMIC during the internals of some other command. Sure, you could save
> and later restore the I2C core's idea of "current" adapter, but it'd
> surely be cleaner to just pass around the I2C adapter ID or struct
> pointer everywhere to avoid the need for save/restore.

Yes, you are right, but just the same problem with current code!
You mixed here two things!

The idea behind the current i2c adapter was/is, that the i2c
core need to know, which bus is "current" because there is the
possibility that on one adapter are more i2c busses, because
of using i2c muxes ... and we must know, on which bus we are
currently, because if we want to switch to another bus, we must
first disable the old way (and maybe disable the i2c adapter too).
-> If we want this feature, we need a current adapter. If we say,
    Ok, we do not want this disabling... we can get rid of it, yes!

But I think it is safer to disable the i2c muxes, before
switching to another bus ... so this "current i2c adapter"
is an i2c core internal! We should of course change the i2c
API that we pass the bus to the i2c API, but, as I said this
to simon, this is another big change, and I want to have this
step after getting in the new i2c framework (and maybe hope,
that when we convert to mareks DM, we get this for free),
because we must also adapt for example all dtt, RTC, because
they need to know on which bus they resist, and we have to
config this ...

The problem from storing/restoring the "current cmdline i2c
bus", is another problem, which is an independent patch!
You can make with current code a patch, which holds the current
i2c cmdline bus in a variable, and then get rid of this store/
restore calls as for example found in cmd_date.c ...

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

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Heiko Schocher]

Hello Tom,

On 26.10.2012 20:44, Tom Rini wrote:
> On Thu, Oct 25, 2012 at 02:37:08PM -0700, Simon Glass wrote:
>
> [snip]
>> Later, I wonder whether the concept of a 'current' i2c bus should be
>> maintained by the command line interpreter, rather than the i2c
>> system. Because to be honest, most of the drivers I see have to save
>> the current bus number, set the current bus, do the operation, then
>> set the bus back how they found it (to preserve whatever the user
>> things is the current bus).
>
> I agree.  Lets move the notion of "current" to cmd_i2c and make
> everything internally pass around the bus to operate on.  Or try going
> down this path and find a fatal problem :)

As I wrote to simon, stephen, this is an independent problem from the
new framework patches!

- There are two steps to do:
   - save the curent cmdline bus in a variable, and call i2c_set_bus_num()
     before you call i2c_* from the commandline, get rid of the store/restore
     calls ...

   - change the i2c API to pass the i2c bus in the i2c_* functions
     -> in the new i2c framework, i2c_set_bus() gets static and the
        gd->current_i2c_bus is used only in i2c_core.c

This two steps can be done in one step, but the second step is complicated
enough, so it is better to do it in two steps (I think)!

Waiting for patches ;-)

The i2c framework change is independent from this! The current_i2c_bus is
used i2c_core internally for storing the current active i2c bus, based
on the idea of having only one i2c bus active in U-Boot, see therefore
the U-Boot principle:

http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast
"Initialize devices only when they are needed within U-Boot, i.e. don't
initialize the Ethernet interface(s) unless U-Boot performs" [...]", etc.
(and don't forget to shut down these devices after using them -
otherwise nasty things may happen when you try to boot your OS). "

therefore the "current i2c bus" is needed! If we want to drop this
"U-Boot principle", we can get rid of current_i2c_bus ... (Ok, currently
U-Boot do not deactivate not used i2c adapters ...)

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

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Simon Glass]

Hi Heiko,

On Mon, Oct 29, 2012 at 2:44 AM, Heiko Schocher <hs@denx.de> wrote:
> Hello Simon,
>
>
> On 26.10.2012 18:08, Simon Glass wrote:
>>
>> On Thu, Oct 25, 2012 at 10:48 PM, Heiko Schocher<hs@denx.de>  wrote:
>>>
>>> Hello Simon,
>>>
>>>
>>> On 25.10.2012 23:37, Simon Glass wrote:
>>>>
>>>>
>>>> On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher<hs@denx.de>   wrote:
>
> [...]
>
>>>> 2. The init is a bit odd, in that we can call init() repeatedly.
>>>> Perhaps that function should be renamed to reset, and the init should
>>>> be used for calling just once at the start?
>>>
>>>
>>>
>>> What do you mean here exactly? I couldn?t parse this ...
>>
>>
>> Well there is start-of-day setup, which I think should be called init.
>> This is done once on boot for each i2c adapter.
>
>
> Hmm... I am not sure if this is only done on boot, because we should
> "close" or "deinit" an adapter if not used any more in U-Boot as the
> U-Boot principle says:
>
> http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast
>
> So I want to add in future some "deinit" to every adapter, and
> call it from i2c_set_bus() when switching to another i2c adapter ...

Well deinit() should be probably be done before finishing U-Boot, not
after every transaction, since you don't know that the current
transaction will be the last.

When using FDT, you need to look up the available i2c ports in the
driver, and this should be done once at the start. If the i2c core can
call a suitable init function then this is easier, rather than us
having to keep track of whether the init is done or not.

>
>
>> And then there is the i2c_init() which seems to be called whenever we
>> feel like it - e.g. to change speed. I suggest that we use init() to
>> mean start-of-day init and reset(), or similar, to mean any post-init.
>> I am not suggest that for this series, just as a future effort.
>
>
> Yes, that should be changed. We do not need an init() in the i2c
> API, as i2c_set_bus_num() do this for us (and later also the deinit())
>
> We just need a set/get_speed() and a deblock()/reset() ?
>
> Maybe a step in the API cleanup?

Yes, a future step I think. I feel that i2c is one of the darker
corners of U-Boot and so am keen to get this tidied up.

>
>
>>>> 3. Rather than each device having to call i2c_get_bus_num() to find
>>>> out what bus is referred to, why not just pass this information in? In
>>>> fact the adapter pointer can serve for this.
>>>
>>>
>>>
>>> Not the "struct i2c_adapter" must passed, but the "struct
>>> i2c_bus"!
>>>
>>> And each device should know, which i2c bus it uses, or? So at
>>> the end we should have something like i2c_read(struct i2c_bus *bus, ...)
>>> calls ... and the i2c core can detect, if this bus is the
>>> current, if so go on, if not switch to this bus. So at the
>>> end i2c_set_bus_num would be go static ...
>>>
>>>
>>>> 4. So perhaps the i2c read/write functions should change from:
>>>>
>>>> int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
>>>>
>>>> to:
>>>>
>>>> int i2c_read((struct i2c_adapter *adapter, uint addr, int alen, uchar
>>>> *buffer, int len)
>>>
>>>
>>>
>>> Yep, exactly, see comments to point 3 ...
>>>
>>> That would be the best (and I think in previous discussions I defined
>>> this as one goal ...), but this would be (another) big change,
>>> because this is an *API* change, with maybe a lot of config file
>>> changes ...
>>>
>>> Let us bring in the new i2c framework with all i2c drivers converted,
>>> and then do the next step ... maybe one step more, if mareks device
>>> model is ready, we can switch easy to DM ... and maybe get this API
>>> change for free ...
>>
>>
>> Yes. I certainly understand the need to fit in with what is already
>> there, and avoid a massive API change, which can be performed as a
>> follow-on patch anyway. With your info above I will adjust the tegra
>> driver to work with this and test it.
>
>
> Ok, great! So I post v2 patches after you tested ...
>
> And Yes, we should do this API change, but I tend to do this step after
> the new i2c framework is stable and all i2c drivers are converted to it ...

[snip]

Regards,
Simon

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

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Stephen Warren]

On 10/29/2012 03:47 AM, Heiko Schocher wrote:
> Hello Stephen,
> 
> On 26.10.2012 18:07, Stephen Warren wrote:
>> On 10/25/2012 11:48 PM, Heiko Schocher wrote:
>>> Hello Simon,
>>>
>>> On 25.10.2012 23:37, Simon Glass wrote:
>>>> On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher<hs@denx.de>   wrote:
>>>>> rebased/reworked the I2C multibus patches from Simon Glass found
>>>>> here:
>>>>>
>>>>> http://www.mail-archive.com/u-boot at lists.denx.de/msg75530.html
>>>>>
>>>>> It seems the timing is coming, to bring this in mainline and
>>>>> move boards over to the new i2c framework. As an example I
>>>>> converted the soft-i2c driver (and all boards using it) to
>>>>> the new framework, so this patchseries has to be tested
>>>>> intensively, as I can check compile only ...
>>>>
>>>> I am very happy to see this, thank you.
>>>
>>> I am too ;-) and Sorry that I am only now ready ...
>>>
>>>> I have brought this in and tried to get it running for Tegra. A few
>>>> points:
>>>>
>>>> 1. The methods in struct i2c_adapter should IMO be passed a struct
>>>> i2c_adapter *, so they can determine which adapter is being accessed.
>>>> Otherwise I can't see how they would know.
>>>
>>> They can get the current used adapter through the defines in
>>> include/i2c.h:
>>> [...]
>>> #define I2C_ADAP_NR(bus)        i2c_adap[i2c_bus[bus].adapter]
>>> #define I2C_BUS                 ((struct i2c_bus_hose *)gd->cur_i2c_bus)
>>> #define I2C_ADAP                i2c_adap[I2C_BUS->adapter]
>>> #define I2C_ADAP_HWNR           (I2C_ADAP->hwadapnr)
>>>
>>> preparing just the fsl i2c driver and there I do for example:
>>> drivers/i2c/fsl_i2c.c
>>> [...]
>>> static const struct fsl_i2c *i2c_dev[2] = {
>>>          (struct fsl_i2c *) (CONFIG_SYS_IMMR +
>>> CONFIG_SYS_FSL_I2C_OFFSET),
>>> #ifdef CONFIG_SYS_FSL_I2C2_OFFSET
>>>          (struct fsl_i2c *) (CONFIG_SYS_IMMR +
>>> CONFIG_SYS_FSL_I2C2_OFFSET)
>>> #endif
>>> };
>>> [...]
>>> static int fsl_i2c_probe(uchar chip)
>>> {
>>>          struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
>>> [...]
>>>
>>> but of course, we still can change the "struct i2c_adapter" if
>>> needed ... but we have one more parameter ... Ok, not really a bad
>>> problem.
>>
>> That rather relies on their being a concept of a "current" I2C adapter.
>> It seems a little limiting to require that. What if the "current"
>> adapter is the user-selected adapter for commands to operate on, but
>> e.g. some power-management driver wants to use I2C to communicate with a
>> PMIC during the internals of some other command. Sure, you could save
>> and later restore the I2C core's idea of "current" adapter, but it'd
>> surely be cleaner to just pass around the I2C adapter ID or struct
>> pointer everywhere to avoid the need for save/restore.
> 
> Yes, you are right, but just the same problem with current code!
> You mixed here two things!

I think you're reading more into what I was saying than what I actually
said.

If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
which one is in use. Passing that information directly to the driver
functions is much simple than requiring the SoC I2C driver to go grovel
in some I2C core global variables to find out the same information.

This is all unrelated to I2C bus muxes; they shouldn't be implemented as
part of an SoC I2C driver anyway, so the driver shouldn't know about bus
muxes before or after this patch - the I2C core should manage that.

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Simon Glass]

Hi Stephen,

On Mon, Oct 29, 2012 at 8:34 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/29/2012 03:47 AM, Heiko Schocher wrote:
>> Hello Stephen,
>>
>> On 26.10.2012 18:07, Stephen Warren wrote:
>>> On 10/25/2012 11:48 PM, Heiko Schocher wrote:
>>>> Hello Simon,
>>>>
>>>> On 25.10.2012 23:37, Simon Glass wrote:
>>>>> On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher<hs@denx.de>   wrote:
>>>>>> rebased/reworked the I2C multibus patches from Simon Glass found
>>>>>> here:
>>>>>>
>>>>>> http://www.mail-archive.com/u-boot at lists.denx.de/msg75530.html
>>>>>>
>>>>>> It seems the timing is coming, to bring this in mainline and
>>>>>> move boards over to the new i2c framework. As an example I
>>>>>> converted the soft-i2c driver (and all boards using it) to
>>>>>> the new framework, so this patchseries has to be tested
>>>>>> intensively, as I can check compile only ...
>>>>>
>>>>> I am very happy to see this, thank you.
>>>>
>>>> I am too ;-) and Sorry that I am only now ready ...
>>>>
>>>>> I have brought this in and tried to get it running for Tegra. A few
>>>>> points:
>>>>>
>>>>> 1. The methods in struct i2c_adapter should IMO be passed a struct
>>>>> i2c_adapter *, so they can determine which adapter is being accessed.
>>>>> Otherwise I can't see how they would know.
>>>>
>>>> They can get the current used adapter through the defines in
>>>> include/i2c.h:
>>>> [...]
>>>> #define I2C_ADAP_NR(bus)        i2c_adap[i2c_bus[bus].adapter]
>>>> #define I2C_BUS                 ((struct i2c_bus_hose *)gd->cur_i2c_bus)
>>>> #define I2C_ADAP                i2c_adap[I2C_BUS->adapter]
>>>> #define I2C_ADAP_HWNR           (I2C_ADAP->hwadapnr)
>>>>
>>>> preparing just the fsl i2c driver and there I do for example:
>>>> drivers/i2c/fsl_i2c.c
>>>> [...]
>>>> static const struct fsl_i2c *i2c_dev[2] = {
>>>>          (struct fsl_i2c *) (CONFIG_SYS_IMMR +
>>>> CONFIG_SYS_FSL_I2C_OFFSET),
>>>> #ifdef CONFIG_SYS_FSL_I2C2_OFFSET
>>>>          (struct fsl_i2c *) (CONFIG_SYS_IMMR +
>>>> CONFIG_SYS_FSL_I2C2_OFFSET)
>>>> #endif
>>>> };
>>>> [...]
>>>> static int fsl_i2c_probe(uchar chip)
>>>> {
>>>>          struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
>>>> [...]
>>>>
>>>> but of course, we still can change the "struct i2c_adapter" if
>>>> needed ... but we have one more parameter ... Ok, not really a bad
>>>> problem.
>>>
>>> That rather relies on their being a concept of a "current" I2C adapter.
>>> It seems a little limiting to require that. What if the "current"
>>> adapter is the user-selected adapter for commands to operate on, but
>>> e.g. some power-management driver wants to use I2C to communicate with a
>>> PMIC during the internals of some other command. Sure, you could save
>>> and later restore the I2C core's idea of "current" adapter, but it'd
>>> surely be cleaner to just pass around the I2C adapter ID or struct
>>> pointer everywhere to avoid the need for save/restore.
>>
>> Yes, you are right, but just the same problem with current code!
>> You mixed here two things!
>
> I think you're reading more into what I was saying than what I actually
> said.
>
> If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
> which one is in use. Passing that information directly to the driver
> functions is much simple than requiring the SoC I2C driver to go grovel
> in some I2C core global variables to find out the same information.

I think Heiko agreed with this, just that he wants to take things a
step at a time.

>
> This is all unrelated to I2C bus muxes; they shouldn't be implemented as
> part of an SoC I2C driver anyway, so the driver shouldn't know about bus
> muxes before or after this patch - the I2C core should manage that.

Regards,
Simon

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Heiko Schocher]

Hello Simon,

On 29.10.2012 14:48, Simon Glass wrote:
> Hi Heiko,
>
> On Mon, Oct 29, 2012 at 2:44 AM, Heiko Schocher<hs@denx.de>  wrote:
>> Hello Simon,
>>
>>
>> On 26.10.2012 18:08, Simon Glass wrote:
>>>
>>> On Thu, Oct 25, 2012 at 10:48 PM, Heiko Schocher<hs@denx.de>   wrote:
>>>>
>>>> Hello Simon,
>>>>
>>>>
>>>> On 25.10.2012 23:37, Simon Glass wrote:
>>>>>
>>>>>
>>>>> On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher<hs@denx.de>    wrote:
>>
>> [...]
>>
>>>>> 2. The init is a bit odd, in that we can call init() repeatedly.
>>>>> Perhaps that function should be renamed to reset, and the init should
>>>>> be used for calling just once at the start?
>>>>
>>>>
>>>>
>>>> What do you mean here exactly? I couldn?t parse this ...
>>>
>>>
>>> Well there is start-of-day setup, which I think should be called init.
>>> This is done once on boot for each i2c adapter.
>>
>>
>> Hmm... I am not sure if this is only done on boot, because we should
>> "close" or "deinit" an adapter if not used any more in U-Boot as the
>> U-Boot principle says:
>>
>> http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast
>>
>> So I want to add in future some "deinit" to every adapter, and
>> call it from i2c_set_bus() when switching to another i2c adapter ...
>
> Well deinit() should be probably be done before finishing U-Boot, not
> after every transaction, since you don't know that the current
> transaction will be the last.

Not after every transaction, only when switching to another adapter,
or before booting linux ...

> When using FDT, you need to look up the available i2c ports in the
> driver, and this should be done once at the start. If the i2c core can
> call a suitable init function then this is easier, rather than us
> having to keep track of whether the init is done or not.

Dummy question, why is this needed when using FDT?

>>> And then there is the i2c_init() which seems to be called whenever we
>>> feel like it - e.g. to change speed. I suggest that we use init() to
>>> mean start-of-day init and reset(), or similar, to mean any post-init.
>>> I am not suggest that for this series, just as a future effort.
>>
>>
>> Yes, that should be changed. We do not need an init() in the i2c
>> API, as i2c_set_bus_num() do this for us (and later also the deinit())
>>
>> We just need a set/get_speed() and a deblock()/reset() ?
>>
>> Maybe a step in the API cleanup?
>
> Yes, a future step I think. I feel that i2c is one of the darker
> corners of U-Boot and so am keen to get this tidied up.

Patches are welcome! Let us bring in the new framework, clean up
the i2c API / maybe DM integration, then we are on a good way I
think ...

[...]

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

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Heiko Schocher]

Hello Stephen,

On 29.10.2012 16:34, Stephen Warren wrote:
> On 10/29/2012 03:47 AM, Heiko Schocher wrote:
>> Hello Stephen,
>>
>> On 26.10.2012 18:07, Stephen Warren wrote:
>>> On 10/25/2012 11:48 PM, Heiko Schocher wrote:
>>>> Hello Simon,
>>>>
>>>> On 25.10.2012 23:37, Simon Glass wrote:
>>>>> On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher<hs@denx.de>    wrote:
[...]
>>> That rather relies on their being a concept of a "current" I2C adapter.
>>> It seems a little limiting to require that. What if the "current"
>>> adapter is the user-selected adapter for commands to operate on, but
>>> e.g. some power-management driver wants to use I2C to communicate with a
>>> PMIC during the internals of some other command. Sure, you could save
>>> and later restore the I2C core's idea of "current" adapter, but it'd
>>> surely be cleaner to just pass around the I2C adapter ID or struct
>>> pointer everywhere to avoid the need for save/restore.
>>
>> Yes, you are right, but just the same problem with current code!
>> You mixed here two things!
>
> I think you're reading more into what I was saying than what I actually
> said.

Sorry, maybe I misunderstood you ...

> If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
> which one is in use. Passing that information directly to the driver
> functions is much simple than requiring the SoC I2C driver to go grovel
> in some I2C core global variables to find out the same information.

Ah, do you mean we should change the i2c adapter struct from:

struct i2c_adapter {
        void            (*init)(int speed, int slaveaddr);
        int             (*probe)(uint8_t chip);
        int             (*read)(uint8_t chip, uint addr, int alen,
                                uint8_t *buffer, int len);
        int             (*write)(uint8_t chip, uint addr, int alen,
                                uint8_t *buffer, int len);
        uint            (*set_bus_speed)(uint speed);
[...]

to

struct i2c_adapter {
        void            (*init)(struct i2c_adapter *adap, int speed, int slaveaddr);
        int             (*probe)(struct i2c_adapter *adap, uint8_t chip);
        int             (*read)(struct i2c_adapter *adap, uint8_t chip, uint addr, int alen,
                                uint8_t *buffer, int len);
        int             (*write)(struct i2c_adapter *adap, uint8_t chip, uint addr, int alen,
                                uint8_t *buffer, int len);
        uint            (*set_bus_speed)(struct i2c_adapter *adap, uint speed);
[...]
?

We can do this. Simon suggested this too ...

@Simon: Is this Ok with you?

Nevertheless, we need a "cur_i2c_bus" pointer, as the i2c core, needs
to know the current i2c bus for detecting if the bus, which whould be
accessed is the current or not and a switching to another bus is needed.

> This is all unrelated to I2C bus muxes; they shouldn't be implemented as
> part of an SoC I2C driver anyway, so the driver shouldn't know about bus
> muxes before or after this patch - the I2C core should manage that.

Exactly! And that do the new i2c framework in i2c_core.c!

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

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Stephen Warren]

On 10/29/2012 11:57 PM, Heiko Schocher wrote:
> Hello Stephen,
> 
> On 29.10.2012 16:34, Stephen Warren wrote:
...
>> If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
>> which one is in use. Passing that information directly to the driver
>> functions is much simple than requiring the SoC I2C driver to go grovel
>> in some I2C core global variables to find out the same information.
> 
> Ah, do you mean we should change the i2c adapter struct from:
> 
> struct i2c_adapter {
>        void            (*init)(int speed, int slaveaddr);
>        int             (*probe)(uint8_t chip);
>        int             (*read)(uint8_t chip, uint addr, int alen,
>                                uint8_t *buffer, int len);
>        int             (*write)(uint8_t chip, uint addr, int alen,
>                                uint8_t *buffer, int len);
>        uint            (*set_bus_speed)(uint speed);
> [...]
> 
> to
> 
> struct i2c_adapter {
>        void            (*init)(struct i2c_adapter *adap, int speed, int
> slaveaddr);
>        int             (*probe)(struct i2c_adapter *adap, uint8_t chip);
>        int             (*read)(struct i2c_adapter *adap, uint8_t chip,
> uint addr, int alen,
>                                uint8_t *buffer, int len);
>        int             (*write)(struct i2c_adapter *adap, uint8_t chip,
> uint addr, int alen,
>                                uint8_t *buffer, int len);
>        uint            (*set_bus_speed)(struct i2c_adapter *adap, uint
> speed);
> [...]
> ?
> 
> We can do this. Simon suggested this too ...

Yes, exactly. (the functions will need some way to get information out
of the struct i2c_adapter; I assume there's some kind of driver_private
field in there too).

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Simon Glass]

Hi,

On Tue, Oct 30, 2012 at 9:50 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/29/2012 11:57 PM, Heiko Schocher wrote:
>> Hello Stephen,
>>
>> On 29.10.2012 16:34, Stephen Warren wrote:
> ...
>>> If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
>>> which one is in use. Passing that information directly to the driver
>>> functions is much simple than requiring the SoC I2C driver to go grovel
>>> in some I2C core global variables to find out the same information.
>>
>> Ah, do you mean we should change the i2c adapter struct from:
>>
>> struct i2c_adapter {
>>        void            (*init)(int speed, int slaveaddr);
>>        int             (*probe)(uint8_t chip);
>>        int             (*read)(uint8_t chip, uint addr, int alen,
>>                                uint8_t *buffer, int len);
>>        int             (*write)(uint8_t chip, uint addr, int alen,
>>                                uint8_t *buffer, int len);
>>        uint            (*set_bus_speed)(uint speed);
>> [...]
>>
>> to
>>
>> struct i2c_adapter {
>>        void            (*init)(struct i2c_adapter *adap, int speed, int
>> slaveaddr);
>>        int             (*probe)(struct i2c_adapter *adap, uint8_t chip);
>>        int             (*read)(struct i2c_adapter *adap, uint8_t chip,
>> uint addr, int alen,
>>                                uint8_t *buffer, int len);
>>        int             (*write)(struct i2c_adapter *adap, uint8_t chip,
>> uint addr, int alen,
>>                                uint8_t *buffer, int len);
>>        uint            (*set_bus_speed)(struct i2c_adapter *adap, uint
>> speed);
>> [...]
>> ?
>>
>> We can do this. Simon suggested this too ...
>
> Yes, exactly. (the functions will need some way to get information out
> of the struct i2c_adapter; I assume there's some kind of driver_private
> field in there too).

Yes.

I will post a few patches to move Tegra over to the new framework as
it is, so that people can see the impact. Given that every driver has
to change, it would be my preference to set the i2c API once. But I
suppose within a short while everything will move to the device model,
so perhaps the job of this series is just to move things in the right
direction?

Regards,
Simon

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Tom Rini]

On Mon, Oct 29, 2012 at 10:53:10AM +0100, Heiko Schocher wrote:
> Hello Tom,
> 
> On 26.10.2012 20:44, Tom Rini wrote:
> >On Thu, Oct 25, 2012 at 02:37:08PM -0700, Simon Glass wrote:
> >
> >[snip]
> >>Later, I wonder whether the concept of a 'current' i2c bus should be
> >>maintained by the command line interpreter, rather than the i2c
> >>system. Because to be honest, most of the drivers I see have to save
> >>the current bus number, set the current bus, do the operation, then
> >>set the bus back how they found it (to preserve whatever the user
> >>things is the current bus).
> >
> >I agree.  Lets move the notion of "current" to cmd_i2c and make
> >everything internally pass around the bus to operate on.  Or try going
> >down this path and find a fatal problem :)
> 
> As I wrote to simon, stephen, this is an independent problem from the
> new framework patches!

OK.  I was hoping we could just:
- Change init_func_i2c (for ARM/m68k/ppc/nds32, equiv elsewhere) to init
  all configured busses.
- Change the CLI part to track what bus it is operating on.

It sounds like it's more work than just this, really.  Since there is
agreement to push things further after this update, yes, OK, we can go
one step at a time here.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121030/30eda42a/attachment.pgp>

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Heiko Schocher]

Hello Simon,

On 30.10.2012 18:22, Simon Glass wrote:
> Hi,
>
> On Tue, Oct 30, 2012 at 9:50 AM, Stephen Warren<swarren@wwwdotorg.org>  wrote:
>> On 10/29/2012 11:57 PM, Heiko Schocher wrote:
>>> Hello Stephen,
>>>
>>> On 29.10.2012 16:34, Stephen Warren wrote:
>> ...
>>>> If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
>>>> which one is in use. Passing that information directly to the driver
>>>> functions is much simple than requiring the SoC I2C driver to go grovel
>>>> in some I2C core global variables to find out the same information.
>>>
>>> Ah, do you mean we should change the i2c adapter struct from:
>>>
>>> struct i2c_adapter {
>>>         void            (*init)(int speed, int slaveaddr);
>>>         int             (*probe)(uint8_t chip);
>>>         int             (*read)(uint8_t chip, uint addr, int alen,
>>>                                 uint8_t *buffer, int len);
>>>         int             (*write)(uint8_t chip, uint addr, int alen,
>>>                                 uint8_t *buffer, int len);
>>>         uint            (*set_bus_speed)(uint speed);
>>> [...]
>>>
>>> to
>>>
>>> struct i2c_adapter {
>>>         void            (*init)(struct i2c_adapter *adap, int speed, int
>>> slaveaddr);
>>>         int             (*probe)(struct i2c_adapter *adap, uint8_t chip);
>>>         int             (*read)(struct i2c_adapter *adap, uint8_t chip,
>>> uint addr, int alen,
>>>                                 uint8_t *buffer, int len);
>>>         int             (*write)(struct i2c_adapter *adap, uint8_t chip,
>>> uint addr, int alen,
>>>                                 uint8_t *buffer, int len);
>>>         uint            (*set_bus_speed)(struct i2c_adapter *adap, uint
>>> speed);
>>> [...]
>>> ?
>>>
>>> We can do this. Simon suggested this too ...
>>
>> Yes, exactly. (the functions will need some way to get information out
>> of the struct i2c_adapter; I assume there's some kind of driver_private
>> field in there too).
>
> Yes.
>
> I will post a few patches to move Tegra over to the new framework as
> it is, so that people can see the impact. Given that every driver has
> to change, it would be my preference to set the i2c API once. But I
> suppose within a short while everything will move to the device model,
> so perhaps the job of this series is just to move things in the right
> direction?

Yes, but not only, I think the i2c framework needs an update.

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

[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Tom Rini]

On Tue, Oct 30, 2012 at 10:22:54AM -0700, Simon Glass wrote:
> Hi,
> 
> On Tue, Oct 30, 2012 at 9:50 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> > On 10/29/2012 11:57 PM, Heiko Schocher wrote:
> >> Hello Stephen,
> >>
> >> On 29.10.2012 16:34, Stephen Warren wrote:
> > ...
> >>> If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
> >>> which one is in use. Passing that information directly to the driver
> >>> functions is much simple than requiring the SoC I2C driver to go grovel
> >>> in some I2C core global variables to find out the same information.
> >>
> >> Ah, do you mean we should change the i2c adapter struct from:
> >>
> >> struct i2c_adapter {
> >>        void            (*init)(int speed, int slaveaddr);
> >>        int             (*probe)(uint8_t chip);
> >>        int             (*read)(uint8_t chip, uint addr, int alen,
> >>                                uint8_t *buffer, int len);
> >>        int             (*write)(uint8_t chip, uint addr, int alen,
> >>                                uint8_t *buffer, int len);
> >>        uint            (*set_bus_speed)(uint speed);
> >> [...]
> >>
> >> to
> >>
> >> struct i2c_adapter {
> >>        void            (*init)(struct i2c_adapter *adap, int speed, int
> >> slaveaddr);
> >>        int             (*probe)(struct i2c_adapter *adap, uint8_t chip);
> >>        int             (*read)(struct i2c_adapter *adap, uint8_t chip,
> >> uint addr, int alen,
> >>                                uint8_t *buffer, int len);
> >>        int             (*write)(struct i2c_adapter *adap, uint8_t chip,
> >> uint addr, int alen,
> >>                                uint8_t *buffer, int len);
> >>        uint            (*set_bus_speed)(struct i2c_adapter *adap, uint
> >> speed);
> >> [...]
> >> ?
> >>
> >> We can do this. Simon suggested this too ...
> >
> > Yes, exactly. (the functions will need some way to get information out
> > of the struct i2c_adapter; I assume there's some kind of driver_private
> > field in there too).
> 
> Yes.
> 
> I will post a few patches to move Tegra over to the new framework as
> it is, so that people can see the impact. Given that every driver has
> to change, it would be my preference to set the i2c API once. But I
> suppose within a short while everything will move to the device model,
> so perhaps the job of this series is just to move things in the right
> direction?

Funny enough I don't see a UDM-i2c.txt file, so I guess the community
gets to think about that one :)

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121030/48d42ea8/attachment.pgp>