[U-Boot] [PATCH 0/2] i2c: fti2c010: migrate to new i2c model

[U-Boot] [PATCH 0/2] i2c: fti2c010: migrate to new i2c model

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

This changeset migrate the Faraday I2C controller (i.e., FTI2C010)
from legacy i2c model to the new one.

Kuo-Jung Su (2):
  i2c: fti2c010: cosmetic: coding style cleanup
  i2c: fti2c010: migrate to new i2c model

 drivers/i2c/fti2c010.c |  326 ++++++++++++++++++++++--------------------------
 1 file changed, 147 insertions(+), 179 deletions(-)

--
1.7.9.5

[U-Boot] [PATCH 1/2] i2c: fti2c010: cosmetic: coding style cleanup

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

Coding style cleanup

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
Cc: Heiko Schocher <hs@denx.de>
---
 drivers/i2c/fti2c010.c |   31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/fti2c010.c b/drivers/i2c/fti2c010.c
index ddeb941..ec6afc9 100644
--- a/drivers/i2c/fti2c010.c
+++ b/drivers/i2c/fti2c010.c
@@ -18,22 +18,23 @@
 #endif

 #ifndef CONFIG_SYS_I2C_SPEED
-#define CONFIG_SYS_I2C_SPEED    50000
+#define CONFIG_SYS_I2C_SPEED    5000
 #endif

-#ifndef CONFIG_FTI2C010_FREQ
-#define CONFIG_FTI2C010_FREQ    clk_get_rate("I2C")
+#ifndef CONFIG_FTI2C010_CLOCK
+#define CONFIG_FTI2C010_CLOCK   clk_get_rate("I2C")
 #endif

-/* command timeout */
-#define CFG_CMD_TIMEOUT         10 /* ms */
+#ifndef CONFIG_FTI2C010_TIMEOUT
+#define CONFIG_FTI2C010_TIMEOUT 10 /* ms */
+#endif

-/* 7-bit chip address + 1-bit read/write */
-#define I2C_RD(chip)            ((((chip) << 1) & 0xff) | 1)
-#define I2C_WR(chip)            (((chip) << 1) & 0xff)
+/* 7-bit dev address + 1-bit read/write */
+#define I2C_RD(dev)             ((((dev) << 1) & 0xfe) | 1)
+#define I2C_WR(dev)             (((dev) << 1) & 0xfe)

 struct fti2c010_chip {
-	void __iomem *regs;
+	struct fti2c010_regs *regs;
 	uint bus;
 	uint speed;
 };
@@ -41,25 +42,25 @@ struct fti2c010_chip {
 static struct fti2c010_chip chip_list[] = {
 	{
 		.bus  = 0,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE,
 	},
 #ifdef CONFIG_I2C_MULTI_BUS
 # ifdef CONFIG_FTI2C010_BASE1
 	{
 		.bus  = 1,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE1,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE1,
 	},
 # endif
 # ifdef CONFIG_FTI2C010_BASE2
 	{
 		.bus  = 2,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE2,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE2,
 	},
 # endif
 # ifdef CONFIG_FTI2C010_BASE3
 	{
 		.bus  = 3,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE3,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE3,
 	},
 # endif
 #endif  /* #ifdef CONFIG_I2C_MULTI_BUS */
@@ -73,7 +74,7 @@ static int fti2c010_wait(uint32_t mask)
 	uint32_t stat, ts;
 	struct fti2c010_regs *regs = curr->regs;

-	for (ts = get_timer(0); get_timer(ts) < CFG_CMD_TIMEOUT; ) {
+	for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) {
 		stat = readl(&regs->sr);
 		if ((stat & mask) == mask) {
 			ret = 0;
@@ -324,7 +325,7 @@ uint i2c_get_bus_num(void)
 int i2c_set_bus_speed(uint speed)
 {
 	struct fti2c010_regs *regs = curr->regs;
-	uint clk = CONFIG_FTI2C010_FREQ;
+	uint clk = CONFIG_FTI2C010_CLOCK;
 	uint gsr = 0, tsr = 32;
 	uint spd, div;

--
1.7.9.5

[U-Boot] [PATCH 2/2] i2c: fti2c010: migrate to new i2c model

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

Replace the legacy i2c model with the new one.

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
Cc: Heiko Schocher <hs@denx.de>
---
 drivers/i2c/fti2c010.c |  299 +++++++++++++++++++++---------------------------
 1 file changed, 133 insertions(+), 166 deletions(-)

diff --git a/drivers/i2c/fti2c010.c b/drivers/i2c/fti2c010.c
index ec6afc9..eccc1da 100644
--- a/drivers/i2c/fti2c010.c
+++ b/drivers/i2c/fti2c010.c
@@ -13,14 +13,14 @@
 
 #include "fti2c010.h"
 
-#ifndef CONFIG_HARD_I2C
-#error "fti2c010: CONFIG_HARD_I2C is not defined"
-#endif
-
 #ifndef CONFIG_SYS_I2C_SPEED
 #define CONFIG_SYS_I2C_SPEED    5000
 #endif
 
+#ifndef CONFIG_SYS_I2C_SLAVE
+#define CONFIG_SYS_I2C_SLAVE    0
+#endif
+
 #ifndef CONFIG_FTI2C010_CLOCK
 #define CONFIG_FTI2C010_CLOCK   clk_get_rate("I2C")
 #endif
@@ -35,44 +35,54 @@
 
 struct fti2c010_chip {
 	struct fti2c010_regs *regs;
-	uint bus;
-	uint speed;
 };
 
 static struct fti2c010_chip chip_list[] = {
 	{
-		.bus  = 0,
 		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE,
 	},
-#ifdef CONFIG_I2C_MULTI_BUS
-# ifdef CONFIG_FTI2C010_BASE1
+#ifdef CONFIG_FTI2C010_BASE1
 	{
-		.bus  = 1,
 		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE1,
 	},
-# endif
-# ifdef CONFIG_FTI2C010_BASE2
+#endif
+#ifdef CONFIG_FTI2C010_BASE2
 	{
-		.bus  = 2,
 		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE2,
 	},
-# endif
-# ifdef CONFIG_FTI2C010_BASE3
+#endif
+#ifdef CONFIG_FTI2C010_BASE3
 	{
-		.bus  = 3,
 		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE3,
 	},
-# endif
-#endif  /* #ifdef CONFIG_I2C_MULTI_BUS */
+#endif
 };
 
-static struct fti2c010_chip *curr = chip_list;
+static int fti2c010_reset(struct fti2c010_chip *chip)
+{
+	ulong ts;
+	int ret = -1;
+	struct fti2c010_regs *regs = chip->regs;
 
-static int fti2c010_wait(uint32_t mask)
+	writel(CR_I2CRST, &regs->cr);
+	for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) {
+		if (!(readl(&regs->cr) & CR_I2CRST)) {
+			ret = 0;
+			break;
+		}
+	}
+
+	if (ret)
+		printf("fti2c010: reset timeout\n");
+
+	return ret;
+}
+
+static int fti2c010_wait(struct fti2c010_chip *chip, uint32_t mask)
 {
 	int ret = -1;
 	uint32_t stat, ts;
-	struct fti2c010_regs *regs = curr->regs;
+	struct fti2c010_regs *regs = chip->regs;
 
 	for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) {
 		stat = readl(&regs->sr);
@@ -85,74 +95,97 @@ static int fti2c010_wait(uint32_t mask)
 	return ret;
 }
 
-/*
- * u-boot I2C API
- */
+static unsigned int set_i2c_bus_speed(struct fti2c010_chip *chip,
+	unsigned int speed)
+{
+	struct fti2c010_regs *regs = chip->regs;
+	unsigned int clk = CONFIG_FTI2C010_CLOCK;
+	unsigned int gsr = 0;
+	unsigned int tsr = 32;
+	unsigned int div, rate;
+
+	for (div = 0; div < 0x3ffff; ++div) {
+		/* SCLout = PCLK/(2*(COUNT + 2) + GSR) */
+		rate = clk / (2 * (div + 2) + gsr);
+		if (rate <= speed)
+			break;
+	}
+
+	writel(TGSR_GSR(gsr) | TGSR_TSR(tsr), &regs->tgsr);
+	writel(CDR_DIV(div), &regs->cdr);
+
+	return rate;
+}
 
 /*
  * Initialization, must be called once on start up, may be called
  * repeatedly to change the speed and slave addresses.
  */
-void i2c_init(int speed, int slaveaddr)
+static void fti2c010_init(struct i2c_adapter *adap, int speed, int slaveaddr)
 {
-	if (speed || !curr->speed)
-		i2c_set_bus_speed(speed);
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
 
-	/* if slave mode disabled */
-	if (!slaveaddr)
+	if (adap->init_done)
 		return;
 
-	/*
-	 * TODO:
-	 * Implement slave mode, but is it really necessary?
-	 */
+#ifdef CONFIG_SYS_I2C_INIT_BOARD
+	/* Call board specific i2c bus reset routine before accessing the
+	 * environment, which might be in a chip on that bus. For details
+	 * about this problem see doc/I2C_Edge_Conditions.
+	*/
+	i2c_init_board();
+#endif
+
+	/* master init */
+
+	fti2c010_reset(chip);
+
+	set_i2c_bus_speed(chip, speed);
+
+	/* slave init, don't care */
+
+#ifdef CONFIG_SYS_I2C_BOARD_LATE_INIT
+	/* Call board specific i2c bus reset routine AFTER the bus has been
+	 * initialized. Use either this callpoint or i2c_init_board;
+	 * which is called before fti2c010_init operations.
+	 * For details about this problem see doc/I2C_Edge_Conditions.
+	*/
+	i2c_board_late_init();
+#endif
 }
 
 /*
  * Probe the given I2C chip address.  Returns 0 if a chip responded,
  * not 0 on failure.
  */
-int i2c_probe(uchar chip)
+static int fti2c010_probe(struct i2c_adapter *adap, u8 dev)
 {
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
+	struct fti2c010_regs *regs = chip->regs;
 	int ret;
-	struct fti2c010_regs *regs = curr->regs;
-
-	i2c_init(0, 0);
 
 	/* 1. Select slave device (7bits Address + 1bit R/W) */
-	writel(I2C_WR(chip), &regs->dr);
+	writel(I2C_WR(dev), &regs->dr);
 	writel(CR_ENABLE | CR_TBEN | CR_START, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 	if (ret)
 		return ret;
 
 	/* 2. Select device register */
 	writel(0, &regs->dr);
 	writel(CR_ENABLE | CR_TBEN, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 
 	return ret;
 }
 
-/*
- * 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(uchar chip, uint addr, int alen, uchar *buf, int len)
+static int fti2c010_read(struct i2c_adapter *adap,
+			u8 dev, uint addr, int alen, uchar *buf, int len)
 {
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
+	struct fti2c010_regs *regs = chip->regs;
 	int ret, pos;
 	uchar paddr[4];
-	struct fti2c010_regs *regs = curr->regs;
-
-	i2c_init(0, 0);
 
 	paddr[0] = (addr >> 0)  & 0xFF;
 	paddr[1] = (addr >> 8)  & 0xFF;
@@ -164,9 +197,9 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 	 */
 
 	/* A.1 Select slave device (7bits Address + 1bit R/W) */
-	writel(I2C_WR(chip), &regs->dr);
+	writel(I2C_WR(dev), &regs->dr);
 	writel(CR_ENABLE | CR_TBEN | CR_START, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 	if (ret)
 		return ret;
 
@@ -176,7 +209,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 
 		writel(paddr[pos], &regs->dr);
 		writel(ctrl, &regs->cr);
-		ret = fti2c010_wait(SR_DT);
+		ret = fti2c010_wait(chip, SR_DT);
 		if (ret)
 			return ret;
 	}
@@ -186,9 +219,9 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 	 */
 
 	/* B.1 Select slave device (7bits Address + 1bit R/W) */
-	writel(I2C_RD(chip), &regs->dr);
+	writel(I2C_RD(dev), &regs->dr);
 	writel(CR_ENABLE | CR_TBEN | CR_START, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 	if (ret)
 		return ret;
 
@@ -202,7 +235,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 			stat |= SR_ACK;
 		}
 		writel(ctrl, &regs->cr);
-		ret = fti2c010_wait(stat);
+		ret = fti2c010_wait(chip, stat);
 		if (ret)
 			break;
 		buf[pos] = (uchar)(readl(&regs->dr) & 0xFF);
@@ -211,25 +244,13 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 	return ret;
 }
 
-/*
- * 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_write(uchar chip, uint addr, int alen, uchar *buf, int len)
+static int fti2c010_write(struct i2c_adapter *adap,
+			u8 dev, uint addr, int alen, u8 *buf, int len)
 {
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
+	struct fti2c010_regs *regs = chip->regs;
 	int ret, pos;
 	uchar paddr[4];
-	struct fti2c010_regs *regs = curr->regs;
-
-	i2c_init(0, 0);
 
 	paddr[0] = (addr >> 0)  & 0xFF;
 	paddr[1] = (addr >> 8)  & 0xFF;
@@ -241,9 +262,9 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
 	 *
 	 * A.1 Select slave device (7bits Address + 1bit R/W)
 	 */
-	writel(I2C_WR(chip), &regs->dr);
+	writel(I2C_WR(dev), &regs->dr);
 	writel(CR_ENABLE | CR_TBEN | CR_START, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 	if (ret)
 		return ret;
 
@@ -253,7 +274,7 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
 
 		writel(paddr[pos], &regs->dr);
 		writel(ctrl, &regs->cr);
-		ret = fti2c010_wait(SR_DT);
+		ret = fti2c010_wait(chip, SR_DT);
 		if (ret)
 			return ret;
 	}
@@ -268,7 +289,7 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
 			ctrl |= CR_STOP;
 		writel(buf[pos], &regs->dr);
 		writel(ctrl, &regs->cr);
-		ret = fti2c010_wait(SR_DT);
+		ret = fti2c010_wait(chip, SR_DT);
 		if (ret)
 			break;
 	}
@@ -276,94 +297,40 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
 	return ret;
 }
 
-/*
- * Functions for setting the current I2C bus and its speed
- */
-#ifdef CONFIG_I2C_MULTI_BUS
-
-/*
- * 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(uint bus)
-{
-	if (bus >= ARRAY_SIZE(chip_list))
-		return -1;
-	curr = chip_list + bus;
-	i2c_init(0, 0);
-	return 0;
-}
-
-/*
- * i2c_get_bus_num:
- *
- *  Returns index of currently active I2C bus.  Zero-based.
- */
-
-uint i2c_get_bus_num(void)
-{
-	return curr->bus;
-}
-
-#endif    /* #ifdef CONFIG_I2C_MULTI_BUS */
-
-/*
- * i2c_set_bus_speed:
- *
- *  Change the speed of the active I2C bus
- *
- *    speed - bus speed in Hz
- *
- *    Returns: 0 on success, not 0 on failure
- */
-int i2c_set_bus_speed(uint speed)
+static unsigned int fti2c010_set_bus_speed(struct i2c_adapter *adap,
+			unsigned int speed)
 {
-	struct fti2c010_regs *regs = curr->regs;
-	uint clk = CONFIG_FTI2C010_CLOCK;
-	uint gsr = 0, tsr = 32;
-	uint spd, div;
-
-	if (!speed)
-		speed = CONFIG_SYS_I2C_SPEED;
-
-	for (div = 0; div < 0x3ffff; ++div) {
-		/* SCLout = PCLK/(2*(COUNT + 2) + GSR) */
-		spd = clk / (2 * (div + 2) + gsr);
-		if (spd <= speed)
-			break;
-	}
-
-	if (curr->speed == spd)
-		return 0;
-
-	writel(CR_I2CRST, &regs->cr);
-	mdelay(100);
-	if (readl(&regs->cr) & CR_I2CRST) {
-		printf("fti2c010: reset timeout\n");
-		return -1;
-	}
-
-	curr->speed = spd;
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
+	int ret;
 
-	writel(TGSR_GSR(gsr) | TGSR_TSR(tsr), &regs->tgsr);
-	writel(CDR_DIV(div), &regs->cdr);
+	fti2c010_reset(chip);
+	ret = set_i2c_bus_speed(chip, speed);
 
-	return 0;
+	return ret;
 }
 
 /*
- * i2c_get_bus_speed:
- *
- *  Returns speed of currently active I2C bus in Hz
+ * Register i2c adapters
  */
-
-uint i2c_get_bus_speed(void)
-{
-	return curr->speed;
-}
+U_BOOT_I2C_ADAP_COMPLETE(i2c_0, fti2c010_init, fti2c010_probe, fti2c010_read,
+			fti2c010_write, fti2c010_set_bus_speed,
+			CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE,
+			0)
+#ifdef CONFIG_FTI2C010_BASE1
+U_BOOT_I2C_ADAP_COMPLETE(i2c_1, fti2c010_init, fti2c010_probe, fti2c010_read,
+			fti2c010_write, fti2c010_set_bus_speed,
+			CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE,
+			1)
+#endif
+#ifdef CONFIG_FTI2C010_BASE2
+U_BOOT_I2C_ADAP_COMPLETE(i2c_2, fti2c010_init, fti2c010_probe, fti2c010_read,
+			fti2c010_write, fti2c010_set_bus_speed,
+			CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE,
+			2)
+#endif
+#ifdef CONFIG_FTI2C010_BASE3
+U_BOOT_I2C_ADAP_COMPLETE(i2c_3, fti2c010_init, fti2c010_probe, fti2c010_read,
+			fti2c010_write, fti2c010_set_bus_speed,
+			CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE,
+			3)
+#endif
-- 
1.7.9.5

[U-Boot] [PATCH v2 0/4] i2c: fti2c010: bug fix & new driver model

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

This changeset adapts the driver to the new one.
In patch v2, it also includes a bug fix for i2c r/w address and eeprom.

Changes for v2:
  1. eeprom: bug fix for i2c read/write
  2. fti2c010: serial out r/w address in MSB order

Kuo-Jung Su (4):
  i2c: fti2c010: cosmetic: coding style cleanup
  i2c: fti2c010: migrate to new i2c model
  i2c: fti2c010: serial out r/w address in MSB order
  cmd_eeprom: bug fix for i2c read/write

 common/cmd_eeprom.c    |    4 +-
 drivers/i2c/fti2c010.c |  364 +++++++++++++++++++++++-------------------------
 2 files changed, 179 insertions(+), 189 deletions(-)

--
1.7.9.5

[U-Boot] [PATCH v2 1/4] i2c: fti2c010: cosmetic: coding style cleanup

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

Coding style cleanup

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
Cc: Heiko Schocher <hs@denx.de>
---
 Changes for v2:
  - Nothing updates

 drivers/i2c/fti2c010.c |   31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/fti2c010.c b/drivers/i2c/fti2c010.c
index ddeb941..ec6afc9 100644
--- a/drivers/i2c/fti2c010.c
+++ b/drivers/i2c/fti2c010.c
@@ -18,22 +18,23 @@
 #endif

 #ifndef CONFIG_SYS_I2C_SPEED
-#define CONFIG_SYS_I2C_SPEED    50000
+#define CONFIG_SYS_I2C_SPEED    5000
 #endif

-#ifndef CONFIG_FTI2C010_FREQ
-#define CONFIG_FTI2C010_FREQ    clk_get_rate("I2C")
+#ifndef CONFIG_FTI2C010_CLOCK
+#define CONFIG_FTI2C010_CLOCK   clk_get_rate("I2C")
 #endif

-/* command timeout */
-#define CFG_CMD_TIMEOUT         10 /* ms */
+#ifndef CONFIG_FTI2C010_TIMEOUT
+#define CONFIG_FTI2C010_TIMEOUT 10 /* ms */
+#endif

-/* 7-bit chip address + 1-bit read/write */
-#define I2C_RD(chip)            ((((chip) << 1) & 0xff) | 1)
-#define I2C_WR(chip)            (((chip) << 1) & 0xff)
+/* 7-bit dev address + 1-bit read/write */
+#define I2C_RD(dev)             ((((dev) << 1) & 0xfe) | 1)
+#define I2C_WR(dev)             (((dev) << 1) & 0xfe)

 struct fti2c010_chip {
-	void __iomem *regs;
+	struct fti2c010_regs *regs;
 	uint bus;
 	uint speed;
 };
@@ -41,25 +42,25 @@ struct fti2c010_chip {
 static struct fti2c010_chip chip_list[] = {
 	{
 		.bus  = 0,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE,
 	},
 #ifdef CONFIG_I2C_MULTI_BUS
 # ifdef CONFIG_FTI2C010_BASE1
 	{
 		.bus  = 1,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE1,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE1,
 	},
 # endif
 # ifdef CONFIG_FTI2C010_BASE2
 	{
 		.bus  = 2,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE2,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE2,
 	},
 # endif
 # ifdef CONFIG_FTI2C010_BASE3
 	{
 		.bus  = 3,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE3,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE3,
 	},
 # endif
 #endif  /* #ifdef CONFIG_I2C_MULTI_BUS */
@@ -73,7 +74,7 @@ static int fti2c010_wait(uint32_t mask)
 	uint32_t stat, ts;
 	struct fti2c010_regs *regs = curr->regs;

-	for (ts = get_timer(0); get_timer(ts) < CFG_CMD_TIMEOUT; ) {
+	for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) {
 		stat = readl(&regs->sr);
 		if ((stat & mask) == mask) {
 			ret = 0;
@@ -324,7 +325,7 @@ uint i2c_get_bus_num(void)
 int i2c_set_bus_speed(uint speed)
 {
 	struct fti2c010_regs *regs = curr->regs;
-	uint clk = CONFIG_FTI2C010_FREQ;
+	uint clk = CONFIG_FTI2C010_CLOCK;
 	uint gsr = 0, tsr = 32;
 	uint spd, div;

--
1.7.9.5

[U-Boot] [PATCH v2 2/4] i2c: fti2c010: migrate to new i2c model

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

Replace the legacy i2c model with the new one.

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
Cc: Heiko Schocher <hs@denx.de>
---
 Changes for v2:
  - Nothing updates

 drivers/i2c/fti2c010.c |  299 +++++++++++++++++++++---------------------------
 1 file changed, 133 insertions(+), 166 deletions(-)

diff --git a/drivers/i2c/fti2c010.c b/drivers/i2c/fti2c010.c
index ec6afc9..eccc1da 100644
--- a/drivers/i2c/fti2c010.c
+++ b/drivers/i2c/fti2c010.c
@@ -13,14 +13,14 @@

 #include "fti2c010.h"

-#ifndef CONFIG_HARD_I2C
-#error "fti2c010: CONFIG_HARD_I2C is not defined"
-#endif
-
 #ifndef CONFIG_SYS_I2C_SPEED
 #define CONFIG_SYS_I2C_SPEED    5000
 #endif

+#ifndef CONFIG_SYS_I2C_SLAVE
+#define CONFIG_SYS_I2C_SLAVE    0
+#endif
+
 #ifndef CONFIG_FTI2C010_CLOCK
 #define CONFIG_FTI2C010_CLOCK   clk_get_rate("I2C")
 #endif
@@ -35,44 +35,54 @@

 struct fti2c010_chip {
 	struct fti2c010_regs *regs;
-	uint bus;
-	uint speed;
 };

 static struct fti2c010_chip chip_list[] = {
 	{
-		.bus  = 0,
 		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE,
 	},
-#ifdef CONFIG_I2C_MULTI_BUS
-# ifdef CONFIG_FTI2C010_BASE1
+#ifdef CONFIG_FTI2C010_BASE1
 	{
-		.bus  = 1,
 		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE1,
 	},
-# endif
-# ifdef CONFIG_FTI2C010_BASE2
+#endif
+#ifdef CONFIG_FTI2C010_BASE2
 	{
-		.bus  = 2,
 		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE2,
 	},
-# endif
-# ifdef CONFIG_FTI2C010_BASE3
+#endif
+#ifdef CONFIG_FTI2C010_BASE3
 	{
-		.bus  = 3,
 		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE3,
 	},
-# endif
-#endif  /* #ifdef CONFIG_I2C_MULTI_BUS */
+#endif
 };

-static struct fti2c010_chip *curr = chip_list;
+static int fti2c010_reset(struct fti2c010_chip *chip)
+{
+	ulong ts;
+	int ret = -1;
+	struct fti2c010_regs *regs = chip->regs;

-static int fti2c010_wait(uint32_t mask)
+	writel(CR_I2CRST, &regs->cr);
+	for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) {
+		if (!(readl(&regs->cr) & CR_I2CRST)) {
+			ret = 0;
+			break;
+		}
+	}
+
+	if (ret)
+		printf("fti2c010: reset timeout\n");
+
+	return ret;
+}
+
+static int fti2c010_wait(struct fti2c010_chip *chip, uint32_t mask)
 {
 	int ret = -1;
 	uint32_t stat, ts;
-	struct fti2c010_regs *regs = curr->regs;
+	struct fti2c010_regs *regs = chip->regs;

 	for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) {
 		stat = readl(&regs->sr);
@@ -85,74 +95,97 @@ static int fti2c010_wait(uint32_t mask)
 	return ret;
 }

-/*
- * u-boot I2C API
- */
+static unsigned int set_i2c_bus_speed(struct fti2c010_chip *chip,
+	unsigned int speed)
+{
+	struct fti2c010_regs *regs = chip->regs;
+	unsigned int clk = CONFIG_FTI2C010_CLOCK;
+	unsigned int gsr = 0;
+	unsigned int tsr = 32;
+	unsigned int div, rate;
+
+	for (div = 0; div < 0x3ffff; ++div) {
+		/* SCLout = PCLK/(2*(COUNT + 2) + GSR) */
+		rate = clk / (2 * (div + 2) + gsr);
+		if (rate <= speed)
+			break;
+	}
+
+	writel(TGSR_GSR(gsr) | TGSR_TSR(tsr), &regs->tgsr);
+	writel(CDR_DIV(div), &regs->cdr);
+
+	return rate;
+}

 /*
  * Initialization, must be called once on start up, may be called
  * repeatedly to change the speed and slave addresses.
  */
-void i2c_init(int speed, int slaveaddr)
+static void fti2c010_init(struct i2c_adapter *adap, int speed, int slaveaddr)
 {
-	if (speed || !curr->speed)
-		i2c_set_bus_speed(speed);
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;

-	/* if slave mode disabled */
-	if (!slaveaddr)
+	if (adap->init_done)
 		return;

-	/*
-	 * TODO:
-	 * Implement slave mode, but is it really necessary?
-	 */
+#ifdef CONFIG_SYS_I2C_INIT_BOARD
+	/* Call board specific i2c bus reset routine before accessing the
+	 * environment, which might be in a chip on that bus. For details
+	 * about this problem see doc/I2C_Edge_Conditions.
+	*/
+	i2c_init_board();
+#endif
+
+	/* master init */
+
+	fti2c010_reset(chip);
+
+	set_i2c_bus_speed(chip, speed);
+
+	/* slave init, don't care */
+
+#ifdef CONFIG_SYS_I2C_BOARD_LATE_INIT
+	/* Call board specific i2c bus reset routine AFTER the bus has been
+	 * initialized. Use either this callpoint or i2c_init_board;
+	 * which is called before fti2c010_init operations.
+	 * For details about this problem see doc/I2C_Edge_Conditions.
+	*/
+	i2c_board_late_init();
+#endif
 }

 /*
  * Probe the given I2C chip address.  Returns 0 if a chip responded,
  * not 0 on failure.
  */
-int i2c_probe(uchar chip)
+static int fti2c010_probe(struct i2c_adapter *adap, u8 dev)
 {
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
+	struct fti2c010_regs *regs = chip->regs;
 	int ret;
-	struct fti2c010_regs *regs = curr->regs;
-
-	i2c_init(0, 0);

 	/* 1. Select slave device (7bits Address + 1bit R/W) */
-	writel(I2C_WR(chip), &regs->dr);
+	writel(I2C_WR(dev), &regs->dr);
 	writel(CR_ENABLE | CR_TBEN | CR_START, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 	if (ret)
 		return ret;

 	/* 2. Select device register */
 	writel(0, &regs->dr);
 	writel(CR_ENABLE | CR_TBEN, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);

 	return ret;
 }

-/*
- * 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(uchar chip, uint addr, int alen, uchar *buf, int len)
+static int fti2c010_read(struct i2c_adapter *adap,
+			u8 dev, uint addr, int alen, uchar *buf, int len)
 {
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
+	struct fti2c010_regs *regs = chip->regs;
 	int ret, pos;
 	uchar paddr[4];
-	struct fti2c010_regs *regs = curr->regs;
-
-	i2c_init(0, 0);

 	paddr[0] = (addr >> 0)  & 0xFF;
 	paddr[1] = (addr >> 8)  & 0xFF;
@@ -164,9 +197,9 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 	 */

 	/* A.1 Select slave device (7bits Address + 1bit R/W) */
-	writel(I2C_WR(chip), &regs->dr);
+	writel(I2C_WR(dev), &regs->dr);
 	writel(CR_ENABLE | CR_TBEN | CR_START, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 	if (ret)
 		return ret;

@@ -176,7 +209,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)

 		writel(paddr[pos], &regs->dr);
 		writel(ctrl, &regs->cr);
-		ret = fti2c010_wait(SR_DT);
+		ret = fti2c010_wait(chip, SR_DT);
 		if (ret)
 			return ret;
 	}
@@ -186,9 +219,9 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 	 */

 	/* B.1 Select slave device (7bits Address + 1bit R/W) */
-	writel(I2C_RD(chip), &regs->dr);
+	writel(I2C_RD(dev), &regs->dr);
 	writel(CR_ENABLE | CR_TBEN | CR_START, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 	if (ret)
 		return ret;

@@ -202,7 +235,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 			stat |= SR_ACK;
 		}
 		writel(ctrl, &regs->cr);
-		ret = fti2c010_wait(stat);
+		ret = fti2c010_wait(chip, stat);
 		if (ret)
 			break;
 		buf[pos] = (uchar)(readl(&regs->dr) & 0xFF);
@@ -211,25 +244,13 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 	return ret;
 }

-/*
- * 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_write(uchar chip, uint addr, int alen, uchar *buf, int len)
+static int fti2c010_write(struct i2c_adapter *adap,
+			u8 dev, uint addr, int alen, u8 *buf, int len)
 {
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
+	struct fti2c010_regs *regs = chip->regs;
 	int ret, pos;
 	uchar paddr[4];
-	struct fti2c010_regs *regs = curr->regs;
-
-	i2c_init(0, 0);

 	paddr[0] = (addr >> 0)  & 0xFF;
 	paddr[1] = (addr >> 8)  & 0xFF;
@@ -241,9 +262,9 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
 	 *
 	 * A.1 Select slave device (7bits Address + 1bit R/W)
 	 */
-	writel(I2C_WR(chip), &regs->dr);
+	writel(I2C_WR(dev), &regs->dr);
 	writel(CR_ENABLE | CR_TBEN | CR_START, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 	if (ret)
 		return ret;

@@ -253,7 +274,7 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)

 		writel(paddr[pos], &regs->dr);
 		writel(ctrl, &regs->cr);
-		ret = fti2c010_wait(SR_DT);
+		ret = fti2c010_wait(chip, SR_DT);
 		if (ret)
 			return ret;
 	}
@@ -268,7 +289,7 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
 			ctrl |= CR_STOP;
 		writel(buf[pos], &regs->dr);
 		writel(ctrl, &regs->cr);
-		ret = fti2c010_wait(SR_DT);
+		ret = fti2c010_wait(chip, SR_DT);
 		if (ret)
 			break;
 	}
@@ -276,94 +297,40 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
 	return ret;
 }

-/*
- * Functions for setting the current I2C bus and its speed
- */
-#ifdef CONFIG_I2C_MULTI_BUS
-
-/*
- * 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(uint bus)
-{
-	if (bus >= ARRAY_SIZE(chip_list))
-		return -1;
-	curr = chip_list + bus;
-	i2c_init(0, 0);
-	return 0;
-}
-
-/*
- * i2c_get_bus_num:
- *
- *  Returns index of currently active I2C bus.  Zero-based.
- */
-
-uint i2c_get_bus_num(void)
-{
-	return curr->bus;
-}
-
-#endif    /* #ifdef CONFIG_I2C_MULTI_BUS */
-
-/*
- * i2c_set_bus_speed:
- *
- *  Change the speed of the active I2C bus
- *
- *    speed - bus speed in Hz
- *
- *    Returns: 0 on success, not 0 on failure
- */
-int i2c_set_bus_speed(uint speed)
+static unsigned int fti2c010_set_bus_speed(struct i2c_adapter *adap,
+			unsigned int speed)
 {
-	struct fti2c010_regs *regs = curr->regs;
-	uint clk = CONFIG_FTI2C010_CLOCK;
-	uint gsr = 0, tsr = 32;
-	uint spd, div;
-
-	if (!speed)
-		speed = CONFIG_SYS_I2C_SPEED;
-
-	for (div = 0; div < 0x3ffff; ++div) {
-		/* SCLout = PCLK/(2*(COUNT + 2) + GSR) */
-		spd = clk / (2 * (div + 2) + gsr);
-		if (spd <= speed)
-			break;
-	}
-
-	if (curr->speed == spd)
-		return 0;
-
-	writel(CR_I2CRST, &regs->cr);
-	mdelay(100);
-	if (readl(&regs->cr) & CR_I2CRST) {
-		printf("fti2c010: reset timeout\n");
-		return -1;
-	}
-
-	curr->speed = spd;
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
+	int ret;

-	writel(TGSR_GSR(gsr) | TGSR_TSR(tsr), &regs->tgsr);
-	writel(CDR_DIV(div), &regs->cdr);
+	fti2c010_reset(chip);
+	ret = set_i2c_bus_speed(chip, speed);

-	return 0;
+	return ret;
 }

 /*
- * i2c_get_bus_speed:
- *
- *  Returns speed of currently active I2C bus in Hz
+ * Register i2c adapters
  */
-
-uint i2c_get_bus_speed(void)
-{
-	return curr->speed;
-}
+U_BOOT_I2C_ADAP_COMPLETE(i2c_0, fti2c010_init, fti2c010_probe, fti2c010_read,
+			fti2c010_write, fti2c010_set_bus_speed,
+			CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE,
+			0)
+#ifdef CONFIG_FTI2C010_BASE1
+U_BOOT_I2C_ADAP_COMPLETE(i2c_1, fti2c010_init, fti2c010_probe, fti2c010_read,
+			fti2c010_write, fti2c010_set_bus_speed,
+			CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE,
+			1)
+#endif
+#ifdef CONFIG_FTI2C010_BASE2
+U_BOOT_I2C_ADAP_COMPLETE(i2c_2, fti2c010_init, fti2c010_probe, fti2c010_read,
+			fti2c010_write, fti2c010_set_bus_speed,
+			CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE,
+			2)
+#endif
+#ifdef CONFIG_FTI2C010_BASE3
+U_BOOT_I2C_ADAP_COMPLETE(i2c_3, fti2c010_init, fti2c010_probe, fti2c010_read,
+			fti2c010_write, fti2c010_set_bus_speed,
+			CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE,
+			3)
+#endif
--
1.7.9.5

[U-Boot] [PATCH v2 3/4] i2c: fti2c010: serial out r/w address in MSB order

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

For a eeprom with a 2-bytes address (e.g., Ateml AT24C1024B),
the r/w address should be serial out in MSB order.

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
Cc: Heiko Schocher <hs@denx.de>
---
 Changes for v2:
  - Initial release

 drivers/i2c/fti2c010.c |   38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/fti2c010.c b/drivers/i2c/fti2c010.c
index eccc1da..351fa38 100644
--- a/drivers/i2c/fti2c010.c
+++ b/drivers/i2c/fti2c010.c
@@ -179,6 +179,34 @@ static int fti2c010_probe(struct i2c_adapter *adap, u8 dev)
 	return ret;
 }

+static void addr_to_i2c_data(u8 *buf, uint32_t val, int len)
+{
+	if (!buf || len <= 0)
+		return;
+
+	/* MSB first */
+	switch (len) {
+	case 1:
+		buf[0] = (u8)(val & 0xff);
+		break;
+	case 2:
+		buf[0] = (u8)((val >> 8) & 0xff);
+		buf[1] = (u8)(val & 0xff);
+		break;
+	case 3:
+		buf[0] = (u8)((val >> 16) & 0xff);
+		buf[1] = (u8)((val >> 8) & 0xff);
+		buf[2] = (u8)(val & 0xff);
+		break;
+	default:
+		buf[0] = (u8)((val >> 24) & 0xff);
+		buf[1] = (u8)((val >> 16) & 0xff);
+		buf[2] = (u8)((val >> 8) & 0xff);
+		buf[3] = (u8)(val & 0xff);
+		break;
+	}
+}
+
 static int fti2c010_read(struct i2c_adapter *adap,
 			u8 dev, uint addr, int alen, uchar *buf, int len)
 {
@@ -187,10 +215,7 @@ static int fti2c010_read(struct i2c_adapter *adap,
 	int ret, pos;
 	uchar paddr[4];

-	paddr[0] = (addr >> 0)  & 0xFF;
-	paddr[1] = (addr >> 8)  & 0xFF;
-	paddr[2] = (addr >> 16) & 0xFF;
-	paddr[3] = (addr >> 24) & 0xFF;
+	addr_to_i2c_data(paddr, addr, alen);

 	/*
 	 * Phase A. Set register address
@@ -252,10 +277,7 @@ static int fti2c010_write(struct i2c_adapter *adap,
 	int ret, pos;
 	uchar paddr[4];

-	paddr[0] = (addr >> 0)  & 0xFF;
-	paddr[1] = (addr >> 8)  & 0xFF;
-	paddr[2] = (addr >> 16) & 0xFF;
-	paddr[3] = (addr >> 24) & 0xFF;
+	addr_to_i2c_data(paddr, addr, alen);

 	/*
 	 * Phase A. Set register address
--
1.7.9.5

[U-Boot] [PATCH v2 4/4] cmd_eeprom: bug fix for i2c read/write

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

The local pointer of address (i.e., addr) only gets
referenced in SPI mode, and it won't be appropriate
to pass only 1 bytes addr[1] to i2c_read/i2c_write while
CONFIG_SYS_I2C_EEPROM_ADDR_LEN > 1.

To avoid ambiguity, this patch would drop the use of
address pointer in I2C mode, and directly pass (dev_addr, offset)
to i2c_read/i2c_write.

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
cc: Peter Tyser <ptyser@xes-inc.com>
Cc: Heiko Schocher <hs@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Mischa Jonker <mjonker@synopsys.com>
---
 Changes for v2:
  - Initial release

 common/cmd_eeprom.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/cmd_eeprom.c b/common/cmd_eeprom.c
index 02539c4..0edc259 100644
--- a/common/cmd_eeprom.c
+++ b/common/cmd_eeprom.c
@@ -161,7 +161,7 @@ int eeprom_read (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cnt
 #if defined(CONFIG_SPI) && !defined(CONFIG_ENV_EEPROM_IS_ON_I2C)
 		spi_read (addr, alen, buffer, len);
 #else
-		if (i2c_read (addr[0], addr[1], alen-1, buffer, len) != 0)
+		if (i2c_read (dev_addr, offset, alen-1, buffer, len) != 0)
 			rcode = 1;
 #endif
 		buffer += len;
@@ -339,7 +339,7 @@ int eeprom_write (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cn
 		/* Write is enabled ... now write eeprom value.
 		 */
 #endif
-		if (i2c_write (addr[0], addr[1], alen-1, buffer, len) != 0)
+		if (i2c_write (dev_addr, offset, alen-1, buffer, len) != 0)
 			rcode = 1;

 #endif
--
1.7.9.5

[U-Boot] [PATCH v2 4/4] cmd_eeprom: bug fix for i2c read/write

[Alexey Brodkin]

On Thu, 2013-11-28 at 10:47 +0800, Kuo-Jung Su wrote:
> From: Kuo-Jung Su <dantesu@faraday-tech.com>
> 
> The local pointer of address (i.e., addr) only gets
> referenced in SPI mode, and it won't be appropriate
> to pass only 1 bytes addr[1] to i2c_read/i2c_write while
> CONFIG_SYS_I2C_EEPROM_ADDR_LEN > 1.
> 
> To avoid ambiguity, this patch would drop the use of
> address pointer in I2C mode, and directly pass (dev_addr, offset)
> to i2c_read/i2c_write.

Unfortunately this patch breaks cases with
"CONFIG_SYS_I2C_EEPROM_ADDR_LEN = 1" where address is limited to 1 byte
- thus a need to pass "addr[0]" which is combined from real I2C device
address and 256-byte word offset (i.e. MSB part of offset). And
"addr[1]" only carries lower 8 bit of offset.

So I would recommend to separate 2 invocations of "i2c_{read|write}":
1) for "CONFIG_SYS_I2C_EEPROM_ADDR_LEN = 1"
2) for "CONFIG_SYS_I2C_EEPROM_ADDR_LEN > 1"

-Alexey

> 
>  common/cmd_eeprom.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/common/cmd_eeprom.c b/common/cmd_eeprom.c
> index 02539c4..0edc259 100644
> --- a/common/cmd_eeprom.c
> +++ b/common/cmd_eeprom.c
> @@ -161,7 +161,7 @@ int eeprom_read (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cnt
>  #if defined(CONFIG_SPI) && !defined(CONFIG_ENV_EEPROM_IS_ON_I2C)
>  		spi_read (addr, alen, buffer, len);
>  #else
> -		if (i2c_read (addr[0], addr[1], alen-1, buffer, len) != 0)
> +		if (i2c_read (dev_addr, offset, alen-1, buffer, len) != 0)
>  			rcode = 1;
>  #endif
>  		buffer += len;
> @@ -339,7 +339,7 @@ int eeprom_write (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cn
>  		/* Write is enabled ... now write eeprom value.
>  		 */
>  #endif
> -		if (i2c_write (addr[0], addr[1], alen-1, buffer, len) != 0)
> +		if (i2c_write (dev_addr, offset, alen-1, buffer, len) != 0)
>  			rcode = 1;
> 
>  #endif
> --
> 1.7.9.5
> 

[U-Boot] [PATCH v2 4/4] cmd_eeprom: bug fix for i2c read/write

[Kuo-Jung Su]

2013/11/28 Alexey Brodkin <Alexey.Brodkin@synopsys.com>:
> On Thu, 2013-11-28 at 10:47 +0800, Kuo-Jung Su wrote:
>> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>>
>> The local pointer of address (i.e., addr) only gets
>> referenced in SPI mode, and it won't be appropriate
>> to pass only 1 bytes addr[1] to i2c_read/i2c_write while
>> CONFIG_SYS_I2C_EEPROM_ADDR_LEN > 1.
>>
>> To avoid ambiguity, this patch would drop the use of
>> address pointer in I2C mode, and directly pass (dev_addr, offset)
>> to i2c_read/i2c_write.
>
> Unfortunately this patch breaks cases with
> "CONFIG_SYS_I2C_EEPROM_ADDR_LEN = 1" where address is limited to 1 byte
> - thus a need to pass "addr[0]" which is combined from real I2C device
> address and 256-byte word offset (i.e. MSB part of offset). And
> "addr[1]" only carries lower 8 bit of offset.
>
> So I would recommend to separate 2 invocations of "i2c_{read|write}":
> 1) for "CONFIG_SYS_I2C_EEPROM_ADDR_LEN = 1"
> 2) for "CONFIG_SYS_I2C_EEPROM_ADDR_LEN > 1"
>
> -Alexey
>

Hi Alexey:

No it's a common misunderstanding, I also made the same mistake before.
Please check my bug fix for Faraday FTI2C010 I2C driver:

http://patchwork.ozlabs.org/patch/294732/

The address should be rebuild inside the i2c driver in u-boot's I2C model.

Another example could be found at drivers/i2c/soft_i2c.c, line 382 - 389

static int soft_i2c_read (...)
{
    ......
    shift = (alen-1) * 8;
    while(alen-- > 0) {
        if(write_byte(addr >> shift)) {
            PRINTD("i2c_read, address not <ACK>ed\n");
            return(1);
        }
        shift -= 8;
    }
    ......
}

However the root cause is the u-boot i2c driver model, the address should
never be passed to i2c_read/i2c_write in uint format.

Please take a peak at how ecos defineds the relative i2c routines:

externC cyg_uint32  cyg_i2c_transaction_tx(const cyg_i2c_device*,
cyg_bool, const cyg_uint8*, cyg_uint32, cyg_bool);
externC cyg_uint32  cyg_i2c_transaction_rx(const cyg_i2c_device*,
cyg_bool, cyg_uint8*, cyg_uint32, cyg_bool, cyg_bool);

In this way, the eeprom address would be forced to rebuild in address
pointer (i.e., addr[]) as what we did in SPI mode.

Best Wishes
Kuo-Jung Su

[U-Boot] [PATCH v2 4/4] cmd_eeprom: bug fix for i2c read/write

[Alexey Brodkin]

Hi Kuo-Jung,

On Fri, 2013-11-29 at 08:59 +0800, Kuo-Jung Su wrote:
> No it's a common misunderstanding, I also made the same mistake before.
> Please check my bug fix for Faraday FTI2C010 I2C driver:
> 
> http://patchwork.ozlabs.org/patch/294732/
> 
> The address should be rebuild inside the i2c driver in u-boot's I2C model.
> 
> Another example could be found at drivers/i2c/soft_i2c.c, line 382 - 389
> 
> static int soft_i2c_read (...)
> {
>     ......
>     shift = (alen-1) * 8;
>     while(alen-- > 0) {
>         if(write_byte(addr >> shift)) {
>             PRINTD("i2c_read, address not <ACK>ed\n");
>             return(1);
>         }
>         shift -= 8;
>     }
>     ......
> }
> 
> However the root cause is the u-boot i2c driver model, the address should
> never be passed to i2c_read/i2c_write in uint format.
> 
> Please take a peak at how ecos defineds the relative i2c routines:
> 
> externC cyg_uint32  cyg_i2c_transaction_tx(const cyg_i2c_device*,
> cyg_bool, const cyg_uint8*, cyg_uint32, cyg_bool);
> externC cyg_uint32  cyg_i2c_transaction_rx(const cyg_i2c_device*,
> cyg_bool, cyg_uint8*, cyg_uint32, cyg_bool, cyg_bool);
> 
> In this way, the eeprom address would be forced to rebuild in address
> pointer (i.e., addr[]) as what we did in SPI mode.

Unfortunately I still cannot agree with you.
In my opinion I2C driver has nothing to do with current situation.

It's purely how manufacturers of EEPROM use I2C.
For example I we use "PCF8594C-2" EEPROM.
Here's a datasheet -
http://www.nxp.com/documents/data_sheet/PCF8594C_2.pdf

Its capacity is 512 bytes. And as you may see from "Fig 3. Device
addressing." Each IC houses 2 virtual EEPROMs (each housing 256 bytes of
data).

And if you accept this philosophy you'll understand why "I2C chip
address" is modified and why following approach is used in
"cmd_eeprom.c" for "CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1" (which says
that EEPROM has address length of 1 byte):
==================
addr[0] = offset >> 8;	/* block number */
addr[1] = blk_off;	/* block offset */
addr[0] |= dev_addr;	/* insert device address */
==================

[U-Boot] [PATCH v2 4/4] cmd_eeprom: bug fix for i2c read/write

[Kuo-Jung Su]

2013/11/29 Alexey Brodkin <Alexey.Brodkin@synopsys.com>:
> Hi Kuo-Jung,
>
> On Fri, 2013-11-29 at 08:59 +0800, Kuo-Jung Su wrote:
>> No it's a common misunderstanding, I also made the same mistake before.
>> Please check my bug fix for Faraday FTI2C010 I2C driver:
>>
>> http://patchwork.ozlabs.org/patch/294732/
>>
>> The address should be rebuild inside the i2c driver in u-boot's I2C model.
>>
>> Another example could be found at drivers/i2c/soft_i2c.c, line 382 - 389
>>
>> static int soft_i2c_read (...)
>> {
>>     ......
>>     shift = (alen-1) * 8;
>>     while(alen-- > 0) {
>>         if(write_byte(addr >> shift)) {
>>             PRINTD("i2c_read, address not <ACK>ed\n");
>>             return(1);
>>         }
>>         shift -= 8;
>>     }
>>     ......
>> }
>>
>> However the root cause is the u-boot i2c driver model, the address should
>> never be passed to i2c_read/i2c_write in uint format.
>>
>> Please take a peak at how ecos defineds the relative i2c routines:
>>
>> externC cyg_uint32  cyg_i2c_transaction_tx(const cyg_i2c_device*,
>> cyg_bool, const cyg_uint8*, cyg_uint32, cyg_bool);
>> externC cyg_uint32  cyg_i2c_transaction_rx(const cyg_i2c_device*,
>> cyg_bool, cyg_uint8*, cyg_uint32, cyg_bool, cyg_bool);
>>
>> In this way, the eeprom address would be forced to rebuild in address
>> pointer (i.e., addr[]) as what we did in SPI mode.
>
> Unfortunately I still cannot agree with you.
> In my opinion I2C driver has nothing to do with current situation.
>

Yes, that's why I said the root cause is U-Boot's I2C model.
The address should never be rebuilt/reformated inside the I2C driver.
The better solution is to update the i2c_read/i2c_write to:

int i2c_read(struct i2c_adapter *adap, u8 dev, uint *addr, int alen,
uchar *buf, int len)

or

int i2c_read(struct i2c_adapter *adap, u8 dev, uchar *buf, int len)

i.e., drop the use of uint 'addr'

> It's purely how manufacturers of EEPROM use I2C.
> For example I we use "PCF8594C-2" EEPROM.
> Here's a datasheet -
> http://www.nxp.com/documents/data_sheet/PCF8594C_2.pdf
>
> Its capacity is 512 bytes. And as you may see from "Fig 3. Device
> addressing." Each IC houses 2 virtual EEPROMs (each housing 256 bytes of
> data).
>

Yes, it looks like a special case which use BIT0 of device address
for page selection. Which means we can't directly pass the device address
to i2c_read/i2c_write. But it's still o.k to directly the 'offset' as
what we did before.

> And if you accept this philosophy you'll understand why "I2C chip
> address" is modified and why following approach is used in
> "cmd_eeprom.c" for "CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1" (which says
> that EEPROM has address length of 1 byte):
> ==================
> addr[0] = offset >> 8;  /* block number */
> addr[1] = blk_off;      /* block offset */
> addr[0] |= dev_addr;    /* insert device address */
> ==================
>
> From source code above you see that virtually it addresses multiple 256
> byte EEPROMs.
>
> Regards,
> Alexey
>



-- 
Best wishes,
Kuo-Jung Su

[U-Boot] [PATCH v2 4/4] cmd_eeprom: bug fix for i2c read/write

[Alexey Brodkin]

On Fri, 2013-11-29 at 17:32 +0800, Kuo-Jung Su wrote:
> > Unfortunately I still cannot agree with you.
> > In my opinion I2C driver has nothing to do with current situation.
> >
> 
> Yes, that's why I said the root cause is U-Boot's I2C model.
> The address should never be rebuilt/reformated inside the I2C driver.
> The better solution is to update the i2c_read/i2c_write to:
> 
> int i2c_read(struct i2c_adapter *adap, u8 dev, uint *addr, int alen,
> uchar *buf, int len)
> 
> or
> 
> int i2c_read(struct i2c_adapter *adap, u8 dev, uchar *buf, int len)
> 
> i.e., drop the use of uint 'addr'

Well, even in current state of I2C core and some particular drivers
(like DW I2C) no reformat of address happens as far as I can tell.

But I cannot tell about each and every I2C driver in U-boot.

Still if you'd like to modify "U-Boot's I2C model" it is up to you to
start this discussion/work if you haven't start it yet (sorry I don't
read entire U-boot mailing list daily).

But then it's good to modify all drivers as well so nobody gets
dead/not-built I2C drivers at some point, right?

> > It's purely how manufacturers of EEPROM use I2C.
> > For example I we use "PCF8594C-2" EEPROM.
> > Here's a datasheet -
> > http://www.nxp.com/documents/data_sheet/PCF8594C_2.pdf
> >
> > Its capacity is 512 bytes. And as you may see from "Fig 3. Device
> > addressing." Each IC houses 2 virtual EEPROMs (each housing 256 bytes of
> > data).
> >
> 
> Yes, it looks like a special case which use BIT0 of device address
> for page selection. Which means we can't directly pass the device address
> to i2c_read/i2c_write. But it's still o.k to directly the 'offset' as
> what we did before.

Frankly I don't understand how passing entire "offset" will work in this
particular corner-case. From my explanation you see that we have to
mimic 8-bit address of target byte in EEPROM. And even if our I2C
controller may use 10 bits for addressing it is something that we don't
want use here - we need 8-bit as we do now.

Regards,
Alexey

[U-Boot] [PATCH v2 4/4] cmd_eeprom: bug fix for i2c read/write

[Kuo-Jung Su]

2013/11/29 Alexey Brodkin <Alexey.Brodkin@synopsys.com>:
> On Fri, 2013-11-29 at 17:32 +0800, Kuo-Jung Su wrote:
>> > Unfortunately I still cannot agree with you.
>> > In my opinion I2C driver has nothing to do with current situation.
>> >
>>
>> Yes, that's why I said the root cause is U-Boot's I2C model.
>> The address should never be rebuilt/reformated inside the I2C driver.
>> The better solution is to update the i2c_read/i2c_write to:
>>
>> int i2c_read(struct i2c_adapter *adap, u8 dev, uint *addr, int alen,
>> uchar *buf, int len)
>>
>> or
>>
>> int i2c_read(struct i2c_adapter *adap, u8 dev, uchar *buf, int len)
>>
>> i.e., drop the use of uint 'addr'
>
> Well, even in current state of I2C core and some particular drivers
> (like DW I2C) no reformat of address happens as far as I can tell.
>

Did you mean designware_i2c.c?
Well... it does not support multi-bytes address, so it didn't reformat
the address in MSB order.

Please check line 208 ~ 211 & line 227 in following link:

http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/designware_i2c.c;h=cb2ac04b609864412a8054888f3420bf35ca0287;hb=d19ad726bcd5d9106f7ba9c750462fcc369f1020

The soft_i2c.c is still the best example for this issue.

> But I cannot tell about each and every I2C driver in U-boot.
>

As I know both soft_i2c.c & zynq_i2c.c do the address reformat
(i.e., MSB shift)
I think the soft_i2c.c is the best example for this issue.
All the answer could be found from it.

e.g., CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW

is the solution for your EEPROM, but it's only get implemented
inside soft_i2c.c....

> Still if you'd like to modify "U-Boot's I2C model" it is up to you to
> start this discussion/work if you haven't start it yet (sorry I don't
> read entire U-boot mailing list daily).
>
> But then it's good to modify all drivers as well so nobody gets
> dead/not-built I2C drivers at some point, right?
>

No, it's merely a suggestion, not a critical bug fix request.
If the I2C devices supported in U-Boot are all use MSB address,
then it's no good to do it.
It simply looks weird....
e.g.,
CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW is only implemented
inside soft_i2c.c (i2c driver), not in cmd_eeprom.c
And thus some drivers (e.g., my fti2c010) might not support this feature.

>> > It's purely how manufacturers of EEPROM use I2C.
>> > For example I we use "PCF8594C-2" EEPROM.
>> > Here's a datasheet -
>> > http://www.nxp.com/documents/data_sheet/PCF8594C_2.pdf
>> >
>> > Its capacity is 512 bytes. And as you may see from "Fig 3. Device
>> > addressing." Each IC houses 2 virtual EEPROMs (each housing 256 bytes of
>> > data).
>> >
>>
>> Yes, it looks like a special case which use BIT0 of device address
>> for page selection. Which means we can't directly pass the device address
>> to i2c_read/i2c_write. But it's still o.k to directly the 'offset' as
>> what we did before.
>
> Frankly I don't understand how passing entire "offset" will work in this
> particular corner-case. From my explanation you see that we have to
> mimic 8-bit address of target byte in EEPROM. And even if our I2C
> controller may use 10 bits for addressing it is something that we don't
> want use here - we need 8-bit as we do now.
>

Check soft_i2c.c, you could the answer in line 351 ~ 367

http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/soft_i2c.c;h=396fea89af545bb2eba8a1827a21f423e717d40b;hb=d19ad726bcd5d9106f7ba9c750462fcc369f1020

Best Wishes
Dante Su

> Regards,
> Alexey
>



-- 
Best wishes,
Kuo-Jung Su

[U-Boot] [PATCH v3 0/4] i2c: fti2c010: bug fix & new driver model

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

This changeset adapts the fti2c010.c to the new i2c driver model.

Changes for v3:
  1. cmd_eeprom: Pass 'addr[0]' instead of 'dev_addr' into i2c r/w
     routines.
  2. fti2c010: serial out r/w address in MSB order: coding style update

Changes for v2:
  1. cmd_eeprom: bug fix for i2c read/write
  2. fti2c010: serial out r/w address in MSB order

Kuo-Jung Su (4):
  i2c: fti2c010: cosmetic: coding style cleanup
  i2c: fti2c010: migrate to new i2c model
  i2c: fti2c010: serial out r/w address in MSB order
  cmd_eeprom: bug fix for i2c read/write

 common/cmd_eeprom.c    |    4 +-
 drivers/i2c/fti2c010.c |  352 +++++++++++++++++++++++-------------------------
 2 files changed, 167 insertions(+), 189 deletions(-)

--
1.7.9.5

[U-Boot] [PATCH v3 1/4] i2c: fti2c010: cosmetic: coding style cleanup

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

Coding style cleanup

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
Cc: Heiko Schocher <hs@denx.de>
---
 Changes for v2 & v3:
  - Nothing updates

 drivers/i2c/fti2c010.c |   31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/fti2c010.c b/drivers/i2c/fti2c010.c
index ddeb941..ec6afc9 100644
--- a/drivers/i2c/fti2c010.c
+++ b/drivers/i2c/fti2c010.c
@@ -18,22 +18,23 @@
 #endif

 #ifndef CONFIG_SYS_I2C_SPEED
-#define CONFIG_SYS_I2C_SPEED    50000
+#define CONFIG_SYS_I2C_SPEED    5000
 #endif

-#ifndef CONFIG_FTI2C010_FREQ
-#define CONFIG_FTI2C010_FREQ    clk_get_rate("I2C")
+#ifndef CONFIG_FTI2C010_CLOCK
+#define CONFIG_FTI2C010_CLOCK   clk_get_rate("I2C")
 #endif

-/* command timeout */
-#define CFG_CMD_TIMEOUT         10 /* ms */
+#ifndef CONFIG_FTI2C010_TIMEOUT
+#define CONFIG_FTI2C010_TIMEOUT 10 /* ms */
+#endif

-/* 7-bit chip address + 1-bit read/write */
-#define I2C_RD(chip)            ((((chip) << 1) & 0xff) | 1)
-#define I2C_WR(chip)            (((chip) << 1) & 0xff)
+/* 7-bit dev address + 1-bit read/write */
+#define I2C_RD(dev)             ((((dev) << 1) & 0xfe) | 1)
+#define I2C_WR(dev)             (((dev) << 1) & 0xfe)

 struct fti2c010_chip {
-	void __iomem *regs;
+	struct fti2c010_regs *regs;
 	uint bus;
 	uint speed;
 };
@@ -41,25 +42,25 @@ struct fti2c010_chip {
 static struct fti2c010_chip chip_list[] = {
 	{
 		.bus  = 0,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE,
 	},
 #ifdef CONFIG_I2C_MULTI_BUS
 # ifdef CONFIG_FTI2C010_BASE1
 	{
 		.bus  = 1,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE1,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE1,
 	},
 # endif
 # ifdef CONFIG_FTI2C010_BASE2
 	{
 		.bus  = 2,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE2,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE2,
 	},
 # endif
 # ifdef CONFIG_FTI2C010_BASE3
 	{
 		.bus  = 3,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE3,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE3,
 	},
 # endif
 #endif  /* #ifdef CONFIG_I2C_MULTI_BUS */
@@ -73,7 +74,7 @@ static int fti2c010_wait(uint32_t mask)
 	uint32_t stat, ts;
 	struct fti2c010_regs *regs = curr->regs;

-	for (ts = get_timer(0); get_timer(ts) < CFG_CMD_TIMEOUT; ) {
+	for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) {
 		stat = readl(&regs->sr);
 		if ((stat & mask) == mask) {
 			ret = 0;
@@ -324,7 +325,7 @@ uint i2c_get_bus_num(void)
 int i2c_set_bus_speed(uint speed)
 {
 	struct fti2c010_regs *regs = curr->regs;
-	uint clk = CONFIG_FTI2C010_FREQ;
+	uint clk = CONFIG_FTI2C010_CLOCK;
 	uint gsr = 0, tsr = 32;
 	uint spd, div;

--
1.7.9.5

[U-Boot] [PATCH v3 0/4] i2c: fti2c010: bug fix & new driver model

[Heiko Schocher]

Hello Kuop-Jung,

Am 02.12.2013 03:57, schrieb Kuo-Jung Su:
> From: Kuo-Jung Su<dantesu@faraday-tech.com>
>
> This changeset adapts the fti2c010.c to the new i2c driver model.
>
> Changes for v3:
>    1. cmd_eeprom: Pass 'addr[0]' instead of 'dev_addr' into i2c r/w
>       routines.
>    2. fti2c010: serial out r/w address in MSB order: coding style update
>
> Changes for v2:
>    1. cmd_eeprom: bug fix for i2c read/write
>    2. fti2c010: serial out r/w address in MSB order
>
> Kuo-Jung Su (4):
>    i2c: fti2c010: cosmetic: coding style cleanup
>    i2c: fti2c010: migrate to new i2c model
>    i2c: fti2c010: serial out r/w address in MSB order
>    cmd_eeprom: bug fix for i2c read/write

I see only v3 1/4 on the ML. Did you send the patches v3 2-4/4 ?

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

[U-Boot] [PATCH v3 0/4] i2c: fti2c010: bug fix & new driver model

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

This changeset adapts the fti2c010.c to the new i2c driver model.

Changes for v3:
  1. cmd_eeprom: Pass 'addr[0]' instead of 'dev_addr' into i2c r/w
     routines.
  2. fti2c010: serial out r/w address in MSB order: coding style update

Changes for v2:
  1. cmd_eeprom: bug fix for i2c read/write
  2. fti2c010: serial out r/w address in MSB order

Kuo-Jung Su (4):
  i2c: fti2c010: cosmetic: coding style cleanup
  i2c: fti2c010: migrate to new i2c model
  i2c: fti2c010: serial out r/w address in MSB order
  cmd_eeprom: bug fix for i2c read/write

 common/cmd_eeprom.c    |    4 +-
 drivers/i2c/fti2c010.c |  352 +++++++++++++++++++++++-------------------------
 2 files changed, 167 insertions(+), 189 deletions(-)

--
1.7.9.5

[U-Boot] [PATCH v3 1/4] i2c: fti2c010: cosmetic: coding style cleanup

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

Coding style cleanup

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
Cc: Heiko Schocher <hs@denx.de>
---
 Changes for v2 & v3:
  - Nothing updates

 drivers/i2c/fti2c010.c |   31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/fti2c010.c b/drivers/i2c/fti2c010.c
index ddeb941..ec6afc9 100644
--- a/drivers/i2c/fti2c010.c
+++ b/drivers/i2c/fti2c010.c
@@ -18,22 +18,23 @@
 #endif

 #ifndef CONFIG_SYS_I2C_SPEED
-#define CONFIG_SYS_I2C_SPEED    50000
+#define CONFIG_SYS_I2C_SPEED    5000
 #endif

-#ifndef CONFIG_FTI2C010_FREQ
-#define CONFIG_FTI2C010_FREQ    clk_get_rate("I2C")
+#ifndef CONFIG_FTI2C010_CLOCK
+#define CONFIG_FTI2C010_CLOCK   clk_get_rate("I2C")
 #endif

-/* command timeout */
-#define CFG_CMD_TIMEOUT         10 /* ms */
+#ifndef CONFIG_FTI2C010_TIMEOUT
+#define CONFIG_FTI2C010_TIMEOUT 10 /* ms */
+#endif

-/* 7-bit chip address + 1-bit read/write */
-#define I2C_RD(chip)            ((((chip) << 1) & 0xff) | 1)
-#define I2C_WR(chip)            (((chip) << 1) & 0xff)
+/* 7-bit dev address + 1-bit read/write */
+#define I2C_RD(dev)             ((((dev) << 1) & 0xfe) | 1)
+#define I2C_WR(dev)             (((dev) << 1) & 0xfe)

 struct fti2c010_chip {
-	void __iomem *regs;
+	struct fti2c010_regs *regs;
 	uint bus;
 	uint speed;
 };
@@ -41,25 +42,25 @@ struct fti2c010_chip {
 static struct fti2c010_chip chip_list[] = {
 	{
 		.bus  = 0,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE,
 	},
 #ifdef CONFIG_I2C_MULTI_BUS
 # ifdef CONFIG_FTI2C010_BASE1
 	{
 		.bus  = 1,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE1,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE1,
 	},
 # endif
 # ifdef CONFIG_FTI2C010_BASE2
 	{
 		.bus  = 2,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE2,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE2,
 	},
 # endif
 # ifdef CONFIG_FTI2C010_BASE3
 	{
 		.bus  = 3,
-		.regs = (void __iomem *)CONFIG_FTI2C010_BASE3,
+		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE3,
 	},
 # endif
 #endif  /* #ifdef CONFIG_I2C_MULTI_BUS */
@@ -73,7 +74,7 @@ static int fti2c010_wait(uint32_t mask)
 	uint32_t stat, ts;
 	struct fti2c010_regs *regs = curr->regs;

-	for (ts = get_timer(0); get_timer(ts) < CFG_CMD_TIMEOUT; ) {
+	for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) {
 		stat = readl(&regs->sr);
 		if ((stat & mask) == mask) {
 			ret = 0;
@@ -324,7 +325,7 @@ uint i2c_get_bus_num(void)
 int i2c_set_bus_speed(uint speed)
 {
 	struct fti2c010_regs *regs = curr->regs;
-	uint clk = CONFIG_FTI2C010_FREQ;
+	uint clk = CONFIG_FTI2C010_CLOCK;
 	uint gsr = 0, tsr = 32;
 	uint spd, div;

--
1.7.9.5

[U-Boot] [PATCH v3 2/4] i2c: fti2c010: migrate to new i2c model

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

Replace the legacy i2c model with the new one.

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
Cc: Heiko Schocher <hs@denx.de>
---
 Changes for v2 & v3:
  - Nothing updates

 drivers/i2c/fti2c010.c |  299 +++++++++++++++++++++---------------------------
 1 file changed, 133 insertions(+), 166 deletions(-)

diff --git a/drivers/i2c/fti2c010.c b/drivers/i2c/fti2c010.c
index ec6afc9..eccc1da 100644
--- a/drivers/i2c/fti2c010.c
+++ b/drivers/i2c/fti2c010.c
@@ -13,14 +13,14 @@

 #include "fti2c010.h"

-#ifndef CONFIG_HARD_I2C
-#error "fti2c010: CONFIG_HARD_I2C is not defined"
-#endif
-
 #ifndef CONFIG_SYS_I2C_SPEED
 #define CONFIG_SYS_I2C_SPEED    5000
 #endif

+#ifndef CONFIG_SYS_I2C_SLAVE
+#define CONFIG_SYS_I2C_SLAVE    0
+#endif
+
 #ifndef CONFIG_FTI2C010_CLOCK
 #define CONFIG_FTI2C010_CLOCK   clk_get_rate("I2C")
 #endif
@@ -35,44 +35,54 @@

 struct fti2c010_chip {
 	struct fti2c010_regs *regs;
-	uint bus;
-	uint speed;
 };

 static struct fti2c010_chip chip_list[] = {
 	{
-		.bus  = 0,
 		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE,
 	},
-#ifdef CONFIG_I2C_MULTI_BUS
-# ifdef CONFIG_FTI2C010_BASE1
+#ifdef CONFIG_FTI2C010_BASE1
 	{
-		.bus  = 1,
 		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE1,
 	},
-# endif
-# ifdef CONFIG_FTI2C010_BASE2
+#endif
+#ifdef CONFIG_FTI2C010_BASE2
 	{
-		.bus  = 2,
 		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE2,
 	},
-# endif
-# ifdef CONFIG_FTI2C010_BASE3
+#endif
+#ifdef CONFIG_FTI2C010_BASE3
 	{
-		.bus  = 3,
 		.regs = (struct fti2c010_regs *)CONFIG_FTI2C010_BASE3,
 	},
-# endif
-#endif  /* #ifdef CONFIG_I2C_MULTI_BUS */
+#endif
 };

-static struct fti2c010_chip *curr = chip_list;
+static int fti2c010_reset(struct fti2c010_chip *chip)
+{
+	ulong ts;
+	int ret = -1;
+	struct fti2c010_regs *regs = chip->regs;

-static int fti2c010_wait(uint32_t mask)
+	writel(CR_I2CRST, &regs->cr);
+	for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) {
+		if (!(readl(&regs->cr) & CR_I2CRST)) {
+			ret = 0;
+			break;
+		}
+	}
+
+	if (ret)
+		printf("fti2c010: reset timeout\n");
+
+	return ret;
+}
+
+static int fti2c010_wait(struct fti2c010_chip *chip, uint32_t mask)
 {
 	int ret = -1;
 	uint32_t stat, ts;
-	struct fti2c010_regs *regs = curr->regs;
+	struct fti2c010_regs *regs = chip->regs;

 	for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) {
 		stat = readl(&regs->sr);
@@ -85,74 +95,97 @@ static int fti2c010_wait(uint32_t mask)
 	return ret;
 }

-/*
- * u-boot I2C API
- */
+static unsigned int set_i2c_bus_speed(struct fti2c010_chip *chip,
+	unsigned int speed)
+{
+	struct fti2c010_regs *regs = chip->regs;
+	unsigned int clk = CONFIG_FTI2C010_CLOCK;
+	unsigned int gsr = 0;
+	unsigned int tsr = 32;
+	unsigned int div, rate;
+
+	for (div = 0; div < 0x3ffff; ++div) {
+		/* SCLout = PCLK/(2*(COUNT + 2) + GSR) */
+		rate = clk / (2 * (div + 2) + gsr);
+		if (rate <= speed)
+			break;
+	}
+
+	writel(TGSR_GSR(gsr) | TGSR_TSR(tsr), &regs->tgsr);
+	writel(CDR_DIV(div), &regs->cdr);
+
+	return rate;
+}

 /*
  * Initialization, must be called once on start up, may be called
  * repeatedly to change the speed and slave addresses.
  */
-void i2c_init(int speed, int slaveaddr)
+static void fti2c010_init(struct i2c_adapter *adap, int speed, int slaveaddr)
 {
-	if (speed || !curr->speed)
-		i2c_set_bus_speed(speed);
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;

-	/* if slave mode disabled */
-	if (!slaveaddr)
+	if (adap->init_done)
 		return;

-	/*
-	 * TODO:
-	 * Implement slave mode, but is it really necessary?
-	 */
+#ifdef CONFIG_SYS_I2C_INIT_BOARD
+	/* Call board specific i2c bus reset routine before accessing the
+	 * environment, which might be in a chip on that bus. For details
+	 * about this problem see doc/I2C_Edge_Conditions.
+	*/
+	i2c_init_board();
+#endif
+
+	/* master init */
+
+	fti2c010_reset(chip);
+
+	set_i2c_bus_speed(chip, speed);
+
+	/* slave init, don't care */
+
+#ifdef CONFIG_SYS_I2C_BOARD_LATE_INIT
+	/* Call board specific i2c bus reset routine AFTER the bus has been
+	 * initialized. Use either this callpoint or i2c_init_board;
+	 * which is called before fti2c010_init operations.
+	 * For details about this problem see doc/I2C_Edge_Conditions.
+	*/
+	i2c_board_late_init();
+#endif
 }

 /*
  * Probe the given I2C chip address.  Returns 0 if a chip responded,
  * not 0 on failure.
  */
-int i2c_probe(uchar chip)
+static int fti2c010_probe(struct i2c_adapter *adap, u8 dev)
 {
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
+	struct fti2c010_regs *regs = chip->regs;
 	int ret;
-	struct fti2c010_regs *regs = curr->regs;
-
-	i2c_init(0, 0);

 	/* 1. Select slave device (7bits Address + 1bit R/W) */
-	writel(I2C_WR(chip), &regs->dr);
+	writel(I2C_WR(dev), &regs->dr);
 	writel(CR_ENABLE | CR_TBEN | CR_START, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 	if (ret)
 		return ret;

 	/* 2. Select device register */
 	writel(0, &regs->dr);
 	writel(CR_ENABLE | CR_TBEN, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);

 	return ret;
 }

-/*
- * 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(uchar chip, uint addr, int alen, uchar *buf, int len)
+static int fti2c010_read(struct i2c_adapter *adap,
+			u8 dev, uint addr, int alen, uchar *buf, int len)
 {
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
+	struct fti2c010_regs *regs = chip->regs;
 	int ret, pos;
 	uchar paddr[4];
-	struct fti2c010_regs *regs = curr->regs;
-
-	i2c_init(0, 0);

 	paddr[0] = (addr >> 0)  & 0xFF;
 	paddr[1] = (addr >> 8)  & 0xFF;
@@ -164,9 +197,9 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 	 */

 	/* A.1 Select slave device (7bits Address + 1bit R/W) */
-	writel(I2C_WR(chip), &regs->dr);
+	writel(I2C_WR(dev), &regs->dr);
 	writel(CR_ENABLE | CR_TBEN | CR_START, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 	if (ret)
 		return ret;

@@ -176,7 +209,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)

 		writel(paddr[pos], &regs->dr);
 		writel(ctrl, &regs->cr);
-		ret = fti2c010_wait(SR_DT);
+		ret = fti2c010_wait(chip, SR_DT);
 		if (ret)
 			return ret;
 	}
@@ -186,9 +219,9 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 	 */

 	/* B.1 Select slave device (7bits Address + 1bit R/W) */
-	writel(I2C_RD(chip), &regs->dr);
+	writel(I2C_RD(dev), &regs->dr);
 	writel(CR_ENABLE | CR_TBEN | CR_START, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 	if (ret)
 		return ret;

@@ -202,7 +235,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 			stat |= SR_ACK;
 		}
 		writel(ctrl, &regs->cr);
-		ret = fti2c010_wait(stat);
+		ret = fti2c010_wait(chip, stat);
 		if (ret)
 			break;
 		buf[pos] = (uchar)(readl(&regs->dr) & 0xFF);
@@ -211,25 +244,13 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 	return ret;
 }

-/*
- * 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_write(uchar chip, uint addr, int alen, uchar *buf, int len)
+static int fti2c010_write(struct i2c_adapter *adap,
+			u8 dev, uint addr, int alen, u8 *buf, int len)
 {
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
+	struct fti2c010_regs *regs = chip->regs;
 	int ret, pos;
 	uchar paddr[4];
-	struct fti2c010_regs *regs = curr->regs;
-
-	i2c_init(0, 0);

 	paddr[0] = (addr >> 0)  & 0xFF;
 	paddr[1] = (addr >> 8)  & 0xFF;
@@ -241,9 +262,9 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
 	 *
 	 * A.1 Select slave device (7bits Address + 1bit R/W)
 	 */
-	writel(I2C_WR(chip), &regs->dr);
+	writel(I2C_WR(dev), &regs->dr);
 	writel(CR_ENABLE | CR_TBEN | CR_START, &regs->cr);
-	ret = fti2c010_wait(SR_DT);
+	ret = fti2c010_wait(chip, SR_DT);
 	if (ret)
 		return ret;

@@ -253,7 +274,7 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)

 		writel(paddr[pos], &regs->dr);
 		writel(ctrl, &regs->cr);
-		ret = fti2c010_wait(SR_DT);
+		ret = fti2c010_wait(chip, SR_DT);
 		if (ret)
 			return ret;
 	}
@@ -268,7 +289,7 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
 			ctrl |= CR_STOP;
 		writel(buf[pos], &regs->dr);
 		writel(ctrl, &regs->cr);
-		ret = fti2c010_wait(SR_DT);
+		ret = fti2c010_wait(chip, SR_DT);
 		if (ret)
 			break;
 	}
@@ -276,94 +297,40 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
 	return ret;
 }

-/*
- * Functions for setting the current I2C bus and its speed
- */
-#ifdef CONFIG_I2C_MULTI_BUS
-
-/*
- * 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(uint bus)
-{
-	if (bus >= ARRAY_SIZE(chip_list))
-		return -1;
-	curr = chip_list + bus;
-	i2c_init(0, 0);
-	return 0;
-}
-
-/*
- * i2c_get_bus_num:
- *
- *  Returns index of currently active I2C bus.  Zero-based.
- */
-
-uint i2c_get_bus_num(void)
-{
-	return curr->bus;
-}
-
-#endif    /* #ifdef CONFIG_I2C_MULTI_BUS */
-
-/*
- * i2c_set_bus_speed:
- *
- *  Change the speed of the active I2C bus
- *
- *    speed - bus speed in Hz
- *
- *    Returns: 0 on success, not 0 on failure
- */
-int i2c_set_bus_speed(uint speed)
+static unsigned int fti2c010_set_bus_speed(struct i2c_adapter *adap,
+			unsigned int speed)
 {
-	struct fti2c010_regs *regs = curr->regs;
-	uint clk = CONFIG_FTI2C010_CLOCK;
-	uint gsr = 0, tsr = 32;
-	uint spd, div;
-
-	if (!speed)
-		speed = CONFIG_SYS_I2C_SPEED;
-
-	for (div = 0; div < 0x3ffff; ++div) {
-		/* SCLout = PCLK/(2*(COUNT + 2) + GSR) */
-		spd = clk / (2 * (div + 2) + gsr);
-		if (spd <= speed)
-			break;
-	}
-
-	if (curr->speed == spd)
-		return 0;
-
-	writel(CR_I2CRST, &regs->cr);
-	mdelay(100);
-	if (readl(&regs->cr) & CR_I2CRST) {
-		printf("fti2c010: reset timeout\n");
-		return -1;
-	}
-
-	curr->speed = spd;
+	struct fti2c010_chip *chip = chip_list + adap->hwadapnr;
+	int ret;

-	writel(TGSR_GSR(gsr) | TGSR_TSR(tsr), &regs->tgsr);
-	writel(CDR_DIV(div), &regs->cdr);
+	fti2c010_reset(chip);
+	ret = set_i2c_bus_speed(chip, speed);

-	return 0;
+	return ret;
 }

 /*
- * i2c_get_bus_speed:
- *
- *  Returns speed of currently active I2C bus in Hz
+ * Register i2c adapters
  */
-
-uint i2c_get_bus_speed(void)
-{
-	return curr->speed;
-}
+U_BOOT_I2C_ADAP_COMPLETE(i2c_0, fti2c010_init, fti2c010_probe, fti2c010_read,
+			fti2c010_write, fti2c010_set_bus_speed,
+			CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE,
+			0)
+#ifdef CONFIG_FTI2C010_BASE1
+U_BOOT_I2C_ADAP_COMPLETE(i2c_1, fti2c010_init, fti2c010_probe, fti2c010_read,
+			fti2c010_write, fti2c010_set_bus_speed,
+			CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE,
+			1)
+#endif
+#ifdef CONFIG_FTI2C010_BASE2
+U_BOOT_I2C_ADAP_COMPLETE(i2c_2, fti2c010_init, fti2c010_probe, fti2c010_read,
+			fti2c010_write, fti2c010_set_bus_speed,
+			CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE,
+			2)
+#endif
+#ifdef CONFIG_FTI2C010_BASE3
+U_BOOT_I2C_ADAP_COMPLETE(i2c_3, fti2c010_init, fti2c010_probe, fti2c010_read,
+			fti2c010_write, fti2c010_set_bus_speed,
+			CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE,
+			3)
+#endif
--
1.7.9.5

[U-Boot] [PATCH v3 3/4] i2c: fti2c010: serial out r/w address in MSB order

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

For a eeprom with a 2-bytes address (e.g., Ateml AT24C1024B),
the r/w address should be serial out in MSB order.

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
Cc: Heiko Schocher <hs@denx.de>
---
 Changes for v3:
  - Coding style update

 Changes for v2:
  - Initial release

 drivers/i2c/fti2c010.c |   26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/fti2c010.c b/drivers/i2c/fti2c010.c
index eccc1da..fb9fa35 100644
--- a/drivers/i2c/fti2c010.c
+++ b/drivers/i2c/fti2c010.c
@@ -179,6 +179,22 @@ static int fti2c010_probe(struct i2c_adapter *adap, u8 dev)
 	return ret;
 }

+static void to_i2c_addr(u8 *buf, uint32_t addr, int alen)
+{
+	int i, shift;
+
+	if (!buf || alen <= 0)
+		return;
+
+	/* MSB first */
+	i = 0;
+	shift = (alen - 1) * 8;
+	while (alen-- > 0) {
+		buf[i] = (u8)(addr >> shift);
+		shift -= 8;
+	}
+}
+
 static int fti2c010_read(struct i2c_adapter *adap,
 			u8 dev, uint addr, int alen, uchar *buf, int len)
 {
@@ -187,10 +203,7 @@ static int fti2c010_read(struct i2c_adapter *adap,
 	int ret, pos;
 	uchar paddr[4];

-	paddr[0] = (addr >> 0)  & 0xFF;
-	paddr[1] = (addr >> 8)  & 0xFF;
-	paddr[2] = (addr >> 16) & 0xFF;
-	paddr[3] = (addr >> 24) & 0xFF;
+	to_i2c_addr(paddr, addr, alen);

 	/*
 	 * Phase A. Set register address
@@ -252,10 +265,7 @@ static int fti2c010_write(struct i2c_adapter *adap,
 	int ret, pos;
 	uchar paddr[4];

-	paddr[0] = (addr >> 0)  & 0xFF;
-	paddr[1] = (addr >> 8)  & 0xFF;
-	paddr[2] = (addr >> 16) & 0xFF;
-	paddr[3] = (addr >> 24) & 0xFF;
+	to_i2c_addr(paddr, addr, alen);

 	/*
 	 * Phase A. Set register address
--
1.7.9.5

[U-Boot] [PATCH v3 4/4] cmd_eeprom: bug fix for i2c read/write

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

The local pointer of address (i.e., addr) only gets
referenced under SPI mode, and it won't be appropriate
to pass only 1-byte addr[1] to i2c_read/i2c_write while
CONFIG_SYS_I2C_EEPROM_ADDR_LEN > 1.

1. In U-boot's I2C model, the address would be re-assembled
   to a byte string in MSB order inside I2C controller drivers.

2. The 'CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW' option which could
   be found at soft_i2c.c is always turned on in cmd_eeprom.c,
   the addr[0] always contains the device address with overflowed
   MSB address bits.

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
cc: Peter Tyser <ptyser@xes-inc.com>
Cc: Heiko Schocher <hs@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Mischa Jonker <mjonker@synopsys.com>
---
 Changes for v3:
  - It turns out that what we did before 2013-11-13
    (i.e., cmd_eeprom: fix i2c_{read|write} usage if env is in I2C EEPROM)
    is still the best one, this patch simply rollback to it with coding
    style fix.

 Changes for v2:
  - Initial release

 common/cmd_eeprom.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/cmd_eeprom.c b/common/cmd_eeprom.c
index 02539c4..3924805 100644
--- a/common/cmd_eeprom.c
+++ b/common/cmd_eeprom.c
@@ -161,7 +161,7 @@ int eeprom_read (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cnt
 #if defined(CONFIG_SPI) && !defined(CONFIG_ENV_EEPROM_IS_ON_I2C)
 		spi_read (addr, alen, buffer, len);
 #else
-		if (i2c_read (addr[0], addr[1], alen-1, buffer, len) != 0)
+		if (i2c_read(addr[0], offset, alen - 1, buffer, len))
 			rcode = 1;
 #endif
 		buffer += len;
@@ -339,7 +339,7 @@ int eeprom_write (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cn
 		/* Write is enabled ... now write eeprom value.
 		 */
 #endif
-		if (i2c_write (addr[0], addr[1], alen-1, buffer, len) != 0)
+		if (i2c_write(addr[0], offset, alen - 1, buffer, len))
 			rcode = 1;

 #endif
--
1.7.9.5

[U-Boot] [PATCH v3 0/4] i2c: fti2c010: bug fix & new driver model

[Kuo-Jung Su]

Hi Heiko:

Sorry, my 3G network was unstable this morning, I've re-sent this v3
patch series.

Please check it out few minutes later, thank you.

Best Wishes
Dante Su

2013/12/2 Heiko Schocher <hs@denx.de>:
> Hello Kuop-Jung,
>
> Am 02.12.2013 03:57, schrieb Kuo-Jung Su:
>>
>> From: Kuo-Jung Su<dantesu@faraday-tech.com>
>>
>> This changeset adapts the fti2c010.c to the new i2c driver model.
>>
>> Changes for v3:
>>    1. cmd_eeprom: Pass 'addr[0]' instead of 'dev_addr' into i2c r/w
>>       routines.
>>    2. fti2c010: serial out r/w address in MSB order: coding style update
>>
>> Changes for v2:
>>    1. cmd_eeprom: bug fix for i2c read/write
>>    2. fti2c010: serial out r/w address in MSB order
>>
>> Kuo-Jung Su (4):
>>    i2c: fti2c010: cosmetic: coding style cleanup
>>    i2c: fti2c010: migrate to new i2c model
>>    i2c: fti2c010: serial out r/w address in MSB order
>>    cmd_eeprom: bug fix for i2c read/write
>
>
> I see only v3 1/4 on the ML. Did you send the patches v3 2-4/4 ?
>
> bye,
> Heiko
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany



-- 
Best wishes,
Kuo-Jung Su

[U-Boot] [PATCH v3 0/4] i2c: fti2c010: bug fix & new driver model

[Heiko Schocher]

Hello Kuo-Jung,

Am 02.12.2013 09:09, schrieb Kuo-Jung Su:
> Hi Heiko:
>
> Sorry, my 3G network was unstable this morning, I've re-sent this v3
> patch series.
>
> Please check it out few minutes later, thank you.

Your patches have reached the ML now, thanks!

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

[U-Boot] [PATCH v3 4/4] cmd_eeprom: bug fix for i2c read/write

[Alexey Brodkin]

On Mon, 2013-12-02 at 16:02 +0800, Kuo-Jung Su wrote:
> From: Kuo-Jung Su <dantesu@faraday-tech.com>

> diff --git a/common/cmd_eeprom.c b/common/cmd_eeprom.c
> index 02539c4..3924805 100644
> --- a/common/cmd_eeprom.c
> +++ b/common/cmd_eeprom.c
> @@ -161,7 +161,7 @@ int eeprom_read (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cnt
>  #if defined(CONFIG_SPI) && !defined(CONFIG_ENV_EEPROM_IS_ON_I2C)
>  		spi_read (addr, alen, buffer, len);
>  #else
> -		if (i2c_read (addr[0], addr[1], alen-1, buffer, len) != 0)
> +		if (i2c_read(addr[0], offset, alen - 1, buffer, len))
>  			rcode = 1;
>  #endif
>  		buffer += len;

I think this change is whether incomplete or improper.
Let's look at source code above line 161:
=============================
 125 #if CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1 && !defined(CONFIG_SPI_X)
 126                 uchar addr[2];
 127 
 128                 blk_off = offset & 0xFF;        /* block offset */
 129 
 130                 addr[0] = offset >> 8;          /* block number */
 131                 addr[1] = blk_off;              /* block offset */
 132                 alen    = 2;
 133 #else
 134                 uchar addr[3];
 135 
 136                 blk_off = offset & 0xFF;        /* block offset */
 137 
 138                 addr[0] = offset >> 16;         /* block number */
 139                 addr[1] = offset >>  8;         /* upper address
octet */
 140                 addr[2] = blk_off;              /* lower address
octet */
 141                 alen    = 3;
 142 #endif  /* CONFIG_SYS_I2C_EEPROM_ADDR_LEN, CONFIG_SPI_X */
 143 
 144                 addr[0] |= dev_addr;            /* insert device
address */
=============================

[U-Boot] [PATCH v3 4/4] cmd_eeprom: bug fix for i2c read/write

[Kuo-Jung Su]

2013/12/2 Alexey Brodkin <Alexey.Brodkin@synopsys.com>:
> On Mon, 2013-12-02 at 16:02 +0800, Kuo-Jung Su wrote:
>> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>
>> diff --git a/common/cmd_eeprom.c b/common/cmd_eeprom.c
>> index 02539c4..3924805 100644
>> --- a/common/cmd_eeprom.c
>> +++ b/common/cmd_eeprom.c
>> @@ -161,7 +161,7 @@ int eeprom_read (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cnt
>>  #if defined(CONFIG_SPI) && !defined(CONFIG_ENV_EEPROM_IS_ON_I2C)
>>               spi_read (addr, alen, buffer, len);
>>  #else
>> -             if (i2c_read (addr[0], addr[1], alen-1, buffer, len) != 0)
>> +             if (i2c_read(addr[0], offset, alen - 1, buffer, len))
>>                       rcode = 1;
>>  #endif
>>               buffer += len;
>
> I think this change is whether incomplete or improper.
> Let's look at source code above line 161:
> =============================
>  125 #if CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1 && !defined(CONFIG_SPI_X)
>  126                 uchar addr[2];
>  127
>  128                 blk_off = offset & 0xFF;        /* block offset */
>  129
>  130                 addr[0] = offset >> 8;          /* block number */
>  131                 addr[1] = blk_off;              /* block offset */
>  132                 alen    = 2;
>  133 #else
>  134                 uchar addr[3];
>  135
>  136                 blk_off = offset & 0xFF;        /* block offset */
>  137
>  138                 addr[0] = offset >> 16;         /* block number */
>  139                 addr[1] = offset >>  8;         /* upper address
> octet */
>  140                 addr[2] = blk_off;              /* lower address
> octet */
>  141                 alen    = 3;
>  142 #endif  /* CONFIG_SYS_I2C_EEPROM_ADDR_LEN, CONFIG_SPI_X */
>  143
>  144                 addr[0] |= dev_addr;            /* insert device
> address */
> =============================
>
> From these line you see that "addr[0]" is set like this:
> ===========
> If "CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1":
> addr[0] = offset >> 8;
>
> If "CONFIG_SYS_I2C_EEPROM_ADDR_LEN > 1":
> addr[0] = offset >> 16;
>
> And in both cases then OR with "dev_addr":
> addr[0] |= dev_addr;
> ===========
>

This is the reason why I said:

CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW is always enabled inside cmd_eeprom.c

so everything is O.K.

> In other words it gets both real I2S slave address + MSB bits of offset.
> But note that "offset" value stays unchanged.
>

The comment bellow clearly explain the issue here.

soft_i2c.c: line 351 ~ 367:

#ifdef CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW
    /*
     * EEPROM chips that implement "address overflow" are ones
     * like Catalyst 24WC04/08/16 which has 9/10/11 bits of
     * address and the extra bits end up in the "chip address"
     * bit slots. This makes a 24WC08 (1Kbyte) chip look like
     * four 256 byte chips.
     *
     * Note that we consider the length of the address field to
     * still be one byte because the extra address bits are
     * hidden in the chip address.
     */
    chip |= ((addr >> (alen * 8)) & CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW);

    PRINTD("i2c_read: fix addr_overflow: chip %02X addr %02X\n",
        chip, addr);
#endif

> So if you pass both "addr[0]" (which already has MSB bits of "offset")
> and "offset" itself then you'll get completely incorrect I2C command.
>
>> @@ -339,7 +339,7 @@ int eeprom_write (unsigned dev_addr, unsigned offset, uchar *buffer, unsigned cn
>>               /* Write is enabled ... now write eeprom value.
>>                */
>>  #endif
>> -             if (i2c_write (addr[0], addr[1], alen-1, buffer, len) != 0)
>> +             if (i2c_write(addr[0], offset, alen - 1, buffer, len))
>>                       rcode = 1;
>>
>>  #endif
>
> Same goes to "eeprom_write".
>
> Moreover I'd say that this address/offset tricks are very
> EEPROM-specific and because of this we'd better keep it here and don't
> modify generic I2C code.
>

Yes,the address/offset tricks are device specific (not only EEPROM, it
also applies to Audio Codecs..etc.)
But this code was there over a decade. And if everything works just
fine, why bother ?

> Regards,
> Alexey
>



-- 
Best wishes,
Kuo-Jung Su

[U-Boot] [PATCH v3 4/4] cmd_eeprom: bug fix for i2c read/write

[Alexey Brodkin]

On Tue, 2013-12-03 at 08:55 +0800, Kuo-Jung Su wrote:
> The comment bellow clearly explain the issue here.
> 
> soft_i2c.c: line 351 ~ 367:
> 
> #ifdef CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW
>     /*
>      * EEPROM chips that implement "address overflow" are ones
>      * like Catalyst 24WC04/08/16 which has 9/10/11 bits of
>      * address and the extra bits end up in the "chip address"
>      * bit slots. This makes a 24WC08 (1Kbyte) chip look like
>      * four 256 byte chips.
>      *
>      * Note that we consider the length of the address field to
>      * still be one byte because the extra address bits are
>      * hidden in the chip address.
>      */
>     chip |= ((addr >> (alen * 8)) & CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW);
> 
>     PRINTD("i2c_read: fix addr_overflow: chip %02X addr %02X\n",
>         chip, addr);
> #endif

Indeed comment is pretty clear.
But IMHO this code is very generic (how is it bound to any specific
device driver?) and because of this I believe it should be in common I2C
sources but not in device-specific ones.

Otherwise do we need to copy-paste this snippet to all I2C drivers?

I do like the code above for modification of slave address ("chip") -
for me it looks very clear and CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW makes
perfect sense.

So why don't we try to push this in generic "eeprom_{read|write}"?

> Yes,the address/offset tricks are device specific (not only EEPROM, it
> also applies to Audio Codecs..etc.)

Saying "device specific" I meant not "I2C driver specific" but "attached
I2C slave specific".

As you correctly stated this kind of tricky addressing is used by
EEPROMs, audio codecs etc.

So when we need to work with EEPPROM with "ADDR_OVERFLOW" I expect to
enable CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW in configuration.

But imagine if the same I2C bus has another slave which expects "normal"
I2C addressing. So how then you're going to configure your I2C driver so
it correctly works with both slaves?

My vision of resolution is like this: I2C driver always work in "normal
addressing" mode - just uses "chip" and "address" values as they are,
but in higher level code like in ours "cmd_eeprom" we may modify both
"chip" and "offset" values for each particular type of attached I2C
device.

> But this code was there over a decade. And if everything works just
> fine, why bother ?

Well as it turned out not everything worked that fine. As I discovered
"dw_i2c" didn't work because of missing address re-calculation.

Indeed I may agree with your previous patch:
=====
if (i2c_write(dev_addr, offset, alen - 1, buffer, len))
=====
and implement address modification in "dw_i2c" driver.

But still there're questions:
1. Which other drivers will require update? and who's going to check/fix
it there?
2. Why do we need all this address modification part in "cmd_eeprom.c"?
And if we don't need - who's going to clean this up?

BTW what I cannot understand is why "soft_i2c_read" has this "chip"
modification part while "soft_i2c_write" doesn't? Is it done on purpose?
And how it actually works at all then?

Regards,
Alexey

[U-Boot] [U-Boot, v3, 1/4] i2c: fti2c010: cosmetic: coding style cleanup

[Heiko Schocher]

Hello Kuo-jung,

Am 02.12.2013 09:02, schrieb Kuo-Jung Su:
> From: Kuo-Jung Su<dantesu@faraday-tech.com>
>
> Coding style cleanup
>
> Signed-off-by: Kuo-Jung Su<dantesu@faraday-tech.com>
> Cc: Heiko Schocher<hs@denx.de>
>
> ---
> Changes for v2&  v3:
>    - Nothing updates
>
>   drivers/i2c/fti2c010.c |   31 ++++++++++++++++---------------
>   1 file changed, 16 insertions(+), 15 deletions(-)

Applied to u-boot-i2c-git, thanks!

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

[U-Boot] [U-Boot, v3, 2/4] i2c: fti2c010: migrate to new i2c model

[Heiko Schocher]

Hello Kuo-jung,

Am 02.12.2013 09:02, schrieb Kuo-Jung Su:
> From: Kuo-Jung Su<dantesu@faraday-tech.com>
>
> Replace the legacy i2c model with the new one.
>
> Signed-off-by: Kuo-Jung Su<dantesu@faraday-tech.com>
> Cc: Heiko Schocher<hs@denx.de>
>
> ---
> Changes for v2&  v3:
>    - Nothing updates
>
>   drivers/i2c/fti2c010.c |  299 +++++++++++++++++++++---------------------------
>   1 file changed, 133 insertions(+), 166 deletions(-)

Thanks! Applied to u-boot-i2c.git

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

[U-Boot] [U-Boot, v3, 3/4] i2c: fti2c010: serial out r/w address in MSB order

[Heiko Schocher]

Hello Kuo-jung,

Am 02.12.2013 09:02, schrieb Kuo-Jung Su:
> From: Kuo-Jung Su<dantesu@faraday-tech.com>
>
> For a eeprom with a 2-bytes address (e.g., Ateml AT24C1024B),
> the r/w address should be serial out in MSB order.
>
> Signed-off-by: Kuo-Jung Su<dantesu@faraday-tech.com>
> Cc: Heiko Schocher<hs@denx.de>
>
> ---
> Changes for v3:
>    - Coding style update
>
>   Changes for v2:
>    - Initial release
>
>   drivers/i2c/fti2c010.c |   26 ++++++++++++++++++--------
>   1 file changed, 18 insertions(+), 8 deletions(-)

Applied to u-boot-i2c.git, thanks!

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

[U-Boot] [U-Boot, v3, 4/4] cmd_eeprom: bug fix for i2c read/write

[Heiko Schocher]

Hello Kuo-jung,

Am 02.12.2013 09:02, schrieb Kuo-Jung Su:
> From: Kuo-Jung Su<dantesu@faraday-tech.com>
>
> The local pointer of address (i.e., addr) only gets
> referenced under SPI mode, and it won't be appropriate
> to pass only 1-byte addr[1] to i2c_read/i2c_write while
> CONFIG_SYS_I2C_EEPROM_ADDR_LEN>  1.
>
> 1. In U-boot's I2C model, the address would be re-assembled
>     to a byte string in MSB order inside I2C controller drivers.
>
> 2. The 'CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW' option which could
>     be found at soft_i2c.c is always turned on in cmd_eeprom.c,
>     the addr[0] always contains the device address with overflowed
>     MSB address bits.
>
> Signed-off-by: Kuo-Jung Su<dantesu@faraday-tech.com>
> Cc: Alexey Brodkin<abrodkin@synopsys.com>
> Cc: Jean-Christophe PLAGNIOL-VILLARD<plagnioj@jcrosoft.com>
> cc: Peter Tyser<ptyser@xes-inc.com>
> Cc: Heiko Schocher<hs@denx.de>
> Cc: Wolfgang Denk<wd@denx.de>
> Cc: Stefan Roese<sr@denx.de>
> Cc: Mischa Jonker<mjonker@synopsys.com>
>
> ---
> Changes for v3:
>    - It turns out that what we did before 2013-11-13
>      (i.e., cmd_eeprom: fix i2c_{read|write} usage if env is in I2C EEPROM)
>      is still the best one, this patch simply rollback to it with coding
>      style fix.
>
>   Changes for v2:
>    - Initial release
>
>   common/cmd_eeprom.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Applied to u-boot.i2c.git, thanks!

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

[U-Boot] [U-Boot, v3, 4/4] cmd_eeprom: bug fix for i2c read/write

[Alexey Brodkin]

Hi Heiko,

On Mon, 2013-12-09 at 07:56 +0100, Heiko Schocher wrote:

> Applied to u-boot.i2c.git, thanks!
> 

I'm wondering if you've seen a discussion between me and Kuo-jung
regarding this patch and consequences of it being applied.

Do you mind to comment on questions we discussed there?

My main concern is how to keep underlying I2C drivers working because as
I wrote some of them will not function any more correctly.

Regards,
Alexey

[U-Boot] [U-Boot, v3, 4/4] cmd_eeprom: bug fix for i2c read/write

[Heiko Schocher]

Hello Alexey,

Am 09.12.2013 11:35, schrieb Alexey Brodkin:
> Hi Heiko,
>
> On Mon, 2013-12-09 at 07:56 +0100, Heiko Schocher wrote:
>
>> Applied to u-boot.i2c.git, thanks!
>>
>
> I'm wondering if you've seen a discussion between me and Kuo-jung
> regarding this patch and consequences of it being applied.
>
> Do you mind to comment on questions we discussed there?

Sorry, seems I missed this ...

> My main concern is how to keep underlying I2C drivers working because as
> I wrote some of them will not function any more correctly.

I thought the v3 patch just rolls things back as patch comment states:
------------------------
  Changes for v3:
   - It turns out that what we did before 2013-11-13
     (i.e., cmd_eeprom: fix i2c_{read|write} usage if env is in I2C EEPROM)
     is still the best one, this patch simply rollback to it with coding
     style fix.
------------------------

Without this roll back, I think, some boards are broken now in current
mainline... so first we should go this step back.

Next step, drivers who do not work, should be fixed if needed, or:

You wrote:
on Alexey Brodkin - Dec. 3, 2013, 7:42 a.m. wrote:
 > On Tue, 2013-12-03 at 08:55 +0800, Kuo-Jung Su wrote:
 >>  The comment bellow clearly explain the issue here.
 > [...]
 >>  But this code was there over a decade. And if everything works just
 >>  fine, why bother ?
 >
 > Well as it turned out not everything worked that fine. As I discovered
 > "dw_i2c" didn't work because of missing address re-calculation.
 >
 > Indeed I may agree with your previous patch:
 > =====
 > if (i2c_write(dev_addr, offset, alen - 1, buffer, len))
 > =====
 > and implement address modification in "dw_i2c" driver.

Hmm.. this is real ancient code ... seems, until now, nobody had problems
with it ... if you see problems now in dw_i2c driver, you have two
possibilites:

- fix the dw_i2c driver

- The best way would really be to rework this, as you stated:
   "My vision of resolution is like this: I2C driver always work in "normal
   addressing" mode - just uses "chip" and "address" values as they are,
   but in higher level code like in ours "cmd_eeprom" we may modify both
   "chip" and "offset" values for each particular type of attached I2C
   device."

   This touches I think a lot of boards ... the problem would be, how to
   test this change...

Nevertheless, this should be done in a new thread ...

 > But still there're questions:
 > 1. Which other drivers will require update? and who's going to check/fix
 > it there?

Yes thats the problem. For the "old state" this should be done, if new
boards need it and fix the i2c driver.

If you want to clean up this, as you mentioned above, this touches maybe
a lot of current i2c drivers (drivers who do this address modification)

So, this must be done carefully, and you/we need some board maintainers
which can test it on their boards.

 > 2. Why do we need all this address modification part in "cmd_eeprom.c"?
 > And if we don't need - who's going to clean this up?

The first who have issues with it? Or you volunteer to send a cleanup
patch?

 > BTW what I cannot understand is why "soft_i2c_read" has this "chip"
 > modification part while "soft_i2c_write" doesn't? Is it done on purpose?
 > And how it actually works at all then?

Good question ... maybe currently only used on i2c reads ?

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

[U-Boot] [U-Boot, v3, 4/4] cmd_eeprom: bug fix for i2c read/write

[Alexey Brodkin]

On Mon, 2013-12-09 at 12:21 +0100, Heiko Schocher wrote:
> I thought the v3 patch just rolls things back as patch comment states:
> ------------------------
>   Changes for v3:
>    - It turns out that what we did before 2013-11-13
>      (i.e., cmd_eeprom: fix i2c_{read|write} usage if env is in I2C EEPROM)
>      is still the best one, this patch simply rollback to it with coding
>      style fix.
> ------------------------
> 
> Without this roll back, I think, some boards are broken now in current
> mainline... so first we should go this step back.

I'd say it's very questionable. For example if you look at I2C driver
that Kuo-Jung refers to ("soft_i2c") you'll find that with my patch
applied (the one you've just rolled back) it will work flawlessly
because:
1. "chip" address there has double applied MSB bits of offset (the first
time it gets applied in "cmd_eeprom")
2. Only LSB byte of offset gets written in I2C device if addr_len = 1.

This one makes IMHO much more sense because it excludes an extra "chip"
address modification - so it's clear that it will be done by particular
I2C driver so people won't be confused with their expectations.

>  > BTW what I cannot understand is why "soft_i2c_read" has this "chip"
>  > modification part while "soft_i2c_write" doesn't? Is it done on purpose?
>  > And how it actually works at all then?
> 
> Good question ... maybe currently only used on i2c reads ?

Frankly I have only 1 supposition regarding this strange state of things
in "soft_i2c_write". Because without any doubts the same modification of
"chip" address is applicable to both "read" and "write" because it is an
integral part of data addressing.

But due to discussed a lot in this thread "double chip address
modification" (application of MSB bits of offset) proper support of
"chip" address was never implemented in "soft_i2c_write". As you
correctly mentioned - "real ancient code" works and nobody has problems
with it. Well, just because we have current implementation of
"eeprom_write" that does "chip" address modification "soft_i2c" may not
have this feature in both "read" and "write".

So I'd prefer to go with previous version of patch sent by Kuo-Jung
http://patchwork.ozlabs.org/patch/294733/

And indeed this will "break" functionality of currently incomplete
implementation of "soft_i2c_write".

Regards,
Alexey

[U-Boot] [U-Boot, v3, 4/4] cmd_eeprom: bug fix for i2c read/write

[Kuo-Jung Su]

2013/12/10 Alexey Brodkin <Alexey.Brodkin@synopsys.com>:
> On Mon, 2013-12-09 at 12:21 +0100, Heiko Schocher wrote:
>> I thought the v3 patch just rolls things back as patch comment states:
>> ------------------------
>>   Changes for v3:
>>    - It turns out that what we did before 2013-11-13
>>      (i.e., cmd_eeprom: fix i2c_{read|write} usage if env is in I2C EEPROM)
>>      is still the best one, this patch simply rollback to it with coding
>>      style fix.
>> ------------------------
>>
>> Without this roll back, I think, some boards are broken now in current
>> mainline... so first we should go this step back.
>
> I'd say it's very questionable. For example if you look at I2C driver
> that Kuo-Jung refers to ("soft_i2c") you'll find that with my patch
> applied (the one you've just rolled back) it will work flawlessly
> because:
> 1. "chip" address there has double applied MSB bits of offset (the first
> time it gets applied in "cmd_eeprom")
> 2. Only LSB byte of offset gets written in I2C device if addr_len = 1.

But if addr_len = 2, everything goes wrong. This is the real problem here...

>
> This one makes IMHO much more sense because it excludes an extra "chip"
> address modification - so it's clear that it will be done by particular
> I2C driver so people won't be confused with their expectations.
>
>>  > BTW what I cannot understand is why "soft_i2c_read" has this "chip"
>>  > modification part while "soft_i2c_write" doesn't? Is it done on purpose?
>>  > And how it actually works at all then?
>>
>> Good question ... maybe currently only used on i2c reads ?
>
> Frankly I have only 1 supposition regarding this strange state of things
> in "soft_i2c_write". Because without any doubts the same modification of
> "chip" address is applicable to both "read" and "write" because it is an
> integral part of data addressing.
>
> But due to discussed a lot in this thread "double chip address
> modification" (application of MSB bits of offset) proper support of
> "chip" address was never implemented in "soft_i2c_write". As you
> correctly mentioned - "real ancient code" works and nobody has problems
> with it. Well, just because we have current implementation of
> "eeprom_write" that does "chip" address modification "soft_i2c" may not
> have this feature in both "read" and "write".
>
> So I'd prefer to go with previous version of patch sent by Kuo-Jung
> http://patchwork.ozlabs.org/patch/294733/
>
> And indeed this will "break" functionality of currently incomplete
> implementation of "soft_i2c_write".
>

No, it will works just fine.

1. eeprom_read:
The overflowed address was shifted to chip address by cmd_eeprom.c, and then
the same operations would be repeated by soft_i2c.c, but it's no harm
to do a OR operation twice
on the same bits, so everything is all right.

2. eeprom_write:
The overflowed address was shifted to chip address by cmd_eeprom.c,
and get passed to soft_i2c.c
without further modification. So everything is all right.

The 'CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW' in soft_i2c.c is actually a
redundant code snippet.
And it would be better to move/copied its comment to cmd_eeprom.c.

Which means if we don't rollback, none of the EEPROMs with addr_len >
1 will work on u-boot.
And this rollback actually does no harm to the EEPROMs with addr_len = 1 or 2.

P.S: The designware_i2c.c should work just fine with this rollback, too....

-- 
Best wishes,
Kuo-Jung Su