Patchwork [U-Boot,V2,05/11] I2C: mxc_i2c: address failure with mx35 processor

login
register
mail settings
Submitter Stefano Babic
Date Jan. 20, 2011, 8:46 a.m.
Message ID <1295513194-16158-6-git-send-email-sbabic@denx.de>
Download mbox | patch
Permalink /patch/79654/
State Superseded
Delegated to: Stefano Babic
Headers show

Comments

Stefano Babic - Jan. 20, 2011, 8:46 a.m.
There is sporadic failures when more as one I2C slave
is on the bus and the processor tries to communicate
with more as one slave.
The problem was seen on a mx35pdk (two I2C slaves,
PMIC controller and CAN/RTC chip).

Signed-off-by: Stefano Babic <sbabic@denx.de>
CC: Heiko Schocher <hs@denx.de>
---
 drivers/i2c/mxc_i2c.c |   93 +++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 71 insertions(+), 22 deletions(-)
Wolfgang Denk - Jan. 20, 2011, 9:30 a.m.
Dear Stefano Babic,

In message <1295513194-16158-6-git-send-email-sbabic@denx.de> you wrote:
> There is sporadic failures when more as one I2C slave
> is on the bus and the processor tries to communicate
> with more as one slave.
> The problem was seen on a mx35pdk (two I2C slaves,
> PMIC controller and CAN/RTC chip).

Please also mention which approach the patch takes to fix this
problem.

Checkpatch says:

	total: 1 errors, 1 warnings, 166 lines checked

Please have a look.

> -#ifdef DEBUG
> -#define DPRINTF(args...)  printf(args)
> -#else
> -#define DPRINTF(args...)
> -#endif

Ah.  Please forget the related comment in my previous message.

But please mention this change in the commit message.


Best regards,

Wolfgang Denk
Stefano Babic - Jan. 20, 2011, 10:27 a.m.
On 01/20/2011 10:30 AM, Wolfgang Denk wrote:
> Dear Stefano Babic,
> 
> In message <1295513194-16158-6-git-send-email-sbabic@denx.de> you wrote:
>> There is sporadic failures when more as one I2C slave
>> is on the bus and the processor tries to communicate
>> with more as one slave.
>> The problem was seen on a mx35pdk (two I2C slaves,
>> PMIC controller and CAN/RTC chip).
> 
> Please also mention which approach the patch takes to fix this
> problem.

Ok

> 
> Checkpatch says:
> 
> 	total: 1 errors, 1 warnings, 166 lines checked

I know, but I found the construct ok. checkpatch complains against using
assignment in if function, even if it is large used. Again, as we have a
tool to check patches, I will change to remove this issue.

> 
> Please have a look.
> 
>> -#ifdef DEBUG
>> -#define DPRINTF(args...)  printf(args)
>> -#else
>> -#define DPRINTF(args...)
>> -#endif
> 
> Ah.  Please forget the related comment in my previous message.

...and you please forget my comment to your comment...

> 
> But please mention this change in the commit message.

I will do it.

Best regards,
Stefano Babic

Patch

diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index eead502..5667e68 100755
--- a/drivers/i2c/mxc_i2c.c
+++ b/drivers/i2c/mxc_i2c.c
@@ -72,11 +72,8 @@ 
 #error "define CONFIG_SYS_I2C_MX<Processor>_PORTx to use the mx I2C driver"
 #endif
 
-#ifdef DEBUG
-#define DPRINTF(args...)  printf(args)
-#else
-#define DPRINTF(args...)
-#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,
@@ -110,37 +107,67 @@  void i2c_init(int speed, int unused)
 		if (freq / div[i] <= speed)
 			break;
 
-	DPRINTF("%s: speed: %d\n",__FUNCTION__, speed);
+	debug("%s: speed: %d\n", __FUNCTION__, speed);
 
 	writew(i, I2C_BASE + IFDR);
 	i2c_reset();
 }
 
+static int wait_idle(void)
+{
+	int timeout = I2C_MAX_TIMEOUT;
+
+	while ((readw(I2C_BASE + I2SR) & I2SR_IBB) && --timeout) {
+		writew(0, I2C_BASE + I2SR);
+		udelay(1);
+	}
+	return timeout ? timeout : (!(readw(I2C_BASE + I2SR) & I2SR_IBB));
+}
+
 static int wait_busy(void)
 {
-	int timeout = 10000;
+	int timeout = I2C_MAX_TIMEOUT;
 
-	while (!(readw(I2C_BASE + I2SR) & I2SR_IIF) && --timeout)
+	while (!(readw(I2C_BASE + I2SR) & I2SR_IBB) && --timeout)
 		udelay(1);
 	writew(0, I2C_BASE + I2SR); /* clear interrupt */
 
 	return timeout;
 }
 
+static int wait_complete(void)
+{
+	int timeout = I2C_MAX_TIMEOUT;
+
+	while ((!(readw(I2C_BASE + I2SR) & I2SR_ICF)) && (--timeout)) {
+		writew(0, I2C_BASE + I2SR);
+		udelay(1);
+	}
+	udelay(200);
+
+	writew(0, I2C_BASE + I2SR);	/* clear interrupt */
+
+	return timeout;
+}
+
+
 static int tx_byte(u8 byte)
 {
 	writew(byte, I2C_BASE + I2DR);
 
-	if (!wait_busy() || readw(I2C_BASE + I2SR) & I2SR_RX_NO_AK)
+	if (!wait_complete() || readw(I2C_BASE + I2SR) & I2SR_RX_NO_AK)
 		return -1;
 	return 0;
 }
 
-static int rx_byte(void)
+static int rx_byte(int last)
 {
-	if (!wait_busy())
+	if (!wait_complete())
 		return -1;
 
+	if (last)
+		writew(I2CR_IEN, I2C_BASE + I2CR);
+
 	return readw(I2C_BASE + I2DR);
 }
 
@@ -160,24 +187,48 @@  int i2c_probe(uchar chip)
 
 static int i2c_addr(uchar chip, uint addr, int alen)
 {
-	writew(0, I2C_BASE + I2SR);
-	writew(I2CR_IEN |  I2CR_MSTA | I2CR_MTX, I2C_BASE + I2CR);
+	int i, retry = 0;
+	for (retry = 0; retry < 3; retry++) {
+		if (wait_idle())
+			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 (tx_byte(chip << 1))
+	if (!wait_busy()) {
+		debug("%s:trigger start fail(%x)\n",
+		       __func__, readw(I2C_BASE + I2SR));
 		return -1;
+	}
 
+	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))
+		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;
 }
 
 int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 {
-	int timeout = 10000;
+	int timeout = I2C_MAX_TIMEOUT;
 	int ret;
 
-	DPRINTF("%s chip: 0x%02x addr: 0x%04x alen: %d len: %d\n",
+	debug("%s chip: 0x%02x addr: 0x%04x alen: %d len: %d\n",
 		__FUNCTION__, chip, addr, alen, len);
 
 	if (i2c_addr(chip, addr, alen)) {
@@ -197,7 +248,7 @@  int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 	ret = readw(I2C_BASE + I2DR);
 
 	while (len--) {
-		if ((ret = rx_byte()) < 0)
+		if ((ret = rx_byte(len == 0)) < 0)
 			return -1;
 		*buf++ = ret;
 		if (len <= 1)
@@ -206,8 +257,6 @@  int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 				I2C_BASE + I2CR);
 	}
 
-	wait_busy();
-
 	writew(I2CR_IEN, I2C_BASE + I2CR);
 
 	while (readw(I2C_BASE + I2SR) & I2SR_IBB && --timeout)
@@ -218,8 +267,8 @@  int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
 
 int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
 {
-	int timeout = 10000;
-	DPRINTF("%s chip: 0x%02x addr: 0x%04x alen: %d len: %d\n",
+	int timeout = I2C_MAX_TIMEOUT;
+	debug("%s chip: 0x%02x addr: 0x%04x alen: %d len: %d\n",
 		__FUNCTION__, chip, addr, alen, len);
 
 	if (i2c_addr(chip, addr, alen))