[U-Boot] [PATCH v3] OMAP: I2C: New read, write and probe functions

[U-Boot] [PATCH v3] OMAP: I2C: New read, write and probe functions

[Lubomir Popov]

Tested on OMAP4/5 only, but should work on older OMAPs and
derivatives as well.

- Rewritten i2c_read to operate correctly with all types of chips
  (old function could not read consistent data from some I2C slaves).
- Optimised i2c_write.
- New i2c_probe, optionally selectable via CONFIG_I2C_PROBE_WRITE,
  performs write access vs read. The old probe could hang the system
  under certain conditions (e.g. unconfigured pads).
- The read/write/probe functions try to identify unconfigured bus.
- Status functions now read irqstatus_raw as per TRM guidelines
  (except for OMAP243X and OMAP34XX).
- Driver now supports up to I2C5 (OMAP5).

Signed-off-by: Lubomir Popov <lpopov@mm-sol.com>
---
V3 changes:
- Removed old functions and conditional compilation. New functions
  are now built unconditionally for all SoCs using this driver. The
  only chip that should break is the OMAP2420 dinosaur.
- Interrupts are enabled for OMAP243X and OMAP34XX only (where we
  don't have an irqstatus_raw register).
V2 changes:
- Probe tries to identify misconfigured pads as well.
- Status is retrieved from irqstatus_raw rather than from stat.
- Some minor style & format changes.

 drivers/i2c/omap24xx_i2c.c | 482 ++++++++++++++++++++++++++++++---------------
 1 file changed, 326 insertions(+), 156 deletions(-)
 mode change 100644 => 100755 drivers/i2c/omap24xx_i2c.c

diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
old mode 100644
new mode 100755
index 54e9b15..deac0ec
--- a/drivers/i2c/omap24xx_i2c.c
+++ b/drivers/i2c/omap24xx_i2c.c
@@ -18,6 +18,18 @@
  *
  * Adapted for OMAP2420 I2C, r-woodruff2 at ti.com
  *
+ * Copyright (c) 2013 Lubomir Popov <lpopov@mm-sol.com>, MM Solutions
+ * New i2c_read, i2c_write and i2c_probe functions, tested on OMAP4/5
+ * only, but should work on older OMAPs and derivatives as well.
+ * - Rewritten i2c_read to operate correctly with all types of chips
+ *   (old function could not read consistent data from some I2C slaves).
+ * - Optimized i2c_write.
+ * - New i2c_probe, optionally selectable via CONFIG_I2C_PROBE_WRITE,
+ *   performs write access vs read. The old probe could hang the system
+ *   under certain conditions (e.g. unconfigured pads).
+ * - The read/write/probe functions try to identify unconfigured bus.
+ * - Status functions now read irqstatus_raw as per TRM guidelines.
+ * - Driver now supports up to I2C5 (OMAP5).
  */

 #include <common.h>
@@ -31,8 +43,11 @@ DECLARE_GLOBAL_DATA_PTR;

 #define I2C_TIMEOUT	1000

+/* Absolutely safe for status update at 100 kHz I2C: */
+#define I2C_WAIT	200
+
 static int wait_for_bb(void);
-static u16 wait_for_pin(void);
+static u16 wait_for_event(void);
 static void flush_fifo(void);

 /*
@@ -137,10 +152,14 @@ void i2c_init(int speed, int slaveadd)
 	/* own address */
 	writew(slaveadd, &i2c_base->oa);
 	writew(I2C_CON_EN, &i2c_base->con);
-
-	/* have to enable intrrupts or OMAP i2c module doesn't work */
+#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
+	/*
+	 * Have to enable interrupts for OMAP2/3, these IPs don't have
+	 * an 'irqstatus_raw' register and we shall have to poll 'stat'
+	 */
 	writew(I2C_IE_XRDY_IE | I2C_IE_RRDY_IE | I2C_IE_ARDY_IE |
 		I2C_IE_NACK_IE | I2C_IE_AL_IE, &i2c_base->ie);
+#endif
 	udelay(1000);
 	flush_fifo();
 	writew(0xFFFF, &i2c_base->stat);
@@ -150,88 +169,6 @@ void i2c_init(int speed, int slaveadd)
 		bus_initialized[current_bus] = 1;
 }

-static int i2c_read_byte(u8 devaddr, u16 regoffset, u8 alen, u8 *value)
-{
-	int i2c_error = 0;
-	u16 status;
-	int i = 2 - alen;
-	u8 tmpbuf[2] = {(regoffset) >> 8, regoffset & 0xff};
-	u16 w;
-
-	/* wait until bus not busy */
-	if (wait_for_bb())
-		return 1;
-
-	/* one byte only */
-	writew(alen, &i2c_base->cnt);
-	/* set slave address */
-	writew(devaddr, &i2c_base->sa);
-	/* no stop bit needed here */
-	writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT |
-	      I2C_CON_TRX, &i2c_base->con);
-
-	/* send register offset */
-	while (1) {
-		status = wait_for_pin();
-		if (status == 0 || status & I2C_STAT_NACK) {
-			i2c_error = 1;
-			goto read_exit;
-		}
-		if (status & I2C_STAT_XRDY) {
-			w = tmpbuf[i++];
-#if !(defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \
-	defined(CONFIG_OMAP44XX) || defined(CONFIG_AM33XX) || \
-	defined(CONFIG_OMAP54XX))
-			w |= tmpbuf[i++] << 8;
-#endif
-			writew(w, &i2c_base->data);
-			writew(I2C_STAT_XRDY, &i2c_base->stat);
-		}
-		if (status & I2C_STAT_ARDY) {
-			writew(I2C_STAT_ARDY, &i2c_base->stat);
-			break;
-		}
-	}
-
-	/* set slave address */
-	writew(devaddr, &i2c_base->sa);
-	/* read one byte from slave */
-	writew(1, &i2c_base->cnt);
-	/* need stop bit here */
-	writew(I2C_CON_EN | I2C_CON_MST |
-		I2C_CON_STT | I2C_CON_STP,
-		&i2c_base->con);
-
-	/* receive data */
-	while (1) {
-		status = wait_for_pin();
-		if (status == 0 || status & I2C_STAT_NACK) {
-			i2c_error = 1;
-			goto read_exit;
-		}
-		if (status & I2C_STAT_RRDY) {
-#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \
-	defined(CONFIG_OMAP44XX) || defined(CONFIG_AM33XX) || \
-	defined(CONFIG_OMAP54XX)
-			*value = readb(&i2c_base->data);
-#else
-			*value = readw(&i2c_base->data);
-#endif
-			writew(I2C_STAT_RRDY, &i2c_base->stat);
-		}
-		if (status & I2C_STAT_ARDY) {
-			writew(I2C_STAT_ARDY, &i2c_base->stat);
-			break;
-		}
-	}
-
-read_exit:
-	flush_fifo();
-	writew(0xFFFF, &i2c_base->stat);
-	writew(0, &i2c_base->cnt);
-	return i2c_error;
-}
-
 static void flush_fifo(void)
 {	u16 stat;

@@ -241,13 +178,7 @@ static void flush_fifo(void)
 	while (1) {
 		stat = readw(&i2c_base->stat);
 		if (stat == I2C_STAT_RRDY) {
-#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \
-	defined(CONFIG_OMAP44XX) || defined(CONFIG_AM33XX) || \
-	defined(CONFIG_OMAP54XX)
 			readb(&i2c_base->data);
-#else
-			readw(&i2c_base->data);
-#endif
 			writew(I2C_STAT_RRDY, &i2c_base->stat);
 			udelay(1000);
 		} else
@@ -255,6 +186,66 @@ static void flush_fifo(void)
 	}
 }

+#if defined(CONFIG_I2C_PROBE_WRITE)
+/*
+ * i2c_probe: Use write access. Allows to identify addresses that are
+ *            write-only (like the config register of dual-port EEPROMs)
+ */
+int i2c_probe(uchar chip)
+{
+	u16 status;
+	int res = 1; /* default = fail */
+
+	if (chip == readw(&i2c_base->oa))
+		return res;
+
+	/* Wait until bus is free */
+	if (wait_for_bb())
+		return res;
+
+	/* No data transfer, slave addr only */
+	writew(0, &i2c_base->cnt);
+	/* set slave address */
+	writew(chip, &i2c_base->sa);
+	/* stop bit needed here */
+	writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX |
+	       I2C_CON_STP, &i2c_base->con);
+
+	status = wait_for_event();
+
+	if ((status & ~I2C_STAT_XRDY) == 0 || (status & I2C_STAT_AL)) {
+		/*
+		 * With current high-level command implementation, notifying
+		 * the user shall flood the console with 127 messages. If
+		 * silent exit is desired upon unconfigured bus, remove the
+		 * following 'if' section:
+		 */
+		if (status == I2C_STAT_XRDY)
+			printf("i2c_probe: pads on bus %d "
+                               "probably not configured (status=0x%x)\n",
+			      		current_bus, status);
+
+		goto pr_exit;
+	}
+
+	/* check for ACK (!NAK) */
+	if (!(status & I2C_STAT_NACK)) {
+		res = 0;	/* Device found */
+		/* Abort transfer (force idle state) */
+		writew(I2C_CON_MST | I2C_CON_TRX, &i2c_base->con); /* Reset */
+		udelay(1000);
+		writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_TRX |
+			I2C_CON_STP, &i2c_base->con);		/* STP */
+	}
+pr_exit:
+	flush_fifo();
+	writew(0xFFFF, &i2c_base->stat);
+	writew(0, &i2c_base->cnt);
+	return res;
+}
+
+#else
+/* Old function, uses read access */
 int i2c_probe(uchar chip)
 {
 	u16 status;
@@ -275,7 +266,7 @@ int i2c_probe(uchar chip)
 	writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP, &i2c_base->con);

 	while (1) {
-		status = wait_for_pin();
+		status = wait_for_event();
 		if (status == 0 || status & I2C_STAT_AL) {
 			res = 1;
 			goto probe_exit;
@@ -296,13 +287,7 @@ int i2c_probe(uchar chip)
 		}
 		if (status & I2C_STAT_RRDY) {
 			res = 0;
-#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \
-	defined(CONFIG_OMAP44XX) || defined(CONFIG_AM33XX) || \
-	defined(CONFIG_OMAP54XX)
 			readb(&i2c_base->data);
-#else
-			readw(&i2c_base->data);
-#endif
 			writew(I2C_STAT_RRDY, &i2c_base->stat);
 		}
 	}
@@ -314,10 +299,38 @@ probe_exit:
 	writew(0xFFFF, &i2c_base->stat);
 	return res;
 }
+#endif  /* CONFIG_I2C_PROBE_WRITE */

+/*
+ * i2c_read: Function now uses a single I2C read transaction with bulk transfer
+ *           of the requested number of bytes (note that the 'i2c md' command
+ *           limits this to 16 bytes anyway). If CONFIG_I2C_REPEATED_START is
+ *           defined in the board config header, this transaction shall be with
+ *           Repeated Start (Sr) between the address and data phases; otherwise
+ *           Stop-Start (P-S) shall be used (some I2C chips do require a P-S).
+ *           The address (reg offset) may be 0, 1 or 2 bytes long.
+ *           Function now reads correctly from chips that return more than one
+ *           byte of data per addressed register (like TI temperature sensors),
+ *           or that do not need a register address at all (such as some clock
+ *           distributors).
+ */
 int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 {
-	int i;
+	int i2c_error = 0;
+	u16 status;
+
+	if (alen < 0) {
+		printf("I2C read: addr len < 0\n");
+		return 1;
+	}
+	if (len < 0) {
+		printf("I2C read: data len < 0\n");
+		return 1;
+	}
+	if (buffer == NULL) {
+		printf("I2C read: NULL pointer passed\n");
+		return 1;
+	}

 	if (alen > 2) {
 		printf("I2C read: addr len %d not supported\n", alen);
@@ -329,25 +342,125 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 		return 1;
 	}

-	for (i = 0; i < len; i++) {
-		if (i2c_read_byte(chip, addr + i, alen, &buffer[i])) {
-			puts("I2C read: I/O error\n");
-			i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
-			return 1;
+	/* Wait until bus not busy */
+	if (wait_for_bb())
+		return 1;
+
+	/* Zero, one or two bytes reg address (offset) */
+	writew(alen, &i2c_base->cnt);
+	/* Set slave address */
+	writew(chip, &i2c_base->sa);
+
+	if (alen) {
+		/* Must write reg offset first */
+#ifdef CONFIG_I2C_REPEATED_START
+		/* No stop bit, use Repeated Start (Sr) */
+		writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT |
+	      		I2C_CON_TRX, &i2c_base->con);
+#else
+		/* Stop - Start (P-S) */
+		writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP |
+	      		I2C_CON_TRX, &i2c_base->con);
+#endif
+		/* Send register offset */
+		while (1) {
+			status = wait_for_event();
+			/* Try to identify bus that is not padconf'd for I2C */
+			if (status == I2C_STAT_XRDY) {
+				i2c_error = 2;
+				printf("i2c_read (addr phase): pads on bus %d "
+                                       "probably not configured (status=0x%x)\n",
+			      		current_bus, status);
+				goto rd_exit;
+			}
+			if (status == 0 || status & I2C_STAT_NACK) {
+				i2c_error = 1;
+				printf("i2c_read: error waiting for addr ACK "
+                                       "(status=0x%x)\n", status);
+				goto rd_exit;
+			}
+			if (alen) {
+				if (status & I2C_STAT_XRDY) {
+					alen--;
+					/* Do we have to use byte access? */
+					writeb((addr >> (8 * alen)) & 0xff,
+                                               &i2c_base->data);
+					writew(I2C_STAT_XRDY, &i2c_base->stat);
+				}
+			}
+			if (status & I2C_STAT_ARDY) {
+				writew(I2C_STAT_ARDY, &i2c_base->stat);
+				break;
+			}
 		}
 	}
+	/* Set slave address */
+	writew(chip, &i2c_base->sa);
+	/* Read len bytes from slave */
+	writew(len, &i2c_base->cnt);
+	/* Need stop bit here */
+	writew(I2C_CON_EN | I2C_CON_MST |
+		I2C_CON_STT | I2C_CON_STP,
+		&i2c_base->con);

-	return 0;
+	/* Receive data */
+	while (1) {
+		status = wait_for_event();
+		/*
+		 * Try to identify bus that is not padconf'd for I2C. This
+		 * state could be left over from previous transactions if
+		 * the address phase is skipped due to alen=0.
+		 */
+		if (status == I2C_STAT_XRDY) {
+			i2c_error = 2;
+			printf("i2c_read (data phase): pads on bus %d "
+                               "probably not configured (status=0x%x)\n",
+		      		current_bus, status);
+			goto rd_exit;
+		}
+		if (status == 0 || status & I2C_STAT_NACK) {
+			i2c_error = 1;
+			goto rd_exit;
+		}
+		if (status & I2C_STAT_RRDY) {
+			*buffer++ = readb(&i2c_base->data);
+			writew(I2C_STAT_RRDY, &i2c_base->stat);
+		}
+		if (status & I2C_STAT_ARDY) {
+			writew(I2C_STAT_ARDY, &i2c_base->stat);
+			break;
+		}
+	}
+
+rd_exit:
+	flush_fifo();
+	writew(0xFFFF, &i2c_base->stat);
+	writew(0, &i2c_base->cnt);
+	return i2c_error;
 }

+/* i2c_write: Address (reg offset) may be 0, 1 or 2 bytes long. */
 int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 {
 	int i;
 	u16 status;
 	int i2c_error = 0;
-	u16 w;
-	u8 tmpbuf[2] = {addr >> 8, addr & 0xff};

+	if (alen < 0) {
+		printf("I2C write: addr len < 0\n");
+		return 1;
+	}
+
+	if (len < 0) {
+		printf("I2C write: data len < 0\n");
+		return 1;
+	}
+
+	if (buffer == NULL) {
+		printf("I2C write: NULL pointer passed\n");
+		return 1;
+	}
+
 	if (alen > 2) {
 		printf("I2C write: addr len %d not supported\n", alen);
 		return 1;
@@ -355,92 +468,141 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)

 	if (addr + len > (1 << 16)) {
 		printf("I2C write: address 0x%x + 0x%x out of range\n",
-				addr, len);
+                        addr, len);
 		return 1;
 	}

-	/* wait until bus not busy */
+	/* Wait until bus not busy */
 	if (wait_for_bb())
 		return 1;

-	/* start address phase - will write regoffset + len bytes data */
-	/* TODO consider case when !CONFIG_OMAP243X/34XX/44XX */
+	/* Start address phase - will write regoffset + len bytes data */
 	writew(alen + len, &i2c_base->cnt);
-	/* set slave address */
+	/* Set slave address */
 	writew(chip, &i2c_base->sa);
-	/* stop bit needed here */
+	/* Stop bit needed here */
 	writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX |
 		I2C_CON_STP, &i2c_base->con);

-	/* Send address and data */
-	for (i = -alen; i < len; i++) {
-		status = wait_for_pin();
-
+	while (alen) {
+		/* Must write reg offset (one or two bytes) */
+		status = wait_for_event();
+		/* Try to identify bus that is not padconf'd for I2C */
+		if (status == I2C_STAT_XRDY) {
+			i2c_error = 2;
+			printf("i2c_write: pads on bus %d "
+                               "probably not configured (status=0x%x)\n",
+		      		current_bus, status);
+			goto wr_exit;
+		}
 		if (status == 0 || status & I2C_STAT_NACK) {
 			i2c_error = 1;
-			printf("i2c error waiting for data ACK (status=0x%x)\n",
-					status);
-			goto write_exit;
+			printf("i2c_write: error waiting for addr ACK "
+                               "(status=0x%x)\n", status);
+			goto wr_exit;
 		}
-
 		if (status & I2C_STAT_XRDY) {
-			w = (i < 0) ? tmpbuf[2+i] : buffer[i];
-#if !(defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \
-	defined(CONFIG_OMAP44XX) || defined(CONFIG_AM33XX) || \
-	defined(CONFIG_OMAP54XX))
-			w |= ((++i < 0) ? tmpbuf[2+i] : buffer[i]) << 8;
-#endif
-			writew(w, &i2c_base->data);
+			alen--;
+			writeb((addr >> (8 * alen)) & 0xff, &i2c_base->data);
 			writew(I2C_STAT_XRDY, &i2c_base->stat);
-		} else {
+		}
+		else {
 			i2c_error = 1;
-			printf("i2c bus not ready for Tx (i=%d)\n", i);
-			goto write_exit;
+			printf("i2c_write: bus not ready for addr Tx "
+                               "(status=0x%x)\n", status);
+			goto wr_exit;
+		}
+	}
+	/* Address phase is over, now write data */
+	for (i = 0; i < len; i++) {
+		status = wait_for_event();
+		if (status == 0 || status & I2C_STAT_NACK) {
+			i2c_error = 1;
+			printf("i2c_write: error waiting for data ACK "
+                               "(status=0x%x)\n", status);
+			goto wr_exit;
+		}
+		if (status & I2C_STAT_XRDY) {
+			writeb(buffer[i], &i2c_base->data);
+			writew(I2C_STAT_XRDY, &i2c_base->stat);
+		}
+		else {
+			i2c_error = 1;
+			printf("i2c_write: bus not ready for data Tx "
+                               "(i=%d)\n", i);
+			goto wr_exit;
 		}
 	}

-write_exit:
+wr_exit:
 	flush_fifo();
 	writew(0xFFFF, &i2c_base->stat);
+	writew(0, &i2c_base->cnt);
 	return i2c_error;
 }

+/*
+ * Wait for the bus to be free by checking the Bus Busy (BB)
+ * bit to become clear
+ */
 static int wait_for_bb(void)
 {
 	int timeout = I2C_TIMEOUT;
 	u16 stat;

 	writew(0xFFFF, &i2c_base->stat);	/* clear current interrupts...*/
+#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
 	while ((stat = readw(&i2c_base->stat) & I2C_STAT_BB) && timeout--) {
+#else
+	/* Read RAW status */
+	while ((stat = readw(&i2c_base->irqstatus_raw) &
+                I2C_STAT_BB) && timeout--) {
+#endif
 		writew(stat, &i2c_base->stat);
-		udelay(1000);
+		udelay(I2C_WAIT);
 	}

 	if (timeout <= 0) {
-		printf("timed out in wait_for_bb: I2C_STAT=%x\n",
-			readw(&i2c_base->stat));
+		printf("Timed out in wait_for_bb: status=%04x\n",
+			stat);
 		return 1;
 	}
 	writew(0xFFFF, &i2c_base->stat);	 /* clear delayed stuff*/
 	return 0;
 }

-static u16 wait_for_pin(void)
+/*
+ * Wait for the I2C controller to complete current action
+ * and update status
+ */
+static u16 wait_for_event(void)
 {
 	u16 status;
 	int timeout = I2C_TIMEOUT;

 	do {
-		udelay(1000);
+		udelay(I2C_WAIT);
+#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
 		status = readw(&i2c_base->stat);
+#else
+		/* Read RAW status */
+		status = readw(&i2c_base->irqstatus_raw);
+#endif
 	} while (!(status &
 		   (I2C_STAT_ROVR | I2C_STAT_XUDF | I2C_STAT_XRDY |
 		    I2C_STAT_RRDY | I2C_STAT_ARDY | I2C_STAT_NACK |
 		    I2C_STAT_AL)) && timeout--);

 	if (timeout <= 0) {
-		printf("timed out in wait_for_pin: I2C_STAT=%x\n",
-			readw(&i2c_base->stat));
+		printf("Timed out in wait_for_event: status=%04x\n",
+			status);
+		/*
+		 * If status is still 0 here, probably the bus pads have
+		 * not been configured for I2C, and/or pull-ups are missing.
+		 */
+		printf("Check if pads/pull-ups of bus %d "
+                       "are properly configured\n",
+			current_bus);
 		writew(0xFFFF, &i2c_base->stat);
 		status = 0;
 	}
@@ -450,28 +612,36 @@ static u16 wait_for_pin(void)

 int i2c_set_bus_num(unsigned int bus)
 {
-	if ((bus < 0) || (bus >= I2C_BUS_MAX)) {
-		printf("Bad bus: %d\n", bus);
+	if (bus >= I2C_BUS_MAX) {
+		printf("Bad bus: %x\n", bus);
 		return -1;
 	}

-#if I2C_BUS_MAX == 4
-	if (bus == 3)
-		i2c_base = (struct i2c *)I2C_BASE4;
-	else
-	if (bus == 2)
-		i2c_base = (struct i2c *)I2C_BASE3;
-	else
+	switch (bus) {
+		default:
+			bus = 0;	/* Fall through */
+		case 0:
+			i2c_base = (struct i2c *)I2C_BASE1;
+			break;
+		case 1:
+			i2c_base = (struct i2c *)I2C_BASE2;
+			break;
+#if (I2C_BUS_MAX > 2)
+		case 2:
+			i2c_base = (struct i2c *)I2C_BASE3;
+			break;
+#if (I2C_BUS_MAX > 3)
+		case 3:
+			i2c_base = (struct i2c *)I2C_BASE4;
+			break;
+#if (I2C_BUS_MAX > 4)
+		case 4:
+			i2c_base = (struct i2c *)I2C_BASE5;
+			break;
 #endif
-#if I2C_BUS_MAX == 3
-	if (bus == 2)
-		i2c_base = (struct i2c *)I2C_BASE3;
-	else
 #endif
-	if (bus == 1)
-		i2c_base = (struct i2c *)I2C_BASE2;
-	else
-		i2c_base = (struct i2c *)I2C_BASE1;
+#endif
+	}

 	current_bus = bus;

-- 
1.7.12.4 (Apple Git-37)

[U-Boot] [PATCH v3] OMAP: I2C: New read, write and probe functions

[Tom Rini]

On Thu, May 30, 2013 at 01:24:42AM +0300, Lubomir Popov wrote:

> Tested on OMAP4/5 only, but should work on older OMAPs and
> derivatives as well.
> 
> - Rewritten i2c_read to operate correctly with all types of chips
>   (old function could not read consistent data from some I2C slaves).
> - Optimised i2c_write.
> - New i2c_probe, optionally selectable via CONFIG_I2C_PROBE_WRITE,
>   performs write access vs read. The old probe could hang the system
>   under certain conditions (e.g. unconfigured pads).
> - The read/write/probe functions try to identify unconfigured bus.
> - Status functions now read irqstatus_raw as per TRM guidelines
>   (except for OMAP243X and OMAP34XX).
> - Driver now supports up to I2C5 (OMAP5).
> 
> Signed-off-by: Lubomir Popov <lpopov@mm-sol.com>

With CONFIG_I2C_PROBE_WRITE set:
Tested-by: Tom Rini <trini@ti.com> on Beagleboard / Beagleboard xM

So lets just go with the write probe always being on again.

Now, when I git am'd I saw some whitespace problems, so please make sure
v4 is checkpatch clean.  And note that printf("Long than 80 char wide",
a, b) is OK and expected to NOT break the string (but do align the
args).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130530/19c26d6a/attachment.pgp>

[U-Boot] [PATCH v3] OMAP: I2C: New read, write and probe functions

[Lubomir Popov]

Hi Tom,

On 30/05/13 17:37, Tom Rini wrote:
> On Thu, May 30, 2013 at 01:24:42AM +0300, Lubomir Popov wrote:
> 
>> Tested on OMAP4/5 only, but should work on older OMAPs and
>> derivatives as well.
>>
>> - Rewritten i2c_read to operate correctly with all types of chips
>>   (old function could not read consistent data from some I2C slaves).
>> - Optimised i2c_write.
>> - New i2c_probe, optionally selectable via CONFIG_I2C_PROBE_WRITE,
>>   performs write access vs read. The old probe could hang the system
>>   under certain conditions (e.g. unconfigured pads).
>> - The read/write/probe functions try to identify unconfigured bus.
>> - Status functions now read irqstatus_raw as per TRM guidelines
>>   (except for OMAP243X and OMAP34XX).
>> - Driver now supports up to I2C5 (OMAP5).
>>
>> Signed-off-by: Lubomir Popov <lpopov@mm-sol.com>
> 
> With CONFIG_I2C_PROBE_WRITE set:
> Tested-by: Tom Rini <trini@ti.com> on Beagleboard / Beagleboard xM
> 
> So lets just go with the write probe always being on again.
> 
> Now, when I git am'd I saw some whitespace problems, so please make sure
> v4 is checkpatch clean.  And note that printf("Long than 80 char wide",
> a, b) is OK and expected to NOT break the string (but do align the
> args).
> 
OK, shall do it tonight. I see a minor problem however: if we are not
going to support all OMAP2 chips (the 2420 in particular), isn't it
somewhat misleading to keep the driver named omap24xx_i2c? Letting
you decide...

-- 
Lubo

[U-Boot] [PATCH v3] OMAP: I2C: New read, write and probe functions

[Tom Rini]

On Thu, May 30, 2013 at 05:51:44PM +0300, Lubomir Popov wrote:
> Hi Tom,
> 
> On 30/05/13 17:37, Tom Rini wrote:
> > On Thu, May 30, 2013 at 01:24:42AM +0300, Lubomir Popov wrote:
> > 
> >> Tested on OMAP4/5 only, but should work on older OMAPs and
> >> derivatives as well.
> >>
> >> - Rewritten i2c_read to operate correctly with all types of chips
> >>   (old function could not read consistent data from some I2C slaves).
> >> - Optimised i2c_write.
> >> - New i2c_probe, optionally selectable via CONFIG_I2C_PROBE_WRITE,
> >>   performs write access vs read. The old probe could hang the system
> >>   under certain conditions (e.g. unconfigured pads).
> >> - The read/write/probe functions try to identify unconfigured bus.
> >> - Status functions now read irqstatus_raw as per TRM guidelines
> >>   (except for OMAP243X and OMAP34XX).
> >> - Driver now supports up to I2C5 (OMAP5).
> >>
> >> Signed-off-by: Lubomir Popov <lpopov@mm-sol.com>
> > 
> > With CONFIG_I2C_PROBE_WRITE set:
> > Tested-by: Tom Rini <trini@ti.com> on Beagleboard / Beagleboard xM
> > 
> > So lets just go with the write probe always being on again.
> > 
> > Now, when I git am'd I saw some whitespace problems, so please make sure
> > v4 is checkpatch clean.  And note that printf("Long than 80 char wide",
> > a, b) is OK and expected to NOT break the string (but do align the
> > args).
> > 
> OK, shall do it tonight. I see a minor problem however: if we are not
> going to support all OMAP2 chips (the 2420 in particular), isn't it
> somewhat misleading to keep the driver named omap24xx_i2c? Letting
> you decide...

Please add a comment that this driver needs some re-adaptation for the
OMAP2420 implementation of this IP block now.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130530/ef3cf64a/attachment.pgp>

[U-Boot] [PATCH v3] OMAP: I2C: New read, write and probe functions

[Tom Rini]

On Thu, May 30, 2013 at 10:37:42AM -0400, Tom Rini wrote:
> On Thu, May 30, 2013 at 01:24:42AM +0300, Lubomir Popov wrote:
> 
> > Tested on OMAP4/5 only, but should work on older OMAPs and
> > derivatives as well.
> > 
> > - Rewritten i2c_read to operate correctly with all types of chips
> >   (old function could not read consistent data from some I2C slaves).
> > - Optimised i2c_write.
> > - New i2c_probe, optionally selectable via CONFIG_I2C_PROBE_WRITE,
> >   performs write access vs read. The old probe could hang the system
> >   under certain conditions (e.g. unconfigured pads).
> > - The read/write/probe functions try to identify unconfigured bus.
> > - Status functions now read irqstatus_raw as per TRM guidelines
> >   (except for OMAP243X and OMAP34XX).
> > - Driver now supports up to I2C5 (OMAP5).
> > 
> > Signed-off-by: Lubomir Popov <lpopov@mm-sol.com>
> 
> With CONFIG_I2C_PROBE_WRITE set:
> Tested-by: Tom Rini <trini@ti.com> on Beagleboard / Beagleboard xM

But, crap, breaks am335x_evm (and probably beaglebones, etc).  I'll
dig into this more to see if I can spot something obvious tomorrow.


-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130530/01008034/attachment.pgp>

[U-Boot] [PATCH v3] OMAP: I2C: New read, write and probe functions

[Lubomir Popov]

Hi Tom,

> On Thu, May 30, 2013 at 10:37:42AM -0400, Tom Rini wrote:
>> On Thu, May 30, 2013 at 01:24:42AM +0300, Lubomir Popov wrote:
>>
>> > Tested on OMAP4/5 only, but should work on older OMAPs and
>> > derivatives as well.
>> >
>> > - Rewritten i2c_read to operate correctly with all types of chips
>> >   (old function could not read consistent data from some I2C slaves).
>> > - Optimised i2c_write.
>> > - New i2c_probe, optionally selectable via CONFIG_I2C_PROBE_WRITE,
>> >   performs write access vs read. The old probe could hang the system
>> >   under certain conditions (e.g. unconfigured pads).
>> > - The read/write/probe functions try to identify unconfigured bus.
>> > - Status functions now read irqstatus_raw as per TRM guidelines
>> >   (except for OMAP243X and OMAP34XX).
>> > - Driver now supports up to I2C5 (OMAP5).
>> >
>> > Signed-off-by: Lubomir Popov <lpopov@mm-sol.com>
>>
>> With CONFIG_I2C_PROBE_WRITE set:
>> Tested-by: Tom Rini <trini@ti.com> on Beagleboard / Beagleboard xM
>
> But, crap, breaks am335x_evm (and probably beaglebones, etc).  I'll
> dig into this more to see if I can spot something obvious tomorrow.
>
OK, holding off. And thanks for testing.
>
> --
> Tom
>
-- 
Lubo

[U-Boot] [PATCH v3] OMAP: I2C: New read, write and probe functions

[Lubomir Popov]

Hi Tom,

> On Thu, May 30, 2013 at 10:37:42AM -0400, Tom Rini wrote:
>> On Thu, May 30, 2013 at 01:24:42AM +0300, Lubomir Popov wrote:
>>
>> > Tested on OMAP4/5 only, but should work on older OMAPs and
>> > derivatives as well.
>> >
>> > - Rewritten i2c_read to operate correctly with all types of chips
>> >   (old function could not read consistent data from some I2C slaves).
>> > - Optimised i2c_write.
>> > - New i2c_probe, optionally selectable via CONFIG_I2C_PROBE_WRITE,
>> >   performs write access vs read. The old probe could hang the system
>> >   under certain conditions (e.g. unconfigured pads).
>> > - The read/write/probe functions try to identify unconfigured bus.
>> > - Status functions now read irqstatus_raw as per TRM guidelines
>> >   (except for OMAP243X and OMAP34XX).
>> > - Driver now supports up to I2C5 (OMAP5).
>> >
>> > Signed-off-by: Lubomir Popov <lpopov@mm-sol.com>
>>
>> With CONFIG_I2C_PROBE_WRITE set:
>> Tested-by: Tom Rini <trini@ti.com> on Beagleboard / Beagleboard xM
>
> But, crap, breaks am335x_evm (and probably beaglebones, etc).  I'll
> dig into this more to see if I can spot something obvious tomorrow.

How does it actually break? Does not build, or I2C is not working,
or what?

If I2C is not working, could you try changing the condition

#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)

to

#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || defined(CONFIG_AM33XX)

found in 3 places in driver (lines 155, 554, 585) and test again?

Thanks,
Lubo

[U-Boot] [PATCH v3] OMAP: I2C: New read, write and probe functions

[Lubomir Popov]

Hi Tom,

On 30/05/13 20:07, Tom Rini wrote:
> On Thu, May 30, 2013 at 10:37:42AM -0400, Tom Rini wrote:
>> On Thu, May 30, 2013 at 01:24:42AM +0300, Lubomir Popov wrote:
>>
>>> Tested on OMAP4/5 only, but should work on older OMAPs and
>>> derivatives as well.
>>>
>>> - Rewritten i2c_read to operate correctly with all types of chips
>>>   (old function could not read consistent data from some I2C slaves).
>>> - Optimised i2c_write.
>>> - New i2c_probe, optionally selectable via CONFIG_I2C_PROBE_WRITE,
>>>   performs write access vs read. The old probe could hang the system
>>>   under certain conditions (e.g. unconfigured pads).
>>> - The read/write/probe functions try to identify unconfigured bus.
>>> - Status functions now read irqstatus_raw as per TRM guidelines
>>>   (except for OMAP243X and OMAP34XX).
>>> - Driver now supports up to I2C5 (OMAP5).
>>>
>>> Signed-off-by: Lubomir Popov <lpopov@mm-sol.com>
>>
>> With CONFIG_I2C_PROBE_WRITE set:
>> Tested-by: Tom Rini <trini@ti.com> on Beagleboard / Beagleboard xM
> 
> But, crap, breaks am335x_evm (and probably beaglebones, etc).  I'll
> dig into this more to see if I can spot something obvious tomorrow.
Made it work on the am335x_evm (tested, with AM3359 on board).
Problem was in the new i2c_probe, which on this board is called by
the SPL and caused a hang (interestingly, when called from regular
u-boot, works fine). Fix is to add a small delay. Clocking and I2C
speed should be identical between SPL and U-Boot, but probably are
not; I did not have time to look with a scope, but don't have any
other reasonable explanation. Whatever, now it is working. I shall
submit the patch tonight.

Would like as well to note that the current u-boot-ti/master
(freshly cloned, no changes whatsoever) does not boot on the
am335x-evm:

U-Boot SPL 2013.04-11562-g47c6ea0 (May 31 2013 - 09:56:46)
musb-hdrc: ConfigData=0xde (UTMI-8, dyn FIFOs, HB-ISO Rx, HB-ISO Tx, SoftConn)
musb-hdrc: MHDRC RTL version 2.0 
musb-hdrc: setup fifo_mode 4
musb-hdrc: 28/31 max ep, 16384/16384 memory
USB Peripheral mode controller at 47401000 using PIO, IRQ 0
musb-hdrc: ConfigData=0xde (UTMI-8, dyn FIFOs, HB-ISO Rx, HB-ISO Tx, SoftConn)
musb-hdrc: MHDRC RTL version 2.0 
musb-hdrc: setup fifo_mode 4
musb-hdrc: 28/31 max ep, 16384/16384 memory
USB Host mode controller at 47401800 using PIO, IRQ 0
### ERROR ### Please RESET the board ###

-- 
Lubo

[U-Boot] [PATCH v3] OMAP: I2C: New read, write and probe functions

[Lubomir Popov]

Hi Tom,

[snip]

>> But, crap, breaks am335x_evm (and probably beaglebones, etc).  I'll
>> dig into this more to see if I can spot something obvious tomorrow.

> Made it work on the am335x_evm (tested, with AM3359 on board).
> Problem was in the new i2c_probe, which on this board is called by
> the SPL and caused a hang (interestingly, when called from regular
> u-boot, works fine). Fix is to add a small delay. Clocking and I2C
> speed should be identical between SPL and U-Boot, but probably are
> not; I did not have time to look with a scope, but don't have any
> other reasonable explanation. Whatever, now it is working. I shall
> submit the patch tonight.
> 
[snip]

Just tested on Beaglebone, works fine as well.

-- 
Lubo