diff mbox

[U-Boot,1/1] OMAP4/5: I2C: New read/write/probe functions

Message ID 515D82C0.30909@mm-sol.com
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Lubomir Popov April 4, 2013, 1:40 p.m. UTC 
These new functions are built for OMAP4 and OMAP5 only. The major
improvement is that the new i2c_read fixes the inability to read
correctly from certain types of I2C chips.

- Rewritten i2c_read to operate correctly with all types of chips.
- Optimized i2c_write. Both read/write functions try to identify
  unconfigured bus.
- New i2c_probe performs write access vs read. Optionally selectable
  via CONFIG_I2C_PROBE_WRITE.
- Driver supports up to I2C5 (OMAP5; I2C_BASE5 must be defined).

Signed-off-by: Lubomir Popov <lpopov@mm-sol.com>

---
 drivers/i2c/omap24xx_i2c.c |  332 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 315 insertions(+), 17 deletions(-)

Comments

Tom Rini May 9, 2013, 3:25 p.m. UTC | #1 
On Thu, Apr 04, 2013 at 04:40:16PM +0300, Lubomir Popov wrote:

> These new functions are built for OMAP4 and OMAP5 only. The major
> improvement is that the new i2c_read fixes the inability to read
> correctly from certain types of I2C chips.
> 
> - Rewritten i2c_read to operate correctly with all types of chips.
> - Optimized i2c_write. Both read/write functions try to identify
>   unconfigured bus.
> - New i2c_probe performs write access vs read. Optionally selectable
>   via CONFIG_I2C_PROBE_WRITE.
> - Driver supports up to I2C5 (OMAP5; I2C_BASE5 must be defined).
> 
> Signed-off-by: Lubomir Popov <lpopov@mm-sol.com>

So, I've tested the i2c_probe on the was failing last time we tried a
write-style probe and it's OK.  But I'm not sure that we're obeying the
comments I spell out in 168a5acb81a8d92a7effcb2727aaa89367b97e05 about
why we can't do a write-style probe all the same.  Can you be sure we're
following the constraints there?  Thanks!
Lubomir Popov May 9, 2013, 4:20 p.m. UTC | #2 
Hi Tom,

On 09/05/13 18:25, Tom Rini wrote:
> On Thu, Apr 04, 2013 at 04:40:16PM +0300, Lubomir Popov wrote:
> 
>> These new functions are built for OMAP4 and OMAP5 only. The major
>> improvement is that the new i2c_read fixes the inability to read
>> correctly from certain types of I2C chips.
>>
>> - Rewritten i2c_read to operate correctly with all types of chips.
>> - Optimized i2c_write. Both read/write functions try to identify
>>   unconfigured bus.
>> - New i2c_probe performs write access vs read. Optionally selectable
>>   via CONFIG_I2C_PROBE_WRITE.
>> - Driver supports up to I2C5 (OMAP5; I2C_BASE5 must be defined).
>>
>> Signed-off-by: Lubomir Popov <lpopov@mm-sol.com>
> 
> So, I've tested the i2c_probe on the was failing last time we tried a
> write-style probe and it's OK.  But I'm not sure that we're obeying the
> comments I spell out in 168a5acb81a8d92a7effcb2727aaa89367b97e05 about
> why we can't do a write-style probe all the same.  Can you be sure we're
> following the constraints there?  Thanks!
> 
Quite a long time has passed since I played with that faulty i2c_probe
for the first time (on an old version of U-Boot that I initially ported
for our OMAP5 board, and this totally erroneous output was in fact the
reason for me to rework the I2C driver). As far as I remember, the main
issue was that the byte count register was loaded with the value of 1,
although no data was transmitted after the address byte (when the
transfer is aborted), leaving the controller in a bad state after the
first probe. This explains why only the first probe result is valid,
and the following faulty positive probes of all subsequent addresses.
So this was actually a bug; while fixing it, I decided to rewrite the
poorly designed i2c_read as well, and also touched the other functions
a bit. Because the changes were significant, I decided to present a
separate driver and even submitted it to the list; later I patched the
existing omap24xx_i2c.c with the most important changes only. You know
this history.

Meanwhile I upgraded to the current version of U-Boot and found that
the probe was changed to read access vs write (the above revert,
right?). Nevertheless I stayed with my driver; as you know, it has
been tested on OMAP4430/60/70 and OMAP5430, with not a single issue
found so far.

I don't see any conflict with the TRM whatsoever (for OMAP4/5 at
least).

Best regards,
Lubo
diff mbox

Patch

diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
index 54e9b15..6890d3c 100644
--- a/drivers/i2c/omap24xx_i2c.c
+++ b/drivers/i2c/omap24xx_i2c.c
@@ -18,6 +18,14 @@ 
  *
  * Adapted for OMAP2420 I2C, r-woodruff2@ti.com
  *
+ * Copyright (c) 2013 Lubomir Popov <lpopov@mm-sol.com>, MM Solutions
+ * New i2c_read, i2c_write and i2c_probe functions for OMAP4/5 only.
+ * - Rewritten i2c_read to operate correctly with all types of chips.
+ * - Optimized i2c_write. Both read/write functions try to identify
+ *   unconfigured bus.
+ * - New i2c_probe performs write access vs read. Optionally selectable
+ *   via CONFIG_I2C_PROBE_WRITE.
+ * - Driver supports up to I2C5 (OMAP5; I2C_BASE5 must be defined).
  */
 
 #include <common.h>
@@ -31,6 +39,12 @@  DECLARE_GLOBAL_DATA_PTR;
 
 #define I2C_TIMEOUT	1000
 
+#if defined(CONFIG_OMAP44XX) || defined(CONFIG_OMAP54XX)
+#define I2C_WAIT	200	/* Absolutely safe for status update at 100 kHz I2C */
+#else
+#define I2C_WAIT	1000
+#endif
+
 static int wait_for_bb(void);
 static u16 wait_for_pin(void);
 static void flush_fifo(void);
@@ -150,6 +164,7 @@  void i2c_init(int speed, int slaveadd)
 		bus_initialized[current_bus] = 1;
 }
 
+#if !(defined(CONFIG_OMAP44XX) || defined(CONFIG_OMAP54XX))
 static int i2c_read_byte(u8 devaddr, u16 regoffset, u8 alen, u8 *value)
 {
 	int i2c_error = 0;
@@ -231,6 +246,7 @@  read_exit:
 	writew(0, &i2c_base->cnt);
 	return i2c_error;
 }
+#endif
 
 static void flush_fifo(void)
 {	u16 stat;
@@ -255,6 +271,52 @@  static void flush_fifo(void)
 	}
 }
 
+#if (defined(CONFIG_OMAP44XX) || defined(CONFIG_OMAP54XX)) && \
+	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_pin();
+
+	if ((status & ~I2C_STAT_XRDY) == 0 || (status & I2C_STAT_AL))
+		goto pr_exit;			/* If pads are not configured for I2C */
+		
+	/* 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 cntrlr */
+		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
 int i2c_probe(uchar chip)
 {
 	u16 status;
@@ -314,7 +376,132 @@  probe_exit:
 	writew(0xFFFF, &i2c_base->stat);
 	return res;
 }
+#endif
+
+#if defined(CONFIG_OMAP44XX) || defined(CONFIG_OMAP54XX)
+/* 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 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);
+		return 1;
+	}
+
+	if (addr + len > (1 << 16)) {
+		puts("I2C read: address out of range\n");
+		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_pin();
+			/* Try to identify bus that is not padconf'd for I2C */
+			if (status == I2C_STAT_XRDY) {
+				i2c_error = 2;
+				printf("i2c_read: 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);
+
+	/* Receive data */
+	while (1) {
+		status = wait_for_pin();
+		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;
+}
+#else
 int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 {
 	int i;
@@ -339,7 +526,109 @@  int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 
 	return 0;
 }
+#endif
+
+#if defined(CONFIG_OMAP44XX) || defined(CONFIG_OMAP54XX)
+/* 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;
+
+	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;
+	}
+
+	if (addr + len > (1 << 16)) {
+		printf("I2C write: address 0x%x + 0x%x out of range\n",
+				addr, len);
+		return 1;
+	}
+
+	/* Wait until bus not busy */
+	if (wait_for_bb())
+		return 1;
+
+	/* Start address phase - will write regoffset + len bytes data */
+	writew(alen + len, &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);
 
+	while (alen) {
+		/* Must write reg offset (one or two bytes) */
+		status = wait_for_pin();
+		/* 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_write: error waiting for addr ACK (status=0x%x)\n",
+			      	status);
+			goto wr_exit;
+		}
+		if (status & I2C_STAT_XRDY) {
+			alen--;
+			writeb((addr >> (8 * alen)) & 0xff, &i2c_base->data);
+			writew(I2C_STAT_XRDY, &i2c_base->stat);
+		}
+		else {
+			i2c_error = 1;
+			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_pin();
+		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;
+		}
+	}
+
+wr_exit:
+	flush_fifo();
+	writew(0xFFFF, &i2c_base->stat);
+	writew(0, &i2c_base->cnt);
+	return i2c_error;
+}
+#else
 int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 {
 	int i;
@@ -404,6 +693,7 @@  write_exit:
 	writew(0xFFFF, &i2c_base->stat);
 	return i2c_error;
 }
+#endif
 
 static int wait_for_bb(void)
 {
@@ -413,7 +703,7 @@  static int wait_for_bb(void)
 	writew(0xFFFF, &i2c_base->stat);	/* clear current interrupts...*/
 	while ((stat = readw(&i2c_base->stat) & I2C_STAT_BB) && timeout--) {
 		writew(stat, &i2c_base->stat);
-		udelay(1000);
+		udelay(I2C_WAIT);
 	}
 
 	if (timeout <= 0) {
@@ -431,7 +721,7 @@  static u16 wait_for_pin(void)
 	int timeout = I2C_TIMEOUT;
 
 	do {
-		udelay(1000);
+		udelay(I2C_WAIT);
 		status = readw(&i2c_base->stat);
 	} while (!(status &
 		   (I2C_STAT_ROVR | I2C_STAT_XUDF | I2C_STAT_XRDY |
@@ -455,23 +745,31 @@  int i2c_set_bus_num(unsigned int 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) && defined(I2C_BASE5)
+		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;