[U-Boot] Better I2C support for OMAP

[U-Boot] Better I2C support for OMAP

[Tom Rix]

> 
> Hugo Vincent wrote:
> > Hi everyone,
> > 
> > This patch improves I2C support on OMAP3 platforms, including  
> > supporting the second and third I2C controllers (mainline only  
> > supports the first controller), and supporting writes where alen=0  
> > (i.e. no register address - this was needed for some I2C devices I  
> > needed to drive from u-boot).
> > 
> > Tested on Gumstix Overo with several I2C devices, on I2C1 and 3.
> > 
> > The multi-bus support is mostly from this patch:
> > http://www.beagleboard.org/gitweb/?p=u-boot-arm.git;a=commit;h=52eddcd07c2e7ad61d15bab2cf2d0d21466eaca2
> > 
> > Best regards,
> > Hugo Vincent
> > 
> > Signed-off-by: Hugo Vincent <hugo.vincent@gmail.com>
>
> Does anybody have an up to date version of this patch? Seems we need 
> it for Beagle Zippy, too.
> 
> Thanks
> 
> Dirk

I have cherry picked this change from the above git repo. 

There was some cleanup. 

The omap24xx version changes were done.

The structure elements were changed from unsigned int to 
unsigned shorts + pads to make the elements consistent with
the writew/readw 's used in the driver. 

The cmd_i2c changes that did not seem to fit were removed.

I have run tested this on beagle and zoom1.

Since I don't have a zippy board handy I did not get anything
meaningfull when switching to i2c dev 2.  The default i2c dev
worked as expected.

Tom

[U-Boot] [PATCH] Add I2C multibus support for OMAP2/3 boards

[Tom Rix]

From: Syed Mohammed Khasim <khasim@ti.com>

This was cherry-picked from

repo: http://www.beagleboard.org/u-boot-arm.git
commit: 52eddcd07c2e7ad61d15bab2cf2d0d21466eaca2

In addition to adding multibus support, this patch
also cleans up the register access.  The register
access has been changed from #defines to a structure.

Signed-off-by: Syed Mohammed Khasim <khasim@ti.com>
Signed-off-by: Jason Kridner <jkridner@beagleboard.org>
Signed-off-by: Tom Rix <Tom.Rix@windriver.com>
---
 drivers/i2c/omap24xx_i2c.c          |  220 ++++++++++++++++++++++-------------
 include/asm-arm/arch-omap24xx/i2c.h |   52 ++++++---
 include/asm-arm/arch-omap3/i2c.h    |   48 +++++---
 include/configs/omap3_beagle.h      |    1 +
 4 files changed, 209 insertions(+), 112 deletions(-)

diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
index 1a4c8c9..763d2f8 100644
--- a/drivers/i2c/omap24xx_i2c.c
+++ b/drivers/i2c/omap24xx_i2c.c
@@ -1,7 +1,7 @@
 /*
  * Basic I2C functions
  *
- * Copyright (c) 2004 Texas Instruments
+ * Copyright (c) 2004, 2009 Texas Instruments
  *
  * This package is free software;  you can redistribute it and/or
  * modify it under the terms of the license found in the file
@@ -25,10 +25,18 @@
 #include <asm/arch/i2c.h>
 #include <asm/io.h>
 
+#ifdef CONFIG_OMAP34XX
+#define I2C_NUM_IF 3
+#else
+#define I2C_NUM_IF 2
+#endif
+
 static void wait_for_bb (void);
 static u16 wait_for_pin (void);
 static void flush_fifo(void);
 
+static i2c_t *i2c = (i2c_t *)I2C_DEFAULT_BASE;
+
 void i2c_init (int speed, int slaveadd)
 {
 	int psc, fsscll, fssclh;
@@ -95,30 +103,30 @@ void i2c_init (int speed, int slaveadd)
 		sclh = (unsigned int)fssclh;
 	}
 
-	writew(0x2, I2C_SYSC); /* for ES2 after soft reset */
+	writew(0x2, &i2c->sysc);		/* for ES2 after soft reset */
 	udelay(1000);
-	writew(0x0, I2C_SYSC); /* will probably self clear but */
+	writew(0x0, &i2c->sysc);		/* will probably self clear but */
 
-	if (readw (I2C_CON) & I2C_CON_EN) {
-		writew (0, I2C_CON);
-		udelay (50000);
+	if (readw(&i2c->con) & I2C_CON_EN) {
+		writew(0, &i2c->con);
+		udelay(50000);
 	}
 
-	writew(psc, I2C_PSC);
-	writew(scll, I2C_SCLL);
-	writew(sclh, I2C_SCLH);
+	writew(psc, &i2c->psc);
+	writew(scll, &i2c->scll);
+	writew(sclh, &i2c->sclh);
 
 	/* own address */
-	writew (slaveadd, I2C_OA);
-	writew (I2C_CON_EN, I2C_CON);
+	writew(slaveadd, &i2c->oa);
+	writew(I2C_CON_EN, &i2c->con);
 
 	/* have to enable intrrupts or OMAP i2c module doesn't work */
-	writew (I2C_IE_XRDY_IE | I2C_IE_RRDY_IE | I2C_IE_ARDY_IE |
-		I2C_IE_NACK_IE | I2C_IE_AL_IE, I2C_IE);
-	udelay (1000);
+	writew(I2C_IE_XRDY_IE | I2C_IE_RRDY_IE | I2C_IE_ARDY_IE |
+		I2C_IE_NACK_IE | I2C_IE_AL_IE, &i2c->ie);
+	udelay(1000);
 	flush_fifo();
-	writew (0xFFFF, I2C_STAT);
-	writew (0, I2C_CNT);
+	writew(0xFFFF, &i2c->stat);
+	writew(0, &i2c->cnt);
 }
 
 static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
@@ -130,19 +138,19 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
 	wait_for_bb ();
 
 	/* one byte only */
-	writew (1, I2C_CNT);
+	writew(1, &i2c->cnt);
 	/* set slave address */
-	writew (devaddr, I2C_SA);
+	writew(devaddr, &i2c->sa);
 	/* no stop bit needed here */
-	writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX, I2C_CON);
+	writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX, &i2c->con);
 
 	status = wait_for_pin ();
 
 	if (status & I2C_STAT_XRDY) {
 		/* Important: have to use byte access */
-		writeb (regoffset, I2C_DATA);
-		udelay (20000);
-		if (readw (I2C_STAT) & I2C_STAT_NACK) {
+		writeb(regoffset, &i2c->data);
+		udelay(20000);
+		if (readw(&i2c->stat) & I2C_STAT_NACK) {
 			i2c_error = 1;
 		}
 	} else {
@@ -151,28 +159,28 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
 
 	if (!i2c_error) {
 		/* free bus, otherwise we can't use a combined transction */
-		writew (0, I2C_CON);
-		while (readw (I2C_STAT) || (readw (I2C_CON) & I2C_CON_MST)) {
+		writew(0, &i2c->con);
+		while (readw(&i2c->stat) || (readw(&i2c->con) & I2C_CON_MST)) {
 			udelay (10000);
 			/* Have to clear pending interrupt to clear I2C_STAT */
-			writew (0xFFFF, I2C_STAT);
+			writew(0xFFFF, &i2c->stat);
 		}
 
 		wait_for_bb ();
 		/* set slave address */
-		writew (devaddr, I2C_SA);
+		writew(devaddr, &i2c->sa);
 		/* read one byte from slave */
-		writew (1, I2C_CNT);
+		writew(1, &i2c->cnt);
 		/* need stop bit here */
-		writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP,
-			I2C_CON);
+		writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP,
+			&i2c->con);
 
 		status = wait_for_pin ();
 		if (status & I2C_STAT_RRDY) {
 #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
-			*value = readb (I2C_DATA);
+			*value = readb(&i2c->data);
 #else
-			*value = readw (I2C_DATA);
+			*value = readw(&i2c->data);
 #endif
 			udelay (20000);
 		} else {
@@ -180,17 +188,17 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
 		}
 
 		if (!i2c_error) {
-			writew (I2C_CON_EN, I2C_CON);
-			while (readw (I2C_STAT)
-			       || (readw (I2C_CON) & I2C_CON_MST)) {
+			writew(I2C_CON_EN, &i2c->con);
+			while (readw(&i2c->stat)
+			       || (readw(&i2c->con) & I2C_CON_MST)) {
 				udelay (10000);
-				writew (0xFFFF, I2C_STAT);
+				writew(0xFFFF, &i2c->stat);
 			}
 		}
 	}
 	flush_fifo();
-	writew (0xFFFF, I2C_STAT);
-	writew (0, I2C_CNT);
+	writew(0xFFFF, &i2c->stat);
+	writew(0, &i2c->cnt);
 	return i2c_error;
 }
 
@@ -203,12 +211,12 @@ static int i2c_write_byte (u8 devaddr, u8 regoffset, u8 value)
 	wait_for_bb ();
 
 	/* two bytes */
-	writew (2, I2C_CNT);
+	writew(2, &i2c->cnt);
 	/* set slave address */
-	writew (devaddr, I2C_SA);
+	writew(devaddr, &i2c->sa);
 	/* stop bit needed here */
-	writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX |
-		I2C_CON_STP, I2C_CON);
+	writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX |
+		I2C_CON_STP, &i2c->con);
 
 	/* wait until state change */
 	status = wait_for_pin ();
@@ -216,24 +224,24 @@ static int i2c_write_byte (u8 devaddr, u8 regoffset, u8 value)
 	if (status & I2C_STAT_XRDY) {
 #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
 		/* send out 1 byte */
-		writeb (regoffset, I2C_DATA);
-		writew (I2C_STAT_XRDY, I2C_STAT);
+		writeb(regoffset, &i2c->data);
+		writew(I2C_STAT_XRDY, &i2c->stat);
 
 		status = wait_for_pin ();
 		if ((status & I2C_STAT_XRDY)) {
 			/* send out next 1 byte */
-			writeb (value, I2C_DATA);
-			writew (I2C_STAT_XRDY, I2C_STAT);
+			writeb(value, &i2c->data);
+			writew(I2C_STAT_XRDY, &i2c->stat);
 		} else {
 			i2c_error = 1;
 		}
 #else
 		/* send out two bytes */
-		writew ((value << 8) + regoffset, I2C_DATA);
+		writew((value << 8) + regoffset, &i2c->data);
 #endif
 		/* must have enough delay to allow BB bit to go low */
-		udelay (50000);
-		if (readw (I2C_STAT) & I2C_STAT_NACK) {
+		udelay(50000);
+		if (readw(&i2c->stat) & I2C_STAT_NACK) {
 			i2c_error = 1;
 		}
 	} else {
@@ -243,18 +251,18 @@ static int i2c_write_byte (u8 devaddr, u8 regoffset, u8 value)
 	if (!i2c_error) {
 		int eout = 200;
 
-		writew (I2C_CON_EN, I2C_CON);
-		while ((stat = readw (I2C_STAT)) || (readw (I2C_CON) & I2C_CON_MST)) {
-			udelay (1000);
+		writew(I2C_CON_EN, &i2c->con);
+		while ((stat = readw(&i2c->stat)) || (readw(&i2c->con) & I2C_CON_MST)) {
+			udelay(1000);
 			/* have to read to clear intrrupt */
-			writew (0xFFFF, I2C_STAT);
+			writew(0xFFFF, &i2c->stat);
 			if(--eout == 0) /* better leave with error than hang */
 				break;
 		}
 	}
 	flush_fifo();
-	writew (0xFFFF, I2C_STAT);
-	writew (0, I2C_CNT);
+	writew(0xFFFF, &i2c->stat);
+	writew(0, &i2c->cnt);
 	return i2c_error;
 }
 
@@ -265,14 +273,14 @@ static void flush_fifo(void)
 	 * you get a bus error
 	 */
 	while(1){
-		stat = readw(I2C_STAT);
+		stat = readw(&i2c->stat);
 		if(stat == I2C_STAT_RRDY){
 #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
-			readb(I2C_DATA);
+			readb(&i2c->data);
 #else
-			readw(I2C_DATA);
+			readw(&i2c->data);
 #endif
-			writew(I2C_STAT_RRDY,I2C_STAT);
+			writew(I2C_STAT_RRDY, &i2c->stat);
 			udelay(1000);
 		}else
 			break;
@@ -283,7 +291,7 @@ int i2c_probe (uchar chip)
 {
 	int res = 1; /* default = fail */
 
-	if (chip == readw (I2C_OA)) {
+	if (chip == readw(&i2c->oa)) {
 		return res;
 	}
 
@@ -291,27 +299,27 @@ int i2c_probe (uchar chip)
 	wait_for_bb ();
 
 	/* try to read one byte */
-	writew (1, I2C_CNT);
+	writew(1, &i2c->cnt);
 	/* set slave address */
-	writew (chip, I2C_SA);
+	writew(chip, &i2c->sa);
 	/* stop bit needed here */
-	writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP, I2C_CON);
+	writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP, &i2c->con);
 	/* enough delay for the NACK bit set */
-	udelay (50000);
+	udelay(50000);
 
-	if (!(readw (I2C_STAT) & I2C_STAT_NACK)) {
+	if (!(readw(&i2c->stat) & I2C_STAT_NACK)) {
 		res = 0;      /* success case */
 		flush_fifo();
-		writew(0xFFFF, I2C_STAT);
+		writew(0xFFFF, &i2c->stat);
 	} else {
-		writew(0xFFFF, I2C_STAT);	 /* failue, clear sources*/
-		writew (readw (I2C_CON) | I2C_CON_STP, I2C_CON); /* finish up xfer */
+		writew(0xFFFF, &i2c->stat);	 /* failue, clear sources*/
+		writew(readw(&i2c->con) | I2C_CON_STP, &i2c->con); /* finish up xfer */
 		udelay(20000);
 		wait_for_bb ();
 	}
 	flush_fifo();
-	writew (0, I2C_CNT); /* don't allow any more data in...we don't want it.*/
-	writew(0xFFFF, I2C_STAT);
+	writew(0, &i2c->cnt); /* don't allow any more data in...we don't want it.*/
+	writew(0xFFFF, &i2c->stat);
 	return res;
 }
 
@@ -345,18 +353,18 @@ int i2c_write (uchar chip, uint addr, int alen, uchar * buffer, int len)
 	int i;
 
 	if (alen > 1) {
-		printf ("I2C read: addr len %d not supported\n", alen);
+		printf ("I2C write: addr len %d not supported\n", alen);
 		return 1;
 	}
 
 	if (addr + len > 256) {
-		printf ("I2C read: address out of range\n");
+		printf ("I2C write: address out of range\n");
 		return 1;
 	}
 
 	for (i = 0; i < len; i++) {
 		if (i2c_write_byte (chip, addr + i, buffer[i])) {
-			printf ("I2C read: I/O error\n");
+			printf ("I2C write: I/O error\n");
 			i2c_init (CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
 			return 1;
 		}
@@ -370,17 +378,17 @@ static void wait_for_bb (void)
 	int timeout = 10;
 	u16 stat;
 
-	writew(0xFFFF, I2C_STAT);	 /* clear current interruts...*/
-	while ((stat = readw (I2C_STAT) & I2C_STAT_BB) && timeout--) {
-		writew (stat, I2C_STAT);
-		udelay (50000);
+	writew(0xFFFF, &i2c->stat);	 /* clear current interruts...*/
+	while ((stat = readw(&i2c->stat) & I2C_STAT_BB) && timeout--) {
+		writew(stat, &i2c->stat);
+		udelay(50000);
 	}
 
 	if (timeout <= 0) {
 		printf ("timed out in wait_for_bb: I2C_STAT=%x\n",
-			readw (I2C_STAT));
+			readw(&i2c->stat));
 	}
-	writew(0xFFFF, I2C_STAT);	 /* clear delayed stuff*/
+	writew(0xFFFF, &i2c->stat);	 /* clear delayed stuff*/
 }
 
 static u16 wait_for_pin (void)
@@ -390,7 +398,7 @@ static u16 wait_for_pin (void)
 
 	do {
 		udelay (1000);
-		status = readw (I2C_STAT);
+		status = readw(&i2c->stat);
 	} while (  !(status &
 		   (I2C_STAT_ROVR | I2C_STAT_XUDF | I2C_STAT_XRDY |
 		    I2C_STAT_RRDY | I2C_STAT_ARDY | I2C_STAT_NACK |
@@ -398,8 +406,58 @@ static u16 wait_for_pin (void)
 
 	if (timeout <= 0) {
 		printf ("timed out in wait_for_pin: I2C_STAT=%x\n",
-			readw (I2C_STAT));
-			writew(0xFFFF, I2C_STAT);
+			readw(&i2c->stat));
+			writew(0xFFFF, &i2c->stat);
 }
 	return status;
 }
+
+int i2c_set_bus_num(unsigned int bus)
+{
+	if ((bus < 0) || (bus >= I2C_NUM_IF)) {
+		printf("Bad bus ID-%d\n", bus);
+		return -1;
+	}
+
+#if defined(CONFIG_OMAP34XX)
+	if (bus == 2)
+		i2c = (i2c_t *)I2C_BASE3;
+	else
+#endif
+	if (bus == 1)
+		i2c = (i2c_t *)I2C_BASE2;
+	else
+		i2c = (i2c_t *)I2C_BASE1;
+
+	return 0;
+}
+
+unsigned int i2c_get_bus_num(void)
+{
+	if (i2c == (i2c_t *)I2C_BASE1)
+		return 0;
+	else
+	if (i2c == (i2c_t *)I2C_BASE2)
+		return 1;
+#if defined(CONFIG_OMAP34XX)
+	else
+	if (i2c == (i2c_t *)I2C_BASE3)
+		return 2;
+#endif
+	else
+		return 0xFFFFFFFF;
+}
+
+/*
+ * To be Implemented
+ */
+int i2c_set_bus_speed(unsigned int speed)
+{
+	return 0;
+}
+
+unsigned int i2c_get_bus_speed(void)
+{
+	return 0;
+}
+
diff --git a/include/asm-arm/arch-omap24xx/i2c.h b/include/asm-arm/arch-omap24xx/i2c.h
index 44db7a2..832a0e1 100644
--- a/include/asm-arm/arch-omap24xx/i2c.h
+++ b/include/asm-arm/arch-omap24xx/i2c.h
@@ -23,24 +23,44 @@
 #ifndef _OMAP24XX_I2C_H_
 #define _OMAP24XX_I2C_H_
 
-#define I2C_BASE                0x48070000
+#define I2C_BASE1               0x48070000
 #define I2C_BASE2               0x48072000 /* nothing hooked up on h4 */
 
-#define I2C_REV                 (I2C_BASE + 0x00)
-#define I2C_IE                  (I2C_BASE + 0x04)
-#define I2C_STAT                (I2C_BASE + 0x08)
-#define I2C_IV                  (I2C_BASE + 0x0c)
-#define I2C_BUF                 (I2C_BASE + 0x14)
-#define I2C_CNT                 (I2C_BASE + 0x18)
-#define I2C_DATA                (I2C_BASE + 0x1c)
-#define I2C_SYSC                (I2C_BASE + 0x20)
-#define I2C_CON                 (I2C_BASE + 0x24)
-#define I2C_OA                  (I2C_BASE + 0x28)
-#define I2C_SA                  (I2C_BASE + 0x2c)
-#define I2C_PSC                 (I2C_BASE + 0x30)
-#define I2C_SCLL                (I2C_BASE + 0x34)
-#define I2C_SCLH                (I2C_BASE + 0x38)
-#define I2C_SYSTEST             (I2C_BASE + 0x3c)
+#define I2C_DEFAULT_BASE	I2C_BASE1
+
+typedef struct i2c {
+	unsigned short rev;		/* 0x00 */
+	unsigned short pad_rev;		/* 0x02 */
+	unsigned short ie;		/* 0x04 */
+	unsigned short pad_ie;		/* 0x06 */
+	unsigned short stat;		/* 0x08 */
+	unsigned short pad_stat;	/* 0x0a */
+	unsigned short iv;		/* 0x0c */
+	unsigned short pad_iv;		/* 0x0e */
+	unsigned int res1;		/* 0x10 */
+	unsigned short buf;		/* 0x14 */
+	unsigned short pad_buf;		/* 0x16 */
+	unsigned short cnt;		/* 0x18 */
+	unsigned short pad_cnt;		/* 0x1a */
+	unsigned short data;		/* 0x1c */
+	unsigned short pad_data;	/* 0x1e */
+	unsigned short sysc;		/* 0x20 */
+	unsigned short pad_sysc;	/* 0x22 */
+	unsigned short con;		/* 0x24 */
+	unsigned short pad_con;		/* 0x26 */
+	unsigned short oa;		/* 0x28 */
+	unsigned short pad_oa;		/* 0x2a */
+	unsigned short sa;		/* 0x2c */
+	unsigned short pad_sa;		/* 0x2e */
+	unsigned short psc;		/* 0x30 */
+	unsigned short pad_psc;		/* 0x32 */
+	unsigned short scll;		/* 0x34 */
+	unsigned short pad_scll;	/* 0x36 */
+	unsigned short sclh;		/* 0x38 */
+	unsigned short pad_sclh;	/* 0x3a */
+	unsigned short systest;		/* 0x3c */
+	unsigned short pad_systest;	/* 0x3e */
+} i2c_t;
 
 /* I2C masks */
 
diff --git a/include/asm-arm/arch-omap3/i2c.h b/include/asm-arm/arch-omap3/i2c.h
index 8b339cc..3da1fcf 100644
--- a/include/asm-arm/arch-omap3/i2c.h
+++ b/include/asm-arm/arch-omap3/i2c.h
@@ -25,21 +25,39 @@
 
 #define I2C_DEFAULT_BASE	I2C_BASE1
 
-#define I2C_REV		(I2C_DEFAULT_BASE + 0x00)
-#define I2C_IE 		(I2C_DEFAULT_BASE + 0x04)
-#define I2C_STAT	(I2C_DEFAULT_BASE + 0x08)
-#define I2C_IV 		(I2C_DEFAULT_BASE + 0x0c)
-#define I2C_BUF		(I2C_DEFAULT_BASE + 0x14)
-#define I2C_CNT		(I2C_DEFAULT_BASE + 0x18)
-#define I2C_DATA	(I2C_DEFAULT_BASE + 0x1c)
-#define I2C_SYSC	(I2C_DEFAULT_BASE + 0x20)
-#define I2C_CON		(I2C_DEFAULT_BASE + 0x24)
-#define I2C_OA 		(I2C_DEFAULT_BASE + 0x28)
-#define I2C_SA 		(I2C_DEFAULT_BASE + 0x2c)
-#define I2C_PSC		(I2C_DEFAULT_BASE + 0x30)
-#define I2C_SCLL	(I2C_DEFAULT_BASE + 0x34)
-#define I2C_SCLH	(I2C_DEFAULT_BASE + 0x38)
-#define I2C_SYSTEST	(I2C_DEFAULT_BASE + 0x3c)
+typedef struct i2c {
+	unsigned short rev;		/* 0x00 */
+	unsigned short pad_rev;		/* 0x02 */
+	unsigned short ie;		/* 0x04 */
+	unsigned short pad_ie;		/* 0x06 */
+	unsigned short stat;		/* 0x08 */
+	unsigned short pad_stat;	/* 0x0a */
+	unsigned short iv;		/* 0x0c */
+	unsigned short pad_iv;		/* 0x0e */
+	unsigned int res1;		/* 0x10 */
+	unsigned short buf;		/* 0x14 */
+	unsigned short pad_buf;		/* 0x16 */
+	unsigned short cnt;		/* 0x18 */
+	unsigned short pad_cnt;		/* 0x1a */
+	unsigned short data;		/* 0x1c */
+	unsigned short pad_data;	/* 0x1e */
+	unsigned short sysc;		/* 0x20 */
+	unsigned short pad_sysc;	/* 0x22 */
+	unsigned short con;		/* 0x24 */
+	unsigned short pad_con;		/* 0x26 */
+	unsigned short oa;		/* 0x28 */
+	unsigned short pad_oa;		/* 0x2a */
+	unsigned short sa;		/* 0x2c */
+	unsigned short pad_sa;		/* 0x2e */
+	unsigned short psc;		/* 0x30 */
+	unsigned short pad_psc;		/* 0x32 */
+	unsigned short scll;		/* 0x34 */
+	unsigned short pad_scll;	/* 0x36 */
+	unsigned short sclh;		/* 0x38 */
+	unsigned short pad_sclh;	/* 0x3a */
+	unsigned short systest;		/* 0x3c */
+	unsigned short pad_systest;	/* 0x3e */
+} i2c_t;
 
 /* I2C masks */
 
diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h
index 19a5ec9..d5a0d49 100644
--- a/include/configs/omap3_beagle.h
+++ b/include/configs/omap3_beagle.h
@@ -128,6 +128,7 @@
 #define CONFIG_SYS_I2C_BUS		0
 #define CONFIG_SYS_I2C_BUS_SELECT	1
 #define CONFIG_DRIVER_OMAP34XX_I2C	1
+#define CONFIG_I2C_MULTI_BUS		1
 
 /*
  * TWL4030
-- 
1.6.0.6

[U-Boot] [PATCH] Add I2C multibus support for OMAP2/3 boards

[Dirk Behme]

Tom Rix wrote:
> From: Syed Mohammed Khasim <khasim@ti.com>
> 
> This was cherry-picked from
> 
> repo: http://www.beagleboard.org/u-boot-arm.git
> commit: 52eddcd07c2e7ad61d15bab2cf2d0d21466eaca2
> 
> In addition to adding multibus support, this patch
> also cleans up the register access.  The register
> access has been changed from #defines to a structure.

Have you looked at my proposal I sent some hours before your patch?

http://lists.denx.de/pipermail/u-boot/2009-October/063556.html

> Signed-off-by: Syed Mohammed Khasim <khasim@ti.com>
> Signed-off-by: Jason Kridner <jkridner@beagleboard.org>
> Signed-off-by: Tom Rix <Tom.Rix@windriver.com>
> ---
>  drivers/i2c/omap24xx_i2c.c          |  220 ++++++++++++++++++++++-------------
>  include/asm-arm/arch-omap24xx/i2c.h |   52 ++++++---
>  include/asm-arm/arch-omap3/i2c.h    |   48 +++++---
>  include/configs/omap3_beagle.h      |    1 +
>  4 files changed, 209 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
> index 1a4c8c9..763d2f8 100644
> --- a/drivers/i2c/omap24xx_i2c.c
> +++ b/drivers/i2c/omap24xx_i2c.c
> @@ -1,7 +1,7 @@
>  /*
>   * Basic I2C functions
>   *
> - * Copyright (c) 2004 Texas Instruments
> + * Copyright (c) 2004, 2009 Texas Instruments
>   *
>   * This package is free software;  you can redistribute it and/or
>   * modify it under the terms of the license found in the file
> @@ -25,10 +25,18 @@
>  #include <asm/arch/i2c.h>
>  #include <asm/io.h>
>  
> +#ifdef CONFIG_OMAP34XX
> +#define I2C_NUM_IF 3
> +#else
> +#define I2C_NUM_IF 2
> +#endif

I prefer something like I2C_BUS_MAX for this. And move it to header 
file. Moving it OMAP2 and OMAP3 i2c.h headers will remove the #ifdef, too.

>  static void wait_for_bb (void);
>  static u16 wait_for_pin (void);
>  static void flush_fifo(void);
>  
> +static i2c_t *i2c = (i2c_t *)I2C_DEFAULT_BASE;

Looking at the other OMAP3 files, the recent syntax seems to be

static struct i2c *i2c_base = (struct i2c *)I2C_DEFAULT_BASE;

Just to be consistent.

>  void i2c_init (int speed, int slaveadd)
>  {
>  	int psc, fsscll, fssclh;
> @@ -95,30 +103,30 @@ void i2c_init (int speed, int slaveadd)
>  		sclh = (unsigned int)fssclh;
>  	}
>  
> -	writew(0x2, I2C_SYSC); /* for ES2 after soft reset */
> +	writew(0x2, &i2c->sysc);		/* for ES2 after soft reset */
>  	udelay(1000);
> -	writew(0x0, I2C_SYSC); /* will probably self clear but */
> +	writew(0x0, &i2c->sysc);		/* will probably self clear but */
>  
> -	if (readw (I2C_CON) & I2C_CON_EN) {
> -		writew (0, I2C_CON);
> -		udelay (50000);
> +	if (readw(&i2c->con) & I2C_CON_EN) {
> +		writew(0, &i2c->con);
> +		udelay(50000);

udelay: Are this formatting changes? You are correct, a lot of 
formatting improvements are needed here. But they should go to an 
other patch.

> -	writew(psc, I2C_PSC);
> -	writew(scll, I2C_SCLL);
> -	writew(sclh, I2C_SCLH);
> +	writew(psc, &i2c->psc);
> +	writew(scll, &i2c->scll);
> +	writew(sclh, &i2c->sclh);
>  
>  	/* own address */
> -	writew (slaveadd, I2C_OA);
> -	writew (I2C_CON_EN, I2C_CON);
> +	writew(slaveadd, &i2c->oa);
> +	writew(I2C_CON_EN, &i2c->con);
>  
>  	/* have to enable intrrupts or OMAP i2c module doesn't work */
> -	writew (I2C_IE_XRDY_IE | I2C_IE_RRDY_IE | I2C_IE_ARDY_IE |
> -		I2C_IE_NACK_IE | I2C_IE_AL_IE, I2C_IE);
> -	udelay (1000);
> +	writew(I2C_IE_XRDY_IE | I2C_IE_RRDY_IE | I2C_IE_ARDY_IE |
> +		I2C_IE_NACK_IE | I2C_IE_AL_IE, &i2c->ie);
> +	udelay(1000);

Formatting change?

>  	flush_fifo();
> -	writew (0xFFFF, I2C_STAT);
> -	writew (0, I2C_CNT);
> +	writew(0xFFFF, &i2c->stat);
> +	writew(0, &i2c->cnt);
>  }
>  
>  static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
> @@ -130,19 +138,19 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
>  	wait_for_bb ();
>  
>  	/* one byte only */
> -	writew (1, I2C_CNT);
> +	writew(1, &i2c->cnt);
>  	/* set slave address */
> -	writew (devaddr, I2C_SA);
> +	writew(devaddr, &i2c->sa);
>  	/* no stop bit needed here */
> -	writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX, I2C_CON);
> +	writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX, &i2c->con);
>  
>  	status = wait_for_pin ();
>  
>  	if (status & I2C_STAT_XRDY) {
>  		/* Important: have to use byte access */
> -		writeb (regoffset, I2C_DATA);
> -		udelay (20000);
> -		if (readw (I2C_STAT) & I2C_STAT_NACK) {
> +		writeb(regoffset, &i2c->data);
> +		udelay(20000);

Formatting change?

> +		if (readw(&i2c->stat) & I2C_STAT_NACK) {
>  			i2c_error = 1;
>  		}
>  	} else {
> @@ -151,28 +159,28 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
>  
>  	if (!i2c_error) {
>  		/* free bus, otherwise we can't use a combined transction */
> -		writew (0, I2C_CON);
> -		while (readw (I2C_STAT) || (readw (I2C_CON) & I2C_CON_MST)) {
> +		writew(0, &i2c->con);
> +		while (readw(&i2c->stat) || (readw(&i2c->con) & I2C_CON_MST)) {
>  			udelay (10000);
>  			/* Have to clear pending interrupt to clear I2C_STAT */
> -			writew (0xFFFF, I2C_STAT);
> +			writew(0xFFFF, &i2c->stat);
>  		}
>  
>  		wait_for_bb ();
>  		/* set slave address */
> -		writew (devaddr, I2C_SA);
> +		writew(devaddr, &i2c->sa);
>  		/* read one byte from slave */
> -		writew (1, I2C_CNT);
> +		writew(1, &i2c->cnt);
>  		/* need stop bit here */
> -		writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP,
> -			I2C_CON);
> +		writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP,
> +			&i2c->con);
>  
>  		status = wait_for_pin ();
>  		if (status & I2C_STAT_RRDY) {
>  #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
> -			*value = readb (I2C_DATA);
> +			*value = readb(&i2c->data);
>  #else
> -			*value = readw (I2C_DATA);
> +			*value = readw(&i2c->data);
>  #endif
>  			udelay (20000);
>  		} else {
> @@ -180,17 +188,17 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
>  		}
>  
>  		if (!i2c_error) {
> -			writew (I2C_CON_EN, I2C_CON);
> -			while (readw (I2C_STAT)
> -			       || (readw (I2C_CON) & I2C_CON_MST)) {
> +			writew(I2C_CON_EN, &i2c->con);
> +			while (readw(&i2c->stat)
> +			       || (readw(&i2c->con) & I2C_CON_MST)) {
>  				udelay (10000);
> -				writew (0xFFFF, I2C_STAT);
> +				writew(0xFFFF, &i2c->stat);
>  			}
>  		}
>  	}
>  	flush_fifo();
> -	writew (0xFFFF, I2C_STAT);
> -	writew (0, I2C_CNT);
> +	writew(0xFFFF, &i2c->stat);
> +	writew(0, &i2c->cnt);
>  	return i2c_error;
>  }
>  
> @@ -203,12 +211,12 @@ static int i2c_write_byte (u8 devaddr, u8 regoffset, u8 value)
>  	wait_for_bb ();
>  
>  	/* two bytes */
> -	writew (2, I2C_CNT);
> +	writew(2, &i2c->cnt);
>  	/* set slave address */
> -	writew (devaddr, I2C_SA);
> +	writew(devaddr, &i2c->sa);
>  	/* stop bit needed here */
> -	writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX |
> -		I2C_CON_STP, I2C_CON);
> +	writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX |
> +		I2C_CON_STP, &i2c->con);
>  
>  	/* wait until state change */
>  	status = wait_for_pin ();
> @@ -216,24 +224,24 @@ static int i2c_write_byte (u8 devaddr, u8 regoffset, u8 value)
>  	if (status & I2C_STAT_XRDY) {
>  #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
>  		/* send out 1 byte */
> -		writeb (regoffset, I2C_DATA);
> -		writew (I2C_STAT_XRDY, I2C_STAT);
> +		writeb(regoffset, &i2c->data);
> +		writew(I2C_STAT_XRDY, &i2c->stat);
>  
>  		status = wait_for_pin ();
>  		if ((status & I2C_STAT_XRDY)) {
>  			/* send out next 1 byte */
> -			writeb (value, I2C_DATA);
> -			writew (I2C_STAT_XRDY, I2C_STAT);
> +			writeb(value, &i2c->data);
> +			writew(I2C_STAT_XRDY, &i2c->stat);
>  		} else {
>  			i2c_error = 1;
>  		}
>  #else
>  		/* send out two bytes */
> -		writew ((value << 8) + regoffset, I2C_DATA);
> +		writew((value << 8) + regoffset, &i2c->data);
>  #endif
>  		/* must have enough delay to allow BB bit to go low */
> -		udelay (50000);
> -		if (readw (I2C_STAT) & I2C_STAT_NACK) {
> +		udelay(50000);
> +		if (readw(&i2c->stat) & I2C_STAT_NACK) {
>  			i2c_error = 1;
>  		}
>  	} else {
> @@ -243,18 +251,18 @@ static int i2c_write_byte (u8 devaddr, u8 regoffset, u8 value)
>  	if (!i2c_error) {
>  		int eout = 200;
>  
> -		writew (I2C_CON_EN, I2C_CON);
> -		while ((stat = readw (I2C_STAT)) || (readw (I2C_CON) & I2C_CON_MST)) {
> -			udelay (1000);
> +		writew(I2C_CON_EN, &i2c->con);
> +		while ((stat = readw(&i2c->stat)) || (readw(&i2c->con) & I2C_CON_MST)) {
> +			udelay(1000);

Formatting change?

>  			/* have to read to clear intrrupt */
> -			writew (0xFFFF, I2C_STAT);
> +			writew(0xFFFF, &i2c->stat);
>  			if(--eout == 0) /* better leave with error than hang */
>  				break;
>  		}
>  	}
>  	flush_fifo();
> -	writew (0xFFFF, I2C_STAT);
> -	writew (0, I2C_CNT);
> +	writew(0xFFFF, &i2c->stat);
> +	writew(0, &i2c->cnt);
>  	return i2c_error;
>  }
>  
> @@ -265,14 +273,14 @@ static void flush_fifo(void)
>  	 * you get a bus error
>  	 */
>  	while(1){
> -		stat = readw(I2C_STAT);
> +		stat = readw(&i2c->stat);
>  		if(stat == I2C_STAT_RRDY){
>  #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
> -			readb(I2C_DATA);
> +			readb(&i2c->data);
>  #else
> -			readw(I2C_DATA);
> +			readw(&i2c->data);
>  #endif
> -			writew(I2C_STAT_RRDY,I2C_STAT);
> +			writew(I2C_STAT_RRDY, &i2c->stat);
>  			udelay(1000);
>  		}else
>  			break;
> @@ -283,7 +291,7 @@ int i2c_probe (uchar chip)
>  {
>  	int res = 1; /* default = fail */
>  
> -	if (chip == readw (I2C_OA)) {
> +	if (chip == readw(&i2c->oa)) {
>  		return res;
>  	}
>  
> @@ -291,27 +299,27 @@ int i2c_probe (uchar chip)
>  	wait_for_bb ();
>  
>  	/* try to read one byte */
> -	writew (1, I2C_CNT);
> +	writew(1, &i2c->cnt);
>  	/* set slave address */
> -	writew (chip, I2C_SA);
> +	writew(chip, &i2c->sa);
>  	/* stop bit needed here */
> -	writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP, I2C_CON);
> +	writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP, &i2c->con);
>  	/* enough delay for the NACK bit set */
> -	udelay (50000);
> +	udelay(50000);

Formatting change?

> -	if (!(readw (I2C_STAT) & I2C_STAT_NACK)) {
> +	if (!(readw(&i2c->stat) & I2C_STAT_NACK)) {
>  		res = 0;      /* success case */
>  		flush_fifo();
> -		writew(0xFFFF, I2C_STAT);
> +		writew(0xFFFF, &i2c->stat);
>  	} else {
> -		writew(0xFFFF, I2C_STAT);	 /* failue, clear sources*/
> -		writew (readw (I2C_CON) | I2C_CON_STP, I2C_CON); /* finish up xfer */
> +		writew(0xFFFF, &i2c->stat);	 /* failue, clear sources*/
> +		writew(readw(&i2c->con) | I2C_CON_STP, &i2c->con); /* finish up xfer */
>  		udelay(20000);
>  		wait_for_bb ();
>  	}
>  	flush_fifo();
> -	writew (0, I2C_CNT); /* don't allow any more data in...we don't want it.*/
> -	writew(0xFFFF, I2C_STAT);
> +	writew(0, &i2c->cnt); /* don't allow any more data in...we don't want it.*/
> +	writew(0xFFFF, &i2c->stat);
>  	return res;
>  }
>  
> @@ -345,18 +353,18 @@ int i2c_write (uchar chip, uint addr, int alen, uchar * buffer, int len)
>  	int i;
>  
>  	if (alen > 1) {
> -		printf ("I2C read: addr len %d not supported\n", alen);
> +		printf ("I2C write: addr len %d not supported\n", alen);
>  		return 1;
>  	}
>  
>  	if (addr + len > 256) {
> -		printf ("I2C read: address out of range\n");
> +		printf ("I2C write: address out of range\n");

Here and above, typo change?

>  		return 1;
>  	}
>  
>  	for (i = 0; i < len; i++) {
>  		if (i2c_write_byte (chip, addr + i, buffer[i])) {
> -			printf ("I2C read: I/O error\n");
> +			printf ("I2C write: I/O error\n");

Typo change?

>  			i2c_init (CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
>  			return 1;
>  		}
> @@ -370,17 +378,17 @@ static void wait_for_bb (void)
>  	int timeout = 10;
>  	u16 stat;
>  
> -	writew(0xFFFF, I2C_STAT);	 /* clear current interruts...*/
> -	while ((stat = readw (I2C_STAT) & I2C_STAT_BB) && timeout--) {
> -		writew (stat, I2C_STAT);
> -		udelay (50000);

Formatting change?

> +	writew(0xFFFF, &i2c->stat);	 /* clear current interruts...*/
> +	while ((stat = readw(&i2c->stat) & I2C_STAT_BB) && timeout--) {
> +		writew(stat, &i2c->stat);
> +		udelay(50000);
>  	}
>  
>  	if (timeout <= 0) {
>  		printf ("timed out in wait_for_bb: I2C_STAT=%x\n",
> -			readw (I2C_STAT));
> +			readw(&i2c->stat));
>  	}
> -	writew(0xFFFF, I2C_STAT);	 /* clear delayed stuff*/
> +	writew(0xFFFF, &i2c->stat);	 /* clear delayed stuff*/
>  }
>  
>  static u16 wait_for_pin (void)
> @@ -390,7 +398,7 @@ static u16 wait_for_pin (void)
>  
>  	do {
>  		udelay (1000);
> -		status = readw (I2C_STAT);
> +		status = readw(&i2c->stat);
>  	} while (  !(status &
>  		   (I2C_STAT_ROVR | I2C_STAT_XUDF | I2C_STAT_XRDY |
>  		    I2C_STAT_RRDY | I2C_STAT_ARDY | I2C_STAT_NACK |
> @@ -398,8 +406,58 @@ static u16 wait_for_pin (void)
>  
>  	if (timeout <= 0) {
>  		printf ("timed out in wait_for_pin: I2C_STAT=%x\n",
> -			readw (I2C_STAT));
> -			writew(0xFFFF, I2C_STAT);
> +			readw(&i2c->stat));
> +			writew(0xFFFF, &i2c->stat);
>  }
>  	return status;
>  }
> +
> +int i2c_set_bus_num(unsigned int bus)

Do we need an extern declaration in i2c.h for this? To be able to call 
it from somewhere else without warning?

> +{
> +	if ((bus < 0) || (bus >= I2C_NUM_IF)) {

As mentioned above, I'd like something like bus max here.

> +		printf("Bad bus ID-%d\n", bus);
> +		return -1;
> +	}
> +
> +#if defined(CONFIG_OMAP34XX)
> +	if (bus == 2)
> +		i2c = (i2c_t *)I2C_BASE3;
> +	else
> +#endif
> +	if (bus == 1)
> +		i2c = (i2c_t *)I2C_BASE2;
> +	else
> +		i2c = (i2c_t *)I2C_BASE1;
> +
> +	return 0;
> +}

I would remove the following three functions as they are not needed at 
the moment (i.e. without command line interface).

> +unsigned int i2c_get_bus_num(void)
> +{
> +	if (i2c == (i2c_t *)I2C_BASE1)
> +		return 0;
> +	else
> +	if (i2c == (i2c_t *)I2C_BASE2)
> +		return 1;
> +#if defined(CONFIG_OMAP34XX)
> +	else
> +	if (i2c == (i2c_t *)I2C_BASE3)
> +		return 2;
> +#endif
> +	else
> +		return 0xFFFFFFFF;
> +}
> +
> +/*
> + * To be Implemented
> + */
> +int i2c_set_bus_speed(unsigned int speed)
> +{
> +	return 0;
> +}
> +
> +unsigned int i2c_get_bus_speed(void)
> +{
> +	return 0;
> +}
> +
> diff --git a/include/asm-arm/arch-omap24xx/i2c.h b/include/asm-arm/arch-omap24xx/i2c.h
> index 44db7a2..832a0e1 100644
> --- a/include/asm-arm/arch-omap24xx/i2c.h
> +++ b/include/asm-arm/arch-omap24xx/i2c.h

Ah, thanks for pointing to this. I missed this.

> @@ -23,24 +23,44 @@
>  #ifndef _OMAP24XX_I2C_H_
>  #define _OMAP24XX_I2C_H_
>  
> -#define I2C_BASE                0x48070000
> +#define I2C_BASE1               0x48070000
>  #define I2C_BASE2               0x48072000 /* nothing hooked up on h4 */

I wonder why arch-omap24xx/i2c.h has no I2C_DEFAULT_BASE (used above now).

> -#define I2C_REV                 (I2C_BASE + 0x00)
> -#define I2C_IE                  (I2C_BASE + 0x04)
> -#define I2C_STAT                (I2C_BASE + 0x08)
> -#define I2C_IV                  (I2C_BASE + 0x0c)
> -#define I2C_BUF                 (I2C_BASE + 0x14)
> -#define I2C_CNT                 (I2C_BASE + 0x18)
> -#define I2C_DATA                (I2C_BASE + 0x1c)
> -#define I2C_SYSC                (I2C_BASE + 0x20)
> -#define I2C_CON                 (I2C_BASE + 0x24)
> -#define I2C_OA                  (I2C_BASE + 0x28)
> -#define I2C_SA                  (I2C_BASE + 0x2c)
> -#define I2C_PSC                 (I2C_BASE + 0x30)
> -#define I2C_SCLL                (I2C_BASE + 0x34)
> -#define I2C_SCLH                (I2C_BASE + 0x38)
> -#define I2C_SYSTEST             (I2C_BASE + 0x3c)
> +#define I2C_DEFAULT_BASE	I2C_BASE1
> +
> +typedef struct i2c {

Seems the recent syntax is without typedef.

> +	unsigned short rev;		/* 0x00 */
> +	unsigned short pad_rev;		/* 0x02 */

Looking at

include/asm-arm/arch-omap3/cpu.h

the current style is to use resX for padding. Like done below with 
res1 already. We should stay consistent here.

> +	unsigned short ie;		/* 0x04 */
> +	unsigned short pad_ie;		/* 0x06 */
> +	unsigned short stat;		/* 0x08 */
> +	unsigned short pad_stat;	/* 0x0a */
> +	unsigned short iv;		/* 0x0c */
> +	unsigned short pad_iv;		/* 0x0e */
> +	unsigned int res1;		/* 0x10 */

unsigned short resX[3] instead?

> +	unsigned short buf;		/* 0x14 */
> +	unsigned short pad_buf;		/* 0x16 */
> +	unsigned short cnt;		/* 0x18 */
> +	unsigned short pad_cnt;		/* 0x1a */
> +	unsigned short data;		/* 0x1c */
> +	unsigned short pad_data;	/* 0x1e */
> +	unsigned short sysc;		/* 0x20 */
> +	unsigned short pad_sysc;	/* 0x22 */
> +	unsigned short con;		/* 0x24 */
> +	unsigned short pad_con;		/* 0x26 */
> +	unsigned short oa;		/* 0x28 */
> +	unsigned short pad_oa;		/* 0x2a */
> +	unsigned short sa;		/* 0x2c */
> +	unsigned short pad_sa;		/* 0x2e */
> +	unsigned short psc;		/* 0x30 */
> +	unsigned short pad_psc;		/* 0x32 */
> +	unsigned short scll;		/* 0x34 */
> +	unsigned short pad_scll;	/* 0x36 */
> +	unsigned short sclh;		/* 0x38 */
> +	unsigned short pad_sclh;	/* 0x3a */
> +	unsigned short systest;		/* 0x3c */
> +	unsigned short pad_systest;	/* 0x3e */
> +} i2c_t;
>  
>  /* I2C masks */
>  
> diff --git a/include/asm-arm/arch-omap3/i2c.h b/include/asm-arm/arch-omap3/i2c.h
> index 8b339cc..3da1fcf 100644
> --- a/include/asm-arm/arch-omap3/i2c.h
> +++ b/include/asm-arm/arch-omap3/i2c.h
> @@ -25,21 +25,39 @@
>  
>  #define I2C_DEFAULT_BASE	I2C_BASE1
>  
> -#define I2C_REV		(I2C_DEFAULT_BASE + 0x00)
> -#define I2C_IE 		(I2C_DEFAULT_BASE + 0x04)
> -#define I2C_STAT	(I2C_DEFAULT_BASE + 0x08)
> -#define I2C_IV 		(I2C_DEFAULT_BASE + 0x0c)
> -#define I2C_BUF		(I2C_DEFAULT_BASE + 0x14)
> -#define I2C_CNT		(I2C_DEFAULT_BASE + 0x18)
> -#define I2C_DATA	(I2C_DEFAULT_BASE + 0x1c)
> -#define I2C_SYSC	(I2C_DEFAULT_BASE + 0x20)
> -#define I2C_CON		(I2C_DEFAULT_BASE + 0x24)
> -#define I2C_OA 		(I2C_DEFAULT_BASE + 0x28)
> -#define I2C_SA 		(I2C_DEFAULT_BASE + 0x2c)
> -#define I2C_PSC		(I2C_DEFAULT_BASE + 0x30)
> -#define I2C_SCLL	(I2C_DEFAULT_BASE + 0x34)
> -#define I2C_SCLH	(I2C_DEFAULT_BASE + 0x38)
> -#define I2C_SYSTEST	(I2C_DEFAULT_BASE + 0x3c)
> +typedef struct i2c {

Comments from above apply here, too.

> +	unsigned short rev;		/* 0x00 */
> +	unsigned short pad_rev;		/* 0x02 */
> +	unsigned short ie;		/* 0x04 */
> +	unsigned short pad_ie;		/* 0x06 */
> +	unsigned short stat;		/* 0x08 */
> +	unsigned short pad_stat;	/* 0x0a */
> +	unsigned short iv;		/* 0x0c */
> +	unsigned short pad_iv;		/* 0x0e */
> +	unsigned int res1;		/* 0x10 */
> +	unsigned short buf;		/* 0x14 */
> +	unsigned short pad_buf;		/* 0x16 */
> +	unsigned short cnt;		/* 0x18 */
> +	unsigned short pad_cnt;		/* 0x1a */
> +	unsigned short data;		/* 0x1c */
> +	unsigned short pad_data;	/* 0x1e */
> +	unsigned short sysc;		/* 0x20 */
> +	unsigned short pad_sysc;	/* 0x22 */
> +	unsigned short con;		/* 0x24 */
> +	unsigned short pad_con;		/* 0x26 */
> +	unsigned short oa;		/* 0x28 */
> +	unsigned short pad_oa;		/* 0x2a */
> +	unsigned short sa;		/* 0x2c */
> +	unsigned short pad_sa;		/* 0x2e */
> +	unsigned short psc;		/* 0x30 */
> +	unsigned short pad_psc;		/* 0x32 */
> +	unsigned short scll;		/* 0x34 */
> +	unsigned short pad_scll;	/* 0x36 */
> +	unsigned short sclh;		/* 0x38 */
> +	unsigned short pad_sclh;	/* 0x3a */
> +	unsigned short systest;		/* 0x3c */
> +	unsigned short pad_systest;	/* 0x3e */
> +} i2c_t;
>  
>  /* I2C masks */
>  
> diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h
> index 19a5ec9..d5a0d49 100644
> --- a/include/configs/omap3_beagle.h
> +++ b/include/configs/omap3_beagle.h
> @@ -128,6 +128,7 @@
>  #define CONFIG_SYS_I2C_BUS		0
>  #define CONFIG_SYS_I2C_BUS_SELECT	1
>  #define CONFIG_DRIVER_OMAP34XX_I2C	1
> +#define CONFIG_I2C_MULTI_BUS		1

Not needed.


While all above are only style questions, what's about the main 
functionality topic:

Do we have to call i2c_init() for bus 1 and 2 if switching to it? If 
yes, who does it? For bus 0 it's already in the code. I'd like that 
the user doesn't have to care about it. Therefore, I added

static unsigned int bus_initialized[3] = {0, 0, 0};
static unsigned int current_bus = 0;

void i2c_init (int speed, int slaveadd) {
...
bus_initialized[current_bus] = 1;
}

int i2c_set_bus_num(unsigned int bus) {
...
current_bus = bus;

if(!bus_initialized[current_bus])
	i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
...
}


Do you like to test my patch in attachment?

It is an extension of

http://lists.denx.de/pipermail/u-boot/2009-October/063556.html

to cover arch-omap24xx/i2c.h, too.

Compile tested for omap3_beagle and omap2420h4.

Best regards

Dirk
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: omap3_i2c_multibus_patch.txt
Url: http://lists.denx.de/pipermail/u-boot/attachments/20091101/62cb74ab/attachment.txt 

[U-Boot] [PATCH] Add I2C multibus support for OMAP2/3 boards

[Tom]

Dirk Behme wrote:
> Tom Rix wrote:
>> From: Syed Mohammed Khasim <khasim@ti.com>
>>
>> This was cherry-picked from
>>
>> repo: http://www.beagleboard.org/u-boot-arm.git
>> commit: 52eddcd07c2e7ad61d15bab2cf2d0d21466eaca2
>>
>> In addition to adding multibus support, this patch
>> also cleans up the register access.  The register
>> access has been changed from #defines to a structure.
> 
> Have you looked at my proposal I sent some hours before your patch?

Sorry.  I did not.
Not surprisingly it looks similar.

The formatting changes you mentioned were part of the original
cherry-picked commit.  I tried to leave as much of the original
commit intact as possible.  I ran but ignored the output of
checkpatch.. The biggest complaint it has is with 80 char lines.

I looked over your patch.
It looks like an improvement to mine.

I'm all for dropping mine and using yours.
I will run test yours.

Do you have a zippy board that you can use to test the 2nd bus ?

Tom

[U-Boot] [PATCH] Add I2C multibus support for OMAP2/3 boards

[Tom]

Tom wrote:
> Dirk Behme wrote:
>> Tom Rix wrote:
>>> From: Syed Mohammed Khasim <khasim@ti.com>
>>>
>>> This was cherry-picked from
>>>
>>> repo: http://www.beagleboard.org/u-boot-arm.git
>>> commit: 52eddcd07c2e7ad61d15bab2cf2d0d21466eaca2
>>>
>>> In addition to adding multibus support, this patch
>>> also cleans up the register access.  The register
>>> access has been changed from #defines to a structure.
>> Have you looked at my proposal I sent some hours before your patch?
> 
> Sorry.  I did not.
> Not surprisingly it looks similar.
> 
> The formatting changes you mentioned were part of the original
> cherry-picked commit.  I tried to leave as much of the original
> commit intact as possible.  I ran but ignored the output of
> checkpatch.. The biggest complaint it has is with 80 char lines.
> 
> I looked over your patch.
> It looks like an improvement to mine.
> 
> I'm all for dropping mine and using yours.
> I will run test yours.
> 
> Do you have a zippy board that you can use to test the 2nd bus ?
> 
Dirk,

Some feedback.

No regressions on MAKEALL arm

I ran tested your patch Zoom1 and Beagle.
Default i2c looks good.
I did not test the multibus support.

Looks good, please officially submit your patch.

Tom

> Tom
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] Add I2C multibus support for OMAP2/3 boards

[Heiko Schocher]

Hello Dirk,

Dirk Behme wrote:
> Tom Rix wrote:
>> From: Syed Mohammed Khasim <khasim@ti.com>
>>
>> This was cherry-picked from
>>
>> repo: http://www.beagleboard.org/u-boot-arm.git
>> commit: 52eddcd07c2e7ad61d15bab2cf2d0d21466eaca2
>>
>> In addition to adding multibus support, this patch
>> also cleans up the register access.  The register
>> access has been changed from #defines to a structure.
> 
> Have you looked at my proposal I sent some hours before your patch?
> 
> http://lists.denx.de/pipermail/u-boot/2009-October/063556.html
> 
>> Signed-off-by: Syed Mohammed Khasim <khasim@ti.com>
>> Signed-off-by: Jason Kridner <jkridner@beagleboard.org>
>> Signed-off-by: Tom Rix <Tom.Rix@windriver.com>
>> ---
>>  drivers/i2c/omap24xx_i2c.c          |  220
>> ++++++++++++++++++++++-------------
>>  include/asm-arm/arch-omap24xx/i2c.h |   52 ++++++---
>>  include/asm-arm/arch-omap3/i2c.h    |   48 +++++---
>>  include/configs/omap3_beagle.h      |    1 +
>>  4 files changed, 209 insertions(+), 112 deletions(-)
>>
>> diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
>> index 1a4c8c9..763d2f8 100644
>> --- a/drivers/i2c/omap24xx_i2c.c
>> +++ b/drivers/i2c/omap24xx_i2c.c
>> @@ -1,7 +1,7 @@
>>  /*
>>   * Basic I2C functions
>>   *
>> - * Copyright (c) 2004 Texas Instruments
>> + * Copyright (c) 2004, 2009 Texas Instruments
>>   *
>>   * This package is free software;  you can redistribute it and/or
>>   * modify it under the terms of the license found in the file
>> @@ -25,10 +25,18 @@
>>  #include <asm/arch/i2c.h>
>>  #include <asm/io.h>
>>  
>> +#ifdef CONFIG_OMAP34XX
>> +#define I2C_NUM_IF 3
>> +#else
>> +#define I2C_NUM_IF 2
>> +#endif
> 
> I prefer something like I2C_BUS_MAX for this. And move it to header
> file. Moving it OMAP2 and OMAP3 i2c.h headers will remove the #ifdef, too.

Yep, I2C_BUS_MAX would be better.


[...]
>> @@ -398,8 +406,58 @@ static u16 wait_for_pin (void)
>>  
>>      if (timeout <= 0) {
>>          printf ("timed out in wait_for_pin: I2C_STAT=%x\n",
>> -            readw (I2C_STAT));
>> -            writew(0xFFFF, I2C_STAT);
>> +            readw(&i2c->stat));
>> +            writew(0xFFFF, &i2c->stat);
>>  }
>>      return status;
>>  }
>> +
>> +int i2c_set_bus_num(unsigned int bus)
> 
> Do we need an extern declaration in i2c.h for this? To be able to call
> it from somewhere else without warning?

Hmm.. isn;t it declared in i2c.h?

>> +{
>> +    if ((bus < 0) || (bus >= I2C_NUM_IF)) {
> 
> As mentioned above, I'd like something like bus max here.
> 
>> +        printf("Bad bus ID-%d\n", bus);
>> +        return -1;
>> +    }
>> +
>> +#if defined(CONFIG_OMAP34XX)
>> +    if (bus == 2)
>> +        i2c = (i2c_t *)I2C_BASE3;
>> +    else
>> +#endif
>> +    if (bus == 1)
>> +        i2c = (i2c_t *)I2C_BASE2;
>> +    else
>> +        i2c = (i2c_t *)I2C_BASE1;
>> +
>> +    return 0;
>> +}
> 
> I would remove the following three functions as they are not needed at
> the moment (i.e. without command line interface).

Hmm... really? If someone uses multibus support,
this functions are needed, or?

>> +unsigned int i2c_get_bus_num(void)
>> +{
>> +    if (i2c == (i2c_t *)I2C_BASE1)
>> +        return 0;
>> +    else
>> +    if (i2c == (i2c_t *)I2C_BASE2)
>> +        return 1;
>> +#if defined(CONFIG_OMAP34XX)
>> +    else
>> +    if (i2c == (i2c_t *)I2C_BASE3)
>> +        return 2;
>> +#endif
>> +    else
>> +        return 0xFFFFFFFF;
>> +}
>> +
>> +/*
>> + * To be Implemented
>> + */
>> +int i2c_set_bus_speed(unsigned int speed)
>> +{
>> +    return 0;
>> +}
>> +
>> +unsigned int i2c_get_bus_speed(void)
>> +{
>> +    return 0;
>> +}
>> +
[...]
>> diff --git a/include/configs/omap3_beagle.h
>> b/include/configs/omap3_beagle.h
>> index 19a5ec9..d5a0d49 100644
>> --- a/include/configs/omap3_beagle.h
>> +++ b/include/configs/omap3_beagle.h
>> @@ -128,6 +128,7 @@
>>  #define CONFIG_SYS_I2C_BUS        0
>>  #define CONFIG_SYS_I2C_BUS_SELECT    1
>>  #define CONFIG_DRIVER_OMAP34XX_I2C    1
>> +#define CONFIG_I2C_MULTI_BUS        1
> 
> Not needed.
> 
> 
> While all above are only style questions, what's about the main
> functionality topic:
> 
> Do we have to call i2c_init() for bus 1 and 2 if switching to it? If
> yes, who does it? For bus 0 it's already in the code. I'd like that the
> user doesn't have to care about it. Therefore, I added

I solved this in the multibus_v2 branch, see:

http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2

i2c_init() gets just called if switching to an I2C bus.
This is done in i2c_set_bus_num()

http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=blob;f=drivers/i2c/i2c_core.c;h=e1bb67f1c136f5904c195d31d2916b17a30c9632;hb=6ef1bb6e9a0c03d5cbc8ef296b5022b5698207a6

pro:

a I2C adapter gets only initialized, if used, and there
is the option to extend this functionality to:
- when switching to another i2c adapter call (a not yet existing)
  i2c adapter deinit function ...

> static unsigned int bus_initialized[3] = {0, 0, 0};
> static unsigned int current_bus = 0;
> 
> void i2c_init (int speed, int slaveadd) {
> ...
> bus_initialized[current_bus] = 1;
> }
> 
> int i2c_set_bus_num(unsigned int bus) {
> ...
> current_bus = bus;
> 
> if(!bus_initialized[current_bus])
>     i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
> ...
> }

Ah, you also tend to this direction ;-) Great!

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] Add I2C multibus support for OMAP2/3 boards

[Dirk Behme]

Hi Heiko,

Heiko Schocher wrote:
> Dirk Behme wrote:
>> I would remove the following three functions as they are not needed at
>> the moment (i.e. without command line interface).
> 
> Hmm... really? If someone uses multibus support,
> this functions are needed, or?

Well, most probably yes. But I was not really looking for multibus 
support ;)

The functionality needed for Zippy I'm currently working on is just to 
be able to do

i2c_get_bus_num(1); /* Switch to bus 1 */
... do some stuff on bus 1 ...
i2c_get_bus_num(0); /* Switch back to default bus 0 */

And for this having only i2c_get_bus_num() is sufficient. And the only 
tested ;)

>>> +unsigned int i2c_get_bus_num(void)
>>> +{
>>> +    if (i2c == (i2c_t *)I2C_BASE1)
>>> +        return 0;
>>> +    else
>>> +    if (i2c == (i2c_t *)I2C_BASE2)
>>> +        return 1;
>>> +#if defined(CONFIG_OMAP34XX)
>>> +    else
>>> +    if (i2c == (i2c_t *)I2C_BASE3)
>>> +        return 2;
>>> +#endif
>>> +    else
>>> +        return 0xFFFFFFFF;
>>> +}
>>> +
>>> +/*
>>> + * To be Implemented
>>> + */
>>> +int i2c_set_bus_speed(unsigned int speed)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>> +unsigned int i2c_get_bus_speed(void)
>>> +{
>>> +    return 0;
>>> +}
>>> +
...
>> While all above are only style questions, what's about the main
>> functionality topic:
>>
>> Do we have to call i2c_init() for bus 1 and 2 if switching to it? If
>> yes, who does it? For bus 0 it's already in the code. I'd like that the
>> user doesn't have to care about it. Therefore, I added
> 
> I solved this in the multibus_v2 branch, see:
> 
> http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2

Looks like we have to merge that with what is in this patch somehow. 
What are your plans with that branch? Is it intended to go into 
mainline, soon?

It seems to me that multibus_v2 and this patch are somehow orthogonal 
(but yes, they will need some merging).
Regarding omap24xx_i2c.c, multibus_v2 seems to deal mainly with the 
API stuff

http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=blobdiff;f=drivers/i2c/omap24xx_i2c.c;h=993b995d34a298d07fbd11bce23f2d0cfdebb40f;hp=1a4c8c9ad2701b550fc1ad0ab2a9fde741902080;hb=899dbf9fbd089a9e8133026be3817e0116f44390;hpb=24d3d6307e1e5d5cddddee399a15e70ef9da8818

while the main part of this patch is to replace all hard coded 
registers by the ability to switch to an other bus.

I will send an updated patch with fixing at least the review comments, 
soon.

Best regards

Dirk

[U-Boot] [PATCH] Add I2C multibus support for OMAP2/3 boards

[Heiko Schocher]

Hello Dirk,

Dirk Behme wrote:
> Heiko Schocher wrote:
>> Dirk Behme wrote:
[...]
>>> While all above are only style questions, what's about the main
>>> functionality topic:
>>>
>>> Do we have to call i2c_init() for bus 1 and 2 if switching to it? If
>>> yes, who does it? For bus 0 it's already in the code. I'd like that the
>>> user doesn't have to care about it. Therefore, I added
>>
>> I solved this in the multibus_v2 branch, see:
>>
>> http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2
>>
> 
> Looks like we have to merge that with what is in this patch somehow.
> What are your plans with that branch? Is it intended to go into
> mainline, soon?

I have to do some more work, before this is ready to go in mainline...

> It seems to me that multibus_v2 and this patch are somehow orthogonal
> (but yes, they will need some merging).

Yep, I hold the multibus_v2 branch in sync with mainline.

> Regarding omap24xx_i2c.c, multibus_v2 seems to deal mainly with the API
> stuff
> 
> http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=blobdiff;f=drivers/i2c/omap24xx_i2c.c;h=993b995d34a298d07fbd11bce23f2d0cfdebb40f;hp=1a4c8c9ad2701b550fc1ad0ab2a9fde741902080;hb=899dbf9fbd089a9e8133026be3817e0116f44390;hpb=24d3d6307e1e5d5cddddee399a15e70ef9da8818

Yep, I adapt this driver only for working with this new API.

> while the main part of this patch is to replace all hard coded registers
> by the ability to switch to an other bus.
> 
> I will send an updated patch with fixing at least the review comments,
> soon.

OK, thanks.

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] Add I2C multibus support for OMAP2/3 boards

[Dirk Behme]

Hi Heiko,

Heiko Schocher wrote:
> Hello Dirk,
> 
> Dirk Behme wrote:
>> Heiko Schocher wrote:
>>> Dirk Behme wrote:
> [...]
>>>> While all above are only style questions, what's about the main
>>>> functionality topic:
>>>>
>>>> Do we have to call i2c_init() for bus 1 and 2 if switching to it? If
>>>> yes, who does it? For bus 0 it's already in the code. I'd like that the
>>>> user doesn't have to care about it. Therefore, I added
>>> I solved this in the multibus_v2 branch, see:
>>>
>>> http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2
>>>
>> Looks like we have to merge that with what is in this patch somehow.
>> What are your plans with that branch? Is it intended to go into
>> mainline, soon?
> 
> I have to do some more work, before this is ready to go in mainline...
> 
>> It seems to me that multibus_v2 and this patch are somehow orthogonal
>> (but yes, they will need some merging).
> 
> Yep, I hold the multibus_v2 branch in sync with mainline.
> 
>> Regarding omap24xx_i2c.c, multibus_v2 seems to deal mainly with the API
>> stuff
>>
>> http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=blobdiff;f=drivers/i2c/omap24xx_i2c.c;h=993b995d34a298d07fbd11bce23f2d0cfdebb40f;hp=1a4c8c9ad2701b550fc1ad0ab2a9fde741902080;hb=899dbf9fbd089a9e8133026be3817e0116f44390;hpb=24d3d6307e1e5d5cddddee399a15e70ef9da8818
> 
> Yep, I adapt this driver only for working with this new API.
> 
>> while the main part of this patch is to replace all hard coded registers
>> by the ability to switch to an other bus.
>>
>> I will send an updated patch with fixing at least the review comments,
>> soon.

Hmm, do you like to push

http://lists.denx.de/pipermail/u-boot/2009-November/063658.html

to I2C git main branch then?

Or are you fine that we push it to u-boot-ti?

Or?

Thanks

Dirk

[U-Boot] [PATCH] Add I2C multibus support for OMAP2/3 boards

[Heiko Schocher]

Hello Dirk,

Dirk Behme wrote:
> Hmm, do you like to push
> 
> http://lists.denx.de/pipermail/u-boot/2009-November/063658.html
> 
> to I2C git main branch then?
> 
> Or are you fine that we push it to u-boot-ti?
> 
> Or?

It;s in u-boot-i2c next, see

http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/next

because I think it is for the next merge window ...

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany