Hi Kim,

On Saturday 04 November 2006 03:11, Kim Phillips wrote:

Please pull from 'master' branch of:

http://opensource.freescale.com/pub/scm/u-boot-83xx.git

to receive the following updates (essentially MPC8349mITX and MPC8360EMDS support):

I did a quick check. The overall impression is quite good. Thanks a lot for your contribution (and you colleagues too of course).

Here a few notes:

- Please don't add changelog comments in the files (like in cpu/mpc83xx/i2c.c or spd_sdram.c). Instead please remove all the file internal changelog comments, since we decided to only use git for this history.

- I won't comment on the "support for multiple I2C buses" in this mail, since it's done by Ben Warren. I'll will send a separate mail for this.

- Please add the missing entries to the MAINTAINERS files (MPC8349EMDS & MPC8360EMDS).

Another idea (this would also affect the other Freescale board ports): What do you think of moving all your Freescale board ports into a Freescale board directory (as done for AMCC or esd for example). So we would get something like:

board/freescale/mpc8349emds board/freescale/mpc8349itx board/freescale/mpc8360emds ... board/freescale/common (if needed)

This way we would not "pollute" the main board directory too much. Especially when thinking of additional Freescale board, which hopefully will come...

We could start with the mpc83xx boards and contine soon with the 85xx and 86xx eval boards (and so on...).

Here some further remarks to your patches:

--------------------------- cpu/mpc83xx/spd_sdram.c --------------------------- index 48624fe..153848d 100644 @@ -1,4 +1,6 @@ /*

• • (C) Copyright 2006 Freescale Semiconductor, Inc.
• • (C) Copyright 2006
  • Wolfgang Denk, DENX Software Engineering, wd@denx.de.

@@ -28,6 +30,10 @@

• 20050101: Eran Liberty (liberty@freescale.com)

•       Initial file creating (porting from 85XX & 8260)
  

• • 20060601: Dave Liu (daveliu@freescale.com)

• •       DDR ECC support
    

• •       unify variable names for 83xx
    

• •       code cleanup
    

Please remove the change history.

*/

#include <common.h> @@ -39,7 +45,7 @@

#ifdef CONFIG_SPD_EEPROM

-#if defined(CONFIG_DDR_ECC) +#if defined(CONFIG_DDR_ECC) && !defined(CONFIG_ECC_INIT_VIA_DDRC) extern void dma_init(void); extern uint dma_check(void); extern int dma_xfer(void *dest, uint count, void *src); @@ -52,15 +58,18 @@ extern int dma_xfer(void *dest, uint cou /*

• Convert picoseconds into clock cycles (rounding up if needed).

*/ +extern ulong get_ddr_clk(ulong dummy);

int picos_to_clk(int picos) {

• unsigned int ddr_bus_clk; int clks;

• clks = picos / (2000000000 / (get_bus_freq(0) / 1000));
• if (picos % (2000000000 / (get_bus_freq(0) / 1000)) != 0) {
• clks++;

• ddr_bus_clk = get_ddr_clk(0) >> 1;
• clks = picos / ((1000000000 / ddr_bus_clk) * 1000);
• if (picos % ((1000000000 / ddr_bus_clk) * 1000) !=0) {

Add a space after the !=.

• clks++;
  

  }

Remove the { } here (1 line statements shouldn't have braces).

<snip>

@@ -246,36 +304,49 @@ long int spd_sdram()

debug("DDR:timing_cfg_1=0x%08x\n", ddr->timing_cfg_1); debug("DDR:timing_cfg_2=0x%08x\n", ddr->timing_cfg_2);

• /* Setup init value, but not enable */
• ddr->sdram_cfg = 0x42000000;
• /* Check DIMM data bus width */
• if (spd.dataw_lsb == 0x20)
• {

Please move the brace in the line above.

• burstlen = 0x03; /* 32 bit data bus, burst len is 8 */
  

• printf("\n   DDR DIMM: data bus width is 32 bit");
  

• }
• else
• {

This should be "} else {".

• burstlen = 0x02; /* Others act as 64 bit bus, burst len is 4 */
  

• printf("\n   DDR DIMM: data bus width is 64 bit");
  

• }

• /*

• * Only DDR I is supported
  

• * DDR I and II have different mode-register-set definition
  

• /* Is this an ECC DDR chip? */
• if (spd.config == 0x02) {

• printf(" with ECC\n");
  

• }

No brace here.

<snip>

@@ -352,38 +405,48 @@ long int spd_sdram() * sdram_cfg[0] = 1 (ddr sdram logic enable) * sdram_cfg[1] = 1 (self-refresh-enable) * sdram_cfg[6:7] = 2 (SDRAM type = DDR SDRAM)

• * sdram_cfg[12] = 0 (32_BE =0 , 64 bit bus mode)
  

• * sdram_cfg[13] = 0 (8_BE =0, 4-beat bursts)
  

  */

• tmp = 0xc2000000;

• sdram_cfg = 0xC2000000;

-#if defined (CONFIG_DDR_32BIT)

• /* in 32-Bit mode burst len is 8 beats */
• tmp |= (SDRAM_CFG_32_BE | SDRAM_CFG_8_BE);

-#endif

• /*

• * sdram_cfg[3] = RD_EN - registered DIMM enable
  

• *   A value of 0x26 indicates micron registered DIMMS (micron.com)
  

• */
  

• if (spd.mod_attr == 0x26) {

• tmp |= 0x10000000;
  

• /* sdram_cfg[3] = RD_EN - registered DIMM enable */
• if (spd.mod_attr & 0x02) {

• sdram_cfg |= 0x10000000;
  

  }

Again, no brace.

• /* The DIMM is 32bit width */
• if (spd.dataw_lsb == 0x20) {

• sdram_cfg |= 0x000C0000;
  

• }

Again.

----------------------------- cpu/mpc83xx/speed.c ----------------------------- index ad6b3f6..412713f 100644 @@ -337,6 +337,12 @@ int get_clocks (void) return 0; }

+ulong get_ddr_clk(ulong dummy) +{

• return gd->ddr_clk;

+}

Hmmm. Is this function really needed?

Author: Timur Tabi timur@freescale.com 2006-10-26 01:45:23 Committer: Kim Phillips kim.phillips@freescale.com 2006-11-04 02:42:19 Parent: 31068b7c4abeefcb2c8fd4fbeccc8ec6c6d0475a (mpc83xx: Add support for variable flash memory sizes on 83xx systems) Child: bed85caf872714ebf53013967a695c9d63acfc68 (mpc83xx: Add support for Errata DDR6 on MPC 834x systems) Branches: master, 83xx Follows: U-Boot-1_1_6 Precedes:

mpc83xx: fix TQM build by defining a CFG_FLASH_SIZE for it

-------------------------- include/configs/TQM834x.h -------------------------- index b1f033d..f0e4900 100644 @@ -95,6 +95,7 @@ #define CFG_FLASH_CFI_DRIVER /* use the CFI driver */ #undef CFG_FLASH_CHECKSUM #define CFG_FLASH_BASE 0x80000000 /* start of FLASH */ +#define CFG_FLASH_SIZE 8 /* FLASH size in MB */

Do you only support sizes bigger or equal to 1 MByte? What if a board only has 512kBytes?

I also do get some warning upon compiling for MPC8349ITX: mpc8349itx.c: In function 'misc_init_r': mpc8349itx.c:398: warning: pointer targets in passing argument 4 of 'i2c_read' differ in signedness mpc8349itx.c:443: warning: pointer targets in passing argument 4 of 'i2c_write' differ in signedness

And for MPC8360EMDS: uccf.c: In function 'ucc_set_clk_src': uccf.c:121: warning: 'reg_num' is used uninitialized in this function uccf.c:105: warning: 'shift' may be used uninitialized in this function uccf.c:103: warning: 'p_cmxucr' may be used uninitialized in this function

Could you please clean this up too?

Thanks a lot for your patience. ;-)

Best regards, Stefan

Hi Ben,

On Saturday 04 November 2006 03:11, Kim Phillips wrote:

Please pull from 'master' branch of:

http://opensource.freescale.com/pub/scm/u-boot-83xx.git

to receive the following updates (essentially MPC8349mITX and MPC8360EMDS support):

Ben Warren: Add support for multiple I2C buses Multi-bus I2C implementation of MPC834x Additional MPC8349 support for multibus i2c

I'm commenting on the I2C code you submitted, which is now included in the git tree of Kim Phillips. Sorry for the late review, but I have some more or less cosmetic comments:

------------------------------- common/cmd_i2c.c ------------------------------- index c543bb5..62378af 100644 @@ -101,8 +101,31 @@ static uchar i2c_mm_last_chip; static uint i2c_mm_last_addr; static uint i2c_mm_last_alen;

+/* If only one I2C bus is present, the list of devices to ignore when

• • the probe command is issued is represented by a 1D array of addresses.
• • When multiple buses are present, the list is an array of bus-address
• • pairs. The following macros take care of this */

#if defined(CFG_I2C_NOPROBES) +#if defined(CONFIG_I2C_MULTI_BUS) +static struct +{

• uchar bus;
• uchar addr;

+} i2c_no_probes[] = CFG_I2C_NOPROBES; +#define GET_BUS_NUM i2c_get_bus_num() +#define COMPARE_BUS(b,i) (i2c_no_probes[(i)].bus == (b)) +#define COMPARE_ADDR(a,i) (i2c_no_probes[(i)].addr == (a)) +#define NO_PROBE_ADDR(i) i2c_no_probes[(i)].addr +#else /* single bus */ static uchar i2c_no_probes[] = CFG_I2C_NOPROBES; +#define GET_BUS_NUM 0 +#define COMPARE_BUS(b,i) ((b) == 0) /* Make compiler happy */ +#define COMPARE_ADDR(a,i) (i2c_no_probes[(i)] == (a)) +#define NO_PROBE_ADDR(i) i2c_no_probes[(i)] +#endif /* CONFIG_MULTI_BUS */

+#define NUM_ELEMENTS_NOPROBE (sizeof(i2c_no_probes)/sizeof(i2c_no_probes[0])) #endif

static int @@ -538,14 +561,17 @@ int do_i2c_probe (cmd_tbl_t *cmdtp, int int j; #if defined(CFG_I2C_NOPROBES) int k, skip; -#endif

• uchar bus = GET_BUS_NUM;

+#endif /* NOPROBES */

puts ("Valid chip addresses:"); for(j = 0; j < 128; j++) { #if defined(CFG_I2C_NOPROBES) skip = 0;

• for (k = 0; k < sizeof(i2c_no_probes); k++){
  

• 	if (j == i2c_no_probes[k]){
  

• for(k=0; k < NUM_ELEMENTS_NOPROBE; k++)
  

• {
  

Please move the "{" brace into the "for" loop line. And please also insert a space between "for" and the "(" parenthesis. So the line above should look like this:

for (k=0; k < NUM_ELEMENTS_NOPROBE; k++) {

It seems that you use this "brace style" throughout the complete patch. This is not according to the U-Boot coding styles (and Linux by the way). So could you please rework your patch and merge it with Kim again? Or perhaps Kim can rework the patch according to the U-Boot/Linux coding style (Lindent?).

Thanks.

• 	if(COMPARE_BUS(bus, k) && COMPARE_ADDR(j, k))
  

• 	{
  	skip = 1;
  	break;
  }
  

@@ -561,8 +587,11 @@ int do_i2c_probe (cmd_tbl_t *cmdtp, int

#if defined(CFG_I2C_NOPROBES) puts ("Excluded chip addresses:");

• for( k = 0; k < sizeof(i2c_no_probes); k++ )

• printf(" %02X", i2c_no_probes[k] );
  

• for(k=0; k < NUM_ELEMENTS_NOPROBE; k++)
• {

• if(COMPARE_BUS(bus,k))
  

Again, please insert a space after the "if".

• 	printf(" %02X", NO_PROBE_ADDR(k));
  

• } putc ('\n');

#endif

@@ -873,6 +902,104 @@ int do_sdram ( cmd_tbl_t *cmdtp, int fl } #endif /* CFG_CMD_SDRAM */

+#if defined(CONFIG_I2C_CMD_TREE) +#if defined(CONFIG_I2C_MULTI_BUS) +int do_i2c_bus_num(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) +{

• int bus_idx, ret=0;
• if (argc == 1) /* querying current setting */
• {

• printf("Current bus is %d\n", i2c_get_bus_num());
  

• }
• else
• {

That should be

} else {

• bus_idx = simple_strtoul(argv[1], NULL, 10);
  

• printf("Setting bus to %d\n", bus_idx);
  

• ret = i2c_set_bus_num(bus_idx);
  

• if(ret)
  

• {
  

• 	printf("Failure changing bus number (%d)\n", ret);
  

• }
  

Single line statements don't have braces.

And so on...

Please "talk" with Kim on how to clean up this patch.

Thanks.

Best regards, Stefan