From mboxrd@z Thu Jan  1 00:00:00 1970
From: Dirk Behme <dirk.behme@googlemail.com>
Date: Sun, 01 Nov 2009 09:24:01 +0100
Subject: [U-Boot] [PATCH] Add I2C multibus support for OMAP2/3 boards
In-Reply-To: <1257040373-1569-2-git-send-email-Tom.Rix@windriver.com>
References: <1257040373-1569-1-git-send-email-Tom.Rix@windriver.com>
	<1257040373-1569-2-git-send-email-Tom.Rix@windriver.com>
Message-ID: <4AED45A1.1090703@googlemail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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