[U-Boot-Users] [PATCH 2/2] Multi-bus I2C implementation of MPC834x

[Ben Warren <bwarren@qstreams.com>]

Comments on delta between our patches.  I'll create another supplemental
patch to merge:

On Wed, 2006-08-30 at 16:39 -0500, Timur Tabi wrote:

> --- a/board/mpc8349emds/pci.c
> +++ b/board/mpc8349emds/pci.c
> @@ -72,7 +72,7 @@ pib_init(void)
>       /*
>        * Assign PIB PMC slot to desired PCI bus
>        */
> -    mpc8349_i2c = (i2c_t*)(CFG_IMMRBAR + CFG_I2C2_OFFSET);
> +    I2C = (i2c_t*) (CFG_IMMRBAR + CFG_I2C2_OFFSET);
>       i2c_init(CFG_I2C_SPEED, CFG_I2C_SLAVE);
> 
This part is covered in part 2 of my patch, but I instead use the new
API call i2c_set_bus(1) and then set it back to 0 at the end of the
function.  Net effect - identical.  Note that the i2c_init call is
redundant with what you've done below...  More on that later

>       val8 = 0;
> diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
> index c543bb5..9f0980b 100644
> --- a/common/cmd_i2c.c
> +++ b/common/cmd_i2c.c
> @@ -529,11 +529,7 @@ mod_i2c_mem(cmd_tbl_t *cmdtp, int incrfl
>       return 0;
>   }
> 
> -/*
> - * Syntax:
> - *    iprobe {addr}{.0, .1, .2}
> - */
> -int do_i2c_probe (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +static void _do_i2c_probe(void)
>   {
>       int j;
>   #if defined(CFG_I2C_NOPROBES)
> @@ -565,6 +561,29 @@ int do_i2c_probe (cmd_tbl_t *cmdtp, int
>           printf(" %02X", i2c_no_probes[k] );
>       putc ('\n');
>   #endif
> +}
> +
> +/*
> + * Syntax:
> + *    iprobe {addr}{.0, .1, .2}
> + */
> +int do_i2c_probe(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +#ifdef CFG_I2C2_OFFSET
> +
> +/* If we have two I2C busses, then we need to probe each one separately.
> +   Note that if an I2C address is defined in i2c_no_probes[], we skip it
> +   on both busses.
> +*/
> +    printf("I2C1 ");
> +    I2C = (i2c_t *) (CFG_IMMRBAR + CFG_I2C_OFFSET);
> +    _do_i2c_probe();
> +
> +    printf("I2C2 ");
> +    I2C = (i2c_t *) (CFG_IMMRBAR + CFG_I2C2_OFFSET);
> +#endif
> +
> +    _do_i2c_probe();
> 
>       return 0;
>   }
My reworking of the I2C commands is much more comprehensive and has been
discussed and (generally) approved on this list already.  Your code is
not portable, and will not compile with other CPUs because IMMRBAR is
only defined with the MPC834x (the equivalent register is called IMMR on
other PowerQUICC CPUs).  Probably not ideal, but it is what it is...
> diff --git a/cpu/mpc83xx/i2c.c b/cpu/mpc83xx/i2c.c
> index 70450f9..308a65d 100644
> --- a/cpu/mpc83xx/i2c.c
> +++ b/cpu/mpc83xx/i2c.c
> @@ -41,21 +41,30 @@
>   #include <i2c.h>
>   #include <asm/i2c.h>
> 
> -#if defined(CONFIG_MPC8349EMDS) || defined(CONFIG_TQM834X)
> -i2c_t * mpc8349_i2c = (i2c_t*)(CFG_IMMRBAR + CFG_I2C_OFFSET);
> +#ifdef CFG_I2C2_OFFSET
> +/*
> +To configure DDR RAM, we need to query the I2C bus.  Since RAM hasn't
> +been initialized, U-Boot has not been copied yet to RAM.  That means that
> +the global variable 'I2C' is located in flash, which means it can't be
> +modified.  Therefore, 'I2C' needs to be initialized to the I2C bus that
> +DDR is on.
> +
> +CFG_I2C_DDR_OFFSET is the offset of the I2C bus that DDR is using.  It
> +should be set to either CFG_I2C_OFFSET or CFG_I2C2_OFFSET.
> +*/
> +volatile i2c_t *I2C = (i2c_t *) (CFG_IMMRBAR + CFG_I2C_DDR_OFFSET);
>   #endif
> 
This is important.  I think it's fair to say that most people would hang
the SPD_EEPROM on I2C bus 0, but we should probably allow an option to
override that, defaulting to bus 0 if not set.  I'll make an adjustment
to incorporate. 
> -void
> -i2c_init(int speed, int slaveadd)
> +static void _i2c_init(int slaveadd)
>   {
>       /* stop I2C controller */
>       writeb(0x00 , &I2C->cr);
> 
>       /* set clock */
> -    writeb(0x3f, &I2C->fdr);
> +    writeb(IC2_FDR, &I2C->fdr);
> 
>       /* set default filter */
> -    writeb(0x10,&I2C->dfsrr);
> +    writeb(I2C_CR_MTX, &I2C->dfsrr);
> 
>       /* write slave address */
>       writeb(slaveadd, &I2C->adr);
> @@ -67,6 +76,20 @@ i2c_init(int speed, int slaveadd)
>       writeb(I2C_CR_MEN, &I2C->cr);
>   }
> +void
> +i2c_init(int speed, int slaveadd)
> +{
> +#ifdef CFG_I2C2_OFFSET
> +    /* If it exists, initialize the 2nd I2C bus */
> +    I2C = (i2c_t *) (CFG_IMMRBAR + CFG_I2C_OFFSET);
> +    _i2c_init(slaveadd);
> +
> +    I2C = (i2c_t *) (CFG_IMMRBAR + CFG_I2C2_OFFSET);
> +#endif
> +    _i2c_init(slaveadd);
> +
> +}
> +
I like the replacement of magic numbers, and will rework i2c_init to do
something equivalent.  Initializing both controllers at once is a good
idea.
>   static __inline__ int
>   i2c_wait4bus (void)
>   {
> diff --git a/include/asm-ppc/i2c.h b/include/asm-ppc/i2c.h
> index 1680d3a..bd6a51d 100644
> --- a/include/asm-ppc/i2c.h
> +++ b/include/asm-ppc/i2c.h
> @@ -87,14 +87,13 @@ typedef struct i2c
>   #error CFG_I2C_OFFSET is not defined in /include/configs/${BOARD}.h
>   #endif
> 
> -#if defined(CONFIG_MPC8349EMDS) || defined(CONFIG_TQM834X)
> +#ifdef CFG_I2C2_OFFSET
>   /*
> - * MPC8349 have two i2c bus
> + * If we have two I2C busses, then 'I2C' should be a variable, not a constant.
>    */
> -extern i2c_t * mpc8349_i2c;
> -#define I2C mpc8349_i2c
> +extern volatile i2c_t *I2C;
>   #else
> -#define I2C ((i2c_t*)(CFG_IMMRBAR + CFG_I2C_OFFSET))
> +#define I2C ((i2c_t*) (CFG_IMMRBAR + CFG_I2C_OFFSET))
>   #endif
This stuff is covered in my patch