From: Wolfgang Denk <wd@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) board support
Date: Fri, 01 Dec 2006 09:59:38 +0100 [thread overview]
Message-ID: <20061201085938.5E2AB353B0E@atlas.denx.de> (raw)
In-Reply-To: Your message of "01 Dec 2006 10:31:14 +0800." <1164940274.5921.26.camel@localhost.localdomain>
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_
next prev parent reply other threads:[~2006-12-01 8:59 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-08-11 10:44 [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) board support Zang Roy-r61911
2006-10-30 7:11 ` [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) boardsupport Zang Roy-r61911
2006-10-30 9:01 ` Wolfgang Denk
2006-10-30 9:23 ` Zang Roy-r61911
2006-11-03 4:54 ` Zang Roy-r61911
2006-11-07 2:32 ` [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2(Taiga) board support Zang Roy-r61911
2006-11-13 4:02 ` [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) boardsupport Zang Roy-r61911
2006-11-13 6:12 ` Jaksa David
2006-11-13 6:36 ` Zang Roy-r61911
2006-11-27 2:12 ` [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) board support Zang Roy-r61911
2006-11-27 15:49 ` Wolfgang Denk
2006-11-28 1:51 ` Zang Roy-r61911
2006-12-01 2:31 ` Zang Roy-r61911
2006-12-01 8:59 ` Wolfgang Denk [this message]
2006-12-01 11:23 ` Zang Roy-r61911
2006-12-01 8:09 ` Zang Roy-r61911
2006-12-01 14:31 ` Wolfgang Denk
2006-12-04 2:04 ` Zang Roy-r61911
2006-12-05 2:31 ` Zang Roy-r61911
2006-12-06 5:32 ` Jaksa David
2006-12-08 15:51 ` Zang Roy-r61911
2006-12-14 4:50 ` Zang Roy-r61911
2006-12-20 2:25 ` Jaksa David
2006-12-22 8:36 ` Zang Roy-r61911
2006-12-22 10:02 ` Wolfgang Denk
2006-12-23 11:59 ` Zang Roy-r61911
2007-01-08 2:13 ` Zang Roy-r61911
2007-01-15 2:43 ` Zang Roy-r61911
2007-01-15 21:21 ` Wolfgang Denk
2007-01-23 1:58 ` Zang Roy-r61911
2007-01-29 1:50 ` Zang Roy-r61911
2007-02-12 3:24 ` Zang Roy-r61911
2007-03-08 10:45 ` Wolfgang Denk
2007-03-09 2:03 ` Zang Roy-r61911
2007-03-09 7:40 ` Wolfgang Denk
2007-04-06 1:22 ` Zang Roy-r61911
2007-04-11 23:25 ` Zang Roy-r61911
2007-04-18 15:33 ` Wolfgang Denk
2007-04-18 23:44 ` Zang Roy-r61911
2006-11-02 9:27 ` [U-Boot-Users] [PATCH 00/10 v3]: " Zang Roy-r61911
2006-11-08 8:10 [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2(Taiga)board support Ch.Vict
2006-11-08 8:31 ` Zang Roy-r61911
2006-11-08 8:53 ` Ch.Vict
2006-11-10 7:37 ` Ch.Vict
2006-11-10 7:45 ` [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2(Taiga) board support Zang Roy-r61911
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20061201085938.5E2AB353B0E@atlas.denx.de \
--to=wd@denx.de \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.