Skip to contentSkip to conversation

I2C: mxc_i2c rework

PATCH New Marek Vasut 6 participants 15 messages Jul 13, 2011 – Sep 15, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 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

---
 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

drivers/i2c/mxc_i2c.c +290 -134
index 89d1973..03e2448 100644
@@ -1,7 +1,15 @@
 /*
- * i2c driver for Freescale mx31
+ * i2c driver for Freescale i.MX series
  *
  * (c) 2007 Pengutronix, Sascha Hauer <[email protected]>
+ * (c) 2011 Marek Vasut <[email protected]>
+ *
+ * Based on i2c-imx.c from linux kernel:
+ *  Copyright (C) 2005 Torsten Koschorrek <[email protected]>
+ *  Copyright (C) 2005 Matthias Blaschke <[email protected]>
+ *  Copyright (C) 2007 RightHand Technologies, Inc.
+ *  Copyright (C) 2008 Darius Augulis <[email protected]>
+ *
  *
  * 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 */
Signature · 1 line
1.7.5.3
#1 lore raw

PATCH 1/1 I2C: mxc_i2c rework

Hello Marek,

Quoted 15 lines · Marek Vasut

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

---
 drivers/i2c/mxc_i2c.c |  424 +++++++++++++++++++++++++++++++++----------------
 1 files changed, 290 insertions(+), 134 deletions(-)
Quoted 5 lines

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?

Quoted 4 lines
drivers/i2c/mxc_i2c.c +0 -0
index 89d1973..03e2448 100644

[...]

Quoted 30 lines
drivers/i2c/mxc_i2c.c +17 -7
@@ -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 */

drivers/i2c/mxc_i2c.c line 104

Quoted 10 lines
drivers/i2c/mxc_i2c.c +5 -5
-	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!

Quoted 12 lines
drivers/i2c/mxc_i2c.c +5 -1
+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;

drivers/i2c/mxc_i2c.c line 123

Quoted 10 lines
drivers/i2c/mxc_i2c.c +2 -2
 
-	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

Signature · 2 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
#2 lore raw

PATCH 1/1 I2C: mxc_i2c rework

Dear Marek,

Quoted 26 lines · In message <[email protected]> Heiko wrote:

Hello Marek,

Quoted 15 lines · Marek Vasut

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

---
 drivers/i2c/mxc_i2c.c |  424 +++++++++++++++++++++++++++++++++----------------
 1 files changed, 290 insertions(+), 134 deletions(-)
Quoted 5 lines

V2: Convert register access to struct mxc_i2c_regs.

V3: Update licensing info

V3+: Add commit ID into commit message

checkpatch says:

Quoted 7 lines

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

Signature · 6 lines
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.
#3 lore raw

PATCH 1/1 I2C: mxc_i2c rework

In reply to Marek Vasut

Hi Marek,

Le 13/07/2011 23:58, Marek Vasut a ?crit :

Quoted 1 line

V3+: Add commit ID into commit message

What prevents a simple V4 here?

Amicalement,

Signature · 1 line
Albert.
#4 lore raw

PATCH 1/1 I2C: mxc_i2c rework

Quoted 6 lines · Albert ARIBAUD

Hi Marek,

Le 13/07/2011 23:58, Marek Vasut a ?crit :

Quoted 1 line

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.

Quoted 2 lines

Amicalement,

#5 lore raw

PATCH 1/1 I2C: mxc_i2c rework

Hi again Marek,

Le 14/07/2011 16:35, Marek Vasut a ?crit :

Quoted 9 lines · Albert ARIBAUD
Quoted 6 lines

Hi Marek,

Le 13/07/2011 23:58, Marek Vasut a ?crit :

Quoted 1 line

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,

Signature · 1 line
Albert.
#6 lore raw

PATCH 1/1 I2C: mxc_i2c rework

In reply to Marek Vasut

Hi, Marek,

Quoted 6 lines · Marek Vasut

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?

[...]

Quoted 13 lines

+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;

Quoted 9 lines

- ? ? ? 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

Quoted 1 line
#7 lore raw

PATCH 1/1 I2C: mxc_i2c rework

Quoted 10 lines · Jason Hui

Hi, Marek,

Quoted 5 lines · Marek Vasut

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.

Quoted 26 lines

[...]

Quoted 12 lines
drivers/i2c/mxc_i2c.c +5 -1
+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;

drivers/i2c/mxc_i2c.c line 123

Quoted 10 lines
drivers/i2c/mxc_i2c.c +2 -2
 
-       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
Quoted 7 lines

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.

Quoted 2 lines

BTW, do you test this driver on mx53?

No, I don't have one.

Quoted 2 lines

Jason

#8 lore raw

PATCH 1/1 I2C: mxc_i2c rework

Quoted 6 lines · Marek Vasut
Quoted 2 lines

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

Signature · 5 lines
=====================================================================
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
=====================================================================
#9 lore raw

PATCH 1/1 I2C: mxc_i2c rework

In reply to Jason Hui
Quoted 9 lines · Jason Hui

Hi, Marek,

Quoted 5 lines · Marek Vasut

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 ;-)

Quoted 3 lines

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

#10 lore raw

PATCH 1/1 I2C: mxc_i2c rework

Quoted 12 lines · Marek Vasut
Quoted 11 lines · Jason Hui

Hi, Marek,

Quoted 5 lines · Marek Vasut

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?

Quoted 7 lines
Quoted 1 line

[...]

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 <[email protected]>
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

Quoted 3 lines

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

Quoted 3 lines

Cheers

#11 lore raw

PATCH 1/1 I2C: mxc_i2c rework

Quoted 15 lines · Jason Hui
Quoted 14 lines · Marek Vasut
Quoted 12 lines · Jason Hui

Hi, Marek,

Quoted 5 lines · Marek Vasut

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?

[...]

Quoted 8 lines
Quoted 5 lines

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.

#12 lore raw

PATCH 1/1 I2C: mxc_i2c rework

Quoted 18 lines · Marek Vasut
Quoted 17 lines · Jason Hui
Quoted 16 lines · Marek Vasut
Quoted 9 lines · Jason Hui

Hi, Marek,

Quoted 5 lines · Marek Vasut

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 ;-)

Quoted 3 lines

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.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 3 lines

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

Quoted 1 line
#13 lore raw

PATCH 1/1 I2C: mxc_i2c rework

Quoted 7 lines · Jason Hui
Quoted 6 lines · Marek Vasut
Quoted 5 lines · Jason Hui
Quoted 4 lines · Marek Vasut
Quoted 3 lines · Jason Hui

Hi, Marek,

On Thu, Jul 14, 2011 at 5:58 AM, Marek Vasut <[email protected]>

Quoted 22 lines · wrote:
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 9 lines
Quoted 5 lines

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.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

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 ?

Quoted 16 lines

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.

Quoted 6 lines
Quoted 3 lines

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

Quoted 11 lines
Quoted 6 lines

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

#14 lore raw

PATCH 1/1 I2C: mxc_i2c rework

In reply to Marek Vasut
Quoted 5 lines · Marek Vasut

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

Signature · 5 lines
=====================================================================
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
=====================================================================