From mboxrd@z Thu Jan 1 00:00:00 1970 From: Timur Tabi Date: Mon, 27 Nov 2006 16:45:12 -0600 Subject: [U-Boot-Users] Please pull u-boot-83xx.git In-Reply-To: <200611261446.24607.sr@denx.de> References: <20061103201138.1d7fe056.kim.phillips@freescale.com> <200611261446.24607.sr@denx.de> Message-ID: <456B6A78.1080705@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 at denx.de. >> * >> @@ -28,6 +30,10 @@ >> * >> * 20050101: Eran Liberty (liberty at freescale.com) >> * Initial file creating (porting from 85XX & 8260) >> + * 20060601: Dave Liu (daveliu at freescale.com) >> + * DDR ECC support >> + * unify variable names for 83xx >> + * code cleanup > > Please remove the change history. > >> */ >> >> #include >> @@ -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). > > > >> @@ -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(). -- Timur Tabi Linux Kernel Developer @ Freescale