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

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

[Ben Warren]

Hello,

Attached is a patch implementing multiple I2C buses on the MPC834x CPU
family and the MPC8349EMDS board in particular.
This patch requires Patch 1 (Add support for multiple I2C buses).
Testing was performed on a 533MHz board.

CHANGELOG:
        Implemented driver-level code to support two I2C buses on the
MPC834x CPU family and the MPC8349EMDS board.  Available I2C bus speeds
are 50kHz, 100kHz and 400kHz on each bus.

regards,
Ben
-------------- next part --------------
A non-text attachment was scrubbed...
Name: i2c_8349.PATCH
Type: text/x-patch
Size: 7941 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20060830/73a17475/attachment.bin 

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

[Timur Tabi]

Ben Warren wrote:
> Hello,
> 
> Attached is a patch implementing multiple I2C buses on the MPC834x CPU
> family and the MPC8349EMDS board in particular.

Here's my patch which does the same thing.

CHANGELOG:

* The current support for dual I2C busses is specific to the MPC 8349.  This patch
   makes it more generic.  If two I2C busses are present (i.e. CFG_I2C2_OFFSET is
   defined), then the macro 'I2C' becomes a variable.  Any code which needs to
   reference the I2C bus should make sure that 'I2C' points to the right bus before
   calling an i2c_* function.  i2c_init() and do_i2c_probe() have been updated to
   automatically process both busses.  And lastly, some hard-code constants have
   been replaced by macros.

Signed-off-by: Timur Tabi <timur@freescale.com>

---

  board/mpc8349emds/pci.c |    2 +-
  common/cmd_i2c.c        |   29 ++++++++++++++++++++++++-----
  cpu/mpc83xx/i2c.c       |   35 +++++++++++++++++++++++++++++------
  include/asm-ppc/i2c.h   |    9 ++++-----
  4 files changed, 58 insertions(+), 17 deletions(-)

4fa4904374e01e31ea155dbe340d72abd944617a
diff --git a/board/mpc8349emds/pci.c b/board/mpc8349emds/pci.c
index 63e4405..e91bfef 100644
--- 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);

      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;
  }
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

-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);
+
+}
+
  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

  #define I2C_READ  1
-- 
1.2.4

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

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

[Wolfgang Denk]

In message <44F605AE.3010405@freescale.com> you wrote:
>
> > Attached is a patch implementing multiple I2C buses on the MPC834x CPU
> > family and the MPC8349EMDS board in particular.
> 
> Here's my patch which does the same thing.

What do you mean by "does the same thing"?


For example, in your patch, I don't see any code to extend  the  user
interface?

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I must follow the people.  Am I not their leader? - Benjamin Disraeli

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

[Timur Tabi]

Wolfgang Denk wrote:

> What do you mean by "does the same thing"?
> 
> For example, in your patch, I don't see any code to extend  the  user
> interface?

Well, okay, you have a point.  Ben's patch does more.  My changes just make the dual-I2C bus support that's currently in U-Boot more generic (i.e. not specific to the 8349).

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

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

[Wolfgang Denk]

Dear Timur,

in message <44F61044.2030907@freescale.com> you wrote:
> 
> Well, okay, you have a point.  Ben's patch does more.  My changes just make the dual-I2C bus support that's currently in U-Boot more generic (i.e. not specific to the 8349).

I don't have time right now for a thorough review, so can you  please
do  me  a  favour  and  check  Ben's  code  and  eventually provide a
incremental patch to fix or extend his patch with  things  you  might
have in your patch and he doesn't? Thanks.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It is necessary to have purpose.
	-- Alice #1, "I, Mudd", stardate 4513.3

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

[Timur Tabi]

Wolfgang Denk wrote:

> I don't have time right now for a thorough review, so can you  please
> do  me  a  favour  and  check  Ben's  code  and  eventually provide a
> incremental patch to fix or extend his patch with  things  you  might
> have in your patch and he doesn't? Thanks.

Sure, I'll work with Ben (if he's willing) on it this week.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

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

[Ben Warren]

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

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

[Ben Warren]

Hello,

Attached is a patch implementing multiple I2C buses on the MPC834x CPU
family and the MPC8349EMDS board in particular.
This patch requires Patch 1 (Add support for multiple I2C buses).
Testing was performed on a 533MHz board.

/*** Note: This patch replaces ticket DNX#2006083042000027 ***/

Signed-off-by: Ben Warren <bwarren@qstreams.com>

CHANGELOG:
        Implemented driver-level code to support two I2C buses on the
MPC834x CPU family and the MPC8349EMDS board.  Available I2C bus speeds
are 50kHz, 100kHz and 400kHz on each bus.

regards,
Ben
-------------- next part --------------
A non-text attachment was scrubbed...
Name: i2c_8349.PATCH
Type: text/x-patch
Size: 8277 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20060907/9c76b80c/attachment.bin 

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

[Timur Tabi]

Ben,

Did you forget to include the patch for board/mpc8349emds/pci.c?

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

[Ben Warren]

On Mon, 2006-09-11 at 17:06 -0500, Timur Tabi wrote:
> Ben,
> 
> Did you forget to include the patch for board/mpc8349emds/pci.c?

Ugh.  I guess double-checking wasn't enough.  I'll submit another patch
in a few minutes.

BTW - please forgive my ignorance, but how do you get a changed-file
summary at the top of a patch?

regards,
Ben