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

[Wolfgang Denk]

In message <1164940274.5921.26.camel at 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_