[U-Boot] [PATCH V3+] I2C: mxc_i2c rework
Rewrite the mxc_i2c driver. * This version is much closer to Linux implementation. * Fixes IPG_PERCLK being incorrectly used as clock source * Fixes behaviour of the driver on iMX51 * Clean up coding style a bit ;-)
Based on commit: e39428d53d080ad2615b772d7f99d2a70c2aaab2 Date: Mon Jun 21 09:27:05 2010 +0200 i2c-imx: do not allow interruptions when waiting for I2C to complete
Signed-off-by: Marek Vasut marek.vasut@gmail.com --- drivers/i2c/mxc_i2c.c | 424 +++++++++++++++++++++++++++++++++---------------- 1 files changed, 290 insertions(+), 134 deletions(-)
V2: Convert register access to struct mxc_i2c_regs.
V3: Update licensing info
V3+: Add commit ID into commit message
diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c index 89d1973..03e2448 100644 --- a/drivers/i2c/mxc_i2c.c +++ b/drivers/i2c/mxc_i2c.c @@ -1,7 +1,15 @@ /* - * i2c driver for Freescale mx31 + * i2c driver for Freescale i.MX series * * (c) 2007 Pengutronix, Sascha Hauer s.hauer@pengutronix.de + * (c) 2011 Marek Vasut marek.vasut@gmail.com + * + * Based on i2c-imx.c from linux kernel: + * Copyright (C) 2005 Torsten Koschorrek <koschorrek at synertronixx.de> + * Copyright (C) 2005 Matthias Blaschke <blaschke at synertronixx.de> + * Copyright (C) 2007 RightHand Technologies, Inc. + * Copyright (C) 2008 Darius Augulis <darius.augulis at teltonika.lt> + * * * See file CREDITS for list of people who contributed to this * project. @@ -30,11 +38,13 @@ #include <asm/arch/clock.h> #include <asm/arch/imx-regs.h>
-#define IADR 0x00 -#define IFDR 0x04 -#define I2CR 0x08 -#define I2SR 0x0c -#define I2DR 0x10 +struct mxc_i2c_regs { + uint32_t iadr; + uint32_t ifdr; + uint32_t i2cr; + uint32_t i2sr; + uint32_t i2dr; +};
#define I2CR_IEN (1 << 7) #define I2CR_IIEN (1 << 6) @@ -68,218 +78,364 @@ #endif
#define I2C_MAX_TIMEOUT 10000 -#define I2C_MAX_RETRIES 3
-static u16 div[] = { 30, 32, 36, 42, 48, 52, 60, 72, 80, 88, 104, 128, 144, - 160, 192, 240, 288, 320, 384, 480, 576, 640, 768, 960, - 1152, 1280, 1536, 1920, 2304, 2560, 3072, 3840}; +static u16 i2c_clk_div[50][2] = { + { 22, 0x20 }, { 24, 0x21 }, { 26, 0x22 }, { 28, 0x23 }, + { 30, 0x00 }, { 32, 0x24 }, { 36, 0x25 }, { 40, 0x26 }, + { 42, 0x03 }, { 44, 0x27 }, { 48, 0x28 }, { 52, 0x05 }, + { 56, 0x29 }, { 60, 0x06 }, { 64, 0x2A }, { 72, 0x2B }, + { 80, 0x2C }, { 88, 0x09 }, { 96, 0x2D }, { 104, 0x0A }, + { 112, 0x2E }, { 128, 0x2F }, { 144, 0x0C }, { 160, 0x30 }, + { 192, 0x31 }, { 224, 0x32 }, { 240, 0x0F }, { 256, 0x33 }, + { 288, 0x10 }, { 320, 0x34 }, { 384, 0x35 }, { 448, 0x36 }, + { 480, 0x13 }, { 512, 0x37 }, { 576, 0x14 }, { 640, 0x38 }, + { 768, 0x39 }, { 896, 0x3A }, { 960, 0x17 }, { 1024, 0x3B }, + { 1152, 0x18 }, { 1280, 0x3C }, { 1536, 0x3D }, { 1792, 0x3E }, + { 1920, 0x1B }, { 2048, 0x3F }, { 2304, 0x1C }, { 2560, 0x1D }, + { 3072, 0x1E }, { 3840, 0x1F } +}; + +static u8 clk_idx;
-static inline void i2c_reset(void) -{ - writew(0, I2C_BASE + I2CR); /* Reset module */ - writew(0, I2C_BASE + I2SR); - writew(I2CR_IEN, I2C_BASE + I2CR); -} - -void i2c_init(int speed, int unused) +/* + * Calculate and set proper clock divider + * + * FIXME: remove #ifdefs ! + */ +static void i2c_imx_set_clk(unsigned int rate) { - int freq; + struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE; + unsigned int i2c_clk_rate; + unsigned int div; int i;
+ /* Divider value calculation */ #if defined(CONFIG_MX31) struct clock_control_regs *sc_regs = (struct clock_control_regs *)CCM_BASE;
- freq = mx31_get_ipg_clk(); + i2c_clk_rate = mx31_get_ipg_clk(); /* start the required I2C clock */ writel(readl(&sc_regs->cgr0) | (3 << I2C_CLK_OFFSET), &sc_regs->cgr0); #else - freq = mxc_get_clock(MXC_IPG_PERCLK); + i2c_clk_rate = mxc_get_clock(MXC_IPG_CLK); #endif + div = (i2c_clk_rate + rate - 1) / rate; + if (div < i2c_clk_div[0][0]) + i = 0; + else if (div > i2c_clk_div[ARRAY_SIZE(i2c_clk_div) - 1][0]) + i = ARRAY_SIZE(i2c_clk_div) - 1; + else + for (i = 0; i2c_clk_div[i][0] < div; i++); + + /* Store divider value */ + writeb(div, &i2c_regs->ifdr); + clk_idx = div; +}
- for (i = 0; i < 0x1f; i++) - if (freq / div[i] <= speed) - break; +/* + * Reset I2C Controller + */ +void i2c_reset(void) +{ + struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
- debug("%s: speed: %d\n", __func__, speed); + writeb(0, &i2c_regs->i2cr); /* Reset module */ + writeb(0, &i2c_regs->i2sr); +}
- writew(i, I2C_BASE + IFDR); +/* + * Init I2C Bus + */ +void i2c_init(int speed, int unused) +{ + i2c_imx_set_clk(speed); i2c_reset(); }
-static int wait_idle(void) +/* + * Wait for bus to be busy (or free if for_busy = 0) + * + * for_busy = 1: Wait for IBB to be asserted + * for_busy = 0: Wait for IBB to be de-asserted + */ +int i2c_imx_bus_busy(int for_busy) { + struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE; + unsigned int temp; + int timeout = I2C_MAX_TIMEOUT;
- while ((readw(I2C_BASE + I2SR) & I2SR_IBB) && --timeout) { - writew(0, I2C_BASE + I2SR); + while (timeout--) { + temp = readb(&i2c_regs->i2sr); + + if (for_busy && (temp & I2SR_IBB)) + return 0; + if (!for_busy && !(temp & I2SR_IBB)) + return 0; + udelay(1); } - return timeout ? timeout : (!(readw(I2C_BASE + I2SR) & I2SR_IBB)); + + return 1; }
-static int wait_busy(void) +/* + * Wait for transaction to complete + */ +int i2c_imx_trx_complete(void) { + struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE; int timeout = I2C_MAX_TIMEOUT;
- while (!(readw(I2C_BASE + I2SR) & I2SR_IBB) && --timeout) + while (timeout--) { + if (readb(&i2c_regs->i2sr) & I2SR_IIF) { + writeb(0, &i2c_regs->i2sr); + return 0; + } + udelay(1); - writew(0, I2C_BASE + I2SR); /* clear interrupt */ + }
- return timeout; + return 1; }
-static int wait_complete(void) +/* + * Check if the transaction was ACKed + */ +int i2c_imx_acked(void) { - int timeout = I2C_MAX_TIMEOUT; + struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
- while ((!(readw(I2C_BASE + I2SR) & I2SR_ICF)) && (--timeout)) { - writew(0, I2C_BASE + I2SR); - udelay(1); - } - udelay(200); + return readb(&i2c_regs->i2sr) & I2SR_RX_NO_AK; +}
- writew(0, I2C_BASE + I2SR); /* clear interrupt */ +/* + * Start the controller + */ +int i2c_imx_start(void) +{ + struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE; + unsigned int temp = 0; + int result;
- return timeout; -} + writeb(clk_idx, &i2c_regs->ifdr);
+ /* Enable I2C controller */ + writeb(0, &i2c_regs->i2sr); + writeb(I2CR_IEN, &i2c_regs->i2cr);
-static int tx_byte(u8 byte) -{ - writew(byte, I2C_BASE + I2DR); + /* Wait controller to be stable */ + udelay(50); + + /* Start I2C transaction */ + temp = readb(&i2c_regs->i2cr); + temp |= I2CR_MSTA; + writeb(temp, &i2c_regs->i2cr); + + result = i2c_imx_bus_busy(1); + if (result) + return result; + + temp |= I2CR_MTX | I2CR_TX_NO_AK; + writeb(temp, &i2c_regs->i2cr);
- if (!wait_complete() || readw(I2C_BASE + I2SR) & I2SR_RX_NO_AK) - return -1; return 0; }
-static int rx_byte(int last) +/* + * Stop the controller + */ +void i2c_imx_stop() { - if (!wait_complete()) - return -1; + struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE; + unsigned int temp = 0; + + /* Stop I2C transaction */ + temp = readb(&i2c_regs->i2cr); + temp |= ~(I2CR_MSTA | I2CR_MTX); + writeb(temp, &i2c_regs->i2cr);
- if (last) - writew(I2CR_IEN, I2C_BASE + I2CR); + i2c_imx_bus_busy(0);
- return readw(I2C_BASE + I2DR); + /* Disable I2C controller */ + writeb(0, &i2c_regs->i2cr); }
-int i2c_probe(uchar chip) +/* + * Set chip address and access mode + * + * read = 1: READ access + * read = 0: WRITE access + */ +int i2c_imx_set_chip_addr(uchar chip, int read) { + struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE; int ret;
- writew(0, I2C_BASE + I2CR); /* Reset module */ - writew(I2CR_IEN, I2C_BASE + I2CR); + writeb((chip << 1) | read, &i2c_regs->i2dr); + + ret = i2c_imx_trx_complete(); + if (ret) + return ret;
- writew(I2CR_IEN | I2CR_MSTA | I2CR_MTX, I2C_BASE + I2CR); - ret = tx_byte(chip << 1); - writew(I2CR_IEN | I2CR_MTX, I2C_BASE + I2CR); + ret = i2c_imx_acked(); + if (ret) + return ret;
return ret; }
-static int i2c_addr(uchar chip, uint addr, int alen) +/* + * Write register address + */ +int i2c_imx_set_reg_addr(uint addr, int alen) { - int i, retry = 0; - for (retry = 0; retry < 3; retry++) { - if (wait_idle()) + struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE; + int ret; + int i; + + for (i = 0; i < (8 * alen); i += 8) { + writeb((addr >> i) & 0xff, &i2c_regs->i2dr); + + ret = i2c_imx_trx_complete(); + if (ret) break; - i2c_reset(); - for (i = 0; i < I2C_MAX_TIMEOUT; i++) - udelay(1); - } - if (retry >= I2C_MAX_RETRIES) { - debug("%s:bus is busy(%x)\n", - __func__, readw(I2C_BASE + I2SR)); - return -1; - } - writew(I2CR_IEN | I2CR_MSTA | I2CR_MTX, I2C_BASE + I2CR);
- if (!wait_busy()) { - debug("%s:trigger start fail(%x)\n", - __func__, readw(I2C_BASE + I2SR)); - return -1; + ret = i2c_imx_acked(); + if (ret) + break; }
- if (tx_byte(chip << 1) || (readw(I2C_BASE + I2SR) & I2SR_RX_NO_AK)) { - debug("%s:chip address cycle fail(%x)\n", - __func__, readw(I2C_BASE + I2SR)); - return -1; - } - while (alen--) - if (tx_byte((addr >> (alen * 8)) & 0xff) || - (readw(I2C_BASE + I2SR) & I2SR_RX_NO_AK)) { - debug("%s:device address cycle fail(%x)\n", - __func__, readw(I2C_BASE + I2SR)); - return -1; - } - return 0; + return ret; }
-int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len) +/* + * Try if a chip add given address responds (probe the chip) + */ +int i2c_probe(uchar chip) { - int timeout = I2C_MAX_TIMEOUT; int ret;
- debug("%s chip: 0x%02x addr: 0x%04x alen: %d len: %d\n", - __func__, chip, addr, alen, len); + ret = i2c_imx_start(); + if (ret) + return ret;
- if (i2c_addr(chip, addr, alen)) { - printf("i2c_addr failed\n"); - return -1; - } + ret = i2c_imx_set_chip_addr(chip, 0); + if (ret) + return ret;
- writew(I2CR_IEN | I2CR_MSTA | I2CR_MTX | I2CR_RSTA, I2C_BASE + I2CR); + i2c_imx_stop();
- if (tx_byte(chip << 1 | 1)) - return -1; + return ret; +}
- writew(I2CR_IEN | I2CR_MSTA | - ((len == 1) ? I2CR_TX_NO_AK : 0), - I2C_BASE + I2CR); +/* + * Read data from I2C device + */ +int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len) +{ + struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE; + int ret; + unsigned int temp; + int i;
- ret = readw(I2C_BASE + I2DR); + ret = i2c_imx_start(); + if (ret) + return ret; + + /* write slave address */ + ret = i2c_imx_set_chip_addr(chip, 0); + if (ret) + return ret; + + ret = i2c_imx_set_reg_addr(addr, alen); + if (ret) + return ret; + + temp = readb(&i2c_regs->i2cr); + temp |= I2CR_RSTA; + writeb(temp, &i2c_regs->i2cr); + + ret = i2c_imx_set_chip_addr(chip, 1); + if (ret) + return ret; + + /* setup bus to read data */ + temp = readb(&i2c_regs->i2cr); + temp &= ~I2CR_MTX; + if (len == 1) + temp &= ~I2CR_TX_NO_AK; + writeb(temp, &i2c_regs->i2cr); + readb(&i2c_regs->i2dr); + + /* read data */ + for (i = 0; i < len; i++) { + ret = i2c_imx_trx_complete(); + if (ret) + return ret; + + /* + * It must generate STOP before read I2DR to prevent + * controller from generating another clock cycle + */ + if (i == (len - 1)) { + temp = readb(&i2c_regs->i2cr); + temp &= ~(I2CR_MSTA | I2CR_MTX); + writeb(temp, &i2c_regs->i2cr); + i2c_imx_bus_busy(0); + } else if (i == (len - 2)) { + temp = readb(&i2c_regs->i2cr); + temp |= I2CR_TX_NO_AK; + writeb(temp, &i2c_regs->i2cr); + }
- while (len--) { - ret = rx_byte(len == 0); - if (ret < 0) - return -1; - *buf++ = ret; - if (len <= 1) - writew(I2CR_IEN | I2CR_MSTA | - I2CR_TX_NO_AK, - I2C_BASE + I2CR); + buf[i] = readb(&i2c_regs->i2dr); }
- writew(I2CR_IEN, I2C_BASE + I2CR); - - while (readw(I2C_BASE + I2SR) & I2SR_IBB && --timeout) - udelay(1); + i2c_imx_stop();
- return 0; + return ret; }
+/* + * Write data to I2C device + */ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len) { - int timeout = I2C_MAX_TIMEOUT; - debug("%s chip: 0x%02x addr: 0x%04x alen: %d len: %d\n", - __func__, chip, addr, alen, len); + struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE; + int ret; + unsigned int temp; + int i;
- if (i2c_addr(chip, addr, alen)) - return -1; + ret = i2c_imx_start(); + if (ret) + return ret;
- while (len--) - if (tx_byte(*buf++)) - return -1; + /* write slave address */ + ret = i2c_imx_set_chip_addr(chip, 0); + if (ret) + return ret;
- writew(I2CR_IEN, I2C_BASE + I2CR); + ret = i2c_imx_set_reg_addr(addr, alen); + if (ret) + return ret;
- while (readw(I2C_BASE + I2SR) & I2SR_IBB && --timeout) - udelay(1); + for (i = 0; i < len; i++) { + writeb(buf[i], &i2c_regs->i2dr);
- return 0; -} + ret = i2c_imx_trx_complete(); + if (ret) + return ret; + + ret = i2c_imx_acked(); + if (ret) + return ret; + }
+ i2c_imx_stop(); + + return ret; +} #endif /* CONFIG_HARD_I2C */
Hello Marek,
Marek Vasut wrote:
Rewrite the mxc_i2c driver.
- This version is much closer to Linux implementation.
- Fixes IPG_PERCLK being incorrectly used as clock source
- Fixes behaviour of the driver on iMX51
- Clean up coding style a bit ;-)
Based on commit: e39428d53d080ad2615b772d7f99d2a70c2aaab2 Date: Mon Jun 21 09:27:05 2010 +0200 i2c-imx: do not allow interruptions when waiting for I2C to complete
Signed-off-by: Marek Vasut marek.vasut@gmail.com
drivers/i2c/mxc_i2c.c | 424 +++++++++++++++++++++++++++++++++---------------- 1 files changed, 290 insertions(+), 134 deletions(-)
V2: Convert register access to struct mxc_i2c_regs.
V3: Update licensing info
V3+: Add commit ID into commit message
checkpatch says:
ERROR: trailing statements should be on next line #143: FILE: drivers/i2c/mxc_i2c.c:130: + for (i = 0; i2c_clk_div[i][0] < div; i++);
total: 1 errors, 0 warnings, 526 lines checked
Can you fix this?
diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c index 89d1973..03e2448 100644 --- a/drivers/i2c/mxc_i2c.c +++ b/drivers/i2c/mxc_i2c.c
[...]
@@ -68,218 +78,364 @@ #endif
#define I2C_MAX_TIMEOUT 10000 -#define I2C_MAX_RETRIES 3
-static u16 div[] = { 30, 32, 36, 42, 48, 52, 60, 72, 80, 88, 104, 128, 144,
160, 192, 240, 288, 320, 384, 480, 576, 640, 768, 960,1152, 1280, 1536, 1920, 2304, 2560, 3072, 3840};+static u16 i2c_clk_div[50][2] = {
- { 22, 0x20 }, { 24, 0x21 }, { 26, 0x22 }, { 28, 0x23 },
- { 30, 0x00 }, { 32, 0x24 }, { 36, 0x25 }, { 40, 0x26 },
- { 42, 0x03 }, { 44, 0x27 }, { 48, 0x28 }, { 52, 0x05 },
- { 56, 0x29 }, { 60, 0x06 }, { 64, 0x2A }, { 72, 0x2B },
- { 80, 0x2C }, { 88, 0x09 }, { 96, 0x2D }, { 104, 0x0A },
- { 112, 0x2E }, { 128, 0x2F }, { 144, 0x0C }, { 160, 0x30 },
- { 192, 0x31 }, { 224, 0x32 }, { 240, 0x0F }, { 256, 0x33 },
- { 288, 0x10 }, { 320, 0x34 }, { 384, 0x35 }, { 448, 0x36 },
- { 480, 0x13 }, { 512, 0x37 }, { 576, 0x14 }, { 640, 0x38 },
- { 768, 0x39 }, { 896, 0x3A }, { 960, 0x17 }, { 1024, 0x3B },
- { 1152, 0x18 }, { 1280, 0x3C }, { 1536, 0x3D }, { 1792, 0x3E },
- { 1920, 0x1B }, { 2048, 0x3F }, { 2304, 0x1C }, { 2560, 0x1D },
- { 3072, 0x1E }, { 3840, 0x1F }
+};
+static u8 clk_idx;
-static inline void i2c_reset(void) -{
- writew(0, I2C_BASE + I2CR); /* Reset module */
- writew(0, I2C_BASE + I2SR);
- writew(I2CR_IEN, I2C_BASE + I2CR);
-}
-void i2c_init(int speed, int unused) +/*
- Calculate and set proper clock divider
- FIXME: remove #ifdefs !
- */
As Stefano just posted a patch for this, see here:
http://patchwork.ozlabs.org/patch/104642/
Can you fix this please?
Thanks!
+static void i2c_imx_set_clk(unsigned int rate) {
- int freq;
struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
unsigned int i2c_clk_rate;
unsigned int div; int i;
/* Divider value calculation */
#if defined(CONFIG_MX31) struct clock_control_regs *sc_regs = (struct clock_control_regs *)CCM_BASE;
- freq = mx31_get_ipg_clk();
- i2c_clk_rate = mx31_get_ipg_clk(); /* start the required I2C clock */ writel(readl(&sc_regs->cgr0) | (3 << I2C_CLK_OFFSET), &sc_regs->cgr0);
#else
- freq = mxc_get_clock(MXC_IPG_PERCLK);
- i2c_clk_rate = mxc_get_clock(MXC_IPG_CLK);
#endif
[...]
bye, Heiko
Dear Marek,
In message 4E1EB127.3040505@denx.de Heiko wrote:
Hello Marek,
Marek Vasut wrote:
Rewrite the mxc_i2c driver.
- This version is much closer to Linux implementation.
- Fixes IPG_PERCLK being incorrectly used as clock source
- Fixes behaviour of the driver on iMX51
- Clean up coding style a bit ;-)
Based on commit: e39428d53d080ad2615b772d7f99d2a70c2aaab2 Date: Mon Jun 21 09:27:05 2010 +0200 i2c-imx: do not allow interruptions when waiting for I2C to complete
Signed-off-by: Marek Vasut marek.vasut@gmail.com
drivers/i2c/mxc_i2c.c | 424 +++++++++++++++++++++++++++++++++---------------- 1 files changed, 290 insertions(+), 134 deletions(-)
V2: Convert register access to struct mxc_i2c_regs.
V3: Update licensing info
V3+: Add commit ID into commit message
checkpatch says:
ERROR: trailing statements should be on next line #143: FILE: drivers/i2c/mxc_i2c.c:130:
for (i = 0; i2c_clk_div[i][0] < div; i++);total: 1 errors, 0 warnings, 526 lines checked
Can you fix this?
Are you going to send a fixed version any time soon?
Best regards,
Wolfgang Denk
Hi again Marek,
Le 14/07/2011 16:35, Marek Vasut a écrit :
On Thursday, July 14, 2011 03:55:03 PM Albert ARIBAUD wrote:
Hi Marek,
Le 13/07/2011 23:58, Marek Vasut a écrit :
V3+: Add commit ID into commit message
What prevents a simple V4 here?
No change in code ... but I suspect there'll be V4 anyway.
Introducing variations in patch numbering, especially non-numeric variations of what is expected to be a number, is IMO useful only for testing how resilient patch processing tools can be. :)
Amicalement,
Hi, Marek,
On Thu, Jul 14, 2011 at 5:58 AM, Marek Vasut marek.vasut@gmail.com wrote:
Rewrite the mxc_i2c driver. * This version is much closer to Linux implementation. * Fixes IPG_PERCLK being incorrectly used as clock source * Fixes behaviour of the driver on iMX51 * Clean up coding style a bit ;-)
why you change i2c clock from IPG_PERCLK to IPG_CLK?
[...]
+static void i2c_imx_set_clk(unsigned int rate) {
- int freq;
- struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
- unsigned int i2c_clk_rate;
- unsigned int div;
int i;
- /* Divider value calculation */
#if defined(CONFIG_MX31) struct clock_control_regs *sc_regs = (struct clock_control_regs *)CCM_BASE;
- freq = mx31_get_ipg_clk();
- i2c_clk_rate = mx31_get_ipg_clk();
/* start the required I2C clock */ writel(readl(&sc_regs->cgr0) | (3 << I2C_CLK_OFFSET), &sc_regs->cgr0); #else
- freq = mxc_get_clock(MXC_IPG_PERCLK);
- i2c_clk_rate = mxc_get_clock(MXC_IPG_CLK);
#endif
There are two clocks for i2c:
Peripheral clock (IPBus): source from ipg_clk_root, which is for IP bus register read/write. Block clock: source from perclk_root, which is I2C function clock.
We need get perclk not ipg clock, right?
BTW, do you test this driver on mx53?
Jason
On Friday, July 29, 2011 08:55:14 AM Jason Hui wrote:
Hi, Marek,
On Thu, Jul 14, 2011 at 5:58 AM, Marek Vasut marek.vasut@gmail.com wrote:
Rewrite the mxc_i2c driver.
- This version is much closer to Linux implementation.
- Fixes IPG_PERCLK being incorrectly used as clock source
- Fixes behaviour of the driver on iMX51
- Clean up coding style a bit ;-)
why you change i2c clock from IPG_PERCLK to IPG_CLK?
On MX51, PERCLK are those fast (680MHz) clock, that's not source of clock for I2C. The IPG_CLK (they are 68.5MHz iirc) are source for the I2C. Also, I discussed this with Stefano and we agreed this is likely a bug.
[...]
+static void i2c_imx_set_clk(unsigned int rate) {
int freq;
struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;unsigned int i2c_clk_rate;unsigned int div; int i;/* Divider value calculation */#if defined(CONFIG_MX31) struct clock_control_regs *sc_regs = (struct clock_control_regs *)CCM_BASE;
freq = mx31_get_ipg_clk();
i2c_clk_rate = mx31_get_ipg_clk(); /* start the required I2C clock */ writel(readl(&sc_regs->cgr0) | (3 << I2C_CLK_OFFSET), &sc_regs->cgr0);#else
freq = mxc_get_clock(MXC_IPG_PERCLK);
i2c_clk_rate = mxc_get_clock(MXC_IPG_CLK);#endif
There are two clocks for i2c:
Peripheral clock (IPBus): source from ipg_clk_root, which is for IP bus register read/write. Block clock: source from perclk_root, which is I2C function clock.
We need get perclk not ipg clock, right?
For divider, we need those slower ones, the IPG_CLK, not PERCLK.
BTW, do you test this driver on mx53?
No, I don't have one.
Jason
On 07/29/2011 11:35 AM, Marek Vasut wrote:
why you change i2c clock from IPG_PERCLK to IPG_CLK?
On MX51, PERCLK are those fast (680MHz) clock, that's not source of clock for I2C. The IPG_CLK (they are 68.5MHz iirc) are source for the I2C. Also, I discussed this with Stefano and we agreed this is likely a bug.
Well, this code must be suitable for all i.MX processor. If we have a different source for i.MX51 and i.MX53, we can hide this adding an entry to mxc_get_clock(), such as mxc_get_clock(MXC_I2C_CLK). the right clock is then chosen in the specific mxc_get_clock function.
Best regards, Stefano Babic
On Friday, July 29, 2011 08:55:14 AM Jason Hui wrote:
Hi, Marek,
On Thu, Jul 14, 2011 at 5:58 AM, Marek Vasut marek.vasut@gmail.com wrote:
Rewrite the mxc_i2c driver.
- This version is much closer to Linux implementation.
- Fixes IPG_PERCLK being incorrectly used as clock source
- Fixes behaviour of the driver on iMX51
- Clean up coding style a bit ;-)
why you change i2c clock from IPG_PERCLK to IPG_CLK?
[...]
Ok, I investigated a bit deeper and I suspect the clock.c is the culprit.
Apparently, the PERCLK doesn't run at the frequency the clock.c reports it runs on. Therefore, the i2c miscalculates the divider etc -- falling crap model (waterfall model).
Anyway, Jason, can you look into that clock problem? I think there are more than just perclk miscalculated.
Cheers
On Thu, Sep 15, 2011 at 3:39 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Friday, July 29, 2011 08:55:14 AM Jason Hui wrote:
Hi, Marek,
On Thu, Jul 14, 2011 at 5:58 AM, Marek Vasut marek.vasut@gmail.com wrote:
Rewrite the mxc_i2c driver. * This version is much closer to Linux implementation. * Fixes IPG_PERCLK being incorrectly used as clock source * Fixes behaviour of the driver on iMX51 * Clean up coding style a bit ;-)
why you change i2c clock from IPG_PERCLK to IPG_CLK?
[...]
Ok, I investigated a bit deeper and I suspect the clock.c is the culprit.
Apparently, the PERCLK doesn't run at the frequency the clock.c reports it runs on. Therefore, the i2c miscalculates the divider etc -- falling crap model (waterfall model).
But apparently, the i2c function clock should be IPG_PERCLK not IPG clock. And Linux also fix it already.
commit 9d73242458d9a2fe26e2e240488063d414eacb1c Author: Lothar Waßmann LW@KARO-electronics.de Date: Mon Jul 4 15:52:17 2011 +0200
mach-mx5: fix the I2C clock parents
The clock from which the I2C timing is derived is the ipg_perclk not ipg_clk.
I2C bus frequency was lower by a factor of ~8 due to the clock divider calculation being based on 66.5MHz IPG clock while the bus actually uses 8MHz ipg_perclk.
Kernel version: 3.0.0-rc2 branch 'imx-for-next' of git://git.pengutronix.de/git/imx/linux-2.6
Signed-off-by: Lothar Waßmann LW@KARO-electronics.de Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
Anyway, Jason, can you look into that clock problem? I think there are more than just perclk miscalculated.
OK, I will check that part.
Jason
Cheers
On Thursday, September 15, 2011 03:43:42 AM Jason Hui wrote:
On Thu, Sep 15, 2011 at 3:39 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Friday, July 29, 2011 08:55:14 AM Jason Hui wrote:
Hi, Marek,
On Thu, Jul 14, 2011 at 5:58 AM, Marek Vasut marek.vasut@gmail.com wrote:
Rewrite the mxc_i2c driver.
- This version is much closer to Linux implementation.
- Fixes IPG_PERCLK being incorrectly used as clock source
- Fixes behaviour of the driver on iMX51
- Clean up coding style a bit ;-)
why you change i2c clock from IPG_PERCLK to IPG_CLK?
[...]
Ok, I investigated a bit deeper and I suspect the clock.c is the culprit.
Apparently, the PERCLK doesn't run at the frequency the clock.c reports it runs on. Therefore, the i2c miscalculates the divider etc -- falling crap model (waterfall model).
But apparently, the i2c function clock should be IPG_PERCLK not IPG clock. And Linux also fix it already.
Then there's bulls**t in your mx51 and mx53 datasheet or what ? besides, PERCLK is faster than IPGCLK on MX51 so it makes even less sense! Can you please talk to the HW guys or whatever to clear this once and for all ? I smell noone really knows where the clock are sourced from and all this crap is just blind guessing.
On Thu, Sep 15, 2011 at 10:07 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Thursday, September 15, 2011 03:43:42 AM Jason Hui wrote:
On Thu, Sep 15, 2011 at 3:39 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Friday, July 29, 2011 08:55:14 AM Jason Hui wrote:
Hi, Marek,
On Thu, Jul 14, 2011 at 5:58 AM, Marek Vasut marek.vasut@gmail.com wrote:
Rewrite the mxc_i2c driver. * This version is much closer to Linux implementation. * Fixes IPG_PERCLK being incorrectly used as clock source * Fixes behaviour of the driver on iMX51 * Clean up coding style a bit ;-)
why you change i2c clock from IPG_PERCLK to IPG_CLK?
[...]
Ok, I investigated a bit deeper and I suspect the clock.c is the culprit.
Apparently, the PERCLK doesn't run at the frequency the clock.c reports it runs on. Therefore, the i2c miscalculates the divider etc -- falling crap model (waterfall model).
But apparently, the i2c function clock should be IPG_PERCLK not IPG clock. And Linux also fix it already.
Then there's bulls**t in your mx51 and mx53 datasheet or what ?
Please refer to MCIMX51RM.PDF, page 305, Table 7-41. PERCLK-dependent Module Clock Sources PERCLK-dependent Module Clocks Associated CCGR Register uart1_perclk CCGR1 uart2_perclk uart3_perclk i2c1 clocks i2c2 clocks epit1_highfreq CCGR2 epit2_highfreq pwm1_highfreq pwm2_highfreq gpt_highfreq owire clocks
besides, PERCLK is faster than IPGCLK on MX51 so it makes even less sense!
I don't think PERCLK is always faster than IPG clock, it's configurable. please refer to MCIMX51RM.PDF, page 307.
Can you please talk to the HW guys or whatever to clear this once and for all ? I smell noone really knows where the clock are sourced from and all this crap is just blind guessing.
I have asked the IC module owner again. It confirms that I2C function clock is ipg_perclk.
Jason
On Thursday, September 15, 2011 04:26:17 AM Jason Hui wrote:
On Thu, Sep 15, 2011 at 10:07 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Thursday, September 15, 2011 03:43:42 AM Jason Hui wrote:
On Thu, Sep 15, 2011 at 3:39 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Friday, July 29, 2011 08:55:14 AM Jason Hui wrote:
Hi, Marek,
On Thu, Jul 14, 2011 at 5:58 AM, Marek Vasut marek.vasut@gmail.com
wrote:
Rewrite the mxc_i2c driver.
- This version is much closer to Linux implementation.
- Fixes IPG_PERCLK being incorrectly used as clock source
- Fixes behaviour of the driver on iMX51
- Clean up coding style a bit ;-)
why you change i2c clock from IPG_PERCLK to IPG_CLK?
[...]
Ok, I investigated a bit deeper and I suspect the clock.c is the culprit.
Apparently, the PERCLK doesn't run at the frequency the clock.c reports it runs on. Therefore, the i2c miscalculates the divider etc -- falling crap model (waterfall model).
But apparently, the i2c function clock should be IPG_PERCLK not IPG clock. And Linux also fix it already.
Then there's bulls**t in your mx51 and mx53 datasheet or what ?
Please refer to MCIMX51RM.PDF, page 305, Table 7-41. PERCLK-dependent Module Clock Sources PERCLK-dependent Module Clocks Associated CCGR Register uart1_perclk CCGR1 uart2_perclk uart3_perclk i2c1 clocks i2c2 clocks epit1_highfreq CCGR2 epit2_highfreq pwm1_highfreq pwm2_highfreq gpt_highfreq owire clocks
You see ... I'm starting to understand what is actually going wrong. The lowlevel_init.S is bloated with crap (why? why can't that be in cpu init C code ?) and there is this one part, where CBCDR is overwritten with a configurable value instead of hardcoded value.
No documentation about that at all, but it's there ... and that's -- amongst other bugs -- my problem I assume. So I need to set this CONFIG_SYS_CLKTL_CBCDR to another magic value, now I get it.
besides, PERCLK is faster than IPGCLK on MX51 so it makes even less sense!
I don't think PERCLK is always faster than IPG clock, it's configurable. please refer to MCIMX51RM.PDF, page 307.
Yea ... it's configurable via some undocumented macro in assembler code. Damn.
Can you please comment on the other patches?
Thanks
Can you please talk
to the HW guys or whatever to clear this once and for all ? I smell noone really knows where the clock are sourced from and all this crap is just blind guessing.
I have asked the IC module owner again. It confirms that I2C function clock is ipg_perclk.
Jason
On 07/13/2011 11:58 PM, Marek Vasut wrote:
Rewrite the mxc_i2c driver.
- This version is much closer to Linux implementation.
- Fixes IPG_PERCLK being incorrectly used as clock source
- Fixes behaviour of the driver on iMX51
- Clean up coding style a bit ;-)
Hi Marek,
as it seems we need a while to fix all issues with MX53, I would prefer to fix the MX31, or some boards are broken (mx31phycore). I will send a patch *only* to fix this issue, that means to get the clock via mxc_get_clock() for MX31. This was also a comment in your patch. Please then base your new version on the fix I will send.
Thanks, Stefano
participants (6)
-
Albert ARIBAUD -
Heiko Schocher -
Jason Hui -
Marek Vasut -
Stefano Babic -
Wolfgang Denk