[U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) board support

[Zang Roy-r61911 <tie-fei.zang@freescale.com>]

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