From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zang Roy-r61911 Date: 01 Dec 2006 10:31:14 +0800 Subject: [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) board support In-Reply-To: <20061127154901.CE3E7353B57@atlas.denx.de> References: <20061127154901.CE3E7353B57@atlas.denx.de> Message-ID: <1164940274.5921.26.camel@localhost.localdomain> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Dear Wolfgang On Mon, 2006-11-27 at 23:49, Wolfgang Denk wrote: > Hello, > > in message <1164593553.28194.2.camel@localhost.localdomain> you wrote: > > > > How about the progress of mpc7448hpc2 board in u-boot? > > I had a look at your code. > > I see a few problems: > > 1) it does not merge cleanly with the current top of tree in the git > repo; there are conflicts with common/cfi_flash.c Fixed. > > 2) there are lots of coding style violations: I checked all my commit files and fixed all the coding style violations as far as my understand :-). Sorry for some garbage codes :-). > 5) board/mpc7448hpc2/mpc7448hpc2.c contains yet another memory test. > Do we really need another copy of this code? Do you mean testdram () function? I can see this function in many other boards. memtest command can do dram test. While I still think ,this function can benefit end users when testing or debugging. > > 6) Some files - like mpc7448hpc2/tsi108_init.c - contain deep magic > with little or no comments (see for example function > board_early_init_f()). I guess you want to add some more comments > here and there... I will add necessary comments. > > 7) Can you please try keeping the look and feel we have on most other > boards? Fixed. > > 8) Your code adds some data structures globally (like > hid1_7447A_multipliers_x_10[] in cpu/74xx_7xx/speed.c) which are > probably not needed for all processors. Maybe you can use some > #ifdef's here to prevent adding lots of dead code / data to most > board configurations? I will consider to add a ifdef here. > > 9) The login here looks weird to me - is this correct? > cpu/74xx_7xx/speed.c: > ... > +#ifdef CFG_CONFIG_BUS_CLK > + gd->bus_clk = get_board_bus_clk(); > +#else > + gd->bus_clk = CFG_BUS_CLK; > +#endif #ifdef CFG_CONFIG_BUS_CLK the bus clock can be configured by external switch, just as the mpc7448hpc2 board #else the bus clock is a fixed one. I will add comments here. > > 10) Please keep your line length within the allowed limits. Fixed as far as I can see :-). > > 11) Please don't define CONFIG_ETHADDR / CONFIG_ETH1ADDR in your board > config file. It is really evil when all boards have the same MAC > addresses. Also, are the addresses you used officially assigned > ones? 00:06:D2 is officially assigned to Tundra :-). > > Same is for CONFIG_IPADDR, CONFIG_SERVERIP, CONFIG_NETMASK, > CONFIG_GATEWAYIP - it may save some time to have these set during > development, but for a public source version I don't ever want to > see these. Can you make sure all of these define are not necessary? I can see them in many other board. > > 12) In lib_ppc/extable.c you add code with a "#ifdef > CFG_EXCEPTION_AFTER_RELOCATE; there is absolutely no explanation > nor comment anywhere why you think this is necessary. I need to deal with exception after the code relocation. I need add the gd->reloc_off to search the exception table. If I do not find better method for this, I will add a detailed comment here. > Please clean up and resubmit. I have cleaned most of the code, after a testing. I will resubmit them hopefully the early next week. Thanks for your effort to review my code :-). Roy