[U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) board support
Zang Roy-r61911
tie-fei.zang at freescale.com
Fri Dec 1 03:31:14 CET 2006
Dear Wolfgang
On Mon, 2006-11-27 at 23:49, Wolfgang Denk wrote:
> Hello,
>
> in message <1164593553.28194.2.camel at 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
More information about the U-Boot
mailing list