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

Wolfgang Denk
2006-12-01T08:59:38Z

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_