[U-Boot] [PATCH 0/2] i2c: fti2c010: migrate to new i2c model
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
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(®s->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
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, ®s->cr); + for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) { + if (!(readl(®s->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(®s->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), ®s->tgsr); + writel(CDR_DIV(div), ®s->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), ®s->dr); + writel(I2C_WR(dev), ®s->dr); writel(CR_ENABLE | CR_TBEN | CR_START, ®s->cr); - ret = fti2c010_wait(SR_DT); + ret = fti2c010_wait(chip, SR_DT); if (ret) return ret;
/* 2. Select device register */ writel(0, ®s->dr); writel(CR_ENABLE | CR_TBEN, ®s->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), ®s->dr); + writel(I2C_WR(dev), ®s->dr); writel(CR_ENABLE | CR_TBEN | CR_START, ®s->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], ®s->dr); writel(ctrl, ®s->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), ®s->dr); + writel(I2C_RD(dev), ®s->dr); writel(CR_ENABLE | CR_TBEN | CR_START, ®s->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, ®s->cr); - ret = fti2c010_wait(stat); + ret = fti2c010_wait(chip, stat); if (ret) break; buf[pos] = (uchar)(readl(®s->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), ®s->dr); + writel(I2C_WR(dev), ®s->dr); writel(CR_ENABLE | CR_TBEN | CR_START, ®s->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], ®s->dr); writel(ctrl, ®s->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], ®s->dr); writel(ctrl, ®s->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, ®s->cr); - mdelay(100); - if (readl(®s->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), ®s->tgsr); - writel(CDR_DIV(div), ®s->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
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
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(®s->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
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, ®s->cr); + for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) { + if (!(readl(®s->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(®s->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), ®s->tgsr); + writel(CDR_DIV(div), ®s->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), ®s->dr); + writel(I2C_WR(dev), ®s->dr); writel(CR_ENABLE | CR_TBEN | CR_START, ®s->cr); - ret = fti2c010_wait(SR_DT); + ret = fti2c010_wait(chip, SR_DT); if (ret) return ret;
/* 2. Select device register */ writel(0, ®s->dr); writel(CR_ENABLE | CR_TBEN, ®s->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), ®s->dr); + writel(I2C_WR(dev), ®s->dr); writel(CR_ENABLE | CR_TBEN | CR_START, ®s->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], ®s->dr); writel(ctrl, ®s->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), ®s->dr); + writel(I2C_RD(dev), ®s->dr); writel(CR_ENABLE | CR_TBEN | CR_START, ®s->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, ®s->cr); - ret = fti2c010_wait(stat); + ret = fti2c010_wait(chip, stat); if (ret) break; buf[pos] = (uchar)(readl(®s->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), ®s->dr); + writel(I2C_WR(dev), ®s->dr); writel(CR_ENABLE | CR_TBEN | CR_START, ®s->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], ®s->dr); writel(ctrl, ®s->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], ®s->dr); writel(ctrl, ®s->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, ®s->cr); - mdelay(100); - if (readl(®s->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), ®s->tgsr); - writel(CDR_DIV(div), ®s->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
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
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
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
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}":
- for "CONFIG_SYS_I2C_EEPROM_ADDR_LEN = 1"
- 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
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 */ ==================
From source code above you see that virtually it addresses multiple 256
byte EEPROMs.
Regards, Alexey
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
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
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=cb2...
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=396fea89a...
Best Wishes Dante Su
Regards, Alexey
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
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(®s->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
Hello Kuop-Jung,
Am 02.12.2013 03:57, schrieb Kuo-Jung Su:
From: Kuo-Jung Sudantesu@faraday-tech.com
This changeset adapts the fti2c010.c to the new i2c driver model.
Changes for v3:
- cmd_eeprom: Pass 'addr[0]' instead of 'dev_addr' into i2c r/w routines.
- fti2c010: serial out r/w address in MSB order: coding style update
Changes for v2:
- cmd_eeprom: bug fix for i2c read/write
- 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
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 Sudantesu@faraday-tech.com
This changeset adapts the fti2c010.c to the new i2c driver model.
Changes for v3:
- cmd_eeprom: Pass 'addr[0]' instead of 'dev_addr' into i2c r/w routines.
- fti2c010: serial out r/w address in MSB order: coding style update
Changes for v2:
- cmd_eeprom: bug fix for i2c read/write
- 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
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
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
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(®s->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
Hello Kuo-jung,
Am 02.12.2013 09:02, schrieb Kuo-Jung Su:
From: Kuo-Jung Sudantesu@faraday-tech.com
Coding style cleanup
Signed-off-by: Kuo-Jung Sudantesu@faraday-tech.com Cc: Heiko Schocherhs@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
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, ®s->cr); + for (ts = get_timer(0); get_timer(ts) < CONFIG_FTI2C010_TIMEOUT; ) { + if (!(readl(®s->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(®s->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), ®s->tgsr); + writel(CDR_DIV(div), ®s->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), ®s->dr); + writel(I2C_WR(dev), ®s->dr); writel(CR_ENABLE | CR_TBEN | CR_START, ®s->cr); - ret = fti2c010_wait(SR_DT); + ret = fti2c010_wait(chip, SR_DT); if (ret) return ret;
/* 2. Select device register */ writel(0, ®s->dr); writel(CR_ENABLE | CR_TBEN, ®s->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), ®s->dr); + writel(I2C_WR(dev), ®s->dr); writel(CR_ENABLE | CR_TBEN | CR_START, ®s->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], ®s->dr); writel(ctrl, ®s->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), ®s->dr); + writel(I2C_RD(dev), ®s->dr); writel(CR_ENABLE | CR_TBEN | CR_START, ®s->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, ®s->cr); - ret = fti2c010_wait(stat); + ret = fti2c010_wait(chip, stat); if (ret) break; buf[pos] = (uchar)(readl(®s->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), ®s->dr); + writel(I2C_WR(dev), ®s->dr); writel(CR_ENABLE | CR_TBEN | CR_START, ®s->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], ®s->dr); writel(ctrl, ®s->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], ®s->dr); writel(ctrl, ®s->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, ®s->cr); - mdelay(100); - if (readl(®s->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), ®s->tgsr); - writel(CDR_DIV(div), ®s->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
Hello Kuo-jung,
Am 02.12.2013 09:02, schrieb Kuo-Jung Su:
From: Kuo-Jung Sudantesu@faraday-tech.com
Replace the legacy i2c model with the new one.
Signed-off-by: Kuo-Jung Sudantesu@faraday-tech.com Cc: Heiko Schocherhs@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
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
Hello Kuo-jung,
Am 02.12.2013 09:02, schrieb Kuo-Jung Su:
From: Kuo-Jung Sudantesu@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 Sudantesu@faraday-tech.com Cc: Heiko Schocherhs@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
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
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; ===========
In other words it gets both real I2S slave address + MSB bits of offset. But note that "offset" value stays unchanged.
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.
Regards, Alexey
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
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
Hello Kuo-jung,
Am 02.12.2013 09:02, schrieb Kuo-Jung Su:
From: Kuo-Jung Sudantesu@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.
In U-boot's I2C model, the address would be re-assembled to a byte string in MSB order inside I2C controller drivers.
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 Sudantesu@faraday-tech.com Cc: Alexey Brodkinabrodkin@synopsys.com Cc: Jean-Christophe PLAGNIOL-VILLARDplagnioj@jcrosoft.com cc: Peter Tyserptyser@xes-inc.com Cc: Heiko Schocherhs@denx.de Cc: Wolfgang Denkwd@denx.de Cc: Stefan Roesesr@denx.de Cc: Mischa Jonkermjonker@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
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
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:
- 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.
- 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
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
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:
- "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....
participants (3)
-
Alexey Brodkin -
Heiko Schocher -
Kuo-Jung Su