* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
@ 2011-07-13 21:58 Marek Vasut
2011-07-14 9:04 ` Heiko Schocher
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Marek Vasut @ 2011-07-13 21:58 UTC (permalink / raw)
To: u-boot
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@synertronixx.de>
+ * Copyright (C) 2005 Matthias Blaschke <blaschke@synertronixx.de>
+ * Copyright (C) 2007 RightHand Technologies, Inc.
+ * Copyright (C) 2008 Darius Augulis <darius.augulis@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 */
--
1.7.5.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-07-13 21:58 [U-Boot] [PATCH V3+] I2C: mxc_i2c rework Marek Vasut
@ 2011-07-14 9:04 ` Heiko Schocher
2011-07-28 14:29 ` Wolfgang Denk
2011-07-14 13:55 ` Albert ARIBAUD
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Heiko Schocher @ 2011-07-14 9:04 UTC (permalink / raw)
To: u-boot
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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-07-13 21:58 [U-Boot] [PATCH V3+] I2C: mxc_i2c rework Marek Vasut
2011-07-14 9:04 ` Heiko Schocher
@ 2011-07-14 13:55 ` Albert ARIBAUD
2011-07-14 14:35 ` Marek Vasut
2011-07-29 6:55 ` Jason Hui
2011-08-30 10:48 ` Stefano Babic
3 siblings, 1 reply; 17+ messages in thread
From: Albert ARIBAUD @ 2011-07-14 13:55 UTC (permalink / raw)
To: u-boot
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?
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-07-14 13:55 ` Albert ARIBAUD
@ 2011-07-14 14:35 ` Marek Vasut
2011-07-14 14:45 ` Albert ARIBAUD
0 siblings, 1 reply; 17+ messages in thread
From: Marek Vasut @ 2011-07-14 14:35 UTC (permalink / raw)
To: u-boot
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.
>
> Amicalement,
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-07-14 14:35 ` Marek Vasut
@ 2011-07-14 14:45 ` Albert ARIBAUD
0 siblings, 0 replies; 17+ messages in thread
From: Albert ARIBAUD @ 2011-07-14 14:45 UTC (permalink / raw)
To: u-boot
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,
--
Albert.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-07-14 9:04 ` Heiko Schocher
@ 2011-07-28 14:29 ` Wolfgang Denk
0 siblings, 0 replies; 17+ messages in thread
From: Wolfgang Denk @ 2011-07-28 14:29 UTC (permalink / raw)
To: u-boot
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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
THIS IS A 100% MATTER PRODUCT: In the Unlikely Event That This
Merchandise Should Contact Antimatter in Any Form, a Catastrophic
Explosion Will Result.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-07-13 21:58 [U-Boot] [PATCH V3+] I2C: mxc_i2c rework Marek Vasut
2011-07-14 9:04 ` Heiko Schocher
2011-07-14 13:55 ` Albert ARIBAUD
@ 2011-07-29 6:55 ` Jason Hui
2011-07-29 9:35 ` Marek Vasut
2011-09-14 19:39 ` Marek Vasut
2011-08-30 10:48 ` Stefano Babic
3 siblings, 2 replies; 17+ messages in thread
From: Jason Hui @ 2011-07-29 6:55 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-07-29 6:55 ` Jason Hui
@ 2011-07-29 9:35 ` Marek Vasut
2011-07-29 9:42 ` Stefano Babic
2011-09-14 19:39 ` Marek Vasut
1 sibling, 1 reply; 17+ messages in thread
From: Marek Vasut @ 2011-07-29 9:35 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-07-29 9:35 ` Marek Vasut
@ 2011-07-29 9:42 ` Stefano Babic
0 siblings, 0 replies; 17+ messages in thread
From: Stefano Babic @ 2011-07-29 9:42 UTC (permalink / raw)
To: u-boot
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
--
=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
=====================================================================
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-07-13 21:58 [U-Boot] [PATCH V3+] I2C: mxc_i2c rework Marek Vasut
` (2 preceding siblings ...)
2011-07-29 6:55 ` Jason Hui
@ 2011-08-30 10:48 ` Stefano Babic
3 siblings, 0 replies; 17+ messages in thread
From: Stefano Babic @ 2011-08-30 10:48 UTC (permalink / raw)
To: u-boot
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
--
=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
=====================================================================
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-07-29 6:55 ` Jason Hui
2011-07-29 9:35 ` Marek Vasut
@ 2011-09-14 19:39 ` Marek Vasut
2011-09-15 1:43 ` Jason Hui
1 sibling, 1 reply; 17+ messages in thread
From: Marek Vasut @ 2011-09-14 19:39 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-09-14 19:39 ` Marek Vasut
@ 2011-09-15 1:43 ` Jason Hui
2011-09-15 2:07 ` Marek Vasut
0 siblings, 1 reply; 17+ messages in thread
From: Jason Hui @ 2011-09-15 1:43 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-09-15 1:43 ` Jason Hui
@ 2011-09-15 2:07 ` Marek Vasut
2011-09-15 2:26 ` Jason Hui
0 siblings, 1 reply; 17+ messages in thread
From: Marek Vasut @ 2011-09-15 2:07 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-09-15 2:07 ` Marek Vasut
@ 2011-09-15 2:26 ` Jason Hui
2011-09-15 4:07 ` Marek Vasut
0 siblings, 1 reply; 17+ messages in thread
From: Jason Hui @ 2011-09-15 2:26 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3+] I2C: mxc_i2c rework
2011-09-15 2:26 ` Jason Hui
@ 2011-09-15 4:07 ` Marek Vasut
0 siblings, 0 replies; 17+ messages in thread
From: Marek Vasut @ 2011-09-15 4:07 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3] I2C: mxc_i2c rework
2011-07-13 21:05 [U-Boot] [PATCH V3] " Marek Vasut
@ 2011-07-13 21:34 ` Wolfgang Denk
0 siblings, 0 replies; 17+ messages in thread
From: Wolfgang Denk @ 2011-07-13 21:34 UTC (permalink / raw)
To: u-boot
Dear Marek Vasut,
In message <1310591152-3791-1-git-send-email-marek.vasut@gmail.com> you 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 ;-)
>
> 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
Sorry, but can you _PLEASE_ read
http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sign
?
Especially the part that starts: "provide terse but precise
information which exact version or even commit ID was used" etc. etc.
Thanks.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There are very few personal problems that cannot be solved through a
suitable application of high explosives.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH V3] I2C: mxc_i2c rework
@ 2011-07-13 21:05 Marek Vasut
2011-07-13 21:34 ` Wolfgang Denk
0 siblings, 1 reply; 17+ messages in thread
From: Marek Vasut @ 2011-07-13 21:05 UTC (permalink / raw)
To: u-boot
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 ;-)
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
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@synertronixx.de>
+ * Copyright (C) 2005 Matthias Blaschke <blaschke@synertronixx.de>
+ * Copyright (C) 2007 RightHand Technologies, Inc.
+ * Copyright (C) 2008 Darius Augulis <darius.augulis@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 */
--
1.7.5.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2011-09-15 4:07 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-13 21:58 [U-Boot] [PATCH V3+] I2C: mxc_i2c rework Marek Vasut
2011-07-14 9:04 ` Heiko Schocher
2011-07-28 14:29 ` Wolfgang Denk
2011-07-14 13:55 ` Albert ARIBAUD
2011-07-14 14:35 ` Marek Vasut
2011-07-14 14:45 ` Albert ARIBAUD
2011-07-29 6:55 ` Jason Hui
2011-07-29 9:35 ` Marek Vasut
2011-07-29 9:42 ` Stefano Babic
2011-09-14 19:39 ` Marek Vasut
2011-09-15 1:43 ` Jason Hui
2011-09-15 2:07 ` Marek Vasut
2011-09-15 2:26 ` Jason Hui
2011-09-15 4:07 ` Marek Vasut
2011-08-30 10:48 ` Stefano Babic
-- strict thread matches above, loose matches on Subject: below --
2011-07-13 21:05 [U-Boot] [PATCH V3] " Marek Vasut
2011-07-13 21:34 ` Wolfgang Denk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.