Re: [U-Boot-Users] Please pull u-boot-83xx.git

Stefan Roese wrote:

• 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 will remove the changelog comments from all the files in cpu/mpc83xx.

• 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.

I see the brace is already on the correct line. I think some of the style errors you're seeing have already been fixed in a later commit that's also part of our 83xx tree.

...

• if (spd.config == 0x02) {

• printf(" with ECC\n");
  

• }

No brace here.

I will go through all of spd_sdram.c and remove the braces from all one-line if-else statements and make sure the remaining braces are on the right lines.

...

+ulong get_ddr_clk(ulong dummy) +{

• return gd->ddr_clk;

+}

Hmmm. Is this function really needed?

I will delete it.

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

I don't get these warnings, but I think I fixed the problem. You will need to tell me if they still show up.

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

I don't get these warnings either, but I don't see how you could. Perhaps you're not using the top of our tree? All of these variables are initialized by the call to ucc_get-cmxucr_reg().