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:
- it does not merge cleanly with the current top of tree in the git repo; there are conflicts with common/cfi_flash.c
Fixed.
- 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 :-).
- 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.
- 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.
- Can you please try keeping the look and feel we have on most other boards?
Fixed.
- 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.
- 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.
- Please keep your line length within the allowed limits.
Fixed as far as I can see :-).
- 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.
- 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