From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Denk Date: Fri, 01 Dec 2006 09:59:38 +0100 Subject: [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) board support In-Reply-To: Your message of "01 Dec 2006 10:31:14 +0800." <1164940274.5921.26.camel@localhost.localdomain> Message-ID: <20061201085938.5E2AB353B0E@atlas.denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de In message <1164940274.5921.26.camel@localhost.localdomain> you wrote: > > > 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. Thanks. > > 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 :-). Thanks. > > 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. As far as I can tell your code is mostly a verbatim copy of post/memory.c - if you need a good memory test then please configure POST on your system and use the POST code instead of copying it. > > 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. But why do we need both CFG_CONFIG_BUS_CLK and CFG_BUS_CLK ? > > 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 :-). Anyway: please don;t define MAC addresses in the board config file. It will only cause harm. > > 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. There may be a *few* boards that do this, but in general it's a very bad idea: it works fine for the guy who is eorking on the U-Boot port, because he usually uses just a single board. But as soon as you have a second board in the net it becomes a major PITA. Please don't do it. If needed, you can set up a valid per-board network setup as part of the software initialization - a simple expect script can do magic. > > 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 Why do you need to do this? > gd->reloc_off to search the exception table. > If I do not find better method for this, I will add a detailed comment > here. I think this part of the code is pretty generic. I would like to understand why on your board such a change is necessary which is not needed on any other system. > Thanks for your effort to review my code :-). You are welcome. 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 In any group of employed individuals the only naturally early riser is _always_ the office manager, who will _always_ leave reproachful little notes ... on the desks of their subordinates. - Terry Pratchett, _Lords and Ladies_