diff mbox

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

Message ID 1295513194-16158-6-git-send-email-sbabic@denx.de
State Superseded
Delegated to: Stefano Babic
Headers show

Commit Message

Stefano Babic Jan. 20, 2011, 8:46 a.m. UTC 
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(-)

Comments

Wolfgang Denk Jan. 20, 2011, 9:30 a.m. UTC | #1 
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. UTC | #2 
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
diff mbox

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